cluster: fix PR-body auto-close trap (#87 #74)#94
Merged
Conversation
…ate (Refs #87 #74) The canonical PR-body template embedded `Closes #${N}` in the anti-trailer warning. Skills (idd-implement, idd-all) copied this pattern; on heredoc substitution it becomes a real `Closes #<num>` that GitHub auto-close matches context-blind, bypassing /idd-close. Reword to a digit-free form so no `<keyword> #<number>` substring can exist. Refs #87 #74
…lates (Refs #74 #87) idd-implement Step 5.5 and idd-all Phase 5 PR-body heredocs substituted ${NUMBER}/${N} into the anti-trailer warning, producing a real `Closes #<num>` that GitHub auto-close matched on merge — bypassing /idd-close's gate. Confirmed incidents: #559, che-apple-mail-mcp#99, #73, #56. Reword all to the digit-free form. idd-all-chain already used literal '#N' (safe) but is aligned to identical wording so the safe pattern is uniform and cannot be re-parameterized. Refs #74 #87
…efs #74 #87) Defence-in-depth gate complementing the #87/#74 template rewords. In PR mode, scans the rendered PR body for GitHub auto-close trailers (closes|fixes|resolves #<digit>) and warns — these would auto-close the issue at merge, bypassing /idd-close's checklist gate. Warn-only (a PR body may legitimately quote the keywords in prose). Adds matching scan_pr_body_trailers entry to the Step 0 bootstrap TaskList. Refs #74 #87
…false positive (Refs #74 #87) The initial Step 0.8 regex used \b word boundaries, which matched the 'close' inside '/idd-close #N' — an IDD skill invocation instruction, not a GitHub auto-close trailer. GitHub treats 'idd-close' as one hyphenated token and does NOT auto-close it (verified: PR #94 closingIssuesReferences is empty despite its body containing '/idd-close #87 #74'). Replace \b prefix with (^|[^-/[:alnum:]]) so the keyword must not be preceded by '-', '/', or an alphanumeric — precisely matching GitHub's behavior while still catching context-blind hits like (Closes #N) / **Closes #N**. Found by dogfooding the new gate against this PR's own body. Refs #74 #87
Contributor
Author
Verify Report — PR #94 (cluster #87 + #74, auto-close trap)6-AI ensemble: 5 general-purpose reviewer Agents + Codex (gpt-5.5 xhigh), independent. Aggregate verdict:
|
| # | Severity | Finding | Source | Action |
|---|---|---|---|---|
| H1 | HIGH | Step 0.8 regex misses the colon form Closes: #87. GitHub documents Closes: #10 as a valid auto-close form; the regex demands [[:space:]]+ immediately after the keyword, so a : breaks the match. A warn-only "defence-in-depth" gate that silently misses a documented form gives false assurance. |
security+devils-advocate (both reproduced) | Blocking — Round 2 |
| H2 | HIGH (disputed) | Step 0.8 regex misses cross-repo closes owner/repo#87. Security reviewer cites GitHub docs that this auto-closes. However #74's own issue body reports first-hand that cross-repo auto-close did not fire (che-apple-mail-mcp#76 PR #61). The dispute is itself the reason to stop reimplementing GitHub's parser by regex. |
security+devils-advocate | Resolve via Round 2 redesign |
| L1 | LOW | Step 0.8 regex would false-positive on the implausible string foo_closes #N (_ not in the prefix-exclusion class). Harmless (warn-only). |
logic | moot if regex dropped |
| L2 | LOW | idd-implement/SKILL.md is a CRLF-line-ending file (632 CR chars, pre-existing); the reworded line 487 now trips git diff --check as "trailing whitespace". Not introduced by the logic change — the whole file predates this PR as CRLF. |
codex | see Round 2 |
| L3 | LOW | The reworded warning names only Closes/Fixes/Resolves; GitHub also honors Closed/Fixed/Resolved and bare Fix/Resolve. A reader might think Fixed #87 is safe. |
security | optional polish |
Process note — under-tested PASS
Requirements / Logic / Regression reviewers all PASSed Step 0.8 but only exercised the naive Closes #N family. The Security reviewer and Devil's Advocate independently tested the colon + cross-repo forms and found H1/H2. The Devil's Advocate explicitly flagged the 3 unconditional-PASS verdicts as incomplete test matrices. Aggregate verdict follows the adversarial finding, not the majority.
Confirmed solid (could not be broken)
- All 4 template rewords — digit-free, no substitution path to a trailer. Root cause eliminated at source. (all 6 AI agree)
- No scope creep — diff touches exactly the 5 planned files. Heredoc syntax intact.
idd-all-chaincosmetic align introduced no${N}.- Step 0.8 correctly excludes
/idd-close #Nskill invocations (the5831128regex fix holds — verified against PR cluster: fix PR-body auto-close trap (#87 #74) #94's own body,closingIssuesReferences: []). - No shell-injection surface in Step 0.8 (
$PR_BODYpiped togrep, nevereval'd).
Out-of-scope (noted, not blocking)
- Effectiveness gap (Devil's Advocate): the fix lands in plugin source; installed users keep the buggy template until a plugin release + marketplace sync. This is documented in the plan's Out-of-scope — a post-merge release action, not a PR defect.
- warn-only vs hard-block (Devil's Advocate Challenge 3): plan decision D3 chose warn-only. DA argues an authoritative signal could justify hard-block — see Round 2 decision.
Recommendation: Round 2 — redesign Step 0.8 to query gh pr view --json closingIssuesReferences (GitHub's own authoritative parse of what the PR will auto-close) instead of reimplementing GitHub's keyword parser by regex. This closes H1, resolves the H2 dispute, and removes L1 — all at once, with simpler code. Then re-verify.
Generated by /idd-verify --pr 94 — 5 reviewer Agents + Codex.
…d-rolled regex (Refs #74 #87) R1 verify (/idd-verify --pr 94) found the Step 0.8 regex missed the colon form 'Closes: #87' — a GitHub-documented auto-close form. Reimplementing GitHub's keyword parser by regex is inherently fragile (colon form, cross-repo form, issue-URL form all need separate handling, and cross-repo behavior is itself disputed). Replace the regex with GitHub's own authoritative parse: gh pr view --json closingIssuesReferences lists exactly which issues the PR will auto-close on merge — covering every trailer form GitHub honors, with zero false positive (it IS GitHub's determination) and zero parser-reimplementation risk. This supersedes the regex from 3ba7435 + the regex false-positive fix from 5831128; /idd-close #N skill invocations naturally never appear in closingIssuesReferences so no special exclusion is needed. Refs #74 #87
Contributor
Author
Verify Report — PR #94 ROUND 2 (cluster #87 + #74)6-AI ensemble: 5 reviewer Agents + Codex. Scope: R2 commit Aggregate verdict:
|
| # | Severity | Finding | Source | Action |
|---|---|---|---|---|
| DEF-1 | MEDIUM | Orphan doc — idd-verify/SKILL.md:242 still carries the full "Regex 設計" paragraph describing the `(^ |
[^-/[:alnum:]])` regex that R2 deleted. Contradicts the new L219/L240 ("不自寫 regex") two lines above. R2's Edit replaced the block up to the warn-only line but left this trailing paragraph. | 6-way unanimous (all 5 Agents + Codex) |
| L-1 | LOW | 2>/dev/null || true makes a gh failure (auth/network/old gh) indistinguishable from a clean PR — silent fail-open. A defence-in-depth gate should print a one-line "could not check" note rather than skip silently. |
logic + devils-advocate + codex | Round 3 |
| L-2 | LOW | L219 prose overclaims "定義上零漏判" (zero false negative by definition). closingIssuesReferences is eventually-consistent — a short propagation window after gh pr create could return empty transiently. Claim should be scoped to GitHub's settled state. |
devils-advocate | Round 3 |
| L-3 | LOW | Codex polish: warn message prints bare .number; for a cross-repo close ref a bare number is ambiguous — .url (or owner/repo#number) is unambiguous. |
codex | Round 3 (fold in) |
| L-4 | LOW | Task key scan_pr_body_trailers is now a slight misnomer (no longer scans body text) — description is accurate though. |
requirements | optional |
Confirmed solid
- R1's H1 (colon) + H2 (cross-repo) structurally closed — no regex left to miss anything.
- No scope creep — R2 touched only
idd-verify/SKILL.md(18 ins / 17 del). The 4 R1-verified template rewords are untouched. - New bash: jq path correct, warn-only intact,
bash -nclean, no shell-injection surface. - No executable regex cruft — only the one doc paragraph (DEF-1).
Out-of-scope (tracked)
- CRLF on
idd-implement/SKILL.md→ filed as [refactor] idd-implement/SKILL.md has CRLF line endings — trips git diff --check #95 (separate refactor; a 632-line line-ending diff would dwarf this cluster). - Effectiveness gap (installed skill keeps the bug until a plugin release) → post-merge release action per plan Out-of-scope.
Recommendation: Round 3 — delete the orphan "Regex 設計" paragraph (DEF-1, blocking), surface the gh-failure skip with a note (L-1), soften the L219 overclaim to "settled state" (L-2), and switch the warn output to .url for cross-repo clarity (L-3). All small, all in the same Step 0.8 block. Then re-verify.
Generated by /idd-verify --pr 94 Round 2 — 5 reviewer Agents + Codex.
… use .url (Refs #74 #87) R2 verify (/idd-verify --pr 94 round 2) found 3 cleanup items, all in Step 0.8: - DEF-1 (6-AI unanimous, blocking): the R2 regex->closingIssuesReferences rewrite deleted the regex code but left the 'Regex 設計' paragraph documenting it — orphan dead doc contradicting the new prose. Deleted. - L-1: '2>/dev/null || true' conflated a gh failure with a clean PR (silent fail-open). Switched to 'if CMD; then ... else ...' so a gh failure prints a 'Step 0.8 skipped' note instead of silently passing. - L-2: softened the 'zero false negative by definition' overclaim — note that closingIssuesReferences is eventually-consistent (settled by verify time). - L-3 (Codex polish): query .url instead of bare .number so a cross-repo close ref is unambiguous in the warning output. Refs #74 #87
Contributor
Author
/idd-verify --pr 94 — Round 3 (cluster #87 #74)Aggregate verdict: ✅ PASS — 6/6 reviewers. Ready to merge. R3 (commit 6-AI verdict
R2 → R3 finding closure
Verify-fix convergence (R1 → R2 → R3)
DogfoodPR #94's own body ( Next: human review of this PR → merge → |
kiki830621
added a commit
that referenced
this pull request
May 19, 2026
…auto-close-trap fix) PR #94 already merged as d2125fe; this commit publishes the fix to the marketplace by bumping plugin.json + marketplace.json versions so '/plugin update' picks it up. Description entry summarizes the cluster fix + Step 0.8 preventive gate + R1→R2→R3 verify journey + the two follow-up issues #96 (cluster-PR vs direct-commit design conflict) and #97 (squash-commit-body trap caught #87 at merge time). Refs #87 #74
5 tasks
kiki830621
added a commit
that referenced
this pull request
May 19, 2026
…PR body (Refs #97) PR #94 (v2.60.1) added Step 0.8 to flag GitHub auto-close trailers in PR bodies via `closingIssuesReferences`. When PR #94 itself was squash-merged, GitHub auto-closed `#87` anyway — bypassing the gate. Root cause: GitHub's auto-close parser fires off any close-keyword + `#<digit>` substring in the *merge commit message*, and `gh pr merge --squash` concatenates every commit's subject + body into that message. PR #94's commit `d918270` had a body that quoted the trap pattern as a verify finding (single-quoted as an R1-regex-gap example); single quotes do not suppress the parser. Result: `#87` closed 2 seconds post-merge, bypassing `/idd-close`. Step 0.8 detection face = PR body only; trap trigger face = PR body ∪ every commit subject ∪ body landing on main. Two faces, out of sync. This change extends Step 0.8 to a 2-source scan: Source 1 (kept, GitHub authoritative — PR body): `gh pr view --json closingIssuesReferences` Source 2 (NEW — per-commit messageHeadline + messageBody, local regex): `gh pr view --json commits` + trap regex `(^|[^-/[:alnum:]])(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]*:?[[:space:]]+#[0-9]+` with `tolower($0)` for case-insensitive match. Source 2 was extended in R2 (per /idd-verify R1 Devil's Advocate + Codex finding DA-H1) to also include `messageHeadline` after empirical evidence that subject-line traps had auto-closed at least two issues historically in this repo: - commit `8ac8206` subject contained `resolves #N` form -> closed `#70` - commit `a82867d` subject contained `fix #N` form -> closed `#26` R1's jq filter emitted only `.messageBody` and would have missed both. Regex has R1 / R2 / R3 lessons baked in (from PR #94 close history): - `(^|[^-/[:alnum:]])` prefix excludes IDD skill invocations like `/idd-close #N` and `idd-close-skill #N` hyphenated tokens - `:?` covers the colon form (R1 missed; R2 redesign source) - `[[:space:]]+` requires the space GitHub also requires - same-repo `#N` form only; cross-repo trailers out of scope (#97 Plan Decision Point D7) Source 3 (predicted squash message simulation) was considered and dropped — Source 2 already covers all merge strategies (squash / merge / rebase), since the parser fires off any commit subject + body landing on main regardless of how it got there. Also adds a `### 引用 trap pattern 作反例的寫作紀律` subsection under `## Commit Conventions` in `plugins/issue-driven-dev/CLAUDE.md` codifying the writing discipline. The discipline: - Code fence is visual only — `` `Closes #N` `` renders as code but the parser still scans the substring; it works only when the body has no digit (letter N keeps it safe). - Literal letter N is the actual suppression — `Closes #N` with capital N cannot match `#[0-9]+`, mirrors the safe pattern at `references/pr-flow.md:127`. - Cite via link is strongest — reference an issue or comment URL instead of repeating the keyword. Single / double quotes do NOT suppress the parser. This is the write-time root fix; Source 2 is defence-in-depth at verify time. Renames the Step 0 bootstrap TaskCreate entry `scan_pr_body_trailers` -> `scan_pr_body_and_commits_trailers` to reflect Source 2. R2 cosmetic fix (per /idd-verify R1 Codex Finding 1): the SKILL.md fix-options echo now says "use literal letter N (optionally in a code fence; code fence alone is visual-only)" instead of the misleading R1 phrasing "wrap in code fence or use literal letter N" that implied either form alone suffices. Verification (implement-time, all examples here use letter `N` per the discipline — no real digits after the keyword): - bash -n on Step 0.8 region: PASS - Retroactive dogfood vs PR #94: the new Source 2 regex hits commit `d918270` (the trap source) and does NOT hit `5831128` / `747e0ec` (which contain `/idd-close #N` references — prefix guard works). - R2 empirical re-verification (commit subject channel): the new R2 jq filter with `messageHeadline` correctly hits `8ac8206` and `a82867d` headlines that R1's body-only filter missed. - Self-dogfood: ran the new R2 logic (subject + body combined) against this commit's own message — NO HIT. The CLAUDE.md discipline holds for the commit message itself. Plan: /Users/che/.claude/plans/snug-tumbling-bentley.md (approved via EnterPlanMode; R2 fixes performed within Plan's intent — Plan's design said "every commit channel that lands on main is scanned", subject is just another channel). Refs #97
kiki830621
added a commit
that referenced
this pull request
May 19, 2026
…PR body (Refs #97) (#98) PR #94 (v2.60.1) added Step 0.8 to flag GitHub auto-close trailers in PR bodies via `closingIssuesReferences`. When PR #94 itself was squash-merged, GitHub auto-closed `#87` anyway — bypassing the gate. Root cause: GitHub's auto-close parser fires off any close-keyword + `#<digit>` substring in the *merge commit message*, and `gh pr merge --squash` concatenates every commit's subject + body into that message. PR #94's commit `d918270` had a body that quoted the trap pattern as a verify finding (single-quoted as an R1-regex-gap example); single quotes do not suppress the parser. Result: `#87` closed 2 seconds post-merge, bypassing `/idd-close`. Step 0.8 detection face = PR body only; trap trigger face = PR body ∪ every commit subject ∪ body landing on main. Two faces, out of sync. This change extends Step 0.8 to a 2-source scan: Source 1 (kept, GitHub authoritative — PR body): `gh pr view --json closingIssuesReferences` Source 2 (NEW — per-commit messageHeadline + messageBody, local regex): `gh pr view --json commits` + trap regex `(^|[^-/[:alnum:]])(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]*:?[[:space:]]+#[0-9]+` with `tolower($0)` for case-insensitive match. Source 2 was extended in R2 (per /idd-verify R1 Devil's Advocate + Codex finding DA-H1) to also include `messageHeadline` after empirical evidence that subject-line traps had auto-closed at least two issues historically in this repo: - commit `8ac8206` subject contained `resolves #N` form -> closed `#70` - commit `a82867d` subject contained `fix #N` form -> closed `#26` R1's jq filter emitted only `.messageBody` and would have missed both. Regex has R1 / R2 / R3 lessons baked in (from PR #94 close history): - `(^|[^-/[:alnum:]])` prefix excludes IDD skill invocations like `/idd-close #N` and `idd-close-skill #N` hyphenated tokens - `:?` covers the colon form (R1 missed; R2 redesign source) - `[[:space:]]+` requires the space GitHub also requires - same-repo `#N` form only; cross-repo trailers out of scope (#97 Plan Decision Point D7) Source 3 (predicted squash message simulation) was considered and dropped — Source 2 already covers all merge strategies (squash / merge / rebase), since the parser fires off any commit subject + body landing on main regardless of how it got there. Also adds a `### 引用 trap pattern 作反例的寫作紀律` subsection under `## Commit Conventions` in `plugins/issue-driven-dev/CLAUDE.md` codifying the writing discipline. The discipline: - Code fence is visual only — `` `Closes #N` `` renders as code but the parser still scans the substring; it works only when the body has no digit (letter N keeps it safe). - Literal letter N is the actual suppression — `Closes #N` with capital N cannot match `#[0-9]+`, mirrors the safe pattern at `references/pr-flow.md:127`. - Cite via link is strongest — reference an issue or comment URL instead of repeating the keyword. Single / double quotes do NOT suppress the parser. This is the write-time root fix; Source 2 is defence-in-depth at verify time. Renames the Step 0 bootstrap TaskCreate entry `scan_pr_body_trailers` -> `scan_pr_body_and_commits_trailers` to reflect Source 2. R2 cosmetic fix (per /idd-verify R1 Codex Finding 1): the SKILL.md fix-options echo now says "use literal letter N (optionally in a code fence; code fence alone is visual-only)" instead of the misleading R1 phrasing "wrap in code fence or use literal letter N" that implied either form alone suffices. Verification (implement-time, all examples here use letter `N` per the discipline — no real digits after the keyword): - bash -n on Step 0.8 region: PASS - Retroactive dogfood vs PR #94: the new Source 2 regex hits commit `d918270` (the trap source) and does NOT hit `5831128` / `747e0ec` (which contain `/idd-close #N` references — prefix guard works). - R2 empirical re-verification (commit subject channel): the new R2 jq filter with `messageHeadline` correctly hits `8ac8206` and `a82867d` headlines that R1's body-only filter missed. - Self-dogfood: ran the new R2 logic (subject + body combined) against this commit's own message — NO HIT. The CLAUDE.md discipline holds for the commit message itself. Plan: /Users/che/.claude/plans/snug-tumbling-bentley.md (approved via EnterPlanMode; R2 fixes performed within Plan's intent — Plan's design said "every commit channel that lands on main is scanned", subject is just another channel). Refs #97
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #87 #74
Summary
IDD skill PR-body templates embedded a literal
Closes #${N}inside their anti-trailer warnings. On heredoc substitution this rendered a realCloses #<num>that GitHub's auto-close parser matched context-blind, auto-closing the issue at merge and bypassing/idd-close's checklist gate + closing summary. Confirmed incidents: #559, che-apple-mail-mcp#99, #73, and #56 (this very repo, last session).This cluster rewords all 4 template sites to a digit-free form and adds a defence-in-depth verify gate. #89 was closed as a duplicate of #74.
Changes
3435367—pr-flow.mdcanonical PR-body template reword (drop#${N})75bc21f—idd-implement+idd-all+idd-all-chainPR-body template rewords3ba7435— newidd-verifyStep 0.8 PR-body auto-close-trailer scan gate (warn-only) + bootstrap TaskCreate entryAll 4 anti-trailer warnings now read
**Do NOT add a GitHub close trailer** (Closes/Fixes/Resolves)— keywords named but never followed by#<digits>, so GitHub's regex cannot match.Checklist
/idd-verify --pron this PR)/idd-close #87 #74after mergeGenerated by /idd-implement (cluster-PR path). Do NOT add a GitHub close trailer (Closes/Fixes/Resolves) — IDD discipline requires manual /idd-close after merge to enforce checklist gate + closing summary.