feat(observability): correlation envelope across orchestrator logs, TaskEvents, agent OTel (#245)

[krokoko]

What

Phases 0–2 of #245 — make the {task_id, user_id, repo, trace_id} correlation envelope observable on every plane, so a task's actions join across CloudWatch logs, TaskEvents, and the X-Ray trace without manual stitching.

Builds on #515, which already persists otel_trace_id on the TaskRecord (the durable orchestrator↔agent bridge).

Why

Vision tenet 7 (observable, attributable, replayable). Answers "who triggered this, on which repo, in which task?" for incident response, feeds #237's audit correlation block from real propagated context, and lets #215 Bedrock cost logs join to spans by shared trace_id/task_id.

Changes

Phase 0 — contract

• docs/design/OBSERVABILITY.md: "Correlation envelope" section — field table (repo/trace_id/session_id nullable), shared snake_case naming with feat: Bedrock cost attribution — session tags, request metadata, and operator FinOps guidance #215/RFC: Governance planes — analytics and compliance export #237/RFC: Identity propagation — composable credential delegation via AgentCore Identity Token Vault #249, and the join model: the TaskRecord bridges the two planes; there is no single W3C trace root because the orchestrator runs before the agent trace exists. Starlight mirror synced.

Phase 1 — orchestrator (TS)

• logger.ts: logger.child(context) merges a persistent envelope into every line (per-call data wins on collision).
• orchestrate-task.ts + shared/orchestrator.ts: lifecycle functions (orchestrate handler, finalizeTask, hydrateAndTransition, failTask) log via a child logger, so admission→terminal lines carry {task_id, user_id, repo}.
• emitTaskEvent gains an optional EventCorrelation param; lifecycle events stamp user_id/repo as top-level fields (repo omitted for repo-less workflows, feat(tasks): capability-driven tasks — declarative steps, non-coding workflows, migrate existing task types #248).

Phase 1 — agent (Python)

• progress_writer.py: _ProgressWriter takes user_id/repo; _put_event stamps {user_id, repo, trace_id} (trace_id read per-event from the active span) — the agent event stream now joins to X-Ray. Optional fields omitted, never null.
• observability.py: set_session_id propagates user.id/repo.url baggage, guarding each field (empty/None not stamped).
• pipeline.py: root task.pipeline span gains user.id; both writer sites pass the envelope. server.py: propagate even when session_id is empty.

Phase 2 — no new code. #515's TaskRecord.otel_trace_id + the Phase 1 event trace_id stamping give the cross-plane join. Both _ProgressWriters are constructed inside an active span, so trace_id is always valid.

Acceptance criteria (#245)

• Correlation contract documented (source + Starlight sync)
• {task_id, user_id, repo, trace_id} in orchestrator logs, admission→terminal
• Agent OTel spans carry the envelope; baggage survives between phases
• TaskEvents joinable to logs + trace by task_id/trace_id
• Tests for context injection
• No regression to feat(security): per-session IAM scoping — session-tagged AssumeRole, DynamoDB leading-key conditions, Bedrock ARN allowlist #209 SessionRole isolation (user_id is read-only here)

Deferred to child issues

• feat(observability): Phase 3 — downstream attribution (GitHub PR + external API correlation) #555 — Phase 3: downstream attribution (GitHub PR body/comments + external webhook/API correlation headers). Separable, touches the integration handlers, and has its own design questions.
• feat(observability): Claude Code native OTel events + operator runbook #556 — Claude Code native OTel events + operator runbook (per-model-call cost/token forensics; gated on ADOT sidecar OTLP-logs forwarding).

Testing

• New: logger child-merge (4), orchestrate-task envelope incl. repo-less (3), progress_writer envelope incl. omit-not-null (4), set_session_id incl. empty session (3), _run_task_background envelope propagation + trace-read fail-open (2).
• CDK 2236 pass + synth clean; agent 1182 pass; eslint / ruff / tsc / docs-sync clean.

Live verification (deployed, dev)

Three tasks submitted post-deploy, one per scenario. Values below are redacted — user_id shown as <user> (same Cognito sub across all three), repo owners masked, trace ids shortened to an 8-hex prefix (the prefix equality is the point).

Scenario	Task status	user_id on events	repo on events	trace_id on agent events
Repo-less workflow	COMPLETED	<user> ✅	absent ✅ (omitted, not "")	6a4ea846… ✅
Code repo	COMPLETED	<user> ✅	<owner>/agent-plugins ✅	6a4ea5de… ✅
Fail-fast (pre-flight)	FAILED	<user> ✅	<owner>/agent-plugins ✅	n/a (failed before agent ran)

Per-plane checks

• Orchestrator log lines carry the envelope (not just events): Session started / Task already in terminal state lines show user_id + repo (repo omitted for the repo-less task) — confirms the logger.child wiring.
• TaskEvents: user_id present on every event from hydration_started onward; pre-span events (task_created) correctly have trace_id absent — matching the documented "read per-event, omitted before the root span opens" behavior.
• Fail-fast path: both preflight_failed and task_failed carry user_id + repo, confirming the failTask correlation threading works on the failure path (review finding Installation: docker image inspect fails #1).
• Repo-less contract (feat(tasks): capability-driven tasks — declarative steps, non-coding workflows, migrate existing task types #248): repo is genuinely absent (not null/"") on both the events and the TaskRecord.

Cross-plane join (the core deliverable) — agent-event trace_id exactly equals the TaskRecord.otel_trace_id (#515 bridge) on both completed tasks:

Task	TaskRecord.otel_trace_id	agent event trace_id	match
Repo-less	6a4ea846…	6a4ea846…	✅
Code repo	6a4ea5de…	6a4ea5de…	✅
Fail-fast	null (no agent trace)	—	✅ (joins by task_id/user_id instead)

Net: from any plane you can reach the others — orchestrator log → task_id → TaskRecord → otel_trace_id → agent events/X-Ray trace.

[krokoko]

Update — API passthrough follow-up + re-verified live

Since the initial Phases 0–2 work, folded in the independent-review follow-ups and re-ran the live suite against a fresh deploy.

What was added (commit c3f31493)

1. Correlation envelope surfaced through the event API (Installation: docker image inspect fails #1). The {user_id, repo, trace_id} envelope was stamped on TaskEvents in DynamoDB but stripped by get-task-events.ts / get-task-replay.ts, so bgagent watch/events/replay never saw per-event correlation. Added the three optional fields to EventRecord + ReplayEvent (cdk) and TaskEvent + ReplayEvent (cli mirror), and pass them through per-event (omit-when-absent) in both handlers.
2. Coverage docs (Docs: specify that you can't use the agent with the canned repo #2) — OBSERVABILITY.md notes task_created predates the envelope (joins by task_id) and that CLI consumers can join at task level via TaskRecord.otel_trace_id.
3. Doc wording (Docs: user guide hard-codes us-east-1 #3) — repo documented as "absent (key omitted, not null/"")" for repo-less workflows, matching the implementation.
4. Consistency (feat: add FargateAgentStack as alternative compute backend #4) — failTask + loadBlueprintConfig now use logger.child() like the rest of the lifecycle code.
5. API_CONTRACT.md documents the per-event envelope; types-sync gate passes.

Reviewed with the PR-review plugins (code-review, silent-failure, type-design): no critical/important issues, no silent-failure or data-integrity risks, cdk↔cli mirror exact.

Live verification (re-run on deployed dev)

Three tasks submitted, one per scenario. Redacted: user_id → <user>, repo owners masked, trace ids shown as 8-hex prefix (prefix equality is the point).

DDB stamping (TaskEvents):

Scenario	Status	user_id	repo	trace_id (agent events)
Repo-less	COMPLETED	<user> ✅	absent ✅	6a4eb357… ✅
Code repo	RUNNING	<user> ✅	<owner>/agent-plugins ✅	6a4eb380… ✅
Fail-fast (pre-flight)	FAILED	<user> ✅	<owner>/agent-plugins ✅	n/a (failed before agent ran)

task_created carries no envelope on all three; everything from hydration_started onward carries user_id/repo; agent events add trace_id. Fail-fast: both preflight_failed and task_failed carry user_id + repo.

API passthrough (invoked the GetTaskEvents / GetTaskReplay Lambdas with owner-scoped payloads):

• GET /tasks/{id}/events — repo-less: task_created has no envelope, session_started carries user_id (repo absent), agent events carry user_id + trace_id. Code repo: same plus repo. Omit-when-absent confirmed on the wire (keys genuinely absent, not null).
• GET /tasks/{id}/replay — bundle returned otel_trace_id, events carried the envelope, and the join assertion bundle.otel_trace_id === event.trace_id passed.

Cross-plane bridge — on the completed repo-less task, agent-event trace_id == TaskRecord.otel_trace_id == replay-bundle otel_trace_id (all 6a4eb357…). The code-repo task was still RUNNING, so its otel_trace_id isn't persisted on the TaskRecord yet (written at terminal) — expected; its events already carry the trace id, so the bridge completes on finalize. Fail-fast: otel_trace_id null (no agent trace), events join by task_id/user_id.

Net: the envelope stamping and the newly-deployed API passthrough both work end-to-end. Verified via direct Lambda invoke (no bgagent binary in the verification env; the handler + auth path are exercised identically to a real API call).

[ayushtr-aws]

Review — correlation envelope across orchestrator logs, TaskEvents, agent OTel

Verdict: well-executed — approve with minor comments. The design is documented before the code, the omit-not-null contract is enforced consistently at every write and read site, and tests cover the new branches including failure paths. I verified coverage against the branch: all 9 emitTaskEvent call sites in orchestrator.ts and all 3 in orchestrate-task.ts carry correlation (task_created is written in create-task-core.ts and is documented as pre-envelope), and both _ProgressWriter construction sites (pipeline.py, runner.py) pass the envelope.

Strengths

• current_otel_trace_id hardening is thoughtful: the new try/except exists specifically because callers read it inside the DDB-write try-block, where a tracer fault would be misclassified as a DDB failure and trip the progress circuit breaker. The comment explains this and there's a dedicated regression test.
• Consistent optional-field contract: if value: guards on the Python write side, ...(x && { x }) spreads on the TS side, and tests assert key absence at every layer — writer, emitTaskEvent, API pass-through, and replay bundle.
• The widened trigger in server.py (if session_id or user_id or repo_url:) correctly handles the empty-session case, and set_session_id now guards each field so an empty session_id no longer writes empty-string baggage — with a test for exactly that branch.
• Durable-execution awareness preserved: correlation was added without disturbing the transition-gated emit logic or the duplicate-event-on-retry protections.
• logger.child is minimal and correct: immutable merge, per-call data wins, composable, base logger unaffected — all four properties tested.
• The cross-plane join holds by construction: the agent's repo_url is fed from task.repo (orchestrator.ts payload assembly), so agent-event repo values match orchestrator-event repo values exactly.

Minor issues

1. set_session_id is now misnamed (agent/src/observability.py). It propagates the full correlation envelope, not just the session id — server.py even calls it when session_id is empty. Consider renaming (e.g. propagate_correlation_context); as-is, a reader seeing set_session_id("") has to go read the docstring to understand why that's not a bug.

2. Stray // ponytail: marker (cdk/src/handlers/shared/logger.ts:40). The comment content is useful; the prefix looks like a leftover codename/typo and should go.

3. loadBlueprintConfig's child logger omits user_id (cdk/src/handlers/shared/orchestrator.ts). It builds logger.child({ task_id, ...repo }) while every other lifecycle function includes user_id, and task.user_id is in scope. This leaves the blueprint-load log lines (including the poll_interval_ms clamped warning) short of the "admission→terminal lines carry {task_id, user_id, repo}" acceptance criterion. One-token fix.

4. Envelope-construction duplication in TS: logger.child({ task_id, user_id, ...(task.repo && { repo: task.repo }) }) plus a parallel EventCorrelation literal is hand-rolled in four places. A small envelopeFor(task) helper returning { log, correlation } would prevent the drift that already produced item 3.

5. Type asymmetry in Python params: user_id: str = "" vs repo: str | None = None in both set_session_id and _ProgressWriter. It mirrors the upstream config types, but mixing absent-value conventions ("" vs None) within one envelope invites mistakes; a short comment on why they differ would help.

6. Test brittleness (very minor): test_sets_all_envelope_fields_when_present asserts the exact order of baggage keys. Order isn't part of the contract; a set/sorted comparison would survive harmless reordering.

Security / risk

• user_id (Cognito sub) now lands in CloudWatch logs and DDB items — an opaque UUID, and attribution is the explicit purpose of the feature. The API handlers only return events for tasks the caller is authorized on, so no new exposure surface.
• The broad except Exception in current_otel_trace_id is a silent-success mask, but it's narrowly scoped, justified in docstring + nosemgrep annotation, and tested.
• No schema/infra changes: DDB items gain optional attributes only, so old readers and pre-envelope items are unaffected (explicit back-compat test for the historical item shape).

Test coverage

Strong: logger child-merge semantics, lifecycle event correlation including repo-less omission, emitTaskEvent back-compat, _ProgressWriter omit-not-null in all combinations, tracer fail-open, empty-session baggage propagation, and API pass-through in both events and replay handlers. The one seam not directly unit-tested is failTask's new repo param on the successful-transition path, but it's exercised transitively and via the live fail-fast verification in the PR description.

None of the above is blocking; items 1–3 are worth a quick follow-up commit before merge.

[isadeks]

Approve with nits ✅

Clean, well-executed observability plumbing. Threads the {task_id, user_id, repo, trace_id} envelope across orchestrator logs (logger.child), TaskEvents (EventCorrelation on emitTaskEvent + agent-side _ProgressWriter stamping), and OTel baggage/spans. Strong test suite + live dev verification of the cross-plane trace_id == TaskRecord.otel_trace_id join. No correctness bugs or regressions found — the findings below are all minor.

Verified clean (the subtle bits)

• Spread patterns omit correctly — reproduced ...(correlation?.user_id && {…}) and the get-task-events passthrough in Node: empty-string/undefined fields are cleanly omitted, never stamped as ""/null, matching the "omitted, not null" contract.
• No PII leak to external channels — the TaskEvents NEW_IMAGE stream feeds FanOutConsumer (Slack/GitHub/email), but parseStreamRecord whitelists fields (task_id, event_id, event_type, timestamp, metadata); the new top-level user_id/repo/trace_id (incl. the Cognito sub) are not forwarded.
• Trace-read can't trip the circuit breaker — trace_id = current_otel_trace_id() is inside _put_event's try-block, but the function now swallows all exceptions → None, so a tracer fault won't be misclassified as a DDB failure.
• All call sites consistent — all 9 failTask calls got task.repo (optional param → back-compat); all 12 emitTaskEvent calls well-formed; finalizeTask's userId param and task.user_id reference the same (immutable) task; failTask's catch-scoped log is used only in-scope with the correct userId.
• set_session_id has one guarded caller (server.py, if session_id or user_id or repo_url) on a single-task thread → the never-detached context.attach is genuinely thread-scoped.

Findings (all minor)

🟡 1. loadBlueprintConfig's child logger omits user_id — cdk/src/handlers/shared/orchestrator.ts:222

const log = logger.child({ task_id: task.task_id, ...(task.repo && { repo: task.repo }) });

Every other function converted in this PR (hydrateAndTransition, finalizeTask, failTask, the durableHandler) includes user_id: task.user_id, and it's available on the TaskRecord here too. The comment even says this logger is "matching the orchestrate handler's child logger" — but the handler's carries user_id and this one doesn't. So the three blueprint-load lines won't surface in a CloudWatch Insights query filtered on user_id, defeating the per-line-envelope goal for this function. Not a regression, but the one I'd actually fix (one line: add user_id: task.user_id).

🟡 2. repo ""-vs-None contract drift — agent/src/pipeline.py:692 + runner.py:490 pass repo=config.repo_url (which is "", never None, for repo-less tasks), while server.py:460 normalizes repo=repo_url or None, and _ProgressWriter's comment says "repo is None for repo-less workflows." Harmless today (_put_event guards if self._repo:, so "" and None both omit the key), but the stored contract is violated and the three producers disagree — a latent trap for any consumer that distinguishes is None from == "". Suggest config.repo_url or None in pipeline/runner.

⚪ 3. Logger clobber footgun (latent only) — logger.ts serializes { level, message, ...context, ...data }, so a caller-supplied data.message/data.level would override the positional args. No current caller does this, and the ordering matches the pre-PR logger, so there's no active bug — just a sharp edge worth a comment.

⚪ 4. progress_writer now eagerly imports opentelemetry (posture note) — the new from observability import current_otel_trace_id forces an eager opentelemetry import at module-load, and the docstring advertises fail-open import safety. But that safety is specifically about boto3 (lazily imported); observability/opentelemetry is already eagerly imported by pipeline.py:29 and is a hard prod dep (aws-opentelemetry-distro==0.18.0), so practical risk ≈ nil. Noting only for completeness.

Bottom line

Solid, low-risk, no blockers. #1 is the only one worth changing before merge (restores the PR's own invariant); #2 is a nice consistency tidy; #3/#4 are optional.

Findings verified against the feat/245-observability-attribution checkout, including direct Node repro of the spread patterns and a trace of the fanout stream field-extraction.

[krokoko]

Thanks for the thorough review — all 6 items addressed in f560cf33:

1. Renamed set_session_id → propagate_correlation_context (+ sole caller in server.py and tests). It reads correctly now when called with an empty session_id.
2. Dropped the stray // ponytail: prefix in logger.ts (kept the explanatory content).
3. loadBlueprintConfig now carries user_id on its child logger — fixed via item 4.
4. Extracted envelopeFor(task) → { log, correlation } in orchestrator.ts as the single source for the log child and the event correlation. Replaces the hand-rolled logger.child(...) + EventCorrelation literal across all 5 sites (orchestrate handler, hydrateAndTransition, finalizeTask, failTask, loadBlueprintConfig) — removing the drift that produced item 3. Added unit tests asserting the child carries user_id (incl. repo-less omission).
5. Added a comment on the ""-vs-None param asymmetry in propagate_correlation_context (they mirror the upstream config types; the if x: guards flatten both).
6. Set-compare on baggage keys in the observability test instead of exact order.

On the one seam you flagged as not directly unit-tested — failTask's successful-transition repo path — it's still covered transitively + via the live fail-fast run; I left it as-is since it's exercised, but happy to add a direct assertion if you'd prefer.

CDK 2258 pass + synth clean, agent 1182 pass, eslint/ruff/tsc all clean.

[theagenticguy]

Approve. Reviewed the full diff and spot-verified the load-bearing claims against the source rather than trusting the description:

• logger.child merge precedence is correct — { level, message, ...context, ...data } spreads per-call data last, so it wins on collision as documented.
• Omit-not-null is done right on both planes: TS ...(task.repo && { repo }) and Python guards (if user_id: / if repo: / if trace_id:). Absent fields are omitted, never null/"" — important for the repo-less contract (#248).
• task.repo is threaded through the failure path (failTask), so preflight_failed/task_failed also carry correlation.

Tests cover the new paths (logger child-merge, orchestrate-task envelope incl. repo-less, progress_writer omit-not-null, set_session_id empty-session, background trace-read fail-open), CI is green across the board, and scope is cleanly deferred to #555/#556. Nice, well-documented change. 🤖