docs(fase3): remediation plan for the 2 /trekreview MAJOR findings (F1+F2)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019any9zfGNNwWJPX5Zq2QRz
2026-06-26 15:07:48 +02:00 · 2026-06-26 15:07:48 +02:00 · 55f98f2af7
commit 55f98f2af7
parent a7c69dbd55
1 changed files with 260 additions and 0 deletions
--- a/.claude/projects/2026-06-26-fase3-portfolio-fanout/plan-remediation.md
+++ 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 |