From 5a10600a93d35781d59b978d6b1c0f850ff43292 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Wed, 22 Jul 2026 17:57:22 +0300 Subject: [PATCH] =?UTF-8?q?feat:=20service.Archive=20=E2=80=94=20one=20acc?= =?UTF-8?q?essor,=20one=20tree=20walk,=20one=20link=20graph?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit web/ and mcpsrv/ both needed a *doc.Archive and each invented its own way to get one. web/ handed sp.Repo to doc.Scan — reaching past service/ into gitx, which the layering rule forbids — and then listed the documents a second time for their bodies, two tree walks per page view. mcpsrv/ converted []service.Document back into []gitx.Document, rebuilding hashes service/ had already stringified. Two workarounds around one missing accessor is how three agent-facing surfaces stop being identical. Service.Archive resolves the revision, walks the tree once, and returns the archive with the bodies. ArchiveFrom is the same construction over a Document set a caller already holds — the one place a hex object name is converted back to a hash, malformed ids refused rather than zeroed. doc.Page.Links and Page.WordCount were documented as "filled in by a render pass" and nothing filled them, so Archive.Backlinks always returned empty; web/ worked around that by re-rendering every document of the space on each page view. doc.Archive.LinkPass now owns the pass and the accessor runs it, so the link graph exists wherever the archive does. web/ and mcpsrv/ no longer import gitx or go-git, in tests either. doc.DirOf replaces the three copies of the same fromDir helper. --- doc/archive.go | 53 ++++++++++++++++++ doc/archive_test.go | 54 ++++++++++++++++++ doc/doc.go | 5 +- mcpsrv/backend.go | 70 ++++++----------------- mcpsrv/mcpsrv_test.go | 31 ++++++----- search/extract.go | 7 +-- service/archive.go | 97 ++++++++++++++++++++++++++++++++ service/archive_test.go | 108 ++++++++++++++++++++++++++++++++++++ service/doc.go | 8 ++- service/fixture_test.go | 4 +- service/integration_test.go | 6 +- service/push_test.go | 24 ++++---- service/read_test.go | 14 ++--- service/reconcile_test.go | 2 +- web/handlers.go | 45 ++------------- web/reader.go | 31 +++-------- web/web_test.go | 42 +++++++++----- 17 files changed, 420 insertions(+), 181 deletions(-) create mode 100644 service/archive.go create mode 100644 service/archive_test.go diff --git a/doc/archive.go b/doc/archive.go index 85571edb4913897e16ca8ec4e43bb59d5bf2797f..fceb10a007ad78d51a901f3851a254912ad31047 100644 --- a/doc/archive.go +++ b/doc/archive.go @@ -1,6 +1,8 @@ package doc import ( + "errors" + "fmt" "net/url" "path" "regexp" @@ -179,9 +181,60 @@ func (a *Archive) Roots() []*Page { return out } +// LinkPass fills in Page.Links and Page.WordCount for every document of the +// archive, by rendering each body against the archive itself. +// +// It is a second pass rather than part of Scan because links come out of a +// render, not out of a frontmatter parse: a wikilink inside a fenced code block +// is not a link, and deciding that needs the markdown AST. It is here rather +// than in a caller because Archive.Backlinks reads exactly what this writes — +// left to each surface, one of them renders the revision twice a page view and +// the next one silently reports no backlinks at all. +// +// bodies holds each page's raw markdown, frontmatter included, keyed by +// Page.Path — the map a caller already has from the same tree walk that built +// the archive. A page with no body is an inconsistency between the two and is +// reported rather than skipped: skipping it would drop that document's outbound +// links and under-report backlinks everywhere else, invisibly. +// +// The archive resolves the links, so every href produced here is the plain, +// unpinned site path. A caller rendering for display wraps the resolver to +// carry its own ?rev=; that wrapper must not be used here, or the link graph +// would depend on how the reader arrived. +func (a *Archive) LinkPass(r *Renderer, bodies map[string][]byte) error { + if r == nil { + return errors.New("doc: link pass needs a renderer") + } + for _, p := range a.Pages { + raw, ok := bodies[p.Path] + if !ok { + return fmt.Errorf("doc: %s is in the archive of %s at %s but has no body", + p.Path, a.Space, a.Rev) + } + _, body := ParseFront(raw) + res := r.Render(body, DirOf(p.Path), a) + p.Links = res.LinkedIDs + p.WordCount = res.WordCount + } + return nil +} + +// DirOf is the directory a document lives in, space-relative, with "" for the +// space root — the shape Resolve expects as fromDir. +func DirOf(p string) string { + d := path.Dir(p) + if d == "." || d == "/" { + return "" + } + return d +} + // Backlinks returns documents that link to id. Catalog and log documents are // skipped: they link to nearly everything, so counting them would make every // document look referenced and orphan detection would never return a result. +// +// It reads Page.Links, which LinkPass fills: an archive that has not been +// through one has no link graph, and every document looks unreferenced. func (a *Archive) Backlinks(id string) []*Page { var out []*Page for _, p := range a.Pages { diff --git a/doc/archive_test.go b/doc/archive_test.go index c81463a459b20940788f1b18c99382d5af156374..cc2ddb8d5e6b49bd1f2d588e6603c32fa7ca8a13 100644 --- a/doc/archive_test.go +++ b/doc/archive_test.go @@ -179,6 +179,60 @@ func TestHrefsAreSpaceScopedAndEscaped(t *testing.T) { } } +// The link graph is what Backlinks reads, and nothing but LinkPass writes it. +func TestLinkPassFillsLinksAndWordCount(t *testing.T) { + files := map[string]string{ + "specs/storage.md": "---\nid: SPEC-0001\ntitle: Storage\n---\n\n# Storage\n\nfour words of prose\n", + "specs/review.md": "---\nid: SPEC-0002\ntitle: Review\n---\n\n" + + "Supersedes [[SPEC-0001]], and again [[SPEC-0001]].\n\n" + + "```\n[[SPEC-0009]]\n```\n", + "notes/root.md": "See [[SPEC-0001]] from a note.\n", + } + arc := archiveOf(t, files) + bodies := make(map[string][]byte, len(files)) + for p, body := range files { + bodies[p] = []byte(body) + } + + // Before the pass there is no graph at all — the state every caller of + // Backlinks was silently in. + if got := arc.Backlinks("SPEC-0001"); len(got) != 0 { + t.Fatalf("Backlinks before LinkPass = %v, want none", got) + } + + if err := arc.LinkPass(NewRenderer(), bodies); err != nil { + t.Fatalf("LinkPass: %v", err) + } + + review := mustPage(t, arc, "specs/review.md") + // Deduped, and the fenced [[SPEC-0009]] is code, not a link. + if len(review.Links) != 1 || review.Links[0] != "SPEC-0001" { + t.Errorf("review links = %v, want [SPEC-0001]", review.Links) + } + if mustPage(t, arc, "specs/storage.md").WordCount != 5 { + t.Errorf("storage word count = %d, want 5", mustPage(t, arc, "specs/storage.md").WordCount) + } + + var backPaths []string + for _, p := range arc.Backlinks("SPEC-0001") { + backPaths = append(backPaths, p.Path) + } + if len(backPaths) != 2 || backPaths[0] != "notes/root.md" || backPaths[1] != "specs/review.md" { + t.Errorf("backlinks = %v, want notes/root.md and specs/review.md", backPaths) + } +} + +// A page with no body is the archive and the bodies disagreeing, which cannot +// happen while both come off one tree walk. Reported, never skipped: a skip +// drops that document's outbound links and under-reports backlinks elsewhere. +func TestLinkPassRefusesAMissingBody(t *testing.T) { + arc := archiveOf(t, map[string]string{"notes/a.md": "# A\n"}) + err := arc.LinkPass(NewRenderer(), map[string][]byte{}) + if err == nil || !strings.Contains(err.Error(), "notes/a.md") { + t.Fatalf("LinkPass with no bodies = %v, want an error naming the document", err) + } +} + func TestBacklinksIgnoreCatalogsAndLogs(t *testing.T) { arc := archiveOf(t, map[string]string{ "notes/a.md": "# A\n", diff --git a/doc/doc.go b/doc/doc.go index fd0b8f5a5de7c08cad1ee329917f1982f8938e5c..8f308a12537d87fb849a29372a87b8e2eaf1798c 100644 --- a/doc/doc.go +++ b/doc/doc.go @@ -98,10 +98,11 @@ type Page struct { // parent (not including the document itself). Crumbs []string `json:"crumbs,omitempty"` // Links holds the IDs of documents this one links to, deduplicated, in - // order of first appearance. Filled in by a render pass, not by Scan. + // order of first appearance. Filled in by [Archive.LinkPass], not by Scan: + // links come out of a render, not out of a frontmatter parse. Links []string `json:"links,omitempty"` // WordCount is an approximate word count of the rendered text. Filled in by - // a render pass, not by Scan. + // [Archive.LinkPass], from the same render. WordCount int `json:"word_count,omitempty"` } diff --git a/mcpsrv/backend.go b/mcpsrv/backend.go index 57d2b2e8e0ff732ec0cdeff62247f1d8cd50651b..b3a90a45462f8e1654beb8a9513dca31d0d9bb7b 100644 --- a/mcpsrv/backend.go +++ b/mcpsrv/backend.go @@ -6,11 +6,8 @@ import ( "fmt" "strings" - "github.com/go-git/go-git/v5/plumbing" - "sourcecraft.dev/bigbes/sr-ht-spec/core" "sourcecraft.dev/bigbes/sr-ht-spec/doc" - "sourcecraft.dev/bigbes/sr-ht-spec/gitx" "sourcecraft.dev/bigbes/sr-ht-spec/search" "sourcecraft.dev/bigbes/sr-ht-spec/service" ) @@ -18,10 +15,10 @@ import ( // Reader is the part of the orchestration layer the read tools call. // *service.Service satisfies it. // -// It is deliberately four methods. Every one of them is the same function the +// It is deliberately three methods. Every one of them is the same function the // REST handlers, the GraphQL resolvers and the web UI call, which is what the // design means by "the MCP tools call the same resolver layer, not a parallel -// implementation" — a fifth method here that service/ does not have would be +// implementation" — a fourth method here that service/ does not have would be // the beginning of a second implementation. // // ReadDocument is absent on purpose. Addressing a document by id needs the @@ -31,8 +28,11 @@ import ( type Reader interface { ListSpaces(ctx context.Context) ([]*service.Space, error) OpenSpace(ctx context.Context, ref core.SpaceRef) (*service.Space, error) - ResolveRev(ctx context.Context, sp *service.Space, rev string) (string, error) - ListDocuments(ctx context.Context, sp *service.Space, rev string) ([]service.Document, error) + // Archive resolves rev and returns the space's addressable document set at + // it, with every document's bytes, from one tree walk. It replaced a + // ResolveRev/ListDocuments pair here, and the conversion back into git + // documents this package used to perform to reach doc.FromDocuments. + Archive(ctx context.Context, sp *service.Space, rev string) (*doc.Archive, map[string][]byte, error) } // Searcher is the query side of the one global index. *search.Index satisfies @@ -102,15 +102,14 @@ func parseRev(s string) (string, error) { return rev, nil } -// archiveAt reads a space at a revision and builds the addressable document -// set, returning it together with the bodies keyed by path and the resolved -// commit. +// archiveAt reads a space at a revision and returns its addressable document +// set, the bodies keyed by path, and the resolved commit. // -// The revision is resolved first and everything below is read at the resolved -// sha, never at the caller's string. That is what makes an unpinned read -// pinnable: the rev reported back names the exact bytes returned, so a merge -// landing between the resolve and the read cannot make one answer describe two -// revisions. +// service/ does the work: it resolves the revision first and reads at the +// resolved sha, never at the caller's string. That is what makes an unpinned +// read pinnable — the rev reported back names the exact bytes returned, so a +// merge landing between the resolve and the read cannot make one answer +// describe two revisions. // // The whole space is read for one document. At the confirmed volume — tens of // documents a day — that is cheap, and the alternative is worse: doc.Archive is @@ -118,48 +117,11 @@ func parseRev(s string) (string, error) { // actually lives, and it is built from a revision's whole document set. A // path-only fast path would be a second addressing implementation. func archiveAt(ctx context.Context, b Backend, sp *service.Space, rev string) (*doc.Archive, map[string][]byte, string, error) { - resolved, err := b.Docs.ResolveRev(ctx, sp, rev) - if err != nil { - return nil, nil, "", err - } - docs, err := b.Docs.ListDocuments(ctx, sp, resolved) - if err != nil { - return nil, nil, "", err - } - converted, err := toGitDocuments(docs) + arc, bodies, err := b.Docs.Archive(ctx, sp, rev) if err != nil { return nil, nil, "", err } - arc := doc.FromDocuments(sp.Ref, resolved, converted) - bodies := make(map[string][]byte, len(docs)) - for _, d := range docs { - bodies[d.Path] = d.Data - } - return arc, bodies, resolved, nil -} - -// toGitDocuments converts service/'s hex-object-name documents back into the -// shape doc.FromDocuments takes. -// -// This conversion should not exist, and it is the one place this package -// reaches below service/. service.Document carries Blob and Rev as hex strings -// precisely so that api/, mcpsrv/, graph/ and web/ need not import gitx — but -// doc.FromDocuments, which owns the addressing rule those surfaces have to -// apply, takes []gitx.Document. Until service/ exposes the archive itself, -// something above it has to bridge the two, and doing it here in four lines is -// better than restating the addressing rule in Go. A malformed sha is an -// error, not a zero hash: plumbing.NewHash silently yields the zero value for -// anything it cannot parse, and a document whose render-cache key is zero is a -// cache collision waiting to happen. -func toGitDocuments(docs []service.Document) ([]gitx.Document, error) { - out := make([]gitx.Document, 0, len(docs)) - for _, d := range docs { - if !plumbing.IsHash(d.Blob) { - return nil, fmt.Errorf("document %q carries a malformed blob id %q", d.Path, d.Blob) - } - out = append(out, gitx.Document{Path: d.Path, Blob: plumbing.NewHash(d.Blob), Data: d.Data}) - } - return out, nil + return arc, bodies, arc.Rev, nil } // resolvePage applies the design's addressing rule to one caller-supplied diff --git a/mcpsrv/mcpsrv_test.go b/mcpsrv/mcpsrv_test.go index 75c06163c8a9983f9d5a96d6514995b68b8869f7..422fccc3fd9e7b44bda5e2c36919bd4ec0f9a9a1 100644 --- a/mcpsrv/mcpsrv_test.go +++ b/mcpsrv/mcpsrv_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/doc" "sourcecraft.dev/bigbes/sr-ht-spec/mcpsrv" "sourcecraft.dev/bigbes/sr-ht-spec/search" "sourcecraft.dev/bigbes/sr-ht-spec/service" @@ -68,7 +69,23 @@ func (f *fakeReader) OpenSpace(_ context.Context, ref core.SpaceRef) (*service.S return nil, fmt.Errorf("%w: space %s", service.ErrNotFound, ref) } -func (f *fakeReader) ResolveRev(_ context.Context, _ *service.Space, rev string) (string, error) { +// Archive is the one read the tools make. It mirrors what service/ does — +// resolve the revision first, read at the resolved sha, build the archive out +// of the result — and builds it through service.ArchiveFrom, so the addressing +// rule under test is the production one rather than a fixture's idea of it. +func (f *fakeReader) Archive(_ context.Context, sp *service.Space, rev string) (*doc.Archive, map[string][]byte, error) { + resolved, err := f.resolveRev(rev) + if err != nil { + return nil, nil, err + } + if f.listErr != nil { + return nil, nil, f.listErr + } + f.lastRevs = append(f.lastRevs, resolved) + return service.ArchiveFrom(sp.Ref, resolved, f.revs[resolved]) +} + +func (f *fakeReader) resolveRev(rev string) (string, error) { if rev == service.ApprovedRev { return f.approved, nil } @@ -78,18 +95,6 @@ func (f *fakeReader) ResolveRev(_ context.Context, _ *service.Space, rev string) return rev, nil } -func (f *fakeReader) ListDocuments(_ context.Context, _ *service.Space, rev string) ([]service.Document, error) { - if f.listErr != nil { - return nil, f.listErr - } - f.lastRevs = append(f.lastRevs, rev) - docs, ok := f.revs[rev] - if !ok { - return nil, fmt.Errorf("%w: revision %q", service.ErrNotFound, rev) - } - return docs, nil -} - type fakeSearcher struct { last search.Query calls int diff --git a/search/extract.go b/search/extract.go index a220949c4335e797b6fa9453efc6414ff3581706..f55973f5a9c3105872e721233c2e700adc360849 100644 --- a/search/extract.go +++ b/search/extract.go @@ -2,7 +2,6 @@ package search import ( "fmt" - "path" "strings" "sync" @@ -62,11 +61,7 @@ func Extract(arc *doc.Archive, bodies map[string][]byte) ([]Document, error) { return nil, fmt.Errorf("search: no body supplied for %s in %s", p.Path, arc.Space) } front, body := doc.ParseFront(src) - dir := path.Dir(p.Path) - if dir == "." { - dir = "" - } - res := r.Render(body, dir, arc) + res := r.Render(body, doc.DirOf(p.Path), arc) d := Document{ Space: arc.Space, diff --git a/service/archive.go b/service/archive.go new file mode 100644 index 0000000000000000000000000000000000000000..ebcd24d50dac0df5b3cbaa14b2adadf30dced2b1 --- /dev/null +++ b/service/archive.go @@ -0,0 +1,97 @@ +package service + +import ( + "context" + "fmt" + "sync" + + "github.com/go-git/go-git/v5/plumbing" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/doc" + "sourcecraft.dev/bigbes/sr-ht-spec/gitx" +) + +// archiveRenderer is shared across calls: doc.Renderer is documented as +// reusable and concurrency-safe, and building one per archive would rebuild the +// whole goldmark pipeline on every read. search/ shares one the same way. +var archiveRenderer = sync.OnceValue(doc.NewRenderer) + +// Archive builds a space's addressable document set at a revision, together +// with every document's bytes, from one tree walk. +// +// This is the read every surface above this layer needs: the archive owns the +// design's addressing rule (a document's id when it is well-formed and unique +// in the space, otherwise its path), resolves the [[wikilinks]] between +// documents, and carries the link graph backlinks are read from. Before it +// existed, web/ scanned the git tree itself — reaching past this layer into +// gitx, which the layering rule forbids — and mcpsrv/ converted this package's +// documents back into git ones. Two workarounds around one missing accessor is +// how three agent-facing surfaces stop being behaviourally identical. +// +// The revision is resolved first and the documents are read at the resolved +// sha, so the archive and the bodies are the same revision by construction: a +// merge landing mid-read cannot make one answer describe two revisions. The +// resolved sha is the archive's Rev, and is what a caller hands back as the +// pinned ?rev=. +// +// bodies is keyed by tree path — the key doc.Page.Path carries — and holds the +// whole document, frontmatter included. +func (s *Service) Archive(ctx context.Context, sp *Space, rev string) (*doc.Archive, map[string][]byte, error) { + commit, resolved, err := s.resolveRev(ctx, sp, rev) + if err != nil { + return nil, nil, err + } + docs, err := sp.Repo.ListDocuments(ctx, resolved) + if err != nil { + return nil, nil, readErr(err, "list documents at %s in %s", resolved, sp.Ref) + } + return archiveOf(sp.Ref, commit.String(), docs) +} + +// ArchiveFrom builds the same archive out of documents this package already +// returned, without touching git. +// +// It is the seam for a caller that holds a [Document] set — a test double +// standing in for this service, a future surface that has already listed a +// revision — and it exists so that the one conversion from a hex object name +// back to a git hash lives here rather than in each of them. A malformed sha is +// an error, not a zero hash: plumbing.NewHash silently yields the zero value +// for anything it cannot parse, and a document whose render-cache key is zero +// is a cache collision waiting to happen. +// +// rev must be the resolved commit the documents were read at, for the same +// reason [Service.Archive] resolves before it reads. +func ArchiveFrom(sp core.SpaceRef, rev string, docs []Document) (*doc.Archive, map[string][]byte, error) { + converted := make([]gitx.Document, 0, len(docs)) + for _, d := range docs { + if !plumbing.IsHash(d.Blob) { + return nil, nil, fmt.Errorf("service: document %q carries a malformed blob id %q", d.Path, d.Blob) + } + converted = append(converted, gitx.Document{Path: d.Path, Blob: plumbing.NewHash(d.Blob), Data: d.Data}) + } + return archiveOf(sp, rev, converted) +} + +// archiveOf assembles the archive, the bodies and the link graph out of one +// revision's documents. +// +// The link pass runs here rather than at each caller. doc.Page.Links is what +// Archive.Backlinks reads, and an archive handed out without it looks like a +// space where nothing links to anything — which is what every surface saw +// until web/ grew its own pass and re-rendered the revision on each page view. +// Filling it once, in the accessor, is the difference between a link graph and +// a per-surface convention. It costs one render of each document; at the +// confirmed volume — tens of documents a day — that is the same walk's data +// being parsed once more, not a second read. +func archiveOf(sp core.SpaceRef, rev string, docs []gitx.Document) (*doc.Archive, map[string][]byte, error) { + arc := doc.FromDocuments(sp, rev, docs) + bodies := make(map[string][]byte, len(docs)) + for _, d := range docs { + bodies[d.Path] = d.Data + } + if err := arc.LinkPass(archiveRenderer(), bodies); err != nil { + return nil, nil, fmt.Errorf("service: archive of %s at %s: %w", sp, rev, err) + } + return arc, bodies, nil +} diff --git a/service/archive_test.go b/service/archive_test.go new file mode 100644 index 0000000000000000000000000000000000000000..ff5cb90fdbc9ba7f95cc8fc012d9d0cd2c80bfd8 --- /dev/null +++ b/service/archive_test.go @@ -0,0 +1,108 @@ +package service + +import ( + "context" + "testing" +) + +// The accessor every surface above this layer reads a space through: one call, +// the addressing rule applied, the bodies alongside, and the link graph filled +// in. web/ used to scan the git tree itself and mcpsrv/ used to convert this +// package's documents back into git ones; both are gone because this exists. +func TestArchiveReturnsDocumentsBodiesAndLinks(t *testing.T) { + svc, root := newService(t) + sp := newSpace(t, root, 1) + ctx := context.Background() + + head := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "Supersedes [[SPEC-0003]]."), + "specs/0003-old.md": mdDoc("SPEC-0003", "Older sketch", "The first attempt."), + "notes/plain.md": []byte("# Just a note\n\nNo frontmatter at all.\n"), + }) + + arc, bodies, err := svc.Archive(ctx, sp, ApprovedRev) + if err != nil { + t.Fatalf("Archive: %v", err) + } + + // The revision is resolved before anything is read, so the archive names + // the commit it describes and a caller can pin to it. + if arc.Rev != head.String() { + t.Errorf("archive Rev = %s, want the resolved approved head %s", arc.Rev, head) + } + if arc.Space != sp.Ref { + t.Errorf("archive space = %s, want %s", arc.Space, sp.Ref) + } + if len(arc.All()) != 3 || len(bodies) != 3 { + t.Fatalf("archive holds %d documents and %d bodies, want 3 of each", len(arc.All()), len(bodies)) + } + + // The addressing rule: a well-formed unique id addresses the document, and + // a document without one is addressed by its path. + page, ok := arc.Page("SPEC-0007") + if !ok || page.Path != "specs/0007-storage.md" { + t.Fatalf("SPEC-0007 = %+v, ok=%v", page, ok) + } + if _, ok := arc.Page("notes/plain"); !ok { + t.Error("a document with no frontmatter must be addressed by its path") + } + + // Bodies are the whole document, keyed the way the archive keys pages. + if body, ok := bodies[page.Path]; !ok || !contains(body, "Supersedes") { + t.Errorf("body of %s = %q", page.Path, body) + } + + // The link graph: filled here, so Backlinks answers rather than always + // returning nothing. + if len(page.Links) != 1 || page.Links[0] != "SPEC-0003" { + t.Errorf("links of SPEC-0007 = %v, want [SPEC-0003]", page.Links) + } + if page.WordCount == 0 { + t.Error("word count is filled by the same pass and must not be zero") + } + back := arc.Backlinks("SPEC-0003") + if len(back) != 1 || back[0].ID != "SPEC-0007" { + t.Fatalf("Backlinks(SPEC-0003) = %v, want SPEC-0007", back) + } +} + +// A pinned revision and the approved head are the same call with a different +// revision string, and the read contract still applies: a branch name is not a +// revision this method will serve. +func TestArchiveHonoursTheReadContract(t *testing.T) { + svc, root := newService(t) + sp := newSpace(t, root, 1) + ctx := context.Background() + + first := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "first"), + }) + commitFiles(t, sp, sp.ApprovedBranch(), 2, map[string][]byte{ + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "second"), + }) + + _, bodies, err := svc.Archive(ctx, sp, first.String()) + if err != nil { + t.Fatalf("pinned Archive: %v", err) + } + if !contains(bodies["specs/0007-storage.md"], "first") { + t.Errorf("pinned archive returned %q, want the pinned revision", bodies["specs/0007-storage.md"]) + } + + cutBranch(t, sp, "proposals/1", sp.ApprovedBranch()) + if _, _, err := svc.Archive(ctx, sp, "proposals/1"); err == nil { + t.Fatal("Archive served a branch name; the read plane takes object names only") + } +} + +// ArchiveFrom is the same construction over documents a caller already holds, +// and it refuses a document whose blob id is not an object name rather than +// keying the render cache on a zero hash. +func TestArchiveFromRefusesAMalformedBlobID(t *testing.T) { + _, _, err := ArchiveFrom(fxSpace, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", []Document{ + {Path: "specs/0007-storage.md", Blob: "not-a-sha", Data: mdDoc("SPEC-0007", "Storage model", "body")}, + }) + if err == nil { + t.Fatal("ArchiveFrom accepted a malformed blob id") + } +} diff --git a/service/doc.go b/service/doc.go index 635427e3dd00a909ac721a6639ccae036056fc48..1de11a489829179c69cd6ae6fff48c06562aeb48 100644 --- a/service/doc.go +++ b/service/doc.go @@ -17,9 +17,11 @@ // - Space lifecycle: [Service.CreateSpace], [Service.OpenSpace], // [Service.ListSpaces]. // - Reads: [Service.ReadDocument], [Service.ListDocuments], -// [Service.Policy] — each resolving either the approved head or a pinned -// revision through the same code path, because there is one storage tier -// and no checkout. +// [Service.Archive], [Service.Policy] — each resolving either the approved +// head or a pinned revision through the same code path, because there is +// one storage tier and no checkout. [Service.Archive] is the one every +// surface that shows a space reads through: it applies the addressing rule +// and fills the link graph once, here, rather than in each of them. // - Push validation: [Service.ValidatePush], the function the `update` hook // calls over RPC before a ref moves. // - Repair: [Reconcile] and [Service.RunReconciler]. diff --git a/service/fixture_test.go b/service/fixture_test.go index 46aca7c0e479583cc90bf204050108f9f3e8ca81..db8bd832bb2255ae1053e1fb1d8c35843e754f99 100644 --- a/service/fixture_test.go +++ b/service/fixture_test.go @@ -148,8 +148,8 @@ func cutBranch(t *testing.T, sp *Space, branch, base string) { } } -// doc renders a minimal valid document: the three required keys, then a body. -func doc(id, title, body string) []byte { +// mdDoc renders a minimal valid document: the three required keys, then a body. +func mdDoc(id, title, body string) []byte { return []byte(fmt.Sprintf("---\nid: %s\ntitle: %s\nstatus: draft\n---\n\n%s\n", id, title, body)) } diff --git a/service/integration_test.go b/service/integration_test.go index 33efb5498c092e82840bdfb052f5acfca4e2eb06..e2076db93989649c6205986e65f2e9a0f7cc55f7 100644 --- a/service/integration_test.go +++ b/service/integration_test.go @@ -28,7 +28,7 @@ func TestValidatePushAcceptsAGoodPush(t *testing.T) { t.Fatalf("approved head: %v", err) } head := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "body"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "body"), }) err = svc.ValidatePush(ctx, PushRequest{ @@ -64,7 +64,7 @@ func TestValidatePushRejectsAGlobalIDCollision(t *testing.T) { t.Fatal(err) } head := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "body"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "body"), }) err = svc.ValidatePush(ctx, PushRequest{ @@ -156,7 +156,7 @@ func TestReconcileRepairsTheRepairTable(t *testing.T) { merged := mustOpenProposal(t, svc, sp.ID, "merged", base.String()) cutBranch(t, sp, merged.Branch, sp.ApprovedBranch()) commitFiles(t, sp, merged.Branch, 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "body"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "body"), }) branchHead, err := sp.Repo.BranchHead(ctx, merged.Branch) if err != nil { diff --git a/service/push_test.go b/service/push_test.go index 30991827005c1e580146c5866db158fd80d67062..f37d3a1580a17baa174ab75ef241785ee59befc7 100644 --- a/service/push_test.go +++ b/service/push_test.go @@ -26,14 +26,14 @@ func agent() authn.Principal { func TestValidateDocuments(t *testing.T) { schema := core.DefaultSchema() - spec7 := Document{Path: "specs/0007.md", Blob: "a", Data: doc("SPEC-0007", "Storage", "body")} - spec8 := Document{Path: "specs/0008.md", Blob: "b", Data: doc("SPEC-0008", "Other", "body")} - dupe := Document{Path: "notes/copy.md", Blob: "c", Data: doc("SPEC-0007", "Copy", "body")} + spec7 := Document{Path: "specs/0007.md", Blob: "a", Data: mdDoc("SPEC-0007", "Storage", "body")} + spec8 := Document{Path: "specs/0008.md", Blob: "b", Data: mdDoc("SPEC-0008", "Other", "body")} + dupe := Document{Path: "notes/copy.md", Blob: "c", Data: mdDoc("SPEC-0007", "Copy", "body")} noStatus := Document{Path: "specs/0009.md", Blob: "d", Data: []byte("---\nid: SPEC-0009\ntitle: No status\n---\n\nbody\n")} noFrontmatter := Document{Path: "specs/0010.md", Blob: "e", Data: []byte("# just a heading\n")} badID := Document{Path: "specs/0011.md", Blob: "f", - Data: doc("spec-11", "Lowercase id", "body")} + Data: mdDoc("spec-11", "Lowercase id", "body")} tests := []struct { name string @@ -232,14 +232,14 @@ func TestCheckRefsRule(t *testing.T) { ctx := context.Background() first := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "one"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "one"), }) second := commitFiles(t, sp, sp.ApprovedBranch(), 2, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "two"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "two"), }) cutBranch(t, sp, "proposals/1", sp.ApprovedBranch()) branch := commitFiles(t, sp, "proposals/1", 3, map[string][]byte{ - "specs/0008.md": doc("SPEC-0008", "Other", "one"), + "specs/0008.md": mdDoc("SPEC-0008", "Other", "one"), }) tests := []struct { @@ -297,12 +297,12 @@ func TestChangedDocumentsBaselinesANewBranchOnTheApprovedHead(t *testing.T) { ctx := context.Background() commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "one"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "one"), "specs/0009.md": []byte("not a document at all\n"), }) cutBranch(t, sp, "proposals/1", sp.ApprovedBranch()) branch := commitFiles(t, sp, "proposals/1", 2, map[string][]byte{ - "specs/0008.md": doc("SPEC-0008", "Other", "one"), + "specs/0008.md": mdDoc("SPEC-0008", "Other", "one"), }) all, changed, err := svc.changedDocuments(ctx, sp, zeroHash(t), branch) @@ -323,11 +323,11 @@ func TestChangedDocumentsUsesTheOldRefValueWhenThereIsOne(t *testing.T) { ctx := context.Background() old := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "one"), - "specs/0008.md": doc("SPEC-0008", "Other", "one"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "one"), + "specs/0008.md": mdDoc("SPEC-0008", "Other", "one"), }) newHead := commitFiles(t, sp, sp.ApprovedBranch(), 2, map[string][]byte{ - "specs/0008.md": doc("SPEC-0008", "Other", "two"), + "specs/0008.md": mdDoc("SPEC-0008", "Other", "two"), }) _, changed, err := svc.changedDocuments(ctx, sp, old, newHead) diff --git a/service/read_test.go b/service/read_test.go index 7ecc2bdd7a6705de7297cbd9111765597775ab81..4e78a1132b17a9d68640c4cc15811c566da7d726 100644 --- a/service/read_test.go +++ b/service/read_test.go @@ -15,10 +15,10 @@ func TestReadDocumentResolvesTheApprovedHeadByDefault(t *testing.T) { ctx := context.Background() first := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007-storage.md": doc("SPEC-0007", "Storage model", "first"), + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "first"), }) second := commitFiles(t, sp, sp.ApprovedBranch(), 2, map[string][]byte{ - "specs/0007-storage.md": doc("SPEC-0007", "Storage model", "second"), + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "second"), }) got, err := svc.ReadDocument(ctx, sp, ApprovedRev, "specs/0007-storage.md") @@ -57,11 +57,11 @@ func TestReadDocumentDoesNotServeAProposalByDefault(t *testing.T) { ctx := context.Background() commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007-storage.md": doc("SPEC-0007", "Storage model", "approved"), + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "approved"), }) cutBranch(t, sp, "proposals/1", sp.ApprovedBranch()) commitFiles(t, sp, "proposals/1", 2, map[string][]byte{ - "specs/0007-storage.md": doc("SPEC-0007", "Storage model", "draft"), + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage model", "draft"), }) approved, err := svc.ReadDocument(ctx, sp, ApprovedRev, "specs/0007-storage.md") @@ -118,8 +118,8 @@ func TestListDocumentsReturnsEveryDocumentAtARevision(t *testing.T) { ctx := context.Background() commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007-storage.md": doc("SPEC-0007", "Storage", "body"), - "notes/scratch.md": doc("NOTE-0001", "Scratch", "body"), + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage", "body"), + "notes/scratch.md": mdDoc("NOTE-0001", "Scratch", "body"), "attachment.png": []byte("\x89PNG not a document"), core.PolicyFile: []byte("review:\n auto_merge: [notes/**]\n"), }) @@ -204,7 +204,7 @@ func TestResolveRevPinsTheApprovedHead(t *testing.T) { sp := newSpace(t, root, 1) head := commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007-storage.md": doc("SPEC-0007", "Storage", "body"), + "specs/0007-storage.md": mdDoc("SPEC-0007", "Storage", "body"), }) got, err := svc.ResolveRev(context.Background(), sp, ApprovedRev) if err != nil { diff --git a/service/reconcile_test.go b/service/reconcile_test.go index 6d80e2b9ab6824f3dd031a09068a7e6871d76d38..a8db492da47dc769c874311eec5121ce63c68dcd 100644 --- a/service/reconcile_test.go +++ b/service/reconcile_test.go @@ -231,7 +231,7 @@ func TestDeleteProposalRefRemovesOnlyProposalBranches(t *testing.T) { ctx := context.Background() commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ - "specs/0007.md": doc("SPEC-0007", "Storage", "one"), + "specs/0007.md": mdDoc("SPEC-0007", "Storage", "one"), }) cutBranch(t, sp, "proposals/1", sp.ApprovedBranch()) diff --git a/web/handlers.go b/web/handlers.go index 5dcbbdf7cc7d478ffe1f6e8eb13d30b877ac898c..8f1769c3061dea20ba4d9a0bac9bea29e0e1f4fd 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -7,7 +7,6 @@ import ( "html/template" "log" "net/http" - "path" "strings" "github.com/go-chi/chi/v5" @@ -424,13 +423,10 @@ func (s *Server) handleDocument(w http.ResponseWriter, r *http.Request) { return } - // Backlinks need the whole revision's link graph, and doc.Page.Links is - // filled by a render pass rather than by Scan — so the pass happens here. - if err := s.linkPass(snap); err != nil { - s.fail(w, r, err) - return - } - res := s.renderer.Render(mdBody, dirOf(docPath), pinned{inner: snap.Archive, rq: rq}) + // The link graph backlinks are read from is filled in by the reader, at the + // same revision as the archive itself. This handler used to render every + // document of the space here to build it, once per page view. + res := s.renderer.Render(mdBody, doc.DirOf(docPath), pinned{inner: snap.Archive, rq: rq}) data := docData{ SpaceRef: ref.String(), @@ -497,29 +493,6 @@ func (p pinned) Resolve(fromDir, dest string) doc.Target { return t } -// linkPass renders every document of the revision to fill in doc.Page.Links, -// which is what Archive.Backlinks reads. Scan deliberately does not do it — -// links come out of a render, not out of a frontmatter parse. -// -// A page with no body is an inconsistency between the archive and the bodies, -// which cannot happen while both are read at one resolved sha. It is reported -// rather than skipped: skipping it would silently drop that document's outbound -// links and quietly under-report backlinks everywhere else. -func (s *Server) linkPass(snap *Snapshot) error { - for _, p := range snap.Archive.All() { - raw, ok := snap.Bodies[p.Path] - if !ok { - return fmt.Errorf("web: %s is in the archive of %s at %s but has no body", - p.Path, snap.Ref, snap.Rev) - } - _, mdBody := doc.ParseFront(raw) - res := s.renderer.Render(mdBody, dirOf(p.Path), snap.Archive) - p.Links = res.LinkedIDs - p.WordCount = res.WordCount - } - return nil -} - // ---- search --------------------------------------------------------------- type searchHit struct { @@ -637,13 +610,3 @@ func revQuery(rev string) string { } return "?rev=" + rev } - -// dirOf is the directory a document lives in, space-relative, with "" for the -// space root — the shape doc's resolver expects. -func dirOf(p string) string { - d := path.Dir(p) - if d == "." || d == "/" { - return "" - } - return d -} diff --git a/web/reader.go b/web/reader.go index 99852ccc38326a18ac7cac1ba04354a803b3a933..f907531f19561b58054185255c73eac476f55116 100644 --- a/web/reader.go +++ b/web/reader.go @@ -2,7 +2,6 @@ package web import ( "context" - "fmt" "sourcecraft.dev/bigbes/sr-ht-spec/core" "sourcecraft.dev/bigbes/sr-ht-spec/doc" @@ -18,10 +17,9 @@ import ( // a merge landing mid-request cannot make one page describe two revisions — // and the sha it resolved to is the value the page offers as its permalink. // -// Bodies is keyed by tree path, the same key doc.Page.Path carries. It is -// present because doc.Page.Links is filled by a render pass rather than by -// Scan, so backlinks require rendering every document in the space; see -// linkPass. +// Bodies is keyed by tree path, the same key doc.Page.Path carries. It comes +// off the same tree walk as the archive, so a document listed here and a +// document in the archive are the same bytes at the same revision. type Snapshot struct { Ref core.SpaceRef Rev string @@ -78,26 +76,15 @@ func (r serviceReader) Snapshot(ctx context.Context, ref core.SpaceRef, rev stri if err != nil { return nil, err } - resolved, err := r.svc.ResolveRev(ctx, sp, rev) + // One call, one tree walk, and the link graph filled in. This package used + // to scan the git tree itself and then list the documents a second time for + // their bodies — two walks per page view, and a reach past service/ into + // gitx that the layering rule forbids. + arc, bodies, err := r.svc.Archive(ctx, sp, rev) if err != nil { return nil, err } - // Both reads below are issued against the resolved sha rather than against - // the caller's rev, so the archive and the bodies are the same revision by - // construction and cannot disagree if a merge lands between them. - arc, err := doc.Scan(ctx, sp.Repo, ref, resolved) - if err != nil { - return nil, fmt.Errorf("web: scan %s at %s: %w", ref, resolved, err) - } - docs, err := r.svc.ListDocuments(ctx, sp, resolved) - if err != nil { - return nil, err - } - bodies := make(map[string][]byte, len(docs)) - for _, d := range docs { - bodies[d.Path] = d.Data - } - return &Snapshot{Ref: ref, Rev: resolved, Archive: arc, Bodies: bodies}, nil + return &Snapshot{Ref: ref, Rev: arc.Rev, Archive: arc, Bodies: bodies}, nil } func (r serviceReader) ReadDocument(ctx context.Context, ref core.SpaceRef, rev, p string) (service.Document, error) { diff --git a/web/web_test.go b/web/web_test.go index 3fb79b0e491ce6b3dcfea84c5487c6ce800703e7..b188bdef1fc9e4663e10a8e251171ae7a39cae2c 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -3,7 +3,9 @@ package web import ( "context" "crypto/rand" + "crypto/sha1" "encoding/base64" + "encoding/hex" "encoding/json" "fmt" "net/http" @@ -14,14 +16,11 @@ import ( "testing" "github.com/fernet/fernet-go" - "github.com/go-git/go-git/v5/plumbing" "github.com/vaughan0/go-ini" "sourcecraft.dev/bigbes/sr-ht-core/crypto" "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/core" - "sourcecraft.dev/bigbes/sr-ht-spec/doc" - "sourcecraft.dev/bigbes/sr-ht-spec/gitx" "sourcecraft.dev/bigbes/sr-ht-spec/search" "sourcecraft.dev/bigbes/sr-ht-spec/service" ) @@ -150,23 +149,36 @@ func (f *fakeReader) Snapshot(_ context.Context, ref core.SpaceRef, rev string) } sort.Strings(paths) - gd := make([]gitx.Document, 0, len(paths)) - bodies := make(map[string][]byte, len(paths)) + // Built through service.ArchiveFrom, exactly as serviceReader gets it: the + // addressing rule and the link graph under test are the production ones, + // not a fixture's idea of them. + sd := make([]service.Document, 0, len(paths)) for _, p := range paths { data := []byte(docs[p]) - gd = append(gd, gitx.Document{ + sd = append(sd, service.Document{ Path: p, - Blob: plumbing.ComputeHash(plumbing.BlobObject, data), + Blob: blobSha(data), + Rev: resolved, Data: data, }) - bodies[p] = data } - return &Snapshot{ - Ref: ref, - Rev: resolved, - Archive: doc.FromDocuments(ref, resolved, gd), - Bodies: bodies, - }, nil + arc, bodies, err := service.ArchiveFrom(ref, resolved, sd) + if err != nil { + return nil, err + } + return &Snapshot{Ref: ref, Rev: resolved, Archive: arc, Bodies: bodies}, nil +} + +// blobSha is git's object name for a blob: sha1 over "blob \0" and the +// content. Spelled out rather than taken from go-git so that this package — +// including its tests — never imports the git layer, which is the layering rule +// the Reader interface exists to keep. +func blobSha(data []byte) string { + h := sha1.New() + fmt.Fprintf(h, "blob %d", len(data)) + h.Write([]byte{0}) + h.Write(data) + return hex.EncodeToString(h.Sum(nil)) } func (f *fakeReader) ReadDocument(_ context.Context, ref core.SpaceRef, rev, p string) (service.Document, error) { @@ -180,7 +192,7 @@ func (f *fakeReader) ReadDocument(_ context.Context, ref core.SpaceRef, rev, p s } return service.Document{ Path: p, - Blob: plumbing.ComputeHash(plumbing.BlobObject, []byte(content)).String(), + Blob: blobSha([]byte(content)), Rev: resolved, Data: []byte(content), }, nil