Hardening pass + one golden ratio for the whole surface - #27
Merged
archdex-art merged 8 commits intoAug 2, 2026
Conversation
… servers The bundle was broken in three independent ways and nothing caught it, because `verify.ts` only checked that directories existed: - electron-builder could not resolve the hoisted electron, so `pack:mac` failed outright. Pinned `electronVersion`. - `files: ["build/**/*"]` packed the Next standalone tree into app.asar, but `config.ts` resolves it at `process.resourcesPath/standalone` — and electron-builder silently drops nested `node_modules`, so ZERO of them shipped, including the 257MB @Anthropic-AI native binary. Now `extraResources`. - `static/` was never packaged at all, so the splash screen 404'd on boot. Lifecycle, all reachable and none covered by the existing tests: - a second `spawnServer` dropped the handle to the first child, which then held its port for the lifetime of the machine - an unhandled 'error' event from a failed spawn took down the main process instead of showing the error screen - `http.get` with no deadline never settled against a server that accepts the connection and then stalls, so `waitForHealth`'s own timeout was never re-checked - `stop()` returned before the child had exited - one crash could fan out into several live Next processes, because both the child's exit AND the health check giving up started a new attempt - a restart pending at quit spawned a server with nothing left to shut it down - the navigation guard was a prefix test, not an origin check: `http://127.0.0.1:41000@evil.com/` passed it and inherited the preload's filesystem access Verified end to end: packaged .app boots, serves, shuts down with no orphan.
fsx — `resolveSafe` let a DANGLING symlink escape the workspace. realpath fails
identically for "absent" and "dangling", so the ancestor walk fell back to the
parent (the root) and passed — but open(2) FOLLOWS a dangling symlink and creates
the file at its target. Also adds a hop budget so a symlink cycle is refused
rather than looped.
persistence — three races, all reachable on the shipped image, which runs the web
tier and apps/worker against one SQLite file:
- busy_timeout was set AFTER journal_mode, so no busy handler existed for the
pragma that needs one; and a journal_mode change never invokes it anyway. 5 of 6
concurrent boots died with "database is locked".
- runMigrations computed its pending list outside the write lock, so both
processes applied the same migration and the loser hit a UNIQUE violation.
- enqueueJob did SELECT-then-INSERT, so the loser of that race got a 500 for
exactly the double-submitted POST the idempotency key exists to absorb.
persistence — a poison job was re-leased forever: only failJob enforced
max_attempts, and a worker that dies never reports. It sorted oldest-first, ahead
of every healthy job, so the queue stopped draining behind it.
vcs — `log()` interpolated its count into an argv token, and the callers are HTTP
routes passing `Number(searchParams.get("limit"))`. `limit=-5` produced `git log
--5`, a 500 for a merely malformed query. Now clamped, which also caps an
unbounded buffer of history into memory.
Tests: the concurrency ones spawn real processes behind a start barrier; every
new test was confirmed to fail against the pre-fix source.
/api/index accepted a localPath and indexed it with no containment check, while /api/browse refused the same path outside CG_LOCAL_ACCESS_ROOT. Indexing is the STRICTLY more powerful capability — browse discloses directory names, indexing walks the tree and makes file CONTENTS readable through the repo's fs/search/ editor endpoints — so the boundary was enforced on the weaker entry point only. The check now lives in lib/localAccess.ts and both callers share it. It also stopped refusing legitimate not-yet-existing paths wherever the root crosses a symlink (every macOS /var). - SSE progress stream threw ReferenceError for any client attaching to an ALREADY-FINISHED job: finish() cleared a timer declared below it, in its TDZ. That is the normal outcome of a fast index and of every reconnect. - The trash route echoed raw ENOENT messages carrying absolute server paths, while the fs route beside it sanitises. Same policy now. - The settings save could hang forever: fetch has no default timeout and the route passed no signal, though the helper already accepted one. - OAuth returnTo was validated only where the cookie was WRITTEN. new URL() honours an absolute URL, so a cookie holding https://evil.com would redirect a freshly signed-in user off-site. Re-validated at redirect time.
historicalAnalysis -> evolutionEngine -> graphDiff -> historicalAnalysis, and evolutionEngine <-> narrativeAgent. The call chain is one-way and legitimate; what made it a cycle is that each module also had to NAME the types the others produce. Types carry no runtime edge, so hoisting the shared vocabulary to a leaf module (gitops/types.ts) breaks all three without moving a line of logic. The dependency-cruiser known-violations file is now empty, so a new cycle fails the build instead of being recorded.
`npm run boundaries` failed for anyone who had packaged the desktop app: dist-bin/ was not in SKIP_DIR_NAMES, so the checker linted Next's generated server.js and reported its process.env reads as violations.
Measured before writing any of it: the landing page alone rendered 22 distinct font sizes (12.5, 13.5, 14.5, 15.5, 16.5, 20.8, 23.2, 43.68, 49.6...), the source carried 25 across 40 components — 17 arbitrary px, seven at half-pixels — and spacing came off 20 ad-hoc rungs of Tailwind's linear ramp. Twenty-five sizes is not a hierarchy; it is noise that happens to be legible, and it accumulated one locally-defensible text-[13.5px] at a time. The ladder steps by sqrt(phi) so every SECOND step is exactly phi: pure phi (16 -> 26 -> 42) offers nothing between body copy and a section heading, which a dense instrument UI needs. Eight sizes replace twenty-five. Values are rounded to whole pixels — the ratio survives rounding, half-pixel type does not survive a hinting engine. Space uses the same progression and the same numbers. Measures are terms of one series: 272 -> 440 -> 712 -> 1152 -> 1864. Structure: - ONE page frame on every route. There were two (1152 marketing, 1440 repo), so content jumped sideways on navigation. Now .shell everywhere: content's left edge is identical on all twelve routes, it fills the viewport to the 1864 cap, and the gutter climbs the ladder with it (26 -> 42 -> 68). - The hero is a golden section (measured 634/392 = 1.618); code intelligence is inverted (1 : 1.618) because the detail pane is the primary term and an even split starved it. - A measure belongs to a SIZE, not a block: 712 is 89ch at 16px but 142ch at 10px, so prose takes the series term matched to its rung. Also fixed en route: --spacing-md shadows Tailwind's max-w-md, which resolved to 16px and pushed every page 8px wide at 768; a <select> sizing to its widest option (527px inside a 390px viewport); the mobile rail missing min-w-0 (232px of overflow); and the index page's segmented control stretching the full card instead of hugging its three labels. The report rail collapses to icons (272 -> 68, main 1134 -> 1338) with the active section still chartreuse, labels removed from the DOM rather than clipped, and the state read from localStorage with useSyncExternalStore. design-system.test.ts makes the ladder a property of the repository: 164 assertions that no arbitrary size, off-ladder step or shadowed measure comes back, and that the series is still golden. That last group caught a real 5% error in the first draft of the ladder.
Two halves of one workflow. A finding names a file and a line and then makes you
go and find them yourself, which is the one step of this the product can just do.
- Every row in the report's findings table gets an open-in-editor control linking
to `?file=…&line=…`. Disabled with an explanation when there is no live
workspace, rather than linking to an empty state.
- The editor grows an Issues panel: all findings, grouped by FILE and ordered by
their worst severity. The report already ranks by severity — that answers "what
should I look at first"; this answers "what is wrong with the file I am in", and
the file is also the unit the editor opens.
Three real bugs surfaced building it, each found by driving the thing:
- `openFile` read `tabs` from its closure and appended after an await, so two
calls for the same path both passed the "already open?" guard. StrictMode's
double-invoked effect made that reproducible: the deep link opened the file in
two tabs. Now guarded by an in-flight set and deduped inside the updater.
- The restore path replayed `openTabs` verbatim, so ONE duplicate ever written
reopened forever, and its `setTabs(restored)` clobbered a deep-link arrival that
landed mid-restore. Deduped and merged.
- Revealing the line failed in both directions. The editor is a dynamic import, so
a reveal requested on arrival happened before Monaco existed — deep links opened
the right file at line 1. And `key={activeTab.path}` remounts Monaco on every tab
switch, so a cross-file reveal landed on the dying instance — clicking an issue in
another file switched tab and stayed at line 1. The request now carries its path
and is applied by whichever instance owns it.
Verified in the browser: arriving at tasks.py?line=215 centres line 215; clicking a
correctness.js:86 issue from that same panel switches tab and centres line 86; no
duplicate tabs; no overflow at 390/768/1440.
`syntacticSpans` returns [] for anything the TypeScript parser cannot read, and
`detect.ts` reads an empty span list as "everything is code" — so `context:
["comment"]` and `context: ["code"]` were unenforced on every non-TS language.
Reproduced before fixing: a Python file whose only content is a docstring reading
"an interactive eval() is available" and a string containing "# TODO" produced TWO
findings, one of them `Use of eval()` at SEVERITY 5. The byte-identical TypeScript
produced none.
The module's own comment refuses to lex non-TS languages, on the grounds that a
wrong span suppresses a real finding. That reasoning is correct about JavaScript
and only about JavaScript — regex-versus-division, template substitution and
`rescanTemplateToken` are the hazards it names, and none exist in Python, Go,
Java, Ruby, C#, Rust, C, C++, Kotlin, Scala, Swift, Shell or PHP. Declining to lex
was not neutral; it took 100% of the risk in the other direction.
`lexicalSpans` covers those grammars; `spansFor` picks parser or lexer. A language
with no rules still returns [] and still fails open, so nothing is silently
swallowed.
Measured on real corpora, not fixtures:
- psf/requests and pallets/flask: UNCHANGED (88 -> 88). No false negatives.
- psf/black: 279 -> 275, and all four are unambiguous false positives:
- literals.py:4 — `Use of eval()`, severity 5, on the module docstring "Safely
evaluate Python string literals WITHOUT USING EVAL()". The one file in the
repo whose stated purpose is not using eval.
- test_black.py:1898,2185,2188 — `# type: ignore` inside string literals.
11 new tests assert the failure direction the module warned about: an apostrophe
in a comment must not swallow the file, an unterminated single-quoted string must
not run past its line, an escape must not close a string early, a triple-quoted
docstring is one span.
Found by the research in docs/design/DETECTION_RELIABILITY.md, which is added here
along with docs/design/COMPETITIVE_LANDSCAPE.md.
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.
Stacked on #26. Two bodies of work: a systematic bug hunt (26 confirmed defects, 2 critical) and a proportional redesign of every route.
Part 1 — Bug hunt
Baseline was green: typecheck clean, 957 tests passing, dependency-cruiser clean. So every bug below was invisible to the suite. Each fix ships with a regression test that was confirmed to fail against the pre-fix source.
Critical
resolveSafelet a dangling symlink escape the workspacerealpathfails identically for "absent" and "dangling", so the ancestor walk fell back to the root and passed — butopen(2)follows a dangling symlink and creates the file at its target/api/indexindexed any local path, ignoringCG_LOCAL_ACCESS_ROOT/api/browsehad it. Indexing is the strictly more powerful capability: browse leaks directory names, indexing makes file contents readable through the fs/search/editor endpointsHigh
ReferenceErrorfor any client attaching to an already-finished job —finish()cleared a timer declared below it, in its TDZ. That's the normal outcome of a fast index and of every reconnect.busy_timeoutwas set afterjournal_mode, and a journal-mode change never invokes the busy handler anyway. Reproduced: 5 of 6 processes died, on 3 of 3 runs.failJobenforcedmax_attempts, and a worker that dies never reports.http://127.0.0.1:41000@evil.com/passed it and inherited the preload's filesystem access.Medium / Low
Idempotency race returning a 500 for the exact double-submit the key exists to absorb ·
git log?limit=-5→git log --5→ 500, plus an unbounded history buffer · trash route echoing absolute server paths · settings save able to hang forever (fetchhas no default timeout and the route passed no signal) · OAuthreturnTovalidated only where written · three dependency-cruiser cycles (allowlist now empty) ·npm run boundariesfailing for anyone who had packaged the desktop app · refs read during render in two graph components (visible symptom: the grabbing cursor never appeared on mousedown) · stuck search spinner · stale-response races in Git and Trash panels.Desktop packaging
pack:macfailed outright, and once forced through the bundle was broken three ways — the Next standalone tree packed where the runtime never looks, zero of itsnode_modulesshipped (electron-builder drops nested ones silently, including the 257 MB native binary), andstatic/was never packaged so the splash 404'd. Plus: double-spawn leaking a child, unhandledspawnerror killing the main process, an unbounded health probe,stop()returning before exit, one crash fanning out into several live servers, and a restart firing after quit.Verified end to end: packaged
.appboots → serves → shuts down with no orphan.Part 2 — The proportional system
1:1φ = 1.618, base 16. The ladder steps by √φ so every second step is exactly φ — pure φ (16 → 26 → 42) offers nothing between body copy and a section heading, which a dense instrument UI needs. Rounded to whole pixels: the ratio survives rounding, half-pixel type does not survive a hinting engine.
Space uses the same progression and the same numbers. Measures are terms of one series:
272 → 440 → 712 → 1152 → 1864.Structure
.shell) — content's left edge now identical on all twelve, fills the viewport to the 1864 cap, gutter climbs the ladder with it (26 → 42 → 68).1 : 1.618because the detail pane is the primary term.useSyncExternalStore.The "Instrument" identity is untouched — not one colour, font or animation changed. This is the geometry underneath it.
Found by doing this:
--spacing-mdshadows Tailwind'smax-w-md, which resolved to 16px and pushed every page 8px wide at 768. And the guard test caught a genuine 5% error in my own ladder (20/13 = 1.538), which is why it asserts the ratio and not just token presence.Verification
design-system.test.ts: 164 assertions that the ladder cannot grow backdocs/design/PROPORTION.mdKnown gaps
Three audit slices never completed (
core-graph/imports/analysisReDoS + SARIF off-by-one;config/observability/worker/cli; CI workflows) — those areas are un-audited, not clean. Desktop remains unsigned/un-notarized with no update feed.