feat: add requireVerificationPassed to EnvironmentProgressionRule#991
feat: add requireVerificationPassed to EnvironmentProgressionRule#991adityachoudhari26 merged 8 commits intomainfrom
Conversation
Fixes issue #742 by adding a new `requireVerificationPassed` flag to the `EnvironmentProgressionRule`. When enabled, jobs with a failed or running verification status are excluded from the success rate calculation, preventing progression to dependent environments when verification has not passed. Changes: - Add `require_verification_passed` column to DB schema with migration - Add `requireVerificationPassed` field to EnvironmentProgressionRule in all API schemas - Update SQL query to compute aggregate job verification status inline - Update ReleaseTargetJob struct and tracker to check verification status - Add tests for the new verification-aware success tracking Co-authored-by: Aditya Choudhari <adityachoudhari26@users.noreply.github.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/types/openapi.ts`:
- Around line 1413-1417: The OpenAPI-generated type marks
requireVerificationPassed as required but the server treats it as optional with
a default of false; update the OpenAPI source schema to make the property
optional (remove it from required[] or mark it with nullable/optional) for the
relevant schema that contains requireVerificationPassed, then regenerate the
TypeScript artifacts so the generated type (requireVerificationPassed) is
optional and clients won't be forced to send it.
In `@e2e/api/schema.ts`:
- Around line 1413-1417: The generated schema declares requireVerificationPassed
as a required boolean which breaks existing contracts; update the source OpenAPI
schema so requireVerificationPassed is optional (remove it from the property's
required list or omit it from the parent object's required array) or mark it as
not required/nullable in the schema, then regenerate the TypeScript types so the
generated symbol requireVerificationPassed becomes optional (e.g., boolean |
undefined or with a trailing ? in the interface) across the generated output.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e94dd90b-13fc-433e-9907-cd896cc9fddd
📒 Files selected for processing (33)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/policies.jsonnetapps/api/src/routes/v1/workspaces/policies.tsapps/api/src/types/openapi.tsapps/web/app/api/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/policy.jsonnetapps/workspace-engine/pkg/db/convert.goapps/workspace-engine/pkg/db/job_verification_metric.sql.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/policies.sql.goapps/workspace-engine/pkg/db/queries/job_verification_metric.sqlapps/workspace-engine/pkg/db/queries/policies.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/mock_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/test/controllers/harness/mocks.godocs/policies/environment-progression.mdxe2e/api/schema.tspackages/db/drizzle/0186_cooing_doomsday.sqlpackages/db/drizzle/meta/0186_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/policy.tspackages/workspace-engine-sdk/src/schema.ts
| /** | ||
| * @description If true, jobs must also have passed verification to count toward the success percentage | ||
| * @default false | ||
| */ | ||
| requireVerificationPassed: boolean; |
There was a problem hiding this comment.
Make requireVerificationPassed optional in the API contract.
Line 1417 defines requireVerificationPassed as required, but this feature is described and implemented as defaulting to false when omitted. Keeping it required here can break generated clients and conflicts with server-side defaulting behavior.
Suggested contract shape
- requireVerificationPassed: boolean;
+ requireVerificationPassed?: boolean;Since this file is generated, apply the fix in the OpenAPI source schema and regenerate artifacts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/types/openapi.ts` around lines 1413 - 1417, The
OpenAPI-generated type marks requireVerificationPassed as required but the
server treats it as optional with a default of false; update the OpenAPI source
schema to make the property optional (remove it from required[] or mark it with
nullable/optional) for the relevant schema that contains
requireVerificationPassed, then regenerate the TypeScript artifacts so the
generated type (requireVerificationPassed) is optional and clients won't be
forced to send it.
| /** | ||
| * @description If true, jobs must also have passed verification to count toward the success percentage | ||
| * @default false | ||
| */ | ||
| requireVerificationPassed: boolean; |
There was a problem hiding this comment.
Make requireVerificationPassed optional to avoid a breaking contract.
Line 1417 declares this new field as required (requireVerificationPassed: boolean). For a default-false toggle, this forces all existing policy payloads/types to include it and creates an unnecessary breaking change. Please mark it optional in the source OpenAPI schema and regenerate.
Suggested generated output shape
- requireVerificationPassed: boolean;
+ requireVerificationPassed?: boolean;📝 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.
| /** | |
| * @description If true, jobs must also have passed verification to count toward the success percentage | |
| * @default false | |
| */ | |
| requireVerificationPassed: boolean; | |
| /** | |
| * `@description` If true, jobs must also have passed verification to count toward the success percentage | |
| * `@default` false | |
| */ | |
| requireVerificationPassed?: boolean; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/api/schema.ts` around lines 1413 - 1417, The generated schema declares
requireVerificationPassed as a required boolean which breaks existing contracts;
update the source OpenAPI schema so requireVerificationPassed is optional
(remove it from the property's required list or omit it from the parent object's
required array) or mark it as not required/nullable in the schema, then
regenerate the TypeScript types so the generated symbol
requireVerificationPassed becomes optional (e.g., boolean | undefined or with a
trailing ? in the interface) across the generated output.
There was a problem hiding this comment.
Pull request overview
Adds a new requireVerificationPassed flag to EnvironmentProgressionRule so environment progression success-rate/soak-time calculations can ignore “successful” jobs whose verification hasn’t passed.
Changes:
- Extends the policy schema/API/SDK types and docs to include
requireVerificationPassed(defaultfalse). - Persists the flag in Postgres (Drizzle + sqlc schema/query updates) and returns/accepts it via API.
- Updates the workspace-engine environment progression evaluator/job-tracker to optionally gate “success” by verification status, including new DB query + unit tests.
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/workspace-engine-sdk/src/schema.ts | SDK schema adds requireVerificationPassed on EnvironmentProgressionRule. |
| packages/db/src/schema/policy.ts | Drizzle table adds require_verification_passed column w/ default. |
| packages/db/drizzle/meta/_journal.json | Records new migration in Drizzle journal. |
| packages/db/drizzle/meta/0186_snapshot.json | Drizzle snapshot updated to include new column. |
| packages/db/drizzle/0186_cooing_doomsday.sql | Migration to add require_verification_passed column. |
| e2e/api/schema.ts | E2E generated OpenAPI types include requireVerificationPassed. |
| docs/policies/environment-progression.mdx | Documents new flag and updates examples/“How it works”. |
| apps/workspace-engine/test/controllers/harness/mocks.go | Test harness getter adds GetVerificationStatusForJobs. |
| apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go | Mock updated to satisfy new getters interface. |
| apps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.go | Mock updated to satisfy new getters interface. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go | Mock updated to satisfy new getters interface. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime.go | Tracker construction updated for new ctor parameter. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate.go | Tracker construction updated for new ctor parameter. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/mock_test.go | Mock adds job verification status support. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.go | Adds extensive tests covering verification-gated success. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.go | Implements verification-aware success/soak-time tracking. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.go | Adds GetVerificationStatusForJobs + Postgres implementation. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression.go | Wires rule flag into tracker construction. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Generated Go model includes RequireVerificationPassed. |
| apps/workspace-engine/pkg/db/queries/schema.sql | Schema includes new column for env progression rule table. |
| apps/workspace-engine/pkg/db/queries/policies.sql | Reads/writes require_verification_passed in policy rule queries. |
| apps/workspace-engine/pkg/db/queries/job_verification_metric.sql | Adds batch query for metric + measurement statuses by job IDs. |
| apps/workspace-engine/pkg/db/policies.sql.go | sqlc output updated for new column in env progression rules. |
| apps/workspace-engine/pkg/db/models.go | sqlc model updated with RequireVerificationPassed. |
| apps/workspace-engine/pkg/db/job_verification_metric.sql.go | sqlc output adds new batch query function/types. |
| apps/workspace-engine/pkg/db/convert.go | Converts DB JSON rule payload into OAPI rule incl. new flag. |
| apps/workspace-engine/oapi/spec/schemas/policy.jsonnet | Schema adds requireVerificationPassed property. |
| apps/workspace-engine/oapi/openapi.json | Generated OpenAPI updated with new property. |
| apps/web/app/api/openapi.ts | Web app generated OpenAPI types include new property. |
| apps/api/src/types/openapi.ts | API generated OpenAPI types include new property. |
| apps/api/src/routes/v1/workspaces/policies.ts | API persistence/response includes requireVerificationPassed. |
| apps/api/openapi/schemas/policies.jsonnet | API OpenAPI schema adds new property. |
| apps/api/openapi/openapi.json | API generated OpenAPI updated with new property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| verificationStatuses, err := t.fetchVerificationStatuses(ctx, rows) | ||
| if err != nil { | ||
| span.AddEvent("GetVerificationStatusForJobs error", | ||
| trace.WithAttributes(attribute.String("error", err.Error())), | ||
| ) | ||
| return t.jobs | ||
| } |
There was a problem hiding this comment.
When GetVerificationStatusForJobs returns an error, compute() returns early with an empty t.jobs. This causes downstream evaluation to report "No jobs found" and can block progression for the wrong reason. Consider still populating t.jobs (and jobsByStatus) and treating verification as not-passed/unknown for success calculations (or propagating the error up so the evaluator can surface an explicit failure message) instead of returning before iterating rows.
Fixes #742 by adding a new
requireVerificationPassedflag to theEnvironmentProgressionRule. When enabled, jobs with a failed or running verification status are excluded from the success rate calculation, preventing progression to dependent environments when verification has not passed.Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
requireVerificationPassedsetting to environment progression policies, allowing jobs to count toward success percentage only when verification has passed (defaults to false).Documentation