fix(auto-fix): detect rickyWorkflow* alias imports via TypeScript AST#86
Merged
fix(auto-fix): detect rickyWorkflow* alias imports via TypeScript AST#86
Conversation
…substring
`injectWorkflowEnvLoader` decides whether to add
`import * as rickyWorkflowFs from 'node:fs'` and
`import * as rickyWorkflowPath from 'node:path'` aliases by checking
whether the substring `from 'node:fs'` (or `node:path`) appears anywhere
in the artifact text. Master-rendered workflows (master-workflow-renderer.ts:138-149)
emit a `node --input-type=module` HEREDOC inside a
`.step({ command: ... })` body that contains literal
`import { mkdirSync, writeFileSync } from 'node:fs'` /
`import { dirname } from 'node:path'` lines as part of the embedded
shell script. The substring check matches that string literal and skips
adding the real top-level alias imports. The injected
`loadRickyWorkflowEnv` body still references `rickyWorkflowFs` and
`rickyWorkflowPath`, so Node throws
`ReferenceError: rickyWorkflowPath is not defined` at module load and
the auto-fix loop burns 7/7 attempts on
`UNSUPPORTED_RUNTIME at runtime-launch`.
Repro from the user's session (after PR #84 unblocked the artifact
write so this latent bug could surface):
Generation: ok — workflows/generated/ricky-...ts
Workflow name: wf-1e14175ec3b5
Execution: blocked — UNSUPPORTED_RUNTIME at runtime-launch
Cause: node --experimental-strip-types exited with code 1
Auto-fix: stopped after 7/7 attempt(s) (UNSUPPORTED_RUNTIME)
ReferenceError: rickyWorkflowPath is not defined
at loadRickyWorkflowEnv (workflows/generated/ricky-...ts:10:18)
Fix: introduce `hasRickyWorkflowAliasImport(content, alias, moduleName)`
which matches an actual top-of-file
`import * as <alias> from '<module>'` statement (anchored to line start
with the multiline flag). Substring matches inside HEREDOC strings no
longer count, so the alias imports always get added when the helpers
that reference them are.
Tests:
- `src/local/auto-fix-loop.test.ts` — new regression case
"adds the rickyWorkflow* alias imports even when the workflow embeds
`from 'node:fs'` inside a .step command HEREDOC" reproduces the
master-rendered shape (`.step({ command: "node --input-type=module
<<'NODE'\\nimport { mkdirSync, ... } from 'node:fs';\\n..." })`) and
asserts the top-level aliases land while the HEREDOC stays untouched.
Existing env-loader injection test still passes.
Evals:
- `evals/suites/runtime-recovery/cases.md` — new
`runtime-recovery.env-loader-injection-runtime-loadable` case captures
the behavioral requirement at the human-review layer (compiled into
cases.jsonl by `npm run evals:compile`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR fixes env-loader injection logic to use anchored, top-of-file import detection instead of substring matching. When repairing workflows with missing environment variables, the injector must add Node.js alias imports but avoid false positives from embedded HEREDOC command text. A new helper function validates actual import statements rather than string presence. ChangesEnv-Loader Deterministic Injection Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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
🤖 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 `@src/local/auto-fix-loop.ts`:
- Around line 755-769: The hasRickyWorkflowAliasImport function currently tests
importPattern against the whole file (using the 'm' flag) so a HEREDOC/template
literal with a left-anchored import line can produce a false positive; change
hasRickyWorkflowAliasImport to first extract the file preamble (scan lines from
the top and stop at the first line that is not a top-of-file
import/export/blank/comment statement), then run the existing importPattern.test
only against that preamble string; keep the same importPattern and
escapedAlias/escapedModule logic and update references in
hasRickyWorkflowAliasImport to perform the header-scan-and-slice before testing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 396c6af8-7009-409f-a684-ec416c0a6736
📒 Files selected for processing (4)
evals/suites/runtime-recovery/cases.jsonlevals/suites/runtime-recovery/cases.mdsrc/local/auto-fix-loop.test.tssrc/local/auto-fix-loop.ts
Contributor
Ricky Eval ReviewRun: Passed: 5 | Needs human: 43 | Reviewable: 43 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesThese cases passed deterministic checks and include captured Ricky output for a human verdict against their REVIEW
|
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 `@src/local/auto-fix-loop.ts`:
- Around line 768-786: The preamble-slicing logic uses line.length + 1 to
advance preambleLength which breaks on CRLF and can mis-slice content before
importPattern check (causing duplicate alias imports); update the loop that
computes preambleLength so it advances by the actual substring length including
the real newline separator instead of assuming a single-byte newline: iterate
over the original content with a cursor using content.indexOf to find the next
newline (handling "\r\n" and "\n"), compute the exact increment to add to
preambleLength, and keep the same trimmed checks for comment/import/export
markers; ensure this updated preambleLength is used with
importPattern.test(content.slice(0, preambleLength)) so alias imports are
reliably detected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3753c073-d40f-4756-b601-1ec06271a916
📒 Files selected for processing (1)
src/local/auto-fix-loop.ts
Follow-up to b9f9f47 (preamble-limited regex). Both this commit and b9f9f47 fix the same user-visible bug — `injectWorkflowEnvLoader` silently skipping the rickyWorkflowFs/rickyWorkflowPath alias imports when a master-rendered workflow embeds `from 'node:fs'` inside a `.step({ command: ... })` shell HEREDOC — but the regex/preamble approach is structurally fragile and has class-of-bug failure modes that AST-based detection eliminates for free: - Multi-line `import * as\n alias\n from 'mod'` declarations slip through a line-anchored regex and get re-injected as duplicates, which the strip-types loader rejects with `SyntaxError: Identifier has already been declared`. - A future template that emits an alias import below the file preamble (after a const, function, comment block boundary, etc.) would also be missed. - Comment-stripping heuristics ("starts with //", "starts with /*") miss inline `import { x } /* trailing */ from 'mod';` and other edge cases the parser owns. Replaces the regex with `ts.createSourceFile` + a walk over module- scope `ImportDeclaration` nodes, matching `import * as <alias> from '<module>'` structurally. Contents inside StringLiteral / NoSubstitutionTemplateLiteral / TemplateExpression nodes are inert, so HEREDOC strings stop fooling detection regardless of how the embedded shell text is escaped. `typescript` moves from devDependencies to dependencies. The bundler externalizes it (it is already used at runtime via the AST API), so `dist/ricky.js` stays at 2.4MB. Install footprint grows by ~23MB at `npm install` time but matches the dep weight ricky already carries through @agent-relay/sdk and @agentworkforce/harness-kit. Tests: - New regression case in `src/local/auto-fix-loop.test.ts`: "recognizes already-present rickyWorkflow* alias imports declared via multi-line statement and skips re-injection" — proves the AST upgrade meaningfully improves on the regex approach by handling a shape neither the original substring check nor the preamble regex could distinguish from "not imported." - Existing HEREDOC regression test still passes. - npm test green (1029 pass; the lone failing test in local-run-monitor.test.ts is a pre-existing flake — passes 3/3 in isolation, unrelated to this change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kjgbot
changed the title
2 tasks
This was referenced May 10, 2026
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
injectWorkflowEnvLoaderdecided whether to add theimport * as rickyWorkflowFs from 'node:fs'andimport * as rickyWorkflowPath from 'node:path'aliases by checking whether the substringfrom 'node:fs'(orfrom 'node:path') appeared anywhere in the artifact text. Master-rendered workflows embed those exact substrings inside a shell HEREDOC in a.step({ command: ... })body — seemaster-workflow-renderer.ts:138-149, where the master renderer emits anode --input-type=module <<'NODE'\nimport { mkdirSync, writeFileSync } from 'node:fs';\n…NODEblock as part of a deterministic step's command string.The substring check matched that string literal and skipped adding the real top-level alias imports. The injected
loadRickyWorkflowEnvbody still referencedrickyWorkflowFsandrickyWorkflowPath, so Node threwReferenceError: rickyWorkflowPath is not definedat module load and the auto-fix loop burned 7/7 attempts onUNSUPPORTED_RUNTIME at runtime-launch.Repro
After #84 unblocked the artifact write so this latent bug surfaces:
The generated workflow had
loadRickyWorkflowEnvdefined and called frommain(), but norickyWorkflowFs/rickyWorkflowPathimport at module scope — exactly because the substring check was fooled by the master renderer's embedded HEREDOC.Fix
Replace the substring check with
ts.createSourceFile+ a walk over module-scopeImportDeclarationnodes.hasRickyWorkflowAliasImport(content, alias, moduleName)now matchesimport * as <alias> from '<module>'structurally: contents insideStringLiteral,NoSubstitutionTemplateLiteral, andTemplateExpressionnodes are inert, so HEREDOC strings stop fooling detection regardless of how the embedded shell text is escaped. Comments are also inert.Why AST and not a tighter regex / preamble parser:
import * as\n alias\n from 'mod'declarations slip through any line-anchored regex and get re-injected as duplicates, which the strip-types loader rejects withSyntaxError: Identifier has already been declared. (See the newrecognizes already-present rickyWorkflow* alias imports declared via multi-line statement and skips re-injectiontest.)startsWith('//'),startsWith('/*')) miss inlineimport { x } /* trailing */ from 'mod';and other edge cases the parser owns for free.typescriptmoves fromdevDependenciestodependencies. The bundler externalizes it (already in the externals list now), sodist/ricky.jsstays at 2.4MB. Install footprint grows by ~23MB atnpm installtime, comparable to the dep weight ricky already carries through@agent-relay/sdkand@agentworkforce/harness-kit.Test plan
npm run typechecknpm test— 1029 / 1030 tests pass; the lone failure is a pre-existing flake inlocal-run-monitor.test.ts > exposes background monitor progress across subprocess status checks(race between checking subprocess state and the subprocess completing — passes 3/3 in isolation, unrelated to this change).src/local/auto-fix-loop.test.ts:adds the rickyWorkflow* alias imports even when the workflow embeds 'from \\'node:fs\\'' inside a .step command HEREDOC— pins the original bug.recognizes already-present rickyWorkflow* alias imports declared via multi-line statement and skips re-injection— exercises a shape that neither the original substring check nor the intermediate preamble regex could distinguish from "not imported", proving the AST upgrade is meaningful.runtime-recovery.env-loader-injection-runtime-loadableinevals/suites/runtime-recovery/cases.mdcaptures the behavioral requirement at the human-review layer (compiled intocases.jsonlvianpm run evals:compile).npm run build— bundle still emits 2.4MBdist/ricky.js;typescriptcorrectly resolved as an external.Commits
This PR has two commits that fix the same user-visible bug at successive levels of structural robustness:
b9f9f47 fix(auto-fix): limit alias import detection to file preamble— pragmatic intermediate fix using a line-anchored regex limited to the file preamble.cc2e2d9 refactor(auto-fix): use TypeScript AST for alias-import detection— final fix usingts.createSourceFile, eliminating the class of failure modes a regex/preamble approach is structurally vulnerable to.Reviewers may prefer to squash. The intermediate commit is preserved so the rationale for upgrading to AST is visible in history.
Relationship to other PRs
Stacks logically on top of #84 (persona-writer fallback) — that PR ensures the deterministic artifact actually gets written to disk; this PR ensures the deterministic artifact actually loads under Node. Either PR is independently valuable; together they unblock the user's
ricky --mode local --spec-file <md> --runflow when the workforce-persona writer is unavailable.🤖 Generated with Claude Code