Skip to content

Preserve individual hook injection lifetimes - #111

Merged
Brian Krabach (bkrabach) merged 3 commits into
mainfrom
fix/injection-journal
Sep 21, 2026
Merged

Brian Krabach (bkrabach) merged 3 commits into
mainfrom
fix/injection-journal

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator

What changed

This Core-only carrier preserves each hook context injection as an ordered ContextInjection item instead of reducing mixed lifetimes into one scalar result. Each item retains its content, role, ephemeral lifetime, append-to-last-tool-result flag, hook provenance, and event provenance.

HookRegistry continues to project legacy scalar fields for existing consumers: content is concatenated, the first item supplies the role, and boolean flags are ORed. Existing scalar-only producers remain compatible.

The coordinator validates the complete item sequence before context writes, writes durable items once, and returns only ephemeral items as the residual result. Python, gRPC/proto, and Node bindings carry the additive schema and conversions.

Compatibility and generated-stub repairs

The single-handler normalization path preserves all non-injection HookResult fields, including UI and approval metadata, while updating only the legacy injection projection from the canonical ordered items.

The committed Python gRPC stub uses its package-relative sibling import. A canonical, pinned grpcio-tools==1.78.0 generation script produces both committed stubs; Proto Sync runs it and compares complete generated output. The loader remediation and developer instructions point to that command.

Test-only follow-up

This final follow-up changes only tests. It asserts the append-only 16-field HookResult proto schema and exact field numbers, validates the repeated ContextInjection descriptor and public alias export, and replaces scheduler-order dependence in request-ID isolation coverage with a deterministic async barrier.

Why

The scalar aggregation path collapses mixed durable and ephemeral injections. That loses per-item lifetime and provenance before the coordinator can make the persistence decision. The associated compatibility and generated-stub repairs address the failures discovered on the first draft CI run.

Scope and non-goals

This is the Core carrier, not a claim that the wider CI-journal transition is complete. Dependent context, loop, and CLI work remains local and is not included in this PR.

No Core logging or policy dependency, compaction policy, Core dependency, version change, or production change was added in the test-only follow-up.

Verification

  • cargo fmt --all -- --check passed.
  • Rust hook tests: 33 passing.
  • Isolated subprocess package-import regression test: 1 passing.
  • Canonical regeneration matched both committed Python gRPC stub files exactly.
  • Rebuilt native-engine public CI Python command: pytest tests/ bindings/python/tests/ -m 'not slow' — 1052 passed, 1 skipped, 6 deselected, 2 warnings. The existing workflow excludes the slow WASM integration category.
  • Targeted three-file test set: 47 passing with the native engine.
  • Offline native probe confirmed inject_context retains UI message, suppression, and ordered injection fields.
  • Full CLI suite: 2332 passed, 4 skipped, 13 deselected, 1 expected failure; runtime unchanged.
  • Strict first-turn session-store and CLI create/resume/SIGKILL controls passed without provider API calls.

The preceding Core PR head passed its public Rust, Node, and Proto runs after the compatibility repair. This test-only commit requires its own GitHub Actions confirmation; do not infer final status from prior-SHA runs.

Breaking changes

None intended. context_injections is additive, and existing scalar hook-result producers continue to work through the compatibility projection.

Draft status

Draft only; do not merge or mark ready. It remains under review until this commit's applicable GitHub Actions checks pass, release prerequisites are reviewed, and downstream compatibility is confirmed.

Generated with Amplifier

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

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

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach
Brian Krabach (bkrabach) merged commit 69bb861 into main Sep 21, 2026
8 checks passed
@bkrabach

Copy link
Copy Markdown
Collaborator Author

This PR's head 667fe3caa6f5c2c9f5c4b132172561e706996ab0 is included by ancestry in the combined carrier #109, merged at 4f53e0f666aa370ec7e579bf282b7a4b6c96fbbc. GitHub marked this PR merged through that inclusion; it did not receive a separate merge into main.

The carrier retains the complete change and passed the combined-state native, consumer, and default Docker smoke gates after CLI companion microsoft/amplifier-app-cli#358 landed. See the combined validation and source receipt and the migration notes in #109. No package was published by this review lane.

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