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-26	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	6ef4efc	01c3f0d	9

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: ALLOW — zero findings after dedup and the HubSpot Judge filters. This is a remediation review: the first review of this task (scope 9973d9f..308b553) returned BLOCK (2 BLOCKER + 5 MAJOR + 1 MINOR) because the slice's three load-bearing seams — debate→validator dataflow, ChatMiddleware token-budget enforcement, and the MCP-tool retrieval path — were unit-tested in isolation but never connected by the orchestrator, so the e2e suite passed on proposal shape only (green-but-dead). The remediation (plan-remediation.md, 7 steps, commits bf0bbff..01c3f0d) wired all three seams; both independent reviewers verified the new tests are genuinely load-bearing by the detach criterion (each fails when its seam is removed) and explicitly refused to treat the green suite as evidence. Standing items remain (F9 deferred MINOR, SC9 real-profile coverage, retrieval-exposed-not-invoked, untested strict_usage fail-closed) — these are named below and are NOT findings; do not read ALLOW as "nothing left to do."

Coverage

File	Treatment	Reason
.claude/projects/2026-06-24-fase2-mvp-vertical-slice/plan-remediation.md [uncommitted-tracked]	summary-only	Remediation plan of record (7 steps); documentation, no executable seam.
env.template	summary-only	Resolves F3 — documents both profiles + 127.0.0.1 + no-egress notes; config artifact.
pyproject.toml	summary-only	Resolves F4 — records intentional PEP 735 [dependency-groups] deviation (:26-31); agent_framework_openai declared as core dep.
src/portfolio_optimiser/run.py	summary-only	Orchestrator — the file where all three seams were previously unwired. F1 (:162-169,166), F2/F5/F8 (:153,160), F7 (:154,159) now wired; F9 (:173) deferred residual.
src/portfolio_optimiser/workflow.py	summary-only	Attaches middleware + tools to every Agent (:62-63); output_from=[agents[0]] (:105) surfaces proposer's converged output for F1.
tests/conftest.py	summary-only	:27 re-bases SyntheticUsageChatClient onto OpenAIChatCompletionClient — verified NECESSARY (the prior BaseChatClient base silently no-op'd middleware: the exact green-but-dead trap).
tests/test_budget.py	summary-only	:70 real agent.run("hi") asserts BudgetExceeded + meter.tokens==8; load-bearing for F2/F5/F8.
tests/test_vertical_slice_e2e.py	summary-only	test_g (F1 marker in debate_output + generation context), test_h (F2 — BudgetExceeded can only originate in debate middleware); both fail on detach.
tests/test_workflow.py	summary-only	Construction-spy + wiring-spy tests for retrieval-tool exposure (F7) and middleware attachment.

Triage map: 9 files in scope, all summary-only, 0 skipped. Independent ground-truth check at HEAD (01c3f0d): offline suite 103 passed / 3 skipped — the 3 skips are operator-gated live-profile (Azure) tests, not silent drops.

Confirmed remediation outcomes (folded from both reviewers' verified conclusions):

• F1 — debate output discarded → RESOLVED. run.py:162-169 extracts the proposer's converged output (surfaced via output_from=[agents[0]], workflow.py:105) and feeds it via gen_context = debate_output or context (run.py:166). test_g asserts the "Reduce scope" marker (in _VALID, absent from the docs_dir fixture) appears in both result.debate_output and the captured generation context — fails if either seam reverts. output_from verified a real GroupChatBuilder.__init__ param (_group_chat.py:629).
• F2/F5/F8 — BudgetMiddleware never wired / no real-client test → RESOLVED. run.py:153,160 constructs BudgetMiddleware(meter) and passes middleware=[...] into fresh_workflow; workflow.py:62-63 attaches to every Agent. conftest.py:27 re-base verified NECESSARY against _clients.py:214-231. test_budget.py:70 and test_h both fail on detach.
• F7 — retrieval tool not exposed → RESOLVED. run.py:154,159 builds make_retrieval_tool, passed via tools=[...]; workflow.py:62 attaches per agent; spy tests assert it. (Exposure, not invocation-reliability — per F7's recommendation + 90% principle.)
• F3 — env contract → RESOLVED (env.template).
• F4 — PEP 735 deviation unrecorded → RESOLVED (pyproject.toml:26-31).
• Security: exposing the retrieval tool creates no injection vector — LLM-controlled query reaches only in-memory keyword scoring; docs_dir is boundary-checked, fail-closed against ../symlink/prefix-collision (retrieval.py:48-70). New keyword-only params default to None; all call sites updated or default-covered; no regression.

Findings (BLOCKER)

None.

Findings (MAJOR)

None.

Findings (MINOR)

None — verdict ALLOW. See Standing items for the deferred F9 MINOR (not re-raised as a finding because it is unchanged by this diff and explicitly deferred by the remediation plan).

Findings (SUGGESTION)

None.

Standing items (NOT findings)

These are honest residuals carried forward, not defects introduced or left unaddressed by this remediation. They do not affect the mechanical verdict and are recorded so ALLOW is not mistaken for "complete":

• F9 (MINOR, deferred) — model = "fake-model" keyed off client_factory is not None (run.py:173) is still present, unchanged by this diff. Explicitly deferred by the remediation plan (Handover 6 consumes only BLOCKER + MAJOR). Real but latent; deserves the planned follow-on, not closure here.
• SC9 real-profile coverage — exercised in CI only via the offline synthetic client (local) plus operator-gated/skipped live arms (Azure). Unchanged from round 1, which accepted it; the brief frames concrete endpoints as operator-supplied OPEN items. Not a remediation defect.
• Retrieval tool exposed but not proven invoked by agents — the synthetic client ignores tools=. Matches F7's recommendation + the 90% principle.
• strict_usage=True on the budget middleware — on the gated real backend, a debate response lacking usage_details.total_token_count raises UsageUnavailable (intentional fail-closed; untested offline).

Suppressed Findings

None — both reviewers returned zero findings; nothing was dropped by Pass 1 (dedup) or Pass 2 (HubSpot Judge filters). Pass 3 (Cloudflare reasonableness) was bypassed under high-effort mode; with an empty findings set there were no rule_keys to normalize.

Remediation Summary

• BLOCKER: 0
• MAJOR: 0
• MINOR: 0
• SUGGESTION: 0

Clean ALLOW resolving a prior BLOCK: all 7 actionable original findings (2 BLOCKER + 5 MAJOR) are verified RESOLVED with genuinely load-bearing tests (detach criterion confirmed by both reviewers). /trekplan --brief review.md will consume an empty findings set — there is no remediation work to schedule from this review. The four named Standing items above remain operator-owned follow-ons (notably the deferred F9 MINOR) and are not actionable findings.

{"verdict":"ALLOW","counts":{"BLOCKER":0,"MAJOR":0,"MINOR":0,"SUGGESTION":0},"findings":[]}