fix: unbreak dev branch compilation after upstream/main merge#1
Open
Sigmachan wants to merge 9 commits into
Open
fix: unbreak dev branch compilation after upstream/main merge#1Sigmachan wants to merge 9 commits into
Sigmachan wants to merge 9 commits into
Conversation
The `dev` HEAD (merge of upstream/main) did not compile. This restores a green build and test suite without changing intended behavior. - session_control: bind `canonical_cwd` in `SessionStore::from_cwd` (referenced but never defined after the merge) - api/providers: cover `ProviderKind::Ollama` in `model_family_identity_for_kind` (Generic, matching the other OpenAI-compatible providers) - cli/main: close the unbalanced `else` block in `main()` error reporting - cli/main: restore the model-provenance subsystem the merge dropped (`ModelProvenance`, `ModelSource`; used by `status`) - cli/main: restore `stale_base_state_for` / `stale_base_json_value` (thin wrappers over the existing runtime `check_base_commit`) - cli/main: make `CliAction::HelpTopic` a struct variant carrying `output_format`, matching the parser and `print_help_topic` arity - cli/main: populate the `lifecycle` field when listing managed sessions via the existing `classify_session_lifecycle_for` - tests: `CARGO_BIN_EXE_claw` -> `CARGO_BIN_EXE_hackcode` after the binary rename Build: `cargo build --release` clean. Tests: 191 passing. Seven pre-existing failures remain (incomplete claw->hackcode help/banner strings, an mcp config error-capture gap, and a spinner-timing test) — all unrelated to compilation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extends provider model support (opencode, NVIDIA-NIM, kimi), updates Claude model aliases and token limits, implements reliable Ctrl+C interruption handling via signal-hook, canonicalizes session paths to fix symlink equivalence, adds model-provenance tracking and stale-base JSON serialization, refines terminal rendering and permission classification, introduces per-turn iteration limits with consecutive-continue capping, and migrates all test harnesses to validate against the hackcode binary. Additionally, ChangesProvider API and Model Support Extensions
CLI UX, Interruption, and Session Management
Terminal Rendering and Execution
Permission System and Tools
Test Harness and Config Migration
🎯 4 (Complex) | ⏱️ ~70 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/crates/api/src/providers/mod.rs (1)
566-581:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd test coverage for Ollama mapping.
The test verifies provider-to-identity mapping for Anthropic, OpenAi, and Xai but omits Ollama. Adding coverage for the Ollama case will prevent regression and document the intended Generic mapping.
✅ Proposed test extension
#[test] fn maps_provider_kind_to_model_family_identity() { - // given: each supported provider kind + // given: all supported provider kinds let anthropic = ProviderKind::Anthropic; let openai = ProviderKind::OpenAi; let xai = ProviderKind::Xai; + let ollama = ProviderKind::Ollama; - // when: converting provider kinds to prompt model family identities + // when: converting provider kinds to model family identities let anthropic_identity = model_family_identity_for_kind(anthropic); let openai_identity = model_family_identity_for_kind(openai); let xai_identity = model_family_identity_for_kind(xai); + let ollama_identity = model_family_identity_for_kind(ollama); // then: Anthropic stays Claude and OpenAI-compatible providers are generic assert_eq!(anthropic_identity, runtime::ModelFamilyIdentity::Claude); assert_eq!(openai_identity, runtime::ModelFamilyIdentity::Generic); assert_eq!(xai_identity, runtime::ModelFamilyIdentity::Generic); + assert_eq!(ollama_identity, runtime::ModelFamilyIdentity::Generic); }🤖 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 `@rust/crates/api/src/providers/mod.rs` around lines 566 - 581, Extend the maps_provider_kind_to_model_family_identity test to include the Ollama provider: create let ollama = ProviderKind::Ollama, call model_family_identity_for_kind(ollama), and add an assertion that the result equals runtime::ModelFamilyIdentity::Generic; modify the existing test block (maps_provider_kind_to_model_family_identity) to include these lines so Ollama mapping coverage is present alongside Anthropic/OpenAi/Xai.rust/crates/rusty-claude-cli/src/main.rs (1)
6417-6424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix model provenance resolution/source attribution in status output.
ModelProvenance::from_env_or_config_or_defaultnever emitsModelSource::Config, and on env hits it keepsresolvedas the caller-provided fallback model. This can emit inconsistent status payloads (model_source: "env"with a non-envmodel).Suggested patch
impl ModelProvenance { /// Probe the model-selection env vars; attribute to env when one is set, /// otherwise treat the resolved model as a built-in default. fn from_env_or_config_or_default(model: &str) -> Self { for key in ["HACKCODE_MODEL", "CLAW_MODEL", "ANTHROPIC_MODEL"] { if let Ok(value) = std::env::var(key) { let trimmed = value.trim(); if !trimmed.is_empty() { return Self { - resolved: model.to_string(), + resolved: resolve_model_alias_with_config(trimmed), raw: Some(trimmed.to_string()), source: ModelSource::Env, }; } } } + if let Some(config_model) = config_model_for_current_dir() { + let trimmed = config_model.trim(); + if !trimmed.is_empty() { + return Self { + resolved: resolve_model_alias_with_config(trimmed), + raw: Some(trimmed.to_string()), + source: ModelSource::Config, + }; + } + } Self { - resolved: model.to_string(), + resolved: resolve_model_alias_with_config(model), raw: None, source: ModelSource::Default, } } }Also applies to: 6481-6500
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 6417 - 6424, The status output misattributes model provenance because ModelProvenance::from_env_or_config_or_default never sets ModelSource::Config and, on env hits, leaves resolved as the fallback; replace the blind call with logic that (a) checks for a flag (model_flag_raw) first (keep current branch), (b) otherwise explicitly attempts to resolve the model from config and env in order and sets ModelProvenance { resolved: <actual found value>, raw: None, source: ModelSource::Config|ModelSource::Env } (or ModelSource::Default if neither found). Update or add a helper (e.g., ModelProvenance::resolve_from_config_or_env_or_default) and use it where the old call appears (the provenance variable here and the similar block around the 6481–6500 region) so resolved and source are consistent.
🤖 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.
Outside diff comments:
In `@rust/crates/api/src/providers/mod.rs`:
- Around line 566-581: Extend the maps_provider_kind_to_model_family_identity
test to include the Ollama provider: create let ollama = ProviderKind::Ollama,
call model_family_identity_for_kind(ollama), and add an assertion that the
result equals runtime::ModelFamilyIdentity::Generic; modify the existing test
block (maps_provider_kind_to_model_family_identity) to include these lines so
Ollama mapping coverage is present alongside Anthropic/OpenAi/Xai.
In `@rust/crates/rusty-claude-cli/src/main.rs`:
- Around line 6417-6424: The status output misattributes model provenance
because ModelProvenance::from_env_or_config_or_default never sets
ModelSource::Config and, on env hits, leaves resolved as the fallback; replace
the blind call with logic that (a) checks for a flag (model_flag_raw) first
(keep current branch), (b) otherwise explicitly attempts to resolve the model
from config and env in order and sets ModelProvenance { resolved: <actual found
value>, raw: None, source: ModelSource::Config|ModelSource::Env } (or
ModelSource::Default if neither found). Update or add a helper (e.g.,
ModelProvenance::resolve_from_config_or_env_or_default) and use it where the old
call appears (the provenance variable here and the similar block around the
6481–6500 region) so resolved and source are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ab3b56a-5872-4ec4-9ee1-070949f49757
📒 Files selected for processing (9)
rust/crates/api/src/providers/mod.rsrust/crates/runtime/src/session_control.rsrust/crates/rusty-claude-cli/src/main.rsrust/crates/rusty-claude-cli/tests/cli_flags_and_config_defaults.rsrust/crates/rusty-claude-cli/tests/compact_output.rsrust/crates/rusty-claude-cli/tests/compact_repl_panic.rsrust/crates/rusty-claude-cli/tests/mock_parity_harness.rsrust/crates/rusty-claude-cli/tests/output_format_contract.rsrust/crates/rusty-claude-cli/tests/resume_slash_commands.rs
added 5 commits
June 15, 2026 01:12
aeon intermittently opens a <think> block and never emits the closing </think> within the token budget — even on trivial turns. The server's --reasoning-parser qwen3 then can't split the output, so content comes back empty and the REPL prints (no response). Inject chat_template_kwargs.enable_thinking=false for the aeon/qwen3.5 wire model so it answers directly. Targeted by model name; no effect on Claude or other backends.
…ider is set When OPENAI_BASE_URL/OPENAI_API_KEY/ANTHROPIC_BASE_URL/ANTHROPIC_AUTH_TOKEN is configured (e.g. local vLLM aeon), the local-first Ollama path is not in use — skip the wizard and the Ollama-running requirement entirely.
…ncode-go, and NVIDIA NIM - Claude aliases updated to current ids in BOTH resolvers (cli main.rs is authoritative at parse time; api crate mod.rs mirrors it): opus->claude-opus-4-8, haiku->claude-haiku-4-5, +fable->claude-fable-5. Routed via the :8788 OAuth proxy. - opencode-go 'Zen' gateway: OpenAiCompatConfig::opencode(), OPENCODE_API_KEY/ OPENCODE_BASE_URL, routed by the opencode/ prefix (https://opencode.ai/zen/go/v1). - NVIDIA NIM: OpenAiCompatConfig::nim(), NIM_API_KEY/NIM_BASE_URL, routed by the nim/ prefix; multi-segment vendor/model ids now pass model-syntax validation. - model_token_limit: current ids (opus-4-8/fable-5 1M/128K, sonnet-4-6 1M/64K, haiku-4-5 200K/64K).
The old spinner drew via absolute cursor positioning (MoveTo/SavePosition) that silently no-ops in terminals not answering cursor-position queries, had no elapsed timer, and died at the first token -> 'nothing at all', can't tell frozen-vs-working. - Spinner: braille animation + live elapsed seconds, rendered with carriage- return + clear-line (no absolute positioning). A ticking clock makes 'slow' visibly distinct from 'frozen'. - clear_spinner_row: relative clear (was emitting stray DECSC/cursor-address escapes before the answer). - run_turn: per-turn footer 'Nk^ M v . T tok . Ss' (tokens in/out/total + wall-clock), so usage and latency are always visible. Stage 1 of the live-TUI work. Next: Esc/Ctrl+C interrupt + live token-by-token counter during streaming.
Finish the in-flight SIGINT interrupt feature and clear the pre-existing test rot on this branch. `cargo test --workspace` is now fully green (1168 passed, 0 failed) and the release binary builds clean. Ctrl+C / turn interrupt: - Replace the non-compiling raw-libc handler (undeclared dep + violated the workspace `unsafe_code = forbid`) with a signal-hook implementation: register_conditional_shutdown(SIGINT,130) + register(SIGINT) for a two-stage "first press aborts the turn, second press exits" contract. - TURN_ABORT is now a LazyLock<Arc<AtomicBool>> the handler can own; the REPL polls it to unwind cleanly to the prompt. Spinner shows "ctrl-c to stop". Correctness fixes: - tools: read_file/glob/grep required workspace-write, so read-only mode could not read; add classify_read_path_permission (ReadOnly floor). - cli: skip the first-run Ollama wizard when ANTHROPIC_API_KEY is set (previously only checked AUTH_TOKEN/BASE_URL) — fixed a 30s REPL hang. - runtime/bash: wire test.hung timeout classification into execute_bash and drop the untested 30s timeout floor (honor explicit timeouts). - api: kimi alias -> kimi-k2.5 + kimi->DashScope metadata routing; sonnet-4-6 context window 1M -> 200K (1M under-blocked the preflight); explicit OPENAI_BASE_URL beats the Ollama colon-heuristic fallback. Rebrand + drift cleanup (claw -> hackcode): - Fix space-eating help text (hackcode--resume/hackcodestatus -> spaced), .claw -> .hackcode config/session paths, CLAW(D)_* -> HACKCODE_* env vars, opus-4-6 -> opus-4-8 alias, spinner/done-message and HackCode persona system-prompt test expectations.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/crates/rusty-claude-cli/src/main.rs (3)
6577-6594:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve status model provenance from the winning source.
When no
--modelflag is passed, env vars only setsource: Env/raw;resolvedremains the parser default passed in asmodel. Config is never checked despiteModelSource::Config, soANTHROPIC_MODEL=sonnet hackcode statuscan report the default model with env provenance.Suggested fix
fn from_env_or_config_or_default(model: &str) -> Self { for key in ["HACKCODE_MODEL", "CLAW_MODEL", "ANTHROPIC_MODEL"] { if let Ok(value) = std::env::var(key) { let trimmed = value.trim(); if !trimmed.is_empty() { return Self { - resolved: model.to_string(), + resolved: resolve_model_alias_with_config(trimmed), raw: Some(trimmed.to_string()), source: ModelSource::Env, }; } } } + if let Some(config_model) = config_model_for_current_dir() + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()) + { + return Self { + resolved: resolve_model_alias_with_config(&config_model), + raw: Some(config_model), + source: ModelSource::Config, + }; + } Self { - resolved: model.to_string(), + resolved: resolve_model_alias_with_config(model), raw: None, source: ModelSource::Default, }🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 6577 - 6594, The from_env_or_config_or_default method incorrectly sets the resolved field to the model parameter (the default) instead of using the actual environment variable value that was found. When an environment variable is successfully retrieved and trimmed in the loop, the resolved field should be set to the trimmed value instead of model. This ensures the method returns the correct resolved model from the winning source rather than always defaulting to the model parameter.
8906-8927:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake Ctrl+C wake a pending stream read.
TURN_ABORTis checked only before awaitingstream.next_event(). If the provider stalls before the next event, the first Ctrl+C does not abort the turn until the HTTP stream yields or errors, which breaks the “reliable interruption” path.Suggested direction
- let next = if apply_stall_timeout && !received_any_event { + let next = if apply_stall_timeout && !received_any_event { match tokio::time::timeout(POST_TOOL_STALL_TIMEOUT, stream.next_event()).await { @@ } else { - stream.next_event().await.map_err(|error| { - RuntimeError::new(format_user_visible_api_error(&self.session_id, &error)) - })? + loop { + if turn_abort_requested() { + return Err(RuntimeError::new("interrupted by user")); + } + tokio::select! { + result = stream.next_event() => { + break result.map_err(|error| { + RuntimeError::new(format_user_visible_api_error(&self.session_id, &error)) + })?; + } + _ = tokio::time::sleep(Duration::from_millis(50)) => {} + } + } };Apply the same interruptible wait to the initial-event timeout branch so both paths observe Ctrl+C promptly.
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 8906 - 8927, The TURN_ABORT flag is checked only before awaiting stream.next_event(), meaning if the provider stalls, the first Ctrl+C won't interrupt until the stream yields. Make both the timeout branch (with POST_TOOL_STALL_TIMEOUT) and the regular branch interruptible by using tokio::select! to race stream.next_event() against a check that monitors TURN_ABORT, so Ctrl+C can promptly cancel either wait without requiring the stream to respond first. Apply this pattern consistently to both the apply_stall_timeout and non-timeout paths so interruption is reliable in all scenarios.
6974-7053:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish the help-text migration and remove dead auth commands.
These help surfaces still emit
claw,.claw, andclaw-path JSON while the CLI has moved tohackcode/.hackcode. The top-level help also advertiseshackcode loginandhackcode logout, butparse_argsrejects those commands as removed.Suggested direction
- Usage claw init [--output-format <format>] - Purpose create .claw/, .claw.json, .gitignore, and CLAUDE.md in the current project + Usage hackcode init [--output-format <format>] + Purpose create .hackcode/, .hackcode.json, .gitignore, and CLAUDE.md in the current project @@ - "usage": "claw export [--session <id|latest>] [--output <path>] [--output-format <format>]", + "usage": "hackcode export [--session <id|latest>] [--output <path>] [--output-format <format>]", @@ - "session_source": ".claw/sessions/", + "session_source": ".hackcode/sessions/", @@ - writeln!(out, " hackcode login")?; - writeln!(out, " hackcode logout")?; + writeln!(out, " # Auth: set ANTHROPIC_API_KEY or ANTHROPIC_AUTH_TOKEN")?; @@ - writeln!(out, " hackcode login")?;Also applies to: 7073-7113, 7143-7169, 10240-10241, 10326-10329
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 6974 - 7053, Update all help text in the render_help_topic function to reflect the migration from `claw` to `hackcode` command references. Replace all occurrences of command references like `claw`, `claw init`, `claw status`, etc. with their `hackcode` equivalents, and update path references from `.claw` to `.hackcode`. Additionally, remove or update any references to `hackcode login` and `hackcode logout` commands since these are no longer supported according to parse_args. Apply the same systematic updates to all other help text definitions across the codebase at the sibling locations (lines 7073-7113, 7143-7169, 10240-10241, 10326-10329) to ensure consistent terminology throughout all help surfaces.
🧹 Nitpick comments (1)
rust/crates/api/src/client.rs (1)
43-46: ⚡ Quick winAdd regression coverage for opencode/NIM config selection.
DashScope already has a test proving provider-specific metadata does not fall back to
OPENAI_API_KEY/api.openai.com; the new string-matchedOPENCODE_API_KEYandNIM_API_KEYbranches should get the same guard.🧪 Suggested test shape
fn dashscope_model_uses_dashscope_config_not_openai() { // ... } + + #[test] + fn opencode_model_uses_opencode_config_not_openai() { + let _lock = env_lock(); + let _opencode = EnvVarGuard::set("OPENCODE_API_KEY", Some("test-opencode-key")); + let _openai = EnvVarGuard::set("OPENAI_API_KEY", None); + let _base_url = EnvVarGuard::set("OPENCODE_BASE_URL", None); + + let client = ProviderClient::from_model("opencode/minimax-m3") + .expect("opencode model should build with OPENCODE_API_KEY"); + + match client { + ProviderClient::OpenAi(openai_client) => { + assert!(openai_client.base_url().contains("opencode.ai")); + } + other => panic!("Expected ProviderClient::OpenAi for opencode model, got: {other:?}"), + } + } + + #[test] + fn nim_model_uses_nim_config_not_openai() { + let _lock = env_lock(); + let _nim = EnvVarGuard::set("NIM_API_KEY", Some("test-nim-key")); + let _openai = EnvVarGuard::set("OPENAI_API_KEY", None); + let _base_url = EnvVarGuard::set("NIM_BASE_URL", None); + + let client = ProviderClient::from_model("nim/deepseek-ai/deepseek-r1") + .expect("NIM model should build with NIM_API_KEY"); + + match client { + ProviderClient::OpenAi(openai_client) => { + assert!(openai_client.base_url().contains("integrate.api.nvidia.com")); + } + other => panic!("Expected ProviderClient::OpenAi for NIM model, got: {other:?}"), + } + }🤖 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 `@rust/crates/api/src/client.rs` around lines 43 - 46, Add regression test coverage for the OPENCODE_API_KEY and NIM_API_KEY configuration branches in the client configuration selection logic. Similar to the existing DashScope test that verifies provider-specific metadata does not incorrectly fall back to OPENAI_API_KEY and api.openai.com defaults, create tests that ensure when auth_env matches OPENCODE_API_KEY it returns OpenAiCompatConfig::opencode() and when auth_env matches NIM_API_KEY it returns OpenAiCompatConfig::nim(), preventing unintended fallback to default OpenAI configuration for these providers.
🤖 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 `@rust/crates/api/src/providers/mod.rs`:
- Around line 288-290: The condition checking
`std::env::var_os("OPENAI_BASE_URL").is_some()` returns true even when the
environment variable is set to an empty string, causing Ollama models to be
incorrectly routed to OpenAI with an empty base URL. Modify the check for
OPENAI_BASE_URL to validate that not only does the variable exist, but that it
also contains a non-empty value. This way, empty values will be treated as
unset, and Ollama routing will proceed correctly instead of creating requests
with an empty base URL.
- Around line 155-158: The `opus` alias migration from `claude-opus-4-6` to
`claude-opus-4-8` is incomplete across the codebase. Update all references to
reflect this change: in USAGE.md (lines 200, 317, 334), change documentation and
config fixture references from `claude-opus-4-6` to `claude-opus-4-8`; in
rust/README.md (lines 112–113, 213), update documentation to reference
`claude-opus-4-8` instead of `claude-opus-4-6`; in
rust/crates/rusty-claude-cli/src/main.rs:59, change the DEFAULT_MODEL constant
from `claude-opus-4-6` to `claude-opus-4-8`; in
rust/crates/tools/src/lib.rs:3661, update DEFAULT_AGENT_MODEL from
`claude-opus-4-6` to `claude-opus-4-8`; in
rust/crates/rusty-claude-cli/src/main.rs:11044, ensure config fixture for
"smart" uses the consistent value; and in
rust/crates/runtime/src/config.rs:1855–1860, align test fixtures to use either
`claude-opus-4-8` or the `opus` alias consistently throughout.
In `@rust/crates/api/src/providers/openai_compat.rs`:
- Around line 1061-1063: The `model_disables_thinking` function is currently too
broad and matches all qwen3.5* and qwen3_5* wire models, causing unintended
models from DashScope/opencode/NIM to receive the non-standard
chat_template_kwargs field. Narrow the predicate to specifically match only the
local aeon variants (aeon/Qwen3.5-MTP rig) by adding an additional check to
verify the model is from the local aeon source, not just checking the model name
pattern. This ensures the enable_thinking: false setting is only applied to the
intended local aeon implementation.
In `@rust/crates/runtime/src/bash.rs`:
- Around line 177-189: The timeout operation on the command does not terminate
the child process when the timeout is exceeded because kill_on_drop is false by
default in Tokio's Command. To fix this, call kill_on_drop(true) on the command
object returned by prepare_tokio_command before passing it to the timeout call.
This ensures that when the timeout future is cancelled and dropped, the
underlying shell process is properly terminated rather than left running in the
background.
In `@rust/crates/rusty-claude-cli/src/main.rs`:
- Around line 5224-5230: The TURN_ABORT flag is not reset after handling the
interrupted turn in the turn_abort_requested() block, causing the REPL prompt to
remain in a stale "already interrupted" state for subsequent Ctrl+C presses. Add
a statement to clear the TURN_ABORT flag after the println! call that outputs
the clean interrupt message and before the return Ok(()) statement, ensuring the
flag is reset so later interrupts are handled correctly.
- Around line 1584-1594: The current Ollama setup gate checks for unrelated
environment variables rather than the actual selected provider, causing issues
where non-Ollama providers (like xAI, Qwen, opencode, NIM) still trigger Ollama
setup if their env vars aren't specifically checked. Move the conditional check
from this location (that checks env::var for OPENAI_BASE_URL, OPENAI_API_KEY,
ANTHROPIC_BASE_URL, ANTHROPIC_AUTH_TOKEN, and ANTHROPIC_API_KEY) to after the
model and provider have been resolved in the REPL/prompt paths. Replace the
environment variable checks with a single check that skips Ollama setup only
when the resolved/active provider is not Ollama, rather than checking for
presence of unrelated env vars.
- Around line 1469-1474: The model validation logic in the error handling around
the split('/') operation is rejecting valid bare model IDs like claude-opus-4-8
that should be allowed, since they are used internally after alias resolution
and documented in help text. Modify the validation condition to allow either
bare model IDs (when no slash is present) or the provider/model format, removing
the strict requirement that parts.len() < 2 must always fail. Additionally,
update the error message format string to include fable in the list of known
aliases alongside opus, sonnet, and haiku.
In `@rust/crates/rusty-claude-cli/src/render.rs`:
- Around line 150-155: The spinner thread performs a cleanup clear operation
(the execute! call with Print and Clear) that runs after a 90ms sleep, but
stop_global() only waits 50ms before returning, allowing streaming to start
before the thread's cleanup finishes. This causes the late-running clear to wipe
the first line of streamed output. Remove or guard the clearing operation (the
execute! and flush calls) so it does not run in the detached spinner thread
after stop_global() has signaled the thread to stop and returned control to the
caller.
In `@rust/crates/tools/src/lib.rs`:
- Around line 7162-7163: The assertions at lines 7162-7163 and lines 7185-7188
expect the permission denial text with quotes around 'workspace-write', but the
actual runtime permission contract uses the unquoted format `requires
workspace-write permission`. Update both assertion locations to match the actual
runtime contract text by removing the quotes around workspace-write in the
expected error messages so they align with what the enforcer actually produces.
---
Outside diff comments:
In `@rust/crates/rusty-claude-cli/src/main.rs`:
- Around line 6577-6594: The from_env_or_config_or_default method incorrectly
sets the resolved field to the model parameter (the default) instead of using
the actual environment variable value that was found. When an environment
variable is successfully retrieved and trimmed in the loop, the resolved field
should be set to the trimmed value instead of model. This ensures the method
returns the correct resolved model from the winning source rather than always
defaulting to the model parameter.
- Around line 8906-8927: The TURN_ABORT flag is checked only before awaiting
stream.next_event(), meaning if the provider stalls, the first Ctrl+C won't
interrupt until the stream yields. Make both the timeout branch (with
POST_TOOL_STALL_TIMEOUT) and the regular branch interruptible by using
tokio::select! to race stream.next_event() against a check that monitors
TURN_ABORT, so Ctrl+C can promptly cancel either wait without requiring the
stream to respond first. Apply this pattern consistently to both the
apply_stall_timeout and non-timeout paths so interruption is reliable in all
scenarios.
- Around line 6974-7053: Update all help text in the render_help_topic function
to reflect the migration from `claw` to `hackcode` command references. Replace
all occurrences of command references like `claw`, `claw init`, `claw status`,
etc. with their `hackcode` equivalents, and update path references from `.claw`
to `.hackcode`. Additionally, remove or update any references to `hackcode
login` and `hackcode logout` commands since these are no longer supported
according to parse_args. Apply the same systematic updates to all other help
text definitions across the codebase at the sibling locations (lines 7073-7113,
7143-7169, 10240-10241, 10326-10329) to ensure consistent terminology throughout
all help surfaces.
---
Nitpick comments:
In `@rust/crates/api/src/client.rs`:
- Around line 43-46: Add regression test coverage for the OPENCODE_API_KEY and
NIM_API_KEY configuration branches in the client configuration selection logic.
Similar to the existing DashScope test that verifies provider-specific metadata
does not incorrectly fall back to OPENAI_API_KEY and api.openai.com defaults,
create tests that ensure when auth_env matches OPENCODE_API_KEY it returns
OpenAiCompatConfig::opencode() and when auth_env matches NIM_API_KEY it returns
OpenAiCompatConfig::nim(), preventing unintended fallback to default OpenAI
configuration for these providers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a274b2ba-2963-44b1-bb1b-61b6bcb926d1
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.hackcode-todos.jsonrust/crates/api/src/client.rsrust/crates/api/src/providers/mod.rsrust/crates/api/src/providers/openai_compat.rsrust/crates/commands/src/lib.rsrust/crates/runtime/src/bash.rsrust/crates/runtime/src/conversation.rsrust/crates/runtime/src/session_control.rsrust/crates/rusty-claude-cli/Cargo.tomlrust/crates/rusty-claude-cli/src/init.rsrust/crates/rusty-claude-cli/src/main.rsrust/crates/rusty-claude-cli/src/render.rsrust/crates/rusty-claude-cli/tests/cli_flags_and_config_defaults.rsrust/crates/rusty-claude-cli/tests/compact_output.rsrust/crates/rusty-claude-cli/tests/compact_repl_panic.rsrust/crates/rusty-claude-cli/tests/mock_parity_harness.rsrust/crates/rusty-claude-cli/tests/output_format_contract.rsrust/crates/rusty-claude-cli/tests/resume_slash_commands.rsrust/crates/tools/src/lib.rs
💤 Files with no reviewable changes (1)
- .hackcode-todos.json
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/crates/rusty-claude-cli/tests/compact_repl_panic.rs
- rust/crates/runtime/src/session_control.rs
Comment on lines
+155
to
+158
| "opus" => "claude-opus-4-8", | ||
| "sonnet" => "claude-sonnet-4-6", | ||
| "haiku" => "claude-haiku-4-5-20251213", | ||
| "haiku" => "claude-haiku-4-5", | ||
| "fable" => "claude-fable-5", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find remaining opus 4.6 aliases/defaults after `opus` now resolves to 4.8.
# Expectation: Product defaults that should track `opus` are migrated to claude-opus-4-8;
# legacy fixtures may remain only if intentionally pinned.
rg -n -C 2 'claude-opus-4-6|resolve_model_alias\("opus"\)|"smart"\s*:' --glob '!target/**'Repository: itwizardo/hackcode
Length of output: 33429
🏁 Script executed:
cat -n rust/crates/api/src/providers/mod.rs | sed -n '150,165p'Repository: itwizardo/hackcode
Length of output: 873
Update documentation, hardcoded defaults, and config fixtures to reflect the opus alias migration from claude-opus-4-6 to claude-opus-4-8.
The opus alias now resolves to claude-opus-4-8 (line 155), but the migration is incomplete across the codebase:
- Documentation stale:
USAGE.md(lines 200, 317) andrust/README.md(lines 112–113, 213) still documentopus→claude-opus-4-6. - Hardcoded defaults unchanged:
DEFAULT_MODELinrust/crates/rusty-claude-cli/src/main.rs:59andDEFAULT_AGENT_MODELinrust/crates/tools/src/lib.rs:3661remain hardcoded toclaude-opus-4-6. - Config fixture inconsistency:
USAGE.md:334hardcodes"smart": "claude-opus-4-6"(stale);rust/crates/rusty-claude-cli/src/main.rs:11044uses"smart": "opus"(alias);rust/crates/runtime/src/config.rs:1855–1860show mixed values across test fixtures.
Update all references, documentation, and fixture defaults to use claude-opus-4-8 or the alias opus consistently.
🤖 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 `@rust/crates/api/src/providers/mod.rs` around lines 155 - 158, The `opus`
alias migration from `claude-opus-4-6` to `claude-opus-4-8` is incomplete across
the codebase. Update all references to reflect this change: in USAGE.md (lines
200, 317, 334), change documentation and config fixture references from
`claude-opus-4-6` to `claude-opus-4-8`; in rust/README.md (lines 112–113, 213),
update documentation to reference `claude-opus-4-8` instead of
`claude-opus-4-6`; in rust/crates/rusty-claude-cli/src/main.rs:59, change the
DEFAULT_MODEL constant from `claude-opus-4-6` to `claude-opus-4-8`; in
rust/crates/tools/src/lib.rs:3661, update DEFAULT_AGENT_MODEL from
`claude-opus-4-6` to `claude-opus-4-8`; in
rust/crates/rusty-claude-cli/src/main.rs:11044, ensure config fixture for
"smart" uses the consistent value; and in
rust/crates/runtime/src/config.rs:1855–1860, align test fixtures to use either
`claude-opus-4-8` or the `opus` alias consistently throughout.
Comment on lines
+288
to
+290
| if metadata.provider == ProviderKind::Ollama | ||
| && std::env::var_os("OPENAI_BASE_URL").is_some() | ||
| && openai_compat::has_api_key("OPENAI_API_KEY") |
There was a problem hiding this comment.
Treat empty OPENAI_BASE_URL as unset before overriding Ollama routing.
var_os("OPENAI_BASE_URL").is_some() is true for OPENAI_BASE_URL=, so an Ollama-shaped model plus OPENAI_API_KEY gets routed to OpenAI with an empty base URL; read_base_url then produces a /chat/completions request URL instead of using the Ollama/default route.
🐛 Proposed fix
+fn openai_base_url_is_configured() -> bool {
+ matches!(std::env::var("OPENAI_BASE_URL"), Ok(value) if !value.trim().is_empty())
+}
+
#[must_use]
pub fn detect_provider_kind(model: &str) -> ProviderKind {
if let Some(metadata) = metadata_for_model(model) {
@@
if metadata.provider == ProviderKind::Ollama
- && std::env::var_os("OPENAI_BASE_URL").is_some()
+ && openai_base_url_is_configured()
&& openai_compat::has_api_key("OPENAI_API_KEY")
{
return ProviderKind::OpenAi;
}
@@
- if std::env::var_os("OPENAI_BASE_URL").is_some() && openai_compat::has_api_key("OPENAI_API_KEY")
+ if openai_base_url_is_configured() && openai_compat::has_api_key("OPENAI_API_KEY")
{
return ProviderKind::OpenAi;
}🤖 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 `@rust/crates/api/src/providers/mod.rs` around lines 288 - 290, The condition
checking `std::env::var_os("OPENAI_BASE_URL").is_some()` returns true even when
the environment variable is set to an empty string, causing Ollama models to be
incorrectly routed to OpenAI with an empty base URL. Modify the check for
OPENAI_BASE_URL to validate that not only does the variable exist, but that it
also contains a non-empty value. This way, empty values will be treated as
unset, and Ollama routing will proceed correctly instead of creating requests
with an empty base URL.
Comment on lines
+1061
to
+1063
| if model_disables_thinking(wire_model) { | ||
| payload["chat_template_kwargs"] = json!({ "enable_thinking": false }); | ||
| } |
There was a problem hiding this comment.
Narrow the thinking-disable predicate to the local aeon variants.
model_disables_thinking currently matches every qwen3.5* / qwen3_5* wire model, so DashScope/opencode/NIM models with those names will receive the non-standard chat_template_kwargs field even though the documented target is the local aeon/Qwen3.5-MTP rig.
🐛 Proposed fix
pub fn model_disables_thinking(wire_model: &str) -> bool {
let canonical = wire_model.rsplit('/').next().unwrap_or(wire_model);
let lowered = canonical.to_ascii_lowercase();
- lowered == "aeon" || lowered.starts_with("qwen3.5") || lowered.starts_with("qwen3_5")
+ matches!(lowered.as_str(), "aeon" | "qwen3.5-mtp" | "qwen3_5_mtp")
}Also applies to: 1072-1075
🤖 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 `@rust/crates/api/src/providers/openai_compat.rs` around lines 1061 - 1063, The
`model_disables_thinking` function is currently too broad and matches all
qwen3.5* and qwen3_5* wire models, causing unintended models from
DashScope/opencode/NIM to receive the non-standard chat_template_kwargs field.
Narrow the predicate to specifically match only the local aeon variants
(aeon/Qwen3.5-MTP rig) by adding an additional check to verify the model is from
the local aeon source, not just checking the model name pattern. This ensures
the enable_thinking: false setting is only applied to the intended local aeon
implementation.
Comment on lines
177
to
+189
| let mut command = prepare_tokio_command(&input.command, &cwd, &sandbox_status, true); | ||
|
|
||
| // The model often passes timeout values thinking they're seconds (e.g. 60) | ||
| // but the parameter is in milliseconds. Enforce a 30-second floor so | ||
| // network commands (nmap, curl, ping) actually have time to finish. | ||
| // `timeout` is honored exactly as supplied (milliseconds). We deliberately do | ||
| // not floor it: an explicit timeout should mean what it says so callers can | ||
| // bound a hung `cargo test`/network probe precisely. Sensible defaults for | ||
| // long-running network commands belong at the caller, not a silent override. | ||
| let output_result = if let Some(timeout_ms) = input.timeout { | ||
| let timeout_ms = timeout_ms.max(30_000); | ||
| match timeout(Duration::from_millis(timeout_ms), command.output()).await { | ||
| Ok(result) => (result?, false), | ||
| Err(_) => { | ||
| return Ok(BashCommandOutput { | ||
| stdout: String::new(), | ||
| stderr: format!("Command exceeded timeout of {timeout_ms} ms"), | ||
| raw_output_path: None, | ||
| interrupted: true, | ||
| is_image: None, | ||
| background_task_id: None, | ||
| backgrounded_by_user: None, | ||
| assistant_auto_backgrounded: None, | ||
| dangerously_disable_sandbox: input.dangerously_disable_sandbox, | ||
| return_code_interpretation: Some(String::from("timeout")), | ||
| no_output_expected: Some(true), | ||
| structured_content: None, | ||
| persisted_output_path: None, | ||
| persisted_output_size: None, | ||
| sandbox_status: Some(sandbox_status), | ||
| }); | ||
| } | ||
| // On timeout, classify the failure (a hung `cargo test`/`pytest` | ||
| // reads differently from a slow network command) and emit structured | ||
| // provenance instead of a bare "timeout" string. | ||
| Err(_) => return Ok(timeout_output(&input, timeout_ms, sandbox_status)), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tokio process Command::output future cancellation Child drop behavior kill_on_drop documentation
💡 Result:
In the Tokio asynchronous process module, the behavior of a child process when its handle or the future representing its execution is dropped depends entirely on the configuration of the Command object [1][2]. By default, the kill_on_drop property for a Command is set to false [1][3]. When this is the case, dropping the Child handle or the future returned by Command::output does not terminate the child process; the process will continue to execute independently of the parent [1][2]. This behavior aligns with the standard library's process handling and differs from the typical expectations of the futures paradigm, where dropping a future often implies cancellation [1][4][2]. To change this behavior, you must explicitly call.kill_on_drop(true) on the Command object before spawning the process [1][3]. When kill_on_drop is enabled, the destructor of the associated Child handle or the future returned by Command::output will issue a kill signal to the child process if it has not yet exited [1][2]. Important considerations regarding this mechanism include: - Resource Cleanup (Zombies): On Unix platforms, terminated processes must be "reaped" by their parent to release system resources [4][5]. If a process is killed by the drop mechanism, it may become a "zombie" process [4][6]. Tokio makes a best-effort attempt to reap these processes in the background, but provides no guarantees regarding the speed or frequency of this cleanup [4][5]. - Recommendation: Because the drop-based cleanup is best-effort, it is recommended to avoid dropping a Child handle or the output future before it has been fully awaited if strict cleanup guarantees are necessary [1][4]. For reliable management, explicitly call child.kill.await or child.wait.await [1][3]. - Output Future Specifics: When using Command::output, the future returned will also respect the kill_on_drop setting [1][2]. If set to true, dropping the future before it resolves will kill the process [1]. If set to false, the process will continue to run [2].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/process/struct.Command.html
- 2: https://docs.rs/tokio/latest/tokio/process/
- 3: https://docs.rs/tokio/1.48.0/tokio/process/struct.Command.html
- 4: https://github.com/tokio-rs/tokio/blob/master/tokio/src/process/mod.rs
- 5: https://docs.rs/tokio/latest/tokio/process/index.html
- 6: https://docs.rs/tokio/1.19.1/tokio/process/struct.Command.html
🏁 Script executed:
# Find and read the Cargo.toml to check Tokio version
fd -name "Cargo.toml" | head -5Repository: itwizardo/hackcode
Length of output: 293
🏁 Script executed:
# Read the full function context around the timeout code
cat -n rust/crates/runtime/src/bash.rs | sed -n '150,210p'Repository: itwizardo/hackcode
Length of output: 2780
🏁 Script executed:
# Find Cargo.toml and grep for tokio version
find . -name "Cargo.toml" -type f | xargs grep -l "tokio"Repository: itwizardo/hackcode
Length of output: 245
🏁 Script executed:
# Look for the prepare_tokio_command function definition
rg "fn prepare_tokio_command" -A 20Repository: itwizardo/hackcode
Length of output: 1387
🏁 Script executed:
# Read the runtime Cargo.toml to find tokio version
cat rust/crates/runtime/Cargo.toml | grep -A 5 "tokio"Repository: itwizardo/hackcode
Length of output: 216
🏁 Script executed:
# Get the full prepare_tokio_command function
rg "fn prepare_tokio_command" -A 40 rust/crates/runtime/src/bash.rsRepository: itwizardo/hackcode
Length of output: 1308
Kill the foreground child when the timeout future is cancelled.
Line 184 times out command.output(), but line 189 returns immediately without terminating the child process. By default, Tokio's Command::kill_on_drop is false, so dropping the timed-out future leaves the shell command running. This becomes a practical issue now that tiny caller-provided timeouts are honored exactly (lines 179–182).
Set kill_on_drop(true) on the command before the timeout to ensure the process terminates:
Proposed fix
let mut command = prepare_tokio_command(&input.command, &cwd, &sandbox_status, true);
// `timeout` is honored exactly as supplied (milliseconds). We deliberately do
@@ -183,6 +183,7 @@ async fn execute_bash_async(
// not floor it: an explicit timeout should mean what it says so callers can
// bound a hung `cargo test`/network probe precisely. Sensible defaults for
// long-running network commands belong at the caller, not a silent override.
let output_result = if let Some(timeout_ms) = input.timeout {
+ command.kill_on_drop(true);
match timeout(Duration::from_millis(timeout_ms), command.output()).await {For stricter cleanup guarantees, use explicit spawn() + timeout + kill()/wait() instead of command.output().
📝 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 mut command = prepare_tokio_command(&input.command, &cwd, &sandbox_status, true); | |
| // The model often passes timeout values thinking they're seconds (e.g. 60) | |
| // but the parameter is in milliseconds. Enforce a 30-second floor so | |
| // network commands (nmap, curl, ping) actually have time to finish. | |
| // `timeout` is honored exactly as supplied (milliseconds). We deliberately do | |
| // not floor it: an explicit timeout should mean what it says so callers can | |
| // bound a hung `cargo test`/network probe precisely. Sensible defaults for | |
| // long-running network commands belong at the caller, not a silent override. | |
| let output_result = if let Some(timeout_ms) = input.timeout { | |
| let timeout_ms = timeout_ms.max(30_000); | |
| match timeout(Duration::from_millis(timeout_ms), command.output()).await { | |
| Ok(result) => (result?, false), | |
| Err(_) => { | |
| return Ok(BashCommandOutput { | |
| stdout: String::new(), | |
| stderr: format!("Command exceeded timeout of {timeout_ms} ms"), | |
| raw_output_path: None, | |
| interrupted: true, | |
| is_image: None, | |
| background_task_id: None, | |
| backgrounded_by_user: None, | |
| assistant_auto_backgrounded: None, | |
| dangerously_disable_sandbox: input.dangerously_disable_sandbox, | |
| return_code_interpretation: Some(String::from("timeout")), | |
| no_output_expected: Some(true), | |
| structured_content: None, | |
| persisted_output_path: None, | |
| persisted_output_size: None, | |
| sandbox_status: Some(sandbox_status), | |
| }); | |
| } | |
| // On timeout, classify the failure (a hung `cargo test`/`pytest` | |
| // reads differently from a slow network command) and emit structured | |
| // provenance instead of a bare "timeout" string. | |
| Err(_) => return Ok(timeout_output(&input, timeout_ms, sandbox_status)), | |
| let mut command = prepare_tokio_command(&input.command, &cwd, &sandbox_status, true); | |
| // `timeout` is honored exactly as supplied (milliseconds). We deliberately do | |
| // not floor it: an explicit timeout should mean what it says so callers can | |
| // bound a hung `cargo test`/network probe precisely. Sensible defaults for | |
| // long-running network commands belong at the caller, not a silent override. | |
| let output_result = if let Some(timeout_ms) = input.timeout { | |
| command.kill_on_drop(true); | |
| match timeout(Duration::from_millis(timeout_ms), command.output()).await { | |
| Ok(result) => (result?, false), | |
| // On timeout, classify the failure (a hung `cargo test`/`pytest` | |
| // reads differently from a slow network command) and emit structured | |
| // provenance instead of a bare "timeout" string. | |
| Err(_) => return Ok(timeout_output(&input, timeout_ms, sandbox_status)), |
🤖 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 `@rust/crates/runtime/src/bash.rs` around lines 177 - 189, The timeout
operation on the command does not terminate the child process when the timeout
is exceeded because kill_on_drop is false by default in Tokio's Command. To fix
this, call kill_on_drop(true) on the command object returned by
prepare_tokio_command before passing it to the timeout call. This ensures that
when the timeout future is cancelled and dropped, the underlying shell process
is properly terminated rather than left running in the background.
Comment on lines
1469
to
1474
| let parts: Vec<&str> = trimmed.split('/').collect(); | ||
| if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { | ||
| if parts.len() < 2 || parts.iter().any(|p| p.is_empty()) { | ||
| // #154: hint if the model looks like it belongs to a different provider | ||
| let mut err_msg = format!( | ||
| "invalid model syntax: '{}'. Expected provider/model (e.g., anthropic/claude-opus-4-6) or known alias (opus, sonnet, haiku)", | ||
| "invalid model syntax: '{}'. Expected provider/model (e.g., anthropic/claude-opus-4-8) or known alias (opus, sonnet, haiku)", | ||
| trimmed |
There was a problem hiding this comment.
Allow canonical bare model IDs, or stop documenting them.
This validation rejects bare IDs like claude-opus-4-8, claude-sonnet-4-6, and hackcode-uncensored unless they arrive through a short alias, even though alias resolution emits bare Claude IDs and help examples use bare model names. Also include fable in the error’s alias list.
Suggested fix
match trimmed {
"opus" | "sonnet" | "haiku" | "fable" => return Ok(()),
_ => {}
}
+ if trimmed == DEFAULT_MODEL || trimmed.starts_with("claude-") {
+ return Ok(());
+ }
@@
- "invalid model syntax: '{}'. Expected provider/model (e.g., anthropic/claude-opus-4-8) or known alias (opus, sonnet, haiku)",
+ "invalid model syntax: '{}'. Expected provider/model (e.g., anthropic/claude-opus-4-8) or known alias (opus, sonnet, haiku, fable)",🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 1469 - 1474, The model
validation logic in the error handling around the split('/') operation is
rejecting valid bare model IDs like claude-opus-4-8 that should be allowed,
since they are used internally after alias resolution and documented in help
text. Modify the validation condition to allow either bare model IDs (when no
slash is present) or the provider/model format, removing the strict requirement
that parts.len() < 2 must always fail. Additionally, update the error message
format string to include fable in the list of known aliases alongside opus,
sonnet, and haiku.
Comment on lines
+1584
to
+1594
| // A non-Ollama provider configured via env (OpenAI-compatible / Anthropic, | ||
| // e.g. a local vLLM) means the local-first Ollama path is not in use — skip | ||
| // the Ollama setup wizard and the local-Ollama requirement entirely. | ||
| if env::var("OPENAI_BASE_URL").is_ok() | ||
| || env::var("OPENAI_API_KEY").is_ok() | ||
| || env::var("ANTHROPIC_BASE_URL").is_ok() | ||
| || env::var("ANTHROPIC_AUTH_TOKEN").is_ok() | ||
| || env::var("ANTHROPIC_API_KEY").is_ok() | ||
| { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Gate Ollama setup on the selected provider, not unrelated env vars.
Any non-empty or even empty-present OPENAI_API_KEY/ANTHROPIC_API_KEY skips Ollama setup while the CLI may still run the default local model. Conversely, new non-Ollama routes such as xAI/Qwen/opencode/NIM still fall into the Ollama setup path if only their provider env vars are configured. Pass the resolved model/provider into this check and skip only when the active provider is not Ollama.
Suggested direction
-fn ensure_hackcode_ready() -> Result<(), Box<dyn std::error::Error>> {
- // A non-Ollama provider configured via env (OpenAI-compatible / Anthropic,
- // e.g. a local vLLM) means the local-first Ollama path is not in use — skip
- // the Ollama setup wizard and the local-Ollama requirement entirely.
- if env::var("OPENAI_BASE_URL").is_ok()
- || env::var("OPENAI_API_KEY").is_ok()
- || env::var("ANTHROPIC_BASE_URL").is_ok()
- || env::var("ANTHROPIC_AUTH_TOKEN").is_ok()
- || env::var("ANTHROPIC_API_KEY").is_ok()
- {
+fn ensure_hackcode_ready(model: &str) -> Result<(), Box<dyn std::error::Error>> {
+ if !matches!(detect_provider_kind(model), ProviderKind::Ollama) {
return Ok(());
}Then call it after resolving the effective model for REPL/prompt paths.
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 1584 - 1594, The
current Ollama setup gate checks for unrelated environment variables rather than
the actual selected provider, causing issues where non-Ollama providers (like
xAI, Qwen, opencode, NIM) still trigger Ollama setup if their env vars aren't
specifically checked. Move the conditional check from this location (that checks
env::var for OPENAI_BASE_URL, OPENAI_API_KEY, ANTHROPIC_BASE_URL,
ANTHROPIC_AUTH_TOKEN, and ANTHROPIC_API_KEY) to after the model and provider
have been resolved in the REPL/prompt paths. Replace the environment variable
checks with a single check that skips Ollama setup only when the resolved/active
provider is not Ollama, rather than checking for presence of unrelated env vars.
Comment on lines
+5224
to
+5230
| if turn_abort_requested() { | ||
| // User pressed Ctrl+C: stop the animation, print a clean | ||
| // interrupt line, and discard the turn (drop the local | ||
| // runtime so the session is unchanged) — the REPL continues. | ||
| Spinner::stop_global(); | ||
| println!("\x1b[2m ⎿ Interrupted\x1b[0m\n"); | ||
| return Ok(()); |
There was a problem hiding this comment.
Reset the interrupt flag before returning to the prompt.
After an interrupted turn, TURN_ABORT stays true until another turn starts. That leaves the REPL prompt in a stale “already interrupted” state, so a later Ctrl+C before the next submitted turn can be treated as the second-stage abort path. Clear the flag after rendering the clean interrupted line.
Suggested fix
if turn_abort_requested() {
// User pressed Ctrl+C: stop the animation, print a clean
// interrupt line, and discard the turn (drop the local
// runtime so the session is unchanged) — the REPL continues.
Spinner::stop_global();
println!("\x1b[2m ⎿ Interrupted\x1b[0m\n");
+ reset_turn_abort();
return Ok(());
}📝 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
| if turn_abort_requested() { | |
| // User pressed Ctrl+C: stop the animation, print a clean | |
| // interrupt line, and discard the turn (drop the local | |
| // runtime so the session is unchanged) — the REPL continues. | |
| Spinner::stop_global(); | |
| println!("\x1b[2m ⎿ Interrupted\x1b[0m\n"); | |
| return Ok(()); | |
| if turn_abort_requested() { | |
| // User pressed Ctrl+C: stop the animation, print a clean | |
| // interrupt line, and discard the turn (drop the local | |
| // runtime so the session is unchanged) — the REPL continues. | |
| Spinner::stop_global(); | |
| println!("\x1b[2m ⎿ Interrupted\x1b[0m\n"); | |
| reset_turn_abort(); | |
| return Ok(()); | |
| } |
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 5224 - 5230, The
TURN_ABORT flag is not reset after handling the interrupted turn in the
turn_abort_requested() block, causing the REPL prompt to remain in a stale
"already interrupted" state for subsequent Ctrl+C presses. Add a statement to
clear the TURN_ABORT flag after the println! call that outputs the clean
interrupt message and before the return Ok(()) statement, ensuring the flag is
reset so later interrupts are handled correctly.
Comment on lines
+150
to
155
| // The animation thread already wipes its line via carriage-return on | ||
| // exit; do a relative clear here too (no absolute cursor positioning, | ||
| // which emits stray DECSC/cursor-address escapes in many terminals). | ||
| let mut stdout = io::stdout(); | ||
| let row = SPINNER_ROW.load(std::sync::atomic::Ordering::SeqCst); | ||
| let _ = execute!( | ||
| stdout, | ||
| SavePosition, | ||
| MoveTo(0, row), | ||
| Clear(ClearType::CurrentLine), | ||
| RestorePosition | ||
| ); | ||
| let _ = execute!(stdout, Print("\r"), Clear(ClearType::CurrentLine)); | ||
| let _ = stdout.flush(); |
There was a problem hiding this comment.
Avoid clearing from the detached spinner thread after stop_global() returns.
Line 235 lets the thread sleep for 90 ms, while stop_global() only waits 50 ms before returning. Since streaming starts immediately after Spinner::stop_global() in main.rs, the unguarded clear on Lines 237-239 can run late and wipe the first streamed output line.
Proposed fix
- // The animation thread already wipes its line via carriage-return on
- // exit; do a relative clear here too (no absolute cursor positioning,
- // which emits stray DECSC/cursor-address escapes in many terminals).
+ // Do a single relative clear here (no absolute cursor positioning,
+ // which emits stray DECSC/cursor-address escapes in many terminals).
let mut stdout = io::stdout();
let _ = execute!(stdout, Print("\r"), Clear(ClearType::CurrentLine));
let _ = stdout.flush();
@@
- // Wipe the status line so streamed output starts on a clean line.
- let _ = execute!(stdout, Print("\r"), Clear(ClearType::CurrentLine));
- let _ = stdout.flush();
+ // Do not clear here: `stop_global`/`stop_animation` perform the
+ // single guarded clear after signalling shutdown.Also applies to: 235-239
🤖 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 `@rust/crates/rusty-claude-cli/src/render.rs` around lines 150 - 155, The
spinner thread performs a cleanup clear operation (the execute! call with Print
and Clear) that runs after a 90ms sleep, but stop_global() only waits 50ms
before returning, allowing streaming to start before the thread's cleanup
finishes. This causes the late-running clear to wipe the first line of streamed
output. Remove or guard the clearing operation (the execute! and flush calls) so
it does not run in the detached spinner thread after stop_global() has signaled
the thread to stop and returned control to the caller.
Comment on lines
7162
to
+7163
| // then | ||
| assert!(error.contains("requires workspace-write permission")); | ||
| assert!(error.contains("requires 'workspace-write' permission")); |
There was a problem hiding this comment.
Align the expected denial text with the runtime permission contract.
Line 7163 and Line 7188 now expect requires 'workspace-write' permission, but the supplied runtime permission tests and downstream mock parity harness still use requires workspace-write permission; this file propagates the enforcer reason verbatim, so these assertions will fail unless the runtime contract is updated everywhere.
Proposed fix if the runtime wording remains unquoted
- assert!(error.contains("requires 'workspace-write' permission"));
+ assert!(error.contains("requires workspace-write permission"));
@@
assert!(error
.to_string()
- .contains("requires 'workspace-write' permission"));
+ .contains("requires workspace-write permission"));Also applies to: 7185-7188
🤖 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 `@rust/crates/tools/src/lib.rs` around lines 7162 - 7163, The assertions at
lines 7162-7163 and lines 7185-7188 expect the permission denial text with
quotes around 'workspace-write', but the actual runtime permission contract uses
the unquoted format `requires workspace-write permission`. Update both assertion
locations to match the actual runtime contract text by removing the quotes
around workspace-write in the expected error messages so they align with what
the enforcer actually produces.
added 2 commits
June 15, 2026 05:59
Untrack REPL session transcripts (.hackcode/sessions/*.jsonl), the .hackcode dir, and stray .DS_Store files that were committed from a local working tree, and gitignore them so session/agent state never lands in the public repo again. Verified: none contained API keys or secrets — this is hygiene, not a leak remediation.
- rounded welcome box (render_welcome_box/banner_pad_cell) replacing the HACKCODE ASCII banner; keeps the fsociety-green accent - tool-call glyph ▶ → ⏺ - tool results render under a ⎿ tree connector (✓/✗ kept) - per-turn footer now a Claude-style status line: model · tokens↑↓ · tok · s All 198 rusty-claude-cli tests green.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/crates/rusty-claude-cli/src/main.rs (3)
6660-6664:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExtract
session_idfrom the filename, not the parent directory.For the hashed layout, the parent directory is the workspace hash; for the legacy flat layout, it is
sessions. This makesworkspace.session_idin status JSON incorrect. Use the session file stem instead.Proposed fix
"session_id": context.session_path.as_ref().and_then(|path| { - // Session files live under .hackcode/sessions/<session-id>/<file>.jsonl - // Extract the session-id directory component. - path.parent().and_then(|p| p.file_name()).map(|n| n.to_string_lossy().into_owned()) + path.file_stem() + .and_then(|name| name.to_str()) + .map(ToOwned::to_owned) }),🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 6660 - 6664, The session_id extraction is currently getting the parent directory name, which is incorrect as it returns either the workspace hash (hashed layout) or "sessions" (legacy layout) instead of the actual session ID. Fix this by extracting the filename stem from the session file itself rather than its parent directory. Modify the logic in the session_id JSON field to use the file_stem() method on the session_path to get the session ID directly from the filename, removing the unnecessary parent().and_then(|p| p.file_name()) chain.
6562-6580:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve config/env model provenance instead of only labeling it.
from_env_or_config_or_default()marks env values as the source but keepsresolvedas the caller-provided fallback, and it never checks project/HackCode config despiteModelSource::Config.hackcode statuscan therefore reporthackcode-uncensored/defaultwhile the REPL would select an env or config model.Proposed fix
fn from_env_or_config_or_default(model: &str) -> Self { for key in ["HACKCODE_MODEL", "CLAW_MODEL", "ANTHROPIC_MODEL"] { if let Ok(value) = std::env::var(key) { let trimmed = value.trim(); if !trimmed.is_empty() { return Self { - resolved: model.to_string(), + resolved: resolve_model_alias_with_config(trimmed), raw: Some(trimmed.to_string()), source: ModelSource::Env, }; } } } + if let Some(config_model) = config_model_for_current_dir() + .or_else(|| hackcode_config().and_then(|(model, _)| model)) + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()) + { + return Self { + resolved: resolve_model_alias_with_config(&config_model), + raw: Some(config_model), + source: ModelSource::Config, + }; + } Self { - resolved: model.to_string(), + resolved: resolve_model_alias_with_config(model), raw: None, source: ModelSource::Default, } }🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 6562 - 6580, The from_env_or_config_or_default method does not properly resolve the model value from environment variables or config - it marks the source correctly but keeps resolved set to the caller-provided fallback parameter instead of using the actual found value. Fix this by setting resolved to the trimmed environment variable value when one is found (not the model parameter). Additionally, the method never checks the project/HackCode config file despite having ModelSource::Config as an option, so add logic to check config sources between env and default, ensuring resolved actually reflects which model will be used rather than just labeling the source.
2255-2255:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish migrating user-facing help from
claw/.clawtohackcode/.hackcode.These help/error surfaces still direct users to old commands and paths (
claw,.claw/worker-state.json,.claw/sessions/). That conflicts with the HackCode path migration and leaves copied commands broken or misleading. The PR objective says fixtures and expected paths are being migrated from.clawconventions to.hackcodeconventions.Suggested direction
- Run: claw # start the REPL (writes state on first turn) - Or: claw prompt <text> # run one non-interactive turn - Then rerun: claw state [--output-format json]", + Run: hackcode # start the REPL (writes state on first turn) + Or: hackcode prompt <text> # run one non-interactive turn + Then rerun: hackcode state [--output-format json]", - Usage claw init [--output-format <format>] - Purpose create .claw/, .claw.json, .gitignore, and CLAUDE.md in the current project + Usage hackcode init [--output-format <format>] + Purpose create .hackcode/, .hackcode.json, .gitignore, and CLAUDE.md in the current project - Purpose read .claw/worker-state.json written by the interactive REPL or a one-shot prompt + Purpose read .hackcode/worker-state.json written by the interactive REPL or a one-shot prompt - Defaults --session latest (most recent managed session in .claw/sessions/) + Defaults --session latest (most recent managed session in .hackcode/sessions/) - "usage": "claw export [--session <id|latest>] [--output <path>] [--output-format <format>]", + "usage": "hackcode export [--session <id|latest>] [--output <path>] [--output-format <format>]", - "session_source": ".claw/sessions/", + "session_source": ".hackcode/sessions/",Also applies to: 6979-7068
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` at line 2255, Update all user-facing help and error messages in the file to replace references to the old `claw` command and `.claw` directory conventions with the new `hackcode` command and `.hackcode` directory conventions. Specifically, in the error message at line 2255 that mentions "no worker state file found", replace all instances of the `claw` command with `hackcode` and update paths from `.claw/worker-state.json` and `.claw/sessions/` to `.hackcode/worker-state.json` and `.hackcode/sessions/` respectively. Apply the same migration pattern to all other user-facing error messages and help text throughout the file, particularly in the range indicated by the "Also applies to" reference, ensuring all command examples and file path hints guide users to the new `hackcode` conventions instead of the deprecated `claw` conventions.
🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs`:
- Around line 9466-9469: The help banner text in the row() function call is
advertising a `/tools` slash command that is not actually implemented in the
REPL—only the `--tools` and `--scan` CLI flags exist. Remove the reference to
`/tools` from the help message string and keep only the features that are
actually supported by the REPL, such as `/help`, Tab completion for workflow
completions, and Shift+Enter for newline insertion.
---
Outside diff comments:
In `@rust/crates/rusty-claude-cli/src/main.rs`:
- Around line 6660-6664: The session_id extraction is currently getting the
parent directory name, which is incorrect as it returns either the workspace
hash (hashed layout) or "sessions" (legacy layout) instead of the actual session
ID. Fix this by extracting the filename stem from the session file itself rather
than its parent directory. Modify the logic in the session_id JSON field to use
the file_stem() method on the session_path to get the session ID directly from
the filename, removing the unnecessary parent().and_then(|p| p.file_name())
chain.
- Around line 6562-6580: The from_env_or_config_or_default method does not
properly resolve the model value from environment variables or config - it marks
the source correctly but keeps resolved set to the caller-provided fallback
parameter instead of using the actual found value. Fix this by setting resolved
to the trimmed environment variable value when one is found (not the model
parameter). Additionally, the method never checks the project/HackCode config
file despite having ModelSource::Config as an option, so add logic to check
config sources between env and default, ensuring resolved actually reflects
which model will be used rather than just labeling the source.
- Line 2255: Update all user-facing help and error messages in the file to
replace references to the old `claw` command and `.claw` directory conventions
with the new `hackcode` command and `.hackcode` directory conventions.
Specifically, in the error message at line 2255 that mentions "no worker state
file found", replace all instances of the `claw` command with `hackcode` and
update paths from `.claw/worker-state.json` and `.claw/sessions/` to
`.hackcode/worker-state.json` and `.hackcode/sessions/` respectively. Apply the
same migration pattern to all other user-facing error messages and help text
throughout the file, particularly in the range indicated by the "Also applies
to" reference, ensuring all command examples and file path hints guide users to
the new `hackcode` conventions instead of the deprecated `claw` conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12028de9-87db-4e99-ada9-5c2dad9931db
📒 Files selected for processing (1)
rust/crates/rusty-claude-cli/src/main.rs
Comment on lines
+9466
to
+9469
| out.push_str(&row( | ||
| "/help · /tools · Tab → workflow completions · Shift+Enter newline".to_string(), | ||
| dim, | ||
| )); |
There was a problem hiding this comment.
Don’t advertise an unsupported /tools slash command.
The banner points users to /tools, but this file only exposes --tools/--scan as CLI flags and does not handle a /tools REPL command. Link to implemented help/completion surfaces instead.
Proposed fix
out.push_str(&row(
- "/help · /tools · Tab → workflow completions · Shift+Enter newline".to_string(),
+ "/help · Tab → workflow completions · Shift+Enter newline".to_string(),
dim,
));🤖 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 `@rust/crates/rusty-claude-cli/src/main.rs` around lines 9466 - 9469, The help
banner text in the row() function call is advertising a `/tools` slash command
that is not actually implemented in the REPL—only the `--tools` and `--scan` CLI
flags exist. Remove the reference to `/tools` from the help message string and
keep only the features that are actually supported by the REPL, such as `/help`,
Tab completion for workflow completions, and Shift+Enter for newline insertion.
The interactive ConversationRuntime was built without ever setting max_iterations, so it defaulted to usize::MAX. Combined with the auto-injected "continue where you left off" on finish_reason=length (which had no cap of its own), a local model that kept emitting tool calls or kept hitting its output-token limit would loop forever. - Default max_iterations to 80 (env HACKCODE_MAX_ITERATIONS, 0 = unbounded) via max_iterations_from_env(); subagent path keeps its explicit cap of 32. - Cap consecutive length-driven auto-continues at 4, reset on real progress (a tool call), so a model that streams to the limit every turn can no longer spin indefinitely. - Add parse_max_iterations unit test.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/crates/runtime/src/conversation.rs (1)
360-360: ⚡ Quick winAdd regression coverage for capped length-limit continues.
The parser test doesn’t exercise the new safety behavior in
run_turn: repeatedStopReason("length")responses with no tool calls should stop afterMAX_CONSECUTIVE_LENGTH_CONTINUES + 1API calls. A focused test would lock down the PR’s infinite-loop fix.🧪 Proposed test coverage
fn max_iterations_defaults_parses_and_treats_zero_as_unbounded() { assert_eq!(parse_max_iterations(None), DEFAULT_MAX_ITERATIONS); assert_eq!(parse_max_iterations(Some("32")), 32); assert_eq!(parse_max_iterations(Some(" 10 ")), 10); assert_eq!(parse_max_iterations(Some("0")), usize::MAX); assert_eq!( parse_max_iterations(Some("not-a-number")), DEFAULT_MAX_ITERATIONS ); } + + #[test] + fn run_turn_caps_consecutive_length_continues_without_tool_progress() { + struct LengthLimitedApi { + calls: usize, + } + + impl ApiClient for LengthLimitedApi { + fn stream( + &mut self, + _request: ApiRequest, + ) -> Result<Vec<AssistantEvent>, RuntimeError> { + self.calls += 1; + Ok(vec![ + AssistantEvent::TextDelta(format!("chunk {}", self.calls)), + AssistantEvent::StopReason("length".to_string()), + AssistantEvent::MessageStop, + ]) + } + } + + let mut runtime = ConversationRuntime::new( + Session::new(), + LengthLimitedApi { calls: 0 }, + StaticToolExecutor::new(), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + ) + .with_max_iterations(usize::MAX); + + let summary = runtime + .run_turn("write a long answer", None) + .expect("length-limit auto-continues should stop cleanly at the cap"); + + assert_eq!(summary.iterations, MAX_CONSECUTIVE_LENGTH_CONTINUES + 1); + assert_eq!( + runtime.api_client_mut().calls, + MAX_CONSECUTIVE_LENGTH_CONTINUES + 1 + ); + assert_eq!(summary.tool_results.len(), 0); + }Also applies to: 425-437
🤖 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 `@rust/crates/runtime/src/conversation.rs` at line 360, Add a regression test in the parser tests that exercises the safety behavior of `run_turn` to verify that consecutive `StopReason("length")` responses with no tool calls stop the API calls after `MAX_CONSECUTIVE_LENGTH_CONTINUES + 1` iterations. This test should verify that the `consecutive_length_continues` counter correctly caps the number of repeated length-triggered API calls to prevent infinite loops, ensuring the PR's fix is locked down and doesn't regress in the future.
🤖 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.
Nitpick comments:
In `@rust/crates/runtime/src/conversation.rs`:
- Line 360: Add a regression test in the parser tests that exercises the safety
behavior of `run_turn` to verify that consecutive `StopReason("length")`
responses with no tool calls stop the API calls after
`MAX_CONSECUTIVE_LENGTH_CONTINUES + 1` iterations. This test should verify that
the `consecutive_length_continues` counter correctly caps the number of repeated
length-triggered API calls to prevent infinite loops, ensuring the PR's fix is
locked down and doesn't regress in the future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e2cb59-bcfb-4314-ac6d-8f7a0b179233
📒 Files selected for processing (1)
rust/crates/runtime/src/conversation.rs
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
1 participant
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.
What
The
devHEAD (theMerge remote-tracking branch 'upstream/main' into devcommit) does not compile —cargo build --releasefails before producing thehackcodebinary. This PR restores a green build and test suite without changing intended behavior: every change either re-binds a symbol the merge left dangling, restores a subsystem the merge dropped, or repairs a syntactic/arity mismatch.Fixes
Build blockers (
cargo build --release):runtime/session_control.rs—SessionStore::from_cwdreferencedcanonical_cwd, which was never bound. Canonicalizecwdthe same wayfrom_data_dirdoes.api/providers/mod.rs—model_family_identity_for_kinddid not coverProviderKind::Ollama(non-exhaustive match). Mapped toGeneric, alongside the other OpenAI-compatible providers.rusty-claude-cli/main.rs— unbalanced delimiters: the finalelseblock inmain()'s error reporting was never closed.rusty-claude-cli/main.rs— restored the model-provenance types the merge dropped (ModelProvenance,ModelSource) used by thestatuscommand (#148).rusty-claude-cli/main.rs— restoredstale_base_state_for/stale_base_json_value(#148) as thin wrappers over the existingruntime::{resolve_expected_base, check_base_commit}.rusty-claude-cli/main.rs—CliAction::HelpTopicis now a struct variant{ topic, output_format }, matching its parser construction site andprint_help_topic's two-argument signature.rusty-claude-cli/main.rs— populate thelifecyclefield when collecting managed sessions, via the existingclassify_session_lifecycle_for(restores thesaved only · dirty worktree · abandoned?signal).Test build blocker (
cargo test):rusty-claude-cli/tests/*.rs—env!("CARGO_BIN_EXE_claw")→CARGO_BIN_EXE_hackcodeafter the binary was renamed tohackcode(Cargo derives the env var from the[[bin]] name).Verification
cargo build --release— clean.cargo test -p rusty-claude-cli— 191 passing.claw→hackcodestrings in help/banner text (init_help_mentions_direct_subcommand,help_mentions_jsonl_resume_examples,startup_banner_mentions_workflow_completions), an MCP-config error-capture gap (status_degrades_gracefully_on_malformed_mcp_config_143), a boot-preflight contract test, an init artifacts test, and a spinner-timing test (render::spinner_advances_frames). Happy to fold fixes for any of these into a follow-up if useful.Summary by CodeRabbit
Bug Fixes
New Features
Tests
hackcodeand.hackcodelayout.Chores