Repository navigation
Document browser navigation verification loop - #974
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds an explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/cmux-browser/templates/authenticated-session.sh (1)
13-15: Consider enforcing URL validity before waiting.Line 13 retrieves the URL, but Line 14 still runs even if navigation is empty/
about:blank. Adding a small guard here would make this template fail fast instead of entering a misleading wait path.Proposed tweak
cmux browser "$SURFACE" goto "$DASHBOARD_URL" -cmux browser "$SURFACE" get url +CURRENT_URL="$(cmux browser "$SURFACE" get url | tr -d '\r\n')" +if [ -z "$CURRENT_URL" ] || [ "$CURRENT_URL" = "about:blank" ]; then + echo "Navigation not ready (url='$CURRENT_URL'); navigate first before wait." >&2 + exit 1 +fi cmux browser "$SURFACE" wait --load-state complete --timeout-ms 15000 cmux browser "$SURFACE" snapshot --interactive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/cmux-browser/templates/authenticated-session.sh` around lines 13 - 15, After calling cmux browser "$SURFACE" get url, read the resulting URL and validate it is non-empty and not "about:blank" before running cmux browser "$SURFACE" wait --load-state complete --timeout-ms 15000; if the URL is empty or about:blank, emit an error/exit and skip the wait/snapshot steps. Locate the three commands (cmux browser "$SURFACE" get url, wait --load-state complete --timeout-ms 15000, snapshot --interactive) and add a guard between the get and wait that checks the retrieved URL value and fails fast on invalid navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/cmux-browser/SKILL.md`:
- Around line 84-86: The example's described flow includes an explicit
"navigate" step but the snippet begins with `cmux browser surface:7 get url`;
add an explicit navigation command (e.g., `cmux browser surface:7 goto <url>` or
`cmux browser surface:7 open <url>`) before the `get url` and `wait` lines so
the sample matches "navigate -> verify -> wait -> snapshot" and is executable as
shown.
---
Nitpick comments:
In `@skills/cmux-browser/templates/authenticated-session.sh`:
- Around line 13-15: After calling cmux browser "$SURFACE" get url, read the
resulting URL and validate it is non-empty and not "about:blank" before running
cmux browser "$SURFACE" wait --load-state complete --timeout-ms 15000; if the
URL is empty or about:blank, emit an error/exit and skip the wait/snapshot
steps. Locate the three commands (cmux browser "$SURFACE" get url, wait
--load-state complete --timeout-ms 15000, snapshot --interactive) and add a
guard between the get and wait that checks the retrieved URL value and fails
fast on invalid navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e073585e-36c2-4842-9678-e09254953fe1
📒 Files selected for processing (3)
skills/cmux-browser/SKILL.mdskills/cmux-browser/templates/authenticated-session.shskills/cmux-browser/templates/form-automation.sh
| # navigate -> verify -> wait -> snapshot -> action -> snapshot | ||
| cmux browser surface:7 get url | ||
| cmux browser surface:7 wait --load-state complete --timeout-ms 15000 |
There was a problem hiding this comment.
Stable loop snippet is missing the explicit navigate step.
Line 84 says navigate -> verify -> wait -> snapshot, but the snippet starts at get url on Line 85. Add a goto/open command so the example is executable as written.
Suggested doc fix
# navigate -> verify -> wait -> snapshot -> action -> snapshot
+cmux browser surface:7 goto https://example.com
cmux browser surface:7 get url
cmux browser surface:7 wait --load-state complete --timeout-ms 15000
cmux browser surface:7 snapshot --interactive📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # navigate -> verify -> wait -> snapshot -> action -> snapshot | |
| cmux browser surface:7 get url | |
| cmux browser surface:7 wait --load-state complete --timeout-ms 15000 | |
| # navigate -> verify -> wait -> snapshot -> action -> snapshot | |
| cmux browser surface:7 goto https://example.com | |
| cmux browser surface:7 get url | |
| cmux browser surface:7 wait --load-state complete --timeout-ms 15000 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/cmux-browser/SKILL.md` around lines 84 - 86, The example's described
flow includes an explicit "navigate" step but the snippet begins with `cmux
browser surface:7 get url`; add an explicit navigation command (e.g., `cmux
browser surface:7 goto <url>` or `cmux browser surface:7 open <url>`) before the
`get url` and `wait` lines so the sample matches "navigate -> verify -> wait ->
snapshot" and is executable as shown.
![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: 85e4724e3d
ℹ️ 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".
| cmux --json browser surface:7 click e2 --snapshot-after | ||
| cmux browser surface:7 snapshot --interactive |
There was a problem hiding this comment.
The updated core loop now runs click ... --snapshot-after and immediately snapshots again without any explicit wait, which makes the recommended flow unreliable when the click triggers async DOM updates or navigation. In those cases the immediate snapshot can still capture the pre-update page and produce stale refs for the next step; the prior sequence included a wait to synchronize on page state before re-snapshotting.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR documents a Key changes:
Findings:
Confidence Score: 4/5
Sequence DiagramsequenceDiagram
participant Agent
participant cmux
participant Browser
Agent->>cmux: browser open <url> --json
cmux->>Browser: navigate to URL
cmux-->>Agent: surface:N ref
Agent->>cmux: browser surface:N get url
cmux->>Browser: query current URL
Browser-->>cmux: URL string (or about:blank)
cmux-->>Agent: URL output (stdout only — not checked in templates)
alt URL is about:blank / empty
Note over Agent: PR tip says: navigate first
Agent->>cmux: browser surface:N goto <url>
else URL looks correct
Agent->>cmux: browser surface:N wait --load-state complete
cmux->>Browser: wait for load state
Browser-->>cmux: ready
Agent->>cmux: browser surface:N snapshot --interactive
cmux-->>Agent: element refs
end
Last reviewed commit: 85e4724 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # navigate -> verify -> wait -> snapshot -> action -> snapshot | ||
| cmux browser surface:7 get url |
There was a problem hiding this comment.
The comment header lists "navigate" as the first step in the pattern, but the code snippet starts with get url and contains no goto or open command. This could confuse readers about where navigation happens.
Consider either:
- Adding a representative
gotocommand at the top (beforeget url) - Adjusting the comment to clarify navigation is a prerequisite:
| # navigate -> verify -> wait -> snapshot -> action -> snapshot | |
| cmux browser surface:7 get url | |
| # (after goto) verify -> wait -> snapshot -> action -> snapshot | |
| cmux browser surface:7 get url |
| cmux browser "$SURFACE" goto "$DASHBOARD_URL" | ||
| cmux browser "$SURFACE" get url | ||
| cmux browser "$SURFACE" wait --load-state complete --timeout-ms 15000 |
There was a problem hiding this comment.
get url is called after goto but its output is not captured or checked—wait --load-state complete runs unconditionally. The SKILL.md guidance (line 92) states: "If get url is empty or about:blank, navigate first instead of waiting on load state." This template should follow that guidance.
This same issue appears in skills/cmux-browser/templates/form-automation.sh (lines 7–9).
For a defensive template that guards against the silent-failure scenario documented in the PR, capture and check the result:
cmux browser "$SURFACE" goto "$DASHBOARD_URL"
CURRENT_URL=$(cmux browser "$SURFACE" get url)
if [ -z "$CURRENT_URL" ] || [ "$CURRENT_URL" = "about:blank" ]; then
echo "Navigation may have failed (url='$CURRENT_URL'). Aborting." >&2
exit 1
fi
cmux browser "$SURFACE" wait --load-state complete --timeout-ms 15000
cmux browser "$SURFACE" snapshot --interactiveAlternatively, if the intent is purely diagnostic (print the URL for an agent to observe), add a comment explaining why the result is not checked.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests_v2/test_browser_skill_navigation_docs.py (1)
36-39: Consider loosening exact sentence matching to reduce false test failures.These assertions are very strict on wording; minor doc phrasing edits could fail the test without changing behavior. Consider matching stable key fragments instead of full sentences.
Optional resilience tweak
- _must("Verify navigation with `get url` before waiting or snapshotting." in skill, "Expected core workflow to require get url verification") - _must("If `get url` is empty or `about:blank`, navigate first instead of waiting on load state." in skill, "Expected blank-page guidance in stable agent loop") + _must( + "Verify navigation with `get url`" in skill and "before waiting or snapshotting" in skill, + "Expected core workflow to require get url verification", + ) + _must( + "If `get url` is empty" in skill and "`about:blank`" in skill and "navigate first" in skill, + "Expected blank-page guidance in stable agent loop", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_browser_skill_navigation_docs.py` around lines 36 - 39, The assertions in tests_v2/test_browser_skill_navigation_docs.py use exact full-sentence matches via _must which is brittle; update the four _must calls to check for smaller, stable key fragments instead (e.g., replace "Verify navigation with `get url` before waiting or snapshotting." with a fragment like "Verify navigation" or "`get url` before waiting", replace the about:blank check with "about:blank" or "navigate first" fragment, replace "cmux --json browser open https://example.com" with "cmux --json browser open" and change the negative check for "cmux browser open https://example.com --json" to either assert absence of the fragment "--json" appearing after "cmux browser open" or assert that "cmux browser open" and "--json" do not appear together in that order; modify the _must calls (the calls to _must in this diff) to use those shorter substrings or minimal regex checks so the tests are resilient to minor phrasing changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests_v2/test_browser_skill_navigation_docs.py`:
- Around line 36-39: The assertions in
tests_v2/test_browser_skill_navigation_docs.py use exact full-sentence matches
via _must which is brittle; update the four _must calls to check for smaller,
stable key fragments instead (e.g., replace "Verify navigation with `get url`
before waiting or snapshotting." with a fragment like "Verify navigation" or
"`get url` before waiting", replace the about:blank check with "about:blank" or
"navigate first" fragment, replace "cmux --json browser open
https://example.com" with "cmux --json browser open" and change the negative
check for "cmux browser open https://example.com --json" to either assert
absence of the fragment "--json" appearing after "cmux browser open" or assert
that "cmux browser open" and "--json" do not appear together in that order;
modify the _must calls (the calls to _must in this diff) to use those shorter
substrings or minimal regex checks so the tests are resilient to minor phrasing
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25386423-13fe-4db4-aabe-cd76c859050d
📒 Files selected for processing (1)
tests_v2/test_browser_skill_navigation_docs.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92cef9d250
ℹ️ 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".
| _must("Verify navigation with `get url` before waiting or snapshotting." in skill, "Expected core workflow to require get url verification") | ||
| _must("If `get url` is empty or `about:blank`, navigate first instead of waiting on load state." in skill, "Expected blank-page guidance in stable agent loop") | ||
| _must("cmux --json browser open https://example.com" in skill, "Expected safe browser open example with --json before browser") | ||
| _must("cmux browser open https://example.com --json" not in skill, "Unexpected trailing --json browser open example") |
There was a problem hiding this comment.
This check only excludes one hard-coded string, so the test still passes while skills/cmux-browser/SKILL.md contains cmux browser open https://example.com --workspace workspace:2 --window window:1 --json (line 38). In CLI/cmux.swift, --json is parsed only as a global option before the command (cmux [global-options] <command>), and browser open joins remaining tokens into the URL, so trailing --json is treated as URL text instead of enabling JSON output. Please validate trailing---json forms generically so this regression is actually caught.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e47b884a49
ℹ️ 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".
| cmux browser "$SURFACE" get url | ||
| cmux browser "$SURFACE" wait --load-state complete --timeout-ms 15000 |
There was a problem hiding this comment.
This template adds get url but does not use its result before wait --load-state complete; in Sources/TerminalController.swift (v2BrowserWait), load-state waiting is only document.readyState equality, which is already complete on about:blank. If goto fails or has not committed yet, the script still succeeds and snapshots a blank page, so the new verification step does not actually prevent the silent no-op path.
Useful? React with 👍 / 👎.
* Document browser navigation verification loop * Add docs regression test for browser navigation guidance * Remove invalid docs text inspection test
This branch was successfully deployed
1 participant
Summary
get urlbefore waiting or snapshottinggoto -> get url -> wait -> snapshotpattern so blank/new-tab states are easier to diagnoseTesting
git diff --check(pass)Issues
wait --load-state completesilently failed when the page had not actually navigated)Summary by cubic
Verify navigation with get url before waiting or snapshotting in the browser skill. Prevents silent waits on about:blank and makes blank/new-tab states easier to diagnose.
Refactors
Bug Fixes
Written for commit e47b884. Summary will update on new commits.
Summary by CodeRabbit
Documentation
Tests