fix(bin): prevent Codex visual workspace leaks#665
Open
AviKaufman wants to merge 11 commits into
Open
Conversation
4 tasks
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.
Intent
Fix Codex-owned browser leakage from Firstmate visual workflows. The branch adds bin/fm-visual-guard.sh, routes generated briefs/docs to use it, preserves user browser behavior, and hardens guarded launches with dedicated FirstmateVisual identity, legacy CodexVisual/codex-visual-browser coverage, Hyprland client remediation to hidden workspace 99 on CODEX-HEADLESS, post-dispatch client tracking, stable hidden settling before success, numeric monitor resolution, robust client-field parsing, and focused tests for the exact leak and settle regressions. It also includes the narrow session-start missing-node fixture isolation. The required report remains private at /home/avi/dev/tools/firstmate/data/codex-browser-workspace-leak-k8/report.md; no tracked report artifact should be added. The PR diff must stay scoped to the browser-isolation task files.
What Changed
CODEX-HEADLESS, and fails closed unless placement remains stable.PATH.Risk Assessment
✅ Low: Captain, the prior fail-open defect is fixed across all client-query paths, the regression coverage is deterministic, and the complete diff remains scoped to the required browser-isolation files with no tracked report artifact.
Testing
The supplied full baseline and focused regression suites passed; a real guarded Chrome launch appeared as
FirstmateVisualon workspace 99 /CODEX-HEADLESSwhile visible workspaces 1–3 remained unchanged. The locked host causedgrimto capture black or the compositor lock surface rather than browser content, so no misleading screenshot is presented; the rendered HTML and exact live placement transcript are retained as evidence.Evidence: Live visual-guard transcript
Evidence: Rendered browser proof page
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-visual-guard.sh:297- Captain,client_rowsis the producer in a pipeline whose finalwhileloop determines the status, while the script enables nopipefail. A failedhyprctl clients -j, monitor query, orjqparse therefore becomes an empty client set; verification can subsequently report success without proving placement, potentially leaving a visible leak. Capture and validateclient_rowsbefore iterating, or otherwise propagate producer failures.🔧 Fix: Fail closed on visual client query errors
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"Supplied successful baseline:command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bash tests/fm-visual-guard.test.shbash tests/fm-brief.test.shbash tests/fm-session-start.test.shScope verification:git diff --name-status 85643eec1f8cbe2136895c7dbe4277899f057348..7dc44534398c7cab0416c57c71daf372e2ee3c87and tracked-report checkbin/fm-visual-guard.sh doctorFM_VISUAL_VERIFY_ATTEMPTS=30 FM_VISUAL_VERIFY_SLEEP=0.2 bin/fm-visual-guard.sh browser file:///tmp/no-mistakes-evidence/01KXQ62Y2EQC3TPM62860V9HZ4/visual-guard-proof.htmlbin/fm-visual-guard.sh clientsplus before/afterhyprctlmonitor, workspace, and client readbacksbin/fm-visual-guard.sh screenshot /tmp/no-mistakes-evidence/01KXQ62Y2EQC3TPM62860V9HZ4/firstmate-visual-guard-hidden.pngand visual inspectionExact-address browser cleanup and verification that visible workspaces and DPMS state were restored🔧 **Document** - 1 issue found → auto-fixed ✅
AGENTS.md:145- The unconditional visual-guard rule conflicts with Firstmate's documented macOS support: the guard requires Hyprland/hyprctl. Resolving this requires either a non-Hyprland fallback/platform gate or intentionally narrowing platform support.🔧 Fix: Scope visual guard docs to Hyprland workflows
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.