portfolio-optimiser/.claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md

type	review_version	created	task	slug	project_dir	brief_path	scope_sha_start	scope_sha_end	reviewed_files_count	findings
trekreview	1.0	2026-06-24	Fase 2 MVP vertical slice — one synthetic project end-to-end on MAF 1.9.0: debate → block-validate → HITL → learn	fase2-mvp-vertical-slice	.claude/projects/2026-06-24-fase2-mvp-vertical-slice/	.claude/projects/2026-06-24-fase2-mvp-vertical-slice/brief.md	9973d9f	308b553	32	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.

{
  "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'."
    }
  ]
}