chore(verify): remove the dead same_commit helper; record a measured ablation - #675
Conversation
`same_commit`'s last call site went with #645's `_canonical_commit_oid`, which resolves both sides to a canonical full object id and compares them exactly. The helper's prefix-tolerant equality — either argument a prefix of the other once both reach 7 characters — would have handed that looseness back to whichever caller reached for it next. An AST scan over `src/` and `tests/` (161 files) finds one def site and zero executable references: no `Name`, no `Attribute`, no import alias. A textual sweep of every text file in the repo (244) adds only four prose mentions, all restated here: src/bmad_loop/verify.py:255 rev_parse_head's docstring src/bmad_loop/verify.py:349 the _OBJECT_ID length-floor comment tests/test_verify.py:2004 the floor-characterization docstring (#647) tests/test_verify.py:3685 the rev-parse-under-noise docstring Neither comment sources its 7 from the helper any more. #647 had just corrected the docstring this commit deletes (#674): there is no "default `--short` length" to mirror, since `core.abbrev` defaults to `auto`, which scales the abbreviation with the repository's object count — 8 in this repo — and clamps upward to 7 only for small ones. The floor is a constant the gate chooses; the correction survives the removal in the two places that still state it.
`test_file_bytes_at_revision_distinguishes_blob_absence_tree_and_git_failure` asserted three refusals with no record of what a regression would look like. The four single-side mutations of `entry is None or entry[1] != "blob"` were run against both oracles; all four redden, each on exactly one of the test's four `is None` assertions: file_bytes_at_revision, absence side dropped → TypeError on missing.bin file_bytes_at_revision, type side dropped → GitError on oracle/ worktree_file_bytes_at_revision, absence side → TypeError on missing.bin worktree_file_bytes_at_revision, type side → returns tree-listing bytes Only the last is silent in production: `cat-file blob` refuses a tree oid, but `cat-file --filters --path=` renders the tree's listing and returns it as file content. The docstring now names that asymmetry, since it is what the test's own claim — "never tree listings" — actually rests on.
|
@codex review |
|
Warning Review limit reached
Next review available in: 51 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change removes the unused ChangesVerification cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR removes an unused helper and updates related documentation and test rationale without changing runtime behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e13031f028
ℹ️ 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".
| - **Dropped `verify.same_commit`, which no longer had a caller.** Its last call site went with | ||
| #645's `_canonical_commit_oid`, which resolves both sides to a canonical full object id and | ||
| compares them exactly. The helper's prefix-tolerant equality — either argument a prefix of the | ||
| other once both reach 7 characters — would have handed that looseness back to whichever caller | ||
| reached for it next. |
There was a problem hiding this comment.
This five-line item uses past-tense “Dropped” and includes detailed implementation history about canonicalization and potential future callers, making the release notes harder to scan. Condense it to an imperative summary such as Remove the unused verify.same_commit helper. to follow the repository’s explicit changelog convention.
AGENTS.md reference: AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly taken, partly declined — measured before deciding.
Declined: the proposed shape. Remove the unused verify.same_commit helper. as a lone bullet would be the outlier in this file. Counting every top-level entry in CHANGELOG.md (409 bold-lead entries):
- lead words:
A×94,The×56,An×16 — entries opening with a bare imperative verb: 0 - continuation lines: median 5, mean 5.3 — entries with none: 4 of 409
So the current item's five lines sit exactly at the house median, and the AGENTS.md:68 line quoted here ("terse, scannable, imperative") governs the body, not the bolded headline; every headline in the file is a noun phrase or a state description. The sibling entry in this same Removed section reads "The `bmad-auto` → `bmad-loop` rename shims are gone."
Taken: the scannability half. The lead did bury the subject behind a past-tense verb. It now fronts the symbol, matching that sibling's shape:
verify.same_commitis gone — nothing called it.
and the body is a line shorter. Fixed in eb594675.
… the 7
Codex read the changelog entry as non-scannable and proposed collapsing it to
a single imperative bullet. Measured against the file, that shape would be the
outlier: of 409 bold-lead entries, zero open with a bare imperative verb, the
median entry carries 5 continuation lines, and only 4 carry none. AGENTS.md's
"terse, scannable, imperative" governs the body, not the bolded headline — so
the proposal is declined as written, and its legitimate half is taken instead
by fronting the symbol, the shape the sibling `Removed` entry already uses
("The `bmad-auto` -> `bmad-loop` rename shims are gone.").
Separately, self-review caught the two surviving statements of the 7 reading as
if they disagreed: the `_OBJECT_ID` comment sources the floor from git's
shortest auto abbreviation, while #647's test docstring said the 7 is "not a
length git derives". Both are true — the gate picks the constant, and it sits
where git's auto abbreviation bottoms out — so the docstring now says that,
instead of leaving one file arguing with itself about where the number comes
from, which is the defect #674 named.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review |
|
1 participant
What
Two housekeeping items carried out of the #647 review, both small and independent:
verify.same_commit— dead production code — and restate the four prose references to it.test_file_bytes_at_revision_distinguishes_blob_absence_tree_and_git_failure, which asserted three refusals with no note of what a regression would look like.Why the helper goes
Its last call site went with #645's
_canonical_commit_oid, which resolves both sides to a canonical full object id and compares them exactly. What was left is prefix-tolerant equality — either argument a prefix of the other once both reach 7 characters — with nothing calling it. That is precisely the looseness the gate spent #645 removing, sitting in the module as a ready-made shortcut for the next caller who needs to compare two shas.Proven dead, not grepped. The repo's shell
grepisugrep, which exits silently on zero matches, so absence was established in Python instead: an AST scan oversrc/andtests/(161 files) walks every module for aName,Attribute, or import alias bindingsame_commitand finds one def site and zero executable references. A textual sweep of every text file in the repo (244) then catches anything the AST cannot see — dynamicgetattr, docs, config — and adds only four prose mentions, all restated here:4650ca3f)verify.py:255rev_parse_head— a warning-suffixed sha "flows intosame_commitcomparisons"verify.py:349_OBJECT_IDlength-floor comment — "Length floor mirrorssame_commit's 7"tests/test_verify.py:2004same_commit's 7"tests/test_verify.py:3685The 7 survives the removal
#647 had just corrected this docstring (#674) rather than deleting it, and that correction is worth keeping: there is no fixed "default
--shortlength" for the floor to mirror.core.abbrevdefaults toauto, which scales the abbreviation with the repository's object count — 8 in this repo, 7 in the test sandbox — and clamps upward to 7 only for small repos. Both surviving statements of the floor now say the same thing — the 7 is the gate's own constant, set where git's auto abbreviation bottoms out — rather than one sourcing it from git and the other denying git derives it. Deleting the function removes the third copy of the claim rather than leaving a corrected docstring on an uncallable helper.The ablation, measured before it was written down
Per the repo's doctrine, a negative assertion is only trustworthy once the gate has been deleted and the test seen to fail. All four single-side mutations of
entry is None or entry[1] != "blob"were run — both sides, both oracles, one at a time, each verified to change exactly one line. All four redden, and on four disjoint assertions, one per mutation:file_bytes_at_revision, absence side droppedmissing.binTypeError: 'NoneType' object is not subscriptablefile_bytes_at_revision, type side droppedoracleGitError—cat-file blob <tree-oid>: bad fileworktree_file_bytes_at_revision, absence side droppedmissing.binTypeErrorworktree_file_bytes_at_revision, type side droppedoracleThe last row is the one the docstring now names, because the two oracles are not symmetric there. Plain
cat-file blobis refused by git on a tree oid, so that leg fails loudly on its own.cat-file --filters --path=instead renders the tree's listing and hands it back as file content —b'100644 spec.bin\x00<20 raw oid bytes>'— which recovery would compare against a live file as if it were a baseline blob. That clause is the only thing standing between a caller and tree bytes, which is exactly the claim the test's summary line makes.No mutation passed, so nothing here documents an uncaught guard side.
Scope
No behavior change. One deleted function with no callers, four prose restatements, one test docstring. The
Removedchangelog entry covers the helper; the docstring work takes none.Testing
uv run pytest -q -n logical→ 6118 passed, 53 skippeduv run pyright→ 0 errorstrunk check→ no issues