Repository navigation
feat(control): under an agent, calls that bring an app to the front or move the pointer are refused, and recordings leave out excluded windows - #591
Merged
Conversation
|
🐉 eve review — 🔴 REQUEST_CHANGES · 3 findings
run Previous runs (7)
|
Contributor
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 22 minutes. View limit details
Comment |
![eve-bot-lovinka[bot]](https://avatars.githubusercontent.com/in/4092731?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐉 eve review — 🔴 Changes requested
00ca301· 5 actionable findings · reviewed by find codex/gpt-6.1-sol + codex/gpt-6-luna, verify codex/gpt-6-luna · view run ↗
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 2 |
| 🔵 Low | 1 |
Blast radius
ComputerUse(~) → 6 dependent(s) outside this PR: src/control/mcp/server.ts, src/control/lib/capture-native.tsmoved(+) → 5 dependent(s) outside this PR: src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/VolumeSpace.swift, src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/BigFileRadar.swift, src/macos/GenesisKit/Sources/GenesisKitDisk/Disk/DiskHistoryStore.swift, +2 morebringFrontmost(~) → 4 dependent(s) outside this PR: native/ax-tool/Sources/commands/Hotkey.swift, native/ax-tool/Sources/commands/ValueCommands.swift, native/ax-tool/Sources/commands/Scroll.swift, +1 moreactionRefusal(~) → 2 dependent(s) outside this PR: src/control/lib/decision/assist.tscomputerSchemas(~) → 2 dependent(s) outside this PR: src/control/mcp/server.tsRecoveryController(~) → 2 dependent(s) outside this PR: src/control/lib/decision/assist.tsComputerUse.find(~) → 1 dependent(s) outside this PR: src/control/mcp/server.tsComputerUse.launch_app(~) → 1 dependent(s) outside this PR: src/control/mcp/server.ts- graph truncated by caps — dependents above are a lower bound
ℹ️ Claims vs diff — 5 claims could not be checked from the diff (c2, c4, c5, c6, c7).
genesiscz
added a commit
that referenced
this pull request
Oct 26, 2026
…foreground gate even into the frontmost app, synthetic input never shortens the user's quiet time, a restore is gated too, and window mode refuses an excluded target
Owner
Author
Review fixes 2026-10-26 02:19 from GPT-6.1 Sol judged by Claude Opus 5.5 (fable-judge)Round 1. 5 open threads (eve 5). All 5 fixed by the worker's edits; the lead wrote this reply file and verified the change after the worker's session closed. Fixed in 62c5112. A pointer-moving click skipped the guard when its app was already frontmost (eve-bot-lovinka)
The agent's own synthetic input could erase recent human activity (eve-bot-lovinka)
The synthetic-input file ignored the canonical storage root (eve-bot-lovinka)
Single-window capture did not honour --exclude-window-ids (eve-bot-lovinka)
The plugin version was not bumped for the new skill guidance (eve-bot-lovinka)
Threads: t1 accepted · t2 accepted · t3 accepted · t4 accepted · t5 accepted |
genesiscz
added a commit
that referenced
this pull request
Oct 26, 2026
…foreground gate even into the frontmost app, synthetic input never shortens the user's quiet time, a restore is gated too, and window mode refuses an excluded target
genesiscz
force-pushed
the
fix/2026-10-25-control-background
branch
from
October 26, 2026 01:21
62c5112 to
6784e4a
Compare
There was a problem hiding this comment.
🐉 eve review — 🔴 Changes requested
6784e4a· 3 actionable findings · reviewed by find codex/gpt-6.1-sol + codex/gpt-6-luna, verify codex/gpt-6-luna · view run ↗
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 1 |
Blast radius
bringFrontmost(~) → 3 dependent(s) outside this PR: native/ax-tool/Sources/commands/Scroll.swift, native/ax-tool/Sources/commands/Hotkey.swift, native/ax-tool/Sources/commands/ValueCommands.swiftComputerUse(~) → 3 dependent(s) outside this PR: src/control/mcp/server.tsactionRefusal(~) → 2 dependent(s) outside this PR: src/control/lib/decision/assist.tscomputerSchemas(~) → 2 dependent(s) outside this PR: src/control/mcp/server.tsmoved(+) → 2 dependent(s) outside this PR: src/macos/GenesisTools/Tests/ReviewDiffTests.swift, src/macos/GenesisTools/Tests/PRThreadsTests.swiftRecoveryController(~) → 2 dependent(s) outside this PR: src/control/lib/decision/assist.tsComputerUse.find(~) → 1 dependent(s) outside this PR: src/control/mcp/server.tsComputerUse.launch_app(~) → 1 dependent(s) outside this PR: src/control/mcp/server.ts- graph truncated by caps — dependents above are a lower bound
ℹ️ Claims vs diff — 6 claims could not be checked from the diff (c2, c4, c5, c6, c7, c8).
Not line-anchorable (review without the full diff context)
- 🟠 High
native/ax-tool/Sources/commands/Inspect.swift:190· confidence 90/100 — Apply the foreground guard to window focus and raise commands: The existing publiccontrol window --action focuspath directly activates the application and raises its window without calling either new gate.src/control/commands/discovery.ts:200-228still exposes this through runAx, so even with the new runner enabling GENESIS_CONTROL_FOREGROUND_GUARD, an agent can take focus while the user types. The close branch also raises the window unless --no-raise is supplied. Route these foreground mutations through the shared guard immediately before activation/raise; the project rule requires safety checks at consuming operations, not only selected callers.
genesiscz
added a commit
that referenced
this pull request
Oct 26, 2026
…fore a window raise or a focus move, and only a parsed --allow-foreground flag lifts the guard, never a typed value
Owner
Author
Review fixes 2026-10-26 02:55 from Claude Opus 5.5 judged by Claude Opus 5.5 (fable-judge)Round 2. 2 open eve threads, both security findings (1 High, 1 Medium); both fixed in 1d7a6b1. The lead ran ForegroundGuardTests and WorkflowArgumentsTests (37 tests, 0 failures) and built the ax-tool package. An app already in front skipped the foreground guard before a window raise and a focus move (eve-bot-lovinka)
A typed value that read --allow-foreground lifted the guard (eve-bot-lovinka)
Threads: t1 accepted · t2 accepted |
There was a problem hiding this comment.
🐉 eve review — 🟡 Review comments
1d7a6b1· 1 actionable finding · reviewed by find codex/gpt-6.1-sol + codex/gpt-6-luna, verify codex/gpt-6-luna · view run ↗
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
genesiscz
added a commit
that referenced
this pull request
Oct 26, 2026
…m the preparation check, because it neither raises nor focuses
Owner
Author
Review fixes 2026-10-26 02:59 from Claude Opus 5.5 judged by Claude Opus 5.5 (fable-judge)Round 3. 1 open eve thread (Medium), a regression of round 2's security fix; fixed in ccf9800, applied as eve suggested. The new preparation check refused a prepared right-click that runs in the background (eve-bot-lovinka)
Threads: t1 accepted |
There was a problem hiding this comment.
🐉 eve review — ✅ Approved
ccf9800· 0 actionable findings · reviewed by codex/gpt-6.1-sol · view run ↗
… names (ScreenCaptureKit ignores sharingType none since macOS 15), and a frame the PNG cap keeps out still counts as motion, so a long movie no longer drops to the idle rate
…front or move the pointer is refused while the user worked in the last 5 s, a background click into a window that is not key says it may have done nothing, and every action takes the window_id get_app_state took
…e background first, names the calls that take focus and the new refusal, and records the window_id, list_windows, drag and inactive-window facts agents tripped over
…foreground gate even into the frontmost app, synthetic input never shortens the user's quiet time, a restore is gated too, and window mode refuses an excluded target
…fore a window raise or a focus move, and only a parsed --allow-foreground flag lifts the guard, never a typed value
…m the preparation check, because it neither raises nor focuses
genesiscz
force-pushed
the
fix/2026-10-25-control-background
branch
from
October 26, 2026 02:41
ccf9800 to
2e6e8d0
Compare
There was a problem hiding this comment.
🐉 eve review — 🔴 Changes requested
2e6e8d0· 3 actionable findings · reviewed by find codex/gpt-6.1-sol + codex/gpt-6-luna, verify codex/gpt-6-luna · view run ↗
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
ℹ️ This pass reviewed the full PR diff: the incremental delta since
00ca3010was unusable after a base-branch merge saturated GitHub's compare API (300+ files, 270 of them merge noise, list truncated).
Blast radius
bringFrontmost(~) → 3 dependent(s) outside this PR: native/ax-tool/Sources/commands/ValueCommands.swift, native/ax-tool/Sources/commands/Scroll.swift, native/ax-tool/Sources/commands/Hotkey.swiftComputerUse(~) → 3 dependent(s) outside this PR: src/control/mcp/server.tsactionRefusal(~) → 2 dependent(s) outside this PR: src/control/lib/decision/assist.tscomputerSchemas(~) → 2 dependent(s) outside this PR: src/control/mcp/server.tsRecoveryController(~) → 2 dependent(s) outside this PR: src/control/lib/decision/assist.tsComputerUse.find(~) → 1 dependent(s) outside this PR: src/control/mcp/server.tsComputerUse.launch_app(~) → 1 dependent(s) outside this PR: src/control/mcp/server.ts- graph truncated by caps — dependents above are a lower bound
ℹ️ Claims vs diff — 4 claims could not be checked from the diff (c2, c3, c4, c8).
Not line-anchorable (review without the full diff context)
- 🟠 High
native/ax-tool/Sources/SnapshotWorkflow.swift:1391· confidence 94/100 — Guard explicit AXRaise actions as well as prepared focus: The foreground check covers preparation and workflowFocus, but an unpreparedact --action perform --ax-action AXRaisereaches workflowAXAction directly. That helper verifies that AXRaise is exposed and then calls AXUIElementPerformAction without checking foreground permission. The ComputerUse perform_secondary_action API exposes this path too. An agent can therefore raise an observed window while the user is active, without allow_foreground. This is a separate entry point from the previously reported Inspect.swift window-command issue. Apply the foreground check at the AXRaise dispatch boundary and test that the actual AX dispatch is not reached on refusal, as required by the project's safety-guard rule.
| * The window a call means, as get_app_state takes it. Refused unless the app's latest observation is of that | ||
| * window, so a call never acts on another window's references or pixels. | ||
| */ | ||
| const windowIdSchema = z.number().int().positive().optional(); |
There was a problem hiding this comment.
📋 Spec | 🟡 Medium · confidence 90/100
📋 Spec drift
Window ID is optional on action calls
The intent says, “Every action takes the window_id obtained by get_app_state.” This schema makes window_id optional, and assertObservedWindow skips validation when it is omitted, so calls can still act without naming the observed window.
🧩 Analysis
Grep evidence: const windowIdSchema = z.number().int().positive().optional()
Owner
Author
There was a problem hiding this comment.
Accepted. The action schema should state when a window id is required, so a call without one fails with a clear message instead of acting on the frontmost window. The pull request was merged after its fix rounds, so this finding waits for a follow-up, and this thread stays open until then.
| // A window-addressed left click into an app that is not in front lands in a window that is not key. | ||
| // A SwiftUI control there takes it as the activation click and does nothing, while the click itself | ||
| // was delivered (a sheet's "Cancel" stayed open, 2026-10-23). Say so, so the caller checks the effect. | ||
| if action == "click", workflowFlag("--background"), !secondaryClick, !nonActivatingPanel, startingFrontmost != pid { |
There was a problem hiding this comment.
🧹 Quality | 🟡 Medium · confidence 85/100
Detect inactive windows within the frontmost app too
The new effect note tests only whether another application was frontmost. When the observed target is a non-key window of the already-frontmost app, a background window-addressed click has the same activation-click limitation, but startingFrontmost equals pid and the response omits the warning. Multi-window operation is explicitly supported by the new window_id plumbing, so this misses an important supported case and callers receive plain ok:true for a potentially ineffective click. Base the note on the target window's actual key/focused-window status, not just the application's PID, and cover two windows in one app.
🧩 Analysis
Grep evidence: startingFrontmost != pid|let windowFocused = frontmostPid\(\) == pid|inactiveWindowClick
Owner
Author
There was a problem hiding this comment.
Accepted. The follow-up will detect inactive windows within the frontmost app too. The pull request was merged after its fix rounds, so this finding waits for a follow-up, and this thread stays open until then.
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.
Under an agent, a
tools controlcall that would bring an app to the front or move the pointer is refused while the user works, recordings leave out the windows--exclude-window-idsnames (ScreenCaptureKit ignoressharingType), and the macos-control skill puts working in the background first.Gates on this branch (clean worktree, 2026-10-25):
bun scripts/git/check-commits.ts, roottsc --noEmit, biome on the changed files, the related tests, andscripts/ci/placeholder-check.sh. Tests: 4 files, 177 pass.swift test --package-path native/ax-tool: 432 tests, 0 failures.Split out of the unified branch (#507); each commit is a cherry-pick of its unified original.