feat(policy-engine) : built policy engine#4
Conversation
There was a problem hiding this comment.
15 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/policy/rules/budget.ts">
<violation number="1" location="apps/api/src/policy/rules/budget.ts:20">
P1: `token` is used without validation in budget calculation. Negative/NaN values can bypass budget-deny logic.</violation>
<violation number="2" location="apps/api/src/policy/rules/budget.ts:21">
P1: The budget check reads `tokens_used` but nothing in the execution path increments it after a successful tool call. Every subsequent invocation will see the same stale counter, meaning budget limits are effectively never enforced across repeated calls in a conversation.</violation>
</file>
<file name="apps/api/src/index.ts">
<violation number="1" location="apps/api/src/index.ts:16">
P1: Policy management routes are exposed without authentication/authorization. This allows unauthorized clients to create/update/delete enforcement policies.</violation>
</file>
<file name="apps/api/src/policy/engine.ts">
<violation number="1" location="apps/api/src/policy/engine.ts:75">
P1: Unknown tools can be implicitly allowed. Missing policy currently reaches default ALLOW instead of requiring approval.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
| app.use(cors()); | ||
| app.use(express.json({ limit: "1mb" })); | ||
| app.use(policiesRouter); |
There was a problem hiding this comment.
P1: Policy management routes are exposed without authentication/authorization. This allows unauthorized clients to create/update/delete enforcement policies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/index.ts, line 16:
<comment>Policy management routes are exposed without authentication/authorization. This allows unauthorized clients to create/update/delete enforcement policies.</comment>
<file context>
@@ -12,6 +13,7 @@ const port = process.env.PORT || 3001;
app.use(cors());
app.use(express.json({ limit: "1mb" }));
+app.use(policiesRouter);
// Health check endpoint
</file context>
| }; | ||
| } | ||
|
|
||
| const isExceeded = |
There was a problem hiding this comment.
P1: token is used without validation in budget calculation. Negative/NaN values can bypass budget-deny logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/policy/rules/budget.ts, line 20:
<comment>`token` is used without validation in budget calculation. Negative/NaN values can bypass budget-deny logic.</comment>
<file context>
@@ -0,0 +1,36 @@
+ };
+ }
+
+ const isExceeded =
+ conversation.tokens_used + token > conversation.budget_limit;
+
</file context>
| } | ||
|
|
||
| const isExceeded = | ||
| conversation.tokens_used + token > conversation.budget_limit; |
There was a problem hiding this comment.
P1: The budget check reads tokens_used but nothing in the execution path increments it after a successful tool call. Every subsequent invocation will see the same stale counter, meaning budget limits are effectively never enforced across repeated calls in a conversation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/policy/rules/budget.ts, line 21:
<comment>The budget check reads `tokens_used` but nothing in the execution path increments it after a successful tool call. Every subsequent invocation will see the same stale counter, meaning budget limits are effectively never enforced across repeated calls in a conversation.</comment>
<file context>
@@ -0,0 +1,36 @@
+ }
+
+ const isExceeded =
+ conversation.tokens_used + token > conversation.budget_limit;
+
+ return {
</file context>
There was a problem hiding this comment.
5 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/policy/rules/budget.ts">
<violation number="1" location="apps/api/src/policy/rules/budget.ts:20">
P1: `token` is used without validation in budget calculation. Negative/NaN values can bypass budget-deny logic.</violation>
<violation number="2" location="apps/api/src/policy/rules/budget.ts:21">
P1: The budget check reads `tokens_used` but nothing in the execution path increments it after a successful tool call. Every subsequent invocation will see the same stale counter, meaning budget limits are effectively never enforced across repeated calls in a conversation.</violation>
</file>
<file name="apps/api/src/index.ts">
<violation number="1" location="apps/api/src/index.ts:16">
P1: Policy management routes are exposed without authentication/authorization. This allows unauthorized clients to create/update/delete enforcement policies.</violation>
</file>
<file name="apps/api/src/policy/engine.ts">
<violation number="1" location="apps/api/src/policy/engine.ts:75">
P1: Unknown tools can be implicitly allowed. Missing policy currently reaches default ALLOW instead of requiring approval.</violation>
<violation number="2" location="apps/api/src/policy/engine.ts:79">
P2: Using one cached policy for the approval stage creates a stale-decision window. A request can miss stricter policy updates applied during evaluation.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/policy/rules/budget.ts">
<violation number="1" location="apps/api/src/policy/rules/budget.ts:20">
P1: `token` is used without validation in budget calculation. Negative/NaN values can bypass budget-deny logic.</violation>
<violation number="2" location="apps/api/src/policy/rules/budget.ts:21">
P1: The budget check reads `tokens_used` but nothing in the execution path increments it after a successful tool call. Every subsequent invocation will see the same stale counter, meaning budget limits are effectively never enforced across repeated calls in a conversation.</violation>
</file>
<file name="apps/api/src/index.ts">
<violation number="1" location="apps/api/src/index.ts:16">
P1: Policy management routes are exposed without authentication/authorization. This allows unauthorized clients to create/update/delete enforcement policies.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Summary by cubic
Introduces a policy engine to gate tool execution with ALLOW/DENY/PENDING and human approvals. Adds
/policiesCRUD and mounts the router;/mcp/executestill trusts request-supplied decisions (nodecide()wiring yet).New Features
tool_name, and consumes approvals on use./policiesCRUD endpoints (list, get, create, update, delete) with implicitAPPROVALfor unknown tools; router mounted.ApprovalStatusfrom@repo/db; addsApprovalRequest,RuleResult,ConversationRequest; adds@repo/dbREADME; tests for rules, engine, decisions, and routes.Bug Fixes
conversationIdis missing or"unknown"; non-existent explicit IDs also fail closed.Written for commit e9680bb. Summary will update on new commits.
Greptile Summary
This PR introduces a policy engine for gating MCP tool executions with ALLOW/DENY/PENDING decisions and human-in-the-loop approvals, along with
/policiesCRUD endpoints. Several issues flagged in the previous review round have been resolved: approval records are now consumed (deleted) on use,tool_nameis validated against the approval record, the pre-fetched policy is correctly passed to all rule checkers (resolving the TOCTOU concern), and unknown tools now correctly default to requiring APPROVAL rather than executing freely.engine.ts,decision.ts,rules/) chains block → budget → approval checks using a single pre-fetched policy record, and creates/validates approval records indecide().router.ts,index.ts) provides/policieslist/get/create/update/delete with enum validation and conflict detection; mounted globally at app startup.decide()is not yet wired toPOST /mcp/execute— the executor still readsdecisiondirectly from the request body, so the entire policy engine can currently be bypassed by any caller; this is acknowledged as deferred work.Confidence Score: 4/5
The policy engine module is internally correct, but the executor still reads the decision from the request body rather than calling decide(), leaving the gate wide open on the actual execution path.
The policy engine logic itself (chained rules, TOCTOU fix, approval consumption, tool-name validation) is sound and the tests are comprehensive. However, POST /mcp/execute never calls decide() — any caller can send {"decision":"ALLOW"} and bypass every block, budget, and approval rule in this PR. That gap is acknowledged and deferred, but it means the feature is not enforced yet on any real request.
apps/api/mcp/execute.ts and apps/api/src/policy/decision.ts — the missing wiring between the decision engine and the execution handler is the key gap to close next.
Important Files Changed
Comments Outside Diff (2)
apps/api/src/index.ts, line 42-60 (link)The
POST /mcp/executehandler readsdecisiondirectly fromreq.bodyand passes it straight tomcpExecutor.execute(). No call todecide()is made, so any client can send{"decision": "ALLOW"}and bypass all block, budget, and approval rules entirely. The policy engine built in this PR is completely disconnected from the execution path.apps/api/types.ts, line 69-73 (link)ApprovalStatusenum duplicated betweentypes.tsand@repo/db@repo/dbalready exportsApprovalStatus(imported indecision.ts). Redeclaring it locally intypes.tsmeans two independent sources of truth for the same values. If the DB schema enum changes, only the Prisma-generated version updates automatically; the local copy will silently diverge.Reviews (4): Last reviewed commit: "fix(reviews) : fixed neccassary reviews" | Re-trigger Greptile