From 3c563e4d3eafe7ead010a894d18b0eb8ab31e6c5 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 23 Jul 2026 09:48:16 +0300 Subject: [PATCH] =?UTF-8?q?feat(web):=20proposal=20review=20page=20?= =?UTF-8?q?=E2=80=94=20prose=20diff=20+=20approve/reject=20(Phase=204)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The browser review plane at /~owner/space/p/, the stable URL every write already returns. The owner opens the link an agent handed them, reads a prose diff of each changed document, and approves (merges now) or rejects. - web/diff.go: the prose-diff HTML renderer, consuming prosediff's block model (the package renders text only; HTML is the web layer's job). It implements the Phase 0 verdict's hard requirement — inline word diffs above 0.75 block similarity, a two-column old/new view below it, because 13% of real edits shred and are unreadable inline. All document content is HTML-escaped; only the diff structure is markup. - service/review.go: ProposalDiff reads each changed document's base and proposed content for the page to diff (branch tip resolved to a sha, the legitimate pinned-rev read, not the ReadDocumentAtRef bypass), and MergeHuman fixes the approval kind so a browser approve is always human. - web/proposal.go: the GET page and the approve/reject POSTs. Only the owner may act (an agent is authenticated but has no more approval authority than anyone); a cross-site guard on Origin/Referer is the CSRF defense a form post needs when the session cookie is meta's. Post- redirect-get back to the page. Stale/already-merged approve → 409. - web.Reader gains the proposal reads and the two actions; the diff-view styles go in scss/main.scss (inline marks, two-column, code line diffs). Inbox and the policy-merged digest are the remaining Phase 4 surfaces. --- scss/main.scss | 149 ++++++++++++++++++++++ service/review.go | 103 ++++++++++++++++ service/review_test.go | 77 ++++++++++++ web/diff.go | 239 ++++++++++++++++++++++++++++++++++++ web/diff_internal_test.go | 90 ++++++++++++++ web/handlers.go | 11 ++ web/proposal.go | 203 ++++++++++++++++++++++++++++++ web/proposal_test.go | 221 +++++++++++++++++++++++++++++++++ web/reader.go | 48 +++++++- web/router.go | 8 ++ web/templates.go | 2 +- web/templates/proposal.html | 64 ++++++++++ web/web_test.go | 71 ++++++++++- 13 files changed, 1277 insertions(+), 9 deletions(-) create mode 100644 service/review.go create mode 100644 service/review_test.go create mode 100644 web/diff.go create mode 100644 web/diff_internal_test.go create mode 100644 web/proposal.go create mode 100644 web/proposal_test.go create mode 100644 web/templates/proposal.html diff --git a/scss/main.scss b/scss/main.scss index 4082c216728dbda04309ac07b9c6b3a18ef47c66..cc50c75fd77d0b735d35b7611a5032aaeac6fabb 100644 --- a/scss/main.scss +++ b/scss/main.scss @@ -89,3 +89,152 @@ color: $gray-300; } } + +// ---- Proposal review ------------------------------------------------------ + +.proposal-actions { + margin: 1rem 0; + + form { + margin-right: 0.5rem; + } +} + +.proposal-doc { + margin-top: 1.5rem; + padding-top: 1rem; + border-top: 1px solid $gray-300; + + @media (prefers-color-scheme: dark) { + border-top-color: $gray-700; + } +} + +// ---- Prose diff ----------------------------------------------------------- +// +// The renderer (web/diff.go) emits this markup: a hunk per heading path, a +// block per change, and inline / spans or a two-column view depending +// on how badly the block was rewritten. Colours follow Bootstrap's success and +// danger so added and removed read the same here as everywhere else. + +.prosediff { + font-size: 0.95rem; + + .ph-hunk { + margin-bottom: 1rem; + } + + .ph-path { + font-family: $font-family-monospace; + font-size: 0.8rem; + color: $gray-600; + padding: 0.15rem 0; + + @media (prefers-color-scheme: dark) { + color: $gray-400; + } + } + + .ph-block { + padding: 0.35rem 0.6rem; + margin: 0.2rem 0; + border-left: 3px solid transparent; + border-radius: 2px; + } + + .ph-label { + display: block; + font-size: 0.72rem; + text-transform: uppercase; + letter-spacing: 0.03em; + color: $gray-600; + margin-bottom: 0.15rem; + + @media (prefers-color-scheme: dark) { + color: $gray-400; + } + } + + .ph-insert { + border-left-color: $success; + background: rgba($success, 0.08); + } + + .ph-delete { + border-left-color: $danger; + background: rgba($danger, 0.08); + } + + .ph-modify { + border-left-color: $gray-400; + background: rgba($gray-500, 0.06); + } + + .ph-move { + border-left-color: $info; + color: $gray-600; + font-style: italic; + } + + // Inline marks: a deletion is struck through in danger, an insertion is + // underlined in success. Both keep a faint background so a one-word change is + // visible without reading the colour. + del { + text-decoration: line-through; + color: $danger; + background: rgba($danger, 0.12); + text-decoration-thickness: 1px; + } + + ins { + text-decoration: none; + color: darken($success, 8%); + background: rgba($success, 0.14); + + @media (prefers-color-scheme: dark) { + color: lighten($success, 8%); + } + } + + // The two-column fallback for shredded blocks: old on the left, new on the + // right, stacking on a narrow screen so it never overflows the page. + .ph-cols { + display: flex; + flex-wrap: wrap; + gap: 0.75rem; + } + + .ph-col { + flex: 1 1 20rem; + min-width: 0; + padding: 0.4rem 0.6rem; + border-radius: 2px; + } + + .ph-old { + background: rgba($danger, 0.06); + } + + .ph-new { + background: rgba($success, 0.06); + } + + // Code / frontmatter line diffs keep their wrapping and are read line by line. + .ph-code { + margin: 0; + padding: 0.25rem 0; + background: transparent; + white-space: pre-wrap; + word-break: break-word; + + .ph-line-del { + display: block; + background: rgba($danger, 0.1); + } + + .ph-line-ins { + display: block; + background: rgba($success, 0.1); + } + } +} diff --git a/service/review.go b/service/review.go new file mode 100644 index 0000000000000000000000000000000000000000..e252c925381feac98e1aff3e891db9020ab258d2 --- /dev/null +++ b/service/review.go @@ -0,0 +1,103 @@ +package service + +import ( + "context" + "fmt" + "sort" + + "sourcecraft.dev/bigbes/sr-ht-spec/core" +) + +// ProposalDoc is one document a proposal touches, as the review page needs it: +// its path, the approved content it was based on, and the proposed content on +// the branch. It is the input to the prose diff, which is the web layer's to +// render — this layer reads git and hands over bytes. +type ProposalDoc struct { + // Path is the document's path on the proposal branch. + Path string + + // Base is the document's content at the proposal's base — the approved text + // the change was made against. Nil for a document the proposal adds, which + // is the signal to render it as wholly new rather than as a diff. + Base []byte + + // Proposed is the document's content on the proposal branch. + Proposed []byte + + // New reports whether the document did not exist at the base. + New bool +} + +// ProposalDiff returns every document a proposal changes, each with the base and +// proposed content the review page diffs. +// +// It reads the base and the proposal branch through the normal pinned-revision +// path, not the ReadDocumentAtRef bypass: the branch tip is resolved to a commit +// sha first, and an object name is a legitimate read whatever it points at. The +// bypass exists for reading a branch *by name*; here the review already holds +// the proposal and can pin it. +// +// Only genuinely changed documents are returned — a proposal branch is cut from +// the base, so most of its documents are byte-identical to it and are not diffs. +// A proposal changes only documents (agents cannot rename or delete), so a +// document present at the base is present on the branch; the reverse asymmetry, +// a document added by the proposal, is marked New. +func (s *Service) ProposalDiff(ctx context.Context, p Proposal) ([]ProposalDoc, error) { + sp, err := s.OpenSpace(ctx, p.Space) + if err != nil { + return nil, err + } + + baseDocs, err := s.ListDocuments(ctx, sp, p.BaseRev) + if err != nil { + return nil, fmt.Errorf("service: read base %s of proposal %d: %w", short(p.BaseRev), p.ID, err) + } + base := make(map[string][]byte, len(baseDocs)) + for _, d := range baseDocs { + base[d.Path] = d.Data + } + + head, err := sp.Repo.BranchHead(ctx, p.Branch) + if err != nil { + return nil, readErr(err, "read head of %s in %s", p.Branch, p.Space) + } + branchDocs, err := s.ListDocuments(ctx, sp, head.String()) + if err != nil { + return nil, fmt.Errorf("service: read proposal branch %s: %w", p.Branch, err) + } + + var out []ProposalDoc + for _, d := range branchDocs { + prior, existed := base[d.Path] + switch { + case !existed: + out = append(out, ProposalDoc{Path: d.Path, Proposed: d.Data, New: true}) + case !bytesEqual(prior, d.Data): + out = append(out, ProposalDoc{Path: d.Path, Base: prior, Proposed: d.Data}) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].Path < out[j].Path }) + return out, nil +} + +// bytesEqual reports byte equality. It exists so ProposalDiff does not pull in +// bytes for a single comparison, and reads as intent at the call site. +func bytesEqual(a, b []byte) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// 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 +// ApprovalPolicy here would be able to launder a firehose merge as reviewed. +func (s *Service) MergeHuman(ctx context.Context, ref core.SpaceRef, proposalID int) (Proposal, error) { + return s.Merge(ctx, ref, proposalID, core.ApprovalHuman) +} diff --git a/service/review_test.go b/service/review_test.go new file mode 100644 index 0000000000000000000000000000000000000000..c170eaaaf2a0dd9c6583b8f2dc8efc79d2c0c3c8 --- /dev/null +++ b/service/review_test.go @@ -0,0 +1,77 @@ +package service + +import ( + "bytes" + "context" + "testing" +) + +// 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. +func TestProposalDiffReturnsChangedDocuments(t *testing.T) { + svc, _ := newTestService(t) + ctx := context.Background() + sp, err := svc.CreateSpace(ctx, fxSpace) + if err != nil { + t.Fatalf("CreateSpace: %v", err) + } + // The approved head carries two documents; the proposal edits one, adds a + // third, and leaves the second untouched. + commitFiles(t, sp, sp.ApprovedBranch(), 1, map[string][]byte{ + "specs/a.md": mdDoc("S-1", "A", "original body"), + "specs/keep.md": mdDoc("S-2", "Keep", "unchanged body"), + }) + base, err := sp.Repo.ApprovedHead(ctx) + if err != nil { + t.Fatalf("ApprovedHead: %v", err) + } + res, err := svc.Propose(ctx, ProposeRequest{ + Space: fxSpace, + Principal: agentPrincipal(), + Title: "edit and add", + IfMatch: base.String(), + Message: "two changes", + Writes: []DocumentWrite{ + {Path: "specs/a.md", Content: mdDoc("S-1", "A", "revised body")}, + {Path: "specs/new.md", Content: mdDoc("S-3", "New", "brand new body")}, + }, + }) + if err != nil { + t.Fatalf("Propose: %v", err) + } + + docs, err := svc.ProposalDiff(ctx, res.Proposal) + if err != nil { + t.Fatalf("ProposalDiff: %v", err) + } + byPath := make(map[string]ProposalDoc, len(docs)) + for _, d := range docs { + byPath[d.Path] = d + } + if _, ok := byPath["specs/keep.md"]; ok { + t.Errorf("ProposalDiff returned the untouched specs/keep.md") + } + + edited, ok := byPath["specs/a.md"] + if !ok { + t.Fatalf("ProposalDiff missing the edited document") + } + if edited.New { + t.Errorf("specs/a.md marked new, want an edit") + } + if !bytes.Contains(edited.Base, []byte("original body")) { + t.Errorf("edited doc base = %q, want the approved content", edited.Base) + } + if !bytes.Contains(edited.Proposed, []byte("revised body")) { + t.Errorf("edited doc proposed = %q, want the proposal content", edited.Proposed) + } + + added, ok := byPath["specs/new.md"] + if !ok { + t.Fatalf("ProposalDiff missing the added document") + } + if !added.New || added.Base != nil { + t.Errorf("added doc = %+v, want New with a nil base", added) + } +} diff --git a/web/diff.go b/web/diff.go new file mode 100644 index 0000000000000000000000000000000000000000..00adca59110cc9b64c03905c5cade426de4ab775 --- /dev/null +++ b/web/diff.go @@ -0,0 +1,239 @@ +package web + +import ( + "fmt" + "html/template" + "strings" + + "sourcecraft.dev/bigbes/sr-ht-spec/prosediff" +) + +// inlineSimilarityThreshold is the Phase 0 verdict's presentation switch: a +// modified prose block whose token similarity is at or above it renders as an +// inline word diff, and one below it renders as a two-column old/new view. +// +// 13% of real prose modifications shred into interleaved fragments — those +// paragraphs really were rewritten sentence by sentence — and every one of them +// scores at or below 0.73. Rendering them inline makes one review in eight +// unreadable, which is the one where the agent changed the most. prosediff +// exports BlockChange.Similarity for exactly this decision and computes no HTML +// itself; this is where the decision is made. +const inlineSimilarityThreshold = 0.75 + +// diffView is the whole rendered diff of one document, ready for the proposal +// template. Unchanged reports the degenerate case — a proposal that touches a +// document without changing it — so the page can say so rather than show an +// empty diff. +type diffView struct { + HTML template.HTML + Stats prosediff.Stats + Unchanged bool +} + +// renderDocDiff diffs the approved (old) and proposed (new) source of one +// document and renders it to HTML. +// +// It walks prosediff's block-change model rather than its text renderer: the +// text renderer is for a terminal, and the review page needs headings grouped, +// word edits marked with /, and — the load-bearing part — the +// two-column fallback for shredded blocks. Every piece of document content is +// HTML-escaped before it reaches the output; the only markup this produces is +// its own structure. +func renderDocDiff(oldSrc, newSrc []byte) diffView { + d := prosediff.Compare(oldSrc, newSrc) + view := diffView{Stats: d.Stats, Unchanged: !d.Stats.Changed()} + if view.Unchanged { + return view + } + + var b strings.Builder + b.WriteString(`
`) + lastPath := "\x00" // impossible path, so the first real one always prints + inHunk := false + closeHunk := func() { + if inHunk { + b.WriteString(`
`) + inHunk = false + } + } + + for _, c := range d.Changes { + if c.Kind == prosediff.ChangeEqual { + continue // the review shows only what changed + } + blk := c.New + if blk == nil { + blk = c.Old + } + if path := strings.Join(blk.HeadingPath, " › "); path != lastPath { + closeHunk() + lastPath = path + shown := path + if shown == "" { + shown = "(document preamble)" + } + b.WriteString(`
`) + b.WriteString(template.HTMLEscapeString(shown)) + b.WriteString(`
`) + inHunk = true + } + writeBlock(&b, c) + } + closeHunk() + b.WriteString(`
`) + view.HTML = template.HTML(b.String()) + return view +} + +// writeBlock renders one changed block. Insert/delete/move show the whole +// block; a modify chooses among a code line diff, an inline word diff and the +// two-column view, on the rules the design pins. +func writeBlock(b *strings.Builder, c prosediff.BlockChange) { + switch c.Kind { + case prosediff.ChangeInsert: + writeWholeBlock(b, "ph-insert", "added", c.New) + case prosediff.ChangeDelete: + writeWholeBlock(b, "ph-delete", "removed", c.Old) + case prosediff.ChangeMoveIn: + fmt.Fprintf(b, `
%s moved here (was line %d)
`, + template.HTMLEscapeString(c.New.Label()), c.Old.StartLine) + case prosediff.ChangeMoveOut: + fmt.Fprintf(b, `
%s moved away (now line %d)
`, + template.HTMLEscapeString(c.Old.Label()), c.New.StartLine) + case prosediff.ChangeModify: + writeModify(b, c) + } +} + +// writeWholeBlock renders an inserted or deleted block: its whole text, in a +// 
 for a non-prose kind so code keeps its wrapping, and reflowed prose
+// otherwise.
+func writeWholeBlock(b *strings.Builder, class, verb string, blk *prosediff.Block) {
+	label := verb + " " + blk.Label()
+	fmt.Fprintf(b, `
%s`,
+		class, template.HTMLEscapeString(label))
+	writeBody(b, blk.Text, blk.Kind.Prose())
+	b.WriteString(`
`)
+}
+
+// writeModify renders a modified block. A code, frontmatter or HTML block has a
+// line-oriented edit script and renders line by line; a prose block has a word
+// edit script and renders inline when it stayed similar enough to follow, and
+// two-column when it did not.
+func writeModify(b *strings.Builder, c prosediff.BlockChange) {
+	label := "changed " + c.New.Label()
+	if c.StructureOnly {
+		label = fmt.Sprintf("changed %s → %s (structure)", c.Old.Label(), c.New.Label())
+	}
+	if c.Moved {
+		label += fmt.Sprintf(" (moved from line %d)", c.Old.StartLine)
+	}
+
+	if len(c.Lines) > 0 {
+		fmt.Fprintf(b, `
%s
`,
+			template.HTMLEscapeString(label))
+		writeLineSpans(b, c.Lines)
+		b.WriteString(`
`)
+		return
+	}
+
+	if c.Similarity >= inlineSimilarityThreshold {
+		fmt.Fprintf(b, `
%s
`,
+			template.HTMLEscapeString(label))
+		writeInlineSpans(b, c.Words)
+		b.WriteString(`
`)
+		return
+	}
+
+	// The two-column fallback: the block was rewritten enough that inline marks
+	// would shred it. The old column keeps deletions, the new keeps insertions,
+	// each still marked, so a reviewer reads two coherent paragraphs side by side.
+	fmt.Fprintf(b, `
%s (rewritten)
`,
+		template.HTMLEscapeString(label))
+	writeColumnSpans(b, c.Words, true)
+	b.WriteString(`
`)
+	writeColumnSpans(b, c.Words, false)
+	b.WriteString(`
`)
+}
+
+// writeBody renders whole-block text: escaped, in a 
 when it is not prose.
+func writeBody(b *strings.Builder, text string, prose bool) {
+	if prose {
+		b.WriteString(`
`)
+		b.WriteString(template.HTMLEscapeString(text))
+		b.WriteString(`
`)
+		return
+	}
+	b.WriteString(`
`)
+	b.WriteString(template.HTMLEscapeString(text))
+	b.WriteString(`
`)
+}
+
+// writeInlineSpans renders a word edit script inline, marking deletions and
+// insertions where they sit. The Space flag reproduces prosediff's own spacing:
+// a span that replaces the one before it carries Space=false, so a one-word
+// substitution does not render with a gap in the middle.
+func writeInlineSpans(b *strings.Builder, spans []prosediff.Span) {
+	emitted := false
+	for _, s := range spans {
+		if s.Space && emitted {
+			b.WriteByte(' ')
+		}
+		emitted = true
+		writeSpan(b, s)
+	}
+}
+
+// writeColumnSpans renders one side of the two-column view: the old side keeps
+// equal and deleted words, the new side keeps equal and inserted ones. Both
+// still mark their changes, so each column is a readable paragraph that also
+// shows what moved.
+func writeColumnSpans(b *strings.Builder, spans []prosediff.Span, old bool) {
+	emitted := false
+	for _, s := range spans {
+		keep := s.Op == prosediff.OpEqual ||
+			(old && s.Op == prosediff.OpDelete) ||
+			(!old && s.Op == prosediff.OpInsert)
+		if !keep {
+			continue
+		}
+		if s.Space && emitted {
+			b.WriteByte(' ')
+		}
+		emitted = true
+		writeSpan(b, s)
+	}
+}
+
+// writeSpan writes one span's escaped text, wrapped in  or  for a
+// change and bare for an equal run.
+func writeSpan(b *strings.Builder, s prosediff.Span) {
+	esc := template.HTMLEscapeString(s.Text)
+	switch s.Op {
+	case prosediff.OpDelete:
+		b.WriteString("")
+		b.WriteString(esc)
+		b.WriteString("")
+	case prosediff.OpInsert:
+		b.WriteString("")
+		b.WriteString(esc)
+		b.WriteString("")
+	default:
+		b.WriteString(esc)
+	}
+}
+
+// writeLineSpans renders a line-oriented edit script (a code fence, frontmatter
+// or HTML block) one line per row inside a 
, each line class-marked.
+func writeLineSpans(b *strings.Builder, spans []prosediff.Span) {
+	for _, s := range spans {
+		class := "ph-line-eq"
+		switch s.Op {
+		case prosediff.OpDelete:
+			class = "ph-line-del"
+		case prosediff.OpInsert:
+			class = "ph-line-ins"
+		}
+		fmt.Fprintf(b, `%s`+"\n", class, template.HTMLEscapeString(s.Text))
+	}
+}
diff --git a/web/diff_internal_test.go b/web/diff_internal_test.go
new file mode 100644
index 0000000000000000000000000000000000000000..fe9b7559988731e9c43bcb454e363388d15a62da
--- /dev/null
+++ b/web/diff_internal_test.go
@@ -0,0 +1,90 @@
+package web
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestRenderDocDiffUnchanged proves a proposal that does not change a document
+// is reported as unchanged rather than as an empty diff.
+func TestRenderDocDiffUnchanged(t *testing.T) {
+	src := []byte("# Title\n\nOne paragraph.\n")
+	v := renderDocDiff(src, src)
+	if !v.Unchanged {
+		t.Fatalf("Unchanged = false, want true for identical input")
+	}
+	if v.HTML != "" {
+		t.Errorf("HTML = %q, want empty for an unchanged document", v.HTML)
+	}
+}
+
+// TestRenderDocDiffInlineWordChange proves a small edit renders inline with
+// / marks and not as two columns.
+func TestRenderDocDiffInlineWordChange(t *testing.T) {
+	old := []byte("# Title\n\nThe quick brown fox jumps over the lazy dog.\n")
+	nw := []byte("# Title\n\nThe quick red fox jumps over the lazy dog.\n")
+	v := renderDocDiff(old, nw)
+	if v.Unchanged {
+		t.Fatalf("Unchanged = true, want a change")
+	}
+	html := string(v.HTML)
+	if !strings.Contains(html, "brown") {
+		t.Errorf("missing inline deletion of 'brown'; got:\n%s", html)
+	}
+	if !strings.Contains(html, "red") {
+		t.Errorf("missing inline insertion of 'red'; got:\n%s", html)
+	}
+	if strings.Contains(html, "ph-columns") {
+		t.Errorf("a one-word edit rendered as two columns; got:\n%s", html)
+	}
+}
+
+// TestRenderDocDiffTwoColumnBelowThreshold proves a block rewritten enough to
+// fall below the similarity threshold renders as the two-column old/new view —
+// the Phase 0 verdict's hard requirement.
+func TestRenderDocDiffTwoColumnBelowThreshold(t *testing.T) {
+	// A block rewritten to ~0.58 similarity: paired as a modify (above the 0.40
+	// pairing floor) but shredded enough to fall below the 0.75 inline switch.
+	old := []byte("# Title\n\nThe committee approved the annual budget after a long and " +
+		"contentious debate that lasted well into the evening.\n")
+	nw := []byte("# Title\n\nThe committee rejected the annual budget after a brief and " +
+		"quiet discussion that ended early in the afternoon.\n")
+	v := renderDocDiff(old, nw)
+	html := string(v.HTML)
+	if !strings.Contains(html, "ph-columns") {
+		t.Fatalf("a wholesale rewrite did not render as two columns; got:\n%s", html)
+	}
+	if !strings.Contains(html, "ph-col ph-old") || !strings.Contains(html, "ph-col ph-new") {
+		t.Errorf("two-column view missing an old or new column; got:\n%s", html)
+	}
+}
+
+// TestRenderDocDiffEscapesContent proves document content is HTML-escaped: a
+// document that contains markup cannot inject it into the review page.
+func TestRenderDocDiffEscapesContent(t *testing.T) {
+	old := []byte("# Title\n\nplain text here.\n")
+	nw := []byte("# Title\n\nplain  text here.\n")
+	v := renderDocDiff(old, nw)
+	html := string(v.HTML)
+	if strings.Contains(html, "