feat: opt-in browser tool family (chromedp via EgressProxy) (#94)#260
Conversation
initializ-mk
force-pushed
the
feat/browser-tools
branch
from
53382ee to
0b7de21
Compare
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: opt-in browser tool family
The architecture is well designed: conditional registration gated three-ways (decision fn / NewManager / RegisterTools, all fail-closed), all Chromium traffic forced through the EgressProxy with --proxy-bypass-list=<-loopback> closing Chrome's loopback bypass, token-optimized indexed digests with generation-counter staleness recovery, and path-only screenshot results. The chromium-gated e2e suite is the best part of the PR — proxy-routed navigation, protected fields, stale-gen recovery, native input events, pagination, blocked-domain-as-policy-error.
I'd hold merge for the five Important findings posted as inline comments: two security regressions (the name-gated arbitrary file read in loop.go, and the agent-wide OR of allow_sensitive_fill) and three where the PR's own guarantees don't hold (the bundled web-browse skill's non-functional $BROWSE_ALLOWED_DOMAIN egress placeholder, unvalidated capability names silently recreating the no-chromium image, and the incomplete sibling of the fixed early-return bug in skills_stage.go). All five are small, targeted fixes. Two Medium hardening items are also inline (runtime fail-open on missing Chromium; page-world window.__forge_* trust boundary).
Minor / nits (no inline anchor)
file_createbehavior change (loop.go): empty-contentfile_createno longer attaches an artifact (old code attached zero-byte files whenfilename != ""), and an unparseable-but-largefile_createresult now falls into the auto-attachdetectFileTypebranch it structurally never hit before. Pin or restore.- Ubuntu image regression (
image-registry.yaml):apt: chromium-browser→chromiumis right for Debian bookworm, but on Ubuntu baseschromiumis a snap transitional stub that fails in Docker; the registry already models this distinction for playwright (requires_ubuntu) but not here. - Chromium dedup is exact-name only (
requirements_stage.go): a skill declaringchromium-browserorgoogle-chromeas a bin plus the capability installs two browsers. - Analyzer: no test pins that a Critical violation fails the report (
reportAccumulator.addfolding criticals intototalErrorsis the load-bearing enforcement and is untested);capabilities: [""]yields a nonsensecapability:+3 factor;PolicySummaryhas no separate criticals count. extract.jsclean(): char class[ \t ]+has a duplicate space — likely meant\u00A0(nbsp).- Orphaned profiles:
.forge-browser-*temp dirs under WorkDir leak on SIGKILL; consider sweeping stale ones at Manager init. - Test heuristic (
registry_embedded_test.go): skipping the## Tool:assertion whenever raw content contains the substringcapabilities:lets non-browser skills escape the check; parse the frontmatter instead.
Test coverage
Strong overall: both early-return regressions have direct tests, the registration gate is table-tested, and the artifact/pagination/digest layers are unit-tested. Gaps: nothing pins path confinement for artifact reads, nothing asserts the cross-skill allow_sensitive_fill aggregation semantics (the current OR isn't even pinned), the root-SKILL.md synthesizeInstructional branch is untested, and the analyzer critical-fails-report path is untested.
Verdict
Hold for the five Important findings — each is a small fix. Everything else can be follow-ups. Happy to re-review once addressed.
| } | ||
| data := []byte(fc.Content) | ||
| if len(data) == 0 && fc.Path != "" { | ||
| fileBytes, err := os.ReadFile(fc.Path) |
There was a problem hiding this comment.
Important (security): unconfined os.ReadFile gated only by tool name.
This reads whatever path appears in the result JSON of any tool named browser_screenshot — but the dispatch is name-based, not identity-based. On an agent where the builtin is NOT registered (no browser capability), a skill-script tool named browser_screenshot — a name the parser accepts — can return {"filename":"x.png","path":"/agent/.forge/secrets.enc"} and the loop reads that file and attaches it as a channel artifact. That's a new read-any-file→channel exfiltration primitive that file_create (inline bytes) never had; TestFileArtifactFromToolResult_BrowserScreenshotPath even demonstrates reading an arbitrary absolute path.
Fix: confine fc.Path to the known screenshot directories (FilesDir / <WorkDir>/.forge-browser/shots) before reading, or dispatch on the registered builtin's identity rather than the name — and add a test pinning that an out-of-tree path is rejected.
| } | ||
| // OR across skills: any one opting in enables | ||
| // sensitive fill for the agent's browser. | ||
| if gc.Browser != nil && gc.Browser.AllowSensitiveFill { |
There was a problem hiding this comment.
Important (security): allow_sensitive_fill is OR'd across ALL skills — cross-skill privilege escalation, contradicts the documented per-skill opt-in.
docs/reference/browser-tools.md and the PR body say password/payment fill is refused "unless the skill opts in." Here the opt-in is OR'd across every skill entry — including skills that never declared the browser capability (this comment says so explicitly). Installing any unrelated skill carrying guardrails: {browser: {allow_sensitive_fill: true}} silently enables password-filling for a browser granted by a different skill.
Compounding it, the analyzer assigns no risk factor or warning to allow_sensitive_fill: true, so a skill audit won't surface it.
Fix: honor the opt-in only from entries that also declare CapabilityBrowser (DeriveBrowserConfig already walks entries, so this is a natural place), and add an analyzer factor/warning for the opt-in.
| capabilities: | ||
| - browser | ||
| egress_domains: | ||
| - "$BROWSE_ALLOWED_DOMAIN" |
There was a problem hiding this comment.
Important: this egress placeholder is non-functional as shipped.
Nothing in this PR (or the existing egress model — allowlist entries are exact/wildcard domains) expands environment variables in egress_domains. The literal string $BROWSE_ALLOWED_DOMAIN becomes an allowlist entry matching no real host, so the flagship embedded skill registers the browser tools and then every navigation is egress-blocked — while the Notes section below instructs users to "set $BROWSE_ALLOWED_DOMAIN", which has no effect.
Fix: ship a commented "edit this list" placeholder instead, or implement (and document + test) env expansion for egress_domains. Either way, the registry test should assert the intended contract.
| if e.ForgeReqs == nil { | ||
| continue | ||
| } | ||
| for _, c := range e.ForgeReqs.Capabilities { |
There was a problem hiding this comment.
Important: capability names are never validated — a typo silently recreates the bug this PR headline-fixes.
capabilities: [browsr] parses fine, aggregates here as browsr, DeriveBrowserConfig returns nil, the RequirementsStage injects no chromium, and the analyzer scores it a generic +3 with no warning. Net effect: the agent builds an image without Chromium and starts without browser tools, with zero diagnostics at parse, build, or audit time — exactly the "browser agent without chromium" failure the early-return fixes were for.
The parser already validates enums elsewhere (validateCategoryAndTags), so the pattern exists. Fix: validate against the known capability set at parse time (error, or at minimum a SkillsStage stderr warning), and/or an analyzer unknown_capability violation instead of the silent +3.
| // chromium install in the requirements stage. | ||
| reqs := requirements.AggregateRequirements(entries) | ||
| if len(reqs.Bins) > 0 || len(reqs.EnvRequired) > 0 || len(reqs.EnvOneOf) > 0 || len(reqs.EnvOptional) > 0 { | ||
| if len(reqs.Bins) > 0 || len(reqs.EnvRequired) > 0 || len(reqs.EnvOneOf) > 0 || len(reqs.EnvOptional) > 0 || len(reqs.Capabilities) > 0 { |
There was a problem hiding this comment.
Important: this gate still drops egress-only / guardrails-only instructional skills — the fix is incomplete for the exact bug class it targets.
Adding len(reqs.Capabilities) > 0 admits capability-only aggregations, but the gate still omits EgressDomains and SkillGuardrails — the very fields synthesizeInstructional's comment says it exists to preserve ("capabilities, egress_domains, guardrails"). An instructional skill declaring only egress_domains (or only deny_output) gets synthesized and aggregated, then bc.SkillRequirements is left nil, so its domains/guardrails never reach downstream build stages (e.g. the egress allowlist stage). This is the sibling of the bug the PR fixes — and the runtime side got it right (validateSkillRequirements checks egress/denied-tools/capabilities/phases).
Fix: mirror the runtime condition, e.g. add || len(reqs.EgressDomains) > 0 || len(reqs.DeniedTools) > 0 || reqs.SkillGuardrails != nil.
| if r.derivedBrowserConfig != nil { | ||
| binPath, resErr := browser.ResolveBinary() | ||
| if ok, reason := browserRegistrationDecision(r.derivedBrowserConfig, binPath, resErr, proxyURL); !ok { | ||
| r.logger.Error("browser capability declared but browser tools not registered", map[string]any{ |
There was a problem hiding this comment.
Medium (consistency): missing Chromium/proxy logs an Error and keeps serving — weaker than the requires.bins posture.
Missing required bins fail startup validation; a declared browser capability with no Chromium (or a proxy that failed to start) logs and continues, leaving the LLM with a skill that instructs browser use and no tools to do it. Defensible for dev ergonomics, but it's inconsistent with the analogous bins path and with this PR's own fail-closed framing. Consider failing startup (matching bins semantics), or gating the leniency on dev mode.
| // Side effects: the index → element map and helpers the interaction | ||
| // tools use. A navigation wipes these, which is exactly the staleness | ||
| // signal (window.__forge_gen becomes undefined). | ||
| window.__forge_els = els; |
There was a problem hiding this comment.
Medium (hardening): all interaction state lives in the page's main JS world.
window.__forge_els/_gen/_center/_protected/_select_* are page-world globals. A hostile page can spoof digests (feeding the LLM attacker-controlled "observations" attributed to tooling), redirect click coordinates, or defeat the live-DOM sensitive-fill re-check — and a merely-defensive page that freezes/clobbers globals breaks the tools accidentally. The sensitive-fill bypass is low-value to an attacker (the page receives typed text regardless of field type), but digest spoofing is a real prompt-injection amplifier.
Consider evaluating in an isolated world (Page.createIsolatedWorld / chromedp's isolated evaluate) — element refs can live there while clicks still dispatch by coordinate via trusted CDP input. At minimum, document the trust boundary: digests are page-controlled content.
| // when large. cli_execute output is raw command JSON; browser_* outputs are | ||
| // page digests/extracts. | ||
| func isIntermediateOutputTool(toolName string) bool { | ||
| return toolName == "cli_execute" || strings.HasPrefix(toolName, "browser_") |
There was a problem hiding this comment.
Nit: strings.HasPrefix(toolName, "browser_") also swallows any future or skill-defined tool that happens to be named browser_*, suppressing large-output auto-attach for it. Prefer the explicit six-name set, mirroring artifactEmittingTools.
|
Thanks for the thorough review. All five Important findings are fixed in Important (all fixed)
Also addressed
Deferred to follow-ups (noted, not in this change set)
|
…Proxy M0 of the opt-in browser tool family (#94). Adds forge-cli/tools/browser with a Manager that lazily launches one Chromium per agent: - --headless=new, throwaway user-data-dir under WorkDir, no persisted cookies/profile across runs - --proxy-server=<EgressProxy> with --proxy-bypass-list=<-loopback> so even loopback traffic is proxied (Chrome bypasses proxies for localhost by default, which would be an egress escape hatch) - fail-closed: NewManager refuses an empty ProxyURL - first chromedp.Run receives the undecorated tab context (a timeout-derived context would tie the browser lifetime to its cancel) - chromedp pinned to v0.14.1: v0.15+ requires go 1.26, CI is on 1.25 Spike tests prove the risky chain end-to-end against a real EgressProxy and httptest server: loopback traffic observed via proxy OnAttempt, non-allowlisted HTTPS domain refused at CONNECT with ERR_TUNNEL_CONNECTION_FAILED, profile removed on Stop. Tests skip via binary probe (never GOOS) when no chromium is installed.
…families M1 of the opt-in browser tool family (#94): - SkillRequirements.Capabilities: skills declare opt-in runtime capabilities (requires.capabilities: [browser]) distinct from bins — a capability asks the runner for a conditional tool family, not a binary on PATH - ForgeSkillMeta.TrustHints: trust_hints (network/filesystem/shell) get a typed representation so the analyzer can check consistency (e.g. browser capability + explicit network: false). Network/Shell are pointer-bools because absence and explicit false are different statements; Filesystem stays a mode string per existing skills - AggregatedRequirements.Capabilities: sorted union across skills - DerivedBrowserConfig + DeriveBrowserConfig: non-nil iff any active skill declares the browser capability; names declaring skills for startup errors. No merge semantics — capability presence is binary - parser.ExtractForgeMeta: full typed forge-namespace extraction; ExtractForgeReqs is now a thin wrapper over it - Makefile/CI: forge-skills joins MODULES and the vet/fmt/test/lint paths — it was silently missing from the default test loop
M2 of the opt-in browser tool family (#94). The LLM never sees raw HTML: every observation is an indexed digest (~1-3KB vs 50-200KB), and interactions go by index instead of CSS selectors, so no round trips are spent composing selectors. - snapshot.js (go:embed): walks the DOM incl. shadow roots and same-origin iframes, filters to visible interactive elements, stores live refs in window.__forge_els, returns url/title/elements/text. Marks password + payment (autocomplete cc-*/…-password) inputs as fill-protected - generation counter: each snapshot carries a generation; click/fill require it and get ErrStale + a fresh digest when the page changed — the LLM recovers in one turn instead of clicking the wrong element - act-returns-state: navigate/click/fill all return the new digest, collapsing observe-act-observe into one round trip - click = fresh viewport coords + trusted CDP Input.dispatchMouseEvent; fill = click-focus, select-all, Input.insertText (native input events — verified against a JS input listener in the e2e), change dispatch, optional Enter; selects pick options by label - browser_extract: markdown-ish text / deduped links / selector-scoped HTML with offset pagination (default 16k chars); full-page HTML mode is refused - browser_screenshot: PNG written to the agent files dir, path-only JSON result — zero tokens of image data in context - every result self-caps at 7500 chars, under forge-core's 8000 auto-attach threshold and far under loop-level truncation - egress denials surface as policy errors, not flakiness Tests: pure-Go digest rendering/caps, plus a chromium-gated e2e through a real EgressProxy covering the full flow (skips when no browser).
…lity M3 of the opt-in browser tool family (#94). Registration mirrors the cli_execute conditional path: iff an active skill declares requires.capabilities: [browser] AND a Chromium binary resolves AND the egress proxy is up, the six browser_* tools register; otherwise an actionable error names the reason and the declaring skills. denied_tools removal covers browser_* by name with no extra wiring. - proxy force-start: the browser capability starts the EgressProxy even in-container / dev-open mode — browser traffic never bypasses it (in dev-open the matcher is allow-all, so it is a pass-through with audit) - fail closed: no proxy → no browser tools, logged at Error level - browserRegistrationDecision extracted as a pure, table-tested gate - defer mgr.Stop() ordered LIFO before egressProxy.Stop() Two latent bug fixes in validateSkillRequirements: - capability/egress-only skills (zero bins/env) hit an early return that silently dropped their egress_domains and denied_tools before any derived config was stored; the early return now derives first - instructional skills (no '## Tool:' entries) contributed no forge metadata at all; a metadata-only entry is now synthesized so their capabilities/egress/guardrails aggregate (tool registration untouched)
…ts from auto-attach M4 of the opt-in browser tool family (#94). The loop's artifact handling was hardcoded to the file_create tool name and inline-content shape. - fileArtifactFromToolResult: extracted artifact routing covering file_create (bytes inline in content, unchanged behavior) and browser_screenshot (bytes read from path — PNG data never enters the LLM conversation, only the path-bearing confirmation JSON does) - isIntermediateOutputTool: browser_* joins cli_execute in the >8000 char auto-attach exclusion — digests and extracts are intermediate observations; attaching them as files makes the LLM say 'see attached' instead of answering
M5 of the opt-in browser tool family (#94). The skill guardrail engine was hardcoded to the cli_execute tool name; the two gates are now: - guardrailInputExtractors: per-tool extractors producing the string deny_commands match against — cli_execute keeps extractCommandLine byte-identical; browser_navigate exposes the url (skills can constrain navigation targets), browser_extract the selector, browser_fill 'fill[i] <text>'. Tools without checkable input skip the check - guardrailOutputChecked: deny_output block/redact now scrubs cli_execute output AND browser_* digests/extracts, closing the gap where a page could exfiltrate secrets past skill-scoped redaction Sensitive-fill opt-in plumbing (enforced in browser_fill, not the engine): guardrails.browser.allow_sensitive_fill in SKILL.md → SkillGuardrailConfig.Browser → OR across skills in AggregateRequirements → DerivedBrowserConfig.AllowSensitiveFill. Existing guardrail test suite passes untouched; new tests cover navigate/fill deny patterns, digest redaction, and the opt-in flow.
…guardrail-gap rules M6 of the opt-in browser tool family (#94). - scoreCapabilities: browser capability scores +15 (high-risk, on par with a high-risk binary; additive with them); unknown capabilities +3 - Rule 7 capability_trust_conflict (critical): browser capability + explicit trust_hints.network:false is a contradiction — browsing requires network. Absent (nil) network hint does not conflict - Rule 8 capability_guardrail_gap (warning): browser skill with no guardrails.deny_output returns extracted page content unredacted - 'critical' is a new severity: FormatText renders it as CRIT and it counts as a policy failure (Passed=false) alongside error - scanner + entryToDescriptor populate SkillDescriptor.Capabilities / TrustHints / HasDenyOutput via the typed ExtractForgeMeta - GenerateReport (registry path) now analyzes descriptors directly instead of round-tripping through entries, which dropped the new typed fields; shared reportAccumulator dedups the aggregation
M7 of the opt-in browser tool family (#94). - requirements stage injects a synthetic chromium BinRequirement (origin capability:browser) when a skill declares the browser capability, so the smart Dockerfile installs it — resolved via the well-known image registry. Only browser agents get it; the browser stays optional. No duplicate when a skill also lists chromium as an explicit bin - skills stage stores AggregatedRequirements when only capabilities are present (same early-return class as the runtime fix in M3), else a capability-only browser skill reached the requirements stage empty - image-registry: chromium apt package corrected to 'chromium' for the debian:bookworm-slim base ('chromium-browser' is Ubuntu's transitional name and does not exist on Debian) --prod dev-open rejection already exists in export.ValidateProdConfig; no change needed there.
M8 of the opt-in browser tool family (#94). - forge-skills/local/embedded/web-browse: bundled reference skill — declares requires.capabilities: [browser], example egress_domains, trust_hints.network: true, a deny_output secret pattern, and a body teaching the digest-index workflow. Instructional (no '## Tool:' script entries — the browser tools are runtime-registered) - docs/reference/browser-tools.md: activation, digest model, tool reference, egress/guardrail/audit/packaging behavior, env vars, troubleshooting - environment-variables reference: FORGE_BROWSER_BIN / FORGE_BROWSER_HEADLESS - CHANGELOG entry - embedded-registry test: expect 16 skills incl. web-browse; capability skills are exempt from the '## Tool:' heading assertion Verified end-to-end: forge skills list shows web-browse; forge skills audit reports the +15 capability factor with no violations, and a browser + trust_hints.network:false fixture is flagged Critical.
- drop now-unused toAnySlice in analyzer/report.go (the M6 descriptor refactor removed its only caller) - check fmt.Fprintf return in the browser e2e test server - simplify a negated boolean condition per staticcheck QF1001
Build-tagged 'manual' (excluded from CI). Drives the real Manager against a live site and prints the indexed digest and extract the LLM would see — lets the token-optimized output and egress enforcement be eyeballed without an LLM or API key. Configurable via DEMO_URL and FORGE_BROWSER_HEADLESS.
The build's skill scanner dropped SKILL.md files with forge metadata but no '## Tool:' entries (instructional/capability-only skills), so a browser skill's capability never reached the requirements stage and no chromium was injected into the generated Dockerfile — images shipped without a browser despite declaring the capability. Mirror the runtime's validateSkillRequirements handling (M3): when a SKILL.md parses to zero tool entries but carries forge metadata, synthesize a metadata-only entry so its capabilities/egress aggregate. Applied to both the root SKILL.md and skills/ subdirectory scan paths. Verified end-to-end: forge build on a capability-only browser agent now emits 'apt-get install ... chromium'; a non-browser agent does not.
CI images ship a chromium that exists but cannot start in the runner's
restricted sandbox ('chrome failed to start'), so the presence-only skip
guard let the launch tests run and fail. requireChromium now probes an
actual launch once (real Manager path, same flags) and skips the
browser-launching tests when it fails — so they run wherever a browser
truly works and skip where it does not. Pure-Go tests (digest, pagination,
registration) are unaffected and still run everywhere.
Also add --disable-dev-shm-usage: /dev/shm is small or absent in
containers and CI runners, and without it Chrome crashes on startup.
Confine screenshot artifact reads (security): the loop read any path from a browser_screenshot-named result, so a skill-script tool using that name on a non-browser agent could exfiltrate an arbitrary file to a channel. Reads are now confined to the agent files directory; file_create is restored to inline-bytes-only (including its historical zero-byte attach), and the large-output auto-attach exclusion uses an explicit browser tool-name set instead of a 'browser_' prefix. Per-skill allow_sensitive_fill (security): the password/payment fill opt-in was OR'd across ALL skills, so any unrelated skill could enable it for a browser granted by a different skill. It is now honored only from a skill that itself declares the browser capability, and surfaces as an analyzer risk factor. web-browse egress placeholder: the reference skill shipped a '' entry with instructions to 'set' it. It now ships an empty, commented allowlist with explicit guidance. (Env vars ARE expanded in egress_domains via expandEgressDomains — the docs now say so accurately.) Validate capability names: an unknown capability (e.g. a 'browsr' typo) parsed fine, derived nothing, and installed no Chromium with zero diagnostics — the exact failure the plumbing prevents. The parser now rejects unknown capabilities, surfacing the error at build/runtime. Complete the skills_stage instructional gate: it admitted capability-only skills but still dropped egress-only / guardrails-only ones, stripping their domains/guardrails from the build. Now mirrors the runtime condition. Also: analyzer critical-fails-report test (pins that a Critical trust conflict fails the audit), extract.js nbsp char class made an explicit \u00A0 escape, and the snapshot.js page-world trust boundary is documented (isolated-world evaluation tracked as a follow-up).
initializ-mk
force-pushed
the
feat/browser-tools
branch
from
6158497 to
6519c87
Compare
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review of 6519c87 — all five Important findings fixed, CI green
All 10 checks pass including integration. Verification per finding:
1. Path confinement (file-read exfil) — properly fixed. confinedFilesPath requires a non-empty files dir from context, resolves both paths to absolute, and rejects escapes via filepath.Rel + .. prefix check. TestFileArtifactFromToolResult_ScreenshotPathConfinement pins the exact attack from the review: absolute path outside the files dir, parent traversal, /etc/passwd, and the no-files-dir case. file_create is restored to inline-bytes-only including the historical zero-byte attach, and the unparseable-large-file_create fall-through is fixed by excluding artifact-emitting tools from the auto-attach branch.
Two tiny residuals, both non-blocking:
- No
filepath.EvalSymlinks— a symlink inside the files dir pointing outside would pass the confinement check (requires an attacker who can already create symlinks there). - The screenshot tool's fallback dir (
<WorkDir>/.forge-browser/shots, used when no files dir is on the context) is now a location the loop will never attach from — screenshots written there are saved but silently never attached. Worth a one-liner to align (skip the fallback and tell the LLM, or confine to either dir).
2. Cross-skill allow_sensitive_fill — fixed at the right layer. The opt-in is read per-entry inside DeriveBrowserConfig's capability-filtered loop, so only a skill that itself declares the browser capability can enable it; the agent-wide aggregation was removed with an explanatory NOTE. The +10 analyzer risk factor is wired into both scoring paths plus the scanner/policy descriptors. TestDeriveBrowserConfig_NoCrossSkillEscalation pins the exact scenario from the review.
3. web-browse placeholder — fixed, and the review gets a correction. The skill ships an empty, commented allowlist with accurate guidance. The commit rightly corrects the review's claim: expandEgressDomains in forge-cli/runtime/runner.go (pre-existing, outside this PR's diff) does expand $VAR/${VAR} at runtime — verified via code search. The actual bug was only the misleading placeholder + docs, now accurate. Registry test now decides from parsed descriptors, also resolving the substring-heuristic nit.
4. Capability validation — fixed at parse time. validateCapabilities rejects unknown names with an error listing the known set, so build, runtime, and analyzer all surface a browsr typo. Positive + negative parser tests added. Bonus: capabilities: [""] is now rejected at parse, resolving the nonsense-factor nit implicitly.
5. skills_stage gate — completed. Mirrors the full runtime condition (capabilities, egress, denied tools, workflow phases, guardrails), with an egress-only instructional-skill regression test.
Nits taken: \u00A0 in extract.js, explicit browserToolNames set (with a browser_export_report negative test), analyzer critical-fails-report test, registry heuristic.
Remaining asks (non-blocking)
- The snapshot.js trust-boundary comment (which is accurate and well done) says isolated-world evaluation is "a tracked hardening follow-up" — no such issue exists in the tracker yet. Please file it so the comment is true.
- The two Medium findings got no change or reply: runtime fail-open on missing Chromium (vs. the
requires.binsfail-startup posture), and the Ubuntuchromiumapt-name regression. A one-line "declined because X" or a follow-up issue is fine for each. - Untracked minors: exact-name chromium dedup, orphaned
.forge-browser-*profile dirs on SIGKILL,PolicySummarycriticals count.
Verdict
The hold-merge set is fully resolved with regression tests. Ready to merge from my side once the promised isolated-world follow-up issue is filed.
…ndary comment The isolated-world hardening the comment mentions is now tracked as #290; cite it so the comment is concrete.
|
Thanks for the re-review. Filed the follow-ups you asked for, and cited the tracking issue in the code comment so it's concrete. Merge gate (isolated-world)
The two Mediums
Minors (incl. the two residuals you flagged on the path-confinement fix)
All non-blocking and out of this PR's scope; happy to pick any up next. Nothing else changed on the branch except the one-line comment citing #290 — CI is re-running on |
…ects (#276 re-review) Two CI problems the re-review flagged, plus the optional parser hardening: 1. CI was structurally blind to forge-ui. The vet / fmt / test / lint / integration steps listed forge-core/cli/skills/plugins but not ./forge-ui/... (a separate workspace module needing explicit listing), so this PR's ~300 lines of parser/handler tests never ran — a green check certified nothing about the code. Added ./forge-ui/... to every step (+ a `go mod verify` for it). Same class of gap #260 closed for forge-skills. (Finding 1 — the workflow never *triggering* — was already fixed by the earlier amend+force-push; this makes the run actually exercise forge-ui.) 2. Parser hardening: a wrapper object like `{"response": {"message":…, "skill":…}}` passed the both-keys substring guard but unmarshalled to a zero-value envelope (unknown top-level field ignored) and, because we advanced past the WHOLE object, the inner real envelope was never tried. Now: reject zero-value envelopes (message=="" && skill==nil → continue), and advance by start+1 (just inside the failed object) so nested candidates are reachable. jsonObjectAt returns the opening-brace index instead of the end. Tests: wrapper-reaches-inner-envelope and non-envelope-wrapper-falls- back-to-legacy. Verified locally with the exact CI command set including ./forge-ui/...: vet, gofmt -l, go test, and golangci-lint all clean.
…ects (#276 re-review) Two CI problems the re-review flagged, plus the optional parser hardening: 1. CI was structurally blind to forge-ui. The vet / fmt / test / lint / integration steps listed forge-core/cli/skills/plugins but not ./forge-ui/... (a separate workspace module needing explicit listing), so this PR's ~300 lines of parser/handler tests never ran — a green check certified nothing about the code. Added ./forge-ui/... to every step (+ a `go mod verify` for it). Same class of gap #260 closed for forge-skills. (Finding 1 — the workflow never *triggering* — was already fixed by the earlier amend+force-push; this makes the run actually exercise forge-ui.) 2. Parser hardening: a wrapper object like `{"response": {"message":…, "skill":…}}` passed the both-keys substring guard but unmarshalled to a zero-value envelope (unknown top-level field ignored) and, because we advanced past the WHOLE object, the inner real envelope was never tried. Now: reject zero-value envelopes (message=="" && skill==nil → continue), and advance by start+1 (just inside the failed object) so nested candidates are reachable. jsonObjectAt returns the opening-brace index instead of the end. Tests: wrapper-reaches-inner-envelope and non-envelope-wrapper-falls- back-to-legacy. Verified locally with the exact CI command set including ./forge-ui/...: vet, gofmt -l, go test, and golangci-lint all clean.
1 participant
Closes #94.
Adds an opt-in browser tool family —
browser_navigate,browser_state,browser_click,browser_fill,browser_extract,browser_screenshot— that lets an agent drive a real headless Chromium (viagithub.com/chromedp/chromedp) to navigate, read, click, fill, and screenshot web pages. It follows thecli_executeprecedent: a first-class conditional tool family registered by the runner only when an active skill declaresrequires.capabilities: [browser]and a Chromium binary is present. Agents without a browser-requiring skill never register the tools, never start the proxy for the browser, and never launch Chromium.Token-optimized by design
The LLM never sees raw HTML. Every observation is a compact indexed digest (title, URL, numbered interactive elements, start of page text) built by an injected
snapshot.js, and interactions go by element index + a generation counter, not CSS selectors. A complex page is ~1–3 KB instead of 50–200 KB, and every action returns the new state — collapsing observe→act→observe into one round trip. A stale generation returns an error with a fresh digest, so the model recovers in one turn. Screenshots are written to disk and attached as channel artifacts (path-only result JSON), so image bytes never enter the conversation.Security
EgressProxy(same allowlist, SSRF IP validation, DNS-rebinding protection). Chromium runs with--proxy-server+--proxy-bypass-list=<-loopback>so even localhost is proxied; the proxy is force-started for browser agents even in-container / dev-open. Fail-closed: no proxy ⇒ no browser tools.deny_outputredaction and navigation/fill throughdeny_commands.browser_fillrefuses password/payment fields unless the skill opts in viaguardrails.browser.allow_sensitive_fill.browsercapability high-risk (+15) and flagsbrowser+trust_hints.network: falseas a Critical trust violation; a browser skill with nodeny_outputis a warning.Packaging
forge buildinjects a syntheticchromiumbin requirement only for browser-capability agents, so the image installs the Debianchromiumpackage solely when needed. A non-browser agent's image is unchanged.Notable review points
## Tool:entries — were silently dropped before their requirements aggregated. Without these fixes a browser agent would run without egress domains applied and build an image with no Chromium.Verification
make test,make vet,make lintgreen across all modules (forge-skillsnewly added to the default test loop, since it was silently untested).forge-cli/tools/browser/e2e_test.go) drives real Chromium through a realEgressProxy: navigate → protected-field marking → click-to-navigate → stale-generation recovery → fill fires native input events → select → submit → extract pagination → screenshot PNG → blocked-domain-as-policy-error. Skips cleanly when no browser is installed.browser_navigate, the proxy allowed the target and blocked Chrome's phone-home traffic, and the LLM summarized from the digest.web-browsereference skill; docs atdocs/reference/browser-tools.md.Env:
FORGE_BROWSER_BIN(binary override),FORGE_BROWSER_HEADLESS=false(headful for local debugging).🤖 Generated with Claude Code