fix(core): migrate error serialization surfaces#703
Merged
rafaelscosta merged 1 commit intomainfrom May 8, 2026
Merged
Conversation
rafaelscosta
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
area: agents
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements AIOX-ERROR.2: BuildStateManager and PipelineMetrics.errorLayer now normalize errors to a canonical Error Governance contract, storing structured ChangesError Governance: Serialization-Sensitive Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
rafaelscosta
force-pushed
the
feature/issue-621-error-normalizer-migration
branch
from
85066b5 to
f3f6e26
Compare
rafaelscosta
force-pushed
the
feature/issue-621-error-normalizer-migration
branch
from
f3f6e26 to
4170e74
Compare
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/data/entity-registry.yaml:
- Line 8714: The registry lists a dependency key "errors" that doesn't exist
(the defined key is "errors-index"), which breaks dependency resolution; update
the dependency entry that references "errors" to use the correct module/entity
key "errors-index" at both occurrences (the ones noted in the review), or
alternatively add a new entity definition named "errors" if the alias was
intentional, ensuring the dependency names match exactly with the keys defined
in the registry.
- Around line 12922-12941: The synapse-engine registry entry is missing the
standard module fields expected by validators; update the synapse-engine record
to include externalDeps (e.g., an empty list or the actual external package
names), plannedDeps (e.g., [] or any planned internal dependencies), and
lifecycle (e.g., an object with status, created/updated timestamps or the same
structure used by other module entries) so the entry matches the schema used by
other modules; modify the .aiox-core/data/entity-registry.yaml synapse-engine
entry to add externalDeps, plannedDeps, and lifecycle keys with sensible default
values consistent with other module records.
🪄 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: 0c8d90d5-3bcc-40a1-a526-2e91c3320927
📒 Files selected for processing (8)
.aiox-core/core/execution/build-state-manager.js.aiox-core/core/synapse/engine.js.aiox-core/data/entity-registry.yaml.aiox-core/install-manifest.yamldocs/stories/epic-error-governance/EPIC-AIOX-ERROR-GOVERNANCE.mddocs/stories/epic-error-governance/STORY-AIOX-ERROR.2-SERIALIZATION-SENSITIVE-MIGRATION.mdtests/core/build-state-manager.test.jstests/synapse/engine.test.js
rafaelscosta
force-pushed
the
feature/issue-621-error-normalizer-migration
branch
from
4170e74 to
63e3b6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/data/entity-registry.yaml (1)
11459-11464:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
context-builderlifecycle conflicts with new active usage.
context-buildernow hassynapse-engineinusedBy(Line 11459), but lifecycle is stillorphan(Line 11464). That inconsistency can mislead tooling that treats orphaned entries as removable/inactive.💡 Proposed minimal fix
context-builder: path: .aiox-core/core/synapse/context/context-builder.js layer: L1 type: module purpose: Entity at .aiox-core/core/synapse/context/context-builder.js keywords: - context - builder usedBy: - synapse-engine dependencies: [] externalDeps: [] plannedDeps: [] - lifecycle: orphan + lifecycle: experimental🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/data/entity-registry.yaml around lines 11459 - 11464, The registry entry for context-builder is inconsistent: it lists synapse-engine under usedBy but retains lifecycle: orphan; update the lifecycle value for the context-builder entry from "orphan" to the appropriate active state (e.g., "active" or "in-use") so the usedBy relationship is reflected correctly, ensuring you edit the same context-builder block that contains the usedBy: - synapse-engine and lifecycle fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/core/ids/registry-updater.js:
- Around line 345-347: The code currently only backfills defaults for
externalDeps, plannedDeps, and lifecycle instead of recomputing them after
dependencies change; after any place that refreshes the dependencies array (the
blocks that assign/update dependencies around the existing refresh points),
rederive externalDeps, plannedDeps and lifecycle from the new dependencies
rather than setting static defaults—i.e., replace the default assignments for
externalDeps/plannedDeps/lifecycle with calls to the existing
dependency-derivation logic (or implement a small helper) so these fields are
recalculated whenever dependencies is updated (apply this fix at both update
points where dependencies is refreshed).
In @.aiox-core/development/scripts/populate-entity-registry.js:
- Around line 115-117: findScanConfigForPath currently uses simple prefix
matching which can misidentify siblings (e.g., ".aiox-core/corex" matching
".aiox-core/core"); normalize config.basePath and relPath (trim trailing
slashes, replace backslashes) and change the test to accept either exact
equality or a directory-bound prefix (i.e., relPath === basePath OR
relPath.startsWith(basePath + '/')) when iterating SCAN_CONFIG to return the
correct config.
---
Outside diff comments:
In @.aiox-core/data/entity-registry.yaml:
- Around line 11459-11464: The registry entry for context-builder is
inconsistent: it lists synapse-engine under usedBy but retains lifecycle:
orphan; update the lifecycle value for the context-builder entry from "orphan"
to the appropriate active state (e.g., "active" or "in-use") so the usedBy
relationship is reflected correctly, ensuring you edit the same context-builder
block that contains the usedBy: - synapse-engine and lifecycle fields.
🪄 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: 97506711-f3d9-4131-8618-52ba428484c0
📒 Files selected for processing (11)
.aiox-core/core/execution/build-state-manager.js.aiox-core/core/ids/registry-updater.js.aiox-core/core/synapse/engine.js.aiox-core/data/entity-registry.yaml.aiox-core/development/scripts/populate-entity-registry.js.aiox-core/install-manifest.yamldocs/stories/epic-error-governance/EPIC-AIOX-ERROR-GOVERNANCE.mddocs/stories/epic-error-governance/STORY-AIOX-ERROR.2-SERIALIZATION-SENSITIVE-MIGRATION.mdtests/core/build-state-manager.test.jstests/core/ids/populate-entity-registry.test.jstests/synapse/engine.test.js
✅ Files skipped from review due to trivial changes (2)
- docs/stories/epic-error-governance/EPIC-AIOX-ERROR-GOVERNANCE.md
- .aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- .aiox-core/core/synapse/engine.js
- tests/synapse/engine.test.js
- .aiox-core/core/execution/build-state-manager.js
- tests/core/build-state-manager.test.js
rafaelscosta
force-pushed
the
feature/issue-621-error-normalizer-migration
branch
from
63e3b6e to
8bbb7d2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/core/ids/registry-updater.js (1)
301-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLifecycle values become stale when entities are deleted due to unmutated
changedEntities.When
_handleFileDeleteremoves an entity, it filters that entity's ID fromusedByacross all referencing entities (lines 434–440, 452–458). However, those referencing entities are not added tochangedEntitiesbecause theaction !== 'unlink'guard at line 280 excludes deletes. Since_refreshDerivedDependencyFieldsonly recomputes lifecycle for entities inchangedEntities, anddetectLifecycledepends onusedBylength (line 576–578 in populate-entity-registry.js), their lifecycle values will not update. This causes persisted lifecycles to drift on delete-only batches: an entity might remain marked "production" even after its last consumer is deleted.Consider adding deleted entity IDs to
changedEntitiesor running a full lifecycle recomputation after delete operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/ids/registry-updater.js around lines 301 - 313, The delete path is leaving lifecycle values stale because _handleFileDelete removes IDs from usedBy but the deleted references aren't added to changedEntities due to the action !== 'unlink' guard, so _refreshDerivedDependencyFields (which calls detectLifecycle in populate-entity-registry.js) never recomputes those consumers; fix by ensuring deleted entity IDs (or the referencing entity IDs) are added to changedEntities when processing unlink/deletes in _handleFileDelete, or alternatively trigger a full lifecycle recompute after deletes by invoking _refreshDerivedDependencyFields for all affected entities (or the whole registry) after _applyCodeIntelEnrichments/_resolveAllUsedBy; update the logic around the action !== 'unlink' check and references to changedEntities accordingly.
🧹 Nitpick comments (2)
.aiox-core/core/ids/registry-updater.js (2)
278-291: ⚡ Quick winAvoid recomputing
entityIdin the batch loop.
resolveEntityIdis already invoked inside_handleFileCreate(L350) and_handleFileModify(L390) and is recomputed here a third time per mutation. For batches with many files this duplicates non-trivial path resolution work. Consider returning{ mutated, category, entityId }from the handlers and using that directly.♻️ Sketch
- let mutated = false; + let result = { mutated: false }; switch (action) { case 'add': - mutated = this._handleFileCreate(registry, abs); + result = this._handleFileCreate(registry, abs); break; case 'change': - mutated = this._handleFileModify(registry, abs); + result = this._handleFileModify(registry, abs); break; case 'unlink': - mutated = this._handleFileDelete(registry, abs); + result = this._handleFileDelete(registry, abs); break; default: console.warn(`[IDS-Updater] Unknown action: ${action}`); } - if (mutated) { + if (result.mutated) { updated++; - if (action !== 'unlink') { - const relPath = path.relative(this._repoRoot, abs).replace(/\\/g, '/'); - const config = this._detectCategory(relPath); - if (config) { - const category = config.category; - const entityId = resolveEntityId(abs, config, registry.entities[category] || {}, this._repoRoot); - if (registry.entities[category]?.[entityId]) { - changedEntities.push({ category, entityId }); - } - } - } + if (action !== 'unlink' && result.category && result.entityId) { + changedEntities.push({ category: result.category, entityId: result.entityId }); + } }(Handlers would return their already-known
category/entityIdalong withmutated.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/ids/registry-updater.js around lines 278 - 291, The batch loop is recomputing entityId with resolveEntityId even though _handleFileCreate and _handleFileModify already compute it; change those handlers to return an object like { mutated, category, entityId } (or null/false when not mutated) and update the batch-processing code that currently inspects mutated to consume the returned category and entityId directly instead of calling resolveEntityId again, then push { category, entityId } to changedEntities when present; refer to _handleFileCreate, _handleFileModify, resolveEntityId, mutated, category, entityId, and changedEntities when making the change.
411-418: 💤 Low valueOrder-sensitive comparison may over-trigger writes.
Two concerns on L416:
JSON.stringifyis order-sensitive — re-ordered (but semantically equal) dependency arrays fromdetectDependencieswill be treated as different.- After the first batch,
existing.dependenciesis stripped to internal-only by_refreshDerivedDependencyFields, whilenextDependencieshere is the full unclassified list fromdetectDependencies. For any entity with external or planned deps, this comparison is effectively always!==, so the branch always runs and re-triggers a refresh on every unchanged-checksum touch.Functionally fine (the immediate refresh re-classifies), but on busy watchers this is needless churn. A
Set-based equality on the post-classified internal subset would be more accurate:♻️ Suggested direction
- } else if (JSON.stringify(existing.dependencies || []) !== JSON.stringify(nextDependencies)) { - existing.dependencies = nextDependencies; - } + } else { + const prev = new Set(existing.dependencies || []); + const next = new Set(nextDependencies); + const same = prev.size === next.size && [...prev].every((d) => next.has(d)); + if (!same) existing.dependencies = nextDependencies; + }(Even cleaner: compare against the union of
existing.dependencies + externalDeps + plannedDepsso post-classified entities don't always look "changed".)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/ids/registry-updater.js around lines 411 - 418, The dependency comparison currently uses JSON.stringify on existing.dependencies vs nextDependencies which is order-sensitive and compares unclassified vs post-classified lists; change the check in the block that updates dependencies to compute and compare normalized sets instead: derive the internal-only dependency IDs from nextDependencies (or compute the union of existing.internal + external + planned if preferred), build Sets for existing.dependencies (after _refreshDerivedDependencyFields normalization) and for the normalized nextDependencies, and only assign existing.dependencies = nextDependencies (or the normalized internal subset) when the Sets differ; this removes order-sensitivity and avoids always-triggering updates for entities where only external/planned deps differ (use Set.has and size equality to test set equality).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.aiox-core/core/ids/registry-updater.js:
- Around line 301-313: The delete path is leaving lifecycle values stale because
_handleFileDelete removes IDs from usedBy but the deleted references aren't
added to changedEntities due to the action !== 'unlink' guard, so
_refreshDerivedDependencyFields (which calls detectLifecycle in
populate-entity-registry.js) never recomputes those consumers; fix by ensuring
deleted entity IDs (or the referencing entity IDs) are added to changedEntities
when processing unlink/deletes in _handleFileDelete, or alternatively trigger a
full lifecycle recompute after deletes by invoking
_refreshDerivedDependencyFields for all affected entities (or the whole
registry) after _applyCodeIntelEnrichments/_resolveAllUsedBy; update the logic
around the action !== 'unlink' check and references to changedEntities
accordingly.
---
Nitpick comments:
In @.aiox-core/core/ids/registry-updater.js:
- Around line 278-291: The batch loop is recomputing entityId with
resolveEntityId even though _handleFileCreate and _handleFileModify already
compute it; change those handlers to return an object like { mutated, category,
entityId } (or null/false when not mutated) and update the batch-processing code
that currently inspects mutated to consume the returned category and entityId
directly instead of calling resolveEntityId again, then push { category,
entityId } to changedEntities when present; refer to _handleFileCreate,
_handleFileModify, resolveEntityId, mutated, category, entityId, and
changedEntities when making the change.
- Around line 411-418: The dependency comparison currently uses JSON.stringify
on existing.dependencies vs nextDependencies which is order-sensitive and
compares unclassified vs post-classified lists; change the check in the block
that updates dependencies to compute and compare normalized sets instead: derive
the internal-only dependency IDs from nextDependencies (or compute the union of
existing.internal + external + planned if preferred), build Sets for
existing.dependencies (after _refreshDerivedDependencyFields normalization) and
for the normalized nextDependencies, and only assign existing.dependencies =
nextDependencies (or the normalized internal subset) when the Sets differ; this
removes order-sensitivity and avoids always-triggering updates for entities
where only external/planned deps differ (use Set.has and size equality to test
set equality).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 285cb2f2-ceb0-49d5-8ce2-58f377a4a63b
📒 Files selected for processing (11)
.aiox-core/core/execution/build-state-manager.js.aiox-core/core/ids/registry-updater.js.aiox-core/core/synapse/engine.js.aiox-core/data/entity-registry.yaml.aiox-core/development/scripts/populate-entity-registry.js.aiox-core/install-manifest.yamldocs/stories/epic-error-governance/EPIC-AIOX-ERROR-GOVERNANCE.mddocs/stories/epic-error-governance/STORY-AIOX-ERROR.2-SERIALIZATION-SENSITIVE-MIGRATION.mdtests/core/build-state-manager.test.jstests/core/ids/populate-entity-registry.test.jstests/synapse/engine.test.js
✅ Files skipped from review due to trivial changes (2)
- .aiox-core/install-manifest.yaml
- docs/stories/epic-error-governance/EPIC-AIOX-ERROR-GOVERNANCE.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .aiox-core/core/synapse/engine.js
- tests/core/ids/populate-entity-registry.test.js
- tests/synapse/engine.test.js
- .aiox-core/development/scripts/populate-entity-registry.js
- .aiox-core/core/execution/build-state-manager.js
- tests/core/build-state-manager.test.js
- .aiox-core/data/entity-registry.yaml
rafaelscosta
force-pushed
the
feature/issue-621-error-normalizer-migration
branch
from
8bbb7d2 to
2ffe410
Compare
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
1 participant
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
errormessage fielderrorDetailsto Synapse PipelineMetrics layer failuresValidation
git diff --checknpm test -- --runTestsByPath tests/core/build-state-manager.test.js tests/synapse/engine.test.js tests/core/errors/aiox-error.test.js tests/core/errors/serializer.test.js --runInBandnpx eslint .aiox-core/core/execution/build-state-manager.js .aiox-core/core/synapse/engine.js tests/core/build-state-manager.test.js tests/synapse/engine.test.js --no-warn-ignorednpm run generate:manifest && npm run validate:manifestnpm run typechecknpm run lint(0 errors, 114 existing warnings)npm test -- --runInBand --forceExit --silentRefs #621
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
Documentation