Add plain REPL sessions, model catalog, CLI subcommands, and tool approval policy#6
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Forge PR ReviewError: No API key for provider 'zai'. Pass --api-key or set ZAI_API_KEY. (Use --provider mock for offline runs.) |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let provider = create_provider_instance(&provider_name, &model, &api_key)?; | ||
| let _ = context; | ||
| let context = context::ContextEngine::new(&project_path)?; |
There was a problem hiding this comment.
🚩 launch_plain_mode discards the caller's Arc<Mutex>
In forge-cli/src/main.rs:856-858, the context parameter (an Arc<Mutex<ContextEngine>> potentially wrapped by a file watcher) is immediately discarded with let _ = context; and a fresh ContextEngine is created. If --watch was enabled, the file watcher task continues updating the discarded context object while the REPL uses an independent one. This doesn't cause a crash but means file watching is silently ineffective in plain mode.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn serve(config: ServeConfig) -> Result<()> { | ||
| let listener = TcpListener::bind(&config.bind) | ||
| .with_context(|| format!("failed to bind Forge local server at {}", config.bind))?; | ||
| println!("Forge local server listening on http://{}", config.bind); | ||
| println!("Routes: {}", ROUTES.join(", ")); | ||
|
|
||
| for stream in listener.incoming() { | ||
| let stream = stream?; | ||
| handle_stream(stream, &config.project_path)?; | ||
| if config.once { | ||
| break; | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🚩 Serve module has no authentication on configurable bind address
The serve command (forge-cli/src/serve.rs:36-49) binds a plain TCP HTTP server with no authentication. The default bind address is 127.0.0.1:4545 (localhost-only), which is safe. However, the --bind flag allows binding to any address (e.g., 0.0.0.0:4545), which would expose session data and model info to the network without auth. The endpoints are read-only, so the risk is information disclosure rather than mutation. The docs say 'local read-only' but the CLI doesn't enforce localhost.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Forge PR ReviewError: No API key for provider 'zai'. Pass --api-key or set ZAI_API_KEY. (Use --provider mock for offline runs.) |
|
|
||
| async fn run_verify_command(project_path: &str, dry_run: bool, json: bool) -> Result<()> { | ||
| let workdir = std::path::Path::new(project_path); | ||
| let commands = verify::detect_verify_commands(workdir).unwrap_or_default(); |
There was a problem hiding this comment.
🟡 forge verify --dry-run uses detect_verify_commands instead of resolve_verify_commands, showing wrong commands
The run_verify_command function at forge-cli/src/main.rs:716 calls verify::detect_verify_commands(workdir) to determine which commands to display in dry-run mode and to include in the commands field of JSON reports. However, the actual execution path (line 739-740) goes through BuildVerifier::verify, which internally calls resolve_verify_commands (verify/src/verifier.rs:120). resolve_verify_commands first checks forge.toml for explicit [verify].commands before falling back to auto-detection, while detect_verify_commands only does auto-detection and ignores forge.toml entirely. This means forge verify --dry-run can report different commands than what forge verify would actually execute — e.g. a project with [verify] commands = ["make all"] in forge.toml and a Cargo.toml present would show cargo build --quiet / cargo test --quiet in dry-run, but actually run make all.
| let commands = verify::detect_verify_commands(workdir).unwrap_or_default(); | |
| let commands = verify::resolve_verify_commands(workdir).unwrap_or_default(); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| restored_state.approve_session = | ||
| policy.allow_writes.unwrap_or(false) && policy.allow_commands.unwrap_or(false); |
There was a problem hiding this comment.
🟡 Permission config collapses separate allow_writes/allow_commands into a single boolean, losing granularity
The PermissionSection config struct supports separate allow_writes and allow_commands fields, and the ToolPolicy struct in the event loop also has separate fields. However, at forge-cli/src/plain.rs:161-162, the two config values are collapsed into a single approve_session boolean via &&, and at lines 259-264, this single boolean is mapped identically to both ToolPolicy.allow_writes and ToolPolicy.allow_commands. This means a config like allow_writes=true, allow_commands=false results in both being false (because true && false = false). The user's intent to pre-approve writes while blocking commands is silently ignored.
Prompt for agents
The ReplState struct uses a single `approve_session: bool` to track both write and command permissions, but the config (PermissionSection) and the ToolPolicy both support separate allow_writes and allow_commands fields. To fix this, split `approve_session` in ReplState into two fields (e.g., `approve_writes: bool` and `approve_commands: bool`), initialize them independently from the policy at line 161-162, map them independently to ToolPolicy at lines 259-264, and update the /approve command handler to either toggle both or support per-category approval (e.g., `/approve writes`, `/approve commands`). Files affected: forge-cli/src/plain.rs (ReplState struct, run_plain_repl initialization, ToolPolicy construction, handle_command /approve branch).
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn check_context_store_parent(config: &ReleaseCheckConfig) -> ReleaseCheck { | ||
| let forge_dir = config.project_path.join(".forge"); | ||
| ReleaseCheck { | ||
| name: "forge state dir", | ||
| passed: forge_dir.exists() || config.project_path.exists(), | ||
| detail: forge_dir.display().to_string(), | ||
| } |
There was a problem hiding this comment.
🚩 release_check::check_context_store_parent always passes when project_path exists
At forge-cli/src/release_check.rs:100, the check forge_dir.exists() || config.project_path.exists() will pass as long as the project directory itself exists, even if .forge doesn't. The || makes the .forge existence check meaningless in practice since the project path almost always exists (it defaults to .). This may be intentional as a minimal sanity check, but the check name 'forge state dir' suggests it should verify .forge specifically.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit cef443e on Jun 18, 2026 9:49am UTC. |
1 participant
Motivation
Description
forge-cli/src/plain.rsimplementing a plain REPL with session logging (.forge/sessions/*.jsonl), commands like/approve,/verify,/diff,/undo, and resume/list helpers wired into the CLI via a newSessionsubcommand and--sessionflag forrepl.provider::catalogwithMODEL_CATALOG,list_models, anddefault_model, plus alist_modelsCLI command and formatted table output.EventLoop(forge-core) with aToolPolicyandwith_repl_turn/with_taskhelpers, and enforced approvals forwrite_file,diff_edit, andrun_commandinexecute_tool_with_resultto require session approval when configured.Sandbox::preview_diff_editandPatchPreview, a minimalserveHTTP server (forge-cli/src/serve.rs),release_checktooling (forge-cli/src/release_check.rs), and assorted CLI subcommands (Models,Context,Verify,Serve,ReleaseCheck), plus small refactors (task sorting, doctor checks, and exports).Testing
cargo testwhich exercised new tests inprovider::catalog,release_check,servehelpers, andforge-coreevent loop policy tests, and the test run completed successfully.provider::catalog, therelease_check::model_catalog_check_passestest, theserve::list_sessionsbehavior, and an event loop test verifying writes are blocked without approval, all of which passed.Codex Task