fix: preserve blocked Kanban cards from auto-decomposition - #64
Open
sahilm-ai wants to merge 67 commits into
Open
fix: preserve blocked Kanban cards from auto-decomposition#64sahilm-ai wants to merge 67 commits into
sahilm-ai wants to merge 67 commits into
Conversation
…e_execution (#4) * fix(tools): defensive ENOENT handling for deleted cwd in terminal/code_execution Workers spawned by the kanban dispatcher were crash-looping within 60s of spawn when their scratch workspace directory got cleaned out from under the live worker process. Every tool dispatch hit FileNotFoundError on os.getcwd() in _get_env_config (terminal_tool.py:1021) and _resolve_child_cwd (code_execution_tool.py:1638), killing the worker before it could do any work. Observed concretely: merge-train task t_83c080e5 spawned 6 consecutive workers (runs 49-55), each dying with 'pid X exited with code 1' after ~60s. errors.log showed FileNotFoundError on os.getcwd(). This is the symptom fix — wrap os.getcwd() in try/except in both call sites and fall back to $HOME / staging_dir respectively. The root-cause workspace lifecycle bug (scratch dir getting cleaned mid-run) still needs to be tracked separately, but with these guards the worker survives the race instead of dying. Tests: tests/tools/test_terminal_cwd_enoent.py covers both call sites by rmdir-ing the cwd before invoking the function under test. * chore: add sahil@trilogy.com to AUTHOR_MAP (→sahilm-ti) --------- Co-authored-by: Sahil Marwaha <sahil@trilogy.com>
…spatcher GC (#6) complete_task -> _cleanup_workspace was calling shutil.rmtree(scratch_dir) inside the worker process itself, with cwd == the scratch dir. Removing the dir under the live worker caused every subsequent os.getcwd() to raise FileNotFoundError and crashed the worker mid-completion, and again every 60s from the terminal_tool cleanup thread. Fix: - _cleanup_workspace no longer does rmtree; only kills the stale tmux session - New gc_scratch_workspaces(conn) reaps scratch dirs for tasks in done/blocked/archived state with no active claim. Called from dispatch_once, so it runs in the dispatcher process — out-of-process from any worker — safe to rmtree. Complements the symptom belt shipped in PR #4 (defensive ENOENT handling in tools/terminal_tool.py + tools/code_execution_tool.py). Tests: tests/hermes_cli/test_kanban_workspace_self_delete.py (3 tests, all pass). Co-authored-by: Sahil Marwaha <sahil@trilogy.com>
…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>
* kanban: pre-flight skill validation to prevent worker crash-loops
The dispatcher injects --skills <name> for each task-required skill
plus the auto-loaded kanban-worker. If any of those names does not
resolve under the worker's HERMES_HOME, the CLI aborts at startup
with:
Error: Unknown skill(s): <names>
The dispatcher sees exit code 1, marks the run crashed, and
re-spawns on the next tick — burning attempts indefinitely
(observed: 21 consecutive crashes / 4h of wasted compute before an
operator noticed via the diagnostics panel).
Three changes:
1. kanban_db.py: New _resolve_skill_under_home() + _validate_task_skills().
Filesystem-only check (no module import, no skill_view) so the
dispatcher can validate against a *different* HERMES_HOME than
its own without contaminating cached resolver state. Mirrors the
CLI's resolver order: <home>/skills + skills.external_dirs from
<home>/config.yaml.
2. kanban_db.py: Both spawn paths (normal + review) call the
validator after claim, before spawn. Missing skill -> block_task()
with a precise reason naming the missing skills and the HERMES_HOME
they were looked up under. Operator can fix once (install skill or
add external_dirs entry) and unblock, instead of watching the loop
tick.
3. hermes_cli/main.py + gateway/run.py: Honor a new
.no-bundled-skills marker in HERMES_HOME so bundled-skill sync
stays out of profile dirs that intentionally rely exclusively on
external_dirs for their skill library. Prevents stale per-profile
copies from re-seeding and colliding on skill name with the
root-home source of truth.
Before: 21+ crashed runs, no progress, no signal to the operator.
After: 1 blocked run with the exact failing skill list in the reason.
Repro:
- Profile P with HERMES_HOME=~/.hermes/profiles/P
- ~/.hermes/profiles/P/skills/ does not contain skill X
- skills.external_dirs in profile config does not include the dir
that hosts X
- Task assigned to P with skills: [X]
- Before: infinite crash loop until consecutive_failures cap.
- After: task moves to blocked with reason
'missing skills under HERMES_HOME=...: X. Install them or add
their source dir to skills.external_dirs ...'
* kanban: skip skill pre-flight when spawn_fn is injected (tests)
Test failures: test_dispatch_review_spawns_with_correct_skills,
test_dispatch_review_spawns_when_ready_empty asserted len(res.spawned)==1
but got 0. Tests inject a capture_spawn stub that never execs the CLI,
so a missing skill on the test fixture's HERMES_HOME is harmless —
but the pre-flight was auto-blocking the task anyway.
Skip the pre-flight whenever a spawn_fn is injected: by definition the
caller isn't going through _default_spawn, so the 'CLI dies at startup
on missing skill' failure mode doesn't apply.
Also: add sahil.marwaha@trilogy.com -> sahilm-ti to AUTHOR_MAP so the
attribution check passes (it was failing on this PR with 'New
contributor email(s) not in AUTHOR_MAP').
Add the new 'human_review' status (introduced alongside the kanban_review/approve/reject machinery) to the dashboard's column list so cards in that state are actually visible on the board. Backend: append "human_review" between "review" and "done" in plugins/kanban/dashboard/plugin_api.py::BOARD_COLUMNS. The endpoint already buckets tasks by status into BOARD_COLUMNS, so this is the only Python change required. Frontend (dist/index.js — there is no src/ tree, the bundled JS is the canonical UI): - COLUMN_ORDER: extended to the full status list (scheduled and review were also missing from the JS fallback). The render loop iterates board.columns from the API, so missing keys here would just degrade labels; aligning it with the backend keeps the frontend's hard-coded order honest. - FALLBACK_COLUMN_LABEL / FALLBACK_COLUMN_HELP: added entries for scheduled, review, and human_review so the dashboard renders sane text when the i18n catalog has no key. - COLUMN_DOT: added scheduled/review/human_review classes. Style (dist/style.css): added .hermes-kanban-dot-scheduled, .hermes-kanban-dot-review, .hermes-kanban-dot-human-review with distinct colors so the new columns are visually distinguishable. The Python human_review status itself ships on the feat/kanban-human-review branch and PR NousResearch#30967; this change is a no-op until that lands but is safe to merge independently — any tasks ever set to status='human_review' will now have a home. Co-authored-by: Sahil Marwaha <sahilm@triangleinvestments.com>
…nal) (#13) Stale per-profile skill copies were crashing every kanban worker spawn: `skill_view('kanban-worker')` returned success=False with 'Ambiguous skill name' when two SKILL.md files matched the bare name, which the CLI's --skills preload path surfaced as 'Unknown skill(s): kanban-worker' and aborted before the agent loop ran. Today's incident: stale ~/.hermes/profiles/braintrusteng/skills/devops/kanban-worker/ (v2.0.0) collided with the canonical ~/.hermes/skills/devops/ kanban-worker/ (v2.2.0). Four crash cycles burned before the dispatcher gave up. Refusing to guess made sense for an interactive skill_view() call — not for a CLI preload where there's no human to disambiguate. Pick deterministically and warn loudly so operators still see the stale copy. Resolution order (in tools/skills_tool.py): 1. SKILLS_DIR (= $HERMES_HOME/skills) wins by tier. 2. external_dirs in declaration order, one tier per entry. 3. Within a tier, most-recent SKILL.md mtime wins. A WARN log names the chosen path and the shadowed candidates so the operator can clean up. Part 2: _kanban_worker_skill_available delegated to the existing _resolve_skill_under_home helper, which already walked the full <home>/skills + skills.external_dirs set the worker would actually use. The bespoke check missed profiles (like braintrusteng) that keep an empty per-profile skills/ and route every lookup through external_dirs. Tests: - tests/tools/test_skills_tool.py: TestSkillViewCollisionDetection rewritten — local-wins-by-tier, external-wins-by-declaration-order, same-tier-wins-by-mtime, explicit-path-still-works, WARN-emitted-on- silent-resolve. - tests/hermes_cli/test_kanban_db.py: TestKanbanWorkerSkillAvailable added — local-only, external-only, and the today's-incident shape (local v2.0.0 + external v2.2.0 collision) all return True. 416 tests in the affected modules pass (415 + 1 unrelated skip). Closes kanban task t_4a5d78f8.
…#14) - Add _infer_workspace_kind() that picks 'worktree' when title/body mentions hermes-agent source paths (hermes_cli/, tools/, gateway/, tests/, agent/) or git/PR verbs (git rebase, gh pr); 'scratch' otherwise. Applies only when caller omits workspace_kind. - Default worktree path: ~/.hermes/worktrees/<task_id> (deterministic on task id so respawns reuse committed work via the kanban/<task_id> branch). - New ensure_worktree() provisions the worktree via 'git worktree add' against HERMES_KANBAN_LIVE_CHECKOUT (default ~/.hermes/hermes-agent), base ref HERMES_KANBAN_WORKTREE_BASE_REF (default myfork/main). Idempotent: respawn reuses the existing worktree + branch. - Wire into dispatcher's spawn loop (ready + review paths) right after resolve_workspace. - New gc_worktree_workspaces() removes worktree + kanban/<task_id> branch 24h after task completion. Wired into dispatch_once tick. - tools/kanban_tools.py: pass None to kanban_db when caller omitted workspace_kind so inference kicks in. - Tests: keyword inference (positive, neutral, explicit-override, git-rebase keyword), ensure_worktree fresh + respawn, gc removal and 24h grace skip. Why: workers concurrently editing /Users/sahilmarwaha/.hermes/hermes-agent were clobbering each other's git state — checkout/reset/rebase against the same HEAD. Per-task worktrees isolate everyone. Closes t_3158705c. Co-authored-by: braintrusteng <sahil@nousresearch.com>
…ult; add prune-skills tool (#15) Profiles now resolve skills from the canonical root tree (~/.hermes/skills) via skills.external_dirs instead of getting per-profile bundled copies that drift and create ambiguous-skill collisions in skill_view. Changes: - create_profile() default: no_skills=True. The new profile gets an empty skills/ dir, a .no-bundled-skills marker, and a config.yaml stanza pointing skills.external_dirs at ~/.hermes/skills. Clone flows (--clone / --clone-all) still carry their own skills. - New --with-bundled-skills opt-in flag restores the old per-profile bundled-copy behavior for users who explicitly want it. - New 'hermes profile prune-skills <name>' command moves stale duplicate skill copies out of an existing profile to a timestamped backup dir: same-content / older-mtime copies prune by default; user-edited copies are kept unless --force is passed; skills absent from root are kept (profile-local). After a successful prune the marker + external_dirs stanza are written so future syncs don't recreate the drift. - Docs: website/docs/user-guide/features/skills.md gains a Profile-skills model section, and reference/profile-commands.md documents both flags and the new prune-skills subcommand. - Config schema: skills.external_dirs description rewritten to name the canonical model. Tests: 465 pass (test_profiles, test_web_server, test_setup, gateway runner-startup, skill_manager, skills_tool). New TestNewSkillsDefault and TestPruneProfileSkills classes cover the default flip, the opt-in, marker + external_dirs seeding, and the four prune decision branches (identical, user-modified, profile-local, dry-run / force / default reject). Closes the per-profile skill drift root-cause that triggered the t_8502998b worker crash loop. Co-authored-by: braintrusteng-worker <kanban-worker@localhost>
…arch.com to AUTHOR_MAP (#16) Stops Contributor Attribution Check from failing on every kanban-worker PR when the machine's git global identity drifts. - hermes_cli/kanban_db.py: _default_spawn now sets GIT_AUTHOR_NAME/EMAIL and GIT_COMMITTER_NAME/EMAIL on the worker subprocess env. Defaults are the sahilm-ti noreply identity, overridable via kanban.git_identity_name and kanban.git_identity_email in config.yaml. Pre-existing values in the parent env survive untouched. - scripts/release.py: add sahil@nousresearch.com -> sahilm-ti to AUTHOR_MAP so legacy commits sitting on branches still pass the check. - tests/hermes_cli/test_kanban_db.py: add test_worker_spawn_sets_git_identity and test_worker_spawn_respects_user_git_identity_override.
PR #6 moved inline shutil.rmtree out of complete_task into gc_scratch_workspaces (run by the dispatcher) so workers don't delete their own cwd. Two upstream tests that asserted post-complete_task dir removal now also drive gc_scratch_workspaces to verify the new out-of-process cleanup path.
…ban_review (#17) * kanban-worker prompt: replace kanban_block(review-required:) with kanban_review The KANBAN_GUIDANCE block injected into every kanban worker's system prompt (agent/prompt_builder.py, step 5 of the lifecycle) was still documenting the old pre-two-stage-flow review handoff pattern. Workers were therefore calling kanban_block(reason='review-required:...') on code-change cards instead of using the dedicated kanban_review verb, which: * caused blocked notifications to fire on cards that are actually ready for review (gateway pings as 'blocked' even though work is complete), and * left the cards invisible to the sdlc-review auto-reviewer, which only polls 'review' status. This commit rewrites step 5 to instruct workers to open a PR, drop the structured metadata into kanban_comment, then call kanban_review(reason='PR <url>, AC: <one-line>'). Adds an explicit note that the auto-reviewer does NOT merge (Sahil merges every PR himself) and reserves kanban_block for genuine blockers with semantic prefixes (needs-creds, needs-smoke-test, etc.). Paired with sahilm-ti/hermes-config#TBD which cleans up the equivalent documentation in the skill library (kanban-worker, kanban-orchestrator, sdlc-review, braintrust-eng-process, kanban-codex-lane). Both PRs need to land together for worker behaviour and skill docs to stay in sync. * fix: trim KANBAN_GUIDANCE to satisfy 4096-char prompt-size cap Removes the 'Reviewing-then-completing is more honest than auto-completing work' sentence (32-char savings; final length 4032). The two-stage flow's core instruction (use kanban_review for PR handoffs; kanban_block reserved for genuine blockers) is unchanged. Fixes test_kanban_guidance_prompt_size_bounded (was 4128 > 4096). --------- Co-authored-by: Sahil Marwaha <sahil@trilogy.com> Co-authored-by: Sahil Marwaha <sahilmarwaha@Sahils-MacBook-Pro.local>
Co-authored-by: braintrusteng <braintrusteng@kanban>
GH_TOKEN and GITHUB_TOKEN are general-purpose gh CLI / git auth
variables — used by every git remote helper, CI job, and developer
machine. Listing them on the Copilot provider's api_key_env_vars
propagated them into _HERMES_PROVIDER_ENV_BLOCKLIST and silently
stripped them from every terminal subprocess, breaking 'gh pr
create', 'git push', 'gh auth status' for kanban workers.
Changes:
- hermes_cli/auth.py: Copilot provider api_key_env_vars scoped to
('COPILOT_GITHUB_TOKEN',). Generic GitHub tokens stay reachable
via copilot_auth.COPILOT_ENV_VARS (lookup precedence unchanged).
- hermes_cli/providers.py: same trim on the github-copilot overlay
extra_env_vars tuple.
- tools/environments/local.py: drop the explicit 'GH_TOKEN' entry
from the hardcoded extras set in _build_provider_env_blocklist.
- hermes_cli/config.py: re-category GITHUB_TOKEN from 'tool' to
'skill' so OPTIONAL_ENV_VARS no longer feeds it into the blocklist
(the 'skill' category exists precisely for vars that legitimately
need subprocess passthrough).
- hermes_cli/setup.py: _model_section_has_credentials() now consults
copilot_auth.COPILOT_ENV_VARS when checking the copilot provider so
an explicit copilot config + GH_TOKEN still counts as configured.
Tests:
- tests/tools/test_env_passthrough.py: new regression covering
GH_TOKEN / GITHUB_TOKEN passthrough + COPILOT_ENV_VARS lookup.
- Updated affected blocklist/api-key tests to assert the new shape
while pointing at copilot_auth.COPILOT_ENV_VARS for the full
lookup precedence (docs-facing list unchanged in behaviour).
Refs kanban task t_e9b3a894.
Co-authored-by: Sahil Marwaha <sahil@nousresearch.com>
…d yields on post-PR rejection (#10) Two related infra bugs surfaced during the t_8f22a89e auto-review loop. Bug 1: reject_task / kanban_reject rejected review-claimed tasks. claim_review_task transitions review -> running before spawning the reviewer worker, so by the time the worker calls kanban_reject, the task is in 'running' and the old WHERE clause (status IN review, human_review) refused the transition. Workers were writing the transition directly to SQLite to work around this. reject_task now has a fallback path: when the primary review/human_review update misses, it checks the most recent 'claimed' event for the task's current_run_id and accepts the rejection iff payload.source_status is review or human_review. The reviewer's active run is closed via _end_run with outcome='rejected', the claim_lock is dropped, and the task flips back to 'ready' the same way the parked-review path does. Bug 2: the active_pr respawn guard blocked the reject -> fix-same-PR flow. check_respawn_guard returned 'active_pr' whenever any GitHub PR URL appeared in a recent comment, even after the auto-reviewer had rejected that PR — leaving the task stuck in 'ready' while the dispatcher refused to spawn the fix attempt. Now the guard suppresses itself when the most recent rejection signal (task_events.kind='rejected' OR task_runs.outcome='rejected') post-dates the most recent PR-URL comment. The rejection is the signal to keep iterating on the existing PR. If the worker opens a new PR after the reject, the guard re-engages on that newer PR comment. Also tightens the kanban_reject tool error to name the running-claim case explicitly. Tests: 8 new in tests/hermes_cli/test_kanban_db.py covering both bugs and their inverse cases (normal running tasks still refuse rejection; guard still fires when PR post-dates rejection). Full kanban suite (786 tests) green. Co-authored-by: braintrusteng worker <braintrusteng@sahilm.local>
The dispatcher's default GIT_AUTHOR_NAME/EMAIL now point at the dedicated sahilm-ai persona so agent-generated commits are distinguishable from Sahil's personal commits. Also adds the sahilm-ai noreply email to the release.py AUTHOR_MAP so contributor_audit recognises agent commits. Old sahilm-ti AUTHOR_MAP entries stay for historical commits. Refs: kanban t_b3e79473 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
Adds a dispatcher-side breaker that quarantines an assignee after N crashes in M seconds (defaults 3/120s) so a broken venv / missing module / wedged credential storm doesn't burn the spawn budget forever.
- New kanban_quarantine table (assignee PK, cooldown, trip_count, single probe slot).
- evaluate_crash_breaker() counts crashed events per assignee within window; trip writes/refreshes quarantine row and emits a quarantined event on the most-recent crashed task (existing notify-subs deliver it).
- is_assignee_quarantined() atomically grants exactly one probe slot when cooldown elapses.
- Dispatcher ready + review loops check quarantine before claiming; probe success in complete_task clears, probe crash in detect_crashed_workers extends cooldown with 2x backoff (capped at max_cooldown).
- Gateway reads kanban.crash_breaker_{enabled,max_crashes,window_seconds,cooldown_seconds,max_cooldown_seconds} (defaults 3/120s/300s/3600s, enabled=true) and passes through every tick.
- Notifier learns the quarantined event kind and formats a 🚨 message naming assignee, crash count, window, cooldown, and last reason.
- 10 new tests covering trip threshold, window scope, no-double-trip, dispatch block, review-column block, single-probe-per-tick, probe success clears, probe crash backoff + cap, breaker-off backwards compat.
…copy (finish PR #15) (#22) PR #15 added the .no-bundled-skills marker check at every documented caller of sync_skills(), but the DESCRIPTION.md copy loop inside sync_skills() ran unconditionally — so when a profile-bootstrap path that *had* the marker check still ended up calling sync_skills() directly (e.g. seed_profile_skills() racing the marker write, or a caller path PR #15 didn't cover), the SKILL.md copy was correctly skipped but the category DESCRIPTION.md files still landed in the profile skills dir. Result: opted-out profiles (braintrusteng, bt-optimizer) accumulated useless byte-identical DESCRIPTION.md stubs under */skills/<category>/ that duplicate the global versions at ~/.hermes/skills/<category>/. agent/prompt_builder.py reads category descriptions from the external (global) dir already (line 1162), so the profile stubs are dead bytes. Fix is defense-in-depth: sync_skills() itself now checks the marker at HERMES_HOME (SKILLS_DIR.parent) and short-circuits to a zero-effect return with skipped_opt_out=True before either loop runs. Regression tests cover both paths (marker present skips everything; marker absent proceeds normally). Also fixes test_nonexistent_bundled_dir to patch SKILLS_DIR to a clean tmp path so the developer's live opt-out marker doesn't short-circuit the test. Refs PR #15 (t_00313e4b), kanban t_ad76c02b
… subs (#21) When a worker (e.g. a kanban_orchestrator-style fan-out, or any dispatcher-spawned worker calling kanban_create with parents=[...]) creates a child task, the child must keep notifying the same gateway chats that are tracking the parent. Otherwise notifications silently drop on fan-outs: Sahil stops getting Telegram pings on a child even though he's actively tracking the parent. The old behaviour relied entirely on session-origin context vars (HERMES_SESSION_PLATFORM / _CHAT_ID / _THREAD_ID) bound by the gateway when handling the originating message. Workers spawned by the kanban dispatcher (CLI process tree) have no such vars, so kanban_create produced a card with zero rows in kanban_notify_subs. Fix: when parents=[...] is non-empty and auto_subscribe is True, inherit every parent's subscriptions onto the new child. UNION across parents, dedupe by (platform, chat_id, thread_id), preserve each sub's notifier_profile so the right gateway delivers the event. add_notify_sub is already INSERT OR IGNORE, so origin + parent overlap is handled at the DB layer. Edge cases covered by new tests: - no origin + 1 parent w/ sub -> child inherits 1 sub - no origin + 2 parents, overlapping subs -> child gets UNION deduped - origin + parent point at same chat -> single row (dedupe) - auto_subscribe=False -> neither origin nor parents inherited - parent has zero subs -> child has zero subs (status quo) Refs t_2b0e7ab6. Follows on t_b212a749 which fixed notifier_profile propagation but didn't cover the 'new card from worker context' case.
* design: post-approve merger agent (t_5a521a19) Add DESIGN.md covering: - New 'merging' kanban status between human_review and done - approve_task() PR-detection routing (human_review -> merging vs done) - New post-approve-merger profile and skill - Idempotency via gh pr view state as source of truth - All 6 PR-state branches and their outcomes - dispatch_once() merging column (parallel to review column) - Gateway notifier events - Open Q on dashboard column order for Sahil No code yet — approval gate. * feat(kanban): post-approve merger agent (t_5a521a19) When kanban_approve is called on a human_review task that has an associated PR URL, instead of transitioning directly to done, the task is claimed for a post-approve-merger worker which merges the PR and transitions to done/blocked. Changes: - kanban_db.py: add _extract_pr_url(), claim_merger_task(), update approve_task() to return (bool, outcome, pr_url, task) tuple and route PR-bearing tasks via claim_merger_task - kanban_tools.py: update _handle_approve() to spawn post-approve-merger worker when outcome=merge_triggered - kanban.py: update _cmd_approve() to spawn merger from CLI path too, print descriptive message for merge-triggered outcome - gateway/run.py: add merge_requested to TERMINAL_KINDS, add notifier message for merge_requested events - skills/devops/post-approve-merger/SKILL.md: new skill for the merger worker with full PR state-machine (6 branches), auth pattern, and idempotency rules - tests/hermes_cli/test_kanban_merging.py: new tests for _extract_pr_url, claim_merger_task, approve_task routing (15 test cases) - tests/hermes_cli/test_kanban_human_review.py: update existing tests for new approve_task tuple return type - DESIGN.md: updated to reflect no-merging-status decision No new VALID_STATUSES added. No new dispatcher column needed. The post-approve-merger profile lives at ~/.hermes/profiles/post-approve-merger/. * fix(skill): rewrite negations as positive imperatives in post-approve-merger SKILL.md S1 auto-review rejection fix: 3 negation-form lines rewritten as positive imperatives per sdlc-review rules. - 'Never use sahilm-ti credentials' -> 'Use sahilm-ai credentials exclusively' - 'Do NOT call kanban_review' -> 'The ONLY success terminator is kanban_complete' - 'After it, stop - do not attempt anything else' -> 'After the single terminal call, stop' --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com> Co-authored-by: sahilm-ai <sahilm.ai@users.noreply.github.com>
…LL.md (#24) PR #23 had three negation-form directives rewritten as positive imperatives during the auto-reviewer pass, in a mistaken application of the BT-agent optimization-playbook S1 rule to a Hermes infrastructure skill. The S1/SD4 rules in sdlc-review have since been scoped BT-agent-only — but the rewrites in #23 landed before that scoping fix, so the load-bearing prohibitions are gone from the merged skill content. Restore them: 1. Auth: add explicit 'Never use sahilm-ti credentials' with the two concrete consequences (audit-trail misattribution + keychain prompt blocking the worker). The bare 'use sahilm-ai exclusively' positive form left the door open to drift, as evidenced by PR #23 itself — commit 7da3474 in that PR was authored as sahilm-ti. 2. Terminator contract: add explicit 'Do NOT call kanban_review' — without this prohibition the worker could re-loop the card through the auto-reviewer after Sahil has already approved, defeating the one-approval-equals-merged contract that motivated #23 in the first place. 3. Stop clause: 'After the single terminal call, stop — do not attempt any further gh/git/kanban_* operations.' A second terminal call corrupts the event log; the positive-only 'stop' form is too ambiguous (stop what? stop thinking? the model may interpret it as stop typing but keep tool-calling). No code changes. No test changes. Skill-content-only fix. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…OME (#25) _resolve_skill_under_home() used os.path.expanduser() which expands ~ against the calling process's HOME env var. When the kanban dispatcher runs inside a profile-sandboxed orchestrator (HOME overridden to ~/.hermes/profiles/<orch>/home/), validating another profile's skills.external_dirs config mis-expands the path and returns False even when the skill is present. Fix: - Add _real_user_home() helper using pwd.getpwuid(os.getuid()).pw_dir (unaffected by HOME env var) - Use _real_user_home() when expanding ~ in external_dirs entries - Use _real_user_home() in the None-hermes_home fallback (was Path.home() which also reads HOME) Root cause of t_da084aa4 crashing 3x with 'Unknown skill(s): kanban-worker'. The bt-optimizer / braintrusteng profiles had their external_dirs set to ~/.hermes/skills; the dispatcher running from the orchestrator's sandboxed HOME failed to find kanban-worker and aborted worker spawn. Tests: add TestResolveSkillUnderHomeCrossProfile (2 tests) — simulate cross-profile call with fake HOME, assert both _real_user_home() and _resolve_skill_under_home() return correct results. 196/196 kanban_db tests pass. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
* feat(kanban): heartbeat-aware stuck-worker detection in dispatcher Implements the stuck-worker detection described in the incident with t_1bcf9f7b (worker silent for 2h 26m while PID was alive). kanban_db.py - Add DEFAULT_STUCK_AFTER_SECONDS = 900 (15 min) constant. - Add stuck_after_seconds and no_heartbeat_required columns to the tasks table schema + idempotent migration. - Add corresponding fields to Task dataclass with from_row() fallbacks. - Add detect_stuck_workers() function: kills tasks with a live PID but a stale heartbeat (last_heartbeat_at IS NOT NULL and stale) and re- queues them as ready WITHOUT incrementing consecutive_failures. - Add stuck: list[str] to DispatchResult. - Add stuck_after_seconds_default param to dispatch_once() and wire in detect_stuck_workers() call. config.py - Add kanban.stuck_after_seconds_default: 900 to defaults. gateway/run.py - Parse kanban.stuck_after_seconds_default from config and pass it to dispatch_once(). - Add 'stuck' event notification handler in _kanban_notifier_watcher (heart emoji + silent duration + will retry). agent/prompt_builder.py - Update KANBAN_GUIDANCE heartbeat section to mention stuck detection so workers know to keep heartbeating once they start. tests/hermes_cli/test_kanban_db.py - 8 new tests covering happy path, no-heartbeat skip, recent-heartbeat skip, per-task override, no_heartbeat_required opt-out, disabled=0, dead-pid skip, and dispatch_once wiring. tests/tools/test_kanban_tools.py - Update KANBAN_GUIDANCE size bound from 4096 to 5000 chars (guidance grew legitimately with the stuck-worker note). - detect_stale_running: handles workers with no heartbeat at all after 4h (dispatch_stale_timeout_seconds). Unchanged. - release_stale_claims / detect_crashed_workers: unchanged. - detect_stuck_workers: NEW, fast (15 min default) for workers that opted into heartbeating but went silent. * fix(kanban): address stuck-detector review findings (heartbeat guidance + race condition) Finding 1 (agent/prompt_builder.py): - Fix contradictory heartbeat guidance: distinguish initial window (first heartbeat) vs steady-state (after first heartbeat) - Initial window: up to kanban.dispatch_stale_timeout_seconds (4h) before the stale-claim reaper fires - Steady state: MUST heartbeat every kanban.stuck_after_seconds_default (15min) or risk stuck-kill - Name both config keys alongside their respective rules Finding 2 (hermes_cli/kanban_db.py): - Fix stale-heartbeat-carryover on retry: source last_heartbeat_at from task_runs JOIN on current_run_id instead of tasks.last_heartbeat_at A fresh run's task_runs row has last_heartbeat_at=NULL so it is excluded from the SELECT entirely — new workers are safe until they heartbeat - Fix TOCTOU race: make the UPDATE a CAS guarded by current_run_id, worker_pid, and claim_lock so a racing reclaim makes the UPDATE a no-op (rowcount==0) instead of wiping a fresh run tests/hermes_cli/test_kanban_db.py: - Update all 8 existing detect_stuck_workers tests to also set task_runs.last_heartbeat_at (since detector now reads from there) - Add test_detect_stuck_workers_retried_task_not_killed_before_new_heartbeat: verifies a fresh run is not killed before it sends its first heartbeat - Add test_detect_stuck_workers_cas_noop_when_heartbeat_advanced: verifies CAS UPDATE is a no-op when current_run_id changes mid-flight --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…hook (#28) Part 1: Add pin_workspace_git_identity() to hermes_cli/kanban_db.py - Sets user.name/user.email in the workspace's local git config (survives subprocess shells, rebase resolution commits, auto-format follow-ups) - Installs a pre-commit hook in <workspace>/.hermes-hooks/ wired via core.hooksPath that hard-rejects commits whose GIT_AUTHOR_EMAIL does not match the configured worker identity - Opt-out: HERMES_KANBAN_ENFORCE_GIT_IDENTITY=false (env) or kanban.enforce_worker_git_identity: false (config.yaml) - Best-effort on non-git dirs: hook dir created; config write skipped Part 2: Call pin_workspace_git_identity from all workspace init paths - ensure_worktree(): called at end (both create and respawn) - dispatch_once(): called for scratch/dir workspace kinds in both the main and review dispatcher call sites Tests: 11 new tests in tests/hermes_cli/test_kanban_db.py covering worktree/scratch/dir workspace kinds, hook rejection, hook allowance, opt-out behaviour, and non-git dir noop. Also patches test_ensure_worktree_respawn_reuses_existing to opt out of identity enforcement (test is about respawn semantics, not identity). Fixes recurring C5 auto-reviewer failure where workers committed as sahilm-ti instead of sahilm-ai (PR #27 recurrence of PR #23 pattern). Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…ts (#27) * fix(kanban): extend active_pr respawn guard to honor `unblocked` events The active_pr guard in check_respawn_guard() previously only yielded when a 'rejected' event or rejected run outcome post-dated the latest PR-URL comment. This meant that when a worker opened a PR then got blocked (budget exhausted, manual kanban_block, etc.) and Sahil/ orchestrator called kanban_unblock, the guard still fired on every dispatcher tick and prevented the worker from respawning to amend the existing PR. Changes: - Query a third candidate: most-recent 'unblocked' event for the task - Merge it into the candidates list alongside reject_event/reject_run - Rename latest_reject_ts -> latest_resume_ts to reflect broader intent - Update comment to describe 'resume signal' semantics New tests (paralleling the existing rejection tests): - test_respawn_guard_active_pr_suppressed_when_unblock_postdates_pr - test_respawn_guard_active_pr_still_fires_when_pr_postdates_unblock All 20 respawn_guard tests pass. * style: black format after rebase on myfork/main * style: black format tests/hermes_cli/test_kanban_db.py * style: black format post-rebase on myfork/main (PR #28 drift) --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com> Co-authored-by: Sahil (AI) <***>
…ocs (#30) - memory_tool.py: _detect_procedural_content() with 4 heuristics (.md paths, SQL/code/shell, numbered steps, signal word near verb); wired into add() and replace() before size check; bypass_procedural_check=True param - tests/tools/test_memory_procedural_gate.py: 39 tests covering all 4 patterns and bypass flag - skills/autonomous-ai-agents/hermes-agent/SKILL.md: 'Memory - When NOT to use it' section with the 4 anti-patterns, bypass guidance, 3-layer-defence - scripts/release.py: add sahil.ai@ti.trilogy.com and 97122673+sahilm-ti@users.noreply.github.com to AUTHOR_MAP Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…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>
…es, route findings to orchestrator (#34) Today's auto-reviewer only fires on kanban_review (PR open). Cards that complete via kanban_complete skip the review pass entirely. This led to real misses: - t_b0e9a537 (investigation): shipped findings in kanban_comment instead of a Google Doc — no rule fired. - t_aa450c9d (skill edit): category completeness not verified. Option B was chosen over A because the regime semantics diverge enough (no retry, audit-only read-only pass, orchestrator-targeted verdict) that folding into sdlc-review would add noisy conditionals throughout. A separate skill keeps both reviewers readable. - New column: tasks.completion_audit_at (INTEGER, NULL = no audit pending) - Migration: _migrate_add_optional_columns adds the column + sparse index - _maybe_schedule_completion_audit: sets the flag on kanban_complete when: 1. At least one real worker run (claimed event exists) 2. No GitHub PR URL in events/comments 3. No skip-review directive in body 4. Not already scheduled (idempotent) - claim_completion_audit_task: atomically claims for audit (CAS on completion_audit_at IS NOT NULL, task stays done) - complete_completion_audit: closes audit run, emits completion_audit_done event carrying the failed_rules list for repeat-offense detection - dispatch_once: new completion-audit column dispatch loop that: - Scans done tasks with completion_audit_at IS NOT NULL - Claims, resolves workspace, spawns with skills=[sdlc-completion-audit] - Re-arms trigger on workspace/spawn failures (retry next tick) - Counts audit spawns against max_spawn - Reports in DispatchResult.audited (task_id, assignee, workspace_path) Ships separately from this PR (profiles-level skill, not bundled). Task-class classifier: investigation / exploration / skill-edit / memory-write / deliverable-doc / other (first-match keyword lookup). Per-class rule sets: INV-1…5, EXP-1…4, SKL-1…3, MEM-1…2, DOC-1…2, OTH-1. Repeat-offense detection: 3+ distinct cards failing same rule in 7 days → PATTERN ALERT prepended to the orchestrator comment. Historical smoke-test on 5 cards from the past 7 days: | Card | Class | Expected audit | Actual schedule decision | |---|---|---|---| | t_b0e9a537 | investigation | YES (no PR, no skip) | scheduled=True ✓ | | t_a83ff71d | investigation | YES (no PR, no skip) | scheduled=True ✓ | | t_3877a824 | skill-edit/PR | NO (has PR #30) | scheduled=False ✓ | | t_aa450c9d | skill-edit/PR | NO (has PR #35) | scheduled=False ✓ | | t_0bc7806c | other | YES (no PR) | scheduled=True ✓ | Lint verification on t_b0e9a537 (INV-1 expected to FAIL): - INV-1: FAIL — no google Doc URL in summary or comments - INV-4: PASS — summary has verdict (>50 chars) Lint verification on t_a83ff71d (should PASS): - INV-1: PASS — Doc URL in summary - INV-4: PASS — summary has conclusion PR flow unchanged: test_review_flow_unchanged_with_audit_present confirms review-status tasks still spawn with skills=[sdlc-review]; audited list is empty for those cards. - 18 new tests in tests/hermes_cli/test_kanban_completion_audit.py - All 18 pass; 266 total (existing kanban_db suite) pass - Acceptance criteria covered: - Schema migration (test_schema_has_completion_audit_at) - Schedule / no-schedule conditions (3 tests) - Idempotency (test_completion_audit_scheduling_idempotent) - Atomic claim + double-claim prevention (2 tests) - claim returns None when not scheduled (test_claim_...not_scheduled) - Run row created on claim (test_claim_...creates_run_row) - complete_completion_audit releases claim, emits event (2 tests) - dispatch_once dry-run, spawn, trigger-cleared, no-double-spawn (4 tests) - max_spawn budgeting (test_dispatch_completion_audit_counts_toward_max_spawn) - PR flow unchanged (2 tests) Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
Adds gateway/code_watcher.py — a background daemon thread that polls the mtime of every hermes-agent .py file imported into sys.modules. When any file's mtime is newer than the gateway process start time, the watcher logs a warning and calls os.execv to reload in-place, preserving the PID for launchd/systemd supervisors. This eliminates the stale-dispatcher-runs-old-code class of bugs: a PR merging while the gateway is running will be picked up within 60 seconds (one poll cycle) without any manual restart. Changes: - gateway/code_watcher.py: new module — CodeWatcher class, start_code_watcher() factory, helper functions - gateway/run.py: start_code_watcher() called alongside the cron thread in start_gateway(); stopped cleanly on shutdown - tests/gateway/test_code_watcher.py: 21 unit tests covering env opt-out, config opt-out, mtime detection, os.execv invocation, file filtering Config opt-outs: gateway.auto_restart_on_code_change: false (config.yaml) HERMES_GATEWAY_NO_AUTO_RESTART=1 (env, wins over config) Fixes: stale-gateway recurring pattern documented in kanban t_014159c0 Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…_procedural_gate (#35) Add TestBulletShapeBypass class with 3 tests: - test_bullet_shape_via_verb_blocked_without_bypass: confirms heuristic-4 blocks 'via' + imperative verb content without bypass flag - test_bullet_shape_via_verb_passes_with_bypass: same content lands with bypass=True - test_bypass_end_to_end_via_memory_tool_dispatcher: end-to-end dispatcher test with orchestrator-class policy fact content Regression guard for the case where bypass_procedural_check=True was being ignored for multi-sentence operational facts in orchestrator-class profiles. The code path was already correct; tests pin it against future regressions. All 59 tests in test_memory_procedural_gate.py pass. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…in (#47) The CLI auto-pull path (_cmd_update_impl) ran `reset --hard origin/<branch>` on ff-only divergence. On an inverted-topology checkout (origin=NousResearch upstream, myfork=the fork that runs locally) this discards every fork-only commit. Add hermes_cli/fork_tracking.py implementing the track-fork / merge-upstream / abort-on-conflict+alert contract, gated on a no-running-worker + clean-tree + not-mid-merge guard, and wire it into the divergence fallback. Canonical checkouts (origin=fork) keep the historical reset path. Also harden gateway/hermes_home_puller.py: it operates on the config checkout and is already ff-only (never resets), but add the same running-worker / mid-merge guard so it can never fast-forward HEAD out from under a live kanban worker. Tests: track-fork detection, merge-clean->push-to-fork, conflict->abort+ alert+no-reset, running-worker->blocked, puller worker-guard. 58 pass. Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…ision on macOS 26.5 (#48) * fix(gateway): detect launchd domain (gui vs user) so macOS 26.5 keeps supervision On a GUI (Aqua) login on macOS 26.5, LaunchAgents load in gui/<uid>, not user/<uid>. _launchd_domain() hardcoded user/<uid> (issue NousResearch#23387), so the CLI service-reload path ran `kickstart user/<uid>/<label>` -> exit 113 (not found) -> re-bootstrap into user/<uid> -> exit 5 (I/O error) -> detached fallback. The gateways then ran as plain background processes with no auto-start at login and no auto-restart on crash. Detect the session type instead of hardcoding: managername == "Aqua" -> gui/<uid>, else user/<uid> (headless/SSH). Add _launchd_domain_for_existing_job(label) which probes gui then user for an already-loaded job and targets that domain for kickstart/reload/stop/ bootout, avoiding the wrong-domain -> re-bootstrap -> exit-5 cascade. Fresh bootstraps (first install, missing-plist self-heal) use the session-type default. Verified live on macOS 26.5 (build 25F71): kickstart -k user/501/<label> returns 113 (the old failing path) while kickstart -k gui/501/<label> returns 0 and genuinely reloads the supervised job (PID changed, state=running in gui/501). Tests: new units for the domain detection + existing-job resolution; existing launchd start/reload/stop/install orchestration tests pin the domain helpers. 30 launchd tests pass. Pre-existing systemd/D-Bus-on-macOS failures untouched. * test(gateway): pin launchd domain helpers in detached-fallback path tests The two failure-path tests in TestLaunchdServiceRecovery computed their kickstart `target` from `_launchd_domain()` but production now resolves the target via `_launchd_domain_for_existing_job(label)`, which probes `launchctl print gui/<uid>/<label>` then `user/<uid>/<label>`. On CI the probe's subprocess is monkeypatched by each test's `fake_run`, whose fallthrough returns rc=0 -> the probe reports the job present in gui/<uid>, so the production target diverged from the locally computed `target`, the simulated 5/125 fault never fired, the happy path ran, and the detached spawn assertion (`assert spawned == [True]`) failed. Pin both `_launchd_domain` and `_launchd_domain_for_existing_job` to a fixed gui/<uid> domain (matching the success-path tests already in this PR) so the target is deterministic and independent of the probe. No production change. --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…) (#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>
…ot upstream, on fork-tracking checkouts (#49) On an inverted-topology checkout (origin = NousResearch upstream, myfork = the operator fork the machine deploys from), the startup banner counted HEAD..origin/main and perpetually nagged "N commits behind" even on a freshly-synced tree. The remedy hint was also wrong: the upstream CLI auto-pull path resets to upstream and discards fork-only commits. banner.py predated the fork-tracking work (PR #47) and never consumed hermes_cli.fork_tracking. This wires _check_via_local_git to it: - Resolve fork-tracking config first (read-only consumer of detect_fork_tracking). When it applies, count HEAD..{cfg.fork_ref} (myfork/main), fetching the fork remote — never origin. The banner then reads "N behind your fork" semantics: 0 on a synced checkout, non-zero only on genuine local drift behind the fork. - Else (canonical upstream-tracking install) keep today's exact behavior: count behind origin/main, suggest the upstream CLI auto-pull command. That path stays byte-identical for PyPI/container/official-remote installs. - Fork-tracking case gets fork-appropriate remedy copy instead of the upstream update hint. - _check_via_local_git now returns (behind, baseline); check_for_updates threads the baseline into the 6h .update_check cache (new "baseline" key) and a module-level getter so a fork cache is never read as an upstream cache or vice-versa, and the render picks the correct remedy. Verified on the live inverted checkout: HEAD..origin/main=5 (old, wrong), HEAD..myfork/main=0 (new, correct) -> banner shows 0 behind / no nag. Adds regression tests for the fork baseline (counts against myfork, fetches the fork, reports drift), the unchanged non-fork path, and baseline cache isolation. Updates the one expired-cache test to assert the upstream-path git calls are present rather than an exact subprocess call count (the fork-detection probe adds one leading call). Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com> Co-authored-by: Sahil Marwaha <97122673+sahilm-ti@users.noreply.github.com>
… flow (#51) * feat(kanban): add `merging` status + `skip_merge` flag to the approve flow Approving a PR-carrying card no longer silently races to `done`. A new first-class `merging` status sits between `human_review` and `done`; the post-approve merger now runs WHILE the card is in `merging` (not `running`), giving an explicit, board-visible "the worker is merging" state. A per-card `skip_merge` flag (column + `set_skip_merge()` + `--skip-merge` CLI flag + `skip_merge` tool arg) is the explicit opt-out: approving a PR-carrying card with it set goes straight to `done` with NO merger spawned. Safety net: `recover_stuck_merging()` (wired into the dispatch tick) bounces a `merging` card whose merger PID died or whose heartbeat stalled to `blocked` with a usable reason — `merging` can never silently stall. The `running`-scoped reapers are intentionally left untouched; this dedicated reaper has the correct reclaim destination (blocked, not ready, to avoid a double-merge). - VALID_STATUSES += merging; WORKER_ACTIVE_STATUSES constant - claim_merger_task: human_review -> merging (was -> running) - approve_task: honor skip_merge; PR+no-skip -> merging, PR+skip/no-PR -> done - complete_task / block_task: accept `merging` as a source status so the merger can land the card (-> done) or block on a genuine conflict - dashboard: BOARD_COLUMNS + dist status maps + CSS dot for `merging` - tests: 3 approve paths, merging->done/->blocked, recover_stuck_merging (dead pid / healthy / other-host), status-enum + column registration * fix(kanban): count `merging` workers against dispatcher concurrency caps + runtime cap Addresses the auto-review rejection on PR #51. [Major] The post-approve merger now runs in `merging`, but the three dispatcher concurrency counters (max_spawn / max_in_progress / per-profile cap) were scoped `status='running'` only — making live mergers invisible to the caps and letting the dispatcher over-spawn on top of them. The defined- but-unused WORKER_ACTIVE_STATUSES is now wired in via a single-source-of-truth SQL fragment (_WORKER_ACTIVE_STATUS_SQL = "status IN ('running','merging')") spliced into all three COUNT(*) queries. [Minor] enforce_max_runtime stays scoped to `running` (a timed-out merger must NOT go back to `ready` — that risks a double-merge), documented inline. The merger's runtime cap is instead enforced in recover_stuck_merging, which already routes to `blocked`: added a `runtime_exceeded` trigger so an alive, heartbeating-but-wedged merger past max_runtime_seconds is bounced to blocked with a clear reason. Tests: 3 new concurrency tests (merging counts toward per-profile / max_in_progress / max_spawn) + 2 new runtime-cap recovery tests (over-cap -> blocked with runtime_exceeded; within-cap -> untouched). * fix(kanban): let `merging` workers heartbeat + extend their claim heartbeat_worker and heartbeat_claim were both scoped to `status = 'running'`, so a post-approve merger running in the new `merging` status could never write last_heartbeat_at or advance its claim TTL. A merger taking >15 min (rebase / conflict / CI wait) kept its heartbeat NULL, its claim expired, and recover_stuck_merging hit the "never heartbeated and its claim expired" branch — force-blocking a still-healthy in-flight merger, the exact opposite of the durable visible `merging` state the card asked for. Widen both UPDATE WHERE clauses to the existing single-source-of-truth _WORKER_ACTIVE_STATUS_SQL fragment (status IN ('running','merging')) so the merger heartbeats through its real path (kanban_heartbeat + the tool-layer auto-heartbeat bridge). The reaper then sees a fresh heartbeat and leaves a live merger alone, while a genuinely dead/stalled merger is still recovered to `blocked`. Tests (TestMergingHeartbeat, 5 new) exercise the REAL API (not raw SQL): - heartbeat_worker succeeds in `merging` and bumps last_heartbeat_at - heartbeat_worker expected_run_id variant succeeds in `merging` - heartbeat_claim extends a `merging` claim's claim_expires - a healthy merger past its claim TTL that heartbeats is NOT blocked - a dead/never-heartbeating merger past TTL is still recovered to blocked * fix(kanban): route skip_merge tool arg through _parse_bool_arg The `skip_merge` tool arg in _handle_approve used a raw `bool(args.get("skip_merge", False))`, the lone exception among the file's boolean tool args (triage, auto_subscribe, goal_mode, include_archived all use `_parse_bool_arg`). Since `bool("false")` is truthy and there is no schema coercion before the handler, a caller sending `"skip_merge": "false"` to KEEP the merger would instead skip it and approve straight to done -- the opposite of intent, on the irreversible approve path. Switch to `_parse_bool_arg` (matching the four siblings): coerces the string forms and returns a structured tool_error for malformed values. Tests (TestApproveSkipMergeArgParsing, 4 new, exercise the real _handle_approve tool handler): - "false" string -> routes to merging, does NOT skip - "true" string -> skips merger, straight to done, no spawn - "maybe" garbage -> structured tool_error, task untouched in human_review - omitted -> default merging-with-visibility path * fix(kanban): remove the merging-status reaper — merger is sole authority to exit `merging` Follow-up to the `merging`-status work in this PR. Sahil's explicit decision: remove the reaper's authority over `merging` cards entirely. The merging worker itself is the ONLY thing that can move a card out of `merging`. - Remove `recover_stuck_merging()` and its dispatch-tick wiring. No reaper / stale-claim / runtime-cap / crash automation touches a `merging` card any more. - Drop the now-dead `DispatchResult.recovered_merging` field and the `merge_failed` outcome/event it emitted. - Update the docstrings on `heartbeat_worker`, `enforce_max_runtime`, and the dispatch tick to reflect that nothing reaps `merging` — the heartbeat path still works so a live merger CAN signal liveness, but is no longer forced to. The merger's own exit paths are unchanged and are the only ways out: `merging → done` (complete_task) and `merging → blocked` (block_task, when the merger self-blocks on an unrecoverable conflict). A merger whose process genuinely dies leaves the card stuck in `merging` until a human intervenes — an intentional tradeoff. Tests: replace the old TestRecoverStuckMerging suite (which asserted auto-bounce-to-blocked) with TestMergingNotReaped, which asserts the inverse — dead / never-heartbeating-past-TTL / over-runtime mergers are LEFT in `merging`, and that the merger can still complete or self-block. Drop the two TestMergingHeartbeat tests that drove the now-removed reaper; keep the heartbeat-path tests proving liveness signalling still works. 51 merging/cap tests + 268 kanban_db tests green; ruff clean; no new ty diagnostics. * fix(kanban): gate skip_merge write on human_review so it can't persist on a failed approve `set_skip_merge` is a general column setter (existence-gated, not status-gated). The approve flow wrote the flag BEFORE calling `approve_task`, so a `--skip-merge` approve on a card NOT in `human_review` persisted skip_merge=True even though `approve_task` then failed — leaving a stray flag with the wrong intent on the card. Fix both approve call sites (CLI `_cmd_approve` and tool-layer `_handle_approve`) to read the card status first and bail out with the existing "not in human_review" error BEFORE writing the flag. The flag now only persists when the paired approve will actually go through. `set_skip_merge` itself stays a pure setter (its hydration test is unchanged). Regression tests (tool + CLI layer): `--skip-merge` approve on a non-human_review card returns an error AND leaves skip_merge unset. Verified these fail on the pre-fix code and pass after. Addresses CodeRabbit's set_skip_merge finding on PR #51. --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
…rams into _dispatch_once_locked Rebase merge artifact: dispatch_once had these parameters but _dispatch_once_locked did not, causing NameError at runtime. Both call sites now forward all params.
1. test_kanban_guidance_prompt_size_bounded: bump ceiling 5500→6500 KANBAN_GUIDANCE grew to 5823 after upstream content additions. 2. test_bypass_true_roundtrip: remove entries-in-response assertion Upstream removed 'entries' from _success_response to prevent model thrash. The store._entries_for() assertion still covers bypass intent. 3. test_create_subscribes_gateway_session: use set_session_vars instead of monkeypatch.setenv. clear_session_vars sets ContextVars to '' (not _UNSET), blocking the os.environ fallback in get_session_env when a prior test called clear_session_vars.
When set to true in config.yaml, the active_pr respawn guard is skipped entirely — dispatcher will re-spawn ready tasks even when a GitHub PR URL appears in a recent comment. Follows the same pattern as kanban.enforce_worker_git_identity: reads kanban config at guard-check time, wrapped in try/except so import failures are silent. No change to default behaviour (false).
_launchd_domain_for_existing_job probes gui/<uid> first and fake_run was returning returncode=0 for all calls, causing the domain probe to return gui/<uid> on Aqua session runners instead of falling through to the session-type default. Fix: return returncode=1 for launchctl print probes in fake_run so the probe falls through, and patch _launchctl_session_managername to return None (headless) so _launchd_domain() consistently returns user/<uid> regardless of the CI runner's session type.
…cutor (#53) * fix(tools): support CPython 3.14 WorkerContext in DaemonThreadPoolExecutor CPython 3.14 replaced ThreadPoolExecutor's (initializer, initargs) attribute pair + 4-arg _worker free function with a WorkerContext object built via prepare_context()/_create_worker_context(). Our DaemonThreadPoolExecutor._adjust_thread_count mirrored the pre-3.14 internals directly, so on 3.14 every worker spawn raised AttributeError: 'DaemonThreadPoolExecutor' object has no attribute '_initializer' inside the worker thread, silently breaking every batch of 2+ concurrent tool calls (agent/tool_executor.py routes through this pool). Reproduced empirically on the pre-fix code against the local venv (Python 3.14.6, home = /opt/homebrew/opt/python@3.14): $ python3 -c "from tools.daemon_pool import DaemonThreadPoolExecutor; \ DaemonThreadPoolExecutor(max_workers=2).submit(lambda: 1+1).result()" AttributeError: 'DaemonThreadPoolExecutor' object has no attribute '_initializer' CI runs Python 3.13 (pyproject.toml: requires-python = ">=3.11,<3.14"), so this never surfaced there — only on interpreters that have since moved to 3.14. Fix: branch on sys.version_info at import time and mirror whichever shape of _worker/_adjust_thread_count the running interpreter actually has (confirmed against 3.14.6's concurrent.futures.thread source: the 3.14 _adjust_thread_count calls _worker(weakref, self._create_worker_context(), work_queue), the legacy shape calls _worker(weakref, work_queue, initializer, initargs)). Both branches keep the two behavioral changes that make this pool useful: daemon=True and no _threads_queues registration. Added a regression test (test_many_concurrent_submits_like_tool_executor) reproducing the tool-executor's concurrent-submit shape; verified it fails with the exact AttributeError above against the pre-fix code and passes against the fix, on both Python 3.11 (uv venv, officially supported) and 3.14.6 (local dev venv). * fix(test): pin launchd domain in restart-recovery test to survive Aqua CI runners test_launchd_restart_boots_out_stale_registration_before_bootstrap called the real _launchd_domain() helper, which probes the live session type via launchctl and returns gui/<uid> on an Aqua GUI runner instead of the user/<uid> the test hardcodes into its plist path fixture. Pin the domain the same way 9c155f3 pinned it for the refresh test: stub _launchctl_session_managername to force headless, and make the launchctl print probe (used by _launchd_domain_for_existing_job) fail so domain resolution falls through to _launchd_domain() instead of matching an already-loaded gui/<uid> job. Unrelated to the daemon-pool py3.14 fix in this branch; found while tracking down PR #53's slice-4/8 CI failure. --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
Remove the active_pr respawn guard so rejected or reclaimed PR tasks can respawn and continue on the existing PR instead of deadlocking behind comment history.
Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
(cherry picked from commit 4f30f4432576cb1708662156956c16bf650a6921) Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
* fix(kanban): restore completion audit dispatch * fix(kanban): close failed completion audit runs * test(kanban): keep audit fixtures isolated * fix(kanban): reclaim stale completion audits * fix(kanban): recover active completion audits * fix(kanban): preserve audit worker claims * test(kanban): assert audit recovery requeues --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
* fix(kanban): recover missed goal finalizers * test(kanban): cover duplicate blocker recovery --------- Co-authored-by: Sahil (AI) <266772320+sahilm-ai@users.noreply.github.com>
📝 WalkthroughWalkthroughKanban tasks now opt into gateway auto-decomposition through CLI, dashboard, and tool creation surfaces. Eligibility filters inspect task state and evidence, gateway defaults are disabled, intervention events are notified, and decomposition validates graph links against cycles. ChangesKanban auto-decomposition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TaskCreator
participant kanban_db
participant GatewayDispatcher
participant EligibilityFilter
TaskCreator->>kanban_db: create task with auto_decompose
GatewayDispatcher->>EligibilityFilter: list eligible triage IDs
EligibilityFilter->>kanban_db: inspect task and evidence
kanban_db-->>EligibilityFilter: filtered task IDs
EligibilityFilter-->>GatewayDispatcher: candidate IDs
GatewayDispatcher->>kanban_db: decompose eligible task
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hermes_cli/kanban_diagnostics.py (1)
266-314: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiagnostic ignores the per-card
auto_decomposeopt-in, so it can recommend the wrong aux slot.
triage_aux_status()derivesauto_decomposesolely from the globalkanban.auto_decomposeconfig key, and_rule_triage_aux_unavailable()uses that global flag to pick the "primary" aux slot (decomposer vs. specifier) for a specific task — but it never readstask.auto_decompose. Since eligibility for gateway auto-decompose now also requires the per-card opt-in (defaultFalse), a task that hasn't opted in will never go throughauxiliary.kanban_decomposereven whenkanban.auto_decomposeis globallyTrue; the diagnostic would still tell the operator to configure the decomposer slot instead of the specifier slot that actually matters for this card.🩹 Suggested fix
-def triage_aux_status(config: Optional[dict]) -> Optional[dict]: +def triage_aux_status(config: Optional[dict], *, task_auto_decompose: bool = False) -> Optional[dict]: ... - auto_decompose = False + global_auto_decompose = False if isinstance(kanban_cfg, dict) and "auto_decompose" in kanban_cfg: - auto_decompose = bool(kanban_cfg.get("auto_decompose")) + global_auto_decompose = bool(kanban_cfg.get("auto_decompose")) + auto_decompose = global_auto_decompose and task_auto_decompose return { "auto_decompose": auto_decompose,and pass
task_auto_decompose=bool(_task_field(task, "auto_decompose"))from_rule_triage_aux_unavailable.Also applies to: 372-481
🤖 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_diagnostics.py` around lines 266 - 314, Update triage_aux_status() to accept a task_auto_decompose override and use it when determining auto_decompose, while retaining the global kanban.auto_decompose fallback when no override is provided. In _rule_triage_aux_unavailable(), pass task_auto_decompose=bool(_task_field(task, "auto_decompose")) so the diagnostic selects the aux slot based on the specific card’s opt-in.
🤖 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 `@tests/hermes_cli/test_kanban_decompose_db.py`:
- Around line 173-175: Update the rollback assertion in the decomposition test
to preserve the committed tid-to-reserved_child link by removing the incorrect
root-status check or replacing it with an assertion that expects the committed
relationship. Keep the existing get_task assertion proving no child row was
inserted.
---
Outside diff comments:
In `@hermes_cli/kanban_diagnostics.py`:
- Around line 266-314: Update triage_aux_status() to accept a
task_auto_decompose override and use it when determining auto_decompose, while
retaining the global kanban.auto_decompose fallback when no override is
provided. In _rule_triage_aux_unavailable(), pass
task_auto_decompose=bool(_task_field(task, "auto_decompose")) so the diagnostic
selects the aux slot based on the specific card’s opt-in.
🪄 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 Plus
Run ID: 911431b5-8813-464c-9d59-573decc7b356
📒 Files selected for processing (12)
gateway/kanban_watchers.pyhermes_cli/config.pyhermes_cli/kanban.pyhermes_cli/kanban_db.pyhermes_cli/kanban_decompose.pyhermes_cli/kanban_diagnostics.pyplugins/kanban/dashboard/plugin_api.pytests/gateway/test_kanban_auto_decompose_live.pytests/hermes_cli/test_kanban_block_kinds.pytests/hermes_cli/test_kanban_decompose.pytests/hermes_cli/test_kanban_decompose_db.pytools/kanban_tools.py
Comment on lines
+173
to
+175
| # Atomic rollback: no child row inserted, root remains triage. | ||
| assert kb.get_task(conn, reserved_child) is None | ||
| assert kb.get_task(conn, tid).status == "triage" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the committed legacy edge in the rollback assertion.
Line 175 cannot be empty: Lines 159-163 commit tid -> reserved_child before decomposition. Correct rollback retains that link, so this test fails even when production behavior is correct. The preceding assertion already proves no child task row was committed.
Proposed fix
- assert kb.child_ids(conn, tid) == []
+ assert kb.child_ids(conn, tid) == [reserved_child]🤖 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_decompose_db.py` around lines 173 - 175, Update
the rollback assertion in the decomposition test to preserve the committed
tid-to-reserved_child link by removing the incorrect root-status check or
replacing it with an assertion that expects the committed relationship. Keep the
existing get_task assertion proving no child row was inserted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
scripts/run_tests.sh tests/hermes_cli/test_kanban_decompose.py tests/hermes_cli/test_kanban_decompose_db.py tests/hermes_cli/test_kanban_block_kinds.py tests/gateway/test_kanban_auto_decompose_live.py(43 passed)ruff checkon all touched Python files (passed)scripts/run_tests.shis currently running locallyKnown environment debt
npm run checkcannot complete because this worktree has missing workspace dependencies and existing unrelated web imports.Summary by CodeRabbit
New Features
auto_decomposesupport when creating and viewing Kanban tasks through the CLI, dashboard, and tools.Bug Fixes
Configuration