diff --git a/.claude/projects/2026-06-26-fase3-portfolio-fanout/plan-remediation.md b/.claude/projects/2026-06-26-fase3-portfolio-fanout/plan-remediation.md new file mode 100644 index 0000000..9a3d360 --- /dev/null +++ b/.claude/projects/2026-06-26-fase3-portfolio-fanout/plan-remediation.md @@ -0,0 +1,260 @@ +--- +source_findings: + - b3abbdd7035b5463b3bb9e23f83a158d678cb4f1 + - a2376be90affdffea4c9b4ef83014d0a30243e23 +--- + +# Fase 3 remediation — provenance model leak + portfolio rejection-arm coverage + +> **Plan quality: A** (93/100) — APPROVE +> +> Generated by trekplan v5.6.1 on 2026-06-26 — `plan_version: 1.7` +> +> Destination note: written as `plan-remediation.md` (not `plan.md`) to preserve +> the existing Fase 3 delivery plan in this project dir. +> +> Revised once after adversarial review (see Revisions): F1 narrowed to the +> injected branch only; minor specification + headless fixes folded in. + +## Context + +`/trekreview` closed Fase 3 on **WARN** with two MAJOR findings, both in +`src/portfolio_optimiser/run.py`. This plan remediates exactly those two, +nothing more: + +- **F1 (`b3abbdd…`, PLACEHOLDER_IN_CODE, run.py:197)** — `model = "fake-model" + if client_factory is not None else resolve_model(...)` stamps the literal + `"fake-model"` into `ProvenanceStamp.model` for **any** deployer who injects a + custom `client_factory` (a public seam threaded through `run_portfolio(..., + client_factory=...)`). This falsifies a load-bearing provenance invariant — + one of the few hard technical guarantees the framework actually owns — on the + exact path an external MAF user hits. Confirmed against ground truth. +- **F2 (`a2376be9…`, MISSING_TEST, run.py:240-241)** — `_aggregate` computes + `rejected_count=len(rejected)` and a validated-only `sum_claimed_saving_nok`, + but no `run_portfolio` test ever produces a `Rejection` (`test_portfolio.py:64` + asserts `rejected_count==0`). The rejection partition and the validated-only + filter ship green-but-unproven under SC2. + +Intended outcome: provenance reflects the real proposer model on the injected +seam (never a fabricated id), and the portfolio aggregate's rejection arm is +exercised by a deterministic rejection. Each finding's `id` is carried in +`source_findings` above for the audit trail back to `review.md`. + +## Architecture Diagram + +```mermaid +graph TD + subgraph "Fase 3 remediation" + F[injected client_factory] -->|"factory('proposer')"| PC[proposer_client] + PC -->|generate_via_llm| OUT[outcome: Validated/Rejection] + PC -->|"F1: injected branch -> getattr(.model) or 'unknown'"| PS[ProvenanceStamp.model] + DEF[default path] -->|"unchanged: resolve_model(profile)"| PS + OUT -->|_aggregate partition| AGG[PortfolioResult] + AGG -->|"F2 test: 1 rejected"| RC[rejected_count / validated-only sum] + end + style PS fill:#cfe + style RC fill:#fec +``` + +## Codebase Analysis + +- **Tech stack:** Python ≥3.10, Microsoft Agent Framework (`agent-framework-core` + 1.9.0), Pydantic IR, `uv` / `pytest` / `mypy` / `ruff`. Repo-wide `find` sweep = + 82 files; `mypy src` type-checks 14 source modules (the two counts measure + different scopes — repo vs `src/`). +- **Key patterns:** vertical-slice orchestrator (`run_project` → `run_portfolio`), + frozen result dataclasses, `client_factory` test-injection seam, deterministic + blocking validator, synthetic-usage fake clients in `conftest.py`. +- **Relevant files:** + - `src/portfolio_optimiser/run.py:176` — `factory = client_factory or _default_factory(profile)` + - `src/portfolio_optimiser/run.py:193` — proposer client currently inlined into `generate_via_llm` + - `src/portfolio_optimiser/run.py:197` — the `fake-model` leak (F1 site) + - `src/portfolio_optimiser/run.py:231-243` — `_aggregate` (F2 target, correct as-is) + - `tests/test_vertical_slice_e2e.py:28` — `run_project` call shape + `_VALID`/`_VI` fixtures + - `tests/test_portfolio.py:28-45` — `REPLIES` + `_PORTFOLIO_IDS` (portfolio fixtures) + - `tests/conftest.py:84-93,144-163` — `make_client_factory` / `make_portfolio_client_factory` +- **Reusable code:** + - `_default_factory` (run.py:126-130) — the default (non-injected) path is left exactly as-is on `resolve_model(profile, "proposer")`; only the injected branch changes. + - `SyntheticUsageChatClient` (conftest.py:46) — injected fakes carry `.model == "synthetic"`, giving the F1 test a concrete non-fake value to assert. + - `_OUT_OF_RANGE` magnitude `800000` (test_vertical_slice_e2e.py:23) — already proven to reject FV42 (claim 800000 > P90 444750, ≤ Σ 1,482,500), reused for the portfolio rejection. +- **External tech (researched):** none — fully internal change. +- **Recent git activity:** `4253dd6` (review.md committed), `497399e..207f057` Fase 3 delivery (10 commits). Suite green at 114 passed / 4 skipped, mypy + ruff clean (observed this session). + +## Implementation Plan + +### Step 1: Stamp the real proposer model on the injected seam + +- **Files:** `src/portfolio_optimiser/run.py`, `tests/test_vertical_slice_e2e.py` +- **Changes:** In `run_project`, capture the proposer client once instead of + inlining it: replace `outcome = await generate_via_llm(factory("proposer"), + project, gen_context, meter)` (run.py:193) with two lines — `proposer_client = + factory("proposer")` then `outcome = await generate_via_llm(proposer_client, + project, gen_context, meter)`. Then change ONLY the injected arm of run.py:197: + keep the ternary on `client_factory is not None`, leave the default arm as + `resolve_model(profile, "proposer")` untouched, and replace the injected arm's + `"fake-model"` literal with `getattr(proposer_client, "model", None) or + "unknown"` — i.e. `model = (getattr(proposer_client, "model", None) or + "unknown") if client_factory is not None else resolve_model(profile, + "proposer")`. Add a short inline comment: the injected branch stamps the + injected client's real model (`"unknown"` is the neutral fallback for a client + that does not surface one — never a fabricated name); the default path keeps + the deterministic `resolve_model`. This is the narrowest fix — the leak lived + only on the injected branch, and the proven production path is unchanged. No + signature change; `resolve_model` stays imported. +- **Reuses:** `factory` local (run.py:176), `ProvenanceStamp` (provenance.py:32-39); default path keeps `resolve_model` (backends.py:54-65) verbatim. +- **Test first:** + - File: `tests/test_vertical_slice_e2e.py` *(existing)* — add `test_a1_provenance_stamps_injected_client_model_not_sentinel` + - Verifies: an injected `make_client_factory(_VALID)` (SyntheticUsageChatClient, `.model == "synthetic"`) yields `result.provenance.model == "synthetic"` and `result.provenance.model != "fake-model"`. FAILS on current code (stamps `"fake-model"`); PASSES after the fix. Load-bearing: reverting run.py:197 reddens it. + - Pattern: `tests/test_vertical_slice_e2e.py:28` `test_a_valid_proposal_end_to_end` (same `run_project` call shape, `docs_dir`/`make_client_factory`/`fresh_store` fixtures, `_VALID`/`_VI`) +- **Verify:** `uv run pytest tests/test_vertical_slice_e2e.py -q && uv run mypy src` → expected: all e2e tests pass (incl. the new one) and `Success: no issues found in 14 source files` +- **On failure:** revert — `git checkout -- src/portfolio_optimiser/run.py tests/test_vertical_slice_e2e.py` — then stop; do not proceed to Step 2. +- **Checkpoint:** `git commit -m "fix(fase3): stamp real proposer model into provenance, kill fake-model leak (F1)"` +- **Manifest:** + ```yaml + manifest: + expected_paths: + - src/portfolio_optimiser/run.py + - tests/test_vertical_slice_e2e.py + min_file_count: 2 + commit_message_pattern: "^fix\\(fase3\\): stamp real proposer model into provenance, kill fake-model leak \\(F1\\)$" + bash_syntax_check: [] + forbidden_paths: + - tests/test_portfolio.py + must_contain: + - path: src/portfolio_optimiser/run.py + pattern: "proposer_client" + - path: tests/test_vertical_slice_e2e.py + pattern: "test_a1_provenance_stamps_injected_client_model_not_sentinel" + ``` + +### Step 2: Exercise the portfolio rejection arm and the portfolio provenance seam + +- **Files:** `tests/test_portfolio.py` +- **Depends on:** Step 1 (the portfolio-seam model assertion below expects the F1 fix in place). +- **Changes:** Add `from portfolio_optimiser.validator import Rejection` to the + imports. Add `test_a3_rejected_proposal_excluded_from_aggregate`: build + `replies = {**REPLIES, "FV42-GSV-E1": REPLIES["FV42-GSV-E1"].replace("200000", + "800000")}` so FV42's claim (800000) exceeds its P90 (444750) but stays ≤ its + affected-items total (1,482,500) — it constructs cleanly and the validator + rejects it, while RV13 and BRU still validate. Drive `run_portfolio(_PORTFOLIO_IDS, + "local", store=fresh_store, client_factory=make_portfolio_client_factory(replies))` + and assert: `rejected_count == 1`, `validated_count == 2`, + `sum_claimed_saving_nok == 340000` (130000 + 210000, EXCLUDING the rejected + 800000). Locate the rejection explicitly — `rejected = [r for r in result.runs + if isinstance(r.outcome, Rejection)]; assert len(rejected) == 1` — then assert + `rejected[0].outcome.proposal.claimed_saving_nok == 800000` (carried but not + summed) and `rejected[0].provenance.validator_decision == "rejected"`. Add the + portfolio-seam F1 cross-check on the same result: `assert all(r.provenance.model + == "synthetic" for r in result.runs)` — proves the injected model is stamped + across **all N** records via `run_portfolio` (the seam finding `b3abbdd` names), + not just the single-project `run_project`. Finally, correct the now-stale + docstring in `test_d_both_profiles` (the `run.py:152,173 model="fake-model"` + parenthetical) to read that under an injected `client_factory` the stamp + mirrors the injected client's own model (run.py:197), with `resolve_model` + remaining the offline profile-dependent seam that test exercises. +- **Reuses:** `make_portfolio_client_factory` (conftest.py:144-163), `fresh_store` + (conftest.py:166-168), `REPLIES` / `_PORTFOLIO_IDS` (test_portfolio.py:28-45), + the proven `800000` rejection magnitude (test_vertical_slice_e2e.py:23). + Exercises `_aggregate` (run.py:235/240/241) as-is — no `src` change. +- **Test first:** + - File: `tests/test_portfolio.py` *(existing)* — this step IS the test + - Verifies: the rejection partition + validated-only sum + portfolio-seam model. Load-bearing — reverting run.py:241's `for r in validated` filter to sum over all runs makes the sum 1,140,000 and reddens it; reverting the Step-1 fix reddens the `provenance.model == "synthetic"` assertion. + - Pattern: `tests/test_portfolio.py:48` `test_a_fanout_returns_one_runresult_per_project` (same `run_portfolio` call shape + fixtures) +- **Verify:** `uv run pytest tests/test_portfolio.py -q && uv run pytest -q` → expected: portfolio tests pass; full suite `116 passed, 4 skipped` (the observed 114 baseline + 2 new tests; equivalently: +2 passed, zero new failures/skips) +- **On failure:** revert — `git checkout -- tests/test_portfolio.py` — then stop and report. +- **Checkpoint:** `git commit -m "test(fase3): portfolio rejection arm + provenance-seam assertions (F2 + F1 follow-up)"` +- **Manifest:** + ```yaml + manifest: + expected_paths: + - tests/test_portfolio.py + min_file_count: 1 + commit_message_pattern: "^test\\(fase3\\): portfolio rejection arm \\+ provenance-seam assertions \\(F2 \\+ F1 follow-up\\)$" + bash_syntax_check: [] + forbidden_paths: + - src/portfolio_optimiser/run.py + must_contain: + - path: tests/test_portfolio.py + pattern: "test_a3_rejected_proposal_excluded_from_aggregate" + ``` + +## Alternatives Considered + +| Approach | Pros | Cons | Why rejected | +|----------|------|------|--------------| +| F1: unify both branches onto `getattr(client,"model")` | One expression, no ternary | Changes the proven default (production) path to read a third-party `.model` attribute no test guards; silent degrade to `"unknown"` if `agent_framework` moves it | Violates smallest-change; flagged MAJOR by both reviewers — narrowed to the injected branch only | +| F1: make `client_factory` surface its model (signature change) | Explicit model contract | Breaks every injection site (both conftest fixtures, ~12 e2e + ~7 portfolio call sites, workflow.py callers) for zero extra fidelity | Largest blast radius | +| F1: neutral sentinel `"injected"` (no client read) | One-token change | Discards the real `.model` an injected client genuinely carries — lossy | `getattr(.model)` is honest AND minimal; sentinel kept only as the model-less fallback | +| F2: unit-test `_aggregate` directly | Smaller | Doesn't exercise the real `run_portfolio` → validator → Rejection path the finding names | Portfolio-level test is the faithful proof | + +## Test Strategy + +- **Framework:** `pytest` (async, `pytest-asyncio` auto mode), deterministic synthetic clients — no LLM, no egress. +- **Existing patterns:** `run_project`/`run_portfolio` driven by `make_client_factory`/`make_portfolio_client_factory`; the synthetic reply JSON *is* the proposal; P90 = 0.30·Σ when `assumptions` omitted. +- **New tests in this plan:** 2 (one per step). + +### Tests to write + +| Type | File | Verifies | Model test | +|------|------|----------|------------| +| Integration | `tests/test_vertical_slice_e2e.py` | injected client's real model stamped (`"synthetic"`, not `"fake-model"`) on `run_project` | `test_a_valid_proposal_end_to_end` | +| Integration | `tests/test_portfolio.py` | rejection partition + validated-only sum + injected model across all N `run_portfolio` records | `test_a_fanout_returns_one_runresult_per_project` | + +## Risks and Mitigations + +| Priority | Risk | Location | Impact | Mitigation | +|----------|------|----------|--------|------------| +| Low | `SyntheticUsageChatClient.model` is not exactly `"synthetic"` | `tests/conftest.py:46` | F1 test asserts wrong value | Runtime-verified `.model == "synthetic"`; the `!= "fake-model"` assertion is the load-bearing half regardless | +| Low | FV42 `800000` no longer rejects if fixture magnitudes change | `tests/test_portfolio.py:28` | F2 test goes green vacuously | Test asserts `rejected_count == 1` explicitly (not `>= 0`); a non-rejection reddens it | +| Low | A custom injected client exposes its model under a non-`.model` attribute | `src/portfolio_optimiser/run.py:197` | injected-path stamp falls to `"unknown"` | Honest fallback (not a fabricated id); the default production path is untouched and keeps `resolve_model` | + +## Assumptions + +| # | Assumption | Why unverifiable | Impact if wrong | +|---|-----------|-----------------|-----------------| +| 1 | `SyntheticUsageChatClient` exposes `.model == "synthetic"` at the stamp site | Confirmed by sub-agent runtime check, not re-run here | F1 test value adjusts to the actual `.model`; fix logic unaffected | + +## Verification + +- [ ] `uv run pytest -q` → expected: `116 passed, 4 skipped` (114 observed baseline + 2 new; equivalently +2 passed, no new failures) +- [ ] `uv run mypy src` → expected: `Success: no issues found in 14 source files` +- [ ] `uv run ruff check .` → expected: `All checks passed!` +- [ ] `grep -rn "fake-model" src/` → expected: no matches (the leak literal is gone from `src`) +- [ ] Load-bearing check (F1): revert the run.py:197 edit, run `uv run pytest tests/test_vertical_slice_e2e.py -q` → the new test FAILS; restore the edit → it passes +- [ ] Load-bearing check (F2): change run.py:241 to sum over all runs, run `uv run pytest tests/test_portfolio.py -q` → `test_a3` FAILS (sum 1,140,000 ≠ 340,000); restore → passes + +## Estimated Scope + +- **Files to modify:** 3 (`run.py`, `test_vertical_slice_e2e.py`, `test_portfolio.py`) +- **Files to create:** 0 +- **Complexity:** low + +## Plan Quality Score + +| Dimension | Weight | Score | Notes | +|-----------|--------|-------|-------| +| Structural integrity | 0.15 | 92 | 2 steps, F1→F2 order correct, Step-2 dependency on Step-1 made explicit | +| Step quality | 0.20 | 92 | TDD, concrete diffs, exact fixtures + magnitudes, explicit rejection locator | +| Coverage completeness | 0.20 | 95 | Both findings addressed; F1 now proven on both run_project and run_portfolio seams | +| Specification quality | 0.15 | 92 | Exact line numbers, no placeholders, all arithmetic verified by review | +| Risk & pre-mortem | 0.15 | 90 | Narrowed fix removed the default-path coupling risk; remaining risks surfaced | +| Headless readiness | 0.10 | 92 | On-failure revert + halt + checkpoint per step; ASCII commit messages | +| Manifest quality | 0.05 | 92 | Checkable manifests, forbidden_paths fences, ASCII-only patterns | +| **Weighted total** | **1.00** | **93** | **Grade: A** | + +**Adversarial review:** +- **Plan critic:** APPROVE_WITH_NOTES — 0 blockers, 1 major, 8 minor. Major (F1 unnecessarily changed the proven default path) ADDRESSED by narrowing the fix to the injected branch. Minors on the rejection locator, portfolio-seam coverage, em-dash commit, docstring commit-scope, file-count wording, and circuit breaker all ADDRESSED. +- **Scope guardian:** ALIGNED — 0 creep, 0 gaps. Its lone minor (default-path `.model` untested) is dissolved by the narrowed fix (default path unchanged). + +## Revisions + +| # | Finding | Severity | Resolution | +|---|---------|----------|------------| +| 1 | F1 unified both ternary branches, changing the proven default path to an untested `.model` read | major (plan-critic + scope-guardian) | Narrowed: `getattr` only on the injected branch; default path keeps `resolve_model` verbatim | +| 2 | F1 tested at `run_project`, not the `run_portfolio` seam the finding names | minor | Added `assert all(r.provenance.model == "synthetic")` on the portfolio result in `test_a3` | +| 3 | `test_a3` did not specify how to locate the single Rejection | minor | Added explicit `rejected = [r ... if isinstance(r.outcome, Rejection)]; assert len(rejected) == 1` | +| 4 | F1 docstring fallout committed under an F2-only `test()` message | minor | Step-2 commit message now names "F2 + F1 follow-up"; docstring correction scoped into it | +| 5 | Em-dash in Step-2 commit message + `commit_message_pattern` on an em-dash-fragile machine | minor | Replaced with ASCII hyphen | +| 6 | Verify pinned absolute `116 passed` | minor | Kept (114 baseline now observed) + added relative "+2 passed, no new failures" framing | +| 7 | `82 source files` vs `14 source files` inconsistency | minor | Clarified the two counts measure repo vs `src/` scope | +| 8 | On-failure clauses did not state halt vs proceed | minor | Both steps now say revert then stop / do not proceed |