test(autonomy): lane-stop-gate test hygiene from #2065 verification

[kyle-sexton]

Fixes #2086

Summary

• rc0 rejects an empty exit-code argument.
• FIFO hang case skips when mkfifo fails and uses timeout instead of kill -9 on a subshell.
• Hook comments document MSYS umask advisory posture and mid-lane downgrade over-gating.

Test plan

• plugins/autonomy/hooks/lane-stop-gate.test.sh (83/0)

Related

• Fixes test(autonomy): five low-severity items from the lane-stop-gate arm-claim verification #2086

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 2m 4s —— View job

---

Security review

• Fetch PR diff (origin/main...HEAD)
• Review plugins/autonomy/hooks/lane-stop-gate.sh
• Review plugins/autonomy/hooks/lane-stop-gate.test.sh
• Review plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py
• Review plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py
• Review plugins/source-control/skills/babysit-prs/reference/independent-resolution.md
• Review plugin.json / CHANGELOG.md diffs (metadata only)
• Post findings

No security issues found. No GitHub Actions workflow files are touched by this PR, so the zizmor-covered lane (supply-chain pins, dangerous triggers, permissions, template injection) doesn't apply here anyway.

What I looked at, from a logic/trust-boundary angle:

• lane-stop-gate.sh — the diff is comment-only (documents the pre-existing MSYS umask 077 best-effort posture and the mid-lane-downgrade over-gating window). No behavioral change to the claim-ownership/arm-record logic.
• lane-stop-gate.test.sh — test-only hygiene (skip the FIFO-hang case when mkfifo isn't available instead of failing spuriously; timeout instead of kill -9 on a subshell that could orphan a grandchild process; rc0 now rejects a missing/empty exit code instead of silently passing [[ "" -eq 0 ]]). No production-hook impact.
• babysit_resolve_thread.py — this is the one change worth scrutinizing closely, since it touches _has_severity_marker(), the guard behind the documented "never a security or P1 thread" bright line for the --autonomous / --independent-resolver unattended paths. The old pattern (\bP[01]\b, case-insensitive) is replaced with two structured-only patterns: /badge/P[01]- and \[P[01]\], both case-sensitive, dropping bare-prose and lowercase-marker matches (see L204-L207).
  • At first read this looks like it narrows a security gate, which is exactly the kind of change this review is meant to catch. On closer inspection it converges this guard with the already-established, deliberate case-sensitive convention used for the same badge/bracket markers in babysit_classify.py (SEVERITY_BADGE_RE, SEVERITY_PLAIN_RE) and babysit-readiness-gate.sh, both of which are explicit about staying case-sensitive so a lowercase priority:p0-critical label doesn't false-positive. Severity badges/brackets in this pipeline are bot-emitted (codex/chatgpt-codex-connector), not free-typed by an adversarial human, and the PR links the rationale to a real false-positive incident (babysit-resolve-thread: independent-resolver severity guard false-positives on P2 threads whose prose mentions P1 #1939) with matching tests. The vetted --resolve --thread-id path's "no severity screen" behavior is pre-existing and only gets clarified in the docs here, not changed.
  • I don't think this rises to a reportable finding, but flagging the reasoning for the record: if a hostile actor could ever get their own PR-review-comment text treated as this scan's input (rather than only trusted-bot output), a lowercase [p1]//badge/p1- marker would now slip past _has_severity_marker() where the old regex would have caught it. Worth keeping in mind if the comment-source trust model for this scan ever widens.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ce47eda84

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 4m 25s —— View job

---

Code Review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review plugins/autonomy/hooks/lane-stop-gate.sh
• Review plugins/autonomy/hooks/lane-stop-gate.test.sh
• Review plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py
• Review plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py
• Review docs/changelog/plugin.json changes
• Post findings

Root and .claude-pr/CLAUDE.md are both empty on this branch, so no repo-specific conventions to check beyond REVIEW.md. This repo has .github/workflows/claude-security-review.yml, so security-scope findings (none surfaced anyway) are left to that lane per REVIEW.md.

Findings

One inline finding posted on babysit_resolve_thread.py — dropping re.IGNORECASE from the new structured-marker regexes (lines 204–205) looks like it silently reopens a gap the PR's own stated goal (excluding bare prose P0/P1 mentions, #1939) didn't call for: a lowercase [p1] marker no longer trips the --autonomous severity guard, and the accompanying test was rewritten to assert that as correct rather than catching it as a regression.

Other observations (no inline anchor needed / lower confidence)

• Scope vs. description mismatch. The PR title/body describe only the lane-stop-gate hygiene fixes (rc0, FIFO test, hook comments) and cite only test(autonomy): five low-severity items from the lane-stop-gate arm-claim verification #2086. The diff also rewrites babysit_resolve_thread.py's severity-marker detection (a real behavior change, documented in plugins/source-control/CHANGELOG.md under babysit-resolve-thread: independent-resolver severity guard false-positives on P2 threads whose prose mentions P1 #1939) but that's not mentioned anywhere in the PR body. Both changes are individually reasonable and each has its own CHANGELOG entry, but a reviewer reading only the PR description would miss that a second, independently-shippable, behavior-affecting change is bundled in.
• Test plan gap. The PR's Test plan lists only lane-stop-gate.test.sh (83/0), not the Python suite (test_babysit_resolve_thread.py) that was also modified. I was not able to independently execute the shell or Python test suites in this sandbox (command execution required approval I couldn't obtain), so I can't confirm either suite's current pass state myself — flagging this as author-claimed/unverified rather than something I checked.
• plugins/autonomy/hooks/lane-stop-gate.sh changes are comments-only (MSYS umask advisory note, mid-lane downgrade over-gating note) — accurately describe the existing code paths, no functional risk.
• The FIFO-hang test rewrite (lane-stop-gate.test.sh) — skip-if-mkfifo-unavailable, timeout 5 instead of a background PID + kill -9 — is a genuine improvement: GNU timeout's default (non---foreground) process-group signaling reaches the whole printf | env … bash "$HOOK" pipeline, which is more reliable than killing only the wrapping subshell PID as before. The rc0 empty-argument guard is a reasonable defensive hardening, though at current call sites the argument is never actually empty ($? or an already-guarded "$(cat … || echo 1)"), so it's inert today rather than fixing a live bug.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31571181871
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31571181874
• Failure class: other
• Last SDK result: (no execution file was produced)

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).