Repository navigation
shape-doctor.py: the five checks and verdicts Sonar capped now answer one question per function, every one of them under the 15 cognitive complexity allowed and behaviour byte-identical (#134) - #142
Conversation
…unction instead of three in one body, dropping its cognitive complexity from 26 to 8 (#134) `python:S3776` was OPEN against `check_leg_shape_files` at 26, over the 15 Sonar allows. The body did three separate jobs: it classified every shape file of every leg through a five-way `if`/`elif` chain, it worded the row's reason, and it decided which of the missing files a copy would even fix. Each of those is a question with its own answer, and a reader who came here because the row said something unexpected had to trace all three to find out which one said it. Four helpers, one per decision, each carrying the part of the WHY that moved with it: `shape_file_state` the five-way chain: what ONE file is against the template's copy, and why `rendered` is an answer rather than `differs`. `leg_shape_comparison` ONE leg: its `--json` entry and the files that are not there, with both early returns saying which kind of "nothing to compare" they are. `leg_shape_summary` one leg's half of the row's reason. `missing_shape_file_fix` the next command, or why a copy is not one -- `copy_command` is still the only place a shell verb is spelled (#105), and the rendered-template argument from #96 moved into the docstring intact. Behaviour is byte-identical: the same strings, in the same order, on every branch. `LABEL_LEG_SHAPE_FILES` still names all three returns, the `note`-not-`FINDING` comment stays on the branch it argues, and the `CHECKS` entry is untouched. Measured with complexipy (SonarSource's spec): `check_leg_shape_files` 31 -> 8, and no helper above 6. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…and reaches a verdict in three functions instead of one body, dropping its cognitive complexity from 22 to 13 (#134) `python:S3776` was OPEN against `check_placement` at 22, over the 15 Sonar allows. The body opened with four different `n/a` refusals before it had classified a single path, closed with a five-branch precedence ladder, and wrote one leg's half of the reason as a conditional expression nested three deep inside a `join`. A reader who came here because the row said `n/a` had to read past the audit to find out which of the four refusals said it. Three helpers, one per decision, each carrying the part of the WHY that moved with it: `placement_policy` the three refusals that are THIS CHECKOUT being short rather than the repository being wrong, including the unreadable policy file, whose `n/a`-not-a-finding argument moved into the docstring intact. `leg_audit_summary` one leg's half of the row's reason: its counts, or the `state` that says why it has none -- with the Copilot PR #100 argument for printing the state rather than a zero. `placement_verdict` the precedence ladder, and why each branch outranks the one below it. It reads the counts out of the `detail` the row prints, so a number this judges on and a number the reader is shown cannot drift apart. Behaviour is byte-identical: the same strings, in the same order, on every branch. The row's `next` command is built before the verdict rather than after the first of its branches, which is a pure f-string and writes nothing. `placement_row` still puts the `--json` keys on every branch, the `placement` id and label are untouched, and no message text, exit code or written file changes. Measured with complexipy (SonarSource's spec): `check_placement` 25 -> 13, `placement_policy` 6, `placement_verdict` 7, `leg_audit_summary` 2. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tead of in one run of appends, dropping its cognitive complexity from 22 to 7 (#134) `python:S3776` was OPEN against `placement_plan_text` at 22, over the 15 Sonar allows. The function wrote all four blocks of one YAML file into a single list: the header, every leg, every path to resolve, and what was never judged. Nothing separated them but blank-line appends, so a reader looking for the line that wrote a key they did not expect had to count indents to find out which block they were even in. Four helpers, one per block of the file, each carrying the part of the WHY that moved with it: `placement_plan_header` the first block, and that `kind:` is the SAFETY property -- `PLACEMENT_PLAN_KIND`, never `PLAN_KIND`, is what stops `adopt-project.py execute` being handed one of these. `placement_plan_leg` ONE leg's counts, and why a tracked name no plan can carry is written as a comment with its newlines flattened rather than as a value. `placement_plan_entry` ONE path, in an adoption plan entry's keys exactly -- the claim the adoption's own reader is held to in `test_the_placement_plans_entries_are_adoption_plan_entries`. `placement_plan_paths` the `paths:` block, including the empty one that says what its emptiness means when a leg could not be read at all. Behaviour is byte-identical: the same lines, in the same order, emitted through the same `adopt.y` and `adopt.emit`. Each helper builds and returns its own list rather than appending into the caller's, which is the only change to how the lines are accumulated; the file they join into is unchanged, `--placement-plan` still writes exactly one file where the caller named it, and `write_placement_plan` is untouched. Measured with complexipy (SonarSource's spec): `placement_plan_text` 22 -> 7, and no helper above 4. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…and words the row in three functions instead of one loop, dropping its cognitive complexity from 29 to 8 (#134) `python:S3776` was OPEN against `check_members` at 29, over the 15 Sonar allows. One loop body read both copies of every member, decided which of three faults it had, decided whether its state was worth saying, and fed three different lists that the wording below the loop then read back. A reader who came here because a member was called `not populated` had to hold the whole loop in their head to see that the same pass had also decided whether a working clone was beside the holder. Three helpers, one per decision, each carrying the part of the WHY that moved with it: `member_state` both copies of ONE member as they are found, and why nothing is asked of a mount that is not populated: `git rev-parse HEAD` inside an empty submodule answers about the HOLDER, so an unfetched member would otherwise be reported as checked out at the holder's commit. The 40-hex rule for `pin.commit` moved here too. `member_problem` the three faults, in the only order they make sense in, and that a member at its pin but on a branch is deliberately not one of them. `members_verdict` the finding and the two `ok` wordings, and why only the pinned copies can make this row red -- the PR #96 argument about `family.py add` leaving a branch behind stays on the branch that records it. Behaviour is byte-identical: the same strings, in the same order, on every branch, and the `--json` entry for a member has the same keys in the same order it always had. The `if`/`elif` ladder over the three faults became three early returns in the helper, which answers in the same order and therefore with the same message. Measured with complexipy (SonarSource's spec): `check_members` 26 -> 8, and no helper above 8. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…arguing its own place, dropping the ladder's cognitive complexity from 33 to 11 (#134) `python:S3776` was OPEN against `verdict_for` at 33, the highest in the file and over twice the 15 Sonar allows. The precedence the module docstring argues at length was one body with the argument scattered through it as comments between branches, so the order -- the thing a reader comes to this function for -- had to be reconstructed from forty lines of counting, comprehensions and early returns. Five helpers. The order of the calls in `verdict_for` is now the precedence, readable as the list it is, and each rung argues its own place in its own docstring: `drifted_file_reasons` the drifted copies quoted in the line, and why the two `if` chains were two groups: the states that MADE it drifted, then the states in which the standard and the copy no longer map onto each other. Same states, same order, still named only when non-zero. `drifted_reasons` that a drifted copy, a leg off its pin and a member off its pin are one verdict because they are one fact. `misplaced_verdict` UNDER DRIFTED AND OVER INVALID, and the PR #100 argument for why the list has to be non-empty rather than the row merely red. `invalid_verdict` the named gates, each of which somebody else owns. `behind_verdict` both kinds of behind, and why the pin-only one quotes commits where the other quotes counts. Behaviour is byte-identical: the same verdict strings and the same exit codes (0, 1, 2, 3) in the same order of precedence. `counts` is read inside the two helpers that use it instead of once at the top, which is the same dictionary read the same way. The catch-all stays in `verdict_for` with its comment, because it is the rung that exists so that a rung nobody wrote still fails loudly. Measured with complexipy (SonarSource's spec): `verdict_for` 38 -> 11, and no helper above 10. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@codex review |
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 3 days and 10 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideRefactors the five Sonar cognitive-complexity hotspots in shape-doctor.py into focused, single-purpose helpers, reducing each flagged function below the allowed threshold while preserving output, verdict precedence, exit codes, command generation, and other observable behavior. Sequence diagram for placement check and verdict constructionsequenceDiagram
participant Doctor as check_placement
participant Policy as placement_policy
participant Audit as audit_leg
participant Summary as leg_audit_summary
participant Verdict as placement_verdict
Doctor->>Policy: placement_policy(ctx, policy_path)
Policy-->>Doctor: adopt, policy, or refusal row
loop each leg
Doctor->>Audit: audit_leg(adopt, policy, patterns, role, rel, ...)
Audit-->>Doctor: entry, misplaced, review
Doctor->>Summary: leg_audit_summary(entry)
Summary-->>Doctor: audited text
end
Doctor->>Verdict: placement_verdict(detail, audited, plan)
Verdict-->>Doctor: placement row with status and reason
Flow diagram for shape-doctor verdict precedenceflowchart TD
Start[verdict_for] --> Root{ctx.kind is NOT_A_ROOT?}
Root -->|yes| NotRoot[V_NOT_A_ROOT, exit 2]
Root -->|no| Blocked{cannot_answer returns a row?}
Blocked -->|yes| Cannot[CANNOT_ANSWER, exit 3]
Blocked -->|no| Drift[drifted_reasons]
Drift -->|reasons| Drifted[DRIFTED, exit 1]
Drift -->|none| Misplaced[misplaced_verdict]
Misplaced -->|verdict| MisplacedResult[MISPLACED, exit 1]
Misplaced -->|none| Invalid[invalid_verdict]
Invalid -->|verdict| InvalidResult[INVALID, exit 1]
Invalid -->|none| Behind[behind_verdict]
Behind -->|verdict| BehindResult[SHAPE BEHIND, exit 1]
Behind -->|none| CatchAll[INVALID for remaining findings, exit 1]
Flow diagram for placement plan YAML generationflowchart TD
Start[placement_plan_text] --> Header[placement_plan_header]
Header --> Legs[placement_plan_leg for each leg]
Legs --> Entries[placement_plan_paths]
Entries --> HasEntries{entries present?}
HasEntries -->|yes| Entry[placement_plan_entry for each path]
HasEntries -->|no| Empty["paths: [] and unread-leg explanation"]
Entry --> Output[Complete placement plan text]
Empty --> Output
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.
🔵 Needs a closer look
Member detection can accept a same-ID repository with the wrong origin; add origin validation and a regression test.
Pull request overview
Refactors five high-complexity shape-doctor.py functions into focused helpers while preserving behavior and output.
Changes:
- Split check, placement, member, plan, and verdict logic.
- Preserved safety checks and output contracts.
- Limited changes to
shape-doctor.py.
File summaries
| File | Summary |
|---|---|
shape-doctor.py |
Refactored complex checks and verdict logic; member detection still needs origin validation. |
Review details
Suppressed comments (1)
shape-doctor.py:2100
- This still treats any beside-holder repository with the same
project.yamlid (or name) as the member, becauseis_this_memberdoes not verify the clone'sorigin. A fork or unrelated repository can therefore be reported asworking_clone, somemberssays the required working checkout exists when it does not; the family sibling verifier requires both origin and project id (seetemplates/family-root/scripts/siblings.py:405-435). Reuse the same origin-normalization check here and add a same-id/wrong-origin regression case.
has_sibling = is_this_member(sibling, row)
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
Lane: openreposhape-2 (openRepoShape-2) (lane openreposhape-2 (openRepoShape-2), 2026-09-12T14:58Z) Copilot round 1, one finding, The finding is real and it is pre-existing. Why not in this pull request. So it is filed, unclaimed, with the evidence and the shape decision in it: CI is green on all three platforms (ubuntu, macos, windows) and the SonarCloud |



Lane: openreposhape-2 (openRepoShape-2)
Claim: #134 (comment)
Closes #134.
What was wrong
SonarCloud had five open
python:S3776issues againstshape-doctor.py, oneper function whose cognitive complexity was over the 15 the rule allows. Every
one of them was a check or a verdict — the parts of this file a reader opens
when the doctor said something they did not expect — and each was long enough
that the answer was found by tracing a ladder rather than by reading a
sentence.
What landed
Five commits, one per function, each moving the part of the WHY that went with
the code it moved. No file but
shape-doctor.pyis touched, and nodigest-pinned file is touched:
shape-doctor.pyis not one of the pinnedcopies, no
--helptext moved, sodocs/cli.mdis unchanged andtests/test_cli_reference.pyhad nothing to regenerate.check_leg_shape_filesshape_file_state(what ONE file is against the template),leg_shape_comparison(ONE leg and the files that are absent),leg_shape_summary(one leg's half of the reason),missing_shape_file_fix(the next command, or why a copy is not one)check_placementplacement_policy(the three refusals that are this CHECKOUT being short),leg_audit_summary(one leg's counts, or why it has none),placement_verdict(the precedence ladder, reading its counts out of thedetailthe row prints)placement_plan_textplacement_plan_header,placement_plan_leg,placement_plan_entry,placement_plan_pathscheck_membersmember_state(both copies of ONE member as found),member_problem(the three faults, in the only order they make sense in),members_verdict(the finding and the twookwordings)verdict_fordrifted_file_reasons,drifted_reasons,misplaced_verdict,invalid_verdict,behind_verdict— the catch-all stays, with its commentcc beforeis SonarCloud's number, with complexipy's in parentheses where thetwo differ;
cc afteris complexipy's, which implements the same SonarSourcespec. No new helper is above 10.
Behaviour is byte-identical. No message text, exit code, output order,
file written or exception type changes.
quote_argis still the one quoterand
copy_commandstill the only place in the file a shell verb is spelled;the
CHECKSregistry keeps its shape and its emptyfixslots; theLABEL_*constants still name the rows; and every guarded import of
repo_shape,path_classifyandshape_materializeis untouched — the refactor adds nocross-module import.
Two moves worth naming, neither of them observable:
check_placementbuildsthe row's
nextcommand (a pure f-string) before the verdict rather thanafter the first of its branches, and
verdict_forreadscountsinside thetwo helpers that use it instead of once at the top.
Left as a follow-up, unclaimed. complexipy measures
check_shape_currencyat 20, but SonarCloud does not flag it — it is not one of the five issues #134
names, so it was left alone rather than refactored on a second tool's word.
Worth an issue if Sonar ever raises it.
Copilot's round-1 finding is the second, and it is pre-existing:
is_this_memberaccepts a beside-the-holder repository on itsproject.yamlid alone, where the family's own
verify_siblingrefusesWRONG ORIGINfirst. That function is byte-identical here, so it was left alone under this
PR's law and filed instead — #146, unclaimed, with the evidence and the
placement decision the fix needs. Nothing else was found while restructuring.
How it was proven
tests/test_shape_doctor.py— 144 passed after each of the fivecommits, including the AST-level guards this refactor had to keep true:
test_the_only_shell_verb_this_file_spells_goes_through_copy_command,test_the_registry_leaves_a_fix_slot_for_the_repair_mode,test_no_other_row_grows_a_trailer,test_the_placement_plans_entries_are_adoption_plan_entriesandtest_a_tracked_name_no_plan_can_carry_is_reported_and_never_written.366 passed / 1 skipped (
test_[a-m]*.py), 349 passed (test_[n-r]*.py),421 passed (
test_[s-z]*.py) — 1136 passed, 1 skipped, against 1137collected, which is the whole of
tests/. No red, no flake.and in each commit message.
🤖 Generated with Claude Code