refactor(api): resolve_config carries lifecycle config fields [LTV-Po.2a]#131
Merged
Conversation
….2a] First half of the split LTV-Po.2 (the config-plumbing enabler; the recipe assets + e2e are Po.2b). Recipe.resolve_config and Generator.from_recipe/generate were lead-scoring-shaped — they only threaded n_accounts/n_contacts/n_leads to GenerationConfig — so a scheme: lifecycle recipe could not size its customer cohort (n_customers always took the package default). - resolve_config now resolves n_customers (from default_population, an explicit kwarg, or the override dict) and early_tenure_weeks / observation_date (from override) and passes all three to GenerationConfig. forward_windows_days is deliberately NOT resolvable — the lifecycle scheme locks it to its exported constant and rejects overrides (LTV-Pn.4c). - Generator.from_recipe and generate gain an n_customers parameter. Lead-scoring config resolution is unchanged: a recipe without these keys leaves the lifecycle fields at their GenerationConfig defaults, which is exactly what the previous construction produced. Verified byte-identical via full-bundle SHA-256 of a lead-scoring bundle (both exposure modes) against main. Tests: n_customers flows from default_population; kwarg + override precedence; lifecycle fields via override; lead-scoring-style recipe keeps the lifecycle defaults. Full suite 1885 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
This PR refactors the public API config plumbing so Recipe.resolve_config and Generator can carry lifecycle-specific sizing/config fields (notably n_customers) through to GenerationConfig, enabling lifecycle recipes to control cohort size instead of always using package defaults.
Changes:
- Extend
Recipe.resolve_configto resolven_customers,early_tenure_weeks, andobservation_dateintoGenerationConfig. - Add
n_customerssupport toGenerator.from_recipe(...)and per-call overrides inGenerator.generate(...). - Add lifecycle-focused
resolve_configtests and update LTV roadmap/agent plan notes to reflect the Po.2a split.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/api/test_resolve_config_lifecycle.py | Adds tests covering lifecycle-field resolution via resolve_config. |
| leadforge/api/recipes.py | Threads lifecycle config fields (n_customers, early_tenure_weeks, observation_date) through config resolution. |
| leadforge/api/generator.py | Adds n_customers parameter passthrough/override support in from_recipe and generate. |
| docs/ltv/roadmap.md | Documents Po.2 split and records Po.2a scope/constraints. |
| .agent-plan.md | Updates workstream plan text to reflect the Po.2a/Po.2b split. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
51
to
55
| n_accounts: int | None = None, | ||
| n_contacts: int | None = None, | ||
| n_leads: int | None = None, | ||
| n_customers: int | None = None, | ||
| horizon_days: int | None = None, |
Comment on lines
129
to
133
| n_accounts: int | None = None, | ||
| n_contacts: int | None = None, | ||
| n_leads: int | None = None, | ||
| n_customers: int | None = None, | ||
| difficulty: str | DifficultyProfile = _MISSING, # type: ignore[assignment] |
Comment on lines
+46
to
+55
| def test_defaults_when_recipe_omits_lifecycle_fields() -> None: | ||
| # A lead-scoring-style recipe (no n_customers) → lifecycle fields keep their | ||
| # GenerationConfig defaults; this is why lead-scoring resolution is unchanged. | ||
| cfg = _recipe(n_accounts=10, n_contacts=30, n_leads=30).resolve_config( | ||
| seed=1, difficulty="intro" | ||
| ) | ||
| assert cfg.n_customers == 1500 # GenerationConfig default | ||
| assert cfg.forward_windows_days == (90, 365, 730) | ||
| assert cfg.early_tenure_weeks == 4 | ||
| assert cfg.observation_date is None |
…LTV-Po.2a] Self-review of the config-plumbing refactor: verified a lifecycle-shaped recipe (default_population with only n_customers, no n_accounts) both parses via Recipe.from_dict and resolves n_customers through resolve_config — so Po.2a is sufficient for Po.2b's recipe to size its cohort. Found one limitation worth flagging (not a Po.2a defect): early_tenure_weeks / observation_date are override-only — the Recipe schema has no field for them, so the Po.2b recipe.yaml cannot declare them and will use the GenerationConfig defaults (4 weeks; observation_date derived). Recorded in the roadmap Po.2b entry: extend the Recipe dataclass/from_dict if the recipe must declare them, rather than silently relying on override. Docs only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
pr-agent-context report: This run includes unresolved review comments on PR #131 in repository https://github.com/leadforge-dev/leadforge
For each unresolved review comment, recommend one of: resolve as irrelevant, accept and implement
the recommended solution, open a separate issue and resolve as out-of-scope for this PR, accept and
implement a different solution, or resolve as already treated by the code.
After I reply with my decision per item, implement the accepted actions, resolve the corresponding
PR comments, and push all of these changes in a single commit.
# Copilot Comments
## COPILOT-1
Location: leadforge/api/generator.py:55
URL: https://github.com/leadforge-dev/leadforge/pull/131#discussion_r3433266088
Root author: copilot-pull-request-reviewer
Comment:
`n_customers` was added to `from_recipe(...)`, but the docstring Args list below still jumps from `n_leads` to `horizon_days`, so the new parameter isn't discoverable via docs/help text.
## COPILOT-2
Location: leadforge/api/generator.py:133
URL: https://github.com/leadforge-dev/leadforge/pull/131#discussion_r3433266119
Root author: copilot-pull-request-reviewer
Comment:
`generate(...)` now accepts `n_customers`, but the docstring still describes only `n_accounts`/`n_contacts`/`n_leads` + `difficulty` as supported per-call overrides, and the Args list omits `n_customers`.
## COPILOT-3
Location: tests/api/test_resolve_config_lifecycle.py:55
URL: https://github.com/leadforge-dev/leadforge/pull/131#discussion_r3433266142
Root author: copilot-pull-request-reviewer
Comment:
This test module doesn't exercise the advertised override/kwarg precedence for `n_customers` (i.e., override dict beats recipe defaults, and an explicit kwarg beats override). Adding a small precedence test would prevent regressions in the new plumbing.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
First half of the split
LTV-Po.2(the config-plumbing enabler; the recipe YAMLs + end-to-end round-trip are Po.2b).Recipe.resolve_configandGenerator.from_recipe/generatewere lead-scoring-shaped — they only threadedn_accounts/n_contacts/n_leadstoGenerationConfig— so ascheme: lifecyclerecipe couldn't size its customer cohort (n_customersalways took the package default).Change
resolve_confignow resolvesn_customers(fromdefault_population, an explicit kwarg, or theoverridedict) andearly_tenure_weeks/observation_date(fromoverride), and passes all three toGenerationConfig.forward_windows_daysis deliberately not resolvable — the lifecycle scheme locks it to its exported constant and rejects overrides (LTV-Pn.4c).Generator.from_recipeandgenerategain ann_customersparameter.Lead-scoring is unchanged
A recipe without these keys leaves the lifecycle fields at their
GenerationConfigdefaults — exactly what the previous construction produced. Verified byte-identical via full-bundle SHA-256 of a lead-scoring bundle (both exposure modes) againstmain.Tests (4 new; full suite 1885 passed / 51 skipped)
n_customersflows fromdefault_population; kwarg + override precedence; lifecycle fields via override; a lead-scoring-style recipe keeps the lifecycle defaults.Next: Po.2b — the
b2b_saas_ltv_v1recipe YAMLs (narrative.yamlwith ≥2 industries + ≥2 geographies per the Po.1 constraint,difficulty_profiles.yaml), registry registration,difficulty_paramsresolution inbuild_world, and theGenerator.from_recipe("b2b_saas_ltv_v1").generate()round-trip — which completes M6.🤖 Generated with Claude Code