From be5fa1a511a8a92247c833250a5ecee3a4accb4c Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 03:01:18 -0400 Subject: [PATCH 1/3] docs(#770): delete slot.rs's false backstop claim, name the canonical refusal spelling in its assert #758's round-4 review filed three non-blocking findings after the PR merged (b0246bc): 1. The slot.rs module-doc paragraph justifying the line-based (not normalised) source guard ended "a reflow can cost it a line, not the guarantee." Re-measured against the tree: the crate's six blind command-slot writes (lifecycle::request_camp, nav::request_goto/request_follow/ request_zone_cross, social::request_who/request_friends_who) return (), u64, u64, u64, (), () respectively -- none returns bool, so none is in declared_no_overwrite_requests()'s coverage set, so none is exercised by every_no_overwrite_request_refuses_a_second_write. For a newly added blind write, a reflow costs the only defence there is, not "a line." Deleted the false clause rather than write a new coverage claim -- this repo's own #772 history shows a replacement tends to re-instantiate the finding. 2. guild.rs's no_silent_overwrite_guard rejects a behaviourally-correct match-shaped refusal by design (canonicalisation lint, not a malfunction), but its failure message only cited conjunct numbers a reader has to cross-reference against a fixture table. Now names the one accepted spelling inline. Mutation-checked by making it fire on a temporarily-mutated real call site, then reverted via copy-back + md5sum verification. The PR's "1575 / 0 / 45" test-count figure (round-2 reasoned prediction, not measured for the merged head) lives in PR #758's body, not in any tracked file. Ran the full workspace test suite once on this branch instead of reasoning forward, and appended a dated, attributed correction to #758's body with the real number. Refs #758. --- crates/eqoxide-command/src/slot.rs | 2 +- crates/eqoxide-http/src/guild.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/eqoxide-command/src/slot.rs b/crates/eqoxide-command/src/slot.rs index 1fb5939d..04109f00 100644 --- a/crates/eqoxide-command/src/slot.rs +++ b/crates/eqoxide-command/src/slot.rs @@ -161,7 +161,7 @@ mod tests { /// out of the sources. **No reformatting evades a behavioural test.** Wrap a write across five /// lines and the second write still has to be refused, or that test fails. This static rule is /// a redundant early-warning on top of it, catching a *newly added* blind write before anyone - /// writes a behaviour test for it; a reflow can cost it a line, not the guarantee. + /// writes a behaviour test for it. #[test] fn no_domain_module_blind_writes_a_command_slot() { let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); diff --git a/crates/eqoxide-http/src/guild.rs b/crates/eqoxide-http/src/guild.rs index 1dea3378..bcfac43d 100644 --- a/crates/eqoxide-http/src/guild.rs +++ b/crates/eqoxide-http/src/guild.rs @@ -569,7 +569,8 @@ mod no_silent_overwrite_guard { } else { offenders.push(format!( "{file}:{line}: {name} — not the canonical refusal (violates conjunct(s) \ - {faults:?} of refusal_shape_faults)" + {faults:?} of refusal_shape_faults). The one accepted spelling is:\n \ + if let Some(busy) = s.command.{name}(..).refused(MSG) {{ return busy; }}" )); } } From 6bfc5c2221172b1b8713f4f860a1faf98eaae852 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 04:37:33 -0400 Subject: [PATCH 2/3] =?UTF-8?q?docs(#770):=20fix=20round=202=20=E2=80=94?= =?UTF-8?q?=20name=20the=20accepted=20refusal=20shape=20per-site,=20finish?= =?UTF-8?q?=20slot.rs's=20false-claim=20deletion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent reviewer requested changes on the round-1 fix, finding four more false/misleading claims introduced by that fix itself: - guild.rs's assert message named `.refused(` as "the one accepted spelling," but the guard accepts either `.refused(` or `.refused_json(` — five real sites use the latter, and applying the old suggestion there does not compile. `Site` now records which terminator conjunct 2 actually found at each call site, and the message echoes that one back instead of guessing; when neither is present, it shows both accepted shapes side by side. Compile-verified at both a `.refused(` site and a `.refused_json(` site via mutation (combat.rs's post_attack_on and post_target_name handlers). - slot.rs's paragraph deletion was incomplete: it still called the static rule "a redundant early-warning on top of" the behavioural test and said the eqoxide-http reflow argument "does not transfer here." Both are false for the crate's blind writes (confirmed: a reflowed, un-annotated blind write evades both the static guard and the behavioural test). Deleted both remaining claims; composed no replacement beyond the issue's own accepted-limit sentence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-command/src/slot.rs | 10 +++----- crates/eqoxide-http/src/guild.rs | 41 +++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/crates/eqoxide-command/src/slot.rs b/crates/eqoxide-command/src/slot.rs index 04109f00..e5d07267 100644 --- a/crates/eqoxide-command/src/slot.rs +++ b/crates/eqoxide-command/src/slot.rs @@ -153,15 +153,13 @@ mod tests { /// **Why this one stayed line-based when `eqoxide-http`'s guard did not (round 2, N2 / round /// 3).** That guard was rewritten to normalise the source because a call site reflowed across /// lines silently left its coverage — the guard was the ONLY thing standing behind those call - /// sites, so a formatting change could delete a check. That argument does not transfer here, - /// and the reason is stronger than "the risk is lower": this crate's real universal is + /// sites, so a formatting change could delete a check. This crate's real universal is /// [`every_no_overwrite_request_refuses_a_second_write`], which is BEHAVIOURAL — it issues a /// second write to a live slot and asserts the first message is the one that drains — and its /// case table is completeness-checked against [`declared_no_overwrite_requests`], itself read - /// out of the sources. **No reformatting evades a behavioural test.** Wrap a write across five - /// lines and the second write still has to be refused, or that test fails. This static rule is - /// a redundant early-warning on top of it, catching a *newly added* blind write before anyone - /// writes a behaviour test for it. + /// out of the sources. That coverage set is exactly the `-> bool` refusing methods; this + /// crate's blind writes are not among them, so this static rule is the only defence for a + /// blind write, and that is an accepted limit, not a backstopped one. #[test] fn no_domain_module_blind_writes_a_command_slot() { let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); diff --git a/crates/eqoxide-http/src/guild.rs b/crates/eqoxide-http/src/guild.rs index bcfac43d..76a63229 100644 --- a/crates/eqoxide-http/src/guild.rs +++ b/crates/eqoxide-http/src/guild.rs @@ -328,6 +328,13 @@ mod no_silent_overwrite_guard { /// Carried rather than collapsed to a `bool` so the table test can pin each conjunct /// separately — see R3-1 on [`refusal_shape_faults`]. faults: Vec, + /// Which terminator conjunct 2 actually found in this site's statement, if either — + /// `.refused(` and `.refused_json(` are not interchangeable (one returns + /// `(StatusCode, String)`, the other `Response`), so a failure message must echo back + /// the one this call site actually uses rather than assume either one (#770 B1: a + /// hardcoded `.refused(` suggestion does not compile at a `.refused_json(` site). + /// `None` only when conjunct 2 itself is violated (neither string present). + terminator: Option<&'static str>, } impl Site { @@ -358,10 +365,20 @@ mod no_silent_overwrite_guard { (_, None) => text.len(), }; let stmt = &text[start..end.max(start)]; + // Same two literals conjunct 2 checks; recorded independently so a failure message + // can name the one this site actually contains instead of assuming either one. + let terminator = if stmt.contains(".refused_json(") { + Some(".refused_json(") + } else if stmt.contains(".refused(") { + Some(".refused(") + } else { + None + }; out.push(Site { line: map.get(pos).copied().unwrap_or(0), name, faults: refusal_shape_faults(stmt), + terminator, }); } out @@ -561,16 +578,34 @@ mod no_silent_overwrite_guard { let file = path.file_name().unwrap().to_string_lossy().to_string(); for site in refusal_sites(&text) { let canonical = site.canonical(); - let Site { line, name, faults } = site; + let Site { line, name, faults, terminator } = site; if !refusable.contains(&name) { ignored_by_design.push(format!("{file}:{line}: {name}")); } else if canonical { checked.push(format!("{file}:{line}: {name}")); } else { + // #770 B1: there are two accepted terminators, not one, and they are not + // interchangeable — `.refused(` returns `(StatusCode, String)`, + // `.refused_json(` returns `Response`. When conjunct 2 already found one of + // them at this site, echo that one back (it is guaranteed to typecheck, + // since it is the terminator the handler already calls); only when neither + // is present (conjunct 2 itself is violated) do we not know which return + // type this handler uses, so both accepted shapes are shown side by side + // instead of guessing. + let shape = match terminator { + Some(t) => format!( + "if let Some(busy) = s.command.{name}(..){t}MSG) {{ return busy; }}" + ), + None => format!( + "if let Some(busy) = s.command.{name}(..).refused(MSG) {{ return busy; }} \ + // if the handler returns `(StatusCode, String)`\n \ + if let Some(busy) = s.command.{name}(..).refused_json(MSG) {{ return busy; }} \ + // if the handler returns `Response`" + ), + }; offenders.push(format!( "{file}:{line}: {name} — not the canonical refusal (violates conjunct(s) \ - {faults:?} of refusal_shape_faults). The one accepted spelling is:\n \ - if let Some(busy) = s.command.{name}(..).refused(MSG) {{ return busy; }}" + {faults:?} of refusal_shape_faults). The accepted shape is:\n {shape}" )); } } From 6c9b4e621ff0ab8c80af3145e7622f1ead358a44 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 06:22:38 -0400 Subject: [PATCH 3/3] =?UTF-8?q?docs(#770):=20fix=20round=203=20=E2=80=94?= =?UTF-8?q?=20stop=20guessing=20which=20refusal=20terminator=20a=20site=20?= =?UTF-8?q?uses,=20restore=20slot.rs's=20dropped=20reflow=20clause?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2's per-site terminator echo was itself false: the code lived entirely inside untested dead code on a clean tree, and its "guaranteed to compile" claim breaks by construction whenever the flagged call's statement span bleeds into the next one (a missing semicolon inside `{ return busy` — exactly what conjunct 3 exists to flag). Reverts the terminator field/detection and replaces it with a standalone offender_message() that always states both accepted shapes, plus a direct unit test (offender_message_names_both_accepted_shapes) that pins its text without needing the scanner to reach a real non-canonical site. Mutation-checked both directions: reintroducing the old single-shape design fails the new test; the reviewer's exact span-bleed reproduction now names both shapes at each flagged site instead of guessing one. Also restores "and a reflow does cost it," a clause round 2's edit dropped from slot.rs's accepted-limit sentence while this PR's body claimed it was unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-command/src/slot.rs | 8 ++- crates/eqoxide-http/src/guild.rs | 106 ++++++++++++++++++----------- 2 files changed, 71 insertions(+), 43 deletions(-) diff --git a/crates/eqoxide-command/src/slot.rs b/crates/eqoxide-command/src/slot.rs index e5d07267..78aacba7 100644 --- a/crates/eqoxide-command/src/slot.rs +++ b/crates/eqoxide-command/src/slot.rs @@ -158,8 +158,12 @@ mod tests { /// second write to a live slot and asserts the first message is the one that drains — and its /// case table is completeness-checked against [`declared_no_overwrite_requests`], itself read /// out of the sources. That coverage set is exactly the `-> bool` refusing methods; this - /// crate's blind writes are not among them, so this static rule is the only defence for a - /// blind write, and that is an accepted limit, not a backstopped one. + /// crate's blind writes are not among them, so for a blind write this static rule is the only + /// defence *and a reflow does cost it* — measured, not assumed: an un-annotated blind write + /// added to a domain module and wrapped across lines the way rustfmt wraps a long receiver + /// chain passes this crate's tests fully green, because the wrap breaks this rule's `*self.` + /// anchor and a blind writer was never in the behavioural universal's coverage set to begin + /// with. That is an accepted limit, not a backstopped one. #[test] fn no_domain_module_blind_writes_a_command_slot() { let src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); diff --git a/crates/eqoxide-http/src/guild.rs b/crates/eqoxide-http/src/guild.rs index 76a63229..246fafcf 100644 --- a/crates/eqoxide-http/src/guild.rs +++ b/crates/eqoxide-http/src/guild.rs @@ -328,13 +328,6 @@ mod no_silent_overwrite_guard { /// Carried rather than collapsed to a `bool` so the table test can pin each conjunct /// separately — see R3-1 on [`refusal_shape_faults`]. faults: Vec, - /// Which terminator conjunct 2 actually found in this site's statement, if either — - /// `.refused(` and `.refused_json(` are not interchangeable (one returns - /// `(StatusCode, String)`, the other `Response`), so a failure message must echo back - /// the one this call site actually uses rather than assume either one (#770 B1: a - /// hardcoded `.refused(` suggestion does not compile at a `.refused_json(` site). - /// `None` only when conjunct 2 itself is violated (neither string present). - terminator: Option<&'static str>, } impl Site { @@ -347,6 +340,17 @@ mod no_silent_overwrite_guard { /// The statement bounds are the previous `;`/`{`/`}` and — because the canonical shape carries /// its `return busy;` inside a block — the first `;` after the statement's opening brace, when /// a brace comes before the next `;`. + /// + /// **Do not add a "which terminator did this site use" field here (#770 B1, round 2 attempted + /// exactly this and it was reverted).** The span above ends at the first `;` *after* the + /// opening brace, so a malformed site missing its `;` inside `{ return busy` — the very thing + /// conjunct 3 exists to flag — lets the span run on into the FOLLOWING statement and pick up + /// *its* terminator instead. A message that echoes "the terminator this span contains" is then + /// not "the terminator this call site uses"; it can name the wrong one, on exactly the + /// malformed input the guard is supposed to describe accurately. Fixing the span to stop at + /// the offending statement's own closing brace would be a behaviour change, out of place in a + /// docs-only fix; until that happens, [`offender_message`] states both accepted shapes rather + /// than guessing which one applies. fn refusal_sites(src: &str) -> Vec { const NEEDLE: &str = "s.command.request_"; let (text, map) = normalize(src); @@ -365,25 +369,37 @@ mod no_silent_overwrite_guard { (_, None) => text.len(), }; let stmt = &text[start..end.max(start)]; - // Same two literals conjunct 2 checks; recorded independently so a failure message - // can name the one this site actually contains instead of assuming either one. - let terminator = if stmt.contains(".refused_json(") { - Some(".refused_json(") - } else if stmt.contains(".refused(") { - Some(".refused(") - } else { - None - }; out.push(Site { line: map.get(pos).copied().unwrap_or(0), name, faults: refusal_shape_faults(stmt), - terminator, }); } out } + /// The offender message for a non-canonical [`Site`]. Kept as its own function, rather than + /// inlined at the one call site, so a test can pin its exact text without needing the scanner + /// to find a real non-canonical site on this (canonical) tree — see + /// `offender_message_names_both_accepted_shapes` below. + /// + /// #770 B1, round 2: this used to echo back "the" terminator found in the site's scanned + /// statement, labelled as the one accepted spelling / guaranteed to compile. Both claims were + /// false — conjunct 2 accepts two non-interchangeable terminators, and the span that finds one + /// of them is not reliably bounded to the flagged call (see the note on [`refusal_sites`]). So + /// this states both accepted shapes plainly instead of asserting which one is correct here: a + /// message that says less but is never wrong beats one that is usually right. + fn offender_message(file: &str, line: usize, name: &str, faults: &[u8]) -> String { + format!( + "{file}:{line}: {name} — not the canonical refusal (violates conjunct(s) {faults:?} of \ + refusal_shape_faults). The accepted shape is one of:\n \ + if let Some(busy) = s.command.{name}(..).refused(MSG) {{ return busy; }} \ + // handler returns (StatusCode, String)\n \ + if let Some(busy) = s.command.{name}(..).refused_json(json!({{ .. }})) {{ return busy; }} \ + // handler returns Response" + ) + } + /// R2-1 + R2-3 + R3-1, pinned. Every row is a literal source snippet; the expectation is the /// EXACT set of [`refusal_shape_faults`] conjuncts it violates (`None` = no site enumerated at /// all, `Some([])` = canonical). A return to line-oriented detection flips the wrapped rows to @@ -541,6 +557,36 @@ mod no_silent_overwrite_guard { ); } + /// #770 B1, round 2: [`offender_message`] is built inside `!canonical`, which no site on this + /// (canonical) tree ever reaches — a green run of + /// [`every_refusable_command_request_is_checked_by_its_http_caller`] exercises it zero times, so + /// it was possible to publish a false claim ("guaranteed to compile") about a code path no test + /// ever executed. Calling the function directly, on a fabricated non-canonical case, is what + /// makes it not-dead: this test fails if the message text drifts from either accepted shape, or + /// if it starts claiming to know which one applies to a given site (the thing that was false). + #[test] + fn offender_message_names_both_accepted_shapes() { + let msg = offender_message("combat.rs", 152, "request_attack", &[1, 3]); + assert_eq!( + msg, + "combat.rs:152: request_attack — not the canonical refusal (violates conjunct(s) [1, 3] \ + of refusal_shape_faults). The accepted shape is one of:\n \ + if let Some(busy) = s.command.request_attack(..).refused(MSG) { return busy; } \ + // handler returns (StatusCode, String)\n \ + if let Some(busy) = s.command.request_attack(..).refused_json(json!({ .. })) { return busy; } \ + // handler returns Response" + ); + // Anti-vacuity against the exact failure mode this replaces: the message must not single + // out one shape as "the" accepted one, or name a terminator without the other beside it — + // both `.refused(` and `.refused_json(` must be present, uncoupled from which one (if + // either) actually appears at the flagged site, because the scanner cannot reliably tell + // (see the note on `refusal_sites`). + assert!(msg.contains(".refused(MSG)") && msg.contains(".refused_json(json!({ .. }))"), + "offender_message must always name BOTH accepted shapes, not the one it guesses: {msg:?}"); + assert!(!msg.to_lowercase().contains("guarantee"), + "offender_message must not claim a compile guarantee it cannot back: {msg:?}"); + } + /// The universal at the HTTP boundary: a handler that calls a refusable `request_*` and IGNORES /// the returned `bool` has re-created exactly the #347 defect — the command was dropped on the /// floor and the caller was still told `200`. Every such call site must consume the result. @@ -578,35 +624,13 @@ mod no_silent_overwrite_guard { let file = path.file_name().unwrap().to_string_lossy().to_string(); for site in refusal_sites(&text) { let canonical = site.canonical(); - let Site { line, name, faults, terminator } = site; + let Site { line, name, faults } = site; if !refusable.contains(&name) { ignored_by_design.push(format!("{file}:{line}: {name}")); } else if canonical { checked.push(format!("{file}:{line}: {name}")); } else { - // #770 B1: there are two accepted terminators, not one, and they are not - // interchangeable — `.refused(` returns `(StatusCode, String)`, - // `.refused_json(` returns `Response`. When conjunct 2 already found one of - // them at this site, echo that one back (it is guaranteed to typecheck, - // since it is the terminator the handler already calls); only when neither - // is present (conjunct 2 itself is violated) do we not know which return - // type this handler uses, so both accepted shapes are shown side by side - // instead of guessing. - let shape = match terminator { - Some(t) => format!( - "if let Some(busy) = s.command.{name}(..){t}MSG) {{ return busy; }}" - ), - None => format!( - "if let Some(busy) = s.command.{name}(..).refused(MSG) {{ return busy; }} \ - // if the handler returns `(StatusCode, String)`\n \ - if let Some(busy) = s.command.{name}(..).refused_json(MSG) {{ return busy; }} \ - // if the handler returns `Response`" - ), - }; - offenders.push(format!( - "{file}:{line}: {name} — not the canonical refusal (violates conjunct(s) \ - {faults:?} of refusal_shape_faults). The accepted shape is:\n {shape}" - )); + offenders.push(offender_message(&file, line, &name, &faults)); } } }