chore: unship governance#959
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 3m 18s —— View job Code Review: chore/unship-1
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 coverageThe two new test files are well-targeted:
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
|
Greptile SummaryThis 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.
Confidence Score: 4/5Safe 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
Important Files Changed
Prompt To Fix All With AIFix 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 |
|
Addressed review feedback in
Re-verified with agent/server typechecks plus |
|
You were right: the shared ACL/tool-approval exports are governance code too. Addressed in
Verification for this follow-up: shared/agent/server typechecks, changed-file Biome, |
Summary
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