---
type: trekreview
review_version: "1.0"
created: 2026-06-26
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: 6ef4efc
scope_sha_end: 01c3f0d
reviewed_files_count: 9
findings: []
---

# 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.

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