fix(source-control): drop Refs #N from pull-request create opt-out gate (#630) - #651
Conversation
…te (#630) The §2.4.2 pre-create gate's OPTOUT_REGEX accepted a bare `Refs #N` as a closing-keyword opt-out, but the real pr-issue-linkage reusable CI workflow accepts only a native closing keyword or a `No linked issue` / `No related issue:` phrase — `Refs #N` is not in that set. A `Refs #N`-only body cleared the skill's own gate yet failed CI on push. Drop `Refs #N` from OPTOUT_REGEX (`^No related issue:` only) so any body the local gate passes the validator also passes (strict safe subset). Reconcile the §2.4.0 orphan-PR prompt, §2.4.1 asymmetry note, and §2.4.2 gate messages; `Refs #N` remains valid as a link-without-close reference in `## Related`. Bump source-control 0.13.1 -> 0.13.2 with a CHANGELOG entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 53s —— View job PR Review
SummaryThis is a clean, narrow bug fix that restores the safety property of the local pre-create gate: every body the skill's gate passes, the real Correctness
The old The safety-subset invariant is now satisfied in both directions:
Case-sensitivity asymmetry — intentional and correct.
Prose ConsistencyAll five remaining
Zero occurrences present CHANGELOG and Version Bump
No FindingsNo bugs, security concerns, stale references, or prose inconsistencies found. The PR does exactly what it says, no more. Approved. |
|
Claude finished @kyle-sexton's task in 1m 29s —— View job
SummaryThis is a correct, well-scoped fix that restores the safety-subset invariant: every body the skill's §2.4.2 gate passes, the real Gate Logic — Correct
The old Case-sensitivity split is intentional and correct. One Stale Inline CommentLine 134 still reads: ISSUE_NUM="" # fall through to orphan-PR 3-option prompt belowThe orphan-PR prompt was reduced from three options to two in this PR (lines 148–152 now list only Prose Consistency — CleanAll five remaining
Zero occurrences present CHANGELOG and Version Bump
VerdictThe core fix is correct. One stale |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a030d47da
ℹ️ 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".
…an-PR eval (#630) Resolve two internal contradictions the §2.4.2 opt-out fix left behind: - §2.4.0 multi-issue prompt appended accepted `Refs #Y` lines to ${CLOSES_LINE} (rendered on the closing-keyword line), contradicting the new "Refs never on the closing-keyword line" invariant. Refs lines now route into `## Related`; `Closes #X` still carries the closing-keyword line. - evals.json eval 8 (closed-branch-issue-does-not-autoclose) still described a three-option orphan prompt (Closes, Refs, no-related-issue); aligned to the two-option prompt (Closes or No related issue:). - Stale inline comment at the branch-issue fallback said "3-option prompt"; corrected to "2-option". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 17s —— View job
SummaryThe second commit ( Codex Finding 1 — evals.json eval 8: Resolved
Codex Finding 2 — Multi-issue Refs routing: Resolved
Stale
|
|
@claude re your line-134 finding:
Corrected to |
…ce (#642) (#666) ## Summary The babysit readiness gate blocks while source findings outnumber their per-finding classification rows. The shared classifier counted a self-authored classification pipe-row in ANY comment, including PR-level review-summary comments that are never thread-resolved. A review thread's findings are discounted when it resolves (the #465 lifetime-vs-open guard), but a PR-level comment can never be — so a stale classification posted outside a thread kept counting after its finding was discounted, inflating the classified count past a fresh, still-unclassified open-thread finding and emitting a fail-open `READINESS_OK`. This was the single live fail-open on `main` and the ratified backstop for the #476 gate-off flip: per the backstop clause it must be fixed before the flip. The dispatched fix shape ("non-thread PR-level comments never contribute to `classified`") turned out unsafe on verification: `review-discipline.md` §D5 *mandates* that issue/review-level (PR-level) findings be answered with a detached PR-level classification comment. Thread-only counting would count every such classification as zero and permanently block any PR whose findings come from review summaries — and break the Python↔bash convergence. Corrected in agreement with the tower. ## Fix - **Per-surface classification credit (`babysit_classify.py`).** `count_effective_classified` buckets comments by surface and caps credit within each bucket via `min(classified, findings)`, then sums — a classification can only offset a finding on its own surface, so a stale PR-level row can no longer spill over to cover an open-thread finding. `count_classified` and `count_findings` stay pure raw counters; the bucketing composes them. On unsignalled input every comment lands in one bucket and this collapses to `min(classified, findings)`, preserving existing behavior and the bash convergence property. - **Surface discriminator (`comment_surface`).** Three surfaces — review-thread, PR-level, and an isolated bucket for comments bearing no surface signal — resolved from two signals in order: the explicit `in_review_thread` stamp (authoritative when present), then the `fetch-all-pr-comments.sh` `type` tag on the `--comments-json` reuse path (`inline` → thread; `general`/`review` → PR-level). A comment with neither signal is isolated so its rows cannot offset — and its findings cannot be offset by — a known surface (fail-closed for unknown provenance), preserving the "no signal = PR-level lifetime" model `thread_is_open` documents. - **Surface stamping (`babysit_findings.py`).** `_comment()` records `in_review_thread` (true only when fetched from a review thread); issue-level and review-summary comments are stamped PR-level. The entrypoint emits the effective count. - **Bash degrade cap (`babysit-readiness-gate.sh`).** The thread-blind safe-tier degrade gains the thread-state-free analogue `classified = min(classified, findings)` so a row over-count can't mask a finding and the degrade stays convergent with the Python `min` on unsignalled input. Per-surface bucketing is inherently surface-aware and remains Python-only, exactly like the #465 discount. ## Behavior flip (documented, fail-closed) A PR whose inline-thread findings are answered only by detached PR-level classification replies now reports `READINESS_BLOCKED` where it previously passed — a PR-level row no longer offsets an inline-thread finding. This mechanically enforces §D5's already-ratified reply routing (inline findings MUST reply threaded, "NEVER a detached `pr comment`"). Runs already following §D5 are unaffected; only runs relying on the previously-tolerated detached-reply shape change verdict. ## Residual (documented, not closable here) An orphaned PR-level classification covering a fresh *PR-level* finding is irreducible: GitHub's flat issue comments carry no finding↔classification linkage, so a stale PR-level row is data-identical to a live one. It is reachable only via a §D5 routing violation (an inline finding answered with a detached PR comment) or a reviewer editing/deleting a finding; a §D5-compliant, no-edit run never hits it. The per-surface fix closes the entire linkable (thread) side, on both the live and `--comments-json` reuse paths. ## Verification - `python -m unittest discover -s tests` — 269 tests OK, incl. new `EffectiveClassifiedTests` (per-surface credit, reuse-path `type` inference, explicit-stamp precedence, isolated-unknown), `SurfaceStampingTests`, and `Main642FailOpenTests`. - `babysit-readiness-gate.test.sh` — 60 pass, incl. the live-path #642 scenario, the reuse-path inline-`type` scenario, the unsignalled-provenance isolation scenario (all Python-gated like #465), and all five convergence cases including the over-classified cap. - `engine.test.sh` — pass (unittest + `ruff check`); shellcheck + shfmt clean; `markdownlint-cli2` + `validate-plugins.sh` clean. ## Versioning - `plugins/source-control/.claude-plugin/plugin.json` → **0.13.3**; CHANGELOG entry under `[0.13.3]` documenting the fail-open closure and the §D5 PASS→BLOCK behavior flip. `origin/main` merged in (0.13.2 base from #651). ## Related Closes #642 - #534 / #634 — shared babysit classifier this hardens (found during the #634 digest). - #465 — the lifetime-vs-open discount this mirrors for classifications. - #476 — gate-off flip this fail-open blocks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
main advanced source-control to 0.13.3 (#651, #666) while this branch bumped from an older 0.13.1 base; resolve the version and CHANGELOG conflict by re-homing this PR's entry as [0.13.4] on top of main's [0.13.3], keeping all main entries intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
Summary
The
pull-requestcreate flow's §2.4.2 pre-create gate treated a bareRefs #Nline as a valid closing-keyword opt-out, but the realpr-issue-linkagereusable CI workflow does not acceptRefs #N. A PR body that satisfied the skill's own local gate could therefore still fail the CI gate on push. This is the narrow, same-repo fix (issue #630, option 1): align the skill's local opt-out marker set to the validator by droppingRefs #N.Fix
In
plugins/source-control/skills/pull-request/reference/create.md:OPTOUT_REGEXdropsRefs #N:^(Refs #[0-9]+|No related issue:)→^No related issue:. A rationale comment explains whyRefs #Nis excluded. The two abort-message lines no longer listRefs #Nas an opt-out.Closes #N/No related issue:); aRefs #Nlink-without-close reference is now explicitly routed to the## Relatedsection instead of the closing-keyword line.Refs #Nline satisfies the closing-keyword half of neither the §2.4.2 gate nor the real validator.Refs #Nas a silent-pass path.Refs #Nremains valid where it always belonged — as a link-without-close reference inside the## Relatedsection (§2.4.1) and in the multi-issue prompt where a primaryCloses #Nalready carries the gate. Extending the upstream validator to acceptRefs #N(option 2) requires a change inmelodic-software/ci-workflowsand is out of scope for this lane.source-controlbumped 0.13.1 → 0.13.2 (patch — contract-alignment bug fix) with a matching CHANGELOG entry at the top.Verification
The real validator's accepted set — read live from the pinned reusable source (
melodic-software/ci-workflows/.github/workflows/pr-issue-linkage.yml@90f1c54935203fa31b5b3d1f41531228be2c2b7f, the SHA this repo's.github/workflows/pr-issue-linkage.ymlpins). Its closing-keyword half accepts exactly two forms:Refs #Nmatches neither.The skill's local gate after this change (§2.4.2):
Refs #Nno longer matchesOPTOUT_REGEX— the specific false-accept this issue reports is eliminated.Marker-set relationship. The two sets are not literally identical, and this PR does not claim they are: the validator additionally honors
No linked issue, owner/repo-qualified keywords, and looser-whitespace keyword forms that the line-anchored local gate does not. What now holds is the safety property that was broken: every body the local gate passes, the validator also passes (the local gate is a strict safe subset). The one marker the local gate accepted that the validator rejects —Refs #N— has been removed, so a "skill-gate-satisfied" body can no longer fail the real CI gate on this axis. The skill only ever emitsNo related issue:(never the siblingNo linked issuephrasing), so the local gate is deliberately mirrored to what the skill produces, not widened to the validator's full superset.No stale references remain. Grep of the whole plugin for
Refs #:Every surviving
Refs #is either legitimate## Related/ multi-issue linking or prose that now statesRefs #Ndoes not satisfy the gate. No occurrence presentsRefs #Nas a closing-keyword opt-out.Closes #630
Related
## Relatedscaffold to the create flow and documented thisRefs #Nopt-out asymmetry in §2.4.1 as a known, deliberately-out-of-scope gap; this PR is the follow-up that closes it.🤖 Generated with a Claude Code implementation subagent (issue #630)