v6.7 Part 6: auto-spawn integrative architectural review at archive (#30) - #15
Merged
Merged
Conversation
…ew at archive When a JARVIS goal-mode umbrella reaches archive time with all per-block reviews terminal, the dispatcher now spawns ONE final review task assigned to T'Challa titled "Integrative architectural review (v6.7 NousResearch#30)" and blocks the umbrella's archive until that review completes with `verdict: approve`. Closes hermes-jarvis#30 Context: hermes-jarvis#61 ## Why this gate exists The v6.6 case study (hermes-jarvis#61) showed that per-block reviews miss cross-block concerns: double-DB-opens, server-path leaks, error propagation, request-handler perf. Each per-block reviewer was right about their slice; nobody held the whole system in their head. The integrative review forces ONE final pass that no per-block reviewer could do: 1. End-to-end request trace (count round-trips, identify caches) 2. End-to-end page render trace (look for redundant work) 3. Adversarial enumeration (env vars + inputs + paths + IO) 4. Error-path audit (deliberately break one layer) ## Trigger conditions (all must hold) - Task is `goal_mode=True` (JARVIS keep_running umbrella) - Has ≥1 child via task_links - Every non-review child is in {done, archived, blocked} - ≥1 review-role child exists (no point integrating over a chain with no per-block reviews) - No existing integrative-review child task yet (idempotent) Non-goal_mode tasks, tasks without children, or chains without reviews archive normally — the gate doesn't trigger. ## Workspace inheritance If the umbrella has `workspace_kind` in `{dir, worktree}`, the review task inherits the same path so T'Challa can `gh pr diff`, `npm test`, and exercise the deliverable end-to-end. Otherwise the review uses scratch. ## Idempotency + state machine The review task uses an idempotency_key of `v6.7-integrative-review:<umbrella_id>` so repeated archive calls don't create duplicates. Archive returns False (blocked) when: - New review just spawned (event: archive_blocked_pending_integrative_review) - Existing review not yet terminal (same event) - Existing review done but verdict doesn't approve (event: archive_blocked_integrative_review_rejected) Archive succeeds when the integrative review is done and its result contains `verdict: approve` (case-insensitive). ## Tests 14 new tests in `test_v6_7_integrative_review.py`: - TestSpawnTriggerConditions (6): canonical chain spawns; non-goal_mode archives; orphan umbrella archives; review-only chain spawns (vacuous non-review condition); no-review chain doesn't spawn; in-flight build-child doesn't spawn - TestIdempotency (6): no double-spawn, blocks in-flight, blocks reject, succeeds on approve, both event kinds emitted - TestSpawnedReviewBody (2): scope items present in body, workspace inherited from dir-umbrella 37 passed across Part 6 + adjacent regression set (worker_exit_code, oneshot_runtime_fallback, dispatcher_heartbeat) — zero failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dict, reject respawn Independent review of the first Part 6 commit found three BROKEN issues plus several P1s. This commit addresses every one of them. ## BROKEN #1 — spawned review was stuck in todo (deadlock) `create_task` overrides `initial_status` whenever parents are present and any parent is not done. The umbrella stays `running` until the review approves, but the review can never be claimed because parent isn't done. Classic deadlock. Fix: spawn the integrative review as a PEER task, not a child of the umbrella. The relationship is tracked via the `archive_blocked_pending_integrative_review` event payload (`review_id`) and via title-prefix + tenant lookup. New review correctly lands in `ready` and tchalla can claim it immediately. ## BROKEN #2 — substring verdict matcher was exploitable The old check used `"verdict: approve" in result.lower()` which matches inside prose like "After consideration my verdict: approve would be wrong because...". Reviewers (or hostile patches) could satisfy the gate without ever issuing a canonical verdict. Fix: new `_v6_7_parse_verdict` uses a strict line-anchored regex `^\s*verdict:\s*(approve|reject)\b` with `re.MULTILINE`. First match wins, so `verdict: reject` before `verdict: approve` wins. Prose mentions mid-sentence don't anchor. ## BROKEN #3 — reject path had no re-spawn flow After a reject, `_v6_7_should_spawn_integrative_review` returned False (existing review existed) and the umbrella was permanently stuck. Operators had to manually delete the rejected review row. Fix: `_should_spawn` now considers a done-with-reject review as spawning-ready (after the orchestrator remediates). The next archive call spawns round 2 with title suffix `:r2` (and `:r3`, etc.). The event payload includes `supersedes` and `supersedes_verdict` so operators can audit the round transitions. ## Other findings addressed - BROKEN: `blocked` was in `terminal_statuses` — a blocked Friday means INCOMPLETE work. Removed `blocked` from the set; the gate now only treats `done` / `archived` as terminal for non-review children. - BROKEN: `_v6_7_should_spawn_integrative_review` had no `has_non_review_child` check — review-only chains pathologically spawned. Added the check. - BROKEN: Dead `IntegrativeReviewSpawned` dataclass was declared but never used. Deleted. - WEAK: Integrative-review children were counted as "non-review children" in the previous title-match. The new loop explicitly skips integrative-review children by title prefix. - WEAK: docstring for `archive_task` was missing. Added. ## Tests Rewrote the test file. 27 new tests (up from 14): - TestVerdictParser (8): line-anchored regex correctness, case- insensitive, prose-mention rejection, first-match-wins, empty - TestSpawnTriggerConditions (8): canonical chain → peer review spawned with `ready` status (the critical assertion that catches the deadlock); non-goal_mode archives; orphan archives; review- only does NOT spawn (corrected from prior test); no-review chain doesn't spawn; in-flight build doesn't spawn; blocked build child doesn't qualify as terminal - TestStateMachine (5): no double-spawn on in-flight, approve unblocks, reject triggers respawn on next archive (the critical fix), event emitted on pending, event includes `supersedes` on respawn - TestSpawnedReviewBody (4): scope items present, umbrella id in body, strict verdict format documented, workspace inheritance - TestVerdictBypassClosed (2): prose-approve does NOT unblock, canonical approve does unblock Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on + dispatcher-only lookup Second self-review found one BROKEN issue and one WEAK adversarial concern. Both addressed. ## BROKEN #7 — same-tenant cross-umbrella collision Two goal_mode umbrellas sharing a tenant (the common marvel-swarm-* case) would both look up integrative reviews by the same title prefix + tenant. Umbrella B's archive could find umbrella A's review and either falsely unblock (if A was approved) or falsely block (if A was in flight). Fix: embed `umbrella_id` in the review title: ``Integrative architectural review (v6.7 NousResearch#30) for <umbrella_id>`` Lookup uses a per-umbrella LIKE pattern via two new helpers: `_v6_7_integrative_title_for(umbrella_id, round)` and `_v6_7_integrative_title_pattern(umbrella_id)`. Tenant is no longer load-bearing for isolation — the title is unique per umbrella. ## WEAK #8a — manual fake-review bypass mitigated A worker with kanban write access could create a task with the right title, mark it done with `result = "verdict: approve"`, and unblock the umbrella. Now the lookup query also filters `created_by = 'dispatcher'`, so hand-crafted fake reviews are ignored. This isn't a complete defense (a worker who can write arbitrary `created_by` values could still spoof) but defense in depth that catches the common case. ## Tests 3 new tests added (30 total, up from 27): - TestCrossUmbrellaIsolation.test_two_umbrellas_same_tenant_have_separate_reviews - TestCrossUmbrellaIsolation.test_approving_one_umbrella_does_not_unblock_another - TestFakeReviewMitigation.test_lookup_requires_created_by_dispatcher Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Final v6.7 issue. When a JARVIS goal-mode umbrella reaches archive with all per-block reviews terminal, the dispatcher spawns ONE final review task assigned to T'Challa and blocks the archive until that review approves.
Closes hermes-jarvis#30.
Why this gate exists
The v6.6 case study (hermes-jarvis#61) showed per-block reviews miss cross-block concerns: double-DB-opens, server-path leaks, error propagation, request-handler perf. Each per-block reviewer was right about their slice; nobody held the whole system in their head.
The integrative review forces ONE final pass:
Trigger conditions (all must hold)
Non-goal_mode tasks, tasks without children, or chains without reviews archive normally.
State machine
Test plan
v6.7 status after this PR
All 11 v6.7 issues shipped across 6 hermes-agent PRs (#11/#12/#13/#14 + this one) and 1 hermes-jarvis PR (NousResearch#66 for SOULs). Ready to run the v6.7 validation chain on a small test project.
🤖 Generated with Claude Code