[auto] #29 automation: issue claim lease + auto-release on failed/aborted runs - #33
Merged
Conversation
…sweep
Implements time-bounded claim leases to prevent permanently blocked issues
when automation runs fail or are cancelled. Claims are stored as GitHub
issue comments with structured JSON metadata, enabling rich event history
and status tracking without label pollution.
New CLI: `tt issue-claim {acquire,heartbeat,release,sweep}`
- acquire: select + claim with configurable TTL (default 30m)
- heartbeat: renew active lease (called every 5m during workflow)
- release: explicit release on workflow completion/failure
- sweep: periodic cleanup of expired claims (scheduled every 30m)
Race-condition prevention via comment-based tie-breaking.
Workflow updated with heartbeat loop and trap-based release on exit.
Closes #29
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Rust-based GitHub issue claim system (acquire, heartbeat, release, sweep), exposes it via CLI subcommands, updates workflows to manage claim lifecycle (heartbeat, cleanup) and adds a scheduled job to sweep stale claims; replaces shell-based issue selection with the new cargo-based acquire command. Changes
Sequence DiagramsequenceDiagram
participant WF as Workflow
participant CLI as CLI (issue_claim)
participant GH as GitHub API
participant FS as Filesystem
WF->>CLI: acquire(label, lease_ttl, output_path)
CLI->>GH: list issues with label
CLI->>GH: add claim label to chosen issue
CLI->>GH: create claim comment (encoded ClaimRecord)
CLI->>GH: fetch issue body/metadata
CLI->>FS: write SelectedIssueOutput JSON
CLI-->>WF: return state
loop every 5 minutes
WF->>CLI: heartbeat(state_path)
CLI->>FS: read state file
CLI->>GH: update claim comment (renew)
CLI->>FS: update state file
end
WF->>CLI: release(state_path, reason)
CLI->>FS: read/update state
CLI->>GH: update claim comment (released)
CLI->>GH: remove label if no active claims remain
CLI->>FS: persist final state
CLI-->>WF: release complete
WF->>CLI: sweep()
CLI->>GH: list issues with automation-claimed label
CLI->>GH: inspect claim comments, identify stale claims
CLI->>GH: release stale claims and remove label if appropriate
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (5)
src/cli/issue_claim.rs (4)
339-341: Silent failure on serialization error.Using
unwrap_or_default()onserde_json::to_string(record)silently produces an empty string if serialization fails. While unlikely, this could result in a malformed claim comment that cannot be decoded, potentially causing orphaned claims. Consider propagating the error or logging a warning.🔧 Propagate serialization errors
-fn encode_claim_comment(record: &ClaimRecord) -> String { - let json = serde_json::to_string(record).unwrap_or_default(); +fn encode_claim_comment(record: &ClaimRecord) -> Result<String> { + let json = serde_json::to_string(record)?; let status_emoji = match record.status { ClaimStatus::Active => "🔒", ClaimStatus::Released => "🔓", }; - format!( + Ok(format!( "{CLAIM_MARKER_START}{json}{CLAIM_MARKER_END}\n\n\ {status_emoji} **Claim** — run `{}` | status: `{:?}` | \ expires: {} | heartbeat: {}", record.run_id, record.status, record.expires_at().format("%Y-%m-%dT%H:%M:%SZ"), record.last_heartbeat_at.format("%Y-%m-%dT%H:%M:%SZ"), - ) + )) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/issue_claim.rs` around lines 339 - 341, The function encode_claim_comment currently swallows serialization errors by using serde_json::to_string(record).unwrap_or_default(), which can produce an empty/malformed comment; change encode_claim_comment to return a Result<String, serde_json::Error> (or at least propagate the error) instead of silently defaulting, update callers to handle the Result, and/or replace unwrap_or_default() with proper error handling/logging so failures in serializing ClaimRecord are surfaced; locate the encode_claim_comment function and ClaimRecord usage to implement this change.
598-599: Non-atomic write to state file.Unlike
acquirewhich uses atomic tmp+rename,heartbeatwrites directly to the state file. While less critical, a crash during write could corrupt the state file. Consider using the same atomic pattern for consistency.🔧 Optional: Use atomic write pattern
// Also update local state let mut updated_output: serde_json::Value = serde_json::from_str(&data)?; updated_output["last_heartbeat_at"] = serde_json::Value::String(record.last_heartbeat_at.to_rfc3339()); let json = serde_json::to_string_pretty(&updated_output)?; - std::fs::write(state_path, json)?; + let tmp_path = state_path.with_extension("json.tmp"); + std::fs::write(&tmp_path, &json)?; + std::fs::rename(&tmp_path, state_path)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/issue_claim.rs` around lines 598 - 599, The direct write of serialized state (serde_json::to_string_pretty(&updated_output) -> std::fs::write(state_path, json)) can corrupt the file on crash; change to an atomic write: serialize updated_output to a temp file next to state_path (e.g., state_path.with_extension("tmp") or state_path + ".tmp"), write and flush/sync the temp file to disk, then rename (std::fs::rename) the temp file over state_path so the replace is atomic; use the same tmp+rename pattern as the acquire path to ensure consistency and durability when updating state_path.
66-68: Potential overflow iflease_ttl_secsexceedsi64::MAX.The cast
self.lease_ttl_secs as i64could overflow for values exceedingi64::MAX. While unrealistic in practice (that's ~292 billion years), consider usingi64::try_fromfor robustness or documenting the practical limit.🔧 Optional: Saturating conversion
fn expires_at(&self) -> DateTime<Utc> { - self.last_heartbeat_at + chrono::Duration::seconds(self.lease_ttl_secs as i64) + let secs = i64::try_from(self.lease_ttl_secs).unwrap_or(i64::MAX); + self.last_heartbeat_at + chrono::Duration::seconds(secs) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/issue_claim.rs` around lines 66 - 68, The cast in expires_at() uses self.lease_ttl_secs as i64 which can overflow; change the conversion to use i64::try_from(self.lease_ttl_secs) and handle the Result (e.g., clamp to i64::MAX on Err or return a sensible fallback) before passing into chrono::Duration::seconds, so expires_at() won't silently overflow for extremely large lease_ttl_secs values.
690-701: Sweep makes multiple API calls per issue without progress feedback.For each issue,
sweepcallsrelease_stale_claims(which fetches comments and potentially updates them) and thenfind_claim_commentsagain. For repositories with many claimed issues, this could be slow and hit rate limits. The currenteprintln!logging provides progress, which is good. Consider adding a brief delay between issues if rate limiting becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/issue_claim.rs` around lines 690 - 701, The loop is making two fetches per issue (release_stale_claims(...) and find_claim_comments(...)) causing extra API calls; modify release_stale_claims to return the claims it fetched (or accept a mutable Vec return parameter) so the loop can reuse that result instead of calling find_claim_comments again, then use winner_active_claim(&claims) and gh_remove_label(...) as before; additionally add a small configurable delay (e.g., sleep for a short duration) between iterations of the for number in issues loop to reduce rate-limit pressure..github/workflows/sdlc-orchestrator.yml (1)
95-98: Heartbeat starts before claim is acquired.The heartbeat loop begins immediately but the state file won't exist until
acquiresucceeds (which happens insidecargo run --quiet -- run sdlc-smoke). This works due to2>/dev/null || true, but the first heartbeat after 5 minutes may also run while acquire is still in progress. Consider starting the heartbeat loop only after the workflow has had time to acquire a claim, or check for file existence in the loop condition.🔧 Optional: Add file existence check to heartbeat loop
# Start heartbeat in background (every 5 min) - ( while true; do sleep 300; cargo run --quiet -- issue-claim heartbeat --state "$STATE_FILE" 2>/dev/null || true; done ) & + ( while true; do sleep 300; [ -f "$STATE_FILE" ] && cargo run --quiet -- issue-claim heartbeat --state "$STATE_FILE" || true; done ) &🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/sdlc-orchestrator.yml around lines 95 - 98, The heartbeat loop is started before a claim/state file exists, causing premature heartbeat attempts; modify the background loop (the "( while true; do ... issue-claim heartbeat --state \"$STATE_FILE\" ...; done ) &" block that sets HEARTBEAT_PID and trap) so it either waits until the state file exists before sending heartbeats (e.g., loop checks for file existence with [ -f "$STATE_FILE" ] before calling cargo run) or is started only after the acquire step completes (start the heartbeat after the command that runs sdlc-smoke/acquire finishes successfully); keep the kill-on-exit trap and HEARTBEAT_PID logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/issue_claim.rs`:
- Around line 494-498: The label is added with gh_add_label(&repo, number,
"automation-claimed") before creating the claim comment, so if
gh_create_comment(&repo, number, &comment_body) fails the label will remain;
update the flow around these calls to catch a failure from gh_create_comment,
call gh_remove_label(&repo, number, "automation-claimed") (or equivalent cleanup
function) to remove the label, then propagate the original error; reference the
two symbols gh_add_label and gh_create_comment and ensure cleanup runs only on
comment-creation failure to avoid leaving stale labels.
---
Nitpick comments:
In @.github/workflows/sdlc-orchestrator.yml:
- Around line 95-98: The heartbeat loop is started before a claim/state file
exists, causing premature heartbeat attempts; modify the background loop (the "(
while true; do ... issue-claim heartbeat --state \"$STATE_FILE\" ...; done ) &"
block that sets HEARTBEAT_PID and trap) so it either waits until the state file
exists before sending heartbeats (e.g., loop checks for file existence with [ -f
"$STATE_FILE" ] before calling cargo run) or is started only after the acquire
step completes (start the heartbeat after the command that runs
sdlc-smoke/acquire finishes successfully); keep the kill-on-exit trap and
HEARTBEAT_PID logic unchanged.
In `@src/cli/issue_claim.rs`:
- Around line 339-341: The function encode_claim_comment currently swallows
serialization errors by using serde_json::to_string(record).unwrap_or_default(),
which can produce an empty/malformed comment; change encode_claim_comment to
return a Result<String, serde_json::Error> (or at least propagate the error)
instead of silently defaulting, update callers to handle the Result, and/or
replace unwrap_or_default() with proper error handling/logging so failures in
serializing ClaimRecord are surfaced; locate the encode_claim_comment function
and ClaimRecord usage to implement this change.
- Around line 598-599: The direct write of serialized state
(serde_json::to_string_pretty(&updated_output) -> std::fs::write(state_path,
json)) can corrupt the file on crash; change to an atomic write: serialize
updated_output to a temp file next to state_path (e.g.,
state_path.with_extension("tmp") or state_path + ".tmp"), write and flush/sync
the temp file to disk, then rename (std::fs::rename) the temp file over
state_path so the replace is atomic; use the same tmp+rename pattern as the
acquire path to ensure consistency and durability when updating state_path.
- Around line 66-68: The cast in expires_at() uses self.lease_ttl_secs as i64
which can overflow; change the conversion to use
i64::try_from(self.lease_ttl_secs) and handle the Result (e.g., clamp to
i64::MAX on Err or return a sensible fallback) before passing into
chrono::Duration::seconds, so expires_at() won't silently overflow for extremely
large lease_ttl_secs values.
- Around line 690-701: The loop is making two fetches per issue
(release_stale_claims(...) and find_claim_comments(...)) causing extra API
calls; modify release_stale_claims to return the claims it fetched (or accept a
mutable Vec return parameter) so the loop can reuse that result instead of
calling find_claim_comments again, then use winner_active_claim(&claims) and
gh_remove_label(...) as before; additionally add a small configurable delay
(e.g., sleep for a short duration) between iterations of the for number in
issues loop to reduce rate-limit pressure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca8ccb6b-9174-4910-846a-a1c6449c0a3b
📒 Files selected for processing (6)
.github/workflows/sdlc-orchestrator.ymldocs/examples/tutti-codex-sdlc.tomlsrc/cli/issue_claim.rssrc/cli/mod.rssrc/error.rssrc/main.rs
Contributor
Author
|
Addressed CodeRabbit findings in 9024557:
Please re-review. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/issue_claim.rs`:
- Around line 214-234: gh_remove_label currently swallows all failures from the
`gh issue edit` command and always returns Ok, which hides real failures; change
gh_remove_label to propagate errors when the command fails but treat the
explicit idempotent “label absent” case as non-fatal by inspecting stderr.
Specifically, in function gh_remove_label: after running the command, if
out.status is not success check out.stderr (String::from_utf8_lossy) for known
“label not found/does not exist” messages (or the exact gh CLI phrasing) and
return Ok only in that case; otherwise return an Err (e.g., using anyhow::anyhow
or the crate’s Result error constructor) including the stderr text so callers
can detect and log real failures. Ensure the signature and error types remain
consistent with existing Result in this module.
- Around line 47-54: The constructor new currently sets claimed_at using
Utc::now(), which lets each runner's clock affect claim ordering; instead,
remove reliance on local time for race-winning ordering and set claimed_at to a
deterministic server-ordered value (or leave it null/zero) and use the issue
comment ordering logic (lowest comment_id) when determining the winner; update
the new function (and any other places that set claimed_at such as the other
occurrence around lines 411-416) to avoid stamping with Utc::now() and ensure
claim ordering is performed by comparing comment_id, while still retaining
last_heartbeat_at and lease_ttl_secs behavior.
- Around line 649-651: The early return when record.status ==
ClaimStatus::Released (the if block that prints "claim already released for #{}"
and returns Ok(())) prevents label reconciliation and rewriting
selected_issue.json; remove the premature return so a "already released" case
falls through to the existing cleanup code that reconciles labels and updates
local state (the same change must be applied to the analogous block around the
669-680 region). In practice, keep the log message but do not return from the
function when record.status == ClaimStatus::Released; allow execution to
continue to the label reconciliation and the code that rewrites
selected_issue.json so the automation-claimed label can be cleared and local
state becomes consistent.
- Around line 488-526: The current logic stops the whole acquisition when a
created claim loses the race: in the loop over candidates (variables candidates,
issue, number) after creating a ClaimRecord (ClaimRecord::new) and posting a
comment (encode_claim_comment, gh_add_label, gh_create_comment), the code checks
winner_active_claim and if we lost it updates our comment (our_record,
gh_update_comment) then returns Err(TuttiError::IssueClaim...), which aborts
processing remaining candidates; change this so after marking our_record
released, updating the comment, and removing any labels as needed
(gh_remove_label already used on create failure), the function continues to the
next candidate instead of returning—i.e., replace the early return with logic to
clean up and continue iterating through candidates so other unclaimed issues can
still be attempted.
- Around line 302-317: gh_list_comments currently calls the GitHub CLI with
"--paginate" and then deserializes out.stdout with serde_json::from_slice, which
fails on JSON text sequences; modify gh_list_comments to call gh api with
"--paginate" and "--slurp" (or replace "--paginate" with "--limit" if you prefer
a single large page) so the CLI returns a single JSON document (nested arrays),
then deserialize and flatten the outer array into Vec<serde_json::Value> before
returning; update error handling around serde_json::from_slice and keep the same
TuttiError::IssueClaim propagation if parsing fails.
- Around line 265-273: The fallback loops in gh_claim_comment_id and
find_claim_comments currently accept any comment containing CLAIM_MARKER_START;
update both to verify the comment author and context before returning the id by
checking c["user"]["login"] (or c["user"]["type"] for bot accounts) matches the
expected automation identity and/or that the comment contains the current run's
run_id (e.g., compare to the run id available in the claim flow), and only
return the comment id when those validations pass; use gh_list_comments as the
source but filter each comment using these checks in the same loop that inspects
CLAIM_MARKER_START so spoofed comments are ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e614771-05fb-47a4-bf49-763dada04a9d
📒 Files selected for processing (2)
.github/workflows/sdlc-orchestrator.ymlsrc/cli/issue_claim.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/sdlc-orchestrator.yml
Contributor
Author
|
On it — pushed follow-up commit 5619727 to address the latest CodeRabbit findings:
Tests:
Please re-review. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli/issue_claim.rs (2)
139-165: Consider documenting or increasing the issue limit.The
--limit 100cap means repositories with more than 100 matching open issues will have some candidates silently ignored. This is likely acceptable for most use cases, but consider either documenting this limitation or using pagination for completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/issue_claim.rs` around lines 139 - 165, The gh_list_issues function currently uses a hard-coded "--limit 100" which silently omits matching issues beyond 100; either document this limitation in the function comment or remove the static cap and implement pagination: update gh_list_issues to call the GitHub CLI repeatedly (or use the GraphQL/REST API) to fetch all pages until exhausted, aggregating serde_json::Value items before returning, and ensure errors from each Command::new("gh") call are handled similarly to the existing out.status check.
718-722: Use atomic write pattern for consistency.The
releasefunction writes the local state file directly withstd::fs::write(line 722), whileacquireandheartbeatuse the safer temp-file-plus-rename pattern. This inconsistency could lead to corrupted state files if the process is interrupted mid-write.♻️ Proposed fix
// Update local state let mut updated: serde_json::Value = serde_json::from_str(&data)?; updated["claim_status"] = serde_json::Value::String("released".into()); let json = serde_json::to_string_pretty(&updated)?; - std::fs::write(state_path, json)?; + let tmp_path = state_path.with_extension("json.tmp"); + std::fs::write(&tmp_path, &json)?; + std::fs::rename(&tmp_path, state_path)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/issue_claim.rs` around lines 718 - 722, The release code updates `updated` and currently calls `std::fs::write(state_path, json)?`; change this to the same atomic temp-file-and-rename pattern used by `acquire`/`heartbeat`: create a temp path (e.g. `state_path` + ".tmp" or using a unique suffix), write the pretty JSON into that temp file, flush and sync if the other functions do so, then rename the temp into `state_path` with `std::fs::rename`; keep existing error propagation and use the same helper/semantics as the other functions to ensure consistency in `release`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/issue_claim.rs`:
- Around line 139-165: The gh_list_issues function currently uses a hard-coded
"--limit 100" which silently omits matching issues beyond 100; either document
this limitation in the function comment or remove the static cap and implement
pagination: update gh_list_issues to call the GitHub CLI repeatedly (or use the
GraphQL/REST API) to fetch all pages until exhausted, aggregating
serde_json::Value items before returning, and ensure errors from each
Command::new("gh") call are handled similarly to the existing out.status check.
- Around line 718-722: The release code updates `updated` and currently calls
`std::fs::write(state_path, json)?`; change this to the same atomic
temp-file-and-rename pattern used by `acquire`/`heartbeat`: create a temp path
(e.g. `state_path` + ".tmp" or using a unique suffix), write the pretty JSON
into that temp file, flush and sync if the other functions do so, then rename
the temp into `state_path` with `std::fs::rename`; keep existing error
propagation and use the same helper/semantics as the other functions to ensure
consistency in `release`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d634ab9b-9cde-4916-aadb-8b6fc648f14a
📒 Files selected for processing (1)
src/cli/issue_claim.rs
This was referenced Mar 16, 2026
This was referenced Mar 18, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated SDLC cycle for #29.
Summary by CodeRabbit