Skip to content

fix(permission): persist "always allow" grants so they survive a restart#1331

Merged
Astro-Han merged 2 commits into
devfrom
claude/fix-permission-persistence
Jun 15, 2026
Merged

fix(permission): persist "always allow" grants so they survive a restart#1331
Astro-Han merged 2 commits into
devfrom
claude/fix-permission-persistence

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Jun 15, 2026

Copy link
Copy Markdown
Owner

Summary

On an "always allow" reply, the granted rules were pushed into the in-memory approved list but never written back to PermissionTable. Capture the project id in the permission State at load, and on an "always" reply upsert the full cumulative approved ruleset under the project's primary key so the grant persists.

Why

The approved list is loaded from the project's PermissionTable row at instance startup (permission/index.ts:186), but nothing ever wrote it back — approved.push(...) only mutated memory. So every app restart (or instance reload) dropped all "always allow" grants and the next action re-asked. This affected every tool that supports "always" (edit, bash, browser, …), and was the real reason "always" felt like it never stuck.

Related Issue

None — found while investigating the embedded-browser permission prompts (context: PR #1330).

Human Review Status

Pending

Review Focus

  • The upsert in reply(): it writes the whole cumulative approved array under project_id (the table's primary key) via onConflictDoUpdate. Confirm that's the right shape and that writing inside the reply effect (synchronous Database.use, current instance's DB) is correct.
  • That the write is gated to existing.info.always.length > 0, so plain "once" replies and empty-always browser asks don't write.

Risk Notes

Behavior change limited to permission persistence: "always allow" now survives restarts as intended. No schema/migration change — PermissionTable already exists and is already read at startup; this only starts writing the column that was already being read. No platform/UI surface touched.

Skipped conditional checklist items: visible-UI item (no UI or copy changed); platform item (no Electron/packaging/updater/signing/paths surface — server-side permission persistence).

How To Verify

opencode typecheck (tsgo --noEmit): clean
bun test test/permission/: 113 pass / 0 fail (includes new reload test)
new test "reply - an 'always' grant survives an instance reload":
  approve always -> Instance.disposeAll() -> reload same dir -> ask again resolves with no prompt
  confirmed it FAILS (re-asks, ask pends -> times out) with the persistence write reverted

Screenshots or Recordings

None — no visible UI or copy changed.

Checklist

How to use this checklist:

  • Tick a box by replacing [ ] with [x]. Do not edit, add, or remove items.
  • The bot-applied label items can only be honestly ticked AFTER the PR is opened and the labeler / priority-triage bots have run — return to the PR description and tick them then.
  • Most items are required. The few that are conditional are explicitly marked (conditional); for those, leave unticked if they truly do not apply and explain why in Risk Notes. All other items must be ticked before requesting human review.
  • Type label — this PR carries exactly one of bug, enhancement, task, documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.
  • Routing labels — this PR carries at least one of app, ui, platform, harness, ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.
  • Priority label — this PR carries exactly one of P0, P1, P2, P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.
  • Human Review Status above is set to Pending, Approved by @<reviewer>, or Not required: <reason> (default is Pending; "not required" is restricted to bot-authored low-risk PRs).
  • I linked the related issue, or stated in Summary why there is no issue.
  • I described the review focus and any meaningful risks.
  • I replaced the example block in How To Verify with the real verification steps and the key result for each.
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope.
  • (conditional) I manually checked visible UI or copy changes when needed, with screenshots or recordings. Leave unticked only if no visible UI or copy changed.
  • (conditional) I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes. Leave unticked only if no platform/packaging surface was touched.
  • (conditional) I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant. Leave unticked only if none of those surfaces was touched.
  • I reviewed the final diff for unrelated changes and suspicious dependency changes.
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English.

Summary by CodeRabbit

  • Bug Fixes
    • Permissions marked as "always allow" now correctly persist and remain in effect after application restarts, instead of being lost on reload.

Approvals from an "always allow" reply were pushed into the in-memory `approved` list but never written back to PermissionTable. That list is loaded from the project's row at instance startup, so every app restart (or instance reload) dropped all "always" grants and the next action re-asked — for every tool, not just the browser.

Capture the project id in the permission State at load, and on an "always" reply upsert the full cumulative approved ruleset under the project's primary key. On reload the grant is read back and the same target auto-allows with no prompt.

Test: new next.test.ts case approves "always", disposes the instance, then reloads the same project directory and asks again — resolves with no prompt. Confirmed it fails (re-asks, times out) with the write reverted. Verification: opencode typecheck clean; permission tests 113 pass / 0 fail.
@Astro-Han Astro-Han added bug Something isn't working P2 Medium priority labels Jun 15, 2026
@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@Astro-Han, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 7 minutes and 30 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 666269a2-793a-46ad-b264-7b91f926b20c

📥 Commits

Reviewing files that changed from the base of the PR and between de234b9 and 88fde5a.

📒 Files selected for processing (2)
  • packages/opencode/src/permission/index.ts
  • packages/opencode/test/permission/next.test.ts
📝 Walkthrough

Walkthrough

Permission.Service state gains a projectID field set at initialization. In reply(), when an approved request contains always patterns, the cumulative approved ruleset is upserted into PermissionTable keyed by project_id. A new test confirms these grants survive Instance.disposeAll() and a subsequent reload.

Changes

Always-grant persistence

Layer / File(s) Summary
State projectID capture and always-grant upsert
packages/opencode/src/permission/index.ts
State type adds a projectID field populated from ctx.project.id at service init. reply() destructures projectID from state and, when the approval includes always patterns, inserts or upserts the full cumulative approved ruleset into PermissionTable on project_id conflict, updating time_updated to Date.now().
Reload persistence test
packages/opencode/test/permission/next.test.ts
New test approves an ask with always, calls Instance.disposeAll(), reloads from the same temp directory via Instance.provide(), then asserts the subsequent same-origin ask auto-resolves and Permission.list() returns empty.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

app, harness

Poem

🐇 A grant was approved, then gone with the breeze,
But now little bunny persists it with ease!
projectID tucked in the state just right,
Upserted to the table — survives the night.
Reload the instance, the "always" stays true,
No more asking twice — the ruleset shines through! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly summarizes the main change: persisting 'always allow' grants across restarts, which is the core objective of the PR.
Description check ✅ Passed The description comprehensively covers all required sections: Summary, Why, Related Issue, Human Review Status, Review Focus, Risk Notes, How To Verify, and a detailed completed checklist with conditional items properly marked.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-permission-persistence

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the harness Model harness, prompts, tool descriptions, and session mechanics label Jun 15, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces changes to persist "always allow" permission grants to the database, ensuring they survive application restarts and instance reloads. Specifically, it captures the projectID in the permission state and performs an upsert to the PermissionTable when an "always" grant is approved. A new integration test has also been added to verify this persistence behavior. I have no feedback to provide as there are no review comments to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

The "always allow" persistence write replaced the whole project row with the
current instance's cached `approved`. Multiple instances can share one project
row — project id is the shared git-common-dir, so different worktrees or
subdirectories of one repo resolve to the same row — while each keeps its own
`approved` loaded at startup. An "always" reply in one instance therefore
overwrote the row with only that instance's grants, dropping grants another
instance had persisted after it loaded (last-writer-wins data loss).

Re-read the current row and union it with `approved` inside one synchronous
`Database.use` callback before writing. Single process + no await between the
read and write makes the read-merge-write atomic against other fibers, so the
row converges to the union of every instance's grants. `mergeRulesets` dedupes
by (permission, pattern, action) so the row does not grow unboundedly.

Adds a regression test: two sibling instances of one git repo each grant a
different "always", and both survive an instance reload. Verified it fails
(the first grant re-asks on reload) when the merge is reverted to a whole-row
write.
@Astro-Han

Copy link
Copy Markdown
Owner Author

Follow-up: merge persisted grants instead of clobbering (commit 88fde5a)

An independent Codex review flagged a lost-update in the first commit that I verified is real:

The bug. The persistence write replaced the entire project row with the current instance's cached approved. Project id is the shared git-common-dir (project/project.ts resolves via git rev-parse --git-common-dir and caches the id in the shared .git/opencode), so different worktrees / subdirectories of one repo resolve to the same row — but instance state is keyed by Instance.directory (project/instance.ts:98), so each holds its own approved loaded at startup. An "always" reply in instance B (whose approved was loaded before instance A's grant) overwrote the row with only B's grants, dropping A's. PawWork's multi-worktree workflow hits this directly.

The fix. Re-read the current row and union it with approved inside one synchronous Database.use callback before writing. Single process + no await between read and write makes the read-merge-write atomic against other fibers, so the row converges to the union of all instances' grants. mergeRulesets dedupes by (permission, pattern, action) to bound growth.

Test. Added a regression test: two sibling instances of one git repo each grant a different "always"; both survive an instance reload. Confirmed it fails (the first grant re-asks on reload) when the merge is reverted to a whole-row write.

Verification: bun test test/permission/ → 114 pass / 0 fail; tsgo --noEmit clean.

@Astro-Han Astro-Han merged commit 1d17f2e into dev Jun 15, 2026
32 checks passed
@Astro-Han Astro-Han deleted the claude/fix-permission-persistence branch June 15, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant