docs(fase2): trekreview review.md — verdict BLOCK (2 BLOCKER, 5 MAJOR, 1 MINOR)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fif1r1En5W542HbZV88yMH
This commit is contained in:
parent
308b5539e2
commit
6ef4efcdd1
1 changed files with 233 additions and 0 deletions
233
.claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md
Normal file
233
.claude/projects/2026-06-24-fase2-mvp-vertical-slice/review.md
Normal file
|
|
@ -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'."
|
||||
}
|
||||
]
|
||||
}
|
||||
```
|
||||
Loading…
Add table
Add a link
Reference in a new issue