fix: stop trailing comment parsing from spanning visible prose

[dwin-gharibi]

Closes #1069.

What Problem This Solves

Fixes an issue where trailingHtmlComments could return an "HTML comment" that
spans visible prose. It scans backward from the end of a comment body, pairing each
--> with the nearest preceding <!--, without checking that the two actually
delimit one comment. A stray --> in prose therefore bridges back to an earlier
opener and swallows everything between them — including any real ClawSweeper marker
in that span.

This is the parser that recovers ClawSweeper's durable state markers from its own
published comment, so a swallowed marker is no longer independently recoverable.

--> is not exotic in these bodies: Mermaid flowchart edges are literally A --> B,
and ClawSweeper renders a Mermaid architecture diagram in its own reviews.

Honest severity — please read before rating

I could not construct a body from the real renderer that triggers this, and I
tried hard. The trigger needs all three of:

1. an earlier <!-- to bridge back to;
2. visible prose ending in --> after it;
3. the trailing marker block immediately after that prose.

Four realistic bodies — a Mermaid diagram in a closed fence, the same with an
unclosed fence, prose ending in an arrow, and a table cell ending in an arrow — all
parse correctly even before this change, because condition 1 is missing: the
durable comment's only mid-body opener is the <!-- clawsweeper-review-history -->
marker, and </details> always follows it before any trailing prose.

Both consumers are also fail-safe today, which I verified rather than assumed:

• clawsweeper-review-comment-state.ts anchors both ends with [^>]*, so an entry
  containing > fails to match and the marker is ignored;
• review-recovery-label-backfill.ts uses a start-anchored matcher and scans the
  whole entry for name=value, but its canonical check is strictly anchored, so a
  blob makes the recovery label be retained rather than wrongly cleared.

So this is a latent parser defect, currently safe by accident of its consumers'
regexes rather than by design — not a live incident. I would not rate it above
P3. The reason it is still worth fixing: the backfill harvests attributes from the
entire entry with a loose matcher, so if a blob ever satisfies its canonical check
— one regex refactor away — prose-derived attributes become authoritative for a
label-clearing decision.

I am flagging this rather than dressing it up, because a reviewer reading only the
"swallows real markers" framing would over-rate it.

Why This Change Was Made

An HTML comment ends at its first -->. The pairing never verified that, so the
fix rejects a candidate unless the opener's first terminator is the one being
matched:

if (value.indexOf("-->", commentStart + 4) !== end - 3) break;

Anything else means prose lies between them, so the trailing block has ended. That
makes the function honor its own documented contract — "the final contiguous
comment block", which is the name of its oldest test.

One behavior note, stated plainly: for the defect input, the earlier marker is now
not returned at all rather than returned inside a malformed blob. That is
correct — prose separates it from the trailing block — and it was unusable either
way, since every consumer regex rejected the blob.

Non-goals: no change to marker syntax, attribute parsing,
neutralizeReviewControlMarkers, or any consumer regex.

User Impact

No observable change in normal operation — by design, since no realistic body
reaches the defect today. The parser now cannot emit a malformed entry, which
removes a latent hazard for the recovery-label lane.

No configuration change, no migration, no data-contract change.

OpenClaw Bay: not affected. No status, lifecycle, or telemetry shape Bay renders
changes. No Bay update or Bay proof is needed.

Release note: fix: stop trailing comment parsing from spanning visible prose.
No CHANGELOG.md edit is included — happy to add one if this repo's release policy
wants it.

Evidence

Diff

 src/review-comment-markers.ts       | 18 +++++++++
 test/review-comment-markers.test.ts | 76 +++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

Plus a new proof package under docs/proof/trailing-html-comment-parsing/.

Focused tests

node --test test/review-comment-markers.test.ts → 6/6 pass (3 pre-existing + 3 new).

Consumer suites (review-recovery-label-backfill, review-placeholder-recovery)
are green and unchanged: 40/40 across all three files in-container.

New cases:

• a stray --> in prose never merges an earlier marker into a blob;
• a <details> review-history marker cannot be bridged into by later prose — the
  realistic shape, built from what renderReviewHistorySection actually emits;
• a property guard over seven bodies: every returned entry opens with <!--, closes
  with -->, and holds exactly one terminator.

Red/green

Reverted only src/review-comment-markers.ts, rebuilt, re-ran:

build	result
pre-fix	3 fail, 3 pass — exactly the 3 new tests
post-fix	6 pass, 0 fail

All 3 pre-existing assertions pass in both directions.

Real Behavior Proof

Claim. Every entry returned is a single well-formed comment, the mid-body
review-history marker is never bridged into, and clean bodies parse exactly as
before.

Exercised surface. dist/review-comment-markers.js.

Scenario / fixture. Three claims:

1. Well-formed — six bodies; every entry must contain no interior terminator.
2. Bounded — the <!-- clawsweeper-review-history --> marker stays out of the
   trailing block while the durable review marker is still recovered.
3. No loss — seven clean bodies parse byte-identically to a pre-fix build
   compiled from the base commit inside the lease. This is the claim that
   matters: a stricter parser must not make a previously-recoverable marker
   unrecoverable. If that build is unavailable the claim reports SKIPPED and
   fails.

Fixtures are shaped like real renderer output — a Mermaid flowchart (edge syntax
-->), a <details> review-history block with its own marker, then the trailing
block.

Command and environment. Node 24 in a Docker-backed Crabbox local-container
lease:

crabbox run \
  --provider local-container \
  --local-container-image node:24 \
  --no-hydrate \
  --timing-json \
  --artifact-glob '.artifacts/trailing-html-comment-proof/**' \
  --script docs/proof/trailing-html-comment-parsing/run-proof.sh

field	value
provider	Crabbox local-container (Docker/OrbStack)
crabbox	0.15.0
image	node:24 @ sha256:934240a162082fd8b8a2f90cd5114446443f1eba1c5378f6687167ca405e6584
container node	v24.19.0 (satisfies engines.node >= 24)
lease	cbx_035d141014da (violet-crab)
run	run_236711b279b6
artifact	.crabbox/runs/run_236711b279b6/run_236711b279b6-artifacts.tgz
exit	0

Observed result. In-container: 19/19 proof checks PASS, focused suites
40/40, exit 0. The lease verified the guard on both sides (post-fix: 1 interior-terminator check, pre-fix: 0) before running the claims.

defect case: marker, prose ending in an arrow, marker
  pre-fix : ["<!-- clawsweeper-verdict:needs-human item=321 sha=head -->\nrenders as -->",
             "<!-- clawsweeper-review item=321 -->"]
  post-fix: ["<!-- clawsweeper-review item=321 -->"]

All seven clean bodies: identical to pre-fix.

An environment failure this proof caught. The first lease run failed during
build: TypeScript 7 dispatches to a platform-native binary published as an optional
dependency, and @typescript/typescript-linux-arm64 was missing after
pnpm install --frozen-lockfile, so tsc could not start. It surfaced far from its
cause. The script now checks for the platform package explicitly, fetches it if
absent, and fails with a clear message otherwise — the retry shows
NOTE: … missing after install; fetching it explicitly followed by a clean build.
Recording it because the same trap will hit any future arm64 lease.

Artifact / trace. docs/proof/trailing-html-comment-parsing/ holds
run-proof.sh, run-proof.mjs, and the contract README. The lease artifact tarball
holds proof-output.txt, focused-tests.txt, install.log, build.log, and
prefix-build.log.

Limits. Covers the trailing-block scan only. Claim 3 covers seven clean bodies,
not an exhaustive enumeration; the new pairing rule matches the HTML spec, so
divergence is confined to inputs where prose sits between an opener and the matched
terminator — exactly the defect. No live GitHub comment is published or read.

Repository test suite status

The proof and focused suites run on Node 24 inside the Crabbox lease recorded above
(40/40 in-container). The note below concerns only the full suite on the macOS host.

• comment-webhook.test.ts passes 25/25, three runs in a row in isolation on
  this branch;
• a module-graph walk from dist/repair/comment-webhook.js shows
  review-comment-markers is not reachable from it.

Green on this branch:

check	environment	result
Crabbox local-container proof	Node 24 container	PASS, 19/19, exit 0
marker + recovery suites	Node 24 container	40/40
pnpm run build:all	macOS host	clean
pnpm run format:check	macOS host	clean
pnpm run lint	macOS host	clean (all four projects)

[Copilot]

Pull request overview

Fixes trailingHtmlComments so it can’t fabricate a “comment” that spans visible prose when a stray --> appears outside of an actual HTML comment, ensuring durable ClawSweeper markers remain safely recoverable from published review bodies.

Changes:

• Harden trailingHtmlComments by rejecting backward-paired <!-- ... --> candidates unless the opener’s first --> is the terminator being matched.
• Add targeted regression/property tests covering the stray-terminator-in-prose case and a realistic mid-body review-history marker layout.
• Add a Crabbox/Node 24 proof contract + scripts under docs/proof/ to demonstrate well-formedness, boundedness, and no-loss behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/review-comment-markers.ts	Adds an interior-terminator guard so backward pairing can’t bridge across prose.
test/review-comment-markers.test.ts	Adds regression and property-guard tests for well-formed trailing comment extraction.
docs/proof/trailing-html-comment-parsing/run-proof.sh	Container proof runner that builds, compiles a pre-fix baseline, and runs proof + focused tests.
docs/proof/trailing-html-comment-parsing/run-proof.mjs	Proof script asserting well-formed, bounded, and no-loss claims against dist/.
docs/proof/trailing-html-comment-parsing/README.md	Documents the proof contract, claims, fixtures, and expected outputs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 19, 2026, 2:14 AM ET / 06:14 UTC.

ClawSweeper review

What this changes

The branch adds a first-terminator guard to durable review-marker parsing, regression tests, and a Crabbox proof package.

Merge readiness

⛔ Blocked until stronger real behavior proof is added - 6 items remain

Keep open: current main still has the unbounded parser pairing, and the code fix is narrow, but the current-head proof command cannot run as documented.

Priority: P3
Reviewed head: 07f54329111d53a859d506b2decb03071f1b3b93

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The parser patch is focused, but proof for the current head is not yet runnable or independently attributable.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The body reports an earlier Crabbox run, but it is not tied to the current head and the documented command cannot stage the required pre-fix module; publish a redacted current-head trace or artifact after repair. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🦐 gold shrimp (3/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The body reports an earlier Crabbox run, but it is not tied to the current head and the documented command cannot stage the required pre-fix module; publish a redacted current-head trace or artifact after repair. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	5 items	Current main still lacks the guard: Current main pairs the final terminator with the nearest preceding opener and returns that slice without verifying an earlier terminator. Parser provenance: The current parser was introduced as a new file by Peter Steinberger in commit c175faa; blame assigns the relevant current-main lines to that commit. Narrow repair and coverage: The branch rejects a pair unless the opener’s first terminator is the candidate terminator, with focused malformed-marker cases.
Findings	1 actionable finding	[P2] Stage the pre-fix fixture before the documented Crabbox run
Security	Needs attention	Constrain registry-backed proof bootstrap: The fallback executes npm install -g pnpm@11.10.0 outside the lockfile and without lifecycle suppression, creating a new executable supply-chain path for anyone running the proof.

How this fits together

Rendered ClawSweeper review comments contain durable HTML markers and optional review-history prose. Recovery code scans the final contiguous marker block to restore review state and labels.

flowchart LR
A[GitHub review comment] --> B[Rendered review history]
B --> C[Trailing marker parser]
C --> D{Contiguous comments?}
D --> E[Durable review markers]
E --> F[Review state and labels]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The body reports an earlier Crabbox run, but it is not tied to the current head and the documented command cannot stage the required pre-fix module; publish a redacted current-head trace or artifact after repair. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Stage the pre-fix fixture before the documented Crabbox run (P2) - The README command invokes a no-git lease without stage-before.sh, yet no before/ fixture is committed. The runner therefore exits at its missing-fixture branch instead of exercising the no-loss claim; add staging and cleanup to the invocation and correct the stale “committed” wording.
• Resolve security concern: Constrain registry-backed proof bootstrap - The fallback executes npm install -g pnpm@11.10.0 outside the lockfile and without lifecycle suppression, creating a new executable supply-chain path for anyone running the proof.
• Resolve merge risk (P1) - The documented no-git Crabbox command cannot produce its promised pre-fix contrast until it stages the base module.
• Resolve merge risk (P1) - The proof runner bootstraps executable registry tooling outside the lockfile, so it should not be run on a credentialed host until that bootstrap is constrained.
• Complete next step (P2) - The contributor must repair the proof command and provide fresh current-head evidence; automation cannot establish that external proof on their behalf.

Findings

• [P2] Stage the pre-fix fixture before the documented Crabbox run — docs/proof/trailing-html-comment-parsing/README.md:58-66
• [low] Constrain registry-backed proof bootstrap — docs/proof/trailing-html-comment-parsing/run-proof.sh:113

Agent review details

Security

Needs attention: The added proof runner introduces a registry-backed package-manager bootstrap outside locked dependency resolution.

Review metrics

Metric	Value	Why it matters
Change composition	production +18, tests +83, proof/docs +600	The parser repair is small while persistent proof tooling is most of the change, so its executable path needs to be reliable.
Files affected	6 files changed	The change crosses parser, tests, and proof documentation rather than a single isolated source file.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1069
Summary: This PR is the linked candidate fix for the canonical trailing-comment parser issue.

Members:

• canonical: Trailing HTML comment parsing #1069 - The PR explicitly closes that issue and implements its described parser boundary.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

1. Make the proof reproducible and safe (recommended)
   Stage the pre-fix module before the no-git lease, correct the stale README claim, constrain the registry bootstrap, then publish a redacted current-head trace.

Technical review

Best possible solution:

Retain the parser guard and focused tests, then make the proof invocation stage and clean up its pre-fix input deterministically, use a trusted bootstrap path, and publish a redacted trace tied to the current head.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main can pair an earlier opener with visible prose ending in -->; normal renderer reachability remains unproven.

Is this the best way to solve the issue?

Yes for the parser itself: the first-terminator guard is the narrowest correction, but its proof package needs repair before it demonstrates the current head.

Full review comments:

• [P2] Stage the pre-fix fixture before the documented Crabbox run — docs/proof/trailing-html-comment-parsing/README.md:58-66
  The README command invokes a no-git lease without stage-before.sh, yet no before/ fixture is committed. The runner therefore exits at its missing-fixture branch instead of exercising the no-loss claim; add staging and cleanup to the invocation and correct the stale “committed” wording.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 02c930c53d1c.

Labels

Label justifications:

• P3: This is a source-proven latent parser defect with no demonstrated normal renderer incident.
• merge-risk: 🚨 automation: The added proof automation currently fails to obtain its required pre-fix input in the documented no-git run.
• merge-risk: 🚨 security-boundary: The added proof script downloads and executes package-manager tooling outside the repository lockfile.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports an earlier Crabbox run, but it is not tied to the current head and the documented command cannot stage the required pre-fix module; publish a redacted current-head trace or artifact after repair. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

• [low] Constrain registry-backed proof bootstrap — docs/proof/trailing-html-comment-parsing/run-proof.sh:113
  The fallback executes npm install -g pnpm@11.10.0 outside the lockfile and without lifecycle suppression, creating a new executable supply-chain path for anyone running the proof.
  Confidence: 0.93

What I checked:

• Current main still lacks the guard: Current main pairs the final terminator with the nearest preceding opener and returns that slice without verifying an earlier terminator. (src/review-comment-markers.ts:10, 02c930c53d1c)
• Parser provenance: The current parser was introduced as a new file by Peter Steinberger in commit c175faa; blame assigns the relevant current-main lines to that commit. (src/review-comment-markers.ts:10, c175faa03a95)
• Narrow repair and coverage: The branch rejects a pair unless the opener’s first terminator is the candidate terminator, with focused malformed-marker cases. (src/review-comment-markers.ts:27, 07f54329111d)
• Documented proof command cannot stage its base module: The documented no-hydrate invocation does not run the staging helper; no before/ fixture is committed, while the runner explicitly fails when neither git nor that fixture is available. (docs/proof/trailing-html-comment-parsing/README.md:58, 07f54329111d)
• Current-head proof is not established: The current head was committed after the previous reviewed SHA, while the PR body’s reported lease output does not identify the current head or provide a current artifact trace. (docs/proof/trailing-html-comment-parsing/run-proof.sh:96, 07f54329111d)

Likely related people:

• Peter Steinberger: Git history and blame identify this author as introducing the current-main parser and its recovery boundary. (role: introduced current parser; confidence: high; commits: c175faa03a95; files: src/review-comment-markers.ts, src/clawsweeper-review-comment-state.ts, src/review-recovery-label-backfill.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Correct the documented staging path and stale fixture description.
• Run the repaired proof against the current head and add a redacted SHA-identified trace or artifact.
• Remove or constrain the registry-backed package-manager fallback.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (25 earlier review cycles; latest 8 shown)

• reviewed 2026-08-09T19:11:27.921Z sha 84f12f1 :: needs real behavior proof before merge. :: [P2] Keep fallback installation out of the synced checkout
• reviewed 2026-08-09T21:04:19.322Z sha 84f12f1 :: needs real behavior proof before merge. :: [P2] Keep fallback provisioning out of the synced checkout
• reviewed 2026-08-09T22:50:08.684Z sha 84f12f1 :: needs real behavior proof before merge. :: [P2] Provision fallback dependencies outside the synced checkout
• reviewed 2026-08-11T12:13:37.465Z sha 84f12f1 :: needs real behavior proof before merge. :: [P2] Provision fallback dependencies outside the synced checkout
• reviewed 2026-08-12T06:24:16.132Z sha 84f12f1 :: needs real behavior proof before merge. :: [P2] Provision proof dependencies outside the synced checkout
• reviewed 2026-08-12T11:58:17.636Z sha 61cfcbf :: needs maintainer review before merge. :: none
• reviewed 2026-08-12T13:02:42.603Z sha 40dcf94 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-12T23:32:40.990Z sha 40dcf94 :: needs real behavior proof before merge. :: [P2] Pass and verify the tested head in the Crabbox command | [P2] Stop overwriting the committed pre-fix fixture before proof

[dwin-gharibi]

@steipete