fix(condition-evaluator): return false for unknown conditions (fail-safe)#474
fix(condition-evaluator): return false for unknown conditions (fail-safe)#474nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
nikolasdehor
requested review from
Pedrovaleriolopez and
oalanicolas
as code owners
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request fixes a critical issue in ConditionEvaluator where unknown conditions defaulted to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/core/orchestration/condition-evaluator.test.js (2)
292-295: Consider adding a guard or comment for the minimal profile in this test.
new ConditionEvaluator({ database: null })creates a profile missing most fields (hasDatabase,hasFrontend,hasBackend, etc.). This works because the test only exercises the dot-notation path, but if someone extends this test to also check a built-in evaluator, it would throw accessingthis.profile.hasDatabaseon the sparse object. A brief comment noting the intent would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/condition-evaluator.test.js` around lines 292 - 295, The test creates a sparse profile via new ConditionEvaluator({ database: null }) which can cause other evaluators to throw when they reference this.profile.hasDatabase; update the test to either include a brief comment clarifying the intent (that this is a minimal profile only exercising dot-notation) or expand the mock profile to include the standard boolean flags (e.g., hasDatabase, hasFrontend, hasBackend) so built-in evaluators won't access undefined properties; reference ConditionEvaluator, evaluate, and this.profile.hasDatabase when making the change.
87-119: Minor:project_has_backend,project_has_typescript, andproject_has_testsonly test thetruecase.The database and frontend conditions test both true and false paths (using EMPTY_PROFILE), but
project_has_backend,project_has_typescript, andproject_has_testsonly verify the positive result with FULL_PROFILE. Adding a negative-path assertion with EMPTY_PROFILE for at least one of these would round out coverage symmetrically. Not blocking — the underlying evaluator logic is trivially a property access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/condition-evaluator.test.js` around lines 87 - 119, Add negative-path assertions for the built-in conditions by instantiating a ConditionEvaluator with EMPTY_PROFILE and asserting evaluate(...) returns false for the same keys currently only tested true; specifically, after the positive checks for 'project_has_backend', 'project_has_typescript', and 'project_has_tests' call const ev = new ConditionEvaluator(EMPTY_PROFILE) (or reuse one) and add expect(ev.evaluate('project_has_backend')).toBe(false), expect(ev.evaluate('project_has_typescript')).toBe(false), and expect(ev.evaluate('project_has_tests')).toBe(false) to mirror the coverage used for 'project_has_database' and 'project_has_frontend'..aios-core/core/orchestration/condition-evaluator.js (3)
69-100: Theevaluatorsmap is re-created on everyevaluate()call.Since
evaluate()is called recursively for composite conditions (AND/OR splits, negation), this object literal is allocated multiple times per top-level evaluation. For a performance-sensitive orchestration path, consider hoisting the evaluator map to the constructor or using a lazy-initialized instance property.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/orchestration/condition-evaluator.js around lines 69 - 100, The evaluators map is being rebuilt on every evaluate() invocation causing unnecessary allocations during recursive condition checks; move the evaluators object out of evaluate() into a single instance property initialized either in the constructor (e.g., this.evaluators) or lazily on first use, ensuring the functions still reference this.profile and instance methods like _checkQAApproval and _checkPhaseCompleted correctly; update evaluate() to use this.evaluators (or initialize it if undefined) so composite conditions (AND/OR/negation) reuse the same evaluator map and avoid repeated allocations.
62-66: Consider adding basic input validation for theconditionparameter type.
evaluate()accepts any value. If a non-string truthy value (e.g., an object or number) is passed, the method will proceed to call.startsWith(),.includes(), etc., causing a runtime TypeError. A quick guard at the top would harden this public API.As per coding guidelines, "Check for proper input validation on public API methods" for
.aios-core/core/**.🛡️ Proposed defensive guard
evaluate(condition) { // Handle null/undefined conditions if (!condition) { return true; } + if (typeof condition !== 'string') { + console.warn(`[ConditionEvaluator] Invalid condition type: ${typeof condition}`); + return false; + } + // Built-in condition evaluators const evaluators = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/orchestration/condition-evaluator.js around lines 62 - 66, The evaluate method currently assumes condition is a string and will call .startsWith/.includes, so add a defensive type check in evaluate: keep the existing null/undefined guard, then if typeof condition !== 'string' throw a TypeError (or return a clear failure) with a message like "condition must be a string"; reference the evaluate function in condition-evaluator.js and ensure this check runs before any .startsWith/.includes calls to prevent runtime TypeErrors.
108-111: Pre-existing: negation is evaluated before operator splitting, giving!(expr)semantics instead of(!a) op b.
!a && benters the negation branch (line 109) and is interpreted as!(a && b)rather than(!a) && b. This is documented as a limitation on lines 114-115, but it could surprise users writing conditions like!project_has_database && project_has_frontend. Since this is a critical-path orchestration module, consider addressing this in a follow-up.A robust fix would check for operators before stripping the
!, or only strip the!when the remainder contains no unparenthesized operators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aios-core/core/orchestration/condition-evaluator.js around lines 108 - 111, The negation handling in evaluate currently strips a leading '!' before operator splitting, causing "!a && b" to be parsed as "!(a && b)"; update evaluate (the method using condition.startsWith and this.evaluate) to check for logical operators first and only treat a leading '!' as a simple negation when the remainder contains no unparenthesized operators (or when the leading '!' applies to a parenthesized expression). Implement a check that scans the remainder for unparenthesized operators (&&, ||, etc.) and parentheses depth, and only remove the leading '!' and recurse when no such operators are found (otherwise proceed to operator-splitting logic first).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.aios-core/core/orchestration/condition-evaluator.js:
- Around line 69-100: The evaluators map is being rebuilt on every evaluate()
invocation causing unnecessary allocations during recursive condition checks;
move the evaluators object out of evaluate() into a single instance property
initialized either in the constructor (e.g., this.evaluators) or lazily on first
use, ensuring the functions still reference this.profile and instance methods
like _checkQAApproval and _checkPhaseCompleted correctly; update evaluate() to
use this.evaluators (or initialize it if undefined) so composite conditions
(AND/OR/negation) reuse the same evaluator map and avoid repeated allocations.
- Around line 62-66: The evaluate method currently assumes condition is a string
and will call .startsWith/.includes, so add a defensive type check in evaluate:
keep the existing null/undefined guard, then if typeof condition !== 'string'
throw a TypeError (or return a clear failure) with a message like "condition
must be a string"; reference the evaluate function in condition-evaluator.js and
ensure this check runs before any .startsWith/.includes calls to prevent runtime
TypeErrors.
- Around line 108-111: The negation handling in evaluate currently strips a
leading '!' before operator splitting, causing "!a && b" to be parsed as "!(a &&
b)"; update evaluate (the method using condition.startsWith and this.evaluate)
to check for logical operators first and only treat a leading '!' as a simple
negation when the remainder contains no unparenthesized operators (or when the
leading '!' applies to a parenthesized expression). Implement a check that scans
the remainder for unparenthesized operators (&&, ||, etc.) and parentheses
depth, and only remove the leading '!' and recurse when no such operators are
found (otherwise proceed to operator-splitting logic first).
In `@tests/core/orchestration/condition-evaluator.test.js`:
- Around line 292-295: The test creates a sparse profile via new
ConditionEvaluator({ database: null }) which can cause other evaluators to throw
when they reference this.profile.hasDatabase; update the test to either include
a brief comment clarifying the intent (that this is a minimal profile only
exercising dot-notation) or expand the mock profile to include the standard
boolean flags (e.g., hasDatabase, hasFrontend, hasBackend) so built-in
evaluators won't access undefined properties; reference ConditionEvaluator,
evaluate, and this.profile.hasDatabase when making the change.
- Around line 87-119: Add negative-path assertions for the built-in conditions
by instantiating a ConditionEvaluator with EMPTY_PROFILE and asserting
evaluate(...) returns false for the same keys currently only tested true;
specifically, after the positive checks for 'project_has_backend',
'project_has_typescript', and 'project_has_tests' call const ev = new
ConditionEvaluator(EMPTY_PROFILE) (or reuse one) and add
expect(ev.evaluate('project_has_backend')).toBe(false),
expect(ev.evaluate('project_has_typescript')).toBe(false), and
expect(ev.evaluate('project_has_tests')).toBe(false) to mirror the coverage used
for 'project_has_database' and 'project_has_frontend'.
There was a problem hiding this comment.
Pull request overview
This PR fixes bug #472 by changing ConditionEvaluator.evaluate() to return false (fail-safe) instead of true (permissive) for unknown condition strings. The change prevents workflow phases from executing when their guard conditions contain typos or undefined condition names.
Changes:
- Modified default return value from
truetofalsefor unknown conditions in ConditionEvaluator - Added 56 comprehensive unit tests covering all condition evaluation scenarios
- Updated install-manifest.yaml with new file hashes and metadata
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.aios-core/core/orchestration/condition-evaluator.js |
Changed unknown condition fallback from permissive (true) to fail-safe (false) on line 150 |
tests/core/orchestration/condition-evaluator.test.js |
Added comprehensive test suite with 56 tests including 5 regression tests for bug #472 |
.aios-core/install-manifest.yaml |
Updated file hashes, sizes, and generation timestamp (metadata only) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Unknown condition — fail-safe: deny rather than allow (fixes #472) | ||
| console.warn(`[ConditionEvaluator] Unknown condition: ${condition}`); | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
This change from returning true to false for unknown conditions is a breaking change that will affect existing workflows. Several workflow files contain conditions that are not registered in the ConditionEvaluator's evaluators map (lines 69-100), such as:
based_on_classification(brownfield-fullstack.yaml:47)major_enhancement_path(brownfield-fullstack.yaml:69)documentation_inadequate(brownfield-fullstack.yaml:83)user_wants_ai_generation(greenfield-fullstack.yaml:98)architecture_suggests_prd_changes(multiple workflow files)po_checklist_issues(multiple workflow files)- And many others
With this change, phases using these conditions will now be skipped instead of executing, which reverses the current behavior. Before merging, you should either:
- Add all these conditions to the evaluators map in the ConditionEvaluator class, OR
- Verify that these workflows are deprecated/unused, OR
- Update the workflows to remove or replace these conditions, OR
- Add a migration path that warns about these specific condition names
The legacy evaluator (_evaluateConditionLegacy in workflow-orchestrator.js:687) also returns true for unknown conditions, so this issue exists in both code paths.
nikolasdehor
force-pushed
the
fix/condition-evaluator-fail-safe-unknown
branch
from
ec4eab0 to
81f6239
Compare
…afe) Previously evaluate() returned true for any unrecognized condition, causing workflow phases to execute even when their guard was invalid or mistyped. Change to return false so unknown conditions are denied rather than silently permitted. 56 unit tests added including 5 regression tests for this fix. Closes SynkraAI#472
nikolasdehor
force-pushed
the
fix/condition-evaluator-fail-safe-unknown
branch
from
81f6239 to
30139e2
Compare
|
Rebased on latest main and resolved the install-manifest.yaml conflict. Also fixed a wrong import path in the test file ( |
github-actions
bot
added
area: agents
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@Pedrovaleriolopez, gostaria de solicitar review deste PR. Ele está aberto desde 23/fev e corrige a issue #472 (ConditionEvaluator retornando Recentemente foi aberto o PR #584 por outro contribuidor que inclui a mesma correção em um PR consolidado, porém 16 dias depois do nosso. Nosso PR está MERGEABLE e pronto para merge. Agradeço a atenção! |
Resumo
evaluateCondition()que retornavatruepara condições desconhecidasfalse(fail-safe) — condições desconhecidas bloqueiam em vez de permitirCloses #472
Motivação
Princípio de fail-safe: se o sistema não reconhece uma condição, deve negar por padrão. O comportamento anterior significava que um typo em
condition_namesilenciosamente autorizava a execução.Plano de teste