~bigbes/sr-ht-spec

ref: 6e59bf0ffc7d16f509dad40063d35bf8f6fb98a7 sr-ht-spec/service/comment.go -rw-r--r-- 10.7 KiB
472bcb1f — Eugene Blikh 24 days ago
feat(web): commentable prose diff with honest anchor state (spec-by6.3.3)

Reverses the diff view's founding rule. renderDocDiff skipped ChangeEqual
outright — "the review shows only what changed" — but any block of a proposed
document must be commentable, so unchanged blocks now render as collapsed,
dimmed context. Changed blocks keep their border, tint and full body, so the
page still reads as a diff at a glance rather than as a document dump. A
context block carrying a comment renders open. ChangeMoveIn now shows its text
too, since a comment control on invisible text is a control on nothing;
ChangeMoveOut stays a bare marker and is deliberately not commentable, because
the same paragraph is anchorable at its move-in position and two anchors for
one paragraph is the bug that avoids.

Every rendered block carries id="b-<16 hex>", hashed from the whole anchor
tuple. Not the page ordinal: an ordinal renumbers on any insertion above it, so
a saved link would silently scroll to a neighbouring paragraph, whereas
including the block hash makes a stale link resolve to nothing instead.

Threads are placed by anchor and by nothing else. Anything no rendered block
claims — an outdated anchor, an old-side anchor whose block the diff no longer
draws, a document the proposal no longer changes — is collected into a
page-level "comments that lost their anchor" area. Never dropped, never moved
onto a neighbour: a comment reads as authoritative about the block it sits
beside, so attaching it to the wrong one is worse than admitting it lost its
place. An edited anchor is drawn on its block and badged.

The comment form's anchor is built at submit time from the branch as it now
reads, through service.AnchorOf — hand-rolling the ordinal conversion here
would put the browser's comments on different blocks than the MCP tool's, which
is the one way two surfaces of one conversation disagree without either looking
broken. The form's block hash guards it: a block that moved while the page sat
open is a 409, not a comment attached to whatever took its place.

That hash is required rather than checked-when-present. Skipping the guard for
a form that omits it would let a later template refactor drop the hidden field
and disable the staleness check silently, with every test still green.

Authority is surfaced, not re-implemented: compose and resolve are the owner's
because service says so and ErrForbidden becomes a 403.

spec-by6.3.3
c6e5d667 — Eugene Blikh 24 days ago
feat(service): review threads and the policy auto-merge gate (spec-by6.3.2)

The comment API above db/, plus the rule that gives comments teeth: an
unresolved review thread suppresses policy auto-merge.

The gate matters because tryAutoMerge runs on every Propose, including an
agent's revision of an open proposal — so a proposal the owner stopped to
comment on could otherwise land unattended on the agent's next push. It gates
policy merges only. MergeHuman does not consult it: clicking approve is the
judgement the thread was asking for, and a comment nobody got round to
resolving must not be able to wedge a proposal shut.

Authority follows from that. The owner opens and resolves threads; an agent may
reply but may do neither, because both would hand the gate's control to the
thing it exists to hold back. An agent reply is not a resolution — answering a
critique is not the owner accepting the answer.

AnchorThreads lives here rather than in each surface, for the reason
Service.Archive does: the review page and the MCP tool must agree about whether
a comment still fits, and two surfaces each segmenting and matching would agree
only until one was changed. That is why service/ now imports prosediff. Each
document is segmented at most once per side however many threads hang off it.

AnchorOf is the other half: a surface offering "comment on this block" has a
document-global block ordinal, and the anchor needs the index within the block's
own heading path. Converting in one place is what stops the web form and the MCP
tool numbering blocks differently and putting their comments on different
blocks of the same document. A test asserts the two agree for every block of a
document.

A thread whose document is no longer among the proposal's changes — the agent
reverted it — is outdated, not dropped: a comment that silently vanished would
look like one that was never made.

Tested against a real Postgres 16, with a control test proving policy
auto-merge still fires without a thread, so the gate test cannot pass for the
wrong reason.

spec-by6.3.2