portfolio-optimiser/.claude/projects/2026-06-26-fase3-portfolio-fanout/plan-remediation.md

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

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:

  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:

  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