[patextreme]

Problem

PI_SANDBOX=true only auto-allowed commands that matched no rule. Commands hitting an ask rule — git push, git rebase, and any non-read-only gh command (rules 1, 2 and 21) — still prompted, and blocked outright in headless runs (-p, --mode json) with ... - no UI for confirmation. Inside a sandbox there is nothing for those prompts to protect, so they were pure friction.

Change

• extensions/permission/index.ts: the sandbox check moves into the ask branch, ahead of always-allow/no-UI/prompting. Written generically (ask → allow under PI_SANDBOX=true) rather than listing rule IDs, so future ask rules inherit the same semantics and rules.ts stays the single source of rule data.
• deny rules still block in the sandbox — only YOLO mode bypasses them. This is covered by a test so the sandbox cannot silently swallow a deny.
• Docs: README.md rule list and the AGENTS.md sandbox bullet now describe the wider sandbox behavior.

Net effect: in the sandbox every bash command either proceeds silently or is denied; no prompt, no headless block.

Tests

extensions/permission/index.test.ts:

• auto-allows an ask rule in a sandbox without prompting — rules 1/2/21, asserts no prompt and no herdr:blocked events
• auto-allows an ask rule in a sandbox even without UI — the previously blocked headless case
• still blocks a deny rule in a sandbox — guard against sandbox swallowing deny

Verification

npm run check, npm run typecheck, npm test — 443 tests, 0 failures. No package*.json or nix/ changes, so no lockfile or npmDepsHash impact.

[patextreme]

Review complete — limited coverage, no repair-worthy findings

Standards axis only. The Spec axis was skipped at your explicit direction: this PR has no originating issue and no OpenSpec change, so there is no spec to review against. Full diff reviewed at 38cde107.

No blockers and no significant bugs. 7 findings were recorded: 0 repair-worthy, 6 report-only, 1 refuted. Nothing was repaired, so there are no new commits and no push.

Report-only worth your attention

• F-001 — a sandbox now auto-allows git push, git rebase, and non-read-only gh commands on a bare PI_SANDBOX=true assertion that Pi itself never verifies. This is the PR's documented intent, so nothing was changed; narrowing it would be new product intent.
• F-003 — the ask bullet at README.md:137 still advertises the "Always allow" option without a sandbox carve-out, though that option is unreachable in a sandbox.
• F-002 / F-004 — the new sandbox-deny test duplicates the existing deny fixture and shares its mutable unshift/shift scaffolding.
• F-007 — pre-existing: no built-in deny rules ship, so "deny still blocks" is only reachable by editing rules.ts.

Validation: npm test 443 tests / 442 pass / 1 self-skipped (PI_FACTORY_SKILLS_OUTPUT tier), tsc --noEmit and biome check . clean, required CI green at 38cde107. nix flake check was not required — no package*.json and no nix/ change.

Scope: Standards axis only, and no live Pi runtime, Herdr signal, or real PI_SANDBOX provisioning was exercised; evidence is static reading + unit tests + CI at a single SHA.

Reviewed: 38cde107 · Repairs: none · Attempts: 0/10 · Human input: none required (Q-1 is an informational acknowledgement only)

Full review reports, finding ledger, and audit details

Standards report (complete, verbatim)

Sources read: AGENTS.md, README.md (§ Permission Extension), docs/adr/*, biome.json, tsconfig.json, package.json, tests/run.mjs, nix/modules/pi-package.nix, .github/workflows/ci.yml, plus extensions/permission/index.ts, rules.ts, index.test.ts in full.

Hard violations: none found. Verified against the documented standards:

• Test placement rule ("unit tests live beside their extension module") — the new cases live in extensions/permission/index.test.ts, discovered by tests/run.mjs with no registration edit (confirmed: npm test lists extensions/permission/index.test.ts).
• "Every extension must have a flake output" — unchanged; pi-permission copies only index.ts/rules.ts, so co-located tests correctly ship nowhere.
• Doc parity — AGENTS.md:74 and README.md:147 both now state ask rules auto-allow in sandbox and deny still blocks, matching the code exactly. openspec/specs/permission-yolo/spec.md is untouched and still correct (it says YOLO bypasses deny "regardless of … sandbox"; no YOLO behavior changed).
• Formatting/style — biome check . clean, so the single-line if (isSandbox) return undefined; collapse at index.ts:162 is the formatter-preferred shape.

Judgement calls (baseline smells):

1. Duplicated Code — index.test.ts:318-337 ("still blocks a deny rule in a sandbox") is a near-verbatim copy of the pre-existing index.test.ts:243-262 ("does not emit for a deny rule"); only sandbox: true and id: -2 differ, and the unshift/finally { shift() } fixture is copy-pasted:

   it("still blocks a deny rule in a sandbox", async () => {
     PERMISSION_RULES.unshift({
       id: -2,
       regex: /\bblocked-cmd\b/,
       action: "deny",
     });

   Extending the existing test to loop over [{ sandbox: false }, { sandbox: true }] would prove the same thing with one fixture.

2. Primitive Obsession / Mysterious Name (mild) — id: -1 / id: -2 are hand-assigned negative sentinels whose only meaning is "not a real rule id". A withRule(rule, run) helper (or a named TEST_DENY_RULE fixture) would remove both the magic numbers and the duplication in (1).

3. Duplicated intent between comment and docs — the inline comment at index.ts:123-126 restates README.md:147 almost word for word. Defensible (the code is the enforcement point), not flagged as a violation.

4. The index.ts:158-162 reformat is unrelated cosmetic churn in the same commit; harmless.

Behavioral risks

Separate from Standards; listed in descending severity. No ordering defect found on the documented invariants.

1. Sandbox now authorizes remote-mutating commands (design consequence, documented). extensions/permission/index.ts:126 returns undefined for every ask match when isSandbox. Trigger: PI_SANDBOX=true + git push --force origin main, git rebase -i, or gh pr merge 42. Consequence: no prompt, no Herdr blocked signal, and remote state is mutated and credentials are exercised — effects that a filesystem/process sandbox does not contain. The PR's own README.md:147 enumerates this, and README.md:116 already warns "read-only delegation is not an OS sandbox"; PI_SANDBOX remains an unverified env assertion (index.ts:27). This is the change's stated intent, so it is a posture risk to accept deliberately, not a bug — but it is a materially larger blast radius than the previous "unmatched commands only" rule, because the built-in ask rules are exactly the destructive ones.

2. PI_SANDBOX is resolved once at factory time (index.ts:27), so the new auto-allow silently fails open/closed depending on load order. Trigger: a harness or wrapper that sets PI_SANDBOX after extension factories run (the extension's own comments at index.ts:41-43 note that flag values are applied after factories, which is why yoloPinned() is lazy). Consequence: ask rules keep prompting (or, if the env is later cleared, keep auto-allowing) contrary to the operator's intent. Pre-existing shape, but this PR raises the cost of getting it wrong. Not evidenced as an actual harness defect — flagged for a judge.

3. Untested branch: always-allow bookkeeping in a sandbox. In sandbox mode the alwaysAllowed.has(rule.id) check (index.ts:128) and the "Always allow" prompt option are now unreachable, so /permission-list-always-allow is permanently empty there. README.md:140 still describes ask as "prompt the user for confirmation (with a 'Always allow' option)" without a sandbox carve-out; the following paragraph implies it, but no test asserts that always-allow state is simply never recorded in a sandbox. No correctness harm found — the mode is process-fixed, so there is no path where stale state matters.

4. Test-fixture fragility (latent). index.test.ts:319-336 mutates the shared exported PERMISSION_RULES array with unshift/shift. Two tests now do this (id: -1, id: -2) with the same \bblocked-cmd\b regex. Under node:test's default serial in-file execution both pass (verified), but enabling describe/it concurrency would make the finally { shift() } remove the wrong element and silently drop the other fixture. Low risk, cheap to harden.

5. Invariants confirmed preserved (evidence: code order + tests): YOLO still returns before any rule/sandbox check (index.ts:105); deny still returns at index.ts:114-119, before the new isSandbox return at line 126 — covered by the new sandbox deny test; both Herdr emits remain inside whileBlocked around only the two prompt sites, so skipping prompts in a sandbox cannot unbalance the { active: true }/{ active: false } pairs; no-UI sandbox ask no longer blocks (new test), while no-UI non-sandbox ask still blocks (existing test unchanged).

Spec report

Skipped — explicitly authorized by the user. No originating issue exists (the PR body has a ## Problem section but no linked/closing issue; the repository has no open issues), no openspec label is applied, and no OpenSpec change covers permissions (openspec/changes/ holds continue-cleanup-quiescence and generalize-pr-base-branch; the only permission-related change, 2026-09-15-permission-yolo-toggle, is archived history). The change's intent could only be checked against the PR body, the README.md/AGENTS.md edits made in the same commit, and the live openspec/specs/permission-yolo/spec.md, which constrains YOLO/deny semantics only and does not specify sandbox behavior for ask rules. Code and both doc surfaces agree.

Judge ledger

Prior ledger: empty, confirmed. gh api --paginate returned 0 issue comments, 0 reviews, and 0 inline review comments on PR #92, so there were no prior actionable IDs to reconcile, no recurring findings, and no duplicate reservations.

State verified by the judge: pinned base == merge-base 6b4820b8; reviewed head == local HEAD == PR headRefOid 38cde1072; worktree clean; diff 4 files, +56/−6; run identity 38cde107…, round 1, repairs used 0/10.

F-001 — Sandbox auto-allow of ask rules authorizes remote-mutating commands on an unverified PI_SANDBOX assertion

• ID / provenance: F-001; Standards. Spec: n/a (skipped by authorization).
• Category: report-only — deliberate, documented design/security-posture decision (see Q-1).
• Evidence: extensions/permission/index.ts:126 if (isSandbox) return undefined; inside the ask branch; index.ts:27 isSandbox = process.env.PI_SANDBOX === "true". The built-in ask rules are the destructive ones: extensions/permission/rules.ts:11-12 (git push, git rebase), :40 (gh ). PI_SANDBOX appears zero times anywhere under node_modules/@earendil-works/; repo-wide it appears only at AGENTS.md:74, README.md:145/147, index.ts:27, and the test harness. Intent is stated in the PR body ("Inside a sandbox there is nothing for those prompts to protect"), README.md:147, AGENTS.md:74.
• Impact: Trigger PI_SANDBOX=true + git push --force origin main / git rebase -i / gh pr merge 42. Consequence: no prompt, no Herdr blocked signal, remote state mutated, credentials exercised — effects outside any filesystem/process containment. Real, but it is the change's declared purpose and the operator sets the flag; the previous rule was strictly narrower. Intentional by design rather than a defect in the judged diff.
• Correction: None recommended under this PR's intent. A narrower rule (sandbox auto-allow limited to non-remote-mutating commands, or a distinct opt-in for remote mutations) would be new product intent / a security-posture change → human input, not a repair.
• Disposition: open (report-only, accepted as documented intent).
• Resolution evidence: Code and docs at 38cde10 are mutually consistent (index.ts:126 ↔ README.md:147 ↔ AGENTS.md:74 ↔ PR body). No prior occurrence.

F-002 — Duplicated deny-rule test fixture with hand-assigned negative sentinel ids

• ID / provenance: F-002; Standards (reviewer judgement calls 1 + 2, merged: same root cause — no rule-fixture helper).
• Category: report-only — smell without material consequence.
• Evidence: index.test.ts:243-262 ("does not emit for a deny rule", id: -1) vs :318-337 ("still blocks a deny rule in a sandbox", id: -2) — identical PERMISSION_RULES.unshift({…, regex: /\bblocked-cmd\b/, action: "deny"}) / finally { PERMISSION_RULES.shift() } scaffolding and identical expected reason; only sandbox: true and the id differ.
• Impact: Not applicable to runtime. Maintenance risk only: two copies of one fixture must be updated together, and -1/-2 carry no meaning beyond "not a real rule id". Both tests pass.
• Correction: Optional consolidation — loop the existing test over [{ sandbox: false }, { sandbox: true }], or add a withRule(rule, run) helper / named TEST_DENY_RULE. Pure refactor of index.test.ts; cheap but does not meet the repair bar.
• Disposition: open (report-only).
• Resolution evidence: Both fixtures present and passing at 38cde10; no prior occurrence.

F-003 — Sandbox carve-out documented in a separate paragraph; always-allow interaction undocumented and untested

• ID / provenance: F-003; Standards (reviewer behavioral risk 3 + doc completeness).
• Category: report-only — minor documentation gap; no correctness or contract harm.
• Evidence: README.md:137 "ask — prompt the user for confirmation (with a 'Always allow' option)" carries no sandbox qualifier; the qualifier arrives two blocks later at README.md:147. In sandbox mode alwaysAllowed.has (index.ts:128), alwaysAllowed.add (:147), and the "Always allow" choice (:142) are unreachable, so /permission-list-always-allow (index.ts:193) always reports "No commands are currently always-allowed." No test asserts that sandbox mode never records always-allow state.
• Impact: Trigger: an operator reads only the Actions list, or runs /permission-list-always-allow inside a sandbox. Consequence: mild expectation mismatch. No stale state is possible because isSandbox is process-fixed, so there is no behavioral hazard.
• Correction: Optional docs-only edit — add "outside a sandbox" to the ask bullet and one sentence that sandbox mode records no always-allow state. Not repair-worthy (cosmetic documentation completeness).
• Disposition: open (report-only).
• Resolution evidence: README.md:137/145/147 and index.ts:126-149/193-205 as of 38cde10.

F-004 — Shared mutable PERMISSION_RULES fixture mutation is concurrency-fragile

• ID / provenance: F-004; Standards (reviewer behavioral risk 4).
• Category: report-only — speculative hardening; latent only.
• Evidence: index.test.ts:244 PERMISSION_RULES.unshift(...) + :261 finally { PERMISSION_RULES.shift() }, now duplicated at :319/:336 with the same \bblocked-cmd\b regex. PERMISSION_RULES is the exported module-level literal (rules.ts:10). Neither describe nor it opts into concurrency (no concurrency option in the file), so node:test runs these subtests serially and npm test passes.
• Impact: Would require enabling describe/it concurrency; then shift() removes the wrong element and silently drops the sibling deny fixture. Not reachable under the current test configuration; no runtime consequence.
• Correction: Optional hardening via the F-002 helper or a local rules-array injection point. Do not repair here (speculative).
• Disposition: open (report-only).
• Resolution evidence: Serial execution confirmed by absence of a concurrency option and a green npm test at 38cde10.

F-005 — REFUTED: "PI_SANDBOX resolved once at factory time causes fail-open/fail-closed by load order"

• ID / provenance: F-005; Standards (reviewer behavioral risk 2).
• Category: refuted as a PR-caused defect.
• Evidence: index.ts:27 is unchanged from base (git show 6b4820b:extensions/permission/index.ts:27 is the identical line; base used it for the unmatched path), so the shape is not introduced here. The lazy-read rationale the reviewer cites (index.ts:40-44) is explicitly about pi.registerFlag/getFlag ("flag values are applied after extension factories run"), not about process.env. PI_SANDBOX is process-level and set before the process starts; it appears nowhere in the pi SDK, so no runtime path assigns it after factories. The in-repo consumer (index.test.ts:90-99) correctly sets the env before invoking the factory.
• Impact: No demonstrated trigger. The only conceivable one is out-of-repo tooling mutating the parent process environment after extension load, which cannot change process.env of an already-running Pi process.
• Correction: None. Residual external-tooling question tracked as U-1.
• Disposition: refuted.
• Resolution evidence: Base-vs-head comparison of index.ts:27; SDK grep; index.test.ts:90-99.

F-006 — Unrelated cosmetic reformat of the unmatched-command sandbox return

• ID / provenance: F-006; Standards (reviewer judgement call 4).
• Category: report-only — no action.
• Evidence: index.ts:161-162 collapses a four-line if (isSandbox) { return undefined; } block into if (isSandbox) return undefined; while editing the adjacent comment. biome check . is clean, so this is the formatter/idiom-consistent shape (matching :111, :126, :162).
• Impact: Not applicable — semantics identical (same condition, same return). Diff noise only, inside the same function the PR edits.
• Correction: None. Reverting would create churn without benefit.
• Disposition: open (report-only, no action).
• Resolution evidence: Biome clean at 38cde10; behavior covered by the sandbox/unmatched tests.

F-007 — Pre-existing: the "deny still blocks" safety net ships zero built-in deny rules

• ID / provenance: F-007; Standards (judge's own verification; not PR-introduced).
• Category: report-only — pre-existing condition, not caused by this diff.
• Evidence: extensions/permission/rules.ts contains 0 action: "deny" entries; PERMISSION_RULES (rules.ts:10) is a hardcoded literal with no configuration loading anywhere in extensions/permission/*.ts (no settings/config/readFile/JSON.parse), and no other module adds rules. The new sandbox-deny test therefore injects a synthetic rule (index.test.ts:319, id: -2).
• Impact: Trigger: an operator reads README.md:147 or the PR body ("deny rules still block … covered by a test") and infers destructive commands are still blocked in a sandbox. No runtime consequence — no deny rule exists, so nothing is bypassed, and the enumerated built-in rule list honestly shows none. Unchanged by this PR (deny was already documented at base).
• Correction: None here. A separate change could state that no built-in deny rules ship. Out of this diff's intent.
• Disposition: open (report-only, pre-existing).
• Resolution evidence: rules.ts content at 38cde10; absence of configuration loading; base README.md already documented deny.

Unresolved investigations

• U-1 (from F-005) — whether any out-of-repo harness or wrapper exports/overrides PI_SANDBOX in a way that diverges from operator intent. No in-repo evidence either way: PI_SANDBOX has zero occurrences in the pi SDK and no repo documentation defines who sets it or what guarantees it carries. Next step: inspect the exporting harness, which is not in this repository. This is the trust assumption behind F-001, not a defect in this PR.
• U-2 — no live Pi runtime execution. All behavioral evidence comes from the in-file createHarness fake ExtensionAPI (index.test.ts:45-113); the real runtime's treatment of PI_SANDBOX, headless ctx.hasUI, and Herdr listener presence was not exercised. No live-model or live-tool checks were run, and no claim is made about them.

Both are disclosed as material coverage limitations; neither is uncertainty about whether an actionable finding is real, and neither blocks finishing.

Escalation questions

• Q-1 (informational, non-blocking — the PR already answers "yes"): should PI_SANDBOX=true authorize remote-mutating commands (git push --force, git rebase -i, gh pr merge) when Pi itself provides no sandbox and the flag is a bare operator assertion that appears nowhere in the pi SDK? If a narrower rule is wanted, that is new product intent for a human to decide, not a repair in this round. If the current answer stands, no action is required and F-001 stays report-only.

Audit record

• Run identity: pr-review-repair, run = 38cde1072ec7f4dfc2d3d56652d210b1529d9740, round 1, kind = summary. Repairs used 0/10; no repair-start or scope-amendment record exists because no repair was authorized.
• Refs: base main @ 6b4820b839647120e8e4232549c98165306244e8 (= merge-base); reviewed head = local HEAD = PR headRefOid = 38cde1072ec7f4dfc2d3d56652d210b1529d9740. Delivered head: not applicable (no repairs, no commits, no push).
• Diff: 4 files, +56/−6 — AGENTS.md (+1/−1), README.md (+2/−0), extensions/permission/index.test.ts (+46/−0), extensions/permission/index.ts (+7/−5).
• Check commands and results at 38cde10 (executed fresh by both the reviewer and the judge; no cached evidence was relied on):
  • npm test → exit 0; tests 443 / suites 18 / pass 442 / fail 0 / skipped 1. The skip is tests/gates/factory-skills.test.mjs:167 ("standalone Nix skills output contains the complete MIT notice"), self-skipped by { skip: !process.env.PI_FACTORY_SKILLS_OUTPUT }; that env var is provided by nix flake check's node-tests derivation.
  • ./node_modules/.bin/tsc --noEmit → exit 0, no output.
  • nix develop -c biome check . → exit 0, "Checked 49 files. No fixes applied."
  • gh pr checks 92 → CI pass at head 38cde107…; cross-confirmed via the check-runs API ({name: CI, conclusion: success, head_sha: 38cde107…}) as current-head evidence.
  • nix flake check → not run, not required: the diff changes no package*.json and no nix/ file, so the lockfile/npmDepsHash gate does not apply; nix/modules/pi-package.nix still copies only index.ts/rules.ts, so the enlarged test file ships nowhere.
• Skipped checks with reasons: Spec axis (no issue/spec exists; explicitly authorized skip); live runtime/Herdr/sandbox provisioning (no harness in this environment); full nix flake check (not applicable per the repo's own rule).
• Coverage limitations: Standards axis only; ADR bodies were not read by the reviewer, and the judge closed that gap (grep -rin sandbox docs/adr/ → no matches, so no ADR constrains sandbox behavior). No dependency or Nix package-build gate applied to this diff.
• Delivery state: review and judgment complete; no repairs, no commits, no push, no state mutation. Worktree clean at 38cde107 before and after review.
• Receipts: this comment is the round-1 record.