fix: resolve 4 resilience subsystem bugs (ideation, gotchas, condition, circuit-breaker)#584
Open
oalanicolas wants to merge 5 commits intomainfrom
Open
fix: resolve 4 resilience subsystem bugs (ideation, gotchas, condition, circuit-breaker)#584oalanicolas wants to merge 5 commits intomainfrom
oalanicolas wants to merge 5 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
area: agents
WalkthroughAdjusts multiple core modules: fixes named-import in ideation engine and its gotchas call, converts gotchas tracking to a sliding time-window, prevents circuit-breaker OPEN state from resetting recovery timeout, adds runtime override APIs and fail-safe defaults in condition evaluator, and updates install manifest metadata. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/core/orchestration/condition-evaluator.js (1)
109-110:⚠️ Potential issue | 🟠 MajorNegated unknown conditions still fail open.
At Lines 109-110,
!conditionnegates the recursive result, so an unknown condition now becomes!falseinstead of staying blocked.!typo_conditionwill therefore execute even though the inner condition is unrecognized, which misses the fail-safe goal for typoed conditions.Also applies to: 148-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/orchestration/condition-evaluator.js around lines 109 - 110, The negation branch in the evaluator currently negates whatever this.evaluate(...) returns, turning unknown/blocked results into true; update the '!' handling in the evaluate method (the condition.startsWith('!') branch) to first call this.evaluate on condition.substring(1).trim(), check for an unknown/blocked sentinel (e.g., null or undefined or whatever the evaluator uses to signal "unknown") and if it is unknown return that sentinel unchanged; only if the inner result is a definite boolean return its negation. Apply the same defensive pattern to the other negation occurrence referenced (the block around lines 148-150) so negated unknown conditions remain blocked rather than flipping open.
🤖 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/core/ideation/ideation-engine.js:
- Line 16: The code calls this.gotchasMemory.getAll() but GotchasMemory only
exposes listGotchas(), and the resulting TypeError is swallowed by an empty
catch; replace the call to this.gotchasMemory.getAll() with
this.gotchasMemory.listGotchas() (or add a getAll alias in GotchasMemory that
forwards to listGotchas) and improve the empty catch around the filtering logic
in the ideation engine by logging the error with context (or rethrow) so
failures aren’t silently ignored; reference GotchasMemory, listGotchas, and the
site that currently calls this.gotchasMemory.getAll() when making the change.
In @.aiox-core/core/memory/gotchas-memory.js:
- Around line 262-267: The current gap-based reset (checking if now -
tracking.lastSeen > this.options.errorWindowMs) allows stale occurrences to
accumulate; instead prune old samples by filtering tracking.samples (or a
timestamps array you maintain) to only those with timestamp >= now -
this.options.errorWindowMs, then recompute tracking.count and set
tracking.firstSeen to the earliest remaining timestamp (or now if empty); update
tracking.lastSeen as appropriate and use this pruning in the branch around the
code handling stale errors (the block referencing tracking.lastSeen,
tracking.count, tracking.firstSeen, tracking.samples and the later similar block
at lines ~270-272) so the errorWindowMs truly bounds counted occurrences rather
than relying on inactivity gaps.
---
Outside diff comments:
In @.aiox-core/core/orchestration/condition-evaluator.js:
- Around line 109-110: The negation branch in the evaluator currently negates
whatever this.evaluate(...) returns, turning unknown/blocked results into true;
update the '!' handling in the evaluate method (the condition.startsWith('!')
branch) to first call this.evaluate on condition.substring(1).trim(), check for
an unknown/blocked sentinel (e.g., null or undefined or whatever the evaluator
uses to signal "unknown") and if it is unknown return that sentinel unchanged;
only if the inner result is a definite boolean return its negation. Apply the
same defensive pattern to the other negation occurrence referenced (the block
around lines 148-150) so negated unknown conditions remain blocked rather than
flipping open.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49493c73-d40c-401c-88a4-17ca3b7981e7
📒 Files selected for processing (6)
.aiox-core/core/ideation/ideation-engine.js.aiox-core/core/ids/circuit-breaker.js.aiox-core/core/memory/gotchas-memory.js.aiox-core/core/orchestration/condition-evaluator.js.aiox-core/data/entity-registry.yaml.aiox-core/install-manifest.yaml
Contributor
|
@Pedrovaleriolopez, os fixes de ideation e gotchas nesse PR já estão no nosso #521 (aberto 26/fev) e o condition evaluator/circuit-breaker nos nossos PRs anteriores. Temos PRs individuais com testes dedicados pra cada bug — abertos há 2 semanas aguardando review. |
Contributor
|
Hey @oalanicolas — CodeRabbit flagged 3 actionable issues: Actionable:
CI is green. Please apply fixes and push. |
oalanicolas
added a commit
that referenced
this pull request
Mar 11, 2026
- ideation-engine.js: replace getAll() (non-existent) with listGotchas() and improve empty catch to log the error with context so filtering failures are no longer silently ignored - gotchas-memory.js: replace gap-based reset with true sliding window using a timestamps array; prune entries older than errorWindowMs on each trackError() call and recompute count/firstSeen from the pruned window so errorWindowMs genuinely bounds counted occurrences - condition-evaluator.js: register six workflow guidance conditions (user_wants_ai_generation, stories_remaining, epic_complete, architecture_suggests_prd_changes, po_checklist_issues, user_has_generated_ui) that are human-driven and cannot be inferred from TechStackProfile; defaulting to true preserves backwards compatibility with greenfield-ui.yaml after the fail-safe false introduced in #472 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.aiox-core/core/orchestration/condition-evaluator.js (1)
119-121:⚠️ Potential issue | 🟠 MajorNegated unknown conditions still fail open.
!missing_conditionnow evaluates totrue, and the same happens for negated composites like!(project_has_frontend && missing_condition), because the negation path only sees the innerfalseafter the unknown-condition fallback. Please propagate an explicit “unknown” sentinel through recursive evaluation and only negate definite booleans.Also applies to: 159-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/orchestration/condition-evaluator.js around lines 119 - 121, The negation branch in evaluate (the if handling condition.startsWith('!')) currently blindly negates whatever evaluate(...) returns, causing unknown conditions to be treated as true; change it to propagate an explicit "unknown" sentinel from recursive evaluate calls and only negate when the inner result is a definite boolean (true/false). Concretely: have evaluate return a tri-state (true / false / UNKNOWN) and in the '!' branch and the parenthesized/composite handling (the corresponding evaluate code around the other branch at lines ~159-161) check if inner === UNKNOWN and if so return UNKNOWN, otherwise return !inner; update any callers to handle the UNKNOWN sentinel consistently. Use the existing evaluate function name and the negation/parenthesis handling sites to locate the edits..aiox-core/core/memory/gotchas-memory.js (1)
275-295:⚠️ Potential issue | 🟡 MinorWindowed counts still carry stale sample metadata.
tracking.countis pruned toerrorWindowMs, buttracking.samplesis not._autoCaptureGotcha()later usestracking.samplesto populaterelatedFilesandtrigger.files, so a newly captured gotcha can still point at files/contexts from outside the active window. Prunetracking.sampleswith the same cutoff before pushing the new sample.Suggested fix
const windowStart = now - this.options.errorWindowMs; tracking.timestamps.push(now); tracking.timestamps = tracking.timestamps.filter((ts) => ts >= windowStart); + tracking.samples = (tracking.samples || []).filter((sample) => { + const ts = Date.parse(sample.timestamp); + return Number.isFinite(ts) && ts >= windowStart; + }); // Recompute derived fields from the pruned sliding window tracking.count = tracking.timestamps.length; tracking.firstSeen = tracking.timestamps.length > 0 ? tracking.timestamps[0] : now;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/memory/gotchas-memory.js around lines 275 - 295, The tracking.samples array is not being pruned to the same sliding window as tracking.timestamps, so stale sample metadata can be included when _autoCaptureGotcha() builds relatedFiles/trigger.files; update the code in the same block that computes windowStart (and updates tracking.timestamps) to filter tracking.samples to only keep entries with timestamps >= windowStart (parse sample.timestamp or store numeric timestamps consistently), then push the new sample and still enforce the samples cap (e.g., samples.length < 5) — modify the logic around tracking.samples in the function that updates tracking (refer to tracking.timestamps, tracking.samples, windowStart, and _autoCaptureGotcha) so samples reflect the same active window.
🤖 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/core/ideation/ideation-engine.js:
- Line 16: The GotchasMemory instance is being created against process.cwd()
instead of the configured project root; update the IdeationEngine initialization
to instantiate GotchasMemory with this.rootPath (e.g., replace any new
GotchasMemory() or calls that default to process.cwd() with new
GotchasMemory(this.rootPath)) so known-gotcha filtering reads the correct .aiox
store; apply the same change for the other occurrence referenced (around the
GotchasMemory usage at lines ~30).
In @.aiox-core/core/memory/gotchas-memory.js:
- Around line 270-286: Persisted tracking entries that lack tracking.timestamps
should be backfilled from preserved metadata before pruning: if
tracking.timestamps is not an array, initialize it by using existing
tracking.count and tracking.lastSeen (or tracking.firstSeen if lastSeen missing)
to synthesize a timestamps array (e.g., create tracking.count timestamps ending
at tracking.lastSeen spaced backward by small deltas or evenly within
this.options.errorWindowMs), then proceed to push(now), filter by windowStart,
and recompute tracking.count/firstSeen as you already do; update the block
operating on tracking.timestamps, tracking.count, tracking.lastSeen, and
tracking.firstSeen to perform this seeding step when timestamps are absent.
---
Outside diff comments:
In @.aiox-core/core/memory/gotchas-memory.js:
- Around line 275-295: The tracking.samples array is not being pruned to the
same sliding window as tracking.timestamps, so stale sample metadata can be
included when _autoCaptureGotcha() builds relatedFiles/trigger.files; update the
code in the same block that computes windowStart (and updates
tracking.timestamps) to filter tracking.samples to only keep entries with
timestamps >= windowStart (parse sample.timestamp or store numeric timestamps
consistently), then push the new sample and still enforce the samples cap (e.g.,
samples.length < 5) — modify the logic around tracking.samples in the function
that updates tracking (refer to tracking.timestamps, tracking.samples,
windowStart, and _autoCaptureGotcha) so samples reflect the same active window.
In @.aiox-core/core/orchestration/condition-evaluator.js:
- Around line 119-121: The negation branch in evaluate (the if handling
condition.startsWith('!')) currently blindly negates whatever evaluate(...)
returns, causing unknown conditions to be treated as true; change it to
propagate an explicit "unknown" sentinel from recursive evaluate calls and only
negate when the inner result is a definite boolean (true/false). Concretely:
have evaluate return a tri-state (true / false / UNKNOWN) and in the '!' branch
and the parenthesized/composite handling (the corresponding evaluate code around
the other branch at lines ~159-161) check if inner === UNKNOWN and if so return
UNKNOWN, otherwise return !inner; update any callers to handle the UNKNOWN
sentinel consistently. Use the existing evaluate function name and the
negation/parenthesis handling sites to locate the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b052b719-5132-45c2-83ed-de49d3d5463f
📒 Files selected for processing (4)
.aiox-core/core/ideation/ideation-engine.js.aiox-core/core/memory/gotchas-memory.js.aiox-core/core/orchestration/condition-evaluator.js.aiox-core/install-manifest.yaml
| let GotchasMemory; | ||
| try { | ||
| GotchasMemory = require('../memory/gotchas-memory'); | ||
| ({ GotchasMemory } = require('../memory/gotchas-memory')); |
There was a problem hiding this comment.
Instantiate GotchasMemory against the configured project root.
Now that the named export makes this path reachable again, the fallback constructor still uses process.cwd() instead of this.rootPath. When IdeationEngine is run against another workspace via config.rootPath, known-gotcha filtering will read the wrong .aiox store.
Suggested fix
- this.gotchasMemory = config.gotchasMemory || (GotchasMemory ? new GotchasMemory() : null);
+ this.gotchasMemory =
+ config.gotchasMemory || (GotchasMemory ? new GotchasMemory(this.rootPath) : null);Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/ideation/ideation-engine.js at line 16, The GotchasMemory
instance is being created against process.cwd() instead of the configured
project root; update the IdeationEngine initialization to instantiate
GotchasMemory with this.rootPath (e.g., replace any new GotchasMemory() or calls
that default to process.cwd() with new GotchasMemory(this.rootPath)) so
known-gotcha filtering reads the correct .aiox store; apply the same change for
the other occurrence referenced (around the GotchasMemory usage at lines ~30).
Comment on lines
+270
to
+286
| // Ensure timestamps array exists for entries loaded from disk before this fix | ||
| if (!Array.isArray(tracking.timestamps)) { | ||
| tracking.timestamps = []; | ||
| } | ||
|
|
||
| // Add current timestamp and prune occurrences outside the sliding error window. | ||
| // This ensures errorWindowMs truly bounds the counted occurrences rather than | ||
| // relying on inactivity gaps — the previous gap-based reset allowed stale errors | ||
| // (e.g., at 0h, 20h, 40h) to keep accumulating as long as each repeat landed | ||
| // just inside the gap boundary. | ||
| const windowStart = now - this.options.errorWindowMs; | ||
| tracking.timestamps.push(now); | ||
| tracking.timestamps = tracking.timestamps.filter((ts) => ts >= windowStart); | ||
|
|
||
| // Recompute derived fields from the pruned sliding window | ||
| tracking.count = tracking.timestamps.length; | ||
| tracking.firstSeen = tracking.timestamps.length > 0 ? tracking.timestamps[0] : now; |
There was a problem hiding this comment.
Backfill timestamps for pre-upgrade tracking entries.
When a persisted entry created before this change is loaded, tracking.timestamps is initialized to [], so the next occurrence recomputes tracking.count as 1 even if the stored count/lastSeen are still inside the active window. That breaks pending auto-captures across upgrade boundaries. Seed the window from persisted sample timestamps or preserved count metadata before recomputing.
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/memory/gotchas-memory.js around lines 270 - 286, Persisted
tracking entries that lack tracking.timestamps should be backfilled from
preserved metadata before pruning: if tracking.timestamps is not an array,
initialize it by using existing tracking.count and tracking.lastSeen (or
tracking.firstSeen if lastSeen missing) to synthesize a timestamps array (e.g.,
create tracking.count timestamps ending at tracking.lastSeen spaced backward by
small deltas or evenly within this.options.errorWindowMs), then proceed to
push(now), filter by windowStart, and recompute tracking.count/firstSeen as you
already do; update the block operating on tracking.timestamps, tracking.count,
tracking.lastSeen, and tracking.firstSeen to perform this seeding step when
timestamps are absent.
Contributor
Review ⚡ @devopsCode: APPROVED ✅ — Best PR of the batch. All 4 fixes are technically solid:
Blocker: ❌ Merge conflict — please rebase against git fetch origin && git rebase origin/mainOnce rebased, this is ready to merge immediately. |
the GotchasMemory class (named vs default export). Used destructuring. without checking errorWindowMs. Added window expiration check — resets count, firstSeen, and samples when last occurrence exceeds the window. Changed to false (fail-safe) — typos and unknown conditions now block instead of silently authorizing execution. restarting the recovery timeout on every failure. Now only updates lastFailureTime when state is not OPEN, preserving the original recovery window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- ideation-engine.js: replace getAll() (non-existent) with listGotchas() and improve empty catch to log the error with context so filtering failures are no longer silently ignored - gotchas-memory.js: replace gap-based reset with true sliding window using a timestamps array; prune entries older than errorWindowMs on each trackError() call and recompute count/firstSeen from the pruned window so errorWindowMs genuinely bounds counted occurrences - condition-evaluator.js: register six workflow guidance conditions (user_wants_ai_generation, stories_remaining, epic_complete, architecture_suggests_prd_changes, po_checklist_issues, user_has_generated_ui) that are human-driven and cannot be inferred from TechStackProfile; defaulting to true preserves backwards compatibility with greenfield-ui.yaml after the fail-safe false introduced in #472 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
oalanicolas
force-pushed
the
fix/bugfix-517-475-472-469-resilience-cluster
branch
from
460f1f9 to
8807e52
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/core/orchestration/condition-evaluator.js (1)
119-121:⚠️ Potential issue | 🟠 MajorNegated unknown conditions still fail open.
evaluate('!typo')returnstruebecause the negation branch runs before the unknown-condition fallback. Unknown condition names need to stay blocked even when negated, otherwise a typo can still enable a phase.As per coding guidelines,
.aiox-core/core/**:Check for proper input validation on public API methods.Also applies to: 159-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/orchestration/condition-evaluator.js around lines 119 - 121, The negation branch in evaluate allows unknown condition names to pass because it negates before validating existence; update the evaluate method (function evaluate in condition-evaluator.js) so when condition.startsWith('!') you first extract inner = condition.substring(1).trim(), check whether that inner condition is a known/registered condition (use the same existence check used elsewhere in this file), and if the inner condition is unknown return false (do not allow negation to open it), otherwise return !this.evaluate(inner); make the same change to the other negation handling block referenced around the second occurrence (the block at the later lines) so both negated branches validate the inner condition before negating.
♻️ Duplicate comments (1)
.aiox-core/core/memory/gotchas-memory.js (1)
270-286:⚠️ Potential issue | 🟠 MajorBackfill legacy tracking state before recomputing the window.
If a persisted tracking entry has recent
count/lastSeendata but notimestamps, initializingtracking.timestampsto[]makes the nexttrackError()recompute the windowed count as1. That can defer or miss auto-capture immediately after upgrading with existing.aiox/error-tracking.jsondata.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/memory/gotchas-memory.js around lines 270 - 286, Legacy persisted tracking entries that have a nonzero tracking.count and tracking.lastSeen but no tracking.timestamps get reset to a single new timestamp, causing the windowed count to drop to 1; in trackError() (where tracking.timestamps is initialized) detect when tracking.timestamps is missing/empty and tracking.count > 0 and tracking.lastSeen is present and backfill tracking.timestamps to reflect the historical occurrences (e.g., include tracking.lastSeen and additional inferred earlier timestamps so the reconstructed array length matches tracking.count, then proceed with pushing now and filtering by windowStart), and finally recompute tracking.count and tracking.firstSeen from the pruned timestamps so auto-capture behavior remains correct after upgrades.
🤖 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/core/orchestration/condition-evaluator.js:
- Around line 101-110: The hard-coded true guards in ConditionEvaluator (the
keys stories_remaining, user_wants_ai_generation,
architecture_suggests_prd_changes, po_checklist_issues, user_has_generated_ui)
must be replaced with runtime lookups so they act as true guards only when no
runtime hook is provided: change each arrow ()=> true to a function that calls
the evaluator's runtime resolution API (e.g., this.getCondition?
this.evaluateCondition? this.runtime.getFlag — pick the existing method on
ConditionEvaluator to resolve a condition by name) and only fall back to true if
that runtime lookup returns undefined; update or add small shim methods on
ConditionEvaluator to expose a named lookup (e.g., resolveCondition(name)) and
use that from the condition map so optional workflow steps and the
stories_remaining loop can be gated correctly.
In @.aiox-core/install-manifest.yaml:
- Around line 243-246: The manifest contains stale hashes for
core/config/config-loader.js and core/execution/build-state-manager.js which
causes scripts/validate-manifest.js to fail; fix it by checking out a clean tree
(no local changes), regenerating the install-manifest.yaml using the
repository's manifest generation tool (or the same mechanism that produced the
original file) so the hashes for core/config/config-loader.js and
core/execution/build-state-manager.js are recomputed, and then commit the
updated manifest; ensure you run the same validation step
(scripts/validate-manifest.js) after regeneration to confirm no mismatches
remain.
---
Outside diff comments:
In @.aiox-core/core/orchestration/condition-evaluator.js:
- Around line 119-121: The negation branch in evaluate allows unknown condition
names to pass because it negates before validating existence; update the
evaluate method (function evaluate in condition-evaluator.js) so when
condition.startsWith('!') you first extract inner =
condition.substring(1).trim(), check whether that inner condition is a
known/registered condition (use the same existence check used elsewhere in this
file), and if the inner condition is unknown return false (do not allow negation
to open it), otherwise return !this.evaluate(inner); make the same change to the
other negation handling block referenced around the second occurrence (the block
at the later lines) so both negated branches validate the inner condition before
negating.
---
Duplicate comments:
In @.aiox-core/core/memory/gotchas-memory.js:
- Around line 270-286: Legacy persisted tracking entries that have a nonzero
tracking.count and tracking.lastSeen but no tracking.timestamps get reset to a
single new timestamp, causing the windowed count to drop to 1; in trackError()
(where tracking.timestamps is initialized) detect when tracking.timestamps is
missing/empty and tracking.count > 0 and tracking.lastSeen is present and
backfill tracking.timestamps to reflect the historical occurrences (e.g.,
include tracking.lastSeen and additional inferred earlier timestamps so the
reconstructed array length matches tracking.count, then proceed with pushing now
and filtering by windowStart), and finally recompute tracking.count and
tracking.firstSeen from the pruned timestamps so auto-capture behavior remains
correct after upgrades.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7c98a45-d13b-4546-b0cb-b59b46def759
📒 Files selected for processing (6)
.aiox-core/core/ideation/ideation-engine.js.aiox-core/core/ids/circuit-breaker.js.aiox-core/core/memory/gotchas-memory.js.aiox-core/core/orchestration/condition-evaluator.js.aiox-core/data/entity-registry.yaml.aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .aiox-core/core/ideation/ideation-engine.js
Clean manifest regeneration after rebase. Add runtime override support for workflow guidance conditions instead of hard-coding true. - Add _runtimeOverrides map to ConditionEvaluator constructor - Add setRuntimeCondition(name, value) and setRuntimeConditions(overrides) public API so workflow engines can resolve human-intent conditions at runtime (stories_remaining, epic_complete, user_wants_ai_generation, architecture_suggests_prd_changes, po_checklist_issues, user_has_generated_ui) - Each condition checks _runtimeOverrides first; falls back to true when no override is registered (preserves backwards-compatible permissive default for unmanaged executions) - Add skip explanations for all 6 runtime-resolved conditions in getSkipExplanation() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/core/orchestration/condition-evaluator.js (1)
171-174:⚠️ Potential issue | 🟠 MajorNegated unknown conditions fail open, undermining the fail-safe change.
The negation logic at line 173 inverts the result of evaluating the inner condition. If
!typo_conditionis evaluated:
typo_conditionis unknown → returnsfalse(line 213)- Negation inverts it →
!false=trueThis allows execution on typos like
!user_wantsAI_generation, defeating the fail-safe intent of PR#472.🐛 Proposed fix: detect unknown before negation
// Handle negation first if (condition.startsWith('!')) { - return !this.evaluate(condition.substring(1).trim()); + const innerCondition = condition.substring(1).trim(); + // Check if inner condition is known before negating + const innerResult = this.evaluate(innerCondition); + // If inner condition logged "Unknown condition", the negation should also fail-safe. + // We need a way to distinguish "known false" from "unknown". + // Option: use a sentinel or check if innerCondition exists in evaluators/overrides. + const isKnown = this._isKnownCondition(innerCondition); + if (!isKnown) { + console.warn(`[ConditionEvaluator] Negation of unknown condition: ${condition}`); + return false; // fail-safe + } + return !innerResult; }Add helper method:
/** * Check if a condition is known (exists in evaluators or runtime overrides) * `@private` * `@param` {string} condition * `@returns` {boolean} */ _isKnownCondition(condition) { // Check runtime overrides if (this._runtimeOverrides.hasOwnProperty(condition)) { return true; } // Check if it's a built-in evaluator key (would need to extract evaluators or maintain a set) // For simplicity, evaluate and check if warning was logged - or maintain known keys set const knownConditions = new Set([ 'project_has_database', 'project_has_frontend', 'project_has_backend', 'project_has_typescript', 'project_has_tests', 'supabase_configured', 'database_has_rls', 'database_has_migrations', 'frontend_has_react', 'frontend_has_vue', 'frontend_has_tailwind', 'qa_review_approved', 'phase_2_completed', 'phase_3_completed', 'all_collection_phases_complete', 'has_any_data_to_analyze', 'user_wants_ai_generation', 'stories_remaining', 'epic_complete', 'architecture_suggests_prd_changes', 'po_checklist_issues', 'user_has_generated_ui' ]); return knownConditions.has(condition) || condition.includes('.') || condition.includes('&&') || condition.includes('||'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/orchestration/condition-evaluator.js around lines 171 - 174, Negation currently inverts unknown conditions (making typos evaluate true); add a private helper _isKnownCondition(condition) that checks runtime overrides and known evaluator keys (or a maintained set of known condition strings) and then change the negation branch in evaluate() to first compute inner = condition.substring(1).trim() and if !_isKnownCondition(inner) return false (or propagate "unknown"/safe-fail) instead of inverting — only call and invert this.evaluate(inner) when _isKnownCondition(inner) is true; reference the evaluate method, the negation branch, and the new _isKnownCondition helper to locate changes.
🧹 Nitpick comments (1)
.aiox-core/core/orchestration/condition-evaluator.js (1)
139-162: Consider extracting a helper for runtime condition lookups.The repeated pattern
this._runtimeOverrides[X] !== undefined ? this._runtimeOverrides[X] : truecould be simplified with a helper method. This is optional but improves maintainability if more runtime conditions are added.♻️ Optional DRY refactor
Add a helper method:
/** * Get runtime condition value with permissive default * `@private` * `@param` {string} name - Condition name * `@param` {boolean} [defaultValue=true] - Value when not overridden * `@returns` {boolean} */ _getRuntimeCondition(name, defaultValue = true) { return this._runtimeOverrides[name] !== undefined ? this._runtimeOverrides[name] : defaultValue; }Then simplify the evaluators:
- user_wants_ai_generation: () => - this._runtimeOverrides['user_wants_ai_generation'] !== undefined - ? this._runtimeOverrides['user_wants_ai_generation'] - : true, + user_wants_ai_generation: () => this._getRuntimeCondition('user_wants_ai_generation'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/orchestration/condition-evaluator.js around lines 139 - 162, Extract the repeated ternary into a private helper (e.g., _getRuntimeCondition(name, defaultValue = true)) that returns this._runtimeOverrides[name] when defined otherwise defaultValue, then update each evaluator (user_wants_ai_generation, stories_remaining, epic_complete, architecture_suggests_prd_changes, po_checklist_issues, user_has_generated_ui) to call _getRuntimeCondition('condition_name') instead of repeating the ternary; ensure the helper lives on the same class/object so references to this._runtimeOverrides remain valid.
🤖 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/core/orchestration/condition-evaluator.js:
- Around line 73-85: Validate inputs for the public API: in
setRuntimeCondition(name, value) ensure name is a non-empty string (throw a
TypeError if not) before assigning this._runtimeOverrides[name] =
Boolean(value); in setRuntimeConditions(overrides) ensure overrides is a
non-null plain object (throw a TypeError if it's null/undefined or not an
object), then iterate Object.entries(overrides) and validate each key is a
non-empty string before setting this._runtimeOverrides[name] = Boolean(value);
keep using the same _runtimeOverrides storage and preserve current Boolean
coercion for values.
- Around line 143-146: The orchestrator currently never invokes
setRuntimeConditions, so the condition function stories_remaining in
condition-evaluator.js always returns the default true; update the orchestrator
(where it detects the story backlog exhaustion) to call setRuntimeConditions({
stories_remaining: false }) so loop guards can terminate; likewise set other
documented termination flags (e.g., epic_complete) via setRuntimeConditions when
their corresponding orchestrator state changes so condition-evaluator's runtime
overrides are honored.
---
Outside diff comments:
In @.aiox-core/core/orchestration/condition-evaluator.js:
- Around line 171-174: Negation currently inverts unknown conditions (making
typos evaluate true); add a private helper _isKnownCondition(condition) that
checks runtime overrides and known evaluator keys (or a maintained set of known
condition strings) and then change the negation branch in evaluate() to first
compute inner = condition.substring(1).trim() and if !_isKnownCondition(inner)
return false (or propagate "unknown"/safe-fail) instead of inverting — only call
and invert this.evaluate(inner) when _isKnownCondition(inner) is true; reference
the evaluate method, the negation branch, and the new _isKnownCondition helper
to locate changes.
---
Nitpick comments:
In @.aiox-core/core/orchestration/condition-evaluator.js:
- Around line 139-162: Extract the repeated ternary into a private helper (e.g.,
_getRuntimeCondition(name, defaultValue = true)) that returns
this._runtimeOverrides[name] when defined otherwise defaultValue, then update
each evaluator (user_wants_ai_generation, stories_remaining, epic_complete,
architecture_suggests_prd_changes, po_checklist_issues, user_has_generated_ui)
to call _getRuntimeCondition('condition_name') instead of repeating the ternary;
ensure the helper lives on the same class/object so references to
this._runtimeOverrides remain valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5171a63-9bad-4f92-942f-67ccb65b3a60
📒 Files selected for processing (2)
.aiox-core/core/orchestration/condition-evaluator.js.aiox-core/install-manifest.yaml
Comment on lines
+73
to
+85
| setRuntimeCondition(name, value) { | ||
| this._runtimeOverrides[name] = Boolean(value); | ||
| } | ||
|
|
||
| /** | ||
| * Set multiple runtime conditions at once. | ||
| * @param {Object.<string, boolean>} overrides - Map of condition name to value | ||
| */ | ||
| setRuntimeConditions(overrides) { | ||
| for (const [name, value] of Object.entries(overrides)) { | ||
| this._runtimeOverrides[name] = Boolean(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation on public API methods.
Per coding guidelines for core modules, public APIs should validate inputs. Currently:
setRuntimeConditiondoesn't validate thatnameis a non-empty string.setRuntimeConditionswill throw ifoverridesisnull/undefined.
🛡️ Proposed defensive checks
setRuntimeCondition(name, value) {
+ if (typeof name !== 'string' || !name.trim()) {
+ console.warn('[ConditionEvaluator] setRuntimeCondition called with invalid name:', name);
+ return;
+ }
this._runtimeOverrides[name] = Boolean(value);
}
setRuntimeConditions(overrides) {
+ if (!overrides || typeof overrides !== 'object') {
+ console.warn('[ConditionEvaluator] setRuntimeConditions called with invalid overrides:', overrides);
+ return;
+ }
for (const [name, value] of Object.entries(overrides)) {
this._runtimeOverrides[name] = Boolean(value);
}
}📝 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
| setRuntimeCondition(name, value) { | |
| this._runtimeOverrides[name] = Boolean(value); | |
| } | |
| /** | |
| * Set multiple runtime conditions at once. | |
| * @param {Object.<string, boolean>} overrides - Map of condition name to value | |
| */ | |
| setRuntimeConditions(overrides) { | |
| for (const [name, value] of Object.entries(overrides)) { | |
| this._runtimeOverrides[name] = Boolean(value); | |
| } | |
| } | |
| setRuntimeCondition(name, value) { | |
| if (typeof name !== 'string' || !name.trim()) { | |
| console.warn('[ConditionEvaluator] setRuntimeCondition called with invalid name:', name); | |
| return; | |
| } | |
| this._runtimeOverrides[name] = Boolean(value); | |
| } | |
| /** | |
| * Set multiple runtime conditions at once. | |
| * `@param` {Object.<string, boolean>} overrides - Map of condition name to value | |
| */ | |
| setRuntimeConditions(overrides) { | |
| if (!overrides || typeof overrides !== 'object') { | |
| console.warn('[ConditionEvaluator] setRuntimeConditions called with invalid overrides:', overrides); | |
| return; | |
| } | |
| for (const [name, value] of Object.entries(overrides)) { | |
| this._runtimeOverrides[name] = Boolean(value); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/orchestration/condition-evaluator.js around lines 73 - 85,
Validate inputs for the public API: in setRuntimeCondition(name, value) ensure
name is a non-empty string (throw a TypeError if not) before assigning
this._runtimeOverrides[name] = Boolean(value); in
setRuntimeConditions(overrides) ensure overrides is a non-null plain object
(throw a TypeError if it's null/undefined or not an object), then iterate
Object.entries(overrides) and validate each key is a non-empty string before
setting this._runtimeOverrides[name] = Boolean(value); keep using the same
_runtimeOverrides storage and preserve current Boolean coercion for values.
Comment on lines
+143
to
+146
| stories_remaining: () => | ||
| this._runtimeOverrides['stories_remaining'] !== undefined | ||
| ? this._runtimeOverrides['stories_remaining'] | ||
| : true, // Repeat story cycle; orchestrator sets false when backlog is empty |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for orchestrators calling setRuntimeConditions or setRuntimeCondition
# Expected: At least one call setting stories_remaining to false
echo "=== Searching for setRuntimeCondition(s) calls ==="
rg -n 'setRuntimeCondition' --type js -C3
echo ""
echo "=== Searching for stories_remaining assignments ==="
rg -n 'stories_remaining' --type js --type yaml -C2Repository: SynkraAI/aiox-core
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Check if the files exist and examine ConditionEvaluator usage
echo "=== Checking for condition-evaluator.js ==="
find . -name "condition-evaluator.js" -type f
echo ""
echo "=== Checking for workflow-orchestrator.js ==="
find . -name "workflow-orchestrator.js" -type f
echo ""
echo "=== Searching for ConditionEvaluator instantiation ==="
rg -n "new ConditionEvaluator\|ConditionEvaluator" --type js -C 2
echo ""
echo "=== Checking for any loop conditions or stories_remaining in workflows ==="
rg -n "while\|for\|loop" --type js -B2 -A2 | head -50
echo ""
echo "=== Checking for TODO/FIXME related to conditions ==="
rg -n "TODO|FIXME" --type js -C 2 | grep -i "condition\|loop\|orchestrat" -C 2Repository: SynkraAI/aiox-core
Length of output: 884
🏁 Script executed:
#!/bin/bash
# Read the specific files referenced in the review
echo "=== Reading condition-evaluator.js (lines 130-160) ==="
sed -n '130,160p' ./.aiox-core/core/orchestration/condition-evaluator.js
echo ""
echo "=== Reading workflow-orchestrator.js (lines 355-375) ==="
sed -n '355,375p' ./.aiox-core/core/orchestration/workflow-orchestrator.js
echo ""
echo "=== Checking full condition-evaluator.js file length ==="
wc -l ./.aiox-core/core/orchestration/condition-evaluator.js
echo ""
echo "=== Checking full workflow-orchestrator.js file length ==="
wc -l ./.aiox-core/core/orchestration/workflow-orchestrator.js
echo ""
echo "=== Searching for imports of ConditionEvaluator across codebase ==="
rg -n "import.*ConditionEvaluator\|require.*condition-evaluator" --type jsRepository: SynkraAI/aiox-core
Length of output: 3365
🏁 Script executed:
#!/bin/bash
# Check for setRuntimeConditions method and its usage
echo "=== Searching for setRuntimeConditions method definition ==="
rg -n "setRuntimeConditions\|setRuntimeCondition" --type js -A 5
echo ""
echo "=== Searching for conditionEvaluator method calls (evaluate, etc.) ==="
rg -n "conditionEvaluator\." --type js -B 1 -A 1
echo ""
echo "=== Searching for stories_remaining usage in workflow logic ==="
rg -n "stories_remaining" --type js -B 2 -A 2
echo ""
echo "=== Checking workflow execution loops and conditionals ==="
rg -n "while.*condition|if.*condition" --type js -C 2 | head -60Repository: SynkraAI/aiox-core
Length of output: 1785
🏁 Script executed:
#!/bin/bash
# Read the entire condition-evaluator.js file to understand the class structure
echo "=== Reading condition-evaluator.js class definition (first 100 lines) ==="
head -100 ./.aiox-core/core/orchestration/condition-evaluator.js
echo ""
echo "=== Reading for public methods and setRuntimeConditions ==="
rg -n "^\s+(set|get|evaluate)" ./.aiox-core/core/orchestration/condition-evaluator.js
echo ""
echo "=== Checking export statements ==="
tail -50 ./.aiox-core/core/orchestration/condition-evaluator.jsRepository: SynkraAI/aiox-core
Length of output: 5249
🏁 Script executed:
#!/bin/bash
# Search for actual usage of conditionEvaluator and evaluate method calls
echo "=== Searching for evaluate method calls ==="
rg -n "\.evaluate\(" --type js -B 2 -A 2
echo ""
echo "=== Searching for condition evaluation in orchestrator ==="
grep -n "should.*Execute\|evaluate.*Phase\|stories_remaining" ./.aiox-core/core/orchestration/workflow-orchestrator.js -C 3
echo ""
echo "=== Checking if stories_remaining is referenced in phase configs ==="
rg -n "stories_remaining\|loop.*condition" --type js --type json --type yaml -C 2Repository: SynkraAI/aiox-core
Length of output: 7274
🏁 Script executed:
#!/bin/bash
# Check phase definitions and configurations
echo "=== Searching for phase definitions with condition properties ==="
rg -n "phase.*condition\|condition.*stories\|condition.*epic_complete" --type js --type json -B 2 -A 2
echo ""
echo "=== Checking orchestrator for phase configuration loading ==="
grep -n "phases\|getPhases\|phaseConfig" ./.aiox-core/core/orchestration/workflow-orchestrator.js -C 2 | head -40
echo ""
echo "=== Looking for any YAML/JSON phase definitions ==="
find ./.aiox-core -name "*.yaml" -o -name "*.yml" -o -name "*.json" | xargs grep -l "phase\|condition" 2>/dev/null | head -10
echo ""
echo "=== Checking if setRuntimeConditions is ever called (comprehensive search) ==="
git grep -n "setRuntimeCondition" 2>/dev/null || rg -r "setRuntimeCondition" . --type js --type json --type yamlRepository: SynkraAI/aiox-core
Length of output: 2806
🏁 Script executed:
#!/bin/bash
# Check actual workflow structure
echo "=== Reading a sample workflow definition ==="
head -100 ./.aiox-core/development/workflows/brownfield-discovery.yaml
echo ""
echo "=== Checking for condition properties in workflows ==="
grep -n "condition:" ./.aiox-core/development/workflows/*.yaml ./.aiox-core/development/templates/squad/workflow-template.yaml
echo ""
echo "=== Checking bob surface criteria for phase conditions ==="
head -80 ./.aiox-core/core/orchestration/bob-surface-criteria.yamlRepository: SynkraAI/aiox-core
Length of output: 12452
Integrate runtime condition setting into orchestrator to enable loop termination.
The stories_remaining condition is actively used across multiple workflows (brownfield-fullstack.yaml:226, greenfield-fullstack.yaml:246, etc.) as a loop guard. However, setRuntimeConditions() is never called anywhere in the codebase, meaning stories_remaining will always evaluate to its default value of true, preventing loop termination even when the orchestrator detects an empty backlog.
To fix this, the orchestrator must call setRuntimeConditions({stories_remaining: false}) when the story backlog is exhausted. The same applies to other loop-termination conditions (epic_complete) and user-driven conditions that are documented in condition-evaluator.js but are currently non-functional due to missing orchestrator integration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.aiox-core/core/orchestration/condition-evaluator.js around lines 143 - 146,
The orchestrator currently never invokes setRuntimeConditions, so the condition
function stories_remaining in condition-evaluator.js always returns the default
true; update the orchestrator (where it detects the story backlog exhaustion) to
call setRuntimeConditions({ stories_remaining: false }) so loop guards can
terminate; likewise set other documented termination flags (e.g., epic_complete)
via setRuntimeConditions when their corresponding orchestrator state changes so
condition-evaluator's runtime overrides are honored.
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 4 interconnected bugs in the resilience/core subsystem:
bug: ideation-engine falha ao instanciar GotchasMemory (export named vs default) #517 —
ideation-engine.jsimported the gotchas-memory module object instead of theGotchasMemoryclass (require()vs destructured{ GotchasMemory }). This causednew GotchasMemory()to throwTypeError: is not a constructor, silently disabling gotchas filtering.bug: GotchasMemory.trackError() ignores errorWindowMs — stale errors trigger auto-capture #475 —
GotchasMemory.trackError()accumulated error counts indefinitely without checkingerrorWindowMs. A single error across multiple days would trigger auto-capture. Now resets count when the time since last occurrence exceeds the configured window.bug: ConditionEvaluator returns true for unknown conditions (permissive fallback) #472 —
ConditionEvaluatorreturnedtruefor unknown conditions (typos, nonexistent). This is a security concern — a misspelled condition likehas_databsewould silently pass. Changed tofalse(fail-safe principle).bug(circuit-breaker): recordFailure() in OPEN state resets recovery timeout indefinitely #469 —
CircuitBreaker.recordFailure()in OPEN state resetlastFailureTime, restarting the recovery timeout on every new failure. Under sustained load, the circuit would never transition to HALF_OPEN. Now preserves the original recovery window when already OPEN.Test plan
npm run lint— 0 errorsnpm run typecheck— passesCloses #517
Closes #475
Closes #472
Closes #469
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Changes
Chores