refactor: add uuid field to release (noop)#826
Merged
adityachoudhari26 merged 1 commit intomainfrom Mar 3, 2026
Merged
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughThe PR refactors release identification by adding a required UUID Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/svc/controllers/jobdispatch/reconcile.go (1)
40-42:⚠️ Potential issue | 🔴 CriticalFix the identifier mismatch between job query and creation.
Line 40 queries jobs using
release.UUID()but line 114 creates jobs withrelease.ContentHash()as the ReleaseId. SinceUUID()is derived fromContentHash()but produces a different string value,GetJobsForRelease()will fail to find existing jobs once implemented, causing duplicate job creation. Userelease.ContentHash()for both operations:releaseID := release.ContentHash() existingJobs, err := getter.GetJobsForRelease(ctx, releaseID)Also verify that
GetJobsForRelease()ingetters_postgres.go(currently unimplemented) queries the jobs table by matching the ReleaseId string field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobdispatch/reconcile.go` around lines 40 - 42, The code uses release.UUID() when fetching existing jobs but uses release.ContentHash() when creating jobs, causing a mismatch; change the lookup to use release.ContentHash() by setting releaseID := release.ContentHash() before calling getter.GetJobsForRelease(ctx, releaseID), and ensure the getter.GetJobsForRelease implementation in getters_postgres.go queries the jobs table by the ReleaseId string field (matching ContentHash) so the lookup and creation use the same identifier.
🧹 Nitpick comments (2)
apps/workspace-engine/test/e2e/engine_nil_release_handling_test.go (1)
297-298: Optional: rename variable to reflect content hash semantics.
releaseIdToDeletenow stores a content hash, so a more explicit name would reduce ambiguity in future maintenance.Proposed rename
- releaseIdToDelete := release.ContentHash() - engine.Workspace().Releases().Remove(ctx, releaseIdToDelete) + releaseContentHashToDelete := release.ContentHash() + engine.Workspace().Releases().Remove(ctx, releaseContentHashToDelete)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_nil_release_handling_test.go` around lines 297 - 298, The variable releaseIdToDelete actually holds a content hash (from release.ContentHash()), which is misleading; rename it to something explicit like releaseContentHash or contentHashToDelete and update its usage in the call to engine.Workspace().Releases().Remove(ctx, ...) so the identifier reflects the data it contains (change both the declaration and the argument passed to Releases().Remove to use the new name).apps/workspace-engine/pkg/persistence/integration_test.go (1)
667-667: Strengthen this test to fully validate content-hash linkage.
Releases().Get(release.ContentHash())is correct, but this test still seeds jobs with a separate randomreleaseId, so it doesn’t verify release↔job linkage under the new identifier.Proposed test-alignment diff
- releaseId := uuid.New().String() @@ jobPending := &oapi.Job{ Id: uuid.New().String(), Status: "pending", - ReleaseId: releaseId, + ReleaseId: release.ContentHash(), } @@ jobInProgress := &oapi.Job{ Id: uuid.New().String(), Status: "in_progress", - ReleaseId: releaseId, + ReleaseId: release.ContentHash(), } @@ jobCompleted := &oapi.Job{ Id: uuid.New().String(), Status: "completed", - ReleaseId: releaseId, + ReleaseId: release.ContentHash(), }
🤖 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/workspace-engine/pkg/workspace/store/releases.go`:
- Around line 29-31: The upsert currently only sets release.Id when it's nil,
allowing a stale non-nil Id to be persisted and become unreachable by Get/Remove
which derive the DB UUID from release.ContentHash(); modify the upsert logic in
releases.go (the block that touches release.Id and uses uuid.NewSHA1) to always
compute and assign release.Id = uuid.NewSHA1(uuid.NameSpaceOID,
[]byte(release.ContentHash())) (i.e., overwrite release.Id unconditionally) so
the stored Id always matches release.ContentHash() and Get/Remove/references
remain consistent.
---
Outside diff comments:
In `@apps/workspace-engine/svc/controllers/jobdispatch/reconcile.go`:
- Around line 40-42: The code uses release.UUID() when fetching existing jobs
but uses release.ContentHash() when creating jobs, causing a mismatch; change
the lookup to use release.ContentHash() by setting releaseID :=
release.ContentHash() before calling getter.GetJobsForRelease(ctx, releaseID),
and ensure the getter.GetJobsForRelease implementation in getters_postgres.go
queries the jobs table by the ReleaseId string field (matching ContentHash) so
the lookup and creation use the same identifier.
---
Nitpick comments:
In `@apps/workspace-engine/test/e2e/engine_nil_release_handling_test.go`:
- Around line 297-298: The variable releaseIdToDelete actually holds a content
hash (from release.ContentHash()), which is misleading; rename it to something
explicit like releaseContentHash or contentHashToDelete and update its usage in
the call to engine.Workspace().Releases().Remove(ctx, ...) so the identifier
reflects the data it contains (change both the declaration and the argument
passed to Releases().Remove to use the new name).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (60)
apps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/entities.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/oapi/oapi.goapps/workspace-engine/pkg/oapi/persistence.goapps/workspace-engine/pkg/persistence/integration_test.goapps/workspace-engine/pkg/workspace/jobs/factory.goapps/workspace-engine/pkg/workspace/jobs/factory_test.goapps/workspace-engine/pkg/workspace/releasemanager/action/orchestrator.goapps/workspace-engine/pkg/workspace/releasemanager/action/orchestrator_test.goapps/workspace-engine/pkg/workspace/releasemanager/action/rollback/hooks.goapps/workspace-engine/pkg/workspace/releasemanager/action/rollback/rollback.goapps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification.goapps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility_retry_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/planner.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/planner_test.goapps/workspace-engine/pkg/workspace/releasemanager/manager.goapps/workspace-engine/pkg/workspace/releasemanager/manager_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_policy_action.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentwindow/deploymentwindow_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environment_progression_action.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/smart_defaults_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/rollback/rollback_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/version_summary_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/versioncooldown_test.goapps/workspace-engine/pkg/workspace/releasemanager/state_index_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/executor_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/manager_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/recorder_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.goapps/workspace-engine/pkg/workspace/store/jobs.goapps/workspace-engine/pkg/workspace/store/release_targets_test.goapps/workspace-engine/pkg/workspace/store/releases.goapps/workspace-engine/pkg/workspace/store/repository/db/releases/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.goapps/workspace-engine/pkg/workspace/store/repository/memory/indexstore/schema_release.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/adapters.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile.goapps/workspace-engine/test/controllers/pipeline_test.goapps/workspace-engine/test/e2e/engine_job_verification_query_test.goapps/workspace-engine/test/e2e/engine_nil_release_handling_test.goapps/workspace-engine/test/e2e/engine_releasemanager_test.goapps/workspace-engine/test/e2e/engine_releasetarget_state_test.goapps/workspace-engine/test/e2e/engine_verification_hooks_test.gopackages/workspace-engine-sdk/src/schema.ts
Comment on lines
+29
to
+31
| if release.Id == uuid.Nil { | ||
| release.Id = uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) | ||
| } |
Contributor
There was a problem hiding this comment.
Keep release.Id synchronized with ContentHash() on every upsert.
Get/Remove paths derive DB UUID from the content hash key, but this block only derives Id when it is nil. A non-nil stale/mismatched Id can be persisted and then become unreachable via Get(release.ContentHash()).
🔧 Proposed fix
func (r *Releases) Upsert(ctx context.Context, release *oapi.Release) error {
- if release.Id == uuid.Nil {
- release.Id = uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash()))
- }
+ expectedID := uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash()))
+ if release.Id != expectedID {
+ release.Id = expectedID
+ }
if err := r.repo.Set(release); err != nil {
return err
}📝 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.
Suggested change
| if release.Id == uuid.Nil { | |
| release.Id = uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) | |
| } | |
| func (r *Releases) Upsert(ctx context.Context, release *oapi.Release) error { | |
| expectedID := uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) | |
| if release.Id != expectedID { | |
| release.Id = expectedID | |
| } | |
| if err := r.repo.Set(release); err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/releases.go` around lines 29 - 31,
The upsert currently only sets release.Id when it's nil, allowing a stale
non-nil Id to be persisted and become unreachable by Get/Remove which derive the
DB UUID from release.ContentHash(); modify the upsert logic in releases.go (the
block that touches release.Id and uses uuid.NewSHA1) to always compute and
assign release.Id = uuid.NewSHA1(uuid.NameSpaceOID,
[]byte(release.ContentHash())) (i.e., overwrite release.Id unconditionally) so
the stored Id always matches release.ContentHash() and Get/Remove/references
remain consistent.
This was referenced Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Refactor