docs: improve core JSDoc coverage - #679
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
WalkthroughExpanded JSDoc across core config, events, and orchestration modules; added exported helper getConfigSection(sectionName); hardened RecoveryHandler APIs with epicNum validation and sanitized cloned attempt-history returns; tests added; install manifest hashes refreshed. ChangesCore Documentation & Public API Clarification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
added
area: agents
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.aiox-core/core/orchestration/recovery-handler.js (2)
699-735: ⚡ Quick winNew public methods accept
epicNumwithout validation — critical path input guard required.
resetAttempts(null)createsthis.attempts[null] = [], injecting a null-keyed entry into the internal map.canRetry(undefined)returnstrue(0 < maxRetries), silently enabling retries for a phantom epic.getAttemptCountpropagates the same flaw.🛡️ Proposed fix — add a shared validator
+ /** + * Validates that epicNum is a non-negative integer. + * `@param` {number} epicNum + * `@throws` {TypeError} + * `@private` + */ + _assertValidEpicNum(epicNum) { + if (typeof epicNum !== 'number' || !Number.isInteger(epicNum) || epicNum < 0) { + throw new TypeError(`epicNum must be a non-negative integer, got: ${JSON.stringify(epicNum)}`); + } + } getAttemptCount(epicNum) { + this._assertValidEpicNum(epicNum); return (this.attempts[epicNum] || []).length; } canRetry(epicNum) { + this._assertValidEpicNum(epicNum); return this.getAttemptCount(epicNum) < this.maxRetries; } resetAttempts(epicNum) { + this._assertValidEpicNum(epicNum); this.attempts[epicNum] = []; this._log(`Reset attempts for Epic ${epicNum}`, 'info'); }As per coding guidelines: "Check for proper input validation on public API methods."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 699 - 735, Public methods getAttemptCount, canRetry, and resetAttempts accept epicNum without validation, allowing null/undefined to create invalid keys; add a shared validator method (e.g., _validateEpicNum(epicNum)) that asserts epicNum is a finite non-negative integer (or the project-required integer range) and throws a TypeError for invalid input, then call this validator at the start of getAttemptCount, canRetry, and resetAttempts to prevent creating null/undefined keys and to fail-fast on bad callers.
687-696: ⚡ Quick win
getAttemptHistory()shallow copy exposes mutable internal arrays — callers can silently corrupt recovery state.
{ ...this.attempts }creates a new outer object, but each value is the same array reference asthis.attempts[epicNum]. Any caller that pushes to or splices a returned array will corrupt the handler's internal attempt records:const h = handler.getAttemptHistory(); h[4].push({ fake: true }); // mutates handler.attempts[4] directlyThis is a state-corruption risk in a critical orchestration module. Each array should also be copied:
🛡️ Proposed fix
getAttemptHistory() { - return { ...this.attempts }; + const copy = {}; + for (const [epicNum, attempts] of Object.entries(this.attempts)) { + copy[epicNum] = [...attempts]; + } + return copy; }As per coding guidelines: "Check session-state.js for thread-safety and state corruption prevention" (same principle applies to recovery-handler state).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 687 - 696, getAttemptHistory() currently returns a shallow copy of this.attempts so the outer object is new but the inner arrays are the same references; update getAttemptHistory to return a new map where each epic's array is cloned (e.g., copy each this.attempts[epicNum] into a new array) so callers cannot mutate the handler's internal arrays; locate getAttemptHistory and replace the spread-return with logic that builds a new object (or uses Object.entries/Object.fromEntries) mapping each key to a copied array (e.g., array.slice() or equivalent) while preserving the original keys and values order..aiox-core/core/orchestration/master-orchestrator.js (1)
937-945: ⚡ Quick win
startDashboard()should not let dashboard initialization errors propagate unhandled.The new implementation calls
dashboardIntegration.start()with no error guard. If the dashboard integration fails to initialize (e.g., port conflict, write permission, or serialization error), the error propagates to the caller with no context. The dashboard is a non-critical observability subsystem and consistent with how other non-critical paths are handled in this class (e.g.,_saveStatecatches and logs,_evaluateGateswallows on non-strict mode).🛡️ Proposed fix
async startDashboard() { - await this.dashboardIntegration.start(); + try { + await this.dashboardIntegration.start(); + } catch (error) { + this._log(`Dashboard start failed (non-blocking): ${error.message}`, { level: 'warn' }); + } }As per coding guidelines: "Verify error handling is comprehensive with proper try/catch and error context."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/master-orchestrator.js around lines 937 - 945, startDashboard currently awaits this.dashboardIntegration.start() with no error handling, so any dashboard init failure will propagate; wrap the call to this.dashboardIntegration.start() in a try/catch inside startDashboard, catch any error, log a descriptive non-fatal error via the existing logger (include the caught error details and context like "dashboard start failed"), and do not rethrow so the dashboard remains non-critical; ensure you reference startDashboard and dashboardIntegration.start in your change and keep behavior consistent with other non-critical handlers (e.g., _saveState)..aiox-core/core/config/config-loader.js (1)
295-309: ⚡ Quick winLGTM —
getConfigSectionis clean and correctly documented.The implementation correctly delegates to
loadConfigSectionsand returns only the requested key, inheriting all existing cache/TTL behavior. The JSDoc@exampleaids discoverability in this deprecated module.One minor note: the module-level migration guide (Lines 6–10) doesn't mention a replacement for
getConfigSection. Users who adopt this new function won't have a migration path when the module is removed in v4.0.0.💡 Suggested migration guide amendment
* Migration guide: * - Config resolution: const { resolveConfig } = require('./config-resolver'); * const config = await resolveConfig(projectRoot); * - Agent config: const { AgentConfigLoader } = require('./agent-config-loader'); * const loader = new AgentConfigLoader(agentId); * const config = await loader.load(coreConfig); +* - Single section: const { resolveConfig } = require('./config-resolver'); +* const result = await resolveConfig(projectRoot); +* const section = result?.config?.['sectionName'];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/config/config-loader.js around lines 295 - 309, Update the module-level migration guide to mention the deprecated getConfigSection function and show its replacement: call loadConfigSections([sectionName]) and return the desired key (e.g., const cfg = await loadConfigSections([name]); return cfg[name]); and include a brief example and note that getConfigSection will be removed in v4.0.0 so callers should migrate to loadConfigSections (or the new config API if one exists).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.aiox-core/core/config/config-loader.js:
- Around line 295-309: Update the module-level migration guide to mention the
deprecated getConfigSection function and show its replacement: call
loadConfigSections([sectionName]) and return the desired key (e.g., const cfg =
await loadConfigSections([name]); return cfg[name]); and include a brief example
and note that getConfigSection will be removed in v4.0.0 so callers should
migrate to loadConfigSections (or the new config API if one exists).
In @.aiox-core/core/orchestration/master-orchestrator.js:
- Around line 937-945: startDashboard currently awaits
this.dashboardIntegration.start() with no error handling, so any dashboard init
failure will propagate; wrap the call to this.dashboardIntegration.start() in a
try/catch inside startDashboard, catch any error, log a descriptive non-fatal
error via the existing logger (include the caught error details and context like
"dashboard start failed"), and do not rethrow so the dashboard remains
non-critical; ensure you reference startDashboard and dashboardIntegration.start
in your change and keep behavior consistent with other non-critical handlers
(e.g., _saveState).
In @.aiox-core/core/orchestration/recovery-handler.js:
- Around line 699-735: Public methods getAttemptCount, canRetry, and
resetAttempts accept epicNum without validation, allowing null/undefined to
create invalid keys; add a shared validator method (e.g.,
_validateEpicNum(epicNum)) that asserts epicNum is a finite non-negative integer
(or the project-required integer range) and throws a TypeError for invalid
input, then call this validator at the start of getAttemptCount, canRetry, and
resetAttempts to prevent creating null/undefined keys and to fail-fast on bad
callers.
- Around line 687-696: getAttemptHistory() currently returns a shallow copy of
this.attempts so the outer object is new but the inner arrays are the same
references; update getAttemptHistory to return a new map where each epic's array
is cloned (e.g., copy each this.attempts[epicNum] into a new array) so callers
cannot mutate the handler's internal arrays; locate getAttemptHistory and
replace the spread-return with logic that builds a new object (or uses
Object.entries/Object.fromEntries) mapping each key to a copied array (e.g.,
array.slice() or equivalent) while preserving the original keys and values
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a82de67d-c809-4513-8664-566e93b1d62f
📒 Files selected for processing (12)
.aiox-core/core/config/config-loader.js.aiox-core/core/events/dashboard-emitter.js.aiox-core/core/orchestration/bob-orchestrator.js.aiox-core/core/orchestration/brownfield-handler.js.aiox-core/core/orchestration/data-lifecycle-manager.js.aiox-core/core/orchestration/gate-evaluator.js.aiox-core/core/orchestration/greenfield-handler.js.aiox-core/core/orchestration/lock-manager.js.aiox-core/core/orchestration/master-orchestrator.js.aiox-core/core/orchestration/recovery-handler.js.aiox-core/core/orchestration/session-state.js.aiox-core/install-manifest.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/core/orchestration/recovery-handler.js (1)
167-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
handleEpicFailureskips the validation added by this PR.Every other public epic-number API now calls
_assertValidEpicNum, buthandleEpicFailure— the primary mutation entry point — does not. Passing a non-integer (e.g.null,"3",1.5) silently creates a spurious key inthis.attemptsand produces misleading log messages, making subsequent calls togetAttemptCount/canRetryreturn incorrect results for the intended epic.🛡️ Proposed fix
async handleEpicFailure(epicNum, error, context = {}) { + this._assertValidEpicNum(epicNum); const errorMessage = error instanceof Error ? error.message : String(error);As per coding guidelines, "Check for proper input validation on public API methods" is required for
.aiox-core/core/**.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 167 - 169, handleEpicFailure currently omits the input validation used elsewhere and can accept non-integer epic identifiers, creating spurious keys in this.attempts; fix it by calling the existing _assertValidEpicNum(epicNum) at the start of handleEpicFailure (before any mutation or logging) so invalid values (null, "3", 1.5, etc.) are rejected consistently with other public epic-number APIs like getAttemptCount and canRetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/core/orchestration/master-orchestrator.js:
- Around line 944-948: The current startDashboard() swallows errors by logging
and not signaling failure; change it to preserve non-blocking behavior but
surface outcome by returning a success flag: call await
this.dashboardIntegration.start() inside startDashboard(), on success return
true, and in the catch block use this._log(`Dashboard start failed
(non-blocking): ${error.message}`, { level: 'warn' }) and then return false (do
not rethrow). Update any callers expecting a Promise<void> to handle the
Promise<boolean> result from startDashboard() so they can detect failure.
---
Outside diff comments:
In @.aiox-core/core/orchestration/recovery-handler.js:
- Around line 167-169: handleEpicFailure currently omits the input validation
used elsewhere and can accept non-integer epic identifiers, creating spurious
keys in this.attempts; fix it by calling the existing
_assertValidEpicNum(epicNum) at the start of handleEpicFailure (before any
mutation or logging) so invalid values (null, "3", 1.5, etc.) are rejected
consistently with other public epic-number APIs like getAttemptCount and
canRetry.
🪄 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: 0c2f7540-3be2-4037-8ca2-515ecc6d1227
📒 Files selected for processing (6)
.aiox-core/core/config/config-loader.js.aiox-core/core/orchestration/master-orchestrator.js.aiox-core/core/orchestration/recovery-handler.js.aiox-core/install-manifest.yamltests/core/master-orchestrator.test.jstests/core/recovery-handler.test.js
✅ Files skipped from review due to trivial changes (2)
- tests/core/master-orchestrator.test.js
- .aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .aiox-core/core/config/config-loader.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/core/orchestration/recovery-handler.js:
- Around line 762-765: The TypeError message in _assertValidEpicNum uses
JSON.stringify(epicNum) which can itself throw for BigInt; change the thrown
message to use String(epicNum) instead (e.g., replace JSON.stringify(epicNum)
with String(epicNum)) so the validation error always formats the epicNum safely
when throwing from _assertValidEpicNum.
🪄 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: 476ce756-51c2-4f5b-a4dd-1c18c5129b06
📒 Files selected for processing (3)
.aiox-core/core/orchestration/recovery-handler.js.aiox-core/install-manifest.yamltests/core/recovery-handler.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/core/orchestration/recovery-handler.js:
- Around line 693-707: The JSDoc for getAttemptHistory incorrectly states keys
are numbers; JavaScript object keys are strings, so update the JSDoc for
getAttemptHistory to reflect that it returns an Object<string, Array> (or add a
note that epic numbers are stringified), and ensure callers know to access
entries with string keys or convert numeric indices; reference
getAttemptHistory, this.attempts and _cloneAttempt when making the doc/type
change.
- Around line 775-781: The JSON fallback in _cloneAttempt can throw or lose data
for non-serializable fields (Error, undefined, Map/Set, circular refs, BigInt);
update _cloneAttempt to wrap the JSON.parse(JSON.stringify(attempt)) fallback in
a try/catch and, on any error or detected loss, return a safe sanitized clone
instead: create a shallow copy of attempt but replace the context with a
safeRepresentation (if context is an Error, replace with {name,message,stack};
if Map/Set convert to arrays; if BigInt convert to string; preserve
undefined-valued keys explicitly; and for circulars/other failures fall back to
stringified summary), ensuring _cloneAttempt never throws and getAttemptHistory
callers receive a stable, non-throwing data-only object.
🪄 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: 1315e4bf-22b6-43ae-9612-daa639400ffd
📒 Files selected for processing (3)
.aiox-core/core/orchestration/recovery-handler.js.aiox-core/install-manifest.yamltests/core/recovery-handler.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/recovery-handler.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.aiox-core/core/orchestration/recovery-handler.js (4)
723-737: 💤 Low value
canRetryvalidation is transitive — fine, but worth a one-liner note.
canRetrydoesn’t call_assertValidEpicNumdirectly; it relies ongetAttemptCountto throw. That’s correct today, but it means anyone refactoringgetAttemptCount(e.g., to return0for unknown epics without validating) could silently weakencanRetry’s contract. A directthis._assertValidEpicNum(epicNum)call here would make the throw contract documented in the JSDoc self-evident from the body.♻️ Optional defensive tweak
canRetry(epicNum) { + this._assertValidEpicNum(epicNum); return this.getAttemptCount(epicNum) < this.maxRetries; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 723 - 737, canRetry currently relies on getAttemptCount to validate epicNum transitively; add an explicit validation call to make the throw contract self-evident and robust by invoking this._assertValidEpicNum(epicNum) at the start of canRetry (before calling this.getAttemptCount(epicNum)) so invalid epic numbers are rejected here even if getAttemptCount is later changed.
799-840: ⚖️ Poor tradeoff
_requiresSanitizedClonetraversal looks correct; one observation about reuse.The traversal correctly handles circular refs via a per-call
WeakSetand the add/process/delete pattern means siblings don’t false-positive each other. One thing to note: when a non-JSON-safe value is detected, you’ll then call_sanitizeValuewhich independently re-walks the same structure with its ownWeakSet. For attempt records (typically small) this is fine, but ifcontextever grows large, the double-walk could become a hot spot. Not actionable now — flagging for future awareness if attempts/context ever balloon.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 799 - 840, The traversal in _requiresSanitizedClone is correct but causes a duplicate walk because _sanitizeValue re-traverses the same object with its own WeakSet; to avoid double work for large contexts, add an optional seen/visited WeakSet parameter to _sanitizeValue (and any helper it calls) so you can pass the same WeakSet returned or used by _requiresSanitizedClone instead of creating a fresh one, update calls to _sanitizeValue to pass the existing seen set (falling back to new WeakSet() when not provided), and ensure both methods continue to add/remove or otherwise manage the shared set consistently to preserve circular-detection semantics.
692-708: 💤 Low valueBehavior change worth surfacing in the JSDoc: deep clone vs. prior shallow copy.
The previous return shape exposed live attempt object references; this version returns deep clones, so callers that previously mutated entries to drive internal state (or relied on reference equality) will silently see different behavior. The JSDoc mentions stringified keys but not the reference-equality break. A short note would help downstream consumers audit their usage.
📝 Suggested doc tweak
* Gets attempt history for all epics. * * Returns cloned attempt arrays to prevent callers from mutating internal * recovery state. Epic number keys are stringified because JavaScript object - * keys are strings. + * keys are strings. Note: returned attempt records are deep clones, so + * reference equality with internal state is intentionally not preserved. * * `@returns` {Object<string, Array>} Map of stringified epic numbers to attempt records.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 692 - 708, Update the JSDoc for getAttemptHistory to explicitly state that it now returns deep-cloned attempt objects (via this._cloneAttempt) rather than the previous live references/shallow copies, and call out that callers who mutated returned attempts or relied on reference equality must update their usage; reference the getAttemptHistory method and the _cloneAttempt helper so reviewers can locate the change.
779-797: ⚡ Quick winOutput shape of
getAttemptHistory()is runtime-dependent — consider documenting or normalizing.
_cloneAttemptreturns three structurally different shapes depending on the input and runtime:
structuredClonepath (Node ≥ 17, JSON-incompatible values): preservesError,Map,Set, andBigIntinstances.- JSON path: returns plain JSON-cloned data.
- Sanitize path: converts
Error→{name, message, stack}plain object,Map→Array<[k,v]>,Set→Array,BigInt→string, etc.Consumers doing
attempt.context.someErr instanceof Errororattempt.context.metrics instanceof Mapwill see the check pass under structuredClone but fail under the sanitize fallback. Since this code path is only exercised whenstructuredClonethrows or is unavailable, drift can hide in production until a non-cloneable value first appears. Two practical options:
- Document the divergence on
getAttemptHistory/_cloneAttemptso callers know to treat the output as data-only and avoidinstanceofchecks.- Always run the result through
_sanitizeValue(or always re-shape Errors/Maps/Sets) so the output shape is deterministic regardless of Node version.Option 1 is the lower-cost fix and is consistent with the “returns cloned attempt arrays … data-only” intent already in the doc.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.aiox-core/core/orchestration/recovery-handler.js around lines 779 - 797, _cloneAttempt currently returns different shapes depending on runtime which breaks consumers that expect deterministic types; fix it by normalizing output: after the structuredClone or JSON.parse(JSON.stringify(...)) path (in _cloneAttempt) pass the cloned result through this._sanitizeValue before returning so Errors/Maps/Sets/BigInts are consistently converted, ensuring getAttemptHistory always returns the same data-only shape regardless of Node version; keep the fallback sanitize path as-is for when cloning fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.aiox-core/core/orchestration/recovery-handler.js:
- Around line 723-737: canRetry currently relies on getAttemptCount to validate
epicNum transitively; add an explicit validation call to make the throw contract
self-evident and robust by invoking this._assertValidEpicNum(epicNum) at the
start of canRetry (before calling this.getAttemptCount(epicNum)) so invalid epic
numbers are rejected here even if getAttemptCount is later changed.
- Around line 799-840: The traversal in _requiresSanitizedClone is correct but
causes a duplicate walk because _sanitizeValue re-traverses the same object with
its own WeakSet; to avoid double work for large contexts, add an optional
seen/visited WeakSet parameter to _sanitizeValue (and any helper it calls) so
you can pass the same WeakSet returned or used by _requiresSanitizedClone
instead of creating a fresh one, update calls to _sanitizeValue to pass the
existing seen set (falling back to new WeakSet() when not provided), and ensure
both methods continue to add/remove or otherwise manage the shared set
consistently to preserve circular-detection semantics.
- Around line 692-708: Update the JSDoc for getAttemptHistory to explicitly
state that it now returns deep-cloned attempt objects (via this._cloneAttempt)
rather than the previous live references/shallow copies, and call out that
callers who mutated returned attempts or relied on reference equality must
update their usage; reference the getAttemptHistory method and the _cloneAttempt
helper so reviewers can locate the change.
- Around line 779-797: _cloneAttempt currently returns different shapes
depending on runtime which breaks consumers that expect deterministic types; fix
it by normalizing output: after the structuredClone or
JSON.parse(JSON.stringify(...)) path (in _cloneAttempt) pass the cloned result
through this._sanitizeValue before returning so Errors/Maps/Sets/BigInts are
consistently converted, ensuring getAttemptHistory always returns the same
data-only shape regardless of Node version; keep the fallback sanitize path
as-is for when cloning fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 484905ba-74b4-403c-bf8d-4fa49b330fea
📒 Files selected for processing (3)
.aiox-core/core/orchestration/recovery-handler.js.aiox-core/install-manifest.yamltests/core/recovery-handler.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/recovery-handler.test.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.aiox-core/core/orchestration/recovery-handler.js:
- Around line 819-825: The current _sanitizeValue function strips Error
instances to only {name, message, stack}; update _sanitizeValue to preserve
enumerable own-properties (e.g., code, errno, syscall, path, status) by copying
them onto the sanitized object and also detect and recursively sanitize an
error.cause (using the same _sanitizeValue) so nested causes are preserved;
ensure you only copy own-properties (Object.getOwnPropertyNames/Object.keys) and
avoid circular references (track seen objects) so getAttemptHistory() returns
richer error diagnostics without throwing.
🪄 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: 1db81fa3-0a2a-43dc-9284-f6269dd430ba
📒 Files selected for processing (3)
.aiox-core/core/orchestration/recovery-handler.js.aiox-core/install-manifest.yamltests/core/recovery-handler.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…verage-20260507 # Conflicts: # .aiox-core/install-manifest.yaml
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
Status de triagem em 2026-05-07: checks requeridos verdes e CodeRabbit aprovado. O PR está tecnicamente mergeable, mas segue bloqueado por CODEOWNER/human review ( |
1 participant
Summary
Validation
Closes #89
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores