refactor: extract GoalTree into separate package

[vieirin]

Summary

This PR extracts the GoalTree module into a dedicated package and introduces a generic, type-safe engine mapper architecture that enables multiple transformation engines (PRISM/Edge and SLEEC) to coexist with their own typed property systems.

Major Changes

1. New @goal-controller/goal-tree Package

• Extracted GoalTree functionality into its own package for better separation of concerns
• Moved core files: creation.ts, dependencyResolver.ts, nodeUtils.ts, printTree.ts, treeVariables.ts, types.d.ts, utils.ts, and parsers
• GoalTree can now be used independently by other packages

2. Generic Type System with Engine Mapper

• Made GoalNode, Task, and Resource generic types that accept engine-specific property types
• Introduced createEngineMapper() factory for type-safe property transformation from raw iStar strings to rich typed structures
• Added afterCreationMapper hook for cross-node reference resolution (e.g., dependsOn)
• Supports both goal/task mappers and optional resource mappers with validation

3. Multi-Engine Architecture

• Restructured packages/lib/src into engines/edge/ and engines/sleec/ directories
• Each engine has its own:
  • mapper.ts - Property transformation from iStar to engine-specific types
  • types.ts - Engine-specific type definitions
  • template/ - Output generation templates
• Edge engine includes validator/, logger/, and mdp/ modules

4. Strict TypeScript Mode

• Enabled strict TypeScript mode for the lib package
• Added explicit type annotations throughout to eliminate implicit any errors

5. Improved Validation & Error Handling

• Fail fast when tasks have goal children (invalid model structure)
• Validate resource properties (bool must be "true"/"false", int bounds validation)
• Validate choice goals have pursueable children
• Better error messages with context (goal/task ID, property name)
• Added logger warnings for missing achievability variables

6. Documentation

• Added comprehensive Engine Development Tutorial in README
• Documents the two-phase architecture (Mapper → Template)
• Includes complete SLEEC engine implementation example
• Architecture diagrams using Mermaid

Files Changed (119 files)

• New: packages/goal-tree/ - Complete goal tree package
• Restructured: packages/lib/src/engines/ - Edge and SLEEC engines
• Updated: All imports across the codebase
• Tests: Updated to use new generic types and engine mappers

Breaking Changes

• GoalNode, Task, and Resource types now require generic parameters
• Properties accessed via node.properties.engine.* instead of direct properties
• Must use an engine mapper with GoalTree.fromModel() or GoalTree.fromJSON()

Benefits

• Type Safety: Engine-specific properties are fully typed at compile time
• Extensibility: Easy to add new transformation engines
• Separation of Concerns: Goal tree logic independent from engine implementations
• Validation: Invalid models fail fast with clear error messages

Summary by CodeRabbit

• New Features

  • Added a Goal Tree package with public Model and GoalTree APIs for loading, parsing, building, querying, and serializing iStar goal trees.
  • Added two transformation engines (Edge/PRISM and SLEEC) with validated generation of models/specs and CLI support to export JSON trees.

• Chores

  • Workspace, build/dev scripts and TypeScript/test configs updated; build pipeline now builds the new package.

• Documentation

  • Transformation engine docs reorganized; legacy READMEs removed.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
goal-controller	Ready	Preview, Comment	Feb 11, 2026 0:39am

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a new workspace package @goal-controller/goal-tree (types, Model, GoalTree SDK, engine mapper, ANTLR artifacts), reorganizes lib into edge/sleec engines (mappers, templates, validators), migrates CLI/UI/tests to use the new public API, updates scripts/configs, and adapts many templates/tests to engine-prefixed properties.

Changes

Cohort / File(s)	Summary
Workspace & scripts package.json, vercel.json, packages/goal-tree/package.json, Makefile	Add @goal-controller/goal-tree workspace; add dev:goal-tree/build:goal-tree scripts; include goal-tree build in dev/build/vercel and minor Makefile fix.
Goal-tree public SDK packages/goal-tree/src/index.ts, packages/goal-tree/src/GoalTree.ts, packages/goal-tree/src/Model.ts, packages/goal-tree/src/Node.ts	Introduce public Model namespace (load/parse/validate), GoalTree class with factory methods and query interface, Node helpers and exported types.
Types & internals (goal-tree) packages/goal-tree/src/types/*, packages/goal-tree/src/internal/*, packages/goal-tree/src/internal/creation.ts, packages/goal-tree/src/internal/engineMapper.ts	Add iStar and goal-tree type system, engine-mapper factory (createEngineMapper), convertToTree pipeline, traversal/utils/variables helpers and internal barrel.
ANTLR artifacts (goal-tree) packages/goal-tree/src/antlr/*	Add generated ANTLR TypeScript lexer/parser/listener files (AssertionRegex, RTRegex) with serialized ATNs and DFA initialization.
TS projects & tests (goal-tree) packages/goal-tree/tsconfig.json, packages/goal-tree/tsconfig.test.json, packages/goal-tree/.mocharc.json, packages/goal-tree/test/**	Add tsconfigs and mocha config for the new package and tests.
Lib reorg: engines packages/lib/src/engines/index.ts, packages/lib/src/engines/edge/**, packages/lib/src/engines/sleec/**	Reorganize template engines into engines/edge and engines/sleec; add mappers, types, templates, validators and central engine exports.
Edge engine templates & validator packages/lib/src/engines/edge/template/**, packages/lib/src/engines/edge/validator/**	Add edgeDTMCTemplate, generateValidatedPrismModel, changeManagerModule and modular template pieces; adapt templates/validators to Edge types and the new GoalTree API.
SLEEC engine templates packages/lib/src/engines/sleec/**	Add SLEEC mapper, types, template modules (definitions, rules, purposes) adapted to new GoalTree API.
Lib public API & CLI/menu packages/lib/src/index.ts, packages/lib/src/cli.ts, packages/lib/src/cli/menu/*	Replace internal conversions with public Model.load and GoalTree.fromModel; adapt CLI/menu to load variables, use tree.query, and pass tree.nodes to engines.
Template modules refactor packages/lib/src/.../template/modules/*, many packages/lib/src/...	Widespread updates: read engine properties under properties.engine, replace internal GoalTree helpers with GoalTree/Node public APIs, and change GoalNode→Edge/Sleec typed usages across many template modules.
Removed / moved code & docs packages/lib/src/templateEngine/modules/changeManager/*, packages/lib/src/prismValidator/README.md, packages/lib/src/sleecTemplateEngine/README.md, README.md	Remove legacy module(s) and old READMEs; top-level README updated to engine-centric documentation.
Tests & tsconfigs (lib) packages/lib/test/**, packages/lib/test/tsconfig.json, packages/lib/.mocharc.json	Update tests to use Model.load/GoalTree.fromModel, add test tsconfigs, and adjust mocha/ts-node settings.
Package integrations packages/lib/package.json, packages/ui/package.json	Add workspace dependency @goal-controller/goal-tree to lib and ui packages; update lib test script to set TS_NODE compiler options.
Tooling & config tweaks .claude/settings.local.json, .gitignore, .vscode/settings.json	Expand Bash allowlist entries, add tsbuildinfo ignores, change TypeScript default formatter, and other tooling adjustments.
ANTLR updates in lib packages/lib/src/antlr/*	Reformat/modernize generated ANTLR files and listener signatures in lib; switch to lazy ATN/DFA initialization pattern.
Numerous small type & import updates many packages/lib/**, packages/ui/** files	Adjust imports, types, and property access to the new GoalTree API and engine-prefixed properties across templates, utilities, and tests.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant Model
    participant GoalTree
    participant Mapper
    participant Template
    participant Validator
    participant Output

    User->>CLI: submit model (file or JSON)
    CLI->>Model: Model.load() / Model.parse()
    Model-->>CLI: IStarModel
    CLI->>GoalTree: GoalTree.fromModel(model, Mapper)
    GoalTree->>Mapper: convertToTree / mapGoalProps
    Mapper-->>GoalTree: TreeNode[] (with engine props)
    CLI->>Template: generate({ gm: tree.nodes }, variables)
    Template->>GoalTree: query / Node helpers (allByType, children)
    Template->>Validator: validate(gm: tree.nodes, model)
    Validator-->>Template: ValidationReport
    Template-->>Output: generated model string
    Output-->>User: deliver transformed model

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🐰 I hopped through types and parser trees,

Mapped raw props into engine seas,
GoalTree sprouted, engines spun,
Edge and SLEEC now working as one,
A tiny hop — a build begun!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'refactor: extract GoalTree into separate package' is a clear and specific summary of the main change in the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch generic-goal-tree

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

packages/lib/src/mdp/common.ts (1)

36-37: Pre-existing bug: AND function uses wrong separator.

The AND function on line 37 uses separator('or') instead of separator('and'). This appears to be a pre-existing bug not introduced by this PR, but it should be fixed.

🐛 Proposed fix

 export const AND = (elements: string[]): string =>
-  elements.join(separator('or'));
+  elements.join(separator('and'));

packages/lib/src/templateEngine/modules/changeManager/template/achievabilityVariables/taskAchievabilityVariables.ts (1)

12-16: Falsy check on numeric value may reject valid 0 values.

!variableValue evaluates to true when variableValue is 0, which could be a legitimate value. Consider checking for undefined or null explicitly.

🐛 Proposed fix

-  if (!variableValue) {
+  if (variableValue === undefined || variableValue === null) {

Or using nullish check:

-  if (!variableValue) {
+  if (variableValue == null) {

🤖 Fix all issues with AI agents

In @.claude/settings.local.json:
- Around line 13-23: The .claude/settings.local.json contains user-specific
absolute paths and overly broad allowed commands (e.g., entries like "Bash(git
-C /Users/vieira.neto/vieirin/goal-controller ...)" and "Bash(find:*)",
"Bash(pnpm test:*)"); remove or replace these user-specific entries with
repo-safe, generic defaults (e.g., non-absolute git commands and constrained
command patterns), move any personal overrides into an ignored local file, and
add .claude/settings.local.json to .gitignore (or rename the tracked file to
.claude/settings.json with sanitized contents) so personal paths/permissions are
not committed for all contributors.

In `@packages/goal-tree/package.json`:
- Around line 1-24: Update the lodash dependency in
packages/goal-tree/package.json by changing the "lodash" entry under
"dependencies" from ^4.17.21 to ^4.17.23, then regenerate the lockfile
(npm/yarn/pnpm install) so the updated version is pinned; ensure the change is
committed before publishing and run your test/build scripts (scripts:
build/watch/clean) to verify no regressions.

In `@packages/goal-tree/src/antlr/AssertionRegexLexer.ts`:
- Around line 1-14: The generated ANTLR lexer AssertionRegexLexer.ts is not
covered by CI regeneration; add a clear regeneration step and docs: update
project CONTRIBUTING or README to document that
packages/lib/grammar/AssertionRegex.g4 is the source and must be regenerated
with ANTLR 4.13.2, add an npm script (e.g., "generate:antlr") that runs the
ANTLR TypeScript generation for AssertionRegex.g4, and add a CI job that runs
this script and fails the build if generated files (like AssertionRegexLexer.ts)
are out of date; reference the generated symbol AssertionRegexLexer and the
grammar file AssertionRegex.g4 in the docs and CI job to make the relationship
explicit.

In `@packages/goal-tree/tsconfig.tsbuildinfo`:
- Line 1: The committed file packages/goal-tree/tsconfig.tsbuildinfo is a
generated TypeScript build artifact and must be removed from version control and
ignored; delete packages/goal-tree/tsconfig.tsbuildinfo from the repo, add an
entry to the repository .gitignore (e.g., tsconfig.tsbuildinfo or
packages/goal-tree/tsconfig.tsbuildinfo) so it isn’t committed again, and commit
the .gitignore change plus the file deletion (use git rm --cached if already
tracked) to stop machine-specific build info from leaking into the repo.

In `@packages/lib/.mocharc.json`:
- Around line 10-19: The .mocharc.json currently mixes CommonJS and ESM configs:
remove the "node-option": ["loader=ts-node/esm"] and the "ts-node" section so
the file relies on the existing "require": ["ts-node/register"] (CommonJS), or
if you intend to move to ESM instead, remove "require": ["ts-node/register"] and
add "type": "module" to package.json and keep the ESM loader; update whichever
path you choose so the config is consistent (reference: "node-option", "ts-node"
entries in .mocharc.json and "require": ["ts-node/register"] and "type":
"module" in package.json).

In `@packages/lib/package.json`:
- Line 14: The test script currently disables strict type checking by setting
TS_NODE_COMPILER_OPTIONS with "strict":false and "noImplicitAny":false; remove
these overrides from the "test" npm script in package.json and instead fix the
failing test typings: run the test suite with the default/strict TS settings,
then update test files (correct any incorrect types, add explicit annotations,
fix missing imports or casts) so that functions and test helpers referenced in
tests (e.g., any custom helpers or mocked functions in the test files) satisfy
TypeScript's strict and noImplicitAny rules; once tests compile under strict
mode, restore the standard test script without TS_NODE_COMPILER_OPTIONS.

In `@packages/lib/test/tsconfig.json`:
- Around line 1-8: test/tsconfig.json is dead/unused duplicate config; either
delete this file or wire it into the Mocha/ts-node setup by referencing it from
.mocharc.json via TS_NODE_PROJECT (or remove TS_NODE_COMPILER_OPTIONS from
package.json and centralize options). Locate the test/tsconfig.json and
.mocharc.json and choose one: if keeping the tsconfig, update .mocharc.json to
set "ts-node": { "project": "test/tsconfig.json" } (or set TS_NODE_PROJECT
accordingly in package.json); otherwise remove test/tsconfig.json and ensure
tests rely only on the existing TS_NODE_COMPILER_OPTIONS settings already used
by .mocharc.json and package.json.

🧹 Nitpick comments (8)

packages/lib/src/templateEngine/decisionVariables.ts (1)

17-25: Remove redundant type annotations and rely on TypeScript inference.

The inline type { variable: string; space: number } is duplicated in lines 17-23. Since GoalNode.decisionVars is already typed as Array<{ variable: string; space: number }> in the Decision type, you can use shorthand destructuring to let TypeScript infer the parameter types.

♻️ Proposed refactor

-  const spaceArray = goal.decisionVars.map(
-    (decision: { variable: string; space: number }) =>
-      Array.from({ length: decision.space }, (_, i) => i),
-  );
-  const variableArray = goal.decisionVars.map(
-    (decision: { variable: string; space: number }) => decision.variable,
-  );
+  const spaceArray = goal.decisionVars.map(({ space }) =>
+    Array.from({ length: space }, (_, i) => i),
+  );
+  const variableArray = goal.decisionVars.map(({ variable }) => variable);

packages/goal-tree/src/treeVariables.ts (1)

4-6: Code duplication with packages/lib/src/templateEngine/common.ts.

This helper is now duplicated in both packages. While this is reasonable to avoid circular dependencies between goal-tree and lib, consider documenting this coupling or extracting shared utilities to a common constants package if the naming convention changes in the future.

packages/lib/src/templateEngine/modules/changeManager/changeManager.ts (1)

3-5: Consider consolidating imports from the same module.

Multiple import statements from @goal-controller/goal-tree can be combined for cleaner code.

♻️ Suggested consolidation

-import { getTaskAchievabilityVariables } from '@goal-controller/goal-tree';
-import type { GoalTree } from '@goal-controller/goal-tree';
-import { allByType } from '@goal-controller/goal-tree';
+import {
+  allByType,
+  getTaskAchievabilityVariables,
+  type GoalTree,
+} from '@goal-controller/goal-tree';

packages/lib/src/templateEngine/modules/goalModule/template/index.ts (1)

5-6: Consider consolidating imports from the same module.

Similar to other files, these can be combined.

♻️ Suggested consolidation

-import type { GoalNode } from '@goal-controller/goal-tree';
-import { childrenIncludingTasks } from '@goal-controller/goal-tree';
+import {
+  childrenIncludingTasks,
+  type GoalNode,
+} from '@goal-controller/goal-tree';

packages/lib/src/templateEngine/modules/goalModule/template/pursue/index.ts (1)

1-2: Consider consolidating imports from the same module.

♻️ Suggested consolidation

-import type { GoalNode } from '@goal-controller/goal-tree';
-import { childrenIncludingTasks } from '@goal-controller/goal-tree';
+import {
+  childrenIncludingTasks,
+  type GoalNode,
+} from '@goal-controller/goal-tree';

packages/lib/src/prismValidator/expectedElements.ts (1)

92-94: Consider using library-provided types instead of inline type annotations.

The inline type { name: string } works but may drift from the actual type definition in @goal-controller/goal-tree. If the library exports a Variable type or similar, importing and using that would provide stronger type safety and automatic updates if the type changes.

packages/goal-tree/src/antlr/RTRegexParser.ts (1)

1-17: Auto-generated ANTLR parser file.

This file is generated from grammar/RTRegex.g4 by ANTLR 4.13.2. As auto-generated code, it should not be manually modified. Consider adding a build step to regenerate these files from the grammar, and potentially adding the generated files to .gitignore to avoid committing generated code.

packages/lib/src/cli.ts (1)

15-17: Consider consolidating imports from the same module.

These three separate imports from @goal-controller/goal-tree can be combined into a single statement for cleaner code.

♻️ Suggested consolidation

-import { loadPistarModel } from '@goal-controller/goal-tree';
-import { convertToTree } from '@goal-controller/goal-tree';
-import { dumpTreeToJSON } from '@goal-controller/goal-tree';
+import {
+  loadPistarModel,
+  convertToTree,
+  dumpTreeToJSON,
+} from '@goal-controller/goal-tree';

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Remove user-specific/local permissions from the repo.

This file now embeds absolute, user-specific paths and broadens the allow-list for commands in a shared config. That’s non-portable and weakens safety defaults for other contributors. Please keep local overrides untracked (or move shared defaults to a sanitized file and ignore the local one).

✅ Suggested direction

-  "Bash(git -C /Users/vieira.neto/vieirin/goal-controller status)",
-  "Bash(git -C /Users/vieira.neto/vieirin/goal-controller log --oneline -20 -- packages/lib/src/GoalTree/)",
-  "Bash(git -C /Users/vieira.neto/vieirin/goal-controller log --oneline -10)",
-  "Bash(find:*)",
-  "Bash(pnpm run build:goal-tree:*)",
-  "Bash(tsc:*)",
-  "Bash(pnpm exec tsc --help:*)",
-  "Bash(pnpm exec tsc:*)",
-  "Bash(pnpm run build:ui:*)",
-  "Bash(pnpm test:*)"

And ensure .claude/settings.local.json is gitignored (or replace with a repo-safe .claude/settings.json).

🤖 Prompt for AI Agents

In @.claude/settings.local.json around lines 13 - 23, The
.claude/settings.local.json contains user-specific absolute paths and overly
broad allowed commands (e.g., entries like "Bash(git -C
/Users/vieira.neto/vieirin/goal-controller ...)" and "Bash(find:*)", "Bash(pnpm
test:*)"); remove or replace these user-specific entries with repo-safe, generic
defaults (e.g., non-absolute git commands and constrained command patterns),
move any personal overrides into an ignored local file, and add
.claude/settings.local.json to .gitignore (or rename the tracked file to
.claude/settings.json with sanitized contents) so personal paths/permissions are
not committed for all contributors.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

latest npm versions antlr4 lodash 2025 2026

💡 Result:

• antlr4 (npm package antlr4): 4.13.2 (latest) [2][3]
• lodash (npm package lodash): 4.17.23 (latest) [1]

Sources: npm registry “latest” for lodash [1], npm package pages / ANTLR site for antlr4 [2][3].

---

🌐 Web query:

Node.js 22.6.0 LTS release date status

💡 Result:

• Node.js v22.6.0 release date: 2024-08-06 — released as Current (not LTS). [1]
• When v22 became LTS: 2024-10-29 (the v22.11.0 release marked the LTS transition to “Active LTS”). [2]
• Current status of the v22 line (as of 2026-01-28): Maintenance LTS (v22 “Jod”). [3]

Sources: [1] [2] [3]

---

🌐 Web query:

antlr4 npm security advisories vulnerabilities

💡 Result:

• antlr4 (npm package): As of Jan 28, 2026, there are no published security advisories in the upstream GitHub repo’s Security tab, and no known direct vulnerabilities reported by Snyk for the latest npm version (4.13.2). [1][2][3]
• Watch for name-squatting / malicious lookalikes: OSV lists a malicious npm package named antlr4-graps (not antlr4). Don’t confuse it with the official antlr4 runtime. [4]

Sources: [1] GitHub Security overview (antlr/antlr4) • [2] Snyk vulns page for antlr4 • [3] npm package page for antlr4 • [4] OSV entry for antlr4-graps

---

🌐 Web query:

lodash npm security advisories vulnerabilities 2025

💡 Result:

Lodash (npm) security advisories / vulnerabilities relevant to 2025

As of calendar year 2025, there weren’t widely reported new lodash CVEs disclosed that newly affected the current maintained lodash line; most activity in 2025 was updates/metadata refreshes to older advisories (e.g., GitHub advisory entries updated in 2025). For example, the GitHub advisory for CVE-2020-28500 (ReDoS) shows an update date in Sep 2025, but the issue itself is older and is fixed by upgrading to lodash ≥ 4.17.21. [1]

The “2025” CVE you’ll see in tooling (but published in 2026)

Many scanners will surface CVE-2025-13465 (alias GHSA-xxjr-mmjv-4gpg) for lodash:

• Issue: Prototype pollution via _.unset / _.omit enabling deletion of properties from global prototypes (deletion, not overwriting).
• Affected: lodash 4.0.0 – 4.17.22
• Fixed: lodash 4.17.23
• Published: Jan 21–23, 2026 (despite the CVE number being “2025”).
  Sources: OSV/GitHub advisory record and NVD description. [2][3] (also summarized in GitLab’s advisory DB) [4]

What to do (practical)

• If you want to be clear of both the historical issues and CVE-2025-13465, upgrade to lodash ≥ 4.17.23. [2][3][4]
• Current scanner views generally report no known issues affecting the latest lodash (at time of writing) when on the fixed release. [5]

Sources
[1] GitHub Advisory Database – GHSA-29mw-wpgm-hmr9 / CVE-2020-28500 (updated Sep 29, 2025) (github.com)
[2] OSV – GHSA-xxjr-mmjv-4gpg (published 2026-01-21; aliases CVE-2025-13465; fixed in 4.17.23) (osv.dev)
[3] NVD – CVE-2025-13465 (affected through 4.17.22; patched in 4.17.23) (nvd.nist.gov)
[4] GitLab Advisory DB – CVE-2025-13465 (affected <4.17.23; fixed 4.17.23) (advisories.gitlab.com)
[5] Snyk – lodash package page (latest version shows no vulnerabilities) (security.snyk.io)

Citations:

• 1: https://github.com/advisories/GHSA-29mw-wpgm-hmr9?utm_source=openai
• 2: https://osv.dev/vulnerability/GHSA-xxjr-mmjv-4gpg?utm_source=openai
• 3: https://nvd.nist.gov/vuln/detail/CVE-2025-13465?utm_source=openai
• 4: https://advisories.gitlab.com/pkg/npm/lodash/CVE-2025-13465/?utm_source=openai
• 5: https://security.snyk.io/package/npm/lodash?utm_source=openai

---

Upgrade lodash to ^4.17.23 to fix CVE-2025-13465.

The lodash dependency version ^4.17.21 is vulnerable to CVE-2025-13465 (prototype pollution via _.unset / _.omit), which affects versions 4.0.0–4.17.22 and is fixed in 4.17.23. Do not publish until this is resolved.

The antlr4 version (^4.13.0) is current and has no known security advisories. The Node.js engine floor (>=22.6.0) is appropriate.

🤖 Prompt for AI Agents

In `@packages/goal-tree/package.json` around lines 1 - 24, Update the lodash
dependency in packages/goal-tree/package.json by changing the "lodash" entry
under "dependencies" from ^4.17.21 to ^4.17.23, then regenerate the lockfile
(npm/yarn/pnpm install) so the updated version is pinned; ensure the change is
committed before publishing and run your test/build scripts (scripts:
build/watch/clean) to verify no regressions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find the grammar file referenced
fd -type f -name "AssertionRegex.g4"

Repository: vieirin/goal-controller

Length of output: 237

---

🏁 Script executed:

# Check package.json for build/generate scripts
fd -type f -name "package.json" packages/goal-tree --exec cat {} \;

Repository: vieirin/goal-controller

Length of output: 237

---

🏁 Script executed:

# Look for CI configuration files
fd -type f \( -name "*.yml" -o -name "*.yaml" -o -name ".github" \) . --max-depth 3

Repository: vieirin/goal-controller

Length of output: 237

---

🏁 Script executed:

# Check if ANTLR files are mentioned in .gitignore
find . -name ".gitignore" -type f -exec grep -H "antlr\|AssertionRegex" {} \;

Repository: vieirin/goal-controller

Length of output: 94

---

🏁 Script executed:

# Get the full content of AssertionRegexLexer.ts to check actual usage of imported symbols
wc -l packages/goal-tree/src/antlr/AssertionRegexLexer.ts

Repository: vieirin/goal-controller

Length of output: 122

---

🏁 Script executed:

# Find the grammar file
find . -name "AssertionRegex.g4" -type f

Repository: vieirin/goal-controller

Length of output: 107

---

🏁 Script executed:

# Check package.json in packages/goal-tree
cat packages/goal-tree/package.json

Repository: vieirin/goal-controller

Length of output: 644

---

🏁 Script executed:

# Read the full AssertionRegexLexer.ts file
cat -n packages/goal-tree/src/antlr/AssertionRegexLexer.ts

Repository: vieirin/goal-controller

Length of output: 7291

---

🏁 Script executed:

# Look for CI configuration
find . -path "./.git" -prune -o -type f \( -name "*.yml" -o -name "*.yaml" \) -print | head -20

Repository: vieirin/goal-controller

Length of output: 210

---

🏁 Script executed:

# Check for build-related configuration
ls -la .github/workflows/ 2>/dev/null || echo "No GitHub workflows found"

Repository: vieirin/goal-controller

Length of output: 301

---

🏁 Script executed:

cat .github/workflows/test.yaml

Repository: vieirin/goal-controller

Length of output: 667

---

🏁 Script executed:

cat .github/workflows/prism-test.yaml

Repository: vieirin/goal-controller

Length of output: 1308

---

🏁 Script executed:

# Check if there's a root-level script or documentation for ANTLR regeneration
find . -maxdepth 2 -type f \( -name "Makefile" -o -name "*.md" \) -exec grep -l -i "antlr" {} \;

Repository: vieirin/goal-controller

Length of output: 96

---

ANTLR-generated lexer file — add regeneration documentation.

This is auto-generated code from packages/lib/grammar/AssertionRegex.g4 by ANTLR 4.13.2. The structure follows the standard ANTLR TypeScript lexer pattern.

Note: The noinspection comment on line 2 correctly suppresses IDE warnings for the imports. While Token is actually used on line 31 (Token.EOF), the RuleContext import is genuinely unused but remains typical in ANTLR-generated files. Do not manually edit this file.

⚠️ Action needed: The current CI/build pipeline does not include an ANTLR regeneration step. Add documentation or a build task that regenerates these files when the grammar changes to prevent drift between AssertionRegex.g4 and the generated lexer.

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/antlr/AssertionRegexLexer.ts` around lines 1 - 14, The
generated ANTLR lexer AssertionRegexLexer.ts is not covered by CI regeneration;
add a clear regeneration step and docs: update project CONTRIBUTING or README to
document that packages/lib/grammar/AssertionRegex.g4 is the source and must be
regenerated with ANTLR 4.13.2, add an npm script (e.g., "generate:antlr") that
runs the ANTLR TypeScript generation for AssertionRegex.g4, and add a CI job
that runs this script and fails the build if generated files (like
AssertionRegexLexer.ts) are out of date; reference the generated symbol
AssertionRegexLexer and the grammar file AssertionRegex.g4 in the docs and CI
job to make the relationship explicit.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Avoid committing TypeScript build artifacts.

tsconfig.tsbuildinfo is generated output and should be ignored to prevent repo churn and machine-specific paths leaking into version control.

🧹 Suggested cleanup

# .gitignore
+packages/goal-tree/tsconfig.tsbuildinfo
+packages/**/tsconfig.tsbuildinfo

Also remove the committed packages/goal-tree/tsconfig.tsbuildinfo file from the repo.

🤖 Prompt for AI Agents

In `@packages/goal-tree/tsconfig.tsbuildinfo` at line 1, The committed file
packages/goal-tree/tsconfig.tsbuildinfo is a generated TypeScript build artifact
and must be removed from version control and ignored; delete
packages/goal-tree/tsconfig.tsbuildinfo from the repo, add an entry to the
repository .gitignore (e.g., tsconfig.tsbuildinfo or
packages/goal-tree/tsconfig.tsbuildinfo) so it isn’t committed again, and commit
the .gitignore change plus the file deletion (use git rm --cached if already
tracked) to stop machine-specific build info from leaking into the repo.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the full .mocharc.json file to see if ts-node/register is actually being used
cat packages/lib/.mocharc.json

Repository: vieirin/goal-controller

Length of output: 422

---

🏁 Script executed:

#!/bin/bash
# Check package.json for mocha, ts-node versions and module type
cd packages/lib && cat package.json | jq -r '.dependencies, .devDependencies, .type'

Repository: vieirin/goal-controller

Length of output: 1023

---

🏁 Script executed:

#!/bin/bash
# Also check root package.json for versions and type
cat package.json | jq -r '.dependencies, .devDependencies, .type' | head -50

Repository: vieirin/goal-controller

Length of output: 157

---

Remove the ESM loader configuration; it conflicts with ts-node/register.

The .mocharc.json mixes "require": ["ts-node/register"] (CommonJS) with "node-option": ["loader=ts-node/esm"] (ESM). Since package.json does not specify "type": "module", the project is CommonJS-based, and the ESM loader is inappropriate. Keep ts-node/register and remove the "node-option" and "ts-node" entries, or if ESM support is intended, set "type": "module" in package.json and remove ts-node/register.

🤖 Prompt for AI Agents

In `@packages/lib/.mocharc.json` around lines 10 - 19, The .mocharc.json currently
mixes CommonJS and ESM configs: remove the "node-option": ["loader=ts-node/esm"]
and the "ts-node" section so the file relies on the existing "require":
["ts-node/register"] (CommonJS), or if you intend to move to ESM instead, remove
"require": ["ts-node/register"] and add "type": "module" to package.json and
keep the ESM loader; update whichever path you choose so the config is
consistent (reference: "node-option", "ts-node" entries in .mocharc.json and
"require": ["ts-node/register"] and "type": "module" in package.json).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Disabling strict type checking in tests masks potential issues.

Setting "strict":false,"noImplicitAny":false in test compilation options defeats the purpose of adding explicit type annotations elsewhere in this PR. This approach hides type errors rather than fixing them, which can lead to runtime issues being caught late.

Consider fixing the underlying type issues in the test files instead of loosening the compiler options.

🤖 Prompt for AI Agents

In `@packages/lib/package.json` at line 14, The test script currently disables
strict type checking by setting TS_NODE_COMPILER_OPTIONS with "strict":false and
"noImplicitAny":false; remove these overrides from the "test" npm script in
package.json and instead fix the failing test typings: run the test suite with
the default/strict TS settings, then update test files (correct any incorrect
types, add explicit annotations, fix missing imports or casts) so that functions
and test helpers referenced in tests (e.g., any custom helpers or mocked
functions in the test files) satisfy TypeScript's strict and noImplicitAny
rules; once tests compile under strict mode, restore the standard test script
without TS_NODE_COMPILER_OPTIONS.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look for references to the test tsconfig and ts-node configuration
echo "=== Searching for TS_NODE_PROJECT and test/tsconfig.json references ==="
rg "TS_NODE_PROJECT|ts-node\.project|test/tsconfig\.json" --max-count 20

echo ""
echo "=== Check for package.json test script ==="
if [ -f "package.json" ]; then
  cat package.json | head -50
fi

echo ""
echo "=== Look for mocha config files ==="
fd -e mocharc.json -e mocharc.cjs -o .mocharc.json -o .mocharc.cjs

Repository: vieirin/goal-controller

Length of output: 1924

---

🏁 Script executed:

#!/bin/bash
echo "=== packages/lib/package.json test script ==="
if [ -f "packages/lib/package.json" ]; then
  cat packages/lib/package.json | grep -A 30 '"scripts"'
fi

echo ""
echo "=== Look for mocha config files in packages/lib ==="
find packages/lib -maxdepth 2 -name ".mocharc.*" -o -name "mocha.opts" -o -name ".mocharc.json" -o -name ".mocharc.cjs" -o -name ".mocharc.js" | head -20

echo ""
echo "=== Search for ts-node or typescript test config ==="
rg "ts-node|ts-loader|tsx" packages/lib --max-count 20 -A 3

echo ""
echo "=== Check if test/tsconfig.json is referenced in packages/lib ==="
rg "test/tsconfig" packages/lib/ --max-count 20
</parameter>

Repository: vieirin/goal-controller

Length of output: 1676

---

🏁 Script executed:

#!/bin/bash
echo "=== Content of packages/lib/.mocharc.json ==="
cat packages/lib/.mocharc.json

echo ""
echo "=== Verify test/tsconfig.json exists and content ==="
cat packages/lib/test/tsconfig.json

Repository: vieirin/goal-controller

Length of output: 675

---

Remove or wire the test tsconfig—it's currently unused dead code.

The test configuration is applied via .mocharc.json's ts-node.compilerOptions and the TS_NODE_COMPILER_OPTIONS environment variable in package.json, not through test/tsconfig.json. Since test/tsconfig.json is never referenced (no TS_NODE_PROJECT in mocha config), it duplicates settings already defined elsewhere and should either be removed or integrated by having .mocharc.json reference it via TS_NODE_PROJECT instead.

🤖 Prompt for AI Agents

In `@packages/lib/test/tsconfig.json` around lines 1 - 8, test/tsconfig.json is
dead/unused duplicate config; either delete this file or wire it into the
Mocha/ts-node setup by referencing it from .mocharc.json via TS_NODE_PROJECT (or
remove TS_NODE_COMPILER_OPTIONS from package.json and centralize options).
Locate the test/tsconfig.json and .mocharc.json and choose one: if keeping the
tsconfig, update .mocharc.json to set "ts-node": { "project":
"test/tsconfig.json" } (or set TS_NODE_PROJECT accordingly in package.json);
otherwise remove test/tsconfig.json and ensure tests rely only on the existing
TS_NODE_COMPILER_OPTIONS settings already used by .mocharc.json and
package.json.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@packages/lib/src/templateEngine/modules/goalModule/template/pursue/index.ts`:
- Around line 283-292: The access to execCondition.assertion.sentence in the
activationContextCondition computation can throw if execCondition exists but
assertion is missing; update the expression that builds
activationContextCondition (which references isItself(child) and
child.properties.edge.execCondition.assertion.sentence) to use optional chaining
(e.g., child.properties.edge.execCondition?.assertion?.sentence) so it safely
falls back to '' when assertion or sentence is absent; leave the
maintainContextGuard logic as-is since it already uses optional chaining.
- Around line 306-309: The activation-context log call is passing the
state-update variable maintainContextGuard instead of the actual activation
context sentence; change the argument to the activation context (the sentence
derived from the edge, e.g., the value tied to
child.properties.edge.execCondition / the activation context variable created
earlier) when calling pursueLogger.executionDetail.activationContext so logs
show the activation context text rather than the state update. Locate the block
with child.properties.edge.execCondition, logger.trace(child.id, ...) and
replace the maintainContextGuard argument to
pursueLogger.executionDetail.activationContext(...) with the activation context
sentence variable used in that scope.

🧹 Nitpick comments (3)

.gitignore (1)

41-42: Consider removing duplicate ignore entries.

Lines 41-42 duplicate entries already present at lines 7-8. While this doesn't cause functional issues, consolidating these would improve maintainability.

♻️ Suggested cleanup

-packages/ui/.next/
-packages/lib/out/
 packages/lib/tsconfig.tsbuildinfo
 packages/goal-tree/tsconfig.tsbuildinfo

packages/lib/src/prismValidator/expectedElements.ts (1)

91-96: Consider extracting the variable type annotation.

The inline type annotation (v: { name: string }) is repeated in multiple places (lines 93, 121, 128, 175, 182). Consider defining a type alias to improve maintainability.

♻️ Suggested refactor

Define a type alias at the top of the file or import it from the goal-tree package:

type AssertionVariable = { name: string };

Then use it in the forEach callbacks:

-      (v: { name: string }) => {
+      (v: AssertionVariable) => {

packages/lib/src/templateEngine/modules/system/system.ts (1)

118-133: Duplication with expectedElements.ts — consider extracting shared logic.

The task context variable extraction logic (lines 118-133) is nearly identical to the code in packages/lib/src/prismValidator/expectedElements.ts (lines 172-186). Both iterate over tasks, check for assertion and maintain variables, and collect variable names.

This could be extracted into a shared utility function in the goal-tree package or a common lib utility to reduce duplication.

♻️ Potential shared utility

// Could be added to goal-tree or a shared utility file
export const collectTaskContextVariables = (tasks: GoalNode[]): Set<string> => {
  const taskContextVariables = new Set<string>();
  tasks.forEach((task: GoalNode) => {
    if (task.properties.edge.execCondition?.assertion) {
      task.properties.edge.execCondition.assertion.variables.forEach(
        (v: { name: string }) => taskContextVariables.add(v.name)
      );
    }
    if (task.properties.edge.execCondition?.maintain?.variables) {
      task.properties.edge.execCondition.maintain.variables.forEach(
        (v: { name: string }) => taskContextVariables.add(v.name)
      );
    }
  });
  return taskContextVariables;
};

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

packages/lib/src/templateEngine/modules/goalModule/template/achieve.ts (1)

17-29: Avoid returning () when all children are resources.
If children contains only resources, the filter yields an empty list and the function returns (). That can produce an invalid condition string. Consider checking the filtered list before formatting.

🔧 Proposed fix

   if (isValidSeparator(goal.relationToChildren)) {
     const children = childrenIncludingTasks({ node: goal });
-    if (children.length) {
-      return `(${children
-        .filter(
-          (child): child is Exclude<TreeNode, { type: 'resource' }> =>
-            !isResource(child),
-        )
-        .map((child) =>
-          child.properties.edge.execCondition?.maintain
-            ? `${achievedMaintain(child.id)}=true`
-            : `${achievedVariable(child.id)}=1`,
-        )
-        .join(separator(goal.relationToChildren))})`;
-    }
+    const pursueableChildren = children.filter(
+      (child): child is Exclude<TreeNode, { type: 'resource' }> =>
+        !isResource(child),
+    );
+    if (pursueableChildren.length) {
+      return `(${pursueableChildren
+        .map((child) =>
+          child.properties.edge.execCondition?.maintain
+            ? `${achievedMaintain(child.id)}=true`
+            : `${achievedVariable(child.id)}=1`,
+        )
+        .join(separator(goal.relationToChildren))})`;
+    }
   }

packages/lib/src/sleecTemplateEngine/rules.ts (1)

4-34: Guard against missing sleec properties to prevent "undefined" in rules.

The optional chaining operator injects undefined into template strings when properties are missing. Since task.properties.sleec is optional (TaskSleecProps), tasks reaching generateTaskRules() without SLEEC data will generate invalid rules like:

Rule[id]_1 when undefined and undefined then ...

This occurs because there is no upstream filtering—allByType() returns all tasks regardless of SLEEC presence. Add a check at the start of the .map() callback to either skip tasks without SLEEC or throw an error, similar to the validation pattern in purposes.ts.

🛠️ Suggested guard

 export const generateTaskRules = (tasks: Task[]): string => {
   const hasObstacleEvents = (task: Task) =>
     !!task.properties.sleec?.ObstacleEvent;
   return `rule_start
       ${tasks
         .map((task) => {
+          const sleec = task.properties.sleec;
+          if (!sleec) {
+            throw new Error(`Task "${task.name}" (${task.id}) lacks required SLEEC properties`);
+          }
           return `
-      Rule${renameTaskId(task.id)}_1 when ${
-        task.properties.sleec?.TriggeringEvent
-      } and ${task.properties.sleec?.PreCond} then ${taskFluentName(task, 'Start')}
+      Rule${renameTaskId(task.id)}_1 when ${
+        sleec.TriggeringEvent
+      } and ${sleec.PreCond} then ${taskFluentName(task, 'Start')}
       Rule${renameTaskId(task.id)}_2 when ${taskFluentName(
         task,
         'Start',
       )} then ${taskFluentName(task, 'Pursuing')} within ${
-        task.properties.sleec?.TemporalConstraint
+        sleec.TemporalConstraint
       }
       Rule${renameTaskId(task.id)}_3 when ${taskFluentName(
         task,
         'Pursuing',
-      )} and ${task.properties.sleec?.PostCond} then ${taskFluentName(
+      )} and ${sleec.PostCond} then ${taskFluentName(
         task,
         'Achieved',
-      )} unless (not ${task.properties.sleec?.PostCond}) then ${taskFluentName(
+      )} unless (not ${sleec.PostCond}) then ${taskFluentName(
         task,
         'ReportFailure',
       )} 
       ${hasObstacleEvents(task) ? generateObstacleEventsRules(task) : ''}`;
         })
         .join('\n')}

🤖 Fix all issues with AI agents

In `@packages/goal-tree/src/creation.ts`:
- Around line 529-537: The code in resolvedDependencies maps
node.properties.edge.dependsOn and unconditionally casts depNode from nodeMap to
GoalNode, which is unsafe because depNode may be a Task or Resource; update the
mapping to validate the runtime type before casting: use a type guard or
discriminator on depNode (e.g., check depNode.type or instanceof checks) to
accept only GoalNode when building a goal's resolvedDependencies, throw a clear
error if the referenced id is not a GoalNode, and adjust any downstream
consumers if they must handle TaskNode/ResourceNode instead; ensure you
reference the resolvedDependencies variable, the nodeMap lookup, the dependsOn
property, and GoalNode type in your changes.

In `@packages/goal-tree/src/types/istar.ts`:
- Around line 15-41: The CustomProperties type currently intersects a `{ type:
never }` with `{ type: 'maintain'; maintain: string; assertion: string }`,
making `type` impossible to satisfy; change it to a proper discriminated union:
define the shared/common structure (the customProperties object keys like
Description, cost, etc.) as a base type without `type: never`, then create a
union of variants (e.g., one variant with `type: 'maintain'` that includes
`maintain` and `assertion` and another variant for the default/other types that
omits those fields or uses optional fields). Update `CustomProperties` to be
that union so the runtime check in creation.ts (`if (customProperties.type ===
'maintain')`) typesafe-ly narrows to the variant exposing `maintain` and
`assertion`.

In `@packages/lib/src/logger/logger.ts`:
- Around line 216-218: The log line calls .join() on the result of
goal.tasks?.map(...), which can be undefined and throws; change the expression
so .join() always runs on an array—e.g., use a safe fallback like (goal.tasks ??
[]).map((task: Task) => task.id).join(', ') or a conditional check goal.tasks &&
goal.tasks.length ? goal.tasks.map((task: Task) => task.id).join(', ') : 'none'
in the write call to ensure goals without tasks do not cause an exception; refer
to the existing write call and the Task type to implement the fix.

In `@packages/lib/src/sleecTemplateEngine/definitions.ts`:
- Around line 158-163: The extractObstacleEvents function is producing entries
like "Achievedundefined" because it prefixes "Achieved" before filtering out
missing sleec.ObstacleEvent values; update extractObstacleEvents to only
map/format after verifying task.properties.sleec?.ObstacleEvent is present
(e.g., filter tasks with a defined ObstacleEvent first or use a flatMap that
returns [] for missing values) so the Set is built from actual event strings;
key symbols: extractObstacleEvents, Task, and
task.properties.sleec?.ObstacleEvent.

In `@packages/lib/src/templateEngine/modules/goalModule/template/pursue/index.ts`:
- Around line 29-35: The dependency guard in goalDependencyStatement currently
reads legacy goal.dependsOn; update it to read the new path
(goal.properties.edge.dependsOn) and guard for existence/length there, and when
mapping use the resolved GoalNode objects (or resolve IDs to GoalNode if
dependsOn now contains IDs) before calling hasBeenAchieved; keep using
separator('and') and ensure the function still returns an empty string when no
dependencies exist.

In `@packages/lib/test/sleecTemplateEngine/purposes.test.ts`:
- Around line 3-26: The test helper createGoalNode is missing required GoalNode
properties used at runtime (properties.edge and properties.isQuality); update
createGoalNode to populate properties.edge with a sensible minimal default
(e.g., null or an empty object consistent with GoalNode.edge shape) and
properties.isQuality with a boolean default (false) so fixtures match the
runtime shape expected by generatePurposes and other GoalTree consumers; ensure
the defaults are set on the returned object's properties field alongside sleec
to avoid undefined access in tests.

🧹 Nitpick comments (7)

packages/lib/src/templateEngine/modules/goalModule/template/skip.ts (1)

1-1: Unused Task import.

The Task type is imported but not used anywhere in this file. Consider removing it.

-import type { GoalNode, Task, TreeNode } from '@goal-controller/goal-tree';
+import type { GoalNode, TreeNode } from '@goal-controller/goal-tree';

packages/lib/src/templateEngine/modules/system/system.ts (1)

117-132: Remove redundant type annotations where TypeScript can infer them.

The explicit type annotations are unnecessary because:

• allByType({ gm, type: 'task' }) already narrows the return type to Task[] through function overloads
• ExecCondition properties already define variables as Array<{ name: string; value: boolean | null }>

This pattern is already used elsewhere in the codebase (see treeVariables.ts lines 11-27) without explicit annotations. Simplify to:

-  tasks.forEach((task: Task) => {
+  tasks.forEach((task) => {
     if (task.properties.edge.execCondition?.assertion) {
       task.properties.edge.execCondition.assertion.variables.forEach(
-        (v: { name: string }) => {
+        (v) => {
           taskContextVariables.add(v.name);
         },
       );
     }
     if (task.properties.edge.execCondition?.maintain?.variables) {
       task.properties.edge.execCondition.maintain.variables.forEach(
-        (v: { name: string }) => {
+        (v) => {
           taskContextVariables.add(v.name);
         },
       );
     }
   });

packages/lib/src/templateEngine/modules/changeManager/template/tasks/variables.ts (1)

1-34: Guard against undefined maxRetries when building ranges.

If a task lacks edge.maxRetries, the current output can become [0..undefined]. A defensive default keeps the generated PRISM valid.

♻️ Proposed hardening

-export const maxRetriesVariable = (task: Task): string => {
-  const maxRetries = task.properties.edge.maxRetries;
-  if (maxRetries === 0) {
+export const maxRetriesVariable = (task: Task): string => {
+  const maxRetries = task.properties.edge.maxRetries ?? 0;
+  if (maxRetries <= 0) {
     return '';
   }

packages/goal-tree/src/creation.ts (4)

73-97: Potential empty string in decision variable name.

When d[0] is undefined, the fallback ?? '' will produce an empty variable name. This could silently create invalid decision entries.

Consider adding validation:

🔧 Suggested improvement

   return parsedDecision.map((d) => ({
-    variable: d[0]?.trim() ?? '',
+    variable: d[0]?.trim() || (() => { throw new Error(`[INVALID DECISION] variable name is empty in: ${decision}`); })(),
     space: parseInt(d[1] ?? ''),
   }));

---

178-179: Consider specifying radix for parseInt calls.

Multiple parseInt calls lack an explicit radix parameter. While modern JavaScript defaults to base 10 for most inputs, specifying radix 10 explicitly is a best practice for clarity and to avoid edge cases with unusual input formats.

♻️ Suggested fix

-      const lowerBoundInt = parseInt(lowerBound);
-      const upperBoundInt = parseInt(upperBound);
+      const lowerBoundInt = parseInt(lowerBound, 10);
+      const upperBoundInt = parseInt(upperBound, 10);

-      const initialValueInt = parseInt(initialValue);
+      const initialValueInt = parseInt(initialValue, 10);

-          maxRetries: maxRetries ? parseInt(maxRetries) : 0,
+          maxRetries: maxRetries ? parseInt(maxRetries, 10) : 0,

Also applies to: 193-193, 364-364, 396-396

---

305-306: Variable shadowing: sleec is redeclared inside the task block.

The outer sleec (line 305-306, from extractSleecProps) is computed but unused when nodeType === 'task'. Inside the task block (line 327), a new sleec variable with a different structure shadows it. This works but is confusing.

Consider renaming one of the variables for clarity:

♻️ Suggested refactor

   // Extract SLEEC properties if present
-  const sleec = extractSleecProps(customProperties);
+  const goalSleec = extractSleecProps(customProperties);

   if (nodeType === 'resource') {
     // ...unchanged
   }

   if (nodeType === 'task') {
-    const sleec =
+    const taskSleec =
       customProperties.PreCond ||
       // ... rest unchanged
-          }
-        : undefined;
+          }
+        : undefined;

     const taskNode: Task = {
       // ...
       properties: {
         edge: { /* ... */ },
-        ...(sleec && { sleec }),
+        ...(taskSleec && { sleec: taskSleec }),
       },
     };
     return taskNode;
   }

   if (nodeType === 'goal') {
     const goalNode: GoalNode = {
       // ...
       properties: {
         // ...
-        ...(sleec && { sleec }),
+        ...(goalSleec && { sleec: goalSleec }),
       },
       // ...
     };
     return goalNode;
   }

Also applies to: 326-350

---

540-542: Type assertion as GoalNode[] | undefined may mask type errors.

The resolveNodeDependencies recursively processes children, but the assertion assumes all children are GoalNode. If node.children contains other types in the future, this could silently produce incorrect types.

Consider using a type guard or validating the result:

♻️ Alternative approach

-    const resolvedChildren = node.children?.map(resolveNodeDependencies) as
-      | GoalNode[]
-      | undefined;
+    const resolvedChildren = node.children?.map((child) => {
+      const resolved = resolveNodeDependencies(child);
+      if (resolved.type !== 'goal') {
+        throw new Error(`[INVALID MODEL]: Child ${resolved.id} is not a goal`);
+      }
+      return resolved;
+    });

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Unsafe type assertion: dependency could be a Task or Resource.

The nodeMap contains all node types (goals, tasks, resources), but line 536 casts depNode to GoalNode without validation. If a goal's dependsOn references a task or resource ID, this would produce an incorrect type at runtime.

Consider adding a type guard:

🔧 Suggested fix

     const resolvedDependencies = node.properties.edge.dependsOn.map((depId) => {
       const depNode = nodeMap.get(depId);
       if (!depNode) {
         throw new Error(
           `[INVALID MODEL]: Dependency ${depId} not found for node ${node.id}`,
         );
       }
-      return depNode as GoalNode;
+      if (depNode.type !== 'goal') {
+        throw new Error(
+          `[INVALID MODEL]: Dependency ${depId} for node ${node.id} must be a goal, but found ${depNode.type}`,
+        );
+      }
+      return depNode;
     });

📝 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 resolvedDependencies = node.properties.edge.dependsOn.map((depId) => {
	const depNode = nodeMap.get(depId);
	if (!depNode) {
	throw new Error(
	`[INVALID MODEL]: Dependency ${depId} not found for node ${node.id}`,
	);
	}
	return depNode;
	return depNode as GoalNode;
	});
	const resolvedDependencies = node.properties.edge.dependsOn.map((depId) => {
	const depNode = nodeMap.get(depId);
	if (!depNode) {
	throw new Error(
	`[INVALID MODEL]: Dependency ${depId} not found for node ${node.id}`,
	);
	}
	if (depNode.type !== 'goal') {
	throw new Error(
	`[INVALID MODEL]: Dependency ${depId} for node ${node.id} must be a goal, but found ${depNode.type}`,
	);
	}
	return depNode;
	});

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/creation.ts` around lines 529 - 537, The code in
resolvedDependencies maps node.properties.edge.dependsOn and unconditionally
casts depNode from nodeMap to GoalNode, which is unsafe because depNode may be a
Task or Resource; update the mapping to validate the runtime type before
casting: use a type guard or discriminator on depNode (e.g., check depNode.type
or instanceof checks) to accept only GoalNode when building a goal's
resolvedDependencies, throw a clear error if the referenced id is not a
GoalNode, and adjust any downstream consumers if they must handle
TaskNode/ResourceNode instead; ensure you reference the resolvedDependencies
variable, the nodeMap lookup, the dependsOn property, and GoalNode type in your
changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n packages/goal-tree/src/types/istar.ts | sed -n '1,50p'

Repository: vieirin/goal-controller

Length of output: 1426

---

🏁 Script executed:

rg "CustomProperties|customProperties" packages/goal-tree/src --type ts --type tsx -A 2 -B 2 | head -100

Repository: vieirin/goal-controller

Length of output: 94

---

🏁 Script executed:

rg "CustomProperties|customProperties" packages/goal-tree/src --type ts -A 2 -B 2 | head -100

Repository: vieirin/goal-controller

Length of output: 5748

---

🏁 Script executed:

# Check if there are other type definitions in istar.ts that might extend CustomProperties
cat -n packages/goal-tree/src/types/istar.ts | sed -n '40,120p'

Repository: vieirin/goal-controller

Length of output: 1248

---

🏁 Script executed:

# Check if there's a tsconfig.json to see type checking configuration
cat -n packages/goal-tree/tsconfig.json 2>/dev/null || echo "No tsconfig in goal-tree"

Repository: vieirin/goal-controller

Length of output: 436

---

🏁 Script executed:

# Let's check how Node, Actor, and Diagram types (which use CustomProperties) are actually instantiated/used
rg "new Node|new Actor|new Diagram|: Node|: Actor|: Diagram" packages/goal-tree/src --type ts -B 2 -A 2 | head -80

Repository: vieirin/goal-controller

Length of output: 2219

---

🏁 Script executed:

# Let me verify the actual type behavior by checking if there are any compile errors
# Look for the build output or check if tests pass
ls -la packages/goal-tree/

Repository: vieirin/goal-controller

Length of output: 403

---

Fix CustomProperties type intersection that makes type property impossible to satisfy.

The intersection { type: never } & { type: 'maintain' } is logically contradictory and creates an uninhabitable type. The code in creation.ts checks if (customProperties.type === 'maintain') and accesses customProperties.maintain and customProperties.assertion, but the type definition declares these properties can never exist simultaneously. Use a union-based approach to allow conditional properties:

🛠️ Proposed fix

+type MaintainCustomProperties =
+  | {
+      type: 'maintain';
+      maintain: string;
+      assertion: string;
+    }
+  | {
+      type?: never;
+      maintain?: never;
+      assertion?: never;
+    };
+
 export type CustomProperties = {
   customProperties: {
     Description: '';
     root?: string;
     cost: string;
     alt?: string;
     utility: string;
     dependsOn?: string;
     variables?: string;
     uniqueChoice?: string;
-    type: never;
     maxRetries?: string;
     selected?: string;
     PreCond?: string;
     TriggeringEvent?: string;
     TemporalConstraint?: string;
     PostCond?: string;
     ObstacleEvent?: string;
     // Resource-specific properties
     initialValue?: string;
     lowerBound?: string;
     upperBound?: string;
-  } & {
-    type: 'maintain';
-    maintain: string;
-    assertion: string;
-  };
+  } & MaintainCustomProperties;
 };

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/types/istar.ts` around lines 15 - 41, The
CustomProperties type currently intersects a `{ type: never }` with `{ type:
'maintain'; maintain: string; assertion: string }`, making `type` impossible to
satisfy; change it to a proper discriminated union: define the shared/common
structure (the customProperties object keys like Description, cost, etc.) as a
base type without `type: never`, then create a union of variants (e.g., one
variant with `type: 'maintain'` that includes `maintain` and `assertion` and
another variant for the default/other types that omits those fields or uses
optional fields). Update `CustomProperties` to be that union so the runtime
check in creation.ts (`if (customProperties.type === 'maintain')`) typesafe-ly
narrows to the variant exposing `maintain` and `assertion`.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard against undefined tasks before calling .join()

goal.tasks?.map(...).join(', ') still calls .join() on undefined when tasks is missing, which throws and can abort logging for goals without tasks.

🛠️ Proposed fix

-      write(
-        `\tTasks: ${goal.tasks?.map((task: Task) => task.id).join(', ') ?? 'none'}\n`,
-      );
+      write(
+        `\tTasks: ${
+          goal.tasks && goal.tasks.length > 0
+            ? goal.tasks.map((task: Task) => task.id).join(', ')
+            : 'none'
+        }\n`,
+      );

📝 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

	write(
	`\tTasks: ${goal.tasks?.map((task) => task.id).join(', ') ?? 'none'}\n`,
	`\tTasks: ${goal.tasks?.map((task: Task) => task.id).join(', ') ?? 'none'}\n`,
	);
	write(
	`\tTasks: ${
	goal.tasks && goal.tasks.length > 0
	? goal.tasks.map((task: Task) => task.id).join(', ')
	: 'none'
	}\n`,
	);

🤖 Prompt for AI Agents

In `@packages/lib/src/logger/logger.ts` around lines 216 - 218, The log line calls
.join() on the result of goal.tasks?.map(...), which can be undefined and
throws; change the expression so .join() always runs on an array—e.g., use a
safe fallback like (goal.tasks ?? []).map((task: Task) => task.id).join(', ') or
a conditional check goal.tasks && goal.tasks.length ? goal.tasks.map((task:
Task) => task.id).join(', ') : 'none' in the write call to ensure goals without
tasks do not cause an exception; refer to the existing write call and the Task
type to implement the fix.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Avoid emitting Achievedundefined obstacle events.

The current mapping prefixes before checking for an actual ObstacleEvent, so a missing value yields a truthy Achievedundefined entry.

🐛 Proposed fix

-  new Set(
-    tasks
-      .map((task) => `Achieved${task.properties.sleec?.ObstacleEvent}`)
-      .filter((event): event is string => !!event),
-  );
+  new Set(
+    tasks
+      .map((task) => task.properties.sleec?.ObstacleEvent)
+      .filter((event): event is string => !!event)
+      .map((event) => `Achieved${event}`),
+  );

📝 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 extractObstacleEvents = (tasks: Task[]): Set<string> =>
	new Set(
	tasks
	.map((task) => `Achieved${task.properties.ObstacleEvent}`)
	.map((task) => `Achieved${task.properties.sleec?.ObstacleEvent}`)
	.filter((event): event is string => !!event),
	);
	const extractObstacleEvents = (tasks: Task[]): Set<string> =>
	new Set(
	tasks
	.map((task) => task.properties.sleec?.ObstacleEvent)
	.filter((event): event is string => !!event)
	.map((event) => `Achieved${event}`),
	);

🤖 Prompt for AI Agents

In `@packages/lib/src/sleecTemplateEngine/definitions.ts` around lines 158 - 163,
The extractObstacleEvents function is producing entries like "Achievedundefined"
because it prefixes "Achieved" before filtering out missing sleec.ObstacleEvent
values; update extractObstacleEvents to only map/format after verifying
task.properties.sleec?.ObstacleEvent is present (e.g., filter tasks with a
defined ObstacleEvent first or use a flatMap that returns [] for missing values)
so the Set is built from actual event strings; key symbols:
extractObstacleEvents, Task, and task.properties.sleec?.ObstacleEvent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Populate required GoalNode properties in the test helper.
The helper now omits properties.edge and properties.isQuality, which can cause runtime undefined access if the code under test touches them. Add minimal defaults to keep test fixtures aligned with the runtime shape.

✅ Suggested minimal fixture defaults

 const createGoalNode = (
   id: string,
   name: string,
   sleec?: SleecProps,
 ): GoalNode =>
   ({
     id,
     name,
     type: 'goal',
     properties: {
       sleec,
+      isQuality: false,
+      edge: {},
     },
     resources: [],
   }) as unknown as GoalNode;

🤖 Prompt for AI Agents

In `@packages/lib/test/sleecTemplateEngine/purposes.test.ts` around lines 3 - 26,
The test helper createGoalNode is missing required GoalNode properties used at
runtime (properties.edge and properties.isQuality); update createGoalNode to
populate properties.edge with a sensible minimal default (e.g., null or an empty
object consistent with GoalNode.edge shape) and properties.isQuality with a
boolean default (false) so fixtures match the runtime shape expected by
generatePurposes and other GoalTree consumers; ensure the defaults are set on
the returned object's properties field alongside sleec to avoid undefined access
in tests.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

packages/goal-tree/src/creation.ts (2)

520-575: Ensure pendingDependencies is cleared on all paths (and avoid shared state).

pendingDependencies is module-scoped and only cleared on the happy path. If dependency resolution throws, stale entries can leak into subsequent calls, and concurrent calls can cross-contaminate. Please clear in a finally (or make the map function-scoped).

🧹 Minimal hardening (ensure cleanup on error)

-  const result = tree.map(resolveNodeDependencies);
-
-  // Clear pending dependencies after resolution
-  pendingDependencies.clear();
-
-  return result;
+  try {
+    const result = tree.map(resolveNodeDependencies);
+    return result;
+  } finally {
+    // Clear pending dependencies after resolution (even on error)
+    pendingDependencies.clear();
+  }

---

173-202: Allow zero-valued bounds/initial values for int resources.

The current falsy checks reject "0" for initialValue, lowerBound, or upperBound, which makes valid resources fail parsing. Check for null/undefined/'' instead.

✅ Proposed fix

-      if (!initialValue || !lowerBound || !upperBound) {
+      if (
+        initialValue == null ||
+        lowerBound == null ||
+        upperBound == null ||
+        initialValue === '' ||
+        lowerBound === '' ||
+        upperBound === ''
+      ) {
         throw new Error(
           `[INVALID RESOURCE]: Resource ${resource.id} must have an initial value, lower bound, and upper bound`,
         );
       }

🧹 Nitpick comments (2)

packages/lib/test/templateEngine/modules/goalModule/template/pursue/goalDependency.test.ts (1)

3-5: Consolidate @goal-controller/goal-tree imports.

Three separate imports from the same package add noise; a single import is clearer.

♻️ Suggested refactor

-import { loadPistarModel } from '@goal-controller/goal-tree';
-import { convertToTree } from '@goal-controller/goal-tree';
-import { allGoalsMap } from '@goal-controller/goal-tree';
+import { loadPistarModel, convertToTree, allGoalsMap } from '@goal-controller/goal-tree';

packages/goal-tree/src/utils.ts (1)

105-109: Fix leafGoals return type (currently unsound).

leafGoals always returns GoalNode[], but its signature returns T. This can mislead callers and weaken type safety.

♻️ Proposed fix

-export const leafGoals = <T extends GenericTree>({ gm }: { gm: T }): T => {
-  const leaves = allByType({ gm, type: 'goal' })?.filter(
+export const leafGoals = ({ gm }: { gm: GenericTree }): GoalNode[] => {
+  const leaves = allByType({ gm, type: 'goal' }).filter(
     (goal) => !!goal.tasks,
   );
-  return leaves as T;
+  return leaves;
 };

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

packages/lib/src/templateEngine/modules/goalModule/template/achieve.ts (1)

16-29: Guard against resource-only children returning ().
Right now the length check happens before filtering resources. If a goal’s children are all resources, this produces an empty () condition and can generate invalid output. Filter first, then guard on the filtered length.

🛠️ Proposed fix

-    const children = Node.children(goal);
-    if (children.length) {
-      return `(${children
-        .filter(
-          (child): child is Exclude<TreeNode, { type: 'resource' }> =>
-            !Node.isResource(child),
-        )
-        .map((child) =>
-          child.properties.edge.execCondition?.maintain
-            ? `${achievedMaintain(child.id)}=true`
-            : `${achievedVariable(child.id)}=1`,
-        )
-        .join(separator(goal.relationToChildren))})`;
-    }
+    const children = Node.children(goal).filter(
+      (child): child is Exclude<TreeNode, { type: 'resource' }> =>
+        !Node.isResource(child),
+    );
+    if (children.length) {
+      return `(${children
+        .map((child) =>
+          child.properties.edge.execCondition?.maintain
+            ? `${achievedMaintain(child.id)}=true`
+            : `${achievedVariable(child.id)}=1`,
+        )
+        .join(separator(goal.relationToChildren))})`;
+    }

packages/lib/src/mdp/common.ts (1)

36-37: Pre-existing bug: AND function uses wrong separator.

The AND function incorrectly uses separator('or') instead of separator('and'). While this appears to be a pre-existing bug and not introduced by this PR, it should be fixed.

🐛 Proposed fix

 export const AND = (elements: string[]): string =>
-  elements.join(separator('or'));
+  elements.join(separator('and'));

🤖 Fix all issues with AI agents

In `@packages/goal-tree/src/antlr/RTRegexLexer.ts`:
- Around line 1-14: The generated ANTLR files in packages/goal-tree/src/antlr
(e.g., RTRegexLexer.ts) are duplicated while the grammar source RTRegex.g4 lives
only in packages/lib/grammar; fix by either (A) removing the generated files
from goal-tree and making goal-tree depend on lib’s ANTLR artifacts (export the
generated JS/TS from packages/lib and import them in goal-tree, updating
package.json to add that dependency and removing RTRegexLexer.ts and sibling
files), or (B) copy packages/lib/grammar/RTRegex.g4 into
packages/goal-tree/grammar and add a documented npm script (e.g.,
"generate:antlr") that runs the ANTLR generator to produce
RTRegexLexer.ts/related files in packages/goal-tree/src/antlr, plus a README
note on regeneration; pick one approach and implement it so the authoritative
grammar and a clear regeneration procedure live with the generated outputs
(reference RTRegexLexer.ts and RTRegex.g4 when making the change).

In `@packages/goal-tree/src/GoalTree.ts`:
- Around line 70-73: fromModel currently skips model validation causing unmarked
roots; update GoalTree.fromModel to validate the incoming Model the same way
fromFile/fromJSON do by calling validateModel(model) before calling
convertToTree(model), handle validation errors consistently (throw or propagate
the same error type as fromFile/fromJSON), and only construct the GoalTree with
convertToTree(model) after validation succeeds so this.root will be correctly
set; reference functions: fromModel, validateModel, convertToTree, fromFile,
fromJSON, and the GoalTree constructor/root behavior.

In `@packages/goal-tree/src/internal/creation.ts`:
- Around line 76-78: pendingDependencies is module-scoped and can leak stale IDs
across failed conversions; make it local to the conversion flow or ensure it's
cleared in a try/finally around the tree creation so each run is isolated.
Locate the usage of pendingDependencies in the conversion/creation routine (and
the other occurrences around the 555-575 area) and either move the Map
declaration inside the function that performs the conversion or wrap the
conversion logic in try { ... } finally { pendingDependencies.clear(); } so
pendingDependencies cannot persist after an exception.
- Around line 522-531: The resolution of rawDependsOn in the
resolvedDependencies block currently casts any found node to GoalNode; update
this to validate that each depNode is actually a Goal (e.g., check a
distinguishing property or type such as depNode.type === 'goal' or use an
instanceof/TypeGuard for GoalNode) before casting. For each depId, if
nodeMap.get(depId) returns undefined keep the existing error, but if it returns
a node that is not a Goal, throw a clear error mentioning the dependency id and
node.id (same format as the existing error) stating the dependency is not a
Goal. Modify the resolvedDependencies mapping (and any helper TypeGuard if
present) to perform this check and only return depNode as GoalNode after
validation.

In `@packages/goal-tree/src/internal/variables.ts`:
- Around line 7-8: The helper achievableFormulaVariable is duplicated; export
the existing function (achievableFormulaVariable) from the goal-tree package
module where it's defined, then remove the local duplicate in the lib package's
templateEngine/common.ts and replace it with an import from the goal-tree
export; ensure the exported symbol signature matches the usage in
templateEngine/common.ts and update any imports/exports so TypeScript resolves
the symbol from the goal-tree package.

In `@packages/lib/src/templateEngine/modules/changeManager/changeManager.ts`:
- Around line 14-21: The calls to GoalTree.static helpers are passing the
GoalTree instance `gm` but these helpers (`GoalTree.allByType` and
`GoalTree.taskAchievabilityVariables`) expect a GoalTreeArray (`TreeNode[]`);
update the calls to pass `gm.nodes` instead (i.e., replace
`GoalTree.allByType(gm, 'task')` with `GoalTree.allByType(gm.nodes, 'task')` and
`GoalTree.taskAchievabilityVariables(gm)` with
`GoalTree.taskAchievabilityVariables(gm.nodes)`) so the static methods receive
the expected array and any internal array operations like flatMap will work.

In `@packages/lib/src/templateEngine/modules/goalModule/template/pursue/index.ts`:
- Around line 62-65: The call to pursueLogger.goalDependency currently assumes
goal.properties.edge.dependsOn is always defined and uses .map, which can throw;
update the call site (pursueLogger.goalDependency with goal.id and
goal.properties.edge.dependsOn.map(...)) to guard against undefined by using
optional chaining or a fallback array (e.g., (goal.properties.edge.dependsOn ??
[]).map(...) or goal.properties.edge.dependsOn?.map(...) ?? []) so it safely
passes an empty array when no dependencies exist.

🧹 Nitpick comments (8)

packages/goal-tree/src/internal/utils.ts (1)

21-25: Dead code: the !first check is unreachable.

After the arrays.length === 0 check on line 16, first is guaranteed to be defined (though possibly an empty array). Since ![] is false, this branch never executes for well-typed input.

The function is functionally correct; this is just a minor cleanup opportunity.

♻️ Suggested simplification

   const [first, ...rest] = arrays;
-  if (!first) {
-    yield [];
-    return;
-  }
+  // first is guaranteed defined after length check; empty array yields nothing (correct)
   for (const item of first) {

packages/goal-tree/src/Model.ts (1)

7-47: validateModel has a side effect: it mutates the model to mark the root node.

The function sets node.customProperties.root = 'true' on line 37, making it both a validator and a mutator. Consider documenting this behavior in the JSDoc or renaming to validateAndMarkRoot to make the side effect explicit.

📝 Suggested documentation update

 /**
  * Validate an iStar model
+ * Also marks the root node by setting customProperties.root = 'true'
  * `@throws` Error if the model is invalid
  */
 function validateModel(model: IStarModel): void {

packages/goal-tree/src/types/istar.ts (1)

93-103: Consider documenting the never[] fields.

The orphans: never[] and dependencies: never[] indicate these arrays should always be empty. A brief comment explaining why (e.g., "unused in goal models" or "preserved for piStar compatibility") would help future maintainers understand the intent.

📝 Suggested documentation

 export type Model = {
   actors: Actor[];
+  /** Always empty - preserved for piStar format compatibility */
   orphans: never[];
+  /** Always empty - preserved for piStar format compatibility */
   dependencies: never[];
   links: Link[];

packages/ui/lib/models/goal.ts (1)

31-56: Type safety relies on runtime validation.

The JSON.parse result is assigned directly to IStarModel without compile-time type checking. While Model.validate(model) provides runtime validation, consider using a type guard or schema validation library (like Zod) that provides both runtime validation and compile-time type narrowing.

Current approach is acceptable since validation occurs immediately after parsing.

packages/goal-tree/src/internal/traversal.ts (1)

47-66: Consider using a Map for deduplication to improve performance.

The current implementation creates a new object on each reduce iteration via spread. For large trees, this is O(n²). Using a Map directly would be more efficient.

♻️ Optional performance improvement

 export function allByType(gm: GoalTree, type: Type): TreeNode[] {
-  const allCurrent = gm
+  const allNodes = gm
     .flatMap((node) => {
       const children = childrenWithTasksAndResources(node);
       if (children.length > 0) {
         return [node, ...(allByType(children, type) ?? [])];
       }
       return [node];
     })
     .filter((node) => node.type === type)
-    .sort((a, b) => a.id.localeCompare(b.id))
-    .reduce<Record<string, TreeNode>>((acc, current) => {
-      if (acc[current.id]) {
-        return { ...acc };
-      }
-      return { ...acc, [current.id]: current };
-    }, {});
+    .sort((a, b) => a.id.localeCompare(b.id));
 
-  return Object.values(allCurrent);
+  // Deduplicate using Map for O(n) instead of O(n²)
+  const seen = new Map<string, TreeNode>();
+  for (const node of allNodes) {
+    if (!seen.has(node.id)) {
+      seen.set(node.id, node);
+    }
+  }
+  return Array.from(seen.values());
 }

packages/lib/src/prismValidator/validator.ts (1)

216-228: Consider simplifying the nested ternary chain.

The deeply nested ternary for determining goalType is hard to read. A lookup or early return pattern would improve clarity.

♻️ Suggested refactor

-    const goalType =
-      goal.properties.edge.executionDetail?.type === 'choice'
-        ? 'choice'
-        : goal.properties.edge.executionDetail?.type === 'degradation'
-          ? 'degradation'
-          : goal.properties.edge.executionDetail?.type === 'sequence'
-            ? 'sequence'
-            : goal.properties.edge.executionDetail?.type === 'interleaved'
-              ? 'interleaved'
-              : goal.properties.edge.executionDetail?.type === 'alternative'
-                ? 'alternative'
-                : 'basic';
+    const executionType = goal.properties.edge.executionDetail?.type;
+    const goalType: keyof GoalTypeCounts =
+      executionType && executionType in goalTypes ? executionType : 'basic';

packages/goal-tree/src/GoalTree.ts (1)

127-147: Debug print() method may not belong in production SDK.

The print() method uses console.log extensively and assumes the first node is a GoalNode with children. Consider marking it as @internal or @deprecated, or moving it to a separate debug utility.

packages/lib/src/antlr/RTRegexListener.ts (1)

1-21: Consider consolidating ANTLR artifacts to avoid drift.

This listener file appears to be duplicated in both packages/lib/src/antlr/ and packages/goal-tree/src/antlr/. If the grammar (RTRegex.g4) is updated in the future, both locations would need regeneration.

Consider either:

1. Having packages/lib re-export the listener from @goal-controller/goal-tree, or
2. Keeping a single source of truth for the grammar and generated files

If the duplication is intentional (e.g., different consumers or versioning requirements), please disregard.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if both RTRegexLexer files exist and compare their content
echo "=== Checking for RTRegexLexer in both locations ==="
fd -t f "RTRegexLexer.ts" packages/

echo ""
echo "=== Checking if files are identical ==="
if [ -f "packages/goal-tree/src/antlr/RTRegexLexer.ts" ] && [ -f "packages/lib/src/antlr/RTRegexLexer.ts" ]; then
  if diff -q "packages/goal-tree/src/antlr/RTRegexLexer.ts" "packages/lib/src/antlr/RTRegexLexer.ts"; then
    echo "Files are identical"
  else
    echo "Files differ"
    echo ""
    echo "=== First 50 lines of goal-tree version ==="
    head -50 "packages/goal-tree/src/antlr/RTRegexLexer.ts"
    echo ""
    echo "=== First 50 lines of lib version ==="
    head -50 "packages/lib/src/antlr/RTRegexLexer.ts"
  fi
else
  echo "One or both files do not exist"
  [ ! -f "packages/goal-tree/src/antlr/RTRegexLexer.ts" ] && echo "Missing: packages/goal-tree/src/antlr/RTRegexLexer.ts"
  [ ! -f "packages/lib/src/antlr/RTRegexLexer.ts" ] && echo "Missing: packages/lib/src/antlr/RTRegexLexer.ts"
fi

Repository: vieirin/goal-controller

Length of output: 263

---

🏁 Script executed:

#!/bin/bash
# Check package.json files to understand dependencies
echo "=== packages/goal-tree/package.json ==="
[ -f "packages/goal-tree/package.json" ] && cat "packages/goal-tree/package.json" || echo "Not found"

echo ""
echo "=== Check imports of RTRegexLexer in goal-tree ==="
rg "RTRegexLexer" packages/goal-tree/ -t ts --max-count 10

echo ""
echo "=== Check imports of RTRegexLexer in lib ==="
rg "RTRegexLexer" packages/lib/ -t ts --max-count 10

echo ""
echo "=== Check if goal-tree depends on lib ==="
grep -i "\"lib\"" "packages/goal-tree/package.json" || echo "No direct dependency on lib found"

Repository: vieirin/goal-controller

Length of output: 2753

---

🏁 Script executed:

#!/bin/bash
# Check for other ANTLR files in both packages
echo "=== ANTLR files in goal-tree ==="
fd -t f "RTRegex" packages/goal-tree/src/antlr/ | sort

echo ""
echo "=== ANTLR files in lib ==="
fd -t f "RTRegex" packages/lib/src/antlr/ | sort

echo ""
echo "=== Check if files are identical (Parser, Listener, Visitor, etc.) ==="
for file in RTRegexLexer.ts RTRegexParser.ts RTRegexParserVisitor.ts RTRegexParserListener.ts; do
  if [ -f "packages/goal-tree/src/antlr/$file" ] && [ -f "packages/lib/src/antlr/$file" ]; then
    echo -n "$file: "
    if diff -q "packages/goal-tree/src/antlr/$file" "packages/lib/src/antlr/$file" > /dev/null; then
      echo "identical"
    else
      echo "DIFFER"
    fi
  fi
done

echo ""
echo "=== Check README or docs for package extraction intent ==="
find packages/goal-tree -name "README*" -type f | head -5 | xargs cat 2>/dev/null | head -20

Repository: vieirin/goal-controller

Length of output: 574

---

🏁 Script executed:

#!/bin/bash
# Check for ANTLR grammar source files
echo "=== Looking for RTRegex.g4 grammar files ==="
fd -t f "RTRegex.g4" packages/

echo ""
echo "=== Check all ANTLR files across both packages ==="
echo "goal-tree ANTLR directory:"
ls -la packages/goal-tree/src/antlr/ 2>/dev/null || echo "Directory not found"

echo ""
echo "lib ANTLR directory:"
ls -la packages/lib/src/antlr/ 2>/dev/null || echo "Directory not found"

echo ""
echo "=== Check git log for recent changes to these files ==="
git log --oneline -10 -- "packages/goal-tree/src/antlr/" "packages/lib/src/antlr/" 2>/dev/null || echo "No recent changes tracked"

Repository: vieirin/goal-controller

Length of output: 1525

---

Address ANTLR file duplication without grammar source.

The identical RTRegexLexer.ts and other ANTLR files are duplicated in packages/goal-tree/src/antlr/, but the grammar source (RTRegex.g4) only exists in packages/lib/grammar/. This creates a maintenance risk: if the grammar is updated in lib, the generated files in goal-tree won't be regenerated automatically, causing synchronization drift.

Either copy the grammar source to goal-tree with a documented regeneration procedure, or have goal-tree depend on lib for these ANTLR utilities.

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/antlr/RTRegexLexer.ts` around lines 1 - 14, The
generated ANTLR files in packages/goal-tree/src/antlr (e.g., RTRegexLexer.ts)
are duplicated while the grammar source RTRegex.g4 lives only in
packages/lib/grammar; fix by either (A) removing the generated files from
goal-tree and making goal-tree depend on lib’s ANTLR artifacts (export the
generated JS/TS from packages/lib and import them in goal-tree, updating
package.json to add that dependency and removing RTRegexLexer.ts and sibling
files), or (B) copy packages/lib/grammar/RTRegex.g4 into
packages/goal-tree/grammar and add a documented npm script (e.g.,
"generate:antlr") that runs the ANTLR generator to produce
RTRegexLexer.ts/related files in packages/goal-tree/src/antlr, plus a README
note on regeneration; pick one approach and implement it so the authoritative
grammar and a clear regeneration procedure live with the generated outputs
(reference RTRegexLexer.ts and RTRegex.g4 when making the change).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

fromModel bypasses model validation.

fromFile and fromJSON call validateModel, but fromModel does not. If a caller passes an unvalidated Model directly, the root node won't be marked and this.root will return undefined.

🛠️ Suggested fix

  static fromModel(model: Model): GoalTree {
+   validateModel(model);
    const nodes = convertToTree(model);
    return new GoalTree(nodes);
  }

📝 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

	static fromModel(model: Model): GoalTree {
	const nodes = convertToTree(model);
	return new GoalTree(nodes);
	}
	static fromModel(model: Model): GoalTree {
	validateModel(model);
	const nodes = convertToTree(model);
	return new GoalTree(nodes);
	}

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/GoalTree.ts` around lines 70 - 73, fromModel currently
skips model validation causing unmarked roots; update GoalTree.fromModel to
validate the incoming Model the same way fromFile/fromJSON do by calling
validateModel(model) before calling convertToTree(model), handle validation
errors consistently (throw or propagate the same error type as
fromFile/fromJSON), and only construct the GoalTree with convertToTree(model)
after validation succeeds so this.root will be correctly set; reference
functions: fromModel, validateModel, convertToTree, fromFile, fromJSON, and the
GoalTree constructor/root behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Validate that dependsOn targets are goals.

dependsOn is typed as GoalNode[], but resolution currently casts any node. If a model references a task/resource, the graph becomes inconsistent. Add a type check and throw.

✅ Suggested validation

     const resolvedDependencies = rawDependsOn.map((depId) => {
       const depNode = nodeMap.get(depId);
-      if (!depNode) {
+      if (!depNode || depNode.type !== 'goal') {
         throw new Error(
-          `[INVALID MODEL]: Dependency ${depId} not found for node ${node.id}`,
+          `[INVALID MODEL]: Dependency ${depId} must reference a goal node`,
         );
       }
-      return depNode as GoalNode;
+      return depNode;
     });

📝 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 resolvedDependencies = rawDependsOn.map((depId) => {
	const depNode = nodeMap.get(depId);
	if (!depNode) {
	throw new Error(
	`[INVALID MODEL]: Dependency ${depId} not found for node ${node.id}`,
	);
	}
	return depNode;
	return depNode as GoalNode;
	});
	const resolvedDependencies = rawDependsOn.map((depId) => {
	const depNode = nodeMap.get(depId);
	if (!depNode || depNode.type !== 'goal') {
	throw new Error(
	`[INVALID MODEL]: Dependency ${depId} must reference a goal node`,
	);
	}
	return depNode;
	});

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/internal/creation.ts` around lines 522 - 531, The
resolution of rawDependsOn in the resolvedDependencies block currently casts any
found node to GoalNode; update this to validate that each depNode is actually a
Goal (e.g., check a distinguishing property or type such as depNode.type ===
'goal' or use an instanceof/TypeGuard for GoalNode) before casting. For each
depId, if nodeMap.get(depId) returns undefined keep the existing error, but if
it returns a node that is not a Goal, throw a clear error mentioning the
dependency id and node.id (same format as the existing error) stating the
dependency is not a Goal. Modify the resolvedDependencies mapping (and any
helper TypeGuard if present) to perform this check and only return depNode as
GoalNode after validation.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all occurrences of achievableFormulaVariable
rg -n "achievableFormulaVariable" --type=ts

Repository: vieirin/goal-controller

Length of output: 1756

---

🏁 Script executed:

#!/bin/bash
# Read the lib version
echo "=== packages/lib/src/templateEngine/common.ts ==="
sed -n '10,20p' packages/lib/src/templateEngine/common.ts

echo -e "\n=== packages/goal-tree/src/internal/variables.ts (first 30 lines) ==="
head -30 packages/goal-tree/src/internal/variables.ts

Repository: vieirin/goal-controller

Length of output: 1454

---

🏁 Script executed:

#!/bin/bash
# Check if goal-tree imports from lib
echo "=== Checking imports in goal-tree/src/internal/variables.ts ==="
head -10 packages/goal-tree/src/internal/variables.ts

# Check dependencies in goal-tree package.json
echo -e "\n=== goal-tree package.json dependencies ==="
cat packages/goal-tree/package.json | jq '.dependencies // {}'

# Check if lib is a dependency of goal-tree
echo -e "\n=== Does goal-tree depend on lib? ==="
cat packages/goal-tree/package.json | jq '.dependencies | has("@goal-controller/lib")'

Repository: vieirin/goal-controller

Length of output: 552

---

🏁 Script executed:

#!/bin/bash
# Check if lib depends on goal-tree
echo "=== lib package.json dependencies ==="
cat packages/lib/package.json | jq '.dependencies // {}'

echo -e "\n=== Does lib depend on goal-tree? ==="
cat packages/lib/package.json | jq '.dependencies | has("@goal-controller/goal-tree")'

Repository: vieirin/goal-controller

Length of output: 357

---

Refactor to avoid duplication: export achievableFormulaVariable from goal-tree and import in lib.

This helper is currently duplicated across both packages with identical implementations. Since lib already depends on goal-tree, the viable solution is to:

1. Export achievableFormulaVariable from packages/goal-tree/src/internal/variables.ts
2. Import it in packages/lib/src/templateEngine/common.ts instead of defining a local copy

This prevents divergence if the naming convention changes and eliminates unnecessary duplication.

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/internal/variables.ts` around lines 7 - 8, The helper
achievableFormulaVariable is duplicated; export the existing function
(achievableFormulaVariable) from the goal-tree package module where it's
defined, then remove the local duplicate in the lib package's
templateEngine/common.ts and replace it with an import from the goal-tree
export; ensure the exported symbol signature matches the usage in
templateEngine/common.ts and update any imports/exports so TypeScript resolves
the symbol from the goal-tree package.

[coderabbitai]

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)

packages/lib/src/cli.ts (1)

86-90: Missing return after recursive mainMenu() call.

When no files are found, mainMenu() is called recursively but execution continues to the inquirer.prompt below, which will fail or behave unexpectedly with an empty files array.

🐛 Proposed fix

     if (files.length === 0) {
       console.log('No files found in the example directory.');
       await mainMenu();
+      return;
     }

🤖 Fix all issues with AI agents

In `@packages/lib/src/engines/edge/template/modules/goalModule/goalModules.ts`:
- Around line 5-12: The goalNumberId function currently uses goalId.match(/\d+/)
which allows digits anywhere (e.g., "foo1"); change the validation to require
the "G" prefix by matching the whole string against a pattern like /^G(\d+)$/
(or case-insensitive if needed) and return the captured digit group; update the
thrown error message to reflect the enforced pattern and ensure you reference
goalNumberId when making the change.

In
`@packages/lib/src/engines/edge/template/modules/goalModule/template/pursue/index.ts`:
- Line 27: The cast `dep as EdgeGoalNode` masks a type mismatch between the
declared type of dependsOn and what hasBeenAchieved requires; update the type of
dependsOn (the collection type used where dependsOn is declared) to use the more
specific GoalNode<EdgeGoalPropsResolved, EdgeTaskProps, EdgeResourceProps>
(EdgeGoalNode) so the map call can pass each dep directly to hasBeenAchieved, or
alternatively add a proper type guard function (e.g., isEdgeGoalNode(dep): dep
is EdgeGoalNode) and use .filter/is guard before .map to narrow the type; adjust
the declaration of dependsOn or add the guard where .map((dep) =>
hasBeenAchieved(...)) is called so the explicit cast is no longer necessary.

In `@README.md`:
- Around line 431-451: The example call passes tree.nodes but the function
signature is sleecTemplateEngine(tree: GoalTreeType<...>); fix the example by
calling sleecTemplateEngine(tree) (remove .nodes) so the passed value matches
the expected GoalTreeType used by sleecTemplateEngine and its internal calls to
GoalTree.allByType; update the example lines with
Model.load/GoalTree.fromModel/sleecEngineMapper accordingly.

🧹 Nitpick comments (5)

packages/lib/src/cli.ts (3)

74-84: Consider validating loaded variables for consistency with MODE=last.

The MODE=last flow (lines 40-51) validates that variables are non-empty after loading, but the interactive "last" action does not. If inputDefaultVariables fails or user cancels, loadVariables may return {} and runModel proceeds with empty variables.

♻️ Suggested consistency improvement

     const variables = loadVariables(lastSelectedModel);
+    if (Object.keys(variables).length === 0) {
+      console.error('Failed to load variables. Please try again.');
+      return;
+    }
     await runModel(lastSelectedModel, variables, cleanFlag);

---

132-132: Prompt message should reference "dump" not "run".

The message says "Select a model file to run:" but this is the dumpTree action.

✏️ Suggested fix

-        message: 'Select a model file to run:',
+        message: 'Select a model file to dump:',

---

141-144: Consider adding error handling for model loading.

Model.load and GoalTree.fromModel may throw if the model file is invalid or malformed. Wrapping in try-catch would provide a more user-friendly error message instead of a stack trace.

🛡️ Suggested error handling

+    try {
       const model = Model.load(selectedFile);
       const tree = GoalTree.fromModel(model, edgeEngineMapper);
       const json = tree.toJSON();
       await writeFile(`output/${path.parse(selectedFile).name}.json`, json);
       console.log(
         `The file ${path.parse(selectedFile).name}.json was saved successfully!`,
       );
+    } catch (error) {
+      console.error(`Failed to process model file: ${error instanceof Error ? error.message : error}`);
+    }

packages/lib/src/engines/edge/template/modules/changeManager/template/achievabilityVariables/taskAchievabilityVariables.ts (1)

28-28: Remove unreachable ?? -1 fallback.

At line 28, variableValue is guaranteed to be either the configured value or DEFAULT_ACHIEVABILITY (0.5) due to the ternary on lines 16-18. The ?? -1 fallback is dead code and may mislead readers into thinking variableValue can be nullish.

🧹 Proposed cleanup

-  logger.achievabilityTaskConstant(task.id, variableName, variableValue ?? -1);
+  logger.achievabilityTaskConstant(task.id, variableName, variableValue);

packages/lib/src/engines/edge/template/modules/goalModule/template/pursue/index.ts (1)

385-385: Unnecessary nullish coalescing.

pursueLines is the result of a .map() chain, which always returns an array (never null/undefined). The ?? [] fallback is unreachable.

♻️ Suggested simplification

-  return pursueLines ?? [];
+  return pursueLines;

[coderabbitai]

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)

README.md (1)

588-590: Fix typo in directory name.

Line 588 says "goalmngt folder" but line 589 shows "goalmgmt/edgeModel.txt". The directory name is inconsistent.

📝 Suggested fix

-3. Add the goal model to the goalmngt folder
+3. Add the goal model to the goalmgmt folder

🤖 Fix all issues with AI agents

In `@packages/lib/src/engines/edge/mapper.ts`:
- Around line 57-80: The parseDecision function currently allows empty variable
names (e.g., ":3" or " ,:2") to pass through; update parseDecision to trim the
variable portion (d[0]) and validate it is non-empty after trimming, throwing a
clear Error like `[INVALID DECISION]: variable must be a non-empty string` for
any entry with an empty trimmed variable; keep existing numeric validation for
space (parseInt) and use the trimmed variable when returning the final objects
so templates never receive empty variable names.
- Around line 87-101: In parseMaxRetries, reject non-strict integer strings
before parsing: trim the input maxRetries and validate it matches a whole-number
regex (e.g. /^\d+$/) so values like "5abc" or "1.5" fail; if the trimmed string
doesn't match, throw the same Error using nodeType, otherwise safely parseInt
and return the number (preserve the existing undefined -> 0 behavior). Ensure
you update the logic inside the parseMaxRetries function to do the trim + regex
check prior to parsing.

In `@README.md`:
- Around line 264-284: The README shows inconsistent query APIs—use one approach
consistently (prefer the instance API) by replacing static calls like
GoalTree.allByType(tree, 'task') in the "generatePurposes" and "Complete
Template Engine" examples with the instance form tree.query.allByType('task'),
and update any other occurrences (e.g., GoalTree.leafGoals(...) →
tree.query.leafGoals()) so all examples use tree.query.*; alternatively, if you
intend to support both patterns, add a short paragraph explaining when to use
GoalTree.static methods vs the tree.query instance methods and keep both
examples annotated accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Reject empty decision variable names.
Inputs like ":3" or " ,:2" currently produce an empty variable string, which will leak into templates.

🔧 Suggested fix

   const parsedDecision = decision.split(',').map((d) => d.split(':'));
   parsedDecision.forEach((d) => {
     if (d.length !== 2) {
       throw new Error(
         `[INVALID DECISION]: decision must be a variable and space: got ${decision}, expected format variable:space`,
       );
     }
+    const variable = (d[0] ?? '').trim();
+    if (!variable) {
+      throw new Error(
+        `[INVALID DECISION]: variable name must be non-empty: got ${decision}`,
+      );
+    }
     if (isNaN(parseInt(d[1] ?? ''))) {
       throw new Error(
         `[INVALID DECISION]: space must be a number: got ${d[1]}`,
       );
     }
   });
@@
   return parsedDecision.map((d) => ({
-    variable: d[0]?.trim() ?? '',
+    variable: d[0]?.trim() ?? '',
     space: parseInt(d[1] ?? '', 10),
   }));

🤖 Prompt for AI Agents

In `@packages/lib/src/engines/edge/mapper.ts` around lines 57 - 80, The
parseDecision function currently allows empty variable names (e.g., ":3" or "
,:2") to pass through; update parseDecision to trim the variable portion (d[0])
and validate it is non-empty after trimming, throwing a clear Error like
`[INVALID DECISION]: variable must be a non-empty string` for any entry with an
empty trimmed variable; keep existing numeric validation for space (parseInt)
and use the trimmed variable when returning the final objects so templates never
receive empty variable names.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

JavaScript parseInt semantics "1.5" "5abc" strict integer validation

💡 Result:

• parseInt(str, radix) parses up to the first non-valid digit (after optional whitespace and +/-). It does not require the whole string to be an integer. If it hits an invalid character, it stops and returns what it has so far; if it can’t parse any digit at the start, it returns NaN. Also, . is not a digit, so decimals get truncated. [1]
  • parseInt("1.5", 10) → 1 (stops at .) [1]
  • parseInt("5abc", 10) → 5 (stops at a) [1]

Strict integer validation (string must be only an integer)

Use a full-string check plus numeric validation:

const isStrictIntString = s => /^[-+]?\d+$/.test(s);

If you want an actual number too:

const parseStrictInt = s => {
  if (!/^[-+]?\d+$/.test(s)) return null;     // reject "1.5", "5abc", " 5 "
  const n = Number(s);
  return Number.isInteger(n) ? n : null;
};

Number.isInteger(x) returns true only if x is a number and an integer (not NaN/Infinity). [2]

Why not just Number()? Because Number("5abc") becomes NaN (good), but you still typically want the regex to enforce the exact accepted format (sign/digits only) and reject things like whitespace if desired.

Sources: MDN on parseInt() behavior and stopping at invalid characters / truncating at . [1]; MDN on Number.isInteger() semantics [2].

References:
[1] MDN — parseInt() (Global object)
[2] MDN — Number.isInteger()

---

🏁 Script executed:

# Find usages of parseMaxRetries in the codebase to understand impact
rg "parseMaxRetries" --type ts --type tsx -B 2 -A 2

Repository: vieirin/goal-controller

Length of output: 94

---

🏁 Script executed:

# Check if there are any tests for parseMaxRetries
rg "parseMaxRetries" --type ts --type tsx --type json

Repository: vieirin/goal-controller

Length of output: 94

---

🏁 Script executed:

# Search for parseMaxRetries usage
rg "parseMaxRetries" -t ts

Repository: vieirin/goal-controller

Length of output: 330

---

🏁 Script executed:

# Check for any test files mentioning this function
find . -type f -name "*.test.ts" -o -name "*.spec.ts" | xargs rg "parseMaxRetries" 2>/dev/null || echo "No test files found with parseMaxRetries"

Repository: vieirin/goal-controller

Length of output: 107

---

🏁 Script executed:

# Check the context around where parseMaxRetries is defined and used
rg "parseMaxRetries|maxRetries" -t ts -B 3 -A 3 | head -100

Repository: vieirin/goal-controller

Length of output: 6345

---

🏁 Script executed:

# Verify the file content and exact line numbers to ensure diff accuracy
sed -n '85,105p' packages/lib/src/engines/edge/mapper.ts

Repository: vieirin/goal-controller

Length of output: 630

---

Enforce strict integer parsing for maxRetries.

parseInt truncates at the first non-digit character, so parseInt("5abc", 10) returns 5 and parseInt("1.5", 10) returns 1—both valid numbers. The isNaN check won't catch these cases. Invalid strings like "5abc" or "1.5" will pass validation even though the error message requires "a non-negative integer."

🔧 Suggested fix

 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)) {
     throw new Error(
       `[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`,
     );
   }
-  return parsed;
+  return Number(maxRetries);
 };

📝 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)) {
	throw new Error(
	`[INVALID ${nodeType.toUpperCase()}]: maxRetries must be a non-negative integer: got "${maxRetries}"`,
	);
	}
	return Number(maxRetries);
	};

🤖 Prompt for AI Agents

In `@packages/lib/src/engines/edge/mapper.ts` around lines 87 - 101, In
parseMaxRetries, reject non-strict integer strings before parsing: trim the
input maxRetries and validate it matches a whole-number regex (e.g. /^\d+$/) so
values like "5abc" or "1.5" fail; if the trimmed string doesn't match, throw the
same Error using nodeType, otherwise safely parseInt and return the number
(preserve the existing undefined -> 0 behavior). Ensure you update the logic
inside the parseMaxRetries function to do the trim + regex check prior to
parsing.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

API inconsistency across tutorial examples.

The tutorial demonstrates two different APIs for querying nodes without explanation:

1. Instance method (lines 270, 280): tree.query.allByType('task')
2. Static method (lines 390, 434): GoalTree.allByType(tree, 'task')

Within the same "Transformation Example (SLEEC)" section, the "Querying the Tree" subsection shows the instance method, while the "generatePurposes" and "Complete Template Engine" subsections use the static method. This inconsistency will confuse developers following the tutorial.

Either use one API consistently throughout, or explicitly document why both patterns exist and when to use each.

🔄 Suggested fix for consistency

Use the instance method consistently throughout the tutorial:

 const generatePurposes = (tree: SleecGoalTree): string => {
-  const goals = GoalTree.allByType(tree, 'goal');
+  const goals = tree.query.allByType('goal');

 export const sleecTemplateEngine = (
   tree: GoalTreeType<SleecGoalProps, SleecTaskProps>
 ): string => {
-  const tasks = GoalTree.allByType(tree, 'task');
+  const tasks = tree.query.allByType('task');

Also applies to: 379-421, 424-453

🤖 Prompt for AI Agents

In `@README.md` around lines 264 - 284, The README shows inconsistent query
APIs—use one approach consistently (prefer the instance API) by replacing static
calls like GoalTree.allByType(tree, 'task') in the "generatePurposes" and
"Complete Template Engine" examples with the instance form
tree.query.allByType('task'), and update any other occurrences (e.g.,
GoalTree.leafGoals(...) → tree.query.leafGoals()) so all examples use
tree.query.*; alternatively, if you intend to support both patterns, add a short
paragraph explaining when to use GoalTree.static methods vs the tree.query
instance methods and keep both examples annotated accordingly.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@packages/goal-tree/src/internal/engineMapper.ts`:
- Around line 153-175: The code currently treats skipResource
(allowedResourceKeys === undefined) as silently dropping any provided
mapResourceProps; update the guard in engineMapper.ts so that if skipResource is
true but config.mapResourceProps is non-undefined you throw a clear error (or
assert) instead of returning a mapper that ignores mapping; reference the
symbols skipResource, config.mapResourceProps, allowedResourceKeys and the
EngineMapper return path to locate where to add the check and error to fail fast
when a consumer supplies mapResourceProps without allowedResourceKeys.

In `@packages/lib/src/cli/menu/selectModel.ts`:
- Around line 20-22: The code in selectModel.ts uses filePath.split('/') to
derive fileName, which breaks on Windows; replace that logic by calling
path.basename(filePath) (ensure the Node 'path' module is imported) and use the
resulting fileName variable in place of the split-based value; keep the existing
error throw for empty/undefined fileName but update the lookup to rely on
path.basename for cross-platform correctness.

🧹 Nitpick comments (2)

packages/lib/src/engines/sleec/mapper.ts (1)

57-68: Inconsistent validation logic with extractSleecGoalProps.

extractSleecGoalProps uses typeof value === 'string' && value.trim() !== '' which rejects whitespace-only strings, while this function uses simple truthy checks that would accept whitespace-only strings. Consider aligning the validation for consistent behavior.

♻️ Proposed refactor using the same pattern

 const extractSleecTaskProps = (raw: RawProps<SleecTaskKey>): SleecTaskProps => {
   const sleecProps: SleecTaskProps = {};
 
-  if (raw.PreCond) sleecProps.PreCond = raw.PreCond;
-  if (raw.TriggeringEvent) sleecProps.TriggeringEvent = raw.TriggeringEvent;
-  if (raw.TemporalConstraint)
-    sleecProps.TemporalConstraint = raw.TemporalConstraint;
-  if (raw.PostCond) sleecProps.PostCond = raw.PostCond;
-  if (raw.ObstacleEvent) sleecProps.ObstacleEvent = raw.ObstacleEvent;
+  for (const key of SLEEC_TASK_KEYS) {
+    const value = raw[key];
+    if (typeof value === 'string' && value.trim() !== '') {
+      sleecProps[key] = value;
+    }
+  }
 
   return sleecProps;
 };

packages/lib/src/engines/edge/mapper.ts (1)

218-298: Consider strict integer parsing for bounds and initialValue.

JavaScript's parseInt() accepts partial numeric strings—parseInt("5abc", 10) returns 5, not NaN—so "5abc", "1.5", and similar malformed inputs silently pass the current isNaN() validation despite failing the empty string check. If you want strict integer-only validation, enforce full-string matching:

♻️ Suggested refactor

-        const lowerBoundInt = parseInt(lowerBound, 10);
-        const upperBoundInt = parseInt(upperBound, 10);
+        const parseStrictInt = (value: string, label: string): number => {
+          const trimmed = value.trim();
+          if (!/^-?\d+$/.test(trimmed)) {
+            throw new Error(
+              `[INVALID RESOURCE]: ${label} must be an integer string, got: "${value}"`,
+            );
+          }
+          return Number.parseInt(trimmed, 10);
+        };
+
+        const lowerBoundInt = parseStrictInt(lowerBound, 'lowerBound');
+        const upperBoundInt = parseStrictInt(upperBound, 'upperBound');
@@
-        const initialValueInt = parseInt(initialValue, 10);
+        const initialValueInt = parseStrictInt(initialValue, 'initialValue');

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Guard against mapResourceProps being silently ignored.
Right now, if allowedResourceKeys is omitted but mapResourceProps is still provided (e.g., JS consumer or any-cast), the mapper gets dropped without feedback. Consider failing fast to avoid subtle runtime misconfigurations.

🔧 Proposed guard

     const skipResource = config.allowedResourceKeys === undefined;
+    if (skipResource && 'mapResourceProps' in config && config.mapResourceProps) {
+      throw new Error(
+        '[INVALID MAPPER]: mapResourceProps was provided but allowedResourceKeys is undefined. ' +
+          'Either provide allowedResourceKeys or remove mapResourceProps.',
+      );
+    }
 
     // If skipResource is explicitly set (no allowedResourceKeys), skip resource mapping
     if (skipResource) {

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/internal/engineMapper.ts` around lines 153 - 175, The
code currently treats skipResource (allowedResourceKeys === undefined) as
silently dropping any provided mapResourceProps; update the guard in
engineMapper.ts so that if skipResource is true but config.mapResourceProps is
non-undefined you throw a clear error (or assert) instead of returning a mapper
that ignores mapping; reference the symbols skipResource,
config.mapResourceProps, allowedResourceKeys and the EngineMapper return path to
locate where to add the check and error to fail fast when a consumer supplies
mapResourceProps without allowedResourceKeys.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

head -40 packages/lib/src/cli/menu/selectModel.ts

Repository: vieirin/goal-controller

Length of output: 1172

---

Use path.basename for cross-platform filename parsing (Line 20).

filePath.split('/') fails on Windows paths which use backslashes. Replace with path.basename to handle both Unix and Windows paths correctly.

🛠️ Proposed fix

-    const fileName = filePath.split('/').pop();
+    const fileName = path.basename(filePath);

🤖 Prompt for AI Agents

In `@packages/lib/src/cli/menu/selectModel.ts` around lines 20 - 22, The code in
selectModel.ts uses filePath.split('/') to derive fileName, which breaks on
Windows; replace that logic by calling path.basename(filePath) (ensure the Node
'path' module is imported) and use the resulting fileName variable in place of
the split-based value; keep the existing error throw for empty/undefined
fileName but update the lookup to rely on path.basename for cross-platform
correctness.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/goal-tree/src/internal/creation.ts`:
- Around line 350-355: The thrown error message after checking const allEqual =
relations.every((v) => v === relations[0]) contains leading newlines and
indentation; replace the multi-line template literal in the throw statement with
a single-line string (or call .trim() on the template) so the error reads
cleanly, e.g. throw new Error("[INVALID MODEL]: You can't mix and/or
relations"); update the throw in the same block where allEqual and relations are
used.

🧹 Nitpick comments (1)

packages/goal-tree/src/internal/creation.ts (1)

529-535: afterCreationMapper only processes goal nodes; tasks and resources are skipped.

The tree mapping at lines 529-535 only invokes processGoal for nodes where type === 'goal'. If an engine's afterCreationMapper needs to transform task or resource properties post-creation (e.g., resolving cross-references), those transformations won't be applied.

This appears intentional given the current Edge mapper only uses afterCreationMapper for goal-level dependsOn resolution. If future engines require post-creation processing for tasks/resources, this would need to be extended.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Normalize error message whitespace.

The template literal includes leading newlines and excessive indentation that will appear in the error message.

🔧 Suggested fix

   const allEqual = relations.every((v) => v === relations[0]);
   if (!allEqual) {
-    throw new Error(`
-            [INVALID MODEL]: You can't mix and/or relations
-        `);
+    throw new Error("[INVALID MODEL]: You can't mix and/or relations");
   }

📝 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 allEqual = relations.every((v) => v === relations[0]);
	if (!allEqual) {
	throw new Error(`
	[INVALID MODEL]: You can't mix and/or relations
	`);
	}
	const allEqual = relations.every((v) => v === relations[0]);
	if (!allEqual) {
	throw new Error("[INVALID MODEL]: You can't mix and/or relations");
	}

🤖 Prompt for AI Agents

In `@packages/goal-tree/src/internal/creation.ts` around lines 350 - 355, The
thrown error message after checking const allEqual = relations.every((v) => v
=== relations[0]) contains leading newlines and indentation; replace the
multi-line template literal in the throw statement with a single-line string (or
call .trim() on the template) so the error reads cleanly, e.g. throw new
Error("[INVALID MODEL]: You can't mix and/or relations"); update the throw in
the same block where allEqual and relations are used.

[coderabbitai]

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 (3)

packages/ui/app/api/transform/route.ts (1)

97-105: ⚠️ Potential issue | 🟡 Minor

Logger not closed on generation errors either.

When generateValidatedPrismModel or sleecTemplateEngine throws, the catch block returns without closing the logger, leaking the resource. Consider using a finally block (or closing in each exit path) to ensure logger.close() is always called.

🧹 Proposed fix — use `finally` for logger cleanup

+    let output: string;
+    let report: LoggerReport | null = null;
     try {
-      // ... engine branches ...
+      // ... engine branches (unchanged) ...
 
       // Get logger report
       report = logger.getReport();
-      logger.close();
 
       // ... dev log + return success ...
     } catch (generationError) {
       console.error('[API] Generation error:', generationError);
       return ApiResponse.error(
         `Generation failed: ${ApiResponse.extractMessage(generationError)}`,
         422,
         ApiResponse.extractDetails(generationError),
       );
+    } finally {
+      logger.close();
     }

packages/lib/src/index.ts (1)

85-97: ⚠️ Potential issue | 🟡 Minor

Use console.error for error output.

Line 90 logs errors to stdout via console.log(err). Errors should go to stderr.

Proposed fix

-        console.log(err);
+        console.error(err);

packages/lib/src/cli.ts (1)

118-123: ⚠️ Potential issue | 🟠 Major

Pre-existing: missing return after recursive mainMenu() when no files found.

When files.length === 0, mainMenu() is called but execution falls through to inquirer.prompt with an empty choices array. The dumpTree branch (Line 158) correctly has return after the same pattern.

Suggested fix

     if (files.length === 0) {
       console.log('No files found in the example directory.');
       await mainMenu();
+      return;
     }

🤖 Fix all issues with AI agents

In `@packages/lib/src/cli.ts`:
- Around line 19-27: The loadVariables function currently swallows parse/read
errors and returns {}, causing corrupted variables to propagate into runModel
for interactive paths; change loadVariables (in packages/lib/src/cli.ts) to
throw the read/parse error instead of returning {} so callers must handle
failures (i.e., replace the catch to rethrow or throw a new Error with context).
Also update the call-sites that invoke loadVariables for the MODE=last branch
and the interactive branches (the code paths checking action === 'last' and
action === 'run' which pass the result into runModel) to catch the thrown error
and exit/return with a clear error message instead of letting an empty object
reach runModel. Ensure references to loadVariables and runModel are used to
locate the changes.

In `@packages/lib/src/index.ts`:
- Around line 78-87: The call to sleecTemplateEngine is being passed tree.nodes
(an array) but sleecTemplateEngine expects the full GoalTreeType so functions
like GoalTree.allByType(tree, 'task') work; update the writeFile invocation to
pass the entire tree object (the variable tree produced by GoalTree.fromModel
and used with Model.load) instead of tree.nodes so sleecTemplateEngine receives
the full structure.

🧹 Nitpick comments (1)

packages/lib/src/index.ts (1)

1-10: Consider separating the CLI entry point from the barrel export.

The shebang (#!/usr/bin/env node) and the CLI block (lines 64–97) live in the same file as all the public re-exports. This couples two distinct responsibilities: being the package's public API surface and being an executable script. A dedicated cli.ts entry point (referenced via "bin" in package.json) would make each concern independently testable and avoid shipping CLI-only imports (like writeFile, path) to library consumers who tree-shake.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

loadVariables silently returns {} on error, but only one call-site validates the result.

The MODE=last path (Lines 73-84) correctly checks for an empty object, but the interactive action === 'last' path (Line 116) and action === 'run' path (Line 149) both pass the result of loadVariables directly into runModel without validation. If the file is corrupt or unreadable (despite existing), an empty {} will silently flow into the PRISM generator.

Consider either:

1. Having loadVariables throw on error (fail-fast), or
2. Adding the same empty-check after each loadVariables call.

Option 1: Fail-fast approach

 const loadVariables = (modelPath: string): Record<string, boolean | number> => {
   const variablesFilePath = getVariablesFilePath(modelPath);
-  try {
-    return JSON.parse(readFileSync(variablesFilePath, 'utf8'));
-  } catch (error) {
-    console.error('Error reading variables file:', error);
-    return {};
-  }
+  if (!existsSync(variablesFilePath)) {
+    throw new Error(`Variables file not found: ${variablesFilePath}`);
+  }
+  return JSON.parse(readFileSync(variablesFilePath, 'utf8'));
 };

🤖 Prompt for AI Agents

In `@packages/lib/src/cli.ts` around lines 19 - 27, The loadVariables function
currently swallows parse/read errors and returns {}, causing corrupted variables
to propagate into runModel for interactive paths; change loadVariables (in
packages/lib/src/cli.ts) to throw the read/parse error instead of returning {}
so callers must handle failures (i.e., replace the catch to rethrow or throw a
new Error with context). Also update the call-sites that invoke loadVariables
for the MODE=last branch and the interactive branches (the code paths checking
action === 'last' and action === 'run' which pass the result into runModel) to
catch the thrown error and exit/return with a clear error message instead of
letting an empty object reach runModel. Ensure references to loadVariables and
runModel are used to locate the changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for sleecTemplateEngine definition
echo "=== Searching for sleecTemplateEngine definition ==="
rg -n "sleecTemplateEngine" --type=ts -C 5

echo ""
echo "=== Check package.json for module type ==="
head -30 package.json

Repository: vieirin/goal-controller

Length of output: 9337

---

🏁 Script executed:

#!/bin/bash
# Find GoalTree class definition to understand tree vs tree.nodes
echo "=== GoalTree class definition ==="
rg -n "class GoalTree" --type=ts -A 10

echo ""
echo "=== Check how tree.nodes is used elsewhere ==="
rg -n "tree\.nodes" --type=ts | head -20

echo ""
echo "=== Verify GoalTreeType definition ==="
rg -n "type GoalTreeType" --type=ts -A 2

Repository: vieirin/goal-controller

Length of output: 2642

---

Pass the full tree object to sleecTemplateEngine, not tree.nodes.

The sleecTemplateEngine function expects a GoalTreeType object and calls GoalTree.allByType(tree, 'task') internally, which requires the full tree. Currently, the code passes tree.nodes (an array) instead of the tree object. The UI implementation at packages/ui/app/api/transform/route.ts:82 correctly passes the full tree object.

🤖 Prompt for AI Agents

In `@packages/lib/src/index.ts` around lines 78 - 87, The call to
sleecTemplateEngine is being passed tree.nodes (an array) but
sleecTemplateEngine expects the full GoalTreeType so functions like
GoalTree.allByType(tree, 'task') work; update the writeFile invocation to pass
the entire tree object (the variable tree produced by GoalTree.fromModel and
used with Model.load) instead of tree.nodes so sleecTemplateEngine receives the
full structure.