Uh oh!
There was an error while loading. Please reload this page.
Improve workflow generation feedback and rg fallback - #52
Conversation
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds granular local-generation progress logs and an optional Workforce-persona generation path, enforces ripgrep fallback/guarding in generation validation and guidance, and extends the CLI to support a new one-shot ChangesLocal Workflow Progress and Persona Integration
Ripgrep Fallback Validation and Guidance
CLI Workflow Command Support
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)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d17de78111
ℹ️ 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 probesRipgrep = /\b(?:command\s+-v|which)\s+rg\b/.test(normalized); | ||
| const hasPortableSearch = /\b(?:git\s+grep|grep)\b/.test(normalized); | ||
| const hasOrFallback = /\brg\b.+\|\|.+\b(?:git\s+grep|grep)\b/.test(normalized); | ||
| return (probesRipgrep && hasPortableSearch) || hasOrFallback; |
There was a problem hiding this comment.
When a generated gate probes rg and also contains any grep command that is not actually a fallback, this returns true because (probesRipgrep && hasPortableSearch) ignores command ordering/control flow. For example, command -v rg && rg foo src && grep bar README.md passes validation but still fails immediately on systems without ripgrep, which is the environment this new check is meant to protect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6671ae8. hasRipgrepFallback now requires actual fallback control flow (if command -v rg ... then rg ... else grep/git grep ... fi or rg ... || grep/git grep ...) instead of accepting a probe plus unrelated grep. Added a regression test for command -v rg && rg ... && grep ....
| ' ricky status Show local and Cloud readiness', | ||
| ' ricky connect cloud Connect AgentWorkforce Cloud', | ||
| ' ricky workflow --spec-file <path> --run Generate, then run a workflow', | ||
| ' ricky workflow --spec-file <path> --mode cloud --run Generate, then start via Cloud', |
There was a problem hiding this comment.
For ricky workflow --spec-file ... --mode cloud --run, runRequested is only used for summary wording; buildCloudRequest sends a generate request for spec-file handoff, and cloudRickyExecutionBodyFor adds run/auto-fix fields only when the handoff is an existing artifact. Users following this help entry will submit Cloud generation, not start or monitor a Cloud run as advertised.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6671ae8. The help text no longer advertises workflow --mode cloud --run for spec generation; it now describes the Cloud spec path as generating a Cloud workflow artifact. Tests now assert the Cloud workflow form does not imply runRequested.
d17de78 to
6671ae8CompareThere was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pipeline.test.ts`:
- Around line 685-689: The test currently asserts that validation.issues does
not equal an array containing both codes together, so it only fails if both
RIPGREP_REQUIRES_FALLBACK and GREP_GATE_MISSING are present simultaneously;
change it to two separate negative assertions so each code is checked
individually: replace the single
expect(validation.issues).not.toEqual(expect.arrayContaining([expect.objectContaining({
code: 'RIPGREP_REQUIRES_FALLBACK' }), expect.objectContaining({ code:
'GREP_GATE_MISSING' })])) with two assertions that each run
expect(validation.issues).not.toEqual(expect.arrayContaining([expect.objectContaining({
code: 'RIPGREP_REQUIRES_FALLBACK' })])) and
expect(validation.issues).not.toEqual(expect.arrayContaining([expect.objectContaining({
code: 'GREP_GATE_MISSING' })])); this ensures validation.issues is validated
separately for each code.
In `@src/product/generation/pipeline.ts`:
- Around line 385-391: The current hasRipgrepFallback allows any presence of
"command -v rg" plus any "grep" to pass; change it so it only accepts explicit
fallback control flow: keep hasOrFallback but tighten it to detect direct
fallback operators (e.g., /\brg\b\s*\|\|\s*(?:git\s+grep|grep)\b/), and add
checks for common fallback constructs that run grep when rg fails such as
"command -v rg ... || ...grep" and shell-negation/if patterns (e.g.,
/\bcommand\s+-v\s+rg\b[^|]*\|\|[^;]*\b(?:git\s+grep|grep)\b/ and
/\bif\s+!\s*command\s+-v\s+rg\b[\s\S]*?\b(?:git\s+grep|grep)\b/); remove the
loose branch that returns true for probesRipgrep && hasPortableSearch so only
these explicit fallback-pattern matches cause hasRipgrepFallback to return true.
In `@src/surfaces/cli/flows/power-user-parser.ts`:
- Around line 69-76: The current `mode` selection silently defaults to 'local'
for `surface === 'workflow'` when `readMode(effectiveArgv)` returns undefined;
change this to reject explicitly-invalid `--mode` values by checking whether the
user actually provided the `--mode` flag (inspect `effectiveArgv` or enhance
`readMode` to report “explicit but invalid”), and if `surface === 'workflow'`
and the flag was present but `explicitMode === undefined` throw/return a parser
error instead of falling back to 'local'; otherwise keep the existing behavior
of defaulting to 'local' when `--mode` was not provided. Also apply the same
explicit-flag vs implicit-default handling to the analogous logic around
`explicitMode`/`mode` later in the file (the block referenced at lines 108–116).
🪄 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: 93bca36c-a5bb-4b1a-8b44-520adc6a3cdf
📒 Files selected for processing (11)
src/local/entrypoint.test.tssrc/local/entrypoint.tssrc/product/generation/pipeline.test.tssrc/product/generation/pipeline.tssrc/product/generation/template-renderer.tssrc/product/generation/workforce-persona-writer.test.tssrc/product/generation/workforce-persona-writer.tssrc/surfaces/cli/commands/cli-main.test.tssrc/surfaces/cli/commands/cli-main.tssrc/surfaces/cli/flows/power-user-parser.test.tssrc/surfaces/cli/flows/power-user-parser.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
6671ae8 to
b89e996CompareUh oh!
There was an error while loading. Please reload this page.
Summary
ricky workflow --spec-file <path> --runone-shot command surface for spec-to-workflow-to-run, with Cloud mode support via--mode cloud.rggates unless a grep fallback is present.Review
Tests
npm test -- --run src/surfaces/cli/flows/power-user-parser.test.ts src/surfaces/cli/commands/cli-main.test.ts src/local/entrypoint.test.ts src/product/generation/pipeline.test.ts src/product/generation/workforce-persona-writer.test.tsnpm run typecheckgit diff --check -- src/local/entrypoint.test.ts src/local/entrypoint.ts src/product/generation/pipeline.test.ts src/product/generation/pipeline.ts src/product/generation/template-renderer.ts src/product/generation/workforce-persona-writer.test.ts src/product/generation/workforce-persona-writer.ts src/surfaces/cli/commands/cli-main.test.ts src/surfaces/cli/commands/cli-main.ts src/surfaces/cli/flows/power-user-parser.test.ts src/surfaces/cli/flows/power-user-parser.ts