[kyle-sexton]

No related issue: implements the owner-approved review policy (one review venue per lane, decided at run time); the workflow-side changes it needs are listed under Related.

Summary

Keeps the local review skills but runs a local AI review only where CI does not cover it. A new read action, /source-control:pull-request review-venue, reports per lane (code-review, security-review) whether the repository's CI review lane is present and healthy (ci) or a local review must stand in (local: absent, failing, which includes a usage-limit stop, or stale). Prep, ready and the work CI mode act on it. Also fixes the readiness contradiction that called a usage-limited lane "safe to proceed".

Fix

• Detection, one place. skills/pull-request/scripts/review-venue.sh (+ review-venue.test.sh) and reference/review-venue.md own the rule; the GitHub facts sit in reference/providers/github/pull-request/review-venue.md (ADR 0060: generic skill text names the operation, the provider file names gh). A lane is a caller workflow whose pull_request runs reference melodic-software/ci-workflows/.github/workflows/pr-review.yml or pr-review-security.yml (referenced_workflows). A lane whose workflow is not active (disabled manually, for inactivity, or deleted) is local absent. Healthy = its newest pull_request run with a verdict (completed, not skipped or cancelled) succeeded within 7 days (--max-age-days). Any read failure treats every lane as local, so an unread venue never skips a review.
• prep.md §1.2 / table / §1.6: skip the code review when code-review reads ci, report the venue line; otherwise run it as the CI lane's stand-in.
• ready-for-review.md §2.5.3 / §2.5.6: re-read the venue before the reviews; a ci lane is a built-in skip like docs-only; reports name the venue line either way.
• SKILL.md: review-venue action row, argument hint, read-action list, a short section, and the prep/ready rows.
• readiness.md: Gate 2 adds that a usage/rate-limited review lane is ABSENT, never informational; the verdict template line no longer says "safe to proceed".
• work-items work CI mode step 4: skip the review when the runner-written review-venue.txt has a code-review ci line (record review as skipped, note it in not_done); otherwise review as before.
• Changelog fragments for source-control and work-items (minor each).

All edits to existing skill text are additions or rewording; nothing was cut.

Verification

• review-venue.test.sh: 38 assertions pass, black-box with a stubbed gh; two hand mutants (skipped counted as a verdict; non-failure counted as healthy) each fail 2 assertions.
• Live run of review-venue.sh: melodic-software/claude-code-plugins and melodic-software/standards both lanes ci healthy; cli/cli both local absent.
• /skill-quality:check over plugins/{source-control,work-items,review}/skills: 26 passed, 0 failed (pull-request, work, code-review, security-review PASS; warnings are advisory length/growth notes).
• scripts/check-affected.sh --tests: 8 passed, 1 skipped (no changed .py).
• Not verified: no eval run for the edited skills (additions only); the work CI-mode path is unexercised until the runner writes review-venue.txt.

Review round 1 (Codex, claude-review, claude-security-review) found 6 issues plus a typo CI failure; all were VALID and fixed. The "Converge across rounds" section was removed, not patched: it had no observed-stumble evidence (no lane posted findings on more than 3 heads across the last 80 merged PRs), restated a volatile upstream cap, and its no-repeat rule trusted any PR comment.

Related

Needed outside this PR (not edited here; .github/** is out of scope):

• .github/workflows/work-resolve-issue.yml (or its runner script): before the agent step, run plugins/source-control/skills/pull-request/scripts/review-venue.sh with a token holding actions: read and write its stdout to review-venue.txt in the scratchpad directory the agent reads issue.json from. On a non-zero exit, write nothing (the skill then reviews).
• .github/scripts/work-resolve-issue/post-statuses.mjs: a review step reported skipped currently posts no pipeline/review status. If the merge gate or a ruleset expects pipeline/review, post it as success with fixed text such as "Covered by the CI code-review lane." when the outcome's not_done carries the code-review ci line, and update post-statuses.test.mjs.
• Policy rows 1-4 and 9 of the review-policy change list (fallback token retry, scheduled re-run, pr-verify narrowing, account= parsing) stay separate.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-17T17:58:27.450431Z	26db4a5	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 3m 17s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Invoke /review:security-review per custom instructions
• Post findings / summary

Scope

Reviewed all 15 files changed in this PR (git diff origin/main...HEAD), with focus on the new review-venue.sh script (shells out to gh/jq) and the new "Converge across rounds" instruction-surface additions to the review skills, per the instruction-surface lens this lane applies to standing-instruction changes.

Findings (1)

IMPORTANT — plugins/review/skills/security-review/SKILL.md lines 144-153 (same pattern in plugins/review/skills/code-review/SKILL.md lines 113-121): the new "Converge across rounds" rule decides whether to suppress a finding as a repeat by reading gh pr view <n> --json reviews,comments with no instruction to filter by author/identity. Since that API returns every comment on the PR, not just this lane's own, an attacker who can comment on the PR could post a forged comment mimicking this lane's past review output (e.g. "CRITICAL: <real bug>, already reported, open"), causing a later round to treat a genuine, never-actually-reported vulnerability as a duplicate under the "No repeats" clause and silently drop it. The repo already mitigates this exact forgery class elsewhere (plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py's MARKER_AUTHOR/CLAUDE_APP_LOGINS/claude-lane: checks) but that filter wasn't carried into this new text. Fix: require matching the lane's own authenticated bot identity or the claude-lane: trailer before treating any reviews/comments entry as this lane's own prior output.

Posted as an inline comment on plugins/review/skills/security-review/SKILL.md.

Reviewed, no issues found

• scripts/review-venue.sh / review-venue.test.sh: --repo is validated against ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ before use, --max-age-days against a numeric pattern, workflow IDs are validated as digits before interpolation into a further gh api call, and jq filters receive untrusted-shaped values only via --arg/--argjson. No command/argument injection path.
• readiness.md: the usage-limit wording change tightens a guardrail (no longer calls a limited lane "safe to proceed"); not a weakening.
• review-venue.md (both copies), prep.md, ready-for-review.md, SKILL.md (pull-request), work/SKILL.md: the venue read fails closed everywhere (unread/absent/failing/stale all fall back to running the local review), so a repo-level read never skips a review it shouldn't.

claude-lane-summary: lane=claude-security-review sha=26db4a586739fdc4e070188596b3d38793c5f377 run=39055838085

[claude]

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

---

Code review complete

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md)
• Invoke /review:code-review and follow it for this PR
• Post inline findings (none survived verification)
• Final summary

Summary

Reviewed the full diff (.claude-lane/pr.diff, 15 files) against the code-review lane's criteria
(correctness, maintainability; security is out of scope here since this repository runs a separate
pr-review-security-hosted.yml lane).

Most of the change is prose additions to skill/reference markdown (prep.md, ready-for-review.md,
readiness.md, both SKILL.md files, the new review-venue.md reference pair) plus one new script,
plugins/source-control/skills/pull-request/scripts/review-venue.sh, and its test suite. I traced the
cross-references between these files (e.g. readiness.md Gate 2's new bullet against Gate 5's
lane-substitution item, the ready-for-review.md §2.5.3/§2.5.6 wording, the work/SKILL.md
review-venue.txt line-prefix check against the script's actual output strings) and didn't find a
contradiction.

I looked hardest at review-venue.sh itself:

• The default REPO="{owner}/{repo}" (no explicit --repo) initially looked like a bug — unlike
  sibling scripts fetch-annotations.sh/fetch-failed-logs.sh, which resolve the repo via
  gh repo view — but {owner}/{repo} is gh api's own placeholder syntax, substituted from the
  current repository context, and this exact pattern is already an established convention elsewhere
  in this codebase (reference/providers/github/pull-request/create.md:13, README.md:25,
  CHANGELOG.md:3080). Not a bug.
• Pagination, the per-lane workflow-id lookup, the jq verdict-selection filter (completed, not
  skipped/cancelled), the staleness math, and the exit-code contract all check out against the
  stated behavior and the 32-assertion test file.

No findings met the high-signal bar (something a careful senior reviewer would block or flag with a
concrete failing input). 0 inline comments posted.

Not independently verified (author-claimed only): the review-venue.test.sh 32-assertion pass
count, the live gh run against real repositories, and the /skill-quality:check results — I did
not re-run these myself.

claude-lane-summary: lane=claude-review sha=26db4a586739fdc4e070188596b3d38793c5f377 run=39055838087

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26db4a5867

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

[github-actions]

claude-security-review has reviewed this pull request through b1358cb; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through b1358cb; a later push is reviewed from there.

[claude]

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

---

Incremental code review complete

• Gather context (incremental diff, custom instructions)
• Invoke /review:code-review for the incremental scope
• Post findings / summary

Scope

This round's diff (.claude-lane/incremental.diff) since 26db4a5, limited to:

• .changes/source-control/feat-skills-ci-review-detection-fcfa6216.md
• plugins/source-control/reference/providers/github/pull-request/review-venue.md
• plugins/source-control/skills/pull-request/reference/review-venue.md
• plugins/source-control/skills/pull-request/scripts/review-venue.sh
• plugins/source-control/skills/pull-request/scripts/review-venue.test.sh

This push addresses the two prior Codex findings on review-venue.sh: it now reads the caller
workflow's state via gh api repos/{owner}/{repo}/actions/workflows/<id> and treats anything
but active as local absent (review-venue.sh:100-106), and it narrows the per-workflow runs
query to event=pull_request (review-venue.sh:107) so a workflow_dispatch/scheduled success can't
mask a failing PR run.

Checked

• state=$(jq -er '.state | strings' <<<"$wf"): verified directly that a missing or null
.state makes jq -e exit non-zero (no output), so || die 2 fires and the read fails closed
  rather than silently treating an unreadable state as healthy.
• $id is still validated as ^[0-9]+$ before interpolation into the new gh api repos/$REPO/actions/workflows/$id call — no injection path opened by the new call.
• Traced both new test cases (disabled_manually/disabled_inactivity lanes, and a
  workflow_dispatch success paired with a pull_request failure) against the stub's routing
  and the script's call order (state check before the runs call) — they match the implementation.
• Doc updates (reference/providers/github/pull-request/review-venue.md,
  skills/pull-request/reference/review-venue.md, the changelog fragment) stay consistent with
  the script's new behavior and output strings (local absent now covers "disabled").

No findings. 0 inline comments posted.

claude-lane-summary: lane=claude-review sha=b1358cbd4cc056e0c46229c8111a761062b5f1f2 run=39056505193

[claude]

Claude finished @kyle-sexton's task in 1m 7s —— View job

---

Security review complete

• Gather context (incremental diff, custom instructions)
• Invoke /review:security-review for the incremental scope
• Post findings / summary

Scope

Incremental review of the diff since 26db4a5 (.claude-lane/incremental.diff), limited to the 5 files named in scope: both review-venue.md reference copies, review-venue.sh, review-venue.test.sh, and the changelog fragment. gh pr diff used only for surrounding context.

Findings (0)

This diff is a hardening-only change that fixes exactly the two issues chatgpt-codex-connector flagged on the prior commit against review-venue.sh:

• P1 (workflow must be active): now checked via gh api repos/$REPO/actions/workflows/$id → .state, with $id still validated as ^[0-9]+$ before interpolation. Any state other than active is treated as local absent. A read failure or a missing/non-string .state (jq -er '.state | strings') dies with exit 2, which the caller (and review-venue.md's documented contract) treats as "every lane local" — fails closed, so a broken state read never causes a review to be skipped.
• P2 (restrict health history to PR runs): the per-lane runs listing now requests ?event=pull_request&..., so a workflow_dispatch/schedule success on the same workflow can no longer mask a failing pull-request run. review-venue.test.sh adds a case (wf 11 completed:success:1:workflow_dispatch completed:failure:2) asserting the manual success doesn't mask the PR-run failure.

No new vulnerability surface: $REPO and $id keep the same validation as before interpolation into gh api, the new jq filters take untrusted JSON only via stdin/--arg/--argjson (no string-built filters), and $path/$state are only ever printf'd, never executed. The disabled-workflow and PR-run checks are two sequential reads with no attacker-controlled input between them (changing a workflow's enabled state requires write access to the repo already), so the narrow TOCTOU window between them isn't an exploitable path for an external contributor — it's a reliability edge case, not a security finding.

The test-stub additions in review-venue.test.sh (new */actions/workflows/*) case, wf_state helper) are test-only code operating on script-validated inputs; no injection path there either.

No inline comments posted (nothing met the high-signal bar).

claude-lane-summary: lane=claude-security-review sha=b1358cbd4cc056e0c46229c8111a761062b5f1f2 run=39056505177