Skip to content

🐛 fix(kanban): refuse complete_task -> done when a required PR artifact is absent - #57

Merged
cwest merged 1 commit into
cwest/integrationfrom
wt/t_b8c91b61
Jul 12, 2026
Merged

🐛 fix(kanban): refuse complete_task -> done when a required PR artifact is absent#57
cwest merged 1 commit into
cwest/integrationfrom
wt/t_b8c91b61

Conversation

@cwest

@cwest cwest commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Why

done on the board means exactly one thing: the declared reviewable artifact exists. A worker can build its deliverable in a git worktree but exit WITHOUT committing / pushing / opening a PR — and its kanban_complete still flips the card to done. This happened live (2026-07-08): a research card wrote three files into its worktree, opened no PR, completed anyway, and its gated children auto-promoted onto a foundation that did not exist (one began running before it was caught). A done card with no PR is a false state.

complete_task in hermes_cli/kanban_db.py already guards two false-done classes before its write txn (both a clean no-op refusal + an auditable event):

  • phantom created_cardscompletion_blocked_hallucination (raises)
  • acceptance-lane park (awaiting-casey-signoff) → completion_refused_acceptance (returns False)

There was no guard for "the card's own contract required a PR/committed artifact and none exists." This adds that third guard in the same shape.

What

A required-artifact completion guard in complete_task, slotted in beside the acceptance-lane guard (before the main with write_txn(conn):), with the same contract: a rejected completion never mutates task state, returns False, and emits an auditable completion_refused_missing_pr event.

1. Opt-in per card, derived from the workspace (zero schema footprint). A new _card_requires_pr(workspace_kind, workspace_path) returns True only for workspace_kind == 'worktree' — the sole kind that materializes an isolated linked git worktree on a branch (via _ensure_git_worktree; branch_name is worktree-only), i.e. precisely the implementer→PR shape — whose workspace_path is NOT anchored under any ~/.hermes tree (both get_default_hermes_root() and $HOME/.hermes are excluded, so edit-in-place config / live-install worktrees stay completable). scratch (throwaway tmp dir) and dir (plain shared directory — persistent build dirs, edit-in-place config trees) are NOT PR-requiring and complete exactly as before. No requires_pr column, no migration, no backfill: existing cards classify correctly the moment this ships.

Design note: the card offered workspace_kind in ('worktree','dir') as a candidate signal. dir was rejected after ground-truth analysis — a dir workspace is a plain shared directory with legitimate non-PR uses (e.g. a persistent build dir; the test_dir_child_completion_unblocks_deferred_scratch_parent regression exercises exactly this), whereas worktree is definitionally a git worktree cut for a branch/PR. Narrowing to worktree is the precise, more-conservative signal and keeps every edit-in-place card completable.

2. The artifact check reuses the existing PR→card linkage. _card_has_pr_artifact(conn, task_id) scans task comments for a resolvable pull/<n> URL — the ready-for-review handoff the implementer lane posts on PR open — matched by the same _RESPAWN_GUARD_PR_URL_RE the active_pr respawn guard and _lane_work_provably_done already trust as durable proof of a landed lane. Extend, don't duplicate.

3. The merge path bypasses the guard. Casey's merge (allow_acceptance_complete=True, the github-pr-closed webhook) skips this guard exactly as it skips the acceptance guard.

4. Auditable refusal. Emits completion_refused_missing_pr with a summary_preview, mirroring completion_refused_acceptance.

Whole-class coverage. The guard lives inside complete_task — the single chokepoint every completion path calls (worker kanban_complete, hermes kanban complete, the swarm root helper, and the dashboard plugin_api paths, none of which pass the merge override) — so all paths are covered by construction, not site-by-site.

Tests

tests/hermes_cli/test_kanban_complete_missing_pr_guard.py — 7 behavior-contract cases against a real temp kanban DB (no mocks of the unit under test):

  • worktree card, no PR → refused (returns False, stays running, no completed_at)
  • refusal emits completion_refused_missing_pr with the summary_preview, and no completed event
  • dir card, no PR → completes (not PR-requiring)
  • worktree card WITH a pull/<n> comment → completes
  • scratch card, no PR → completes
  • worktree card under ~/.hermes → completes (path carve-out)
  • merge override (allow_acceptance_complete=True) → completes with no PR

Verification:

scripts/run_tests.sh tests/hermes_cli/test_kanban_complete_missing_pr_guard.py tests/hermes_cli/test_kanban_db.py -q
=== Summary: 2 files, 271 tests passed, 0 failed ===

Full kanban surface green with zero attributable regressions (the 13 failures across the broader tests/hermes_cli/ run are pre-existing environment flakes — systemd/WSL/qwen-oauth/SIGTERM-timing — confirmed identical on the clean base without this change).

@cwest cwest 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.

This adds the third completion guard as a faithful clone of the acceptance-lane guard: same pre-write-txn placement so a refusal never mutates task state, same clean False return, same auditable event shape. The design choices hold up under scrutiny.

Narrowing the PR-requiring signal to worktree only (dropping the dir candidate) is the right call. A dir workspace is a plain shared directory with legitimate non-PR uses, while a worktree is definitionally a branch cut for a PR. The ~/.hermes carve-out keeps config / live-install edits completable, and checking both get_default_hermes_root() and $HOME/.hermes covers profile-scoped and custom-home deployments.

The artifact check reuses _RESPAWN_GUARD_PR_URL_RE rather than inventing a second PR-detection path, so the guard and the respawn logic agree on what "has a PR" means. The merge bypass reuses the existing allow_acceptance_complete flag and gate, identical to the acceptance guard already in place. I traced all six complete_task call sites (worker tool, CLI, swarm helper, two dashboard paths, docstring) and confirmed none of the non-merge paths pass the override, so the whole class is covered at the single chokepoint.

Ran tests/hermes_cli/test_kanban_complete_missing_pr_guard.py plus the full tests/hermes_cli/test_kanban_db.py at this head SHA: 271 passed, 0 failed. The new cases are behavior contracts against a real temp DB, and the PR-present case pins allow_acceptance_complete=False to prove the artifact, not the override, is what unlocks completion.

Merge signals are green (MERGEABLE, CLEAN, no unresolved threads, all required checks passing). No changes needed.

@cwest
cwest marked this pull request as ready for review July 10, 2026 14:41
…ct is absent

`done` on the board means exactly one thing: the declared reviewable
artifact exists. A worker could build its deliverable in a git worktree,
exit WITHOUT committing / pushing / opening a PR, and still have
`kanban_complete` flip the card to `done` — a false state whose gated
children auto-promote onto a foundation that does not exist.

`complete_task` already guards two false-`done` classes before its write
txn (phantom `created_cards`; the acceptance-lane park). This adds the
third guard in the same shape: a required-artifact completion guard.

- Signal (opt-in per card, zero schema footprint): `_card_requires_pr`
  returns True only for `workspace_kind == 'worktree'` (the sole kind
  that materializes an isolated linked git worktree on a branch — the
  implementer->PR shape) whose path is not anchored under `~/.hermes`.
  `scratch` and `dir` cards, and any `~/.hermes` edit-in-place workspace,
  are not PR-requiring and complete exactly as before.
- Artifact check: `_card_has_pr_artifact` reuses the existing PR->card
  linkage — a resolvable `pull/<n>` URL in a task comment, matched by the
  same regex the `active_pr` respawn guard already trusts.
- A guarded card with no PR is a clean no-op refusal: returns False, no
  task-state mutation, and an auditable `completion_refused_missing_pr`
  event with a `summary_preview` (mirrors `completion_refused_acceptance`).
- The merge path (`allow_acceptance_complete=True`) bypasses the guard,
  exactly as it bypasses the acceptance guard.

The guard lives inside `complete_task`, the single chokepoint every
completion path calls (worker tool, CLI complete, swarm root helper,
dashboard), so the whole class is covered by construction.

Adds tests/hermes_cli/test_kanban_complete_missing_pr_guard.py (7
behavior-contract cases against a real temp kanban DB) and a PATCHES.md row.
@cwest
cwest merged commit 24ccb59 into cwest/integration Jul 12, 2026
31 checks passed
@cwest
cwest deleted the wt/t_b8c91b61 branch July 12, 2026 20:12
cwest added a commit that referenced this pull request Jul 26, 2026
…ct is absent (#57)

`done` on the board means exactly one thing: the declared reviewable
artifact exists. A worker could build its deliverable in a git worktree,
exit WITHOUT committing / pushing / opening a PR, and still have
`kanban_complete` flip the card to `done` — a false state whose gated
children auto-promote onto a foundation that does not exist.

`complete_task` already guards two false-`done` classes before its write
txn (phantom `created_cards`; the acceptance-lane park). This adds the
third guard in the same shape: a required-artifact completion guard.

- Signal (opt-in per card, zero schema footprint): `_card_requires_pr`
  returns True only for `workspace_kind == 'worktree'` (the sole kind
  that materializes an isolated linked git worktree on a branch — the
  implementer->PR shape) whose path is not anchored under `~/.hermes`.
  `scratch` and `dir` cards, and any `~/.hermes` edit-in-place workspace,
  are not PR-requiring and complete exactly as before.
- Artifact check: `_card_has_pr_artifact` reuses the existing PR->card
  linkage — a resolvable `pull/<n>` URL in a task comment, matched by the
  same regex the `active_pr` respawn guard already trusts.
- A guarded card with no PR is a clean no-op refusal: returns False, no
  task-state mutation, and an auditable `completion_refused_missing_pr`
  event with a `summary_preview` (mirrors `completion_refused_acceptance`).
- The merge path (`allow_acceptance_complete=True`) bypasses the guard,
  exactly as it bypasses the acceptance guard.

The guard lives inside `complete_task`, the single chokepoint every
completion path calls (worker tool, CLI complete, swarm root helper,
dashboard), so the whole class is covered by construction.

Adds tests/hermes_cli/test_kanban_complete_missing_pr_guard.py (7
behavior-contract cases against a real temp kanban DB) and a PATCHES.md row.

(cherry picked from commit 24ccb59)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant