Conversation
…workflow
Console — Plan Annotation UI (Spec tab):
- Block-based markdown rendering with native Selection API in Specifications tab
- Select any text in annotate mode → write free-text note → auto-saves to JSON immediately
- Single annotation type (free-text only — no DELETION/INSERTION/REPLACEMENT sub-types)
- Removed type toolbar and 'Add general comment' button
- View/Review toggle next to Specifications title; smart default (annotate for unapproved PENDING, view otherwise)
- AnnotationPanel, AnnotationToolbar, BlockRenderer, PlanAnnotator, useAnnotation, parser components
Console — Code Review Mode (Changes tab):
- View/Review toggle moved to Changes page header (previously buried in DiffPanel)
- Inline diff annotations: click '+' on any hunk line, write note, auto-saves to unified JSON
- Staged/unstaged counts in file list sidebar header
- CodeReviewPanel, CodeReviewWidget, useCodeReview components extracted
Console — Unified Annotation Storage:
- Single JSON per spec at docs/plans/.annotations/{plan-filename}.json
- Both planAnnotations and codeReviewAnnotations in one file
- Auto-save on every change (debounced) — no 'Send Feedback' button anywhere
- Removed feedback markdown file generation entirely
- AnnotationRoutes.ts and annotationStore.ts refactored to unified model
- Active spec resolution for Changes tab scopes code review annotations per spec
Console — Sidebar + Layout:
- Sidebar reorder: Dashboard → Changes → Specifications → Extensions → ...
- Changes and Specifications now adjacent for the plan→review→verify flow
Spec Commands — E2E Test Scenarios:
- spec-plan Step 1.5.2: write structured TS-NNN scenarios during planning for UI features
(step table, priority, preconditions, mapped tasks — agent-browser executable)
- Task structure gains 'Mapped Scenarios' field; plan template gains '## E2E Test Scenarios' section
- spec-verify Step 3.9 refactored: executes plan's TS-NNN scenarios with TaskCreate tracking,
fix attempts (max 2 → known issue escalation), results written to '## E2E Results' in plan file
- spec-bugfix-plan: Verification Scenario for UI-facing bugs (single TS-001 acceptance scenario)
- spec-bugfix-verify Step 3.5b: executes verification scenario, loops back on 2-attempt failure
- Console Spec view: E2E Test Scenarios, E2E Results, Verification Scenario added to DISPLAYED_SECTIONS with icons
Spec Commands — Annotation JSON Integration:
- spec-plan Step 1.7b and spec-bugfix-plan Step 1.4b read planAnnotations from unified JSON
- spec-verify Step 3.12b and spec-bugfix-verify Step 3.7b read codeReviewAnnotations from unified JSON
- All annotation feedback steps updated to DELETE via new /api/annotations endpoints
Launcher — Worktree Sync Auto-Stash:
- sync_worktree() now auto-stashes uncommitted changes on base branch before merging
- Stash is preserved (never dropped) if restore encounters conflicts
- spec-verify and spec-bugfix-verify: removed pre-sync working tree cleanliness check
- Fixes GitHub issue #119 ('Main branch has an unstashed file')
Rules + Commands — playwright-cli → agent-browser:
- pilot/rules/playwright-cli.md removed; pilot/rules/agent-browser.md added
- All spec commands, rules, and settings updated to reference agent-browser
Installer:
- finalize.py: kill stale Console worker (port 41777) on install so new files take effect immediately
- dependencies.py: agent-browser replaces playwright-cli in dependency checks
- shell_config.py: encoding improvements
Other:
- LICENSE: Enterprise Source Code License clause for 100+ seat organizations
- settings.json: autoDreamEnabled; removed CLAUDE_CODE_MAX_OUTPUT_TOKENS/EFFORT_LEVEL overrides; agent-browser tip
- pilot/ui/: viewer bundle rebuilt with annotation components (PlanAnnotator.js)
- README, Docusaurus, site: updated spec workflow descriptions, E2E scenarios, console features
- docs/docusaurus: remote-control.md removed, console.md expanded with annotation features
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Claude finished @maxritter's task in 2m 14s —— View job Code Review — PR #121
OverviewLarge but well-scoped PR. The annotation system, E2E scenario structure, and Issues1.
|
WalkthroughReplaces Playwright CLI with agent-browser, adds Headless Mode and Console annotation/code-review workflows, introduces an Enterprise source-code license and pricing tier, updates installer/uninstall flows and tests, and adjusts site/docs content and git ignore/attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Console)
participant Console as Console UI
participant Agent as Agent
participant Browser as agent-browser
participant Plan as Plan storage (repo/file)
User->>Console: Open spec (PENDING) and annotate
Console->>Plan: Save annotations (auto-save)
User->>Console: Submit for approval
Console->>Agent: Send plan + annotations at approval checkpoint
Agent->>Plan: Read plan and annotations
Agent->>Browser: Execute E2E scenarios from plan
Browser-->>Agent: Scenario results (pass/fail)
Agent->>Plan: Write verification results (pass/fail, attempts)
Agent->>Console: Request Code Review (opens Changes)
User->>Console: Add inline diff annotations
Console->>Plan: Persist diff annotations
Console->>Agent: Notify agent, agent reads annotations, marks spec verified
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @maxritter's task in 36s —— View job Incremental Code Review — PR #121The new push changed only Still open from prior review
No previously reported issues were resolved by this push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installer/tests/unit/steps/test_dependencies.py (1)
36-66:⚠️ Potential issue | 🟡 MinorAdd an explicit assertion for the new
install_agent_browserpath.The patch target was migrated, but this test still doesn’t verify that the agent-browser installer is actually invoked during
DependenciesStep.run. Also rename_mock_playwrightto avoid stale naming.Suggested test tightening
- _mock_playwright, + mock_agent_browser, _mock_probe, _mock_rtk, @@ mock_python_tools.assert_called_once() mock_plugin_deps.assert_called_once() + mock_agent_browser.assert_called_once()As per coding guidelines
**/tests/**: Review test code briefly. Focus on: Test coverage for the feature being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/tests/unit/steps/test_dependencies.py` around lines 36 - 66, The test test_dependencies_run_installs_core does not assert that install_agent_browser is called and still uses a stale parameter name (_mock_playwright); update the test to assert that install_agent_browser was invoked when DependenciesStep.run executes and rename the corresponding patched parameter from _mock_playwright to a meaningful name (e.g., mock_install_agent_browser) to match the patch target; ensure the assertion uses the mock returned by the `@patch` of installer.steps.dependencies.install_agent_browser and keep existing patches for other installers intact.
🧹 Nitpick comments (1)
installer/steps/finalize.py (1)
53-53: Consider centralizing the port constant.Port
41777is hardcoded here and in other locations (e.g.,pilot/hooks/_lib/dashboard_notify.pydefinesCONSOLE_URL = "http://localhost:41777"). Extracting this to a shared constant would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/steps/finalize.py` at line 53, Multiple locations hardcode port 41777 (seen in the lsof call in finalize.py and CONSOLE_URL in pilot/hooks/_lib/dashboard_notify.py); introduce a single shared constant (e.g., CONSOLE_PORT = 41777) in a common module and import it where needed, then update the lsof invocation in finalize.py to use f":{CONSOLE_PORT}" and rebuild CONSOLE_URL in dashboard_notify.py from that port (e.g., CONSOLE_URL = f"http://localhost:{CONSOLE_PORT}") so all modules reference the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docusaurus/docs/features/console.md`:
- Around line 33-49: The text is inconsistent about whether annotations autosave
or require an explicit Save; update the Plan Annotation and Code Review sections
so the UX wording matches actual behavior by either removing "save automatically
as you write them" and clarifying that users must press the Save button, or by
removing the "press Save" mentions and documenting that annotations autosave;
specifically edit the sentences referencing "save automatically as you write
them" (Annotate mode / Plan Annotation), the phrase "press Save" in the Plan
Annotation paragraph, and the "press Save" in the Code Review paragraph (inline
annotation form under Review mode), and ensure the toggles "Annotate mode" and
"Review mode" and the inline "+" button description reflect the chosen behavior.
In `@docs/docusaurus/docs/workflows/spec.md`:
- Around line 70-72: The doc introduces a human "review" checkpoint (Review mode
/ Changes tab) but earlier text still claims "Approve" is the only manual step
and that "/spec" can run end-to-end when the three toggles are off; reconcile by
either making review optional or updating the contract to require it: if
mandatory, update earlier references of the manual step (the "Approve" step) and
the "/spec" description to state that a human review in Review mode is required
before marking a spec verified; if optional, clearly state here that the review
checkpoint is optional and document the new toggle (add a toggle name and
behavior) alongside the existing three toggles so the condition for fully
automated /spec runs is accurate (also update any mentions of "three toggles" to
include the new toggle or clarify its absence).
In `@docs/site/src/components/FAQSection.tsx`:
- Line 24: The sentence in FAQSection.tsx that currently asserts "No copyleft or
restrictive-licensed dependencies are introduced into your environment" is too
absolute and conflicts with other docs (e.g., Hypothesis under MPL-2.0). Update
the string in the FAQSection component to remove the absolute claim and instead
state that most bundled tools are under permissive licenses, while noting that
some included tools may use other open-source licenses (e.g., MPL-2.0) and to
refer to the open-source tools page for full license details; keep the change
localized to the quoted FAQ text.
In `@installer/steps/dependencies.py`:
- Around line 324-357: The current install_agent_browser returns immediately
after running "agent-browser upgrade" when _is_agent_browser_ready() is true,
skipping the necessary "agent-browser install" step; change
install_agent_browser so that if the tool is present you still run the upgrade
but then always run "agent-browser install" (use platform.system() to decide
whether to append "--with-deps" on Linux) and return the result of that final
_run_bash_with_retry call; keep using npm_global_cmd("npm install -g
agent-browser") and _run_bash_with_retry for the initial install path but ensure
the install step is executed unconditionally after upgrade/installation to
guarantee runtime readiness.
In `@installer/steps/finalize.py`:
- Around line 48-67: The _kill_stale_worker function uses Unix-only lsof/kill;
update it to be cross-platform by adding a platform check (use sys.platform or
os.name) and implement Windows logic (e.g., use psutil to find processes
listening on port 41777 or fallback to subprocess for Unix only). Mirror the
graceful termination pattern used in dependencies.py::_kill_proc(): attempt a
graceful terminate/terminate-equivalent first with a short timeout, then
escalate to forceful kill if still alive. Ensure you reference
_kill_stale_worker when changing the port-scanning and termination logic and
keep the Unix lsof/kill branch only under the appropriate platform guard.
In `@installer/tests/unit/steps/test_dependencies_playwright.py`:
- Around line 98-106: The test should lock down the short-circuit by making the
patched _run_bash_with_retry explicitly fail on the first call and then
asserting no subsequent install call was attempted: set mock_run.side_effect to
fail on the first invocation (e.g., [False]) and then assert mock_run was called
exactly once and that that single call was the npm install command passed into
_run_bash_with_retry; keep the existing patches for _is_agent_browser_ready and
_run_bash_with_retry and update test_returns_false_when_npm_fails to verify only
the first (npm) command was executed and no second install step was invoked.
---
Outside diff comments:
In `@installer/tests/unit/steps/test_dependencies.py`:
- Around line 36-66: The test test_dependencies_run_installs_core does not
assert that install_agent_browser is called and still uses a stale parameter
name (_mock_playwright); update the test to assert that install_agent_browser
was invoked when DependenciesStep.run executes and rename the corresponding
patched parameter from _mock_playwright to a meaningful name (e.g.,
mock_install_agent_browser) to match the patch target; ensure the assertion uses
the mock returned by the `@patch` of
installer.steps.dependencies.install_agent_browser and keep existing patches for
other installers intact.
---
Nitpick comments:
In `@installer/steps/finalize.py`:
- Line 53: Multiple locations hardcode port 41777 (seen in the lsof call in
finalize.py and CONSOLE_URL in pilot/hooks/_lib/dashboard_notify.py); introduce
a single shared constant (e.g., CONSOLE_PORT = 41777) in a common module and
import it where needed, then update the lsof invocation in finalize.py to use
f":{CONSOLE_PORT}" and rebuild CONSOLE_URL in dashboard_notify.py from that port
(e.g., CONSOLE_URL = f"http://localhost:{CONSOLE_PORT}") so all modules
reference the same source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c13199a6-31a9-4701-a0a4-d84be1f96237
⛔ Files ignored due to path filters (65)
console/package.jsonis excluded by!console/**console/src/cli/handlers/user-message.tsis excluded by!console/**console/src/services/worker-service.tsis excluded by!console/**console/src/services/worker/http/routes/AnnotationRoutes.tsis excluded by!console/**console/src/services/worker/http/routes/LicenseRoutes.tsis excluded by!console/**console/src/services/worker/http/routes/PlanRoutes.tsis excluded by!console/**console/src/services/worker/http/routes/ViewerRoutes.tsis excluded by!console/**console/src/services/worker/http/routes/utils/annotationStore.tsis excluded by!console/**console/src/services/worker/http/routes/utils/planFileReader.tsis excluded by!console/**console/src/ui/viewer/components/LicenseBadge.tsxis excluded by!console/**console/src/ui/viewer/hooks/useLicense.tsis excluded by!console/**console/src/ui/viewer/layouts/Sidebar/SidebarNav.tsxis excluded by!console/**console/src/ui/viewer/views/Changes/DiffPanel.tsxis excluded by!console/**console/src/ui/viewer/views/Changes/index.tsxis excluded by!console/**console/src/ui/viewer/views/Changes/review/CodeReviewPanel.tsxis excluded by!console/**console/src/ui/viewer/views/Changes/review/CodeReviewWidget.tsxis excluded by!console/**console/src/ui/viewer/views/Changes/review/types.tsis excluded by!console/**console/src/ui/viewer/views/Changes/review/useCodeReview.tsis excluded by!console/**console/src/ui/viewer/views/Spec/SpecHeaderCard.tsxis excluded by!console/**console/src/ui/viewer/views/Spec/SpecSection.tsxis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/AnnotationPanel.tsxis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/AnnotationToolbar.tsxis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/BlockRenderer.tsxis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/PlanAnnotator.tsxis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/index.tsis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/parser.tsis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/types.tsis excluded by!console/**console/src/ui/viewer/views/Spec/annotation/useAnnotation.tsis excluded by!console/**console/src/ui/viewer/views/Spec/index.tsxis excluded by!console/**console/tests/annotation/code-review.test.tsis excluded by!console/**console/tests/annotation/parser.test.tsis excluded by!console/**console/tests/annotation/use-annotation.test.tsis excluded by!console/**console/tests/server/license-routes.test.tsis excluded by!console/**console/tests/ui/ChangesNavigation.test.tsis excluded by!console/**console/tests/ui/license-badge.test.tsis excluded by!console/**console/tests/worker/annotation-routes.test.tsis excluded by!console/**console/tests/worker/license-routes.test.tsis excluded by!console/**console/tests/worker/utils/isValidPlanPath.test.tsis excluded by!console/**launcher/cli.pyis excluded by!launcher/**launcher/tests/unit/test_cli.pyis excluded by!launcher/**launcher/tests/unit/test_worktree.pyis excluded by!launcher/**launcher/tests/unit/test_wrapper.pyis excluded by!launcher/**launcher/worktree.pyis excluded by!launcher/**launcher/wrapper.pyis excluded by!launcher/**pilot/commands/create-skill.mdis excluded by!pilot/**pilot/commands/spec-bugfix-plan.mdis excluded by!pilot/**pilot/commands/spec-bugfix-verify.mdis excluded by!pilot/**pilot/commands/spec-implement.mdis excluded by!pilot/**pilot/commands/spec-plan.mdis excluded by!pilot/**pilot/commands/spec-verify.mdis excluded by!pilot/**pilot/commands/spec.mdis excluded by!pilot/**pilot/rules/agent-browser.mdis excluded by!pilot/**pilot/rules/development-practices.mdis excluded by!pilot/**pilot/rules/playwright-cli.mdis excluded by!pilot/**pilot/rules/standards-backend.mdis excluded by!pilot/**pilot/rules/standards-frontend.mdis excluded by!pilot/**pilot/rules/task-and-workflow.mdis excluded by!pilot/**pilot/rules/testing.mdis excluded by!pilot/**pilot/rules/verification.mdis excluded by!pilot/**pilot/scripts/mcp-server.cjsis excluded by!pilot/**pilot/scripts/worker-service.cjsis excluded by!pilot/**pilot/settings.jsonis excluded by!pilot/**pilot/ui/PlanAnnotator.jsis excluded by!pilot/**pilot/ui/viewer-bundle.jsis excluded by!pilot/**pilot/ui/viewer.cssis excluded by!pilot/**
📒 Files selected for processing (23)
.gitignoreLICENSEREADME.mddocs/docusaurus/.gitignoredocs/docusaurus/docs/features/cli.mddocs/docusaurus/docs/features/console.mddocs/docusaurus/docs/features/extensions.mddocs/docusaurus/docs/features/open-source-tools.mddocs/docusaurus/docs/features/remote-control.mddocs/docusaurus/docs/features/rules.mddocs/docusaurus/docs/getting-started/installation.mddocs/docusaurus/docs/workflows/spec.mddocs/site/src/components/ConsoleSection.tsxdocs/site/src/components/FAQSection.tsxdocs/site/src/components/PricingSection.tsxdocs/site/src/components/WhatsInside.tsxinstaller/steps/dependencies.pyinstaller/steps/finalize.pyinstaller/steps/shell_config.pyinstaller/tests/unit/steps/test_dependencies.pyinstaller/tests/unit/steps/test_dependencies_playwright.pyinstaller/tests/unit/steps/test_shell_config.pyuninstall.sh
💤 Files with no reviewable changes (1)
- docs/docusaurus/docs/features/remote-control.md
| def _is_agent_browser_ready() -> bool: | ||
| """Check if agent-browser is installed and Chrome is available.""" | ||
| if not command_exists("agent-browser"): | ||
| return False | ||
|
|
||
| for cache_dir in _get_playwright_cache_dirs(): | ||
| if not cache_dir.exists(): | ||
| continue | ||
|
|
||
| for chromium_dir in cache_dir.glob("chromium-*"): | ||
| if (chromium_dir / "INSTALLATION_COMPLETE").exists(): | ||
| return True | ||
|
|
||
| for chromium_dir in cache_dir.glob("chromium_headless_shell-*"): | ||
| if (chromium_dir / "INSTALLATION_COMPLETE").exists(): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def _install_playwright_system_deps() -> bool: | ||
| """Install OS-level system dependencies required by Playwright browsers. | ||
|
|
||
| Runs 'npx playwright install-deps' which installs system libraries | ||
| (libglib, libatk, etc.) needed by Chromium. Required on Linux/devcontainers, | ||
| no-op on macOS. | ||
| """ | ||
| cmd = ["npx", "-y", "playwright", "install-deps"] | ||
| for attempt in range(MAX_RETRIES): | ||
| try: | ||
| result = subprocess.run(cmd, capture_output=True, text=True, timeout=300) | ||
| if result.returncode == 0: | ||
| return True | ||
| except Exception: | ||
| pass | ||
| if attempt < MAX_RETRIES - 1: | ||
| time.sleep(RETRY_DELAY) | ||
| return False | ||
| try: | ||
| result = subprocess.run( | ||
| ["agent-browser", "--version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| return result.returncode == 0 | ||
| except Exception: | ||
| return False | ||
|
|
||
|
|
||
| def install_playwright_cli() -> bool: | ||
| """Install playwright-cli for headless browser automation. | ||
| def install_agent_browser() -> bool: | ||
| """Install agent-browser for headless browser automation. | ||
|
|
||
| On Linux ARM64, installs chromium specifically (Chrome has no ARM64 builds). | ||
| On Linux, installs with --with-deps for system dependencies. | ||
| """ | ||
| if _is_playwright_cli_ready(): | ||
| return True | ||
|
|
||
| if not _run_bash_with_retry(npm_global_cmd("npm install -g @playwright/cli@latest")): | ||
| return False | ||
| import platform | ||
|
|
||
| if _is_playwright_cli_ready(): | ||
| _install_playwright_system_deps() | ||
| if _is_agent_browser_ready(): | ||
| # Upgrade existing installation | ||
| _run_bash_with_retry("agent-browser upgrade", timeout=120) | ||
| return True | ||
|
|
||
| install_cmd = ["playwright-cli", "install", "chromium"] if is_linux_arm64() else ["playwright-cli", "install"] | ||
|
|
||
| for attempt in range(MAX_RETRIES): | ||
| try: | ||
| result = subprocess.run(install_cmd, capture_output=True, text=True, timeout=300) | ||
| if result.returncode == 0: | ||
| break | ||
| except Exception: | ||
| pass | ||
| if attempt < MAX_RETRIES - 1: | ||
| time.sleep(RETRY_DELAY) | ||
| continue | ||
| if not _run_bash_with_retry(npm_global_cmd("npm install -g agent-browser")): | ||
| return False | ||
|
|
||
| _install_playwright_system_deps() | ||
| return True | ||
| install_cmd = "agent-browser install --with-deps" if platform.system() == "Linux" else "agent-browser install" | ||
| return _run_bash_with_retry(install_cmd, timeout=300) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the official agent-browser CLI docs, does agent-browser upgradeguarantee browser binaries/system dependencies are installed, or isagent-browser install still required to ensure runtime readiness?
💡 Result:
No, agent-browser upgrade does not guarantee that browser binaries/system dependencies are installed. agent-browser install is still required to ensure runtime readiness.
Citations:
- 1: https://agent-browser.dev/installation
- 2: https://github.com/vercel-labs/agent-browser
- 3: https://github.com/vercel-labs/agent-browser/blob/main/README.md
- 4: https://agent-browser.dev/commands
- 5: https://registry.npmjs.org/agent-browser
- 6: https://github.com/vercel-labs/agent-browser/
Ensure agent-browser install runs unconditionally to guarantee runtime readiness.
The current implementation returns success immediately after agent-browser upgrade if the tool is already installed, skipping the agent-browser install step. However, agent-browser upgrade does not guarantee that browser binaries and system dependencies are installed—agent-browser install is still required for runtime readiness. This violates the idempotency requirement (installer must be safely re-runnable).
Refactor to always call agent-browser install after upgrade, even when the tool already exists:
Fix
def install_agent_browser() -> bool:
if _is_agent_browser_ready():
- # Upgrade existing installation
+ # Best-effort upgrade; continue to install to guarantee runtime assets
_run_bash_with_retry("agent-browser upgrade", timeout=120)
- return True
-
- if not _run_bash_with_retry(npm_global_cmd("npm install -g agent-browser")):
- return False
+ else:
+ if not _run_bash_with_retry(npm_global_cmd("npm install -g agent-browser")):
+ return False
install_cmd = "agent-browser install --with-deps" if platform.system() == "Linux" else "agent-browser install"
return _run_bash_with_retry(install_cmd, timeout=300)🧰 Tools
🪛 Ruff (0.15.7)
[error] 331-331: Starting a process with a partial executable path
(S607)
[warning] 337-337: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/steps/dependencies.py` around lines 324 - 357, The current
install_agent_browser returns immediately after running "agent-browser upgrade"
when _is_agent_browser_ready() is true, skipping the necessary "agent-browser
install" step; change install_agent_browser so that if the tool is present you
still run the upgrade but then always run "agent-browser install" (use
platform.system() to decide whether to append "--with-deps" on Linux) and return
the result of that final _run_bash_with_retry call; keep using
npm_global_cmd("npm install -g agent-browser") and _run_bash_with_retry for the
initial install path but ensure the install step is executed unconditionally
after upgrade/installation to guarantee runtime readiness.
| @staticmethod | ||
| def _kill_stale_worker() -> None: | ||
| """Kill any running Console worker so it restarts with the newly installed files.""" | ||
| try: | ||
| result = subprocess.run( | ||
| ["lsof", "-ti", ":41777"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=5, | ||
| ) | ||
| pids = result.stdout.strip() | ||
| if pids: | ||
| for pid in pids.splitlines(): | ||
| subprocess.run( | ||
| ["kill", "-9", pid.strip()], | ||
| capture_output=True, | ||
| timeout=5, | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Cross-platform compatibility: lsof and kill are Unix-only.
This implementation won't work on Windows since lsof and kill are Unix utilities. Consider adding a platform check or using a cross-platform approach.
🛠️ Suggested platform guard
+import platform
+
`@staticmethod`
def _kill_stale_worker() -> None:
"""Kill any running Console worker so it restarts with the newly installed files."""
+ if platform.system() == "Windows":
+ return # lsof/kill not available on Windows
try:
result = subprocess.run(Additionally, consider using graceful termination (SIGTERM) before SIGKILL, similar to _kill_proc() in dependencies.py which uses terminate() with a timeout before escalating to kill(). This gives the worker a chance to clean up.
As per coding guidelines: "Review installer code for: Cross-platform compatibility".
📝 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.
| @staticmethod | |
| def _kill_stale_worker() -> None: | |
| """Kill any running Console worker so it restarts with the newly installed files.""" | |
| try: | |
| result = subprocess.run( | |
| ["lsof", "-ti", ":41777"], | |
| capture_output=True, | |
| text=True, | |
| timeout=5, | |
| ) | |
| pids = result.stdout.strip() | |
| if pids: | |
| for pid in pids.splitlines(): | |
| subprocess.run( | |
| ["kill", "-9", pid.strip()], | |
| capture_output=True, | |
| timeout=5, | |
| ) | |
| except Exception: | |
| pass | |
| import platform | |
| `@staticmethod` | |
| def _kill_stale_worker() -> None: | |
| """Kill any running Console worker so it restarts with the newly installed files.""" | |
| if platform.system() == "Windows": | |
| return # lsof/kill not available on Windows | |
| try: | |
| result = subprocess.run( | |
| ["lsof", "-ti", ":41777"], | |
| capture_output=True, | |
| text=True, | |
| timeout=5, | |
| ) | |
| pids = result.stdout.strip() | |
| if pids: | |
| for pid in pids.splitlines(): | |
| subprocess.run( | |
| ["kill", "-9", pid.strip()], | |
| capture_output=True, | |
| timeout=5, | |
| ) | |
| except Exception: | |
| pass |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 53-53: Starting a process with a partial executable path
(S607)
[error] 61-61: subprocess call: check for execution of untrusted input
(S603)
[error] 62-62: Starting a process with a partial executable path
(S607)
[error] 66-67: try-except-pass detected, consider logging the exception
(S110)
[warning] 66-66: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/steps/finalize.py` around lines 48 - 67, The _kill_stale_worker
function uses Unix-only lsof/kill; update it to be cross-platform by adding a
platform check (use sys.platform or os.name) and implement Windows logic (e.g.,
use psutil to find processes listening on port 41777 or fallback to subprocess
for Unix only). Mirror the graceful termination pattern used in
dependencies.py::_kill_proc(): attempt a graceful terminate/terminate-equivalent
first with a short timeout, then escalate to forceful kill if still alive.
Ensure you reference _kill_stale_worker when changing the port-scanning and
termination logic and keep the Unix lsof/kill branch only under the appropriate
platform guard.
| @patch("installer.steps.dependencies._is_agent_browser_ready") | ||
| @patch("installer.steps.dependencies._run_bash_with_retry") | ||
| def test_returns_false_when_browser_install_fails( | ||
| self, mock_run, mock_ready, mock_subprocess, mock_deps, _mock_sleep | ||
| ): | ||
| """Returns False when playwright-cli install fails.""" | ||
| from installer.steps.dependencies import install_playwright_cli | ||
| def test_returns_false_when_npm_fails(self, mock_run, mock_ready): | ||
| """Returns False when npm install fails.""" | ||
| from installer.steps.dependencies import install_agent_browser | ||
|
|
||
| mock_ready.side_effect = [False, False] | ||
| mock_run.return_value = True | ||
| mock_result = MagicMock() | ||
| mock_result.returncode = 1 | ||
| mock_subprocess.run.return_value = mock_result | ||
| assert install_playwright_cli() is False | ||
| mock_deps.assert_not_called() | ||
| mock_ready.return_value = False | ||
| mock_run.return_value = False | ||
| assert install_agent_browser() is False |
There was a problem hiding this comment.
Lock down the npm-failure short-circuit.
With mock_run.return_value = False, this still passes even if install_agent_browser() incorrectly attempts the second install step after the npm failure. Patch the platform/npm helpers here and assert that only the first command ran.
As per coding guidelines, **/tests/** should focus on test coverage for the feature being tested and proper mocking of external dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/tests/unit/steps/test_dependencies_playwright.py` around lines 98 -
106, The test should lock down the short-circuit by making the patched
_run_bash_with_retry explicitly fail on the first call and then asserting no
subsequent install call was attempted: set mock_run.side_effect to fail on the
first invocation (e.g., [False]) and then assert mock_run was called exactly
once and that that single call was the npm install command passed into
_run_bash_with_retry; keep the existing patches for _is_agent_browser_ready and
_run_bash_with_retry and update test_returns_false_when_npm_fails to verify only
the first (npm) command was executed and no second install step was invoked.
|
Claude finished @maxritter's task in 2m 5s —— View job Incremental Code Review — PR #121
Resolved by this push✅ ✅ Tests for ✅ ✅ ✅ New issue introduced in this push
|
|
Claude finished @maxritter's task in 57s —— View job Incremental Code Review — PR #121
No new issues in this push
Still open from prior reviews
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
installer/steps/dependencies.py (1)
341-356:⚠️ Potential issue | 🟠 Major
agent-browser installis skipped when tool already exists, breaking idempotency.The function returns immediately after
agent-browser upgrade(line 349-350) without runningagent-browser install, which is required to ensure browser binaries and system dependencies are present. Per coding guidelines, installer code must be safely re-runnable.Proposed fix
def install_agent_browser() -> bool: """Install agent-browser for headless browser automation. On Linux, installs with --with-deps for system dependencies. """ import platform if _is_agent_browser_ready(): - _run_bash_with_retry("agent-browser upgrade", timeout=120) # best-effort; failure is non-fatal — existing version still works - return True + _run_bash_with_retry("agent-browser upgrade", timeout=120) # best-effort upgrade + else: + if not _run_bash_with_retry(npm_global_cmd("npm install -g agent-browser")): + return False - if not _run_bash_with_retry(npm_global_cmd("npm install -g agent-browser")): - return False - install_cmd = "agent-browser install --with-deps" if platform.system() == "Linux" else "agent-browser install" return _run_bash_with_retry(install_cmd, timeout=300)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/steps/dependencies.py` around lines 341 - 356, The current install_agent_browser function returns immediately after calling _run_bash_with_retry("agent-browser upgrade") when _is_agent_browser_ready() is true, which skips the required "agent-browser install" step; change the control flow so that when _is_agent_browser_ready() is true you still run the install step (to ensure browser binaries/deps) instead of returning early — e.g., call _run_bash_with_retry("agent-browser upgrade") for best-effort upgrade, then compute install_cmd (using platform.system()) and call _run_bash_with_retry(install_cmd, timeout=300); keep the existing npm_global_cmd("npm install -g agent-browser") path for the first-time install and preserve timeouts and return boolean results from _run_bash_with_retry.
🧹 Nitpick comments (2)
docs/site/src/components/WhatsInside.tsx (1)
123-124: Duplicatehover:border-primary/50class.The class
hover:border-primary/50appears twice on line 124. This is harmless but redundant.🧹 Proposed fix
- className={`group relative rounded-lg p-5 border border-border/50 bg-card - hover:border-primary/50 hover:bg-card hover:border-primary/50 + className={`group relative rounded-lg p-5 border border-border/50 bg-card + hover:border-primary/50 hover:bg-card🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/site/src/components/WhatsInside.tsx` around lines 123 - 124, Duplicate Tailwind class in the JSX className prop inside the WhatsInside component: remove the redundant "hover:border-primary/50" entry so the template string for the element's className only contains a single instance of that class; edit the className in the JSX (the template string shown) to delete the duplicate token.installer/tests/unit/steps/test_finalize.py (1)
90-108: Good test coverage; consider verifying the lsof call arguments.The tests cover the main scenarios well (multiple PIDs, empty result, lsof missing). One minor gap:
test_kills_pids_found_by_lsofverifies thekillcalls but doesn't confirm thelsofcall used the correct arguments["lsof", "-ti", ":41777"].💡 Optional: Add lsof call verification
assert mock_run.call_count == 3 + mock_run.assert_any_call( + ["lsof", "-ti", ":41777"], + capture_output=True, + text=True, + timeout=5, + ) mock_run.assert_any_call(["kill", "-9", "1234"], capture_output=True, timeout=5) mock_run.assert_any_call(["kill", "-9", "5678"], capture_output=True, timeout=5)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/tests/unit/steps/test_finalize.py` around lines 90 - 108, The test TestKillStaleWorker.test_kills_pids_found_by_lsof currently asserts kill calls but doesn't verify the initial lsof invocation; update the test to also assert that subprocess.run was called for the lsof command with the exact args ["lsof", "-ti", ":41777"] (i.e. the first call to subprocess.run when invoking FinalizeStep._kill_stale_worker) and preserve existing assertions for the kill calls, referencing FinalizeStep._kill_stale_worker and the patched subprocess.run mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@installer/steps/dependencies.py`:
- Around line 341-356: The current install_agent_browser function returns
immediately after calling _run_bash_with_retry("agent-browser upgrade") when
_is_agent_browser_ready() is true, which skips the required "agent-browser
install" step; change the control flow so that when _is_agent_browser_ready() is
true you still run the install step (to ensure browser binaries/deps) instead of
returning early — e.g., call _run_bash_with_retry("agent-browser upgrade") for
best-effort upgrade, then compute install_cmd (using platform.system()) and call
_run_bash_with_retry(install_cmd, timeout=300); keep the existing
npm_global_cmd("npm install -g agent-browser") path for the first-time install
and preserve timeouts and return boolean results from _run_bash_with_retry.
---
Nitpick comments:
In `@docs/site/src/components/WhatsInside.tsx`:
- Around line 123-124: Duplicate Tailwind class in the JSX className prop inside
the WhatsInside component: remove the redundant "hover:border-primary/50" entry
so the template string for the element's className only contains a single
instance of that class; edit the className in the JSX (the template string
shown) to delete the duplicate token.
In `@installer/tests/unit/steps/test_finalize.py`:
- Around line 90-108: The test TestKillStaleWorker.test_kills_pids_found_by_lsof
currently asserts kill calls but doesn't verify the initial lsof invocation;
update the test to also assert that subprocess.run was called for the lsof
command with the exact args ["lsof", "-ti", ":41777"] (i.e. the first call to
subprocess.run when invoking FinalizeStep._kill_stale_worker) and preserve
existing assertions for the kill calls, referencing
FinalizeStep._kill_stale_worker and the patched subprocess.run mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 024c0423-239b-4211-bda3-82f3c9717336
⛔ Files ignored due to path filters (4)
console/package.jsonis excluded by!console/**console/src/utils/logger.tsis excluded by!console/**pilot/commands/spec-verify.mdis excluded by!pilot/**pilot/ui/viewer.cssis excluded by!pilot/**
📒 Files selected for processing (10)
.gitattributesdocs/docusaurus/docs/features/console.mddocs/docusaurus/docs/workflows/spec.mddocs/site/src/components/ComparisonSection.tsxdocs/site/src/components/FAQSection.tsxdocs/site/src/components/NavBar.tsxdocs/site/src/components/WhatsInside.tsxdocs/site/src/pages/Index.tsxinstaller/steps/dependencies.pyinstaller/tests/unit/steps/test_finalize.py
💤 Files with no reviewable changes (2)
- docs/site/src/components/NavBar.tsx
- docs/site/src/components/ComparisonSection.tsx
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/docusaurus/docs/features/console.md
- docs/site/src/components/FAQSection.tsx
- docs/docusaurus/docs/workflows/spec.md
|
🎉 This PR is included in version 7.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Single commit on top of
main(7.7.6).Console — Plan Annotation UI (Spec tab)
Console — Code Review Mode (Changes tab)
Console — Unified Annotation Storage
docs/plans/.annotations/{plan}.jsonplanAnnotationsandcodeReviewAnnotationsin one fileConsole — Sidebar
Spec Commands — E2E Test Scenarios
spec-planStep 1.5.2: structured TS-NNN scenarios written during planning for UI featuresspec-verifyStep 3.9: executes TS-NNN scenarios with TaskCreate tracking, fix attempt limits, results written back to planspec-bugfix-plan/verify: lightweight Verification Scenario for UI-facing bugsSpec Commands — Annotation JSON Integration
planAnnotationsfrom unified JSONcodeReviewAnnotationsfrom unified JSONLauncher — Worktree Sync Auto-Stash
sync_worktree()auto-stashes uncommitted changes before mergingRules — playwright-cli → agent-browser
pilot/rules/playwright-cli.mdremoved;pilot/rules/agent-browser.mdaddedOther
settings.json: autoDreamEnabled, agent-browser tipSummary by CodeRabbit
New Features
-p) for CI/CD/automationBug Fixes
Chores