feat(claude-ops): surface review findings stranded on merged pull requests - #1781
Conversation
…uests A review that lands after a merge has nowhere to go. The ruleset's required_review_thread_resolution is a merge-time predicate that already passed, the babysit lane works only open pull requests, and nothing on a merged pull request surfaces its open threads. Six findings — one P1 — posted 46 seconds after #1720 merged sat unread for a day, and surfaced only because a later session happened to audit that merge batch. The morning brief is the right home: read-only, unattended, and already the place attention signals land. The discriminator is each thread's first-comment timestamp against the pull request's mergedAt, so only threads the gate could never have seen are reported; a thread predating the merge is an ordinary unresolved thread and is excluded. Findings collapse to one line per pull request at that pull request's WORST severity with a count, so a P0 beside advisory findings is never softened and one noisy pull request cannot bury the rest. The section fails loud rather than clear. A GraphQL error document is well-formed JSON carrying no `data`, so an unread API would extract to an empty list and render as "every merged PR in the window is clear" — an all-clear asserted from an answer never received, the same fail-open shape this section exists to catch. A rate-limit error did exactly that during development. Its first live run against this repository immediately found four more stranded findings on other merged pull requests, including a P1 recording that a shipped plugin cell never reached installations. This is a standing leak, not a one-off. Tests: 30 -> 63 cases, 0 failures. The negative cases carry the weight — a pre-merge thread, an already-resolved one, and a merge outside the window must all stay silent, or the section is noise rather than signal. Closes #1777 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 26s —— View job Review: feat(claude-ops): surface review findings stranded on merged pull requestsThe problem statement is well-argued, the fail-loud design is sound, and the test suite's negative cases carry real weight. One correctness bug in the jq severity logic, two documentation gaps, and two minor test coverage gaps. [CORRECTNESS] Severity collapsing picks
|
|
Claude finished @kyle-sexton's task in 3m 52s —— View job Security Review
No critical or important security vulnerabilities found. One suggestion-level finding, and one noteworthy positive security improvement worth calling out explicitly. SUGGESTION — Unfiltered GitHub API data echoed to terminalConfidence: PLAUSIBLE
# L601-602
echo "$stranded" # contains PR titles and author.login from GitHub API
# L544
echo " $api_err" # contains GitHub GraphQL error message text
Practical exposure is low: the operator controls which repository is targeted, No remediation required to accept this PR. If hardening is ever desired, piping the Positive security finding — Fail-loud API error handlingL535–547 correctly detects the fail-open shape where a well-formed GraphQL error document (no Other areas reviewed (no findings)
|
Live verification — the gap this closes is far larger than the case that found itThe known gap in the PR body is now closed. Fresh live run against this repository, on the current 43 merged pull requests are carrying review findings that were posted after they merged — in a
The P0 is #969 ( This started from six findings on one PR. It is a standing leak at a scale nobody had measured, The window filter is correctSpot-checked, because a PR numbered #969 appearing in a five-day window looks wrong at a glance:
Low PR number, recent merge. The gate filters on Triage noteAt least one of these is already handled in substance: #1694's P1 asked for an That is the section behaving correctly rather than a false positive — an unresolved thread is |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: ec9634d6f9
ℹ️ 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".
…report truncation Three review findings on the PR that added this section, each a way the section could mislead the operator it exists to inform. - Severity was tested against the whole comment body, so a P2 titled "Preserve P1 labels" reported as P1, and any finding merely discussing CRITICAL or SECURITY was promoted to P0. Severity now comes only from the structured marker — the badge alt-text, the shields badge URL, or a leading bracket. - Ranking compared display strings, and "--" sorts before "P0", so a single unclassified thread beside a genuine P0 collapsed the whole PR to "[--]" and hid it. Ranking is numeric; unrecognized sorts last. - The thread connection requested one page with no pageInfo, and --paginate follows only the outer cursor, so a PR with more than 100 review threads was silently truncated. It now requests the page maximum, reads hasNextPage, and says the read was PARTIAL rather than letting a truncated read render as an all-clear. Also fixes a jq abort found while testing the above: `capture(...).s` and `.captures[0]` both index an object when the regex does not match, which killed the whole program and rendered EVERY pull request as clear — the exact fail-open this section exists to catch, reintroduced by the fix for it. The match now yields empty and falls through to the next alternative. A second instance mapped over grouped objects rather than their `.sev` strings. `\s` in the test file is a GNU ERE extension the portability lint bans; replaced with `[[:space:]]`. No suppression added. Tests: 63 -> 71 cases, 0 failures. Every finding above gets both directions — prose must not promote, an unclassified thread must not mask a P0, both marker forms must be recognized, and a truncated read must announce itself. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 2s —— View job Review: follow-up pass after
|
|
Claude finished @kyle-sexton's task in 6m 2s —— View job Security Review
No critical or important security vulnerabilities found. One suggestion-level finding in the fail-loud path, and one positive security finding worth calling out explicitly. SUGGESTION — Non-JSON stdout bypasses the fail-loud guard and produces a silent false all-clearConfidence: PLAUSIBLE if [[ -z "$merged" ]]; then
echo " (unable to read merged PRs)"
...
fi
# FAIL LOUD.
api_err="$(jq -s -r '... | first // empty' <<<"$merged" 2>/dev/null)"
if [[ -n "$api_err" ]]; then ...The No "NOT an all-clear" qualifier. The section designed specifically to prevent silent false all-clears produces one. The previous security review's table entry marked this case as "Correct — degrades to 'unable to read' not all-clear," but that conclusion only holds when Practical exposure is low. A targeted fix: validate that if ! jq -e . >/dev/null 2>&1 <<<"$merged"; then
echo " (unable to read merged PRs — response is not valid JSON, this is NOT an all-clear)"
echo
return
fiPositive security finding — Severity inflation via body-wide string matching is closed in e561a35The initial commit ( | if test("P0|CRITICAL|SECURITY"; "i") then "P0"
elif test("P1"; "i") then "P1"
elif test("P2"; "i") then "P2"
else "--" endAny comment body containing the word "CRITICAL" or "SECURITY" anywhere — including in a P2 advisory note discussing "critical infrastructure" — would be promoted to P0 in the morning brief. This is a semantic integrity issue in the opposite direction: instead of hiding a high-severity finding, an operator could see a false P0 with nothing to act on, eroding trust in the section. The fix commit ( Other areas reviewed (no findings)
|
Correction: the P0 in my earlier comment was a false promotion, by the bug review then caughtThe severity figures in my comment above were produced by the pre-review severity logic, which The #969 "P0" was a P1 whose prose mentioned a higher severity. There is no P0 in the window. Re-run on
The P1 count went up, not down: numeric ranking now stops a single unclassified thread from The ten P1s: No truncation warning fired, so every affected PR is under the 100-thread page limit and this read What the review round changedAll three findings are fixed in Tests: 63 → 71, 0 failures. |
`comment-hygiene` flags `tracker-ref:pr-reference`: a comment citing the pull request that produced it is a back-reference no future reader can resolve, and version control already owns that history. The section header carried one. The cases below it already state what they cover, so nothing is lost. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 26s —— View job Review pass at
|
|
Claude finished @kyle-sexton's task in 3m 4s —— View job Security Review
No new security vulnerabilities in the final commit ( SUGGESTION — Non-JSON API response bypasses the fail-loud guard → silent false all-clearConfidence: PLAUSIBLE — raised in the prior security review pass on The guard against an empty response fires when if [[ -z "$merged" ]]; then
echo " (unable to read merged PRs)"
...
fiBut if No NOT-an-all-clear qualifier. The section built specifically to prevent silent false all-clears produces one, for exactly the same shape of failure it was built to catch. A targeted fix: validate JSON-parsability before entering the error-detection block: if ! jq -e . >/dev/null 2>&1 <<<"$merged"; then
echo " (unable to read merged PRs — response is not valid JSON, this is NOT an all-clear)"
echo
return
fiSUGGESTION — Unfiltered GitHub API data reaches the terminalConfidence: PLAUSIBLE — raised in both prior security review passes; a systemic pattern throughout the script, not new to this PR
echo "$stranded" # contains PR titles from GitHub API
echo " $api_err" # contains GraphQL error message text
No remediation required to accept this PR. If hardening is desired, piping through Areas reviewed — no findings
What was fixed in
|
…sent (#1851) No linked issue ## Summary Fixes two of the three defects re-verified in #1784 — the two stranded #969 findings. Both were a value that is *present but falsy* being read as *absent*. **#1784 is deliberately NOT closed by this PR.** Its P1 (the lane-stop gate reading its enable flag off channel B) is untouched — see "Not addressed" below. ## Fix **`plugins/autonomy/hooks/lane-stop-gate.sh` (0.11.7 → 0.11.8).** The completion marker's one-shot authorization was latched solely by deleting the file, and the marker lives in the watched checkout — a directory the hook is not guaranteed to be able to write. An `rm` the OS refused left a file that still satisfied `[[ -f "$MARKER" ]]` on the next run: exactly the cross-run bypass that consuming the marker exists to close. The surrounding comment asserted "the next run must not rely on that stale file" while nothing enforced it. Consumption is now recorded in the plugin's own persistent data directory — the marker path plus the consumed file's identity (mtime and size) — and the deletion is the tidy-up rather than the latch. A marker recorded as consumed is not a signal however long it survives on disk; a marker later recreated has a new identity, so the stale record is dropped and the fresh marker authorizes normally. Recreation recovery is **best-effort by design**, and the second commit scopes the claim to what the identity read actually resolves. Both portable `stat` dialects report whole-second mtime, so a marker recreated at the same size within the same second — an empty `touch`-style marker being the realistic case — is indistinguishable from the consumed one and stays latched until the second turns over. Sub-second (`%.9Y`) and inode spellings would narrow that window but are GNU-only, and this identity feeds a **gate**: the coarse read costs a stop delayed by under a second, while a wrong "recreated" verdict costs the unearned second authorization the ledger exists to prevent. Delay is the correct failure direction, so the portable spelling stands and the comment and CHANGELOG say so rather than implying recovery is guaranteed. A host where neither dialect reports an identity holds the record for the same reason. The data directory is derived from the hook's **own install path** (the `plugins/cache` anchor Claude Code documents), falling back to `CLAUDE_PLUGIN_DATA` only for a `--plugin-dir` install carrying no such anchor: the script's own location is not something a watched repository can redirect, whereas `CLAUDE_PLUGIN_DATA` is an env value a repo `settings.json` `env` block reaches. Where no data directory can be written, the deletion remains the only latch — the behavior that predates this ledger. **`plugins/claude-ops/skills/lanes/scripts/lane-launcher.sh` (0.24.4 → 0.24.5).** Both field readers used jq's `//` alternative operator, which fires on every FALSY value rather than on absence. A lane configured `"settings": false` yielded `empty`, reached bash as `""`, and — because `validate_launch_inputs` guards its "settings must be a JSON object" check on `[[ -n "$settings" ]]` — that type check never ran at all: the lane launched with `--settings` silently omitted, no error, nothing for the operator to see. `lane_json_field` now tests presence with `has`, so `false` reaches the type check and the lane is skipped with the error already written for it. The scalar reader had the same collapse for `name`/`model`/`effort`/`prompt` (a mistyped `"effort": false` launched a lane with no effort), so those fields are typed once at config time and a non-string value is a config error alongside the existing duplicate-name and path-traversal checks. An explicit `null` stays the JSON spelling of "no value" and remains equivalent to an absent field in both readers. ## Test plan Red-to-green proven for both defects by running the new cases against the **pre-fix** sources checked out from `origin/main` into a staged copy, then against the fixed sources. | Suite | Pre-fix (`origin/main` source) | Post-fix | |---|---|---| | `plugins/autonomy/hooks/lane-stop-gate.test.sh` | **1 fail** — `a surviving consumed marker wrongly authorized a later run` | **0 fail** (37 cases) | | `plugins/claude-ops/skills/lanes/scripts/lane-launcher.test.sh` | **10 fail** — the 8 `boolean .name/.model/.effort/.prompt` cases plus `settings:false reaches the type check`, `settings:false lane not launched`, `settings:false surfaces a non-zero exit` | **0 fail** (144 cases) | Case counts differ across the two columns for the gate suite because cases 21b and 22–23 were added *after* that red run; the red run is the marker regression alone. Case 21b pins the same-second/same-size recreation boundary described above, and reports which side of the second it landed on rather than asserting a timing race — so a future finer-grained identity has to move that case deliberately. **A defect the new coverage found.** The first draft of the marker ledger tested only the `CLAUDE_PLUGIN_DATA` fallback — never the install-path derivation the tamper-resistance claim actually rests on. Cases 22–23 stage the hook under a synthetic `<root>/plugins/cache/<marketplace>/<name>/<version>/hooks/` tree with an unrelated `CLAUDE_PLUGIN_DATA` present, and they failed: `gate_data_dir` appended `/plugins` twice, writing to `<root>/plugins/plugins/data/<id>` instead of the documented `<root>/plugins/data/<id>`. The layout was checked against a real install on this machine (`~/.claude/plugins/cache/melodic-software/autonomy/<ver>` alongside `~/.claude/plugins/data/autonomy-melodic-software`) and the path is corrected here. Gates run locally against `origin/main` as base before pushing, all green (and re-confirmed by the full CI run on this PR): - `scripts/check-changelog-parity.sh --check`, `--check-bump origin/main`, `--check-order` - `scripts/check-changed-skills.sh origin/main` - `scripts/check-silent-skips.sh` - `scripts/check-shell-portability.sh origin/main` - `shellcheck --rcfile=.shellcheckrc` over all four changed shell files - `markdownlint-cli2 --config .markdownlint-cli2.jsonc` over the two CHANGELOGs and `config.md` - Sibling suites unaffected and still green: `lane-notify`, `machine-behavior`, `restart-consumer`, `telemetry-upsert` ## Not addressed **#1784's P1 — the lane-stop gate reads its enable flag off channel B — is not fixed here, and the issue stays open for it.** `lane-stop-gate.sh` still reads `CLAUDE_PLUGIN_OPTION_LANE_STOP_GATE_ENABLED` from bare env, which `docs/conventions/hook-config-delivery/README.md` rule 3 forbids for a safety-critical optional-with-default toggle (fact 4: an unconfigured key lets a repo's own `settings.json` `env` block populate it freely). That fix is a channel-F reader, and the only shipped exemplar is Python (`plugins/disk-hygiene/lib/killswitch_config.py`); a bash equivalent needs the `${CLAUDE_PLUGIN_ROOT}` anchor, the fixed-path managed-settings table plus `managed-settings.d/` drop-ins, an explicit fail direction, and an entry in the convention's Adopters table. That is its own design and its own security review, not a rider on this one. Acceptance criterion 1 of #1784 therefore remains unmet; criterion 5 is only partly reachable — two of the three #969 threads become resolvable when this lands, the channel-B thread does not. #1784 remains the tracker for both. ## Related Refs #1784 Refs #969 Refs #1777 Refs #1781 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Why
A review that lands after a merge has nowhere to go:
required_review_thread_resolutionis a merge-time predicate that already passed;Six findings — one P1 — posted 46 seconds after #1720 merged and sat unread for a day. They
surfaced only because a later session happened to audit that merge batch. Nothing was bypassed; the
gate was satisfied because the threads did not yet exist.
The morning brief is the right home: read-only, unattended, and already where attention signals land.
What it does
Compares each unresolved thread's first-comment timestamp against the PR's
mergedAt, andreports only threads the gate could never have seen. A thread that predates the merge was visible to
the gate — that is an ordinary unresolved thread, not this failure mode, and it stays out.
are one thing to go look at; repeating the title per thread buries every other PR. Collapsing on
the worst severity means a P0 sitting beside advisory findings can never be softened.
--stranded-days(default 3) — wide enough to cover slow bot review and an operator-absentweekend.
It fails loud, not clear
A GraphQL error document is well-formed JSON that simply carries no
data. The extraction wouldyield an empty list and render "every merged PR in the window is clear" — an all-clear asserted
from an answer never received, which is the same fail-open shape this section exists to catch.
This is not hypothetical: a rate-limit error did exactly that during development. An API error now
says explicitly that it is not an all-clear, and prints the message. Covered by a regression case.
This is a standing leak, not a one-off
Its first live run against this repository immediately surfaced four more stranded findings on
other merged PRs — including a P1 on #1694 (merged
05:04:45Z, finding posted05:05:20Z, 35seconds later) recording that a shipped
autonomycell never reached installations.Verification
morning-brief.test.sh: 30 → 63 cases, 0 failures.thread, and a merge outside the window must all stay silent, or the section is noise rather than
signal. Plus: collapse-does-not-soften-severity, highest-severity-first, window-widening, and the
API-error case above.
shellcheck -xon script and test — clean. OneSC2016is declared, not blanket-suppressed: the$owner/$name/$endCursorin the GraphQL query are server-side variables bound by-Fandmust reach the server unexpanded.
node scripts/validate-plugin-contracts.mjs— 43 setup skills, 2153 files, pass.npx markdownlint-cli2on both changed markdown files — 0 errors.Live run — posted in full in the comments below
A live run on the current branch found 44 merged PRs carrying post-merge findings in a five-day
window: 0 P0, 10 P1, 34 P2. Among the P1s: #1503, a guardrail-bypass fix whose own review
landed unread, and #1322 with 5 findings.
Read the second comment for the authoritative figures — the first was produced by the
pre-review severity logic and reported a false P0, which review then caught. No truncation warning
fired, so the read is complete.
The five-day window filter was spot-checked against
mergedAt(a PR numbered #969 in a 5-day windowlooks wrong until you check: it merged
2026-07-25, 4.2 days before the run).This is a far larger leak than the six findings that exposed it.
Related
Closes #1777
Refs #1720
Refs #1759