From 6ef4efcdd159a169caf45a2b0c41579297703057 Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Wed, 24 Jun 2026 15:10:07 +0200 Subject: [PATCH] =?UTF-8?q?docs(fase2):=20trekreview=20review.md=20?= =?UTF-8?q?=E2=80=94=20verdict=20BLOCK=20(2=20BLOCKER,=205=20MAJOR,=201=20?= =?UTF-8?q?MINOR)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent post-hoc review (high-effort) of the Fase 2 vertical slice (scope 9973d9f..308b553, 32 files). Two independent reviewers converged, ground-truth-verified against source: - F1 BLOCKER (run.py:124): maker-checker debate output discarded; the validated proposal comes from a separate generate_via_llm call. - F2 BLOCKER (budget.py:81): BudgetMiddleware defined+tested but never wired onto the debate; the NFR short-circuit path is absent. - F7/F3/F4/F8/F5 MAJOR, F9 MINOR. The slice's three load-bearing seams are built + unit-tested but not connected by the orchestrator; the green e2e suite asserts proposal shape only. Fase 2 not closed — awaits operator decision. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Fif1r1En5W542HbZV88yMH --- .../review.md | 233 ++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 .claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md diff --git a/.claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md b/.claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md new file mode 100644 index 0000000..66c042b --- /dev/null +++ b/.claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md @@ -0,0 +1,233 @@ +--- +type: trekreview +review_version: "1.0" +created: 2026-06-24 +task: "Fase 2 MVP vertical slice — one synthetic project end-to-end on MAF 1.9.0: debate → block-validate → HITL → learn" +slug: fase2-mvp-vertical-slice +project_dir: .claude/projects/2026-06-24-fase2-mvp-vertical-slice/ +brief_path: .claude/projects/2026-06-24-fase2-mvp-vertical-slice/brief.md +scope_sha_start: 9973d9f +scope_sha_end: 308b553 +reviewed_files_count: 32 +findings: + - 4d8f1a2c9b6e3057f4a1d8c2b9e0537a6c1f4e92 + - 7b3e9c1d8a4f2056e9b0c3d7f1a85249c6e3b801 + - 9f2a4d7c1e8b3056a4f9d2c7b1e08345f6c29d1b + - 2c7e1a9f4d8b3650c1e7a4f2d9b08537e6c14a90 + - 5a9d2f7c4e1b8360a9f4d7c2e1b95048f3c6a72d + - 8e1c4a7f2d9b5063c8a1e4f7d2b9043507c6e1a8 + - 3f7a1c9e4d2b8560f7a3c1e9d4b25081c6e7a34f + - 1d4f7a2c9e8b3650d1f4a7c2e9b08537a6c1e49d +--- + +# Review: Fase 2 MVP vertical slice — one synthetic project end-to-end on MAF 1.9.0: debate → block-validate → HITL → learn + +## Executive Summary + +Verdict: **BLOCK**. The slice's three load-bearing seams — the debate→validator dataflow, the ChatMiddleware token-budget enforcement, and the MCP-tool retrieval path — are each built and unit-tested in isolation but are NOT connected by the orchestrator; the e2e suite passes on proposal *shape* only and never asserts that the emitted proposal derives from the debate. Look first at F1 (debate output discarded at `run.py:124`) and F2 (BudgetMiddleware never wired): these two BLOCKERs mean the brief's core "candidate measures debated → validator decides value → enforced token budget" dataflow is decorative, not delivered. Master verification from execute is green (pytest 97 passed / 3 skipped — the 3 are operator-gated live-profile tests; ruff clean; mypy clean), which is exactly why these defects are dangerous: the suite is green *because* the unconnected seams are tested only against stand-ins. This was a full review (brief-conformance + code-correctness reviewers, both passes) in HIGH-EFFORT mode — Pass 3 (reasonableness filter) was skipped per the high-effort directive; Passes 1, 2, and 4 ran. + +## Coverage + +| File | Treatment | Reason | +|------|-----------|--------| +| `uv.lock` | skip | Generated lockfile (`*.lock` pattern) | +| `pyproject.toml` | summary-only | Config; in-scope, no deep dataflow | +| `src/portfolio_optimiser/run.py` | summary-only | Orchestrator entry; carries F1/F2/F5/F7/F9 | +| `src/portfolio_optimiser/budget.py` | summary-only | BudgetMiddleware definition; carries F2 | +| `src/portfolio_optimiser/backends.py` | summary-only | Backend profiles; carries F3 context | +| `src/portfolio_optimiser/workflow.py` | summary-only | `fresh_workflow` factory | +| `src/portfolio_optimiser/datasource.py` | summary-only | Retrieval/MCP tool; carries F7 context | +| `src/portfolio_optimiser/*.py` (remaining 9 modules) | summary-only | In-scope src modules | +| `src/portfolio_optimiser/data/model_map.json` | summary-only | Model-map config | +| `tests/*.py` (14 files) | summary-only | Test suite; carries F8 | + +32 files in scope: 1 `skip` (`uv.lock`), 31 `summary-only`, 0 `deep-review`. + +## Findings (BLOCKER) + +### 4d8f1a2c9b6e3057f4a1d8c2b9e0537a6c1f4e92 +- file: src/portfolio_optimiser/run.py +- line: 124 +- rule_key: BROKEN_SUCCESS_CRITERION +- brief_ref: Goal — run the debate (Group Chat maker-checker) → pass every candidate through the obligatory blocking validator → emit exactly one provenance-stamped proposal +- title: Maker-checker debate output is discarded; emitted candidate does not derive from the debate +- detail: run.py:124 runs `await debate.run(...)` as a bare statement whose return value is never captured. The validated/emitted proposal is produced separately by generate_via_llm(factory('proposer'), project, context, meter) at run.py:127, where context = retrieved doc snippets (run.py:118), not the debate transcript. Deleting run.py:124 would not change the emitted proposal — the debate is decorative; the brief's core "candidate measures debated → validator decides value" dataflow is not delivered. No test detects it: test_vertical_slice_e2e.py asserts only proposal shape, never debate origin. Also flagged by code-correctness-reviewer: "Debate output discarded; validated proposal comes from a separate unrelated call" (run.py:124, framed as PLAN_EXECUTE_DRIFT/MAJOR — folded here under the BLOCKER framing). +- recommended_action: Feed the debate's converged output (transcript or proposer IR) into candidate generation so the validated proposal provably derives from the debate; add an e2e assertion. + +### 7b3e9c1d8a4f2056e9b0c3d7f1a85249c6e3b801 +- file: src/portfolio_optimiser/budget.py +- line: 81 +- rule_key: BROKEN_SUCCESS_CRITERION +- brief_ref: NFR — "a token budget overrun short-circuits via the ChatMiddleware"; Goal — "a shared middleware-driven token budget" +- title: Specified ChatMiddleware token short-circuit is never wired onto the debate +- detail: BudgetMiddleware (budget.py:81) is defined + unit-tested but never imported/attached in src/: run.py never instantiates it, fresh_workflow (workflow.py) takes no middleware parameter and builds GroupChatBuilder without it, backends.py never attaches it. workflow.py:10 docstring claims it is "wired by the orchestrator" but run.py never does. The debate's per-call token usage is never metered/short-circuited (only with_max_rounds bounds it); the TokenMeter charged in generate_via_llm sees only the post-debate proposer call. The brief's mandated enforcement mechanism is structurally absent. SC7 still passes via the round cap + generate-loop meter, so this is a mechanism defect on the exact path the NFR names. Wiring-site pointer retained separately as F5 (run.py:121). +- recommended_action: Add a middleware/meter parameter to fresh_workflow, construct BudgetMiddleware(meter) in run_project, register on the debate's chat clients/GroupChat, add an e2e assertion that a tiny max_tokens halts the debate via the middleware. + +## Findings (MAJOR) + +### 9f2a4d7c1e8b3056a4f9d2c7b1e08345f6c29d1b +- file: src/portfolio_optimiser/backends.py +- line: 0 +- rule_key: PLAN_EXECUTE_DRIFT +- brief_ref: Plan Step 8 (manifest expected_path .env.template) implementing brief NFR "No silent egress — any egress explicit and configured" +- title: .env.template (Step 8 deliverable) not delivered; env/no-egress contract has no committed artifact +- detail: Plan Step 8 + Revision #6 made .env.template a required deliverable documenting PORTFOLIO_LOCAL_BASE_URL/API_KEY, Foundry endpoint/credential, no-egress notes. It is absent from the tree + diff; progress.json manifest_audit.status='drift' records it. The brief does not sanction the omission — it is an execute-time pathguard consequence, not a brief/plan exception. The no-egress NFR is only partially covered behaviorally (loopback _DEFAULT_LOCAL_BASE_URL in backends.py; loopback assertion in gated test_local_profile_live.py); the operator-facing env contract has no committed artifact. Content is staged → moderate risk, but the plan deliverable is unmet. +- recommended_action: Commit the staged template content under a pathguard-permitted name (e.g. env.template or docs/env.example.md) and reconcile the Step 8 manifest path. + +### 2c7e1a9f4d8b3650c1e7a4f2d9b08537e6c14a90 +- file: pyproject.toml +- line: 25 +- rule_key: PLAN_EXECUTE_DRIFT +- brief_ref: Plan Step 1 — "Move agent-framework-orchestrations and pulp from dev into [project.dependencies]" +- title: Dev dependencies restructured to PEP 735 [dependency-groups] beyond the stated Step 1 changes +- detail: Plan Step 1 authorized only moving orchestrations + pulp from the dev extra into core. The diff additionally converts the entire dev group from [project.optional-dependencies] to PEP 735 [dependency-groups] (pyproject.toml:25; uv.lock requires-dev), dropping provides-extras=['dev'] — a packaging-structure change not in the plan. Defensible (makes bare `uv sync` + `uv run pytest` install dev tooling without --extra, serving SC1 "Suite green") and low-risk, but approach-level divergence from the plan's stated Step 1 scope, unflagged as a deviation. +- recommended_action: Record the migration as an intentional Step 1 deviation in plan/progress notes, or revert to the extra-based structure if PEP 735 is not required. + +### 5a9d2f7c4e1b8360a9f4d7c2e1b95048f3c6a72d +- file: src/portfolio_optimiser/run.py +- line: 121 +- rule_key: PLAN_EXECUTE_DRIFT +- brief_ref: NFR — code correctness +- title: BudgetMiddleware never attached to any chat client in the orchestrator (wiring site) +- detail: run.py:121 builds a TokenMeter but never instantiates BudgetMiddleware (budget.py:81) nor attaches it to the debate's chat clients. The debate at run.py:124 runs with no token cap; the only guard is generate_via_llm's manual loop (a separate call). brief NFR requires short-circuit via ChatMiddleware; workflow.py:10 documents the orchestrator wires it, but it does not. BudgetMiddleware is dead in the runtime path. This is the wiring-site pointer for the same root defect as the F2 BLOCKER (definition site budget.py:81); retained for Actionability — it names the exact line where the construct-and-attach must happen. +- recommended_action: Construct BudgetMiddleware(meter) in run_project and attach to the chat clients passed into fresh_workflow. + +### 8e1c4a7f2d9b5063c8a1e4f7d2b9043507c6e1a8 +- file: src/portfolio_optimiser/run.py +- line: 114 +- rule_key: PLAN_EXECUTE_DRIFT +- brief_ref: NFR — code correctness +- title: Retrieval data-source tool never exposed to the agents; retrieval string-stuffed out-of-band +- detail: make_retrieval_tool / build_mcp_server (datasource.py:49/63) are never called from run.py. The agents in fresh_workflow receive no tools. run.py:114 calls retrieve_chunks once and run.py:118 concatenates snippets into a prompt string passed to the debate. brief Goal ("load its data via a local-folder MCP source with citations, run the debate") implies the agents reach the data source as a tool; as delivered the GA FunctionTool / MCP server are exercised only by test_datasource.py and are dead on the runtime path. +- recommended_action: Attach make_retrieval_tool(docs_dir, top_k=top_k) to the proposer/checker agents so they call the citation-bearing data source during the debate. + +### 3f7a1c9e4d2b8560f7a3c1e9d4b25081c6e7a34f +- file: tests/test_vertical_slice_e2e.py +- line: 74 +- rule_key: MISSING_TEST +- brief_ref: NFR — code correctness +- title: No test exercises BudgetMiddleware against a real chat client / on the debate path +- detail: test_e_tiny_budget_halts_without_exceeding (test_vertical_slice_e2e.py:74) asserts BudgetExceeded, but that is raised by generate_via_llm's manual meter loop, not by any BudgetMiddleware attached to a chat client. The only BudgetMiddleware test (test_budget.py:41-69) drives mw.process with a hand-rolled _Ctx stand-in, never a real BaseChatClient. The "short-circuits via the ChatMiddleware" NFR + the middleware↔real-client integration are untested; detaching the middleware would fail no test. +- recommended_action: Add a test registering BudgetMiddleware on a real synthetic-usage BaseChatClient, run an actual chat call, assert BudgetExceeded from the middleware path. + +## Findings (MINOR) + +### 1d4f7a2c9e8b3650d1f4a7c2e9b08537a6c1e49d +- file: src/portfolio_optimiser/run.py +- line: 131 +- rule_key: MISSING_ERROR_HANDLING +- brief_ref: NFR — code correctness +- title: Provenance hard-codes model='fake-model' whenever a client factory is injected +- detail: run.py:131 sets model = 'fake-model' if client_factory is not None else resolve_model(profile,'proposer'). The label is keyed off whether a factory was injected, not the real model. client_factory is documented as a general injection seam (run.py:101-102, "defaults to the real backend"), so an operator supplying a genuine custom factory gets a ProvenanceStamp falsely recording model='fake-model'. brief NFR requires the model be traceable on the proposal. +- recommended_action: Derive provenance model from the actual resolved model id instead of the literal 'fake-model'. + +## Suppressed Findings + +- F6 (code-correctness, run.py:124, PLAN_EXECUTE_DRIFT/MAJOR, "Debate output discarded; validated proposal comes from a separate unrelated call") — folded into F1 by Pass 1/Succinctness: same defect at the same file:line, BLOCKER framing retained. Attribution preserved in F1's detail. +- No findings were dropped by Pass 2 (all survived Succinctness, Accuracy, Actionability). Pass 3 (reasonableness) was skipped under high-effort mode. + +## Remediation Summary + +- BLOCKER: 2 +- MAJOR: 5 +- MINOR: 1 +- SUGGESTION: 0 + +`/trekplan --brief review.md` will consume the 2 BLOCKER + 5 MAJOR findings as the actionable remediation set: wire the debate output into candidate generation (F1), construct and attach BudgetMiddleware on the debate's chat clients (F2 + F5 wiring site), commit the Step 8 .env.template artifact (F3), record or revert the PEP 735 dependency restructuring (F4), expose make_retrieval_tool to the agents (F7), and add a real-client BudgetMiddleware integration test (F8); the MINOR provenance label fix (F9) is optional follow-on. + +```json +{ + "verdict": "BLOCK", + "counts": { "BLOCKER": 2, "MAJOR": 5, "MINOR": 1, "SUGGESTION": 0 }, + "findings": [ + { + "id": "4d8f1a2c9b6e3057f4a1d8c2b9e0537a6c1f4e92", + "severity": "BLOCKER", + "rule_key": "BROKEN_SUCCESS_CRITERION", + "file": "src/portfolio_optimiser/run.py", + "line": 124, + "brief_ref": "Goal — run the debate (Group Chat maker-checker) → pass every candidate through the obligatory blocking validator → emit exactly one provenance-stamped proposal", + "title": "Maker-checker debate output is discarded; emitted candidate does not derive from the debate", + "detail": "run.py:124 runs `await debate.run(...)` as a bare statement whose return value is never captured. The validated/emitted proposal is produced separately by generate_via_llm(factory('proposer'), project, context, meter) at run.py:127, where context = retrieved doc snippets (run.py:118), not the debate transcript. Deleting run.py:124 would not change the emitted proposal — the debate is decorative; the brief's core 'candidate measures debated → validator decides value' dataflow is not delivered. No test detects it: test_vertical_slice_e2e.py asserts only proposal shape, never debate origin. Also flagged by code-correctness-reviewer (run.py:124, PLAN_EXECUTE_DRIFT/MAJOR — folded here).", + "recommended_action": "Feed the debate's converged output (transcript or proposer IR) into candidate generation so the validated proposal provably derives from the debate; add an e2e assertion." + }, + { + "id": "7b3e9c1d8a4f2056e9b0c3d7f1a85249c6e3b801", + "severity": "BLOCKER", + "rule_key": "BROKEN_SUCCESS_CRITERION", + "file": "src/portfolio_optimiser/budget.py", + "line": 81, + "brief_ref": "NFR — 'a token budget overrun short-circuits via the ChatMiddleware'; Goal — 'a shared middleware-driven token budget'", + "title": "Specified ChatMiddleware token short-circuit is never wired onto the debate", + "detail": "BudgetMiddleware (budget.py:81) is defined + unit-tested but never imported/attached in src/: run.py never instantiates it, fresh_workflow (workflow.py) takes no middleware parameter and builds GroupChatBuilder without it, backends.py never attaches it. workflow.py:10 docstring claims it is 'wired by the orchestrator' but run.py never does. The debate's per-call token usage is never metered/short-circuited (only with_max_rounds bounds it). SC7 still passes via the round cap + generate-loop meter, so this is a mechanism defect on the exact path the NFR names. Wiring-site pointer retained separately as F5 (run.py:121).", + "recommended_action": "Add a middleware/meter parameter to fresh_workflow, construct BudgetMiddleware(meter) in run_project, register on the debate's chat clients/GroupChat, add an e2e assertion that a tiny max_tokens halts the debate via the middleware." + }, + { + "id": "9f2a4d7c1e8b3056a4f9d2c7b1e08345f6c29d1b", + "severity": "MAJOR", + "rule_key": "PLAN_EXECUTE_DRIFT", + "file": "src/portfolio_optimiser/backends.py", + "line": 0, + "brief_ref": "Plan Step 8 (manifest expected_path .env.template) implementing brief NFR 'No silent egress — any egress explicit and configured'", + "title": ".env.template (Step 8 deliverable) not delivered; env/no-egress contract has no committed artifact", + "detail": "Plan Step 8 + Revision #6 made .env.template a required deliverable documenting PORTFOLIO_LOCAL_BASE_URL/API_KEY, Foundry endpoint/credential, no-egress notes. It is absent from the tree + diff; progress.json manifest_audit.status='drift' records it. The brief does not sanction the omission — it is an execute-time pathguard consequence. The no-egress NFR is only partially covered behaviorally (loopback _DEFAULT_LOCAL_BASE_URL in backends.py; loopback assertion in gated test_local_profile_live.py); the operator-facing env contract has no committed artifact. Content is staged → moderate risk, but the plan deliverable is unmet.", + "recommended_action": "Commit the staged template content under a pathguard-permitted name (e.g. env.template or docs/env.example.md) and reconcile the Step 8 manifest path." + }, + { + "id": "2c7e1a9f4d8b3650c1e7a4f2d9b08537e6c14a90", + "severity": "MAJOR", + "rule_key": "PLAN_EXECUTE_DRIFT", + "file": "pyproject.toml", + "line": 25, + "brief_ref": "Plan Step 1 — 'Move agent-framework-orchestrations and pulp from dev into [project.dependencies]'", + "title": "Dev dependencies restructured to PEP 735 [dependency-groups] beyond the stated Step 1 changes", + "detail": "Plan Step 1 authorized only moving orchestrations + pulp from the dev extra into core. The diff additionally converts the entire dev group from [project.optional-dependencies] to PEP 735 [dependency-groups] (pyproject.toml:25; uv.lock requires-dev), dropping provides-extras=['dev'] — a packaging-structure change not in the plan. Defensible (makes bare `uv sync` + `uv run pytest` install dev tooling without --extra, serving SC1 'Suite green') and low-risk, but approach-level divergence from the plan's stated Step 1 scope, unflagged as a deviation.", + "recommended_action": "Record the migration as an intentional Step 1 deviation in plan/progress notes, or revert to the extra-based structure if PEP 735 is not required." + }, + { + "id": "5a9d2f7c4e1b8360a9f4d7c2e1b95048f3c6a72d", + "severity": "MAJOR", + "rule_key": "PLAN_EXECUTE_DRIFT", + "file": "src/portfolio_optimiser/run.py", + "line": 121, + "brief_ref": "NFR — code correctness", + "title": "BudgetMiddleware never attached to any chat client in the orchestrator (wiring site)", + "detail": "run.py:121 builds a TokenMeter but never instantiates BudgetMiddleware (budget.py:81) nor attaches it to the debate's chat clients. The debate at run.py:124 runs with no token cap; the only guard is generate_via_llm's manual loop (a separate call). brief NFR requires short-circuit via ChatMiddleware; workflow.py:10 documents the orchestrator wires it, but it does not. BudgetMiddleware is dead in the runtime path. Wiring-site pointer for the same root defect as the F2 BLOCKER (definition site budget.py:81); retained for Actionability.", + "recommended_action": "Construct BudgetMiddleware(meter) in run_project and attach to the chat clients passed into fresh_workflow." + }, + { + "id": "8e1c4a7f2d9b5063c8a1e4f7d2b9043507c6e1a8", + "severity": "MAJOR", + "rule_key": "PLAN_EXECUTE_DRIFT", + "file": "src/portfolio_optimiser/run.py", + "line": 114, + "brief_ref": "NFR — code correctness", + "title": "Retrieval data-source tool never exposed to the agents; retrieval string-stuffed out-of-band", + "detail": "make_retrieval_tool / build_mcp_server (datasource.py:49/63) are never called from run.py. The agents in fresh_workflow receive no tools. run.py:114 calls retrieve_chunks once and run.py:118 concatenates snippets into a prompt string passed to the debate. brief Goal ('load its data via a local-folder MCP source with citations, run the debate') implies the agents reach the data source as a tool; as delivered the GA FunctionTool / MCP server are exercised only by test_datasource.py and are dead on the runtime path.", + "recommended_action": "Attach make_retrieval_tool(docs_dir, top_k=top_k) to the proposer/checker agents so they call the citation-bearing data source during the debate." + }, + { + "id": "3f7a1c9e4d2b8560f7a3c1e9d4b25081c6e7a34f", + "severity": "MAJOR", + "rule_key": "MISSING_TEST", + "file": "tests/test_vertical_slice_e2e.py", + "line": 74, + "brief_ref": "NFR — code correctness", + "title": "No test exercises BudgetMiddleware against a real chat client / on the debate path", + "detail": "test_e_tiny_budget_halts_without_exceeding (test_vertical_slice_e2e.py:74) asserts BudgetExceeded, but that is raised by generate_via_llm's manual meter loop, not by any BudgetMiddleware attached to a chat client. The only BudgetMiddleware test (test_budget.py:41-69) drives mw.process with a hand-rolled _Ctx stand-in, never a real BaseChatClient. The 'short-circuits via the ChatMiddleware' NFR + the middleware↔real-client integration are untested; detaching the middleware would fail no test.", + "recommended_action": "Add a test registering BudgetMiddleware on a real synthetic-usage BaseChatClient, run an actual chat call, assert BudgetExceeded from the middleware path." + }, + { + "id": "1d4f7a2c9e8b3650d1f4a7c2e9b08537a6c1e49d", + "severity": "MINOR", + "rule_key": "MISSING_ERROR_HANDLING", + "file": "src/portfolio_optimiser/run.py", + "line": 131, + "brief_ref": "NFR — code correctness", + "title": "Provenance hard-codes model='fake-model' whenever a client factory is injected", + "detail": "run.py:131 sets model = 'fake-model' if client_factory is not None else resolve_model(profile,'proposer'). The label is keyed off whether a factory was injected, not the real model. client_factory is documented as a general injection seam (run.py:101-102, 'defaults to the real backend'), so an operator supplying a genuine custom factory gets a ProvenanceStamp falsely recording model='fake-model'. brief NFR requires the model be traceable on the proposal.", + "recommended_action": "Derive provenance model from the actual resolved model id instead of the literal 'fake-model'." + } + ] +} +```