fix(overview): the card answers about a root it cannot read, and counts the rest #4
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/overview-card-unreadable-roots"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Note 1 from your review of #3, plus two things next to it that turned out to be
wrong the same way. Notes 2 and 3 are in here too; 4 and 5 are argued below
rather than changed.
1. The card could not say "unreadable" — and did not say nothing, either
probeRoots()settled what a root IS; the card still built its own rows out ofthe probe and dropped
okanderror. So an unreadable root reached the widgetas null usage, and null usage renders as
—free plus a 0%-full bar with0%next to it. That is not a missing answer, it is the rendering that means an
empty healthy mount — on the one surface nobody reads, only glances at.
rootInfos()infsops.tsmaps probes for both web surfaces now, so what acaller is TOLD cannot drift the way what a caller ASKED FOR used to.
/rootswas already correct and goes through it unchanged;
/overviewis a subset anda total away from being the same route.
The card renders
unreadableand the reason, and drops the bar.2. "3 more roots" was counted from the slice
/overviewprobedpolicy.roots.slice(0, 4)and the widget computed thehidden count from what it received, so with sixteen roots configured the card
said 1 more root while hiding thirteen. The route sends
totalnow.Pre-existing —
b49df6fhas the same slice — so it did not arrive with #3.3. One probe per load was paid for a row nobody draws
The slice was four; the widget renders three. Every dashboard load stat'd and
df'd a fourth root that is never on screen, which on a spun-down disk costs
seconds. One constant, named for what the card can fit, decides both now.
Your notes 2 and 3
The tripwire. Inverted, as you said: every mention of
policy.rootsin aroute file must be a
probeRoots()argument, with by-name lookups (.length,.find(,[0], …) the exception, and the window is six lines because fmt putsthe call's arguments on their own lines. Verified by injecting each shape you
named into
routes.ts:None of the three failed the old regex.
barrierDf. NowprobeRoots(...).finally(df.done). One correction to thenote:
df.done()already ran before the assertions, and the only path thatskipped it was a barrier timeout — where the timer had fired, so nothing leaked.
usageOfcatches its exec rejection (fsops.ts:330), soprobeRootsdoes notreject either. The
finallyis worth having; it was not covering a live leak.Your notes 4 and 5, not changed
4 — a missing root's
not found. For a configured root the resolver's404 can only mean the directory is absent:
guard()'s "not inside any root"spelling needs a path outside the roots, and a root is inside itself. The only
caller that passes a non-root is a test. A deny rule still carries its own
message. Happy to be shown a case where the two are confusable on
/roots.5 — the uncapped fan-out. Left as is.
dfis cheap,UsageCachecollapsesrepeats per path, and the burst is bounded by how many roots an operator
configured — which is now three for the card, and the listing genuinely wants
all of them at once.
Verification
deno test --allow-read --allow-write --allow-run=python3 backend/— 60passed, up from 58: the mapper keeps a failed probe's
ok/error, and giveseach root its own scan in the configured order.
deno lintclean (33 files).deno check backend/mod.tsclean in a corecheckout,
svelte-check0 errors (the one warning istsconfig.check.json'sbaseUrldeprecation, on main too), andmodule-builderbuilds the frontend.deno fmt --check: every line this branch adds is clean — the complaints inroutes.ts,fsops.ts,api.ts,contract_test.tsandCLAUDE.md's tableland on lines it does not touch, checked by diffing a formatted copy against the
branch's own hunks.
Against a running server — six roots,
photosdenied,goneabsent, host root/:and the card itself, rendered headless in its real slot:
mediawith its bar,goneasunreadable/not found,photosasunreadablewith the denyrule ellipsised to the slot width, footer
read/write · 3 more roots.The fix itself is right and the tripwire holds — one blocker, one thing the PR's own argument applies to, two nits.
Blocker:
totalis a claim without a testBug 2 ("counted from the slice") is fixed by
total: policy.roots.lengthin/overview, and bug 3 byOVERVIEW_ROOTS. Neither is asserted anywhere: no*_test.tstouches/overview, so the mutationtotal: probes.length— bug 2 reintroduced — passes all 60 tests. The newrootInfostests cover bug 1 only. Add a route-level test: six roots configured, the response carries three rows andtotal === 6.Also
OverviewWidget.svelte:73—ok=true, usage=null(the no-coreutils case) still draws<ProgressBar value={0}>. That is the same "empty healthy mount" rendering this PR removes for unreadable roots, and README line ~130 promises "no usage bar rather than a wrong one" for exactly this case. Pre-existing, but the argument in your description is the argument against it: wrap the bar in{#if root.usage}.roots_test.ts:168— comment still says "the overview card renders four roots"; this PR makes it three.contract_test.ts:93— the window isindex-6 .. index+1, so apolicy.roots.map(up to six lines after anyprobeRoots(call also passes — e.g. one placed inside/overview'shono.json({...}). Lines above only, or require the mention to sit inside theprobeRoots(argument list.Notes 4 and 5 as argued: agreed, no change needed.
Verification (Windows box)
deno test --allow-read --allow-write --allow-run=python3 backend/: 48 passed, 12 failed on this branch; main: 46 passed, the same 12 failed (POSIX/srvroots, symlinks, python3 — environmental). Consistent with your 58 → 60.deno lint: clean, 33 files.deno fmt --check: clean on this branch and on main here, so the untouched-line complaints you saw are a deno version difference, not the branch.const rs = policy.roots;intoroutes.ts, the contract test failed at that line. Claim holds.svelte-check/module-buildernot rerun here.@ -86,1 +91,3 @@if (!walks) continue;if (!line.includes("policy.roots") || lookup.test(line)) continue;// the call may be several lines up: `probeRoots(c, usage, roots.slice())`// is over fmt's column limit and lands one argument per lineWindow reaches one line below and six above — a
policy.roots.map(within six lines after aprobeRoots(call (e.g. inside/overview'shono.json) still passes. Lines above only, or require the mention inside the argument list.Stale: the card renders three now.
@ -667,0 +671,4 @@// how many are configured, not how many were probed: the card counts the// rest into "N more", and counting the slice told an operator with// sixteen roots about onetotal: policy.roots.length,Not asserted by any test —
total: probes.length(bug 2 reintroduced) passes all 60. Needs a route-level test: six roots, three rows,total === 6.@ -68,2 +72,2 @@{root.scan ? `${formatBytes(root.scan.bytes)} · ${ago(root.scan.finishedAtMs)}` : ago(null)}</div>{#if root.ok}<ProgressBar value={root.usage?.usedPct ?? 0} />ok=true, usage=null(no coreutils) still draws a 0%-full bar — the same rendering this PR removes for unreadable roots, and README promises no bar for this case.{#if root.usage}around the bar.`ok=true, usage=null` is a deployment without coreutils, and the card drew `<ProgressBar value={0}>` for it: the same "empty healthy mount" rendering this branch removes for an unreadable root, on the surface nobody reads and only glances at. README line 130 has promised "no usage bar rather than a wrong one" for exactly this case since before the branch. The bar is now inside `{#if root.usage}`; the free-space column already said "—" here, and the scan line below it never depended on df.The window was `index-6 .. index+1`, and a window reaches both ways: a `policy.roots.map(` up to six lines BELOW a `probeRoots(` call passed too — inside the `hono.json({...})` that answers with the probes, which is precisely where the drift this tripwire exists for would land. "Near the call" only ever stood in for "inside the call", because fmt puts a long call's arguments on their own lines. So the argument list is what is matched now: every `probeRoots(` argument list is blanked out by a paren scan, newlines and columns kept, and a `policy.roots` surviving that is the walk. Both legitimate shapes still pass — the call on one line and split over four — and every shape the review named is caught, wherever it sits relative to a call: an alias, `forEach`, a multi-line `for (`, a spread, `flatMap`, `filter`. By-name lookups stay the exception.The shipped fix is right, and the placement is the good part:
rootInfos()andoverviewRoots()sit next toprobeRoots()because that is where a test can reach them without core's import map, and the route is left with nothing to get wrong.totalfixes the count, the card stops drawing a 0%-full bar for a root nobody can read, and the fourth probe nobody rendered is gone. Your answers on notes 4 and 5 are fair; I have nothing to add to either..finally(df.done)is safe —doneis an arrow closure overtimer, so the unbound reference is fine — andDirScanSummarysurvives inapi.tsthrough the re-export, so the narrowed overview type leaves no dead import.Requesting changes on two holes in the tripwire itself. Both are cases where the test passes while checking nothing, which is the one failure mode a tripwire cannot have: the shapes you injected all get caught today, and both of these would still catch them, so the injection run does not distinguish a working tripwire from a disabled one.
withoutProbeArgs()has no bail-out when the parens never balance. AnyprobeRoots(whose(does not close — a prose comment, a)inside a string argument — blanks every remaining character in the file, and everypolicy.rootsbelow it goes unchecked with no signal.lookupregex is tested against the whole line, so one legitimate lookup anywhere on a line excuses a walk on that same line.The rest are nits, take them or leave them: the widget's own row cap is gone, so the number that has to fit a fixed slot now lives only in the backend, and an unreadable root drops its scan line even though
rootInfos()still carries it.@ -34,0 +47,4 @@let at = source.indexOf(CALL);while (at >= 0) {let depth = 0;for (let i = at + CALL.length - 1; i < chars.length; i++) {No bail-out when the parens never balance. If the scan reaches
chars.lengthwithdepth > 0, every character from theprobeRoots(onward is blanked — the whole rest of the file — and the loop below then asserts against blank lines, so everypolicy.rootsafter that point passes unchecked with nothing printed.Two ways in, both plausible in this repo: prose in a comment that names the call without closing it (
// probeRoots(c, usagewrapped by fmt), or a)inside a string argument. routes.ts already carriesprobeRoots()in a comment on line 653 — balanced today, so this is latent rather than live.Make it loud instead of silent:
A thrown error fails the test, which is the correct outcome — the source stopped being something this check can reason about.
@ -79,1 +112,4 @@// Asking how many there are or for one by name is not describing them.const lookup =/policy\.roots\.(length\b|find\(|findIndex\(|findLast\(|some\(|every\(|includes\(|indexOf\(|at\()|policy\.roots\[/;for (const file of ROUTE_FILES) {Question rather than a change:
some(andevery(are inlookup, but they iterate.policy.roots.some((r) => { render(r); return false; })is the walk this test exists to catch, spelled as a predicate.find(I would keep — it answers with one root, which is the by-name case the comment describes — butsome/everyseem to be on the wrong side of the line you drew. Narrowing tolength|find|findIndex|findLast|indexOf|includes|at|[costs nothing today, since no route file uses either.@ -87,3 +122,3 @@assert(line.includes("probeRoots("),!named.includes("policy.roots") || lookup.test(named),`${file}:${index + 1} walks the roots itself:\n ${line.trim()}`,lookup.test(named)matches anywhere on the line, so a line that does both passes on the strength of the half that is allowed:fmt keeps those on one line when they are short enough, and a
const { length } = policy.rootsnext to an alias is the same shape. Subtract the allowed mentions rather than looking for one:That also removes the need to keep the regex anchored to a single mention per line.
@ -34,3 +36,2 @@const shown = $derived(data?.roots.slice(0, 3) ?? []);const hidden = $derived(Math.max(0, (data?.roots.length ?? 0) - shown.length));const shown = $derived(data?.roots ?? []);The widget's own
slice(0, 3)is gone, so the number that has to satisfy a fixed ~260px slot is now onlyOVERVIEW_ROOTSinfsops.ts. The constant's doc comment states the constraint, but it states it in a file where nobody is looking at the slot: raise it to 6 and the card clips four rows with no error on either side, and the backend test assertsOVERVIEW_ROOTSagainst itself so it stays green.Single source of truth is the right call — I would not send the number back to the frontend. But the rendering side could still refuse to overflow:
data?.roots.slice(0, 3) ?? []withhiddenstill counted fromtotalkeeps both fixes and puts the CSS constraint back next to the CSS.@ -70,0 +81,4 @@{:else}<!-- no bar: a 0%-full one on a root nobody can read says "empty",which is the opposite of what happened --><div class="meta why" title={root.error ?? ""}>Dropping the bar here is right. Dropping the
.metascan line with it is a second decision, and I do not think it follows:rootInfos()carriesscanfor a failed probe, so the data is in hand, and an unreadable root is usually a mount that was fine an hour ago. "14.2 GB · 3h ago" under the error is the sentence an operator wants — it says the root has content and this is a mount problem, not an empty disk.Two lines where the ok branch has one, so check it still fits three roots in the slot — if it does not, this is not worth the row.
The allowed shapes were looked FOR, with lookup.test(named) matching anywhere on the line, so a line that did both passed on the strength of the half that is allowed: `const n = policy.roots.length, all = policy.roots;` is a bare alias next to a count, and fmt keeps it on one line while it is short. The allowed mentions are subtracted now and whatever is left over fails, which also drops the need to keep the pattern to one mention per line. some( and every( leave the allow-list with it. Both visit every root — `policy.roots.some((r) => { render(r); return false; })` is the walk this test exists to catch, spelled as a predicate — where find( answers with one root, which is the by-name case the comment describes. No route file uses either.Four of your five, plus one measurement that changed the answer.
1, 2, 3 — the tripwire holes. All three real, all three fixed. Verified by injecting each into
routes.tsand restoring afterwards: an unbalancedprobeRoots(now throwsunbalanced probeRoots( at offset 27021instead of blanking the rest of the file;const n = policy.roots.length, all = policy.roots;andpolicy.roots.some((r) => …)both now fail. Every real route-file line still passes.One correction on the regex you suggested for 2 — I did not take it verbatim. Its
(\.(length\b|…)\(|\[)shape requirespolicy.roots.length(, so plainpolicy.roots.lengthwould stop being stripped and the test would fail onbackend/ui.ts:347(rootCount: policy.roots.length,). I keptlength\boutside the\(requirement; the anchoring fix you were actually after is in.5 — the scan line. You were right to ask, and the answer is that it does not fit, so it is out. I measured it rather than argued it. Rendered headless on the real dashboard via the
@astral/astralrecipe incore/CLAUDE.md:376— six roots,mediahealthy,gonescanned then unmounted,photosscanned then covered by a deny rule. The slot is 205px and.od-card > .bodyisoverflow: hidden(HomePage.svelte:56,69,grid-auto-rows: 276px).The second
.metatakes an unreadable row from 38.52px to 58.19px and silently cuts theread/write · 3 more rootsfooter. Same figures at a 376px card (1440 viewport) and a 360px card (380 viewport). My structural reasoning that the branch could not be taller was wrong: the bar is 17.39px against a meta line's 19.67px, so the readable row was never the ceiling I assumed. Commit dropped;OverviewWidget.svelteandREADME.mdare byte-identical to738f33fagain.4 — the row cap. Declining, and the measurement above is part of why.
Leaving
slice(0, 3)out is deliberate, and putting it back re-creates both halves of what this PR removes. The number would exist twice —OVERVIEW_ROOTSinfsops.ts:491and a bare3in the template — free to disagree exactly the way the three root loops did beforeprobeRoots(). And it fails silently in the direction you are worried about: raiseOVERVIEW_ROOTSto 6 and the backend probes six roots, a stat and a df each, seconds apiece on a spun-down disk, so the card can throw three away. That is bug 3 of this PR — the fourth probe nobody rendered — moved to the frontend.A slice is also not a refusal to overflow. It is the same silent truncation as the clip, minus the cue that something is misconfigured. The constraint is already stated on both sides:
fsops.ts:487-490names the slot the number has to fit, and the widget's own header comment (OverviewWidget.svelte:5-9) says the backend decides how many and why — so an edit that raises the constant reads the reason at the line it is editing. If the real worry is that nobody re-measures the slot, the thing that catches that is a rendering test, not a second constant.Separately, and not from this PR: three readable roots already measure 212px in that 205px slot and clip the footer by 7.39px — identical on
main.OVERVIEW_ROOTS = 3does not fit the slot it is named for today, which contradicts both its own doc comment andHomePage.svelte's "widgets must fit the slot — never show a scrollbar". Filed as its own issue rather than smuggled in here.Gates on
c6855f5:deno test61 passed / 0 failed,deno lintclean (33 files),deno check backend/mod.tsexit 0,svelte-check0 errors,module-builderbuilds.deno fmt --checkwants the same six hunks incontract_test.tsas at738f33f, offsets shifted only.Both blockers from my last round are closed, and I checked them the way that distinguishes a working tripwire from a disabled one rather than by re-reading the diff. Extracting
withoutProbeArgs()and running the shapes against it directly:Subtracting the allowed mentions instead of looking for them is the right shape — it is what makes the both-on-one-line case fail, and it drops the one-mention-per-line assumption with it. Blanking the argument list is a better rule than the window it replaces, and the throw means the file can no longer go silently unchecked.
Verification of your numbers, on Linux this time rather than the Windows box that gave me 12 environmental failures last round:
deno test --allow-read --allow-write --allow-run=python3 backend/— 61 passed, 0 failed (your body says 60;6490b46andc6855f5landed after you wrote it).deno lintclean, 33 files. Ondeno fmt --checkI checked the claim rather than took it: intersecting the complained-about line numbers per file with the lines this branch adds gives an empty set forcontract_test.ts,fsops.tsandroutes.ts, and the one overlap inapi.ts(103–104) is the pre-existingcancelScanline shifted down by your hunk. The claim holds.svelte-checkandmodule-buildernot rerun here.On the two nits I left last time: the widget's row cap is still only in the backend, but
the card probes the roots it draws and counts the ones configuredpins the names to["r0", "r1", "r2"], so raisingOVERVIEW_ROOTSpast what the slot fits fails a test rather than silently clipping. That is the guard I was asking for. The scan line for an unreadable root is still dropped — not mentioned either way in your description, so flagging it once more below and then leaving it.Approving. Everything below is non-blocking; the first is the one I would actually spend a commit on.
@ -59,0 +62,4 @@healthy mount. `rootInfos()` maps them for both web surfaces, and`overviewRoots()` decides how many the card draws and how many it left out — aroute deciding that counted "N more" from the rows it sent. There are testsfor each, and route files may name `policy.roots` only inside a `probeRoots()`"only inside a
probeRoots()argument list or to look one up by name" is narrower than what the test allows:policy.roots.lengthis neither, and bothroutes.tsandui.tsrely on it passing. The comment in the test gets the rule right — "asking how many there are or for one by name". Worth matching the wording here, since this line is what someone reads before the test tells them no.@ -34,0 +41,4 @@* the arguments says what the window approximated, and every other character* keeps its line and column.*/function withoutProbeArgs(source: string): string {This helper is now the load-bearing part of the tripwire, and it has no test of its own. Both route files pass by construction, so a
withoutProbeArgs()that blanked nothing would look identical in CI to the one you wrote — the same argument that made the last two holes blockers.The verification is real but it lives in a commit message. A table of half a dozen sources asserted against the helper directly — the four shapes from your description, the legitimate multi-line argument list, and the unbalanced case asserting the throw — puts it where the next edit re-runs it.
@ -34,0 +51,4 @@if (chars[i] === "(") depth++;else if (chars[i] === ")") depth--;else if (chars[i] !== "\n") chars[i] = " ";if (depth === 0) break;The unbalanced case throws, but the closes-early case is the other half of the same problem and still passes — a
)inside a string argument dropsdepthto 0 before the real closing paren, so the tail of the argument list is never blanked:Run against the helper, this reports
routes.ts:5 walks the roots itself— apolicy.rootsthat is a legitimateprobeRoots()argument, failing the test that exists to allow it.It fails safe, which is why this is not a blocker: it is a false positive, so the tripwire never goes quiet the way the unbalanced case used to. But it is a landmine for whoever writes the first
probeRoots()argument containing a paren in a string, and the message will point them at the wrong thing.The cheap version is to skip string, template and comment spans while scanning; a full one is not worth it here.
@ -459,0 +488,4 @@* — overflow is clipped, never scrolled — so the card shows this many and says* how many it left out.*/export const OVERVIEW_ROOTS = 3;OVERVIEW_ROOTSis a CSS slot width expressed as a count, and it now lives in the file whose docblock opens "Filesystem work. Everything here takes an already-resolved path".rootInfos()returning an HTTP response shape is the same drift in smaller print.Your reason is right and I would not move these back into
routes.ts: it importshonoand@opsdeck/sdk, so a bare clone cannot load it, and untestable is worse than misfiled. But a newbackend/overview.tshas only relative imports too, so it is equally reachable from the tests — and then the card's row count sits in a file named after the card. Worth it the next time something else about the overview needs a home; not worth a commit on its own.@ -70,0 +81,4 @@{:else}<!-- no bar: a 0%-full one on a root nobody can read says "empty",which is the opposite of what happened --><div class="meta why" title={root.error ?? ""}>rootInfos()still carriesscanfor a root that failed to stat, and the store may well hold a complete scan from before the mount went away — but the{:else}branch drops it, sogoneshows the error and nothing about what was last measured there. On a card whose whole job is "how full, and how old is that number", the last known size of a root that just disappeared is the more useful half.A second line under the error costs the slot height you already spend on healthy rows. Your call — it is a product decision, not a defect.