fix: fail closed when a status comment author cannot be read

[dwin-gharibi]

Closes #1072.

What Problem This Solves

Fixes an issue where a comment whose author could not be read was treated as
trusted. isTrustedStatusComment is the guard that decides whether ClawSweeper
may adopt a comment as its own status comment — read durable state from it, and
PATCH its body — so an unknown author must never satisfy it.

const author = String(comment.user?.login ?? "").toLowerCase();
return !author || author === "clawsweeper" || trustedBots.has(author);
//     ^^^^^^^ an absent author authenticates as ClawSweeper

Note the internal inconsistency: the author read is defensive (?. and ?? ""),
so the code anticipates an absent author — and then trusts it. If an absent author
is impossible the defensive read is dead code; if it is possible this is a
fail-open. Either way the clause is wrong. Every other identity check in the repair
lane fails closed (comment-router-core.ts:1990, :2053, :2106).

Honest severity — please read before rating

I could not demonstrate a path where user.login is absent, and I looked hard:

• every consumer sources comments from gh api /repos/{repo}/issues/{n}/comments
  or /issues/comments/{id} — full REST objects, no --jq projection dropping user;
• no consumer uses gh pr view --json comments, which would supply author instead;
• deleted accounts are reattributed to ghost, a real login the guard correctly
  rejects on its own merits;
• no test constructs a comment without user.

The one remaining vector is GitHub's own contract: its OpenAPI schema types
issue-comment.user as nullable — which is exactly what the defensive read was
written for.

So this is a latent fail-open default, not a demonstrated exploit. I would rate it
P3. There is also defence in depth after the guard: an adopted comment must match
the issue number and already carry ClawSweeper's exact ack marker. The guard is the
first check, not the only one.

I am saying this plainly because a reviewer reading only "trust predicate fails
open" would over-rate it, and because the honest case for merging is "the cost is
one clause and the current behaviour is indefensible on its face", not "there is a
live incident".

Why This Change Was Made

Fail closed on an absent author, and collapse the two private copies into one
shared, exported, unit-testable comparator in comment-router-core.ts — a module
both consumers already import.

The two copies had already drifted: comment-router.ts consulted the
configurable trustedBots set while execute-fix-artifact.ts hardcoded two bot
logins. Deduplicating removes that drift risk; the hardcoded set is preserved
explicitly as REPAIR_TRUSTED_STATUS_AUTHORS so this PR changes only the
absent-author case, not which bots each consumer trusts.

Non-goals: no change to the marker, issue-number, or ack checks that run after the
guard, nor to the configurable trustedBots set.

Regression direction to weigh: if an absent author is reachable for a genuine
ClawSweeper comment, failing closed means ClawSweeper stops recognising its own
status comment and posts a duplicate instead of editing. Visible but harmless, and
it needs the same unproven precondition as the defect.

User Impact

No observable change in normal operation — by design, since no reachable path
produces an unreadable author today. A trust guard that could answer "yes" to an
unanswerable question no longer can.

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: fail closed when a status comment author cannot be read.
No CHANGELOG.md edit is included — happy to add one if this repo's release policy
wants it.

Evidence

Diff

 src/repair/comment-router-core.ts       | 24 +++++++++++++++++
 src/repair/comment-router.ts            |  4 +--
 src/repair/execute-fix-artifact.ts      | 18 +++++++------
 test/repair/comment-router-core.test.ts | 46 +++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 10 deletions(-)

Plus a new proof package under docs/proof/trusted-status-comment-fail-open/.

Focused tests

node --test test/repair/comment-router-core.test.ts → 129/129 pass
(127 pre-existing + 2 new). In-container across three repair suites: 201/201.

Neither private copy had a behavioural test before this. The existing suite only
used "function isTrustedStatusComment" as a source-slice boundary marker, so
nothing asserted the predicate's behaviour at all.

New cases:

• an unreadable author is not trusted — user: null, no user key, user without
  login, empty login, whitespace login, null comment, plus ghost;
• genuine ClawSweeper and configured bots remain trusted across casing and
  whitespace, while untrusted humans and an empty trusted set stay rejected.

Red/green

Reverted only src/repair/, rebuilt, re-ran: 2 fail, 127 pass. Restored:
129 pass.

Pre-fix the failure is "the shared export does not exist", which is a weak signal on
its own — so the proof below measures the behavioural contrast directly.

Real Behavior Proof

Claim. An unreadable author is not trusted; genuine authors are unaffected; and
no private copy of the predicate remains.

Exercised surface. isTrustedStatusCommentAuthor in
dist/repair/comment-router-core.js, plus a scan of both consumers' built output.

Scenario / fixture. Three claims: fails-closed over seven unreadable shapes;
no-loss over six genuine spellings and three rejections; and a built-output scan
requiring both consumers to reference the shared comparator and to contain no
return !author || shape.

How the contrast is measured — and a security note. The pre-fix predicate was
module-private, so it cannot be imported. My first draft eval'd the extracted
source via new Function; a security hook flagged that as a code-injection pattern,
and it was right — that does not belong in a committed script. The proof now:

1. reads the predicate's source text from the base commit;
2. asserts that text actually contains the return !author || clause and the
   same author read;
3. runs an explicit reimplementation of it.

Step 2 is what makes step 3 trustworthy — the reimplementation is proven faithful
to the shipped code rather than assumed. This is both safer and stronger evidence
than the eval it replaced.

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/trusted-status-comment-proof/**' \
  --script docs/proof/trusted-status-comment-fail-open/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_7a849d5a91ea (tidal-shrimp)
run	run_691f050226af
artifact	.crabbox/runs/run_691f050226af/run_691f050226af-artifacts.tgz
exit	0

Observed result. In-container: 25/25 proof checks PASS, focused suites
201/201, exit 0. The lease verified both sides first
(post-fix: 1 shared comparator reference, pre-fix fail-open clause present: 1).

the predicate as it shipped at the base commit:
  function isTrustedStatusComment(comment: LooseRecord) {
    const author = String(comment.user?.login ?? "").toLowerCase();
    return !author || author === "clawsweeper" || trustedBots.has(author);
  }

  { user: null }  pre-fix: true   post-fix: false

Artifact / trace. docs/proof/trusted-status-comment-fail-open/ 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-extract.log.

Limits. Covers the trust predicate only. Claim 3 scans for the return !author ||
shape specifically; a differently-spelled fail-open would not be caught. No live
GitHub comment is read, adopted, or edited.

Repository test suite status

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

Host full suite on this branch: 24 distinct failures, five not in the clean-tree
baseline. I verified every one rather than assuming:

• comment webhook settles duplicate fast ack comments after dispatch and
  webhook signature verification uses sha256 body hmac — known flakes; the file
  passes 25/25 in isolation, and a module-graph walk shows nothing this PR
  touches is reachable from comment-webhook.
• three pinned-base reproduction … cases in target-validation.test.ts — these
  pass in isolation and take 24–28 seconds each (heavy git operations), so
  they lose under parallel full-suite load. A module-graph walk from
  target-validation.js confirms comment-router-core, comment-router, and
  execute-fix-artifact are all unreachable from it.

Green on this branch:

check	environment	result
Crabbox local-container proof	Node 24 container	PASS, 25/25, exit 0
repair suites	Node 24 container	201/201
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

This PR fixes a fail-open condition in the repair lane’s “trusted status comment” guard by ensuring that an unreadable/absent comment author is not treated as trusted. It centralizes the trust check into a single exported helper in comment-router-core.ts, updates both prior call sites to use it, and adds focused regression coverage plus a proof contract.

Changes:

• Introduce isTrustedStatusCommentAuthor(...) that trims/normalizes the login and fails closed when the author cannot be read.
• Replace the two prior private predicate copies in comment-router.ts and execute-fix-artifact.ts with the shared helper (preserving the hardcoded repair trusted authors set).
• Add unit tests and a reproducible proof package under docs/proof/trusted-status-comment-fail-open/.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/repair/comment-router-core.ts	Adds the shared exported trust comparator that fails closed on missing authors.
src/repair/comment-router.ts	Switches status-comment trust to the shared comparator using trustedBots.
src/repair/execute-fix-artifact.ts	Switches status-comment trust to the shared comparator with a fixed trusted-author set.
test/repair/comment-router-core.test.ts	Adds behavioral coverage for unreadable authors and trusted logins.
docs/proof/trusted-status-comment-fail-open/run-proof.sh	Adds container proof harness to build, extract pre-fix source, and run proof + focused tests.
docs/proof/trusted-status-comment-fail-open/run-proof.mjs	Adds proof script validating fail-closed behavior, no-loss, and deduplication in dist/.
docs/proof/trusted-status-comment-fail-open/README.md	Documents the proof contract, claims, commands, and limits.

---

💡 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:18 AM ET / 06:18 UTC.

ClawSweeper review

What this changes

This PR centralizes status-comment author validation so missing or malformed GitHub comment authors are rejected before repair automation adopts or edits a status comment.

Merge readiness

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

Keep open: the fail-closed guard is directionally correct, but the new Docker reproduction path cannot supply the required pre-fix fixture and the PR body records 25 checks while the current proof contract requires 28. Fresh current-head proof is needed after correcting that path.

Priority: P3
Reviewed head: b3ecd01d6907d1de51b23b817af4be74ac7d22e2

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The core repair is focused, but the current-head proof record and newly added reproduction instructions are inconsistent.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The body’s 25/25 recorded output conflicts with the current 28-check proof contract, and the newly documented Docker reproduction path cannot reach the comparison without a staged fixture. 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)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The body’s 25/25 recorded output conflicts with the current 28-check proof contract, and the newly documented Docker reproduction path cannot reach the comparison without a staged fixture. 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	6 items	Fail-closed implementation: The shared comparator converts a missing login to an empty string and returns false before accepting the ClawSweeper identity or configured trusted authors. Both affected consumers use the shared guard: The comment router and repair executor delegate their status-comment checks to the new comparator; their comment sources are GitHub issue-comment REST endpoints. Focused coverage: Tests cover missing user/login shapes, rejected ghost and padded identities, and accepted ClawSweeper/trusted-bot identities.
Findings	1 actionable finding	[P2] Stage the fixture before the Docker proof run
Security	None	None.

How this fits together

The repair lane reads GitHub issue comments to locate its own status records before updating them during review, repair, and automerge workflows. The changed guard decides whether a comment may cross that trust boundary; rejected comments are ignored.

flowchart LR
  A[GitHub issue comments] --> B[Status-comment author guard]
  B --> C{Known automation author?}
  C -->|Yes| D[Read status marker]
  D --> E[Update status comment]
  C -->|No or missing| F[Ignore comment]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The body’s 25/25 recorded output conflicts with the current 28-check proof contract, and the newly documented Docker reproduction path cannot reach the comparison without a staged fixture. 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 fixture before the Docker proof run (P2) - The Docker recipe excludes .git, but it never runs stage-before.sh, so /work has neither Git history nor before/comment-router.ts. run-proof.sh therefore exits at its fixture check instead of producing the claimed contrast. Stage the fixture on the host before copying it, or otherwise copy a verified fixture into the container.
• Resolve merge risk (P1) - If GitHub ever returns a missing author for a genuine automation comment, the intended fail-closed behavior can post a duplicate status comment instead of editing the existing one.
• Resolve merge risk (P1) - The current Docker instructions fail before the comparison, and the recorded 25-check result does not match the 28-check current contract.
• Complete next step (P2) - The contributor needs to correct the proof documentation and supply a fresh current-head real-behavior trace; this cannot be established by a separate automated repair lane.

Findings

• [P2] Stage the fixture before the Docker proof run — docs/proof/trusted-status-comment-fail-open/README.md:203-206

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch composition	production +42/-10, tests +56, proof/docs +569	Most of the 8-file patch is proof material, so its documented reproduction path must execute correctly.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1072
Summary: This PR is the direct candidate fix for the canonical report about unreadable status-comment authors.

Members:

• canonical: Trusted status comment fails open on an unreadable author #1072 - The issue describes the same author-trust predicate and remains open while this implementation PR is reviewed.

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

Merge-risk options

Maintainer options:

1. Repair and re-run the proof contract (recommended)
   Stage the pre-fix fixture before the Docker copy, then publish a current-head trace that reports the contract's 28 checks and focused-suite result.
2. Pause the change
   Close or defer the PR if a reproducible current-head proof for this latent condition is not worth maintaining.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Correct the Docker proof recipe so it stages the pre-fix fixture, then rerun the Node 24 proof on the current head and include the 28-check result and focused repair-suite output.

Technical review

Best possible solution:

Keep the shared fail-closed comparator, amend the Docker recipe to stage the pre-fix fixture before copying the checkout, and publish a fresh current-head Node 24 proof showing all 28 checks plus the focused repair suites.

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

Yes, from source: a null or missing login reaches the old empty-author default, while the new comparator returns false. A live GitHub response with that shape remains unproven.

Is this the best way to solve the issue?

Yes for the product code: a shared fail-closed comparator is the narrowest solution and preserves configured trusted authors. The proof packaging is not yet the best path because its Docker recipe cannot run as documented.

Full review comments:

• [P2] Stage the fixture before the Docker proof run — docs/proof/trusted-status-comment-fail-open/README.md:203-206
  The Docker recipe excludes .git, but it never runs stage-before.sh, so /work has neither Git history nor before/comment-router.ts. run-proof.sh therefore exits at its fixture check instead of producing the claimed contrast. Stage the fixture on the host before copying it, or otherwise copy a verified fixture into the container.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

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 fail-open default without a demonstrated live GitHub response that omits an author.
• merge-risk: 🚨 security-boundary: The PR changes the author predicate that permits repair automation to adopt and edit GitHub status comments.
• 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’s 25/25 recorded output conflicts with the current 28-check proof contract, and the newly documented Docker reproduction path cannot reach the comparison without a staged fixture. 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

What I checked:

• Fail-closed implementation: The shared comparator converts a missing login to an empty string and returns false before accepting the ClawSweeper identity or configured trusted authors. (src/repair/comment-router-core.ts:847, b3ecd01d6907)
• Both affected consumers use the shared guard: The comment router and repair executor delegate their status-comment checks to the new comparator; their comment sources are GitHub issue-comment REST endpoints. (src/repair/comment-router.ts:5300, b3ecd01d6907)
• Focused coverage: Tests cover missing user/login shapes, rejected ghost and padded identities, and accepted ClawSweeper/trusted-bot identities. (test/repair/comment-router-core.test.ts:4444, b3ecd01d6907)
• Broken Docker reproduction path: The documented Docker command excludes .git and does not first create the untracked before/comment-router.ts fixture, while the proof script exits when neither git nor that fixture is available. (docs/proof/trusted-status-comment-fail-open/README.md:203, b3ecd01d6907)
• Current proof mismatch: The checked-in proof contract expects 28 passing checks, but the PR body reports 25/25; that output cannot establish the current proof contract was run. (docs/proof/trusted-status-comment-fail-open/README.md:56, b3ecd01d6907)
• Available local provenance: The supplied shallow review checkout attributes the changed guard and wrappers to the PR head; it does not retain pre-PR main history, so it cannot establish a stronger feature-history owner from local git alone. (src/repair/comment-router-core.ts:843, b3ecd01d6907)

Likely related people:

• steipete: The PR timeline records an explicit mention followed by subscription, and repository policy identifies Peter as the owner of sensitive sweep-operation decisions; local history is shallow. (role: likely security-boundary decision owner; confidence: low; files: AGENTS.md, src/repair/comment-router-core.ts)

Rank-up moves

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

• Fix the Docker recipe to stage or supply the pre-fix fixture.
• Re-run the current-head Node 24 proof and publish redacted output showing all 28 checks and focused repair suites.

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 (4 earlier review cycles)

• reviewed 2026-08-08T08:56:12.272Z sha be849eb :: needs changes before merge. :: [P2] Reject padded author logins | [P3] Soften the orphan-status guarantee
• reviewed 2026-08-08T17:17:11.151Z sha 8f4be80 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-12T11:58:48.748Z sha dc1b5e1 :: needs real behavior proof before merge. :: none
• reviewed 2026-08-12T12:36:29.109Z sha 307173f :: needs changes before merge. :: [P2] Remove the full pre-fix source snapshot

[dwin-gharibi]

@steipete