fix(generation): package long workflow context#59
Conversation
📝 WalkthroughWalkthroughThe PR precomputes and writes a multi-file generated context package, updates renderSource to call a runtime writer, changes prepare-context to validate packaged files and emit readiness, rewires step renderers to read artifactsDir-based inputs (normalized spec, acceptance contract, instructions, checklists, optional target-context), and adds tests that ensure long specs are stored in sidecars not inlined into task bodies. ChangesContext Packaging Refactor
Sequence Diagram(s)sequenceDiagram
participant RS as renderSource
participant Writer as ContextWriter
participant ArtDir as artifactsDir
participant Prepare as prepare-context
participant Steps as workflow steps
RS->>Writer: buildGeneratedContextPackage + write files to ArtDir
Writer-->>ArtDir: files stored
RS->>Prepare: emit prepare-context step (validate files)
Prepare->>ArtDir: check expected files exist
Steps->>ArtDir: read packaged files for lead/impl/review steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/product/generation/pipeline.test.ts (1)
1378-1380: 💤 Low valueMinor fragility in regex if task bodies ever contain backticks.
The non-greedy regex
/task:\s*([\s\S]*?)/gwill terminate at the first backtick character, which would incorrectly truncate extraction if a task body contained escaped backticks (e.g.,\foo``). Current task bodies don't include embedded backticks, so this works today.💡 More robust alternative (optional)
If future task bodies might contain backticks, consider a state-machine approach or matching the
templateLiteralescaping pattern:function renderedTaskBodies(content: string): string[] { - return [...content.matchAll(/task:\s*`([\s\S]*?)`/g)].map((match) => match[1]); + // Match task template literals, accounting for escaped backticks + return [...content.matchAll(/task:\s*`((?:[^`\\]|\\.)*)\\`/g)].map((match) => match[1].replace(/\\`/g, '`')); }🤖 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.test.ts` around lines 1378 - 1380, The helper renderedTaskBodies currently uses a regex that stops at the first backtick and will break if a task body contains an escaped backtick; update renderedTaskBodies to use a pattern that allows either a backslash-escaped backtick or any non-backtick character repeated (i.e., match sequences of "(escaped backtick) or (not backtick)" until the closing backtick) so the capture includes embedded escaped backticks, or alternatively replace the regex with a tiny template-literal parser/state-machine that walks the string and honors backslash escapes when extracting the content after "task:".
🤖 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/product/generation/pipeline.test.ts`:
- Around line 1378-1380: The helper renderedTaskBodies currently uses a regex
that stops at the first backtick and will break if a task body contains an
escaped backtick; update renderedTaskBodies to use a pattern that allows either
a backslash-escaped backtick or any non-backtick character repeated (i.e., match
sequences of "(escaped backtick) or (not backtick)" until the closing backtick)
so the capture includes embedded escaped backticks, or alternatively replace the
regex with a tiny template-literal parser/state-machine that walks the string
and honors backslash escapes when extracting the content after "task:".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ba7022c-841a-4640-80d9-7f684f4c8bcb
📒 Files selected for processing (2)
src/product/generation/pipeline.test.tssrc/product/generation/template-renderer.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab5c6b9040
ℹ️ 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".
| const commands = [ | ||
| `mkdir -p ${shellQuote(artifactsDir)}`, | ||
| `printf '%s\\n' ${shellQuote(spec.description)} > ${shellQuote(`${artifactsDir}/normalized-spec.txt`)}`, | ||
| ...contextPackage.map((file) => `printf '%s\\n' ${shellQuote(file.content)} > ${shellQuote(file.path)}`), |
There was a problem hiding this comment.
For very large specs, this still inlines the entire context package into the generated deterministic prepare-context command; that command has to be handed to the shell/runtime before any sidecar files can be written, so specs approaching the OS command-argument limit can fail in prepare-context instead of being packaged. The new package also duplicates the description into normalized-spec.txt, normalized-spec.md, and acceptance-contract.json, lowering that threshold; write these sidecars from generated TS/Node code or otherwise avoid putting the full package in the command string.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/product/generation/pipeline.test.ts (1)
1362-1365: ⚡ Quick winAvoid hard-coding the current packaging mechanism in this regression.
The important contract here is that the long spec stays out of task bodies and the
prepare-contextcommand while the packaged files remain addressable. Requiringcontentto contain the sentinel andwriteRickyGeneratedContextFiles([will fail if we later materialize the sidecars earlier without changing the externally visible behavior. Prefer asserting the packaged file paths are referenced and the sentinel is absent from prompts/commands.🤖 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.test.ts` around lines 1362 - 1365, The test currently hard-codes the packaging implementation by asserting presence of longSpecSentinel and the writeRickyGeneratedContextFiles call; instead update the assertions to verify only the externally visible contract: keep checks that content references the packaged file paths ('.workflow-artifacts/generated/packaged-context/normalized-spec.md' and '.workflow-artifacts/generated/packaged-context/acceptance-contract.json'), remove the positive assertions for longSpecSentinel and 'writeRickyGeneratedContextFiles([', and add an assertion that longSpecSentinel is absent from prompts/commands (e.g. ensure content does NOT contain longSpecSentinel and that the 'prepare-context' command/task bodies do not include the sentinel), leaving the test resilient to changes in how sidecars are materialized.
🤖 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/product/generation/template-renderer.ts`:
- Around line 781-799: The lead and review read lists currently omit the actual
target context file; update renderLeadPlanStep and renderReviewStep to accept a
hasTargetContext boolean parameter, and when hasTargetContext is true append
`${artifactsDir}/target-context.txt` to the ordered "Read these files in order"
list inside the task template literal; ensure you update all call sites to pass
the new flag and keep the rest of the template (headings, output file, and end
marker) unchanged so both planner and reviewers see the real target context when
present.
---
Nitpick comments:
In `@src/product/generation/pipeline.test.ts`:
- Around line 1362-1365: The test currently hard-codes the packaging
implementation by asserting presence of longSpecSentinel and the
writeRickyGeneratedContextFiles call; instead update the assertions to verify
only the externally visible contract: keep checks that content references the
packaged file paths
('.workflow-artifacts/generated/packaged-context/normalized-spec.md' and
'.workflow-artifacts/generated/packaged-context/acceptance-contract.json'),
remove the positive assertions for longSpecSentinel and
'writeRickyGeneratedContextFiles([', and add an assertion that longSpecSentinel
is absent from prompts/commands (e.g. ensure content does NOT contain
longSpecSentinel and that the 'prepare-context' command/task bodies do not
include the sentinel), leaving the test resilient to changes in how sidecars are
materialized.
🪄 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: 0f6baca1-cdb4-4f9b-943d-b70933b20250
📒 Files selected for processing (2)
src/product/generation/pipeline.test.tssrc/product/generation/template-renderer.ts
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)
src/product/generation/template-renderer.ts (1)
136-147:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove sidecar materialization back into
prepare-context.
writeRickyGeneratedContextFiles(...)runs beforeworkflow(...).run(), whileprepare-contextnow only doestest -f. That means packaging failures bypass step-level retry/reporting, and retryingprepare-contextcannot recreate missing sidecars because the step itself never writes them.Also applies to: 595-603
🤖 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 136 - 147, The generated code currently calls renderGeneratedContextWriterCall (which invokes writeRickyGeneratedContextFiles) before creating and running workflow(...).run(), so prepare-context only tests for files and cannot recreate sidecars on retry; move the sidecar materialization into the prepare-context step by invoking renderGeneratedContextWriterCall from inside the code emitted by renderPrepareContextStep (i.e., ensure renderPrepareContextStep calls writeRickyGeneratedContextFiles/its writer), and remove the earlier standalone renderGeneratedContextWriterCall call before workflow(...).description(...). This ensures prepare-context both creates and verifies sidecars and that retries/reporting apply to materialization.
🤖 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/product/generation/template-renderer.ts`:
- Around line 1058-1064: The code currently allows copying arbitrary filesystem
paths from targetContext into target-context.txt (see
rickyWorkflowFs.existsSync, statSync, copyFileSync, writeFileSync), which can
leak sensitive local files; change the logic to only allow reads when
targetContext.value resolves (use fs.realpath) to a path under the repository
root (process.cwd()) and is a regular file before allowing copyFileSync,
otherwise treat targetContext.value as literal content and write it via
writeFileSync; alternatively require an explicit trusted file-reference flag on
the spec before performing any filesystem reads.
---
Outside diff comments:
In `@src/product/generation/template-renderer.ts`:
- Around line 136-147: The generated code currently calls
renderGeneratedContextWriterCall (which invokes writeRickyGeneratedContextFiles)
before creating and running workflow(...).run(), so prepare-context only tests
for files and cannot recreate sidecars on retry; move the sidecar
materialization into the prepare-context step by invoking
renderGeneratedContextWriterCall from inside the code emitted by
renderPrepareContextStep (i.e., ensure renderPrepareContextStep calls
writeRickyGeneratedContextFiles/its writer), and remove the earlier standalone
renderGeneratedContextWriterCall call before workflow(...).description(...).
This ensures prepare-context both creates and verifies sidecars and that
retries/reporting apply to materialization.
🪄 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: f85de1e8-7199-4bac-b257-9b34db2a354b
📒 Files selected for processing (2)
src/product/generation/pipeline.test.tssrc/product/generation/template-renderer.ts
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)
src/product/generation/template-renderer.ts (1)
132-147:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove context-file materialization back under
prepare-context.
writeRickyGeneratedContextFiles(...)now runs before.workflow(...), so any write/copy failure aborts before the deterministicprepare-contextstep exists, and even dry-run/graph-inspection paths will still mutate.workflow-artifacts. That makes the workflow less observable and shifts a step-owned side effect into process startup.🤖 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 132 - 147, The generated context writer (writeRickyGeneratedContextFiles) is being emitted before the workflow invocation, so failures mutate artifacts during process startup; move the writer invocation into the deterministic prepare-context step. Concretely: remove the call emitted by renderGeneratedContextWriterCall(contextSetup) from the top-level sequence that builds main() (before workflow(...)), and instead ensure renderPrepareContextStep(input.artifactsDir, contextSetup) emits or invokes the generated writer (i.e., have renderPrepareContextStep include the writeRickyGeneratedContextFiles logic or call renderGeneratedContextWriterCall there) so context file materialization happens inside the prepare-context step.
🧹 Nitpick comments (2)
src/product/generation/pipeline.test.ts (2)
1395-1411: ⚡ Quick winCover the implementer prompt too.
target-context.txtis also threaded intoimplement-artifact, but this test only checks the planning and review prompts. A small extra assertion for the body that readsimplementation-instructions.mdwould close the highest-value regression gap here.🤖 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.test.ts` around lines 1395 - 1411, The test threads target-context sidecars into planning and review prompts but misses asserting the implementer prompt; update the test "threads target context sidecars into planning and review prompts" to also check that one of the rendered task bodies (taskBodies from renderedTaskBodies(artifact(result).content)) includes both "implementation-instructions.md" (or the implementer prompt identifier "implement-artifact") and "target-context.txt" so the implement-artifact/implementation-instructions.md prompt is validated for the presence of the target context sidecar.
1362-1393: ⚡ Quick winAssert the sentinel survives inside the packaged context payload, not just outside task bodies.
This regression currently proves “not inlined into prompts,” but it does not prove “still available in the generated context package.” It would still pass if the generator emitted the sidecar filenames but dropped the long spec content from the serialized
normalized-spec.md/acceptance-contract.jsonentries. Add one positive assertion against the generated-context file payload itself.🤖 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.test.ts` around lines 1362 - 1393, Add a positive assertion that the longSpecSentinel actually appears inside the packaged-context sidecar payload itself: after you get content = artifact(result).content, load the generated sidecar file (e.g. the normalized-spec.md path previously asserted: '.workflow-artifacts/generated/packaged-context/normalized-spec.md') from the generated artifact and assert it contains longSpecSentinel (and optionally also check acceptance-contract.json contains it). This ensures the sentinel is present in the serialized packaged context rather than only referenced by filename in task bodies; reference artifact(result).content, longSpecSentinel and the sidecar filename when locating the payload to assert.
🤖 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/product/generation/template-renderer.ts`:
- Around line 723-750: The rendered lead-plan text requires the literal
"Non-goals" heading but buildLeadPlanGateCommand(...) still accepts synonyms
like "out of scope", causing a validation mismatch; fix by updating
buildLeadPlanGateCommand to validate for the exact heading "Non-goals" (reject
synonyms) so the gate enforces the same contract the template expects, and
ensure any tests or callers that construct the gate (e.g., code paths that
reference artifactsDir/lead-plan.md or GENERATION_LEAD_PLAN_READY) are updated
to use the exact literal; alternatively, if you prefer to relax the contract,
change the template text in template-renderer.ts to allow the accepted synonyms
instead of requiring the exact heading—pick one approach and make the
corresponding change consistently in buildLeadPlanGateCommand and the
lead-plan-instructions content.
---
Outside diff comments:
In `@src/product/generation/template-renderer.ts`:
- Around line 132-147: The generated context writer
(writeRickyGeneratedContextFiles) is being emitted before the workflow
invocation, so failures mutate artifacts during process startup; move the writer
invocation into the deterministic prepare-context step. Concretely: remove the
call emitted by renderGeneratedContextWriterCall(contextSetup) from the
top-level sequence that builds main() (before workflow(...)), and instead ensure
renderPrepareContextStep(input.artifactsDir, contextSetup) emits or invokes the
generated writer (i.e., have renderPrepareContextStep include the
writeRickyGeneratedContextFiles logic or call renderGeneratedContextWriterCall
there) so context file materialization happens inside the prepare-context step.
---
Nitpick comments:
In `@src/product/generation/pipeline.test.ts`:
- Around line 1395-1411: The test threads target-context sidecars into planning
and review prompts but misses asserting the implementer prompt; update the test
"threads target context sidecars into planning and review prompts" to also check
that one of the rendered task bodies (taskBodies from
renderedTaskBodies(artifact(result).content)) includes both
"implementation-instructions.md" (or the implementer prompt identifier
"implement-artifact") and "target-context.txt" so the
implement-artifact/implementation-instructions.md prompt is validated for the
presence of the target context sidecar.
- Around line 1362-1393: Add a positive assertion that the longSpecSentinel
actually appears inside the packaged-context sidecar payload itself: after you
get content = artifact(result).content, load the generated sidecar file (e.g.
the normalized-spec.md path previously asserted:
'.workflow-artifacts/generated/packaged-context/normalized-spec.md') from the
generated artifact and assert it contains longSpecSentinel (and optionally also
check acceptance-contract.json contains it). This ensures the sentinel is
present in the serialized packaged context rather than only referenced by
filename in task bodies; reference artifact(result).content, longSpecSentinel
and the sidecar filename when locating the payload to assert.
🪄 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: 0191b4a4-eac6-4f3d-b879-909644703fff
📒 Files selected for processing (2)
src/product/generation/pipeline.test.tssrc/product/generation/template-renderer.ts
| path: `${artifactsDir}/lead-plan-instructions.md`, | ||
| content: [ | ||
| '# Lead Plan Instructions', | ||
| '', | ||
| 'Plan the workflow execution from the packaged context files, not from the short task prompt.', | ||
| '', | ||
| 'Required sections:', | ||
| '', | ||
| '- Non-goals', | ||
| '- Routing contract', | ||
| '- Implementation contract', | ||
| '- Deliverables', | ||
| '- Verification gates', | ||
| '', | ||
| 'Use this exact section heading in the lead plan. Do not rename "Non-goals" to "Out of scope" or another synonym.', | ||
| '', | ||
| `Write ${artifactsDir}/lead-plan.md and end it with GENERATION_LEAD_PLAN_READY.`, | ||
| '', | ||
| 'Generation-time skill boundary:', | ||
| '', | ||
| `- Read ${artifactsDir}/skill-application-boundary.json and treat it as generator metadata only.`, | ||
| '- Skills are applied by Ricky during selection, loading, and template rendering.', | ||
| '- Do not claim generated agents load, retain, or embody skill files at runtime unless a future runtime test proves that path.', | ||
| '', | ||
| 'Loaded skills summary:', | ||
| '', | ||
| loadedSkillsReport, | ||
| ].join('\n'), |
There was a problem hiding this comment.
The new lead-plan contract is stricter than the gate that enforces it.
These instructions now require the exact Non-goals heading, but buildLeadPlanGateCommand(...) still accepts out of scope/not in scope. The workflow can therefore pass deterministic validation while violating its packaged contract. Either tighten the gate to require the literal heading or relax this instruction text.
🤖 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 723 - 750, The
rendered lead-plan text requires the literal "Non-goals" heading but
buildLeadPlanGateCommand(...) still accepts synonyms like "out of scope",
causing a validation mismatch; fix by updating buildLeadPlanGateCommand to
validate for the exact heading "Non-goals" (reject synonyms) so the gate
enforces the same contract the template expects, and ensure any tests or callers
that construct the gate (e.g., code paths that reference
artifactsDir/lead-plan.md or GENERATION_LEAD_PLAN_READY) are updated to use the
exact literal; alternatively, if you prefer to relax the contract, change the
template text in template-renderer.ts to allow the accepted synonyms instead of
requiring the exact heading—pick one approach and make the corresponding change
consistently in buildLeadPlanGateCommand and the lead-plan-instructions content.
1 participant
Summary
Validation