Conversation
📝 WalkthroughWalkthroughThe goal loop now emits finalization recovery outcomes, the CLI persists recovery contracts and resets eligible tasks, and terminal Kanban tools reconcile repeated or racing lifecycle calls as no-op successes. Tests and documentation cover recovery, handoffs, terminal races, and updated goal-mode behavior. ChangesGoal finalization recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Worker
participant GoalLoop
participant Judge
participant CLI
participant KanbanDB
Worker->>GoalLoop: execute goal-mode turns
GoalLoop->>Judge: evaluate task output
Judge-->>GoalLoop: done verdict without finalizer
GoalLoop-->>CLI: finalization_recovery contract
CLI->>KanbanDB: record recovery with expected run id
KanbanDB-->>CLI: reset task, close run, record event
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 `@cli.py`:
- Around line 15600-15612: Redact the worker/LLM-influenced recovery_contract
before passing it to _kb.record_finalization_recovery, using the same
redact_sensitive_text(..., force=True) pattern as _handle_complete and
_handle_block in tools/kanban_tools.py. Preserve the existing recovery_reason
handling and pass the redacted contract to both persisted task comments and
event payloads through record_finalization_recovery.
In `@tools/kanban_tools.py`:
- Around line 298-336: Remove the non-terminal "todo" and "triage" values from
_TERMINAL_RECONCILIATION_STATUSES, leaving only statuses that represent a
successful terminal outcome for the guarded lifecycle calls. Keep
_terminal_transition_noop and its four call sites unchanged so reopened or
rework states return the existing error path instead of a false noop success.
🪄 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: 6a82943d-6543-4634-bc53-7c613996613a
📒 Files selected for processing (7)
cli.pyhermes_cli/goals.pyhermes_cli/kanban_db.pytests/hermes_cli/test_kanban_goal_mode.pytests/tools/test_kanban_tools.pytools/kanban_tools.pywebsite/docs/user-guide/features/kanban.md
| recovery_contract = str(loop_result.get("recovery_contract") or "").strip() | ||
| recovery_reason = str(loop_result.get("reason") or "").strip() | ||
| run_id_raw = (_os.environ.get("HERMES_KANBAN_RUN_ID") or "").strip() | ||
| expected_run_id = int(run_id_raw) if run_id_raw.isdigit() else None | ||
| c = _kb.connect() | ||
| try: | ||
| saved = _kb.record_finalization_recovery( | ||
| c, | ||
| task_id, | ||
| reason=recovery_reason, | ||
| contract=recovery_contract, | ||
| expected_run_id=expected_run_id, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Recovery contract text isn't redacted before being persisted.
recovery_contract embeds the goal judge's freeform reason (via KANBAN_GOAL_FINALIZATION_RECOVERY_TEMPLATE, which can echo content derived from the worker's response) and flows straight into record_finalization_recovery, which writes it verbatim into task_comments and the finalization_recovery_requested event payload. Every sibling lifecycle path (_handle_complete, _handle_block in tools/kanban_tools.py) redacts comparable user/LLM-influenced text with redact_sensitive_text(..., force=True) before persistence; this path skips that step.
🛡️ Proposed fix
recovery_contract = str(loop_result.get("recovery_contract") or "").strip()
recovery_reason = str(loop_result.get("reason") or "").strip()
+ if recovery_contract:
+ from agent.redact import redact_sensitive_text
+ recovery_contract = redact_sensitive_text(recovery_contract, force=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| recovery_contract = str(loop_result.get("recovery_contract") or "").strip() | |
| recovery_reason = str(loop_result.get("reason") or "").strip() | |
| run_id_raw = (_os.environ.get("HERMES_KANBAN_RUN_ID") or "").strip() | |
| expected_run_id = int(run_id_raw) if run_id_raw.isdigit() else None | |
| c = _kb.connect() | |
| try: | |
| saved = _kb.record_finalization_recovery( | |
| c, | |
| task_id, | |
| reason=recovery_reason, | |
| contract=recovery_contract, | |
| expected_run_id=expected_run_id, | |
| ) | |
| recovery_contract = str(loop_result.get("recovery_contract") or "").strip() | |
| recovery_reason = str(loop_result.get("reason") or "").strip() | |
| if recovery_contract: | |
| from agent.redact import redact_sensitive_text | |
| recovery_contract = redact_sensitive_text(recovery_contract, force=True) | |
| run_id_raw = (_os.environ.get("HERMES_KANBAN_RUN_ID") or "").strip() | |
| expected_run_id = int(run_id_raw) if run_id_raw.isdigit() else None | |
| c = _kb.connect() | |
| try: | |
| saved = _kb.record_finalization_recovery( | |
| c, | |
| task_id, | |
| reason=recovery_reason, | |
| contract=recovery_contract, | |
| expected_run_id=expected_run_id, | |
| ) |
🤖 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 `@cli.py` around lines 15600 - 15612, Redact the worker/LLM-influenced
recovery_contract before passing it to _kb.record_finalization_recovery, using
the same redact_sensitive_text(..., force=True) pattern as _handle_complete and
_handle_block in tools/kanban_tools.py. Preserve the existing recovery_reason
handling and pass the redacted contract to both persisted task comments and
event payloads through record_finalization_recovery.
| _TERMINAL_RECONCILIATION_STATUSES = ( | ||
| "done", | ||
| "blocked", | ||
| "todo", | ||
| "triage", | ||
| "review", | ||
| "human_review", | ||
| ) | ||
|
|
||
|
|
||
| def _terminal_transition_noop( | ||
| kb, | ||
| conn, | ||
| task_id: str, | ||
| *, | ||
| terminal_statuses: tuple[str, ...], | ||
| action: str, | ||
| extra: Optional[dict] = None, | ||
| ) -> Optional[str]: | ||
| """Reconcile a lost CAS race when the desired terminal state already won.""" | ||
| task = kb.get_task(conn, task_id) | ||
| if task is None or task.status not in terminal_statuses: | ||
| return None | ||
| run = kb.latest_run(conn, task_id) | ||
| payload = { | ||
| "task_id": task_id, | ||
| "run_id": run.id if run else None, | ||
| "status": task.status, | ||
| "noop": True, | ||
| } | ||
| if isinstance(extra, dict): | ||
| payload.update(extra) | ||
| logger.info( | ||
| "%s: reconciled as no-op for task %s already in %s", | ||
| action, | ||
| task_id, | ||
| task.status, | ||
| ) | ||
| return _ok(**payload) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
_TERMINAL_RECONCILIATION_STATUSES includes non-terminal statuses, causing false-positive noop successes.
The docstring says this reconciles "a lost CAS race when the desired terminal state already won," but "todo" and "triage" are not a "won" outcome for any of the four guarded calls (kanban_complete, kanban_block, kanban_review, kanban_human_review) — they represent the task being sent back for rework. If a task is externally reset to todo/triage (e.g. reopened) while a stale finalize call races in, this helper will report a false success (ok=True, noop=True, status="todo") instead of an error, and the worker/orchestrator will believe its lifecycle call succeeded while the task actually needs redoing — silently dropped work with no error signal. This affects all four call sites at Lines 724-732, 801-810, 1523-1531, and 1570-1578.
🐛 Proposed fix — drop non-terminal statuses from the shared tuple
_TERMINAL_RECONCILIATION_STATUSES = (
"done",
"blocked",
- "todo",
- "triage",
"review",
"human_review",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _TERMINAL_RECONCILIATION_STATUSES = ( | |
| "done", | |
| "blocked", | |
| "todo", | |
| "triage", | |
| "review", | |
| "human_review", | |
| ) | |
| def _terminal_transition_noop( | |
| kb, | |
| conn, | |
| task_id: str, | |
| *, | |
| terminal_statuses: tuple[str, ...], | |
| action: str, | |
| extra: Optional[dict] = None, | |
| ) -> Optional[str]: | |
| """Reconcile a lost CAS race when the desired terminal state already won.""" | |
| task = kb.get_task(conn, task_id) | |
| if task is None or task.status not in terminal_statuses: | |
| return None | |
| run = kb.latest_run(conn, task_id) | |
| payload = { | |
| "task_id": task_id, | |
| "run_id": run.id if run else None, | |
| "status": task.status, | |
| "noop": True, | |
| } | |
| if isinstance(extra, dict): | |
| payload.update(extra) | |
| logger.info( | |
| "%s: reconciled as no-op for task %s already in %s", | |
| action, | |
| task_id, | |
| task.status, | |
| ) | |
| return _ok(**payload) | |
| _TERMINAL_RECONCILIATION_STATUSES = ( | |
| "done", | |
| "blocked", | |
| "review", | |
| "human_review", | |
| ) |
🤖 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 `@tools/kanban_tools.py` around lines 298 - 336, Remove the non-terminal "todo"
and "triage" values from _TERMINAL_RECONCILIATION_STATUSES, leaving only
statuses that represent a successful terminal outcome for the guarded lifecycle
calls. Keep _terminal_transition_noop and its four call sites unchanged so
reopened or rework states return the existing error path instead of a false noop
success.
* fix(kanban): recover missed goal finalizers * test(kanban): cover duplicate blocker recovery --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
* fix(kanban): recover missed goal finalizers * test(kanban): cover duplicate blocker recovery --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
* fix(kanban): recover missed goal finalizers * test(kanban): cover duplicate blocker recovery --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
* fix(kanban): recover missed goal finalizers * test(kanban): cover duplicate blocker recovery --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
2 participants
Diagnosis
Previously, a goal-mode worker that produced completion prose but missed the lifecycle tool was nudged once and then force-blocked by the outer goal loop. That could race a valid
kanban_complete/kanban_review, mark clean work as blocked, and permit scratch cleanup before recovery.Change
doneprose followed by one missed finalizer nudge returns a boundedfinalization_recoverycontract; the outer loop never callsblock_fnfor that case.finalization_recovery, records the recovery contract in the event/comment history, and returns the task toreadyfor deterministic re-dispatch.Tests
scripts/run_tests.sh tests/hermes_cli/test_kanban_goal_mode.py tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_completion_audit.py tests/hermes_cli/test_kanban_core_functionality.pyruff checkon changed Python files passed.Compatibility / migration
No schema migration: recovery uses the existing
readytask state, task-run history, comments, and event log. Existing realdependencyandneeds_inputblock behavior is unchanged.Environment-only check limitations
npm run checkis currently blocked by missing workspace dependencies/types (for example@tauri-apps/api/core,@assistant-ui/react,@nous-research/ui, andplist). Full-filety checkreports 208 existing diagnostics in unrelated legacy areas; no new diagnostics were reported in the modified recovery code.Summary by CodeRabbit
New Features
Documentation