fix(agent): end the additional_permissions retry loop

[Vasanthdev2004]

Summary

git branch --show-current — an ordinary read-only command — could leave the agent bouncing between two rejections instead of running, burning turns without converging.

Both errors come from inlineAdditionalPermissionsProfile, and each names only the fix that adds something. One pushes toward setting the flag, the other back toward requesting a permission the command doesn't need. Neither ever says to omit the field, which is the correct action here — so together they form a two-state oscillator. Every rejection now carries that remedy.

The schema half is why the object gets attached at all: neither field description said to omit them for ordinary commands, so a model that legitimately used with_additional_permissions once had nothing telling it to stop. Both descriptions now lead with "omit", and they're shared constants instead of being duplicated verbatim across bash.go and exec_command.go.

This does not loosen the sandbox

Nothing previously refused is now accepted on the strength of an empty or mismatched payload, and sandbox_permissions is never upgraded on the model's behalf. Only the guidance changed.

The one behavioural change: an explicit additional_permissions: null is treated as the omitted field, since a null grants nothing and carries no flag. That flips three shapes from reject to execute — null with use_default, with require_escalated, and with no mode at all. The escalated case still routes through the normal escalation prompt and is not a new capability; the identical request without the null key already behaved that way. A flagged null still errors, so with_additional_permissions + null isn't silently downgraded.

Note on versions

The reporter was on 0.6.0, where both legs of the loop were live. On current main, #838's normalizeNoopAdditionalPermissions already absorbs the empty payload, so only the first error reproduces today. Both legs were reproduced and fixed.

Test plan

• TestAdditionalPermissionsRejectionsConverge replays exec_command repeatedly requests incorrect sandbox permissions and fails before using the required network permission #845's step-1/step-2 sequence and Sandbox tool invocation fails when additional_permissions is empty or mismatched #847's two inconsistent states in one test
• TestExecuteToolCallMissingAdditionalPermissionsErrorIsActionable, TestEmptyAdditionalPermissionsErrorNamesOmitRemedy pin that the errors name the omit remedy
• TestExecuteToolCallTreatsNullAdditionalPermissionsAsOmitted, TestExecuteToolCallRejectsFlaggedNullAdditionalPermissions cover the null split
• Reverting only internal/agent/loop.go turns all five red on message text, and reproduces both issue strings verbatim
• go build ./..., go vet, gofmt clean; internal/agent, internal/tools, internal/sandbox green

Fixes #845
Fixes #847

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of optional sandbox permission fields, including explicit null values.
  • Clarified validation errors and retry guidance when unnecessary or invalid permissions are provided.

• Documentation

  • Updated command-line tool guidance with valid permission combinations and examples.
  • Clearly indicates when permission fields should be omitted, including for read-only commands.

• Tests

  • Added coverage for permission validation, error consistency, null handling, and successful retries.

[coderabbitai]

Walkthrough

The change normalizes explicit null additional permissions as omitted when unflagged, standardizes validation guidance, and shares permission descriptions across Bash and exec_command. Tests cover invalid payloads, successful retries, null handling, and schema consistency.

Changes

Permission validation and tool guidance

Layer / File(s)	Summary
Normalize and validate permission payloads internal/agent/loop.go, internal/agent/additional_permissions_validation_test.go	Explicit null payloads are omitted when unflagged. Missing, unscoped, and empty payloads return shared omission-and-retry guidance. Regression tests cover rejection and successful retry behavior.
Share permission schema descriptions internal/tools/args.go, internal/tools/bash.go, internal/tools/exec_command.go, internal/tools/sandbox_permissions_schema_test.go	Bash and exec_command use identical shared descriptions for permission fields. Tests verify omission guidance, valid combinations, rejected shapes, and read-only commands.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• Gitlawb/zero#838: Both changes modify additional-permission normalization and validation in internal/agent/loop.go and related tests.

Suggested reviewers: anandh8x, euxaristia, gnanem1990

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary fix: preventing the additional_permissions retry loop.
Linked Issues check	✅ Passed	The changes address read-only command failures, inconsistent permission payloads, null handling, retry convergence, and valid permission requests from issues [#845] and [#847].
Out of Scope Changes check	✅ Passed	The schema updates, shared descriptions, validation changes, and regression tests directly support the linked issue objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/845-additional-permissions-loop

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/tools/args.go`:
- Around line 152-155: Update sandboxPermissionsDescription so omission is
described as appropriate only for commands that require no additional sandbox
access, rather than implying all read-only commands can omit the override.
Preserve the guidance for using with_additional_permissions with non-empty
additional_permissions when read-only commands need sandboxed network or file
access.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 96535456-6df2-423b-9144-b7e9716c8509

📥 Commits

Reviewing files that changed from the base of the PR and between 8e26679 and ac46828.

📒 Files selected for processing (6)

• internal/agent/additional_permissions_validation_test.go
• internal/agent/loop.go
• internal/tools/args.go
• internal/tools/bash.go
• internal/tools/exec_command.go
• internal/tools/sandbox_permissions_schema_test.go

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not state that every read-only command needs no sandbox override.

Line 153 conflicts with commands that are read-only but require sandboxed network access, such as gh issue view. This wording can cause the agent to omit the required network permission. Describe omission as appropriate only when the command needs no extra sandbox access.

Proposed fix

- sandboxPermissionsDescription = "Per-command sandbox override. Omit it for ordinary commands, including every " +
-     "read-only one; use `with_additional_permissions` with a non-empty `additional_permissions` for sandboxed " +
+ sandboxPermissionsDescription = "Per-command sandbox override. Omit it when the command needs no extra sandbox " +
+     "access, such as `git branch --show-current`; use `with_additional_permissions` for sandboxed " +

📝 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.

Suggested change

	sandboxPermissionsDescription = "Per-command sandbox override. Omit it for ordinary commands, including every " +
	"read-only one; use `with_additional_permissions` with a non-empty `additional_permissions` for sandboxed " +
	"file/network access, or `require_escalated` only when the command must run outside the sandbox, such as " +
	"host/global process, socket, service, or desktop state hidden by sandbox namespaces."
	sandboxPermissionsDescription = "Per-command sandbox override. Omit it when the command needs no extra sandbox " +
	"access, such as `git branch --show-current`; use `with_additional_permissions` for sandboxed " +
	"file/network access, or `require_escalated` only when the command must run outside the sandbox, such as " +
	"host/global process, socket, service, or desktop state hidden by sandbox namespaces."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/tools/args.go` around lines 152 - 155, Update
sandboxPermissionsDescription so omission is described as appropriate only for
commands that require no additional sandbox access, rather than implying all
read-only commands can omit the override. Preserve the guidance for using
with_additional_permissions with non-empty additional_permissions when read-only
commands need sandboxed network or file access.

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: ac4682886316
Changed files (6): internal/agent/additional_permissions_validation_test.go, internal/agent/loop.go, internal/tools/args.go, internal/tools/bash.go, internal/tools/exec_command.go, internal/tools/sandbox_permissions_schema_test.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[Vasanthdev2004]

@jatmn could you take this one when you have a moment? It closes #845 and #847, which are the same bug — #847 was actually filed by the agent while it was stuck in the loop the issue describes.

The part worth your eye is the null handling, since it's the only behavioural change and it touches the escalation path. An explicit additional_permissions: null is now treated as the omitted field, on the reasoning that a null grants nothing and carries no flag. That flips three shapes from reject to execute: null with use_default, with require_escalated, and with no mode at all. The escalated one is the one to push on — it now reaches the normal escalation prompt, and if the user approves, runs outside the sandbox. My read is that this isn't a new capability, because the identical request without the null key already behaved exactly that way, so the null was only ever a spurious rejection. But that's a judgement about a sandbox boundary and I'd rather you checked it than took my word.

A flagged null still errors, deliberately — with_additional_permissions plus null must not be silently downgraded into a different outcome.

Everything else is guidance-only: the two rejection messages now name "omit the field" as a remedy, which neither did, and the schema descriptions lead with omit so the object stops being attached to ordinary commands in the first place. Nothing previously refused is accepted now, and sandbox_permissions is never upgraded on the model's behalf.

Reverting only internal/agent/loop.go turns all five new tests red on message text and reproduces both issue strings verbatim, so the loop is pinned in both directions.

One version note so the repro doesn't confuse you: the reporter was on 0.6.0 where both legs of the loop were live. On current main, #838's normalizeNoopAdditionalPermissions already absorbs the empty payload, so only the first error reproduces today. Both were reproduced and fixed.