Repository navigation
fix(core): eliminate false positives on untrusted command flags and compound loops - #29672
Merged
Merged
Conversation
…ompound loops Preserves shell variables during flag parsing in findUntrustedFlags, exempts safe read-only/navigation flags (-C, --all, --grep, --oneline, ls -ld, grep -rn), restricts argument matching to high-risk patterns, and allows compound loops in PolicyEngine when subcommands match ALLOW rules. Fixes #29579 Fixes #29650
…bstring flags - Preserves case sensitivity for short flags in findUntrustedFlags so -C is not matched against high-risk lowercase -c - Guards isHighRiskValSubstring so safe inspection flags (like --grep) taking URL/path substrings are not falsely flagged - Strips leading newlines/whitespace in ShellTool Output metadata - Adds unit test coverage for compound loops in PolicyEngine
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several false-positive security warnings encountered during shell command execution. By refining how shell variables are parsed, introducing a whitelist of safe POSIX flags, and improving the logic for evaluating compound commands, the changes ensure that legitimate, non-malicious operations are no longer blocked by the security policy engine. These updates enhance the usability of the CLI while maintaining security boundaries. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/L
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the policy engine to upgrade catch-all ASK_USER decisions to ALLOW for compound commands and loops when all decomposed sub-commands explicitly match ALLOW rules. It also refactors the untrusted context tracker to reduce false positives by introducing safe short/long flags and high-risk value flags, and preserving shell variables during parsing. The review feedback correctly identifies that checking rule?.decision === PolicyDecision.ALLOW for sub-commands and wrapped commands will fail to recognize sub-commands allowed by heuristics (which may have an undefined rule or a catch-all rule), and suggests checking the overall decision instead.
|
Size Change: +6.2 kB (+0.02%) Total Size: 35.5 MB
ℹ️ View Unchanged
|
Checking rule?.decision === PolicyDecision.ALLOW fails to recognize sub-commands allowed by heuristics, checkers, or compound subshells where rule may be undefined or point to a catch-all ASK_USER rule. Checking result.decision === ALLOW correctly recognizes all allowed sub-commands when upgrading compound commands.
kschaab
requested changes
Oct 8, 2026
kschaab
left a comment
kschaab
left a comment
Contributor
There was a problem hiding this comment.
Executive Summary & Verdict
This PR addresses false-positive security warnings on shell commands (#29579, #29650) across untrustedContextTracker and PolicyEngine.
Verdict: Request Changes
While the untrustedContextTracker updates effectively eliminate false positives on POSIX flags and shell loop variables, there are two blocking issues to address before merging:
[PITFALL-008]/[OOP-CVA-002](Security Regression inPolicyEngine.checkShellCommand): Because recursive sub-command checks incheckShellCommandpassskipHeuristics = true, the new bottom-upASK_USER->ALLOWupgrade reverses top-levelapplyShellHeuristicssafety downgrades (dir_pathescaping the workspace boundary,gitcommands in untrusted workspaces, orisDangerousCommand) whenever two commands are chained.[TEST-002](Short-flag case sensitivity inextractUntrustedContext): WhileisHighRiskFlagnow checksHIGH_RISK_VALUE_FLAGScase-sensitively for short flags,extractUntrustedContextstill lowercases all tokens when populatinguntrustedTokensand Check 1A queriesuntrustedTokensusing lowercased flag names.
Good PR Checkmarks Scorecard
| ID | Checkmark | Grade | Justification |
|---|---|---|---|
[CHECK-1] |
PR Scope & Pure Refactor Isolation | ✅ PASS | Focused on eliminating false-positive shell confirmations across untrustedContextTracker and PolicyEngine for #29579 and #29650. |
[CHECK-2] |
Pragmatic DRY (Don't Repeat Yourself) | Check 1A duplicates a narrower version of Check 1C's nextToken logic, and the normalized untrustedTexts substring scan is repeated three times in findUntrustedFlags. |
|
[CHECK-3] |
Low Cyclomatic Complexity & Linear Flow | findUntrustedFlags has grown to 159 lines with 5 levels of nested conditionals inside the token loop. |
|
[CHECK-4] |
OOP Class Hierarchy & CVA Fit | ❌ FAIL | checkShellCommand's bottom-up ASK_USER -> ALLOW upgrade conflicts with PolicyEngine's top-down applyShellHeuristics downgrade contract when skipHeuristics = true. |
[CHECK-5] |
Self-Documenting Code Over Comments | Multi-clause boolean conditions in Check 1B/1C rely on section comments rather than small named predicates. | |
[CHECK-6] |
Comprehensive Test & Branch Coverage | ❌ FAIL | Missing negative tests for compound commands under heuristic downgrades (dir_path outside workspace / untrusted folder), and one new test mocks engine.check ([PITFALL-003]) to simulate a state unreachable when skipHeuristics = true. |
Additional Note ([PR-HYGIENE-002])
- PR Title: Please update the PR title from
Fix/untrusted flags false positivesto follow Conventional Commits perCONTRIBUTING.md(e.g.,fix(core): eliminate false positives on untrusted command flags and compound loops).
DavidAPierce
changed the title
…ntextTracker - Enforce safety heuristics on decomposed sub-commands and prevent compound command upgrades from reversing top-level heuristic safety downgrades - Remove self-spy in PolicyEngine test and add negative tests for compound commands under heuristic downgrades - Preserve case for short flags in extractUntrustedContext and add -E to safe flags - Deduplicate nextToken and substring scanning in findUntrustedFlags - Restrict variable matching to bare shell variables to detect env-prefixed script paths
…and upgrades - In checkShellCommand, require decomposed sub-commands to match an explicit argsPattern ALLOW rule before upgrading catch-all ASK_USER to ALLOW - Pass skipHeuristics=true during recursive sub-command checks so default heuristic allowances (e.g. isKnownSafeCommand) do not bypass user allowlists in chained commands - Enforce workspace boundary, untrusted git, and dangerous command checks prior to compound upgrades - Fixes test failures in shell-safety-regression.test.ts
kschaab
requested changes
Oct 8, 2026
kschaab
left a comment
kschaab
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing the initial review comments and tightening checkShellCommand in 03889af to require explicit argsPattern ALLOW rules on decomposed sub-commands!
All of the untrustedContextTracker fixes look great. There is one remaining issue in PolicyEngine.checkShellCommand (and its accompanying unit tests) around how isDangerous and isUntrustedGit are evaluated on compound commands—see inline comments below.
kschaab
approved these changes
Oct 8, 2026
kschaab
left a comment
kschaab
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Thanks for addressing all the review feedback and adding thorough regression coverage.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 8, 2026
DavidAPierce
enabled auto-merge
October 8, 2026 20:35
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 8, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 8, 2026
Contributor
Author
|
/patch preview |
|
🚀 [Step 1/4] Patch workflow(s) waiting for approval! 📋 Details:
⏳ Status: The patch creation workflow has been triggered and is waiting for deployment approval. Please visit the specific workflow links below and approve the runs. 🔗 Track Progress: |
|
🚀 [Step 2/4] Patch PR Created! 📋 Patch Details:
📝 Next Steps:
🔗 Track Progress: |
|
🚀 [Step 3/4] Patch Release Waiting for Approval! 📋 Release Details:
⏳ Status: The patch release has been triggered and is waiting for deployment approval. Please visit the specific workflow run link below and approve the deployment. You'll receive another update when it completes. 🔗 Track Progress: |
|
✅ [Step 4/4] Patch Release Complete! 📦 Release Details:
🎉 Status: Your patch has been successfully released and published to npm! 📝 What's Available:
🔗 Links: |
This was referenced Oct 10, 2026
|
I just upgraded to the preview version and I still get the warnings. |
|
Are you just going to white list all command flags ? |
4 of 18 tasks
Blackmanx
pushed a commit
to Blackmanx/gemini-cli
that referenced
this pull request
Oct 20, 2026
…ompound loops (google-gemini#29672) Co-authored-by: Keith Schaab <keith.schaab@gmail.com>
Bulibas
added a commit
to SinishaDjukic/worca-cc
that referenced
this pull request
Oct 24, 2026
# Multi-harness: Gemini CLI and Qwen Code harnesses Adds two run engines, both marked **beta**: `worca --engine gemini` (Google's Gemini CLI) and `worca --engine qwen` (Qwen Code). Qwen Code is a fork of Gemini CLI, so both run on one shared base, and each per-engine file keeps only what really differs. Unlike Cursor (#606), both adapters were **spiked against the real binaries** (gemini 0.63.0, qwen 0.25.0). The captured streams, and every fact the adapters rely on, are in `test/fixtures/gemini/README.md` and `test/fixtures/qwen/README.md`. ## Usage ```bash npm i -g @google/gemini-cli # GEMINI_API_KEY in worca's env (or ~/.gemini/.env), or sign in once with `gemini` npm i -g @qwen-code/qwen-code # choose an auth type once with `qwen`, or set a whole provider's env worca --project . --prompt "…" --engine gemini [--model gemini-3.8-flash] [--allow-unguarded-engine] ``` `WORCA_GEMINI_BIN` / `WORCA_QWEN_BIN` point worca at another binary. Both engines are also offered on New pipeline, as a default engine in Settings › Models, and in a project's Settings. ## Layout - `src/core/engines/gemini-family.mjs` holds the shared parts: - sessions named up front (`--session-id`) and resumed (`--resume`), with one fresh retry when the CLI no longer knows the session - prompt on stdin - workspace folders (`--include-directories`), refused inside Worca's home as for Copilot - `--allowed-mcp-server-names`, so only the run's servers attach - MCP server shape, `${VAR}` refs kept - Claude tool names - error classes, preflight, the turn cap and the spawn runner - `src/core/engines/gemini.mjs`: Gemini's own stream normalizer, its policy TOML and the checkout `.gemini/settings.json`. - `src/core/engines/qwen.mjs`: Qwen's stream is Claude-shaped, so it goes through the existing `claude-events.mjs` normalizer after a small rewrite: the session id gets its prefix, tool names become Claude's, and the `tool_call` meta tool is unwrapped. Rules and MCP servers go into a system settings file. - `src/core/engines/project-files.mjs`: Cursor's checkout-file writer and ownership ledger, moved out of `cursor.mjs` and given an engine parameter. The harness now handles any engine's checkout files: register, teardown, commit and diff. ## Capabilities (from the spike) | | Gemini CLI | Qwen Code | |---|---|---| | resume, stream events, MCP tools, permission rules | yes | yes | | system-prompt flag | **no**: `GEMINI_SYSTEM_MD` replaces the whole built-in prompt, so the prompt is folded in | **yes**: `--append-system-prompt`; a prompt too long for argv is folded into stdin | | cost | **no**: tokens only, shown as *cost unknown* | **no**: tokens only | | allowed tools, effort, sub-agents, hook telemetry, skills, sub-agent system prompt, turn budget | no | no (`--max-session-turns` did not cap tool calls in the spike) | ## Guardrails | Rule | Gemini CLI (`--admin-policy` TOML, admin tier) | Qwen Code (Claude syntax, as written) | |---|---|---| | bare `Bash`, `WebFetch`, `WebSearch`, MCP server or tool | held | held | | `Bash(git push:*)` | **partial**: prefix match; the spike showed `env git log` passes | **partial**: same gap | | `Read(…)` | **partial**: argsPattern on the file tools; the shell is not covered | held (a Read rule also stopped `cat` in the spike) | | `Edit(…)`, `Write(…)`, `NotebookEdit(…)` | **partial**: same | **partial**: the file tools stop, but the YOLO shell still writes the path | | unknown tools | unenforced | unenforced | - **Gemini ignores a policy file it cannot parse.** It only prints "Policy file error" on stderr and runs with no rule. The adapter stops the child on that line (a new `onStderrLine` hook in `superviseSpawn`). - **Qwen's env under scrub:** with a scrubbing set, Qwen gets `QWEN_` names plus only the provider it is set to (its chosen sign-in, else the provider its env names): `OPENAI_`/`DASHSCOPE_`, `GEMINI_`, `GOOGLE_` or `ANTHROPIC_`. worca's own keys for other providers stay out of its shell. - An `--admin-policy` file is also skipped when the system policy folder holds `.toml` files, so the run refuses then. ## Checkout files and MCP - **Gemini** reads MCP servers only from settings files, and its system-settings path must be owned by root. So worca writes the checkout's `.gemini/settings.json`, with the same ownership, exclude-line and teardown handling as Cursor's files. A tracked `.gemini/settings.json` makes a run with MCP servers refuse. - Gemini needs `GEMINI_CLI_TRUST_WORKSPACE=true`: without it a headless run exits 55, and `--skip-trust` drops the checkout's MCP servers. - **Qwen** honours `QWEN_CODE_SYSTEM_SETTINGS_PATH` for any owner. worca writes its servers and deny rules there, in its temp folder, merged over the machine's own system settings. Nothing goes into the checkout. (`--mcp-config` does not fill `${VAR}`, so it is not used.) ## Elsewhere - **Helper jobs** (titles, overview, PR description, Auto classifier, night decider) run on Claude, as on Cursor. The new `SIGN_IN_ENGINES` (cursor, gemini, qwen) replaces the `engine === 'cursor'` checks for: - model fields: no env, no upstream, no effort - helper routing - the Test button's scratch cwd and signed-out hint - Ask proposals and the models editor - **Read-only spawns and Ask refuse** on both engines. Both CLIs have an `--approval-mode plan`, but no lockdown was verified, so read-only jobs and Ask stay on Claude. - **Models:** worca ships none. Users add catalog models with Engine Gemini CLI or Qwen Code. - **UI:** engine picker buttons with the Beta badge, Settings engine cards, the models editor (connection text, no effort or pricing), and the usage-limit resume offers. - **Docs:** a new "Gemini CLI and Qwen Code" section in `docs/models.md`. ## Tests - New test files: - `test/gemini-adapter.test.mjs` and `test/qwen-adapter.test.mjs`: the normalizers against the real captures, argv, the rule plans, preflight, and the spawn paths with fake binaries (`test/helpers/fake-gemini-family.mjs`) - `test/gemini-qwen-helpers.test.mjs` - Extended: registry, beta, readiness, switch, catalog, manifest, policy, cascade, gate (`.gemini/settings.json` teardown and commit), picker, settings and models-view tests. ```bash node tools/test.mjs test/gemini-adapter.test.mjs test/qwen-adapter.test.mjs test/gemini-qwen-helpers.test.mjs ``` **Status: draft.** **End-to-end runs.** Real worca pipelines (`--yes`, default workflow, Permissive set) ran on both real CLIs, with a scratch `HOME` and `WORCA_HOME` and a small task: add `add(a, b)` to `math.mjs` with a `node:test` test. - **Gemini CLI** (`gemini-3.8-flash` via `auto`): two runs. Both completed all 7 executions, the reviewer passed, and the result was committed on the run branch; the summary says *cost unknown*. - **Qwen Code** (signed in through Gemini: `GEMINI_API_KEY` + `GEMINI_MODEL`): one run. It completed the same way; the summary says *cost unknown*. - **Rerun on the rebased commit (2026-10-20):** gemini 0.64.0 and qwen 0.25.3 (the current stable releases), with the Normal set and `--allow-unguarded-engine`, the same task, and a scratch `HOME` and `WORCA_HOME`. - **Gemini CLI** (model `auto`): 7 executions in 5m35s. Refine and the reviewer were clean, the result was committed, and `npm test` passes on it (2/2). No "denied by policy" this time. - **Qwen Code** (`GEMINI_API_KEY` + `GEMINI_MODEL=gemini-3.8-flash`): 7 executions in 4m03s. Same outcome, `npm test` 8/8. - The only tool errors were reads of files that did not exist yet, and one `npm test` run before the test file was written. - The first Gemini run found three bugs, now fixed: - The implementer could not read its plan: Gemini's file tools read only the workspace, and plans live in worca's store. Each store folder now brings its project's store folder into the workspace. - Gemini passes cwd-relative paths, so the run log showed them under worca's own directory. Tool paths are now made absolute against the spawn's cwd. - The CLI's step lines printed `$0.00` on engines that report no cost. This affected Copilot and Cursor too. They now show no cost, matching the run total. - **Known Gemini behaviour:** inside worca runs, Gemini now and then answers a plain `node --test math.test.mjs` with "Tool execution denied by policy". - worca passed no policy file on those runs (the spawn argv was checked). - The same command ran fine outside worca, and the agent recovered every time (it re-ran the test as `./math.test.mjs`). - **Likely cause:** a taint check in Gemini CLI itself, added in 0.63 ([#29250](google-gemini/gemini-cli#29250)). In a non-interactive run it turns an allowed shell call into a deny, with no rule named, in two cases: - a flag or path in the command appeared earlier in tool output (shell, MCP or web output). For example, `--test` printed by `cat package.json` taints a later `node --test`. - the session edited a build file and then runs a build or test command (npm, npx, make, pytest and others). - It runs before YOLO and has no setting to turn it off. 0.64 ([#29672](google-gemini/gemini-cli#29672)) narrows it, but `--test` is not on its safe list. Two open Gemini PRs (#29833, #29719) would narrow it further. - worca cannot switch it off, so it is left as a known limitation. **Tests.** - Full suite: 9,050 tests, 15 failures on the first run. - 9 were tests that pin the engine lists and the resume offers. They now include Gemini CLI and Qwen Code. - 4 were perf/timing tests that failed under the parallel run's load. They pass when run alone. - `wsmap-deps` fails locally only because this worktree has no `node_modules`. CI installs them. - My real `~/.worca-cc/settings.json` was unchanged before and after. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eliminates false-positive security warnings and confirmation halts during shell command execution caused by:
untrustedContextTracker.ls -ld,grep -rn,git -C,git log --all --grep --oneline) being flagged as tainted arguments.for repo in ...; do ...; doneor chained commands) being blocked by catch-allASK_USERpolicies even when every decomposed sub-command explicitly matches anALLOWrule.Details
shellParseinfindUntrustedFlagswith a custom variable resolver(key) => \${key}`so shell variables (e.g."$repo") do not expand to empty strings and shift positional token parsing. Skips variable tokens starting with$` from argument matching.SAFE_SHORT_FLAGSandSAFE_LONG_FLAGSsets covering read-only, formatting, and directory navigation flags (-C,-la,-ld,-rn,--all,--grep,--oneline, etc.) that cannot execute arbitrary code or escalate privileges on their own.HIGH_RISK_VALUE_FLAGSagainst raw flag names for short flags and lowercased names only for long flags (--...), preventing safe uppercase flags like-Cfrom colliding with lowercase-c.src/index.ts,package.json) and file mode listings from triggering taint warnings.isHighRiskValSubstringwith(!isSafeFlag || isHighRiskFlag)to ensure safe flags taking URLs or paths as search patterns (e.g.,git log --grep="http://...") are not flagged.ShellToolexecution wrappers (Output:,Exit Code:,PGID:) from indexed tokens with leading newline/whitespace resilience.ASK_USERdecision toALLOWif every decomposed sub-command matches an explicitALLOWrule without redirection downgrade.Related Issues
Fixes #29579
Fixes #29650
How to Validate
npm test -w @google/gemini-cli-core -- src/policy/policy-engine.test.ts src/utils/untrustedContextTracker.test.tsnpm run typecheck && npm run lintfor repo in dir1 dir2; do git -C "$repo" log --all --grep="11541" --oneline; donegit -C <dir> log --onelinels -ld srcandls -lagrep -rn "pattern" src/index.tsgit diff package.jsonPre-Merge Checklist