Repository navigation
update-shape.py: the four functions Sonar scored at 24, 19, 16 and 47 for cognitive complexity hand each decision to a named helper, so every one of them is readable alone and every refusal, message and exit code is byte-identical (#133) - #139
Conversation
…and the verdict ladder to `_row_verdict`, so the function Sonar scored at 24 is a loop over rows and each decision it makes is readable alone (#133) SonarCloud's `python:S3776` scored `classify` at 24 against a limit of 15 (complexipy 8.0.1 agrees: 24). One loop body answered three "these bytes cannot be compared" questions and then walked a five-branch verdict ladder over three derived booleans, so the four byte-strings the docstring names — the recorded digest, the root's bytes, the upstream's bytes at the pinned commit and at the target — had to be reconstructed from the ladder by anyone reading it. It is now three functions and the same code. `classify` is the loop. `_classify_row` holds the three exits where a byte-string does not exist at all (no file in the root, no upstream path this pin maps to, nothing at the target), each of which is a human's decision rather than a difference this tool acts on. `_row_verdict` is the ladder, reached only when all four read. Nothing moved across the reads: the root's bytes, then the pinned blob, then the target blob, in that order, because `Upstream.blob` shells out to git and a reordering would change what a failing run prints. `file_sha256` is still called exactly where it was, passed into `_row_verdict` as the argument that was the local variable. Every state, detail string and digest assignment is byte-identical, `continue` becoming `return` in a body that now ends the function instead of the iteration. classify 24 -> 1; _classify_row 3 (new); _row_verdict 11 (new). Proven by tests/test_update_shape.py, tests/test_update_shape_add.py and tests/test_commit_trailers.py: 178 passed. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…aterializer's copy lists are and `_listed_copies` what they mean, so the `ast` scan and the mapping onto `Copy` are two functions rather than one body Sonar scored at 19 (#133) SonarCloud's `python:S3776` scored `upstream_copies` at 19 against a limit of 15 (complexipy 8.0.1 scored it 21). It did two unrelated jobs in one body: an `ast` walk that harvests the upstream module's top-level string and tuple constants, and then the mapping of the three copy lists that walk found onto `Copy` objects. Reading either required stepping over the other. The scan is now `_tuple_constants`, which returns only the tuples — the strings exist so `as_literal` can resolve a list entry that names a constant rather than repeating a path, and they are not the answer, so returning them would have left the caller holding a value it never reads. The mapping is now `_listed_copies`, which keeps both loops in their original order, so a `..._FROM_SHAPE` entry still overwrites a verbatim entry of the same path exactly as it did before. `upstream_copies` keeps the two exits that make it the entry point: no materializer at that commit, and a module that will not parse. No shape check moved and none was relaxed: the entry-shape tests each loop runs are the ones that were there, because these tuples arrive from a file read out of somebody's git history and a copy list this tool cannot vouch for is dropped rather than guessed at. upstream_copies 21 -> 2; _tuple_constants 9 (new); _listed_copies 10 (new). (complexipy's numbers; Sonar scored the original 19.) Proven by tests/test_update_shape.py, tests/test_update_shape_add.py and tests/test_commit_trailers.py: 178 passed. `tests/test_update_shape_add.py` is the one that drives this path — it renames copy-list entries out of a throwaway upstream clone and asserts the `upstream-added` verdicts that come back. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…the `shape:` block is and then only edits inside it, so finding the block and rewriting it are no longer interleaved in one body Sonar scored at 16 (#133) SonarCloud's `python:S3776` scored `rewrite_manifest` at 16 against a limit of 15. Two scans located the block — one for the `shape:` key, one for the next top-level key after it — with the `update-manifest-no-shape` refusal between them, and only then did the rewrite begin. Finding the block is a different question from editing it, and the answer is what SCOPES the edit: `commit:` and `tree_sha256:` are spelled the same way in every pin a manifest carries, so a rewrite that ran past the end of this block would move a leg's pin while claiming to move the shape's. `_shape_block_bounds` now answers it and raises the one refusal that belongs to it, because a bounds function with no bounds to return has nothing else to do. The end of the block is unchanged in every case: the next non-blank unindented line, or the end of the file when `shape:` is the last block a scaffold wrote. `rewrite_manifest` keeps the rewrite, the `seen` counts and the `update-manifest-unrecognised` refusal, all byte-identical. rewrite_manifest 15 -> 7; _shape_block_bounds 8 (new). (complexipy's numbers; Sonar scored the original 16.) Proven by tests/test_update_shape.py, tests/test_update_shape_add.py, tests/test_commit_trailers.py and tests/test_family.py: 252 passed. The family suite is in that list because a holder mirrors the same pin into `family.yaml` through this function. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he rollback ledger to a `Rollback` object, so the body Sonar scored at 47 is the sequence of what `apply` does rather than every decision inside all of it (#133) The function carried, in one body: the `--accept-local` unknown-path refusal, the `--add` notes, the nothing-to-do exit, the `update-conflict` and `update-local-drift` refusals, two nested closures that remembered every byte written, the copy/kept/added print loops, both pin rewrites, the validator run and its refusal, the branch/push/pull-request landing, and the two loops in the `except Refusal` arm that put the tree back. A reader who wanted to check one refusal read all of it, which is what a cognitive complexity of 47 is a statement about. What is here is the same code in the same order, moved: * `Rollback` is the two closures and the two undo loops as one object. It keeps the distinction they had — a file the root already owned is restored from its first bytes, a file `--add` created is deleted along with any directory made to hold it — and `undo` unwinds the creations in reverse so a directory is only considered after the file it was made for is gone. * `_refuse_unknown_accept_local`, `_refuse_conflicts`, `_refuse_local_drift` and `_refuse_red_validators` are one refusal each, asked in the order they were asked in, with every id, message and remediation byte-identical. The reasons those orderings are deliberate — a mistyped path reported as a mistyped path, `both` alone having an accepted exit — moved with them. * `_note_untaken_additions`, `_nothing_to_do`, `_write_copies`, `_repin` and `_land` are the report, the exit test, the write pass, the re-pin and the three optional acts after a clean rewrite. `cmd_apply` keeps the sequence, the `try`/`except`/`finally` and the two `return` values, so what it reads as now is the order in which `apply` does things. Nothing moved across the boundary `scripts/shape_materialize.py` shares: `commit_on_branch` and the `--trailer` path (#112) are untouched, and `_land` only calls them. complexipy 8.0.1 scores the function at 8, and every helper at 11 or less. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 2 days and 13 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideRefactors the four Sonar-flagged functions in Flow diagram for the refactored apply commandflowchart TD
A[cmd_apply] --> B[_refuse_unknown_accept_local]
B --> C[_note_untaken_additions]
C --> D{_nothing_to_do}
D -->|yes| E[return 0]
D -->|no| F[_refuse_conflicts]
F --> G[_refuse_local_drift]
G --> H[_write_copies]
H --> I[_repin]
I --> J[_refuse_red_validators]
J --> K[_land]
K --> L[return 0]
H -. Refusal .-> M[Rollback.undo]
I -. Refusal .-> M
J -. Refusal .-> M
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
🟢 Approval recommended
The only findings are minor documentation nits; behavior-preservation tests pass.
Pull request overview
Refactors update-shape.py into focused helpers to reduce cognitive complexity while preserving behavior, diagnostics, ordering, and rollback semantics.
Changes:
- Split classification, copy discovery, manifest rewriting, and apply phases.
- Centralized rollback tracking for overwritten and newly added files.
- Preserved CLI behavior and validation flow.
File summaries
| File | Summary | Findings |
|---|---|---|
update-shape.py |
Complexity-reduction refactor with centralized rollback handling. | Two nit comments request clarifying docstrings for missing pinned blobs and helper contracts (2 votes and 1 vote). |
Review details
Suppressed comments (2)
update-shape.py:1230
- This new docstring sentence is grammatically incomplete: it ends with “scored that function at;” and omits the score before continuing with “split out”. Please complete the sentence so the documentation is readable.
Sonar scored that function at; split out for #133, with the same writes in
update-shape.py:460
- The preceding explanation also implies that
_row_verdictis never called with a missing byte-string, butupstream.blob(pinned, source)may returnNoneand is deliberately handled by that helper as an upstream change. Distinguish these three early exits from the missing-pinned-blob case so the new helper contract is accurate.
THE THREE EXITS HERE ARE THE ONES WHERE THE COMPARISON CANNOT BE MADE AT
ALL, because one of the four byte-strings `classify` names does not
exist: the root has no file at this path, no upstream path corresponds to
it, or the upstream has none at the target commit. Each is a decision for
a human rather than a difference this tool can act on, so `_row_verdict`
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… say that the PINNED blob is the one byte-string allowed to be absent, so neither claims a contract the code does not have (#133) Copilot read the three new docstrings against the code on this branch and found that two of them overstate what the split guarantees. It is right, and the fix is the prose: nothing here changes a statement, and the file's AST is identical to the previous commit's once docstrings are stripped from both. * `_classify_row` said its three early exits cover "one of the four byte-strings" being missing, and that `_row_verdict` is therefore "never reached holding a hole". Three of the four are covered; the PINNED blob is not. `upstream.blob(pinned, source)` returns `None` for a source the standard did not carry at the pinned commit, and that value is passed straight on. The docstring now says which three the exits are, and that the pinned blob is the deliberate exception. * `_row_verdict` said it is "reached only for a row whose four byte-strings all read", which is the same overstatement from the other side, and its very next line is `pinned_bytes is None or ...`. It now says what a `None` pinned blob MEANS to the ladder: `upstream_changed` true and `verbatim_at_pin` false, because a copy cannot be a verbatim copy of a blob that was never there. * `Rollback`'s docstring compared the moved code to "a third of what Sonar scored that function at" without naming the score, which read to a reviewer as a sentence with a number missing from it. It names the 47. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Lane: openreposhape-2 (openRepoShape-2) Both suppressed comments from the 2026-09-12T03:43:48Z review are taken, in 4f302ec, along with the inline one. All three were documentation findings and all three were right; the commit changes prose only. Suppressed comment 1 — Suppressed comment 2 — Proof that behaviour did not move. Stripping every docstring from both revisions and comparing |
|
There was a problem hiding this comment.
🟢 Approval recommended
The only finding is a non-blocking naming nit; behavior and rollback semantics are preserved.
Review details
Suppressed comments (1)
update-shape.py:1229
Rollbackis a private implementation helper: it is only constructed bycmd_apply, alongside the private_...helpers introduced here. The refactor's documented convention is to give such helpers a leading underscore (as inscripts/repo_shape.py:178); please rename this class to_Rollbackand update its type references so it is not exposed as a public module symbol.
class Rollback:
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Lane: openreposhape-2 (openRepoShape-2) Answering the one suppressed comment from the 2026-09-12T14:37:39Z review ( The underscore convention this refactor followed is for private functions, and every helper it adds honours it:
The rename is safe in the mechanical sense, and if the house decides classes should take the underscore too, the right change is all six at once in its own commit rather than one new one here — this pull request's law is that it moves code without changing it, and a single inconsistent rename is a change to argue for on its own terms. Everything else in the review is agreed: behaviour and rollback semantics are preserved, which is what the docstring-only proof (identical |
Lane: openreposhape-2 (openRepoShape-2)
Claim: #133 (comment)
Closes #133.
What was wrong
SonarCloud's
python:S3776named four functions inupdate-shape.py. The rulecounts +1 per branch, loop and boolean-operator sequence, plus one per level of
nesting, so a number this far over the limit of 15 is a statement about how many
decisions a reader has to hold at once.
cmd_applyat 47 was the extreme case:one body carrying every refusal, the rollback ledger, the copy, both pin
rewrites, the validator run and the branch/push/pull-request landing, so a human
who wanted to check one refusal read all of it.
What landed
Code moved, nothing changed. Every refusal id, message, remediation, print, exit
code and exception type is byte-identical, and no side effect was reordered.
classify_classify_row(3), which keeps the three "cannot be compared" guards and hands the verdict ladder to_row_verdict(11)upstream_copies_tuple_constants(9) is theastscan,_listed_copies(10) the mapping of the three lists ontoCopyrewrite_manifest_shape_block_bounds(8) answers where theshape:block is and raisesupdate-manifest-no-shape; the rewrite stayscmd_applycmd_apply's phases, in the order it still runs them:_refuse_unknown_accept_local(1),
_note_untaken_additions(4),_nothing_to_do(1),_refuse_conflicts(5),_refuse_local_drift(4),_write_copies(10),_repin(0),_refuse_red_validators(3),_land(3). Its two nested closures and the two undoloops in the
except Refusalarm became oneRollbackobject (write0,write_added4,undo8), which keeps the distinction they had: a file the rootalready owned is restored from its first bytes, a file
--addcreated is deletedalong with any directory made to hold it.
cmd_applykeeps its sequence, itstry/except/finallyand bothreturnvalues. Nothing moved across the boundary
scripts/shape_materialize.pyshares:commit_on_branchand the--trailerpath (#112) are untouched and only called.cmd_checkis deliberately out of scope, as the issue records — Sonar does notflag it, and touching it would put a function at the limit into this diff for no
gain.
Pinned files touched: none. The diff is
update-shape.pyalone, 417insertions and 222 deletions. No
--helptext moved, sodocs/cli.mdisunchanged and
tests/test_cli_reference.pystill passes.Follow-ups left, not fixed here: none — no bug was found while restructuring.
How it was proven
tests/test_update_shape.py,tests/test_update_shape_add.pyandtests/test_commit_trailers.py, 178 passed. They assert the exact refusal ids(
update-accept-local-unknown,update-conflict,update-local-drift,update-add-would-overwrite,update-validators-red,update-branch-required,update-manifest-no-shape), the exactNEXTspellings including--trailer "Lane: $LANES_LANE", the rollback-on-red-validator behaviour and theper-file
--accept-local/--addsemantics from update-shape.py: --accept-local re-pins abothrow from the merged file, as AGENTS.md promised, and --branch may name the branch you are on (#84) #85 and update-shape.py:checkreportsupstream-added;apply --add <path>takes a file the standard gained since the pin #29.tests/test_[a-m]*.py366 passed, 1 skipped;tests/test_[n-z]*.py770 passed.1136 passed, 1 skipped — the baseline on
maintoday, with no new red andno new skip.
complexipy update-shape.py -d low) is the local measurebehind every number in the table above; no function added by this pull request
scores above 11.
PR-scoped after CI at head
4f302ec:curl -s 'https://sonarcloud.io/api/issues/search?componentKeys=opensoft_openRepoShape&pullRequest=139&statuses=OPEN&ps=100'returns
"total": 0— nopython:S3776left in this file, and no newissue of any rule the refactor could have introduced (S1192 duplicated
literals, S1172 unused parameter, S3776 in a new helper). The SonarCloud
gate passes, as do
tests,tests-macosandtests-windows.184becb): approval recommended, threedocumentation findings — two docstrings overstating that
_row_verdictcannever be reached with a missing byte-string, when the PINNED blob may be
Noneby design, and one sentence that compared against a score withoutnaming it. All three taken in
4f302ec, which is docstrings only: strippingevery docstring from both revisions and comparing
ast.dumpgives anidentical tree. Round 2 (head
4f302ec): approval recommended, one namingnit (rename
Rollbackto_Rollback) declined on the record — this filenames none of its six classes with a leading underscore, and the convention
the refactor followed is the underscore for private functions, which all
fourteen new helpers carry.
🤖 Generated with Claude Code