From eaa2b65f2b205ccda00b2f09264f6fc966209373 Mon Sep 17 00:00:00 2001
From: bigbes <bigbes@gmail.com>
Date: Wed, 22 Jul 2026 14:46:16 +0300
Subject: [PATCH] fix(service): separate the read-plane rev guard from the
 review path

The previous commit's guard broke two service tests, which were right and
the guard was too broad: rendering and diffing a proposal branch genuinely
needs to read one, so a blanket ban on ref names is not the shape.

ReadDocument keeps the strict object-name rule. ReadDocumentAtRef reaches
an arbitrary ref, deliberately as a separate awkwardly-named method rather
than a flag, so serving unreviewed content is something a caller asks for
by name and a reviewer can grep for.

A rejected rev now wraps ErrNotFound as well, preserving the existing
deliberate property that a crafted revision cannot distinguish malformed
from absent by probing.
---
 service/read.go      | 35 ++++++++++++++++++++++++++++++++++-
 service/read_test.go | 11 +++++++++--
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/service/read.go b/service/read.go
index bd4db419e2800bcf3804c43540d28849c0cc79b7..7e7a35a1e964459367bdecfa366d85b8a32643fe 100644
--- a/service/read.go
+++ b/service/read.go
@@ -146,12 +146,45 @@ func (s *Service) ResolveRev(ctx context.Context, sp *Space, rev string) (string
 // issued against. Re-issuing reads against the resolved hash instead would be
 // one extra object lookup per read for no gain, and would lose the branch name
 // from error messages.
+// ReadDocumentAtRef reads a document at an arbitrary ref, bypassing the read
+// contract's object-name requirement.
+//
+// This is the review path's entry point: rendering and diffing a proposal
+// branch genuinely needs to read one. It is deliberately a separate,
+// awkwardly-named method rather than a flag on ReadDocument, so that serving
+// unreviewed content is something a caller has to ask for by name and a reviewer
+// can grep for — never something a read surface can be talked into by a crafted
+// rev parameter.
+//
+// Do not call this from any surface that answers "read SPEC-0007".
+func (s *Service) ReadDocumentAtRef(ctx context.Context, sp *Space, ref, path string) (Document, error) {
+	if sp == nil || sp.Repo == nil {
+		return Document{}, errors.New("service: space has no open repository")
+	}
+	resolved := ref
+	if resolved == ApprovedRev {
+		resolved = sp.Repo.ApprovedBranch()
+	}
+	commit, err := sp.Repo.ResolveRev(ctx, resolved)
+	if err != nil {
+		return Document{}, readErr(err, "resolve revision %q in %s", resolved, sp.Ref)
+	}
+	d, err := sp.Repo.ReadDocument(ctx, resolved, path)
+	if err != nil {
+		return Document{}, readErr(err, "read %s at %s in %s", path, resolved, sp.Ref)
+	}
+	return Document{Path: d.Path, Blob: d.Blob.String(), Rev: commit.String(), Data: d.Data}, nil
+}
+
 func (s *Service) resolveRev(ctx context.Context, sp *Space, rev string) (plumbing.Hash, string, error) {
 	if sp == nil || sp.Repo == nil {
 		return plumbing.ZeroHash, "", errors.New("service: space has no open repository")
 	}
 	if err := ValidateReadRev(rev); err != nil {
-		return plumbing.ZeroHash, "", err
+		// Wrapped as ErrNotFound so a crafted revision cannot tell "malformed"
+		// from "absent" by probing, matching readErr's existing choice.
+		// ErrBadReadRev stays in the chain for logs and for callers that care.
+		return plumbing.ZeroHash, "", fmt.Errorf("%w: %w", ErrNotFound, err)
 	}
 	resolved := rev
 	if resolved == ApprovedRev {
diff --git a/service/read_test.go b/service/read_test.go
index d0aa915709e48b590cec8c2a440159b186f30af1..7ecc2bdd7a6705de7297cbd9111765597775ab81 100644
--- a/service/read_test.go
+++ b/service/read_test.go
@@ -71,9 +71,16 @@ func TestReadDocumentDoesNotServeAProposalByDefault(t *testing.T) {
 	if !contains(approved.Data, "approved") {
 		t.Fatalf("default read returned the draft: %q", approved.Data)
 	}
-	draft, err := svc.ReadDocument(ctx, sp, "proposals/1", "specs/0007-storage.md")
+	// The read plane refuses a ref name outright — that is the guard that stops
+	// a crafted ?rev= from serving unreviewed text as approved.
+	if _, err := svc.ReadDocument(ctx, sp, "proposals/1", "specs/0007-storage.md"); !errors.Is(err, ErrBadReadRev) {
+		t.Fatalf("ReadDocument accepted a ref name: err = %v, want ErrBadReadRev", err)
+	}
+
+	// The review path reaches it deliberately, by a differently-named method.
+	draft, err := svc.ReadDocumentAtRef(ctx, sp, "proposals/1", "specs/0007-storage.md")
 	if err != nil {
-		t.Fatalf("ReadDocument at the proposal branch: %v", err)
+		t.Fatalf("ReadDocumentAtRef at the proposal branch: %v", err)
 	}
 	if !contains(draft.Data, "draft") {
 		t.Errorf("proposal read returned %q", draft.Data)