diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index aef24c8e..35cbe7a0 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -30,6 +30,7 @@ motivation, not just the mechanics. --> - [ ] Docs updated in the same PR (`README.md`, JSDoc, `docs/`) when behaviour changed. - [ ] Public API changes were regenerated with `./zuke apiDocs` (`llms.txt`, `llms-full.txt`, package README `## API`). - [ ] No `any`, no `as` casts or `!` non-null assertions in `src/` (narrow with type guards instead). +- [ ] No dead code: unreachable branches and can't-fire fallbacks are removed, with types tightened so the impossible state is unrepresentable. - [ ] A new package was wired into all seven places (see [AGENTS.md](../blob/master/AGENTS.md#good-open-source-practices-to-follow)), if applicable. - [ ] The code is written using AI assisted coding. diff --git a/AGENTS.md b/AGENTS.md index 46f93fb3..f26f483d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -207,7 +207,22 @@ can't see Deno's module graph. header from its template (see `internal/hcl_tool.ts.tmpl`) — put the header in the template, never hand-edit generated output. -10. **Configuration is a fluent settings lambda, not an options object.** When +10. **No dead code.** Code that cannot execute does not ship: remove unreachable + branches, unused helpers and exports, and fallbacks whose condition can + never fire (e.g. a `?? default` behind a parameter no call site ever omits). + Prefer making the impossible state _unrepresentable_ — tighten the + parameter's type, drop the `?`, narrow with `Exclude<...>` — over keeping a + loose signature guarded by an arm that never runs. Keep the line straight, + though: narrowing that the **type system** forces (`map.get(x) ?? fallback`, + `value instanceof Error ? value.message : String(value)`, an `undefined` + check after `.find(...)`) is not dead code — guideline 1 bans the `!`/`as` + shortcuts that would replace it. A branch is dead only when it is + unreachable _after_ the types are as tight as the call sites allow. And + never write a test whose sole purpose is to "cover" a dead arm — an + uncoverable branch is the signal to delete the branch, not to feed the + coverage gate. + +11. **Configuration is a fluent settings lambda, not an options object.** When an API takes more than a trivial amount of configuration, expose it as a chainable settings class configured through a lambda — the `Configure = (s: S) => S` shape the tool wrappers use — not a positional @@ -421,11 +436,11 @@ gemini-extension.json # Gemini CLI extension manifest (serves skills/) `.agents/plugins/marketplace.json`), and the root `gemini-extension.json` makes the repo a Gemini CLI extension that auto-discovers `skills/`. The `skillsCheck` gate target validates `skills/` against the Agent Skills spec - (frontmatter `name` must match the folder), since Codex and Gemini load - those folders directly. Any change to - the authoring surface or to a documented guarantee — a new `target()` method, - a new `Build` override, changed CLI or authorization semantics — must be - reflected in `skills/zuke-write-build/SKILL.md` and + (frontmatter `name` must match the folder), since Codex and Gemini load those + folders directly. Any change to the authoring surface or to a documented + guarantee — a new `target()` method, a new `Build` override, changed CLI or + authorization semantics — must be reflected in + `skills/zuke-write-build/SKILL.md` and `skills/zuke-write-build/references/cheatsheet.md` **in the same PR**. The cheatsheet is one of the two canonical answers to "does a wrapper exist?", so a new package belongs in its catalogue table as well. Then, in order: @@ -436,18 +451,17 @@ gemini-extension.json # Gemini CLI extension manifest (serves skills/) `plugins/zuke/.codex-plugin/plugin.json`, the entry in `.claude-plugin/marketplace.json`, and the root `gemini-extension.json` (the `VERSIONED_MANIFESTS` list in `build/plugin_version_check.ts`). - Clients use the version to decide - whether an installed plugin is stale, so skills edited without a bump - simply never reach agents that already hold the old copy. release-please - does **not** manage `plugins/` — it is not a workspace package and has no - `deno.json`. Additive skill content is a minor bump; a correction is a - patch. + Clients use the version to decide whether an installed plugin is stale, so + skills edited without a bump simply never reach agents that already hold + the old copy. release-please does **not** manage `plugins/` — it is not a + workspace package and has no `deno.json`. Additive skill content is a minor + bump; a correction is a patch. Two gate targets hold this up, so a miss fails the build rather than shipping quietly: `pluginVersionCheck` fails when a published skill changed against the base branch and the version did not move, and `tests/plugin_manifest_test.ts` - fails when the manifests disagree. `pluginVersionCheck` is the one part of - the gate that needs history — it compares against `origin/`, or + fails when the manifests disagree. `pluginVersionCheck` is the one part of the + gate that needs history — it compares against `origin/`, or `ZUKE_PLUGIN_BASE_REF` when you set one — and it reports itself _skipped_, never passed, in a clone that has no base to compare against. - **Always read the reviewer comments on every PR.** This repo runs AI reviewers diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f534b23c..dc517e0a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -68,6 +68,13 @@ code in this repo is written (`CLAUDE.md` is a one-line pointer to it): 5. **Tests are hermetic and fast.** No network and no reliance on ambient tools. When a test needs a subprocess, invoke `Deno.execPath()` (the running `deno`), which is always present and shell-free. +6. **No dead code.** Remove unreachable branches, unused helpers, and fallbacks + that can never fire — tighten the types so the impossible state is + unrepresentable rather than guarding it with an arm that never runs. + Narrowing the type system itself forces (a `?? fallback` after `Map.get`, an + `instanceof Error` check in a `catch`) is not dead code; see + [`AGENTS.md`](./AGENTS.md#coding-guidelines-non-negotiable) for the full + rule. See the architecture notes in [`AGENTS.md`](./AGENTS.md#architecture-notes) for how targets, the dependency graph, the shell `$`, and tool wrappers fit @@ -98,34 +105,34 @@ the squash commit that [release-please](./RELEASING.md) parses. ## Code review -Every change reaches `master` through a pull request — there is no direct -push path — and review has documented requirements: +Every change reaches `master` through a pull request — there is no direct push +path — and review has documented requirements: **How review is conducted.** Each PR is reviewed by (1) the required CI gate (`deno task ci`, the same gate you run locally), (2) the AI reviewers, which -post a security assessment and a code-quality assessment as PR comments, and -(3) a human maintainer, who reads the diff and every reviewer finding. AI -findings are advisory: a maintainer addresses each one or answers it on the -thread, quoting the finding's id — they never merge unexamined. +post a security assessment and a code-quality assessment as PR comments, and (3) +a human maintainer, who reads the diff and every reviewer finding. AI findings +are advisory: a maintainer addresses each one or answers it on the thread, +quoting the finding's id — they never merge unexamined. **What must be checked.** Reviewers verify that the change: - is correct, and covered by tests per the testing policy above (unit + integration in the same PR; e2e for cross-process or cross-OS behaviour); - introduces no security regression (injection, privilege escalation, secret - exposure — see the - [assurance case](./docs/assurance-case.md) for the boundaries to respect); + exposure — see the [assurance case](./docs/assurance-case.md) for the + boundaries to respect); - meets the coding standards above (strict types, no `any`/`as`/`!`, JSDoc on all public symbols) and keeps coverage at 95%+; -- updates the affected docs in the same PR, and regenerates the API docs on - any public-API change; +- updates the affected docs in the same PR, and regenerates the API docs on any + public-API change; - carries a Conventional Commit PR title, since the squash subject is what release-please parses. -**What is required to be acceptable.** A PR merges only when the required -status checks are green, every AI-reviewer finding has been fixed or answered, -and a maintainer approves. Larger features additionally get an adversarial -review pass before the PR is finalized (see +**What is required to be acceptable.** A PR merges only when the required status +checks are green, every AI-reviewer finding has been fixed or answered, and a +maintainer approves. Larger features additionally get an adversarial review pass +before the PR is finalized (see [`AGENTS.md`](./AGENTS.md#adversarial-review-every-feature)). ## Reporting bugs and requesting features diff --git a/cspell.json b/cspell.json index 20dbf905..cc401039 100644 --- a/cspell.json +++ b/cspell.json @@ -13,6 +13,8 @@ "headsha", "idempotently", "unanchorable", + "uncoverable", + "unrepresentable", "unresolve", "unresolving", "GHES", diff --git a/packages/core/src/ci.ts b/packages/core/src/ci.ts index d5d21d1f..8accbc85 100644 --- a/packages/core/src/ci.ts +++ b/packages/core/src/ci.ts @@ -521,7 +521,7 @@ function stepsCoverPrelude(steps: readonly CiStep[] | undefined): boolean { */ function jobBootstrap( job: CiJob, - pins?: CiPinResolver, + pins: CiPinResolver, ): CiBootstrap | false | undefined { const pinnedSeparately = (job.harden !== false && job.harden?.action !== undefined) || @@ -531,9 +531,11 @@ function jobBootstrap( ? (pinnedSeparately ? false : undefined) : job.bootstrap; if (declared === false || declared === undefined) return declared; + // No DEFAULT_ZUKE_ACTION fallback here: this only runs from `withPins`' + // resolver-present branch, so the resolver always answers. return { ...declared, - action: declared.action ?? pins?.(ZUKE_ACTION) ?? DEFAULT_ZUKE_ACTION, + action: declared.action ?? pins(ZUKE_ACTION), }; } @@ -1002,7 +1004,9 @@ const DEFAULT_SETUP_STEPS: CiStep[] = [{ uses: "actions/checkout@v4" }]; * * A trailing `Workflow`, `Ci`, or `Yaml` is noise once the file is a workflow, * and camelCase reads better as kebab-case in a filename. A name that reduces to - * nothing (a field called just `workflow`) keeps the provider's default. + * nothing (a field called just `Workflow` or `Ci` — the match is deliberately + * case-sensitive, so a camelCase word boundary is never split) keeps the + * provider's default. */ function pathForField(field: string, provider: CiProvider): string { const leaf = field.split(".").pop() ?? field; diff --git a/packages/core/src/ci_schedule.ts b/packages/core/src/ci_schedule.ts index c7b21599..adcc7501 100644 --- a/packages/core/src/ci_schedule.ts +++ b/packages/core/src/ci_schedule.ts @@ -253,9 +253,10 @@ export function anyScheduleNeedsGuard( return schedule.some(scheduleNeedsGuard); } -/** A space-padded membership list for a `case` test, or "" for `*` (always matches). */ -function caseList(field: Field): string { - return field === "*" ? "" : ` ${field.join(" ")} `; +/** A space-padded membership list for a `case` test. `*` never reaches here — + * {@link memberTest} answers `"true"` for it before building a list. */ +function caseList(field: Exclude): string { + return ` ${field.join(" ")} `; } /** One `case`-based membership test on `value`, or "true" when the field is `*`. */ diff --git a/packages/core/src/cli.ts b/packages/core/src/cli.ts index db433704..ce76fb9a 100644 --- a/packages/core/src/cli.ts +++ b/packages/core/src/cli.ts @@ -187,9 +187,9 @@ export interface ParsedArgs { help: boolean; } -/** Parse a `--parallel`/`--parallel=N` value: a positive count, or `true`. */ -function parseParallel(value: string | undefined): boolean | number { - if (value === undefined || value === "") return true; +/** Parse a `--parallel=N` value (the inline text after `=`): a positive count, or `true`. */ +function parseParallel(value: string): boolean | number { + if (value === "") return true; const n = Number(value); return Number.isFinite(n) && n > 0 ? Math.floor(n) : true; }