fix(repo): hold a checkout across an approval park — supervised write-tier deadlock (#796) - #803
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change adds task-scoped checkout retention across approval redispatches and introduces approval-gated repository publishing. It stages exact commits on host-managed refs, validates publish permissions, and pushes only approved commits. ChangesTask-origin routing and checkout lifecycle
Repository publishing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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 (6)
src/harness/repo/test.rs (1)
537-573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a tool-level test for the new
repo_checkoutreuse branch.
has_activeis covered here as a ledger method. The new early-return inRepoCheckoutTool::executeatsrc/harness/repo.rsLines 845-869 has no test. That branch decides whether an agent's committed tree survives a resumed step, and it is the branch that drops therefandprarguments.The fixtures needed already exist in this file:
context,committed_checkout, andCheckoutLedger::record.Cover three cases: reuse preserves the commit, the message names the reused commit, and a second call naming a different
refdoes not silently return the old tree.As per coding guidelines for
**/*.rs: "Add focused tests with every behavior change."🤖 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 `@src/harness/repo/test.rs` around lines 537 - 573, The existing test only covers CheckoutLedger::has_active; add focused tool-level coverage for RepoCheckoutTool::execute’s repo_checkout reuse branch using the existing context, committed_checkout, and CheckoutLedger::record fixtures. Verify reuse preserves the committed tree, the returned message identifies the reused commit, and a subsequent call with a different ref does not return the previously reused tree.Source: Coding guidelines
src/harness/repo.rs (1)
1207-1240: 🩺 Stability & Availability | 🔵 TrivialPlan the cleanup of
oc/<company>/<task>mirror refs for a denied or expired publish.
executestages the ref before the operator decides. A denied or expired approval never reachesperform_effect, so the staged ref stays in the mirror. Each published task then leaves one ref behind, and the mirror grows without bound over a long-lived company.Two options: delete the ref on the deny and expiry paths, or sweep refs under
refs/heads/oc/<company>/whose task has no live grant, in the same style assweep_orphans.🤖 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 `@src/harness/repo.rs` around lines 1207 - 1240, Trace the REPO_PUBLISH_TOOL approval lifecycle from execute through the deny and expiry handling, and remove the staged oc/<company>/<task> mirror ref whenever the publish is not approved. Implement cleanup on both denial and expiration paths, reusing the existing ref identity and cleanup conventions such as sweep_orphans where appropriate, while leaving approved publishes unchanged.src/runtime/repo_manager.rs (2)
1211-1220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
first_lineinfetch_into.
fetch_intoinlines the same "first non-empty stderr line, elsegit wrote nothing to stderr" logic. Call the new helper there so one function owns that formatting.♻️ Proposed change at lines 760-771
if !out.ok { let refs = binding.branches.join(", "); return Err(OpenCompanyError::Store(format!( - "fetching {} ({refs}) failed: {}", - binding.url, - out.stderr - .lines() - .map(str::trim) - .find(|l| !l.is_empty()) - .unwrap_or("git wrote nothing to stderr") + "fetching {} ({refs}) failed: {}", + binding.url, + first_line(&out.stderr) ))); }🤖 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 `@src/runtime/repo_manager.rs` around lines 1211 - 1220, Update fetch_into to call the existing first_line helper when formatting git stderr, removing its duplicated first-nonempty-line fallback logic while preserving the current error message behavior.
893-911: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPass
--no-auto-gcon the staging fetch.
fetch_intopasses--no-auto-gcand states the reason: checkouts alternate through the mirror's objects, so a prune must never run there. This new fetch writes into the same mirror but omits the flag. The mirror setsgc.auto=0at init, so this is defence in depth rather than a live prune, but the invariant should hold on every fetch this module runs.🔧 Proposed fix
let out = git::run( &mirror, - &["fetch", "--quiet", "--no-tags", &url, &refspec], + &["fetch", "--quiet", "--no-tags", "--no-auto-gc", &url, &refspec], None, None, )🤖 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 `@src/runtime/repo_manager.rs` around lines 893 - 911, Update the staging fetch in the publish flow around the `git::run` call to include `--no-auto-gc`, matching the existing `fetch_into` behavior. Preserve the current URL, refspec, and error handling while ensuring every module-managed fetch protects the mirror from automatic garbage collection.src/harness/build.rs (1)
1914-1959: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an exact-set assertion for the push-capable belt.
The neighbouring gates use exact-set comparisons (
the_repo_grant_adds_exactly_two_tools,repo_write_grant_wires_no_tool_beyond_the_read_pair). This test usescontainsonly. An exact-set assertion would also fail if a later change widens the write belt beyondrepo_publish.♻️ Proposed addition
let pushable = built_tool_names_with_repos_cap(&["repo.write"], 1, true); assert!( pushable.contains(&publish), "a push-capable `repo.write` must wire repo_publish: {pushable:?}" ); - assert!( - pushable.contains(&"repo_checkout".to_string()) - && pushable.contains(&"repo_pr".to_string()), - "the read pair must still be wired: {pushable:?}" - ); + // Exact set: the read pair plus repo_publish, and nothing else. + let mut expected = built_tool_names(&[], false); + expected.push("repo_checkout".to_string()); + expected.push("repo_pr".to_string()); + expected.push(publish.clone()); + expected.sort(); + assert_eq!( + pushable, expected, + "a push-capable `repo.write` belt must be the read pair plus repo_publish" + );🤖 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 `@src/harness/build.rs` around lines 1914 - 1959, Update repo_write_with_a_push_capable_credential_wires_repo_publish to assert the complete expected tool set for the push-capable repo.write case, not merely contains checks. Use the neighboring exact-set assertion pattern to require repo_publish plus the existing read pair and fail if any additional tool is wired; preserve the existing negative checks for read-only credentials, bare repo, and plaintext backends.src/runtime/repo_manager/test.rs (1)
1211-1229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the multi-segment case inside the namespace.
assert_publish_branchchecks only theoc/<company>/prefix and thenvalidate_ref, which permits/. A branch such asoc/acme/a/bis therefore accepted, whilestage_publishcan never generate one. Add a row that records the intended answer, so a later tightening or loosening ofassert_publish_branchis a deliberate change.🤖 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 `@src/runtime/repo_manager/test.rs` around lines 1211 - 1229, Extend the invalid-branch cases in the test covering push_published to include a multi-segment branch such as oc/acme/a/b, asserting it returns OpenCompanyError::InvalidRequest. Keep the existing valid-shaped commit ID and error-message assertion unchanged so the test records the intended rejection behavior in assert_publish_branch.
🤖 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 `@src/harness/brain.rs`:
- Around line 399-419: Update the orphan-sweep flow around
CheckoutLedger::sweep_orphans so tasks with unresolved approvals are treated as
live and their retained checkouts are preserved until resolution. Use the
approval gate or journal’s pending-approval state in addition to
GrantSet::any_for_task, and add coverage that parks an approval, performs an
unrelated claim, and verifies the checkout remains.
In `@src/harness/build.rs`:
- Around line 1903-1908: Update the doc comment for the test containing
built_tool_names_with_repos and the write_granted assertion to remove the
obsolete claim that repo.write wires no tools or that repo_publish is pending;
state that a non-push-capable credential wires only the read pair.
In `@src/harness/repo.rs`:
- Around line 1193-1205: Update the Err branch handling stage_publish in the
repository publish flow to log the underlying error for diagnostics, while
returning a bounded, path-safe ToolResult error message to the agent instead of
interpolating err. Preserve the existing successful staging behavior and use the
file’s established clone-error messaging pattern.
- Around line 845-869: Update the reuse guard around has_active(&dest) to
compare the requested binding ref/pr with the reused checkout’s current ref
before returning success. If they differ, refuse the reuse and return an error
instead of proceeding with the earlier tree; otherwise include the tree’s
current ref and the requested ref in the success message while preserving reuse
for matching requests.
In `@src/runtime/repo_manager.rs`:
- Around line 1205-1209: Centralize the duplicate file_url helper by reusing the
existing implementation in harness/repo.rs and removing the runtime-local copy.
Ensure file_url receives an absolute path by validating or canonicalizing
caller-provided paths from agent_workspace and RuntimeBuilder before formatting
the URL, while preserving existing checkout behavior. Add a focused test
covering rejection or canonicalization of relative checkout paths.
In `@src/runtime/repo_manager/test.rs`:
- Around line 1091-1118: Update the checkout_with_commit helper’s git clone
invocation to explicitly select the main branch, ensuring the destination starts
from refs/heads/main rather than relying on the mirror’s default HEAD. Keep the
existing clone setup and subsequent commit behavior unchanged.
---
Nitpick comments:
In `@src/harness/build.rs`:
- Around line 1914-1959: Update
repo_write_with_a_push_capable_credential_wires_repo_publish to assert the
complete expected tool set for the push-capable repo.write case, not merely
contains checks. Use the neighboring exact-set assertion pattern to require
repo_publish plus the existing read pair and fail if any additional tool is
wired; preserve the existing negative checks for read-only credentials, bare
repo, and plaintext backends.
In `@src/harness/repo.rs`:
- Around line 1207-1240: Trace the REPO_PUBLISH_TOOL approval lifecycle from
execute through the deny and expiry handling, and remove the staged
oc/<company>/<task> mirror ref whenever the publish is not approved. Implement
cleanup on both denial and expiration paths, reusing the existing ref identity
and cleanup conventions such as sweep_orphans where appropriate, while leaving
approved publishes unchanged.
In `@src/harness/repo/test.rs`:
- Around line 537-573: The existing test only covers CheckoutLedger::has_active;
add focused tool-level coverage for RepoCheckoutTool::execute’s repo_checkout
reuse branch using the existing context, committed_checkout, and
CheckoutLedger::record fixtures. Verify reuse preserves the committed tree, the
returned message identifies the reused commit, and a subsequent call with a
different ref does not return the previously reused tree.
In `@src/runtime/repo_manager.rs`:
- Around line 1211-1220: Update fetch_into to call the existing first_line
helper when formatting git stderr, removing its duplicated first-nonempty-line
fallback logic while preserving the current error message behavior.
- Around line 893-911: Update the staging fetch in the publish flow around the
`git::run` call to include `--no-auto-gc`, matching the existing `fetch_into`
behavior. Preserve the current URL, refspec, and error handling while ensuring
every module-managed fetch protects the mirror from automatic garbage
collection.
In `@src/runtime/repo_manager/test.rs`:
- Around line 1211-1229: Extend the invalid-branch cases in the test covering
push_published to include a multi-segment branch such as oc/acme/a/b, asserting
it returns OpenCompanyError::InvalidRequest. Keep the existing valid-shaped
commit ID and error-message assertion unchanged so the test records the intended
rejection behavior in assert_publish_branch.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc23c9e7-5be1-4857-8c1b-1cd00343aae4
📒 Files selected for processing (13)
docs/spec/runtime/repos.mdsrc/harness/brain.rssrc/harness/build.rssrc/harness/policy.rssrc/harness/repo.rssrc/harness/repo/test.rssrc/policy/consequence.rssrc/runtime/cycle.rssrc/runtime/grants.rssrc/runtime/journal.rssrc/runtime/repo_manager.rssrc/runtime/repo_manager/test.rssrc/server/operator.rs
9abe578 to
891a7ba
Compare
…nsai#796) The per-turn checkout janitor deletes a checkout at every turn end, so under `supervised` — where each write step parks for approval — the commit `repo_publish` needs is wiped between the parked steps and the chain deadlocks (issue tinyhumansai#796). Give `CheckoutLedger` a task-keyed retained set the janitor does not touch: `retain_for_task` moves a parked turn's checkout off the turn-scoped list, `reclaim` brings it back for the resume, `purge_task` deletes it at task end, and `sweep_orphans` reclaims the disk once no live grant names the task. `repo_checkout` reuses a reclaimed tree (`has_active`) rather than re-cloning over the agent's own commits. The wiring that calls these lands next; this is the primitive plus its tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…yhumansai#796) Carry the parked call's task on the grant (`GrantedCall::origin_task`, `StandingGrant::origin_task`), stamped at mint from the approval's `approval_task` join, and expose `GrantSet::any_for_task`. The approval re-issue then stamps the resumed task on the ledger — so `repo_publish` can name its branch, which tinyhumansai#735 could not do here — and `reclaim`s the checkout the parked step left, so the resumed commit and publish work on the same tree. Each janitor claim `sweep_orphans` any task whose approval was denied or expired; a dispatch that parks holds its checkout across the park. This is the "deleted at task end" tinyhumansai#245 §5 / tinyhumansai#247 §7 promised; a per-turn delete made it a deadlock under supervision. Covered by two brain tests (reclaim-across-park, orphan-sweep) and the ledger tests from the previous commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hats (tinyhumansai#796) Two gaps the live supervised test surfaced: 1. Batched approvals. An operator commonly approves checkout, edit, commit and publish up front, so each grant is re-issued in its own turn parking nothing new. Retaining the checkout only on a turn that parked a fresh approval dropped the tree the very turn `repo_checkout` materialized it, so the next approved step hit "Not in a git repository" and the loop was back. Hold the task's checkout across EVERY re-issue instead; `sweep_orphans` reclaims it once no live grant names the unit. 2. DM chats. The write flow deadlocked identically in a DM, where there is no card. Treat the conversation as the work unit: a grant's `origin_task` now carries the card id OR the sanitized DM thread (`approval_work_key` + `sanitize_work_segment`, `dm-`-prefixed to a safe `oc/<company>/<unit>` segment). The entire task-scoped machinery — retention, reclaim, sweep and the publish branch — then covers a DM unchanged. Tests: a two-re-issue batch keeps the checkout; sanitize maps a thread to a safe segment. 205 touched-module tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
891a7ba to
c71260e
Compare
…d re-checkout (tinyhumansai#796) Two review findings on the tinyhumansai#796 checkout-retention change: - `sweep_orphans` keyed liveness on live grants alone, but a parked-yet- unresolved approval mints no grant until the operator decides it. In that window an unrelated turn's sweep could delete the checkout the parked step is holding for its own resume — the tinyhumansai#796 deadlock, reopened one turn upstream. The shared `GrantSet` now carries a `pending` set: `park` marks the work unit, settle/deny/expire clear it, and `any_for_task` treats a pending approval as live. - `repo_checkout`'s reuse guard keyed on the repo alone, so a second checkout of the same repo at a different `ref` or `pr` silently returned the held tree. It now reuses only when the tree is on the requested ref/pr, naming the ref in the notice, and otherwise refuses — naming both the held ref and the requested one — rather than returning the wrong tree or cloning over the reclaimed commits. A resume re-issues the same call, so it still matches and reuses. Tests: a parked approval survives an unrelated sweep and is reclaimed once denied; a same-ref re-checkout reuses while a different ref/pr is refused. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai on the 6 nitpicks in the review summary:
The other five are anchored to the pre-rebase tip and land on code that isn't in this PR's post-rebase diff — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- cited commit ---'
git show --stat --oneline 500b0ebf
git show --format=fuller --find-renames --find-copies 500b0ebf -- src/harness/repo.rs src/harness/repo/test.rs
echo '--- current PR merge base and changed files ---'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
printf 'merge-base: %s\n' "$base"
git diff --name-status "$base"...HEAD
echo '--- current PR changes in named out-of-scope files ---'
git diff --stat "$base"...HEAD -- \
src/runtime/repo_manager.rs \
src/harness/build.rs \
src/runtime/repo_manager/test.rs \
src/harness/repo.rs
echo '--- checkout reuse implementation and test references ---'
ast-grep outline src/harness/repo.rs --items all --match 'repo_checkout|CheckoutLedger' --view expanded
rg -n -C 5 \
'a_second_checkout_at_a_different_ref_is_refused_not_silently_reused|repo_checkout|reclaim|requested.*(ref|pr)|ref.*pr' \
src/harness/repo.rs src/harness/repo/test.rsLength of output: 42269 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
echo '--- `#803` diff: stage_publish and mirror-ref cleanup context ---'
git diff -U20 "$base"...HEAD -- src/harness/repo.rs |
rg -n -C 20 'stage_publish|oc/|mirror|publish|cleanup|delete|remove|refs/' || true
echo '--- current implementation context ---'
rg -n -C 20 'stage_publish\s*\(|oc/<|refs/oc|push_published|remove.*ref|delete.*ref' \
src/harness/repo.rs src/runtime/repo_manager.rs
echo '--- introducing commits for relevant current lines ---'
git blame -L 1180,1280 -- src/harness/repo.rs 2>/dev/null || trueLength of output: 82304
I will treat the five remaining review-summary nitpicks as out of scope for ✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
2 participants
Summary
Fixes the supervised-mode deadlock the write tier (#735/#736) hits in real use (issue #796): an agent can check out a repo, edit, and commit, but never publish — the working tree is deleted between every parked step, so the commit
repo_publishneeds is gone by the time it runs.Root cause. Under
supervisedevery step ofrepo_checkout→ edit →git_operationscommit →repo_publishisReach::Consequenceand parks, and each park ends the turn. TheCheckoutJanitordeletes the checkout at every turn end (and again at the next turn's claim), so the tree never survives from one parked step to the next. The tool contract — "deleted at task end" (#245 §5, #247 §7) — was never actually honoured; the janitor was per-turn.Fix. A checkout a task turn parks with is held on a task-keyed retained set the janitor does not touch, and deleted only when the task truly ends.
CheckoutLedgergainsretain_for_task/reclaim/purge_task/sweep_orphans(+has_active) beside the existing turn-scoped list.GrantedCall::origin_task,StandingGrant::origin_task), stamped at mint from the approval'sapproval_taskjoin.repo_publishcan name its branch — which feat(repo): host-siderepo_publish— namespaced branches, structural push refusals, approval-gated #735 could not do on this path) and reclaims the checkout the parked step left, so the resumed commit and publish operate on the same tree. If it parks again, the tree is held again.repo_checkoutreuses a reclaimed tree instead of re-cloning over the agent's own commits.Autonomous/
fullmode is unaffected — the whole chain runs in one un-parked turn there, so the retention never engages.Stacking
Built on #778 (
feat/735-repo-publish), which is not yet onmain, so like #784 this PR's diff shows the parent's changes too until #735 merges. The net #796 change is the two commits0c39c054(ledger primitive + tests) and103aed2d(origin-task wiring + re-issue reclaim + docs). Rebase ontomainfollows #735 merging.Test plan
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings(default)cargo clippy --features openhuman,tinycortex --all-targets -- -D warningspurge_task,sweep_orphans(live vs orphaned), retain-twicean_approved_grant_reclaims_the_task_checkout_it_resumes,an_orphaned_task_checkout_is_swept_at_the_next_janitor_claimruntime::grants,runtime::cycle,harness::repo,harness::brain— 204 passed, 0 failed (RUST_MIN_STACK=16777216, as CI sets)Closes #796.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes