Skip to content

refactor: convert release id to deterministic uuid#828

Merged
adityachoudhari26 merged 6 commits intomainfrom
convert-release-uuid
Mar 3, 2026
Merged

refactor: convert release id to deterministic uuid#828
adityachoudhari26 merged 6 commits intomainfrom
convert-release-uuid

Conversation

@adityachoudhari26
Copy link
Member

@adityachoudhari26 adityachoudhari26 commented Mar 3, 2026

Summary by CodeRabbit

  • Improvements

    • Release identification moved to stable unique IDs (instead of content-hash) and telemetry now emits both release ID and content hash.
    • Automatic migration/backfill of legacy release and job identifiers during restore to preserve continuity.
  • Tests

    • Updated and added tests to validate ID backfill, job ID migration, and end-to-end behaviors with the new ID format.
  • Chores

    • Miscellaneous test and telemetry updates to align with the new identifier scheme.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 3, 2026

📝 Walkthrough

Walkthrough

This PR replaces usage of release.ContentHash() with release.Id.String() across the workspace engine, updates persistence/indexing to use UUID-based release IDs, and adds restore-time migrations to backfill deterministic UUIDs for legacy content-hash identifiers. Tests and telemetry are updated accordingly.

Changes

Cohort / File(s) Summary
Core ID switch
apps/workspace-engine/pkg/oapi/persistence.go, apps/workspace-engine/pkg/workspace/jobs/factory.go, apps/workspace-engine/pkg/workspace/store/jobs.go
Compaction keys, job creation, and job lookup use release.Id.String() instead of release.ContentHash().
Persistence & restore migration
apps/workspace-engine/pkg/persistence/integration_test.go, apps/workspace-engine/pkg/workspace/store/store.go, apps/workspace-engine/pkg/workspace/store/releases.go, apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go, apps/workspace-engine/pkg/workspace/store/repository/memory/indexstore/schema_release.go
Store restore now backfills deterministic UUIDs for legacy releases and migrates Job.ReleaseId values; repo Get/Remove handle string-to-UUID fallback; indexing uses release.Id.String().
Job factory & job signatures
apps/workspace-engine/pkg/workspace/jobs/..., apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go, apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go
Job creation and some job factory function signatures updated to accept releaseId (UUID string) rather than content-hash; Release.Id initialization moved to deterministic SHA1-based UUID derived from content hash.
Telemetry / tracing updates
apps/workspace-engine/pkg/workspace/releasemanager/.../orchestrator.go, .../rollback/..., .../verification/..., .../deployment/..., svc/controllers/desiredrelease/reconcile.go, svc/controllers/jobdispatch/reconcile.go
Telemetry and log attributes now emit release.id = Id.String() and include a new release.content_hash field; error/tracing payloads updated accordingly.
Policy evaluators & retry logic
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/.../retry/retry.go, other evaluator files and tests
Evaluator logic and tests switched comparisons/grouping from content-hash to Id.String(); retry counting and result details now include both release_id and release_content_hash.
Manager, planner, eligibility, and related components
apps/workspace-engine/pkg/workspace/releasemanager/manager.go, .../deployment/planner.go, .../deployment/job_eligibility.go, .../deployment/job_eligibility_*.go
Diagnostic messages, spans and job eligibility telemetry now reference Id.String() and include content-hash as a separate attribute; control flow unchanged.
Store & repository consumers (tests)
apps/workspace-engine/pkg/workspace/.../state_index_test.go, .../manager_test.go, .../store/release_targets_test.go, many evaluator/verification/deployment tests under apps/workspace-engine
Numerous tests updated to construct Job.ReleaseId and job/verification references from release.Id.String() rather than release.ContentHash(); added new restore tests validating UUID backfill and job migration.
End-to-end & controller tests
apps/workspace-engine/test/..., apps/workspace-engine/test/e2e/...
E2E and controller tests updated to use release.Id.String() for queries, removals and assertions; one test added/adjusted to construct JobUpdateEvent payloads with FieldsToUpdate.

Sequence Diagram(s)

(Skipped — changes are primarily identifier migration, telemetry, persistence and tests rather than introduction of a new multi-component control flow.)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐇 I hopped through code where hashes once played,
Now UUIDs guide the release parade.
Jobs find their releases by Id so neat,
ContentHash stays in logs — a tidy seat.
Hooray — the rabbit's mapped each release's beat!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: converting Release IDs from content hash-based identifiers to deterministic UUIDs throughout the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch convert-release-uuid

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Aditya Choudhari <48932219+adityachoudhari26@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go (1)

234-237: ⚠️ Potential issue | 🟡 Minor

Replace the remaining legacy ContentHash() release ID in this test.

Line 236 still uses release.ContentHash() while the rest of the file has moved to release.Id.String(). This inconsistency can hide regressions in the UUID migration path.

Suggested fix
  job := &oapi.Job{
      Id:        uuid.New().String(),
-     ReleaseId: release.ContentHash(),
+     ReleaseId: release.Id.String(),
      Status:    oapi.JobStatusSuccessful,
      CreatedAt: time.Now(),
  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go`
around lines 234 - 237, The test constructs an oapi.Job using
release.ContentHash(), which is legacy; update the job creation to use
release.Id.String() for the ReleaseId field so it matches the rest of the file's
UUID migration (replace release.ContentHash() with release.Id.String() in the
oapi.Job literal where ReleaseId is set, e.g., in the job variable
initialization) and ensure the ReleaseId value remains a string compatible with
oapi.Job.ReleaseId.
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go (1)

92-95: ⚠️ Potential issue | 🟠 Major

release.id trace attribute still reports content hash instead of UUID.

Line [94] sets "release.id" to releaseToDeploy.ContentHash(). After this migration, that key should carry releaseToDeploy.Id.String(); otherwise tracing correlation diverges from persisted/job release IDs.

Suggested fix
  ctx, span := tracer.Start(ctx, "ExecuteRelease",
  	oteltrace.WithAttributes(
- 		attribute.String("release.id", releaseToDeploy.ContentHash()),
+ 		attribute.String("release.id", releaseToDeploy.Id.String()),
+ 		attribute.String("release.content_hash", releaseToDeploy.ContentHash()),
  		attribute.String("deployment.id", releaseToDeploy.ReleaseTarget.DeploymentId),
  		attribute.String("environment.id", releaseToDeploy.ReleaseTarget.EnvironmentId),
  		attribute.String("resource.id", releaseToDeploy.ReleaseTarget.ResourceId),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`
around lines 92 - 95, The trace attribute "release.id" in ExecuteRelease is
incorrectly using releaseToDeploy.ContentHash(); update the attribute to use the
release's UUID string by setting attribute.String("release.id",
releaseToDeploy.Id.String()) (locate the tracer.Start call and the attribute
list around ExecuteRelease to change ContentHash() to Id.String()).
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go (1)

112-129: Consider extracting repeated system/link setup into a shared test helper.

The same setup sequence is repeated in multiple tests; a helper would reduce drift and keep test intent tighter.

♻️ Suggested refactor
+func setupSystemScopedDeploymentAndEnvironment(
+	t *testing.T,
+	ctx context.Context,
+	testStore *store.Store,
+	systemID string,
+	deployment *oapi.Deployment,
+	environment *oapi.Environment,
+) {
+	t.Helper()
+	testStore.Systems.Upsert(ctx, &oapi.System{
+		Id:   systemID,
+		Name: "test-system",
+	})
+	_ = testStore.Deployments.Upsert(ctx, deployment)
+	testStore.SystemDeployments.Link(systemID, deployment.Id)
+	_ = testStore.Environments.Upsert(ctx, environment)
+	testStore.SystemEnvironments.Link(systemID, environment.Id)
+}

Also applies to: 184-217, 347-363

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`
around lines 112 - 129, Extract the repeated setup sequence (calls to
testStore.Systems.Upsert, createTestJobAgent, testStore.JobAgents.Upsert,
createTestDeploymentForExecutor + testStore.Deployments.Upsert,
testStore.SystemDeployments.Link, createTestEnvironmentForExecutor +
testStore.Environments.Upsert, testStore.SystemEnvironments.Link) into a single
test helper (e.g., setupTestSystemWithDeploymentAndEnv) that accepts IDs/names
and returns the created entities; replace the duplicated blocks in
executor_test.go (and at the other indicated ranges) with calls to this helper
to reduce duplication and make tests clearer.
apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go (1)

47-50: Extract release ID normalization helper to avoid drift

Get and Remove now repeat the same UUID-parse/fallback block. Please centralize it into one helper so future migration behavior can’t diverge between read and delete paths.

♻️ Suggested refactor
+func normalizeReleaseID(id string) uuid.UUID {
+	uid, err := uuid.Parse(id)
+	if err == nil {
+		return uid
+	}
+	return uuid.NewSHA1(uuid.NameSpaceOID, []byte(id))
+}
+
 func (r *Repo) Get(id string) (*oapi.Release, bool) {
-	uid, err := uuid.Parse(id)
-	if err != nil {
-		uid = uuid.NewSHA1(uuid.NameSpaceOID, []byte(id))
-	}
+	uid := normalizeReleaseID(id)
 
 	row, err := db.GetQueries(r.ctx).GetReleaseByID(r.ctx, uid)
@@
 func (r *Repo) Remove(id string) error {
-	uid, err := uuid.Parse(id)
-	if err != nil {
-		uid = uuid.NewSHA1(uuid.NameSpaceOID, []byte(id))
-	}
+	uid := normalizeReleaseID(id)
 
 	if err := db.GetQueries(r.ctx).DeleteReleaseVariablesByReleaseID(r.ctx, uid); err != nil {

Also applies to: 140-143

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go`
around lines 47 - 50, Create a single helper (e.g., normalizeReleaseID or
parseOrSHA1UUID) that encapsulates the uid parsing logic: try uuid.Parse(id) and
on error return uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)); have it return a
uuid.UUID and error only if you want to propagate parse failures. Replace the
duplicated blocks inside the Get and Remove methods (and the other occurrence
around lines 140-143) to call this helper so both read and delete paths share
identical normalization behavior; update callers to use the helper's returned
uuid.
🤖 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/releasemanager/manager_test.go`:
- Line 106: The test uses release.Id.String() (e.g., when calling createTestJob)
but discards errors from the prior release upsert, so add require.NoError(...)
immediately after each release upsert call that populates release (the same
upsert invocations whose result is used for release.Id.String()) to fail fast on
setup errors; update the test lines around release upsert, including the setup
before createTestJob and the other upsert locations referenced (lines where
release is assigned), to call require.NoError(t, err) (or the project’s test
require helper) before using release.Id.String().

In
`@apps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.go`:
- Line 272: The test fixtures call createTestVerification with
release.Id.String() for the jobId parameter, which yields invalid
JobVerification.JobId links; update each affected call (e.g., the call at
createTestVerification(s, ctx, release.Id.String(), 1, 3600)) to pass the real
job id (job.Id.String()) instead of release.Id.String(), ensuring the local job
variable (the Job under test) is used or created before the call so
JobVerification.JobId correctly references the Job; apply this change to all
listed sites that call createTestVerification and verify the job variable name
matches the surrounding test scope.

In `@apps/workspace-engine/pkg/workspace/store/store.go`:
- Around line 474-485: The migration updates job.ReleaseId in-memory and calls
s.repo.Jobs.Set but does not persist the change to the active jobs store when
running in DB-backed mode (WithDBJobs), leaving persisted jobs with legacy
ReleaseId; update the migration so that after computing the new UUID you persist
it through the active jobs repository used for runtime persistence as well
(i.e., in addition to s.repo.Jobs.Set(job) also call the active/DB-backed jobs
persistence method the service uses when WithDBJobs is enabled — the same
persistent setter used elsewhere for active jobs), ensuring you reference
job.ReleaseId and use the same error handling/logging strategy as the existing
s.repo.Jobs.Set call.

---

Outside diff comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go`:
- Around line 234-237: The test constructs an oapi.Job using
release.ContentHash(), which is legacy; update the job creation to use
release.Id.String() for the ReleaseId field so it matches the rest of the file's
UUID migration (replace release.ContentHash() with release.Id.String() in the
oapi.Job literal where ReleaseId is set, e.g., in the job variable
initialization) and ensure the ReleaseId value remains a string compatible with
oapi.Job.ReleaseId.

In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 92-95: The trace attribute "release.id" in ExecuteRelease is
incorrectly using releaseToDeploy.ContentHash(); update the attribute to use the
release's UUID string by setting attribute.String("release.id",
releaseToDeploy.Id.String()) (locate the tracer.Start call and the attribute
list around ExecuteRelease to change ContentHash() to Id.String()).

---

Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`:
- Around line 112-129: Extract the repeated setup sequence (calls to
testStore.Systems.Upsert, createTestJobAgent, testStore.JobAgents.Upsert,
createTestDeploymentForExecutor + testStore.Deployments.Upsert,
testStore.SystemDeployments.Link, createTestEnvironmentForExecutor +
testStore.Environments.Upsert, testStore.SystemEnvironments.Link) into a single
test helper (e.g., setupTestSystemWithDeploymentAndEnv) that accepts IDs/names
and returns the created entities; replace the duplicated blocks in
executor_test.go (and at the other indicated ranges) with calls to this helper
to reduce duplication and make tests clearer.

In `@apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go`:
- Around line 47-50: Create a single helper (e.g., normalizeReleaseID or
parseOrSHA1UUID) that encapsulates the uid parsing logic: try uuid.Parse(id) and
on error return uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)); have it return a
uuid.UUID and error only if you want to propagate parse failures. Replace the
duplicated blocks inside the Get and Remove methods (and the other occurrence
around lines 140-143) to call this helper so both read and delete paths share
identical normalization behavior; update callers to use the helper's returned
uuid.

ℹ️ 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a514a86 and efed6ca.

📒 Files selected for processing (53)
  • apps/workspace-engine/pkg/oapi/persistence.go
  • apps/workspace-engine/pkg/persistence/integration_test.go
  • apps/workspace-engine/pkg/workspace/jobs/factory.go
  • apps/workspace-engine/pkg/workspace/jobs/factory_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/action/orchestrator.go
  • apps/workspace-engine/pkg/workspace/releasemanager/action/orchestrator_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/action/rollback/hooks.go
  • apps/workspace-engine/pkg/workspace/releasemanager/action/rollback/rollback.go
  • apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification.go
  • apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility.go
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility_retry_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/planner.go
  • apps/workspace-engine/pkg/workspace/releasemanager/manager.go
  • apps/workspace-engine/pkg/workspace/releasemanager/manager_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_policy_action.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentwindow/deploymentwindow_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environment_progression_action.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/smart_defaults_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/rollback/rollback_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/version_summary_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/versioncooldown_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/state_index_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/verification/executor_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/verification/manager_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/verification/recorder_test.go
  • apps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.go
  • apps/workspace-engine/pkg/workspace/store/jobs.go
  • apps/workspace-engine/pkg/workspace/store/release_targets_test.go
  • apps/workspace-engine/pkg/workspace/store/releases.go
  • apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go
  • apps/workspace-engine/pkg/workspace/store/repository/memory/indexstore/schema_release.go
  • apps/workspace-engine/pkg/workspace/store/store.go
  • apps/workspace-engine/svc/controllers/desiredrelease/reconcile.go
  • apps/workspace-engine/svc/controllers/jobdispatch/reconcile.go
  • apps/workspace-engine/test/controllers/pipeline_test.go
  • apps/workspace-engine/test/e2e/engine_job_verification_query_test.go
  • apps/workspace-engine/test/e2e/engine_nil_release_handling_test.go
  • apps/workspace-engine/test/e2e/engine_releasemanager_test.go
  • apps/workspace-engine/test/e2e/engine_releasetarget_state_test.go
  • apps/workspace-engine/test/e2e/engine_verification_hooks_test.go


// Create a pending job for this release target
job := createTestJob(release.ContentHash(), oapi.JobStatusPending)
job := createTestJob(release.Id.String(), oapi.JobStatusPending)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Assert release upsert success before using release.Id.String()

Line 106, Lines 269-272, and Line 326 now depend on release IDs populated by the prior release upsert, but those upsert errors are discarded. If setup fails, these tests can proceed with invalid IDs and produce misleading outcomes. Please fail fast with require.NoError(...) on each release upsert.

Suggested test setup fix
-	_ = testStore.Releases.Upsert(ctx, release)
+	require.NoError(t, testStore.Releases.Upsert(ctx, release))
-	_ = testStore.Releases.Upsert(ctx, release)
+	require.NoError(t, testStore.Releases.Upsert(ctx, release))
-	_ = testStore.Releases.Upsert(ctx, release2)
+	require.NoError(t, testStore.Releases.Upsert(ctx, release2))

Also applies to: 269-272, 326-326

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/workspace-engine/pkg/workspace/releasemanager/manager_test.go` at line
106, The test uses release.Id.String() (e.g., when calling createTestJob) but
discards errors from the prior release upsert, so add require.NoError(...)
immediately after each release upsert call that populates release (the same
upsert invocations whose result is used for release.Id.String()) to fail fast on
setup errors; update the test lines around release upsert, including the setup
before createTestJob and the other upsert locations referenced (lines where
release is assigned), to call require.NoError(t, err) (or the project’s test
require helper) before using release.Id.String().


release := createTestRelease(s, ctx)
verification := createTestVerification(s, ctx, release.ContentHash(), 1, 3600)
verification := createTestVerification(s, ctx, release.Id.String(), 1, 3600)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use real job.Id when creating JobVerification fixtures

At Line 272 (and Lines 303, 333, 353, 388, 408, 455, 475, 711, 735, 755), createTestVerification is being passed release.Id.String(), but that parameter is jobId. This produces invalid JobVerification.JobId linkage in test fixtures.

Suggested fix pattern
- release := createTestRelease(s, ctx)
- verification := createTestVerification(s, ctx, release.Id.String(), 1, 3600)
+ release := createTestRelease(s, ctx)
+ job := createTestJob(s, ctx, release.Id.String())
+ verification := createTestVerification(s, ctx, job.Id, 1, 3600)

Apply the same pattern to each affected call site above.

Also applies to: 303-303, 333-333, 353-353, 388-388, 408-408, 455-455, 475-475, 711-711, 735-735, 755-755

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.go`
at line 272, The test fixtures call createTestVerification with
release.Id.String() for the jobId parameter, which yields invalid
JobVerification.JobId links; update each affected call (e.g., the call at
createTestVerification(s, ctx, release.Id.String(), 1, 3600)) to pass the real
job id (job.Id.String()) instead of release.Id.String(), ensuring the local job
variable (the Job under test) is used or created before the call so
JobVerification.JobId correctly references the Job; apply this change to all
listed sites that call createTestVerification and verify the job variable name
matches the surrounding test scope.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/store/restore_test.go (1)

1086-1107: Strengthen expected UUID assertion to avoid post-restore coupling.

Line 1105 derives expectedUUID from the restored object; precomputing from the pre-restore release makes this test stricter against future regressions in migration inputs.

Suggested diff
 func TestStore_Restore_ReleaseIdBackfill(t *testing.T) {
 	ctx := context.Background()
 	namespace := "workspace-" + uuid.New().String()

 	persistenceStore := memory.NewStore()
@@
 	release := &oapi.Release{
@@
 	}
+	expectedUUID := uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash()))

 	changes := persistence.NewChangesBuilder(namespace).
 		Set(release).
 		Build()
@@
 	releases := testStore.Releases.Items()
 	require.Len(t, releases, 1)

-	for _, r := range releases {
-		assert.NotEqual(t, uuid.Nil, r.Id, "Release should have a non-nil Id after restore")
-		expectedUUID := uuid.NewSHA1(uuid.NameSpaceOID, []byte(r.ContentHash()))
-		assert.Equal(t, expectedUUID, r.Id, "Release Id should be deterministic UUID from content hash")
-	}
+	restored := releases[0]
+	assert.NotEqual(t, uuid.Nil, restored.Id, "Release should have a non-nil Id after restore")
+	assert.Equal(t, expectedUUID, restored.Id, "Release Id should be deterministic UUID from content hash")
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/workspace-engine/pkg/workspace/store/restore_test.go` around lines 1086
- 1107, The test currently computes expectedUUID from the restored release
object which couples the assertion to post-restore behavior; before calling
persistenceStore.Save/Load/Restore, compute the deterministic UUID from the
original release's content hash (use uuid.NewSHA1(uuid.NameSpaceOID,
[]byte(release.ContentHash())) or equivalent) and store it in a variable, then
after testStore.Restore assert that each restored release's Id equals that
precomputed expected UUID; update references around
persistence.NewChangesBuilder, persistenceStore.Save/Load, store.New and
testStore.Restore to use this precomputed expected value for the equality
assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/store/restore_test.go`:
- Around line 1086-1107: The test currently computes expectedUUID from the
restored release object which couples the assertion to post-restore behavior;
before calling persistenceStore.Save/Load/Restore, compute the deterministic
UUID from the original release's content hash (use
uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) or equivalent)
and store it in a variable, then after testStore.Restore assert that each
restored release's Id equals that precomputed expected UUID; update references
around persistence.NewChangesBuilder, persistenceStore.Save/Load, store.New and
testStore.Restore to use this precomputed expected value for the equality
assertion.

ℹ️ 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.

📥 Commits

Reviewing files that changed from the base of the PR and between efed6ca and 10d226d.

📒 Files selected for processing (3)
  • apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go
  • apps/workspace-engine/pkg/workspace/store/restore_test.go
  • apps/workspace-engine/pkg/workspace/store/store.go

@adityachoudhari26 adityachoudhari26 merged commit 2fd0b4d into main Mar 3, 2026
9 checks passed
@adityachoudhari26 adityachoudhari26 deleted the convert-release-uuid branch March 3, 2026 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant