feat(ci): dead-code detection gate — knip + no-unused-vars + vulture (#282) (#520)

ClintEastman02 · bgagent · krokoko · web-flow · commit c4b2b7ecf85a · 2026-07-01T18:51:52.000Z
* feat(ci): dead-code detection gate — knip + no-unused-vars + vulture (#282) Closes the unused-symbol detection asymmetry (cairn MVG gate #6): Python already fails on unused imports (ruff F401) but unused TS symbols passed silently in cdk/. Adds three detection layers plus advisory CI wiring. - TS lint-level: @typescript-eslint/no-unused-vars (error) in cdk+cli flat configs with ^_ ignore patterns; cdk tsconfig flips noUnusedLocals/noUnusedParameters to true (parity with cli). - TS project-graph: knip v6 with per-workspace knip.json (cdk/cli/docs) + a count-only ratchet (scripts/check-deadcode-ratchet.mjs, knip-baseline.json) that fails only when the count rises — the cairn "dead-code count should not increase" metric. - Python module-level: vulture 2.16 (config in pyproject [tool.vulture], min-confidence 80) with a minimal reviewed .vulture_allowlist.py for the SDK-mandated hook_context params. - CI: advisory dead-code job in security-pr.yml (continue-on-error, emits ::notice::/::warning:: annotations; not blocking yet). Resolves the three known live instances: drops the dead DEFAULT_MAX_TURNS import, removes the orphaned cdk/src/bootstrap/index.ts barrel (consumers import the submodules directly), and removes the unused LINEAR_SECRET_PREFIX export. Also clears the unused symbols the new rule surfaces across cdk/cli. The eslint half is blocking via the existing build check; knip+vulture stay advisory until the knip baseline is driven to zero (deferred AC). * fix(agent): exclude vulture allowlist from ty typecheck (#282) The ty typecheck step scanned .vulture_allowlist.py and failed with unresolved-reference on the bare hook_context names (the allowlist is bare-name expressions by design, not type-checkable Python). Same conflict already handled for ruff via extend-exclude; add the matching [tool.ty.src] exclude so `mise run build` passes. * chore(ci): ratchet dead-code baseline 79 → 78 after main merge (#282) Merging main (#505 CLI dedupe) removed a dead symbol, dropping knip's count to 78. Lock in the gain so the ratchet holds at the lower number. * fix(cdk): restore exhaustiveness guard + correct knip ratchet keys (#282) Addresses PR review (ayushtr-aws): 1. validation.ts: the `void (true as _AssertAttachmentExhaustive)` form silently defeated the AttachmentType exhaustiveness guard — a type assertion never errors, whereas the original ASSIGNMENT to a `never` type is the actual compile-time check. Restored the assignment and consume the binding with `void` so it survives noUnusedLocals and no-unused-vars without weakening the guard. Verified: tsc errors when a member is missing from ATTACHMENT_TYPE_LIST, compiles clean when exhaustive. 2. check-deadcode-ratchet.mjs: COUNTED_KEYS used non-existent knip 6.x keys (classMembers, nsExports, nsTypes) and counted unused files from a non-existent top-level report.files. Switched to the real keys (namespaceMembers, catalog) and count files from issues[].files[] so unused files actually move the ratchet. Count unchanged at 78. * chore(ci): harden dead-code ratchet + comment accuracy (#282 review nits) Non-blocking follow-ups from krokoko's review: - Pin knip exactly (6.20.0, drop caret) and make the ratchet fail loud instead of open: if a future knip reshapes its JSON so `issues` is not an array, countIssues now exits 2 rather than silently counting 0 and reporting a passing build (which would disable the gate). Needed before the gate flips to blocking. - COUNTED_KEYS comment: note the list is complete for the pinned knip and that nsExports/nsTypes/classMembers are not part of its schema. - validation.ts: attribute the `void` to tsconfig noUnusedLocals specifically (no-unused-vars already exempts the ^_ binding). * refactor(ci): move dead-code job to its own workflow + use uv run for vulture Address PR #520 review feedback: - Extract dead-code detection into dead-code-pr.yml (separate concern from security gate) - Use `uv run vulture` instead of `uvx vulture` for consistency with project conventions --------- Co-authored-by: bgagent <bgagent@noreply.github.com> Co-authored-by: Alain Krok <alkrok@amazon.com>
diff --git a/.github/workflows/dead-code-pr.yml b/.github/workflows/dead-code-pr.yml
@@ -0,0 +1,64 @@
+name: dead-code-pr
+# Advisory dead-code detection gate (issue #282, cairn MVG gate #6).
+# Steps use continue-on-error so findings surface as annotations without
+# blocking the merge queue. Flips to required/blocking once the knip
+# baseline (knip-baseline.json) is driven to zero. The eslint no-unused-vars
+# half of the gate is already blocking via the `build` check.
+on:
+  pull_request: {}
+  merge_group: {}
+  workflow_dispatch: {}
+
+permissions:
+  contents: read
+
+concurrency:
+  group: dead-code-pr-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  dead-code:
+    name: Dead-code detection (advisory)
+    runs-on: ubuntu-latest
+    timeout-minutes: 15
+    permissions:
+      contents: read
+    env:
+      CI: "true"
+      MISE_EXPERIMENTAL: "1"
+    steps:
+      - name: Checkout
+        uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
+        with:
+          persist-credentials: false
+
+      - name: Install mise
+        uses: jdx/mise-action@dba19683ed58901619b14f395a24841710cb4925 # v4.1.0
+        with:
+          cache: true
+
+      - name: Setup Node.js
+        uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
+        with:
+          node-version: 22.x
+
+      - name: Install dependencies
+        run: mise run install
+
+      - name: TS dead-code ratchet (knip, advisory)
+        continue-on-error: true
+        run: |
+          if mise run check:deadcode-ratchet; then
+            echo "::notice title=knip ratchet::Dead-code count is at or below baseline."
+          else
+            echo "::warning title=knip ratchet::Dead-code count increased above the baseline (knip-baseline.json). Run 'yarn knip' locally and remove the new dead code, or suppress a false positive in knip.json."
+          fi
+
+      - name: Python dead-code scan (vulture, advisory)
+        continue-on-error: true
+        run: |
+          if mise //agent:lint:deadcode; then
+            echo "::notice title=vulture::No Python dead code above the confidence threshold."
+          else
+            echo "::warning title=vulture::vulture found unused Python code. Remove it, or add an intentional keep to agent/.vulture_allowlist.py."
+          fi
diff --git a/agent/.vulture_allowlist.py b/agent/.vulture_allowlist.py
@@ -0,0 +1,23 @@
+# Vulture allowlist (issue #282, cairn MVG gate #6).
+#
+# Names here are reported by vulture as unused but are intentionally kept.
+# vulture "uses" every attribute access in this file, so listing a name
+# suppresses its finding. Only entries that fire at `--min-confidence 80`
+# (the CI threshold) belong here — lower-confidence noise (Pydantic
+# `model_config`, FastAPI route handlers, dynamically-dispatched methods)
+# is below the threshold and must NOT be blanket-allowlisted, so that real
+# dead code in those forms still surfaces.
+#
+# Regenerate/extend (then hand-review the diff):
+#   cd agent && uvx vulture src --make-whitelist
+#
+# Keep this list MINIMAL. Prefer deleting dead code over allowlisting it.
+
+# claude-agent-sdk invokes hooks positionally as
+# (hook_input, tool_use_id, hook_context). The dispatcher in hooks.py passes
+# its own `ctx`, so the `hook_context` positional is part of the required
+# signature but unread in these three hook bodies. Cannot be removed without
+# breaking the SDK calling convention.
+hook_context  # unused variable (src/hooks.py:132)
+hook_context  # unused variable (src/hooks.py:918)
+hook_context  # unused variable (src/hooks.py:1258)
diff --git a/agent/mise.toml b/agent/mise.toml
@@ -44,6 +44,10 @@ run = "uvx ruff format --check"
 description = "Type check with ty"
 run = "uv run ty check"
 
+[tasks."lint:deadcode"]
+description = "Dead-code detection with vulture (#282, cairn MVG gate #6): unused functions/classes/methods ruff F cannot see. Config + allowlist in pyproject.toml / .vulture_allowlist.py."
+run = "uv run vulture"
+
 [tasks.test]
 description = "Run tests with pytest (coverage floor enforced; use `uv run pytest --no-cov` for focused runs)"
 run = "uv run pytest --cov=src --cov-branch --cov-fail-under=72 --cov-report=term-missing:skip-covered --cov-report=lcov:coverage/lcov.info --cov-report=json:coverage/coverage.json"
diff --git a/agent/pyproject.toml b/agent/pyproject.toml
@@ -55,6 +55,14 @@ constraint-dependencies = [
     "pydantic-settings>=2.14.2",  # GHSA-4xgf-cpjx-pc3j — transitive via mcp; remove when mcp bumps floor
 ]
 
+# Dead-code detection (#282, cairn MVG gate #6). ruff "F" catches unused
+# imports/locals; vulture catches unused module-level functions/classes/methods
+# that ruff cannot see. min_confidence 80 keeps the signal high; intentional
+# keeps (e.g. SDK-mandated hook params) live in .vulture_allowlist.py.
+[tool.vulture]
+paths = ["src", ".vulture_allowlist.py"]
+min_confidence = 80
+
 [tool.bandit]
 exclude_dirs = ["tests", ".venv"]
 skips = [
@@ -73,11 +81,16 @@ dev = [
     "pytest",
     "pygments==2.20.0",
     "pytest-cov==7.1.0",
+    "vulture==2.16",  # dead-code detection (#282): unused functions/classes ruff F can't see
 ]
 
 [tool.ruff]
 target-version = "py313"
 line-length = 100
+# The vulture allowlist is bare-name expressions by design (that is how vulture
+# "uses" a name to suppress it). ruff would flag every line F821/B018, so it is
+# not Python source ruff should lint. See .vulture_allowlist.py (#282).
+extend-exclude = [".vulture_allowlist.py"]
 
 [tool.ruff.lint]
 select = [
@@ -124,3 +137,9 @@ skip_covered = true
 [tool.ty.environment]
 python-version = "3.13"
 extra-paths = ["src"]
+
+[tool.ty.src]
+# The vulture allowlist is bare-name expressions by design, not type-checkable
+# Python — ty flags each line unresolved-reference. Excluded here for the same
+# reason ruff excludes it via extend-exclude (#282).
+exclude = [".vulture_allowlist.py"]
diff --git a/agent/uv.lock b/agent/uv.lock
diff --git a/cdk/eslint.config.mjs b/cdk/eslint.config.mjs
@@ -156,6 +156,19 @@ export default [
       }],
 
       // TypeScript rules
+      // Dead-code parity with Python ruff F401 (#282): unused imports/locals/params
+      // are errors. `^_` opts out an intentionally-unused binding. Base rule must be
+      // off so the typescript-eslint variant (type-aware) is the only one active.
+      'no-unused-vars': 'off',
+      '@typescript-eslint/no-unused-vars': ['error', {
+        args: 'all',
+        argsIgnorePattern: '^_',
+        varsIgnorePattern: '^_',
+        caughtErrors: 'all',
+        caughtErrorsIgnorePattern: '^_',
+        ignoreRestSiblings: true,
+        destructuredArrayIgnorePattern: '^_',
+      }],
       // AI007 guard (#258): inline numeric literals should be named constants.
       // Values shared across Python/TypeScript belong in contracts/constants.json
       // (see contracts/constants.md). Baseline cleaned in #312; blocking like cli/.
diff --git a/cdk/src/bootstrap/index.ts b/cdk/src/bootstrap/index.ts
diff --git a/cdk/src/handlers/confirm-uploads.ts b/cdk/src/handlers/confirm-uploads.ts
@@ -294,9 +294,9 @@ export async function handler(event: APIGatewayProxyEvent, context: Context): Pr
 
 async function screenSingleAttachment(
   att: AttachmentRecord,
-  task: TaskRecord,
+  _task: TaskRecord,
   screeningConfig: ScreeningConfig,
-  taskId: string,
+  _taskId: string,
   meta: S3ObjectMeta,
 ): Promise<AttachmentRecord> {
   const { s3Key, versionId, sizeBytes } = meta;
@@ -530,7 +530,7 @@ async function transitionToSubmitted(
 // ---------------------------------------------------------------------------
 
 async function failTaskOnScreening(
-  task: TaskRecord,
+  _task: TaskRecord,
   taskId: string,
   filename: string,
   reason: string,
diff --git a/cdk/src/handlers/shared/create-task-core.ts b/cdk/src/handlers/shared/create-task-core.ts
@@ -53,7 +53,7 @@ import {
   type TaskRecord,
   toTaskDetail,
 } from './types';
-import { computeTtlEpoch, DEFAULT_MAX_TURNS, hasTaskSpec, isValidIdempotencyKey, isValidRepo, isValidTaskDescriptionLength, MAX_ATTACHMENT_SIZE_BYTES, MAX_TASK_DESCRIPTION_LENGTH, validateAttachments, validateMaxBudgetUsd, validateMaxTurns, validatePrNumber } from './validation';
+import { computeTtlEpoch, hasTaskSpec, isValidIdempotencyKey, isValidRepo, isValidTaskDescriptionLength, MAX_ATTACHMENT_SIZE_BYTES, MAX_TASK_DESCRIPTION_LENGTH, validateAttachments, validateMaxBudgetUsd, validateMaxTurns, validatePrNumber } from './validation';
 import { disallowedWorkflowModel, getWorkflowDescriptor, isValidWorkflowRef, resolveWorkflowRef, resolveWorkflowRefError } from './workflows';
 import { ATTACHMENT_OBJECT_KEY_PREFIX } from '../../constructs/attachments-bucket';
 import { TaskStatus } from '../../constructs/task-status';
@@ -522,7 +522,6 @@ export async function createTaskCore(
       if (att.delivery !== 'presigned') continue;
       const presignedAtt = att as PresignedAttachment;
       const attachmentId = ulid();
-      const s3Key = `${ATTACHMENT_OBJECT_KEY_PREFIX}${context.userId}/${taskId}/${attachmentId}/${presignedAtt.filename}`;
 
       attachmentRecords.push(createAttachmentRecord({
         attachment_id: attachmentId,
diff --git a/cdk/src/handlers/shared/linear-verify.ts b/cdk/src/handlers/shared/linear-verify.ts
@@ -28,9 +28,6 @@ import { logger } from './logger';
 const sm = new SecretsManagerClient({});
 const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({}));
 
-/** Prefix for Linear-related secrets in Secrets Manager. */
-export const LINEAR_SECRET_PREFIX = 'bgagent/linear/';
-
 // In-memory secret cache with 5-minute TTL (same pattern as slack-verify.ts).
 const secretCache = new Map<string, { secret: string; expiresAt: number }>();
 const CACHE_TTL_MINUTES = 5;
diff --git a/cdk/src/handlers/shared/validation.ts b/cdk/src/handlers/shared/validation.ts
@@ -286,7 +286,15 @@ export const MAX_TOTAL_ATTACHMENT_SIZE_BYTES = MAX_TOTAL_ATTACHMENT_SIZE_MB * 10
 /** Compile-time exhaustiveness check for AttachmentType. */
 const ATTACHMENT_TYPE_LIST = ['image', 'file', 'url'] as const satisfies readonly AttachmentType[];
 type _AssertAttachmentExhaustive = Exclude<AttachmentType, (typeof ATTACHMENT_TYPE_LIST)[number]> extends never ? true : never;
+// The ASSIGNMENT is the guard: if AttachmentType gains a member missing from
+// ATTACHMENT_TYPE_LIST, _AssertAttachmentExhaustive resolves to `never` and
+// assigning `true` is a hard compile error. (A `true as never` assertion would
+// NOT error — assertions are permitted whenever either side is assignable — so
+// it must stay an assignment.) `void` consumes the binding to satisfy
+// tsconfig noUnusedLocals, which — unlike @typescript-eslint/no-unused-vars,
+// already exempt via its ^_ varsIgnorePattern — does not honor the _ prefix.
 const _attachmentExhaustiveCheck: _AssertAttachmentExhaustive = true;
+void _attachmentExhaustiveCheck;
 const VALID_ATTACHMENT_TYPES = new Set<string>(ATTACHMENT_TYPE_LIST);
 
 /** Allowed image MIME types (PNG and JPEG only — passed directly to Bedrock). */
@@ -411,7 +419,7 @@ export function isValidFilename(filename: string): boolean {
 }
 
 /** Generate a default filename when none was provided. */
-function generateFilename(type: string, contentType: string, index: number): string {
+function generateFilename(_type: string, contentType: string, index: number): string {
   const ext = MIME_TO_EXTENSION[contentType] ?? 'bin';
   return `attachment_${index}.${ext}`;
 }
diff --git a/cdk/test/handlers/confirm-uploads.test.ts b/cdk/test/handlers/confirm-uploads.test.ts
@@ -234,9 +234,7 @@ describe('confirm-uploads handler', () => {
     });
 
     // Use mockImplementation for S3 calls to handle interleaving
-    let s3CallCount = 0;
     s3Send.mockImplementation((cmd: any) => {
-      s3CallCount++;
       if (cmd._type === 'S3Head') {
         // HeadObject — return valid metadata
         const isAtt1 = cmd.input.Key?.includes('att-1');
diff --git a/cdk/test/handlers/shared/context-hydration.test.ts b/cdk/test/handlers/shared/context-hydration.test.ts
@@ -54,7 +54,6 @@ import {
   hydrateContext,
   resolveGitHubToken,
   screenWithGuardrail,
-  type ContentTrustLevel,
   type GitHubIssueContext,
   type GuardrailScreeningResult,
   type IssueComment,
diff --git a/cdk/test/handlers/shared/resolve-url-attachments.test.ts b/cdk/test/handlers/shared/resolve-url-attachments.test.ts
@@ -166,7 +166,7 @@ jest.mock('../../../src/handlers/shared/logger', () => ({
 describe('resolveUrlAttachments', () => {
   const dns = jest.requireMock('dns').promises;
   const { screenImage } = jest.requireMock('../../../src/handlers/shared/attachment-screening');
-  const { isAllowedMimeType, validateMagicBytes } = jest.requireMock('../../../src/handlers/shared/validation');
+  const { isAllowedMimeType } = jest.requireMock('../../../src/handlers/shared/validation');
 
   const PNG_MAGIC = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]);
 
diff --git a/cdk/test/handlers/slack-command-processor.test.ts b/cdk/test/handlers/slack-command-processor.test.ts
@@ -121,7 +121,7 @@ describe('slack-command-processor handler', () => {
   test('slash submit tells user to use @mention', async () => {
     await handler(slashCommand({ text: 'submit org/repo fix' }));
     const posted = fetchMock.mock.calls.find(
-      ([url, opts]) => String((opts as { body: string }).body).includes('Use `@Shoof` to submit tasks'),
+      ([_url, opts]) => String((opts as { body: string }).body).includes('Use `@Shoof` to submit tasks'),
     );
     expect(posted).toBeTruthy();
     expect(createTaskCoreMock).not.toHaveBeenCalled();
@@ -215,15 +215,15 @@ describe('slack-command-processor handler', () => {
     expect(putCall![0].input.Item.slack_identity).toMatch(/^pending#/);
     expect(putCall![0].input.Item.status).toBe('pending');
     const posted = fetchMock.mock.calls.find(
-      ([url, opts]) => String((opts as { body: string }).body).includes('bgagent slack link'),
+      ([_url, opts]) => String((opts as { body: string }).body).includes('bgagent slack link'),
     );
     expect(posted).toBeTruthy();
   });
 
   test('help subcommand replies with usage text', async () => {
     await handler(slashCommand({ text: 'help' }));
     const posted = fetchMock.mock.calls.find(
-      ([url, opts]) => String((opts as { body: string }).body).includes('Using Shoof'),
+      ([_url, opts]) => String((opts as { body: string }).body).includes('Using Shoof'),
     );
     expect(posted).toBeTruthy();
   });
diff --git a/cdk/tsconfig.json b/cdk/tsconfig.json
@@ -18,8 +18,8 @@
     "noImplicitAny": true,
     "noImplicitReturns": true,
     "noImplicitThis": true,
-    "noUnusedLocals": false,
-    "noUnusedParameters": false,
+    "noUnusedLocals": true,
+    "noUnusedParameters": true,
     "resolveJsonModule": true,
     "strict": true,
     "strictNullChecks": true,
diff --git a/cli/eslint.config.mjs b/cli/eslint.config.mjs
@@ -153,6 +153,19 @@ export default [
       }],
 
       // TypeScript rules
+      // Dead-code parity with Python ruff F401 (#282): unused imports/locals/params
+      // are errors. `^_` opts out an intentionally-unused binding. Base rule must be
+      // off so the typescript-eslint variant (type-aware) is the only one active.
+      'no-unused-vars': 'off',
+      '@typescript-eslint/no-unused-vars': ['error', {
+        args: 'all',
+        argsIgnorePattern: '^_',
+        varsIgnorePattern: '^_',
+        caughtErrors: 'all',
+        caughtErrorsIgnorePattern: '^_',
+        ignoreRestSiblings: true,
+        destructuredArrayIgnorePattern: '^_',
+      }],
       // AI007 guard (#258): inline numeric literals should be named constants.
       // Values shared across Python/TypeScript belong in contracts/constants.json
       // (see contracts/constants.md).
diff --git a/cli/src/linear-oauth.ts b/cli/src/linear-oauth.ts
@@ -253,7 +253,7 @@ async function parseTokenResponse(
   let body: unknown;
   try {
     body = await response.json();
-  } catch (err) {
+  } catch (_err) {
     throw new CliError(
       `Linear /oauth/token returned non-JSON during ${contextLabel}: HTTP ${response.status}`,
     );
diff --git a/cli/test/commands/github-token.test.ts b/cli/test/commands/github-token.test.ts
@@ -17,7 +17,7 @@
  *  SOFTWARE.
  */
 
-import { GetSecretValueCommand, PutSecretValueCommand } from '@aws-sdk/client-secrets-manager';
+import { PutSecretValueCommand } from '@aws-sdk/client-secrets-manager';
 import { GetCommand } from '@aws-sdk/lib-dynamodb';
 import { CliError } from '../../src/errors';
 import {
diff --git a/knip-baseline.json b/knip-baseline.json
@@ -0,0 +1,4 @@
+{
+  "count": 78,
+  "comment": "Dead-code ratchet baseline for issue #282 (cairn MVG gate #6). This is the knip issue count at the time the gate was introduced — pre-existing unused exports/types that are out of scope to remove in the gate PR. The ratchet (scripts/check-deadcode-ratchet.mjs) fails the build only if the count rises above this number. When dead code is removed and the count drops, lower this value in the same PR to lock in the gain. Per-category false positives belong in knip.json, not here."
+}
diff --git a/knip.json b/knip.json
diff --git a/mise.toml b/mise.toml
diff --git a/package.json b/package.json
diff --git a/scripts/check-deadcode-ratchet.mjs b/scripts/check-deadcode-ratchet.mjs
diff --git a/yarn.lock b/yarn.lock
