Kjell Tore Guttormsen 01c3f0dc8a docs(fase2): remediation plan (Handover 6) — consumes 2 BLOCKER + 5 MAJOR

2026-06-26 00:46:23 +02:00

39 KiB

Raw Blame History

Fase 2 MVP Vertical Slice — Remediation (connect the three load-bearing seams)

Plan quality: A (91/100) — APPROVE_WITH_NOTES

Generated by trekplan v5.6.1 on 2026-06-26 — plan_version: 1.7

Input: review.md (type:trekreview, verdict BLOCK). Consumes the 2 BLOCKER + 5 MAJOR findings as the actionable remediation set (MINOR F9 noted optional). Destination note (deviation from default): the skill's default --brief+project_dir destination is {project_dir}/plan.md, which is the ORIGINAL Fase 2 plan (the audit trail referenced by progress.json + review.md). This remediation is written to plan-remediation.md to avoid clobbering that record.

Context

/trekreview returned BLOCK: the slice's three load-bearing seams — the debate→validator dataflow, the ChatMiddleware token-budget enforcement, and the MCP/retrieval tool path — are each built and unit-tested in isolation but are never connected by the orchestrator (run.py). The e2e suite is green because it tests the pieces against stand-ins and asserts proposal shape only. This plan wires the seams together so the brief's core contract — "candidate measures debated → a deterministic validator decides the value → enforced via a shared ChatMiddleware token budget, provenance-stamped" (brief Goal + NFR) — is actually delivered, not decorative.

The danger the review surfaced is that the green suite masks the defect. So a hard constraint on this remediation: the tests must exercise the real mechanism, not a stand-in. The single most important discovery during planning is that the existing test double (SyntheticUsageChatClient) extends the minimal BaseChatClient, which has no ChatMiddlewareLayer — so attaching the budget middleware to the debate agents would silently no-op in every test, reproducing the exact "green tests, dead mechanism" failure the review caught. The remediation therefore fixes the test double (Step 1) before wiring the mechanism (Steps 2–4), and proves the mechanism fires on a real layered client (Step 5).

Architecture Diagram

graph TD
    subgraph "run_project orchestrator (run.py) — the seams this plan connects"
        meter["TokenMeter(Budget)"]
        mw["BudgetMiddleware(meter)  (NEW wiring)"]
        tool["make_retrieval_tool(docs_dir)  (NEW wiring)"]
        fw["fresh_workflow(factory, tools=[tool], middleware=[mw])"]
        debate["debate = GroupChat (proposer/checker)"]
        run["result = await debate.run(...)  → get_outputs()  (NEW: captured)"]
        gen["generate_via_llm(proposer, debate_output+context, meter)"]
        val["validate_proposal → ValidatedProposal | Rejection"]
        stamp["ProvenanceStamp(token_usage=meter.tokens)"]

        meter --> mw
        mw --> fw
        tool --> fw
        fw --> debate
        debate --> run
        run -->|debate_output| gen
        gen --> val
        val --> stamp
    end
    subgraph "MAF 1.9.0 (installed-source-verified)"
        layer["Agent(client, tools=, middleware=) — middleware fires only if client has ChatMiddlewareLayer"]
        mw -.attaches via.-> layer
        tool -.attaches via.-> layer
    end

Codebase Analysis

Tech stack: Python ≥3.10, uv, ruff, mypy, pytest (asyncio_mode=auto), Pydantic 2.x. MAF agent-framework-core 1.9.0 + -openai/-foundry/-orchestrations.
Key patterns: fail-fast contracts before any chat client; first-class ProvenanceStamp; validator-as-retry as the reliability mechanism; fresh_workflow isolation factory; injected client_factory test seam returning synthetic-usage clients.
Relevant files (all real, verified):
- src/portfolio_optimiser/run.py — orchestrator; carries F1, F2-wiring (F5), F7-wiring, F9.
- src/portfolio_optimiser/workflow.py — fresh_workflow/maker_checker_agents; no tools/middleware params today.
- src/portfolio_optimiser/budget.py — BudgetMiddleware(ChatMiddleware) (defined, unwired).
- src/portfolio_optimiser/datasource.py — make_retrieval_tool → GA FunctionTool (defined, unexposed).
- src/portfolio_optimiser/generate.py — generate_via_llm (async parse→validate→retry, meter-bounded).
- src/portfolio_optimiser/backends.py — OpenAIChatCompletionClient (LOCAL) / FoundryChatClient (AZURE).
- tests/conftest.py — SyntheticUsageChatClient(BaseChatClient) ← minimal base, no middleware layer.
- tests/test_vertical_slice_e2e.py, tests/test_budget.py, tests/test_workflow.py.
- pyproject.toml — [dependency-groups] (PEP 735) dev group.
Reusable code: BudgetMiddleware, make_retrieval_tool, fresh_workflow, generate_via_llm, SyntheticUsageChatClient (to be re-based), all e2e fixtures.
Recent git activity: 308b553 closed /trekexecute (14/14); 6ef4efc recorded the BLOCK review. Branch main, clean.

Research Sources

Installed-source introspection (MAF 1.9.0) performed during planning is the authority ("installed source wins" — brief Constraints). Each mechanism below was verified empirically, not assumed.

Mechanism	Source	Key finding	Confidence
`Agent(tools=, middleware=)`	`agent_framework._agents.Agent.__init__` (introspected)	Both kwargs exist; `ChatMiddleware ∈ MiddlewareTypes`	high
Middleware application	`OpenAIChatCompletionClient.__mro__` = `…→ChatMiddlewareLayer→…→BaseChatClient`	Chat middleware fires only if the client carries `ChatMiddlewareLayer`; minimal `BaseChatClient` does NOT	high
Middleware fires + propagates + override + strict_usage	empirical run (offline: layered synthetic client subclassing `OpenAIChatCompletionClient`, tiny cap)	`BudgetExceeded` raised by middleware propagates out of `debate.run()`; the `_inner_get_response(, messages, stream, options, kwargs)` override intercepts offline (no network call); the synthetic `UsageDetails` reaches `context.result`* inside the real middleware layer — it raised `BudgetExceeded`, NOT `UsageUnavailable`, so `strict_usage=True` propagation holds	high
`debate.run()` output capture	empirical run (FakeChatClient debate to completion)	`get_outputs()` returns `AgentResponse` (has `.text`/`.messages`); by DEFAULT it surfaces only the orchestrator's "reached max rounds" notice (author `group_chat_orchestrator`) — the proposer's converged text is surfaced ONLY when the builder sets `output_from=[proposer]`. `get_intermediate_outputs()` is empty; the event list carries no participant `.text`	high
Local client / UsageDetails	research 03 (`research/03-local-profile-chat-client.md`)	`OpenAIChatCompletionClient` non-streaming populates `UsageDetails` None-safely	high
Pathguard on `.env*`	`~/.claude/hooks/pre-write-pathguard.sh:90` regex `\.env(\.[a-zA-Z]+)?$`	`env.template` (no leading dot) is permitted; `.env.template` is blocked	high

Implementation Plan

Ordered by dependency. Steps 1–5 keep the suite green at every checkpoint; the test double is re-based (Step 1) before the mechanism is wired (Steps 2–4) so the mechanism is provable, not theater.

Step 1: Re-base the synthetic test client on the layered client so chat middleware fires

Files: tests/conftest.py
Changes: Change SyntheticUsageChatClient to subclass agent_framework_openai.OpenAIChatCompletionClient instead of the minimal agent_framework.BaseChatClient, so it inherits the ChatMiddlewareLayer (verified empirically: the minimal base has no middleware layer, so a ChatMiddleware attached to an agent silently no-ops; with the layered client the middleware fired offline and raised BudgetExceeded). Construct via super().__init__(model="synthetic", api_key="synthetic", base_url="http://127.0.0.1:9/v1") (offline — verified no network call until a request is actually sent). Keep the existing behavior verbatim: scripted/default replies, call_count, the tokens_per_reply constructor kwarg name (unchanged), and a synthetic UsageDetails(total_token_count=tokens_per_reply) returned from the _inner_get_response(*, messages, stream, options, **kwargs) override (verified to be the slot the layered client calls for the raw response — it intercepts offline). Preserve the make_client_factory, fresh_store, seeded_store, docs_dir fixtures unchanged. No production code changes in this step.
Reuses: existing SyntheticUsageChatClient body (tests/conftest.py:26-73); OpenAIChatCompletionClient construction pattern from src/portfolio_optimiser/backends.py:89-94.
Test first:
- File: tests/conftest.py is test infra; the regression gate is the existing suite.
- Verifies: the re-based double is a drop-in — every existing test still passes (the double still emits synthetic UsageDetails, so strict-usage accounting does not hard-fail and token_usage > 0 still holds). conftest.py is shared by the whole suite, so the gate MUST be the full suite, not a subset.
- Pattern: tests/conftest.py:26-73 (current double).
Verify: uv run pytest -q → expected: 97 passed, 3 skipped (unchanged from pre-remediation — this proves the re-base is a true drop-in across every fixture consumer).
On failure: retry once adjusting the _inner_get_response override signature to match the layered client's call contract; if the layered base demands network on construction, escalate (it does not — verified offline). Then revert — git checkout -- tests/conftest.py.
Checkpoint: git commit -m "test(fase2): re-base synthetic client on layered client so chat middleware fires"

Manifest:

manifest:
  expected_paths:
    - tests/conftest.py
  min_file_count: 1
  commit_message_pattern: "^test\\(fase2\\): re-base synthetic client on layered client so chat middleware fires$"
  bash_syntax_check: []
  forbidden_paths:
    - src/portfolio_optimiser/run.py
    - src/portfolio_optimiser/budget.py
  must_contain:
    - path: tests/conftest.py
      pattern: "OpenAIChatCompletionClient"

Step 2: Thread tools + middleware through fresh_workflow onto each debate agent

Files: src/portfolio_optimiser/workflow.py, tests/test_workflow.py
Changes: Add keyword-only params tools: Sequence[Any] | None = None and middleware: Sequence[Any] | None = None to both maker_checker_agents(...) and fresh_workflow(...). In maker_checker_agents, pass them to each agent: Agent(client_factory(role), _INSTRUCTIONS[role], name=role, tools=tools, middleware=middleware). fresh_workflow forwards both to maker_checker_agents. Also set output_from=[agents[0]] (the proposer) on the GroupChatBuilder — this is a verified prerequisite for F1: empirically, get_outputs() by default surfaces ONLY the orchestrator's "reached max rounds" notice; output_from=[proposer] is what surfaces the proposer's converged text (Step 4 consumes it). Defaults None preserve current behavior. Update the module docstring line that claims the budget middleware is "wired by the orchestrator" to point at the new param. (Verified: Agent.__init__ accepts both tools=/middleware=; ChatMiddleware ∈ MiddlewareTypes; GroupChatBuilder(output_from=[proposer]) surfaces the proposer output.)
Reuses: maker_checker_agents / fresh_workflow (workflow.py:45-86); Agent signature + GroupChatBuilder(output_from=) (introspected).
Test first:
- File: tests/test_workflow.py (existing — add)
- Verifies: TWO assertions. (a) Construction-spy: monkeypatch portfolio_optimiser.workflow.Agent with a recorder, call maker_checker_agents(factory, tools=[sentinel_tool], middleware=[sentinel_mw]), assert every recorded construction received tools containing sentinel_tool and middleware containing sentinel_mw (.tools is not a public attr on a built Agent, so assert at the construction boundary). (b) Behavioral: run a real fresh_workflow(scripted_factory).run(...) where the proposer's FakeChatClient reply is a distinctive marker, and assert that marker appears in [getattr(o,"text","") for o in result.get_outputs()] — proving output_from=[proposer] actually surfaces the proposer's output (not just the orchestrator notice).
- Pattern: tests/test_workflow.py:14-40 (factory style + monkeypatch.setattr).
Verify: uv run pytest tests/test_workflow.py -q → expected: all passed
On failure: revert — git checkout -- src/portfolio_optimiser/workflow.py tests/test_workflow.py
Checkpoint: git commit -m "feat(fase2): thread tools + budget middleware through fresh_workflow onto agents"

Manifest:

manifest:
  expected_paths:
    - src/portfolio_optimiser/workflow.py
    - tests/test_workflow.py
  min_file_count: 2
  commit_message_pattern: "^feat\\(fase2\\): thread tools \\+ budget middleware through fresh_workflow onto agents$"
  bash_syntax_check: []
  forbidden_paths:
    - src/portfolio_optimiser/budget.py
  must_contain:
    - path: src/portfolio_optimiser/workflow.py
      pattern: "middleware=middleware"
    - path: src/portfolio_optimiser/workflow.py
      pattern: "tools=tools"
    - path: src/portfolio_optimiser/workflow.py
      pattern: "output_from"

Step 3: Construct and attach BudgetMiddleware + retrieval tool in the orchestrator

Files: src/portfolio_optimiser/run.py, tests/test_vertical_slice_e2e.py
Changes: In run_project, after building meter (run.py:121), construct mw = BudgetMiddleware(meter) and retrieval_tool = make_retrieval_tool(docs_dir, top_k=top_k), then pass both into the factory call: debate = fresh_workflow(factory, max_rounds=max_rounds, enable_layer1_hitl=enable_layer1_hitl, tools=[retrieval_tool], middleware=[mw]). Add the imports from portfolio_optimiser.budget import Budget, TokenMeter, BudgetMiddleware and from portfolio_optimiser.datasource import ..., make_retrieval_tool. This makes the debate's per-call token usage flow through the shared meter via the ChatMiddleware (the brief's named mechanism) and gives the agents the citation-bearing data-source tool. (F2 definition-site budget.py:81 + F5 wiring-site run.py:121 + F7 tool exposure all resolved here.)
Reuses: BudgetMiddleware (budget.py:81), make_retrieval_tool (datasource.py:49), fresh_workflow new params (Step 2).
Test first:
- File: tests/test_vertical_slice_e2e.py (existing — add)
- Verifies: a spy test that monkeypatches portfolio_optimiser.run.fresh_workflow to capture its kwargs, runs run_project with the synthetic factory, and asserts the captured middleware is a list containing a BudgetMiddleware and tools is a list containing a FunctionTool (proves the orchestrator wires both, independent of MAF internals).
- Pattern: tests/test_vertical_slice_e2e.py:85-100 (spy factory style).
Verify: uv run pytest tests/test_vertical_slice_e2e.py -q → expected: all passed
On failure: revert — git checkout -- src/portfolio_optimiser/run.py tests/test_vertical_slice_e2e.py
Checkpoint: git commit -m "feat(fase2): wire BudgetMiddleware + retrieval tool onto the debate in run_project"

Manifest:

manifest:
  expected_paths:
    - src/portfolio_optimiser/run.py
    - tests/test_vertical_slice_e2e.py
  min_file_count: 2
  commit_message_pattern: "^feat\\(fase2\\): wire BudgetMiddleware \\+ retrieval tool onto the debate in run_project$"
  bash_syntax_check: []
  forbidden_paths:
    - src/portfolio_optimiser/budget.py
    - src/portfolio_optimiser/validator.py
  must_contain:
    - path: src/portfolio_optimiser/run.py
      pattern: "BudgetMiddleware\\(meter\\)"
    - path: src/portfolio_optimiser/run.py
      pattern: "make_retrieval_tool\\("

Step 4: Feed the debate's converged output into candidate generation

Files: src/portfolio_optimiser/run.py, tests/test_vertical_slice_e2e.py
Changes: Capture the debate result: result = await debate.run(...) (currently a bare statement at run.py:124). Add a helper _debate_text(result) -> str with the verified extraction: iterate result.get_outputs(), read each element's .text, exclude the orchestrator's termination notice (an element whose author_name == "group_chat_orchestrator", or whose text contains "reached the maximum number of rounds"), and return the last remaining non-empty text (the proposer's converged output surfaced by output_from=[proposer] from Step 2); return "" if none. Build the generation context from BOTH the debate output and the citations: gen_context = _debate_text(result) or context — passed to generate_via_llm(factory("proposer"), project, gen_context, meter), so the validated proposal provably derives from the debate (retrieval context only as a last-resort fallback; note: the fallback path is a test failure per test_g, not a silent degradation — the test asserts the debate marker reaches generation). Add debate_output: str to RunResult and populate it (the minimal observable surface F1's mandated assertion needs). F9 (MINOR, the fake-model provenance label) is OUT OF SCOPE for this remediation — it is advisory only (Handover 6 excludes MINOR); deferred as an optional follow-on, NOT addressed here, to keep this step focused on the BLOCKER.
Reuses: generate_via_llm (generate.py:92), WorkflowRunResult.get_outputs() returning AgentResponse with .text (verified), RunResult dataclass (run.py:52).
Test first:
- File: tests/test_vertical_slice_e2e.py (existing — add)
- Verifies: test_g_validated_proposal_derives_from_debate — a spy captures the context argument passed to generate_via_llm (monkeypatch portfolio_optimiser.run.generate_via_llm), scripts the debate agents (via the proposer's FakeChatClient/synthetic reply) to emit a distinctive marker string, runs run_project, and asserts (a) result.debate_output is non-empty and contains the scripted marker, and (b) the captured generation context contains that marker — i.e. deleting the debate would change the generation input (the precise gap the review named). The marker is chosen so it could only come from the debate, never from the retrieval docs_dir fixture.
- Pattern: tests/test_vertical_slice_e2e.py:85-100 (spy) + :28-37 (assertion style).
Verify: uv run pytest tests/test_vertical_slice_e2e.py -q → expected: all passed
On failure: the extraction shape is verified (AgentResponse.text, orchestrator notice filtered by author); if test_g fails, the orchestrator-notice filter or the output_from wiring (Step 2) is the cause — fix the filter predicate, do not loosen the assertion. Else revert — git checkout -- src/portfolio_optimiser/run.py tests/test_vertical_slice_e2e.py
Checkpoint: git commit -m "feat(fase2): feed debate converged output into candidate generation"

Manifest:

manifest:
  expected_paths:
    - src/portfolio_optimiser/run.py
    - tests/test_vertical_slice_e2e.py
  min_file_count: 2
  commit_message_pattern: "^feat\\(fase2\\): feed debate converged output into candidate generation$"
  bash_syntax_check: []
  forbidden_paths:
    - src/portfolio_optimiser/generate.py
    - src/portfolio_optimiser/validator.py
  must_contain:
    - path: src/portfolio_optimiser/run.py
      pattern: "get_outputs\\("
    - path: src/portfolio_optimiser/run.py
      pattern: "debate_output"

Step 5: Prove the middleware short-circuits a real chat call and the debate path

Files: tests/test_budget.py, tests/test_vertical_slice_e2e.py
Changes: Add the integration test the review demands (F8). In tests/test_budget.py, add test_budget_middleware_fires_on_real_agent_chat: build an Agent(SyntheticUsageChatClient(tokens_per_reply=8), "x", name="proposer", middleware=[BudgetMiddleware(TokenMeter(Budget(max_tokens=5, max_rounds=10)))]), await agent.run("hi"), assert BudgetExceeded is raised from the middleware path with kind == "tokens" (verified empirically: meter charged to 8 > cap 5). In tests/test_vertical_slice_e2e.py, add test_h_tiny_budget_halts_via_debate_middleware — and make it genuinely load-bearing (the critic's catch): monkeypatch portfolio_optimiser.run.generate_via_llm to raise AssertionError if it is ever called, then run run_project with a tiny max_tokens and per-reply tokens above it, asserting BudgetExceeded is raised. Because generate_via_llm independently raises BudgetExceeded (generate.py:111), a plain pytest.raises would pass even if the debate middleware no-ops (the exact "green-but-dead" trap) — failing the test the moment generation is reached forces the BudgetExceeded to originate in the debate ChatMiddleware (verified it propagates out of debate.run()), so detaching the debate middleware fails this test. Keep the existing test_e as-is (still green; its mechanism shifts from the generate loop to the debate middleware — note this in its docstring).
Reuses: BudgetMiddleware (budget.py:81), the re-based SyntheticUsageChatClient (Step 1), run_project wiring (Steps 2-4).
Test first:
- File: tests/test_budget.py (existing — add) and tests/test_vertical_slice_e2e.py (existing — add)
- Verifies: detaching BudgetMiddleware from the agent now fails a test (the middleware↔real-client integration is no longer untested — the exact gap from F8).
- Pattern: tests/test_budget.py:41-56 (middleware assertion) + tests/test_vertical_slice_e2e.py:74-82 (tiny-budget e2e).
Verify: uv run pytest tests/test_budget.py tests/test_vertical_slice_e2e.py -q → expected: all passed
On failure: revert — git checkout -- tests/test_budget.py tests/test_vertical_slice_e2e.py
Checkpoint: git commit -m "test(fase2): assert BudgetMiddleware short-circuits a real chat call + the debate"

Manifest:

manifest:
  expected_paths:
    - tests/test_budget.py
    - tests/test_vertical_slice_e2e.py
  min_file_count: 2
  commit_message_pattern: "^test\\(fase2\\): assert BudgetMiddleware short-circuits a real chat call \\+ the debate$"
  bash_syntax_check: []
  forbidden_paths:
    - src/portfolio_optimiser/budget.py
  must_contain:
    - path: tests/test_budget.py
      pattern: "agent.run"

Step 6: Commit the env contract artifact under a pathguard-permitted name

Files: env.template (new)
Changes: Create env.template at the repo root with the staged content documenting PORTFOLIO_LOCAL_BASE_URL / PORTFOLIO_LOCAL_API_KEY, the Foundry PORTFOLIO_FOUNDRY_PROJECT_ENDPOINT, and the research-03 no-egress notes (loopback only, Ollama bind 127.0.0.1, pin ≥ 0.17.1, OLLAMA_DEBUG unset, model-pull = explicit egress). The leading-dot name .env.template is blocked by pre-write-pathguard.sh:90 (\.env(\.[a-zA-Z]+)?$); env.template is permitted (verified). The staged file is the source of truth for the no-egress notes (…/eed58028-…/scratchpad/env.template.staged, content confirmed during planning) — the env-var names are in backends.py but the no-egress notes are NOT, so if the staged file is GC'd, reconstruct the notes from research/03-local-profile-chat-client.md §Dim 6, do not just emit bare var names. Grep README*/docs/ for any \.env\.template reference and reconcile it to env.template.
Reuses: staged template content (source of truth); env-var names from src/portfolio_optimiser/backends.py:74,90-91; no-egress notes from research/03-local-profile-chat-client.md §Dim 6.
Test first:
- File: none (it is a config artifact, not code) — the manifest is the predicate.
- Verifies: env.template exists, documents BOTH profiles' env vars, AND carries the no-egress notes (so the commit message's "+ no-egress notes" is actually true).
Verify: test -f env.template && grep -q PORTFOLIO_LOCAL_BASE_URL env.template && grep -q PORTFOLIO_FOUNDRY_PROJECT_ENDPOINT env.template && grep -q 127.0.0.1 env.template && grep -qi egress env.template && echo OK → expected: OK
On failure: if the pathguard hook blocks the write, the name is wrong — confirm no leading dot; if the no-egress grep fails, the notes are missing — reconstruct from research/03. Then escalate.
Checkpoint: git commit -m "docs(fase2): add env.template documenting both profiles + no-egress notes"

Manifest:

manifest:
  expected_paths:
    - env.template
  min_file_count: 1
  commit_message_pattern: "^docs\\(fase2\\): add env.template documenting both profiles \\+ no-egress notes$"
  bash_syntax_check: []
  forbidden_paths: []
  must_contain:
    - path: env.template
      pattern: "PORTFOLIO_LOCAL_BASE_URL"
    - path: env.template
      pattern: "127.0.0.1"

Step 7: Record the PEP 735 dependency-groups migration as an intentional deviation

Files: pyproject.toml
Changes: The execute-time migration of the dev group from [project.optional-dependencies] to PEP 735 [dependency-groups] was beyond the original plan Step 1 scope but is the correct structure — it is what makes the brief's SC1 bare uv sync + uv run pytest install dev tooling without --extra (reverting would break SC1). Keep the structure and resolve the drift by flagging it: extend the existing comment block immediately above the [dependency-groups] table (confirm the anchor at execute time; the review cites the table at pyproject.toml:25) to state explicitly that this is an intentional, recorded deviation from the original plan Step 1 (which authorized only moving orchestrations + pulp to core), justified by SC1, and cross-reference this remediation. (No dependency change; documentation only.)
Reuses: existing comment block pyproject.toml:23-24.
Test first:
- File: none (config comment) — the manifest is the predicate.
- Verifies: the deviation is now explicitly recorded in-tree (no longer silent drift).
Verify: grep -qi "intentional" pyproject.toml && grep -qi "Step 1" pyproject.toml && echo OK → expected: OK
On failure: revert — git checkout -- pyproject.toml
Checkpoint: git commit -m "docs(fase2): record PEP 735 dependency-groups as intentional Step 1 deviation"

Manifest:

manifest:
  expected_paths:
    - pyproject.toml
  min_file_count: 1
  commit_message_pattern: "^docs\\(fase2\\): record PEP 735 dependency-groups as intentional Step 1 deviation$"
  bash_syntax_check: []
  forbidden_paths:
    - src/portfolio_optimiser/backends.py
  must_contain:
    - path: pyproject.toml
      pattern: "[Ii]ntentional"

Failure recovery rules

revert — git checkout -- {files}, do not proceed.
retry — one alternative attempt, then revert.
escalate — stop; the issue needs human judgment.
Checkpoint — commit after each green step so later failures cannot corrupt completed work.

Alternatives Considered

Approach	Pros	Cons	Why rejected
F1: Parse the proposer's final debate message directly into the IR and validate that (skip `generate_via_llm`)	Most literal "candidate from debate"	Small local models leak text / emit non-JSON (research 03 Dim 3); needs the parse-retry loop anyway; fragile	Rejected — `generate_via_llm` IS the validator-as-retry reliability mechanism the research mandates; feed the debate output into it instead
F2: attach `BudgetMiddleware` to the chat client (not the agent)	Conceptually "per-client"	`BaseChatClient.__init__` takes no `middleware`; the layered client applies it via `client_kwargs`/agent — the GA-canonical seam is `Agent(middleware=…)`	Rejected — `Agent(middleware=…)` is the documented + verified seam
F2: keep the minimal `BaseChatClient` test double, assert the meter via the generate loop only	No conftest change	The middleware would never fire in tests — reproduces the review's "green-but-dead" defect	Rejected — the whole point is to test the real mechanism
F4: revert dev deps to `[project.optional-dependencies]`	Matches the literal Step 1 plan	Breaks SC1 (`uv sync` would no longer install dev tooling without `--extra`)	Rejected — keep PEP 735, record the deviation instead
Write to `{project_dir}/plan.md` (skill default)	Default path	Clobbers the original Fase 2 plan (audit trail)	Rejected — write `plan-remediation.md`

Test Strategy

Framework: pytest, asyncio_mode=auto; synthetic-usage clients (no LLM, deterministic).
Existing patterns: spy/recording factories; assertions on RunResult; synthetic UsageDetails.
New/changed tests: re-based double (Step 1); construction-spy for tool+middleware threading (Step 2); orchestrator-wiring spy (Step 3); debate-derivation test (Step 4); real-client middleware + debate-halt tests (Step 5). Net: the three seams each gain a test that fails if the seam is detached.

Tests to write

Type	File	Verifies	Model test
Unit	`tests/test_workflow.py`	tool+middleware threaded to each `Agent`	`tests/test_workflow.py:14-40`
Integration	`tests/test_vertical_slice_e2e.py` `test_g_…`	validated proposal derives from the debate	`tests/test_vertical_slice_e2e.py:85-100`
Integration	`tests/test_budget.py` `test_…fires_on_real_agent_chat`	middleware short-circuits a real `agent.run()`	`tests/test_budget.py:41-56`
Integration	`tests/test_vertical_slice_e2e.py` `test_h_…`	tiny budget halts via the debate middleware	`tests/test_vertical_slice_e2e.py:74-82`

Risks and Mitigations

Priority	Risk	Location	Impact	Mitigation
Low (was Medium — now verified)	`get_outputs()` surfaces only the orchestrator notice unless `output_from=[proposer]` is set	`workflow.py` Step 2 / `run.py` Step 4	`_debate_text` extracts boilerplate → F1 hollow	RESOLVED in planning: Step 2 sets `output_from=[proposer]` (empirically surfaces proposer text); Step 4 filters the orchestrator notice by author; `test_g` fails loudly if the marker doesn't reach generation
Low (was Medium — now verified)	Re-basing the test double on the layered client changes call semantics	`tests/conftest.py` Step 1	existing tests regress	Verified offline: layered client constructs with no network, `_inner_get_response` override intercepts, synthetic `UsageDetails` reaches the middleware (raised `BudgetExceeded`, not `UsageUnavailable`); Step 1 gate is the FULL suite
Low	Middleware on debate agents shifts where `test_e` raises (generate loop → debate)	`test_vertical_slice_e2e.py`	semantic surprise	Keep `test_e` green; document the shift in its docstring; `test_h` (generate-fenced) makes the debate-path halt explicit
Low	`make_retrieval_tool` attached but the local model is weak at tool-calling	`run.py` Step 3	tool not invoked on a weak model	Out of scope for wiring; research 03 covers it; validator-as-retry absorbs weak tool use; exposure (not invocation reliability) is the F7 fix

Assumptions

#	Assumption	Why unverifiable now	Impact if wrong
1	Re-based double is a drop-in for ALL existing tests (the 3 sampled categories passed offline during planning; the full 97/3 not re-run)	Full suite not re-run during planning	Step 1's verify gate is the full `uv run pytest` → caught immediately, before any mechanism wiring
2	The staged `env.template` content is still at the prior-session scratchpad	Cross-session scratchpad may be GC'd	Reconstruct no-egress notes from `research/03` §Dim 6 (Step 6 fallback) — NOT from `backends.py` (which lacks them)

Note: the three planning-time unknowns that mattered most — the get_outputs() output shape (needs output_from=[proposer]), the _inner_get_response override + strict_usage propagation, and BudgetExceeded propagation out of debate.run() — were all empirically resolved during planning and are no longer assumptions.

Verification

End-to-end checks that cross step boundaries (per-step manifests are auto-checked by trekexecute):

uv sync (fresh resolve) → expected: exit 0 and the dev tooling (pytest/ruff/mypy) installed WITHOUT --extra — this is the SC1 premise that motivates keeping PEP 735 (Step 7).
uv run pytest → expected: exit 0, strictly MORE tests than the pre-remediation 97 passed / 3 skipped (the new seam tests), 0 failed.
uv run ruff check . && uv run ruff format --check . → expected: clean.
uv run mypy src → expected: clean.
grep -n "get_outputs" src/portfolio_optimiser/run.py → expected: a match (debate output captured, not discarded).
grep -n "BudgetMiddleware(meter)" src/portfolio_optimiser/run.py → expected: a match (middleware constructed + wired).
grep -n "make_retrieval_tool(" src/portfolio_optimiser/run.py → expected: a match (tool exposed to agents).
test -f env.template → expected: present.
Detach assertion (the anti-theater check): temporarily removing middleware=[mw] from the Step 3 wiring MUST make test_h fail (it is generate-fenced, so the BudgetExceeded can only come from the debate); and temporarily reverting Step 4's debate capture MUST make test_g fail. If either still passes detached, the seam is still theater — do not close Fase 2.

Estimated Scope

Files to modify: 6 (run.py, workflow.py, pyproject.toml, conftest.py, test_workflow.py, test_budget.py, test_vertical_slice_e2e.py — 7 distinct files across steps; production code is 2: run.py, workflow.py).
Files to create: 1 (env.template).
Complexity: medium (the mechanisms are verified; the work is precise wiring + test-double rebasing, not new design).

Execution Strategy

7 steps; sequential within a single session (the code seams share run.py/workflow.py). Two independent artifact steps can run in parallel if desired.

Session 1: Wire and prove the three seams

Steps: 1, 2, 3, 4, 5
Wave: 1
Depends on: none
Scope fence:
- Touch: tests/conftest.py, src/portfolio_optimiser/workflow.py, src/portfolio_optimiser/run.py, tests/test_workflow.py, tests/test_vertical_slice_e2e.py, tests/test_budget.py
- Never touch: validator.py, budget.py (definitions are correct), generate.py, ir.py

Session 2: Artifact + record reconciliation

Steps: 6, 7
Wave: 1 (independent of Session 1 — can run in parallel)
Depends on: none
Scope fence:
- Touch: env.template, pyproject.toml
- Never touch: src/, tests/

Execution Order

Wave 1: Session 1 + Session 2 (independent; run Session 1 sequentially step-by-step, Session 2 anytime)

Grouping rules applied

Steps sharing run.py (3, 4) ordered and same session.
Artifact steps (6, 7) touch no code → separate parallelizable session.
Step 1 (test double) precedes the mechanism steps so the mechanism is provable.

Plan Quality Score

Dimension	Weight	Score	Notes
Structural integrity	0.15	94	Dependency-ordered; test double before mechanism; Step 1 gates full suite
Step quality	0.20	92	Each step 1–2 files, TDD; `test_h` generate-fenced; F9 removed from prose
Coverage completeness	0.20	95	All 2 BLOCKER + 5 MAJOR mapped; F8 debate-path now genuinely closed
Specification quality	0.15	93	All three load-bearing mechanics empirically verified vs installed source
Risk & pre-mortem	0.15	92	Test-double no-op trap + `get_outputs()` boilerplate trap both caught up front
Headless readiness	0.10	91	On-failure + Checkpoint per step; deferred-discovery language removed
Manifest quality	0.05	90	All steps have checkable manifests + must_contain (incl. `output_from`, no-egress)
Weighted total	1.00	93	Grade: A

Adversarial review:

Plan critic: REVISE — 0 blocker, 5 major, 5 minor (score 74/C on the pre-revision draft). All 10 findings addressed below (3 of them were already empirically verified during planning and were under-documented, not wrong).
Scope guardian: ALIGNED — 0 scope creep, 0 finding-gaps; all 7 source_findings map to concrete steps; F9 correctly optional. 2 minor verification gaps addressed below.

Revisions

#	Finding (source)	Severity	Resolution
1	`test_h` is theater — passes even if debate middleware no-ops, because `generate_via_llm` raises `BudgetExceeded` independently (plan-critic)	major	Step 5: `test_h` now monkeypatches `generate_via_llm` to fail if reached → `BudgetExceeded` must originate in the debate; detaching the debate middleware fails it
2	F1 rests on an unverified `get_outputs()` shape; `or context` fallback could re-introduce F1 (plan-critic)	major	Verified empirically: default `get_outputs()` surfaces only the orchestrator notice; Step 2 sets `output_from=[proposer]`; Step 4 filters the notice; `test_g` fails (not degrades) if the debate marker doesn't reach generation
3	`_inner_get_response` override slot unverified — could hit the network (plan-critic)	major	Verified empirically offline: the override intercepts, no network call, middleware fires; recorded in Research Sources
4	Step 1 verify gate too narrow (conftest is shared) (plan-critic)	major	Step 1 now gates on full `uv run pytest` (97/3)
5	`_debate_text` shape deferred to execute-time discovery (plan-critic)	major	Extraction now concrete (verified `AgentResponse.text` + orchestrator-notice filter); deferred-discovery language removed
6	F9 optional embedded inside Step 4 prose (plan-critic)	minor	F9 removed from Step 4; explicitly out-of-scope optional follow-on
7	env.template no-egress notes unverified; fallback can't reproduce them (plan-critic)	minor	Step 6 verify greps `127.0.0.1` + `egress`; manifest asserts `127.0.0.1`; fallback sources notes from research/03
8	`strict_usage` propagation on the debate path unverified (plan-critic)	minor	Verified empirically (middleware raised `BudgetExceeded`, not `UsageUnavailable`); recorded in Research Sources
9	`debate_output` adds a field to frozen `RunResult` — confirm scope (plan-critic)	minor	Kept — the minimal observable surface F1's mandated assertion requires (scope-guardian concurred it is justified, not creep)
10	Step 7 line-ref drift (`23-24` vs `25`) (plan-critic)	minor	Anchor de-brittled — references the comment block above `[dependency-groups]`, confirm at execute
11	SC1 bare `uv sync` (Step 7's motivating premise) not verified (scope-guardian)	minor	Added `uv sync` fresh-resolve check to Verification
12	Step 5 example used `tokens=8`; client kwarg is `tokens_per_reply` (scope-guardian)	minor	Corrected to `tokens_per_reply=8`

39 KiB Raw Blame History Unescape Escape

Fase 2 MVP Vertical Slice — Remediation (connect the three load-bearing seams)

Context

Architecture Diagram

Codebase Analysis

Research Sources

Implementation Plan

Step 1: Re-base the synthetic test client on the layered client so chat middleware fires

Step 2: Thread tools + middleware through fresh_workflow onto each debate agent

Step 3: Construct and attach BudgetMiddleware + retrieval tool in the orchestrator

Step 4: Feed the debate's converged output into candidate generation

Step 5: Prove the middleware short-circuits a real chat call and the debate path

Step 6: Commit the env contract artifact under a pathguard-permitted name

Step 7: Record the PEP 735 dependency-groups migration as an intentional deviation

Failure recovery rules

Alternatives Considered

Test Strategy

Tests to write

Risks and Mitigations

Assumptions

Verification

Estimated Scope

Execution Strategy

Session 1: Wire and prove the three seams

Session 2: Artifact + record reconciliation

Execution Order

Grouping rules applied

Plan Quality Score

Revisions

39 KiB

Raw Blame History