feat(kanban): add human_review status + review/approve/reject tools and CLI - #1
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 40 minutes and 32 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 52cc0a4329d029c8c60a738b8f6ae581acdd8fb0 and e88ec07. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis pull request introduces a complete human review workflow for Kanban tasks. It adds database state transitions to move tasks through review stages, CLI commands to trigger those transitions with audit trails, notification rendering for new review events with emoji indicators, agent-accessible tools for review actions, and comprehensive test coverage across all layers. ChangesHuman Review Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 5
🧹 Nitpick comments (5)
tests/hermes_cli/test_kanban_human_review.py (2)
82-93: ⚖️ Poor tradeoffConsider testing downstream effects of
approve_task.Per the implementation (snippet cg_010, lines 3315-3317),
approve_tasktriggers three side effects beyond the status transition:
- Clears the failure counter via
_clear_failure_counter- Recomputes readiness for dependent tasks via
recompute_ready- Cleans up the workspace via
_cleanup_workspaceThe current test verifies the status transition,
completed_atfield, and event emission, but doesn't exercise these downstream behaviors. Consider adding a test with dependent tasks and/or a previous failure to validate the full approval flow.🤖 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_human_review.py` around lines 82 - 93, The test only asserts status, completed_at and events for approve_task but misses side effects; update test_approve_task_human_review_to_done to also (1) seed a prior failure count (call whatever helper increments failures or set failure counter for tid) and assert that _clear_failure_counter(tid) effect occurred (failure count is zero) after kb.approve_task, (2) create a dependent task (use kb.create_task with depends_on or equivalent) and assert recompute_ready caused the dependent task to become ready/assigned as expected after approve_task, and (3) verify workspace cleanup by asserting _cleanup_workspace side-effect (e.g., workspace files removed or workspace flag cleared) for tid; reference approve_task, _clear_failure_counter, recompute_ready, _cleanup_workspace, kb.create_task, kb.move_to_human_review, and kb.list_events to locate where to add these assertions.
37-54: ⚡ Quick winConsider adding test coverage for
move_to_reviewfromreadystatus.Per the implementation (snippet cg_001, line 3167),
move_to_reviewaccepts tasks in'running'or'ready'status. The current tests only exercise therunning -> reviewtransition. Adding a test case forready -> reviewwould provide more complete coverage of the documented state machine.📋 Suggested test to add
def test_move_to_review_ready_to_review(kanban_home): """move_to_review should work from ready status (unclaimed task).""" with kb.connect() as conn: tid = kb.create_task(conn, title="x", assignee="worker") # Task is in 'ready' status after creation (no parents, not triage) ok = kb.move_to_review(conn, tid, reason="PR opened before claim") assert ok t = kb.get_task(conn, tid) assert t.status == "review" events = [e.kind for e in kb.list_events(conn, tid)] assert "review_requested" in events🤖 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_human_review.py` around lines 37 - 54, Add a new test covering the ready->review transition: create an unclaimed task with kb.create_task (so it remains in 'ready'), call kb.move_to_review(tid, reason=...) and assert it returns True, then verify the task status via kb.get_task is "review" and that "review_requested" appears in kb.list_events; place the new test alongside test_move_to_review_running_to_review using names like test_move_to_review_ready_to_review to mirror existing style and exercise kb.move_to_review, kb.get_task and kb.list_events.gateway/run.py (2)
4870-4879: ⚡ Quick winFormatting inconsistency: rejected uses newline while approved uses inline colon.
The
rejectedmessage formats its reason with a newline prefix (\n{note}), whileapproved(line 4863-4865) andhuman_review_requested(line 4853-4855) use an inline colon prefix (: {note}). If this asymmetry is intentional (e.g., rejection rationale tends to be longer or multi-paragraph), that's fine—but it's worth confirming the design intent.🔄 Option: unify formatting if not intentional
If the difference wasn't intentional, you could align all three to use the same inline format:
elif kind == "rejected": note = "" if ev.payload and ev.payload.get("reason"): note = ( - f"\n{str(ev.payload['reason'])[:NOTIFY_BLOCKED_REASON_MAX]}" + f": {str(ev.payload['reason'])[:NOTIFY_BLOCKED_REASON_MAX]}" ) msg = ( f"↩ {tag}Kanban {sub['task_id']} rejected " f"— back to ready{note}" )🤖 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 `@gateway/run.py` around lines 4870 - 4879, The rejected branch in the kind handling constructs note with a leading newline (f"\n{...}") causing formatting inconsistency with the approved and human_review_requested branches that use an inline colon; update the rejected branch (the block under "elif kind == \"rejected\"") to build note the same way (use ": {str(ev.payload['reason'])[:NOTIFY_BLOCKED_REASON_MAX]}" or prepend ": " when note exists) so msg uses an inline colon like the others, preserving the truncation via NOTIFY_BLOCKED_REASON_MAX and the existing ev.payload check.
4850-4869: ⚡ Quick winConsider handling multiline reasons in inline-formatted messages.
The
human_review_requestedandapprovedbranches format their reason inline (: {note}), but unlike thecompletedhandler (lines 4809-4811), they don't apply.splitlines()[0]before truncation. If a reason payload contains newlines, the message could span multiple lines unexpectedly.This mirrors the existing
blockedbehavior (line 4826), so it may be intentional to preserve multiline formatting—but for inline messages, taking only the first line (as done fordonesummaries) often yields cleaner chat notifications.♻️ Optional: extract first line for inline formats
If single-line inline display is preferred:
elif kind == "human_review_requested": note = "" if ev.payload and ev.payload.get("reason"): + reason_text = str(ev.payload['reason']).strip() + first_line = reason_text.splitlines()[0] if reason_text else "" note = ( - f": {str(ev.payload['reason'])[:NOTIFY_BLOCKED_REASON_MAX]}" + f": {first_line[:NOTIFY_BLOCKED_REASON_MAX]}" ) msg = ( f"⏳ {tag}Kanban {sub['task_id']} ready for " f"your review — {title}{note}" ) elif kind == "approved": note = "" if ev.payload and ev.payload.get("reason"): + reason_text = str(ev.payload['reason']).strip() + first_line = reason_text.splitlines()[0] if reason_text else "" note = ( - f": {str(ev.payload['reason'])[:NOTIFY_BLOCKED_REASON_MAX]}" + f": {first_line[:NOTIFY_BLOCKED_REASON_MAX]}" ) msg = ( f"✅ {tag}Kanban {sub['task_id']} approved " f"— {title}{note}" )🤖 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 `@gateway/run.py` around lines 4850 - 4869, The inline messages in the "human_review_requested" and "approved" branches build note from ev.payload['reason'] without extracting the first line, which can produce multi-line chat notifications; change the construction to mirror the completed/done handling by taking only the first line (e.g., use str(ev.payload['reason']).splitlines()[0] before applying the NOTIFY_BLOCKED_REASON_MAX slice) when creating note so the inline formatting in the message strings for those branches remains single-line; update the note logic in the branches handling kind == "human_review_requested" and kind == "approved" accordingly.tests/gateway/test_kanban_notifier_human_review.py (1)
102-106: ⚡ Quick winTighten event-sequence assertions in approve/reject notifier tests.
Line 102 says two messages are expected, but the tests currently don’t assert count or explicitly require the preceding
human_review_requestednotification. Add strict assertions so sequence regressions are caught.Proposed test assertion hardening
def test_notifier_renders_approved(tmp_path, monkeypatch): @@ - # Two messages expected: human_review_requested then approved. - kinds_seen = " | ".join(m["text"] for m in adapter.sent) + # Two messages expected: human_review_requested then approved. + assert len(adapter.sent) == 2 + kinds_seen = " | ".join(m["text"] for m in adapter.sent) + assert "⏳" in kinds_seen assert "✅" in kinds_seen assert "approved" in kinds_seen.lower() assert tid in kinds_seen @@ def test_notifier_renders_rejected(tmp_path, monkeypatch): @@ - text_all = " | ".join(m["text"] for m in adapter.sent) + assert len(adapter.sent) == 2 + text_all = " | ".join(m["text"] for m in adapter.sent) + assert "⏳" in text_all assert "↩" in text_all assert "rejected" in text_all.lower() assert "needs tests" in text_allAlso applies to: 125-128
🤖 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/gateway/test_kanban_notifier_human_review.py` around lines 102 - 106, The tests currently only check message contents loosely; tighten them by asserting the exact sequence and count: assert len(adapter.sent) == 2, then check adapter.sent[0]["text"] (or ["type"] if available) explicitly contains the human review request marker (e.g., "human_review_requested" or "human review requested") and adapter.sent[1]["text"] contains the approval marker (for approve test check "✅" and "approved" plus tid; for reject test check the reject marker like "❌" and "rejected" plus tid). Update both places that build/assert kinds_seen (referencing adapter.sent and tid) to use these ordered, index-based assertions so sequence regressions are caught.
🤖 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`:
- Line 97: VALID_STATUSES now contains "human_review" but the dashboard column
contract in the Kanban plugin API does not, so tasks can be mis-bucketed; update
the board-columns definition in the plugin API (the board columns list /
get_board_columns implementation) to include "human_review" and ensure any
mapping/validation logic that converts statuses to columns uses that new value
(reference VALID_STATUSES and the board-columns list/get_board_columns symbol in
the plugin_api to locate and update the code).
- Around line 3309-3313: approve_task currently calls _synthesize_ended_run with
outcome="approved", but the parent handoff rendering logic only considers runs
with outcome='completed', so approved tasks are excluded from parent summary;
fix by either changing _synthesize_ended_run call in approve_task to use
outcome="completed" and add an approval marker in the run metadata/event (so
downstream can detect approvals), or update the parent handoff/parent-summary
reader to treat outcome=='approved' as equivalent to 'completed' when assembling
parent results; modify the approve_task/_synthesize_ended_run call-site or the
parent-summary reader accordingly and ensure you add/recognize a unique metadata
key (e.g., approval_flag or event type) so approvals remain distinguishable.
In `@hermes_cli/kanban.py`:
- Around line 2007-2015: The audit comment is being added before the CAS
transition, causing persistent incorrect notes when the state change fails;
change the sequence so the comment is written only after a successful transition
(i.e., after checking the return value `ok`) or combine into a single DB
helper/transaction. Specifically, in the handlers using `kb.connect()` and
calling `kb.add_comment(conn, tid, author, ...)` followed by `ok =
kb.move_to_review(...)` (and the analogous pairs `kb.move_to_human_review`,
`kb.move_to_approved`, `kb.move_to_rejected`), move the `add_comment` call to
occur only if `ok` is truthy (or refactor `move_to_*` to atomically record the
audit note on success), ensuring `tid`, `author`, `_worker_run_id_for(tid)`, and
`conn` are passed unchanged.
In `@tools/kanban_tools.py`:
- Around line 868-870: The current flow calls kb.add_comment(...) before
kb.reject_task(...), which can leave a spurious "REJECTED:" comment if
kb.reject_task returns False; change the logic in the function containing these
lines so you call kb.reject_task(conn, str(tid), reason=reason) first, check the
returned ok boolean, and only when ok is True call kb.add_comment(conn,
str(tid), author, f"REJECTED: {reason}"); alternatively, update the DB helper to
expose an atomic operation (e.g., kb.reject_task_and_comment or a transactional
wrapper) that performs the state check, state update, and comment insert in one
transaction to avoid inconsistent comments.
- Around line 829-845: The approve/reject handlers (_handle_approve and the
corresponding _handle_reject) are missing an ownership check allowing any worker
knowing a task_id to mutate state; after obtaining kb and conn from _connect
(and after validating task_id) call _enforce_worker_task_ownership(conn,
str(tid)) and handle its failure by returning a tool_error, then proceed to call
kb.approve_task / kb.reject_task; ensure this check runs before the mutating
calls (kb.approve_task, kb.reject_task) so only the owning worker can
approve/reject tasks — apply the same change to the other handler variants
noted.
---
Nitpick comments:
In `@gateway/run.py`:
- Around line 4870-4879: The rejected branch in the kind handling constructs
note with a leading newline (f"\n{...}") causing formatting inconsistency with
the approved and human_review_requested branches that use an inline colon;
update the rejected branch (the block under "elif kind == \"rejected\"") to
build note the same way (use ":
{str(ev.payload['reason'])[:NOTIFY_BLOCKED_REASON_MAX]}" or prepend ": " when
note exists) so msg uses an inline colon like the others, preserving the
truncation via NOTIFY_BLOCKED_REASON_MAX and the existing ev.payload check.
- Around line 4850-4869: The inline messages in the "human_review_requested" and
"approved" branches build note from ev.payload['reason'] without extracting the
first line, which can produce multi-line chat notifications; change the
construction to mirror the completed/done handling by taking only the first line
(e.g., use str(ev.payload['reason']).splitlines()[0] before applying the
NOTIFY_BLOCKED_REASON_MAX slice) when creating note so the inline formatting in
the message strings for those branches remains single-line; update the note
logic in the branches handling kind == "human_review_requested" and kind ==
"approved" accordingly.
In `@tests/gateway/test_kanban_notifier_human_review.py`:
- Around line 102-106: The tests currently only check message contents loosely;
tighten them by asserting the exact sequence and count: assert len(adapter.sent)
== 2, then check adapter.sent[0]["text"] (or ["type"] if available) explicitly
contains the human review request marker (e.g., "human_review_requested" or
"human review requested") and adapter.sent[1]["text"] contains the approval
marker (for approve test check "✅" and "approved" plus tid; for reject test
check the reject marker like "❌" and "rejected" plus tid). Update both places
that build/assert kinds_seen (referencing adapter.sent and tid) to use these
ordered, index-based assertions so sequence regressions are caught.
In `@tests/hermes_cli/test_kanban_human_review.py`:
- Around line 82-93: The test only asserts status, completed_at and events for
approve_task but misses side effects; update
test_approve_task_human_review_to_done to also (1) seed a prior failure count
(call whatever helper increments failures or set failure counter for tid) and
assert that _clear_failure_counter(tid) effect occurred (failure count is zero)
after kb.approve_task, (2) create a dependent task (use kb.create_task with
depends_on or equivalent) and assert recompute_ready caused the dependent task
to become ready/assigned as expected after approve_task, and (3) verify
workspace cleanup by asserting _cleanup_workspace side-effect (e.g., workspace
files removed or workspace flag cleared) for tid; reference approve_task,
_clear_failure_counter, recompute_ready, _cleanup_workspace, kb.create_task,
kb.move_to_human_review, and kb.list_events to locate where to add these
assertions.
- Around line 37-54: Add a new test covering the ready->review transition:
create an unclaimed task with kb.create_task (so it remains in 'ready'), call
kb.move_to_review(tid, reason=...) and assert it returns True, then verify the
task status via kb.get_task is "review" and that "review_requested" appears in
kb.list_events; place the new test alongside
test_move_to_review_running_to_review using names like
test_move_to_review_ready_to_review to mirror existing style and exercise
kb.move_to_review, kb.get_task and kb.list_events.
🪄 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: 86a0ae3b-3b7a-4f8b-8862-a98c599916d3
📥 Commits
Reviewing files that changed from the base of the PR and between f3fb789 and 52cc0a4329d029c8c60a738b8f6ae581acdd8fb0.
📒 Files selected for processing (7)
gateway/run.pyhermes_cli/kanban.pyhermes_cli/kanban_db.pytests/gateway/test_kanban_notifier.pytests/gateway/test_kanban_notifier_human_review.pytests/hermes_cli/test_kanban_human_review.pytools/kanban_tools.py
Comment on lines
+3309
to
+3313
| run_id = _synthesize_ended_run( | ||
| conn, task_id, | ||
| outcome="approved", | ||
| summary=reason, | ||
| ) |
There was a problem hiding this comment.
approved runs are currently excluded from parent handoff context
At Line 3309-3313, approve_task writes a synthetic run with outcome="approved", but parent handoff rendering only reads outcome='completed' runs (Line 5858). Approved tasks with no tasks.result will show as having no parent result to downstream workers.
Consider either:
- writing approval as
outcome="completed"with a marker in metadata/event, or - updating parent-summary readers to include
approved.
One localized fix option
- run_id = _synthesize_ended_run(
- conn, task_id,
- outcome="approved",
- summary=reason,
- )
+ run_id = _synthesize_ended_run(
+ conn, task_id,
+ outcome="completed",
+ summary=reason,
+ metadata={"terminal_kind": "approved"},
+ )📝 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.
Suggested change
| run_id = _synthesize_ended_run( | |
| conn, task_id, | |
| outcome="approved", | |
| summary=reason, | |
| ) | |
| run_id = _synthesize_ended_run( | |
| conn, task_id, | |
| outcome="completed", | |
| summary=reason, | |
| metadata={"terminal_kind": "approved"}, | |
| ) |
🤖 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 `@hermes_cli/kanban_db.py` around lines 3309 - 3313, approve_task currently
calls _synthesize_ended_run with outcome="approved", but the parent handoff
rendering logic only considers runs with outcome='completed', so approved tasks
are excluded from parent summary; fix by either changing _synthesize_ended_run
call in approve_task to use outcome="completed" and add an approval marker in
the run metadata/event (so downstream can detect approvals), or update the
parent handoff/parent-summary reader to treat outcome=='approved' as equivalent
to 'completed' when assembling parent results; modify the
approve_task/_synthesize_ended_run call-site or the parent-summary reader
accordingly and ensure you add/recognize a unique metadata key (e.g.,
approval_flag or event type) so approvals remain distinguishable.
Comment on lines
+2007
to
+2015
| with kb.connect() as conn: | ||
| if reason: | ||
| kb.add_comment(conn, tid, author, f"REVIEW: {reason}") | ||
| ok = kb.move_to_review( | ||
| conn, | ||
| tid, | ||
| reason=reason, | ||
| expected_run_id=_worker_run_id_for(tid), | ||
| ) |
There was a problem hiding this comment.
Only append the audit comment after the transition succeeds.
add_comment() runs before the CAS transition in all four handlers, so a wrong-state attempt still leaves a durable REVIEW:, HUMAN-REVIEW:, APPROVED:, or REJECTED: note even when the status change fails. That makes the task thread lie about what actually happened and can mislead the next worker. Move the comment write behind the ok check, or fold both operations into one DB helper/transaction.
Also applies to: 2030-2038, 2056-2059, 2080-2082
🤖 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 `@hermes_cli/kanban.py` around lines 2007 - 2015, The audit comment is being
added before the CAS transition, causing persistent incorrect notes when the
state change fails; change the sequence so the comment is written only after a
successful transition (i.e., after checking the return value `ok`) or combine
into a single DB helper/transaction. Specifically, in the handlers using
`kb.connect()` and calling `kb.add_comment(conn, tid, author, ...)` followed by
`ok = kb.move_to_review(...)` (and the analogous pairs
`kb.move_to_human_review`, `kb.move_to_approved`, `kb.move_to_rejected`), move
the `add_comment` call to occur only if `ok` is truthy (or refactor `move_to_*`
to atomically record the audit note on success), ensuring `tid`, `author`,
`_worker_run_id_for(tid)`, and `conn` are passed unchanged.
Comment on lines
+829
to
+845
| def _handle_approve(args: dict, **kw) -> str: | ||
| """Approve a human_review task (-> done).""" | ||
| tid = args.get("task_id") | ||
| if not tid: | ||
| return tool_error("task_id is required") | ||
| reason = args.get("reason") | ||
| board = args.get("board") | ||
| try: | ||
| kb, conn = _connect(board=board) | ||
| try: | ||
| ok = kb.approve_task(conn, str(tid), reason=reason) | ||
| if not ok: | ||
| return tool_error( | ||
| f"could not approve {tid} (not in human_review or unknown)" | ||
| ) | ||
| return _ok(task_id=str(tid), status="done") | ||
| finally: |
There was a problem hiding this comment.
Enforce worker ownership on kanban_approve and kanban_reject.
These tools are exposed to dispatcher-spawned workers via _check_kanban_mode, but unlike kanban_review / kanban_human_review the handlers never call _enforce_worker_task_ownership(). A prompt-injected worker can therefore approve or reject any board task whose id it knows. Add the same ownership guard here before mutating state.
Also applies to: 854-875, 1564-1580
🤖 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 829 - 845, The approve/reject handlers
(_handle_approve and the corresponding _handle_reject) are missing an ownership
check allowing any worker knowing a task_id to mutate state; after obtaining kb
and conn from _connect (and after validating task_id) call
_enforce_worker_task_ownership(conn, str(tid)) and handle its failure by
returning a tool_error, then proceed to call kb.approve_task / kb.reject_task;
ensure this check runs before the mutating calls (kb.approve_task,
kb.reject_task) so only the owning worker can approve/reject tasks — apply the
same change to the other handler variants noted.
Comment on lines
+868
to
+870
| author = os.environ.get("HERMES_PROFILE") or "worker" | ||
| kb.add_comment(conn, str(tid), author, f"REJECTED: {reason}") | ||
| ok = kb.reject_task(conn, str(tid), reason=reason) |
There was a problem hiding this comment.
Avoid persisting a rejection comment when the reject CAS fails.
If the task exists but is no longer in review/human_review, add_comment() succeeds and reject_task() returns False, leaving a bogus REJECTED: note in the thread. Write the comment only after ok is true, or make the DB helper perform both steps atomically.
🤖 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 868 - 870, The current flow calls
kb.add_comment(...) before kb.reject_task(...), which can leave a spurious
"REJECTED:" comment if kb.reject_task returns False; change the logic in the
function containing these lines so you call kb.reject_task(conn, str(tid),
reason=reason) first, check the returned ok boolean, and only when ok is True
call kb.add_comment(conn, str(tid), author, f"REJECTED: {reason}");
alternatively, update the DB helper to expose an atomic operation (e.g.,
kb.reject_task_and_comment or a transactional wrapper) that performs the state
check, state update, and comment insert in one transaction to avoid inconsistent
comments.
added 2 commits
May 25, 2026 15:48
The kanban terminal-event notifier was hard-truncating payloads at
160-200 chars, which made blocked notifications routinely unactionable
— users couldn't see the question the worker was asking without
opening the dashboard.
Extract the caps as named module-level constants and bump them per
event kind:
- NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one
users actually answer)
- NOTIFY_DONE_SUMMARY_MAX = 800 (was 200)
- NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200)
- NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result
field, kept smaller because
new code uses summary)
All caps stay under Discord/Slack's ~2000-char single-message ceiling
so the largest payload still fits in one chat message on the tightest
platform we target. Telegram (4096) has plenty of headroom.
Tests cover: blocked carries the full reason, blocked truncates at
the documented cap on overflow, done carries the extended summary,
gave_up carries the extended error, and the cap budget stays ordered
(blocked > done > error > legacy result) so a chatty done summary
can't crowd out a critical blocked reason.
…ions
Adds a second review-flavored status to differentiate automated review
(the existing 'review' column the dispatcher auto-claims with the
sdlc-review agent) from human review (parked, awaiting Sahil's decision).
Workflow:
running -> review --[agent passes; merges PR]--> human_review
--[approve]--> done
--[reject]---> ready
-> review --[agent rejects]----------------> ready
kanban_db.py
- 'human_review' added to VALID_STATUSES.
- New atomic helpers: move_to_review, move_to_human_review, approve_task,
reject_task. Each does CAS on the expected source status and writes a
dedicated audit event (review_requested / human_review_requested /
approved / rejected) so the notifier and dashboard can render
differently per kind. approve_task also clears the failure counter,
recomputes ready for dependents, and cleans up the workspace
(mirroring complete_task).
- dispatch_once skips human_review tasks: no auto-spawn. The gateway
notifier subscription path is the only thing that fires for them.
CLI (hermes_cli/kanban.py)
- New subcommands: review / human-review / approve / reject. Pattern
matches block/unblock/complete: positional task_id, optional reason
with a comment+event audit trail.
Tool surface (tools/kanban_tools.py)
- kanban_review, kanban_human_review, kanban_approve, kanban_reject.
Mirror kanban_block/kanban_complete shape. Worker-ownership enforced
on the worker-initiated transitions (review, human_review).
Gateway notifier (gateway/run.py)
- TERMINAL_KINDS includes the three new event kinds.
- Renders ⏳ for human_review_requested, ✅ for approved, ↩ for
rejected, all capped at NOTIFY_BLOCKED_REASON_MAX.
Tests
- tests/hermes_cli/test_kanban_human_review.py: 12 tests covering
status validity, each transition, CAS atomicity, and dispatcher
hands-off behavior.
- tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning
the notifier glyph + content for each new event kind.
174 kanban + notifier tests pass; ruff clean on all touched files.
sahilm-ti
force-pushed
the
feat/kanban-human-review-v2
branch
from
May 25, 2026 10:18
52cc0a4 to
2a136e3
Compare
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-attribute |
2 |
unresolved-import |
1 |
First entries
tests/hermes_cli/test_kanban_human_review.py:129: [unresolved-attribute] unresolved-attribute: Attribute `status` is not defined on `None` in union `Task | None`
tests/hermes_cli/test_kanban_human_review.py:116: [unresolved-attribute] unresolved-attribute: Attribute `completed_at` is not defined on `None` in union `Task | None`
tests/hermes_cli/test_kanban_human_review.py:12: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues: none
Unchanged: 4806 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
sahilm-ti
pushed a commit
that referenced
this pull request
May 25, 2026
…NousResearch#31416) PR NousResearch#31416 (avoid persisting borrowed credential secrets) added sanitize_borrowed_credential_payload, which strips access_token from any auth.json pool entry whose (provider, source) isn't in the _PERSISTABLE_PROVIDER_SOURCES allowlist. (copilot, gh_cli) is borrowed (not in the allowlist), so the test fixture's pre-seeded access_token now gets stripped at load_pool() time, leaving the pool empty. resolve_target('1') then fails with 'No credential #1. Provider: copilot.' Fix: align the test with the new contract. At runtime, copilot tokens are hydrated by resolve_copilot_token() — mock that path so the pool gets an entry the test can remove. The behavior under test (suppression of gh_cli + env variants on remove) is unchanged. CI repro on origin/main HEAD; reproduced locally with stock checkout.
sahilm-ti
pushed a commit
that referenced
this pull request
May 25, 2026
…NousResearch#31416) PR NousResearch#31416 (avoid persisting borrowed credential secrets) added sanitize_borrowed_credential_payload, which strips access_token from any auth.json pool entry whose (provider, source) isn't in the _PERSISTABLE_PROVIDER_SOURCES allowlist. (copilot, gh_cli) is borrowed (not in the allowlist), so the test fixture's pre-seeded access_token now gets stripped at load_pool() time, leaving the pool empty. resolve_target('1') then fails with 'No credential #1. Provider: copilot.' Fix: align the test with the new contract. At runtime, copilot tokens are hydrated by resolve_copilot_token() — mock that path so the pool gets an entry the test can remove. The behavior under test (suppression of gh_cli + env variants on remove) is unchanged. CI repro on origin/main HEAD; reproduced locally with stock checkout.
The new human_review status added to VALID_STATUSES needs a column in
the dashboard, otherwise test_board_empty (which asserts the columns
exactly mirror VALID_STATUSES - {archived}) fails.
Also map the worker's commit-author email so check-attribution passes.
sahilm-ti
added a commit
that referenced
this pull request
May 25, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
This was referenced May 26, 2026
sahilm-ti
pushed a commit
that referenced
this pull request
May 28, 2026
…ookies
Mission-control style deploys reverse-proxy the dashboard at a path
prefix (e.g. mission-control.tilos.com/hermes/* -> :9119) and inject
X-Forwarded-Prefix: /hermes on every request. The SPA mount already
honoured this for asset URLs and the bootstrap __HERMES_BASE_PATH__,
but the OAuth gate didn't:
1. The gate's Location: header to /login and the 401 envelope's
login_url were built bare ("/login?next=..."). Under a /hermes
prefix the browser follows that to mission-control.tilos.com/login
which the proxy doesn't route to the dashboard.
2. _redirect_uri (the OAuth callback URL handed to the IDP) used
request.url_for() which doesn't honour X-Forwarded-Prefix
(Starlette/uvicorn only proxy_headers Host + Proto + For). The
IDP redirects back to /auth/callback instead of /hermes/auth/
callback → 404 in the user's browser.
3. Cookies were set with Path=/ which leaks them to other apps on
the same origin and won't be sent back on requests under the
prefix in the first place.
Fix threads the normalised prefix through every boundary:
* New hermes_cli/dashboard_auth/prefix.py — single source of truth
for X-Forwarded-Prefix parsing. web_server._normalise_prefix
becomes a re-export so the SPA mount, the gate, and the cookies
helper all agree.
* middleware._unauth_response builds login_url = f"{prefix}/login".
* routes._redirect_uri splices the prefix into the path component
of the IDP-bound URL (with full validation of the header).
* cookies.{set,clear}_{session,pkce}_cookie now take prefix="".
Path attribute switches to /hermes when set; cookie name switches
name variant (see below). Every caller passes the request's
normalised prefix.
Cookie hardening (Teknium's lesser-note #1 in the PR review): adopt
the __Host- / __Secure- cookie name prefixes per draft-west-cookie-
prefixes. The variant is selected from (use_https, prefix):
* Loopback HTTP → bare "hermes_session_at" (both prefixes require
Secure, incompatible with HTTP).
* HTTPS, direct deploy (Path=/) → "__Host-hermes_session_at".
Strongest spec: bound to exact origin, no Domain attribute, Secure
required.
* HTTPS, behind a proxy prefix (Path=/hermes) →
"__Secure-hermes_session_at". __Host- forbids Path != "/"; the
explicit Path=/hermes covers same-origin app isolation.
Setter and reader BOTH consult the prefix because the cookie *name*
changes — a reader that looked up the bare name when the setter wrote
__Secure- would never find the value. The reader falls back across
all three variants so a request whose shape changed mid-session (e.g.
post-deploy from no-prefix to /hermes) still picks up the existing
cookie until it expires.
Test coverage:
- tests/hermes_cli/test_dashboard_auth_prefix.py — new file. 11 tests
pinning:
• Location: /hermes/login on the gate's HTML redirect
• 401 envelope login_url carries the prefix
• Malformed X-Forwarded-Prefix is ignored (header-injection
defence; the script-tag value is normalised to empty string)
• _redirect_uri splices /hermes into the path (the property
that prevents the IDP-returns-to-404 failure)
• PKCE cookie uses Path=/hermes + __Secure- when proxied
• Session cookies use __Host- when direct, __Secure- when
proxied, bare on loopback HTTP
• End-to-end round trip with hand-managed PKCE cookie carriage
(TestClient can't simulate a Path=/hermes cookie automatically)
- tests/hermes_cli/test_dashboard_auth_cookies.py — rewritten to pin
each (use_https, prefix) shape produces its expected cookie name,
plus reader-side coverage that __Host- and __Secure- variants are
both recognised.
- Existing tests across middleware / 401-reauth / etc. updated to
match the new cookie names (substring contains instead of
startswith).
Mutation-tested: reverting _unauth_response to build the bare
"/login" URL trips exactly the two tests that pin the prefix
carriage, confirming the suite discriminates the regression.
sahilm-ti
added a commit
that referenced
this pull request
May 28, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
May 28, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
pushed a commit
that referenced
this pull request
May 28, 2026
…enderer The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712
sahilm-ti
added a commit
that referenced
this pull request
May 28, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
May 28, 2026
…enderer (#33) The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
added a commit
that referenced
this pull request
May 29, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
May 29, 2026
…enderer (#33) The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jun 1, 2026
Two CI flakes surfaced on PR NousResearch#34572 (both in files this PR doesn't touch; pre-existing host-dependent flakes): 1. test_process_registry::TestPopenLeakOnSetupFailure — the failure-cleanup tests use a fake proc.pid (8888/9999) and assert proc.kill() runs. But spawn_local's primary cleanup is os.killpg(os.getpgid(pid), SIGKILL), falling back to proc.kill() only on ProcessLookupError/PermissionError/ OSError. When the fake PID happens to exist on a busy host, os.getpgid succeeds, os.killpg fires against an UNRELATED real process group, and proc.kill() is never reached -> flaky AssertionError (and a real risk of SIGKILLing an innocent process group from a unit test). Patch os.getpgid to raise ProcessLookupError so the fallback path runs deterministically and no real killpg is ever issued. 2. test_web_server::test_resize_escape_is_forwarded — the receive loop calls the blocking conn.receive_bytes() with no exception guard. Once the child prints its winsize and exits, the PTY closes; on a missed-marker run the next recv blocks until the 30s pytest-timeout instead of failing fast. Add a try/except break (matching the working sibling tests) and bump the child's pre-read sleep 0.15s -> 0.5s so the resize reliably lands first. Verified: 4/4 pass across 3 consecutive runs; root cause for #1 reproduced (os.getpgid(1) succeeds -> old code skips proc.kill).
sahilm-ti
pushed a commit
that referenced
this pull request
Jun 3, 2026
Seven Copilot inline review comments on NousResearch#37679, four worth landing in a polish pass before merge: 1. _dispose_unused_adapter signature: 'BasePlatformAdapter' -> 'BasePlatformAdapter | None'. The function explicitly handles None and the reconnect watcher calls it with None in the except arm, so the annotation now matches the actual contract. 2. (duplicate of #1 on a different line) — same fix. 3. except Exception in _dispose_unused_adapter — the reviewer asked about asyncio.CancelledError swallowing. On Python 3.8+ (Hermes requires 3.13, see pyproject.toml), CancelledError inherits from BaseException, NOT Exception, so the existing 'except Exception' does NOT swallow task cancellation. Added an explicit comment explaining the contract so future readers don't repeat the analysis. We don't re-raise because the watcher loop intentionally treats dispose failures as best-effort: a failed dispose on an unowned adapter should not take down the watcher that's keeping the gateway alive. 4. _response_store = None after close in api_server.py — the reviewer flagged this for idempotency. Decided to keep the non-None state intentionally: setting it to None cascades to ~9 callers that access self._response_store without a None check, and 'close() is idempotent on a closed sqlite3 Connection' means the current code is already safe. The type stays stable; LSP doesn't flag a cascade of reportOptionalMemberAccess errors. (This matches the pre-existing pattern in the codebase — e.g. _mark_disconnected doesn't reset state to None either.) 5. _build_adapter_with_store: reviewer worried about disconnect() failing on the self.name property if __init__ wasn't called. Already handled: we set 'adapter.platform = Platform.API_SERVER' so the 'self.platform.value.title()' property returns 'Api_Server' without raising. The exception-swallowing branch in disconnect() does call self.name via the logger.debug format, so this is a real path that needs the platform attribute, and we have it. 6. test_disconnect_closes_response_store: bare 'pytest.raises(Exception)' -> 'pytest.raises(sqlite3.ProgrammingError)'. The bare Exception matcher would silently accept AttributeError, OperationalError, env-related issues, etc. The specific exception type ('Cannot operate on a closed database') is the actual signal we want — proves the SQLite conn is closed, not just that *something* raised. 7. test_nonretryable_failure_disposes_unowned_adapter: assertion tightened from '>= 1' to '== 1' on adapter._disconnect_calls. The docstring said 'exactly once', the assertion now matches. Catches the hypothetical 'watcher disposes the same adapter twice' regression that '>=' would have missed.
sahilm-ti
pushed a commit
that referenced
this pull request
Jun 25, 2026
…eation snapshot (NousResearch#44585) An unpinned cron job follows the global default provider (config.yaml model.default + resolve_runtime_provider). If that global state is changed after the job is created — e.g. a temporary switch to a paid provider like nous/claude-fable-5 — the job silently inherits it on its next tick and spends real money. This is the reported $7.73 incident: a job created under a free/default provider later inherited a temporary paid switch. Fix (ask #1 only) preserves the legitimate "unpinned job should follow model.default" use case by detecting *drift* rather than freezing the model: - create_job (cron/jobs.py): for UNPINNED, agent-backed jobs (no explicit provider, not no_agent), snapshot the provider that resolution WOULD pick right now into a new optional `provider_snapshot` field, resolved via the same resolve_runtime_provider() path the ticker uses. Fail-open to None on any resolution error so job creation never breaks. - run_job (cron/scheduler.py): right after runtime resolution, if the job has a provider_snapshot AND is unpinned AND the currently-resolved provider DIFFERS from the snapshot, fail closed for that run — make no paid call and deliver a loud, actionable alert naming both providers and telling the user to pin explicitly (`cronjob action=update job_id=.. provider=..`). Back-compat: jobs with no snapshot (pre-existing jobs, no_agent jobs, or any job whose creation-time resolution failed) behave exactly as before — the guard only engages when a snapshot exists. Explicitly-pinned jobs (job.provider set) are unaffected since they don't drift with global state. Tests: tests/cron/test_cron_provider_pin.py covers snapshot-matches (runs), snapshot-differs (fail closed, no agent constructed), no-snapshot back-compat, None-snapshot back-compat, explicitly-pinned (runs regardless), plus create_job snapshot capture/skip/fail-open. The fail-closed case is load-bearing (fails without the guard). Issue NousResearch#44585 asks #2-4 (hard-stop a running job, gateway-stop containment, fail-closed on provider mutation) are out of scope for this change.
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 1, 2026
…ture get_copilot_api_token now returns (api_token, base_url); the auth-remove suppression test still mocked it as a bare string, mis-unpacking into the credential-pool seed path and failing with 'No credential #1'.
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 1, 2026
…_id signature churn Two independent bugs evicted the cached gateway AIAgent on every turn, preventing the prompt cache from ever warming: 1. Model normalization mismatch: the post-run fallback-eviction check compared _agent.model (stripped in AIAgent.__init__) against the raw _resolve_gateway_model() config string. For vendor-prefixed config on native providers (e.g. 'deepseek/deepseek-v4-pro' vs 'deepseek-v4-pro') this was always unequal, so the agent was evicted after every successful run. Normalize _cfg_model the same way (skip aggregators). 2. Discord triggering message_id leaked into the cached system prompt via build_session_context_prompt()'s Discord IDs block. message_id changes every turn, so the agent-cache signature (computed from the ephemeral prompt) changed every Discord turn -> rebuild every message. The id is now injected per-turn into the user message (where per-turn content belongs and does not touch the cache signature); the cached IDs block carries a static pointer to it, preserving reply/react/pin via the discord tools. Adapted from NousResearch#28846. Bug #1 fix is the contributor's; bug #2 reworked to be non-destructive (keeps the triggering-id capability instead of deleting it). Redundant auto-reset eviction (already on main via NousResearch#9893/NousResearch#48031) and the wrong-premise reset_context_note plumbing from the original PR were dropped. Co-authored-by: Hermes Agent <hermes@nousresearch.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 1, 2026
… fail on '(empty)' sentinel Two related bugs caused subagent delegation to silently return empty summaries with 0 tokens when the user configured delegation.provider=bedrock alongside delegation.base_url=https://bedrock-runtime.<region>.amazonaws.com. Root cause #1 — misrouting in _resolve_delegation_credentials(): The configured_base_url branch unconditionally forced provider='custom' and api_mode='chat_completions', only specializing for chatgpt.com, anthropic, and kimi hosts. Bedrock (and other native-SDK providers) fell through as 'custom' + chat_completions, which then POSTed OpenAI-shaped JSON at Bedrock's native API. Bedrock rejected the payload and returned nothing, which looked like an empty LLM response to the child agent. Fix: when provider is one of {bedrock, vertex, google, google-genai}, skip the base_url short-circuit and fall through to resolve_runtime_provider(), which knows how to construct the proper SDK client. base_url can still be forwarded through that path for regional overrides. Root cause #2 — '(empty)' sentinel accepted as success: After N retries of empty LLM responses, run_agent.py emits the literal string '(empty)' as final_response. _run_single_child then hit `elif summary:` — '(empty)' is truthy, so status became 'completed' and the parent surfaced a blank result with no error. Users saw api_calls=4, tokens=0, duration~0.4s, status=completed. Fix: treat final_response.strip() == '(empty)' as a failure so the parent surfaces it instead of silently accepting zero-content 'success'. Both paths were reproduced in a live Hermes TUI session on us-west-2 Bedrock (provider=bedrock, model=us.anthropic.claude-sonnet-4-6) and are covered by new tests in tests/tools/test_delegate.py.
sahilm-ti
added a commit
that referenced
this pull request
Jul 3, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
Jul 3, 2026
…enderer (#33) The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 3, 2026
…) (#50) Two trailing test gaps left by the fork-sync rebase (t_2a1428d4): Defect A — stale NOTIFY_*_MAX imports (5 failures): Commit #1 relocated the notification-cap constants from gateway/run.py into the extracted mixin gateway/kanban_watchers.py, but tests/gateway/test_kanban_notifier.py still imported them from the old location (5 import sites). Repointed all to gateway.kanban_watchers. Defect B — scratch-cleanup contract regression (1 failure): Commit #6 deliberately moved own-dir scratch reaping out of complete_task (rmtree-ing the live worker's cwd crashes it) into the out-of-process gc_scratch_workspaces dispatcher tick. The upstream test test_cleanup_workspace_swept_after_last_child_completes asserted the child's own dir was gone synchronously after complete_task — an expectation the #6 synthesis removed by design. Verified production GC reliably reaps the completed child (status=done + claim_lock IS NULL satisfies gc_scratch_workspaces' WHERE clause; no leak window), so honored the GC-deferred model (option 1): the test now exercises gc_scratch_workspaces for the own-dir assertion and keeps the synchronous parent-sweep assertion intact. All 6 originally-failing tests pass; 279 tests across both files green. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
added a commit
that referenced
this pull request
Jul 9, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
Jul 9, 2026
…enderer (#33) The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 9, 2026
…) (#50) Two trailing test gaps left by the fork-sync rebase (t_2a1428d4): Defect A — stale NOTIFY_*_MAX imports (5 failures): Commit #1 relocated the notification-cap constants from gateway/run.py into the extracted mixin gateway/kanban_watchers.py, but tests/gateway/test_kanban_notifier.py still imported them from the old location (5 import sites). Repointed all to gateway.kanban_watchers. Defect B — scratch-cleanup contract regression (1 failure): Commit #6 deliberately moved own-dir scratch reaping out of complete_task (rmtree-ing the live worker's cwd crashes it) into the out-of-process gc_scratch_workspaces dispatcher tick. The upstream test test_cleanup_workspace_swept_after_last_child_completes asserted the child's own dir was gone synchronously after complete_task — an expectation the #6 synthesis removed by design. Verified production GC reliably reaps the completed child (status=done + claim_lock IS NULL satisfies gc_scratch_workspaces' WHERE clause; no leak window), so honored the GC-deferred model (option 1): the test now exercises gc_scratch_workspaces for the own-dir assertion and keeps the synchronous parent-sweep assertion intact. All 6 originally-failing tests pass; 279 tests across both files green. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
added a commit
that referenced
this pull request
Jul 10, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
Jul 10, 2026
…enderer (#33) The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 10, 2026
…) (#50) Two trailing test gaps left by the fork-sync rebase (t_2a1428d4): Defect A — stale NOTIFY_*_MAX imports (5 failures): Commit #1 relocated the notification-cap constants from gateway/run.py into the extracted mixin gateway/kanban_watchers.py, but tests/gateway/test_kanban_notifier.py still imported them from the old location (5 import sites). Repointed all to gateway.kanban_watchers. Defect B — scratch-cleanup contract regression (1 failure): Commit #6 deliberately moved own-dir scratch reaping out of complete_task (rmtree-ing the live worker's cwd crashes it) into the out-of-process gc_scratch_workspaces dispatcher tick. The upstream test test_cleanup_workspace_swept_after_last_child_completes asserted the child's own dir was gone synchronously after complete_task — an expectation the #6 synthesis removed by design. Verified production GC reliably reaps the completed child (status=done + claim_lock IS NULL satisfies gc_scratch_workspaces' WHERE clause; no leak window), so honored the GC-deferred model (option 1): the test now exercises gc_scratch_workspaces for the own-dir assertion and keeps the synchronous parent-sweep assertion intact. All 6 originally-failing tests pass; 279 tests across both files green. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
added a commit
that referenced
this pull request
Jul 11, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
sahilm-ti
added a commit
that referenced
this pull request
Jul 11, 2026
…enderer (#33) The _render_table_block_for_telegram function had two failure modes that caused the first column cell to appear twice in Telegram output — once as the bold heading and once as the first bullet: 1. The has_row_label_col=False path used string-equality dedup (value == heading) which failed for markdown-wrapped cells like **#1 — foo** (heading matched raw cell, but dedup compared after .strip() without any markdown normalization, producing ****text**** double-bold). 2. The has_row_label_col=True path had no dedup at all and was only invoked when the header row had an explicitly-empty first cell — a form the orchestrator rarely emits. Fix: remove both branches entirely. The design invariant is now structural: cells[0] is ALWAYS the heading, cells[1:] are ALWAYS the bullets aligned with headers[1:]. The heading cell is excluded by construction, so no dedup step is needed. New helper _normalize_table_heading() strips the outermost markdown wrapper (**/__/*/_/`) and NBSP/ZWSP chars before re-wrapping the heading in bold, preventing double-bold artefacts. Empty bullet values (short rows) are silently omitted instead of emitting '• Header: ' with a trailing blank. BEFORE (Shape B): INPUT: | **#1 — @-mention rule** | `path/foo.md` | ADD | OUTPUT: ****#1 — @-mention rule**** • Entry: **#1 — @-mention rule** • Target: `path/foo.md` AFTER: OUTPUT: **#1 — @-mention rule** • Target: `path/foo.md` • Status: ADD BEFORE (Shape A): INPUT: | 1 | Bundle download | ✅ confirmed | OUTPUT: **1** • #: 1 • Topic: Bundle download (dedup would skip '• #: 1' only if strict equality matched — numeric cols worked but markdown-wrapped ones did not) AFTER: OUTPUT: **1** • Topic: Bundle download • Verdict: ✅ confirmed Shape C (empty first header / has_row_label_col=True) is unchanged in behavior — Alice/Lab1 become headings, data columns become bullets. Adds 10 new unit tests: 3 reproduction shapes (A/B/C), 7 unit tests for _normalize_table_heading covering bold/italic/backtick/NBSP/ZWSP/nested wrappers, plus the empty-bullet omission edge case. Existing 101 tests all pass. ty diagnostics unchanged at 77 (pre-existing). Resolves: kanban/t_e611f712 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
pushed a commit
that referenced
this pull request
Jul 11, 2026
…) (#50) Two trailing test gaps left by the fork-sync rebase (t_2a1428d4): Defect A — stale NOTIFY_*_MAX imports (5 failures): Commit #1 relocated the notification-cap constants from gateway/run.py into the extracted mixin gateway/kanban_watchers.py, but tests/gateway/test_kanban_notifier.py still imported them from the old location (5 import sites). Repointed all to gateway.kanban_watchers. Defect B — scratch-cleanup contract regression (1 failure): Commit #6 deliberately moved own-dir scratch reaping out of complete_task (rmtree-ing the live worker's cwd crashes it) into the out-of-process gc_scratch_workspaces dispatcher tick. The upstream test test_cleanup_workspace_swept_after_last_child_completes asserted the child's own dir was gone synchronously after complete_task — an expectation the #6 synthesis removed by design. Verified production GC reliably reaps the completed child (status=done + claim_lock IS NULL satisfies gc_scratch_workspaces' WHERE clause; no leak window), so honored the GC-deferred model (option 1): the test now exercises gc_scratch_workspaces for the own-dir assertion and keeps the synchronous parent-sweep assertion intact. All 6 originally-failing tests pass; 279 tests across both files green. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
sahilm-ti
added a commit
that referenced
this pull request
Jul 13, 2026
…nd CLI (#1) * fix(gateway): bump kanban notifier truncation caps and name them The kanban terminal-event notifier was hard-truncating payloads at 160-200 chars, which made blocked notifications routinely unactionable — users couldn't see the question the worker was asking without opening the dashboard. Extract the caps as named module-level constants and bump them per event kind: - NOTIFY_BLOCKED_REASON_MAX = 1500 (was 160; this is the one users actually answer) - NOTIFY_DONE_SUMMARY_MAX = 800 (was 200) - NOTIFY_GAVE_UP_ERROR_MAX = 600 (was 200) - NOTIFY_DONE_RESULT_LEGACY_MAX = 400 (was 160; legacy task.result field, kept smaller because new code uses summary) All caps stay under Discord/Slack's ~2000-char single-message ceiling so the largest payload still fits in one chat message on the tightest platform we target. Telegram (4096) has plenty of headroom. Tests cover: blocked carries the full reason, blocked truncates at the documented cap on overflow, done carries the extended summary, gave_up carries the extended error, and the cap budget stays ordered (blocked > done > error > legacy result) so a chatty done summary can't crowd out a critical blocked reason. * feat(kanban): add human_review status + review/approve/reject transitions Adds a second review-flavored status to differentiate automated review (the existing 'review' column the dispatcher auto-claims with the sdlc-review agent) from human review (parked, awaiting Sahil's decision). Workflow: running -> review --[agent passes; merges PR]--> human_review --[approve]--> done --[reject]---> ready -> review --[agent rejects]----------------> ready kanban_db.py - 'human_review' added to VALID_STATUSES. - New atomic helpers: move_to_review, move_to_human_review, approve_task, reject_task. Each does CAS on the expected source status and writes a dedicated audit event (review_requested / human_review_requested / approved / rejected) so the notifier and dashboard can render differently per kind. approve_task also clears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirroring complete_task). - dispatch_once skips human_review tasks: no auto-spawn. The gateway notifier subscription path is the only thing that fires for them. CLI (hermes_cli/kanban.py) - New subcommands: review / human-review / approve / reject. Pattern matches block/unblock/complete: positional task_id, optional reason with a comment+event audit trail. Tool surface (tools/kanban_tools.py) - kanban_review, kanban_human_review, kanban_approve, kanban_reject. Mirror kanban_block/kanban_complete shape. Worker-ownership enforced on the worker-initiated transitions (review, human_review). Gateway notifier (gateway/run.py) - TERMINAL_KINDS includes the three new event kinds. - Renders ⏳ for human_review_requested, ✅ for approved, ↩ for rejected, all capped at NOTIFY_BLOCKED_REASON_MAX. Tests - tests/hermes_cli/test_kanban_human_review.py: 12 tests covering status validity, each transition, CAS atomicity, and dispatcher hands-off behavior. - tests/gateway/test_kanban_notifier_human_review.py: 3 tests pinning the notifier glyph + content for each new event kind. 174 kanban + notifier tests pass; ruff clean on all touched files. * fix(kanban-dashboard): include human_review in BOARD_COLUMNS The new human_review status added to VALID_STATUSES needs a column in the dashboard, otherwise test_board_empty (which asserts the columns exactly mirror VALID_STATUSES - {archived}) fails. Also map the worker's commit-author email so check-attribution passes. --------- Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
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.
What
Differentiates automated review (existing
reviewstatus — dispatcher auto-spawns thesdlc-reviewagent) from human review (newhuman_reviewstatus — parked, waits for the human reviewer to approve / reject).Also surfaces a CLI + tool API for manual transitions, so a worker can request review itself after opening a PR.
Re-target note
This is a re-open of NousResearch#30967, which was opened in error against upstream. Re-targeted at the personal fork per the fork-only rule. Same two commits, cherry-picked onto sahilm-ti/hermes-agent main.
Changes
hermes_cli/kanban_db.pyhuman_reviewadded toVALID_STATUSES.move_to_review,move_to_human_review,approve_task,reject_task. Each writes a dedicated audit event (review_requested/human_review_requested/approved/rejected).approve_taskclears the failure counter, recomputes ready for dependents, and cleans up the workspace (mirrorscomplete_task).reject_taskflips status back toreadyand synthesises a closing run so the rejection rationale surfaces via the normal prior-runs path on the next worker'skanban_show.dispatch_onceskipshuman_reviewtasks: no auto-spawn.hermes_cli/kanban.py(CLI)hermes kanban review,human-review,approve,reject.tools/kanban_tools.py(worker tool surface)kanban_review,kanban_human_review,kanban_approve,kanban_reject. Worker-ownership enforced on worker-initiated transitions.gateway/run.py(notifier)TERMINAL_KINDSextended; ⏳ forhuman_review_requested, ✅ forapproved, ↩ forrejected, all capped atNOTIFY_BLOCKED_REASON_MAX(1500 chars).Tests
tests/hermes_cli/test_kanban_human_review.py— 12 teststests/gateway/test_kanban_notifier_human_review.py— 3 testsAll 174 kanban + notifier tests pass locally. Ruff clean on touched files.
Out of scope
sdlc-reviewskill itself (separate config repo PR).Summary by CodeRabbit
New Features
review,human_review,approve, andrejectCLI commands.Tests