feat(envelope): mutation result envelope via --result-out/--result-fd (AIP Phase 2)

[igor-ctrl]

Summary

Implements Phase 2 of the AIP v0.1 contract — an opt-in JSON result envelope an agent runtime can consume on a side channel, never on stdout. Adds --result-out PATH and --result-fd N to every mutating command:

• bcli post / patch / delete / attach upload / batch run

Envelope shape matches the contract: version, invocation_id, tool_version, profile, environment, company, method, endpoint, resolved_url, record_id, dry_run, status, exit_code, bc_correlation_id, telemetry_event_id, audit_log_offset, started_at, duration_ms.

Why

Agent runtimes (and bcli_mcp) need to parse mutation outcomes deterministically. Today they have to scrape stdout prose — fragile when --format changes, when the user sees [green]Error:[/red] styling, or when the BC correlation id only appears in stderr. The envelope is a stable, atomic, side-channel attestation: profile + environment + company + correlation id + outcome.

This unblocks Phase 5 (the bcli_mcp rewrite that consumes --result-out).

Implementation

• bcli.result_envelope — frozen dataclass + atomic writer (tmp file + os.replace, fsync, mkdir(parents=True)).
• bcli.exit_codes — taxonomy seeded for Phase 4 (Worker A) to extend; Phase 2 only wires 0/1/6/7 today.
• bcli_cli._envelope_wrap — sibling to _audit_wrap. Context-manager lifecycle so the five commands share one implementation: validate flags, capture started_at + invocation_id, build + write the envelope on success or failure.

The envelope is written on both success and failure paths:

• HTTP 4xx → status="failed", exit_code=6, bc_correlation_id captured.
• HTTP 5xx → status="failed", exit_code=7, bc_correlation_id captured.
• Other exceptions → status="failed", exit_code=1.
• --dry-run → dry_run=true, status="succeeded", no HTTP call.

stdout is untouched; the existing --format flag still drives whatever the human / CSV / JSON dump looks like. Stderr too.

record_id is extracted from the response body in order: systemId → id → first *Id field; null when none available.

--result-out and --result-fd are mutually exclusive (typer.BadParameter on conflict).

Both flags are strictly opt-in — when unset, commands behave exactly as before. This is an additive change.

Smoke proof

$ bcli post vendors --data '{"displayName": "Smoke"}' --result-out /tmp/x.json
$ cat /tmp/x.json
{
  "version": "0.1",
  "invocation_id": "3b673fa7c6e743fb816801842f0cd990",
  "tool_version": "0.3.0",
  "profile": "dev",
  "environment": "Sandbox",
  "company": "c-1",
  "method": "POST",
  "endpoint": "vendors",
  "resolved_url": "https://example.test/vendors",
  "record_id": "vnd-real-1",
  "dry_run": false,
  "status": "succeeded",
  "exit_code": 0,
  "bc_correlation_id": null,
  "telemetry_event_id": null,
  "audit_log_offset": null,
  "started_at": "2026-05-17T01:00:51.355583Z",
  "duration_ms": 1
}

Known limitations / follow-ups

1. telemetry_event_id / audit_log_offset are always null. Wiring needs a protocol extension on the telemetry + audit sinks (returning the emitted event id / log offset). Deferred to keep this PR additive — extending those Protocols touches every backend including the optional Azure Monitor extra.
2. Policy violations don't emit an envelope. confirm_write_or_exit raises typer.Exit(1) before the capture(...) block. A read-only profile in non-interactive mode without --yes exits 1 with no envelope. Phase 4 (exit-code taxonomy) defines exit 8 for this case — the right place to wire envelope-on-policy-violation is together with that change.
3. status="succeeded" for dry-run. I chose this over status="dry_run" because the dry-run completed successfully; dry_run=true is the disambiguator. Defensible vs. the spec's example which only shows the happy path. Open to flipping if you'd prefer.
4. OptionInfo tolerance. The wrapper treats typer.OptionInfo placeholders as "not provided" so existing direct-call tests (test_batch_safety.py calls run_batch(file=..., dry_run=..., …) without the new kwargs) keep working. A separate cleanup PR could update every call site; this helper keeps the change contained.

Test plan

• uv run pytest tests/ -v — 667 passed, 5 skipped.
• uv run ruff check src/ tests/ — clean.
• 36 new tests under tests/test_envelope/ (core module + per-verb integration).
• CLI --help shows the new flags on post / patch / delete / attach upload / batch run.
• Live smoke run with mocked HTTP layer produces the envelope shown above.
• Reviewer: spot-check that stdout still matches --format exactly on a real post (no envelope keys leaking).
• Reviewer: confirm mkdir(parents=True, exist_ok=True) for --result-out is OK (matches existing --output flag elsewhere).

Spec / context

• Contract: agent-cli-contract-v0.1.md §Phase 2.
• Work plan: tasks/todo.md Phase 2 checklist.

[igor-ctrl]

Lead review — REQUEST_CHANGES (one issue) + APPROVE the rest

Reviewed against agent-cli-contract-v0.1.md §Phase 2 and tasks/todo.md Phase 2 checklist. Worker self-report verified: 667 passed / 5 skipped, ruff clean, additive only. Beautiful PR — frozen dataclass + os.replace + fsync, mutual-exclusion on the flags, per-verb integration tests, dedicated _envelope_wrap module mirroring _audit_wrap. The ergonomics of with capture(...) as cap: are exactly right.

Verdict on the three flagged items

(1) telemetry_event_id / audit_log_offset always null — APPROVE.
The fields are present in the dataclass and pinned by test_has_all_spec_fields. The spec example in §Phase 2 shows non-null values but the schema permits null for unknown/unavailable. Wiring those needs a TelemetrySink / audit-sink protocol extension that touches every backend including the optional Azure Monitor extra — keeping this PR additive is the right call. Track as a Phase 2.1 follow-up so it doesn't drift indefinitely.

(3) Dry-run uses status="succeeded" + dry_run=true — APPROVE.
Per the contract doc §Phase 2 schema, status and dry_run are independent fields. Your interpretation is correct: a completed dry-run is a success at the "completed the dry-run preview" layer; dry_run=true is the disambiguator. Verified in test_envelope_on_dry_run. No change needed.

(2) Policy violations skip the envelope — REQUEST_CHANGES.

I empirically confirmed this with a readonly Production profile + --result-out:

typer.Exit code=1
NO ENVELOPE WRITTEN (policy gate exited before capture)

The four single-mutation commands (post, patch, delete, attach upload) call confirm_write_or_exit(...) before opening the with capture(...) block, so a policy refusal exits without emitting the envelope. batch_cmd.py already does this correctly — its confirm_write_or_exit("BATCH WRITE", preview, yes=yes) is inside with capture(...) and the envelope captures the policy exit. That asymmetry is the bug.

Why this matters for the AIP contract: the spec says "a write's value is its attestation: profile, environment, target, correlation id, outcome" — a refused write is an outcome. An agent runtime driving production needs to distinguish "BC said no" (4xx → envelope), "BC errored" (5xx → envelope), and "bcli policy refused locally" (no envelope today). The last case is the one a runtime is most likely to encounter when piloting against a disable_writes profile.

You correctly identified Phase 4 will add EXIT_POLICY = 8 to the taxonomy — but the envelope wiring is here, in Phase 2. Phase 4 can flip exit_code=1 → exit_code=8 without re-touching the envelope code path.

Asked fix:

Move the confirm_write_or_exit(...) call inside the with capture(...) block for the four single-mutation commands, mirroring batch_cmd.py (lines ~756 onward). The fix per command is ~3-line reorganization. The pattern:

with capture(method="POST", endpoint=endpoint,
             result_out=result_out, result_fd=result_fd) as cap:
    cap.set_resolved_url(try_resolve_url(...))
    confirm_write_or_exit("POST", endpoint, yes=yes)   # ← now inside; typer.Exit will hit the BaseException branch of capture(), which writes a failed envelope
    ...

Then add one test per verb (e.g. test_post_envelope_on_policy_violation) — readonly profile + no --yes + non-TTY stdin + --result-out → file exists, status="failed", exit_code=1 (Phase 4 will update to 8 along with the taxonomy roll-out).

Smoke test result_out behavior on batch run confirms the pattern works — your existing test_batch_run_envelope_failed_when_any_step_fails already exercises a non-policy failure-on-exit; a sibling policy test would close the loop.

Verification summary

• uv run pytest tests/ → 667 passed, 5 skipped (matches self-report)
• uv run ruff check src/ tests/ → clean
• Envelope schema: 18 fields present per spec ✓
• Atomic write via os.replace ✓ (tested + verified via spy_replace)
• Mutual exclusion --result-out / --result-fd ✓
• Stdout never carries envelope keys ✓ (test_stdout_does_not_contain_envelope)
• fd mode closes the fd ✓ (test_fd_is_closed_after_write)
• record_id extraction: systemId → id → first *Id ✓
• Per-verb integration: post, patch, delete, attach upload, batch run all pinned

Other observations (non-blocking)

• _envelope_wrap.capture() swallows a typer.Exit raised inside the with block to opportunistically emit an envelope — clever for catching forgotten emit_* calls, but also means a deeply-nested typer.Exit(1) from confirm_write_or_exit would get caught if you moved it inside the block. The "salvage envelope on BaseException" path uses int(exc.exit_code) for failed exits, so the envelope will record exit_code=1 automatically.
• _record_id_from's third clause (first key ending in Id) iterates dict insertion order; for Python 3.7+ that's deterministic. Worth a comment but fine.
• Synthetic RuntimeError("N of M steps failed") in batch_cmd.py for the envelope-failure path is an honest pattern given there isn't a single underlying exception. Phase 3's ledger will give a richer answer; this is the right placeholder.

Action items for worker B (next push):

1. Move confirm_write_or_exit inside with capture(...) for post / patch / delete / attach upload (mirror batch_cmd).
2. Add one test_*_envelope_on_policy_violation per verb under tests/test_envelope/.
3. Keep exit_code=1 for now; Phase 4 will rename to 8 when the taxonomy lands.

After this lands, branch feat/mcp-describe-consumer off main (waiting on Phase 1 to merge first) and proceed with Phase 5.

Verdict: REQUEST_CHANGES on policy-envelope, APPROVE everything else. Igor merges once the policy-envelope fix lands.

[igor-ctrl]

Lead re-review of commit 113200e9 — APPROVE

Verified against the REQUEST_CHANGES item from the prior review.

Gate-move pattern (all four verbs)

For each of post_cmd, patch_cmd, delete_cmd, attach_cmd:

• confirm_write_or_exit(...) removed from the pre-capture region.
• Re-inserted inside with capture(...) as cap:, after cap.set_resolved_url(...) (and cap.set_record_id(...) where the command has one).
• Inline comment cites PR #15 review and explains why (salvage path in _envelope_wrap.capture turns the typer.Exit(1) into a status="failed" envelope).

This mirrors the pattern batch_cmd.py already had — confirmed unchanged.

New tests (tests/test_envelope/test_envelope_policy_violation.py)

One class per verb, four tests total:

• TestPostEnvelopeOnPolicyViolation
• TestPatchEnvelopeOnPolicyViolation
• TestDeleteEnvelopeOnPolicyViolation
• TestAttachUploadEnvelopeOnPolicyViolation

Each pins exactly what the review asked for:

1. disable_writes=true profile (new readonly_state conftest fixture) + force_non_interactive autouse → matches the policy-violation path.
2. Verb invoked with --result-out=<tmp>, yes=False.
3. pytest.raises(typer.Exit) → exit_code == 1 (per the note that Phase 4 owns the taxonomy rename to EXIT_POLICY=8).
4. Shared _assert_policy_failure_envelope helper validates envelope on disk, status="failed", exit_code=1, method/endpoint/profile/environment populated.
5. fake_client.<verb>.assert_not_awaited() — proves the gate fired before any HTTP.

Bonus: the patch and delete tests additionally assert env["record_id"] == "vnd-1" after refusal — proves the gate now sits after set_record_id (structural evidence the move worked, not just behavioral).

Local verification (in /tmp/aip-review-15 worktree)

• uv run pytest tests/test_envelope/ -v → 40 passed in 0.33s (36 prior + 4 new).
• uv run ruff check src/bcli_cli/commands/{post,patch,delete,attach}_cmd.py tests/test_envelope/ → clean.
• Worker totals match: 671 passed (was 667 + 4 new), 5 skipped, ruff clean.

Diff scope

6 files, +220/-12. Touches only the four single-mutation commands + tests/test_envelope/conftest.py (new readonly_state fixture) + new test_envelope_policy_violation.py. batch_cmd.py and _envelope_wrap.py not touched — exactly the surface area expected.

After this merges

Branch feat/mcp-describe-consumer off main (waiting on Phase 1 to merge first) and proceed with Phase 5 per tasks/todo.md.

Verdict: APPROVE. Igor merges; I do not.

[igor-ctrl]

Lead re-review of merge commit d1ead8e — APPROVE

Post-conflict resolution validated. PR is MERGEABLE / CLEAN. d1ead8e is the only new commit on top of 113200e after #14 and #16 landed on main.

Integration walk-through (batch_cmd.py vs current main)

• capture(...) wraps the entire run_batch body — confirmed (line 97 onward).
• ledger.start_run sits inside the capture block, after workflow params setup — confirmed (line 166-174, after _validate_step_names). Preserves #16's "intent row before any step" property.
• cap.set_record_id(run_id) — confirmed (line 179). Reuses the existing envelope field with new semantic meaning for BATCH_RUN: the envelope's record_id IS the ledger run id. No schema change; agents can pivot from envelope → bcli batch state <run-id> with one field. Clean.
• Dry-run finalizes both attestations — confirmed (line 189-195): _print_dry_run → ledger.finish_run(run_id, "completed") → ledger.close() → cap.mark_dry_run() → cap.emit_success() → return. No zombie planned ledger row; dry_run=true on the envelope is the disambiguator. The choice is defensible — operators inspecting the ledger see completed for an intentional no-op, which matches the user's mental model better than a dangling running.
• Policy refusal finalizes both as failed — confirmed (line 217-228): try/except typer.Exit around confirm_write_or_exit → compute_run_state derives → override to failed if needed → ledger.finish_run(run_id, "failed") → cap.emit_failure(RuntimeError("BATCH WRITE refused by disable_writes gate")) → re-raise. Both attestations consistent: envelope status=failed, ledger state=failed.
• Mixed-result on happy path — confirmed (line 295-302): synthetic RuntimeError(f"{failed_count} of {len(results)} steps failed") feeds cap.emit_failure → ledger.finish_run(run_id, final_state) where final_state is partially_committed if any succeeded else failed → raise typer.Exit(1). Envelope reports one-line failure; ledger holds the long-form partially_committed detail.
• BaseException ordering — confirmed (line 309-326): on real Exception (not KeyboardInterrupt/SystemExit): cap.emit_failure(e) first, then compute_run_state + finish_run + close. On KeyboardInterrupt/SystemExit: no envelope emit, ledger still finalizes via derivation, re-raise. SIGKILL realism from #16 preserved — Python signal handlers shouldn't conjure a "complete" envelope.
• except typer.Exit: raise — confirmed (line 306-308). Lets the policy-refusal typer.Exit and mixed-result typer.Exit(1) pass through without re-finalization. Important: prevents double-emit on the envelope (the salvage path in capture() is guarded by cap_state.written so even if it tried, it wouldn't double-write).

Integration test (test_batch_envelope_with_ledger.py)

Three scenarios, each asserting envelope ↔ ledger agreement plus the record_id == run_id cross-reference:

1. TestEnvelopeAndLedgerAgreeOnSuccess::test_success_envelope_matches_ledger_run_id — envelope status="succeeded"/exit_code=0/method="BATCH_RUN", ledger derived state completed, env["record_id"] == run_row["run_id"].
2. TestEnvelopeAndLedgerAgreeOnPartialCommit::test_partial_commit_envelope_failed_ledger_partial — Step 1 commits, step 2 raises RuntimeError; envelope status="failed"/exit_code=1, ledger derived state partially_committed, cross-reference preserved.
3. TestEnvelopeOnPolicyRefusalForBatch::test_policy_refusal_emits_failed_envelope_and_failed_ledger — readonly profile + non-interactive + no --yes; envelope status="failed"/exit_code=1/profile=prod/environment=Production, ledger row exists (start_run ran before gate), state="failed", step table empty, fake_client.post.assert_not_awaited(), cross-reference preserved.

The first two are exactly the "envelope-ledger coupling" check the lead asked about. The third documents the chosen behavior so a future refactor doesn't accidentally short-circuit start_run and lose the audit trail of "the run was attempted but refused."

Test hardening

• isolated_home fixture in tests/test_envelope/conftest.py is now autouse — every envelope test redirects HOME and Path.home() to tmp_path. Prevents run_batch-touching tests from polluting ~/.config/bcli/batch/. Right move.
• TestBatchRunEnvelope.test_batch_run_writes_envelope_all_ok updated: record_id assertion changed from None to len == 32 (uuid4 hex). Comment cites #15 + #16 integration. Consistent with new behavior.

Local verification (in /tmp/aip-review-15 worktree)

• Focused suites — uv run pytest tests/test_envelope/ tests/test_batch_ledger/ tests/test_cli/test_batch_safety.py → 85 passed in 0.67s (43 envelope incl. 3 new integration + 34 ledger + 8 batch_safety).
• Full suite — uv run pytest tests/ → 727 passed, 5 skipped in 20.73s (matches worker self-report).
• uv run ruff check src/ tests/ → clean.

Non-blocking observation

Failure-path ordering differs between branches:

• Policy refusal: ledger.finish_run first, then cap.emit_failure, then re-raise.
• BaseException: cap.emit_failure first, then ledger.finish_run, then re-raise.

Both produce correct attestations because cap.set_record_id(run_id) runs unconditionally at line 179 — by the time either failure branch fires, the envelope already knows which ledger to point at. The BaseException docstring explicitly says "Order matters: emit the envelope BEFORE finalizing the ledger so both attestations describe the same outcome" — but the policy-refusal branch doesn't follow that principle. Not a correctness bug, just a minor inconsistency. Worth noting for Phase 4 worker if they happen to be touching this region.

After this merges

Standing down. Worker B picks up Phase 5 (bcli_mcp rewrite to consume bcli describe + --result-out); Worker A picks up Phase 4 (exit-code taxonomy + JSON-on-pipe + idempotency-key + progress events).

Verdict: APPROVE. Igor merges; I do not.