Skip to content

fix(loader): preserve canonical identity for source-resolved modules - #110

Merged
Brian Krabach (bkrabach) merged 5 commits into
mainfrom
fix/canonical-source-module-imports
Sep 21, 2026
Merged

Brian Krabach (bkrabach) merged 5 commits into
mainfrom
fix/canonical-source-module-imports

Conversation

@bkrabach

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

Copy link
Copy Markdown
Collaborator

Summary

Use the same canonical Python module object for source-path validation and runtime mounting. Source resolution now remains authoritative instead of validating one package and then mounting a conflicting installed entry point.

  • All five Python validators share the canonical import helper. Relative imports, module-local state, class identity, and lifecycle metadata stay associated with the selected package.
  • Refuse conflicting package or child-module origins without replacing another importer's sys.modules entries. Audit cached children even when the matching root is already loaded.
  • Restore temporary sys.path changes by value, including when package initialization reconstructs equivalent path strings; retain import-lock regression coverage.
  • Include the proposed 1.6.2 patch version via scripts/bump_version.py, with the two local Cargo lock entries synchronized. No external dependency upgrades, new kernel events, Rust algorithms, or public protocol changes.
  • Synchronize the tracked uv.lock root package with 1.6.2 in a separate lockfile-only commit. All 20 third-party package records, pins, hashes, markers, and URLs are unchanged.

Why this is draft

The code and release candidate are available for core-owner review, but this is not a merge/release-ready claim. Required release gates still need resolution before merge. No downstream changes, tag, PyPI publish, or local user-install update is included.

Evidence from this candidate

  • Independent Python code review; the two import-isolation edge cases found during review have regression tests.

  • cargo test -p amplifier-core --locked: 505 tests passed across unit, integration, and doctest groups.

  • cargo fmt -p amplifier-core -p amplifier-core-py --check: passed.

  • cargo clippy -p amplifier-core -p amplifier-core-py --locked -- -D warnings: passed.

  • Fresh maturin build --release --locked: built amplifier_core-1.6.2-cp311-abi3-manylinux_2_34_aarch64.whl with the default binding features. SHA-256: f5bbfb07e77133526fbbd646876b41783294f1f988433e7c116817de61c8a66b.

  • Wheel installed into a new isolated Python 3.12 virtual environment. Core, loader, and validator imports resolve to the installed wheel, not the source checkout.

  • Downstream GitHub main checks: no tool.uv.sources.amplifier-core override in amplifier, amplifier-app-cli, or amplifier-foundation. No downstream files changed.

  • Full fresh-wheel Python/binding suite: 1061 passed, 1 failed, 1 skipped, 6 deselected. Failure: tests/test_hooks_request_id.py::test_concurrent_calls_get_distinct_ids_and_pair_correctly expected the agent request before the summarizer request, but observed the reverse. This test and its hook/correlation implementation are outside the patch; the failure remains unresolved and is not waived. Two pytest fixture deprecation warnings also occurred.

    A bounded diagnostic used five fixed independent runs per build: the freshly built Git-base wheel (6d4cd217) passed 5/5; this candidate passed 4/5 and failed 1/5. Hook/correlation Python files and the test are byte-identical between the two sources. This does not establish a pre-existing failure or clear the candidate's failing full-suite result. The initial comparison against published PyPI 1.6.1 could not collect this test because that wheel lacks amplifier_core.correlation; the diagnostic therefore used a wheel built from the exact PR base instead.

  • Mandatory scripts/e2e-smoke-test.sh: not run. It invokes a real Anthropic-backed CLI session; no live-provider credential was used or real-provider release result claimed during this PR preparation.

  • Original code revision 98429cd passed all five GitHub CI jobs (Rust, Node, Python 3.11–3.13) and CLA: https://github.com/microsoft/amplifier-core/actions/runs/34697441139

  • Latest lockfile-follow-up CI and core-owner approval: pending.

The 12 canonical-module regression cases passed in the focused source test; the same file also passed as part of the fresh-wheel run above. Earlier non-wheel integration evidence is historical only and is not substituted for fresh release evidence.

Lockfile-only follow-up verification

uv lock --check --offline failed before the follow-up because the project version was stale. Regenerating with uv lock --offline changed only the root package version to 1.6.2; uv lock --check --offline now passes. The committed lockfile previously recorded 1.5.1; the intervening uncommitted 1.6.1 edit has been superseded by the correct release version.

Rust tests, formatting, and Clippy passed again. All 52 Python source files in the existing candidate wheel match the current source tree. One full Python/binding rerun against that wheel passed 1062 tests, with 1 skipped and 6 deselected (the same two fixture deprecation warnings). This successful rerun does not diagnose or waive the earlier intermittent hook-ordering failure above; that gate remains open.

The lockfile change is isolated so it can be reviewed, dropped, or cherry-picked separately. If cherry-picked onto a different release, its root version must match that target's manifest.

Behavioral trade-off / review focus

For a resolver-selected source, reusing an ambient installed entry point with the same module ID is no longer permitted. A conflicting cached source fails explicitly rather than silently mounting or replacing it. Direct entry-point discovery remains supported when no source resolver applies. Please review that conflict policy and the Python import-lock/path-restoration behavior.

This is Python-specific import machinery in the existing Python module loader, not a new cross-language kernel feature. No Rust/Python event or protocol symmetry additions are required.

Before merge

  1. Resolve or separately disposition the hook-ordering test failure with evidence; no retry-until-green waiver.
  2. Run and attach the mandated live-provider smoke result against the final candidate.
  3. Have the core owner confirm the release version and tag/publish procedure. The mandate describes merge-triggered release automation, while the checked-in wheel workflow is tag-triggered; this PR does not change release automation or create tags.
  4. Wait for required CI and core-owner approval. Downstream follow-ups remain deferred until that decision.

The stale uv.lock edit is now replaced by the verified release-aligned lockfile update described above. Test-generated protobuf files remain excluded; no proto definition changed.

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>
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 6f23ef0 into main Sep 21, 2026
6 checks passed
@bkrabach

Copy link
Copy Markdown
Collaborator Author

This PR's head 6a8bd95b02873695c60d78a63fc2dbc422e95288 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