Skip to content

chore: unship governance#959

Open
Nikhil (shadowfax92) wants to merge 3 commits intodevfrom
chore/unship-1
Open

chore: unship governance#959
Nikhil (shadowfax92) wants to merge 3 commits intodevfrom
chore/unship-1

Conversation

@shadowfax92
Copy link
Copy Markdown
Contributor

Summary

  • Removes the Governance app/sidebar surface, Tool Approvals, ACL Rules, and admin dashboard routes from the agent UI.
  • Deletes ACL/tool-approval storage, request payload fields, server ACL policy/scoring/guard services, and model-level tool approval wiring.
  • Keeps compatibility guard tests proving old governance request fields are omitted by the agent and stripped by the server.

Design

This unships governance by removing the feature surface and server enforcement path rather than hiding it behind flags. Browser tools now register through the plain tool framework again, chat requests no longer carry governance fields, and deleted ACL/tool-approval tests are replaced with narrow negative coverage for stale request data.

Test plan

  • git diff --check
  • bunx biome check on changed files
  • bun run --filter @browseros/agent typecheck
  • bun run --filter @browseros/server typecheck
  • bun run --filter @browseros/agent test
  • bun run --filter @browseros/server test:api
  • bun run --filter @browseros/server test:agent
  • bun run --filter @browseros/server test:tools

@shadowfax92
Copy link
Copy Markdown
Contributor Author

Greptile (@greptileai) review

@shadowfax92
Copy link
Copy Markdown
Contributor Author

Claude (@claude) review

@claude
Copy link
Copy Markdown

claude Bot commented May 7, 2026

Claude finished Nikhil (@shadowfax92)'s task in 3m 18s —— View job


Code Review: chore/unship-1

  • Gather context and understand PR scope
  • Review agent-side changes (UI, request building, session)
  • Review server-side changes (tools, services, routes)
  • Review test coverage changes
  • Post final review

Overall: Clean, well-scoped unship. No blocking issues. A few follow-up items worth tracking.

This is a substantial but clearly correct deletion. The surface area removed (Governance UI, ACL rules, tool approvals, admin dashboard, server-side ACL guard/scorer/embeddings) maps cleanly to the PR summary, and the compatibility guard tests fulfill the stated design intent of proving that stale request data is absent client-side and stripped server-side.


Compatibility guard tests — good coverage

The two new test files are well-targeted:

tests/api/types.test.ts — verifies ChatRequestSchema.parse() strips aclRules, toolApprovalConfig, and toolApprovalResponses. This is solid because Zod strips unknown keys by default on .parse(), so the assertion is actually enforced, not just documentary.

tests/agent/tool-adapter.test.ts — verifies buildBrowserToolSet no longer sets needsApproval on any tool. Clean.

buildChatRequestBody.test.ts test pattern note: the test injects governance fields via as Parameters<typeof buildChatRequestBody>[0] to call a function that no longer accepts them, then asserts they're absent from output. This works but will silently become vacuous if the type is ever narrowed to reject those fields at the call site (the cast will still compile). Consider replacing with // @ts-expect-error comments on the extra fields, which would fail if TypeScript begins rejecting them:

const body = buildChatRequestBody({
  conversationId: '...',
  provider,
  // @ts-expect-error — governance fields no longer in param type
  aclRules: [...],
  // @ts-expect-error
  toolApprovalConfig: { categories: { input: true } },
})

Dead code in @browseros/shared — follow-up needed

These three shared-package files are no longer imported by anything in apps/ after this PR, but were not deleted:

  • packages/browseros-agent/packages/shared/src/constants/tool-approval.tsToolApprovalCategoryId, ToolApprovalConfig, etc.
  • packages/browseros-agent/packages/shared/src/types/acl.tsAclRule, ElementProperties
  • packages/browseros-agent/packages/shared/src/acl/match.ts — full site-pattern / element matching logic

If there are no external consumers of the shared package that depend on these, they should be removed in a follow-up. Keeping dead types here creates confusion about whether ACL functionality is actually gone. Fix this →


Stale tool-approval-request case in compaction utils

packages/browseros-agent/apps/server/src/agent/compaction/utils.ts still contains (lines 175–177):

case 'tool-approval-request':
  chars += part.approvalId.length + part.toolCallId.length
  break

And around line 200 there's another part.approvalId reference. Since nothing in the codebase generates tool-approval-request parts anymore, this is dead code. It won't cause a runtime error (it'll simply never match), but it's misleading. Worth cleaning up in a follow-up. Fix this →


output-denied state — intentional retain?

ToolInvocationState still includes 'output-denied', and the UI handles it (ToolBatch.tsx, tool.tsx, ExecutionStepItem.tsx). The server-side compaction/content.ts also has 'execution-denied'. If this state is still producible through a non-governance path (e.g., MCP tool denial or future use), the retain is correct. If it's also dead code post-governance, it should be removed. The PR as written doesn't clarify this — a comment or follow-up ticket would help.


Structural correctness — looks good

  • defineTool no longer takes approvalCategory — all browser tools correctly updated to use defineTool directly. The defineToolWithCategory helper is gone.
  • AiSdkAgent constructor correctly loses toolContext as a private field (it was only needed for updateAclRules() mutation, which is now removed).
  • Session rebuild on approvalConfigKey change is correctly removed from chat-service.ts.
  • App.tsx routing: /audit, /observability, /executions now unconditionally redirect to /home instead of conditionally to /admin. Correct.
  • browser.ts: resolveElementProperties and highlightBlockedElement removed — these were ACL-only CDP calls, good.
  • useChatSession.ts: aclRulesStorage, toolApprovalConfigStorage, approvalResponsesStorage no longer read on each message send. Correct.

Verdict

No blocking issues. The PR achieves a clean hard-delete of governance rather than a flag-hide. The follow-ups (shared package dead code, stale compaction case) are cosmetic but worth tracking so the codebase doesn't carry misleading stale code long-term.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 7, 2026

Greptile Summary

This PR removes the governance feature surface in its entirety — ACL rules, tool approvals, the admin dashboard, and all related server-side enforcement — across 80 files (~4 000 lines deleted, ~155 added). Compatibility guard tests are added to confirm that stale governance fields are absent from agent requests and stripped by the server's Zod schema.

  • Agent side: useChatSession removes the approval-sync storage effects, the sendAutomaticallyWhen resume trigger, and the aclRules/toolApprovalConfig fields from every chat request body; governance UI pages and sidebar routes are deleted.
  • Server side: ChatRequestSchema drops aclRules, toolApprovalConfig, and toolApprovalResponses; the ACL guard hook in executeTool, the needsApproval flag on every browser tool, approval-config change detection in ChatService, and the /acl-rules HTTP route are all removed.
  • Tests: Deleted ACL scorer, fixture runner, and approval-flow tests are replaced with two narrow negative-coverage tests proving the old fields are neither sent by the agent nor accepted by the server.

Confidence Score: 4/5

Safe to merge — the governance removal is thorough and self-consistent, with new tests guarding against accidental re-introduction of old request fields.

The deletion is wide (80 files) but mechanically straightforward: every reference to ACL rules, tool approvals, and the admin dashboard is traced and removed, the Zod schema strips unknown governance fields at the boundary, and two new tests lock in the expected absence of those fields. The only loose end is the output-denied state, still modelled in ToolInvocationState and handled in ToolBatch.tsx and normalize.ts, even though no remaining code path can produce it.

getMessageSegments.ts, ToolBatch.tsx, execution-history/types.ts, and normalize.ts each retain handling for output-denied, a state that can no longer be reached after removing the approval flow.

Important Files Changed

Filename Overview
packages/browseros-agent/apps/server/src/tools/framework.ts Removes approvalCategory, defineToolWithCategory, and the ACL guard hook from executeTool. defineTool interface is simplified cleanly.
packages/browseros-agent/apps/server/src/agent/tool-adapter.ts Removes getApprovedBrowserToolNames, the approvalConfig parameter, and needsApproval from every built tool. Straightforward cleanup.
packages/browseros-agent/apps/server/src/api/services/chat-service.ts Removes toolApprovalConfig key tracking, approval-config change detection, applyToolApprovalResponses, and the associated tool-approval resume path. Chat handling logic is now simpler.
packages/browseros-agent/apps/server/src/api/types.ts Drops aclRules, toolApprovalConfig, and toolApprovalResponses from ChatRequestSchema. Unknown extra fields are silently stripped by Zod, so old clients sending these fields won't break.
packages/browseros-agent/apps/agent/entrypoints/sidepanel/index/useChatSession.ts Removes approval-sync effects, approval storage polling, addToolApprovalResponse, and the approval-resume sendAutomaticallyWhen trigger. sendAutomaticallyWhen now unconditionally returns false.
packages/browseros-agent/apps/agent/lib/messaging/server/buildChatRequestBody.ts Removes aclRules, toolApprovalConfig, and toolApprovalResponses parameters and the toRequestToolApprovalConfig helper from the request builder.
packages/browseros-agent/apps/agent/entrypoints/sidepanel/index/getMessageSegments.ts Removes approval-requested, approval-responded, and the approval field from tool parts, but retains output-denied in the type even though no code path can now produce it.
packages/browseros-agent/apps/agent/entrypoints/sidepanel/index/ToolBatch.tsx Removes ApprovalButtons, onApprove/onDeny props, and approval-pending open logic. isToolDenied and its ShieldX rendering are now dead since output-denied can't be produced.
packages/browseros-agent/apps/server/src/browser/browser.ts Removes resolveElementProperties and highlightBlockedElement — both were only used by the ACL guard. Clean deletion.
packages/browseros-agent/apps/server/src/api/server.ts Removes GlobalAclPolicyService instantiation and /acl-rules route registration. Server startup is simplified.
packages/browseros-agent/apps/server/tests/api/types.test.ts New negative-coverage test confirming that aclRules, toolApprovalConfig, and toolApprovalResponses are stripped by ChatRequestSchema.parse.
packages/browseros-agent/apps/server/tests/agent/tool-adapter.test.ts New negative-coverage test confirming no browser tool exposes needsApproval in its definition.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
packages/browseros-agent/apps/agent/entrypoints/sidepanel/index/getMessageSegments.ts:8-16
**Dead state variant — `output-denied` can no longer be produced**

`output-denied` was set exclusively by the AI SDK's `needsApproval` flow, which was removed in this PR (`needsApproval` was stripped from every tool definition in `tool-adapter.ts`). The state now lives in `ToolInvocationState` and `ExecutionStepState`, drives an `isToolDenied` branch in `ToolBatch.tsx`, and is handled in `normalize.ts`'s `getPreviewText` — but none of those paths can fire at runtime. Per the team's rule on dead code, these vestiges should be removed alongside the other governance machinery.

Reviews (1): Last reviewed commit: "chore: unship governance" | Re-trigger Greptile

@shadowfax92
Copy link
Copy Markdown
Contributor Author

Addressed review feedback in 5e74487b:

  • Removed the remaining output-denied UI/execution-history state and denied-count plumbing.
  • Removed stale compaction token counting for tool approval request/response parts.

Re-verified with agent/server typechecks plus @browseros/agent test and @browseros/server test:agent.

@shadowfax92
Copy link
Copy Markdown
Contributor Author

You were right: the shared ACL/tool-approval exports are governance code too. Addressed in 7aa4437a by deleting:

  • packages/shared/src/acl/match.ts
  • packages/shared/src/constants/tool-approval.ts
  • packages/shared/src/types/acl.ts
  • the corresponding @browseros/shared package exports

Verification for this follow-up: shared/agent/server typechecks, changed-file Biome, @browseros/agent test, and @browseros/server test:api.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant