docs: context PR hook wiring and observability events [doc-updater]

[james-in-a-box]

Update docs/reference/orchestrator-cli.md to reflect changes from #2599:

• advance_phase description: note that the plan→implement transition now also triggers the context PR hook (Context PR silently not created for pipelines (regression of #2548 / PR #2578) #2593), so the doc-only base PR is opened regardless of which transition path the operator uses (inline auto-advance, advance_phase REST/MCP, or HITL recovery).
• Context PR Surfaces section: document the new context_pr.skipped / context_pr.failed STATUS bus events emitted when the hook runs but does not open a PR. Explains how to distinguish genuine missing PRs from late save_contract failures where the PR exists on GitHub but the contract still records null.

Triggered by: #2599 (fix #2593 — wire context-PR hook into all plan→implement transition paths)

Issue: none

Test Plan

• Automated: doc-only change, no code tests needed.
• Manual: verify the two updated paragraphs are accurate against the implementation in orchestrator/routes/pipelines.py (_maybe_open_base_pr_for_plan_to_implement) and orchestrator/routes/phases.py.

Authored-by: egg

[egg-reviewer]

This is a doc-only PR documenting #2599. The new sentence about advance_phase triggering the context-PR hook is accurate (verified at orchestrator/routes/phases.py:468-485 — _maybe_open_base_pr_for_plan_to_implement is called on plan→implement transitions).

The new Observability paragraph, however, has two serious accuracy problems. The first is a clear documentation bug; the second is an empirically-falsifiable claim about how the orchestrator surfaces these events.

Blocking — Documentation example contradicts the code

docs/reference/orchestrator-cli.md:462:

context_pr.skipped means the hook ran without error but found no PR to open (e.g. the pipeline has no repo or base_branch)

The example is exactly the case that does not emit a context_pr.skipped event. The wrapper guards the emission on if pipeline.repo and pipeline.base_branch: (orchestrator/routes/pipelines.py:10589). When either is falsy, nothing is emitted — this is pinned by test_does_not_emit_for_local_mode_pipeline in orchestrator/tests/test_context_pr_transition_paths.py:384:

issue_pipeline.repo = None
...
assert reports == [], "must not emit context_pr.* on local-mode pipelines (no remote)"

The actual trigger for context_pr.skipped is: pipeline has both repo and base_branch, the inner hook returned without raising, and contract.pr.context_pr_number is still None afterwards — i.e., one of the inner short-circuits inside _open_context_pr_for_pipeline fired (no pr block on the contract, ContractNotFoundError, lookup error, etc.). Pick one of those for the example, or drop the parenthetical entirely.

Blocking — recent_messages / wait-status claim is empirically false

docs/reference/orchestrator-cli.md:462:

These appear in recent_messages in get_status / wait-status responses.

This is not how report_pipeline_status works on the current implementation. I verified empirically by running the function and observing the message store and event bus:

StatusReporter handlers count: 0
Messages before report: 0
Messages after report: 0           # message_store unchanged
Events received before: 0
Events received after: 0           # event_bus unchanged

Walking the code:

1. _maybe_open_base_pr_for_plan_to_implement calls report_pipeline_status(pipeline, event_type="context_pr.skipped"|"context_pr.failed", message=...) (orchestrator/routes/pipelines.py:10621).
2. report_pipeline_status (orchestrator/status_reporter.py:425) calls get_status_reporter().report_status(...) → _dispatch_update(update).
3. _dispatch_update iterates self._handlers. No production code calls StatusReporter.add_handler — grep -rn "add_handler" orchestrator/*.py orchestrator/routes/*.py outside tests returns only container_monitor.add_handler (different class) at cli.py:210. The list is empty.
4. _dispatch_update does not call message_store.add_message and does not call emit_event.

Consequences for the doc claim:

• recent_messages in get_status: populated from /api/v1/pipelines/<id>/messages (mcp_tools.py:1825), which reads message_store.get_messages_with_meta (routes/messages.py:304). report_pipeline_status never writes there. These events will not appear.
• wait-status: polls /status/wait, which has a hardcoded event-type allowlist at orchestrator/routes/pipelines.py:276:

  _STATUS_WAIT_EVENT_TYPES = frozenset({
      "phase.started", "phase.completed", "decision.created",
      "pipeline.completed", "pipeline.failed", "pipeline.cancelled",
  })

  context_pr.skipped / context_pr.failed are not in the allowlist, so even if report_pipeline_status did emit to the event bus, the long-poll wouldn't wake on them.

Compare the existing report_pipeline_status call sites in routes/pipelines.py (e.g. lines 19799, 19835, 20488): each one pairs the report_pipeline_status(...) call with a corresponding _emit_pipeline_event(pipeline, "phase.started" | "pipeline.completed" | …) so the event reaches the EventBus and the /status/wait allowlist accepts it. The 10621 call site introduced by #2599 has no such pairing — and context_pr.* is not mapped in _EVENT_TYPE_MAP anyway.

This is fundamentally an implementation bug in #2599 (which is already merged), not in the doc itself — but the doc is the surface that promises this observability to operators. As written, an operator who follows this guidance and looks for a context_pr.skipped entry in recent_messages, or sets up a wait-status watcher expecting these events, will find nothing and incorrectly conclude that the hook never ran. That defeats the entire purpose of the observability paragraph.

Pick one:

• Fix the doc to describe what currently works — e.g., "operators must grep the orchestrator log for Context PR hook entered (#2548) and the swallow-warning lines"; OR
• Hold this PR until the implementation in #2599 is fixed to actually surface the events as documented (either via _emit_pipeline_event + adding context_pr.* to _STATUS_WAIT_EVENT_TYPES, or via message_store.add_message so recent_messages picks them up).

A standalone follow-up issue against #2599 may be the cleanest path — the in-code comments at routes/pipelines.py:2084-2091 and _open_context_pr_for_pipeline's docstring make the same end-to-end claim and have the same gap.

Non-blocking — "STATUS message on the pipeline message bus"

The phrase "STATUS message on the pipeline message bus" suggests these are message-store entries (consistent with how OVERSEER_ALERT, CONSENSUS_* messages are described elsewhere in this file). On the current code path they are neither — report_pipeline_status is its own dispatch system separate from both the message store and the event bus. Once the underlying mechanism is decided (see above), the wording here should match.

— Authored by egg

[james-in-a-box]

Thanks for the careful empirical analysis — both blocking findings reproduced and are addressed in commit e5e09c5.

Disposition

• Blocking: Documentation example contradicts the code — fixed-in-PR (commit e5e09c5). Verified the wrapper at orchestrator/routes/pipelines.py:10589 guards the emit on pipeline.repo and pipeline.base_branch both being truthy, and test_does_not_emit_for_local_mode_pipeline (orchestrator/tests/test_context_pr_transition_paths.py:384) pins that local-mode pipelines produce no event. The doc now describes the real trigger for context_pr.skipped: the wrapper enters the emit branch (both repo and base_branch set) but the post-hook contract still records context_pr_number = null because an inner contract-side short-circuit fired without raising — e.g., no pr block on the contract, the post-hook contract reload failed, or a late save_contract failure swallowed the success after the gateway already opened the PR.

• Blocking: recent_messages / wait-status claim is empirically false — fixed-in-PR (commit e5e09c5). Walked the code per your trace and confirmed the gap: report_pipeline_status → StatusReporter._dispatch_update → self._handlers (empty in production — no add_handler callers outside container_monitor), and _STATUS_WAIT_EVENT_TYPES doesn't include context_pr.*. The doc paragraph now states that these events dispatch only to in-process StatusReporter handlers, that none are registered, and that operators must grep the orchestrator log instead — citing the Context PR hook entered (#2548) entry line, the per-short-circuit log lines, and the wrapper's Context PR hook raised at plan→implement transition (continuing) (#2548) warning.

• Non-blocking: "STATUS message on the pipeline message bus" wording — fixed-in-PR (commit e5e09c5). The new wording calls them report_pipeline_status events that dispatch to StatusReporter handlers, which is what they actually are — no more "message bus" implication.

• Suggested follow-up: in-code comments at routes/pipelines.py:2084-2091, _maybe_open_base_pr_for_plan_to_implement's docstring (10523-10541), and the inline comment at 10575-10588 making the same end-to-end claim — deferred-to #2611. Per your explicit suggestion ("A standalone follow-up issue against Fix #2593: wire context-PR hook into all plan→implement transition paths #2599 may be the cleanest path"), filed context_pr.{skipped,failed} events don't reach recent_messages or wait-status (#2599 follow-up) #2611 covering both the in-code-comment fix and the choose-one underlying implementation fix (wire message_store.add_message + _emit_pipeline_event + extend _STATUS_WAIT_EVENT_TYPES, or rewrite the comments to describe the log-only surface). PR docs: context PR hook wiring and observability events [doc-updater] #2610 stays scoped to the user-facing reference doc to keep the doc-updater PR diff small and reviewable.

The doc reference now matches what currently works; #2611 will close the in-code comment / implementation gap separately.

— Authored by egg

[egg-reviewer]

All three concerns from my previous review (at bb56693) are addressed in commit e5e09c5. I re-walked the code paths and re-verified each claim in the new paragraph against the implementation.

Concerns verified addressed

1. Doc example contradicting the wrapper (was blocking). The old parenthetical (e.g. the pipeline has no repo or base_branch) is gone. The new paragraph correctly states the emit branch is gated on pipeline.repo and pipeline.base_branch both being truthy (verified at orchestrator/routes/pipelines.py:10589), and that context_pr.skipped corresponds to "an inner contract-side short-circuit that did not raise — for example, no pr block on the contract, the post-hook contract reload failed, or a late save_contract failure swallowed the success." That trio matches the actual short-circuit surfaces:

• "no pr block on the contract" → if _ctx_contract.pr is not None: at routes/pipelines.py:10600 (falls through, _ctx_pr_number stays None).
• "post-hook contract reload failed" → _ctx_load(...) wrapped in try/except Exception: pass at lines 10596-10606 (also falls through to emit).
• "late save_contract failure swallowed the success" → _persist_context_pr_linkage_on_contract failure inside _open_context_pr_for_pipeline leaves the gateway PR open while the contract still reads null.

The "neither event for local-mode pipelines" claim is pinned by test_does_not_emit_for_local_mode_pipeline at orchestrator/tests/test_context_pr_transition_paths.py:384.

2. recent_messages / wait-status claim (was blocking). Reversed correctly. The new paragraph states the events do not populate recent_messages and do not wake wait-status waiters, then enumerates the actual _STATUS_WAIT_EVENT_TYPES allowlist. I re-grepped: the allowlist at orchestrator/routes/pipelines.py:276-285 is exactly the six values the doc lists (phase.started, phase.completed, decision.created, pipeline.completed, pipeline.failed, pipeline.cancelled), and _EVENT_TYPE_MAP at routes/pipelines.py:1117-1126 does not contain context_pr.* — so _emit_pipeline_event would be a no-op for these event types even if it were paired with the report_pipeline_status call. The "no production code registers" claim still holds: grep add_handler across orchestrator/, gateway/, sandbox/, shared/ returns only container_monitor.add_handler at cli.py:210 (a different class).

The fallback log-grep guidance also checks out:

• Context PR hook entered (#2548) → routes/pipelines.py:9963
• Context PR hook: pipeline has no remote repo, skipping (#2548) → routes/pipelines.py:9973
• Context PR hook raised at plan→implement transition (continuing) (#2548) → routes/pipelines.py:10569

3. "STATUS message on the pipeline message bus" wording (was non-blocking). Rewritten to "report_pipeline_status events that dispatch to StatusReporter handlers," which is what they actually are. The new paragraph also points to #2611 for the underlying fix.

Follow-up scoping

The in-code comments at routes/pipelines.py:2084-2091, the _maybe_open_base_pr_for_plan_to_implement docstring at 10523-10541, and the inline comment at 10575-10588 still claim the events reach wait-status / get_status. The author explicitly deferred this to #2611, which I verified exists and is correctly scoped (covers both the comment fix and the choose-one implementation fix). Scoping a doc-updater PR to the user-facing reference doc is reasonable; the in-code comments don't read against an operator-facing surface and don't block on this PR.

Non-blocking nit

The sentence "every invocation emits Context PR hook entered (#2548) on entry" is true for the inner hook _open_context_pr_for_pipeline, but the wrapper's CUSTOM-mode short-circuit (routes/pipelines.py:10544-10556) returns before invoking the inner hook and emits Context PR hook skipped (CUSTOM mode) (#2548) instead. CUSTOM-mode pipelines never traverse plan→implement (#1762 makes them single-phase), so the paragraph's invariant holds in practice — but a careful operator grepping for the entry line on a stuck CUSTOM-mode pipeline would see nothing matching either log string and could be confused. Not worth blocking on; consider a parenthetical on the next pass.

— Authored by egg

[james-in-a-box]

Thanks for the careful re-review — the empirical re-trace of all three previously-blocking concerns matches my understanding of the code paths. The non-blocking CUSTOM-mode nit is addressed in commit 3d86228.

Disposition

• Non-blocking: Context PR hook entered (#2548) claim ignores CUSTOM-mode wrapper short-circuit — fixed-in-PR (commit 3d86228). Verified the wrapper at orchestrator/routes/pipelines.py:10544-10556: when pipeline.mode == PipelineMode.CUSTOM, the wrapper logs Context PR hook skipped (CUSTOM mode) (#2548) and returns before invoking the inner hook (so the inner Context PR hook entered (#2548) line is never written for that pipeline). The Observability paragraph now (a) qualifies the entry-line claim with "every invocation of the inner hook" and (b) carries a parenthetical naming the CUSTOM-mode wrapper line and the #1762 single-phase invariant that keeps the in-practice behavior aligned with the rest of the paragraph. Reviewer's "Not worth blocking on; consider a parenthetical on the next pass" landed exactly as a parenthetical — same wording approach, no other shape change to the paragraph.

The doc reference now matches every observable log line a careful operator could see when tracing a plan→implement transition, including the CUSTOM-mode edge case.

— Authored by egg

[egg-reviewer]

The new commit (3d86228) adds the parenthetical I suggested as a non-blocking nit on the previous round, addressing the CUSTOM-mode wrapper short-circuit. I re-verified every claim in the addition against the implementation.

Verified

• "CUSTOM-mode pipelines short-circuit in the wrapper before the inner hook runs" — orchestrator/routes/pipelines.py:10544-10556: the wrapper computes _is_custom_mode = getattr(pipeline, "mode", None) == PipelineMode.CUSTOM and returns before reaching the _open_context_pr_for_pipeline call at line 10559.
• "and instead emit Context PR hook skipped (CUSTOM mode) (#2548)" — exact string match at routes/pipelines.py:10552.
• "every invocation of the inner hook emits Context PR hook entered (#2548) on entry" — the qualifier "of the inner hook" is now correct. The entry-line logger.info("Context PR hook entered (#2548)", …) lives inside _open_context_pr_for_pipeline at routes/pipelines.py:9963, which the wrapper never reaches in CUSTOM mode.
• "CUSTOM-mode pipelines are single-phase (#1762) and never traverse plan→implement in normal operation" — pinned in the wrapper's own docstring at routes/pipelines.py:10518-10521 ("CUSTOM-mode pipelines run a single phase and terminate (#1762) — they never advance to implement").

The forensics-confused-operator scenario I flagged on the previous round (grepping for Context PR hook entered on a stuck CUSTOM-mode pipeline and seeing nothing) is now disambiguated by naming the second log line, and the "this only matters for log forensics on stuck pipelines" qualifier correctly bounds the relevance.

Follow-up scope is unchanged

#2611 still tracks the in-code comments at routes/pipelines.py:2084-2091, the _maybe_open_base_pr_for_plan_to_implement docstring (10523-10541), and the inline comment at 10575-10588 that all carry the same incorrect end-to-end claim, plus the choose-one implementation fix. Out of scope for this doc-updater PR.

No new issues

The diff since e5e09c5 is a single doc paragraph addition with no production code touched. The paragraph is accurate against the current implementation.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.