Repository navigation
Fail the build on a bare catch, and say why each one is empty - #210
Merged
Merged
Conversation
A catch clause whose block holds neither a statement nor a comment swallows
the error with nothing on record saying why that is safe. check-empty-catch
in .github/scripts/ flags them (statement form only; the void
p.catch(() => {}) idiom and test-helper retries stay out of scope); its test
runs in the package job of tests-ok and in npm run verify. The sixteen bare
catches in core and plugins/social now name their reason, and the rule is
recorded in docs/code-standards.md.
Fixes #205
Docs-not-needed: comment-only edits to owned sources; the rule and its gate live in docs/code-standards.md and .github/scripts/
The binding pattern now matches any catch(...) argument, so catch ({ message }) {}
and catch ({}) {} cannot evade the gate. The hooks.js comment also covers a
resolveAgent failure, not only an ended session.
Docs-not-needed: comment-only edit to an owned source; no behavior change
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
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.
Fixes #205
A
catchclause whose block holds neither a statement nor a comment swallows the error with nothing on record saying why that is safe. This addscheck-empty-catchto.github/scripts/(statement form only; thevoid p.catch(() => {})idiom and test-helper retries stay out of scope, matching the issue's non-goals) with a test that asserts the shipped sources stay clean — thepackagejob oftests-okand rootnpm run verifyalready run it, so no workflow change. The sixteen bare catches it found incore/andplugins/socialnow carry their reason as a comment; the rule is recorded in docs/code-standards.md ("Silent catches").src/host/home-lock.jsis an upstream file edited for the first time, so it joins the0002-flybot-api-hosttopic; the client bundle is byte-identical (esbuild strips comments), and the patches were regenerated.Review follow-ups landed in 59e0bc9: the gate's binding pattern now matches any
catch(...)argument, so destructuring bindings (catch ({ message }) {},catch ({}) {}) cannot evade it, and the hooks.js comment covers a resolve failure, not only an ended session. Remaining review notes are filed as #213.Fail without change
At base (
origin/maina63309f) plus only the two new script files:node .github/scripts/check-empty-catch.mjsexits 1 with 16 violations (core/index.js:71, src/client/index.js:50, src/client/flybot-registry.js:99, src/host/diagnostics.js:108,110, src/host/home-lock.js:53,83,92, src/host/hooks.js:819, plugins/social/index.js:35,37, plugins/social/client.js:900, plugins/social/src/social.js:297,392,1100, plugins/social/src/mcp-client.js:148). At the tip the gate and its tree test pass.Verification
Tip SHA:
59e0bc98c3656de113e756d554ce9b774a738f7aRan (at the tip, worktree
/tmp/flybot-205):npm run buildincore/→ pass (client.js byte-identical: comment-only src/client edits)node scripts/ds-bot-patches.mjs generate+check→ pass ("9 patches apply cleanly in order", "core/ equals ds-bot/ + patches/ds-bot/")TZ=UTC ARCH_DOCS_BASE=origin/main npm run verify→ exit 0 at 47745ad (core 750/750, social 160/160, tree 56/56, repo scripts 60/60); at 59e0bc9 the same suites were run stepwise with the same results — core 750/750, social 160/160 (settled after the known flake below), tree 56/56, repo scripts 60/60, oxlint 28 warnings / 0 errors (same set as base), arch-docs OK (19 changed files,Docs-not-needed:trailers — comment-only edits to owned sources)node .github/scripts/check-empty-catch.mjs→ exit 0, no violationsNot run:
src/host/changed (comments only, no behavior change) but the smoke bench needs a../deepseek-harnesscheckout this machine does not have.Environment notes for the reviewer
TZ=UTC: three social face-room title tests format creation times and fail in a non-UTC local timezone — they fail identically at base.plugins/social#90 hidden Bot re-shareis an intermittent local flake (createBot → resolveModel, "Available models: none"): it failed roughly 1 run in 3 at base as well as at the tips, more often under load; re-running the suite settles it. CI's fresh runner is the arbiter.npm run harnessincore/(577 packages) beforenpm test/npm run lintresolve@deepseek-ai/*.