chore(workflows): remove double-quoted $node.output refs (fixes #2242) - #2262
chore(workflows): remove double-quoted $node.output refs (fixes #2242)#2262kagura-agent wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe workflows revise Bash handling for injected node outputs and extraction failures. The experimental issue-fix workflow also changes issue retrieval, implementation dependencies, reviewer conditions, and synthesis timing. Release steps use local variables for parsed values. ChangesWorkflow output cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Review: #2262 — remove double-quoted
|
| Command | Result |
|---|---|
bun packages/cli/src/cli.ts validate workflows |
0 bash double-quote warnings. 50 valid / 1 error / 2 warnings. Residual: pre-existing MCP-config ERROR in archon-smart-pr-review (.archon/mcp/ntfy.json missing), plus unrelated WARNINGs in e2e-opencode-all-nodes-smoke (opencode hooks) and repo-triage (Task→Agent rename). All three pre-date this PR. |
bun run cli workflow run e2e-deterministic --no-worktree "smoke test" |
PASS. 8 nodes, branch-false correctly skipped, verify-all printed PASS: all deterministic nodes produced output. Exit clean. |
| Purpose-built probe workflow (empty-output semantics) | Confirmed the behavior change — see I0 below. |
| Purpose-built probe workflow (40 KB output, spill path) | Confirmed word splitting — see I1 below. |
| Purpose-built probe workflow (small multi-word output, control) | Confirmed the inline path is safe — see I1 below. |
Probe workflows were created and deleted inside my worktree only; working tree left clean.
No AI-credentialed workflow was run (zero token cost).
Findings
Critical
None.
I0 — Behavior change confirmed (this is a FIX, not a bug)
The light pass's reading is correct, and I verified it empirically rather than by inspection.
Substitution source: packages/workflows/src/dag-executor.ts:662 (shellQuote) and
:749 / :734 (the '' empty return).
Probe result:
OLD_LEN=2 NEW_LEN=0
OLD: assert SILENTLY PASSES
NEW: assert FIRES
old="$empty-node.output"→oldis 2 chars (two literal') →[ -z "$old" ]false → assert vacuous.new=$empty-node.output→newis 0 chars →[ -z "$new" ]true → assert fires.
So every [ -z ... ] non-emptiness assert across these smoke files was previously dead code.
This PR makes them live. That is the correct and desirable outcome.
Will any of the 17 newly FAIL on a legitimately-empty output? No. I checked every consumer:
- The only route to a legitimate empty is
resolution.kind === 'empty', which
dag-executor.ts:747documents as reachable only for an author-declared-optional field.
Every structured-output schema in the changed set marks its accessed fields required —
e2e-copilot-all-nodes-smoke.yaml:63(required: [status, value]) and
e2e-minimax-smoke.yaml:61(required: [name, ok]). A missing required field throws
OutputRefErrorinstead of resolving empty, so the empty branch is unreachable for them. - Every remaining assert reads a whole-node
$n.output, which is empty only if the node genuinely
produced nothing — in which case firing is exactly what the assert exists to do. e2e-deterministic's script nodes (.archon/scripts/echo-args.js,.archon/scripts/echo-py.py)
bothconsole.log/printunconditionally, so they can't emit empty. Confirmed by the live run.
Important
I1 — 8 bare-arg (non-assignment) sites word-split on the >32 KB spill path
packages/workflows/src/dag-executor.ts:681 returns a bare, unquoted $(cat '/path') for
values over NODE_OUTPUT_FILE_THRESHOLD (32 KB, :2370). The validator's own comment
(packages/workflows/src/validator.ts:675-677) is explicit that for the large case
"double-quoting is actually fine" — the double quotes this PR removed were the only thing
protecting these sites. Its hint recommends the assignment form specifically because
assignment context suppresses word splitting.
The PR used the assignment form nearly everywhere, but left 8 sites in bare-argument position:
| File:line | Form |
|---|---|
.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml:132-142 |
check <name> $n.output (11 calls) |
.archon/workflows/test-workflows/e2e-pi-all-nodes-smoke.yaml:93-101 |
check <name> $n.output (9 calls) |
.archon/workflows/e2e-opencode-inline-multi-agents.yaml:44-46 |
echo $n.output | grep -q ... |
.archon/workflows/e2e-opencode-smoke.yaml:17 |
echo $simple.output | grep -q "OPENCODE_OK" |
.archon/workflows/experimental/archon-fix-github-issue-{codex,experimental,minimax}.yaml (:77/:65/:76) |
$(echo $extract-issue-number.output | tr -d ...) |
.archon/workflows/maintainer/maintainer-review-pr.yaml:54 |
$(echo $extract-pr-number.output | tr -d ...) |
Probe, 40 KB output through the real engine:
ASSIGNED_LEN=40012 # var=$n.output → intact
check[bigarg]: argc=3 arg2len=40000 # check name $n.output → SPLIT: $2 truncated, $3 spawned
check[bigarg-quoted]: argc=2 arg2len=40012 # check name "$var" → intact
GREP_UNQUOTED: match
Control at small size (multi-word, embedded newline, embedded *):
check[bare]: argc=2 arg2len=51 arg2=[multi word value with * glob and spaces\nsecond line]
check[quoted]: argc=2 arg2len=51 arg2=[multi word value with * glob and spaces\nsecond line]
So the hazard is exclusively the >32 KB spill path; the inline path is single-quoted by
shellQuote and fully safe, including against glob expansion.
Can it actually bite these nodes today? Practically no. The check() helpers
(e2e-copilot-all-nodes-smoke.yaml:120-127, e2e-pi-all-nodes-smoke.yaml:84-90) only test
[ -z "$2" ], and word splitting leaves $2 non-empty; the grep -q patterns are single tokens
with no whitespace, so they survive splitting too. These are smoke tests with tiny outputs.
Why it still matters: the form is now unsafe-by-construction and silently correct only because
the assertions happen to be weak. Any future edit that needs the exact value at those sites gets a
silent corruption, and glob expansion against cwd is live for any spilled output containing a bare
*. The same trap already exists pre-existing at archon-release.yaml:331
(printf '%s' $draft-changelog.output) — where a changelog draft can plausibly exceed 32 KB.
Fix: mirror what the PR already did elsewhere —
v=$n.output then check <name> "$v" / echo "$v" | grep -q ....
Suggestions
S1 — Stale comment introduced by this PR
.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml:116-119 still reads:
"value-equality on string fields is avoided on purpose — shellQuote() wraps strings in literal
single quotes, so a literal[ "$x" != "ok" ]would always fail."
That rationale was true of the old x="$n.output.status" form. Under the new
status_val=$structured-node.output.status (line 100) the variable holds ok with no literal
quotes, so an equality check would now work correctly. The comment now argues for a restriction
that no longer applies.
S2 — Dedented echo in two of three sibling files
.archon/workflows/experimental/archon-fix-github-issue-codex.yaml:80 and
.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml:68 place the echo at
6-space indent inside an 8-space block; archon-fix-github-issue-minimax.yaml:79 got it right.
Harmless in bash (and the YAML block scalar still parses — I confirmed all three load), but it is a
visible automated-edit artifact across otherwise byte-identical siblings.
S3 — Sweep is complete for the warned form, but siblings now diverge
The validator regex (packages/workflows/src/validator.ts:687) only matches double-quoted
refs. The analogous single-quote-embedded form is unflagged and still present at
.archon/workflows/test-workflows/e2e-pi-all-nodes-smoke.yaml:64
(bash: "echo 'downstream got: $prompt-node.output'"), plus e2e-deterministic.yaml:24 and :40.
This PR did clean exactly that pattern in the two sibling all-nodes-smoke files
(e2e-copilot-all-nodes-smoke.yaml:88, e2e-opencode-all-nodes-smoke.yaml:85) but left Pi's
untouched — so three files that were deliberately parallel now differ. Not a correctness bug
(the injected 'value' quotes happen to merge with the surrounding ones), but worth aligning.
S4 — Regression guard (item 5): warranted, and cheap
Confirmed: bun run validate (package.json:38) runs nine checks, none of which is
validate workflows, and grep -rn "validate workflows" .github/ returns nothing. So the warning
class this PR just zeroed has no guard and will silently re-accumulate.
Cheapest form, in preference order:
- Append
&& bun run cli validate workflowsto thevalidatescript — one line, runs in the
existing CI job, no new workflow file. Needs the command to exit non-zero on errors only
(it currently must not fail on the pre-existingarchon-smart-pr-reviewMCP error, so either
fix that file first or add a--warnings-as-errors-style flag and gate on warnings only). - Alternatively a tiny
bun testcase inpackages/workflowsthat globs.archon/workflows/**
and assertsvalidateWorkflowResources()yields zerobash-field warnings. Fully hermetic,
no CLI/exit-code plumbing, and it fails on the PR that introduces the regression.
Given the archon-smart-pr-review error blocks option 1 today, option 2 is the cheapest real guard.
Item 4 — post-merge risk from e2e-smoke.yml not running on PRs
Confirmed: .github/workflows/e2e-smoke.yml:3-5 triggers on push: [main, dev] and
workflow_dispatch only — there is no pull_request trigger. So e2e-deterministic, a file this
PR edits, is not exercised by this PR's own checks and would first run post-merge on the dev push.
Concrete risk: LOW, and now empirically retired. I ran e2e-deterministic on the PR head and it
passes (full node-by-node output above). The other tiers are unaffected either way:
e2e-claude/e2e-codex/e2e-mixed are opt-in (run_ai_tiers / RUN_AI_SMOKE, deliberately
disabled because CI has no funded keys), and e2e-pi/e2e-copilot/e2e-minimax/e2e-opencode
are not wired into CI at all. e2e-container is path-gated and untouched by this PR. So the only
CI-visible file is the one I verified by execution.
Residual exposure is to maintainers running the AI smokes manually — and there the change is
strictly an improvement (previously-vacuous asserts now actually assert).
What the light triage pass got wrong / left open
- Nothing it asserted was wrong. Sweep completeness (0 bash warnings at head) and parse/
bash -n
safety both reproduce. Its I0 reading of thex="''"→x=''change is correct; I confirmed it
through the real engine rather than by inspection. - What it under-called: it flagged the empty-value change as a risk but did not resolve it. The
answer is that no workflow in the 17 newly fails, because every field-access consumer declares
its fieldsrequired(so the optional-field empty branch is unreachable) — that needed the schemas
read, not just the bash. - What it missed entirely: I1's actual mechanism.
bash -nwith substitution simulated cannot
see word splitting, because the simulation substitutes a quoted small value; the split only appears
on the real >32 KB$(cat ...)path. Also missed S1 (the comment this PR invalidated) and S3 (the
sibling divergence). - Its raw counting caveat: a naive line-grep over the changed files on
origin/devshows ~100
matching lines, vs the validator's 19 warnings. Not a contradiction — the validator emits one
warning per node and only scansbash:/loop.until_bashfields, not prompt bodies.
Strengths
- Correct root-cause diagnosis, and the remedy matches the validator's own documented hint
(validator.ts:696) rather than inventing a new convention. - Scope discipline: 17 non-shipped files, zero engine/test/bundled-default churn, so no
generate:bundledregeneration is needed and the blast radius is genuinely nil for users. - The negative test's semantics are preserved correctly —
e2e-structured-output-failfast.yaml:29-30still throws at substitution time (theOutputRefError
fires on the ref regardless of quoting), so the fail-fast smoke still proves what it claims. e2e-copilot-all-nodes-smoke.yaml:88ande2e-opencode-all-nodes-smoke.yaml:85were upgraded from
a genuinely fragile single-quote-embedded form to a clean assign-then-quote — that is a real
improvement beyond just silencing the warning.
Recommendation
Merge is safe. Ask for I1 (8 bare-arg sites → assignment form) in this PR or a fast follow-up, and
fold S1/S2/S3 in while touching those same files. S4 belongs in a separate issue — without it this
exact warning class re-accumulates, which is how it reached 19 in the first place.
|
Thanks for the incredibly thorough review @Wirasm — the probe-workflow testing is above and beyond. I1: Agreed, I'll convert the 8 bare-arg sites to the assignment form ( S1: Will update the stale comment. S4 (regression guard) I'll open as a separate issue since it involves the MCP-config blocker. Working on fixes now. |
|
Verified this end to end rather than trusting the warning count. On A nice thing fell out of the probes: the old form injected a 2-character value (two literal quote chars), so One change I'd like before merge. At 8 sites the ref sits in bare-argument or pipeline position rather than the Below the 32 KB inline threshold that's completely safe — the substitution is a single-quoted token, one shell word, and I confirmed it survives glob characters. Above the threshold the engine emits a bare unquoted It's harmless today — those helpers only test non-emptiness and grep whitespace-free tokens — but it's unsafe by construction, and the assignment form is immune at both sizes. Converting those 8 is mechanical and closes it properly rather than leaving a comment warning the next person. Two nits while you're in there, both trivial:
Follow-ups on us, not you:
Credit to @cnYui for the original diagnosis and fix pattern in #1953 — noting it here so it lands on the record when #2242 closes. |
|
Addressed all three review items — thanks for the thorough review @Wirasm! Change A — assignment form for bare-arg refs: Converted all 8 sites across the 4 e2e workflow files. Each bare v=$node.output
check "<name>" "$v"Change B — stale comment removed: The Change C — indentation: Already at 8 spaces after rebasing on latest Force-pushed with rebase on current |
939f520 to
65bd8d1
Compare
…m00#2242) Archon injects $node.output values pre-quoted (small values as single-quoted strings, large ones as $(cat) substitutions). Wrapping these in double quotes produces var="'value'" — embedding literal quote chars as data. Fix all 19 validator warnings by: - Removing double quotes from direct assignments: var=$node.output - Extracting to a shell variable before embedding in echo strings: val=$node.output; echo "prefix: $val" Validated: `archon validate workflows` reports 0 bash double-quote warnings after this change.
…mment and indent Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
65bd8d1 to
2f754b2
Compare
|
Rebased on latest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml (1)
424-425: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire the mandatory review result before synthesis.
code-reviewis mandatory, buttrigger_rule: one_successletssynthesizerun when any reviewer dependency completes, even ifcode-reviewfails. Useall_done, or use a rule that waits for all optional skipped reviewers and requirescode-reviewto complete successfully before synthesis runs.🤖 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 @.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml around lines 424 - 425, Update the synthesis workflow dependency rule associated with depends_on, including code-review, so synthesize waits for all reviewer outcomes and requires the mandatory code-review result to succeed before running. Replace trigger_rule: one_success with the appropriate all-completion/success condition while preserving support for optional skipped reviewers.
🤖 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 @.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml:
- Line 65: Update the issue-number extraction near ISSUE_NUM to first assign
extract-issue-number.output to a local quoted shell variable, then use that
variable quoted in the tr/grep pipeline and any related error logging. Preserve
the existing numeric extraction behavior while preventing unquoted expansion
from altering command arguments.
---
Outside diff comments:
In @.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml:
- Around line 424-425: Update the synthesis workflow dependency rule associated
with depends_on, including code-review, so synthesize waits for all reviewer
outcomes and requires the mandatory code-review result to succeed before
running. Replace trigger_rule: one_success with the appropriate
all-completion/success condition while preserving support for optional skipped
reviewers.
🪄 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 Plus
Run ID: 056182df-03bd-43ee-ac10-1310d57bc0ab
📒 Files selected for processing (17)
.archon/workflows/e2e-opencode-all-nodes-smoke.yaml.archon/workflows/e2e-opencode-inline-multi-agents.yaml.archon/workflows/e2e-opencode-smoke.yaml.archon/workflows/experimental/archon-fix-github-issue-codex.yaml.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml.archon/workflows/experimental/archon-fix-github-issue-minimax.yaml.archon/workflows/experimental/archon-release.yaml.archon/workflows/maintainer/maintainer-review-pr.yaml.archon/workflows/test-workflows/e2e-claude-smoke.yaml.archon/workflows/test-workflows/e2e-codex-smoke.yaml.archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml.archon/workflows/test-workflows/e2e-deterministic.yaml.archon/workflows/test-workflows/e2e-minimax-smoke.yaml.archon/workflows/test-workflows/e2e-mixed-providers.yaml.archon/workflows/test-workflows/e2e-pi-all-nodes-smoke.yaml.archon/workflows/test-workflows/e2e-pi-smoke.yaml.archon/workflows/test-workflows/e2e-structured-output-failfast.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- .archon/workflows/experimental/archon-release.yaml
- .archon/workflows/test-workflows/e2e-structured-output-failfast.yaml
- .archon/workflows/test-workflows/e2e-claude-smoke.yaml
- .archon/workflows/test-workflows/e2e-minimax-smoke.yaml
- .archon/workflows/maintainer/maintainer-review-pr.yaml
- .archon/workflows/test-workflows/e2e-mixed-providers.yaml
- .archon/workflows/e2e-opencode-inline-multi-agents.yaml
- .archon/workflows/test-workflows/e2e-codex-smoke.yaml
- .archon/workflows/test-workflows/e2e-pi-smoke.yaml
- .archon/workflows/test-workflows/e2e-copilot-all-nodes-smoke.yaml
- .archon/workflows/test-workflows/e2e-deterministic.yaml
- .archon/workflows/e2e-opencode-smoke.yaml
- .archon/workflows/experimental/archon-fix-github-issue-minimax.yaml
- .archon/workflows/experimental/archon-fix-github-issue-codex.yaml
- .archon/workflows/test-workflows/e2e-pi-all-nodes-smoke.yaml
| echo "parse-request returned an empty user_request — the parse step failed." >&2 | ||
| echo "This is a parser defect, not a bad request. Consider raising its model tier." >&2 | ||
| # Strip quotes, whitespace, markdown backticks from AI output | ||
| ISSUE_NUM=$(echo $extract-issue-number.output | tr -d "'\"\`\n " | grep -oE '[0-9]+' | head -1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file context =="
sed -n '50,80p' .archon/workflows/experimental/archon-fix-github-issue-experimental.yaml || true
echo
echo "== matching patterns and nearby assignments =="
rg -n '\$extract-issue-number\.output|ISSUE_NUM=|extract-issue-number|raw_issue_num|printf '\''%s'?\''' .archon/workflows/experimental/archon-fix-github-issue-experimental.yaml || true
echo
echo "== shell-behavior probe: unquoted command substitution in piped echo command line =="
python3 - <<'PY'
import subprocess
payloads = {
"glob_characters": "*?.[",
"leading_trailing_spaces": " 123 ",
"newline": "123\n456",
"multiple_words": "abc xyz",
}
for name, value in payloads.items():
unquoted = subprocess.run(["/bin/bash", "-c", """"ISSUE_NUM=$(echo """ + value + """ | tr -d "'""" + chr(96) + """"\"\`\\n ")"""], capture_output=True, text=True)
quoted = subprocess.run(["/bin/bash", "-c", """""raw_issue_num=""" + repr(value) + """
ISSUE_NUM=$(printf '%s\\n' "$raw_issue_num" | tr -d "'""" + chr(96) + """"\"\`\\n ")"""], capture_output=True, text=True)
print(f"--- {name} ---")
print("input", repr(value))
print("unquoted ISSUES_NUM:", repr(unquoted.stdout.rstrip("\n")), "stderr:", repr(unquoted.stderr))
print("quoted ISSUE_NUM:", repr(quoted.stdout.rstrip("\n")))
PYRepository: coleam00/Archon
Length of output: 2538
Assign $extract-issue-number.output to a quoted shell variable before extracting the issue number.
The unquoted command substitution expands through echo before tr runs, so whitespace, newlines, or glob-like output can change the arguments passed to the extraction pipeline or cause parsing failures. Use a local variable and quote it for both extraction and error logging.
🤖 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 @.archon/workflows/experimental/archon-fix-github-issue-experimental.yaml at
line 65, Update the issue-number extraction near ISSUE_NUM to first assign
extract-issue-number.output to a local quoted shell variable, then use that
variable quoted in the tr/grep pipeline and any related error logging. Preserve
the existing numeric extraction behavior while preventing unquoted expansion
from altering command arguments.
|
Closing. The sweep itself is sound; two things make it not worth landing in this form. 1. One file is a silent revert. It merges clean, so nothing flags it. Dev's copy of that file has zero validator warnings and was never in scope for a quote sweep. 2. The sweep introduced 3 regressions. Three Why close rather than ask for a fix. Both are individually fixable, but the timing argues against it: #2123 re-authors all of these files, so a sweep now is work done twice. And the probe showed the underlying rule is under-enforced — the validator catches double-quoted refs but not bare-arg, single-quoted, or So the durable fix is to extend the validator to the whole rule, then apply the idiom during the re-authoring. I have written that up on #2242 with the two-regime demonstration, so nothing here is lost. The other 16 files were verified structurally identical to dev — parsed node lists plus every |
2 participants
Problem
archon validate workflowsreports 19 bash double-quote warnings across test, experimental, and maintainer workflow files. When a bash node wraps$node.outputin double quotes (e.g.var="$node.output"), Archon's pre-quoting produces incorrect values likevar="'value'"— embedding literal quote characters as data.Fix
Remove double quotes from
$node.outputreferences in bash nodes:var="$node.output"→var=$node.outputChanges
17 workflow YAML files in
.archon/workflows/{experimental,maintainer,test-workflows,e2e-*}.Validation
All 19 bash double-quote warnings are resolved. No new errors introduced (the pre-existing MCP config error in
archon-smart-pr-reviewis unrelated).Closes #2242
Summary by CodeRabbit
Bug Fixes
Tests