open/portfolio-optimiser
1
0
Fork 0
Code Issues Pull requests Releases Wiki Activity Actions

docs(fase2): remediation plan (Handover 6) — consumes 2 BLOCKER + 5 MAJOR

Browse source
This commit is contained in:
Kjell Tore Guttormsen 2026-06-26 00:46:23 +02:00
commit 01c3f0dc8a
1 changed files with 458 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

458
.claude/projects/2026-06-24-fase2-mvp-vertical-slice/plan-remediation.md Normal file
View file

@ -0,0 +1,458 @@
<!--
Remediation plan generated from a type:trekreview input (Handover 6). Lists the
40-char hex IDs of the BLOCKER + MAJOR findings consumed from review.md.
---
source_findings:
- 4d8f1a2c9b6e3057f4a1d8c2b9e0537a6c1f4e92
- 7b3e9c1d8a4f2056e9b0c3d7f1a85249c6e3b801
- 9f2a4d7c1e8b3056a4f9d2c7b1e08345f6c29d1b
- 2c7e1a9f4d8b3650c1e7a4f2d9b08537e6c14a90
- 5a9d2f7c4e1b8360a9f4d7c2e1b95048f3c6a72d
- 8e1c4a7f2d9b5063c8a1e4f7d2b9043507c6e1a8
- 3f7a1c9e4d2b8560f7a3c1e9d4b25081c6e7a34f
---
-->
# 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
```mermaid
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:**
```yaml
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:**
```yaml
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:**
```yaml
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:**
```yaml
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:**
```yaml
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:**
```yaml
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:**
```yaml
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` |