From 4253dd6388b0ce3a126ec4d6b5093af26567f0b5 Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Fri, 26 Jun 2026 14:08:34 +0200 Subject: [PATCH] =?UTF-8?q?review(fase3):=20/trekreview=20WARN=20=E2=80=94?= =?UTF-8?q?=202=20MAJOR=20(provenance=20fake-model=20+=20rejection-arm=20u?= =?UTF-8?q?testet)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_019any9zfGNNwWJPX5Zq2QRz --- .../review.md | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 .claude/projects/2026-06-26-fase3-portfolio-fanout/review.md diff --git a/.claude/projects/2026-06-26-fase3-portfolio-fanout/review.md b/.claude/projects/2026-06-26-fase3-portfolio-fanout/review.md new file mode 100644 index 0000000..05bce14 --- /dev/null +++ b/.claude/projects/2026-06-26-fase3-portfolio-fanout/review.md @@ -0,0 +1,121 @@ +--- +type: trekreview +review_version: "1.0" +created: 2026-06-26 +task: "Fase 3: orkestrator fan-out over N porteføljeprosjekter med state-isolasjon + delt læring" +slug: fase3-portfolio-fanout +project_dir: .claude/projects/2026-06-26-fase3-portfolio-fanout/ +brief_path: .claude/projects/2026-06-26-fase3-portfolio-fanout/brief.md +scope_sha_start: 497399e0aa523208b3188f0c8ca54627ec5863b7 +scope_sha_end: 207f0570755cc5f0d664cfb16f18a5a00aee1ef0 +reviewed_files_count: 15 +verdict: WARN +findings: + - b3abbdd7035b5463b3bb9e23f83a158d678cb4f1 + - a2376be90affdffea4c9b4ef83014d0a30243e23 +--- + +# Review: Fase 3 — orkestrator fan-out over N porteføljeprosjekter med state-isolasjon + delt læring + +## Executive Summary + +Verdict: **WARN** — 0 BLOCKER, 2 MAJOR. Look first at the `fake-model` placeholder leak in `run.py:197`: it stamps a hardcoded sentinel into `ProvenanceStamp.model` for any deployer who supplies a custom `client_factory`, falsifying a load-bearing provenance invariant on every project in the fan-out. The second MAJOR is a coverage gap — the portfolio `rejected_count` / validated-only-sum branch (`run.py:240-241`) is never exercised by an actual rejection, so a regression there ships green under SC2. Two honesty caveats: SC5 (full-suite green / mypy / ruff) was a runtime gate the reviewers could not execute and declined to fabricate, so its pass is asserted from code shape only, not observed; and this is a HIGH-effort review, so Pass 3 (Cloudflare reasonableness filter) was skipped by design — borderline findings are left for the operator to weigh. Both incoming `rule_key`s (`PLACEHOLDER_IN_CODE`, `MISSING_TEST`) are valid catalogue keys, so no high-effort rule_key normalization was applied. + +## Coverage + +15 files reviewed, all `summary-only`. 0 files marked `skip` → 0 silent skips. Working tree clean → no `[uncommitted]` annotations. + +| File | Treatment | Reason | +|------|-----------|--------| +| docs/extending.md | summary-only | SC6 extension-point doc; prose, no executable surface | +| src/portfolio_optimiser/__init__.py | summary-only | Package exports; mechanical | +| src/portfolio_optimiser/data/docs/BRU-LAKS-REHAB/notes.txt | summary-only | Synthetic project fixture data | +| src/portfolio_optimiser/data/docs/FV42-GSV-E1/notes.txt | summary-only | Synthetic project fixture data | +| src/portfolio_optimiser/data/docs/RV13-RAS-TP/notes.txt | summary-only | Synthetic project fixture data | +| src/portfolio_optimiser/data/docs/SKOLE-VVS-OPPGR/notes.txt | summary-only | Synthetic project fixture data | +| src/portfolio_optimiser/data/reference_projects.json | summary-only | Config-only project registry (SC1 seam) | +| src/portfolio_optimiser/reference_domain.py | summary-only | Domain reference module; no diff-critical logic flagged | +| src/portfolio_optimiser/run.py | summary-only | Orchestrator fan-out + aggregate; both MAJOR findings land here | +| tests/conftest.py | summary-only | Shared fixtures | +| tests/spikes/test_b_footguns.py | summary-only | SC1 no-hardcoded-id / 4th-project spike guard | +| tests/test_portfolio.py | summary-only | SC2 aggregate; asserts rejected_count==0 (root of MISSING_TEST gap) | +| tests/test_portfolio_live.py | summary-only | SC7 gated Azure portfolio arm (skip-gated) | +| tests/test_reference_domain.py | summary-only | Domain reference tests | +| tests/test_vertical_slice_e2e.py | summary-only | End-to-end slice | + +## Findings (MAJOR) + +### b3abbdd7035b5463b3bb9e23f83a158d678cb4f1 +- file: src/portfolio_optimiser/run.py +- line: 197 +- rule_key: PLACEHOLDER_IN_CODE +- brief_ref: NFR — code correctness +- title: fake-model placeholder leaks into provenance via public client_factory seam +- detail: `run.py:197` sets `model = 'fake-model' if client_factory is not None else resolve_model(profile, 'proposer')`, writing a hardcoded sentinel into `ProvenanceStamp.model` (run.py:197-204). The gate `client_factory is not None` is a public, documented seam threaded through `run_portfolio(..., client_factory=...)` (run.py:251,278), so a deployer with a custom backend factory stamps `'fake-model'` across all N provenance records, falsifying the load-bearing provenance invariant. Only the default (no injected factory) path is compensated by `resolve_model`. +- recommended_action: Resolve the real model identifier from the injected `client_factory` (or require the factory to surface its model) instead of branching to the `'fake-model'` literal; reserve the sentinel for test-only fixtures, not the public seam. + +### a2376be90affdffea4c9b4ef83014d0a30243e23 +- file: src/portfolio_optimiser/run.py +- line: 241 +- rule_key: MISSING_TEST +- brief_ref: NFR — code correctness +- title: Portfolio rejected_count and validated-only sum never exercised by a rejection +- detail: `_aggregate` computes `rejected_count=len(rejected)` (run.py:240) and a validated-only `sum_claimed_saving_nok` (run.py:241). Every portfolio test feeds proposals that validate — `test_portfolio.py:63` asserts `rejected_count==0` — so no `run_portfolio` test produces a Rejection. The rejected branch and the validated-only filter are unproven: a regression that sums rejected claims or miscounts rejections would pass green while SC2 still claims the aggregate is correct. +- recommended_action: Add a `run_portfolio` test that feeds at least one proposal the deterministic validator rejects, then assert `rejected_count >= 1` and that `sum_claimed_saving_nok` excludes the rejected claim. + +## Suppressed Findings + +None. No finding was dropped by Pass 1 (dedup) or Pass 2 (HubSpot Judge). Pass 3 (reasonableness) was skipped under HIGH effort mode. + +## Conformance trace + +From `brief-conformance-reviewer` (`findings: []` — zero conformance/scope findings): + +- **SC1** — config-only new project: PASS (guarded by no-hardcoded-id grep + 4th-project spike). +- **SC2** — fan-out aggregate: PASS shape-wise, but see MAJOR `a2376…` — the rejected/validated-only branch is unexercised. +- **SC3** — state-isolation (`==`): PASS, load-bearing (shared meter detach breaks the test). +- **SC4** — shared VerdictStore accumulation: PASS, load-bearing. +- **SC5** — full-suite green / mypy / ruff: NOT OBSERVED — runtime gate the reviewers could not execute; no fabricated finding emitted. Operator must confirm at the gate. +- **SC6** — extension doc: PASS (`docs/extending.md`). +- **SC7** — both profiles offline; live-Azure skip-gated: PASS. +- **Non-Goals** — all honored: no concurrency, no dynamic topology, no rich report, one shared store, live-Azure skip-gated, no compliance code. Validator/IR have zero diff (out of scope by absence). + +## Remediation Summary + +- BLOCKER: 0 +- MAJOR: 2 +- MINOR: 0 +- SUGGESTION: 0 + +/trekplan (`--brief review.md`) will consume the two MAJOR findings from the trailing JSON block: fix the `fake-model` provenance leak on the public `client_factory` seam, and add a rejection-path test for the portfolio aggregate. Fase 3 closes on WARN per the gate (a BLOCK would require a BLOCKER). + +```json +{ + "verdict": "WARN", + "counts": { "BLOCKER": 0, "MAJOR": 2, "MINOR": 0, "SUGGESTION": 0 }, + "findings": [ + { + "id": "b3abbdd7035b5463b3bb9e23f83a158d678cb4f1", + "severity": "MAJOR", + "rule_key": "PLACEHOLDER_IN_CODE", + "file": "src/portfolio_optimiser/run.py", + "line": 197, + "brief_ref": "NFR — code correctness", + "title": "fake-model placeholder leaks into provenance via public client_factory seam", + "detail": "run.py:197 sets model = 'fake-model' if client_factory is not None else resolve_model(profile, 'proposer'), writing a hardcoded sentinel into ProvenanceStamp.model (run.py:197-204). The gate client_factory is not None is a public, documented seam threaded through run_portfolio(..., client_factory=...) (run.py:251,278), so a deployer with a custom backend factory stamps 'fake-model' across all N provenance records, falsifying the load-bearing provenance invariant. Only the default (no injected factory) path is compensated by resolve_model.", + "recommended_action": "Resolve the real model identifier from the injected client_factory (or require the factory to surface its model) instead of branching to the 'fake-model' literal; reserve the sentinel for test-only fixtures, not the public seam." + }, + { + "id": "a2376be90affdffea4c9b4ef83014d0a30243e23", + "severity": "MAJOR", + "rule_key": "MISSING_TEST", + "file": "src/portfolio_optimiser/run.py", + "line": 241, + "brief_ref": "NFR — code correctness", + "title": "Portfolio rejected_count and validated-only sum never exercised by a rejection", + "detail": "_aggregate computes rejected_count=len(rejected) (run.py:240) and a validated-only sum_claimed_saving_nok (run.py:241). Every portfolio test feeds proposals that validate — test_portfolio.py:63 asserts rejected_count==0 — so no run_portfolio test produces a Rejection. The rejected branch and the validated-only filter are unproven: a regression that sums rejected claims or miscounts rejections would pass green while SC2 still claims the aggregate is correct.", + "recommended_action": "Add a run_portfolio test that feeds at least one proposal the deterministic validator rejects, then assert rejected_count >= 1 and that sum_claimed_saving_nok excludes the rejected claim." + } + ] +} +```