Repository navigation
Fix browser screenshot to return image URL - #936
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR implements comprehensive screenshot output functionality to address issue Changes
Sequence DiagramsequenceDiagram
actor CLI as CLI Handler
participant FS as Filesystem
participant TC as TerminalController
participant Browser as Browser Surface
participant Parser as JSON/Output
CLI->>TC: v2BrowserScreenshot(contentWorld)
TC->>Browser: evaluateJavaScript/callAsyncJavaScript
Browser-->>TC: png_base64 or file_path
TC->>FS: Save PNG to temp directory<br/>(or explicit --out path)
TC->>TC: Compute file URL & path
TC->>TC: Mutate payload with path/url fields
TC-->>CLI: enriched result (path, url, png_base64)
alt --json flag
CLI->>Parser: Format JSON output
Parser->>Parser: Remove png_base64 if needed
Parser-->>CLI: emit JSON with path/url
else default
CLI-->>CLI: emit path/URL or OK
end
CLI->>FS: Prune old screenshots<br/>(bestEffortPruneTemporaryFiles)
FS-->>CLI: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef2f52186f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let filename = "surface-\(shortSurfaceId)-\(timestampMs)-\(shortRandomId).png" | ||
| let imageURL = screenshotsDirectory.appendingPathComponent(filename, isDirectory: false) | ||
|
|
||
| do { | ||
| try imageData.write(to: imageURL, options: .atomic) |
There was a problem hiding this comment.
Each browser.screenshot call now writes a uniquely named PNG under the app temp directory, but this path is never pruned or reused, so repeated automation runs will accumulate files indefinitely and can eventually hit disk-pressure failures. This regression is introduced here because screenshots previously stayed in-memory (png_base64) without persistent storage side effects.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR makes Key changes:
Issues found:
Confidence Score: 3/5
Last reviewed commit: ef2f521 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| screenshotURL = outputURL.absoluteString | ||
| payload["path"] = screenshotPath | ||
| payload["url"] = screenshotURL | ||
| } else if screenshotPath == nil || screenshotURL == nil { |
There was a problem hiding this comment.
The OR condition will discard a valid server response if only one field is missing. For example, if the server returns path but not url (e.g., from a partial update), the valid path is thrown away and a duplicate temp file is created from base64.
The fallback should only fire when both fields are absent (older server). Use AND instead:
| } else if screenshotPath == nil || screenshotURL == nil { | |
| } else if screenshotPath == nil && screenshotURL == nil { |
| "surface_id": surfaceId.uuidString, | ||
| "surface_ref": v2Ref(kind: .surface, uuid: surfaceId), | ||
| "png_base64": imageData.base64EncodedString() | ||
| "png_base64": imageData.base64EncodedString(), |
There was a problem hiding this comment.
png_base64 is now redundant. Since the server always writes the PNG to disk and returns path and url, including the base64-encoded image in every response means each screenshot IPC call transmits the full image twice (~33% larger on the wire). For a 1 MB screenshot, this adds ~1.3 MB of unnecessary data.
The CLI doesn't use the server's temp file when --out is specified (it decodes base64 instead), so the server-side file is always created but unused in that path. Consider removing png_base64 from the response now that file path is always available, or update the CLI's --out branch to copy from path instead of decoding base64.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f749758d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try FileManager.default.createDirectory(at: outputDir, withIntermediateDirectories: true) | ||
| let timestampMs = Int(Date().timeIntervalSince1970 * 1000) | ||
| let filename = "surface-\(sid)-\(timestampMs)-\(String(UUID().uuidString.prefix(8))).png" | ||
| let outputURL = outputDir.appendingPathComponent(filename, isDirectory: false) | ||
| try writeScreenshot(data, to: outputURL) |
There was a problem hiding this comment.
The app-side v2BrowserScreenshot now treats temp-file persistence as best-effort and can return only png_base64 when path/url writes fail, but this CLI fallback path still uses throwing writes. If the temp directory is unwritable or full, createDirectory/writeScreenshot throws here and the whole browser screenshot command fails even though usable image data is already present in the payload (notably for --json callers). This should be non-fatal so the command can still return the screenshot payload when URL generation is impossible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f15164353b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let timestampMs = Int(Date().timeIntervalSince1970 * 1000) | ||
| let filename = "surface-\(sid)-\(timestampMs)-\(String(UUID().uuidString.prefix(8))).png" | ||
| let outputURL = outputDir.appendingPathComponent(filename, isDirectory: false) | ||
| try writeScreenshot(data, to: outputURL) |
There was a problem hiding this comment.
This fallback path writes each screenshot to a new filename under cmux-browser-screenshots-cli whenever the daemon payload lacks path/url (for example, CLI/daemon version skew or app-side temp-write failure), but nothing ever removes or reuses those files. In long-running automation this will accumulate unbounded temp data and can eventually cause disk-pressure failures, so the CLI should apply retention/cleanup for these transport-only artifacts.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/TerminalController.swift (1)
6719-6730: Add retention control for temp screenshots to avoid disk growth.On Line 6719 through Line 6730, each call creates another PNG under
cmux-browser-screenshotswith no pruning. In long-running automation this can accumulate quickly and cause disk pressure. Consider pruning by age/count after successful writes.♻️ Suggested lightweight pruning pattern
let screenshotsDirectory = FileManager.default.temporaryDirectory .appendingPathComponent("cmux-browser-screenshots", isDirectory: true) if (try? FileManager.default.createDirectory(at: screenshotsDirectory, withIntermediateDirectories: true)) != nil { + // Best-effort retention guard: keep temp screenshot directory bounded. + if let existing = try? FileManager.default.contentsOfDirectory( + at: screenshotsDirectory, + includingPropertiesForKeys: [.contentModificationDateKey], + options: [.skipsHiddenFiles] + ) { + let pngs = existing.filter { $0.pathExtension.lowercased() == "png" } + if pngs.count > 500 { + let sorted = pngs.sorted { + let l = (try? $0.resourceValues(forKeys: [.contentModificationDateKey]).contentModificationDate) ?? .distantPast + let r = (try? $1.resourceValues(forKeys: [.contentModificationDateKey]).contentModificationDate) ?? .distantPast + return l < r + } + for url in sorted.prefix(max(0, pngs.count - 500)) { + try? FileManager.default.removeItem(at: url) + } + } + } + let timestampMs = Int(Date().timeIntervalSince1970 * 1000) let shortSurfaceId = String(surfaceId.uuidString.prefix(8)) let shortRandomId = String(UUID().uuidString.prefix(8)) let filename = "surface-\(shortSurfaceId)-\(timestampMs)-\(shortRandomId).png" let imageURL = screenshotsDirectory.appendingPathComponent(filename, isDirectory: false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 6719 - 6730, The temp screenshot logic that creates screenshotsDirectory and writes imageURL currently never prunes old files; update the success path (after imageURL is written and result is set) to perform lightweight retention: list contents of screenshotsDirectory via FileManager, read file attributes (creationDate or contentModificationDate), sort and delete files older than a configured age (e.g., 7 days) or keep only the most recent N files (e.g., 100), deleting the rest; perform the pruning asynchronously (dispatch to a background queue) so the write operation using imageData, filename, imageURL and result remains fast and non-blocking, and handle errors silently/log via processLogger or similar.tests/test_browser_screenshot_cli_output_regression.py (1)
26-44:extract_blockis fragile for braces inside strings/commentsThe raw brace counter treats every
{/}as syntax, so unrelated braces in Swift string/comment text can cause false regression failures. Consider a lightweight tokenizer that ignores string/comment regions before depth counting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_browser_screenshot_cli_output_regression.py` around lines 26 - 44, The extract_block function miscounts braces because it treats braces inside Swift string literals and comments as code; update extract_block to first scan from brace_start with a simple state machine that tracks and ignores braces when inside double-quoted strings (respecting escaped quotes and Swift multiline string delimiters), single-line comments (// until newline), and block comments (/* ... */), only incrementing/decrementing depth for braces encountered while in normal code; keep references to the existing variables (signature, brace_start, depth) and return the same substring when depth returns to zero or raise the same errors on unbalanced/missing braces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 3055-3108: The outPathOpt copy branch fails to create the
destination parent directory before FileManager.copyItem and the temp filename
can contain unsafe characters from sid; fix by (1) in the outPathOpt branch
where you handle sourcePath (payload["path"]) call
FileManager.default.createDirectory(at: outputURL.deletingLastPathComponent(),
withIntermediateDirectories: true) before any removeItem/copyItem operations
(place this next to where writeScreenshot creates dirs and reference
outputURL/deletingLastPathComponent()), and (2) when building the temp filename
in the fallback branch sanitize sid (e.g. replace characters like ":" with "-"
or otherwise normalize it) before interpolating into filename (the code around
timestampMs/filename/let filename = "surface-\(sid)-..."). Ensure you still
update payload["path"] and payload["url"] after these changes.
In `@tests/test_browser_screenshot_cli_output_regression.py`:
- Around line 16-23: The repo_root() helper currently calls
subprocess.run(["git", "rev-parse", "--show-toplevel"]) which raises
FileNotFoundError when git is missing and prevents the fallback
Path(__file__).resolve().parents[1]; update repo_root() to catch exceptions from
subprocess.run (at least FileNotFoundError and CalledProcessError) and treat
them like a non-zero returncode so the function returns the fallback Path.
Ensure you reference the existing subprocess.run call and the fallback
Path(__file__).resolve().parents[1] in your change.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 6719-6730: The temp screenshot logic that creates
screenshotsDirectory and writes imageURL currently never prunes old files;
update the success path (after imageURL is written and result is set) to perform
lightweight retention: list contents of screenshotsDirectory via FileManager,
read file attributes (creationDate or contentModificationDate), sort and delete
files older than a configured age (e.g., 7 days) or keep only the most recent N
files (e.g., 100), deleting the rest; perform the pruning asynchronously
(dispatch to a background queue) so the write operation using imageData,
filename, imageURL and result remains fast and non-blocking, and handle errors
silently/log via processLogger or similar.
In `@tests/test_browser_screenshot_cli_output_regression.py`:
- Around line 26-44: The extract_block function miscounts braces because it
treats braces inside Swift string literals and comments as code; update
extract_block to first scan from brace_start with a simple state machine that
tracks and ignores braces when inside double-quoted strings (respecting escaped
quotes and Swift multiline string delimiters), single-line comments (// until
newline), and block comments (/* ... */), only incrementing/decrementing depth
for braces encountered while in normal code; keep references to the existing
variables (signature, brace_start, depth) and return the same substring when
depth returns to zero or raise the same errors on unbalanced/missing braces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21b3bf79-5d15-4c5d-9d43-beed34d86111
📒 Files selected for processing (3)
CLI/cmux.swiftSources/TerminalController.swifttests/test_browser_screenshot_cli_output_regression.py
| result = subprocess.run( | ||
| ["git", "rev-parse", "--show-toplevel"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.returncode == 0: | ||
| return Path(result.stdout.strip()) | ||
| return Path(__file__).resolve().parents[1] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/test_browser_screenshot_cli_output_regression.py | head -40Repository: manaflow-ai/cmux
Length of output: 1455
🏁 Script executed:
python3 << 'EOF'
import subprocess
import sys
# Test 1: Verify FileNotFoundError when executable missing
print("Test 1: subprocess.run with non-existent executable")
try:
result = subprocess.run(
["git_nonexistent_command_xyz", "--version"],
capture_output=True,
text=True,
)
print(f" Result: returncode={result.returncode}, stdout={result.stdout}")
except FileNotFoundError as e:
print(f" FileNotFoundError raised: {e}")
except Exception as e:
print(f" Other exception: {type(e).__name__}: {e}")
# Test 2: Verify behavior with check=False
print("\nTest 2: subprocess.run with check=False on non-existent executable")
try:
result = subprocess.run(
["git_nonexistent_command_xyz", "--version"],
capture_output=True,
text=True,
check=False,
)
print(f" Result: returncode={result.returncode}")
except FileNotFoundError as e:
print(f" FileNotFoundError raised: {e}")
except Exception as e:
print(f" Other exception: {type(e).__name__}: {e}")
# Test 3: Verify behavior with OSError catching
print("\nTest 3: subprocess.run wrapped in try-except OSError")
try:
try:
result = subprocess.run(
["git_nonexistent_command_xyz", "--version"],
capture_output=True,
text=True,
check=False,
)
print(f" Result: returncode={result.returncode}")
except OSError as e:
print(f" OSError caught: {type(e).__name__}: {e}")
except Exception as e:
print(f" Uncaught: {type(e).__name__}: {e}")
EOFRepository: manaflow-ai/cmux
Length of output: 527
Handle missing git executable in repo_root()
On Line 16, subprocess.run(["git", ...]) raises FileNotFoundError when git is unavailable, so the fallback on Line 23 is never reached.
🔧 Proposed fix
def repo_root() -> Path:
- result = subprocess.run(
- ["git", "rev-parse", "--show-toplevel"],
- capture_output=True,
- text=True,
- )
- if result.returncode == 0:
- return Path(result.stdout.strip())
+ try:
+ result = subprocess.run(
+ ["git", "rev-parse", "--show-toplevel"],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode == 0 and result.stdout.strip():
+ return Path(result.stdout.strip())
+ except OSError:
+ pass
return Path(__file__).resolve().parents[1]🧰 Tools
🪛 Ruff (0.15.2)
[error] 17-17: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_browser_screenshot_cli_output_regression.py` around lines 16 - 23,
The repo_root() helper currently calls subprocess.run(["git", "rev-parse",
"--show-toplevel"]) which raises FileNotFoundError when git is missing and
prevents the fallback Path(__file__).resolve().parents[1]; update repo_root() to
catch exceptions from subprocess.run (at least FileNotFoundError and
CalledProcessError) and treat them like a non-zero returncode so the function
returns the fallback Path. Ensure you reference the existing subprocess.run call
and the fallback Path(__file__).resolve().parents[1] in your change.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_browser_wait_regression.py">
<violation number="1" location="tests/test_browser_wait_regression.py:58">
P2: The regression guard now checks `contentWorld: WKContentWorld` across the whole file, which can miss a real regression in `v2RunJavaScript` if the same text appears elsewhere.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_browser_wait_regression.py">
<violation number="1" location="tests/test_browser_wait_regression.py:58">
P3: The new missing-declaration check is ineffective because `extract_block(...)` is still called unconditionally afterward, so this path still crashes instead of reporting the intended failure message.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
3075-3085:⚠️ Potential issue | 🟠 MajorCreate
--outparent directories before copy, and sanitizesidin temp filenames.Line 3084 can fail when the destination directory doesn’t exist, and Line 3100 still uses raw
sidas a filename component.Suggested patch
} else if let sourcePath = payload["path"] as? String { - let sourceURL = URL(fileURLWithPath: sourcePath) + let sourceURL = URL(fileURLWithPath: sourcePath).standardizedFileURL if sourceURL.path != outputURL.path { + try FileManager.default.createDirectory( + at: outputURL.deletingLastPathComponent(), + withIntermediateDirectories: true + ) try? FileManager.default.removeItem(at: outputURL) try FileManager.default.copyItem(at: sourceURL, to: outputURL) } } else { throw CLIError(message: "browser screenshot missing image data") @@ try FileManager.default.createDirectory(at: outputDir, withIntermediateDirectories: true) let timestampMs = Int(Date().timeIntervalSince1970 * 1000) - let filename = "surface-\(sid)-\(timestampMs)-\(String(UUID().uuidString.prefix(8))).png" + let safeSid = sid.replacingOccurrences( + of: #"[^A-Za-z0-9._-]+"#, + with: "-", + options: .regularExpression + ) + let filename = "surface-\(safeSid)-\(timestampMs)-\(String(UUID().uuidString.prefix(8))).png" let outputURL = outputDir.appendingPathComponent(filename, isDirectory: false)Also applies to: 3099-3101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3075 - 3085, The copy operation can fail if the destination directory doesn't exist and temp filenames use unsanitized sid; before copying in the block that handles outPathOpt and sourcePath (look for outPathOpt, fileURL(fromPath:), sourcePath and the FileManager.copyItem call) ensure the output directory exists by creating parent directories for outputURL (using FileManager.createDirectory(at:withIntermediateDirectories:)) and handle errors; also sanitize the sid used when constructing temp filenames (escape or strip problematic characters) wherever sid is interpolated into filenames (search for usages of sid in temp filename construction) so no invalid path components are produced; apply the same fixes to the other copy block mentioned (lines around 3099–3101).
🧹 Nitpick comments (2)
tests/test_browser_wait_regression.py (1)
10-39: Consider consolidating shared static-regression helpers.
repo_root()andextract_block()are now duplicated across multiple regression scripts. Moving them to a small shared test utility module would reduce drift and keep future parser fixes centralized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_browser_wait_regression.py` around lines 10 - 39, Move the duplicated helpers into a single shared test utility module and update the regression scripts to import them: create a new test helper module exposing repo_root() and extract_block() (preserve their current signatures and behavior), remove the duplicate implementations from individual regression scripts, and change those scripts to import repo_root and extract_block from the shared module; ensure the shared functions include the same error types/messages so existing tests keep working.Sources/TerminalController.swift (1)
6815-6839: Consider bounded retention for temp browser screenshots.The new temp-file export is useful, but the
cmux-browser-screenshotsdirectory grows unbounded. A simple cap (count/age) would prevent long-running agent sessions from accumulating large disk usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 6815 - 6839, The temporary screenshots directory (screenshotsDirectory) is currently unbounded; after successfully writing imageData to imageURL in the existing write block, add a pruning step that enforces a retention policy (e.g., keep only the newest N files or delete files older than M days). Implement this by enumerating FileManager.default.contentsOfDirectory(at:includingPropertiesForKeys: [.creationDateKey], options: []), sorting by creationDate (or parsing the timestamp in the filename), deleting older files until only the most recent N remain or removing files with creationDate older than the age threshold, and swallow/log any errors (do not crash the handler). Keep these changes local to the same scope that uses screenshotsDirectory/imageURL so behavior remains best-effort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Line 6861: Update the CLI usage string that advertises the "browser
screenshot" command to include the new local JSON flag by adding "`--json`"
alongside the existing "`--out <path>`" option; locate the usage/help text that
contains the literal "browser screenshot [--out <path>]" (search for that string
in CLI/cmux.swift or the function that builds command usage/help like the
command registration for the "browser screenshot" subcommand) and modify it to
read "browser screenshot [--out <path>] [--json]" (or equivalent) so the
displayed help matches the implemented behavior.
In `@Sources/TerminalController.swift`:
- Around line 6366-6370: The wait condition in browser.wait is built from
v2BrowserSelector and v2JSONLiteral but does not resolve element refs (eN /
`@eN`), so replace the raw selector resolution with v2BrowserResolveSelector
before creating the literal: call v2BrowserResolveSelector(selector, params) (or
the equivalent resolver used elsewhere) to expand any element_ref/ref tokens,
then pass the resolved selector into v2JSONLiteral and build the querySelector
check; apply the same change to the other wait branch(s) referenced around the
6403-6424 area to ensure all wait conditions resolve snapshot refs.
In `@tests/test_browser_eval_async_wrapper_regression.py`:
- Around line 67-69: Calls to extract_block(...) (e.g., when populating
run_browser_js_block from terminal_source and the other two blocks) can raise
ValueError and currently abort the script; instead wrap each extract_block
invocation in a try/except that catches ValueError, appends a descriptive
message to the failures list (including which block name failed and the error
text), and continues so the script can print collected failures later; locate
the three extract_block usages (the one assigning run_browser_js_block and the
two at the other flagged spots) and replace the direct calls with this
try/except+failures append pattern while preserving the original variable names.
In `@tests/test_browser_wait_regression.py`:
- Line 49: The test currently calls extract_block(...) (e.g., when assigning
wait_block = extract_block(terminal_source, "private func v2BrowserWait(params:
[String: Any]) -> V2CallResult") and at the two other extract_block call sites)
and lets a ValueError propagate; wrap each extract_block call in a try/except
ValueError that catches the failure, appends a descriptive message including the
signature and exception text to the failures list, and sets the related block
variable to None (or otherwise skips assertions that depend on it) so the test
records a failure without raising a traceback.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 3075-3085: The copy operation can fail if the destination
directory doesn't exist and temp filenames use unsanitized sid; before copying
in the block that handles outPathOpt and sourcePath (look for outPathOpt,
fileURL(fromPath:), sourcePath and the FileManager.copyItem call) ensure the
output directory exists by creating parent directories for outputURL (using
FileManager.createDirectory(at:withIntermediateDirectories:)) and handle errors;
also sanitize the sid used when constructing temp filenames (escape or strip
problematic characters) wherever sid is interpolated into filenames (search for
usages of sid in temp filename construction) so no invalid path components are
produced; apply the same fixes to the other copy block mentioned (lines around
3099–3101).
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 6815-6839: The temporary screenshots directory
(screenshotsDirectory) is currently unbounded; after successfully writing
imageData to imageURL in the existing write block, add a pruning step that
enforces a retention policy (e.g., keep only the newest N files or delete files
older than M days). Implement this by enumerating
FileManager.default.contentsOfDirectory(at:includingPropertiesForKeys:
[.creationDateKey], options: []), sorting by creationDate (or parsing the
timestamp in the filename), deleting older files until only the most recent N
remain or removing files with creationDate older than the age threshold, and
swallow/log any errors (do not crash the handler). Keep these changes local to
the same scope that uses screenshotsDirectory/imageURL so behavior remains
best-effort.
In `@tests/test_browser_wait_regression.py`:
- Around line 10-39: Move the duplicated helpers into a single shared test
utility module and update the regression scripts to import them: create a new
test helper module exposing repo_root() and extract_block() (preserve their
current signatures and behavior), remove the duplicate implementations from
individual regression scripts, and change those scripts to import repo_root and
extract_block from the shared module; ensure the shared functions include the
same error types/messages so existing tests keep working.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e33003c-6c69-4cfd-9b52-34050970e8e4
📒 Files selected for processing (4)
CLI/cmux.swiftSources/TerminalController.swifttests/test_browser_eval_async_wrapper_regression.pytests/test_browser_wait_regression.py
| run_browser_js_block = extract_block( | ||
| terminal_source, "private func v2RunBrowserJavaScript(" | ||
| ) |
There was a problem hiding this comment.
Prevent hard-fail exceptions when expected Swift blocks are missing.
At Line 67, Line 92, and Line 125, extract_block(...) can raise ValueError and abort the script before the collected failures are printed. This breaks the regression guard’s fail-reporting contract.
Proposed fix
+def extract_block_or_record(
+ source: str, signature: str, failures: list[str], failure_message: str
+) -> str:
+ try:
+ return extract_block(source, signature)
+ except ValueError:
+ failures.append(failure_message)
+ return ""
+
@@
- run_browser_js_block = extract_block(
- terminal_source, "private func v2RunBrowserJavaScript("
- )
+ run_browser_js_block = extract_block_or_record(
+ terminal_source,
+ "private func v2RunBrowserJavaScript(",
+ failures,
+ "missing or malformed v2RunBrowserJavaScript() block",
+ )
@@
- hook_block = extract_block(
- terminal_source, "private func v2BrowserEnsureTelemetryHooks("
- )
+ hook_block = extract_block_or_record(
+ terminal_source,
+ "private func v2BrowserEnsureTelemetryHooks(",
+ failures,
+ "missing or malformed v2BrowserEnsureTelemetryHooks() block",
+ )
@@
- panel_init_block = extract_block(
- panel_source,
- "private static func makeWebView() -> CmuxWebView",
- )
+ panel_init_block = extract_block_or_record(
+ panel_source,
+ "private static func makeWebView() -> CmuxWebView",
+ failures,
+ "missing or malformed BrowserPanel.makeWebView() block",
+ )Also applies to: 92-94, 125-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_browser_eval_async_wrapper_regression.py` around lines 67 - 69,
Calls to extract_block(...) (e.g., when populating run_browser_js_block from
terminal_source and the other two blocks) can raise ValueError and currently
abort the script; instead wrap each extract_block invocation in a try/except
that catches ValueError, appends a descriptive message to the failures list
(including which block name failed and the error text), and continues so the
script can print collected failures later; locate the three extract_block usages
(the one assigning run_browser_js_block and the two at the other flagged spots)
and replace the direct calls with this try/except+failures append pattern while
preserving the original variable names.
| terminal_source = (root / "Sources" / "TerminalController.swift").read_text(encoding="utf-8") | ||
| cli_source = (root / "CLI" / "cmux.swift").read_text(encoding="utf-8") | ||
|
|
||
| wait_block = extract_block(terminal_source, "private func v2BrowserWait(params: [String: Any]) -> V2CallResult") |
There was a problem hiding this comment.
Guard block extraction failures to preserve stable FAIL output.
At Line 49, Line 71, and Line 79, a missing signature triggers ValueError and exits with a traceback instead of appending to failures. That makes regressions harder to diagnose.
Proposed fix
+def extract_block_or_record(
+ source: str, signature: str, failures: list[str], failure_message: str
+) -> str:
+ try:
+ return extract_block(source, signature)
+ except ValueError:
+ failures.append(failure_message)
+ return ""
+
@@
- wait_block = extract_block(terminal_source, "private func v2BrowserWait(params: [String: Any]) -> V2CallResult")
+ wait_block = extract_block_or_record(
+ terminal_source,
+ "private func v2BrowserWait(params: [String: Any]) -> V2CallResult",
+ failures,
+ "missing or malformed v2BrowserWait() block",
+ )
@@
- run_js_block = extract_block(terminal_source, "private func v2RunJavaScript(")
+ run_js_block = extract_block_or_record(
+ terminal_source,
+ "private func v2RunJavaScript(",
+ failures,
+ "missing or malformed v2RunJavaScript() block",
+ )
@@
- run_browser_js_block = extract_block(terminal_source, "private func v2RunBrowserJavaScript(")
+ run_browser_js_block = extract_block_or_record(
+ terminal_source,
+ "private func v2RunBrowserJavaScript(",
+ failures,
+ "missing or malformed v2RunBrowserJavaScript() block",
+ )Also applies to: 71-71, 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_browser_wait_regression.py` at line 49, The test currently calls
extract_block(...) (e.g., when assigning wait_block =
extract_block(terminal_source, "private func v2BrowserWait(params: [String:
Any]) -> V2CallResult") and at the two other extract_block call sites) and lets
a ValueError propagate; wrap each extract_block call in a try/except ValueError
that catches the failure, appends a descriptive message including the signature
and exception text to the failures list, and sets the related block variable to
None (or otherwise skips assertions that depend on it) so the test records a
failure without raising a traceback.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_browser_eval_async_wrapper_regression.py">
<violation number="1">
P2: The regression guard now inspects `BrowserPanel.init(...)` for telemetry user-script tokens that actually live in `makeWebView()`, causing false test failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d5a033d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "workspace_ref": self.v2Ref(kind: .workspace, uuid: workspaceId), | ||
| "surface_id": surfaceIdOut.uuidString, | ||
| "surface_ref": self.v2Ref(kind: .surface, uuid: surfaceIdOut), |
There was a problem hiding this comment.
v2BrowserWait now leaves v2MainSync before polling, but this success path still calls v2Ref(...) to populate workspace_ref/surface_ref; v2Ref mutates shared handle maps (v2RefByUUID, v2UUIDByRef, v2NextHandleOrdinal) with no synchronization. Because socket requests are processed on detached per-client threads, concurrent v2 calls can race these writes and intermittently corrupt refs or crash. Please compute these refs under main-thread synchronization (or protect the maps with a lock) before returning.
Useful? React with 👍 / 👎.
* Return browser screenshot image URL * Make screenshot path/url best effort * cli: omit screenshot png_base64 from json output * browser wait: fail fast on js errors and include screenshot in help * browser wait: avoid main-actor default world warning * tests: scope contentWorld regression check to function signature * browser screenshot: clean up output handling and tests * browser wait: resolve snapshot refs in selector waits
This branch was successfully deployed
1 participant
Summary
browser.screenshotsave a PNG to a temp file on the app side and return bothpathandurlin the v2 payload.cmux browser <surface> screenshotto print an accessible image URL in non-JSON mode and preserve--outbehavior.--jsonon the screenshot subcommand path (cmux browser <surface> screenshot --json) and add a static regression guard.Testing
python3 Tests/test_browser_screenshot_cli_output_regression.py(pass)python3 Tests/test_browser_eval_cli_output_regression.py(pass)python3 Tests/test_browser_console_errors_cli_output_regression.py(pass)xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' build(pass)Issues
Summary by cubic
Return a real file:// URL from browser.screenshot and print it by default in the CLI. Cleans up output handling, temp file management, and wait/eval reliability.
Written for commit 7d5a033. Summary will update on new commits.
Summary by CodeRabbit
New Features
--jsonflag to browser screenshot command for JSON-formatted output.--outflag to browser screenshot command for specifying custom output paths.Documentation
Tests