open/portfolio-optimiser
1
0
Fork 0
Code Issues Releases 3 Activity

feat(explore): --plan-review gjoer "be om svar, bruke svarene" naabar fra CLI (F4, ORDRE 20260825T133139Z)

Browse source 
F4 fra misjonsreviewen: begge operatorflatene nektet enable_plan_review, og eneste doer var
explore(..., plan_reviewer=...) i bibliotek-APIet. Maalbildets HITL-loop var dermed unaabar for
enhver som ikke importerte pakka. Reviewens tre fil:linje-paastander ble verifisert mot kilden foer
bygging og stemte.

Flate: CLI. `--plan-review` bygger en terminal_plan_reviewer() og gir den til den UENDREDE sloeyfa.
Operatoren vises planen og svarer "approve" eller "revise <hva>"; en revisjon gaar tilbake til
manageren, som replanlegger og spoer IGJEN om den NYE planen.

Gaten er den ANDRE halvdelen av setningen. En doer som printer planen, leser linja og kaster den
bestaar "operatoren ble spurt" og feiler maalbildet -- repoets vakuoes-gate-klasse. T1 er derfor
test_explore_loadbearing sin T15 loeftet til CLI-niva og er ROED mot en alltid-godkjenn-reviewer.
Vitnet er {run_id}-exploration.json (skrevet fra en finally), ikke skrapet stdout.

Fail-closed paa operatorens egen input: alt utenfor vokabularet spoerres paa nytt, og EOF raiser
PlanReviewInputError -- stillhet er aldri en signatur.

Fire nekter ved navn, hvorav to lukket et stille dropp ingen test dekket: report_forbidden (report-
modus returnerer FOER hver utforsknings-nekt) og portefoelje-partisjonen. De to konfig-avhengige
nektene deler tokenet enable_plan_review og har derfor ulik saertekst; den eksisterende testen
asserterte paa det delte tokenet og er rettet (oekt-57-mutasjonen, niende gang).

Hosting nekter fortsatt -- reviewen er synkron og ville blokkert bade HTTP-requesten og event-loekka
som svarer /readiness -- men meldingen navngir na CLI-doeren i stedet for aa paasta at biblioteket er
den eneste.

Mid-loep-spoersmaal er IKKE bygget, og fravaeret er MAALT: _magentic.py har noeyaktig ETT
ctx.request_info (:1044, plan review) i hele modulen. Reviewens "kun plan-review FOER loepet" er
derimot upresist -- samme forespoersel fyrer ogsaa ved re-plan etter en stall.

Load-bearing MAALT (tests/test_plan_review_cli_door_loadbearing.py, 12 tester), elleve mutasjoner
alle roede mot HELE suiten + groenn kontroll 1040/5 og golden demo-transcript.stdout BYTE-UENDRET
(ea8c534773acdbe41ae68f2c55724d69aaf8be4f).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LtMDsh2zfp4Bmw8KGJ4aLD
This commit is contained in:
Kjell Tore Guttormsen 2026-08-26 00:27:36 +02:00
commit 84e8de8679
7 changed files with 612 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

36
CLAUDE.md
View file

@ -955,6 +955,42 @@ Python ≥3.10. MAF (`agent-framework-core` 1.9.0). Pakkehåndtering: `uv`. To b
åttende gang):** store-testen sammenlignet med `==`, og `VerdictStore` er en pydantic-modell med
VERDI-likhet — tre ulike TOMME stores er alle like, så «fersk store per base» lot HELE suiten stå
grønn. Delt instans er påstanden, så testen asserterer nå på `is`.
- **«Be om svar, BRUKE svarene» er nåbar fra CLI-en, og gaten er den ANDRE halvdelen (F4, økt 63):**
før dette nektet BEGGE operatørflatene `enable_plan_review` (`run.py`, `hosting.py`) og eneste dør
var `explore(..., plan_reviewer=...)` — MÅLT mot kilden, ikke lest ut av reviewens prosa.
`--plan-review` bygger en `terminal_plan_reviewer()` og gir den til den UENDREDE sløyfa: operatøren
vises planen og svarer `approve` eller `revise <hva>`; en revisjon går tilbake til manageren, som
replanlegger og spør IGJEN om den NYE planen. **Diskriminatoren er dét siste** — en dør som printer
planen, leser linja og kaster den består «operatøren ble spurt» og feiler målbildet (repoets
vakuøs-gate-klasse); T1 er derfor bygget som `test_explore_loadbearing`s T15 løftet til CLI-nivå og
er RØD mot en alltid-godkjenn-reviewer. **Vitnet er `{run_id}-exploration.json`, ikke skrapet
stdout:** `trace_payload` bærer alt tre (rekkefølge, beslutning, feedback verbatim) og skrives fra
en `finally`, så den ene kjøringen som mest trenger beviset — den et tak eller en ubesvart review
kappet — etterlater det. **Fail-closed på operatørens EGEN input:** alt utenfor det lukkede
vokabularet spørres på nytt (aldri lest som en beslutning), og **EOF raiser `PlanReviewInputError`**
— å lese stillhet som ja ville latt en autonom sløyfe kjøre på en plan ingen signerte, usynlig.
Strømmene resolveres ved KALL-tid (`shared_root()`-idiomet), ellers svarer reviewer-en fra strømmen
som fantes da den ble BYGGET. **Fire nekter, alle ved navn**, hvorav to lukker et stille dropp
ingen test dekket: `report_forbidden` (report-modus returnerer FØR hver utforsknings-nekt) og
portefølje-partisjonen. De to konfig-avhengige nektene DELER tokenet `enable_plan_review` og har
derfor bevisst ULIK særtekst («no reviewer was offered» / «no review is ever requested») — den
eksisterende testen asserterte på det delte tokenet og er rettet (økt-57-mutasjonen, niende gang).
**Hosting NEKTER fortsatt, og det er en beslutning:** reviewen er synkron, så den ville blokkert
HTTP-requesten på et menneske OG event-løkka som svarer `/readiness` — meldingen navngir nå
CLI-døra i stedet for å påstå at biblioteket er den eneste (Fase 3-klassen). **Mid-løp-spørsmål er
IKKE bygget, og fraværet er MÅLT:** `_magentic.py` har nøyaktig ETT `ctx.request_info` (`:1044`,
plan review) i hele modulen, så stacken kan ikke levere et spørsmål midt i løpet uten en ny
emitter. Reviewens «kun plan-review FØR løpet» er derimot upresist: samme forespørsel fyrer også
ved re-plan etter en stall (`is_stalled=True`), så døra ER nåbar midt i en kjøring på den ene
måten stacken støtter. Load-bearing MÅLT (`tests/test_plan_review_cli_door_loadbearing.py`, 12
tester), elleve mutasjoner alle røde mot HELE suiten + grønn kontroll 1040/5 og golden
`demo-transcript.stdout` BYTE-UENDRET (`ea8c534773acdbe41ae68f2c55724d69aaf8be4f`): detach
`plan_reviewer`-wiringen (5 røde) · EOF blir en godkjenning (1) · alltid-godkjenn (3) · alt som
ikke er en revisjon blir en signatur (1) · dropp `--plan-review` fra portefølje-partisjonen (1) ·
dropp den fra `report_forbidden` (1) · detach `--plan-review requires --explore` (1) · detach
review-uten-reviewer-nekten (2, hvorav én i en test som fantes fra før) · detach
reviewer-ingen-spør-nekten (1) · hostet nekt beholder påstanden fra før F4 (1) · strømmene fanget
ved bygge-tid (1).
- **STATE.md er local-only** (gitignored). Voyage session-state er efemert; STATE.md er kanonisk kontinuitet.
- Prosess: Voyage-plugin (`/trekbrief → /trekplan → /trekexecute → /trekreview`) per større fase.

24
README.md
View file

@ -438,8 +438,28 @@ when the seam is detached, so the loop cannot silently degrade into theater.
`--explore-config` states the bounds, and **every field is required** — `max_rounds`,
`max_tokens`, `max_stall_count`, `max_reset_count`, `max_plan_revisions`, `enable_plan_review`.
None of them has a default, because an omitted cap falls back to an *unbounded* loop rather than
a conservative one. `enable_plan_review` must be `false` on this surface: the plan review is
synchronous and there is no reviewer at a CLI to answer it (the library API takes one).
a conservative one.
**Answering the plan review (`--plan-review`).** With `enable_plan_review` set, the exploration
stops before the loop is allowed to run and asks you to sign the plan off. `--plan-review`
answers it *at your terminal*: you are shown the plan, and you type `approve` or
`revise <what to change>`. A revision goes back to the manager, which replans and asks you
again about the **new** plan; `max_plan_revisions` bounds how many revisions are applied. Every
round trip is recorded in `{run_id}-exploration.json` with your words verbatim. Input that ends
without an answer is an error, never a sign-off — an autonomous loop must not run on a plan
nobody approved. The two flags are refused apart: `enable_plan_review` without `--plan-review`
would stop at a review nobody can answer, and `--plan-review` without it would build a reviewer
nobody ever asks.
```bash
uv run python -m portfolio_optimiser.run FV42-GSV-E1 --docs-dir <docs> --bundle-dir <bundle> \
--explore "Find the cheapest saving worth testing here" --explore-config exploration.json \
--plan-review --outbox-dir out --run-id r1
```
The review is **synchronous**: the loop waits on you. That is why the hosted surface refuses it
— blocking an HTTP request on a human would also block the event loop that answers
`/readiness`.
`--explore` is refused together with `--mandate` — they are two sources of one mandate, and
merging would silently overwrite what you wrote. To seed an exploration with a domain expert's

77
src/portfolio_optimiser/explore.py
View file

@ -24,10 +24,11 @@ This module imports ``agent_framework.orchestrations`` and therefore may never b
from __future__ import annotations
import json
import sys
from collections.abc import Callable, Mapping, Sequence
from dataclasses import dataclass, field
from pathlib import Path
from typing import Any, Final, Literal
from typing import Any, Final, Literal, TextIO
from agent_framework import Agent, BaseChatClient, FunctionTool, tool
from agent_framework.orchestrations import (
@ -363,6 +364,80 @@ class PlanReviewDecision:
PlanReviewer = Callable[[PlanReviewRequest], PlanReviewDecision]
class PlanReviewInputError(ExplorationError):
"""A terminal plan review was left without an answer: the input ended mid-review.
A distinct type rather than a distinguishing message, for the reason ``_classify_stop`` reads
counts instead of the termination prose: a caller deciding what happened should never have to
match wording. It is an ``ExplorationError`` (a ``RuntimeError``) because the run FAILED — the
caller's argv was fine and the loop had already started spending; the same channel an
unreadable marked hypothesis leaves by.
"""
#: The closed answer vocabulary of the terminal door. Two words, matched structurally.
_APPROVE_ANSWER: Final = "approve"
_REVISE_ANSWER: Final = "revise"
def terminal_plan_reviewer(
*, stream_in: TextIO | None = None, stream_out: TextIO | None = None
) -> PlanReviewer:
"""A ``PlanReviewer`` that asks the operator at a terminal and reads their typed answer.
This is the door that makes målbilde §3's "still spørsmål, be om svar, bruke svarene" reachable
without importing the package (F4): ``run.py``'s ``--plan-review`` builds one of these and
hands it to ``explore()``. Blocking is not an oversight — ``explore()`` calls the reviewer
synchronously (it is not awaited), so the loop waits on the human exactly as the ``PlanReviewer``
contract says. That is also the reason the hosted surface keeps refusing the review: there,
blocking the reviewer would block the event loop that answers ``/readiness``.
**The streams are resolved at CALL time, not here** (the ``shared_root()`` idiom): a factory
that captured ``sys.stdin`` at construction could not be driven by a caller — or a test — that
replaces the stream afterwards, and the only way left to exercise the door would be a
subprocess.
**Fail-closed on the operator's own input.** ``approve`` signs off; ``revise <what to change>``
sends the words back to the manager. Anything else — a blank line, a typo, a bare ``revise`` —
is asked AGAIN, never taken as a decision. End of input raises ``PlanReviewInputError``:
reading silence as approval would let an autonomous loop run on a plan no human signed, and do
it invisibly. Validation, NEVER repair (the ``write_concept_file`` rule).
"""
def review(request: PlanReviewRequest) -> PlanReviewDecision:
source = sys.stdin if stream_in is None else stream_in
sink = sys.stdout if stream_out is None else stream_out
stalled = " (a RE-PLAN after a stall)" if request.is_stalled else ""
print(f"\nPLAN REVIEW #{request.index + 1}{stalled}", file=sink)
print("--- the plan the exploration would run ---", file=sink)
print(request.plan, file=sink)
if request.current_progress.strip():
print("--- progress so far ---", file=sink)
print(request.current_progress, file=sink)
while True:
print(
f'Answer "{_APPROVE_ANSWER}" to sign it off, '
f'or "{_REVISE_ANSWER} <what to change>": ',
file=sink,
)
sink.flush()
line = source.readline()
if line == "":
raise PlanReviewInputError(
"the plan review reached end of input without an answer. Silence is not a "
"sign-off: the exploration will not run a plan nobody approved"
)
answer = line.strip()
if answer == _APPROVE_ANSWER:
return PlanReviewDecision.approve()
verb, _, feedback = answer.partition(" ")
if verb == _REVISE_ANSWER and feedback.strip():
return PlanReviewDecision.revise(feedback.strip())
print(f"Not an answer: {answer!r}.", file=sink)
return review
# ---------------------------------------------------------------------------------------------
# The tools. Level 1 of the three-guarantee table: real computation, ADVISORY verdicts.
# ---------------------------------------------------------------------------------------------

4
src/portfolio_optimiser/hosting.py
View file

@ -167,7 +167,9 @@ async def _shaped_mandate(consumed: Mapping[str, Any], kwargs: Mapping[str, Any]
if contract.enable_plan_review:
raise InvocationRefused(
"explore_contract sets enable_plan_review, but this surface has no reviewer to answer "
"it: the synchronous plan review would block the request on nobody"
"it: the synchronous plan review would block the request on nobody, and would block "
"the event loop that answers /readiness while doing it. The operator door is the CLI's "
"--plan-review (or explore(..., plan_reviewer=...) in-process)"
)
result = await explore(
str(prompt),

71
src/portfolio_optimiser/run.py
View file

@ -64,6 +64,7 @@ from portfolio_optimiser.explore import (
explore,
exploration_notice,
load_exploration_contract,
terminal_plan_reviewer,
trace_payload,
)
from portfolio_optimiser.generate import ParseFailure, generate_via_llm
@ -1633,8 +1634,18 @@ def main(argv: list[str] | None = None) -> int:
help="the exploration's bounds (JSON, fail-fast, REQUIRES --explore): max_rounds, "
"max_tokens, max_stall_count, max_reset_count, max_plan_revisions, enable_plan_review. "
"Every field is required and none has a default — an omitted bound would fall back to "
"MAF's unbounded loop, not to something conservative. enable_plan_review must be false "
"here: the synchronous review has no reviewer on this surface",
"MAF's unbounded loop, not to something conservative. enable_plan_review requires "
"--plan-review, which is what answers it",
)
parser.add_argument(
"--plan-review",
action="store_true",
help="U13 synchronous HITL door (REQUIRES --explore, and --explore-config must set "
"enable_plan_review): answer the exploration's plan review AT THIS TERMINAL. Before the "
'loop is allowed to run you are shown the plan and answer "approve" or "revise <what to '
'change>"; a revision goes back to the manager, which replans and asks you again. Every '
"round trip is recorded in {run_id}-exploration.json, feedback verbatim. Input that ends "
"without an answer is an error, NEVER a sign-off",
)
parser.add_argument(
"--mcp-config",
@ -1778,6 +1789,10 @@ def main(argv: list[str] | None = None) -> int:
"--scripted-replies": args.scripted_replies is not None,
"--explore": args.explore is not None,
"--explore-config": args.explore_config is not None,
# Report mode returns BELOW, before every exploration refusal, so a flag missing from
# this list is silently dropped rather than refused — which is the whole reason the
# list enumerates every distinguishable flag instead of the ones that would misbehave.
"--plan-review": args.plan_review,
}
if any(report_forbidden.values()):
print(
@ -1823,6 +1838,11 @@ def main(argv: list[str] | None = None) -> int:
# --portfolio --explore has to hear which of the two is wrong.
"--explore": args.explore,
"--explore-config": args.explore_config,
# It answers --explore's review, so it lives on the same side of the partition. By
# NAME for the same reason --explore is: falling through to "--plan-review requires
# --explore" would tell an operator who wrote --portfolio --plan-review to add the one
# flag this mode also refuses.
"--plan-review": args.plan_review,
}
offending = [name for name, value in single_only.items() if value]
if offending:
@ -1922,6 +1942,16 @@ def main(argv: list[str] | None = None) -> int:
# the library API; the refusal names it rather than only forbidding.
# 4. --explore + --live-dry-run contradict: the drill stops before the first model call and an
# exploration IS model calls (the --scripted-replies precedent, same words).
# 5. --plan-review alone answers a review that is never requested (the (1) case, for the U13
# door). Its config-dependent half — the flag against a config that asks for no review, and
# a config that asks for one with no flag — is refused below, once the bounds are loaded.
if args.plan_review and args.explore is None:
print(
"run refused: --plan-review requires --explore (there is no plan to review without an "
"exploration, so the flag would be accepted and then never used)",
file=sys.stderr,
)
return 1
if args.explore_config is not None and args.explore is None:
print(
"run refused: --explore-config requires --explore (the bounds describe an exploration "
@ -1984,16 +2014,31 @@ def main(argv: list[str] | None = None) -> int:
except (FileNotFoundError, ValidationError, ValueError) as exc:
print(f"run refused: {exc}", file=sys.stderr)
return 1
if exploration_contract.enable_plan_review:
# Refused HERE rather than left to ``explore()``, which refuses it too: ExplorationError
# is a RuntimeError and therefore outside this CLI's (ValueError, FileNotFoundError,
# ValidationError) refusal tuple, so it would leave as a traceback instead of the rc 1
# line every other misconfiguration produces. The synchronous review (U13) needs a
# reviewer that blocks the loop, and this surface has none to offer.
# Both halves are refused HERE rather than left to ``explore()``, which refuses them too:
# ExplorationError is a RuntimeError and therefore outside this CLI's (ValueError,
# FileNotFoundError, ValidationError) refusal tuple, so either would leave as a traceback
# instead of the rc 1 line every other misconfiguration produces.
#
# The two messages share the token ``enable_plan_review`` and must NOT share their
# distinguishing wording: a test asserting on the shared substring passes against a
# surface missing one of the branches entirely (measured in økt 57 on --explore).
if exploration_contract.enable_plan_review and not args.plan_review:
# The refusal SURVIVES F4 — a run must never stop at a review nobody can answer — but
# its old wording ("the synchronous door is the library API") stopped being true the
# moment this CLI grew one, so it names the flag instead. A claim a surface makes about
# itself is exactly what Fase 3 measured drifting.
print(
"run refused: --explore-config sets enable_plan_review, but this surface has no "
"reviewer to answer it (the run would stop at a review nobody can answer). The "
"synchronous door is the library API: explore(..., plan_reviewer=...)",
"run refused: --explore-config sets enable_plan_review but no reviewer was "
"offered, so the run would stop at a review nobody can answer. Add --plan-review "
"to answer it at this terminal, or set enable_plan_review to false",
file=sys.stderr,
)
return 1
if args.plan_review and not exploration_contract.enable_plan_review:
print(
"run refused: --plan-review was given but --explore-config sets enable_plan_review "
"false, so no review is ever requested and the reviewer would never be asked "
"anything (refused, never silently ignored)",
file=sys.stderr,
)
return 1
@ -2089,6 +2134,10 @@ def main(argv: list[str] | None = None) -> int:
profile=args.profile,
client_factory=scripted_client_factory,
trace=exploration_trace,
# The F4 door. Built here and never inside ``explore()``: the loop owns the
# seam, the CLI owns which reviewer fills it, and a library that reached for
# stdin on its own would answer for a caller that never offered to.
plan_reviewer=terminal_plan_reviewer() if args.plan_review else None,
)
)
finally:

12
tests/test_explore_callsites_loadbearing.py
View file

@ -369,19 +369,27 @@ def test_an_exploration_with_no_knowledge_base_is_refused(tmp_path, capsys) -> N
def test_a_plan_review_nobody_can_answer_is_refused_at_the_cli(tmp_path, capsys) -> None:
"""T9: ``enable_plan_review`` is the U13 SYNCHRONOUS door and this surface has no reviewer.
"""T9: ``enable_plan_review`` is the U13 SYNCHRONOUS door, and a run must never stop at a
review nobody offered to answer.
Refused HERE rather than left to ``explore()``: ``ExplorationError`` is a ``RuntimeError``, so
it is outside ``main()``'s ``(ValueError, FileNotFoundError, ValidationError)`` refusal tuple
and would leave as a traceback instead of the rc-1 line every other misconfiguration produces.
**The assertion names wording unique to THIS branch.** Since F4 the CLI has a second refusal
carrying ``enable_plan_review`` (``--plan-review`` against a config that asks for no review),
so asserting on the shared token would pass against a surface missing this branch entirely —
the økt-57 mutation, in the form this repo keeps meeting it.
Detach point: let the flag through to ``explore()`` → RED (traceback, not rc 1).
"""
rc = run.main(
_base_argv(tmp_path)[:-1] + [_config_file(tmp_path, enable_plan_review=True)],
)
assert rc == 1
assert "enable_plan_review" in capsys.readouterr().err
err = capsys.readouterr().err
assert "no reviewer was offered" in err
assert "--plan-review" in err, "the refusal must name the door that answers it (F4)"
def test_explore_belongs_to_single_project_mode(tmp_path, capsys) -> None:

405
tests/test_plan_review_cli_door_loadbearing.py Normal file
View file

@ -0,0 +1,405 @@
"""F4 (docs/2026-08-25-fable-misjonsreview.md) — "be om svar, bruke svarene" must be reachable
from an OPERATOR surface, not only from the library API.
Before this, ``explore(..., plan_reviewer=...)`` (``explore.py:849``) was the sole door onto the
synchronous plan review, and BOTH operator surfaces refused ``enable_plan_review`` outright
(``run.py:1975-1988``, ``hosting.py:167-172``) — measured against the source, not taken from the
review's prose. Målbilde's "still spørsmål, be om svar, bruke svarene" was therefore unreachable
by anyone who was not importing the package.
**The gate is the SECOND half of that phrase.** A door that prints the plan, reads a line and
throws it away passes "the operator was asked" and fails the målbilde — this repo's vacuous-gate
class, eight times over. Every test here is therefore built so an always-approve reviewer is RED:
the discriminator is that a ``revise`` reaches the manager, the manager replans, and the operator
is asked AGAIN about the NEW plan (the T15 shape from ``test_explore_loadbearing.py``, lifted to
the CLI), with the feedback recorded VERBATIM.
The witness is ``{run_id}-exploration.json`` rather than scraped stdout: ``trace_payload``
(``explore.py:285-293``) already carries the decisions, the feedback and the order, and it is
written from a ``finally`` — so it survives the one run that most needs it, the one a cap or an
unanswered review cut short.
"""
from __future__ import annotations
import io
import json
import subprocess
import sys
from pathlib import Path
from typing import Any
import pytest
from portfolio_optimiser import explore as ex
from portfolio_optimiser import hosting, run
_REPO = Path(__file__).resolve().parents[1]
_BUNDLE_DIR = _REPO / "shared" / "examples" / "bygg-energi-mikro"
_PID = "BYGG-KONTOR-NORD"
_RUN_ID = "plan-review-door"
_PROPOSER_REPLY = json.dumps(
{
"measure": "LED-retrofit",
"affected_items": [{"code": "ENERGI-TOTAL-EL", "quantity": 300000, "unit_cost": 1.0}],
"claimed_saving_nok": 30000,
}
)
_MANAGER_REPLY = json.dumps(
{
"is_request_satisfied": {"reason": "r", "answer": True},
"is_in_loop": {"reason": "r", "answer": False},
"is_progress_being_made": {"reason": "r", "answer": True},
"next_speaker": {"reason": "r", "answer": "hypothesiser"},
"instruction_or_question": {"reason": "r", "answer": "go"},
}
)
_REPLIES = {
"proposer": _PROPOSER_REPLY,
"checker": "VERDICT: APPROVE",
"manager": _MANAGER_REPLY,
"navigator": "NAVIGATOR: read the index.",
"hypothesiser": "HYPOTHESIS: " + json.dumps({"label": "Night setback", "rationale": "y"}),
}
_FEEDBACK = "Also test night setback on the ventilation."
def _config_file(tmp_path: Path, **overrides: Any) -> str:
path = tmp_path / "exploration.json"
path.write_text(
json.dumps(
{
"max_rounds": 4,
"max_tokens": 200_000,
"max_stall_count": 2,
"max_reset_count": 1,
"max_plan_revisions": 2,
"enable_plan_review": True,
**overrides,
}
),
encoding="utf-8",
)
return str(path)
def _replies_file(tmp_path: Path) -> str:
path = tmp_path / "replies.json"
path.write_text(json.dumps(_REPLIES), encoding="utf-8")
return str(path)
def _argv(tmp_path: Path, *, plan_review: bool = True, **config: Any) -> list[str]:
argv = [
_PID,
"--docs-dir",
str(_BUNDLE_DIR),
"--bundle-dir",
str(_BUNDLE_DIR),
"--explore",
"Find the cheapest saving.",
"--explore-config",
_config_file(tmp_path, **config),
"--scripted-replies",
_replies_file(tmp_path),
"--outbox-dir",
str(tmp_path / "outbox"),
"--run-id",
_RUN_ID,
]
if plan_review:
argv.append("--plan-review")
return argv
def _artefact(tmp_path: Path) -> dict[str, Any]:
path = tmp_path / "outbox" / f"{_RUN_ID}-exploration.json"
assert path.exists(), "the exploration artefact must be written even when the run failed"
return json.loads(path.read_text(encoding="utf-8"))
def _stdin(monkeypatch: pytest.MonkeyPatch, text: str) -> None:
monkeypatch.setattr(sys, "stdin", io.StringIO(text))
# ---------------------------------------------------------------------------------------------
# 1. THE GOAL — asked, answered, and the answer USED
# ---------------------------------------------------------------------------------------------
def test_an_operator_answer_typed_at_the_cli_reaches_the_manager_and_is_asked_again(
tmp_path, capsys, monkeypatch
) -> None:
"""T1: the whole point. ``revise`` typed at the CLI reaches the manager, the manager replans,
and the operator is asked to sign off on the NEW plan.
RED against an always-approve reviewer (the vacuous door): that yields ONE review and
``["approve"]``, so both the count and the order fail. RED against a reviewer that reads the
line and discards it: the feedback assertion fails and the manager is never asked twice.
Detach point: drop the ``plan_reviewer=`` wiring in ``run.py`` → the CLI refuses instead
(there is no reviewer), so this never runs at all.
"""
_stdin(monkeypatch, f"revise {_FEEDBACK}\napprove\n")
rc = run.main(_argv(tmp_path))
err = capsys.readouterr().err
assert "Traceback" not in err, err
assert rc in (0, 1), f"expected a clean exit, got rc={rc} stderr={err!r}"
reviews = _artefact(tmp_path)["plan_reviews"]
assert [r["decision"] for r in reviews] == ["revise", "approve"], (
"a revision must produce a SECOND review, not resume silently — an always-approve door "
"gives ['approve']"
)
assert reviews[0]["feedback"] == _FEEDBACK, (
"what a human told the loop is worth nothing paraphrased"
)
assert reviews[1]["plan"] != "", "the second review must show the replanned plan"
def test_the_operator_is_shown_the_plan_and_the_answer_vocabulary(
tmp_path, capsys, monkeypatch
) -> None:
"""T2: a review nobody can read is a review nobody can answer. The prompt carries the plan,
the progress and the two words that answer it.
Detach point: print only "plan review?" → RED. The control for T1: T1 proves the answer is
used, this proves the question was askable.
"""
_stdin(monkeypatch, "approve\napprove\napprove\n")
run.main(_argv(tmp_path))
out = capsys.readouterr().out
reviews = _artefact(tmp_path)["plan_reviews"]
assert reviews, "the run must actually have reached a plan review"
assert reviews[0]["plan"][:40] in out, (
"the operator must be shown the plan they are signing off"
)
assert "approve" in out and "revise" in out, "the prompt must name the vocabulary it accepts"
def test_an_unrecognised_answer_is_asked_again_never_taken_as_a_sign_off(
tmp_path, capsys, monkeypatch
) -> None:
"""T3: fail-closed on the operator's own input. Anything outside the vocabulary is re-asked;
it is never read as approval, and never as a revision either.
Detach point: treat any non-``revise`` line as approve → the first line ("yes please") would
sign the plan off and the recorded decision would still be ``approve``, so the count of
prompts is what discriminates: RED here, green there.
"""
_stdin(monkeypatch, f"yes please\n\nrevise {_FEEDBACK}\napprove\n")
run.main(_argv(tmp_path))
out = capsys.readouterr().out
reviews = _artefact(tmp_path)["plan_reviews"]
assert [r["decision"] for r in reviews] == ["revise", "approve"], (
"the junk line and the blank line must both be re-asked, not consumed as decisions"
)
assert reviews[0]["feedback"] == _FEEDBACK
assert out.count("PLAN REVIEW") == 2, (
"two REVIEWS were answered; a third prompt would mean a junk line was consumed as one"
)
def test_end_of_input_never_becomes_an_approval_and_the_evidence_still_lands(
tmp_path, monkeypatch
) -> None:
"""T4: the silence that must not be read as a yes.
A pipe that ends — or an operator who walks away — leaves the review unanswered. Reading that
as approval would let an autonomous loop run on a plan no human signed, which is the exact
thing the door exists to prevent, and it would do so invisibly. It raises instead.
The artefact is asserted TOO, and that is the load-bearing half: the write is in a ``finally``
(``run.py``), so the run that failed still leaves a record of what the operator was asked and
what they had answered so far. ``completed: false`` is what says the run never finished
(``trace_payload``'s required field — an absent ``stop`` cannot say it).
Detach point: return ``PlanReviewDecision.approve()`` at EOF → no exception, ``completed``
true, and the loop runs on an unsigned plan (RED on all three).
"""
_stdin(monkeypatch, "")
with pytest.raises(ex.PlanReviewInputError):
run.main(_argv(tmp_path))
artefact = _artefact(tmp_path)
assert artefact["completed"] is False
assert artefact["plan_reviews"] == [], "nothing was decided, so nothing may be recorded"
# ---------------------------------------------------------------------------------------------
# 2. The refusals — every combination that would silently drop the flag
# ---------------------------------------------------------------------------------------------
def test_plan_review_without_an_exploration_is_refused_by_name(tmp_path, capsys) -> None:
"""T5: there is no plan to review without an exploration. Refused rather than loaded and
dropped — the ``--explore-config`` precedent, verbatim.
Detach point: accept it silently → RED (rc 0, flag ignored).
"""
rc = run.main([_PID, "--docs-dir", str(_BUNDLE_DIR), "--plan-review", "--live-dry-run"])
assert rc == 1
assert "--plan-review" in capsys.readouterr().err
def test_plan_review_against_a_config_that_asks_for_no_review_is_refused(tmp_path, capsys) -> None:
"""T6: a reviewer nobody will ever call. ``explore()`` refuses this too, but as an
``ExplorationError`` — a ``RuntimeError``, outside ``main()``'s refusal tuple — so it would
leave as a traceback instead of the rc-1 line. Hoisted here for that reason alone.
The assertion names wording UNIQUE to this branch: after F4 the CLI has two refusals
containing ``enable_plan_review``, and asserting on the shared token is this repo's
"assert never on wording two branches share" defect (measured in økt 57).
Detach point: leave it to ``explore()`` → RED (traceback, not rc 1).
"""
rc = run.main(_argv(tmp_path, enable_plan_review=False, max_plan_revisions=0))
assert rc == 1
assert "no review is ever requested" in capsys.readouterr().err
def test_a_review_with_no_reviewer_still_refuses_and_now_names_the_door(tmp_path, capsys) -> None:
"""T7: the opposite half — the config asks for a review and no ``--plan-review`` was given.
The refusal SURVIVES F4 (a run must never stop at a review nobody can answer), but its wording
was a claim the surface made about itself: "the synchronous door is the library API" stopped
being true the moment this CLI grew one. It now names the flag.
Detach point: leave the old wording → RED. Same class as the Fase 3 credential claim.
"""
rc = run.main(_argv(tmp_path, plan_review=False))
assert rc == 1
err = capsys.readouterr().err
assert "no reviewer was offered" in err
assert "--plan-review" in err, "the refusal must name the door that answers it"
def test_plan_review_belongs_to_single_project_mode(tmp_path, capsys) -> None:
"""T8: the documented partition. ``--explore`` is single-project-only and ``--plan-review``
answers its review, so a ``--portfolio --plan-review`` argv has to hear which flag is wrong.
The assertion names ``--portfolio`` rather than ``--plan-review``: the refusal below it
(``--plan-review requires --explore``) names ``--plan-review`` too, and asserting on the shared
token would pass against no partition entry at all — the økt-57 mutation, verbatim.
Detach point: leave it out of ``single_only`` → RED (the message names --explore only, or the
run falls through to the requires-refusal).
"""
rc = run.main(["--portfolio", "--plan-review"])
assert rc == 1
assert "--portfolio" in capsys.readouterr().err
def test_plan_review_is_refused_in_report_mode(tmp_path, capsys) -> None:
"""T9: ``--report`` returns BEFORE every exploration refusal, so a flag missing from
``report_forbidden`` is silently dropped rather than refused — the reason that list enumerates
every distinguishable flag in the first place.
Detach point: leave it out of ``report_forbidden`` → rc 0 and a printed report, the flag gone
without a word (RED).
"""
ledger = tmp_path / "ledger.json"
ledger.write_text(json.dumps({"entries": []}), encoding="utf-8")
rc = run.main(["--report", "--ledger", str(ledger), "--plan-review"])
assert rc == 1
assert "mode-exclusive" in capsys.readouterr().err
@pytest.mark.asyncio
async def test_the_hosted_surface_still_refuses_and_names_the_cli_door() -> None:
"""T10: hosting keeps its refusal — a synchronous review would block the HTTP request on a
reviewer that does not exist, and it would block the event loop that answers ``/readiness``
while doing it. What changes is the honesty of the message: there is now an operator door,
and the refusal says where.
Detach point: leave the message pointing only at the library API → RED (the same claim-drift
class as the Fase 3 credential line).
"""
with pytest.raises(ValueError) as excinfo:
await hosting.invoke(
{
"project_id": _PID,
"docs_dir": str(_BUNDLE_DIR),
"verdict_input": {"decision": "approved", "rationale": "expert reviewed"},
"profile": "local",
"bundle_dir": str(_BUNDLE_DIR),
"explore_prompt": "p",
"explore_contract": {
"max_rounds": 4,
"max_tokens": 200_000,
"max_stall_count": 2,
"max_reset_count": 1,
"max_plan_revisions": 2,
"enable_plan_review": True,
},
}
)
assert "--plan-review" in str(excinfo.value)
# ---------------------------------------------------------------------------------------------
# 3. A real argv, in a real process (the P4 precedent)
# ---------------------------------------------------------------------------------------------
def test_the_flag_answers_a_review_from_a_real_argv(tmp_path) -> None:
"""T11: in-process tests patch ``sys.stdin`` and call ``main()`` directly, so neither proves
the flag exists on the parsed command line or that a real pipe reaches the reviewer. A child
process settles both — the same reason the demo's stderr and the hosting shim are measured in
a subprocess rather than with ``capsys``.
Detach point: never add the argparse flag → the child exits 2 with an argparse usage error
(RED).
"""
argv = _argv(tmp_path)
proc = subprocess.run(
[sys.executable, "-m", "portfolio_optimiser.run", *argv],
input=f"revise {_FEEDBACK}\napprove\n",
capture_output=True,
text=True,
cwd=_REPO,
)
assert "unrecognized arguments" not in proc.stderr, proc.stderr
assert "Traceback" not in proc.stderr, proc.stderr
reviews = _artefact(tmp_path)["plan_reviews"]
assert [r["decision"] for r in reviews] == ["revise", "approve"]
assert reviews[0]["feedback"] == _FEEDBACK
def test_the_reviewer_reads_the_stream_that_exists_when_it_is_asked(monkeypatch) -> None:
"""T12: the streams are resolved at CALL time, not when the reviewer is built.
A factory that captured ``sys.stdin`` at construction would answer from whatever stream
happened to be installed when ``run.py`` built the reviewer — before the loop, before anything
was asked. Nothing above catches that (every other test here installs its stream first), so the
claim would be prose. This asks the question the other way round: build FIRST, swap AFTER.
Detach point: resolve the streams in ``terminal_plan_reviewer``'s body instead of inside
``review`` → RED (the reviewer reads the stream that is gone).
"""
reviewer = ex.terminal_plan_reviewer()
monkeypatch.setattr(sys, "stdin", io.StringIO(f"revise {_FEEDBACK}\n"))
monkeypatch.setattr(sys, "stdout", io.StringIO())
decision = reviewer(
ex.PlanReviewRequest(index=0, plan="a plan", current_progress="", is_stalled=False)
)
assert decision.feedback == _FEEDBACK