fix(hosting): _run_kwargs reads _REFUSED_BY_NAME instead of a hardcoded literal
MAJOR-2 review, fokuspunkt 4: `_REFUSED_BY_NAME = ("proposal_review",)` was
documented as the tuple the door reads, but the `if "proposal_review" in
payload` refusal never consulted it — two copies of one fact, free to drift.
Måling FØR bygging: `grep -rn 'no terminal to answer' tests/` gave 0 treff,
men `grep -rn proposal_review tests/ | grep -i hosting` fant
`test_proposal_review_loop_loadbearing.py:1677` (T21) — so half of the fact
(refusing `proposal_review` itself, with the exact CLI-pointing message) WAS
already pinned. The unpinned half was the second name: any OTHER entry added
to `_REFUSED_BY_NAME` fell through to the generic `unknown field(s)` check
instead of being refused by name before it.
Fix: `_run_kwargs` now iterates `_REFUSED_BY_NAME`; `proposal_review` keeps
its verbatim message, any other name gets a message naming the field.
New test `test_the_named_refusal_reads_the_constant_not_a_literal`
(tests/test_hosting_loadbearing.py) is unit-level against `_run_kwargs`
directly: confirms an unlisted name is refused generically (control), then
monkeypatches `_REFUSED_BY_NAME` wider and confirms the SAME payload is now
refused by name, before the generic check.
Mutation check (done and reverted): setting `_REFUSED_BY_NAME = ()`
temporarily turns the EXISTING T21 test red — proving the constant is now
load-bearing. Verified against pre-fix code that the same mutation left T21
green (the old literal-based `if` never consulted the constant at all), so
the fix closes the exact drift the review flagged.
Suite: 1368 passed / 5 skipped (was 1367/5 on 17998af), 0 regressions.
ruff + mypy clean. Golden demo-transcript.stdout byte-unchanged
(shasum -a 1 = ea8c534773acdbe41ae68f2c55724d69aaf8be4f).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
17998af8b7
commit
a790ce3ae1
2 changed files with 40 additions and 7 deletions
|
|
@ -141,13 +141,16 @@ def _run_kwargs(payload: Any) -> tuple[str, dict[str, Any], dict[str, Any]]:
|
|||
raise InvocationRefused("body must be a JSON object")
|
||||
# AFTER the object guard (a non-object body must keep its 400 rather than become a 500) and
|
||||
# BEFORE the generic unknown-field check, which would otherwise answer this one first.
|
||||
if "proposal_review" in payload:
|
||||
raise InvocationRefused(
|
||||
"proposal_review: this surface has no terminal to answer a proposal review — the "
|
||||
"synchronous door 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 "
|
||||
"--proposal-review"
|
||||
)
|
||||
for name in _REFUSED_BY_NAME:
|
||||
if name in payload:
|
||||
if name == "proposal_review":
|
||||
raise InvocationRefused(
|
||||
"proposal_review: this surface has no terminal to answer a proposal review "
|
||||
"— the synchronous door 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 --proposal-review"
|
||||
)
|
||||
raise InvocationRefused(f"{name}: this field is refused by name on this surface")
|
||||
unknown = sorted(set(payload) - _ALLOWED_FIELDS)
|
||||
if unknown:
|
||||
raise InvocationRefused(f"unknown field(s): {', '.join(unknown)}")
|
||||
|
|
|
|||
|
|
@ -274,6 +274,36 @@ async def test_unknown_field_is_refused_never_repaired(
|
|||
assert len(recorder.calls) == 1
|
||||
|
||||
|
||||
def test_the_named_refusal_reads_the_constant_not_a_literal(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""MAJOR-2 review, fokuspunkt 4: ``_run_kwargs`` carried the ``proposal_review`` fact TWICE —
|
||||
once as ``_REFUSED_BY_NAME`` (documented as the tuple the door reads) and once as a literal
|
||||
string in the ``if`` that actually refuses. Nothing wired the two: adding or removing a name
|
||||
in the tuple changed nothing about which payload keys get refused by name.
|
||||
|
||||
``proposal_review`` itself was already pinned (T21, ``test_proposal_review_loop_loadbearing.py``),
|
||||
so that half is NOT what this test proves. The gap is the SECOND name: a ``_REFUSED_BY_NAME``
|
||||
entry other than ``proposal_review`` must ALSO be refused, by the same code path, before the
|
||||
generic ``unknown field(s)`` check — and today it silently falls through to that generic
|
||||
check instead, because the ``if`` only ever tests the one literal."""
|
||||
payload = {**_PAYLOAD, "other_door": True}
|
||||
|
||||
# Control: today's constant does not name "other_door", so it is refused generically.
|
||||
with pytest.raises(hosting.InvocationRefused) as generic:
|
||||
hosting._run_kwargs(payload)
|
||||
assert "unknown field(s)" in str(generic.value)
|
||||
assert "other_door" in str(generic.value)
|
||||
|
||||
# The door must READ the constant: widen it, and the SAME payload must now be refused BY NAME
|
||||
# — before the generic check, with a message naming the field (never "unknown field(s)").
|
||||
monkeypatch.setattr(hosting, "_REFUSED_BY_NAME", ("proposal_review", "other_door"))
|
||||
with pytest.raises(hosting.InvocationRefused) as named:
|
||||
hosting._run_kwargs(payload)
|
||||
assert "other_door" in str(named.value)
|
||||
assert "unknown field(s)" not in str(named.value)
|
||||
|
||||
|
||||
# ``verdict_input`` is deliberately ABSENT from this list since F2: it is optional, and the arms
|
||||
# proving the relaxation live in ``tests/test_ungiven_verdict_loadbearing.py``.
|
||||
@pytest.mark.parametrize("missing", ["project_id", "docs_dir"])
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue