Safety envelope + Gemini-leverage layers (v0.15.0)#2
Conversation
…4.0) Researcher persona, 180s/1200s timeouts, source-first response format, docs + tests. Co-Authored-By: Claude <noreply@anthropic.com>
…ated apply, egress secret-scan Pass A of the nomor-iris consolidation. All flags default-OFF (existing research-mode/chain/session behavior byte-identical). New (opt-in): - --worktree: engine edits a throwaway git worktree, not the real tree. OS write-sandbox (macOS sandbox-exec / Linux bwrap) confines writes to the worktree; a fail-closed pre/post tamper-abort backstop catches real-tree/.git escapes even with no sandbox binary. - --diff-only: capture proposed.diff + sha256, never auto-apply. - record-verdict / apply / discard: apply refuses unless a HMAC-authenticated PASS verdict (per-run key, signs decision+sha) matches the current diff hash; strict git apply (no --3way). A tamper trip burns the session (wipe artifacts + key + breach sentinel) so a planted approval can't survive. - --secret-scan / --allow-secrets: scans the real egress surface (prompt + context files + add-dir tree, symlink-followed) before the model call. Containment boundary: the OS sandbox is the real guarantee; NO_SANDBOX / hosts without sandbox-exec|bwrap run best-effort (documented in SKILL.md). Hardened through 4 adversarial review rounds (cold-audit ∥ santa): closed forged-approval (decision-bound HMAC), worktree escape, untracked/ignored backstop gaps, no-sandbox key-plant + read-only-dir bypasses. Tests demonstrate each (fail-before/pass-after). Suite 102/104 (2 pre-existing head-shadow env fails). Co-Authored-By: Claude <noreply@anthropic.com>
…ontext-pack, mechanical validate
Pass B of the nomor-iris consolidation. All opt-in / additive; default-off
(non-ui, no new flags) prompt is byte-identical to Pass A.
- L1 open-brief persona (auto for ui/auto→ui): invites Gemini to own the visual
layer + explain rationale, so it uses its design taste instead of executing literally.
- W6a inspiration pointer (auto for ui when ~/.design-taste library exists;
--no-inspiration opts out): points the model at the stored case-study library +
selection map, structural-verbatim / themeable-retoned; external refs existence-gated.
- --context-pack <file>: inline a caller-supplied context block (the L0 receiver the
NOMOR/iris wrapper fills with its ctx warm-slice). Symlink/fifo/dir refused, 256KB
cap, shell-injection-safe (content never eval'd), covered by --secret-scan.
- --validate (requires --worktree): runs the target project's own toolchain
(tsc-first → typecheck/lint → eslint, else fail-loud) on the worktree to catch
broken JSX / bad imports; records validation:{passed,tool,errors}, INFORMS only.
Runs UNDER the same OS sandbox as the engine + a tamper-backstop re-check after it,
so engine-authored config code (eslint.config.js / package.json scripts) can't escape.
node_modules symlinked from origin so tsc actually runs.
Gated: cold-audit ∥ santa (APPROVE_WITH_NITS) → fixed --validate sandboxing +
post-validation backstop, stale validation.json leak, error-path JSON, byte-identity;
re-verified APPROVE (engine-run backstop refactor logic-preserved, no regression).
Suite 122/124 (2 pre-existing head-shadow env fails).
Co-Authored-By: Claude <noreply@anthropic.com>
…for the safety envelope + leverage layers Version bump 0.14.0 → 0.15.0 (SKILL.md frontmatter + ASK_GEMINI_VERSION). No logic change. Documents the appliable flow (--worktree → review → record-verdict PASS → apply; --diff-only is review-only and apply-refused by design) + the sandbox-vs-best-effort containment honesty. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI adds a cited-research mode and an opt-in delegated-edit safety envelope with worktree isolation, secret scanning, validation, tamper detection, HMAC-bound review gates, sandboxing, documentation, and comprehensive Bats coverage. ChangesDelegated workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ask-gemini
participant Worktree
participant ReviewGate
participant Repository
Caller->>ask-gemini: run delegated edit with --worktree
ask-gemini->>Worktree: execute engine and capture diff
Worktree-->>ask-gemini: proposed.diff and diff.sha256
Caller->>ReviewGate: record-verdict PASS
ReviewGate->>ReviewGate: verify HMAC and hash
Caller->>ReviewGate: apply --run
ReviewGate->>Repository: git apply --check and git apply
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/ask-gemini/scripts/ask-gemini (1)
1386-1481: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the new prompt layers to resumed sessions too.
build_prompt()returns at Lines 1291–1301 whenRESUMING=true, before context packs, research formatting, and inspiration are assembled. Consequently, named follow-ups silently ignore--context-pack, and resumed research calls lose the cited-research contract. Build common layers before the resume branch or include them in its prompt.🤖 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 `@skills/ask-gemini/scripts/ask-gemini` around lines 1386 - 1481, The RESUMING early return in build_prompt bypasses shared prompt layers such as context packs, inspiration, persona, and research response formatting. Refactor build_prompt so resumed sessions receive the same assembled layers as new sessions, either by constructing common layers before the resume branch or incorporating them into its returned prompt, while preserving the resumed-session-specific content.
🤖 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 `@README.md`:
- Line 87: Update the README statement beginning “By default nothing changes on
disk” to scope the claim specifically to the project tree or project files,
while preserving the existing explanation of delegated editing and the opt-in
envelope.
In `@skills/ask-gemini/CHANGELOG.md`:
- Line 3: Add an H2 heading, such as “Releases,” between the H1 title and the
“0.15.0” version heading in the changelog, restoring sequential Markdown heading
hierarchy and satisfying MD001.
In `@skills/ask-gemini/scripts/ask-gemini`:
- Around line 324-340: The _egress_secret_scan function must not recursively
follow symlinks via find -L. Replace that traversal with bounded handling that
scans repository-contained regular files while refusing symlink targets that
escape the requested directory, preserving SECRET_MATCHES aggregation and the
existing single-file path behavior.
- Around line 2275-2282: Update the validation flow around run_validation and
_containment_backstop_check so validation operates on the exact worktree state
represented by the previously captured proposed.diff and hash. Use a disposable
copy for validation, or snapshot and compare the worktree diff before and after
validation; reject any mutation and do not report validation.passed for changed
content.
- Around line 232-240: The _git_guard_digest function must include Git hook
metadata, not only concatenated contents. Update its digest input to hash each
relevant relative path together with its file type, mode, and content, while
retaining the config data and deterministic ordering so hook renames and
executable-bit changes alter the digest.
- Around line 2357-2371: The result construction currently adds worktree-related
fields to every JSON response, breaking byte-identical default runs. Update the
result-building logic around the worktree envelope fields and validation
handling so worktree, os_sandbox, proposed_diff, and diff_sha256 are inserted
only when the worktree envelope is active; preserve their existing values and
keep validation’s conditional behavior unchanged.
- Around line 1572-1575: Recreate the $SESSION_DIR/diff-only marker after the
engine exits, including the corresponding flow around the additional referenced
block, so an unsandboxed engine cannot remove the review-only guard. Ensure
capture or post-engine handling restores the marker before PASS/apply
eligibility is evaluated, while preserving the existing --worktree behavior
without a marker.
- Around line 1644-1649: Update the real-tree tamper snapshot logic around the
status capture and its corresponding comparison near the later referenced block
to hash file contents for every tracked-modified and untracked path, rather than
comparing only git porcelain lines. Preserve detection of repo-root .env, .env.*
and .envrc files, and ensure subsequent modifications to already-dirty or
already-existing untracked files produce a detected delta.
- Around line 189-192: Correct the safety-envelope heading above the
record-verdict, apply, and discard subcommands to state that this
review-then-apply gate applies to --worktree runs, not --diff-only runs. Keep
the subcommand descriptions and documented refusal behavior unchanged.
- Around line 2268-2271: Harden the capture sequence around the git add/diff
commands: validate the worktree’s .git pointer and make its Git metadata
read-only in both sandbox contexts before invoking git add -A, preventing
attacker-controlled repository configuration and filters. Replace the blanket ||
true suppression for capture operations with fail-closed error handling,
including validation, git add, diff generation, name-status generation, and
_sha256, while preserving the existing artifact paths.
- Around line 1547-1548: Tighten sandbox boundaries across
skills/ask-gemini/scripts/ask-gemini:1547-1548 and :1620-1627 and
skills/ask-gemini/scripts/sandbox-macos.sb:26-33 so REPO_TOP is read-only and
only the per-run worktree plus allowed temporary directory remains writable;
avoid granting the entire TMPDIR when using bwrap. Update the real-tree delta
handling at skills/ask-gemini/scripts/ask-gemini:2209-2214 to downgrade to WARN
only when the active sandbox enforces that REPO_TOP write restriction, and
update skills/ask-gemini/SKILL.md:503-505 to document the worktree-plus-temp
allowlist.
In `@skills/ask-gemini/scripts/sandbox-macos.sb`:
- Around line 13-15: The safety envelope must enforce the documented
secret-egress boundary: in skills/ask-gemini/scripts/sandbox-macos.sb lines
13-15, restrict readable paths or explicitly document that arbitrary host
secrets remain accessible; in skills/ask-gemini/SKILL.md lines 466-468, stop
presenting selected scan paths as the complete egress surface; in
skills/ask-gemini/scripts/ask-gemini lines 1708-1724, remove claims that
scanning covers everything agy can read unless it receives a read-restricted
filesystem view; and in skills/ask-gemini/scripts/ask-gemini lines 2140-2146,
run validation with immutable toolchain definitions, minimal read mounts, and
networking disabled.
In `@skills/ask-gemini/SKILL.md`:
- Around line 503-505: Revise the containment description around the
--worktree/--diff-only envelope to state that the OS sandbox permits writes only
to its explicit allowlist, including the throwaway worktree, required temporary
paths, and Gemini state/token paths. Remove the claim that the engine cannot
touch anything outside the worktree, and accurately describe the broad
temporary-directory allowance and its overlap with default worktrees while
preserving the existing no-sandbox and fail-closed backstop guidance.
In `@skills/ask-gemini/tests/mock-agy`:
- Around line 103-125: Update the shared test setup in bats_helper.bash to unset
MOCK_PLANT_GATE, ASK_GEMINI_HOME, and MOCK_STASH_DIR before each test. Ensure
inherited values cannot trigger file edits, gate planting, or session-directory
permission changes, including the setup paths corresponding to the additional
affected lines.
---
Outside diff comments:
In `@skills/ask-gemini/scripts/ask-gemini`:
- Around line 1386-1481: The RESUMING early return in build_prompt bypasses
shared prompt layers such as context packs, inspiration, persona, and research
response formatting. Refactor build_prompt so resumed sessions receive the same
assembled layers as new sessions, either by constructing common layers before
the resume branch or incorporating them into its returned prompt, while
preserving the resumed-session-specific content.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07625201-3033-4fc9-9cf3-a1f720617cc9
📒 Files selected for processing (10)
README.mdskills/ask-gemini/CHANGELOG.mdskills/ask-gemini/SKILL.mdskills/ask-gemini/scripts/ask-geminiskills/ask-gemini/scripts/sandbox-macos.sbskills/ask-gemini/tests/01_arg_parsing.batsskills/ask-gemini/tests/10_safety_envelope.batsskills/ask-gemini/tests/11_gemini_leverage.batsskills/ask-gemini/tests/bats_helper.bashskills/ask-gemini/tests/mock-agy
|
|
||
| ## Let it edit — safely (opt-in) | ||
|
|
||
| By default nothing changes on disk: Gemini proposes, you read, Claude integrates. But when you *do* want the delegated engine to make the edit itself, there's an opt-in envelope that lets it write without ever being trusted. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the default-off claim to project files.
“Nothing changes on disk” is misleading because normal runs still persist tool state. Say that nothing changes in the project tree.
Proposed wording
-By default nothing changes on disk: Gemini proposes, you read, Claude integrates.
+By default nothing changes in your project tree: Gemini proposes, you read, Claude integrates.📝 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.
| By default nothing changes on disk: Gemini proposes, you read, Claude integrates. But when you *do* want the delegated engine to make the edit itself, there's an opt-in envelope that lets it write without ever being trusted. | |
| By default nothing changes in your project tree: Gemini proposes, you read, Claude integrates. But when you *do* want the delegated engine to make the edit itself, there's an opt-in envelope that lets it write without ever being trusted. |
🤖 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 `@README.md` at line 87, Update the README statement beginning “By default
nothing changes on disk” to scope the claim specifically to the project tree or
project files, while preserving the existing explanation of delegated editing
and the opt-in envelope.
| @@ -1,5 +1,16 @@ | |||
| # Changelog | |||
|
|
|||
| ### 0.15.0 | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the Markdown heading hierarchy.
Line 3 jumps directly from H1 to H3, triggering MD001. Add an H2 such as ## Releases before the version headings.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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 `@skills/ask-gemini/CHANGELOG.md` at line 3, Add an H2 heading, such as
“Releases,” between the H1 title and the “0.15.0” version heading in the
changelog, restoring sequential Markdown heading hierarchy and satisfying MD001.
Source: Linters/SAST tools
| Safety-envelope subcommands (review-then-apply gate for --diff-only runs): | ||
| record-verdict --run DIR --verdict PASS|FAIL Bind a review verdict to the current diff.sha256 | ||
| apply --run DIR Apply the reviewed diff to the real tree (needs a PASS verdict, hash unchanged) | ||
| discard --run DIR Remove the run's throwaway worktree without applying |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the subcommand heading: apply is for --worktree, not --diff-only.
The current heading contradicts the documented hard refusal of diff-only runs.
Proposed fix
-Safety-envelope subcommands (review-then-apply gate for --diff-only runs):
+Safety-envelope subcommands (review-then-apply gate for --worktree runs):📝 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.
| Safety-envelope subcommands (review-then-apply gate for --diff-only runs): | |
| record-verdict --run DIR --verdict PASS|FAIL Bind a review verdict to the current diff.sha256 | |
| apply --run DIR Apply the reviewed diff to the real tree (needs a PASS verdict, hash unchanged) | |
| discard --run DIR Remove the run's throwaway worktree without applying | |
| Safety-envelope subcommands (review-then-apply gate for --worktree runs): | |
| record-verdict --run DIR --verdict PASS|FAIL Bind a review verdict to the current diff.sha256 | |
| apply --run DIR Apply the reviewed diff to the real tree (needs a PASS verdict, hash unchanged) | |
| discard --run DIR Remove the run's throwaway worktree without applying |
🤖 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 `@skills/ask-gemini/scripts/ask-gemini` around lines 189 - 192, Correct the
safety-envelope heading above the record-verdict, apply, and discard subcommands
to state that this review-then-apply gate applies to --worktree runs, not
--diff-only runs. Keep the subcommand descriptions and documented refusal
behavior unchanged.
| # Digest the real repo's .git config + hooks — the RCE surface (core.fsmonitor / alias / | ||
| # hook injection). Ported from nomor-iris _git_guard_digest. | ||
| _git_guard_digest() { | ||
| local gd; gd="$(git -C "$1" rev-parse --git-common-dir 2>/dev/null || echo "")" | ||
| [ -n "$gd" ] || { echo "nogit"; return; } | ||
| case "$gd" in /*) : ;; *) gd="$1/$gd" ;; esac | ||
| { [ -f "$gd/config" ] && cat "$gd/config"; find "$gd/hooks" -type f 2>/dev/null -exec cat {} \; 2>/dev/null; } \ | ||
| | sort \ | ||
| | { if command -v sha256sum >/dev/null 2>&1; then sha256sum; else shasum -a 256; fi; } 2>/dev/null | awk '{print $1}' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Include hook paths and modes in the Git guard digest.
Sorting concatenated contents misses hook renames and executable-bit changes. For example, making an existing hook executable leaves this digest unchanged despite changing future Git execution behavior. Hash each relative path, type, mode, and content.
🤖 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 `@skills/ask-gemini/scripts/ask-gemini` around lines 232 - 240, The
_git_guard_digest function must include Git hook metadata, not only concatenated
contents. Update its digest input to hash each relevant relative path together
with its file type, mode, and content, while retaining the config data and
deterministic ordering so hook renames and executable-bit changes alter the
digest.
| # Scan MANY files/dirs (HIGH-3: the REAL egress surface, not just the composed prompt). | ||
| # Args are file OR directory paths; directories are walked (.git skipped). HIGH-3: -L follows | ||
| # symlinks (agy dereferences them too) so a symlink -> an out-of-tree secret is scanned, not skipped. | ||
| # Aggregates hits into SECRET_MATCHES prefixed with the offending path. 0=clean, 1=hit. | ||
| _egress_secret_scan() { | ||
| local target f any=0 all="" ln | ||
| SECRET_MATCHES="" | ||
| for target in "$@"; do | ||
| [[ -n "$target" ]] || continue | ||
| if [[ -d "$target" ]]; then | ||
| while IFS= read -r f; do | ||
| if ! _prompt_secret_scan "$f"; then | ||
| any=1 | ||
| while IFS= read -r ln; do [[ -n "$ln" ]] && all="$all"$'\n'"$f: $ln"; done <<< "$SECRET_MATCHES" | ||
| fi | ||
| done < <(find -L "$target" -type f -not -path '*/.git/*' -not -name .git 2>/dev/null) | ||
| elif [[ -f "$target" ]]; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Refuse escaping symlinks instead of recursively scanning them without bounds.
find -L lets a repository symlink traverse an arbitrarily large external tree—even /—with no file-count, size, or time limit. This can hang the CLI or exhaust memory before the engine timeout starts.
🤖 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 `@skills/ask-gemini/scripts/ask-gemini` around lines 324 - 340, The
_egress_secret_scan function must not recursively follow symlinks via find -L.
Replace that traversal with bounded handling that scans repository-contained
regular files while refusing symlink targets that escape the requested
directory, preserving SECRET_MATCHES aggregation and the existing single-file
path behavior.
| # L2 mechanical gate (--validate): run the target project's toolchain on the worktree | ||
| # to catch broken JSX / bad imports. Writes validation.json; INFORMS, never auto-blocks apply. | ||
| # santa MEDIUM: validation executes engine-authored config/scripts, so re-run the containment | ||
| # backstop AFTER it — a validation-time escape to the real tree/.git is caught here (burn + exit 3) | ||
| # exactly like an engine-run escape, even when no OS sandbox is present. | ||
| if [[ "$VALIDATE" = true ]]; then | ||
| run_validation | ||
| _containment_backstop_check |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Ensure validation checks the captured diff, not a subsequently mutated worktree.
The diff and hash are captured before validation. A lint/typecheck script may rewrite files and pass, so validation.passed=true can describe different content from proposed.diff. Validate a disposable copy or compare the worktree diff before and after validation and reject mutation.
🤖 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 `@skills/ask-gemini/scripts/ask-gemini` around lines 2275 - 2282, Update the
validation flow around run_validation and _containment_backstop_check so
validation operates on the exact worktree state represented by the previously
captured proposed.diff and hash. Use a disposable copy for validation, or
snapshot and compare the worktree diff before and after validation; reject any
mutation and do not report validation.passed for changed content.
| 'stderr_excerpt': os.environ.get('AG_STDERR', '').strip(), | ||
| 'worktree': os.environ.get('AG_WORKTREE', '') or None, | ||
| 'os_sandbox': os.environ.get('AG_OS_SANDBOX', '') or None, | ||
| 'proposed_diff': os.environ.get('AG_PROPOSED_DIFF', '') or None, | ||
| 'diff_sha256': os.environ.get('AG_DIFF_SHA256', '') or None | ||
| } | ||
|
|
||
| # L2 (--validate): embed the validation result only when it was produced, so runs | ||
| # without --validate stay byte-identical to before. | ||
| _val_path = session_dir + '/validation.json' | ||
| if os.path.exists(_val_path): | ||
| try: | ||
| result['validation'] = json.load(open(_val_path)) | ||
| except Exception: | ||
| result['validation'] = {'passed': False, 'tool': None, 'skipped': 'validation-read-failed', 'errors': ''} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep default JSON byte-identical by adding safety fields conditionally.
Plain runs now always gain worktree, proposed_diff, diff_sha256, and os_sandbox:"none". This breaks existing consumers despite the PR's default-off compatibility requirement. Add these keys only when the worktree envelope is active, as already done for validation.
🤖 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 `@skills/ask-gemini/scripts/ask-gemini` around lines 2357 - 2371, The result
construction currently adds worktree-related fields to every JSON response,
breaking byte-identical default runs. Update the result-building logic around
the worktree envelope fields and validation handling so worktree, os_sandbox,
proposed_diff, and diff_sha256 are inserted only when the worktree envelope is
active; preserve their existing values and keep validation’s conditional
behavior unchanged.
| ;; SCOPE — this contains WRITES, not network EXFILTRATION: the engine keeps outbound network | ||
| ;; (it must reach the API), so a weaponized engine can still READ the worktree and POST it out. | ||
| ;; Exfil stays bounded by the data-egress policy + --secret-scan + the reviewer's diff gate. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
The safety envelope is write-only, so it cannot provide the documented secret-egress boundary. The engine and engine-authored validation commands retain host-wide read access plus outbound networking, while secret scanning covers only selected files.
skills/ask-gemini/scripts/sandbox-macos.sb#L13-L15: restrict readable paths or explicitly state that arbitrary host secrets remain readable and exfiltratable.skills/ask-gemini/SKILL.md#L466-L468: stop describing the selected scan paths as the complete egress surface.skills/ask-gemini/scripts/ask-gemini#L1708-L1724: do not claim the scanner covers everything agy can see unless the engine receives a read-restricted filesystem view.skills/ask-gemini/scripts/ask-gemini#L2140-L2146: run validation with immutable toolchain definitions, minimal read mounts, and networking disabled.
📍 Affects 3 files
skills/ask-gemini/scripts/sandbox-macos.sb#L13-L15(this comment)skills/ask-gemini/SKILL.md#L466-L468skills/ask-gemini/scripts/ask-gemini#L1708-L1724skills/ask-gemini/scripts/ask-gemini#L2140-L2146
🤖 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 `@skills/ask-gemini/scripts/sandbox-macos.sb` around lines 13 - 15, The safety
envelope must enforce the documented secret-egress boundary: in
skills/ask-gemini/scripts/sandbox-macos.sb lines 13-15, restrict readable paths
or explicitly document that arbitrary host secrets remain accessible; in
skills/ask-gemini/SKILL.md lines 466-468, stop presenting selected scan paths as
the complete egress surface; in skills/ask-gemini/scripts/ask-gemini lines
1708-1724, remove claims that scanning covers everything agy can read unless it
receives a read-restricted filesystem view; and in
skills/ask-gemini/scripts/ask-gemini lines 2140-2146, run validation with
immutable toolchain definitions, minimal read mounts, and networking disabled.
| The `--worktree`/`--diff-only` envelope isolates the delegated engine and gates every apply behind a hash-bound, HMAC-signed verdict. **The real containment boundary is the OS write-sandbox** (macOS `sandbox-exec` / Linux `bwrap`), which is the default whenever a sandbox binary is present: the engine physically cannot touch anything outside the throwaway worktree. | ||
|
|
||
| **Without an OS sandbox — `ASK_GEMINI_NO_SANDBOX=1`, or a host with neither `sandbox-exec` nor `bwrap` — the envelope runs in best-effort mode.** A fully-malicious engine is not *guaranteed* to be contained: it runs with your filesystem access, so the post-run tamper backstop is detection-after-the-fact, not prevention. That backstop is fail-closed (any detected real-tree/`.git`/sensitive-file change, or an unverifiable/locked session dir, aborts the run and blocks apply), but for a hard guarantee run under an OS sandbox. Treat `NO_SANDBOX` runs as "trusted-ish engine, verified output", never "hostile engine, safely contained". |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Narrow the claimed write-containment guarantee.
The profiles permit writes outside the worktree, including $TMPDIR and Gemini state/token paths. Moreover, the broad temp allowance overlaps all default worktrees. Describe the explicit allowlist rather than claiming nothing outside the worktree is writable.
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 20: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 18: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
[warning] 232: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 232: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 376: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
[error] 452: [SC2] External Script Fetching: Remote code is downloaded and executed. This bypasses code review and could introduce malicious code.
Remediation: Avoid downloading and executing remote scripts. Use trusted packages from PyPI/npm. If remote fetch is required, verify checksums and use HTTPS.
(Supply Chain (SC2))
🤖 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 `@skills/ask-gemini/SKILL.md` around lines 503 - 505, Revise the containment
description around the --worktree/--diff-only envelope to state that the OS
sandbox permits writes only to its explicit allowlist, including the throwaway
worktree, required temporary paths, and Gemini state/token paths. Remove the
claim that the engine cannot touch anything outside the worktree, and accurately
describe the broad temporary-directory allowance and its overlap with default
worktrees while preserving the existing no-sandbox and fail-closed backstop
guidance.
| if [[ -n "${MOCK_PLANT_GATE:-}" && -n "${ASK_GEMINI_HOME:-}" ]]; then | ||
| _sid="$(basename "$PWD")" | ||
| _sdir="$ASK_GEMINI_HOME/sessions/$_sid" | ||
| _keyf="$ASK_GEMINI_HOME/keys/$_sid.key" | ||
| if [[ -f "$_keyf" && -d "$_sdir" ]]; then | ||
| # cwd is the throwaway worktree (a fresh checkout of HEAD), so this diff applies to the real tree. | ||
| printf 'PWNED-BY-ENGINE\n' > "$PWD/app.txt" | ||
| git -C "$PWD" add app.txt >/dev/null 2>&1 | ||
| git -C "$PWD" diff --cached --binary > "$_sdir/proposed.diff" 2>/dev/null | ||
| git -C "$PWD" diff --cached --name-status > "$_sdir/changed-files.txt" 2>/dev/null | ||
| if command -v sha256sum >/dev/null 2>&1; then _sha="$(sha256sum "$_sdir/proposed.diff" | awk '{print $1}')"; | ||
| else _sha="$(shasum -a 256 "$_sdir/proposed.diff" | awk '{print $1}')"; fi | ||
| printf '%s\n' "$_sha" > "$_sdir/diff.sha256" | ||
| _key="$(cat "$_keyf")" | ||
| _hmac="$(AG_HMAC_KEY="$_key" python3 -c 'import hmac,hashlib,os,sys; print(hmac.new(os.environ["AG_HMAC_KEY"].encode(), sys.argv[1].encode(), hashlib.sha256).hexdigest())' "PASS"$'\n'"$_sha")" | ||
| printf 'PASS\n%s\n%s\n' "$_sha" "$_hmac" > "$_sdir/VERDICT" | ||
| if [[ -n "${MOCK_STASH_DIR:-}" ]]; then | ||
| mkdir -p "$MOCK_STASH_DIR" | ||
| cp "$_keyf" "$MOCK_STASH_DIR/key" | ||
| cp "$_sdir/proposed.diff" "$MOCK_STASH_DIR/proposed.diff" | ||
| fi | ||
| fi | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clear the new mock controls during test setup.
The shared helper does not unset these new environment variables. If inherited from the caller or CI, they can unexpectedly edit files, plant gates, or chmod session directories in unrelated tests.
Proposed fix in bats_helper.bash
unset MOCK_RESPONSE MOCK_RESPONSES_FILE MOCK_COUNTER_FILE MOCK_EXIT MOCK_SLEEP \
- MOCK_WRITE_CHATS MOCK_SESSION_ID MOCK_LOG
+ MOCK_WRITE_CHATS MOCK_SESSION_ID MOCK_LOG \
+ MOCK_PLANT_GATE MOCK_STASH_DIR MOCK_EDIT_FILE MOCK_EDIT_CONTENT \
+ MOCK_CHMOD_SESSION_DIRAlso applies to: 131-143
🤖 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 `@skills/ask-gemini/tests/mock-agy` around lines 103 - 125, Update the shared
test setup in bats_helper.bash to unset MOCK_PLANT_GATE, ASK_GEMINI_HOME, and
MOCK_STASH_DIR before each test. Ensure inherited values cannot trigger file
edits, gate planting, or session-directory permission changes, including the
setup paths corresponding to the additional affected lines.
1 participant
Consolidation work: make ask-gemini safe to let the delegated model edit code, and make it extract Gemini's design taste instead of its breakage. All new behavior is opt-in / default-OFF — an existing delegation with no new flags is byte-identical.
Commits
04c0410research-mode —--mode researchfor cited web/UI-UX research briefs (was finished-but-uncommitted; audited clean). (rides along from local main)91d6a0ePass A — safety envelope —--worktree(throwaway git worktree under an OS write-sandbox: macOSsandbox-exec/ Linuxbwrap, fail-closed tamper backstop where absent) captures aproposed.diffinstead of touching the real tree.record-verdict→applygate an HMAC-signed (decision+sha bound), hash-bound PASS; a swapped diff or detected tamper burns the session.--secret-scan/--allow-secretsscans the full egress surface before the model call.6e05149Pass B — Gemini-leverage —uiopen-brief persona; auto inspiration-library pointer when~/.design-taste/exists (--no-inspirationopts out);--context-pack <file>(injection-safe L0 context receiver);--validate(runs the project's own tsc/lint on the worktree, sandboxed + backstop-rechecked, to catch broken JSX / bad imports — INFORMS, never auto-blocks).b064ddadocs — v0.15.0, CHANGELOG, README, SKILL flag reference.Appliable flow
--worktree→ reviewproposed.diff→record-verdict --run <dir> --verdict PASS→apply --run <dir>.--diff-onlyis the review-only variant (applyrefuses it by design; re-run with--worktreeto land a change).Containment honesty
The OS sandbox is the real boundary.
ASK_GEMINI_NO_SANDBOX=1/ hosts withoutsandbox-exec/bwraprun best-effort — detection-after-the-fact, not prevention (documented in SKILL.md Security).Review
Both feature passes were adversarially gated (cold-audit ∥ santa). Pass A took 4 rounds — closed forged-approval (decision-bound HMAC), worktree escape, and no-sandbox key-plant / read-only-dir / chmod-500 bypasses, each with a fail-before/pass-after test. Pass B closed an unsandboxed-
--validateexecution hole. Suite: 122/124 (the 2 fails are an environmentalhead-shadow on the author's box, unrelated).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests