From 7f779fef12194d49b9ce97ad4e2a80af1c3d6358 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 23 Jul 2026 09:51:51 +0300 Subject: [PATCH] =?UTF-8?q?feat(web):=20review=20queue=20=E2=80=94=20inbox?= =?UTF-8?q?=20+=20policy-merged=20digest=20(Phase=204)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backstop for work no link reached. /inbox lists every open proposal on the instance ("waiting on you") and, below it, the digest of recently policy-merged content — the firehose a human sees after the fact, which is the whole reason approval=policy is kept distinct from human. - service.InboxProposals / DigestProposals list instance-wide (one reviewer, so a per-space inbox would make them hunt), mapping each stored proposal's space_id back to a reference once from the space list. - web/inbox.go + inbox.html render the two sections; the landing page links the queue for a logged-in owner. Follow-up: the digest currently shows recent policy-merges rather than "since you last looked" — the digest_mark table exists to track that, but advancing it is a write and GET stays pure. Filed separately. --- service/review.go | 59 ++++++++++++++++++++++++++++++ service/review_test.go | 55 ++++++++++++++++++++++++++++ web/inbox.go | 79 ++++++++++++++++++++++++++++++++++++++++ web/inbox_test.go | 68 ++++++++++++++++++++++++++++++++++ web/reader.go | 14 +++++++ web/router.go | 1 + web/templates.go | 2 +- web/templates/inbox.html | 53 +++++++++++++++++++++++++++ web/templates/index.html | 1 + web/web_test.go | 22 +++++++++++ 10 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 web/inbox.go create mode 100644 web/inbox_test.go create mode 100644 web/templates/inbox.html diff --git a/service/review.go b/service/review.go index e252c925381feac98e1aff3e891db9020ab258d2..617ad463d79c19d59adcb867fca2630b2fe22700 100644 --- a/service/review.go +++ b/service/review.go @@ -94,6 +94,65 @@ func bytesEqual(a, b []byte) bool { return true } +// InboxProposals is every open proposal on the instance, newest first — the +// reviewer's queue, "N proposals waiting on you". It is instance-wide because +// there is one reviewer: a per-space inbox would make them visit each space to +// find what a link never reached them about. +// +// The digest and this share the mapping from a stored proposal's space_id back +// to a reference, done once from the space list rather than a lookup per row. +func (s *Service) InboxProposals(ctx context.Context) ([]Proposal, error) { + return s.proposalsInState(ctx, core.StateOpen, false, 0) +} + +// DigestProposals is the recently policy-merged proposals — the firehose +// digest. Auto-merged content never stops for review, so this is where a human +// sees it after the fact; the design's whole reason for keeping approval=policy +// distinct from human is so this list can exist. limit bounds it; <= 0 is a +// sane default. +func (s *Service) DigestProposals(ctx context.Context, limit int) ([]Proposal, error) { + if limit <= 0 { + limit = 20 + } + return s.proposalsInState(ctx, core.StateMerged, true, limit) +} + +// proposalsInState lists proposals in one state instance-wide, optionally +// keeping only the policy-approved ones (the digest), and maps each onto its +// space reference. A row whose space no longer lists is skipped rather than +// errored: a deleted space takes its proposals out of every human-facing view, +// and a dangling row is the reconciler's to notice, not this read's to fail on. +func (s *Service) proposalsInState(ctx context.Context, state core.ProposalState, policyOnly bool, limit int) ([]Proposal, error) { + spaces, err := s.ListSpaces(ctx) + if err != nil { + return nil, err + } + refByID := make(map[int]core.SpaceRef, len(spaces)) + for _, sp := range spaces { + refByID[sp.ID] = sp.Ref + } + + rows, err := s.store.ListProposalsByState(ctx, state, 0) + if err != nil { + return nil, fmt.Errorf("service: list %s proposals: %w", state, err) + } + out := make([]Proposal, 0, len(rows)) + for _, p := range rows { + if policyOnly && p.Approval != core.ApprovalPolicy { + continue + } + ref, ok := refByID[p.SpaceID] + if !ok { + continue + } + out = append(out, proposalView(p, ref)) + if limit > 0 && len(out) == limit { + break + } + } + return out, nil +} + // MergeHuman lands a proposal on the owner's approval — the review page's // approve button. It is Merge with the approval kind fixed, so the surface does // not choose it: a browser approve is always human, and a caller that could pass diff --git a/service/review_test.go b/service/review_test.go index c170eaaaf2a0dd9c6583b8f2dc8efc79d2c0c3c8..84cad3915b86591d95d65af0aad56885a5492075 100644 --- a/service/review_test.go +++ b/service/review_test.go @@ -4,8 +4,63 @@ import ( "bytes" "context" "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" ) +// TestInboxAndDigest proves the instance-wide queues: an open proposal lands in +// the inbox, a policy-auto-merged one lands in the digest, and neither shows in +// the other. +func TestInboxAndDigest(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + // A policy that auto-merges notes/, so one proposal lands and one waits. + commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + ".spec.yml": []byte("review:\n auto_merge: [notes/**]\n"), + }) + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + + // Auto-merges (notes/). + auto, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, Principal: agentPrincipal(), Title: "note", IfMatch: base.String(), + Message: "note", Writes: []DocumentWrite{{Path: "notes/a.md", Content: mdDoc("N-1", "A", "b")}}, + }) + if err != nil || !auto.Merged { + t.Fatalf("auto-merge propose: merged=%v err=%v", auto.Merged, err) + } + // Stays open (specs/). + open, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, Principal: agentPrincipal(), Title: "spec", IfMatch: base.String(), + Message: "spec", Writes: []DocumentWrite{{Path: "specs/b.md", Content: mdDoc("S-1", "B", "c")}}, + }) + if err != nil || open.Merged { + t.Fatalf("open propose: merged=%v err=%v", open.Merged, err) + } + + inbox, err := svc.InboxProposals(ctx) + if err != nil { + t.Fatalf("InboxProposals: %v", err) + } + if len(inbox) != 1 || inbox[0].ID != open.Proposal.ID { + t.Fatalf("inbox = %+v, want only the open proposal %d", inbox, open.Proposal.ID) + } + + digest, err := svc.DigestProposals(ctx, 0) + if err != nil { + t.Fatalf("DigestProposals: %v", err) + } + if len(digest) != 1 || digest[0].ID != auto.Proposal.ID || digest[0].Approval != core.ApprovalPolicy { + t.Fatalf("digest = %+v, want only the policy-merged proposal %d", digest, auto.Proposal.ID) + } +} + // TestProposalDiffReturnsChangedDocuments proves ProposalDiff returns exactly // the documents a proposal changes — a modified one with its base and proposed // content, and an added one marked new — and not the documents it leaves alone. diff --git a/web/inbox.go b/web/inbox.go new file mode 100644 index 0000000000000000000000000000000000000000..0817f190d3be0a232fc57f6ce88e0de7153a8784 --- /dev/null +++ b/web/inbox.go @@ -0,0 +1,79 @@ +package web + +import ( + "net/http" + "strconv" + + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// inboxData is the review-queue page: the open proposals waiting on the owner, +// and the digest of what auto-merged without stopping for review. +type inboxData struct { + Open []proposalRow + Digest []proposalRow +} + +// proposalRow is one proposal as a listing line: enough to decide whether to +// open it, and the link that does. +type proposalRow struct { + Href string + ID int + Space string + Title string + Agent string + Approval string +} + +// handleInbox renders the review queue: every open proposal on the instance, +// plus the digest of recently policy-merged content. It is the backstop the +// design describes — the link an agent hands you is the normal way in, and this +// catches the work no link reached. +func (s *Server) handleInbox(w http.ResponseWriter, r *http.Request) { + if !mayRead(r) { + s.loginRedirect(w, r) + return + } + open, err := s.reader.Inbox(r.Context()) + if err != nil { + s.fail(w, r, err) + return + } + digest, err := s.reader.Digest(r.Context()) + if err != nil { + s.fail(w, r, err) + return + } + + vd := s.chrome(r) + vd.Title = "Review queue" + vd.Data = inboxData{ + Open: proposalRows(open), + Digest: proposalRows(digest), + } + s.render(w, http.StatusOK, "inbox", vd) +} + +// proposalRows turns service proposals into listing rows, building each one's +// link from its space and id — the same stable /~owner/space/p/ shape the +// write plane hands back. +func proposalRows(ps []service.Proposal) []proposalRow { + rows := make([]proposalRow, 0, len(ps)) + for _, p := range ps { + rows = append(rows, proposalRow{ + Href: proposalHref(p), + ID: p.ID, + Space: p.Space.String(), + Title: p.Title, + Agent: p.Agent, + Approval: string(p.Approval), + }) + } + return rows +} + +// proposalHref is the review-page link for a proposal: the same path the +// proposal URL uses, minus the origin, so it works as a relative link in the UI. +func proposalHref(p service.Proposal) string { + return "/" + p.Space.String() + "/p/" + strconv.Itoa(p.ID) +} diff --git a/web/inbox_test.go b/web/inbox_test.go new file mode 100644 index 0000000000000000000000000000000000000000..8bd739b70def389c9ed07ca8d4c69d4c382509af --- /dev/null +++ b/web/inbox_test.go @@ -0,0 +1,68 @@ +package web + +import ( + "net/http" + "strings" + "testing" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" + "sourcecraft.dev/bigbes/sr-ht-spec/service" +) + +// TestInboxListsOpenAndDigest proves the review queue lists open proposals under +// "waiting on you" and policy-merged ones under the digest, each linking to its +// review page. +func TestInboxListsOpenAndDigest(t *testing.T) { + r := newFakeReader() + seedProposal(r, service.Proposal{ + ID: 3, Space: demoSpace, Title: "Open one", State: core.StateOpen, + Agent: "claude-code/a", + }, nil) + seedProposal(r, service.Proposal{ + ID: 4, Space: demoSpace, Title: "Auto-merged one", State: core.StateMerged, + Approval: core.ApprovalPolicy, Agent: "claude-code/b", + }, nil) + // A human-merged proposal is neither waiting nor part of the digest. + seedProposal(r, service.Proposal{ + ID: 5, Space: demoSpace, Title: "Human-merged", State: core.StateMerged, + Approval: core.ApprovalHuman, + }, nil) + h, _, _ := testServerWith(t, r) + + rec := get(t, h, "/inbox", "bigbes") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body:\n%s", rec.Code, rec.Body) + } + body := rec.Body.String() + if !strings.Contains(body, "/~bigbes/rfcs/p/3") || !strings.Contains(body, "Open one") { + t.Errorf("open proposal missing from the queue; body:\n%s", body) + } + if !strings.Contains(body, "/~bigbes/rfcs/p/4") || !strings.Contains(body, "Auto-merged one") { + t.Errorf("policy-merged proposal missing from the digest; body:\n%s", body) + } + if strings.Contains(body, "Human-merged") { + t.Errorf("a human-merged proposal leaked into the review queue") + } +} + +// TestInboxEmpty proves the page renders with no proposals rather than erroring. +func TestInboxEmpty(t *testing.T) { + h, _, _ := testServerWith(t, newFakeReader()) + rec := get(t, h, "/inbox", "bigbes") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if !strings.Contains(rec.Body.String(), "queue is clear") { + t.Errorf("empty inbox does not say the queue is clear") + } +} + +// TestInboxAnonymousRedirected proves a viewer with no read authority is sent to +// login rather than shown the queue. +func TestInboxAnonymousRedirected(t *testing.T) { + h, _, _ := testServerWith(t, newFakeReader()) + rec := get(t, h, "/inbox", "") + if rec.Code != http.StatusSeeOther && rec.Code != http.StatusFound { + t.Fatalf("status = %d, want a login redirect", rec.Code) + } +} diff --git a/web/reader.go b/web/reader.go index 5511e21696ab1109695d3af7bc90f8fe551671cb..d5d17cc301e5b86e4aaa1fe65e1b18481073fdcc 100644 --- a/web/reader.go +++ b/web/reader.go @@ -67,6 +67,12 @@ type Reader interface { // stands. Approve(ctx context.Context, ref core.SpaceRef, id int) (service.Proposal, error) Reject(ctx context.Context, ref core.SpaceRef, id int) (service.Proposal, error) + + // Inbox is every open proposal on the instance, newest first — the review + // queue. Digest is the recently policy-merged proposals, the firehose a human + // sees after the fact. + Inbox(ctx context.Context) ([]service.Proposal, error) + Digest(ctx context.Context) ([]service.Proposal, error) } // Searcher is the keyword index. *search.Index satisfies it as declared. @@ -136,3 +142,11 @@ func (r serviceReader) Approve(ctx context.Context, ref core.SpaceRef, id int) ( func (r serviceReader) Reject(ctx context.Context, ref core.SpaceRef, id int) (service.Proposal, error) { return r.svc.Reject(ctx, ref, id) } + +func (r serviceReader) Inbox(ctx context.Context) ([]service.Proposal, error) { + return r.svc.InboxProposals(ctx) +} + +func (r serviceReader) Digest(ctx context.Context) ([]service.Proposal, error) { + return r.svc.DigestProposals(ctx, 0) +} diff --git a/web/router.go b/web/router.go index 0f9328c287ca08554f2ec2de786a209ff2be7273..28e421f97118e9bde0e4743b6e353a09c4d599f7 100644 --- a/web/router.go +++ b/web/router.go @@ -40,6 +40,7 @@ func (s *Server) Register(r chi.Router) { r.Get("/healthz", s.handleHealthz) r.Get("/static/*", s.handleStatic) r.Get("/search", s.handleSearch) + r.Get("/inbox", s.handleInbox) // The proposal routes are registered before the document wildcard. chi gives // the static "p" segment priority over the "*" catch-all regardless, but diff --git a/web/templates.go b/web/templates.go index ff9e8226d7f6c5148d15a4c842ae4403fe8eec15..f9ff1b3911cf469c833d758b04ac413ca5c136c9 100644 --- a/web/templates.go +++ b/web/templates.go @@ -46,7 +46,7 @@ var funcMap = template.FuncMap{ } // pageNames are the content templates; each is parsed with layout.html. -var pageNames = []string{"index", "space", "document", "search", "error", "proposal"} +var pageNames = []string{"index", "space", "document", "search", "error", "proposal", "inbox"} // pages maps a page name to its parsed template set (layout + that page). var pages = func() map[string]*template.Template { diff --git a/web/templates/inbox.html b/web/templates/inbox.html new file mode 100644 index 0000000000000000000000000000000000000000..abba65baf7d11127b25c0f1c2bfe470c772b00eb --- /dev/null +++ b/web/templates/inbox.html @@ -0,0 +1,53 @@ +{{define "content"}} +
+
+

Review queue

+ +

+ Waiting on you + {{len .Data.Open}} +

+ {{if .Data.Open}} + + + + + + {{range .Data.Open}} + + + + + + {{end}} + +
ProposalSpaceAgent
#{{.ID}} — {{.Title}}{{.Space}}{{.Agent}}
+ {{else}} +

No open proposals. Your queue is clear.

+ {{end}} + +

Recently auto-merged

+

+ Content the space's policy merged without stopping for review. +

+ {{if .Data.Digest}} + + + + + + {{range .Data.Digest}} + + + + + + {{end}} + +
ProposalSpaceAgent
#{{.ID}} — {{.Title}}{{.Space}}{{.Agent}}
+ {{else}} +

Nothing has auto-merged recently.

+ {{end}} +
+
+{{end}} diff --git a/web/templates/index.html b/web/templates/index.html index ae41e091736a16150dfe600388b961e5f43f6200..7dee0b2b7c4c03d7a7a7813f5627caa02264d078 100644 --- a/web/templates/index.html +++ b/web/templates/index.html @@ -13,6 +13,7 @@ {{if .Data.LoggedIn}}
+

Review queue — proposals waiting on you.

diff --git a/web/web_test.go b/web/web_test.go index 554080af19f627bccf88b18f2f007c6f5344663f..91a990614f3507a3902efa6078250cbf9a153f13 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -251,6 +251,28 @@ func (f *fakeReader) Reject(_ context.Context, _ core.SpaceRef, id int) (service return p, nil } +func (f *fakeReader) Inbox(_ context.Context) ([]service.Proposal, error) { + var out []service.Proposal + for _, p := range f.proposals { + if p.State == core.StateOpen { + out = append(out, p) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].ID > out[j].ID }) + return out, nil +} + +func (f *fakeReader) Digest(_ context.Context) ([]service.Proposal, error) { + var out []service.Proposal + for _, p := range f.proposals { + if p.State == core.StateMerged && p.Approval == core.ApprovalPolicy { + out = append(out, p) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].ID > out[j].ID }) + return out, nil +} + // fakeSearcher returns one fixed hit whose snippet carries the tags // bleve's highlighter emits. type fakeSearcher struct {