[codex] Document Conductor command wrapper#739
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDocumentation update to ChangesConductor Execution and Compatibility Guidance
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review app commands
For setup details, comment |
Code Review — PR #739: Document Conductor command wrapperOverall: This is a clean, low-risk documentation PR. The content is accurate (verified against Issues1. Build command section is inconsistent with the wrapper guidance The
2. Missing version-number examples in symptoms list CLAUDE.md includes concrete version numbers in the symptom list (e.g., "returns system Ruby (e.g., 2.6.10) instead of project Ruby (e.g., 3.4.3)"). This detail is helpful — it lets an agent recognise the problem immediately from tool output. Consider adding similar specificity to AGENTS.md. 3. Missing reference link CLAUDE.md links to react_on_rails-demos#105 as background reading. Omitting it from AGENTS.md is fine if intentional, but the link provides useful context for anyone investigating the issue further. 4. Missing inline comment on The Minor NitThe added Working Rules bullet ("Run Ruby, Bundler, Node, pnpm, and git hook-sensitive commands through Positives
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| - `prompts/`: shared prompt templates for Codex, GPT, and other non-Claude tools | ||
|
|
||
| ## Build and Test Commands |
There was a problem hiding this comment.
The commands shown here (e.g. bundle exec rspec) are the bare forms, but the Conductor Compatibility section added below recommends wrapping them all with bin/conductor-exec. An agent reading sequentially could run these bare commands before reaching that guidance.
Consider adding a brief callout here, e.g.:
| ## Build and Test Commands | |
| ## Build and Test Commands | |
| > **Conductor users:** prefix every command below with `bin/conductor-exec` — see [Conductor Compatibility](#conductor-compatibility). |
Or alternatively, show the wrapped forms here and drop the duplication in the Conductor section.
| ## Working Rules | ||
|
|
||
| - When the user asks to address PR review comments, follow `prompts/address-review.md`. | ||
| - Run Ruby, Bundler, Node, pnpm, and git hook-sensitive commands through `bin/conductor-exec` so non-interactive shells use the project tool versions. |
There was a problem hiding this comment.
"non-interactive shells" is generic — the section below clarifies this is specifically a Conductor issue. Tying the rule to Conductor here makes the bullet self-contained and matches the section heading just below.
| - Run Ruby, Bundler, Node, pnpm, and git hook-sensitive commands through `bin/conductor-exec` so non-interactive shells use the project tool versions. | |
| - Run Ruby, Bundler, Node, pnpm, and git hook-sensitive commands through `bin/conductor-exec` when running inside Conductor, so version-managed tools are used instead of system defaults. |
| bin/conductor-exec ruby --version | ||
| bin/conductor-exec bundle exec rubocop | ||
| bin/conductor-exec pnpm install | ||
| bin/conductor-exec git commit -m "message" |
There was a problem hiding this comment.
Worth adding the same inline comment that bin/conductor-exec itself uses — it explains the non-obvious reason you'd wrap a plain git commit:
| bin/conductor-exec git commit -m "message" | |
| bin/conductor-exec git commit -m "message" # Pre-commit hooks use the correct tool versions |
|
✅ Review app for PR #739 is deleted |
Code ReviewPR: [codex] Document Conductor command wrapper OverviewThis PR adds two sections to Issues1. Inconsistency between the two new sections (medium) The bundle exec rspec
bundle exec rubocopBut the 2. Misleading fallback description (low)
For users of asdf, rbenv, nvm, or nodenv, the fallback is 3. The Conductor examples show 4.
Positives
|
|
|
||
| # Auto-fix linting issues | ||
| bundle exec rubocop -a | ||
| ``` |
There was a problem hiding this comment.
The commands here are shown without bin/conductor-exec, but the Working Rules section (line 28) says to run Ruby/Bundler/Node commands through that wrapper. An agent scanning only this block will run them bare — exactly the failure mode documented in the Conductor Compatibility section below.
Consider adding a note, e.g.:
In Conductor / non-interactive shells, prefix these with
bin/conductor-exec(see Conductor Compatibility).
Or restructure so the wrapper examples in the Conductor section cover rspec and rubocop too.
| bin/conductor-exec git commit -m "message" | ||
| ``` | ||
|
|
||
| The wrapper uses `mise exec` when mise is available and falls back to direct execution |
There was a problem hiding this comment.
The fallback is exec "$@" — plain direct execution — which also won't resolve correct tool versions for asdf/rbenv/nvm/nodenv users in a non-interactive shell. The current wording ("falls back to direct execution for non-mise users") implies it works for them, but they'll silently get system versions too.
| The wrapper uses `mise exec` when mise is available and falls back to direct execution | |
| The wrapper uses `mise exec` when mise is available. For non-mise users (asdf, rbenv, | |
| nvm, nodenv), the fallback runs the command directly; those users may still need to | |
| ensure their version manager is initialised in the shell environment. |
![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: 14a31b4298
ℹ️ 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".
| bin/dev | ||
|
|
||
| # Run tests | ||
| bundle exec rspec |
There was a problem hiding this comment.
The new build/test quick-reference shows running bundle exec commands directly, but this same document now says Bundler commands in non-interactive Conductor shells must go through bin/conductor-exec to avoid wrong tool versions. That inconsistency makes users likely to copy commands that fail with the version-mismatch symptoms described below (e.g., incompatible Ruby), so the examples should be wrapped or explicitly labeled as interactive-shell-only.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR updates
Confidence Score: 4/5Safe to merge — documentation-only change with no runtime impact. The quick-reference commands in the new Build and Test Commands section show bare AGENTS.md — the quick-reference block and the Working Rules bullet are inconsistent with each other. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Developer runs command] --> B{Using bin/conductor-exec?}
B -- Yes --> C{mise available?}
C -- Yes --> D[mise exec -- command\nCorrect tool versions from .tool-versions]
C -- No --> E[Direct execution\nfallback for asdf/rbenv/nvm users]
B -- No --> F{Interactive shell?}
F -- Yes --> G[.zshrc sourced\nmise PATH active\nCommand works correctly]
F -- No --> H[Conductor non-interactive shell\n.zshrc NOT sourced\nmise PATH NOT active]
H --> I[System Ruby/Node used\nVersion mismatch\nPre-commit hooks may fail]
Reviews (1): Last reviewed commit: "Document conductor command wrapper" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ```bash | ||
| # Run development server | ||
| bin/dev | ||
|
|
||
| # Run tests | ||
| bundle exec rspec | ||
|
|
||
| # Run linting | ||
| bundle exec rubocop | ||
|
|
||
| # Auto-fix linting issues | ||
| bundle exec rubocop -a | ||
| ``` |
There was a problem hiding this comment.
bundle exec rspec, bundle exec rubocop, and bundle exec rubocop -a are exactly the kinds of Bundler commands that the Working Rules say must be prefixed with bin/conductor-exec. A developer reading only this section will copy-paste the unwrapped versions and hit the Conductor shell-environment problem the rest of the doc is trying to prevent.
| ```bash | |
| # Run development server | |
| bin/dev | |
| # Run tests | |
| bundle exec rspec | |
| # Run linting | |
| bundle exec rubocop | |
| # Auto-fix linting issues | |
| bundle exec rubocop -a | |
| ``` | |
| ```bash | |
| # Run development server | |
| bin/dev | |
| # Run tests | |
| bin/conductor-exec bundle exec rspec | |
| # Run linting | |
| bin/conductor-exec bundle exec rubocop | |
| # Auto-fix linting issues | |
| bin/conductor-exec bundle exec rubocop -a |
1 participant
Summary
AGENTS.md.bin/conductor-execfor version-managed Ruby, Bundler, Node, pnpm, and git hook-sensitive commands.master.Validation
git diff --checkNotes
I intentionally left the incidental JS formatting-only edits and the stale staged homepage/control-plane diffs out of this PR.
Note
Low Risk
Documentation-only updates to
AGENTS.md; no runtime code paths are modified. Risk is limited to potential developer confusion if instructions are incorrect.Overview
Adds a build/test command quick reference to
AGENTS.md(dev server, RSpec, RuboCop, and auto-fix).Documents Conductor non-interactive shell pitfalls and standardizes guidance to run Ruby/Bundler/Node/pnpm and git-hook-sensitive commands via
bin/conductor-exec, including example before/after command invocations and a note aboutmise execfallback behavior.Reviewed by Cursor Bugbot for commit 14a31b4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation