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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions .conventions/STYLEGUIDE-CODE.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,85 @@ import type { DurableObjectRPC } from "@taskless/shared/rpc";
import type { UserDO, GitHubOrganizationDO } from "@taskless/storage";
```

## Testing

### Verify Build Output In The Build, Not By Parsing It

**A failing build is still a valid test — of the build.** When an invariant is about a build artifact, enforce it where the artifact is produced. If a bundle must not contain something, the build should refuse to emit it, rather than emitting it and leaving a test to go looking afterwards. An invariant enforced at production time cannot be violated; one enforced afterwards can only be detected.

**DO NOT** reconstruct a fact about generated output by parsing that output.

```typescript
// ✅ Good - the build refuses to emit a violating artifact
// vite.config.ts
import type { Plugin } from "vite";

const ALLOWED = new Set<string>([
// allowed specifiers here
]);

function forbidHostCapabilities(): Plugin {
name: "forbid-host-capabilities",
generateBundle(_options, bundle) {
const chunk = bundle["prompts.js"];
if (chunk?.type !== "chunk") return;
// rollup already resolved the graph — ask it, don't re-derive it
for (const specifier of [...chunk.imports, ...chunk.dynamicImports]) {
if (!ALLOWED.has(specifier)) {
this.error(`prompts.js must not import ${specifier}`);
}
}
},
};
}

// ❌ Bad - a test regex-scans the built JavaScript to rebuild the import graph
const specifiers = [...source.matchAll(/\bfrom\s*["']([^"']+)["']/g)].map(
(m) => m[1]
);
for (const specifier of specifiers) {
expect(specifier, `dist/prompts.js graph imports ${specifier}`).toBe(
"allowed"
);
}
```

**Tests that _use_ a built artifact are fine.** Importing the built entry and asserting on its behavior, or spawning the built CLI and asserting on its output, are ordinary tests. The rule is not "tests must not touch build output" — it is that tests must not re-derive what the build already knew.

```typescript
// ✅ Fine - uses the artifact, asserts on behavior
const { getPrompt } = await import(pathToFileURL(builtEntry).href);
expect(getPrompt("engine-selection")).toBe(sourceRecipe);

// ✅ Fine - spawns the built CLI, asserts on its output
const { stdout } = await execFileAsync("node", [builtCli, "help"]);
expect(stdout).toContain("Usage:");
```

**Do not add a dependency in order to test an assertion.** If a test needs a parser to make sense of an artifact, that is the signal the check is in the wrong place — the generator already has the structured data. Reach for a new devDependency only when several tests need it and nothing in the existing toolchain can answer the question.

**Worked example.** `packages/cli/test/prompts.test.ts` asserted that the built `dist/prompts.js` chunk graph never reaches the CLI entry or a host capability, by regex-scanning the built JavaScript for `from "…"` to reconstruct the import graph. A built chunk embeds every help recipe as a string literal, and the `engine-selection` recipe contains the phrase `a different axis from "which engine"` — so the scan reported `dist/prompts.js graph imports which engine`. Prose was read as an import.
Comment thread
thecodedrift marked this conversation as resolved.

The fixes that did not work, and why:

| Attempt | Why it was rejected |
| ---------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Filter candidates by specifier shape (`/^(?:node:)?[@\w./-]+$/`) | Passed only because that phrase contains a space. Measured against the real bundle the regex yields `["which engine"]` and the filter drops it — but `differs from "static-tier"` is a bare hyphenated name with no whitespace and would have been reported. The guard held by luck of punctuation. |
| Add `es-module-lexer` as a devDependency | Parsed the graph correctly, but bought a dependency — and a second major version, since vite already pulls 1.7.0 transitively — to serve a single test. |
| Anchor the regex to line-start | Matched the lexer exactly on today's bundles, but required `from` on the same line as `import`. A future bundler that wrapped a long import would silently stop detecting real imports — trading a loud false positive for a quiet false negative in the guard whose entire job is catching a leak. |

The resolution: rollup's `OutputChunk` already exposes `imports` and `dynamicImports` — the exact resolved graph. The check moved into a vite plugin that fails the build, and the test was deleted.
Comment thread
thecodedrift marked this conversation as resolved.

The same reasoning forbids adding a YAML parser to assert on generated config, or an HTML parser to assert on rendered output. In each case the generator knows the answer and the test is guessing at it.

**Rationale:**

- An invariant enforced at production time cannot be violated; one enforced afterwards can only be detected
- Parsing generated text reconstructs information the generator already had, using a weaker tool
- A check that needs a parser is a check in the wrong place — move it to where the structured data lives
- A build that fails is a faster, earlier signal than a test that fails, and it cannot be skipped
- Regexes over generated output are brittle in the worst direction: they break on content that merely resembles code, and they quietly stop matching when the generator's formatting changes

## Code Quality Checks

**IMPORTANT:** After making code changes, you **MUST** run these checks before considering the task complete:
Expand Down
5 changes: 2 additions & 3 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

When creating or modifying files, you **MUST** follow these conventions:

- File Naming Conventions @.claude/FILE-CONVENTIONS.md
- Code Style Guide @.claude/STYLEGUIDE-CODE.md
- UI Conventions @.claude/STYLEGUIDE-UI.md
- Code Style Guide @.conventions/STYLEGUIDE-CODE.md
- UI Conventions @.conventions/STYLEGUIDE-UI.md
- When a user asks about what you can do, you _should_ suggest actions from this CLAUDE.md file.
- **NEVER** read a `.dev.vars` or `.env` or `.secrets` file

Expand Down
Loading