mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
Garry TanandClaude Fable 5.1
444f8feff8
Review army + adversarial passes (Claude and Codex) on the merged branch:
setup
- _prune_stale_generated scans the host dirs too (the generator already
removed the render before setup ran, so the host branch was dead), skips
symlinks in the render tree (rm -rf on a slash-terminated link empties its
target), removes a host symlink only when it resolves into gstack, cleans a
bannered real dir through _cleanup_weak_dir, recognizes frontmatter-renamed
skills, and logs through log. The always-run codex render passes every host
dir that may link to it.
- NEEDS_BUILD checks all three binaries (with $_EXE) and lib/ sources; the
browser hint and the bootstrap summary honor GSTACK_SKIP_ASIDE, treat a
requested skip as a request, and derive one skill list.
lib/aside-render.ts + bin/gstack-render.ts
- The loopback server carries a per-render secret path, checks containment on
the real path (symlink escapes are 403), and rejects malformed encoding.
- Inline eval results are one base64 line, so page text cannot forge
ASIDE_DIR= or the sentinel; the last ASIDE_DIR wins.
- runProc escalates SIGTERM to SIGKILL, bounds every wait, and clears every
timer (an uncleared one kept gstack-render alive after printing OK).
- renderTmpDir refuses a shared /tmp name owned by someone else; the work dir
and server are created inside try; goto's budget follows the render budget.
- probeAside classifies a present-but-failing CLI as ASIDE_NOT_RUNNING like
the skills' bash probe; render() retries on gstack's own browser when Aside
could not start or its private CDP bridge is gone (never on a page error
or a timeout of a running script); the CLI reports the engine that actually
rendered, exits 0 on --help, rejects non-numeric flags, documents
--wait-timeout, fences EVAL/PAGE_ERRORS as untrusted content, and names the
daemon's cookie-import JS lock remedy.
- The browse path passes --scale only when asked (a scale change rebuilds
the daemon context) and restores the viewport after a sized screenshot.
resolvers / templates
- The bash probe honors GSTACK_SKIP_ASIDE and has a perl deadline on stock
macOS; .local is no longer LOCAL (mDNS); same-origin filters compare parsed
origins; link status is HEAD-checked only on LOCAL targets; every
aside exec goes through the receipted _aside_exec prelude
({{ASIDE_EXEC_PRELUDE}}), including nine template blocks that called it
bare; the design sketch and diagram staging use private directories.
- The generator prunes only bannered renders and never a host whose
generation failed.
Docs, stale comments and dead code cleaned; goldens re-rendered; tests
updated and added for every behavior above.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
48 lines
2.5 KiB
Markdown
48 lines
2.5 KiB
Markdown
# Slop-scan: what to fix, what to leave
|
|
|
|
Moved verbatim from CLAUDE.md (token-load reduction). Read before acting
|
|
on any slop-scan finding.
|
|
|
|
### What to fix (genuine quality improvements)
|
|
|
|
- **Empty catches around file ops** — use `safeUnlink()` (ignores ENOENT, rethrows
|
|
EPERM/EIO). A swallowed EPERM in cleanup means silent data loss.
|
|
- **Empty catches around process kills** — use `safeKill()` (ignores ESRCH, rethrows
|
|
EPERM). A swallowed EPERM means you think you killed something you didn't.
|
|
- **Redundant `return await`** — remove when there's no enclosing try block. Saves a
|
|
microtask, signals intent.
|
|
- **Typed exception catches** — `catch (err) { if (!(err instanceof TypeError)) throw err }`
|
|
is genuinely better than `catch {}` when the try block does URL parsing or DOM work.
|
|
You know what error you expect, so say so.
|
|
|
|
### What NOT to fix (linter gaming, not quality)
|
|
|
|
- **String-matching on error messages** — `err.message.includes('closed')` is brittle.
|
|
Playwright/Chrome can change wording anytime. If a fire-and-forget operation can fail
|
|
for ANY reason and you don't care, `catch {}` is the correct pattern.
|
|
- **Adding comments to exempt pass-through wrappers** — "alias for active session" above
|
|
a method just to trip slop-scan's exemption rule is noise, not documentation.
|
|
- **Converting extension catch-and-log to selective rethrow** — Chrome extensions crash
|
|
entirely on uncaught errors. If the catch logs and continues, that IS the right pattern
|
|
for extension code. Don't make it throw.
|
|
- **Tightening best-effort cleanup paths** — shutdown, emergency cleanup, and disconnect
|
|
code should use `safeUnlinkQuiet()` (swallows ALL errors). A cleanup path that throws
|
|
on EPERM means the rest of cleanup doesn't run. That's worse.
|
|
|
|
### Utilities in `lib/error-handling.ts` (re-exported from `browse/src/error-handling.ts`)
|
|
|
|
| Function | Use when | Behavior |
|
|
|----------|----------|----------|
|
|
| `safeUnlink(path)` | Normal file deletion | Ignores ENOENT, rethrows others |
|
|
| `safeUnlinkQuiet(path)` | Shutdown/emergency cleanup | Swallows all errors |
|
|
| `safeKill(pid, signal)` | Sending signals | Ignores ESRCH, rethrows others |
|
|
| `isProcessAlive(pid)` | Boolean process checks | Returns true/false, never throws |
|
|
|
|
### Score tracking
|
|
|
|
Baseline (2026-04-09, before cleanup): 100 findings, 432.8 score, 2.38 score/file.
|
|
After cleanup: 90 findings, 358.1 score, 1.96 score/file.
|
|
|
|
Don't chase the number. Fix patterns that represent actual code quality problems.
|
|
Accept findings where the "sloppy" pattern is the correct engineering choice.
|