Fix #2593: wire context-PR hook into all plan→implement transition paths - #2599
Conversation
The context PR (#2548) is the structural base PR for the slice stack — slice-1 stacks on egg/<pipeline>/context, every later slice on its parent, and only the context PR merges into main/deploy. So when the hook fails to open it, the slice stack has no path to the base branch. #2548 wired _open_context_pr_for_pipeline into only the inline _run_pipeline auto-advance path. Operators clearing the plan gate via the advance_phase REST/MCP handler or the HITL-approval recovery in start_pipeline silently bypassed the hook — pipeline #2474 hit exactly this: contract.pr.context_branch was null, the slice-1 base resolver fell back to pipeline_branch (/work), and slice PRs landed on /work with no PR to main. Changes: * Extract _maybe_open_base_pr_for_plan_to_implement wrapper that owns the CUSTOM-mode guard, the swallow-all-exceptions semantics, and a source kwarg for logging. * Wire the wrapper into advance_phase REST (routes/phases.py), the HITL-approval recovery in start_pipeline (also adds the missing _populate_contract_from_plan_safe call on that path so the context PR has a contract.pr to read from), and add an implement-entry backstop at the top of the IMPLEMENT phase handler so any future transition path inherits the hook automatically. The inner context_pr_number short-circuit keeps multiple invocations safe. * Emit a single "Context PR hook entered" log line at the top of _open_context_pr_for_pipeline, before any short-circuit, so the gap is detectable in logs without grepping per-short-circuit strings. * When the hook returns without opening a PR on a pipeline that should have one (has a remote + base_branch but no context_pr_number afterwards), emit a context_pr.skipped or context_pr.failed message on the pipeline message bus so operators using wait-status / get_status see it without grepping orchestrator logs. Tests (orchestrator/tests/test_context_pr_transition_paths.py): pin the wrapper's CUSTOM-mode skip, source-kwarg propagation, exception swallow, "hook entered" log line on idempotent skip, message-bus emission paths, and a textual check that the helper is called from all four expected call sites (auto-advance, advance_phase REST, HITL resume, implement-entry backstop). The existing test_context_pr.py call-site-shape regressions are updated to assert the wrapper-level shape since the per-call-site guard moved into the wrapper. Closes #2593.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid fix for a real bug. The architecture is right: extract a single wrapper that owns the CUSTOM-mode guard, the swallow semantics, and the message-bus emission; route every plan→implement transition path through it; add a phase-entry backstop so a future path that forgets the ceremony still gets caught.
No blocking issues found. A handful of non-blocking concerns below; merge-safe as-is.
Non-blocking
1. HITL recovery now holds the per-pipeline state lock across a multi-second network sequence
_open_context_pr_for_pipeline explicitly documents (pipelines.py:9636-9654) that it deliberately avoids holding a process-wide lock across its network sequence (create_context_branch → file copy → commit → push → gh pr create), trading the lock for convergent gateway-side idempotency. The other three call sites honor that contract — they're outside any pipeline state lock.
The new HITL call site at pipelines.py:20677 is inside with get_pipeline_state_lock(pipeline_id): (opened at line 20488), so the helper runs holding the lock for the full duration of the network sequence. Concurrent advance_phase, status reads, etc. on the same pipeline will block for ~10s.
Consistent with the existing push_worktree_branch at line 20598 that's already inside the same lock, so this isn't a regression in pattern — but it's a noticeable extension of the lock-held duration. Consider moving the _maybe_open_base_pr_for_plan_to_implement call to after the with block exits (the helper only needs pipeline, _hitl_worktree_path, and _gw_mode, all of which are stable after the lock release; the populate+commit can stay inside the lock).
2. Duplicate context_pr.skipped/context_pr.failed bus events on failure
On the auto-advance happy path, the wrapper runs twice (once at pipelines.py:19947 with source="run_pipeline_autoadvance", then again at pipelines.py:18681 via the implement_entry_backstop after PENDING→RUNNING). The inner hook's idempotent short-circuit on context_pr_number makes the second a no-op for the PR-creation path.
But the status-bus emission in _maybe_open_base_pr_for_plan_to_implement (lines 10316-10331) doesn't share that idempotency: it re-reads the contract after every invocation, sees _ctx_pr_number is None if the hook failed the first time, and emits another context_pr.failed. So on a real failure operators see 2 identical events (autoadvance + backstop). Same with HITL recovery (hitl_resume + backstop = 2 events).
Cheapest fix: dedupe in the wrapper by checking whether a context_pr.failed/context_pr.skipped was already emitted for this pipeline since the last successful PR open. Or: only emit from the backstop site, not from the upstream call sites. Or accept the noise — it's not harmful, just noisy in wait-status.
3. Wrapper status-bus emission misclassifies "PR opened but contract write failed"
The wrapper decides failed vs skipped based on whether _open_context_pr_for_pipeline raised. But the inner hook has several "log + return None" paths late in its sequence (after the PR is actually created on GitHub) — e.g. a save_contract write failure that swallows but doesn't re-raise. In those cases the wrapper sees:
- No exception raised →
raised is None _ctx_pr_numberstill None (contract write didn't land)- Emits
context_pr.skippedwith "Slice stack will not have a path to the base branch until an operator opens one manually"
…even though a context PR is sitting on GitHub. Misleading to operators. Worth a docstring note on _maybe_open_base_pr_for_plan_to_implement flagging that "skipped/failed" reflects contract state, not PR state on GitHub.
4. Implement-entry backstop only fires when phase_execution.status == PENDING
The backstop at pipelines.py:18680 lives inside the if phase_execution.status == PipelineStatus.PENDING: block. That branch is entered only on first encounter of a phase. The advance_phase REST handler at phases.py:379 explicitly sets target_execution.status = PipelineStatus.RUNNING before spawning the new _run_pipeline thread, so when that thread iterates and reaches IMPLEMENT, the PENDING branch is skipped and the backstop does not fire.
In other words, the docstring claim that the backstop "catches any future transition path added without ceremony" is overstated — it catches paths that leave the phase status as PENDING, not paths that set RUNNING directly. Currently all four paths are wired, so this is a documentation issue, not a runtime gap. Either tighten the docstring or move the backstop to a position that fires regardless of phase_execution.status (e.g. right after the phase.started event emission).
5. HITL recovery doesn't push the populated contract
pipelines.py:20642-20672 populates and commits the contract on the HITL recovery path, but unlike the auto-advance flow (which pushes via push_worktree_branch at line 19921 before calling the helper), the HITL branch does the push earlier — at line 20598, before the new populate commit lands. So the populated contract.pr block sits locally only until the IMPLEMENT phase's next phase-boundary sync.
The context-PR helper reads from the local worktree, so this isn't a problem for the immediate hook execution. The risk is downstream: any slice-agent container that materializes a fresh worktree from origin (rather than reusing the existing one) before that next sync won't see contract.pr. Worth either adding a follow-up push_worktree_branch after the populate-commit, or noting the asymmetry in the comment.
6. source="advance_phase_force" is semantically inaccurate on the HITL path
At pipelines.py:20650, the HITL recovery calls _populate_contract_from_plan_safe(source="advance_phase_force"). That value is the closest match in the Literal["plan_complete", "advance_phase_force"] type, but the path is not an advance_phase force — it's an HITL plan-gate approval. Easy follow-up: extend the Literal with "hitl_plan_gate_approval" and use it here, so log telemetry is accurate.
7. Wrapper loads contract by pipeline_id; inner hook loads by _pipeline_identifier(...)
_maybe_open_base_pr_for_plan_to_implement at line 10308 does _ctx_load(pipeline_id, worktree_repo_path), but _open_context_pr_for_pipeline at line 9709 does load_contract(identifier, worktree_repo_path) where identifier comes from _pipeline_identifier(issue_number, pipeline_id, mode). Both resolve to the same .egg-state/contracts/<canonical_key>.json path today via _canonical_key, but the inconsistency is fragile if _pipeline_identifier's logic ever changes for ISSUE-mode keys. Cheapest fix: have the wrapper call _pipeline_identifier and pass the result, mirroring the inner hook's usage.
8. test_helper_called_from_all_four_sites is brittle
pl_calls = pl_src.count("_maybe_open_base_pr_for_plan_to_implement(") and the == 4 assertion will trip if anyone adds a docstring example with the call shape, or splits the import onto multiple lines, etc. The textual 'source="..."' checks in the same class have the same brittleness. The behavioural tests for the wrapper itself in this file are already strong — these textual guards add little signal beyond the regex tests they replaced. Consider replacing with ast-based call-site detection if the wiring guard is worth keeping at all.
9. Minor: _CtxCNF catch in wrapper is redundant
pipelines.py:10311-10314:
except _CtxCNF:
pass
except Exception: # noqa: BLE001
passBoth branches converge to _ctx_pr_number = None. The dedicated _CtxCNF catch reads like it's distinguishing "expected" from "unexpected", but neither path acts on the distinction. A single except Exception would be equivalent and one import lighter.
10. CUSTOM-mode pipelines lose the "hook entered" log line
The wrapper returns at line 10269 before calling the inner hook for CUSTOM mode, so the new "Context PR hook entered (#2548)" log line at line 9665 never fires for that mode. The whole point of the log line (per its comment) is to be reachable on every transition path so operators can confirm the hook ran. CUSTOM mode legitimately skips, but a single "Context PR hook skipped (CUSTOM mode)" log emission from the wrapper would close that small observability gap.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* Item 1: Move HITL recovery context-PR open outside per-pipeline state lock so the multi-second gateway sequence no longer extends lock-hold time. Populate + commit stay inside; the network call runs after the lock releases via deferred-flag plumbing. * Item 2: Dedupe context_pr.skipped / context_pr.failed bus events via a module-level set keyed on (pipeline_id, event_type) so two transition paths firing the wrapper for the same pipeline emit at most one event per kind. * Item 3: Docstring note flagging that context_pr.skipped / .failed reflects contract state, not PR state on GitHub (late save_contract swallows can leave a real PR on GitHub with an unset context_pr_number). * Item 4: Tighten implement-entry-backstop docstring to spell out that it only fires when phase_execution.status stays PENDING through the transition — paths that set RUNNING before spawning the runner thread (advance_phase REST) bypass it and must call the wrapper directly. * Item 5: HITL recovery now pushes the populated contract after the populate commit so any slice-agent container that materialises a fresh worktree from origin before the next phase-boundary sync sees contract.pr. * Item 6: Add hitl_plan_gate_approval to the Literal type of _populate_contract_from_plan_safe.source and use it on the HITL path so log telemetry distinguishes HITL recovery from advance_phase force. * Item 7: Wrapper now resolves contract identifier via _pipeline_identifier (mirroring the inner hook) so the two sites never drift if the ISSUE-mode key logic changes. * Item 8: Replace brittle pl_src.count(...) / regex-based call-site audit with an AST walk that counts ast.Call nodes only (function definitions excluded) and asserts the expected source= literals. * Item 9: Remove redundant _CtxCNF except-branch — both converged on _ctx_pr_number = None, so a single except Exception suffices. * Item 10: Emit a "Context PR hook skipped (CUSTOM mode)" log line on the early return so the observability promise of one log record per call site holds for CUSTOM mode too. Tests: 242 pipeline-related tests pass, including 11 new behavioural tests for the dedupe + CUSTOM-mode log line + AST call-site audit.
|
Addressed all 10 non-blocking review items in 596ea5a.
All 242 pipeline-related tests pass (existing 51 in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
All 10 items from the prior review are addressed correctly. Verified:
- HITL context-PR call moved out of the per-pipeline state lock via deferred-flag plumbing (
pipelines.py:20556-20557,20781-20782,20881-20888); the lock release sits between the contract populate/commit/push (inside) and the multi-second gateway sequence (after). Stablepipelineattributes only read by the wrapper, so no race. _context_pr_events_emitted: dict[str, set[str]]+threading.Lock(pipelines.py:10240-10241); dedupe check at10367-10371mark-then-emit, so transientreport_pipeline_statusfailures still consume the slot — that's the right semantic.- Docstring note at
pipelines.py:10285-10295correctly flags "skipped/failed = contract state, not PR state on GitHub." - Backstop docstring at
10263-10270plus the inline comment at18729-18742now spell out the PENDING precondition and nameroutes/phases.py:379as the bypass path. - Second
push_worktree_branchafter the HITL populate commit at20753-20767mirrors the auto-advance flow. Literalextended to"hitl_plan_gate_approval"on_populate_contract_from_plan_safe.source(pipelines.py:17077-17081) and used at the HITL call site.- Wrapper resolves contract id via
_pipeline_identifierat10345-10349, mirroring the inner hook. - AST-based call-site audit (
_collect_helper_call_sourcesattest_context_pr_transition_paths.py:472-497) replaces the brittlepl_src.count(...)+ per-source'source="..."'checks. Verified the AST walk reports[run_pipeline_autoadvance, hitl_resume, implement_entry_backstop]forpipelines.pyand[advance_phase_rest]forphases.py. - Redundant
_CtxCNFbranch collapsed to a singleexcept Exceptionat10356-10360. - CUSTOM-mode log line at
10305-10309closes the observability gap; newtest_emits_skip_log_line_for_custom_modepins it.
No blocking issues found.
Non-blocking
1. _context_pr_events_emitted is not cleared by _clear_pipeline_runtime_state — same shape as #2053
The dedupe dict at pipelines.py:10240 is module-level and indexed by pipeline_id alone. _clear_pipeline_runtime_state (pipelines.py:2005-2090) is the canonical eviction point for per-pipeline state keyed by pipeline_id — it already clears the peer-consensus tracker, the legacy consensus evaluator, and the inter-agent message store specifically to prevent a fresh pipeline that reuses an id from a prior terminal run from inheriting stale state. The PR's docstring on the new dict even names that scenario as the design assumption ("once context_pr_number is set we never reach the emit branch again") — but that assumption is per-pipeline-lifecycle, not per-pipeline-id.
Concrete failure mode: pipeline issue-2593 runs, fails to open the context PR → context_pr.failed is emitted and the dedupe set records {"issue-2593": {"context_pr.failed"}}. Pipeline terminates / is deleted / is recreated with the same id (allowed — branch-reuse logic at pipelines.py:1663 is explicit about terminal-state reuse). The fresh pipeline fails to open its context PR for an unrelated reason. The wrapper checks the dedupe set, finds the prior run's entry, and returns without emitting. Operators using wait-status see no event for the new failure.
Lower severity than #2053 (observability gap, not a correctness claim that affects downstream decisions), but it is the same class of leak the existing eviction function exists to prevent. The autouse reset_context_pr_dedupe fixture in test_context_pr_transition_paths.py:108-117 quietly masks the issue from the tests — its docstring even acknowledges the dedupe persists across calls. The cleanest fix is one line in _clear_pipeline_runtime_state:
try:
_context_pr_events_emitted.pop(pipeline_id, None)
except Exception:
pass(wrapped with the lock, matching the other clear-on-terminal blocks in that function).
2. HITL recovery now has two push_worktree_branch calls inside the per-pipeline lock
The pre-existing push at pipelines.py:20665-20680 (after _persist_phase_gate_resolution + commit) and the newly-added push at 20753-20767 (after _populate_contract_from_plan_safe + commit) both target pipeline.branch with the same gateway mode, back-to-back inside the same with get_pipeline_state_lock(pipeline_id): block. That's ~2-6s of network time inside the lock for what could be a single push of two commits. Net win versus the prior state — item 1 removed ~10s of context-PR work from the lock, so the lock-hold is shorter overall — but the doubled push call is avoidable. Either commit both statefile changes before pushing once, or accept the cost. Not worth blocking on.
3. No integration test for the deferred-call ordering in start_pipeline's HITL branch
The wrapper is well-tested in isolation (the new test file pins CUSTOM-mode skip, dedupe, exception swallow, source propagation, and call-site presence via AST). The AST check confirms source="hitl_resume" exists somewhere in pipelines.py. What is not pinned is the ordering invariant the PR introduces: that the deferred context-PR call runs after lock release and before the runner thread starts. A future refactor could put the deferred call back inside the lock, or run it on a worker thread after the runner has started, without tripping any test in this PR. The behaviour the lock-release achieves (allowing concurrent operations during the network sequence) is hard to test deterministically, so this gap is acknowledged trade-off rather than oversight — but worth flagging.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ntime-state evict
The _context_pr_events_emitted dedupe set is keyed by pipeline_id
alone, but its assumption ('once context_pr_number is set we never
reach the emit branch again') is per-pipeline-lifecycle, not
per-pipeline-id. A fresh pipeline reusing an id from a prior
terminal run (allowed by branch-reuse logic) would inherit the
prior run's emitted-event set; a context_pr.failed in the new run
would be silently swallowed.
Add the eviction to _clear_pipeline_runtime_state — the canonical
terminal-state hook that already evicts the peer-consensus
tracker, legacy consensus evaluator, and inter-agent message
store for the same per-pipeline-id leak shape (#2053).
Test pins the behaviour end-to-end: seed the dedupe set, call
_clear_pipeline_runtime_state, assert the entry is gone.
|
Addressed review 2 feedback. Disposition per item:
All 53 tests in — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Item 1 from the prior review is correctly addressed; the two disagree responses on items 2 and 3 are well-reasoned (the reviewer flagged both as non-blocking and acknowledged the implementation trade-offs).
Verified the new delta (pipelines.py:2076-2093, test_pipelines_api.py:1264-1291):
- The eviction is placed inside
_clear_pipeline_runtime_stateimmediately after the message-store clear and follows the exact same shape (try/except Exception+ warning log) as the surrounding clears for the peer-consensus tracker, legacy consensus evaluator, and message store. - Lock usage is symmetric with the wrapper's emission site at
10386-10390— both go through_context_pr_events_emitted_lock, so the dict mutation is serialised. - All three call sites of
_clear_pipeline_runtime_state(pipeline_create at1955, terminal PATCH at2279, pipeline_delete at2454) now also clear the dedupe set, including the primary eviction site for auto-FAILED prior runs. dict.pop(key, None)cannot raise (the default makesKeyErrorimpossible) so the outertryis purely consistent-with-surrounding-blocks defence — that's fine, the pattern reads cleanly.- The new test seeds the set, calls the real
_clear_pipeline_runtime_state, and asserts the entry is gone. It uses a unique pipeline id (issue-2599-test, doesn't collide with the existingissue-2053-real-clear-test) and clears defensively at the top so a previously-failed run can't leak state. Ran locally: passes.
No blocking issues. Ship it.
— Authored by egg
|
egg review completed. View run logs 8 previous review(s) hidden. |
… bus (#2621) * Fix #2611: route context_pr.{skipped,failed} through message store + event bus PR #2599 wired report_pipeline_status emits for the context-PR hook under the assumption that operators using `wait-status` and `recent_messages` would see them. They did not: report_pipeline_status dispatches to a StatusReporter handler chain that no production code registers, and the wrapper never touched the message store or event bus, so the signal was orchestrator-log-only despite the in-code docstring claim to the contrary. Wire two additional sinks in `_maybe_open_base_pr_for_plan_to_implement` so the operator-visibility promise is actually delivered: - `message_store.add_message` writes a `CONTEXT_PR_SKIPPED` / `CONTEXT_PR_FAILED` entry — picked up by `recent_messages` (`get_messages_with_meta`) and `/pipelines/<id>/messages`. - `_emit_pipeline_event` publishes a typed `EventType.CONTEXT_PR_*` to the EventBus — picked up by `/status/wait` (allowlist updated) and SSE subscribers. Both are best-effort; the swallow-all wrapper contract (#2548 D3) is preserved by giving each sink its own `try/except`. The dedupe set guards all three sinks so a second wrapper invocation on the same pipeline does not double up `recent_messages` entries or wake `wait-status` twice. Also rewrite the now-accurate docstring/comments at lines 10523-10541, 10575-10588, and 2084-2091, and restore the `recent_messages` / `wait-status` description in `docs/reference/orchestrator-cli.md` (removing the "tracked as #2611" follow-up note). Tests in `test_context_pr_transition_paths.py` add a `TestObservabilitySinks` class that pins all three sinks live (no `report_pipeline_status` patch) and asserts the dedupe covers them. Authored-by: egg * Address PR #2621 review feedback - Pin Message.phase to "plan→implement" sentinel so all four transition paths produce the same phase value on message-store entries (review item 1). - Add ordering trade-off comment near the dedupe block to record why ``already.add(event_type)`` must run before the sinks even though it makes a transient sink failure permanent (review item 2). - Strengthen ``test_message_store_failure_does_not_strand_transition`` to assert sinks 1 (``report_pipeline_status``) and 3 (``_emit_pipeline_event``) still fire when sink 2 raises — pins the three-sink isolation property against a future refactor that collapses the try/except blocks (review item 3). - Add ``test_event_bus_dispatch_reaches_real_eventbus`` that subscribes to a sync ``EventBus`` and exercises the wrapper → ``_emit_pipeline_event`` → ``emit_event`` → ``EventBus.publish`` → subscriber chain end-to-end (review item 4). - Drop redundant ``EGG_MESSAGE_STORE_BACKEND`` env var from ``fresh_message_store`` fixture — the explicit-instance ``monkeypatch.setattr`` bypasses ``_create_message_store`` so the env var is never read (review item 5). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
Summary
The context PR (#2548) is the structural base PR for the slice stack — slice-1 stacks on
egg/<pipeline>/context, every later slice on its parent, and only the context PR merges into main/deploy. When the hook fails to open it, the slice stack has no path to the base branch.#2548 wired
_open_context_pr_for_pipelineinto only the inline_run_pipelineauto-advance path. Operators clearing the plan gate via theadvance_phaseREST/MCP handler or the HITL-approval recovery instart_pipelinesilently bypassed the hook — pipelineissue-2474hit exactly this:contract.pr.context_branchwas null, slice-1's base resolver fell back topipeline_branch(/work), and slice PRs landed on/workwith no PR tomain.This PR routes every plan→implement transition through a shared helper and adds an implement-entry backstop so future transition paths inherit the hook automatically.
_maybe_open_base_pr_for_plan_to_implementwrapper that owns the CUSTOM-mode guard, swallow-all-exceptions semantics, and asourcekwarg for logging.advance_phaseREST (routes/phases.py), the HITL-approval recovery instart_pipeline(also adds the missing_populate_contract_from_plan_safecall on that path socontract.pris populated before the hook fires), and an implement-entry backstop at the top of the IMPLEMENT phase handler. The innercontext_pr_numbershort-circuit keeps multiple invocations safe.Context PR hook enteredlog line at the top of_open_context_pr_for_pipeline, before any short-circuit, so the gap is detectable in logs without grepping per-short-circuit strings.base_branchbut nocontext_pr_numberafterwards), emit acontext_pr.skippedorcontext_pr.failedmessage on the pipeline message bus so operators usingwait-status/get_statussee it without grepping orchestrator logs.Test plan
make lint(clean — only pre-existing soft-cap warnings)make test— 17,236 passed, 41 skippedorchestrator/tests/test_context_pr_transition_paths.py(12 tests) covers: CUSTOM-mode skip, source-kwarg propagation, exception swallow, "hook entered" log line on idempotent skip, message-buscontext_pr.skipped/context_pr.failedemission, no emission for local-mode pipelines, and textual checks that the helper is called from all four expected sites (auto-advance,advance_phaseREST, HITL resume, implement-entry backstop).test_context_pr.pyregression checks updated to assert the wrapper-level shape since the per-call-site guard moved into the wrapper. All 45 prior tests still pass.Notes
issue-2474pipeline is not unstuck by this change — the operator should manuallygh pr create --base main --head egg/issue-2474/work(or/contextonce one exists) to give the existing slice stack a path to main.Closes #2593.