feat(edgeV2): EDGE snippet-aligned pursue/skip guards with shared PRISM helpers#7
Merged
Conversation
Emit per-goal achieved formulas (AND/OR over child _achieved) per EDGE snippets. Update validator expectations to match. Co-authored-by: Cursor <cursoragent@cursor.com>
Declare _state for all goals; add _chosen for choice and _failed for degradation. Update validator expectations and context-var filtering. Run lib watch alongside Next dev from packages/ui for faster iteration. Co-authored-by: Cursor <cursoragent@cursor.com>
…achieved Introduce goalState() helper and use _state plus derived G_achieved in guards and updates so emitted PRISM matches the Edge V2 variable model. Co-authored-by: Cursor <cursoragent@cursor.com>
Add configurable inclusive upper bound (API achievabilitySpace or default 10). Thread options through goalModules; validator expects one decision variable per goal. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose decision variable max in EngineSelector for edgeV2; transform API forwards achievabilitySpace to the generator. Co-authored-by: Cursor <cursoragent@cursor.com>
Emit formula G_achievable using child products and OR inclusion–exclusion, consistent with the original edge template. Extend validator expectations and goal-variable context regex; log LEAF achievability for goals with no pursueable children. Co-authored-by: Cursor <cursoragent@cursor.com>
Emit nondeterministic decision_G<id> with const int (no module-bound int or achievabilitySpace). Parse nondetConstants in the validator; stop passing achievabilitySpace for Edge V2 from the API; remove the UI control for Edge V2 decision bound. Co-authored-by: Cursor <cursoragent@cursor.com>
Sequential: parent achievability vs parent decision; prior siblings use G*_achieved (not only maintain). Interleaved: per-child Gk_achievable*10 > decision_Gk on child pursue lines; parent self-pursue unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Add underscoredOrDecisionVariable, emit const int alongside decision_ for relationToChildren or. Validator expects both nondet names and generalizes module+nondet variable matching. Co-authored-by: Cursor <cursoragent@cursor.com>
…idation Align OR degradation with snippet-style retry and failover: per-child pursue lines use sibling _failed from retryMap, alternative guards at cap, and drop parent _failed plus legacy pursueDegradationGoal in the V2 template. Harden validation with multiset transition matching for duplicate labels, coercePositiveIntRetry for JSON string retry counts, and a short failure summary when generateValidatedPrismModel rejects output. Co-authored-by: Cursor <cursoragent@cursor.com>
Emit multiple [skip_G] commands for OR choice (uncommitted plus per branch) and for OR degradation (per-sibling retry + failover guards with sibling _failed). Join skip lines in the goal module and extend expected transition counts for multiset validation. Co-authored-by: Cursor <cursoragent@cursor.com>
Emit const int decision_<task> for every task after goal decision vars. For AND parents with no execution detail, child pursues use the same achievable-vs-decision guard as interleaved; for OR parents with no detail, use alternative-style pursue guards. Enables basic AND children (e.g. tasks) to reference decision_T in pursue lines. Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce prismGuards.ts with ACHIEVABILITY_DECISION_SCALE,
achievableGtDecision/LeDecision, childIdle, and pursueableChildren so
the scale constant and filter/idle patterns have a single source of
truth across skip, pursue, and validator files.
Add cappedDegradationChildren to variables.ts to encapsulate the OR
degradation {child, cap}[] computation used in pursue/index, skip, and
expectedElements.
Remove all local ACHIEVABILITY_DECISION_SCALE copies, duplicated
Node.children().filter(!isResource) calls, and inline childNotPursued /
siblingIdle expressions. Generated PRISM output is unchanged.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a complete new Edge V2 (PRISM/Dtmc) engine. It introduces Edge V2 type definitions and mapper, template generators for decision variables/goal modules/change manager/system, a parser+validator with reporting, a logger, UI integration (engine selection, API routes, model generation), and example snippets. ChangesEdge V2 Engine Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
packages/lib/src/engines/edgeV2/template/modules/system/template/variable.ts (2)
7-31: 💤 Low valueTighten
lowerBound/upperBoundtypes tonumber.Allowing
booleanfor int bounds is misleading — the int branch produces[${lowerBound}..${upperBound}]and a boolean would emit invalid PRISM ([true..false]). Callers insystem/template/index.tsalready pass numeric bounds derived from the int-typedEdgeResourceVariable. Also consider asserting they are defined whentype === 'int'.♻️ Proposed type narrowing
export const defineVariable = ( variable: string, initialValue: number | boolean | 'MISSING_VARIABLE_DEFINITION', type: 'boolean' | 'int', context: 'resource' | 'context', - lowerBound?: number | boolean, - upperBound?: number | boolean, + lowerBound?: number, + upperBound?: number, ): string => {🤖 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 `@packages/lib/src/engines/edgeV2/template/modules/system/template/variable.ts` around lines 7 - 31, The defineVariable function allows lowerBound and upperBound to be boolean which is invalid for the 'int' branch; change the parameter types lowerBound?: number and upperBound?: number (narrow from number | boolean), and add a runtime assertion inside defineVariable: when type === 'int' ensure lowerBound and upperBound are defined (throw or log and fail) before emitting the `[${lowerBound}..${upperBound}]` string; update any callers (e.g., where EdgeResourceVariable is used) if needed to pass numeric bounds only so the int branch always receives numbers.
4-5: 💤 Low valueDrop unnecessary template literal.
resource.idis already a string.♻️ Proposed simplification
-export const resourceVariableName = (resource: EdgeResource): string => - `${resource.id}`; +export const resourceVariableName = (resource: EdgeResource): string => + resource.id;🤖 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 `@packages/lib/src/engines/edgeV2/template/modules/system/template/variable.ts` around lines 4 - 5, The function resourceVariableName currently returns `${resource.id}` using an unnecessary template literal; change the implementation of export const resourceVariableName (taking EdgeResource) to return resource.id directly (ensure the return type remains string and adjust any typings if needed) to simplify the code and avoid the redundant template operation.packages/lib/src/engines/edgeV2/mapper.ts (1)
266-294: ⚡ Quick winAdd validation to reject self-referential and cyclic
dependsOndependencies.The
afterCreationMapperdoes not detect when a goal lists itself independsOnor forms a cyclic chain (e.g.,A → B → A). While downstream code (goalDependencyStatement) currently processes only immediate dependencies without recursion, self-referential and cyclic dependencies are logically invalid and should be rejected at creation time rather than allowed into the model. This prevents downstream code from accidentally introducing recursive traversal without realizing cycles are possible, and maintains data integrity.Consider adding a simple visited set check during resolution:
- For each resolved dependency ID, verify it is not the current node's ID
- For each resolved dependency, optionally verify no cycles exist by walking up the dependency chain with cycle detection
🤖 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 `@packages/lib/src/engines/edgeV2/mapper.ts` around lines 266 - 294, In afterCreationMapper, reject self-referential and cyclic dependsOn entries: when resolving depIds (using parseDependsOn) first throw if any id === node.id (self-dependency), then perform a simple cycle check for each dep id by walking its dependency chain via allNodes and parseDependsOn (maintain a visited set per traversal) and throw if you encounter the original node id or any repeated id (cycle). Reference afterCreationMapper, parseDependsOn, allNodes, node.id and node.properties.engine when adding these validations; keep throwing Error with clear messages (e.g., "[INVALID MODEL]: Self-dependency..." or "[INVALID MODEL]: Cyclic dependency...") and only return the engine props with dependsOn after these checks pass.packages/lib/src/engines/edgeV2/validator/validator.ts (1)
236-250: 💤 Low valueReplace the nested ternary with a direct lookup.
The 6-deep ternary mirrors
executionDetail.typeand is hard to scan/extend. A small set lookup or a single conditional simplifies it and avoids re-readinggoal.properties.engine.executionDetail?.typesix times.♻️ Proposed simplification
- allGoalsMapResult.forEach((goal) => { - const goalType = - goal.properties.engine.executionDetail?.type === 'choice' - ? 'choice' - : goal.properties.engine.executionDetail?.type === 'degradation' - ? 'degradation' - : goal.properties.engine.executionDetail?.type === 'sequence' - ? 'sequence' - : goal.properties.engine.executionDetail?.type === 'interleaved' - ? 'interleaved' - : goal.properties.engine.executionDetail?.type === 'alternative' - ? 'alternative' - : 'basic'; - goalTypes[goalType].expected++; - }); + const KNOWN_GOAL_TYPES = new Set<keyof GoalTypeCounts>([ + 'choice', + 'degradation', + 'sequence', + 'interleaved', + 'alternative', + ]); + allGoalsMapResult.forEach((goal) => { + const detailType = goal.properties.engine.executionDetail?.type; + const goalType: keyof GoalTypeCounts = + detailType && KNOWN_GOAL_TYPES.has(detailType as keyof GoalTypeCounts) + ? (detailType as keyof GoalTypeCounts) + : 'basic'; + goalTypes[goalType].expected++; + });🤖 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 `@packages/lib/src/engines/edgeV2/validator/validator.ts` around lines 236 - 250, The nested ternary in the allGoalsMapResult.forEach block repeatedly reads goal.properties.engine.executionDetail?.type to derive goalType; replace it with a direct lookup/validation: read executionDetailType once into a local variable (e.g., executionDetailType), then check it against an allowed set/array of types ['choice','degradation','sequence','interleaved','alternative'] and set goalType = executionDetailType if it exists in the set, otherwise 'basic'; finally increment goalTypes[goalType].expected as before. This change affects the forEach loop, the goalType computation, and uses goal.properties.engine.executionDetail?.type and goalTypes identifiers to locate the code.packages/lib/src/index.ts (1)
68-70: 💤 Low valueBoth
LoggerReporttypes are structurally identical; consider re-exporting V2's version for symmetry.
initEdgeV2Loggeris re-exported from./engines/edgeV2/logger/logger, butLoggerReportcontinues to be exported only from the V1 path. While the two types are currently identical in structure, this asymmetry means future divergences would go undetected—TransformWorkflow.tsx imports a singleLoggerReporttype that's only explicitly sourced from V1, even though V2's logger may produce a different report shape. To ensure type safety and clarity, either:
- Re-export the V2
LoggerReportas well (alongside or instead of V1's), or- Add a compile-time assertion/test ensuring the two types remain in sync.
🤖 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 `@packages/lib/src/index.ts` around lines 68 - 70, Export the V2 report type alongside the existing one and add a compile-time equality check: re-export the V2 LoggerReport (e.g., introduce a symbol like LoggerReportV2 exported from the same module where LoggerReport is exported) so callers can import the explicit V2 type and update usage (references: LoggerReport and initEdgeV2Logger), and add a TypeScript type-level assertion (e.g., an AssertLoggerReportEqual type alias that verifies LoggerReport extends LoggerReportV2 and vice versa) to fail builds if the two shapes diverge.packages/ui/lib/models/goal.ts (1)
132-157: ⚡ Quick winConsider a shared
parseForEnginehelper.This is now the third near-identical parse/validate/tree-conversion branch. Extracting the common flow would keep error shaping and future parser changes from drifting across
parseForEdge,parseForSleec, andparseForEdgeV2.🤖 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 `@packages/ui/lib/models/goal.ts` around lines 132 - 157, There are three nearly identical methods (parseForEdgeV2, parseForEdge, parseForSleec) that parse JSON, validate, and convert to a GoalTree; extract the common flow into a new helper (e.g., parseForEngine) that takes the modelJson and an engineMapper (like edgeV2EngineMapper) and encapsulates parseModel call, shared error-shaping for parse failure, the try/catch around GoalTree.fromModel, and the returned shape; then have parseForEdgeV2, parseForEdge, and parseForSleec simply call parseForEngine with their respective mapper to avoid duplication and keep error behavior consistent.
🤖 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 `@packages/lib/src/engines/edgeV2/logger/filePath.ts`:
- Around line 10-15: The getLogFilePath function currently interpolates
modelFileName directly into a path allowing directory traversal; fix it by
sanitizing modelFileName in getLogFilePath: use path.basename or otherwise strip
any path separators and .. segments, normalize/replace unsafe characters (e.g.,
slashes) to a safe filename, then build the final path with path.join('logs',
safeName + extension) and verify with path.resolve that the resolved path starts
with the resolved logs directory to prevent escapes; update both getLogFilePath
implementations referenced to use this sanitization and verification.
In `@packages/lib/src/engines/edgeV2/logger/logger.ts`:
- Around line 294-306: The switch on transition incorrectly maps case 'pursue'
to store.tasksSkippedLines; change the mapping so that case 'pursue' increments
the correct counter (store.tasksPursuedLines) instead of tasksSkippedLines in
the switch inside the logger (the transition switch that updates
store.tasksTryLines, tasksFailedLines, tasksAchievedLines, etc.), ensuring
skipped transitions remain counted by the appropriate 'skipped' case if one
exists.
In `@packages/lib/src/engines/edgeV2/mapper.ts`:
- Around line 75-123: getMaintainCondition currently mixes a thrown error for a
missing literal 'maintain' key with only warnings for empty values and then
still returns empty sentences; change it to a consistent "fail fast" validation
when customProperties.type === 'maintain' by requiring both maintain and
assertion to be present and non-empty: in getMaintainCondition validate that
('maintain' in customProperties) && customProperties.maintain &&
customProperties.assertion, throw a clear Error mentioning both missing/empty
fields by name if the check fails, and only call getAssertionVariables / build
ExecCondition when maintain/assertion are non-empty (avoid falling back to ''),
so downstream PRISM won't get empty conditions; reference getMaintainCondition,
RawProps<EdgeGoalKey | EdgeTaskKey>, ExecCondition, and getAssertionVariables
when locating and updating the logic.
- Around line 59-73: parseMaxRetries currently uses parseInt which accepts
decimals and trailing characters (e.g., "1.5" -> 1, "10x" -> 10); tighten
validation by checking the raw string against a strict integer pattern (e.g.,
/^\d+$/ or an equivalent numeric-only check) before converting to Number, and
throw the same invalid-node error if it fails; apply the same
strict-string-integer validation for lowerBound, upperBound, and initialValue in
mapResourceProps so only pure non-negative integer strings are accepted and
converted to numbers.
In
`@packages/lib/src/engines/edgeV2/template/modules/changeManager/template/achievabilityVariables/taskAchievabilityVariables.ts`:
- Around line 14-28: The current check uses isConfigured = variableName in
variableValues which treats a present-but-undefined value as configured and can
emit "const double X = undefined;"; update the condition to treat undefined as
unconfigured (e.g., isConfigured should be variableName in variableValues &&
variableValues[variableName] !== undefined) so that when the key exists but
value is undefined you fall back to DEFAULT_ACHIEVABILITY for variableValue;
ensure the emitted achievabilityConst and the logger.achievabilityTaskConstant
call use the final variableValue (and remove the workaround ?? -1 in the log).
In
`@packages/lib/src/engines/edgeV2/template/modules/changeManager/template/tasks/variables.ts`:
- Around line 19-41: The exported maxRetriesVariable function is never called,
so the failed(task.id) counter is not emitted; update the taskVariables function
to include the maxRetriesVariable(task) output (e.g., append its returned string
when non-empty alongside the pursued/achieved variables) or delete
maxRetriesVariable if you prefer not to emit per-task failed counters;
specifically, call maxRetriesVariable(task) inside taskVariables and include its
non-empty return in the aggregated task variable declarations so the
failed(task.id) counter is declared for retry logic.
In
`@packages/lib/src/engines/edgeV2/template/modules/goalModule/template/formulas.ts`:
- Around line 88-91: The OR branch currently builds an incorrect n-ary formula
(uses childrenVariables.join + productPart) that only works for two children;
update the 'or' case in this module to either reject >2 non-resource children or
generate a correct n-ary union via inclusion–exclusion: compute the sum of all
single-variable terms, subtract the sum of all pairwise products, add the sum of
all triple products, etc., alternating signs up to the full product, and then
build formulaName using that expression (reuse helper parenthesis(...) for term
grouping and childrenVariables/product helpers for forming products); if you
choose to reject, make the code detect childrenVariables.length > 2 and
throw/emit a clear validation error referencing the 'or' case and formulaName.
In
`@packages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/andGoal.ts`:
- Around line 47-75: splitSequence is run on the raw sequence but childrenMap
only contains non-resource children, which causes the mapping in the
previousAchieved computation (inside the goal.relationToChildren === 'and'
branch) to throw when a resource ID is present; fix by filtering the sequence or
skipping unknown IDs: either apply the same resource filter used to build
achievableChildren before calling splitSequence (so leftGoals/rightGoals contain
only achievable IDs), or, simpler, change the previousAchieved map to ignore IDs
not present in childrenMap (e.g., filter leftGoals by childrenMap.has(id) before
mapping) and use that filtered list when calling priorSequentialSiblingGuard,
keeping references to splitSequence, achievableChildren/childrenMap,
leftGoals/rightGoals, previousAchieved and priorSequentialSiblingGuard to locate
the change.
In
`@packages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/index.ts`:
- Around line 409-421: The retry counter update is being emitted for any child
with maxRetries>0 but the model only declares per-sibling _failed counters for
degradation children; restrict creation of updateFailedCounterStatement to the
same degradation-only path that declares those counters. Concretely, change the
condition around computing/using updateFailedCounterStatement so it only applies
when the child is in cappedDegradationChildren(goal) (or when
goal.relationToChildren === 'or' and
goal.properties.engine.executionDetail?.type === 'degradation') and skip
emitting the generic update for other children; you can alternatively remove the
generic updateFailedCounterStatement entirely and rely on the existing
degradation-only branch that handles failure counters.
In `@packages/lib/src/engines/edgeV2/validator/index.ts`:
- Around line 38-51: The validation flow is performing synchronous disk I/O
(writeFileSync) when modelFileName is present, which blocks requests in Edge V2;
update the validate() path that uses serializeValidationReportToJSON,
ensureLogFileDirectory and writeFileSync to use non-blocking I/O instead (e.g.,
fs.promises.writeFile or piping the JSON into the existing async
WriteStream/logger) or conditionally skip writing in production (dev-only flag)
so report writes do not run on every request; ensure you preserve the jsonReport
generation and only replace the synchronous writeFileSync call with an async
write and proper error handling.
In `@packages/lib/src/engines/edgeV2/validator/parser.ts`:
- Around line 35-40: The parser currently coerces any initValue other than the
string "true" into false, hiding malformed tokens; modify the logic in parser.ts
where initialValue is set (look for the initValue variable and the returned
initialValue property) to explicitly accept only "true" or "false" strings and
produce a parse/validation error when initValue is present but not exactly
"true" or "false" (e.g., throw or return a parse error with the offending
initValue and field name) instead of silently coercing to false; ensure callers
handle the new error/path appropriately.
In `@packages/ui/app/api/transform/route.ts`:
- Around line 37-40: The logger created via initLogger/initEdgeV2Logger
(assigned to the logger variable) can be leaked on early returns and on
generation parse errors; wrap the route handler body that uses logger in a
try/finally and call logger.close() in the finally block (or alternatively defer
logger initialization until after parsing/generation succeeds) so every exit
path (parse failure, generation error, and successful completion) always closes
the logger; apply the same change to the other initialization site that also
uses initLogger/initEdgeV2Logger.
In `@packages/ui/app/api/variables/route.ts`:
- Around line 12-23: The endpoint currently accepts any engine string and
silently falls back to GoalModel.parseForEdge when engine is unrecognized;
change the logic in the route handler (where you destructure const { modelJson,
engine = 'prism' } from request.json and call
GoalModel.parseForEdgeV2/parseForSleec/parseForEdge) to validate engine against
the known set ('prism', 'edgeV2', 'sleec') and return
ApiResponse.badRequest('Invalid engine') for unknown values instead of
defaulting to parseForEdge; implement this via an explicit switch or a lookup
map and a default case that returns the bad request before calling any
GoalModel.parse* method.
---
Nitpick comments:
In `@packages/lib/src/engines/edgeV2/mapper.ts`:
- Around line 266-294: In afterCreationMapper, reject self-referential and
cyclic dependsOn entries: when resolving depIds (using parseDependsOn) first
throw if any id === node.id (self-dependency), then perform a simple cycle check
for each dep id by walking its dependency chain via allNodes and parseDependsOn
(maintain a visited set per traversal) and throw if you encounter the original
node id or any repeated id (cycle). Reference afterCreationMapper,
parseDependsOn, allNodes, node.id and node.properties.engine when adding these
validations; keep throwing Error with clear messages (e.g., "[INVALID MODEL]:
Self-dependency..." or "[INVALID MODEL]: Cyclic dependency...") and only return
the engine props with dependsOn after these checks pass.
In
`@packages/lib/src/engines/edgeV2/template/modules/system/template/variable.ts`:
- Around line 7-31: The defineVariable function allows lowerBound and upperBound
to be boolean which is invalid for the 'int' branch; change the parameter types
lowerBound?: number and upperBound?: number (narrow from number | boolean), and
add a runtime assertion inside defineVariable: when type === 'int' ensure
lowerBound and upperBound are defined (throw or log and fail) before emitting
the `[${lowerBound}..${upperBound}]` string; update any callers (e.g., where
EdgeResourceVariable is used) if needed to pass numeric bounds only so the int
branch always receives numbers.
- Around line 4-5: The function resourceVariableName currently returns
`${resource.id}` using an unnecessary template literal; change the
implementation of export const resourceVariableName (taking EdgeResource) to
return resource.id directly (ensure the return type remains string and adjust
any typings if needed) to simplify the code and avoid the redundant template
operation.
In `@packages/lib/src/engines/edgeV2/validator/validator.ts`:
- Around line 236-250: The nested ternary in the allGoalsMapResult.forEach block
repeatedly reads goal.properties.engine.executionDetail?.type to derive
goalType; replace it with a direct lookup/validation: read executionDetailType
once into a local variable (e.g., executionDetailType), then check it against an
allowed set/array of types
['choice','degradation','sequence','interleaved','alternative'] and set goalType
= executionDetailType if it exists in the set, otherwise 'basic'; finally
increment goalTypes[goalType].expected as before. This change affects the
forEach loop, the goalType computation, and uses
goal.properties.engine.executionDetail?.type and goalTypes identifiers to locate
the code.
In `@packages/lib/src/index.ts`:
- Around line 68-70: Export the V2 report type alongside the existing one and
add a compile-time equality check: re-export the V2 LoggerReport (e.g.,
introduce a symbol like LoggerReportV2 exported from the same module where
LoggerReport is exported) so callers can import the explicit V2 type and update
usage (references: LoggerReport and initEdgeV2Logger), and add a TypeScript
type-level assertion (e.g., an AssertLoggerReportEqual type alias that verifies
LoggerReport extends LoggerReportV2 and vice versa) to fail builds if the two
shapes diverge.
In `@packages/ui/lib/models/goal.ts`:
- Around line 132-157: There are three nearly identical methods (parseForEdgeV2,
parseForEdge, parseForSleec) that parse JSON, validate, and convert to a
GoalTree; extract the common flow into a new helper (e.g., parseForEngine) that
takes the modelJson and an engineMapper (like edgeV2EngineMapper) and
encapsulates parseModel call, shared error-shaping for parse failure, the
try/catch around GoalTree.fromModel, and the returned shape; then have
parseForEdgeV2, parseForEdge, and parseForSleec simply call parseForEngine with
their respective mapper to avoid duplication and keep error behavior consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae468712-2b61-452b-a681-6a8bc4d602ec
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (47)
examples/edgeV2/EDGE goal snippets.txtpackage.jsonpackages/lib/src/engines/edgeV2/index.tspackages/lib/src/engines/edgeV2/logger/filePath.tspackages/lib/src/engines/edgeV2/logger/logger.tspackages/lib/src/engines/edgeV2/mapper.tspackages/lib/src/engines/edgeV2/mdp/common.tspackages/lib/src/engines/edgeV2/retryCoercion.tspackages/lib/src/engines/edgeV2/template/common.tspackages/lib/src/engines/edgeV2/template/decisionVariables.tspackages/lib/src/engines/edgeV2/template/index.tspackages/lib/src/engines/edgeV2/template/modules/changeManager/changeManager.tspackages/lib/src/engines/edgeV2/template/modules/changeManager/template/achievabilityVariables/taskAchievabilityVariables.tspackages/lib/src/engines/edgeV2/template/modules/changeManager/template/index.tspackages/lib/src/engines/edgeV2/template/modules/changeManager/template/tasks/transitions.tspackages/lib/src/engines/edgeV2/template/modules/changeManager/template/tasks/variables.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/goalModules.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/achieve.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/formulas.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/index.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/andGoal.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/common.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/index.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/orGoal.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/skip.tspackages/lib/src/engines/edgeV2/template/modules/goalModule/template/variables.tspackages/lib/src/engines/edgeV2/template/modules/rewards/index.tspackages/lib/src/engines/edgeV2/template/modules/system/system.tspackages/lib/src/engines/edgeV2/template/modules/system/template/index.tspackages/lib/src/engines/edgeV2/template/modules/system/template/variable.tspackages/lib/src/engines/edgeV2/template/prismGuards.tspackages/lib/src/engines/edgeV2/types.tspackages/lib/src/engines/edgeV2/validator/expectedElements.tspackages/lib/src/engines/edgeV2/validator/index.tspackages/lib/src/engines/edgeV2/validator/parser.tspackages/lib/src/engines/edgeV2/validator/report.tspackages/lib/src/engines/edgeV2/validator/types.tspackages/lib/src/engines/edgeV2/validator/validator.tspackages/lib/src/index.tspackages/ui/app/api/transform/route.tspackages/ui/app/api/variables/route.tspackages/ui/components/EngineSelector.tsxpackages/ui/components/OutputViewer.tsxpackages/ui/components/TransformWorkflow.tsxpackages/ui/lib/models/goal.tspackages/ui/lib/transformEngine.tspackages/ui/package.json
Comment on lines
+10
to
+15
| export const getLogFilePath = ( | ||
| modelFileName: string, | ||
| extension: string = '.log', | ||
| ): string => { | ||
| return `logs/${modelFileName}${extension}`; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all uses of path helpers
rg -n -C3 '\b(getLogFilePath|ensureLogFileDirectory)\s*\(' --type ts
# Inspect model filename origins in edgeV2 flow
rg -n -C3 '\bmodelFileName\b|\bfileName\b' --type tsRepository: vieirin/goal-controller
Length of output: 50379
🏁 Script executed:
# Check how modelFileName/fileName is derived before being passed to logger/validator
cat -n packages/lib/src/index.ts | sed -n '85,100p'Repository: vieirin/goal-controller
Length of output: 587
🏁 Script executed:
# Check the edge template generator to see how fileName is passed to validate
cat -n packages/lib/src/engines/edge/template/index.ts | sed -n '36,56p'
cat -n packages/lib/src/engines/edgeV2/template/index.ts | sed -n '32,48p'Repository: vieirin/goal-controller
Length of output: 1466
🏁 Script executed:
# Check selectModel.ts to see how fileName is derived
cat -n packages/lib/src/cli/menu/selectModel.ts | sed -n '30,46p'Repository: vieirin/goal-controller
Length of output: 699
🏁 Script executed:
# Check how inputFile is obtained in index.ts - look for argument parsing
head -n 100 packages/lib/src/index.ts | tail -n 30Repository: vieirin/goal-controller
Length of output: 907
🏁 Script executed:
# Check if there are other entry points or if inputFile could contain traversal sequences
rg -n "initLogger\(" --type ts | head -20Repository: vieirin/goal-controller
Length of output: 697
Constrain log paths to the logs root to prevent directory traversal attacks.
modelFileName parameter is interpolated directly into a filesystem path without validation. An attacker passing ../../../tmp/malicious.txt via CLI arguments could escape the logs/ directory and create files anywhere the process has write permissions.
🔒 Proposed hardening
import fs from 'fs';
import path from 'path';
+const LOGS_ROOT = path.resolve('logs');
+
export const getLogFilePath = (
modelFileName: string,
extension: string = '.log',
): string => {
- return `logs/${modelFileName}${extension}`;
+ const relativePath = path.normalize(`${modelFileName}${extension}`);
+ const fullPath = path.resolve(LOGS_ROOT, relativePath);
+ if (!fullPath.startsWith(`${LOGS_ROOT}${path.sep}`)) {
+ throw new Error('Invalid model file path');
+ }
+ return fullPath;
};Also applies to: packages/lib/src/engines/edge/logger/filePath.ts (lines 10-15)
🤖 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 `@packages/lib/src/engines/edgeV2/logger/filePath.ts` around lines 10 - 15, The
getLogFilePath function currently interpolates modelFileName directly into a
path allowing directory traversal; fix it by sanitizing modelFileName in
getLogFilePath: use path.basename or otherwise strip any path separators and ..
segments, normalize/replace unsafe characters (e.g., slashes) to a safe
filename, then build the final path with path.join('logs', safeName + extension)
and verify with path.resolve that the resolved path starts with the resolved
logs directory to prevent escapes; update both getLogFilePath implementations
referenced to use this sanitization and verification.
Comment on lines
+294
to
+306
| switch (transition) { | ||
| case 'try': | ||
| store.tasksTryLines++; | ||
| break; | ||
| case 'failed': | ||
| store.tasksFailedLines++; | ||
| break; | ||
| case 'achieve': | ||
| store.tasksAchievedLines++; | ||
| break; | ||
| case 'pursue': | ||
| store.tasksSkippedLines++; | ||
| break; |
There was a problem hiding this comment.
pursue transitions are being counted as skipped
On Line 304, case 'pursue' increments tasksSkippedLines. This makes the summary counters inaccurate and hides real skipped counts.
Proposed fix
switch (transition) {
case 'try':
store.tasksTryLines++;
break;
case 'failed':
store.tasksFailedLines++;
break;
case 'achieve':
store.tasksAchievedLines++;
break;
case 'pursue':
- store.tasksSkippedLines++;
+ // track pursue separately, or keep skipped exclusively for skip transitions
+ // store.tasksPursueLines++;
break;
}🤖 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 `@packages/lib/src/engines/edgeV2/logger/logger.ts` around lines 294 - 306, The
switch on transition incorrectly maps case 'pursue' to store.tasksSkippedLines;
change the mapping so that case 'pursue' increments the correct counter
(store.tasksPursuedLines) instead of tasksSkippedLines in the switch inside the
logger (the transition switch that updates store.tasksTryLines,
tasksFailedLines, tasksAchievedLines, etc.), ensuring skipped transitions remain
counted by the appropriate 'skipped' case if one exists.
Comment on lines
+59
to
+73
| const parseMaxRetries = ( | ||
| maxRetries: string | undefined, | ||
| nodeType: 'goal' | 'task', | ||
| ): number => { | ||
| if (!maxRetries) { | ||
| return 0; | ||
| } | ||
| const parsed = parseInt(maxRetries, 10); | ||
| if (isNaN(parsed) || parsed < 0) { | ||
| throw new Error( | ||
| `[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`, | ||
| ); | ||
| } | ||
| return parsed; | ||
| }; |
There was a problem hiding this comment.
parseMaxRetries silently accepts decimals and partial-numeric strings.
parseInt('1.5', 10) returns 1 and parseInt('10abc', 10) returns 10, so values like "1.5" or "10x" pass validation and may produce surprising PRISM output (e.g., a failed counter sized to a truncated value). Recommend tightening to a strict integer pattern.
Note: the same parseInt leniency applies to lowerBound/upperBound/initialValue in mapResourceProps (lines 217, 218, 232).
🛡️ Suggested strict parsing
const parseMaxRetries = (
maxRetries: string | undefined,
nodeType: 'goal' | 'task',
): number => {
if (!maxRetries) {
return 0;
}
- const parsed = parseInt(maxRetries, 10);
- if (isNaN(parsed) || parsed < 0) {
+ if (!/^\d+$/.test(maxRetries.trim())) {
+ throw new Error(
+ `[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`,
+ );
+ }
+ const parsed = Number(maxRetries);
+ if (!Number.isSafeInteger(parsed) || parsed < 0) {
throw new Error(
`[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`,
);
}
return parsed;
};📝 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
| const parseMaxRetries = ( | |
| maxRetries: string | undefined, | |
| nodeType: 'goal' | 'task', | |
| ): number => { | |
| if (!maxRetries) { | |
| return 0; | |
| } | |
| const parsed = parseInt(maxRetries, 10); | |
| if (isNaN(parsed) || parsed < 0) { | |
| throw new Error( | |
| `[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`, | |
| ); | |
| } | |
| return parsed; | |
| }; | |
| const parseMaxRetries = ( | |
| maxRetries: string | undefined, | |
| nodeType: 'goal' | 'task', | |
| ): number => { | |
| if (!maxRetries) { | |
| return 0; | |
| } | |
| if (!/^\d+$/.test(maxRetries.trim())) { | |
| throw new Error( | |
| `[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`, | |
| ); | |
| } | |
| const parsed = Number(maxRetries); | |
| if (!Number.isSafeInteger(parsed) || parsed < 0) { | |
| throw new Error( | |
| `[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`, | |
| ); | |
| } | |
| return parsed; | |
| }; |
🤖 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 `@packages/lib/src/engines/edgeV2/mapper.ts` around lines 59 - 73,
parseMaxRetries currently uses parseInt which accepts decimals and trailing
characters (e.g., "1.5" -> 1, "10x" -> 10); tighten validation by checking the
raw string against a strict integer pattern (e.g., /^\d+$/ or an equivalent
numeric-only check) before converting to Number, and throw the same invalid-node
error if it fails; apply the same strict-string-integer validation for
lowerBound, upperBound, and initialValue in mapResourceProps so only pure
non-negative integer strings are accepted and converted to numbers.
Comment on lines
+75
to
+123
| const getMaintainCondition = ( | ||
| customProperties: RawProps<EdgeGoalKey> | RawProps<EdgeTaskKey>, | ||
| nodeType: 'goal' | 'task', | ||
| ): ExecCondition | undefined => { | ||
| if (customProperties.type === 'maintain') { | ||
| if (!('maintain' in customProperties)) { | ||
| throw new Error( | ||
| `[INVALID MODEL]: Maintain condition for ${nodeType} must have 'maintain' and 'assertion'; got maintain: none, assertion: ${customProperties.assertion || "'empty condition'"}`, | ||
| ); | ||
| } | ||
| if (!customProperties.maintain || !customProperties.assertion) { | ||
| console.warn( | ||
| `[INVALID MODEL]: Maintain condition for ${nodeType} must have maintain and assertion: got maintain:${ | ||
| customProperties.maintain || "'empty condition'" | ||
| } and assertion:${customProperties.assertion || "'empty condition'"}`, | ||
| ); | ||
| } | ||
|
|
||
| return { | ||
| maintain: { | ||
| sentence: customProperties.maintain ?? '', | ||
| variables: getAssertionVariables({ | ||
| assertionSentence: customProperties.maintain ?? '', | ||
| }), | ||
| }, | ||
| assertion: { | ||
| sentence: customProperties.assertion ?? '', | ||
| variables: getAssertionVariables({ | ||
| assertionSentence: customProperties.assertion ?? '', | ||
| }), | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| if (customProperties.assertion) { | ||
| const assertionVariables = getAssertionVariables({ | ||
| assertionSentence: customProperties.assertion, | ||
| }); | ||
|
|
||
| return { | ||
| assertion: { | ||
| sentence: customProperties.assertion, | ||
| variables: assertionVariables, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return undefined; | ||
| }; |
There was a problem hiding this comment.
Inconsistent error handling in getMaintainCondition can silently emit empty maintain/assertion PRISM conditions.
The flow conflates errors and warnings:
- Line 80 throws only if the literal key
'maintain'is absent fromcustomProperties. IfRawPropsalways materializes allowed keys (even asundefined), this guard never triggers. - Line 85 only warns when
customProperties.maintainorcustomProperties.assertionis empty/undefined, then lines 93–106 still return anExecConditionwhose sentences fall back to''. DownstreamgetAssertionVariables({ assertionSentence: '' })will produce empty variables, and the generated PRISM will contain empty maintain/assertion expressions for what was declared as an invalid model. - The thrown error message also references
customProperties.assertioneven though the throw branch is only about a missingmaintainkey.
Either fail fast for both required fields when type === 'maintain', or accept partial conditions deliberately (and update the type so assertion is optional). Currently it's neither.
🛡️ Suggested consistent validation
const getMaintainCondition = (
customProperties: RawProps<EdgeGoalKey> | RawProps<EdgeTaskKey>,
nodeType: 'goal' | 'task',
): ExecCondition | undefined => {
if (customProperties.type === 'maintain') {
- if (!('maintain' in customProperties)) {
- throw new Error(
- `[INVALID MODEL]: Maintain condition for ${nodeType} must have 'maintain' and 'assertion'; got maintain: none, assertion: ${customProperties.assertion || "'empty condition'"}`,
- );
- }
- if (!customProperties.maintain || !customProperties.assertion) {
- console.warn(
- `[INVALID MODEL]: Maintain condition for ${nodeType} must have maintain and assertion: got maintain:${
- customProperties.maintain || "'empty condition'"
- } and assertion:${customProperties.assertion || "'empty condition'"}`,
- );
- }
+ if (!customProperties.maintain || !customProperties.assertion) {
+ throw new Error(
+ `[INVALID MODEL]: Maintain condition for ${nodeType} must have non-empty 'maintain' and 'assertion'; got maintain: ${
+ customProperties.maintain || "'empty'"
+ }, assertion: ${customProperties.assertion || "'empty'"}`,
+ );
+ }
return {
maintain: {
- sentence: customProperties.maintain ?? '',
+ sentence: customProperties.maintain,
variables: getAssertionVariables({
- assertionSentence: customProperties.maintain ?? '',
+ assertionSentence: customProperties.maintain,
}),
},
assertion: {
- sentence: customProperties.assertion ?? '',
+ sentence: customProperties.assertion,
variables: getAssertionVariables({
- assertionSentence: customProperties.assertion ?? '',
+ assertionSentence: customProperties.assertion,
}),
},
};
}🤖 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 `@packages/lib/src/engines/edgeV2/mapper.ts` around lines 75 - 123,
getMaintainCondition currently mixes a thrown error for a missing literal
'maintain' key with only warnings for empty values and then still returns empty
sentences; change it to a consistent "fail fast" validation when
customProperties.type === 'maintain' by requiring both maintain and assertion to
be present and non-empty: in getMaintainCondition validate that ('maintain' in
customProperties) && customProperties.maintain && customProperties.assertion,
throw a clear Error mentioning both missing/empty fields by name if the check
fails, and only call getAssertionVariables / build ExecCondition when
maintain/assertion are non-empty (avoid falling back to ''), so downstream PRISM
won't get empty conditions; reference getMaintainCondition, RawProps<EdgeGoalKey
| EdgeTaskKey>, ExecCondition, and getAssertionVariables when locating and
updating the logic.
Comment on lines
+14
to
+28
| // Check if variable is explicitly configured or using default | ||
| const isConfigured = variableName in variableValues; | ||
| const variableValue = isConfigured | ||
| ? variableValues[variableName] | ||
| : DEFAULT_ACHIEVABILITY; | ||
|
|
||
| if (!isConfigured) { | ||
| logger.info( | ||
| `[WARNING] Task ${task.id}: using default achievability ${DEFAULT_ACHIEVABILITY} for ${variableName} (not configured in variables file)`, | ||
| 0, | ||
| ); | ||
| } | ||
|
|
||
| const achievabilityConst = `const double ${variableName} = ${variableValue};`; | ||
| logger.achievabilityTaskConstant(task.id, variableName, variableValue ?? -1); |
There was a problem hiding this comment.
isConfigured only checks key presence, not value validity; ?? -1 masks the bug only in the log, not in the emitted PRISM.
If variableValues[variableName] is undefined even though the key exists (e.g., partial JSON parse), variableValue will be undefined, the warning will be skipped, and line 27 will emit const double X = undefined; — invalid PRISM. The variableValue ?? -1 on line 28 only patches the logger argument.
Tighten the check to also treat undefined as unconfigured (and the ?? -1 becomes dead code that can be removed).
🛡️ Proposed fix
- // Check if variable is explicitly configured or using default
- const isConfigured = variableName in variableValues;
- const variableValue = isConfigured
- ? variableValues[variableName]
- : DEFAULT_ACHIEVABILITY;
+ // Check if variable is explicitly configured (and has a numeric value) or using default
+ const configured = variableValues[variableName];
+ const isConfigured = typeof configured === 'number' && Number.isFinite(configured);
+ const variableValue = isConfigured ? configured : DEFAULT_ACHIEVABILITY;
@@
const achievabilityConst = `const double ${variableName} = ${variableValue};`;
- logger.achievabilityTaskConstant(task.id, variableName, variableValue ?? -1);
+ logger.achievabilityTaskConstant(task.id, variableName, variableValue);📝 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
| // Check if variable is explicitly configured or using default | |
| const isConfigured = variableName in variableValues; | |
| const variableValue = isConfigured | |
| ? variableValues[variableName] | |
| : DEFAULT_ACHIEVABILITY; | |
| if (!isConfigured) { | |
| logger.info( | |
| `[WARNING] Task ${task.id}: using default achievability ${DEFAULT_ACHIEVABILITY} for ${variableName} (not configured in variables file)`, | |
| 0, | |
| ); | |
| } | |
| const achievabilityConst = `const double ${variableName} = ${variableValue};`; | |
| logger.achievabilityTaskConstant(task.id, variableName, variableValue ?? -1); | |
| // Check if variable is explicitly configured (and has a numeric value) or using default | |
| const configured = variableValues[variableName]; | |
| const isConfigured = typeof configured === 'number' && Number.isFinite(configured); | |
| const variableValue = isConfigured ? configured : DEFAULT_ACHIEVABILITY; | |
| if (!isConfigured) { | |
| logger.info( | |
| `[WARNING] Task ${task.id}: using default achievability ${DEFAULT_ACHIEVABILITY} for ${variableName} (not configured in variables file)`, | |
| 0, | |
| ); | |
| } | |
| const achievabilityConst = `const double ${variableName} = ${variableValue};`; | |
| logger.achievabilityTaskConstant(task.id, variableName, variableValue); |
🤖 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
`@packages/lib/src/engines/edgeV2/template/modules/changeManager/template/achievabilityVariables/taskAchievabilityVariables.ts`
around lines 14 - 28, The current check uses isConfigured = variableName in
variableValues which treats a present-but-undefined value as configured and can
emit "const double X = undefined;"; update the condition to treat undefined as
unconfigured (e.g., isConfigured should be variableName in variableValues &&
variableValues[variableName] !== undefined) so that when the key exists but
value is undefined you fall back to DEFAULT_ACHIEVABILITY for variableValue;
ensure the emitted achievabilityConst and the logger.achievabilityTaskConstant
call use the final variableValue (and remove the workaround ?? -1 in the log).
Comment on lines
+409
to
+421
| // TODO: only update if child is part of a degradation goal | ||
| const maxRetries = child.properties.engine.maxRetries; | ||
| const updateFailedCounterStatement = | ||
| maxRetries > 0 | ||
| ? `(${failed(child.id)}'=min(${maxRetries}, ${failed(child.id)}+1))` | ||
| : ''; | ||
|
|
||
| if ( | ||
| !updateFailedCounterStatement || | ||
| isItself(child) || | ||
| (goal.relationToChildren === 'or' && | ||
| goal.properties.engine.executionDetail?.type === 'degradation') | ||
| ) { |
There was a problem hiding this comment.
The generic retry update no longer matches the declared _failed counters.
This block still adds ${child.id}_failed updates for any non-self child with maxRetries > 0, but the new validator/template path only declares per-sibling _failed variables for cappedDegradationChildren(goal). A retryable child outside OR-degradation can now emit an undeclared counter and produce an invalid model. Gate this update to the same degradation-only path that declares the variable, or remove the generic update entirely.
🤖 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
`@packages/lib/src/engines/edgeV2/template/modules/goalModule/template/pursue/index.ts`
around lines 409 - 421, The retry counter update is being emitted for any child
with maxRetries>0 but the model only declares per-sibling _failed counters for
degradation children; restrict creation of updateFailedCounterStatement to the
same degradation-only path that declares those counters. Concretely, change the
condition around computing/using updateFailedCounterStatement so it only applies
when the child is in cappedDegradationChildren(goal) (or when
goal.relationToChildren === 'or' and
goal.properties.engine.executionDetail?.type === 'degradation') and skip
emitting the generic update for other children; you can alternatively remove the
generic updateFailedCounterStatement entirely and rely on the existing
degradation-only branch that handles failure counters.
Comment on lines
+38
to
+51
| if (modelFileName) { | ||
| const jsonReport = serializeValidationReportToJSON(report); | ||
| const jsonFilePath = ensureLogFileDirectory( | ||
| modelFileName, | ||
| '.validation.json', | ||
| ); | ||
| // Only write if directory creation succeeded (not in serverless environment) | ||
| if (jsonFilePath) { | ||
| try { | ||
| writeFileSync( | ||
| jsonFilePath, | ||
| JSON.stringify(jsonReport, null, 2), | ||
| 'utf8', | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites that can reach model generation / validation:"
rg -n -C2 --type=ts 'generateValidatedPrismModel\s*\(|\bvalidate\s*\('
echo
echo "Places that pass or derive modelFileName:"
rg -n -C2 --type=ts '\bmodelFileName\b'
echo
echo "Synchronous filesystem writes in Edge V2 validator code:"
rg -n -C2 --type=ts 'writeFileSync\s*\(' packages/lib/src/engines/edgeV2Repository: vieirin/goal-controller
Length of output: 19424
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if writeFileSync is used elsewhere or if async alternatives exist ==="
rg -n 'writeFileSync|writeFile(?!Sync)' packages/lib/src/engines/ --type=ts | head -30
echo
echo "=== Check edgeV2 logger for async/sync write patterns ==="
rg -n 'write' packages/lib/src/engines/edgeV2/logger/logger.ts --type=ts | grep -E '(writeFile|WriteStream)' | head -20
echo
echo "=== Verify the full API route context and fileName passing ==="
sed -n '50,75p' packages/ui/app/api/transform/route.ts
echo
echo "=== Check if there's any conditional logic that might skip modelFileName ==="
sed -n '30,70p' packages/lib/src/engines/edgeV2/template/index.tsRepository: vieirin/goal-controller
Length of output: 423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for file write patterns ==="
rg -n 'writeFile' packages/lib/src/engines/ --type=ts
echo
echo "=== Check edgeV2 logger write stream usage ==="
rg -n 'WriteStream\|write\(' packages/lib/src/engines/edgeV2/logger/logger.ts --type=ts -A 2 | head -40
echo
echo "=== Verify the full API route context ==="
sed -n '50,80p' packages/ui/app/api/transform/route.ts
echo
echo "=== Check edgeV2 template validation call ==="
sed -n '40,60p' packages/lib/src/engines/edgeV2/template/index.tsRepository: vieirin/goal-controller
Length of output: 481
🏁 Script executed:
#!/bin/bash
set -euo zipf
echo "=== Read the full edgeV2 template to see how fileName is passed ==="
cat -n packages/lib/src/engines/edgeV2/template/index.ts | head -60
echo
echo "=== Read the API transform route around the generateValidatedPrismModel call ==="
cat -n packages/ui/app/api/transform/route.ts | sed -n '50,80p'
echo
echo "=== Check if logger uses WriteStream (which is async) ==="
grep -n 'WriteStream\|createWriteStream' packages/lib/src/engines/edgeV2/logger/logger.ts
echo
echo "=== Confirm all writeFileSync occurrences in the codebase ==="
rg -l 'writeFileSync' --type=ts packages/Repository: vieirin/goal-controller
Length of output: 117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read the full edgeV2 template to see how fileName is passed ==="
cat -n packages/lib/src/engines/edgeV2/template/index.ts | head -60
echo
echo "=== Read the API transform route around the generateValidatedPrismModel call ==="
cat -n packages/ui/app/api/transform/route.ts | sed -n '50,80p'
echo
echo "=== Check if logger uses WriteStream (which is async) ==="
grep -n 'WriteStream\|createWriteStream' packages/lib/src/engines/edgeV2/logger/logger.ts
echo
echo "=== Confirm all writeFileSync occurrences in the codebase ==="
rg -l 'writeFileSync' --type=ts packages/Repository: vieirin/goal-controller
Length of output: 3753
Avoid synchronous writes in the validation flow.
The validate() function receives fileName from the API route (always truthy via fileName || 'model'), which triggers writeFileSync() on every Edge V2 model generation request. Synchronous file I/O blocks the request thread and degrades throughput. The logger already uses async WriteStream; apply the same pattern here or gate report writing behind a dev-only flag.
🤖 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 `@packages/lib/src/engines/edgeV2/validator/index.ts` around lines 38 - 51, The
validation flow is performing synchronous disk I/O (writeFileSync) when
modelFileName is present, which blocks requests in Edge V2; update the
validate() path that uses serializeValidationReportToJSON,
ensureLogFileDirectory and writeFileSync to use non-blocking I/O instead (e.g.,
fs.promises.writeFile or piping the JSON into the existing async
WriteStream/logger) or conditionally skip writing in production (dev-only flag)
so report writes do not run on every request; ensure you preserve the jsonReport
generation and only replace the synchronous writeFileSync call with an async
write and proper error handling.
Comment on lines
+35
to
+40
| // Boolean variable | ||
| return { | ||
| name, | ||
| type: 'bool', | ||
| initialValue: initValue === 'true', | ||
| }; |
There was a problem hiding this comment.
Reject invalid boolean initializers instead of coercing to false
On Line 35, any token other than "true" becomes false silently. That masks malformed model lines and can produce misleading validation results.
Proposed fix
} else {
// Boolean variable
+ if (initValue !== 'true' && initValue !== 'false') {
+ return null;
+ }
return {
name,
type: 'bool',
initialValue: initValue === 'true',
};
}📝 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
| // Boolean variable | |
| return { | |
| name, | |
| type: 'bool', | |
| initialValue: initValue === 'true', | |
| }; | |
| } else { | |
| // Boolean variable | |
| if (initValue !== 'true' && initValue !== 'false') { | |
| return null; | |
| } | |
| return { | |
| name, | |
| type: 'bool', | |
| initialValue: initValue === 'true', | |
| }; | |
| } |
🤖 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 `@packages/lib/src/engines/edgeV2/validator/parser.ts` around lines 35 - 40,
The parser currently coerces any initValue other than the string "true" into
false, hiding malformed tokens; modify the logic in parser.ts where initialValue
is set (look for the initValue variable and the returned initialValue property)
to explicitly accept only "true" or "false" strings and produce a
parse/validation error when initValue is present but not exactly "true" or
"false" (e.g., throw or return a parse error with the offending initValue and
field name) instead of silently coercing to false; ensure callers handle the new
error/path appropriately.
Comment on lines
+37
to
+40
| const logger = | ||
| engine === 'edgeV2' | ||
| ? initEdgeV2Logger(fileName || 'model', false, true) | ||
| : initLogger(fileName || 'model', false, true); |
There was a problem hiding this comment.
Always close the logger on parse/generation failures.
After initLogger / initEdgeV2Logger, several early returns and the generation-error path bypass logger.close(). If the logger buffers output or owns file handles, failed requests leak resources and can lose the final flush. Wrap the body in try/finally, or initialize the logger only after parsing succeeds.
Suggested fix
- const logger =
- engine === 'edgeV2'
- ? initEdgeV2Logger(fileName || 'model', false, true)
- : initLogger(fileName || 'model', false, true);
+ const logger =
+ engine === 'edgeV2'
+ ? initEdgeV2Logger(fileName || 'model', false, true)
+ : initLogger(fileName || 'model', false, true);
// Generate output
let output: string;
let report: LoggerReport | null = null;
try {
@@
- report = logger.getReport();
- logger.close();
+ report = logger.getReport();
return ApiResponse.success({ output, report });
} catch (generationError) {
console.error('[API] Generation error:', generationError);
// Use 422 Unprocessable Entity for lib errors (processing errors, not server crashes)
return ApiResponse.error(
`Generation failed: ${ApiResponse.extractMessage(generationError)}`,
422,
ApiResponse.extractDetails(generationError),
);
+ } finally {
+ logger.close();
}Also applies to: 116-128
🤖 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 `@packages/ui/app/api/transform/route.ts` around lines 37 - 40, The logger
created via initLogger/initEdgeV2Logger (assigned to the logger variable) can be
leaked on early returns and on generation parse errors; wrap the route handler
body that uses logger in a try/finally and call logger.close() in the finally
block (or alternatively defer logger initialization until after
parsing/generation succeeds) so every exit path (parse failure, generation
error, and successful completion) always closes the logger; apply the same
change to the other initialization site that also uses
initLogger/initEdgeV2Logger.
Comment on lines
+12
to
+23
| const { modelJson, engine = 'prism' } = await request.json(); | ||
|
|
||
| if (!modelJson) { | ||
| return ApiResponse.badRequest('Model JSON is required'); | ||
| } | ||
|
|
||
| // Parse and validate model | ||
| const parseResult = GoalModel.parse(modelJson); | ||
| const parseResult = | ||
| engine === 'edgeV2' | ||
| ? GoalModel.parseForEdgeV2(modelJson) | ||
| : engine === 'sleec' | ||
| ? GoalModel.parseForSleec(modelJson) | ||
| : GoalModel.parseForEdge(modelJson); |
There was a problem hiding this comment.
Reject unknown engine values instead of silently falling back to PRISM.
Any typo currently routes through parseForEdge, so this endpoint can return the wrong variable set for the requested engine. That will drift from /api/transform, which already validates engine strictly.
Suggested fix
const { modelJson, engine = 'prism' } = await request.json();
if (!modelJson) {
return ApiResponse.badRequest('Model JSON is required');
}
+
+ if (!['prism', 'sleec', 'edgeV2'].includes(engine)) {
+ return ApiResponse.badRequest(
+ 'Valid engine (prism/sleec/edgeV2) is required',
+ );
+ }
const parseResult =
engine === 'edgeV2'
? GoalModel.parseForEdgeV2(modelJson)📝 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
| const { modelJson, engine = 'prism' } = await request.json(); | |
| if (!modelJson) { | |
| return ApiResponse.badRequest('Model JSON is required'); | |
| } | |
| // Parse and validate model | |
| const parseResult = GoalModel.parse(modelJson); | |
| const parseResult = | |
| engine === 'edgeV2' | |
| ? GoalModel.parseForEdgeV2(modelJson) | |
| : engine === 'sleec' | |
| ? GoalModel.parseForSleec(modelJson) | |
| : GoalModel.parseForEdge(modelJson); | |
| const { modelJson, engine = 'prism' } = await request.json(); | |
| if (!modelJson) { | |
| return ApiResponse.badRequest('Model JSON is required'); | |
| } | |
| if (!['prism', 'sleec', 'edgeV2'].includes(engine)) { | |
| return ApiResponse.badRequest( | |
| 'Valid engine (prism/sleec/edgeV2) is required', | |
| ); | |
| } | |
| const parseResult = | |
| engine === 'edgeV2' | |
| ? GoalModel.parseForEdgeV2(modelJson) | |
| : engine === 'sleec' | |
| ? GoalModel.parseForSleec(modelJson) | |
| : GoalModel.parseForEdge(modelJson); |
🤖 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 `@packages/ui/app/api/variables/route.ts` around lines 12 - 23, The endpoint
currently accepts any engine string and silently falls back to
GoalModel.parseForEdge when engine is unrecognized; change the logic in the
route handler (where you destructure const { modelJson, engine = 'prism' } from
request.json and call GoalModel.parseForEdgeV2/parseForSleec/parseForEdge) to
validate engine against the known set ('prism', 'edgeV2', 'sleec') and return
ApiResponse.badRequest('Invalid engine') for unknown values instead of
defaulting to parseForEdge; implement this via an explicit switch or a lookup
map and a default case that returns the bad request before calling any
GoalModel.parse* method.
parseInt silently accepted decimals ("1.5" → 1) and partial-numeric
strings ("10abc" → 10), which could produce silently wrong PRISM output
(e.g. a failed counter sized to a truncated value).
Add parseStrictInt that validates input against /^-?\d+$/ before
converting, so any non-integer string is rejected with NaN. Apply it to:
- coercePositiveIntRetry: also rejects non-integer number values (1.5)
by requiring Number.isInteger instead of Math.trunc.
- mapResourceProps: replaces parseInt for lowerBound, upperBound, and
initialValue; error messages now include the offending input value.
Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pursueandskipguards for AND sequential, AND interleaved, OR alternative, OR choice, and OR degradation goal types now match the EDGE goal snippets reference exactly.template/prismGuards.ts): Introduces a single source of truth for theachievable*10.0 > decision/<= decisionscale constant and helpers (achievableGtDecision,achievableLeDecision,childIdle,pursueableChildren) used across skip, pursue, and validator files.cappedDegradationChildreninvariables.ts): The{ child, cap }[]list for OR degradation goals is computed once and reused inpursue/index.ts,skip.ts, andexpectedElements.ts, eliminating divergence risk._failedcounters: OR degradation goals declare individualG<sibling>_failedvariables (not a parent-level counter) based onretryMapentries.const int decision_<taskId>is emitted for every task so basic AND goals can include per-child achievability thresholds in pursue guards.pursue_/skip_labels (choice and degradation);summarizeValidationFailuressurfaces missing elements in error messages.Test plan
pnpm exec tsc --noEmitpasses (no type errors)pnpm run buildsucceedspnpm test)Made with Cursor
Summary by CodeRabbit