refactor: shared bundle-writing orchestrator [LTV-Pn.4d]#128
Merged
Conversation
Final sub-PR of the split LTV-Pn.4 (and the last carried M2 cleanup, #1). Both schemes' write_bundle performed the same on-disk sequence — mkdir → relational tables → task splits → dataset card → feature dictionary → exposure metadata → manifest — differing only in the *content*. Lift that sequence into one place. - New render/bundle.py: write_bundle_envelope(bundle, root, *, relational, tasks, dataset_card, feature_specs, generation_scheme, redacted, motif_family, relational_snapshot_safe, structural_redactions, extra_fields, generation_timestamp) + a TaskExport(manifest, frame) record. It owns the I/O and the file-ordering the manifest's hashing depends on; it contains no scheme-specific logic. - LeadScoringScheme.write_bundle and LifecycleScheme.write_bundle now compute only their scheme-specific content (which relational frames + snapshot-safe projection, which task exports, which card, which visible features, which manifest params) and delegate to the envelope. - The dataset card needs table row counts; each scheme computes {name: len(df)} on its final frames (identical to write_relational_tables' counts — redaction drops columns, not rows) and renders the card before delegating. Byte-identity: all FOUR bundles — lead_scoring + lifecycle, each in research_instructor + student_public — verified byte-identical against a pre-refactor SHA-256 reference of every file. The orchestration moved; no output changed. Tests: new tests/render/test_bundle_envelope.py pins the envelope contract (all artefacts written, manifest passthrough fields, multiple task dirs, no metadata in snapshot-safe mode) on a minimal bundle. Full suite 1877 passed / 51 skipped; ruff + mypy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shaypal5
added
type: refactor
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR factors the shared “write bundle to disk” orchestration out of individual schemes into a single, scheme-agnostic envelope function (write_bundle_envelope). Both LeadScoringScheme.write_bundle and LifecycleScheme.write_bundle now focus on computing scheme-specific content (frames, tasks, card, visible features, manifest params) and delegate the deterministic I/O sequence (tables → tasks → card → dictionary → exposure → manifest) to the shared renderer.
Changes:
- Added
leadforge/render/bundle.pywithwrite_bundle_envelopeand aTaskExportcontainer to standardize task export inputs. - Refactored lifecycle and lead-scoring schemes to delegate bundle writing to the new envelope while preserving their scheme-specific computation steps.
- Added
tests/render/test_bundle_envelope.pyto pin the envelope’s contract (artifact outputs, manifest passthrough fields, multi-task behavior, and snapshot-safe metadata omission).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
leadforge/render/bundle.py |
Introduces the shared bundle-writing envelope (I/O ordering + manifest build/write). |
leadforge/schemes/lead_scoring/__init__.py |
Refactors scheme bundle writing to compute content then call write_bundle_envelope. |
leadforge/schemes/lifecycle/__init__.py |
Refactors scheme bundle writing to compute content then call write_bundle_envelope (incl. lifecycle structural redactions passthrough). |
tests/render/test_bundle_envelope.py |
Adds focused tests validating the envelope’s filesystem outputs and manifest behavior. |
docs/ltv/roadmap.md |
Marks LTV-Pn.4d as completed and links PR #128. |
.agent-plan.md |
Updates project plan/status text to reflect the refactor completion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…LTV-Pn.4d] Self-review of the orchestrator extraction: lead-scoring's write_bundle now writes tasks through the shared bundle envelope, so the schemes.lead_scoring.render.tasks wrapper is no longer on the write-bundle path — its docstring still claimed it kept the default "so existing call sites are unchanged," which is now stale. The wrapper is not dead: it remains a tested convenience (its 7 test consumers rely on the CONVERTED_WITHIN_90_DAYS default), so removing it would only churn tests to pass an explicit task and reverse the deliberate Pn.3 layout. Docstring updated to state it's the scheme's default-task helper, off the write path since Pn.4d. No behaviour change. 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 #128 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
Final sub-PR of the split
LTV-Pn.4— and the last carried M2 cleanup (#1, shared render orchestration). Both schemes'write_bundleran the same on-disk sequence (mkdir → relational → task splits → card → dictionary → exposure → manifest), differing only in content. This lifts that sequence into one place.render/bundle.py—write_bundle_envelopeOwns the shared I/O and the file-ordering the manifest's hashing depends on, with no scheme-specific logic. A scheme computes its final exposure-projected relational frames, its per-task
TaskExport(manifest, frame)list, its rendered dataset card, its visible feature catalog, and its manifest params — then delegates.Both
LeadScoringScheme.write_bundleandLifecycleScheme.write_bundleare now thin: scheme-specific content computation + one envelope call.One wrinkle handled cleanly: the dataset card needs table row counts (computed during the relational write). Each scheme computes
{name: len(df)}on its final frames — identical towrite_relational_tables' return (redaction drops columns, not rows) — and renders the card before delegating.Byte-identity
All four bundles — lead_scoring + lifecycle, each in
research_instructor+student_public— verified byte-identical against a pre-refactor SHA-256 reference of every file. The orchestration moved; no output changed.Tests
New
tests/render/test_bundle_envelope.pypins the envelope's own contract (all artefacts written; manifest passthrough fields incl.motif_family/extra_fields; multiple task dirs; nometadata/in snapshot-safe mode) on a minimal bundle. Full suite 1877 passed / 51 skipped, ruff + mypy clean.Milestone
This completes
LTV-Pn.4and discharges the last of the three carried M2 cleanups. Next:LTV-Po— theb2b_saas_ltv_v1recipe assets + end-to-endGenerator.from_recipe(...), where the deferred recipe-driven difficulty resolution and the narrative-consumption decision also come due.🤖 Generated with Claude Code