feat(major2): the reviewer sits after the validator in the attempt loop; the outcome is the validator's last ruling [skip-docs]
Ordre 20260904T173146Z-8102814273-from-portfolio-optimiser, steg 3 av 10. generate_via_llm faar fire keyword-only parametre: reviewer, reviews (KALLER-EID sink), review_key og checker_verdict. Revieweren kalles synkront i det validate_proposal AKSEPTERER en kandidat - aldri paa en avvist (den er alt matet tilbake informert; aa be et menneske kommentere tall maskinen nettopp gjendrev bruker mennesket paa maskinens jobb). EXIT-KONTRAKTEN ER ENDRET. Foer dette hvilte utgangen paa `last`, som KUN en validator- avvisning setter - saa "validert -> revise" paa hvert forsoek naadde slutten av loekka med `last is None` og doede paa `assert last is not None` (og under -O paa None.proposal). `last_ruling` er naa en eksplisitt baerer, og D6 leses rett av den. Sinken er kaller-eid av parse_failures' MAALTE grunn, ett hakk skarpere: meter.tick_round reiser inne i _fetch_parsed paa forsoeket en revise kjoepte, saa paa noeyaktig den kjoeringen posten betyr mest returnerer funksjonen INGENTING. Et felt paa GenerationResult ville vaert blindt for det. attempts_remaining = min(max_attempts - i - 1, meter.budget.max_rounds - meter.rounds) - LEDGER-BEVISST, fordi rundeboka deles av hver approach i et mandat og ofte er det som binder. honoured betyr at forsoeket revisen kjoepte FAKTISK HENTET et svar, ikke at det ble kjoept: posten settes False og forfremmes foerst naar _fetch_parsed har returnert. RODT foer impl: 11 armer. TO AV PLANENS EGNE TALL BLE FALSIFISERT AV MAALINGEN og staar korrigert i testen: (1) planens T3 (max_rounds=2, max_attempts=10 => BudgetExceeded observed=3) er ikke naabar under den ledger-bevisste remaining fra planens egen revisjon 5 - loekka stopper etter to hentinger UTEN unntak; armen er delt i T3a (ledgeren stopper revisjonene, M38s diskriminator) og T3b (honoured=False naar den kjoepte hentingen aldri returnerte, M40s vitne, drevet via parse-retryen som gir noeyaktig rounds/2/3). (2) planens T-ledger sier attempts_remaining == 0 ved max_rounds=2/max_attempts=3; maalt er det 1 - armen bruker max_rounds=1, der ledgeren faktisk binder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
d4c8691326
commit
bbf4d3b5b6
2 changed files with 495 additions and 12 deletions
|
|
@ -25,16 +25,20 @@ to today, golden transcript included.
|
|||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from collections.abc import Sequence
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
import pytest
|
||||
from spikes._harness import message_texts
|
||||
from agent_framework import ChatResponse, ChatResponseUpdate, Message
|
||||
from spikes._harness import FakeChatClient, message_texts
|
||||
|
||||
from portfolio_optimiser import proposal_review as pr
|
||||
from portfolio_optimiser.generate import _build_messages
|
||||
from portfolio_optimiser.budget import Budget, BudgetExceeded, TokenMeter
|
||||
from portfolio_optimiser.generate import ParseFailure, _build_messages, generate_via_llm
|
||||
from portfolio_optimiser.ir import AffectedItem, SavingsProposal
|
||||
from portfolio_optimiser.reference_domain import load_reference_projects
|
||||
from portfolio_optimiser.validator import Rejection, ValidatedProposal
|
||||
from portfolio_optimiser.validator import Rejection, ValidatedProposal, proposal_for
|
||||
|
||||
_REPO = Path(__file__).resolve().parents[1]
|
||||
_BUNDLE_DIR = _REPO / "shared" / "examples" / "bygg-energi-mikro"
|
||||
|
|
@ -274,3 +278,370 @@ def test_the_feedback_sentinel_cannot_arrive_from_the_knowledge_base() -> None:
|
|||
assert texts, "the fixture bundle must exist for this control to mean anything"
|
||||
assert any("ENERGI-TOTAL-EL" in text for text in texts) # the scan CAN find a known string
|
||||
assert not any(_FEEDBACK_SENTINEL in text for text in texts)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------------------------
|
||||
# Group A (Step 3) — the reviewer INSIDE the attempt loop. Driven at ``generate_via_llm``.
|
||||
# ---------------------------------------------------------------------------------------------
|
||||
|
||||
# FV42-GSV-E1 cost codes 05.2 + 03.1 -> affected total 1_482_500, degenerate Monte Carlo
|
||||
# P90 = 0.30 x 1_482_500 = 444_750 (the ``test_step5_refine_loadbearing`` fixture, reused).
|
||||
_CODES = ["05.2", "03.1"]
|
||||
_BAD_CLAIM = 800_000 # parseable, but above P90 -> the DETERMINISTIC validator rejects it
|
||||
_FIRST_CLAIM = 200_000 # validates
|
||||
_REVISED_CLAIM = 150_000 # validates too, and differs -> "the answer was USED" is observable
|
||||
|
||||
|
||||
def _meter(*, rounds: int = 20) -> TokenMeter:
|
||||
return TokenMeter(Budget(max_tokens=10**9, max_rounds=rounds))
|
||||
|
||||
|
||||
class _FeedbackAwareChatClient(FakeChatClient):
|
||||
"""A proposer whose reply depends on the PROMPT: once the expert's sentinel arrives through
|
||||
``prior_feedback`` it answers with the revised amount, otherwise with the first one.
|
||||
|
||||
The ``_ReasonAwareChatClient`` form from Step 5's gate, keyed on the human's words instead of
|
||||
the machine's. It is what makes "the answer was used" observable: a door that renders the
|
||||
candidate, reads the line and discards it leaves the flip key out of attempt 2's prompt, so
|
||||
the outcome equals the always-approve control's and the arm goes RED.
|
||||
"""
|
||||
|
||||
def __init__(self, flip_key: str, first_reply: str, revised_reply: str) -> None:
|
||||
super().__init__()
|
||||
self._flip_key = flip_key
|
||||
self._first = first_reply
|
||||
self._revised = revised_reply
|
||||
|
||||
def _inner_get_response(
|
||||
self, *, messages: Sequence[Message], stream: bool, options: Any, **kwargs: Any
|
||||
) -> Any:
|
||||
received = message_texts(messages)
|
||||
self.received_texts.append(received)
|
||||
self.call_count += 1
|
||||
reply = self._revised if self._flip_key in " ".join(received) else self._first
|
||||
|
||||
if stream:
|
||||
|
||||
async def _agen() -> Any:
|
||||
yield ChatResponseUpdate(
|
||||
role="assistant", contents=[{"type": "text", "text": reply}]
|
||||
)
|
||||
|
||||
return self._build_response_stream(_agen())
|
||||
|
||||
async def _coro() -> ChatResponse:
|
||||
return ChatResponse(
|
||||
messages=[Message(role="assistant", contents=[reply])], response_id="fake"
|
||||
)
|
||||
|
||||
return _coro()
|
||||
|
||||
|
||||
class _RecordingReviewer:
|
||||
"""One reviewer double: scripted decisions, every request recorded. The script's LAST entry
|
||||
repeats, so an always-approve or always-revise policy is a one-element script."""
|
||||
|
||||
def __init__(self, script: Sequence[pr.ProposalReviewDecision]) -> None:
|
||||
self._script = list(script)
|
||||
self.calls: list[pr.ProposalReviewRequest] = []
|
||||
|
||||
def __call__(self, request: pr.ProposalReviewRequest) -> pr.ProposalReviewDecision:
|
||||
self.calls.append(request)
|
||||
return self._script[min(len(self.calls) - 1, len(self._script) - 1)]
|
||||
|
||||
|
||||
def _replies(project) -> tuple[str, str, str]:
|
||||
"""(first, revised, bad) as the model would emit them — built through ``proposal_for`` so the
|
||||
SUT parses exactly what the test built."""
|
||||
return (
|
||||
proposal_for(project, _CODES, claimed_saving_nok=_FIRST_CLAIM).model_dump_json(),
|
||||
proposal_for(project, _CODES, claimed_saving_nok=_REVISED_CLAIM).model_dump_json(),
|
||||
proposal_for(project, _CODES, claimed_saving_nok=_BAD_CLAIM).model_dump_json(),
|
||||
)
|
||||
|
||||
|
||||
async def test_t1_a_revise_reaches_the_next_prompt_verbatim_and_changes_the_outcome(
|
||||
project,
|
||||
) -> None:
|
||||
"""T1 — THE discriminator. Detach points: the reviewer call (M1), ignoring the decision (M2),
|
||||
dropping the ``prior_feedback`` injection (M3), truncating the feedback into the sink (M26).
|
||||
|
||||
A door that shows the candidate, reads the answer and discards it passes "the expert was
|
||||
asked". The gate is the second half: attempt 2's prompt must carry the expert's words
|
||||
VERBATIM, and the outcome must DIFFER from the always-approve control's."""
|
||||
first, revised, _ = _replies(project)
|
||||
feedback = f"Bruk {_REVISED_CLAIM}, ikke {_FIRST_CLAIM}. {_FEEDBACK_SENTINEL}"
|
||||
|
||||
client = _FeedbackAwareChatClient(_FEEDBACK_SENTINEL, first, revised)
|
||||
reviewer = _RecordingReviewer(
|
||||
[pr.ProposalReviewDecision.revise(feedback), pr.ProposalReviewDecision.approve()]
|
||||
)
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client, project, "ctx", _meter(), reviewer=reviewer, reviews=reviews
|
||||
)
|
||||
|
||||
assert client.call_count == 2
|
||||
assert _FEEDBACK_SENTINEL not in " ".join(client.received_texts[0])
|
||||
assert feedback in " ".join(client.received_texts[1]) # VERBATIM, not a paraphrase
|
||||
assert isinstance(result.outcome, ValidatedProposal)
|
||||
assert result.outcome.proposal.claimed_saving_nok == _REVISED_CLAIM
|
||||
assert [(r.decision, r.feedback, r.honoured) for r in reviews] == [
|
||||
("revise", feedback, True),
|
||||
("approve", "", True),
|
||||
]
|
||||
|
||||
# CONTROL: the same fixture with an always-approve reviewer stops at one call and keeps the
|
||||
# original amount — so T1's flip cannot be an artefact of the client.
|
||||
control_client = _FeedbackAwareChatClient(_FEEDBACK_SENTINEL, first, revised)
|
||||
control_reviews: list[pr.ProposalReview] = []
|
||||
control = await generate_via_llm(
|
||||
control_client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(),
|
||||
reviewer=_RecordingReviewer([pr.ProposalReviewDecision.approve()]),
|
||||
reviews=control_reviews,
|
||||
)
|
||||
assert control_client.call_count == 1
|
||||
assert isinstance(control.outcome, ValidatedProposal)
|
||||
assert control.outcome.proposal.claimed_saving_nok == _FIRST_CLAIM
|
||||
assert [r.decision for r in control_reviews] == ["approve"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("max_attempts", [2, 3])
|
||||
async def test_t2_always_revise_stops_at_the_attempt_budget(project, max_attempts: int) -> None:
|
||||
"""T2 — the loop stays BOUNDED: a revise consumes one of the attempts the loop already has,
|
||||
it does not open a new one (M8). Parametrised because a fixed expected count cannot tell a
|
||||
bound from a coincidence — the counter has to MOVE with the budget.
|
||||
|
||||
The final record is ``honoured=False``: the last attempt has nothing left to buy."""
|
||||
first, revised, _ = _replies(project)
|
||||
client = _FeedbackAwareChatClient("never-appears", first, revised)
|
||||
reviewer = _RecordingReviewer([pr.ProposalReviewDecision.revise("again")])
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(),
|
||||
max_attempts=max_attempts,
|
||||
reviewer=reviewer,
|
||||
reviews=reviews,
|
||||
)
|
||||
|
||||
assert client.call_count == max_attempts
|
||||
assert len(reviews) == max_attempts
|
||||
assert [r.honoured for r in reviews] == [True] * (max_attempts - 1) + [False]
|
||||
assert isinstance(result.outcome, ValidatedProposal)
|
||||
|
||||
|
||||
async def test_t2b_validated_then_revise_on_every_attempt_never_hits_an_assert(project) -> None:
|
||||
"""The loop's EXIT CONTRACT. Detach point: reverting to ``assert last is not None`` (M29).
|
||||
|
||||
Before MAJOR-2 the exit rested on ``last``, which only a validator REJECTION sets — so
|
||||
"validated -> revise" on every attempt would reach the end of the loop with ``last is None``
|
||||
and die on the assert with a traceback (and, under ``-O``, on ``None.proposal``). The carrier
|
||||
is explicit now, and the last ruling is what the run gets."""
|
||||
first, revised, _ = _replies(project)
|
||||
client = _FeedbackAwareChatClient("never-appears", first, revised)
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(),
|
||||
max_attempts=3,
|
||||
reviewer=_RecordingReviewer([pr.ProposalReviewDecision.revise("again")]),
|
||||
reviews=reviews,
|
||||
)
|
||||
assert isinstance(result.outcome, ValidatedProposal)
|
||||
assert result.outcome.proposal.claimed_saving_nok == _FIRST_CLAIM
|
||||
assert result.refinements == () # no validator rejection was ever fed back
|
||||
|
||||
|
||||
async def test_t3a_the_round_ledger_and_not_max_attempts_stops_the_revisions(project) -> None:
|
||||
"""T3a — a revise costs a ROUND, and the shared ledger is often what binds. Detach point:
|
||||
``attempts_remaining`` computed from ``max_attempts`` alone (M38).
|
||||
|
||||
With ``max_rounds=2`` and ``max_attempts=10`` the loop must stop after two fetches and return
|
||||
the last validated ruling. Under an attempt-only ``remaining`` the second attempt would buy a
|
||||
third and the meter would raise ``BudgetExceeded(rounds, 2, 3)`` — so "no exception, exactly
|
||||
two calls" is the discriminator, not a mere count."""
|
||||
first, revised, _ = _replies(project)
|
||||
client = _FeedbackAwareChatClient("never-appears", first, revised)
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(rounds=2),
|
||||
max_attempts=10,
|
||||
reviewer=_RecordingReviewer([pr.ProposalReviewDecision.revise("again")]),
|
||||
reviews=reviews,
|
||||
)
|
||||
assert client.call_count == 2
|
||||
assert [r.honoured for r in reviews] == [True, False]
|
||||
assert isinstance(result.outcome, ValidatedProposal)
|
||||
|
||||
|
||||
async def test_t3b_a_revise_whose_follow_up_never_fetched_is_not_honoured(project) -> None:
|
||||
"""T3b — ``honoured`` means **the attempt this revise bought actually FETCHED a reply**, not
|
||||
"was bought". Detach point: setting ``honoured=True`` at revise time (M40).
|
||||
|
||||
The bought attempt's reply does not parse, so the inner parse-retry ticks the ledger again and
|
||||
``BudgetExceeded`` fires before ``_fetch_parsed`` ever returns. The expert asked for a change
|
||||
the run never made, and the artefact must be able to say so — beside ``rounds limit=2
|
||||
observed=3`` (``observed != limit``, the kø-(y) rule)."""
|
||||
first, revised, _ = _replies(project)
|
||||
client = FakeChatClient(scripted=[first, "not json at all"], default_reply="still not json")
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
failures: list[ParseFailure] = []
|
||||
with pytest.raises(BudgetExceeded) as excinfo:
|
||||
await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(rounds=2),
|
||||
max_attempts=10,
|
||||
parse_failures=failures,
|
||||
reviewer=_RecordingReviewer([pr.ProposalReviewDecision.revise(_FEEDBACK_SENTINEL)]),
|
||||
reviews=reviews,
|
||||
)
|
||||
assert (excinfo.value.kind, excinfo.value.limit, excinfo.value.observed) == ("rounds", 2, 3)
|
||||
assert [(r.decision, r.feedback, r.honoured) for r in reviews] == [
|
||||
("revise", _FEEDBACK_SENTINEL, False)
|
||||
]
|
||||
|
||||
|
||||
async def test_t4_a_validator_rejected_attempt_is_never_shown_to_the_expert(project) -> None:
|
||||
"""T4 — only a ``ValidatedProposal`` reaches the reviewer. A rejection is already fed back
|
||||
INFORMED (Step 5); asking a human to comment on numbers the machine just refuted spends the
|
||||
human on the machine's job. Detach point: calling the reviewer before ``validate_proposal``
|
||||
(M6)."""
|
||||
_, _, bad = _replies(project)
|
||||
client = FakeChatClient(scripted=[bad], default_reply=bad)
|
||||
reviewer = _RecordingReviewer([pr.ProposalReviewDecision.revise("x")])
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client, project, "ctx", _meter(), max_attempts=1, reviewer=reviewer, reviews=reviews
|
||||
)
|
||||
assert reviewer.calls == []
|
||||
assert reviews == []
|
||||
assert isinstance(result.outcome, Rejection)
|
||||
assert result.refinements == ()
|
||||
|
||||
|
||||
async def test_t5_the_request_carries_this_attempts_candidate_and_its_context(project) -> None:
|
||||
"""T5 — what the expert is shown. Detach points: a constant ``attempts_remaining`` (M38), a
|
||||
dropped approach key (M10), a dropped checker verdict.
|
||||
|
||||
``attempts_remaining`` is the LEDGER-aware number: with ``max_rounds=1`` the ledger has no
|
||||
headroom after attempt 1, so a revise can buy nothing even though ``max_attempts=3`` — an
|
||||
attempt-only computation would show ``2`` and read false at the terminal."""
|
||||
first, revised, _ = _replies(project)
|
||||
client = _FeedbackAwareChatClient("never-appears", first, revised)
|
||||
reviewer = _RecordingReviewer([pr.ProposalReviewDecision.approve()])
|
||||
await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(rounds=1),
|
||||
max_attempts=3,
|
||||
reviewer=reviewer,
|
||||
reviews=[],
|
||||
review_key=("a2", "Night setback"),
|
||||
checker_verdict="approve",
|
||||
)
|
||||
(request,) = reviewer.calls
|
||||
assert request.project_id == project.id
|
||||
assert (request.approach_id, request.approach_label) == ("a2", "Night setback")
|
||||
assert request.attempt == 0
|
||||
assert request.attempts_remaining == 0 # the LEDGER binds, not max_attempts
|
||||
assert request.checker_verdict == "approve"
|
||||
assert isinstance(request.proposal, ValidatedProposal)
|
||||
assert request.proposal.proposal.claimed_saving_nok == _FIRST_CLAIM
|
||||
|
||||
|
||||
async def test_t6_feedback_is_sticky_while_the_machines_reason_is_per_attempt(project) -> None:
|
||||
"""T6 — composition. Detach points: clearing the feedback after one attempt (M4), or
|
||||
accumulating rejections (M5).
|
||||
|
||||
A1 validates -> the expert asks for a revision. A2 is REJECTED by the validator (and the
|
||||
expert is NOT asked about it, T4). A3's prompt must carry BOTH the expert's standing
|
||||
instruction and A2's fresh reason — the human's words stand until the human next answers,
|
||||
the machine's reason is only the most recent."""
|
||||
first, revised, bad = _replies(project)
|
||||
client = FakeChatClient(scripted=[first, bad, revised], default_reply=revised)
|
||||
reviewer = _RecordingReviewer(
|
||||
[pr.ProposalReviewDecision.revise(_FEEDBACK_SENTINEL), pr.ProposalReviewDecision.approve()]
|
||||
)
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client, project, "ctx", _meter(), max_attempts=3, reviewer=reviewer, reviews=reviews
|
||||
)
|
||||
|
||||
reason = f"{_BAD_CLAIM}"
|
||||
third = " ".join(client.received_texts[2])
|
||||
assert _FEEDBACK_SENTINEL in " ".join(client.received_texts[1])
|
||||
assert third.count(_FEEDBACK_SENTINEL) == 1
|
||||
assert "REJECTED by the deterministic validator" in third
|
||||
assert third.count(reason) == 1
|
||||
assert len(reviewer.calls) == 2 # A2 was never shown
|
||||
assert isinstance(result.outcome, ValidatedProposal)
|
||||
|
||||
|
||||
async def test_t7_a_revise_with_nothing_left_to_buy_leaves_the_validated_ruling_standing(
|
||||
project,
|
||||
) -> None:
|
||||
"""T7 / criterion 12 — an un-honoured revise buys nothing, so the last ruling (the validated
|
||||
one) stands. Detach point: ``honoured`` constant ``True`` (M9).
|
||||
|
||||
The record must be able to STATE the fact rather than leave it to be inferred: the expert
|
||||
asked for a change the run could not make."""
|
||||
first, revised, _ = _replies(project)
|
||||
client = _FeedbackAwareChatClient(_FEEDBACK_SENTINEL, first, revised)
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(),
|
||||
max_attempts=1,
|
||||
reviewer=_RecordingReviewer([pr.ProposalReviewDecision.revise(_FEEDBACK_SENTINEL)]),
|
||||
reviews=reviews,
|
||||
)
|
||||
assert client.call_count == 1 # nothing further was generated
|
||||
assert isinstance(result.outcome, ValidatedProposal)
|
||||
assert result.outcome.proposal.claimed_saving_nok == _FIRST_CLAIM
|
||||
assert [(r.decision, r.honoured) for r in reviews] == [("revise", False)]
|
||||
|
||||
|
||||
async def test_t13_an_honoured_revise_rejected_next_leaves_the_validators_last_ruling(
|
||||
project,
|
||||
) -> None:
|
||||
"""Criterion 13 / D6 — the validator's LAST ruling wins, and the reviewed candidate is not
|
||||
lost. Detach points: falling back to the earlier validated proposal (M27), dropping the
|
||||
reviewed proposal from the record (M28).
|
||||
|
||||
Falling back would hand the run the very candidate the expert asked to change — a silent
|
||||
override of a human decision — and would stamp ``validator_decision`` for a candidate that
|
||||
was not the last one ruled on."""
|
||||
first, _, bad = _replies(project)
|
||||
client = FakeChatClient(scripted=[first, bad], default_reply=bad)
|
||||
reviews: list[pr.ProposalReview] = []
|
||||
result = await generate_via_llm(
|
||||
client,
|
||||
project,
|
||||
"ctx",
|
||||
_meter(),
|
||||
max_attempts=2,
|
||||
reviewer=_RecordingReviewer([pr.ProposalReviewDecision.revise(_FEEDBACK_SENTINEL)]),
|
||||
reviews=reviews,
|
||||
)
|
||||
assert isinstance(result.outcome, Rejection)
|
||||
assert result.outcome.proposal.claimed_saving_nok == _BAD_CLAIM
|
||||
(record,) = reviews
|
||||
assert (record.decision, record.honoured) == ("revise", True)
|
||||
# The candidate the expert LOOKED at survives in the record, keyed and priced.
|
||||
assert record.proposal.proposal.claimed_saving_nok == _FIRST_CLAIM
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue