Skip to content

fix: harden browser tools start and console flag - #28

Merged
steipete merged 5 commits into
steipete:mainfrom
ShiroKSH:fix/browser-tools-start-console
Jul 11, 2026
Merged

fix: harden browser tools start and console flag#28
steipete merged 5 commits into
steipete:mainfrom
ShiroKSH:fix/browser-tools-start-console

Conversation

@ShiroKSH

@ShiroKSH ShiroKSH commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • replace shell-built start setup commands with argv-based process/filesystem calls
  • copy Chrome profile directories with Node filesystem APIs instead of rsync
  • fix console no-serialize handling so Commander negated option is honored

Why

browser-tools start accepted profile paths and interpolated them into shell command strings. A path containing shell metacharacters could change the invoked command. The console command also declared no-serialize as a negated Commander option but read a non-existent noSerialize property, so the flag did not affect serialization.

Validation

  • built patched binary with Bun
  • smoke-tested root, start, and console help on the compiled binary
  • live start profile smoke with a temporary profile source and a profile destination containing a shell metacharacter
  • live console capture comparison showing serialized object output by default and raw text output with no-serialize

Live Proof

Redacted terminal output from the patched compiled binary:

proofRoot=[temp]/browser-tools-proof-75a5d014743241028a8abd5f9d93678c
port=35736
✓ Chrome listening on http://localhost:35736 (profile copied)
profileLocalStateCopied=True
browser=Edg/149.0.4022.80
✓ Opened in new tab: data:text/html,<html><body>browser-tools proof</body></html>
serializedConsole:
Capturing console logs for 2 seconds...
[LOG] 21:19:17.992 { alpha: 1, nested: { beta: 2 } }
rawConsole:
Capturing console logs for 2 seconds...
[LOG] 21:19:20.239 [object Object]

@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 7:30 PM ET / 23:30 UTC.

Summary
The PR replaces shell-interpolated browser profile setup with direct process/filesystem APIs, adds profile-copy and launcher compatibility tests, and fixes Commander --no-serialize handling.

Reproducibility: yes. The maintainer reproduced command substitution through current main's shell-built --profile-dir path, and source inspection independently confirms both that interpolation and the incorrect Commander property read.

Review metrics: 2 noteworthy metrics.

  • Compatibility coverage: 6 tests, 16 expectations reported. The exact-head proof exercises the destructive filesystem cases that generic lint or security checks cannot settle.
  • Patch surface: 3 files; 179 additions, 9 deletions. The change remains focused on one browser-tools workflow despite substantial defensive logic and regression coverage.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] Merging deliberately changes how existing profile destinations are emptied and refreshed, including symlinked directories; the exact-head tests and live proof substantially reduce this compatibility risk but do not make the operation non-destructive.

Maintainer options:

  1. Merge the exact proven head (recommended)
    Accept the intentional profile replacement behavior because this exact candidate has focused compatibility tests and live proof across source, destination, symlink, overlap, console, and launcher paths.
  2. Pause if the head changes
    Require a fresh targeted review and repeat the filesystem/runtime proof if additional commits alter profile-copy or launcher semantics before merge.

Next step before merge

  • [P2] The active exact-head PR is already repaired and well proven; the remaining action is ordinary maintainer merge handling rather than a separate automated fix job.

Security
Cleared: The diff removes user-controlled shell interpretation and adds no dependencies, workflow permissions, secret access, downloads, package-resolution changes, or new third-party execution.

Review details

Best possible solution:

Merge the exact proven head while retaining the focused profile-copy tests as the compatibility contract for source validation, overlap rejection, symlink preservation, destination replacement, and launcher execution.

Do we have a high-confidence way to reproduce the issue?

Yes. The maintainer reproduced command substitution through current main's shell-built --profile-dir path, and source inspection independently confirms both that interpolation and the incorrect Commander property read.

Is this the best way to solve the issue?

Yes. Direct filesystem/process APIs remove shell interpretation at the root, and the follow-up compatibility guards and tests address the previously found symlink and destructive-copy regressions without adding a parallel implementation path.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 5c6abe6de9a2.

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster. Replaced prior rating: 🦐 gold shrimp.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: The PR fixes a concrete command-injection path and broken CLI flag in a bounded optional browser-tools workflow.
  • merge-risk: 🚨 compatibility: The patch changes existing profile destination replacement, symlink handling, and launcher detection during startup.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster. Replaced prior rating: 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR contains redacted after-fix terminal output, and the maintainer supplied exact-head live Chrome, filesystem, console, compiled-launcher, and regression-suite proof with an observed improved result.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR contains redacted after-fix terminal output, and the maintainer supplied exact-head live Chrome, filesystem, console, compiled-launcher, and regression-suite proof with an observed improved result.
Evidence reviewed

What I checked:

  • Shell interpretation removed: Current main interpolates profileDir into mkdir and rsync shell strings, while the candidate uses direct filesystem calls and passes Chrome arguments through spawn. (scripts/browser-tools.ts:119, e281904c32e2)
  • Previous finding resolved: The exact head preserves relative symlink text with verbatimSymlinks: true; its regression test asserts that a copied relative link still targets target. (scripts/browser-tools.ts:85, e281904c32e2)
  • Destructive boundary guarded: The candidate validates the source before destination mutation, rejects source/destination overlap, preserves directory symlink destinations, and safely replaces links to non-directories; focused tests cover each behavior. (scripts/browser-tools-profile.test.ts:25, e281904c32e2)
  • Exact-head live proof: The maintainer reports live Chrome 150 validation of literal shell metacharacters, profile copying, relative and top-level symlinks, linked destinations, overlap rejection, both console serialization modes, compiled launchers, and 6 tests with 16 expectations on this exact candidate. (e281904c32e2)
  • GitHub merge state: GitHub reports the exact head as mergeable against the stated base; the available GitGuardian security check passed, although the aggregate merge state is unstable. (e281904c32e2)
  • Feature provenance: Git history ties browser startup to Peter Steinberger's original browser-tools work and console serialization controls to the merged console-command contribution. (scripts/browser-tools.ts:110, 35355d0d3bae)

Likely related people:

  • steipete: Introduced the browser-tools startup behavior, recently maintained the file, authored four compatibility follow-ups on this candidate, and supplied exact-head live proof. (role: feature owner and recent area contributor; confidence: high; commits: 35355d0d3bae, 9ff786db80b4, 2edc27ba104b; files: scripts/browser-tools.ts, scripts/browser-tools-profile.test.ts)
  • srigi: The merged console-command work introduced the serialization option surface that this PR corrects. (role: console feature introducer; confidence: medium; commits: ea0e72062947; files: scripts/browser-tools.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (4 earlier review cycles)
  • reviewed 2026-07-09T18:13:18.377Z sha 8524425 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-09T18:23:30.368Z sha abe8545 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T18:28:54.224Z sha abe8545 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-10T19:49:21.685Z sha abe8545 :: needs changes before merge. :: [P1] Preserve relative symlinks when copying the Chrome profile

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 9, 2026
@ShiroKSH
ShiroKSH force-pushed the fix/browser-tools-start-console branch from 8524425 to abe8545 Compare July 9, 2026 18:19
@ShiroKSH

ShiroKSH commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Proof added to the PR body.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 9, 2026
@ShiroKSH
ShiroKSH marked this pull request as ready for review July 10, 2026 19:28
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 10, 2026
@steipete
steipete force-pushed the fix/browser-tools-start-console branch from abe8545 to e281904 Compare July 11, 2026 23:26
@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Maintainer proof for exact candidate e281904c32e2641ce522582481e49bc2f83dbfef on base 5c6abe6de9a2dedd370dbc3e0e7779b56c17d2c6:

  • Current-main reproduction: a command substitution embedded in --profile-dir executed through the former shell-built setup path.
  • Exact candidate browser runtime and regression-test blobs are byte-identical to the fully live-proven candidate.
  • Live Chrome 150 proof on macOS: literal shell metacharacters remained path data with no marker side effect; profile copy and navigation succeeded; relative and top-level source symlinks were preserved; a symlinked destination remained linked and refreshed; overlapping paths were rejected without source loss.
  • Console proof: default capture serialized nested objects; --no-serialize emitted raw object text.
  • Browser/profile regression suite: 6 tests, 16 expectations, all passed.
  • Standalone compiled Bun binary and symlinked launcher help: passed. Node 22.12 and Node 20 direct/symlinked TypeScript launcher proof remains exact because the runtime blob is unchanged.
  • Full repository gates: 66-skill validation, docs listing, orchestrator policy test, ClawSweeper status tests, npm auth tests, skill-cleaner tests, and git diff --check all passed.
  • Dependency audit: 0 vulnerabilities across the resolved production graph.
  • AutoReview against current origin/main: clean, zero accepted/actionable findings, correctness confidence 0.96.
  • Public Model Identifier Gate: PASS. Candidate diff, tests, fixtures, compiled proof artifact, commit messages, existing public PR discussion, validation output, and this proof text contain no model identifiers requiring publication review.

Risk: medium. The change deliberately replaces browser-profile contents, but source validation, overlap rejection, symlink-preserving behavior, malformed-destination handling, and regression coverage now protect the destructive boundary.

@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Owner

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

steipete
steipete approved these changes Jul 11, 2026
View reviewed changes

@steipete steipete left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review at exact head e281904c32e2641ce522582481e49bc2f83dbfef: shell-injection reproduction, profile-copy safety and compatibility regressions, console behavior, full tests, live Chrome proof, dependency audit, exact-head security check, ClawSweeper re-review, and model-identifier gate are clean. Approved for squash merge.

@steipete
steipete merged commit 5cc32f9 into steipete:main Jul 11, 2026
1 check passed
@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Landed as 5cc32f9. Thank you, @ShiroKSH.

Verification:

  • Current-main shell-command reproduction established the original profile-path injection boundary.
  • Browser/profile regression suite: 6 tests, 16 expectations, all passed.
  • Compiled Bun CLI, direct and symlinked launchers, full repository tests, and dependency audit passed.
  • Live Chrome 150 proof covered path safety, profile copy/navigation, symlink preservation, overlap rejection, and both console serialization modes.
  • AutoReview and exact-head ClawSweeper re-review: clean; no changes requested.
  • Exact-head GitGuardian check: passed.
  • Post-merge CI: https://github.com/steipete/agent-scripts/actions/runs/29172280605 — smoke passed.
  • Public Model Identifier Gate: PASS.

No remaining caveats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steipete steipete steipete approved these changes

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShiroKSH @steipete