Skip to content
This repository was archived by the owner on Jul 22, 2026. It is now read-only.

fix(kanban): route not-clean reviews to remediation - #4

Merged
s00rz merged 9 commits into
mainfrom
codex/fix-kanban-review-routing
Jul 13, 2026
Merged

fix(kanban): route not-clean reviews to remediation#4
s00rz merged 9 commits into
mainfrom
codex/fix-kanban-review-routing

Conversation

@s00rz

@s00rz s00rz commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • route structured exact-SHA NOT CLEAN reviewer completions to one idempotent same-PR remediation owner and one dependent re-review
  • keep the original PR-bearing producer dependency-gated so active_pr cannot trap it in ready; close it only after a structured CLEAN exact-SHA verdict
  • fail safe in todo when review evidence is incomplete, coalesce unchanged respawn_guarded events, and document the contract/events

Root cause

complete_task() marked the reviewer done and immediately called recompute_ready(). For the established reviewer-as-parent graph, that promoted the original producer back to ready. A recent PR comment then made check_respawn_guard() return active_pr every dispatcher tick. The producer could not run, repeated events accumulated, and no remediation/re-review owner was created.

Validation

  • RED observed for the new routing, capacity, fail-safe, and event-coalescing regressions before implementation
  • uv run --with 'pytest>=8,<9' --with 'pytest-xdist>=3,<4' python -m pytest tests/hermes_cli/test_kanban_db.py -k 'respawn_guard or review' -q --tb=short -n 0 → 36 passed, 192 deselected
  • uvx --from 'ruff==0.15.10' ruff check hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py → passed
  • git diff --check → passed
  • Full Windows module run: 214 passed; 14 existing Windows/platform-assumption tests failed in unrelated rate-limit sentinel, path-format, argv-resolution, and zombie-reaper cases. None touch the new review-routing assertions.

Safety / rollout

  • No live board, gateway, dispatcher, or deployment code was modified.
  • No gateway restart or rollout was performed.
  • Rollback for this branch is a normal revert of commit 424b65e6773886ee0860099976136b48fb9f8ee4.

@s00rz s00rz left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Verdict: NOT CLEAN. This is a comment-only review because the authenticated GitHub account (s00rz) is also the PR author; GitHub can reject formal request-changes reviews on your own PR.

Blocking findings:

  1. hermes_cli/kanban_db.py:4318 only activates the fail-safe review router when _review_verdict(metadata) parses to CLEAN or NOT CLEAN. A PR-bearing reviewer completion with missing/unparseable structured verdict still falls through to recompute_ready(), promoting the producer to ready and re-entering the active_pr guard. I reproduced this locally at the exact PR head: completing a reviewer with summary='NOT CLEAN but no structured metadata' and metadata without verdict returned producer_status: 'ready' with no review_routing_deferred event. That leaves the original stall class open for unstructured/partial reviewer handoffs.
  2. The live GitHub PR is not currently a focused/reviewable merge candidate: gh pr view reports 1,242 changed files (+110,169/-10,933) and gh pr diff fails with HTTP 406 because the diff exceeds GitHub's 300-file limit. The local branch diff against NousResearch/main is only the intended 3 files, so the fork/base/retargeting needs repair before this PR can be reviewed or merged as the stated change.
  3. Required checks are red: Check contributors / check-attribution fails (missing gigakun@agentmail.to and hermes-agent@users.noreply.github.com mappings in the current PR comparison), which makes All required checks pass fail.

Verified during review:

  • Live PR head/base: 424b65e6773886ee0860099976136b48fb9f8ee4 into s00rz-hermes/hermes-agent:main at ed4123792c135558e7be2e486505bc569faa2a74; PR is draft, mergeable, mergeStateStatus=UNSTABLE.
  • Local exact-head diff vs upstream origin/main (4281151ae859241351ba14d8c7682dc67ff4c126) is the intended 3 files: hermes_cli/kanban_db.py, tests/hermes_cli/test_kanban_db.py, website/docs/user-guide/features/kanban.md.
  • Focused tests at exact head: uv run --with 'pytest>=8,<9' --with 'pytest-xdist>=3,<4' python -m pytest tests/hermes_cli/test_kanban_db.py -k 'respawn_guard or review' -q --tb=short -n 0 -> 36 passed, 192 deselected.
  • Static checks at exact head: uvx --from 'ruff==0.15.10' ruff check hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py and git diff --check origin/main...HEAD -- ... passed.

Inline note intended for hermes_cli/kanban_db.py:4318: the fail-safe should cover PR-bearing reviewer completions with missing/unparseable verdict metadata too, with a deterministic regression test. GitHub could not resolve an inline comment line because the PR diff is currently too large/unfocused, so this review records it here.

@s00rz

s00rz commented Jul 12, 2026

Copy link
Copy Markdown
Owner Author

Hermes Agent Review — NOT CLEAN

Exact-SHA re-review of a59fe9fbc30d4ffcc65d93b423cbbcc7e62fce6f against NousResearch/main (4281151ae859241351ba14d8c7682dc67ff4c126).

Blocking finding:

  • hermes_cli/kanban_db.py:4061 / hermes_cli/kanban_db.py:8377_finalize_clean_review() auto-closes the PR-bearing producer by updating the task row and appending events, but it does not persist a completed run summary/metadata or task.result for that producer. Downstream worker context prefers completed runs and falls back to task.result, so a child promoted after a CLEAN review receives (no result recorded) for its done parent and loses the exact-SHA review evidence/handoff.

Ad-hoc reproduction on this exact head:

review_complete True
producer_status done
child_status ready
## Parent task results
### <producer> (completed just now)
(no result recorded)

Green checks/evidence re-verified before this finding:

  • Live PR is still fix(kanban): route not-clean reviews to remediation #4 on codex/fix-kanban-review-routing, head a59fe9fbc30d4ffcc65d93b423cbbcc7e62fce6f; no upstream companion PR for the same head branch.
  • GitHub reports mergeStateStatus=CLEAN; gh pr checks shows All required checks pass and no failed/pending checks.
  • Focused local verification passed: 37 passed, 192 deselected; ruff passed; git diff --check origin/main...a59fe9fbc30d4ffcc65d93b423cbbcc7e62fce6f passed.

Suggested remediation: when _finalize_clean_review() closes the producer, persist a synthetic completed run (or otherwise set durable task.result) carrying the CLEAN exact-SHA summary/metadata and attach its run_id to the review/completed events. Add a regression asserting a downstream child’s build_worker_context() includes the exact-SHA CLEAN handoff instead of (no result recorded).

Leaving the PR draft/unmerged pending same-PR remediation.

@s00rz s00rz left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Verdict: NOT CLEAN for exact head c7ad3ec0d8342b72594a07e22c389c54e6b3a305.

The CLEAN auto-close handoff remediation works, and GitHub CI is green, but the PR still has one blocking fail-safe gap in the same review-routing contract:

Blocking

  • hermes_cli/kanban_db.py:4406-4436review_routing_deferred is not durable across the next dispatcher tick. When a PR-bearing reviewer completes with incomplete/mismatched evidence, complete_task() skips its immediate recompute_ready() call and leaves the producer in todo. However, the next normal dispatcher tick calls global recompute_ready() unconditionally. Because no unfinished parent or other durable gate was added, the producer is promoted to ready and can fall back into the active_pr guard/event storm the PR is meant to prevent.

    Reproduction on this exact SHA with an isolated temp HERMES_HOME and Kanban env unset:

    {'complete_ok': True, 'status_after_complete': 'todo', 'deferred_events': 1, 'status_after_dispatch': 'ready', 'respawn_guarded': []}
    BUG: deferred review producer promoted after dispatcher recompute
    

    The existing focused suite passes (38 passed, 192 deselected) but only checks the immediate post-completion status, not the following dispatcher recompute. Please add a regression that completes a PR-bearing review with missing exact SHA, runs dispatch_once() (or recompute_ready()), and asserts the producer remains durably gated rather than becoming ready. The fix should preserve a durable gate (for example an unfinished parent, explicit blocked/deferred state honored by recompute, or equivalent) until structured exact-SHA evidence is supplied or a remediation/re-review owner is created.

Verified

  • PR #4 is still the same open draft PR on codex/fix-kanban-review-routing with head c7ad3ec0d8342b72594a07e22c389c54e6b3a305.
  • No upstream companion PR exists for s00rz-hermes:codex/fix-kanban-review-routing.
  • Compared exact SHA against NousResearch/main; changed files remain hermes_cli/kanban_db.py, scripts/release.py, tests/hermes_cli/test_kanban_db.py, and website/docs/user-guide/features/kanban.md.
  • gh pr checks reports the aggregate required check green; skipped jobs appear non-blocking for this diff.

Because this is the reviewer account's own PR, I am submitting this as a COMMENT review rather than REQUEST_CHANGES.

@s00rz s00rz left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review\n\nVerdict: CLEAN for exact replacement head .\n\nVerified:\n- PR #4 remains the single open fork PR for branch ; no companion PR exists in .\n- Base is at ; the exact-SHA diff remains focused to , , , and .\n- The deferred/missing review-evidence gate is durable across /: the producer remains , is not spawned, and does not re-enter the guard.\n- The prior CLEAN exact-SHA handoff remains intact: the synthetic completed producer run preserves the exact reviewed SHA and downstream context does not contain .\n- Local validation at this exact head: focused suite ; Ruff 0.15.10 passed; passed.\n- GitHub required checks pass; only non-blocking affected-area skips remain.\n\nThis is a comment-only clean review because the authenticated GitHub account owns the PR, so GitHub cannot accept an approval/request-changes review from this account. No upstream publication or live rollout/restart was performed.

@s00rz s00rz left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Verdict: CLEAN for exact replacement head 59caccd.

Verified:

  • PR #4 remains the single open fork PR for branch codex/fix-kanban-review-routing; no companion PR exists in NousResearch/hermes-agent.
  • Base is main at 4281151; the exact-SHA diff remains focused to hermes_cli/kanban_db.py, scripts/release.py, tests/hermes_cli/test_kanban_db.py, and website/docs/user-guide/features/kanban.md.
  • The deferred/missing review-evidence gate is durable across dispatch_once and recompute_ready: the producer remains todo, is not spawned, and does not re-enter the active_pr guard.
  • The prior CLEAN exact-SHA handoff remains intact: the synthetic completed producer run preserves the exact reviewed SHA and downstream context does not contain (no result recorded).
  • Local validation at this exact head: focused suite 39 passed, 192 deselected; Ruff 0.15.10 passed; git diff --check passed.
  • GitHub required checks pass; only non-blocking affected-area skips remain.

This is a comment-only clean review because the authenticated GitHub account owns the PR, so GitHub cannot accept an approval/request-changes review from this account. No upstream publication or live rollout/restart was performed.

@s00rz
s00rz marked this pull request as ready for review July 12, 2026 06:28
@s00rz

s00rz commented Jul 12, 2026

Copy link
Copy Markdown
Owner Author

VERDICT: NOT CLEAN
HEAD: 099a66f
REVIEWER: reviewer/t_bb25fd00

Blocking finding:

  • hermes_cli/kanban_db.py:4185-4205 accepts any syntactically valid 40-character reviewed_sha and never checks a re-review's SHA against the remediation parent's persisted replacement_sha. A stale or mistyped CLEAN re-review therefore closes the original producer as done, even though the re-review card explicitly requires reviewed_sha == replacement_sha and the docs say mismatched review evidence fails safe. Independent reproduction on this exact head completed remediation at bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, then completed the dependent CLEAN re-review at cccccccccccccccccccccccccccccccccccccccc; completion returned true, the producer became done, and no review_routing_deferred event was emitted. Validate the re-review metadata against its completed remediation parent before routing; on mismatch, retain the durable deferred gate. Add a regression for mismatched reviewed_sha/replacement_sha.

Verified at this exact head:

  • Server-side diff is exactly the intended four files.
  • PR head/base remain 099a66fc70449a69be22e3b1b8ecc609e705768b / 57796864dacd553f29e579f075ec2e573e1cf8fe; merge state is CLEAN and all non-skipped checks are successful.
  • Focused tests: 39 passed, 192 deselected. Ruff 0.15.10 and git diff --check passed.
  • The replacement merge commit did not reintroduce reverted PR feat(kanban): coordinate draft PR review lanes #2 behavior: every path outside the four-file PR diff is tree-identical to base main, including the reverted prompt/worker-guidance paths.

@s00rz

s00rz commented Jul 12, 2026

Copy link
Copy Markdown
Owner Author

VERDICT: NOT CLEAN
HEAD: 72fc7e0
REVIEWER: reviewer/t_93d51427

Blocking finding:

  • hermes_cli/kanban_db.py:4123-4134 identifies a dependent re-review only from remediation parents already in status done. If that parent is missing/not yet done, the empty set returns binding_required=False, so the re-review is treated as an ordinary first-pass review. Because complete_task supports manual blocked-to-done completion, a CLEAN re-review can then close the producer at an arbitrary full SHA with no review_routing_deferred event while remediation is unfinished. Detect the router-generated re-review/all remediation parent links before filtering by done; an absent/incomplete parent or absent handoff must return (True, None), and add regression coverage for that fail-closed path.

Checks: exact server head and four-file scope verified; 40 focused review/respawn tests passed (192 deselected); Ruff 0.15.10 and git diff --check passed; all required GitHub checks pass. The mismatched and normal matching-SHA paths are covered, and reverted PR #2 worker-lane files were not reintroduced.

@s00rz

s00rz commented Jul 12, 2026

Copy link
Copy Markdown
Owner Author

VERDICT: NOT CLEAN
HEAD: eaa357a
REVIEWER: reviewer/t_30763981

Blocking finding:

  • hermes_cli/kanban_db.py:3293 makes the supposedly durable review_routing_deferred gate depend on the review parent still having status = 'done'. archive_task() is a supported transition for completed tasks and immediately runs dependency recomputation; archived parents are normally treated as satisfied. Archiving a deferred reviewer therefore makes _has_deferred_review_routing_gate() return false and promotes the PR-bearing producer from todo to ready, recreating the active_pr respawn path without any exact-SHA route. Independent exact-head reproduction returned before_archive='todo' and after_archive='ready' with a new promoted event. Preserve deferred routing across archival (for example, recognize done and archived review parents, or persist an explicit producer-side gate), and add a regression that archives a deferred reviewer and asserts the producer remains inert.

Exact-head evidence:

  • Live PR head/base rechecked as eaa357af55b3613c5f1987fca220a70badbb64f6 / 57796864dacd553f29e579f075ec2e573e1cf8fe; GitHub reports OPEN, non-draft, CLEAN/MERGEABLE.
  • Full diff reviewed: four files, 933 insertions / 11 deletions. The new unfinished-remediation identity/binding fix is correct for the reported manual-completion case, including all-parent inspection and exact replacement-SHA matching.
  • CI run 29213553122 is success for this exact head; aggregate All required checks pass is green and affected-area skips are non-blocking.
  • Local focused verification: 41 passed, 192 deselected; Ruff 0.15.10 passed; git diff --check origin/main...HEAD passed.
  • Reverted PR feat(kanban): coordinate draft PR review lanes #2 behavior remains absent: agent/prompt_builder.py is tree-identical to base, tests/agent/test_kanban_pr_guidance.py remains absent, and the only overlap with the rollback is attribution-only additions in scripts/release.py.

This is a comment verdict because the authenticated account owns the PR. The PR must remain unmerged pending same-PR remediation and a fresh exact-head independent re-review.

@s00rz

s00rz commented Jul 13, 2026

Copy link
Copy Markdown
Owner Author

VERDICT: CLEAN
HEAD: 61759dd
REVIEWER: reviewer/t_9265c115

Exact-head evidence:

  • Live PR head was rechecked against the remediation handoff replacement SHA before review and remained exact.
  • Full four-file PR diff and complete prior review/comment history were inspected; no blocking correctness, security, regression, or test-sufficiency issue remains.
  • Archive-path regression passed: deferred review evidence keeps the PR producer in todo after reviewer archival, archive-triggered recomputation, and a later dispatcher recomputation; it is neither spawned nor active_pr guarded.
  • Matching replacement-SHA routing, mismatched replacement-SHA fail-closed behavior, and unfinished-remediation manual-completion fail-closed behavior passed.
  • Focused review/respawn suite: 42 passed, 192 deselected. Targeted contract regressions: 4 passed. Ruff 0.15.10 and git diff --check passed.
  • GitHub Actions run 29214193367 is green for this exact head, including all 8 Python slices, blocking lints, security scans, docs checks, and All required checks pass.
  • Reverted PR feat(kanban): coordinate draft PR review lanes #2 prompt/worker-guidance behavior remains absent: agent/prompt_builder.py is tree-identical to the PR base, tests/agent/test_kanban_pr_guidance.py is absent from both, and no prompt/worker-guidance path is in this PR diff.

This is a comment verdict because the authenticated GitHub account owns the PR. PR #4 is non-draft, CLEAN/MERGEABLE, and no companion PR was created.

@s00rz
s00rz merged commit 4d8ed4d into main Jul 13, 2026
30 checks passed
@s00rz
s00rz deleted the codex/fix-kanban-review-routing branch July 13, 2026 00:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants