[codex] fix team redirection false positive for read-only bash#59
[codex] fix team redirection false positive for read-only bash#59
Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
ToolRegistry.layer now requires Question.Service and Todo.Service as direct dependencies after upstream refactor. Build those layers explicitly before providing them to the registry, matching the pattern in prompt-effect.test.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Skip parallel implementation enforcement when repository has no commits yet. This prevents the deadlock where mutations are blocked until team tool is called, but team/yardadd requires HEAD to create worktrees. - chat.message hook: detect HEAD-less repo and suspend parallel policy - yardadd(): fail early with clear error if HEAD is missing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Empty the evals Set so all configured providers are available for standard work. Guard both eval-related gate checks with evals.size > 0 to prevent provider-eval from being blocked when no eval providers are defined. Update tests accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add Notification service with zero dependencies (darwin/linux/win32) - macOS notifications attributed to Terminal.app (fixes click target) - Add terminal focus detection to avoid notifications when focused - Add 'notifications' config option to tui.json (default: true) - Listen to session.idle for completion notifications - Update session.error to also show system notifications - Fix documentation plugin example (osascript attribution) - Beautify TUI toast: full borders + variant icons (✓✗ℹ⚠)
- Add .catch() to session.idle notification to prevent crashes - Use proper escapeForOsascript function for macOS - Add more terminals to detection list (vscode, code) - Restore escapeXml for Windows branch
6536c32 to
e28ebc0
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates guardrail behavior in “team” mode to reduce false positives for non-mutating bash commands, and adds/updates system notification support and CI reporting wiring across packages.
Changes:
- Adjusts
teamguardrail mutation detection to ignore fd-only redirections //dev/nullsinks and adds scenario regression coverage (plus a HEAD-less repo behavior test). - Introduces built-in TUI system notifications (macOS/Linux/Windows) with a new
tui.jsonnotificationsoption, plus associated UI/doc updates. - Tweaks provider admission lane behavior (disables the evaluation-only provider lane by default) and improves CI junit artifact creation/upload.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/test.yml | Upload artifact naming/behavior changes for unit test reports. |
| packages/app/package.json | Ensures junit output directory exists before running CI tests. |
| packages/guardrails/profile/AGENTS.md | Updates written provider-admission guidance. |
| packages/guardrails/profile/plugins/guardrail.ts | Changes eval-lane gating behavior (evals set to empty by default). |
| packages/guardrails/profile/plugins/team.ts | Updates mutation classifier to scrub fd-only redirections and /dev/null sinks; adds bootstrap-mode behavior when HEAD is missing. |
| packages/opencode/package.json | Ensures junit output directory exists before running CI tests. |
| packages/opencode/src/cli/cmd/tui/app.tsx | Emits system notifications on session idle/error (config-gated). |
| packages/opencode/src/cli/cmd/tui/feature-plugins/home/tips-view.tsx | Updates TUI tips to reference built-in notifications config. |
| packages/opencode/src/cli/cmd/tui/ui/toast.tsx | Toast UI tweaks (icons/border) and safer error typing. |
| packages/opencode/src/config/tui-schema.ts | Adds notifications option to TUI config schema. |
| packages/opencode/src/notification/index.ts | Adds cross-platform notification helpers + terminal focus detection (macOS). |
| packages/opencode/test/notification.test.ts | Adds unit tests for terminal app normalization and Windows toast XML escaping. |
| packages/opencode/test/scenario/guardrails.test.ts | Updates provider admission test expectations; adds team fd-redirection regression + HEAD-less repo test. |
| packages/opencode/test/scenario/harness.ts | Updates scenario harness layer wiring to include Question and Todo dependencies. |
| packages/web/src/content/docs/ar/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/bs/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/da/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/de/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/es/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/fr/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/it/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/ja/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/ko/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/nb/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/pl/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/plugins.mdx | Updates notification plugin example and adds note about built-in TUI notifications. |
| packages/web/src/content/docs/pt-br/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/ru/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/th/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/tr/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/zh-cn/plugins.mdx | Updates notification plugin example AppleScript line. |
| packages/web/src/content/docs/zh-tw/plugins.mdx | Updates notification plugin example AppleScript line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: unit-linux-${{ github.run_attempt }} | ||
| name: unit-${{ matrix.settings.name }}-${{ github.run_attempt }} |
There was a problem hiding this comment.
actions/upload-artifact is using matrix.settings.name, but this job doesn’t define a matrix strategy. Referencing matrix in a non-matrix job will fail expression evaluation and can break the workflow; use a concrete label (e.g. linux) or add a strategy.matrix for this job.
| name: unit-${{ matrix.settings.name }}-${{ github.run_attempt }} | |
| name: unit-linux-${{ github.run_attempt }} |
| .default(true) | ||
| .optional() |
There was a problem hiding this comment.
In Zod, chaining .default(true).optional() makes the outer optional() short-circuit on undefined, so the default never applies. If you want notifications to default to true, use .optional().default(true) (or just .default(true) without .optional()).
| .default(true) | |
| .optional() | |
| .optional() | |
| .default(true) |
| if (os === "darwin") { | ||
| const escaped = escapeForOsascript(message) | ||
| const titleEscaped = escapeForOsascript(title) | ||
| await Process.run( | ||
| [ | ||
| "osascript", | ||
| "-e", | ||
| `tell application "Terminal" to display notification "${escaped}" with title "${titleEscaped}"`, | ||
| ], | ||
| { nothrow: true }, | ||
| ) | ||
| return |
There was a problem hiding this comment.
On macOS, osascript -e 'tell application "Terminal" ...' will launch Terminal.app if it isn’t already running (and will attribute notifications to Terminal). That’s surprising for users running OpenCode in iTerm2/VScode/etc; consider using display notification ... without targeting Terminal (or target System Events) so showing a notification never opens Terminal.
| function escapeForOsascript(s: string): string { | ||
| return s.replace(/\\/g, "\\\\").replace(/"/g, '\\"') | ||
| } | ||
|
|
||
| export function terminal(name: string) { | ||
| return APPS.has(norm(name)) | ||
| } | ||
|
|
||
| export function xml(title: string, message: string) { | ||
| return `<toast><visual><binding template='ToastText02'><text id='1'>${escapeXml(title)}</text><text id='2'>${escapeXml(message)}</text></binding></visual></toast>` | ||
| } | ||
|
|
||
| export namespace Notification { | ||
| export async function terminalIsFocused(): Promise<boolean> { | ||
| if (platform() !== "darwin") return false | ||
|
|
||
| const result = await Process.text( | ||
| [ | ||
| "osascript", | ||
| "-e", | ||
| 'tell application "System Events" to get name of first application process whose frontmost is true', | ||
| ], | ||
| { nothrow: true }, | ||
| ) | ||
| return terminal(result.text.trim()) | ||
| } | ||
|
|
||
| export async function show(title: string, message: string): Promise<void> { | ||
| const os = platform() | ||
|
|
||
| if (os === "darwin") { | ||
| const escaped = escapeForOsascript(message) | ||
| const titleEscaped = escapeForOsascript(title) | ||
| await Process.run( | ||
| [ | ||
| "osascript", | ||
| "-e", | ||
| `tell application "Terminal" to display notification "${escaped}" with title "${titleEscaped}"`, | ||
| ], |
There was a problem hiding this comment.
escapeForOsascript doesn’t handle newline characters. If title/message contains \n, the generated AppleScript snippet can become syntactically invalid (AppleScript string literals can’t span raw newlines). Consider normalizing newlines (e.g. replace with spaces) or generating AppleScript that concatenates with return/linefeed.
| // Send notification on session completion | ||
| if (event.type === "session.idle") { | ||
| await $`osascript -e 'display notification "Session completed!" with title "opencode"'` | ||
| await $`osascript -e 'tell application "Terminal" to display notification "Session completed!" with title "opencode"'` |
There was a problem hiding this comment.
This AppleScript example uses tell application "Terminal" ..., which can launch Terminal.app when it isn’t running. For a notification-only snippet, prefer osascript -e 'display notification ...' (or System Events) so following the docs doesn’t unexpectedly open Terminal.
| await $`osascript -e 'tell application "Terminal" to display notification "Session completed!" with title "opencode"'` | |
| await $`osascript -e 'display notification "Session completed!" with title "opencode"'` |
| ``` | ||
|
|
||
| We are using `osascript` to run AppleScript on macOS. Here we are using it to send notifications. | ||
| We are using `osascript` to run AppleScript on macOS. Here we are using it to send notifications. If you want this without a plugin, OpenCode's TUI also has built-in system notifications via `"notifications": true` in `tui.json`. |
There was a problem hiding this comment.
This section says to enable built-in notifications via "notifications": true, but the schema sets notifications to default true (once the Zod default chaining is fixed). Consider clarifying that notifications are on by default and document how to disable them ("notifications": false) to avoid confusing readers.
| We are using `osascript` to run AppleScript on macOS. Here we are using it to send notifications. If you want this without a plugin, OpenCode's TUI also has built-in system notifications via `"notifications": true` in `tui.json`. | |
| We are using `osascript` to run AppleScript on macOS. Here we are using it to send notifications. If you want this without a plugin, OpenCode's TUI also has built-in system notifications enabled by default. You can disable them by setting `"notifications": false` in `tui.json`. |
| const mode = typeof opts?.mode === "string" ? opts.mode : "enforced" | ||
| const evals = new Set(["openrouter"]) | ||
| const evals = new Set<string>([]) | ||
| const evalAgent = "provider-eval" | ||
| const conf = true | ||
| const denyFree = true |
There was a problem hiding this comment.
PR description focuses on the team redirection classifier fix, but this change also removes the OpenRouter evaluation-only lane (evals becomes empty) and updates provider admission semantics. Please confirm this policy change is intended and, if so, reflect it in the PR description to avoid surprise behavior changes.
Summary
2>&1,>&2, and/dev/nullsinks as worktree mutations> output.txtRoot cause
The
teamplugin used a broad/>/mutation heuristic. That matched read-only shell redirections like2>&1, so read-only worker commands were rejected as "mutating bash". In team mode, that forced additional fan-out and subagent retries instead of allowing the bash call to proceed, which could leave sessions effectively stuck.Validation
bun test test/scenario/guardrails.test.tsinpackages/opencodebun run typecheckinpackages/opencodegit pushpre-push hook ran repo typecheck successfullyses_2a778e5d8ffe4MIvPEgJJsM1DB2>&1now reach normal bash permission prompts and execute, instead of tripping the team mutation gate and recursing into more subagentsImpact
This unblocks read-only inspection tasks in team mode without relaxing protection for actual file-mutating shell commands.
Closes #57