fix(run): classify portfolio-mode flags by allowlist so a new flag fails closed
The portfolio entrance refused unsupported flags from a hard-coded BLOCKLIST: --inbox, --out, --outbox, --run-id, --value-report, --live-dry-run. That construction fails OPEN. A flag added to the parser later and forgotten in the list is accepted, does nothing, and says nothing — the operator's flag is a claim the run does not back (§1). MAF's report mode already used an allowlist; the divergence was raised as an open question and the operator decided it this session in favour of fail-closed. unsupported_flags_given() now reports every flag GIVEN that the allowlist does not name. "Given" is measured against the parser's own default, so it needs no knowledge of which flags exist — that is what keeps it correct for flags added after it was written, including store_true switches. Load-bearing (§11), detach-proven twice (before and after ruff format, restored from a copy): swapping the membership test back to a hard-coded refusal list turns test_a_flag_nobody_classified_is_refused RED, while every CLI-level refusal test stays green — they only exercise flags a blocklist already names, so they do not cover this seam. The other direction is covered too: a run passing all fourteen honoured flags still exits 0, and the allowlist entries are checked against the CLI's own --help so a rename cannot leave a dead entry. 612 -> 624 passed, ruff + mypy --strict clean. README states the allowlist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQu2xxwedckjU56byu1aUG
This commit is contained in:
parent
2c1317bdb5
commit
fc4a536e09
3 changed files with 194 additions and 18 deletions
|
|
@ -133,7 +133,9 @@ description, never from its code)
|
|||
nothing** — it returns typed results and prints one line per project, because the outbox
|
||||
names its pairs by `run_id` and a portfolio pass has none of its own. Rather than accept
|
||||
`--outbox`/`--out`/`--value-report` and quietly ignore them, the entrance refuses them
|
||||
there and points at the per-project `--bundle` runs (§1).
|
||||
there and points at the per-project `--bundle` runs (§1). The refusal is an **allowlist**:
|
||||
it names the flags the portfolio pass acts on, so a flag added later and classified nowhere
|
||||
is refused rather than silently ignored — fail-closed, not fail-quiet.
|
||||
- `run_s10.py` — the programme's ONE live run (cost discipline D6); run-path only.
|
||||
- `costsim.py` — pre-run cost simulation (**offline** — the one Run-layer module that never
|
||||
touches the network): a deterministic UPPER-BOUND USD estimate for a (portfolio-)run
|
||||
|
|
|
|||
|
|
@ -531,6 +531,51 @@ def execute_dry_run(
|
|||
return 0
|
||||
|
||||
|
||||
# The dest names the PORTFOLIO pass honours — an ALLOWLIST, deliberately.
|
||||
# Classification is required: a flag added to the parser later and named here
|
||||
# nowhere is REFUSED in portfolio mode rather than accepted and then ignored.
|
||||
# The blocklist this replaces failed OPEN — a forgotten flag no-opped in
|
||||
# silence, which is a claim the run does not back (§1).
|
||||
_PORTFOLIO_SUPPORTED_DESTS: frozenset[str] = frozenset(
|
||||
{
|
||||
"portfolio", # the mode itself
|
||||
"verdict_dir", # portfolio-level expert inbox, read before each fold (K3)
|
||||
"ledger", # read by the pre-spend goal check
|
||||
"goals", # ditto — both are consulted BEFORE the portfolio branch
|
||||
"max_rounds", # §8 caps, shared by every project under one meter
|
||||
"max_tokens",
|
||||
"max_budget_usd_per_call",
|
||||
"max_debate_rounds",
|
||||
"max_attempts",
|
||||
"top_k",
|
||||
"notify_console", # K10 sinks: the portfolio pass emits events too
|
||||
"notify_file",
|
||||
"notify_webhook",
|
||||
"allow_webhook_egress",
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
def unsupported_flags_given(
|
||||
parser: argparse.ArgumentParser,
|
||||
args: argparse.Namespace,
|
||||
*,
|
||||
supported: frozenset[str],
|
||||
) -> list[str]:
|
||||
"""Flags the operator actually GAVE that ``supported`` does not name.
|
||||
|
||||
"Given" is measured against the parser's own default, so this needs no
|
||||
knowledge of which flags exist — which is the point: the answer stays
|
||||
correct for flags added after it was written. Returns the flag spellings,
|
||||
sorted, for a deterministic error message.
|
||||
"""
|
||||
return sorted(
|
||||
"--" + dest.replace("_", "-")
|
||||
for dest, value in vars(args).items()
|
||||
if dest not in supported and value != parser.get_default(dest)
|
||||
)
|
||||
|
||||
|
||||
def main(
|
||||
argv: list[str] | None = None,
|
||||
*,
|
||||
|
|
@ -625,24 +670,16 @@ def main(
|
|||
# the outbox names its pairs by run_id, which a portfolio pass has none of.
|
||||
# Every flag that would therefore do nothing is refused rather than
|
||||
# silently ignored (§1) — a flag that quietly no-ops is a false claim.
|
||||
unsupported = [
|
||||
name
|
||||
for name, value in (
|
||||
("--inbox", args.inbox),
|
||||
("--out", args.out),
|
||||
("--outbox", args.outbox),
|
||||
("--run-id", args.run_id),
|
||||
("--value-report", args.value_report),
|
||||
)
|
||||
if value is not None
|
||||
]
|
||||
if args.live_dry_run:
|
||||
unsupported.append("--live-dry-run")
|
||||
# ALLOWLIST (never a blocklist): the refusal covers flags nobody has
|
||||
# classified yet, so a new one fails closed instead of failing quiet.
|
||||
unsupported = unsupported_flags_given(parser, args, supported=_PORTFOLIO_SUPPORTED_DESTS)
|
||||
if unsupported:
|
||||
parser.error(
|
||||
f"--portfolio does not support {', '.join(sorted(unsupported))}: the "
|
||||
"portfolio pass persists nothing and has no run_id of its own. Run the "
|
||||
"projects individually with --bundle to file per-run artifacts."
|
||||
f"--portfolio does not support {', '.join(unsupported)}: the portfolio "
|
||||
"pass persists nothing and has no run_id of its own, and it honours only "
|
||||
"the flags it is known to act on — anything else is refused rather than "
|
||||
"silently ignored. Run the projects individually with --bundle to file "
|
||||
"per-run artifacts."
|
||||
)
|
||||
|
||||
# fail-fast (§10 spirit): a run persisted to the outbox MUST carry an explicit
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ moment the two drift apart.
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import contextlib
|
||||
import importlib
|
||||
import io
|
||||
|
|
@ -40,7 +41,11 @@ from portfolio_optimiser_claude.inbox import VerdictDocument, write_verdict
|
|||
from portfolio_optimiser_claude.ir import load_validator_input
|
||||
from portfolio_optimiser_claude.ledger import SavingsLedger
|
||||
from portfolio_optimiser_claude.loop import ModelClient, ModelReply
|
||||
from portfolio_optimiser_claude.run import main
|
||||
from portfolio_optimiser_claude.run import (
|
||||
_PORTFOLIO_SUPPORTED_DESTS,
|
||||
main,
|
||||
unsupported_flags_given,
|
||||
)
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[1]
|
||||
BUNDLE = REPO_ROOT / "shared" / "examples" / "bygg-energi-mikro"
|
||||
|
|
@ -361,6 +366,138 @@ class TestPortfolioOnTheEntrance:
|
|||
assert created == []
|
||||
|
||||
|
||||
class TestPortfolioClassifiesByAllowlist:
|
||||
"""LOAD-BEARING (§1, §11): portfolio mode fails CLOSED on an unclassified flag.
|
||||
|
||||
The refusal used to run off a BLOCKLIST — the flags to reject, named one by
|
||||
one. That construction fails OPEN: a flag added to the parser later and
|
||||
forgotten in the list is accepted and then silently ignored, which is
|
||||
exactly the false claim §1 forbids (the operator's flag did nothing and the
|
||||
CLI said nothing). The allowlist names the flags the portfolio pass HONOURS;
|
||||
everything else is refused whether or not anyone remembered it.
|
||||
|
||||
Detach point: replace the ``dest not in supported`` membership test with a
|
||||
hard-coded list of flags to refuse → ``test_a_flag_nobody_classified_is_refused``
|
||||
goes RED, because tomorrow's flag appears in no such list. The CLI-level
|
||||
refusal tests stay GREEN under that mutation (they only exercise flags a
|
||||
blocklist already names), so they do not cover this seam.
|
||||
"""
|
||||
|
||||
def _parser_with(self, *flags: str) -> argparse.ArgumentParser:
|
||||
parser = argparse.ArgumentParser()
|
||||
for flag in flags:
|
||||
parser.add_argument(flag, default=None)
|
||||
return parser
|
||||
|
||||
def test_a_flag_nobody_classified_is_refused(self) -> None:
|
||||
# "--tomorrows-flag" stands in for the next flag someone adds: it is in
|
||||
# no allowlist and in no blocklist. Fail-closed means it is reported.
|
||||
parser = self._parser_with("--portfolio", "--tomorrows-flag")
|
||||
args = parser.parse_args(["--portfolio", "p.json", "--tomorrows-flag", "x"])
|
||||
assert unsupported_flags_given(parser, args, supported=frozenset({"portfolio"})) == [
|
||||
"--tomorrows-flag"
|
||||
]
|
||||
|
||||
def test_a_supported_flag_is_not_reported(self) -> None:
|
||||
parser = self._parser_with("--portfolio", "--ledger")
|
||||
args = parser.parse_args(["--portfolio", "p.json", "--ledger", "l.json"])
|
||||
assert (
|
||||
unsupported_flags_given(parser, args, supported=frozenset({"portfolio", "ledger"}))
|
||||
== []
|
||||
)
|
||||
|
||||
def test_a_flag_left_at_its_default_was_never_given(self) -> None:
|
||||
# Absence is not a claim: an unsupported flag the operator did not pass
|
||||
# must not turn a valid portfolio run into an error.
|
||||
parser = self._parser_with("--portfolio", "--tomorrows-flag")
|
||||
args = parser.parse_args(["--portfolio", "p.json"])
|
||||
assert unsupported_flags_given(parser, args, supported=frozenset({"portfolio"})) == []
|
||||
|
||||
def test_a_store_true_flag_counts_as_given_only_when_passed(self) -> None:
|
||||
# store_true defaults to False, not None — "given" is measured against
|
||||
# the parser's OWN default, so both directions are correct.
|
||||
parser = argparse.ArgumentParser()
|
||||
parser.add_argument("--portfolio", default=None)
|
||||
parser.add_argument("--tomorrows-switch", action="store_true")
|
||||
supported = frozenset({"portfolio"})
|
||||
unset = parser.parse_args(["--portfolio", "p.json"])
|
||||
assert unsupported_flags_given(parser, unset, supported=supported) == []
|
||||
given = parser.parse_args(["--portfolio", "p.json", "--tomorrows-switch"])
|
||||
assert unsupported_flags_given(parser, given, supported=supported) == ["--tomorrows-switch"]
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"extra",
|
||||
[
|
||||
["--out", "out-dir"],
|
||||
["--inbox", "inbox-dir"],
|
||||
["--outbox", "outbox-dir"],
|
||||
["--run-id", "run-1"],
|
||||
["--value-report", "report.json"],
|
||||
["--live-dry-run"],
|
||||
],
|
||||
ids=["out", "inbox", "outbox", "run-id", "value-report", "live-dry-run"],
|
||||
)
|
||||
def test_the_real_cli_refuses_every_unsupported_flag(
|
||||
self, tmp_path: Path, extra: list[str]
|
||||
) -> None:
|
||||
# The wire: main() classifies with the real allowlist, before any client.
|
||||
factory, created = _scripted_factory()
|
||||
with pytest.raises(SystemExit):
|
||||
main(
|
||||
["--portfolio", str(_portfolio_file(tmp_path, ["prosjekt-a"]))] + extra,
|
||||
client_factory=factory,
|
||||
)
|
||||
assert created == []
|
||||
|
||||
def test_the_real_cli_accepts_every_flag_it_honours(self, tmp_path: Path) -> None:
|
||||
# The other half of fail-closed: the allowlist must not refuse the flags
|
||||
# the portfolio pass genuinely uses. A goal far above the book does not
|
||||
# stop the run, so this exercises the full pass.
|
||||
verdict_dir, _ = _portfolio_inbox(tmp_path)
|
||||
factory, created = _scripted_factory(runs=1)
|
||||
code = main(
|
||||
[
|
||||
"--portfolio",
|
||||
str(_portfolio_file(tmp_path, ["prosjekt-a"])),
|
||||
"--verdict-dir",
|
||||
str(verdict_dir),
|
||||
"--ledger",
|
||||
str(_ledger_file(tmp_path, amount_nok=1000.0)),
|
||||
"--goals",
|
||||
str(_goal_file(tmp_path, target_nok=10_000_000.0, mode="hard")),
|
||||
"--max-rounds",
|
||||
"12",
|
||||
"--max-tokens",
|
||||
"150000",
|
||||
"--max-budget-usd-per-call",
|
||||
"0.25",
|
||||
"--max-debate-rounds",
|
||||
"3",
|
||||
"--max-attempts",
|
||||
"3",
|
||||
"--top-k",
|
||||
"3",
|
||||
"--notify-console",
|
||||
"--notify-file",
|
||||
str(tmp_path / "notify"),
|
||||
],
|
||||
client_factory=factory,
|
||||
)
|
||||
assert code == 0
|
||||
assert created, "the portfolio pass must have run"
|
||||
|
||||
def test_the_allowlist_names_only_flags_the_cli_actually_has(self) -> None:
|
||||
# A renamed flag would leave a dead entry behind, and the renamed flag
|
||||
# would start being refused in portfolio mode without anyone saying so.
|
||||
help_text = _full_help("run")
|
||||
missing = sorted(
|
||||
f"--{dest.replace('_', '-')}"
|
||||
for dest in _PORTFOLIO_SUPPORTED_DESTS
|
||||
if f"--{dest.replace('_', '-')}" not in help_text
|
||||
)
|
||||
assert missing == []
|
||||
|
||||
|
||||
# --- the documentation-honesty seam (§1) -----------------------------------------------------
|
||||
|
||||
# Lines about third-party dev tooling are not claims about this framework's CLI.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue