Repository navigation
fix: workspace params - #266
Merged
Merged
Conversation
|
Too many files changed for review. ( |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (220)
📒 Files selected for processing (286)
Disabled knowledge base sources:
WalkthroughThis PR removes workspace ID from agent identity credentials/headers, replacing it with session-identity-derived workspace validation. It introduces a unified workspace resolution engine spanning CLI and daemon (positional/flag/env/session/cwd precedence), renames native tool JSON schema fields from ChangesWorkspace-Scoped Identity & Resolution
Estimated code review effort: 5 (Critical) | ~180 minutes Web Task Agent Context Identity
Estimated code review effort: 2 (Simple) | ~15 minutes Daemon Web OpenAPI Client Removal
Estimated code review effort: 1 (Trivial) | ~5 minutes Build Tooling, Source Policy Enforcement & SQLC Registry
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/daemon/notifier_test.go (1)
250-255: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the hook callback was not invoked.
gotSessionID == ""does not prove dispatch was skipped; a dispatched callback with an empty session ID would pass this test. Track a separatehookCalledboolean and assert it remains false.As per path instructions, tests must verify behavior outcomes rather than only relying on weak indirect assertions.
Also applies to: 283-285
🤖 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/daemon/notifier_test.go` around lines 250 - 255, Strengthen the notifier test around fakeHookRuntime by adding a separate hookCalled flag set inside onToolPreCall, then assert that it remains false after dispatch. Keep gotSessionID assertions only for validating payload data; do not use an empty session ID as evidence that the callback was skipped.Source: Path instructions
internal/api/core/agent_identity.go (1)
28-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report an unknown workspace reference as HTTP 500.
ErrWorkspaceNotFoundis wrapped at Line 137 but has no status mapping, so it reaches the default 500 branch. Map it to the intended client error (normally 404) and cover that response.Also applies to: 135-137
🤖 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/agent_identity.go` around lines 28 - 43, Update StatusForAgentIdentityError to map workspacepkg.ErrWorkspaceNotFound to the intended client-error status, typically http.StatusNotFound, instead of the default 500 response. Add or update coverage for the wrapped unknown-workspace error path around the caller at lines 135-137 and verify it returns 404.internal/daemon/native_tool_approval_grants_test.go (1)
119-123: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the specific denial envelope for the renamed field.
This test only checks
errors.Is(err, ErrToolDenied). Add anerrors.Asassertion for*toolspkg.ToolErrorand verify the error text or reason identifies theworkspacescope mismatch; otherwise an unrelated denial would make this regression pass.🤖 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/daemon/native_tool_approval_grants_test.go` around lines 119 - 123, Strengthen the denial assertion in the workspace-override test by extracting the error with errors.As into *toolspkg.ToolError, then verify its message or reason identifies the workspace scope mismatch. Keep the existing errors.Is(err, toolspkg.ErrToolDenied) assertion while ensuring unrelated denials cannot satisfy the regression.Source: Path instructions
internal/daemon/native_create_tools.go (1)
246-253: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist the requested workspace, not the caller scope.
createReq.Workspacenow contains the selected target workspace, including operator requests, butagentCreatestill storesscope.WorkspaceIDin the catalog entry. An operator targeting another workspace can therefore create an agent with an empty or incorrect workspace binding.Use
createReq.Workspaceforentry.WorkspaceIDand add a test targeting a workspace different fromscope.WorkspaceID. (raw.githubusercontent.com)Proposed fix
- entry.WorkspaceID = strings.TrimSpace(scope.WorkspaceID) + entry.WorkspaceID = strings.TrimSpace(createReq.Workspace)🤖 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/daemon/native_create_tools.go` around lines 246 - 253, Update agentCreate so the catalog entry’s entry.WorkspaceID is populated from the resolved createReq.Workspace rather than scope.WorkspaceID, preserving the selected target workspace for operator requests. Add a test covering an operator targeting a workspace different from scope.WorkspaceID and assert the created agent stores the requested workspace.internal/cli/task_list.go (1)
105-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--scope allbypasses the new workspace-resolution engine.When
scopeRawis"all"(taskpkg.CatalogScopeAll),resolveTaskScopeWorkspacefails validation before ever callingresolveCommandWorkspace(sincetaskpkg.Scope.Validateonly acceptsglobal/workspace). The catch block here then falls back to the raw, unresolvedworkspaceRefinstead of resolving it. Every other scope path in this PR now resolves--workspaceby ID/name/path through the new engine, buttask list --scope all --workspace <name-or-path>will silently pass the raw name/path straight intoTaskListQuery.Workspace, producing incorrect filtering.🐛 Proposed fix
scope, workspace, err := resolveTaskScopeWorkspace(cmd, deps, client, scopeRaw, workspaceRef, false) if err != nil { if strings.TrimSpace(strings.ToLower(scopeRaw)) != string(taskpkg.CatalogScopeAll) { return TaskListQuery{}, err } scope = "" - workspace = strings.TrimSpace(workspaceRef) + workspace = strings.TrimSpace(workspaceRef) + if workspace != "" { + resolution, resolveErr := resolveCommandWorkspace( + cmd.Context(), + cmd, + deps, + client, + workspaceResolutionRequest{FlagRef: workspace}, + ) + if resolveErr != nil { + return TaskListQuery{}, resolveErr + } + workspace = resolution.ID + } }🤖 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/task_list.go` around lines 105 - 112, The --scope all error path assigns the raw workspaceRef without resolving it. Update the scope-all handling around resolveTaskScopeWorkspace to resolve workspaceRef through the existing workspace-resolution engine, such as resolveCommandWorkspace, and assign its canonical workspace value to TaskListQuery.Workspace while preserving the current empty-scope behavior.
🧹 Nitpick comments (14)
internal/daemon/native_automation_tools_test.go (1)
481-516: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
t.Errorf+ a returned error inside the stub callbacks.
t.FatalffromListJobsFn/GetJobFn/ListRunsFnonly behaves correctly if the registry invokes these on the test goroutine; if dispatch ever moves to a worker goroutine the failure is silently mis-reported instead of failing the test. Returning a sentinel error aftert.Errorfkeeps the assertion goroutine-safe and also produces a clearer failure at the call site.🤖 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/daemon/native_automation_tools_test.go` around lines 481 - 516, Replace t.Fatalf calls in the ListJobsFn, GetJobFn, and ListRunsFn stubs with t.Errorf followed by returning an appropriate sentinel error, preserving the existing successful return values otherwise. Apply the same goroutine-safe callback behavior to the related GetTriggerFn and GetRunFn stubs if they use the same assertion pattern, so failures propagate to the registry call site.internal/daemon/native_automation_tool_query.go (1)
13-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
job_id/trigger_id/workspacebetween the outer structs and the embeddedautomationRunQueryInput.
automationJobHistoryInputdeclaresJobID/WorkspaceIDwhile also embeddingautomationRunQueryInput, which declares the same JSON keys. Go's shallower-field rule makes the outer fields win at decode time, so the embedded copies are always zero — including inside the promotedquery()method. The handlers work today only because they overwritequery.JobID/query.TriggerIDafterwards and read the outerWorkspaceID, but any future reader of the embedded values silently gets an empty string.Dropping the outer duplicates (and reading
input.automationRunQueryInputvalues) or dropping the promoted fields from the embedded struct would remove the shadowing.🤖 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/daemon/native_automation_tool_query.go` around lines 13 - 33, Remove the duplicated JobID, TriggerID, and WorkspaceID fields between automationJobHistoryInput, automationTriggerHistoryInput, and automationRunQueryInput so JSON decoding cannot shadow embedded values. Update the affected handlers and promoted query() usage to read the retained automationRunQueryInput fields, while preserving each input’s required identifier behavior.internal/extension/host_api_workspace_binding.go (2)
28-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlias block duplicates every exported constant.
hostAPIWorkspaceBindingand the eight lowercase aliases add a second name for each value with no added meaning; the exported identifiers are usable directly inside the package.🤖 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/extension/host_api_workspace_binding.go` around lines 28 - 40, Remove the redundant hostAPIWorkspaceBinding type alias and the lowercase constant aliases in the alias block, including hostAPIWorkspaceScopeKindKey. Update any references to use the original exported HostAPIWorkspaceBinding type and constants directly within the package.
224-319: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated workspace-binding semantics across
extensionpkgandmcp. Both packages implement the same reference canonicalization, scope-literal binding, and resource-scope validation against a bound workspace, and the copies are already drifting (blankscope.idhandling differs).internal/mcpalready importsextensionpkgforHostAPIWorkspaceBindingFor, so the binder itself can have a single owner.
internal/extension/host_api_workspace_binding.go#L224-L319: export a resolver-parameterized binder (e.g.BindWorkspaceScopedParams(ctx, binding, raw, workspaceID, workspaceRoot, resolver)) that both callers can share, keeping the RPC-error wrapping at the handler layer.internal/mcp/serve_binding.go#L88-L201: replacebindWorkspaceString/bindString/bindResourceScope/validateResourceScopewith calls into that exported binder so MCP and Host API can no longer diverge.🤖 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/extension/host_api_workspace_binding.go` around lines 224 - 319, Centralize workspace-binding behavior by exporting a resolver-parameterized binder from internal/extension/host_api_workspace_binding.go, covering reference canonicalization, scope-literal binding, and resource-scope validation while preserving RPC-error wrapping in the handler layer. Update internal/mcp/serve_binding.go to replace bindWorkspaceString, bindString, bindResourceScope, and validateResourceScope with calls to the shared binder; both sites must use identical blank scope.id handling and resolver behavior.internal/extension/host_api_test.go (1)
256-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInner table subtests omit
t.Parallel().All the cases are read-only or expected-to-fail calls against the shared env, so they can run in parallel like the enclosing suites do. As per coding guidelines, "Go tests must use
Should …subtest names, default tot.Parallel".♻️ Proposed change
t.Run(tc.name, func(t *testing.T) { + t.Parallel() _, missingErr := env.callWithContext(ctx, t, extensionName, tc.method, tc.missingParams)🤖 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/extension/host_api_test.go` at line 256, Update the inner table-driven subtest launched by t.Run in the host API tests to call t.Parallel() at the start of its callback. Preserve the existing test cases and enclosing-suite behavior while ensuring each read-only or expected-failure case uses the required parallel execution pattern.Source: Coding guidelines
internal/extension/host_api_automation_binding.go (1)
25-45: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull list-pagination scan for a single-ID lookup.
Each bound job/trigger lookup pages through every automation record in the workspace just to match one ID, and
requireBoundAutomationRunFiltercan trigger two such scans perautomation/runscall. A directGetJob/GetTriggerfollowed by a workspace-ID comparison is O(1) and preserves the "hide as not-found" parity the tests assert.♻️ Sketch of the direct-lookup alternative
- query := automationpkg.JobListQuery{ - Scope: automationpkg.AutomationScopeWorkspace, - WorkspaceID: strings.TrimSpace(boundWorkspaceID), - Limit: automationpkg.MaxListLimit, - } - for { - page, listErr := automation.ListJobs(ctx, query) - if listErr != nil { - return automationpkg.Job{}, listErr - } - for _, job := range page.Jobs { - if strings.TrimSpace(job.ID) == jobID { - return job, nil - } - } - if !page.HasMore || strings.TrimSpace(page.NextCursor) == "" { - return automationpkg.Job{}, automationpkg.ErrJobNotFound - } - query.Cursor = page.NextCursor - } + job, err := automation.GetJob(ctx, jobID) + if err != nil { + return automationpkg.Job{}, err + } + if strings.TrimSpace(job.WorkspaceID) != strings.TrimSpace(boundWorkspaceID) { + return automationpkg.Job{}, automationpkg.ErrJobNotFound + } + return job, nil🤖 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/extension/host_api_automation_binding.go` around lines 25 - 45, Replace the paginated single-ID lookup loop with a direct GetJob/GetTrigger call in the bound automation lookup helpers, then compare the returned resource’s workspace ID with the bound workspace ID. Convert lookup errors or workspace mismatches to the existing not-found behavior, preserving the hidden-resource semantics and avoiding repeated full workspace scans from requireBoundAutomationRunFilter.internal/daemon/native_tool_workspace_memory_calls.go (1)
30-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWrap the resolver error so
workspace_listreturns a typed tool error.Every other native workspace path (
nativeResolvedWorkspace) maps resolver failures throughnativeNetworkInputError, producing aToolErrorwith a code/reason. Here the rawResolveerror escapes, so a scoped caller whose workspace can't be resolved gets an untyped failure.♻️ Suggested change
if !scope.Operator && strings.TrimSpace(scope.WorkspaceID) != "" { resolved, err := n.deps.Workspaces.Resolve(ctx, scope.WorkspaceID) if err != nil { - return toolspkg.ToolResult{}, err + return toolspkg.ToolResult{}, nativeNetworkInputError(req.ToolID, err) } workspaces = []workspacepkg.Workspace{resolved.Workspace} } else {🤖 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/daemon/native_tool_workspace_memory_calls.go` around lines 30 - 42, Update the Resolve error handling in the scoped branch of native workspace listing to pass the resolver failure through nativeNetworkInputError before returning it. Preserve the existing successful resolution and List error behavior so workspace_list returns a typed ToolError with the expected code and reason for resolution failures.internal/cli/client_workspace_authoring.go (1)
84-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNearest-enclosing algorithm duplicated with a different tie-break vs.
internal/workspace/discovery.go.See consolidated comment for details.
🤖 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/client_workspace_authoring.go` around lines 84 - 115, Replace the duplicated selection logic in nearestCLIWorkspaceRoute with the existing nearest-enclosing workspace algorithm from internal/workspace/discovery.go, including its tie-breaking behavior. Reuse the shared implementation or extract a common helper so CLI and discovery consistently select the same workspace while preserving path canonicalization and invalid-entry handling.internal/workspace/discovery.go (1)
12-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNearest-enclosing algorithm duplicated with a different tie-break in the CLI package.
See consolidated comment for details (shared with
internal/cli/client_workspace_authoring.go).🤖 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/workspace/discovery.go` around lines 12 - 59, Replace the duplicated nearest-enclosing workspace selection logic in Resolver.lookupWorkspaceByEnclosingRoot with the consolidated shared algorithm used by the CLI workspace-authoring flow. Ensure both callers use the same root matching, depth comparison, and tie-breaking behavior, removing the local implementation rather than maintaining a second variant.internal/daemon/loop_runtime_adapters_test.go (1)
207-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the positive outcome, not just the absence of
ResolveOrRegister.The subtest discards
applyResolved's result, so it would still pass ifapplyResolveddegraded into a no-op. Assert the resolved workspace identity/policy actually landed on theCreateOpts(e.g.Workspace,SandboxRef, orPermissions) alongside theresolveOrRegisterCalledcheck.As per path instructions, "Verify tests can fail when business logic changes" and "Ensure tests verify behavior outcomes, not just function calls".
🤖 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/daemon/loop_runtime_adapters_test.go` around lines 207 - 235, Update the “Should resolve a registered workspace path without registering it” subtest to retain the CreateOpts passed to policyGate.applyResolved and assert that the registered workspace’s identity or policy fields, such as Workspace, SandboxRef, or Permissions, are applied. Keep the existing resolveOrRegisterCalled assertion to verify registration is not invoked.Source: Path instructions
internal/automation/manager_definition_sync.go (1)
284-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLeftover if/else after dropping the resolution-mode parameter. In both files the two branches now invoke
Resolveidentically; only the ref (normalized path vs. trimmed ref) differs, so the branch can be reduced to computing the ref followed by oneResolvecall.
internal/automation/manager_definition_sync.go#L284-L294: computeref(normalized when path-like, keeping theautomation: resolve config workspace %qwrapping) and callm.workspaceResolver.Resolve(ctx, ref)once.internal/bundles/service_materialize_bridge.go#L106-L116: computeref(normalized when path-like) and calls.workspaceResolver.Resolve(ctx, ref)once.🤖 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/automation/manager_definition_sync.go` around lines 284 - 294, The workspace resolution branches duplicate the resolver call after removal of the resolution-mode parameter. In internal/automation/manager_definition_sync.go lines 284-294, compute a single ref from trimmedRef, normalize path-like references while preserving the existing wrapped error, then call m.workspaceResolver.Resolve(ctx, ref) once; apply the same ref computation and single s.workspaceResolver.Resolve(ctx, ref) call in internal/bundles/service_materialize_bridge.go lines 106-116.internal/cli/workspace_test.go (1)
433-488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where the session lookup itself fails.
These cases cover a successful identity lookup that mismatches, but not the branch where
resolveAgentCallerFromEnvreturns an error whileCOMPOZY_SESSION_ID/COMPOZY_AGENTare set. That branch currently fails open (see the comment oninternal/cli/workspace_resolution.goLines 305-328), and agetSessionFnreturning an error here would pin the intended behavior.🤖 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/workspace_test.go` around lines 433 - 488, Extend the table-driven tests in the workspace identity validation block with a case where the configured session lookup fails, using getSessionFn to return a sentinel error while COMPOZY_SESSION_ID and COMPOZY_AGENT remain set. Assert that executeRootCommand returns that lookup error rather than proceeding or returning success, covering the resolveAgentCallerFromEnv error branch.Source: Path instructions
internal/cli/agent_mutate_requests.go (1)
66-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared workspace-override resolver instead of re-inlining it four times.
authored_context.goalready extracts the exactcommandWorkspaceFlag(cmd)→resolveOptionalWorkspaceOverride(...)sequence intoresolveAuthoredContextWorkspace; these four sites duplicate it verbatim instead of calling a shared (ideally more generically named) helper.
internal/cli/agent_mutate_requests.go#L66-L76: replace the inline two-step resolution increateAgentRequestFromFlagswith a call to the shared helper.internal/cli/agent_mutate_requests.go#L175-L184: replace the inline resolution induplicateAgentRequestFromFlags's else branch with the same shared helper.internal/cli/agent_mutate.go#L66-L79: replace the inline resolution innewAgentUpdateCommandwith the shared helper, and give the two error-wrap messages distinct text so failures in flag parsing vs. override resolution can be told apart.internal/cli/agent_mutate.go#L117-L134: replace the inline resolution innewAgentDeleteCommandwith the shared helper, with the same distinct error-wrap fix.♻️ Suggested direction
-workspaceRef, err := commandWorkspaceFlag(cmd) -if err != nil { - return fmt.Errorf("cli: resolve update workspace: %w", err) -} -workspace, err := resolveOptionalWorkspaceOverride( - cmd.Context(), - cmd, - deps, - client, - workspaceRef, -) -if err != nil { - return fmt.Errorf("cli: resolve update workspace: %w", err) -} +workspace, err := resolveAuthoredContextWorkspace(cmd, deps, client) +if err != nil { + return fmt.Errorf("cli: resolve update workspace: %w", err) +}(move/rename
resolveAuthoredContextWorkspaceto a shared file if it should no longer be authored-context-specific)🤖 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/agent_mutate_requests.go` around lines 66 - 76, Replace the duplicated commandWorkspaceFlag-to-resolveOptionalWorkspaceOverride sequences with one shared, generically named helper based on resolveAuthoredContextWorkspace. In internal/cli/agent_mutate_requests.go#L66-L76 and `#L175-L184`, update createAgentRequestFromFlags and duplicateAgentRequestFromFlags; in internal/cli/agent_mutate.go#L66-L79 and `#L117-L134`, update newAgentUpdateCommand and newAgentDeleteCommand. For the latter two sites, give flag-parsing and workspace-override error wrappers distinct messages.internal/cli/mcp_serve_test.go (1)
13-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd real-function coverage for the workspace-resolution path.
The new subtest only checks that
newMCPServeCommanddispatches into a stubbedrunMCPServe; it doesn't exercise the actualrunMCPServeworkspace-resolution logic (theresolveCommandWorkspace(ctx, nil, ...)call). The other real-function tests bail out on transport validation before reaching that code. Consider adding a test that stubs the daemon/workspace-lookup client and assertsrunMCPServeresolves a workspace via cwd/session/env when--workspaceis omitted.As per path instructions, "MUST test meaningful business logic, not trivial operations" and tests should "verify tests can fail when business logic changes."
🤖 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/mcp_serve_test.go` around lines 13 - 28, Add a real-function test that invokes runMCPServe rather than only the stubbed callback in newMCPServeCommand, with --workspace omitted. Stub the daemon/workspace lookup client and configure cwd, session, or environment inputs so resolveCommandWorkspace receives nil workspace and resolves the expected workspace; assert the resolved value reaches the serve flow, ensuring the test fails if workspace-resolution behavior changes.Source: Path instructions
🤖 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/api/core/agent_identity.go`:
- Around line 135-143: Update the workspace resolution flow around
Workspaces.Get to call the resolver’s Resolve method and use
ResolvedWorkspace.WorkspaceID as the canonical identity before authorization.
Trim and validate that WorkspaceID is non-empty, preserving the existing error
behavior, and add a test covering differing registration and identity IDs.
In `@internal/cli/marketplace.go`:
- Around line 45-49: Move the local --cursor/--kind validation in the
marketplace command ahead of clientFromDeps and readScope.resolve, including the
--scope workspace case so --cursor without --kind returns the argument error
before workspace resolution. Add a test covering workspace scope with --cursor
page-two and no --kind.
In `@internal/cli/session_event_output.go`:
- Around line 140-146: Update the resolveWorkspaceCandidate call in the session
event output flow to attribute --cwd-derived workspace resolution to
workspaceResolutionCWD instead of workspaceResolutionFlag. Keep the existing
discovery fallback behavior and ensure emitted resolution_source values
consistently represent path-derived workspace provenance.
- Around line 147-160: Update the resolution handling around
workspaceRefFromSessionIdentity and validateResolutionAgainstSessionIdentity so
only errors accepted by workspaceResolutionAllowsGlobalFallback permit the
existing raw --cwd fallback. For all other resolution failures, propagate the
error instead of recording workspaceResolutionFlag or returning the unvalidated
cleaned path; retain identity validation for successful resolutions and the
existing benign 404/path-not-registered fallback.
In `@internal/cli/workspace_resolution.go`:
- Around line 305-328: Update workspaceRefFromSessionIdentity to distinguish
missing credentials from failed session validation: preserve the empty identity
result only when SessionID or AgentName is absent, but propagate
resolveAgentCallerFromEnv errors for configured identities. Adjust its return
contract and validateResolutionAgainstSessionIdentity so lookup failures do not
disable workspace scoping or allow caller-supplied workspace references through.
In `@internal/cli/workspace.go`:
- Around line 153-159: Define a dedicated workspace flag-name constant and use
it consistently: update internal/cli/workspace.go lines 153-159 to register the
flag via that constant instead of workspaceSkillSource, and update
internal/cli/workspace_resolution.go lines 175-181 to pass the same constant to
cmd.Flags().Lookup instead of the hardcoded string. Preserve explicit-blank
--workspace detection.
In `@internal/daemon/native_automation_scope.go`:
- Around line 101-136: Replace the workspace-wide enumeration in
nativeAutomationRunForWorkspace with a direct run-by-ID lookup, then validate
that the returned run’s JobID and TriggerID resolve to jobs/triggers owned by
workspaceID; return ErrRunNotFound for absent or foreign runs. Apply the same
point-lookup and ownership-validation approach to
nativeAutomationJobForWorkspace and nativeAutomationTriggerForWorkspace instead
of paging through full workspace listings, while preserving existing behavior
when workspaceID is empty.
- Around line 16-20: The workspace-scoped automation paths fail open when
workspaceID is blank. In internal/daemon/native_automation_scope.go at lines
16-20, 63-65, and 108-110, and
internal/daemon/native_automation_trigger_tools.go at lines 262-276, confirm
workspace-bound callers always populate workspace or return a deterministic
not-found/denied error instead of delegating to unscoped GetJob, GetTrigger,
GetRun, or run-listing operations.
- Around line 43-53: Update the pagination loop in the jobs-listing method
around automationManager().ListJobs to return when HasMore is true but
NextCursor equals the current query.Cursor, preventing repeated requests and
duplicate accumulation. Apply the same cursor-progress guard to the trigger
pagination loop around its corresponding ListJobs flow, while preserving normal
pagination when the cursor advances.
In `@internal/daemon/native_workspace_input_binder.go`:
- Around line 172-186: Update resolveNativeWorkspaceInput to fail closed when b
is nil or b.workspaces is nil instead of calling setNativeWorkspaceInput with
the unresolved ref. Return nativeNetworkInputError(id,
workspacepkg.ErrWorkspaceResolverUnavailable), matching the existing
nativeWorkspaceRoot behavior, while preserving the normal resolver and
resolved-ID path.
- Around line 28-59: Move the global-scope authorization out of the
ToolsetIDAutomation-only guard in BindCallInput and enforce it generically for
every native descriptor that accepts workspace input. Ensure non-operator
callers with a trusted scope cannot submit scope:"global" or "all" without an
explicit workspace, while preserving operator and untrusted-scope behavior and
the existing bindNativeWorkspaceNode flow.
In `@internal/mcp/serve_binding.go`:
- Around line 183-193: Update the scope validation in the workspace-binding flow
before workspaces.Resolve is called: after trimming id, reject an empty id with
the existing scope-conflict error. Preserve the direct workspaceID match and
ensure blank scope.id values never reach Resolve.
In `@internal/workspace/resolver_crud.go`:
- Around line 348-376: Update lookupWorkspaceBySameRoot to detect os.ErrNotExist
from canonicalRoot or os.Stat(canonicalPath) before scanning registered
workspaces, and return the existing ErrWorkspaceNotFound/root-missing outcome
instead of propagating a generic stat error. Preserve the current
registered-workspace scanning behavior for paths that exist.
In `@web/src/systems/tasks/hooks/__tests__/use-task-orchestration-hooks.test.tsx`:
- Around line 269-291: Update the tests for useAgentContext and
useTaskContextBundle to assert that the first getAgentContext call receives the
expected identity object. Keep the existing result assertions and call-count
check, ensuring both hooks verify identity propagation without relying only on
the mock’s resolved value.
---
Outside diff comments:
In `@internal/api/core/agent_identity.go`:
- Around line 28-43: Update StatusForAgentIdentityError to map
workspacepkg.ErrWorkspaceNotFound to the intended client-error status, typically
http.StatusNotFound, instead of the default 500 response. Add or update coverage
for the wrapped unknown-workspace error path around the caller at lines 135-137
and verify it returns 404.
In `@internal/cli/task_list.go`:
- Around line 105-112: The --scope all error path assigns the raw workspaceRef
without resolving it. Update the scope-all handling around
resolveTaskScopeWorkspace to resolve workspaceRef through the existing
workspace-resolution engine, such as resolveCommandWorkspace, and assign its
canonical workspace value to TaskListQuery.Workspace while preserving the
current empty-scope behavior.
In `@internal/daemon/native_create_tools.go`:
- Around line 246-253: Update agentCreate so the catalog entry’s
entry.WorkspaceID is populated from the resolved createReq.Workspace rather than
scope.WorkspaceID, preserving the selected target workspace for operator
requests. Add a test covering an operator targeting a workspace different from
scope.WorkspaceID and assert the created agent stores the requested workspace.
In `@internal/daemon/native_tool_approval_grants_test.go`:
- Around line 119-123: Strengthen the denial assertion in the workspace-override
test by extracting the error with errors.As into *toolspkg.ToolError, then
verify its message or reason identifies the workspace scope mismatch. Keep the
existing errors.Is(err, toolspkg.ErrToolDenied) assertion while ensuring
unrelated denials cannot satisfy the regression.
In `@internal/daemon/notifier_test.go`:
- Around line 250-255: Strengthen the notifier test around fakeHookRuntime by
adding a separate hookCalled flag set inside onToolPreCall, then assert that it
remains false after dispatch. Keep gotSessionID assertions only for validating
payload data; do not use an empty session ID as evidence that the callback was
skipped.
---
Nitpick comments:
In `@internal/automation/manager_definition_sync.go`:
- Around line 284-294: The workspace resolution branches duplicate the resolver
call after removal of the resolution-mode parameter. In
internal/automation/manager_definition_sync.go lines 284-294, compute a single
ref from trimmedRef, normalize path-like references while preserving the
existing wrapped error, then call m.workspaceResolver.Resolve(ctx, ref) once;
apply the same ref computation and single s.workspaceResolver.Resolve(ctx, ref)
call in internal/bundles/service_materialize_bridge.go lines 106-116.
In `@internal/cli/agent_mutate_requests.go`:
- Around line 66-76: Replace the duplicated
commandWorkspaceFlag-to-resolveOptionalWorkspaceOverride sequences with one
shared, generically named helper based on resolveAuthoredContextWorkspace. In
internal/cli/agent_mutate_requests.go#L66-L76 and `#L175-L184`, update
createAgentRequestFromFlags and duplicateAgentRequestFromFlags; in
internal/cli/agent_mutate.go#L66-L79 and `#L117-L134`, update
newAgentUpdateCommand and newAgentDeleteCommand. For the latter two sites, give
flag-parsing and workspace-override error wrappers distinct messages.
In `@internal/cli/client_workspace_authoring.go`:
- Around line 84-115: Replace the duplicated selection logic in
nearestCLIWorkspaceRoute with the existing nearest-enclosing workspace algorithm
from internal/workspace/discovery.go, including its tie-breaking behavior. Reuse
the shared implementation or extract a common helper so CLI and discovery
consistently select the same workspace while preserving path canonicalization
and invalid-entry handling.
In `@internal/cli/mcp_serve_test.go`:
- Around line 13-28: Add a real-function test that invokes runMCPServe rather
than only the stubbed callback in newMCPServeCommand, with --workspace omitted.
Stub the daemon/workspace lookup client and configure cwd, session, or
environment inputs so resolveCommandWorkspace receives nil workspace and
resolves the expected workspace; assert the resolved value reaches the serve
flow, ensuring the test fails if workspace-resolution behavior changes.
In `@internal/cli/workspace_test.go`:
- Around line 433-488: Extend the table-driven tests in the workspace identity
validation block with a case where the configured session lookup fails, using
getSessionFn to return a sentinel error while COMPOZY_SESSION_ID and
COMPOZY_AGENT remain set. Assert that executeRootCommand returns that lookup
error rather than proceeding or returning success, covering the
resolveAgentCallerFromEnv error branch.
In `@internal/daemon/loop_runtime_adapters_test.go`:
- Around line 207-235: Update the “Should resolve a registered workspace path
without registering it” subtest to retain the CreateOpts passed to
policyGate.applyResolved and assert that the registered workspace’s identity or
policy fields, such as Workspace, SandboxRef, or Permissions, are applied. Keep
the existing resolveOrRegisterCalled assertion to verify registration is not
invoked.
In `@internal/daemon/native_automation_tool_query.go`:
- Around line 13-33: Remove the duplicated JobID, TriggerID, and WorkspaceID
fields between automationJobHistoryInput, automationTriggerHistoryInput, and
automationRunQueryInput so JSON decoding cannot shadow embedded values. Update
the affected handlers and promoted query() usage to read the retained
automationRunQueryInput fields, while preserving each input’s required
identifier behavior.
In `@internal/daemon/native_automation_tools_test.go`:
- Around line 481-516: Replace t.Fatalf calls in the ListJobsFn, GetJobFn, and
ListRunsFn stubs with t.Errorf followed by returning an appropriate sentinel
error, preserving the existing successful return values otherwise. Apply the
same goroutine-safe callback behavior to the related GetTriggerFn and GetRunFn
stubs if they use the same assertion pattern, so failures propagate to the
registry call site.
In `@internal/daemon/native_tool_workspace_memory_calls.go`:
- Around line 30-42: Update the Resolve error handling in the scoped branch of
native workspace listing to pass the resolver failure through
nativeNetworkInputError before returning it. Preserve the existing successful
resolution and List error behavior so workspace_list returns a typed ToolError
with the expected code and reason for resolution failures.
In `@internal/extension/host_api_automation_binding.go`:
- Around line 25-45: Replace the paginated single-ID lookup loop with a direct
GetJob/GetTrigger call in the bound automation lookup helpers, then compare the
returned resource’s workspace ID with the bound workspace ID. Convert lookup
errors or workspace mismatches to the existing not-found behavior, preserving
the hidden-resource semantics and avoiding repeated full workspace scans from
requireBoundAutomationRunFilter.
In `@internal/extension/host_api_test.go`:
- Line 256: Update the inner table-driven subtest launched by t.Run in the host
API tests to call t.Parallel() at the start of its callback. Preserve the
existing test cases and enclosing-suite behavior while ensuring each read-only
or expected-failure case uses the required parallel execution pattern.
In `@internal/extension/host_api_workspace_binding.go`:
- Around line 28-40: Remove the redundant hostAPIWorkspaceBinding type alias and
the lowercase constant aliases in the alias block, including
hostAPIWorkspaceScopeKindKey. Update any references to use the original exported
HostAPIWorkspaceBinding type and constants directly within the package.
- Around line 224-319: Centralize workspace-binding behavior by exporting a
resolver-parameterized binder from
internal/extension/host_api_workspace_binding.go, covering reference
canonicalization, scope-literal binding, and resource-scope validation while
preserving RPC-error wrapping in the handler layer. Update
internal/mcp/serve_binding.go to replace bindWorkspaceString, bindString,
bindResourceScope, and validateResourceScope with calls to the shared binder;
both sites must use identical blank scope.id handling and resolver behavior.
In `@internal/workspace/discovery.go`:
- Around line 12-59: Replace the duplicated nearest-enclosing workspace
selection logic in Resolver.lookupWorkspaceByEnclosingRoot with the consolidated
shared algorithm used by the CLI workspace-authoring flow. Ensure both callers
use the same root matching, depth comparison, and tie-breaking behavior,
removing the local implementation rather than maintaining a second variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/daemon/native_automation_scope.go (1)
118-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePoint lookup with ownership validation resolves the previous enumeration concern.
hasOwnedTargetreads as "already validated" while it is set before the ownership check runs; the control flow is correct (any failed check returns early), but a name likehasTargetwould match the semantics. Fail-closed behavior for runs without job/trigger targets is right.🤖 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/daemon/native_automation_scope.go` around lines 118 - 147, Rename hasOwnedTarget in the run lookup flow to hasTarget, since it indicates that a job or trigger target exists before ownership validation completes. Preserve the existing early returns for failed workspace ownership checks and the fail-closed ErrRunNotFound behavior when no target is present.
🤖 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/mcp_serve_test.go`:
- Around line 101-135: Update the test’s getWorkspaceFn stub to record the
received ref instead of calling t.Fatalf inside the callback, since it may
execute on a worker goroutine. After runMCPServe returns, assert the recorded
ref equals "/workspace/project" from the test goroutine, while preserving the
existing canonical workspace assertion.
In `@internal/extension/host_api_automation_binding.go`:
- Around line 25-33: Update the bound job lookup helper around automation.GetJob
to normalize only automationpkg.ErrJobNotFound, returning the original error for
cancellations and other manager failures; apply the same treatment in the
trigger lookup helper around automation.GetTrigger for
automationpkg.ErrTriggerNotFound. Both affected sites are
internal/extension/host_api_automation_binding.go lines 25-33 and 50-58.
---
Nitpick comments:
In `@internal/daemon/native_automation_scope.go`:
- Around line 118-147: Rename hasOwnedTarget in the run lookup flow to
hasTarget, since it indicates that a job or trigger target exists before
ownership validation completes. Preserve the existing early returns for failed
workspace ownership checks and the fail-closed ErrRunNotFound behavior when no
target is present.
🪄 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 Plus
Run ID: 204f2c4b-0f42-4e1d-b8c4-f7213b27b695
⛔ Files ignored due to path filters (2)
skills/compozy/references/native-tools.mdis excluded by!**/*.mdskills/compozy/references/runtime-operations.mdis excluded by!**/*.md
📒 Files selected for processing (44)
internal/agentidentity/identity.gointernal/api/core/agent_identity.gointernal/api/core/tasks_internal_test.gointernal/automation/manager_definition_sync.gointernal/bundles/service_materialize_bridge.gointernal/cli/agent_mutate.gointernal/cli/agent_mutate_requests.gointernal/cli/authored_context.gointernal/cli/authored_context_heartbeat_commands.gointernal/cli/client_workspace_authoring.gointernal/cli/loop_test.gointernal/cli/marketplace.gointernal/cli/marketplace_test.gointernal/cli/mcp_serve_test.gointernal/cli/session_event_output.gointernal/cli/session_test.gointernal/cli/skill_workspace.gointernal/cli/task_list.gointernal/cli/task_test.gointernal/cli/workspace.gointernal/cli/workspace_resolution.gointernal/cli/workspace_test.gointernal/daemon/loop_runtime_adapters_test.gointernal/daemon/native_automation_scope.gointernal/daemon/native_automation_tool_query.gointernal/daemon/native_automation_tools_test.gointernal/daemon/native_create_tools.gointernal/daemon/native_create_tools_test.gointernal/daemon/native_loop_tools_test.gointernal/daemon/native_tool_approval_grants_test.gointernal/daemon/native_tool_workspace_memory_calls.gointernal/daemon/native_tools_test.gointernal/daemon/native_workspace_input_binder.gointernal/daemon/notifier_test.gointernal/extension/host_api.gointernal/extension/host_api_automation_binding.gointernal/extension/host_api_test.gointernal/extension/host_api_workspace_binding.gointernal/mcp/serve_binding.gointernal/mcp/serve_test.gointernal/workspace/discovery.gointernal/workspace/resolver_crud.gointernal/workspace/resolver_test.goweb/src/systems/tasks/hooks/__tests__/use-task-orchestration-hooks.test.tsx
💤 Files with no reviewable changes (1)
- internal/daemon/native_automation_tool_query.go
🚧 Files skipped from review as they are similar to previous changes (24)
- internal/daemon/native_loop_tools_test.go
- internal/daemon/native_create_tools.go
- internal/daemon/native_tool_approval_grants_test.go
- web/src/systems/tasks/hooks/tests/use-task-orchestration-hooks.test.tsx
- internal/api/core/agent_identity.go
- internal/workspace/discovery.go
- internal/bundles/service_materialize_bridge.go
- internal/workspace/resolver_crud.go
- internal/daemon/native_tool_workspace_memory_calls.go
- internal/extension/host_api_test.go
- internal/daemon/notifier_test.go
- internal/cli/skill_workspace.go
- internal/daemon/loop_runtime_adapters_test.go
- internal/cli/session_event_output.go
- internal/automation/manager_definition_sync.go
- internal/cli/marketplace.go
- internal/mcp/serve_test.go
- internal/workspace/resolver_test.go
- internal/cli/workspace_test.go
- internal/daemon/native_workspace_input_binder.go
- internal/extension/host_api_workspace_binding.go
- internal/cli/workspace_resolution.go
- internal/cli/session_test.go
- internal/daemon/native_tools_test.go
pedronauck
force-pushed
the
fix-workspace
branch
from
July 28, 2026 17:25
be77bef to
6f42a8f
Compare
Merged
This was referenced Jul 29, 2026
Merged
Merged
Merged
This was referenced Aug 4, 2026
Merged
Merged
This was referenced Aug 5, 2026
Merged
This was referenced Aug 9, 2026
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 13, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 20, 2026
Merged
Merged
Merged
Merged
This was referenced Sep 10, 2026
Merged
Merged
Merged
Merged
This was referenced Sep 27, 2026
Merged
Merged
This was referenced Oct 6, 2026
Merged
Merged
Merged
This branch was successfully deployed
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
1 participant
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 by CodeRabbit
New Features
resolution_source).Breaking Changes
workspaceinstead ofworkspace_id.