Skip to content

fix(session): honor config.session.metadata on session:start in the Rust kernel - #103

Draft
Brian Krabach (bkrabach) wants to merge 1 commit into
mainfrom
fix/session-metadata-rust-parity
Draft

fix(session): honor config.session.metadata on session:start in the Rust kernel#103
Brian Krabach (bkrabach) wants to merge 1 commit into
mainfrom
fix/session-metadata-rust-parity

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Parity bug fix, not a new feature

session.metadata is an already-specified, already-tested, already-used channel. It is
dead at runtime.

Layer State
Contract docs/specs/CONTRIBUTION_CHANNELS.md:76session.metadata – runtime metadata snapshots.
Spec + tests tests/test_session_metadata.py — "CP-SM: Kernel reads config.session.metadata and includes it as optional 'metadata' key in event payloads. Pure passthrough." Green today.
Python kernel python/amplifier_core/session.py:154-165 — implements it.
Rust bindings (what actually runs) bindings/python/src/session.rs:389-392 — builds {session_id, parent_id}. Metadata dropped.
Pure-Rust kernel crates/amplifier-core/src/session.rs:318-324 — same omission.
session:fork python/amplifier_core/_session_init.py:274-291already correct (shared Python helper, both session classes delegate to it).

Production does from amplifier_core import AmplifierSession, which is RustSession
(python/amplifier_core/__init__.py:16). So one contract has three independent payload
constructions, and two of them disagreed with it — on session:start and session:resume
specifically, while session:fork quietly kept working.

Why the tests didn't catch it: tests/test_session_metadata.py:13 imports
from amplifier_core.session import AmplifierSession — the pure-Python class. The
metadata tests have been passing, green, on code that no runtime consumer executes. That
switchover blind spot is the actual defect; the missing merge is just its first casualty.

Dead-channel evidence

Measured on a real corpus on one machine (77,553 events.jsonl files under
~/.amplifier/projects/): across a 3,000-file random draw, 2,983 session:start
records were sampled and 0 carried any metadata key. Observed data key-sets:

988  ('parent_id', 'session_id', 'timestamp')
937  ('parent_id', 'redaction', 'session_id', 'timestamp')
689  ('parent_id',)
361  ('parent_id', 'timestamp')

amplifier-app-cli has been writing session.metadata for spawned children since
session_spawner.py:440-449 ("enables kernel CP-SM passthrough on session:start/fork").
It has been landing on session:fork and vanishing on session:start.

The change (~15 LOC of product code)

  • crates/amplifier-core/src/session.rssession_metadata_passthrough(), one shared
    reader for both Rust emit paths so they cannot drift apart again. Its emptiness guard
    mirrors the Python kernel's if session_metadata: truthiness test, so absent or empty
    metadata leaves the payload byte-identical to today
    .
  • bindings/python/src/session.rs — cache session.metadata in new() alongside
    cached_session_id / cached_parent_id, merge it into pre_event_data.
  • crates/amplifier-core/src/session.rs — same merge on the pure-Rust emit.

No new event. No new top-level payload key. No change to
crates/amplifier-core/src/events.rs.

Tests

  • bindings/python/tests/test_session_metadata_rust.py (new, 8 tests) — the parallel
    suite against RustSession that the existing tests could not provide: start/resume with
    and without metadata, empty-metadata parity with the Python guard, verbatim nested
    passthrough, session_id/parent_id not displaced, and session:fork pinned.
  • crates/amplifier-core/src/session.rs (4 new Rust tests) — the passthrough reader
    (configured / absent / {} / [] / "" / null) plus session:start emitted with and
    without metadata.
cargo test -p amplifier-core     473 passed; 0 failed  (lib)
                                  12 passed; 0 failed  (integration binaries)
                                  19 passed; 0 failed  (doc-tests)
cargo clippy -p amplifier-core -p amplifier-core-py -- -D warnings    clean
cargo fmt   -p amplifier-core -p amplifier-core-py --check            clean

uv run pytest tests/ bindings/python/tests/ -q -m "not slow"
                                1031 passed, 1 skipped, 6 deselected

CI that will run on this PR (.github/workflows/rust-core-ci.yml): rust-tests
(cargo test / check / fmt / clippy), node-tests, and python-tests on 3.11 / 3.12 / 3.13.
proto-check.yml is untouched by this diff (no .proto changes).

Back-compat

  1. Metadata absent ⇒ unchanged. Asserted on both classes
    (test_session_metadata.py, test_session_metadata_rust.py) and in Rust
    (execute_omits_metadata_when_not_configured). Empty metadata is treated exactly like
    absent, matching the Python guard.
  2. New kernel + old app ⇒ unchanged. The key only appears when config.session.metadata
    is non-empty, so callers that configure nothing get today's payload byte-for-byte.
  3. No new top-level keys. Everything lands at data.metadata because metadata is not
    in hooks-logging's promoted-key list — no hooks-logging change is required or included.
  4. Existing corpus untouched. Every already-written events.jsonl stays exactly as it
    is. This improves future data only.

Live proof

Built from this branch (maturin build --release), installed into a scratch venv together
with the companion app-cli change (microsoft/amplifier-app-cli#278), then one real
amplifier run --mode single "..." session. The persisted
~/.amplifier/projects/<slug>/sessions/<id>/events.jsonl session:start line, verbatim
apart from formatting:

{"ts":"...","lvl":"INFO","schema":{"name":"amplifier.log","ver":"1.0.0"},
 "event":"session:start","redaction":{"applied":true,"rules":["secrets","pii-basic"]},
 "session_id":"a0122cbb-6438-4014-99cb-c63fefef9985",
 "data":{"metadata":{"invocation":{"launched_by":"cli","launched_by_session_id":null,
 "mode":"single","schema":1,"stdin_isatty":false,"stdout_isatty":false}},"parent_id":null}}

Negative control — same app-cli, released amplifier-core==1.6.0 swapped back in, same
command:

{"ts":"...","lvl":"INFO","schema":{"name":"amplifier.log","ver":"1.0.0"},
 "event":"session:start","redaction":{"applied":true,"rules":["secrets","pii-basic"]},
 "session_id":"bbbbd7c4-8a1b-4b26-ba73-edfcbf9dc577","data":{"parent_id":null}}

Metadata configured on the mount plan, dropped by the kernel — the bug, reproduced on
demand. Note also that hooks-logging needed zero changes: metadata auto-nests under
data.

Notes for review

  • The switchover blind spot (§ "Why the tests didn't catch it") is a general hazard for
    any kernel payload contract, not just this one. Fixing it broadly — e.g. parametrizing
    the payload suites across both classes — is out of scope here, but worth a decision.
  • The pure-Rust emit fix is included deliberately. Fixing only the bindings path would leave
    the same trap armed for the next field added to session:start.

Co-Authored-By: Amplifier 240397093+microsoft-amplifier@users.noreply.github.com

…ust kernel

`docs/specs/CONTRIBUTION_CHANNELS.md` lists `session.metadata` as a runtime
passthrough channel; `tests/test_session_metadata.py` specifies it; and
`python/amplifier_core/session.py` implements it. The Rust switchover shipped
without it.

Production does `from amplifier_core import AmplifierSession`, which is
`RustSession` (`python/amplifier_core/__init__.py`). Its lifecycle emit built
the payload from `{session_id, parent_id}` only, so configured metadata was
dropped before any hook saw it. The pure-Rust kernel had the same omission.
`session:fork` was unaffected -- it is emitted by the shared Python helper
`_session_init.py`, which already honored the contract. So one contract had
three payload constructions and two of them disagreed with it.

The metadata tests were green throughout, because they import the *pure-Python*
`amplifier_core.session.AmplifierSession` -- the class no runtime consumer
executes. That blind spot is what let this ship dead.

- `session_metadata_passthrough()` in `crates/amplifier-core/src/session.rs`:
  one shared reader, so the Rust emit paths cannot drift apart again. Mirrors
  the Python kernel's `if session_metadata:` guard, so absent *or* empty
  metadata leaves the payload byte-identical to before.
- Merged into the bindings emit (cached at construction alongside
  `session_id`/`parent_id`) and into the pure-Rust emit.
- `bindings/python/tests/test_session_metadata_rust.py`: the parallel suite
  against `RustSession` that the existing tests could not provide.

Additive only. No new event, no new top-level payload key, no schema change.

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

PARKED at the maintainer's request (2026-08-20) — amplifier-core cannot change right now. Work is complete and live-proven (see body); converted to draft to prevent accidental merge. Unpark order: this PR first, then amplifier-app-cli #278. Tracking: microsoft/amplifier-bundle-attractor#308.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants