Emit repair-aware generated workflows#75
Merged
khaliqgant merged 6 commits intomainfrom May 8, 2026
Merged
Conversation
Workflows that own the PR-creation boundary must use @agent-relay/github-primitive (createGitHubStep with action createPR, or GitHubClient.createPR) rather than shelling out to gh pr create. The primitive handles runtime selection across local gh, Nango cloud, and relay-cloud proxy and produces structured outputs the orchestrator can chain on. Master/orchestration workflows that complete an end-to-end deliverable should default to including a final-pr step using this primitive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces two independent, significant features: (1) repair-aware retry hardening for generated workflows, ensuring all workflows emit retry handlers that include repair agents rather than fail-fast, and (2) a comprehensive human-authored product evaluation suite framework with six thematic test suites, four npm CLI scripts for compilation and execution, and detailed specifications covering CLI behavior, generation quality, runtime recovery, surface ingress, workflow authoring, and agent-assistant boundary semantics. Supporting changes include trajectory recording, configuration updates, package dependency bumps, and test proof updates. ChangesRepair-Aware Retry Hardening
Evaluation Suite Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| "status": "completed", | ||
| "startedAt": "2026-05-08T14:46:04.652Z", | ||
| "completedAt": "2026-05-08T14:54:06.222Z", | ||
| "path": "/Users/khaliqgant/Projects/AgentWorkforce/ricky-workflow-never-fail/.trajectories/completed/2026-05/traj_2f8sfg3akql2.json" |
Contributor
There was a problem hiding this comment.
🟡 Trajectory index contains hardcoded absolute local path that breaks on all other machines
The .trajectories/index.json file (newly tracked after removing .trajectories/ from .gitignore) stores the trajectory path as an absolute local filesystem path: /Users/khaliqgant/Projects/AgentWorkforce/ricky-workflow-never-fail/.trajectories/completed/2026-05/traj_2f8sfg3akql2.json. The trail CLI tool (documented in AGENTS.md:86-170) uses this index to resolve trajectory files via commands like trail show <id> and trail list. On any machine other than the original author's, this path is invalid, making the index functionally broken for trajectory resolution. The same absolute path pattern appears in projectId at .trajectories/completed/2026-05/traj_2f8sfg3akql2.json:59.
Suggested change
| "path": "/Users/khaliqgant/Projects/AgentWorkforce/ricky-workflow-never-fail/.trajectories/completed/2026-05/traj_2f8sfg3akql2.json" | |
| "path": ".trajectories/completed/2026-05/traj_2f8sfg3akql2.json" |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
evals/suites/workflow-authoring/cases.md (1)
35-50: ⚡ Quick winAdd operator-plan artifact intake to convention-update case criteria
For this convention-update scenario, add a Must requiring the workflow to read the operator plan artifact when one is provided. That closes a known reliability gap for convention-bound edits.
Based on learnings: “For convention-update workflows, read the operator plan artifact when one is provided before editing the declared convention files.”
🤖 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 `@evals/suites/workflow-authoring/cases.md` around lines 35 - 50, Update the convention-update case's "Must" criteria to require reading the operator plan artifact when present: add a Must entry stating the workflow must read the operator plan artifact (e.g., from .workflow-artifacts/operator-plan) before editing the declared convention files; locate the convention-update scenario's Must list in cases.md and insert this requirement alongside the existing persistence and reviewer rules so the workflow is required to consume the operator plan artifact when provided.evals/suites/generation-quality/cases.jsonl (1)
7-7: ⚡ Quick winAlign validation/fix/rerun loop guidance with the repo rule
Line 7 currently makes
dagconditional (“when dependencies matter”). For eval consistency, this should requiredagfor validation/fix/rerun loops (updatecases.md, then regenerate this JSONL).Based on learnings: “Choose the swarm pattern from the work shape … use
dagfor validation, fix, and rerun loops.”🤖 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 `@evals/suites/generation-quality/cases.jsonl` at line 7, Update the test case with id "generation-quality.pattern-selection-deliberate" so the "must" array requires "Use `dag` for validation/fix/rerun loops" unconditionally (remove the conditional phrasing “when dependencies matter”), then update the corresponding guidance in cases.md to match this rule and regenerate the JSONL so the file reflects the tightened requirement; locate the JSON object by its id and edit the "must" entry and the cases.md text accordingly before regenerating the JSONL output.
🤖 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 @.trajectories/index.json:
- Line 10: The index entry contains an absolute "path" value which breaks
portability; update the "path" field in .trajectories/index.json (the JSON
object with key "path") to a relative path from the repo root (e.g., "
.trajectories/completed/2026-05/traj_2f8sfg3akql2.json") and, if this file is
generated, modify the generator to produce relative paths (use
path.relative(repoRoot, filePath) or equivalent) so all entries use
repository-relative paths instead of user-specific absolute paths.
In `@evals/suites/cli-behavior/cases.md`:
- Line 53: Replace the hard-coded semver prefix in the CLI test pattern (the
line currently matching "^ricky 0\.1\.\d+") with a flexible semver-shaped regex
so the test validates version format rather than a specific minor version;
update the pattern in evals/suites/cli-behavior/cases.md (the line containing
the "^ricky 0\.1\.\d+" string) to something like a general semver match (e.g.
start with "^ricky " followed by "\d+\.\d+\.\d+([-.].+)?$") so pre-release/build
metadata are allowed and future releases don't break the case.
In `@scripts/evals/compare-ricky-evals.mjs`:
- Around line 58-65: The loadRuns function currently crashes when a malformed or
truncated result.json is encountered; modify loadRuns to handle per-file
read/parse errors by wrapping the JSON read/parse for each candidate file in a
try/catch, skip files that throw (optionally log or warn which file was
skipped), and only include successfully parsed run objects in the array before
sorting; keep the existing behavior: look under RUNS_DIR, build paths to
result.json, filter for existence, but protect the JSON.parse step in loadRuns
so a single bad result.json does not abort the whole operation.
In `@scripts/evals/compile-ricky-evals.mjs`:
- Around line 42-47: readOptionValue currently only rejects missing values or
tokens beginning with `--`, so short-option tokens like `-h` get accepted as
values; update the function `readOptionValue(argv, index, option)` to also treat
single-dash short-option tokens as missing values (e.g., detect tokens that
start with `-` but are not negative numbers) and throw the same `${option}
requires a value` error when encountered so `--suite -h` is rejected as a
missing value rather than accepted as `suite="-h"`.
In `@scripts/evals/run-ricky-evals.mjs`:
- Around line 193-205: The parseRickyEvalArgs function currently consumes the
token after '--executor' unconditionally; update it to validate that argv[index
+ 1] exists and is not another flag (e.g., does not start with '-') before
setting executorOverride and skipping the next token; if the next token is
missing or looks like a flag, do not consume it—either push '--executor' into
passthrough (or throw a clear error) and continue scanning. Locate
parseRickyEvalArgs, the executorOverride and passthrough logic, and implement
the token-existence and prefix-check (argv[index+1] &&
!argv[index+1].startsWith('-')) before assigning executorOverride.
In `@scripts/evals/summarize-ricky-evals.mjs`:
- Around line 35-39: The summary pipeline currently calls JSON.parse on every
result.json and will crash on a malformed file; update the chain that builds run
objects (the readdirSync(...).map(...).filter(...).map(...).sort(...)) to catch
JSON.parse/readFileSync errors per file: for each file path produced, read and
parse inside a try/catch (referencing RUNS_DIR, result.json, readFileSync,
JSON.parse, existsSync), log or warn about any malformed file and return null
for failures, then filter out null/undefined results before sorting so one bad
result.json no longer terminates the whole summary command.
In `@src/product/generation/pipeline.ts`:
- Around line 321-327: The check using requiresRepairAwareRetry(content) only
inspects top-level lines and misses .onError(...) blocks embedded inside
serialized child workflows; update the validation to examine all rendered child
sources (or the entire rendered content as a whole) so embedded onError handlers
are validated too. Concretely, locate where issues.push(blockingIssue(...)) is
called (the requiresRepairAwareRetry(content) check) and either (A) iterate over
child workflow sources before they are serialized and call
requiresRepairAwareRetry(childSource) for each, or (B) change
requiresRepairAwareRetry to operate on the full rendered content (not
line-based) and use a multiline/dotall regex to find .onError(...) blocks and
ensure they include repairAgent and repairRetries; apply the same fix to the
other occurrence around the 434-443 area so both top-level and embedded child
workflows are validated.
---
Nitpick comments:
In `@evals/suites/generation-quality/cases.jsonl`:
- Line 7: Update the test case with id
"generation-quality.pattern-selection-deliberate" so the "must" array requires
"Use `dag` for validation/fix/rerun loops" unconditionally (remove the
conditional phrasing “when dependencies matter”), then update the corresponding
guidance in cases.md to match this rule and regenerate the JSONL so the file
reflects the tightened requirement; locate the JSON object by its id and edit
the "must" entry and the cases.md text accordingly before regenerating the JSONL
output.
In `@evals/suites/workflow-authoring/cases.md`:
- Around line 35-50: Update the convention-update case's "Must" criteria to
require reading the operator plan artifact when present: add a Must entry
stating the workflow must read the operator plan artifact (e.g., from
.workflow-artifacts/operator-plan) before editing the declared convention files;
locate the convention-update scenario's Must list in cases.md and insert this
requirement alongside the existing persistence and reviewer rules so the
workflow is required to consume the operator plan artifact when provided.
🪄 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: a69fdfcb-c5b3-48bf-a16c-737ed4400ca4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
.gitignore.trajectories/completed/2026-05/traj_2f8sfg3akql2.json.trajectories/completed/2026-05/traj_2f8sfg3akql2.md.trajectories/index.jsonAGENTS.mdREADME.mdevals/README.mdevals/fixtures/transcripts/.gitkeepevals/suites/agent-assistant-boundary/cases.jsonlevals/suites/agent-assistant-boundary/cases.mdevals/suites/agent-assistant-boundary/rubric.mdevals/suites/cli-behavior/cases.jsonlevals/suites/cli-behavior/cases.mdevals/suites/cli-behavior/rubric.mdevals/suites/generation-quality/cases.jsonlevals/suites/generation-quality/cases.mdevals/suites/generation-quality/rubric.mdevals/suites/runtime-recovery/cases.jsonlevals/suites/runtime-recovery/cases.mdevals/suites/runtime-recovery/rubric.mdevals/suites/surfaces-ingress/cases.jsonlevals/suites/surfaces-ingress/cases.mdevals/suites/surfaces-ingress/rubric.mdevals/suites/workflow-authoring/cases.jsonlevals/suites/workflow-authoring/cases.mdevals/suites/workflow-authoring/rubric.mdpackage.jsonscripts/evals/compare-ricky-evals.mjsscripts/evals/compile-ricky-evals.mjsscripts/evals/run-ricky-evals.mjsscripts/evals/summarize-ricky-evals.mjssrc/product/generation/master-workflow-renderer.tssrc/product/generation/pipeline.test.tssrc/product/generation/pipeline.tssrc/product/generation/template-renderer.tssrc/shared/constants.tstest/package-proof/package-layout-proof.test.tstest/package-proof/package-layout-proof.ts
💤 Files with no reviewable changes (1)
- .gitignore
| "status": "completed", | ||
| "startedAt": "2026-05-08T14:46:04.652Z", | ||
| "completedAt": "2026-05-08T14:54:06.222Z", | ||
| "path": "/Users/khaliqgant/Projects/AgentWorkforce/ricky-workflow-never-fail/.trajectories/completed/2026-05/traj_2f8sfg3akql2.json" |
There was a problem hiding this comment.
Absolute path in committed index may cause portability issues.
The path field contains an absolute filesystem path (/Users/khaliqgant/Projects/AgentWorkforce/...), which will not work for other developers or in CI environments. Consider using a relative path from the repository root.
🔧 Suggested fix
- "path": "/Users/khaliqgant/Projects/AgentWorkforce/ricky-workflow-never-fail/.trajectories/completed/2026-05/traj_2f8sfg3akql2.json"
+ "path": ".trajectories/completed/2026-05/traj_2f8sfg3akql2.json"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "path": "/Users/khaliqgant/Projects/AgentWorkforce/ricky-workflow-never-fail/.trajectories/completed/2026-05/traj_2f8sfg3akql2.json" | |
| "path": ".trajectories/completed/2026-05/traj_2f8sfg3akql2.json" |
🤖 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 @.trajectories/index.json at line 10, The index entry contains an absolute
"path" value which breaks portability; update the "path" field in
.trajectories/index.json (the JSON object with key "path") to a relative path
from the repo root (e.g., "
.trajectories/completed/2026-05/traj_2f8sfg3akql2.json") and, if this file is
generated, modify the generator to produce relative paths (use
path.relative(repoRoot, filePath) or equivalent) so all entries use
repository-relative paths instead of user-specific absolute paths.
| ### Deterministic Checks | ||
| ok: true | ||
| contentMatches: | ||
| - ^ricky 0\.1\.\d+ |
There was a problem hiding this comment.
Avoid pinning the version check to 0.1.x
Line 53 hard-codes 0.1, which makes this eval fail on normal package version evolution. Prefer a semver-shape match (for example, ^ricky \d+\.\d+\.\d+([-.].+)?$) so the case validates CLI behavior, not release cadence.
🤖 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 `@evals/suites/cli-behavior/cases.md` at line 53, Replace the hard-coded semver
prefix in the CLI test pattern (the line currently matching "^ricky 0\.1\.\d+")
with a flexible semver-shaped regex so the test validates version format rather
than a specific minor version; update the pattern in
evals/suites/cli-behavior/cases.md (the line containing the "^ricky 0\.1\.\d+"
string) to something like a general semver match (e.g. start with "^ricky "
followed by "\d+\.\d+\.\d+([-.].+)?$") so pre-release/build metadata are allowed
and future releases don't break the case.
Comment on lines
+58
to
+65
| function loadRuns() { | ||
| if (!existsSync(RUNS_DIR)) return []; | ||
| return readdirSync(RUNS_DIR) | ||
| .map((dir) => path.join(RUNS_DIR, dir, 'result.json')) | ||
| .filter((file) => existsSync(file)) | ||
| .map((file) => JSON.parse(readFileSync(file, 'utf8'))) | ||
| .sort((a, b) => String(b.timestamp).localeCompare(String(a.timestamp))); | ||
| } |
There was a problem hiding this comment.
Harden run loading against malformed result.json files.
A single truncated/invalid run artifact currently crashes evals:compare; the command should skip bad entries and continue with readable runs.
💡 Proposed fix
function loadRuns() {
if (!existsSync(RUNS_DIR)) return [];
- return readdirSync(RUNS_DIR)
- .map((dir) => path.join(RUNS_DIR, dir, 'result.json'))
- .filter((file) => existsSync(file))
- .map((file) => JSON.parse(readFileSync(file, 'utf8')))
- .sort((a, b) => String(b.timestamp).localeCompare(String(a.timestamp)));
+ const runs = [];
+ for (const dir of readdirSync(RUNS_DIR)) {
+ const file = path.join(RUNS_DIR, dir, 'result.json');
+ if (!existsSync(file)) continue;
+ try {
+ runs.push(JSON.parse(readFileSync(file, 'utf8')));
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ console.warn(`Skipping unreadable eval run: ${path.relative(ROOT, file)} (${message})`);
+ }
+ }
+ return runs.sort((a, b) => String(b.timestamp).localeCompare(String(a.timestamp)));
}🤖 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 `@scripts/evals/compare-ricky-evals.mjs` around lines 58 - 65, The loadRuns
function currently crashes when a malformed or truncated result.json is
encountered; modify loadRuns to handle per-file read/parse errors by wrapping
the JSON read/parse for each candidate file in a try/catch, skip files that
throw (optionally log or warn which file was skipped), and only include
successfully parsed run objects in the array before sorting; keep the existing
behavior: look under RUNS_DIR, build paths to result.json, filter for existence,
but protect the JSON.parse step in loadRuns so a single bad result.json does not
abort the whole operation.
Comment on lines
+42
to
+47
| function readOptionValue(argv, index, option) { | ||
| const value = argv[index]; | ||
| if (value === undefined || value.startsWith('--')) { | ||
| throw new Error(`${option} requires a value`); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
Reject short-option tokens as missing option values.
--suite -h is currently accepted as suite="-h" instead of being treated as a missing value, which leads to confusing downstream failures.
💡 Proposed fix
function readOptionValue(argv, index, option) {
const value = argv[index];
- if (value === undefined || value.startsWith('--')) {
+ if (value === undefined || value.startsWith('-')) {
throw new Error(`${option} requires a value`);
}
return value;
}🤖 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 `@scripts/evals/compile-ricky-evals.mjs` around lines 42 - 47, readOptionValue
currently only rejects missing values or tokens beginning with `--`, so
short-option tokens like `-h` get accepted as values; update the function
`readOptionValue(argv, index, option)` to also treat single-dash short-option
tokens as missing values (e.g., detect tokens that start with `-` but are not
negative numbers) and throw the same `${option} requires a value` error when
encountered so `--suite -h` is rejected as a missing value rather than accepted
as `suite="-h"`.
Comment on lines
+193
to
+205
| function parseRickyEvalArgs(argv) { | ||
| const passthrough = []; | ||
| let executorOverride; | ||
| for (let index = 0; index < argv.length; index += 1) { | ||
| const arg = argv[index]; | ||
| if (arg === '--executor') { | ||
| executorOverride = argv[index + 1]; | ||
| index += 1; | ||
| continue; | ||
| } | ||
| passthrough.push(arg); | ||
| } | ||
| return { argv: passthrough, executorOverride }; |
There was a problem hiding this comment.
Validate --executor value before consuming the next token.
The parser currently accepts missing/flag-like values for --executor, which can swallow the next CLI option (for example --suite) and change behavior silently.
💡 Proposed fix
function parseRickyEvalArgs(argv) {
const passthrough = [];
let executorOverride;
for (let index = 0; index < argv.length; index += 1) {
const arg = argv[index];
if (arg === '--executor') {
- executorOverride = argv[index + 1];
+ const value = argv[index + 1];
+ if (!value || value.startsWith('-')) {
+ throw new Error('--executor requires a value');
+ }
+ executorOverride = value;
index += 1;
continue;
}
passthrough.push(arg);
}
return { argv: passthrough, executorOverride };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function parseRickyEvalArgs(argv) { | |
| const passthrough = []; | |
| let executorOverride; | |
| for (let index = 0; index < argv.length; index += 1) { | |
| const arg = argv[index]; | |
| if (arg === '--executor') { | |
| executorOverride = argv[index + 1]; | |
| index += 1; | |
| continue; | |
| } | |
| passthrough.push(arg); | |
| } | |
| return { argv: passthrough, executorOverride }; | |
| function parseRickyEvalArgs(argv) { | |
| const passthrough = []; | |
| let executorOverride; | |
| for (let index = 0; index < argv.length; index += 1) { | |
| const arg = argv[index]; | |
| if (arg === '--executor') { | |
| const value = argv[index + 1]; | |
| if (!value || value.startsWith('-')) { | |
| throw new Error('--executor requires a value'); | |
| } | |
| executorOverride = value; | |
| index += 1; | |
| continue; | |
| } | |
| passthrough.push(arg); | |
| } | |
| return { argv: passthrough, executorOverride }; | |
| } |
🤖 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 `@scripts/evals/run-ricky-evals.mjs` around lines 193 - 205, The
parseRickyEvalArgs function currently consumes the token after '--executor'
unconditionally; update it to validate that argv[index + 1] exists and is not
another flag (e.g., does not start with '-') before setting executorOverride and
skipping the next token; if the next token is missing or looks like a flag, do
not consume it—either push '--executor' into passthrough (or throw a clear
error) and continue scanning. Locate parseRickyEvalArgs, the executorOverride
and passthrough logic, and implement the token-existence and prefix-check
(argv[index+1] && !argv[index+1].startsWith('-')) before assigning
executorOverride.
Comment on lines
+35
to
+39
| return readdirSync(RUNS_DIR) | ||
| .map((dir) => path.join(RUNS_DIR, dir, 'result.json')) | ||
| .filter((file) => existsSync(file)) | ||
| .map((file) => JSON.parse(readFileSync(file, 'utf8'))) | ||
| .sort((a, b) => String(b.timestamp).localeCompare(String(a.timestamp))); |
There was a problem hiding this comment.
Handle malformed run files without crashing the summary command.
At Line 38, JSON.parse can throw on one bad result.json and terminate the entire report, even if other runs are valid.
Proposed fix
function loadRuns() {
if (!existsSync(RUNS_DIR)) return [];
return readdirSync(RUNS_DIR)
.map((dir) => path.join(RUNS_DIR, dir, 'result.json'))
.filter((file) => existsSync(file))
- .map((file) => JSON.parse(readFileSync(file, 'utf8')))
+ .flatMap((file) => {
+ try {
+ return [JSON.parse(readFileSync(file, 'utf8'))];
+ } catch {
+ console.warn(`Skipping invalid eval run file: ${path.relative(ROOT, file)}`);
+ return [];
+ }
+ })
.sort((a, b) => String(b.timestamp).localeCompare(String(a.timestamp)));
}🤖 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 `@scripts/evals/summarize-ricky-evals.mjs` around lines 35 - 39, The summary
pipeline currently calls JSON.parse on every result.json and will crash on a
malformed file; update the chain that builds run objects (the
readdirSync(...).map(...).filter(...).map(...).sort(...)) to catch
JSON.parse/readFileSync errors per file: for each file path produced, read and
parse inside a try/catch (referencing RUNS_DIR, result.json, readFileSync,
JSON.parse, existsSync), log or warn about any malformed file and return null
for failures, then filter out null/undefined results before sorting so one bad
result.json no longer terminates the whole summary command.
Comment on lines
+321
to
+327
| if (requiresRepairAwareRetry(content)) { | ||
| issues.push(blockingIssue( | ||
| 'validation', | ||
| 'REPAIR_AWARE_RETRY_MISSING', | ||
| 'Rendered workflow must use retry error handling with repairAgent and repairRetries so repairable deterministic gates do not fail the workflow outright.', | ||
| )); | ||
| } |
There was a problem hiding this comment.
Validate embedded child .onError() blocks, not just top-level lines.
Master artifacts serialize child workflows into JSON strings, so those repair policies never start a physical line in the outer artifact. This line-based scan only validates the outer master workflow, which means a child workflow can regress to fail-fast or plain retry and still avoid REPAIR_AWARE_RETRY_MISSING. Matching .onError(...) across the full rendered content, or validating each child source before serialization, would close that gap and also make the check less brittle to multiline property formatting.
Also applies to: 434-443
🤖 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/pipeline.ts` around lines 321 - 327, The check using
requiresRepairAwareRetry(content) only inspects top-level lines and misses
.onError(...) blocks embedded inside serialized child workflows; update the
validation to examine all rendered child sources (or the entire rendered content
as a whole) so embedded onError handlers are validated too. Concretely, locate
where issues.push(blockingIssue(...)) is called (the
requiresRepairAwareRetry(content) check) and either (A) iterate over child
workflow sources before they are serialized and call
requiresRepairAwareRetry(childSource) for each, or (B) change
requiresRepairAwareRetry to operate on the full rendered content (not
line-based) and use a multiline/dotall regex to find .onError(...) blocks and
ensure they include repairAgent and repairRetries; apply the same fix to the
other occurrence around the 434-443 area so both top-level and embedded child
workflows are validated.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/local/auto-fix-loop.ts (1)
1016-1023: ⚡ Quick winConsider using shared constants for repair-aware retry configuration.
The repair configuration at line 1021 uses hardcoded values (
maxRetries: 2, retryDelayMs: 1000, repairRetries: 2), whilemaster-workflow-renderer.tsuses imported constants (DEFAULT_RETRY_MAX_ATTEMPTS,DEFAULT_RETRY_BACKOFF_MS,DEFAULT_REPAIR_RETRY_ATTEMPTS). This divergence can lead to inconsistent behavior if constants change.♻️ Suggested fix
+import { + DEFAULT_REPAIR_RETRY_ATTEMPTS, + DEFAULT_RETRY_BACKOFF_MS, + DEFAULT_RETRY_MAX_ATTEMPTS, +} from '../shared/constants.js'; // ... in repairMasterChildRunRepairLoop: changes.push('replaced fail-fast error handling with repair-aware retry'); const indent = match.match(/^\s*/)?.[0] ?? ''; - return `${indent}.onError('retry', { maxRetries: 2, retryDelayMs: 1000, repairAgent: "master-lead", repairRetries: 2 })`; + return `${indent}.onError('retry', { maxRetries: ${DEFAULT_RETRY_MAX_ATTEMPTS}, retryDelayMs: ${DEFAULT_RETRY_BACKOFF_MS}, repairAgent: "master-lead", repairRetries: ${DEFAULT_REPAIR_RETRY_ATTEMPTS} })`;🤖 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.ts` around lines 1016 - 1023, Replace the hardcoded retry/repair numbers in the .onError replacement with the shared constants used by master-workflow-renderer.ts: import DEFAULT_RETRY_MAX_ATTEMPTS, DEFAULT_RETRY_BACKOFF_MS, and DEFAULT_REPAIR_RETRY_ATTEMPTS at the top of src/local/auto-fix-loop.ts, then use those constants instead of 2, 1000, and 2 in the returned string inside the next.replace callback (the callback that currently pushes 'replaced fail-fast error handling with repair-aware retry'); this keeps retry, backoff and repair retry values consistent across modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/local/auto-fix-loop.ts`:
- Around line 1016-1023: Replace the hardcoded retry/repair numbers in the
.onError replacement with the shared constants used by
master-workflow-renderer.ts: import DEFAULT_RETRY_MAX_ATTEMPTS,
DEFAULT_RETRY_BACKOFF_MS, and DEFAULT_REPAIR_RETRY_ATTEMPTS at the top of
src/local/auto-fix-loop.ts, then use those constants instead of 2, 1000, and 2
in the returned string inside the next.replace callback (the callback that
currently pushes 'replaced fail-fast error handling with repair-aware retry');
this keeps retry, backoff and repair retry values consistent across modules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6c16171-c532-40a6-8872-79cc0c1ec53d
📒 Files selected for processing (7)
.trajectories/completed/2026-05/traj_b77ch3xz99r8.json.trajectories/completed/2026-05/traj_b77ch3xz99r8.md.trajectories/index.jsonsrc/local/auto-fix-loop.test.tssrc/local/auto-fix-loop.tssrc/product/generation/master-workflow-renderer.tssrc/product/generation/pipeline.test.ts
✅ Files skipped from review due to trivial changes (2)
- .trajectories/completed/2026-05/traj_b77ch3xz99r8.json
- .trajectories/completed/2026-05/traj_b77ch3xz99r8.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/generation/pipeline.test.ts
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
Related
Verification