[issue-3393][slice-1/6] Repo dimension in the persisted contract... - #3422
Merged
Conversation
Grounded the issue's current-state claims against the live tree (create_worktree list-shape, per-repo credentials, repo-param PR creation, Slice schema gap, repos[0] collapse sites). Four grounding corrections flagged for the planner, acceptance criteria restated, HITL decision registered for v1 merge-sequencing gate semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Take ownership of 3393-analysis-human.md: verified faithful against the refiner's 3393-analysis.md (v1, 107c930) and the contract task description; removed remaining jargon and added the per-repo work-branch/ umbrella-PR point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e NACK) Verified live: _spawn.py:452,464, commit_authorship_store.py:932-933, and routes/pipelines.py:732 (overseer_repo). Added plain-language sweep clause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Correction #1 un-inverted: client method IS create_worktrees (plural, gateway_client/_worktree.py:13); singular create_worktree is the gateway-internal helper (worktree_manager/_create.py:115); repo_volumes is the live spawner param fed from WorktreeResult.worktrees — rewritten as a two-layer naming map. - repos[0] collapse sites enumerated as THREE (adds routes/pipelines.py:732 overseer_repo); sdlc_hitl.py:82 cleared as guarded. Verdict row 6, correction #4, AC-4 updated; human summary 'two spots' -> 'three spots'. - Per-repo conventions entailment added (design recommendation #5 + AC-7): slice agent cwd + CLAUDE.md/linters/check commands of the slice's repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tions - Hard-bit #1 now states the operator's decided model: automated draft-hold auto-readied on upstream merge; HITL only for beyond-merge-state waits (release/publish, version pinning) and genuine development blocks. - Added per-repo house-rules bullet (refiner v2 design rec #5 / AC-7). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Operator resolved HITL cq-1 (merge-sequencing gate) with a two-tier model: automated draft->ready on upstream PR merge for plain merge ordering; HITL-resolved holds for beyond-merge-state conditions (release/publish waits, version pinning, genuine development blocks). Added HITL Resolution section with planner-facing consequences; updated design recommendation #2, hard part #2, AC-6, and the human summary's hard-bit #1. No new HITL decisions induced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reverts 3393-analysis-human.md byte-exact to the simplifier's e88c16d rendering, restoring (1) the per-repo house-rules bullet (design rec #5 / AC-7 rendering) and (2) the simplifier's hard-bit #1, which keeps the cq-1 resolution's development-blocks element. Root cause: my v3 rebase conflict resolution used 'git checkout --theirs', which in a rebase selects the replayed (stale) commit, overwriting the simplifier's concurrent v3. 3393-analysis.md is deliberately untouched (reviewers: ACK-ready). BRC memory updated with the ownership rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Hard-bit #3: owner/repo re-key is decided; same-name rejection ruled out; prohibitive fan-out returns to the operator, never a silent fallback. - New 'Where decisions stand' section: all design questions settled and binding; only a new operator decision reopens them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ngs as binding Design questions section retitled to OPERATOR RULINGS (binding, same standing as cq-1): lazy-per-repo work branches/context PRs, per-slice-repo test-gate/reviewer-diff scoping, primary-repo naming + per-repo status surfaces, and per-repo conventions all RATIFIED (substance unchanged, status upgraded from recommendation). New ruling #6: worktree map MUST be re-keyed by full owner/repo; reject-same-name-sets is forbidden; prohibitive fan-out at plan time is a new HITL, never a silent fallback. Consistency-only knock-ons: correction #2 advice sentence points to the ruling (facts unchanged), AC-2 drops the same-name-rejection alternative, AC-4 requires owner/repo keying. cq-1 fold-in and grounding facts untouched; human summary untouched (simplifier-owned). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g bullet Operator ruling (4) is 'first in list unless explicitly flagged'; the bullet had stated the stricter first-in-list-only rule (reviewer_refine NACK). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t, repos[0] de-collapse, per-repo PR routing, cq-1 merge gate Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
7 risks; verdict PROCEED_WITH_MITIGATIONS (MEDIUM-HIGH). Load-bearing: R1 Contract has no repo dimension (Slice.repo absent=>primary unresolvable), R2 EGG_PIPELINE_REPO is a hard-required overseer consumer (collapse != deletion), R3 cq-1 merge-poll terminal/failure states unspecified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Renders task_planner v1 (d067323): fixed-order six-step chain, the two submission safety checks, the three collapse-site fixes with owner/repo re-keying, per-repo PR routing, the cq-1 two-tier hold, and per-repo gate scoping — with the N=1 no-behavior-change guarantee stated plainly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…k_analyst R1 NACK) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r_plan R3 NACK) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
task-1-1 (shared/egg_contracts/models.py):
- Add Slice.repo (str | None, default None), owner/name-shaped, documented
as "None => primary repo resolved at runtime, not by the model".
- Bump Contract.schemaVersion 1.3->1.4 via _migrate_schema_version_to_1_4,
a pure additive after-stamp mirroring the 1.3 precedent (guarded on
"1.3", idempotent, no field mutation). Slice.repo stays None on a legacy
load; the migration never populates it or references any pipeline repo.
task-1-2 (orchestrator/models.py):
- Add RepoSpec{repo, base_branch} and Pipeline.repos: list[RepoSpec].
- _sync_repos_and_legacy_singleton: synthesize repos from the legacy
singleton when absent, mirror repos[0] back onto repo/base_branch.
- primary_repo property (repos[0].repo) — the intentional named-primary
accessor, not one of the slice-3 collapse sites.
- resolve_slice_repo(slice, pipeline): the runtime home of the
absent-Slice.repo => primary default.
No behavioural change for N=1 pipelines; the three collapse sites are
untouched (removed in slice 3). Nothing assumes len(repos) in {1, 2}.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contract layer (shared/egg_contracts/tests/test_models.py, new):
- Slice.repo exists (str|None, default None) and round-trips in a fresh
1.4 contract.
- Loading a persisted 1.3 contract bumps schemaVersion->1.4 as a pure
additive stamp and leaves every Slice.repo None (no model-filled repo);
an explicitly-set repo is preserved.
- The 1.4 stamp is idempotent + version-exact (guards, no downgrade of a
future 2.0), and the prior migration branches still fire: full-chain
None/1.0/1.1/1.2/1.3 -> 1.4, the wrap-mode pr.context_* strip, and
direct-call coverage of the 1.0->1.1 and 1.3->1.4 after-stamps.
Orchestrator layer (orchestrator/tests/test_models.py):
- RepoSpec{repo, base_branch}; Pipeline.repos synthesizes a one-element
list from a legacy singleton and mirrors repos[0] back onto the legacy
repo/base_branch scalars (idempotent on reload); arbitrary repo count.
- primary_repo == repos[0].repo (and None for a repo-less pipeline).
- resolve_slice_repo: explicit slice.repo wins, else pipeline.primary_repo.
- N=1 back-compat asserted at both layers.
147 tests pass (system pytest against the working tree). make test could
not provision its venv in this sandbox (no network); green is enforced at
the integration gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The coder's slice-1 default bump schemaVersion 1.3->1.4 (additive Slice.repo stamp) made the pre-existing migration assertions in the contract test suite stale (assert '1.4' == '1.3'). Update every stale pin to 1.4, rename the default/latest tests, repurpose test_fresh_1_3_payload -> promotes_to_1_4, and add test_fresh_1_4_payload_loads_unchanged to keep the 'latest loads unchanged' invariant pinned at the true latest. Resolves reviewer_code v1 NACK (9 failing contract tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a.json Addresses tester NACK (gap-1 on task-1-1): commit d779b0c bumped the Pydantic Contract.schemaVersion default 1.3->1.4 but did not mirror it into .egg/schemas/contract.schema.json, breaking the lockstep invariant test test_schema_default_schemaversion_tracks_model. Bump the JSON Schema `schemaVersion.default` 1.3->1.4 in lockstep with the model default, exactly as #3033 did for 1.2->1.3. The lockstep guardrail test is left intact and now passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
james-in-a-box
Bot
force-pushed
the
egg/issue-3393/work
branch
from
July 2, 2026 19:47
b2d5528 to
bb7d0ec
Compare
jwbron
approved these changes
Jul 2, 2026
Contributor
Author
There was a problem hiding this comment.
Contract Verification — PR #3422 (issue-3393, slice-1/6)
Verdict: Approve (post-merge verification record — this PR is already merged into main).
Verified the two-layer repo-dimension implementation against the slice-1 contract tasks. All three tasks and every acceptance criterion are objectively satisfied.
task-1-1 — Contract layer (shared/egg_contracts/models.py) ✅
Slice.repo: str | None = Field(default=None, …)added,owner/name-shaped, documented exactly as "None⇒ resolved to the pipeline's primary repo at RUNTIME (seeresolve_slice_repo), NOT filled in by the model." (models.py:438)Contract.schemaVersiondefault bumped1.3→1.4;_migrate_schema_version_to_1_4is a pure additive after-stamp mirroring_migrate_schema_version_to_1_3verbatim —mode="after", guarded onif self.schemaVersion == "1.3", idempotent, no field mutation. schemaVersion field docstring updated to note the new stamp.- The migration does not populate
Slice.repoand does not reference any pipeline/primary repo; no repo list is added toContract. Confirmed bytest_1_3_load_leaves_every_slice_repo_none. - Four pre-existing migration branches untouched.
TestPriorMigrationBranchesStillFire+TestFullMigrationChainComposesgreen.
task-1-2 — Orchestrator layer (orchestrator/models.py) ✅
RepoSpec{repo, base_branch}added;Pipeline.repos: list[RepoSpec]withdefault_factory=list._sync_repos_and_legacy_singleton(mode="after") synthesizesrepos=[RepoSpec(repo, base_branch)]from the legacy singleton when absent, and mirrorsrepos[0]back ontorepo/base_branch. Repo-less (local-mode) pipelines left untouched.primary_repoproperty returnsrepos[0].repo(with aself.repobelt-and-braces fallback). No primary+secondary shape baked in; nothing assumeslen(repos) ∈ {1, 2}—test_arbitrary_repo_count_supportedconfirms.resolve_slice_repo(slice, pipeline)returnsslice.repo if slice.repo else pipeline.primary_repo— the absent⇒primary default lives here, not in the contract migration.- N=1 behavior unchanged; the three
repos[0]collapse sites are correctly deferred to slice 3 (not in this diff).
task-1-3 — Tests (both layers) ✅
- New
shared/egg_contracts/tests/test_models.py(23 tests):Slice.reporound-trip, additive1.3→1.4stamp leavingrepoNone, idempotency + version-exact guarding (no future-version downgrade), all four prior branches firing. orchestrator/tests/test_models.pyextended:RepoSpec, synth-from-singleton + mirror-back (idempotent), arbitrary repo count,primary_repo == repos[0].repo,resolve_slice_reposet/None, N=1 back-compat.- gap-1 (schema/model lockstep) resolved in commit
797a590:.egg/schemas/contract.schema.jsonschemaVersion.defaultbumped1.3→1.4; the lockstep guardtest_schema_default_schemaversion_tracks_modelpasses.
Test execution (targeted, per review policy — full make test left to CI)
shared/egg_contracts/tests/test_models.py: 23 passedorchestrator/tests/test_models.py: 124 passedtests/shared/egg_contracts/test_models.py+test_pr_metadata.py(stale-pin realignment): 113 passed- schema/model lockstep guard: passed
Notes for human reviewer
- The contract's top-level
acceptance_criterialist is empty and per-task criteria are free-text (noac-NIDs), so there are no criterion IDs to mark viaverify-criterion; the orchestrator endpoint was also unreachable during this run. Verification was performed directly against the diff and by running the targeted suites. - The two large
.egg-state/brc-history/3393-implement-slice-1.{json,md}files are pipeline-state persistence (commit "Persist BRC history for slice-1 (#2548)"), not orphaned product code — expected machinery, not a contract violation.
No contract violations found. Implementation matches the ratified two-layer design (risk_analyst R1 / architect aeb3528).
— Authored by egg
Contributor
Author
|
egg contract-verification completed. View run logs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the load-bearing repo dimension across the two model layers, aligned to the ratified architect design. shared/egg_contracts/models.py (contract layer): add
Slice.repo: str | None = None(owner/name; None ⇒ resolved to primary at runtime, NOT in the model) and bumpContract.schemaVersion1.3→1.4 as a PURE ADDITIVE after-stamp_migrate_schema_version_to_1_4mirroring_migrate_schema_version_to_1_3verbatim (guard schemaVersion=="1.3"; idempotent; NO field mutation —Slice.repostays None on legacy load). The Contract model carries NO repo field and cannot see the pipeline, so absent⇒primary CANNOT be a model migration. orchestrator/models.py (orchestrator layer): addRepoSpec{repo, base_branch}andPipeline.repos: list[RepoSpec]with a model validator that synthesizesrepos=[RepoSpec(repo, base_branch)]from the legacy singleton when absent (and mirrorsrepos[0]back ontorepo/base_branchfor legacy readers), aprimary_repoproperty, and a RUNTIME resolverresolve_slice_repo(slice, pipeline) -> slice.repo or pipeline.primary_repo. N=1 pipelines round-trip / behave identically. Chain root: nothing writesSlice.repountil it lands.Base PR: #3418
What's in this PR
Commits (5):
This slice
Repo dimension in the persisted contract & pipeline schema (migration)
Files affected:
shared/egg_contracts/models.pyorchestrator/models.pyshared/egg_contracts/tests/test_models.pyorchestrator/tests/test_models.pyTasks (3) + acceptance criteria
repo: str | None = Noneto theSlicemodel, owner/name-shaped, documented as "exactly one repo per slice; None ⇒ resolved to the pipeline's primary repo at RUNTIME (see resolve_slice_repo), NOT filled by the model." (b) BumpContract.schemaVersion"1.3"→"1.4" by adding_migrate_schema_version_to_1_4as a PURE ADDITIVE after-stamp that mirrors_migrate_schema_version_to_1_3(models.py:1049-1069) VERBATIM: guardif self.schemaVersion == "1.3": self.schemaVersion = "1.4", idempotent, NO field mutation —Slice.repostays None on a legacy load. Update the schemaVersion field docstring (:844-862) to note the new additive stamp. CRITICAL (per risk_analyst R1 / architect aeb3528): the Contract model has NO repo field and cannot see the orchestrator Pipeline, so the validator MUST NOT try to populateSlice.repoand MUST NOT reference any pipeline repo. The absent⇒primary default and the pipeline repo LIST are BOTH orchestrator concerns (TASK-1-2), NOT the contract. Do NOT persist a repo list onto the Contract — the repo list lives only on the orchestrator Pipeline. Preserve every existing field and migration branch byte-for-byte.Slice.repoexists (str | None, default None), documented as "None ⇒ primary repo resolved at runtime, not by the model". -Contract.schemaVersionis "1.4";_migrate_schema_version_to_1_4is a pure additive stamp mirroring the 1.3 precedent (guarded on "1.3", idempotent, no field mutation);Slice.repostays None when a 1.3 contract is loaded. - The contract migration does NOT populateSlice.repoand does NOT reference any pipeline/primary repo; no repo LIST is added to the Contract. - The four existing migration branches are untouched; N=1 contracts serialize/deserialize with no observable change.RepoSpecmodel carryingrepo: str(owner/name) andbase_branch: str | None. (b) addPipeline.repos: list[RepoSpec](default_factory=list) and a model validator that, whenreposis absent but the legacy singletonPipeline.repo(models.py:1131) is set, SYNTHESIZESrepos=[RepoSpec(repo=repo, base_branch=base_branch)](back-compat), and mirrorsrepos[0]back onto the legacyrepo/base_branchscalars so legacy readers keep working until slice 3 rewires them. (c) add aprimary_repoproperty returningrepos[0].repo— the INTENTIONAL primary accessor for naming/defaulting, explicitly NOT one of the threerepos[0]collapse sites removed in slice 3 (those collapse the agent-facing repo set to a single repo; this exposes a named primary while keeping the full list available). (d) add the RUNTIME resolverresolve_slice_repo(slice, pipeline) -> slice.repo if slice.repo else pipeline.primary_repo— this is where absent-Slice.repo⇒primary lives (it takes the pipeline as a second input, which the contract model cannot). Nothing may assumelen(repos)∈ {1, 2}.RepoSpec{repo, base_branch}andPipeline.repos: list[RepoSpec]exist; the validator synthesizesreposfrom a legacy singleton and mirrorsrepos[0]back ontorepo/base_branch. -primary_reporeturnsrepos[0].repo; no two-repo or primary+secondary shape is baked in; nothing assumes len(repos)∈{1,2}. -resolve_slice_repo(slice, pipeline)returnsslice.repowhen set elsepipeline.primary_repo— the absent⇒primary default lives HERE, not in the contract migration. - No behavioral change for N=1 pipelines; the three collapse sites are untouched (removed in slice 3).Slice.repo; (b) loading a persisted 1.3 contract leaves each slice'srepoas None and bumps schemaVersion to "1.4" (pure additive stamp — assert NO repo is filled by the model); (c) the 1.4 stamp is idempotent and the four pre-existing migration branches still fire for their versions (no regression). In orchestrator/tests (ORCHESTRATOR layer — nearest existing pipeline-model test module): (d)resolve_slice_repo(slice, pipeline)returnsslice.repowhen set andpipeline.primary_repowhenslice.repois None; (e) a legacy singletonPipeline.reposynthesizes a one-elementreposlist and mirrors back ontorepo/base_branch; (f)primary_repo==repos[0].repo; (g) N=1 behavior is identical.Slice.repostaying None on legacy load (no model-filled repo), idempotency, and the four prior branches intact. - Orchestrator-layer tests assertresolve_slice_repo(set⇒slice.repo, None⇒primary), legacy singleton⇒one-elementrepos+ mirror-back, andprimary_repo==repos[0].repo. - N=1 back-compat asserted at both layers. -make test(narrowed) andmake test-allgreen.Stack
issue-3393egg/issue-3393/work