Skip to content

Commit 1804190

Browse files
EdwardAngertEdwardAngert
committed
Merge branch 'main' into dev-container-ga
2 parents 032edc3 + 09c5055 commit 1804190

File tree

87 files changed

+4178
-261
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

87 files changed

+4178
-261
lines changed

‎.mcp.json

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
{
2+
"mcpServers": {
3+
"go-language-server": {
4+
"type": "stdio",
5+
"command": "go",
6+
"args": [
7+
"run",
8+
"github.com/isaacphi/mcp-language-server@latest",
9+
"-workspace",
10+
"./",
11+
"-lsp",
12+
"go",
13+
"--",
14+
"run",
15+
"golang.org/x/tools/gopls@latest"
16+
],
17+
"env": {}
18+
},
19+
"typescript-language-server": {
20+
"type": "stdio",
21+
"command": "go",
22+
"args": [
23+
"run",
24+
"github.com/isaacphi/mcp-language-server@latest",
25+
"-workspace",
26+
"./site/",
27+
"-lsp",
28+
"pnpx",
29+
"--",
30+
"typescript-language-server",
31+
"--stdio"
32+
],
33+
"env": {}
34+
}
35+
}
36+
}

‎CLAUDE.md

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ Read [cursor rules](.cursorrules).
8989
- Format: `{number}_{description}.{up|down}.sql`
9090
- Number must be unique and sequential
9191
- Always include both up and down migrations
92+
- **Use helper scripts**:
93+
- `./coderd/database/migrations/create_migration.sh "migration name"` - Creates new migration files
94+
- `./coderd/database/migrations/fix_migration_numbers.sh` - Renumbers migrations to avoid conflicts
95+
- `./coderd/database/migrations/create_fixture.sh "fixture name"` - Creates test fixtures for migrations
9296

9397
2. **Update database queries**:
9498
- MUST DO! Any changes to database - adding queries, modifying queries should be done in the `coderd/database/queries/*.sql` files
@@ -125,6 +129,29 @@ Read [cursor rules](.cursorrules).
125129
4. Run `make gen` again
126130
5. Run `make lint` to catch any remaining issues
127131

132+
### In-Memory Database Testing
133+
134+
When adding new database fields:
135+
136+
- **CRITICAL**: Update `coderd/database/dbmem/dbmem.go` in-memory implementations
137+
- The `Insert*` functions must include ALL new fields, not just basic ones
138+
- Common issue: Tests pass with real database but fail with in-memory database due to missing field mappings
139+
- Always verify in-memory database functions match the real database schema after migrations
140+
141+
Example pattern:
142+
143+
```go
144+
// In dbmem.go - ensure ALL fields are included
145+
code := database.OAuth2ProviderAppCode{
146+
ID: arg.ID,
147+
CreatedAt: arg.CreatedAt,
148+
// ... existing fields ...
149+
ResourceUri: arg.ResourceUri, // New field
150+
CodeChallenge: arg.CodeChallenge, // New field
151+
CodeChallengeMethod: arg.CodeChallengeMethod, // New field
152+
}
153+
```
154+
128155
## Architecture
129156

130157
### Core Components
@@ -209,6 +236,12 @@ When working on OAuth2 provider features:
209236
- Avoid dependency on referer headers for security decisions
210237
- Support proper state parameter validation
211238

239+
6. **RFC 8707 Resource Indicators**:
240+
- Store resource parameters in database for server-side validation (opaque tokens)
241+
- Validate resource consistency between authorization and token requests
242+
- Support audience validation in refresh token flows
243+
- Resource parameter is optional but must be consistent when provided
244+
212245
### OAuth2 Error Handling Pattern
213246

214247
```go
@@ -237,6 +270,114 @@ if errors.Is(err, errInvalidPKCE) {
237270
- Test both positive and negative cases
238271
- Use `testutil.WaitLong` for timeouts in tests
239272

273+
## Code Navigation and Investigation
274+
275+
### Using Go LSP Tools (STRONGLY RECOMMENDED)
276+
277+
**IMPORTANT**: Always use Go 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.
278+
279+
When working with the Coder codebase, leverage Go Language Server Protocol tools for efficient code navigation:
280+
281+
1. **Find function definitions** (USE THIS FREQUENTLY):
282+
283+
```none
284+
mcp__go-language-server__definition symbolName
285+
```
286+
287+
- Example: `mcp__go-language-server__definition getOAuth2ProviderAppAuthorize`
288+
- Example: `mcp__go-language-server__definition ExtractAPIKeyMW`
289+
- Quickly jump to function implementations across packages
290+
- **Use this when**: You see a function call and want to understand its implementation
291+
- **Tip**: Include package prefix if symbol is ambiguous (e.g., `httpmw.ExtractAPIKeyMW`)
292+
293+
2. **Find symbol references** (ESSENTIAL FOR UNDERSTANDING IMPACT):
294+
295+
```none
296+
mcp__go-language-server__references symbolName
297+
```
298+
299+
- Example: `mcp__go-language-server__references APITokenFromRequest`
300+
- Locate all usages of functions, types, or variables
301+
- Understand code dependencies and call patterns
302+
- **Use this when**: Making changes to understand what code might be affected
303+
- **Critical for**: Refactoring, deprecating functions, or understanding data flow
304+
305+
3. **Get symbol information** (HELPFUL FOR TYPE INFO):
306+
307+
```none
308+
mcp__go-language-server__hover filePath line column
309+
```
310+
311+
- Example: `mcp__go-language-server__hover /Users/thomask33/Projects/coder/coderd/httpmw/apikey.go 560 25`
312+
- Get type information and documentation at specific positions
313+
- **Use this when**: You need to understand the type of a variable or return value
314+
315+
4. **Edit files using LSP** (WHEN MAKING TARGETED CHANGES):
316+
317+
```none
318+
mcp__go-language-server__edit_file filePath edits
319+
```
320+
321+
- Make precise edits using line numbers
322+
- **Use this when**: You need to make small, targeted changes to specific lines
323+
324+
5. **Get diagnostics** (ALWAYS CHECK AFTER CHANGES):
325+
326+
```none
327+
mcp__go-language-server__diagnostics filePath
328+
```
329+
330+
- Check for compilation errors, unused imports, etc.
331+
- **Use this when**: After making changes to ensure code is still valid
332+
333+
### LSP Tool Usage Priority
334+
335+
**ALWAYS USE THESE TOOLS FIRST**:
336+
337+
- **Use LSP `definition`** instead of manual searching for function implementations
338+
- **Use LSP `references`** instead of grep when looking for function/type usage
339+
- **Use LSP `hover`** to understand types and signatures
340+
- **Use LSP `diagnostics`** after making changes to check for errors
341+
342+
**When to use other tools**:
343+
344+
- **Use Grep for**: Text-based searches, finding patterns across files, searching comments
345+
- **Use Bash for**: Running tests, git commands, build operations
346+
- **Use Read tool for**: Reading configuration files, documentation, non-Go files
347+
348+
### Investigation Strategy (LSP-First Approach)
349+
350+
1. **Start with route registration** in `coderd/coderd.go` to understand API endpoints
351+
2. **Use LSP `definition` lookup** to trace from route handlers to actual implementations
352+
3. **Use LSP `references`** to understand how functions are called throughout the codebase
353+
4. **Follow the middleware chain** using LSP tools to understand request processing flow
354+
5. **Check test files** for expected behavior and error patterns
355+
6. **Use LSP `diagnostics`** to ensure your changes don't break compilation
356+
357+
### Common LSP Workflows
358+
359+
**Understanding a new feature**:
360+
361+
1. Use `grep` to find the main entry point (e.g., route registration)
362+
2. Use LSP `definition` to jump to handler implementation
363+
3. Use LSP `references` to see how the handler is used
364+
4. Use LSP `definition` on each function call within the handler
365+
366+
**Making changes to existing code**:
367+
368+
1. Use LSP `references` to understand the impact of your changes
369+
2. Use LSP `definition` to understand the current implementation
370+
3. Make your changes using `Edit` or LSP `edit_file`
371+
4. Use LSP `diagnostics` to verify your changes compile correctly
372+
5. Run tests to ensure functionality still works
373+
374+
**Debugging issues**:
375+
376+
1. Use LSP `definition` to find the problematic function
377+
2. Use LSP `references` to trace how the function is called
378+
3. Use LSP `hover` to understand parameter types and return values
379+
4. Use `Read` to examine the full context around the issue
380+
240381
## Testing Scripts
241382
242383
### OAuth2 Test Scripts
@@ -265,3 +406,6 @@ Always run the full test suite after OAuth2 changes:
265406
4. **Missing newlines** - Ensure files end with newline character
266407
5. **Tests passing locally but failing in CI** - Check if `dbmem` implementation needs updating
267408
6. **OAuth2 endpoints returning wrong error format** - Ensure OAuth2 endpoints return RFC 6749 compliant errors
409+
7. **OAuth2 tests failing but scripts working** - Check in-memory database implementations in `dbmem.go`
410+
8. **Resource indicator validation failing** - Ensure database stores and retrieves resource parameters correctly
411+
9. **PKCE tests failing** - Verify both authorization code storage and token exchange handle PKCE fields

‎agent/agentcontainers/api.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,7 @@ func (api *API) updaterLoop() {
449449
// We utilize a TickerFunc here instead of a regular Ticker so that
450450
// we can guarantee execution of the updateContainers method after
451451
// advancing the clock.
452+
var prevErr error
452453
ticker := api.clock.TickerFunc(api.ctx, api.updateInterval, func() error {
453454
done := make(chan error, 1)
454455
var sent bool
@@ -466,9 +467,15 @@ func (api *API) updaterLoop() {
466467
if err != nil {
467468
if errors.Is(err, context.Canceled) {
468469
api.logger.Warn(api.ctx, "updater loop ticker canceled", slog.Error(err))
469-
} else {
470+
return nil
471+
}
472+
// Avoid excessive logging of the same error.
473+
if prevErr == nil || prevErr.Error() != err.Error() {
470474
api.logger.Error(api.ctx, "updater loop ticker failed", slog.Error(err))
471475
}
476+
prevErr = err
477+
} else {
478+
prevErr = nil
472479
}
473480
default:
474481
api.logger.Debug(api.ctx, "updater loop ticker skipped, update in progress")

‎agent/agentcontainers/api_test.go

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"golang.org/x/xerrors"
2727

2828
"cdr.dev/slog"
29+
"cdr.dev/slog/sloggers/sloghuman"
2930
"cdr.dev/slog/sloggers/slogtest"
3031
"github.com/coder/coder/v2/agent/agentcontainers"
3132
"github.com/coder/coder/v2/agent/agentcontainers/acmock"
@@ -342,6 +343,104 @@ func (f *fakeExecer) getLastCommand() *exec.Cmd {
342343
func TestAPI(t *testing.T) {
343344
t.Parallel()
344345

346+
t.Run("NoUpdaterLoopLogspam", func(t *testing.T) {
347+
t.Parallel()
348+
349+
var (
350+
ctx = testutil.Context(t, testutil.WaitShort)
351+
logbuf strings.Builder
352+
logger = slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug).AppendSinks(sloghuman.Sink(&logbuf))
353+
mClock = quartz.NewMock(t)
354+
tickerTrap = mClock.Trap().TickerFunc("updaterLoop")
355+
firstErr = xerrors.New("first error")
356+
secondErr = xerrors.New("second error")
357+
fakeCLI = &fakeContainerCLI{
358+
listErr: firstErr,
359+
}
360+
)
361+
362+
api := agentcontainers.NewAPI(logger,
363+
agentcontainers.WithClock(mClock),
364+
agentcontainers.WithContainerCLI(fakeCLI),
365+
)
366+
api.Start()
367+
defer api.Close()
368+
369+
// Make sure the ticker function has been registered
370+
// before advancing the clock.
371+
tickerTrap.MustWait(ctx).MustRelease(ctx)
372+
tickerTrap.Close()
373+
374+
logbuf.Reset()
375+
376+
// First tick should handle the error.
377+
_, aw := mClock.AdvanceNext()
378+
aw.MustWait(ctx)
379+
380+
// Verify first error is logged.
381+
got := logbuf.String()
382+
t.Logf("got log: %q", got)
383+
require.Contains(t, got, "updater loop ticker failed", "first error should be logged")
384+
require.Contains(t, got, "first error", "should contain first error message")
385+
logbuf.Reset()
386+
387+
// Second tick should handle the same error without logging it again.
388+
_, aw = mClock.AdvanceNext()
389+
aw.MustWait(ctx)
390+
391+
// Verify same error is not logged again.
392+
got = logbuf.String()
393+
t.Logf("got log: %q", got)
394+
require.Empty(t, got, "same error should not be logged again")
395+
396+
// Change to a different error.
397+
fakeCLI.listErr = secondErr
398+
399+
// Third tick should handle the different error and log it.
400+
_, aw = mClock.AdvanceNext()
401+
aw.MustWait(ctx)
402+
403+
// Verify different error is logged.
404+
got = logbuf.String()
405+
t.Logf("got log: %q", got)
406+
require.Contains(t, got, "updater loop ticker failed", "different error should be logged")
407+
require.Contains(t, got, "second error", "should contain second error message")
408+
logbuf.Reset()
409+
410+
// Clear the error to simulate success.
411+
fakeCLI.listErr = nil
412+
413+
// Fourth tick should succeed.
414+
_, aw = mClock.AdvanceNext()
415+
aw.MustWait(ctx)
416+
417+
// Fifth tick should continue to succeed.
418+
_, aw = mClock.AdvanceNext()
419+
aw.MustWait(ctx)
420+
421+
// Verify successful operations are logged properly.
422+
got = logbuf.String()
423+
t.Logf("got log: %q", got)
424+
gotSuccessCount := strings.Count(got, "containers updated successfully")
425+
require.GreaterOrEqual(t, gotSuccessCount, 2, "should have successful update got")
426+
require.NotContains(t, got, "updater loop ticker failed", "no errors should be logged during success")
427+
logbuf.Reset()
428+
429+
// Reintroduce the original error.
430+
fakeCLI.listErr = firstErr
431+
432+
// Sixth tick should handle the error after success and log it.
433+
_, aw = mClock.AdvanceNext()
434+
aw.MustWait(ctx)
435+
436+
// Verify error after success is logged.
437+
got = logbuf.String()
438+
t.Logf("got log: %q", got)
439+
require.Contains(t, got, "updater loop ticker failed", "error after success should be logged")
440+
require.Contains(t, got, "first error", "should contain first error message")
441+
logbuf.Reset()
442+
})
443+
345444
// List tests the API.getContainers method using a mock
346445
// implementation. It specifically tests caching behavior.
347446
t.Run("List", func(t *testing.T) {

‎agent/agentssh/agentssh.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,9 @@ func (s *Server) startNonPTYSession(logger slog.Logger, session ssh.Session, mag
609609
// and SSH server close may be delayed.
610610
cmd.SysProcAttr = cmdSysProcAttr()
611611

612-
// to match OpenSSH, we don't actually tear a non-TTY command down, even if the session ends.
612+
// to match OpenSSH, we don't actually tear a non-TTY command down, even if the session ends. OpenSSH closes the
613+
// pipes to the process when the session ends; which is what happens here since we wire the command up to the
614+
// session for I/O.
613615
// c.f. https://github.com/coder/coder/issues/18519#issuecomment-3019118271
614616
cmd.Cancel = nil
615617

0 commit comments

Comments
 (0)