feat: add permission-aware cross-workspace access - #275

internal/agentidentity/identity.go (1)

172-177: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Preserve the policy error instead of swallowing it.

policy.Authorize failures (store/transport errors, cancelled context) are indistinguishable from real denials here, and the returned hint tells the operator to change permissions.mode even when the cause was infrastructural. Join the underlying error so callers and logs keep the root cause while the deny decision stays fail-closed.

♻️ Suggested change

-	if policy != nil {
-		decision, err := policy.Authorize(ctx, req)
-		if err == nil && decision.Allowed {
-			return nil
-		}
-	}
-	return identityError(
-		ErrIdentityUnauthorized,
-		"identity_unauthorized",
-		"agent session does not belong to the requested workspace",
-		workspaceaccess.DenialHint,
-	)
+	var authorizeErr error
+	if policy != nil {
+		decision, err := policy.Authorize(ctx, req)
+		if err == nil && decision.Allowed {
+			return nil
+		}
+		authorizeErr = err
+	}
+	return identityError(
+		errors.Join(ErrIdentityUnauthorized, authorizeErr),
+		"identity_unauthorized",
+		"agent session does not belong to the requested workspace",
+		workspaceaccess.DenialHint,
+	)

Note: errors.Join(err, nil) returns just err, so the allowed path and the plain-denial path stay unchanged.

🤖 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/agentidentity/identity.go` around lines 172 - 177, Update the policy
authorization handling around policy.Authorize in the identity flow to preserve
any non-nil authorization error while still returning a fail-closed denial. Join
the underlying error with the existing denial error, ensuring allowed requests
and plain policy denials retain their current behavior.

Source: Coding guidelines

internal/daemon/daemon_mock_agents_integration_test.go (1)

1438-1457: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

requireLatestHostedMCPStdioServer duplicates requireHostedMCPStdioServer except for iteration order.

Consider a single helper with a direction flag (or one that always returns the latest, if earliest-match had no semantic requirement) to avoid the two copies drifting.

🤖 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/daemon_mock_agents_integration_test.go` around lines 1438 -
1457, Consolidate requireLatestHostedMCPStdioServer with the existing
requireHostedMCPStdioServer helper instead of maintaining duplicate MCP
server-search logic. Add a direction or latest-selection option only if the
existing helper must preserve earliest-match behavior, while retaining the
current failure message and latest-session semantics.

internal/task/task_resource_authorizer.go (1)

59-79: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Policy authorization errors are silently swallowed.

taskWorkspaceAccessAllowed discards the error from policy.Authorize and only returns a boolean. Fail-closed-on-error is the right default for security, but the actual error (e.g., a misconfigured or unreachable policy backend) is never logged, making it indistinguishable from a legitimate deny when debugging denied cross-workspace task access.

As per path instructions for internal/**/*.go, error handling and logging conventions should be followed when editing production Go under internal/.

♻️ Suggested logging addition

 func taskWorkspaceAccessAllowed(
 	ctx context.Context,
 	policy workspaceaccess.Policy,
 	actor ActorContext,
 	targetWorkspaceID string,
 ) bool {
 	if policy == nil {
 		return false
 	}
 	decision, err := policy.Authorize(ctx, workspaceaccess.Request{
 		Actor: workspaceaccess.ActorRef{
 			Kind:        workspaceaccess.ActorKind(actor.Actor.Kind.Normalize()),
 			SessionID:   strings.TrimSpace(actor.Scope.SessionID),
 			WorkspaceID: strings.TrimSpace(actor.Scope.WorkspaceID),
 			Operator:    actor.Scope.Operator,
 		},
 		TargetWorkspaceID: strings.TrimSpace(targetWorkspaceID),
 		Seam:              workspaceaccess.SeamTask,
 	})
+	if err != nil {
+		logger.FromContext(ctx).Warn("workspace access policy authorization failed", "error", err)
+		return false
+	}
 	return err == nil && decision.Allowed
 }

🤖 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/task/task_resource_authorizer.go` around lines 59 - 79, Update
taskWorkspaceAccessAllowed to log the error returned by policy.Authorize before
preserving the existing fail-closed behavior of returning false. Follow the
repository’s established error-logging convention for internal Go code and keep
legitimate denied decisions returning false without logging an error.

Source: Path instructions

web/src/systems/session/lib/session-owner-query.ts (1)

14-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Query key/options factory not co-located in lib/query-options.ts.

sessionOwnerKeys/sessionOwnerOptions are defined in a new session-owner-query.ts file rather than in lib/query-options.ts. The in-code rationale (keeping the owner projection out of sessionKeys per ADR-004 to avoid workspace-cache leakage) is sound, but the same isolation could be achieved by exporting a separate key/options factory from lib/query-options.ts itself, satisfying the co-location guideline while preserving the namespace separation.

As per coding guidelines, "Co-locate query keys and query functions in lib/query-options.ts; reuse option/key factories... and reconcile mutations through canonical keys."

🤖 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 `@web/src/systems/session/lib/session-owner-query.ts` around lines 14 - 35, The
session-owner query keys and options are in a separate session-owner-query
module instead of the shared query-options module. Move the sessionOwnerKeys,
SessionOwnerDialogState, and sessionOwnerOptions definitions into
lib/query-options.ts, preserving their separate namespace from sessionKeys and
keeping sessionOwnerOptions’ query key, fetchSessionOwner query function, stale
time, and enabled behavior unchanged; update imports and usages accordingly.

Source: Coding guidelines

internal/daemon/native_workspace_input_binder.go (1)

248-250: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Preserve the policy error cause in the denial.

err from Authorize is dropped and replaced with a generic denial, so infrastructure failures (mode source unavailable, audit store down) become indistinguishable from genuine policy denials during triage.

♻️ Wrap the cause

 	if err != nil {
-		return nativeWorkspaceAccessDeniedError(descriptor.ID)
+		return fmt.Errorf("%w: authorize workspace %q: %w",
+			nativeWorkspaceAccessDeniedError(descriptor.ID), targetWorkspaceID, err)
 	}

🤖 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_workspace_input_binder.go` around lines 248 - 250,
Update the Authorize error path in the native workspace input binding flow to
preserve err when constructing the denial, wrapping the original cause while
retaining the nativeWorkspaceAccessDeniedError context. Ensure infrastructure
failures remain distinguishable from genuine policy denials and keep the
existing successful authorization path unchanged.

internal/daemon/native_tools_test.go (1)

1095-1103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

These assertions no longer test authorization; the messages claim otherwise.

adapter is built with only Workspaces — no binder, no policy — so resolving ws-2 from a ws-1 scope succeeds simply because nativeCallerWorkspaceInput no longer rejects foreign refs. The failure messages ("want binder-authorized ws-2") suggest a binder authorized the call, which is misleading for future readers. Consider renaming to reflect that the raw adapter defers authorization to the registry seam.

♻️ Clarify intent

 		if err != nil || foreignResolved.WorkspaceID != "ws-2" {
-			t.Fatalf("nativeResolvedWorkspace(foreign) = %#v, %v, want binder-authorized ws-2", foreignResolved, err)
+			t.Fatalf(
+				"nativeResolvedWorkspace(foreign) = %#v, %v, want ws-2 resolved (authorization deferred to the registry binder)",
+				foreignResolved,
+				err,
+			)
 		}

Also applies to: 1146-1153

🤖 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_tools_test.go` around lines 1095 - 1103, Rename the
assertions around adapter.nativeResolvedWorkspace in the affected test cases to
reflect raw adapter resolution and registry-seam authorization, not binder
authorization. Update the failure messages to remove “binder-authorized”
language while preserving the expected ws-2 result and existing assertion
behavior.

internal/daemon/workspace_access_audit.go (1)

67-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Detached audit write has no deadline and runs synchronously on the authorization path.

context.WithoutCancel(ctx) correctly prevents request cancellation from dropping the ledger append, but it also removes the caller's deadline. Since DefaultPolicy.Authorize emits synchronously (internal/workspaceaccess/default_policy.go Lines 43-61), a wedged or lock-contended event store blocks every workspace-access decision indefinitely. Reattach a bounded deadline.

🛡️ Reattach a deadline

-	if err := e.store.WriteEventSummary(context.WithoutCancel(ctx), store.EventSummary{
+	writeCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), workspaceAccessAuditWriteTimeout)
+	defer cancel()
+	if err := e.store.WriteEventSummary(writeCtx, store.EventSummary{

Plus a package-level constant:

const workspaceAccessAuditWriteTimeout = 5 * time.Second

As per coding guidelines, "Work that outlives an HTTP/UDS request must use context.WithoutCancel(ctx) ...; reattach a deadline when needed."

🤖 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/workspace_access_audit.go` at line 67, Update the audit write
in the surrounding workspace access audit function to derive a context with
WithoutCancel(ctx) and apply a five-second timeout using the package-level
workspaceAccessAuditWriteTimeout constant before calling WriteEventSummary.
Preserve the detached context behavior while ensuring the synchronous
authorization path cannot block indefinitely, and release the derived context
when the write completes.

Source: Coding guidelines

internal/daemon/workspace_access_prompt_bridge.go (1)

107-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Underlying outcome.Validate() error is discarded from the returned message.

The error from outcome.Validate() is checked but its details are dropped in favor of a generic message. Wrapping it would aid debugging malformed ACP outcomes.

♻️ Optional improvement

-	if err := outcome.Validate(); err != nil || outcome.Selected == nil {
-		return false, errors.New("daemon: workspace access prompt returned no selected outcome")
-	}
+	if err := outcome.Validate(); err != nil {
+		return false, fmt.Errorf("daemon: workspace access prompt outcome invalid: %w", err)
+	}
+	if outcome.Selected == nil {
+		return false, errors.New("daemon: workspace access prompt returned no selected outcome")
+	}

🤖 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/workspace_access_prompt_bridge.go` around lines 107 - 109,
Update the validation branch in the workspace access prompt bridge to preserve
and wrap the error returned by outcome.Validate() when validation fails, while
retaining the existing generic error for a nil outcome.Selected case. Ensure
malformed ACP outcome details are included in the returned error message.

web/src/systems/os/components/os-route-hold.tsx (1)

12-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move route coordination out of the component layer.

OsRouteHold is route orchestration, not a presentational component. Move this effect into the decision route or a route-level hook.

As per coding guidelines, “UI components must be pure and presentational; orchestration and data fetching belong at route/page level.”

🤖 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 `@web/src/systems/os/components/os-route-hold.tsx` around lines 12 - 20, Move
the route-hold orchestration from OsRouteHold into the decision route or an
equivalent route-level hook, including the useLocation values and coordinator
hold/release effect. Remove the component-layer effect and ensure the route
still holds on mount or route changes and releases on cleanup.

Source: Coding guidelines

web/src/routes/_app/__tests__/-agent-session-deeplink.router.integration.test.tsx (2)

240-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Harness components are declared after builder's return.

Hoisting makes this valid, but the returned object followed by unreachable-looking declarations is easy to misread. Moving the harness components above the return (or to module scope, parameterized by the route handles) would read better.

🤖 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
`@web/src/routes/_app/__tests__/-agent-session-deeplink.router.integration.test.tsx`
around lines 240 - 256, Move the AgentsFleetRouteHarness and
AgentDetailRouteHarness declarations above the builder’s return statement,
keeping their existing behavior and route-handle references unchanged.

---

306-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse the exported owner/context types instead of restating them inline.

{ permalinkError?: string; sessionOwner?: { sessionId; workspaceId; workspaceName } } duplicates SessionPermalinkRouteContext / SessionOwnerDialogState. Importing those types keeps the harness failing (at typecheck) if the production projection changes shape.

♻️ Suggested change

-    const context = permalinkRoute.useRouteContext() as {
-      permalinkError?: string;
-      sessionOwner?: { sessionId: string; workspaceId: string; workspaceName: string };
-    };
+    const context = permalinkRoute.useRouteContext() as Pick<
+      SessionPermalinkRouteContext,
+      "permalinkError" | "sessionOwner"
+    >;

🤖 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
`@web/src/routes/_app/__tests__/-agent-session-deeplink.router.integration.test.tsx`
around lines 306 - 309, Update the route-context typing near
permalinkRoute.useRouteContext to import and reuse the exported
SessionPermalinkRouteContext and SessionOwnerDialogState types instead of
duplicating the permalinkError and sessionOwner structure inline. Keep the test
context aligned with the production route projection so type changes are
detected automatically.

web/src/routes/_app/session.$id.tsx (1)

40-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Dialog wiring duplicated with the session leaf route.

This OsRouteHold + SessionWorkspaceSwitchDialog + confirm/cancel navigation block is byte-for-byte the same shape as web/src/routes/_app/agents.$name.sessions.$id.tsx (lines 59-80) apart from the to target. A small composite (owner + re-enter target) in @/systems/session would keep both seams from diverging as the confirmation flow evolves.

🤖 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 `@web/src/routes/_app/session`.$id.tsx around lines 40 - 56, The session
workspace-switch confirmation UI is duplicated between the session route and
agent session route. Extract a shared composite in the session system that
accepts the owner and re-entry navigation target, encapsulates OsRouteHold,
SessionWorkspaceSwitchDialog, and confirm/cancel navigation, then replace both
route-local blocks with that component while preserving each route’s existing
target.

internal/session/spawn_test.go (1)

96-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider moving this subtest out of TestValidatePermissionSubset.

The scenario exercises Manager.Spawn cross-workspace authorization (policy invocation + workspace binding + per-workspace cap), not ValidatePermissionSubset. Hosting it in a dedicated top-level test (e.g. TestSpawnWorkspaceAccess) keeps the invariant and owning suite aligned and makes failures easier to attribute.

Also, policy.calls != 2 pins an internal prepareSpawn detail; asserting the recorded requests plus a non-zero call count would be less brittle.

As per path instructions, "Before creating tests, identify the invariant, owning layer, and canonical suite; extend the existing suite rather than creating duplicate standalone regressions."

🤖 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/session/spawn_test.go` around lines 96 - 157, Move the
cross-workspace scenario out of TestValidatePermissionSubset into the dedicated
top-level Manager.Spawn workspace-access suite, such as
TestSpawnWorkspaceAccess, preserving assertions for authorization, workspace
binding, and per-workspace limits. Replace the exact policy.calls == 2 assertion
with a non-zero call-count check while retaining validation of every recorded
request against wantRequest.

Source: Path instructions

web/src/routes/_app/-agent-session-route-loader.ts (1)

53-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Foreign-session resolution rule implemented twice. Both deep-link seams re-implement the same policy — cached foreign owner check, active-workspace detail miss, owner projection lookup, compare owner workspace against the active one — differing only in how a miss is signalled. One shared helper in @/systems/session keeps the rule (and its "owner projection only, never foreign detail before confirm" invariant) in a single place.

• web/src/routes/_app/-agent-session-route-loader.ts#L53-L69: replace resolveForeignSession with the shared helper, mapping a miss to { status: "not-found" }.
• web/src/routes/_app/-session-permalink-route.ts#L117-L126: replace resolveForeignPermalink with the same helper, throwing SessionNotFoundError on a miss.

🤖 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 `@web/src/routes/_app/-agent-session-route-loader.ts` around lines 53 - 69,
Replace resolveForeignSession in
web/src/routes/_app/-agent-session-route-loader.ts lines 53-69 with the shared
`@/systems/session` helper, mapping a miss to { status: "not-found" }. Replace
resolveForeignPermalink in web/src/routes/_app/-session-permalink-route.ts lines
117-126 with the same helper, throwing SessionNotFoundError on a miss; preserve
the owner-projection-only invariant.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes