docs(fase2): trekreview review.md — verdict ALLOW (0 funn), Fase 2 LUKKET
Remediation review of scope 6ef4efc..01c3f0d (high-effort, both reviewers). Two independent reviewers (brief-conformance + code-correctness) each returned zero findings after verifying the load-bearing detach criterion against the actual code and the installed MAF source — explicitly refusing to treat the green suite (103 passed / 3 skipped, offline) as evidence. All 7 actionable original findings (2 BLOCKER + 5 MAJOR) confirmed RESOLVED: F1 debate→generation (test_g), F2/F5/F8 BudgetMiddleware (test_h + test_budget.py:70; conftest re-base verified necessary vs _clients.py:214-231), F7 retrieval-tool exposure (spy tests). Coordinator verdict: ALLOW. Standing items (not findings): F9 MINOR deferred, SC9 real-profile coverage, retrieval-exposed-not-invoked, strict_usage fail-closed untested offline. 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
01c3f0dc8a
commit
fc167deb59
1 changed files with 48 additions and 195 deletions
|
|
@ -1,233 +1,86 @@
|
|||
---
|
||||
type: trekreview
|
||||
review_version: "1.0"
|
||||
created: 2026-06-24
|
||||
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: 9973d9f
|
||||
scope_sha_end: 308b553
|
||||
reviewed_files_count: 32
|
||||
findings:
|
||||
- 4d8f1a2c9b6e3057f4a1d8c2b9e0537a6c1f4e92
|
||||
- 7b3e9c1d8a4f2056e9b0c3d7f1a85249c6e3b801
|
||||
- 9f2a4d7c1e8b3056a4f9d2c7b1e08345f6c29d1b
|
||||
- 2c7e1a9f4d8b3650c1e7a4f2d9b08537e6c14a90
|
||||
- 5a9d2f7c4e1b8360a9f4d7c2e1b95048f3c6a72d
|
||||
- 8e1c4a7f2d9b5063c8a1e4f7d2b9043507c6e1a8
|
||||
- 3f7a1c9e4d2b8560f7a3c1e9d4b25081c6e7a34f
|
||||
- 1d4f7a2c9e8b3650d1f4a7c2e9b08537a6c1e49d
|
||||
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: **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.
|
||||
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 |
|
||||
|------|-----------|--------|
|
||||
| `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 |
|
||||
| `.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. |
|
||||
|
||||
32 files in scope: 1 `skip` (`uv.lock`), 31 `summary-only`, 0 `deep-review`.
|
||||
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)
|
||||
|
||||
### 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.
|
||||
None.
|
||||
|
||||
## 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.
|
||||
None.
|
||||
|
||||
## 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'.
|
||||
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
|
||||
|
||||
- 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.
|
||||
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: 2
|
||||
- MAJOR: 5
|
||||
- MINOR: 1
|
||||
- BLOCKER: 0
|
||||
- MAJOR: 0
|
||||
- MINOR: 0
|
||||
- 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.
|
||||
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": "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'."
|
||||
}
|
||||
]
|
||||
}
|
||||
{"verdict":"ALLOW","counts":{"BLOCKER":0,"MAJOR":0,"MINOR":0,"SUGGESTION":0},"findings":[]}
|
||||
```
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue