docs: secondary-repo work-branch materialization [doc-updater] - #3453
Conversation
There was a problem hiding this comment.
No agent-mode design concerns. This is a documentation-only change describing the gateway/spawner mechanism that materializes secondary-repo work branches on their remotes — no agent prompts, pre-fetching, output formats, post-processing, model identifiers, or LLM call paths are touched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Verified — accurate documentation of #3393 slice-7. No blocking issues.
This is a docs-only change (11 additions to docs/architecture/slice-dag.md). I traced every factual claim in the new paragraph to the code it describes and all of them hold:
len(repos) > 1gating —orchestrator/kubernetes_spawner/_spawn.py:255passespush_branches=bool(repos) and len(repos) > 1. Single-repo spawns passFalse. ✅ Matches "Single-repo pipelines passpush_branches=False."create_worktrees(..., push_branches=True)— signature atorchestrator/gateway_client/_worktree.py:13,21; call site at_spawn.py:228. Threaded to the gateway aspush_branch=Truein the request body (_worktree.py:83). ✅- Gateway wiring —
gateway/gateway.py:7883readspush_branchand forwards it per-repo tomanager.create_worktree(..., push_branch=push_branch)at:7942, which calls_materialize_work_branch_on_remote(gateway/worktree_manager/_create.py:258,508). ✅ Matches "wired through the gateway's_materialize_work_branch_on_remote." - Refspec —
_create.py:602:target = (assigned_branch or branch_name).removeprefix("origin/"),refspec = "HEAD:refs/heads/{target}". ✅ Matches "the fresh worktree HEAD is pushed torefs/heads/{assigned_branch or work-branch}." - Best-effort, non-forced, idempotent — the push is non-
--force;up-to-date/non-fast-forward/! [rejected]stderr is classified asalready_materializedand swallowed atinfo; other failures (auth/network) swallowed atwarning; the outertry/exceptguarantees worktree creation never fails on the push (_create.py:566-660). ✅
The paragraph is placed correctly — directly after the "Per-repo work branch & context PR" section, which describes work branches as lazy-per-repo, so explaining the remote-materialization mechanism here reads naturally and contradicts nothing adjacent.
Minor observations (non-blocking, no change requested):
- The push actually runs for every repo in the list including the primary; on the primary it's a no-op (already materialized by the orchestrator's existing push path). The doc's "each repo's work branch" is technically accurate, and the idempotency guarantee is what keeps the primary's contract-init commit safe. Fine as written for an architecture doc.
- "an already-materialized branch is a no-op" compresses the reality that a divergent-but-present branch also gets treated as materialized (deliberately, to avoid a force push). Acceptable simplification at this altitude.
No security, correctness, or robustness concerns in a documentation change. The description's note that the diff otherwise contains only formatting/test/internal changes with no documented-interface impact matches what's shown (this PR touches only the doc).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address two non-blocking review observations: the push runs for every repo (a no-op on the primary), and a divergent-but-present branch is also treated as already materialized to avoid a force push.
Review feedback addressedThanks for the thorough verification. Both reviews were positive (agent-mode-design APPROVED, review verified accurate, no blocking issues). The review raised two minor observations explicitly marked non-blocking; I folded both into the doc for extra precision rather than leaving them implicit.
Verified against the code:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
Re-reviewed the delta since my last approval. The only change is prose refinement to docs/architecture/slice-dag.md — the paragraph now clarifies that the remote push runs for every repo (a no-op on the primary) and that a divergent-but-present branch is treated as already materialized rather than force-pushed. This folds in the two non-blocking observations from the general review bot.
Still a documentation-only change describing the gateway/spawner mechanism. No agent prompts, pre-fetching, output formats, post-processing, rigid procedures, model identifiers, or LLM call paths are touched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — delta verified accurate. No blocking issues.
Since my last review (84a331f) the only change is one commit (66fc1d1) that folds the two non-blocking observations from the prior review into the docs/architecture/slice-dag.md paragraph. This is still a documentation-only PR (16 additions, one file). I re-traced every factual claim in the revised paragraph to the code and all hold:
len(repos) > 1gating —orchestrator/kubernetes_spawner/_spawn.py:255:push_branches=bool(repos) and len(repos) > 1. ✅ Matches "Single-repo pipelines passpush_branches=False."create_worktrees(..., push_branches=True)threading —orchestrator/gateway_client/_worktree.py:21,83-84setsrequest_data["push_branch"] = True; call site_spawn.py:228. ✅- Runs for every repo including the primary — the gateway loop forwards the flag per-repo:
gateway/gateway.py:7883readspush_branch, and passes it unconditionally into eachmanager.create_worktree(..., push_branch=push_branch)at:7942. So the push is issued for every repo in the list, not just secondaries. ✅ Matches the new "including the primary, where it's a no-op…" wording. - Refspec —
gateway/worktree_manager/_create.py:598-599:target = (assigned_branch or branch_name).removeprefix("origin/"),refspec = "HEAD:refs/heads/{target}". ✅ Matches "pushed torefs/heads/{assigned_branch or work-branch}." - Best-effort, non-forced, idempotent; divergent-but-present treated as materialized —
_create.py:566-660: the push is non---force;up-to-date/up to date/non-fast-forward/fetch first/! [rejected]stderr is classifiedalready_materializedand swallowed atinfo; other failures atwarning; the outertry/exceptguarantees worktree creation never fails on the push. ✅ Matches the new "even one whose tip has diverged … non-fast-forward rejection is swallowed rather than force-pushed, so the primary's contract-init commit is never clobbered."
The revised prose is a strict improvement in precision over the version I first reviewed and introduces no inaccuracy. Placement (immediately after "Per-repo work branch & context PR") remains correct and contradicts nothing adjacent. No security, correctness, or robustness concerns in a docs-only change.
— Authored by egg
|
egg review completed. View run logs 5 previous review(s) hidden. |
Update the Slice-DAG architecture doc's "Per-repo work branch & context PR"
section to describe how a multi-repo pipeline's secondary repos actually get
a work branch on their remote.
gateway/worktree_manager/_create.py's_materialize_work_branch_on_remote: a best-effort, non-forced, idempotentpush of the fresh worktree HEAD to
refs/heads/{assigned_branch or work-branch}, threaded fromorchestrator/kubernetes_spawner/_spawn.pyvia
create_worktrees(..., push_branches=True)wheneverlen(repos) > 1.context/slice PR opener could soft-fail because that repo's
egg/<pipeline_id>/workbranch didn't yet exist on its own remote.PRs was accurate at a high level but didn't explain the mechanism that
makes it true for repos beyond the primary — this PR adds that.
No other docs needed updates: the code changes in this diff are otherwise
formatting-only (shfmt/ruff), test-only, or narrow internal bug fixes
(exact-match cross-repo-hold resolution parsing) with no documented
interface change.
Triggered by: 9daf5cb (merge of #3442, slice-7)
Test Plan
_materialize_work_branch_on_remoteand its call path.Authored-by: egg