fix: safe workspace delete and agh session remove command#239
Open
igorcesarcode wants to merge 3 commits into
Open
fix: safe workspace delete and agh session remove command#239igorcesarcode wants to merge 3 commits into
igorcesarcode wants to merge 3 commits into
Conversation
Workspace delete now guards against active sessions: wraps the operation in a transaction, checks for active sessions using a new listActiveSessionIDsByWorkspace helper, cleans up stopped sessions before removing the workspace row, and returns ErrWorkspaceHasActiveSessions (HTTP 409) when live sessions block the delete. Adds agh session remove <id> CLI command that deletes a session and its persisted history via the daemon UDS client. - globaldb: transactional DeleteWorkspace with active-session guard - workspace: export ErrWorkspaceHasActiveSessions sentinel error - api/core: map ErrWorkspaceHasActiveSessions to HTTP 409 - cli/client: add DeleteSession to DaemonClient interface and implement on unixSocketClient - cli/session: add session remove subcommand
|
@igorcesarcode is attempting to deploy a commit to the Compozy Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds workspace deletion that blocks on active sessions and cascades stopped sessions, defines ErrWorkspaceHasActiveSessions, maps it to HTTP 409, adds CLI DeleteSession transport and ChangesSession deletion and workspace cascade cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/session_test.go`:
- Around line 616-654: Wrap the body of TestSessionRemoveDeletesSession in a
t.Run subtest using the "Should ..." naming convention (e.g., t.Run("Should
delete session", func(t *testing.T) { ... })), preserving the existing setup and
assertions (including variables like deletedID, the stubClient with
getSessionFn/deleteSessionFn, the call to executeRootCommand, and the JSON
unmarshal/asserts against SessionRecord); ensure you run t.Parallel() either
inside the subtest or keep it at the top-level per project convention and update
scope of variables accordingly so tests still compile and behave the same.
In `@internal/store/globaldb/global_db_workspace.go`:
- Around line 156-158: The deferred call defer func() { _ = rows.Close() }
ignores the Close error; update the defer to either handle or document why it's
safe to ignore by replacing the blank-assignment with a short justification
comment explaining that rows.Close() is on a read-only query result where Close
errors are not actionable (or that the caller cannot recover) and referencing
rows.Close() in the comment to satisfy the guideline about not ignoring errors
without justification; keep the defer but add the explanatory comment
immediately above it mentioning rows.Close() and the rationale.
- Around line 102-104: The deferred tx.Rollback() currently ignores its returned
error; add a brief justification comment immediately above or inside the
deferred func explaining why the error is intentionally ignored (e.g., rollback
is best-effort and will be a no-op after Commit, and any real error path will be
handled by the surrounding code), and keep the current _ = tx.Rollback()
pattern. Reference the deferred func and tx.Rollback() in global_db_workspace.go
so reviewers see the rationale inline.
- Line 98: DeleteWorkspace currently calls g.db.BeginTx(ctx, nil) which does not
acquire SQLite write lock immediately; update DeleteWorkspace to use the
immediate-transaction helpers instead — either wrap the delete logic in
GlobalDB.withImmediateTransaction or use store.ExecuteWrite so the helper opens
a BEGIN IMMEDIATE transaction, or explicitly call g.db.ExecContext(ctx, "BEGIN
IMMEDIATE") before performing the deletes and then use the returned tx for
Exec/Prepare/Commit; ensure you replace the direct BeginTx usage and reference
the same tx variable throughout the delete operations (function:
DeleteWorkspace; helpers: GlobalDB.withImmediateTransaction, store.ExecuteWrite,
or ExecContext("BEGIN IMMEDIATE")).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cca23c7a-a374-4580-945f-542da5a8de88
📒 Files selected for processing (10)
internal/api/core/session_workspace.gointernal/api/core/session_workspace_internal_test.gointernal/cli/client.gointernal/cli/helpers_test.gointernal/cli/session.gointernal/cli/session_test.gointernal/store/globaldb/global_db_test.gointernal/store/globaldb/global_db_workspace.gointernal/workspace/workspace.gointernal/workspace/workspace_test.go
Adds `agh open` CLI command that fetches the daemon's HTTP address via DaemonStatus and opens the web UI in the default system browser. Also fixes errcheck lint violation in DeleteWorkspace deferred rollback.
…tion
- Replace BeginTx with store.ExecuteWrite (BEGIN IMMEDIATE with SQLITE_BUSY
retry) in DeleteWorkspace for correct write lock acquisition
- Add justification comment for rows.Close() blank assignment in
listActiveSessionIDsByWorkspace
- Wrap TestSessionRemoveDeletesSession body in t.Run("Should ...") subtest
per project naming convention
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/open.go`:
- Around line 33-34: The URL is built with fmt.Sprintf("http://%s:%d",
status.HTTPHost, status.HTTPPort) which breaks for IPv6; build the host:port
using net.JoinHostPort(status.HTTPHost, strconv.Itoa(status.HTTPPort)) (or
construct a url.URL and set Host via net.JoinHostPort) and then prepend the
scheme before calling openBrowser(ctx, url); update the code around the URL
construction in internal/cli/open.go where status.HTTPHost/status.HTTPPort are
used and pass the corrected url string to openBrowser.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af3d6aac-71d0-43a4-b0df-4d14aefbf11e
📒 Files selected for processing (4)
internal/cli/open.gointernal/cli/root.gointernal/cli/session_test.gointernal/store/globaldb/global_db_workspace.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/session_test.go
- internal/store/globaldb/global_db_workspace.go
Comment on lines
+33
to
+34
| url := fmt.Sprintf("http://%s:%d", status.HTTPHost, status.HTTPPort) | ||
| return openBrowser(ctx, url) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and show the relevant portion of internal/cli/open.go
sed -n '1,120p' internal/cli/open.go | nl -ba | sed -n '1,120p'
# Show surrounding lines around the cited location (lines 25-60)
sed -n '20,70p' internal/cli/open.go | nl -ba
# Find where status.HTTPHost/HTTPPort are defined and what types they are
rg -n "HTTPHost|HTTPPort" internal | head -n 50
# Print the definitions for HTTPHost/HTTPPort if found
rg -n "HTTPHost" -S internal --glob='*.go' | head -n 20
# Search for URL formatting patterns elsewhere for consistency
rg -n "http://%s:%d|JoinHostPort\\(|url\\.URL|http://\\s*\\+" internal/cli internal | head -n 50Repository: compozy/agh
Length of output: 98
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/cli/open.go (top) =="
cat -n internal/cli/open.go | head -n 160
echo "== internal/cli/open.go (around lines ~20-70) =="
sed -n '20,70p' internal/cli/open.go | cat -n
echo "== occurrences of HTTPHost / HTTPPort =="
rg -n "HTTPHost|HTTPPort" internal | head -n 80
echo "== definitions (HTTPHost token) =="
rg -n "HTTPHost" -S internal --glob='*.go' | head -n 80
echo "== URL formatting patterns elsewhere =="
rg -n "http://%s:%d|JoinHostPort\\(|url\\.URL|http://\\s*\\+" internal/cli internal | head -n 120Repository: compozy/agh
Length of output: 14010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/daemon/settings.go around status HTTPHost/HTTPPort =="
sed -n '120,190p' internal/daemon/settings.go | cat -n
echo "== internal/api/core/status.go around HTTPHost/HTTPPort =="
sed -n '150,240p' internal/api/core/status.go | cat -n
echo "== internal/settings/models.go HTTPHost/HTTPPort fields =="
rg -n "type .*HTTPHost|HTTPHost[[:space:]]+string|HTTPPort[[:space:]]+int" internal/settings/models.go
sed -n '420,470p' internal/settings/models.go | cat -n
echo "== open.go current imports =="
sed -n '1,25p' internal/cli/open.go | cat -nRepository: compozy/agh
Length of output: 7934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where HTTP.Host is set/validated (both config defaults and parsing)
rg -n "config\\.HTTP\\.Host|HTTP\\.Host|http\\.host|Host:\\s*.*HTTP" internal --glob='*.go' | head -n 120
# Specifically look for any bracket/IPv6 normalization around HTTP.Host
rg -n "JoinHostPort\\(|\\[.*\\]|IPv6|To16\\(|IsIP\\(|HasPrefix\\(\"\\[\"\\)" internal --glob='*.go' | head -n 120
# Show likely config HTTP host field usages
rg -n "HTTP\\.(Host|Port)" internal --glob='*.go' | head -n 120
# Also check contract fields: how daemon status HTTPHost is serialized
rg -n "http_host" -S internal --glob='*.go' | head -n 120Repository: compozy/agh
Length of output: 29399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/config/config.go around http.host validation =="
# Show around the reported line number 1216 for "http.host is required"
sed -n '1180,1260p' internal/config/config.go | cat -n
echo "== internal/api/httpapi/server.go around host normalization/binding =="
sed -n '500,620p' internal/api/httpapi/server.go | cat -n
echo "== internal/api/httpapi/handlers.go around boundHost assignment =="
sed -n '60,130p' internal/api/httpapi/handlers.go | cat -n
echo "== internal/api/httpapi/helpers_test.go mustURL for URL patterns =="
sed -n '680,760p' internal/api/httpapi/helpers_test.go | cat -nRepository: compozy/agh
Length of output: 13295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "serverLocalhostKey" internal/api/httpapi/server.go internal/api/httpapi/handlers.go internal/api/httpapi/*.go
rg -n "HTTP\\.Host\\s*=|http\\.host" internal/config/config.go internal/config/*.go internal/settings/*.go | head -n 120
rg -n "JoinHostPort\\(|\\bParseIP\\(|\\bIsIP\\(|\\[::|\\[.*\\]" internal/api/httpapi internal/config internal/settings internal/daemon internal/cli --glob='*.go' | head -n 200Repository: compozy/agh
Length of output: 19569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '460,560p' internal/daemon/settings.go | cat -nRepository: compozy/agh
Length of output: 4228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\\[::|::1|IPv6" internal --glob='*.go' | head -n 200
rg -n "Trim\\(.*\"\\[\\]\"|Trim\\(.*\"\\[\\[\\]|strings\\.Trim\\(.*\"\\[\\]\"|Trim\\(.*\\[\\]|\"\\[\\]\"" internal --glob='*.go' | head -n 200
rg -n "Trim\\(\\s*strings\\.TrimSpace\\(.*\\),\\s*\"\\[\\]\"|Trim\\(\\s*strings\\.TrimSpace\\(.*\\),\\s*\"\\[\\]\"" internal --glob='*.go' | head -n 200
rg -n "net\\.JoinHostPort\\(|address := net\\.JoinHostPort" internal/api/httpapi/server.go internal/daemon/settings.go internal/cli/open.go | head -n 200
# Look for any status->URL formatting helper besides open.go
rg -n "http://\\%s:%d|http://\\s*\\+|url\\.URL\\{.*Host" internal/cli --glob='*.go' | head -n 200Repository: compozy/agh
Length of output: 2652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/api/httpapi/server.go around address := net.JoinHostPort =="
sed -n '620,710p' internal/api/httpapi/server.go | cat -n
echo "== internal/api/httpapi/middleware.go around host normalization (strings.Trim(..., \"[]\")) =="
sed -n '180,280p' internal/api/httpapi/middleware.go | cat -n
echo "== internal/testutil/e2e/runtime_harness_test.go IPv6 loopback HTTP cases =="
sed -n '80,140p' internal/testutil/e2e/runtime_harness_test.go | cat -n
echo "== config HTTPConfig definition (type HTTPConfig) =="
rg -n "type HTTPConfig" internal/config --glob='*.go'Repository: compozy/agh
Length of output: 9019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "type HTTPConfig struct|HTTPConfig\\)|HTTPConfig\\b" internal/config/config.go
sed -n '70,140p' internal/config/config.go | cat -n
rg -n "Trim\\(.*Host.*\"\\[\\]\"|Trim\\(.*\"\\[\\]\".*Host|strings\\.Trim\\(.*host.*\"\\[\\]\"|strings\\.Trim\\(.*Host.*\"\\[\\]\"" internal/config --glob='*.go' | head -n 200
rg -n "http\\.host" internal/config --glob='*.go' | head -n 200Repository: compozy/agh
Length of output: 3135
Build the open URL with net.JoinHostPort to handle IPv6 correctly.
internal/cli/open.go currently formats the address as http://%s:%d from status.HTTPHost/status.HTTPPort; for IPv6 like ::1 this produces an invalid URL. Construct the host:port portion with net.JoinHostPort (or url.URL) before calling openBrowser.
Suggested patch
import (
"context"
"errors"
"fmt"
+ "net"
"os/exec"
"runtime"
+ "strconv"
"github.com/spf13/cobra"
)
@@
- url := fmt.Sprintf("http://%s:%d", status.HTTPHost, status.HTTPPort)
+ url := "http://" + net.JoinHostPort(status.HTTPHost, strconv.Itoa(status.HTTPPort))
return openBrowser(ctx, url)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/open.go` around lines 33 - 34, The URL is built with
fmt.Sprintf("http://%s:%d", status.HTTPHost, status.HTTPPort) which breaks for
IPv6; build the host:port using net.JoinHostPort(status.HTTPHost,
strconv.Itoa(status.HTTPPort)) (or construct a url.URL and set Host via
net.JoinHostPort) and then prepend the scheme before calling openBrowser(ctx,
url); update the code around the URL construction in internal/cli/open.go where
status.HTTPHost/status.HTTPPort are used and pass the corrected url string to
openBrowser.
pedronauck
requested changes
May 31, 2026
Member
pedronauck
left a comment
pedronauck
left a comment
There was a problem hiding this comment.
Automated review — self + Codex (gpt-5.5, xhigh)
- Self verdict: FIX_BEFORE_SHIP (0 blockers, 1 risk, 2 nits)
- Codex verdict: unavailable — the external Codex run could not be executed/validated in this environment (intermittent sandbox stdout/file-read drops made the side-effecting
compozy execsubprocess untrustworthy). This is an environment failure, not a model error and not a "clean" result.
This PR already carries a human CHANGES_REQUESTED, so it is not an auto-merge candidate regardless.
Looks good (verified, no action)
- The DELETE session surface is complete.
BaseHandlers.DeleteSession(internal/api/core/handlers.go:466) exists and is registered on bothhttpapi/routes.go:119andudsapi/routes.go:117(workspaceSessions.DELETE("/:session_id")). The new CLIDeleteSessionclient +agh session removecommand wire to a complete, pre-existing HTTP+UDS handler — surface is closed, not a blocker. DeleteWorkspaceis transactional viastore.ExecuteWrite(BEGIN IMMEDIATE+ boundedSQLITE_BUSYretry,internal/store/write.go:57,157), with an active-session guard returningErrWorkspaceHasActiveSessions→ HTTP 409 (internal/api/core/session_workspace.go:138-142).%w/errors.Isconsistent; the single_ = rows.Close()carries a written justification; no SQLite column/index/constraint change, so no numbered migration is required.
Risks
- Workspace-delete cascade may orphan per-session event stores. The cascade
DELETE FROM sessions WHERE workspace_id = ?(internal/store/globaldb/global_db_workspace.go:112) removes onlyagh.db.sessionsrows, but per-session events live in a separateevents.db(internal/store/sessiondb). Unless the workspace-delete path also invokes the per-session cleanup thatDeleteSessionuses, deleting a workspace can orphan stopped sessions' event DBs/files. Fix: ensure the cascade reuses the full session-removal cleanup (events.db file + dir), or document the scope.
Nits
- Misleading CLI help.
newSessionRemoveCommand(internal/cli/session.go:229-254) lists two byte-identical examples, one captioned "Remove an active session (stops it first)", butRunEonly doesGetSession→DeleteSessionand never stops the session. Fix: drop the duplicate/misleading example, actually stop active sessions first, or state that active sessions are rejected. - No CLI↔daemon contract/integration test for the new delete path (only a stub-client unit test in
session_test.go). Fix: add an end-to-end test exercisingagh session removeagainst the real handler.
The external Codex round will be re-run in a healthy environment to add its independent verdict.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DeleteWorkspaceis now transactional and active-session-safe: wraps the delete in aBEGIN IMMEDIATEtransaction, checks for active sessions via a newlistActiveSessionIDsByWorkspacehelper, cascades deletion of stopped/orphaned sessions, and returnsErrWorkspaceHasActiveSessions(HTTP 409) when live sessions block the delete.agh session remove <id>: new CLI subcommand that deletes a session and its persisted history through the daemon UDS client.DeleteSessiononDaemonClient: added to the interface and implemented onunixSocketClient.Changes
internal/store/globaldb/global_db_workspace.goDeleteWorkspacewith active-session guard and stopped-session cascadeinternal/workspace/workspace.goErrWorkspaceHasActiveSessionssentinelinternal/api/core/session_workspace.goErrWorkspaceHasActiveSessions→ HTTP 409internal/cli/client.goDeleteSessiontoDaemonClientinterface + implementationinternal/cli/session.gosession removesubcommand*_test.goTest plan
agh session remove <stopped-session-id>— removes session and history, prints session infoagh workspace remove <id>with active sessions — returns 409 conflict, workspace untouchedagh workspace remove <id>with only stopped sessions — removes workspace and stopped sessionsmake testpassesmake lintpasses (zero golangci-lint issues)Summary by CodeRabbit
New Features
session removecommand to permanently delete a session from history.opencommand to launch the daemon UI in the system browser.Improvements