fix(deploy): reconcile cwest/integration with upstream main so merged work reaches the running gateway by cwest · Pull Request #126 · cwest/hermes-agent

cwest · 2026-08-17T18:00:47Z

Why

The running install runs cwest/integration, ~110 commits ahead of and 3
behind origin/main, so merged upstream work does not reach the running
gateway. Concretely, the sanctioned running->review handoff verb
(submit_for_review, merged to main in f1c288c) was inert here, so every
worker still needed a manual lane move.

Strategy chosen: MERGE (recommended), not rebase

Reconciled with a merge commit (git merge origin/main), not a rebase.

Trade-off:

Merge (chosen): no history rewrite, so every attached worktree keeps its
merge-base and the protected integration branch needs no force-push. ~12
worktrees are attached to this branch; a rebase would orphan all their
merge-bases and force a protected-branch force-push — high blast radius for a
routine catch-up. The cost is a non-linear history, which is fine for an
integration branch.
Rebase (rejected): linear history but rewrites every hash; reserve it for
a deliberate low-worktree history cleanup, not a per-release reconcile.
Upstream the fork commits as PRs (the long game): the durable fix for
recurring divergence is to shrink the fork so the delta trends to zero — a
separate ongoing effort, not a blocker for this reconcile.

The procedure is written down in docs/reconciling-fork-with-upstream-main.md
so the next release is mechanical.

What landed

Merge origin/main. Brings in submit_for_review, the wedged-worker
reclaim/claim-release fix, and (transiently) the agent:end hook change.
Conflicts in hermes_cli/kanban_db.py and tests/tools/test_kanban_tools.py
resolved as union merges (kept both the fork-only PR-URL dedup helpers and
the incoming review-owner resolver; dropped the incoming duplicate
_OWNER_MAP_RE; unioned the expected kanban tool set).
Re-held the agent:end hook change. The integration tip was a signed
revert of that change (deliberate hold on the running install). A plain merge
silently resurrected it, so it is re-reverted on top of the merge. Net effect
on the running gateway: the hook stays record-only, unchanged.
Completed the owner-map reader reconciliation. The incoming naive
review-owner scan collided with this fork's create_task auto-stamp
(owner-map birth guarantee): it read the kind_source=defaulted stamp and
ignored both a later intentional stamp and the caller default.
resolve_review_owner now delegates to the fork's existing, auto-stamp-aware
reader; the orphaned naive parser the merge dragged in was removed. Fixed a
same-name test-helper collision and stale branch-name assertions surfaced by
the merge (these were already red on the running install).

Verification

grep -c "def submit_for_review" hermes_cli/kanban_db.py -> 1
git rev-list --count HEAD..origin/main -> 0
submit_for_review suite: 14/14 green; negative control witnessed RED
under a defect planted by line number inside submit_for_review and confirmed
via inspect.getsource on the loaded function.
Kanban suites (test_kanban_db.py, test_kanban_cli.py,
test_kanban_tools.py): 522 passed, 0 failed.
Gateway hook suites (test_hooks.py, test_session_boundary_hooks.py):
28 passed, 0 failed.
Pyflakes undefined-name sweep on every changed source file: clean.
Imports smoke-tested under BOTH the 3.11 (gateway) and 3.12 (test) venvs.
All 4 stashes and all attached worktrees enumerated identical before/after;
the live install (~/.hermes/hermes-agent) was never touched.

Requires a gateway restart (Casey's)

The reconciled .py modules load at gateway startup and do NOT hot-reload. A
running gateway keeps executing the pre-merge code — including the still-inert
submit_for_review — until the gateway is restarted. Merge alone does not make
this take effect.

The agent:end hook fired via emit(), which discards handler return values, and passed only the first 500 chars of the reply. A handler could record a violation but never stop the reply, and a violation buried past char 500 was structurally invisible. Mirror the proven command:* decision protocol: - Add response_full to the agent:end context (untruncated) alongside the existing response field (kept capped at 500 for backward compatibility). - Dispatch agent:end via emit_collect() so handler decisions are honored. - decision=deny suppresses the reply and surfaces the handler message back into the loop; decision=rewrite substitutes the reply; anything else, None, or a non-dict is a no-op so record-only handlers are unaffected. - Wrap the whole dispatch so a handler that raises, times out, or returns garbage falls through to sending the reply unchanged. A broken predicate can never silence the agent. Decision handling and context building are extracted into two module-level helpers so the deny/rewrite/no-op/wedge-safety paths are unit-tested.

… on lane-exit (#119) A worker's claim could gate the next lane for up to a full hour, starving the review column: the author had pushed and the card had already been MOVED out of its lane, yet the author's claim still blocked the next worker from spawning. Two independent defects, two fixes. 1. Heartbeat-staleness threshold was 60m of pure slack. release_stale_claims reclaims a live-PID worker whose last_heartbeat_at is older than DEFAULT_CLAIM_HEARTBEAT_MAX_STALE_SECONDS, but at 60m a worker that stopped heartbeating 14 minutes ago was still "fresh", so its claim was extended instead of reclaimed. The 60m was legacy slack from before the chunk-level activity bridge: _touch_activity now refreshes last_heartbeat_at at the start of every API call and on every stream delta (rate-limited to 60s), so a genuinely active worker — including one inside a single long tool-free LLM call — is never more than ~60s stale from ordinary traffic. Lower the threshold to 5 min (5x the bridge cadence): a wedged worker is reclaimed in minutes, while a healthy-but-slow worker's fresh heartbeat still extends its claim. 2. A claim leaked onto a non-running lane was never released. Every legitimate claim path sets claim_lock in the same transaction as status='running', so a claim on a card that is NOT running can only be one that leaked across a lane transition (e.g. a running->review MOVE that updated status/assignee but left the prior worker's claim_lock/expires/worker_pid on the row). The review-spawn path requires claim_lock IS NULL, so that dangling claim starved the lane, and neither the TTL scan nor the crashed-worker scan could see it (both scan status='running' only). release_stale_claims now clears any claim on a non-running card, in its current lane, with no TTL wait and no PID check — restoring the invariant "claim state belongs only to running cards" that the dashboard status-set path already enforces. Tests: reproduce the exact 14-minute wedge (reclaimed, not extended); a negative control proving a fresh-heartbeat expired-TTL worker is still extended (guards the spawn-then-reclaim regression); and the lane-exit leak (a review card carrying a departed worker's claim is freed so the review-spawn predicate matches).

Worktree branches for cards without a project link fell through to a bare wt/<task-id>, producing opaque refs like t-39521e0e that are unreadable in a branch list or preview dashboard. Meaningful naming already existed but was gated behind a project link, so the opaque form was the default for most cards rather than a rare fallback. Derive the name from the card title on every path, matching the slug rules already used for project-linked cards. The bare id now appears only when a title is absent or slugs away to nothing. Both worktree provisioning call sites are covered; a single-site fix would have left the second path emitting the old shape.

…author A worker signalling dependency_wait to hand off finished work was parked in todo, which the recompute_ready sweep promoted to ready, which made the dispatcher respawn the author on already-complete work. One card cycled through that loop three times before an operator broke it by hand. Dependency waits whose reason names a review or signoff now land in review.

…k it recompute_ready promotes any blocked card whose parents are all done, which is vacuously true for a parentless card. _has_sticky_block was the guard against that, but it only treated onecard:move_card as a decisive park — acceptance emits onecard:accept_card, so an accepted card was unparked seconds after the PASS and the reviewer re-parked it in a loop. Recognize both one-card verbs as decisive.

…ly (#114)" This reverts commit 0136ddc032877cb0b8e1afcefee844b3d18c0f0e.

#124) * ✨ feat(kanban): give workers a sanctioned running->review handoff verb A worker that finishes its lane had no sanctioned way to move its own card to review. kanban_complete is wrong at a lane boundary — it means the work item is finished (done == merged/accepted), which is premature — so a completed rework would park in blocked until an orchestrator hand-moved it. Add a non-terminal handoff that MOVEs the card running/ready -> review and assigns the reviewer from the card's state_owners owner map: - kanban_db.submit_for_review(): atomic guarded UPDATE (status IN running/ready), clears the claim lock so the review dispatch's claim_review_task can pick it up, ends the worker run with a non-terminal handed_off outcome, and emits status_changed/assigned events. Its only status target is a literal 'review' — there is no code path to done, and it never touches the PR (no undraft, no merge). - kanban_db.resolve_review_owner(): reads state_owners["review"] from the card's audit trail (code -> lamport, writing -> perkins), falling back to the code reviewer for un-stamped cards. - kanban_submit_for_review worker tool + the `hermes kanban review` CLI verb, both resolving the reviewer from the owner map with an optional explicit override. The review-lane dispatch, acceptance gate, and PR webhook are unchanged; the dispatcher already spawns the review agent for status='review' cards, so a handed-off card flows straight into review with no human. * 🧪 test(kanban): make the handoff negative control behavioral, not source-read The submit_for_review negative-control test asserted on inspect.getsource() to prove the string 'done' never appears — a source-text assertion that false-fails on a harmless rename/comment and false-passes if a path to 'done' is reached via a helper. Replace it with a behavioral guard: call submit_for_review on a card in every non-handoffable status (done, review, blocked, triage, todo, scheduled, archived) and assert the call returns False with status AND assignee unchanged, plus a positive half asserting the only produced status is 'review'. Verified as a real guard by mutation: widening the SQL WHERE to admit 'done' makes the [done] case fail. Also document the first-match (not last-write) owner-map resolution in resolve_review_owner: the map is stamped once at submit and not re-negotiated per lane, so the earliest parseable map is authoritative. * 🧪 test(kanban): prove the handoff status guard is the sole gate under test The negative control for submit_for_review asserted that a settled card in any non-handoffable status cannot be dragged to review, but did not pin down *why* the call is refused. Make the fixture self-evidently a settled card: assert claim_lock/claim_expires/worker_pid/current_run_id are all NULL before the call, so the SQL status clause is provably the only thing standing between the call and a successful write. Now a mutation that widens the guard to admit done/blocked/review turns exactly those parametrizations RED — the control measures the status guard, not some incidental precondition. Verified: plant the widened WHERE -> [done]/[review]/[blocked] + terminal-card case go RED; revert -> all green. 425 changed-file tests pass; ruff clean.

Reconcile fork integration-branch divergence (110 ahead, 3 behind) so merged upstream work reaches the running install. Brings in the running->review handoff verb (submit_for_review), the wedged-worker reclaim/claim-release fix, and the agent:end hook decision path. Conflicts resolved as union merges in hermes_cli/kanban_db.py and tests/tools/test_kanban_tools.py: - kanban_db.py: kept the fork-only PR-URL dedup helpers (_canonical_pr_url / _review_pr_url) AND the incoming review-owner resolution (DEFAULT_REVIEW_OWNER / _parse_owner_map / resolve_review_owner, which submit_for_review depends on). Dropped the incoming duplicate _OWNER_MAP_RE assignment: the fork already defines it verbatim earlier in the module, so _parse_owner_map reuses that single definition. - test_kanban_tools.py: expected kanban tool set unions both the fork-only kanban_reassign_origin and the incoming kanban_submit_for_review. The incoming agent:end hook change is re-reverted in the following commit to preserve the deliberate hold on the running install.

The merge of origin/main re-introduced the agent:end block/rewrite hook change, which was deliberately held out of the running install by a signed revert (eccbae1, 2026-08-16). Re-apply that hold so merging upstream does not silently resurrect code that was intentionally kept out of the running gateway. This reverts the agent:end decision-path change on this branch only; it does not affect the same change on origin/main. Net effect on the running install: unchanged (the hook stays record-only), while the rest of merged main — the running->review handoff verb and the worker reclaim fix — lands normally. Reverts the content of b7514ce; tree-identical to the prior hold.

The merge of origin/main brought in a review-owner resolver that assumed no owner map is stamped until submit. This fork stamps a kind_source=defaulted owner map at the create_task chokepoint (the owner-map birth guarantee), so the incoming naive first-match scan read that defaulted stamp and (a) ignored a later intentional stamp and (b) never honored the caller's default for a map-less card. Reconcile to the fork's existing, auto-stamp-aware reader: - resolve_review_owner now delegates to _review_owner_from_owner_map, so an intentional submit stamp (or prose Routing (owner map): {…}) wins over the defaulted chokepoint stamp, matching every other owner-map reader in this module. Removed the orphaned naive _parse_owner_map the merge introduced (its only caller was the old resolver; the fork's _lane_owner_from_map_body already does the parse). - Renamed the incoming test helper _stamp_owner_map -> _stamp_owner_map_str to stop it colliding with the fork's kwargs-based _stamp_owner_map of the same name (Python kept only the last def, breaking the incoming positional callers with a TypeError). - Updated the fallback test to the fork's contract: a plain code card resolves to its defaulted-code reviewer; the caller default applies only to a genuinely map-less card. - Corrected three stale wt/<id> branch-name assertions to the title-derived wt/<id>-<slug> form the fork already ships. Also documents the merge-based reconciliation procedure in docs/reconciling-fork-with-upstream-main.md so the next release is mechanical.

cwest

The reconciliation itself is sound. The merge makes all of origin/main reachable (HEAD..origin/main is 0), submit_for_review lands (grep count 1), the #114 agent:end decision path is correctly re-reverted with its now-dead test file removed, and the owner-map collision is resolved the right way — resolve_review_owner delegates to _review_owner_from_owner_map and the orphaned naive _parse_owner_map is deleted rather than left as dead code. I confirmed the submit_for_review suite at 14/14 and witnessed the guard go red under a planted 'review'->'running' defect on the loaded function, so that coverage is real, not vacuous. The doc reads as a usable per-release runbook.

One thing blocks it: a stale assertion left behind by the branch-naming change.

tests/hermes_cli/test_kanban_worktree_isolation.py:143 still asserts the old bare fallback name:

assert branch == f"wt/{tid}"

But _derive_worktree_branch_name now slugs the title into the fallback, and _resolve_worktree_workspace uses it, so for this test's card (title="second sibling") the branch is wt/<tid>-second-sibling. This turns CI slice 4/8 red:

tests/hermes_cli/test_kanban_worktree_isolation.py::test_resolve_worktree_falls_back_when_path_occupied
assert branch == f"wt/{tid}"
AssertionError: assert 'wt/t_95da5ffa-second-sibling' == 'wt/t_95da5ffa'

The three matching assertions in test_kanban_db.py were updated to the -ship slug form; this fourth one, in a file the diff doesn't otherwise touch, was missed. It passes on the base branch and fails at this head, so it's a new regression, not pre-existing. The fix is the same shape as the others — expect f"wt/{tid}-second-sibling". Worth a grep for any other surviving wt/{tid} / wt/{task.id} assertions so the sweep is complete.

Two other red signals that are not on you: the OSV Scan lockfiles check failed on the SARIF upload step ("No server is currently available") during today's GitHub outage — the scan itself ran clean, so it just needs a re-run. The eight test_kanban_tools.py report-back-subscription failures reproduce identically on the base branch and are environment/config-sensitive, not introduced here.

Fix the assertion, get slice 4/8 green, and this is ready.

The merge-reconciliation commit corrected three stale wt/<id> assertions in test_kanban_db.py but missed a fourth, in a file the merge diff did not otherwise touch. _resolve_worktree_workspace now derives the fallback branch name from the card title (wt/<id>-<slug>), so the occupied-path fallback case for a 'second sibling' card yields wt/<id>-second-sibling, not the bare wt/<id>. Update the assertion to match; the same-branch reuse case on line 172 correctly keeps the bare wt/<id> (it returns the actual pre-existing checkout's branch, not a freshly derived name).

cwest

No changes needed. The one thing flagged last round is fixed and the fix holds up.

The occupied-path fallback in _resolve_worktree_workspace now derives its branch name from the card title, so a "second sibling" card yields wt/<id>-second-sibling rather than the bare wt/<id>. The assertion at test_kanban_worktree_isolation.py:143 was updated to match, and it's correct: I reverted it back to the bare form locally and watched the test fail with wt/<id>-second-sibling != wt/<id>, then restored it and confirmed green — so the assertion genuinely guards the derived-branch behavior rather than just tracking a rename.

Swept the rest of the tree for surviving bare-wt/<id> branch assertions. The two that remain are legitimate, not misses: the same-branch-reuse case (line 172) and the foreign-branch checkout (line 198) both return the pre-existing checkout's actual branch, which is bare by construction; the git worktree list substring checks in test_kanban_db.py match on a prefix. Nothing else stale.

Full suite is green at the head commit — the previously-red Python slice is now passing, everything else success or skipped, no unresolved threads, merge state clean.

cwest added 10 commits August 4, 2026 10:23

Revert "🐛 fix(gateway): let the agent:end hook block or rewrite a rep…

eccbae1

…ly (#114)" This reverts commit 0136ddc032877cb0b8e1afcefee844b3d18c0f0e.

cwest commented Aug 17, 2026

View reviewed changes

cwest marked this pull request as ready for review August 17, 2026 18:32

cwest commented Aug 17, 2026

View reviewed changes

cwest merged commit eaf977a into cwest/integration Aug 17, 2026
37 checks passed

cwest deleted the wt/t_a27435e7-fix-deploy-l-reconcile-cwest-integration branch August 17, 2026 20:04

Navigation Menu

fix(deploy): reconcile cwest/integration with upstream main so merged work reaches the running gateway - #126

fix(deploy): reconcile cwest/integration with upstream main so merged work reaches the running gateway#126
cwest merged 11 commits into
cwest/integrationfrom
wt/t_a27435e7-fix-deploy-l-reconcile-cwest-integration

cwest commented Aug 17, 2026

Uh oh!

cwest left a comment

Uh oh!

cwest left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cwest commented Aug 17, 2026

Why

Strategy chosen: MERGE (recommended), not rebase

What landed

Verification

Requires a gateway restart (Casey's)

Uh oh!

cwest left a comment

Choose a reason for hiding this comment

Uh oh!

cwest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant