create endpoint for checking env status of policies #534
Conversation
WalkthroughThe changes introduce a new AI-powered router for generating policy names and descriptions, refactor the main policy API router to use a unified schema namespace, and add a nested router for evaluating environment-specific policy applicability to deployment versions. The environment policy evaluation logic fetches relevant policies, their rules, and checks both approval and version selector rules against a candidate version, returning detailed evaluation results. In the rule engine, method signatures are updated to remove the use of a context parameter, simplifying interfaces for rule evaluation, filtering, and pre-validation. Several functions and rules are made publicly exportable for broader use. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant AI
participant DB
Client->>API: Call policyAiRouter.generateName/Description(policyConfig)
API->>AI: Send policy config to GPT-4 Turbo
AI-->>API: Return generated name/description
API-->>Client: Respond with generated metadata
Client->>API: Call policyRouter.environmentPolicy.evaluateVersion(envId, versionId)
API->>DB: Fetch environment, workspace, policies, policy rules, version
API->>DB: Evaluate approval rules and version selectors
API-->>Client: Return policies and evaluation results
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/rule-engine/src/rules/version-approval-rule.ts (1)
50-58: 🛠️ Refactor suggestionNo rejection reason when approvals are simply insufficient
Only explicit rejections generate a
rejectionReasonsentry.
When a version just lacks enough approvals (<minApprovals) it is silently filtered out, leaving the caller with no hint why.- return approvals.length >= this.options.minApprovals; + if (approvals.length < this.options.minApprovals) { + rejectionReasons.set( + release.id, + `Requires ≥${this.options.minApprovals} approvals; has ${approvals.length}.`, + ); + return false; + } + return true;Providing feedback avoids confusing “version disappeared” scenarios for users.
🧹 Nitpick comments (4)
packages/rule-engine/src/rules/version-approval-rule.ts (1)
36-43: Good refactor, but consider DB round-trippingWith the context argument removed, the method signature is cleaner.
However, each call togetApprovalRecordsstill performs one DB query per rule evaluation. If many policies share the same versions, you may issue the same query repeatedly. Caching byversionIdsfor the lifetime of the request (or batching queries) would cut latency.packages/api/src/router/policy.ts (3)
37-97: Sanitisation is incomplete for AI-generated namesThe post-processing only strips quotes/back-ticks.
Newlines and other control characters could still slip through and break UI rendering or logs.-return text - .trim() - .replaceAll("`", "") - .replaceAll("'", "") - .replaceAll('"', ""); +return text + .trim() + .replace(/[`'"\n\r\t]/g, ""); // strip quotes and control chars
631-645: Inefficient existence check ingetVersionSelectorSelecting full rows only to count them is wasteful.
Most SQL builders (including drizzle) expose anexists/select(1)helper that can short-circuit.-return db - .select() +return db + .select({ ok: 1 })or use
.count()/.exists()to avoid transferring data you never use.
658-676: Serial DB queries inside nested Promise loops
getApprovalReasonsexecutesrule.filter()for every rule of every policy, each of which may hit the database.
For large policy sets this scales O(n·m) in round-trips.Investigate:
- Let each rule implementation accept a shared
cacheobject, or- Aggregate
versionIdsper rule type and fetch approvals in one shot (similar to comment inVersionApprovalRule).This will cut latency under heavy policy counts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/api/src/router/policy.ts(15 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(3 hunks)packages/rule-engine/src/manager/version-manager.ts(1 hunks)packages/rule-engine/src/manager/version-rule-engine.ts(4 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(1 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(1 hunks)packages/rule-engine/src/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/types.tspackages/rule-engine/src/rules/deployment-deny-rule.tspackages/rule-engine/src/manager/version-rule-engine.tspackages/rule-engine/src/manager/version-manager.tspackages/rule-engine/src/manager/version-manager-rules.tspackages/rule-engine/src/rules/version-approval-rule.tspackages/api/src/router/policy.ts
🧬 Code Graph Analysis (3)
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
packages/rule-engine/src/types.ts (1)
PreValidationResult(45-48)
packages/rule-engine/src/manager/version-rule-engine.ts (1)
packages/rule-engine/src/types.ts (2)
RuleSelectionResult(87-90)isPreValidationRule(67-71)
packages/rule-engine/src/rules/version-approval-rule.ts (2)
packages/rule-engine/src/manager/version-rule-engine.ts (1)
Version(12-18)packages/rule-engine/src/types.ts (1)
RuleEngineRuleResult(30-33)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (7)
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
94-94: Signature simplification looks goodRemoving the unused
RuleEngineContextparameter from thepassing()method simplifies the interface while maintaining the same functionality. This change aligns with the broader refactoring to remove context parameters from rule methods throughout the rule engine.packages/rule-engine/src/manager/version-manager.ts (1)
192-192: API call updated correctlyThe call to
engine.evaluate()has been properly updated to remove the context parameter, consistent with the changes to the rule engine interfaces. The context is still properly retrieved from the database but is no longer passed to the evaluation method.packages/rule-engine/src/types.ts (2)
52-52: Interface simplification is consistentRemoving the
contextparameter from thepassing()method in thePreValidationRuleinterface properly aligns with the implementation changes seen in the rule classes. This simplification makes the API cleaner while maintaining functionality.
93-93: RuleEngine type updated correctlyThe
evaluatemethod signature in theRuleEnginetype has been properly updated to remove the context parameter, ensuring type consistency throughout the rule engine system.packages/rule-engine/src/manager/version-manager-rules.ts (1)
23-23: Making rule functions exportable improves modularityExporting the approval rule factory functions makes them available for external use, which improves the modularity of the system. This change enables these functions to be used in the new environment-specific policy evaluation logic while maintaining their current internal usage.
Also applies to: 35-35, 48-48
packages/rule-engine/src/manager/version-rule-engine.ts (1)
64-64: Validate all call-sites after the breaking API change
evaluatenow only acceptscandidates: Version[]. Please make sure every place in the codebase that previously calledengine.evaluate(ctx, candidates)has been migrated; otherwise you will get silentundefinedarguments or compile-time errors that break the deploy.
A quickrg "evaluate("search should be enough.packages/api/src/router/policy.ts (1)
510-539: Policy targeting logic ignores resource/deployment selectorsThe
applicablePolicyIdsquery explicitly filters forisNull(policyTarget.resourceSelector), isNull(policyTarget.deploymentSelector),meaning policies that do define resource or deployment selectors are never evaluated, even if they might still match the current environment.
Is that intentional? If not, dropping those predicates (or adding the appropriate selector evaluation) will prevent accidental policy bypass.
| const result = rule.passing(); | ||
|
|
There was a problem hiding this comment.
Await the possibly-async passing() call
passing() used to accept a context argument and could legitimately be async (e.g. involve DB checks).
Removing the context did not guarantee it became synchronous, so calling it without await may turn result into a Promise, causing the subsequent property access (result.passing) to be undefined.
-const result = rule.passing();
+const result = await rule.passing();If passing() is indeed synchronous, mark its return type as non-Promise so the compiler enforces this and prevents future misuse.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = rule.passing(); | |
| const result = await rule.passing(); |
| .query(async ({ ctx, input: { environmentId, versionId } }) => { | ||
| // First, find the environment and its associated workspace This is | ||
| // needed to scope the policy search to the correct workspace | ||
| const environment = await ctx.db.query.environment.findFirst({ | ||
| where: eq(schema.environment.id, environmentId), | ||
| with: { system: { with: { workspace: true } } }, | ||
| }); | ||
| if (environment == null) throw new Error("Environment not found"); |
There was a problem hiding this comment.
Missing workspace / environment authorisation check
protectedProcedure guards the route, but there is no explicit check that the authenticated user can actually read the workspace linked to the environment.
If the caller knows an environmentId belonging to another workspace, this endpoint may leak policy metadata.
Consider adding:
authorizationCheck: ({ canUser, input }) =>
canUser.perform(Permission.PolicyList).on({ type: "workspace", id: workspace.id }),after the environment lookup.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes