Draft
Conversation
…security (14->2 vulns) Includes:
…security (14->2 vulns)
…uption, circularity)
- Coalesce multiple concurrent log() calls into a single batched disk I/O - Reduce lock contention and improve performance under high load - Add self-healing for log file corruption - Ensure safe serialization of circular references in metadata - Resolve merge conflicts with feature/error-governance
EmiyaKiritsugu3
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@EmiyaKiritsugu3 is attempting to deploy a commit to the SINKRA - AIOX Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
area: agents
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds centralized error governance: new AIOXError class and ErrorRegistry singleton with persistent JSON logging and locking; migrates many modules and CLI scripts from raw console logging to awaitable ErrorRegistry.log(...); updates constitution and PR-quality checks; extracts pipeline/session utilities; removes many generated .aiox artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Code
participant ErrReg as ErrorRegistry
participant Lock as LockManager
participant FS as File System
App->>ErrReg: log(error, options)
ErrReg->>ErrReg: _normalizeError -> AIOXError
alt display && !silent
ErrReg->>App: write formatted/raw to stderr
end
ErrReg->>ErrReg: enqueue entry for persistence
Note over ErrReg: drain scheduled (coalesce batch)
ErrReg->>Lock: acquire('errors-json')
Lock-->>ErrReg: acquired
ErrReg->>FS: read errors.json (or create)
FS-->>ErrReg: content / error
ErrReg->>ErrReg: append batch, rotate to max 500
ErrReg->>FS: write updated errors.json
FS-->>ErrReg: write complete
ErrReg->>Lock: release('errors-json')
Lock-->>ErrReg: released
ErrReg-->>App: resolve awaiting log() promise(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Welcome to aiox-core! Thanks for your first pull request.
What happens next?
- Automated checks will run on your PR
- A maintainer will review your changes
- Once approved, we'll merge your contribution!
PR Checklist:
- Tests pass (
npm test) - Linting passes (
npm run lint) - Commit messages follow Conventional Commits
Thanks for contributing!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.aiox-core/core/utils/output-formatter.js (1)
136-138:⚠️ Potential issue | 🟠 MajorRemove remaining non-governed
console.warnin the formatter path.This warning bypasses the centralized governance path introduced in this PR and can fragment observability behavior.
♻️ Suggested governance-consistent warning path
- if (duration > 100) { - console.warn(`[OutputFormatter] Performance warning: ${duration.toFixed(2)}ms (target: <50ms)`); - } + if (duration > 100) { + void ErrorRegistry + .log(`[OutputFormatter] Performance warning: ${duration.toFixed(2)}ms (target: <50ms)`, { + category: 'OPERATIONAL', + display: true, + raw: true, + }) + .catch(() => {}); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/utils/output-formatter.js around lines 136 - 138, Replace the direct console.warn in the OutputFormatter duration check with the centralized observability/logging API used across the repo (don’t emit console.warn from output-formatter.js); in the block that checks if (duration > 100) where console.warn is called, call the project’s governed warning function (e.g., processLogger.warn or the app’s central logger/observability.warn) and pass the same message and duration metadata so the warning follows the centralized governance path and preserves context.tests/unit/output-formatter.test.js (1)
297-304:⚠️ Potential issue | 🟡 MinorMissing
awaitin performance test may produce inaccurate timing.The test measures time for
formatter.format()but does notawaitthe result. Sinceformat()is now async, this test only measures Promise creation time, not actual formatting completion time.Proposed fix
- test('formatter completes in reasonable time', () => { + test('formatter completes in reasonable time', async () => { const start = process.hrtime.bigint(); const formatter = new PersonalizedOutputFormatter(mockAgent, mockTask, mockResults); - formatter.format(); + await formatter.format(); const duration = Number(process.hrtime.bigint() - start) / 1000000; expect(duration).toBeLessThan(100); // Should be <100ms (2x target) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/output-formatter.test.js` around lines 297 - 304, The test currently measures only Promise creation because formatter.format() is async; make the test function async and await the call to PersonalizedOutputFormatter.prototype.format (i.e., change the test to async () => { ... await formatter.format(); ... }) so the timing captures the actual formatting work and then compute duration after the awaited call; keep the same duration assertion on Number(process.hrtime.bigint() - start) / 1000000.
🧹 Nitpick comments (17)
.aiox-core/workflow-intelligence/learning/gotcha-registry.js (1)
13-13: Use absolute import forErrorRegistry.This newly added relative import conflicts with the project import rule; please switch it to the project’s absolute import convention.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/workflow-intelligence/learning/gotcha-registry.js at line 13, The import of ErrorRegistry uses a relative path; replace the relative require in gotcha-registry.js with the project’s absolute import convention so it reads the module via its absolute module name (i.e., require('monitor/error-registry') or the configured absolute path) instead of '../../monitor/error-registry'; update the require call that references ErrorRegistry to the absolute import form to satisfy the project rule..aiox-core/core/utils/output-formatter.js (1)
16-16: Switch newErrorRegistryimport to absolute form.This newly introduced relative import breaks the repository import convention.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/utils/output-formatter.js at line 16, Replace the relative require for ErrorRegistry with the project's absolute-import form: locate the ErrorRegistry import in output-formatter.js (the const ErrorRegistry = require('../../monitor/error-registry') line) and change it to use the repository's absolute import pattern (import or require using the module's absolute path) so it follows the codebase convention for absolute imports..aiox-core/workflow-intelligence/learning/qa-feedback.js (1)
15-15: Use absolute import forErrorRegistryin this new dependency.Please convert the newly added relative import to the project’s absolute import pattern.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/workflow-intelligence/learning/qa-feedback.js at line 15, The require for ErrorRegistry is using a relative path; replace the relative import require('../../monitor/error-registry') with the project's absolute import style for that module (e.g., require('monitor/error-registry') or the repo's established root alias) so ErrorRegistry is imported via the absolute module path; update the import in qa-feedback.js (symbol: ErrorRegistry) and re-run lint/tests to ensure the absolute-import pattern passes project rules..aiox-core/infrastructure/scripts/config-loader.js (1)
25-25: Use absolute import instead of relative import.The import path uses a relative path which violates the coding guidelines.
Proposed fix
-const ErrorRegistry = require('../monitor/error-registry'); +const ErrorRegistry = require('.aiox-core/monitor/error-registry');As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/infrastructure/scripts/config-loader.js at line 25, The require call for ErrorRegistry uses a relative path ('../monitor/error-registry'); replace it with the project's absolute module path so it follows the absolute-import rule: update the require in config-loader.js where ErrorRegistry is imported (const ErrorRegistry = require(...)) to use the repository's absolute import name for the monitor module (e.g., 'monitor/error-registry' or your project's configured package alias) and ensure any module resolution/aliases are respected so tests and runtime still resolve correctly..aiox-core/workflow-intelligence/engine/suggestion-engine.js (1)
20-20: Use absolute import instead of relative import.The import path uses a relative path which violates the coding guidelines.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/workflow-intelligence/engine/suggestion-engine.js at line 20, The require for ErrorRegistry uses a relative path ("../../monitor/error-registry"); replace it with the project’s absolute import for that module (e.g. require('monitor/error-registry') or the repository’s configured import alias) so ErrorRegistry is imported via an absolute path instead of a relative one; update the const ErrorRegistry = require(...) line accordingly..aiox-core/workflow-intelligence/learning/pattern-store.js (1)
14-14: Use absolute import instead of relative import.The import path uses a relative path which violates the coding guidelines.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/workflow-intelligence/learning/pattern-store.js at line 14, Replace the relative require for ErrorRegistry with an absolute import: change the require('../../monitor/error-registry') that assigns to ErrorRegistry to use the project’s absolute module path (so ErrorRegistry is imported via an absolute module specifier per the coding guidelines) and ensure the symbol name ErrorRegistry remains unchanged..aiox-core/infrastructure/scripts/pattern-extractor.js (1)
31-31: Use absolute import instead of relative import.The import path uses a relative path which violates the coding guidelines.
Proposed fix
-const ErrorRegistry = require('../../monitor/error-registry'); +const ErrorRegistry = require('.aiox-core/monitor/error-registry');As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/infrastructure/scripts/pattern-extractor.js at line 31, Replace the relative require for ErrorRegistry in pattern-extractor.js with an absolute import per project conventions: locate the line requiring ErrorRegistry (const ErrorRegistry = require('../../monitor/error-registry')) and change it to use the absolute module path (e.g., require('monitor/error-registry') or the project's configured absolute alias) so ErrorRegistry is imported via an absolute import rather than a relative one; ensure the symbol name ErrorRegistry stays unchanged and run linter to verify compliance.bin/utils/framework-guard.js (1)
216-216: Consider using absolute import for consistency.The
ErrorRegistryimport uses a relative path. While this may be necessary given the CLI script's location inbin/utils/, consider whether a path alias or module resolution could enable absolute imports for consistency.As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/utils/framework-guard.js` at line 216, The require for ErrorRegistry in framework-guard.js uses a relative path ('../../.aiox-core/monitor/error-registry'); change it to an absolute/module-style import (e.g. require('@aiox-core/monitor/error-registry') or the package-root export) so it conforms to the absolute-import guideline; to enable this, update the project/module resolution config (tsconfig/paths, webpack/rollup config, or package.json exports) if needed and then replace the require in framework-guard.js (referencing ErrorRegistry) to the new absolute path.bin/utils/validate-publish.js (1)
30-30: Relative import should use absolute path with@/alias.The import uses a relative path which violates the coding guidelines requiring absolute imports.
-const ErrorRegistry = require('../../.aiox-core/monitor/error-registry'); +const ErrorRegistry = require('@/.aiox-core/monitor/error-registry');As per coding guidelines: "Use absolute imports instead of relative imports in all code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/utils/validate-publish.js` at line 30, Replace the relative require for ErrorRegistry with the project absolute alias: change the require('../../.aiox-core/monitor/error-registry') call to use the '@/...' alias (e.g. require('@/.aiox-core/monitor/error-registry')) so ErrorRegistry is imported via the absolute path consistent with coding guidelines; update the require expression referencing ErrorRegistry accordingly.tests/monitor/error-registry.test.js (2)
22-24: EmptyafterAllhook can be removed.The empty
afterAllblock adds no value. Remove it or implement actual cleanup if needed.- afterAll(() => { - // Cleanup is optional for local dev, but good practice - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitor/error-registry.test.js` around lines 22 - 24, Remove the empty afterAll hook in tests/monitor/error-registry.test.js (the afterAll() block) since it provides no cleanup; either delete the afterAll() entirely or replace it with real teardown logic that calls the appropriate cleanup utilities used by the test suite (e.g., closing mocks, restoring timers, or clearing test data) so the test file no longer contains an empty lifecycle hook.
8-9: Consider using absolute imports for consistency.-const ErrorRegistry = require('../../.aiox-core/monitor/error-registry'); -const AIOXError = require('../../.aiox-core/utils/aiox-error'); +const ErrorRegistry = require('@/.aiox-core/monitor/error-registry'); +const AIOXError = require('@/.aiox-core/utils/aiox-error');As per coding guidelines: "Use absolute imports instead of relative imports in all code". However, test files commonly use relative imports for test subject locality—verify if your project conventions exempt test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitor/error-registry.test.js` around lines 8 - 9, Replace the relative require() calls that import ErrorRegistry and AIOXError with the project's absolute module paths (e.g. require('@aiox-core/monitor/error-registry') and require('@aiox-core/utils/aiox-error') or the configured package alias your project uses) so that ErrorRegistry and AIOXError are imported via absolute imports in the test file per the coding guideline; if your project intentionally exempts tests from this rule, confirm that exemption and leave the relative imports unchanged..aiox-core/infrastructure/scripts/output-formatter.js (2)
54-78:ErrorRegistry.log()calls are not awaited, risking lost persistence.The
_loadPersonaProfilemethod is synchronous (called from constructor), butErrorRegistry.log()returns a Promise. These fire-and-forget calls may not persist if the process exits shortly after construction.Since the constructor cannot be async, consider:
- Making
_loadPersonaProfilereturn a Promise and adding an asyncinit()method- Or accepting this as a known trade-off for constructor simplicity (document it)
if (!fs.existsSync(agentPath)) { - ErrorRegistry.log(`[OutputFormatter] Agent file not found: ${agentPath}`, { category: 'OPERATIONAL', display: true, raw: true }); + // Fire-and-forget: persistence not guaranteed in constructor context + ErrorRegistry.log(`[OutputFormatter] Agent file not found: ${agentPath}`, { category: 'OPERATIONAL', display: true, raw: true }); this.personaProfile = this._getNeutralProfile();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/infrastructure/scripts/output-formatter.js around lines 54 - 78, The ErrorRegistry.log calls inside _loadPersonaProfile are async (return a Promise) but are invoked fire-and-forget from the constructor path, risking lost persistence; change _loadPersonaProfile to return a Promise and make caller initialize the instance via an async init() method (e.g., add async init() that awaits this._loadPersonaProfile()), or alternatively convert _loadPersonaProfile to always await ErrorRegistry.log results before returning; update the constructor to no longer call _loadPersonaProfile directly (call init() after construction) and ensure all ErrorRegistry.log invocations in _loadPersonaProfile are awaited to guarantee persistence.
14-14: Relative import should use absolute path.-const ErrorRegistry = require('../../monitor/error-registry'); +const ErrorRegistry = require('@/.aiox-core/monitor/error-registry');As per coding guidelines: "Use absolute imports instead of relative imports in all code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/infrastructure/scripts/output-formatter.js at line 14, The file currently requires ErrorRegistry via a relative path (require('../../monitor/error-registry')); change this to use the project's absolute import for that module (e.g., require('monitor/error-registry') or the configured absolute alias used in the repo) so ErrorRegistry is imported with an absolute path; update the require for ErrorRegistry accordingly and ensure any module resolution config (if needed) is consistent with other absolute imports..aiox-core/infrastructure/scripts/pr-review-ai.js (1)
20-20: Relative import should use absolute path.-const ErrorRegistry = require('../../monitor/error-registry'); +const ErrorRegistry = require('@/.aiox-core/monitor/error-registry');As per coding guidelines: "Use absolute imports instead of relative imports in all code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/infrastructure/scripts/pr-review-ai.js at line 20, Replace the relative require call that defines const ErrorRegistry (currently require('../../monitor/error-registry')) with an absolute import using the project's module alias; keep the symbol name ErrorRegistry and switch the require to the absolute module path (or equivalent ES module import) so the file uses an absolute import instead of a relative one..aiox-core/monitor/error-registry.js (3)
74-87: Minor TOCTOU race in initialization can be simplified.The
existsSyncchecks beforemkdir/writeFilecreate a small race window. Sincemkdir({ recursive: true })is idempotent and won't fail if the directory already exists, you can call it unconditionally. For the file, you could usewriteFilewith{ flag: 'wx' }to create only if it doesn't exist.♻️ Optional simplification
this._initPromise = (async () => { try { - if (!fs.existsSync(this.logDir)) { - await fsp.mkdir(this.logDir, { recursive: true }); - } - - if (!fs.existsSync(this.logFile)) { - await fsp.writeFile(this.logFile, JSON.stringify([], null, 2), 'utf8'); + await fsp.mkdir(this.logDir, { recursive: true }); + + try { + await fsp.writeFile(this.logFile, JSON.stringify([], null, 2), { encoding: 'utf8', flag: 'wx' }); + } catch (e) { + if (e.code !== 'EEXIST') throw e; } } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 74 - 87, The init code in ErrorRegistry's this._initPromise has a TOCTOU due to existsSync checks; remove the pre-checks and instead call fsp.mkdir(this.logDir, { recursive: true }) unconditionally and attempt to create the file with fsp.writeFile(this.logFile, JSON.stringify([], null, 2), 'utf8', { flag: 'wx' }) so the write only succeeds if the file doesn't already exist (handle EEXIST by ignoring it); keep the outer try/catch around the async IIFE (the _initPromise initialization) and retain the existing stderr fallback on error.
178-182: Consider suppressing "(unknown)" agent suffix.When
agentIddefaults to'unknown'(as perAIOXErrorconstructor), the output will display(unknown), which adds noise without value. Consider excluding this case:♻️ Optional refinement
- const agentSuffix = aioxError.agentId ? ` (${aioxError.agentId})` : ''; + const agentSuffix = aioxError.agentId && aioxError.agentId !== 'unknown' + ? ` (${aioxError.agentId})` + : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 178 - 182, The log currently appends an agent suffix whenever aioxError.agentId is set, causing noisy "(unknown)" output; change the agent suffix logic in error-registry.js so it only appends when aioxError.agentId is present and not the literal 'unknown' (e.g., const agentSuffix = aioxError.agentId && aioxError.agentId !== 'unknown' ? ` (${aioxError.agentId})` : '';), keeping the rest of the console.error call using the same aioxError, category, and message.
259-261: Consider write-then-rename pattern for improved crash safety during writes.
fsp.writeFileis not atomic; if the process crashes mid-write, the log file may be corrupted. However, the registry has built-in recovery:_readLogFile()detects corruption via JSON parsing errors, creates a timestamped backup, and restarts with a fresh state.To eliminate even temporary data loss during crashes, you could adopt the atomic write pattern:
♻️ Atomic write pattern (optional)
// 5. Write - await fsp.writeFile(this.logFile, serialized, 'utf8'); + const tmpFile = `${this.logFile}.tmp.${process.pid}`; + await fsp.writeFile(tmpFile, serialized, 'utf8'); + await fsp.rename(tmpFile, this.logFile);On POSIX systems, rename is atomic. On Windows, you may need to handle the case where the target exists or use a library like
write-file-atomic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 259 - 261, Replace the non-atomic fsp.writeFile(this.logFile, ...) with an atomic write-then-rename pattern: write the serialized data to a temp file (e.g., this.logFile + '.tmp.' + process.pid or a timestamp) using fsp.writeFile, fsync the temp file if available, then fsp.rename(tempPath, this.logFile) to atomically replace the log file; handle Windows/rename collision errors by unlinking the target and retrying the rename or fall back to a library like write-file-atomic. Ensure you change the code around the fsp.writeFile call and keep compatibility with the existing recovery logic in _readLogFile().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/constitution.md:
- Around line 145-147: Remove the duplicated horizontal rule separator by
deleting one of the consecutive `---` lines so there is only a single `---`
paragraph break; target the adjacent duplicate `---` separator lines in the
section that currently contains an empty section between two `---` markers and
remove one to eliminate the empty section.
In @.aiox-core/core/utils/output-formatter.js:
- Line 57: The synchronous function _loadPersonaProfile calls the async
ErrorRegistry.log without awaiting or handling rejections, causing possible
unhandled promise rejections; fix by making _loadPersonaProfile async and
awaiting each ErrorRegistry.log invocation (e.g., await ErrorRegistry.log(...))
or, if converting to async is infeasible, explicitly suppress the promise by
calling void ErrorRegistry.log(...).catch(() => {}) for each call (ensure you
update the three occurrences that currently call ErrorRegistry.log in
_loadPersonaProfile).
In @.aiox-core/infrastructure/scripts/conflict-resolver.js:
- Line 7: The require path for ErrorRegistry in conflict-resolver.js is
incorrect and will fail at runtime; update the import for the ErrorRegistry
symbol used in this file (ErrorRegistry) to point to the actual module location
by changing the relative path so it resolves to
.aiox-core/monitor/error-registry.js (i.e., adjust the require from
'../monitor/error-registry' to the correct two-level-up relative path).
In @.aiox-core/infrastructure/scripts/pr-review-ai.js:
- Around line 1039-1044: The code calls await ErrorRegistry.log(...) while not
inside an async function (the CLI block using prNumber and args), causing a
syntax error; fix by wrapping the entire CLI execution block that references
prNumber and calls ErrorRegistry.log in an async IIFE (e.g. (async () => { ...
})().catch(...)) or converting the enclosing function to async so await is
valid, ensuring ErrorRegistry.log is awaited correctly and any process.exit
paths are preserved.
- Around line 363-372: Update the rule set that contains the two regex entries
matching "console.error" and catch blocks (the objects with regex:
/console\.error\s*\(/g and regex:
/catch\s*\(\s*(\w+)\s*\)\s*\{\s*(?![^}]*ErrorRegistry\.log)/g) to skip files
named error-registry.js by adding a path-based exemption check before applying
these patterns; ensure the validator ignores matches when the file path ends
with error-registry.js (or a configured infra whitelist) so legitimate
console.error usages and bootstrap catch blocks in that module are not flagged.
In @.aiox-core/monitor/error-registry.js:
- Around line 146-150: The _normalizeError function currently mutates the
incoming AIOXError via Object.assign(error, options); instead create and return
a non-mutated clone when error instanceof AIOXError: allocate a new object that
preserves the original prototype (using
Object.create(Object.getPrototypeOf(error))) and copy the original error
properties plus the options into that clone (e.g., Object.assign(clone, error,
options)), then return the clone; update the code in _normalizeError to
reference AIOXError and the new cloned object so callers’ original error
instances are not modified.
In @.aiox-core/workflow-intelligence/engine/suggestion-engine.js:
- Around line 598-599: The synchronous method _applyLearnedPatternBoost() calls
the asynchronous ErrorRegistry.log() without awaiting it, which can produce
unhandled promise rejections; modify the call site inside
_applyLearnedPatternBoost() to append .catch(() => {}) to the
ErrorRegistry.log(...) invocation so any rejection is swallowed and no unhandled
promise is emitted, keeping the surrounding method synchronous and preserving
the existing return of suggestions.
- Around line 139-141: In buildContext's catch block, the call to
ErrorRegistry.log is not awaited; change it to await ErrorRegistry.log(...) so
the async log completes before proceeding (keep buildContext as async), ensuring
the error is flushed; update the catch in the buildContext function to use await
ErrorRegistry.log and handle/propagate any rethrowing if needed.
- Around line 395-396: The call to the async ErrorRegistry.log() inside the
synchronous _resolveStoryPath() should handle promise rejections; update the
call in _resolveStoryPath where ErrorRegistry.log(`[SuggestionEngine] Story path
not found: ${storyPath}`, ...) is invoked to append .catch(() => {}) so the
returned promise is swallowed and won't cause unhandled rejections in this sync
method while preserving the logging side-effect.
- Around line 76-77: The async ErrorRegistry.log calls inside _loadDependencies
(the four fire-and-forget invocations that log WIS/load errors and similar) must
not be left unhandled; either make _loadDependencies async and await each
ErrorRegistry.log call, or (minimal fix) append .catch(() => {}) to each
ErrorRegistry.log invocation to suppress unhandled rejections; update the four
occurrences (the ErrorRegistry.log calls around loading WIS and the other three
failure branches) to follow one of these patterns so promise rejections are
handled.
In @.aiox-core/workflow-intelligence/learning/gotcha-registry.js:
- Line 100: The call to the async method ErrorRegistry.log(...) inside the
synchronous load() catch path is fire-and-forget; change it to explicitly handle
the returned Promise so rejections are not unhandled and telemetry isn't
dropped—e.g., call ErrorRegistry.log(...).catch(err => {/* log error locally or
forward to processLogger */}) or invoke an immediately-invoked async function to
await the call; update the catch block around load() to use
ErrorRegistry.log(...).catch(...) (referencing ErrorRegistry.log and the load()
catch path) so the promise is observed and failures are reported.
In @.aiox-core/workflow-intelligence/learning/pattern-store.js:
- Around line 337-339: The catch block in _load() calls the async
ErrorRegistry.log() without awaiting it, causing potential unhandled rejections
or lost logs; either make _load() async and await ErrorRegistry.log(...) so the
log is flushed before returning, or keep _load() synchronous but append
.catch(...) to ErrorRegistry.log(...) to handle rejections (and optionally log
the rejection locally) — update the catch in _load() to use one of these two
approaches referencing ErrorRegistry.log and the _load function.
- Around line 372-374: The catch block in the save path is calling the async
ErrorRegistry.log() without awaiting it, which can lead to unhandled rejections
and lost logs; update the catch to await ErrorRegistry.log(...) so the logging
completes before re-throwing the error (do the same fix in _load if present),
e.g., replace ErrorRegistry.log(...) with await ErrorRegistry.log(...) in the
catch inside the save/_save function so the promise is resolved before throw.
In @.aiox-core/workflow-intelligence/learning/qa-feedback.js:
- Line 90: Replace the fire-and-forget ErrorRegistry.log(...) calls in the
synchronous catch blocks (the occurrences of ErrorRegistry.log(`Failed to load
feedback history: ${error.message}`, ...) and the similar calls at the other
noted sites) with a safe non-blocking pattern that explicitly handles the
returned Promise, e.g. call ErrorRegistry.log(...).catch(err => {/* handle or
swallow persist error, optionally log to console/processLogger */}) or prefix
with void to indicate intentional non-await and attach .catch(...) so any
rejection is handled; update each ErrorRegistry.log invocation at the mentioned
locations (lines with the same ErrorRegistry.log(...) symbol) to use this
pattern.
In `@tests/synapse/engine.test.js`:
- Around line 248-250: The test 'should handle missing layer modules gracefully'
weakly asserts engine.layers.length <= 4; change it to assert the Engine
constructor does not throw and that engine.layers.length === 4 to ensure exactly
the 4 expected layers were loaded in this mocked failure scenario. Locate the
test block (the test function with description 'should handle missing layer
modules gracefully') and replace the <= assertion with two checks: one that
creating the Engine (or calling the constructor used in the test) does not
throw, and one that engine.layers.length === 4, referencing the engine variable
and its layers property.
---
Outside diff comments:
In @.aiox-core/core/utils/output-formatter.js:
- Around line 136-138: Replace the direct console.warn in the OutputFormatter
duration check with the centralized observability/logging API used across the
repo (don’t emit console.warn from output-formatter.js); in the block that
checks if (duration > 100) where console.warn is called, call the project’s
governed warning function (e.g., processLogger.warn or the app’s central
logger/observability.warn) and pass the same message and duration metadata so
the warning follows the centralized governance path and preserves context.
In `@tests/unit/output-formatter.test.js`:
- Around line 297-304: The test currently measures only Promise creation because
formatter.format() is async; make the test function async and await the call to
PersonalizedOutputFormatter.prototype.format (i.e., change the test to async ()
=> { ... await formatter.format(); ... }) so the timing captures the actual
formatting work and then compute duration after the awaited call; keep the same
duration assertion on Number(process.hrtime.bigint() - start) / 1000000.
---
Nitpick comments:
In @.aiox-core/core/utils/output-formatter.js:
- Line 16: Replace the relative require for ErrorRegistry with the project's
absolute-import form: locate the ErrorRegistry import in output-formatter.js
(the const ErrorRegistry = require('../../monitor/error-registry') line) and
change it to use the repository's absolute import pattern (import or require
using the module's absolute path) so it follows the codebase convention for
absolute imports.
In @.aiox-core/infrastructure/scripts/config-loader.js:
- Line 25: The require call for ErrorRegistry uses a relative path
('../monitor/error-registry'); replace it with the project's absolute module
path so it follows the absolute-import rule: update the require in
config-loader.js where ErrorRegistry is imported (const ErrorRegistry =
require(...)) to use the repository's absolute import name for the monitor
module (e.g., 'monitor/error-registry' or your project's configured package
alias) and ensure any module resolution/aliases are respected so tests and
runtime still resolve correctly.
In @.aiox-core/infrastructure/scripts/output-formatter.js:
- Around line 54-78: The ErrorRegistry.log calls inside _loadPersonaProfile are
async (return a Promise) but are invoked fire-and-forget from the constructor
path, risking lost persistence; change _loadPersonaProfile to return a Promise
and make caller initialize the instance via an async init() method (e.g., add
async init() that awaits this._loadPersonaProfile()), or alternatively convert
_loadPersonaProfile to always await ErrorRegistry.log results before returning;
update the constructor to no longer call _loadPersonaProfile directly (call
init() after construction) and ensure all ErrorRegistry.log invocations in
_loadPersonaProfile are awaited to guarantee persistence.
- Line 14: The file currently requires ErrorRegistry via a relative path
(require('../../monitor/error-registry')); change this to use the project's
absolute import for that module (e.g., require('monitor/error-registry') or the
configured absolute alias used in the repo) so ErrorRegistry is imported with an
absolute path; update the require for ErrorRegistry accordingly and ensure any
module resolution config (if needed) is consistent with other absolute imports.
In @.aiox-core/infrastructure/scripts/pattern-extractor.js:
- Line 31: Replace the relative require for ErrorRegistry in
pattern-extractor.js with an absolute import per project conventions: locate the
line requiring ErrorRegistry (const ErrorRegistry =
require('../../monitor/error-registry')) and change it to use the absolute
module path (e.g., require('monitor/error-registry') or the project's configured
absolute alias) so ErrorRegistry is imported via an absolute import rather than
a relative one; ensure the symbol name ErrorRegistry stays unchanged and run
linter to verify compliance.
In @.aiox-core/infrastructure/scripts/pr-review-ai.js:
- Line 20: Replace the relative require call that defines const ErrorRegistry
(currently require('../../monitor/error-registry')) with an absolute import
using the project's module alias; keep the symbol name ErrorRegistry and switch
the require to the absolute module path (or equivalent ES module import) so the
file uses an absolute import instead of a relative one.
In @.aiox-core/monitor/error-registry.js:
- Around line 74-87: The init code in ErrorRegistry's this._initPromise has a
TOCTOU due to existsSync checks; remove the pre-checks and instead call
fsp.mkdir(this.logDir, { recursive: true }) unconditionally and attempt to
create the file with fsp.writeFile(this.logFile, JSON.stringify([], null, 2),
'utf8', { flag: 'wx' }) so the write only succeeds if the file doesn't already
exist (handle EEXIST by ignoring it); keep the outer try/catch around the async
IIFE (the _initPromise initialization) and retain the existing stderr fallback
on error.
- Around line 178-182: The log currently appends an agent suffix whenever
aioxError.agentId is set, causing noisy "(unknown)" output; change the agent
suffix logic in error-registry.js so it only appends when aioxError.agentId is
present and not the literal 'unknown' (e.g., const agentSuffix =
aioxError.agentId && aioxError.agentId !== 'unknown' ? ` (${aioxError.agentId})`
: '';), keeping the rest of the console.error call using the same aioxError,
category, and message.
- Around line 259-261: Replace the non-atomic fsp.writeFile(this.logFile, ...)
with an atomic write-then-rename pattern: write the serialized data to a temp
file (e.g., this.logFile + '.tmp.' + process.pid or a timestamp) using
fsp.writeFile, fsync the temp file if available, then fsp.rename(tempPath,
this.logFile) to atomically replace the log file; handle Windows/rename
collision errors by unlinking the target and retrying the rename or fall back to
a library like write-file-atomic. Ensure you change the code around the
fsp.writeFile call and keep compatibility with the existing recovery logic in
_readLogFile().
In @.aiox-core/workflow-intelligence/engine/suggestion-engine.js:
- Line 20: The require for ErrorRegistry uses a relative path
("../../monitor/error-registry"); replace it with the project’s absolute import
for that module (e.g. require('monitor/error-registry') or the repository’s
configured import alias) so ErrorRegistry is imported via an absolute path
instead of a relative one; update the const ErrorRegistry = require(...) line
accordingly.
In @.aiox-core/workflow-intelligence/learning/gotcha-registry.js:
- Line 13: The import of ErrorRegistry uses a relative path; replace the
relative require in gotcha-registry.js with the project’s absolute import
convention so it reads the module via its absolute module name (i.e.,
require('monitor/error-registry') or the configured absolute path) instead of
'../../monitor/error-registry'; update the require call that references
ErrorRegistry to the absolute import form to satisfy the project rule.
In @.aiox-core/workflow-intelligence/learning/pattern-store.js:
- Line 14: Replace the relative require for ErrorRegistry with an absolute
import: change the require('../../monitor/error-registry') that assigns to
ErrorRegistry to use the project’s absolute module path (so ErrorRegistry is
imported via an absolute module specifier per the coding guidelines) and ensure
the symbol name ErrorRegistry remains unchanged.
In @.aiox-core/workflow-intelligence/learning/qa-feedback.js:
- Line 15: The require for ErrorRegistry is using a relative path; replace the
relative import require('../../monitor/error-registry') with the project's
absolute import style for that module (e.g., require('monitor/error-registry')
or the repo's established root alias) so ErrorRegistry is imported via the
absolute module path; update the import in qa-feedback.js (symbol:
ErrorRegistry) and re-run lint/tests to ensure the absolute-import pattern
passes project rules.
In `@bin/utils/framework-guard.js`:
- Line 216: The require for ErrorRegistry in framework-guard.js uses a relative
path ('../../.aiox-core/monitor/error-registry'); change it to an
absolute/module-style import (e.g. require('@aiox-core/monitor/error-registry')
or the package-root export) so it conforms to the absolute-import guideline; to
enable this, update the project/module resolution config (tsconfig/paths,
webpack/rollup config, or package.json exports) if needed and then replace the
require in framework-guard.js (referencing ErrorRegistry) to the new absolute
path.
In `@bin/utils/validate-publish.js`:
- Line 30: Replace the relative require for ErrorRegistry with the project
absolute alias: change the require('../../.aiox-core/monitor/error-registry')
call to use the '@/...' alias (e.g.
require('@/.aiox-core/monitor/error-registry')) so ErrorRegistry is imported via
the absolute path consistent with coding guidelines; update the require
expression referencing ErrorRegistry accordingly.
In `@tests/monitor/error-registry.test.js`:
- Around line 22-24: Remove the empty afterAll hook in
tests/monitor/error-registry.test.js (the afterAll() block) since it provides no
cleanup; either delete the afterAll() entirely or replace it with real teardown
logic that calls the appropriate cleanup utilities used by the test suite (e.g.,
closing mocks, restoring timers, or clearing test data) so the test file no
longer contains an empty lifecycle hook.
- Around line 8-9: Replace the relative require() calls that import
ErrorRegistry and AIOXError with the project's absolute module paths (e.g.
require('@aiox-core/monitor/error-registry') and
require('@aiox-core/utils/aiox-error') or the configured package alias your
project uses) so that ErrorRegistry and AIOXError are imported via absolute
imports in the test file per the coding guideline; if your project intentionally
exempts tests from this rule, confirm that exemption and leave the relative
imports unchanged.
🪄 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: 41d5778e-2325-4d8c-9f70-1b3cedd68e12
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.aiox-core/constitution.md.aiox-core/core/utils/output-formatter.js.aiox-core/data/entity-registry.yaml.aiox-core/infrastructure/scripts/config-loader.js.aiox-core/infrastructure/scripts/conflict-resolver.js.aiox-core/infrastructure/scripts/output-formatter.js.aiox-core/infrastructure/scripts/pattern-extractor.js.aiox-core/infrastructure/scripts/pr-review-ai.js.aiox-core/install-manifest.yaml.aiox-core/monitor/error-registry.js.aiox-core/utils/aiox-error.js.aiox-core/workflow-intelligence/engine/suggestion-engine.js.aiox-core/workflow-intelligence/learning/gotcha-registry.js.aiox-core/workflow-intelligence/learning/pattern-store.js.aiox-core/workflow-intelligence/learning/qa-feedback.js.aiox/audit/branch-protection.json.aiox/audit/repo-settings.json.aiox/cache/devcontext_docs_framework_coding_standards_md_summary.json.aiox/cache/devcontext_docs_framework_source_tree_md_summary.json.aiox/cache/devcontext_docs_framework_tech_stack_md_summary.json.aiox/codebase-map.json.aiox/dashboard/status.json.aiox/environment-report.yaml.aiox/epic-EPIC-ACT-state.yaml.aiox/error-tracking.json.aiox/feedback.json.aiox/gotchas.json.aiox/gotchas.md.aiox/merge-rules.yaml.aiox/migration-inventory.json.aiox/path-analysis-report.json.aiox/patterns.md.aiox/session-digests/.gitkeep.aiox/session-digests/README.md.aiox/session-digests/example-session-abc123-2026-02-09T18-30-45-123Z.yaml.aiox/session-state.json.aiox/status.json.synapse/.gitignorebin/aiox-init.jsbin/utils/framework-guard.jsbin/utils/validate-publish.jsdocs/stories/7.1.1.error-governance.mddocs/stories/7.1.2.legacy-error-migration.mdpackage.jsontests/cli/validate-publish.test.jstests/core/health-check/check-registry.test.jstests/integration/formatter-integration.test.jstests/monitor/error-registry.test.jstests/synapse/engine.test.jstests/synapse/generate-constitution.test.jstests/unit/output-formatter.test.js
💤 Files with no reviewable changes (19)
- .aiox/error-tracking.json
- .aiox/session-state.json
- .aiox/cache/devcontext_docs_framework_tech_stack_md_summary.json
- .aiox/status.json
- .aiox/gotchas.json
- .aiox/audit/repo-settings.json
- .aiox/cache/devcontext_docs_framework_source_tree_md_summary.json
- .aiox/feedback.json
- .aiox/cache/devcontext_docs_framework_coding_standards_md_summary.json
- .aiox/session-digests/example-session-abc123-2026-02-09T18-30-45-123Z.yaml
- .aiox/audit/branch-protection.json
- .aiox/codebase-map.json
- .aiox/session-digests/README.md
- .aiox/dashboard/status.json
- .aiox/environment-report.yaml
- .aiox/patterns.md
- .aiox/gotchas.md
- .aiox/merge-rules.yaml
- .aiox/epic-EPIC-ACT-state.yaml
- Implement safe logInstallerError helper in Wizard with dynamic registry loading - Update PRReviewAI regex to exempt error-registry.js from console.error checks - Wrap PRReviewAI CLI in async IIFE to support awaited error logging
- Migrate generic Error throws to standard AIOXError in PatternStore and GotchaRegistry - Ensure async ErrorRegistry.log calls are observed via .catch() in synchronous paths - Update unit tests to expect explicit error throws for invalid operations
- Move PipelineMetrics class to a dedicated utility file for better SoC - Extract layer execution loop from process() to private _executeLayers method - Strengthen engine test assertions to verify exact layer loading count
…l model - Resolve active_agent vs activeAgent naming drift at the engine entry point - Maintain backward compatibility for snake_case dependent layers - Ensure consistent prompt_count access throughout the pipeline
There was a problem hiding this comment.
Actionable comments posted: 9
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/utils/output-formatter.js (1)
46-63:⚠️ Potential issue | 🟠 MajorReplace the remaining
console.warnpaths to fully enforce governance.Line 49 and Line 149 still emit non-governed logs directly, which leaves gaps in the new mandatory registry-based error policy.
Suggested fix
- console.warn('[OutputFormatter] Agent ID missing, using neutral profile'); + void ErrorRegistry.log('[OutputFormatter] Agent ID missing, using neutral profile', { + category: 'OPERATIONAL', + display: true, + raw: true, + }).catch(() => {}); ... - if (duration > 100) { - console.warn(`[OutputFormatter] Performance warning: ${duration.toFixed(2)}ms (target: <50ms)`); - } + if (duration > 100) { + void ErrorRegistry.log( + `[OutputFormatter] Performance warning: ${duration.toFixed(2)}ms (target: <50ms)`, + { category: 'OPERATIONAL', display: true, raw: true } + ).catch(() => {}); + }Also applies to: 122-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/utils/output-formatter.js around lines 46 - 63, The code still uses direct console.warn calls in _loadPersonaProfile (and the other spots noted) which bypass the ErrorRegistry; replace those console.warn usages with ErrorRegistry.log(...) calls mirroring the existing pattern (include a descriptive message, category: 'OPERATIONAL', display: true, raw: true) and retain the fallback behavior (set this.personaProfile = this._getNeutralProfile()); ensure you call .catch(() => {}) on the promise to swallow errors like the surrounding ErrorRegistry usage so logging remains non-blocking.
♻️ Duplicate comments (1)
tests/synapse/engine.test.js (1)
249-252:⚠️ Potential issue | 🟡 MinorAssert against the instance created inside this test.
The constructor call is wrapped in
expect(...).not.toThrow(), but the length check still reads the sharedenginefrombeforeEach. This can pass even if the newly constructed instance loads the wrong modules.Suggested fix
- expect(() => new SynapseEngine('/fake/.synapse', { manifest: {} })).not.toThrow(); - expect(engine.layers.length).toBe(4); + let localEngine; + expect(() => { + localEngine = new SynapseEngine('/fake/.synapse', { manifest: {} }); + }).not.toThrow(); + expect(localEngine.layers).toHaveLength(4);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/synapse/engine.test.js` around lines 249 - 252, The test constructs a new SynapseEngine but asserts against the shared `engine` from beforeEach; change the test to capture the newly created instance (e.g., const localEngine = new SynapseEngine('/fake/.synapse', { manifest: {} })) inside the expect(...).not.toThrow() block and assert against that instance's layers (expect(localEngine.layers.length).toBe(4)); ensure you reference `localEngine` (or similarly named local variable) instead of the shared `engine` so the assertion verifies the exact instance created in the test.
🧹 Nitpick comments (2)
.aiox-core/workflow-intelligence/learning/pattern-store.js (1)
14-15: Switch new imports to project-absolute paths.Line 14 and Line 15 introduce relative imports (
../../...), which conflicts with the repository import rule for JS/TS files.As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/workflow-intelligence/learning/pattern-store.js around lines 14 - 15, The two relative requires in pattern-store.js (ErrorRegistry and AIOXError) violate the repo rule; replace require('../../monitor/error-registry') and require('../../utils/aiox-error') with project-absolute imports so the modules are required as root-level modules (e.g., require('monitor/error-registry') and require('utils/aiox-error')) ensuring ErrorRegistry and AIOXError are imported via absolute paths instead of ../../ relative paths..aiox-core/core/synapse/engine.js (1)
28-29: Use the repo's absolute import form for the new requires.These new relative paths make core imports brittle during moves and violate the import rule.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use absolute imports instead of relative imports in all code.Also applies to: 306-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/synapse/engine.js around lines 28 - 29, The new requires use relative paths which violate the project's absolute-import rule; update the imports in engine.js so that PipelineMetrics and normalizeSession are required via the repository's absolute import form (replace the relative require('../utils/pipeline-metrics') and require('../utils/session-normalizer') with the corresponding absolute module paths), ensuring both PipelineMetrics and normalizeSession references remain unchanged where used elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/constitution.md:
- Around line 128-144: Update the document header to reflect the addition of
Principle VII by bumping the version (e.g., change Version from 1.0.0 to 1.1.0)
and updating the Last Amended date to the current amendment date (replace
2025-01-30 with 2026-04-04), and ensure any header metadata references to
Principle VII, ErrorRegistry, or the gate script scripts/quality/pr-review-ai.js
remain consistent.
- Around line 139-141: The fenced code block under the "Hierarchy" section lacks
a language tag which triggers markdownlint MD040; fix it by adding a language
identifier (e.g., text) to the triple-backtick fence so the block becomes
```text ... ```, updating the fenced block in the "Hierarchy" content to include
that tag.
In @.aiox-core/core/synapse/engine.js:
- Around line 304-311: The fallback that requires and calls ErrorRegistry.log
must be fully guarded: wrap the synchronous
require('../../monitor/error-registry') and the subsequent
ErrorRegistry.log(...) invocation in a try/catch so a missing/malformed module
cannot throw out of the fire-and-forget path; inside that try, call
ErrorRegistry.log(...) and attach a .catch handler that writes a safe message to
stderr (e.g., process.stderr.write) instead of console.error and include the
original error stack (err.stack) in the log payload/message rather than only
err.message; in the outer catch for the require/logging fallback silently
swallow errors (no throwing) to preserve the non-blocking behavior.
In @.aiox-core/core/utils/output-formatter.js:
- Around line 154-155: In format() inside OutputFormatter (output-formatter.js)
wrap the call to ErrorRegistry.log(...) in its own try/catch so any rejection
from ErrorRegistry.log does not overwrite the original formatter error; catch
and handle/log the logging error (e.g., fallback to console.error or a
non-throwing logger) and then rethrow the original formatter error (the one
named error) so callers receive the original failure instead of the registry
failure.
- Line 122: The performance test calls PersonalizedOutputFormatter.format()
without awaiting the async function, so change the test in
tests/unit/output-formatter.test.js to await formatter.format() (i.e., use await
formatter.format()) so the duration measures actual execution; also scan for any
other test usages of PersonalizedOutputFormatter.format and update them to await
the call, and update public docs/examples or migration notes referencing
PersonalizedOutputFormatter (exported via core/index.js and core/index.esm.js)
to indicate format() is async so external consumers can migrate to await the
call.
In @.aiox-core/core/utils/session-normalizer.js:
- Around line 38-40: The current normalization synthesizes
normalized.active_agent as an object ({ id: agentId }) which breaks consumers
expecting the legacy scalar (e.g. "dev"); change the normalization so when
filling from the legacy agentId it preserves the scalar shape (set
normalized.active_agent to agentId string) instead of an object, so
formatSynapseRules()/formatter.js that reads session.active_agent continues to
receive the original scalar value.
- Around line 25-26: normalizeSession currently uses the || operator for
prompt_count which treats 0 as falsy and can overwrite a valid 0; replace that
logic with nullish coalescing (use ??) when assigning normalized.prompt_count so
explicit 0 is preserved, and fix the active_agent synthesis (the block that sets
active_agent = { id: agentId } on lines around 39–40) to set active_agent to the
agentId string (or otherwise extract and store the id) so formatter.js
interpolation (@${session.active_agent}) receives a string instead of an object;
also add unit tests for normalizeSession() covering prompt_count: 0 and
active_agent id handling to prevent regressions.
In @.aiox-core/workflow-intelligence/learning/pattern-store.js:
- Around line 351-355: Remove the console.error fallback in the PatternStore
load error paths and replace it with a governed fallback: when
ErrorRegistry.log(...) rejects (the call in PatternStore that currently ends
with .catch(e => console.error(...))), instead call ErrorRegistry.log again with
a concise message like "[PatternStore] Failed to log load error to
ErrorRegistry: <error>" (marking category 'OPERATIONAL' and
display:false/raw:true to avoid noisy user-facing output) and finally swallow
any subsequent rejection (e.g., a terminating .catch(() => {}) ) to prevent
unhandled rejections; apply this change to the ErrorRegistry.log calls in
PatternStore where console.error is currently used.
In `@packages/installer/src/wizard/index.js`:
- Around line 47-56: The helper logInstallerError is unsafe because it eagerly
requires AIOXError at module load and reads error.message before validating the
thrown value; change it to lazily require AIOXError inside logInstallerError and
avoid direct access to error.message—first normalize the incoming error (e.g.,
if error is falsy or not an object/instance of Error/AIOXError, wrap it or
coerce to a new Error/AIOXError or use String(error) for a message), then
perform your try/catch and calls to ErrorRegistry; update references to
AIOXError and the message construction inside logInstallerError (and the similar
usage on line 64) so the function never assumes error is an Error and only
requires AIOXError when needed.
---
Outside diff comments:
In @.aiox-core/core/utils/output-formatter.js:
- Around line 46-63: The code still uses direct console.warn calls in
_loadPersonaProfile (and the other spots noted) which bypass the ErrorRegistry;
replace those console.warn usages with ErrorRegistry.log(...) calls mirroring
the existing pattern (include a descriptive message, category: 'OPERATIONAL',
display: true, raw: true) and retain the fallback behavior (set
this.personaProfile = this._getNeutralProfile()); ensure you call .catch(() =>
{}) on the promise to swallow errors like the surrounding ErrorRegistry usage so
logging remains non-blocking.
---
Duplicate comments:
In `@tests/synapse/engine.test.js`:
- Around line 249-252: The test constructs a new SynapseEngine but asserts
against the shared `engine` from beforeEach; change the test to capture the
newly created instance (e.g., const localEngine = new
SynapseEngine('/fake/.synapse', { manifest: {} })) inside the
expect(...).not.toThrow() block and assert against that instance's layers
(expect(localEngine.layers.length).toBe(4)); ensure you reference `localEngine`
(or similarly named local variable) instead of the shared `engine` so the
assertion verifies the exact instance created in the test.
---
Nitpick comments:
In @.aiox-core/core/synapse/engine.js:
- Around line 28-29: The new requires use relative paths which violate the
project's absolute-import rule; update the imports in engine.js so that
PipelineMetrics and normalizeSession are required via the repository's absolute
import form (replace the relative require('../utils/pipeline-metrics') and
require('../utils/session-normalizer') with the corresponding absolute module
paths), ensuring both PipelineMetrics and normalizeSession references remain
unchanged where used elsewhere in the file.
In @.aiox-core/workflow-intelligence/learning/pattern-store.js:
- Around line 14-15: The two relative requires in pattern-store.js
(ErrorRegistry and AIOXError) violate the repo rule; replace
require('../../monitor/error-registry') and require('../../utils/aiox-error')
with project-absolute imports so the modules are required as root-level modules
(e.g., require('monitor/error-registry') and require('utils/aiox-error'))
ensuring ErrorRegistry and AIOXError are imported via absolute paths instead of
../../ relative paths.
🪄 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: 5bf9b37f-b6b0-4e0f-9563-a287361ee3b9
📒 Files selected for processing (14)
.aiox-core/constitution.md.aiox-core/core/synapse/engine.js.aiox-core/core/utils/output-formatter.js.aiox-core/core/utils/pipeline-metrics.js.aiox-core/core/utils/session-normalizer.js.aiox-core/data/entity-registry.yaml.aiox-core/infrastructure/scripts/pr-review-ai.js.aiox-core/install-manifest.yaml.aiox-core/workflow-intelligence/learning/gotcha-registry.js.aiox-core/workflow-intelligence/learning/pattern-store.js.aiox-core/workflow-intelligence/learning/qa-feedback.jspackages/installer/src/wizard/index.jstests/synapse/engine.test.jstests/unit/workflow-intelligence/pattern-store.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- .aiox-core/workflow-intelligence/learning/gotcha-registry.js
- .aiox-core/workflow-intelligence/learning/qa-feedback.js
- .aiox-core/install-manifest.yaml
- .aiox-core/infrastructure/scripts/pr-review-ai.js
Comment on lines
+128
to
144
| ### VII. Error Governance (MUST) | ||
|
|
||
| All errors must be traceable, persistent, and categorized to ensure framework observability. | ||
|
|
||
| **Rules:** | ||
| - MUST: All errors must be registered and persisted via `ErrorRegistry`. | ||
| - MUST NOT: Empty `catch` blocks are strictly prohibited; errors must be at least logged. | ||
| - MUST: Categorize errors as `OPERATIONAL` (validation/user-info) or `SYSTEM` (framework bugs/agent failures). | ||
| - MUST: Include contextual metadata (Agent ID, Action, and Timestamp) in every log. | ||
|
|
||
| **Hierarchy:** | ||
| ``` | ||
| System Errors (Critical/Red) → Operational Errors (Audit/Yellow) → Info (Blue) | ||
| ``` | ||
|
|
||
| **Gate:** `scripts/quality/pr-review-ai.js` - BLOCK if raw `console.error` or empty `catch` detected. | ||
|
|
There was a problem hiding this comment.
Update constitution metadata after introducing Principle VII.
This section adds a new constitutional principle, but the document header still shows Version: 1.0.0 and Last Amended: 2025-01-30. Please bump version/amendment metadata to keep governance history accurate.
🧰 Tools
🪛 LanguageTool
[grammar] ~135-~135: Possível erro de concordância de número.
Context: ...AL(validation/user-info) orSYSTEM` (framework bugs/agent failures). - MUST: Include contex...
(GENERAL_NUMBER_AGREEMENT_ERRORS)
🪛 markdownlint-cli2 (0.22.0)
[warning] 139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/constitution.md around lines 128 - 144, Update the document
header to reflect the addition of Principle VII by bumping the version (e.g.,
change Version from 1.0.0 to 1.1.0) and updating the Last Amended date to the
current amendment date (replace 2025-01-30 with 2026-04-04), and ensure any
header metadata references to Principle VII, ErrorRegistry, or the gate script
scripts/quality/pr-review-ai.js remain consistent.
Comment on lines
+139
to
+141
| ``` | ||
| System Errors (Critical/Red) → Operational Errors (Audit/Yellow) → Info (Blue) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced hierarchy block (markdownlint MD040).
The code fence under Hierarchy has no language identifier, which triggers the configured markdownlint warning.
Suggested patch
-```
+```text
System Errors (Critical/Red) → Operational Errors (Audit/Yellow) → Info (Blue)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/constitution.md around lines 139 - 141, The fenced code block
under the "Hierarchy" section lacks a language tag which triggers markdownlint
MD040; fix it by adding a language identifier (e.g., text) to the
triple-backtick fence so the block becomes ```text ... ```, updating the fenced
block in the "Hierarchy" content to include that tag.
Comment on lines
+304
to
+311
| } catch (err) { | ||
| // Fire-and-forget: never block the hook pipeline, but log the failure | ||
| const ErrorRegistry = require('../../monitor/error-registry'); | ||
| ErrorRegistry.log(`[SynapseEngine] Failed to persist hook metrics: ${err.message}`, { | ||
| category: 'SYSTEM', | ||
| display: false, | ||
| raw: true, | ||
| }).catch((e) => console.error(`Failed to log metric error to ErrorRegistry: ${e.message}`)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "engine.js" -path "*synapse*" | head -20Repository: SynkraAI/aiox-core
Length of output: 97
🏁 Script executed:
sed -n '300,320p' ./.aiox-core/core/synapse/engine.jsRepository: SynkraAI/aiox-core
Length of output: 715
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '280,330p'Repository: SynkraAI/aiox-core
Length of output: 1769
🏁 Script executed:
find . -name "error-registry.js" -type fRepository: SynkraAI/aiox-core
Length of output: 100
🏁 Script executed:
cat -n ./.aiox-core/monitor/error-registry.js | head -100Repository: SynkraAI/aiox-core
Length of output: 4021
🏁 Script executed:
cat -n ./.aiox-core/monitor/error-registry.js | sed -n '92,200p'Repository: SynkraAI/aiox-core
Length of output: 4264
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '260,315p'Repository: SynkraAI/aiox-core
Length of output: 2654
🏁 Script executed:
rg -n "_persistHookMetrics" ./.aiox-core/core/synapse/engine.jsRepository: SynkraAI/aiox-core
Length of output: 181
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '185,205p'Repository: SynkraAI/aiox-core
Length of output: 729
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '140,200p'Repository: SynkraAI/aiox-core
Length of output: 2564
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '1,50p'Repository: SynkraAI/aiox-core
Length of output: 2331
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '90,150p'Repository: SynkraAI/aiox-core
Length of output: 2861
🏁 Script executed:
rg -n "process\(" ./.aiox-core/core/synapse/engine.js | head -10Repository: SynkraAI/aiox-core
Length of output: 185
🏁 Script executed:
cat -n ./.aiox-core/core/synapse/engine.js | sed -n '304,315p'Repository: SynkraAI/aiox-core
Length of output: 586
Wrap the error fallback in a try/catch to prevent synchronous require() from escaping governance.
The require('../../monitor/error-registry') call at line 306 is unprotected and can throw synchronously if the module is missing or malformed. When it does, the exception escapes the fire-and-forget intent and propagates to the caller. The .catch() handler only catches promise rejections, not synchronous throws. Additionally, console.error at line 311 reintroduces raw logging that bypasses error governance, and using err.message drops the original stack context.
Wrap both the require and the log call:
try {
const ErrorRegistry = require('../../monitor/error-registry');
ErrorRegistry.log(`[SynapseEngine] Failed to persist hook metrics: ${err.message}`, {
category: 'SYSTEM',
display: false,
raw: true,
}).catch((e) => {
// Log safely to stderr without governance bypass
process.stderr.write(`[SynapseEngine] Failed to log metric error: ${e.message}\n`);
});
} catch (fallbackErr) {
// Silent fallback — never block the pipeline
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/synapse/engine.js around lines 304 - 311, The fallback that
requires and calls ErrorRegistry.log must be fully guarded: wrap the synchronous
require('../../monitor/error-registry') and the subsequent
ErrorRegistry.log(...) invocation in a try/catch so a missing/malformed module
cannot throw out of the fire-and-forget path; inside that try, call
ErrorRegistry.log(...) and attach a .catch handler that writes a safe message to
stderr (e.g., process.stderr.write) instead of console.error and include the
original error stack (err.stack) in the log payload/message rather than only
err.message; in the outer catch for the require/logging fallback silently
swallow errors (no throwing) to preserve the non-blocking behavior.
| * @returns {string} Formatted markdown output | ||
| */ | ||
| format() { | ||
| async format() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find formatter usages that may still assume sync return.
# Expected: all `format()` call sites are awaited or explicitly promise-handled.
rg -nP --type=js --type=mjs --type=cjs -C2 '\bPersonalizedOutputFormatter\b'
rg -nP --type=js --type=mjs --type=cjs -C2 '\.format\s*\('
rg -nP --type=js --type=mjs --type=cjs -C2 '(?<!await\s)(new\s+PersonalizedOutputFormatter\([^)]*\)\.format\s*\(|\b\w+\.format\s*\()'Repository: SynkraAI/aiox-core
Length of output: 145
🏁 Script executed:
#!/bin/bash
# First, check the actual file to see the format() method signature
cat -n .aiox-core/core/utils/output-formatter.js | head -150 | tail -50Repository: SynkraAI/aiox-core
Length of output: 1686
🏁 Script executed:
#!/bin/bash
# Find PersonalizedOutputFormatter usages
rg -n 'PersonalizedOutputFormatter' --type=js -C2Repository: SynkraAI/aiox-core
Length of output: 14782
🏁 Script executed:
#!/bin/bash
# Find all .format() calls to check if they're awaited
rg -n '\.format\s*\(' --type=js -C2Repository: SynkraAI/aiox-core
Length of output: 7125
🏁 Script executed:
#!/bin/bash
# Check the exports in the index files
cat .aiox-core/core/index.js | grep -n -i 'PersonalizedOutputFormatter\|export'Repository: SynkraAI/aiox-core
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Also check the esm index
cat .aiox-core/core/index.esm.js | grep -n -i 'PersonalizedOutputFormatter\|export'Repository: SynkraAI/aiox-core
Length of output: 1083
🏁 Script executed:
#!/bin/bash
# Find .format() calls outside test directories
rg -n '\.format\s*\(' --type=js | grep -v '/tests/' | grep -v '\.test\.'Repository: SynkraAI/aiox-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look for production/main call sites that may use PersonalizedOutputFormatter
rg -n 'PersonalizedOutputFormatter' --type=js | grep -v '/tests/' | grep -v '\.test\.'Repository: SynkraAI/aiox-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if there's a synchronous wrapper or compatibility layer
rg -n 'format\(\)' --type=js .aiox-core/core/ | head -30Repository: SynkraAI/aiox-core
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Search for any file that might execute the formatter (agent files, task execution)
fd -type f -name '*.js' | xargs rg -l 'format' --type=js | grep -v '/tests/' | head -20Repository: SynkraAI/aiox-core
Length of output: 975
🏁 Script executed:
#!/bin/bash
# Look at the test file line 300 more closely to understand the context
sed -n '296,305p' tests/unit/output-formatter.test.jsRepository: SynkraAI/aiox-core
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Check the complete exports in both index files to see what's exported
grep -A 5 -B 5 'PersonalizedOutputFormatter' .aiox-core/core/index.jsRepository: SynkraAI/aiox-core
Length of output: 808
🏁 Script executed:
#!/bin/bash
# Look for any agent execution logic that might use the formatter
fd -type f -name 'agent*' -o -name '*execution*' -o -name '*runner*' | head -10Repository: SynkraAI/aiox-core
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Look in the actual source files (not tests) for usage
find .aiox-core/core -name '*.js' -type f | xargs rg 'format\(\)' 2>/dev/null || echo "No .format() found in core source"Repository: SynkraAI/aiox-core
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or migration guide
fd -type f -name 'CHANGELOG*' -o -name 'MIGRATION*' -o -name 'README*' | head -5Repository: SynkraAI/aiox-core
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check the full context around the test on line 300 to understand the intent
sed -n '295,310p' tests/unit/output-formatter.test.jsRepository: SynkraAI/aiox-core
Length of output: 727
🏁 Script executed:
#!/bin/bash
# Search for any agent files or main execution entry points
find .aiox-core -type f -name '*.js' | xargs rg -l 'new PersonalizedOutputFormatter\|\.format\(' | grep -v test | head -10Repository: SynkraAI/aiox-core
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if PersonalizedOutputFormatter is actually used anywhere in production code (not tests)
rg -n 'PersonalizedOutputFormatter' .aiox-core/core --type=jsRepository: SynkraAI/aiox-core
Length of output: 512
🏁 Script executed:
#!/bin/bash
# Check if the test at line 300 is actually problematic or intentional - look at the complete test
sed -n '297,310p' tests/unit/output-formatter.test.jsRepository: SynkraAI/aiox-core
Length of output: 692
🏁 Script executed:
#!/bin/bash
# Verify the actual exports to see if there's maybe a synchronous version or wrapper
cat .aiox-core/core/utils/output-formatter.js | head -10 && echo "---" && tail -20 .aiox-core/core/utils/output-formatter.jsRepository: SynkraAI/aiox-core
Length of output: 973
🏁 Script executed:
#!/bin/bash
# Check git history to understand when format became async
git log --oneline -n 20 .aiox-core/core/utils/output-formatter.js 2>/dev/null || echo "Git history not available"Repository: SynkraAI/aiox-core
Length of output: 133
Fix test at line 300 that doesn't await the async format() call.
The format() method is correctly async, and most tests properly await it. However, the performance test at line 300 (tests/unit/output-formatter.test.js) calls formatter.format() without await, which means the duration measurement is incomplete and doesn't actually test the method's execution time. Update this test to await formatter.format() to correctly measure performance.
Regarding backwards compatibility: the API change from sync to async is a breaking change for any external consumers of PersonalizedOutputFormatter. While no production call sites outside tests were found during verification, the public export via .aiox-core/core/index.js and .aiox-core/core/index.esm.js means this affects the public contract. Ensure all documented usage examples and migration guides reflect the async requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/utils/output-formatter.js at line 122, The performance test
calls PersonalizedOutputFormatter.format() without awaiting the async function,
so change the test in tests/unit/output-formatter.test.js to await
formatter.format() (i.e., use await formatter.format()) so the duration measures
actual execution; also scan for any other test usages of
PersonalizedOutputFormatter.format and update them to await the call, and update
public docs/examples or migration notes referencing PersonalizedOutputFormatter
(exported via core/index.js and core/index.esm.js) to indicate format() is async
so external consumers can migrate to await the call.
Comment on lines
+154
to
155
| await ErrorRegistry.log(`[OutputFormatter] Format error: ${error.message}`, { category: 'SYSTEM', display: true, raw: true }); | ||
| throw error; |
There was a problem hiding this comment.
Do not let registry failures mask the original formatter error.
If ErrorRegistry.log() rejects at Line 154, format() can throw the logging failure instead of the original formatting failure.
Suggested fix
- await ErrorRegistry.log(`[OutputFormatter] Format error: ${error.message}`, { category: 'SYSTEM', display: true, raw: true });
+ await ErrorRegistry
+ .log(`[OutputFormatter] Format error: ${error.message}`, { category: 'SYSTEM', display: true, raw: true })
+ .catch(() => {});
throw error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/utils/output-formatter.js around lines 154 - 155, In
format() inside OutputFormatter (output-formatter.js) wrap the call to
ErrorRegistry.log(...) in its own try/catch so any rejection from
ErrorRegistry.log does not overwrite the original formatter error; catch and
handle/log the logging error (e.g., fallback to console.error or a non-throwing
logger) and then rethrow the original formatter error (the one named error) so
callers receive the original failure instead of the registry failure.
Comment on lines
+25
to
+26
| // 1. Normalize prompt_count | ||
| normalized.prompt_count = session.prompt_count || session.promptCount || 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "session-normalizer.js" | head -20Repository: SynkraAI/aiox-core
Length of output: 107
🏁 Script executed:
cat -n .aiox-core/core/utils/session-normalizer.jsRepository: SynkraAI/aiox-core
Length of output: 1788
🏁 Script executed:
cat -n .aiox-core/core/synapse/engine.js | head -80Repository: SynkraAI/aiox-core
Length of output: 3530
🏁 Script executed:
grep -n "prompt_count\|bracket" .aiox-core/core/synapse/engine.js | head -40Repository: SynkraAI/aiox-core
Length of output: 1733
🏁 Script executed:
grep -n "calculateBracket" .aiox-core/core/synapse/context/context-tracker.js | head -20Repository: SynkraAI/aiox-core
Length of output: 132
🏁 Script executed:
sed -n '81,120p' .aiox-core/core/synapse/context/context-tracker.jsRepository: SynkraAI/aiox-core
Length of output: 1259
🏁 Script executed:
sed -n '320,330p' .aiox-core/core/synapse/output/formatter.jsRepository: SynkraAI/aiox-core
Length of output: 435
🏁 Script executed:
find . -type f -name "*.test.js" -o -name "*.spec.js" | xargs grep -l "session-normalizer\|normalizeSession" 2>/dev/null | head -10Repository: SynkraAI/aiox-core
Length of output: 44
🏁 Script executed:
find . -type f \( -name "*.test.js" -o -name "*.spec.js" \) | head -20Repository: SynkraAI/aiox-core
Length of output: 1438
🏁 Script executed:
grep -r "normalizeSession\|session-normalizer" ./.aiox-core/core --include="*.test.js" --include="*.spec.js"Repository: SynkraAI/aiox-core
Length of output: 44
🏁 Script executed:
cat -n .aiox-core/core/synapse/output/formatter.js | sed -n '310,335p'Repository: SynkraAI/aiox-core
Length of output: 1150
🏁 Script executed:
grep -n "formatSynapseRules\|formatter" .aiox-core/core/synapse/engine.js | head -20Repository: SynkraAI/aiox-core
Length of output: 281
🏁 Script executed:
grep -rn "session\.active_agent\|active_agent" .aiox-core/core --include="*.js" | grep -v "node_modules"Repository: SynkraAI/aiox-core
Length of output: 3241
🏁 Script executed:
sed -n '190,210p' .aiox-core/core/synapse/engine.jsRepository: SynkraAI/aiox-core
Length of output: 535
🏁 Script executed:
sed -n '28,44p' .aiox-core/core/utils/session-normalizer.jsRepository: SynkraAI/aiox-core
Length of output: 698
🏁 Script executed:
sed -n '1,10p' .aiox-core/core/synapse/output/formatter.jsRepository: SynkraAI/aiox-core
Length of output: 436
Preserve explicit prompt_count: 0 during normalization and fix formatter incompatibility with active_agent object shape.
The || operator treats 0 as falsy, so a session with prompt_count: 0 will fall through to a stale promptCount value. This directly impacts engine.js line 138, which uses the normalized value to calculate context percentage and select the bracket (FRESH/MODERATE/DEPLETED/CRITICAL). Use nullish coalescing instead.
Additionally, lines 39-40 synthesize active_agent as an object { id: agentId }, but formatter.js line 326 interpolates it as a string (@${session.active_agent}), producing @[object Object] in output. Either extract the ID when setting it or update the formatter to handle the object shape.
Suggested fix for prompt_count
- normalized.prompt_count = session.prompt_count || session.promptCount || 0;
+ normalized.prompt_count = session.prompt_count ?? session.promptCount ?? 0;No tests exist for normalizeSession() — add coverage for edge cases like prompt_count: 0 before landing.
Per coding guidelines (.aiox-core/core/**): Ensure backwards compatibility — core modules are consumed by all agents.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 1. Normalize prompt_count | |
| normalized.prompt_count = session.prompt_count || session.promptCount || 0; | |
| // 1. Normalize prompt_count | |
| normalized.prompt_count = session.prompt_count ?? session.promptCount ?? 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/utils/session-normalizer.js around lines 25 - 26,
normalizeSession currently uses the || operator for prompt_count which treats 0
as falsy and can overwrite a valid 0; replace that logic with nullish coalescing
(use ??) when assigning normalized.prompt_count so explicit 0 is preserved, and
fix the active_agent synthesis (the block that sets active_agent = { id: agentId
} on lines around 39–40) to set active_agent to the agentId string (or otherwise
extract and store the id) so formatter.js interpolation
(@${session.active_agent}) receives a string instead of an object; also add unit
tests for normalizeSession() covering prompt_count: 0 and active_agent id
handling to prevent regressions.
Comment on lines
+38
to
+40
| // 3. Keep backward compatibility for layers that still use snake_case | ||
| if (!normalized.active_agent && agentId) { | ||
| normalized.active_agent = { id: agentId }; |
There was a problem hiding this comment.
Don't synthesize active_agent as an object unless every legacy consumer was migrated.
engine.js now passes the normalized session into formatSynapseRules(), but .aiox-core/core/synapse/output/formatter.js:325-327 still renders session.active_agent directly. With an input like { activeAgent: 'dev' }, this produces Agent: @[object Object] instead of @dev. Preserve the legacy scalar shape here or update the formatter contract in the same PR.
As per coding guidelines, .aiox-core/core/**: Ensure backwards compatibility — core modules are consumed by all agents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/utils/session-normalizer.js around lines 38 - 40, The
current normalization synthesizes normalized.active_agent as an object ({ id:
agentId }) which breaks consumers expecting the legacy scalar (e.g. "dev");
change the normalization so when filling from the legacy agentId it preserves
the scalar shape (set normalized.active_agent to agentId string) instead of an
object, so formatSynapseRules()/formatter.js that reads session.active_agent
continues to receive the original scalar value.
Comment on lines
+47
to
+56
| const AIOXError = require('../../../../.aiox-core/utils/aiox-error'); | ||
|
|
||
| /** | ||
| * Safe helper to log installer errors to ErrorRegistry if available (Principle VII) | ||
| * @param {string} message - Error message | ||
| * @param {Error|AIOXError} error - Error object | ||
| * @param {Object} metadata - Optional metadata | ||
| */ | ||
| async function logInstallerError(message, error, metadata = {}) { | ||
| const fullMessage = `${message}: ${error.message}`; |
There was a problem hiding this comment.
Harden logInstallerError against non-Error throw values and remove eager .aiox-core load
error.message is read before the try block (Line 56). If a caller throws a non-Error, this helper can throw and bypass your fallback path. Also, the top-level AIOXError require (Line 47) creates a hard startup dependency that this “safe” helper does not need.
💡 Proposed fix
-const AIOXError = require('../../../../.aiox-core/utils/aiox-error');
-
/**
* Safe helper to log installer errors to ErrorRegistry if available (Principle VII)
* `@param` {string} message - Error message
- * `@param` {Error|AIOXError} error - Error object
+ * `@param` {unknown} error - Error-like value
* `@param` {Object} metadata - Optional metadata
*/
async function logInstallerError(message, error, metadata = {}) {
- const fullMessage = `${message}: ${error.message}`;
+ let fullMessage = message;
try {
+ const normalizedError = error instanceof Error ? error : new Error(String(error));
+ fullMessage = `${message}: ${normalizedError.message}`;
+
// Attempt to load ErrorRegistry dynamically from the expected relative path
const ErrorRegistry = require('../../../../.aiox-core/monitor/error-registry');
await ErrorRegistry.log(fullMessage, {
category: 'SYSTEM',
action: 'installer-wizard',
- metadata: { ...metadata, stack: error.stack }
+ metadata: { ...metadata, stack: normalizedError.stack }
});
} catch (e) {
// Fallback if ErrorRegistry is not yet installed or accessible
console.error(`\n[Installer Fallback] ${fullMessage}`);
}
}Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/installer/src/wizard/index.js` around lines 47 - 56, The helper
logInstallerError is unsafe because it eagerly requires AIOXError at module load
and reads error.message before validating the thrown value; change it to lazily
require AIOXError inside logInstallerError and avoid direct access to
error.message—first normalize the incoming error (e.g., if error is falsy or not
an object/instance of Error/AIOXError, wrap it or coerce to a new
Error/AIOXError or use String(error) for a message), then perform your try/catch
and calls to ErrorRegistry; update references to AIOXError and the message
construction inside logInstallerError (and the similar usage on line 64) so the
function never assumes error is an Error and only requires AIOXError when
needed.
…agram - Integrate native LockManager for cross-process file locking - Transition from console.error to process.stderr.write for direct emission - Strengthen self-healing with robust log backup on corruption - Optimize log() entry point with fast-path init() checks
- Update ErrorRegistry._normalizeError to clone AIOXError instances instead of mutating them - Add .catch() handlers to fire-and-forget ErrorRegistry.log calls in SuggestionEngine to prevent unhandled rejections
- Change unawaited .catch() pattern to proper await in buildContext catch block - Ensure log flushing before proceeding in async execution path
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.aiox-core/monitor/error-registry.js (2)
246-290: Lock release is always attempted, even when acquisition failed.When
acquireLockreturnsfalse(line 249), the code throws, but thefinallyblock still callsreleaseLock(line 280). This works safely becauseLockManager.releaseLockchecks PID ownership before removing the lock. However, for clarity, consider restructuring to only release if acquired:♻️ Optional: Track lock acquisition state explicitly
async _drain() { if (this._isDraining) return; this._isDraining = true; this._drainScheduled = false; const batch = [...this._queue]; this._queue = []; if (batch.length === 0) { this._isDraining = false; return; } + let lockAcquired = false; try { - const lockAcquired = await this.lockManager.acquireLock('errors-json', { ttlSeconds: 60 }); + lockAcquired = await this.lockManager.acquireLock('errors-json', { ttlSeconds: 60 }); if (!lockAcquired) { throw new Error('Could not acquire lock on error registry after multiple retries'); } // ... rest of try block } catch (err) { // ... error handling } finally { - await this.lockManager.releaseLock('errors-json'); + if (lockAcquired) { + await this.lockManager.releaseLock('errors-json'); + } this._isDraining = false; // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 246 - 290, The finally block always calls this.lockManager.releaseLock even when this.lockManager.acquireLock returned false; fix by tracking the acquisition result (keep the existing lockAcquired variable) and only call this.lockManager.releaseLock('errors-json') in finally if lockAcquired is true; update the try/catch/finally around this.lockManager.acquireLock, _readLogFile, _rotateEntries, _safeStringify and fsp.writeFile so that on failure you still set this._isDraining = false, schedule _scheduleDrain if needed, and reject batch items, but guard the releaseLock call with the lockAcquired flag.
72-91: Initialization failure handling is lenient by design.The
init()method catches errors and logs to stderr but allows the promise to resolve successfully even on failure. This means subsequentlog()calls will proceed and fail in_drain()when attempting to write. The self-healing approach in_drain()handles this gracefully, but consider whether a failed init should set a flag to enable a more explicit fallback path (e.g., skip persistence entirely rather than attempting and failing each time).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 72 - 91, init() currently swallows filesystem errors and allows callers to proceed, causing repeated failures in _drain(); modify init() so that in the catch block you set a explicit flag (e.g., this._persistenceAvailable = false or this._initFailed = true) and keep _initPromise resolved, and then update _drain() (and log()) to check that flag and skip any attempt to write/read files when persistence is unavailable (fall back to in-memory/no-op behavior); use the existing symbols init(), _initPromise, _drain(), and log() to locate and implement the flag and guards..aiox-core/infrastructure/scripts/config-loader.js (1)
345-348: CLI error handling could be more resilient.Similar to the earlier catch block: if
ErrorRegistry.log()rejects,process.exit(1)is never reached. Node's unhandled rejection handling will exit the process, but behavior varies by Node version. Since this file is deprecated, this is acceptable, but the same fire-and-forget pattern would be safer.♻️ Optional: Fire-and-forget for CLI error path
} catch (error) { - await ErrorRegistry.log(`Error: ${error.message}`, { category: 'SYSTEM', display: true, raw: true, metadata: { stack: error.stack } }); + // Fire-and-forget: message is displayed immediately; persistence is best-effort + ErrorRegistry.log(`Error: ${error.message}`, { category: 'SYSTEM', display: true, raw: true, metadata: { stack: error.stack } }) + .catch(() => {}); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/infrastructure/scripts/config-loader.js around lines 345 - 348, The catch block awaits ErrorRegistry.log which can reject and prevent process.exit(1) from running; change to a fire-and-forget call so the process exits reliably: invoke ErrorRegistry.log(...) without awaiting (or append a .catch(() => {}) to swallow errors) inside the catch handler that currently calls process.exit(1), ensuring you still pass the same message, category, display, raw, and metadata (stack) parameters to ErrorRegistry.log while immediately calling process.exit(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/infrastructure/scripts/config-loader.js:
- Around line 113-116: The catch block currently awaits ErrorRegistry.log and
can mask the original error if logging rejects; change the call to
fire-and-forget so the original error is always re-thrown: call
ErrorRegistry.log(...) without await and attach a .catch(...) to swallow or
internally report logging failures, then immediately throw the original
Error(`Config load failed: ${error.message}`) as before; update the catch
handling around ErrorRegistry.log and the throw in the same catch block
(referencing ErrorRegistry.log and the thrown Error in this file) so logging
failures cannot override the original exception flow.
---
Nitpick comments:
In @.aiox-core/infrastructure/scripts/config-loader.js:
- Around line 345-348: The catch block awaits ErrorRegistry.log which can reject
and prevent process.exit(1) from running; change to a fire-and-forget call so
the process exits reliably: invoke ErrorRegistry.log(...) without awaiting (or
append a .catch(() => {}) to swallow errors) inside the catch handler that
currently calls process.exit(1), ensuring you still pass the same message,
category, display, raw, and metadata (stack) parameters to ErrorRegistry.log
while immediately calling process.exit(1).
In @.aiox-core/monitor/error-registry.js:
- Around line 246-290: The finally block always calls
this.lockManager.releaseLock even when this.lockManager.acquireLock returned
false; fix by tracking the acquisition result (keep the existing lockAcquired
variable) and only call this.lockManager.releaseLock('errors-json') in finally
if lockAcquired is true; update the try/catch/finally around
this.lockManager.acquireLock, _readLogFile, _rotateEntries, _safeStringify and
fsp.writeFile so that on failure you still set this._isDraining = false,
schedule _scheduleDrain if needed, and reject batch items, but guard the
releaseLock call with the lockAcquired flag.
- Around line 72-91: init() currently swallows filesystem errors and allows
callers to proceed, causing repeated failures in _drain(); modify init() so that
in the catch block you set a explicit flag (e.g., this._persistenceAvailable =
false or this._initFailed = true) and keep _initPromise resolved, and then
update _drain() (and log()) to check that flag and skip any attempt to
write/read files when persistence is unavailable (fall back to in-memory/no-op
behavior); use the existing symbols init(), _initPromise, _drain(), and log() to
locate and implement the flag and guards.
🪄 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: ae64f3ca-017a-4843-a62f-5a26be59f533
📒 Files selected for processing (5)
.aiox-core/infrastructure/scripts/config-loader.js.aiox-core/infrastructure/scripts/conflict-resolver.js.aiox-core/install-manifest.yaml.aiox-core/monitor/error-registry.jstests/monitor/error-registry.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- .aiox-core/infrastructure/scripts/conflict-resolver.js
- tests/monitor/error-registry.test.js
- .aiox-core/install-manifest.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/monitor/error-registry.js:
- Around line 247-291: The try/finally must track whether the lock was actually
acquired so releaseLock isn't called when acquireLock returned false; update the
persistence routine to record the boolean from lockManager.acquireLock (already
assigned to lockAcquired) and in the finally block call
lockManager.releaseLock('errors-json') only if lockAcquired is true, while
preserving the existing cleanup steps (setting this._isDraining = false and
calling this._scheduleDrain() if this._queue.length > 0) and keeping existing
resolve/reject behavior for batch items; reference acquireLock, releaseLock,
lockManager, lockAcquired, this._isDraining, and this._scheduleDrain when making
the change.
🪄 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: e9741331-6bec-47d7-835e-3c1229eb242e
📒 Files selected for processing (3)
.aiox-core/install-manifest.yaml.aiox-core/monitor/error-registry.js.aiox-core/workflow-intelligence/engine/suggestion-engine.js
✅ Files skipped from review due to trivial changes (2)
- .aiox-core/workflow-intelligence/engine/suggestion-engine.js
- .aiox-core/install-manifest.yaml
…ning - Ensure message and stack are explicitly copied during normalization - Prevent regression where log entries lost critical error details - Add regression test for cloning integrity and mutation prevention
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.aiox-core/monitor/error-registry.js (1)
251-295:⚠️ Potential issue | 🟠 MajorLock may be released without being acquired (past issue persists).
The
lockAcquiredvariable is scoped inside thetryblock, butreleaseLockis called unconditionally in thefinallyblock. IfacquireLockreturnsfalseand the code throws at line 255, thefinallyblock will still attempt to release a lock that was never held.While
LockManager.releaseLock()handles this gracefully (returnsfalseif not owned), it's semantically incorrect and could mask bugs.🐛 Proposed fix: track lock acquisition state
async _drain() { if (this._isDraining) return; this._isDraining = true; this._drainScheduled = false; // Capture the current queue and clear it immediately const batch = [...this._queue]; this._queue = []; if (batch.length === 0) { this._isDraining = false; return; } + let lockAcquired = false; try { // 1. Acquire lock (Formal LockManager from Story 12.3) - const lockAcquired = await this.lockManager.acquireLock('errors-json', { ttlSeconds: 60 }); + lockAcquired = await this.lockManager.acquireLock('errors-json', { ttlSeconds: 60 }); if (!lockAcquired) { throw new Error('Could not acquire lock on error registry after multiple retries'); } // ... rest unchanged ... } catch (err) { // ... unchanged ... } finally { // 7. Release lock only if acquired - await this.lockManager.releaseLock('errors-json'); + if (lockAcquired) { + await this.lockManager.releaseLock('errors-json'); + } // 8. Ready for next this._isDraining = false; // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 251 - 295, Declare and initialize a boolean lockAcquired flag outside the try (e.g., let lockAcquired = false), set it to the result of this.lockManager.acquireLock('errors-json', { ttlSeconds: 60 }) inside the try, and then in the finally only call this.lockManager.releaseLock('errors-json') if lockAcquired is true; keep the current error handling that rejects batch items and the post-write logic (_isDraining = false, _scheduleDrain()) intact so you don't release a lock you never obtained and still handle failures correctly.
🧹 Nitpick comments (5)
tests/monitor/error-registry.test.js (3)
22-24: Consider removing emptyafterAllhook or implementing cleanup.The empty hook adds no value. Either remove it or implement the cleanup it describes:
- afterAll(() => { - // Cleanup is optional for local dev, but good practice - }); + afterAll(() => { + // Clean up test artifacts + if (fs.existsSync(logFile)) { + fs.unlinkSync(logFile); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitor/error-registry.test.js` around lines 22 - 24, The empty afterAll hook in tests/monitor/error-registry.test.js (the afterAll(() => { /* Cleanup is optional... */ }); block) should be removed or replaced with actual cleanup logic; either delete the afterAll entirely to avoid a no-op hook, or implement the intended teardown (e.g., restore mocks, close DB/connections, or clear test fixtures) inside that afterAll to perform proper cleanup after the error-registry tests run.
59-80: Consider adding assertion for merged metadata preservation.The test validates that the original isn't mutated, but doesn't verify whether the logged error preserves the original's metadata keys. Adding this would catch the shallow merge behavior:
// Add after line 73: expect(logged.metadata.original).toBe(true); // Verify original metadata preservedThis would fail with the current implementation, exposing the shallow merge issue flagged in the source file review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitor/error-registry.test.js` around lines 59 - 80, The test should assert that logged metadata preserves original keys and that logging doesn't mutate the original AIOXError: add an assertion like expect(logged.metadata.original).toBe(true) in the test case, and fix the shallow-merge bug in ErrorRegistry.log so it creates the logged error metadata by merging original.metadata and options.metadata into a new object (e.g., newMeta = { ...original.metadata, ...options.metadata }) without mutating original.metadata when constructing the logged AIOXError; ensure AIOXError instances and stack are preserved as before.
6-9: Use absolute imports instead of relative imports.Per coding guidelines, use absolute imports:
-const ErrorRegistry = require('../../.aiox-core/monitor/error-registry'); -const AIOXError = require('../../.aiox-core/utils/aiox-error'); +const ErrorRegistry = require('.aiox-core/monitor/error-registry'); +const AIOXError = require('.aiox-core/utils/aiox-error');As per coding guidelines: "Use absolute imports instead of relative imports in all code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitor/error-registry.test.js` around lines 6 - 9, Replace the relative requires with absolute module imports: change require('../../.aiox-core/monitor/error-registry') to require('aiox-core/monitor/error-registry') and require('../../.aiox-core/utils/aiox-error') to require('aiox-core/utils/aiox-error'), leaving core modules (fs, path) as-is; update the import statements in tests/monitor/error-registry.test.js so they reference the package-style absolute paths and run the tests to ensure the module resolver finds the new paths..aiox-core/monitor/error-registry.js (2)
12-16: Use absolute imports instead of relative imports.The coding guidelines require absolute imports. Consider updating these to use absolute paths from the project root or a configured alias.
-const LockManager = require('../core/orchestration/lock-manager'); -const AIOXError = require('../utils/aiox-error'); +const LockManager = require('.aiox-core/core/orchestration/lock-manager'); +const AIOXError = require('.aiox-core/utils/aiox-error');As per coding guidelines: "Use absolute imports instead of relative imports in all code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 12 - 16, Update the two relative requires for your internal modules to use the project's absolute import scheme instead of ../ paths: replace require('../core/orchestration/lock-manager') (LockManager) and require('../utils/aiox-error') (AIOXError) with the configured absolute paths or alias used in the repo (e.g., core/orchestration/lock-manager or `@aiox/core/orchestration/lock-manager` and `@aiox/utils/aiox-error`), leaving Node built-ins (fs, fs/promises, path) as-is; ensure the new module specifiers resolve with the project's runtime/build config (or adjust the alias if needed).
349-358: WeakSet-based serialization marks repeated references as circular.The
cacheWeakSet persists across the entire array being serialized. If the same object reference appears in multiple entries (not circular, just shared), subsequent occurrences will be replaced with'[Circular]'. This is more aggressive than necessary.Example: if two error entries share the same metadata object, the second would lose that metadata.
If this behavior is intentional for safety, consider adding a brief comment. Otherwise:
♻️ Alternative: reset cache per top-level entry
A more precise approach would reset the cache per entry, but the current approach prioritizes crash-safety over fidelity, which may be acceptable for error logging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/monitor/error-registry.js around lines 349 - 358, _safeStringify currently uses a single WeakSet cache for the whole serialization which turns repeated shared references into '[Circular]'; change it to reset the WeakSet for each top-level entry: detect when data is an array and, if so, stringify each element with a fresh WeakSet-based replacer (or otherwise create a new cache per top-level object) so shared references across different entries are preserved, and add a brief comment in _safeStringify explaining the chosen behavior if keeping the aggressive approach is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.aiox-core/monitor/error-registry.js:
- Around line 153-162: The _normalizeError method shallowly merges error and
options via Object.assign which replaces error.metadata when options.metadata
exists; modify _normalizeError (handling AIOXError clones) to merge metadata
deeply: after creating clone, preserve existing error.message/stack, then if
both error.metadata and options.metadata are plain objects create a merged
metadata object (e.g., merge error.metadata with options.metadata so original
keys are retained unless overridden) and assign it to clone.metadata; otherwise
fallback to assigning whichever metadata exists; keep the rest of Object.assign
for top-level fields or use a utility deep-merge for nested metadata if
available.
In `@tests/monitor/error-registry.test.js`:
- Around line 26-32: The test currently calls ErrorRegistry.getRecentErrors()
which is synchronous and may not invoke initialization; change the test to
explicitly call and await ErrorRegistry.init() (make the test async) before
asserting the log directory/file exist to reliably exercise the initialization
path in ErrorRegistry.init() and ensure the assertions use the same
logDir/logFile variables after init completes.
- Around line 15-20: The beforeEach block can throw ENOENT because
fs.writeFileSync(logFile, ...) assumes the log directory exists; update the
beforeEach to ensure the directory for logFile exists before writing (use
path.dirname(logFile) and fs.existsSync or fs.mkdirSync with { recursive: true
}) and then call fs.writeFileSync(logFile, JSON.stringify([], null, 2), 'utf8');
reference the beforeEach, logFile, and fs.writeFileSync symbols when making this
change.
---
Duplicate comments:
In @.aiox-core/monitor/error-registry.js:
- Around line 251-295: Declare and initialize a boolean lockAcquired flag
outside the try (e.g., let lockAcquired = false), set it to the result of
this.lockManager.acquireLock('errors-json', { ttlSeconds: 60 }) inside the try,
and then in the finally only call this.lockManager.releaseLock('errors-json') if
lockAcquired is true; keep the current error handling that rejects batch items
and the post-write logic (_isDraining = false, _scheduleDrain()) intact so you
don't release a lock you never obtained and still handle failures correctly.
---
Nitpick comments:
In @.aiox-core/monitor/error-registry.js:
- Around line 12-16: Update the two relative requires for your internal modules
to use the project's absolute import scheme instead of ../ paths: replace
require('../core/orchestration/lock-manager') (LockManager) and
require('../utils/aiox-error') (AIOXError) with the configured absolute paths or
alias used in the repo (e.g., core/orchestration/lock-manager or
`@aiox/core/orchestration/lock-manager` and `@aiox/utils/aiox-error`), leaving Node
built-ins (fs, fs/promises, path) as-is; ensure the new module specifiers
resolve with the project's runtime/build config (or adjust the alias if needed).
- Around line 349-358: _safeStringify currently uses a single WeakSet cache for
the whole serialization which turns repeated shared references into
'[Circular]'; change it to reset the WeakSet for each top-level entry: detect
when data is an array and, if so, stringify each element with a fresh
WeakSet-based replacer (or otherwise create a new cache per top-level object) so
shared references across different entries are preserved, and add a brief
comment in _safeStringify explaining the chosen behavior if keeping the
aggressive approach is intentional.
In `@tests/monitor/error-registry.test.js`:
- Around line 22-24: The empty afterAll hook in
tests/monitor/error-registry.test.js (the afterAll(() => { /* Cleanup is
optional... */ }); block) should be removed or replaced with actual cleanup
logic; either delete the afterAll entirely to avoid a no-op hook, or implement
the intended teardown (e.g., restore mocks, close DB/connections, or clear test
fixtures) inside that afterAll to perform proper cleanup after the
error-registry tests run.
- Around line 59-80: The test should assert that logged metadata preserves
original keys and that logging doesn't mutate the original AIOXError: add an
assertion like expect(logged.metadata.original).toBe(true) in the test case, and
fix the shallow-merge bug in ErrorRegistry.log so it creates the logged error
metadata by merging original.metadata and options.metadata into a new object
(e.g., newMeta = { ...original.metadata, ...options.metadata }) without mutating
original.metadata when constructing the logged AIOXError; ensure AIOXError
instances and stack are preserved as before.
- Around line 6-9: Replace the relative requires with absolute module imports:
change require('../../.aiox-core/monitor/error-registry') to
require('aiox-core/monitor/error-registry') and
require('../../.aiox-core/utils/aiox-error') to
require('aiox-core/utils/aiox-error'), leaving core modules (fs, path) as-is;
update the import statements in tests/monitor/error-registry.test.js so they
reference the package-style absolute paths and run the tests to ensure the
module resolver finds the new paths.
🪄 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: 574a2d34-2031-4080-875c-cc32ac47d5dc
📒 Files selected for processing (3)
.aiox-core/install-manifest.yaml.aiox-core/monitor/error-registry.jstests/monitor/error-registry.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
…king errors
- Change awaited ErrorRegistry.log to fire-and-forget with .catch(() => {}) in loadFullConfig
- Ensure original configuration load error is always thrown even if logging fails
…engine syntax - Convert PatternStore persistence to async/await with fs/promises - Refactor learning module index and QA feedback processor for async support - Fix suggestion-engine syntax error by marking _applyLearnedPatternBoost as async - Update unit tests to support async execution and fix engine test assertions
…ance) Migrate generic Error throws to AIOXError and replace console.error/warn with ErrorRegistry.log in config-loader and config-resolver modules.
Replace console.warn/error with ErrorRegistry.log in layer loading and metrics persistence to align with Principle VII.
Update qa-feedback and gotcha-registry to use ErrorRegistry governance and absolute-style imports.
…otstrap - Implement lazy-loading ErrorRegistry in bin/aiox.js for safe bootstrap error reporting. - Align MasterOrchestrator with Principle VII using AIOXError and persistent logging. - Synchronize versioning and fix various typos across core modules.
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.
Pull Request: Framework Observability & Error Governance (Epic 7)
📋 Description
This Pull Request codifies and implements Principle VII: Error Governance within the AIOX core. It establishes a centralized observability layer by
introducing a persistent ErrorRegistry, effectively eliminating "silent failures" and non-governed logging patterns (console.error/console.warn).
Update (Radiography Refinement): This PR now includes a complete architectural audit (Radiography) fixing critical hotspots in SynapseEngine, MasterOrchestrator, and CLI Bootstrap.
🎯 AIOX Story Reference
Acceptance Criteria Addressed
🔗 Related Issues
📦 Type of Change
📝 Technical Changes & Improvements (Post-Audit)
🧪 Validation & Testing
✅ Quality Checklist
🔒 Security & Performance