Repository navigation
Fetch exactly the branch being launched, and nothing else - #185
Conversation
The cold launch path made an unbounded network call while holding the per-repo flock: once the fetch interval had elapsed, ensure_repo swept every head and tag before the launch could proceed, so whichever launch drew the short straw paid for everyone's freshness and every concurrent launch of the same repo queued behind it. The workspace clone then made a second fetch whose output nothing read. Per the devlaunch#144 resolution, and now that the detached updater sweep (devlaunch#149) exists to carry broad freshness: - ensure_repo becomes the pure clone-if-missing primitive. Its auto_fetch parameter goes with the fetch it gated; production never passed False. - ensure_branch fetches one targeted refspec, unconditionally. The outcome is a three-way sum -- Updated / RefMissingOnRemote / FetchFailed -- so "the remote says the ref is not there" stays distinguishable from "the remote did not answer": only the former licenses creating the branch from the default branch, and only the latter means fall back to cache. It does not write last_fetched, which keeps the sweep's bookkeeping and one repo->metadata lock nesting off this path. - _prepare_workspace's fetch is deleted; the checkout beside it is a plain `git checkout <branch>` that never consulted a remote-tracking ref. The staleness contract this buys: push upstream, immediately dl the branch, and the workspace starts at the pushed tip. Offline still launches from cache. Lock cycles are untouched -- devlaunch#143 owns that seam. Closes #150
Reviewer's GuideRefactors the cold launch path to fetch only the specific branch being launched, removes foreground broad fetches and workspace-level fetches, introduces a typed fetch outcome for targeted ref fetches, and updates tests and docs to pin the new staleness contract and API surface. Sequence diagram for the updated cold launch branch fetch pathsequenceDiagram
actor User
participant DevpodCLI
participant WorkspaceCloneManager
participant RepositoryManager
participant GitRemote
User->>DevpodCLI: dl owner/repo branch
DevpodCLI->>WorkspaceCloneManager: ensure_workspace(owner, repo, branch)
WorkspaceCloneManager->>RepositoryManager: ensure_repo(owner, repo, remote_url)
note over RepositoryManager: clone-if-missing only, no fetch sweep
WorkspaceCloneManager->>WorkspaceCloneManager: ensure_branch(owner, repo, branch)
WorkspaceCloneManager->>RepositoryManager: fetch_ref(owner, repo, branch)
RepositoryManager->>GitRemote: git fetch origin +refs/heads/branch:refs/heads/branch
GitRemote-->>RepositoryManager: exit status + stderr
alt outcome is Updated
RepositoryManager-->>WorkspaceCloneManager: Updated()
WorkspaceCloneManager->>WorkspaceCloneManager: proceed without extra fetch
else outcome is RefMissingOnRemote
RepositoryManager-->>WorkspaceCloneManager: RefMissingOnRemote()
WorkspaceCloneManager->>RepositoryManager: fetch_ref(owner, repo, default_branch)
else outcome is FetchFailed
RepositoryManager-->>WorkspaceCloneManager: FetchFailed(reason)
WorkspaceCloneManager->>WorkspaceCloneManager: log warning, use cached branch
else unexpected outcome
WorkspaceCloneManager->>WorkspaceCloneManager: unhandled_fetch_outcome(outcome)
end
WorkspaceCloneManager->>WorkspaceCloneManager: _prepare_workspace(workspace, ws_path, remote_url)
note over WorkspaceCloneManager: no git fetch in workspace clone
WorkspaceCloneManager->>WorkspaceCloneManager: git checkout branch
WorkspaceCloneManager-->>DevpodCLI: workspace ready
DevpodCLI-->>User: devpod launched at branch tip or cache
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
==========================================
+ Coverage 95.33% 95.57% +0.23%
==========================================
Files 22 22
Lines 3025 3051 +26
==========================================
+ Hits 2884 2916 +32
+ Misses 141 135 -6
🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
fetch_reflogic relies on matching the literal substring"couldn't find remote ref"in stderr to detect a missing ref; consider using a more robust criterion (e.g. parsing git’s exit code and standardized message pattern) to avoid locale/format changes breaking this classification. - In
fetch_ref, onlyrefs/heads/<branch>is supported and hard-coded; if you ever need to handle tags or non-head refs, it may be cleaner to generalize the refspec construction (or validate that callers only ever pass branch names) and document this constraint near the method signature.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `fetch_ref` logic relies on matching the literal substring `"couldn't find remote ref"` in stderr to detect a missing ref; consider using a more robust criterion (e.g. parsing git’s exit code and standardized message pattern) to avoid locale/format changes breaking this classification.
- In `fetch_ref`, only `refs/heads/<branch>` is supported and hard-coded; if you ever need to handle tags or non-head refs, it may be cleaner to generalize the refspec construction (or validate that callers only ever pass branch names) and document this constraint near the method signature.
## Individual Comments
### Comment 1
<location path="devlaunch/worktree/repo_manager.py" line_range="79-82" />
<code_context>
+FetchOutcome = Union[Updated, RefMissingOnRemote, FetchFailed]
+
+
+def unhandled_fetch_outcome(outcome: NoReturn) -> NoReturn:
+ """Reject a fetch outcome nobody handled -- at type-check time, not at runtime.
+
+ The same shape as :func:`devlaunch.workspace_state.unhandled_unsaved`, and
+ exported for the same reason: the arms are read outside this module, and an
+ ``else`` hand-rolled at a call site is how a fourth arm gets silently read as
+ one of the existing three.
+ """
+ raise AssertionError(f"Unhandled fetch outcome: {outcome!r}")
+
+
</code_context>
<issue_to_address>
**suggestion:** The `NoReturn` parameter type on `unhandled_fetch_outcome` is counterintuitive and may hinder static checking.
Given that callers pass a `FetchOutcome` and this function always raises, typing the parameter as `NoReturn` is misleading and may confuse type checkers. Using `FetchOutcome` (or a union of the concrete outcome types) for the parameter and relying on the `NoReturn` return type would better express the intent, avoid call-site type incompatibilities, and keep `isinstance`-based narrowing working as expected.
```suggestion
FetchOutcome = Union[Updated, RefMissingOnRemote, FetchFailed]
def unhandled_fetch_outcome(outcome: FetchOutcome) -> NoReturn:
```
</issue_to_address>
### Comment 2
<location path="devlaunch/worktree/repo_manager.py" line_range="35" />
<code_context>
return ref.rsplit("/", maxsplit=1)[-1]
+@dataclass(frozen=True)
+class Updated:
+ """The requested ref in the cache now matches the remote's.
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the three separate outcome dataclasses and union with a single tagged FetchOutcome object carrying a kind enum and optional reason to simplify the result type while preserving semantics.
You can keep the three-way semantics and type-safety while collapsing the “marker dataclasses + union + unhandled helper” into a single tagged outcome object. This removes two empty classes, the union alias, and the NoReturn helper, while preserving all behavior.
For example:
```python
from dataclasses import dataclass
from enum import Enum
from typing import Optional
class FetchOutcomeKind(Enum):
UPDATED = "updated"
REF_MISSING_ON_REMOTE = "ref_missing_on_remote"
FETCH_FAILED = "fetch_failed"
@dataclass(frozen=True)
class FetchOutcome:
"""Result of a fetch_ref call.
kind:
- UPDATED: cache now matches remote
- REF_MISSING_ON_REMOTE: remote explicitly says ref does not exist
- FETCH_FAILED: remote could not be queried; reason explains why
"""
kind: FetchOutcomeKind
reason: Optional[str] = None
```
Then `fetch_ref` becomes:
```python
def fetch_ref(self, owner: str, repo: str, branch: str) -> FetchOutcome:
validate_ref_name(branch)
bare_path = self.get_bare_path(owner, repo)
if not bare_path.exists():
return FetchOutcome(
kind=FetchOutcomeKind.FETCH_FAILED,
reason=f"Repository {owner}/{repo} does not exist locally",
)
logger.info(f"Fetching {branch} for {owner}/{repo}")
try:
subprocess.run(
["git", "fetch", "origin", f"+refs/heads/{branch}:refs/heads/{branch}"],
cwd=bare_path,
capture_output=True,
text=True,
check=True,
)
return FetchOutcome(kind=FetchOutcomeKind.UPDATED)
except subprocess.CalledProcessError as e:
stderr = (e.stderr or "").strip()
if "couldn't find remote ref" in stderr:
logger.debug(f"Remote has no ref {branch} for {owner}/{repo}")
return FetchOutcome(kind=FetchOutcomeKind.REF_MISSING_ON_REMOTE)
return FetchOutcome(
kind=FetchOutcomeKind.FETCH_FAILED,
reason=stderr or f"git fetch exited {e.returncode}",
)
```
And call sites do explicit exhaustive handling on `kind` without needing `unhandled_fetch_outcome`:
```python
outcome = repo_manager.fetch_ref(owner, repo, branch)
if outcome.kind is FetchOutcomeKind.UPDATED:
...
elif outcome.kind is FetchOutcomeKind.REF_MISSING_ON_REMOTE:
...
elif outcome.kind is FetchOutcomeKind.FETCH_FAILED:
log_error(outcome.reason)
...
```
Benefits:
- Removes two zero-field dataclasses and the separate `FetchOutcome` union.
- Keeps a single, obvious “result object” with a `kind` and optional `reason`.
- Maintains the semantic distinction the caller relies on (`updated` vs `no ref` vs `failed`).
- Keeps type-level clarity while making the control flow easier to read and reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| FetchOutcome = Union[Updated, RefMissingOnRemote, FetchFailed] | ||
|
|
||
|
|
||
| def unhandled_fetch_outcome(outcome: NoReturn) -> NoReturn: |
There was a problem hiding this comment.
suggestion: The NoReturn parameter type on unhandled_fetch_outcome is counterintuitive and may hinder static checking.
Given that callers pass a FetchOutcome and this function always raises, typing the parameter as NoReturn is misleading and may confuse type checkers. Using FetchOutcome (or a union of the concrete outcome types) for the parameter and relying on the NoReturn return type would better express the intent, avoid call-site type incompatibilities, and keep isinstance-based narrowing working as expected.
| FetchOutcome = Union[Updated, RefMissingOnRemote, FetchFailed] | |
| def unhandled_fetch_outcome(outcome: NoReturn) -> NoReturn: | |
| FetchOutcome = Union[Updated, RefMissingOnRemote, FetchFailed] | |
| def unhandled_fetch_outcome(outcome: FetchOutcome) -> NoReturn: |
| return ref.rsplit("/", maxsplit=1)[-1] | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
There was a problem hiding this comment.
issue (complexity): Consider replacing the three separate outcome dataclasses and union with a single tagged FetchOutcome object carrying a kind enum and optional reason to simplify the result type while preserving semantics.
You can keep the three-way semantics and type-safety while collapsing the “marker dataclasses + union + unhandled helper” into a single tagged outcome object. This removes two empty classes, the union alias, and the NoReturn helper, while preserving all behavior.
For example:
from dataclasses import dataclass
from enum import Enum
from typing import Optional
class FetchOutcomeKind(Enum):
UPDATED = "updated"
REF_MISSING_ON_REMOTE = "ref_missing_on_remote"
FETCH_FAILED = "fetch_failed"
@dataclass(frozen=True)
class FetchOutcome:
"""Result of a fetch_ref call.
kind:
- UPDATED: cache now matches remote
- REF_MISSING_ON_REMOTE: remote explicitly says ref does not exist
- FETCH_FAILED: remote could not be queried; reason explains why
"""
kind: FetchOutcomeKind
reason: Optional[str] = NoneThen fetch_ref becomes:
def fetch_ref(self, owner: str, repo: str, branch: str) -> FetchOutcome:
validate_ref_name(branch)
bare_path = self.get_bare_path(owner, repo)
if not bare_path.exists():
return FetchOutcome(
kind=FetchOutcomeKind.FETCH_FAILED,
reason=f"Repository {owner}/{repo} does not exist locally",
)
logger.info(f"Fetching {branch} for {owner}/{repo}")
try:
subprocess.run(
["git", "fetch", "origin", f"+refs/heads/{branch}:refs/heads/{branch}"],
cwd=bare_path,
capture_output=True,
text=True,
check=True,
)
return FetchOutcome(kind=FetchOutcomeKind.UPDATED)
except subprocess.CalledProcessError as e:
stderr = (e.stderr or "").strip()
if "couldn't find remote ref" in stderr:
logger.debug(f"Remote has no ref {branch} for {owner}/{repo}")
return FetchOutcome(kind=FetchOutcomeKind.REF_MISSING_ON_REMOTE)
return FetchOutcome(
kind=FetchOutcomeKind.FETCH_FAILED,
reason=stderr or f"git fetch exited {e.returncode}",
)And call sites do explicit exhaustive handling on kind without needing unhandled_fetch_outcome:
outcome = repo_manager.fetch_ref(owner, repo, branch)
if outcome.kind is FetchOutcomeKind.UPDATED:
...
elif outcome.kind is FetchOutcomeKind.REF_MISSING_ON_REMOTE:
...
elif outcome.kind is FetchOutcomeKind.FETCH_FAILED:
log_error(outcome.reason)
...Benefits:
- Removes two zero-field dataclasses and the separate
FetchOutcomeunion. - Keeps a single, obvious “result object” with a
kindand optionalreason. - Maintains the semantic distinction the caller relies on (
updatedvsno refvsfailed). - Keeps type-level clarity while making the control flow easier to read and reason about.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed at merge-base dcdb077, diff dcdb077...dd6c3f7. Preflight green: ref resolves, diff non-empty, all 11 checks passing (ci on 3.10–3.14, e2e, gate, prek, codecov). Two axes below were run as independent agents in fresh context and are not merged or reranked against each other — a change can pass one and fail the other.
Standards
Repo-documented standards first, then the Fowler baseline. Things tooling already enforces are skipped.
Verified clean, because the PR description makes claims worth checking: no dead code — _should_fetch is still reached via lazy_fetch:342, lazy_fetch via the detached updater at dl.py:502, fetch_repo via lazy_fetch:343. The Updated/RefMissingOnRemote/FetchFailed + unhandled_fetch_outcome shape does genuinely match workspace_state.py's Unsaved/unhandled_unsaved (frozen no-payload arms, reason-carrying failure arm, NoReturn rejector). Stringly-typed stderr matching has repo precedent at branch_manager.py:83. Comment density matches the surrounding norm. The deleted step-4 fetch was not propping up the checkout.
-
test/integration/test_repo_manager_real.py:380-382— two statements inside onewith pytest.raises(RuntimeError):.ensure_branchraises first, so theensure_workspaceline below it is unreachable and never asserted. The test passes on half of what it names. -
devlaunch/worktree/workspace_clone.py:381— the second (default-branch)fetch_refdiscards itsFetchOutcomeentirely, so aFetchFailedthere is silent, while the identical condition one arm down at:393is warned about with its reason. Inconsistent with this module's own stated contract that the reason is carried so it can be printed. -
devlaunch/worktree/config.py:23,51,72—WorktreeConfig.auto_fetchnow has zero production readers but is still serialized intoconfig.toml. Partly pre-existing (it was never wired to the parameter), but deletingensure_repo'sauto_fetchremoved the last thing that name plausibly controlled. A config knob that does nothing is worse than no knob. -
Judgement call.
workspace_clone.py:361vs:381—ValueErrorfromvalidate_ref_nameis caught for the default-branch call and not for the requested-branch call. The comment justifies this with "usually holds aWorkspaceId", butensure_branchis public and the integration tests call it with bare strings. -
Minor.
test/integration/test_repo_manager_real.py:226—_commit_on(..., push=True)is never called withpush=False; speculative parameter in a new helper. -
Judgement call.
workspace_clone.py:371-372—if isinstance(outcome, Updated): passempty arm, whereworkspace_state.unsaved_as_jsonuses an early-return chain for the same exhaustiveness. The empty arm does buy the "each arm named" property the comment claims.
Spec
Against #150, whose criteria trace to the #144 decision. All five spec bullets are genuinely done — see the verified list at the bottom.
-
devlaunch/worktree/repo_manager.py:311-318— the ref-missing arm is detected by an English substring, which collapses the three-way model back to two under a non-English locale.#150: "Model the outcome as the three-way result from #144 (updated / ref-missing / network-failed → proceed-fresh / create-from-default / warn-and-use-cache-or-error), not a bool."
if "couldn't find remote ref" in stderrmatches a string git marks for translation (_()inbuiltin/fetch.c); Debian and Fedora shipgit.mo. Verified: the message is exactlyfatal: couldn't find remote ref refs/heads/<b>, andgrep -rn "LC_ALL\|LANGUAGE" devlaunch/returns nothing — no subprocess here pins a locale. So underLANG=de_DEetc., an ordinary "start a new branch" launch classifies asFetchFailed, which by the spec's own model means warn-and-use-cache-or-error: no default-branch fetch, the new branch is cut from a stale cached default, and the user gets a spurious warning. PinningLC_ALL=C/LANGUAGE=Cin thefetch_refsubprocess env, or classifying withgit ls-remote --heads, closes it. -
test/integration/test_repo_manager_real.py:380-382— criterion (3)'s "ref nowhere" pin is weaker than it reads.#150: "staleness contract integration: … ref nowhere → existing error."
Confirmed
ensure_branchraises first (Failed to create branch: fatal: not a valid object name: 'main'), so line 382 is dead. It does still pin that the error happens — same error as atdcdb077— but not where, and it contradictsensure_branch's own docstring atworkspace_clone.py:342-344: "the checkout in_prepare_workspaceraises — that is where its emptiness is discovered." It is not;ensure_branchis. -
Note, not a change request — belongs on #150.
devlaunch/worktree/workspace_clone.py:494-500— existing-workspace relaunch now opens with staleorigin/*refs.#150: "Delete
_prepare_workspacestep 4 (its output is unused)."Its output is unused by the checkout, which is exactly what the spec asserts and what this PR argues. But the deleted
git fetch originhad a second effect the checkout never read: it refreshed the workspace clone's remote-tracking refs on every relaunch. The old code gated it onnot is_new_workspace, so only the relaunch path regresses — the container now comes up withorigin/mainas stale as the clone's creation, so in-containergit statusmis-reports "up to date" andgit rebase origin/mainrebases onto old history. Spec-sanctioned, but the sanctioning rationale is narrower than the effect, so the contract is worth restating on the ticket rather than changed here.
Verified met, with the pins that hold them: ensure_repo is clone-if-missing with no auto_fetch (test_ensure_repo_never_fetches_an_existing_clone, plus a signature test); the targeted refspec is unconditional and the ref-missing arm does exactly one extra fetch (test_absent_ref_costs_exactly_one_extra_fetch, call_count == 2); last_fetched stays unwritten on the whole path; the lock pin in test_cold_launch_fetches.py uses a real second fd + flock(LOCK_EX|LOCK_NB), so it reads the production lock, and asserts every locked fetch's refspecs equal the one targeted spec, with wildcards/--tags/--prune separately ruled out foreground-wide; criterion (2) is pinned twice; offline-launches-from-cache is pinned against a real moved remote; lock cycles are byte-identical, and the diff only removes a metadata-lock-under-repo-lock nesting, which is the direction #143 wants. No tag/SHA regression: ensure_branch_exists only ever consulted refs/heads, so the old --tags never fed a launch decision.
Verdict
Request changes (posted as a comment — GitHub refuses --request-changes on a same-account PR; this written verdict is the gate, not the GitHub state).
Blocking:
- Spec 1 — locale-dependent classification silently collapses the three-way outcome the ticket specifically required not be a bool. One-line fix.
- Standards 1 / Spec 2 — the unreachable assertion at
test_repo_manager_real.py:382. Found independently by both axes. Split into twopytest.raisesblocks, and reconcile whichever one holds with theensure_branchdocstring that currently names the wrong site.
Non-blocking, worth doing while in there: Standards 2 (discarded FetchOutcome) and Standards 3 (dead auto_fetch knob). Spec 3 is a note for #150, not a change to this PR.
Two prior Sourcery comments are, on this repo's standards, not actionable: unhandled_fetch_outcome(outcome: NoReturn) is the exact pre-existing idiom at workspace_state.py:122, documented there as the 3.10-compatible stand-in for Never; and collapsing the three dataclasses into a tagged enum would undo the thing #150 explicitly asked for.
The ref-missing arm of fetch_ref matches "couldn't find remote ref" in git's stderr, and git marks that message for translation -- under a non-English locale an ordinary start-a-new-branch launch classified as FetchFailed, collapsing the three-way outcome to two. The fetch subprocess now runs with LC_ALL=C and LANGUAGE=C (both, so the guarantee does not hang on the one glibc rule that lets LC_ALL=C outrank LANGUAGE), with the rest of the environment preserved.
… fetch's outcome The "ref nowhere" integration test held both calls inside one pytest.raises, so ensure_branch raised first and the ensure_workspace line was never reached -- each now sits in its own block, and both hold. ensure_branch's docstring named _prepare_workspace's checkout as where an empty cache is discovered; on the launch path it is ensure_branch's own branch creation, so the docstring (and the unit test echoing it) now say so. Also from the same review, non-blocking: the default-branch fetch_ref discarded its outcome, so a FetchFailed there was silent while the identical condition one arm down warned with its reason. It now warns the same way.
|
Both blocking findings addressed, pushed as Spec 1 — locale-dependent Standards 1 / Spec 2 — unreachable assertion in the "ref nowhere" pin ( Also picked up from the same review, non-blocking Standards 2 ( |
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Re-review after the fix pass. Reviewed at merge-base dcdb077, diff dcdb077...af06dd8; fix commits are c7fe46a and af06dd8. Preflight green: ref resolves, diff non-empty, all checks passing (ci 3.10–3.14, e2e, gate, prek, codecov). Both axes ran as independent agents in fresh context and are not merged or reranked. Both claimed fixes were verified against the code at HEAD, not the commit messages.
Standards
Prior blocking finding (unreachable assertion): FIXED. test_repo_manager_real.py:382-388 now holds each call in its own pytest.raises(RuntimeError), and both halves are genuinely exercisable for distinct reasons: ensure_branch fails in create_local_branch (empty cache, no start point), leaving the cache unchanged; ensure_workspace then clones the empty bare successfully and raises the intended Cannot create branch 'nosuch': neither 'origin/nosuch' nor 'origin/main' at workspace_clone.py:524 — not a repeat of the first failure, not tautological. The docstring correction is accurate: on the launch path ensure_branch runs first and its branch creation is the first cache-contents consultation that raises; the unit-test docstring at test_workspace_clone.py:320-324 matches.
Prior non-blocking finding (discarded base-fetch outcome): FIXED. workspace_clone.py:382-399 captures base_outcome and warns on FetchFailed with the same shape as the neighboring arm (Could not fetch {ref} for {owner}/{repo}: {reason}). The ValueError guard survives intact, and the new behavior-level test (test_workspace_clone.py:264-287, caplog asserts the reason text and that the launch proceeds) pins it properly.
New-code findings, none blocking:
workspace_clone.py:390-399— the innerbase_outcomedispatch names onlyFetchFailed, silently absorbingUpdated,RefMissingOnRemote, and any future fourth arm — two lines below a comment explaining that this file's idiom is "each arm named, so a fourth one cannot be silently read as one of these three." ARefMissingOnRemoteon the recorded default branch is arguably also worth a warning. Note.branch_manager.py:83—"already exists" in e.stderris the same translated-stderr hazardc7fe46ajust fixed, at the repo's other textual-classification site; under a non-English locale a benign already-exists becomes aRuntimeError. Pre-existing and outside this diff — suggest a follow-up ticket alongside theauto_fetchone. Non-blocking.test_worktree_repo_manager.py:428-459— the locale-pin test asserts the env handed to a mockedsubprocess.run; implementation-level, but the behavioral version needs git translations installed in CI, so acceptable. The name..._survives_a_translated_gitslightly overstates what is exercised. TheSSH_AUTH_SOCK-preserved assertion is a good touch. Note.- Minor:
repo_manager.py:316builds the env with{**os.environ, ...}whereworkspace_clone.py:477usesos.environ.copy()— two idioms for one job, not worth a helper at two sites. Note.
Prior judgement-call items (ValueError caught only for the default-branch call, the Updated: pass arm, push=True-only _commit_on) are unchanged and remain accepted. The auto_fetch deferral to its own ticket stands and is not re-raised.
Spec
Spec is #150, implementing the #144 resolution.
Prior Spec 1 (locale-dependent classification): FIXED — HOLDS. Spec: "Model the outcome as the three-way result from #144 (updated / ref-missing / network-failed …), not a bool." fetch_ref (repo_manager.py:304-317) now runs the fetch with env={**os.environ, "LC_ALL": "C", "LANGUAGE": "C"}. Coverage is complete: the "couldn't find remote ref" match at :321 is the only textual stderr classification in the codebase, and the default-branch fetch in ensure_branch goes through this same method, so both classifying fetches get the pin. Env construction preserves the rest of the environment (GIT_*/SSH vars — asserted in the new test). The both-variables approach is sound: glibc gettext consults LANGUAGE only when the locale is not C/POSIX, so LC_ALL=C alone suffices there, but pinning LANGUAGE too removes dependence on that one rule and covers non-glibc gettext. The new test fails if the env pin is removed.
Prior Spec 2 (ref-nowhere pin weaker than it reads): FIXED — HOLDS. Spec: "staleness contract integration: … ref nowhere → existing error." Each call now sits in its own pytest.raises, both raise the existing errors for the spec's reason (details under Standards), and no state leaks between the blocks — the failed first call creates no refs. The corrected ensure_branch docstring now names the actual discovery site on the launch path.
No regression of the previously verified criteria. The fix commits touch only the env kwarg, the base_outcome warning, docstrings, and tests. ensure_repo stays pure clone-if-missing, the targeted refspec and exactly-one-extra-fetch pins stand, last_fetched stays unwritten, the lock pin and re-registration zero-fetch pins are untouched, offline-from-cache still passes. 95 unit + 15 integration tests pass locally.
New fix-pass code against the spec: the now-warned default-branch FetchFailed arm fits the three-way model — create-from-default is licensed only by RefMissingOnRemote on the requested ref; the base fetch failing is network-failed → warn-and-use-cache-or-error, matching the README's documented "Offline: a warning, and the launch proceeds from whatever the cache holds."
Prior Spec 3 (stale origin/* on relaunch) remains a note for #150, not a change to this PR; the fix pass did not touch that path.
Verdict
Approve (posted as a comment — GitHub refuses --approve on a same-account PR; this written verdict is the gate, not the GitHub state).
Both prior blocking findings are verified fixed at HEAD, the non-blocking Standards 2 was fixed as claimed, the auto_fetch deferral is accepted, and the fix pass introduced no blocking problems. The four new items above are notes for follow-up, none of which should hold this PR. Not merging — merge stays human.
1 participant
Closes #150. Implements the devlaunch#144 resolution now that the detached updater sweep (#149) has landed.
The defect
The cold launch path made an unbounded network call while holding the per-repo flock. Once the fetch interval had elapsed,
ensure_reposwept every head and tag (+refs/heads/*:refs/heads/* --tags --prune) before the launch could proceed — so whichever launch drew the short straw paid for everyone's freshness, and every concurrent launch of the same repo queued behind it. The workspace clone then made a second fetch whose output nothing read.The change
ensure_repobecomes the pure clone-if-missing primitive. Itsauto_fetchparameter goes with the fetch it gated (production never passedFalse).ensure_branchfetches one targeted refspec, unconditionally. The outcome is a three-way sum —Updated/RefMissingOnRemote/FetchFailed— following theUnsavedidiom inworkspace_state.py, complete with anunhandled_fetch_outcomearm-checker. The distinction is load-bearing: only "the remote says the ref is not there" licenses creating the branch from the default branch, and only "the remote did not answer" means fall back to cache. A bool would have collapsed them.last_fetched, keeping the sweep's bookkeeping — and one repo→metadata lock nesting — off this path._prepare_workspace's fetch is deleted. The checkout beside it is a plaingit checkout <branch>that never consulted a remote-tracking ref.Lock cycles are deliberately untouched — #143 owns that seam.
The staleness contract
Push upstream, immediately
dlthe branch → the workspace starts at the pushed tip. A brand-new branch is cut from the default branch's freshly fetched tip. Offline warns and launches from cache, failing only when there is nothing cached to launch from. Everything else converges via the background sweep, blocking nobody. Documented in the README and inensure_branch's docstring.Tests
Written test-first, and each pin was verified to fail against a reintroduction of the regression rather than merely passing against the fix:
test/test_cold_launch_fetches.py(new) — the headline pin. A fakesubprocess.runrecords every git argv plus whether the repo lock was held at call time, probed by opening the lock file in a second file description and attempting a non-blockingflock(flock conflicts across separate open file descriptions even within one process, so it reads the truth about the real lock). Asserts no wildcard refspec anywhere in the foreground, and that the only fetch ever running under the lock is the single targeted one. 5 of its 6 tests fail when the two deleted fetches are put back.test/integration/test_repo_manager_real.py— the staleness contract over real git against a local file-path remote: commit pushed after the cache was built, branch that reached the remote after the cache, brand-new branch off a moved default branch, nothing-to-launch-from error, and unreachable-remote degradation. The three freshness assertions fail whenfetch_refis stubbed to a no-op.last_fetchedis not advanced, and that an unsafe ref is rejected before reaching argv.test_the_launch_path_keeps_its_untimed_fetch) explicitly deferred its subject to this ticket; it is rewritten, since the foreground fetch it bounded no longer exists.pixi run cigreen on bothdefault(3.14) andpy310: 1464 passed, pylint 10.00/10, exit 0. Built and smoke-installed via./dev.sh.🤖 Generated with Claude Code
Summary by Sourcery
Clarify and enforce the cold launch staleness contract by fetching only the targeted branch into the shared bare repo, leaving broad repo-wide refreshes to the background updater, and removing redundant workspace-level fetches.
New Features:
Enhancements:
Tests: