fix: safe workspace delete and agh session remove command#239
Merged
pedronauck merged 5 commits intoJun 2, 2026
Merged
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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
WalkthroughBlocks workspace deletion when active sessions exist (new ErrWorkspaceHasActiveSessions), cascades deletion of stopped sessions, maps the sentinel to HTTP 409, adds CLI DeleteSession 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
Contributor
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/core/workspaces.go (1)
228-285:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid deleting session history before the workspace delete commits.
DeleteWorkspacenow removes stopped sessions beforeUnregister. IfUnregisterfails afterwards—especially on the race where a session becomes active afterListAll—the request returns an error but the stopped session artifacts are already gone. That leaves the workspace intact in one layer and partially deleted in another.Please move the irreversible session cleanup to a post-commit step, or have the workspace layer return the stopped session IDs to clean up only after a successful unregister.
🤖 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/api/core/workspaces.go` around lines 228 - 285, The current flow deletes stopped sessions in deleteStoppedWorkspaceSessions before calling Workspaces.Unregister, which can leave the system in a partial state if Unregister fails; change the flow so deletion of stopped session artifacts happens only after a successful workspace unregister (or move the deletion into a post-commit step). Specifically, in the handler that calls deleteStoppedWorkspaceSessions and Workspaces.Unregister (the DeleteWorkspace flow), stop invoking Sessions.Delete before Unregister; instead, collect stopped session IDs via Sessions.ListAll (reuse deleteStoppedWorkspaceSessions or extract a new function that returns []string), call Workspaces.Unregister(ctx, workspace.ID), and only upon successful Unregister iterate over the returned stopped IDs and call Sessions.Delete, handling session.ErrSessionNotFound as before; alternatively have the workspace layer return the stopped session IDs to be cleaned up after Unregister.
🧹 Nitpick comments (2)
internal/api/httpapi/handlers_test.go (1)
1353-1361: ⚡ Quick winAlso assert the 204 response body is empty.
The ordering checks are solid, but this still leaves the HTTP contract half-covered. For a
204 No Contenthandler, please assert that the body is empty too.As per coding guidelines, "Assert both HTTP status code AND response body in HTTP tests — status-code-only assertions are insufficient."
🤖 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/api/httpapi/handlers_test.go` around lines 1353 - 1361, The test currently checks only the status for the DELETE handler using performRequest and recorder; add an assertion that the response body is empty for the 204 No Content case by asserting recorder.Body.String() (or recorder.Body.Len()) is empty after verifying recorder.Code == http.StatusNoContent in the same test (the block that uses performRequest(t, engine, http.MethodDelete, "/api/workspaces/ws_alpha", nil) and variable recorder).internal/api/httpapi/handlers_error_test.go (1)
231-242: ⚡ Quick winAssert the 409 response body, not just the status.
This new handler test verifies the branch, but it still leaves the error payload contract untested. Please decode the body and assert the expected conflict message as well.
As per coding guidelines, "Assert both HTTP status code AND response body in HTTP tests — status-code-only assertions are insufficient."
🤖 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/api/httpapi/handlers_error_test.go` around lines 231 - 242, Update the test to assert the 409 response body payload in addition to the status: after performRequest(t, engine, http.MethodDelete, "/api/workspaces/ws_alpha", nil) and the existing status check, decode resp.Body (e.g., into the error response struct used by your handlers or a map[string]interface{}) and assert the expected conflict message text (the same message your delete workspace handler returns on active sessions). Keep the existing unregisterCalled check; use the same response shape your handlers emit so the test validates both status and body contract.
🤖 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.
Outside diff comments:
In `@internal/api/core/workspaces.go`:
- Around line 228-285: The current flow deletes stopped sessions in
deleteStoppedWorkspaceSessions before calling Workspaces.Unregister, which can
leave the system in a partial state if Unregister fails; change the flow so
deletion of stopped session artifacts happens only after a successful workspace
unregister (or move the deletion into a post-commit step). Specifically, in the
handler that calls deleteStoppedWorkspaceSessions and Workspaces.Unregister (the
DeleteWorkspace flow), stop invoking Sessions.Delete before Unregister; instead,
collect stopped session IDs via Sessions.ListAll (reuse
deleteStoppedWorkspaceSessions or extract a new function that returns []string),
call Workspaces.Unregister(ctx, workspace.ID), and only upon successful
Unregister iterate over the returned stopped IDs and call Sessions.Delete,
handling session.ErrSessionNotFound as before; alternatively have the workspace
layer return the stopped session IDs to be cleaned up after Unregister.
---
Nitpick comments:
In `@internal/api/httpapi/handlers_error_test.go`:
- Around line 231-242: Update the test to assert the 409 response body payload
in addition to the status: after performRequest(t, engine, http.MethodDelete,
"/api/workspaces/ws_alpha", nil) and the existing status check, decode resp.Body
(e.g., into the error response struct used by your handlers or a
map[string]interface{}) and assert the expected conflict message text (the same
message your delete workspace handler returns on active sessions). Keep the
existing unregisterCalled check; use the same response shape your handlers emit
so the test validates both status and body contract.
In `@internal/api/httpapi/handlers_test.go`:
- Around line 1353-1361: The test currently checks only the status for the
DELETE handler using performRequest and recorder; add an assertion that the
response body is empty for the 204 No Content case by asserting
recorder.Body.String() (or recorder.Body.Len()) is empty after verifying
recorder.Code == http.StatusNoContent in the same test (the block that uses
performRequest(t, engine, http.MethodDelete, "/api/workspaces/ws_alpha", nil)
and variable recorder).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00f8ea24-ea90-4e62-a370-017749629652
⛔ Files ignored due to path filters (18)
packages/site/content/runtime/cli-reference/agh.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/onboarding/complete.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/onboarding/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/onboarding/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/onboarding/reset.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/onboarding/status.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/open.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/scheduler/drain.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/session/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/session/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/session/remove.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/task/fail.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/task/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/task/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/task/recover.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/workspace/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/workspace/remove.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/workspaces/resolver.mdxis excluded by!**/*.mdx
📒 Files selected for processing (7)
internal/api/core/memory_workspace_test.gointernal/api/core/workspaces.gointernal/api/httpapi/handlers_error_test.gointernal/api/httpapi/handlers_test.gointernal/cli/cli_integration_test.gointernal/cli/session.gointernal/cli/workspace.go
💤 Files with no reviewable changes (1)
- internal/cli/session.go
✅ Files skipped from review due to trivial changes (2)
- internal/api/core/memory_workspace_test.go
- internal/cli/workspace.go
This was referenced Jun 2, 2026
Closed
Closed
Merged
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 and display the removed session record.opencommand to launch the daemon UI in the system browser.Improvements
Tests