refactor: scheme-agnostic WorldBundle + exposure metadata hook [LTV-Pn.2]#122
Merged
Merged
Conversation
…n.2] Second sub-PR of the split LTV-Pn. Removes the last core/shared-layer references to the lead-scoring scheme (carried cleanups #2 and #3), so the lifecycle scheme can plug into the same envelope. WorldBundle (cleanup #3): - Replaced the three lead-scoring-typed fields (population / simulation_result / world_graph) with a single opaque `artifacts: Any`. Each scheme defines and unwraps its own container; lead-scoring adds LeadScoringArtifacts (schemes/lead_scoring/artifacts.py). core.models no longer imports any lead_scoring type — the layering inversion introduced in LTV-Pf.1 is gone. Exposure (cleanup #2): - apply_exposure is now scheme-agnostic: it writes the generic, spec-only world_spec.json (kept in exposure/metadata.py as write_world_spec_json) and dispatches the scheme-specific hidden-truth files to a new GenerationScheme.write_metadata(bundle, meta_dir) hook, resolved from bundle.spec.scheme via the registry. The lead-scoring graph / latent registry / mechanism-summary writers moved out of exposure/ into LeadScoringScheme.write_metadata. exposure/ no longer references lead_scoring. - Protocol gains write_metadata; the lifecycle stub implements it (raises NotImplementedError until Pn.4). Byte-identity: the full lead-scoring bundle is byte-identical across BOTH exposure modes (research_instructor 21 files, student_public 14) — verified against a pre-refactor SHA-256 reference. The metadata writers moved, not changed; world_spec.json content/order is preserved. Re-scope: the shared bundle orchestrator (cleanup #1) moves from this PR to LTV-Pn.4. Per the roadmap's own note it is best designed with the second scheme's write_bundle in hand; extracting it now against one scheme would guess the hook shape. Roadmap + deferred-cleanups updated (#2, #3 done; #1 → Pn.4). Tests: new tests/schemes/test_scheme_metadata_hook.py (artifacts populated; WorldBundle has only spec+artifacts; generic world_spec writer; lead-scoring hook emits the 4 hidden-truth files; unpopulated-bundle + lifecycle-stub raise). Updated field-access sites in 5 test modules. Full suite 1808 passed / 51 skipped; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Refactors the generation “envelope” to remove lead-scoring coupling by making WorldBundle scheme-agnostic (via an opaque artifacts payload) and by moving hidden-truth metadata writing behind a new GenerationScheme.write_metadata(bundle, meta_dir) hook dispatched from apply_exposure.
Changes:
- Replace
WorldBundle’s lead-scoring-typed fields withartifacts: Any, with lead-scoring providing aLeadScoringArtifactscontainer. - Make exposure metadata scheme-agnostic by splitting out
world_spec.jsonwriting and dispatching scheme-specific hidden truth toGenerationScheme.write_metadata. - Update tests and roadmap/docs to reflect the new bundle shape and metadata hook behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
leadforge/core/models.py |
Replaces scheme-specific bundle fields with artifacts: Any on WorldBundle. |
leadforge/schemes/base.py |
Adds GenerationScheme.write_metadata protocol hook for hidden-truth emission. |
leadforge/schemes/lead_scoring/artifacts.py |
Introduces LeadScoringArtifacts as the scheme-owned artifacts container. |
leadforge/schemes/lead_scoring/__init__.py |
Populates WorldBundle.artifacts and implements write_metadata for lead scoring. |
leadforge/schemes/lifecycle/__init__.py |
Adds stub write_metadata method (NotImplemented) to satisfy the protocol. |
leadforge/exposure/modes.py |
Updates apply_exposure to write world_spec.json and dispatch scheme metadata via registry. |
leadforge/exposure/metadata.py |
Narrows exposure metadata to scheme-agnostic world_spec.json writing only. |
tests/api/test_generator.py |
Updates expectations to reference bundle.artifacts.*. |
tests/test_difficulty_modulation.py |
Updates access paths from bundle.simulation_result to bundle.artifacts.simulation_result. |
tests/render/test_render.py |
Updates bundle construction to use LeadScoringArtifacts. |
tests/schemes/test_registry.py |
Updates scheme-registry generation assertions to use bundle.artifacts.*. |
tests/schemes/test_render_dispatch.py |
Updates error-message assertions for unpopulated bundle writes. |
tests/schemes/test_scheme_metadata_hook.py |
Adds coverage for scheme metadata hook + scheme-agnostic world_spec.json writer. |
docs/ltv/roadmap.md |
Marks Pn.2 as complete and updates roadmap notes accordingly. |
.agent-plan.md |
Updates plan/status notes to reflect Pn.2 progress. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Self-review finding on the WorldBundle generalization: five dataset-build scripts still read the removed bundle.simulation_result / bundle.population fields and would AttributeError at runtime. This slipped both nets: - mypy leadforge does not type-check scripts/, and - the existing tests/scripts/test_build_v*_snapshot.py cover only the pipelines.build_v* transform helpers (pure DataFrame functions), never the generate_bundle() entry point that touches the bundle. CI's "Validate v6/v7" jobs validate a pre-built CSV; they do not regenerate, so they would not have caught it either. Fix: build_v4/v5/v6/v7_snapshot.py and build_midproject_lead_scoring.py now read bundle.artifacts.simulation_result / bundle.artifacts.population. Smoke- ran the v6 builder end-to-end through Generator.generate() to confirm. Guard: tests/scripts/test_build_v6_snapshot.py gains TestGenerateBundleArtifactsPath, which loads the script via importlib and runs generate_bundle(small) so a future WorldBundle field rename can't silently break the generate path again. The other four scripts share the identical access pattern. Full suite 1809 passed / 51 skipped; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…TV-Pn.2] Two Copilot review findings on #122. 1. apply_exposure reused an existing metadata/ via mkdir(exist_ok=True) when writing hidden truth, so a reused output path could retain stale files. Pre-existing behavior, but Pn.2 makes it newly dangerous: once the lifecycle scheme writes a different hidden-truth file set, regenerating a lifecycle bundle over a path that previously held a lead-scoring bundle would orphan graph.graphml / mechanism_summary.json into the new bundle. Now apply_exposure always removes any existing metadata/ first, then recreates it when writing — so contents exactly match the current bundle (mirroring the non-writing branch, which already rmtree'd it). Byte-identity preserved for both modes (fresh paths have no metadata/, so the rmtree is a guarded no-op). Regression tests: a pre-seeded stale file is gone after an instructor rewrite; student_public still removes the dir entirely. 2. The lead_scoring.write_bundle docstring still described apply_exposure as writing the lead-scoring hidden graph + latent registry directly. Updated to reflect the Pn.1/Pn.2 reality: build_manifest and apply_exposure are scheme-agnostic, and hidden truth is delegated to write_metadata; the remaining shared-orchestrator extraction is deferred to Pn.4. Full suite 1811 passed / 51 skipped; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
pr-agent-context report: No unresolved review comments, failing checks, or actionable patch coverage gaps were found on PR #122 in repository https://github.com/leadforge-dev/leadforge. Treat this PR as all clear unless new signals appear.Run metadata: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second sub-PR of the split
LTV-Pn(M6). Removes the lastcore/shared-layer references to the lead-scoring scheme — carried cleanups #2 (apply_exposurecoupling) and #3 (core→scheme layering inversion) — so the lifecycle scheme can plug into the same envelope.WorldBundle→ scheme-agnostic (cleanup #3)WorldBundle's three lead-scoring-typed fields (population/simulation_result/world_graph) are replaced by a single opaqueartifacts: Any. Each scheme defines and unwraps its own container; lead-scoring addsLeadScoringArtifacts(schemes/lead_scoring/artifacts.py).core.modelsno longer imports anylead_scoringtype — the inversion introduced in LTV-Pf.1 is gone.Exposure → scheme hook (cleanup #2)
apply_exposureis now scheme-agnostic:world_spec.json(kept inexposure/metadata.pyaswrite_world_spec_json);GenerationScheme.write_metadata(bundle, meta_dir)hook, resolved frombundle.spec.schemevia the registry.The lead-scoring graph / latent-registry / mechanism-summary writers moved out of
exposure/intoLeadScoringScheme.write_metadata.exposure/no longer referenceslead_scoring. The protocol gainswrite_metadata; the lifecycle stub implements it (raisesNotImplementedErroruntil Pn.4).Byte-identity
The full lead-scoring bundle is byte-identical across both exposure modes (research_instructor 21 files, student_public 14), verified against a pre-refactor SHA-256 reference. The metadata writers moved, not changed.
Re-scope
The shared bundle orchestrator (cleanup #1) moves from this PR to
LTV-Pn.4. The roadmap's own note says it's best designed with the second scheme'swrite_bundlein hand — extracting it now against one scheme would guess the hook shape. Roadmap + deferred-cleanups updated (#2, #3 done; #1 → Pn.4).Tests
tests/schemes/test_scheme_metadata_hook.py: artifacts populated bybuild_world;WorldBundleexposes only{spec, artifacts}; genericworld_spec.jsonwriter needs only aWorldSpec; the lead-scoring hook emits the four hidden-truth files; unpopulated-bundle and lifecycle-stub both raise.bundle.X→bundle.artifacts.X).Next: Pn.3 (lifecycle config + regression task model).
🤖 Generated with Claude Code