fix(kanban): hard-gate changes-required review completion

[solovision24]

Summary

• Harden review-remediation dependency authorization: the intentional exception now fails closed when the exact internal remediation idempotency key is already present.
• Add a regression for a pre-seeded exact key whose unchecked model_override differs, proving no authoritative review_changes_requested event is written and the attacker row remains unclaimable.
• Preserve the native/webhook review lifecycle and existing remediation tests.

Verification

• HERMES_PYTHON=/home/solo/.hermes/hermes-agent/venv/bin/python bash scripts/run_tests.sh tests/hermes_cli/test_kanban_review_lifecycle.py tests/hermes_cli/test_kanban_db.py tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_cli.py -q — 112 passed.
• git diff --check — passed.
• python3 -m compileall -q hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_review_lifecycle.py — passed.

Kanban Task

• Active remediation task: t_5799fff9
• Review task: t_46e19507
• Mission Control: https://mc.solobot.cloud/tasks?task=t_5799fff9
• Original parent task: t_46e19507
• Original parent Mission Control: https://mc.solobot.cloud/tasks?task=t_46e19507

Route Evidence

• task_type=coding
• use_coding_router=true
• coding_agent=codex
• coding_agent_resolution=default
• router_run_id=run-20260802T142353-71c310f2
• router_result_path=/home/solo/.hermes/coding-worker/runs/run-20260802T142353-71c310f2/result.json
• router_status=verified_no_commit
• execution_fallback=none

Implementation

• Changed hermes_cli/kanban_db.py to reject any pre-existing exact internal remediation key rather than adopting a pre-seeded row.
• Added test_review_changes_rejects_preseeded_key_with_model_override_mismatch in tests/hermes_cli/test_kanban_review_lifecycle.py.
• Commit: 6e5c8159c10d67cd6d173eb714c8cd8dbe7f1c72

Deployment implications

• No deployment or runtime cutover performed; review/merge remains pending.

[solovision24]

Orion Review — changes required

Reviewed immutable head 3df57ed87608561131305d3eb219bf6445b143d1 directly. Focused repository-native verification passed 95/95, but two fail-closed lifecycle guarantees are still bypassable and the remediation-card acceptance criterion is not implemented.

P1 — dependency gate can still be bypassed at claim time

• hermes_cli/kanban_db.py:4257-4262 checks only parent status. A child already/racing into ready can be claimed when its parent is done with task_runs.outcome=changes_requested (or another unsuccessful review outcome).
• Reproduced against this head: after forcing that incident state and setting the child ready, claim_task() returned a task and moved it to running.
• Acceptance: make the claim-time structural invariant use the same successful-parent predicate as recompute_ready, demote/reject the child, and add a regression that exercises claim_task, not only recompute_ready.

P1 — exact-head evidence is optional instead of required

• hermes_cli/kanban_db.py:5104-5109 requires checks_passed=true but rejects exact_head_checks only when it is explicitly false; omission passes.
• Reproduced against this head: a native review run completed to done with only {"approved": true, "checks_passed": true}.
• Acceptance: require exact_head_checks is True (and preserve the immutable reviewed SHA in closing evidence), with missing/false regression cases.

P1 — requested remediation child is not created

• The task requires exactly one idempotent remediation child. request_review_changes() instead rewrites the reviewed task to ready and returns the same task id (hermes_cli/kanban_db.py:4621-4637); tests explicitly assert the board still contains one task.
• Acceptance: implement the specified one-child/idempotency contract, keep downstream dependencies unsatisfied, and cover replay/stale/archive races so repeated changes-required signals cannot create duplicates.

CI was still in progress at review time; no merge attempted.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3df57ed876

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Apply outcome gating to every dependency path

When the legacy/goal-mode race leaves a parent done with a current run outcome of changes_requested, this condition protects only recompute_ready. create_task still treats the parent's done status as success (lines 3101-3108), creating a later child as ready, and claim_task checks only the parent status (lines 4257-4260), so that child can dispatch despite the failed review. Move the unsuccessful-outcome predicate into the shared dependency/claim boundary so every path enforces it.

AGENTS.md reference: AGENTS.md:L54-L57

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Require affirmative exact-head verification

When review metadata omits exact_head_checks, dict.get() returns None, which bypasses this rejection and allows approved=true plus checks_passed=true to complete the review without proving those checks belong to the immutable PR head. This omission is especially plausible because KANBAN_COMPLETE_SCHEMA describes metadata as free-form and does not require this field; require exact_head_checks is True rather than rejecting only explicit False.

Useful? React with 👍 / 👎.

[solovision24]

Orion Review — changes required

Reviewed immutable head a20ca0567288f393e51aa2c718fed24ba4d33547 directly. CI is green and focused local verification passes 49/49, but the review gate still does not satisfy the immutable-head evidence contract.

P1 — closing evidence does not preserve or bind the reviewed SHA

• hermes_cli/kanban_db.py:5117-5131 now requires approved=true, checks_passed=true, and exact_head_checks=true, but it never requires a reviewed/head SHA in completion metadata and never compares it with the immutable head_sha from review_submitted.
• Independently reproduced: a native review completed to done with closing metadata {approved: true, checks_passed: true, exact_head_checks: true} and no SHA; the stored closing run metadata likewise contains no reviewed SHA.
• Acceptance: require a full immutable reviewed SHA in closing evidence, compare it to the submitted review head, preserve it on the closing run, and add missing/mismatch regressions.

P2 — remediation lifecycle surfaces and required race coverage remain stale/incomplete

• tools/kanban_tools.py:832-852, hermes_cli/kanban.py:2242-2258, and request_review_changes()’s docstring still claim the same card is returned/ready even though this patch marks the reviewed card done and creates a separate remediation id. The tool currently reports task_id=<reviewed card>, status=ready, which is false state and can mislead clients.
• The requested replay/stale/archive idempotency regressions were not added; the only new idempotency test performs one request and counts two tasks.
• Acceptance: update public tool/CLI semantics and tests to report the reviewed card as done plus the remediation card as ready, and add explicit replay, stale-run, and archived-remediation race regressions proving exactly one remediation child.

No merge attempted.

[solovision24]

Orion Review — changes required

Reviewed immutable head 760e1ca9480ba8e32cce47ab0e5bdbea9b71f796 directly. The immutable-head binding is now correctly enforced and focused local verification passes 63/63, but the requested public contract and canonical PR lifecycle are still incomplete.

P1 — native Review was resubmitted against the wrong PR artifact

• This branch already updates PR #14, which is clean and mergeable against dev/hermes-upgrade-t_16bbffad.
• The Review handoff instead identifies newly-opened PR #15 against main; GitHub reports PR #15 as CONFLICTING/DIRTY, and its diff exceeds 300 files because it includes the carried runtime history. That artifact is not mergeable or reviewable as submitted.
• PR #14's body also still links only stale task t_48a4f3c2, not active remediation task t_b5c6a724.
• Acceptance: keep the existing PR #14 lifecycle, update its ## Kanban Task section with t_b5c6a724 plus parent t_0a0e8e4a, close duplicate PR #15, and resubmit native Review with PR #14 at the new immutable head.

P2 — remediation response contract is only partially implemented and untested

• tools/kanban_tools.py:851 reports the parent as done and returns a remediation id, but does not report that separate remediation card as ready, which was an explicit acceptance criterion.
• hermes_cli/kanban.py:2258 likewise names the remediation task without reporting its ready state.
• No behavioral test in tests/tools/test_kanban_tools.py or tests/hermes_cli/test_kanban_cli.py asserts the new parent-done/remediation-ready response contract; the only CLI test change supplies reviewed_head_sha to an unrelated completion path.
• Acceptance: return explicit remediation status (ready) in both public surfaces and add behavioral assertions for the exact parent/remediation ids and statuses.

Evidence: scripts/run_tests.sh tests/hermes_cli/test_kanban_review_lifecycle.py tests/hermes_cli/test_kanban_cli.py tests/tools/test_kanban_tools.py -q → 63 passed; PR #14 CI is green.

[solovision24]

Orion Review — changes required

Reviewed immutable head 2a828907677cac7cac63a05fc5deb1f0a1beb41c directly. Focused repository-native verification passes 89/89, and the requested tool/CLI response contract is now covered, but the artifact is not mergeable and the new dependency exception is forgeable.

P1 — caller-controlled idempotency key bypasses unsuccessful-parent gating

• hermes_cli/kanban_db.py:4257-4265 identifies the sole remediation exception only by an idempotency_key prefix. Generic task creation exposes caller-supplied idempotency keys, so any task named review-remediation:* is treated as privileged.
• Independently reproduced on this head: after a parent ended done with run outcome changes_requested, a forged child with idempotency_key=review-remediation:forged:<run> started as ready and claim_task() accepted it; an otherwise identical ordinary child was rejected and demoted to todo.
• Acceptance: bind the exception to authoritative lifecycle evidence (the parent review-changes event/run must name this exact remediation child and expected key), reject/reserve forged remediation keys on generic creation, and add a behavioral regression proving a caller-supplied prefix cannot bypass the gate.

P1 — PR #14 conflicts with the current canonical base

• The base advanced to af8e57b3e via PR #13 after this branch diverged. GitHub now reports PR #14 CONFLICTING / DIRTY; git merge-tree --write-tree confirms content conflicts in hermes_cli/kanban.py and tests/hermes_cli/test_kanban_review_lifecycle.py.
• PR #13 also changed native/webhook convergence and intentionally restored same-card remediation, so this cannot be treated as a mechanical merge: the required child-remediation hard gate must be reconciled without regressing the newly merged webhook/native lifecycle behavior.
• Acceptance: update onto dev/hermes-upgrade-t_16bbffad, resolve the semantic conflicts, preserve PR #13 convergence behavior plus this task’s hard gates, rerun focused/full relevant verification, push a new immutable head, and wait for exact-head CI/checks.

No merge attempted. PR #15 is correctly closed, and PR #14’s Kanban links are current.

[solovision24]

Orion Review — changes required

Reviewed immutable head b72dc880f8192a41d8fe310ce35f0a171ab9c720 directly. Exact-head CI is green and focused repository-native verification passes 106/106, but the remediation authorization can still be preempted through the caller-controlled idempotency namespace.

P1 — predictable exact-key preemption hijacks the authoritative remediation handoff

• hermes_cli/kanban_db.py:3039-3052 lets any generic caller reserve an arbitrary non-archived idempotency_key. request_review_changes() then uses the predictable key review-remediation:{task_id}:{current_run_id} at hermes_cli/kanban_db.py:4808-4819 and blindly accepts any existing row returned by create_task().
• Independently reproduced on this head: pre-creating an attacker task under that exact key makes request_review_changes() return and authorize the attacker row (title=attacker-controlled, assignee=attacker, status=ready) with zero parent links. The new prefix-only forgery test does not cover exact-key preemption.
• Acceptance: reserve the internal remediation namespace from generic creation (or use a trusted internal creation path), reject/idempotently validate any pre-existing row against the complete expected child contract, persist the expected remediation key in the authoritative parent event, and require claim-time evidence to match both the exact child id and key. Add a regression that pre-seeds the exact predictable key and proves the handoff cannot be hijacked or authorized.

P2 — conflict reconciliation deleted webhook/native convergence protection

• tests/hermes_cli/test_kanban_review_lifecycle.py removes the canonical-base regressions test_webhook_first_native_submission_preserves_native_card, test_webhook_first_claimed_review_run_is_finalized, and test_webhook_first_reconciliation_merges_duplicate_subscriptions, even though preserving PR #13 webhook/native convergence was an explicit requirement. No equivalent tests exist elsewhere.
• The same reconciliation also removes submit-review, review-changes, and ingest-pr from _DELEGATED_CHILD_DENIED_ACTIONS and replaces the canonical _cmd_ingest_pr implementation with unrelated handler churn. The DB boundary remains protective, but the documented CLI fast-fail guard and focused convergence coverage should not regress as collateral conflict resolution.
• Acceptance: restore the canonical-base convergence tests and delegated CLI guard/handler behavior (or provide equivalent behavioral coverage and a specific rationale for any intentional change), while retaining the separate-child hard gate.

No merge or deployment attempted.

[solovision24]

Changes required at immutable head 02a9452b99f545ee4c291c02d76c627cfb0ff7cb.

P1 — exact-key preemption still authorizes an incomplete child contract (hermes_cli/kanban_db.py:4843-4855). The reconciliation validates title/body/assignee/creator/tenant/priority/workspace kind/branch/project/skills/parents, but omits the copied workspace_path and the expected pristine child state. I independently pre-seeded the exact predictable key with a row matching every checked field and differing only in workspace_path; request_review_changes() returned that attacker row, persisted it in the authoritative event, and claim_task() successfully claimed it from /attacker/workspace. Focused tests remain green (110/110), so the current regression does not cover this bypass.

Acceptance: validate the complete expected remediation contract before accepting any pre-existing exact-key row, including workspace_path and pristine lifecycle state (status, no current run/claim/completion/result), or reject pre-existing rows entirely if atomic creation means no legitimate retry can observe one. Add a regression whose pre-seeded row matches every currently checked field but differs only in one omitted security-relevant field and prove it is neither returned nor authorized/claimable.

No merge attempted.

[solovision24]

Orion Review — Changes Required at 4c531bec3 (P1 incomplete-contract bypass persists)

I inspected the real diff (02a9452b9..4c531bec3), ran the focused suite (111/111 pass), and independently probed the remediation contract reconciliation against an isolated temp DB.

What still fails

hermes_cli/kanban_db.py:4843-4860 now validates workspace_path and pristine lifecycle state (status=todo, no current_run_id/completed_at/result) before accepting a pre-existing exact-key row. But the field-by-field allowlist still omits security-relevant routing fields that the legitimate create_task() call at kanban_db.py:4862-4872 either copies from the parent or leaves None: model_override, provider_override, max_runtime_seconds, max_retries, goal_mode, goal_max_turns, session_id.

Independently reproduced on this head. Pre-seeding the exact predictable key (review-remediation:{task_id}:{current_run_id}) with a row matching every currently-checked field but setting model_override="evil-model", provider_override="evil-endpoint", max_runtime_seconds=1:

{"returned_attacker_id": "t_bf1fc888",
 "event_persisted": true,
 "persisted_remediation_task_id": "t_bf1fc888",
 "persisted_remediation_key": "review-remediation:t_ea27d549:2",
 "child_status": "ready",
 "claim_succeeded": true,
 "child_model_override": "evil-model",
 "child_provider_override": "evil-endpoint",
 "child_max_runtime_seconds": 1}

request_review_changes() returned the attacker row, persisted it in the authoritative parent review_changes_requested event, and claim_task() claimed and authorized it. The new regression test_review_changes_rejects_preseeded_key_with_workspace_mismatch does not cover this because it diverges on workspace_path, which is now checked.

The claim-time parent gate (kanban_db.py:4489-4495) only re-validates remediation_task_id and remediation_key; it does not re-check the child routing fields. So an attacker who can pre-create a task under the predictable key injects arbitrary worker routing (model_override/provider_override/max_runtime_seconds) into the authoritative remediation handoff.

Acceptance (preferred → simplest, fail-closed)

Option A (recommended): reject pre-existing exact-key rows entirely. The idempotent create_task() insert now runs inside the request_review_changes() lifecycle transaction (kanban_db.py:3092), so no legitimate retry can observe a pre-existing row. Return None when existing is not None and fail the changes-requested cycle. This eliminates the entire class of "which fields must match" bugs and matches the structural-integrity spirit of the dependency gate.

Option B (if keeping reconciliation): validate the complete contract. Add at minimum model_override, provider_override, max_runtime_seconds, max_retries, goal_mode, goal_max_turns, and session_id to the rejection condition at kanban_db.py:4843-4860. Note this is a perpetual allowlist maintenance burden — every new routing field on Task re-opens the bypass.

Either way, add a regression whose pre-seeded row matches every currently-checked field but differs in exactly one routing field (e.g. model_override), and prove it is neither returned, persisted in the authoritative event, nor claimable. The existing workspace_path regression is necessary but insufficient.

Verification context

• Focused suite: scripts/run_tests.sh tests/hermes_cli/test_kanban_review_lifecycle.py tests/hermes_cli/test_kanban_db.py tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_cli.py -q → 111 passed.
• Exact-head CI (run 30760390673): all required checks SUCCESS.
• git diff --check: clean.
• Adversarial reproduction used an isolated temp HERMES_KANBAN_DB; live board untouched.

Routing remediation back to DEV on this branch. No merge.

[solovision24]

Changes required on immutable head 4c531bec30ae283a2885244add12fab88df5bebc:

• High — incomplete exact-key contract remains claimable (hermes_cli/kanban_db.py:4833-4861). The added checks cover workspace_path and the listed lifecycle fields, but still accept a pre-seeded exact-key row with unvalidated execution fields. I reproduced this with every checked field matching and only model_override="attacker-model" differing: request_review_changes() returned that existing child, promoted it to ready, preserved the attacker-controlled model override, and claim_task() succeeded. Other creation-default fields are also unchecked.

Acceptance: fail closed on any pre-existing exact remediation key (preferred; there is no legitimate pre-existing child for a fresh review run), or validate the complete row contract including execution/routing/default lifecycle fields. Add the regression using an omitted execution field and prove no authoritative handoff event is persisted and the row is not claimable.

Independent verification: the required focused suite is green (111/111) and git diff --check passes, so this is a missing security invariant rather than a general test failure.

[solovision24]

Orion Review — Accepted at 6e5c8159c10d67cd6d173eb714c8cd8dbe7f1c72

• Inspected the remediation diff and the complete six-file PR surface against the canonical base.
• The exact-key path now fails closed on any pre-existing remediation key, eliminating unchecked execution-field adoption rather than extending a fragile field allowlist.
• Regression coverage proves a pre-seeded row differing only by model_override produces no authoritative review_changes_requested event and remains unclaimable.
• Independent focused verification: 112/112 passed.
• Exact-head CI is fully green, including all Python slices, lints, supply-chain checks, and Desktop E2E.

GitHub cannot record a native same-account approval here; this comment is Orion governance acceptance. Proceeding with an immutable-head squash merge.