feat(workflows/condition-evaluator): support shorthand path and unquoted numeric/boolean RHS by lraphael · Pull Request #1777 · coleam00/Archon

lraphael · 2026-05-27T10:08:07Z

Summary

Problem: when: clauses force the verbose $nodeId.output.field form and only accept single-quoted RHS literals — even for numbers, so $test.exit_code > '0' quotes a number as a string.
Why it matters: reduces friction when authoring conditions; numeric/boolean comparisons read naturally.
What changed: the atom regex now accepts $nodeId.field shorthand (≡ $nodeId.output.field) and bare numeric/boolean RHS (== 0, == true); evaluateAtom resolves the canonical-vs-shorthand path and picks quoted-or-unquoted RHS.
What did not change (scope boundary): the AND/OR splitter, resolveOutputRef, and the numeric coercion path are untouched. No new operators, parentheses, or function calls. Every currently-valid when: clause evaluates identically. Shorthand with a sub-field ($n.field.sub) is rejected fail-closed.

UX Journey

Before

Workflow author                  condition-evaluator
───────────────                  ───────────────────
writes when: clause
  $test.output.exit_code > '0' ─▶ atom regex requires `.output.`
  $test.output.passed == 'true'   and single-quoted RHS only
                                ◀─ numbers must be quoted as strings

After

Workflow author                  condition-evaluator
───────────────                  ───────────────────
writes when: clause
  $test.exit_code > 0          ─▶ *shorthand path accepted*
  $test.passed == true            *unquoted numeric/boolean RHS accepted*
  $test.output.exit_code > '0' ─▶ canonical + quoted forms still valid
                                ◀─ identical evaluation result

Architecture Diagram

Before

dag-executor.ts ──▶ evaluateCondition() ──▶ splitOutsideQuotes() ──▶ evaluateAtom()
                                                                        │
                                                                        ▼
                                                                   resolveOutputRef()

After

dag-executor.ts ──▶ evaluateCondition() ──▶ splitOutsideQuotes() ──▶ evaluateAtom() [~]
                                                                        │  (regex + path/RHS
                                                                        │   resolution updated)
                                                                        ▼
                                                                   resolveOutputRef()  (unchanged)

Connection inventory:

From	To	Status	Notes
dag-executor	evaluateCondition	unchanged	call site untouched
evaluateCondition	evaluateAtom	unchanged
evaluateAtom	resolveOutputRef	unchanged	shorthand resolves to same `field` arg

Label Snapshot

Risk: risk: low
Size: size: S
Scope: workflows
Module: workflows:condition-evaluator

Change Metadata

Change type: feature
Primary scope: workflows

Linked Issue

Closes feat(workflows/condition-evaluator): support shorthand path $nodeId.field and unquoted numeric/boolean RHS #1763

Validation Evidence (required)

bun test packages/workflows/src/condition-evaluator.test.ts   # 69 pass / 0 fail
bun --filter @archon/workflows type-check                      # ok
bun run lint                                                   # ok (--max-warnings 0)
prettier --check (changed files)                               # ok

Evidence provided: 69 unit tests pass (54 existing + 15 new); type-check, lint, prettier all green.
End-to-end: ran a temporary all-bash:-node workflow through the CLI (workflow run --no-worktree) exercising the real DAG executor. condition_evaluated debug logs confirmed shorthand path resolves to the correct field and unquoted int/bool/decimal RHS evaluated correctly; the $probe.passed == false node was correctly skipped. Temp workflow removed afterward.

Security Impact (required)

New permissions/capabilities? No
New external network calls? No
Secrets/tokens handling changed? No
File system access scope changed? No

Compatibility / Migration

Backward compatible? Yes
Config/env changes? No
Database migration needed? No

Human Verification (required)

Verified scenarios: shorthand path ($n.field), unquoted integer/decimal/boolean RHS, canonical .output. form still valid, mixed quoted+unquoted in AND/OR compounds, both unit-tested and run through the live DAG executor.
Edge cases checked: sub-field rejection on shorthand ($n.field.sub → fail-closed), empty quoted RHS (== ''), negative integers, false-case skip.
What was not verified: AI-backed node types (not relevant — change is purely in the condition string parser).

Side Effects / Blast Radius (required)

Affected subsystems/workflows: only when: clause evaluation on DAG nodes.
Potential unintended effects: a previously-unparseable expression that now parses — mitigated by the regex being a strict superset and full regression coverage.
Guardrails/monitoring: condition_parse_failed / condition_evaluated debug logs already in place.

Rollback Plan (required)

Fast rollback: revert this commit; the evaluator returns to the prior regex with no data or schema impact.
Feature flags or config toggles: none.
Observable failure symptoms: dag_node_skipped_condition_parse_error messages on previously-valid clauses (none expected).

Risks and Mitigations

Risk: regex change unintentionally alters evaluation of an existing clause.
- Mitigation: superset grammar + all 54 prior tests pass unchanged + live DAG e2e.

Summary by CodeRabbit

New Features
- Condition expressions accept a shorthand field reference form and allow unquoted numeric and boolean literals in comparisons.
- Compound expressions mixing quoted and unquoted values for &&/|| are supported.
Bug Fixes / Safety
- Deep chained shorthand references are rejected to avoid ambiguous resolution.
- Missing shorthand fields resolve to an empty value without error.
Tests
- Expanded coverage for shorthand parsing, unquoted literals, comparisons, and compound expressions.

coderabbitai · 2026-05-27T10:09:33Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fb657237-dd1d-4f70-ac8a-5a4cfe773622

📥 Commits

Reviewing files that changed from the base of the PR and between 457798c and 23c9f5c.

📒 Files selected for processing (1)

packages/workflows/src/condition-evaluator.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

packages/workflows/src/condition-evaluator.test.ts

📝 Walkthrough

Walkthrough

Extends the workflow condition evaluator to accept shorthand node paths ($nodeId.field as alias for $nodeId.output.field) and unquoted numeric/boolean RHS literals; updates the atom regex and evaluateAtom logic and adds tests covering shorthand equivalence, absent-field behavior, sub-field rejection, unquoted literals, and compound expressions.

Changes

Condition Evaluator Shorthand and Unquoted Literals

Layer / File(s)	Summary
Regex pattern, documentation, and core atom evaluation logic `packages/workflows/src/condition-evaluator.ts`	Updated when: expression header docs; expanded `atomPattern` to capture canonical `$nodeId.output[.field]` and shorthand `$nodeId.field` plus quoted-string or unquoted numeric/boolean RHS; reworked `evaluateAtom` to resolve effective field, reject shorthand sub-field chaining fail-closed, and select RHS from quoted vs unquoted captures.
Shorthand path, unquoted literal, and compound expression tests `packages/workflows/src/condition-evaluator.test.ts`	Added tests for shorthand `$node.field` equivalence to `$node.output.field`, structuredOutput resolution, numeric operator support on shorthand, fail-closed chained shorthand sub-fields, absent-shorthand-field behavior (`''` with `parsed: true`), unquoted integer/decimal/negative/boolean RHS parsing, canonical-form parity, and AND/OR compound expressions mixing quoted and unquoted RHS.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

bug(workflows/condition-evaluator): when: silently false on non-primitive field access, hides upstream output bugs #1770: Modifies the same condition-evaluator parsing and resolution logic and may overlap with shorthand/unquoted-literal semantics.

Possibly related PRs

coleam00/Archon#1654: Related changes to condition-evaluator output resolution behavior (structuredOutput preference) that interact with field-resolution semantics introduced here.

Poem

🐰
I hopped through regex, quick and spry,
$node.field now meets the eye.
Numbers bare and booleans true,
Conditions parse — the fields say who.
A tiny hop, the runner's through.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely summarizes the main feature additions: shorthand path syntax and unquoted numeric/boolean RHS support.
Description check	✅ Passed	The PR description comprehensively follows the template with all major sections completed: summary with problem/impact/changes/scope, UX journey diagrams, architecture diagram, metadata, validation evidence, security impact, compatibility, human verification, side effects, and rollback plan.
Linked Issues check	✅ Passed	The PR implementation fully satisfies issue `#1763` requirements: shorthand path `$nodeId.field` accepted, unquoted numeric/boolean RHS supported, atom regex and evaluateAtom updated as specified, AND/OR splitter and resolveOutputRef left unchanged, backward compatible, shorthand sub-fields rejected fail-closed, and comprehensive test coverage provided.
Out of Scope Changes check	✅ Passed	All changes are within scope: modifications limited to the atom regex pattern and evaluateAtom resolution logic in condition-evaluator.ts, with corresponding test additions. No alterations to AND/OR splitter, resolveOutputRef, numeric coercion, or introduction of new operators/parentheses.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

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

coderabbitai

Actionable comments posted: 2

🤖 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 `@auth-service/server.js`:
- Around line 159-162: The redirect builds an absolute Location from unvalidated
headers (x-forwarded-proto and x-forwarded-host) in the request handler where
proto, host, base, and safeRd are used; validate and normalize proto to only
"http" or "https" (default to "https" if invalid), validate host against a safe
pattern (hostname with optional port) or fall back to req.headers['host'] or a
configured canonical host, and construct the redirect URL using a safe URL
builder (e.g., URL) ensuring host and rd are properly encoded before calling
res.writeHead(302, { Location: ... }); replace the direct template string with
this validated/normalized approach to prevent malformed or unsafe Location
values.

In `@docker-compose.yml`:
- Around line 72-73: Update docker-compose.yml to use Docker compose required
variable interpolation for DOMAIN and ACME_EMAIL so parsing fails early: replace
all occurrences of ${DOMAIN} with ${DOMAIN:?DOMAIN is not set} and ${ACME_EMAIL}
with ${ACME_EMAIL:?ACME_EMAIL is not set}; specifically update the traefik label
keys like traefik.http.routers.archon.rule and any traefik.certresolver/email
labels (and the other two places where DOMAIN/ACME_EMAIL are used) to use the :?
form so compose-parse fails fast when those env vars are missing.

🪄 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: ee47bfc3-6203-4176-a009-b53b4e522588

📥 Commits

Reviewing files that changed from the base of the PR and between ac18034 and 4f9e2d7.

📒 Files selected for processing (7)

.env.example
auth-service/server.js
docker-compose.yml
packages/workflows/src/condition-evaluator.test.ts
packages/workflows/src/condition-evaluator.ts
traefik-dynamic.yml
traefik.yml

…ted numeric/boolean RHS Accept `$nodeId.field` as shorthand for `$nodeId.output.field`, and allow bare numeric/boolean RHS (`$n.exit_code == 0`, `$n.passed == true`) so number and flag comparisons no longer have to quote their literal. The change is confined to the atom regex plus the path/RHS resolution in evaluateAtom; the AND/OR splitter, resolveOutputRef, and numeric coercion path are untouched, so every existing `when:` clause evaluates identically. Shorthand refs with a sub-field (`$n.field.sub`) are rejected fail-closed. Closes coleam00#1763

Wirasm · 2026-05-27T14:11:01Z

Review Summary

Verdict: blocking-issues

This PR adds two useful features to the condition evaluator — shorthand path syntax ($node.field) and unquoted numeric/boolean RHS — and includes 14 new test cases covering both. However, the implementation changes to condition-evaluator.ts (the new regex pattern and rewritten evaluateAtom) appear to be absent from the working tree. Running the test suite will fail. Fix the production file first, then clean up two stale task references in the test comments.

Blocking issues

packages/workflows/src/condition-evaluator.ts:126–128: The production file still has the old regex pattern. Shorthand paths and unquoted RHS will be rejected by the old pattern — none of the new test cases will pass as-is.
Fix: Apply the new atomPattern regex from the diff (the one that handles 6 capture groups and the unquoted RHS alternative).
packages/workflows/src/condition-evaluator.ts:130–190: evaluateAtom still destructures 4 capture groups. The new tests pass expressions requiring the 6-group version. Every new test case hits condition_parse_failed and returns parsed: false.
Fix: Apply the evaluateAtom rewrite — destructure all 6 groups, handle canonical vs. shorthand field resolution, call resolveOutputRef with the resolved field name.

Suggested fixes

condition-evaluator.test.ts:513, 528: Section comments still reference task #1763. Remove the task prefix — the test names already describe the feature. Search for // --- #1763 and strip the prefix.
condition-evaluator.ts:173 (shorthand path, absent field): Add a test case to verify that $n.missing == 'x' returns result: false when the field key is absent from the JSON output.

Minor / nice-to-have

condition-evaluator.ts:139–140: The three-bullet comment restates the code structure rather than surfacing the key constraint. Trim to: // Shorthand (segment1 ≠ 'output') rejects a third segment fail-closed.
condition-evaluator.ts:134: Capture group 6 comment should note that both groups 5/6 may be undefined on a failed match, not just on success.

Compliments

The test suite is well-structured: happy paths, error paths (parsed: false), and compound expressions are all covered. The explicit parsed: false assertions confirm the fail-closed contract throughout.
The top-of-file feature list in condition-evaluator.ts remains accurate and serves as a useful capability reference.

Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality.

…absent-field case Address review feedback: remove the stale task-id prefix from the shorthand/unquoted RHS section comments, and add a test asserting an absent shorthand field resolves to '' (parsed: true) rather than a parse error, matching the canonical $node.output.field semantics.

lraphael · 2026-05-27T14:26:28Z

Thanks for the thorough review! A couple of clarifications.

On the two "blocking" items — the condition-evaluator.ts changes are part of this PR; I think the review ran against a stale/empty working tree. On 457798c both are already in place:

atomPattern (regex): the new 6-group pattern is committed — it captures $nodeId.(segment1)(.segment2)? OP ('value' | unquoted), including the unquoted-RHS alternative '([^']*)'|(-?\d+(?:\.\d+)?|true|false).
evaluateAtom: already destructures all 6 groups (nodeId, segment1, segment2, operator, quotedValue, unquotedValue), resolves canonical-vs-shorthand field paths, rejects shorthand sub-fields fail-closed, and selects quoted-or-unquoted RHS.

bun test src/condition-evaluator.test.ts passes locally — 70/70 green. Could you re-run against the current head? I suspect the production diff just wasn't picked up.

On the two suggested fixes — both applied in 23c9f5c8:

Removed the #1763: task prefix from the section comments in the test file.
Added a shorthand absent-field test: $n.missing == 'x' → result: false, parsed: true (resolves to '', mirroring the canonical $node.output.field empty-string semantics — so it's intentionally not a parse error).

The minor comment-wording nits I'm leaving as-is for now, since the capture-group docblock already covers the group semantics — happy to trim if you feel strongly.

coderabbitai Bot reviewed May 27, 2026

View reviewed changes

Comment thread auth-service/server.js Outdated

Comment thread docker-compose.yml Outdated

lraphael force-pushed the feat/condition-evaluator-shorthand-unquoted-rhs branch from 4f9e2d7 to 457798c Compare May 27, 2026 10:14

Wirasm merged commit 8d35386 into coleam00:dev May 29, 2026
4 checks passed

coderabbitai Bot mentioned this pull request Jun 5, 2026

feat(workflows): reliable cross-provider structured output — validate + repair + fail-fast (PR 1/2) #1883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(workflows/condition-evaluator): support shorthand path and unquoted numeric/boolean RHS#1777

feat(workflows/condition-evaluator): support shorthand path and unquoted numeric/boolean RHS#1777
Wirasm merged 2 commits into
coleam00:devfrom
lraphael:feat/condition-evaluator-shorthand-unquoted-rhs

lraphael commented May 27, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 27, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Wirasm commented May 27, 2026

Uh oh!

lraphael commented May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lraphael commented May 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

UX Journey

Before

After

Architecture Diagram

Before

After

Label Snapshot

Change Metadata

Linked Issue

Validation Evidence (required)

Security Impact (required)

Compatibility / Migration

Human Verification (required)

Side Effects / Blast Radius (required)

Rollback Plan (required)

Risks and Mitigations

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Wirasm commented May 27, 2026

Review Summary

Blocking issues

Suggested fixes

Minor / nice-to-have

Compliments

Uh oh!

lraphael commented May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lraphael commented May 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 27, 2026 •

edited

Loading