mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix: adapt agent docs, skills, and Coder Agents prompts for Claude Opus 5.5 (#29817)
This commit is contained in:
Michael Suchacz
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: coder-agents-review
|
||||
description: "Use this skill when a repository already has an open pull request and you need to run the Coder Agents Review loop: request review with `/coder-agents-review` when needed, wait for feedback from the `coder-agents-review` GitHub app, fix issues, and repeat until the app comments `approved`."
|
||||
description: "Use this skill when a repository already has an open pull request and you need to run the Coder Agents Review loop: request review with `/coder-agents-review` when needed, wait for feedback from the `coder-agents-review` GitHub app, fix issues, and repeat until the app approves (an `approved` comment or an `APPROVED` review)."
|
||||
---
|
||||
|
||||
# Coder Agents Review Loop
|
||||
@@ -18,7 +18,8 @@ The loop is:
|
||||
3. fix actionable issues with the smallest safe diff
|
||||
4. validate and push
|
||||
5. request another review with `/coder-agents-review`
|
||||
6. repeat until the app comments `approved`
|
||||
6. repeat until the app approves the current work (an `approved` comment
|
||||
or an `APPROVED` review)
|
||||
|
||||
## Definition of done
|
||||
|
||||
|
||||
@@ -114,7 +114,7 @@ Tier 2 file filters:
|
||||
- TypeScript: `*.ts` `*.tsx`: reference `.agents/skills/deep-review/references/typescript.md` before reviewing.
|
||||
- React: `*.tsx` `*.jsx`: reference `.agents/skills/deep-review/references/react.md` before reviewing.
|
||||
|
||||
`.tsx` files match both TypeScript and React filters. Spawn both instances when the diff contains `.tsx` changes — TS covers language-level patterns; React covers component and hooks patterns. Before spawning, verify each instance's filter produces a non-empty diff. Skip instances whose filtered diff is empty.
|
||||
`.tsx` files match both TypeScript and React filters. Spawn both instances when the diff contains `.tsx` changes: TS covers language-level patterns; React covers component and hooks patterns. Before spawning, list the changed paths (`gh pr diff {number} --name-only` for a PR) and skip instances whose filter matches none of them.
|
||||
|
||||
- **Style Reviewer**: `*.go` `*.ts` `*.tsx` `*.py` `*.sh`
|
||||
|
||||
@@ -260,7 +260,7 @@ Fresh review found one new issue: 1 P2 across 1 inline comment.
|
||||
|
||||
Keep the review body to 2–4 sentences. Don't use markdown headers in the body — they render oversized in GitHub's review UI.
|
||||
|
||||
**Inline comments.** Every finding is an inline comment, pinned to the most relevant file and line. For findings that span multiple files, pin to the primary file (GitHub supports file-level comments when `position` is omitted or set to 1).
|
||||
**Inline comments.** Every finding is an inline comment, pinned to the most relevant file and line. For findings that span multiple files, pin to the most relevant line in the primary file.
|
||||
|
||||
Inline comment format:
|
||||
|
||||
@@ -297,13 +297,9 @@ For P0 or P1 findings, add a note in the review body: "This review contains find
|
||||
|
||||
**Posting via GitHub API.**
|
||||
|
||||
The `gh api` endpoint for posting reviews routes through GraphQL by default. Field names differ from the REST API docs:
|
||||
Post through the REST endpoint below. Each comment uses `path` and `position`. The review `comments` array has no file-level option, so pin cross-file findings to a line.
|
||||
|
||||
- Use `position` (diff-relative line number), not `line` + `side`. `side` is not a valid field in the GraphQL schema.
|
||||
- `subject_type: "file"` is not recognized. Pin file-level comments to `position: 1` instead.
|
||||
- Use `-X POST` with `--input` to force REST API routing.
|
||||
|
||||
To compute positions: save the PR diff to a file, then count lines from the first `@@` hunk header of each file's diff section. For new files, position = line number + 1 (the hunk header is position 1, first content line is position 2).
|
||||
To compute positions: save the PR diff to a file. In each file's diff section, the line immediately after the first `@@` hunk header is position 1. Positions keep increasing through later hunks in the same file, and each later `@@` header counts as one line. For a new file, position equals the file line number.
|
||||
|
||||
```sh
|
||||
gh pr diff {number} > /tmp/pr.diff
|
||||
@@ -327,12 +323,12 @@ Where `review.json`:
|
||||
{
|
||||
"path": "file.go",
|
||||
"position": 42,
|
||||
"body": "**P1** Finding... *(Reviewer Role)*\n\n> Evidence..."
|
||||
"body": "**P2** Finding... *(Reviewer Role)*\n\n> Evidence..."
|
||||
},
|
||||
{
|
||||
"path": "other.go",
|
||||
"position": 1,
|
||||
"body": "**P2** Cross-file finding... *(Reviewer Role)*\n\n> Evidence..."
|
||||
"position": 17,
|
||||
"body": "**P3** Finding that spans files, pinned to the most relevant line in the primary file... *(Reviewer Role)*\n\n> Evidence..."
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
Get the diff for the review target specified in your prompt, filtered to the file scope specified, then review it.
|
||||
|
||||
- **PR:** `gh pr diff {number} -- {file filter from prompt}`
|
||||
- **PR:** `gh pr diff {number}`, then review only the file sections whose paths match the file filter from your prompt (`gh pr diff` takes no path arguments)
|
||||
- **Branch:** `git diff origin/main...{branch} -- {file filter from prompt}`
|
||||
- **Commit range:** `git diff {base}..{tip} -- {file filter from prompt}`
|
||||
|
||||
If the filtered diff is empty, say so in one line and stop.
|
||||
If the filtered diff is empty, write "No findings." to the output file and stop.
|
||||
|
||||
You are a nit reviewer. Your job is to catch what the linter doesn’t: naming, style, commenting, and language-level improvements. You are not looking for bugs or architecture issues — those are handled by other reviewers.
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
# Modern TypeScript (5.0–6.0 RC) — Reference
|
||||
# Modern TypeScript (5.0 to 6.0) Reference
|
||||
|
||||
Reference for writing idiomatic TypeScript. Covers what changed, what it replaced, and what to reach for. Respect the project's minimum TypeScript version: don't emit features from a version newer than what the project targets. Check `package.json` and `tsconfig.json` before writing code.
|
||||
|
||||
@@ -35,11 +35,11 @@ The left column reflects patterns still common before TypeScript 5.x. Write the
|
||||
| `myArray.reverse()` mutating in place | `myArray.toReversed()` (returns new array) | 5.2 |
|
||||
| `myArray.sort(cmp)` mutating in place | `myArray.toSorted(cmp)` (returns new array) | 5.2 |
|
||||
| `const copy = [...arr]; copy[i] = v` | `arr.with(i, v)` (returns new array) | 5.2 |
|
||||
| Manual `has`/`get`/`set` pattern on `Map` | `map.getOrInsert(key, defaultValue)` or `getOrInsertComputed(key, fn)` | 6.0 RC |
|
||||
| `new RegExp(str.replace(/[.\*+?^${}()\[\]\\]/g, '\\$&'))` | `new RegExp(RegExp.escape(str))` | 6.0 RC |
|
||||
| `--moduleResolution node` (node10) | `--moduleResolution nodenext` (Node.js) or `--moduleResolution bundler` (bundlers/Bun) | 6.0 RC |
|
||||
| `"baseUrl": "./src"` + `"@app/*": ["app/*"]` in paths | Remove `baseUrl`; use `"@app/*": ["./src/app/*"]` in paths directly | 6.0 RC |
|
||||
| `module Foo { export const x = 1; }` | `namespace Foo { export const x = 1; }` | 6.0 RC |
|
||||
| Manual `has`/`get`/`set` pattern on `Map` | `map.getOrInsert(key, defaultValue)` or `getOrInsertComputed(key, fn)` | 6.0 |
|
||||
| `new RegExp(str.replace(/[.\*+?^${}()\[\]\\]/g, '\\$&'))` | `new RegExp(RegExp.escape(str))` | 6.0 |
|
||||
| `--moduleResolution node` (node10) | `--moduleResolution nodenext` (Node.js) or `--moduleResolution bundler` (bundlers/Bun) | 6.0 |
|
||||
| `"baseUrl": "./src"` + `"@app/*": ["app/*"]` in paths | Remove `baseUrl`; use `"@app/*": ["./src/app/*"]` in paths directly | 6.0 |
|
||||
| `module Foo { export const x = 1; }` | `namespace Foo { export const x = 1; }` | 6.0 |
|
||||
| `export * from "..."` when all re-exported members are types | `export type * from "..."` (or `export type * as ns from "..."`) | 5.0 |
|
||||
| `function f(): undefined { return undefined; }` — explicit return required in `: undefined`-returning function | Remove the `return` entirely; `undefined`-returning functions no longer require any return statement | 5.1 |
|
||||
| Manual type predicate annotation on a simple arrow: `(x: T \| undefined): x is T => x !== undefined` | Remove the annotation; TypeScript infers `x is T` from `!== null/undefined` and `instanceof` checks automatically | 5.5 |
|
||||
@@ -72,7 +72,7 @@ These enable things that weren't practical before. Reach for them in the describ
|
||||
| `import defer * as ns from "..."` | 5.9 | Defer module _evaluation_ (not loading) until first property access. Module is loaded and verified at import time; side-effects are delayed. Only works with `--module preserve` or `esnext`. |
|
||||
| `Set` algebra methods | 5.5 | Non-mutating: `union`, `intersection`, `difference`, `symmetricDifference` → new `Set`. Predicate: `isSubsetOf`, `isSupersetOf`, `isDisjointFrom` → `boolean`. Requires `esnext` or `es2025` lib. |
|
||||
| `Object.groupBy` / `Map.groupBy` | 5.4 | Group an iterable into buckets by key function. Return type has all keys as optional (not every key is guaranteed present). Requires `esnext` or `es2024`+ lib. |
|
||||
| `Temporal` API types | 6.0 RC | `Temporal.Now`, `Temporal.Instant`, `Temporal.PlainDate`, etc. Available under `esnext` or `esnext.temporal` lib. Usable in runtimes that already ship it (V8 118+, SpiderMonkey, etc.). |
|
||||
| `Temporal` API types | 6.0 | `Temporal.Now`, `Temporal.Instant`, `Temporal.PlainDate`, etc. Available under `esnext` or `esnext.temporal` lib. Usable in runtimes that already ship it (V8 118+, SpiderMonkey, etc.). |
|
||||
| `@satisfies` in JSDoc | 5.0 | Validates that a JS expression satisfies a type without widening it — the TS `satisfies` operator for `.js` files. Write `/** @satisfies {MyType} */` above the declaration or inline on a parenthesized expression. |
|
||||
| `@overload` in JSDoc | 5.0 | Declare multiple call signatures for a JS function. Each JSDoc comment tagged `@overload` is treated as a distinct overload; the final JSDoc comment (without `@overload`) describes the implementation signature. |
|
||||
| Getter/setter with completely unrelated types | 5.1 | `get style(): CSSStyleDeclaration` and `set style(v: string)` can now have fully unrelated types, provided both have explicit type annotations. Previously the getter type was required to be a subtype of the setter type. |
|
||||
|
||||
@@ -20,8 +20,8 @@ Use the canonical docs for shared conventions and validation guidance:
|
||||
|
||||
- PR title and description conventions:
|
||||
`.claude/docs/PR_STYLE_GUIDE.md`
|
||||
- Local validation commands and git hooks: `AGENTS.md` (Essential Commands and
|
||||
Git Hooks sections)
|
||||
- Local validation commands and git hooks: `AGENTS.md` (Essential commands and
|
||||
Workflow sections) and `.claude/docs/WORKFLOWS.md` (Git Hooks section)
|
||||
|
||||
## Body Formatting
|
||||
|
||||
|
||||
@@ -29,7 +29,7 @@ Every step should be concrete enough that another agent could execute it without
|
||||
- ❌ "Improve error handling" → ✓ "Add try-catch to API calls in user-service.ts, return 400 with error message"
|
||||
- ❌ "Update tests" → ✓ "Add test case to auth.test.ts: 'should reject expired tokens with 401'"
|
||||
|
||||
NEVER include thinking output or other stream-of-consciousness prose mid-plan.
|
||||
Write the plan as finished, executable steps. Leave exploratory reasoning out of the plan text.
|
||||
|
||||
### 2. Resolve Unclear Requirements
|
||||
|
||||
@@ -39,9 +39,9 @@ Resolve ambiguity using the conversation, relevant code, existing tests, and doc
|
||||
|
||||
Writing test cases forces disambiguation. Use test definition as a requirements clarification tool.
|
||||
|
||||
### 4. TDD is Non-Negotiable
|
||||
### 4. Plans Follow TDD
|
||||
|
||||
All plans follow: **Red → Green → Refactor**. The refactor phase is MANDATORY.
|
||||
All plans follow **Red → Green → Refactor**, including an explicit refactor phase.
|
||||
|
||||
## The TDD Workflow
|
||||
|
||||
@@ -59,8 +59,6 @@ All plans follow: **Red → Green → Refactor**. The refactor phase is MANDATOR
|
||||
- Integration tests: Component interactions (use for critical paths)
|
||||
- E2E tests: Complete workflows (use sparingly)
|
||||
|
||||
**Write descriptive test cases:**
|
||||
|
||||
If you cannot define a test yet, investigate the behavior and comparable tests first. Use the clarification criteria above only if essential requirements remain unresolved.
|
||||
|
||||
### Green Phase: Make Tests Pass
|
||||
@@ -73,7 +71,7 @@ Focus on correctness first. Hardcode if needed. Add just enough logic. Resist ur
|
||||
|
||||
**Purpose:** Apply insights gained during implementation.
|
||||
|
||||
**This phase is MANDATORY.** During implementation you'll discover better structure, repeated patterns, and simplification opportunities.
|
||||
Every plan includes this phase, because during implementation you'll discover better structure, repeated patterns, and simplification opportunities.
|
||||
|
||||
**When to Extract vs Keep Duplication:**
|
||||
|
||||
|
||||
@@ -92,10 +92,11 @@ shown below when adding new failures.
|
||||
the develop output. Check listeners with `lsof -iTCP:<port> -sTCP:LISTEN`.
|
||||
- Existing docs or tools: [Development Isolation Guide for Agents](DEV_ISOLATION.md)
|
||||
and `scripts/develop/main.go`.
|
||||
- Missing harness piece: There is no automatic per-worktree port allocator.
|
||||
- Proposed prevention: Assign each worktree a unique `CODER_DEV_PORT`,
|
||||
`CODER_DEV_WEB_PORT`, `CODER_DEV_PROXY_PORT`, and
|
||||
`CODER_DEV_PROMETHEUS_PORT` before starting the app.
|
||||
- Missing harness piece: The per-worktree port offset is opt-in, so default
|
||||
runs still collide.
|
||||
- Proposed prevention: Start parallel worktrees with `--port-offset` or
|
||||
`CODER_DEV_PORT_OFFSET=true`. If two paths hash to the same offset, set the
|
||||
`CODER_DEV_*` port variables for one worktree.
|
||||
|
||||
## Symptom: Test using `time.Sleep`
|
||||
|
||||
@@ -136,7 +137,7 @@ shown below when adding new failures.
|
||||
or related annotations.
|
||||
- How to diagnose: Compare the new handler with nearby handlers and inspect
|
||||
generated API docs for the route.
|
||||
- Existing docs or tools: `AGENTS.md`, [Documentation Style Guide](DOCS_STYLE_GUIDE.md),
|
||||
- Existing docs or tools: `AGENTS.md`, [API Design Guardrails](WORKFLOWS.md#api-design-guardrails),
|
||||
and API generation checks.
|
||||
- Missing harness piece: Agents need a doc reminder that endpoint work includes
|
||||
docs unless the route is intentionally experimental.
|
||||
|
||||
@@ -74,7 +74,7 @@ The implementation is primarily in the `coderd/workspaceapps/` directory with co
|
||||
|
||||
## Implementation Details
|
||||
|
||||
The project structure separates frontend and backend concerns. React components and pages are organized in the `site/src/` directory, with Jest used for testing. The backend is primarily written in Go, with a strong emphasis on error handling patterns and test coverage.
|
||||
The project structure separates frontend and backend concerns. React components and pages are organized in the `site/src/` directory, with Vitest and Storybook used for testing. The backend is primarily written in Go, with a strong emphasis on error handling patterns and test coverage.
|
||||
|
||||
Database interactions are carefully managed through migrations in `coderd/database/migrations/` and queries in `coderd/database/queries/`. All new queries require proper database authorization (dbauthz) implementation to ensure that only users with appropriate permissions can access specific resources.
|
||||
|
||||
@@ -113,13 +113,13 @@ Coder emphasizes clear error handling, with specific patterns required:
|
||||
|
||||
All tests should run in parallel using `t.Parallel()` to ensure efficient testing and expose potential race conditions. The codebase is rigorously linted with golangci-lint to maintain consistent code quality.
|
||||
|
||||
Git contributions follow [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/). See [CONTRIBUTING.md](CONTRIBUTING.md#commit-messages) for full rules. PR titles are linted in CI.
|
||||
Git contributions follow [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/). See [CONTRIBUTING.md](../../CONTRIBUTING.md#commit-messages) for full rules. PR titles are linted in CI.
|
||||
|
||||
## Development Workflow
|
||||
|
||||
Development can be initiated using `scripts/develop.sh` to start the application after making changes. Database schema updates should be performed through the migration system using `create_migration.sh <name>` to generate migration files, with each `.up.sql` migration paired with a corresponding `.down.sql` that properly reverts all changes.
|
||||
|
||||
If the development database gets into a bad state, it can be completely reset by removing the PostgreSQL data directory with `rm -rf .coderv2/postgres`. This will destroy all data in the development database, requiring you to recreate any test users, templates, or workspaces after restarting the application.
|
||||
If the development database gets into a bad state, reset it with `./scripts/develop.sh --db-reset`, or stop the app and remove `.coderv2/postgres`. Either way destroys all data in the development database, so you must recreate any test users, templates, or workspaces afterward. See [DEV_ISOLATION.md](DEV_ISOLATION.md).
|
||||
|
||||
Code generation for the database layer uses `coderd/database/generate.sh`, and developers should refer to `sqlc.yaml` for the appropriate style and patterns to follow when creating new queries or tables.
|
||||
|
||||
|
||||
+24
-25
@@ -166,7 +166,7 @@ func TestDatabaseFunction(t *testing.T) {
|
||||
|
||||
### Schema Design
|
||||
|
||||
1. **Use appropriate data types**: VARCHAR for strings, TIMESTAMP for times
|
||||
1. **Match existing column types**: `text` for strings, `timestamp with time zone` for times (see `coderd/database/dump.sql`)
|
||||
2. **Add constraints**: NOT NULL, UNIQUE, FOREIGN KEY as appropriate
|
||||
3. **Create indexes**: For frequently queried columns
|
||||
4. **Consider performance**: Normalize appropriately but avoid over-normalization
|
||||
@@ -203,48 +203,47 @@ func TestDatabaseFunction(t *testing.T) {
|
||||
### Complex Queries
|
||||
|
||||
```sql
|
||||
-- Example: Complex join with aggregation
|
||||
-- name: GetWorkspaceCountsPerUser :many
|
||||
SELECT
|
||||
u.id,
|
||||
u.username,
|
||||
COUNT(w.id) as workspace_count
|
||||
COUNT(w.id) AS workspace_count
|
||||
FROM users u
|
||||
LEFT JOIN workspaces w ON u.id = w.owner_id
|
||||
WHERE u.created_at > $1
|
||||
WHERE u.created_at > @created_after
|
||||
GROUP BY u.id, u.username
|
||||
ORDER BY workspace_count DESC;
|
||||
```
|
||||
|
||||
### Conditional Queries
|
||||
|
||||
sqlc makes cast parameters non-nullable, so an optional filter compares
|
||||
against the zero value instead of `IS NULL`. Use `sqlc.narg` when the caller
|
||||
must pass a real NULL. `GetTemplatesWithFilter` in
|
||||
`coderd/database/queries/templates.sql` is a complete example.
|
||||
|
||||
```sql
|
||||
-- Example: Dynamic filtering
|
||||
SELECT * FROM oauth2_provider_apps
|
||||
-- name: GetTemplatesFiltered :many
|
||||
SELECT * FROM templates
|
||||
WHERE
|
||||
($1::text IS NULL OR name ILIKE '%' || $1 || '%')
|
||||
AND ($2::uuid IS NULL OR organization_id = $2)
|
||||
CASE
|
||||
WHEN @fuzzy_name :: text != '' THEN name ILIKE '%' || @fuzzy_name || '%'
|
||||
ELSE true
|
||||
END
|
||||
AND CASE
|
||||
WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN organization_id = @organization_id
|
||||
ELSE true
|
||||
END
|
||||
ORDER BY created_at DESC;
|
||||
```
|
||||
|
||||
### Audit Patterns
|
||||
|
||||
```go
|
||||
// Example: Auditable database operation
|
||||
func (q *sqlQuerier) UpdateUser(ctx context.Context, arg UpdateUserParams) (User, error) {
|
||||
// Implementation here
|
||||
|
||||
// Audit the change
|
||||
if auditor := audit.FromContext(ctx); auditor != nil {
|
||||
auditor.Record(audit.UserUpdate{
|
||||
UserID: arg.ID,
|
||||
Old: oldUser,
|
||||
New: newUser,
|
||||
})
|
||||
}
|
||||
|
||||
return newUser, nil
|
||||
}
|
||||
```
|
||||
Auditable resources are tracked field by field in `enterprise/audit/table.go`.
|
||||
When you add a column to an auditable type, add it there with an action and
|
||||
rerun `make gen`. HTTP handlers record audit entries with
|
||||
`audit.InitRequest[T]`; read a nearby handler such as `coderd/ai_providers.go`
|
||||
for the pattern.
|
||||
|
||||
## Debugging Database Issues
|
||||
|
||||
|
||||
@@ -165,7 +165,7 @@ superseded by the canonical content guidelines.
|
||||
### Instructions
|
||||
|
||||
- **Numbered lists** for sequential steps
|
||||
- **Start with verb**: "Navigate to", "Click", "Select", "Run"
|
||||
- **Start with verb**: "Navigate to", "Select", "Run" (write `select`, not `click`; DOC6 in [docs/AGENTS.md](../../docs/AGENTS.md))
|
||||
- **Be specific**: Include exact button/menu names in bold
|
||||
|
||||
## Code Examples
|
||||
@@ -276,7 +276,7 @@ Document exact values from code:
|
||||
|
||||
## Documentation Manifest
|
||||
|
||||
**CRITICAL**: All documentation pages must be added to `docs/manifest.json` to appear in navigation. Read the manifest file to understand the structure and find the appropriate section for your documentation. Place new pages in logical sections matching the existing hierarchy.
|
||||
Add every documentation page to `docs/manifest.json`; pages missing from the manifest do not appear in navigation. Read the manifest file to understand the structure and find the appropriate section for your documentation. Place new pages in logical sections matching the existing hierarchy.
|
||||
|
||||
## Documentation lands with the change
|
||||
|
||||
@@ -372,7 +372,7 @@ When you rename or move a doc page, create a PR in coder/coder.com to add the re
|
||||
2. **Be precise** - Use exact numbers, permission names, and API paths for
|
||||
the values that belong on the page per
|
||||
[Evidence justifies a claim; it does not belong in the claim](../../docs/.style/content-guidelines.md#evidence-justifies-a-claim-it-does-not-belong-in-the-claim)
|
||||
3. **Visual structure** - Organize around screenshots when available
|
||||
3. **Screenshots** - Include one only when the topic needs it; refer to [Screenshot policy](#screenshot-policy)
|
||||
4. **Link everything** - Related docs, API endpoints, CLI references
|
||||
5. **Manifest inclusion** - Add to manifest.json for navigation
|
||||
6. **Add redirects** - When moving/renaming pages, add redirects in coder/coder.com repo
|
||||
|
||||
+10
-13
@@ -7,7 +7,9 @@ declares. Check `go.mod` before writing code.
|
||||
|
||||
## Go LSP Navigation
|
||||
|
||||
Use Go LSP tools first for backend code navigation:
|
||||
Use the Go language server when available for backend code navigation.
|
||||
Claude Code exposes it from `.mcp.json` under the tool names below;
|
||||
other harnesses name the same operations differently:
|
||||
|
||||
- **Find definitions**: `mcp__go-language-server__definition symbolName`
|
||||
- **Find references**: `mcp__go-language-server__references symbolName`
|
||||
@@ -120,14 +122,14 @@ directive version required in `go.mod`.
|
||||
| `atomic.LoadInt64` / `AddInt64` / `StoreInt64` etc. | `atomic.Int64` (also `Int32`, `Uint32`, `Uint64`, `Bool`, `Pointer[T]`) | 1.19 |
|
||||
| `crypto/rand.Read(buf)` + hex/base64 encode | `crypto/rand.Text()` (one call) | 1.24 |
|
||||
| Checking `crypto/rand.Read` error | don't: return is always nil | 1.24 |
|
||||
| `time.Sleep` in tests | `testing/synctest` (deterministic fake clock) | 1.24/1.25 |
|
||||
| `time.Sleep` in tests | `quartz.NewMock(t)` (this repo's fake clock; see TESTING.md) | n/a |
|
||||
| `json:",omitempty"` on zero-value structs like `time.Time{}` | `json:",omitzero"` (uses `IsZero()` method) | 1.24 |
|
||||
| `strings.Title` | `golang.org/x/text/cases` | 1.18 |
|
||||
| `net.IP` in new code | `net/netip.Addr` (immutable, comparable, lighter) | 1.18 |
|
||||
| `tools.go` with blank imports | `tool` directive in `go.mod` | 1.24 |
|
||||
| `runtime.SetFinalizer` | `runtime.AddCleanup` (multiple per object, no pointer cycles) | 1.24 |
|
||||
| `httputil.ReverseProxy.Director` | `.Rewrite` hook + `ProxyRequest` (Director deprecated in 1.26) | 1.20 |
|
||||
| `sql.NullString`, `sql.NullInt64`, etc. | `sql.Null[T]` | 1.22 |
|
||||
| `sql.NullString`, `sql.NullInt64`, etc. in hand-written code | `sql.Null[T]` (sqlc-generated models keep `sql.NullString`; match them) | 1.22 |
|
||||
| Manual `ctx, cancel := context.WithCancel(…)` + `t.Cleanup(cancel)` | `t.Context()` (auto-canceled when test ends) | 1.24 |
|
||||
| `if d < 0 { d = -d }` on durations | `d.Abs()` (handles `math.MinInt64`) | 1.19 |
|
||||
| Implement only `TextMarshaler` | also implement `TextAppender` for alloc-free marshaling | 1.24 |
|
||||
@@ -204,13 +206,8 @@ integer type. Default source is `ChaCha8` (crypto-quality). No global
|
||||
|
||||
### `log/slog` (1.21)
|
||||
|
||||
`slog.Info`, `slog.Warn`, `slog.Error`, `slog.Debug` with key-value
|
||||
pairs. `slog.With(attrs...)` for logger with preset fields.
|
||||
`slog.GroupAttrs` (1.25) for clean group creation. Implement
|
||||
`slog.Handler` for custom backends.
|
||||
|
||||
**Note:** This project uses `cdr.dev/slog/v3`, not `log/slog`. The
|
||||
API is different. Read existing code for usage patterns.
|
||||
This project logs with `cdr.dev/slog/v3`, not `log/slog`. The API is
|
||||
different. Read existing code for usage patterns.
|
||||
|
||||
## Pitfalls
|
||||
|
||||
@@ -219,9 +216,9 @@ exists. Check your output against these.
|
||||
|
||||
**Version misuse.** The replacement table has a "Since" column. If the
|
||||
project's `go.mod` says `go 1.22`, you cannot use `wg.Go` (1.25),
|
||||
`errors.AsType` (1.26), `new(expr)` (1.26), `b.Loop()` (1.24), or
|
||||
`testing/synctest` (1.24). Fall back to the older pattern. Always
|
||||
check before reaching for a replacement.
|
||||
`errors.AsType` (1.26), `new(expr)` (1.26), or `b.Loop()` (1.24).
|
||||
Fall back to the older pattern. Always check before reaching for a
|
||||
replacement.
|
||||
|
||||
**`slices.Sort` vs `slices.SortFunc`.** `slices.Sort` requires
|
||||
`cmp.Ordered` types (int, string, float64, etc.). For structs, custom
|
||||
|
||||
+5
-10
@@ -8,7 +8,7 @@ When implementing standard protocols (OAuth2, OpenID Connect, etc.):
|
||||
|
||||
1. **Fetch and Analyze Official RFCs**:
|
||||
- Always read the actual RFC specifications before implementation
|
||||
- Use WebFetch tool to get current RFC content for compliance verification
|
||||
- Fetch the current RFC text (for example, with your harness's web fetch tool) for compliance verification
|
||||
- Document RFC requirements in code comments
|
||||
|
||||
2. **Default Values Matter**:
|
||||
@@ -66,15 +66,10 @@ var (
|
||||
errInvalidPKCE = xerrors.New("invalid code_verifier")
|
||||
)
|
||||
|
||||
// Use OAuth2-compliant error responses
|
||||
type OAuth2Error struct {
|
||||
Error string `json:"error"`
|
||||
ErrorDescription string `json:"error_description,omitempty"`
|
||||
}
|
||||
|
||||
// Return proper OAuth2 errors
|
||||
// Return RFC 6749 errors with the shared helper; codersdk defines the
|
||||
// response type (codersdk.OAuth2Error) and the error codes.
|
||||
if errors.Is(err, errInvalidPKCE) {
|
||||
writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "The PKCE code verifier is invalid")
|
||||
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidGrant, "The PKCE code verifier is invalid")
|
||||
return
|
||||
}
|
||||
```
|
||||
@@ -136,7 +131,7 @@ roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user
|
||||
## OAuth2/Authentication Work Patterns
|
||||
|
||||
- Types go in `codersdk/oauth2.go` or similar
|
||||
- Handlers go in `coderd/oauth2.go` or `coderd/identityprovider/`
|
||||
- Handlers go in `coderd/oauth2.go` or `coderd/oauth2provider/`
|
||||
- Database fields need migration + audit table updates
|
||||
- Always support backward compatibility
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@ This guide documents the PR description style used in the Coder repository, base
|
||||
|
||||
## PR Title Format
|
||||
|
||||
Format: `type(scope): description`. See [CONTRIBUTING.md](CONTRIBUTING.md#commit-messages) for full rules. PR titles are linted in CI.
|
||||
Format: `type(scope): description`. See [CONTRIBUTING.md](../../CONTRIBUTING.md#commit-messages) for full rules. PR titles are linted in CI.
|
||||
|
||||
- Types: `feat`, `fix`, `docs`, `style`, `refactor`, `perf`, `test`, `build`, `ci`, `chore`, `revert`
|
||||
- Scopes must be a real path (directory or file stem) containing all changed files
|
||||
@@ -141,7 +141,7 @@ Refs #[issue-number]
|
||||
|
||||
- ❌ **Test plans** - Testing is handled through code review and CI
|
||||
- ❌ **"Benefits" sections** - Benefits should be clear from the description
|
||||
- ❌ **Implementation details** - Keep it high-level
|
||||
- ❌ **Line-by-line implementation walkthroughs** - Name the key change, as the bugfix example does, and leave the rest to the diff
|
||||
- ❌ **Marketing language** - Stay technical and factual
|
||||
- ❌ **Bullet lists of features** (unless it's a large refactor that needs enumeration)
|
||||
|
||||
@@ -182,7 +182,7 @@ Changes from https://github.com/upstream/repo/pull/XXX/
|
||||
|
||||
## Creating PRs as Draft
|
||||
|
||||
**IMPORTANT**: Unless explicitly told otherwise, always create PRs as drafts using the `--draft` flag:
|
||||
Unless explicitly told otherwise, create PRs as drafts using the `--draft` flag:
|
||||
|
||||
```bash
|
||||
gh pr create --draft --title "..." --body "..."
|
||||
@@ -213,30 +213,3 @@ Only create non-draft PRs when the user explicitly requests it or when following
|
||||
6. **Use soft wrapping** - Let GitHub wrap PR body prose naturally
|
||||
7. **No test plans** - Code review and CI handle testing
|
||||
8. **No benefits sections** - Benefits should be obvious from the technical description
|
||||
|
||||
## Examples by Category
|
||||
|
||||
### Performance Improvements
|
||||
|
||||
Includes query timing metrics and explains the index solution
|
||||
|
||||
### Bug Fixes
|
||||
|
||||
Describes broken behavior then the fix in two sentences
|
||||
|
||||
### Documentation
|
||||
|
||||
- **Major rewrite**: Long form explaining inaccuracies and improvements
|
||||
- **Simple correction**: One sentence for simple correction
|
||||
|
||||
### Features
|
||||
|
||||
Simple statement of what was added and dependencies
|
||||
|
||||
### Refactoring
|
||||
|
||||
Explains why client-side sorting is now redundant
|
||||
|
||||
### Configuration
|
||||
|
||||
Adds guidelines with issue reference
|
||||
|
||||
+13
-8
@@ -54,7 +54,7 @@ dominate package wall time.
|
||||
### Test Package Naming
|
||||
|
||||
- **Black-box tests**: Default to a `package foo_test` test file (e.g.,
|
||||
`identityprovider_test`). This is what the `testpackage` linter enforces.
|
||||
`oauth2provider_test`). This is what the `testpackage` linter enforces.
|
||||
- **White-box / internal tests**: When a test needs to touch unexported
|
||||
symbols, put it in a file named `*_internal_test.go` with `package foo`.
|
||||
The `testpackage` linter's `skip-regexp` already exempts that filename
|
||||
@@ -84,10 +84,12 @@ dominate package wall time.
|
||||
|
||||
```text
|
||||
coderd/
|
||||
├── oauth2.go # Implementation
|
||||
├── oauth2_test.go # Main tests
|
||||
├── oauth2_test_helpers.go # Test utilities
|
||||
└── oauth2_validation.go # Validation logic
|
||||
├── oauth2.go # Route handlers that delegate to oauth2provider
|
||||
├── oauth2_test.go # API-level tests
|
||||
└── oauth2provider/
|
||||
├── tokens.go # Implementation
|
||||
├── tokens_test.go # Black-box tests (package oauth2provider_test)
|
||||
└── tokens_internal_test.go # White-box tests (package oauth2provider)
|
||||
```
|
||||
|
||||
### Test Categories
|
||||
@@ -160,7 +162,7 @@ When facing multiple failing tests or complex integration issues:
|
||||
- Test each fix individually before moving to next issue
|
||||
- Use `make lint` and `make gen` after database changes
|
||||
- Verify RFC compliance with actual specifications
|
||||
- Run comprehensive test suites before considering complete
|
||||
- Before handoff, run the affected packages' tests and the broader checks the changed area requires
|
||||
|
||||
## Test Data Management
|
||||
|
||||
@@ -210,6 +212,8 @@ tests := []struct {
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
result, err := functionUnderTest(tt.input)
|
||||
if tt.wantErr {
|
||||
require.Error(t, err)
|
||||
@@ -236,13 +240,14 @@ require.True(t, condition)
|
||||
### Load Testing
|
||||
|
||||
- Use `scaletest/` directory for load testing scenarios
|
||||
- Run `./scaletest/scaletest.sh` for performance testing
|
||||
- Run load scenarios with `coder exp scaletest` (see `cli/exp_scaletest.go`);
|
||||
the runner template lives in `scaletest/templates/scaletest-runner/`
|
||||
|
||||
### Benchmarking
|
||||
|
||||
```go
|
||||
func BenchmarkFunction(b *testing.B) {
|
||||
for i := 0; i < b.N; i++ {
|
||||
for b.Loop() {
|
||||
// Function call to benchmark
|
||||
_ = functionUnderTest(input)
|
||||
}
|
||||
|
||||
@@ -25,7 +25,7 @@
|
||||
|
||||
1. **"package should be X_test"**
|
||||
- **Solution**: Use `package_test` naming for test files
|
||||
- Example: `identityprovider_test` for black-box testing
|
||||
- Example: `oauth2provider_test` for black-box testing
|
||||
|
||||
2. **Race conditions in tests**
|
||||
- **Solution**: Use unique identifiers instead of hardcoded names
|
||||
@@ -55,7 +55,7 @@
|
||||
|
||||
1. **RFC compliance failures**
|
||||
- **Solution**: Verify against actual RFC specifications, not assumptions
|
||||
- Use WebFetch tool to get current RFC content for compliance verification
|
||||
- Fetch the current RFC text (for example, with your harness's web fetch tool) for compliance verification
|
||||
- Read the actual RFC specifications before implementation
|
||||
|
||||
2. **Default value mismatches**
|
||||
@@ -91,8 +91,8 @@
|
||||
|
||||
## Systematic Debugging Approach
|
||||
|
||||
YOU MUST ALWAYS find the root cause of any issue you are debugging
|
||||
YOU MUST NEVER fix a symptom or add a workaround instead of finding a root cause, even if it is faster.
|
||||
Find the root cause of an issue before fixing it. Do not fix a symptom or
|
||||
add a workaround in place of a root-cause fix, even when the workaround is faster.
|
||||
|
||||
### Multi-Issue Problem Solving
|
||||
|
||||
@@ -118,21 +118,24 @@ When facing multiple failing tests or complex integration issues:
|
||||
- Verify Before Continuing: Did your test work? If not, form new hypothesis - don't add more fixes
|
||||
- Use `make lint` and `make gen` after database changes
|
||||
- Verify RFC compliance with actual specifications
|
||||
- Run comprehensive test suites before considering complete
|
||||
- Before handoff, run the affected packages' tests and the broader checks the changed area requires
|
||||
|
||||
## Debug Commands
|
||||
|
||||
### Useful Debug Commands
|
||||
|
||||
| Command | Purpose |
|
||||
|----------------------------------------------|---------------------------------------|
|
||||
| `make lint` | Run all linters |
|
||||
| `make gen` | Generate mocks, database queries |
|
||||
| `go test -v ./path/to/package -run TestName` | Run specific test with verbose output |
|
||||
| `go test -race ./...` | Run tests with race detector |
|
||||
| Command | Purpose |
|
||||
|------------------------------------------------------|---------------------------------------|
|
||||
| `make lint` | Run all linters |
|
||||
| `make gen` | Generate mocks, database queries |
|
||||
| `go test -v ./path/to/package -run TestName` | Run specific test with verbose output |
|
||||
| `make test-race TEST_PACKAGES=./path/to/package/...` | Run tests with race detector |
|
||||
|
||||
### LSP Debugging
|
||||
|
||||
These are the Claude Code tool names from `.mcp.json`; other harnesses name
|
||||
the same language-server operations differently.
|
||||
|
||||
#### Go LSP (Backend)
|
||||
|
||||
| Command | Purpose |
|
||||
@@ -168,7 +171,7 @@ When facing multiple failing tests or complex integration issues:
|
||||
|
||||
### Go Compilation Errors
|
||||
|
||||
**Error**: `package should be identityprovider_test`
|
||||
**Error**: `package should be oauth2provider_test`
|
||||
|
||||
- **Cause**: Test package naming convention violation
|
||||
- **Solution**: Use `package_test` naming for black-box tests
|
||||
@@ -201,16 +204,16 @@ When facing multiple failing tests or complex integration issues:
|
||||
|
||||
### During Development
|
||||
|
||||
1. **Run tests frequently**: `make test`
|
||||
2. **Use LSP tools for navigation**: Avoid manual searching
|
||||
1. **Run targeted tests frequently**: `make test RUN=TestName`
|
||||
2. **Use a language server for symbol navigation when available**; use text search for strings, SQL, and config
|
||||
3. **Follow RFC specifications precisely**
|
||||
4. **Update audit tables when adding database fields**
|
||||
|
||||
### Before Committing
|
||||
|
||||
1. **Run full test suite**: `make test`
|
||||
1. **Run the affected packages' tests**: `make test TEST_PACKAGES=./path/to/package/...`
|
||||
2. **Check linting**: `make lint`
|
||||
3. **Test with race detector**: `make test-race`
|
||||
3. **Run the race detector when the change touches concurrency**: `make test-race TEST_PACKAGES=./path/to/package/...`
|
||||
|
||||
## Getting Help
|
||||
|
||||
|
||||
+12
-12
@@ -4,7 +4,7 @@
|
||||
|
||||
### Before Starting
|
||||
|
||||
- [ ] Run `git pull` to ensure you're on latest code
|
||||
- [ ] Inspect the working tree and current branch; for an existing PR, check out its branch (see [Working on PR branches](#working-on-pr-branches))
|
||||
- [ ] Check if feature touches database - you'll need migrations
|
||||
- [ ] Check if feature touches audit logs - update `enterprise/audit/table.go`
|
||||
|
||||
@@ -30,7 +30,7 @@
|
||||
- Follow [Effective Go](https://go.dev/doc/effective_go) and [Go's Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)
|
||||
- Create packages when used during implementation
|
||||
- Validate abstractions against implementations
|
||||
- **Test packages**: Use `package_test` naming (e.g., `identityprovider_test`) for black-box testing
|
||||
- **Test packages**: Use `package_test` naming (e.g., `oauth2provider_test`) for black-box testing
|
||||
|
||||
### Error Handling
|
||||
|
||||
@@ -38,7 +38,7 @@
|
||||
- Wrap errors with context
|
||||
- Propagate errors appropriately
|
||||
- Use proper error types
|
||||
- Pattern: `xerrors.Errorf("failed to X: %w", err)`
|
||||
- Pattern: `xerrors.Errorf("get workspace: %w", err)`; name the operation and omit "failed to" (see [ARCHITECTURE.md](ARCHITECTURE.md#development-philosophy))
|
||||
|
||||
## Naming Conventions
|
||||
|
||||
@@ -135,8 +135,8 @@
|
||||
|
||||
### Git Hooks
|
||||
|
||||
**You MUST install and use the git hooks. NEVER bypass them with
|
||||
`--no-verify`. Skipping hooks wastes CI cycles and is unacceptable.**
|
||||
Install and use the git hooks, and do not bypass them with
|
||||
`--no-verify`. Skipping them pushes the same failures to CI and wastes CI cycles.
|
||||
|
||||
The first run will be slow as caches warm up. Consecutive runs are
|
||||
**significantly faster** (often 10x) thanks to Go build cache,
|
||||
@@ -192,7 +192,7 @@ Then make your changes and push normally. Don't use `git push --force` unless th
|
||||
|
||||
## Commit Style
|
||||
|
||||
Format: `type(scope): message`. See [CONTRIBUTING.md](CONTRIBUTING.md#commit-messages) for full rules. PR titles are linted in CI.
|
||||
Format: `type(scope): message`. See [CONTRIBUTING.md](../../CONTRIBUTING.md#commit-messages) for full rules. PR titles are linted in CI.
|
||||
|
||||
- Types: `feat`, `fix`, `docs`, `style`, `refactor`, `perf`, `test`, `build`, `ci`, `chore`, `revert`
|
||||
- Scopes must be a real path (directory or file stem) containing all changed files
|
||||
@@ -202,18 +202,18 @@ Format: `type(scope): message`. See [CONTRIBUTING.md](CONTRIBUTING.md#commit-mes
|
||||
|
||||
## Code Navigation and Investigation
|
||||
|
||||
### Using LSP Tools (STRONGLY RECOMMENDED)
|
||||
### Using Language Servers
|
||||
|
||||
**IMPORTANT**: Always use LSP tools for code navigation and understanding. These tools provide accurate, real-time analysis of the codebase and should be your first choice for code investigation.
|
||||
Use the Go and TypeScript language servers when available for definitions, references, type information, diagnostics, and renames. They resolve symbols across packages more reliably than text search; text search remains the right tool for string literals, SQL, configuration, and docs. The tool names below are the ones Claude Code exposes from `.mcp.json`; other harnesses name the same operations differently.
|
||||
|
||||
#### Go LSP Tools (for backend code)
|
||||
|
||||
1. **Find function definitions** (USE THIS FREQUENTLY):
|
||||
1. **Find function definitions**:
|
||||
- `mcp__go-language-server__definition symbolName`
|
||||
- Example: `mcp__go-language-server__definition getOAuth2ProviderAppAuthorize`
|
||||
- Quickly jump to function implementations across packages
|
||||
|
||||
2. **Find symbol references** (ESSENTIAL FOR UNDERSTANDING IMPACT):
|
||||
2. **Find symbol references**:
|
||||
- `mcp__go-language-server__references symbolName`
|
||||
- Locate all usages of functions, types, or variables
|
||||
- Critical for refactoring and understanding data flow
|
||||
@@ -224,12 +224,12 @@ Format: `type(scope): message`. See [CONTRIBUTING.md](CONTRIBUTING.md#commit-mes
|
||||
|
||||
#### TypeScript LSP Tools (for frontend code in site/)
|
||||
|
||||
1. **Find component/function definitions** (USE THIS FREQUENTLY):
|
||||
1. **Find component/function definitions**:
|
||||
- `mcp__typescript-language-server__definition symbolName`
|
||||
- Example: `mcp__typescript-language-server__definition LoginPage`
|
||||
- Quickly navigate to React components, hooks, and utility functions
|
||||
|
||||
2. **Find symbol references** (ESSENTIAL FOR UNDERSTANDING IMPACT):
|
||||
2. **Find symbol references**:
|
||||
- `mcp__typescript-language-server__references symbolName`
|
||||
- Locate all usages of components, types, or functions
|
||||
- Critical for refactoring React components and understanding prop usage
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: code-review
|
||||
description: Reviews code changes for bugs, security issues, and quality problems
|
||||
description: Reviews code changes for bugs, security issues, and quality problems. Use for a single-pass review of a PR, branch, or local diff; for large PRs that span several subsystems, use deep-review (`.agents/skills/deep-review/`).
|
||||
---
|
||||
|
||||
# Code Review Skill
|
||||
@@ -65,7 +65,7 @@ api.Database.GetResource(ctx, id)
|
||||
|
||||
```go
|
||||
// OAuth2 endpoints use RFC-compliant errors
|
||||
writeOAuth2Error(ctx, rw, http.StatusBadRequest, "invalid_grant", "description")
|
||||
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidGrant, "description")
|
||||
|
||||
// Regular endpoints use httpapi
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{...})
|
||||
@@ -94,6 +94,8 @@ GitHub Actions context variables (`github.*`, `inputs.*`) are always defined.
|
||||
|
||||
- **Only comment when confident** - If you're not 80%+ sure it's a real issue,
|
||||
don't comment. Verify claims before posting.
|
||||
- **No speculation** - Avoid "might", "could", "consider". State facts or skip.
|
||||
- **No speculation about bugs** - For correctness and security issues, avoid
|
||||
"might", "could", "consider"; state facts or skip. Design suggestions may be
|
||||
phrased as questions ("Could we use X instead?").
|
||||
- **Verify technical claims** - Check documentation or code before asserting how
|
||||
something works. Don't guess at API behavior or syntax rules.
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: doc-check
|
||||
description: Checks if code changes require documentation updates
|
||||
description: Checks if code changes require documentation updates. Use when reviewing a PR or diff for missing, stale, or incorrect docs under docs/; to write or restructure docs, use write-docs.
|
||||
---
|
||||
|
||||
# Documentation Check Skill
|
||||
@@ -163,9 +163,7 @@ second one, and how the Slack notice knows a review had findings. A
|
||||
comment without it reads as silence to everything downstream.
|
||||
|
||||
After you post or edit, read the comment back and confirm the marker is
|
||||
there. If it is not, edit the comment to add it. This happened on
|
||||
`coder/coder#28723`: the review found a real gap, posted it without the
|
||||
marker, and the notice said "No docs needed".
|
||||
there. If it is not, edit the comment to add it.
|
||||
|
||||
### One comment per pull request
|
||||
|
||||
|
||||
@@ -105,6 +105,6 @@ when all rules PASS or remaining FAILs have a written justification.
|
||||
## Notes
|
||||
|
||||
- This audit does not replace `pnpm check`, `pnpm lint`, `pnpm format`, or
|
||||
tests; run those too (see site/AGENTS.md Pre-PR Checklist).
|
||||
tests; run those too (see the Completion section of `site/AGENTS.md`).
|
||||
- Report findings in the current diff only. Do not refactor pre-existing
|
||||
violations in untouched code; note them at most.
|
||||
|
||||
@@ -70,7 +70,7 @@ Docs use `pnpm run format-docs` and `pnpm run lint-docs`. Frontend commands live
|
||||
|
||||
- **Database changes:** edit `coderd/database/queries/*.sql`, run `make gen`, update `enterprise/audit/table.go` for audit errors, then run `make gen` again.
|
||||
- **New resources:** scope every new resource to an organization (`organization_id` column, organization-scoped RBAC and routes), never deployment-wide.
|
||||
- **OAuth2:** return RFC-compliant errors such as `writeOAuth2Error(...)`. Public endpoints that need system access use `dbauthz.AsSystemRestricted`.
|
||||
- **OAuth2:** return RFC-compliant errors with `httpapi.WriteOAuth2Error(...)`. Public endpoints that need system access use `dbauthz.AsSystemRestricted`.
|
||||
- **Chatd:** when a change affects the documented architecture, do not edit the architecture document yourself. Leave TODO items in the affected sections; the human PR author writes the actual updates.
|
||||
- When you review a PR, treat changes to that document as the owner's work. TODOs are notes for the author, so flag any that the PR adds to the architecture document.
|
||||
- **Public API:** add the required Swagger annotations for new public HTTP endpoints.
|
||||
|
||||
@@ -1851,7 +1851,7 @@ func TestPlanTurnPromptContract(t *testing.T) {
|
||||
require.True(t, requestHasSystemSubstring(recorded[0], "You may use execute and process_output for exploration"))
|
||||
require.True(t, requestHasSystemSubstring(recorded[0], "approved external MCP tools when available"))
|
||||
require.True(t, requestHasSystemSubstring(recorded[0], "Workspace MCP tools are not available in root plan mode"))
|
||||
require.True(t, requestHasSystemSubstring(recorded[0], "After a successful propose_plan call, stop immediately"))
|
||||
require.False(t, requestHasSystemSubstring(recorded[0], "After a successful propose_plan call, stop immediately"))
|
||||
require.True(t, requestHasSystemSubstring(recorded[0], planModeInstructions))
|
||||
for _, msg := range recorded[0].Messages {
|
||||
if msg.Role != "system" {
|
||||
|
||||
@@ -113,7 +113,7 @@ type ExecuteArgs struct {
|
||||
ModelIntent *string `json:"model_intent,omitempty" description:"A short, natural-language, present-participle phrase describing what you are doing. This is shown to the user alongside the command, with backgrounded commands framed as \"<intent> in the background using <command>\", so do not include the word \"background\" or restate the command or a duration. Use plain English with no underscores or technical jargon. Keep it under 100 characters. Good examples: \"Running the unit tests\", \"Checking repository state\", \"Inspecting build output\"."`
|
||||
Timeout *string `json:"timeout,omitempty" description:"How long to wait for completion (e.g. '30s', '5m'). Default is 10s. The process keeps running if this expires and you get a background_process_id to re-attach. Only applies to foreground commands."`
|
||||
WorkDir *string `json:"workdir,omitempty" description:"Working directory for the command."`
|
||||
RunInBackground *bool `json:"run_in_background,omitempty" description:"Run without blocking. Use for persistent processes (dev servers, file watchers) or when you want to continue working while a command runs and check the result later with process_output. For commands whose result you need before continuing, prefer foreground with a longer timeout. Do NOT use shell & to background processes. It will not work correctly. Always use this parameter instead."`
|
||||
RunInBackground *bool `json:"run_in_background,omitempty" description:"Run without blocking. Use for persistent processes (dev servers, file watchers) or when you want to continue working while a command runs and check the result later with process_output. For commands whose result you need before continuing, prefer foreground with a longer timeout. Use this parameter instead of shell '&', which leaves an untracked process that process_output cannot read."`
|
||||
}
|
||||
|
||||
// ExecuteToolName is the registered name of the execute tool.
|
||||
|
||||
@@ -33,7 +33,7 @@ func ProposePlan(options ProposePlanOptions) fantasy.AgentTool {
|
||||
"propose_plan",
|
||||
"Present a Markdown plan file from the workspace for user review. "+
|
||||
"The file must already exist with a .md extension. Use write_file to create it or edit_files to refine it before calling this tool. "+
|
||||
"Pass the absolute file path to the plan. Important: use the chat-specific absolute plan path, not a generic path like PLAN.md in the home directory. "+
|
||||
"Pass the chat-specific absolute plan path, not a generic path like PLAN.md in the home directory. "+
|
||||
"The tool reads the content from the workspace.",
|
||||
func(ctx context.Context, args ProposePlanArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) {
|
||||
if options.IsPlanTurn {
|
||||
|
||||
@@ -46,7 +46,7 @@ func StartWorkspace(db database.Store, chatID uuid.UUID, options StartWorkspaceO
|
||||
return fantasy.NewAgentTool(
|
||||
"start_workspace",
|
||||
"Start the chat's workspace if it is currently stopped. "+
|
||||
"This tool is idempotent — if the workspace is already "+
|
||||
"This tool is idempotent: if the workspace is already "+
|
||||
"running, it returns immediately. Use create_workspace "+
|
||||
"first if no workspace exists yet. Provide parameter "+
|
||||
"values (from read_template) only if necessary or "+
|
||||
|
||||
@@ -16,14 +16,16 @@ type WriteFileOptions struct {
|
||||
}
|
||||
|
||||
type WriteFileArgs struct {
|
||||
Path string `json:"path"`
|
||||
Content string `json:"content"`
|
||||
Path string `json:"path" description:"Absolute path of the file to write. Plan files must use the chat-specific absolute plan path."`
|
||||
Content string `json:"content" description:"Complete file contents. Replaces any existing contents."`
|
||||
}
|
||||
|
||||
func WriteFile(options WriteFileOptions) fantasy.AgentTool {
|
||||
return fantasy.NewAgentTool(
|
||||
"write_file",
|
||||
"Write a file to the workspace.",
|
||||
"Create a file in the workspace or overwrite an existing one with the given content. "+
|
||||
"Use edit_files for targeted changes to an existing file. "+
|
||||
"During plan turns, only the chat-specific plan file path is writable.",
|
||||
func(ctx context.Context, args WriteFileArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) {
|
||||
var planPath string
|
||||
if options.IsPlanTurn {
|
||||
|
||||
@@ -246,7 +246,7 @@ func TestDefaultSystemPromptSupportsWorkspaceFallback(t *testing.T) {
|
||||
require.Contains(t, DefaultSystemPrompt, "use its available context and capabilities to continue the user's request")
|
||||
require.Contains(t, DefaultSystemPrompt, "Workspace readiness does not guarantee that skills, MCP tools, or context have finished loading")
|
||||
require.Contains(t, DefaultSystemPrompt, "root chats should create one when missing tools, skills, or context block planning")
|
||||
require.Contains(t, DefaultSystemPrompt, "In Plan Mode, workspace MCP tools remain unavailable after workspace creation; do not provision a workspace solely to access them")
|
||||
require.NotContains(t, DefaultSystemPrompt, "In Plan Mode, workspace MCP tools remain unavailable after workspace creation; do not provision a workspace solely to access them")
|
||||
require.NotContains(t, DefaultSystemPrompt, "missing tools, skills, MCPs, or context block planning")
|
||||
require.NotContains(t, DefaultSystemPrompt, "Do not create a workspace by default")
|
||||
require.NotContains(t, DefaultSystemPrompt, "do not create one as the first action merely because you are planning")
|
||||
|
||||
@@ -117,7 +117,6 @@ Do not require plan approval for routine implementation that the user has alread
|
||||
|
||||
Use the conversation, available tools, skills, MCPs, and template metadata when they are sufficient for planning.
|
||||
If no workspace is attached, root chats should create one when missing tools, skills, or context block planning, when the plan requires inspecting, editing, or running workspace files, or before writing the required plan artifact if no other valid plan path is available. Delegated chats must report workspace needs to the parent agent. Use the workspace's available context and capabilities to continue planning.
|
||||
In Plan Mode, workspace MCP tools remain unavailable after workspace creation; do not provision a workspace solely to access them.
|
||||
Once a workspace is available:
|
||||
` + defaultSystemPromptPlanningGuidance + `
|
||||
2. Use write_file to create a Markdown plan file at the absolute
|
||||
@@ -147,7 +146,6 @@ If the plan file already exists, read it first with read_file before replacing o
|
||||
Use write_file to create the plan file and edit_files to refine it.
|
||||
Use ask_user_question for structured clarification instead of freeform questions.
|
||||
When the plan is ready, call propose_plan with the plan file path.
|
||||
After a successful propose_plan call, stop immediately. Do not produce follow-up output.
|
||||
` + defaultSystemPromptPlanPathBlockPlaceholder
|
||||
|
||||
// PlanningOverlayPrompt returns the plan-mode-only instructions appended
|
||||
|
||||
@@ -34,7 +34,7 @@ import (
|
||||
|
||||
const titleGenerationPrompt = "Write a short title for the user's message. " +
|
||||
"Populate the title field with the result. " +
|
||||
"Return only the title text in 2-8 words. " +
|
||||
"Keep the title to 2-8 words. " +
|
||||
"Do not answer the user or describe the title-writing task. " +
|
||||
"Preserve specific identifiers such as PR numbers, repo names, file paths, function names, and error messages. " +
|
||||
"If the message is short or vague, stay close to the user's wording instead of inventing context. " +
|
||||
@@ -75,12 +75,12 @@ func generateQuickgenObject[T any](
|
||||
var result *fantasy.ObjectResult[T]
|
||||
err := chatretry.Retry(ctx, func(retryCtx context.Context) error {
|
||||
var genErr error
|
||||
result, genErr = object.Generate[T](retryCtx, model, call)
|
||||
result, genErr = generateObject[T](retryCtx, model, call)
|
||||
if call.Temperature != nil && isTemperatureRejectedError(genErr) {
|
||||
// The model rejects the temperature parameter. Drop it
|
||||
// for this and any later retry attempts.
|
||||
call.Temperature = nil
|
||||
result, genErr = object.Generate[T](retryCtx, model, call)
|
||||
result, genErr = generateObject[T](retryCtx, model, call)
|
||||
}
|
||||
return genErr
|
||||
}, nil)
|
||||
@@ -106,6 +106,49 @@ func isTemperatureRejectedError(err error) bool {
|
||||
return strings.Contains(text, "temperature")
|
||||
}
|
||||
|
||||
// generateObject generates a structured object, falling back to
|
||||
// text-mode generation when the provider rejects the forced tool_choice
|
||||
// that tool-mode generation sends. Some models, such as Claude Opus 5.5,
|
||||
// return a bad request for tool_choice "tool" and "any", and gateway
|
||||
// model aliases cannot be matched by name.
|
||||
func generateObject[T any](
|
||||
ctx context.Context,
|
||||
model fantasy.LanguageModel,
|
||||
call fantasy.ObjectCall,
|
||||
) (*fantasy.ObjectResult[T], error) {
|
||||
result, err := object.Generate[T](ctx, model, call)
|
||||
if isToolChoiceRejectedError(err) {
|
||||
return object.Generate[T](ctx, textObjectModel{LanguageModel: model}, call)
|
||||
}
|
||||
return result, err
|
||||
}
|
||||
|
||||
// textObjectModel generates objects by parsing a JSON text response
|
||||
// instead of forcing a tool call.
|
||||
type textObjectModel struct {
|
||||
fantasy.LanguageModel
|
||||
}
|
||||
|
||||
func (m textObjectModel) GenerateObject(ctx context.Context, call fantasy.ObjectCall) (*fantasy.ObjectResponse, error) {
|
||||
return object.GenerateWithText(ctx, m.LanguageModel, call)
|
||||
}
|
||||
|
||||
// isToolChoiceRejectedError reports whether a provider rejected the
|
||||
// request because the model does not accept a forced tool_choice, for
|
||||
// example Anthropic's "tool_choice: type \"tool\" and \"any\" are not
|
||||
// supported for this model.".
|
||||
func isToolChoiceRejectedError(err error) bool {
|
||||
var providerErr *fantasy.ProviderError
|
||||
if !errors.As(err, &providerErr) {
|
||||
return false
|
||||
}
|
||||
if providerErr.StatusCode != http.StatusBadRequest {
|
||||
return false
|
||||
}
|
||||
text := strings.ToLower(providerErr.Error() + " " + string(providerErr.ResponseBody))
|
||||
return strings.Contains(text, "tool_choice")
|
||||
}
|
||||
|
||||
const (
|
||||
// maxConversationContextRunes caps the conversation sample in manual
|
||||
// title prompts to avoid exceeding model context windows.
|
||||
@@ -470,7 +513,13 @@ func (p *Server) maybeGenerateChatTitle(
|
||||
p.publishChatPubsubEvent(chat, codersdk.ChatWatchEventKindTitleChange, nil)
|
||||
}
|
||||
|
||||
const titleMaxOutputTokens = int64(256)
|
||||
// Quickgen caps leave room for adaptive thinking, which counts toward the cap
|
||||
// on models that think by default (Claude 5+). Keep them at or below 1024:
|
||||
// for older Claude models with a configured effort, fantasy derives a
|
||||
// budget_tokens thinking budget from the cap and enables thinking once it
|
||||
// reaches 1024, and Anthropic rejects thinking with tool-mode generation's
|
||||
// forced tool_choice.
|
||||
const titleMaxOutputTokens = int64(1024)
|
||||
|
||||
func titleObjectCall(resolved resolvedModelCall) fantasy.ObjectCall {
|
||||
return resolved.newObjectCall("propose_title", "Propose a short chat title.", titleMaxOutputTokens)
|
||||
@@ -905,7 +954,7 @@ func renderManualTitlePrompt(
|
||||
}
|
||||
|
||||
write("\n\nRequirements:\n")
|
||||
write("- Return only the title text in 2-8 words.\n")
|
||||
write("- Keep the title to 2-8 words.\n")
|
||||
write("- Populate the title field only.\n")
|
||||
write("- Do not answer the user or describe the title-writing task.\n")
|
||||
write("- Preserve specific identifiers (PR numbers, repo names, file paths, function names, error messages).\n")
|
||||
@@ -1013,7 +1062,8 @@ const (
|
||||
summaryTranscriptMaxRunes = 16000
|
||||
// Cap a single turn so one long message cannot dominate the budget.
|
||||
summaryTranscriptPerMessageMaxRunes = 4000
|
||||
summaryMaxOutputTokens = 512
|
||||
// Includes thinking headroom; see titleMaxOutputTokens.
|
||||
summaryMaxOutputTokens = 1024
|
||||
// Reject pathologically long or verbose summaries.
|
||||
summaryMaxRunes = 750
|
||||
summaryHeadlineMaxRunes = 200
|
||||
@@ -1154,7 +1204,7 @@ func generateChatSummary(
|
||||
var result *fantasy.ObjectResult[generatedChatSummary]
|
||||
err := chatretry.Retry(ctx, func(retryCtx context.Context) error {
|
||||
var genErr error
|
||||
result, genErr = object.Generate[generatedChatSummary](retryCtx, model, call)
|
||||
result, genErr = generateObject[generatedChatSummary](retryCtx, model, call)
|
||||
return genErr
|
||||
}, nil)
|
||||
if err != nil {
|
||||
@@ -1404,7 +1454,8 @@ const turnStatusLabelPrompt = "You write compact chat status labels for a sideba
|
||||
"Prefer short action or state phrases such as Finished, Submitted, Fixed, Testing, Still working, or Waiting for. " +
|
||||
"No quotes, emoji, markdown, or trailing punctuation."
|
||||
|
||||
const turnStatusLabelMaxOutputTokens = int64(64)
|
||||
// Includes thinking headroom; see titleMaxOutputTokens.
|
||||
const turnStatusLabelMaxOutputTokens = int64(1024)
|
||||
|
||||
func turnStatusLabelObjectCall(resolved resolvedModelCall) fantasy.ObjectCall {
|
||||
return resolved.newObjectCall("propose_turn_status_label", "Propose a compact chat status label.", turnStatusLabelMaxOutputTokens)
|
||||
|
||||
@@ -11,6 +11,7 @@ import (
|
||||
"time"
|
||||
|
||||
"charm.land/fantasy"
|
||||
fantasyanthropic "charm.land/fantasy/providers/anthropic"
|
||||
fantasyopenai "charm.land/fantasy/providers/openai"
|
||||
fantasyopenaicompat "charm.land/fantasy/providers/openaicompat"
|
||||
"github.com/google/uuid"
|
||||
@@ -360,7 +361,7 @@ func Test_renderManualTitlePrompt(t *testing.T) {
|
||||
|
||||
require.Contains(t, prompt, "Primary user objective:")
|
||||
require.Contains(t, prompt, "Requirements:")
|
||||
require.Contains(t, prompt, "- Return only the title text in 2-8 words.")
|
||||
require.Contains(t, prompt, "- Keep the title to 2-8 words.")
|
||||
require.Contains(t, prompt, "Do not answer the user or describe the title-writing task")
|
||||
require.Contains(t, prompt, "stay close to the user's wording")
|
||||
require.Contains(t, prompt, "same language as the user's messages")
|
||||
@@ -630,7 +631,7 @@ func TestMaybeGenerateChatTitleAppliesModelConfigReasoningEffort(t *testing.T) {
|
||||
ModelName: "gpt-4o-mini",
|
||||
GenerateObjectFn: func(_ context.Context, call fantasy.ObjectCall) (*fantasy.ObjectResponse, error) {
|
||||
require.NotNil(t, call.MaxOutputTokens)
|
||||
require.Equal(t, int64(256), *call.MaxOutputTokens)
|
||||
require.Equal(t, titleMaxOutputTokens, *call.MaxOutputTokens)
|
||||
providerOptions, ok := call.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions)
|
||||
require.True(t, ok, "%T", call.ProviderOptions[fantasyopenai.Name])
|
||||
require.NotNil(t, providerOptions.ReasoningEffort)
|
||||
@@ -672,7 +673,7 @@ func TestMaybeGenerateChatTitleAppliesModelConfigReasoningEffort(t *testing.T) {
|
||||
func Test_titleGenerationPrompt_UsesSlimRules(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
require.Contains(t, titleGenerationPrompt, "Return only the title text in 2-8 words")
|
||||
require.Contains(t, titleGenerationPrompt, "Keep the title to 2-8 words")
|
||||
require.Contains(t, titleGenerationPrompt, "Do not answer the user or describe the title-writing task")
|
||||
require.Contains(t, titleGenerationPrompt, "stay close to the user's wording")
|
||||
require.Contains(t, titleGenerationPrompt, "same language as the user's message")
|
||||
@@ -982,6 +983,101 @@ func TestGenerateStructuredTitleWithUsage_DropsRejectedTemperature(t *testing.T)
|
||||
"generation should retry without temperature after the model rejects it")
|
||||
}
|
||||
|
||||
// Mirrors Claude Opus 5.5, which rejects both the forced tool_choice of
|
||||
// tool-mode object generation and the temperature parameter.
|
||||
func TestGenerateStructuredTitleWithUsage_FallsBackToTextWhenToolChoiceRejected(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var textCallTemperatures []bool
|
||||
model := &chattest.FakeModel{
|
||||
GenerateObjectFn: func(context.Context, fantasy.ObjectCall) (*fantasy.ObjectResponse, error) {
|
||||
return nil, &fantasy.ProviderError{
|
||||
Title: "bad request",
|
||||
Message: `tool_choice: type "tool" and "any" are not supported for this model.`,
|
||||
StatusCode: http.StatusBadRequest,
|
||||
}
|
||||
},
|
||||
GenerateFn: func(_ context.Context, call fantasy.Call) (*fantasy.Response, error) {
|
||||
textCallTemperatures = append(textCallTemperatures, call.Temperature != nil)
|
||||
if call.Temperature != nil {
|
||||
return nil, newTemperatureRejectedError()
|
||||
}
|
||||
return &fantasy.Response{
|
||||
Content: fantasy.ResponseContent{fantasy.TextContent{Text: `{"title":"Failed workspace logs"}`}},
|
||||
}, nil
|
||||
},
|
||||
}
|
||||
|
||||
title, _, err := generateStructuredTitleWithUsage(
|
||||
t.Context(),
|
||||
model,
|
||||
titleObjectCall(resolvedModelCall{}),
|
||||
titleGenerationPrompt,
|
||||
"summarize failed workspace build logs",
|
||||
)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "Failed workspace logs", title)
|
||||
require.Equal(t, []bool{true, false}, textCallTemperatures,
|
||||
"text-mode generation should also retry without a rejected temperature")
|
||||
}
|
||||
|
||||
// Budget thinking would reject the forced tool_choice of tool-mode generation,
|
||||
// so no configured effort may turn it on for older Claude quickgen models.
|
||||
func TestQuickgenCallsKeepBudgetThinkingOff(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
calls := map[string]func(resolvedModelCall) fantasy.ObjectCall{
|
||||
"title": titleObjectCall,
|
||||
"summary": summaryObjectCall,
|
||||
"label": turnStatusLabelObjectCall,
|
||||
}
|
||||
efforts := []fantasyanthropic.Effort{
|
||||
fantasyanthropic.EffortMinimal,
|
||||
fantasyanthropic.EffortLow,
|
||||
fantasyanthropic.EffortMedium,
|
||||
fantasyanthropic.EffortHigh,
|
||||
fantasyanthropic.EffortXHigh,
|
||||
fantasyanthropic.EffortMax,
|
||||
}
|
||||
for name, newCall := range calls {
|
||||
for _, effort := range efforts {
|
||||
t.Run(name+"/"+string(effort), func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
requests := make(chan *chattest.AnthropicRequest, 1)
|
||||
serverURL := chattest.NewAnthropic(t, func(req *chattest.AnthropicRequest) chattest.AnthropicResponse {
|
||||
select {
|
||||
case requests <- req:
|
||||
default:
|
||||
}
|
||||
return chattest.AnthropicResponse{Error: &chattest.ErrorResponse{
|
||||
StatusCode: http.StatusBadRequest,
|
||||
Type: "invalid_request_error",
|
||||
Message: "request captured",
|
||||
}}
|
||||
})
|
||||
client, err := fantasyanthropic.New(
|
||||
fantasyanthropic.WithAPIKey("test-key"),
|
||||
fantasyanthropic.WithBaseURL(serverURL),
|
||||
)
|
||||
require.NoError(t, err)
|
||||
model, err := client.LanguageModel(t.Context(), "claude-haiku-4-5")
|
||||
require.NoError(t, err)
|
||||
|
||||
call := newCall(resolvedModelCall{providerOptions: fantasy.ProviderOptions{
|
||||
fantasyanthropic.Name: &fantasyanthropic.ProviderOptions{Effort: &effort},
|
||||
}})
|
||||
call.Prompt = fantasy.Prompt{fantasy.NewUserMessage("fix the login bug")}
|
||||
_, err = generateObject[generatedTitle](t.Context(), model, call)
|
||||
require.Error(t, err)
|
||||
|
||||
req := testutil.RequireReceive(t.Context(), t, requests)
|
||||
require.Empty(t, req.Thinking, "max_tokens %d enabled budget thinking", req.MaxTokens)
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestGenerateStructuredTitleWithUsage_TruncatesOverlongTitle(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user