fix: match an agent's quote against rendered text

GLOSSARY.md

@@ -108,6 +108,13 @@ This document defines the core domain terms used within the `reviewer` codebase
 ### Anchor Quote
 * **Description**: The passage a thread the agent opened was written against (`anchorQuote`).
 * **Behavior**: Re-resolved to an **Anchor** on every render rather than trusted once — by the server on a diff, by the browser on Markdown, where the `spec-element-N` numbering lives. Several matches take the first; no match leaves the thread without a target, shown under the **About this document** section of the **Feedback Panel** rather than dropped. An empty quote is a question about the document as a whole.
+* **Behavior (Markdown, text form)**: The agent quotes the Markdown source while the page matches against a block's rendered text, so the server derives the **Anchor Quote Text** the browser compares — otherwise a quote carrying `**`, a backtick or a `##` never matched the very block it was copied from.
+* **Implementation**: `AnchorQuote` / `anchorQuote`, `NormalizeAnchorQuote`, `resolveQuoteAnchors()`.
+
+### Anchor Quote Text
+* **Description**: An **Anchor Quote** with its Markdown resolved to the text a rendered block displays (`anchorQuoteText`).
+* **Behavior**: Derived from the **Anchor Quote** on every read of the feedback, never stored — the quote the agent wrote stays the single source of truth, and a **Sidecar** written before this existed resolves its threads too. Markdown only: a diff review resolves its quotes against the diff lines in Go, so the page never reads one there. It is put through the same renderer as the document body, because a second Markdown parser would drift from what is on screen.
+* **Implementation**: `AnchorQuoteText` / `anchorQuoteText`, `NormalizeAnchorQuote`, `withQuoteText`.
 
 ### Change Summary
 * **Description**: The agent's page-level `summary` of the latest round's document changes, rendered in the **Change Summary Block** at the top of the **Feedback Panel**.

anchorquote.go

@@ -0,0 +1,99 @@
+package reviewer
+
+import (
+	"bytes"
+	"regexp"
+	"strings"
+
+	"github.com/yuin/goldmark"
+	meta "github.com/yuin/goldmark-meta"
+	"github.com/yuin/goldmark/extension"
+	"github.com/yuin/goldmark/parser"
+)
+
+var (
+	htmlTagRegex = regexp.MustCompile(`<[^>]*>`)
+	loneTableRow = regexp.MustCompile(`^\|.*\|$`)
+)
+
+// NormalizeAnchorQuote reduces an Anchor Quote to the text the browser will see, so a quote
+// copied from the Markdown source can be matched against the rendered block it came from.
+//
+// The agent quotes the source — `**bold**`, a backtick code span, a `##` or `- ` marker — while
+// the page matches against an element's textContent, which carries none of that. Every quote
+// with inline syntax therefore missed the very block it was copied from, and the thread lost its
+// target. The quote is put through the same renderer as the document body rather than having its
+// syntax stripped by hand: a second, approximate Markdown parser here is exactly what would drift
+// from what the page displays.
+func NormalizeAnchorQuote(quote string) string {
+	var buf bytes.Buffer
+	if err := newMarkdown().Convert([]byte(completeLoneTableRow(quote)), &buf, parser.WithContext(parser.NewContext())); err != nil {
+		// An unparseable quote is not an error: it falls back to the raw text and simply fails to
+		// find a target, which is where it already was.
+		return collapseWhitespace(quote)
+	}
+	return collapseWhitespace(stripHTMLTags(postProcessHTML(buf.String())))
+}
+
+// newMarkdown builds the converter both the document body and NormalizeAnchorQuote go through.
+// One constructor because the two must agree: a quote normalized by a different parser than the
+// one that rendered the page is a quote that matches text the page never displayed.
+func newMarkdown() goldmark.Markdown {
+	return goldmark.New(
+		goldmark.WithExtensions(
+			meta.Meta,
+			extension.GFM,
+		),
+	)
+}
+
+// completeLoneTableRow gives a table row quoted on its own the delimiter row GFM needs to read it
+// as a table. A `tr` carries an Anchor of its own, so a quoted row has to resolve; without the
+// delimiter goldmark reads the line as a paragraph and its pipes survive into the text, which no
+// rendered row contains.
+//
+// A pipe inside a code span throws the column count off and the table then does not parse — which
+// leaves the quote exactly where it was, so the miscount costs nothing it did not already cost.
+func completeLoneTableRow(quote string) string {
+	row := strings.TrimSpace(quote)
+	if strings.Contains(row, "\n") || !loneTableRow.MatchString(row) {
+		return quote
+	}
+	columns := strings.Count(row, "|") - 1
+	if columns < 1 {
+		return quote
+	}
+	return row + "\n|" + strings.Repeat(" --- |", columns)
+}
+
+// stripHTMLTags reduces rendered HTML to its text, the way textContent does in the browser.
+// It runs on goldmark's own output, where a `<` that is not a tag has already been escaped.
+//
+// A tag becomes nothing, not a space: textContent draws no whitespace from an element, so a space
+// here would put one where the browser has none — enough to lose the match on a code span that
+// ends a sentence. Block elements keep their separation from the newlines goldmark writes between
+// them, which the browser reads as text nodes too.
+func stripHTMLTags(htmlStr string) string {
+	return htmlUnescape(htmlTagRegex.ReplaceAllString(htmlStr, ""))
+}
+
+// htmlUnescape resolves the entities goldmark writes. html.UnescapeString is not used because it
+// also resolves entities the source wrote literally as text (`&amp;amp;`), which textContent
+// leaves alone.
+func htmlUnescape(s string) string {
+	return strings.NewReplacer(
+		"&amp;", "&",
+		"&lt;", "<",
+		"&gt;", ">",
+		"&#34;", `"`,
+		"&quot;", `"`,
+		"&#39;", "'",
+		"&#x27;", "'",
+	).Replace(s)
+}
+
+// collapseWhitespace mirrors normalizeQuote in references/template.html. Both sides of the
+// comparison have to be folded the same way, so the rule is stated identically in both places.
+func collapseWhitespace(s string) string {
+	return strings.Join(strings.Fields(s), " ")
+}

anchorquote_test.go

@@ -0,0 +1,152 @@
+package reviewer
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// An Anchor Quote is copied from the Markdown source, but the page matches it against the
+// rendered text of a block (references/template.html, resolveQuoteAnchors). Every quote that
+// carries inline Markdown syntax therefore fails to match the very block it was copied from,
+// and the thread loses its target.
+//
+// The quotes here are the ones the reproduction drove through review_reply; want is the text
+// the browser reports for the block each was copied from.
+func TestNormalizeAnchorQuoteMatchesRenderedText(t *testing.T) {
+	tests := []struct {
+		name  string
+		quote string
+		want  string
+	}{
+		{
+			name:  "plain prose is already the rendered text",
+			quote: "This is a plain paragraph with no markup at all and it serves as the control case.",
+			want:  "This is a plain paragraph with no markup at all and it serves as the control case.",
+		},
+		{
+			name:  "strong emphasis",
+			quote: "The retry budget is **at most three attempts** before the request is abandoned.",
+			want:  "The retry budget is at most three attempts before the request is abandoned.",
+		},
+		{
+			name:  "code span",
+			quote: "The column name is `segment_lock` with no generation suffix.",
+			want:  "The column name is segment_lock with no generation suffix.",
+		},
+		{
+			name:  "heading marker",
+			quote: "## Fallback Strategy",
+			want:  "Fallback Strategy",
+		},
+		{
+			name:  "list marker with strong emphasis",
+			quote: "- The queue drains **oldest first** when back-pressure is applied.",
+			want:  "The queue drains oldest first when back-pressure is applied.",
+		},
+		{
+			// An inline element contributes no whitespace to textContent, so neither may it here:
+			// a space where the browser has none costs the match the period at the end.
+			name:  "code span followed immediately by punctuation",
+			quote: "> The sandbox environment URL is `https://sandbox.gateway-api.com/v3`.",
+			want:  "The sandbox environment URL is https://sandbox.gateway-api.com/v3.",
+		},
+		{
+			// A tr is a comment target of its own, so a quoted row has to resolve. On its own the
+			// row is not a table — GFM wants the delimiter row too — and read as a paragraph its
+			// pipes survive into the text, which no rendered row ever contains.
+			name:  "table row quoted on its own",
+			quote: "| `/api/payment/refund` | `POST` | Refund a processed payment. | [Should] | [Inferred] |",
+			want:  "/api/payment/refund POST Refund a processed payment. Should Inferred",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := NormalizeAnchorQuote(tt.quote); got != tt.want {
+				t.Errorf("NormalizeAnchorQuote(%q)\n got = %q\nwant = %q", tt.quote, got, tt.want)
+			}
+		})
+	}
+}
+
+// The page matches an Anchor Quote against an element's textContent, so the text form has to
+// reach it. GET /api/feedback is the page's only source of comments, so that is where it is
+// derived — and it is derived on every read rather than stored, which is what lets a sidecar
+// written before this existed resolve its threads too.
+func TestFeedbackForDisplay_CarriesTheQuoteAsRenderedText(t *testing.T) {
+	ctx := t.Context()
+
+	s, err := StartSession(ctx, writeTempSpec(t), 0, true)
+	if err != nil {
+		t.Fatalf("StartSession failed: %v", err)
+	}
+	defer s.Close()
+
+	err = s.Reply(nil, []AskInput{
+		{Quote: "The retry budget is **at most three attempts** here.", Question: "why three?"},
+		{Question: "a question about the document as a whole"},
+	}, "round 1")
+	if err != nil {
+		t.Fatalf("Reply failed: %v", err)
+	}
+
+	var shown Feedback
+	if err := json.Unmarshal(s.feedbackForDisplay(), &shown); err != nil {
+		t.Fatalf("failed to decode /api/feedback payload: %v", err)
+	}
+	if len(shown.Comments) != 2 {
+		t.Fatalf("got %d comments, want 2: %#v", len(shown.Comments), shown.Comments)
+	}
+
+	const want = "The retry budget is at most three attempts here."
+	if got := shown.Comments[0].AnchorQuoteText; got != want {
+		t.Errorf("AnchorQuoteText = %q, want %q", got, want)
+	}
+	// A question about the document as a whole has no quote, so it must not gain a text form
+	// either — an empty quote is how the page tells "no target" from "target not found".
+	if got := shown.Comments[1].AnchorQuoteText; got != "" {
+		t.Errorf("document-level thread got AnchorQuoteText = %q, want empty", got)
+	}
+
+	// Derived, never stored: the sidecar keeps the quote the agent wrote and nothing else.
+	stored := s.readFeedbackDoc().Comments
+	if got := stored[0].AnchorQuoteText; got != "" {
+		t.Errorf("sidecar stored AnchorQuoteText = %q; it is derived on read, not persisted", got)
+	}
+}
+
+// A diff review resolves its quotes in Go, against the diff lines themselves, and the page never
+// looks at the text form there. Deriving one anyway would put a Markdown reading of a line of
+// source code on the wire — meaningless, and an invitation to use it.
+func TestFeedbackForDisplay_DerivesNoQuoteTextForADiff(t *testing.T) {
+	ctx := t.Context()
+
+	path := filepath.Join(t.TempDir(), "change.diff")
+	if err := os.WriteFile(path, []byte(round1Diff), 0644); err != nil {
+		t.Fatalf("failed to write temp diff: %v", err)
+	}
+
+	s, err := StartSession(ctx, path, 0, true)
+	if err != nil {
+		t.Fatalf("StartSession failed: %v", err)
+	}
+	defer s.Close()
+
+	if err := s.Reply(nil, []AskInput{{Quote: "\treplacement()", Question: "why?"}}, "round 1"); err != nil {
+		t.Fatalf("Reply failed: %v", err)
+	}
+
+	var shown Feedback
+	if err := json.Unmarshal(s.feedbackForDisplay(), &shown); err != nil {
+		t.Fatalf("failed to decode /api/feedback payload: %v", err)
+	}
+	got := shown.Comments[0]
+	if got.Anchor == "" {
+		t.Fatalf("a diff quote should have been resolved to an anchor by the server: %#v", got)
+	}
+	if got.AnchorQuoteText != "" {
+		t.Errorf("AnchorQuoteText = %q, want empty on a diff review", got.AnchorQuoteText)
+	}
+}

references/template.html

@@ -2814,6 +2814,10 @@ <h2>{{.Title}}</h2>
         // only in this page's DOM walk — reproducing that numbering in Go would be the very
         // duplication the anchor rules forbid. So the quote is resolved here, on every render.
         //
+        // Only the anchor, though. Reading the Markdown in the quote is the server's half, because
+        // it is the server that rendered the document and a second parser here would drift from
+        // what is on screen; the page is handed the result as anchorQuoteText.
+        //
         // A diff is different: its anchors are derived from the diff file, so the server has
         // already resolved those quotes by the time the page sees them.
         function normalizeQuote(text) {
@@ -2826,7 +2830,12 @@ <h2>{{.Title}}</h2>
 
             comments.forEach(c => {
                 if (!c.anchorQuote) return;
-                const want = normalizeQuote(c.anchorQuote);
+                // anchorQuoteText is the quote with its Markdown resolved to the text this page
+                // displays (NormalizeAnchorQuote in Go). Matching the raw quote instead would
+                // miss every passage the agent copied with its `**`, backticks or `##` intact —
+                // which is to say almost all of them. It falls back to the raw quote for a
+                // payload written before the server derived one.
+                const want = normalizeQuote(c.anchorQuoteText || c.anchorQuote);
                 if (!want) return;
                 // The first match only: exactly one element may carry the anchor, and the first
                 // occurrence is a more useful guess than none. A quote that matches nothing keeps

render.go

@@ -9,9 +9,7 @@ import (
 	"strings"
 	"text/template"
 
-	"github.com/yuin/goldmark"
 	meta "github.com/yuin/goldmark-meta"
-	"github.com/yuin/goldmark/extension"
 	"github.com/yuin/goldmark/parser"
 )
 
@@ -73,12 +71,7 @@ func Render(content []byte) ([]byte, error) {
 
 // RenderSpec compiles markdown to fully designed interactive HTML
 func RenderSpec(mdContent []byte) ([]byte, error) {
-	markdown := goldmark.New(
-		goldmark.WithExtensions(
-			meta.Meta,
-			extension.GFM,
-		),
-	)
+	markdown := newMarkdown()
 
 	var buf bytes.Buffer
 	context := parser.NewContext()

server.go

@@ -75,6 +75,13 @@ type Comment struct {
 	// AnchorQuote is the passage an agent-opened thread was written against. It is re-resolved to
 	// an Anchor on every render rather than trusted, so the thread survives the document moving.
 	AnchorQuote string `json:"anchorQuote,omitempty"`
+	// AnchorQuoteText is AnchorQuote as the browser will see it: Markdown syntax resolved to the
+	// text a rendered block actually displays. The agent quotes the source, the page matches
+	// against textContent, and the two only meet through this.
+	//
+	// It is derived on every read of the feedback rather than stored, so a sidecar written before
+	// it existed gets one too, and the quote the agent wrote stays the single source of truth.
+	AnchorQuoteText string `json:"anchorQuoteText,omitempty"`
 	// Messages is the rest of the thread, in chronological order.
 	Messages []Message `json:"messages,omitempty"`
 	// Reply and ReplyTimestamp are the pre-threading shape of a single agent response.

session.go

@@ -273,7 +273,9 @@ func (s *ReviewSession) feedbackForDisplay() []byte {
 	// Without this guard every Markdown comment fails FindFile, turns Outdated, and the Markdown
 	// review silently loses its indicators and connector lines.
 	if err != nil || DetectKind(content) != KindDiff {
-		return marshalFeedback(fb)
+		// This is the Markdown page, and it is the browser that turns a quote into an anchor
+		// there. Hand it the quote in the form it can match: see withQuoteText.
+		return marshalFeedback(withQuoteText(fb))
 	}
 
 	files, err := ParseUnifiedDiff(content)
@@ -284,6 +286,22 @@ func (s *ReviewSession) feedbackForDisplay() []byte {
 	return marshalFeedback(fb)
 }
 
+// withQuoteText gives every agent-opened thread the text form of its Anchor Quote, which is what
+// the page matches against a block's textContent.
+//
+// Markdown only: a diff review resolves its quotes in Go, against the diff lines themselves, so
+// the page never reads this there and a Markdown reading of a line of source code would be
+// nothing but a trap. It is derived on every read rather than stored, so a sidecar written before
+// the field existed gets one too.
+func withQuoteText(fb Feedback) Feedback {
+	for i, c := range fb.Comments {
+		if c.AnchorQuote != "" {
+			fb.Comments[i].AnchorQuoteText = NormalizeAnchorQuote(c.AnchorQuote)
+		}
+	}
+	return fb
+}
+
 func marshalFeedback(fb Feedback) []byte {
 	if fb.Comments == nil {
 		fb.Comments = []Comment{}
@@ -527,6 +545,12 @@ func (s *ReviewSession) newMux() *http.ServeMux {
 			// above spans all of it.
 			stored := s.readFeedbackDoc()
 			fb = mergeFeedback(stored, fb)
+			// The page posts back what it was served, derived fields included. Dropping the text
+			// form here keeps the sidecar to what the agent actually wrote: it is recomputed on
+			// every read, so a stored copy could only ever go stale.
+			for i := range fb.Comments {
+				fb.Comments[i].AnchorQuoteText = ""
+			}
 			fb.Comments = pruneResolved(fb.Comments, resolvedIDs(stored.Comments))
 			// Give every comment a stable identity so the agent can address it by ID.
 			fb.Comments = assignCommentIDs(fb.Comments)