From 3c068290ed9d4e12560876befa1b8ac6a1650cdb Mon Sep 17 00:00:00 2001 From: Cristian Greco Date: Mon, 12 Oct 2026 17:08:41 +0100 Subject: [PATCH 1/5] Add a write-code skill for code conventions Writes down the conventions that Biome, knip and tsc don't check: class and helper layout, function size, waits and startup timeouts in modules, and reusing core. AGENTS.md, review-pr and add-module point to it. --- .agents/skills/add-module/SKILL.md | 1 + .agents/skills/review-pr/SKILL.md | 1 + .agents/skills/write-code/SKILL.md | 46 ++++++++++++++++++++++++++++++ .claude/skills/write-code | 1 + AGENTS.md | 3 +- 5 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 .agents/skills/write-code/SKILL.md create mode 120000 .claude/skills/write-code diff --git a/.agents/skills/add-module/SKILL.md b/.agents/skills/add-module/SKILL.md index 3322878bf..58b5d9765 100644 --- a/.agents/skills/add-module/SKILL.md +++ b/.agents/skills/add-module/SKILL.md @@ -25,6 +25,7 @@ For the container class, tests and docs page, start from the module closest in s - Started-container getters call `getMappedPort()` when they're invoked. A restarted container can get different host ports. - Incompatible major versions (different ports, auth or startup) get separate classes, not image-tag parsing. - Keep it small: no getters that exist only for tests, and no speculative options. +- Follow `write-code` for class layout, setup and waits after start, and what to reuse from core. ## Tests diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index 578e0a7e1..851c26d17 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -40,6 +40,7 @@ gh pr checks - Zero-config defaults work, and invalid or half-set config fails fast. - The wait strategy is set in the constructor, and waits are robust (listening ports, health check or HTTP rather than log regexes). - Images are pinned in the module `Dockerfile`. +- **Style:** new code follows `write-code`. Helpers sit below their callers, long functions are split into named steps, waits added after start take the user's startup timeout, and nothing is hand-rolled that core already provides. - **Scope:** - Flag new files for a few lines of logic, dead fallbacks, duplicated constants, getters that exist only for tests, and tests of third-party behaviour. - Flag unrelated dependency bumps; leave those to Dependabot. diff --git a/.agents/skills/write-code/SKILL.md b/.agents/skills/write-code/SKILL.md new file mode 100644 index 000000000..9cfc9a747 --- /dev/null +++ b/.agents/skills/write-code/SKILL.md @@ -0,0 +1,46 @@ +--- +name: write-code +description: Writes and refactors TypeScript in testcontainers-node following the code conventions that Biome, knip and tsc don't check (class and helper layout, function size, waits and startup timeouts in modules, reusing core instead of hand-rolling). Use when writing, changing or refactoring code under packages/, e.g. "add an option to the Kafka module", "fix this bug", "split this function up", and when review feedback says code is too long, duplicated or in the wrong place. +--- + +# Write code + +Biome formats and lints, knip removes unused exports, and `tsc` requires `override`. AGENTS.md covers comments, root-cause fixes and import boundaries. The rules below are what none of those catch, and what reviewers keep flagging. + +Some files predate a rule. Follow the rule in new code and say that the file is inconsistent, rather than copying it. + +## Layout + +- Order a container class as fields, constructor, `with*` methods, `start()`, then private helpers. Its `Started*Container` class follows in the same file. +- Define a helper below the first function that calls it, so a file reads top-down. In `kafka-container.ts`, `containerStarted` comes before `updateAdvertisedListeners` and `createUser`. +- Mark public methods `public` and give them a return type. `with*` methods return `this` typed as `: this`. Naming the class there breaks chaining in a subclass. +- Name ports and paths as constants at the top of the file (`const KAFKA_PORT = 9093`). + +## Functions + +- Keep functions short. Nearly all of them here are under 30 lines. +- When a function does several things in sequence, move each one into a named private helper so the function reads as a list of steps. `KafkaContainer.containerStarted` builds its start script from `commandKraft()`, `commandZookeeper()` and `commandKraftCreateUser()`. +- A block written a second time in the same file becomes a helper. + +## Waits and timeouts in modules + +- Readiness goes in the wait strategy set in the constructor, combining conditions with `Wait.forAll([...])`. Users can override it there, and `withStartupTimeout` applies to it. +- Setup that needs a running container (`exec`, copying config, HTTP calls) goes in `containerStarted`, as in `RedisContainer`. +- Wait inside `containerStarted` only when setup has to run before the container can become ready. Call `waitForContainer` as `RedpandaContainer` does, not `waitUntilReady` on a strategy. It also stops and removes the container when the wait fails. +- Give every such wait the user's timeout with `withStartupTimeout(this.startupTimeoutMs)` when it is set. Core applies it only to the constructor's wait strategy, so a later wait otherwise runs on its own default. +- Express a condition as a `WaitStrategy` (`Wait.forHttp(...).forResponsePredicate(...)`, `Wait.forSuccessfulCommand(...)`) instead of a polling loop. +- Check every `exec`: throw on a non-zero `exitCode` with `output` in the message. + +## Reuse + +- Before writing a helper, check what `testcontainers` exports in `packages/testcontainers/src/index.ts` and what a started container offers (`exec`, `copyContentToContainer`, `getMappedPort`). +- Configure the container through `with*` methods such as `withEnvironment` and `withCommand`. Don't assign to `this.createOpts` or `this.hostConfig`. +- Compare image versions with `compare-versions`, as `KafkaContainer` does with `satisfies(this.imageName.tag, ">=8.0.0")`. +- Log with `log` from `testcontainers`, not `console`. +- Modules can't import each other, so short setters such as `withUsername` repeat across modules. Leave those. If two modules need the same non-trivial block, raise moving it into core. + +## Details + +- Default with `??`, not `||`. +- An option that may be unset is `undefined` (`private password?: string`), not `""`. +- Test titles start with `should`. diff --git a/.claude/skills/write-code b/.claude/skills/write-code new file mode 120000 index 000000000..ce96cc1bb --- /dev/null +++ b/.claude/skills/write-code @@ -0,0 +1 @@ +../../.agents/skills/write-code \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 32afa6fdf..886747603 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,7 +5,7 @@ This is a working guide for contributors and coding agents in this repository. It captures practical rules that prevent avoidable CI and PR churn. -This file holds the rules for every task. Workflows live in skills under `.agents/skills/`, which Claude Code reads through symlinks in `.claude/skills/`. They cover opening and reviewing PRs, adding modules, issue triage, CI and dependency maintenance, and release review. +This file holds the rules for every task. Workflows live in skills under `.agents/skills/`, which Claude Code reads through symlinks in `.claude/skills/`. They cover writing code, opening and reviewing PRs, adding modules, issue triage, CI and dependency maintenance, and release review. If a skill or this file turns out to be wrong or incomplete, update it in the same PR. ## Repository Layout @@ -30,6 +30,7 @@ If a skill or this file turns out to be wrong or incomplete, update it in the sa ### Code +- Follow the `write-code` skill when writing or changing code under `packages/`: where helpers go, function size, waits and timeouts in modules, and reusing core. - Keep code comments to a minimum; the codebase has very few. Add one only when it tells the reader something they can't get from the code, the PR or the git history, such as why a non-obvious approach was needed, or a link to an upstream bug. - Don't narrate what the code does, describe the change you made, or reference the task or review that prompted it. - Fix the root cause. A change that only makes a failure less likely is not a fix. From 514469d15d1cc6520da1057ee51abe4d33c3a026 Mon Sep 17 00:00:00 2001 From: Cristian Greco Date: Mon, 12 Oct 2026 19:17:41 +0100 Subject: [PATCH 2/5] Remove module-specific examples and trim the write-code pointers The pointers in AGENTS.md, review-pr and add-module now only name the skill. The skill defines what a helper is and covers duplication within a file, across a package and across modules. --- .agents/skills/add-module/SKILL.md | 2 +- .agents/skills/review-pr/SKILL.md | 2 +- .agents/skills/write-code/SKILL.md | 19 ++++++++++--------- AGENTS.md | 2 +- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/.agents/skills/add-module/SKILL.md b/.agents/skills/add-module/SKILL.md index 58b5d9765..7ab315083 100644 --- a/.agents/skills/add-module/SKILL.md +++ b/.agents/skills/add-module/SKILL.md @@ -25,7 +25,7 @@ For the container class, tests and docs page, start from the module closest in s - Started-container getters call `getMappedPort()` when they're invoked. A restarted container can get different host ports. - Incompatible major versions (different ports, auth or startup) get separate classes, not image-tag parsing. - Keep it small: no getters that exist only for tests, and no speculative options. -- Follow `write-code` for class layout, setup and waits after start, and what to reuse from core. +- Write the class following `write-code`. ## Tests diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index 851c26d17..81069152b 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -40,7 +40,7 @@ gh pr checks - Zero-config defaults work, and invalid or half-set config fails fast. - The wait strategy is set in the constructor, and waits are robust (listening ports, health check or HTTP rather than log regexes). - Images are pinned in the module `Dockerfile`. -- **Style:** new code follows `write-code`. Helpers sit below their callers, long functions are split into named steps, waits added after start take the user's startup timeout, and nothing is hand-rolled that core already provides. +- **Style:** new code follows `write-code`. - **Scope:** - Flag new files for a few lines of logic, dead fallbacks, duplicated constants, getters that exist only for tests, and tests of third-party behaviour. - Flag unrelated dependency bumps; leave those to Dependabot. diff --git a/.agents/skills/write-code/SKILL.md b/.agents/skills/write-code/SKILL.md index 9cfc9a747..ef8685389 100644 --- a/.agents/skills/write-code/SKILL.md +++ b/.agents/skills/write-code/SKILL.md @@ -12,21 +12,23 @@ Some files predate a rule. Follow the rule in new code and say that the file is ## Layout - Order a container class as fields, constructor, `with*` methods, `start()`, then private helpers. Its `Started*Container` class follows in the same file. -- Define a helper below the first function that calls it, so a file reads top-down. In `kafka-container.ts`, `containerStarted` comes before `updateAdvertisedListeners` and `createUser`. +- A helper is a private method, or a function the file doesn't export. Define it below the first function that calls it, so a file reads top-down. - Mark public methods `public` and give them a return type. `with*` methods return `this` typed as `: this`. Naming the class there breaks chaining in a subclass. -- Name ports and paths as constants at the top of the file (`const KAFKA_PORT = 9093`). +- Name ports and paths as constants at the top of the file. ## Functions -- Keep functions short. Nearly all of them here are under 30 lines. -- When a function does several things in sequence, move each one into a named private helper so the function reads as a list of steps. `KafkaContainer.containerStarted` builds its start script from `commandKraft()`, `commandZookeeper()` and `commandKraftCreateUser()`. -- A block written a second time in the same file becomes a helper. +- Keep functions short. When one does several things in sequence, move each into a named helper so the function reads as a list of steps. +- Don't write the same code twice: + - In one file, extract a helper. + - Across files in a package, move it into a function in its own file. + - Across modules, it belongs in core, exported from `testcontainers`, because modules can't import each other. That adds public API, so propose it instead of copying the code. ## Waits and timeouts in modules - Readiness goes in the wait strategy set in the constructor, combining conditions with `Wait.forAll([...])`. Users can override it there, and `withStartupTimeout` applies to it. -- Setup that needs a running container (`exec`, copying config, HTTP calls) goes in `containerStarted`, as in `RedisContainer`. -- Wait inside `containerStarted` only when setup has to run before the container can become ready. Call `waitForContainer` as `RedpandaContainer` does, not `waitUntilReady` on a strategy. It also stops and removes the container when the wait fails. +- Setup that needs a running container (`exec`, copying config, HTTP calls) goes in `containerStarted`. +- Wait inside `containerStarted` only when setup has to run before the container can become ready. Call `waitForContainer`, not `waitUntilReady` on a strategy. It also stops and removes the container when the wait fails. - Give every such wait the user's timeout with `withStartupTimeout(this.startupTimeoutMs)` when it is set. Core applies it only to the constructor's wait strategy, so a later wait otherwise runs on its own default. - Express a condition as a `WaitStrategy` (`Wait.forHttp(...).forResponsePredicate(...)`, `Wait.forSuccessfulCommand(...)`) instead of a polling loop. - Check every `exec`: throw on a non-zero `exitCode` with `output` in the message. @@ -35,9 +37,8 @@ Some files predate a rule. Follow the rule in new code and say that the file is - Before writing a helper, check what `testcontainers` exports in `packages/testcontainers/src/index.ts` and what a started container offers (`exec`, `copyContentToContainer`, `getMappedPort`). - Configure the container through `with*` methods such as `withEnvironment` and `withCommand`. Don't assign to `this.createOpts` or `this.hostConfig`. -- Compare image versions with `compare-versions`, as `KafkaContainer` does with `satisfies(this.imageName.tag, ">=8.0.0")`. +- Compare image versions with `compare-versions` (`satisfies(this.imageName.tag, ">=8.0.0")`). - Log with `log` from `testcontainers`, not `console`. -- Modules can't import each other, so short setters such as `withUsername` repeat across modules. Leave those. If two modules need the same non-trivial block, raise moving it into core. ## Details diff --git a/AGENTS.md b/AGENTS.md index 886747603..0ec65673c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ If a skill or this file turns out to be wrong or incomplete, update it in the sa ### Code -- Follow the `write-code` skill when writing or changing code under `packages/`: where helpers go, function size, waits and timeouts in modules, and reusing core. +- Follow the `write-code` skill when writing or changing code under `packages/`. - Keep code comments to a minimum; the codebase has very few. Add one only when it tells the reader something they can't get from the code, the PR or the git history, such as why a non-obvious approach was needed, or a link to an upstream bug. - Don't narrate what the code does, describe the change you made, or reference the task or review that prompted it. - Fix the root cause. A change that only makes a failure less likely is not a fix. From e0f4ba375db653a6e72af50727ccdc206cffac18 Mon Sep 17 00:00:00 2001 From: Cristian Greco Date: Mon, 12 Oct 2026 19:21:43 +0100 Subject: [PATCH 3/5] Have post-start waits share the remaining startup timeout Follows #1514, where Couchbase gives each wait in containerStarted what is left of the startup timeout. --- .agents/skills/write-code/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/write-code/SKILL.md b/.agents/skills/write-code/SKILL.md index ef8685389..3d84e8ffe 100644 --- a/.agents/skills/write-code/SKILL.md +++ b/.agents/skills/write-code/SKILL.md @@ -29,7 +29,7 @@ Some files predate a rule. Follow the rule in new code and say that the file is - Readiness goes in the wait strategy set in the constructor, combining conditions with `Wait.forAll([...])`. Users can override it there, and `withStartupTimeout` applies to it. - Setup that needs a running container (`exec`, copying config, HTTP calls) goes in `containerStarted`. - Wait inside `containerStarted` only when setup has to run before the container can become ready. Call `waitForContainer`, not `waitUntilReady` on a strategy. It also stops and removes the container when the wait fails. -- Give every such wait the user's timeout with `withStartupTimeout(this.startupTimeoutMs)` when it is set. Core applies it only to the constructor's wait strategy, so a later wait otherwise runs on its own default. +- Waits added after start share the user's startup timeout. Give each one what is left of it with `withStartupTimeout(...)`, never a fresh timeout or its own default. Core applies the timeout only to the constructor's wait strategy. - Express a condition as a `WaitStrategy` (`Wait.forHttp(...).forResponsePredicate(...)`, `Wait.forSuccessfulCommand(...)`) instead of a polling loop. - Check every `exec`: throw on a non-zero `exitCode` with `output` in the message. From 65d74387636ba6aed779b45452aeacb618d4eaf0 Mon Sep 17 00:00:00 2001 From: Cristian Greco Date: Mon, 12 Oct 2026 19:39:37 +0100 Subject: [PATCH 4/5] Stop listing what AGENTS.md and the tooling cover in the write-code intro --- .agents/skills/write-code/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/write-code/SKILL.md b/.agents/skills/write-code/SKILL.md index 3d84e8ffe..65706cfba 100644 --- a/.agents/skills/write-code/SKILL.md +++ b/.agents/skills/write-code/SKILL.md @@ -5,7 +5,7 @@ description: Writes and refactors TypeScript in testcontainers-node following th # Write code -Biome formats and lints, knip removes unused exports, and `tsc` requires `override`. AGENTS.md covers comments, root-cause fixes and import boundaries. The rules below are what none of those catch, and what reviewers keep flagging. +Biome, knip and `tsc` already check formatting, lint rules, unused exports and `override`, and AGENTS.md holds the rules for every task. The rules below are what none of those cover, and what reviewers keep flagging. Some files predate a rule. Follow the rule in new code and say that the file is inconsistent, rather than copying it. From a139a8b55058063e1c2d7c06d1d658692faf7f63 Mon Sep 17 00:00:00 2001 From: Cristian Greco Date: Tue, 13 Oct 2026 10:23:17 +0100 Subject: [PATCH 5/5] Cut the write-code skill down to reuse and refactoring Lint-style conventions are left to Biome. The skill keeps what a linter can't see: look for existing code first, refactor instead of copying, and use core's building blocks. --- .agents/skills/write-code/SKILL.md | 48 ++++++++---------------------- 1 file changed, 13 insertions(+), 35 deletions(-) diff --git a/.agents/skills/write-code/SKILL.md b/.agents/skills/write-code/SKILL.md index 65706cfba..a5ee2dcb9 100644 --- a/.agents/skills/write-code/SKILL.md +++ b/.agents/skills/write-code/SKILL.md @@ -1,47 +1,25 @@ --- name: write-code -description: Writes and refactors TypeScript in testcontainers-node following the code conventions that Biome, knip and tsc don't check (class and helper layout, function size, waits and startup timeouts in modules, reusing core instead of hand-rolling). Use when writing, changing or refactoring code under packages/, e.g. "add an option to the Kafka module", "fix this bug", "split this function up", and when review feedback says code is too long, duplicated or in the wrong place. +description: Writes and refactors TypeScript in testcontainers-node by reusing what the project already has (existing helpers, shared functions, core's building blocks) instead of adding another copy. Use when writing, changing or refactoring code under packages/, e.g. "add an option to the Kafka module", "fix this bug", "add a wait for X", and when review feedback says code is duplicated or should be refactored. --- # Write code -Biome, knip and `tsc` already check formatting, lint rules, unused exports and `override`, and AGENTS.md holds the rules for every task. The rules below are what none of those cover, and what reviewers keep flagging. +Biome enforces the lint and formatting rules (`biome.json`). This skill covers what a linter can't see. The most common review finding is new code that repeats something the project already has. -Some files predate a rule. Follow the rule in new code and say that the file is inconsistent, rather than copying it. +## Before adding code -## Layout +- Search for code that already does it: in the file, in the package, and in what `testcontainers` exports (`packages/testcontainers/src/index.ts`). If it exists, use it. +- If something close exists, change it so it serves both callers. Don't copy it and adapt the copy. -- Order a container class as fields, constructor, `with*` methods, `start()`, then private helpers. Its `Started*Container` class follows in the same file. -- A helper is a private method, or a function the file doesn't export. Define it below the first function that calls it, so a file reads top-down. -- Mark public methods `public` and give them a return type. `with*` methods return `this` typed as `: this`. Naming the class there breaks chaining in a subclass. -- Name ports and paths as constants at the top of the file. +## When the same code is needed twice -## Functions +- In one file, extract a helper: a private method, or a function the file doesn't export. Define it below the first function that calls it. +- Across files in a package, move it into a function in its own file. +- Across modules, it belongs in core, exported from `testcontainers`, because modules can't import each other. That adds public API, so propose it instead of copying the code. -- Keep functions short. When one does several things in sequence, move each into a named helper so the function reads as a list of steps. -- Don't write the same code twice: - - In one file, extract a helper. - - Across files in a package, move it into a function in its own file. - - Across modules, it belongs in core, exported from `testcontainers`, because modules can't import each other. That adds public API, so propose it instead of copying the code. +## Use what core provides -## Waits and timeouts in modules - -- Readiness goes in the wait strategy set in the constructor, combining conditions with `Wait.forAll([...])`. Users can override it there, and `withStartupTimeout` applies to it. -- Setup that needs a running container (`exec`, copying config, HTTP calls) goes in `containerStarted`. -- Wait inside `containerStarted` only when setup has to run before the container can become ready. Call `waitForContainer`, not `waitUntilReady` on a strategy. It also stops and removes the container when the wait fails. -- Waits added after start share the user's startup timeout. Give each one what is left of it with `withStartupTimeout(...)`, never a fresh timeout or its own default. Core applies the timeout only to the constructor's wait strategy. -- Express a condition as a `WaitStrategy` (`Wait.forHttp(...).forResponsePredicate(...)`, `Wait.forSuccessfulCommand(...)`) instead of a polling loop. -- Check every `exec`: throw on a non-zero `exitCode` with `output` in the message. - -## Reuse - -- Before writing a helper, check what `testcontainers` exports in `packages/testcontainers/src/index.ts` and what a started container offers (`exec`, `copyContentToContainer`, `getMappedPort`). -- Configure the container through `with*` methods such as `withEnvironment` and `withCommand`. Don't assign to `this.createOpts` or `this.hostConfig`. -- Compare image versions with `compare-versions` (`satisfies(this.imageName.tag, ">=8.0.0")`). -- Log with `log` from `testcontainers`, not `console`. - -## Details - -- Default with `??`, not `||`. -- An option that may be unset is `undefined` (`private password?: string`), not `""`. -- Test titles start with `should`. +- Configure containers through the `with*` methods. Don't assign to `this.createOpts` or `this.hostConfig`. +- Wait with a `WaitStrategy` run through `waitForContainer`. Don't call `waitUntilReady` on a strategy or write a polling loop. +- A wait added after start gets what is left of the user's startup timeout, never a fresh one.