feat(pair): open the editor and launch an agent - #207
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds a ChangesPair command
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PairCommand
participant GraphQLClient
participant Sessions
participant Agent
User->>PairCommand: run pair target
PairCommand->>GraphQLClient: resolve workspace or dataset editor
GraphQLClient-->>PairCommand: editor URL and token
PairCommand->>Sessions: wait for authenticated session
Sessions-->>PairCommand: session ready
PairCommand->>Agent: launch with pairing prompt
Agent-->>User: pairing session
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/commands/pair/session.rs (1)
65-81: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
waitsleeps once more after the final failed check.The loop checks readiness, then always sleeps, even when the deadline has passed. The command therefore waits up to
TIMEOUT + POLL_INTERVALbefore it reports the error. Move the sleep behind a remaining-time check.🤖 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/commands/pair/session.rs` around lines 65 - 81, Update Session::wait so it checks the remaining time after each failed is_ready call and only sleeps when the deadline has not been reached, preventing an extra POLL_INTERVAL delay before returning the existing timeout error.
🤖 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/commands/pair/mod.rs`:
- Line 99: Update the process-command construction in the pair execution flow
around agent.command(...).spawn() to enable kill-on-drop for the spawned
interactive agent, ensuring it is terminated when Cli::do_run aborts pair during
shutdown. Preserve the existing wait and error propagation behavior.
- Around line 97-100: Update the Some(agent) branch in the pair command so the
ExitStatus returned by agent.command(&prompt).spawn()?.wait().await? is checked
and a non-zero status is propagated as a command error, ensuring aqora pair
reports agent failures to scripts and CI.
In `@src/commands/pair/prompt.rs`:
- Around line 37-48: Update the token-file lifecycle in write_token and its
caller pair so the temporary directory is removed when the pairing agent exits,
while keeping the private directory and file permissions unchanged. Ensure the
returned path remains usable for the active session and cleanup occurs after
that session completes rather than leaving accumulated token files behind.
In `@src/commands/pair/session.rs`:
- Around line 30-32: Update the reqwest client construction in the session setup
around Client::builder to configure a per-request timeout shorter than
POLL_INTERVAL plus the total wait budget, while preserving the existing
invalid-certificate behavior and error propagation.
In `@src/commands/pair/target.rs`:
- Around line 154-171: Update the missing-workspace error advice in the version
lookup around workspace.version to interpolate the actual owner and workspace
values in the aqora pair command, instead of displaying literal
{owner}/{workspace} placeholders; preserve the existing guidance and error
behavior.
---
Nitpick comments:
In `@src/commands/pair/session.rs`:
- Around line 65-81: Update Session::wait so it checks the remaining time after
each failed is_ready call and only sleeps when the deadline has not been
reached, preventing an extra POLL_INTERVAL delay before returning the existing
timeout error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0ce4913-8791-443d-9a8e-1b99ea185566
📒 Files selected for processing (8)
src/commands/mod.rssrc/commands/pair/agent.rssrc/commands/pair/mod.rssrc/commands/pair/prompt.rssrc/commands/pair/session.rssrc/commands/pair/target.rssrc/graphql/workspace_pair_editor.graphqlsrc/graphql/workspace_version_pair_editor.graphql
| match agent { | ||
| Some(agent) => { | ||
| pb.finish_with_message(format!("Launching {}", agent.display_name())); | ||
| agent.command(&prompt).spawn()?.wait().await?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the shutdown handler and any existing kill_on_drop usage.
rg -n -C 8 'fn shutdown_signal' src
rg -n 'kill_on_drop|process::Command' srcRepository: aqora-io/cli
Length of output: 1289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files involving pair, shutdown, run commands:\n'
git ls-files | rg '(^src/commands/pair/|src/shutdown\.rs|src/commands/run\.rs|src/commands/prompt|src/main\.rs|Cargo\.toml)'
printf '\n-- src/commands/pair/mod.rs outline/contents --\n'
wc -l src/commands/pair/mod.rs
sed -n '1,160p' src/commands/pair/mod.rs
printf '\n-- src/shutdown.rs --\n'
sed -n '1,120p' src/shutdown.rs
printf '\n-- pair agent command implementation --\n'
sed -n '1,140p' src/commands/pair/agent.rs
printf '\n-- select usages --\n'
rg -n -C 3 'tokio::select!|shutdown_signal|ctrl_c|tokio::process::Command' src/commands src/main.rs app 2>/dev/null || true
printf '\n-- Cargo deps tokio signal/process --\n'
if [ -f Cargo.toml ]; then
sed -n '1,220p' Cargo.toml
fiRepository: aqora-io/cli
Length of output: 16606
Avoid leaving the interactive agent running after shutdown.
Cli::do_run runs pair inside tokio::select! { run, shutdown_signal() }. When Ctrl-C selects the shutdown branch, pair is aborted and dropped, but tokio::process::Command::spawn() does not kill the agent on drop unless kill_on_drop(true) is set. Use kill_on_drop(true) or omit the shutdown race from pair so the client cannot exit while the agent still owns the terminal.
🤖 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/commands/pair/mod.rs` at line 99, Update the process-command construction
in the pair execution flow around agent.command(...).spawn() to enable
kill-on-drop for the spawned interactive agent, ensuring it is terminated when
Cli::do_run aborts pair during shutdown. Preserve the existing wait and error
propagation behavior.
6b51754 to
09edc23
Compare
|
What do you think about putting that behind a flag? That way, if I want, I can compile the CLI without the agent-related features (as a default=true option). I think this would be useful for people who don't want to work with AI |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/commands/pair/mod.rs (1)
17-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftGate the agent-related CLI surface behind a compile-time feature, per PR discussion.
The
claude,codex, andopencodeflags, along with theagentmodule dependency, are always compiled into thepaircommand. A reviewer on this PR requested that agent-related features sit behind a compile-time flag, enabled by default, so the CLI can build without them for users who do not want AI integration. This file is the wiring point where such a#[cfg(feature = "agent")]gate (or similar) would need to apply to theclaude/codex/opencodeargs, theagent()method, and themod agent;import.🤖 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/commands/pair/mod.rs` around lines 17 - 45, Gate the agent integration behind the default-enabled "agent" compile-time feature: conditionally compile the claude, codex, and opencode fields in Pair, the Pair::agent() method, and the mod agent; declaration using the same feature. Ensure the pair command still builds and exposes its non-agent behavior when the feature is disabled, including updating prompt_only’s agent conflict configuration as needed.
🤖 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/commands/pair/prompt.rs`:
- Around line 11-31: Update build_prompt to POSIX-shell-quote both
editor.base_url and token_path before inserting them into the generated
execute-code.sh commands, including the URL’s single-quoted argument. Ensure
embedded quotes and shell metacharacters cannot alter command parsing, and use
cat -- when reading the quoted token path.
In `@src/commands/pair/target.rs`:
- Around line 288-296: Update the pinned dataset resolution flow around
DatasetVersionPairEditor to preserve semver prerelease/build metadata when the
schema supports it; otherwise reject targets whose version contains pre or build
with a user-facing error. Replace direct major/minor/patch as i64 casts with
i64::try_from, propagating a user error for out-of-range components instead of
allowing wrapping.
In `@src/graphql/workspace_pair_editor.graphql`:
- Line 4: The draft queries rely on unspecified version ordering and may select
the wrong draft. Update src/graphql/workspace_pair_editor.graphql:4-4 and
src/graphql/dataset_pair_editor.graphql:4-4 to request newest-first ordering; if
the schema cannot order these connections, update resolve_draft at
src/commands/pair/target.rs:232-239 and resolve_dataset_draft at
src/commands/pair/target.rs:346-351 to select the returned node with the highest
version instead of the first.
---
Nitpick comments:
In `@src/commands/pair/mod.rs`:
- Around line 17-45: Gate the agent integration behind the default-enabled
"agent" compile-time feature: conditionally compile the claude, codex, and
opencode fields in Pair, the Pair::agent() method, and the mod agent;
declaration using the same feature. Ensure the pair command still builds and
exposes its non-agent behavior when the feature is disabled, including updating
prompt_only’s agent conflict configuration as needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5a70573-6cae-45a4-b5f1-1f5a223611e9
📒 Files selected for processing (10)
src/commands/mod.rssrc/commands/pair/agent.rssrc/commands/pair/mod.rssrc/commands/pair/prompt.rssrc/commands/pair/session.rssrc/commands/pair/target.rssrc/graphql/dataset_pair_editor.graphqlsrc/graphql/dataset_version_pair_editor.graphqlsrc/graphql/workspace_pair_editor.graphqlsrc/graphql/workspace_version_pair_editor.graphql
🚧 Files skipped from review as they are similar to previous changes (4)
- src/graphql/workspace_version_pair_editor.graphql
- src/commands/mod.rs
- src/commands/pair/session.rs
- src/commands/pair/agent.rs
| pub fn build_prompt(editor: &PairEditor, token_path: &Path, editor_page: &Url) -> String { | ||
| format!( | ||
| "Use the /marimo-pair skill to pair-program on a running marimo notebook. | ||
|
|
||
| Connect to the notebook at: {base_url} | ||
|
|
||
| Use `execute-code.sh --url {base_url}` from the marimo-pair skill to execute code in the \ | ||
| notebook. | ||
|
|
||
| An auth token is stored at {token_path}. Pass it via `execute-code.sh --url '{base_url}' \ | ||
| --token \"$(cat '{token_path}')\"`. | ||
|
|
||
| The notebook must be open in a browser for a session to exist. If the server reports no \ | ||
| active sessions, ask the user to open {editor_page} and then try again. | ||
|
|
||
| Once you are connected, send a fun toast (mo.status.toast(...)) to the user inside marimo \ | ||
| letting them know you're ready to pair.", | ||
| base_url = editor.base_url, | ||
| token_path = token_path.display(), | ||
| editor_page = editor_page, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape values before inserting them into shell syntax.
Line 17 inserts editor.base_url into a single-quoted shell argument without escaping embedded single quotes. A URL path can contain ' and command-substitution characters. This closes the argument quote. An agent that follows the generated command can execute injected shell code.
Use POSIX-safe quoting for both the URL and token path. Use cat -- for the token path.
Proposed fix
+fn shell_quote(value: &str) -> String {
+ format!("'{}'", value.replace('\'', "'\"'\"'"))
+}
+
pub fn build_prompt(editor: &PairEditor, token_path: &Path, editor_page: &Url) -> String {
+ let base_url = editor.base_url.as_str();
+ let token_path = token_path.display().to_string();
+ let shell_base_url = shell_quote(base_url);
+ let shell_token_path = shell_quote(&token_path);
+
format!(
"Use the /marimo-pair skill to pair-program on a running marimo notebook.
...
-Use `execute-code.sh --url {base_url}` from the marimo-pair skill to execute code in the \
+Use `execute-code.sh --url {shell_base_url}` from the marimo-pair skill to execute code in the \
notebook.
...
-An auth token is stored at {token_path}. Pass it via `execute-code.sh --url '{base_url}' \
---token \"$(cat '{token_path}')\"`.
+An auth token is stored at {token_path}. Pass it via `execute-code.sh --url {shell_base_url} \
+--token \"$(cat -- {shell_token_path})\"`.
...
- base_url = editor.base_url,
- token_path = token_path.display(),
+ base_url,
+ token_path,
+ shell_base_url,
+ shell_token_path,
editor_page = editor_page,
)
}🤖 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/commands/pair/prompt.rs` around lines 11 - 31, Update build_prompt to
POSIX-shell-quote both editor.base_url and token_path before inserting them into
the generated execute-code.sh commands, including the URL’s single-quoted
argument. Ensure embedded quotes and shell metacharacters cannot alter command
parsing, and use cat -- when reading the quoted token path.
| query WorkspacePairEditor($owner: String!, $slug: String!, $notebook: String) { | ||
| workspaceBySlug(owner: $owner, slug: $slug) { | ||
| id | ||
| versions(first: 1, filters: { published: false }) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Draft selection depends on an unstated server ordering. Both draft queries request versions(first: 1, filters: { published: false }) without an order argument, and both Rust resolvers treat the single returned node as the newest draft. If the server default order is not newest-first, pairing opens the wrong draft version.
src/graphql/workspace_pair_editor.graphql#L4-L4: add an explicit newest-first order argument to theversionsconnection, or raisefirstso the caller can choose.src/graphql/dataset_pair_editor.graphql#L4-L4: apply the same explicit ordering to the datasetversionsconnection.src/commands/pair/target.rs#L232-L239: if the schema cannot order the connection, select the highestversionfrom the returned nodes inresolve_draftinstead of taking the first node.src/commands/pair/target.rs#L346-L351: apply the same selection rule inresolve_dataset_draft.
📍 Affects 3 files
src/graphql/workspace_pair_editor.graphql#L4-L4(this comment)src/graphql/dataset_pair_editor.graphql#L4-L4src/commands/pair/target.rs#L232-L239src/commands/pair/target.rs#L346-L351
🤖 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/graphql/workspace_pair_editor.graphql` at line 4, The draft queries rely
on unspecified version ordering and may select the wrong draft. Update
src/graphql/workspace_pair_editor.graphql:4-4 and
src/graphql/dataset_pair_editor.graphql:4-4 to request newest-first ordering; if
the schema cannot order these connections, update resolve_draft at
src/commands/pair/target.rs:232-239 and resolve_dataset_draft at
src/commands/pair/target.rs:346-351 to select the returned node with the highest
version instead of the first.
|
I mean this is a fairly light feature (no extra deps, a few lines of code) so putting it behind a flag seems excessive. If you don't want to use it, you don't have to but I don't see the reasoning behind feature gating it |
|
I was just being very conservative with the Agents feature, but you're right it's really lightweight, so it's fine!! |
2 participants
Summary by CodeRabbit
paircommand for connecting workspace or dataset notebooks to supported coding agents.