Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions corpus/skills/principle-explicit-errors/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down Expand Up @@ -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."
<https://peps.python.org/pep-0020/>; Joshua Bloch, *Effective Java* 3rd ed.
(2018), Item 77 "Don't ignore exceptions". A batch job that swallows an
should never pass silently." <https://peps.python.org/pep-0020/>. 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) <https://martinfowler.com/ieeeSoftware/failFast.pdf>. Shore's
Expand Down
10 changes: 7 additions & 3 deletions corpus/skills/principle-explicit-errors/tests/fires_example.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
28 changes: 28 additions & 0 deletions corpus/skills/principle-explicit-errors/tests/test_void_discard.py
Original file line number Diff line number Diff line change
@@ -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()
11 changes: 6 additions & 5 deletions engine/hooks/explicit-failures/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 - <<EOF`) runs both.

A hit is suppressed when the block contains any of `log`, `raise`, `throw`,
`warn`, `print(`, `status`, or `reason` (so a status row or a log line with
context already satisfies it), or when the substring `explicit-failures`
A hit is suppressed when the block contains any of `log`, `console.error`,
`raise`, `throw`, `warn`, `print(`, `status`, or `reason` (so a status row
or a log line with context already satisfies it), or when the substring `explicit-failures`
appears on the header line, the line before it, or inside the block:
`# explicit-failures: allow`, `# pragma: explicit-failures: allow`,
`// eslint-disable-next-line explicit-failures`.
Expand Down
13 changes: 9 additions & 4 deletions engine/hooks/explicit-failures/detect.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
guard whose last statement is such an exit. In both cases the block must
contain nothing that says the failure happened: no log, raise, warn, print,
status or reason assignment. JS/TS shapes: `catch {}` with an empty or
comment-only body (or 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
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`).
Expand All @@ -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.*)?$")
Expand Down Expand Up @@ -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):
Expand Down
11 changes: 11 additions & 0 deletions engine/hooks/explicit-failures/tests/test_hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading