[STG-2510] fix: source eval browser skill from packages/cli browse SKILL.md#2334
Open
shrey150 wants to merge 3 commits into
Open
[STG-2510] fix: source eval browser skill from packages/cli browse SKILL.md#2334shrey150 wants to merge 3 commits into
shrey150 wants to merge 3 commits into
Conversation
|
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Architecture diagram
sequenceDiagram
participant Harness as Eval Harness
participant Adapter as claudeCodeToolAdapter
participant Fs as File System
participant Skill as CLI Browse Skill (packages/cli/skills/browse/SKILL.md)
participant Agent as Claude Code Agent (Eval)
participant Prompt as Harness Prompt
Note over Harness,Prompt: Eval Setup: Skill Installation
Harness->>Adapter: prepareBrowseCliHarnessAdapter()
Adapter->>Adapter: installBrowseSkill(wrapperDir)
Adapter->>Fs: readFile(BROWSE_SKILL_SOURCE)
Fs-->>Adapter: CLI skill markdown (355 lines, single source of truth)
Adapter->>Adapter: insertAfterFrontmatter(cliSkill, EVAL_HARNESS_ADDENDUM)
Note over Adapter: Injects addendum between YAML frontmatter and CLI skill body
Adapter->>Fs: writeFile(.claude/skills/browse/SKILL.md)
Fs-->>Adapter: written
Adapter-->>Harness: Installed browse skill at ...
Note over Agent,Skill: Eval Runtime: Agent Loads & Uses Skill
Agent->>Skill: Skill tool: load browse skill
Skill-->>Agent: Combined skill content (CLI skill + addendum)
Note over Agent: Addendum overrides CLI cloud/functions/skills examples
Agent->>Agent: Follows addendum rules (one browse command per Bash call)
Agent->>Adapter: Bash: browse open <url>
Adapter->>Adapter: isAllowedBrowseCommand() - checks starts with "browse " + no shell metacharacters
alt Valid browse command
Adapter-->>Agent: Executes browse command
Agent->>Adapter: Bash: browse snapshot
Adapter->>Adapter: isAllowedBrowseCommand()
Adapter-->>Agent: Executes browse snapshot
Agent->>Adapter: Bash: browse doctor
Adapter->>Adapter: isAllowedBrowseCommand()
Adapter-->>Agent: Executes browse doctor
else Command violates rules
Agent->>Adapter: Bash: browse cloud fetch
Adapter->>Adapter: isAllowedBrowseCommand()
Note over Adapter: Allowed syntactically, but addendum says ignore
Agent->>Agent: Addendum prohibits cloud commands, agent should skip
end
Note over Agent,Harness: Eval Runtime: Result Reporting
Agent->>Prompt: EVAL_RESULT format
Prompt-->>Harness: Benchmark result
alt Agent attempts disallowed pattern
Agent->>Adapter: Bash: browse snapshot | grep title
Adapter->>Adapter: isAllowedBrowseCommand() - detects shell operators
Adapter-->>Agent: Rejected: only single browse command allowed
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
shrey150
added a commit
that referenced
this pull request
Jul 9, 2026
Addresses cubic review comment on #2334. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ajmcquilkin
approved these changes
Jul 9, 2026
shrey150
added a commit
that referenced
this pull request
Jul 9, 2026
…idance (#2336) ## Summary A 3-provider bare-loop smoke test (E2B/Modal/Vercel, real sandboxes, local CLI build) surfaced two systematic gaps in the bundled `browse` skill (`packages/cli/skills/browse/SKILL.md`): 1. **3/3 agents ignored the pre-wired `BROWSE_SESSION` env var** and self-named sessions via `--session <name>` (one run orphaned a Browserbase session as a result). The skill taught named `--session` usage but never said "if `BROWSE_SESSION` is already set, every command already targets that session — don't pass `--session` or invent a new name." This defeats the sandbox-template env-steering pattern (`BROWSERBASE_API_KEY` + `BROWSE_SESSION=<name>` → flagless remote commands). 2. **2/3 agents tried `--local` first inside Chrome-less containers**, hit "No Chrome or Chromium found", then recovered on retry. The skill lacked explicit guidance that `--local` requires local Chrome/Chromium and that containers/CI/sandboxes without Chrome should go straight to `--remote`. ## What changed Two minimal additions to `packages/cli/skills/browse/SKILL.md`, both in the existing "Browser Target Selection" section (the section that already teaches local/remote choice and named-session usage) — no restructuring, no new sections: ``` `--local` requires Chrome or Chromium already installed on the machine. In containers, CI, and sandboxes with no browser installed, use `--remote` instead of `--local`. If `--local` fails with "No Chrome or Chromium found" and `BROWSERBASE_API_KEY` is set, switch to `--remote` — do not retry `--local`. ``` ``` If `BROWSE_SESSION` is already set in the environment, every command already targets that session — do not pass `--session` or invent a new name. An explicit `--session <name>` always overrides `BROWSE_SESSION` for that command, so only pass it to deliberately target a different session. ``` Verified the precedence claim against the actual implementation before writing the wording (not just trusting the smoke-test writeup): - `packages/cli/src/lib/driver/flags.ts:73` — `export function sessionName(value?: string): string { return value ?? process.env.BROWSE_SESSION ?? "default"; }`. An explicit `--session` flag always wins over `BROWSE_SESSION`; when omitted, `BROWSE_SESSION` becomes the effective session; with neither, it's `"default"`. - `packages/cli/src/lib/driver/command-cli.ts:75` — `runDriverCommandFromFlags` calls `sessionName(flags.session)` for every driver command, so this precedence applies uniformly across `open`, `snapshot`, `click`, `stop`, etc. - `packages/cli/src/lib/driver/remote.ts:130` — exact no-Chrome error string is `"No Chrome or Chromium found on this machine. Install one (Linux: apt install chromium · macOS: brew install --cask google-chrome, or Chromium with CHROME_PATH set), attach to a running browser with --cdp <port>, or set BROWSERBASE_API_KEY to use a remote browser."` ## No changeset This is a skill-text-only change (`SKILL.md`), same category as PR #2329 (skill-text-only fix, no changeset). The updated text ships with the next `browse` release — a release is already queued by #2335's changeset — and is consumed at runtime by the evals harness added in #2334. ## E2E Test Matrix | Command / flow | Observed output | Confidence / sufficiency | | --- | --- | --- | | `pnpm install && pnpm turbo run build --filter=browse` (worktree, local build) | `Tasks: 4 successful, 4 total` | Proves the local CLI build succeeds with the change in place; no build regressions from a docs-only edit. | | Resolve `bundledCliSkillPath()` from the compiled `dist/lib/skills/install.js` via `require.resolve` + manual path join (no `skills show` command exists on `main` yet — it ships in PR #2335, unmerged at time of this PR) | `resolved bundledCliSkillPath: <worktree>/packages/cli/skills/browse` | Proves the exact file `browse skills install` resolves to at runtime is the file this PR edits, not a stale copy. | | `grep -n "already set in the environment\|do not retry" packages/cli/skills/browse/SKILL.md` | `76:If \`BROWSE_SESSION\` is already set in the environment, every command already targets that session...` `68:...If \`--local\` fails with "No Chrome or Chromium found"...do not retry \`--local\`.` | Confirms the exact new lines are present at the bundled path resolved above — the new guidance flows through to the real command path. | | `node bin/run.js skills install --help` | Prints usage for `browse skills install` (no side effects triggered — did not run the real install, which mutates the global `~/.agents/skills` pool via `npx skills add --global`) | Confirms the command is wired and reachable; deliberately did not execute the real install to avoid mutating local global skill state outside this change's scope. | | `pnpm test:cli` (vitest, worktree build) | `Test Files 23 passed (23)` / `Tests 344 passed (344)` | Full existing CLI test suite still green; this change touches no runtime code. | | `pnpm eslint .` | No output / exit 0 | Lint clean. | | `pnpm format:check` (prettier) | `All matched files use Prettier code style!` | Formatting clean, including the edited Markdown. | | `tsc --noEmit -p tsconfig.json` | No output / exit 0 | Typecheck clean (unaffected by a Markdown-only change, included for completeness). | ## Related - Linear: https://linear.app/browserbase/issue/STG-2513/browse-skill-document-browse-session-precedence-container-remote - Evidence source: 3-provider (E2B/Modal/Vercel) bare-loop smoke test, 2026-07-09 - Text will be exercised by the evals harness landing in #2334 - Release vehicle: changeset already queued in #2335 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Clarifies the `browse` skill docs to prevent session misrouting and container failures by documenting `BROWSE_SESSION` precedence and when to use `--remote` instead of `--local`. Addresses Linear STG-2513. - **Bug Fixes** - Documented that if `BROWSE_SESSION` is set, commands target that session; only pass `--session` to override (precedence: `--session` > `BROWSE_SESSION` > `default`). - Stated that `--local` requires Chrome/Chromium; in containers/CI/sandboxes use `--remote`. If you see "No Chrome or Chromium found," switch to `--remote` instead of retrying `--local`. <sup>Written for commit 829ff7b. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/browserbase/stagehand/pull/2336?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses cubic review comment on #2334. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…regex Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2b7bbfc to
486cab8
Compare
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
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.
Why
The Claude Code eval harness installs a browser skill for the agent it spawns from
packages/evals/skills/browser/SKILL.md— a 68-line eval-local copy created once in "Evals v2" (#2011) and never touched again. The real, maintained browse skill lives atpackages/cli/skills/browse/SKILL.md(355 lines, ships with the CLI). Checking git history:packages/evals/skills/browser/SKILL.md: 1 commit ever (2026-05-01, Evals v2 #2011), never edited since.packages/cli/skills/browse/SKILL.md: 10 commits since 2026-06-05, most recently 2 days ago — roughly one edit every 3 days.That gap already produced a concrete factual error: the most recent CLI-skill commit (#2296, STG-2450) made
browse snapshotlean-by-default with--fullneeded for ref maps, but the frozen eval skill still documented the old always-full behavior. It also never mentioned--verified/--proxies/--auto-connect,browse doctor, retry discipline, tab/network/cdp/mouse/viewport commands, or Browse.sh skill discovery. The harness was testing an outdated mental model ofbrowse.Design
Single source of truth with an install-time eval addendum:
BROWSER_SKILL_SOURCE→BROWSE_SKILL_SOURCE, now pointing atpackages/cli/skills/browse/SKILL.md(built fromgetRepoRootDir(), matching the existingBROWSE_CLI_ENTRYPOINT-style constants).installBrowserSkill→installBrowseSkillnow reads the CLI skill and inserts a code-levelEVAL_HARNESS_ADDENDUMtemplate literal immediately after the YAML frontmatter (not appended at the end) before writing the combined file. The addendum:browseis preinstalled/pinned by the harness (nonpm install, no--local/--remote/--session— the wrapper injects those).browse ...command per Bash call (shell operators rejected by the harness).browsenetwork tools, and theEVAL_RESULTreporting format.Why prepend and not append:
isAllowedBrowseCommandonly checks that a Bash command starts withbrowseand has no shell metacharacters — it does not restrict whichbrowsesubcommand runs. So the addendum's "ignore cloud/functions/skills" instruction is the actual scope-enforcement mechanism, not just a courtesy note, and it needs to be read before the model encounters the CLI skill's concrete (and tempting) examples of those commands, not after. A live smoke run (see below) shows the model reaching forbrowse cloud fetchandbrowse skills findonce it got stuck on a bot-protected page — evidence this ordering concern is real, not theoretical.Skill name consistency: installed skill dir renamed
.claude/skills/browser/→.claude/skills/browse/to match the CLI skill's ownname: browsefrontmatter; all harness prompt/log references to "a project skill named browser" updated to "browse". Thestagehand_browserMCP server name (used by the unrelatedplaywright_code/cdp_codetool surfaces) is untouched.Deleted
packages/evals/skills/browser/SKILL.mdand the now-emptypackages/evals/skills/directory.Updated
packages/evals/tests/framework/claudeCodeToolAdapter.test.tsfor the renamed export/path/skill-name, plus new assertions that the installed file contains both the CLI-skill content and the addendum, with the addendum's string index before## Cloud APIs's index (regression guard against the addendum silently drifting back to append-at-end).Follow-up (review comment from ajmcquilkin): the hand-rolled regex in
insertAfterFrontmatterhad already needed a CRLF patch and still failed silently on BOM-prefixed files or a---line embedded in a YAML multiline string. Swapped it forgray-matter(newpackages/evalsdevDependency, private package, no changeset), but only for boundary detection —matter(markdown)locates where the frontmatter block ends; reassembly still uses the original raw string (markdown.slice(0, markdown.length - parsed.content.length)for the frontmatter,parsed.contentfor the body) rather thanmatter.stringify(), since that would re-serialize the YAML through js-yaml and reformat the shipped skill's frontmatter (e.g. its foldeddescription: >block).insertAfterFrontmatteris now exported and directly unit-tested.No changeset —
packages/evalsis private, eval-infra only.Overlap note: this touches
claudeCodeToolAdapter.ts; open PR #2299 also touches that file but in a different region (contract fix, not the skill-install path). Trivial rebase for whichever lands second.Linear: STG-2510
E2E Test Matrix
pnpm turbo run build --filter=@browserbasehq/stagehand --filter=browsethenpnpm --dir packages/evals buildbuild:esm/build:clicompleted successfully in<worktree>packages/cli/skills/browse/SKILL.md,packages/cli/dist/...).pnpm --dir packages/evals exec vitest run tests/framework/claudeCodeToolAdapter.test.tsTest Files 1 passed (1),Tests 17 passed (17)pnpm --dir packages/evals run test:unit(full evals suite)Test Files 48 passed (48),Tests 362 passed (362)browserskill name/path,installBrowserSkillexport, or the removed regex helper.node -escript importinginstallBrowseSkillfrom builtpackages/evals/dist/esm/framework/claudeCodeToolAdapter.js, run against a temp dir<temp dir>/.claude/skills/browse/SKILL.md; head matches CLI skill frontmatter (name: browse, full description); tail/body contains## Eval Harness Addendumpositioned before## Cloud APIs(indices 1054 vs 9018); frontmatter YAML block intact as the first bytes of the fileEVAL_CLAUDE_CODE_ALLOW_UNSANDBOXED_LOCAL=true EVAL_CLAUDE_CODE_MAX_TURNS=45 node packages/evals/dist/cli/cli.js run b:webtailbench -l 1 -t 1 -c 1 --harness claude_code -e local -m anthropic/claude-haiku-4-5-20251001(verbose logging on)Installed browse skill at <temp dir>/.claude/skills/browse/SKILL.md; agent then issuedbrowse open,browse snapshot,browse doctor,browse status,browse stop --force,browse cloud fetch,browse skills find— every one of them accepted by the harness with no "Only browse commands are allowed" / "Only Skill and Bash are allowed" contract denial anywhere in the log. Task itself endederror_max_turnsfighting a bot-protected United.com page (unrelated to this change)pnpm --dir packages/evals build(esm + cli) after addinggray-matteras a devDependency and rewritinginsertAfterFrontmatterto use it for boundary detection onlybuild:esm/build:clicompleted;dist/esm/framework/claudeCodeToolAdapter.jsshowsimport matter from "gray-matter"and the new exportedinsertAfterFrontmatterpackages/cli/skills/browse/SKILL.md.nodescript against the builtinstallBrowseSkill/insertAfterFrontmatter(not source), run in a temp dirfrontmatter byte-identical to source? true; text immediately after the frontmatter is only whitespace before## Eval Harness Addendum(no leftover body content);## Cloud APIsstill present after it; directinsertAfterFrontmattercalls for the no-frontmatter fallback and an embedded---inside a YAML multiline string both produced correctly-bounded outputmatter.stringify(), which this implementation deliberately avoids.claudeCodeToolAdapter.test.ts: LF/CRLF/BOM-prefixed frontmatter, a---line inside an indented YAML>block, no-frontmatter fallback, unterminated/invalid-YAML fallback (gray-matter throws; now caught), and a byte-identical-to-source frontmatter assertion viainstallBrowseSkillmatter.stringify().pnpm --dir packages/evals run lint(prettier + eslint + tsc)All matched files use Prettier code style!; eslint clean;tsc --noEmitcleanmatter.GrayMatterFile<string>) satisfy the package's strict TS config and formatting rules.Future work
A separate in-flight PR (
shrey/cli-skills-show, not yet on main) addsbrowse skills show(prints the bundled skill to stdout) plus a "Start here (for AI agents)" pointer inbrowse --help, so a real sandboxed agent with no eval scaffolding could self-discover the skill instead of having it handed to it.This PR's shape — inject the real skill + eval addendum into
.claude/skills/browse/at prepare time — stays the right default: it's the conventional eval pattern (benchmarks inject tool docs deterministically, the agent can't skip it), and it mirrors the actual supported CLI workflow (a user who already ranbrowse skills install). It also can't referencebrowse skills showtoday since that command doesn't exist on main yet.Once
browse skills showships, "agent discovers the skill itself viabrowse --help" becomes a good second, more sandbox-realistic eval arm (skill-injected vs. self-discovered A/B), not a replacement — worth noting for whoever builds it:isAllowedBrowseCommandin this file only checks for abrowseprefix and absence of shell metacharacters, sobrowse skills showalready passes that gate today with zero adapter changes needed for permissions. The one thing that arm would still need is its own way to deliver the eval-specific overrides (session/environment pinning, one-command-per-call, out-of-scope sections) —browse skills showwould print the bundled skill verbatim, so that arm likely wants the sameEVAL_HARNESS_ADDENDUMcontent delivered via the top-level task prompt instead of a pre-installed skill file.🤖 Generated with Claude Code
Summary by cubic
Switches the eval harness to install the real
browseskill frompackages/cli/skills/browse/SKILL.md, injecting a small eval-only addendum right after the frontmatter. Fixes stale guidance and keeps eval behavior aligned with the CLI (Linear STG-2510).Bug Fixes
browseskill frompackages/cli/skills/browse/SKILL.mdto stop drift and correct outdated docs/behavior.EVAL_HARNESS_ADDENDUMafter frontmatter to pin env/session, require onebrowsecommand per Bash call, and de-scope cloud/functions/templates/skills install.gray-matterto handle BOM/CRLF/embedded---, ensuring correct insertion across platforms.Refactors
installBrowserSkill→installBrowseSkill; install to.claude/skills/browse/; update prompts/logs to "browse". Keepstagehand_browserMCP name unchanged.packages/evals/skills/browser/SKILL.md.insertAfterFrontmatter, add unit tests for boundary cases and addendum-before-"Cloud APIs"; addgray-matteras a devDependency.Written for commit 486cab8. Summary will update on new commits.