Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughImplements a Subagents feature: discovery of Markdown/YAML agent definitions, inventory and invocation pipeline, invoke_subagent tool and approval protocol, CLI/TUI commands (list/run/show), Node IPC client, Rust core executor, protocol types, docs/specs, and extensive tests and CI/script updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant NodeCLI as codex-cli
participant CodexBin as codex agents CLI
participant Core as codex-rs core
participant Exec as executor
Note over User,NodeCLI: User requests running a subagent
User->>NodeCLI: codex agents run --name code-reviewer
NodeCLI->>CodexBin: spawn codex agents run (IPC)
CodexBin->>Core: build_inventory_for_config(config)
Core->>Core: SubagentRunner::invoke(session) -> Prepared or ConfirmationRequired
alt ConfirmationRequired
Core-->>CodexBin: return confirmation-required error
CodexBin-->>NodeCLI: propagate error
NodeCLI-->>User: prompt for confirmation
else Prepared
Core->>Exec: execute_subagent_invocation(prepared)
Exec->>Exec: spawn Codex instance, stream events, persist transcript
Exec-->>Core: InvocationSession result (summary, artifacts, model)
Core-->>CodexBin: RunOutput
CodexBin-->>NodeCLI: stdout result
NodeCLI-->>User: display result
end
sequenceDiagram
autonumber
participant Core
participant TUI
participant Widget
participant Human
participant Exec as NonInteractiveExec
Note over Core,TUI: Approval flow (interactive)
Core-->>TUI: EventMsg::SubagentApprovalRequest
TUI->>Widget: render approval prompt
Human-->>Widget: Approve / Deny
Widget-->>Core: Op::SubagentApproval(decision)
Core->>Core: proceed or abort invocation
Note over Core,Exec: Non-interactive (auto-deny)
Core-->>Exec: EventMsg::SubagentApprovalRequest
Exec->>Exec: auto-deny -> Op::SubagentApproval(Denied)
Exec-->>Core: denial op processed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codex-rs/core/src/spawn.rs (1)
47-49: Avoid logging environment values; risk of leaking secrets
trace!(… {env:?})may expose tokens/keys. Log only keys (or count) instead.Apply this diff:
- trace!( - "spawn_child_async: {program:?} {args:?} {arg0:?} {cwd:?} {sandbox_policy:?} {stdio_policy:?} {env:?}" - ); + trace!( + program = ?program, + args = ?args, + arg0 = ?arg0, + cwd = ?cwd, + sandbox_policy = ?sandbox_policy, + stdio_policy = ?stdio_policy, + env_keys = ?env.keys().collect::<Vec<_>>(), + "spawn_child_async" + );
🧹 Nitpick comments (77)
codex-rs/core/src/spawn.rs (1)
57-60: Use Command::envs for clarityThe per-key loop is fine but unnecessary. Using envs is more idiomatic and concise.
Apply this diff:
- for (key, value) in &env { - cmd.env(key, value); - } + cmd.envs(&env);codex-rs/tui/tests/suite/subagents_quickstart.rs (1)
11-11: Make the first-step assertion less brittle across platforms.Hard‑coding "~/.codex/config.toml" can break on non‑POSIX environments. Match the prefix instead.
- assert_eq!(steps[0], "Enable subagents via ~/.codex/config.toml"); + assert!(steps[0].starts_with("Enable subagents via "), + "first quickstart step should begin with 'Enable subagents via ...'");docs/subagents.md (2)
44-54: Great workflow section; add a brief JSON example and verify cross‑refs.
- Consider adding a tiny
agents list --jsonsample for quick copy/paste.- Verify that
docs/subagents-tool.mdexists and the link/path is correct.
25-27: Conflict precedence note is good; call out exact resolution rule.You already imply it; explicitly say “project overrides user when names collide” to remove ambiguity.
-Project definitions override user definitions when names collide. +When names collide, the project-scoped definition overrides the user-scoped one.codex-rs/tui/src/user_approval_widget.rs (1)
443-449: Prefer theme/stylize helpers over hard‑coded colors.The cyan/black combo can clash with some themes. Consider Stylize helpers and a theme‑provided accent instead of fixed colors.
- let style = if idx == self.selected_option { - Style::new().bg(Color::Cyan).fg(Color::Black) - } else { - Style::new().add_modifier(Modifier::DIM) - }; + let style = if idx == self.selected_option { + Style::new().reversed() // or `.underlined()` / theme accent + } else { + Style::new().add_modifier(Modifier::DIM) + };- Block::bordered() - .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().fg(Color::Cyan)) + Block::bordered() + .border_type(BorderType::QuadrantOutside) + // rely on default theme border colorAlso applies to: 482-489
.codex/agents/code-reviewer.md (1)
1-7: Good minimal example; consider pinning model/tools for clarity.Optional: add
modeland a minimaltoolsallowlist so behavior is fully deterministic for readers.--- name: code-reviewer description: Reviews staged diffs for safety regressions +model: gpt-4.1-mini +tools: + - git_diff --- Please inspect the staged changes and summarize any risks.specs/004-subagents-md/research.md (1)
1-12: Good Phase‑0 spec; add “Last updated” meta for traceability.Recommend adding a datestamp near the title to make decision staleness obvious during later phases.
Apply this diff:
# Phase 0 Research: Codex Subagents Parity + +Last updated: 2025-09-23docs/getting-started.md (1)
62-75: Quickstart is clear; include a minimal frontmatter example.A short, copy‑pasteable agent definition reduces setup friction and prevents YAML shape drift.
Apply this diff to insert a minimal example after the frontmatter mention:
2. Create `.codex/agents/<name>.md` inside your project with YAML frontmatter (`name`, `description`, optional `tools` and `model`). Project definitions override user-scoped ones. + Example: + ```md + --- + name: code-reviewer + description: "Reviews staged diffs for risks" + # Optional overrides (else defaults from config are used) + # model: gpt-5-codex-medium + # tools: ["local_shell"] + --- + Provide a short purpose and any usage notes here. + ```specs/004-subagents-md/tasks.md (1)
6-33: Add languages to fenced code blocks for markdownlint and readability.Label code fences to satisfy MD040 and enable proper rendering.
Apply:
-## Execution Flow (main) -``` +## Execution Flow (main) +```text ... -``` +```-## Parallel Example -``` +## Parallel Example +```shell # Launch contract and integration tests together once scaffolding exists: /tasks run --feature 004-subagents-md --id T003 ... -``` +```Also applies to: 111-120
codex-rs/core/src/subagents/config.rs (2)
24-39: API is fine; consider renamingenabled(...)to avoid shadowingenabledfield.The constructor name can be mistaken for the boolean.
enabled_with(...)is clearer.Apply this diff and update call sites:
- pub fn enabled(discovery: SubagentDiscoveryMode) -> Self { + pub fn enabled_with(discovery: SubagentDiscoveryMode) -> Self { Self::new(true, None, discovery) }
1-5: Optional: implementDefaultfor the enum to encodeAutoas the type default.This makes defaults more idiomatic and self‑documenting.
Apply:
-#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum SubagentDiscoveryMode { Auto, Manual, } + +impl Default for SubagentDiscoveryMode { + fn default() -> Self { + SubagentDiscoveryMode::Auto + } +}And simplify
SubagentConfig::default():- discovery: SubagentDiscoveryMode::Auto, + discovery: SubagentDiscoveryMode::default(),Also applies to: 14-22
codex-rs/tui/src/lib.rs (1)
80-80: Re‑exportingsubagent_quickstart_stepswidens the public TUI API; confirm intent.If this is primarily for internal UI/tests, prefer
pub(crate)or keep it underchatwidgetto avoid ossifying a string‑list helper as public API.codex-rs/core/tests/suite/mod.rs (1)
19-20: Subagents test modules wired into suite — add missing pretty_assertions import
- Both files exist: codex-rs/core/tests/suite/subagents_primary_story.rs and codex-rs/core/tests/suite/subagents_tool_restrictions.rs.
- pretty_assertions::assert_eq is present in subagents_primary_story.rs (line 10); add the same import to subagents_tool_restrictions.rs for consistency.
codex-cli/tests/agents_cli.contract.test.ts (1)
24-28: Mirror the run-path error matching here too (optional).To keep symmetry with run(), consider allowing the “feature disabled” variant here as well, if the show path can also be gated by the feature flag.
- await expect(showAgent("nonexistent-subagent", { format: "json" })).rejects.toThrow( - /No subagent named/i, - ); + await expect(showAgent("nonexistent-subagent", { format: "json" })).rejects.toThrow( + /(subagents feature disabled|No subagent named)/i, + );codex-rs/core/tests/suite/subagents_tool_restrictions.rs (1)
33-43: Prefer the constructor/builder over struct literals to reduce coupling.Constructing InvocationSession via struct literal ties tests to internal fields and breaks on shape changes. Use the constructor and set fields via helpers or mutable updates.
- let session = InvocationSession { - parent_session_id: Some("root".into()), - subagent_name: "shell-guard".into(), - requested_tools: vec!["filesystem".into()], - execution_log: Vec::new(), - summary: None, - detail_artifacts: Vec::new(), - confirmed: true, - resolved_model: None, - extra_instructions: None, - }; + let mut session = InvocationSession::new("shell-guard"); + session.parent_session_id = Some("root".into()); + session.requested_tools = vec!["filesystem".into()]; + session.confirmed = true;docs/subagents-tool.md (2)
8-13: Clarify feature-disabled behavior in tool registration.You note the tool is “available in both cases”; add a sentence that actual invocations return FeatureDisabled when the feature is off.
16-22: Include a minimal success/error payload example (optional).A short JSON example for success and for
{ success: false, error: "..." }will make contracts actionable.specs/004-subagents-md/quickstart.md (3)
5-6: Avoid blindly appending to config.Using
>>can duplicate the stanza on repeated runs. Suggest idempotent guidance.-echo "[subagents]\nenabled = true" >> ~/.codex/config.toml +mkdir -p ~/.codex +grep -q '^\[subagents\]' ~/.codex/config.toml 2>/dev/null || { + printf "[subagents]\nenabled = true\n" >> ~/.codex/config.toml +}
12-15: Verify tool identifiers shown in the example.Ensure
testsis a valid tool name in this repo; otherwise, use onlygit_diffor a known tool to avoid confusion.
24-27: Make explicit that natural language triggers the tool.Add a note that this prompt causes the model to call
invoke_subagentrather than relying on keywords.codex-cli/src/commands/agents/run.ts (2)
14-21: Don’t pass presentation-only options into the IPC layer.Strip
formatbefore delegating for tighter typing and future-proofing.-export async function runAgent( +export async function runAgent( name: string, options: RunAgentOptions = {}, ): Promise<RunAgentResult> { - const payload = await invokeSubagent(name, options); + const { format, ...runOpts } = options; + const payload = await invokeSubagent(name, runOpts); - if (options.format === "text") { + if (format === "text") { renderRunResult(payload); } return payload; }
23-37: Optional: include counts/fallbacks in text rendering.Show a “(none)” fallback for empty lists and indicate number of detail artifacts. Improves UX without touching JSON mode.
if (result.tools.length > 0) { console.log(`Tools: ${result.tools.join(", ")}`); + } else { + console.log("Tools: (none)"); } if (result.summary) { console.log(`Summary: ${result.summary}`); } if (result.detailArtifacts.length > 0) { - console.log(`Detail: ${result.detailArtifacts[0]}`); + console.log(`Detail[1/${result.detailArtifacts.length}]: ${result.detailArtifacts[0]}`); + } else { + console.log("Detail: (none)"); }specs/004-subagents-md/contracts/auto-suggest.md (2)
10-22: Specify value ranges and caps.Document
confidenceas [0,1] and optionally cap suggestions (e.g., ≤3) to guide implementers.
24-28: Call out feature flag interaction explicitly.Note that in manual mode and/or when disabled, no invocation occurs regardless of confirmation.
codex-rs/core/tests/suite/subagents_primary_story.rs (4)
41-41: Rename test to reflect prepare semantics (summary asserted None).You assert summary is None after invoke; if invoke returns a prepared session (pre-exec), the name should say “prepared”. Otherwise, assert is_some. Rename for clarity.
-#[test] -fn subagents_primary_story_project_override_and_summary() { +#[test] +fn subagents_primary_story_project_override_prepared_session() {
77-80: Fix assertion message to match the expectation (None).Message currently implies summary should be captured, but the assertion expects None.
- assert!( - prepared.session.summary.is_none(), - "summary should be captured during execution" - ); + assert!( + prepared.session.summary.is_none(), + "summary should not be set by the prepare step" + );
21-22: Avoid hard‑coded user-specific absolute paths.Replace personal paths with neutral placeholders to avoid PII and improve portability.
- source_path: PathBuf::from("/home/iatzmon/workspace/codex/.codex/agents/code-reviewer.md"), + source_path: PathBuf::from("/workspace/.codex/agents/code-reviewer.md"), @@ - source_path: PathBuf::from("/home/user/.codex/agents/code-reviewer.md"), + source_path: PathBuf::from("/home/username/.codex/agents/code-reviewer.md"),Also applies to: 35-36
56-66: Prefer InvocationSession::new to reduce brittleness.Construct via new() and set only the differing fields to stay resilient to struct changes.
- let session = InvocationSession { - parent_session_id: Some("root".into()), - subagent_name: "code-reviewer".into(), - requested_tools: vec!["git_diff".into(), "tests".into()], - execution_log: Vec::new(), - summary: None, - detail_artifacts: Vec::new(), - confirmed: true, - resolved_model: None, - extra_instructions: None, - }; + let mut session = InvocationSession::new("code-reviewer"); + session.parent_session_id = Some("root".into()); + session.requested_tools = vec!["git_diff".into(), "tests".into()]; + session.confirmed = true;Would you confirm SubagentInvocation::invoke returns a prepared (not executed) session so the None assertion is correct?
specs/004-subagents-md/contracts/agents-list.md (2)
13-37: Consider including “invalid” records in the response schema.Inventory exposes invalid records; CLI tests often surface them behind a flag. Reflect that in the contract for completeness.
{ "subagents": [ /* ... */ ], "conflicts": [ /* ... */ ] + , + "invalid": [ + { + "name": "broken-agent", + "scope": "user", + "validation_errors": ["missing description"], + "source_path": "/abs/path/to/.codex/agents/broken-agent.md" + } + ] }If CLI already supports --invalid, ensure the contract mirrors its JSON payload shape.
39-46: Document the --format option for symmetry with CLI.Add “--format [json|text]” to options if supported, aligning with list/show commands.
- - `--scope [project|user|all]` (default `all`) + - `--scope [project|user|all]` (default `all`) + - `--format [json|text]` (default `json`) - `--invalid` (show definitions with validation errors)specs/004-subagents-md/contracts/subagent-invoke.md (2)
30-38: Specify exit codes for failure modes.Explicit exit codes improve testability and CLI parity.
-## Failure Modes -- Unknown subagent → message: "No subagent named 'code-reviewer'. Run `codex agents list`." -- Validation error → message: "Subagent 'code-reviewer' invalid: missing description field." -- Restricted tool denial → message: "Tool 'filesystem' not allowed for subagent 'code-reviewer'." +## Failure Modes +- Unknown subagent → exit code 1, message: "No subagent named 'code-reviewer'. Run `codex agents list`." +- Validation error → exit code 2, message: "Subagent 'code-reviewer' invalid: missing description field." +- Restricted tool denial → exit code 3, message: "Tool 'filesystem' not allowed for subagent 'code-reviewer'."
12-18: Clarify effective_model resolution order.Document the precedence explicitly (definition > session model > config.default_model).
-3. Apply `effective_model` and tool allowlist. +3. Resolve `effective_model` with precedence: + 1) subagent.definition.model + 2) parent_session.model + 3) config.default_model + Then apply the tool allowlist.specs/004-subagents-md/spec.md (2)
9-25: Add a language to fenced code blocks (MD040).Mark narrative blocks as text to satisfy markdownlint.
-``` +```text 1. Parse user description from Input ... 8. Return: SUCCESS (spec ready for planning)--- `57-67`: **Replace personal absolute paths with neutral placeholders.** Avoid embedding local machine paths; use <repo_root> to keep docs portable and privacy-safe. ```diff -/home/iatzmon/workspace/codex/specs/004-subagents-md/ +<repo_root>/specs/004-subagents-md/ @@ -/home/iatzmon/workspace/codex/ +<repo_root>/ @@ -Quickstart instructions documented at `/home/iatzmon/workspace/codex/specs/004-subagents-md/quickstart.md` for enabling, defining, and invoking subagents. -Agent context update triggered via `/home/iatzmon/workspace/codex/.specify/scripts/bash/update-agent-context.sh codex` (will re-run after final review to incorporate updated plan content). +Quickstart instructions documented at `<repo_root>/specs/004-subagents-md/quickstart.md` for enabling, defining, and invoking subagents. +Agent context update triggered via `<repo_root>/.specify/scripts/bash/update-agent-context.sh codex` (will re-run after final review to incorporate updated plan content).Also applies to: 71-78, 90-91
codex-rs/core/tests/contracts/subagent_invoke.rs (2)
22-24: Avoid user-specific absolute paths in tests.Swap hard-coded home directory for a neutral workspace path.
- source_path: PathBuf::from(format!( - "/home/iatzmon/workspace/codex/.codex/agents/{name}.md" - )), + source_path: PathBuf::from(format!( + "/workspace/.codex/agents/{name}.md" + )),
75-85: Prefer InvocationSession::new for forward compatibility.Construct via new() and set fields you need.
- let session = InvocationSession { - parent_session_id: Some("root".into()), - subagent_name: "code-reviewer".into(), - requested_tools: vec!["git_diff".into()], - execution_log: Vec::new(), - summary: None, - detail_artifacts: Vec::new(), - confirmed: true, - resolved_model: None, - extra_instructions: None, - }; + let mut session = InvocationSession::new("code-reviewer"); + session.parent_session_id = Some("root".into()); + session.requested_tools = vec!["git_diff".into()]; + session.confirmed = true; @@ - let session = InvocationSession { - parent_session_id: Some("root".into()), - subagent_name: "code-reviewer".into(), - requested_tools: vec!["git_diff".into()], - execution_log: Vec::new(), - summary: None, - detail_artifacts: Vec::new(), - confirmed: false, - resolved_model: None, - extra_instructions: None, - }; + let mut session = InvocationSession::new("code-reviewer"); + session.parent_session_id = Some("root".into()); + session.requested_tools = vec!["git_diff".into()]; + session.confirmed = false;Also applies to: 103-113
codex-rs/core/tests/contracts/agents_list.rs (2)
35-36: Remove author-identifying absolute paths.Use neutral paths to reduce brittleness and avoid PII leakage in tests.
- "/home/iatzmon/workspace/codex/.codex/agents/code-reviewer.md", + "/workspace/.codex/agents/code-reviewer.md", @@ - "/home/iatzmon/workspace/codex/.codex/agents/code-reviewer.md", + "/workspace/.codex/agents/code-reviewer.md",Also applies to: 75-76
58-66: Check conflict reason string stability.If reason strings are user-facing, consider using an enum serialized to a stable value to avoid brittle tests on exact wording.
codex-cli/src/commands/agents/show.ts (1)
16-21: LGTM; consider output injection for testability.Functionality is sound. Optionally allow passing a writer (defaults to console) to make text output test-friendly.
-export async function showAgent( +export async function showAgent( name: string, options: ShowAgentOptions = {}, ): Promise<ShowAgentResult> { const record = await fetchSubagentRecord(name); if (options.format === "text") { - renderAgent(record); + renderAgent(record); } return record; } -function renderAgent(record: SubagentRecord): void { - console.log(`Name: ${record.name}`); - console.log(`Scope: ${record.scope}`); - console.log(`Description: ${record.description}`); - console.log(`Tools: ${record.tools.join(", ") || "(all)"}`); - console.log(`Model: ${record.model ?? "(inherit)"}`); - console.log(`Status: ${record.status}`); - console.log(`Source: ${record.sourcePath}`); - if (record.validationErrors.length > 0) { - console.log(`Validation errors: ${record.validationErrors.join("; ")}`); - } +function renderAgent( + record: SubagentRecord, + out: Pick<typeof console, "log"> = console, +): void { + out.log(`Name: ${record.name}`); + out.log(`Scope: ${record.scope}`); + out.log(`Description: ${record.description}`); + out.log(`Tools: ${record.tools.length ? record.tools.join(", ") : "(all)"}`); + out.log(`Model: ${record.model ?? "(inherit)"}`); + out.log(`Status: ${record.status}`); + out.log(`Source: ${record.sourcePath}`); + if (record.validationErrors.length > 0) { + out.log(`Validation errors: ${record.validationErrors.join("; ")}`); + } }Also applies to: 23-34
specs/004-subagents-md/plan.md (2)
8-28: Add language to fenced blocks (MD040).Mark as text for lint compliance.
-``` +```text 1. Load feature spec from Input path ... 9. STOP - Ready for /tasks command ...--- `3-5`: **Replace personal absolute paths with placeholders.** Use <repo_root> to avoid machine-specific paths in docs. ```diff -**Branch**: `[004-subagents-md]` | **Date**: September 21, 2025 | **Spec**: `/home/iatzmon/workspace/codex/specs/004-subagents-md/spec.md` -**Input**: Feature specification from `/home/iatzmon/workspace/codex/specs/004-subagents-md/spec.md` +**Branch**: `[004-subagents-md]` | **Date**: September 21, 2025 | **Spec**: `<repo_root>/specs/004-subagents-md/spec.md` +**Input**: Feature specification from `<repo_root>/specs/004-subagents-md/spec.md` @@ - → Completed: decisions logged in `/home/iatzmon/workspace/codex/specs/004-subagents-md/research.md`. + → Completed: decisions logged in `<repo_root>/specs/004-subagents-md/research.md`. @@ -/home/iatzmon/workspace/codex/specs/004-subagents-md/ +<repo_root>/specs/004-subagents-md/ @@ -/home/iatzmon/workspace/codex/ +<repo_root>/ @@ -- Quickstart instructions documented at `/home/iatzmon/workspace/codex/specs/004-subagents-md/quickstart.md` for enabling, defining, and invoking subagents. -- Agent context update triggered via `/home/iatzmon/workspace/codex/.specify/scripts/bash/update-agent-context.sh codex` (will re-run after final review to incorporate updated plan content). +- Quickstart instructions documented at `<repo_root>/specs/004-subagents-md/quickstart.md` for enabling, defining, and invoking subagents. +- Agent context update triggered via `<repo_root>/.specify/scripts/bash/update-agent-context.sh codex` (will re-run after final review to incorporate updated plan content). @@ -*Based on Constitution v3.0.0 - See `/home/iatzmon/workspace/codex/.specify/memory/constitution.md`* +*Based on Constitution v3.0.0 - See `<repo_root>/.specify/memory/constitution.md`*Also applies to: 18-21, 57-67, 71-78, 91-92, 121-123
subagents.md (3)
1-1: Remove third‑party hotlinked logo.Hotlinking an external image (Perplexity CDN) in repo docs is brittle and can break offline docs; it also raises branding/licensing concerns.
Apply this diff to drop the tag:
-<img src="https://r2cdn.perplexity.ai/pplx-full-logo-primary-dark%402x.png" style="height:64px;margin-right:32px"/> +
139-141: Drop hidden footnote block and inline HTML styling.The hidden
<span style="display:none">[...]</span>injects many low‑signal links and mixes HTML into Markdown. Prefer plain Markdown references and only sources you actually rely on.Apply this diff to remove the hidden block:
-<span style="display:none">[^10][^11][^12][^13][^14][^15][^16][^17][^18][^19][^20][^21][^6][^7][^8][^9]</span> +
95-101: Clarify isolation defaults and expandability.State explicitly that full transcripts are persisted but not inlined by default, and how to enable expansion in CLI/TUI. Acceptance criteria should note where transcripts are written and retention policy.
Also applies to: 127-133
codex-rs/core/src/subagents/invocation.rs (1)
31-34: Builder ergonomics: keep, but considerwith_requested_tools.The
confirmed()helper is handy. Consider adding awith_requested_tools<I, S>(I)builder for concise construction in tests/callers.codex-rs/core/src/subagents/service.rs (1)
35-38: Path deduplication could canonicalize to avoid near‑duplicates.Sorting/dedup alone won’t collapse
~/..vs absolute, or case differences on case‑insensitive FS. Consider optionalcanonicalizewith best‑effort ignore of errors.codex-rs/core/src/conversation_manager.rs (3)
179-188: Inventory builder: consider reuse or caching to avoid repeated I/O.Building the inventory on every call can be expensive for large trees. Provide an overload that accepts a precomputed
SubagentInventory, or introduce a small LRU keyed by(cwd, codex_home, discovery_mode).If helpful, I can sketch a small cache wrapper.
255-281: Discovery logging could include source path and scope.Add
source_path/scopefields to invalid record logs for faster triage.
283-297: Be careful logging summaries and artifact paths at info level.Summaries/artifact locations can contain sensitive content or user paths. Consider truncating summaries and logging at debug; keep info to subagent name and status.
codex-rs/tui/src/chatwidget/tests.rs (1)
246-253: Use deterministic test AuthManager across helpers.Elsewhere you use
with_auth(...). For consistency and to avoid FS dependencies, preferwith_authhere too instead ofAuthManager::shared(cfg.codex_home.clone()).Apply this diff if desired:
- _conversation_manager: Arc::new(ConversationManager::new(AuthManager::shared( - cfg.codex_home.clone(), - ))), + _conversation_manager: Arc::new(ConversationManager::with_auth( + CodexAuth::from_api_key("test"), + )),codex-rs/core/tests/subagents_invocation.rs (1)
9-21: Prefer constructor to manual struct literal for definitions.If
SubagentDefinition::new(...)exists, use it to ensure consistent normalization/validation defaults and reduce test fragility.specs/004-subagents-md/data-model.md (2)
16-20: Tighten validation semantics for tools.Discovery can’t reliably validate MCP tool presence; move “map to registered tools” to invocation time and document discovery‑time validation as syntax/shape only.
Apply this diff:
- - Ensure `tools` values map to registered tools when feature enabled. + - Ensure `tools` values are syntactically valid; enforce registered tool availability at invocation time.
47-56: Documentrequested_toolssemantics.Clarify whether an empty
requested_toolsmeans “inherit all” or “no tools”, and who sets it (caller vs runner).codex-cli/src/commands/agents/list.ts (1)
23-43: Console output UX: stable headers and empty states handled well.Minor: for very wide tool lists, consider truncating in text mode.
codex-rs/core/src/subagents/parser.rs (2)
60-73: Dedup tools with a set for clarity.Current O(n²) scan is fine for tiny lists; a HashSet reads cleaner.
- if let Some(tool_entries) = raw.tools { - let mut tools = Vec::new(); - for tool in tool_entries { - let trimmed = tool.trim(); - if trimmed.is_empty() { - definition.add_validation_error("`tools` entries must be non-empty strings"); - continue; - } - if !tools.iter().any(|existing: &String| existing == trimmed) { - tools.push(trimmed.to_string()); - } - } - definition = definition.with_tools(tools); - } + if let Some(tool_entries) = raw.tools { + use std::collections::BTreeSet; + let mut tools = BTreeSet::new(); + for tool in tool_entries { + let trimmed = tool.trim(); + if trimmed.is_empty() { + definition.add_validation_error("`tools` entries must be non-empty strings"); + continue; + } + tools.insert(trimmed.to_string()); + } + definition = definition.with_tools(tools.into_iter().collect()); + }
120-141: Frontmatter terminator parsing is strict.Using "\n---" requires the closing marker to be on its own line; that’s correct for common MD, but won’t match files ending exactly with '---' without a trailing newline. Consider supporting EOF‑terminated frontmatter as well.
codex-rs/core/src/subagents/executor.rs (1)
228-246: Transcript persistence path selection.Storing next to source_path is convenient but can fail for read‑only workspaces. Consider defaulting to CODEX_HOME (e.g., $CODEX_HOME/agents//sessions) and falling back to source adjacency.
codex-cli/src/ipc/subagents.ts (2)
134-170: Binary resolution fallback may point to a non-existent path.Consider checking the fallback exists and, if not, error early with guidance or fall back to PATH lookup.
246-255: normalizeScope default.Returning "all" for unknown scope values can mislead consumers; prefer a stricter default (e.g., treat unknown as "project" or throw).
codex-rs/tui/src/chatwidget.rs (2)
137-137: Rename_conversation_manageror add a clarifying comment.Leading underscore implies “unused”, yet this field pins the Arc to keep the server alive. Prefer
conversation_managerand add a brief doc comment.- _conversation_manager: Arc<ConversationManager>, + /// Hold a strong reference to keep the conversation server alive. + conversation_manager: Arc<ConversationManager>,And update initializers accordingly.
1896-1905: Avoid per-call allocations for static quickstart text.Use a static slice and map when needed to
String.+const SUBAGENT_QUICKSTART: [&str; 5] = [ + "Enable subagents via ~/.codex/config.toml", + "Create or place Markdown definitions under .codex/agents/", + "Inspect available subagents with `codex agents list`", + "Run a subagent with `codex agents run <name>`", + "View invocation details with `codex agents show <name>`", +]; - -pub fn subagent_quickstart_steps() -> Vec<String> { - vec![ - "Enable subagents via ~/.codex/config.toml".to_string(), - "Create or place Markdown definitions under .codex/agents/".to_string(), - "Inspect available subagents with `codex agents list`".to_string(), - "Run a subagent with `codex agents run <name>`".to_string(), - "View invocation details with `codex agents show <name>`".to_string(), - ] -} +pub fn subagent_quickstart_steps() -> Vec<String> { + SUBAGENT_QUICKSTART.iter().map(|s| s.to_string()).collect() +}codex-rs/cli/src/agents.rs (2)
118-121: Surface a friendly hint when subagents are disabled.Logging with
info!is easy to miss. Consider printing a one-line hint to stdout/stderr in plaintext mode (keep JSON output unchanged).- if !config.subagents.is_enabled() { - info!("subagents feature disabled via configuration"); - } + if !config.subagents.is_enabled() { + info!("subagents feature disabled via configuration"); + if !cmd.json { + eprintln!("Hint: enable subagents in ~/.codex/config.toml (subagents.enabled = true)"); + } + }
254-298: Optionally include conflicts in plaintextlistoutput.Exposing
inventory.conflicts(e.g., “project override” vs “duplicate definition”) helps users understand why a definition lost.If desired, append after the “Available subagents” section:
if !inventory.conflicts.is_empty() { println!("\nConflicts:"); for c in &inventory.conflicts { println!(" - {} ← {} ({})", c.name, display_scope(c.losing_scope), c.reason); } }codex-rs/core/src/subagents/definition.rs (1)
88-115: Consider Unicode-aware normalization (optional).
is_ascii_alphanumeric()rejects non‑ASCII names, yielding empty normalized names for many locales. If i18n is desired, switch toch.is_alphanumeric()and lowercasing viato_lowercase()while keeping separator coalescing.codex-rs/core/src/subagents/discovery.rs (3)
54-58: Don’t drop WalkDir errors; surface them as DiscoveryEvents.
.filter_map(Result::ok)hides IO/permission errors and makes troubleshooting hard. Emit aDiscoveryEventfor iterator errors so users see why files were skipped.- for entry in WalkDir::new(&root) - .follow_links(false) - .into_iter() - .filter_map(Result::ok) - { + let walker = WalkDir::new(&root).follow_links(false).into_iter(); + for entry in walker { + let entry = match entry { + Ok(e) => e, + Err(err) => { + outcome.events.push(DiscoveryEvent { + message: format!( + "Failed to traverse directory under {}: {}", + root.display(), + err + ), + }); + continue; + } + };
103-112: Scope detection should honor CODEX_HOME, not just $HOME.
detect_scope()assumes~/.codex/agents. WhenCODEX_HOMEis set (Config.codex_home), this will misclassify user-scoped agents. Consider passingcodex_home: &Pathor consultingConfigso user scope matchesservice::source_tree_from_config.-pub fn detect_scope(path: &PathBuf) -> SubagentScope { +pub fn detect_scope(path: &Path, codex_home: &Path) -> SubagentScope { - if let Some(home) = dirs::home_dir() { - let user_root = home.join(".codex/agents"); + { + let user_root = codex_home.join("agents"); if path.starts_with(&user_root) { return SubagentScope::User; } } SubagentScope::Project }
114-122: Nit: prefer &Path over &PathBuf.Idiomatic signatures take
&Pathfor borrowed paths.codex-rs/core/src/subagents/runner.rs (1)
103-109: Remove no‑ops.These branches do nothing and add noise.
- if session.summary.is_none() { - session.summary = None; - } - - if session.detail_artifacts.is_empty() { - session.detail_artifacts.clear(); - } + // no default summary or artifacts here; executor will populate as neededcodex-rs/core/src/codex.rs (2)
388-404: Avoid rebuilding inventory on every per‑turn override.
compute_subagent_tooling()walks the filesystem each time it’s called (e.g., per‑turn model overrides). Cache the inventory per session and only rebuild whencwd/codex_homechanges.
3718-3755: Surface MissingAuthManager via the existing error type for consistency.You special‑case auth manager absence to a plain string. Prefer returning
SubagentInvocationError::MissingAuthManagerso callers and tests can match on it.- None => { - return ResponseInputItem::FunctionCallOutput { - call_id, - output: FunctionCallOutputPayload { - content: "Subagent execution requires authentication context." - .to_string(), - success: Some(false), - }, - }; - } + None => { + let message = SubagentInvocationError::MissingAuthManager.to_string(); + return ResponseInputItem::FunctionCallOutput { + call_id, + output: FunctionCallOutputPayload { + content: message, + success: Some(false), + }, + }; + }codex-rs/core/src/openai_tools.rs (7)
12-17: Compact the subagents imports per repo style.Follow the “prefer compact imports” guideline for Rust files.
Apply:
-use crate::subagents::SubagentConfig; -use crate::subagents::SubagentDiscoveryMode; -use crate::subagents::SubagentInventory; -use crate::subagents::SubagentRecord; -use crate::subagents::SubagentScope; -use crate::subagents::SubagentStatus; +use crate::subagents::{ + SubagentConfig, SubagentDiscoveryMode, SubagentInventory, SubagentRecord, SubagentScope, + SubagentStatus, +};
47-56: Public struct with private fields—confirm intended API surface.If these types aren’t needed outside this crate, prefer
pub(crate)to avoid leaking surface. If they must be public, current private fields are fine to preserve invariants.Would you like me to generate an API grep to verify any external (out-of-crate) usage before narrowing visibility?
71-103: Avoid join-allocation in summary_line.Minor: building a Vec and joining allocates more than needed. Write directly into a String.
- let mut parts = vec![format!( - "{} ({}) — {}", - self.name, - describe_scope(self.scope), - description - )]; + let mut parts = String::with_capacity(128); + parts.push_str(&format!("{} ({}) — {}", self.name, describe_scope(self.scope), description)); - if !self.tools.is_empty() { - parts.push(format!("tools: {}", self.tools.join(", "))); - } + if !self.tools.is_empty() { + parts.push_str(&format!(" | tools: {}", self.tools.join(", "))); + } if let Some(model) = &self.model { - parts.push(format!("model: {model}")); + parts.push_str(&format!(" | model: {model}")); } if !matches!(self.status, SubagentStatus::Active) { - parts.push(format!("status: {}", describe_status(&self.status))); + parts.push_str(&format!(" | status: {}", describe_status(&self.status))); } if !self.validation_errors.is_empty() { - parts.push(format!("errors: {}", self.validation_errors.join("; "))); + parts.push_str(&format!(" | errors: {}", self.validation_errors.join("; "))); } - parts.join(" | ") + parts
154-200: Tool description reads well. Minor polish optional.Consider adding a short note that
requested_tools/modelare optional overrides, to align wording with the schema below.
201-219: Stabilize example list.With sorted entries, the examples become deterministic. If sorting isn’t adopted above, sort locally before sampling.
- let sample: Vec<String> = self - .entries + let mut names: Vec<String> = self + .entries .iter() - .map(|entry| entry.name.clone()) - .take(6) - .collect(); - let mut desc = format!("Normalized subagent name (examples: {}", sample.join(", ")); - if self.entries.len() > sample.len() { + .map(|e| e.name.clone()) + .collect(); + names.sort(); + let sample: Vec<String> = names.into_iter().take(6).collect(); + let mut desc = format!("Normalized subagent name (examples: {}", sample.join(", ")); + if self.entries.len() > sample.len() { desc.push_str(", …"); }
246-259: Fix O(n^2) char counting in truncate; handle edge cases.Current loop recomputes
out.chars().count()each iteration. Track indices usingchar_indices()and only slice once.-fn truncate(input: &str, max: usize) -> String { - if input.chars().count() <= max { - return input.to_string(); - } - let mut out = String::new(); - for ch in input.chars() { - if out.chars().count() + 1 > max.saturating_sub(1) { - break; - } - out.push(ch); - } - out.push('…'); - out -} +fn truncate(input: &str, max: usize) -> String { + let char_count = input.chars().count(); + if char_count <= max { + return input.to_string(); + } + // keep at most (max - 1) chars and append an ellipsis + let keep = max.saturating_sub(1); + let mut end = 0usize; + for (i, (byte_idx, ch)) in input.char_indices().enumerate() { + if i >= keep { + break; + } + end = byte_idx + ch.len_utf8(); + } + let mut out = input[..end].to_string(); + out.push('…'); + out +}
547-592: invoke_subagent tool schema looks good.Required/optional fields align with the description; JSON‑Schema matches the rest of the tools.
If you want stricter validation via Responses API, consider
strict: truehere (you already providerequiredandadditional_properties: false).
| { | ||
| "subagent": "code-reviewer", | ||
| "timestamp": "20250923T003110Z", | ||
| "model": "gpt-5-codex", | ||
| "requested_tools": [], | ||
| "instructions": "Please inspect the staged changes and summarize any risks.", | ||
| "events": [ | ||
| { | ||
| "id": "", | ||
| "msg": { | ||
| "type": "session_configured", | ||
| "session_id": "e26e27d8-e2d3-4fcb-a72d-a0abdab12ea6", | ||
| "model": "gpt-5-codex", | ||
| "reasoning_effort": "high", | ||
| "history_log_id": 36652734, | ||
| "history_entry_count": 595, | ||
| "rollout_path": "/home/iatzmon/.codex/sessions/2025/09/23/rollout-2025-09-23T00-30-56-e26e27d8-e2d3-4fcb-a72d-a0abdab12ea6.jsonl" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "task_started", | ||
| "model_context_window": 272000 | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_section_break" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**Inspect" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "ing" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " git" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " changes" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning", | ||
| "text": "**Inspecting staged git changes**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_begin", | ||
| "call_id": "call_C5ukCCeOw46RRo7eHqREchPT", | ||
| "command": [ | ||
| "bash", | ||
| "-lc", | ||
| "git diff --staged" | ||
| ], | ||
| "cwd": "/home/iatzmon/workspace/codex/.", | ||
| "parsed_cmd": [ | ||
| { | ||
| "type": "unknown", | ||
| "cmd": "git diff --staged" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_end", | ||
| "call_id": "call_C5ukCCeOw46RRo7eHqREchPT", | ||
| "stdout": "", | ||
| "stderr": "", | ||
| "aggregated_output": "", | ||
| "exit_code": 0, | ||
| "duration": { | ||
| "secs": 0, | ||
| "nanos": 333723727 | ||
| }, | ||
| "formatted_output": "" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "token_count", | ||
| "info": { | ||
| "total_token_usage": { | ||
| "input_tokens": 3768, | ||
| "cached_input_tokens": 0, | ||
| "output_tokens": 168, | ||
| "reasoning_output_tokens": 128, | ||
| "total_tokens": 3936 | ||
| }, | ||
| "last_token_usage": { | ||
| "input_tokens": 3768, | ||
| "cached_input_tokens": 0, | ||
| "output_tokens": 168, | ||
| "reasoning_output_tokens": 128, | ||
| "total_tokens": 3936 | ||
| }, | ||
| "model_context_window": 272000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_section_break" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**Checking" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " git" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " staging" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " status" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning", | ||
| "text": "**Checking git staging status**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_begin", | ||
| "call_id": "call_RWYAGzcKi0lnpJJvKdnbjpUM", | ||
| "command": [ | ||
| "bash", | ||
| "-lc", | ||
| "git status --short" | ||
| ], | ||
| "cwd": "/home/iatzmon/workspace/codex/.", | ||
| "parsed_cmd": [ | ||
| { | ||
| "type": "unknown", | ||
| "cmd": "git status --short" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_output_delta", | ||
| "call_id": "call_RWYAGzcKi0lnpJJvKdnbjpUM", | ||
| "stream": "stdout", | ||
| "chunk": "IE0gQUdFTlRTLm1kCiBNIGNvZGV4LWNsaS9wYWNrYWdlLmpzb24KIE0gY29kZXgtcnMvY2xpL3NyYy9hZ2VudHMucnMKIE0gY29kZXgtcnMvY29yZS9zcmMvY2xpZW50LnJzCiBNIGNvZGV4LXJzL2NvcmUvc3JjL2NvZGV4LnJzCiBNIGNvZGV4LXJzL2NvcmUvc3JjL2NvbnZlcnNhdGlvbl9tYW5hZ2VyLnJzCiBNIGNvZGV4LXJzL2NvcmUvc3JjL2xpYi5ycwogTSBjb2RleC1ycy9jb3JlL3NyYy9vcGVuYWlfdG9vbHMucnMKIE0gY29kZXgtcnMvY29yZS9zcmMvc3Bhd24ucnMKIE0gY29kZXgtcnMvY29yZS90ZXN0cy9jb250cmFjdHMucnMKIE0gY29kZXgtcnMvY29yZS90ZXN0cy9zdWl0ZS9tb2QucnMKIE0gY29kZXgtcnMvdHVpL3NyYy9hcHAucnMKIE0gY29kZXgtcnMvdHVpL3NyYy9hcHBfYmFja3RyYWNrLnJzCiBNIGNvZGV4LXJzL3R1aS9zcmMvY2hhdHdpZGdldC5ycwogTSBjb2RleC1ycy90dWkvc3JjL2NoYXR3aWRnZXQvdGVzdHMucnMKIE0gY29kZXgtcnMvdHVpL3NyYy9saWIucnMKIE0gY29kZXgtcnMvdHVpL3NyYy91c2VyX2FwcHJvdmFsX3dpZGdldC5ycwogTSBjb2RleC1ycy90dWkvdGVzdHMvc3VpdGUvbW9kLnJzCiBNIGRvY3MvZ2V0dGluZy1zdGFydGVkLm1kCiBNIHNwZWNzLzAwNC1zdWJhZ2VudHMtbWQvdGFza3MubWQKPz8gLmNvZGV4L2FnZW50cy8KPz8gY29kZXgtcnMvY29yZS9zcmMvc3ViYWdlbnRzLwo/PyBjb2RleC1ycy9jb3JlL3Rlc3RzL2NvbnRyYWN0cy9hZ2VudHNfbGlzdC5ycwo/PyBjb2RleC1ycy9jb3JlL3Rlc3RzL2NvbnRyYWN0cy9zdWJhZ2VudF9pbnZva2UucnMKPz8gY29kZXgtcnMvY29yZS90ZXN0cy9zdWJhZ2VudHNfY29uZmlnLnJzCj8/IGNvZGV4LXJzL2NvcmUvdGVzdHMvc3ViYWdlbnRzX2ludm9jYXRpb24ucnMKPz8gY29kZXgtcnMvY29yZS90ZXN0cy9zdWl0ZS9zdWJhZ2VudHNfcHJpbWFyeV9zdG9yeS5ycwo/PyBjb2RleC1ycy9jb3JlL3Rlc3RzL3N1aXRlL3N1YmFnZW50c190b29sX3Jlc3RyaWN0aW9ucy5ycwo/PyBjb2RleC1ycy90dWkvdGVzdHMvc3VpdGUvc3ViYWdlbnRzX3F1aWNrc3RhcnQucnMKPz8gZG9jcy9zdWJhZ2VudHMtdG9vbC5tZAo/PyBkb2NzL3N1YmFnZW50cy5tZAo/PyBzcGVjcy8wMDQtc3ViYWdlbnRzLW1kL2NvbnRyYWN0cy8KPz8gc3BlY3MvMDA0LXN1YmFnZW50cy1tZC9kYXRhLW1vZGVsLm1kCj8/IHNwZWNzLzAwNC1zdWJhZ2VudHMtbWQvcGxhbi5tZAo/PyBzcGVjcy8wMDQtc3ViYWdlbnRzLW1kL3F1aWNrc3RhcnQubWQKPz8gc3BlY3MvMDA0LXN1YmFnZW50cy1tZC9yZXNlYXJjaC5tZAo/PyBzcGVjcy8wMDQtc3ViYWdlbnRzLW1kL3NwZWMubWQKPz8gc3ViYWdlbnRzLm1kCg==" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_end", | ||
| "call_id": "call_RWYAGzcKi0lnpJJvKdnbjpUM", | ||
| "stdout": " M AGENTS.md\n M codex-cli/package.json\n M codex-rs/cli/src/agents.rs\n M codex-rs/core/src/client.rs\n M codex-rs/core/src/codex.rs\n M codex-rs/core/src/conversation_manager.rs\n M codex-rs/core/src/lib.rs\n M codex-rs/core/src/openai_tools.rs\n M codex-rs/core/src/spawn.rs\n M codex-rs/core/tests/contracts.rs\n M codex-rs/core/tests/suite/mod.rs\n M codex-rs/tui/src/app.rs\n M codex-rs/tui/src/app_backtrack.rs\n M codex-rs/tui/src/chatwidget.rs\n M codex-rs/tui/src/chatwidget/tests.rs\n M codex-rs/tui/src/lib.rs\n M codex-rs/tui/src/user_approval_widget.rs\n M codex-rs/tui/tests/suite/mod.rs\n M docs/getting-started.md\n M specs/004-subagents-md/tasks.md\n?? .codex/agents/\n?? codex-rs/core/src/subagents/\n?? codex-rs/core/tests/contracts/agents_list.rs\n?? codex-rs/core/tests/contracts/subagent_invoke.rs\n?? codex-rs/core/tests/subagents_config.rs\n?? codex-rs/core/tests/subagents_invocation.rs\n?? codex-rs/core/tests/suite/subagents_primary_story.rs\n?? codex-rs/core/tests/suite/subagents_tool_restrictions.rs\n?? codex-rs/tui/tests/suite/subagents_quickstart.rs\n?? docs/subagents-tool.md\n?? docs/subagents.md\n?? specs/004-subagents-md/contracts/\n?? specs/004-subagents-md/data-model.md\n?? specs/004-subagents-md/plan.md\n?? specs/004-subagents-md/quickstart.md\n?? specs/004-subagents-md/research.md\n?? specs/004-subagents-md/spec.md\n?? subagents.md\n", | ||
| "stderr": "", | ||
| "aggregated_output": " M AGENTS.md\n M codex-cli/package.json\n M codex-rs/cli/src/agents.rs\n M codex-rs/core/src/client.rs\n M codex-rs/core/src/codex.rs\n M codex-rs/core/src/conversation_manager.rs\n M codex-rs/core/src/lib.rs\n M codex-rs/core/src/openai_tools.rs\n M codex-rs/core/src/spawn.rs\n M codex-rs/core/tests/contracts.rs\n M codex-rs/core/tests/suite/mod.rs\n M codex-rs/tui/src/app.rs\n M codex-rs/tui/src/app_backtrack.rs\n M codex-rs/tui/src/chatwidget.rs\n M codex-rs/tui/src/chatwidget/tests.rs\n M codex-rs/tui/src/lib.rs\n M codex-rs/tui/src/user_approval_widget.rs\n M codex-rs/tui/tests/suite/mod.rs\n M docs/getting-started.md\n M specs/004-subagents-md/tasks.md\n?? .codex/agents/\n?? codex-rs/core/src/subagents/\n?? codex-rs/core/tests/contracts/agents_list.rs\n?? codex-rs/core/tests/contracts/subagent_invoke.rs\n?? codex-rs/core/tests/subagents_config.rs\n?? codex-rs/core/tests/subagents_invocation.rs\n?? codex-rs/core/tests/suite/subagents_primary_story.rs\n?? codex-rs/core/tests/suite/subagents_tool_restrictions.rs\n?? codex-rs/tui/tests/suite/subagents_quickstart.rs\n?? docs/subagents-tool.md\n?? docs/subagents.md\n?? specs/004-subagents-md/contracts/\n?? specs/004-subagents-md/data-model.md\n?? specs/004-subagents-md/plan.md\n?? specs/004-subagents-md/quickstart.md\n?? specs/004-subagents-md/research.md\n?? specs/004-subagents-md/spec.md\n?? subagents.md\n", | ||
| "exit_code": 0, | ||
| "duration": { | ||
| "secs": 0, | ||
| "nanos": 346167362 | ||
| }, | ||
| "formatted_output": " M AGENTS.md\n M codex-cli/package.json\n M codex-rs/cli/src/agents.rs\n M codex-rs/core/src/client.rs\n M codex-rs/core/src/codex.rs\n M codex-rs/core/src/conversation_manager.rs\n M codex-rs/core/src/lib.rs\n M codex-rs/core/src/openai_tools.rs\n M codex-rs/core/src/spawn.rs\n M codex-rs/core/tests/contracts.rs\n M codex-rs/core/tests/suite/mod.rs\n M codex-rs/tui/src/app.rs\n M codex-rs/tui/src/app_backtrack.rs\n M codex-rs/tui/src/chatwidget.rs\n M codex-rs/tui/src/chatwidget/tests.rs\n M codex-rs/tui/src/lib.rs\n M codex-rs/tui/src/user_approval_widget.rs\n M codex-rs/tui/tests/suite/mod.rs\n M docs/getting-started.md\n M specs/004-subagents-md/tasks.md\n?? .codex/agents/\n?? codex-rs/core/src/subagents/\n?? codex-rs/core/tests/contracts/agents_list.rs\n?? codex-rs/core/tests/contracts/subagent_invoke.rs\n?? codex-rs/core/tests/subagents_config.rs\n?? codex-rs/core/tests/subagents_invocation.rs\n?? codex-rs/core/tests/suite/subagents_primary_story.rs\n?? codex-rs/core/tests/suite/subagents_tool_restrictions.rs\n?? codex-rs/tui/tests/suite/subagents_quickstart.rs\n?? docs/subagents-tool.md\n?? docs/subagents.md\n?? specs/004-subagents-md/contracts/\n?? specs/004-subagents-md/data-model.md\n?? specs/004-subagents-md/plan.md\n?? specs/004-subagents-md/quickstart.md\n?? specs/004-subagents-md/research.md\n?? specs/004-subagents-md/spec.md\n?? subagents.md\n" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "token_count", | ||
| "info": { | ||
| "total_token_usage": { | ||
| "input_tokens": 7742, | ||
| "cached_input_tokens": 3840, | ||
| "output_tokens": 207, | ||
| "reasoning_output_tokens": 128, | ||
| "total_tokens": 7949 | ||
| }, | ||
| "last_token_usage": { | ||
| "input_tokens": 3974, | ||
| "cached_input_tokens": 3840, | ||
| "output_tokens": 39, | ||
| "reasoning_output_tokens": 0, | ||
| "total_tokens": 4013 | ||
| }, | ||
| "model_context_window": 272000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_section_break" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**Expl" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "aining" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " absence" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " of" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " changes" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning", | ||
| "text": "**Explaining absence of staged changes**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "No" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " changes" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " right" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " now" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " Everything" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " listed" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " under" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " `" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "git" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " status" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "`" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " is" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " still" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " unst" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "aged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "," | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " so" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " there" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "’s" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " nothing" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " for" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " me" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " to" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " review" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " for" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " risk" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " yet" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " Let" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " me" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " know" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " once" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " you" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "’ve" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " the" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " updates" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " you" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " want" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " inspected" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message", | ||
| "message": "No staged changes right now. Everything listed under `git status` is still unstaged, so there’s nothing for me to review for risk yet. Let me know once you’ve staged the updates you want inspected." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "token_count", | ||
| "info": { | ||
| "total_token_usage": { | ||
| "input_tokens": 12241, | ||
| "cached_input_tokens": 7808, | ||
| "output_tokens": 448, | ||
| "reasoning_output_tokens": 320, | ||
| "total_tokens": 12689 | ||
| }, | ||
| "last_token_usage": { | ||
| "input_tokens": 4499, | ||
| "cached_input_tokens": 3968, | ||
| "output_tokens": 241, | ||
| "reasoning_output_tokens": 192, | ||
| "total_tokens": 4740 | ||
| }, | ||
| "model_context_window": 272000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "task_complete", | ||
| "last_agent_message": "No staged changes right now. Everything listed under `git status` is still unstaged, so there’s nothing for me to review for risk yet. Let me know once you’ve staged the updates you want inspected." | ||
| } | ||
| } | ||
| ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
Remove committed session log (PII + repo noise).
This log captures local absolute paths (/home/...), command outputs, and token metrics. It shouldn’t live in the repo. Move a sanitized example under docs or tests fixtures and add the sessions path to .gitignore.
Suggested actions:
- git rm this file
- Add ignore rule:
+.codex/agents/sessions/Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .codex/agents/sessions/20250923T003110Z.json (lines 1–656): this committed
session log contains PII (absolute local paths, command outputs, token metrics)
and repo noise; remove it from history and stop tracking such files. Run git rm
--cached (or git rm) on the file, add an appropriate .gitignore entry (e.g.
ignore .codex/agents/ or sessions JSONs), and commit the removal; if you need
examples, add a sanitized fixture (with all paths/tokens scrubbed) under docs/
or tests/fixtures and commit that instead.
.codex/agents/sessions/latest.json
Outdated
| { | ||
| "subagent": "code-reviewer", | ||
| "timestamp": "20250923T003110Z", | ||
| "model": "gpt-5-codex", | ||
| "requested_tools": [], | ||
| "instructions": "Please inspect the staged changes and summarize any risks.", | ||
| "events": [ | ||
| { | ||
| "id": "", | ||
| "msg": { | ||
| "type": "session_configured", | ||
| "session_id": "e26e27d8-e2d3-4fcb-a72d-a0abdab12ea6", | ||
| "model": "gpt-5-codex", | ||
| "reasoning_effort": "high", | ||
| "history_log_id": 36652734, | ||
| "history_entry_count": 595, | ||
| "rollout_path": "/home/iatzmon/.codex/sessions/2025/09/23/rollout-2025-09-23T00-30-56-e26e27d8-e2d3-4fcb-a72d-a0abdab12ea6.jsonl" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "task_started", | ||
| "model_context_window": 272000 | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_section_break" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**Inspect" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "ing" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " git" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " changes" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning", | ||
| "text": "**Inspecting staged git changes**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_begin", | ||
| "call_id": "call_C5ukCCeOw46RRo7eHqREchPT", | ||
| "command": [ | ||
| "bash", | ||
| "-lc", | ||
| "git diff --staged" | ||
| ], | ||
| "cwd": "/home/iatzmon/workspace/codex/.", | ||
| "parsed_cmd": [ | ||
| { | ||
| "type": "unknown", | ||
| "cmd": "git diff --staged" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_end", | ||
| "call_id": "call_C5ukCCeOw46RRo7eHqREchPT", | ||
| "stdout": "", | ||
| "stderr": "", | ||
| "aggregated_output": "", | ||
| "exit_code": 0, | ||
| "duration": { | ||
| "secs": 0, | ||
| "nanos": 333723727 | ||
| }, | ||
| "formatted_output": "" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "token_count", | ||
| "info": { | ||
| "total_token_usage": { | ||
| "input_tokens": 3768, | ||
| "cached_input_tokens": 0, | ||
| "output_tokens": 168, | ||
| "reasoning_output_tokens": 128, | ||
| "total_tokens": 3936 | ||
| }, | ||
| "last_token_usage": { | ||
| "input_tokens": 3768, | ||
| "cached_input_tokens": 0, | ||
| "output_tokens": 168, | ||
| "reasoning_output_tokens": 128, | ||
| "total_tokens": 3936 | ||
| }, | ||
| "model_context_window": 272000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_section_break" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**Checking" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " git" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " staging" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " status" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning", | ||
| "text": "**Checking git staging status**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_begin", | ||
| "call_id": "call_RWYAGzcKi0lnpJJvKdnbjpUM", | ||
| "command": [ | ||
| "bash", | ||
| "-lc", | ||
| "git status --short" | ||
| ], | ||
| "cwd": "/home/iatzmon/workspace/codex/.", | ||
| "parsed_cmd": [ | ||
| { | ||
| "type": "unknown", | ||
| "cmd": "git status --short" | ||
| } | ||
| ] | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_output_delta", | ||
| "call_id": "call_RWYAGzcKi0lnpJJvKdnbjpUM", | ||
| "stream": "stdout", | ||
| "chunk": "IE0gQUdFTlRTLm1kCiBNIGNvZGV4LWNsaS9wYWNrYWdlLmpzb24KIE0gY29kZXgtcnMvY2xpL3NyYy9hZ2VudHMucnMKIE0gY29kZXgtcnMvY29yZS9zcmMvY2xpZW50LnJzCiBNIGNvZGV4LXJzL2NvcmUvc3JjL2NvZGV4LnJzCiBNIGNvZGV4LXJzL2NvcmUvc3JjL2NvbnZlcnNhdGlvbl9tYW5hZ2VyLnJzCiBNIGNvZGV4LXJzL2NvcmUvc3JjL2xpYi5ycwogTSBjb2RleC1ycy9jb3JlL3NyYy9vcGVuYWlfdG9vbHMucnMKIE0gY29kZXgtcnMvY29yZS9zcmMvc3Bhd24ucnMKIE0gY29kZXgtcnMvY29yZS90ZXN0cy9jb250cmFjdHMucnMKIE0gY29kZXgtcnMvY29yZS90ZXN0cy9zdWl0ZS9tb2QucnMKIE0gY29kZXgtcnMvdHVpL3NyYy9hcHAucnMKIE0gY29kZXgtcnMvdHVpL3NyYy9hcHBfYmFja3RyYWNrLnJzCiBNIGNvZGV4LXJzL3R1aS9zcmMvY2hhdHdpZGdldC5ycwogTSBjb2RleC1ycy90dWkvc3JjL2NoYXR3aWRnZXQvdGVzdHMucnMKIE0gY29kZXgtcnMvdHVpL3NyYy9saWIucnMKIE0gY29kZXgtcnMvdHVpL3NyYy91c2VyX2FwcHJvdmFsX3dpZGdldC5ycwogTSBjb2RleC1ycy90dWkvdGVzdHMvc3VpdGUvbW9kLnJzCiBNIGRvY3MvZ2V0dGluZy1zdGFydGVkLm1kCiBNIHNwZWNzLzAwNC1zdWJhZ2VudHMtbWQvdGFza3MubWQKPz8gLmNvZGV4L2FnZW50cy8KPz8gY29kZXgtcnMvY29yZS9zcmMvc3ViYWdlbnRzLwo/PyBjb2RleC1ycy9jb3JlL3Rlc3RzL2NvbnRyYWN0cy9hZ2VudHNfbGlzdC5ycwo/PyBjb2RleC1ycy9jb3JlL3Rlc3RzL2NvbnRyYWN0cy9zdWJhZ2VudF9pbnZva2UucnMKPz8gY29kZXgtcnMvY29yZS90ZXN0cy9zdWJhZ2VudHNfY29uZmlnLnJzCj8/IGNvZGV4LXJzL2NvcmUvdGVzdHMvc3ViYWdlbnRzX2ludm9jYXRpb24ucnMKPz8gY29kZXgtcnMvY29yZS90ZXN0cy9zdWl0ZS9zdWJhZ2VudHNfcHJpbWFyeV9zdG9yeS5ycwo/PyBjb2RleC1ycy9jb3JlL3Rlc3RzL3N1aXRlL3N1YmFnZW50c190b29sX3Jlc3RyaWN0aW9ucy5ycwo/PyBjb2RleC1ycy90dWkvdGVzdHMvc3VpdGUvc3ViYWdlbnRzX3F1aWNrc3RhcnQucnMKPz8gZG9jcy9zdWJhZ2VudHMtdG9vbC5tZAo/PyBkb2NzL3N1YmFnZW50cy5tZAo/PyBzcGVjcy8wMDQtc3ViYWdlbnRzLW1kL2NvbnRyYWN0cy8KPz8gc3BlY3MvMDA0LXN1YmFnZW50cy1tZC9kYXRhLW1vZGVsLm1kCj8/IHNwZWNzLzAwNC1zdWJhZ2VudHMtbWQvcGxhbi5tZAo/PyBzcGVjcy8wMDQtc3ViYWdlbnRzLW1kL3F1aWNrc3RhcnQubWQKPz8gc3BlY3MvMDA0LXN1YmFnZW50cy1tZC9yZXNlYXJjaC5tZAo/PyBzcGVjcy8wMDQtc3ViYWdlbnRzLW1kL3NwZWMubWQKPz8gc3ViYWdlbnRzLm1kCg==" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "exec_command_end", | ||
| "call_id": "call_RWYAGzcKi0lnpJJvKdnbjpUM", | ||
| "stdout": " M AGENTS.md\n M codex-cli/package.json\n M codex-rs/cli/src/agents.rs\n M codex-rs/core/src/client.rs\n M codex-rs/core/src/codex.rs\n M codex-rs/core/src/conversation_manager.rs\n M codex-rs/core/src/lib.rs\n M codex-rs/core/src/openai_tools.rs\n M codex-rs/core/src/spawn.rs\n M codex-rs/core/tests/contracts.rs\n M codex-rs/core/tests/suite/mod.rs\n M codex-rs/tui/src/app.rs\n M codex-rs/tui/src/app_backtrack.rs\n M codex-rs/tui/src/chatwidget.rs\n M codex-rs/tui/src/chatwidget/tests.rs\n M codex-rs/tui/src/lib.rs\n M codex-rs/tui/src/user_approval_widget.rs\n M codex-rs/tui/tests/suite/mod.rs\n M docs/getting-started.md\n M specs/004-subagents-md/tasks.md\n?? .codex/agents/\n?? codex-rs/core/src/subagents/\n?? codex-rs/core/tests/contracts/agents_list.rs\n?? codex-rs/core/tests/contracts/subagent_invoke.rs\n?? codex-rs/core/tests/subagents_config.rs\n?? codex-rs/core/tests/subagents_invocation.rs\n?? codex-rs/core/tests/suite/subagents_primary_story.rs\n?? codex-rs/core/tests/suite/subagents_tool_restrictions.rs\n?? codex-rs/tui/tests/suite/subagents_quickstart.rs\n?? docs/subagents-tool.md\n?? docs/subagents.md\n?? specs/004-subagents-md/contracts/\n?? specs/004-subagents-md/data-model.md\n?? specs/004-subagents-md/plan.md\n?? specs/004-subagents-md/quickstart.md\n?? specs/004-subagents-md/research.md\n?? specs/004-subagents-md/spec.md\n?? subagents.md\n", | ||
| "stderr": "", | ||
| "aggregated_output": " M AGENTS.md\n M codex-cli/package.json\n M codex-rs/cli/src/agents.rs\n M codex-rs/core/src/client.rs\n M codex-rs/core/src/codex.rs\n M codex-rs/core/src/conversation_manager.rs\n M codex-rs/core/src/lib.rs\n M codex-rs/core/src/openai_tools.rs\n M codex-rs/core/src/spawn.rs\n M codex-rs/core/tests/contracts.rs\n M codex-rs/core/tests/suite/mod.rs\n M codex-rs/tui/src/app.rs\n M codex-rs/tui/src/app_backtrack.rs\n M codex-rs/tui/src/chatwidget.rs\n M codex-rs/tui/src/chatwidget/tests.rs\n M codex-rs/tui/src/lib.rs\n M codex-rs/tui/src/user_approval_widget.rs\n M codex-rs/tui/tests/suite/mod.rs\n M docs/getting-started.md\n M specs/004-subagents-md/tasks.md\n?? .codex/agents/\n?? codex-rs/core/src/subagents/\n?? codex-rs/core/tests/contracts/agents_list.rs\n?? codex-rs/core/tests/contracts/subagent_invoke.rs\n?? codex-rs/core/tests/subagents_config.rs\n?? codex-rs/core/tests/subagents_invocation.rs\n?? codex-rs/core/tests/suite/subagents_primary_story.rs\n?? codex-rs/core/tests/suite/subagents_tool_restrictions.rs\n?? codex-rs/tui/tests/suite/subagents_quickstart.rs\n?? docs/subagents-tool.md\n?? docs/subagents.md\n?? specs/004-subagents-md/contracts/\n?? specs/004-subagents-md/data-model.md\n?? specs/004-subagents-md/plan.md\n?? specs/004-subagents-md/quickstart.md\n?? specs/004-subagents-md/research.md\n?? specs/004-subagents-md/spec.md\n?? subagents.md\n", | ||
| "exit_code": 0, | ||
| "duration": { | ||
| "secs": 0, | ||
| "nanos": 346167362 | ||
| }, | ||
| "formatted_output": " M AGENTS.md\n M codex-cli/package.json\n M codex-rs/cli/src/agents.rs\n M codex-rs/core/src/client.rs\n M codex-rs/core/src/codex.rs\n M codex-rs/core/src/conversation_manager.rs\n M codex-rs/core/src/lib.rs\n M codex-rs/core/src/openai_tools.rs\n M codex-rs/core/src/spawn.rs\n M codex-rs/core/tests/contracts.rs\n M codex-rs/core/tests/suite/mod.rs\n M codex-rs/tui/src/app.rs\n M codex-rs/tui/src/app_backtrack.rs\n M codex-rs/tui/src/chatwidget.rs\n M codex-rs/tui/src/chatwidget/tests.rs\n M codex-rs/tui/src/lib.rs\n M codex-rs/tui/src/user_approval_widget.rs\n M codex-rs/tui/tests/suite/mod.rs\n M docs/getting-started.md\n M specs/004-subagents-md/tasks.md\n?? .codex/agents/\n?? codex-rs/core/src/subagents/\n?? codex-rs/core/tests/contracts/agents_list.rs\n?? codex-rs/core/tests/contracts/subagent_invoke.rs\n?? codex-rs/core/tests/subagents_config.rs\n?? codex-rs/core/tests/subagents_invocation.rs\n?? codex-rs/core/tests/suite/subagents_primary_story.rs\n?? codex-rs/core/tests/suite/subagents_tool_restrictions.rs\n?? codex-rs/tui/tests/suite/subagents_quickstart.rs\n?? docs/subagents-tool.md\n?? docs/subagents.md\n?? specs/004-subagents-md/contracts/\n?? specs/004-subagents-md/data-model.md\n?? specs/004-subagents-md/plan.md\n?? specs/004-subagents-md/quickstart.md\n?? specs/004-subagents-md/research.md\n?? specs/004-subagents-md/spec.md\n?? subagents.md\n" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "token_count", | ||
| "info": { | ||
| "total_token_usage": { | ||
| "input_tokens": 7742, | ||
| "cached_input_tokens": 3840, | ||
| "output_tokens": 207, | ||
| "reasoning_output_tokens": 128, | ||
| "total_tokens": 7949 | ||
| }, | ||
| "last_token_usage": { | ||
| "input_tokens": 3974, | ||
| "cached_input_tokens": 3840, | ||
| "output_tokens": 39, | ||
| "reasoning_output_tokens": 0, | ||
| "total_tokens": 4013 | ||
| }, | ||
| "model_context_window": 272000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_section_break" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**Expl" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "aining" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " absence" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " of" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": " changes" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning_delta", | ||
| "delta": "**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_reasoning", | ||
| "text": "**Explaining absence of staged changes**" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "No" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " changes" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " right" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " now" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " Everything" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " listed" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " under" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " `" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "git" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " status" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "`" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " is" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " still" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " unst" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "aged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "," | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " so" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " there" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "’s" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " nothing" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " for" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " me" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " to" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " review" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " for" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " risk" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " yet" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " Let" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " me" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " know" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " once" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " you" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "’ve" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " staged" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " the" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " updates" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " you" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " want" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": " inspected" | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message_delta", | ||
| "delta": "." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "agent_message", | ||
| "message": "No staged changes right now. Everything listed under `git status` is still unstaged, so there’s nothing for me to review for risk yet. Let me know once you’ve staged the updates you want inspected." | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "token_count", | ||
| "info": { | ||
| "total_token_usage": { | ||
| "input_tokens": 12241, | ||
| "cached_input_tokens": 7808, | ||
| "output_tokens": 448, | ||
| "reasoning_output_tokens": 320, | ||
| "total_tokens": 12689 | ||
| }, | ||
| "last_token_usage": { | ||
| "input_tokens": 4499, | ||
| "cached_input_tokens": 3968, | ||
| "output_tokens": 241, | ||
| "reasoning_output_tokens": 192, | ||
| "total_tokens": 4740 | ||
| }, | ||
| "model_context_window": 272000 | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "0", | ||
| "msg": { | ||
| "type": "task_complete", | ||
| "last_agent_message": "No staged changes right now. Everything listed under `git status` is still unstaged, so there’s nothing for me to review for risk yet. Let me know once you’ve staged the updates you want inspected." | ||
| } | ||
| } | ||
| ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Do not commit live session logs; they contain PII and reasoning traces.
The file leaks a local username/home path and detailed agent reasoning. Move session logs out of VCS and add an ignore. If you want an example, commit a redacted, minimal sample under docs/ with scrubbed paths and no reasoning deltas.
Apply this repo‑root .gitignore diff:
+# Avoid committing local agent session logs
+.codex/agents/sessions/Optionally replace this file with a sanitized example (shortened, no agent_reasoning_*, paths as <WORKSPACE>).
Run to locate any other committed session artifacts or absolute home paths:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Committed session logs:"
rg -n --glob '!**/.git/**' -g '.codex/agents/sessions/**/*.json' || true
echo
echo "Absolute home paths in repo:"
rg -nP --glob '!**/.git/**' '/home/[A-Za-z0-9._-]+/workspace/codex' || trueLength of output: 599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked session JSONs:"
git ls-files | grep -nE '^\.codex/agents/sessions/.*\.json$' || true
echo
echo "Untracked session JSONs:"
git ls-files --others --exclude-standard | grep -nE '^\.codex/agents/sessions/.*\.json$' || true
echo
echo "Committed (tracked) files containing absolute home paths (/home/.../workspace/codex):"
git grep -nP '/home/[A-Za-z0-9._-]+/workspace/codex' || true
echo
echo "Any files in working tree (tracked + untracked) containing absolute home paths (excluding .git):"
grep -RIn --binary-files=without-match --exclude-dir=.git -E '/home/[A-Za-z0-9._-]+/workspace/codex' || trueLength of output: 56093
Critical — remove committed agent session logs and purge PII
Committed session JSONs contain an absolute home path (/home/iatzmon/...) and detailed agent reasoning; remove and purge from history.
- Affected (tracked): .codex/agents/sessions/20250923T003110Z.json, .codex/agents/sessions/latest.json
- Other files exposing absolute home paths (inspect/sanitize): codex-rs/core/tests/contracts/agents_list.rs, codex-rs/core/tests/contracts/subagent_invoke.rs, codex-rs/core/tests/suite/subagents_primary_story.rs, codex-rs/core/tests/suite/subagents_tool_restrictions.rs, and many files under specs/*.
Immediate remediation (minimal):
- Stop tracking and commit removal: git rm --cached .codex/agents/sessions/*.json && git commit -m "Remove local session logs"
- Add ignore (apply diff below) and commit the .gitignore change.
- Replace tracked logs with a redacted sample under docs/ (no agent_reasoning_* entries, replace absolute paths with ).
Recommended .gitignore diff:
+# Avoid committing local agent session logs
+.codex/agents/sessions/Permanent cleanup and follow-up:
- Purge the files from repository history using git filter-repo or the BFG Repo Cleaner and force-push the cleaned branch (coordinate with your team).
- Audit the repo for any other PII/secrets and rotate any credentials if exposed.
- Re-run the ripgrep checks you used to confirm no session JSONs remain tracked and no sensitive absolute paths persist.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .codex/agents/sessions/latest.json (lines 1-656) a full agent session JSON
with absolute home paths and detailed reasoning was committed; remove all
tracked session logs and purge PII from history: untrack and delete the session
JSONs from the index and commit that removal, add a .codex/agents/ (or .codex/)
entry to .gitignore and commit the ignore change, replace the tracked logs with
a redacted sample stored under docs/ that removes agent_reasoning_* entries and
replaces absolute paths with a <WORKSPACE> placeholder, then permanently purge
the removed files from repository history using a history-rewrite tool
(git-filter-repo or BFG) and force-push after coordinating with the team;
finally run a repo-wide search for other absolute home paths/PII, rotate any
exposed credentials, and verify no session JSONs remain tracked.
| /// Invoke a subagent by building the current inventory and delegating to the | ||
| /// runner. The caller supplies an `InvocationSession` describing the | ||
| /// requested tools, confirmation status, and optional pre-populated log | ||
| /// metadata. Discovery diagnostics are logged prior to execution. | ||
| pub async fn invoke_subagent( | ||
| &self, | ||
| config: &Config, | ||
| session: InvocationSession, | ||
| ) -> Result<InvocationSession, SubagentInvocationError> { | ||
| let inventory = build_inventory_for_config(config); | ||
| log_subagent_inventory(&inventory); | ||
| let runner = SubagentRunner::new(&config.subagents, &inventory); | ||
| let prepared = runner.invoke(session)?; | ||
| let config_clone = config.clone(); | ||
| let result = | ||
| execute_subagent_invocation(&config_clone, self.auth_manager(), prepared).await; | ||
| match result { | ||
| Ok(invocation) => { | ||
| log_subagent_invocation(&invocation); | ||
| Ok(invocation) | ||
| } | ||
| Err(err) => { | ||
| log_subagent_invocation_error(&err); | ||
| Err(err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Invocation path: early‑out when feature disabled and minimize rebuilds.
- Short‑circuit when
config.subagents.enabled == falsewith a clear error. - Accept an optional inventory to avoid rebuilding for batch invocations.
I can propose a patch once we confirm the desired error variant (e.g., FeatureDisabled).
| source_path: PathBuf::from("/home/iatzmon/workspace/codex/.codex/agents/shell-guard.md"), | ||
| validation_errors: Vec::new(), |
There was a problem hiding this comment.
Avoid developer-specific absolute paths in tests.
Hard-coding /home/iatzmon/... breaks CI and other machines. Use a stable dummy path like /workspace/... as in other tests.
- source_path: PathBuf::from("/home/iatzmon/workspace/codex/.codex/agents/shell-guard.md"),
+ source_path: PathBuf::from("/workspace/.codex/agents/shell-guard.md"),📝 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.
| source_path: PathBuf::from("/home/iatzmon/workspace/codex/.codex/agents/shell-guard.md"), | |
| validation_errors: Vec::new(), | |
| source_path: PathBuf::from("/workspace/.codex/agents/shell-guard.md"), | |
| validation_errors: Vec::new(), |
🤖 Prompt for AI Agents
In codex-rs/core/tests/suite/subagents_tool_restrictions.rs around lines 20 to
21, the test uses a developer-specific absolute path (/home/iatzmon/...), which
breaks CI and other machines; replace it with a stable, non-user-specific dummy
path used elsewhere (for example /workspace/... or a relative temp path) so
tests are reproducible across environments and update the PathBuf::from(...)
value accordingly.
| ``` | ||
| --- | ||
| name: code-reviewer | ||
| description: Expert code review specialist; use immediately after writing or modifying code. | ||
| tools: Read, Grep, Glob, Bash | ||
| model: inherit | ||
| --- | ||
| You are a senior code reviewer ensuring high standards of code quality and security. | ||
| When invoked: | ||
| 1. Run git diff to see recent changes | ||
| 2. Focus on modified files | ||
| 3. Begin review immediately | ||
| Provide feedback organized by priority (Critical, Warnings, Suggestions) with concrete fixes. | ||
| ``` |
There was a problem hiding this comment.
Fix YAML frontmatter example: tools should be a list, not a comma-separated string.
As written, tools: Read, Grep, Glob, Bash parses as a scalar and will not match your schema (array). Use a YAML list.
Apply this diff to correct the example:
---
name: code-reviewer
description: Expert code review specialist; use immediately after writing or modifying code.
-tools: Read, Grep, Glob, Bash
+tools:
+ - Read
+ - Grep
+ - Glob
+ - Bash
model: inherit
---📝 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.
| ``` | |
| --- | |
| name: code-reviewer | |
| description: Expert code review specialist; use immediately after writing or modifying code. | |
| tools: Read, Grep, Glob, Bash | |
| model: inherit | |
| --- | |
| You are a senior code reviewer ensuring high standards of code quality and security. | |
| When invoked: | |
| 1. Run git diff to see recent changes | |
| 2. Focus on modified files | |
| 3. Begin review immediately | |
| Provide feedback organized by priority (Critical, Warnings, Suggestions) with concrete fixes. | |
| ``` | |
| --- | |
| name: code-reviewer | |
| description: Expert code review specialist; use immediately after writing or modifying code. | |
| tools: | |
| - Read | |
| - Grep | |
| - Glob | |
| - Bash | |
| model: inherit | |
| --- | |
| You are a senior code reviewer ensuring high standards of code quality and security. | |
| When invoked: | |
| 1. Run git diff to see recent changes | |
| 2. Focus on modified files | |
| 3. Begin review immediately | |
| Provide feedback organized by priority (Critical, Warnings, Suggestions) with concrete fixes. |
🤖 Prompt for AI Agents
In subagents.md around lines 61 to 74, the YAML frontmatter example uses a
comma-separated scalar for tools ("Read, Grep, Glob, Bash") which breaks the
expected array<string> schema; update the frontmatter so tools is a YAML list
(each tool as its own - item) and ensure proper indentation and dashes so the
parser recognizes an array.
![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 suggestions.
Reply with @codex fix comments to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
|
|
||
| let mut session = InvocationSession::new(args.name.trim()); | ||
| session = session.confirmed(); | ||
| session.parent_session_id = Some(sess.conversation_id.to_string()); | ||
| if !args.requested_tools.is_empty() { |
There was a problem hiding this comment.
[P1] Avoid auto‑confirming subagent invocations
The invoke_subagent tool path marks every invocation as confirmed before handing it to SubagentRunner. Because of the unconditional session = session.confirmed(); here, the confirmation gate in SubagentRunner::invoke is never triggered, even when the discovery mode is Auto and no user approval has been collected. This means auto‑suggested subagents execute immediately without the required confirmation and the ConfirmationRequired error can never surface. The CLI can safely set the flag because the user explicitly ran the command, but the generic tool handler should leave confirmed false and let callers flip it only after the user approves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codex-rs/core/src/openai_tools.rs (1)
246-259: Avoid O(n²) char counting in truncate().Track the character count instead of recomputing
out.chars().count()each iteration.fn truncate(input: &str, max: usize) -> String { if input.chars().count() <= max { return input.to_string(); } - let mut out = String::new(); - for ch in input.chars() { - if out.chars().count() + 1 > max.saturating_sub(1) { - break; - } - out.push(ch); - } + let mut out = String::with_capacity(max.saturating_sub(1)); + let mut n = 0usize; + for ch in input.chars() { + if n + 1 > max.saturating_sub(1) { + break; + } + out.push(ch); + n += 1; + } out.push('…'); out }codex-rs/core/src/codex.rs (1)
389-405: Cache subagent inventory per session — avoid rescanning on every turn.compute_subagent_tooling() is defined at codex-rs/core/src/codex.rs:389 and is called at 488, 640, 2048, 2250, 2715, 4926; it currently rebuilds the inventory each call. Move inventory construction into session state (store Arc), compute once at session init and refresh only when config.subagents changes or on explicit refresh.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.codex/agents/code-reviewer.md(1 hunks)codex-rs/core/src/codex.rs(23 hunks)codex-rs/core/src/openai_tools.rs(15 hunks)codex-rs/core/src/subagents/executor.rs(1 hunks)codex-rs/core/tests/contracts/subagent_invoke.rs(1 hunks)docs/subagents.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/subagents.md
- codex-rs/core/tests/contracts/subagent_invoke.rs
- codex-rs/core/src/subagents/executor.rs
- .codex/agents/code-reviewer.md
🧰 Additional context used
📓 Path-based instructions (2)
codex-rs/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/**/*.rs: Use rustfmt defaults for all Rust code
Prefer compact imports in Rust files
Inline variables directly inside format! braces rather than pre-binding temporaries
Files:
codex-rs/core/src/codex.rscodex-rs/core/src/openai_tools.rs
codex-rs/**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust module names snake_case
Files:
codex-rs/core/src/codex.rscodex-rs/core/src/openai_tools.rs
🧬 Code graph analysis (2)
codex-rs/core/src/codex.rs (3)
codex-rs/core/src/subagents/service.rs (1)
build_inventory_for_config(28-33)codex-rs/core/src/subagents/executor.rs (1)
execute_subagent_invocation(137-270)codex-rs/core/src/openai_tools.rs (5)
inventory(120-124)inventory(126-130)inventory(132-143)from_inventory(115-152)new(309-354)
codex-rs/core/src/openai_tools.rs (5)
codex-rs/core/src/codex.rs (2)
lines(4400-4404)new(510-753)codex-rs/core/src/subagents/config.rs (2)
default(15-21)enabled(37-39)codex-rs/core/tests/subagents_config.rs (1)
definition(10-22)codex-rs/core/tests/subagents_invocation.rs (1)
definition(9-21)codex-rs/core/src/subagents/record.rs (1)
from_definition(21-42)
🔇 Additional comments (6)
codex-rs/core/src/openai_tools.rs (3)
805-809: Signature change acknowledged; callers pass third arg.
get_openai_toolsnow takessubagent_tool; this file’s tests already passNoneand compile.
860-862: Conditional inclusion of invoke_subagent looks good.Tool is appended when registration exists; ordering remains deterministic.
1394-1461: Strengthen test to assert all invoke_subagent parameters.Also assert presence of
requested_toolsandmodelto guard regressions.if let JsonSchema::Object { properties, .. } = invoke_tool.parameters { assert!(properties.contains_key("name")); assert!(properties.contains_key("instructions")); + assert!(properties.contains_key("requested_tools")); + assert!(properties.contains_key("model")); } else { panic!("invoke_subagent should expose object parameters"); }codex-rs/core/src/codex.rs (3)
376-379: TurnContext wiring for subagents: LGTM.Carrying inventory, registration, and config per turn is clean and keeps prompts accurate.
3083-3087: Call site updated for new get_openai_tools signature.Passing
turn_context.subagent_tool.as_ref()is correct.
3635-3799: Do not trust client-providedconfirmed; require server-side confirmationIgnore the InvokeSubagent tool's confirmed flag and only mark session.confirmed() server-side when subagent_config.discovery == SubagentDiscoveryMode::Manual.
Location: codex-rs/core/src/codex.rs (invoke_subagent handler)
- #[serde(default)] - confirmed: bool, @@ - if args.confirmed || matches!(subagent_config.discovery, SubagentDiscoveryMode::Manual) - { - session = session.confirmed(); - } + if matches!(subagent_config.discovery, SubagentDiscoveryMode::Manual) { + session = session.confirmed(); + }Optionally remove the param from the tool schema (openai_tools.rs).
| fn create_invoke_subagent_tool(registration: &SubagentToolRegistration) -> OpenAiTool { | ||
| let mut properties = BTreeMap::new(); | ||
| properties.insert( | ||
| "name".to_string(), | ||
| JsonSchema::String { | ||
| description: Some(registration.format_name_description()), | ||
| }, | ||
| ); | ||
| properties.insert( | ||
| "instructions".to_string(), | ||
| JsonSchema::String { | ||
| description: Some( | ||
| "Optional instructions for the subagent to execute inside its isolated session." | ||
| .to_string(), | ||
| ), | ||
| }, | ||
| ); | ||
| properties.insert( | ||
| "requested_tools".to_string(), | ||
| JsonSchema::Array { | ||
| items: Box::new(JsonSchema::String { | ||
| description: Some("Tool identifier to request for the invocation.".to_string()), | ||
| }), | ||
| description: Some( | ||
| "Optional subset of allowed tools to request on the subagent's behalf.".to_string(), | ||
| ), | ||
| }, | ||
| ); | ||
| properties.insert( | ||
| "model".to_string(), | ||
| JsonSchema::String { | ||
| description: Some("Optional model override for this subagent run.".to_string()), | ||
| }, | ||
| ); | ||
| properties.insert( | ||
| "confirmed".to_string(), | ||
| JsonSchema::Boolean { | ||
| description: Some( | ||
| "Set to true after the user explicitly approves invoking the subagent when auto discovery is enabled.".to_string(), | ||
| ), | ||
| }, | ||
| ); | ||
|
|
||
| OpenAiTool::Function(ResponsesApiTool { | ||
| name: "invoke_subagent".to_string(), | ||
| description: registration.format_tool_description(), | ||
| strict: false, | ||
| parameters: JsonSchema::Object { | ||
| properties, | ||
| required: Some(vec!["name".to_string()]), | ||
| additional_properties: Some(false), | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do not expose a 'confirmed' flag to the model; this bypasses user approval.
Allowing the model to set confirmed lets it self‑approve subagent execution under Auto discovery, weakening the confirmation gate. The runner cannot distinguish real user approval from a forged flag.
- Immediate mitigation: ignore the flag in the handler and do not include it in the tool schema.
- Follow‑up (recommended): add a proper approval event flow (akin to Exec approval) and set confirmation server‑side only after explicit user consent.
Apply this diff to remove the parameter from the tool schema:
fn create_invoke_subagent_tool(registration: &SubagentToolRegistration) -> OpenAiTool {
@@
- properties.insert(
- "confirmed".to_string(),
- JsonSchema::Boolean {
- description: Some(
- "Set to true after the user explicitly approves invoking the subagent when auto discovery is enabled.".to_string(),
- ),
- },
- );📝 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.
| fn create_invoke_subagent_tool(registration: &SubagentToolRegistration) -> OpenAiTool { | |
| let mut properties = BTreeMap::new(); | |
| properties.insert( | |
| "name".to_string(), | |
| JsonSchema::String { | |
| description: Some(registration.format_name_description()), | |
| }, | |
| ); | |
| properties.insert( | |
| "instructions".to_string(), | |
| JsonSchema::String { | |
| description: Some( | |
| "Optional instructions for the subagent to execute inside its isolated session." | |
| .to_string(), | |
| ), | |
| }, | |
| ); | |
| properties.insert( | |
| "requested_tools".to_string(), | |
| JsonSchema::Array { | |
| items: Box::new(JsonSchema::String { | |
| description: Some("Tool identifier to request for the invocation.".to_string()), | |
| }), | |
| description: Some( | |
| "Optional subset of allowed tools to request on the subagent's behalf.".to_string(), | |
| ), | |
| }, | |
| ); | |
| properties.insert( | |
| "model".to_string(), | |
| JsonSchema::String { | |
| description: Some("Optional model override for this subagent run.".to_string()), | |
| }, | |
| ); | |
| properties.insert( | |
| "confirmed".to_string(), | |
| JsonSchema::Boolean { | |
| description: Some( | |
| "Set to true after the user explicitly approves invoking the subagent when auto discovery is enabled.".to_string(), | |
| ), | |
| }, | |
| ); | |
| OpenAiTool::Function(ResponsesApiTool { | |
| name: "invoke_subagent".to_string(), | |
| description: registration.format_tool_description(), | |
| strict: false, | |
| parameters: JsonSchema::Object { | |
| properties, | |
| required: Some(vec!["name".to_string()]), | |
| additional_properties: Some(false), | |
| }, | |
| }) | |
| } | |
| fn create_invoke_subagent_tool(registration: &SubagentToolRegistration) -> OpenAiTool { | |
| let mut properties = BTreeMap::new(); | |
| properties.insert( | |
| "name".to_string(), | |
| JsonSchema::String { | |
| description: Some(registration.format_name_description()), | |
| }, | |
| ); | |
| properties.insert( | |
| "instructions".to_string(), | |
| JsonSchema::String { | |
| description: Some( | |
| "Optional instructions for the subagent to execute inside its isolated session." | |
| .to_string(), | |
| ), | |
| }, | |
| ); | |
| properties.insert( | |
| "requested_tools".to_string(), | |
| JsonSchema::Array { | |
| items: Box::new(JsonSchema::String { | |
| description: Some("Tool identifier to request for the invocation.".to_string()), | |
| }), | |
| description: Some( | |
| "Optional subset of allowed tools to request on the subagent's behalf.".to_string(), | |
| ), | |
| }, | |
| ); | |
| properties.insert( | |
| "model".to_string(), | |
| JsonSchema::String { | |
| description: Some("Optional model override for this subagent run.".to_string()), | |
| }, | |
| ); | |
| OpenAiTool::Function(ResponsesApiTool { | |
| name: "invoke_subagent".to_string(), | |
| description: registration.format_tool_description(), | |
| strict: false, | |
| parameters: JsonSchema::Object { | |
| properties, | |
| required: Some(vec!["name".to_string()]), | |
| additional_properties: Some(false), | |
| }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In codex-rs/core/src/openai_tools.rs around lines 547 to 600, the tool schema
exposes a "confirmed" boolean that lets the model self-approve subagent
invocation; remove the properties.insert(...) block that creates the "confirmed"
field so the tool schema no longer contains it, keep the required vector as-is
(only "name" required), and ensure additional_properties remains false;
additionally, update the subagent invocation handler to ignore any incoming
"confirmed" key from the model payload and only accept confirmation set
server-side after explicit user consent (do not rely on model-supplied flags).
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codex-rs/mcp-server/src/codex_tool_runner.rs (1)
283-294: Potential hang: SubagentApprovalRequest is ignored; auto‑deny to unblock MCP sessionsUnlike exec CLI, MCP path neither approves nor denies subagent invocations. If core pauses awaiting approval, tool sessions can stall indefinitely. Mirror CLI behavior by auto‑denying in non‑interactive MCP server.
Apply this match‑arm before the catch‑all block:
@@ - | EventMsg::PlanModeApplied(_) - | EventMsg::SubagentApprovalRequest(_) + | EventMsg::PlanModeApplied(_) => { + // ... + } + EventMsg::SubagentApprovalRequest(req) => { + // Non-interactive: auto-deny. + let _ = codex + .submit(Op::SubagentApproval { + name: req.subagent.clone(), + decision: SubagentApprovalDecision::Denied, + }) + .await; + continue; + } + | EventMsg::HookListResponse(_)And add the missing import:
// Near other protocol imports use codex_core::protocol::SubagentApprovalDecision;
🧹 Nitpick comments (11)
codex-rs/core/src/conversation_history.rs (1)
39-45: Test-only helper looks good; consider generalizing for reuse.Optional: expose a role-parametrized helper and keep this as a thin wrapper.
#[cfg(test)] -pub(crate) fn user_message_count(&self) -> usize { - self.items - .iter() - .filter(|item| matches!(item, ResponseItem::Message { role, .. } if role == "user")) - .count() -} +pub(crate) fn count_messages_by_role(&self, role: &str) -> usize { + self.items + .iter() + .filter(|item| matches!(item, ResponseItem::Message { role: r, .. } if r == role)) + .count() +} + +#[cfg(test)] +pub(crate) fn user_message_count(&self) -> usize { + self.count_messages_by_role("user") +}codex-cli/src/ipc/subagents.ts (3)
96-100: Clip huge stderr/stdout in errors to prevent megabyte-sized messages.Safer in CI and TUI logs.
-function buildCommandError(command: string, result: CommandResult): Error { - const output = result.stderr.trim() || result.stdout.trim(); - const context = output.length > 0 ? `: ${output}` : ""; - return new Error(`${command} failed with exit code ${result.code}${context}`); -} +function buildCommandError(command: string, result: CommandResult): Error { + const output = (result.stderr || result.stdout || "").trim(); + const snippet = truncate(output); + const context = snippet.length > 0 ? `: ${snippet}` : ""; + return new Error(`${command} failed with exit code ${result.code}${context}`); +}
33-36: Unused SubagentErrorPayload.Remove if not part of the public surface, or wire it into error paths.
292-301: Defaulting unknown scope to "all" may mislead. Confirm expected semantics.For a single record, "all" isn’t a meaningful scope. Consider defaulting to "project" or leaving out-of-band (e.g., null) if unknown.
codex-rs/core/src/subagents/parser.rs (2)
124-145: Frontmatter terminator detection can false‑match; prefer line‑anchored ‘---’Using
.find("\n---")may match a literal---embedded in YAML/body. Consider scanning for a line that is exactly---(after normalizing CRLF) to terminate frontmatter, or require\n---\nas a delimiter. This will avoid accidental splits inside values.
92-109: Duplicate user‑facing validation for name/description; confirm intent
SubagentDefinition::new(..)already adds validation when normalized name/description are empty. Here you also add errors when frontmatter omits those fields even if a path‑based fallback name is used. If the UX should warn specifically about missing frontmatter fields, keep both; otherwise, drop one to avoid duplicate/overlapping messages.codex-rs/exec/src/event_processor_with_human_output.rs (1)
187-198: Inline the temporary variable in format!Minor style per repo guideline: inline directly in the format! call.
- EventMsg::SubagentApprovalRequest(request) => { - let name = request.subagent.as_str(); + EventMsg::SubagentApprovalRequest(request) => { ts_println!( self, "{}", format!( "Auto-denying subagent '{}' – interactive approval is not supported in CLI mode.", - name + request.subagent ) .style(self.dimmed) ); }codex-rs/core/src/subagents/inventory.rs (1)
62-70: Emit a discovery event when no active definition is availableWhen no Active record exists for a name, consider surfacing a discovery event for visibility.
Apply this diff just after computing chosen_idx:
let chosen_idx = records .iter() .position(|record| matches!(record.status, SubagentStatus::Active)); + if chosen_idx.is_none() { + inventory.discovery_events.push(DiscoveryEvent { + message: format!( + "no active subagent definition available for '{}'", + name + ), + }); + }codex-rs/tui/src/chatwidget.rs (1)
811-816: ConversationManager fieldStoring the ConversationManager as a private, intentionally-unused field is fine to keep ownership alive. If not needed, consider dropping it later.
Also applies to: 821-822, 855-856, 892-893
codex-rs/core/src/codex.rs (2)
393-410: Avoid rebuilding subagent inventory every turn; cache per session.
compute_subagent_tooling()rebuilds the inventory on every per‑turn config override and review/slash flows. Discovery can be I/O heavy.
- Build once at session start.
- Cache
(Arc<SubagentInventory>, Option<SubagentToolRegistration>, SubagentConfig)on Session and reuse when model-only overrides occur.- Recompute only when subagent config or discovery sources change.
218-224: Typo in comment."oonly" -> "only".
Apply:
-// Model-formatting limits: clients get full streams; oonly content sent to the model is truncated. +// Model-formatting limits: clients get full streams; only content sent to the model is truncated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.codex/agents/code-reviewer.md(1 hunks).gitignore(1 hunks)AGENTS.md(1 hunks)codex-cli/src/ipc/subagents.ts(1 hunks)codex-rs/cli/src/agents.rs(1 hunks)codex-rs/core/src/codex.rs(24 hunks)codex-rs/core/src/conversation_history.rs(2 hunks)codex-rs/core/src/rollout/policy.rs(1 hunks)codex-rs/core/src/subagents/executor.rs(1 hunks)codex-rs/core/src/subagents/inventory.rs(1 hunks)codex-rs/core/src/subagents/parser.rs(1 hunks)codex-rs/core/src/subagents/runner.rs(1 hunks)codex-rs/core/tests/contracts/agents_list.rs(1 hunks)codex-rs/core/tests/contracts/subagent_invoke.rs(1 hunks)codex-rs/exec/src/event_processor_with_human_output.rs(1 hunks)codex-rs/exec/src/lib.rs(5 hunks)codex-rs/mcp-server/src/codex_tool_runner.rs(1 hunks)codex-rs/protocol/src/protocol.rs(4 hunks)codex-rs/tui/src/chatwidget.rs(9 hunks)codex-rs/tui/src/user_approval_widget.rs(14 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (6)
- .codex/agents/code-reviewer.md
- codex-rs/core/tests/contracts/subagent_invoke.rs
- codex-rs/core/tests/contracts/agents_list.rs
- AGENTS.md
- codex-rs/cli/src/agents.rs
- codex-rs/core/src/subagents/executor.rs
🧰 Additional context used
📓 Path-based instructions (3)
codex-rs/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/**/*.rs: Use rustfmt defaults for all Rust code
Prefer compact imports in Rust files
Inline variables directly inside format! braces rather than pre-binding temporaries
Files:
codex-rs/core/src/conversation_history.rscodex-rs/core/src/rollout/policy.rscodex-rs/mcp-server/src/codex_tool_runner.rscodex-rs/exec/src/event_processor_with_human_output.rscodex-rs/core/src/subagents/parser.rscodex-rs/protocol/src/protocol.rscodex-rs/core/src/subagents/inventory.rscodex-rs/core/src/subagents/runner.rscodex-rs/exec/src/lib.rscodex-rs/tui/src/chatwidget.rscodex-rs/core/src/codex.rscodex-rs/tui/src/user_approval_widget.rs
codex-rs/**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust module names snake_case
Files:
codex-rs/core/src/conversation_history.rscodex-rs/core/src/rollout/policy.rscodex-rs/mcp-server/src/codex_tool_runner.rscodex-rs/exec/src/event_processor_with_human_output.rscodex-rs/core/src/subagents/parser.rscodex-rs/protocol/src/protocol.rscodex-rs/core/src/subagents/inventory.rscodex-rs/core/src/subagents/runner.rscodex-rs/exec/src/lib.rscodex-rs/tui/src/chatwidget.rscodex-rs/core/src/codex.rscodex-rs/tui/src/user_approval_widget.rs
codex-rs/tui/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/tui/**/*.rs: In the TUI, use Ratatui Stylize helpers (e.g., "text".dim(), url.cyan().underlined()) instead of manual Style construction
Avoid hard-coded white foregrounds in the TUI; defer to theme defaults
Files:
codex-rs/tui/src/chatwidget.rscodex-rs/tui/src/user_approval_widget.rs
🧬 Code graph analysis (8)
codex-rs/core/src/subagents/parser.rs (2)
codex-rs/core/tests/subagents_config.rs (1)
definition(10-22)codex-rs/core/src/subagents/definition.rs (2)
new(28-32)new(36-68)
codex-rs/core/src/subagents/inventory.rs (5)
codex-rs/core/src/openai_tools.rs (4)
inventory(120-124)inventory(126-130)inventory(132-143)new(309-354)codex-rs/core/src/subagents/config.rs (2)
default(15-21)new(25-35)codex-rs/core/src/subagents/runner.rs (1)
new(44-46)codex-rs/core/src/subagents/definition.rs (3)
new(28-32)new(36-68)normalize_name(88-115)codex-rs/core/src/subagents/record.rs (1)
from_definition(21-42)
codex-rs/core/src/subagents/runner.rs (3)
codex-rs/core/src/subagents/config.rs (1)
new(25-35)codex-rs/core/src/subagents/definition.rs (2)
new(28-32)new(36-68)codex-rs/core/src/subagents/invocation.rs (1)
new(17-29)
codex-rs/exec/src/lib.rs (2)
codex-rs/core/src/codex.rs (1)
submit(290-298)codex-rs/core/src/codex_conversation.rs (1)
submit(21-24)
codex-cli/src/ipc/subagents.ts (2)
codex-cli/bin/codex.js (2)
child(110-113)process(11-11)codex-cli/scripts/dev_build.mjs (1)
cliRoot(6-6)
codex-rs/tui/src/chatwidget.rs (3)
codex-rs/tui/src/chatwidget/agent.rs (1)
spawn_agent(16-62)codex-rs/protocol/src/models.rs (1)
items(212-238)codex-rs/tui/src/user_approval_widget.rs (1)
new(193-301)
codex-rs/core/src/codex.rs (5)
codex-rs/core/src/subagents/service.rs (1)
build_inventory_for_config(28-33)codex-rs/core/src/subagents/executor.rs (1)
execute_subagent_invocation(144-278)codex-rs/core/src/openai_tools.rs (5)
inventory(120-124)inventory(126-130)inventory(132-143)from_inventory(115-152)new(309-354)codex-rs/core/src/client.rs (2)
new(80-99)s(295-295)codex-rs/core/src/subagents/definition.rs (1)
normalize_name(88-115)
codex-rs/tui/src/user_approval_widget.rs (2)
codex-rs/tui/src/history_cell.rs (5)
new(137-142)new(511-513)new(810-822)line(1676-1679)new_user_approval_decision(782-784)codex-rs/tui/src/app_event_sender.rs (1)
new(12-14)
🔇 Additional comments (20)
codex-rs/core/src/conversation_history.rs (2)
129-139: LGTM: solid, focused test.Covers the intended behavior and aligns with record_items filtering.
48-60: Update comment:Reasoningis treated as an API message by the codeis_api_message() matches ResponseItem::Reasoning — the comment is outdated; update the doc or change the predicate.
-/// Anything that is not a system message or "reasoning" message is considered -/// an API message. +/// API messages include all non-system messages and tool-related entries, +/// including "reasoning".Confirm whether "reasoning" should be considered an API message in this module.
codex-cli/src/ipc/subagents.ts (1)
102-132: Child process can hang; add timeout, once-listeners, and cleanup.Process has no timeout and uses repeating listeners; can hang indefinitely and risk double resolve/reject. Add a kill-after timeout, prefer once-listeners, and cleanup timers/listeners. Reuses prior review feedback.
Apply this diff:
-async function runCodexAgentsCommand(args: string[]): Promise<CommandResult> { +async function runCodexAgentsCommand(args: string[], timeoutMs = defaultTimeoutMs()): Promise<CommandResult> { @@ - return new Promise<CommandResult>((resolve, reject) => { - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; + return new Promise<CommandResult>((resolve, reject) => { + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + let settled = false; @@ - child.stdout.on("data", (chunk) => stdoutChunks.push(chunk)); - child.stderr.on("data", (chunk) => stderrChunks.push(chunk)); + child.stdout?.on("data", (chunk) => stdoutChunks.push(chunk)); + child.stderr?.on("data", (chunk) => stderrChunks.push(chunk)); + + let forceKiller: NodeJS.Timeout | undefined; + const cleanup = () => { + clearTimeout(killer); + if (forceKiller) clearTimeout(forceKiller); + }; + + const killer = setTimeout(() => { + // Graceful terminate, then force-kill after a short grace period. + try { child.kill(process.platform === "win32" ? undefined : "SIGTERM"); } catch {} + forceKiller = setTimeout(() => { + try { + if (process.platform === "win32") child.kill(); + else child.kill("SIGKILL"); + } catch {} + }, 5_000); + if (!settled) { + settled = true; + const stdout = Buffer.concat(stdoutChunks).toString("utf8"); + const stderr = Buffer.concat(stderrChunks).toString("utf8"); + cleanup(); + reject(new Error(`codex ${args.join(" ")} timed out after ${timeoutMs}ms; stdout=${truncate(stdout)}, stderr=${truncate(stderr)}`)); + } + }, timeoutMs); @@ - child.on("error", (err) => reject(err)); - child.on("close", (code) => { - resolve({ - stdout: Buffer.concat(stdoutChunks).toString("utf8"), - stderr: Buffer.concat(stderrChunks).toString("utf8"), - code: code ?? 1, - }); - }); + child.once("error", (err) => { + if (settled) return; + settled = true; + cleanup(); + reject(err); + }); + child.once("close", (code) => { + if (settled) return; + settled = true; + cleanup(); + resolve({ + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + code: code ?? 1, + }); + }); }); }Add these helpers (outside the selected range):
function defaultTimeoutMs(): number { const v = Number(process.env.CODEX_CLI_TIMEOUT_MS ?? process.env.CODEX_SUBAGENTS_TIMEOUT_MS ?? "300000"); return Number.isFinite(v) && v > 0 ? v : 300000; } function truncate(s: string, max = 4000): string { return s.length > max ? s.slice(0, max) + "…(truncated)" : s; }codex-rs/core/src/subagents/parser.rs (1)
7-13: Validation error variant is never used; align parser vs discovery behavior
SubagentParserError::Validationexists butparse_definitionalways returnsOk(definition)with embeddedvalidation_errors, so discovery’sErr(Validation(..))arm will never trigger. Decide one approach and make both sides consistent.
- Option A (preferred): Keep returning
Ok(definition)and remove/avoid matching onErr(Validation(..))in discovery, emitting validation events from inventory.- Option B: Return
Err(Validation(..))whendefinition.validation_errorsis non‑empty and update discovery/inventory to still record invalid definitions.codex-rs/core/src/rollout/policy.rs (1)
81-83: LGTM: Don’t persist SubagentApprovalRequest eventsTreating subagent approval prompts as ephemeral is consistent with other approval requests and avoids polluting rollout history.
codex-rs/exec/src/event_processor_with_human_output.rs (1)
187-198: LGTM: Clear non‑interactive message for subagent approvalsGood user feedback path; complements auto‑deny in the exec loop.
codex-rs/exec/src/lib.rs (3)
248-257: LGTM: Auto‑deny subagent requests in non‑interactive modeSubmitting an immediate
Op::SubagentApproval { Denied }keeps the session progressing. Logging at warn level is appropriate.
325-334: LGTM: Helper is simple and test‑coveredThe
auto_deny_subagent_requesthelper cleanly isolates the policy.
356-395: LGTM: Unit tests cover both positive and negative pathsGood coverage for the helper’s behavior.
codex-rs/protocol/src/protocol.rs (2)
524-526: LGTM: New EventMsg variant integrates cleanlyVariant is documented and consistent with other approval request events.
1325-1335: LGTM: Decision enum shape mirrors ReviewDecisionSnake_case serde, Default=Denied, TS derives — consistent with existing enums.
codex-rs/tui/src/user_approval_widget.rs (1)
15-16: Subagent approval flow: wiring, UI, and events — LGTMEnd-to-end handling looks solid: options, ESC/Ctrl‑C denial, history entry, and Op::SubagentApproval emission, plus tests. Nice work.
Also applies to: 53-61, 71-72, 160-176, 245-286, 289-295, 333-337, 356-359, 379-381, 394-427, 507-510, 522-523, 576-577, 731-785
codex-rs/core/src/subagents/inventory.rs (2)
75-84: Remove unreachable Disabled handlingWith the early return on Line 37 when config.enabled is false, SubagentStatus::Disabled cannot happen here. Drop the branch and the “disabled subagent” conflict reason to simplify logic.
[ suggest_recommended_refactor ]
Apply this diff:@@ - match record.status { - SubagentStatus::Invalid => inventory.invalid_records.push(record.clone()), - SubagentStatus::Disabled => inventory.discovery_events.push(DiscoveryEvent { - message: format!( - "subagent '{}' skipped because feature is disabled", - record.definition.name - ), - }), - SubagentStatus::Active => {} - } + if matches!(record.status, SubagentStatus::Invalid) { + inventory.invalid_records.push(record.clone()); + } @@ - let reason = if record.is_invalid() { + let reason = if record.is_invalid() { "invalid definition".to_string() - } else if matches!(record.status, SubagentStatus::Disabled) { - "disabled subagent".to_string() } else if let Some(chosen_scope) = chosen_scope { if chosen_scope != record.definition.scope { "project override".to_string() } else { "duplicate definition".to_string() } } else { "no active definition available".to_string() };Also applies to: 90-103
62-70: Fallback selection behavior now correctSelecting the first Active record in precedence order resolves the previous “invalid top precedence drops agent” issue. Thanks for addressing it.
Also applies to: 72-73, 110-113
codex-rs/tui/src/chatwidget.rs (3)
31-32: Subagent approval request handling — LGTMImport, event dispatch, and UI enqueueing into BottomPane look correct. Redraw is triggered appropriately.
Also applies to: 459-470, 1378-1379
1274-1296: Submit-user-message: guard and ordering — LGTMEarly return on empty text+images and conditional AddToHistory/UserInput sending are sensible and prevent empty turns.
1911-1919: Utility copy: quickstart steps — LGTMHandy helper for onboarding flows.
codex-rs/core/src/subagents/runner.rs (1)
56-70: Fix ownership/clone mismatch (compile error) and avoid no-op code
- record is a &SubagentRecord; cloning it clones the reference, not the value. Fetch an owned SubagentRecord via cloned() and move it where needed.
- Also remove the no-op summary/detail blocks.
Apply this diff:
@@ - let record = self + let record: SubagentRecord = self .inventory .subagents - .get(&session.subagent_name) - .or_else(|| { + .get(&session.subagent_name) + .cloned() + .or_else(|| { self.inventory .invalid() .into_iter() - .find(|record| record.definition.name == session.subagent_name) - .map(|record| record) + .find(|record| record.definition.name == session.subagent_name) + .cloned() }) .ok_or_else(|| { SubagentInvocationError::UnknownSubagent(session.subagent_name.clone()) })?; @@ - if matches!(self.config.discovery, SubagentDiscoveryMode::Auto) && !session.confirmed { + if matches!(self.config.discovery, SubagentDiscoveryMode::Auto) && !session.confirmed { return Err(SubagentInvocationError::ConfirmationRequired { subagent: record.definition.name.clone(), - record: record.clone(), + record, session, }); } @@ - if session.summary.is_none() { - session.summary = None; - } - - if session.detail_artifacts.is_empty() { - session.detail_artifacts.clear(); - } + // no-op blocks removed @@ - Ok(PreparedSubagentInvocation { - session, - record: record.clone(), - }) + Ok(PreparedSubagentInvocation { session, record })Also applies to: 85-91, 117-121
codex-rs/core/src/codex.rs (2)
3837-3841: Do not bypass subagent approval; remove auto-confirm on Manual discovery.Auto-confirming when discovery is Manual defeats the approval gate and regresses the security/UX guarantee. Only set
confirmedwhen explicitly requested by the caller.Apply this diff:
- if args.confirmed || matches!(subagent_config.discovery, SubagentDiscoveryMode::Manual) - { - session = session.confirmed(); - } + if args.confirmed { + session = session.confirmed(); + }Optionally align the arg with earlier feedback by making it optional and off by default:
- #[serde(default)] - confirmed: bool, + #[serde(default)] + confirm: Option<bool>,and
- if args.confirmed { + if args.confirm.unwrap_or(false) { session = session.confirmed(); }
3875-3895: Resolved — default is non‑permissive (Denied).
SubagentApprovalDecision derives Default and the Denied variant is annotated with #[default] in codex-rs/protocol/src/protocol.rs, so rx.await.unwrap_or_default() yields Denied.
| let user_instructions = merge_subagent_user_instructions( | ||
| base_context.user_instructions.clone(), | ||
| subagent_inventory.as_deref(), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Per‑turn subagent guidance is computed but not sent to the model.
You recompute user_instructions per turn, but they only affect the model when recorded into history (build_initial_context runs only once). Net effect: subsequent turns won’t see updated guidance.
Record updated instructions when they change:
- // Install the new persistent context for subsequent tasks/turns.
- turn_context = Arc::new(new_turn_context);
+ // Install the new persistent context for subsequent tasks/turns.
+ let changed_instructions =
+ new_turn_context.user_instructions != prev.user_instructions;
+ turn_context = Arc::new(new_turn_context);
+ if changed_instructions {
+ if let Some(text) = turn_context.user_instructions.clone() {
+ sess.record_conversation_items(&[UserInstructions::new(text).into()])
+ .await;
+ }
+ }Apply the same pattern for the per‑turn UserTurn path after creating fresh_turn_context.
Also applies to: 2167-2174, 2377-2381
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (25)
specs/004-subagents-md/tasks.md (3)
7-33: Add code fence language to satisfy markdownlint (MD040).Label this fenced block as text.
-``` +```text ... -``` +```
112-120: Add code fence language to satisfy markdownlint (MD040).Label this commands block as bash.
-``` +```bash # Launch contract and integration tests together once scaffolding exists: /tasks run --feature 004-subagents-md --id T003 /tasks run --feature 004-subagents-md --id T004 /tasks run --feature 004-subagents-md --id T005 /tasks run --feature 004-subagents-md --id T006 /tasks run --feature 004-subagents-md --id T007 /tasks run --feature 004-subagents-md --id T008 -``` +```
39-45: Document Windows user path variant for agents directory.Small portability polish.
- Node CLI commands/IPCs live in `<WORKSPACE>/codex-cli/src` with tests in `<WORKSPACE>/codex-cli/tests`. + +Notes: +- On Windows, the user agents directory path is `%USERPROFILE%\\.codex\\agents\\`.codex-rs/exec/src/lib.rs (3)
29-33: Consolidate tracing imports per guidelinesUse compact imports.
-use tracing::debug; -use tracing::error; -use tracing::info; -use tracing::warn; +use tracing::{debug, error, info, warn};
253-255: Warn likely suppressed by default log levelDefault level is "error", so warn! won’t show by default. If you intend users to see this, either raise the default to "warn", print to stderr as well, or use info! and adjust defaults accordingly.
362-385: Add test for differing event.id vs request.idCurrent tests use equal ids, masking the correlation bug. Add a case where they differ.
#[test] fn auto_deny_subagent_request_uses_request_id_when_different() { let event = Event { id: "outer-event-id".into(), msg: EventMsg::SubagentApprovalRequest(SubagentApprovalRequestEvent { id: "inner-request-id".into(), subagent: "code-reviewer".into(), description: None, extra_instructions: None, allowed_tools: vec![], requested_tools: vec![], model: None, }), }; match auto_deny_subagent_request(&event) { Some(Op::SubagentApproval { id, name, decision }) => { assert_eq!(id, "inner-request-id"); assert_eq!(name, "code-reviewer"); assert_eq!(decision, SubagentApprovalDecision::Denied); } other => panic!("expected auto-deny op, got {other:?}"), } }codex-rs/core/tests/suite/subagents_tool_restrictions.rs (2)
11-23: Use the public constructor to preserve validation/normalization and reduce brittleness.Constructing
SubagentDefinitionvia struct literal bypasses invariants and ties the test to field names. UseSubagentDefinition::new(...)and then set only what's necessary.Apply this diff:
fn restricted_definition() -> SubagentDefinition { - SubagentDefinition { - raw_name: "shell-guard".into(), - name: "shell-guard".into(), - description: "Runs shell commands with a limited allowlist".into(), - tools: vec!["git_diff".into()], - model: None, - instructions: "Review staged diffs without running arbitrary shell commands.".into(), - scope: SubagentScope::Project, - source_path: PathBuf::from("/workspace/codex/.codex/agents/shell-guard.md"), - validation_errors: Vec::new(), - } + let mut def = SubagentDefinition::new( + "shell-guard", + "Runs shell commands with a limited allowlist", + SubagentScope::Project, + PathBuf::from("/workspace/codex/.codex/agents/shell-guard.md"), + ); + def.tools = vec!["git_diff".into()]; + def.instructions = + "Review staged diffs without running arbitrary shell commands.".into(); + def }
33-43: PreferInvocationSession::new(...)over struct literal to reduce coupling.Use the constructor and set only the fields you need.
Apply this diff:
- let session = InvocationSession { - parent_session_id: Some("root".into()), - subagent_name: "shell-guard".into(), - requested_tools: vec!["filesystem".into()], - execution_log: Vec::new(), - summary: None, - detail_artifacts: Vec::new(), - confirmed: true, - resolved_model: None, - extra_instructions: None, - }; + let mut session = InvocationSession::new("shell-guard"); + session.parent_session_id = Some("root".into()); + session.requested_tools = vec!["filesystem".into()]; + session.confirmed = true;codex-rs/tui/src/user_approval_widget.rs (2)
555-566: Prefer Stylize helpers over manual Style constructionRatatui guideline: use Stylize (e.g., .dim(), .on_cyan(), .black()) instead of building Style directly for the selected/unselected button states.
183-184: Use slice for select_options fieldPrefer &'static [SelectOption] over &'static Vec for API clarity.
Apply this diff:
- select_options: &'static Vec<SelectOption>, + select_options: &'static [SelectOption],No other changes required; existing references (e.g., &SUBAGENT_SELECT_OPTIONS) coerce to slices.
codex-rs/tui/src/chatwidget.rs (2)
459-471: Flush streaming before showing Subagent approval (consistency with other approvals)Exec and patch approvals flush the answer stream to keep transcript ordering predictable. Do the same here.
Apply this diff:
fn on_subagent_approval_request(&mut self, event: SubagentApprovalRequestEvent) { - self.bottom_pane + // Keep ordering consistent with other approval flows. + self.flush_answer_stream_with_separator(); + self.bottom_pane .push_approval_request(ApprovalRequest::Subagent { id: event.id.clone(), name: event.subagent, description: event.description, extra_instructions: event.extra_instructions, allowed_tools: event.allowed_tools, requested_tools: event.requested_tools, model: event.model, }); self.request_redraw(); }
1912-1920: Helper is ok; consider relocating laterPublic quickstart helper is handy; could be moved to a docs/help module in a future cleanup.
codex-rs/core/src/subagents/discovery.rs (4)
50-52: Emit events for missing or invalid root pathsSilent return hides why discovery produced no results. Emit a DiscoveryEvent and also guard against non-directory roots.
Apply this diff:
- if !root.exists() { - return outcome; - } + if !root.exists() { + outcome.events.push(DiscoveryEvent { + message: format!("Discovery root does not exist: {}", root.display()), + }); + return outcome; + } + if !root.is_dir() { + outcome.events.push(DiscoveryEvent { + message: format!("Discovery root is not a directory: {}", root.display()), + }); + return outcome; + }
105-113: Optionally broaden recognized Markdown extensionsIf desired, include common aliases (mdown, mkd, mdx). Low risk, improves UX.
Apply this diff:
- matches!(ext_lower.as_str(), "md" | "markdown") + matches!(ext_lower.as_str(), "md" | "markdown" | "mdown" | "mkd" | "mdx")
69-80: Handle all parser error variants, not just ParseErrorSubagentParserError currently only defines ParseError(String), but matching a single variant is brittle—use a catch-all and format the error to future-proof parsing failures.
- Ok(contents) => match parse_definition(&path, &contents, scope) { - Ok(definition) => outcome.definitions.push(definition), - Err(SubagentParserError::ParseError(message)) => { - outcome.events.push(DiscoveryEvent { - message: format!( - "Failed to parse subagent definition at {}: {}", - path.display(), - message - ), - }); - } - }, + Ok(contents) => match parse_definition(&path, &contents, scope) { + Ok(definition) => outcome.definitions.push(definition), + Err(error) => { + outcome.events.push(DiscoveryEvent { + message: format!( + "Failed to parse subagent definition at {}: {}", + path.display(), + error + ), + }); + } + },
95-100: Replace dirs with dirs-next or homedirs is unmaintained; use dirs-next for full directory helpers or home if you only need home_dir — in codex-rs/core/src/subagents/discovery.rs (lines 95–100) replace dirs::home_dir() with home::home_dir() (or dirs_next::home_dir()) and update Cargo.toml.
codex-rs/core/src/subagents/record.rs (1)
44-49: Tool allowlist lookup can be O(1) with a HashSet.If called frequently, consider storing
effective_toolsasHashSet<String>to speed upallows_tool.- pub effective_tools: Vec<String>, + pub effective_tools: std::collections::HashSet<String>, @@ - Self { - effective_tools: definition.tools.clone(), + Self { + effective_tools: definition.tools.iter().cloned().collect(), @@ - self.effective_tools.iter().any(|allowed| allowed == tool) + self.effective_tools.contains(tool)codex-rs/core/src/subagents/executor.rs (1)
46-73: Blocking std::fs in async path.
persist_transcriptuses blocking I/O inside an async flow. Prefertokio::fs(and make the function async) or wrap inspawn_blockingto avoid stalling the runtime.Would you like me to convert this to an async version and update the tests accordingly?
codex-rs/core/src/config.rs (1)
215-223: Return Path, not PathBuf, from accessors.Expose
&Pathto avoid leaking the concrete buffer type.- pub fn cwd(&self) -> &PathBuf { - &self.cwd - } + pub fn cwd(&self) -> &std::path::Path { + &self.cwd + } @@ - pub fn codex_home(&self) -> &PathBuf { - &self.codex_home - } + pub fn codex_home(&self) -> &std::path::Path { + &self.codex_home + }codex-rs/core/src/openai_tools.rs (1)
246-259: Minor: avoid O(n^2) in truncate.
out.chars().count()inside the loop makes it quadratic. Track a counter instead.- let mut out = String::new(); - for ch in input.chars() { - if out.chars().count() + 1 > max.saturating_sub(1) { - break; - } - out.push(ch); - } + let mut out = String::new(); + let mut count = 0usize; + for ch in input.chars() { + if count + 1 > max.saturating_sub(1) { + break; + } + out.push(ch); + count += 1; + }codex-rs/core/src/codex.rs (2)
393-409: Avoid rebuilding subagent inventory each turn; cache it
build_inventory_for_configon every model override/turn can be expensive on large trees. Consider caching the inventory (e.g., in Session or a small LRU keyed by cwd+subagents config) and only rebuilding when config/cwd/subagent settings change.
452-478: Produce deterministic ordering for “Available subagents”HashMap iteration order is nondeterministic. Sort by name for stable UX and reproducible prompts.
Apply:
- for record in inventory.subagents.values() { - let name = &record.definition.name; + let mut entries: Vec<_> = inventory.subagents.values().collect(); + entries.sort_by(|a, b| a.definition.name.cmp(&b.definition.name)); + for record in entries { + let name = &record.definition.name; let description = record.definition.description.trim(); if description.is_empty() { lines.push(format!(" - {name}")); } else { lines.push(format!(" - {name}: {description}")); } }codex-cli/src/ipc/subagents.ts (3)
486-525: Run-path handling is much sturdier; optional: parse JSON from stderr when stdout is empty.Some agents may print structured errors to stderr. Consider parsing whichever stream has content first.
- if (stdoutText) { + const text = stdoutText || stderrText; + if (text) { try { - const raw = JSON.parse(stdoutText) as RawRunPayload; - const message = raw.error ?? stderrText || stdoutText; + const raw = JSON.parse(text) as RawRunPayload; + const message = raw.error ?? (stderrText || stdoutText); const error = new Error( `codex agents run exited with code ${result.code}: ${message}`, ); (error as { stdout?: string }).stdout = result.stdout; (error as { stderr?: string }).stderr = result.stderr; (error as { exitCode?: number }).exitCode = result.code; throw error; } catch (err) { const errorLines = [ `codex agents run exited with code ${result.code} and produced invalid JSON output.`, - `stdout:\n${stdoutText}`, + `stdout:\n${stdoutText || "(empty)"}`, ]; if (stderrText.length > 0) { errorLines.push(`stderr:\n${stderrText}`); } errorLines.push(`parse error: ${(err as Error).message}`); const error = new Error(errorLines.join("\n\n")); (error as { cause?: unknown }).cause = err; (error as { stdout?: string }).stdout = result.stdout; (error as { stderr?: string }).stderr = result.stderr; (error as { exitCode?: number }).exitCode = result.code; throw error; } }Also applies to: 528-565
33-35: Unused type alias.
SubagentErrorPayloadisn’t referenced; consider removing until needed to keep the surface lean.
359-362: Consider PATH fallback before throwing.Return binaryName() as a PATH-resolvable fallback so spawn can use a globally installed
codex; codex-cli/bin only containscodex.js(no nativecodex-<triple>), so PATH fallback improves UX.Apply in: codex-cli/src/ipc/subagents.ts and codex-cli/src/ipc/hooks.ts.
- throw new Error( - "Unable to locate Codex CLI binary. Set CODEX_CLI_BIN to the compiled codex executable.", - ); + // Fallback: rely on PATH (Node will resolve .exe on win32 via PATHEXT) + return binaryName();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
codex-cli/src/ipc/subagents.ts(1 hunks)codex-rs/core/src/codex.rs(24 hunks)codex-rs/core/src/config.rs(11 hunks)codex-rs/core/src/openai_tools.rs(15 hunks)codex-rs/core/src/spawn.rs(1 hunks)codex-rs/core/src/subagents/discovery.rs(1 hunks)codex-rs/core/src/subagents/executor.rs(1 hunks)codex-rs/core/src/subagents/parser.rs(1 hunks)codex-rs/core/src/subagents/record.rs(1 hunks)codex-rs/core/src/subagents/runner.rs(1 hunks)codex-rs/core/src/subagents/service.rs(1 hunks)codex-rs/core/tests/suite/subagents_tool_restrictions.rs(1 hunks)codex-rs/exec/src/lib.rs(5 hunks)codex-rs/protocol/src/protocol.rs(4 hunks)codex-rs/tui/src/chatwidget.rs(9 hunks)codex-rs/tui/src/user_approval_widget.rs(14 hunks)specs/004-subagents-md/tasks.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- codex-rs/core/src/spawn.rs
- codex-rs/core/src/subagents/parser.rs
- codex-rs/core/src/subagents/service.rs
- codex-rs/protocol/src/protocol.rs
- codex-rs/core/src/subagents/runner.rs
🧰 Additional context used
📓 Path-based instructions (5)
codex-rs/core/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Place integration and regression tests under codex-rs/core/tests
Files:
codex-rs/core/tests/suite/subagents_tool_restrictions.rs
codex-rs/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/**/*.rs: Use rustfmt defaults for all Rust code
Prefer compact imports in Rust files
Inline variables directly inside format! braces rather than pre-binding temporaries
Files:
codex-rs/core/tests/suite/subagents_tool_restrictions.rscodex-rs/core/src/subagents/discovery.rscodex-rs/core/src/subagents/record.rscodex-rs/core/src/openai_tools.rscodex-rs/core/src/config.rscodex-rs/exec/src/lib.rscodex-rs/tui/src/chatwidget.rscodex-rs/core/src/codex.rscodex-rs/tui/src/user_approval_widget.rscodex-rs/core/src/subagents/executor.rs
codex-rs/**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use pretty_assertions::assert_eq in unit/integration tests for readable diffs
Files:
codex-rs/core/tests/suite/subagents_tool_restrictions.rs
codex-rs/**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust module names snake_case
Files:
codex-rs/core/src/subagents/discovery.rscodex-rs/core/src/subagents/record.rscodex-rs/core/src/openai_tools.rscodex-rs/core/src/config.rscodex-rs/exec/src/lib.rscodex-rs/tui/src/chatwidget.rscodex-rs/core/src/codex.rscodex-rs/tui/src/user_approval_widget.rscodex-rs/core/src/subagents/executor.rs
codex-rs/tui/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/tui/**/*.rs: In the TUI, use Ratatui Stylize helpers (e.g., "text".dim(), url.cyan().underlined()) instead of manual Style construction
Avoid hard-coded white foregrounds in the TUI; defer to theme defaults
Files:
codex-rs/tui/src/chatwidget.rscodex-rs/tui/src/user_approval_widget.rs
🧬 Code graph analysis (10)
codex-rs/core/tests/suite/subagents_tool_restrictions.rs (5)
codex-rs/core/src/subagents/runner.rs (1)
new(44-46)codex-rs/core/src/subagents/builder.rs (1)
new(15-21)codex-rs/core/src/subagents/config.rs (2)
new(25-35)enabled(37-39)codex-rs/core/src/subagents/invocation.rs (1)
new(17-29)codex-rs/core/src/subagents/definition.rs (2)
new(28-32)new(36-68)
codex-cli/src/ipc/subagents.ts (1)
codex-cli/bin/codex.js (2)
process(11-11)child(110-113)
codex-rs/core/src/subagents/discovery.rs (2)
codex-rs/core/src/subagents/parser.rs (1)
parse_definition(25-109)codex-rs/core/src/subagents/definition.rs (2)
new(28-32)new(36-68)
codex-rs/core/src/subagents/record.rs (3)
codex-rs/core/tests/subagents_config.rs (1)
definition(10-22)codex-rs/core/tests/subagents_invocation.rs (1)
definition(9-21)codex-cli/src/ipc/subagents.ts (1)
SubagentRecord(9-18)
codex-rs/core/src/openai_tools.rs (4)
codex-cli/src/ipc/subagents.ts (1)
SubagentRecord(9-18)codex-rs/core/src/subagents/config.rs (2)
default(15-21)enabled(37-39)codex-rs/core/tests/subagents_invocation.rs (1)
definition(9-21)codex-rs/core/src/subagents/record.rs (1)
from_definition(21-42)
codex-rs/exec/src/lib.rs (2)
codex-rs/core/src/codex.rs (1)
submit(290-298)codex-rs/core/src/codex_conversation.rs (1)
submit(21-24)
codex-rs/tui/src/chatwidget.rs (2)
codex-rs/tui/src/chatwidget/agent.rs (1)
spawn_agent(16-62)codex-rs/tui/src/user_approval_widget.rs (1)
new(194-303)
codex-rs/core/src/codex.rs (5)
codex-rs/core/src/subagents/service.rs (1)
build_inventory_for_config(28-33)codex-rs/core/src/subagents/executor.rs (1)
execute_subagent_invocation(144-282)codex-rs/core/src/openai_tools.rs (5)
inventory(120-124)inventory(126-130)inventory(132-143)from_inventory(115-152)new(309-354)codex-rs/core/src/conversation_manager.rs (3)
new(45-50)subagent_inventory(184-188)auth_manager(52-54)codex-rs/core/src/subagents/definition.rs (1)
normalize_name(88-115)
codex-rs/tui/src/user_approval_widget.rs (3)
codex-rs/tui/src/bottom_pane/approval_modal_view.rs (1)
new(22-28)codex-rs/tui/src/history_cell.rs (5)
new(137-142)new(511-513)new(810-822)line(1676-1679)new_user_approval_decision(782-784)codex-rs/tui/src/app_event_sender.rs (1)
new(12-14)
codex-rs/core/src/subagents/executor.rs (6)
codex-rs/core/src/codex.rs (3)
new(588-833)spawn(228-287)spawn(1981-1999)codex-rs/core/src/model_family.rs (1)
find_family_for_model(71-118)codex-rs/core/src/openai_model_info.rs (1)
get_model_info(30-70)codex-rs/core/src/subagents/config.rs (3)
new(25-35)disabled(41-43)enabled(37-39)codex-rs/core/src/subagents/invocation.rs (1)
new(17-29)codex-rs/core/src/subagents/record.rs (1)
from_definition(21-42)
🪛 markdownlint-cli2 (0.18.1)
specs/004-subagents-md/tasks.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (42)
specs/004-subagents-md/tasks.md (2)
32-38: Avoid “absolute paths” mandate; require workspace-anchored placeholders (/...).Keeps docs portable and avoids PII while staying precise.
- → Paths are absolute. + → Paths are workspace-anchored (<WORKSPACE>/...) and must not include personal absolute paths. @@ -## Format: `[ID] [P?] Description` +## Format: `[ID] [P?] Description` @@ -- Include the exact absolute file path(s) or command location for every task. +- Include the exact workspace-anchored path(s) (<WORKSPACE>/...) or command location for every task.
82-82: Confirmed: TUI filename is chatwidget.rs — no change required.
File exists at codex-rs/tui/src/chatwidget.rs; update only if you intentionally wanted chat_widget.rs.codex-rs/core/tests/suite/subagents_tool_restrictions.rs (3)
46-49: LGTM: precise assertion on ToolNotAllowed.The pattern match correctly validates the denied tool is “filesystem”.
20-20: Avoid env-specific absolute paths; use the stable dummy path used elsewhere.Switch to
/workspace/.codex/agents/shell-guard.mdto keep tests portable (per earlier review).Apply this diff:
- source_path: PathBuf::from("/workspace/codex/.codex/agents/shell-guard.md"), + source_path: PathBuf::from("/workspace/.codex/agents/shell-guard.md"),
27-31: Incorrect — Auto discovery doesn't trigger FS scans for this testSubagentBuilder::with_definition(...).build() uses only the supplied definitions; filesystem scanning (WalkDir) happens in discovery::discover_from_source and only when you call SubagentBuilder::discover_tree or service::build_inventory_for_config. The runner uses discovery mode only to require confirmation (session.confirmed); this test sets confirmed = true. No change required.
Likely an incorrect or invalid review comment.
codex-rs/tui/src/user_approval_widget.rs (8)
53-61: Subagent ApprovalRequest variant looks solidField set covers UX needs (id, name, description, tools, model, extras) and maps cleanly to protocol.
161-176: Good, minimal Subagent choicesY/N is appropriate for first iteration and aligns with non-interactive auto‑deny.
246-287: Clear subagent prompt contentWell-structured: optional description/model, requested vs allowed tools, extra instructions, and final prompt.
291-296: Correct option set wiringSUBAGENT_SELECT_OPTIONS correctly selected for Subagent requests.
331-341: Esc/Ctrl‑C path denies subagent — correctConsistent with other flows and non-interactive behavior.
Also applies to: 357-363
579-581: Title copy is clear“Run subagent?” matches the action and sets correct user expectation.
734-762: Test: approve path coverage is goodAsserts Op::SubagentApproval payload contents and decision.
764-792: Test: deny path coverage is goodCovers the negative path fully with payload assertions.
codex-rs/tui/src/chatwidget.rs (4)
138-139: Storing ConversationManager is fineUnderscore-prefixed field avoids unused warnings and keeps room for future features (forks, subagent sessions).
812-816: Correct spawn wiring with ConversationManagerPassing Arc into spawn_agent is consistent with agent.rs signature.
1275-1297: Submit guard + history sync looks correctAvoids empty submissions, sends UserInput with images/text, and mirrors text to transcript and core history.
1379-1379: Dispatch wiring for SubagentApprovalRequestEvent is routed to the handler in both live and replay paths. Good.
codex-rs/core/src/subagents/discovery.rs (2)
10-20: Public discovery types look goodDiscoverySource and SubagentSourceTree surfaces are clean and appropriately derived. The tree API is straightforward.
94-103: Prefer &Path over &PathBuf for detect_scope’s argumentIdiomatic API; avoids unnecessary type coupling and reduces conversions at call sites. Repo search found only the function definition (no internal callers) — safe internally but this is a public API, so verify external callers before changing.
Apply this diff:
-pub fn detect_scope(path: &PathBuf) -> SubagentScope { +pub fn detect_scope(path: &Path) -> SubagentScope {codex-rs/core/src/subagents/record.rs (1)
21-27: Validation precedence fixed (LGTM).Invalid definitions now take precedence over Disabled as previously requested.
codex-rs/core/src/config.rs (3)
203-205: Config surface wiring for subagents (LGTM).Runtime
Confignow carriessubagents: SubagentConfig; this aligns with TOML parsing added below.
842-884: TOML surface and conversions for subagents (LGTM).
SubagentsToml+ conversions are clean. Trimming and empty-string filtering fordefault_modelis a nice touch.
1142-1147: Defaulting behavior is sensible.Gracefully falls back to
SubagentConfig::default()when config is absent.codex-rs/core/src/openai_tools.rs (2)
805-809: Signature change: verify all call sites pass the new third argument.
get_openai_toolsnow requiressubagent_tool. Confirm all callers updated.#!/bin/bash # Find get_openai_tools calls and classify by arg count (2 vs 3). ast-grep --pattern $'get_openai_tools($_, $_)' || true echo '---' ast-grep --pattern $'get_openai_tools($_, $_, $_)' || true # Also show surrounding context to fix stragglers. rg -nP -C3 --type=rust '\bget_openai_tools\s*\('
581-588: Remove “confirmed” parameter from invoke_subagent tool (model must not self-approve).Exposing
confirmedlets the model bypass user approval in Auto discovery mode.- properties.insert( - "confirmed".to_string(), - JsonSchema::Boolean { - description: Some( - "Set to true after the user explicitly approves invoking the subagent when auto discovery is enabled.".to_string(), - ), - }, - );Follow-up: ensure the handler ignores any incoming
confirmedfield and tests assert it is not present.codex-rs/core/src/codex.rs (15)
80-80: LGTM: subagents imports and protocol wiringNew imports for Subagent tool registration, inventory, runner, executor, and approval protocol look correct and cohesive.
Also applies to: 126-128, 146-155
320-333: State: add pending_subagent_approvals map — LGTMGood separation of subagent approval channels from other approvals.
380-383: Thread subagent state through TurnContext — LGTMCarrying inventory, tool registration, and config per turn is the right choice.
411-413: Guidance block markers — LGTM
414-450: Merge/update guidance into user instructions — LGTMRobust replacement/append behavior with clear sentinel markers.
538-541: Slash-command turn: subagent wiring and guidance merge — LGTMCorrectly rebuilds client, computes tooling, merges guidance into instructions, and threads into TurnContext.
Also applies to: 560-567, 568-581
607-611: Session init: subagents + guidance — LGTMBuilding inventory once and merging guidance into initial instructions is sound. TurnContext fields are set consistently.
Also applies to: 610-611, 722-745
2448-2451: Subagent approval op uses id-based routing — LGTMRoutes on request id (not name), matching the session map key.
2625-2628: Preserve subagent state across Plan Mode transitions — LGTMAlso applies to: 2669-2672, 2715-2718
3230-3231: Wire subagent tool into tools config — LGTMPassing
turn_context.subagent_tool.as_ref()enables the function-call path.
967-991: Subagent approval plumbing — LGTMGood: key by request id, include id in payload, remove entry on notify, and helper to clear a specific id.
Also applies to: 992-1015
1621-1629: Clear pending_subagent_approvals on interrupt to avoid leaks/dangling channelsInterrupt should purge subagent approval waiters, like other approvals; otherwise channels dangle until drop.
fn interrupt_task(&self) { info!("interrupt received: abort current task, if any"); let mut state = self.state.lock_unchecked(); state.pending_approvals.clear(); + state + .pending_subagent_approvals + .clear(); state.pending_input.clear(); if let Some(task) = state.current_task.take() { task.abort(TurnAbortReason::Interrupted); } }
3780-4028: Subagent invocation pipeline — LGTM overallValidation, approval request flow, invocation prep, and execution result shaping are clean and match the executor’s contract.
5154-5173: Tests: respect subagent wiring in fixtures — LGTMTest helper builds TurnContext with subagent fields and merged guidance.
3848-3851: Don’t auto-confirm on Manual discovery; gate only on explicit args.confirmedAuto-confirming when
discovery == Manualbypasses the runner’s confirmation guard. The generic function-call path should only setconfirmedwhen the caller explicitly requested it.- if args.confirmed || matches!(subagent_config.discovery, SubagentDiscoveryMode::Manual) - { - session = session.confirmed(); - } + if args.confirmed { + session = session.confirmed(); + }codex-cli/src/ipc/subagents.ts (2)
80-101: JSON.parse now guarded with actionable diagnostics — nice.This addresses prior feedback to fail gracefully on malformed stdout.
123-156: Show path: enforce success, error on empty, and guard parse — solid.This implements the recommended validations and error reporting.
| let updated_config = Arc::new(updated_config); | ||
| let client = ModelClient::new( | ||
| Arc::new(updated_config), | ||
| updated_config.clone(), | ||
| auth_manager, |
There was a problem hiding this comment.
Persist per-turn user_instructions changes so the model sees them
You recompute/merge guidance but don’t record it into history, so subsequent turns won’t see updated instructions (build_initial_context is only used once). Record a UserInstructions item when text changes.
- let new_turn_context = TurnContext {
+ let new_turn_context = TurnContext {
client,
tools_config,
- user_instructions,
+ user_instructions,
base_instructions: prev.base_instructions.clone(),
approval_policy: new_approval_policy,
sandbox_policy: new_sandbox_policy.clone(),
shell_environment_policy: prev.shell_environment_policy.clone(),
cwd: new_cwd.clone(),
is_review_mode: false,
subagent_inventory,
subagent_tool,
subagent_config,
};
- // Install the new persistent context for subsequent tasks/turns.
- turn_context = Arc::new(new_turn_context);
+ // Install the new persistent context for subsequent tasks/turns.
+ let changed_instructions =
+ new_turn_context.user_instructions != prev.user_instructions;
+ turn_context = Arc::new(new_turn_context);
+ if changed_instructions {
+ if let Some(text) = turn_context.user_instructions.clone() {
+ sess.record_conversation_items(&[UserInstructions::new(text).into()]).await;
+ }
+ }Also applies to: 2177-2184, 2185-2198, 2200-2202
| // Build a new client with per‑turn reasoning settings. | ||
| // Reuse the same provider and session id; auth defaults to env/API key. | ||
| let per_turn_config = Arc::new(per_turn_config); | ||
| let client = ModelClient::new( | ||
| Arc::new(per_turn_config), | ||
| per_turn_config.clone(), | ||
| auth_manager, |
There was a problem hiding this comment.
Also persist per-turn user_instructions changes in UserTurn path
Same issue as OverrideTurnContext: merged guidance isn’t recorded into history for subsequent turns.
- let fresh_turn_context = TurnContext {
+ let fresh_turn_context = TurnContext {
client,
tools_config: ToolsConfig::new(&ToolsConfigParams {
model_family: &model_family,
approval_policy,
sandbox_policy: sandbox_policy.clone(),
include_plan_tool: config.include_plan_tool,
include_apply_patch_tool: config.include_apply_patch_tool,
include_web_search_request: config.tools_web_search_request,
use_streamable_shell_tool: config
.use_experimental_streamable_shell_tool,
include_view_image_tool: config.include_view_image_tool,
experimental_unified_exec_tool: config
.use_experimental_unified_exec_tool,
}),
- user_instructions,
+ user_instructions,
base_instructions: turn_context.base_instructions.clone(),
approval_policy,
sandbox_policy,
shell_environment_policy: turn_context.shell_environment_policy.clone(),
cwd,
is_review_mode: false,
subagent_inventory,
subagent_tool,
subagent_config,
};
@@
- // Install the new persistent context for subsequent tasks/turns.
- turn_context = Arc::new(fresh_turn_context);
+ // Install the new persistent context for subsequent tasks/turns.
+ let changed_instructions =
+ fresh_turn_context.user_instructions != turn_context.user_instructions;
+ turn_context = Arc::new(fresh_turn_context);
+ if changed_instructions {
+ if let Some(text) = turn_context.user_instructions.clone() {
+ sess.record_conversation_items(&[UserInstructions::new(text).into()]).await;
+ }
+ }Also applies to: 2384-2391, 2407-2417, 2427-2429
| let rx = sess | ||
| .request_subagent_approval(sub_id.clone(), payload) | ||
| .await; | ||
| let decision = rx.await.unwrap_or_default(); | ||
| sess.clear_pending_subagent_approval(&sub_id); | ||
| match decision { |
There was a problem hiding this comment.
Default approval decision should deny, not depend on Default impl
If the oneshot sender is dropped, unwrap_or_default() relies on the enum’s Default (which could change). Be explicit and safe.
- let decision = rx.await.unwrap_or_default();
+ let decision = rx.await.unwrap_or(SubagentApprovalDecision::Denied);📝 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.
| let rx = sess | |
| .request_subagent_approval(sub_id.clone(), payload) | |
| .await; | |
| let decision = rx.await.unwrap_or_default(); | |
| sess.clear_pending_subagent_approval(&sub_id); | |
| match decision { | |
| let rx = sess | |
| .request_subagent_approval(sub_id.clone(), payload) | |
| .await; | |
| let decision = rx.await.unwrap_or(SubagentApprovalDecision::Denied); | |
| sess.clear_pending_subagent_approval(&sub_id); | |
| match decision { |
🤖 Prompt for AI Agents
In codex-rs/core/src/codex.rs around lines 3887 to 3892, the code uses
rx.await.unwrap_or_default() which relies on the enum’s Default and can silently
change behavior if Default changes; instead treat a dropped oneshot as an
explicit denial. Replace the unwrap_or_default() usage with an explicit map_or
or unwrap_or that returns Decision::Deny (e.g., rx.await.map_or(Decision::Deny,
|d| d) or rx.await.unwrap_or(Decision::Deny)), keeping the rest of the flow the
same.
| for entry in WalkDir::new(&root) | ||
| .follow_links(false) | ||
| .into_iter() | ||
| .filter_map(Result::ok) | ||
| { | ||
| if !entry.file_type().is_file() { | ||
| continue; | ||
| } | ||
|
|
||
| let path = entry.into_path(); | ||
| if !is_markdown(&path) { | ||
| continue; | ||
| } | ||
|
|
||
| match fs::read_to_string(&path) { | ||
| Ok(contents) => match parse_definition(&path, &contents, scope) { | ||
| Ok(definition) => outcome.definitions.push(definition), | ||
| Err(SubagentParserError::ParseError(message)) => { | ||
| outcome.events.push(DiscoveryEvent { | ||
| message: format!( | ||
| "Failed to parse subagent definition at {}: {}", | ||
| path.display(), | ||
| message | ||
| ), | ||
| }); | ||
| } | ||
| }, | ||
| Err(error) => outcome.events.push(DiscoveryEvent { | ||
| message: format!( | ||
| "Failed to read subagent definition at {}: {}", | ||
| path.display(), | ||
| error | ||
| ), | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t drop WalkDir errors; also sort for deterministic traversal
filter_map(Result::ok) silently discards IO/permission errors, losing diagnostics and potentially missing definitions. Sorting stabilizes traversal order for reproducible results/tests.
Apply this diff:
- for entry in WalkDir::new(&root)
- .follow_links(false)
- .into_iter()
- .filter_map(Result::ok)
- {
- if !entry.file_type().is_file() {
- continue;
- }
-
- let path = entry.into_path();
- if !is_markdown(&path) {
- continue;
- }
-
- match fs::read_to_string(&path) {
- Ok(contents) => match parse_definition(&path, &contents, scope) {
- Ok(definition) => outcome.definitions.push(definition),
- Err(SubagentParserError::ParseError(message)) => {
- outcome.events.push(DiscoveryEvent {
- message: format!(
- "Failed to parse subagent definition at {}: {}",
- path.display(),
- message
- ),
- });
- }
- },
- Err(error) => outcome.events.push(DiscoveryEvent {
- message: format!(
- "Failed to read subagent definition at {}: {}",
- path.display(),
- error
- ),
- }),
- }
- }
+ for entry in WalkDir::new(&root)
+ .follow_links(false)
+ .sort_by(|a, b| a.path().cmp(b.path()))
+ {
+ match entry {
+ Ok(entry) => {
+ if !entry.file_type().is_file() {
+ continue;
+ }
+ let path = entry.into_path();
+ if !is_markdown(&path) {
+ continue;
+ }
+ match fs::read_to_string(&path) {
+ Ok(contents) => match parse_definition(&path, &contents, scope) {
+ Ok(definition) => outcome.definitions.push(definition),
+ Err(error) => {
+ outcome.events.push(DiscoveryEvent {
+ message: format!(
+ "Failed to parse subagent definition at {}: {}",
+ path.display(),
+ error
+ ),
+ });
+ }
+ },
+ Err(error) => outcome.events.push(DiscoveryEvent {
+ message: format!(
+ "Failed to read subagent definition at {}: {}",
+ path.display(),
+ error
+ ),
+ }),
+ }
+ }
+ Err(err) => outcome.events.push(DiscoveryEvent {
+ message: format!("Failed to traverse {}: {}", root.display(), err),
+ }),
+ }
+ }📝 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.
| for entry in WalkDir::new(&root) | |
| .follow_links(false) | |
| .into_iter() | |
| .filter_map(Result::ok) | |
| { | |
| if !entry.file_type().is_file() { | |
| continue; | |
| } | |
| let path = entry.into_path(); | |
| if !is_markdown(&path) { | |
| continue; | |
| } | |
| match fs::read_to_string(&path) { | |
| Ok(contents) => match parse_definition(&path, &contents, scope) { | |
| Ok(definition) => outcome.definitions.push(definition), | |
| Err(SubagentParserError::ParseError(message)) => { | |
| outcome.events.push(DiscoveryEvent { | |
| message: format!( | |
| "Failed to parse subagent definition at {}: {}", | |
| path.display(), | |
| message | |
| ), | |
| }); | |
| } | |
| }, | |
| Err(error) => outcome.events.push(DiscoveryEvent { | |
| message: format!( | |
| "Failed to read subagent definition at {}: {}", | |
| path.display(), | |
| error | |
| ), | |
| }), | |
| } | |
| } | |
| for entry in WalkDir::new(&root) | |
| .follow_links(false) | |
| .sort_by(|a, b| a.path().cmp(b.path())) | |
| { | |
| match entry { | |
| Ok(entry) => { | |
| if !entry.file_type().is_file() { | |
| continue; | |
| } | |
| let path = entry.into_path(); | |
| if !is_markdown(&path) { | |
| continue; | |
| } | |
| match fs::read_to_string(&path) { | |
| Ok(contents) => match parse_definition(&path, &contents, scope) { | |
| Ok(definition) => outcome.definitions.push(definition), | |
| Err(error) => { | |
| outcome.events.push(DiscoveryEvent { | |
| message: format!( | |
| "Failed to parse subagent definition at {}: {}", | |
| path.display(), | |
| error | |
| ), | |
| }); | |
| } | |
| }, | |
| Err(error) => outcome.events.push(DiscoveryEvent { | |
| message: format!( | |
| "Failed to read subagent definition at {}: {}", | |
| path.display(), | |
| error | |
| ), | |
| }), | |
| } | |
| } | |
| Err(err) => outcome.events.push(DiscoveryEvent { | |
| message: format!("Failed to traverse {}: {}", root.display(), err), | |
| }), | |
| } | |
| } |
🤖 Prompt for AI Agents
In codex-rs/core/src/subagents/discovery.rs around lines 54 to 89, the current
use of WalkDir::into_iter().filter_map(Result::ok) drops IO/permission errors
and yields non-deterministic traversal order; change logic to collect iterator
results, push a DiscoveryEvent for any Err(entry_error) instead of discarding
it, then collect the Ok(entries) into a Vec, sort that Vec by the entry path
(e.g., entry.path().to_string_lossy()) to ensure deterministic traversal, and
then iterate the sorted entries performing the existing file/type/markdown
checks and read/parse handling as before.
| match codex.next_event().await { | ||
| Ok(event) => transcript.push(event), | ||
| Err(err) => { | ||
| return Err(SubagentInvocationError::ExecutionFailed(format!( | ||
| "failed to initialize subagent session: {err}" | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a timeout for the initial session event to avoid indefinite hangs.
codex.next_event().await can block forever if the session never emits the first event.
- match codex.next_event().await {
- Ok(event) => transcript.push(event),
- Err(err) => {
- return Err(SubagentInvocationError::ExecutionFailed(format!(
- "failed to initialize subagent session: {err}"
- )));
- }
- }
+ let first = tokio::time::timeout(std::time::Duration::from_secs(300), codex.next_event())
+ .await
+ .map_err(|_| {
+ SubagentInvocationError::ExecutionFailed(
+ "timed out initializing subagent session after 300s".to_string(),
+ )
+ })?
+ .map_err(|err| {
+ SubagentInvocationError::ExecutionFailed(format!(
+ "failed to initialize subagent session: {err}"
+ ))
+ })?;
+ transcript.push(first);📝 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.
| match codex.next_event().await { | |
| Ok(event) => transcript.push(event), | |
| Err(err) => { | |
| return Err(SubagentInvocationError::ExecutionFailed(format!( | |
| "failed to initialize subagent session: {err}" | |
| ))); | |
| } | |
| } | |
| let first = tokio::time::timeout(std::time::Duration::from_secs(300), codex.next_event()) | |
| .await | |
| .map_err(|_| { | |
| SubagentInvocationError::ExecutionFailed( | |
| "timed out initializing subagent session after 300s".to_string(), | |
| ) | |
| })? | |
| .map_err(|err| { | |
| SubagentInvocationError::ExecutionFailed(format!( | |
| "failed to initialize subagent session: {err}" | |
| )) | |
| })?; | |
| transcript.push(first); |
🤖 Prompt for AI Agents
In codex-rs/core/src/subagents/executor.rs around lines 180 to 187, the await on
codex.next_event() can hang indefinitely; wrap the await in a timeout (e.g.,
tokio::time::timeout) using a reasonable Duration (or a configurable timeout),
handle the Timeout error by returning SubagentInvocationError::ExecutionFailed
with a clear timeout message, and propagate any real errors from next_event() as
before; add the necessary tokio::time import and ensure the session is cleaned
up/aborted if the timeout triggers.
| if let Some(op) = auto_deny_subagent_request(&event) { | ||
| let subagent = match &op { | ||
| Op::SubagentApproval { name, .. } => name.clone(), | ||
| _ => String::new(), | ||
| }; | ||
| warn!("Auto-denying subagent '{subagent}' in non-interactive mode"); | ||
| if let Err(err) = conversation.submit(op).await { | ||
| error!("Failed to auto-deny subagent invocation: {err:?}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Auto‑deny is missing in the image pre‑wait loop; can deadlock
During the initial image handshake (Lines 289–301), events are polled directly via conversation.next_event() without auto‑deny. A SubagentApprovalRequest at this stage will block forever waiting for an approval op.
Add the same auto‑deny handling inside the image wait loop:
// Inside the while-let loop that waits for TaskComplete for images:
while let Ok(event) = conversation.next_event().await {
if let Some(op) = auto_deny_subagent_request(&event) {
let subagent = match &op {
Op::SubagentApproval { name, .. } => name.clone(),
_ => String::new(),
};
warn!("Auto-denying subagent '{subagent}' in non-interactive mode");
if let Err(err) = conversation.submit(op).await {
error!("Failed to auto-deny subagent invocation: {err:?}");
}
}
if event.id == initial_images_event_id
&& matches!(
event.msg,
EventMsg::TaskComplete(TaskCompleteEvent { last_agent_message: _ })
)
{
break;
}
}Note: Longer‑term, avoid consuming next_event() in two places (spawned loop and main thread) to prevent racy event routing; prefer routing all events through the spawned loop and waiting on rx for the specific TaskComplete.
🤖 Prompt for AI Agents
In codex-rs/exec/src/lib.rs around lines 248 to 257, the image pre-wait loop
that polls conversation.next_event() lacks the auto-deny handling and can
deadlock on SubagentApprovalRequest; add the same auto_deny_subagent_request()
handling used earlier: for each received event call
auto_deny_subagent_request(&event), and if it returns Some(op) extract the
subagent name from Op::SubagentApproval for the warn log, call
conversation.submit(op).await, and log any submission error; ensure this logic
runs before checking for the TaskComplete event so approval requests are
auto-denied immediately.
| fn auto_deny_subagent_request(event: &Event) -> Option<Op> { | ||
| if let EventMsg::SubagentApprovalRequest(request) = &event.msg { | ||
| Some(Op::SubagentApproval { | ||
| id: event.id.clone(), | ||
| name: request.subagent.clone(), | ||
| decision: SubagentApprovalDecision::Denied, | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Correlate approval using request.id, not event.id
Tie the SubagentApproval to the SubagentApprovalRequestEvent.id to avoid mismatches if Event.id differs.
- Some(Op::SubagentApproval {
- id: event.id.clone(),
+ Some(Op::SubagentApproval {
+ id: request.id.clone(),
name: request.subagent.clone(),
decision: SubagentApprovalDecision::Denied,
})📝 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.
| fn auto_deny_subagent_request(event: &Event) -> Option<Op> { | |
| if let EventMsg::SubagentApprovalRequest(request) = &event.msg { | |
| Some(Op::SubagentApproval { | |
| id: event.id.clone(), | |
| name: request.subagent.clone(), | |
| decision: SubagentApprovalDecision::Denied, | |
| }) | |
| } else { | |
| None | |
| } | |
| } | |
| fn auto_deny_subagent_request(event: &Event) -> Option<Op> { | |
| if let EventMsg::SubagentApprovalRequest(request) = &event.msg { | |
| Some(Op::SubagentApproval { | |
| id: request.id.clone(), | |
| name: request.subagent.clone(), | |
| decision: SubagentApprovalDecision::Denied, | |
| }) | |
| } else { | |
| None | |
| } | |
| } |
🤖 Prompt for AI Agents
In codex-rs/exec/src/lib.rs around lines 325 to 336, the
auto_deny_subagent_request function currently ties the SubagentApproval Op to
event.id which can mismatch; change the Op so its id uses the
SubagentApprovalRequest's own id (request.id) instead of event.id, preserving
the rest of the fields (name and decision) so approvals correlate to the
original request identifier.
| fn send_subagent_decision(&mut self, decision: SubagentApprovalDecision) { | ||
| if let ApprovalRequest::Subagent { id, name, .. } = &self.approval_request { | ||
| let subagent_name = name.clone(); | ||
|
|
||
| let spans: Vec<Span<'static>> = match decision { | ||
| SubagentApprovalDecision::Approved => vec![ | ||
| "✔ ".fg(Color::Green), | ||
| "You ".into(), | ||
| "approved".bold(), | ||
| " invoking subagent ".into(), | ||
| subagent_name.clone().dim(), | ||
| ], | ||
| SubagentApprovalDecision::Denied => vec![ | ||
| "✗ ".fg(Color::Red), | ||
| "You ".into(), | ||
| "canceled".bold(), | ||
| " the subagent ".into(), | ||
| subagent_name.clone().dim(), | ||
| ], | ||
| }; | ||
|
|
||
| self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( | ||
| history_cell::new_user_approval_decision(vec![Line::from(spans)]), | ||
| ))); | ||
|
|
||
| self.app_event_tx | ||
| .send(AppEvent::CodexOp(Op::SubagentApproval { | ||
| id: id.clone(), | ||
| name: subagent_name, | ||
| decision, | ||
| })); | ||
| self.done = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Unify copy: use “denied” for consistency with decision enum
Small wording tweak keeps UX consistent with SubagentApprovalDecision::Denied.
Apply this diff:
SubagentApprovalDecision::Denied => vec![
"✗ ".fg(Color::Red),
"You ".into(),
- "canceled".bold(),
- " the subagent ".into(),
+ "denied".bold(),
+ " invoking subagent ".into(),
subagent_name.clone().dim(),
],📝 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.
| fn send_subagent_decision(&mut self, decision: SubagentApprovalDecision) { | |
| if let ApprovalRequest::Subagent { id, name, .. } = &self.approval_request { | |
| let subagent_name = name.clone(); | |
| let spans: Vec<Span<'static>> = match decision { | |
| SubagentApprovalDecision::Approved => vec![ | |
| "✔ ".fg(Color::Green), | |
| "You ".into(), | |
| "approved".bold(), | |
| " invoking subagent ".into(), | |
| subagent_name.clone().dim(), | |
| ], | |
| SubagentApprovalDecision::Denied => vec![ | |
| "✗ ".fg(Color::Red), | |
| "You ".into(), | |
| "canceled".bold(), | |
| " the subagent ".into(), | |
| subagent_name.clone().dim(), | |
| ], | |
| }; | |
| self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( | |
| history_cell::new_user_approval_decision(vec![Line::from(spans)]), | |
| ))); | |
| self.app_event_tx | |
| .send(AppEvent::CodexOp(Op::SubagentApproval { | |
| id: id.clone(), | |
| name: subagent_name, | |
| decision, | |
| })); | |
| self.done = true; | |
| } | |
| } | |
| fn send_subagent_decision(&mut self, decision: SubagentApprovalDecision) { | |
| if let ApprovalRequest::Subagent { id, name, .. } = &self.approval_request { | |
| let subagent_name = name.clone(); | |
| let spans: Vec<Span<'static>> = match decision { | |
| SubagentApprovalDecision::Approved => vec![ | |
| "✔ ".fg(Color::Green), | |
| "You ".into(), | |
| "approved".bold(), | |
| " invoking subagent ".into(), | |
| subagent_name.clone().dim(), | |
| ], | |
| SubagentApprovalDecision::Denied => vec![ | |
| "✗ ".fg(Color::Red), | |
| "You ".into(), | |
| "denied".bold(), | |
| " invoking subagent ".into(), | |
| subagent_name.clone().dim(), | |
| ], | |
| }; | |
| self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( | |
| history_cell::new_user_approval_decision(vec![Line::from(spans)]), | |
| ))); | |
| self.app_event_tx | |
| .send(AppEvent::CodexOp(Op::SubagentApproval { | |
| id: id.clone(), | |
| name: subagent_name, | |
| decision, | |
| })); | |
| self.done = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
codex-rs/tui/src/user_approval_widget.rs around lines 396-429: the Denied branch
currently uses the wording "canceled" which is inconsistent with the
SubagentApprovalDecision::Denied enum and the requested UX; replace the
"canceled" token with "denied" (keeping same formatting/bold and surrounding
fragments) so the Denied spans read e.g. "✗ " (red), "You ", "denied".bold(), "
the subagent ", subagent_name.dim(), and keep the rest of the logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.codex/agents/commit-agent.md (8)
6-7: Call out non-interactive (no TTY) safety: require explicit approvalDocument that in non-interactive contexts the agent must not execute git commands; it should propose the commands and auto-deny until an approval token/flag is provided.
Apply:
-Use this playbook whenever the user asks for help committing work: +Use this playbook whenever the user asks for help committing work. In non-interactive (no TTY) contexts, do not execute; propose commands and require explicit approval:
8-8: Prefer porcelain output for reliable parsingUse stable porcelain flags to capture state and tracking info.
-1. Run `git status --short` and `git status -sb` to capture the working tree state and upstream tracking info. +1. Run `git status --porcelain=v1 -b` (or `--porcelain=v2 -b`) to capture working tree and upstream tracking info in a stable format.
9-9: Offer interactive staging and safe unstage guidanceRecommend patch mode and an escape hatch to unstage mistakes.
-2. Clarify which paths to include; stage them explicitly with `git add <path>` (avoid `git add .` unless the user insists). +2. Clarify which paths to include; stage them explicitly with `git add <path>` (avoid `git add .` unless the user insists). Offer `git add -p` for interactive staging and `git restore --staged <path>` to unstage if needed.
11-11: Surface pre-commit hooks and repo-native checksIf the repo has pre-commit hooks or a standard test command, prefer those first; avoid
--no-verifyunless explicitly requested.-4. If the user mentions required checks (formatting, tests, lint), run them before committing and report the results. +4. If the repo defines checks (e.g., pre-commit hooks, package/test scripts), run them before committing and report results (e.g., `pre-commit run --all-files`, `npm test`, `cargo test`). Do not use `--no-verify` unless explicitly requested.
12-12: Commit message conventions (50/72 + optional Conventional Commits)Briefly encode best practices to drive consistent history.
-5. Draft an imperative, single-line commit summary plus optional wrapped body that explains the why; surface it to the user for approval or edits. +5. Draft an imperative, single-line summary (~50 chars), with an optional body wrapped at ~72 chars explaining the why. Optionally follow Conventional Commits (e.g., `feat: ...`, `fix: ...`). Surface for approval/edits.
13-13: Show safe, exact commit command including quoting and signingEnsure safe quoting for multi-line messages; respect signing if configured/requested.
-6. Echo the exact `git commit` command you intend to run (include `--amend`, `--no-verify`, etc. only if requested), wait for confirmation, then execute it. +6. Echo the exact `git commit` command you intend to run (include `--amend`, `--no-verify`, etc. only if requested), wait for confirmation, then execute it. Use safe quoting and multi-line flags, e.g.: + - `git commit -m "Title" -m "Body..."` (avoid shell interpolation) + - Include `-S` if signing is configured/requested.
14-14: Prefer precise post-commit outputsUse rev-parse for hash and show concise stats.
-7. After committing, show the short commit hash and message using `git log -1 --stat` and remind the user of any follow-up actions (push, additional commits, etc.). +7. After committing, show `git rev-parse --short HEAD` plus `git show -1 --oneline --stat`. Remind about follow-ups (e.g., `git push -u origin <current-branch>`).
16-16: Expand conflict/special states detectionAlso gate on detached HEAD, in-progress cherry-pick/revert, submodules/LFS, and stashes.
-If the working tree has conflicts, rebase/merge state, or pending stash entries, pause and alert the user before proceeding. +If the working tree has conflicts, rebase/merge/cherry-pick/revert in progress, a detached HEAD, submodule/LFS locks, or pending stash entries, pause and alert the user before proceeding. Offer next steps (e.g., `git rebase --continue`, `git merge --continue`, `git cherry-pick --abort`, review `git stash list`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.codex/agents/commit-agent.md(1 hunks)codex-rs/cli/src/agents.rs(1 hunks)codex-rs/cli/src/main.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- codex-rs/cli/src/agents.rs
- codex-rs/cli/src/main.rs
🔇 Additional comments (2)
.codex/agents/commit-agent.md (2)
1-17: Solid, actionable playbookClear steps, good safety notes (explicit staging, confirmation before commit). With the small refinements above, this will be robust across CLI/TUI and non-interactive flows.
1-4: Confirm frontmatter schema matches subagent discovery requirements.codex/agents/commit-agent.md frontmatter currently has only "name" and "description" — add/align discovery keys used by other agents (e.g., type/kind, schema_version, capabilities) so inventory/invocation doesn't skip this file.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
codex-rs/core/src/subagents/discovery.rs (2)
7-12: Compact imports for definition/parser modulesConsolidate imports per guidelines.
-use crate::subagents::definition::SubagentDefinition; -use crate::subagents::definition::SubagentScope; -use crate::subagents::inventory::DiscoveryEvent; -use crate::subagents::parser::SubagentParserError; -use crate::subagents::parser::parse_definition; +use crate::subagents::definition::{SubagentDefinition, SubagentScope}; +use crate::subagents::inventory::DiscoveryEvent; +use crate::subagents::parser::{parse_definition, SubagentParserError};
105-114: Generalize parameter type to &PathBroaden API to accept any Path-like type.
-pub fn detect_scope(path: &PathBuf) -> SubagentScope { +pub fn detect_scope(path: &Path) -> SubagentScope {Note: Update call sites accordingly.
codex-cli/src/commands/agents/list.ts (2)
29-33: Stable-sort the rendered tables by name for readability.Improves determinism and UX when many agents exist.
Apply this diff:
- const rows = result.subagents.map((record) => formatRecord(record)); + const rows = result.subagents + .map((record) => formatRecord(record)) + .sort((a, b) => String(a.Name).localeCompare(String(b.Name))); @@ - const rows = result.invalid.map((record) => ({ + const rows = result.invalid.map((record) => ({ ...formatRecord(record), Errors: record.validationErrors.join("; ") || "(none)", - })); + })) + .sort((a, b) => String(a.Name).localeCompare(String(b.Name)));Also applies to: 35-42
45-46: Narrow return type to Record<string, string>.All fields are rendered as strings; this tightens the type and improves editor hints.
Apply this diff:
-function formatRecord(record: SubagentRecord): Record<string, unknown> { +function formatRecord(record: SubagentRecord): Record<string, string> {codex-cli/src/ipc/subagents.ts (1)
530-556: Prefer parsing JSON from stderr when stdout is empty on failures.Some CLIs emit structured error JSON to stderr. Parsing only stdout may miss useful messages.
Apply this diff:
- if (stdoutText) { + const jsonText = stdoutText || stderrText; + if (jsonText) { try { - const raw = JSON.parse(stdoutText) as RawRunPayload; - const message = raw.error ?? stderrText || stdoutText; + const raw = JSON.parse(jsonText) as RawRunPayload; + const message = raw.error ?? stderrText || stdoutText || "(no output)"; const error = new Error( `codex agents run exited with code ${result.code}: ${message}`, ); (error as { stdout?: string }).stdout = result.stdout; (error as { stderr?: string }).stderr = result.stderr; (error as { exitCode?: number }).exitCode = result.code; throw error; } catch (err) { const errorLines = [ - `codex agents run exited with code ${result.code} and produced invalid JSON output.`, - `stdout:\n${stdoutText}`, + `codex agents run exited with code ${result.code} and produced invalid JSON output.`, + `stdout:\n${stdoutText || "(empty)"}`, ]; if (stderrText.length > 0) { errorLines.push(`stderr:\n${stderrText}`); } errorLines.push(`parse error: ${(err as Error).message}`); const error = new Error(errorLines.join("\n\n")); (error as { cause?: unknown }).cause = err; (error as { stdout?: string }).stdout = result.stdout; (error as { stderr?: string }).stderr = result.stderr; (error as { exitCode?: number }).exitCode = result.code; throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
codex-cli/src/commands/agents/list.ts(1 hunks)codex-cli/src/commands/agents/show.ts(1 hunks)codex-cli/src/ipc/subagents.ts(1 hunks)codex-rs/cli/src/agents.rs(1 hunks)codex-rs/core/src/openai_tools.rs(15 hunks)codex-rs/core/src/subagents/discovery.rs(1 hunks)codex-rs/core/src/subagents/runner.rs(1 hunks)codex-rs/core/tests/subagents_invocation.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- codex-rs/cli/src/agents.rs
- codex-rs/core/tests/subagents_invocation.rs
- codex-cli/src/commands/agents/show.ts
- codex-rs/core/src/subagents/runner.rs
🧰 Additional context used
📓 Path-based instructions (2)
codex-rs/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/**/*.rs: Use rustfmt defaults for all Rust code
Prefer compact imports in Rust files
Inline variables directly inside format! braces rather than pre-binding temporaries
Files:
codex-rs/core/src/openai_tools.rscodex-rs/core/src/subagents/discovery.rs
codex-rs/**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust module names snake_case
Files:
codex-rs/core/src/openai_tools.rscodex-rs/core/src/subagents/discovery.rs
🧬 Code graph analysis (4)
codex-rs/core/src/openai_tools.rs (4)
codex-rs/cli/src/agents.rs (3)
default(50-52)from(357-368)from(381-393)codex-rs/core/src/subagents/config.rs (2)
default(15-21)enabled(37-39)codex-rs/core/tests/subagents_invocation.rs (1)
definition(9-21)codex-rs/core/src/subagents/record.rs (1)
from_definition(21-42)
codex-cli/src/commands/agents/list.ts (1)
codex-cli/src/ipc/subagents.ts (4)
ListOptions(60-64)SubagentListPayload(20-23)fetchSubagentList(66-102)SubagentRecord(9-18)
codex-cli/src/ipc/subagents.ts (2)
codex-cli/bin/codex.js (2)
process(11-11)child(110-113)codex-cli/scripts/dev_build.mjs (1)
cliRoot(6-6)
codex-rs/core/src/subagents/discovery.rs (2)
codex-rs/core/src/subagents/parser.rs (1)
parse_definition(25-109)codex-rs/core/src/subagents/definition.rs (2)
new(28-32)new(36-68)
🔇 Additional comments (4)
codex-rs/core/src/subagents/discovery.rs (1)
53-100: Fix compile errors, inverted existence check, and ensure deterministic traversal
- if condition is inverted and missing braces.
- Leftover iterator chain (into_iter/filter_map) after match is invalid.
- Match on parse_definition is non‑exhaustive.
- Add deterministic traversal order and keep traversal errors.
Apply:
- if !root.exists() { - for entry in WalkDir::new(&root) - .follow_links(false) - .into_iter() - { - let entry = match entry { - Ok(e) => e, - Err(err) => { - outcome.events.push(DiscoveryEvent { - message: format!("Failed to traverse directory: {}", err), - }); - continue; - } - }; - .into_iter() - .filter_map(Result::ok) - { - if !entry.file_type().is_file() { - continue; - } - - let path = entry.into_path(); - if !is_markdown(&path) { - continue; - } - - match fs::read_to_string(&path) { - Ok(contents) => match parse_definition(&path, &contents, scope) { - Ok(definition) => outcome.definitions.push(definition), - Err(SubagentParserError::ParseError(message)) => { - outcome.events.push(DiscoveryEvent { - message: format!( - "Failed to parse subagent definition at {}: {}", - path.display(), - message - ), - }); - } - }, - Err(error) => outcome.events.push(DiscoveryEvent { - message: format!( - "Failed to read subagent definition at {}: {}", - path.display(), - error - ), - }), - } - } + if !root.exists() { + outcome.events.push(DiscoveryEvent { + message: format!("Discovery root does not exist: {}", root.display()), + }); + return outcome; + } + + for entry in WalkDir::new(&root) + .follow_links(false) + .sort_by(|a, b| a.path().cmp(b.path())) + { + match entry { + Ok(entry) => { + if !entry.file_type().is_file() { + continue; + } + let path = entry.into_path(); + if !is_markdown(&path) { + continue; + } + match fs::read_to_string(&path) { + Ok(contents) => match parse_definition(&path, &contents, scope) { + Ok(definition) => outcome.definitions.push(definition), + Err(error) => outcome.events.push(DiscoveryEvent { + message: format!( + "Failed to parse subagent definition at {}: {}", + path.display(), + error + ), + }), + }, + Err(error) => outcome.events.push(DiscoveryEvent { + message: format!( + "Failed to read subagent definition at {}: {}", + path.display(), + error + ), + }), + } + } + Err(err) => outcome.events.push(DiscoveryEvent { + message: format!("Failed to traverse {}: {}", root.display(), err), + }), + } + }codex-rs/core/src/openai_tools.rs (2)
581-588: Remove model-controlled “confirmed” flag from tool schemaExposing confirmed lets the model self-approve invocation. Remove it; confirmations must be enforced by the runner.
- properties.insert( - "confirmed".to_string(), - JsonSchema::Boolean { - description: Some( - "Set to true after the user explicitly approves invoking the subagent when auto discovery is enabled.".to_string(), - ), - }, - );
805-863: Signature change verified — all call sites updated to pass the new subagent_tool argumentSearch shows all references are in codex-rs/core (calls in src/codex.rs and tests in src/openai_tools.rs) and each call supplies the third argument (None or Some(®istration)).
codex-cli/src/ipc/subagents.ts (1)
136-156: Treat missing/empty record as not found; surface a clear error.If the CLI returns an object without a name, we currently return a blank record instead of a not-found error.
Apply this diff:
try { - const raw = JSON.parse(payloadText) as RawRecordPayload; - return normalizeRecord(raw); + const raw = JSON.parse(payloadText) as RawRecordPayload; + const normalized = normalizeRecord(raw); + if (!normalized.name) { + const notFound = new Error(`Subagent not found: ${name}`); + (notFound as { stdout?: string }).stdout = result.stdout; + (notFound as { stderr?: string }).stderr = result.stderr; + throw notFound; + } + return normalized; } catch (err) {
| const payload = await fetchSubagentList({ | ||
| scope: options.scope, | ||
| includeInvalid: options.includeInvalid, | ||
| }); |
There was a problem hiding this comment.
Honor caller-provided timeoutMs in listAgents.
The timeout option is defined on ListAgentsOptions but not forwarded, so callers cannot override the default.
Apply this diff:
const payload = await fetchSubagentList({
scope: options.scope,
includeInvalid: options.includeInvalid,
+ timeoutMs: options.timeoutMs,
});📝 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.
| const payload = await fetchSubagentList({ | |
| scope: options.scope, | |
| includeInvalid: options.includeInvalid, | |
| }); | |
| const payload = await fetchSubagentList({ | |
| scope: options.scope, | |
| includeInvalid: options.includeInvalid, | |
| timeoutMs: options.timeoutMs, | |
| }); |
🤖 Prompt for AI Agents
In codex-cli/src/commands/agents/list.ts around lines 11 to 14, the call to
fetchSubagentList doesn't forward the caller-provided timeoutMs from
ListAgentsOptions; update the payload call to pass timeoutMs: options.timeoutMs
(or the correct option name) so the caller's timeout overrides the default by
including timeoutMs in the object sent to fetchSubagentList.
| const clearTimersAndListeners = () => { | ||
| if (timeoutTimer) { | ||
| clearTimeout(timeoutTimer); | ||
| timeoutTimer = undefined; | ||
| } | ||
| if (killTimer) { | ||
| clearTimeout(killTimer); | ||
| killTimer = undefined; | ||
| } | ||
| child.stdout.off("data", onStdout); | ||
| child.stderr.off("data", onStderr); | ||
| child.off("error", onError); | ||
| child.off("close", onClose); | ||
| }; |
There was a problem hiding this comment.
Timeout path cancels SIGKILL escalation; child may linger.
On timeout you immediately clear killTimer via settleReject -> clearTimersAndListeners, so the SIGKILL escalation never fires if the process ignores SIGTERM.
Apply this diff to preserve the escalation timer until the child exits:
- const clearTimersAndListeners = () => {
+ const clearStreams = () => {
+ child.stdout.off("data", onStdout);
+ child.stderr.off("data", onStderr);
+ child.off("error", onError);
+ };
+
+ const clearTimersAndListeners = () => {
if (timeoutTimer) {
clearTimeout(timeoutTimer);
timeoutTimer = undefined;
}
if (killTimer) {
clearTimeout(killTimer);
killTimer = undefined;
}
- child.stdout.off("data", onStdout);
- child.stderr.off("data", onStderr);
- child.off("error", onError);
+ clearStreams();
child.off("error", onError);
child.off("close", onClose);
};
@@
- const settleReject = (error: Error) => {
+ const settleReject = (error: Error, opts: { preserveKillTimer?: boolean } = {}) => {
if (completed) {
return;
}
completed = true;
- clearTimersAndListeners();
+ if (opts.preserveKillTimer) {
+ if (timeoutTimer) {
+ clearTimeout(timeoutTimer);
+ timeoutTimer = undefined;
+ }
+ // Keep onClose so we can clear killTimer when the child actually exits.
+ clearStreams();
+ } else {
+ clearTimersAndListeners();
+ }
reject(error);
};
@@
- if (typeof timeoutMs === "number") {
- timeoutTimer = setTimeout(() => {
+ if (typeof timeoutMs === "number") {
+ timeoutTimer = setTimeout(() => {
const { stdout, stderr } = collectOutput();
child.kill("SIGTERM");
killTimer = setTimeout(() => {
if (!completed) {
child.kill("SIGKILL");
}
}, KILL_GRACE_PERIOD_MS);
@@
- settleReject(timeoutError);
+ settleReject(timeoutError, { preserveKillTimer: true });
}, timeoutMs);
}
@@
- const onClose = (code: number | null, signal: NodeJS.Signals | null) => {
+ const onClose = (code: number | null, signal: NodeJS.Signals | null) => {
+ if (killTimer) {
+ clearTimeout(killTimer);
+ killTimer = undefined;
+ }
const { stdout, stderr } = collectOutput();
settleResolve({
stdout,
stderr,
code: code ?? 1,
signal,
});
};Also applies to: 238-245, 259-287, 302-310
| const triple = detectTargetTriple(); | ||
| if (triple) { | ||
| const packagedBase = path.resolve(cliRoot, "bin", `codex-${triple}`); | ||
| const packaged = platform === "win32" ? `${packagedBase}.exe` : packagedBase; | ||
| if (existsSync(packaged)) { | ||
| return packaged; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix packaged binary lookup path (currently points to dist/bin).
Resolving from cliRoot ends up under dist/bin when bundled; packaged binaries live in codex-cli/bin. This will break binary resolution in packaged installs.
Apply this diff:
- const packagedBase = path.resolve(cliRoot, "bin", `codex-${triple}`);
+ const packagedBase = path.resolve(workspaceRoot, "bin", `codex-${triple}`);📝 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.
| const triple = detectTargetTriple(); | |
| if (triple) { | |
| const packagedBase = path.resolve(cliRoot, "bin", `codex-${triple}`); | |
| const packaged = platform === "win32" ? `${packagedBase}.exe` : packagedBase; | |
| if (existsSync(packaged)) { | |
| return packaged; | |
| } | |
| } | |
| const triple = detectTargetTriple(); | |
| if (triple) { | |
| const packagedBase = path.resolve(workspaceRoot, "bin", `codex-${triple}`); | |
| const packaged = platform === "win32" ? `${packagedBase}.exe` : packagedBase; | |
| if (existsSync(packaged)) { | |
| return packaged; | |
| } | |
| } |
🤖 Prompt for AI Agents
In codex-cli/src/ipc/subagents.ts around lines 352 to 359, the packaged binary
lookup uses path.resolve(cliRoot, "bin", ...) which resolves into dist/bin for
bundled installs; change the lookup to resolve to the package's bin directory
(codex-cli/bin) instead by using the parent directory of cliRoot (e.g.
path.resolve(cliRoot, "..", "bin", `codex-${triple}`) and append .exe on Windows
as before), so the lookup points to codex-cli/bin rather than dist/bin.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codex-rs/core/src/subagents/discovery.rs (2)
7-11: Compact crate imports (per guidelines)Group related imports to reduce noise.
-use crate::subagents::definition::SubagentDefinition; -use crate::subagents::definition::SubagentScope; -use crate::subagents::inventory::DiscoveryEvent; - use crate::subagents::parser::parse_definition; +use crate::subagents::{ + definition::{SubagentDefinition, SubagentScope}, + inventory::DiscoveryEvent, + parser::parse_definition, +};
53-58: Also handle non-directory rootsGuard against a file path being passed as the discovery root.
if !root.exists() { outcome.events.push(DiscoveryEvent { message: format!("Discovery root does not exist: {}", root.display()), }); return outcome; } + if !root.is_dir() { + outcome.events.push(DiscoveryEvent { + message: format!("Discovery root is not a directory: {}", root.display()), + }); + return outcome; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codex-rs/core/src/subagents/discovery.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
codex-rs/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
codex-rs/**/*.rs: Use rustfmt defaults for all Rust code
Prefer compact imports in Rust files
Inline variables directly inside format! braces rather than pre-binding temporaries
Files:
codex-rs/core/src/subagents/discovery.rs
codex-rs/**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust module names snake_case
Files:
codex-rs/core/src/subagents/discovery.rs
🧬 Code graph analysis (1)
codex-rs/core/src/subagents/discovery.rs (2)
codex-rs/core/src/subagents/parser.rs (1)
parse_definition(25-109)codex-rs/core/src/subagents/definition.rs (2)
new(28-32)new(36-68)
🔇 Additional comments (2)
codex-rs/core/src/subagents/discovery.rs (2)
60-106: Fix invalid WalkDir loop; add deterministic ordering and robust error handlingThere’s a syntax error from the stray
.into_iter().filter_map(Result::ok)inside the loop block, and traversal currently lacks deterministic ordering. Also, onlyParseErroris handled; other parser errors would be unmatched if they exist. Replace the whole traversal with the following:- for entry in WalkDir::new(&root) - .follow_links(false) - .into_iter() - { - let entry = match entry { - Ok(e) => e, - Err(err) => { - outcome.events.push(DiscoveryEvent { - message: format!("Failed to traverse directory: {}", err), - }); - continue; - } - }; - .into_iter() - .filter_map(Result::ok) - { - if !entry.file_type().is_file() { - continue; - } - - let path = entry.into_path(); - if !is_markdown(&path) { - continue; - } - - match fs::read_to_string(&path) { - Ok(contents) => match parse_definition(&path, &contents, scope) { - Ok(definition) => outcome.definitions.push(definition), - Err(SubagentParserError::ParseError(message)) => { - outcome.events.push(DiscoveryEvent { - message: format!( - "Failed to parse subagent definition at {}: {}", - path.display(), - message - ), - }); - } - }, - Err(error) => outcome.events.push(DiscoveryEvent { - message: format!( - "Failed to read subagent definition at {}: {}", - path.display(), - error - ), - }), - } - } + for entry in WalkDir::new(&root) + .follow_links(false) + .sort_by(|a, b| a.path().cmp(b.path())) + { + match entry { + Ok(entry) => { + if !entry.file_type().is_file() { + continue; + } + let path = entry.into_path(); + if !is_markdown(&path) { + continue; + } + match fs::read_to_string(&path) { + Ok(contents) => match parse_definition(&path, &contents, scope) { + Ok(definition) => outcome.definitions.push(definition), + Err(error) => outcome.events.push(DiscoveryEvent { + message: format!( + "Failed to parse subagent definition at {}: {}", + path.display(), + error + ), + }), + }, + Err(error) => outcome.events.push(DiscoveryEvent { + message: format!( + "Failed to read subagent definition at {}: {}", + path.display(), + error + ), + }), + } + } + Err(err) => outcome.events.push(DiscoveryEvent { + message: format!("Failed to traverse {}: {}", root.display(), err), + }), + } + }
111-120: Prefer &Path over &PathBuf for detect_scopeUse &Path for a more idiomatic, flexible API.
-pub fn detect_scope(path: &PathBuf) -> SubagentScope { +pub fn detect_scope(path: &Path) -> SubagentScope {Location: codex-rs/core/src/subagents/discovery.rs:111
No call sites found in the repo—ensure external consumers are considered if this is a public API.
| use crate::subagents::definition::SubagentDefinition; | ||
| use crate::subagents::definition::SubagentScope; | ||
| use crate::subagents::inventory::DiscoveryEvent; | ||
| use crate::subagents::parser::SubagentParserError; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove now-unused SubagentParserError import
After adopting catch‑all parser error handling, this import becomes unused.
-use crate::subagents::parser::SubagentParserError;📝 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.
| use crate::subagents::parser::SubagentParserError; |
🤖 Prompt for AI Agents
In codex-rs/core/src/subagents/discovery.rs around line 10, the import use
crate::subagents::parser::SubagentParserError is now unused after switching to
catch-all parser error handling; remove this import line to clean up unused code
and run cargo check to verify there are no remaining unused-import warnings.
1 participant
Summary
Testing