diff --git a/corpus/skills/principle-explicit-errors/SKILL.md b/corpus/skills/principle-explicit-errors/SKILL.md index a655a0aaf..e50fd4dfb 100644 --- a/corpus/skills/principle-explicit-errors/SKILL.md +++ b/corpus/skills/principle-explicit-errors/SKILL.md @@ -7,16 +7,21 @@ disable-model-invocation: true # Make errors and failures explicit Every failure path must make its disposition visible: fail fast, propagate, -translate into a named domain result, or suppress only a narrow, documented -case whose absence is safe and observable when that assumption changes. +translate into a named domain result, or log the exception with its context. +Never suppress an exception. Silence is not a policy. Each rule below restates standard practice (see Grounding). ## Exceptions -An empty or comments-only handler is not an error policy. It hides failures -from callers, logs, tests, and maintainers. Do not write `catch {}`, -`except: pass`, ignored promise rejections, or equivalent silent fallbacks. -Replace them with an explicit action and keep the original error context. +An empty handler, a comment-only handler, and a handler whose only action +is to discard the error (`catch {}`, `void err`, `except: pass`, +`.catch(() => {})`) hide the failure from callers, logs, tests, and +maintainers. Do not write them. + +If the exception must not propagate — a diagnostic callback, a metric +write, a best-effort side path — log it, then continue. The log names the +operation and includes the original exception. A comment that says why you +caught it is not a log. `void err` is not a log. Mechanical enforcement: `engine/hooks/explicit-failures` (advisory PreToolUse hook, on by default; its README lists the shapes it catches). @@ -85,9 +90,12 @@ The repo context is a batch data pipeline (filings in, CSV grids out), not a long-running service; each line says how the source fits that. - Silent handlers → Tim Peters, PEP 20 "The Zen of Python" (2004): "Errors - should never pass silently. Unless explicitly silenced." - ; Joshua Bloch, *Effective Java* 3rd ed. - (2018), Item 77 "Don't ignore exceptions". A batch job that swallows an + should never pass silently." . This + skill does not take the next sentence ("Unless explicitly silenced") as + permission to drop an exception. Joshua Bloch, *Effective Java* 3rd ed. + (2018), Item 77 "Don't ignore exceptions". When the exception must not + propagate, log it with the original exception attached. A comment, an + empty catch, or `void err` is not that log. A batch job that swallows an error ships a wrong CSV under a green run, so this applies as written. - Fail immediately and visibly → Jim Shore, "Fail Fast", *IEEE Software* 21(5) (2004) . Shore's diff --git a/corpus/skills/principle-explicit-errors/tests/fires_example.md b/corpus/skills/principle-explicit-errors/tests/fires_example.md index 5f9bd33ec..a092a7a2c 100644 --- a/corpus/skills/principle-explicit-errors/tests/fires_example.md +++ b/corpus/skills/principle-explicit-errors/tests/fires_example.md @@ -1,7 +1,11 @@ An agent is adding a fallback around an optional dependency and proposes -`try { loadOptionalModule() } catch {}`. The agent invokes -`/principle-explicit-errors`, names the expected import failure, and replaces -the silent handler with an explicit fallback plus a test for unexpected errors. +`try { loadOptionalModule() } catch {}`. A review bot then "fixes" a +diagnostic callback by replacing `void reportingFailure` with an empty +`catch {}` so the callback cannot change the caller's outcome. The agent +invokes `/principle-explicit-errors`: neither shape is allowed. If the +exception must not propagate, the handler logs the operation and the +original exception, and a test asserts that log. An empty catch, a +comment-only catch, and `void err` are all suppression. A second shape, same skill, no exception in sight. A realized-gains tab passes "every value traces to a source", then a review pass finds a sell diff --git a/corpus/skills/principle-explicit-errors/tests/test_void_discard.py b/corpus/skills/principle-explicit-errors/tests/test_void_discard.py new file mode 100644 index 000000000..6b15c7c8d --- /dev/null +++ b/corpus/skills/principle-explicit-errors/tests/test_void_discard.py @@ -0,0 +1,28 @@ +"""The skill's discard rule is the same check the failure hook runs.""" +from __future__ import annotations + +import os +import sys +import unittest + +HOOK_DIR = os.path.abspath( + os.path.join(os.path.dirname(__file__), "..", "..", "..", "..", "engine", "hooks", "explicit-failures") +) +sys.path.insert(0, HOOK_DIR) + +import detect # noqa: E402 + + +class VoidDiscardTest(unittest.TestCase): + def test_void_only_catch_is_reported(self): + text = "try { a() } catch (err) {\n void err;\n}\n" + hits = detect.scan_js(text) + self.assertEqual(hits, [(1, "catch block that only discards the error with `void`")]) + + def test_logged_catch_is_quiet(self): + text = "try { a() } catch (err) {\n console.error('failed', err);\n}\n" + self.assertEqual(detect.scan_js(text), []) + + +if __name__ == "__main__": + unittest.main() diff --git a/engine/hooks/explicit-failures/README.md b/engine/hooks/explicit-failures/README.md index f409fa0c8..2159bf941 100644 --- a/engine/hooks/explicit-failures/README.md +++ b/engine/hooks/explicit-failures/README.md @@ -22,8 +22,9 @@ Python (`.py`): JS/TS (`.js .jsx .ts .tsx .mjs .cjs .vue .svelte`): -- `catch {}` / `catch (e) {}` with an empty or comment-only body, or a body - that only `continue`s / `break`s / returns bare. +- `catch {}` / `catch (e) {}` with an empty or comment-only body, a body + whose only statements are `void err`, or a body that only `continue`s / + `break`s / returns bare. - `.catch(() => {})`, `.catch(e => null)`, `.catch(function () {})`. - `if (!x)` / `if (x == null)` / `if (x === undefined)` / `if (x.length === 0)` whose only statement is `continue`, `break`, or a bare / `null` / `[]` return. @@ -32,9 +33,9 @@ Bash: every heredoc body in the command is scanned; the redirect target (`cat > build.py <<'EOF'`) picks the grammar, a heredoc with no target (`python3 - < {})` no-op, and an `if (!x)` / `if (x == null)` guard whose -only statement is a bare exit. The substring `explicit-failures` on the +comment-only body, a body whose only statements are `void err`, a body that +only continues / returns bare, a `.catch(() => {})` no-op, and an +`if (!x)` / `if (x == null)` guard whose only statement is a bare exit. The substring `explicit-failures` on the header line, the line before it, or inside the block suppresses a hit (`# explicit-failures: allow`, `# pragma: explicit-failures: allow`, `// eslint-disable-next-line explicit-failures`). @@ -35,7 +35,10 @@ PY_SUFFIXES = (".py", ".pyi") JS_SUFFIXES = (".js", ".jsx", ".ts", ".tsx", ".mjs", ".cjs", ".vue", ".svelte") -ALLOW_TOKEN_RE = re.compile(r"(?i)(?:\blog|_log\b|\braise\b|\bthrow\b|status|reason|warn|\bprint\s*\()") +ALLOW_TOKEN_RE = re.compile( + r"(?i)(?:\blog|_log\b|\braise\b|\bthrow\b|status|reason|warn|\bprint\s*\(|console\.error)" +) +JS_VOID_ONLY_RE = re.compile(r"(?:void\s+\S+\s*;\s*)*void\s+\S+") ALLOW_MARK = "explicit-failures" PY_EXCEPT_RE = re.compile(r"^(\s*)except\b[^:]*:\s*(\S.*)?$") @@ -189,6 +192,8 @@ def scan_js(text: str) -> list[tuple[int, str]]: continue if stripped == "": hits.append((_line_of(text, m.start()), "`catch {}` with an empty body")) + elif JS_VOID_ONLY_RE.fullmatch(stripped) and not ALLOW_TOKEN_RE.search(body): + hits.append((_line_of(text, m.start()), "catch block that only discards the error with `void`")) elif re.fullmatch(JS_EXIT, stripped) and not ALLOW_TOKEN_RE.search(body): hits.append((_line_of(text, m.start()), f"catch block that only `{stripped}`s")) for m in JS_PROMISE_CATCH_RE.finditer(text): diff --git a/engine/hooks/explicit-failures/tests/test_hooks.py b/engine/hooks/explicit-failures/tests/test_hooks.py index 141152893..f07c57007 100644 --- a/engine/hooks/explicit-failures/tests/test_hooks.py +++ b/engine/hooks/explicit-failures/tests/test_hooks.py @@ -108,6 +108,17 @@ def test_fires_on_js_comment_only_catch_body(self): hits = detect.scan_js(text) self.assertEqual(hits, [(1, "`catch {}` with an empty body")]) + def test_fires_on_void_discard_in_catch(self): + text = "try { a() } catch (reportingFailure) {\n void reportingFailure;\n}\n" + hits = detect.scan_js(text) + self.assertEqual(hits, [(1, "catch block that only discards the error with `void`")]) + + def test_silent_on_console_error_and_void_console_error(self): + logged = "try { a() } catch (e) {\n console.error('metric failed', e);\n}\n" + discarded_return = "try { a() } catch (e) {\n void console.error('metric failed', e);\n}\n" + self.assertEqual(detect.scan_js(logged), []) + self.assertEqual(detect.scan_js(discarded_return), []) + def test_fires_on_promise_catch_noop_fixture(self): hits = lines_for(write_payload("promise_catch_noop_fires.js")) self.assertEqual([h.split(": ", 1)[1].split(" — ")[0] for h in hits], ["`.catch(() => {})` no-op rejection handler"] * 2)