fix(kanban): restore completion audit dispatch - #61
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds completion audits for done tasks, including database scheduling fields, skip-review parsing, atomic claim/complete APIs, task-run events, dispatcher support, worker recovery, failure requeueing, and workspace-retention rules. ChangesCompletion audit workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant dispatch_once
participant kanban_db
participant AuditWorker
participant WorkspaceGC
dispatch_once->>kanban_db: Find due done task
dispatch_once->>kanban_db: Claim completion audit
dispatch_once->>AuditWorker: Spawn sdlc-completion-audit worker
AuditWorker->>kanban_db: Heartbeat or complete audit
kanban_db->>kanban_db: Requeue stale, timed-out, stuck, or crashed audit
WorkspaceGC->>kanban_db: Exclude tasks with queued completion audits
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 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 `@hermes_cli/kanban_db.py`:
- Around line 4298-4365: Update claim_completion_audit_task and its three
failure/rearm branches to centralize rearming through a helper that calls
_end_run() for the active audit run before clearing claim_lock/claim_expires and
restoring completion_audit_at. Ensure the task’s current_run_id and run status
are closed consistently so crashed audits can be requeued without leaving an
open task_runs row.
- Around line 9908-9944: Update the task-selection query in
gc_scratch_workspaces to require completion_audit_at IS NULL, preventing queued
completion audits from being deleted; apply the same guard in
gc_worktree_workspaces when its 24-hour cleanup query can remove audit-pending
tasks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Owner
|
auto-review: changes requested. Matrix checks (U1-U5, C1-C5)
Code-quality judgment (role-reviewer)
Findings are mechanical (matrix) or judgment-based (role-reviewer). If a finding looks wrong, leave a counter-comment on the kanban task and Sahil will adjudicate on human-review. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/hermes_cli/test_kanban_completion_audit.py (2)
340-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid hardcoded temporary paths.
To align with standard pytest practices and resolve the static analysis warning for insecure temporary files, consider using the
tmp_pathfixture instead of hardcoding/tmp.♻️ Proposed refactor
-def test_queued_completion_audit_worktree_survives_gc(kanban_home, monkeypatch): +def test_queued_completion_audit_worktree_survives_gc(kanban_home, monkeypatch, tmp_path): """Worktree GC also keeps a done workspace while its audit is queued.""" removed = [] monkeypatch.setattr(kb, "remove_worktree", lambda task_id, path: removed.append(task_id)) with kb.connect() as conn: task_id = kb.create_task( conn, title="audit before worktree cleanup", assignee="alice", workspace_kind="worktree", - workspace_path=str(Path("/tmp") / "audit-worktree"), + workspace_path=str(tmp_path / "audit-worktree"), )🤖 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 `@tests/hermes_cli/test_kanban_completion_audit.py` around lines 340 - 352, Update test_queued_completion_audit_worktree_survives_gc to accept pytest’s tmp_path fixture and derive workspace_path from it instead of constructing a hardcoded /tmp path; preserve the existing task setup and worktree cleanup assertions.Source: Linters/SAST tools
285-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove readability and avoid hardcoded temporary paths.
Consider using a nested function instead of the lambda generator
throw()trick for better readability. Additionally, using the standardtmp_pathpytest fixture instead of hardcoding/tmpavoids static analysis warnings for insecure temporary files.♻️ Proposed refactor
-@pytest.mark.parametrize("failure", ["workspace", "spawn"]) -def test_dispatch_completion_audit_failure_closes_run_and_requeues( - kanban_home, all_assignees_spawnable, monkeypatch, failure -): +@pytest.mark.parametrize("failure", ["workspace", "spawn"]) +def test_dispatch_completion_audit_failure_closes_run_and_requeues( + kanban_home, all_assignees_spawnable, monkeypatch, failure, tmp_path +): """A failed audit dispatch leaves no running run behind before requeueing.""" def failing_spawn(task, workspace, board=None): raise RuntimeError("audit worker unavailable") with kb.connect() as conn: task_id = kb.create_task(conn, title="retry audit", assignee="alice") _complete_task_no_pr(conn, task_id) if failure == "workspace": - monkeypatch.setattr( - kb, - "resolve_workspace", - lambda task, board=None: (_ for _ in ()).throw( - RuntimeError("workspace unavailable") - ), - ) + def failing_resolve(task, board=None): + raise RuntimeError("workspace unavailable") + + monkeypatch.setattr(kb, "resolve_workspace", failing_resolve) else: - monkeypatch.setattr(kb, "resolve_workspace", lambda task, board=None: Path("/tmp")) + monkeypatch.setattr(kb, "resolve_workspace", lambda task, board=None: tmp_path)🤖 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 `@tests/hermes_cli/test_kanban_completion_audit.py` around lines 285 - 306, Improve test_dispatch_completion_audit_failure_closes_run_and_requeues by replacing the generator-based lambda used for the workspace failure with a nested function that raises RuntimeError directly, and add pytest’s tmp_path fixture to use for the successful resolve_workspace mock instead of the hardcoded /tmp path.Source: Linters/SAST tools
🤖 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 `@tests/hermes_cli/test_kanban_completion_audit.py`:
- Around line 340-352: Update test_queued_completion_audit_worktree_survives_gc
to accept pytest’s tmp_path fixture and derive workspace_path from it instead of
constructing a hardcoded /tmp path; preserve the existing task setup and
worktree cleanup assertions.
- Around line 285-306: Improve
test_dispatch_completion_audit_failure_closes_run_and_requeues by replacing the
generator-based lambda used for the workspace failure with a nested function
that raises RuntimeError directly, and add pytest’s tmp_path fixture to use for
the successful resolve_workspace mock instead of the hardcoded /tmp path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 401afaa0-92ea-429c-a3f3-5b71f1c5373e
📒 Files selected for processing (2)
hermes_cli/kanban_db.pytests/hermes_cli/test_kanban_completion_audit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- hermes_cli/kanban_db.py
Collaborator
Author
Completion-audit follow-upAddresses the auto-review findings from the prior head:
Validation:
CodeRabbit’s two substantive lifecycle/GC comments are addressed in |
Owner
|
auto-review: changes requested. Matrix checks (U1–U5, C1–C5)
Code-quality judgment (role-reviewer)
Findings are mechanical (matrix) or judgment-based (role-reviewer). If a finding looks wrong, leave a counter-comment on the kanban task and Sahil will adjudicate on human-review. |
Owner
|
auto-review: changes requested. Matrix checks (U1–U5, C1–C5)
Code-quality judgment (role-reviewer)
Focused verification: Findings are mechanical (matrix) or judgment-based (role-reviewer). If a finding looks wrong, leave a counter-comment on the kanban task and Sahil will adjudicate on human-review. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/hermes_cli/test_kanban_completion_audit.py (1)
355-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that each recovery actually requeues the audit.
These tests pass if claims/runs are cleared but
completion_audit_atis accidentally leftNULL.
tests/hermes_cli/test_kanban_completion_audit.py#L355-L381: assertcompletion_audit_at IS NOT NULLafter timeout recovery.tests/hermes_cli/test_kanban_completion_audit.py#L384-L419: assert it after stuck-worker recovery.tests/hermes_cli/test_kanban_completion_audit.py#L422-L444: assert it after crash recovery.As per coding guidelines, “Write behavioral and invariant-based tests.”
🤖 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 `@tests/hermes_cli/test_kanban_completion_audit.py` around lines 355 - 381, Strengthen the recovery tests by asserting that each recovered task has a non-null completion_audit_at after enforcement: add this assertion in tests/hermes_cli/test_kanban_completion_audit.py lines 355-381 for timeout recovery, lines 384-419 for stuck-worker recovery, and lines 422-444 for crash recovery. Use the task returned by get_task and preserve the existing status, run, and claim-lock assertions.Source: Coding guidelines
🤖 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 `@hermes_cli/kanban_db.py`:
- Around line 8139-8157: Prevent audit requeue while the prior worker is still
alive: in hermes_cli/kanban_db.py lines 8139-8157, 7977-7999, and 8347-8359, use
the existing termination helper, verify liveness after signaling/SIGKILL, and
defer recovery instead of releasing or rearming the claim when the worker
survives.
- Around line 9056-9068: Update the completion-audit handling around
_recover_stale_completion_audit_claim so rate_limited_exit is handled
separately: preserve the rate_limited outcome and backoff, avoid adding the
audit to crashed, and allow the existing quota-blocker handling to run. Keep the
current crashed recovery behavior for non-rate-limited failures.
- Around line 8970-8973: The crash-launch grace query in hermes_cli/kanban_db.py
lines 8970-8973 must join the active current_run_id to task_runs and select
COALESCE(run.started_at, task.started_at) for the timestamp used by the
running-task check. Update tests/hermes_cli/test_kanban_completion_audit.py
lines 429-432 to age the active audit task run rather than the completed parent
task.
---
Nitpick comments:
In `@tests/hermes_cli/test_kanban_completion_audit.py`:
- Around line 355-381: Strengthen the recovery tests by asserting that each
recovered task has a non-null completion_audit_at after enforcement: add this
assertion in tests/hermes_cli/test_kanban_completion_audit.py lines 355-381 for
timeout recovery, lines 384-419 for stuck-worker recovery, and lines 422-444 for
crash recovery. Use the task returned by get_task and preserve the existing
status, run, and claim-lock assertions.
🪄 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: 1ffef087-4bd8-4df1-8334-2d8cddf7b4d4
📒 Files selected for processing (2)
hermes_cli/kanban_db.pytests/hermes_cli/test_kanban_completion_audit.py
Collaborator
Author
|
auto-review: changes requested. Matrix checks (U1–U5, C1–C5)
Code-quality judgment (role-reviewer)
Findings are mechanical (matrix) or judgment-based (role-reviewer). If a finding looks wrong, leave a counter-comment on the kanban task and Sahil will adjudicate on human-review. |
Collaborator
Author
|
Addressed the remaining CodeRabbit test-coverage nit in 6ce3c56: timeout, stuck-worker, and crash recovery tests now each assert completion_audit_at is restored after requeue. Focused 378-test Kanban matrix passed before the amendment. |
Owner
|
auto-review: approved, awaiting human merge + kanban_approve. Matrix checks (U1–U5, C1–C5)
Code-quality judgment (role-reviewer)Violations: None. Reviewed the full PR diff and the active-audit lifecycle paths: claims retain Independent verification: Findings are mechanical (matrix) or judgment-based (role-reviewer). Sahil retains the substantive merge decision. |
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 17, 2026
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 21, 2026
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 23, 2026
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 28, 2026
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Aug 24, 2026
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Sep 2, 2026
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
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.
Diagnosis
The fork rebase replayed the completion-audit commit as
e0042ea6de, but omitted its corekanban_db.pyhunks. The canonical earlier commit5f65c7af92contains the full implementation and is not an ancestor of the rebased fork main.Fix
completion_audit_atmigration and sparse index.skip-reviewdetection plus atomic audit claim and completion lifecycle helpers.Verification
scripts/run_tests.sh tests/hermes_cli/test_kanban_completion_audit.py tests/hermes_cli/test_kanban_dispatcher_crash_breaker.py tests/hermes_cli/test_kanban_human_review.py tests/hermes_cli/test_kanban_merging.py -q— 85 passed.scripts/run_tests.sh tests/hermes_cli/test_kanban_*.py -q— 882 passed; one pre-existing failure intest_default_spawn_injects_hermes_max_iterations, reproduced unchanged onmyfork/main(11 passed, 1 failed).uv run ruff check hermes_cli/kanban_db.py— passed.uv run ty check hermes_cli/kanban_db.pyreports 9 existing diagnostics outside this change.Closes the fork-main completion-audit CI regression.
Summary by CodeRabbit
New Features
skip-review:directive.Bug Fixes
Tests