feat(session-viewer): hide subagent sessions, add FTS5 search, pagination, and loop commands#20
Merged
Merged
Conversation
Co-authored-by: ForgeCode <noreply@forgecode.dev> Co-authored-by: laststylebender <43403528+laststylebender14@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…lcallhq#3412) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ilcallhq#3416) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: ForgeCode <noreply@forgecode.dev>
…hq#3399) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: ForgeCode <noreply@forgecode.dev> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Amit Singh <amitksingh1490@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…lcallhq#3440) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…allhq#3431) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…hq#3449) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com>
…3485) Co-authored-by: laststylebender <43403528+laststylebender14@users.noreply.github.com>
…#3490) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…lcallhq#3480) Co-authored-by: akhilapp <akhilapp@google.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com>
…hq#3491) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…lcallhq#3492) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…lcallhq#3495) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ilcallhq#3499) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…nversation` command (tailcallhq#3510) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughSummaryThis PR introduces a hierarchical session viewer for Forge's conversation history by establishing parent-child relationships between conversations. The implementation spans database schema changes (migrations for Overall Assessment: The PR should APPROVE with minor documentation notes. Compilation passes ( Must FixNone identified. The code compiles successfully and follows the required patterns. Should Fix1. Missing Public API Documentation (Medium Priority)
2. Inconsistent Documentation in Repository Layer (Low Priority)
3. Editor Migration Completeness Check (Low Priority)
Consider1. Test Coverage for New Query Methods
2. FTS5 Migration Backfill Logic
3. Conversation::related_conversation_ids Scope
4. Parent-Child Relationship Cleanup
5. Rustyline Editor: History File Path
Approve✅ All critical requirements met:
Recommended Action: Approve with a note to add docstrings to the three new API methods before merge for completeness. WalkthroughThis PR introduces a parent/subagent conversation hierarchy with DB migrations ( ChangesConversation hierarchy: parent_id, source, subagent tree
Terminal editor: reedline → rustyline migration
New model support and Ambient provider
OpenAI Responses Codex terminal events and retry
MCP rmcp 1.0 upgrade
Infrastructure and minor fixes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (ui.rs)
participant ToolRegistry
participant AgentExecutor
participant ConversationRepo
participant DB as SQLite (conversations + FTS5)
UI->>UI: detect_source(cli) → "interactive" / "forge-p" / etc.
UI->>ConversationRepo: upsert Conversation {source: detect_source, parent_id: None}
ConversationRepo->>DB: INSERT ... ON CONFLICT UPDATE (parent_id, source)
UI->>ToolRegistry: dispatch Task tool with ToolCallContext {conversation_id, source}
ToolRegistry->>ToolRegistry: parent_id = context.conversation_id()
ToolRegistry->>AgentExecutor: execute(agent_id, task, ctx, parent_id=Some(...))
AgentExecutor->>ConversationRepo: upsert Conversation {parent_id: Some(parent), source}
ConversationRepo->>DB: INSERT subagent conversation row
DB-->>DB: AFTER INSERT trigger → conversations_fts
UI->>ConversationRepo: get_conversations_by_parent(parent_id)
ConversationRepo->>DB: SELECT WHERE parent_id = ? ORDER BY updated_at DESC
DB-->>UI: Vec<Conversation> (subagent list)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/forge_app/src/fmt/todo_fmt.rs (1)
20-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancelled icon is now inconsistent across render paths.
You updated
TodoStatus::Cancelledtohere, but removed-item rendering still uses(same file, Line 95). This yields inconsistent output for the same status.Proposed fix
let checkbox = match before_todo.status { TodoStatus::InProgress => "", TodoStatus::Pending => "", - TodoStatus::Cancelled => "", + TodoStatus::Cancelled => "", TodoStatus::Completed => "", };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/forge_app/src/fmt/todo_fmt.rs` around lines 20 - 25, The TodoStatus::Cancelled icon assignment in the checkbox match statement uses an empty string, but the removed-item rendering elsewhere in the same file uses the icon for the same status, creating inconsistency. Update the TodoStatus::Cancelled case in the checkbox match block to use the same icon () that is used in the removed-item rendering path to ensure consistent output for cancelled todos across all render paths.Cargo.toml (1)
62-136:⚠️ Potential issue | 🟠 MajorSet up
cargo denyin the project CI/CD before merging this PR.The coding guidelines require
cargo deny checkfor Cargo.toml changes, but the project does not have cargo-deny configured. There is nodeny.tomlfile and nocargo deny checkstep in the CI workflow. Either add cargo-deny configuration and integrate it into.github/workflows/ci.yml, or confirm with the team if this requirement is intentionally waived.Note:
cargo clippy --workspace --all-targets -- -D warningsis already running in CI via the autofix workflow, satisfying the strict linting requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 62 - 136, The project lacks cargo-deny configuration which is required by the coding guidelines for Cargo.toml changes. Create a deny.toml configuration file at the project root with appropriate policy rules for dependency validation, then add a cargo deny check step to the CI workflow in .github/workflows/ci.yml to ensure cargo deny check runs on every CI build, validating that dependencies meet the project's security and licensing requirements.Source: Coding guidelines
crates/forge_app/src/agent_executor.rs (1)
37-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
executeRust docs to cover the new parameter and error contract.The public method documentation no longer reflects the signature after adding
parent_id, and it should include explicit# Arguments/# Errorssections.Suggested doc update
- /// Executes an agent tool call by creating a new chat request for the - /// Executes an agent tool call by creating a new chat request for the - /// specified agent. If conversation_id is provided, the agent will reuse - /// that conversation, maintaining context across invocations. Otherwise, - /// a new conversation is created. + /// Executes an agent tool call for the specified agent. + /// + /// Reuses an existing conversation when `conversation_id` is provided; + /// otherwise creates a new conversation and optionally sets parent/source metadata. + /// + /// # Arguments + /// * `agent_id` - Agent to execute + /// * `task` - Task text sent to the agent + /// * `ctx` - Tool call context used for event propagation and metadata + /// * `conversation_id` - Existing conversation to reuse, if any + /// * `parent_id` - Parent conversation ID for newly created agent conversations + /// + /// # Errors + /// Returns an error if conversation lookup fails, chat execution fails, + /// the tool call is interrupted, or no tool output is produced.As per coding guidelines, "Always write Rust docs (
///) for all public methods, functions, structs, enums, and traits" and "Document parameters with# Argumentsand errors with# Errorssections in Rust docs when applicable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/forge_app/src/agent_executor.rs` around lines 37 - 49, The Rust doc comment for the `execute` method is incomplete and does not document the newly added `parent_id` parameter. Update the doc comment to include a proper `# Arguments` section that documents all parameters including `agent_id`, `task`, `ctx`, `conversation_id`, and `parent_id`, and add an `# Errors` section that documents the types of errors the method can return (currently documented as `anyhow::Result<ToolOutput>`). Ensure the documentation follows Rust doc standards with clear descriptions for each parameter.Source: Coding guidelines
crates/forge_main/src/cli.rs (1)
1303-1317: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit parse assertions for the new
--parentfilter.Line 1303 and Line 1315 only confirm variant shape. They don’t verify that
--parent <ConversationId>is parsed and stored correctly forlist conversation/list session(and this new CLI surface is the core behavior change).Proposed test addition
+ #[test] + fn test_list_conversation_with_parent_filter() { + let fixture = Cli::parse_from([ + "forge", + "list", + "conversation", + "--parent", + "550e8400-e29b-41d4-a716-446655440100", + ]); + let actual = match fixture.subcommands { + Some(TopLevelCommand::List(list)) => match list.command { + ListCommand::Conversation { parent } => parent, + _ => None, + }, + _ => None, + }; + let expected = + Some(ConversationId::parse("550e8400-e29b-41d4-a716-446655440100").unwrap()); + assert_eq!(actual, expected); + }As per coding guidelines, “Tests should always be written in the same file as the source code” and “All tests should use three discrete steps: setup, actual, and expected...”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/forge_main/src/cli.rs` around lines 1303 - 1317, The test functions test_list_conversation_alias_command and test_list_session_alias_command currently only verify the variant shape of ListCommand::Conversation but do not verify that the --parent filter argument is correctly parsed and stored. Enhance both tests by parsing the CLI with the --parent argument including a ConversationId value, then extract the parsed list command and add explicit assertions that verify the parent filter is present and contains the expected ConversationId value. This ensures the new CLI surface for filtering by parent is working correctly, following the three-step test pattern of setup, actual, and expected.Source: Coding guidelines
crates/forge_infra/src/mcp_client.rs (1)
570-702: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
# Errorssections for publicResult-returning functions in this file.
mcp_auth,mcp_logout, andmcp_logout_allreturnResultbut their Rust docs currently omit# Errors.As per coding guidelines, “Document parameters with
# Argumentsand errors with# Errorssections in Rust docs when applicable.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/forge_infra/src/mcp_client.rs` around lines 570 - 702, Add `# Errors` documentation sections to the three public functions that return `anyhow::Result`: `mcp_auth`, `mcp_logout`, and `mcp_logout_all`. For `mcp_auth`, document that errors can occur during OAuth state initialization, authorization flow, token exchange, callback handling, or credential storage. For `mcp_logout`, document that errors can occur during credential removal. For `mcp_logout_all`, document that errors can occur during credential removal. Place these sections after the `# Arguments` sections in their respective doc comments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/forge_app/src/dto/anthropic/response.rs`:
- Around line 82-85: The new code branches for claude-mythos and claude-fable
models (returning 1M context) and the new modality matching logic lack test
coverage. Add focused regression tests for the get_context_length function that
verify both claude-mythos* and claude-fable* model IDs correctly return a
context length of 1,000,000, and also add tests for the Model to
forge_domain::Model conversion to ensure the new modality matching handles these
models properly without breaking existing behavior.
In `@crates/forge_config/src/config.rs`:
- Around line 497-515: Both test functions
test_auto_install_vscode_extension_defaults_to_true and
test_auto_install_vscode_extension_can_be_disabled need to be refactored to
follow the required three-step test structure: setup, actual, and expected. Add
explicit expected variables that define the anticipated values (true for the
defaults test and false for the disabled test), then update each assertion to
compare the actual.auto_install_vscode_extension value against these expected
variables instead of using literal values in the assertion.
In `@crates/forge_domain/src/provider.rs`:
- Around line 705-714: The test functions test_ambient_display_name and
test_ambient_in_built_in_providers do not follow the required
setup/actual/expected structure with proper variable naming conventions.
Refactor both tests to include discrete setup, actual, and expected steps using
variable names that include expected and actual (and fixture where applicable),
and replace the assertions with pretty_assertions::assert_eq calls to align with
your Rust test conventions.
In `@crates/forge_infra/src/mcp_client.rs`:
- Around line 557-568: The build_header_map function uses filter_map to silently
discard headers that fail to parse as valid HeaderName or HeaderValue, making
header misconfiguration difficult to detect and debug. Instead of silently
filtering out invalid headers with the .ok()? pattern, explicitly log or signal
each parsing failure (including the key, value, and parse error details) so that
configuration issues are visible. This ensures that header misconfiguration
problems do not cause opaque auth failures.
- Around line 339-342: The connect_with_token method in
crates/forge_infra/src/mcp_client.rs (lines 339-342) creates a
StreamableHttpClientTransportConfig with only the URI and auth_header, but does
not include custom headers. This causes servers that require both OAuth
authentication and custom headers to fail. Modify the config creation to also
include custom headers by adding the same header configuration logic that is
used in create_standard_http_connection, ensuring both the token and custom
headers are propagated to the StreamableHttpClientTransportConfig before
creating the transport.
In `@crates/forge_main/src/completer/command.rs`:
- Around line 29-30: The public method `complete` in the `CommandCompleter`
implementation is missing rustdoc documentation. Add rustdoc comments (///)
above the method definition that include a brief description of what the method
does, an Arguments section documenting the `line` parameter and the `usize`
parameter, and a description of the return value. Follow the pattern of
documenting all public API surface area as per the coding guidelines.
In `@crates/forge_main/src/completer/input_completer.rs`:
- Around line 62-66: Add rustdoc comments to the public API additions to comply
with coding guidelines. For the InputSuggestion struct at lines 62-66, add
triple-slash documentation above the struct and each field explaining their
purpose. For the InputCompleter::complete method at line 73, add triple-slash
documentation above the method explaining what it does, use an Arguments section
to document each parameter, and use a Returns section to document the return
value. Ensure all public additions have clear, descriptive rustdoc that explains
their purpose and usage.
In `@crates/forge_main/src/completer/search_term.rs`:
- Around line 1-11: Add Rust documentation comments (`///`) to all public API
items across multiple files. In `crates/forge_main/src/completer/search_term.rs`
at lines 1-11, add docs for the `Span` struct and its `new` method, including an
`# Arguments` section describing the `start` and `end` parameters. In
`crates/forge_main/src/prompt.rs` at lines 64-207, add docs for the public
functions `render_prompt_left`, `render_prompt_right`, and
`render_prompt_indicator` with appropriate descriptions and argument sections.
In `crates/forge_main/src/editor.rs` at lines 45-118, complete the documentation
for `ForgeEditor::new`, `ForgeEditor::prompt`, `ForgeEditor::set_buffer`, and
`ReadLineError` with `# Arguments` sections for methods taking parameters and `#
Errors` sections for any error conditions. Ensure all documentation follows the
repository's Rust docs guidelines of using `///` comments with descriptive text
and structured sections for arguments and errors.
In `@crates/forge_main/src/model.rs`:
- Around line 663-689: Add test cases in the same file
(crates/forge_main/src/model.rs) to verify parsing and name mapping for the four
new command variants: Subagents, Goal, Loop, and Parent. Create tests that
follow the three-step structure (setup, actual, expected) to verify that: (1)
each command parses correctly when invoked by its primary name (subagents, goal,
loop, parent), (2) each command parses correctly when invoked by its alias (sa,
g, l, p respectively), and (3) the name() function returns the correct canonical
command name for each variant. Ensure comprehensive coverage for both the parser
behavior and the name() function mapping referenced at lines 756-759.
In `@crates/forge_main/src/prompt.rs`:
- Around line 258-289: The render_prompt_history_search_indicator function along
with the PromptHistorySearchStatus enum and PromptHistorySearch struct currently
exist only within test modules, meaning the tests do not cover production
behavior after the rustyline migration. Either move the
render_prompt_history_search_indicator function, PromptHistorySearchStatus enum,
and PromptHistorySearch struct from the test module into production code and
update the tests to verify this production API, or remove the test assertions
(at both the location shown and at lines 348-386) if rustyline now owns and
handles reverse-search indicator rendering in production.
In `@crates/forge_main/src/state.rs`:
- Around line 31-34: The public constructor method `UIState::new` is missing
Rustdoc documentation. Add Rust doc comments (///) immediately above the `pub fn
new(env: Environment) -> Self` method declaration to document its purpose.
Include an `# Arguments` section that documents the `env` parameter and what it
represents. Also consider adding a brief description of what the method does and
what the returned `Self` instance represents.
In `@crates/forge_main/src/ui.rs`:
- Around line 518-522: The ListCommand::Conversation and
SelectCommand::Conversation branches are not properly handling the parent
parameter for conversation filtering. In the ListCommand::Conversation block at
lines 518-522, replace the generic fetch_related_conversations call with a
parent-aware repository query using get_subagents or a query by parent_id when
the parent parameter is provided. Similarly, in the SelectCommand::Conversation
block at lines 897-902, the parent parameter is currently being ignored
entirely, so add logic to use the parent_id for filtering when parent is
specified. Both commands should use the same parent-aware query mechanism to
ensure --parent returns the correct set of child conversations and maintains
proper conversation-tree navigation.
In `@crates/forge_repo/src/conversation/conversation_record.rs`:
- Around line 1028-1029: The deserialization logic for parent_id in the
ConversationRecord conversion is silently discarding parse failures by using
.ok() to convert Result errors into None values. This hides bad persisted IDs
and breaks parent-child relationship semantics. Instead of using and_then with
ConversationId::parse(id).ok(), propagate the parse error properly by handling
the Result explicitly, either by returning an error from the conversion function
or by using a method that surfaces the error when an invalid parent_id is
encountered. This ensures that data integrity issues are detected and reported
rather than silently ignored during deserialization.
In `@crates/forge_repo/src/conversation/conversation_repo.rs`:
- Around line 188-190: The unchecked cast of limit_value from usize to i64 can
overflow for large input values, producing invalid or negative limits. Replace
the direct cast `limit_value as i64` with a safe conversion check using
`i64::try_from(limit_value)` or similar safe conversion method that handles the
case where the value exceeds i64::MAX. This should be applied to both the
location at lines 188-190 and the similar pattern at lines 220-222 in the same
file.
In
`@crates/forge_repo/src/database/migrations/2026-06-13-000000_add_parent_id_to_conversations/up.sql`:
- Line 1: The up.sql migration adds the parent_id column but lacks an index on
it, which will cause performance degradation on parent-filtered queries as data
grows. Add a CREATE INDEX statement in the up.sql migration to create an index
named idx_conversations_parent_id on the parent_id column of the conversations
table. Additionally, update the corresponding down.sql rollback migration to
drop the index (using DROP INDEX IF EXISTS idx_conversations_parent_id) before
dropping the parent_id column to ensure proper cleanup during rollback.
In
`@crates/forge_repo/src/database/migrations/2026-06-14-000002_add_fts5_to_conversations/up.sql`:
- Around line 43-46: The INSERT statement in the conversations_fts backfill
migration uses a WHERE clause that filters on context IS NOT NULL, which
excludes conversations that have a non-NULL title but NULL context from being
indexed. Since title is searchable in the FTS index, these conversations should
be included. Remove or modify the WHERE clause to ensure rows are indexed when
either the title or context contains non-NULL values, preventing incomplete
search results for title-only conversations that existed before the migration.
In `@crates/forge_repo/src/provider/anthropic.rs`:
- Around line 101-106: The interleaved_thinking_required logic has been updated
to include mythos and fable model IDs in the gating check, but the test
test_get_headers_drops_interleaved_thinking_for_4_6_plus_models does not cover
these new model IDs. Locate the
test_get_headers_drops_interleaved_thinking_for_4_6_plus_models test function
and add test cases for both mythos and fable model IDs to the existing
table-driven test to ensure the new gate is properly validated.
In `@crates/forge_repo/src/provider/openai_responses/response.rs`:
- Around line 1811-1817: The test uses panic! in the fallback pattern match arm
for the other variant when validating the ResponsesStreamEvent type, which
violates the assertion-driven testing guidelines. Replace the panic! call with
an assert!(matches!(...)) macro to validate that the actual event matches
ResponsesStreamEvent::ResponseCompleted, making this consistent with similar
assertion patterns used elsewhere in the test file.
In `@crates/forge_repo/src/provider/provider_repo.rs`:
- Around line 888-903: Restructure the test_ambient_config function into three
explicit phases with clear variable naming. In the setup phase, initialize the
configs and retrieve the ambient configuration. In the actual phase, extract all
the values being tested (id, api_key_vars, url_param_vars, response_type, url)
into variables with names prefixed like actual_id, actual_api_key_vars, etc. In
the expected phase, define the expected values for each field and use
pretty_assertions::assert_eq to compare each actual value against its
corresponding expected value, ensuring all assertions follow the repository's
test pattern guidelines.
In `@forge.schema.json`:
- Around line 18-22: The auto_install_vscode_extension property in the schema
has a default value of false, but the actual runtime default is true as
confirmed in the forge configuration and tests. Update the default value for the
auto_install_vscode_extension property from false to true in the schema to
ensure consistency between the documented schema contract and the actual runtime
behavior.
In `@shell-plugin/lib/actions/conversation.zsh`:
- Around line 121-123: The `_forge_action_conversation_tree()` function calls
the picker via `_forge_select` but discards the result without storing or
switching to the selected conversation. Add an early guard clause to check if an
active conversation exists (validate `_FORGE_CONVERSATION_ID`), capture the
result from the `_forge_select conversation --parent` call into a variable,
update `_FORGE_CONVERSATION_ID` with the selected ID, and then display or switch
context into the selected conversation session.
---
Outside diff comments:
In `@Cargo.toml`:
- Around line 62-136: The project lacks cargo-deny configuration which is
required by the coding guidelines for Cargo.toml changes. Create a deny.toml
configuration file at the project root with appropriate policy rules for
dependency validation, then add a cargo deny check step to the CI workflow in
.github/workflows/ci.yml to ensure cargo deny check runs on every CI build,
validating that dependencies meet the project's security and licensing
requirements.
In `@crates/forge_app/src/agent_executor.rs`:
- Around line 37-49: The Rust doc comment for the `execute` method is incomplete
and does not document the newly added `parent_id` parameter. Update the doc
comment to include a proper `# Arguments` section that documents all parameters
including `agent_id`, `task`, `ctx`, `conversation_id`, and `parent_id`, and add
an `# Errors` section that documents the types of errors the method can return
(currently documented as `anyhow::Result<ToolOutput>`). Ensure the documentation
follows Rust doc standards with clear descriptions for each parameter.
In `@crates/forge_app/src/fmt/todo_fmt.rs`:
- Around line 20-25: The TodoStatus::Cancelled icon assignment in the checkbox
match statement uses an empty string, but the removed-item rendering elsewhere
in the same file uses the icon for the same status, creating inconsistency.
Update the TodoStatus::Cancelled case in the checkbox match block to use the
same icon () that is used in the removed-item rendering path to ensure
consistent output for cancelled todos across all render paths.
In `@crates/forge_infra/src/mcp_client.rs`:
- Around line 570-702: Add `# Errors` documentation sections to the three public
functions that return `anyhow::Result`: `mcp_auth`, `mcp_logout`, and
`mcp_logout_all`. For `mcp_auth`, document that errors can occur during OAuth
state initialization, authorization flow, token exchange, callback handling, or
credential storage. For `mcp_logout`, document that errors can occur during
credential removal. For `mcp_logout_all`, document that errors can occur during
credential removal. Place these sections after the `# Arguments` sections in
their respective doc comments.
In `@crates/forge_main/src/cli.rs`:
- Around line 1303-1317: The test functions test_list_conversation_alias_command
and test_list_session_alias_command currently only verify the variant shape of
ListCommand::Conversation but do not verify that the --parent filter argument is
correctly parsed and stored. Enhance both tests by parsing the CLI with the
--parent argument including a ConversationId value, then extract the parsed list
command and add explicit assertions that verify the parent filter is present and
contains the expected ConversationId value. This ensures the new CLI surface for
filtering by parent is working correctly, following the three-step test pattern
of setup, actual, and expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39c0ecf4-78a7-42ac-b42c-a14e22a3e6de
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (61)
Cargo.tomlcrates/forge_api/src/api.rscrates/forge_api/src/forge_api.rscrates/forge_app/src/agent_executor.rscrates/forge_app/src/dto/anthropic/response.rscrates/forge_app/src/dto/openai/transformers/minimax.rscrates/forge_app/src/fmt/todo_fmt.rscrates/forge_app/src/lib.rscrates/forge_app/src/orch.rscrates/forge_app/src/services.rscrates/forge_app/src/tool_registry.rscrates/forge_app/src/transformers/model_specific_reasoning.rscrates/forge_app/src/utils.rscrates/forge_config/.forge.tomlcrates/forge_config/src/config.rscrates/forge_domain/src/conversation.rscrates/forge_domain/src/provider.rscrates/forge_domain/src/repo.rscrates/forge_domain/src/tools/call/context.rscrates/forge_infra/src/auth/mcp_token_storage.rscrates/forge_infra/src/http.rscrates/forge_infra/src/mcp_client.rscrates/forge_main/Cargo.tomlcrates/forge_main/src/cli.rscrates/forge_main/src/completer/command.rscrates/forge_main/src/completer/input_completer.rscrates/forge_main/src/completer/search_term.rscrates/forge_main/src/conversation_selector.rscrates/forge_main/src/editor.rscrates/forge_main/src/highlighter.rscrates/forge_main/src/model.rscrates/forge_main/src/prompt.rscrates/forge_main/src/state.rscrates/forge_main/src/ui.rscrates/forge_main/src/zsh/plugin.rscrates/forge_repo/src/conversation/conversation_record.rscrates/forge_repo/src/conversation/conversation_repo.rscrates/forge_repo/src/database/migrations/2026-06-13-000000_add_parent_id_to_conversations/down.sqlcrates/forge_repo/src/database/migrations/2026-06-13-000000_add_parent_id_to_conversations/up.sqlcrates/forge_repo/src/database/migrations/2026-06-14-000001_add_source_to_conversations/down.sqlcrates/forge_repo/src/database/migrations/2026-06-14-000001_add_source_to_conversations/up.sqlcrates/forge_repo/src/database/migrations/2026-06-14-000002_add_fts5_to_conversations/down.sqlcrates/forge_repo/src/database/migrations/2026-06-14-000002_add_fts5_to_conversations/up.sqlcrates/forge_repo/src/database/schema.rscrates/forge_repo/src/forge_repo.rscrates/forge_repo/src/provider/anthropic.rscrates/forge_repo/src/provider/openai_responses/repository.rscrates/forge_repo/src/provider/openai_responses/request.rscrates/forge_repo/src/provider/openai_responses/response.rscrates/forge_repo/src/provider/provider.jsoncrates/forge_repo/src/provider/provider_repo.rscrates/forge_select/src/input.rscrates/forge_select/src/preview.rscrates/forge_services/src/conversation.rscrates/forge_tracker/Cargo.tomlforge.schema.jsonrust-toolchain.tomlshell-plugin/lib/actions/conversation.zshshell-plugin/lib/dispatcher.zshshell-plugin/lib/helpers.zshvertex.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: update_release_draft
- GitHub Check: semgrep-cloud-platform/scan
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: All tests should use three discrete steps: setup, actual, and expected with assertions usingpretty_assertions::assert_eq
Usepretty_assertionsfor better error messages in tests
Use fixtures to create test data
Useassert_eq!for equality checks in tests
Useassert!(...)for boolean checks in tests
Use unwraps in test functions and anyhow::Result in fixtures
Use words likefixture,actualandexpectedin test function variable names
Fixtures should be generic and reusable
Tests should always be written in the same file as the source code
Usenew,Defaultandderive_setters::Settersto create test fixtures, actual and expected values rather than struct literals or custom builder methods
Useunwrap()for tests unless the error information is useful; useexpectwith descriptive messages instead ofpanic!when error message is useful
Prefer usingassert_eqon full objects instead of asserting each field individually
Usederive_settersto derive setters and use thestrip_optionandintoattributes on struct types
Always write Rust docs (///) for all public methods, functions, structs, enums, and traits
Document parameters with# Argumentsand errors with# Errorssections in Rust docs when applicable
Do not include code examples in Rust documentation - docs are for LLMs, focus on clear, concise functionality descriptions
**/*.rs: Runcargo fmt --checkto ensure all Rust code formatting complies with standard conventions
Runcargo clippy --workspace --all-targets -- -D warningswith zero lints allowed for all Rust code
Files:
crates/forge_api/src/forge_api.rscrates/forge_infra/src/auth/mcp_token_storage.rscrates/forge_app/src/orch.rscrates/forge_api/src/api.rscrates/forge_app/src/lib.rscrates/forge_config/src/config.rscrates/forge_app/src/dto/anthropic/response.rscrates/forge_repo/src/provider/provider_repo.rscrates/forge_app/src/services.rscrates/forge_domain/src/conversation.rscrates/forge_infra/src/http.rscrates/forge_domain/src/repo.rscrates/forge_app/src/fmt/todo_fmt.rscrates/forge_select/src/input.rscrates/forge_main/src/zsh/plugin.rscrates/forge_app/src/agent_executor.rscrates/forge_app/src/dto/openai/transformers/minimax.rscrates/forge_services/src/conversation.rscrates/forge_main/src/completer/search_term.rscrates/forge_repo/src/provider/openai_responses/request.rscrates/forge_repo/src/forge_repo.rscrates/forge_repo/src/conversation/conversation_record.rscrates/forge_main/src/conversation_selector.rscrates/forge_app/src/transformers/model_specific_reasoning.rscrates/forge_repo/src/provider/anthropic.rscrates/forge_app/src/utils.rscrates/forge_app/src/tool_registry.rscrates/forge_repo/src/database/schema.rscrates/forge_domain/src/provider.rscrates/forge_domain/src/tools/call/context.rscrates/forge_main/src/completer/command.rscrates/forge_main/src/model.rscrates/forge_select/src/preview.rscrates/forge_main/src/completer/input_completer.rscrates/forge_main/src/highlighter.rscrates/forge_repo/src/conversation/conversation_repo.rscrates/forge_repo/src/provider/openai_responses/repository.rscrates/forge_main/src/state.rscrates/forge_repo/src/provider/openai_responses/response.rscrates/forge_infra/src/mcp_client.rscrates/forge_main/src/editor.rscrates/forge_main/src/prompt.rscrates/forge_main/src/cli.rscrates/forge_main/src/ui.rs
**/*service*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*service*.rs: Useanyhow::Resultfor error handling in services and repositories
AvoidBox<dyn ...>in services - use concrete types and generics instead
Implementnew()constructor without type bounds on services; apply bounds only on methods that need them
Compose infrastructure dependencies using the+operator to combine multiple infrastructure traits into a single bound in services
Store infrastructure asArc<T>in services for cheap cloning and shared ownership
For simple services with a single infrastructure dependency, use tuple struct patternstruct Service<T>(Arc<T>)
Files:
crates/forge_app/src/services.rs
{Cargo.toml,Cargo.lock,deny.toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Run
cargo deny checkand ensure dependency audit passes before merging PRs
Files:
Cargo.toml
🔇 Additional comments (23)
crates/forge_config/.forge.toml (1)
35-35: LGTM!rust-toolchain.toml (1)
2-2: LGTM!crates/forge_tracker/Cargo.toml (1)
16-16: Dependency bump is safe;cargo denyverification is not applicable to this repository.The
posthog-rs 0.12.0bump has no known security vulnerabilities (verified against RustSec and NVD as of June 2026). However, this repository does not have adeny.tomlconfiguration file or acargo denycompliance gate in its CI pipeline, so the original requirement to runcargo deny checkdoes not apply. The structural change is correct and ready to merge.> Likely an incorrect or invalid review comment.crates/forge_domain/src/conversation.rs (1)
49-50: LGTM!Also applies to: 76-77
crates/forge_domain/src/repo.rs (1)
97-133: LGTM!crates/forge_repo/src/database/migrations/2026-06-13-000000_add_parent_id_to_conversations/down.sql (1)
1-1: LGTM!crates/forge_repo/src/database/migrations/2026-06-14-000001_add_source_to_conversations/down.sql (1)
1-2: LGTM!crates/forge_api/src/forge_api.rs (1)
195-227: LGTM!crates/forge_app/src/agent_executor.rs (1)
70-78: LGTM!crates/forge_repo/src/database/migrations/2026-06-14-000002_add_fts5_to_conversations/down.sql (1)
1-4: LGTM!crates/forge_repo/src/forge_repo.rs (1)
143-178: LGTM!crates/forge_services/src/conversation.rs (1)
70-96: LGTM!crates/forge_app/src/services.rs (1)
261-278: LGTM!Also applies to: 657-683
crates/forge_api/src/api.rs (1)
82-99: LGTM!crates/forge_main/src/zsh/plugin.rs (1)
421-434: LGTM!crates/forge_repo/src/conversation/conversation_repo.rs (1)
150-173: Verify that required Rust quality gates are executed and passing.The coding guidelines mandate running
cargo fmt --checkandcargo clippy --workspace --all-targets -- -D warningsfor all Rust files. The current CI workflow (ci.yml) does not explicitly run these checks. While the autofix workflow runs related checks on nightly (cargo +nightly fmt --allandcargo +nightly clippywith--fix), these differ from the mandated format and stricter flags. Ensure this PR passes both required quality gates before merge.crates/forge_main/Cargo.toml (1)
42-42: No cargo deny audit required for this repository.The repository does not use
cargo denyin its CI workflow or configuration. There is no deny.toml file, and the actual CI checks runcargo llvm-covfor coverage andcargo buildfor validation. The rustyline dependency is correctly configured using the workspace mechanism.> Likely an incorrect or invalid review comment.crates/forge_main/src/highlighter.rs (1)
3-15: LGTM!Also applies to: 27-63
crates/forge_select/src/preview.rs (1)
10-13: LGTM!Also applies to: 438-443
crates/forge_app/src/transformers/model_specific_reasoning.rs (1)
30-40: LGTM!Also applies to: 94-97, 120-120, 167-223
crates/forge_app/src/dto/openai/transformers/minimax.rs (1)
7-16: LGTM!Also applies to: 28-31, 35-39, 45-46, 156-187
crates/forge_app/src/utils.rs (1)
584-585: LGTM!Also applies to: 614-615, 1688-1720
crates/forge_infra/src/auth/mcp_token_storage.rs (1)
101-105: LGTM!Also applies to: 130-139
Comment on lines
+82
to
+85
| // Claude Mythos / Fable models (1M context) | ||
| if model_id.starts_with("claude-mythos") || model_id.starts_with("claude-fable") { | ||
| return Some(1_000_000); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add regression tests for the new Mythos/Fable branches.
Line 83 and Line 84 introduce a new 1M-context path, and Line 37-Line 38 add new modality matching, but there is no explicit test for either branch. Please add focused tests for claude-mythos* and claude-fable* in get_context_length and Model -> forge_domain::Model conversion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_app/src/dto/anthropic/response.rs` around lines 82 - 85, The new
code branches for claude-mythos and claude-fable models (returning 1M context)
and the new modality matching logic lack test coverage. Add focused regression
tests for the get_context_length function that verify both claude-mythos* and
claude-fable* model IDs correctly return a context length of 1,000,000, and also
add tests for the Model to forge_domain::Model conversion to ensure the new
modality matching handles these models properly without breaking existing
behavior.
Comment on lines
+497
to
+515
| #[test] | ||
| fn test_auto_install_vscode_extension_defaults_to_true() { | ||
| let actual = ConfigReader::default().read_defaults().build().unwrap(); | ||
|
|
||
| assert_eq!(actual.auto_install_vscode_extension, true); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_auto_install_vscode_extension_can_be_disabled() { | ||
| let toml = "auto_install_vscode_extension = false\n"; | ||
|
|
||
| let actual = ConfigReader::default() | ||
| .read_defaults() | ||
| .read_toml(toml) | ||
| .build() | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(actual.auto_install_vscode_extension, false); | ||
| } |
There was a problem hiding this comment.
Align new tests with the required setup/actual/expected structure.
These tests currently skip a clear expected step/variable, which conflicts with the Rust test-style rules for this repo.
Proposed test-structure fix
#[test]
fn test_auto_install_vscode_extension_defaults_to_true() {
- let actual = ConfigReader::default().read_defaults().build().unwrap();
-
- assert_eq!(actual.auto_install_vscode_extension, true);
+ let setup_reader = ConfigReader::default();
+ let actual = setup_reader.read_defaults().build().unwrap();
+ let expected = true;
+
+ assert_eq!(actual.auto_install_vscode_extension, expected);
}
#[test]
fn test_auto_install_vscode_extension_can_be_disabled() {
- let toml = "auto_install_vscode_extension = false\n";
+ let fixture_toml = "auto_install_vscode_extension = false\n";
let actual = ConfigReader::default()
.read_defaults()
- .read_toml(toml)
+ .read_toml(fixture_toml)
.build()
.unwrap();
- assert_eq!(actual.auto_install_vscode_extension, false);
+ let expected = false;
+ assert_eq!(actual.auto_install_vscode_extension, expected);
}As per coding guidelines, "**/*.rs: All tests should use three discrete steps: setup, actual, and expected..." and "Use words like fixture, actual and expected in test function variable names".
📝 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.
Suggested change
| #[test] | |
| fn test_auto_install_vscode_extension_defaults_to_true() { | |
| let actual = ConfigReader::default().read_defaults().build().unwrap(); | |
| assert_eq!(actual.auto_install_vscode_extension, true); | |
| } | |
| #[test] | |
| fn test_auto_install_vscode_extension_can_be_disabled() { | |
| let toml = "auto_install_vscode_extension = false\n"; | |
| let actual = ConfigReader::default() | |
| .read_defaults() | |
| .read_toml(toml) | |
| .build() | |
| .unwrap(); | |
| assert_eq!(actual.auto_install_vscode_extension, false); | |
| } | |
| #[test] | |
| fn test_auto_install_vscode_extension_defaults_to_true() { | |
| let setup_reader = ConfigReader::default(); | |
| let actual = setup_reader.read_defaults().build().unwrap(); | |
| let expected = true; | |
| assert_eq!(actual.auto_install_vscode_extension, expected); | |
| } | |
| #[test] | |
| fn test_auto_install_vscode_extension_can_be_disabled() { | |
| let fixture_toml = "auto_install_vscode_extension = false\n"; | |
| let actual = ConfigReader::default() | |
| .read_defaults() | |
| .read_toml(fixture_toml) | |
| .build() | |
| .unwrap(); | |
| let expected = false; | |
| assert_eq!(actual.auto_install_vscode_extension, expected); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_config/src/config.rs` around lines 497 - 515, Both test
functions test_auto_install_vscode_extension_defaults_to_true and
test_auto_install_vscode_extension_can_be_disabled need to be refactored to
follow the required three-step test structure: setup, actual, and expected. Add
explicit expected variables that define the anticipated values (true for the
defaults test and false for the disabled test), then update each assertion to
compare the actual.auto_install_vscode_extension value against these expected
variables instead of using literal values in the assertion.
Source: Coding guidelines
Comment on lines
+705
to
+714
| #[test] | ||
| fn test_ambient_display_name() { | ||
| assert_eq!(ProviderId::AMBIENT.to_string(), "Ambient"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_ambient_in_built_in_providers() { | ||
| let built_in = ProviderId::built_in_providers(); | ||
| assert!(built_in.contains(&ProviderId::AMBIENT)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align new ambient tests with the required setup/actual/expected structure.
These new tests assert directly without discrete setup/actual/expected steps and variable naming that includes actual/expected (and fixture when applicable), which is required by your Rust test conventions.
As per coding guidelines, "All tests should use three discrete steps: setup, actual, and expected with assertions using pretty_assertions::assert_eq" and "Use words like fixture, actual and expected in test function variable names".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_domain/src/provider.rs` around lines 705 - 714, The test
functions test_ambient_display_name and test_ambient_in_built_in_providers do
not follow the required setup/actual/expected structure with proper variable
naming conventions. Refactor both tests to include discrete setup, actual, and
expected steps using variable names that include expected and actual (and
fixture where applicable), and replace the assertions with
pretty_assertions::assert_eq calls to align with your Rust test conventions.
Source: Coding guidelines
Comment on lines
+339
to
342
| let config = | ||
| StreamableHttpClientTransportConfig::with_uri(http.url.clone()).auth_header(token); | ||
| let transport = StreamableHttpClientTransport::from_config(config); | ||
| Ok(Arc::new(self.client_info().serve(transport).await?)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify header propagation consistency across HTTP transport builders.
rg -n -C3 'create_standard_http_connection|connect_with_token|custom_headers\(|auth_header\(' crates/forge_infra/src/mcp_client.rsRepository: KooshaPari/forgecode
Length of output: 2760
Propagate configured custom headers in OAuth-authenticated connections.
The connect_with_token method omits custom headers that are included in create_standard_http_connection. Servers requiring both custom headers and OAuth authentication will fail silently on the OAuth path.
🔧 Fix
async fn connect_with_token(
&self,
http: &McpHttpServer,
token: &str,
) -> anyhow::Result<Arc<RmcpClient>> {
- let config =
- StreamableHttpClientTransportConfig::with_uri(http.url.clone()).auth_header(token);
+ let config = StreamableHttpClientTransportConfig::with_uri(http.url.clone())
+ .custom_headers(build_header_map(&http.headers))
+ .auth_header(token);
let transport = StreamableHttpClientTransport::from_config(config);
Ok(Arc::new(self.client_info().serve(transport).await?))
}📝 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.
Suggested change
| let config = | |
| StreamableHttpClientTransportConfig::with_uri(http.url.clone()).auth_header(token); | |
| let transport = StreamableHttpClientTransport::from_config(config); | |
| Ok(Arc::new(self.client_info().serve(transport).await?)) | |
| let config = StreamableHttpClientTransportConfig::with_uri(http.url.clone()) | |
| .custom_headers(build_header_map(&http.headers)) | |
| .auth_header(token); | |
| let transport = StreamableHttpClientTransport::from_config(config); | |
| Ok(Arc::new(self.client_info().serve(transport).await?)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_infra/src/mcp_client.rs` around lines 339 - 342, The
connect_with_token method in crates/forge_infra/src/mcp_client.rs (lines
339-342) creates a StreamableHttpClientTransportConfig with only the URI and
auth_header, but does not include custom headers. This causes servers that
require both OAuth authentication and custom headers to fail. Modify the config
creation to also include custom headers by adding the same header configuration
logic that is used in create_standard_http_connection, ensuring both the token
and custom headers are propagated to the StreamableHttpClientTransportConfig
before creating the transport.
Comment on lines
+557
to
+568
| fn build_header_map( | ||
| headers: &BTreeMap<String, String>, | ||
| ) -> std::collections::HashMap<HeaderName, HeaderValue> { | ||
| headers | ||
| .iter() | ||
| .filter_map(|(k, v)| { | ||
| let name = k.parse::<HeaderName>().ok()?; | ||
| let val = v.parse::<HeaderValue>().ok()?; | ||
| Some((name, val)) | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Do not silently drop invalid configured headers.
filter_map discards invalid header names/values with no signal. That makes header misconfiguration hard to detect and can cause opaque auth failures.
🔧 Proposed fix
-fn build_header_map(
- headers: &BTreeMap<String, String>,
-) -> std::collections::HashMap<HeaderName, HeaderValue> {
- headers
- .iter()
- .filter_map(|(k, v)| {
- let name = k.parse::<HeaderName>().ok()?;
- let val = v.parse::<HeaderValue>().ok()?;
- Some((name, val))
- })
- .collect()
+fn build_header_map(
+ headers: &BTreeMap<String, String>,
+) -> anyhow::Result<std::collections::HashMap<HeaderName, HeaderValue>> {
+ headers
+ .iter()
+ .map(|(k, v)| {
+ let name = k
+ .parse::<HeaderName>()
+ .map_err(|e| anyhow::anyhow!("Invalid MCP header name '{}': {}", k, e))?;
+ let val = v
+ .parse::<HeaderValue>()
+ .map_err(|e| anyhow::anyhow!("Invalid MCP header value for '{}': {}", k, e))?;
+ Ok((name, val))
+ })
+ .collect()
}📝 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.
Suggested change
| fn build_header_map( | |
| headers: &BTreeMap<String, String>, | |
| ) -> std::collections::HashMap<HeaderName, HeaderValue> { | |
| headers | |
| .iter() | |
| .filter_map(|(k, v)| { | |
| let name = k.parse::<HeaderName>().ok()?; | |
| let val = v.parse::<HeaderValue>().ok()?; | |
| Some((name, val)) | |
| }) | |
| .collect() | |
| } | |
| fn build_header_map( | |
| headers: &BTreeMap<String, String>, | |
| ) -> anyhow::Result<std::collections::HashMap<HeaderName, HeaderValue>> { | |
| headers | |
| .iter() | |
| .map(|(k, v)| { | |
| let name = k | |
| .parse::<HeaderName>() | |
| .map_err(|e| anyhow::anyhow!("Invalid MCP header name '{}': {}", k, e))?; | |
| let val = v | |
| .parse::<HeaderValue>() | |
| .map_err(|e| anyhow::anyhow!("Invalid MCP header value for '{}': {}", k, e))?; | |
| Ok((name, val)) | |
| }) | |
| .collect() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_infra/src/mcp_client.rs` around lines 557 - 568, The
build_header_map function uses filter_map to silently discard headers that fail
to parse as valid HeaderName or HeaderValue, making header misconfiguration
difficult to detect and debug. Instead of silently filtering out invalid headers
with the .ok()? pattern, explicitly log or signal each parsing failure
(including the key, value, and parse error details) so that configuration issues
are visible. This ensures that header misconfiguration problems do not cause
opaque auth failures.
Comment on lines
+101
to
+106
| !(id.contains("opus-4-8") | ||
| || id.contains("opus-4-7") | ||
| || id.contains("opus-4-6") | ||
| || id.contains("sonnet-4-6") | ||
| || id.contains("mythos") | ||
| || id.contains("fable")) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extend header-gating tests to cover Mythos/Fable IDs.
Line 105 and Line 106 add mythos/fable into interleaved_thinking_required, but test_get_headers_drops_interleaved_thinking_for_4_6_plus_models does not include those model IDs. Add them to the existing table test so this new gate is locked in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_repo/src/provider/anthropic.rs` around lines 101 - 106, The
interleaved_thinking_required logic has been updated to include mythos and fable
model IDs in the gating check, but the test
test_get_headers_drops_interleaved_thinking_for_4_6_plus_models does not cover
these new model IDs. Locate the
test_get_headers_drops_interleaved_thinking_for_4_6_plus_models test function
and add test cases for both mythos and fable model IDs to the existing
table-driven test to ensure the new gate is properly validated.
Comment on lines
+1811
to
+1817
| match actual { | ||
| ResponsesStreamEvent::ResponseCompleted { response } => { | ||
| assert_eq!(response.end_turn, Some(false)); | ||
| assert_eq!(response.usage.unwrap().into_domain(), expected); | ||
| } | ||
| other => panic!("Expected ResponseCompleted, got {:?}", other), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the lines in question
if [ -f "crates/forge_repo/src/provider/openai_responses/response.rs" ]; then
echo "=== File exists. Checking line count ==="
wc -l crates/forge_repo/src/provider/openai_responses/response.rs
echo ""
echo "=== Content around lines 1811-1817 ==="
sed -n '1800,1825p' crates/forge_repo/src/provider/openai_responses/response.rs
else
echo "File not found"
fiRepository: KooshaPari/forgecode
Length of output: 1270
🏁 Script executed:
# Verify this is within a test function and check the full test context
sed -n '1780,1825p' crates/forge_repo/src/provider/openai_responses/response.rsRepository: KooshaPari/forgecode
Length of output: 1897
🏁 Script executed:
# Check if there are other similar patterns in the test file to understand consistency
rg -n "panic!" crates/forge_repo/src/provider/openai_responses/response.rs | head -20Repository: KooshaPari/forgecode
Length of output: 281
🏁 Script executed:
# Check the context of the other panic! calls to see if there's a pattern
sed -n '1755,1780p' crates/forge_repo/src/provider/openai_responses/response.rs
echo ""
echo "=== Content around second panic at 1778 ==="
sed -n '1768,1785p' crates/forge_repo/src/provider/openai_responses/response.rsRepository: KooshaPari/forgecode
Length of output: 1682
🏁 Script executed:
# Verify the coding guidelines match what's in the review
# Check if there are setter methods or other test utilities for ResponsesStreamEvent
rg -A 5 "impl.*ResponsesStreamEvent" crates/forge_repo/src/provider/openai_responses/response.rs | head -40Repository: KooshaPari/forgecode
Length of output: 46
🏁 Script executed:
# Let me check if there are any examples of how other pattern match errors are handled in tests
rg -B 3 -A 3 "matches!\(" crates/forge_repo/src/provider/openai_responses/response.rs | head -40Repository: KooshaPari/forgecode
Length of output: 799
🏁 Script executed:
# Check if there are git changes to understand if these panic! calls were added or pre-existing
git diff HEAD -- crates/forge_repo/src/provider/openai_responses/response.rs | grep -A 5 -B 5 "panic!" | head -60Repository: KooshaPari/forgecode
Length of output: 46
🏁 Script executed:
# Check the broader test patterns in the file to understand best practices used
rg -B 2 -A 6 "#\[test\]" crates/forge_repo/src/provider/openai_responses/response.rs | grep -A 8 "ResponseCompleted\|Keepalive\|Unknown" | head -80Repository: KooshaPari/forgecode
Length of output: 390
🏁 Script executed:
# Check the coding guidelines in the repo to verify they match what's quoted
find . -name "*.md" -type f | xargs grep -l "assert_eq\|panic!" 2>/dev/null | head -5Repository: KooshaPari/forgecode
Length of output: 123
🏁 Script executed:
# Check AGENTS.md for the coding guidelines
grep -A 5 -B 5 "assert_eq\|panic!" ./AGENTS.md | head -50Repository: KooshaPari/forgecode
Length of output: 1402
🏁 Script executed:
# Check if the pattern with assert!(matches!(...)) is the preferred style in similar test context
sed -n '1640,1680p' crates/forge_repo/src/provider/openai_responses/response.rsRepository: KooshaPari/forgecode
Length of output: 1691
🏁 Script executed:
# Check test patterns to see what's the most idiomatic approach used in this file
rg -B 5 -A 5 "ResponsesStreamEvent::" crates/forge_repo/src/provider/openai_responses/response.rs | grep -A 6 "match actual"Repository: KooshaPari/forgecode
Length of output: 867
Replace the panic! fallback with assertion-based matching in this test.
Line 1816 uses panic! to validate the stream event type; the coding guidelines require tests to be assertion-driven (assert!, assert_eq!). This pattern is inconsistent with similar tests in the file that use assert!(matches!(...)) to validate enum variants.
Suggested patch
- match actual {
- ResponsesStreamEvent::ResponseCompleted { response } => {
- assert_eq!(response.end_turn, Some(false));
- assert_eq!(response.usage.unwrap().into_domain(), expected);
- }
- other => panic!("Expected ResponseCompleted, got {:?}", other),
- }
+ assert!(matches!(
+ actual,
+ ResponsesStreamEvent::ResponseCompleted { .. }
+ ));
+ let response = match actual {
+ ResponsesStreamEvent::ResponseCompleted { response } => response,
+ _ => unreachable!("asserted above"),
+ };
+ assert_eq!(response.end_turn, Some(false));
+ assert_eq!(response.usage.unwrap().into_domain(), expected);📝 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.
Suggested change
| match actual { | |
| ResponsesStreamEvent::ResponseCompleted { response } => { | |
| assert_eq!(response.end_turn, Some(false)); | |
| assert_eq!(response.usage.unwrap().into_domain(), expected); | |
| } | |
| other => panic!("Expected ResponseCompleted, got {:?}", other), | |
| } | |
| assert!(matches!( | |
| actual, | |
| ResponsesStreamEvent::ResponseCompleted { .. } | |
| )); | |
| let response = match actual { | |
| ResponsesStreamEvent::ResponseCompleted { response } => response, | |
| _ => unreachable!("asserted above"), | |
| }; | |
| assert_eq!(response.end_turn, Some(false)); | |
| assert_eq!(response.usage.unwrap().into_domain(), expected); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_repo/src/provider/openai_responses/response.rs` around lines
1811 - 1817, The test uses panic! in the fallback pattern match arm for the
other variant when validating the ResponsesStreamEvent type, which violates the
assertion-driven testing guidelines. Replace the panic! call with an
assert!(matches!(...)) macro to validate that the actual event matches
ResponsesStreamEvent::ResponseCompleted, making this consistent with similar
assertion patterns used elsewhere in the test file.
Source: Coding guidelines
Comment on lines
+888
to
+903
| #[test] | ||
| fn test_ambient_config() { | ||
| let configs = get_provider_configs(); | ||
| let config = configs | ||
| .iter() | ||
| .find(|c| c.id == ProviderId::AMBIENT) | ||
| .unwrap(); | ||
| assert_eq!(config.id, ProviderId::AMBIENT); | ||
| assert_eq!(config.api_key_vars, Some("AMBIENT_API_KEY".to_string())); | ||
| assert!(config.url_param_vars.is_empty()); | ||
| assert_eq!(config.response_type, Some(ProviderResponse::OpenAI)); | ||
| assert_eq!( | ||
| config.url.as_str(), | ||
| "https://api.ambient.xyz/v1/chat/completions" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Refactor test_ambient_config to the required test-step pattern.
Please restructure this test into explicit setup/actual/expected phases and use actual/expected variable naming consistently to match repository test rules.
As per coding guidelines, "All tests should use three discrete steps: setup, actual, and expected with assertions using pretty_assertions::assert_eq" and "Use words like fixture, actual and expected in test function variable names".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/forge_repo/src/provider/provider_repo.rs` around lines 888 - 903,
Restructure the test_ambient_config function into three explicit phases with
clear variable naming. In the setup phase, initialize the configs and retrieve
the ambient configuration. In the actual phase, extract all the values being
tested (id, api_key_vars, url_param_vars, response_type, url) into variables
with names prefixed like actual_id, actual_api_key_vars, etc. In the expected
phase, define the expected values for each field and use
pretty_assertions::assert_eq to compare each actual value against its
corresponding expected value, ensuring all assertions follow the repository's
test pattern guidelines.
Source: Coding guidelines
Comment on lines
+18
to
+22
| "auto_install_vscode_extension": { | ||
| "description": "Enables automatic VS Code extension installation when Forge runs inside\nVS Code and the extension is not already installed.", | ||
| "type": "boolean", | ||
| "default": false | ||
| }, |
There was a problem hiding this comment.
Schema default conflicts with actual runtime default.
auto_install_vscode_extension is documented here as default: false, but defaults resolve to true (crates/forge_config/.forge.toml Line 35 and config tests). Update schema default to keep contract consistency.
Proposed fix
"auto_install_vscode_extension": {
"description": "Enables automatic VS Code extension installation when Forge runs inside\nVS Code and the extension is not already installed.",
"type": "boolean",
- "default": false
+ "default": true
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forge.schema.json` around lines 18 - 22, The auto_install_vscode_extension
property in the schema has a default value of false, but the actual runtime
default is true as confirmed in the forge configuration and tests. Update the
default value for the auto_install_vscode_extension property from false to true
in the schema to ensure consistency between the documented schema contract and
the actual runtime behavior.
Comment on lines
+121
to
+123
| function _forge_action_conversation_tree() { | ||
| _forge_select conversation --parent "$_FORGE_CONVERSATION_ID" | ||
| } |
There was a problem hiding this comment.
conversation-tree discards the selected conversation and never switches context.
The action invokes the picker but does not store the chosen ID, update _FORGE_CONVERSATION_ID, or display/switch into that session. Add selection handling (and an early guard when no active conversation exists).
Suggested fix
function _forge_action_conversation_tree() {
- _forge_select conversation --parent "$_FORGE_CONVERSATION_ID"
+ if [[ -z "$_FORGE_CONVERSATION_ID" ]]; then
+ _forge_log error "No active conversation. Start a conversation first."
+ return 0
+ fi
+
+ local conversation_id
+ conversation_id=$(_forge_select conversation --parent "$_FORGE_CONVERSATION_ID")
+
+ if [[ -n "$conversation_id" ]]; then
+ _forge_switch_conversation "$conversation_id"
+ echo
+ _forge_exec conversation show "$conversation_id"
+ _forge_exec conversation info "$conversation_id"
+ _forge_log success "Switched to subagent \033[1m${conversation_id}\033[0m"
+ fi
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell-plugin/lib/actions/conversation.zsh` around lines 121 - 123, The
`_forge_action_conversation_tree()` function calls the picker via
`_forge_select` but discards the result without storing or switching to the
selected conversation. Add an early guard clause to check if an active
conversation exists (validate `_FORGE_CONVERSATION_ID`), capture the result from
the `_forge_select conversation --parent` call into a variable, update
`_FORGE_CONVERSATION_ID` with the selected ID, and then display or switch
context into the selected conversation session.
| /// Creates a new ToolCallContext with default values | ||
| pub fn new(metrics: Metrics) -> Self { | ||
| Self { sender: None, metrics: Arc::new(Mutex::new(metrics)) } | ||
| Self { sender: None, metrics: Arc::new(Mutex::new(metrics)), conversation_id: None, parent_id: None, source: None } |
There was a problem hiding this comment.
CRITICAL: This single-line struct initializer exceeds rustfmt's default max_width (100 cols) — cargo fmt --check will fail. The new fields force a multi-line layout.
Suggested change
| Self { sender: None, metrics: Arc::new(Mutex::new(metrics)), conversation_id: None, parent_id: None, source: None } | |
| Self { | |
| sender: None, | |
| metrics: Arc::new(Mutex::new(metrics)), | |
| conversation_id: None, | |
| parent_id: None, | |
| source: None, | |
| } |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 New Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Existing inline review comments (21 from prior reviewers) already cover the major concerns:
Files Reviewed (63 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · 2,277,953 tokens |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a hierarchical session viewer to Forge. It solves a real pain point in the
session history: subagent traces from the orchestrator were polluting the conversation
list, making it hard to find the user's own sessions. It also adds search, pagination,
and a couple of new CLI commands.
Problem
When a user runs Forge, the orchestrator spawns subagents (e.g.
forge_app/orch.rs,agent_executor.rs) that each open their own conversation rows. Before this PR, everyone of those rows showed up in the
:conversationpicker. The list quickly became a mixof user-initiated sessions and dozens of subagent traces, with no way to distinguish them
or drill into a parent's children.
Solution
Add hierarchical linking between conversations and filter the picker to user-initiated
sessions by default. New commands expose the children of a session and the loop/goal
state, and an FTS5-backed search index makes it possible to find a session by content.
Key Changes
parent_idcolumn onconversationsfor hierarchical linking. Subagent sessionsnow point at the parent that spawned them; user-initiated sessions have
parent_id = NULL.:conversationnow shows only user-initiated parents (whereparent_id IS NULL).Implemented in
forge_main::uiand applied toSelectCommand::Conversationinforge_main::ui.rs(the post-merge fix on7065903a0).:subagents <id>and:parent <id>commands to walk the tree down and up.(
up.sqlmigration adds the index and triggers). The repo exposes asearchqueryin
conversation_repo.rs.sourcecolumn onconversationsso we can tell apart-p/headless sessionsfrom interactive ones. The
ToolCallContextcarries the source through to the repo.:goal <id>and:loop <id>commands for inspecting and toggling theloop/goal state of a session.
ConversationSelectoroptimization inforge_main::conversation_selector.rsfor paginated loading of large histories.
upstream/main(commit34cfeb666/v2.13.11): the
:conversationfilter is now correctly applied toSelectCommand::Conversationas well, not just the picker entry point.Files Changed
Branch HEAD:
7065903a0. Upstream base:34cfeb666(v2.13.11).Testing
cargo check— clean across the workspace.cargo build— succeeds; binary produced at~/.local/bin/forge-devand smoke-runlocally.
:conversationlists only user-initiated sessions.:subagents <id>lists the children of a parent.:parent <id>jumps to the parent.up.sqlfiles apply cleanly on a fresh SQLite database and thedown.sqlfiles roll back symmetrically.Commits