feat: support configurable loop permission overrides - #84
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34538785 | Triggered | Generic Password | 4781cfd | test/utils/tui-remote-launch.test.ts | View secret |
| 34538785 | Triggered | Generic Password | f0fddff | test/utils/tui-remote-launch.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/loop/runtime.ts (1)
360-373: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPortable permission rules are resolved from a workspace id that can differ from the id the session binds to. Both sites read
workspaceIdfrom a snapshot taken before the authoritative workspace id is established, then bind the created session to the later id. When the two ids differ, the session receives the wrong portable rule set, which can widen permissions.
src/loop/runtime.ts#L360-L373: move theresolveLoopPermissionOptionsForLoopcall afterensureWorkspaceForLoopand passensured.workspaceId ?? state.workspaceId; apply the same change at Line 539, Line 1337, and Line 1504.src/services/execution.ts#L1760-L1761: move theresolveLoopPermissionOptionsForWorkspaceandbuildLoopPermissionRulesetcalls inside therunExclusivecallback, after thestoppedStaterefresh at Line 1821 and Line 1828.🤖 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 `@src/loop/runtime.ts` around lines 360 - 373, Resolve portable permission rules only after the authoritative workspace ID is established. In src/loop/runtime.ts at ranges 360-373, 539, 1337, and 1504, move resolveLoopPermissionOptionsForLoop after ensureWorkspaceForLoop and pass ensured.workspaceId ?? state.workspaceId; in src/services/execution.ts at ranges 1760-1761, move resolveLoopPermissionOptionsForWorkspace and buildLoopPermissionRuleset inside the runExclusive callback, after the stoppedState refresh.
🧹 Nitpick comments (4)
src/agents/auditor.ts (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the
questionexception.
AUDITOR_TOOL_EXCLUDESdeliberately keepsquestionavailable by filtering it out ofSHARED_STRUCTURAL_DENY_PERMISSIONS.test/agents.test.tsasserts several auditor exclusions but never asserts thatquestionstays excluded from the exclude list. If someone later removesquestionfromSHARED_STRUCTURAL_DENY_PERMISSIONS, the filter becomes dead code and no test fails. If someone removes the filter, the auditor silently loses the tool and no test fails either.💚 Proposed test addition in test/agents.test.ts
test('auditor agent has expected tool exclusions', () => { expect(auditorAgent.tools?.exclude).toBeDefined() expect(auditorAgent.tools?.exclude).toContain('apply_patch') + // The auditor keeps `question`; it is filtered out of the shared structural denies.+ expect(auditorAgent.tools?.exclude).not.toContain('question')+ expect(SHARED_STRUCTURAL_DENY_PERMISSIONS).toContain('question')🤖 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 `@src/agents/auditor.ts` around lines 6 - 8, Add a regression assertion in the auditor exclusions tests in test/agents.test.ts verifying that question is not present in AUDITOR_TOOL_EXCLUDES. Keep the assertion focused on the existing AUDITOR_TOOL_EXCLUDES behavior and preserve the current checks for other auditor exclusions.test/constants/loop.test.ts (1)
192-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the structural-deny probe order-independent.
This line finds the first structural deny by testing for
review-writeoredit. It relies onLOOP_ONLY_STRUCTURAL_DENY_PERMISSIONSandAUDIT_ONLY_STRUCTURAL_DENY_PERMISSIONSeach emitting that specific name first. Reordering either list makes the assertion probe the wrong rule while still passing. The later test at Line 228 already uses an order-independent form; reuse it here.♻️ Proposed refactor
- const firstStructuralDenyIdx = rules.findIndex(r => r.permission === 'review-write' || r.permission === 'edit')+ const firstStructuralDenyIdx = rules.findIndex(+ r => r.action === 'deny' && FORGE_MANAGED_PERMISSIONS.has(r.permission) && r.permission !== 'external_directory',+ )🤖 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 `@test/constants/loop.test.ts` at line 192, Update the structural-deny lookup assigned to firstStructuralDenyIdx to use the same order-independent predicate as the later test near line 228, rather than relying on review-write or edit appearing first. Reuse that existing predicate form while leaving the surrounding assertion unchanged.test/loop-permission-ruleset.test.ts (1)
601-633: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit
mockUpdatecall assertion. This gives a direct failure before indexingmock.calls[0].🤖 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 `@test/loop-permission-ruleset.test.ts` around lines 601 - 633, Add an explicit assertion that mockUpdate was called before reading mockUpdate.mock.calls[0] in the fallback-path test, while preserving the existing permission assertion and test behavior.test/services/execution.start-loop.test.ts (1)
445-449: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit assertion for the configured deny rule.
The expected value repeats the exact expression used in
src/services/execution.tsat Line 1193. The assertion therefore passes even ifresolveLoopPermissionOptionsstops translatingpermissions.denyinto a deny rule. Add a concrete assertion for the rule, matching the style used intest/loop-runtime-audit-permissions.test.tsat Line 276.💚 Proposed addition
expect(client.session.create).toHaveBeenCalledWith( expect.objectContaining({ permission: buildLoopPermissionRuleset(resolveLoopPermissionOptions(configuredConfig as any)), }), ) + const createArgs = (client.session.create as any).mock.calls[0][0]+ expect(createArgs.permission).toContainEqual({ permission: 'webfetch', pattern: '*', action: 'deny' })🤖 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 `@test/services/execution.start-loop.test.ts` around lines 445 - 449, Update the expectation around client.session.create to assert the configured deny rule explicitly, rather than deriving permission from resolveLoopPermissionOptions(configuredConfig as any). Follow the concrete deny-rule assertion style used in the loop runtime audit permissions test, while preserving the existing permission expectation for the remaining rules.
🤖 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 `@src/hooks/loop-permission.ts`:
- Line 95: Update applyRuleset around getPermissionOptions so resolver
rejections are caught instead of bypassing session.update. Log the failure, then
build the default ruleset from empty options and continue applying it, while
preserving the existing successful resolver path.
In `@src/services/execution.ts`:
- Around line 1863-1868: Update the workspace recreation flow around
createBuiltinWorktreeWorkspace so previousEntry.extra does not reuse stale
startRef, syncRef, or gitRemote values. Filter those fields out before passing
extra, or recompute them from the new launch context, while preserving unrelated
extra metadata.
In `@src/types.ts`:
- Around line 98-104: Update the doc comment above LoopPermissionsConfig to
state that only blanket `*` denies for Forge-required permissions are rejected,
while scoped denies such as `bash` with a command pattern remain honored.
Preserve the existing guidance about Forge-managed permissions and
`allowExternalDirectories`.
In `@src/utils/loop-permission-options.ts`:
- Around line 24-33: Update the lookup flow around getForgeWorkspaceEntry so
exceptions return the empty rules result without executing perClient.set. Only
cache rules after a successful workspace lookup, including the valid empty
result when no rules are configured, and preserve returning [] for lookup
failures.
In `@src/workspace/forge-worktree.ts`:
- Around line 64-74: Update getForgeWorkspacePermissionRules to accept only
canonical validated deny rules, rejecting any rule with action "allow" and
Forge-managed permissions such as external_directory. Preserve validation of the
rule shape and add a regression test covering { permission:
'external_directory', pattern: '*', action: 'allow' } to ensure it is excluded
before buildLoopPermissionRuleset consumes the rules.
In `@test/utils/tui-remote-launch.test.ts`:
- Around line 600-603: Fix the filter predicate used to build externalAllows by
removing the duplicate arrow function and ensuring the callback directly
evaluates each rule’s permission and action checks. Preserve the intended result
of retaining only external_directory rules with action allow.
---
Outside diff comments:
In `@src/loop/runtime.ts`:
- Around line 360-373: Resolve portable permission rules only after the
authoritative workspace ID is established. In src/loop/runtime.ts at ranges
360-373, 539, 1337, and 1504, move resolveLoopPermissionOptionsForLoop after
ensureWorkspaceForLoop and pass ensured.workspaceId ?? state.workspaceId; in
src/services/execution.ts at ranges 1760-1761, move
resolveLoopPermissionOptionsForWorkspace and buildLoopPermissionRuleset inside
the runExclusive callback, after the stoppedState refresh.
---
Nitpick comments:
In `@src/agents/auditor.ts`:
- Around line 6-8: Add a regression assertion in the auditor exclusions tests in
test/agents.test.ts verifying that question is not present in
AUDITOR_TOOL_EXCLUDES. Keep the assertion focused on the existing
AUDITOR_TOOL_EXCLUDES behavior and preserve the current checks for other auditor
exclusions.
In `@test/constants/loop.test.ts`:
- Line 192: Update the structural-deny lookup assigned to firstStructuralDenyIdx
to use the same order-independent predicate as the later test near line 228,
rather than relying on review-write or edit appearing first. Reuse that existing
predicate form while leaving the surrounding assertion unchanged.
In `@test/loop-permission-ruleset.test.ts`:
- Around line 601-633: Add an explicit assertion that mockUpdate was called
before reading mockUpdate.mock.calls[0] in the fallback-path test, while
preserving the existing permission assertion and test behavior.
In `@test/services/execution.start-loop.test.ts`:
- Around line 445-449: Update the expectation around client.session.create to
assert the configured deny rule explicitly, rather than deriving permission from
resolveLoopPermissionOptions(configuredConfig as any). Follow the concrete
deny-rule assertion style used in the loop runtime audit permissions test, while
preserving the existing permission expectation for the remaining rules.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d410cc6-e4c0-4f1e-bb11-f9857fa82904
📒 Files selected for processing (48)
AGENTS.mddocs/api/_media/architecture.mddocs/api/_media/configuration.mddocs/api/functions/createForgePlugin.mddocs/api/functions/createParentSessionLookup.mddocs/api/functions/createSessionDirectoryLookup.mddocs/api/interfaces/CompactionConfig.mddocs/api/interfaces/CreateParentSessionLookupOptions.mddocs/api/interfaces/CreateSessionDirectoryLookupOptions.mddocs/api/interfaces/DashboardConfig.mddocs/api/interfaces/PluginConfig.mddocs/api/variables/VERSION.mddocs/api/variables/default.mddocs/architecture.mddocs/configuration.mddocs/modules.mdforge-config.jsoncsrc/agents/auditor.tssrc/agents/code.tssrc/agents/feature-splitter.tssrc/constants/loop.tssrc/hooks/loop-permission.tssrc/hooks/plan-approval.tssrc/index.tssrc/loop/runtime.tssrc/services/execution.tssrc/services/session-loop-resolver.tssrc/tui.tsxsrc/types.tssrc/utils/audit-session.tssrc/utils/loop-permission-options.tssrc/utils/loop-permission-warnings.tssrc/utils/loop-session.tssrc/utils/toast.tssrc/utils/tui-client.tssrc/utils/tui-remote-launch.tssrc/workspace/forge-worktree.tstest/agents.test.tstest/constants/loop-permissions-config.test.tstest/constants/loop.test.tstest/loop-permission-ruleset.test.tstest/loop-runtime-audit-permissions.test.tstest/plugin.test.tstest/services/execution-restart.test.tstest/services/execution.start-loop.test.tstest/utils/loop-permission-warnings.test.tstest/utils/tui-client-loop-inline-plan.test.tstest/utils/tui-remote-launch.test.ts
| logger.error(`[loop-permission] failed to fetch parent ${parentID} for inheritance`, err) | ||
| } | ||
| if (!ruleset) ruleset = buildLoopPermissionRuleset({ allowDirectories: getAllowExternalDirectories?.() }) | ||
| if (!ruleset) ruleset = buildLoopPermissionRuleset((await getPermissionOptions?.(workspaceId)) ?? {}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Description: Check whether the workspace-aware resolver can reject, or catches internally.set -euo pipefail
F=$(fd --type f 'loop-permission-options.ts' src/utils | head -1)
ast-grep outline "$F" --items all
echo"=== source ==="
cat -n "$F"Repository: chriswritescode-dev/opencode-forge
Length of output: 3135
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"=== loop-permission outline ==="
ast-grep outline src/hooks/loop-permission.ts --items all
echo"=== loop-permission relevant source ==="
sed -n '1,145p' src/hooks/loop-permission.ts
echo"=== index resolver and call sites ==="
rg -n -A12 -B8 'resolveLoopPermissionOptionsForWorkspace|getPermissionOptions|applyRuleset|ensurePatched|onSessionCreated' src/index.ts src/hooks src
echo"=== logger and type availability ==="
rg -n -A4 -B4 'logger\.error|LoopPermissionRulesetOptions' src/hooks/loop-permission.ts src | head -160Repository: chriswritescode-dev/opencode-forge
Length of output: 50390
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"=== workspace lookup and option construction ==="
sed -n '1,65p' src/utils/loop-permission-options.ts
rg -n -A25 -B8 'function resolveLoopPermissionOptions|export function resolveLoopPermissionOptions|function getForgeWorkspaceEntry' \
src/constants/loop.ts src/workspace/forge-worktree.ts
echo"=== focused rejection-path verifier ==="
python3 - <<'PY'from pathlib import Pathhook = Path("src/hooks/loop-permission.ts").read_text()resolver = Path("src/utils/loop-permission-options.ts").read_text()assert "const portable = await readWorkspacePortableRules(client, workspaceId)" in resolverassert " } catch {" in resolverassert "if (!ruleset) ruleset = buildLoopPermissionRuleset((await getPermissionOptions?.(workspaceId)) ?? {})" in hook# The workspace read is inside readWorkspacePortableRules' catch, while the# patcher's await is not inside the parent-fetch or update try blocks.read_start = resolver.index("async function readWorkspacePortableRules")read_end = resolver.index("export async function resolveLoopPermissionOptionsForWorkspace")read_body = resolver[read_start:read_end]assert " } catch {" in read_bodyawait_pos = hook.index("await getPermissionOptions?.(workspaceId)")parent_try = hook.index(" try {", hook.index("async function applyRuleset"))parent_catch = hook.index(" } catch (err) {", parent_try)update_try = hook.index(" try {", parent_catch)update_catch = hook.index(" } catch (err) {", update_try)assert not (parent_try < await_pos < parent_catch)assert not (update_try < await_pos < update_catch)print("workspace read failures are caught internally")print("permission-option await remains outside applyRuleset try/catch")PYRepository: chriswritescode-dev/opencode-forge
Length of output: 7433
Handle failures from getPermissionOptions.
The workspace lookup catches read errors, but the awaited resolver remains outside a try block. If it rejects, applyRuleset skips session.update, so the session does not receive structural denies. Catch the rejection, log it, and build the default ruleset.
🤖 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 `@src/hooks/loop-permission.ts` at line 95, Update applyRuleset around
getPermissionOptions so resolver rejections are caught instead of bypassing
session.update. Log the failure, then build the default ruleset from empty
options and continue applying it, while preserving the existing successful
resolver path.
| const { createBuiltinWorktreeWorkspace, getForgeWorkspaceEntry } = await import('../workspace/forge-worktree') | ||
| const previousEntry = stoppedState.workspaceId ? await getForgeWorkspaceEntry(deps.client, stoppedState.workspaceId) : undefined | ||
| const wsResult = await createBuiltinWorktreeWorkspace(deps.client, { | ||
| loopName: stoppedState.loopName, | ||
| directory: stoppedState.projectDir || ctx.directory, | ||
| extra: previousEntry?.extra ?? undefined, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Inspect how createBuiltinWorktreeWorkspace consumes the `extra` parameter.
rg -n -C25 'export async function createBuiltinWorktreeWorkspace' src/workspace/forge-worktree.ts
rg -n -C5 'startRef|syncRef|gitRemote' src/workspace/forge-worktree.tsRepository: chriswritescode-dev/opencode-forge
Length of output: 2745
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- workspace creation implementation ---'
sed -n '163,235p' src/workspace/forge-worktree.ts
printf'%s\n''--- workspace entry definitions and field usage ---'
rg -n -C8 'interface .*Workspace|type .*Workspace|startRef|syncRef|gitRemote|workspace\.create' src/workspace src/services src -g '*.ts' -g '*.tsx'| head -n 240
printf'%s\n''--- restart call sites ---'
sed -n '1845,1910p' src/services/execution.tsRepository: chriswritescode-dev/opencode-forge
Length of output: 24665
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- workspace entry retrieval and extra propagation ---'
rg -n -C18 'getForgeWorkspaceEntry|ForgeWorkspaceEntry|extra:' src/workspace/forge-worktree.ts src -g '*.ts'| head -n 260
printf'%s\n''--- adapter creation path ---'
sed -n '245,325p' src/workspace/forge-adapter.ts
printf'%s\n''--- adapter registration and workspace info mapping ---'
sed -n '1,215p' src/workspace/forge-adapter.ts
printf'%s\n''--- static verifier for the restart data flow ---'
python3 - <<'PY'from pathlib import Pathrestart = Path('src/services/execution.ts').read_text()creator = Path('src/workspace/forge-worktree.ts').read_text()adapter = Path('src/workspace/forge-adapter.ts').read_text()checks = { 'restart forwards previous extra': 'extra: previousEntry?.extra ?? undefined' in restart, 'creator spreads caller extra': '...options.extra' in creator, 'adapter reads startRef from extra': 'extra.startRef' in adapter, 'adapter reads syncRef from extra': 'extra.syncRef' in adapter, 'adapter reads gitRemote from extra': 'extra.gitRemote' in adapter, 'adapter uses startRef for branch creation': 'const startPoint = pin && !branchExists ? pin.startRef : undefined' in adapter,}for name, result in checks.items(): print(f'{name}: {result}')PYRepository: chriswritescode-dev/opencode-forge
Length of output: 33262
Do not copy stale Git refs into the recreated workspace. Filter startRef, syncRef, and gitRemote from previousEntry.extra, or recompute them for the new launch. forge-adapter treats startRef as authoritative and can fetch the stale ref, use it as the branch base, or reject the existing branch when its tip differs.
🤖 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 `@src/services/execution.ts` around lines 1863 - 1868, Update the workspace
recreation flow around createBuiltinWorktreeWorkspace so previousEntry.extra
does not reuse stale startRef, syncRef, or gitRemote values. Filter those fields
out before passing extra, or recompute them from the new launch context, while
preserving unrelated extra metadata.
| /** | ||
| * Extra `deny` rules layered over Forge's structural denies for loop, audit, and post-action | ||
| * sessions. Entries are applied before Forge's structural denies, so a user rule for a permission | ||
| * that Forge manages or that the loop requires is rejected. Use `external_directory` allow entries | ||
| * via `allowExternalDirectories` instead, which Forge manages for every session. | ||
| */ | ||
| permissions?: LoopPermissionsConfig |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the doc comment for scoped denies of required permissions.
The comment states that any rule naming a permission the loop requires is rejected. parseLoopPermissionRules rejects only blanket denies (pattern *) of FORGE_REQUIRED_PERMISSIONS. A scoped deny such as { permission: 'bash', pattern: 'git push *' } is honoured, and test/constants/loop-permissions-config.test.ts asserts this.
📝 Proposed doc fix
/**
* Extra `deny` rules layered over Forge's structural denies for loop, audit, and post-action
* sessions. Entries are applied before Forge's structural denies, so a user rule for a permission
- * that Forge manages or that the loop requires is rejected. Use `external_directory` allow entries- * via `allowExternalDirectories` instead, which Forge manages for every session.+ * that Forge manages is rejected, as is a blanket (`*`) deny of a permission the loop requires;+ * a scoped deny such as `{ permission: 'bash', pattern: 'git push *' }` is honoured. Use+ * `allowExternalDirectories` for `external_directory` grants, which Forge manages for every session.
*/
permissions?: LoopPermissionsConfig📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| *Extra`deny`ruleslayeredoverForge's structural denies for loop, audit, and post-action | |
| *sessions.EntriesareappliedbeforeForge's structural denies, so a user rule for a permission | |
| *thatForgemanagesorthatthelooprequiresisrejected.Use`external_directory`allowentries | |
| *via`allowExternalDirectories`instead,whichForgemanagesforeverysession. | |
| */ | |
| permissions?: LoopPermissionsConfig | |
| /** | |
| *Extra`deny`ruleslayeredoverForge's structural denies for loop, audit, and post-action | |
| *sessions.EntriesareappliedbeforeForge's structural denies, so a user rule for a permission | |
| *thatForgemanagesisrejected,asisablanket(`*`)denyofapermissionthelooprequires; | |
| *ascopeddenysuchas `{ permission: 'bash', pattern: 'git push *' }` ishonoured.Use | |
| *`allowExternalDirectories`for`external_directory`grants,whichForgemanagesforeverysession. | |
| */ | |
| permissions?: LoopPermissionsConfig |
🤖 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 `@src/types.ts` around lines 98 - 104, Update the doc comment above
LoopPermissionsConfig to state that only blanket `*` denies for Forge-required
permissions are rejected, while scoped denies such as `bash` with a command
pattern remain honored. Preserve the existing guidance about Forge-managed
permissions and `allowExternalDirectories`.
| let rules: PermissionRule[] | ||
| try { | ||
| const entry = await getForgeWorkspaceEntry(client, workspaceId) | ||
| rules = entry ? getForgeWorkspacePermissionRules(entry) : [] | ||
| } catch { | ||
| rules = [] | ||
| } | ||
| perClient.set(workspaceId, rules) | ||
| return rules | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not cache the empty result when the workspace lookup throws.
The catch block sets rules = [] and line 31 stores that value in the cache. A transient failure of getForgeWorkspaceEntry (network error, server restart) therefore pins an empty portable-rule set for that client and workspace for the rest of the process. Every later loop, audit, and post-action session for that workspace then drops the persisted deny rules, which widens permissions silently. Cache only successful lookups.
🛡️ Proposed fix
- let rules: PermissionRule[]
try {
const entry = await getForgeWorkspaceEntry(client, workspaceId)
- rules = entry ? getForgeWorkspacePermissionRules(entry) : []+ const rules = entry ? getForgeWorkspacePermissionRules(entry) : []+ perClient.set(workspaceId, rules)+ return rules
} catch {
- rules = []+ // Do not cache a lookup failure: a transient error must not permanently+ // drop the workspace's portable deny rules.+ return []
}
- perClient.set(workspaceId, rules)- return rules📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| letrules: PermissionRule[] | |
| try{ | |
| constentry=awaitgetForgeWorkspaceEntry(client,workspaceId) | |
| rules=entry ? getForgeWorkspacePermissionRules(entry) : [] | |
| }catch{ | |
| rules=[] | |
| } | |
| perClient.set(workspaceId,rules) | |
| returnrules | |
| } | |
| try{ | |
| constentry=awaitgetForgeWorkspaceEntry(client,workspaceId) | |
| construles=entry ? getForgeWorkspacePermissionRules(entry) : [] | |
| perClient.set(workspaceId,rules) | |
| returnrules | |
| }catch{ | |
| // Do not cache a lookup failure: a transient error must not permanently | |
| // drop the workspace's portable deny rules. | |
| return[] | |
| } | |
| } |
🤖 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 `@src/utils/loop-permission-options.ts` around lines 24 - 33, Update the lookup
flow around getForgeWorkspaceEntry so exceptions return the empty rules result
without executing perClient.set. Only cache rules after a successful workspace
lookup, including the valid empty result when no rules are configured, and
preserve returning [] for lookup failures.
| export function getForgeWorkspacePermissionRules(entry: Pick<ForgeWorkspaceEntry, 'extra'>): PermissionRule[] { | ||
| const raw = entry.extra?.permissionRules | ||
| if (!Array.isArray(raw)) return [] | ||
| return (raw as unknown[]).filter( | ||
| (r): r is PermissionRule => | ||
| typeof r === 'object' && r !== null && | ||
| typeof (r as PermissionRule).permission === 'string' && | ||
| typeof (r as PermissionRule).pattern === 'string' && | ||
| ((r as PermissionRule).action === 'allow' || (r as PermissionRule).action === 'deny'), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject non-deny portable workspace rules.
This helper accepts a well-formed allow rule from workspace metadata. buildLoopPermissionRuleset() applies these rules after the blanket external_directory deny. A stale or modified workspace entry can therefore restore external-directory access for later loop sessions and bypass the configuration validation.
Persist and consume only canonical validated deny rules. Reject Forge-managed permissions during workspace-rule extraction. Add a regression test for { permission: 'external_directory', pattern: '*', action: 'allow' }.
🤖 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 `@src/workspace/forge-worktree.ts` around lines 64 - 74, Update
getForgeWorkspacePermissionRules to accept only canonical validated deny rules,
rejecting any rule with action "allow" and Forge-managed permissions such as
external_directory. Preserve validation of the rule shape and add a regression
test covering { permission: 'external_directory', pattern: '*', action: 'allow'
} to ensure it is excluded before buildLoopPermissionRuleset consumes the rules.
| const externalAllows = createArgs.permission.filter( | ||
| (r: { permission: string; action: string }) => | ||
| r.permission === 'external_directory' && r.action === 'allow', | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the filter predicate.
The first callback returns the second callback function. Functions are truthy, so filter() retains every permission rule. Remove the duplicate arrow function so externalAllows contains only external_directory allow rules.
Proposed fix
const externalAllows = createArgs.permission.filter(
(r: { permission: string; action: string }) =>
- (r: { permission: string; action: string }) =>
r.permission === 'external_directory' && r.action === 'allow',
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexternalAllows=createArgs.permission.filter( | |
| (r: {permission: string;action: string})=> | |
| r.permission==='external_directory'&&r.action==='allow', | |
| ) | |
| constexternalAllows=createArgs.permission.filter( | |
| (r: {permission: string;action: string})=> | |
| r.permission==='external_directory'&&r.action==='allow', | |
| ) |
🤖 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 `@test/utils/tui-remote-launch.test.ts` around lines 600 - 603, Fix the filter
predicate used to build externalAllows by removing the duplicate arrow function
and ensuring the callback directly evaluates each rule’s permission and action
checks. Preserve the intended result of retaining only external_directory rules
with action allow.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds
loop.permissions— per-toolallow/denypermission overrides for loop, audit, and post-action sessions, layered between the external-directory allow rules and Forge's structural denies.Behavior
allowentries are processed first, thendeny; on tiesdenywins. Duplicate entries are dropped.*) or an object{ permission, pattern }.external_directorydeny → external-directory allows → configuredallow→ configureddeny→ Forge structural denies.Forge-managed permissions are rejected
*,external_directory, and every structural deny (plan,plan_enter,plan_exit,plan-write,plan-edit,execute-plan,execute-goal,question,loop-cancel,loop-status,launch-group,group-status,group-cancel,review-write,review-delete,edit,write,multiedit,apply_patch) are rejected with a log warning and a one-time TUI toast.Notes
loop.allowExternalDirectories(host-specific paths are not portable to a remote server).loop.permissionsto the singleFORGE_MANAGED_PERMISSIONSlist insrc/constants/loop.ts; both permission rulesets derive their structural denies from the shared name lists.Validation
pnpm typecheck && pnpm lint && pnpm test && pnpm build— all green (inclusive of newloop-permissions-configand updated ruleset/runtime tests).Summary by CodeRabbit
New Features
Documentation
Tests