Audit fixes and dependency upgrade (ink 7, React 19, js-yaml 5, TS 7) #1
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/audit-and-dependency-upgrade"
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?
Audit of kPong plus a full dependency upgrade, focused on stability, UI/UX, user flow, security and maintainability.
Findings came from a 65-agent audit across six dimensions, each finding then attacked by an adversarial verifier: 46 confirmed, 8 refuted. Everything below was reproduced before being fixed.
Dependencies
ink 5.2.1 → 7.1.1, react 18.3.1 → 19.2.8, js-yaml 4.1.1 → 5.2.2, TypeScript 5.9.3 → 7.0.2, knip 5.88.1 → 6.29.0, @types/react → 19.2.17.
bun auditgoes from 4 vulnerabilities (2 high) to 0.Three upgrades had traps that typechecking could not catch, so each was verified by running the app:
import yaml from "js-yaml"throws at startup, andallowSyntheticDefaultImportshides it fromtsc— a silent total boot failure. Converted to named imports.0x7fasbackspace, notdelete. The existing "Delete falls back to Backspace at end-of-line" workaround therefore became an active bug that deleted the wrong character.interactive: truepreserves the previous behaviour.TypeScript 7 ships the CLI only — no
tsserver, no compiler API. knip 5 crashes on it, so knip 6 lands in the same commit. Thepicomatch/smol-toml/yamloverrides pinned knip transitives below what knip 6 requires and are removed;wsis pinned to 8.21.1 because ink's own range does not reach the patched version.Data loss
An unreadable
kpong.yamlleftconfignull, which fell through to the first-run wizard — and completing that wizard overwrote the file the user needed to repair and ranreplaceAllTargets, whose first statement isDELETE FROM targets. A YAML typo cost the user every stored target.A config that exists but will not load is now a repair situation: a recovery view shows the path, the parse error, and that stored targets are intact, and the existing file watcher clears it as soon as the file is fixed. The wizard additionally backs up any config it is about to replace and seeds targets only into an empty table.
Also:
parseConfigTextno longer coerces a non-mapping document to{}(an empty file, a bare scalar and a sequence each produced a complete default config); the legacy import no longer writeskpong.yamlinside a SQLite transaction that cannot roll it back; config writes go through a temp file and rename; the config read is bounded and must be a regular file; legacy targets with noenabled:key no longer import as paused;deleteTargetis atomic and bulk replacement purges orphaned samples.Stability
pingHosttrusted anymstoken regardless of exit code, and iputils prints100% packet loss, time 0ms. Now gated on exit code plus a locale-independent100%check.maxConcurrentPings. Ticks past the cap returned without queueing, and interval timers stay phase-locked, so the same trailing targets starved forever. Reproduced with 6 targets and a cap of 2: only two were ever probed. Now a deferral queue plus a start stagger. The in-flight counter also leaked, because the decrement sat behind a generation guard.initializeDatabaseran before the check and itsON CONFLICT DO UPDATEstamped a newer version back down to 1.busy_timeout; failed sample writes requeue instead of being dropped; the scheduler no longer restarts on UI-only config edits; uncaught errors unmount cleanly instead of leaving the terminal in raw mode.UI and UX
unreachablerendered in the muted "unknown" colour and as "Unknown" in the details pane — identical to a never-probed target.Maintainability
App.tsxdrops from 1589 to ~1030 lines: the five keyboard controllers and the settings-field builder move to their own modules.AlertService.evaluateno longer both mutates its argument and returns a value. OnedescribeErrorconvention replaces tenString(error)calls and oneerror.message. A duplicated watch helper is removed and the copy that is actually called is now the one under test.Quality gates
oxlint + oxfmt added;
bun run check= typecheck + lint + format check + knip + tests, and CI runs the same plusbun audit. All 45 lint findings are fixed rather than silenced, except where the rule is wrong for this code — the control-character regexes are the theme sanitisation, and sixexhaustive-depsreports are mount-only effects or the scheduler effect whose purpose is to not re-subscribe. Each suppression carries its reason at the site.Formatting is a separate commit. One trap worth flagging for reviewers: oxfmt collapses runs of literal whitespace in JSX text, and in a terminal UI those runs are layout. Every load-bearing space now lives in a string expression; verified by diffing rendered frames before and after, byte-identical for both the empty and populated views.
Repo hygiene
.gitignorenow covers any*.dbplus WAL sidecars.dist/is cleared before a build — it was holding a 3.6 MBkpong.dbwith real monitored hostnames next to the executable, which is the directory a maintainer would zip. The Windows icon is now actually applied, via a separate script because Bun rejects the flag on non-Windows targets. Forgejo CI added.scripts/purge-db-from-history.shis provided unrun, for thekpong.dbblob still reachable in history frommain. It rewrites history and needs a force-push, so it is deliberately a manual step.Verification
96 tests pass (was 64 passing / 2 failing), typecheck clean, lint clean, format clean, knip clean,
bun auditclean, the compiled binary builds and runs.Not covered by tests: the keyboard paths, since nothing drives
useInput. A manual pass on Backspace/Delete mid-string and at end-of-line, and Esc in each mode, is worth doing on Windows Terminal.An unreadable kpong.yaml left config null, which fell through to the first-run wizard. Finishing that wizard overwrote the file the user needed to repair and ran replaceAllTargets, whose first statement is DELETE FROM targets. A YAML typo therefore cost the user every stored target. A config that exists but will not load is now a repair situation: a recovery view shows the path, the parse error, and that stored targets are intact, and the existing file watcher clears it as soon as the file is fixed. The wizard additionally backs up any config it is about to replace and seeds targets only into an empty table. parseConfigText no longer coerces a non-mapping document to {}: an empty file, a bare scalar and a sequence each produced a complete default config, so a damaged file was indistinguishable from a healthy one. Also in this area: legacy targets with no `enabled:` key imported as paused because undefined coerced to 0; the legacy import wrote kpong.yaml inside a SQLite transaction that could not roll it back; config writes now go through a temp file and rename; the config read is bounded and must be a regular file; deleteTarget is atomic and bulk replacement purges orphaned samples; CSV text columns are guarded against spreadsheet formula injection; isCompiledBinary compares the executable name rather than searching the whole path for "bun"; and Windows desktop alerts use a self-disposing balloon instead of a modal MessageBox, capped at three pending helpers.App.tsx owned its own state plus five keyboard controllers and a 200-line settings-field builder, none of which touched that state. Those move to inputControllers.tsx and settingsFields.ts; the shared SettingsField type moves to appSupport.ts so neither has to import from App and form a cycle. App.tsx drops from 1589 to ~1030 lines with no behaviour change. AlertService.evaluate both mutated its argument and returned a value, so callers had to read both in the right order to be correct. It now returns { event, spikeActive } and takes a Readonly state. Error notices used String(error) in ten places and error.message in one, so the same failure rendered as "Error: ENOENT ..." or "ENOENT ..." depending on which path produced it. One describeError helper now covers all of them. appSupport.ts carried a copy of shouldHandleWatchEvent identical to the private one in fileReloadWatcher.ts, and the test exercised the copy rather than the function actually called at the watch site. The real one is now exported and under test. reconcileStatesWithConfig and validateHostInput had no callers outside their own tests. Repo hygiene: .gitignore now covers any *.db plus the WAL sidecars this change introduces, since getAppDirectory falls back to cwd in dev and a database with real hostnames was committed once before. `dist/` is cleared before a build — it was holding a 3.6 MB kpong.db and a kpong.yaml next to the executable, which is what a maintainer would zip. The Windows icon is now actually applied, via a separate script because Bun rejects the flag on non-Windows targets, and .images is tracked so a fresh clone can build. Added a Forgejo CI workflow matching the remote, and corrected the README's claim that the devtools shim exists because Ink declares an optional peer. scripts/purge-db-from-history.sh is provided, unrun, for the committed database still reachable in history. It rewrites history and needs a force-push, so it is deliberately a manual step.Formatting only; no behaviour change. Kept as its own commit so later diffs are not buried in reflow. printWidth is 110 rather than the default 80 because that is where this code already sits (99th percentile line length was 112), so the reflow stays close to the author's existing shape. One thing needed fixing first: oxfmt collapses runs of literal whitespace in JSX text, and in a terminal UI those runs are layout. The empty-state indents (" No targets configured yet."), the double-space separators in the settings key hints, and the trailing spaces between adjacent Text nodes in the remove-confirm card are now written as string expressions, which no formatter will touch. Verified by diffing the rendered frames before and after: byte-identical for both the empty and populated views.Gate is oxlint's correctness + suspicious categories as errors; pedantic was left off after measuring it at 219 findings of mostly style preference against 36 actionable ones. Fixed, not silenced: - 25 redundant `?? {}` fallbacks in object spreads (spreading undefined is already a no-op) - two `throw new Error(...)` in the config parser that discarded the original failure; they now pass `{ cause }` - dead `normalizeCursorState` in textInput - `loadStoredTarget` and `numberField` were redeclared on every call despite capturing nothing; both are now module-scope - the notifications `useMemo` was keyed on a bump counter while reading a mutable service, so it could only ever go stale. getFeed() is a six-element slice, so it is read directly and the counter is now explicitly write-only. Annotated where the lint rule is wrong for this code, with the reason at the site: the two control-character regexes are the theme-file sanitisation, and six exhaustive-deps reports are mount-only effects or the scheduler effect whose whole purpose is to not re-subscribe on identity changes. Adding those dependencies would restart every probe on every render. react-in-jsx-scope is off because tsconfig uses the automatic JSX runtime. oxlint has no no-restricted-syntax, so AGENTS.md's "colours come from ResolvedTheme" rule is enforced by tests/conventions.test.ts instead, which also runs in CI. Verified it fails on an injected literal rather than passing vacuously. The autofix introduced toSorted/toReversed, which need lib ES2023 — caught by typecheck, not by the linter. Target moves to ES2024, which Bun implements in full and which also allows Promise.withResolvers in the scheduler test.kReview review
Verdict: no findings
No findings to address in the reviewed diff.
Overall risk is low; no changed line demonstrates a concrete correctness, security, or data-loss regression with the available evidence.
Excluded as generated or vendored (not reviewed):
bun.lock,vendor/react-devtools-core-shim/index.cjs,vendor/react-devtools-core-shim/index.js.Reviewed by kReview at
fedc1276e5. This comment is conservative and based only on the PR diff, metadata, and supplied repository context.Est. cost ~$31.61 total (9.3M in / 115.2k out) · this run ~$0.10 (128.4k in / 1.1k out) / gpt-5.6-sol.
Rejecting every zero latency was wrong in the other direction: some ping implementations report exactly 0 for a genuinely fast reply, and that target would have been recorded as unknown. The real distinction is not the value but where it came from — a per-reply time field or a statistics summary. extractReplyLatencyMs now requires the separator to sit directly against the number, which is how every implementation formats a reply time ("time=23ms", "Zeit=23ms", "time<1ms", "time=0.000 ms") and how none of them format a summary ("Minimum = 0ms", "Mittelwert = 0ms", "rtt min/avg/max = 0.0/…", iputils' trailing "time 0ms"). That is a property of the ping binary's format strings rather than its translations, so it holds for localized output — which is what the localized Windows unreachable reply needed, since it exits 0, reports 0% loss, and matches no English marker. A genuine "time=0.000 ms" is now accepted again. The write backoff only guarded against duplicate timers, so every 50 ms flush still called straight through to appendSamples while a database was unwritable. The next permitted attempt time is now checked by the attempt itself, so all callers honour it. Config saves preserve the original file's permission bits: openSync creates the temp file at 0666 minus umask and the rename replaces the inode, so a 0600 config silently became 0644 on every save.Two problems in the build scripts. The prebuild step ran `rmSync('dist', {recursive: true})`. A compiled binary resolves its runtime paths from its own directory, so anyone who runs dist/kpong keeps their kpong.yaml, kpong.db and exports/ in dist/ — and the next build erased all of it. I had added that step to keep build output clean and did not consider that the directory holds live data. It now removes only names the build itself produces. `build:all` also cross-compiled to Windows with --windows-icon. Bun documents that the Windows metadata flags depend on Windows APIs and cannot be used when cross-compiling, so the whole all-platform build failed on its first command on any Linux or macOS host. The icon now lives in a separate Windows-host-only script and build:all uses the icon-free Windows target.