Skip to content

Improve missing-env workflow recovery#49

Merged
khaliqgant merged 3 commits into
mainfrom
codex/fix-env-var-blocker-classification
May 5, 2026
Merged

Improve missing-env workflow recovery#49
khaliqgant merged 3 commits into
mainfrom
codex/fix-env-var-blocker-classification

Conversation

@khaliqgant

@khaliqgant khaliqgant commented May 5, 2026

Copy link
Copy Markdown
Member

Summary

  • Load repo-local .env.local/.env in generated workflows before Agent Relay steps run, preserving shell-export precedence.
  • Let auto-fix repair MISSING_ENV_VAR workflow failures by adding env loading and fast MISSING_ENV_VAR assertions instead of stopping immediately.
  • Tighten missing env extraction so prose acronyms like API/MSD are not suggested as bogus export commands.
  • Update Workforce persona prompts and shared workflow authoring rules with the env preflight pattern.

Test Plan

  • npm run typecheck
  • npm test

@coderabbitai

coderabbitai Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Improved detection/extraction of missing-environment messages, allow MISSING_ENV_VAR into the auto-fix path, add a deterministic preflight that injects a repo-local .env loader and assertions into generated workflows, update persona repair requirements and generation, and extend tests and docs.

Changes

Deterministic missing-env detection → auto-repair → generated workflow loader

Layer / File(s) Summary
Message Detection / Extraction
src/local/entrypoint.ts
Replaced broad regex with hasMissingEnvironmentMessage(text) and rewrote extractMissingEnvVars(text) to perform per-line, context-aware extraction; only accepts strict uppercase env-name tokens and excludes ENOENT/PATH.
Blocker Classification
src/local/entrypoint.ts
classifyCoordinatorBlocker now returns MISSING_ENV_VAR when extractMissingEnvVars yields candidates or hasMissingEnvironmentMessage matches.
Auto-fix Control Flow
src/local/auto-fix-loop.ts
Removed MISSING_ENV_VAR from external/unsafe blockers so it can be auto-repaired; expanded repairWorkflowDeterministically signature to accept optional response and run a new missing-env preflight.
Deterministic Repair Implementation
src/local/auto-fix-loop.ts
Adds repairMissingEnvVarPreflight(...): infers required env names from evidence/response, injects a repo-local .env/.env.local loader and optional assertRickyWorkflowEnv([...]) calls into the workflow source, and records deterministic diffs/changes.
Generated Workflow Runtime Helper
src/product/generation/template-renderer.ts
Adds renderWorkflowEnvLoaderHelper() generating loadRickyWorkflowEnv, unquoteRickyWorkflowEnvValue, and assertRickyWorkflowEnv; imports node:fs/node:path as rickyWorkflowFs/rickyWorkflowPath, and inserts loadRickyWorkflowEnv() at main() start.
Persona Repair Requirements & Writer
src/product/generation/workforce-persona-repairer.ts, src/product/generation/workforce-persona-writer.ts
Persona task and writer instructions updated to require loading .env.local/.env without clobbering existing exports and to include deterministic preflight assertions that emit MISSING_ENV_VAR: NAME.
Tests
src/local/entrypoint.test.ts, src/local/auto-fix-loop.test.ts, src/product/generation/pipeline.test.ts
Added/updated tests: validate explicit missing env var ... extraction yields named candidates and export suggestions; ensure prose mentions of acronyms (API/MSD) don't produce exports; expect NETWORK_UNREACHABLE in unrelated timeout case; assert deterministic repair injects RICKY_WORKFLOW_ENV_LOADER, loadRickyWorkflowEnv();, ['.env.local', '.env'], and MISSING_ENV_VAR: assertions; runWithAutoFix now expects persona-driven auto-repair for missing-env.
Docs / Rules
workflows/shared/WORKFLOW_AUTHORING_RULES.md
Added rule requiring deterministic env initialization: load .env.local/.env before .run(...), avoid overwriting existing exports, and fail-fast with MISSING_ENV_VAR: NAME for required vars.

Sequence Diagram(s)

sequenceDiagram
  actor Runtime
  participant Entrypoint
  participant Repairer
  participant Generator
  participant WorkflowArtifact
  participant Filesystem

  Runtime->>Entrypoint: emit stderr / evidence
  Entrypoint->>Entrypoint: hasMissingEnvironmentMessage + extractMissingEnvVars
  Entrypoint-->>Repairer: report MISSING_ENV_VAR evidence
  Repairer->>Generator: repairWorkflowDeterministically(evidence)
  Generator->>WorkflowArtifact: inject loadRickyWorkflowEnv() & assertions
  WorkflowArtifact->>Filesystem: read .env.local / .env at runtime (loader)
  Repairer->>Runtime: retry startFromStep: runtime-launch with repaired artifact
  Runtime->>Entrypoint: run proceeds / success or new evidence
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • AgentWorkforce/ricky#45: Also modifies repairWorkflowDeterministically to add deterministic repair passes; touches the same function and tests.
  • AgentWorkforce/ricky#47: Related/conflicting changes around MISSING_ENV_VAR handling and auto-fix escalation.

Poem

🐰 I sniffed the logs at break of dawn,

Found missing vars and carried on,
I load the repo's secret stew,
I add a guard that prints what's due,
Tiny hops, deterministic brew.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main change: improving how missing-environment-variable workflow failures are recovered, which is the primary focus of the PR.
Description check ✅ Passed The description is directly related to the changeset, detailing the four primary changes: env-file loading, auto-fix repair, tightened extraction logic, and updated rules.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-env-var-blocker-classification

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 83d63acd69

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/local/entrypoint.ts Outdated
Comment on lines +2098 to +2100
const context = line.slice(Math.max(0, index - 100), index + name.length + 100).toLowerCase();
const hasEnvContext = /env(?:ironment)?(?:\s+variable)?|variable/.test(context);
const hasMissingContext = /missing|required|not\s+set|unset/.test(context);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid exporting nearby uppercase log terms

When an explicit missing-env line also mentions any uppercase workflow/product term within 100 characters, this context window makes that term look like a missing variable too. For example, missing env var GITHUB_TOKEN before calling API produces recovery steps for both GITHUB_TOKEN and API, so the false export API=... guidance this change is meant to prevent can still occur whenever the unrelated term is on the same line as the real missing-env message.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/entrypoint.ts`:
- Around line 2074-2109: hasMissingEnvironmentMessage and extractMissingEnvVars
are too permissive and allow unrelated ALL-CAPS tokens (like API) to be treated
as env vars; tighten the parsing so only names syntactically tied to an
env-message are returned. Change hasMissingEnvironmentMessage to require an
explicit env/variable token (env, environment, variable) when matching the "NAME
is required/missing/not set" pattern rather than treating any ALL-CAPS + "is
required" line as a missing-env line. In extractMissingEnvVars, when iterating
matches in a line, restrict the env-context check to a small local window around
the match (e.g., ±20 chars) and require an env/variable keyword inside that
window (instead of scanning the whole line), and only add the name if that local
context also contains a missing-state token (missing|required|not set|unset);
keep excluding known non-env tokens like ENOENT/PATH. Apply these changes inside
the functions hasMissingEnvironmentMessage and extractMissingEnvVars.
🪄 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: 1b19a4fd-700e-4c8a-976c-7c6d3bd913ee

📥 Commits

Reviewing files that changed from the base of the PR and between ef26140 and 83d63ac.

📒 Files selected for processing (2)
  • src/local/entrypoint.test.ts
  • src/local/entrypoint.ts

Comment thread src/local/entrypoint.ts
@khaliqgant khaliqgant changed the title Fix false missing-env blocker classification Improve missing-env workflow recovery May 5, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/local/entrypoint.ts (1)

2074-2115: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Tighten this parser to explicit missing-env syntax only.

The current heuristic still leaves the original false-positive path open. For example, MISSING_ENV_VAR: TEST_TOKEN will add the sentinel itself because Line 2093 captures MISSING_ENV_VAR and Line 2099 treats the ENV substring as env context, so the blocker can still suggest export MISSING_ENV_VAR=.... Line 2079 also still classifies uppercase prose like API is required, and Line 2076 does not skip negated forms like no missing env variables. Please switch both helpers to parse only names that are syntactically attached to an explicit env/variable phrase instead of scanning whole lines for ALL_CAPS tokens.

🤖 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 `@src/local/entrypoint.ts` around lines 2074 - 2115,
hasMissingEnvironmentMessage and extractMissingEnvVars are too permissive —
update both to only detect explicit "env/variable" syntax (and skip negations)
instead of scanning whole lines for ALL_CAPS tokens; change
hasMissingEnvironmentMessage to reject lines with negations like "not missing"
or "no missing" and only return true for tight patterns such as
"(env|environment|variable|env
var|process.env\\.|\\$)\\s*(NAME|\\bNAME=\\b|NAME\\b).* (missing|required|not
set|unset)" or "NAME\\s*(=|:)\\s*.*" where NAME is the identifier, and change
extractMissingEnvVars to stop global ALL_CAPS scanning and only add a name when
it is matched by those explicit adjacent patterns (or explicit forms
process.env.NAME, $NAME, NAME=, or "NAME is missing"), referencing functions
extractMissingEnvVars, hasMissingEnvironmentMessage and looksLikeConfigEnvName
so you locate and replace the broad regex logic with these stricter,
proximity-based regex checks.
🤖 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.test.ts`:
- Around line 404-408: The retry metadata currently propagates synthetic
launch-time step IDs (e.g., 'runtime-launch', 'runtime-precheck') from
auto-fix-loop.ts → entrypoint.ts → local-coordinator.ts into the retry object
used by runSingleAttempt/agent-relay; filter these out when building retry
metadata in the place where retry is constructed (the code that creates the
retry object consumed by runSingleAttempt) so that for launch-time blockers
(MISSING_ENV_VAR, MISSING_BINARY) you either omit startFromStep entirely or drop
any startFromStep that matches the synthetic IDs
('runtime-launch'|'runtime-precheck') before passing to
local-coordinator/agent-relay. Ensure the change is applied where retry is
assembled (referencing the retry object passed into runSingleAttempt) so tests
expecting no synthetic startFromStep (or omitted startFromStep) will pass.

In `@src/product/generation/template-renderer.ts`:
- Around line 831-855: unquoteRickyWorkflowEnvValue currently only trims and
unquotes values so unquoted inline comments like `FOO=bar # local` become part
of the value; update unquoteRickyWorkflowEnvValue to, after trimming and
determining the value is unquoted, strip any inline comment by removing the
first `#` and everything after it when the `#` is preceded by whitespace (or at
line start), then trim the result before returning; keep the existing branch
that returns the exact inner content for quoted values so `#` inside quotes is
preserved. Ensure you only change unquoteRickyWorkflowEnvValue (referenced by
loadRickyWorkflowEnv) so the loader will normalize unquoted `.env` values
correctly.

---

Duplicate comments:
In `@src/local/entrypoint.ts`:
- Around line 2074-2115: hasMissingEnvironmentMessage and extractMissingEnvVars
are too permissive — update both to only detect explicit "env/variable" syntax
(and skip negations) instead of scanning whole lines for ALL_CAPS tokens; change
hasMissingEnvironmentMessage to reject lines with negations like "not missing"
or "no missing" and only return true for tight patterns such as
"(env|environment|variable|env
var|process.env\\.|\\$)\\s*(NAME|\\bNAME=\\b|NAME\\b).* (missing|required|not
set|unset)" or "NAME\\s*(=|:)\\s*.*" where NAME is the identifier, and change
extractMissingEnvVars to stop global ALL_CAPS scanning and only add a name when
it is matched by those explicit adjacent patterns (or explicit forms
process.env.NAME, $NAME, NAME=, or "NAME is missing"), referencing functions
extractMissingEnvVars, hasMissingEnvironmentMessage and looksLikeConfigEnvName
so you locate and replace the broad regex logic with these stricter,
proximity-based regex checks.
🪄 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: c4347ba8-0e4c-4b18-9bdd-a3b01d6d0887

📥 Commits

Reviewing files that changed from the base of the PR and between 83d63ac and 9c3f85a.

📒 Files selected for processing (9)
  • src/local/auto-fix-loop.test.ts
  • src/local/auto-fix-loop.ts
  • src/local/entrypoint.test.ts
  • src/local/entrypoint.ts
  • src/product/generation/pipeline.test.ts
  • src/product/generation/template-renderer.ts
  • src/product/generation/workforce-persona-repairer.ts
  • src/product/generation/workforce-persona-writer.ts
  • workflows/shared/WORKFLOW_AUTHORING_RULES.md
✅ Files skipped from review due to trivial changes (1)
  • workflows/shared/WORKFLOW_AUTHORING_RULES.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/local/entrypoint.test.ts

Comment on lines +404 to +408
expect(runSingleAttempt.mock.calls[1][0].retry).toMatchObject({
previousRunId: 'run-1',
retryOfRunId: 'run-1',
startFromStep: 'runtime-launch',
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Inspect retry construction in src/local/auto-fix-loop.ts"
sed -n '1,260p' src/local/auto-fix-loop.ts | nl -ba

echo
echo "Inspect synthetic failed-step producers/consumers"
rg -n -C3 '\bruntime-launch\b|\bstartFromStep\b|\bfailed_step\b' src/local/entrypoint.ts src/local/auto-fix-loop.ts

Repository: AgentWorkforce/ricky

Length of output: 182


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Inspect auto-fix-loop.ts: retry construction (lines 1-260) ==="
head -260 src/local/auto-fix-loop.ts

echo ""
echo "=== Search for runtime-launch and startFromStep usage ==="
rg -n -C3 'runtime-launch|startFromStep|failed_step' src/local/entrypoint.ts src/local/auto-fix-loop.ts

Repository: AgentWorkforce/ricky

Length of output: 17833


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search for failedStepFromEvidence function definition ==="
rg -n 'failedStepFromEvidence' src/

echo ""
echo "=== Search for any remapping/filtering of runtime-launch ==="
rg -n 'runtime-launch|runtime-precheck' src/ -B2 -A2

echo ""
echo "=== Check test file context around lines 404-408 ==="
sed -n '390,420p' src/local/auto-fix-loop.test.ts

Repository: AgentWorkforce/ricky

Length of output: 7960


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check failedStepFromEvidence implementation (lines 1348-1370) ==="
sed -n '1348,1370p' src/local/auto-fix-loop.ts

echo ""
echo "=== Check if coordinator/runner has handling for synthetic step IDs ==="
fd -t f 'coordinator' src/ | head -5

echo ""
echo "=== Search for any filtering logic before passing to coordinator ==="
rg -n 'coordinator.launch|--start-from' src/local/entrypoint.ts -B3 -A3

Repository: AgentWorkforce/ricky

Length of output: 2137


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search local-coordinator.ts for startFrom handling ==="
rg -n 'startFrom|start.from|start-from' src/runtime/local-coordinator.ts -B2 -A2

echo ""
echo "=== Check if there's any remapping in coordinator ==="
cat src/runtime/local-coordinator.ts | head -100

Repository: AgentWorkforce/ricky

Length of output: 3737


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check if there's filtering for synthetic step IDs anywhere ==="
rg -n 'runtime-launch|runtime-precheck' src/runtime/ -B2 -A2

echo ""
echo "=== Check agent-relay documentation or handling of --start-from ==="
fd -t f 'agent-relay' src/ 2>/dev/null || echo "agent-relay not found in src"

echo ""
echo "=== Check if startFromStep is conditionally set based on step ID ==="
rg -n 'startFromStep.*=|startFromStep\?' src/local/ -B2 -A2

Repository: AgentWorkforce/ricky

Length of output: 131


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Look at the test helper blockerResponse to understand how runtime-launch gets set ==="
rg -n 'blockerResponse.*runtime-launch|function blockerResponse' src/local/auto-fix-loop.test.ts -B2 -A15

echo ""
echo "=== Check what happens in the SDK runner when --start-from is called with invalid step ==="
fd -t f 'runner|relay' src/product/ | head -3

echo ""
echo "=== Check if there's any step validation or filtering before execution ==="
rg -n 'startFromStep|start-from' src/product/ -B1 -A1

Repository: AgentWorkforce/ricky

Length of output: 4796


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Look at the full blockerResponse function ==="
sed -n '580,620p' src/local/auto-fix-loop.test.ts

echo ""
echo "=== Check how the test evidence is constructed ==="
rg -n 'missingEnvEvidence|function.*Evidence' src/local/auto-fix-loop.test.ts -A20 | head -60

Repository: AgentWorkforce/ricky

Length of output: 4420


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check if agent-relay or coordinator ignores invalid startFromStep ==="
rg -n 'start.from|startFromStep|--start-from' src/runtime/local-coordinator.ts | head -20

echo ""
echo "=== Check what buildInvocationSummary does with startFromStep ==="
sed -n '275,340p' src/runtime/local-coordinator.ts

echo ""
echo "=== Look for any documentation or tests about synthetic step IDs ==="
rg -n 'runtime-launch.*resume|synthetic.*step|step.*validation' src/ -i

Repository: AgentWorkforce/ricky

Length of output: 6384


🏁 Script executed:

#!/bin/bash
set -euo pipefochop pipefail

echo "=== Check if there's any agent-relay handling or documentation of synthetic steps ==="
rg -n 'agent-relay|--start-from' README.md CONTRIBUTING.md 2>/dev/null || echo "No docs found"

echo ""
echo "=== Check test to see if second call actually validates the retry was properly formed ==="
sed -n '375,420p' src/local/auto-fix-loop.test.ts

echo ""
echo "=== Verify: does the second mockResolvedValue actually test agent-relay behavior or just return success? ==="
rg -n 'successResponse|mockResolvedValue' src/local/auto-fix-loop.test.ts | grep -A2 -B2 'successResponse.*run-2'

Repository: AgentWorkforce/ricky

Length of output: 120


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check if there's any agent-relay handling or documentation of synthetic steps ==="
rg -n 'agent-relay|--start-from' README.md CONTRIBUTING.md 2>/dev/null || echo "No docs found"

echo ""
echo "=== Check test to see if second call actually validates the retry was properly formed ==="
sed -n '375,420p' src/local/auto-fix-loop.test.ts

echo ""
echo "=== Verify: does the second mockResolvedValue actually test agent-relay behavior or just return success? ==="
rg -n 'successResponse|mockResolvedValue' src/local/auto-fix-loop.test.ts | grep -A2 -B2 'successResponse.*run-2'

Repository: AgentWorkforce/ricky

Length of output: 4865


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Final check: Is there any remapping between failedStep extraction and retry construction? ==="
sed -n '115,230p' src/local/auto-fix-loop.ts | grep -A10 -B10 'startFromStep'

echo ""
echo "=== Confirm: agent-relay does NOT have special handling for runtime-launch ==="
fd -t f 'relay' src/ 2>/dev/null | head -5 || echo "agent-relay source not in repo"

echo ""
echo "=== Check if there's any comment or documentation about resuming from synthetic steps ==="
rg -n 'runtime-launch.*resume|resume.*synthetic|step.*synthetic' src/ -B2 -A2

Repository: AgentWorkforce/ricky

Length of output: 1011


Filter out synthetic step IDs before constructing retry metadata.

The test now expects startFromStep: 'runtime-launch', but runtime-launch is a synthetic label from src/local/entrypoint.ts used only to mark launch-time blockers—it is not an actual workflow step. Currently, this synthetic ID flows unchanged through auto-fix-loop.tsentrypoint.tslocal-coordinator.ts and reaches agent-relay as --start-from runtime-launch, where it will fail because the step does not exist in the workflow.

For launch-time blockers (MISSING_ENV_VAR, MISSING_BINARY), either:

  1. Omit startFromStep from the retry metadata entirely, or
  2. Filter out synthetic step IDs (e.g., runtime-launch, runtime-precheck) before passing to the retry constructor.
Test expectation showing the issue
expect(runSingleAttempt.mock.calls[1][0].retry).toMatchObject({
  previousRunId: 'run-1',
  retryOfRunId: 'run-1',
  startFromStep: 'runtime-launch',
});
🤖 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 `@src/local/auto-fix-loop.test.ts` around lines 404 - 408, The retry metadata
currently propagates synthetic launch-time step IDs (e.g., 'runtime-launch',
'runtime-precheck') from auto-fix-loop.ts → entrypoint.ts → local-coordinator.ts
into the retry object used by runSingleAttempt/agent-relay; filter these out
when building retry metadata in the place where retry is constructed (the code
that creates the retry object consumed by runSingleAttempt) so that for
launch-time blockers (MISSING_ENV_VAR, MISSING_BINARY) you either omit
startFromStep entirely or drop any startFromStep that matches the synthetic IDs
('runtime-launch'|'runtime-precheck') before passing to
local-coordinator/agent-relay. Ensure the change is applied where retry is
assembled (referencing the retry object passed into runSingleAttempt) so tests
expecting no synthetic startFromStep (or omitted startFromStep) will pass.

Comment on lines +831 to +855
function renderWorkflowEnvLoaderHelper(): string {
return `function loadRickyWorkflowEnv(cwd = process.cwd()) {
for (const file of ['.env.local', '.env']) {
const path = rickyWorkflowPath.join(cwd, file);
if (!rickyWorkflowFs.existsSync(path)) continue;
const body = rickyWorkflowFs.readFileSync(path, 'utf8');
for (const rawLine of body.split(/\\r?\\n/)) {
const line = rawLine.trim();
if (!line || line.startsWith('#')) continue;
const match = /^(?:export\\s+)?([A-Za-z_][A-Za-z0-9_]*)\\s*=\\s*(.*)$/.exec(line);
if (!match) continue;
const [, key, rawValue] = match;
if (!key || rawValue === undefined || process.env[key] !== undefined) continue;
process.env[key] = unquoteRickyWorkflowEnvValue(rawValue);
}
}
}

function unquoteRickyWorkflowEnvValue(value: string): string {
const trimmed = value.trim();
if ((trimmed.startsWith('"') && trimmed.endsWith('"')) || (trimmed.startsWith("'") && trimmed.endsWith("'"))) {
return trimmed.slice(1, -1);
}
return trimmed;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Strip inline comments from unquoted .env values.

Right now FOO=bar # local is loaded as bar # local, because the helper only trims and unwraps quotes. That will corrupt common .env files and break the values this new loader is supposed to recover. Please normalize unquoted values before assigning them, while still preserving # inside quoted strings.

🤖 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 `@src/product/generation/template-renderer.ts` around lines 831 - 855,
unquoteRickyWorkflowEnvValue currently only trims and unquotes values so
unquoted inline comments like `FOO=bar # local` become part of the value; update
unquoteRickyWorkflowEnvValue to, after trimming and determining the value is
unquoted, strip any inline comment by removing the first `#` and everything
after it when the `#` is preceded by whitespace (or at line start), then trim
the result before returning; keep the existing branch that returns the exact
inner content for quoted values so `#` inside quotes is preserved. Ensure you
only change unquoteRickyWorkflowEnvValue (referenced by loadRickyWorkflowEnv) so
the loader will normalize unquoted `.env` values correctly.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
src/local/entrypoint.ts (2)

2074-2080: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle negated missing-env lines before matching positives.

Line 2077 only ignores not missing env...; lines like No missing environment variables still match Line 2078 and can misclassify blockers as MISSING_ENV_VAR via Line 2003.

Suggested fix
 function hasMissingEnvironmentMessage(text: string): boolean {
   for (const rawLine of text.split(/\r?\n/)) {
     const line = stripAnsi(rawLine);
-    if (/\bnot\s+missing\s+env(?:ironment)?\b/i.test(line)) continue;
+    if (/\b(?:no|not)\s+missing\s+env(?:ironment)?(?:\s+var(?:iable)?s?)?\b/i.test(line)) continue;
     if (/(?:missing|required).{0,80}(?:env(?:ironment)?(?:\s+variable)?)/i.test(line)) return true;
     if (/(?:env(?:ironment)?(?:\s+variable)?).{0,80}(?:missing|required|not\s+set|unset)/i.test(line)) return true;
   }
   return false;
 }
🤖 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 `@src/local/entrypoint.ts` around lines 2074 - 2080, The
hasMissingEnvironmentMessage function currently only skips lines matching "not
missing env..." but still counts messages like "No missing environment
variables" as positive; update hasMissingEnvironmentMessage to detect common
negation patterns (e.g., "no missing", "none missing", "no .* missing", "no
missing environment variables", "environment variables are not missing") before
the positive regex checks and skip those lines (continue) so they don't trigger
the positive matches; modify the checks inside hasMissingEnvironmentMessage to
first run a negation-regex branch and then the existing positive/required env
regexes.

2120-2123: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align env-name acceptance with concrete-var filtering to avoid acronym false positives.

Line 2122 still admits short all-caps tokens like API/MSD, which can leak into blocker missing and export ... recovery steps even when they’re prose. This conflicts with stricter filtering used in src/local/auto-fix-loop.ts (isConcreteEnvVarName, Lines 513-517 in provided context).

Suggested fix
 function addEnvNames(names: Set<string>, value: string | undefined): void {
   for (const name of (value ?? '').split(/\s*,\s*/)) {
-    if (/^[A-Z][A-Z0-9_]{2,}$/.test(name) && !['ENOENT', 'PATH'].includes(name)) {
+    if (
+      /^[A-Z][A-Z0-9_]{2,}$/.test(name) &&
+      !['ENOENT', 'PATH'].includes(name) &&
+      (name.includes('_') || name.length > 3)
+    ) {
       names.add(name);
     }
   }
 }
🤖 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 `@src/local/entrypoint.ts` around lines 2120 - 2123, The addEnvNames function
is allowing short all-caps tokens (e.g., "API") that the concrete-var filter
disallows; instead of the current inline regex in addEnvNames, call the shared
isConcreteEnvVarName(name) predicate (import it from its module) and use that
for the acceptance check (while still excluding ['ENOENT','PATH']) so env-name
acceptance matches the stricter concrete-var filtering used in auto-fix-loop.
🤖 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.

Duplicate comments:
In `@src/local/entrypoint.ts`:
- Around line 2074-2080: The hasMissingEnvironmentMessage function currently
only skips lines matching "not missing env..." but still counts messages like
"No missing environment variables" as positive; update
hasMissingEnvironmentMessage to detect common negation patterns (e.g., "no
missing", "none missing", "no .* missing", "no missing environment variables",
"environment variables are not missing") before the positive regex checks and
skip those lines (continue) so they don't trigger the positive matches; modify
the checks inside hasMissingEnvironmentMessage to first run a negation-regex
branch and then the existing positive/required env regexes.
- Around line 2120-2123: The addEnvNames function is allowing short all-caps
tokens (e.g., "API") that the concrete-var filter disallows; instead of the
current inline regex in addEnvNames, call the shared isConcreteEnvVarName(name)
predicate (import it from its module) and use that for the acceptance check
(while still excluding ['ENOENT','PATH']) so env-name acceptance matches the
stricter concrete-var filtering used in auto-fix-loop.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 56b029d9-ff5d-412f-92b6-a787100f4c84

📥 Commits

Reviewing files that changed from the base of the PR and between 9c3f85a and fce8ca1.

📒 Files selected for processing (2)
  • src/local/entrypoint.test.ts
  • src/local/entrypoint.ts
✅ Files skipped from review due to trivial changes (1)
  • src/local/entrypoint.test.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant