review(fase3): /trekreview WARN — 2 MAJOR (provenance fake-model + rejection-arm utestet)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019any9zfGNNwWJPX5Zq2QRz
This commit is contained in:
parent
207f057075
commit
4253dd6388
1 changed files with 121 additions and 0 deletions
121
.claude/projects/2026-06-26-fase3-portfolio-fanout/review.md
Normal file
121
.claude/projects/2026-06-26-fase3-portfolio-fanout/review.md
Normal file
|
|
@ -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."
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
Loading…
Add table
Add a link
Reference in a new issue