Skip to content

fix: Core ID assignment — SHA-256 content-addressed IDs (ADR-006) - #6

Merged
ThePlenkov merged 4 commits into
wave-4-runtime-hostfrom
fix-core-id-assignment
Aug 11, 2026
Merged

ThePlenkov merged 4 commits into
wave-4-runtime-hostfrom
fix-core-id-assignment

Conversation

@ThePlenkov

@ThePlenkov ThePlenkov commented Aug 9, 2026 •

Copy link
Copy Markdown
Contributor

User description

Summary

  • Fixes the P0 spec violation from Wave 1 review (sv-bxf): core's assignId/assignIds used human-readable string derivation instead of SHA-256 content-addressed IDs
  • Created packages/core/src/internal/canonical.ts with computeOperationId (SHA-256 via node:crypto.createHash)
  • Rewrote ids.ts + plan.ts: op-<64 hex> format, spec.id folded into hash context as userId (not used as-is)
  • Exported computeOperationId from core public API
  • Added cross-consistency test: core computeOperationId === ir computeOperationId for identical inputs
  • Core and IR are now consistent per ADR-006

Test plan

  • core: 90 tests pass (up from 79)
  • ir: 87 tests pass (including 6 new cross-consistency tests)
  • full monorepo: 16 projects test+typecheck+build green
  • reviewer finding sv-bxf resolved

Stacked on #5

Generated with Devin


Summary by cubic

Switched core operation IDs to SHA‑256 content‑addressed IDs (op-<64hex>) per ADR‑006, and unified ID/serialization primitives across @sverka/core and @sverka/ir for consistent planning, execution, and compilation.

  • New Features

    • @sverka/core: Deterministic IDs from computeOperationId(kind, name, context) using canonical JSON (sorted keys, compact; objects omit undefined; arrays emit null for undefined; rejects NaN/Infinity/BigInt). Context includes matrix dims, userId (from spec.id), command, and args. True duplicates (same kind/name/context) fail. Legacy derived IDs and matrix suffix logic were removed. Public API exports computeOperationId and canonicalStringify.
    • Planning: Resolves dependsOn strings that reference either op IDs or spec.id aliases; duplicate or unknown aliases fail. Cycle detection runs on resolved op IDs.
    • @sverka/ir: Re‑exports canonicalStringify and computeOperationId from @sverka/core; added a cross‑package test to ensure identical op IDs.
  • Migration

    • IDs are now op-<64hex>; update any consumers expecting readable IDs or previous formatting.
    • dependsOn may reference op IDs or spec.id aliases; aliases must be unique and resolvable.
    • Use computeOperationId from @sverka/core to recompute IDs; @sverka/core and @sverka/ir now produce identical results.

Written for commit fe92e6b. Summary will update on new commits.

Review in cubic


CodeAnt-AI Description

Use stable content-based operation IDs across planning, execution, and compilation

What Changed

  • Operations now receive deterministic op- IDs generated from their content instead of readable command-based IDs or user IDs.
  • Matrix operations get distinct IDs for each combination, while identical operations are rejected as duplicates.
  • dependsOn can reference either generated operation IDs or user-provided aliases; cycles and unresolved references are validated against the resolved IDs.
  • Core exposes the shared ID and canonical serialization helpers so core and IR produce matching results.
  • Canonical serialization now consistently handles key ordering, arrays, special numbers, bigints, and lone surrogate characters.

Impact

✅ Stable operation IDs across planning and execution
✅ Reliable matrix and dependency tracking
✅ Consistent IDs between core and IR

🔄 Retrigger CodeAnt AI Review

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

@codeant-ai

codeant-ai Bot commented Aug 9, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Incremental review completed fe92e6b Aug 11, 2026 · 10:51 10:52
✅ Incremental review completed adb2275 Aug 11, 2026 · 08:32 08:32
✅ Incremental review completed 77cbd9d Aug 11, 2026 · 06:23 06:24
✅ Incremental review completed 6817dad Aug 10, 2026 · 23:52 23:53
✅ Incremental review completed b62958f Aug 10, 2026 · 21:28 21:28

@coderabbitai

coderabbitai Bot commented Aug 9, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@ThePlenkov, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 51ac0094-4f83-46af-862c-c307cd6515ac

📥 Commits

Reviewing files that changed from the base of the PR and between 5908184 and fe92e6b.

📒 Files selected for processing (14)
  • packages/core/src/__tests__/composables/workflow.test.ts
  • packages/core/src/__tests__/dag.test.ts
  • packages/core/src/__tests__/ids.test.ts
  • packages/core/src/__tests__/laziness.test.ts
  • packages/core/src/__tests__/matrix.test.ts
  • packages/core/src/__tests__/public-api.test.ts
  • packages/core/src/__tests__/runtime-modes.test.ts
  • packages/core/src/index.ts
  • packages/core/src/internal/canonical.ts
  • packages/core/src/internal/ids.ts
  • packages/core/src/internal/plan.ts
  • packages/ir/src/__tests__/core-consistency.test.ts
  • packages/ir/src/ids.ts
  • packages/ir/src/internal/canonical.ts
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-core-id-assignment

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

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Running ultrareview automatically — This rewrites core ID assignment and dependency resolution to content-addressed op- IDs, changing public ID semantics and duplicate-operation behavior across every planned workflow — high blast radius if a hashing, canonicalization, or edge-resolution bug slips through.. I'll post findings when complete.

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Aug 9, 2026
@codacy-production

codacy-production Bot commented Aug 9, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -10 complexity · 0 duplication

Metric Results
Complexity -10
Duplication 0

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix core operation ID assignment with ADR-006 SHA-256 content-addressed IDs

🐞 Bug fix 🧪 Tests ✨ Enhancement 🕐 40+ Minutes

Grey Divider

AI Description

• Replace planner-derived human-readable ids with ADR-006 SHA-256 content-addressed op- ids.
• Add canonical JSON serialization and fold user spec.id into hash context as userId.
• Export computeOperationId and add/update tests, including core↔ir consistency coverage.
Diagram

graph TD
  A["Workflow planner (planWorkflow)"] --> B["Assign IDs (assignIds)"] --> C["Canonical JSON (canonicalStringify)"] --> D["SHA-256 (node:crypto)"] --> F[("Operation ids (op-…)")]
  A --> E["Resolve edges (resolveEdges)"] --> G[("Planned specs (dependsOn)")]
  H["IR computeOperationId"] --> I["Core/IR consistency test"] --> D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Create shared canonical/id package
  • ➕ Eliminates duplicated canonicalStringify logic between core and ir
  • ➕ Single place to evolve ADR-006 behavior with shared tests
  • ➖ Introduces a new shared package/dependency edge
  • ➖ Requires careful versioning to avoid tightening core’s dependency surface
2. Use a deterministic stringify library
  • ➕ Less bespoke canonicalization code to maintain
  • ➕ Likely broader edge-case handling
  • ➖ Adds a dependency to core (size/supply-chain/compatibility risk)
  • ➖ Harder to guarantee byte-identical output required by ADR-006 long term
3. Keep spec.id as alias-only (not in hash context)
  • ➕ op- ids remain stable even if user alias changes
  • ➕ Clearer separation of identity vs. aliasing
  • ➖ Would diverge from the chosen ADR-006 interpretation in this PR
  • ➖ Increases risk of collisions for otherwise-identical operations without extra discriminators

Recommendation: The PR’s approach is appropriate for ADR-006: core stays dependency-light (node:crypto only), ids are fully content-addressed, and the new core↔ir consistency test is the right long-term guardrail against drift. Consider a shared package only if maintaining two canonicalizers becomes a recurring cost.

Files changed (12) +462 / -124

Enhancement (2) +136 / -0
index.tsExport computeOperationId from core public API +1/-0

Export computeOperationId from core public API

• Adds computeOperationId to the @sverka/core public barrel to support consumers recomputing ids from content.

packages/core/src/index.ts

canonical.tsIntroduce canonical JSON serializer used for stable hashing (ADR-006) +135/-0

Introduce canonical JSON serializer used for stable hashing (ADR-006)

• Adds a manual canonical JSON emitter with sorted object keys, omission of undefined fields, and strict handling of non-JSON numbers. Documented as an independent copy of IR’s canonicalization, with drift guarded by tests.

packages/core/src/internal/canonical.ts

Bug fix (2) +126 / -81
ids.tsImplement computeOperationId via canonical JSON + SHA-256; remove legacy id derivation +22/-52

Implement computeOperationId via canonical JSON + SHA-256; remove legacy id derivation

• Deletes the prior derived-id and matrix-child suffix logic and replaces it with computeOperationId(kind,name,context) using node:crypto sha256. Retains isKnownKind validation helper.

packages/core/src/internal/ids.ts

plan.tsRework ID assignment and dependsOn resolution for content-addressed ids +104/-29

Rework ID assignment and dependsOn resolution for content-addressed ids

• Assigns op ids via computeOperationId using nameFor() and contextFor() (matrix dims, userId alias, command, args) and rejects true duplicates by construction. Adds aliasMap and resolves dependsOn strings as either op ids or spec.id aliases, erroring on dangling references.

packages/core/src/internal/plan.ts

Tests (8) +200 / -43
workflow.test.tsUpdate workflow planning assertions to op-<64hex> ids +9/-7

Update workflow planning assertions to op-<64hex> ids

• Stops asserting old human-readable ids and instead validates op-<64hex> format for planned operations. Updates dependsOn expectations to reference the computed op ids.

packages/core/src/tests/composables/workflow.test.ts

dag.test.tsAlign DAG tests with alias resolution and content-addressed collisions +37/-16

Align DAG tests with alias resolution and content-addressed collisions

• Updates wording and expectations to treat spec.id as a dependsOn alias resolved to op ids. Adds a new test that identical operations collide (same op id) and surface an op id in CompositionError context.

packages/core/src/tests/dag.test.ts

ids.test.tsAdd computeOperationId unit tests +67/-0

Add computeOperationId unit tests

• Introduces coverage for id format, determinism, key-order independence, and variation across kind/name/context. Verifies output matches a manual sha256 over canonical JSON input.

packages/core/src/tests/ids.test.ts

laziness.test.tsAvoid true-duplicate ops in laziness test under content-addressed ids +5/-1

Avoid true-duplicate ops in laziness test under content-addressed ids

• Changes the workflow roots to use distinct operations so planning doesn't reject duplicates. Keeps the focus on ensuring no filesystem/process/network I/O occurs.

packages/core/src/tests/laziness.test.ts

matrix.test.tsUpdate matrix tests to expect content-addressed op ids +21/-10

Update matrix tests to expect content-addressed op ids

• Replaces suffix-based id assertions with op-id regex and uniqueness checks. Adds expectations that ids match computeOperationId over the expected context fields.

packages/core/src/tests/matrix.test.ts

public-api.test.tsValidate computeOperationId is public and internals stay private +18/-1

Validate computeOperationId is public and internals stay private

• Extends the internal-leak check list and adds an explicit test that computeOperationId is exported and returns op-<64hex> ids.

packages/core/src/tests/public-api.test.ts

runtime-modes.test.tsUpdate runtime mode expectations to use planned op ids +15/-8

Update runtime mode expectations to use planned op ids

• Makes execute/compile/condition tests compare evaluated ids to plan ids rather than hard-coded strings. Ensures artifacts and evaluated ids follow op-<64hex> format.

packages/core/src/tests/runtime-modes.test.ts

core-consistency.test.tsAdd core↔ir computeOperationId consistency tests (ADR-006) +28/-0

Add core↔ir computeOperationId consistency tests (ADR-006)

• Adds parameterized tests ensuring core and ir implementations produce byte-identical op ids for representative inputs, preventing canonicalization/hashing drift.

packages/ir/src/tests/core-consistency.test.ts

Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

The PR implementation of SHA-256 content-addressed IDs is currently not up to standards, primarily due to a logic flaw in the ID context generation and failure of cross-package consistency checks.

Critical Blocking Issues

  • ID Collision Risk: The 'contextFor' function in 'plan.ts' omits critical fields like 'env', 'image', and 'workingDir'. This causes different operations to hash to the same ID, which will trigger composition errors or incorrect cache behavior.
  • Test Resolution Failure: The consistency test in the IR package cannot resolve the core module, meaning the cross-package verification required by ADR-006 is not actually executing in CI.
  • Complexity & Coverage: 'packages/core/src/internal/canonical.ts' has been identified as a complex file with significant uncovered logic, posing a maintenance risk for such a critical component of the hashing system.
  • Platform Compatibility: The use of 'node:crypto' and ES2017 features (like 'padStart') may break the package's requirements for isomorphic or legacy environment support.

About this PR

  • The implementation of 'canonicalStringify' is manually duplicated between the core and ir packages. While this avoids circular dependencies, any future changes to the canonical format (ADR-006) must be manually synced in both locations to prevent ID mismatches.

Test suggestions

  • Verify computeOperationId produces the same hash for identical inputs regardless of key insertion order in context
  • Verify matrix expansion produces distinct, deterministic IDs for each combination
  • Verify planning fails when duplicate user-provided spec.id aliases are defined
  • Verify planning fails when two operations have identical kind, name, and context (true duplicates)
  • Verify dependsOn strings correctly resolve user-provided spec.id aliases to the resulting op- IDs
  • Verify cross-package consistency: core and ir implementations of computeOperationId produce identical outputs
  • Verify canonicalStringify correctly escapes control characters and quotes per JSON spec
  • Address code coverage gaps in 'packages/core/src/internal/canonical.ts'
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Address code coverage gaps in 'packages/core/src/internal/canonical.ts'

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

Comment thread packages/ir/src/__tests__/core-consistency.test.ts
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/ids.ts
Comment thread packages/core/src/internal/canonical.ts Outdated
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts
@qodo-code-review

qodo-code-review Bot commented Aug 9, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Missing positional index causes false duplicate rejection 🐞 Bug ≡ Correctness
Description
Spec 01-core §ID assignment requires the hash context to include a positional discovery index,
and the operation identity should distinguish execution-affecting configuration, but the new
contextFor() in plan.ts omits the index and only includes matrix combo, userId, command, and
args. This makes repeated or semantically different operations hash to the same op-<hash> and,
because assignIds() rejects any collision, causes planWorkflow to throw
CompositionError('duplicate operation id ...') for workflows that previously planned successfully
(old behavior disambiguated with a counter suffix).
Code

packages/core/src/internal/plan.ts[R295-308]

+function contextFor(node: OperationNode): Record<string, unknown> {
+  const s = node.spec;
+  const ctx: Record<string, unknown> = {};
+  const combo = (node as unknown as Record<string, unknown>).__matrixCombo as
+    | readonly [string, unknown][]
+    | undefined;
+  if (combo !== undefined) {
+    for (const [k, v] of combo) ctx[k] = v;
+  }
+  if (s.id !== undefined) ctx["userId"] = s.id;
+  if (s.command !== undefined) ctx["command"] = s.command;
+  if (s.args !== undefined) ctx["args"] = [...s.args];
+  return ctx;
}
Evidence
The spec text in specs/01-core/spec.md (lines 384–392) explicitly calls out a positional index
within the discovery walk as part of the context used for ID assignment, indicating IDs should be
distinct “by construction” even when operations share kind/name and overlap in other fields;
however, the shipped contextFor() implementation only carries matrix dimensions, user ID, command,
and args, omitting any index and other execution-affecting fields present in the operation model
(e.g., env, image, workingDir, credentials, timeout). With assignIds() rejecting every hash
collision, two operations that are repeated in a workflow or differ only in those omitted
configuration fields can legitimately exist yet still collide and fail planning, and the new test in
packages/core/src/__tests__/dag.test.ts (“rejects true duplicate operations (identical
kind/name/context)”) codifies collision rejection as expected behavior, which diverges from the
spec’s stated discriminator and from the prior counter-suffix behavior that allowed repeated steps
to coexist.

specs/01-core/spec.md[384-392]
packages/core/src/internal/plan.ts[245-275]
packages/core/src/internal/plan.ts[258-260]
packages/core/src/internal/plan.ts[295-307]
packages/core/src/operation.ts[18-42]
specs/01-core/plan.md[151-158]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`contextFor()` in `packages/core/src/internal/plan.ts` computes the hash context used by `computeOperationId`, but it omits discriminators required/expected by the ID assignment contract: the spec mandates including a positional discovery `index`, and there are additional execution-affecting operation fields (e.g., `env`, `image`, `workingDir`, credentials, timeout) that are not represented in the current context. Because `assignIds()` rejects any ID collision, this overly narrow context causes repeated operations or semantically different operations to hash to the same `op-<sha256>` ID and fail planning with `CompositionError('duplicate operation id ...')`, even for workflows that previously planned successfully.

## Issue Context
- `specs/01-core/spec.md` (§ID assignment) states the context must include a positional `index` within the discovery walk, specifically to ensure distinct IDs by construction.
- The current `contextFor()` includes only matrix combo, `userId`, `command`, and `args`, so structurally identical operations used twice in a workflow (e.g., two unnamed `run({ command: "test" })` nodes reused for repeated/retry steps) collide.
- The operation model contains many fields that affect execution; if positional identity is not intended, the identity contract may need to include those fields in canonical form to avoid collisions between semantically different operations.
- A new test in `packages/core/src/__tests__/dag.test.ts` (“rejects true duplicate operations (identical kind/name/context)”) treats these collisions as expected; the spec, implementation, and tests need to be reconciled so they agree on whether IDs are positional (include index) and/or configuration-sensitive (include execution-affecting fields).

## Fix Focus Areas
- packages/core/src/internal/plan.ts[245-275]
- packages/core/src/internal/plan.ts[289-308]
- packages/core/src/internal/plan.ts[258-260]
- packages/core/src/internal/plan.ts[295-307]
- specs/01-core/spec.md[384-392]
- packages/core/src/__tests__/dag.test.ts[47-60]
- packages/core/src/operation.ts[18-42]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Matrix user IDs collide 🐞 Bug ≡ Correctness
Description
Every expanded matrix child inherits the template's spec.id, but the new alias map rejects the
second occurrence. Consequently, any multi-value matrix(..., run({ id: ... })) fails planning even
though matrix dimensions give its children distinct content-addressed IDs.
Code

packages/core/src/internal/plan.ts[R264-266]

+    if (node.spec.id !== undefined) {
+      if (aliasMap.has(node.spec.id)) {
+        throw new CompositionError(
Evidence
Expansion copies the full spec into each child, including spec.id; matrix dimensions make child
hashes distinct, but alias uniqueness independently rejects the repeated template alias. The core
specification allows user IDs to influence hashes and defines matrix expansion without prohibiting
that combination.

packages/core/src/internal/plan.ts[141-149]
packages/core/src/internal/plan.ts[264-272]
packages/core/src/internal/plan.ts[295-306]
specs/01-core/spec.md[394-412]
specs/01-core/spec.md[420-430]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Matrix expansion copies one user ID to every child, and global alias uniqueness makes the resulting valid matrix unplannable.

## Issue Context
Define template-alias semantics for expanded children, such as resolving the template alias to all child IDs or deriving unambiguous child aliases. Do not reject repeated aliases that originate from one matrix template, and add a two-value matrix regression test with an explicit `spec.id`.

## Fix Focus Areas
- packages/core/src/internal/plan.ts[141-149]
- packages/core/src/internal/plan.ts[264-272]
- packages/core/src/internal/plan.ts[295-306]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Resolved name not emitted 🐞 Bug ≡ Correctness
Description
The planner hashes the command fallback from nameFor(node), but emits name: "" when spec.name
is absent. A consumer recomputing the ID from the fully resolved operation therefore gets a
different ID for common inputs such as run({ command: "build" }).
Code

packages/core/src/internal/plan.ts[258]

+    const id = computeOperationId(node.kind, nameFor(node), contextFor(node));
Evidence
The hash uses nameFor, whose fallback is spec.command, while buildSpec emits an empty name.
ADR-006 requires operation IDs to be reproducible by tools reading the plan.

packages/core/src/internal/plan.ts[258-258]
packages/core/src/internal/plan.ts[282-286]
packages/core/src/internal/plan.ts[368-374]
engdocs/adr/ADR-006-sha256-content-addressed-plan-ids.md[5-10]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The operation ID is computed with `nameFor(node)`, but the emitted operation may contain a different `name`, preventing ID reproduction from plan content.

## Issue Context
For operations without `spec.name`, `nameFor` uses `spec.command` while `buildSpec` currently emits an empty string. Use one resolved name for both hashing and emission, and add a regression test that recomputes the emitted operation's ID.

## Fix Focus Areas
- packages/core/src/internal/plan.ts[258-258]
- packages/core/src/internal/plan.ts[282-286]
- packages/core/src/internal/plan.ts[368-374]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Alias shadows generated ID 🐞 Bug ≡ Correctness
Description
resolveDep gives generated IDs precedence over unrestricted user aliases. If operation A's
spec.id equals operation B's generated ID, a dependency intended for A silently targets B and
produces the wrong DAG.
Code

packages/core/src/internal/plan.ts[R354-356]

+  if (knownOpIds.has(dep)) return dep;
+  const aliased = aliasMap.get(dep);
+  if (aliased !== undefined) return aliased;
Evidence
Aliases are accepted without comparison to generated IDs, while dependency resolution checks
generated IDs first. Since computeOperationId is public, the overlapping value can be constructed
deterministically.

packages/core/src/internal/plan.ts[264-272]
packages/core/src/internal/plan.ts[349-356]
packages/core/src/index.ts[24-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A dependency string can match both a generated operation ID and a user alias, and the current precedence silently resolves it to the wrong node.

## Issue Context
After collecting all generated IDs and aliases, reject any alias that overlaps the generated-ID namespace. Add a regression test where one operation's alias equals another operation's public `computeOperationId` result.

## Fix Focus Areas
- packages/core/src/internal/plan.ts[264-272]
- packages/core/src/internal/plan.ts[349-356]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

5. spec.id alias silently rewrites unrelated dependsOn strings 🐞 Bug ☼ Reliability
Description
resolveDep() treats any user dependsOn string that isn't already a known op- id as an alias
lookup in aliasMap, and raises an error only if no operation happens to declare that spec.id;
this means a typo'd or stale dependsOn value that accidentally matches another unrelated
operation's spec.id silently creates a wrong dependency edge instead of failing loudly, and there
is no way to reference a truly external/foreign id since any non-matching string throws.
Code

packages/core/src/internal/plan.ts[R349-360]

+function resolveDep(
+  dep: string,
+  aliasMap: Map<string, string>,
+  knownOpIds: Set<string>,
+): string {
+  if (knownOpIds.has(dep)) return dep;
+  const aliased = aliasMap.get(dep);
+  if (aliased !== undefined) return aliased;
+  throw new CompositionError(`unresolved dependsOn reference '${dep}'`, {
+    dependsOn: dep,
+  });
+}
Evidence
resolveDep (packages/core/src/internal/plan.ts lines 349-360) has only two success paths — exact
op- id match or alias-map match — with no additional validation that the alias was intended for
this specific dependsOn reference, so any coincidental id collision across unrelated operations in a
large workflow resolves silently to the wrong edge.

packages/core/src/internal/plan.ts[343-360]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`resolveDep()` resolves a `dependsOn` string against `aliasMap` (user-provided `spec.id` values) whenever it isn't already a known `op-` id, with no additional disambiguation. In large workflows composed from multiple modules, this creates a risk that an unrelated operation's `spec.id` accidentally satisfies a `dependsOn` reference intended for a different (perhaps typo'd or removed) operation, producing a silently wrong dependency edge rather than an error.

## Issue Context
This is a low-probability, hard-to-detect issue since `spec.id` collisions are already rejected earlier in `assignIds()`, but cross-file/cross-module composition could still lead to accidental reuse of common alias names (e.g. "build", "test").

## Fix Focus Areas
- packages/core/src/internal/plan.ts[343-360]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🧠 Deep: This is a high-density behavioral change spanning core ID generation, canonical serialization, planning, dependency alias resolution, public API, IR consistency, and many independent test paths; subtle cross-package or edge-resolution defects could be missed in one pass.

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts
@baz-reviewer

baz-reviewer Bot commented Aug 9, 2026 •

Copy link
Copy Markdown

Merger

Needs Review

The shipped planner rejects any multi-value matrix operation carrying a user-provided spec.id because every expanded child reuses the alias. This contradicts the PR’s claimed support for distinct matrix IDs and prevents valid workflows from being planned.

Commit fe92e6b · Evaluated 2026-08-11 10:52 UTC

Review this PR on Baz | Customize your next review

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ultrareview completed in 9m 37s

5 issues found across 12 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="packages/core/src/internal/ids.ts">

<violation number="1" location="packages/core/src/internal/ids.ts:25">
P2: Operation IDs collide when a name or string context value contains an unpaired surrogate: the canonical serializer writes it raw, and the UTF-8 hash conversion replaces it with U+FFFD. As a result, for example, `"\uD800"` and `"\uFFFD"` hash to the same operation ID despite being distinct inputs. Escaping unpaired surrogates in `canonicalStringify` (as `JSON.stringify` does), or rejecting them before hashing, would preserve content addressing.</violation>
</file>

<file name="packages/core/src/internal/plan.ts">

<violation number="1" location="packages/core/src/internal/plan.ts:265">
P1: A matrix operation with a user-provided `id` can no longer be planned: every expanded child inherits that ID, and the second child is treated as a duplicate alias even though its matrix context gives it a distinct `op-` ID. The alias representation needs to account for a matrix alias resolving to multiple child IDs (and expand a dependency on it accordingly), or matrix-child aliases need to be handled separately so an otherwise valid matrix does not fail during ID assignment.</violation>

<violation number="2" location="packages/core/src/internal/plan.ts:306">
P1: Two otherwise identical operations that use different environments are now rejected as duplicate operations, even though their emitted specs execute with different `env` values. `contextFor()` only hashes `command` and `args` beyond the name, so it needs to include the other execution-discriminating fields (such as `env`, image, working directory, and runtime policy) or retain another documented uniqueness discriminator before duplicate detection.</violation>

<violation number="3" location="packages/core/src/internal/plan.ts:354">
P2: A user alias that happens to equal an existing `op-` ID is silently resolved as the generated ID, so a `dependsOn` edge can point at the wrong operation. Since `spec.id` accepts arbitrary strings and aliases are now supported, detect this namespace collision and reject it (or introduce an unambiguous alias syntax) instead of giving generated IDs implicit precedence.</violation>
</file>

<file name="packages/ir/src/__tests__/core-consistency.test.ts">

<violation number="1" location="packages/ir/src/__tests__/core-consistency.test.ts:21">
P3: The cross-consistency test only asserts `core === ir`, so it guards against one copy drifting but cannot detect a correlative drift where both copies change identically (e.g., a shared change to the `op-` prefix or the hashing/canonicalization). Since ids.test.ts already validates the `op-` + 64-hex format for the ir copy, consider anchoring at least one Consistency test case to a golden expected hash (computed from ADR-006) — or asserting the format directly here — so a simultaneous regression in both packages is caught, not just divergence between them.</violation>
</file>

Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/core/src/internal/ids.ts
Comment thread packages/core/src/internal/plan.ts
Comment thread packages/ir/src/__tests__/core-consistency.test.ts
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 909e11f to 6eb38ac Compare August 9, 2026 21:44
@ThePlenkov ThePlenkov mentioned this pull request Aug 10, 2026
9 tasks done
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 6eb38ac to c29c404 Compare August 10, 2026 20:19
@codeant-ai codeant-ai Bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:L This PR changes 100-499 lines, ignoring generated files labels Aug 10, 2026
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from c29c404 to f3f1bb3 Compare August 10, 2026 20:26
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from f3f1bb3 to 62b3a48 Compare August 10, 2026 20:44
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 62b3a48 to 9e01481 Compare August 10, 2026 20:52
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 9e01481 to 861df23 Compare August 10, 2026 20:57
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 861df23 to 0713582 Compare August 10, 2026 21:05
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from b62958f to 6817dad Compare August 10, 2026 23:52
@codeant-ai codeant-ai Bot added size:XXL This PR changes 1000+ lines, ignoring generated files and removed size:L This PR changes 100-499 lines, ignoring generated files labels Aug 10, 2026
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch 2 times, most recently from d31ef9d to b11116b Compare August 11, 2026 00:09
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from de7531f to 77cbd9d Compare August 11, 2026 06:23
@codeant-ai codeant-ai Bot added size:XXL This PR changes 1000+ lines, ignoring generated files and removed size:XXL This PR changes 1000+ lines, ignoring generated files labels Aug 11, 2026
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 77cbd9d to adb2275 Compare August 11, 2026 08:32
@codeant-ai codeant-ai Bot added size:XL This PR changes 500-999 lines, ignoring generated files and removed size:XXL This PR changes 1000+ lines, ignoring generated files labels Aug 11, 2026
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from adb2275 to 7e355f1 Compare August 11, 2026 08:54
ThePlenkov and others added 4 commits August 11, 2026 10:50
Core's assignId/assignIds used human-readable string derivation
(kind:name-or-command-or-index with collision counter) and passed
user spec.id through as-is, violating ADR-006. Now uses SHA-256
content-addressed ids: op-<64 hex> via node:crypto.createHash over
canonical JSON of {kind, name, context}, with spec.id folded into
hash context as userId.

- Created packages/core/src/internal/canonical.ts (computeOperationId)
- Rewrote packages/core/src/internal/ids.ts + plan.ts for SHA-256
- Exported computeOperationId from core public index.ts
- Fixed all tests to assert op-<64hex> format (90 core tests pass)
- Added cross-consistency test: core === ir computeOperationId (87 ir tests)
- Full monorepo: 16 projects test+typecheck+build green

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… surrogates) and update runtime-modes expectations

Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
@ThePlenkov
ThePlenkov force-pushed the fix-core-id-assignment branch from 7e355f1 to fe92e6b Compare August 11, 2026 10:51
@codeant-ai codeant-ai Bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:XL This PR changes 500-999 lines, ignoring generated files labels Aug 11, 2026
@sonarqubecloud

Copy link
Copy Markdown

@ThePlenkov
ThePlenkov merged commit f9f233b into main Aug 11, 2026
4 checks passed
@ThePlenkov
ThePlenkov deleted the fix-core-id-assignment branch August 11, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

baz: needs review size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant