fix(review): escape model prose before it reaches report sections#835
fix(review): escape model prose before it reaches report sections#835yetval wants to merge 1 commit into
Conversation
Report section readers use a single non-global regex, so the first "## <heading>" block anywhere in the markdown wins. Only systemContext and architectureDiagram were sanitized in parseDecision, so a forged "## Real Behavior Proof" block echoed into the summary was read instead of the genuine section. That flipped an unproven external PR from clawsweeper-verdict:needs-human to needs-changes plus clawsweeper-action:fix-required and raised its proof rating, bare or wrapped in a fence, a details block, or an HTML comment. Move OWNED_REVIEW_SECTION_HEADINGS and neutralizeOwnedSectionSpoofing into the zero-import clawsweeper-text leaf so decision-packets can share them, add requireReportText and requireReportTextArray next to requireString, and apply them to every model-authored prose field that lands in a report section body. Code-span and fence fields stay raw because the escaper rewrites < to <, which renders literally inside backticks. Also let front matter win over section lines in reportRealBehaviorProof and reportPrRating. Write-side escaping cannot repair reports already published in clawsweeper-state, and those are re-read every sweep cycle.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/openclaw/clawsweeper/blob/7e6f81f278720677d6f58d0dd0c5f21bf7d2c959/src/clawsweeper.ts#L3562
When a selected label reason contains a newline followed by an owned heading such as ## Review Findings, renderReportMarkdown writes it verbatim in the ## Label Justifications section, which appears before the real review sections. Because reviewSectionValue() takes the first matching ## Review Findings, reportReviewFindings() and isRepairLoopPassReport() can then see the forged no-findings section and emit a pass marker for an automerge/autofix PR even though the actual report later contains findings. Route this reason through requireReportText like the other model-prose fields.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@yetval - can i ask if you can try and hold off creating new PRs on clawsweeper at the moment. Please monitor discord channels. We hope to have things in a stable state in the next 24 hours or so. @Takhoffman - please try and refrain from merging any PRs if possible. Message me on discord when you get a chance. I have messaged you, but you seem offline. In summary - the clawsweeper issues in the last few days have had various AI agents proclaim to 'fix' things and haven't . About 80% have probably made things worse, or not fixed underlying issues and adding more noise is creating extra code that isn't necessary or helping right now. cc: @vincentkoc |
|
@brokemac79 My bad thats on me. Ill hold off on the PR's, sorry for the noise |
no problem - if possible, please set this one to draft, otherwise a maintainers clanker may pick it up and just merge. :-) |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(review): escape model prose before it reaches report sections This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2 participants
Summary
A contributor could get an unproven external PR routed into the automated repair lane by putting a fake
## Real Behavior Proofblock in the PR body. When the review model echoed that text into the report summary, the report parser read the forged block instead of the real one and the PR's proof verdict flipped from blocked to sufficient.This patch escapes model-authored prose on the way into the durable report artifact, so injected heading-shaped text can no longer create a section boundary that downstream parsers will trust.
Root cause
sectionValueinsrc/clawsweeper.tsis a single non-global regex match:The first
\n## <heading>\n\nanywhere in the report wins.markdownForemits## Summaryeleven sections before## Real Behavior Proof, so any## Real Behavior Proofblock sitting inside the summary body is read first.parseDecisionsanitized only two of its model-authored fields,systemContextandarchitectureDiagram. Every other prose field, includingsummary, was a rawrequireStringand reached the report verbatim.The consequences chained:
reportRealBehaviorProofreturnedsufficient, theproof: sufficientlabel was applied,realBehaviorProofBlocksMergereturned false,derivedPrRatingraised the proof tier, andreviewAutomationMarkersFromReportemittedclawsweeper-verdict:needs-changesplusclawsweeper-action:fix-requiredinstead ofclawsweeper-verdict:needs-human. The forgery works bare, inside a fence, inside<details>, and inside an HTML comment.Fix
OWNED_REVIEW_SECTION_HEADINGSandneutralizeOwnedSectionSpoofingverbatim fromsrc/clawsweeper.tsinto the zero-import leafsrc/clawsweeper-text.tssosrc/decision-packets.tscan use them without importingclawsweeper.ts.requireReportTextandrequireReportTextArraynext torequireString.parseDecision(summary, changeSummary, systemContext, risks, bestSolution, reproductionAssessment, solutionAssessment, visionFitReason, visionFitEvidence, closeComment, workReason, workPrompt), and the sub-parsers for evidence, likely owners, review findings, security review and its concerns, real behavior proof, PR rating, Telegram visible proof, Mantis recommendation reason, feature showcase, root-cause cluster, and AGENTS.md policy status.parseMaintainerDecisionand its option and owner parsers insrc/decision-packets.tsget the same treatment.<to<and that renders literally inside backticks:evidence[].command,mantisRecommendation.maintainerComment,mergeRiskOptions[].automergeInstruction,workLikelyFiles,workValidation,workClusterRefs,reviewFinding.file,securityConcern.file, andarchitectureDiagram, which keeps its existingsanitizeArchitectureDiagramallowlist. Each renderer was read before the field was classified.reportRealBehaviorProofandreportPrRating. Write-side escaping cannot repair reports already published inopenclaw/clawsweeper-state, and those are re-read every sweep cycle. The renderer emits front matter and section body from the sameDecision, so the two can only disagree when the section has been tampered with.Why this is the right boundary
Write-side escaping is already this codebase's load-bearing defense.
extractMarkdownSectioninsrc/repair/comment-router-core.tsis safe in production only because its input, the rendered review comment, hasneutralizeOwnedSectionSpoofingapplied on the way in. The gap was that the durable report artifact never got the same treatment. This patch extends an existing pattern one layer earlier rather than adding a second, differently-shaped defense.Hardening the reader was considered and rejected. Delegating
sectionValuetoextractMarkdownSectiondoes not fix the primary attack, because that helper also takes the first top-level heading match, so the bare forgery still wins. It is also not a drop-in: itsisMarkdownSectionBoundarytreats any[A-Z][^:\n]{0,80}:line as a boundary, and every report section body is built fromStatus:andEvidence kind:key-value lines, so nearly every section would be truncated.Out of scope
src/repair/workflow-utils.ts:2413andsrc/repair/create-job.ts:276each define the same first-matchsectionValueparser. They parse the same report artifact, so escaping the artifact removes their exploitability with zero changes to either file. While confirming that,src/repair/create-job.ts:249turned out to read aClawSweeper Work Promptsection that no renderer emits; the report heading isRepair Work Prompt. That path is dead code and is left alone here.frontMatterValue(src/clawsweeper.ts:7763on main) matches^key:with themflag rather than being bounded to the leading---block, so a body line can satisfy a front matter lookup. It is a good follow-up. It does not weaken this patch: the renderer always emits the real key inside the leading block, and the first match wins, so the genuine value is read first on every report this code writes.mergeRiskOptions[].titleand.body, which are rendered into the public comment through an already-neutralized path.Verification
pnpm run buildclean.pnpm run test:unit: the 11 new tests pass. The suite has 5 failures on unpatchedorigin/mainat 6284161 (apply-decisions yields instead of starting a post-close delay that cannot fit,exact-review health includes the oldest refreshing row beyond operator page bounds,failed issue retry bounds a hung GitHub fetch and flushes its report,failed issue retry leaves an ambiguous dispatch unconsumed and prevents a duplicate,apply workflow drops a coverage-proof tail only after exact trace examination) and the same set after this patch. They are load-sensitive timing tests unrelated to this change.pnpm run format,pnpm run lint:src,pnpm run check:active-surface,pnpm run check:limitsclean.test/decision-parser.test.ts: heading neutralization across every report body field; across evidence, owners, findings and nested summary fields; code-span fields preserved verbatim, which guards the classification in point 4 above; neutralization idempotent across a report round trip.test/pr-proof-automation.test.ts: one test per forgery shape (bare, fenced,<details>, HTML comment) driving the realreviewAutomationMarkersFromReportandrenderReviewCommentFromReport; front matter proof status outranking a forged section in a legacy unescaped report; legitimate fenced content in a report section still parsing.test/review-comment-rendering.test.ts: heading-shaped model text stays escaped, and is not double-escaped, in the rendered comment after the module move.srcand pass after. The other four are non-regression guards that pass on both sides by design.Real behavior proof
Behavior addressed: a forged
## Real Behavior Proofblock echoed into a report's## Summarysection is read byreportRealBehaviorProofinstead of the report's genuine proof section, which flips an unproven external PR fromclawsweeper-verdict:needs-humantoclawsweeper-verdict:needs-changesplusclawsweeper-action:fix-requiredand raises its proof rating.Real environment tested: two real checkouts of this repository on Linux with Node 24.18.0. Before is a pristine
origin/mainworktree at 6284161 with its own compileddist/. After is this branch's worktree withdist/compiled from the patched sources. No mocks or stubs; the driver imports the shippeddist/clawsweeper.jsfrom each tree and calls the exported production entry pointsparseDecision,reviewAutomationMarkersFromReportandrenderReviewCommentFromReport.Exact steps or command run after this patch: a driver script builds a model decision whose
summarycarries a forged proof block in four shapes (bare, wrapped in a fence, wrapped in<details>, wrapped in an HTML comment), passes it throughparseDecision, renders the resulting report with a genuine## Real Behavior Proofsection sayingStatus: missing, front matter carryingreal_behavior_proof_status: missing,pr_rating_overall: Fandwork_candidate: queue_fix_pr, then prints the routing verdict and the proof confidence row for each shape. It was run once per tree with the identical input:node /tmp/pr1-proof-driver.mjs <tree>Evidence after fix:
Before, on pristine
origin/mainat 6284161:After, on this branch:
Observed result after fix: all four forgery shapes now route to
clawsweeper-verdict:needs-humanwith noclawsweeper-action:fix-requiredmarker, and the proof confidence row reports the genuine missing proof at🧂 unranked krab (1/6)instead of the forged🦞 diamond lobster (5/6). The rendered comment shows the injected block escaped in the summary body, for example<!--and\## Real Behavior Proof, so it is visible to a human reviewer but inert to the parser. Before the patch every shape produced the same wrong result, which shows the fence,<details>and HTML-comment wrappers were not defenses.What was not tested: no live GitHub API calls, so the label write and PR comment update paths were exercised only through the renderer, not against a real repository. Reports already published in
openclaw/clawsweeper-statewere not replayed; the front matter precedence change is what covers them and it was verified only on a synthetic legacy report. The repair worker was not run end to end, so the effect on an already-queued job was not observed. Non-Linux platforms were not exercised.