From bf84344da2ec40e36f1ed64f43103e984ccb8009 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:55:39 +0000 Subject: [PATCH 1/2] refactor(core): remove dead branches and codify a no-dead-code rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The coverage push surfaced arms that can never run: parseParallel's undefined case (its one call site always slices a string), the job-level bootstrap fallback behind an optional resolver that is never absent (the parameter is now required), and caseList's wildcard arm that memberTest answers before ever calling it (the parameter type now excludes it). pathForField's doc claimed a lowercase field name reduces to the provider default when the match is deliberately case-sensitive to protect camelCase word boundaries — the doc now says what the code does. The distinction the new rule draws: narrowing the type system forces is not dead code — the no-as/no-non-null style demands it — but a branch still unreachable once types are as tight as the call sites allow gets deleted, never covered for the gate's sake. Recorded as coding guideline 10 in AGENTS.md, mirrored in CONTRIBUTING.md and the PR checklist. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019As9A3ugixLQiAvcat1ZKk --- .github/pull_request_template.md | 1 + AGENTS.md | 42 +++++++++++++++++++++----------- CONTRIBUTING.md | 35 +++++++++++++++----------- cspell.json | 2 ++ packages/core/src/ci.ts | 10 +++++--- packages/core/src/ci_schedule.ts | 7 +++--- packages/core/src/cli.ts | 6 ++--- 7 files changed, 66 insertions(+), 37 deletions(-) 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; } From 9e98a34bd15b96026698d037a2789352285531bc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 13:02:41 +0000 Subject: [PATCH 2/2] chore: retrigger CI so prBodyLint sees the updated PR body A workflow re-run reuses the original event payload, so the lint kept judging the pre-edit description. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019As9A3ugixLQiAvcat1ZKk