Repository navigation
Revert "[13.x] Prevent loose comparison bypass in contains validation rule" - #61330
Merged
Merged
Conversation
This reverts commit 87a21fb.
PupFuzz
added a commit
to PupFuzz/agent-webhook-bridge
that referenced
this pull request
Aug 31, 2026
… the wrapped ref (closes card#8306) (#620) * fix(writeback): a REVERT closes nothing — refuse the quoted title and the wrapped ref (closes card#8306) GitHub composes a revert PR by QUOTING the original's title and WRAPPING its branch. Once card#8294 made `(closes card#N)` the title convention, every revert inherited a closing form — and a card token — for work it UNDOES, so merging one moved the reverted card to Shipped and the board asserted the opposite of the truth. Both closure routes fired, measured end-to-end through the shipped classifier before any code changed: the lexical route on the quoted verb, and the structural route on the wrapped ref, because CardTokenGrammar::parse() returns 8294 for `revert-611-card-8294-slug`. A nested `Revert "Revert "…""` moved it too. The payload shapes are observed, not assumed. No revert PR exists on any repo this shop owns (1,607 PRs scanned, zero) and GitHub's docs state neither format, so both were read through the REST API off public repositories where GitHub minted them (laravel/framework#61330, #61262). New RevertGrammar owns both artifacts of the one upstream act. ClosureGrammar subtracts the quoted span at its single choke point; PrOutcome::mergeClosesCard() refuses `^revert-<n>`. Placing the refusal in the two shared authorities — not at the classifier — is DL-305 §6 / DL-308's lockstep, so bridge:reconcile cannot re-plan on a schedule the move the event path just declined. Correlation is untouched: a revert still stamps PR refs and still moves on `opened`. A revert of a revert does not close either — GitHub does not escape the inner quotes, and the first revert never moved the card back. To close a card ON a revert, put the closing form OUTSIDE the quoted title; no new marker minted. Two operator-facing lines this change would have made false are corrected in it; every non-revert line is byte-identical. The pr-title-lint comment disclosing this hazard is discharged in place — its `revert-*` exemption stays, for its own reason. DL-318. 9 mutations over the new predicate, all RED, each proven landed and restored byte-identically. 2730/2730 phpunit (10312 assertions), phpstan L7 0, pint clean, check-doc-refs clean, decision-log check clean. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_017kFEZEcLwqtADjmWs1dDvo * docs(changelog): relocate the card#8306 entry into the post-v0.79.0 [Unreleased] section v0.79.0 was released while this branch was open. `dev` renamed the old `## [Unreleased]` to `## [0.79.0]` and seeded a fresh empty one above it; this entry's text never changed, so the 3-way merge left it sitting inside the RELEASED section — where it would have claimed shipped work that has not shipped, and where `changelog-gate` correctly reported the [Unreleased] section as naming no card. Moved, not duplicated. Nothing under `## [0.79.0]` is touched: the diff against origin/dev is 4 insertions and 0 deletions. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_017kFEZEcLwqtADjmWs1dDvo * fix(writeback): the structural route must read the TITLE too — a hand-made revert still closed (card#8306) The first revision of this change gated the structural route on the REF wrapper alone, so it delivered less than its own headline claim. GitHub's revert wraps the ref; a HAND-MADE `git revert` pushed to an ordinary branch wraps nothing and announces itself only in the title. Measured on the shipped predicates, card 4811, outcome merged, title `Revert "feat: widget (closes card#4811)"`: revert-611-card-4811-widget structural=false -> no move card-4811-widget structural=TRUE -> *** MOVED 4811 *** revert/card-4811-widget structural=TRUE -> *** MOVED 4811 *** revert-card-4811-widget structural=TRUE -> *** MOVED 4811 *** `isRevert()` was TRUE on every row and consulted by neither closure predicate — its only two call sites rendered messages. `card-<id>-slug` is the spelling `board-card-start` mints, so this was the COMMON branch shape. It was also SILENT: a closing card never reaches warnMentionWithoutClosure(), so nothing warned. PrOutcomeTest pinned the gap as intended; that assertion is flipped. mergeClosesCard() now asks `! isRevert($title, $headRef)` and takes the title as a REQUIRED 4th argument, so no caller can silently keep the ref-only reading. Both callers already held the title, so the DL-305 §6 lockstep is preserved — the ruling stays in the one shared authority. Priced, not asserted away: a title merely QUOTING `revert "…"` loses its structural route. Under-promotion, loud (the warning now fires and names the revert), and 0 of 1,120 real merged PR titles across all three repos match. A mutation found a second gap and it is closed here too: passing a blank title into the BACKSTOP's structural gate left the suite green, because every reconcile revert fixture rode a `revert-<n>-` ref. ReconcileCommandTest gains the hand-made case. Four corrections to claims this PR made about the world: - DL-318 Bounds said `isRevert()` "covers" the hand-run case. It did not. - DL/docs/PR said a revert "still moves on `opened`". isRegressiveMove() makes `opened` forward-only, so in the dominant case the move is REFUSED, not made. - The reconcile revert skip line dropped the head ref the non-revert line quotes and DL-308 added deliberately. Appended. - "filed rather than repaired" named no destination. DL-318 IS the filing. 13 mutations, all RED, each proven landed and restored byte-identically — including the ref-only regression and the two blank-title mutants. 2732/2732 phpunit (10330 assertions), phpstan L7 0, pint clean, check-doc-refs clean, decision-log check clean. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_017kFEZEcLwqtADjmWs1dDvo * docs: replace an inherited false-negative figure with one measured here (card#8306) The "0 of 1,120 merged PR titles" bound on the structural route's new false-negative arrived from review; I restated it without measuring it. Re-run locally over every merged PR title in the three repos this shop owns: the denominator is 1,566, and the count is still 0. The first run of that grep returned 0 with a control that did NOT match a known positive, so that 0 measured nothing. The figure now on the record comes from a run whose control discriminates in both directions, including a known positive planted into the real corpus. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_017kFEZEcLwqtADjmWs1dDvo PR #620 description (verbatim): GitHub composes a revert PR by **quoting** the original's title and **wrapping** its branch. Once card#8294 made `(closes card#N)` the title convention, every revert inherited a closing form — and a card token — **for work it UNDOES**, so merging one moved the reverted card to Shipped and the board asserted the opposite of the truth. ## What's in it - **`app/Bridge/Support/RevertGrammar.php` (new)** — the one authority for the two artifacts GitHub mints for a revert: `withoutQuotedRevert()`, `isRevertRef()`, `isRevert()`, `describeRefusal()`. - **`ClosureGrammar::remainders()`** subtracts the quoted span at the single private choke point all four public predicates read tokens through. - **`PrOutcome::mergeClosesCard()`** gains a `! RevertGrammar::isRevert($title, $headRef)` conjunct — **both surfaces** — and takes the title as a **required** 4th argument so no caller can silently keep the ref-only reading. The structural route's own premise (the ref is the CARD's branch, carrying its work) fails on a revert, whichever surface says so. - **The withheld-merge warning and `bridge:reconcile`'s skip line** gain a revert arm. Both asserted *the ref does not name the card and the title carries no closing form* — untrue on both counts for a revert, which would have sent an operator to rewrite correct prose. Every **non-revert** line is byte-identical. - **`pr-title-lint.yml`**'s *"⚠ DISCLOSED, not fixed here"* comment is discharged in place. The `revert-*` exemption stays — it governs what CI DEMANDS, never what the writeback READS. - **DL-318**, `docs/writeback.md` § *Mention vs closure*, `docs/config-schema.md`, `docs/kanban-integration-contract.md`, `CLAUDE_CONVENTIONS.md` § *PR titles*, `docs/CHANGELOG.md`. ## Both routes were live — measured, not reasoned End-to-end through the shipped classifier over a merged-revert payload **before** any code changed: | fixture | before | after | | --- | --- | --- | | title `Revert "… (closes card#8294)"`, ref `revert-611-fix/8294-slug` | MOVES card#8294 → `merged` | no move | | title `Revert "… (card#8294)"`, ref `revert-611-card-8294-slug` | MOVES card#8294 → `merged` | no move | | both wrappers at once | MOVES card#8294 → `merged` | no move | | `Revert "Revert "… (closes card#8294)""` | MOVES card#8294 → `merged` | no move | | revert of a NON-closing original | no move | no move | | ⛔ `Revert "… (closes card#4811)"` on **`card-4811-widget`** (hand-made; the spelling `board-card-start` mints) | **MOVES — silently** | no move + warning | | ⛔ same title on `revert/card-4811-widget` / `revert-card-4811-widget` | **MOVES — silently** | no move + warning | | ordinary closing PR / ordinary `card-<id>-slug` PR | MOVES | MOVES | ⚠ **The last two rows were found by adversarial review of the first revision of this PR, not by me.** That revision gated the structural route on the REF wrapper alone, so a hand-made `git revert` — which wraps no ref and announces itself only in the title — still promoted the card it reverted, and did so **silently**: a closing card never reaches `warnMentionWithoutClosure()`. Its `PrOutcomeTest` pinned that behaviour as intended. The conjunct now asks `isRevert()` on both surfaces and the assertion is flipped, with the rows above as witnesses at the predicate, end-to-end in classifier witness 9b, and at the `bridge:reconcile` backstop. The original structural hit is `CardTokenGrammar::parse('revert-611-card-8294-slug') === 8294` — GitHub's wrapper carries a **copy** of the original branch's name, so a title-only repair would have left half the hazard open on every install still on the `card-<id>-slug` convention the toolkit mints. ## The payload shapes are OBSERVED No revert PR exists on any repo this shop owns (**1,607 PRs scanned**, zero on title or head ref), and GitHub's *Reverting a pull request* article states **neither** format. Both were pulled through the REST API from public repositories where GitHub minted them: `laravel/framework#61330` (`Revert "<the original's title>"` on `revert-61320-fix/contains-strict-comparison`, against #61320 whose head is `fix/contains-strict-comparison`) and `#61262` (`revert-61245` — the tail is dropped when the original's branch is gone). ## Gotchas - ⚠ **CHANGES WHAT THE WRITEBACK ACTS ON.** No migration, no config key, no token-scope change, no receiver accept/reject change, and the `pr-title-lint` accept-set is byte-untouched. - ⛔ **Correlation is untouched.** A revert still stamps PR refs and the ungated outcomes still fire exactly as DL-305 §2 ruled; only the completion claim is refused. - ⛔ **A revert of a revert does not close either** — GitHub does not escape the inner quotes so the depth is unparseable, and the first revert never moved the card back, so the re-apply has nothing to promote. - ⭐ **To close a card ON a revert, put the closing form OUTSIDE the quoted title.** Positional, no new vocabulary; `[no-close]` stays the CI-only inverse it was. Its reach is bounded by correlation — it re-closes the card the branch names and cannot redirect the move. - ⚠ **A divergence stays open, and DL-318 IS its filing** (no separate card, stated so deliberately): the lint decides *is this a revert* from the `revert-*` BRANCH while the runtime also reads the TITLE, so a hand-made `git revert` on an ordinary branch passes CI and is refused at runtime. Under-promotion, loud, recoverable — the same shape already recorded for the branch predicate. - ⚠ **THE PRICE OF READING THE TITLE, priced rather than asserted away:** a PR whose title merely QUOTES the word (`fix: revert "the streaming change" and re-land it`) now loses its structural route. It is the identical over-refusal the lexical route already accepts by construction, it fails toward under-promotion, and it is LOUD — the card lands in the withheld set so the warning fires and names the revert. **0 of the 1,566 merged PR titles** across all three repos match it, measured here with a known positive planted into that same corpus to prove the grep discriminates. (The first run of that grep had a broken control and its `0` meant nothing; the figure above is the re-run.) - ⚠ **`opened` fires but usually does not MOVE.** A revert PR still emits the `opened` target; `KanbanMoveCardHandler::isRegressiveMove()` makes `opened` strictly forward-only, so in the dominant case — a revert of a merged PR whose card reached Shipped — the move is refused. It moves only where the card still sits behind In-Review. - ⚠ **The obvious DL-239 move was tried and withdrawn:** adding a revert row to `ClosureGrammar::VECTORS` reds two `PrTitleLintTest` ties (the corpus is title-only while the lint's revert handling is branch-keyed; and the message tie derives a verb stem from each accepted row's first word). Both constraints were undocumented until this card measured them and are now in the `VECTORS` docblock, pinned by a test. ## Verification - **13 mutations over the new predicate, all RED**, each proven to have landed (sha256 before/after + the mutated text read back) and restored byte-identically: lexical subtraction deleted · structural conjunct deleted · **structural conjunct regressed to the ref-only reading** · **structural conjunct reduced to the title surface only** · **reconcile passes a blank title** · **classifier passes a blank title** · unterminated-wrapper alternative dropped · quoted span made non-greedy · ref anchor dropped · ref digits dropped · classifier warning arm removed · reconcile skip arm removed · `isRevert` reduced to one surface. - 2730/2730 phpunit (10312 assertions, SQLite) · phpstan L7 0 · pint clean · `check-doc-refs` clean · `decision-log.py check` clean. --------- Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
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.
Reverts #61320