diff --git a/docs/README.skills.md b/docs/README.skills.md index ba1bcafd1..491c57ff9 100644 --- a/docs/README.skills.md +++ b/docs/README.skills.md @@ -312,7 +312,11 @@ See [CONTRIBUTING.md](../CONTRIBUTING.md#adding-skills) for guidelines on how to | [playwright-automation-fill-in-form](../skills/playwright-automation-fill-in-form/SKILL.md)
`gh skills install github/awesome-copilot playwright-automation-fill-in-form` | Automate filling in a form using Playwright MCP | None | | [playwright-explore-website](../skills/playwright-explore-website/SKILL.md)
`gh skills install github/awesome-copilot playwright-explore-website` | Website exploration for testing using Playwright MCP | None | | [playwright-generate-test](../skills/playwright-generate-test/SKILL.md)
`gh skills install github/awesome-copilot playwright-generate-test` | Generate a Playwright test based on a scenario using Playwright MCP | None | -| [poka-yoke](../skills/poka-yoke/SKILL.md)
`gh skills install github/awesome-copilot poka-yoke` | Mistake-proof code so misuse cannot be expressed, rather than warning against it. Use when designing an interface, schema, or state machine and the user wants it hard to get wrong ("make invalid states unrepresentable", "so callers cannot screw it up", "type-safe API", "pit of success"); when auditing existing code for footguns ("what could bite us here", "what is easy to misuse", "poka-yoke this repo", "review this diff for ways to get it wrong"); or when a bug has recurred and the fix must close the class rather than the case ("make sure this never happens again", "this is the third time"). Especially for money, auth, permissions, deletion, migrations, and pipelines where failure is silent. Classifies every finding by what happens when the mistake occurs and how the device notices, which is what keeps it from collapsing into generic code review. | `references/hazard-catalog.md`
`references/lang-python.md`
`references/lang-rust-go.md`
`references/lang-typescript.md`
`scripts/detect_hazards.py` | +| [poka-yoke](../skills/poka-yoke/SKILL.md)
`gh skills install github/awesome-copilot poka-yoke` | Mistake-proof code so misuse cannot be expressed, rather than warning against it. Use when designing an interface, schema or state machine and the user wants it hard to get wrong ("make invalid states unrepresentable", "type-safe API", "pit of success"); when auditing existing code for footguns ("what is easy to misuse here", "review this diff for ways to get it wrong"); or when a bug has recurred and the fix must close the class rather than the case. Especially for money, auth, deletion, migrations and pipelines, where failure is silent. | `references/hazard-catalog.md`
`references/lang-python.md`
`references/lang-rust-go.md`
`references/lang-typescript.md`
`scripts/detect_hazards.py` | +| [poka-yoke-audit](../skills/poka-yoke-audit/SKILL.md)
`gh skills install github/awesome-copilot poka-yoke-audit` | Find footguns in code that already exists: swappable arguments, silent fallbacks, unguarded deletes, signatures that are easy to misuse. Use when someone asks "what could bite us here", "what is easy to misuse", "poka-yoke this repo", or wants a diff or PR reviewed for ways to get it wrong. Ranks by blast radius. For code not yet written use design; for something that already broke use retro. | `references/hazard-catalog.md`
`references/lang-python.md`
`references/lang-rust-go.md`
`references/lang-typescript.md`
`scripts/detect_hazards.py`
`scripts/device_registry.py` | +| [poka-yoke-design](../skills/poka-yoke-design/SKILL.md)
`gh skills install github/awesome-copilot poka-yoke-design` | Design APIs, schemas, types and state machines so misuse cannot be expressed. Use when writing a new interface and someone asks "what should the types look like", "make invalid states unrepresentable", "so callers cannot screw it up", or wants illegal state transitions rejected. Covers branded types, discriminated unions, typestate, parse-don't-validate. For code that already exists use audit. | `references/hazard-catalog.md` | +| [poka-yoke-guardrails](../skills/poka-yoke-guardrails/SKILL.md)
`gh skills install github/awesome-copilot poka-yoke-guardrails` | Pre-commit hooks, CI gates, lint rules, database constraints and branch protection. Use when a rule needs enforcing rather than documenting: "set up enforcement", "unformatted or untyped code must not get merged", "gate this in CI", "we agreed to X and people still do not", "stop secrets getting committed". Covers baselining and ratcheting so existing violations do not block anyone. For constraining an AI agent use agent-guardrails. | `assets/devices/github-actions/poka-yoke-gates.yml`
`assets/devices/lint/README.md`
`assets/devices/pre-commit/.pre-commit-config.yaml` | +| [poka-yoke-retro](../skills/poka-yoke-retro/SKILL.md)
`gh skills install github/awesome-copilot poka-yoke-retro` | Turn a bug, outage or repeated mistake into a device that makes the whole class impossible. Use when something already broke: "make sure this never happens again", "this is the third time", "postmortem", "how did this get through". Root-causes to the missing constraint, then sweeps every other site where the mistake is still available. For a pipeline use data, a deploy use ops, cross-tenant use authz, an AI feature use llm. | `scripts/detect_hazards.py` | | [postgresql-code-review](../skills/postgresql-code-review/SKILL.md)
`gh skills install github/awesome-copilot postgresql-code-review` | PostgreSQL-specific code review assistant focusing on PostgreSQL best practices, anti-patterns, and unique quality standards. Covers JSONB operations, array usage, custom types, schema design, function optimization, and PostgreSQL-exclusive security features like Row Level Security (RLS). | None | | [postgresql-optimization](../skills/postgresql-optimization/SKILL.md)
`gh skills install github/awesome-copilot postgresql-optimization` | PostgreSQL-specific development assistant focusing on unique PostgreSQL features, advanced data types, and PostgreSQL-exclusive capabilities. Covers JSONB operations, array types, custom types, range/geometric types, full-text search, window functions, and PostgreSQL extensions ecosystem. | None | | [power-apps-code-app-scaffold](../skills/power-apps-code-app-scaffold/SKILL.md)
`gh skills install github/awesome-copilot power-apps-code-app-scaffold` | Scaffold a complete Power Apps Code App project with PAC CLI setup, SDK integration, and connector configuration | None | diff --git a/skills/poka-yoke-audit/SKILL.md b/skills/poka-yoke-audit/SKILL.md new file mode 100644 index 000000000..8facab2a7 --- /dev/null +++ b/skills/poka-yoke-audit/SKILL.md @@ -0,0 +1,114 @@ +--- +name: poka-yoke-audit +description: 'Find footguns in code that already exists: swappable arguments, silent fallbacks, unguarded deletes, signatures that are easy to misuse. Use when someone asks "what could bite us here", "what is easy to misuse", "poka-yoke this repo", or wants a diff or PR reviewed for ways to get it wrong. Ranks by blast radius. For code not yet written use design; for something that already broke use retro.' +license: MIT +--- + +# Poka-Yoke Audit + +Find the mistakes that are *available* in this code, then close them. You are not looking for +bugs: a bug is a mistake that already happened. You are looking for **affordances for +mistakes**: places where doing the wrong thing is easy, silent, and looks correct. + +The load-bearing question throughout: *if a competent, tired engineer used this at 4pm on a +Friday, what would go wrong and would anything stop them?* + +## 1. Establish scope + +Default, when the user names no path: + +1. `git diff HEAD`: uncommitted work. This is what they are most likely asking about. +2. If the tree is clean, `git diff HEAD~5..HEAD`: recent commits. +3. If neither yields anything (fresh repo, no git), fall back to the risk surfaces below and + say that's what you did. + +Widen to the whole repo only when asked ("audit the whole codebase", "full audit"). It is +slow and it buries the important findings in volume. When you do go wide, prioritize by +**risk surface** rather than by directory, go straight to code that touches money, +authentication, authorization, deletion or overwriting, migrations, external I/O, +concurrency, and anything with `admin`, `force`, `bulk`, `sync`, or `delete` in its name. + +State the scope you chose in one line before you start, so the user can redirect you cheaply. + +## 2. Run the detector, then think + +```bash +python3 scripts/detect_hazards.py --diff # path is relative to this SKILL.md +``` + +Also `--paths src/`, `--staged`, `--since HEAD~10`, `--json`, `--severity high`, `--id C1 M2`. + +It finds the mechanically detectable shapes: adjacent same-type parameters, boolean flag +arguments, unbounded deletes, money as a float, unvalidated request bodies, retries with no +idempotency key. Shapes a real linter already covers are off by default; `--all` runs them too. +It is a fast first pass with real false positives, not an oracle — read the surrounding code +before you believe a hit. + +Then do the part the script cannot: run the three lenses over the interfaces. + +**Contact — can the wrong thing fit?** Are two adjacent parameters the same type? Could a +caller pass an order ID where a user ID belongs, cents where dollars belong, a raw string where +a validated one belongs? Does the boundary accept `any` / `dict` / `interface{}` and hope? + +**Fixed-value — can an incomplete set pass?** Is every enum branch handled, and does adding a +variant break the build or silently fall through? Can a bulk operation run on an empty or +unexpectedly huge set? Is config validated as a whole, or discovered missing at 3am? + +**Motion-step — can the order be wrong?** Must something be called before something else with +nothing enforcing it? Can a retry double-charge? Can a resource leak on the error path? Can two +callers interleave between a check and the act that depends on it? + +## 3. Classify every finding + +Each finding gets four fields. Fill all four: an unclassified finding is just an opinion. + +- **Mistake**: the specific wrong thing a person can do, stated as an action. + *"Call `transfer(dst, src)` with the accounts reversed."* +- **Consequence**: what happens when they do, and how loudly. Silence is the aggravator: a mistake that throws immediately is far less dangerous than one that returns a plausible + wrong answer. +- **Current rung**: what exists today, Control / Warning / Detection / **None**. +- **Proposed device + rung**: the specific change, and the rung it reaches. If you're + proposing Warning, say what would be needed for Control and why you didn't. + +## 4. Rank by expected damage, not by count + +Priority is **blast radius × ease of mistake**. A hundred stringly-typed internal helpers matter +less than one `delete_users(filter)` where `filter` can be empty. + +Blast radius, descending: irreversible data loss or money movement → authorization bypass → +silent data corruption → wrong output the user acts on → crash → degraded experience. A crash +ranking *below* silent wrong output is deliberate: loud failures are cheap, quiet ones compound. + +Ease, descending: silent and plausible-looking → requires only forgetting → needs an +unusual-but-reachable input → needs deliberate misuse. + +Report in priority order and stop somewhere sensible — ten well-argued findings beat forty. Say +how many you set aside and why. + +## 5. Report + +Use this structure. It is short on purpose; the detail lives per-finding. + +```markdown +# Poka-Yoke Audit — — +**Verdict**: + +### 1. — / +**Where**: `path/file.ts:42` +**Mistake**: +**Consequence**: +**Today**: Control | Warning | Detection | None +**Device**: → **** +``` + +Write it to `docs/poka-yoke/audit-YYYY-MM-DD.md` in the user's repo. If they'd rather not +have a file, keep it in the conversation, ask if it isn't obvious. + +## 6. Propose, then apply + +Present the findings and wait. Do not edit files yet. These changes alter interface shapes +and ripple through call sites; people reasonably want to see the plan first. + +When they approve some or all of it: apply each device, leave a `poka-yoke:` marker comment +at it saying which mistake it blocks, and run the tests. + diff --git a/skills/poka-yoke-audit/references/hazard-catalog.md b/skills/poka-yoke-audit/references/hazard-catalog.md new file mode 100644 index 000000000..b2e1c56ba --- /dev/null +++ b/skills/poka-yoke-audit/references/hazard-catalog.md @@ -0,0 +1,415 @@ +# Hazard Catalog + +The recurring shapes that produce mistakes, organized by the lens that finds them. Each entry: +what to look for, why it bites, and the device that closes it with the rung it reaches. + +Use this as working vocabulary, not a checklist to run top to bottom. The lens questions are +the real tool; this catalog is what the lenses usually turn up. + +## Contents + +- [Contact lens, can the wrong thing fit?](#contact-lens-can-the-wrong-thing-fit) + - [C1. Adjacent same-type parameters](#c1-adjacent-same-type-parameters) + - [C2. Boolean flag parameters](#c2-boolean-flag-parameters) + - [C3. Primitive obsession at boundaries](#c3-primitive-obsession-at-boundaries) + - [C4. Stringly-typed enums](#c4-stringly-typed-enums) + - [C5. Implicit units and magnitudes](#c5-implicit-units-and-magnitudes) + - [C6. Money as a float](#c6-money-as-a-float) + - [C7. Unvalidated external input](#c7-unvalidated-external-input) + - [C8. Bag-of-optionals structs](#c8-bag-of-optionals-structs) + - [C9. Naive datetimes](#c9-naive-datetimes) +- [Fixed-value lens, can an incomplete or wrong-sized set pass?](#fixed-value-lens-can-an-incomplete-or-wrong-sized-set-pass) + - [F1. Non-exhaustive branching](#f1-non-exhaustive-branching) + - [F2. Unbounded destructive operations](#f2-unbounded-destructive-operations) + - [F3. Defaults that hide a decision](#f3-defaults-that-hide-a-decision) + - [F4. Config discovered missing at runtime](#f4-config-discovered-missing-at-runtime) + - [F5. Partial writes without a transaction](#f5-partial-writes-without-a-transaction) + - [F6. Invariants enforced only in the application](#f6-invariants-enforced-only-in-the-application) + - [F7. Unbounded input](#f7-unbounded-input) +- [Motion-step lens, can the order be wrong?](#motion-step-lens-can-the-order-be-wrong) + - [M1. Temporal coupling](#m1-temporal-coupling) + - [M2. Non-idempotent retryable effects](#m2-non-idempotent-retryable-effects) + - [M3. Illegal state transitions](#m3-illegal-state-transitions) + - [M4. Resources that must be released](#m4-resources-that-must-be-released) + - [M5. Check-then-act races](#m5-check-then-act-races) + - [M6. Fire-and-forget async](#m6-fire-and-forget-async) + - [M7. Order-dependent migrations and deploys](#m7-order-dependent-migrations-and-deploys) +- [Cross-cutting, devices that were removed](#cross-cutting-devices-that-were-removed) + - [X1. Swallowed errors](#x1-swallowed-errors) + - [X2. Silent coercion and fallback](#x2-silent-coercion-and-fallback) + - [X3. Disabled tests](#x3-disabled-tests) + - [X4. Escape hatches in the type system](#x4-escape-hatches-in-the-type-system) + - [X5. Mutable shared defaults](#x5-mutable-shared-defaults) + +--- + +## Contact lens, can the wrong thing fit? + +The factory analogy: a part that only seats one way. In software, the type is the shape. + +### C1. Adjacent same-type parameters + +**Signal**: two or more consecutive parameters of the same primitive type, `transfer(from: string, to: string)`, `resize(w: number, h: number)`, +`slice(start: int, end: int)`. + +**Why it bites**: swapping them compiles, passes review, and produces a plausible wrong +result. It is among the most common footguns in software, and one of the most cleanly +solved, once the two types differ, the wrong order will not compile. + +**Device**: distinct types per concept, branded types, newtypes, value objects, so a +`SourceAccount` cannot be passed as a `DestinationAccount`. **Control.** +Fallback where types can't help: force keyword/named arguments so the caller must write the +name at the call site. **Warning**, but nearly free and it makes the swap visible in review. + +### C2. Boolean flag parameters + +**Signal**: `createUser(name, true, false)`, `save(data, force=True)`, any `bool` parameter +that selects behavior rather than carrying data. + +**Why it bites**: the call site is unreadable, so misordered or misunderstood flags are +invisible. Adding a second boolean makes it exponentially worse. + +**Device**: an enum or literal union per axis (`Visibility.Public`), an options object with +named fields, or two separate functions. **Control** for the enum, since the wrong value has +no spelling. Note the exception: a single boolean whose name reads correctly at the call site +in a keyword-argument language is fine. + +### C3. Primitive obsession at boundaries + +**Signal**: `string` for email, URL, path, token, tenant ID, phone; `int` for a percentage or +a duration, especially on public functions. + +**Why it bites**: every downstream function must re-check or trust. Validation that returns a +boolean throws away the proof, so the check gets repeated, skipped, or done inconsistently. + +**Device**: parse-don't-validate. `parseEmail(s): Email | Error` once at the boundary, then +downstream signatures demand `Email`. The type carries the guarantee permanently. **Control.** + +### C4. Stringly-typed enums + +**Signal**: `status: string` with a comment listing the values; string comparison against +literals; a value crossing a boundary as text with no schema. + +**Why it bites**: typos compile. New variants added elsewhere never reach this code. Nothing +tells you which values are legal. + +**Device**: a literal union, enum, or sealed class, with exhaustive matching (F1). **Control.** + +### C5. Implicit units and magnitudes + +**Signal**: `timeout: number`, `distance: float`, `retryAfter: int`: no unit anywhere except +possibly a name or a comment. Two systems in the same codebase disagreeing on seconds vs +milliseconds. + +**Why it bites**: a 1000x error is silent and looks like a hang or a hot loop. This class of +mistake famously destroyed a Mars orbiter. + +**Device**: unit-bearing types (`Duration`, `Milliseconds`), or at minimum encode the unit in +the parameter name (`timeoutMs`). **Control** for the type. The name is **rung 0**: it makes +a mismatch visible to a reader who is looking, and produces no diagnostic for one who is not. +Worth doing; not a device. + +### C6. Money as a float + +**Signal**: `price: float`, `amount: number`, arithmetic on currency in binary floating point, +`==` comparisons on money. + +**Why it bites**: 0.1 + 0.2 ≠ 0.3. Errors accumulate over aggregation and reconciliation +fails in ways that take days to trace. + +**Device**: integer minor units (cents) in a `Money` type carrying its currency, or a decimal +type. Mixed-currency arithmetic should not typecheck. **Control.** + +### C7. Unvalidated external input + +**Signal**: `JSON.parse(body)` into `any`, `request.json()` into a bare dict, a third-party +API response used field-by-field with no schema, `os.environ[...]` read deep inside logic. + +**Why it bites**: the failure surfaces far from the boundary, as a confusing error about a +missing property, long after the malformed data has been partially processed or stored. + +**Device**: a schema at every edge, zod/valibot, Pydantic, `encoding/json` into a typed +struct with validation, serde. Parse once, then work with parsed types. **Control.** +This applies to *your own* services' responses too; "internal" is not a guarantee. + +### C8. Bag-of-optionals structs + +**Signal**: a type with several optional fields where only certain combinations are +meaningful, `{ status, data?, error?, retryAt? }`, `{ isLoading, data, error }`. + +**Why it bites**: N optional fields claim 2^N legal states. Every consumer must guess which +are real, and they guess differently. States like "loading and errored with data" become +reachable and get handled inconsistently. + +**Device**: a discriminated union with exactly the legal variants, so impossible combinations +have no representation. **Control.** This is the canonical "make invalid states +unrepresentable" move. + +### C9. Naive datetimes + +**Signal**: timezone-less timestamps, `datetime.now()` / `new Date()` scattered through +business logic, dates stored as strings, DST-unaware arithmetic. + +**Why it bites**: correct in the developer's timezone, wrong in production, and wrong twice a +year in the places that observe DST. Also hard to test, logic that reads the clock directly +cannot be exercised at a boundary condition without freezing or injecting time. + +**Device**: timezone-aware types everywhere, UTC at rest, an injected clock so time is a +parameter rather than an ambient read. **Control** for the type, and the injected clock buys +testability, which is a Detection-rung device that finally becomes possible. + +--- + +## Fixed-value lens, can an incomplete or wrong-sized set pass? + +The factory analogy: a counter confirming all six screws were fitted. + +### F1. Non-exhaustive branching + +**Signal**: a `switch`/`match` over an enum with a `default` that does nothing meaningful, or +an if/else chain over a closed set of values. + +**Why it bites**: adding a variant silently takes the default branch at every site that +should have been updated. The bug appears months later, in the one code path nobody tested. + +**Device**: compiler-enforced exhaustiveness: an `assertNever(x: never)` arm in TypeScript, +`match` without a catch-all in Rust, `assert_never` with mypy, an exhaustive linter for Go. +**Control**, one line per switch, and among the highest-leverage devices available. + +### F2. Unbounded destructive operations + +**Signal**: `DELETE`/`UPDATE` built from a filter that can be empty; `rm -rf "$VAR"`; +`.deleteMany(where)`; bulk send/publish over a query result; a "cleanup" job with no cap. + +**Why it bites**: irreversible, instant, and proportional to your data volume. An empty filter +frequently means "match everything." + +**Device**: refuse an empty predicate; require an explicit `all=True` for the full-table case; +cap the affected row count and require confirmation above it; dry-run by default with the +count printed. Soft-delete where the domain allows. **Control.** + +### F3. Defaults that hide a decision + +**Signal**: a default value for something with no safe default, `retries=3`, `timeout=30`, +`currency="USD"`, `tenant=None`, `region=default`. + +**Why it bites**: the caller never considers the parameter, and the default is wrong for their +case. Worse than an error, because it produces confident wrong behavior. + +**Device**: make it required. Reserve defaults for parameters where one value is correct for +the overwhelming majority and wrong-but-harmless for the rest. **Control.** + +### F4. Config discovered missing at runtime + +**Signal**: `os.getenv("X")` inside a request handler; config read lazily on first use; a +missing key producing `None` that flows onward. + +**Why it bites**: the service starts, passes health checks, and fails on the one code path +that needs the key, often the payment path, often at 3am. + +**Device**: parse and validate the entire config into a typed object at startup, and exit +non-zero if anything is missing or malformed. Every consumer takes the typed object. +**Control**, and it converts a 3am page into a failed deploy. + +### F5. Partial writes without a transaction + +**Signal**: several writes in sequence with no transaction; a write followed by an external +call followed by another write; "create the record then send the email." + +**Why it bites**: a failure in the middle leaves the system in a state your code does not +model and cannot repair. + +**Device**: wrap in a transaction; move external effects outside it via an outbox; make the +sequence idempotent so replay converges. **Control** for the transaction. + +### F6. Invariants enforced only in the application + +**Signal**: uniqueness checked with a `SELECT` before an `INSERT`; nullability enforced in a +model class but not in the column; a foreign key relationship maintained by convention. + +**Why it bites**: the check races under concurrency, and it is bypassed entirely by any other +service, migration, script, or human with `psql`. + +**Device**: push it into the schema, `NOT NULL`, `UNIQUE`, `CHECK`, foreign keys, partial +unique indexes. The database is a type system shared by everything that touches the data. +**Control**, and uniquely durable. + +### F7. Unbounded input + +**Signal**: pagination with no maximum page size; a file upload with no size limit; a query +built from a user-supplied list with no cap; unbounded recursion or retries. + +**Why it bites**: a resource exhaustion incident indistinguishable from an attack, triggered +by an ordinary user with a large account. + +**Device**: explicit caps at the boundary, enforced by the parsing type where possible. +**Control.** + +--- + +## Motion-step lens, can the order be wrong? + +The factory analogy: a sensor confirming step 3 happened before step 4. + +### M1. Temporal coupling + +**Signal**: `init()`, `connect()`, `configure()`, `validate()` that must be called before +other methods; documentation containing the phrase "you must call X first." + +**Why it bites**: nothing enforces it. The failure is a null dereference or, worse, a +silently-wrong result from a half-configured object. + +**Device**: the constructor or a static factory returns a fully ready object; or typestate, +where `connect()` returns a `Connected` type and the other methods exist only on it. +**Control.** + +### M2. Non-idempotent retryable effects + +**Signal**: a charge, email, webhook, or external mutation reachable from a retry, a queue +consumer, or a UI button, with no idempotency key, or with an optional one. + +**Why it bites**: at-least-once delivery is the norm, not the exception. Duplicate charges are +the canonical version and they are expensive and public. + +**Device**: a **required** idempotency key parameter, backed by a unique constraint on +`(entity, key)`. **Control.** An optional idempotency key is rung zero wearing a costume. + +The constraint is necessary and not sufficient. Rejecting the duplicate is not the same as +being idempotent: the key has to be *reserved in the same transaction as the effect*, bound +to the request payload so a different payload under a reused key is an error rather than a +silent no-op, and the stored result replayed to the second caller. A caller that retries and +gets a constraint violation has learned nothing about whether the first attempt worked. + +### M3. Illegal state transitions + +**Signal**: an entity with a `status` field mutated by assignment from several places; a +refund reachable before a charge; "cancelled" transitioning back to "pending". + +**Why it bites**: every site that assigns the field must know the whole state machine, and one +of them doesn't. + +**Device**: a single transition function that is the only path to a new state, rejecting +illegal transitions; or typestate so illegal transitions don't compile. **Control.** + +A row-level `CHECK` is not defence in depth here: it constrains one row's values and cannot +see the state that row is coming from, so it can forbid `status = 'refunded' AND total < 0` +but not `shipped → pending`. Policing transitions in the database needs a trigger, or a +transition table the row must join against. + +### M4. Resources that must be released + +**Signal**: `open()`/`close()`, `acquire()`/`release()`, `begin()`/`commit()` as separate +statements, especially with a `return` or `throw` reachable between them. + +**Why it bites**: the happy path is fine and the error path leaks. Leaks surface as connection +pool exhaustion under load, which is when you can least afford it. + +**Device**: scope-bound acquisition, `with`, `defer`, RAII, `using`, try-with-resources. +**Control.** + +### M5. Check-then-act races + +**Signal**: `if (!exists(x)) create(x)`, read-modify-write on a shared counter, checking a +balance and then debiting it, `if (!file.exists()) write(file)`. + +**Why it bites**: correct in every test and wrong under concurrency, intermittently, in +production only. + +**Device**: make it atomic: a unique constraint plus `INSERT ... ON CONFLICT`, a conditional +update carrying the expected version, `SELECT FOR UPDATE`, a compare-and-swap. **Control.** + +### M6. Fire-and-forget async + +**Signal**: a promise not awaited, a goroutine with no error path, `asyncio.create_task` with +no reference kept, a background write nobody joins. + +**Why it bites**: errors vanish. Worse, the process may exit before the work completes, so +writes are lost silently and non-deterministically. + +**Device**: `no-floating-promises` as a lint error, an errgroup, structured concurrency, +holding and awaiting the task. **Warning** from the linter, which is the practical answer +in TypeScript, Python and Go. Rust is the closest thing to an exception: futures are lazy and `#[must_use]`, so a dropped +future produces a compiler warning without any linter. That is **Warning**, for free; add +`#![deny(unused_must_use)]` to make the build fail and it becomes **Control**. + +### M7. Order-dependent migrations and deploys + +**Signal**: a migration that drops or renames a column in the same deploy as the code change; +a migration and code that must land in a specific order with nothing enforcing it. + +**Why it bites**: during the rollout window, old code runs against the new schema. This is an +outage, not a bug. + +**Device**: expand/contract, add, backfill, dual-write, switch, then drop in a later deploy, with a CI gate that blocks destructive DDL from co-deploying with code changes. **Control** +via the gate; the pattern itself is the design. + +--- + +## Cross-cutting, devices that were removed + +Several of these are hazards of removal, someone installed a device and someone else took +it out. Others (X2, X5) are defaults nobody chose: the language ships them switched the wrong +way and they stay that way until someone notices. +Treat them with more suspicion than a missing device, since the code around them was written +by someone who knew the failure was possible. + +### X1. Swallowed errors + +**Signal**: `catch {}`, `except: pass`, `except Exception: pass`, `_ = err`, `catch (e) { +console.log(e) }` with execution continuing, `.catch(() => null)`. + +**Why it bites**: converts a loud failure into a quiet wrong answer: the exact inversion of +mistake-proofing. The system continues on corrupted assumptions. + +**Device**: handle it, or let it propagate. Where absorbing genuinely is correct, the comment +must name which specific failure is expected and why continuing is safe; catch that specific +type, not everything. Enforce with `no-empty` / bare-except lint rules as errors. **Warning.** + +### X2. Silent coercion and fallback + +**Signal**: `value || default` where `0`/`""`/`false` are legal values; `parseInt` without a +radix or a NaN check; `int(x)` in a try/except returning a default; `.unwrap_or_default()` on +a genuine error; `?.` chains ending in `undefined` that flow into logic. + +**Why it bites**: produces a plausible value from bad input. The wrongness surfaces far away, +where the cause is invisible. + +**Device**: `??` instead of `||` where zero is legal; explicit parse with an error branch; +fail at the boundary rather than substituting. **Control** at the parse site. + +### X3. Disabled tests + +**Signal**: `it.only`, `describe.skip`, `@pytest.mark.skip`, `t.Skip()`, `#[ignore]`: especially without a reason. Lint and type-checker suppressions (`eslint-disable`, +`# type: ignore`, `@ts-ignore`, `#nosec`) are X4, and the detector splits them the same way. + +**Why it bites**: a Detection-rung device switched off, usually temporarily, permanently. The +suite stays green and stops meaning anything. + +**Device**: fail CI on focused/skipped tests; require a justification comment and an issue +link on every suppression; count suppressions and ratchet the number downward. **Warning.** + +### X4. Escape hatches in the type system + +**Signal**: `any`, `as unknown as T`, `!` non-null assertion, `interface{}` with a type +switch, `# type: ignore`, `unsafe`, `cast()`, `Object` as a parameter type. + +**Why it bites**: every one is a place where the type system's guarantee stops. Concentrated +in the boundary code that most needs the guarantee. + +**Device**: ban them by lint at error level with a narrow, justified allowlist; replace with +parsing at the boundary. **Warning**: a required CI gate is still rung 2 on the ladder: it announces the mistake +rather than removing the ability to make it. Reach **Control** only when the unchecked value cannot be constructed. + +### X5. Mutable shared defaults + +**Signal**: Python's `def f(items=[])`, a module-level dict used as a cache and mutated, a +shared config object mutated after construction, class attributes used as instance state. + +**Why it bites**: state leaks between calls, requests, or tests. The symptom is +order-dependent behavior that disappears when you try to reproduce it. + +**Device**: `None` sentinel with in-function construction, frozen/immutable value types, +per-request construction. `B006` in ruff/flake8-bugbear enforces the argument-default case +only; the module-level cache, the shared config object and the mutable class attribute have +no lint rule and need review or a type that cannot be mutated. +**Warning**, or **Control** with frozen types. diff --git a/skills/poka-yoke-audit/references/lang-python.md b/skills/poka-yoke-audit/references/lang-python.md new file mode 100644 index 000000000..0079b0001 --- /dev/null +++ b/skills/poka-yoke-audit/references/lang-python.md @@ -0,0 +1,181 @@ +# Python Devices + +Python's type hints are optional and unenforced at runtime, which splits every device into two +questions: what the checker catches, and what actually holds when the code runs. + +**Prerequisite**: `mypy --strict` (or `pyright` in strict mode) as a *required* CI check. +Without it, annotations are documentation, rung zero. Pair it with `ruff` at error level. + +## Contact, NewType for cheap distinctness + +```python +from typing import NewType + +UserId = NewType("UserId", str) +OrderId = NewType("OrderId", str) + +def transfer(src: UserId, dst: UserId) -> None: ... + +transfer(order_id, user_id) # mypy: error: zero runtime cost +``` + +`NewType` is free at runtime and stops the mix-up at check time. It does not validate, use it +when the concepts differ but the shape doesn't need checking. + +## Contact, parse at the boundary with Pydantic + +When the value needs checking, parse into a model and let the type carry the proof: + +```python +from pydantic import BaseModel, EmailStr, Field, ConfigDict + +class CreateUser(BaseModel): + model_config = ConfigDict(frozen=True, extra="forbid") + + email: EmailStr + age: int = Field(ge=0, le=150) +``` + +Two settings do most of the work. `extra="forbid"` turns a typo'd field into an error instead +of a silently ignored key: the difference between a 400 and a user whose preference never +saved. `frozen=True` blocks reassignment of the model's fields, so nothing downstream can +quietly replace what you verified. It is shallow, though: a `list` or `dict` field is still +mutable in place, so reach for `tuple`, `frozenset`, or a nested frozen model where that +matters. + +Apply at every edge: request bodies, queue messages, third-party responses, file loads. + +## Contact, keyword-only arguments + +Python's answer to swapped parameters, and it costs one character: + +```python +def transfer(*, source: AccountId, dest: AccountId, amount: Money) -> None: ... + +transfer(source=a, dest=b, amount=m) # the only legal form +transfer(a, b, m) # TypeError +``` + +Force keyword-only for anything with more than two parameters, and always when two share a +type. This is Warning-rung. It makes the mistake visible rather than impossible, but it is +the highest-value one-character change in the language. + +## Fixed-value, exhaustiveness + +```python +from typing import assert_never, Literal + +Status = Literal["pending", "active", "closed"] + +def label(s: Status) -> str: + match s: + case "pending": return "Pending" + case "active": return "Active" + case "closed": return "Closed" + case _: assert_never(s) # mypy errors here if a variant is unhandled +``` + +`assert_never` turns "someone added a status" into a build failure at every site that must +change. Works with `Literal`, `Enum`, and tagged dataclass unions. + +## Fixed-value, config validated at startup + +```python +from pydantic_settings import BaseSettings + +class Settings(BaseSettings): + database_url: str + stripe_key: str + region: str # no default: an unset value should stop the deploy + +settings = Settings() # raises at import, before the service reports healthy +``` + +Import this once at startup and pass the object down. Every `os.getenv` buried in a handler is +a 3am page waiting for the one request that reaches it. + +## Contact, immutable value objects + +```python +from dataclasses import dataclass + +@dataclass(frozen=True, slots=True, kw_only=True) +class Money: + cents: int + currency: str + + def __add__(self, other: "Money") -> "Money": + if self.currency != other.currency: + raise ValueError(f"cannot add {self.currency} to {other.currency}") + return Money(cents=self.cents + other.cents, currency=self.currency) +``` + +`frozen=True` prevents mutation after validation, `slots=True` makes a typo'd attribute +assignment an `AttributeError` rather than a silently-created new attribute, and `kw_only=True` +kills positional swaps. Three flags, three hazard classes closed. + +## Motion-step, context managers + +Any acquire/release pair belongs in a context manager. Never expose `open()`/`close()` as +separate public methods: the error path will leak, and only under load. + +```python +from contextlib import contextmanager + +@contextmanager +def transaction(conn): + tx = conn.begin() + try: + yield tx + tx.commit() + except Exception: + tx.rollback() + raise # re-raise: swallowing here would be X1 +``` + +## Python-specific traps worth checking every time + +- **Mutable default arguments**: `def f(items=[])` shares one list across every call. Use + `None` and construct inside. Caught by ruff `B006`. +- **Bare `except:`** catches `KeyboardInterrupt` and `SystemExit` too. Caught by `E722`. +- **`assert` for validation** is stripped under `python -O`. Never use it for anything + security- or correctness-critical; raise instead. +- **Naive `datetime.now()`**: use `datetime.now(timezone.utc)`, and inject a clock so time + is testable. Caught by ruff `DTZ`. +- **Float money**: use `int` cents or `decimal.Decimal`, never `float`. +- **`==` vs `is`** on strings and ints works by accident via interning and breaks in + production on longer values. Caught by `F632`. +- **`asyncio.create_task` without keeping a reference**: the task can be garbage collected + mid-flight, so the work silently doesn't happen. Caught by ruff `RUF006`. + +## Ruff rule sets that are poka-yoke + +Style rules aren't mistake-proofing; these are, which is why `E` appears only as its +bug-shaped subsets and not whole. Select at error level: + +```toml +[tool.ruff.lint] +select = [ + "F", # pyflakes: undefined names, unused imports + "E4", "E7", "E9", # pycodestyle's bug-shaped rules: bare except, `== None`, syntax errors + "B", # bugbear: mutable defaults, loop variable capture, assert-on-tuple + "S", # bandit: hardcoded secrets, unsafe subprocess, weak crypto + "DTZ", # naive datetimes + "ASYNC", # blocking calls inside async functions + "RUF006", # dangling asyncio tasks + "PLE", # pylint errors: genuine bugs only + "T20", # stray print/pprint +] +``` + +## Known limits + +- **Annotations are not enforced at runtime.** Anything crossing a boundary, or reachable + from unchecked code, needs a real runtime parse. Pydantic is how you get Control; mypy + alone gives you Control only over code mypy actually checks. +- **`Any` is contagious** and an untyped dependency reintroduces it silently. Set + `disallow_any_unimported` and `warn_return_any`; audit `# type: ignore` comments and require + a reason on each. +- **No affine types**, so use-after-close isn't preventable; context managers are the answer. +- **Monkey-patching means no encapsulation is absolute.** Push invariants that truly must hold + into the database rather than into a class. diff --git a/skills/poka-yoke-audit/references/lang-rust-go.md b/skills/poka-yoke-audit/references/lang-rust-go.md new file mode 100644 index 000000000..b2a0614df --- /dev/null +++ b/skills/poka-yoke-audit/references/lang-rust-go.md @@ -0,0 +1,213 @@ +# Rust and Go Devices + +Two languages at opposite ends of the expressiveness spectrum. Rust can encode almost any +invariant in types; Go deliberately cannot, so its devices lean on convention plus tooling. +Know which one you're in before proposing a device. + +--- + +# Rust + +Rust's type system reaches Control for more hazard classes than any other mainstream language. +The affine type system in particular is the only mainstream answer to use-after-move, and it +turns use-after-close into a compile error rather than a convention, where Python has context +managers, TypeScript has scope-bound callbacks, and Go has `defer`, Rust has the compiler. + +## Contact, newtypes and smart constructors + +```rust +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct UserId(Uuid); + +#[derive(Debug, Clone)] +pub struct Email(String); + +impl Email { + // The only way to build one. Private field means no bypass, even in-crate + // if you put it behind a module boundary. + pub fn parse(s: &str) -> Result { + if !s.contains('@') { return Err(InvalidEmail); } + Ok(Email(s.to_owned())) + } + pub fn as_str(&self) -> &str { &self.0 } +} +``` + +A private field plus a fallible constructor means possessing an `Email` *is* proof of +validation. This is the strongest form of parse-don't-validate available anywhere. + +## Contact, enums make illegal states unrepresentable + +```rust +// Each variant carries exactly the data that variant has. There is no +// "succeeded with an error", because it cannot be written. +pub enum JobState { + Queued { enqueued_at: DateTime }, + Running { started_at: DateTime, worker: WorkerId }, + Succeeded { output: Output }, + Failed { error: JobError, retries: u32 }, +} +``` + +`match` without a catch-all is exhaustive by default, adding a variant breaks the build +everywhere it must. Avoid `_ => {}` arms in domain logic for exactly this reason: the wildcard +is what turns a compile error into a silent fallthrough two releases later. + +## Motion-step, typestate + +Ownership makes typestate genuinely practical, since each transition consumes the old state: + +```rust +pub struct Draft; +pub struct Validated; + +pub struct Order { items: Vec, _state: PhantomData } + +impl Order { + pub fn validate(self) -> Result, ValidationError> { /* … */ } +} + +impl Order { + // submit() does not exist on Order. Not "returns an error", does not exist. + pub fn submit(self) -> Result { /* … */ } +} +``` + +The consumed `self` means the draft is gone after validation, so a stale unvalidated copy +cannot be submitted later. + +## Fixed-value, make errors impossible to ignore + +`#[must_use]` on `Result` is built in; add it to your own types where dropping the value is a +bug. Then set the lints: + +```toml +[workspace.lints.clippy] +unwrap_used = "deny" +expect_used = "warn" # allow in tests and startup with a reason +panic = "deny" +indexing_slicing = "deny" # forces .get() and a real branch +float_cmp = "deny" +arithmetic_side_effects = "warn" # forces checked_/saturating_ where overflow matters +todo = "deny" +dbg_macro = "deny" +``` + +`unwrap_used = "deny"` is the highest-value line in that block: it converts every "this can't +fail" assumption into an explicit decision at review time. + +## Rust limits + +- **`unsafe` and `unwrap` are the escape hatches.** Deny both by lint and require a + `// SAFETY:` comment for each `unsafe` block. +- **Compile-time only.** Deserialized input still needs `serde` with `deny_unknown_fields`. +- **Panics bypass the type system.** A device that panics is Warning, not Control. +- **Typestate has real ergonomic cost.** Reserve it for genuinely dangerous sequences, payments, resource lifecycles, protocol state: not for every builder. + +--- + +# Go + +Go rejects most compile-time expressiveness by design. Its devices are therefore fewer, and +tooling plus data-layer constraints carry more of the load. Say so plainly when you propose a +device, Control is often not reachable here, and pretending otherwise is worse than +acknowledging the rung. + +## Contact, defined types + +```go +type UserID string +type OrderID string + +func Transfer(from, to UserID) error { ... } +// Transfer(orderID, userID), compile error, because these are defined types, not aliases. +``` + +Use `type X string` (a defined type), never `type X = string` (an alias, which gives you +nothing). This is the one genuine Control-rung contact device Go offers, and it is +underused. + +## Contact, functional options instead of boolean flags + +```go +type Option func(*Config) + +func WithTimeout(d time.Duration) Option { return func(c *Config) { c.Timeout = d } } +func WithRetries(n int) Option { return func(c *Config) { c.Retries = n } } + +func New(addr string, opts ...Option) (*Client, error) { ... } +``` + +Every option is named at the call site, `time.Duration` carries its unit in the type, and +adding an option later doesn't break callers. This replaces both the boolean-flag hazard and +the implicit-units hazard. + +## Motion-step, constructors and defer + +```go +func NewClient(addr string) (*Client, error) { + // Fully ready on return. No Connect() to forget. +} + +conn, err := pool.Acquire(ctx) +if err != nil { return err } +defer conn.Release() // on the line after acquisition, always +``` + +Put `defer` immediately after the acquisition, before any other statement. Any code between +the two is a leak on the error path. + +## Fixed-value, exhaustiveness + +Go has no exhaustive switch. Use a linter: + +```yaml +# .golangci.yml +version: "2" + +linters: + enable: + - errcheck # unchecked errors: the single most valuable Go linter + - exhaustive # non-exhaustive switch over typed constants + - bodyclose # unclosed HTTP response bodies + - rowserrcheck # unchecked sql.Rows.Err + - sqlclosecheck + - contextcheck # context not propagated + - nilerr # returning nil after a non-nil error + - noctx # HTTP requests without a context + - gosec + settings: + exhaustive: + default-signifies-exhaustive: false +``` + +That is the v2 schema. golangci-lint v2 refuses to run against a v1 file rather than ignoring +the parts it no longer understands, so run `golangci-lint migrate` over an existing config +before upgrading. + +`errcheck` is non-negotiable, Go's error convention is entirely opt-in without it, and +`_ = doSomething()` is how data loss enters a Go codebase. + +## Go-specific traps + +- **Nil maps** accept reads but panic on write. Construct with `make` in the constructor. +- **Loop variable capture** in goroutines, fixed in Go 1.22+, still present in older + codebases and vendored code. +- **`time.Duration` vs bare int**: always take a `Duration`; never an `int` seconds. +- **Zero values are valid**, so a struct with a missing field looks initialized. Use a + constructor that returns `(T, error)` and unexported fields to force it. +- **Slices share backing arrays**, `append` to a sub-slice can mutate the original. Use + three-index slicing `s[a:b:b]` when handing a slice out. +- **`context.Context` dropped** across a call boundary silently disables cancellation and + timeouts. `contextcheck` catches it. + +## Go limits + +Go cannot express: exhaustive matching, non-nullable references, immutability, typestate, or +generic constraints rich enough for units. Its Control-rung devices are essentially defined +types, unexported fields with constructors, and the database schema. + +The practical consequence: in Go, **push more invariants into the database and into required +CI checks** than you would in Rust or TypeScript. `NOT NULL`, `CHECK`, and unique constraints +are doing work the language declines to do, and `golangci-lint` as a required check is what +makes the rest hold. diff --git a/skills/poka-yoke-audit/references/lang-typescript.md b/skills/poka-yoke-audit/references/lang-typescript.md new file mode 100644 index 000000000..fc903af5a --- /dev/null +++ b/skills/poka-yoke-audit/references/lang-typescript.md @@ -0,0 +1,155 @@ +# TypeScript / JavaScript Devices + +What the type system can and cannot enforce, and the constructs that get you to Control. + +**Prerequisite**: none of this is load-bearing without `strict: true` in tsconfig and +`tsc --noEmit` as a *required* CI check. A branded type in a repo that doesn't typecheck in CI +is a comment. Start there. + +Also enable `noUncheckedIndexedAccess` (array access returns `T | undefined`, which is the +truth) and `exactOptionalPropertyTypes`. Both catch real mistakes that `strict` alone misses. + +## Contact, branded types + +TypeScript is structurally typed, so `type UserId = string` gives you nothing. Branding adds a +phantom property that exists only at compile time: + +```ts +declare const brand: unique symbol; +type Brand = T & { readonly [brand]: B }; + +export type UserId = Brand; +export type OrderId = Brand; + +export const UserId = (s: string): UserId => s as UserId; + +// transfer(orderId, userId) is now a compile error +declare function transfer(from: UserId, to: UserId): void; +``` + +Zero runtime cost, no wrapper object. Pair the constructor with validation when the string has +a shape worth checking, and it becomes a parse (below) rather than a cast. + +## Contact, parse, don't validate + +```ts +import { z } from "zod"; + +const Email = z.string().email().brand<"Email">(); +export type Email = z.infer; + +// At the boundary, and only here: +const parsed = Email.safeParse(req.body.email); +if (!parsed.success) return res.status(400).json({ error: parsed.error.format() }); + +sendWelcome(parsed.data); // sendWelcome(to: Email) cannot receive an unvalidated string +``` + +Zod's `.brand()` composes validation and branding in one step, which is the ideal shape: short +of an `as` cast, the only way to obtain an `Email` is to have parsed one, which is why the +lint against `as unknown as T` is part of the device, not a style preference. + +Apply at every edge: HTTP handlers, queue consumers, `process.env`, third-party responses, +file reads. `JSON.parse` returns `any` and `any` is where guarantees go to die. + +## Contact, discriminated unions over optional bags + +```ts +// Permits "success with an error", "loading with data", only three combinations are real +type Result = { status: string; data?: User; error?: Error }; + +// Permits exactly what exists +type Result = + | { status: "loading" } + | { status: "success"; data: User } + | { status: "error"; error: Error }; +``` + +The second version makes `result.data` inaccessible until you've narrowed to `"success"`, +so the check cannot be forgotten: the compiler asks for it. + +## Fixed-value, exhaustiveness + +```ts +function assertNever(x: never): never { + throw new Error(`Unhandled variant: ${JSON.stringify(x)}`); +} + +switch (result.status) { + case "loading": return spinner(); + case "success": return view(result.data); + case "error": return errorView(result.error); + default: return assertNever(result); +} +``` + +Adding a variant now breaks the build at every switch that must change. Enforce repo-wide with +`@typescript-eslint/switch-exhaustiveness-check`. This is the cheapest high-value device in +the language: one line per switch. + +## Motion-step, builders and typestate + +Encode required steps in the type so `.delete()` doesn't exist until they've run: + +```ts +declare const state: unique symbol; + +class QueryBuilder { + // Load-bearing. TypeScript is structural: a type parameter that no member mentions does + // not affect assignability, so without this line QueryBuilder is assignable + // to QueryBuilder and `delete()` is callable with no where clause -- the exact + // mistake the class claims to prevent, silently permitted. Verified with tsc 5 --strict. + private declare readonly [state]: [HasFrom, HasWhere]; + + from(t: string): QueryBuilder { /* … */ } + where(c: Cond): QueryBuilder { /* … */ } + + // Only callable once both have been set + delete(this: QueryBuilder): string { /* … */ } +} +``` + +The `this` parameter is the key trick: it constrains which instances a method exists on. With +the phantom member present, `new QueryBuilder().delete()` is `TS2684` rather than an incident. + +## Motion-step, required idempotency + +```ts +// Optional key = suggestion. Required key = device. +function charge(account: AccountId, amount: Money, idempotencyKey: IdempotencyKey): Promise +``` + +Back it with a unique index on `(account_id, idempotency_key)` so the second attempt is +rejected by the database, not by application logic that might be skipped. + +## The lint rules that are actually poka-yoke + +Style rules are not mistake-proofing. These are, set every one to `error`: + +| Rule | Mistake prevented | +|---|---| +| `@typescript-eslint/no-floating-promises` | A write that is never awaited and silently lost | +| `@typescript-eslint/no-misused-promises` | An async function passed where sync is expected | +| `@typescript-eslint/switch-exhaustiveness-check` | New enum variant silently unhandled | +| `@typescript-eslint/no-unnecessary-condition` | A check that is always true, usually a real bug | +| `@typescript-eslint/no-explicit-any` | Type guarantees silently disabled | +| `@typescript-eslint/no-unsafe-assignment` / `-return` / `-argument` | `any` leaking from untyped libraries | +| `no-empty` (with `allowEmptyCatch: false`) | Empty catch blocks | +| `eqeqeq` | `==` coercion surprises | +| `require-atomic-updates` | Read-modify-write races across `await` | +| `no-restricted-syntax` on `it.only` / `describe.only` | A focused test disabling the rest of the suite | + +`no-empty` only sees the empty block: a catch holding a comment, or one that logs and carries +on, swallows the error and passes the lint. Catching that shape is a review job. + +## Known limits + +- **No runtime enforcement.** Types vanish at compile time. Anything crossing a boundary needs + a runtime schema, and anything reachable from untyped JavaScript needs a runtime check. +- **Structural typing** means every distinct concept needs explicit branding; the compiler + will not distinguish them for you. +- **`as` casts are unchecked.** Confine them to the inside of parse functions, and lint + against `as unknown as T` anywhere else. +- **No affine types**, so use-after-move and use-after-close cannot be prevented; scope-bound + patterns (a `withConnection(fn)` callback rather than `open`/`close`) are the closest you + get, and they are usually enough. diff --git a/skills/poka-yoke-audit/scripts/detect_hazards.py b/skills/poka-yoke-audit/scripts/detect_hazards.py new file mode 100755 index 000000000..34f0e9de0 --- /dev/null +++ b/skills/poka-yoke-audit/scripts/detect_hazards.py @@ -0,0 +1,631 @@ +#!/usr/bin/env python3 +"""Heuristic detector for poka-yoke hazards, shapes in code that make mistakes easy. + +This is a fast first pass, not an oracle. It finds textually-detectable hazards so a +reviewer can spend their attention on the interface-level questions a regex cannot ask. +Expect real false positives; every hit is a question, not a verdict. + +Hazard IDs match references/hazard-catalog.md. Standard library only. + +Examples: + detect_hazards.py --diff # uncommitted changes, changed lines only + detect_hazards.py --staged # staged changes + detect_hazards.py --since HEAD~10 # last 10 commits + detect_hazards.py --paths src/ lib/ # explicit paths + detect_hazards.py --diff --severity high # only the ones that bite hardest + detect_hazards.py --paths . --json # machine-readable +""" + +from __future__ import annotations + +import argparse +import ast +import json +import os +import re +import subprocess +import sys +from dataclasses import dataclass, field +from pathlib import Path + +# -------------------------------------------------------------------------------------- +# Rule definitions +# -------------------------------------------------------------------------------------- + +PY = {".py", ".pyi"} +TS = {".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs"} +GO = {".go"} +RS = {".rs"} +SQL = {".sql"} +ALL_EXTS = PY | TS | GO | RS | SQL + +LENS = {"C": "contact", "F": "fixed-value", "M": "motion-step", "X": "removed-device"} + + +@dataclass(frozen=True) +class Rule: + id: str + name: str + severity: str # high | medium | low + exts: frozenset + pattern: re.Pattern + device: str + negate: re.Pattern | None = None # if this also matches the line, skip + + +def R(id, name, severity, exts, pattern, device, negate=None, flags=0): + return Rule( + id=id, + name=name, + severity=severity, + exts=frozenset(exts), + pattern=re.compile(pattern, flags), + device=device, + negate=re.compile(negate, flags) if negate else None, + ) + + +RULES: list[Rule] = [ + # ---- X: devices that were removed ------------------------------------------------- + R("X1", "Swallowed error", "high", TS, + r"catch\s*(\([^)]*\))?\s*\{\s*\}", + "Handle it or let it propagate; catching to do nothing turns a loud failure quiet."), + R("X1", "Swallowed error", "high", TS, + r"\.catch\s*\(\s*\(\s*\)\s*=>\s*(\{\s*\}|null|undefined)\s*\)", + "Handle the rejection or let it propagate."), + R("X1", "Bare except", "high", PY, + r"^\s*except\s*:", + "Catch the specific exception; bare except also swallows KeyboardInterrupt/SystemExit."), + R("X1", "Discarded error return", "high", GO, + r",\s*_\s*:?=\s*\w|^\s*_\s*=\s*\w[\w.]*\(", + "Check the error. Enable errcheck in golangci-lint to make this a build failure."), + R("X2", "Unwrap / expect on a fallible value", "medium", RS, + r"\.(unwrap|expect)\s*\(", + "Propagate with ? or handle the error; deny clippy::unwrap_used."), + R("X2", "Silent default on error", "medium", RS, + r"\.unwrap_or_default\s*\(\s*\)", + "A default on an error path hides the failure; branch on the error explicitly."), + R("X2", "parseInt without radix", "medium", TS, + r"parseInt\s*\(\s*[^,)]+\)", + "Pass the radix and check for NaN, or use a schema parse at the boundary."), + R("X3", "Focused test disables the suite", "high", TS, + r"\b(it|test|describe|context)\.only\s*\(|\bfdescribe\s*\(|\bfit\s*\(", + "Remove before merge; fail CI on focused tests."), + R("X3", "Skipped test", "medium", PY, + r"@pytest\.mark\.skip|@unittest\.skip", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X3", "Skipped test", "medium", TS, + r"\b(it|test|describe)\.skip\s*\(|\bxit\s*\(|\bxdescribe\s*\(", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X3", "Skipped test", "medium", GO, r"\bt\.Skip\s*\(", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X3", "Skipped test", "medium", RS, r"^\s*#\[ignore\]", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X4", "Type-checker suppression", "medium", TS, + r"@ts-ignore|@ts-nocheck|\bas\s+unknown\s+as\b|eslint-disable(?!-next-line\s+\S+\s+--)", + "Each suppression is a hole in the guarantee. Require a reason and an issue link."), + R("X4", "Explicit any", "medium", TS, + r":\s*any\b||Array|as\s+any\b", + "any disables the type system exactly where guarantees matter. Parse at the boundary."), + R("X4", "Type-checker suppression", "medium", PY, + r"#\s*type:\s*ignore(?!\[)", + "Narrow it to a specific error code and add a reason."), + R("X4", "Untyped container", "low", GO, + r"\binterface\{\}|\bany\b\s*[,)\]]", + "Prefer a concrete type or a constrained generic."), + R("X4", "unsafe block", "medium", RS, r"\bunsafe\s*\{", + "Require a // SAFETY: comment stating the invariant being upheld."), + R("X5", "Mutable default argument", "high", PY, + r"def\s+\w+\s*\([^)]*=\s*(\[\s*\]|\{\s*\}|set\s*\(\s*\))", + "Use None and construct inside the function; the default is shared across all calls."), + + # ---- F: fixed-value --------------------------------------------------------------- + R("F2", "Unbounded DELETE", "high", SQL | PY | TS | GO | RS, + r"\bDELETE\s+FROM\b(?!.*\bWHERE\b)", + "Require a WHERE clause; refuse an empty predicate.", flags=re.I), + R("F2", "Unbounded UPDATE", "high", SQL | PY | TS | GO | RS, + r"\bUPDATE\s+[\w.\"`\[\]]+\s+SET\b(?!.*\bWHERE\b)", + "Require a WHERE clause; refuse an empty predicate.", flags=re.I), + R("F2", "Destructive DDL", "high", SQL | PY | TS | GO | RS, + r"\b(DROP\s+(TABLE|COLUMN|DATABASE|SCHEMA)|TRUNCATE\s+TABLE)\b", + "Use expand/contract; gate destructive DDL behind an explicit CI acknowledgment.", + flags=re.I), + R("F2", "Bulk delete", "high", TS | PY, + r"\.(deleteMany|delete_many|destroy_all|delete_all|drop_all|removeMany)\s*\(\s*\)", + "Refuse an empty filter; cap the affected count and require confirmation above it."), + R("F2", "Recursive force remove", "high", ALL_EXTS, + r"rm\s+-[a-zA-Z]*[rR][a-zA-Z]*f|rm\s+-[a-zA-Z]*f[a-zA-Z]*[rR]", + "Validate the path is non-empty and inside the expected root before deleting."), + R("F4", "Config read away from startup", "medium", PY, + r"os\.(getenv|environ)", + "Parse the whole config into a typed object at startup so a missing key fails the deploy.", + negate=r"(settings|config|conf|env)\.py"), + R("F4", "Config read away from startup", "medium", TS, + r"process\.env\.\w+", + "Parse the whole config into a typed object at startup so a missing key fails the deploy.", + negate=r"(config|env|settings)\.(ts|js)"), + R("F7", "Unbounded read", "low", PY | TS, + r"\.read\s*\(\s*\)|\.readAll\s*\(|ioutil\.ReadAll", + "Cap the size at the boundary; an unbounded read is a resource-exhaustion incident."), + + # ---- C: contact ------------------------------------------------------------------- + R("C2", "Boolean flag parameter", "medium", TS, + r"\b\w+\s*:\s*boolean\s*[,)]", + "Use an enum, a named options object, or two functions; booleans are unreadable at the call site."), + R("C2", "Boolean flag parameter", "medium", GO, + r"func\s+\w+\s*\([^)]*\bbool\b[^)]*\)", + "Use a named option type; a bare bool is unreadable at the call site."), + R("C2", "Boolean default parameter", "medium", PY, + r"def\s+\w+\s*\([^)]*\b\w+\s*(:\s*bool\s*)?=\s*(True|False)", + "Use an enum, or at minimum make it keyword-only so the name appears at the call site."), + R("C5", "Duration without a unit", "medium", TS | GO | PY, + r"\b(timeout|delay|interval|ttl|expiry|duration|retryAfter|retry_after)\s*:?\s*(number|int|float|=\s*\d+)", + "Encode the unit in the type (Duration) or in the name (timeoutMs). Unit mismatches are silent."), + R("C6", "Money as a float", "high", PY | TS | GO | RS, + r"\b(price|amount|total|balance|cost|fee|subtotal|revenue)\w*\s*:\s*(float|number|f32|f64)\b" + r"|\bfloat\s*\(\s*\w*(price|amount|total|balance)", + "Use integer minor units in a Money type carrying its currency, or a decimal type."), + R("C7", "Unvalidated parse", "high", TS, + r"JSON\.parse\s*\(", + "Parse into a schema (zod/valibot) at the boundary; JSON.parse returns any."), + R("C7", "Unvalidated request body", "high", PY, + r"(request|req)\.(json|get_json)\s*\(\s*\)(?!\s*\))", + "Parse into a Pydantic model with extra='forbid' so unknown or missing fields fail loudly."), + R("C9", "Naive datetime", "medium", PY, + r"datetime\.utcnow\s*\(\s*\)|datetime\.now\s*\(\s*\)", + "Use datetime.now(timezone.utc), and inject a clock so time is testable."), + + # ---- M: motion-step --------------------------------------------------------------- + R("M4", "Unmanaged resource", "medium", PY, + r"^\s*(\w+\s*=\s*)?open\s*\(", + "Use a context manager; the error path will leak otherwise.", + negate=r"\bwith\b"), + R("M6", "Dangling async task", "high", PY, + r"^\s*(await\s+)?asyncio\.create_task\s*\(", + "Keep a reference; an unreferenced task can be garbage collected mid-flight (ruff RUF006).", + negate=r"=\s*(await\s+)?asyncio\.create_task"), + R("M6", "Unawaited promise-returning call", "low", TS, + r"^\s*\w+\.(save|update|create|delete|insert|write|send|publish|commit)\s*\(", + "If this returns a promise, await it: a floating write is silently lost. " + "Enable @typescript-eslint/no-floating-promises.", + negate=r"\b(await|return|yield|void)\b|\.then\(|=\s"), + R("M2", "Retryable effect without an idempotency key", "high", ALL_EXTS, + r"\b(def|func|function|fn|async\s+function)\s+\w*(charge|refund|capture|payout|transfer|" + r"sendEmail|send_email|publish|notify)\w*\s*[(<]", + "Require an idempotency key parameter, backed by a unique constraint on (entity, key).", + negate=r"idempot", flags=re.I), + R("M1", "Two-phase construction", "medium", ALL_EXTS, + r"\b(def|func|function|fn)\s+(init|initialize|connect|setup|configure|start)\s*[(<]", + "Have the constructor or a factory return a ready object, or use typestate; " + "'call this first' is not enforceable.", + negate=r"__init__|func\s+init\s*\(\s*\)\s*\{"), + + # ---- F1: exhaustiveness ----------------------------------------------------------- + R("F1", "Wildcard match arm", "medium", RS, + r"^\s*_\s*=>", + "In domain logic a wildcard turns a future compile error into a silent fallthrough."), + R("F1", "Switch without exhaustiveness check", "low", TS, + r"^\s*switch\s*\(", + "Add a default arm calling assertNever(x: never) so a new variant breaks the build."), + R("F1", "Switch without a default", "low", GO, + r"^\s*switch\s+\w+\s*\{", + "Enable the 'exhaustive' linter with default-signifies-exhaustive: false."), +] + +# Rules a real linter already does better. They stay available behind --all for repos that +# do not run those linters, but they are off by default: a tool that does eight things +# nothing else does is more useful than one doing forty things worse. The value here is the +# pointer, knowing which linter to enable beats a second-rate reimplementation of it. +COVERED_BY: dict[tuple[str, str], str] = { + ("X1", "Swallowed error"): "eslint no-empty", + ("X1", "Bare except"): "ruff E722", + ("X1", "Discarded error return"): "golangci-lint errcheck", + ("X2", "Unwrap / expect on a fallible value"): "clippy::unwrap_used", + ("X2", "Silent default on error"): "clippy", + ("X2", "parseInt without radix"): "eslint radix", + ("X3", "Focused test disables the suite"): "eslint jest/no-focused-tests", + ("X3", "Skipped test"): "eslint jest/no-disabled-tests", + ("X4", "Type-checker suppression"): "@typescript-eslint/ban-ts-comment, mypy --strict", + ("X4", "Explicit any"): "@typescript-eslint/no-explicit-any", + ("X4", "Untyped container"): "golangci-lint", + ("X4", "unsafe block"): "clippy", + ("X5", "Mutable default argument"): "ruff B006", + ("C9", "Naive datetime"): "ruff DTZ", + ("M4", "Unmanaged resource"): "ruff SIM115", + ("M6", "Dangling async task"): "ruff RUF006", + ("F1", "Wildcard match arm"): "clippy::wildcard_enum_match_arm", + ("F7", "Unbounded read"): "", + ("F3", "assert used for validation"): "ruff S101", + ("C6", "Equality comparison on a float"): "ruff PLR0133", +} + + +# poka-yoke: keyword-only, so the id and the name cannot be passed transposed [control] +def covered(*, rule_id: str, name: str) -> str: + return COVERED_BY.get((rule_id, name), "") + + +# -------------------------------------------------------------------------------------- +# AST pass (Python only), catches what regexes can't see +# -------------------------------------------------------------------------------------- + + +def python_ast_findings(path: Path, source: str) -> list[dict]: + """Structural checks that need real parsing: adjacent same-type params, assert-as- + validation, and equality comparison on floats.""" + out = [] + try: + tree = ast.parse(source, filename=str(path)) + except SyntaxError: + return out + + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + args = node.args.posonlyargs + node.args.args + # skip self/cls + if args and args[0].arg in ("self", "cls"): + args = args[1:] + annotated = [(a.arg, ast.unparse(a.annotation)) for a in args if a.annotation] + for i in range(len(annotated) - 1): + (n1, t1), (n2, t2) = annotated[i], annotated[i + 1] + if t1 == t2 and t1 in ("str", "int", "float", "bytes", "bool", "UUID"): + out.append({ + "id": "C1", + "name": "Adjacent same-type parameters", + "severity": "high", + "line": node.lineno, + "snippet": f"def {node.name}(..., {n1}: {t1}, {n2}: {t2}, ...)", + "device": f"'{n1}' and '{n2}' are both {t1} and can be swapped silently. " + "Use NewType per concept, or make them keyword-only.", + }) + # positional args on a wide signature + if len(args) >= 4 and not node.args.kwonlyargs: + out.append({ + "id": "C1", + "name": "Wide positional signature", + "severity": "low", + "line": node.lineno, + "snippet": f"def {node.name}({len(args)} positional params)", + "device": "Make parameters keyword-only with '*' so names appear at the call site.", + }) + + elif isinstance(node, ast.Assert): + out.append({ + "id": "F3", + "name": "assert used for validation", + "severity": "medium", + "line": node.lineno, + "snippet": ast.unparse(node)[:100], + "device": "assert is stripped under python -O. Raise an explicit exception instead.", + }) + + elif isinstance(node, ast.Compare): + for op in node.ops: + if isinstance(op, (ast.Eq, ast.NotEq)): + src = ast.unparse(node) + if re.search(r"\d+\.\d+", src): + out.append({ + "id": "C6", + "name": "Equality comparison on a float", + "severity": "medium", + "line": node.lineno, + "snippet": src[:100], + "device": "Use math.isclose, or a Decimal/integer-minor-unit type.", + }) + return out + + +# -------------------------------------------------------------------------------------- +# File and diff collection +# -------------------------------------------------------------------------------------- + +SKIP_DIRS = { + ".git", "node_modules", "vendor", "dist", "build", "target", "__pycache__", + ".venv", "venv", ".tox", ".mypy_cache", ".pytest_cache", ".next", "coverage", + ".terraform", "site-packages", +} + + +class GitUnavailable(RuntimeError): + """git could not answer the question asked of it. + + Previously any git failure became an empty string, which the caller could not tell from + "the tree is clean". A detector that reports a clean bill of health because git is broken + is the exact failure this file's own rules exist to catch. + """ + + +def git(*args: str, cwd: Path) -> str: + try: + r = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True, timeout=30) + except (subprocess.SubprocessError, FileNotFoundError) as exc: + raise GitUnavailable(f"could not run git {' '.join(args)}: {exc}") from exc + if r.returncode != 0: + detail = (r.stderr or r.stdout or "").strip().splitlines() + raise GitUnavailable(f"git {' '.join(args)} exited {r.returncode}" + + (f": {detail[0]}" if detail else "")) + return r.stdout + + +def changed_files_and_lines(cwd: Path, mode: str, since: str | None): + """Return {path: set(changed_line_numbers)}. Empty set means 'whole file'.""" + if mode == "staged": + diff_args = ["diff", "--cached", "-U0"] + elif mode == "since": + diff_args = ["diff", f"{since}..HEAD", "-U0"] + else: + diff_args = ["diff", "HEAD", "-U0"] + + raw = git(*diff_args, cwd=cwd) + if not raw.strip() and mode == "diff": + # Clean tree, fall back to recent commits, which is what the user usually means. + raw = git("diff", "HEAD~5..HEAD", "-U0", cwd=cwd) + + result: dict[str, set[int]] = {} + current = None + for line in raw.splitlines(): + if line.startswith("+++ b/"): + current = line[6:] + result.setdefault(current, set()) + elif line.startswith("@@") and current: + m = re.search(r"\+(\d+)(?:,(\d+))?", line) + if m: + start = int(m.group(1)) + count = int(m.group(2) or 1) + result[current].update(range(start, start + count)) + return {k: v for k, v in result.items() if v} + + +def collect_paths(roots: list[str]) -> list[Path]: + out = [] + for root in roots: + p = Path(root) + if p.is_file(): + if p.suffix in ALL_EXTS: + out.append(p) + elif p.is_dir(): + for dirpath, dirnames, filenames in os.walk(p): + dirnames[:] = [d for d in dirnames if d not in SKIP_DIRS and not d.startswith(".")] + for fn in filenames: + fp = Path(dirpath) / fn + if fp.suffix in ALL_EXTS: + out.append(fp) + return out + + +# -------------------------------------------------------------------------------------- +# Scanning +# -------------------------------------------------------------------------------------- + +COMMENT_ONLY = re.compile(r"^\s*(//|#|/\*|\*|--)") + + +def scan_file(path: Path, only_lines: set[int] | None) -> list[dict]: + try: + source = path.read_text(encoding="utf-8", errors="replace") + except (OSError, UnicodeDecodeError): + return [] + if len(source) > 2_000_000: + return [] + + findings = [] + ext = path.suffix + lines = source.splitlines() + + for lineno, line in enumerate(lines, 1): + if only_lines and lineno not in only_lines: + continue + if COMMENT_ONLY.match(line) or len(line) > 500: + continue + for rule in RULES: + if ext not in rule.exts: + continue + if not INCLUDE_COVERED and covered(rule_id=rule.id, name=rule.name): + continue + if rule.negate and (rule.negate.search(line) or rule.negate.search(str(path))): + continue + if rule.pattern.search(line): + findings.append({ + "id": rule.id, + "name": rule.name, + "severity": rule.severity, + "line": lineno, + "snippet": line.strip()[:120], + "device": rule.device, + }) + + if ext in PY: + for f in python_ast_findings(path, source): + if not INCLUDE_COVERED and covered(rule_id=f["id"], name=f["name"]): + continue + if not only_lines or f["line"] in only_lines: + findings.append(f) + + for f in findings: + f["file"] = str(path) + f["lens"] = LENS.get(f["id"][0], "unknown") + return findings + + +# -------------------------------------------------------------------------------------- +# Output +# -------------------------------------------------------------------------------------- + +INCLUDE_COVERED = False + +SEV_ORDER = {"high": 0, "medium": 1, "low": 2} +COLOR = {"high": "\033[31m", "medium": "\033[33m", "low": "\033[90m"} +RESET = "\033[0m" + + +def render(findings: list[dict], scope: str, use_color: bool) -> str: + if not findings: + return f"No hazards detected in {scope}.\n\nThe lenses still apply, run them by hand:\n" \ + " contact: can the wrong thing fit?\n" \ + " fixed-value: can an incomplete or wrong-sized set pass?\n" \ + " motion-step: can the steps happen in the wrong order?" + + findings.sort(key=lambda f: (SEV_ORDER[f["severity"]], f["file"], f["line"])) + counts = {"high": 0, "medium": 0, "low": 0} + for f in findings: + counts[f["severity"]] += 1 + + out = [ + f"Poka-yoke hazard scan, {scope}", + f"{counts['high']} high · {counts['medium']} medium · {counts['low']} low", + "", + "Heuristics with real false positives. Read the surrounding code before acting.", + "", + ] + + grouped: dict[str, list[dict]] = {} + for f in findings: + grouped.setdefault(f"{f['id']} {f['name']}", []).append(f) + + for key, group in sorted(grouped.items(), key=lambda kv: SEV_ORDER[kv[1][0]["severity"]]): + sev = group[0]["severity"] + tag = f"{COLOR[sev]}{sev.upper():<6}{RESET}" if use_color else f"{sev.upper():<6}" + out.append(f"{tag} {key} ({group[0]['lens']} lens, {len(group)} site" + f"{'s' if len(group) > 1 else ''})") + out.append(f" device: {group[0]['device']}") + for f in group[:8]: + out.append(f" {f['file']}:{f['line']} {f['snippet']}") + if len(group) > 8: + out.append(f" … and {len(group) - 8} more") + out.append("") + + return "\n".join(out) + + +def main() -> int: + ap = argparse.ArgumentParser( + description="Detect poka-yoke hazards, shapes in code that make mistakes easy.", + formatter_class=argparse.RawDescriptionHelpFormatter, + epilog=__doc__.split("Examples:")[-1], + ) + src = ap.add_mutually_exclusive_group() + src.add_argument("--diff", action="store_true", + help="scan uncommitted changes (falls back to HEAD~5..HEAD if clean)") + src.add_argument("--staged", action="store_true", help="scan staged changes") + src.add_argument("--since", metavar="REF", help="scan changes since REF (e.g. HEAD~10)") + src.add_argument("--paths", nargs="+", metavar="PATH", help="scan these files or directories") + ap.add_argument("--severity", choices=["high", "medium", "low"], default="low", + help="minimum severity to report (default: low)") + # Until this existed the script ended in a bare `return 0`, so every gate built on it was + # decorative: the shipped pre-commit hook, the shipped CI template and this repo's own + # "Detector runs clean" step all reported success while printing high-severity findings. + # A linter that cannot fail is a linter nobody has to satisfy. + ap.add_argument("--fail-on", choices=["high", "medium", "low", "none"], default="low", + metavar="SEVERITY", + help="exit non-zero when a finding of at least this severity is reported " + "(default: low, i.e. any reported finding). Use 'none' to report " + "without gating.") + ap.add_argument("--id", nargs="+", metavar="ID", + help="only report these hazard IDs (e.g. --id C1 F2 M2)") + ap.add_argument("--all", action="store_true", dest="include_covered", + help="also run the rules a real linter does better (off by default)") + ap.add_argument("--json", action="store_true", help="emit JSON") + ap.add_argument("--repo", default=".", help="repository root (default: .)") + args = ap.parse_args() + + global INCLUDE_COVERED + INCLUDE_COVERED = args.include_covered + repo = Path(args.repo).resolve() + findings: list[dict] = [] + + # poka-yoke: an empty scan reports itself instead of looking like a clean bill of health [control] + scanned = 0 + + if args.paths: + scope = f"paths: {', '.join(args.paths)}" + for p in collect_paths(args.paths): + scanned += 1 + findings += scan_file(p, None) + empty_because = f"Nothing under {', '.join(args.paths)} has a supported extension." + else: + mode = "staged" if args.staged else ("since" if args.since else "diff") + scope = {"staged": "staged changes", + "since": f"changes since {args.since}", + "diff": "uncommitted changes"}[mode] + try: + changed = changed_files_and_lines(repo, mode, args.since) + except GitUnavailable as exc: + # Exit 2, the same code --paths uses for "scanned nothing". Reporting a clean + # tree because git is broken is worse than reporting nothing at all: a + # pre-commit hook or CI gate reads only the exit code. + msg = (f"Could not determine what changed: {exc}\n" + f"This is NOT an all-clear. Use --paths to scan explicitly.") + print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, + "findings": [], "error": str(exc)}, indent=2) + if args.json else msg, file=sys.stdout if args.json else sys.stderr) + return 2 + for rel, lines in changed.items(): + fp = repo / rel + if fp.suffix in ALL_EXTS and fp.exists(): + scanned += 1 + findings += scan_file(fp, lines) + empty_because = ( + "No changed files found: the tree may be clean, or this may not be a git " + "repository." if not changed else + f"None of the {len(changed)} changed file(s) could be scanned. They were " + "deleted, or have no supported extension.") + + # poka-yoke: one exit for "scanned nothing", shared by every mode [control] + # + # This check used to live inside the --paths branch. --diff, --staged and --since each + # reached the end with scanned == 0 and returned 0, printing "No hazards detected" -- + # a false all-clear in precisely the modes a pre-commit hook and a CI gate use. The + # marker above said [control] while holding on one branch of three. + # + # It is out here now because a check placed after the branches cannot be present on one + # and missing from another. Adding a fourth input mode inherits it without remembering to. + if scanned == 0: + msg = (f"Scanned 0 files. This is NOT an all-clear.\n{empty_because}\n" + f"Supported extensions: {', '.join(sorted(ALL_EXTS))}\n" + "Use --paths to scan explicitly, e.g. detect_hazards.py --paths src/") + print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, + "findings": [], "error": msg}, indent=2) + if args.json else msg, file=sys.stdout if args.json else sys.stderr) + return 2 + + threshold = SEV_ORDER[args.severity] + findings = [f for f in findings if SEV_ORDER[f["severity"]] <= threshold] + if args.id: + wanted = {i.upper() for i in args.id} + findings = [f for f in findings if f["id"] in wanted] + + if args.json: + print(json.dumps({"scope": scope, "files_scanned": scanned, + "count": len(findings), "findings": findings}, indent=2)) + else: + print(render(findings, scope, use_color=sys.stdout.isatty())) + print(f"\nScanned {scanned} file{'' if scanned == 1 else 's'}.") + if not INCLUDE_COVERED: + tools = sorted({v.split(",")[0].split()[0] for v in COVERED_BY.values() if v}) + # len(COVERED_BY) counts ENTRIES, and one entry can suppress several + # per-language rules, so it under-reported by three. Count the rules. + n_suppressed = sum(1 for r in RULES if (r.id, r.name) in COVERED_BY) + # Names the linters rather than a path. `assets/devices/lint/` resolves only + # when this script runs from inside the full plugin; installed as a standalone + # skill it pointed at a directory the user does not have. + print(f"\nNot checked here, {n_suppressed} further hazard rules are covered " + f"better by {', '.join(tools)}.\nEnable those in your own linter config " + f"rather than relying on this. Use --all to run them anyway.") + + if args.fail_on != "none": + rank = {"high": 3, "medium": 2, "low": 1} + threshold = rank[args.fail_on] + gating = [f for f in findings if rank.get(f.get("severity", "low"), 1) >= threshold] + if gating: + worst = max(rank.get(f.get("severity", "low"), 1) for f in gating) + name = {3: "high", 2: "medium", 1: "low"}[worst] + if not args.json: + print(f"\n{len(gating)} finding(s) at or above --fail-on={args.fail_on} " + f"(worst: {name}). Exiting 1.", file=sys.stderr) + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/poka-yoke-audit/scripts/device_registry.py b/skills/poka-yoke-audit/scripts/device_registry.py new file mode 100755 index 000000000..240fcbe68 --- /dev/null +++ b/skills/poka-yoke-audit/scripts/device_registry.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +"""Generate a device registry by reading the code, never by remembering to update a file. + +An earlier version of this plugin told people to hand-maintain `docs/poka-yoke/registry.md` +listing every device and the mistake it prevents. That was rung zero by the plugin's own +argument: a Markdown file someone has to remember to update is training, not a device, and it +goes stale exactly when it matters: the moment someone deletes a "redundant" constraint. + +The fix is to keep the rationale where the device is, as a marker comment, and generate the +index from those markers. A generated index cannot drift, because there is nothing to forget. +Delete the constraint and its row disappears; move it and the row follows. + +Mark a device at its site, in whatever comment syntax the file uses: + + # poka-yoke: rejects a second charge for the same idempotency key [control] + -- poka-yoke: refuses a zero or negative amount [control] + // poka-yoke: forgetting to await this write would lose it silently [warning] + +The rung in brackets is optional and defaults to unstated. Then: + + python3 scripts/device_registry.py # print the registry + python3 scripts/device_registry.py --write docs/poka-yoke/registry.md + python3 scripts/device_registry.py --check # CI: fail if the file is stale +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +MARKER = re.compile( + # A bare "*" prefix was tried and removed: it matches markdown bold (**poka-yoke: ...**), + # so prose about the plugin was being catalogued as devices. Require a real comment token. + r"""(?:^|\s)(?:\#|//|--|/\*|)?\s*$""", + re.IGNORECASE, +) + +# Generated benchmark transcripts are data, not code, and they discuss the plugin at length. +SKIP = {"results", ".git", "node_modules", "vendor", "dist", "build", "target", "__pycache__", + ".venv", "venv", ".next", "coverage", ".terraform"} + +# No path in the header. It used to hard-code this repository's own directory layout, so a +# reader who installed the skill on its own was told to run a path that does not exist for +# them. +# +# Deriving the path from argv instead would be worse, not better: the generated text would +# then depend on how the caller happened to invoke the script, and `--check` compares +# generated text against the committed file. CI invoking it by a different path than the +# author did would report the registry stale when nothing had changed. +HEADER = """ + +# Device Registry + +Every mistake-proofing device in this repository, and the mistake each one prevents. The +"prevents" column is the one that matters: it is what stops a future engineer removing a +device that has never fired because it is doing its job. + +| Device | Prevents | Rung | +|---|---|---| +""" + + +def tracked_files(root: Path) -> list[Path]: + """Prefer git's file list so ignored files are skipped for free.""" + try: + r = subprocess.run(["git", "ls-files"], cwd=root, capture_output=True, + text=True, timeout=30) + if r.returncode == 0 and r.stdout.strip(): + return [root / p for p in r.stdout.splitlines()] + except (subprocess.SubprocessError, FileNotFoundError): + pass + return [p for p in root.rglob("*") + if p.is_file() and not any(s in p.parts for s in SKIP)] + + +def scan(root: Path) -> list[tuple[str, str, str]]: + rows = [] + for f in tracked_files(root): + if any(s in f.parts for s in SKIP): + continue + if not f.is_file() or f.suffix in {".png", ".jpg", ".svg", ".pdf", ".lock"}: + continue + try: + text = f.read_text(encoding="utf-8", errors="ignore") + except OSError: + continue + if "poka-yoke:" not in text: + continue + # A marker inside a docstring or a fenced block is documentation showing the + # syntax, not a device guarding anything. This generator's own module docstring + # demonstrates three of them, and all three were catalogued as real devices and + # counted into the badge. A registry that inflates itself by reading its own + # example is exactly the kind of instrument this repository exists to catch. + in_doc = False + for i, line in enumerate(text.splitlines(), 1): + stripped = line.strip() + if stripped.startswith("```"): + in_doc = not in_doc + continue + if stripped.count('"""') % 2 or stripped.count("'''") % 2: + in_doc = not in_doc + continue + if in_doc: + continue + m = MARKER.search(line) + if m: + rel = f.relative_to(root) + rows.append((f"`{rel}:{i}`", m.group("what").strip(), + # A marker may omit the bracketed rung. "unstated" says so; + # the previous fallback of ", " rendered a stray comma into + # the table cell and read as a formatting bug. + (m.group("rung") or "unstated").capitalize())) + return sorted(rows, key=lambda r: r[0]) + + +def render(rows: list[tuple[str, str, str]]) -> str: + if not rows: + return (HEADER + "| _none found_ | Add `poka-yoke:` marker comments at your " + "devices | unstated |\n") + return HEADER + "".join(f"| {d} | {w} | {r} |\n" for d, w, r in rows) + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter) + ap.add_argument("--root", default=".") + ap.add_argument("--write", metavar="PATH", help="write the registry to PATH") + ap.add_argument("--check", action="store_true", + help="exit non-zero if --write target is missing or stale (for CI)") + a = ap.parse_args() + + root = Path(a.root).resolve() + out = render(scan(root)) + + if a.check: + target = Path(a.write) if a.write else root / "docs/poka-yoke/registry.md" + if not target.exists(): + print(f"registry missing: {target}", file=sys.stderr); return 1 + if target.read_text() != out: + print(f"registry is stale: {target}\n" + f"regenerate with: --write {target}", file=sys.stderr) + return 1 + print(f"registry up to date: {target}") + return 0 + + if a.write: + t = Path(a.write); t.parent.mkdir(parents=True, exist_ok=True) + t.write_text(out) + print(f"wrote {t} ({out.count(chr(10)) - HEADER.count(chr(10))} devices)") + else: + print(out, end="") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/poka-yoke-design/SKILL.md b/skills/poka-yoke-design/SKILL.md new file mode 100644 index 000000000..ee2877c3a --- /dev/null +++ b/skills/poka-yoke-design/SKILL.md @@ -0,0 +1,107 @@ +--- +name: poka-yoke-design +description: 'Design APIs, schemas, types and state machines so misuse cannot be expressed. Use when writing a new interface and someone asks "what should the types look like", "make invalid states unrepresentable", "so callers cannot screw it up", or wants illegal state transitions rejected. Covers branded types, discriminated unions, typestate, parse-don''t-validate. For code that already exists use audit.' +license: MIT +--- + +# Poka-Yoke Design + +Mistake-proofing is cheapest before the code exists. Once an interface has callers, every +device you add is a migration; before it has callers, a device is free. So the work here is +front-loaded: decide how this thing will be misused, *then* pick the shape that makes the +misuse unsayable. + +This is Shingo's **source inspection**: checking the conditions that produce errors rather +than the errors themselves, and it is the strongest of his three inspection types, because +the error never gets the chance to happen. + +## The ritual: enumerate misuse before you write the signature + +Two minutes on this list determines the design. + +1. **Can any two parameters be swapped without complaint?** Same type adjacent to same type is + among the most common footguns in software. +2. **What must a caller remember to do?** Call something first, close a handle, pass the right + units. Every "must remember" is a defect scheduled for later. +3. **Which combinations of state are nonsense?** If you can construct a value that means + nothing, the type is wrong. +4. **What happens on the second call?** Retries, double-clicks, at-least-once queues. If the + answer is "it charges twice", you need a motion-step device. +5. **What is the worst plausible input?** Empty set, enormous set, null, wrong tenant, + yesterday's token, a string from an attacker. +6. **When someone adds a case next year, what breaks?** The right answer is "the build". + +Write the answers where the user can see them, then design against them. + +## The moves, in preference order + +Reach for the highest one the language allows. Each rung down is a real concession — take it +consciously and say why. + +### 1. Make the illegal state unrepresentable (Control, contact lens) + +Change the type so the bad value has no spelling. Distinct types for distinct concepts: +`UserId` and `OrderId` are not both `string`, money is not a float. Sum types over bags of +optionals: `{status, error?, data?, retryAt?}` permits "succeeded with an error"; a +discriminated union permits exactly the states that exist. A struct with N optional fields +claims 2^N states are legal — ask how many actually are. + +### 2. Parse, don't validate (Control at the boundary) + +Validation returns a boolean and throws the knowledge away. Parsing returns a *type* that +carries the proof: `parseEmail(s): Email | Error` means every downstream function taking +`Email` cannot receive garbage. Do it once, at the edge — HTTP handlers, queue consumers, +config loading, third-party responses. Inside the boundary, work only with parsed types. + +### 3. Make order and lifecycle enforceable (Control, motion-step lens) + +Encode the sequence in types rather than prose. Typestate: each operation consumes one state +and returns the next, so `.commit()` does not exist on an unvalidated value. Builders that +cannot `build()` until required steps have run. Constructors that return ready objects — if +`init()` must be called before use, the constructor is doing the wrong job. Scope-bound +resources (context managers, `defer`, RAII) rather than "remember to close". Idempotency keys +as *required* parameters for anything moving money or mutating external state; an optional +idempotency key is a suggestion, and suggestions are rung zero. + +### 4. Make completeness checkable (Control/Warning, fixed-value lens) + +Exhaustive matching with a compiler-enforced unreachable arm, so adding an enum variant breaks +the build at every site that must change — one line per switch, and among the highest-leverage +devices there is. Required arguments where there is no safe default: a default that is wrong +half the time hides the decision. Whole-config validation at startup, so a missing variable +fails the deploy rather than the 3am request. + +### 5. Fail fast and loud (Warning) + +When the type system genuinely cannot express the constraint, assert at the boundary and throw, +with a message naming the mistake and the fix. Two rules decide whether this rung works at all: + +- **No silent fallbacks.** `catch {}`, `except: pass`, `|| default`, `unwrap_or_default()` on an + error path are devices *removed* — they convert a loud mistake into a quiet one. If a fallback + is genuinely correct, say which failure it absorbs and why that failure is expected. +- **Destructive operations default to safe.** Dry-run by default; refuse an empty or oversized + set. `deleteUsers(filter)` with an empty filter should raise, not truncate the table. + +`references/hazard-catalog.md` ships beside this skill: the catalogue of recurring hazard +shapes, with the lens that finds each one and the device that closes it. Read it while +enumerating misuse above, which is the step it is built for. + +### 6. Where the language can't help, use the data layer + +The database is a type system every service shares. `NOT NULL`, `CHECK`, `UNIQUE`, foreign keys +and partial unique indexes hold even when someone connects with `psql` or ships a service in +another language. When application-level enforcement is the only thing between you and corrupt +data, push it down. + +## Deliver the design with its reasoning attached + +You were asked for code, so write the code. But narrate the mistake-proofing in a few lines, +because the reasoning is what stops it being undone later: + +- what misuses you enumerated, +- which ones the design now makes impossible, and at which rung, +- which ones you consciously left possible, and why. + +That last bullet matters most. Every design leaves something possible; naming it is the +difference between a considered tradeoff and an oversight. + diff --git a/skills/poka-yoke-design/references/hazard-catalog.md b/skills/poka-yoke-design/references/hazard-catalog.md new file mode 100644 index 000000000..b2e1c56ba --- /dev/null +++ b/skills/poka-yoke-design/references/hazard-catalog.md @@ -0,0 +1,415 @@ +# Hazard Catalog + +The recurring shapes that produce mistakes, organized by the lens that finds them. Each entry: +what to look for, why it bites, and the device that closes it with the rung it reaches. + +Use this as working vocabulary, not a checklist to run top to bottom. The lens questions are +the real tool; this catalog is what the lenses usually turn up. + +## Contents + +- [Contact lens, can the wrong thing fit?](#contact-lens-can-the-wrong-thing-fit) + - [C1. Adjacent same-type parameters](#c1-adjacent-same-type-parameters) + - [C2. Boolean flag parameters](#c2-boolean-flag-parameters) + - [C3. Primitive obsession at boundaries](#c3-primitive-obsession-at-boundaries) + - [C4. Stringly-typed enums](#c4-stringly-typed-enums) + - [C5. Implicit units and magnitudes](#c5-implicit-units-and-magnitudes) + - [C6. Money as a float](#c6-money-as-a-float) + - [C7. Unvalidated external input](#c7-unvalidated-external-input) + - [C8. Bag-of-optionals structs](#c8-bag-of-optionals-structs) + - [C9. Naive datetimes](#c9-naive-datetimes) +- [Fixed-value lens, can an incomplete or wrong-sized set pass?](#fixed-value-lens-can-an-incomplete-or-wrong-sized-set-pass) + - [F1. Non-exhaustive branching](#f1-non-exhaustive-branching) + - [F2. Unbounded destructive operations](#f2-unbounded-destructive-operations) + - [F3. Defaults that hide a decision](#f3-defaults-that-hide-a-decision) + - [F4. Config discovered missing at runtime](#f4-config-discovered-missing-at-runtime) + - [F5. Partial writes without a transaction](#f5-partial-writes-without-a-transaction) + - [F6. Invariants enforced only in the application](#f6-invariants-enforced-only-in-the-application) + - [F7. Unbounded input](#f7-unbounded-input) +- [Motion-step lens, can the order be wrong?](#motion-step-lens-can-the-order-be-wrong) + - [M1. Temporal coupling](#m1-temporal-coupling) + - [M2. Non-idempotent retryable effects](#m2-non-idempotent-retryable-effects) + - [M3. Illegal state transitions](#m3-illegal-state-transitions) + - [M4. Resources that must be released](#m4-resources-that-must-be-released) + - [M5. Check-then-act races](#m5-check-then-act-races) + - [M6. Fire-and-forget async](#m6-fire-and-forget-async) + - [M7. Order-dependent migrations and deploys](#m7-order-dependent-migrations-and-deploys) +- [Cross-cutting, devices that were removed](#cross-cutting-devices-that-were-removed) + - [X1. Swallowed errors](#x1-swallowed-errors) + - [X2. Silent coercion and fallback](#x2-silent-coercion-and-fallback) + - [X3. Disabled tests](#x3-disabled-tests) + - [X4. Escape hatches in the type system](#x4-escape-hatches-in-the-type-system) + - [X5. Mutable shared defaults](#x5-mutable-shared-defaults) + +--- + +## Contact lens, can the wrong thing fit? + +The factory analogy: a part that only seats one way. In software, the type is the shape. + +### C1. Adjacent same-type parameters + +**Signal**: two or more consecutive parameters of the same primitive type, `transfer(from: string, to: string)`, `resize(w: number, h: number)`, +`slice(start: int, end: int)`. + +**Why it bites**: swapping them compiles, passes review, and produces a plausible wrong +result. It is among the most common footguns in software, and one of the most cleanly +solved, once the two types differ, the wrong order will not compile. + +**Device**: distinct types per concept, branded types, newtypes, value objects, so a +`SourceAccount` cannot be passed as a `DestinationAccount`. **Control.** +Fallback where types can't help: force keyword/named arguments so the caller must write the +name at the call site. **Warning**, but nearly free and it makes the swap visible in review. + +### C2. Boolean flag parameters + +**Signal**: `createUser(name, true, false)`, `save(data, force=True)`, any `bool` parameter +that selects behavior rather than carrying data. + +**Why it bites**: the call site is unreadable, so misordered or misunderstood flags are +invisible. Adding a second boolean makes it exponentially worse. + +**Device**: an enum or literal union per axis (`Visibility.Public`), an options object with +named fields, or two separate functions. **Control** for the enum, since the wrong value has +no spelling. Note the exception: a single boolean whose name reads correctly at the call site +in a keyword-argument language is fine. + +### C3. Primitive obsession at boundaries + +**Signal**: `string` for email, URL, path, token, tenant ID, phone; `int` for a percentage or +a duration, especially on public functions. + +**Why it bites**: every downstream function must re-check or trust. Validation that returns a +boolean throws away the proof, so the check gets repeated, skipped, or done inconsistently. + +**Device**: parse-don't-validate. `parseEmail(s): Email | Error` once at the boundary, then +downstream signatures demand `Email`. The type carries the guarantee permanently. **Control.** + +### C4. Stringly-typed enums + +**Signal**: `status: string` with a comment listing the values; string comparison against +literals; a value crossing a boundary as text with no schema. + +**Why it bites**: typos compile. New variants added elsewhere never reach this code. Nothing +tells you which values are legal. + +**Device**: a literal union, enum, or sealed class, with exhaustive matching (F1). **Control.** + +### C5. Implicit units and magnitudes + +**Signal**: `timeout: number`, `distance: float`, `retryAfter: int`: no unit anywhere except +possibly a name or a comment. Two systems in the same codebase disagreeing on seconds vs +milliseconds. + +**Why it bites**: a 1000x error is silent and looks like a hang or a hot loop. This class of +mistake famously destroyed a Mars orbiter. + +**Device**: unit-bearing types (`Duration`, `Milliseconds`), or at minimum encode the unit in +the parameter name (`timeoutMs`). **Control** for the type. The name is **rung 0**: it makes +a mismatch visible to a reader who is looking, and produces no diagnostic for one who is not. +Worth doing; not a device. + +### C6. Money as a float + +**Signal**: `price: float`, `amount: number`, arithmetic on currency in binary floating point, +`==` comparisons on money. + +**Why it bites**: 0.1 + 0.2 ≠ 0.3. Errors accumulate over aggregation and reconciliation +fails in ways that take days to trace. + +**Device**: integer minor units (cents) in a `Money` type carrying its currency, or a decimal +type. Mixed-currency arithmetic should not typecheck. **Control.** + +### C7. Unvalidated external input + +**Signal**: `JSON.parse(body)` into `any`, `request.json()` into a bare dict, a third-party +API response used field-by-field with no schema, `os.environ[...]` read deep inside logic. + +**Why it bites**: the failure surfaces far from the boundary, as a confusing error about a +missing property, long after the malformed data has been partially processed or stored. + +**Device**: a schema at every edge, zod/valibot, Pydantic, `encoding/json` into a typed +struct with validation, serde. Parse once, then work with parsed types. **Control.** +This applies to *your own* services' responses too; "internal" is not a guarantee. + +### C8. Bag-of-optionals structs + +**Signal**: a type with several optional fields where only certain combinations are +meaningful, `{ status, data?, error?, retryAt? }`, `{ isLoading, data, error }`. + +**Why it bites**: N optional fields claim 2^N legal states. Every consumer must guess which +are real, and they guess differently. States like "loading and errored with data" become +reachable and get handled inconsistently. + +**Device**: a discriminated union with exactly the legal variants, so impossible combinations +have no representation. **Control.** This is the canonical "make invalid states +unrepresentable" move. + +### C9. Naive datetimes + +**Signal**: timezone-less timestamps, `datetime.now()` / `new Date()` scattered through +business logic, dates stored as strings, DST-unaware arithmetic. + +**Why it bites**: correct in the developer's timezone, wrong in production, and wrong twice a +year in the places that observe DST. Also hard to test, logic that reads the clock directly +cannot be exercised at a boundary condition without freezing or injecting time. + +**Device**: timezone-aware types everywhere, UTC at rest, an injected clock so time is a +parameter rather than an ambient read. **Control** for the type, and the injected clock buys +testability, which is a Detection-rung device that finally becomes possible. + +--- + +## Fixed-value lens, can an incomplete or wrong-sized set pass? + +The factory analogy: a counter confirming all six screws were fitted. + +### F1. Non-exhaustive branching + +**Signal**: a `switch`/`match` over an enum with a `default` that does nothing meaningful, or +an if/else chain over a closed set of values. + +**Why it bites**: adding a variant silently takes the default branch at every site that +should have been updated. The bug appears months later, in the one code path nobody tested. + +**Device**: compiler-enforced exhaustiveness: an `assertNever(x: never)` arm in TypeScript, +`match` without a catch-all in Rust, `assert_never` with mypy, an exhaustive linter for Go. +**Control**, one line per switch, and among the highest-leverage devices available. + +### F2. Unbounded destructive operations + +**Signal**: `DELETE`/`UPDATE` built from a filter that can be empty; `rm -rf "$VAR"`; +`.deleteMany(where)`; bulk send/publish over a query result; a "cleanup" job with no cap. + +**Why it bites**: irreversible, instant, and proportional to your data volume. An empty filter +frequently means "match everything." + +**Device**: refuse an empty predicate; require an explicit `all=True` for the full-table case; +cap the affected row count and require confirmation above it; dry-run by default with the +count printed. Soft-delete where the domain allows. **Control.** + +### F3. Defaults that hide a decision + +**Signal**: a default value for something with no safe default, `retries=3`, `timeout=30`, +`currency="USD"`, `tenant=None`, `region=default`. + +**Why it bites**: the caller never considers the parameter, and the default is wrong for their +case. Worse than an error, because it produces confident wrong behavior. + +**Device**: make it required. Reserve defaults for parameters where one value is correct for +the overwhelming majority and wrong-but-harmless for the rest. **Control.** + +### F4. Config discovered missing at runtime + +**Signal**: `os.getenv("X")` inside a request handler; config read lazily on first use; a +missing key producing `None` that flows onward. + +**Why it bites**: the service starts, passes health checks, and fails on the one code path +that needs the key, often the payment path, often at 3am. + +**Device**: parse and validate the entire config into a typed object at startup, and exit +non-zero if anything is missing or malformed. Every consumer takes the typed object. +**Control**, and it converts a 3am page into a failed deploy. + +### F5. Partial writes without a transaction + +**Signal**: several writes in sequence with no transaction; a write followed by an external +call followed by another write; "create the record then send the email." + +**Why it bites**: a failure in the middle leaves the system in a state your code does not +model and cannot repair. + +**Device**: wrap in a transaction; move external effects outside it via an outbox; make the +sequence idempotent so replay converges. **Control** for the transaction. + +### F6. Invariants enforced only in the application + +**Signal**: uniqueness checked with a `SELECT` before an `INSERT`; nullability enforced in a +model class but not in the column; a foreign key relationship maintained by convention. + +**Why it bites**: the check races under concurrency, and it is bypassed entirely by any other +service, migration, script, or human with `psql`. + +**Device**: push it into the schema, `NOT NULL`, `UNIQUE`, `CHECK`, foreign keys, partial +unique indexes. The database is a type system shared by everything that touches the data. +**Control**, and uniquely durable. + +### F7. Unbounded input + +**Signal**: pagination with no maximum page size; a file upload with no size limit; a query +built from a user-supplied list with no cap; unbounded recursion or retries. + +**Why it bites**: a resource exhaustion incident indistinguishable from an attack, triggered +by an ordinary user with a large account. + +**Device**: explicit caps at the boundary, enforced by the parsing type where possible. +**Control.** + +--- + +## Motion-step lens, can the order be wrong? + +The factory analogy: a sensor confirming step 3 happened before step 4. + +### M1. Temporal coupling + +**Signal**: `init()`, `connect()`, `configure()`, `validate()` that must be called before +other methods; documentation containing the phrase "you must call X first." + +**Why it bites**: nothing enforces it. The failure is a null dereference or, worse, a +silently-wrong result from a half-configured object. + +**Device**: the constructor or a static factory returns a fully ready object; or typestate, +where `connect()` returns a `Connected` type and the other methods exist only on it. +**Control.** + +### M2. Non-idempotent retryable effects + +**Signal**: a charge, email, webhook, or external mutation reachable from a retry, a queue +consumer, or a UI button, with no idempotency key, or with an optional one. + +**Why it bites**: at-least-once delivery is the norm, not the exception. Duplicate charges are +the canonical version and they are expensive and public. + +**Device**: a **required** idempotency key parameter, backed by a unique constraint on +`(entity, key)`. **Control.** An optional idempotency key is rung zero wearing a costume. + +The constraint is necessary and not sufficient. Rejecting the duplicate is not the same as +being idempotent: the key has to be *reserved in the same transaction as the effect*, bound +to the request payload so a different payload under a reused key is an error rather than a +silent no-op, and the stored result replayed to the second caller. A caller that retries and +gets a constraint violation has learned nothing about whether the first attempt worked. + +### M3. Illegal state transitions + +**Signal**: an entity with a `status` field mutated by assignment from several places; a +refund reachable before a charge; "cancelled" transitioning back to "pending". + +**Why it bites**: every site that assigns the field must know the whole state machine, and one +of them doesn't. + +**Device**: a single transition function that is the only path to a new state, rejecting +illegal transitions; or typestate so illegal transitions don't compile. **Control.** + +A row-level `CHECK` is not defence in depth here: it constrains one row's values and cannot +see the state that row is coming from, so it can forbid `status = 'refunded' AND total < 0` +but not `shipped → pending`. Policing transitions in the database needs a trigger, or a +transition table the row must join against. + +### M4. Resources that must be released + +**Signal**: `open()`/`close()`, `acquire()`/`release()`, `begin()`/`commit()` as separate +statements, especially with a `return` or `throw` reachable between them. + +**Why it bites**: the happy path is fine and the error path leaks. Leaks surface as connection +pool exhaustion under load, which is when you can least afford it. + +**Device**: scope-bound acquisition, `with`, `defer`, RAII, `using`, try-with-resources. +**Control.** + +### M5. Check-then-act races + +**Signal**: `if (!exists(x)) create(x)`, read-modify-write on a shared counter, checking a +balance and then debiting it, `if (!file.exists()) write(file)`. + +**Why it bites**: correct in every test and wrong under concurrency, intermittently, in +production only. + +**Device**: make it atomic: a unique constraint plus `INSERT ... ON CONFLICT`, a conditional +update carrying the expected version, `SELECT FOR UPDATE`, a compare-and-swap. **Control.** + +### M6. Fire-and-forget async + +**Signal**: a promise not awaited, a goroutine with no error path, `asyncio.create_task` with +no reference kept, a background write nobody joins. + +**Why it bites**: errors vanish. Worse, the process may exit before the work completes, so +writes are lost silently and non-deterministically. + +**Device**: `no-floating-promises` as a lint error, an errgroup, structured concurrency, +holding and awaiting the task. **Warning** from the linter, which is the practical answer +in TypeScript, Python and Go. Rust is the closest thing to an exception: futures are lazy and `#[must_use]`, so a dropped +future produces a compiler warning without any linter. That is **Warning**, for free; add +`#![deny(unused_must_use)]` to make the build fail and it becomes **Control**. + +### M7. Order-dependent migrations and deploys + +**Signal**: a migration that drops or renames a column in the same deploy as the code change; +a migration and code that must land in a specific order with nothing enforcing it. + +**Why it bites**: during the rollout window, old code runs against the new schema. This is an +outage, not a bug. + +**Device**: expand/contract, add, backfill, dual-write, switch, then drop in a later deploy, with a CI gate that blocks destructive DDL from co-deploying with code changes. **Control** +via the gate; the pattern itself is the design. + +--- + +## Cross-cutting, devices that were removed + +Several of these are hazards of removal, someone installed a device and someone else took +it out. Others (X2, X5) are defaults nobody chose: the language ships them switched the wrong +way and they stay that way until someone notices. +Treat them with more suspicion than a missing device, since the code around them was written +by someone who knew the failure was possible. + +### X1. Swallowed errors + +**Signal**: `catch {}`, `except: pass`, `except Exception: pass`, `_ = err`, `catch (e) { +console.log(e) }` with execution continuing, `.catch(() => null)`. + +**Why it bites**: converts a loud failure into a quiet wrong answer: the exact inversion of +mistake-proofing. The system continues on corrupted assumptions. + +**Device**: handle it, or let it propagate. Where absorbing genuinely is correct, the comment +must name which specific failure is expected and why continuing is safe; catch that specific +type, not everything. Enforce with `no-empty` / bare-except lint rules as errors. **Warning.** + +### X2. Silent coercion and fallback + +**Signal**: `value || default` where `0`/`""`/`false` are legal values; `parseInt` without a +radix or a NaN check; `int(x)` in a try/except returning a default; `.unwrap_or_default()` on +a genuine error; `?.` chains ending in `undefined` that flow into logic. + +**Why it bites**: produces a plausible value from bad input. The wrongness surfaces far away, +where the cause is invisible. + +**Device**: `??` instead of `||` where zero is legal; explicit parse with an error branch; +fail at the boundary rather than substituting. **Control** at the parse site. + +### X3. Disabled tests + +**Signal**: `it.only`, `describe.skip`, `@pytest.mark.skip`, `t.Skip()`, `#[ignore]`: especially without a reason. Lint and type-checker suppressions (`eslint-disable`, +`# type: ignore`, `@ts-ignore`, `#nosec`) are X4, and the detector splits them the same way. + +**Why it bites**: a Detection-rung device switched off, usually temporarily, permanently. The +suite stays green and stops meaning anything. + +**Device**: fail CI on focused/skipped tests; require a justification comment and an issue +link on every suppression; count suppressions and ratchet the number downward. **Warning.** + +### X4. Escape hatches in the type system + +**Signal**: `any`, `as unknown as T`, `!` non-null assertion, `interface{}` with a type +switch, `# type: ignore`, `unsafe`, `cast()`, `Object` as a parameter type. + +**Why it bites**: every one is a place where the type system's guarantee stops. Concentrated +in the boundary code that most needs the guarantee. + +**Device**: ban them by lint at error level with a narrow, justified allowlist; replace with +parsing at the boundary. **Warning**: a required CI gate is still rung 2 on the ladder: it announces the mistake +rather than removing the ability to make it. Reach **Control** only when the unchecked value cannot be constructed. + +### X5. Mutable shared defaults + +**Signal**: Python's `def f(items=[])`, a module-level dict used as a cache and mutated, a +shared config object mutated after construction, class attributes used as instance state. + +**Why it bites**: state leaks between calls, requests, or tests. The symptom is +order-dependent behavior that disappears when you try to reproduce it. + +**Device**: `None` sentinel with in-function construction, frozen/immutable value types, +per-request construction. `B006` in ruff/flake8-bugbear enforces the argument-default case +only; the module-level cache, the shared config object and the mutable class attribute have +no lint rule and need review or a type that cannot be mutated. +**Warning**, or **Control** with frozen types. diff --git a/skills/poka-yoke-guardrails/SKILL.md b/skills/poka-yoke-guardrails/SKILL.md new file mode 100644 index 000000000..61ad551a6 --- /dev/null +++ b/skills/poka-yoke-guardrails/SKILL.md @@ -0,0 +1,92 @@ +--- +name: poka-yoke-guardrails +description: 'Pre-commit hooks, CI gates, lint rules, database constraints and branch protection. Use when a rule needs enforcing rather than documenting: "set up enforcement", "unformatted or untyped code must not get merged", "gate this in CI", "we agreed to X and people still do not", "stop secrets getting committed". Covers baselining and ratcheting so existing violations do not block anyone. For constraining an AI agent use agent-guardrails.' +license: MIT +--- + +# Poka-Yoke Guardrails + +Design-time devices protect the code you are writing now. Guardrails protect the code +everyone writes later, including the version of you who is in a hurry. They are Shingo's +*successive check*: the next station refuses to accept bad work. + +The reason this mode exists as its own thing: the most common failure in software quality is +agreeing on a rule and then writing it down. A rule in a wiki has a half-life of about one +onboarding. The same rule wired into a gate applies itself and costs nothing to remember. + +## Building, not reviewing + +This mode is usually reached *while someone is building the thing*. They asked for the config, +so produce the config — working, complete, in their stack. A severity table is not useful to +someone mid-feature. + +Then add three or four closing lines: which misuses the shape you chose makes impossible and at +which rung, and what you left possible on purpose. That note is what stops the device being +undone in six months by someone who cannot see why it is there. + +When the code already exists and they are asking what is wrong with it, switch to the audit +voice. Match the mode to where they are in the work. + +## Pick the earliest gate that can hold the rule + +The same rule can live at several points in the lifecycle. Earlier is better, feedback is +faster, cheaper, and lands while the author still has the context in their head. But earlier +is also easier to bypass. The resolution is to place the device early **and** back it with a +gate that cannot be skipped. + +| Gate | Feedback speed | Bypassable? | Best for | +|---|---|---|---| +| Type system / compiler | instant | no | anything the types can express, always first choice | +| Editor + lint | seconds | yes (ignore comment) | style, banned APIs, unsafe patterns | +| Pre-commit hook | seconds | yes (`--no-verify`) | fast checks: secrets, formatting, obvious footguns | +| Pre-push hook | ~a minute | yes | medium checks you don't want to wait for on every commit | +| CI required check | minutes | **no**, with branch protection | the real enforcement, everything that must not merge | +| Database constraint | instant, at write | no | data invariants, across every service and every script | +| Runtime assertion | at execution | no | invariants no earlier gate can see | + +**Never rely on a pre-commit hook alone for anything that matters.** `--no-verify` exists, and +people under deadline use it. Use the hook for speed and the CI check for authority; run the +same script in both so they cannot drift. + +## The devices worth installing + +Ready-to-adapt templates live in `assets/devices/`. Read the relevant one, adapt it to +the repo's actual stack, and show the user the file before writing it. + +- `assets/devices/pre-commit/`, `.pre-commit-config.yaml` covering secrets, large + files, merge conflict markers, formatting, and a hook for repo-specific rules +- `assets/devices/github-actions/`: a required-check workflow, plus a migration-safety + gate +- `assets/devices/lint/`: ESLint and Ruff rule sets chosen specifically for + mistake-prevention rather than style + +The rules that pay for themselves in nearly every repo, roughly in order of value: + +1. **Secret scanning at commit time.** A leaked key is irreversible; rotation is the only + remedy. This is the highest blast-radius mistake a hook can prevent. +2. **Type checking as a required check**, `tsc --noEmit`, `mypy --strict`, `go vet`. This is + what makes every design-time device in `design` actually load-bearing. A branded + type with no type check in CI is decoration. +3. **The specific lint rules that catch silent failure**: floating promises, unhandled + rejections, unchecked errors, bare `except`, empty catch blocks, non-exhaustive switches. + Ordinary style rules are not poka-yoke; these are. +4. **Migration safety**: block destructive DDL, or require an explicit acknowledgment for it. + Dropping a column in a deploy is a classic irreversible mistake with a trivial device. +5. **Test integrity**: fail CI on `it.only`, `fdescribe`, `@pytest.mark.skip` left behind. A + skipped test is a detection device that has been switched off, usually by accident. +6. **Branch protection with required checks.** Without it, none of the above is enforcement. + +## Verify the device actually fires + +An untested guardrail is a guardrail you *believe in*, which is worse than none. It creates +confidence without protection. Before you call it done, demonstrate it: + +1. Write the mistake it is supposed to catch, deliberately. +2. Run the gate. Confirm it fails, and that the message is the one you wrote. +3. Remove the mistake. Confirm it passes. +4. Show the user both outcomes. + +Then leave a `poka-yoke:` marker comment on the rule naming the mistake it prevents, see the +recording section in `audit`. A device whose purpose nobody remembers is a device +that gets deleted during the next cleanup. + diff --git a/skills/poka-yoke-guardrails/assets/devices/github-actions/poka-yoke-gates.yml b/skills/poka-yoke-guardrails/assets/devices/github-actions/poka-yoke-gates.yml new file mode 100644 index 000000000..6a7d83cb8 --- /dev/null +++ b/skills/poka-yoke-guardrails/assets/devices/github-actions/poka-yoke-gates.yml @@ -0,0 +1,136 @@ +# Poka-yoke CI gates. +# +# poka-yoke: refuses to merge a change that fails a gate, once the jobs are marked required [control] +# +# This is where enforcement actually lives. Pre-commit hooks are bypassable; a required +# check with branch protection is not. Every job here should also be marked "Required" in +# branch protection settings, without that, this workflow is advisory and the whole device +# is inert. +# +# Design note: jobs enforce on CHANGED FILES where possible. Turning a strict rule on across +# a large existing codebase produces hundreds of failures and gets reverted by Friday; +# ratcheting on new code only means the violation count can only go down. + +name: poka-yoke + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +jobs: + hazards: + name: hazard scan + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 # needed to diff against the base branch + - uses: actions/setup-python@v6 + with: + python-version: "3.12" + - name: Scan changed lines for high-severity hazards + # Context values go through env, never inline into run:. Interpolating + # ${{ }} directly into a shell command is how workflow injection happens. + env: + BASE_REF: ${{ github.base_ref || 'main' }} + run: | + python3 scripts/detect_hazards.py \ + --since "origin/${BASE_REF}" \ + --severity high + + types: + # The gate that makes every design-time device load-bearing. A branded type in a repo + # that doesn't typecheck in CI is a comment. + name: type check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + # --- pick the one that matches this repo and delete the rest --- + # - run: npx tsc --noEmit + # - run: uv run mypy --strict src/ + # - run: go vet ./... && go build ./... + # - run: cargo clippy --all-targets -- -D warnings + - run: echo "Replace with this repo's type checker, then delete this line." + + secrets: + name: secret scan + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: gitleaks/gitleaks-action@v2 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + test-integrity: + # A skipped or focused test is a detection device switched off. Usually temporarily. + # Permanently. + name: test integrity + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + - name: No focused tests + run: | + # The skill promises this rejects skipped tests as well as focused ones, and in + # every language it names. It used to grep TS/JS for `.only` alone, so a + # `@pytest.mark.skip` left behind passed a gate that claimed to catch it. + # + # The x-prefixed Jasmine forms are written as an alternation rather than spelled + # out: codespell reads the longer one as a misspelling of `describe`. + if grep -rEn '(\.only\(|fdescribe\(|\bfit\(|\.skip\(|\bx(describe|it)\(|@pytest\.mark\.(skip|xfail)|\bt\.Skip\(|#\[ignore\])' \ + --include='*.ts' --include='*.tsx' --include='*.js' --include='*.jsx' \ + --include='*.py' --include='*.go' --include='*.rs' \ + --exclude-dir=node_modules --exclude-dir=.git . ; then + echo "::error::A focused or skipped test disables coverage. Both are detection" + echo "devices switched off; remove them, or mark the skip with an issue link and" + echo "add that path to this step's excludes so the exemption is visible." + exit 1 + fi + + migration-safety: + # Destructive DDL co-deployed with application code is an outage during the rollout + # window, because old code necessarily runs against the new schema. Use expand/contract: + # add, backfill, dual-write, switch reads, and drop in a LATER deploy. + # + # Escape hatch: label the PR `destructive-migration-approved` when the drop is genuinely + # intended and nothing references the column. The label is the explicit acknowledgment + # that turns an accident into a decision. + name: migration safety + runs-on: ubuntu-latest + if: "!contains(github.event.pull_request.labels.*.name, 'destructive-migration-approved')" + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - name: Block destructive DDL + env: + BASE_REF: ${{ github.base_ref || 'main' }} + run: | + BASE="origin/${BASE_REF}" + # Read into an array rather than splitting a string: an unquoted $CHANGED splits + # on spaces as well as newlines, so a migration whose filename contains a space + # becomes two paths that do not exist, and `git diff` then reports no changes, + # which this gate would read as "nothing destructive here". + # No `|| true`. A missing base ref or any other git error used to become an + # empty CHANGED array, and the next line read that as "No migrations changed" + # and exited 0 -- the gate passing precisely because it could not run. mapfile + # masks the exit status of a process substitution, so the status is checked + # explicitly rather than relied upon. + if ! DIFF=$(git diff --name-only "$BASE"...HEAD \ + -- 'migrations/**' 'db/migrate/**'); then + echo "::error::Could not diff against $BASE. This is NOT an all-clear." + exit 1 + fi + mapfile -t CHANGED < <(printf '%s' "$DIFF") + [ ${#CHANGED[@]} -eq 0 ] && { echo "No migrations changed."; exit 0; } + if git diff "$BASE"...HEAD -- "${CHANGED[@]}" | grep -iE '^\+.*(DROP (TABLE|COLUMN)|TRUNCATE|ALTER .* DROP)'; then + echo "::error::Destructive DDL detected. Use expand/contract and drop in a later deploy." + echo "If this drop is intentional and nothing references the column, add the" + echo "'destructive-migration-approved' label to this PR." + exit 1 + fi diff --git a/skills/poka-yoke-guardrails/assets/devices/lint/README.md b/skills/poka-yoke-guardrails/assets/devices/lint/README.md new file mode 100644 index 000000000..fd0c2c75a --- /dev/null +++ b/skills/poka-yoke-guardrails/assets/devices/lint/README.md @@ -0,0 +1,130 @@ +# Lint rules as poka-yoke devices + +Most lint rules are style. These are not: each one below prevents a specific mistake that +produces a specific wrong behavior. Set them to `error`, not `warn`: a warning in a list of +four hundred warnings is rung zero. + +Install strategy: enforce on changed files, or generate a baseline of existing violations and +fail only on new ones. Turning these on repo-wide in a large codebase produces a wall of +failures and the rule gets reverted. The violation count only needs to go down. + +## TypeScript, `eslint.config.js` + +Requires `@typescript-eslint` with type-aware linting (`projectService: true`), because the +highest-value rules here need type information. + +```js +// eslint.config.js +import tseslint from "typescript-eslint"; + +export default tseslint.config( + ...tseslint.configs.recommendedTypeChecked, + { + languageOptions: { parserOptions: { projectService: true } }, + rules: { + // --- silent failure: the highest-value rules in this file --- + "@typescript-eslint/no-floating-promises": "error", // an unawaited write, silently lost + "@typescript-eslint/no-misused-promises": "error", // async fn passed where sync expected + "no-empty": ["error", { allowEmptyCatch: false }], // swallowed errors + "require-atomic-updates": "error", // read-modify-write race across await + + // --- completeness --- + "@typescript-eslint/switch-exhaustiveness-check": "error", // new variant, silently unhandled + "@typescript-eslint/no-unnecessary-condition": "error", // always-true check = usually a bug + + // --- type guarantees --- + "@typescript-eslint/no-explicit-any": "error", + "@typescript-eslint/no-unsafe-assignment": "error", + "@typescript-eslint/no-unsafe-argument": "error", + "@typescript-eslint/no-unsafe-return": "error", + "@typescript-eslint/no-non-null-assertion": "error", // `!` is an unchecked claim + + // --- coercion surprises --- + eqeqeq: ["error", "always"], + + // --- detection devices switched off --- + "no-restricted-syntax": ["error", + { selector: "MemberExpression[property.name='only']", + message: "Focused tests disable the rest of the suite. Remove before merging." }], + }, + }, +); +``` + +`tsconfig.json` matters as much as the lint config, none of the above is load-bearing without: + +```jsonc +{ + "compilerOptions": { + "strict": true, + "noUncheckedIndexedAccess": true, // array access is T | undefined, which is the truth + "exactOptionalPropertyTypes": true + } +} +``` + +## Python, `pyproject.toml` + +```toml +[tool.ruff.lint] +select = [ + "E", "F", # pyflakes: undefined names, unused imports: real bugs + "B", # bugbear: mutable defaults (B006), loop variable capture, assert on tuple + "S", # bandit: hardcoded secrets, unsafe subprocess, weak crypto + "DTZ", # naive datetimes + "ASYNC", # blocking calls inside async functions + "RUF006", # dangling asyncio task, can be GC'd mid-flight, work silently not done + "PLE", # pylint errors only, not conventions + "T20", # stray print/pprint +] +ignore = ["E501"] # line length is style, not mistake-proofing + +[tool.ruff.lint.per-file-ignores] +"tests/**" = ["S101"] # assert is fine in tests; it is not fine as production validation + +[tool.mypy] +strict = true +disallow_any_unimported = true # an untyped dependency reintroduces Any silently +warn_return_any = true +``` + +## Go, `.golangci.yml` + +```yaml +# v2 is not backward compatible: it rejects a v1 file rather than migrating it, so the +# version key is what stops this template failing on a current golangci-lint. +version: "2" + +linters: + enable: + - errcheck # unchecked errors: Go's error convention is opt-in without this + - exhaustive # non-exhaustive switch over typed constants + - bodyclose # unclosed HTTP response bodies + - rowserrcheck + - sqlclosecheck + - contextcheck # context not propagated: cancellation and timeouts silently disabled + - nilerr # returning nil after a non-nil error + - noctx # HTTP requests without a context + - gosec + + settings: + exhaustive: + default-signifies-exhaustive: false +``` + +`errcheck` is the non-negotiable one. `_ = doSomething()` is how data loss enters a Go +codebase. + +## Rust, `Cargo.toml` + +```toml +[workspace.lints.clippy] +unwrap_used = "deny" # highest value: turns "this can't fail" into an explicit decision +expect_used = "warn" # acceptable at startup and in tests, with a reason +panic = "deny" +indexing_slicing = "deny" # forces .get() and a real branch +float_cmp = "deny" +arithmetic_side_effects = "warn" # forces checked_/saturating_ where overflow matters +todo = "deny" +dbg_macro = "deny" +``` diff --git a/skills/poka-yoke-guardrails/assets/devices/pre-commit/.pre-commit-config.yaml b/skills/poka-yoke-guardrails/assets/devices/pre-commit/.pre-commit-config.yaml new file mode 100644 index 000000000..0617e033f --- /dev/null +++ b/skills/poka-yoke-guardrails/assets/devices/pre-commit/.pre-commit-config.yaml @@ -0,0 +1,65 @@ +# Poka-yoke pre-commit devices. +# +# poka-yoke: stops a mistake reaching a commit, at the cost of being bypassable [warning] +# +# Adapt to the repo's actual stack before installing: an unused hook is friction with no +# protection. Keep the whole run under ~5 seconds; slower than that and people use +# --no-verify, which turns every hook here into decoration. +# +# IMPORTANT: pre-commit is bypassable by design (`git commit --no-verify`). Never rely on it +# alone for anything that matters. Run the same checks as a required CI check so the hook +# provides speed and CI provides authority. + +repos: + # ---- Irreversible mistakes first. A leaked key can only be rotated, never unleaked. ---- + - repo: https://github.com/gitleaks/gitleaks + rev: v8.28.0 + hooks: + - id: gitleaks + + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v6.0.0 + hooks: + - id: check-added-large-files # a committed binary is painful to remove later + args: [--maxkb=1000] + - id: check-merge-conflict # conflict markers shipped to main + - id: check-case-conflict # breaks on case-insensitive filesystems only + - id: detect-private-key + - id: end-of-file-fixer + - id: trailing-whitespace + - id: check-json + - id: check-yaml + - id: check-toml + + # ---- Python: the rules that catch silent failure, not the ones that argue about style ---- + - repo: https://github.com/astral-sh/ruff-pre-commit + rev: v0.14.5 + hooks: + - id: ruff-check + args: [--fix, --exit-non-zero-on-fix] + - id: ruff-format + + # ---- Repo-specific hazards. Fast, changed-files-only. ---- + - repo: local + hooks: + - id: poka-yoke-hazards + name: poka-yoke hazard scan (high severity only) + # Point this at wherever you vendored the detector. It is not on your PATH and this + # file cannot know where you put it: an entry that silently resolves to nothing + # would make the hook pass on every commit, which is worse than not installing it. + # git clone https://github.com/rainmanjam/poka-yoke /tmp/pk + # cp -r /tmp/pk/plugins/poka-yoke/scripts tools/poka-yoke + entry: python3 tools/poka-yoke/detect_hazards.py --staged --severity high + language: system + pass_filenames: false + # Fails the commit on high-severity hazards in staged changes only, so pre-existing + # code doesn't block anyone. New violations can't be added; the count only goes down. + + - id: no-focused-tests + name: no focused or skipped tests + entry: '(\.only\(|fdescribe\(|fit\(|@pytest\.mark\.skip|t\.Skip\()' + language: pygrep + types_or: [python, javascript, ts, tsx, go] + exclude: '^(tests/fixtures/|.*\.md$)' + # A focused test silently disables the rest of the suite: a detection device + # switched off by accident. diff --git a/skills/poka-yoke-retro/SKILL.md b/skills/poka-yoke-retro/SKILL.md new file mode 100644 index 000000000..77cea5687 --- /dev/null +++ b/skills/poka-yoke-retro/SKILL.md @@ -0,0 +1,104 @@ +--- +name: poka-yoke-retro +description: 'Turn a bug, outage or repeated mistake into a device that makes the whole class impossible. Use when something already broke: "make sure this never happens again", "this is the third time", "postmortem", "how did this get through". Root-causes to the missing constraint, then sweeps every other site where the mistake is still available. For a pipeline use data, a deploy use ops, cross-tenant use authz, an AI feature use llm.' +license: MIT +--- + +# Poka-Yoke Retro + +A defect got out. The fix for the defect is the easy part and is usually already done or +obvious. This mode is about the harder and more valuable question: **what made the mistake +available, and what device removes it for good?** + +Shingo's framing is the whole method here. Do not ask why the person erred, people err, that +is a constant. Ask why the *process permitted* the error to become a defect, and what would +have physically stopped it. + +## 1. Separate the three things + +Conflating these is why incidents repeat. + +- **The defect**: what was experienced. "Customers were charged twice." +- **The mistake**: the action that produced it. "The retry path called `charge()` again without + an idempotency key." +- **The hazard**: the property that made that mistake possible and silent. "`charge()` accepts + an optional idempotency key and succeeds without one." + +Fixing the defect ships today. Fixing the mistake helps one code path. **Only fixing the hazard +prevents recurrence** — and the hazard is almost always a missing constraint, not a missing +piece of knowledge. Write all three out before proposing anything. If you cannot state the +hazard as a property of the system, you have not found it yet. + +## 2. Ask why until you reach a constraint + +One discipline: **an acceptable terminal answer is a missing constraint, never a missing human +quality.** If a chain ends in "they forgot", "they didn't know", or "it wasn't documented", you +stopped one step early — ask why forgetting was possible. + +> Double charge → the retry called `charge()` twice → the retry path passed no idempotency key +> → **the key is an optional parameter** → it was added later and made optional to avoid +> breaking callers → **nothing requires a charge to be idempotent.** + +That last line is the hazard, and it is fixable: make the parameter required, or add a unique +constraint on `(account_id, idempotency_key)`. "The engineer should have passed the key" is +fixable only by hiring different humans. + +Ask the escape question separately: **what should have caught this and didn't?** Usually a +device existed and was absent, disabled, or too weak. That gap is a second finding. + +## 3. Sweep for the class — the step that gets skipped + +A device that fixes one call site is not a device. Before proposing anything, find **every +other place the same mistake is still available.** Search by the shape of the hazard, not the +text of the bug: every other caller of the function; every other signature with the same shape +(optional-when-it-should-be-required, same-type adjacent arguments, unguarded bulk operations); +the same pattern in sibling services, scripts, jobs and infrastructure code. Then +`python3 scripts/detect_hazards.py --paths --id `, using the ID printed with +each finding, to catch instances you would not have thought to grep for. + +Report the count plainly: *"the same hazard exists at 6 other call sites"* changes the +conversation about what the fix is worth. + +## 4. Choose the device by rung + +For an incident that already cost something, push hard for **Control**: you have the strongest +evidence you will ever have that this mistake happens. + +| Rung | For this incident, that would mean | +|---|---| +| **Control** | Required parameter · unique constraint · a type that cannot hold the bad state · unmergeable CI check | +| **Warning** | Lint rule · runtime assertion · alert at the moment of the action | +| **Detection** | Regression test · monitor · reconciliation job | +| **None** | "Added a note to the runbook" · "reminded the team" · a new checklist item | + +Write the regression test — it proves the fix. But be honest that it is rung 3: it catches the +mistake after someone makes it, and only on the path you thought of. If the retro produces +*only* a test, say what a Control-rung device would have required. + +Beware rung zero in a costume: more documentation, a checklist item, a training session, an +extra required reviewer. If that is genuinely all that is possible, name it as an accepted risk +rather than a resolution. + +## 5. Write it up + +```markdown +# Retro · · + +**Defect**: +**Mistake**: +**Hazard**: + +## The write-up, five headings + +**Why it was possible** — the property of the system that made the mistake available, not the +person who made it. **Why nothing caught it** — which rung was missing. **Class sweep** — every +other place the same shape exists, listed. **Devices** — one per hazard, with its rung. +**Accepted risk** — what you are choosing to leave open, and why. + +## 6. Verify the device before you close it + +Prove the fix. Reproduce the original mistake against the new device and show it being +refused, then show the correct path still working. A device that was never observed to fire is +a belief, not a control, and after an incident, a false sense of protection is the most +expensive thing you can ship. + diff --git a/skills/poka-yoke-retro/scripts/detect_hazards.py b/skills/poka-yoke-retro/scripts/detect_hazards.py new file mode 100755 index 000000000..40c940593 --- /dev/null +++ b/skills/poka-yoke-retro/scripts/detect_hazards.py @@ -0,0 +1,632 @@ +#!/usr/bin/env python3 +"""Heuristic detector for poka-yoke hazards, shapes in code that make mistakes easy. + +This is a fast first pass, not an oracle. It finds textually-detectable hazards so a +reviewer can spend their attention on the interface-level questions a regex cannot ask. +Expect real false positives; every hit is a question, not a verdict. + +Hazard IDs match the hazard catalogue that ships with the poka-yoke skill. +Standard library only. + +Examples: + detect_hazards.py --diff # uncommitted changes, changed lines only + detect_hazards.py --staged # staged changes + detect_hazards.py --since HEAD~10 # last 10 commits + detect_hazards.py --paths src/ lib/ # explicit paths + detect_hazards.py --diff --severity high # only the ones that bite hardest + detect_hazards.py --paths . --json # machine-readable +""" + +from __future__ import annotations + +import argparse +import ast +import json +import os +import re +import subprocess +import sys +from dataclasses import dataclass, field +from pathlib import Path + +# -------------------------------------------------------------------------------------- +# Rule definitions +# -------------------------------------------------------------------------------------- + +PY = {".py", ".pyi"} +TS = {".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs"} +GO = {".go"} +RS = {".rs"} +SQL = {".sql"} +ALL_EXTS = PY | TS | GO | RS | SQL + +LENS = {"C": "contact", "F": "fixed-value", "M": "motion-step", "X": "removed-device"} + + +@dataclass(frozen=True) +class Rule: + id: str + name: str + severity: str # high | medium | low + exts: frozenset + pattern: re.Pattern + device: str + negate: re.Pattern | None = None # if this also matches the line, skip + + +def R(id, name, severity, exts, pattern, device, negate=None, flags=0): + return Rule( + id=id, + name=name, + severity=severity, + exts=frozenset(exts), + pattern=re.compile(pattern, flags), + device=device, + negate=re.compile(negate, flags) if negate else None, + ) + + +RULES: list[Rule] = [ + # ---- X: devices that were removed ------------------------------------------------- + R("X1", "Swallowed error", "high", TS, + r"catch\s*(\([^)]*\))?\s*\{\s*\}", + "Handle it or let it propagate; catching to do nothing turns a loud failure quiet."), + R("X1", "Swallowed error", "high", TS, + r"\.catch\s*\(\s*\(\s*\)\s*=>\s*(\{\s*\}|null|undefined)\s*\)", + "Handle the rejection or let it propagate."), + R("X1", "Bare except", "high", PY, + r"^\s*except\s*:", + "Catch the specific exception; bare except also swallows KeyboardInterrupt/SystemExit."), + R("X1", "Discarded error return", "high", GO, + r",\s*_\s*:?=\s*\w|^\s*_\s*=\s*\w[\w.]*\(", + "Check the error. Enable errcheck in golangci-lint to make this a build failure."), + R("X2", "Unwrap / expect on a fallible value", "medium", RS, + r"\.(unwrap|expect)\s*\(", + "Propagate with ? or handle the error; deny clippy::unwrap_used."), + R("X2", "Silent default on error", "medium", RS, + r"\.unwrap_or_default\s*\(\s*\)", + "A default on an error path hides the failure; branch on the error explicitly."), + R("X2", "parseInt without radix", "medium", TS, + r"parseInt\s*\(\s*[^,)]+\)", + "Pass the radix and check for NaN, or use a schema parse at the boundary."), + R("X3", "Focused test disables the suite", "high", TS, + r"\b(it|test|describe|context)\.only\s*\(|\bfdescribe\s*\(|\bfit\s*\(", + "Remove before merge; fail CI on focused tests."), + R("X3", "Skipped test", "medium", PY, + r"@pytest\.mark\.skip|@unittest\.skip", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X3", "Skipped test", "medium", TS, + r"\b(it|test|describe)\.skip\s*\(|\bxit\s*\(|\bxdescribe\s*\(", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X3", "Skipped test", "medium", GO, r"\bt\.Skip\s*\(", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X3", "Skipped test", "medium", RS, r"^\s*#\[ignore\]", + "A skipped test is a detection device switched off. Fix or delete it."), + R("X4", "Type-checker suppression", "medium", TS, + r"@ts-ignore|@ts-nocheck|\bas\s+unknown\s+as\b|eslint-disable(?!-next-line\s+\S+\s+--)", + "Each suppression is a hole in the guarantee. Require a reason and an issue link."), + R("X4", "Explicit any", "medium", TS, + r":\s*any\b||Array|as\s+any\b", + "any disables the type system exactly where guarantees matter. Parse at the boundary."), + R("X4", "Type-checker suppression", "medium", PY, + r"#\s*type:\s*ignore(?!\[)", + "Narrow it to a specific error code and add a reason."), + R("X4", "Untyped container", "low", GO, + r"\binterface\{\}|\bany\b\s*[,)\]]", + "Prefer a concrete type or a constrained generic."), + R("X4", "unsafe block", "medium", RS, r"\bunsafe\s*\{", + "Require a // SAFETY: comment stating the invariant being upheld."), + R("X5", "Mutable default argument", "high", PY, + r"def\s+\w+\s*\([^)]*=\s*(\[\s*\]|\{\s*\}|set\s*\(\s*\))", + "Use None and construct inside the function; the default is shared across all calls."), + + # ---- F: fixed-value --------------------------------------------------------------- + R("F2", "Unbounded DELETE", "high", SQL | PY | TS | GO | RS, + r"\bDELETE\s+FROM\b(?!.*\bWHERE\b)", + "Require a WHERE clause; refuse an empty predicate.", flags=re.I), + R("F2", "Unbounded UPDATE", "high", SQL | PY | TS | GO | RS, + r"\bUPDATE\s+[\w.\"`\[\]]+\s+SET\b(?!.*\bWHERE\b)", + "Require a WHERE clause; refuse an empty predicate.", flags=re.I), + R("F2", "Destructive DDL", "high", SQL | PY | TS | GO | RS, + r"\b(DROP\s+(TABLE|COLUMN|DATABASE|SCHEMA)|TRUNCATE\s+TABLE)\b", + "Use expand/contract; gate destructive DDL behind an explicit CI acknowledgment.", + flags=re.I), + R("F2", "Bulk delete", "high", TS | PY, + r"\.(deleteMany|delete_many|destroy_all|delete_all|drop_all|removeMany)\s*\(\s*\)", + "Refuse an empty filter; cap the affected count and require confirmation above it."), + R("F2", "Recursive force remove", "high", ALL_EXTS, + r"rm\s+-[a-zA-Z]*[rR][a-zA-Z]*f|rm\s+-[a-zA-Z]*f[a-zA-Z]*[rR]", + "Validate the path is non-empty and inside the expected root before deleting."), + R("F4", "Config read away from startup", "medium", PY, + r"os\.(getenv|environ)", + "Parse the whole config into a typed object at startup so a missing key fails the deploy.", + negate=r"(settings|config|conf|env)\.py"), + R("F4", "Config read away from startup", "medium", TS, + r"process\.env\.\w+", + "Parse the whole config into a typed object at startup so a missing key fails the deploy.", + negate=r"(config|env|settings)\.(ts|js)"), + R("F7", "Unbounded read", "low", PY | TS, + r"\.read\s*\(\s*\)|\.readAll\s*\(|ioutil\.ReadAll", + "Cap the size at the boundary; an unbounded read is a resource-exhaustion incident."), + + # ---- C: contact ------------------------------------------------------------------- + R("C2", "Boolean flag parameter", "medium", TS, + r"\b\w+\s*:\s*boolean\s*[,)]", + "Use an enum, a named options object, or two functions; booleans are unreadable at the call site."), + R("C2", "Boolean flag parameter", "medium", GO, + r"func\s+\w+\s*\([^)]*\bbool\b[^)]*\)", + "Use a named option type; a bare bool is unreadable at the call site."), + R("C2", "Boolean default parameter", "medium", PY, + r"def\s+\w+\s*\([^)]*\b\w+\s*(:\s*bool\s*)?=\s*(True|False)", + "Use an enum, or at minimum make it keyword-only so the name appears at the call site."), + R("C5", "Duration without a unit", "medium", TS | GO | PY, + r"\b(timeout|delay|interval|ttl|expiry|duration|retryAfter|retry_after)\s*:?\s*(number|int|float|=\s*\d+)", + "Encode the unit in the type (Duration) or in the name (timeoutMs). Unit mismatches are silent."), + R("C6", "Money as a float", "high", PY | TS | GO | RS, + r"\b(price|amount|total|balance|cost|fee|subtotal|revenue)\w*\s*:\s*(float|number|f32|f64)\b" + r"|\bfloat\s*\(\s*\w*(price|amount|total|balance)", + "Use integer minor units in a Money type carrying its currency, or a decimal type."), + R("C7", "Unvalidated parse", "high", TS, + r"JSON\.parse\s*\(", + "Parse into a schema (zod/valibot) at the boundary; JSON.parse returns any."), + R("C7", "Unvalidated request body", "high", PY, + r"(request|req)\.(json|get_json)\s*\(\s*\)(?!\s*\))", + "Parse into a Pydantic model with extra='forbid' so unknown or missing fields fail loudly."), + R("C9", "Naive datetime", "medium", PY, + r"datetime\.utcnow\s*\(\s*\)|datetime\.now\s*\(\s*\)", + "Use datetime.now(timezone.utc), and inject a clock so time is testable."), + + # ---- M: motion-step --------------------------------------------------------------- + R("M4", "Unmanaged resource", "medium", PY, + r"^\s*(\w+\s*=\s*)?open\s*\(", + "Use a context manager; the error path will leak otherwise.", + negate=r"\bwith\b"), + R("M6", "Dangling async task", "high", PY, + r"^\s*(await\s+)?asyncio\.create_task\s*\(", + "Keep a reference; an unreferenced task can be garbage collected mid-flight (ruff RUF006).", + negate=r"=\s*(await\s+)?asyncio\.create_task"), + R("M6", "Unawaited promise-returning call", "low", TS, + r"^\s*\w+\.(save|update|create|delete|insert|write|send|publish|commit)\s*\(", + "If this returns a promise, await it: a floating write is silently lost. " + "Enable @typescript-eslint/no-floating-promises.", + negate=r"\b(await|return|yield|void)\b|\.then\(|=\s"), + R("M2", "Retryable effect without an idempotency key", "high", ALL_EXTS, + r"\b(def|func|function|fn|async\s+function)\s+\w*(charge|refund|capture|payout|transfer|" + r"sendEmail|send_email|publish|notify)\w*\s*[(<]", + "Require an idempotency key parameter, backed by a unique constraint on (entity, key).", + negate=r"idempot", flags=re.I), + R("M1", "Two-phase construction", "medium", ALL_EXTS, + r"\b(def|func|function|fn)\s+(init|initialize|connect|setup|configure|start)\s*[(<]", + "Have the constructor or a factory return a ready object, or use typestate; " + "'call this first' is not enforceable.", + negate=r"__init__|func\s+init\s*\(\s*\)\s*\{"), + + # ---- F1: exhaustiveness ----------------------------------------------------------- + R("F1", "Wildcard match arm", "medium", RS, + r"^\s*_\s*=>", + "In domain logic a wildcard turns a future compile error into a silent fallthrough."), + R("F1", "Switch without exhaustiveness check", "low", TS, + r"^\s*switch\s*\(", + "Add a default arm calling assertNever(x: never) so a new variant breaks the build."), + R("F1", "Switch without a default", "low", GO, + r"^\s*switch\s+\w+\s*\{", + "Enable the 'exhaustive' linter with default-signifies-exhaustive: false."), +] + +# Rules a real linter already does better. They stay available behind --all for repos that +# do not run those linters, but they are off by default: a tool that does eight things +# nothing else does is more useful than one doing forty things worse. The value here is the +# pointer, knowing which linter to enable beats a second-rate reimplementation of it. +COVERED_BY: dict[tuple[str, str], str] = { + ("X1", "Swallowed error"): "eslint no-empty", + ("X1", "Bare except"): "ruff E722", + ("X1", "Discarded error return"): "golangci-lint errcheck", + ("X2", "Unwrap / expect on a fallible value"): "clippy::unwrap_used", + ("X2", "Silent default on error"): "clippy", + ("X2", "parseInt without radix"): "eslint radix", + ("X3", "Focused test disables the suite"): "eslint jest/no-focused-tests", + ("X3", "Skipped test"): "eslint jest/no-disabled-tests", + ("X4", "Type-checker suppression"): "@typescript-eslint/ban-ts-comment, mypy --strict", + ("X4", "Explicit any"): "@typescript-eslint/no-explicit-any", + ("X4", "Untyped container"): "golangci-lint", + ("X4", "unsafe block"): "clippy", + ("X5", "Mutable default argument"): "ruff B006", + ("C9", "Naive datetime"): "ruff DTZ", + ("M4", "Unmanaged resource"): "ruff SIM115", + ("M6", "Dangling async task"): "ruff RUF006", + ("F1", "Wildcard match arm"): "clippy::wildcard_enum_match_arm", + ("F7", "Unbounded read"): "", + ("F3", "assert used for validation"): "ruff S101", + ("C6", "Equality comparison on a float"): "ruff PLR0133", +} + + +# poka-yoke: keyword-only, so the id and the name cannot be passed transposed [control] +def covered(*, rule_id: str, name: str) -> str: + return COVERED_BY.get((rule_id, name), "") + + +# -------------------------------------------------------------------------------------- +# AST pass (Python only), catches what regexes can't see +# -------------------------------------------------------------------------------------- + + +def python_ast_findings(path: Path, source: str) -> list[dict]: + """Structural checks that need real parsing: adjacent same-type params, assert-as- + validation, and equality comparison on floats.""" + out = [] + try: + tree = ast.parse(source, filename=str(path)) + except SyntaxError: + return out + + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + args = node.args.posonlyargs + node.args.args + # skip self/cls + if args and args[0].arg in ("self", "cls"): + args = args[1:] + annotated = [(a.arg, ast.unparse(a.annotation)) for a in args if a.annotation] + for i in range(len(annotated) - 1): + (n1, t1), (n2, t2) = annotated[i], annotated[i + 1] + if t1 == t2 and t1 in ("str", "int", "float", "bytes", "bool", "UUID"): + out.append({ + "id": "C1", + "name": "Adjacent same-type parameters", + "severity": "high", + "line": node.lineno, + "snippet": f"def {node.name}(..., {n1}: {t1}, {n2}: {t2}, ...)", + "device": f"'{n1}' and '{n2}' are both {t1} and can be swapped silently. " + "Use NewType per concept, or make them keyword-only.", + }) + # positional args on a wide signature + if len(args) >= 4 and not node.args.kwonlyargs: + out.append({ + "id": "C1", + "name": "Wide positional signature", + "severity": "low", + "line": node.lineno, + "snippet": f"def {node.name}({len(args)} positional params)", + "device": "Make parameters keyword-only with '*' so names appear at the call site.", + }) + + elif isinstance(node, ast.Assert): + out.append({ + "id": "F3", + "name": "assert used for validation", + "severity": "medium", + "line": node.lineno, + "snippet": ast.unparse(node)[:100], + "device": "assert is stripped under python -O. Raise an explicit exception instead.", + }) + + elif isinstance(node, ast.Compare): + for op in node.ops: + if isinstance(op, (ast.Eq, ast.NotEq)): + src = ast.unparse(node) + if re.search(r"\d+\.\d+", src): + out.append({ + "id": "C6", + "name": "Equality comparison on a float", + "severity": "medium", + "line": node.lineno, + "snippet": src[:100], + "device": "Use math.isclose, or a Decimal/integer-minor-unit type.", + }) + return out + + +# -------------------------------------------------------------------------------------- +# File and diff collection +# -------------------------------------------------------------------------------------- + +SKIP_DIRS = { + ".git", "node_modules", "vendor", "dist", "build", "target", "__pycache__", + ".venv", "venv", ".tox", ".mypy_cache", ".pytest_cache", ".next", "coverage", + ".terraform", "site-packages", +} + + +class GitUnavailable(RuntimeError): + """git could not answer the question asked of it. + + Previously any git failure became an empty string, which the caller could not tell from + "the tree is clean". A detector that reports a clean bill of health because git is broken + is the exact failure this file's own rules exist to catch. + """ + + +def git(*args: str, cwd: Path) -> str: + try: + r = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True, timeout=30) + except (subprocess.SubprocessError, FileNotFoundError) as exc: + raise GitUnavailable(f"could not run git {' '.join(args)}: {exc}") from exc + if r.returncode != 0: + detail = (r.stderr or r.stdout or "").strip().splitlines() + raise GitUnavailable(f"git {' '.join(args)} exited {r.returncode}" + + (f": {detail[0]}" if detail else "")) + return r.stdout + + +def changed_files_and_lines(cwd: Path, mode: str, since: str | None): + """Return {path: set(changed_line_numbers)}. Empty set means 'whole file'.""" + if mode == "staged": + diff_args = ["diff", "--cached", "-U0"] + elif mode == "since": + diff_args = ["diff", f"{since}..HEAD", "-U0"] + else: + diff_args = ["diff", "HEAD", "-U0"] + + raw = git(*diff_args, cwd=cwd) + if not raw.strip() and mode == "diff": + # Clean tree, fall back to recent commits, which is what the user usually means. + raw = git("diff", "HEAD~5..HEAD", "-U0", cwd=cwd) + + result: dict[str, set[int]] = {} + current = None + for line in raw.splitlines(): + if line.startswith("+++ b/"): + current = line[6:] + result.setdefault(current, set()) + elif line.startswith("@@") and current: + m = re.search(r"\+(\d+)(?:,(\d+))?", line) + if m: + start = int(m.group(1)) + count = int(m.group(2) or 1) + result[current].update(range(start, start + count)) + return {k: v for k, v in result.items() if v} + + +def collect_paths(roots: list[str]) -> list[Path]: + out = [] + for root in roots: + p = Path(root) + if p.is_file(): + if p.suffix in ALL_EXTS: + out.append(p) + elif p.is_dir(): + for dirpath, dirnames, filenames in os.walk(p): + dirnames[:] = [d for d in dirnames if d not in SKIP_DIRS and not d.startswith(".")] + for fn in filenames: + fp = Path(dirpath) / fn + if fp.suffix in ALL_EXTS: + out.append(fp) + return out + + +# -------------------------------------------------------------------------------------- +# Scanning +# -------------------------------------------------------------------------------------- + +COMMENT_ONLY = re.compile(r"^\s*(//|#|/\*|\*|--)") + + +def scan_file(path: Path, only_lines: set[int] | None) -> list[dict]: + try: + source = path.read_text(encoding="utf-8", errors="replace") + except (OSError, UnicodeDecodeError): + return [] + if len(source) > 2_000_000: + return [] + + findings = [] + ext = path.suffix + lines = source.splitlines() + + for lineno, line in enumerate(lines, 1): + if only_lines and lineno not in only_lines: + continue + if COMMENT_ONLY.match(line) or len(line) > 500: + continue + for rule in RULES: + if ext not in rule.exts: + continue + if not INCLUDE_COVERED and covered(rule_id=rule.id, name=rule.name): + continue + if rule.negate and (rule.negate.search(line) or rule.negate.search(str(path))): + continue + if rule.pattern.search(line): + findings.append({ + "id": rule.id, + "name": rule.name, + "severity": rule.severity, + "line": lineno, + "snippet": line.strip()[:120], + "device": rule.device, + }) + + if ext in PY: + for f in python_ast_findings(path, source): + if not INCLUDE_COVERED and covered(rule_id=f["id"], name=f["name"]): + continue + if not only_lines or f["line"] in only_lines: + findings.append(f) + + for f in findings: + f["file"] = str(path) + f["lens"] = LENS.get(f["id"][0], "unknown") + return findings + + +# -------------------------------------------------------------------------------------- +# Output +# -------------------------------------------------------------------------------------- + +INCLUDE_COVERED = False + +SEV_ORDER = {"high": 0, "medium": 1, "low": 2} +COLOR = {"high": "\033[31m", "medium": "\033[33m", "low": "\033[90m"} +RESET = "\033[0m" + + +def render(findings: list[dict], scope: str, use_color: bool) -> str: + if not findings: + return f"No hazards detected in {scope}.\n\nThe lenses still apply, run them by hand:\n" \ + " contact: can the wrong thing fit?\n" \ + " fixed-value: can an incomplete or wrong-sized set pass?\n" \ + " motion-step: can the steps happen in the wrong order?" + + findings.sort(key=lambda f: (SEV_ORDER[f["severity"]], f["file"], f["line"])) + counts = {"high": 0, "medium": 0, "low": 0} + for f in findings: + counts[f["severity"]] += 1 + + out = [ + f"Poka-yoke hazard scan, {scope}", + f"{counts['high']} high · {counts['medium']} medium · {counts['low']} low", + "", + "Heuristics with real false positives. Read the surrounding code before acting.", + "", + ] + + grouped: dict[str, list[dict]] = {} + for f in findings: + grouped.setdefault(f"{f['id']} {f['name']}", []).append(f) + + for key, group in sorted(grouped.items(), key=lambda kv: SEV_ORDER[kv[1][0]["severity"]]): + sev = group[0]["severity"] + tag = f"{COLOR[sev]}{sev.upper():<6}{RESET}" if use_color else f"{sev.upper():<6}" + out.append(f"{tag} {key} ({group[0]['lens']} lens, {len(group)} site" + f"{'s' if len(group) > 1 else ''})") + out.append(f" device: {group[0]['device']}") + for f in group[:8]: + out.append(f" {f['file']}:{f['line']} {f['snippet']}") + if len(group) > 8: + out.append(f" … and {len(group) - 8} more") + out.append("") + + return "\n".join(out) + + +def main() -> int: + ap = argparse.ArgumentParser( + description="Detect poka-yoke hazards, shapes in code that make mistakes easy.", + formatter_class=argparse.RawDescriptionHelpFormatter, + epilog=__doc__.split("Examples:")[-1], + ) + src = ap.add_mutually_exclusive_group() + src.add_argument("--diff", action="store_true", + help="scan uncommitted changes (falls back to HEAD~5..HEAD if clean)") + src.add_argument("--staged", action="store_true", help="scan staged changes") + src.add_argument("--since", metavar="REF", help="scan changes since REF (e.g. HEAD~10)") + src.add_argument("--paths", nargs="+", metavar="PATH", help="scan these files or directories") + ap.add_argument("--severity", choices=["high", "medium", "low"], default="low", + help="minimum severity to report (default: low)") + # Until this existed the script ended in a bare `return 0`, so every gate built on it was + # decorative: the shipped pre-commit hook, the shipped CI template and this repo's own + # "Detector runs clean" step all reported success while printing high-severity findings. + # A linter that cannot fail is a linter nobody has to satisfy. + ap.add_argument("--fail-on", choices=["high", "medium", "low", "none"], default="low", + metavar="SEVERITY", + help="exit non-zero when a finding of at least this severity is reported " + "(default: low, i.e. any reported finding). Use 'none' to report " + "without gating.") + ap.add_argument("--id", nargs="+", metavar="ID", + help="only report these hazard IDs (e.g. --id C1 F2 M2)") + ap.add_argument("--all", action="store_true", dest="include_covered", + help="also run the rules a real linter does better (off by default)") + ap.add_argument("--json", action="store_true", help="emit JSON") + ap.add_argument("--repo", default=".", help="repository root (default: .)") + args = ap.parse_args() + + global INCLUDE_COVERED + INCLUDE_COVERED = args.include_covered + repo = Path(args.repo).resolve() + findings: list[dict] = [] + + # poka-yoke: an empty scan reports itself instead of looking like a clean bill of health [control] + scanned = 0 + + if args.paths: + scope = f"paths: {', '.join(args.paths)}" + for p in collect_paths(args.paths): + scanned += 1 + findings += scan_file(p, None) + empty_because = f"Nothing under {', '.join(args.paths)} has a supported extension." + else: + mode = "staged" if args.staged else ("since" if args.since else "diff") + scope = {"staged": "staged changes", + "since": f"changes since {args.since}", + "diff": "uncommitted changes"}[mode] + try: + changed = changed_files_and_lines(repo, mode, args.since) + except GitUnavailable as exc: + # Exit 2, the same code --paths uses for "scanned nothing". Reporting a clean + # tree because git is broken is worse than reporting nothing at all: a + # pre-commit hook or CI gate reads only the exit code. + msg = (f"Could not determine what changed: {exc}\n" + f"This is NOT an all-clear. Use --paths to scan explicitly.") + print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, + "findings": [], "error": str(exc)}, indent=2) + if args.json else msg, file=sys.stdout if args.json else sys.stderr) + return 2 + for rel, lines in changed.items(): + fp = repo / rel + if fp.suffix in ALL_EXTS and fp.exists(): + scanned += 1 + findings += scan_file(fp, lines) + empty_because = ( + "No changed files found: the tree may be clean, or this may not be a git " + "repository." if not changed else + f"None of the {len(changed)} changed file(s) could be scanned. They were " + "deleted, or have no supported extension.") + + # poka-yoke: one exit for "scanned nothing", shared by every mode [control] + # + # This check used to live inside the --paths branch. --diff, --staged and --since each + # reached the end with scanned == 0 and returned 0, printing "No hazards detected" -- + # a false all-clear in precisely the modes a pre-commit hook and a CI gate use. The + # marker above said [control] while holding on one branch of three. + # + # It is out here now because a check placed after the branches cannot be present on one + # and missing from another. Adding a fourth input mode inherits it without remembering to. + if scanned == 0: + msg = (f"Scanned 0 files. This is NOT an all-clear.\n{empty_because}\n" + f"Supported extensions: {', '.join(sorted(ALL_EXTS))}\n" + "Use --paths to scan explicitly, e.g. detect_hazards.py --paths src/") + print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, + "findings": [], "error": msg}, indent=2) + if args.json else msg, file=sys.stdout if args.json else sys.stderr) + return 2 + + threshold = SEV_ORDER[args.severity] + findings = [f for f in findings if SEV_ORDER[f["severity"]] <= threshold] + if args.id: + wanted = {i.upper() for i in args.id} + findings = [f for f in findings if f["id"] in wanted] + + if args.json: + print(json.dumps({"scope": scope, "files_scanned": scanned, + "count": len(findings), "findings": findings}, indent=2)) + else: + print(render(findings, scope, use_color=sys.stdout.isatty())) + print(f"\nScanned {scanned} file{'' if scanned == 1 else 's'}.") + if not INCLUDE_COVERED: + tools = sorted({v.split(",")[0].split()[0] for v in COVERED_BY.values() if v}) + # len(COVERED_BY) counts ENTRIES, and one entry can suppress several + # per-language rules, so it under-reported by three. Count the rules. + n_suppressed = sum(1 for r in RULES if (r.id, r.name) in COVERED_BY) + # Names the linters rather than a path. `assets/devices/lint/` resolves only + # when this script runs from inside the full plugin; installed as a standalone + # skill it pointed at a directory the user does not have. + print(f"\nNot checked here, {n_suppressed} further hazard rules are covered " + f"better by {', '.join(tools)}.\nEnable those in your own linter config " + f"rather than relying on this. Use --all to run them anyway.") + + if args.fail_on != "none": + rank = {"high": 3, "medium": 2, "low": 1} + threshold = rank[args.fail_on] + gating = [f for f in findings if rank.get(f.get("severity", "low"), 1) >= threshold] + if gating: + worst = max(rank.get(f.get("severity", "low"), 1) for f in gating) + name = {3: "high", 2: "medium", 1: "low"}[worst] + if not args.json: + print(f"\n{len(gating)} finding(s) at or above --fail-on={args.fail_on} " + f"(worst: {name}). Exiting 1.", file=sys.stderr) + return 1 + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/poka-yoke/SKILL.md b/skills/poka-yoke/SKILL.md index 5ef7f70a8..3cb50e57a 100644 --- a/skills/poka-yoke/SKILL.md +++ b/skills/poka-yoke/SKILL.md @@ -1,6 +1,6 @@ --- name: poka-yoke -description: 'Mistake-proof code so misuse cannot be expressed, rather than warning against it. Use when designing an interface, schema, or state machine and the user wants it hard to get wrong ("make invalid states unrepresentable", "so callers cannot screw it up", "type-safe API", "pit of success"); when auditing existing code for footguns ("what could bite us here", "what is easy to misuse", "poka-yoke this repo", "review this diff for ways to get it wrong"); or when a bug has recurred and the fix must close the class rather than the case ("make sure this never happens again", "this is the third time"). Especially for money, auth, permissions, deletion, migrations, and pipelines where failure is silent. Classifies every finding by what happens when the mistake occurs and how the device notices, which is what keeps it from collapsing into generic code review.' +description: 'Mistake-proof code so misuse cannot be expressed, rather than warning against it. Use when designing an interface, schema or state machine and the user wants it hard to get wrong ("make invalid states unrepresentable", "type-safe API", "pit of success"); when auditing existing code for footguns ("what is easy to misuse here", "review this diff for ways to get it wrong"); or when a bug has recurred and the fix must close the class rather than the case. Especially for money, auth, deletion, migrations and pipelines, where failure is silent.' license: MIT compatibility: 'Cross-platform. The bundled scanner needs Python 3.9+ and no third-party packages. Everything else is language-agnostic guidance; worked examples are TypeScript, Python, Go, Rust and SQL.' metadata: @@ -33,8 +33,8 @@ fails the build does not. ## What this changes about the output Given a design, models will readily list what to fix. They rarely state what the fix makes -*impossible*, and that is the difference between advice you agree with and a constraint you -can rely on. That habit is most of what this skill is for. +*impossible*, and that is the difference between advice you agree with and a constraint you can +rely on. That habit is most of what this skill is for. The other half is refusing to accept a non-device as a fix. "Add validation", "be careful with this function", "document the invariant" are all rung zero. Each has a real device behind it, @@ -69,90 +69,35 @@ The cheapest place to catch a mistake is where it is made, not where it surfaces that runs three layers below the input has already let the bad value travel, and the stack trace will point at the wrong module. Push the check to the boundary the value crosses. -## Designing something new +## Applying it -Mistake-proofing is cheapest before the code has callers. Once it has them, every device is a -migration; before it has them, a device is free. - -Work from the call site. A signature that reads fine in isolation often reads terribly where -it is used: - -```python -# the mistake is expressible: nothing stops refunding an order that was never paid -def refund(order: dict) -> Refund: - return payments.refund(order["payment_id"]) - -# the mistake is no longer expressible -def refund(order: PaidOrder) -> Refund: ... -``` - -The moves, roughly in order of how often they apply: - -**Make invalid states unrepresentable.** A bag of optional fields where only certain -combinations are legal becomes a discriminated union where the illegal ones cannot be -constructed. - -**Parse, don't validate.** Convert unstructured input into a type that carries proof at the -boundary, once, rather than re-checking the same string in nine places. - -**Distinguish concepts that share a primitive.** `transfer(from: str, to: str)` accepts its -arguments transposed. Distinct types for the two concepts, or keyword-only parameters, make the -transposition a compile error. - -**Encode the order.** When calls must happen in sequence, let each step return the type the -next step requires, so the wrong order does not typecheck. - -**Make the destructive path narrower than the safe one.** A required, non-defaulting argument -for the scope of a delete. A default that means "nothing" rather than "everything". - -Close by naming what the design now makes impossible, and, just as importantly, **what you -deliberately left possible and why**. A design whose limits are unstated will be trusted past +**Designing something new.** Mistake-proofing is cheapest before the code has callers: after +them, every device is a migration. Work from the call site — `refund(order: dict)` cannot stop +you refunding an unpaid order; `refund(order: PaidOrder)` can. Make invalid states +unrepresentable; parse rather than validate at the boundary; distinguish concepts that share a +primitive; encode the order so the wrong sequence does not typecheck; make the destructive path +narrower than the safe one. Close by naming what the design now makes impossible, **and what you +deliberately left possible and why** — a design whose limits are unstated will be trusted past them. -## Auditing code that already exists - -You are not looking for bugs. A bug is a mistake that already happened. You are looking for -**mistakes that are available**: places where doing the wrong thing is easy, silent, and looks -correct. - -Run the bundled scanner first for the textually detectable shapes, then read for the ones no -scanner can see: +**Auditing code that already exists.** You are not looking for bugs; a bug is a mistake that +already happened. You are looking for **mistakes that are available**: places where doing the +wrong thing is easy, silent, and looks correct. Run the bundled scanner for the textually +detectable shapes, then read for the ones no scanner can see: ```bash -python3 scripts/detect_hazards.py --paths . # whole tree -python3 scripts/detect_hazards.py --staged # pre-commit -python3 scripts/detect_hazards.py --diff --json # CI, exits non-zero on findings -python3 scripts/detect_hazards.py --severity high +python3 scripts/detect_hazards.py --paths . # also --staged, --diff, --json, --severity high ``` -No dependencies, so it runs in CI and in a pre-commit hook without an install step. It reports -what it scanned: a scan of zero files exits non-zero rather than reporting a clean bill of -health, because an all-clear you got by typo is worse than no check. +Standard library only, so it runs in CI and in a pre-commit hook. A scan of zero files exits +non-zero rather than reporting a clean bill of health, because an all-clear you got by typo is +worse than no check. Rank findings by **blast radius × ease of mistake**. -Rank findings by **blast radius times ease of the mistake**. An unchecked value reaching a -write, a delete, a payment or an auth decision outranks one that can only produce a clean -crash. For each finding, state: where it is, what the mistake is, what the consequence is, what -device exists today, what device would close it, and which rung that reaches. - -`references/hazard-catalog.md` is the taxonomy of shapes with their IDs and devices. -Language-specific patterns are in `references/lang-python.md`, `references/lang-typescript.md` -and `references/lang-rust-go.md`. - -## After an incident - -Separate three things that get conflated, because the fix belongs to the third: - -- **Defect** — what the user experienced. -- **Mistake** — the specific wrong action someone took. -- **Hazard** — the property of the system that made that mistake available. - -Fixing the mistake fixes one case. Fixing the hazard fixes the class. Then **sweep**: the same -shape almost certainly exists elsewhere, and finding the second and third instance is the -difference between a patch and a lesson. - -Attribute cause to the system rather than to a person. Not primarily for kindness: "they made a -mistake" is a complete-sounding explanation that predicts nothing and prevents nothing, and it -ends the investigation early. +**After an incident.** Separate the **defect** (what the user experienced), the **mistake** (the +wrong action someone took) and the **hazard** (the property that made that mistake available). +Fixing the mistake fixes one case; fixing the hazard fixes the class. Then sweep — the same +shape almost certainly exists elsewhere. Attribute cause to the system, not to a person: "they +made a mistake" predicts nothing and ends the investigation early. ## What good output looks like @@ -165,39 +110,51 @@ ends the investigation early. ## What to avoid -**Accepting rung zero as a fix.** If the proposal is a comment, a doc, or a convention, the -work is not finished. +**Accepting rung zero as a fix.** If the proposal is a comment, a doc, or a convention, the work +is not finished. -**Devices nobody can bypass being confused with devices nobody does bypass.** A pre-commit hook -is skippable with `--no-verify`; it needs CI behind it to be a real gate. Say which one you are -proposing. +**Confusing devices nobody *can* bypass with devices nobody *does*.** A pre-commit hook is +skippable with `--no-verify`; it needs CI behind it to be a real gate. Say which you propose. **Over-fitting to one incident.** Machinery that prevents one specific failure must itself be -understood and maintained. Ask whether the shape is common enough to justify it. +maintained. Ask whether the shape is common enough to justify it. + +**Treating monitoring as prevention.** Detection lowers the cost of a failure, not its +likelihood. Conflating them means the likelihood never gets addressed. -**Treating monitoring as prevention.** Detection lowers the cost of a failure; it does not lower -the likelihood. Both are worth having, and conflating them means the likelihood never gets -addressed. +## Specialist modes + +This skill carries the method and is enough on its own. Four companion skills carry the working +detail it does not: + +| Skill | For | +|---|---| +| `poka-yoke-design` | A new API, schema, type or state machine — make misuse unrepresentable | +| `poka-yoke-audit` | Existing code: swappable arguments, silent fallbacks, unguarded deletes | +| `poka-yoke-retro` | After an incident, when the fix must close the class rather than the case | +| `poka-yoke-guardrails` | Pre-commit hooks, CI gates, lint rules, database constraints | + +They compose. An incident involving a bad migration is `poka-yoke-retro` for the analysis and +`poka-yoke-guardrails` for the device: the retro decides what to install, the other decides +which device. ## Evidence, and its limits -This method was benchmarked at 591 blind-graded runs across six model families, scored against -assertions written before the runs by a grader that never saw which configuration produced a -response. The behaviour it most reliably changes is stating what a design forecloses: **45% of -responses did that unprompted, 80% with the method applied**, across 132 graded verdicts. - -That average conceals where the effect lives. Asked squarely to design an interface, models -already do it 77% of the time; the skills add eleven points. The large gains are in tasks -where nobody asked for a design review — writing an endpoint goes 14% to 79%, shipping an -agent feature 33% to 83%, building a form 29% to 64%. - -Stated honestly, because the limits matter: every run was the first turn of a fresh session, so -this measures the ceiling rather than what survives a long working session. The comparison was -against no methodology at all, not against a different one, so it does not establish that -*this* method is what produced the gain. And the method costs something measurable: responses -became somewhat worse at spotting the specific defect already on the page while becoming better -at changing the shape that allowed it. If you want the bug in front of you found, use a -reviewer. If you want that class of bug to stop being expressible, use this. - -Raw runs, the harness and the assertion checklists are at -. +Benchmarked at 591 blind-graded runs across six model families, scored against assertions +written before the runs by a grader that never saw which configuration produced a response. The +behaviour it most reliably changes is stating what a design forecloses: **45% of responses did +that unprompted, 80% with the method applied**, across 132 graded verdicts. + +The average conceals where the effect lives. Asked squarely to design an interface, models +already do it 77% of the time. The large gains are where nobody asked for a design review — +writing an endpoint goes 14% to 79%, building a form 29% to 64%. + +The limits matter. Every run was the first turn of a fresh session, so this measures the ceiling +rather than what survives a long working session. The comparison was against no methodology at +all, not a different one, so it does not establish that *this* method produced the gain. And the +method costs something measurable: responses became somewhat worse at spotting the specific +defect already on the page while becoming better at changing the shape that allowed it. If you +want the bug in front of you found, use a reviewer. If you want that class of bug to stop being +expressible, use this. + +Raw runs, harness and assertion checklists: . diff --git a/skills/poka-yoke/references/hazard-catalog.md b/skills/poka-yoke/references/hazard-catalog.md index b7808e377..b2e1c56ba 100644 --- a/skills/poka-yoke/references/hazard-catalog.md +++ b/skills/poka-yoke/references/hazard-catalog.md @@ -397,9 +397,8 @@ switch, `# type: ignore`, `unsafe`, `cast()`, `Object` as a parameter type. in the boundary code that most needs the guarantee. **Device**: ban them by lint at error level with a narrow, justified allowlist; replace with -parsing at the boundary. **Warning**: a required CI gate is still rung 2 by the ladder in -[method.md](../../../docs/method.md): it announces the mistake rather than removing the -ability to make it. Reach **Control** only when the unchecked value cannot be constructed. +parsing at the boundary. **Warning**: a required CI gate is still rung 2 on the ladder: it announces the mistake +rather than removing the ability to make it. Reach **Control** only when the unchecked value cannot be constructed. ### X5. Mutable shared defaults diff --git a/skills/poka-yoke/references/lang-typescript.md b/skills/poka-yoke/references/lang-typescript.md index 470b626ec..fc903af5a 100644 --- a/skills/poka-yoke/references/lang-typescript.md +++ b/skills/poka-yoke/references/lang-typescript.md @@ -92,7 +92,15 @@ the language: one line per switch. Encode required steps in the type so `.delete()` doesn't exist until they've run: ```ts +declare const state: unique symbol; + class QueryBuilder { + // Load-bearing. TypeScript is structural: a type parameter that no member mentions does + // not affect assignability, so without this line QueryBuilder is assignable + // to QueryBuilder and `delete()` is callable with no where clause -- the exact + // mistake the class claims to prevent, silently permitted. Verified with tsc 5 --strict. + private declare readonly [state]: [HasFrom, HasWhere]; + from(t: string): QueryBuilder { /* … */ } where(c: Cond): QueryBuilder { /* … */ } @@ -101,8 +109,8 @@ class QueryBuilder str: +# poka-yoke: keyword-only, so the id and the name cannot be passed transposed [control] +def covered(*, rule_id: str, name: str) -> str: return COVERED_BY.get((rule_id, name), "") @@ -325,12 +326,25 @@ def python_ast_findings(path: Path, source: str) -> list[dict]: } +class GitUnavailable(RuntimeError): + """git could not answer the question asked of it. + + Previously any git failure became an empty string, which the caller could not tell from + "the tree is clean". A detector that reports a clean bill of health because git is broken + is the exact failure this file's own rules exist to catch. + """ + + def git(*args: str, cwd: Path) -> str: try: r = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True, timeout=30) - return r.stdout if r.returncode == 0 else "" - except (subprocess.SubprocessError, FileNotFoundError): - return "" + except (subprocess.SubprocessError, FileNotFoundError) as exc: + raise GitUnavailable(f"could not run git {' '.join(args)}: {exc}") from exc + if r.returncode != 0: + detail = (r.stderr or r.stdout or "").strip().splitlines() + raise GitUnavailable(f"git {' '.join(args)} exited {r.returncode}" + + (f": {detail[0]}" if detail else "")) + return r.stdout def changed_files_and_lines(cwd: Path, mode: str, since: str | None): @@ -406,7 +420,7 @@ def scan_file(path: Path, only_lines: set[int] | None) -> list[dict]: for rule in RULES: if ext not in rule.exts: continue - if not INCLUDE_COVERED and covered(rule.id, rule.name): + if not INCLUDE_COVERED and covered(rule_id=rule.id, name=rule.name): continue if rule.negate and (rule.negate.search(line) or rule.negate.search(str(path))): continue @@ -422,7 +436,7 @@ def scan_file(path: Path, only_lines: set[int] | None) -> list[dict]: if ext in PY: for f in python_ast_findings(path, source): - if not INCLUDE_COVERED and covered(f["id"], f["name"]): + if not INCLUDE_COVERED and covered(rule_id=f["id"], name=f["name"]): continue if not only_lines or f["line"] in only_lines: findings.append(f) @@ -497,6 +511,15 @@ def main() -> int: src.add_argument("--paths", nargs="+", metavar="PATH", help="scan these files or directories") ap.add_argument("--severity", choices=["high", "medium", "low"], default="low", help="minimum severity to report (default: low)") + # Until this existed the script ended in a bare `return 0`, so every gate built on it was + # decorative: the shipped pre-commit hook, the shipped CI template and this repo's own + # "Detector runs clean" step all reported success while printing high-severity findings. + # A linter that cannot fail is a linter nobody has to satisfy. + ap.add_argument("--fail-on", choices=["high", "medium", "low", "none"], default="low", + metavar="SEVERITY", + help="exit non-zero when a finding of at least this severity is reported " + "(default: low, i.e. any reported finding). Use 'none' to report " + "without gating.") ap.add_argument("--id", nargs="+", metavar="ID", help="only report these hazard IDs (e.g. --id C1 F2 M2)") ap.add_argument("--all", action="store_true", dest="include_covered", @@ -518,34 +541,52 @@ def main() -> int: for p in collect_paths(args.paths): scanned += 1 findings += scan_file(p, None) - if scanned == 0: - # Zero findings from zero files is not an all-clear, and it used to be - # indistinguishable from one. Exit non-zero: failing to do the job should - # not look like doing the job and finding nothing. - msg = ("Scanned 0 files. This is NOT an all-clear.\n" - f"Nothing under {', '.join(args.paths)} has a supported extension.\n" - f"Supported: {', '.join(sorted(ALL_EXTS))}") - print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, - "findings": [], "error": msg}, indent=2) - if args.json else msg, file=sys.stdout if args.json else sys.stderr) - return 2 + empty_because = f"Nothing under {', '.join(args.paths)} has a supported extension." else: mode = "staged" if args.staged else ("since" if args.since else "diff") scope = {"staged": "staged changes", "since": f"changes since {args.since}", "diff": "uncommitted changes"}[mode] - changed = changed_files_and_lines(repo, mode, args.since) - if not changed: - msg = ("No changed files found. The tree may be clean and have no recent commits, " - "or this may not be a git repository.\nUse --paths to scan explicitly, " - "e.g. detect_hazards.py --paths src/") - print(json.dumps({"findings": [], "note": msg}) if args.json else msg) - return 0 + try: + changed = changed_files_and_lines(repo, mode, args.since) + except GitUnavailable as exc: + # Exit 2, the same code --paths uses for "scanned nothing". Reporting a clean + # tree because git is broken is worse than reporting nothing at all: a + # pre-commit hook or CI gate reads only the exit code. + msg = (f"Could not determine what changed: {exc}\n" + f"This is NOT an all-clear. Use --paths to scan explicitly.") + print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, + "findings": [], "error": str(exc)}, indent=2) + if args.json else msg, file=sys.stdout if args.json else sys.stderr) + return 2 for rel, lines in changed.items(): fp = repo / rel if fp.suffix in ALL_EXTS and fp.exists(): scanned += 1 findings += scan_file(fp, lines) + empty_because = ( + "No changed files found: the tree may be clean, or this may not be a git " + "repository." if not changed else + f"None of the {len(changed)} changed file(s) could be scanned. They were " + "deleted, or have no supported extension.") + + # poka-yoke: one exit for "scanned nothing", shared by every mode [control] + # + # This check used to live inside the --paths branch. --diff, --staged and --since each + # reached the end with scanned == 0 and returned 0, printing "No hazards detected" -- + # a false all-clear in precisely the modes a pre-commit hook and a CI gate use. The + # marker above said [control] while holding on one branch of three. + # + # It is out here now because a check placed after the branches cannot be present on one + # and missing from another. Adding a fourth input mode inherits it without remembering to. + if scanned == 0: + msg = (f"Scanned 0 files. This is NOT an all-clear.\n{empty_because}\n" + f"Supported extensions: {', '.join(sorted(ALL_EXTS))}\n" + "Use --paths to scan explicitly, e.g. detect_hazards.py --paths src/") + print(json.dumps({"scope": scope, "files_scanned": 0, "count": 0, + "findings": [], "error": msg}, indent=2) + if args.json else msg, file=sys.stdout if args.json else sys.stderr) + return 2 threshold = SEV_ORDER[args.severity] findings = [f for f in findings if SEV_ORDER[f["severity"]] <= threshold] @@ -564,9 +605,24 @@ def main() -> int: # len(COVERED_BY) counts ENTRIES, and one entry can suppress several # per-language rules, so it under-reported by three. Count the rules. n_suppressed = sum(1 for r in RULES if (r.id, r.name) in COVERED_BY) + # Names the linters rather than a path. `assets/devices/lint/` resolves only + # when this script runs from inside the full plugin; installed as a standalone + # skill it pointed at a directory the user does not have. print(f"\nNot checked here, {n_suppressed} further hazard rules are covered " - f"better by {', '.join(tools)}.\nEnable those rather than relying on this: " - f"see assets/devices/lint/. Use --all to run them anyway.") + f"better by {', '.join(tools)}.\nEnable those in your own linter config " + f"rather than relying on this. Use --all to run them anyway.") + + if args.fail_on != "none": + rank = {"high": 3, "medium": 2, "low": 1} + threshold = rank[args.fail_on] + gating = [f for f in findings if rank.get(f.get("severity", "low"), 1) >= threshold] + if gating: + worst = max(rank.get(f.get("severity", "low"), 1) for f in gating) + name = {3: "high", 2: "medium", 1: "low"}[worst] + if not args.json: + print(f"\n{len(gating)} finding(s) at or above --fail-on={args.fail_on} " + f"(worst: {name}). Exiting 1.", file=sys.stderr) + return 1 return 0