Six of the seven merge-gating findings from the adversarial review - #11
Open
LucasWaki wants to merge 6 commits into
Open
Six of the seven merge-gating findings from the adversarial review#11LucasWaki wants to merge 6 commits into
LucasWaki wants to merge 6 commits into
Conversation
`_write_kb` wrote the address `public_front_door_url` returns, and that address now ends in `STUDIO_FRONT_DOOR_SECRET`. The file it writes, `config/products/<product>.monarch-kb.yaml`, is tracked and this repository is public, so the next `wb monarch setup` against a hosted front door would have put the secret in history, where deleting the file does not remove it. Nothing is leaked today: the four tracked files stop at `/front-door`. The redaction is in the writer rather than at its one call site, because the writer is the thing that must never write a secret regardless of who calls it, and `env` is a required keyword for the same reason -- a caller that forgot it should fail, not leak quietly. Free either way: `RunConfig.hash` is the only consumer of this field and never uses it as an address, so recording the name loses nothing, and rotating the secret per round -- which `front_door_secret`'s own docstring tells an operator to do -- stops reading as a different configuration. Refs #10. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e wire `_call` used two forms of the same path in one function. The admin guard read the raw path, everything after it read the decoded one, and the decoded one was then sent as the request target. `/locations/name/TechCorp%20NYC%20Headquarters` is a route and a value the shipped ITSM document and its seed data both contain. Decoded and sent, it puts a space in the request line: urllib raises `InvalidURL: URL can't contain control characters`, and a server that accepted it would read the target as `/locations/name/TechCorp`. Either way the call fails, and the approval rule reads a failed call as the competitor failing rather than as our own bug. Decoding now happens once, before every check, and the target is re-encoded from the decoded route. Two consequences worth naming: an encoded separator can no longer read as one path segment to the published-surface check, and an encoded administrative path is now refused as administrative rather than landing on the published-surface refusal by luck. Three tests: the space reaches the world still encoded, `%2F` is refused without reaching it, and `/api/%73ql-runner` is refused as administrative.
`run_attempt` raised the simulated customer's sticky failure whenever the arm returned with one pending. That guard is right in general -- a harness tool wrapper can turn the customer's exception into an ordinary tool response, and an infrastructure break must never be scored as the agent's own outcome -- but it had no exception for the case where the two are the same event. The customer is cut at the episode deadline and records `infra:timeout`. The arm is cut at that same deadline and records `timeout`, a scored verdict by deliberate rule (`wb_arms/api_loop.py:670-673`: "that's a timeout verdict, not an infra retry"). Raising the customer's copy overwrote it, so the attempt left the pass denominator instead of counting as a failure -- and tau2 pass rates came out inflated by exactly the attempts that ran out of time, whenever the agent's last turn happened to call the customer. Narrowed to that pair: the arm's verdict must be `timeout` and the customer's failure must be `infra:timeout`. A customer that broke for any other reason still wins, which is the second test.
…ishing An external round that ran out of admission envelope recorded `infra:budget` on every remaining attempt and carried on to the end. `_stop_reason` was set only for `infra:weekly_budget`, and the `cost_ceiling` branch could not cover it either -- that compares settled spend while the envelope holds maximums, and an unsettled "billing=unknown" hold occupies its whole maximum. The round finished, `wb run` exited 0, and the operator was never told to raise `cost_ceiling_usd` and resume. What the round reported was not wrong: `is_infrastructure` matches any `infra:` prefix, so the cut attempts stayed out of the pass denominator and `caveats.for_run` said how many stopped. The defect is that a truncated round read as a complete one, so nobody resumed it. The obvious one-line fix -- branch on `infra:budget` too -- would have been worse than the bug, and finding out why changed the shape of this commit. That kind is also the correct, tested outcome of an attempt exhausting its own cap, so stopping on it would kill a round the first time any single attempt hit its per-attempt cap. The root cause is a conflation, and it is not in the message. An attempt's cap is itself a run reservation, so the ledger says `run budget exhausted` for both the round's envelope and one attempt's -- word for word. Matching on the text would have reclassified every capped attempt as a round-wide stop; the failing test that already pinned that behaviour is what showed it. So the scope discriminates. `BudgetExceeded` now carries the scope that ran out, `ROUND_ENVELOPE_MARKER` names the convention `_admit` already used by hand, and `_budget_failure` returns three kinds. `STOPS_THE_ROUND` maps the two round-wide ones to their stop reasons and states in one place why `infra:budget` is absent. No end-to-end test: an envelope refusal needs a paid competitor and a containerized world, neither free offline. Tested at the seams instead -- the ledger raises identical wording for both scopes, the classifier separates them, and the mapping excludes the attempt cap. The stop branch mirrors the weekly one above it. Refs #9.
The round's admission envelope was closed by `_execute`'s finally alone, so anything raising between `_admit` and `_execute` leaked the whole round's liability against the US$ 300 week with nothing spent. A duplicate `--run-id` is enough: `_admit` reserves, `create_run` fails the unique constraint, `_execute` is never entered. `resume` had the same gap around `set_stop_reason`. Nothing can recover one afterwards. `wb budget release` reads `budget_reservations` and not `budget_run_reservations`, the scope never appears in `status.unknown_ids`, and `ledger_lines` drops envelopes from other weeks -- so once the week rolled the capacity kept counting in `carried_held_usd` with no page naming it. `_close_envelope` is now the one way out, idempotent, wrapped around everything after admission in both `run` and `resume`. `_execute` keeps its own finally so its behaviour is unchanged for anything that enters it. Verified as a guard, not just green: with the try/finally removed the new test fails with "the round envelope leaked", and passes with it. Still open, and not this commit: a hard process kill cannot be caught by a finally, and `_admit` allocates a fresh `#admission-NNN` on every resume, so a killed round still stacks envelopes. That needs a release path for run reservations, which is a command rather than a fix. Refs #9.
…fatal `_action` raises on any GET/HEAD/DELETE that declares a request body, and `generate` had no per-operation exclusion, so one such operation aborted the whole product's catalogue: `Stop(2, "generate", ...)` and no seed file written. The real EnterpriseOps-Gym ITSM document has four of them, so a task set naming any one could not set Monarch up on that product at all. Being unable to express one operation is not a reason to teach Monarch nothing. It is now excluded, named per line by `wb monarch setup`, and recorded in the pack's own manifest -- what a competitor was not taught belongs in the evidence beside what it was. The excluded operation is also dropped from the published document, which it was not before: the document offered an operation that had no action behind it. Two things kept deliberately narrow: - A dedicated `Unrepresentable` rather than catching `ValueError`. Excluding on any ValueError would silently drop operations for unrelated structural faults; a malformed document still refuses the whole catalogue, which the third test pins. - `excluded_operations` appears in the manifest only when there is something to report. `generate` refuses to replace a frozen catalogue file, so an unconditional new key would break setup for anyone holding a pack frozen before this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six fixes against the review filed as #9
and #10, one commit each, each with
its own test. 3195 tests pass, including
test_monarch_arm.pyandtest_monarch_recipes.pyrun in their own processes.9939c4bwb monarch setupwould have written the front door's secret into a tracked file in this public repository. Latent; nothing leaked.b5bfb56892f7461517ccfbb6bac4d2f2edfwb monarch setupcould not run on the full EOG surface.Two worth reading the commit message for
1517ccfis where the obvious fix was worse than the bug. Branching oninfra:budgetwould have killed a whole round the first time any single attempt hitits own per-attempt cap. The root cause is not the missing branch and not the message
either: an attempt's cap is itself a run reservation, so the ledger says
run budget exhaustedfor both cases word for word. The scope discriminates.d2f2edfuses a dedicated exception rather than catchingValueError, so amalformed document still refuses the whole catalogue instead of quietly losing
operations, and adds its manifest field only when there is something to report, so
packs frozen before this change still regenerate identically.
Not in this change
The seventh gating finding is a decision rather than a defect. The catalogue unions
every frozen task's published operations into one knowledge base, so on
tasks/eog-itsm-smoke-2Monarch is taught 8 operations while the paired raw-modelcompetitor's
api_searchsees that task's 4 or 6. Two competitors given differentsurfaces is a
PLAN.md§1.1 problem, and every way to close it costs something:per-task knowledge, or refusing a task set whose members publish different surfaces,
or scoring the resulting 403 as a setup error rather than a competitor failure.
The other 24 findings are in
monarch-benchmark/docs/rounds/2026-09-11-adversarial-review.md, which is committedon
genesis-loopand deliberately not pushed yet.