Repository navigation
Harden runtime orchestration and provider setup - #85
Merged
Merged
Conversation
Summary: - Add provider settings split/reload support and richer doctor checks. - Harden GitHub pagination, repo scoping, and workpad review handoff. - Persist human-input blockers safely and protect seed worktree cleanup. - Extend provider runtime/setup handling for Codex, Claude, and Gemini. Rationale: - Kairastra needs reliable native orchestration across provider overrides, long GitHub comment/check lists, and interrupted human-input flows. - Cleanup must avoid discarding local workspace work while still removing completed managed worktrees when they are safe to delete. - Review handoff should require completed acceptance and validation sections, not merely any checked workpad item. Tests: - cargo fmt - cargo test - make ci - make install - /Users/dbachko/.local/bin/krstr doctor in ~/dev/kairastra-test - /Users/dbachko/.local/bin/krstr run --once in ~/dev/kairastra-test
|
Warning Review limit reached
More reviews will be available in 37 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces structured human input request/response workflows alongside a provider settings configuration system. It enables agents to pause and request operator input during issue automation, persist pending requests to workspace state, and resume work once authorized responses are provided via GitHub comments. ChangesStructured Human Input & Provider Defaults
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 6
🧹 Nitpick comments (5)
rust/src/doctor.rs (2)
269-277: ⚡ Quick winConsider using
Warnstatus when provider settings file is missing.The check returns
Passwith message "provider settings file not present", but the check is named "provider_settings_defaults" and is meant to validate presence of compact defaults. If the workflow exists (since this check runs after successful workflow load), the provider settings file is expected to exist (setup creates it).Consider either:
- Changing status to
Warnto signal that compact defaults are missing and user should re-run setup- Updating the message to clarify that missing file is acceptable (e.g., "provider settings file not present (compact defaults not in use)")
There's also no test coverage for the missing-file case, which suggests this behavior may not be intentional.
🤖 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/src/doctor.rs` around lines 269 - 277, The check_provider_settings_defaults function currently returns a DoctorCheck with DoctorStatus::Pass when the provider settings file is missing; change this to signal a warning instead and update the message to indicate compact defaults are missing (or advise re-running setup). Locate the provider_settings_path_for_workflow call and the early-return DoctorCheck in check_provider_settings_defaults and replace DoctorStatus::Pass with DoctorStatus::Warn and adjust the detail string to something like "provider settings file not present (compact defaults missing; re-run setup)". Also add or update tests covering the missing-file branch so the Warn behavior is exercised.
314-324: 💤 Low valueMinor: Unnecessary
.as_slice()call.The array already coerces to slice in this context, so
.as_slice()is redundant.🧹 Suggested simplification
let expected = [ ( "codex", - ["model", "reasoning_effort", "collaboration_mode", "fast"].as_slice(), + ["model", "reasoning_effort", "collaboration_mode", "fast"], ), ( "claude", - ["model", "reasoning_effort", "permission_mode"].as_slice(), + ["model", "reasoning_effort", "permission_mode"], ), - ("gemini", ["model", "approval_mode", "sandbox"].as_slice()), + ("gemini", ["model", "approval_mode", "sandbox"]), ];🤖 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/src/doctor.rs` around lines 314 - 324, The tuple array bound to the variable expected contains inner arrays converted to slices using unnecessary .as_slice() calls; remove the .as_slice() invocations for each inner array (the entries for "codex", "claude", and "gemini") so the arrays implicitly coerce to slices when constructing expected in function/variable expected within doctor.rs.rust/src/providers/codex/runtime.rs (1)
695-698: 💤 Low valueUnused variable extraction.
The
_idis extracted but never used. If this will be needed for future response handling, consider documenting the intent. Otherwise, remove the extraction.🤖 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/src/providers/codex/runtime.rs` around lines 695 - 698, The code extracts an unused variable named _id from payload via payload.get("id").and_then(JsonValue::as_u64)... which causes an unused variable; either remove the extraction line entirely or, if the id will be needed, store it in a named variable (e.g., id) and add a comment about planned use; update the code around the extraction in runtime.rs (the payload.get("id")/and_then(JsonValue::as_u64) call) accordingly so there are no unused bindings or silently ignored values.rust/src/providers/codex/setup.rs (1)
80-87: ⚡ Quick winExtract duplicated
env_boolto shared utility.This helper is duplicated in
gemini/setup.rs(lines 61-68). Consider extracting it to a shared module likecrate::configorcrate::providers::commonto avoid drift.🤖 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/src/providers/codex/setup.rs` around lines 80 - 87, The duplicate env_bool function should be moved to a shared utility module (e.g., create a pub fn env_bool in crate::config or crate::providers::common) and both rust/src/providers/codex/setup.rs and rust/src/providers/gemini/setup.rs should import and call that shared function instead of keeping local copies; ensure the new function signature and behavior match the original (fn env_bool(name: &str) -> Option<bool>), make it pub, add a mod or lib re-export if needed, remove the duplicated implementations from codex::setup and gemini::setup, and update use statements to point to the new module.rust/src/orchestrator.rs (1)
812-847: 🏗️ Heavy liftReplace the error-substring sentinel with a structured waiting result.
This flow only works while upstream keeps embedding
waiting_for_user_inputin a free-form error string. Any wording/context change will silently fall through to generic retry/blocker handling and respawn work instead of persisting the request. A dedicatedWorkerOutcome/finished reason would make this pause contract durable.🤖 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/src/orchestrator.rs` around lines 812 - 847, The current branch checks error.contains("waiting_for_user_input") which is brittle; change the worker execution to return a structured outcome (e.g., add a WorkerOutcome enum or a Result variant such as WaitingForUserInput) instead of embedding the sentinel in the error string, then update orchestrator.rs to match on that structured outcome rather than using error.contains; specifically adapt the code path that currently calls persist_human_input_request, hold_blocked_claim, clear_blocked_claim, and log_runtime_error to handle WorkerOutcome::WaitingForUserInput (carrying the pending request payload) so the persist_human_input_request call receives the real payload and the fallback branch is no longer driven by substring matching.
🤖 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/src/model.rs`:
- Around line 44-50: The HumanInputOption struct's boolean field is named
is_other but currently only deserializes snake_case, so incoming payloads with
camelCase isOther are ignored; update the field annotation on pub is_other: bool
in HumanInputOption to accept the provider's camelCase by adding a serde alias
and keeping the default (e.g. #[serde(default, alias = "isOther")]) so
deserialization preserves the provider's isOther flag.
In `@rust/src/orchestrator.rs`:
- Around line 1373-1381: The prompt for operator replies currently always uses
"<option>" even when a question has no choices, which confuses parsing; update
the loop over request.questions (the code using body.push_str and format! with
question.id) to choose a placeholder based on question.options.is_empty(): use
"<answer>" when empty and "<option>" otherwise, then insert that placeholder
into the body string (and keep the existing handling for option.is_other
producing "{}_other: <free text>"). Apply the same change to the other
occurrence mentioned (the similar block around the second location).
- Around line 569-575: The Finished message is using the spawn-time
workspace_hint_for_task instead of the live/updated workspace path from
RuntimeInfo/RunningEntry, causing pending-human-input to be persisted to the
wrong location; change the workspace_path field in the WorkerMessage::Finished
construction (the tx.send(...) that builds Finished) to use the runtime's
current workspace path (e.g., RuntimeInfo.running_entry.workspace_path or
runtime_info.workspace_path) falling back to workspace_hint_for_task only if the
live path is empty, and apply the same change to the other Finished branches
referenced (around the blocks at 691-693 and 776-823) so persisted requests use
the live workspace path.
In `@rust/src/runner.rs`:
- Around line 195-202: Currently the code clears pending human input immediately
when turn_number == 1 and resolved_human_input.is_some() by calling
workspace::clear_pending_human_input(&workspace.path) inside run_turn's early
path; move that operation so it only runs after the entire turn completes
successfully (i.e., after run_turn returns without error) to avoid losing
operator answers on later fallible steps. Locate the
workspace::clear_pending_human_input call (and the turn_number /
resolved_human_input check) and remove it from the early block, then invoke the
same conditional clear in the caller or final success path of run_turn (using
current_issue.identifier context if needed) so the pending-input file is only
deleted after the whole turn finishes successfully.
In `@rust/src/workspace.rs`:
- Around line 200-203: The code currently falls back to calling remove_dir_all
even when remove_managed_git_worktree has detected a managed seed worktree but
git worktree remove failed; change the match so any error indicating a managed
worktree presence is not swallowed and instead is returned (preventing the
remove_dir_all fallback). Concretely, update the match on
remove_managed_git_worktree(...) to return Err(error) for the
managed-worktree-related error variant(s) (use the existing
managed_git_worktree_safety_error or add/extend a predicate that detects
"managed worktree recognized" errors) rather than falling through to the Err(_)
=> {} arm, so that remove_dir_all is only executed when no managed-worktree
condition was detected.
- Around line 367-383: The cleanliness check in ensure_managed_worktree_clean
treats the new .kairastra/pending-input.json as an untracked change; update
ensure_managed_worktree_clean to ignore that file when interpreting git status
output by filtering status_output.stdout lines: after running git status, decode
stdout and split into lines, remove any lines whose path portion equals
".kairastra/pending-input.json" (or that refer to the
.kairastra/pending-input.json relative path), and only return
Err(managed_worktree_dirty) if there remain non-empty lines after that filter;
keep the existing error handling for git failures (status_output.status and
stderr) unchanged and reference the function ensure_managed_worktree_clean and
the literal ".kairastra/pending-input.json" in the change.
---
Nitpick comments:
In `@rust/src/doctor.rs`:
- Around line 269-277: The check_provider_settings_defaults function currently
returns a DoctorCheck with DoctorStatus::Pass when the provider settings file is
missing; change this to signal a warning instead and update the message to
indicate compact defaults are missing (or advise re-running setup). Locate the
provider_settings_path_for_workflow call and the early-return DoctorCheck in
check_provider_settings_defaults and replace DoctorStatus::Pass with
DoctorStatus::Warn and adjust the detail string to something like "provider
settings file not present (compact defaults missing; re-run setup)". Also add or
update tests covering the missing-file branch so the Warn behavior is exercised.
- Around line 314-324: The tuple array bound to the variable expected contains
inner arrays converted to slices using unnecessary .as_slice() calls; remove the
.as_slice() invocations for each inner array (the entries for "codex", "claude",
and "gemini") so the arrays implicitly coerce to slices when constructing
expected in function/variable expected within doctor.rs.
In `@rust/src/orchestrator.rs`:
- Around line 812-847: The current branch checks
error.contains("waiting_for_user_input") which is brittle; change the worker
execution to return a structured outcome (e.g., add a WorkerOutcome enum or a
Result variant such as WaitingForUserInput) instead of embedding the sentinel in
the error string, then update orchestrator.rs to match on that structured
outcome rather than using error.contains; specifically adapt the code path that
currently calls persist_human_input_request, hold_blocked_claim,
clear_blocked_claim, and log_runtime_error to handle
WorkerOutcome::WaitingForUserInput (carrying the pending request payload) so the
persist_human_input_request call receives the real payload and the fallback
branch is no longer driven by substring matching.
In `@rust/src/providers/codex/runtime.rs`:
- Around line 695-698: The code extracts an unused variable named _id from
payload via payload.get("id").and_then(JsonValue::as_u64)... which causes an
unused variable; either remove the extraction line entirely or, if the id will
be needed, store it in a named variable (e.g., id) and add a comment about
planned use; update the code around the extraction in runtime.rs (the
payload.get("id")/and_then(JsonValue::as_u64) call) accordingly so there are no
unused bindings or silently ignored values.
In `@rust/src/providers/codex/setup.rs`:
- Around line 80-87: The duplicate env_bool function should be moved to a shared
utility module (e.g., create a pub fn env_bool in crate::config or
crate::providers::common) and both rust/src/providers/codex/setup.rs and
rust/src/providers/gemini/setup.rs should import and call that shared function
instead of keeping local copies; ensure the new function signature and behavior
match the original (fn env_bool(name: &str) -> Option<bool>), make it pub, add a
mod or lib re-export if needed, remove the duplicated implementations from
codex::setup and gemini::setup, and update use statements to point to the new
module.
🪄 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: 1a7d5dfe-23bf-4768-9e1a-6a81477de076
📒 Files selected for processing (23)
.dockerignoreMakefileREADME.mdWORKFLOW.mddocs/workflow-reference.mdrust/Dockerfilerust/src/agent/events.rsrust/src/doctor.rsrust/src/github.rsrust/src/github_bootstrap.rsrust/src/model.rsrust/src/orchestrator.rsrust/src/providers/claude/setup.rsrust/src/providers/codex/config.rsrust/src/providers/codex/runtime.rsrust/src/providers/codex/setup.rsrust/src/providers/gemini/runtime.rsrust/src/providers/gemini/setup.rsrust/src/providers/mod.rsrust/src/runner.rsrust/src/setup.rsrust/src/workflow.rsrust/src/workspace.rs
💤 Files with no reviewable changes (1)
- WORKFLOW.md
Summary: - Preserve Codex isOther flags when deserializing human input options. - Persist pending human input against the live runtime workspace path. - Keep answered input state until post-turn bookkeeping succeeds. - Tighten managed worktree cleanup fallback and ignore pending input state. Rationale: - Review found retry and cleanup edge cases that could strand operator input or remove managed worktrees unsafely. - Free-text human input should be represented clearly in prompts and preserved across provider payload shapes. Tests: - cargo test human_input -- --nocapture - cargo test managed_worktree -- --nocapture - cargo test run_issue_keeps_answered_input_when_post_turn_update_fails -- --nocapture - cargo test finished_workspace_path_prefers_live_runtime_path -- --nocapture - cargo test removes_managed_git_worktree_with_only_pending_input_state -- --nocapture - make ci
|
| Filename | Overview |
|---|---|
| rust/src/github.rs | Adds paginated list/check-run fetching, extracts list_all_issue_comments helper, adds create_issue_comment public API, expands RestIssueComment with user/association/timestamp fields, and adds transition_issue_to_in_progress. Removes ProjectsV2 owner-relaxation in issue_matches_repo_scope — now always enforces both owner and repo. |
| rust/src/orchestrator.rs | Adds reconcile_pending_human_inputs on every tick, persist_human_input_request, parse_human_input_answer, find_human_input_answer, and moves stall_timeout_ms to per-entry RunningEntry. parse_human_input_answer silently rejects replies with any preamble before the command block. |
| rust/src/runner.rs | run_issue now returns RunIssueResult with outcome and optional HumanInputRequest. workpad_has_progress requires both exact 'Acceptance Criteria' and 'Validation' h3 sections fully checked — a breaking change that could leave workpads without these headings unable to trigger review handoff. |
| rust/src/workspace.rs | Adds load/write/clear_pending_human_input helpers. remove_managed_git_worktree now verifies git common dir, cleanliness (ignoring pending-input.json), and merged branch status before proceeding. Well-tested with integration-style tokio tests. |
| rust/src/workflow.rs | Adds provider-settings.json support with deep-merge semantics (WORKFLOW.md wins), mtime-based cache invalidation, and last-known-good fallback on parse errors. Thoroughly tested. |
| rust/src/model.rs | Adds HumanInputOption, HumanInputQuestion, HumanInputAnswer, PendingHumanInputStatus, HumanInputRequest, and PendingHumanInput with correct serde attributes (snake_case, isOther alias). |
| rust/src/doctor.rs | Adds check_provider_settings_defaults to doctor run; validates provider-settings.json contains expected compact keys for codex, claude, and gemini. Only warns, never fails startup. |
| rust/src/providers/mod.rs | Adds collaboration_mode, permission_mode, and sandbox fields to test setup configs to keep them in sync with expanded SetupConfig structs. |
Reviews (1): Last reviewed commit: "fix(runtime): address review feedback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1452
to
+1460
| let mut lines = comment | ||
| .body | ||
| .lines() | ||
| .map(str::trim) | ||
| .filter(|line| !line.is_empty() && !line.starts_with("```")); | ||
|
|
||
| let command = lines.next()?; | ||
| if command != format!("/kairastra answer {}", request.request_id) { | ||
| return None; |
There was a problem hiding this comment.
parse_human_input_answer strips blank lines and code-fence delimiters, then treats the very first remaining line as the /kairastra answer … command. If an operator writes any introductory text — even a single line like "Sure, here's my answer:" — before the template block, that line is consumed as the command, the check fails, and None is returned. The issue then stays in AwaitingAnswer with no feedback to the user about why.
Comment on lines
+450
to
+453
| is_workpad_comment(body) | ||
| && !is_bootstrap_workpad(body) | ||
| && workpad_section_completed(body, "Acceptance Criteria") | ||
| && workpad_section_completed(body, "Validation") |
There was a problem hiding this comment.
workpad_section_completed requires both sections to exist with at least one checkbox, all checked. A workpad that uses different section names (e.g. ### Acceptance or ### Tests) will always return false, leaving the issue permanently dispatched to the agent until max_turns is hit. Worth verifying the agent prompt template guarantees these exact h3 headings.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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.
Context
Oracle Pro review found orchestration gaps around GitHub pagination, provider reloads, human-input blockers, review handoff, and cleanup safety.
TL;DR
Hardens native runtime orchestration, provider setup, GitHub sync, and kairastra-test smoke validation.
Summary
.kairastra/provider-settings.jsonwith reload safety.Alternatives
WORKFLOW.mdas the workflow source instead of adding a new runtime state store.Test Plan
cargo fmt --checkcargo testcargo clippy --all-targets --all-features -- -D warningscargo fmtmake cimake install/Users/dbachko/.local/bin/krstr doctorfrom/Users/dbachko/dev/kairastra-test/Users/dbachko/.local/bin/krstr run --oncefrom/Users/dbachko/dev/kairastra-testSummary by CodeRabbit
Release Notes
New Features
.kairastra/provider-settings.json) to manage provider defaults centrallyImprovements