Expose full GSD browser tool schema#130
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds browser reference capture and publishing, implements risk-based approval gating for sensitive browser tool operations, introduces a safety classification system to categorize tool risks, updates the browser service with a new Refs RPC pathway, and centralizes browser tool method taxonomy in TypeScript schemas. It also extends sidebar user input command support. Changes
Sequence Diagram(s)sequenceDiagram
participant BrowserManager
participant Service
participant CloudAPI as Cloud API
participant SessionOwner as Session Owner
rect rgba(70, 130, 180, 0.5)
Note over BrowserManager,CloudAPI: Tool invocation with sensitive-operation gating
BrowserManager->>BrowserManager: Tool(method, params) invoked
BrowserManager->>BrowserManager: classifyBrowserTool(method, params)
alt RiskRequiresApproval
BrowserManager->>BrowserManager: set owner -> OwnerApproval, bump controlVersion
BrowserManager->>CloudAPI: Emit BrowserSensitiveActionRequest (2m expiry)
BrowserManager->>SessionOwner: Await approval (external)
BrowserManager-->>BrowserManager: Return error (do not call Service.Tool)
else Allowed
BrowserManager->>Service: Tool(ctx, method, params)
Service-->>BrowserManager: Tool result
end
end
sequenceDiagram
participant BrowserManager
participant Service
participant CloudAPI as Cloud API
rect rgba(34, 139, 34, 0.5)
Note over BrowserManager,CloudAPI: Frame emission with refs publishing
BrowserManager->>CloudAPI: Emit BrowserFrame
BrowserManager->>Service: Refs(ctx, browserID) with 10s timeout
Service->>CloudAPI: cloud_refs RPC
CloudAPI-->>Service: version, refs, capturedAtMs
Service-->>BrowserManager: Refs {Version, Refs, CapturedAt}
BrowserManager->>CloudAPI: Emit BrowserRefs (normalized protocol.BrowserRef[])
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 7/10 reviews remaining, refill in 16 minutes and 45 seconds. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager.go`:
- Around line 482-513: The two branches that call m.sender.Send (the vault_save
rejection branch and the approval branch inside the
browserRiskRequiresApproval(risk) path) ignore send errors and leave the browser
state changed (state.owner = OwnerApproval and state.controlVersion++ in the
approval path), so detect and handle failures from m.sender.Send: capture the
error returned by m.sender.Send in both branches, and if it fails roll back the
state changes (reset state.owner from OwnerApproval and decrement/restore
state.controlVersion as applicable), ensure m.mu is unlocked consistently, and
return the send error (or a wrapped error) instead of suppressing it; locate
these changes around the m.sender.Send calls and the state mutation in the
browserRiskRequiresApproval branch and in the vault_save branch to implement the
rollback and proper error return.
In `@internal/browser/safety.go`:
- Around line 35-65: classifyBrowserBatch currently treats JSON unmarshal
failures and steps missing a method/action as low-risk (BrowserRiskInspection);
change it to fail closed by returning/upgrading to BrowserRiskExternalEffect in
those cases: when json.Unmarshal(params, &payload) returns an error, return
BrowserRiskExternalEffect; when iterating steps if method=="" (after using
stringValue) treat that step as BrowserRiskExternalEffect (i.e., set nested =
BrowserRiskExternalEffect rather than skipping) so riskRank logic will escalate
the batch appropriately; keep using classifyBrowserTool and riskRank for normal
cases.
🪄 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 Plus
Run ID: 52fe8c97-c468-49d8-bd75-fc3838c476d9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modinternal/browser/manager.gointernal/browser/manager_test.gointernal/browser/safety.gointernal/browser/service.gointernal/browser/types.gointernal/loop/daemon_test.gointernal/pi/extension/browser-methods.tsinternal/pi/extension/browser-tool.test.mjsinternal/pi/extension/index.ts
Summary
gsd_browserschema to the categorized cloud browser method registry.github.com/gsd-build/protocol-gotov0.33.0.Verification
go test ./...npm testfrominternal/pi/extensionDependency
v0.33.0, tagged from Add browser refs and command input protocol protocol-go#42.Summary by CodeRabbit
New Features
Tests
Chores