Refactor Caddy from API-based routes to config file generation#27
Refactor Caddy from API-based routes to config file generation#27
Conversation
Replaces API-based route management with config file generation and `caddy reload` for route durability across restarts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
12-task plan covering config generation, caller updates, cleanup, and testing for the API-to-config-file refactor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addresses all critical/important findings from code review: - Keep sess.Routes populated with hostnames (not route IDs) - Fix session/cleanup.go to use stored hostnames directly - Fix TUI openRoutes URL construction bug - Add RemoveSession update for session clear - Fix stale Caddyfile path in help message - Add Viper state cleanup and edge case tests - Clarify design doc: warn on missing Caddy, dont start Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t.sh, tests - #5: Keep discoverServerName during transition for GetAllRoutes compat - #7: Add Task 14 to update caddy-start.sh - #10: Explicitly keep contains helper in Task 7 - #11: Document config file permissions (0600 is intentional) - #13: Sort route statuses in CheckCaddyHealth for deterministic output - #14/#16: Add Task 15 for project alias edge case tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ng logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix double .localhost bug in TUI route display (4 locations) - Extract shared buildSessionInfoMap helper to eliminate cmd/ duplication - Unify NormalizeDNSName and SanitizeHostname via shared sanitizeDNS helper - Remove dead server discovery logic from CaddyClient - Simplify TUI loadHostnames to use stored routes instead of Caddy API - Ensure config directory exists before atomic write in SyncRoutes - Fix outdated comment on Routes field in session metadata Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaces per-route Caddy API provisioning with a config-file approach: generates a deterministic Caddy JSON from all sessions (BuildCaddyConfig), writes it atomically (SyncRoutes), and triggers Caddy reload. Removes legacy provisioning and many per-route APIs, updates callers to use session-to-config synchronization. Changes
Sequence Diagram(s)sequenceDiagram
participant Store as Session Store
participant Sync as Syncer (buildSessionInfoMap / SyncRoutes)
participant File as caddy-config.json
participant Caddy as Caddy Server
rect rgba(0,100,200,0.5)
Note over Store,Sync: New Flow — config-file based
Store->>Sync: buildSessionInfoMap(all sessions)
Sync->>Sync: BuildCaddyConfig(sessions)
Sync->>File: write atomically (caddy-config.json)
Sync->>Caddy: caddy reload --config <file>
Caddy->>Caddy: load routes from file
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@caddy/config_test.go`:
- Around line 178-218: TestSyncRoutes fails on Windows because configPath() uses
os.UserHomeDir(), which reads USERPROFILE on Windows, but the test only sets
HOME via t.Setenv; update the test to also set USERPROFILE when setting HOME so
configPath() points to the temp dir on Windows. Specifically, in the "writes
config file" subtest (and likewise in the "skips when disable_caddy is true"
subtest) after t.Setenv("HOME", tmpDir) call, add a t.Setenv("USERPROFILE",
tmpDir) to ensure SyncRoutes and configPath() use the temp configDir on Windows.
In `@caddy/config.go`:
- Around line 135-138: ProjectAlias is being used raw when building hostname and
routeID (the fmt.Sprintf calls that set hostname and routeID) which can produce
invalid DNS names; sanitize it with NormalizeDNSName() first (e.g., create a
sanitizedProjectAlias := NormalizeDNSName(info.ProjectAlias) and use that in the
fmt.Sprintf for hostname and routeID), and apply the same change in the
analogous code in the health.go block that constructs hostnames/route IDs so
both places use the normalized project alias.
In `@caddy/health.go`:
- Around line 90-94: The RouteStatus struct's ServiceUp field is unused and the
counters RoutesWorking and RoutesExisting are incremented together (so always
identical); remove the unused ServiceUp field from RouteStatus and then decide
whether to consolidate or clarify counters: either (A) collapse RoutesWorking
into RoutesExisting (remove RoutesWorking usage and only expose RoutesExisting)
or (B) rename RoutesWorking to RoutesConfigured and update all references where
RoutesWorking is incremented/printed (e.g., where result.RoutesWorking++ and the
display code that prints both counters) so it reflects presence in Caddy config
rather than service reachability; update any documentation/prints (the code that
prints route counts) to use the chosen single counter/name.
In `@caddy/integration_test.go`:
- Around line 64-80: The test currently calls
SyncRoutes(map[string]*SessionInfo{}) which writes an empty Caddy config and
wipes all routes; instead, before cleanup call client.GetAllRoutes() to capture
existing routes (or the current config) and then either (A) construct a sessions
map containing only the two test session IDs ("sess-integration-test-ui",
"sess-integration-test-api") and pass that to SyncRoutes so only those routes
are removed, or (B) save the pre-test route/config snapshot via GetAllRoutes and
restore it after test by calling SyncRoutes with the original sessions; update
the cleanup code that calls SyncRoutes, and keep the verification using
GetAllRoutes/route.ID as-is.
In `@cmd/caddy_sync.go`:
- Around line 11-31: The function buildSessionInfoMap duplicates logic in
tui/model.go; move it into the caddy package as an exported
BuildSessionInfoMap(store *session.SessionStore, registry
*config.ProjectRegistry) map[string]*caddy.SessionInfo, implementing the same
body (use caddy.SessionInfo, session.SessionStore and config.ProjectRegistry
types) and then update callers in cmd/caddy_sync.go and tui/model.go to call
caddy.BuildSessionInfoMap(...) instead of their local copies; ensure the new
function is exported and imported where needed, removing the duplicate
implementations.
In `@cmd/session_create.go`:
- Around line 219-229: This duplicates the hostname construction from
caddy.config.buildRoutes and lacks the empty-dns guard; add a shared helper in
the caddy package named BuildHostname(sessionName, serviceName, projectAlias
string) string that encapsulates the NormalizeDNSName + SanitizeHostname logic
and returns "" when NormalizeDNSName yields an empty string, then replace the
inline construction in this file (the loop building hostnames) to call
caddy.BuildHostname(name, serviceName, projectAlias) and skip entries where it
returns "" so hostnames stay consistent with buildRoutes.
In `@docs/plans/2026-02-12-caddy-config-file-management-design.md`:
- Line 91: Update the design doc to clarify that concurrent session creation is
not strictly "last writer wins" due to a TOCTOU race in SessionStore.Save():
when two session create flows concurrently read sessions.json, each appends and
writes, the second write can overwrite the first session's metadata so
SyncRoutes may only see one of the two sessions; either note this limitation
explicitly in the Concurrent creation section (mention sessions.json,
SessionStore.Save(), and SyncRoutes) and recommend adding file-locking or atomic
merge logic in SessionStore.Save() as a follow-up, or propose specific
mitigation steps (e.g., advisory file locks or read-modify-write retries) to
prevent lost session entries.
In `@docs/plans/2026-02-12-caddy-config-file-management.md`:
- Around line 335-342: The plan snippet for SyncRoutes omits creating the config
directory before creating a temp file; update the Plan’s SyncRoutes text to
match the implementation by calling os.MkdirAll(dir, 0755) (where dir :=
filepath.Dir(cfgPath)) before os.CreateTemp so the directory is guaranteed to
exist prior to creating tmpFile with os.CreateTemp.
In `@session/metadata.go`:
- Line 22: The Routes map now stores hostnames instead of route IDs which breaks
code that expects hostnames; add a release-note telling users to recreate
sessions or run a migration, and implement a migration on session load that
detects legacy route values (e.g., values matching the old pattern like "sess-*"
or lacking a hostname form) in the Routes map, regenerate hostnames from the
session Ports data, replace/update Routes in memory and persist the updated
session file; also make the affected code paths (cleanup logic, envrc
generation, TUI URL generation, tmuxp template rendering—see references to
Routes usage in cleanup.go, envrc.go, tui/model.go and session/tmuxp.go)
tolerant during transition by accepting both legacy IDs and hostnames until
migration runs.
🧹 Nitpick comments (5)
caddy/config.go (2)
164-170:configPath()silently returns empty string onUserHomeDirerror.If
os.UserHomeDir()fails, the caller gets a generic "could not determine config path" error at line 184, losing the actual underlying error (e.g.,$HOMEunset). Consider returning an error or propagating it.♻️ Suggested change to propagate error
-func configPath() string { - home, err := os.UserHomeDir() - if err != nil { - return "" - } - return filepath.Join(home, ".config", "devx", "caddy-config.json") +func configPath() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("could not determine home directory: %w", err) + } + return filepath.Join(home, ".config", "devx", "caddy-config.json"), nil }Then update
SyncRoutesaccordingly:- cfgPath := configPath() - if cfgPath == "" { - return fmt.Errorf("could not determine config path") - } + cfgPath, err := configPath() + if err != nil { + return err + }
219-224: Reload failure is a warning, not an error — verify this is the intended UX.
SyncRoutesreturnsnileven whenreloadCaddyfails. The design doc confirms this is intentional ("config is written for next start"), but callers likesession createandsession rmwill report success even if Caddy isn't updated. Thefmt.Printfwarning may be missed in non-interactive contexts (e.g., scripted usage).Consider using
log.Warnor returning a distinct "partial success" signal if structured logging is adopted later.caddy/routes_test.go (1)
128-135: Customcontainscan be replaced withstrings.Contains.The
stringspackage is already imported (line 4 — actually no,stringsis not imported in the test file). Sinceencoding/jsonand other stdlib packages are imported, addingstringswould be trivial and more idiomatic.♻️ Suggested simplification
+import "strings" + -func contains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} +func contains(s, substr string) bool { + return strings.Contains(s, substr) +}Or inline
strings.Containsat the call sites directly.tui/model.go (1)
1594-1610: Inline session-info-map construction duplicatesbuildSessionInfoMap.This block is functionally identical to
cmd/caddy_sync.go:buildSessionInfoMap. Since the TUI already imports both thecaddyandconfigpackages, consider movingbuildSessionInfoMapto thecaddypackage (or making it importable) so bothcmdandtuican share it, avoiding the risk of the two copies diverging.docs/plans/2026-02-12-caddy-config-file-management.md (1)
15-15: Minor: Markdown heading levels skip from h1 to h3.The document jumps from
#(h1, line 1) directly to###(h3, line 15). Static analysis flags this as MD001. Consider using##for the task headings, or adding an##level in between.
| // Clean up by syncing empty sessions | ||
| err = SyncRoutes(map[string]*SessionInfo{}) | ||
| if err != nil { | ||
| t.Fatalf("failed to make request after deletion: %v", err) | ||
| } | ||
| resp2.Body.Close() | ||
|
|
||
| // Should get 404 (not found) since route is deleted | ||
| if resp2.StatusCode != 404 { | ||
| t.Errorf("expected 404 after route deletion, got %d", resp2.StatusCode) | ||
| t.Fatalf("cleanup SyncRoutes failed: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestProvisionSessionRoutes(t *testing.T) { | ||
| // Test the provisioning function without requiring Caddy | ||
| sessionName := "test-provision" | ||
| ports := map[string]int{ | ||
| "ui": 3000, | ||
| "api": 3001, | ||
| "db": 5432, | ||
| } | ||
|
|
||
| // This will skip Caddy operations if not available | ||
| routes, err := ProvisionSessionRoutes(sessionName, ports) | ||
|
|
||
| // Should not error even if Caddy is not available | ||
| // Verify routes are gone | ||
| routes, err = client.GetAllRoutes() | ||
| if err != nil { | ||
| t.Logf("Provisioning warning (expected if Caddy not running): %v", err) | ||
| t.Fatalf("GetAllRoutes after cleanup failed: %v", err) | ||
| } | ||
|
|
||
| // If Caddy is available, should have created routes | ||
| if len(routes) > 0 { | ||
| expectedServices := []string{"ui", "api", "db"} | ||
| for _, service := range expectedServices { | ||
| if _, exists := routes[service]; !exists { | ||
| t.Errorf("expected route for service %s not found", service) | ||
| } | ||
| for _, route := range routes { | ||
| if route.ID == "sess-integration-test-ui" || route.ID == "sess-integration-test-api" { | ||
| t.Errorf("route %s should have been removed", route.ID) | ||
| } | ||
| } |
There was a problem hiding this comment.
Cleanup with empty sessions wipes all Caddy routes, not just the test routes.
SyncRoutes(map[string]*SessionInfo{}) writes a config with zero routes and reloads Caddy. If this test is run on a developer's machine with active sessions, it will remove all their real routes. Consider restoring the pre-test config or only running this in CI.
🤖 Prompt for AI Agents
In `@caddy/integration_test.go` around lines 64 - 80, The test currently calls
SyncRoutes(map[string]*SessionInfo{}) which writes an empty Caddy config and
wipes all routes; instead, before cleanup call client.GetAllRoutes() to capture
existing routes (or the current config) and then either (A) construct a sessions
map containing only the two test session IDs ("sess-integration-test-ui",
"sess-integration-test-api") and pass that to SyncRoutes so only those routes
are removed, or (B) save the pre-test route/config snapshot via GetAllRoutes and
restore it after test by calling SyncRoutes with the original sessions; update
the cleanup code that calls SyncRoutes, and keep the verification using
GetAllRoutes/route.ID as-is.
| // buildSessionInfoMap converts stored sessions and project registry into | ||
| // the caddy.SessionInfo map needed by CheckCaddyHealth and SyncRoutes. | ||
| func buildSessionInfoMap(store *session.SessionStore, registry *config.ProjectRegistry) map[string]*caddy.SessionInfo { | ||
| sessionInfos := make(map[string]*caddy.SessionInfo) | ||
| for name, sess := range store.Sessions { | ||
| info := &caddy.SessionInfo{ | ||
| Name: name, | ||
| Ports: sess.Ports, | ||
| } | ||
|
|
||
| for alias, project := range registry.Projects { | ||
| if sess.ProjectPath == project.Path { | ||
| info.ProjectAlias = alias | ||
| break | ||
| } | ||
| } | ||
|
|
||
| sessionInfos[name] = info | ||
| } | ||
| return sessionInfos | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, confirm the buildSessionInfoMap function in cmd/caddy_sync.go
echo "=== cmd/caddy_sync.go buildSessionInfoMap function ==="
sed -n '11,31p' cmd/caddy_sync.goRepository: jfox85/devx
Length of output: 738
🏁 Script executed:
#!/bin/bash
# Find and examine the alleged duplicate in tui/model.go
echo "=== Searching for similar pattern in tui/model.go around lines 1594-1610 ==="
if [ -f tui/model.go ]; then
sed -n '1590,1620p' tui/model.go
else
find . -name "model.go" -type f | head -5
fiRepository: jfox85/devx
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of sessionInfos map construction in the codebase
echo "=== All sessionInfos map constructions ==="
rg -n "sessionInfos\s*:=\s*make\(map\[string\]\*caddy\.SessionInfo\)" --type goRepository: jfox85/devx
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check if buildSessionInfoMap is exported or used elsewhere
echo "=== buildSessionInfoMap references ==="
rg -n "buildSessionInfoMap" --type goRepository: jfox85/devx
Length of output: 483
🏁 Script executed:
#!/bin/bash
# Check the package structure and verify if tui can import from cmd
echo "=== Check if buildSessionInfoMap is exported (capitalized) ==="
rg -n "func [A-Z].*sessionInfos" --type go cmd/
echo ""
echo "=== Check current imports in tui/model.go ==="
head -30 tui/model.go | grep "import" -A 20Repository: jfox85/devx
Length of output: 558
🏁 Script executed:
#!/bin/bash
# Check the caddy package to see if there's a public function there
echo "=== caddy package structure ==="
find . -path "*/caddy/*.go" -type f | head -10
echo ""
echo "=== Check for SessionInfo and related types in caddy package ==="
rg -n "type SessionInfo" --type goRepository: jfox85/devx
Length of output: 326
🏁 Script executed:
#!/bin/bash
# Verify the exact dependency relationship between tui and cmd packages
echo "=== Check tui imports ==="
grep "^import" -A 20 tui/model.go | head -30
echo ""
echo "=== Check if cmd imports are used in tui ==="
rg "cmd\." tui/model.go | head -5Repository: jfox85/devx
Length of output: 774
🏁 Script executed:
#!/bin/bash
# Let's see the exact full implementation in both places side by side
echo "=== Full context around tui/model.go sessionInfos construction ==="
sed -n '1590,1625p' tui/model.go
echo ""
echo "=== Check if there's a BuildSessionInfoMap (capitalized) anywhere ==="
rg -n "BuildSessionInfoMap" --type goRepository: jfox85/devx
Length of output: 1312
🏁 Script executed:
#!/bin/bash
# Check the caddy package to see if it already has or should have this utility
echo "=== caddy/health.go to understand SessionInfo context ==="
sed -n '100,130p' caddy/health.go
echo ""
echo "=== Check caddy package exports ==="
grep "^func " caddy/*.go | grep "^caddy" -o | sort -uRepository: jfox85/devx
Length of output: 725
🏁 Script executed:
#!/bin/bash
# Determine the best place for this function by checking what's in caddy vs cmd
echo "=== What's exported from caddy package ==="
rg "^func [A-Z]" caddy/*.go | head -20Repository: jfox85/devx
Length of output: 993
🏁 Script executed:
#!/bin/bash
# Check if caddy package can access session and config types
echo "=== caddy/health.go imports ==="
head -20 caddy/health.go
echo ""
echo "=== caddy/routes.go imports (to see what's available) ==="
head -20 caddy/routes.goRepository: jfox85/devx
Length of output: 858
🏁 Script executed:
#!/bin/bash
# Check the session and config package structure
echo "=== session package imports ==="
head -10 session/metadata.go
echo ""
echo "=== config package imports ==="
head -10 caddy/config.go | grep -A 10 "^package"Repository: jfox85/devx
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Verify if caddy package can safely import session and config types
echo "=== Check if caddy/*.go files already import session or config ==="
grep -r "^import" caddy/*.go | grep -E "(session|config)"
echo ""
echo "=== Confirm SessionStore location ==="
rg "type SessionStore struct" --type go
echo ""
echo "=== Confirm ProjectRegistry location ==="
rg "type ProjectRegistry struct" --type goRepository: jfox85/devx
Length of output: 353
Consolidate duplicated session-info-map construction in the caddy package.
The logic in buildSessionInfoMap (cmd/caddy_sync.go:13-31) is duplicated in tui/model.go:1594-1610. Move this function to the caddy package as an exported BuildSessionInfoMap() to enable both cmd/ and tui/ to import and reuse it, eliminating the duplication while maintaining proper dependency structure.
🤖 Prompt for AI Agents
In `@cmd/caddy_sync.go` around lines 11 - 31, The function buildSessionInfoMap
duplicates logic in tui/model.go; move it into the caddy package as an exported
BuildSessionInfoMap(store *session.SessionStore, registry
*config.ProjectRegistry) map[string]*caddy.SessionInfo, implementing the same
body (use caddy.SessionInfo, session.SessionStore and config.ProjectRegistry
types) and then update callers in cmd/caddy_sync.go and tui/model.go to call
caddy.BuildSessionInfoMap(...) instead of their local copies; ensure the new
function is exported and imported where needed, removing the duplicate
implementations.
| ## Error Handling | ||
|
|
||
| - **Caddy not available**: Session operations still succeed. Config file is written so next Caddy start picks up correct routes. | ||
| - **Concurrent creation**: Last writer wins. Both sessions are in `sessions.json` before `SyncRoutes` runs, so the final config is correct. |
There was a problem hiding this comment.
Concurrent creation caveat is more nuanced than "last writer wins".
The doc states both sessions are in sessions.json before SyncRoutes runs, but if two session create commands run concurrently, there's a TOCTOU window: both load sessions.json, each adds its session, each saves — the second writer overwrites the first session's metadata entry. This would cause the subsequent SyncRoutes to only include one of the two sessions.
This is a pre-existing concern in SessionStore.Save() (not introduced by this PR), but the design doc's claim that "the final config is correct" may be misleading. Consider noting this limitation or adding file-locking to SessionStore in a follow-up.
🤖 Prompt for AI Agents
In `@docs/plans/2026-02-12-caddy-config-file-management-design.md` at line 91,
Update the design doc to clarify that concurrent session creation is not
strictly "last writer wins" due to a TOCTOU race in SessionStore.Save(): when
two session create flows concurrently read sessions.json, each appends and
writes, the second write can overwrite the first session's metadata so
SyncRoutes may only see one of the two sessions; either note this limitation
explicitly in the Concurrent creation section (mention sessions.json,
SessionStore.Save(), and SyncRoutes) and recommend adding file-locking or atomic
merge logic in SessionStore.Save() as a follow-up, or propose specific
mitigation steps (e.g., advisory file locks or read-modify-write retries) to
prevent lost session entries.
| // Atomic write: temp file + rename | ||
| // Note: os.CreateTemp creates with 0600 permissions, which is appropriate | ||
| // for a local config file (owner read/write only) | ||
| dir := filepath.Dir(cfgPath) | ||
| tmpFile, err := os.CreateTemp(dir, "caddy-config-*.json") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create temp file: %w", err) | ||
| } |
There was a problem hiding this comment.
Plan's SyncRoutes snippet is missing the os.MkdirAll that was added to the actual implementation.
The actual caddy/config.go:SyncRoutes (from the relevant snippets) includes os.MkdirAll(dir, 0755) before os.CreateTemp, which was added to fix the "config directory doesn't exist" issue. This plan snippet doesn't reflect that fix. Consider updating the plan to match the final implementation so it stays a useful reference.
🤖 Prompt for AI Agents
In `@docs/plans/2026-02-12-caddy-config-file-management.md` around lines 335 -
342, The plan snippet for SyncRoutes omits creating the config directory before
creating a temp file; update the Plan’s SyncRoutes text to match the
implementation by calling os.MkdirAll(dir, 0755) (where dir :=
filepath.Dir(cfgPath)) before os.CreateTemp so the directory is guaranteed to
exist prior to creating tmpFile with os.CreateTemp.
| Path string `json:"path"` | ||
| Ports map[string]int `json:"ports"` | ||
| Routes map[string]string `json:"routes,omitempty"` // service -> route ID mapping | ||
| Routes map[string]string `json:"routes,omitempty"` // service -> hostname mapping |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Routes values are consumed across the codebase
rg -n --type=go 'sess\.Routes|\.Routes\[' -C3 --glob '!*_test.go'Repository: jfox85/devx
Length of output: 2927
🏁 Script executed:
#!/bin/bash
# Check cleanup.go specifically for how Routes is used
rg -n --type=go 'Routes' session/cleanup.go -C5Repository: jfox85/devx
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Look for any migration, sync, or initialization logic related to Routes
rg -n --type=go 'SyncRoutes|MigrateRoutes|Routes.*=' -C3 --glob '!*_test.go'Repository: jfox85/devx
Length of output: 5748
🏁 Script executed:
#!/bin/bash
# Check the current metadata.go to understand the full context of Routes field
head -50 session/metadata.goRepository: jfox85/devx
Length of output: 1809
Routes semantic change: Document impact on existing sessions in release notes.
The Routes map now stores hostnames (e.g., "foo-bar.localhost") instead of the previous route IDs (e.g., "sess-foo-bar"). While the JSON field name and type remain unchanged (backward-compatible for deserialization), the values in old session files are now semantically incompatible with code that expects hostnames.
Affected code paths:
session/cleanup.go(line 52–55): Buildshttp://{hostname}from Routes valuessession/envrc.go(line 50–53): Uses Routes values as hostnames in environment variablestui/model.go(line 1915–1916, etc.): Treats Routes values as hostnames for URL generationsession/tmuxp.go(line 31–32, etc.): Uses Routes values as hostnames in templates
Recommendation: Add a note in release documentation that users with existing sessions should recreate them to regenerate routes with the new format. Alternatively, consider a migration step (e.g., regenerating Routes from Ports on first load of old sessions).
🤖 Prompt for AI Agents
In `@session/metadata.go` at line 22, The Routes map now stores hostnames instead
of route IDs which breaks code that expects hostnames; add a release-note
telling users to recreate sessions or run a migration, and implement a migration
on session load that detects legacy route values (e.g., values matching the old
pattern like "sess-*" or lacking a hostname form) in the Routes map, regenerate
hostnames from the session Ports data, replace/update Routes in memory and
persist the updated session file; also make the affected code paths (cleanup
logic, envrc generation, TUI URL generation, tmuxp template rendering—see
references to Routes usage in cleanup.go, envrc.go, tui/model.go and
session/tmuxp.go) tolerant during transition by accepting both legacy IDs and
hostnames until migration runs.
- Extract BuildHostname/BuildRouteID helpers to eliminate hostname construction duplication across config.go, health.go, session_create.go - Sanitize ProjectAlias with NormalizeDNSName before use in hostnames - Add empty-dnsService guard in session_create.go hostname construction - Remove unused ServiceUp field and redundant RoutesWorking counter - Fix Windows test failure by setting USERPROFILE alongside HOME - Add test for unsanitized project alias Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
~/.config/devx/caddy-config.jsonand applied viacaddy reload, eliminating fragile per-route API mutationsSyncRoutes()call instead of individual API provisioning/deletionprovisioning.go, API route CRUD, server discovery, and route repair/reordering logicKey Changes
New: Config file generation (
caddy/config.go)BuildCaddyConfig()generates complete Caddy JSON config from session dataSyncRoutes()performs atomic file write (temp + rename) andcaddy reloadNormalizeDNSName,SanitizeHostname) via shared helperRemoved: API-based route management
caddy/provisioning.go(233 lines)CreateRoute,DeleteRoute,EnsureRoutesArray,RepairRoutesfromroutes.godiscoverServerNameserver discovery (dead code after config file approach)Simplified consumers
session create/session rm: callsyncAllCaddyRoutes()instead of per-service API callscaddy check --fix: usesSyncRoutesinstead ofRepairRoutessess.Routesdirectly instead of querying Caddy APIReview fixes (final commit)
.localhostbug in TUI route display (4 locations)buildSessionInfoMaphelper to eliminate duplicationTest plan
-racedetectionBuildCaddyConfig(7 subtests covering empty sessions, project aliases, determinism, sanitization, mixed projects)SyncRoutes(file write verification, disable_caddy skip)SyncRoutesflowgo vetclean,gofmtcleandevx session create→ verify config file written and routes workdevx session rm→ verify routes removed from configdevx caddy check/devx caddy check --fix→ verify health check and repair🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests / Docs