Repository navigation
Review residues from #247: read the base answer arm by arm, hold the default branch as one value - #257
Conversation
prepare_cold consumed BranchBase with a bare isinstance(base, StaleBase), so a third arm would have been read as a fresh base -- launching from a stale cache in silence, the exact defect the sum was added to close. Adds unhandled_branch_base beside the module's siblings and pins the refusal at the surface that reports staleness.
ensure_branch held it as two locals -- a name and an error string -- reconciled downstream by 'error or "no default branch is recorded"', with nothing stopping them disagreeing and falsiness read twice for the empty-name case. They travel as one sum now, resolved in one place that folds an empty answer into the same arm a raise takes, and the start point is read through one total function rather than an 'or' at each site. The base-branch fetch moves to _fetch_base_branch with it: the dispatch was three levels deep inside the requested ref's own. No behavior change -- the empty-name arm gained the pin it never had, and it passed before this refactor as well as after.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideRefactors default-branch handling into explicit sum types and helper methods and makes BranchBase handling exhaustive, to prevent silent stale-cache launches while keeping behavior unchanged. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
==========================================
+ Coverage 93.22% 93.26% +0.03%
==========================================
Files 25 25
Lines 3750 3771 +21
==========================================
+ Hits 3496 3517 +21
Misses 254 254
🚀 New features to boost your workflow:
|
A fresh-eyes review found one behavior drift in the refactor: the old 'error or sentence' fallback survived for an empty default-branch *name* but was dropped for an empty exception *message*, so an exception whose str() is empty would have printed 'could not be refreshed ()' -- a sentence with its reason cut out. Reported by exception type instead, which is the one intentional difference from what shipped: the old fallback said 'no default branch is recorded', which is not what happened when the resolver raised. Also from the review: _unhandled_default_branch joins its four private siblings in coverage's exclude_also -- ty rejects any reachable call, so a build that type-checks cannot execute it. The public unhandled_branch_base keeps its test instead, as unhandled_fetch_outcome and unhandled_unsaved already have. Drops two dead patches from that test and asserts the start point the empty-name arm cuts from.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Self-review (fresh-context reviewer + empirical parity check)
Verdict: approve. One real finding was raised and fixed; behavior parity is verified by enumeration rather than by argument.
Behavior drift — found and fixed. The refactor dropped the str(e) or <sentence> fallback for an empty exception message while keeping it for an empty name, so OSError() with no args would have printed could not be refreshed (). Fixed by falling back to the exception's type name; see the PR body for why that text rather than the old sentence. Pinned by test_a_resolver_failure_with_no_message_still_reports_a_reason.
Parity method. All 13 combinations of (requested-ref fetch outcome × default-branch resolution, including both empty-name and empty-message) were run against origin/main and against this branch, comparing the start_point handed to ensure_branch_exists, the returned arm, the fetch_ref call count, and the exact multiset of warnings. Diff is empty apart from the one intentional line above. That covers the reviewer's specific concerns: the except set is unchanged, the ValueError catch still scopes only the base fetch_ref (so an AssertionError from unhandled_fetch_outcome still propagates), and _start_point reproduces default_branch or "HEAD" exactly because _DefaultBranchNamed.name is non-empty by construction.
Exhaustiveness helpers. Total, and ty agrees — both arms return, so the parameter narrows to Never at the sentinel call.
One reviewer finding not taken, on a factual correction. The review recommended deleting the unhandled_branch_base test and covering the sentinel by coverage exclusion instead, on the premise that "the analogous branch for FetchOutcome has no such test anywhere in the suite." It does — test_workspace_clone.py asserts match="Unhandled fetch outcome" in two places. The house convention splits on visibility: public sentinels (unhandled_fetch_outcome, unhandled_unsaved) are pinned by tests, and only _-private ones (_unhandled_removal/source/status/usage) sit in exclude_also. Applied that split — private one excluded, public one tested — and took the valid sub-points: the two dead patches are gone and the test says why it patches the producer (ensure_branch's dispatch is total, so no input yields a fourth arm).
Remaining nits accepted as-is: the new test overlaps its neighbour (now differentiated by asserting the start point), the arm is mock-only because the real resolver falls back to "main", and the comment density is heavy but matches this file.
Ships two merges that landed after 0.0.28 was cut. #258: the claude-code feature mounts ~/.claude/skills and ~/.claude/wf-skills read-only. The allow-list had neither, so a workspace this repo's devcontainer built had no ~/.claude/skills at all and every skill installed on the host was invisible inside — a wf launch into a node landing in a session that could not see the /wf skill it was launched to run. Repo content rather than package content, so the fix arrives with a checkout of main and a container recreate, not with this version bump; the release is here so main and the published package agree. #257: prepare_cold names both arms of the base answer and refuses an unhandled one, closing the silent launch-from-stale-cache a third arm would have reintroduced behind #245's report. Goes out through Auto-publish → Conda Publish, the same way 0.0.28 did: no tag or GitHub release is created by hand.
1 participant
The two substantive non-blocking findings from #247's two-axis review, applied, plus the findings from a fresh-eyes review of this PR itself.
1.
prepare_coldconsumedBranchBasewith a bareisinstance(base, StaleBase), so a third arm added later would have been read as a fresh base — launching from a stale cache in silence, the exact defect the sum was added to close.repo_managerstates this rule for its own sum and exportsunhandled_fetch_outcomefor it;BranchBasenow hasunhandled_branch_basebeside it, the dispatch names both arms, and the refusal is pinned. Red first: the bogus arm sailed straight through before the change.2.
ensure_branchheld the default branch as two locals — a name and an error string — reconciled downstream bydefault_branch_error or "no default branch is recorded", with nothing preventing them from disagreeing and falsiness read twice for the empty-name case. They travel as one sum now (_DefaultBranchNamed | _DefaultBranchUnknown), resolved in one place that folds an empty answer into the same arm a raise takes, with the start point read through one total function instead of anorat each site. The base-branch fetch moved to_fetch_base_branchwith it — its dispatch was nested three levels inside the requested ref's own. This one is a refactor, so it is pinned rather than red-first.Behavior parity was verified empirically, not just argued. All 13 combinations of (requested-ref outcome × default-branch resolution) were enumerated against
origin/mainand against this branch, comparing thestart_pointpassed toensure_branch_exists, the returned arm, thefetch_refcount, and the exact set of warnings. Output is identical except for one intentional difference, below.The one intentional behavior difference. The old
str(e) or "no default branch is recorded"fallback survived the refactor for an empty default-branch name but was dropped for an empty exception message — caught by the review of this PR.OSError()with no args would have rendered ascould not be refreshed (), a sentence with its reason cut out. Rather than restore the old string, the exception's type is now the fallback (OSError with no message): the old text claimed no default branch was recorded, which is not what happened when the resolver raised. Unreachable in practice today — the realget_default_branchnever raises on its own — and pinned by a test either way.Also from the review of this PR:
_unhandled_default_branchjoins its four private siblings in coverage'sexclude_also, sincetyrejects any reachable call and a build that type-checks cannot execute it. The publicunhandled_branch_basekeeps its test instead — the house splits exactly this way, withunhandled_fetch_outcomeandunhandled_unsavedboth pinned by tests and only the_-private sentinels excluded. Two dead patches came off that test, and the empty-name arm now asserts the start point it cuts from.Three findings from #247 deliberately not actioned, with reasons:
ensure_branch.BranchBaseaboveprepare_cold— no programmatic consumer exists today, and ensure_branch: the three arms that cut a branch from an unfetched cache report it to the caller #245 framed wf's channel as dl's output. Widening the return shape for a hypothetical reader would trade this finding for Speculative Generality.Full suite: 1786 unit + 48 integration green, pylint 10/10.
🤖 Generated with Claude Code
Summary by Sourcery
Make branch-base and default-branch handling exhaustive and consistent while preserving existing launch behavior.
Bug Fixes:
Enhancements:
Tests: