test(flow): consult a SYNC flow target for real, and close criterion 5's read half (#112) - #960
Conversation
An audit of #112's six acceptance criteria found four unmet for ONE reason: with_flow_target_fetcher had a single caller in the tree, the boot path, and no test installed it. So consult_target had ZERO callers in the whole suite and dispatch_sync past its no-fetcher branch was dead code from the tests' point of view. The half of this feature that can approve or reject a signup IN BAND had no behavioural test at any of the four places coverage lives here. Five tests, each paired so its assertion is attributable to the thing it names: - a refusing PRE-PERSIST target leaves NO ROW, which is the observable half of criterion 4 (storing both timing values is only selectability), paired with a fail_open target that admits so the refusal is the policy talking; - a target that accepts and never answers ELAPSES its budget and triggers the policy, which is criterion 6, paired the same way. Outcome::Unavailable was never once produced BY a timeout anywhere in the suite before this -- only typed in as a literal in unit tests of apply_policy, which tests the mapping and not the timeout; - the consultation reaches the network, dialing the address destination validation approved rather than the loopback the dialer forwards to. Mutation-verified, four for four. Transport failure returning Outcome::Allow -- a fail-closed fraud gate silently admitting the signup it exists to stop -- CAUGHT. The no-fetcher branch treated as a skip, which the source comment predicted would "silently disarm every fail-closed target", CAUGHT. fail_closed mapped to Continue, caught twice. Deleting the pre-persist dispatch from the legacy door, caught three ways. Criterion 3's SYNC half is NOT closed and the test says so rather than implying otherwise. Completing an HTTPS exchange through the hardened fetcher is not possible in this repository: from_parts builds its client with test_tls_config, whose root store is EMPTY by design ("not one completes a handshake to a public host"), and http:// is correctly refused by the plaintext policy. So an in-process target can be dialed but never spoken to. That is a limit of the test seams, not of the feature, and it is why this fixture was never written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The WRITE half was pinned; the READ half was not. Nothing anywhere asserted a
field of FlowTargetView, so returning base64 from the listing -- the exact
affordance criterion 5 exists to forbid -- was green against the whole suite.
The fixture deliberately carries a value that LOOKS like code, because a config
of {"a":1} cannot distinguish a structured object from a re-encoding.
Also closes the secret_shaped_key guard, which the audit found had ZERO test
references: `fn secret_shaped_key(_) -> None` survived the entire suite, which
would let a pasted credential reach the listing, the delivered payload, and --
since D-1 -- a durable outbox row. The NESTED case is what is driven, because a
top-level-only check passes a config whose secret sits one level down, which is
where a copied snippet puts it. Paired with a clean nested config that must be
accepted, so the refusal is the guard talking rather than nesting itself.
Mutation-verified: both guards caught.
The listing mutant took three attempts, and the reason is worth recording. Twice
it reported ANCHOR 0 SKIPPED because I reconstructed the anchor text from memory
of the code rather than reading the bytes -- and a skip sits in the same column
as CAUGHT in a results table, so a sweep that mutates nothing looks like a sweep
that passes. The harness now locates the line by matching l.strip(), preserves
the file's own indentation, and asserts the text actually changed before running.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clippy flagged `Behavior::Answer` as never constructed, and it was right for a reason worth keeping rather than suppressing: the hardened fetcher cannot complete a handshake with an in-process server, so no test can ever be answered. An `#[allow(dead_code)]` would have left a false affordance in the file -- someone would later write a test against it and wonder why the target never replies. The helper now says what it is: a target that accepts and never answers, with the reason and issue #959 in its own doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two commits on this branch staged ZERO-LINE generated files: docs/events/catalog.json -2888 packages/ironauth-sdk/vectors/log-stream-vectors.json -85 Both had the same cause. A regenerating script was killed by a foreground timeout mid-write, leaving the file truncated, and a later `git add -A` swept it into a commit about something else entirely. Nothing in the commit message or the change under review hinted that a published artifact had been deleted. Both are restored by regenerating, and both are byte-identical to origin/main, which is the correct end state precisely because this branch adds no events and no log-stream vectors. Worth recording how close this came to merging. The catalog failure surfaced only because the gate diff named `webhook_endpoint.subscription_changed`, an event this branch never touches, which is what made it clear the artifact rather than the registry was wrong. The vectors file then slipped past my own first sweep for the same defect, because I searched with a >100-line threshold and this deletion was 85 lines. A threshold chosen to find the bug I had already seen was blind to the identical bug one file over; the correct filter is "insertions == 0", which has no magnitude in it at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fresh-eyes review, round 1Four independent lenses over the diff, each finding then handed to a separate verifier told to REFUTE it. 21 raised, 11 refuted, 10 confirmed, which dedupe to four distinct defects. All four are real. Fixing all of them in this PR. 1. The per-target timeout is unmeasured (HIGH, found by three lenses independently)
The verifier's demonstration is a one-character change: This matters beyond the test: criterion 6 reads "exceeding its timeout triggers the configured failure policy INSTEAD OF HANGING THE FLOW", and the boot path builds its fetcher with a 30s ceiling ( So the PR's criteria table overclaims row 4. The tests prove an elapsed consultation triggers the policy rather than returning Allow, with a fail-open control. They do not prove the operator's bound is what ended it. 2. Only one of the two signup doors is driven (HIGH)
3. The header claims a kill the file cannot make (MEDIUM)
That is the same unmeasured-sentence defect this PR exists to remove, so it has to go. 4. A doc comment was stolen from its function (MEDIUM, found by three lenses)
Accepted, not a findingThe empty-root-store TLS ceiling (#959) was ruled out of scope up front and the verifiers correctly refused findings that restated it. |
…nup door Round 1 of fresh-eyes review raised 21 findings; verifiers told to refute them killed 11, leaving 10 confirmed that dedupe to four. All four are fixed here. **The per-target timeout was unmeasured.** The criterion 6 test registered a 250ms bound and then asserted only the signup outcome, so nothing it checked was sensitive to WHICH deadline fired. Changing `.min(budget_remaining_ms)` to `.max(..)` in `consult_target` compiles without a warning, asks for the whole ~45s budget instead of the configured 250ms, and the never-answering server still times out against the fetcher's own ceiling. Every assertion stayed green and the suite merely ran slower. In production the boot fetcher's ceiling is 30s, so that one-character change holds a live registration 30s against a 250ms setting. Now timed, with the ceiling stated as a multiple of the test's OWN configured bound rather than of the fetcher default, so raising that default elsewhere can never silently widen the guard. Mutation-verified: the mutant reports "took 10.009049042s, allowed 5s", which is the fetcher's 10s default stepping in exactly as predicted. **Only one of the two signup doors was driven.** The legacy `POST /register` route and the flow API have separate `dispatch_sync` call sites, and the flow door had zero test callers. They are not interchangeable: the legacy door passes `None` for the signup form and collapses an interruption to Refuse, so it cannot reach the field-mapped path at all. Two tests added at the flow door, refuse and admit, so the refusal is attributable to the POLICY and not to the door. Mutation-verified by making the Refuse arm fall through: the fail-closed test goes red while the fail-open control stays green, which is what makes the pair discriminating. **The header claimed a kill the file cannot make.** It listed JSON pointer resolution among "the mutations these tests exist to kill". That one stays green: it lives in `classify_response`, reachable only after a successful fetch, which the empty root store forbids, and it is unreachable on the legacy door in principle since `dispatch_registration_targets` passes `None` for the form. Replaced with an explicit "what this file does NOT kill, and why" section. Claiming a kill the file cannot make is the same unmeasured sentence this file exists to remove, and it is worse than silence: the next reader takes it as covered and stops looking. **A doc comment was stolen from its function.** `install_flow_target_fetcher` had been inserted between `install_hashing_pool`'s doc block and its signature, so the new helper opened with five lines about an Argon2id hashing pool and `install_hashing_pool` shipped undocumented. Two artifacts wrong from one bad edit anchor. Each block now sits above the function it describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…brary `scripts/invariant-lints.sh` enforces a `time-via-env` rule: all wall-clock and monotonic time in this workspace flows through the `Clock` trait in ironauth-env, so protocol logic stays deterministic under test. The rule budgets exactly two raw uses, which are the production `Clock` impl itself. My elapsed-time guard added a third and the gate caught it. Rewired through `harness.env().clock().monotonic()`, which is the same clock `dispatch_sync` reads to measure the very span this test is checking. That is strictly better than the raw call: the test now measures time the way the code under test measures it, rather than from a second, independent source. Monotonic rather than wall clock for the reason the dispatcher gives at its own call site: an NTP step backwards would make the measured span exceed a budget nothing was actually slow for. One wrinkle worth recording, because it cost a cycle. The lint is a TEXT SCAN, so the first fix still failed: the comment explaining why the raw constructor is avoided contained the constructor's name, and the scanner counted it. The comment is now worded to describe the rule without spelling the token, and says so, since the next person to write that sentence will hit the same wall. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fresh-eyes review, round 2Round 1's fixes are themselves changes that had never been reviewed, so this pass audits the fix commit rather than the original PR. Three lenses, each finding handed to a separate verifier told to refute it. 13 raised, 12 refuted, 1 confirmed. The one confirmed finding was already fixed, independently
The gate caught this while the review was still running, and it is fixed in 1a275eb. Worth noting the reviewer proposed a different remedy: add an The reviewer also independently hit the trap I did: an intermediate version of the fix put the forbidden token inside a comment, and the lint is a plain text scan with no comment awareness. Two of us walking into the same hole is a good argument for the note now sitting in that comment. Three refuted findings I am overriding, having checked them myselfRecurrence across independent lenses made these worth re-checking, and all three are factually right. The verifiers refused them as style; in a PR whose entire subject is prose drifting from behaviour, they are in scope.
Fixing all three, with a compile-time assertion for the third so the failure is a build error naming the problem rather than a guard that quietly stops working. Notable refutationsThe strongest was against a claim that the elapsed guard is an upper bound only and so misses a mutant that SHORTENS the honoured bound. True of any upper bound, but the verifier showed each load-bearing limb fails on the actual code. A claim that both flow-door tests pass with no fetcher installed was also refuted on inspection of the helper. |
…right anyway Round 2 raised 13, refuted 12, confirmed 1. The confirmed one was the `time-via-env` break, already fixed in 1a275eb. These three were REFUTED as style. They recurred across independent lenses, so I checked them against the file, and all three are factually correct. In a change whose whole subject is prose drifting from behaviour, a doc that overclaims is the defect and not a cosmetic note about it. **`HANG_TIMEOUT_MS`'s doc overclaimed.** It reads "the per-target bound the hanging-target tests configure", while two of them still passed a bare `250`. Raising the constant would have left those two on the old value: the same shape as the header claiming a kill the file cannot make, which round 1 removed. Both now take the constant. **A cross-reference pointed the wrong way.** "the empty root store described above" sat at line 37; the first mention of the root store was line 75, below it. Reworded to say the thing rather than point at a description that does not precede it. **The elapsed ceiling could go inert without anyone noticing.** It is stated as `HANG_TIMEOUT_MS * 20`, which is 5s against the fetcher's 10s default. That is a guard at 250. At 500 the product is EXACTLY the default, and the assertion would pass whether the consultation was bounded by the target or by the fallback, which is the entire distinction it exists to draw. Being expressed as a multiple of the right constant does not make it safe at every value of that constant. Now checked at compile time against the real constant, with half the default as the margin so the two are never merely adjacent. Verified the guard can actually fail rather than only pass: at 250 the suite builds, and at 500 the build stops with `error[E0080]` naming the reason. A compile-time assertion nobody has watched fail is just an expression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 3 found a HIGH regression I introduced in round 2's own fix, and neither the
suite nor the gate could see it.
`1a275ebb` rewired the elapsed measurement to `harness.env().clock().monotonic()`
to satisfy the `time-via-env` invariant. It compiled, the suite stayed green 7/7,
and the full gate passed 52 static checks. The guard was dead. `Harness::start`
installs `Env::deterministic`, whose `ManualClock` moves only when a test calls
`advance`, and nothing here does. Both reads returned the same instant, so
`elapsed` was permanently zero and the assertion had degraded to `0 < 5s`. The
mutant round 1 added this guard to kill was quietly alive again.
A vacuous assertion PASSES, so a green suite and green static scans were both
entirely consistent with the defect, and there is no mutation lane in CI. Only
reading the harness finds it.
Real monotonic time restored, with the `invariant-allow` marker this repo already
grants a timing harness. That precedent is not a loophole: `outbound_timing_probe.rs`
states my exact case, that the Clock seam has no monotonic elapsed and injecting one
would measure the seam. The scan ceiling goes 2 to 3 for it.
**And a structural fix rather than only a repair.** An upper bound is satisfied by
zero, so it cannot detect its own death: `elapsed < ceiling` passes when the clock
stops, when the await is deleted, when the measurement never starts. There is now a
LOWER bound too. Against a target that never answers, a correctly bounded
consultation cannot return materially faster than its own bound. The floor is what
makes the ceiling falsifiable.
Two prose defects from the same review, both mine:
- the NTP rationale named the wrong direction. A step BACKWARDS shrinks a measured
span; it is a forwards step that inflates one. Now states both and what each does.
- the const-assert doc was wrong by a factor of ten, claiming the build breaks past
"a quarter of the default" (2500) when the assertion admits 250 and breaks at 251.
It now states the real threshold and that the bound and multiple are tuned together.
Re-verified on THIS tree rather than a superseded one, which is the other half of
the lesson: I had gone on citing a CAUGHT verdict measured before the clock swap,
and editing a guard invalidates the measurement that justified it.
1. suite with the restored measurement and the floor: 7 passed
2. `.min` -> `.max`: CAUGHT, "took 10.00690525s, allowed 5s"
3. reverting to the frozen Env clock: CAUGHT by the new floor, "returned in 0ns,
faster than half its 250ms bound ... or the clock being read does not advance,
which would make the ceiling assertion above vacuous"
The third is the one worth keeping: the guard can now detect the exact failure that
got past two review rounds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fresh-eyes review, round 3Two commits landed after round 2 closed and had never been reviewed, so this pass audits only that delta. 7 raised, 4 confirmed. The headline finding is a HIGH regression that I introduced myself in round 2's own fix, and that nothing in the test suite or the gate was capable of seeing. The round-2 lint fix silently killed the guard it was protecting
The guard was dead. No gate could have caught this. A vacuous assertion passes, so a green suite and green static scans were both fully consistent with the defect, and there is no mutation lane in CI. Reading the harness is the only thing that finds it. I also have to retract a claim I made in this PR's own description: the CAUGHT verdict I cited was measured against What changedReal monotonic time, with the And a structural fix rather than only a repair: the assertion now has a LOWER bound. An upper bound is satisfied by zero, so it cannot detect its own death: it passes when the clock stops, when the await is removed, when the measurement never starts. Against a target that never answers, a correctly bounded consultation cannot return materially faster than its bound. The floor is what makes the ceiling falsifiable. The other two confirmed findings, both prose
Re-verified on the final code, not on a superseded tree
|
1 participant
What
Closes the test half of #112. An audit of its six acceptance criteria found four
unmet for a single reason:
with_flow_target_fetcherhad exactly one caller inthe tree, the boot path, and no test installed it. So
consult_targethad ZEROcallers in the whole suite, and
dispatch_syncpast its no-fetcher branch wasdead code from the tests' point of view. The half of the feature that can reject
a signup in band shipped with no behavioural test at all.
This adds
crates/ironauth-oidc/tests/flow_target_sync.rs(7 tests) driving realregistrations through a real fetcher at BOTH signup doors, plus the criterion 5
read-half pin and the
secret_shaped_keyguard the audit found untested.Criteria this moves
secret_shaped_keyclosedNotes for the reviewer
field of
FlowTargetView, so returning base64 from the listing (the exactaffordance the criterion forbids) passed the whole suite. The fixture carries a
value that LOOKS like code on purpose: a config of
{"a":1}cannot distinguisha structured object from a re-encoding.
secret_shaped_keyhad zero test references.fn secret_shaped_key(_) -> Nonesurvived the entire suite. The NESTED case is what is driven, because a
top-level-only check passes a config whose secret sits one level down, which is
where a copied snippet puts it. Paired with a clean nested config that must be
accepted, so the refusal is the guard talking rather than nesting itself.
Fetcher::for_testsand
from_partsboth use an empty root store, so these tests assert that theconsultation REACHES the network, not that it gets an answer. That ceiling is
filed as No test can complete an HTTPS exchange through the hardened fetcher, blocking two #112 criteria #959 and is why the answering-target mode was deleted rather than
#[allow(dead_code)]-ed: leaving it would be a false affordance.(
catalog.json-2888,log-stream-vectors.json-85). Both are byte-identicalto main, which is correct because this branch adds no events and no vectors.
Review round 1 found four defects, all fixed here
A four-lens fresh-eyes review, each finding handed to a separate verifier told to
refute it: 21 raised, 11 refuted, 10 confirmed, deduping to four. The two that
mattered:
signup outcome, so nothing was sensitive to WHICH deadline fired. Changing
.min(budget_remaining_ms)to.max(..)inconsult_targetcompileswarning-free, requests the whole ~45s budget instead of the configured 250ms, and
the never-answering server still times out against the fetcher's own ceiling. All
five tests stayed green. Now pinned with an elapsed-time assertion stated as a
multiple of the test's OWN configured bound, so raising a default elsewhere can
never silently widen the guard.
POST /registerandthe flow API have separate
dispatch_synccall sites, and the flow door had zerotest callers. Two tests added there, refuse and admit, so the refusal is
attributable to the policy rather than to the door.
Also fixed: the file header claimed a mutation kill the file cannot make (JSON
pointer resolution, unreachable behind the empty root store and unreachable on the
legacy door in principle), and a new harness method had been inserted between
install_hashing_pool's doc comment and its signature, leaving one functionmisdocumented and the other undocumented.
Verification
flow_target_sync7/7,flow_target_delivery21/21, full DB suite set green..minto.max) turns the criterion 6 test redwith
took 10.009049042s, allowed 5s, which is the fetcher's 10s defaultstepping in exactly as predicted;
Refusearm fall through turns the fail-closed door testred while the fail-OPEN control stays green (
1 passed; 1 failed), so the pairdiscriminates the policy rather than the door.
event-catalog.shandlog-stream-vectors.sh.test-registration.shfloor raised 288 to 289, snug against the new count.