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

feat(major2): proposal-review types, record, error and renderer in a MAF-free module [skip-docs]

Browse source 
Ordre 20260904T173146Z-8102814273-from-portfolio-optimiser, steg 1 av 10.

Vokabularet for MAJOR-2-doeren: forespoerselen en ekspert vises, svaret de gir,
posten som registreres, feilen en stillhet reiser, og de to rendererne.

D5(c): egen MAF-fri modul. Maalt import-grense - generate.py er kallstedet og
importerer agent_framework core, men ikke explore; explore.py (F4-soesknenes hjem)
importerer agent_framework.orchestrations. Typene der ville enten dratt
orkestrerings-importen inn i genererings-stien eller definert typen to ganger.
Gatet av tests/test_okf.py::test_okf_is_maf_free.

Klassen ER kanalen: ProposalReviewInputError er en RuntimeError og IKKE en
ValueError - begge halvdeler assertert, fordi issubclass(X, RuntimeError) alene
staar groenn paa en klasse som arver begge.

verdict_key INJISERES i renderen (key_of), fordi verdicts.py importerer MAF og
run er en sykel herfra - _features_of forblir eneste hjem for regelen.

Notice-en sier fra ogsaa paa NULL anmeldelser naar en reviewer VAR tilbudt: et
bevisst avvik fra announce-regelens null-er-stillhet-halvdel, fordi en operatoer
som ga --proposal-review og ser ingenting ikke kan skille "ingen kandidat ble
validert" fra "doeren hang".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-09-05 06:44:56 +02:00
commit ce4c15e6cb
3 changed files with 423 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

217
src/portfolio_optimiser/proposal_review.py Normal file
View file

@ -0,0 +1,217 @@
"""MAJOR-2 — the synchronous HITL door onto the proposal that is ON THE TABLE.
The framework already loops the MACHINE's own falsification back into the same run (Step 5,
``generate._build_messages(prior_rejection=…)``) and lands a HUMAN's verdict in the *next* run
(Step 7's verdict inbox). Between those two there was nothing: no person could speak to a
concrete candidate while it still stood. This module is that seam's vocabulary — the request a
reviewer is shown, the answer they give, the record of what they said, the error a silence
raises, and the two renderers that carry it to disk and to stdout.
**A MAF-free module, and that placement is measured** (D5(c)). ``generate.py`` is the call site
and imports ``agent_framework`` core but not ``explore``; ``explore.py`` — where the F4 plan-review
siblings live — imports ``agent_framework.orchestrations``. Defining these types there would
either pull the orchestration import into the generation path or define the type twice. A new
home imported by both ``generate`` and ``run`` costs no new import edge, and is guarded by
``tests/test_okf.py::test_okf_is_maf_free``.
**Mirrored on F4 by SHAPE, never shared with it.** The terminal door added beside these types
is a sibling of ``explore.terminal_plan_reviewer``: streams resolved at call time, the same
closed vocabulary, the same re-ask loop, the same rule that end of input is an error and never
a sign-off. A common "terminal reviewer" abstraction over the two is the single-use
generalisation this repo refuses until a third door exists.
What the door does NOT do: ``approve`` mints no ``Verdict`` (F2 — a verdict still arrives
through ``--decision/--rationale`` or the Step-7 inbox), and a ``revise`` is not a rejection *by
the human* — it never writes ``provenance.validator_decision`` and never touches the checker
gate. The expert is a third voice that gates nothing except whether one more attempt is spent;
what the validator then rules is the validator's (D6).
"""
from __future__ import annotations
from collections.abc import Callable, Sequence
from dataclasses import dataclass
from typing import TYPE_CHECKING, Any, Final, Literal
if TYPE_CHECKING: # pragma: no cover - typing only, keeps the runtime import surface minimal
from portfolio_optimiser.ir import SavingsProposal
from portfolio_optimiser.validator import ValidatedProposal
@dataclass(frozen=True)
class ProposalReviewRequest:
"""What the reviewer is shown about ONE validated candidate.
It carries no ``ProvenanceStamp``, and that is a measurement rather than an omission (brief
Q1): the stamp is built in ``run.py`` AFTER generation returns, while this request arises
INSIDE ``generate_via_llm``'s attempt loop — the only place a ``revise`` can still buy an
attempt under the existing bound. A reviewer handed the stamp could not sit where the answer
can still be used, and the draft's ``(proposal, verdict, provenance)`` triple is therefore
not constructible without the new loop the brief forbids.
``approach_id`` / ``approach_label`` say WHICH candidate is being asked about: with a
commissioned mandate one expert can be asked N times at one terminal, and two candidates that
cannot be told apart are two answers that cannot be trusted. ``project_id`` is carried for the
same reason one level up — under the multi-base dispatcher the same terminal serves several
bases.
``attempts_remaining`` is what a ``revise`` can ACTUALLY buy: the smaller of the attempt
budget's headroom and the shared round ledger's (Step 3). A number computed from
``max_attempts`` alone would be a claim the terminal makes about itself that the ledger then
refutes — the Fase-3 class.
``checker_verdict`` is the run-level reasoning gate's answer, read-only (D3). It helps an
expert decide whether to spend an attempt; it is NOT part of the decision type and never
enters the record, because the two falsifiers are never blended.
"""
project_id: str
approach_id: str | None
approach_label: str
attempt: int
attempts_remaining: int
proposal: ValidatedProposal
checker_verdict: str
@dataclass(frozen=True)
class ProposalReviewDecision:
"""The reviewer's answer. ``feedback is None`` means approve.
Two named constructors, the ``explore.PlanReviewDecision`` shape: a bare ``revise`` is
refused at CONSTRUCTION, so no surface has to invent for itself whether an empty revision is
a decision. Validation, never repair.
"""
feedback: str | None
@staticmethod
def approve() -> ProposalReviewDecision:
return ProposalReviewDecision(feedback=None)
@staticmethod
def revise(feedback: str) -> ProposalReviewDecision:
if not feedback.strip():
raise ValueError("a revision must say what to change; use approve() to take it as is")
return ProposalReviewDecision(feedback=feedback)
#: The synchronous HITL seam (MAJOR-2). Given the request, answer it. Called in-process from
#: inside the attempt loop, so generation blocks on it exactly as a human at a terminal would —
#: which is also why the hosted surface refuses the door outright.
ProposalReviewer = Callable[[ProposalReviewRequest], ProposalReviewDecision]
@dataclass(frozen=True)
class ProposalReview:
"""One review round trip, recorded VERBATIM.
``honoured`` means **the attempt this revise bought actually FETCHED a reply** — not "was
bought". A revise recorded and then cut by the round ledger before its follow-up fetched
reads ``False``, because at that point the expert asked for a change the run never made; the
flag is set only after the bought attempt's fetch returns. An ``approve`` is trivially
honoured: nothing was asked for.
``proposal`` is the ``ValidatedProposal`` the expert was LOOKING at, kept whole rather than
reduced to a key here, because the key derivation lives in ``run`` (see
``proposal_reviews_payload``). Keeping it is what lets the artefact still show the validated
candidate when a later ruling replaces it (D6).
"""
approach_id: str | None
attempt: int
decision: Literal["approve", "revise"]
feedback: str
honoured: bool
proposal: ValidatedProposal
class ProposalReviewInputError(RuntimeError):
"""A proposal review was left without an answer: the input ended mid-review.
**The class IS the channel, decided by three measurements** (brief § Constraints). The F4
sibling ``PlanReviewInputError`` is a ``RuntimeError`` that no arm catches, so it escapes as
a traceback. A ``ValueError`` would land on ``run.py``'s refusal tuple and print
``run refused: …`` — but the argv was fine and the run had already spent tokens, so "refused"
mislabels it; worse, ``_fetch_parsed`` catches ``ValueError`` one frame away as a *parse
failure*, so a ``ValueError``-shaped human-input error raised inside that frame would be
appended verbatim to ``parse_failures`` and the model re-called until the round ledger fired.
So: a ``RuntimeError``, caught BY NAME at the CLI's full-run dispatch, printed on a distinct
``run stopped:`` line with rc 1 — no traceback, and nothing near the parse catch-all. F4's
own traceback is an asymmetry this door states rather than fixes.
"""
#: The closed answer vocabulary of the terminal door. Two words, matched structurally.
_APPROVE_ANSWER: Final = "approve"
_REVISE_ANSWER: Final = "revise"
def proposal_reviews_payload(
reviews: Sequence[ProposalReview],
*,
key_of: Callable[[SavingsProposal], str],
) -> dict[str, Any]:
"""The ONE renderer of the review record for the outbox (the ``tool_call_payload`` form: a
pure function in the owning module, consumed by ``outbox``, which stays MAF-free).
Structured fields, never a rendered string (kø-(y)): "which candidate", "what was said" and
"did the run act on it" are three different operative questions and a sentence fuses them.
``key_of`` is INJECTED rather than derived here, and that is a measurement:
``verdicts.verdict_key`` takes a ``ProposalFeatures``, the only derivation from a
``SavingsProposal`` is ``run._features_of``, and this module can import neither ``verdicts``
(it imports MAF) nor ``run`` (a cycle). Injection keeps ``_features_of`` the single home of
that rule (kø-(p)) instead of growing a second one here.
``p50`` travels beside the key for D6's sake: when an honoured revise's follow-up is rejected
and the attempts run out, the run carries the ``Rejection`` — and this is where an operator
can still see that a validated candidate existed and that a human asked for it to change.
"""
return {
"reviews": [
{
"approach_id": review.approach_id,
"attempt": review.attempt,
"decision": review.decision,
"feedback": review.feedback,
"honoured": review.honoured,
"verdict_key": key_of(review.proposal.proposal),
"p50": review.proposal.p50,
}
for review in reviews
]
}
def proposal_review_notice(reviews: Sequence[ProposalReview], *, offered: bool) -> str | None:
"""The ONE renderer for the review line on stdout — ``None`` when no reviewer was offered.
It takes the already-resolved reviews and the already-resolved flag, never argv, so the
printed line and ``RunResult.expert_revisions`` descend from the same fact
(``cost_baseline_notice``'s precedent).
**Zero reviews with a reviewer present still SPEAKS**, and that is a deliberate departure
from the announce rule's zero-is-silence half: an operator who passed ``--proposal-review``
and sees nothing cannot tell "no candidate was ever validated, so nobody was asked" from
"the door hung". The departure is stated in the invariant row rather than left to be
discovered as an inconsistency.
``verdict_notice`` is untouched beside it: "no expert verdict given" stays true under F2,
because an ``approve`` here is "stop asking; I take it as is", never an expert verdict.
"""
if not offered:
return None
if not reviews:
return " proposal review offered, never consulted (no candidate validated)"
answers = len(reviews)
candidates = len({review.approach_id for review in reviews})
approvals = sum(1 for review in reviews if review.decision == _APPROVE_ANSWER)
revisions = answers - approvals
unhonoured = sum(1 for review in reviews if not review.honoured)
tail = f" ({unhonoured} not honoured)" if unhonoured else ""
return (
f" proposal review: {answers} answer(s) across {candidates} candidate(s) — "
f"{approvals} approve, {revisions} revise{tail}"
)

1
tests/test_okf.py
View file

@ -28,6 +28,7 @@ _MAF_FREE_MODULES = [
"hitl.py", "hitl.py",
"notify.py", "notify.py",
"semretrieval.py", "semretrieval.py",
"proposal_review.py",
] ]
_EXAMPLES_DIR = Path(__file__).resolve().parents[1] / "shared" / "examples" _EXAMPLES_DIR = Path(__file__).resolve().parents[1] / "shared" / "examples"

205
tests/test_proposal_review_loop_loadbearing.py Normal file
View file

@ -0,0 +1,205 @@
"""MAJOR-2 — a human must be able to answer the proposal that is ON THE TABLE, and the answer
must be USED.
Measured before this door existed (``docs/2026-09-02-misjonsreview-v2.md`` § 3, re-measured at
HEAD ``da5f10f``): ``grep -n "prior_" generate.py`` finds exactly ONE seam — ``prior_rejection``,
the MACHINE's own falsification fed back inside the same run — and the only human seam in the
loop is ``plan_reviewer``, which fires BEFORE any hypothesis exists. A human's verdict lands in
the NEXT run (Step 7's inbox). So "improved proposals given feedback" was, for a person, a
two-run loop with days between it.
**The gate is the SECOND half of that phrase.** A door that renders the candidate, reads a line
and discards it passes "the expert was asked" and fails the målbilde — this repo's vacuous-gate
class, fifteen times over. Every arm here is therefore built so that an **always-approve
reviewer is RED**: the discriminator is that a ``revise`` reaches the NEXT generation attempt's
prompt VERBATIM, that the proposer answers differently because of it, and that the run carries
the validator's ruling on THAT attempt (D6).
Three things the door must NOT do, gated as hard as the things it must: ``approve`` mints no
``Verdict`` (F2 stands); a ``revise`` never writes ``provenance.validator_decision`` and never
touches the checker gate (the two falsifiers stay unblended — the human is a third voice that
gates nothing except asking for one more attempt); and a run with NO reviewer is byte-identical
to today, golden transcript included.
"""
from __future__ import annotations
import json
from pathlib import Path
import pytest
from portfolio_optimiser import proposal_review as pr
from portfolio_optimiser.ir import AffectedItem, SavingsProposal
from portfolio_optimiser.validator import ValidatedProposal
_REPO = Path(__file__).resolve().parents[1]
_BUNDLE_DIR = _REPO / "shared" / "examples" / "bygg-energi-mikro"
_PID = "BYGG-KONTOR-NORD"
_RUN_ID = "proposal-review-door"
#: A string that can reach a generation prompt ONLY by travelling through the expert's feedback:
#: asserted absent from every file of the fixture bundle and from the base prompt (Step 2), so a
#: prompt that carries it can have got it from exactly one place.
_FEEDBACK_SENTINEL = "PROPOSAL-REVIEW-SENTINEL-9c41ae"
#: The shape of the defect BLOCKER-1 measured, asserted NEGATIVELY: any object rendered by its
#: default repr onto a surface a human reads. Stronger than a positive sentinel alone, which a
#: renderer that prints nothing at all would also satisfy.
_REPR_LEAK = " object at 0x"
def _proposal(*, amount: float = 30000.0, measure: str = "LED-retrofit") -> SavingsProposal:
return SavingsProposal(
project_id=_PID,
measure=measure,
affected_items=[AffectedItem(code="ENERGI-TOTAL-EL", quantity=300000.0, unit_cost=1.0)],
claimed_saving_nok=amount,
)
def _validated(*, amount: float = 30000.0, p50: float = 42.0) -> ValidatedProposal:
return ValidatedProposal(
proposal=_proposal(amount=amount),
p10=p50 - 1.0,
p50=p50,
p90=p50 + 1.0,
nominal_feasible=p50 + 2.0,
)
def _review(
*,
approach_id: str | None = None,
attempt: int = 0,
decision: str = "approve",
feedback: str = "",
honoured: bool = True,
p50: float = 42.0,
) -> pr.ProposalReview:
return pr.ProposalReview(
approach_id=approach_id,
attempt=attempt,
decision=decision, # type: ignore[arg-type]
feedback=feedback,
honoured=honoured,
proposal=_validated(p50=p50),
)
# ---------------------------------------------------------------------------------------------
# Group A0 (Step 1) — the types, the error's CLASS, and the two renderers.
# ---------------------------------------------------------------------------------------------
def test_a_revision_must_say_what_to_revise() -> None:
"""Detach point: ``ProposalReviewDecision.revise`` accepting an empty string. A bare
``revise`` is not a decision — it is a line the door must ask again for. The type refuses it
at construction so no surface can invent the distinction for itself."""
with pytest.raises(ValueError):
pr.ProposalReviewDecision.revise("")
with pytest.raises(ValueError):
pr.ProposalReviewDecision.revise(" ")
assert pr.ProposalReviewDecision.revise("do X").feedback == "do X"
assert pr.ProposalReviewDecision.approve().feedback is None
def test_the_input_error_is_a_runtime_error_and_never_a_value_error() -> None:
"""Detach point: making ``ProposalReviewInputError`` a ``ValueError``. **The class IS the
channel**, decided by three measurements (brief § Constraints): a ``ValueError`` would land
on ``run.py``'s refusal tuple and print ``run refused:`` for a run whose argv was fine and
which had already spent tokens; and it would sit ONE frame from ``_fetch_parsed``'s
``except (ValidationError, ValueError, TypeError)`` catch-all, where a human-input error
would be appended to ``parse_failures`` and the model re-called until the ledger fired.
Both halves are asserted, because ``issubclass(X, RuntimeError)`` alone stays green on a
class that inherits from both."""
assert issubclass(pr.ProposalReviewInputError, RuntimeError)
assert not issubclass(pr.ProposalReviewInputError, ValueError)
def test_the_notice_is_none_exactly_when_no_reviewer_was_offered() -> None:
"""Detach point: a renderer that always returns its line. The ``announce`` rule: a run that
offered no reviewer has nothing to say, and an empty row would read as a dropped line."""
assert pr.proposal_review_notice((), offered=False) is None
assert pr.proposal_review_notice((_review(),), offered=False) is None
assert pr.proposal_review_notice((), offered=True) is not None
def test_zero_reviews_with_a_reviewer_offered_says_so_in_words() -> None:
"""Detach point: returning ``None`` on zero reviews with a reviewer present (M33).
A DELIBERATE departure from the announce rule's zero-is-silence half, and the reason is the
operator: someone who passed ``--proposal-review`` and sees nothing cannot tell "no candidate
was ever validated, so nobody was asked" from "the door hung". The departure is stated in the
invariant row rather than left as an inconsistency."""
line = pr.proposal_review_notice((), offered=True)
assert line == " proposal review offered, never consulted (no candidate validated)"
def test_the_notice_counts_answers_candidates_and_the_unhonoured_ones() -> None:
"""Detach point: dropping the un-honoured tail, or counting entries instead of candidates.
An un-honoured revise (no attempt left to buy) is a FACT the operator must be able to read:
the expert asked for a change that the run could not make."""
reviews = (
_review(approach_id="a1", attempt=0, decision="revise", feedback="F", honoured=True),
_review(approach_id="a1", attempt=1, decision="approve"),
_review(approach_id="a2", attempt=0, decision="revise", feedback="G", honoured=False),
)
assert pr.proposal_review_notice(reviews, offered=True) == (
" proposal review: 3 answer(s) across 2 candidate(s) — 1 approve, 2 revise "
"(1 not honoured)"
)
honoured_only = (
_review(approach_id="a1", attempt=0, decision="revise", feedback="F", honoured=True),
_review(approach_id="a1", attempt=1, decision="approve"),
)
assert pr.proposal_review_notice(honoured_only, offered=True) == (
" proposal review: 2 answer(s) across 1 candidate(s) — 1 approve, 1 revise"
)
def test_the_payload_carries_every_field_and_takes_its_key_from_the_injected_function() -> None:
"""Detach point: dropping ``verdict_key`` (M28), or deriving it inside the module.
``verdicts.verdict_key`` takes ``ProposalFeatures`` and the only derivation from a
``SavingsProposal`` is ``run._features_of`` — but ``verdicts.py`` imports MAF and ``run`` is
a cycle from here, so the key function is INJECTED and ``_features_of`` stays the single
home (kø-(p)). The arm asserts the injected callable is handed the REVIEWED proposal, not a
re-derived one."""
seen: list[SavingsProposal] = []
def key_of(proposal: SavingsProposal) -> str:
seen.append(proposal)
return f"key-{len(seen)}"
reviews = (
_review(approach_id="a1", attempt=0, decision="revise", feedback="F", honoured=False),
_review(approach_id=None, attempt=1, decision="approve", p50=99.5),
)
payload = pr.proposal_reviews_payload(reviews, key_of=key_of)
assert list(payload) == ["reviews"]
assert payload["reviews"] == [
{
"approach_id": "a1",
"attempt": 0,
"decision": "revise",
"feedback": "F",
"honoured": False,
"verdict_key": "key-1",
"p50": 42.0,
},
{
"approach_id": None,
"attempt": 1,
"decision": "approve",
"feedback": "",
"honoured": True,
"verdict_key": "key-2",
"p50": 99.5,
},
]
assert seen == [reviews[0].proposal.proposal, reviews[1].proposal.proposal]
# The payload is a PLAIN mapping: ``outbox`` stays MAF-free and byte-deterministic.
assert json.loads(json.dumps(payload)) == payload