Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive Browser CLI packaging and skill system, adding three-skill delivery orchestration, automation manifest projection/roundtrip semantics, packaged skill distribution via wheels, updated CI/release validation, daemon parameter normalization, and uninstall documentation with supporting tests and design specs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as API Server
participant Projections as Projections Module
participant Persistence as Persistence Store
participant Publisher
Client->>Server: POST /automations (payload)
Server->>Projections: payload_to_persisted_definition(payload)
Projections->>Persistence: create_automation(definition)
Persistence-->>Projections: automation_id
Projections-->>Server: PersistedAutomationDefinition
Server->>Server: validate and enable
Client->>Server: GET /automations/{id}/export
Server->>Persistence: load_automation(id)
Persistence-->>Server: PersistedAutomationDefinition
Server->>Projections: persisted_definition_to_manifest_toml(definition)
Projections-->>Server: TOML string
Server-->>Client: TOML manifest
Client->>Publisher: publish_task_dir(directory)
Publisher->>Persistence: load_automation(manifest_id)
Persistence-->>Publisher: PersistedAutomationDefinition
Publisher->>Projections: manifest_to_snapshot_manifest_toml(manifest)
Projections-->>Publisher: snapshot TOML
Publisher->>Publisher: write snapshot manifest
sequenceDiagram
participant User
participant CLI as install-skills CLI
participant Discovery as Packaged Skills Discovery
participant Resources as importlib.resources
participant Installation as Installation Handler
participant Filesystem as Filesystem
User->>CLI: install-skills --target /custom/path
CLI->>CLI: get_skills_target_path('/custom/path')
CLI->>Discovery: discover_packaged_skills()
Discovery->>Resources: files('browser_cli.packaged_skills')
Resources-->>Discovery: packaged skills root
Discovery->>Discovery: iterate PUBLIC_SKILL_NAMES
Discovery->>Filesystem: verify SKILL.md exists for each
Filesystem-->>Discovery: skill validity check
Discovery-->>CLI: list[PackagedSkill]
CLI->>Installation: install_skills_from_paths(skills, target, dry_run=False)
loop for each skill
Installation->>Filesystem: create temp directory
Installation->>Resources: extract skill files as_file()
Resources-->>Filesystem: staged skill content
Installation->>Filesystem: remove old skill directory (if exists)
Installation->>Filesystem: move staged content to target
end
Installation-->>CLI: list[tuple(skill_name, action)]
CLI-->>User: "Installed 3 skills"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
tests/unit/test_daemon_app_validation.py (1)
145-162: Add zero-value regression cases for defaulted numeric fields.Given the new helper migration, it’s worth locking behavior for explicit
0values (dy,timeout, maybecount) so defaults only apply when fields are missing/blank.Suggested tests
+def test_scroll_preserves_explicit_zero_dy() -> None: + payload = _execute( + DaemonRequest( + action="scroll", + args={"dy": 0}, + agent_id="agent-a", + request_id="req-6", + ) + ) + assert payload["ok"] is True + assert payload["data"]["dy"] == 0 + + +def test_wait_network_preserves_explicit_zero_timeout() -> None: + payload = _execute( + DaemonRequest( + action="wait-network", + args={"timeout": 0}, + agent_id="agent-a", + request_id="req-7", + ) + ) + assert payload["ok"] is True + assert payload["data"]["timeout_seconds"] == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_daemon_app_validation.py` around lines 145 - 162, Add regression tests that verify explicit zero values are preserved rather than replaced by defaults: extend or add tests around test_mouse_click_successfully_parses_integer_fields (and similar request handlers that use the helper) to call _execute with DaemonRequest args providing "dy": "0", "timeout": "0" and "count": "0" (both as strings and as ints) and assert payload["data"] contains numeric 0 for those fields; also add cases where fields are missing/blank to confirm defaults still apply when absent. Ensure you reference the same helper invocation (_execute) and DaemonRequest construction so the tests exercise the migrated helper logic.skills/browser-cli-converge/SKILL.md (1)
1-62: Consider a single source of truth for duplicated SKILL docs.This file and
src/browser_cli/packaged_skills/browser-cli-converge/SKILL.mdare currently identical; that’s easy to drift over time. Consider generating the packaged copy from this canonical source during packaging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/browser-cli-converge/SKILL.md` around lines 1 - 62, The SKILL.md content for the browser-cli-converge skill is duplicated (browser-cli-converge SKILL.md and src/browser_cli/packaged_skills/browser-cli-converge/SKILL.md); make the first file the canonical source and remove manual duplication by updating the packaging pipeline to copy or generate src/browser_cli/packaged_skills/browser-cli-converge/SKILL.md from the canonical SKILL.md during build/packaging (or replace the packaged file with a symlink/redirect), and update packaging scripts to assert equality so future edits only need to change the canonical SKILL.md named "browser-cli-converge".README.md (1)
262-264: Update the skill docs link to the split skill stack.The documentation list still references the legacy
browser-cli-explore-deliverypath. Consider linking the current delivery/explore/converge skill docs for consistency.Suggested docs update
-- Explore-to-task skill: [`skills/browser-cli-explore-delivery/SKILL.md`](skills/browser-cli-explore-delivery/SKILL.md) +- Browser CLI delivery skills: + - [`skills/browser-cli-delivery/SKILL.md`](skills/browser-cli-delivery/SKILL.md) + - [`skills/browser-cli-explore/SKILL.md`](skills/browser-cli-explore/SKILL.md) + - [`skills/browser-cli-converge/SKILL.md`](skills/browser-cli-converge/SKILL.md)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 262 - 264, Update the obsolete skill docs link by replacing the legacy "skills/browser-cli-explore-delivery/SKILL.md" entry with links to the split skill stack (delivery, explore, converge); specifically edit the README entry that currently lists "Explore-to-task skill: [`skills/browser-cli-explore-delivery/SKILL.md`]" and change it to three links pointing to the current skill docs for delivery, explore, and converge (use the canonical SKILL.md in each respective skill directory and keep the same list formatting).skills/browser-cli-delivery/SKILL.md (1)
26-26: Minor: Consider hyphenating "Browser CLI-based".Per standard English grammar, compound modifiers before a noun typically use a hyphen.
📝 Optional fix
-- the task is not Browser CLI based +- the task is not Browser CLI-based🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/browser-cli-delivery/SKILL.md` at line 26, The phrase "the task is not Browser CLI based" should use a hyphenated compound modifier; update the text in SKILL.md where that phrase occurs (search for "the task is not Browser CLI based") to read "the task is not Browser CLI-based" so the compound adjective "Browser CLI-based" is grammatically correct.docs/superpowers/plans/2026-04-14-automation-manifest-roundtrip-implementation-plan.md (1)
789-802: Consider parameterized identifiers for defense-in-depth.The
_ensure_columnhelper uses f-string interpolation for SQL identifiers:conn.execute(f"PRAGMA table_info({table})") conn.execute(f"ALTER TABLE {table} ADD COLUMN {column} {ddl}")While these values are hardcoded internal strings (not user input), using string formatting for SQL identifiers is generally discouraged. SQLite doesn't support parameterized identifiers, so this pattern is sometimes unavoidable, but consider adding a comment noting that
table,column, andddlmust be trusted internal values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-14-automation-manifest-roundtrip-implementation-plan.md` around lines 789 - 802, The _ensure_column function interpolates SQL identifiers (table, column, ddl) directly into SQL strings which is acceptable only because these values are internal; add a concise defense-in-depth fix: update the _ensure_column implementation to validate or sanitize identifiers (e.g., assert they match a safe regex like r'^[A-Za-z_][A-Za-z0-9_]*$' for table/column) and/or add a clear comment above _ensure_column stating that table, column, and ddl must be trusted internal values and must never come from user input; reference the _ensure_column function and the two conn.execute calls to locate where to add the validation/comment.src/browser_cli/commands/install_skills.py (1)
46-49: Consider callingresolve()on the default path for consistency.
get_skills_target_pathcallsresolve()when an explicit target is provided (line 48) but not for the default~/.agents/skillspath (line 49). For consistency and to handle edge cases with relative symlinks in$HOME, consider resolving the default path as well.♻️ Optional consistency fix
def get_skills_target_path(target: str | None) -> Path: if target: return Path(target).expanduser().resolve() - return Path.home() / ".agents" / "skills" + return (Path.home() / ".agents" / "skills").resolve()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser_cli/commands/install_skills.py` around lines 46 - 49, get_skills_target_path currently calls resolve() only for explicit targets but returns an unresolved Path.home() / ".agents" / "skills" for the default; change the default return in get_skills_target_path to also expanduser() and resolve() (e.g., resolve Path.home().expanduser()/".agents"/"skills") so both branches return fully resolved absolute paths.
🤖 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/installed-with-uv.md`:
- Around line 112-115: The markdown link in the "Remove Browser CLI" section
currently points to docs/uninstall.md which resolves incorrectly; update the
link target to the same-directory relative path "uninstall.md" (i.e., change the
href string `docs/uninstall.md` to `uninstall.md`) in the installed-with-uv.md
file so the link correctly resolves to the local uninstall document.
In
`@docs/superpowers/plans/2026-04-14-browser-cli-install-skills-implementation-plan.md`:
- Around line 219-222: The plan uses PackagedSkill(...) with a path= keyword but
the actual dataclass PackagedSkill expects source: Traversable | Path; update
the document snippet to use source= instead of path= for entries like
PackagedSkill(name="browser-cli-delivery", source=source_root /
"browser-cli-delivery") so the docs match the implementation in the
PackagedSkill dataclass.
In `@docs/superpowers/specs/2026-04-14-browser-cli-delivery-skills-design.md`:
- Line 5: Replace the hardcoded developer workspace path string
`/home/hongv/workspace/browser-cli` in the design spec with a portable reference
such as `Repo: browser-cli` or remove the path entirely; update the single-line
"Repo: ..." entry in
docs/superpowers/specs/2026-04-14-browser-cli-delivery-skills-design.md so it no
longer contains the developer-specific path.
In `@docs/superpowers/specs/2026-04-14-browser-cli-uninstall-doc-design.md`:
- Line 5: Remove the developer-specific absolute path that appears in the spec
header (the string "/home/hongv/workspace/browser-cli") and replace it with a
neutral, repo-relative identifier or nothing; update the top-of-file header in
the browser-cli uninstall doc (the spec header line) so it contains no
machine/user-specific paths and use a generic repository name like "browser-cli"
or a relative path if context is needed.
In `@src/browser_cli/automation/projections.py`:
- Around line 268-279: The _remap_result_json_path function currently returns
target_artifact_dir / relative even when the computed relative path contains
parent-traversal segments (e.g., '..'), allowing escape from the target artifact
dir; change the logic to detect any '..' segments in the computed relative
(check relative.parts for '..') and if found fall back to returning
target_artifact_dir / source_result_json_path.name instead of joining the unsafe
relative, otherwise continue to return target_artifact_dir / relative; keep the
early None guard and the ValueError fallback behavior unchanged.
- Around line 46-81: In payload_to_persisted_definition, the enabled field
currently uses bool(payload.get("enabled")) which treats non-empty strings like
"false" as True; change the logic to explicitly handle booleans and known string
values: if payload["enabled"] is a bool use it, if it's a str accept
case-insensitive "true"/"false" and convert accordingly, and for other types (or
unknown strings) raise a ValueError so malformed input is rejected rather than
silently enabling automations; update the assignment for enabled in
PersistedAutomationDefinition accordingly.
In `@src/browser_cli/commands/install_skills.py`:
- Around line 84-97: In _install_one_skill change the order so we don't delete
the existing destination before a successful move: keep staging into
TemporaryDirectory as staged (Path tmp_dir / skill.name), then move staged to a
temporary target (e.g., destination.with_suffix or destination + ".tmp") using
shutil.move; only after that move succeeds, remove the old destination with
shutil.rmtree and finally rename/replace the temp target to destination (or use
os.replace for atomic swap). Ensure exceptions still raise OperationFailedError
and that TemporaryDirectory cleanup and any leftover temp targets are handled so
the old directory remains intact if the move fails.
In `@src/browser_cli/daemon/app.py`:
- Around line 476-477: The current use of "or" to apply defaults (e.g., dx =
self._optional_int(request.args, "dx") or 0) will replace valid zeros with the
fallback; instead, change these assignments (dx, dy and the other spots using
self._optional_int/self._optional_float) to check for None explicitly and only
substitute the default when the helper returned None (e.g., value =
self._optional_int(...); dx = value if value is not None else 0). Update the
occurrences referenced (dx, dy and the other similar lines) to follow this
pattern so explicit 0 or 0.0 values are preserved.
---
Nitpick comments:
In
`@docs/superpowers/plans/2026-04-14-automation-manifest-roundtrip-implementation-plan.md`:
- Around line 789-802: The _ensure_column function interpolates SQL identifiers
(table, column, ddl) directly into SQL strings which is acceptable only because
these values are internal; add a concise defense-in-depth fix: update the
_ensure_column implementation to validate or sanitize identifiers (e.g., assert
they match a safe regex like r'^[A-Za-z_][A-Za-z0-9_]*$' for table/column)
and/or add a clear comment above _ensure_column stating that table, column, and
ddl must be trusted internal values and must never come from user input;
reference the _ensure_column function and the two conn.execute calls to locate
where to add the validation/comment.
In `@README.md`:
- Around line 262-264: Update the obsolete skill docs link by replacing the
legacy "skills/browser-cli-explore-delivery/SKILL.md" entry with links to the
split skill stack (delivery, explore, converge); specifically edit the README
entry that currently lists "Explore-to-task skill:
[`skills/browser-cli-explore-delivery/SKILL.md`]" and change it to three links
pointing to the current skill docs for delivery, explore, and converge (use the
canonical SKILL.md in each respective skill directory and keep the same list
formatting).
In `@skills/browser-cli-converge/SKILL.md`:
- Around line 1-62: The SKILL.md content for the browser-cli-converge skill is
duplicated (browser-cli-converge SKILL.md and
src/browser_cli/packaged_skills/browser-cli-converge/SKILL.md); make the first
file the canonical source and remove manual duplication by updating the
packaging pipeline to copy or generate
src/browser_cli/packaged_skills/browser-cli-converge/SKILL.md from the canonical
SKILL.md during build/packaging (or replace the packaged file with a
symlink/redirect), and update packaging scripts to assert equality so future
edits only need to change the canonical SKILL.md named "browser-cli-converge".
In `@skills/browser-cli-delivery/SKILL.md`:
- Line 26: The phrase "the task is not Browser CLI based" should use a
hyphenated compound modifier; update the text in SKILL.md where that phrase
occurs (search for "the task is not Browser CLI based") to read "the task is not
Browser CLI-based" so the compound adjective "Browser CLI-based" is
grammatically correct.
In `@src/browser_cli/commands/install_skills.py`:
- Around line 46-49: get_skills_target_path currently calls resolve() only for
explicit targets but returns an unresolved Path.home() / ".agents" / "skills"
for the default; change the default return in get_skills_target_path to also
expanduser() and resolve() (e.g., resolve
Path.home().expanduser()/".agents"/"skills") so both branches return fully
resolved absolute paths.
In `@tests/unit/test_daemon_app_validation.py`:
- Around line 145-162: Add regression tests that verify explicit zero values are
preserved rather than replaced by defaults: extend or add tests around
test_mouse_click_successfully_parses_integer_fields (and similar request
handlers that use the helper) to call _execute with DaemonRequest args providing
"dy": "0", "timeout": "0" and "count": "0" (both as strings and as ints) and
assert payload["data"] contains numeric 0 for those fields; also add cases where
fields are missing/blank to confirm defaults still apply when absent. Ensure you
reference the same helper invocation (_execute) and DaemonRequest construction
so the tests exercise the migrated helper logic.
🪄 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: 1a274e67-e510-4625-aea6-f32e298ca184
📒 Files selected for processing (54)
.github/workflows/ci.yml.github/workflows/release.ymlAGENTS.mdREADME.mddocs/installed-with-uv.mddocs/superpowers/plans/2026-04-11-browser-cli-network-response-body-implementation-plan.mddocs/superpowers/plans/2026-04-13-browser-cli-task-automation-implementation-plan.mddocs/superpowers/plans/2026-04-14-automation-manifest-roundtrip-implementation-plan.mddocs/superpowers/plans/2026-04-14-browser-cli-delivery-skills-implementation-plan.mddocs/superpowers/plans/2026-04-14-browser-cli-install-skills-implementation-plan.mddocs/superpowers/plans/2026-04-14-browser-cli-uninstall-doc-implementation-plan.mddocs/superpowers/plans/2026-04-14-daemon-parameter-validation-implementation-plan.mddocs/superpowers/specs/2026-04-13-browser-cli-task-automation-design.mddocs/superpowers/specs/2026-04-14-automation-manifest-roundtrip-design.mddocs/superpowers/specs/2026-04-14-browser-cli-delivery-skills-design.mddocs/superpowers/specs/2026-04-14-browser-cli-uninstall-doc-design.mddocs/superpowers/specs/2026-04-14-daemon-parameter-validation-design.mddocs/superpowers/specs/2026-04-14-install-skills-design.mddocs/uninstall.mdpyproject.tomlscripts/guards/architecture.pyskills/browser-cli-converge/SKILL.mdskills/browser-cli-delivery/SKILL.mdskills/browser-cli-explore-delivery/SKILL.mdskills/browser-cli-explore-delivery/agents/openai.yamlskills/browser-cli-explore-delivery/references/preflight-and-runtime.mdskills/browser-cli-explore-delivery/references/task-input-design.mdskills/browser-cli-explore-delivery/references/task-modes.mdskills/browser-cli-explore/SKILL.mdsrc/browser_cli/automation/api/server.pysrc/browser_cli/automation/models.pysrc/browser_cli/automation/persistence/store.pysrc/browser_cli/automation/projections.pysrc/browser_cli/automation/publisher.pysrc/browser_cli/cli/main.pysrc/browser_cli/commands/automation.pysrc/browser_cli/commands/install_skills.pysrc/browser_cli/daemon/app.pysrc/browser_cli/packaged_skills/__init__.pysrc/browser_cli/packaged_skills/browser-cli-converge/SKILL.mdsrc/browser_cli/packaged_skills/browser-cli-delivery/SKILL.mdsrc/browser_cli/packaged_skills/browser-cli-explore/SKILL.mdtests/unit/test_automation_api.pytests/unit/test_automation_commands.pytests/unit/test_automation_projections.pytests/unit/test_automation_publish.pytests/unit/test_cli.pytests/unit/test_daemon_app_validation.pytests/unit/test_install_skills_command.pytests/unit/test_release_artifacts.pytests/unit/test_repo_metadata.pytests/unit/test_repo_skill_docs.pytests/unit/test_repo_text_contracts.pytests/unit/test_task_runtime_automation.py
💤 Files with no reviewable changes (5)
- skills/browser-cli-explore-delivery/agents/openai.yaml
- skills/browser-cli-explore-delivery/references/task-modes.md
- skills/browser-cli-explore-delivery/references/task-input-design.md
- skills/browser-cli-explore-delivery/references/preflight-and-runtime.md
- skills/browser-cli-explore-delivery/SKILL.md
Comment on lines
+112
to
+115
| ## Remove Browser CLI | ||
|
|
||
| To remove Browser CLI later, including Browser CLI home data and local cleanup | ||
| steps for maintainers, see [`docs/uninstall.md`](docs/uninstall.md). |
There was a problem hiding this comment.
Fix the relative uninstall link target.
Line 115 currently links to docs/uninstall.md from inside docs/installed-with-uv.md, which resolves as docs/docs/uninstall.md. Use a same-directory relative link.
Suggested fix
-To remove Browser CLI later, including Browser CLI home data and local cleanup
-steps for maintainers, see [`docs/uninstall.md`](docs/uninstall.md).
+To remove Browser CLI later, including Browser CLI home data and local cleanup
+steps for maintainers, see [`docs/uninstall.md`](uninstall.md).📝 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.
Suggested change
| ## Remove Browser CLI | |
| To remove Browser CLI later, including Browser CLI home data and local cleanup | |
| steps for maintainers, see [`docs/uninstall.md`](docs/uninstall.md). | |
| ## Remove Browser CLI | |
| To remove Browser CLI later, including Browser CLI home data and local cleanup | |
| steps for maintainers, see [`docs/uninstall.md`](uninstall.md). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/installed-with-uv.md` around lines 112 - 115, The markdown link in the
"Remove Browser CLI" section currently points to docs/uninstall.md which
resolves incorrectly; update the link target to the same-directory relative path
"uninstall.md" (i.e., change the href string `docs/uninstall.md` to
`uninstall.md`) in the installed-with-uv.md file so the link correctly resolves
to the local uninstall document.
Comment on lines
+219
to
+222
| install_skills_module.PackagedSkill(name="browser-cli-delivery", path=source_root / "browser-cli-delivery"), | ||
| install_skills_module.PackagedSkill(name="browser-cli-explore", path=source_root / "browser-cli-explore"), | ||
| install_skills_module.PackagedSkill(name="browser-cli-converge", path=source_root / "browser-cli-converge"), | ||
| ], |
There was a problem hiding this comment.
Documentation drift: PackagedSkill attribute name mismatch.
The plan's test code uses path= attribute:
install_skills_module.PackagedSkill(name="browser-cli-delivery", path=source_root / "browser-cli-delivery"),However, the actual implementation (per src/browser_cli/commands/install_skills.py lines 22-25) uses source=:
`@dataclass`(frozen=True, slots=True)
class PackagedSkill:
name: str
source: Traversable | PathSince this plan document serves as a reference, consider updating the attribute name for consistency with the shipped code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@docs/superpowers/plans/2026-04-14-browser-cli-install-skills-implementation-plan.md`
around lines 219 - 222, The plan uses PackagedSkill(...) with a path= keyword
but the actual dataclass PackagedSkill expects source: Traversable | Path;
update the document snippet to use source= instead of path= for entries like
PackagedSkill(name="browser-cli-delivery", source=source_root /
"browser-cli-delivery") so the docs match the implementation in the
PackagedSkill dataclass.
|
|
||
| Date: 2026-04-14 | ||
| Status: Drafted for review | ||
| Repo: `/home/hongv/workspace/browser-cli` |
There was a problem hiding this comment.
Remove developer-specific absolute path from the spec header.
/home/hongv/workspace/browser-cli is machine/user-specific and shouldn’t be committed in shared docs.
Proposed 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.
Suggested change
| 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-14-browser-cli-uninstall-doc-design.md` at
line 5, Remove the developer-specific absolute path that appears in the spec
header (the string "/home/hongv/workspace/browser-cli") and replace it with a
neutral, repo-relative identifier or nothing; update the top-of-file header in
the browser-cli uninstall doc (the spec header line) so it contains no
machine/user-specific paths and use a generic repository name like "browser-cli"
or a relative path if context is needed.
Comment on lines
+46
to
+81
| def payload_to_persisted_definition(payload: dict[str, Any]) -> PersistedAutomationDefinition: | ||
| automation_id = str(payload.get("id") or "").strip() | ||
| if not automation_id: | ||
| raise ValueError("Automation id is required.") | ||
| output_dir_raw = str(payload.get("output_dir") or "").strip() | ||
| result_json_raw = str(payload.get("result_json_path") or "").strip() | ||
| return PersistedAutomationDefinition( | ||
| id=automation_id, | ||
| name=str(payload.get("name") or automation_id), | ||
| description=str(payload.get("description") or ""), | ||
| version=str(payload.get("version") or "0.1.0"), | ||
| task_path=Path(str(payload.get("task_path") or "")), | ||
| task_meta_path=Path(str(payload.get("task_meta_path") or "")), | ||
| entrypoint=str(payload.get("entrypoint") or "run"), | ||
| enabled=bool(payload.get("enabled")), | ||
| definition_status=str(payload.get("definition_status") or "valid"), | ||
| definition_error=str(payload.get("definition_error")) | ||
| if payload.get("definition_error") | ||
| else None, | ||
| schedule_kind=str(payload.get("schedule_kind") or "manual"), | ||
| schedule_payload=dict(payload.get("schedule_payload") or {}), | ||
| timezone=str(payload.get("timezone") or "UTC"), | ||
| output_dir=Path(output_dir_raw) if output_dir_raw else Path(), | ||
| result_json_path=Path(result_json_raw) if result_json_raw else None, | ||
| stdout_mode=str(payload.get("stdout_mode") or "json"), | ||
| input_overrides=dict(payload.get("input_overrides") or {}), | ||
| before_run_hooks=tuple(payload.get("before_run_hooks") or []), | ||
| after_success_hooks=tuple(payload.get("after_success_hooks") or []), | ||
| after_failure_hooks=tuple(payload.get("after_failure_hooks") or []), | ||
| retry_attempts=int(payload.get("retry_attempts") or 0), | ||
| retry_backoff_seconds=int(payload.get("retry_backoff_seconds") or 0), | ||
| timeout_seconds=float(payload["timeout_seconds"]) | ||
| if payload.get("timeout_seconds") is not None | ||
| else None, | ||
| log_level=str(payload.get("log_level") or "info"), | ||
| ) |
There was a problem hiding this comment.
Fix boolean coercion for enabled to avoid silent misconfiguration.
Line 60 currently uses bool(payload.get("enabled")); non-empty strings like "false" evaluate to True, which can silently enable automations instead of rejecting malformed input.
Proposed fix
def payload_to_persisted_definition(payload: dict[str, Any]) -> PersistedAutomationDefinition:
automation_id = str(payload.get("id") or "").strip()
if not automation_id:
raise ValueError("Automation id is required.")
+ enabled_raw = payload.get("enabled")
+ if isinstance(enabled_raw, bool):
+ enabled = enabled_raw
+ elif enabled_raw is None:
+ enabled = False
+ elif isinstance(enabled_raw, str):
+ normalized = enabled_raw.strip().lower()
+ if normalized in {"true", "1", "yes", "on"}:
+ enabled = True
+ elif normalized in {"false", "0", "no", "off", ""}:
+ enabled = False
+ else:
+ raise ValueError("enabled must be a boolean.")
+ else:
+ raise ValueError("enabled must be a boolean.")
output_dir_raw = str(payload.get("output_dir") or "").strip()
result_json_raw = str(payload.get("result_json_path") or "").strip()
return PersistedAutomationDefinition(
id=automation_id,
@@
- enabled=bool(payload.get("enabled")),
+ enabled=enabled,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser_cli/automation/projections.py` around lines 46 - 81, In
payload_to_persisted_definition, the enabled field currently uses
bool(payload.get("enabled")) which treats non-empty strings like "false" as
True; change the logic to explicitly handle booleans and known string values: if
payload["enabled"] is a bool use it, if it's a str accept case-insensitive
"true"/"false" and convert accordingly, and for other types (or unknown strings)
raise a ValueError so malformed input is rejected rather than silently enabling
automations; update the assignment for enabled in PersistedAutomationDefinition
accordingly.
Comment on lines
+268
to
+279
| def _remap_result_json_path( | ||
| source_artifact_dir: Path, | ||
| source_result_json_path: Path | None, | ||
| target_artifact_dir: Path, | ||
| ) -> Path | None: | ||
| if source_result_json_path is None: | ||
| return None | ||
| try: | ||
| relative = source_result_json_path.relative_to(source_artifact_dir) | ||
| except ValueError: | ||
| return target_artifact_dir / source_result_json_path.name | ||
| return target_artifact_dir / relative |
There was a problem hiding this comment.
Guard against .. path segments when remapping snapshot result_json_path.
Lines 276-279 can carry parent traversal into the target path if the source is lexically under the artifact dir (e.g., artifacts/../out.json), which can escape snapshot artifact boundaries.
Proposed fix
def _remap_result_json_path(
source_artifact_dir: Path,
source_result_json_path: Path | None,
target_artifact_dir: Path,
) -> Path | None:
@@
try:
relative = source_result_json_path.relative_to(source_artifact_dir)
except ValueError:
return target_artifact_dir / source_result_json_path.name
+ if ".." in relative.parts:
+ return target_artifact_dir / source_result_json_path.name
return target_artifact_dir / relative🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser_cli/automation/projections.py` around lines 268 - 279, The
_remap_result_json_path function currently returns target_artifact_dir /
relative even when the computed relative path contains parent-traversal segments
(e.g., '..'), allowing escape from the target artifact dir; change the logic to
detect any '..' segments in the computed relative (check relative.parts for
'..') and if found fall back to returning target_artifact_dir /
source_result_json_path.name instead of joining the unsafe relative, otherwise
continue to return target_artifact_dir / relative; keep the early None guard and
the ValueError fallback behavior unchanged.
Comment on lines
+476
to
+477
| dx = self._optional_int(request.args, "dx") or 0 | ||
| dy = self._optional_int(request.args, "dy") or 700 |
There was a problem hiding this comment.
or fallbacks are overriding explicit numeric 0 values.
Using self._optional_* (...) or <default> turns valid 0 into the default. Example: dy=0 becomes 700, timeout=0 becomes 30.0.
Proposed fix (preserve explicit zero via is None)
- dx = self._optional_int(request.args, "dx") or 0
- dy = self._optional_int(request.args, "dy") or 700
+ dx = self._optional_int(request.args, "dx")
+ if dx is None:
+ dx = 0
+ dy = self._optional_int(request.args, "dy")
+ if dy is None:
+ dy = 700
@@
- count = self._optional_int(request.args, "count") or 1
+ count = self._optional_int(request.args, "count")
+ if count is None:
+ count = 1
@@
- timeout_seconds = self._optional_float(request.args, "timeout") or 30.0
+ timeout_seconds = self._optional_float(request.args, "timeout")
+ if timeout_seconds is None:
+ timeout_seconds = 30.0
@@
- timeout_seconds = self._optional_float(request.args, "timeout_seconds") or 30.0
+ timeout_seconds = self._optional_float(request.args, "timeout_seconds")
+ if timeout_seconds is None:
+ timeout_seconds = 30.0
@@
- timeout = self._optional_float(request.args, "timeout") or 5.0
+ timeout = self._optional_float(request.args, "timeout")
+ if timeout is None:
+ timeout = 5.0
@@
- timeout = self._optional_float(request.args, "timeout") or 5.0
+ timeout = self._optional_float(request.args, "timeout")
+ if timeout is None:
+ timeout = 5.0Also applies to: 485-485, 573-573, 624-624, 735-735, 746-746
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/browser_cli/daemon/app.py` around lines 476 - 477, The current use of
"or" to apply defaults (e.g., dx = self._optional_int(request.args, "dx") or 0)
will replace valid zeros with the fallback; instead, change these assignments
(dx, dy and the other spots using self._optional_int/self._optional_float) to
check for None explicitly and only substitute the default when the helper
returned None (e.g., value = self._optional_int(...); dx = value if value is not
None else 0). Update the occurrences referenced (dx, dy and the other similar
lines) to follow this pattern so explicit 0 or 0.0 values are preserved.
…to release-optimization-followups
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
--targetflag toinstall-skillscommand for custom skill installation directoriesdocs/uninstall.md) with cleanup instructionsDocumentation
Improvements