Add long-run runtime stability checks#4
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change implements long-run stability tracking for Browser CLI by adding runtime metrics (command counts, driver switches, extension disconnects, cleanup failures) to Changes
Sequence DiagramsequenceDiagram
participant CLI as Status Command
participant BS as BrowserService
participant RP as RuntimePresentation
participant Out as Output
CLI->>BS: runtime_status()
BS->>BS: Collect stability metrics<br/>(commands_started, driver_switches,<br/>cleanup_failures, last_cleanup_error)
BS-->>CLI: {stability: {...}, ...}
CLI->>RP: build_runtime_presentation(raw_status)
RP->>RP: Extract stability block<br/>Evaluate cleanup_failures
alt cleanup_failures > 0
RP->>RP: Set overall_state = 'degraded'<br/>Set recovery_guidance
end
RP-->>CLI: presentation with stability
CLI->>CLI: render_status_report()
CLI->>CLI: Render Stability section<br/>(commands started, cleanup failures,<br/>last cleanup error)
CLI->>Out: Display status output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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 |
![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)
src/browser_cli/commands/status.py (1)
115-117: Prefer daemon presentation stability snapshot as primary source.Use
presentation["stability"]first (fallback to top-levelstability) so status rendering stays aligned with daemon-normalized shared presentation data.Based on learnings: `runtime-status` includes a daemon-owned `presentation` snapshot; `browser-cli status` should render that shared state.♻️ Proposed refactor
presentation = dict((live_payload or {}).get("presentation") or {}) - stability = dict((live_payload or {}).get("stability") or {}) + stability = dict(presentation.get("stability") or {}) or dict( + (live_payload or {}).get("stability") or {} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser_cli/commands/status.py` around lines 115 - 117, The status rendering currently pulls stability from the top-level live_payload before considering the daemon-normalized presentation snapshot; update the logic so stability is assigned from presentation.get("stability") first (falling back to (live_payload or {}).get("stability") if absent) while keeping presentation = dict((live_payload or {}).get("presentation") or {}) and overall_status computed via presentation.get("overall_state") or _classify_overall_status as before; adjust the variable `stability` (and any downstream uses) to prefer the presentation snapshot to ensure daemon-normalized state is rendered.AGENTS.md (1)
235-237: Consider consolidating redundant validation instructions.Lines 235-237 contain overlapping guidance:
- "After each code change, run lint and guard..."
- "After each code change, run lint, tests, and guard."
- "After each code change, run
scripts/lint.sh,scripts/test.sh..."The first statement (line 235) appears to be stale and contradicts the more complete instructions on lines 236-237.
📝 Suggested consolidation
- `scripts/check.sh` runs lint, tests, and guard in the expected order. - The guard implementations live under `scripts/guards/`. -- After each code change, run lint and guard as part of the full validation flow. -- After each code change, run lint, tests, and guard. - After each code change, run `scripts/lint.sh`, `scripts/test.sh`, and `scripts/guard.sh`, or run `scripts/check.sh`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 235 - 237, Remove the redundant validation sentence and consolidate the three overlapping lines into a single clear instruction: delete the stale line "After each code change, run lint and guard..." and keep a single consolidated line that directs contributors to run `scripts/lint.sh`, `scripts/test.sh`, and `scripts/guard.sh` (or `scripts/check.sh`) after each code change so AGENTS.md contains one authoritative validation instruction.
🤖 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/superpowers/specs/2026-04-13-browser-cli-long-run-stability-design.md`:
- Line 5: The spec contains a developer-specific absolute path string
'/home/hongv/workspace/browser-cli'; remove or replace that exact string in
docs/superpowers/specs/2026-04-13-browser-cli-long-run-stability-design.md with
a generic reference such as "the repository root" or "the checked-out
repository" so tests and workflow fixtures resolve assets relative to the repo
rather than a machine-specific path.
In `@src/browser_cli/daemon/runtime_presentation.py`:
- Around line 95-98: The returned presentation snapshot merges the local
variable stability into presentation["stability"] but doesn't persist the
already-normalized cleanup_failures value, causing inconsistent shapes; update
the code that builds presentation["stability"] (the merge of stability and
"last_cleanup_error") to include the normalized cleanup_failures (the value
produced earlier where cleanup_failures was normalized) so that
presentation["stability"]["cleanup_failures"] is the normalized value rather
than the original unnormalized field.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 235-237: Remove the redundant validation sentence and consolidate
the three overlapping lines into a single clear instruction: delete the stale
line "After each code change, run lint and guard..." and keep a single
consolidated line that directs contributors to run `scripts/lint.sh`,
`scripts/test.sh`, and `scripts/guard.sh` (or `scripts/check.sh`) after each
code change so AGENTS.md contains one authoritative validation instruction.
In `@src/browser_cli/commands/status.py`:
- Around line 115-117: The status rendering currently pulls stability from the
top-level live_payload before considering the daemon-normalized presentation
snapshot; update the logic so stability is assigned from
presentation.get("stability") first (falling back to (live_payload or
{}).get("stability") if absent) while keeping presentation = dict((live_payload
or {}).get("presentation") or {}) and overall_status computed via
presentation.get("overall_state") or _classify_overall_status as before; adjust
the variable `stability` (and any downstream uses) to prefer the presentation
snapshot to ensure daemon-normalized state is rendered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fd5a2cf-fe85-44bf-a0e4-ebcca12f364b
📒 Files selected for processing (17)
AGENTS.mdREADME.mddocs/smoke-checklist.mddocs/superpowers/plans/2026-04-13-browser-cli-long-run-stability-implementation-plan.mddocs/superpowers/specs/2026-04-13-browser-cli-long-run-stability-design.mdscripts/check.shscripts/guards/docs_sync.pyscripts/lint.shscripts/test.shsrc/browser_cli/commands/status.pysrc/browser_cli/daemon/browser_service.pysrc/browser_cli/daemon/runtime_presentation.pytests/integration/test_runtime_stability.pytests/unit/test_daemon_browser_service.pytests/unit/test_extension_transport.pytests/unit/test_lifecycle_commands.pytests/unit/test_runtime_presentation.py
💤 Files with no reviewable changes (1)
- scripts/lint.sh
|
|
||
| Date: 2026-04-13 | ||
| Status: Drafted for review | ||
| Repo: `/home/hongv/workspace/browser-cli` |
There was a problem hiding this comment.
Remove developer-specific absolute path from specification.
The hard-coded path /home/hongv/workspace/browser-cli is machine-specific and will be inaccurate for other contributors. Replace with a generic reference like "the repository root" or simply remove this line.
As per coding guidelines: "Tests and workflow fixtures must resolve repo assets relative to the checked-out repository, not a developer-specific absolute workspace path."
📝 Suggested fix
-Repo: `/home/hongv/workspace/browser-cli`
+Repo: browser-cli📝 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.
| Repo: `/home/hongv/workspace/browser-cli` | |
| Repo: browser-cli |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-13-browser-cli-long-run-stability-design.md`
at line 5, The spec contains a developer-specific absolute path string
'/home/hongv/workspace/browser-cli'; remove or replace that exact string in
docs/superpowers/specs/2026-04-13-browser-cli-long-run-stability-design.md with
a generic reference such as "the repository root" or "the checked-out
repository" so tests and workflow fixtures resolve assets relative to the repo
rather than a machine-specific path.
| "stability": { | ||
| **stability, | ||
| "last_cleanup_error": last_cleanup_error, | ||
| }, |
There was a problem hiding this comment.
Normalize cleanup_failures in the returned presentation snapshot.
Line 20 already normalizes cleanup_failures, but Lines 95-98 don’t persist that normalized value into presentation["stability"]. That can produce inconsistent downstream rendering/contract shape.
🔧 Proposed fix
"stability": {
**stability,
+ "cleanup_failures": cleanup_failures,
"last_cleanup_error": last_cleanup_error,
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser_cli/daemon/runtime_presentation.py` around lines 95 - 98, The
returned presentation snapshot merges the local variable stability into
presentation["stability"] but doesn't persist the already-normalized
cleanup_failures value, causing inconsistent shapes; update the code that builds
presentation["stability"] (the merge of stability and "last_cleanup_error") to
include the normalized cleanup_failures (the value produced earlier where
cleanup_failures was normalized) so that
presentation["stability"]["cleanup_failures"] is the normalized value rather
than the original unnormalized field.
1 participant
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores