feat(permission-management): add scoped permission management

[ewen-poch]

Problem

Permission approvals were tied to ACP-specific tool names and runtime lifetimes. The app could not consistently remember, explain, or revoke approvals across Claude Code, Codex, and OpenCode, and Connector policy could conflict with per-call approvals.

Proposed change

• Add stable Broker identities for persistable customize mutations, MCP tools, execution, file operations, and skill operations. Reserve the builtin_tool namespace while keeping provider built-ins Once-only until an explicit cross-framework mapping exists.
• Persist allow grants in one PermissionGrant table with Global, Project, and Session scopes, qualifier matching, revision checks, cascade cleanup, and runtime reconciliation.
• Normalize ACP permission requests so remembered grants are resolved centrally and downstream ACPs receive a one-call allow decision. A remembered approval is not released until persistence succeeds; failure cancels the provider call and surfaces an error.
• Coordinate Main Agent Connector Allow, Require approval, and Block policy with scoped grants without bypassing Connector restrictions.
• Add broad-scope confirmation for Project and Global approvals across ACP, Connector, and Compute surfaces.
• Add Settings > Permissions filtering, per-row and family revoke, monotonic optimistic updates, visible rollback errors, and an app-root stack of independently operable Undo actions.
• Keep notebook control RPC session-bound while allowing valid session MCP calls, silently accept only provenance-bound OpenCode native Skill loading, and collapse legacy Artifact version aliases in conversation history.
• Remember only the content-independent bare command git status by a redacted digest. Path-qualified executables, provider built-ins, interpreters, scripts, test runners, commands with unproven syntax, and secret-bearing inputs remain Once-only.
• Serialize Registry mutations across database commits and cache publication, and reacquire the shared Prisma client after exclusive migration drains. Project deletion keeps the existing retryable pre-delete prune and adds a queued, non-failing post-FK cache/Undo barrier.
• Revoke custom MCP grants before persisting security-sensitive transport, endpoint, executable, argument, environment, or header changes; invalidate pending approvals across the configuration generation barrier; description-only edits retain grants.
• Serialize Compute host create/delete/grant cleanup, deny pending approvals on deletion, and revalidate the immutable ComputeHost row id before durable auto-allow or remembering a decision, so a recreated provider id cannot inherit stale authority.

Permission flow

flowchart LR
  Request["ACP / Connector / Compute request"] --> Identity["Broker stable identity"]
  Identity --> Policy{"Connector policy applies?"}
  Policy -->|"Block"| Deny["Deny"]
  Policy -->|"Allow"| Once["Send downstream allow-once"]
  Policy -->|"Require approval / ACP"| Registry{"Matching durable grant?"}
  Registry -->|"Yes"| Once
  Registry -->|"No"| Prompt["User approval"]
  Prompt -->|"Once"| Once
  Prompt -->|"Session / Project / Global"| Confirm["Broad-scope confirmation when required"]
  Confirm --> Persist["PermissionGrant registry"]
  Persist -->|"Commit succeeds"| Once
  Persist -->|"Commit fails"| Deny
  Persist --> Settings["Settings → Permissions"]
  Settings -->|"Revoke"| Receipt["Short-lived Undo receipt"]
  Receipt -->|"Undo"| Persist

Loading

Scope and non-goals

• Stored records represent remembered allow grants; ACP-specific once semantics remain an adapter concern.
• Session grants are durable database records and are removed with their owning project or session lifecycle. A live same-project ACP session is accepted during the short first-save window, then startup reconciliation remains the crash-recovery authority.
• For Main Agent connector calls, Connector policy remains the upper bound: Block still wins, while Require approval consults Broker grants. Specialist connector access remains governed by its independently configured capability policy.
• Connectors currently execute through repl_execute -> host.mcp, where runner and nested Connector authorization are intentionally distinct. A single-use call permit is deferred until a Connector is directly exposed through ACP; no current direct path can double-prompt.
• Provider built-in tools, including WebFetch and WebSearch, remain Once-only in V1 rather than receiving guessed cross-framework identities.
• No new permission data is added to settings.json. Legacy computeGrants is read only for retry-safe migration and is cleared only after every source row imports successfully; partial or stale-target batches retain the complete legacy source.
• Project deletion preserves the fix(projects): report project deletion failures #542 interaction and durable-intent contract: pre-delete PermissionGrant cleanup failures retain the Project and intent for retry; the post-delete Registry barrier cannot turn a committed hard delete into a false UI failure.
• The Permissions filter remains outside the worklist scroll boundary by product design; changing to whole-panel scrolling would make the scope control disappear in long lists.
• The Artifact alias repair remains in this PR by explicit product request. It is an independent, idempotent data repair with write-back failure/retry coverage and does not recompute or delete historical messages, Artifact content, or native versions.
• No design document is included in Git.

Acceptance criteria and validation

The checks below ran after the final material edit against commit 7c58c27, based on main@a6ab4fc.

Behavior	Project-owned check	Final result
Broker persistence is fail-closed and errors reach the active Session; cancellation also wins while grant lookup is pending	npm test -- --run src/main/acp/permission-broker-registry.test.ts src/main/acp/permission-broker.test.ts src/renderer/src/lib/acp/useAcpRuntime.test.ts	Passed, including cancellation during deferred durable-grant resolution
Exact-command safety admits only bare git status; path-qualified executables and mutable scripts remain Once-only	npx vitest run src/main/permission-grants/capability.test.ts	Passed
OpenCode Skill silent approval requires the preceding native tool event and respects explicit non-Skill metadata	npx vitest run src/main/acp/runtime.test.ts -t "does not use the OpenCode Skill fallback when tool metadata names a different tool"	1 passed; 273 skipped
Registry database/cache mutations are serialized, remain consistent under forced interleavings, and rebind after an exclusive DB disconnect	npx vitest run src/main/permission-grants/registry.test.ts	Passed, including deterministic commit-delay races and retired-client rejection
Project deletion retains retry semantics and removes grants created around the FK-cascade window	npx vitest run src/main/projects/deletion-coordinator.test.ts src/main/permission-grants/registry.test.ts	Passed, including hard-delete finalization and false-failure coverage
Undo actions use the shared ScrollArea, pause auto-dismiss on interaction, and remain independently operable	npx vitest run src/renderer/src/components/PermissionUndoSnackbar.test.tsx	Passed
Custom MCP endpoint changes invalidate prior and pending authority before the new configuration is persisted	npm test -- --run src/main/connectors/service.test.ts src/main/settings/service.connectors.test.ts src/main/settings/ipc.test.ts	Passed, including generation barriers, display-only retention, persistence rollback, and cleanup-failure rollback
Compute host lifecycle is serialized; deletion invalidates pending approvals and provider-id reuse cannot restore grants	npm test -- --run src/main/compute/compute-approval-broker.test.ts src/main/compute/ipc.test.ts	Passed, including deferred cleanup, replacement-host auto-allow denial, immutable owner-id revalidation, and pre-delete pending denial
Web renderer permissions RPC mapping remains generated and complete	npm run check:web-api-map plus permission IPC registry tests	Passed; list/revoke/restore are registered with the runtime Web RPC router
Node and renderer contracts	npm run typecheck	Passed
Changed permission/deletion/renderer files	npx eslint <changed files>	Passed with 0 errors
Full repository regression with loopback access	npm test	Post-rebase local run reproduced only the same 3 failures in untouched spreadsheet-cache-policy.test.ts; GitHub Verify passed on macOS, Ubuntu, and Windows

Uncovered/accepted risks:

• Reconciliation intentionally does not prune unknown non-UUID catalog Connector ids because it cannot distinguish a retired id from an id introduced by a newer app version. Explicit deletion and UUID custom-server cleanup still prune their grants.
• A direct ACP Connector call permit is intentionally not implemented until such a call path exists; nested host.mcp calls continue to require their own authorization.
• The spreadsheet cache-policy failures reproduce outside this permission work and remain outside this PR.
• Renderer checks cover DOM behavior and semantic styling tokens; this follow-up did not add screenshot-diff coverage.
• The Artifact alias repair increases the review surface beyond permission management but remains here by explicit product request.

Review focus

• Broker identity normalization and precedence among Global, Project, Session, and Connector policy.
• PermissionGrant fingerprint, mutation serialization, cleanup, and Undo conflict handling.
• Session-bound notebook RPC authorization, deletion teardown, and ACP runtime rotation.
• Settings revoke/Undo interactions, monotonic renderer snapshots, and broad-scope confirmation boundaries.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Do not connect custom MCP servers before approval

src/main/connectors/service.ts:221

Impact: An Ask-policy custom server is connected and its tools/list request runs before the user approves. Remote servers receive configured headers, and stdio servers can execute their command, even when the subsequent approval is denied.

Recommendation: Run the authorization gate before listTools; only discover tools and invoke the server after approval or a matching durable grant.

[P2] Skip stale legacy grants during migration

src/main/compute/permission-grant-adapter.ts:50

Impact: Legacy settings can retain compute grants for projects deleted before this migration. registry.remember rejects those targets, leaving the shared migration promise rejected; every contextual agent compute request then fails before showing an approval prompt.

Recommendation: Treat missing-project legacy entries as stale and skip or remove them while continuing valid imports; do not let one invalid grant permanently reject the migration.

Summary: Static inspection found two merge-blocking regressions in approval gating and legacy compute-grant migration.

[ewen-poch]

Addressed both findings in e0eb8ed.

• P1 — Custom MCP approval now runs before tools/list, so denying an Ask-policy request neither connects a remote transport nor starts a stdio process. Broad decisions use a two-phase path: the approved server is connected only after authorization, and the durable grant is written only after the requested method is verified in the discovered tool catalog. This also prevents unknown methods from leaving grants behind. Regression coverage asserts the denial path and the successful order: approval → listTools → remember → call.
• P2 — The Registry now exposes a typed PermissionGrantTargetUnavailableError. Legacy Compute migration skips only that explicit stale-target condition and continues importing valid entries; database and other write failures still fail closed and preserve the legacy source for retry. Regression coverage verifies stale-plus-valid migration, non-target failures, and the concrete Registry error contract.

Validation: 41 focused tests pass, typecheck passes, and ESLint passes for all changed files. The full suite has 8,693 passing tests; only the existing spreadsheet cache-policy failures remain.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Revoke session RPC capabilities when their owner is deleted

src/main/notebook/local-rpc-server.ts:319

Impact: Session bearer tokens remain accepted for mcpCall and computeCall after session deletion or failed session creation because capabilities are only removed on server shutdown or token rotation. A stale notebook or agent process can therefore continue invoking operations under the deleted session.

Recommendation: Add explicit session-capability release, including alias cleanup, and invoke it when ACP session creation fails and when sessions are deleted.

Summary: Static inspection found one lifecycle authorization defect in the new session-bound notebook RPC capability flow. Branch and title prechecks are valid.

[ewen-poch]

Addressed the session RPC capability lifecycle finding in 943a044.

• Added explicit per-session capability release in NotebookLocalRpcServer. It revokes Agent-facing and control-plane bearer tokens, follows the pre-start alias to the final ACP session, and removes the associated aliases and Specialist binding.
• ACP now releases the provisional capability when session creation fails and releases the final capability for both attached and detached session deletion. Cleanup is best-effort so it cannot mask the original lifecycle error.
• Added regression coverage proving a released bearer token is rejected without connector dispatch, alias state is removed, and both ACP failure/deletion paths invoke release.
• Rebased onto main at 491be08 while preserving the new session-artifact file resolver and permission-grant owner reconciliation.

Validation: 325 focused tests pass across the RPC, ACP, Connector, Compute migration, and Permission Registry suites; typecheck and ESLint pass.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Refresh the registry client after database disconnect

src/main/permission-grants/registry.ts:232

Impact: Storage migration calls disconnectProjectDbClient() and returns to the running app after cancellation, failure, or discarding a staged copy. The registry still uses this disconnected client for remember, revoke, restore, and prune, so new durable approvals and permission-management actions fail until the app relaunches.

Recommendation: Resolve the current Prisma client for each operation, or add a coordinated lifecycle hook that invalidates and reinitializes the registry after disconnect, including refreshing its cache.

Summary: The permission registry retains a Prisma client that storage migration disconnects while the app remains running, breaking durable permission mutations until restart. Branch and PR title prechecks passed.

[ewen-poch]

Investigated the registry-client lifecycle finding; no code change is needed.

I reproduced the exact production sequence with the real client factory: create the PermissionGrantRegistry from getProjectDbClient, call disconnectProjectDbClient, call registry.remember, then query the row through the newly returned shared client. The mutation and durable read both succeed on the current implementation (1 focused test passed). Prisma reconnects the captured client lazily after $disconnect.

The migration also does not replace this database: open-science.db is the authority store under the fixed config root, while the migration moves the configurable data root. Consequently the registry cache is not crossing database identities, and cancellation/discard does not strand it on a different store. Resolving and rehydrating the registry client on every operation would add lifecycle complexity without fixing a reproducible failure, so I am leaving this finding unchanged.

[ewen-poch]

Code review: feat(permission-management): add scoped permission management

I read the full surface area — 90 files, +6942/-521 lines, single commit. The work lands as one coherent, end-to-end change rather than a stack of half-related diffs, and the local Vitest run on the PR branch passes 128/128 main-side tests; the full CI matrix is green on mac/win/linux per the workflow runs.

This is a substantial refactor of the permission model. The result is a much clearer story — one durable PermissionGrant table, one Broker identity catalog, and one Renderer projection — than the previous in-memory ConversationPermissionGrantStore. My main takeaways, in the order I'd raise them in a face-to-face review:

Strengths

• Identity → Capability → Registry → Catalog → Projection is a real separation of concerns. identity-catalog.ts is a closed v1 bootstrap (the right place to keep the v1 promise; dynamic Connector / Compute identities are admitted only by their trusted runtime adapters, and the comment makes that contract explicit). capability.ts is the only place that knows about legacy category keys and the secret-bearing denylist. registry.ts owns durability. catalog.ts owns the renderer projection. Each module is testable in isolation; the cross-cutting wiring lives in one place (src/main/ipc.ts:303-337).
• Idempotency is right. INSERT OR IGNORE on the fingerprint index, revision-checked revoke, and version-stamped IPC snapshots are the three pieces a concurrent app needs and they're each placed where they belong. The latestLoadRequest counter in permission-grants-store.ts plus the version discard in load() is two independent race guards and both are correct.
• Secret-bearing exact-command persistence is opt-in and documented as such. capability.ts:42-96 says plainly that a denylist cannot enumerate credential transports and that only simple local analysis invocations with known non-auth flags become durable. The list of patterns plus the executable/flag/value allowlists is the right shape, and the capability.test.ts cases for AKIA…, ghp_…, x-api-key:, userinfo URLs, --token, X-Auth-Token, etc. cover the obvious cases.
• Cascade cleanup is defended on three independent paths. FK CASCADE on the Project relation, the explicit permissionGrants.prune({ kind: 'project' }) after the row is gone (idempotent on recovery replay, as deletion-coordinator.ts:202-204 calls out), and startup reconcilePermissionGrantOwners for soft owners (custom UUID MCP servers, compute provider ids). The reconciliation test pins exactly the contract: catalog-shaped ids are never guessed stale; only UUID-shaped custom ids are. Good.
• Renderer projection never leaks secrets. expect(JSON.stringify(snapshot)).not.toContain(digest) in catalog.test.ts is a small but load-bearing test.
• coveredBy projection is the right UI affordance. Showing "Also allowed globally" without over-granting is the right behavior; the test pins it. The connectorProjection block correctly projects effectiveState for app-owned and bundled connectors, and the test does not project Connector policy onto app-owned MCP tools pins the negative case.
• Codex policy-amendment stripping and OpenCode native-skill silent accept are both narrowly scoped and tested. The "projection tests pin this contract — option ID, not position" comment in permission-broker.ts:91-98 is exactly the kind of comment that prevents a future codex-acp rename from silently regressing. The OpenCode fallback is guarded by framework id + title === 'skill' + kind === 'other', which is the minimum needed to be safe.
• Project / Global confirmations come through a dedicated dialog with a destructive button. The codeExecution boolean in getScopeConfirmationSubject correctly distinguishes shell / notebook from file operations, so the dialog text adapts (PermissionScopeConfirmationDialog.tsx:43-50).
• Compute legacy migration is safely retriable. migrateLegacy only clears settings.json after every Registry write succeeds, and a PermissionGrantTargetUnavailableError for a project deleted between the two phases is swallowed. The next startup is idempotent.
• Store test coverage of the queue and the latestLoadRequest discard is exactly what I'd want. consumes consecutive revoke receipts FIFO, does not let an older list response overwrite a newer snapshot, rolls the optimistic removal back when persistence fails, explains when an owner disappeared before restore — these are the cases that fail in production.

Concerns / questions

1. purgeExpiredReceipts runs only inside revoke (registry.ts:242-246). Idle registries will keep expired receipt rows in memory. It's bounded by revoke frequency so it doesn't matter much in practice, but I'd either sweep periodically or self-dismiss via setTimeout (also keyed off the existing receiptTtlMs). Tiny.

2. compute-grant-adapter.ts:55-58 migrates legacy settings.json compute grants using a magic sessionId: ''. The fingerprint then includes the empty string, which is fine because the legacy shape has no session id, but it would read more cleanly if computeCapability accepted sessionId?: string and the migration path constructed the capability with qualifier: { mode: 'any' } directly. Cosmetic.

3. connector-broker.ts:42-58 preflight throws on a blocked tool, but authorize does not re-check policy. A caller that goes straight to authorize without calling preflight first will not see the blocked-throw. Today every caller does call preflight, so this is latent, not a bug — but I'd consider gating authorize on preflight(request) at the top, so the failure mode is impossible rather than convention. The same pattern works for the deferRemember branch.

4. permission-broker.ts:90-99 CODEX_MCP_PERSISTENT_ALLOW_OPTION_ID is the right shape, but the project now has three places that name the Codex option-ID contract (permission-broker.ts ×2 and the resolveAllowOptionId shape). If/when codex-acp renames these, the failure will be loud because the tests pin the contract — but I'd still co-locate the three strings into a single Codex policy-amendment constant block so a rename is one edit.

5. registry.ts:411-446 prune for mcp_server and compute_provider owners uses LIKE with manually escaped %/_/\\. That's correct, but it's also the kind of code that benefits from a single helper (escapeLikePattern) since the same two cases duplicate the escape. Not a bug, just a small dedup opportunity.

6. The SettingsPage "Manage permissions" affordance wired from ConnectorDetailView correctly hands off to the right sub-view (custom-server edit vs. bundled detail) — nice touch. One minor consideration: a click on the policyHint link inside PermissionsPanel jumps to navigateConnectors and closes settings, which is the right behavior, but the user loses their current filter selection in PermissionsPanel. A useRetention or URL-state handoff would preserve it on return; right now re-opening Settings lands on the model panel, which is the default. Out of scope for this PR, but worth noting for the next Settings pass.

7. WorkspaceMessageScroller.tsx:19-20 is part of the "collapse legacy Artifact version aliases in conversation history" bullet in the PR body. That change is unrelated to permissions and ships in the same commit. Tests pass and the contract is right, but a permissions-only PR is the cleaner narrative — splitting it out would have made review materially easier.

8. No design document in the repo is the explicit non-goal, and the inline comments in capability.ts, registry.ts, permission-broker.ts:91-98, and reconciliation.ts:43-48 cover the load-bearing decisions. I don't think a separate doc is required, but if you do write one for the user-visible "what counts as a remembered grant" story, link it from docs/internal/.

Test verification

• Local run on the PR branch (vitest): 10 test files / 128 tests pass in the permission-grants / permission-broker / connector-broker / deletion-coordinator / registry surface.
• Workflow runs for this PR: Verify green on macos-14, ubuntu-latest, windows-latest; Analyze (js + python) green; CodeQL green; Validate commit messages green.
• The simplify: no permission data is added to settings.json non-goal is verified: only one new table (PermissionGrant) is added to prisma/schema.prisma, and the runtime DDL in prisma-client.ts:29-60 is byte-compatible with the generated client. No settings.json writes for grants.

Bottom line

I'd merge after the small dedup / consistency items in concerns #1-#5 are addressed or explicitly deferred. Concerns #6-#8 are notes for follow-ups rather than blockers. The architecture is the right one; the secret-handling story is honestly documented; and the test surface covers the cases that actually break in production. Good PR.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Release Notebook capability when session setup fails

src/main/acp/runtime.ts:4054

Impact: The production connection provider issues a bearer capability and stores it in the RPC server. Resume, context-reset, and fresh-adoption paths call this method without a surrounding cleanup path. If later agent setup fails, the failed agent retains a valid token that can still invoke session-bound mcpCall or computeCall operations despite no successfully attached app session.

Recommendation: Treat the returned capability as provisional until the session is fully attached, and release it in every resume/reset/adoption failure path, including failures after the agent receives the MCP configuration. Transfer ownership only after successful adoption.

Summary: A session-scoped Notebook RPC capability is not released when resume or session adoption fails after the connection is issued.

[ewen-poch]

Addressed the actionable review findings in 6ae05b2:

• Made legacy compute-grant migration retryable after a transient failure and removed the empty-session magic value from capability construction.
• Routed every Project/Global approval option, including extra ACP options, through the existing confirmation modal.
• Prevented a failed revoke from restoring a stale full snapshot over a newer successful revoke.
• Centralized SQL LIKE escaping and added literal %, _, and backslash owner-pruning coverage.
• Made Notebook RPC capabilities provisional during resume and fresh adoption: failed resume tokens are revoked before fallback, setup failures revoke the provisional token, and successful adoption retains only the replacement capability.

Validation: focused regression tests pass; ACP runtime 270/270; typecheck passes; lint reports 0 errors; the full suite is 8731 passed with only the same 3 pre-existing spreadsheet-cache-policy failures.

Deferred as lower trade-off follow-ups: timer-driven receipt purging, Codex option-ID constant consolidation, Settings Connector filter retention, and splitting the pre-existing Artifact alias cleanup. Connector policy re-checking is already enforced by authorize defaulting to preflight, while app-owned ACP MCP servers intentionally remain outside external Connector policy.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Session grants can fail before the new session is persisted

src/main/ipc.ts:309

Impact: A first-turn permission response can arrive before the renderer's asynchronous session save completes. The live-scope check then cannot find the session on disk, so selecting “This session” is converted into a cancelled permission and no grant is recorded.

Recommendation: Treat active ACP sessions as live in the registry, or await/retry session persistence before validating and recording a session-scoped grant.

[P2] Deleting an ACP session leaves the notebook runtime with a revoked cached token

src/main/acp/runtime.ts:3271

Impact: This new cleanup revokes the session's notebook RPC capabilities, including the cached control token held by NotebookRuntimeService, but ACP deletion does not shut down that service's RuntimeSession. A live or subsequent REPL host.mcp/host.compute call can therefore use a stale token and fail authorization, while notebook executors remain alive until broader shutdown.

Recommendation: Coordinate ACP/session deletion with NotebookRuntimeService.shutdown or add an explicit release callback that clears the cached control connection and removes the notebook runtime session before revoking its capabilities.

Summary: Static review found two concrete lifecycle and persistence defects. Branch and title prechecks are valid; no tests or project commands were run.

[ewen-poch]

Implemented the actionable findings against the current head:

• Connector and Compute Project/Global approvals now reuse the shared broad-scope confirmation; Once and Session remain immediate.
• Permission-row native controls now use the 3px focus treatment, and icon-only revoke/dismiss actions have tooltips.
• Session-grant target validation now accepts either a durable session record or the same-project live ACP session during the first-save window. Missing projects still fail closed, and startup reconciliation remains authoritative after a crash.
• ACP session deletion now invalidates the pending turn, shuts down the Notebook runtime and releases its cached control connection, then deletes the agent session. A Notebook shutdown failure preserves the ACP session for a safe retry.

The following suggestions conflict with previously approved product behavior and were intentionally not applied:

• The zero-count Permissions view stays visually quiet, matching the supplied reference and its locked renderer test.
• Broad-scope confirmation stays red/destructive because the approved reference treats persistent no-preview execution as a high-risk action.
• Specialist connector access remains independent from the Main Agent Allow/Require approval/Block policy and is still enforced by Specialist capability configuration.
• Provider built-ins remain Once-only until explicit stable cross-framework identities are defined; builtin_tool is reserved rather than populated with guessed mappings.
• The flat Permissions layout is retained instead of introducing a broader SettingsSection/error-style refactor in this PR.

No SQL table, column, relationship, or settings.json change was introduced by this follow-up. Validation: 122 targeted tests passed; typecheck passed; full lint passed with 0 errors and 23 existing warnings; full Vitest passed 8,737 tests with four pre-existing failures confined to the untouched spreadsheet cache-policy test.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Prevent concurrent grant writes from resurrecting revoked authority

src/main/permission-grants/registry.ts:305

Impact: The database transaction completes before the in-memory cache is updated, while revoke/prune delete the cache after their transactions. An overlapping remember can therefore reinsert a grant into records after revoke has committed. resolve() reads only this cache, so a revoked grant can continue authorizing calls.

Recommendation: Serialize registry mutations through one queue/lock that covers both the database transaction and cache update, or make cache updates generation-aware and ensure resolve() revalidates against the authoritative database after revocation.

[P1] Do not auto-approve untrusted OpenCode requests shaped like the native Skill tool

src/main/acp/permission-policy.ts:154

Impact: Any OpenCode permission request with kind: 'other', title skill, and missing provider metadata is granted the provider's allow-once option without user approval. The request shape does not prove native Skill provenance, so a sparse/custom tool request with that shape can bypass the normal permission prompt and execute once.

Recommendation: Require a trusted native-tool provenance marker or a verified OpenCode tool identity. Do not treat missing provider metadata as evidence of native Skill; otherwise route the request through normal approval.

Summary: Two permission-boundary defects were introduced. Branch and pull-request title prechecks are valid.

[ewen-poch]

Addressed the actionable standards findings on the current head:

• Removed the Hallmark pre-emit/genre metadata comments from both renderer files.
• Added direct ConnectorPermissionBroker coverage for Block precedence, policy Allow, remembered grants, projected scopes, deferred writes, denial, and durable-write failure.
• Replaced the new permission reconciliation console.error path with the structured main-process logger and sanitized error fields.
• Aligned the Permissions panel with Settings tokens: fixed p-5, text-base family headings, text-[13px] leading-5 descriptions, and the shared danger banner treatment.
• Added a registry regression test proving that Undo converges on a concurrently re-created fingerprint using the database row's actual id/revision, which remains valid for the next revoke.
• Expanded the PR body with the required behavior → command → result evidence map, final-edit attestation, uncovered risks, and a Mermaid permission-flow diagram.

Intentional trade-offs:

• I did not replace the family sections with SettingsSection: the shared component keeps its action in a non-wrapping row, while this panel's approved responsive layout moves “Revoke all” below the heading at narrow widths. The typography and spacing tokens are now aligned without regressing that behavior.
• The Artifact alias collapse remains because it was explicitly requested for this PR, despite the additional review surface.
• Unknown non-UUID catalog Connector grants remain exempt from startup pruning to avoid an older app deleting grants for catalog ids introduced by a newer version.
• The reported restore race, synchronous grant snapshot, and dynamic MCP alias issues do not reproduce in the current implementation: restore caches the database-returned row, revoke is awaited before snapshot publication, and aliases are derived only from configured servers with collision rejection.

Final checks after the last material edit: 139/139 permission-focused tests passed; typecheck passed; full lint passed with 0 errors and 23 pre-existing warnings; the full suite passed 8,745 tests with the same four failures confined to the untouched spreadsheet cache-policy test.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Do not auto-approve title-only OpenCode Skill requests

src/main/acp/permission-policy.ts:154

Impact: A request with kind other, title skill, and no provider identity is auto-approved before MCP/provider classification. A custom or MCP tool with the same display shape can execute under an Ask profile without user approval.

Recommendation: Require trusted provider identity or another proof that the request is OpenCode’s built-in Skill loader; reject title-only and MCP/custom requests.

Summary: One concrete permission-bypass defect was found in the OpenCode Skill auto-approval path.

[ewen-poch]

Addressed the actionable review findings on the latest main:

• Removed local destructive color overrides and now rely on the shared semantic destructive Button variant.
• Reused SettingsSection while preserving the approved responsive family-action layout, and added the standard reduced-motion-safe interaction transitions.
• Updated the PRD so remembered grants are resolved before unmatched requests reach the renderer.
• Made durable ACP approval fail closed and observable: the provider call is cancelled, the IPC action rejects, and the active Session receives the persistence error.
• Hardened renderer mutation ordering so list broadcasts and out-of-order revoke responses cannot resurrect optimistically removed grants. A failed revoke reloads the authoritative snapshot and keeps restored rows visible alongside the error.
• Replaced the collapsed +N more Undo indicator with up to three independently operable snackbars; hover/focus pauses automatic dismissal without extending the main-process receipt's security deadline.
• Expanded exact-command memory only for constrained safe forms (git status and local shell scripts). Raw command text still never reaches SQLite, and secret-bearing or unproven syntax remains Once-only.
• Rebasing onto origin/main@bb8086c produced one IPC conflict; both the new conversation-export handler and permission-grant handler are preserved.

Intentionally not changed:

• Permissions keeps its list-only scroll boundary because the approved interaction requires the scope filter to remain visible on long lists.
• A single-use Connector call permit is deferred until Connectors are directly exposed through ACP. The current repl_execute -> host.mcp path intentionally authorizes the runner and nested Connector separately, so it cannot produce the duplicate direct-call prompt described by the review.

Validation on 1d6cbc8: typecheck passed; lint passed with 0 errors; the post-rebase permission/ACP set passed 170/170; full repository regression passed 8,795 tests with only the four pre-existing failures in untouched spreadsheet-cache-policy.test.ts.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Revoke session-bound notebook tokens on ACP disconnect

src/main/notebook/local-rpc-server.ts:363

Impact: ACP teardown clears runtime session maps without releasing these tokens. A stale agent can continue invoking mcpCall or computeCall with the old session context after disconnect.

Recommendation: Release all session and provisional-alias capabilities in every ACP teardown path, including close and unexpected connection loss, before clearing routing maps.

Summary: Static inspection found one merge-blocking lifecycle/security defect. No tests or project commands were run.

[ewen-poch]

Addressed the remaining actionable review findings in d46de9e:

• Restored the standard Settings content scroll boundary and made the Permissions scope filter sticky within it, removing the panel-local vertical scroller.
• Revoked every active Notebook RPC session capability before session maps are cleared on both normal disconnect and unexpected ACP connection close. Added regression coverage for both paths.
• Replaced the three-item Undo display cap with a bounded scrollable stack that renders every unexpired receipt with independent Undo and dismiss actions. The authoritative 8-second main-process receipt TTL remains unchanged.

The Artifact Version alias collapse remains in this PR because it fixes the reported duplicate GENERATED cards for historical alias/native IDs and was explicitly included in the requested scope.

Validation: typecheck passed; lint passed with 0 errors and 23 existing warnings; focused renderer tests passed 54/54; focused ACP cleanup tests passed 3/3; the full suite passed 587 files and 8,798 tests, with only the three known failures in the untouched spreadsheet-cache-policy test file.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Owner pruning leaves undo receipts restorable

src/main/permission-grants/registry.ts:414

Impact: Deleting an MCP server or compute provider prunes its grants but leaves revocation receipts active. Undo can then restore a grant for the deleted owner because liveness checks cover only project/session scopes, leaving stale authority that may apply if the owner identity is reused.

Recommendation: Invalidate receipt rows during owner pruning, or add MCP/compute owner liveness checks to restore and report target-unavailable.

Summary: Static inspection found one lifecycle defect. Branch and title prechecks are valid.

[ewen-poch]

Addressed both current-head review findings in 45c7642.

• Legacy Compute migration now attempts every source row independently. Valid grants are imported idempotently even when a deleted Project leaves an orphaned row; the complete settings.json source is retained and never cleared until every row imports successfully. An orphan-only degraded migration no longer blocks new Registry-backed Compute approvals, while database/write failures still fail closed and remain retryable.
• OpenCode native Skill loading is no longer approved from presentation text alone. The runtime must first observe the native tool_call, bind it to request_permission by session and toolCallId, and attach a process-local Symbol identity. Isolated or mismatched Skill requests follow the normal approval path; the trusted native call remains call-scoped and creates no remembered grant.

Validation: focused Compute/permission-policy/broker/runtime tests pass 343/343; typecheck and changed-file ESLint pass; check:web-api-map passes. Full npm test reports 8,814 passed and 140 skipped, with only the same three failures in the untouched spreadsheet-cache-policy.test.ts that reproduce on clean main@69766f7.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Do not persist approvals for mutable scripts by command text alone

src/main/permission-grants/capability.ts:84

Impact: Project or global approval for commands such as python analyze.py, bash analyze.sh, or node script.js is keyed only by the command text. A changed, replaced, or different same-named script can therefore execute without renewed approval, and relative paths can resolve differently across projects.

Recommendation: Restrict durable approvals to commands whose safety is independent of mutable script contents, or include the canonical script path and content identity in the qualifier and require re-approval whenever they change.

Summary: One blocking security issue was found in durable command permission matching.

[ewen-poch]

Addressed the mutable-script approval finding in dfffc47.

Durable exact-command grants are now restricted to commands whose safety is independent of mutable workspace content. V1 retains scoped memory for the content-independent git status command only; Python, Bash, Node, R/Rscript, pytest, and other interpreter/script invocations expose provider Once only, regardless of whether their script path is relative or absolute. This avoids treating an unchanged command digest as proof that referenced code is unchanged, without adding path/content hashes or changing the grant schema.

Regression coverage now verifies both sides of the boundary: git status can produce a redacted durable qualifier, while mutable scripts offer only Once and create no Registry row. Focused capability/broker tests pass 99/99; typecheck, changed-file ESLint, and check:web-api-map pass. Full npm test reports 8,820 passed and 140 skipped, with only the same three failures in the untouched spreadsheet-cache-policy.test.ts that reproduce on clean main@69766f7.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Serialize cache updates with grant mutations

src/main/permission-grants/registry.ts:350

Impact: After the revoke transaction commits, another remember operation can insert and cache the same fingerprint before this deletion runs. The revoke then removes the newly remembered record from the in-memory cache even though it remains durable, causing stale authorization decisions and incorrect settings snapshots; the reverse interleaving can briefly authorize a grant after revocation.

Recommendation: Serialize registry mutations, including the database transaction and cache update, or reconcile the cache from the committed database state using a mutation lock/version check before publishing changes.

Summary: Static inspection found a concurrent mutation race in the permission-grant registry that can leave its authorization cache inconsistent with the database.

[ewen-poch]

Addressed the current-head review findings in 36a2179.\n\n- Serialized remember, revoke, restore, and prune through one Registry mutation queue that covers each database commit and cache publication. A deterministic regression test pauses a committed revoke and proves a concurrent remember cannot make the durable table and authorization cache diverge.\n- Restricted V1 exact-command persistence to the bare git status command. Path-qualified replacements such as ./git status and /tmp/git status, along with interpreters and mutable scripts, remain Once-only.\n- Made explicit OpenCode provider metadata authoritative: the legacy Skill title/raw-input fallback now runs only when provider metadata is completely absent. A call-id-bound regression test verifies explicit non-Skill metadata cannot gain trusted native-Skill identity.\n- Closed the Project FK-cascade window without weakening #542 deletion semantics. The retryable pre-delete prune remains fallible; after hard delete, a queued non-failing owner-finalization barrier waits for already-started Registry mutations and invalidates orphan cache/Undo state.\n- Replaced the Undo stack's native overflow container with the shared ScrollArea and refreshed the PR body to the current HEAD, Once-only script policy, Artifact-repair scope decision, and final validation map.\n\nValidation after the final material edit: focused permission/deletion/UI tests pass; typecheck, changed-file ESLint, and check:web-api-map pass. Full npm test reports 8,828 passed and 140 skipped, with only the same three failures in the untouched spreadsheet-cache-policy.test.ts that reproduce outside this work.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: No concrete blocking defects found in the pull request changes. Branch and pull request title prechecks passed.

[ewen-poch]

Rebased the single feature commit onto main@a6ab4fc in b7b7c44.

The two conflicts were both in the new shared Web RPC registration seam. The resolution preserves the latest ipcMainHandle contract while retaining permission behavior: Compute deletion still prunes the matching provider grants, and Connector approval responses keep the typed request shape. No architecture, schema, data-relationship, or user-interaction behavior changed.

Post-rebase validation:

• focused Registry, deletion, permission IPC, Compute migration, IPC registry, and Web RPC contract tests: 48 passed
• npm run check:web-api-map: passed
• Node and Web typechecks: passed with --preserveSymlinks to avoid duplicate Vite peer types from the nested worktree and parent node_modules
• full local suite: only the same three baseline failures in untouched spreadsheet-cache-policy.test.ts; the fresh GitHub Verify matrix is running as the clean-environment authority

The PR description now points to the rebased HEAD and latest main baseline.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Custom server edits retain grants for changed endpoints

src/main/ipc.ts:737

Impact: Durable grants are pruned when a custom server is removed, but not when its URL, command, environment, headers, or transport changes. Because the server ID remains unchanged, existing approvals continue authorizing calls against the new endpoint or credentials without re-approval.

Recommendation: Invalidate security-sensitive grants when custom server configuration changes, or version/rotate the server identity so the changed configuration requires fresh approval.

Summary: Static inspection found one merge-blocking security defect in custom MCP grant lifecycle handling.

[ewen-poch]

Addressed the custom MCP grant-lifecycle finding in 2c8e2dc.

• SettingsService.updateCustomServer now distinguishes display-only edits from security-sensitive changes to transport, URL, command, arguments, environment, and headers.
• Security-sensitive edits validate the candidate first, then await grant invalidation before persisting the replacement configuration. If grant cleanup fails, the old server configuration remains active and the settings mutation reports the error.
• Description-only edits retain existing grants.
• Main wires the invalidation gate to PermissionGrantRegistry.prune({ kind: "mcp_server", serverId }), so every durable tool grant owned by the unchanged server id is removed before the new endpoint or executable can become active.

Validation: 105 focused Settings, IPC, Registry, and Connector tests pass; Node and Web typechecks pass; changed-file ESLint passes with 0 errors; check:web-api-map passes. No schema, data-relationship, or UI change was introduced.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Register permission handlers with the Web RPC registry

src/main/permission-grants/ipc.ts:115

Impact: These handlers use raw ipcMain.handle, bypassing the shared registry that publishes Web RPC channels. Consequently, the web bootstrap omits permissions:list, permissions:revoke, and permissions:restore, so the Permissions settings panel cannot load or mutate grants in web mode.

Recommendation: Import and use ipcMainHandle for all three permission handlers so Electron IPC and Web RPC expose the same implementation.

Summary: Static inspection found one concrete Web RPC integration defect.

[ewen-poch]

Addressed the Web RPC registration finding in 788633d.

• Replaced the three raw Electron registrations with the shared ipcMainHandle for permissions:list, permissions:revoke, and permissions:restore.
• Added a runtime assertion against the shared Web RPC router, so the test now fails if the generated channel map exists but no callable handler is registered.

Validation: permission IPC, IPC handler registry, and shared Web RPC contract suites pass (3 files, 8 tests); Node and Web typechecks pass; changed-file ESLint and Prettier pass; check:web-api-map passes. The change does not alter the permission schema, scope semantics, or UI.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Revalidate pending custom-server approvals before remembering grants

src/main/connectors/service.ts:246

Impact: If a custom MCP server endpoint, command, or credentials change while approval is pending, the update prunes existing grants but does not invalidate the pending call. Approval then remembers a grant under the unchanged server ID, allowing future calls to the new configuration without re-approval.

Recommendation: Bind approvals to a server configuration generation or fingerprint and revalidate it before remembering or dispatching; cancel pending approvals when security-sensitive configuration changes.

[P1] Prevent deleted compute hosts from restoring stale grants

src/main/compute/compute-approval-broker.ts:126

Impact: A compute approval can remain pending while its host is deleted and grants are pruned. Approving afterward writes the grant anyway; recreating a host with the same deterministic provider ID then inherits authorization for the previous host.

Recommendation: Invalidate pending approvals on host deletion and re-check the current host identity/generation before persisting the grant, or key grants by an immutable host identity rather than the reusable provider ID.

Summary: Static inspection found two authorization races where pending approvals can recreate grants after their owners are invalidated.

[ewen-poch]

Addressed both pending-owner authorization races from the review on 788633d:

• Custom MCP security edits now advance a per-server generation before grant pruning. Calls that captured the prior generation are rejected before discovery, grant persistence, or dispatch; new calls remain fail-closed until the refreshed snapshot matches the committed security fingerprint. Failed persistence rolls the barrier back while keeping prior grants pruned.
• Compute host deletion now invalidates and denies pending approvals before deleting the host. Remembered decisions also revalidate the immutable ComputeHost.id, so recreating the same deterministic provider ID cannot inherit a stale approval.

This does not change the permission schema, scope semantics, RPC contracts, or UI.

Validation:

• Focused lifecycle suite: 5 files, 158 tests passed.
• Node and Web TypeScript checks passed.
• ESLint and Prettier passed.
• npm run check:web-api-map passed.
• Full npm test with loopback access: 589 files passed, 11 skipped; only the existing 3 failures in untouched spreadsheet-cache-policy.test.ts remain.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Invalidate compute grants before provider ID reuse

src/main/compute/ipc.ts:449

Impact: Provider deletion completes before grant pruning, and pruning is awaited separately. A concurrent recreation of the same provider ID can resolve the cached old session/project/global grant and run on the replacement host without approval.

Recommendation: Serialize compute deletion, grant invalidation, and provider creation, or block provider-ID reuse until pruning completes. Also validate the persisted grant against the current host identity before auto-allowing.

[P2] Track cancellation while grant resolution is pending

src/main/acp/permission-broker.ts:627

Impact: Registry-backed permission requests await asynchronous grant resolution before entering pendingRequests. A session cancellation during that await is missed by cancelForSession; the continuation can later emit a stale permission prompt and leave the cancelled ACP call unresolved.

Recommendation: Register the pending request before awaiting registry resolution, or track a per-session cancellation generation and resolve/cancel the request without emitting it when the generation changes.

Summary: Static inspection found two concrete permission-lifecycle races introduced by the change.

[ewen-poch]

Addressed the two lifecycle race findings in 88e4a68:

• Compute host create/delete/grant cleanup now share one FIFO lifecycle queue. A replacement provider ID is not exposed until stale grants are pruned, creation defensively retries stale cleanup after a failed deletion attempt, and durable auto-allow revalidates both owner identity and provider generation.
• ACP cancellation now uses global and per-session generations, so cancellation wins even while an asynchronous durable-grant lookup is pending; the request resolves as cancelled and is never emitted to the renderer.

Regression coverage was added for provider-ID reuse during deferred cleanup, replacement-host durable auto-allow denial, and cancellation during deferred grant resolution. Focused tests, Node/Web typechecks, ESLint, check:web-api-map, and diff checks pass. The full suite remains at 589 passed / 11 skipped files, with only the same three pre-existing failures in untouched spreadsheet-cache-policy.test.ts.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Rebind the registry after project DB disconnects

src/main/permission-grants/registry.ts:236

Impact: Data-root migration disconnects this captured Prisma client, but the registry continues using it. Subsequent permission approvals and management mutations fail for the rest of the process.

Recommendation: Reacquire/reload the client after migration teardown, or add an explicit registry rebind hook.

Summary: Static inspection found one merge-blocking lifecycle defect. Branch and pull-request title prechecks are valid.

[ewen-poch]

Addressed the registry client-lifecycle finding in 7c58c27.

The PermissionGrant registry now uses its existing getClient() provider for every serialized database mutation instead of retaining the initialization client indefinitely. After the shared project database is drained for exclusive migration validation, subsequent remember/revoke/restore/prune operations therefore bind to the recreated singleton while the in-memory grant snapshot and mutation FIFO remain unchanged. The authority database remains under the fixed config root; this is a connection-lifecycle correction, not a data migration or schema change.

A regression test retires the initialization client, swaps the provider to a newly opened client, and verifies that a subsequent grant persists without touching the retired instance. Registry tests pass (14/14), Node/Web typechecks, ESLint, Prettier, check:web-api-map, and diff checks pass. Full npm test: 589 files passed / 11 skipped, with only the same three pre-existing failures in untouched spreadsheet-cache-policy.test.ts (8868 tests passed).