aft_inspect: passing `scope` makes the call slower, not faster — the parameter reads as a cost reduction and is the opposite

[iceteaSA]

scope is documented as narrowing output without reducing work. In practice it does something stronger and in the costly direction: passing a scope switches the diagnostics category off the cheap path onto the expensive one. Agents scope by reflex believing it is the considerate call, and it is the single most expensive request in the tool surface.

The branch

inspect/diagnostics_category.rs:86-102:

let collection = if applicability_is_empty {
    // ...
} else if scope_was_provided {
    // A deferred inspection can collect diagnostics for the entire root
    // without an explicit scope. Scope filters rendered results, not work.
    match collect_scoped_diagnostics(ctx, snapshot, scope) { /* ... */ }
} else {
    collect_warm_working_set(ctx, snapshot)
};

• no scope → collect_warm_working_set — the warm set only
• with scope → collect_scoped_diagnostics — per-file collection that opens/ensures a language server per file at up to PULL_FILE_TIMEOUT (10s) each, bounded only by the 120s phase budget

commands/inspect.rs:142 feeds the flag straight in as scope_was_provided || force_root_diagnostics.

The in-code comment is accurate about work not shrinking. The gap is that the parameter doesn't merely fail to help — it selects a different, slower collection strategy.

sections has the related-but-milder version of the problem: commands/inspect.rs:130 iterates InspectCategory::active() regardless, so every category is verified no matter what the caller asks to render. Net effect for a caller: there is no argument that makes aft_inspect cheaper. The only lever is not calling it.

Why the cost lands on other sessions

inspect maps to Lane::SerialLspStatus (subc/manifest.rs:124-131) and holds lsp_inflight, a term in has_epoch_reader, which gates Lane::Mutating (executor/mod.rs:2075-2079). Set once at admission (:2118), cleared once in complete_job (:1894).

So an expensive inspect is not just slow for its caller — it blocks every other session's writes on that root for its whole duration. Production pairing (from #243):

holder:  name=inspect      channel=832 corr=157  exec=77185ms
victim:  name=apply_patch  channel=833 corr=159  queue=71208ms exec=36ms

A 77-second inspect; a sibling's apply_patch waited 71.2s in queue and then ran in 36ms. That inspect was scope=src, sections=all — the exact shape this issue is about.

What the caller sees

aft_inspect [scope=src, sections=all, topK=20]
  route closed by closeRoute
aft_outline [target=tests, files=true]
$ git status --short && git rev-parse --short HEAD
  request on local_port=56346 channel 60 corr 1241 timed out after 18000ms

The follow-on timeouts are subsequent calls aging out behind the inspect. Nothing in that surface suggests scope caused it.

Suggestions, cheapest first

1. Fix the description. "Use scope= to narrow returned results. It does not reduce the fresh verification work" reads as cost-neutral. Something like "scope narrows rendered output; it also forces per-file diagnostics collection and is more expensive than an unscoped call" would stop the reflex.
2. Separate the two intents. Output narrowing and per-file diagnostic collection are different asks currently fused into one parameter. A caller who wants "show me only src/" almost never means "and collect diagnostics file-by-file across it."
3. Default the render filter to the cheap path — let scope narrow output while diagnostics stay on the warm-working-set path unless the caller explicitly opts into scoped collection.

(1) alone would have prevented every instance we've hit.

Minor, unrelated: collect_warm_working_set carries #[allow(dead_code)] at diagnostics_category.rs:115 but is live at :101.

Happy to send a PR for (1) if the wording is all you want.

[alfonso-aft]

Confirmed — the branch does exactly what you traced, and the inversion is real: the inline comment states the intended contract ("Scope filters rendered results, not work") while the scope_was_provided arm routes onto collect_scoped_diagnostics, the expensive path. The comment describes the design; the branch contradicts it. Agents scoping "considerately" are buying the priciest request on the surface — and our own tool description steers them into it.

Fix is queued rather than immediate for a sequencing reason: diagnostics_category.rs currently has a config-deadline change in flight (#241), and the same file sits inside the #242 lane-redesign blast radius — three uncoordinated edits to one file is how regressions ship. The scoped-collection fix lands right after the #241 branch integrates.

Direction, so it's on record: scope stops selecting the collection strategy — collection always takes the warm working-set path, scope filters at render, matching both the comment and the documented v0.51 semantics ("scope narrows returned findings, does not reduce or change verification work"). The one thing collect_scoped_diagnostics buys today — explicit pulls for scope files outside the warm set — moves behind the existing full-root freshness machinery instead of being scope's hidden superpower.

[iceteaSA]

Sequencing makes sense — three uncoordinated edits to diagnostics_category.rs is a worse outcome than waiting.

One hazard in the proposed direction, flagged now because it's cheap to design around and expensive to discover after: "collection always warm + scope filters at render" can produce a confident empty answer for a file nothing ever checked.

collect_warm_working_set is explicit that it reads only what's already there (:121-123):

// No-scope inspect is intentionally cheap: drain already queued LSP
// events, then read only the warm diagnostics store. It does not open
// files or spawn servers.

The freshness gate is is_reportable (:538-546), and its authority signal is global, not per-file:

(self.server_ran || self.applicability_is_empty || !self.producer_failures.is_empty())
    && self.servers_pending.is_empty()
    && self.files_without_server == 0
    && self.diagnostics.iter().all(|d| !d.provisional)

server_ran is lsp.has_any_diagnostic_reports() — any report from any server. So:

1. server reports for file Y → server_ran = true, servers_pending empty
2. caller scopes to file X, which that server has never reported on
3. collection is is_reportable → Fresh
4. render filters to X → empty
5. caller reads FRESH + no findings = X is clean

Today collect_scoped_diagnostics is what prevents this — it explicitly pulls scope files outside the warm set. If that moves behind full-root freshness, the guarantee has to move with it, or step 5 becomes reachable.

This is the exact anti-pattern in your own Honest Reporting Convention:

Returning success: true with empty results when the scope (path/glob) didn't resolve to any files — the agent reads it as "all clear" but really nothing was checked.

And the convention already prescribes the shape: complete: false plus a named gap (unchecked_files, or no_files_matched_scope where that fits). So the ask isn't new machinery — it's that when scope narrows to files with no verified diagnostic source, the payload must say so rather than render clean. Per-file coverage tracking rather than a global server_ran would be the honest version of that flag.

Not arguing against the direction — cheap-by-default with scope as a pure render filter is the right contract, and it's what the comment always claimed. Just that the safety property currently carried by the expensive path needs an explicit owner in the new one, otherwise the failure mode inverts from "slow and correct" to "fast and quietly wrong," which is worse for an unattended agent.

Also still standing from the original report, and now directly in the fix's path: collect_warm_working_set carries #[allow(dead_code)] at :115 while being live at :101.

[alfonso-aft]

Hazard accepted as a design constraint, and it's the right catch at the right time — this is the absence-rendered-as-zero class (the same honesty failure we fixed for dead-code counts when the callgraph is unavailable: absence of analysis must never render as a clean zero).

Folded into the queued fix as a binding requirement: scope filters at render, but the authority verdict becomes per-file-aware for scoped requests. A scoped file outside every producer's coverage (no server reported on it, no producer claims it, or its server never reached quiescence) renders as a named gap — complete: false with the file and reason — never as clean-empty. The global is_reportable conjunction stays for the no-scope path where it's sound (full-root verdicts over full-root collection); the scoped path gains the per-file coverage check before any empty result is allowed to look authoritative.

That also settles what collect_scoped_diagnostics was actually buying: not scope semantics, but per-file authority. The fix keeps the authority and drops the cost inversion.

[alfonso-aft]

Shipped on main (22da1a63 + 1e02dd3d):

1. Cost inversion dead: scope no longer selects the collection strategy — collection is always the warm working set, scope filters at render. The scoped path's document opens, server spawns, and per-file pulls are gone (collect_scoped_diagnostics deleted).
2. Your per-file authority constraint, as specified: a scoped file outside every producer's coverage renders as a named gap (uncovered_file, with the file and a reason distinguishing no-producer / no-current-report / server-still-warming) under complete: false — never clean-empty. The coverage check is mutation-tested (forcing it true fails the reporter-scenario test).
3. Blocking-fresh semantics preserved: the blocking path still starts producers and waits for root-level quiescence (bounded by inspect.diagnostics_timeout_ms, cancellation-aware, lock released across waits) — the wait now fills the warm store, then collection reads it. Scope and cost are finally orthogonal.

The tool description now states the corrected semantics. Ships in the next patch; your framing — "what collect_scoped_diagnostics was buying was per-file authority, not scope semantics" — is quoted almost verbatim in the design comment.

[iceteaSA]

Verified all three in-tree at 5d4b84a7 before accepting. Sound, and the coverage ladder is stronger than what I asked for.

Cost inversion gone. run_diagnostics_category now collects the warm working set unconditionally; scope_was_provided only reaches apply_scope + record_scope_coverage_gaps. collect_scoped_diagnostics is deleted, and the surviving collector's comment says it plainly: "The only diagnostics collection path, for scoped and unscoped requests alike."

The coverage ladder closes the hole I was actually worried about, which was narrower than I managed to say. My concern was per-file authority; the case that would have slipped a weaker fix is a file with a provisional report — present in the warm store, so trivially "covered," but not authoritative. scoped_file_coverage splits it:

no server for the file      → NoProducer
authoritative report        → Covered
report(s), all provisional  → Warming      ← the one that would have leaked
no report                   → NoReport

That matters more than usual here because the scoped path returns Fresh unconditionally and never reaches is_reportable, so the old global all(|d| !d.provisional) guard isn't backing it up. Per-file coverage is strictly stronger for a scoped question, so skipping the global check is right — but only because Warming exists. Without it the bypass would have been a regression.

FORCE_SCOPED_DIAGNOSTIC_COVERAGE is the part I'd call out. Shipping the mutation hook in the code rather than proving the check once and moving on means the coverage check can't silently become vacuous later. That's the same property as the instrument on #243 — a claim that stays checkable after the conversation ends.

I also checked the failure mode a partial version of this fix would have had: if the blocking path had stopped waiting before collecting, a scoped blocking-fresh inspect would return promptly with every file named as a gap — honest and useless, which is a quieter regression than the one we started with. start_applicable_servers → wait_for_root_quiescence still precedes collection, so the wait fills the warm store and collection reads it. Scope and cost genuinely orthogonal.

On the #243 instrument: started_before_oldest_writer per in-flight reader plus reader_admissions_while_promoted_writer_waited is exactly the discriminator I said I couldn't produce from my captures. Turning a claim I couldn't test into a counter that's always visible is a better outcome than either of us being declared right — if my single-mechanism read is wrong, that counter goes nonzero on someone's box and says so without anyone having to re-argue it.

Nothing further from me on these three. Our own guidance told implementers to scope "considerately," which is what surfaced this; I'll unwind that once we're on a build carrying the fix.

[alfonso-aft]

Your verification found the load-bearing detail worth naming for the record: Warming is what makes skipping the global is_reportable sound on the scoped path. A provisional report making a file trivially "covered" was exactly the arm that would have leaked under a weaker coverage ladder — and since scoped payloads never reach the global all(!provisional) guard, the per-file split isn't a refinement, it's the entire backstop. That reasoning now lives in this thread as the review record for anyone touching that ladder later.

Agreed on unwinding the "scope considerately" guidance once you're on a build carrying the fix — after the next release, scoping is genuinely free and the guidance can say so.