Skip to content

Ban PR Citations In Rule Text - #379

Closed
EdbertChan wants to merge 15 commits into
mainfrom
plan/ban-pr-citations-in-rule-text
Closed

Ban PR Citations In Rule Text#379
EdbertChan wants to merge 15 commits into
mainfrom
plan/ban-pr-citations-in-rule-text

Conversation

@EdbertChan

@EdbertChan EdbertChan commented Sep 10, 2026

Copy link
Copy Markdown
Owner

Summary

Rule files in this repo can no longer point at old PRs with a bare number like #322. A test now fails if they do.

"See #322" only tells a reader where to dig. Each rule should say what to do and why, in its own words.

This PR also rewrites the old PR numbers in the detector playbook, so each rule there explains itself.

Numbers that name outside work, like "Cook #3", still pass.

Before and After

A rule file holding the line See the earlier fix (#322). was checked with python3 scripts/check_no_dated_provenance.py <dir> on base commit bd4cdfd and on head commit 791b960.

Before: ok      no dated provenance
After:  fail  corpus/skills/demo/SKILL.md:3: See the earlier fix (#322).

Review Claim

Reject bare references to this repository’s issues and pull requests in standing rule prose, and make existing detector guidance comply without losing its meaning.

Review Lane

behavior

Review Unit

product-skill

Safety Invariant

Preserve legitimate external citations, including Kimball’s Design Tip #164 and Cook’s numbered principles. Keep date checks, fixture and baseline exemptions, and existing rule requirements unchanged. The checker remains read-only.

Slice Rationale

The integration branch combines the gate widening with the prose cleanup it enforces. Product-skill guidance dominates the diff; engine-runtime changes implement enforcement and correct test isolation.

Assumptions: scope and safety invariants follow the supplied workflow. However, the canonical validator rejects this combined branch because it includes product-skill, engine-runtime, and docs files. Publication requires separate review units.

Non-goals

No changes to commit messages, PR bodies, date-matching rules, scan exclusions, hook runtime behavior, or installation wiring.

Test Plan

Test Plan
  • python3 -m unittest discover -s tests -p 'test_no_dated_provenance.py' -v — rerun during body preparation: 21 tests passed.
  • python3 scripts/check_no_dated_provenance.py — rerun during body preparation: ok no dated provenance.
  • bash scripts/run_all_tests.sh — supplied workflow evidence records successful completion in both verification tasks.
  • python3 engine/skills/make-pr/scripts/preflight.py --base main — passed all invoked gates, with a multiple-unit warning.

Preflight summary:

unit engine-runtime: 2 file(s)
unit product-skill: 3 file(s)
neutral 2 file(s): corpus/CLAUDE.learned.md, tests/test_no_dated_provenance.py
warn more than one review unit; declare the dominant one and justify the other in Slice Rationale
ok preflight passed

  • Canonical PR body validation against the complete main...HEAD diff — blocked by branch composition.

Validator output:

PR body validation failed:

  • Review lane behavior cannot ship with docs files in the same PR. Split behavior work from those files into their own slice.
  • PR body Review Unit "product-skill" cannot ship with engine-runtime, docs files in the same PR. Split this into one Review Unit per PR.

Revert Plan

Revert Plan
  • Safe to revert? Yes; reverting restores the previous provenance policy and guidance.
  • Revert command: git revert <squash-merge-sha> for a squash merge, or git revert -m 1 <merge-commit-sha> for a merge commit.
  • Post-revert steps: run python3 scripts/check_no_dated_provenance.py and bash scripts/run_all_tests.sh.
  • Data migration? No.

Note

Medium Risk
The provenance checker’s matching rules changed for all skill/hook prose in CI; misclassification could block legitimate edits or miss new citation shapes, though the new tests cover many edge cases.

Overview
Extends the dated-provenance gate so standing rule markdown also rejects bare #NNN references to this repository (e.g. Cited: #220, (#322)), while still allowing externally bound numbers (Cook #3, Invoker #11593), continued lists, URL/title dashes, and hash numbers inside code spans.

Brings detector guidance in line with that policy: the ship-a-detector skill and detector-lifecycle.md playbook drop PR/issue footnotes and Cited: blocks but keep the same instructions using hook names and narrative. corpus/CLAUDE.learned.md labels one external example as Invoker #11593. Step 13 now explicitly forbids citing this repo’s own PRs/issues in rule text.

Adds regression coverage: expanded test_no_dated_provenance.py (bare refs, bindings, diff mode) and a new test_playbook_names_resolve.py so backtick-cited hooks/skills in the playbook resolve under engine/hooks / skill trees. skill-usage-log disabled-flag test asserts no log file under the patched temp state_dir instead of the global log path.

Reviewed by Cursor Bugbot for commit 791b960. Bugbot is set up for automated code reviews on this repo. Configure here.

edbert-bot and others added 15 commits September 9, 2026 23:35
The ship-a-detector playbook and its SKILL.md cited this repo's pull
requests as bare numbers ("(#322)", "Cited: #220, #298"), 85 lines in all.
That is the same incident history check_no_dated_provenance keeps out of
standing rule text; git log carries it. Each lesson stays, reworded to name
the hook or file it happened in instead of the PR that fixed it.

corpus/CLAUDE.learned.md cited an Invoker pull request as a bare "(#11593)".
GitHub reads an unqualified number as this repo's, so it now names Invoker.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check_no_dated_provenance caught "PR #228" but not "(#322)": a bare
hash-number with no tracker word passed, and one playbook carried 84 lines
of them. GitHub links an unqualified number to the current repo, so a bare
"#N" in rule prose is now a citation of this repo unless the line binds it
to another work: a capitalised title word right in front ("Design Tip #164",
"Cook #3", "Invoker #11593"), a URL or closing title quote before a dash,
or an earlier hash-number it continues ("#10553-#10558", "#3, #4"). Code
spans, markdown anchors, URL fragments and HTML entities are skipped.
Punctuation between the word and the number breaks the bind, so "Cited:
#220" and "(#194)" fail. Capitalised sentence openers (Then, After, Since
and a few more) join the existing list of words that bind nothing.

Measured: on an untouched copy of the previous tree the widened gate reports
86 lines, all genuine repo citations, and neither Kimball nor Cook line.
The date rules and the fixture/baseline exemption are unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a bare repository issue or pull-request reference in standing rule text fails the provenance gate, while a published citation that happens to contain a hash number still passes.

Review lane: behavior
Safety invariant: a legitimate external citation must keep passing; the checker is proven against the live example of Kimball's Design Tip number one six four, which appears twice in corpus and is prior art rather than incident history, and against fixture and baseline data which the existing checker already exempts.
Slice rationale: widening the gate is a separate reviewable claim from cleaning the text it will flag, and it must land first so the cleanup has something to prove itself against.
Architectural effect: extends one existing checker; adds no new script.
Goal: make scripts/check_no_dated_provenance.py reject a bare hash-number reference to this repository's own issues or pull requests when it appears in rule prose.
Effectiveness measurement: the checker flags the one hook README that cites a pull request today, and does not flag either occurrence of the Kimball design-tip citation.
Motivation: the checker's own docstring already forbids incident history in rule text, but matches only dates, so pull-request numbers pass and one playbook accumulated one hundred and forty six of them.
Alternative considerations: a new sibling checker was rejected because the existing one already owns this rule and a sibling would drift from it; banning every hash-number was rejected because published sources legitimately contain them.
Implementation details: the distinguishing signal is whether the number refers to this repository. A bare hash followed by digits, a GitHub pull or issue URL for this repo, and phrasings such as PR hash-number all refer here; a hash-number inside a quoted external title does not.
Non-goals: does not change the date rules, does not touch commit messages or pull-request bodies, does not scan fixture or baseline data.
Layer: infrastructure
Feature state: active
Files: scripts/check_no_dated_provenance.py, tests/test_no_dated_provenance.py
Change types: gate widening
Acceptance criteria: a fixture citing a repository pull request fails; a fixture citing Kimball's design tip passes; the existing date tests still pass.

Exit code: 0
…dened checker passes its own suite and the full repo scan.

Review lane: proof
Safety invariant: read-only; the gate never edits the text it judges.
Slice rationale: a widened matcher trades precision for recall silently unless its negative cases are asserted.
Architectural effect: none.
Goal: run the provenance suite and the full test suite.
Effectiveness measurement: non-zero on any failure.
Motivation: a gate that flags a legitimate citation will be turned off.
Alternative considerations: none.
Implementation details: unittest discover plus the repo suite.
Non-goals: does not judge the cleanup.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.

Exit code: 1
…dened checker passes its own suite and the full repo scan.

Review lane: proof
Safety invariant: read-only; the gate never edits the text it judges.
Slice rationale: a widened matcher trades precision for recall silently unless its negative cases are asserted.
Architectural effect: none.
Goal: run the provenance suite and the full test suite.
Effectiveness measurement: non-zero on any failure.
Motivation: a gate that flags a legitimate citation will be turned off.
Alternative considerations: none.
Implementation details: unittest discover plus the repo suite.
Non-goals: does not judge the cleanup.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.

Solution:
  Review claim: the widened checker passes its own suite and the full repo scan.
Review lane: proof
Safety invariant: read-only; the gate never edits the text it judges.
Slice rationale: a widened matcher trades precision for recall silently unless its negative cases are asserted.
Architectural effect: none.
Goal: run the provenance suite and the full test suite.
Effectiveness measurement: non-zero on any failure.
Motivation: a gate that flags a legitimate citation will be turned off.
Alternative considerations: none.
Implementation details: unittest discover plus the repo suite.
Non-goals: does not judge the cleanup.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.
…dened checker passes its own suite and the full repo scan.

Review lane: proof
Safety invariant: read-only; the gate never edits the text it judges.
Slice rationale: a widened matcher trades precision for recall silently unless its negative cases are asserted.
Architectural effect: none.
Goal: run the provenance suite and the full test suite.
Effectiveness measurement: non-zero on any failure.
Motivation: a gate that flags a legitimate citation will be turned off.
Alternative considerations: none.
Implementation details: unittest discover plus the repo suite.
Non-goals: does not judge the cleanup.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.

Exit code: 0
… standing rule text in this repository cites its own pull requests, and every rule that lost a citation still says what it requires and why it matters.

Review lane: behavior
Safety invariant: no rule loses meaning; a citation is removed only after the sentence around it has been rewritten to carry the rule on its own terms, and any published external citation is left untouched.
Slice rationale: cleaning is a content change with judgment in every edit, which is a different review than the mechanical gate that finds the violations.
Architectural effect: none; prose only.
Goal: remove this repository's own pull-request references from rule prose, rewriting each affected sentence to state the rule and its effect.
Effectiveness measurement: the widened checker passes across the whole tree, and the rewritten sentences still name what to do and what goes wrong without it.
Motivation: one hook README cites a pull request as its justification, which tells a future reader where to look rather than what the rule is.
Alternative considerations: deleting the sentences outright was rejected because the surrounding rule would lose its reason; moving the history into the commit message is the intended home.
Implementation details: the known violation is engine/hooks/new-file-callout/README.md line fourteen; run the widened checker across the tree to find any others rather than trusting this list.
Non-goals: does not touch the Kimball citations, does not edit commit messages or pull-request bodies, does not change any rule's requirement.
Layer: domain
Feature state: active
Files: engine/hooks/new-file-callout/README.md and any other file the widened checker flags
Change types: prose cleanup
Acceptance criteria: the widened checker exits zero on a full-tree scan, and both Kimball citations remain present.

Exit code: 1
… standing rule text in this repository cites its own pull requests, and every rule that lost a citation still says what it requires and why it matters.

Review lane: behavior
Safety invariant: no rule loses meaning; a citation is removed only after the sentence around it has been rewritten to carry the rule on its own terms, and any published external citation is left untouched.
Slice rationale: cleaning is a content change with judgment in every edit, which is a different review than the mechanical gate that finds the violations.
Architectural effect: none; prose only.
Goal: remove this repository's own pull-request references from rule prose, rewriting each affected sentence to state the rule and its effect.
Effectiveness measurement: the widened checker passes across the whole tree, and the rewritten sentences still name what to do and what goes wrong without it.
Motivation: one hook README cites a pull request as its justification, which tells a future reader where to look rather than what the rule is.
Alternative considerations: deleting the sentences outright was rejected because the surrounding rule would lose its reason; moving the history into the commit message is the intended home.
Implementation details: the known violation is engine/hooks/new-file-callout/README.md line fourteen; run the widened checker across the tree to find any others rather than trusting this list.
Non-goals: does not touch the Kimball citations, does not edit commit messages or pull-request bodies, does not change any rule's requirement.
Layer: domain
Feature state: active
Files: engine/hooks/new-file-callout/README.md and any other file the widened checker flags
Change types: prose cleanup
Acceptance criteria: the widened checker exits zero on a full-tree scan, and both Kimball citations remain present.

Exit code: 1
Dropping this repo's PR numbers from the ship-a-detector playbook left three
sentences pointing at nothing: "the next widening", "the next shape", and
"One widening ... the next". Each now names what it points at
(gh-write-verification, the concession shape) and says why the rule matters.

The cleanup swapped PR numbers for hook names, so a lesson is only as good as
the hook it names. tests/test_playbook_names_resolve.py fails when a hook or
skill the playbook cites does not exist, which also gives the skill's prose
change the colocated test check_skill_test_coverage.py asks for.

check_no_new_comments.py reads added lines one at a time, and two docstring
lines in check_no_dated_provenance.py began with a quoted "#3" example, so it
read them as comments. The paragraph is reflowed; no wording changed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… standing rule text in this repository cites its own pull requests, and every rule that lost a citation still says what it requires and why it matters.

Review lane: behavior
Safety invariant: no rule loses meaning; a citation is removed only after the sentence around it has been rewritten to carry the rule on its own terms, and any published external citation is left untouched.
Slice rationale: cleaning is a content change with judgment in every edit, which is a different review than the mechanical gate that finds the violations.
Architectural effect: none; prose only.
Goal: remove this repository's own pull-request references from rule prose, rewriting each affected sentence to state the rule and its effect.
Effectiveness measurement: the widened checker passes across the whole tree, and the rewritten sentences still name what to do and what goes wrong without it.
Motivation: one hook README cites a pull request as its justification, which tells a future reader where to look rather than what the rule is.
Alternative considerations: deleting the sentences outright was rejected because the surrounding rule would lose its reason; moving the history into the commit message is the intended home.
Implementation details: the known violation is engine/hooks/new-file-callout/README.md line fourteen; run the widened checker across the tree to find any others rather than trusting this list.
Non-goals: does not touch the Kimball citations, does not edit commit messages or pull-request bodies, does not change any rule's requirement.
Layer: domain
Feature state: active
Files: engine/hooks/new-file-callout/README.md and any other file the widened checker flags
Change types: prose cleanup
Acceptance criteria: the widened checker exits zero on a full-tree scan, and both Kimball citations remain present.

Exit code: 0
…clean under the widened gate.

Review lane: proof
Safety invariant: read-only.
Slice rationale: a cleanup is only done when the gate that found the violations reports none.
Architectural effect: none.
Goal: full-tree scan plus the repo suite.
Effectiveness measurement: non-zero on any remaining violation.
Motivation: a partial cleanup leaves the gate red and invites disabling it.
Alternative considerations: none.
Implementation details: the checker's full-scan mode plus the suite.
Non-goals: none.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.

Exit code: 1
…clean under the widened gate.

Review lane: proof
Safety invariant: read-only.
Slice rationale: a cleanup is only done when the gate that found the violations reports none.
Architectural effect: none.
Goal: full-tree scan plus the repo suite.
Effectiveness measurement: non-zero on any remaining violation.
Motivation: a partial cleanup leaves the gate red and invites disabling it.
Alternative considerations: none.
Implementation details: the checker's full-scan mode plus the suite.
Non-goals: none.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.

Solution:
  Review claim: the tree is clean under the widened gate.
Review lane: proof
Safety invariant: read-only.
Slice rationale: a cleanup is only done when the gate that found the violations reports none.
Architectural effect: none.
Goal: full-tree scan plus the repo suite.
Effectiveness measurement: non-zero on any remaining violation.
Motivation: a partial cleanup leaves the gate red and invites disabling it.
Alternative considerations: none.
Implementation details: the checker's full-scan mode plus the suite.
Non-goals: none.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.
…clean under the widened gate.

Review lane: proof
Safety invariant: read-only.
Slice rationale: a cleanup is only done when the gate that found the violations reports none.
Architectural effect: none.
Goal: full-tree scan plus the repo suite.
Effectiveness measurement: non-zero on any remaining violation.
Motivation: a partial cleanup leaves the gate red and invites disabling it.
Alternative considerations: none.
Implementation details: the checker's full-scan mode plus the suite.
Non-goals: none.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.

Exit code: 0
…d85-1c5017c7 — Review claim: the widened checker passes its own suite and the full repo scan.

Review lane: proof
Safety invariant: read-only; the gate never edits the text it judges.
Slice rationale: a widened matcher trades precision for recall silently unless its negative cases are asserted.
Architectural effect: none.
Goal: run the provenance suite and the full test suite.
Effectiveness measurement: non-zero on any failure.
Motivation: a gate that flags a legitimate citation will be turned off.
Alternative considerations: none.
Implementation details: unittest discover plus the repo suite.
Non-goals: does not judge the cleanup.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.
…f0eb5f — Review claim: the tree is clean under the widened gate.

Review lane: proof
Safety invariant: read-only.
Slice rationale: a cleanup is only done when the gate that found the violations reports none.
Architectural effect: none.
Goal: full-tree scan plus the repo suite.
Effectiveness measurement: non-zero on any remaining violation.
Motivation: a partial cleanup leaves the gate red and invites disabling it.
Alternative considerations: none.
Implementation details: the checker's full-scan mode plus the suite.
Non-goals: none.
Layer: infrastructure
Feature state: active
Files: none changed
Change types: verification
Acceptance criteria: exits 0.
@cursor

cursor Bot commented Sep 10, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_fe9adcfa-59ef-4875-a518-6819f7aa24da)

@EdbertChan

Copy link
Copy Markdown
Owner Author

Mergify repair stopped: GitHub reports merge conflict. The retry cap was reached for current head 791b960.

@EdbertChan

Copy link
Copy Markdown
Owner Author

Superseded — main's check_no_dated_provenance.py already does what this PR set out to do, landed by another PR while this one sat open.

Verified against origin/main via the gate's own flags_exemplar entry point:

REJECT  A bare #322 in rule text.
REJECT  Cited: #220
REJECT  (#194)
allow   Design Tip #164 is fine.
allow   Cook #3 is fine.

Cited: #220 and (#194) were the two cases this PR's description called out as passing on main; both now fail there. Main also goes further, rejecting bare commit SHAs and incident-narrative openers, with NUMBERED_TITLES carrying the published-title exemption.

Merging this now would mean resolving 20 conflict hunks in product/skills/ship-a-detector/playbooks/detector-lifecycle.md plus 3 more files, to land behavior that is already in place.

One case worth a follow-up rather than a merge: main rejects Invoker #11593 while allowing Invoker PRs #10553. If a bare foreign-repo reference without the word "PRs" should be allowed, that is a small targeted change to NUMBERED_TITLES/FOREIGN_SLUG_RE on main, not this branch.

@EdbertChan EdbertChan closed this Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants