Fix #2263: per-phase consensus timeout defaults (refine/plan/implement)

[jwbron]

Summary

• Splits the global consensus_timeout_minutes into per-phase overrides so refine (1 producer / 2 reviewers, ~1 pass) and implement (3 producers / 5 reviewers, 2-3 NACK iterations common) can have phase-appropriate budgets without recalibrating each one against the smallest case.
• Adds consensus_timeout_minutes_{refine,plan,implement} to PipelineConfig (orchestrator/models.py:377-413) and a small resolver (resolve_consensus_timeout_minutes) consumed at the consensus polling read site (orchestrator/routes/pipelines.py:10705).
• Defaults now come from PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN: refine 30 / plan 60 / implement 90.

Resolution chain

The resolver picks a per-phase timeout in this order, highest priority first:

1. Per-phase override (consensus_timeout_minutes_<phase>) if set — explicit phase tuning wins.
2. Legacy global (consensus_timeout_minutes) if explicitly set — preserves the AC back-compat clause that pipelines passing only the global continue to behave identically across all three phases.
3. Phase-aware default from PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN.

To make step 2's "is it set?" check unambiguous, the legacy global's default flips from int = 30 to int | None = None. The only runtime reader was the line we replaced; tests and snapshots that explicitly set the field are unaffected.

Why this is cushion behind #2243's progress gate

PR #2254 (the progress gate) defers the auto-consensus-failure decision while progress signals are fresh. With the gate in place the timeout is only consulted when there's been no progress — which is the right time to fire. But:

• Early-phase silence is still possible (e.g. green-field implement waiting for plan to land context). A 30-min implement floor still cuts off legitimate slow starts.
• The gate is a deferral, not a removal; if the threshold expires and there's no progress, the decision still fires. Bumping implement to 90 min reduces the false-positive surface even when the gate doesn't help.

This is a different layer from #2245's post-timeout per-iteration clock (post_consensus_iteration_budget_seconds) — that governs time after the timeout fires; this governs time until it fires.

Acceptance criteria

• Per-phase timeout overrides on PipelineConfig.
• Read site at pipelines.py consumes the per-phase override when set, falls back to the global, then to the phase-aware default.
• Defaults: refine 30 / plan 60 / implement 90.
• Existing tests using only the legacy global still pass (the resolver returns the global for all three phases when the per-phase fields are unset).
• New tests cover the override path, the legacy back-compat path, and the phase-default path.
• Documentation updated in docs/guides/sdlc-pipeline.md and docs/guides/concurrent-execution.md.

Test plan

• make lint — clean.
• make test (changeset-aware) — local run skipped per request; CI is the ground truth.
• New TestResolveConsensusTimeoutMinutes cases assert: phase-aware defaults when nothing set; legacy global applies to all phases; per-phase override wins over global; per-phase override doesn't leak into other phases; unknown phase falls back to 30 (and to the legacy global when set).

Related

• Auto BRC-consensus-failure HITL decision at 30 min misfires on healthy long-running phases #2243 — parent; step 1 progress gate landed in Fix #2243: progress gate before BRC consensus-failure HITL decision #2254.
• [#1921 follow-up] Implement phase still force-kills at 90 min — post_timeout_budget is hardcoded 3600s with no per-phase / per-complexity knob #2245 — closed by Fix #2245: rebaseline post-consensus-timeout budget on producer progress #2253; ships the post-timeout per-iteration clock. Same family, different layer.
• Overseer plan-phase nudge thresholds (heartbeat 180s, post-ACK confirm 180s) feel tight for complex pipelines #2242 — overseer 180s thresholds. Same family of phase-blind defaults (out of scope here).

Closes #2263.

[egg-reviewer]

No agent-mode design concerns. This PR is purely orchestrator timeout configuration (per-phase overrides + resolver) with no impact on prompt construction, agent invocation, or output handling.

[egg-reviewer]

Review

Reviewed every changed file in the diff and traced the resolver's call sites and the field's readers across the repo. The change is well-scoped: only one direct reader of pipeline.config.consensus_timeout_minutes existed (orchestrator/routes/pipelines.py:10701, previously getattr(..., 30)) and it was migrated to the new resolver. No other code path reads the field, so the type flip from int = 30 → int | None = None is safe inside the orchestrator. The HITL decision text at pipelines.py:9732,9752 uses the local consensus_timeout (seconds), so operators see the resolved per-phase value when the timeout fires.

Tests cover the priority chain (override > legacy global > phase default), the legacy back-compat clause, the leak-prevention (per-phase doesn't bleed into others), and the unknown-phase fallback. The phase_str derivation at pipelines.py:10390 (phase if isinstance(phase, str) else phase.value) yields a member of PipelinePhase — "refine", "plan", "implement", or "pr" — and getattr(config, f"consensus_timeout_minutes_{phase}", None) falls through cleanly for "pr" (and for any future phase) via the unknown-phase branch returning 30. Pydantic's ge=1 validator on each override field guarantees raw_timeout >= 1, so the existing max(raw_timeout, 1) defensive clamp at line 10702 is now strictly redundant but harmless.

No blocking issues found.

Non-blocking suggestions

1. Stale example value in docs/guides/sdlc-pipeline.md:1235. The sample JSON still shows "consensus_timeout_minutes": 30. With the new defaults, setting the global to 30 overrides the calibrated per-phase budgets (so plan would shrink from 60 → 30 and implement from 90 → 30) — exactly the opposite of what someone copy-pasting the example would want. Either drop that line from the example (since null/unset is now the recommended default) or replace it with a per-phase override to demonstrate the new shape.

2. Hard-coded 30 in the unknown-phase fallback (orchestrator/models.py:43). PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN.get(phase, 30) re-encodes the legacy default outside the constant dict. If the dict ever grows a new phase or the floor is recalibrated, this number won't track it. Consider PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN.get(phase, PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN["refine"]) or pulling the floor into a named constant. Minor — the current behavior is documented in the test at test_models.py:460, so it's intentional, just a brittle seam.

3. Public-API schema break (advisory, not blocking). The field's type in PipelineConfig.model_json_schema() flips from {"type": "integer"} to {"anyOf": [{"type": "integer"}, {"type": "null"}]} and the default flips from 30 to null. External clients that POST configs are unaffected (omit-or-null is accepted; passing the int still works), but clients that read a serialized config and assume a non-null int — or generated TypeScript types pinned to the old schema — will see a contract change. Worth a one-liner in release notes; no code change needed.

4. Override-vs-global precedence is correct but worth a doc nudge. The legend in concurrent-execution.md:36-39 and sdlc-pipeline.md:1252-1255 clearly states "wins over the legacy global," but a reader who sets both consensus_timeout_minutes=120 and consensus_timeout_minutes_implement=60 may intuit that the larger value would apply (more time to converge). The current behavior — explicit-phase always wins, even if smaller — is the right call (explicit > implicit), it's just non-obvious. A short worked example in the docs would close the gap.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
docs/guides/concurrent-execution.md	Additive	Combined per-phase timeout rows + brc_consensus_progress_gate_seconds row from #2265 in the PipelineConfig table; merged the per-phase resolution sentence with the progress-gate paragraph in the Timeout Handling section so both #2263 and #2243's behaviour are described together.
docs/guides/sdlc-pipeline.md	Additive	Same treatment for the config table — kept the four per-phase rows (PR side) and the brc_consensus_progress_gate_seconds row (main side).

No source-code conflicts: orchestrator/models.py and orchestrator/routes/pipelines.py only had context-line drift, no overlapping edits with #2254's progress-gate code. The resolver (this PR) and the progress gate (#2254) interoperate cleanly — the gate gets the per-phase budget from resolve_consensus_timeout_minutes at the same read site.

Extra commit: file-size allowlist bump

Lint surfaced a pre-existing drift: #2254 (progress gate, +158 lines to pipelines.py, +15 lines to peer_consensus.py) merged before #2250 (file-size lint), and #2250 didn't refresh its allowlist before merging — so main itself is over baseline by exactly the amount #2254 added. This PR adds 1 more line to pipelines.py, which trips the lint. Bumped the baseline to the post-merge state (pipelines.py: 15515 / 677159 B; peer_consensus.py: 2003 / 85965 B) in a separate commit; issue #2248 still tracks the underlying decomposition work.

Verification

• make lint — clean (only soft-cap warnings, no errors).
• .venv/bin/pytest orchestrator/tests/test_models.py — 76 passed, including all 7 new TestResolveConsensusTimeoutMinutes cases (phase defaults, legacy back-compat, per-phase override wins, override doesn't leak, unknown-phase fallback paths).
• .venv/bin/pytest -k "consensus_timeout or peer_consensus or progress_gate or polling" — 161 passed across consensus/peer-consensus/timeout-recheck/polling suites.
• make test (changeset-aware full suite, triggered by docs changes) — exceeded the local 10-minute budget; CI is the ground truth here.

Please review: the timeout-handling paragraph in concurrent-execution.md — I kept the per-phase resolution sentence as the lead-in and folded the progress-gate paragraph after it, so the flow is now "per-phase budget elapses → check progress gate → if quiet, evaluate blockers." That ordering matches the runtime flow but is editorial; flag if you'd prefer a different framing.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Custom Checks": 1, "Lint/Python": 3}

[egg-reviewer]

No agent-mode design concerns. The delta since my last review at 38e59ac is a docs merge resolution (folding the brc_consensus_progress_gate_seconds row from #2254 into the per-phase timeout tables and rewriting the Timeout Handling paragraph to describe gate → blocker-eval ordering) plus a file-size allowlist bump. Neither touches prompts, model IDs, agent output structure, or sandbox boundaries.

— Authored by egg

[egg-reviewer]

Re-review (delta since 38e59ac)

The substantive code change (resolver + per-phase config fields + read-site swap) was already approved and is unchanged on this commit. Since 38e59ac the PR added two main commits inbound (#2250 file-size lint, #2265 progress-gate doc), one merge commit, and two allowlist baseline bumps. I verified each.

Doc merge resolution — correct

Both docs/guides/concurrent-execution.md (lines 36–41 / 724–731) and docs/guides/sdlc-pipeline.md (lines 1252–1257) cleanly fold the per-phase rows from this PR with the new brc_consensus_progress_gate_seconds row from #2265. The Timeout Handling paragraph at concurrent-execution.md:726–731 correctly orders the runtime flow as "per-phase budget elapses → progress gate → blockers eval," which matches the read site (pipelines.py:10701 resolves the per-phase budget; PeerConsensusTracker.handle_timeout() is the post-gate evaluator). No semantic loss from either side.

Allowlist bumps — two of three match main

scripts/file-size-allowlist.yaml:

• orchestrator/peer_consensus.py → 2003 / 85965. Main has peer_consensus.py at exactly 85965 bytes (gh api .../contents?ref=main). PR doesn't touch this file — bump just absorbs main's drift from #2254 that #2250 missed. ✓
• orchestrator/routes/pipelines.py → 15515 / 677159. Main is at 677112 bytes; PR adds 1 line at pipelines.py:10376 (resolve_consensus_timeout_minutes import) plus the read-site swap, ~47 bytes — post-merge result lands at 677159, matching the new baseline. ✓
• scripts/select_tests.py → 1850 / 73711. Stale by 1495 bytes. The bump was made after #2262 landed on main (which grew the file to 73711) but before #2266 landed (which grew it further to 75206). Current main has select_tests.py at 75206 bytes; the PR is behind_by=2 against main, with #2266 as the second behind-commit. After GitHub auto-merges main into this PR (or after merge into main), the resulting tree will pair main's 75206-byte select_tests.py with this PR's 73711 baseline → make lint will fail with select_tests.py: 75206 bytes exceeds allowlist baseline (73711). Non-blocking because the PR's own tree still passes (PR's select_tests.py is at 63828, well under 73711), but the next rebase against main will need another bump. Worth a follow-up commit before merge to avoid breaking lint on main.

Resolver + read site — unchanged, still correct

I re-confirmed the resolver wiring on this commit: models.py:26-43 (priority chain override > legacy global > phase default), pipelines.py:10376 import, pipelines.py:10701 consumption. Only one direct production reader of config.consensus_timeout_minutes exists outside the resolver (a test setter at test_agent_exits_recorded.py:31), so the type flip from int = 30 → int | None = None remains safe.

Previous non-blocking suggestions

Three of four suggestions from my prior review remain unaddressed (stale "consensus_timeout_minutes": 30 example at sdlc-pipeline.md:1235, hardcoded 30 fallback at models.py:43, public-API schema flip note). Author's prerogative — none are blocking. The example at sdlc-pipeline.md:1235 is now arguably worse than stale: with the new calibrated defaults, copy-pasting "consensus_timeout_minutes": 30 actively regresses plan from 60→30 and implement from 90→30. If the author is taking another commit anyway for the select_tests.py baseline, this would be a cheap addition.

No blocking issues found.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
scripts/file-size-allowlist.yaml	Additive (baseline numbers)	Set baselines to actual post-merge file sizes

Detail

Both sides bumped allowlist baselines for the same two files differently:

• orchestrator/routes/pipelines.py — PR had 15515 / 677159; main had 15593 / 681405 (from Fix #2245: rebaseline post-consensus-timeout budget on producer progress #2253's post-timeout rebaseline + Fix #2243: progress gate before BRC consensus-failure HITL decision #2254's progress gate). After merging, the file is 15594 / 681452 — main's growth plus the PR's resolve_consensus_timeout_minutes import line. Set baseline to the actual post-merge size.
• scripts/select_tests.py — PR had 1850 / 73711; main had 1875 / 75206 (from select_tests: bare-name AST resolver, empty-diff skip, drop canary #2262's bare-name AST resolver and Fix #2259: scrub <root>/scripts from sys.path during graph build #2266's <root>/scripts sys.path scrub). PR doesn't touch select_tests.py, so the post-merge file size matches main's exactly. Set baseline to main's value.

The auto-merged files (orchestrator/models.py, orchestrator/routes/pipelines.py) integrated cleanly: the PR's resolve_consensus_timeout_minutes import sits next to main's other model imports, and the read site at pipelines.py:10701 consumes the resolver as expected. New file orchestrator/tests/test_post_timeout_rebaseline.py (from #2253) merged in additively.

Verification

• make lint — clean (file-size lint exit 0, ruff/mypy/shellcheck pass).
• orchestrator/tests/test_consensus_polling.py — 15/15 pass (covers the PR's resolver path).
• orchestrator/tests/test_post_timeout_rebaseline.py — 11/11 pass (from main, no regression).
• The full orchestrator suite has 7 pre-existing sandbox-environment failures (missing kubernetes.client.VersionApi in the sandbox image; gateway-blocked git init in test_reconcile_and_push_pr_branch.py). All unrelated to the merge — CI is the ground truth.

Please review: the pipelines.py baseline bump from main's 15593 to 15594. The +1 line is the resolve_consensus_timeout_minutes symbol added to the from models import (…) block, which is the PR's only pipelines.py change. If a reviewer prefers the baseline numbers to be stable across PRs, this can be re-bumped post-merge with a follow-up.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns. The delta since my last review at fd466d10 is two merge commits from origin/main (06db62b8, 2ee6c035) — both mechanical conflict resolutions in scripts/file-size-allowlist.yaml to absorb #2274's schema change (per-file baselines dropped) and the post-merge pipelines.py byte count. The substantive code paths I reviewed previously (models.resolve_consensus_timeout_minutes, the pipelines.py:10701 read site, the per-phase config fields) are byte-identical on the new tip.

— Authored by egg

[james-in-a-box]

egg check fixer encountered an issue fixing Lint. View run logs

— Authored by egg

[james-in-a-box]

egg is investigating the Lint check failure...

• Python

[james-in-a-box]

egg check fixer encountered an issue fixing Lint. View run logs

— Authored by egg

[james-in-a-box]

egg is investigating the Lint check failure...

• Python

[james-in-a-box]

Autofix: Human Input Needed

The following checks could not be fixed automatically after multiple attempts:

Check	Attempts	Logs
Lint / Python	3/3	View

Please investigate manually or push a fix.

— Authored by egg

[james-in-a-box]

egg is investigating the Lint check failure...

• Python

[james-in-a-box]

Autofix: Human Input Needed

The following checks could not be fixed automatically after multiple attempts:

Check	Attempts	Logs
Lint / Python	3/3	View

Please investigate manually or push a fix.

— Authored by egg

[james-in-a-box]

egg is investigating the Lint check failure...

• Python

[james-in-a-box]

Autofix: Human Input Needed

The following checks could not be fixed automatically after multiple attempts:

Check	Attempts	Logs
Lint / Python	3/3	View

Please investigate manually or push a fix.

— Authored by egg

[james-in-a-box]

egg is resolving merge conflicts for this PR...

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
docs/guides/concurrent-execution.md (config table, lines ~36-45)	Semantic (complementary)	Kept this PR's per-phase timeout fields (consensus_timeout_minutes_{refine,plan,implement}); adopted main's OVERSEER_ALERT terminology in the brc_consensus_progress_gate_seconds description (from #2264 / #2277).
docs/guides/concurrent-execution.md (timeout handling, line ~733)	Semantic (complementary)	Combined: kept the per-phase resolution chain prose ("per-phase override > legacy global > calibrated default — refine 30, plan 60, implement 90") and replaced "opening a HITL decision" with "publishing the OVERSEER_ALERT" to match main.
docs/guides/sdlc-pipeline.md (config table, lines ~1259-1268)	Semantic (complementary)	Same shape as above: kept all four per-phase rows; adopted the OVERSEER_ALERT description for brc_consensus_progress_gate_seconds.
docs/guides/sdlc-pipeline.md (consensus protocol step 5, line ~1336)	Semantic (complementary)	Combined: kept this PR's "per-phase: refine 30 / plan 60 / implement 90 by default; configurable via consensus_timeout_minutes_<phase> or the legacy global" wording, then attached main's OVERSEER_ALERT-publishes-instead-of-HITL clause and the /sdlc skill / post-timeout-budget bullets.

Both sides were complementary: this PR adds per-phase overrides; #2264/#2277 swapped the post-timeout escalation surface from a HITL decision to an OVERSEER_ALERT. The merged text retains both.

Post-merge cleanup

• orchestrator/models.py: dropped the "PipelineConfig" quotes on the resolve_consensus_timeout_minutes forward reference. The ruff bump in Sync ruff pin to v0.15.12 (uv.lock + pre-commit) and apply formatter sweep #2297 (now on main) flagged this as UP037; PEP 649 lazy evaluation in py3.14 (the project target) makes the quote unnecessary. Committed separately so it's easy to revert if needed.

Verification

• make lint (python, shell, custom checks) — ✅ clean. yamllint / hadolint / actionlint are skipped in this sandbox (not installed); shfmt is also missing but that only affects make lint-fix, not make lint.
• make test cannot run locally (grimp missing in this sandbox; same constraint the original PR description called out — "local run skipped per request; CI is the ground truth"). Ran the targeted suite directly: pytest orchestrator/tests/test_models.py -k "consensus_timeout or ResolveConsensusTimeout" — 7 passed, covering the resolver paths.

Please review: the four doc conflicts were all the same shape — keep my prose, adopt main's terminology — but they're each in slightly different framings (table row, paragraph, table row, numbered-list item). Worth a skim to confirm the merged sentences read naturally and the OVERSEER_ALERT reference is consistent with the surrounding text added in #2277.

— Authored by egg

[james-in-a-box]

egg conflict resolution completed. View run logs

[egg-reviewer]

No agent-mode design concerns. The delta since my last review at 2ee6c035 is two mechanical changes:

1. Doc conflict resolution (docs/guides/concurrent-execution.md, docs/guides/sdlc-pipeline.md) folding main's OVERSEER_ALERT terminology from #2264/#2277 into this PR's per-phase timeout prose — additive, no semantic loss.
2. orchestrator/models.py — dropped the "PipelineConfig" forward-reference quote on resolve_consensus_timeout_minutes per ruff UP037 (post-#2297 sweep). Pure typing cleanup; no behaviour change.

The substantive code paths (resolve_consensus_timeout_minutes, the per-phase config fields, the pipelines.py:10894 read site) are unchanged from the previously approved commit. No prompts, model IDs, agent output structure, or sandbox boundaries touched.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

26 previous review(s) hidden.

[egg-reviewer]

Re-review (delta since 2ee6c03)

The substantive code (resolver, per-phase fields, read-site swap) was already approved at 38e59ac and is unchanged in functionality. Since 2ee6c035 the PR has added two PR-authored commits plus a merge resolution:

• f692be3 — Addresses two of my prior non-blocking suggestions:
  • docs/guides/sdlc-pipeline.md:1232-1246 — drops the stale "consensus_timeout_minutes": 30 from the JSON example and replaces it with prose + a worked override example. Copy-pasters can no longer accidentally regress plan/implement back to 30. ✓
  • orchestrator/models.py:45-47 — replaces the hardcoded 30 fallback with PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN["refine"], with a doc-comment explaining the choice (smallest calibrated budget = safe upper bound for unknown phases). ✓
  • orchestrator/tests/test_models.py:458-461 — test renamed and asserts against the constant. ✓
• 8a6a241 — Doc-only conflict resolution combining per-phase tables (this PR) with the consensus-timeout-HITL → OVERSEER_ALERT migration (#2264) on the concurrent-execution.md and sdlc-pipeline.md config tables and the Timeout Handling section. I traced both files and the resolved narrative correctly orders runtime flow as "per-phase budget elapses → progress gate → publish OVERSEER_ALERT." No semantic loss from either side.
• 57e9fd8 — Drops the PipelineConfig forward-reference quotes per ruff UP037 (post-#2297 ruff bump). Verified at runtime under py3.14: with PEP 649 lazy annotation evaluation, inspect.signature(resolve_consensus_timeout_minutes) resolves to models.PipelineConfig cleanly even though the function is defined before the class. The annotation is never accessed at function-definition time, so the bare reference works.

#2264 OVERSEER_ALERT integration — verified

pipelines.py:9758 (timeout_minutes = int(consensus_timeout / 60)) and pipelines.py:9802 (metadata["consensus_timeout_minutes"] = timeout_minutes) both consume the resolved per-phase consensus_timeout (from pipelines.py:10897's resolve_consensus_timeout_minutes() call). So the OVERSEER_ALERT body and metadata correctly reflect the resolved per-phase value (e.g., "BRC consensus has not converged after 90 minutes" for default-implement). The two changes compose cleanly.

Tests

I ran the relevant suites locally (system py3.14):

• orchestrator/tests/test_models.py — 76 passed, including all 7 TestResolveConsensusTimeoutMinutes cases.
• test_consensus_polling.py, test_consensus_timeout_recheck.py, test_consensus_complete_with_failures.py, test_pipelines_routes.py — 49 passed.
• test_brc_nack_iteration.py, test_consensus_race_on_exit.py, test_short_flow_contract_reviewer.py, test_agent_exits_recorded.py, test_post_timeout_rebaseline.py — 46 passed.

No regressions from the merge.

Non-blocking observation (one new)

orchestrator/models.py:42: return override triggers mypy's no-any-return warning because getattr(config, ..., None) is typed Any. Not CI-breaking — make lint-python runs mypy on gateway shared sandbox only, not orchestrator, so this is silent today. Verified with a minimal repro: annotating override: int | None = getattr(...) clears it cleanly. Mention only because if orchestrator is ever added to the mypy frontier, this seam will flag.

Previous non-blocking suggestions

Two of four are now resolved (above). The remaining two — public-API JSON-schema flip (advisory release-notes nudge) and a worked override-vs-global precedence example in the docs — are unchanged. The new prose at sdlc-pipeline.md:1240-1246 partially addresses the precedence one with the consensus_timeout_minutes_implement: 120 example, so I'm dropping it. The schema-flip note remains advisory only.

No blocking issues found.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

26 previous review(s) hidden.

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Agreed. Addressed the mypy no-any-return observation in 0a2fdc3 — annotated override: int | None = getattr(...) at orchestrator/models.py:40 per your minimal repro. Cheap insurance for when orchestrator joins the mypy frontier.

Leaving the public-API JSON-schema flip as advisory release-notes only, as you suggested — no code change needed there.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

26 previous review(s) hidden.

[egg-reviewer]

No agent-mode design concerns. Delta since last review is a single mypy annotation fix in orchestrator/models.py (override: int | None = getattr(...)) — no impact on agent design.

— Authored by egg

[egg-reviewer]

Re-review (delta since 57e9fd8)

Single-line delta: orchestrator/models.py:40 annotates override: int | None = getattr(...). This directly addresses the one non-blocking observation from my prior review (mypy no-any-return).

Verified

• Annotation matches the underlying field types — consensus_timeout_minutes_{refine,plan,implement} are all int | None (models.py:420,428,436), and unknown phases fall through to the getattr default None. int | None is the tight union.
• Function return type -> int still holds: override is only returned inside if override is not None.
• git diff prev-review-tip..pr-2267 --stat confirms exactly one file, one insertion, one deletion. No other behavior touched.
• Resolver semantics, per-phase fields, and the pipelines.py read site are unchanged from the previously approved commit.

No blocking issues. The mypy no-any-return seam is now closed pre-emptively, so when orchestrator joins the mypy frontier this won't flag.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

26 previous review(s) hidden.

[james-in-a-box]

egg review completed. View run logs

26 previous review(s) hidden.