Skip to content

docs: document cross-repo merge gate [doc-updater] - #3451

Merged
jwbron merged 7 commits into
mainfrom
egg/doc-update-cross-repo-merge-gate
Jul 3, 2026
Merged

docs: document cross-repo merge gate [doc-updater]#3451
jwbron merged 7 commits into
mainfrom
egg/doc-update-cross-repo-merge-gate

Conversation

@james-in-a-box

@james-in-a-box james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Update documentation to reflect the cross-repo merge-sequencing gate added in
#3393 slice-5 (orchestrator/cross_repo_merge_gate.py, two new gateway
routes, and a new env var):

  • docs/architecture/slice-dag.md — folds the cross-repo merge-gate details
    (orchestrator-only gateway routes, the EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS
    knob, the [hold:beyond-merge-state] Tier-B marker, and the release/keep-held
    HITL options) into main's ### Cross-repo merge-sequencing hold (two-tier)
    subsection, and repoints the Configuration-knobs table row to the surviving anchor.
  • docs/reference/orchestrator-cli.md and docs/architecture/orchestrator.md
    — add EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS to the env-var tables.
  • docs/development/STRUCTURE.md — list the new
    orchestrator/cross_repo_merge_gate.py module.
  • gateway/README.md — document the two new orchestrator-only routes,
    POST /api/v1/gh/pr/merge_state and POST /api/v1/gh/pr/ready.

Triggered by: 4eaa3ce (merge of #3430, egg/issue-3393/slice-5)

Scope: docs-only

The diff is purely documentation — 5 files (docs/architecture/orchestrator.md,
docs/architecture/slice-dag.md, docs/development/STRUCTURE.md,
docs/reference/orchestrator-cli.md, gateway/README.md), no code paths
affected.

The ruff-format churn, the orchestrator/models.py file-size-allowlist entry,
and the tests/shared/egg_contracts/test_pr_metadata.py allowlist reorder that
this branch briefly carried through earlier main merges have all since been
deduped — every one of those files is now byte-identical to main and no
longer appears in this PR's diff.

Test Plan

  • Automated: none for the docs; CI runs the full lint/format/test suite on PR
    HEAD.
  • Manual: reviewed each doc edit against the corresponding source diff
    (gateway/gateway.py, orchestrator/cross_repo_merge_gate.py,
    orchestrator/env_config.py, orchestrator/routes/pipelines.py) to confirm
    accuracy.

Authored-by: egg

@james-in-a-box

This comment has been minimized.

…conflict in docs/architecture/slice-dag.md

main's #3437 added a broader '## Per-slice repo (multi-repo pipelines)'
section whose '### Cross-repo merge-sequencing hold (two-tier)' subsection
overlaps this PR's standalone '## Cross-repo merge-sequencing gate' section.
Kept main's better-integrated section and folded in this PR's unique details
(gateway routes POST /api/v1/gh/pr/merge_state and /pr/ready, Gateway README
link, EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS attempt bound, the
[hold:beyond-merge-state] marker, and the two operator-selectable HITL options).
Repointed the Configuration-knobs table link to the surviving anchor.
@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Resolved merge conflicts with main (merge commit, history preserved).

File Category Resolution
docs/architecture/slice-dag.md Semantic (overlapping docs) Reconciled — see below

Only one file conflicted. The PR's other edits (gateway/README.md, docs/architecture/orchestrator.md, docs/reference/orchestrator-cli.md, docs/development/STRUCTURE.md) were non-conflicting and are preserved intact; orchestrator-cli.md auto-merged cleanly with the PR's EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS row intact.

docs/architecture/slice-dag.md

Both sides documented the same feature (the #3393 cross-repo merge-sequencing gate):

  • This PR added a standalone ## Cross-repo merge-sequencing gate section.
  • main (via [issue-3393][slice-6/7] Per-repo test-gate + reviewer-diff scoping... #3437, slice-6) had since added a broader ## Per-slice repo (multi-repo pipelines) section whose ### Cross-repo merge-sequencing hold (two-tier) subsection covers the same two-tier hold, integrated with the surrounding multi-repo context.

Rather than duplicate the two-tier description, I kept main's better-integrated section and folded this PR's unique technical details into main's subsection, so nothing from either side is lost:

  • The orchestrator-only gateway routes POST /api/v1/gh/pr/merge_state (read) and POST /api/v1/gh/pr/ready (write), with the Gateway README link.
  • The attempt-bound env var EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS (default 240 ticks, ~2h at the 30s cadence).
  • The [hold:beyond-merge-state] opt-in marker syntax for Tier B.
  • The two operator-selectable HITL options (release / keep-held).

I also repointed the Configuration-knobs table link from the now-removed #cross-repo-merge-sequencing-gate anchor to the surviving #cross-repo-merge-sequencing-hold-two-tier anchor.

Please review: the reconciled ### Cross-repo merge-sequencing hold (two-tier) subsection — confirm the folded-in route names, env-var default, and marker syntax read accurately against main's phrasing. The overlap was documentation prose describing the same behavior, so the merge is additive rather than a behavioral choice.

Verification: No conflict markers remain; the config-knobs anchor link resolves to the surviving heading. make lint-custom surfaces only a pre-existing file-sizes failure on orchestrator/models.py (byte-identical to main, untouched by this PR) — no docs/markdown files are flagged, so this docs-only merge introduces no lint regressions. CI will run the full suite.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns.

This is a documentation-only change, and the design it documents aligns well with agent-mode principles: the cross-repo merge gate enforces sequencing through gateway routes (/api/v1/gh/pr/merge_state, /api/v1/gh/pr/ready) and routes its failure terminals through the HITL decision queue — sidecar/infrastructure enforcement rather than prompt-level instructions. No pre-fetching, human-facing structured output, post-processing pipelines, or pinned model identifiers are introduced.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author
Autofix tracking
{"Lint/Python": 1, "Lint/Custom Checks": 1, "Test/Unit Tests": 1}

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: docs for cross-repo merge gate (#3393 slice-5)

Documentation-only PR (5 files, +41/-12). I verified every technical claim against the source it describes — orchestrator/cross_repo_merge_gate.py, orchestrator/env_config.py, orchestrator/routes/pipelines.py, and gateway/gateway.py. Everything is accurate.

Verified accurate

  • EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS default 240 / "~2h at 30s cadence" — matches DEFAULT_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS = 240 (env_config.py:287) and DEFAULT_STACKED_PR_RECONCILER_INTERVAL_SECONDS = 30.0; 240 × 30s = 7200s = 2h. ✓
  • "rides the stacked-PR reconciler cadence" — confirmed: _poll_cross_repo_merge_gate is driven inside _start_stacked_pr_reconciler on the shared tick (routes/pipelines.py:18824-19004), no new scheduler. ✓
  • Merge detection keys off mergedAt/state, not head-SHA_is_merged (cross_repo_merge_gate.py) checks merged_at/mergedAt/merged/state == MERGED; the gateway route requests --json state,mergedAt (gateway.py:4581). ✓
  • Two Tier-A failure terminals (CLOSED-not-merged, attempt-bound timeout)_poll_one_gate registers closed_unmerged via _is_closed_unmerged, and timeout when prog.attempts > max_attempts. ✓
  • Tier-B opt-in via [hold:beyond-merge-state] marker in goal or a task description — matches BEYOND_MERGE_STATE_MARKER and classify_hold_kind, which scans goal then each task.description. ✓
  • HITL decision offers two options — release (marks ready) / keep-held (terminal, stays draft) — matches _CROSS_REPO_HOLD_RELEASE_OPTION_* / _CROSS_REPO_HOLD_KEEP_OPTION_* and the RELEASE/KEEP handling in _poll_one_gate and _cross_repo_hold_resolution (routes/pipelines.py:12778-12845). The fail-safe (ambiguous resolution → KEEP, never auto-ready) is real. ✓
  • Gateway routesPOST /api/v1/gh/pr/merge_state (read {state, mergedAt}) and POST /api/v1/gh/pr/ready (draft→ready, wraps gh pr ready) both exist at gateway.py:4535 / 4631, both decorated @require_launcher_auth, both taking {repo, pr_number}. The documented Auth: Bearer {launcher_secret} (vs. the Policy: line on session-auth routes) is the correct distinction — these are control-plane/orchestrator-only. ✓
  • STRUCTURE.md module entryorchestrator/cross_repo_merge_gate.py exists and the one-line description matches its docstring. ✓
  • Anchor links#cross-repo-merge-sequencing-hold-two-tier resolves to the ### Cross-repo merge-sequencing hold (two-tier) heading (slice-dag.md:795); cross-file links to gateway README and orchestrator-cli/slice-dag are valid. ✓

Non-blocking (optional)

  • gateway/README.md: the two new routes sit inside the "GitHub Operations" block among Policy:-annotated session-auth routes. The Auth:-line + "Orchestrator-only" wording already disambiguates, but if you want them visually grouped with the other launcher-auth control-plane routes (e.g. find_open_pr / list_open_prs), consider co-locating them. Cosmetic only.

No blocking issues. Docs faithfully describe the implemented behavior.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns (re-review).

Since my prior review at 9cbed6d, the delta is a merge with main (reconciling the overlapping slice-dag.md cross-repo-hold section) plus automated formatting reflows across config/, gateway/, and orchestrator/. None of it touches agent-mode surface:

  • The documented cross-repo merge gate still enforces sequencing through gateway routes (/api/v1/gh/pr/merge_state, /api/v1/gh/pr/ready, orchestrator/launcher-auth only) and routes both failure terminals and the Tier-B opt-in hold through the HITL decision queue — sidecar/control-plane enforcement rather than prompt-level instructions.
  • No pre-fetching, human-facing structured output, post-processing pipelines, rigid procedures, prompt-level security, direct Anthropic API calls, or pinned model identifiers introduced in the delta.

The earlier non-blocking suggestion (co-locating the two new launcher-auth gateway routes with the other control-plane routes in the README) was cosmetic and doesn't affect agent-mode alignment.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns (re-review).

Since my prior review at beb459df, the delta is lint/autofix commits plus a file-size allowlist entry for orchestrator/models.py (#3450). None of it touches the agent-mode surface.

The documented design remains well-aligned:

  • The cross-repo merge gate enforces sequencing through gateway routes (/api/v1/gh/pr/merge_state, /api/v1/gh/pr/ready) that are launcher-auth / orchestrator-only control-plane — sidecar enforcement, not prompt-level instructions.
  • Both Tier-A failure terminals (CLOSED-not-merged, attempt-bound timeout) and the Tier-B opt-in hold route through the HITL decision queue with a single release path.
  • No pre-fetching, human-facing structured output, post-processing pipelines, rigid procedures, prompt-level security, direct Anthropic API calls, or pinned model identifiers in the delta.

Note (outside agent-mode scope, deferring to the base review bot / CI): the branch appears stale relative to main — the diff reverts several of main's line-length reformats and, in gateway/gateway.py and orchestrator/routes/pipelines.py, re-introduces except ValueError, TypeError: (invalid Python 3 syntax). That's a correctness issue for the base bot, not an agent-mode concern, but worth a rebase on main before merge.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 8c896170 (delta since 9cbed6d)

The only new commit since my approving review is 8c896170 "Fix checks: allowlist orchestrator/models.py". I re-reviewed its full delta. No blocking issues — but two things need correcting for the record, and one PR-hygiene item is worth fixing before merge.

The delta breaks down as

  1. scripts/file-size-allowlist.yaml — adds orchestrator/models.py: {issue: "3450"}. This is the commit's stated purpose (unblocks the file-size check that models.py trips on main). Legitimate.
  2. Formatting-only churn across config/repo_config.py, gateway/gateway.py, gateway/tests/test_repo_visibility.py, orchestrator/mcp_tools/_submit.py, orchestrator/routes/pipelines.py, orchestrator/tests/test_kubernetes_spawner.py — line-length reflows, implicit-string-concat collapses, and except (A, B):except A, B:.

Correcting the record on the "invalid Python 3 syntax" concern

A prior agent-mode note flagged except ValueError, TypeError: (in gateway.py and pipelines.py) as "invalid Python 3 syntax" and recommended a rebase. That is a false positive. This repo sets requires-python = ">=3.14" / target-version = "py314" (pyproject.toml), and Python 3.14 (PEP 758) permits unparenthesized multiple exception types. I confirmed:

  • ast.parse accepts it and produces an AST byte-identical to the parenthesized form (ExceptHandler(type=Tuple(...))).
  • Runtime catches both types.

So these are semantically identical, not syntax errors.

The churn is ruff-canonical — CI will pass, and main is the stale one

I verified with the pinned toolchain (ruff>=0.15.12,<0.16; system ruff 0.15.20):

  • ruff format --check reports all four changed production files "already formatted" at this PR head.
  • The main versions of repo_config.py, gateway.py, and pipelines.py all report "Would reformat".

So the PR is advancing these files to canonical ruff format, not reverting main's. The format check on this PR's changed files will pass. I also confirmed the three collapsed implicit-string-concats produce byte-identical strings (no dropped/added spaces), and every other hunk is pure whitespace/wrapping. Net semantic change of the churn: zero.

Docs (this PR's actual purpose) — unchanged and still accurate

No docs/* or gateway/README.md file changed in this delta, so my prior verification stands: env-var default 240, both gateway routes (/api/v1/gh/pr/merge_state, /api/v1/gh/pr/ready), and the cross_repo_merge_gate.py module all match source.

Non-blocking — fix before merge

The PR description is now inaccurate. It states "documentation-only change, no code paths affected" and "Automated: none — documentation-only change, no code paths affected." The PR in fact modifies four production files (config/repo_config.py, gateway/gateway.py, orchestrator/mcp_tools/_submit.py, orchestrator/routes/pipelines.py) plus two test files and the file-size allowlist. The changes are provably inert (format-only + a CI allowlist entry), so the risk is nil and I'm not blocking — but the merger is being told there's no code to look at when there is. Please either:

  • update the description/test-plan to note the ruff-format collateral + the #3450 allowlist entry, or
  • split the unrelated formatting churn and the allowlist entry out of a docs PR.

Approving: the delta is merge-safe and CI-clean; the docs are accurate.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Feedback disposition

Thanks for the thorough re-reviews. All reviews are approving; the two non-blocking items:

  1. PR description inaccurate — "documentation-only, no code paths affected" while the branch carries format churn + the #3450 allowlist entry (review bot @ 8c896170) — fixed (PR description edit).
    The reviewer offered two options: update the description, or split the churn out. I took the update path. The collateral originated from a merge with main plus egg's own automated lint/format autofix, so splitting it back out would risk reintroducing the very format-check failures the autofix resolved — whereas the churn is provably inert (except (A, B):except A, B: is byte-identical AST under this repo's requires-python >= 3.14 / PEP 758, and the implicit-string-concat collapses are byte-identical strings). The rewritten description now has a dedicated "Non-doc collateral (provably inert)" section listing the six format-only files and the orchestrator/models.py allowlist entry, and the Test Plan no longer claims "no code paths affected." No commit — this is a GitHub description change, not a working-tree change.

  2. Gateway README: co-locate the two new launcher-auth routes with find_open_pr / list_open_prs (prior review, "Cosmetic only" / "optional") — disagree (subjective grouping preference, no technical impact).
    POST /api/v1/gh/pr/merge_state and POST /api/v1/gh/pr/ready currently sit in the GitHub Operations block alongside the other pr/* routes (pr/create, pr/comment, pr/edit, pr/close), which is a defensible grouping — they are PR operations. Moving them out to a separate control-plane block would separate them from their functional siblings. As the reviewer noted, the Auth: Bearer {launcher_secret} line plus the "Orchestrator-only" wording already disambiguate the trust boundary, so there's no correctness or clarity gap to close. Happy to move them if you'd prefer the auth-boundary grouping over the functional one — just say the word.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

- Rename local var context_title -> secondary_pr_title in
  _maybe_open_secondary_context_prs (it reads contract.pr.title, not the
  removed PRMetadata.context_title field; the grep test flagged the name).
- Allow-list shared/egg_contracts/tests/test_models.py in
  TestNoSurvivingReadSites: its migration test constructs a legacy payload
  with the removed keys to assert they are stripped on load.
@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

No action needed — this branch is already conflict-free and up to date with main.

Investigation:

  • The current main tip (9daf5cbfe) is already an ancestor of this PR's HEAD (cbaaceca9).
  • git merge --no-commit origin/main reports "Already up to date." — no merge is even started, so there is nothing to resolve.
  • GitHub reports the PR as mergeable: MERGEABLE.

The earlier conflicts were already resolved by the existing merge commit on this branch (cbaacec — "Merge main into doc-update-cross-repo-merge-gate; dedupe overlapping CI fixes"). Since then, main has not advanced past what this branch already contains.

The mergeStateStatus: BLOCKED on this PR is driven by required checks / review status, not by merge conflicts.

No merge commit was created and nothing was pushed, since the working tree and branch are unchanged.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns (re-review).

Since my prior review at 8c896170, the PR-authored delta is limited to two CI-check fixes plus a merge with main:

  • orchestrator/routes/pipelines.py: local variable rename context_titlesecondary_pr_title in _open_secondary_context_prs (it reads contract.pr.title, not the removed PRMetadata.context_title field — the rename satisfies the surviving-field grep test).
  • tests/shared/egg_contracts/test_pr_metadata.py: allow-lists shared/egg_contracts/tests/test_models.py in TestNoSurvivingReadSites because that migration test intentionally constructs a legacy payload with the removed keys to assert they're stripped on load.

Neither touches the agent-mode surface. The documented design remains well-aligned: the cross-repo merge gate enforces sequencing through gateway routes (/api/v1/gh/pr/merge_state, /api/v1/gh/pr/ready, launcher-auth / orchestrator-only control-plane) and routes both failure terminals and the Tier-B opt-in hold through the HITL decision queue — sidecar/control-plane enforcement rather than prompt-level instructions. No pre-fetching, human-facing structured output, post-processing pipelines, rigid procedures, prompt-level security, direct Anthropic API calls, or pinned model identifiers in the delta.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at cbaaceca (delta since 8c896170)

The only new commit since my prior review is the merge cbaaceca "Merge main into doc-update-cross-repo-merge-gate; dedupe overlapping CI fixes". I re-reviewed the full delta and re-verified every technical claim against source. No blocking issues.

The merge resolved my prior non-blocking concern

At 8c896170 I flagged that the PR carried non-doc collateral (ruff-format churn across config/repo_config.py, gateway/gateway.py, orchestrator/mcp_tools/_submit.py, orchestrator/routes/pipelines.py, two test files, plus the orchestrator/models.py file-size-allowlist entry) while the description claimed "documentation-only". This merge dedupes all of it — those files are now byte-identical to main and no longer appear in the PR-vs-main diff. The PR is now genuinely close to docs-only.

Current PR content vs main (9daf5cb) — 6 files, clean

Verified via compare/9daf5cb...cbaaceca (ahead 6, behind 0):

  • docs/architecture/slice-dag.md (+22/-12) — the reconciled two-tier section
  • docs/architecture/orchestrator.md, docs/reference/orchestrator-cli.md, docs/development/STRUCTURE.md, gateway/README.md — the env-var / route / module doc rows
  • tests/shared/egg_contracts/test_pr_metadata.py (+4/-4) — inert: relocates the existing shared/egg_contracts/tests/test_models.py entry within ALLOWED_PATHS and rewords its comment. The tuple is consumed as an order-independent substring membership set (any(allowed in path for allowed in self.ALLOWED_PATHS), line 615) and the entry appears exactly once, so behavior is unchanged.

Technical claims re-verified against source (all accurate)

  • EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS default 240 (env_config.py:287); reconciler cadence 30.0s (env_config.py:282); 240 × 30s = 7200s = ~2h ✓
  • Gateway routes POST /api/v1/gh/pr/merge_state (gateway.py:4535) and POST /api/v1/gh/pr/ready (gateway.py:4631), both @require_launcher_auth — matches the documented Auth: Bearer {launcher_secret} / "Orchestrator-only" wording ✓
  • mark_pr_ready(repo, pr_number) verb exists (gateway_client/_pr.py:727, wraps gh pr ready) ✓
  • Tier-B opt-in via [hold:beyond-merge-state] marker scanned in goal then each task.description (cross_repo_merge_gate.py:59,161-164) ✓
  • Two HITL options: "Release the hold and mark the PR ready" / "Keep the PR held for manual handling" (routes/pipelines.py:12783-12785); keep-held is terminal / stays draft ✓
  • orchestrator/cross_repo_merge_gate.py exists; STRUCTURE.md one-liner matches the module docstring ✓
  • Anchor #cross-repo-merge-sequencing-hold-two-tier resolves to ### Cross-repo merge-sequencing hold (two-tier) (slice-dag.md:795); the config-knobs-table link (slice-dag.md:923) points to the same surviving anchor ✓
  • Reconciled slice-dag.md section reads coherently — no conflict markers, no duplicated prose, main's better-integrated multi-repo framing preserved with this PR's route/env-var/marker details folded in ✓

Non-blocking

The test_pr_metadata.py reorder is unrelated collateral pulled in by the merge's conflict resolution. It's provably inert, so I'm not blocking — but strictly it's still a non-doc change in a docs PR. Fine to leave; noting for the record.

Approving: the delta is merge-safe, the docs faithfully describe the implemented behavior, and my earlier collateral concern is resolved.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Feedback disposition

All formal reviews are approving (agent-mode-design: approve; review bot: approve-with-suggestions). Addressing the outstanding non-blocking items, and correcting the PR description now that the cbaacec merge changed what the branch carries.

  1. PR description no longer matches the diff (review bot @ 8c896170, item Phases 1-2: Repository setup, docs, and gateway extraction (partial) #1 — "the merger is being told about content that doesn't match the PR") — fixed (PR description edit).
    My earlier disposition updated the description to add a "Non-doc collateral" section listing the ruff-format churn (config/repo_config.py, gateway/gateway.py, mcp_tools/_submit.py, routes/pipelines.py, two test files) and the orchestrator/models.py allowlist entry. The subsequent cbaacec merge with main then deduped every one of those files — they're now byte-identical to main and absent from the diff — so the description had become inaccurate in the opposite direction (listing gone files, omitting the one that remains). I rewrote the section to state the churn was deduped and to name the single remaining non-doc file, tests/shared/egg_contracts/test_pr_metadata.py. The diff is now genuinely 5 docs + 1 inert test-allowlist reorder. No commit — GitHub description change only.

  2. tests/shared/egg_contracts/test_pr_metadata.py reorder is a non-doc change in a docs PR (review bot @ cbaaceca, non-blocking, "Fine to leave; noting for the record") — disagree (reviewer explicitly said leave it; the change is provably inert).
    The hunk relocates the existing shared/egg_contracts/tests/test_models.py entry within TestNoSurvivingReadSites.ALLOWED_PATHS and rewords its comment. As you verified, the tuple is consumed as an order-independent substring-membership set and the entry appears exactly once, so behavior is unchanged — it's a legitimate CI-fix pulled in via the main merge, and reverting it would risk re-tripping the check it satisfies. Now documented explicitly in the description rather than silently carried.

  3. Gateway README: co-locate the two new launcher-auth routes with find_open_pr / list_open_prs (review @ 9cbed6d, "Cosmetic only" / optional) — disagree (subjective grouping preference, no technical impact).
    POST /api/v1/gh/pr/merge_state and POST /api/v1/gh/pr/ready sit in the GitHub Operations block alongside the other pr/* routes (pr/create, pr/comment, pr/edit, pr/close) — a defensible functional grouping, since they are PR operations. The Auth: Bearer {launcher_secret} line plus the "Orchestrator-only" wording already disambiguate the trust boundary, so there's no correctness or clarity gap. Happy to switch to the auth-boundary grouping if you'd prefer it — just say the word.

  4. "except ValueError, TypeError: is invalid Python 3 syntax → rebase" (agent-mode note @ 8c896170)disagree (false positive, and moot).
    The review bot itself confirmed this is valid under the repo's requires-python >= 3.14 / target-version = py314 (PEP 758 permits unparenthesized multiple exception types; ast.parse produces a byte-identical AST to the parenthesized form). It's also moot now: the files it referenced were deduped by the cbaacec merge and are no longer in the diff.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

…conflicts in slice-dag.md, STRUCTURE.md, test_pr_metadata.py
@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Resolved merge conflicts with main via merge commit 28b3afe2d (preserves PR history; revertable if needed). All three conflicts were additive/cosmetic — no semantic logic changes.

File Category Resolution
docs/architecture/slice-dag.md Additive Both sides append new rows to the Configuration-knobs table. Kept my EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS (typed int, env_config group) followed by main's three EGG_SLICE_GREEN_GATE* rows. Ordering respects the section's intro paragraph, which places the os.environ.get-read green-gate knobs last.
docs/development/STRUCTURE.md Additive Both sides add a new module entry to the orchestrator/ tree. Kept both cross_repo_merge_gate.py and main's slice_green_gate.py.
tests/shared/egg_contracts/test_pr_metadata.py Formatting/comment Comment-only conflict on the same ALLOWED_PATHS entry (shared/egg_contracts/tests/test_models.py). Took main's more detailed comment (names the specific test function). Entry appears exactly once; the tuple is consumed as an order-independent membership set, so behavior is unchanged.

Verification:

  • No conflict markers remain in any file.
  • tests/shared/egg_contracts/test_pr_metadata.py — valid Python syntax; ruff check + ruff format --check clean; allow-list entry present exactly once.
  • Confirmed the #cross-repo-merge-sequencing-hold-two-tier anchor referenced by the new config row exists (### Cross-repo merge-sequencing hold (two-tier) at slice-dag.md:817).
  • Full make test not run (per workflow policy — CI runs the complete suite on this push).

Please review: The Configuration-knobs table row ordering in slice-dag.md — I placed the cross-repo knob among the typed knobs and the green-gate knobs after it, matching the section intro's split between env_config-typed and os.environ.get-read knobs.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns (re-review).

Since my prior review at cbaaceca, the only PR-authored commit is the merge 28b3afe "Merge origin/main into egg/doc-update-cross-repo-merge-gate". Its conflict resolution is confined to three files and is additive/cosmetic:

  • docs/architecture/slice-dag.md — both sides append rows to the Configuration-knobs table; kept the EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS row followed by main's EGG_SLICE_GREEN_GATE* rows. No conflict markers, no duplicated prose.
  • docs/development/STRUCTURE.md — additive module-tree entry (cross_repo_merge_gate.py alongside main's slice_green_gate.py).
  • tests/shared/egg_contracts/test_pr_metadata.py — comment-only resolution on an existing ALLOWED_PATHS entry; provably inert (order-independent membership set, entry present exactly once).

None of these touch the agent-mode surface. The documented design remains well-aligned with agent-mode principles: the cross-repo merge gate enforces sequencing through gateway routes (/api/v1/gh/pr/merge_state, /api/v1/gh/pr/ready — launcher-auth, orchestrator-only control plane) and routes both Tier-A failure terminals and the Tier-B [hold:beyond-merge-state] opt-in through the HITL decision queue. That's sidecar/control-plane enforcement rather than prompt-level instruction — the right pattern.

No pre-fetching, human-facing structured output, post-processing pipelines, rigid procedures, prompt-level security, direct Anthropic API calls, or hardcoded model identifiers in the delta.

— Authored by egg

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

egg conflict resolution completed. View run logs

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

egg agent-mode-design completed. View run logs

24 previous review(s) hidden.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: docs for cross-repo merge gate (#3393 slice-5)

My prior review commit (cbaaceca) was rewritten off the branch, so I re-verified the full current diff against origin/main from scratch. Net diff is now genuinely docs-only — 5 files, +41/−12 (docs/architecture/orchestrator.md, docs/architecture/slice-dag.md, docs/development/STRUCTURE.md, docs/reference/orchestrator-cli.md, gateway/README.md). The earlier ruff-format churn, the except (A,B):except A,B: rewrites, and the models.py allowlist entry are all gone from the diff — the merge with main deduped them (those files are byte-identical to main). The prior "invalid Python 3 syntax" and "PR-description-claims-docs-only-but-carries-code" concerns are therefore both moot: there is no code in this diff.

Every technical claim re-verified against source

  • EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS default 240DEFAULT_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS = 240 (env_config.py:287, read at :400). ✓
  • "~2h at the default 30s reconciler cadence"DEFAULT_STACKED_PR_RECONCILER_INTERVAL_SECONDS = 30.0 (env_config.py:282); 240 × 30s = 7200s = 2h. ✓
  • Gateway routesPOST /api/v1/gh/pr/merge_state (gateway.py:4535) and POST /api/v1/gh/pr/ready (gateway.py:4631), both @require_launcher_auth and self-documented "orchestrator-only." The doc's Auth: Bearer {launcher_secret} line is the correct distinction. ✓
  • merge_state returns {state, mergedAt} — route requests --json state,mergedAt and returns state/mergedAt from the parsed payload. ✓
  • Merge detection keys off mergedAt/state, not head-SHA — matches the route surface and the doc's squash/rebase rationale. ✓
  • mark_pr_ready wraps gh pr readygateway.py:4634 docstring: "wrap gh pr ready (#3393)", fixed narrow argv. ✓
  • [hold:beyond-merge-state] Tier-B marker in goal or a task descriptionBEYOND_MERGE_STATE_MARKER = "[hold:beyond-merge-state]" (cross_repo_merge_gate.py:59); classify_hold_kind scans goal then each task.description (:161-164). ✓
  • Two HITL options (release → ready / keep → stays draft)_CROSS_REPO_HOLD_RELEASE_OPTION_* / _CROSS_REPO_HOLD_KEEP_OPTION_* (routes/pipelines.py:12811-12814), release marks ready, keep leaves it draft (:12951-12958). ✓
  • STRUCTURE.md module entryorchestrator/cross_repo_merge_gate.py exists; the one-line description matches its purpose. ✓
  • Anchor link — the config-knobs row (slice-dag.md:970) links #cross-repo-merge-sequencing-hold-two-tier, which resolves to ### Cross-repo merge-sequencing hold (two-tier) (slice-dag.md:817). Cross-file link ../../gateway/README.md from docs/architecture/ resolves correctly. ✓
  • No conflict markers in any of the 5 merged files. ✓

Non-blocking

  • PR description slightly over-discloses. The "Non-doc collateral" section still lists tests/shared/egg_contracts/test_pr_metadata.py (+4/-4) as "the single remaining non-doc file," but that file is now byte-identical to main and does not appear in this PR's diff. Harmless — this is over-disclosure, not under-disclosure — but the section could be trimmed to say the diff is purely docs. Not blocking.

No blocking issues. The docs faithfully describe the implemented behavior, and the diff is merge-safe and CI-clean.

— Authored by egg

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

egg review completed. View run logs

24 previous review(s) hidden.

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author
egg is addressing review feedback...

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Feedback disposition

All formal reviews are approving (agent-mode-design: approve; review bot: approve-with-suggestions). The diff is now genuinely docs-only — 5 files (docs/architecture/orchestrator.md, docs/architecture/slice-dag.md, docs/development/STRUCTURE.md, docs/reference/orchestrator-cli.md, gateway/README.md). Addressing the outstanding non-blocking items:

  1. PR description over-discloses — the "Non-doc collateral" section named tests/shared/egg_contracts/test_pr_metadata.py as "the single remaining non-doc file," but that file is byte-identical to main and no longer appears in the diff (review bot @ 28b3afe) — fixed (PR description edit, no commit).
    You're right: the 28b3afe merge with main deduped that file along with the earlier ruff-format churn and the orchestrator/models.py allowlist entry, so the description had gone stale in the over-disclosing direction. I replaced the "Non-doc collateral" section with a "Scope: docs-only" section that states the diff is purely documentation (the 5 files above), and notes that the format churn, the models.py allowlist entry, and the test_pr_metadata.py reorder were all deduped and are gone from the diff. This is a GitHub description change, not a working-tree change, so there is no commit.

  2. Gateway README: co-locate the two new launcher-auth routes with find_open_pr / list_open_prs (review @ 9cbed6d, "Cosmetic only" / optional) — disagree (subjective grouping preference, no technical impact).
    POST /api/v1/gh/pr/merge_state and POST /api/v1/gh/pr/ready sit in the GitHub Operations block alongside the other pr/* routes (pr/create, pr/comment, pr/edit, pr/close) — a defensible functional grouping, since they are PR operations, and moving them out would separate them from their functional siblings. Each carries an explicit Auth: Bearer {launcher_secret} line plus "Orchestrator-only" wording, which already disambiguates the trust boundary, so there's no correctness or clarity gap to close — the two groupings (functional vs. auth-boundary) are equally defensible. Happy to switch to the auth-boundary grouping if you'd prefer it — just say the word.

— Authored by egg

@james-in-a-box

james-in-a-box Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

egg feedback addressed. View run logs

24 previous review(s) hidden.

@jwbron
jwbron merged commit d87e5a6 into main Jul 3, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant