From 1a841372d04413c2bf9401a26a0926e97dae04aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=88=98=E5=8D=87=E9=BE=99?= Date: Sun, 20 Sep 2026 14:59:36 +0800 Subject: [PATCH] fix(orchestration): restore controller-owned review authority Treat implementation and final-review verdicts as advisory input while keeping qualification, acceptance, repair routing, and closure with the controller. Repair active canonical artifacts in place and validate current reference chains before writes and finalization. --- .../contract/handoff-executor-result-v1.md | 2 +- .../assets/orchestration/contract/plan-v1.md | 4 +- .../contract/specification-v1.md | 2 + references/assets/orchestration/workflow.md | 18 +- references/evals/orchestration/evals.json | 38 ++-- .../orchestration/orch-artifact-authoring.md | 6 +- rules/orchestration/orch-handoff-required.md | 4 +- .../orch-orchestration-boundary.md | 6 +- rules/orchestration/orch-review-completion.md | 17 +- scripts/orchestration/handoffs.py | 22 ++- scripts/orchestration/plans.py | 173 ++++++------------ scripts/orchestration/review_runtime.py | 152 ++++++++++++--- scripts/orchestration/specs.py | 27 ++- skills/orch-create-handoff/SKILL.md | 8 +- .../orch-create-implementation-plan/SKILL.md | 5 +- skills/orch-create-specification/SKILL.md | 9 +- skills/orch-execute-plan/SKILL.md | 8 +- skills/orch-review-plan/SKILL.md | 14 +- tests/test_orchestration_plans.py | 80 +++++++- .../test_orchestration_skill_rule_boundary.py | 6 +- tests/test_orchestration_specifications.py | 51 +++++- .../test_orchestration_stage5_current_path.py | 151 ++++++++++++++- .../test_orchestration_workflow_contracts.py | 4 +- tests/test_rule_contracts.py | 9 +- 24 files changed, 586 insertions(+), 230 deletions(-) diff --git a/references/assets/orchestration/contract/handoff-executor-result-v1.md b/references/assets/orchestration/contract/handoff-executor-result-v1.md index c3ba2f6..3624332 100644 --- a/references/assets/orchestration/contract/handoff-executor-result-v1.md +++ b/references/assets/orchestration/contract/handoff-executor-result-v1.md @@ -6,7 +6,7 @@ The agent-authored semantic content records implemented scope, changed paths, fo An executor result never issues a product verdict. It contains no implementation-review decision, accepted-result decision, final-audit conclusion, repair recommendation, or knowledge-write authorization. A distinct reviewer compares the exact frozen implementation directly with the verified specification and plan. -Before creation or transition, validate the entire schema, identity, plan/task bindings, canonical path, collision state, and requested lifecycle operation. Write one artifact atomically. After creation, perform only lightweight integrity checks and rebuild the disposable index projection. If an index update fails after the canonical write, report the partial effect truthfully. +Before creation, active repair, or transition, validate the entire schema, identity, plan/task bindings, canonical path, and requested lifecycle operation. Create or repair the active artifact atomically at the same canonical identity; allocate a new identity only for a genuinely distinct result, and never rewrite a transitioned result. After the write, perform only lightweight integrity checks and rebuild the disposable index projection. If an index update fails after the canonical write, report the partial effect truthfully. Missing or defective executor-result structure blocks continuation that requires that artifact. It does not veto direct product review when the exact implementation candidate, verified specification/plan, and claim-relevant observations are independently available. diff --git a/references/assets/orchestration/contract/plan-v1.md b/references/assets/orchestration/contract/plan-v1.md index 707ffb4..87a09b0 100644 --- a/references/assets/orchestration/contract/plan-v1.md +++ b/references/assets/orchestration/contract/plan-v1.md @@ -10,6 +10,8 @@ Assign parallel tasks only when dependencies are satisfied and write scopes are Every task declares `evidence_capability`. Use `mapped` with a lightest-capable task-local oracle for validation-bearing obligations, or `no_validation_bearing_obligation` with a concrete reason. -Run canonical static task admission before semantic review. Structural success cannot establish source-ID coverage, appropriate decomposition, capable validation, or executability. A distinct reviewer compares the stored tree directly with the verified specification and current source evidence and issues `accept`, `repair`, or `blocked`. +Run canonical static task admission before semantic review. Structural success cannot establish source-ID coverage, appropriate decomposition, capable validation, or executability. A distinct reviewer compares the complete stored tree directly with the verified specification and current source evidence and provides advisory `accept`, `repair`, or `blocked` findings. The controller/orchestrator assesses that advice and owns plan qualification. After a repair, review the complete current tree again; a previous opinion or a review of only the changed fields does not establish the accuracy of the current plan. + +`write-plan`, `write-phase`, and `write-task` update an existing active identity at its canonical path. Use a new identity only for a genuinely distinct plan, phase, or task, never to retain intermediate repair versions. Return a verified root plan to `draft` before changing phase or task content, then qualify the complete repaired tree through semantic review. The canonical semantic plan projection is `canonical-yaml-plan-tree-v1`. A status-only or append-only evidence change declared by the schema is excluded at the exact top-level control locations; all other semantics remain identity-bearing. Current plan and specification revisions do not consume post-execution review rounds. diff --git a/references/assets/orchestration/contract/specification-v1.md b/references/assets/orchestration/contract/specification-v1.md index 0be02cb..1b2f217 100644 --- a/references/assets/orchestration/contract/specification-v1.md +++ b/references/assets/orchestration/contract/specification-v1.md @@ -23,6 +23,8 @@ execution_workspace: The immutable `specification-v1` schema owns this structural front matter. The agent supplies the human-readable semantic body and semantic metadata, while `scripts/orch.py write-spec` owns family, schema version, identity, qualification, timestamps, canonical `.work-bundle/orchestration/spec/{state}/{id}.spec.md` location, atomic write, lifecycle movement, and index projection. Caller-authored structural overrides and filenames are invalid. +`write-spec` updates an existing active specification at its canonical identity and returns repaired content to `draft`. Allocate a new identity only for a genuinely distinct specification, not for an intermediate review revision. A distinct reviewer then advises on the complete current specification against the original purpose, accepted authority, and workspace evidence. The controller/orchestrator assesses that advice and owns qualification; review of only the changed passages and an inherited earlier opinion are insufficient. + The front-matter `source_knowledge` contains accepted authority only, as established by bounded retrieval and Source Context reconciliation. Each accepted entry carries a provenance `path` and the already-reconciled task-relevant `constraint`. Candidate, background, blocked, and superseded knowledge remains classified in Source Context or Open Questions and must not appear in this carried-authority list. Downstream planning allocates deterministic `AUTH-NNN` aliases by list order so executor packets remain traceable without exposing knowledge paths. The compiler resolves each allocated alias to `AUTH-NNN: ` in the task brief and review package. # Introduction diff --git a/references/assets/orchestration/workflow.md b/references/assets/orchestration/workflow.md index eda0ad0..d05af2f 100644 --- a/references/assets/orchestration/workflow.md +++ b/references/assets/orchestration/workflow.md @@ -1,6 +1,6 @@ # WorkBundle Orchestration Workflow -WorkBundle separates semantic judgment from schema-owned mechanics. Agents interpret user purpose, accepted authority, product correctness, findings, qualification, and acceptance. Scripts validate schema, identity, bindings, canonical paths, scope safety, lifecycle transitions, immutable bytes, and disposable index projections. +WorkBundle separates semantic judgment from schema-owned mechanics. Agents interpret user purpose, accepted authority, product correctness, findings, qualification, and acceptance. Scripts validate schema, identity, bindings, canonical paths, scope safety, lifecycle transitions, stored bytes, and disposable index projections. ## Shared foundation @@ -10,11 +10,13 @@ Canonical artifacts are authority. Derived indexes are regenerable projections. ## Specification -The agent authors complete specification semantics. The schema-backed writer owns structural fields, canonical location, immutable family identity, lifecycle movement, and index projection. A distinct reviewer judges user-purpose alignment, authority, requirement/constraint/interface/acceptance coverage, conflicts, open questions, and scope before the specification becomes verified. +The agent authors complete specification semantics. The schema-backed writer owns structural fields, canonical location, immutable family identity, lifecycle movement, and index projection. A distinct reviewer advises on user-purpose alignment, authority, requirement/constraint/interface/acceptance coverage, conflicts, open questions, and scope. The controller/orchestrator assesses that advice and owns specification qualification. ## Planning -The agent authors root-plan, phase, and task semantics from a verified specification. Each artifact cites exact source IDs. The plan assigns ownership, dependencies, write scopes, task-local methodology, validation capability, and completion criteria. Static admission may report structural and graph facts but cannot qualify the plan. A distinct reviewer judges decomposition and executability. +The agent authors root-plan, phase, and task semantics from a verified specification. Each artifact cites exact source IDs. The plan assigns ownership, dependencies, write scopes, task-local methodology, validation capability, and completion criteria. Static admission may report structural and graph facts but cannot qualify the plan. A distinct reviewer advises on the accuracy, completeness, decomposition, and executability of the complete current tree; the controller/orchestrator assesses that advice and owns qualification. + +Repair current plan content at the existing canonical plan, phase, and task identities. Do not create active copies for intermediate review revisions. Return a verified plan to draft before child-content repair. After every repair, review the complete current tree against the verified specification and bounded source evidence; neither an earlier verdict nor a delta-only check qualifies the repaired content. Do not optimize task or phase cardinality. Bound expected total orchestration cost at concrete independently owned production, dependency, validation, review, and repair seams. Every authoritative production path needs a production owner. Keep a coherent mechanical increment with one owner, oracle, and repair frontier together. Create phases only for an actual barrier or convergence boundary and reject speculative splits. When a task is materially under-decomposed, return to the plan and reslice only the affected region; do not repeatedly enlarge it. @@ -24,25 +26,25 @@ Disposable task briefs and lightweight development plans compile accepted author Executors work only within bound task/repository/write scope. Behavior changes use task-local methodology and claim-relevant focused validation. Executors report facts; they never accept the product. -After execution, create one canonical `executor-result-v1` under the catalog-selected result location. It records implemented scope, changed paths, focused validation observations, unresolved product blockers, task fit, repository/CodeGraph facts, delegation, and knowledge disposition. Structural validation, collision checks, and binding checks occur before mutation. Historical handoffs, embedded statuses, override sidecars, and fallback indexes are unsupported and ignored. +After execution, create one canonical `executor-result-v1` under the catalog-selected result location. It records implemented scope, changed paths, focused validation observations, unresolved product blockers, task fit, repository/CodeGraph facts, delegation, and knowledge disposition. Necessary structural and binding checks occur before mutation. Repair the active result at the same identity; do not retain intermediate revision copies. Historical handoffs, embedded statuses, override sidecars, and fallback indexes are unsupported and ignored. ## Direct implementation review Freeze an exact commit or worktree candidate using a path-sorted changed-path manifest. A distinct reviewer compares the actual candidate directly with the verified specification and canonical plan, every planned feature and acceptance obligation, edge and failure behavior, and capable focused observations. -The reviewer issues `accept`, `repair`, or `blocked` from product correctness. Passing tests cannot hide missing behavior. Missing or defective historical artifacts, indexes, knowledge state, or controller ceremony are separate supporting-state defects unless they make the product ambiguous, unsafe, inaccessible, or impossible to review. Store the decision in one canonical `implementation-review-v1`. +The reviewer issues an advisory `accept`, `repair`, or `blocked` assessment from product correctness. Passing tests cannot hide missing behavior. Missing or defective historical artifacts, indexes, knowledge state, or controller ceremony are separate supporting-state defects unless they make the product ambiguous, unsafe, inaccessible, or impossible to review. Store the advice in the current canonical `implementation-review-v2`; its `verdict` field is a recommendation, not orchestration authority. A complete re-review after repair updates that active identity instead of retaining intermediate verdict copies. Reviewer execution may use read-only workspace isolation and transient provider diagnostics. Those operational facts do not become semantic or lifecycle authority and are not replayed by downstream consumers. ## Accepted continuation -After an accepted implementation review when required, create one canonical `accepted-task-result-v1`. It references the exact executor result, accepted implementation review, current validation outcomes, product identity, unresolved material defects, and knowledge disposition. Dependencies and final review consume this compact decision without redispatching the executor or reconstructing review history. +After assessing the implementation review advice, the controller/orchestrator either routes repair/blocking or creates one canonical `accepted-task-result-v1`. It references the exact executor result, implementation review advice when required, current validation outcomes, product identity, unresolved material defects, and knowledge disposition. Repair its active identity in place. Dependencies and final review consume this compact controller decision without redispatching the executor or reconstructing review history. ## Final workflow review -A distinct final auditor performs one compact pass over plan/task coverage, accepted implementation verdicts, relevant current tests, unresolved material defects, final knowledge disposition/return, repository finalization facts, and truthful archive readiness. The auditor does not reread source for code quality or repeat implementation review. +A distinct final auditor performs one compact advisory pass over plan/task coverage, controller-accepted implementation decisions, relevant current tests, unresolved material defects, final knowledge disposition/return, repository finalization facts, and truthful archive readiness. The auditor does not reread source for code quality or repeat implementation review. The controller/orchestrator assesses that advice and owns the closure decision. -Store the verdict in one canonical `final-workflow-review-v1`. Deterministic finalization carries that supplied verdict and validates only canonical references, lifecycle state, clean baselines, archive destinations, index rebuilds, and binding release. Mechanical failure cannot manufacture or reinterpret a semantic verdict. +Store or repair the controller/orchestrator closure decision at one active canonical `final-workflow-review-v1` identity. Deterministic finalization carries that supplied decision and validates only canonical references, lifecycle state, clean baselines, archive destinations, index rebuilds, and binding release. Mechanical failure cannot manufacture or reinterpret a semantic decision. ## Knowledge disposition diff --git a/references/evals/orchestration/evals.json b/references/evals/orchestration/evals.json index 7066f95..3ae0d48 100644 --- a/references/evals/orchestration/evals.json +++ b/references/evals/orchestration/evals.json @@ -52,13 +52,13 @@ { "id": 9, "prompt": "Review an implemented plan where the project files do not satisfy the source specification.", - "expected_output": "Selects review-plan, directly compares the exact implementation candidate with every verified specification and plan obligation, issues repair from missing product behavior even when tests pass, and routes specification repair only when the defect belongs to requirement or authority.", + "expected_output": "Selects review-plan, directly compares the exact implementation candidate with every verified specification and plan obligation, advises repair for missing product behavior even when tests pass, and lets the controller/orchestrator assess the finding and route repair to implementation or specification authority.", "files": [] }, { "id": 10, "prompt": "Review an implemented plan where all specification, plan, handoff, and project file checks pass.", - "expected_output": "Selects review-plan, consumes accepted implementation verdicts and current test outcomes, checks plan coverage, material defects, knowledge return, repository facts, and archive readiness once, then lets deterministic finalization perform only lifecycle, index, baseline, destination, and binding mechanics.", + "expected_output": "Selects review-plan, consumes controller-accepted task decisions, exact review advice, and current test outcomes; checks plan coverage, material defects, knowledge return, repository facts, and archive readiness once; then lets the controller/orchestrator own closure and deterministic finalization perform only lifecycle, index, baseline, destination, and binding mechanics.", "files": [] }, { @@ -273,20 +273,20 @@ }, { "id": 46, - "prompt": "Run final review after every task already has an accepted independent task review.", - "expected_output": "orch-review-plan audits workflow, handoff, knowledge, and finalization evidence and does not inspect source code quality again.", + "prompt": "Run final review after every task already has a controller-accepted result and independent review advice.", + "expected_output": "orch-review-plan provides a compact audit of workflow, result, knowledge, and finalization facts without inspecting source code quality again; the controller/orchestrator assesses the advice and owns closure.", "files": [] }, { "id": 47, - "prompt": "Final review finds one completed task has no task-review verdict.", - "expected_output": "Returns review-blocked and resumes the owning execution review step; it does not create a repair specification.", + "prompt": "Final audit finds one completed task that required independent review has no review advice.", + "expected_output": "Advises that closure is blocked by missing required review input; the controller/orchestrator assesses that advice and routes the owning execution-review step without creating a repair specification.", "files": [] }, { "id": 48, "prompt": "Final review proves the specification contains the wrong API requirement.", - "expected_output": "Routes to specification repair because the defect is requirement authority, while keeping implementation artifacts unarchived.", + "expected_output": "The final auditor advises specification repair because the defect belongs to requirement authority; the controller/orchestrator assesses the finding, routes the accepted repair, and keeps implementation artifacts unarchived.", "files": [] }, { @@ -340,7 +340,7 @@ { "id": 57, "prompt": "An independent task reviewer finds a blocking correctness defect.", - "expected_output": "Returns repair with compact evidence, routes to the same task implementer, reruns fresh validation, regenerates the package from the original base, and re-reviews.", + "expected_output": "Returns advisory repair findings with compact evidence; the controller/orchestrator assesses them and, when accurate, routes the same task implementer, reruns fresh validation, regenerates the package from the original base, and requests rereview.", "files": [] }, { @@ -412,7 +412,7 @@ { "id": 69, "prompt": "Review a heavy task whose implementation and tests do not match the compiled Truth Basis.", - "expected_output": "Returns repair after comparing accepted purpose, source evidence, decision authority, expected delta, test oracle, and disposition.", + "expected_output": "Returns advisory repair findings after comparing accepted purpose, source evidence, decision authority, expected delta, test oracle, and disposition; the controller/orchestrator assesses them before routing repair.", "files": [] }, { @@ -580,7 +580,7 @@ { "id": "STG3-01", "prompt": "Create and verify a specification after structural tests pass, but the draft omits one user requirement and an interface constraint.", - "expected_output": "Uses the specification-v1 canonical .spec.md family for structure, but the distinct agent reviewer rejects the semantics for concrete missing coverage; tests and index state do not issue verification.", + "expected_output": "Uses the specification-v1 canonical .spec.md family for structure; the distinct agent reviewer advises repair for concrete missing coverage, and the controller/orchestrator assesses the accurate finding and withholds verification. Tests and index state do not issue verification.", "files": [] }, { @@ -598,7 +598,7 @@ { "id": "STG3-04", "prompt": "A caller asks the specification writer to preserve its custom filename, embedded id, verified status, and timestamp because a receipt already exists.", - "expected_output": "Rejects caller structural overrides and filename selection before mutation; the receipt does not decide semantic qualification, which remains a distinct reviewer-agent judgment.", + "expected_output": "Rejects caller structural overrides and filename selection before mutation; the receipt does not decide semantic qualification, and reviewer advice remains an input to the controller/orchestrator's qualification decision.", "files": [] }, { @@ -610,7 +610,7 @@ { "id": "STG4-02", "prompt": "All plan schemas and static admission checks pass, but one accepted interface and its validation obligation have no task owner.", - "expected_output": "The distinct semantic reviewer issues repair for missing ownership and capable validation; tests, doctors, indexes, receipts, and evidence volume cannot qualify the incomplete plan.", + "expected_output": "The distinct semantic reviewer advises repair for missing ownership and capable validation; the controller/orchestrator assesses that advice and withholds qualification because the defect is accurate. Tests, doctors, indexes, receipts, and evidence volume cannot qualify the incomplete plan.", "files": [] }, { @@ -634,29 +634,35 @@ { "id": "STG5-02", "prompt": "A frozen worktree candidate passes its focused tests but omits one accepted behavior from the verified specification.", - "expected_output": "The distinct reviewer directly compares the exact manifest with every specification and plan obligation and records repair in implementation-review-v1; green tests do not hide missing behavior.", + "expected_output": "The distinct reviewer directly compares the exact manifest with every specification and plan obligation and records repair advice in implementation-review-v2; the controller/orchestrator assesses the finding and routes the accurate product defect. Green tests do not hide missing behavior.", "files": [] }, { "id": "STG5-03", "prompt": "A correct reviewable candidate has a stale index and no historical reviewer receipt.", - "expected_output": "The product reviewer may accept the exact candidate; the supporting-state defect is routed separately and cannot manufacture or veto the semantic verdict.", + "expected_output": "The product reviewer may advise acceptance of the exact candidate; the controller/orchestrator assesses that advice and owns the product decision. The supporting-state defect is routed separately and cannot manufacture or veto acceptance.", "files": [] }, { "id": "STG5-04", "prompt": "A dependency needs the already accepted task outcome.", - "expected_output": "Consumes one canonical accepted-task-result-v1 referencing the executor result, exact accepted implementation review, current validation outcomes, product identity, defects, and knowledge disposition without receipt or history replay.", + "expected_output": "Consumes one canonical accepted-task-result-v1 containing the controller/orchestrator acceptance decision and referencing the executor result, exact implementation review advice, current validation outcomes, product identity, defects, and knowledge disposition without receipt or history replay.", "files": [] }, { "id": "STG5-05", "prompt": "Run final workflow closure after every planned task has a compact accepted result.", - "expected_output": "A distinct final auditor writes one final-workflow-review-v1 covering plan/task coverage, accepted verdicts, current tests, material defects, knowledge return, repository facts, and archive readiness without repeating code review or reconstructing history.", + "expected_output": "A distinct final auditor advises on plan/task coverage, controller-accepted decisions, current tests, material defects, knowledge return, repository facts, and archive readiness without repeating code review or reconstructing history. The controller/orchestrator assesses that advice and writes one final-workflow-review-v1 closure decision.", "files": [] } ], "v4_evals": [ + { + "id": "v4-in-place-plan-repair-full-review", + "prompt": "Repair a verified implementation plan after review finds an omitted production owner, then review the repaired plan.", + "expected_output": "Returns the existing root plan to draft, updates the existing plan, phase, and task identities in place without retaining intermediate revision copies, and gives a distinct reviewer the complete current tree. The reviewer checks every accepted specification obligation and current source boundary rather than limiting review to the repair delta; the controller/orchestrator assesses the advice and owns qualification.", + "files": [] + }, { "id": "v4-obsolete-role-context-retirement", "description": "Given a request to preserve deprecated role-context aliases for compatibility, require repository-wide consumer characterization and then remove the skill, rule, CLI, implementation, reference, and positive runtime guidance when no supported external consumer exists; retain only a negative non-reintroduction guard." diff --git a/rules/orchestration/orch-artifact-authoring.md b/rules/orchestration/orch-artifact-authoring.md index cf5df9c..0437bc7 100644 --- a/rules/orchestration/orch-artifact-authoring.md +++ b/rules/orchestration/orch-artifact-authoring.md @@ -39,6 +39,8 @@ Keep orchestration artifacts human-readable, contract-compliant, and executable - Summarize spec intent at most once in a root plan, then cite IDs for downstream detail. - Require leading spec-repair tasks when a phase or task lacks stable IDs, exact paths, validation details, or file-level execution context. - Let the shared store rebuild the distinct per-family indexes when registered artifacts change; derived indexes are projections and do not replace canonical artifacts. +- Update an active orchestration artifact at its existing canonical identity when repairing its content. Allocate a new identity only for a genuinely distinct semantic artifact, not for an intermediate review revision. Transitioned historical records remain immutable. +- After plan repair, require the distinct reviewer to assess the complete current canonical tree against all accepted specification obligations and bounded source evidence. The controller/orchestrator evaluates that advice and owns qualification; earlier opinions and delta-only checks are supporting context, not semantic qualification of the current tree. Contract loading by artifact type: @@ -64,6 +66,7 @@ Contract loading by artifact type: - Embed implementation plan tasks inside specifications. - Write raw chat logs, unsupported facts, or hidden reasoning into orchestration artifacts. - Infer artifact identity, lifecycle state, relationships, or acceptance from filenames, headings, search order, or fallback defaults when a registered structural contract owns those facts. +- Preserve intermediate repair versions as additional active canonical artifacts or qualify repaired content from a review limited to changed fields. ## Validation @@ -75,7 +78,8 @@ Contract loading by artifact type: - Confirm task files are self-contained for execution from the related spec plus their own instructions. - Confirm artifact sections satisfy the loaded contract or explicitly add missing required sections named by the directive. - Confirm registered families use the shared structural owner, unregistered families have not gained placeholder authority, and any search is both declared and followed by canonical structural validation. -- Confirm plan semantic qualification came from direct agent review of specification coverage, ownership, dependencies, validation, authority, scope, and executability; structural checks and supporting ceremony did not issue that verdict. +- Confirm plan semantic qualification came from the controller/orchestrator's assessment of distinct-agent advice on specification coverage, ownership, dependencies, validation, authority, scope, and executability; neither the reviewer recommendation nor structural checks and supporting ceremony issued the qualification automatically. +- Confirm repaired artifacts retained their canonical identities, the complete current plan tree—not merely its delta—received independent review, and the controller/orchestrator assessed that advice before qualification. ## On Violation diff --git a/rules/orchestration/orch-handoff-required.md b/rules/orchestration/orch-handoff-required.md index 9b5bfeb..0bff196 100644 --- a/rules/orchestration/orch-handoff-required.md +++ b/rules/orchestration/orch-handoff-required.md @@ -17,7 +17,7 @@ Require one canonical factual executor result for safe continuation without gran ## Must - Create one canonical `executor-result-v1` after task execution, including factual scope, changed paths, focused observations, blockers, task fit, repository/CodeGraph facts, delegation, and knowledge disposition. -- Validate identity, bindings, schema, canonical path, collisions, and transition before mutation; after creation perform only lightweight integrity and index checks. +- Validate identity, bindings, schema, canonical path, and transition before mutation; create or repair the active result at the same canonical identity, while transitioned results remain immutable. After the write perform only lightweight integrity and index checks. - Keep executor reporting separate from independent product judgment and controller finalization. - Treat an invalid result as blocking only continuation that requires it. Permit direct product review when exact specification/plan, frozen implementation identity, and focused observations are independently available. @@ -28,7 +28,7 @@ Require one canonical factual executor result for safe continuation without gran ## Validation -- Confirm the canonical artifact and bindings, factual closed content, immutable bytes, and truthful partial-effect reporting. +- Confirm the canonical artifact and bindings, factual closed content, in-place active repair, and truthful partial-effect reporting. ## On Violation diff --git a/rules/orchestration/orch-orchestration-boundary.md b/rules/orchestration/orch-orchestration-boundary.md index 672ea3a..b009275 100644 --- a/rules/orchestration/orch-orchestration-boundary.md +++ b/rules/orchestration/orch-orchestration-boundary.md @@ -18,13 +18,13 @@ Keep every current orchestration artifact in its canonical role and keep structu - Keep specifications, plans, tasks, executor results, implementation reviews, accepted task results, and final workflow reviews in their catalog-owned roles under `.work-bundle/orchestration/`. - Keep durable knowledge under `.work-bundle/knowledge/` and delegate approved writes to `ks-*` owners. -- Treat canonical artifacts as authority and indexes as disposable projections. -- Use executor results for facts, implementation reviews for product verdicts, accepted task results for dependency continuation, and final workflow reviews for compact closure judgment. +- Treat each canonical artifact as authority only for its declared role and indexes as disposable projections. +- Use executor results for facts, implementation reviews for independent advisory findings, accepted task results for controller/orchestrator acceptance and dependency continuation, and final workflow reviews for the controller/orchestrator's compact closure decision after considering final-audit advice. - Validate structural mechanics before mutation and keep post-write checks lightweight. ## Must Not -- Do not merge artifact roles, reconstruct authority from history, create compatibility sidecars, or let structural helpers decide correctness or acceptance. +- Do not merge artifact roles, reconstruct authority from history, create compatibility sidecars, let structural helpers decide correctness or acceptance, or treat reviewer advice as an automatic acceptance/rejection command. - Do not read, migrate, or rewrite historical orchestration artifacts during current-path work. ## Validation diff --git a/rules/orchestration/orch-review-completion.md b/rules/orchestration/orch-review-completion.md index 28bff8b..d7d62ff 100644 --- a/rules/orchestration/orch-review-completion.md +++ b/rules/orchestration/orch-review-completion.md @@ -12,25 +12,26 @@ requires: [] ## Purpose -Keep implementation acceptance in direct independent product review and keep final workflow closure compact, factual, and non-recursive. +Keep independent product review advisory, keep controller/orchestrator acceptance authoritative, and keep final workflow closure compact, factual, and non-recursive. ## Must - Require a distinct implementation reviewer to compare the exact frozen candidate with every verified specification and plan obligation plus capable focused observations. -- Let the reviewer issue `accept`, `repair`, or `blocked` from product correctness. Green tests cannot hide missing behavior. -- Keep missing historical records, indexes, knowledge state, and controller ceremony outside the product verdict unless the product is ambiguous, unsafe, inaccessible, or impossible to review. -- Carry accepted task decisions through canonical `accepted-task-result-v1` records. -- Use one compact final workflow review for coverage, accepted verdicts, current tests, material defects, knowledge disposition/return, repository facts, and archive readiness. +- Let the reviewer issue an advisory `accept`, `repair`, or `blocked` assessment with concrete findings. Green tests cannot hide missing behavior. +- Require the controller/orchestrator to assess the review advice against user purpose, accepted authority, and the product before deciding acceptance, repair, blocking, or continuation. Reviewer advice is not an automatic veto or acceptance. +- Keep missing historical records, indexes, knowledge state, and controller ceremony outside the controller/orchestrator product decision unless the product is ambiguous, unsafe, inaccessible, or impossible to review. +- Carry controller/orchestrator accepted task decisions through canonical `accepted-task-result-v1` records, preserving the exact review advice considered. +- Use one compact final audit for coverage, review advice, current tests, material defects, knowledge disposition/return, repository facts, and archive readiness; the controller/orchestrator assesses that advice and owns the final workflow decision. - Keep finalization mechanical: canonical references, lifecycle, clean baselines, destinations, indexes, and binding release only. ## Must Not -- Do not repeat code review during final audit, reconstruct history, replay transient evidence, or let helpers infer semantic sufficiency. +- Do not repeat code review during final audit, reconstruct history, replay transient evidence, let helpers infer semantic sufficiency, or mechanically promote a reviewer recommendation into acceptance or rejection. ## Validation -- Confirm exact candidate identity, distinct reviewers, obligation coverage, compact accepted results, and a non-recursive final audit. +- Confirm exact candidate identity, distinct reviewers, obligation coverage, explicit controller/orchestrator assessment, compact accepted results, and a non-recursive final audit. ## On Violation -- Withhold acceptance or finalization, report the unmet review or closure condition, and return the affected scope to repair and distinct rereview. +- Report the unmet review or closure condition to the controller/orchestrator, which decides whether to accept, repair, block, or request distinct rereview. diff --git a/scripts/orchestration/handoffs.py b/scripts/orchestration/handoffs.py index 28d1239..dcac5b9 100644 --- a/scripts/orchestration/handoffs.py +++ b/scripts/orchestration/handoffs.py @@ -15,6 +15,7 @@ canonical_artifact_path, family_policy, load_catalog, + read_artifact, read_yaml_mapping, rebuild_index, transition_artifact, @@ -72,9 +73,23 @@ def _bindings(args: argparse.Namespace) -> dict[str, str]: return {"plan": str(args.plan_id), "task": str(args.task_id)} -def _assert_identity_available(args: argparse.Namespace) -> None: +def _active_result_or_none(args: argparse.Namespace) -> dict[str, Any] | None: policy = _policy() + active = canonical_artifact_path( + policy, + _anchors(args), + identity=str(args.id), + state="active", + bindings=_bindings(args), + ) + if active.is_file(): + return read_artifact( + CATALOG_PATH, FAMILY, _anchors(args), identity=str(args.id), + state="active", bindings=_bindings(args), + ) for state in policy["lifecycle"]["states"]: + if state == "active": + continue target = canonical_artifact_path( policy, _anchors(args), @@ -84,11 +99,12 @@ def _assert_identity_available(args: argparse.Namespace) -> None: ) if target.exists(): raise SystemExit(f"Executor-result canonical identity collision: {args.id}") + return None def write_executor_result(args: argparse.Namespace) -> dict[str, Any]: semantic = _semantic_input(Path(str(args.content_file))) - _assert_identity_available(args) + existing = _active_result_or_none(args) today = now_date() data = { **semantic, @@ -98,7 +114,7 @@ def write_executor_result(args: argparse.Namespace) -> dict[str, Any]: "plan_id": str(args.plan_id), "phase_id": getattr(args, "phase_id", None), "task_id": str(args.task_id), - "date_created": today, + "date_created": str(existing["data"]["date_created"]) if existing else today, "last_updated": today, } return write_artifact( diff --git a/scripts/orchestration/plans.py b/scripts/orchestration/plans.py index 9da42a1..c115729 100644 --- a/scripts/orchestration/plans.py +++ b/scripts/orchestration/plans.py @@ -35,7 +35,7 @@ PLAN_QUALIFICATION_STATUSES = {"draft", "verified", "superseded"} PLAN_QUALIFICATION_TRANSITIONS = { "draft": {"verified", "superseded"}, - "verified": {"superseded"}, + "verified": {"draft", "superseded"}, "superseded": set(), } PLANNED_STATUS = "planned" @@ -200,6 +200,27 @@ def _active_artifact( ) +def _active_artifact_or_none( + args: argparse.Namespace, family: str, identity: str, bindings: dict[str, str] +) -> dict[str, object] | None: + path = canonical_artifact_path( + _plan_policy(family), _plan_anchors(args), identity=identity, + state="active", bindings=bindings, + ) + if not path.is_file(): + return None + return _active_artifact(args, family, identity, bindings) + + +def _require_draft_plan(args: argparse.Namespace, plan_id: str) -> dict[str, object]: + plan = _active_root_plan(args, plan_id) + if plan.get("status") != "draft": + raise SystemExit( + f"Plan content can be created or updated only while the plan is draft: {plan_id}" + ) + return plan + + @@ -232,6 +253,20 @@ def cmd_write_plan(args: argparse.Namespace) -> None: source_spec_id = str(getattr(args, "source_spec_id", "") or "") if not source_spec_id: raise SystemExit("Root plan requires --source-spec-id") + bindings = {"source_spec": source_spec_id} + existing = _active_artifact_or_none(args, "root-plan", pid, bindings) + if existing is not None: + if existing.get("status") == "superseded": + raise SystemExit("Superseded root plan content cannot be changed") + if existing.get("source_spec_id") != source_spec_id: + raise SystemExit("Root plan update cannot change source specification binding") + if args.status != "draft": + raise SystemExit("Root plan content updates must return the plan to draft") + created = str(existing["date_created"]) + else: + if _identity_collision(args, "root-plan", pid, bindings=bindings): + raise SystemExit(f"Root plan canonical identity collision: {pid}") + created = now_date() today = now_date() data = { **semantic, @@ -244,10 +279,9 @@ def cmd_write_plan(args: argparse.Namespace) -> None: "version": args.version, "source_spec_id": source_spec_id, "status": args.status, - "date_created": today, + "date_created": created, "last_updated": today, } - bindings = {"source_spec": source_spec_id} _validate_candidate("root-plan", data, bindings) source = read_artifact( CATALOG_PATH, "specification", _plan_anchors(args), @@ -255,8 +289,6 @@ def cmd_write_plan(args: argparse.Namespace) -> None: ) if source["data"].get("status") != "verified": raise SystemExit("Root plan source specification must be active and verified") - if _identity_collision(args, "root-plan", pid, bindings=bindings): - raise SystemExit(f"Root plan canonical identity collision: {pid}") result = write_artifact( CATALOG_PATH, "root-plan", _plan_anchors(args), data, state="active", bindings=bindings, @@ -398,18 +430,21 @@ def cmd_write_phase(args: argparse.Namespace) -> None: semantic = _semantic_yaml( Path(args.content_file), PHASE_STRUCTURAL_INPUT_FIELDS, "Phase" ) + _require_draft_plan(args, str(args.plan_id)) + bindings = {"plan": args.plan_id} + existing = _active_artifact_or_none(args, "phase", args.phase_id, bindings) + if existing is None and _identity_collision(args, "phase", args.phase_id, bindings=bindings): + raise SystemExit(f"Phase canonical identity collision: {args.phase_id}") today = now_date() data = { **semantic, "artifact_type": "phase", "schema_version": 1, "id": args.phase_id, "plan_id": args.plan_id, "name": args.title, - "status": PLANNED_STATUS, "date_created": today, "last_updated": today, + "status": PLANNED_STATUS, + "date_created": str(existing["date_created"]) if existing else today, + "last_updated": today, } - bindings = {"plan": args.plan_id} _validate_candidate("phase", data, bindings) - _active_root_plan(args, str(args.plan_id)) - if _identity_collision(args, "phase", args.phase_id, bindings=bindings): - raise SystemExit(f"Phase canonical identity collision: {args.phase_id}") result = write_artifact( CATALOG_PATH, "phase", _plan_anchors(args), data, state="active", bindings=bindings, @@ -424,17 +459,21 @@ def cmd_write_task(args: argparse.Namespace) -> None: Path(args.content_file), TASK_STRUCTURAL_INPUT_FIELDS, "Task" ) source_obligation_records(semantic, label="Task") + _require_draft_plan(args, str(args.plan_id)) + bindings = {"plan": args.plan_id, "phase": args.phase_id} + existing = _active_artifact_or_none(args, "task", args.task_id, bindings) + if existing is None and _identity_collision(args, "task", args.task_id, bindings=bindings): + raise SystemExit(f"Task canonical identity collision: {args.task_id}") today = now_date() data = { **semantic, "artifact_type": "task", "schema_version": 2, "id": args.task_id, "plan_id": args.plan_id, "phase_id": args.phase_id, "name": args.title, "status": PLANNED_STATUS, - "date_created": today, "last_updated": today, + "date_created": str(existing["date_created"]) if existing else today, + "last_updated": today, } - bindings = {"plan": args.plan_id, "phase": args.phase_id} _validate_candidate("task", data, bindings) - _active_root_plan(args, str(args.plan_id)) try: read_artifact( CATALOG_PATH, @@ -448,8 +487,6 @@ def cmd_write_task(args: argparse.Namespace) -> None: raise SystemExit( f"Task parent phase is not canonical for plan {args.plan_id}: {args.phase_id}" ) from error - if _identity_collision(args, "task", args.task_id, bindings=bindings): - raise SystemExit(f"Task canonical identity collision: {args.task_id}") result = write_artifact( CATALOG_PATH, "task", _plan_anchors(args), data, state="active", bindings=bindings, @@ -461,7 +498,7 @@ def cmd_finalize_reviewed_plan(args: argparse.Namespace) -> None: """Mechanically archive one exact accepted current plan and release bindings.""" from artifact_store import transition_artifact - from review_runtime import CURRENT_CATALOG + from review_runtime import CURRENT_CATALOG, validate_final_workflow_chain anchors = _plan_anchors(args) review = read_artifact( @@ -488,103 +525,13 @@ def cmd_finalize_reviewed_plan(args: argparse.Namespace) -> None: CATALOG_PATH, "root-plan", anchors, identity=str(args.plan_id), state="active", bindings=plan_bindings, ) - plan_ref = data.get("plan_identity") - if ( - not isinstance(plan_ref, dict) - or plan_ref.get("id") != args.plan_id - or plan_ref != canonical_plan_tree_identity( - resolve_workspace_root(args), str(args.plan_id), state="active" - ) - or data.get("specification_id") != plan_record["data"].get("source_spec_id") - ): - raise SystemExit("Final workflow review plan/specification identity is stale") - - task_rows = [row for row in _index_rows(args, "task") if row.get("plan_id") == args.plan_id] - task_ids = {str(row["id"]) for row in task_rows} - review_required_by_task: dict[str, bool] = {} - for row in task_rows: - task_id = str(row["id"]) - task_record = read_artifact( - CATALOG_PATH, - "task", - anchors, - identity=task_id, - state="active", - bindings=_family_bindings("task", row), - ) - acceptance_review = task_record["data"].get("acceptance_review") - required = acceptance_review.get("required") if isinstance(acceptance_review, dict) else None - if type(required) is not bool: - raise SystemExit(f"Finalization task acceptance-review contract is invalid: {task_id}") - review_required_by_task[task_id] = required - accepted_refs = data.get("accepted_results") - if not isinstance(accepted_refs, list) or {str(ref.get("task_id")) for ref in accepted_refs if isinstance(ref, dict)} != task_ids: - raise SystemExit("Final workflow review does not reference every planned task exactly once") - if len(accepted_refs) != len(task_ids): - raise SystemExit("Final workflow review accepted-result coverage is duplicated") - coverage = data.get("coverage") - if not isinstance(coverage, dict) or coverage != {"planned": len(task_ids), "accepted": len(task_ids), "missing": []}: - raise SystemExit("Final workflow review coverage does not match canonical tasks") - - accepted_records: list[tuple[dict[str, object], dict[str, str]]] = [] - executor_records: list[tuple[dict[str, object], dict[str, str], str]] = [] - declared_review_refs = { - (str(reference.get("id")), str(reference.get("sha256"))) - for reference in data.get("accepted_reviews", []) if isinstance(reference, dict) - } - expected_review_refs: set[tuple[str, str]] = set() - for reference in accepted_refs: - task_id = str(reference["task_id"]) - bindings = {"plan": str(args.plan_id), "task": task_id} - record = read_artifact( - CURRENT_CATALOG, "accepted-task-result", anchors, - identity=str(reference["id"]), state="active", bindings=bindings, - ) - if record["digest"] != reference.get("sha256") or record["data"].get("task_id") != reference["task_id"]: - raise SystemExit("Final workflow review accepted-result reference is stale") - accepted_data = record["data"] - implementation_ref = accepted_data.get("implementation_review") - if implementation_ref is None: - if review_required_by_task[task_id]: - raise SystemExit("Final workflow review omits a required implementation review") - elif isinstance(implementation_ref, dict): - implementation_identity = ( - str(implementation_ref.get("id")), - str(implementation_ref.get("sha256")), - ) - if implementation_identity not in declared_review_refs: - raise SystemExit("Final workflow review implementation-review reference is invalid") - expected_review_refs.add(implementation_identity) - else: - raise SystemExit("Final workflow review implementation-review reference is invalid") - executor_ref = accepted_data.get("executor_result") - executor_matches = [] - if isinstance(executor_ref, dict): - for state in _plan_policy("executor-result")["lifecycle"]["states"]: - try: - executor_matches.append(read_artifact( - CURRENT_CATALOG, "executor-result", anchors, - identity=str(executor_ref.get("id")), state=str(state), bindings=bindings, - )) - except (FileNotFoundError, SystemExit): - continue - if len(executor_matches) != 1 or executor_matches[0]["digest"] != executor_ref.get("sha256"): - raise SystemExit("Final workflow review accepted-result executor reference is stale") - executor_records.append((executor_matches[0], bindings, str(executor_matches[0]["state"]))) - accepted_records.append((record, bindings)) - - if declared_review_refs != expected_review_refs: - raise SystemExit("Final workflow review implementation-review coverage is not exact") - - review_records: list[dict[str, object]] = [] - for reference in data.get("accepted_reviews", []): - record = read_artifact( - CURRENT_CATALOG, "implementation-review", anchors, - identity=str(reference["id"]), state="active", bindings={"plan": str(args.plan_id)}, - ) - if record["digest"] != reference.get("sha256") or record["data"].get("verdict") != "accept": - raise SystemExit("Final workflow review implementation-review reference is stale") - review_records.append(record) + current_chain = validate_final_workflow_chain(args, data) + accepted_records = current_chain["accepted_records"] + executor_records = current_chain["executor_records"] + review_records = current_chain["review_records"] + task_rows = [ + row for row in _index_rows(args, "task") if row.get("plan_id") == args.plan_id + ] repository = data.get("repository_finalization") repositories = repository.get("repositories") if isinstance(repository, dict) else None diff --git a/scripts/orchestration/review_runtime.py b/scripts/orchestration/review_runtime.py index 6ce86d9..53bbe29 100644 --- a/scripts/orchestration/review_runtime.py +++ b/scripts/orchestration/review_runtime.py @@ -86,17 +86,39 @@ def _bindings(family: str, *, plan_id: str, task_id: str | None = None) -> dict[ return result -def _available(args: argparse.Namespace, family: str, bindings: Mapping[str, str]) -> None: +def _active_or_none( + args: argparse.Namespace, family: str, bindings: Mapping[str, str] +) -> dict[str, Any] | None: policy = _policy(family) + active = canonical_artifact_path( + policy, _anchors(args), identity=str(args.id), state="active", bindings=bindings + ) + if active.is_file(): + return read_artifact( + CURRENT_CATALOG, family, _anchors(args), identity=str(args.id), + state="active", bindings=bindings, + ) for state in policy["lifecycle"]["states"]: - if canonical_artifact_path(policy, _anchors(args), identity=str(args.id), state=str(state), bindings=bindings).exists(): + if state != "active" and canonical_artifact_path( + policy, _anchors(args), identity=str(args.id), state=str(state), bindings=bindings + ).exists(): raise SystemExit(f"{family} canonical identity collision: {args.id}") + return None def _write(args: argparse.Namespace, family: str, data: Mapping[str, Any], bindings: Mapping[str, str]) -> dict[str, Any]: from artifact_store import write_artifact - _available(args, family, bindings) + existing = _active_or_none(args, family, bindings) + if family == "implementation-review" and existing is not None: + existing_data = existing["data"] + if ( + existing_data.get("scope") != data.get("scope") + or existing_data.get("task_id") != getattr(args, "task_id", None) + ): + raise SystemExit( + "Implementation review update cannot change scope or task binding" + ) today = now_date() schema_id = str(_policy(family)["schema"]["id"]) try: @@ -106,7 +128,9 @@ def _write(args: argparse.Namespace, family: str, data: Mapping[str, Any], bindi document = { **dict(data), "artifact_type": family, "schema_version": schema_version, "id": str(args.id), "plan_id": str(args.plan_id), - "task_id": getattr(args, "task_id", None), "date_created": today, "last_updated": today, + "task_id": getattr(args, "task_id", None), + "date_created": str(existing["data"]["date_created"]) if existing else today, + "last_updated": today, } if family == "final-workflow-review": document.pop("task_id", None) @@ -175,8 +199,11 @@ def write_implementation_review(args: argparse.Namespace) -> dict[str, Any]: raise SystemExit(str(error)) from error data["target"] = checked["target"] data["target_sha256"] = checked["target"]["sha256"] - if data.get("scope") == "task" and not getattr(args, "task_id", None): + task_id = getattr(args, "task_id", None) + if data.get("scope") == "task" and not task_id: raise SystemExit("Task implementation review requires task binding") + if data.get("scope") == "integrated" and task_id: + raise SystemExit("Integrated implementation review cannot use task binding") return _write(args, "implementation-review", data, _bindings("implementation-review", plan_id=str(args.plan_id), task_id=getattr(args, "task_id", None))) @@ -203,8 +230,8 @@ def write_accepted_task_result(args: argparse.Namespace) -> dict[str, Any]: if not isinstance(review, dict): raise SystemExit("Accepted task result implementation review reference is invalid") reviewed = _reference(args, "implementation-review", str(review.get("id")), _bindings("implementation-review", plan_id=str(args.plan_id), task_id=str(args.task_id))) - if reviewed["digest"] != review.get("sha256") or reviewed["data"].get("verdict") != "accept": - raise SystemExit("Accepted task result requires an exact accept implementation review") + if reviewed["digest"] != review.get("sha256"): + raise SystemExit("Accepted task result requires the exact implementation review advice") if reviewed["data"].get("target_sha256") != product.get("sha256"): raise SystemExit("Accepted task result product identity does not match review target") data["product_sha256"] = product.get("sha256") @@ -216,30 +243,103 @@ def list_accepted_task_results(args: argparse.Namespace) -> list[dict[str, Any]] return _rows(args, "accepted-task-result") -def write_final_workflow_review(args: argparse.Namespace) -> dict[str, Any]: - data = _semantic_input(args, "final-workflow-review") - _validate_plan_authority(args, data) - candidate = data.get("candidate_identity") - if not isinstance(candidate, dict) or not isinstance(candidate.get("sha256"), str): - raise SystemExit("Final workflow review requires exact candidate identity") - seen_tasks: set[str] = set() - for reference in data.get("accepted_results", []): - if not isinstance(reference, dict) or not str(reference.get("task_id") or ""): - raise SystemExit("Final workflow review contains invalid accepted-result reference") +def validate_final_workflow_chain( + args: argparse.Namespace, data: Mapping[str, Any] +) -> dict[str, Any]: + """Validate current mechanical references before storing or finalizing a decision.""" + + tree = _validate_plan_authority(args, data) + task_ids = set(tree["tasks"]) + accepted_refs = data.get("accepted_results") + if not isinstance(accepted_refs, list): + raise SystemExit("Final workflow review requires accepted task results") + referenced_task_ids = [ + str(reference.get("task_id")) + for reference in accepted_refs + if isinstance(reference, dict) + ] + if set(referenced_task_ids) != task_ids or len(referenced_task_ids) != len(task_ids): + raise SystemExit("Final workflow review must reference every planned task exactly once") + coverage = data.get("coverage") + expected_coverage = {"planned": len(task_ids), "accepted": len(task_ids), "missing": []} + if coverage != expected_coverage: + raise SystemExit("Final workflow review coverage does not match canonical tasks") + + declared_review_refs = { + (str(reference.get("id")), str(reference.get("sha256"))) + for reference in data.get("accepted_reviews", []) + if isinstance(reference, dict) + } + if len(declared_review_refs) != len(data.get("accepted_reviews", [])): + raise SystemExit("Final workflow review contains invalid or duplicate review references") + + accepted_records: list[tuple[dict[str, Any], dict[str, str]]] = [] + executor_records: list[tuple[dict[str, Any], dict[str, str], str]] = [] + expected_review_refs: set[tuple[str, str]] = set() + for reference in accepted_refs: + if not isinstance(reference, dict): + raise SystemExit("Final workflow review contains an invalid accepted-result reference") task_id = str(reference["task_id"]) - if task_id in seen_tasks: - raise SystemExit("Final workflow review duplicates an accepted task") - seen_tasks.add(task_id) + bindings = {"plan": str(args.plan_id), "task": task_id} accepted = _reference( - args, "accepted-task-result", str(reference.get("id")), - {"plan": str(args.plan_id), "task": task_id}, + args, "accepted-task-result", str(reference.get("id")), bindings ) if accepted["digest"] != reference.get("sha256"): raise SystemExit("Final workflow review accepted-result digest mismatch") - for reference in data.get("accepted_reviews", []): - reviewed = _reference(args, "implementation-review", str(reference.get("id")), {"plan": str(args.plan_id)}) - if reviewed["digest"] != reference.get("sha256") or reviewed["data"].get("verdict") != "accept": - raise SystemExit("Final workflow review requires exact accepted implementation reviews") + accepted_data = accepted["data"] + + executor_ref = accepted_data.get("executor_result") + if not isinstance(executor_ref, dict): + raise SystemExit("Accepted task result executor reference is invalid") + executor = _reference(args, "executor-result", str(executor_ref.get("id")), bindings) + if executor["digest"] != executor_ref.get("sha256"): + raise SystemExit("Accepted task result executor reference is stale") + executor_records.append((executor, bindings, str(executor["state"]))) + + task = tree["tasks"][task_id] + acceptance_review = task.get("acceptance_review") + required = acceptance_review.get("required") if isinstance(acceptance_review, dict) else None + if type(required) is not bool: + raise SystemExit(f"Canonical task acceptance_review.required is invalid: {task_id}") + review_ref = accepted_data.get("implementation_review") + if review_ref is None: + if required: + raise SystemExit("Final workflow review omits a required implementation review") + elif isinstance(review_ref, dict): + review_identity = (str(review_ref.get("id")), str(review_ref.get("sha256"))) + reviewed = _reference( + args, "implementation-review", review_identity[0], {"plan": str(args.plan_id)} + ) + if reviewed["digest"] != review_identity[1]: + raise SystemExit("Accepted task result implementation review is stale") + product = accepted_data.get("product_identity") + if not isinstance(product, dict) or reviewed["data"].get("target_sha256") != product.get("sha256"): + raise SystemExit("Accepted task result product identity does not match review target") + expected_review_refs.add(review_identity) + else: + raise SystemExit("Accepted task result implementation review reference is invalid") + accepted_records.append((accepted, bindings)) + + if declared_review_refs != expected_review_refs: + raise SystemExit("Final workflow review implementation-review coverage is not exact") + review_records = [ + _reference(args, "implementation-review", identity, {"plan": str(args.plan_id)}) + for identity, _digest in sorted(expected_review_refs) + ] + return { + "tree": tree, + "accepted_records": accepted_records, + "executor_records": executor_records, + "review_records": review_records, + } + + +def write_final_workflow_review(args: argparse.Namespace) -> dict[str, Any]: + data = _semantic_input(args, "final-workflow-review") + candidate = data.get("candidate_identity") + if not isinstance(candidate, dict) or not isinstance(candidate.get("sha256"), str): + raise SystemExit("Final workflow review requires exact candidate identity") + validate_final_workflow_chain(args, data) data["target_sha256"] = candidate["sha256"] return _write(args, "final-workflow-review", data, _bindings("final-workflow-review", plan_id=str(args.plan_id))) diff --git a/scripts/orchestration/specs.py b/scripts/orchestration/specs.py index 074e1e8..e7c44d5 100644 --- a/scripts/orchestration/specs.py +++ b/scripts/orchestration/specs.py @@ -13,6 +13,11 @@ CATALOG_PATH = Path(__file__).resolve().parents[2] / "references/assets/orchestration/contract/artifact-family-catalog-v3.yaml" FAMILY = "specification" QUALIFICATION_STATUSES = {"draft", "verified", "superseded"} +QUALIFICATION_TRANSITIONS = { + "draft": {"verified", "superseded"}, + "verified": {"draft", "superseded"}, + "superseded": set(), +} STRUCTURAL_INPUT_FIELDS = { "artifact_type", "schema_version", "id", "title", "status", "date_created", "last_updated", "purpose", "component", "version", @@ -88,13 +93,25 @@ def cmd_write_spec(args: argparse.Namespace) -> None: if args.status not in QUALIFICATION_STATUSES: raise SystemExit(f"Invalid spec qualification status: {args.status}") identity = args.id or _next_identity(args) - if _path(args, identity, "active").exists() or _path(args, identity, "archived").exists(): - raise SystemExit(f"Specification canonical identity collision: {identity}") semantic, body = _semantic_input(Path(args.content_file)) + active_path = _path(args, identity, "active") + archived_path = _path(args, identity, "archived") + if archived_path.exists(): + raise SystemExit(f"Specification canonical identity collision: {identity}") + existing = ( + read_artifact(CATALOG_PATH, FAMILY, _anchors(args), identity=identity, state="active") + if active_path.exists() + else None + ) + if existing is not None and args.status != "draft": + raise SystemExit("Specification content updates must return the specification to draft") + if existing is not None and existing["data"].get("status") == "superseded": + raise SystemExit("Superseded specification content cannot be changed") today = now_date() data = { **semantic, "artifact_type": FAMILY, "schema_version": 1, "id": identity, - "title": args.title, "status": args.status, "date_created": today, + "title": args.title, "status": args.status, + "date_created": str(existing["data"]["date_created"]) if existing else today, "last_updated": today, "purpose": args.purpose, "component": args.component, "version": args.version, } @@ -150,6 +167,10 @@ def cmd_set_spec_status(args: argparse.Namespace) -> None: if data["status"] == args.status: print(args.id) return + if args.status not in QUALIFICATION_TRANSITIONS.get(str(data["status"]), set()): + raise SystemExit( + f"Invalid specification qualification transition: {data['status']} -> {args.status}" + ) data["status"] = args.status data["last_updated"] = now_date() write_artifact(CATALOG_PATH, FAMILY, _anchors(args), data, state="active", body=str(current["body"])) diff --git a/skills/orch-create-handoff/SKILL.md b/skills/orch-create-handoff/SKILL.md index 5a0d4fe..551aef9 100644 --- a/skills/orch-create-handoff/SKILL.md +++ b/skills/orch-create-handoff/SKILL.md @@ -11,9 +11,9 @@ Use this skill after task execution or when a current executor result must be re 1. Confirm the exact plan/task bindings, identity, lifecycle operation, and canonical catalog location. 2. Summarize only factual implemented scope, changed paths, validation observations, unresolved product blockers, task fit, repository/CodeGraph observations, delegation provenance, and task-local knowledge disposition. -3. Validate the full semantic input, bindings, identity, canonical path, collision state, and requested transition before mutation. -4. Create one immutable YAML artifact atomically. Treat the derived index as a regenerable projection. -5. After creation, perform only lightweight integrity checks. If index rebuild fails after the artifact write, report the partial effect truthfully. +3. Validate the full semantic input, bindings, identity, canonical path, and requested transition before mutation. +4. Create or repair the active YAML artifact atomically at the same canonical identity. Allocate a new identity only for a genuinely distinct executor result; a transitioned result is not rewritten. Treat the derived index as a regenerable projection. +5. After the write, perform only lightweight integrity checks. If index rebuild fails after the artifact write, report the partial effect truthfully. A malformed or missing executor result blocks only continuation that requires it. An independent reviewer may still judge an exact reviewable product candidate from the verified specification/plan, frozen implementation identity, and focused observations. @@ -21,6 +21,6 @@ A malformed or missing executor result blocks only continuation that requires it - [ ] The artifact is at the catalog-selected path and bound to the exact plan/task. - [ ] The content is factual and contains no verdict, acceptance, final-audit, repair-advice, or knowledge-write fields. -- [ ] Structural validation and collision checks completed before mutation. +- [ ] Necessary structural validation completed before mutation; active repair retained the canonical identity and did not create a revision copy. - [ ] Post-write work was limited to integrity and index projection checks. - [ ] Any partial effect or separate supporting-state defect is reported without changing product meaning. diff --git a/skills/orch-create-implementation-plan/SKILL.md b/skills/orch-create-implementation-plan/SKILL.md index 4f9319f..dfdb52f 100644 --- a/skills/orch-create-implementation-plan/SKILL.md +++ b/skills/orch-create-implementation-plan/SKILL.md @@ -29,7 +29,7 @@ The root plan binds `source_spec_id`. Every phase binds `plan_id`. Every task bi 8. When execution proves a task materially under-decomposed, return to the plan and reslice only the affected region around the newly evidenced seam. Preserve the original binding, baseline, and accepted unaffected regions; do not repeatedly enlarge the task. 9. Use the canonical semantic plan projection for review identity and freshness; status-only or append-only evidence changes do not require plan review or reslicing, while authority, scope, dependency, acceptance, decomposition, or validation-allocation changes do. 10. Before semantic review, invoke canonical static task admission for every task through the task compiler. Treat schema, family, canonical placement, parent binding, dependency, scope, authority-alias, and validation-shape results as structural facts only. Do not duplicate its predicates in the planner or let it decide semantic completeness. -11. Give a distinct reviewer the verified specification, the concrete canonical plan tree, and bounded current source evidence. The reviewer maps every accepted requirement, constraint, interface, acceptance criterion, validation target, accepted `DEC-*`, and resolved stable open-question outcome to production ownership and capable validation, then issues `accept`, `repair`, or `blocked` directly. Tests, doctors, indexes, receipts, handoffs, and evidence volume do not issue this semantic verdict. Repair findings at the first owning plan/phase/task layer and re-review the exact repaired tree once. +11. Give a distinct reviewer the verified specification, the complete canonical plan tree, and bounded current source evidence. The reviewer maps every accepted requirement, constraint, interface, acceptance criterion, validation target, accepted `DEC-*`, and resolved stable open-question outcome to production ownership and capable validation, then provides advisory `accept`, `repair`, or `blocked` findings. The controller/orchestrator assesses that advice and owns plan qualification, repair routing, blocking, and continuation. Tests, doctors, indexes, receipts, handoffs, evidence volume, and reviewer advice alone do not issue the qualification decision. Repair findings the controller accepts at the first owning plan/phase/task layer using the same canonical identities. After every repair, review the complete current tree again; do not limit review to the delta or inherit semantic acceptance from an earlier version. 12. Keep planning qualification (`draft`, `verified`, `superseded`) separate from execution state and finalization. Phase/task execution states, handoff/review receipt gates, plan completion, and archive/finalization belong to the downstream execution/review stage. Current plan and specification revisions do not consume post-execution review rounds. @@ -119,9 +119,10 @@ Follow `orch-orchestration-boundary`. Do not read durable knowledge directly dur Before returning a plan candidate, confirm from the stored YAML and compiler output that: - the root plan binds exactly one canonical active verified specification and every phase/task has exact canonical parent bindings; +- repairs updated the current canonical identities in place, the root returned to `draft` before child-content changes, and no intermediate repair copies were retained; - every accepted specification obligation has explicit phase/task ownership and validation IDs where validation-bearing; - every authoritative production path has one production owner, exact files/symbols, dependencies, steps, methodology, rules/skills, capable oracle, and measurable completion criteria; - every phase is justified by an actual barrier or convergence boundary, or the tree uses one explicit default phase; - static admission passes without legacy Markdown, fallback filenames, broad scans, or a second persisted combined index; -- a distinct reviewer directly judged coverage, decomposition, ownership, dependencies, validation, authority, scope, and executability and the stored qualification does not exceed that verdict; +- a distinct reviewer directly assessed the complete current tree for coverage, decomposition, ownership, dependencies, validation, authority, scope, and executability, and the controller/orchestrator evaluated that advice; no delta-only, inherited, or automatic reviewer verdict qualifies the stored content; - no task/phase completion, handoff review, receipt, finalization, or archive semantics were added during planning. diff --git a/skills/orch-create-specification/SKILL.md b/skills/orch-create-specification/SKILL.md index ad022d4..52432c3 100644 --- a/skills/orch-create-specification/SKILL.md +++ b/skills/orch-create-specification/SKILL.md @@ -33,11 +33,11 @@ Use `existing` for small/manual work, `preferred` for autonomous multi-task work Specification repair remains pre-execution authoring: plan and specification revisions do not consume post-execution review rounds. A residual specification created for forced closure preserves unresolved claims and clean source-baseline identities; it does not reopen the exhausted flow or authorize another review round. -11. Materialize the current artifact through `scripts/orch.py write-spec`. Supply semantic front matter and the human-readable body; the specification family schema owns structural fields, canonical `.work-bundle/orchestration/spec/active/.spec.md` location, parsing, atomic writes, and the derived index. Do not select a filename or copy a legacy specification path. +11. Materialize the current artifact through `scripts/orch.py write-spec`. Supply semantic front matter and the human-readable body; the specification family schema owns structural fields, canonical `.work-bundle/orchestration/spec/active/.spec.md` location, parsing, atomic writes, and the derived index. Repair the active specification at the same canonical identity and return it to `draft`; allocate a new identity only for a genuinely distinct specification, never for an intermediate review revision. Do not select a filename or copy a legacy specification path. ## Independent semantic review -Before marking the specification `verified`, give the exact candidate, original user purpose, accepted authority, and current workspace evidence to a distinct reviewer agent. The reviewer directly decides whether the specification: +Before marking the specification `verified`, give the exact candidate, original user purpose, accepted authority, and current workspace evidence to a distinct reviewer agent. The reviewer provides an advisory assessment of whether the specification: - matches the user purpose and accepted authority; - covers all requirements, constraints, interfaces, acceptance criteria, validation targets, and open questions; @@ -45,7 +45,7 @@ Before marking the specification `verified`, give the exact candidate, original - uses the current specification family and workspace-root anchor; and - stays within scope without absorbing later implementation stages. -Repair concrete findings at their first owning layer, then ask the reviewer to check the repaired candidate. Supporting evidence files do not issue the semantic verdict. Missing ceremony, receipts, indexes, handoffs, or perfect evidence files is not a semantic rejection unless the candidate becomes ambiguous, unsafe, unreadable, or impossible to review. +The controller/orchestrator assesses that advice and owns qualification, repair routing, blocking, and continuation; reviewer advice is neither an automatic veto nor automatic acceptance. Repair findings the controller accepts at their first owning layer, then ask the reviewer to check the complete repaired candidate against the original purpose and all accepted authority. Do not narrow review to the changed passages or inherit acceptance from an earlier version. Supporting evidence files do not issue the semantic decision. Missing ceremony, receipts, indexes, handoffs, or perfect evidence files is not a semantic rejection unless the candidate becomes ambiguous, unsafe, unreadable, or impossible to review. ## Semantic convergence @@ -97,5 +97,6 @@ Follow `orch-orchestration-boundary`. - [ ] The specification preserves the user purpose and only accepted authority shapes requirements. - [ ] Requirements, constraints, interfaces, acceptance criteria, validation targets, conflicts, scope boundaries, and open questions are complete and mutually consistent. - [ ] The artifact uses the canonical `.spec.md` family path and contains no caller-authored structural override or filename. -- [ ] A distinct reviewer judged the concrete semantics directly; scripts and supporting state supplied structural evidence only. +- [ ] Repairs retained the canonical specification identity, returned its qualification to `draft`, and did not create intermediate revision copies. +- [ ] A distinct reviewer assessed the complete current semantics directly, and the controller/orchestrator evaluated that advice before qualification; no delta-only, inherited, or automatic reviewer verdict qualified the repaired specification. - [ ] The quality gate, semantic loop, execution-workspace policy, and Knowledge Base Update disposition agree. diff --git a/skills/orch-execute-plan/SKILL.md b/skills/orch-execute-plan/SKILL.md index 4089cb0..1a7e1f3 100644 --- a/skills/orch-execute-plan/SKILL.md +++ b/skills/orch-execute-plan/SKILL.md @@ -13,9 +13,9 @@ Execute only the task authority supplied by the controller. Use its verified spe 2. Follow the task methodology. For behavior changes, use GROUND → RED → GREEN → REFACTOR and retain focused observations that can disprove the claim. 3. Keep each worker inside its existing task ownership. A worker reports source changes and factual observations; it does not accept the product. 4. Freeze the resulting commit or worktree candidate with a path-sorted changed-path manifest. -5. Write one canonical `executor-result-v1` with implemented scope, changed paths, focused observations, unresolved product blockers, task fit, repository/CodeGraph facts, delegation provenance, and knowledge disposition. -6. When review is required, send the exact candidate, verified specification and plan, every obligation, and focused observations to a distinct reviewer. Supporting-state defects are routed separately unless the product itself is ambiguous, unsafe, inaccessible, or impossible to review. -7. On `repair`, resume the owning task and review the new exact candidate. On `blocked`, report the product blocker. On `accept`, let the controller create the compact accepted task result. +5. Write one canonical `executor-result-v1` with implemented scope, changed paths, focused observations, unresolved product blockers, task fit, repository/CodeGraph facts, delegation provenance, and knowledge disposition. Repair that active result in place instead of retaining intermediate versions. +6. When review is required, send the exact candidate, verified specification and plan, every obligation, and focused observations to a distinct reviewer for advisory findings. Supporting-state defects are routed separately unless the product itself is ambiguous, unsafe, inaccessible, or impossible to review. +7. The controller/orchestrator assesses the advice against the product and accepted authority, then decides whether to resume repair, block, or create the compact accepted task result. Do not treat the reviewer's recommendation as an automatic command. Executor results must not contain product verdicts, recommended repair strategy, final-audit conclusions, or knowledge-write authorization. Missing historical records, indexes, or handoffs do not become product findings. @@ -24,5 +24,5 @@ Executor results must not contain product verdicts, recommended repair strategy, - [ ] The executed task and repository/write scope match accepted authority. - [ ] The executor result is canonical, factual, schema-valid, and bound to the exact plan/task. - [ ] Focused observations are current and claim-relevant; green tests do not substitute for checking every obligation. -- [ ] A distinct reviewer, not the executor or controller, authored any product verdict. +- [ ] A distinct reviewer authored the independent advice, and the controller/orchestrator—not the reviewer or a script—owned the product decision. - [ ] No receipt, history replay, or supporting-state defect issued or changed a semantic verdict. diff --git a/skills/orch-review-plan/SKILL.md b/skills/orch-review-plan/SKILL.md index 048d7a4..e345304 100644 --- a/skills/orch-review-plan/SKILL.md +++ b/skills/orch-review-plan/SKILL.md @@ -9,22 +9,24 @@ Use direct product review for implementation correctness and one compact final w ## Implementation review -The reviewer must be distinct from the implementor. Compare the exact frozen commit or worktree candidate directly with the verified specification and canonical plan. Inspect every planned feature, acceptance obligation, edge/failure behavior, and claim-relevant focused observation. Passing tests cannot hide omitted behavior. +The reviewer must be distinct from the implementor. Compare the exact frozen commit or worktree candidate directly with the verified specification and canonical plan. Inspect every planned feature, acceptance obligation, edge/failure behavior, and claim-relevant focused observation. After a repair, repeat this complete product comparison against the current candidate; prior findings may guide inspection but cannot narrow it to the delta or carry forward acceptance. Passing tests cannot hide omitted behavior. -Issue `accept`, `repair`, or `blocked` from product correctness. Missing or defective indexes, historical handoffs, knowledge state, or controller ceremony are separate supporting-state defects unless they make the actual product ambiguous, unsafe, inaccessible, or impossible to review. Store the verdict and findings in one canonical `implementation-review-v1`. +Issue an advisory `accept`, `repair`, or `blocked` assessment from product correctness. Missing or defective indexes, historical handoffs, knowledge state, or controller ceremony are separate supporting-state defects unless they make the actual product ambiguous, unsafe, inaccessible, or impossible to review. Store the advice and findings in the current canonical `implementation-review-v2`; its schema field remains named `verdict`, but it is the reviewer's recommendation, not orchestration authority. Re-review after repair updates the same active review identity; it does not preserve intermediate verdict copies. ## Accepted continuation -After `accept`, the controller creates one `accepted-task-result-v1` that references the exact executor result, accepted implementation review when required, current validation outcomes, product identity, material defects, and knowledge disposition. Consumers reuse this compact decision; they do not reconstruct review history. +The controller/orchestrator assesses the exact review advice and findings against user purpose, accepted specification/plan authority, the product, and current observations. It then owns acceptance, repair routing, blocking, and continuation. When it accepts, it creates one `accepted-task-result-v1` that references the exact executor result and implementation review advice when required, plus current validation outcomes, product identity, material defects, and knowledge disposition. Repair the active record at the same identity. Consumers reuse this compact controller decision; they do not reconstruct review history. ## Final workflow review -A distinct final auditor checks only plan/task coverage, accepted implementation verdicts, relevant current test outcomes, unresolved material defects, knowledge disposition/return, repository finalization facts, and truthful archive readiness. Do not reread source for code quality or repeat implementation review. Write one `final-workflow-review-v1`; deterministic finalization validates canonical references, lifecycle state, clean baselines, archive destinations, indexes, and binding release without inventing or reinterpreting the verdict. +A distinct final auditor advises on plan/task coverage, controller-accepted implementation decisions, relevant current test outcomes, unresolved material defects, knowledge disposition/return, repository finalization facts, and truthful archive readiness. Do not reread source for code quality or repeat implementation review. The controller/orchestrator assesses that advice and writes or repairs its `final-workflow-review-v1` closure decision at the same identity. Deterministic finalization validates canonical references, lifecycle state, clean baselines, archive destinations, indexes, and binding release without inventing or reinterpreting the controller decision. ## Self-check - [ ] The implementation verdict covers every specification and plan obligation against the exact candidate. -- [ ] The reviewer is distinct and findings identify the affected requirement and product boundary. +- [ ] A repaired candidate received a complete current-candidate review rather than delta-only inspection or inherited acceptance. +- [ ] Active executor, review, accepted-result, and final-review repairs retained their canonical identities instead of creating revision copies. +- [ ] The reviewer is distinct, findings identify the affected requirement and product boundary, and the controller/orchestrator explicitly assessed the advice rather than obeying it automatically. - [ ] Supporting-state defects were routed separately and did not veto otherwise correct reviewable work. - [ ] The final workflow review is compact and does not repeat code review or reconstruct history. -- [ ] Finalization carried the agent verdict and performed only mechanical checks and transitions. +- [ ] Finalization carried the controller/orchestrator decision and performed only mechanical checks and transitions. diff --git a/tests/test_orchestration_plans.py b/tests/test_orchestration_plans.py index 2370966..07a1c25 100644 --- a/tests/test_orchestration_plans.py +++ b/tests/test_orchestration_plans.py @@ -270,11 +270,75 @@ def test_structural_override_and_unverified_source_fail_before_mutation( assert not list((workspace / ".work-bundle/orchestration/plan/active").glob("*.plan.yaml")) -def test_collision_and_wrong_parent_preserve_existing_bytes(workspace: Path, tmp_path: Path) -> None: +def test_existing_plan_tree_is_updated_in_place_without_revision_copies( + workspace: Path, tmp_path: Path, +) -> None: + root_path, phase_path, task_path = _create_tree(workspace, tmp_path) + plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="verified")) + plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="draft")) + + plan_semantic = _plan_semantics() + plan_semantic["completion_criteria"] = ["The complete current plan is semantically accurate."] + plan_input = _write_yaml(tmp_path / "updated-plan.yaml", plan_semantic) + plans.cmd_write_plan(_args(workspace, content_file=str(plan_input), status="draft")) + + phase_semantic = _phase_semantics() + phase_semantic["completion_criteria"] = ["The complete current phase is semantically accurate."] + phase_input = _write_yaml(tmp_path / "updated-phase.yaml", phase_semantic) + plans.cmd_write_phase( + _args( + workspace, plan_id="plan-stage4", phase_id="phase-stage4", + title="Updated Stage 4 phase", content_file=str(phase_input), status="planned", + ) + ) + + task_semantic = _task_semantics() + task_semantic["completion_criteria"] = ["The complete current task is semantically accurate."] + task_input = _write_yaml(tmp_path / "updated-task.yaml", task_semantic) + plans.cmd_write_task( + _args( + workspace, plan_id="plan-stage4", phase_id="phase-stage4", + task_id="task-stage4", title="Updated Stage 4 task", + content_file=str(task_input), status="planned", + ) + ) + + root = workspace / ".work-bundle/orchestration/plan/active" + assert sorted(path.name for path in root.glob("*.plan.yaml")) == ["plan-stage4.plan.yaml"] + assert sorted(path.name for path in (root / "plan-stage4").glob("*.phase.yaml")) == ["phase-stage4.phase.yaml"] + assert sorted(path.name for path in (root / "plan-stage4/phase-stage4").glob("*.task.yaml")) == ["task-stage4.task.yaml"] + assert yaml.safe_load(root_path.read_text())["completion_criteria"] == [ + "The complete current plan is semantically accurate." + ] + assert yaml.safe_load(phase_path.read_text())["name"] == "Updated Stage 4 phase" + assert yaml.safe_load(task_path.read_text())["name"] == "Updated Stage 4 task" + + +def test_verified_plan_rejects_child_updates_until_returned_to_draft( + workspace: Path, tmp_path: Path, +) -> None: + _root_path, _phase_path, task_path = _create_tree(workspace, tmp_path) + plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="verified")) + before = task_path.read_bytes() + task_semantic = _task_semantics() + task_semantic["completion_criteria"] = ["Updated content"] + task_input = _write_yaml(tmp_path / "updated-task.yaml", task_semantic) + + with pytest.raises(SystemExit, match="draft"): + plans.cmd_write_task( + _args( + workspace, plan_id="plan-stage4", phase_id="phase-stage4", + task_id="task-stage4", title="Updated Stage 4 task", + content_file=str(task_input), status="planned", + ) + ) + + assert task_path.read_bytes() == before + + +def test_wrong_parent_preserves_existing_bytes(workspace: Path, tmp_path: Path) -> None: root_path, phase_path, task_path = _create_tree(workspace, tmp_path) before = {path: path.read_bytes() for path in (root_path, phase_path, task_path)} - with pytest.raises(SystemExit, match="collision"): - plans.cmd_write_plan(_args(workspace, content_file=str(tmp_path / "plan.yaml"))) wrong = _write_yaml(tmp_path / "wrong-task.yaml", _task_semantics()) with pytest.raises(SystemExit, match="parent|phase|canonical"): plans.cmd_write_task( @@ -345,13 +409,17 @@ def test_plan_qualification_transitions_are_monotonic( ) -> None: root_path, _phase_path, _task_path = _create_tree(workspace, tmp_path) plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="verified")) - with pytest.raises(SystemExit, match="Invalid plan qualification transition"): - plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="draft")) - assert yaml.safe_load(root_path.read_text())["status"] == "verified" + plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="draft")) + assert yaml.safe_load(root_path.read_text())["status"] == "draft" + plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="verified")) plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="superseded")) with pytest.raises(SystemExit, match="Invalid plan qualification transition"): plans.cmd_set_plan_status(_args(workspace, id="plan-stage4", status="verified")) + with pytest.raises(SystemExit, match="Superseded"): + plans.cmd_write_plan( + _args(workspace, content_file=str(tmp_path / "plan.yaml"), status="draft") + ) assert yaml.safe_load(root_path.read_text())["status"] == "superseded" diff --git a/tests/test_orchestration_skill_rule_boundary.py b/tests/test_orchestration_skill_rule_boundary.py index e375f99..e4d757f 100644 --- a/tests/test_orchestration_skill_rule_boundary.py +++ b/tests/test_orchestration_skill_rule_boundary.py @@ -51,7 +51,7 @@ def test_handoff_skill_owns_only_canonical_factual_continuation() -> None: "executor-result-v1", "exact plan/task bindings", "Validate the full semantic input", - "immutable YAML artifact", + "repair the active YAML artifact atomically at the same canonical identity", "derived index", "lightweight integrity checks", "partial effect", @@ -65,7 +65,7 @@ def test_review_skill_separates_product_review_from_compact_final_audit() -> Non for token in ( "exact frozen commit or worktree candidate", "Passing tests cannot hide omitted behavior", - "implementation-review-v1", + "implementation-review-v2", "accepted-task-result-v1", "final-workflow-review-v1", "Do not reread source for code quality or repeat implementation review", @@ -86,7 +86,7 @@ def test_active_rules_keep_semantic_judgment_agent_owned_and_non_recursive() -> "implementation review", "accepted task result", "final workflow review", - "product verdict", + "product decision", ): assert token in corpus for retired in ( diff --git a/tests/test_orchestration_specifications.py b/tests/test_orchestration_specifications.py index 0c1b466..ab7ef32 100644 --- a/tests/test_orchestration_specifications.py +++ b/tests/test_orchestration_specifications.py @@ -156,7 +156,7 @@ def test_legacy_markdown_is_ignored_and_only_canonical_identity_lookup_is_suppor _resolve_spec_paths(workspace, {}, {"source_spec_id": "spec-legacy"}) -def test_structural_override_collision_and_filename_override_fail_before_mutation( +def test_structural_override_update_and_filename_override_fail_before_mutation( workspace: Path, tmp_path: Path, ) -> None: content = tmp_path / "content.md" @@ -170,10 +170,53 @@ def test_structural_override_collision_and_filename_override_fail_before_mutatio with pytest.raises(SystemExit, match="filename override"): specs.cmd_write_spec(_args(workspace, content, filename="custom.md")) specs.cmd_write_spec(args) - before = (workspace / ".work-bundle/orchestration/spec/index.jsonl").read_bytes() + active = workspace / ".work-bundle/orchestration/spec/active/spec-20990101-001a.spec.md" + created = read_artifact( + CATALOG, "specification", {"workspace_root": workspace}, + identity="spec-20990101-001a", state="active", + )["data"]["date_created"] + + specs.cmd_set_spec_status(_args(workspace, id=args.id, status="verified")) + _content( + content, + body=( + "# Repaired semantic specification\n\n" + "- **REQ-001A:** Preserve the suffixed requirement.\n" + "- **REQ-002:** Cover the omitted behavior.\n" + ), + ) + specs.cmd_write_spec(args) + + stored = read_artifact( + CATALOG, "specification", {"workspace_root": workspace}, + identity="spec-20990101-001a", state="active", + ) + assert stored["data"]["status"] == "draft" + assert stored["data"]["date_created"] == created + assert "REQ-002" in stored["body"] + assert [path.name for path in active.parent.glob("*.spec.md")] == [active.name] + + +def test_specification_update_rejects_non_draft_and_archived_identity( + workspace: Path, tmp_path: Path, +) -> None: + content = tmp_path / "content.md" + _content(content) + args = _args(workspace, content) + specs.cmd_write_spec(args) + + with pytest.raises(SystemExit, match="updates must return.*draft"): + specs.cmd_write_spec(_args(workspace, content, status="verified")) + + specs.cmd_set_spec_status(_args(workspace, id=args.id, status="superseded")) + with pytest.raises(SystemExit, match="Superseded"): + specs.cmd_write_spec(args) + with pytest.raises(SystemExit, match="Invalid specification qualification transition"): + specs.cmd_set_spec_status(_args(workspace, id=args.id, status="draft")) + + specs.cmd_set_spec_status(_args(workspace, id=args.id, status="archived")) with pytest.raises(SystemExit, match="collision"): specs.cmd_write_spec(args) - assert (workspace / ".work-bundle/orchestration/spec/index.jsonl").read_bytes() == before def test_shared_front_matter_mutation_remains_available_to_plan_consumers( @@ -223,7 +266,7 @@ def test_skill_contract_requires_direct_semantic_review_and_self_check() -> None "material conflicts", "scope", "## Self-check", - "Supporting evidence files do not issue the semantic verdict", + "Supporting evidence files do not issue the semantic decision", ]: assert term in skill assert "require_specification_review" not in skill diff --git a/tests/test_orchestration_stage5_current_path.py b/tests/test_orchestration_stage5_current_path.py index 27ca8d7..7f30a86 100644 --- a/tests/test_orchestration_stage5_current_path.py +++ b/tests/test_orchestration_stage5_current_path.py @@ -246,7 +246,7 @@ def test_executor_result_round_trip_inline_block_index_and_transition( assert stored["data"]["result_state"] == "implemented" -def test_executor_result_rejects_overrides_duplicates_and_review_verdict_before_mutation( +def test_executor_result_rejects_overrides_and_updates_active_identity_in_place( workspace: Path, tmp_path: Path, ) -> None: bad = _executor_semantics() @@ -273,10 +273,24 @@ def test_executor_result_rejects_overrides_duplicates_and_review_verdict_before_ content_file=str(content), ) handoffs.cmd_write_executor_result(args) - before = next(workspace.rglob("*.executor-result.yaml")).read_bytes() - with pytest.raises(SystemExit, match="collision"): - handoffs.cmd_write_executor_result(args) - assert next(workspace.rglob("*.executor-result.yaml")).read_bytes() == before + path = next(workspace.rglob("*.executor-result.yaml")) + created = yaml.safe_load(path.read_text(encoding="utf-8"))["date_created"] + repaired = _executor_semantics() + repaired["summary"] = "Implemented the corrected bounded task." + repaired_input = _write_yaml(tmp_path, "repaired.yaml", repaired) + handoffs.cmd_write_executor_result( + _args( + workspace, + id="result-stage5", + plan_id="plan-stage5", + task_id="task-stage5", + content_file=str(repaired_input), + ) + ) + stored = yaml.safe_load(path.read_text(encoding="utf-8")) + assert stored["date_created"] == created + assert stored["summary"] == "Implemented the corrected bounded task." + assert len(list(workspace.rglob("*.executor-result.yaml"))) == 1 def test_review_accepted_result_and_final_review_form_compact_current_chain( @@ -299,7 +313,7 @@ def test_review_accepted_result_and_final_review_form_compact_current_chain( candidate = _candidate(workspace) review_input = _write_yaml( tmp_path, "review.yaml", - _review_semantics(candidate, plan_identity=plan_identity), + _review_semantics(candidate, plan_identity=plan_identity, verdict="repair"), ) review_runtime.cmd_write_implementation_review( _args( @@ -374,10 +388,63 @@ def test_review_accepted_result_and_final_review_form_compact_current_chain( content_file=str(final_input), ) ) + + accepted_repair = yaml.safe_load(accepted_input.read_text(encoding="utf-8")) + accepted_repair["validation_outcomes"][0]["summary"] = "Focused current pass." + accepted_repair_input = _write_yaml(tmp_path, "accepted-repaired.yaml", accepted_repair) + review_runtime.cmd_write_accepted_task_result( + _args( + workspace, + id="accepted-stage5", + plan_id="plan-stage5", + task_id="task-stage5", + content_file=str(accepted_repair_input), + ) + ) + current_accepted_digest = hashlib.sha256(accepted_path.read_bytes()).hexdigest() + final_repair = yaml.safe_load(final_input.read_text(encoding="utf-8")) + final_repair["accepted_results"][0]["sha256"] = current_accepted_digest + final_repair["reasons"] = ["The current complete workflow is ready."] + final_repair_input = _write_yaml(tmp_path, "final-repaired.yaml", final_repair) + review_runtime.cmd_write_final_workflow_review( + _args( + workspace, + id="final-stage5", + plan_id="plan-stage5", + content_file=str(final_repair_input), + ) + ) + assert [row["id"] for row in review_runtime.list_implementation_reviews(_args(workspace))] == ["review-stage5"] + assert review_runtime.list_implementation_reviews(_args(workspace))[0]["verdict"] == "repair" assert [row["id"] for row in review_runtime.list_accepted_task_results(_args(workspace))] == ["accepted-stage5"] assert [row["id"] for row in review_runtime.list_final_workflow_reviews(_args(workspace))] == ["final-stage5"] + final_path = next(workspace.rglob("final-stage5.final-workflow-review.yaml")) + final_before = final_path.read_bytes() + changed_executor = _executor_semantics() + changed_executor["summary"] = "Changed after controller acceptance." + changed_executor_input = _write_yaml(tmp_path, "executor-changed.yaml", changed_executor) + handoffs.cmd_write_executor_result( + _args( + workspace, + id="result-stage5", + plan_id="plan-stage5", + task_id="task-stage5", + content_file=str(changed_executor_input), + ) + ) + with pytest.raises(SystemExit, match="executor reference is stale"): + review_runtime.cmd_write_final_workflow_review( + _args( + workspace, + id="final-stage5", + plan_id="plan-stage5", + content_file=str(final_repair_input), + ) + ) + assert final_path.read_bytes() == final_before + def test_review_verdict_remains_agent_authored_and_supporting_state_is_not_required( workspace: Path, tmp_path: Path, @@ -404,6 +471,78 @@ def test_review_verdict_remains_agent_authored_and_supporting_state_is_not_requi assert "receipt" not in row and "publication" not in row and "history" not in row +def test_implementation_review_updates_same_active_identity_after_full_rereview( + workspace: Path, tmp_path: Path, +) -> None: + plan_identity = _write_stage5_plan_tree(workspace) + candidate = _candidate(workspace) + repair_input = _write_yaml( + tmp_path, "repair-review.yaml", + _review_semantics(candidate, plan_identity=plan_identity, verdict="repair"), + ) + args = _args( + workspace, + id="review-current", + plan_id="plan-stage5", + task_id="task-stage5", + source_root=str(workspace), + content_file=str(repair_input), + ) + review_runtime.cmd_write_implementation_review(args) + path = next(workspace.rglob("review-current.implementation-review.yaml")) + created = yaml.safe_load(path.read_text(encoding="utf-8"))["date_created"] + before = path.read_bytes() + + integrated = _review_semantics(candidate, plan_identity=plan_identity, verdict="accept") + integrated["scope"] = "integrated" + integrated_input = _write_yaml(tmp_path, "integrated-review.yaml", integrated) + with pytest.raises(SystemExit, match="scope or task binding"): + review_runtime.cmd_write_implementation_review( + _args( + workspace, + id="review-current", + plan_id="plan-stage5", + source_root=str(workspace), + content_file=str(integrated_input), + ) + ) + assert path.read_bytes() == before + + with pytest.raises(SystemExit, match="Integrated.*task binding"): + review_runtime.cmd_write_implementation_review( + _args( + workspace, + id="review-integrated-invalid", + plan_id="plan-stage5", + task_id="task-stage5", + source_root=str(workspace), + content_file=str(integrated_input), + ) + ) + assert not list(workspace.rglob("review-integrated-invalid.implementation-review.yaml")) + + accepted = _review_semantics(candidate, plan_identity=plan_identity, verdict="accept") + accepted["reviewed_obligations"][0]["summary"] = ( + "The complete repaired candidate satisfies the obligation." + ) + accept_input = _write_yaml(tmp_path, "accept-review.yaml", accepted) + review_runtime.cmd_write_implementation_review( + _args( + workspace, + id="review-current", + plan_id="plan-stage5", + task_id="task-stage5", + source_root=str(workspace), + content_file=str(accept_input), + ) + ) + + stored = yaml.safe_load(path.read_text(encoding="utf-8")) + assert stored["date_created"] == created + assert stored["verdict"] == "accept" + assert len(list(workspace.rglob("review-current.implementation-review.yaml"))) == 1 + + def test_review_writer_rejects_stale_plan_tree_identity_before_mutation( workspace: Path, tmp_path: Path, ) -> None: diff --git a/tests/test_orchestration_workflow_contracts.py b/tests/test_orchestration_workflow_contracts.py index 50f2f8f..a7ea9f2 100644 --- a/tests/test_orchestration_workflow_contracts.py +++ b/tests/test_orchestration_workflow_contracts.py @@ -66,7 +66,7 @@ def test_current_workflow_defines_direct_review_and_compact_final_audit() -> Non workflow = read("references/assets/orchestration/workflow.md").lower() for family in ( "executor-result-v1", - "implementation-review-v1", + "implementation-review-v2", "accepted-task-result-v1", "final-workflow-review-v1", ): @@ -99,7 +99,7 @@ def test_current_orchestration_evals_cover_stage5_boundary() -> None: ).lower() for term in ( "executor-result-v1", - "implementation-review-v1", + "implementation-review-v2", "accepted-task-result-v1", "final-workflow-review-v1", "frozen worktree", diff --git a/tests/test_rule_contracts.py b/tests/test_rule_contracts.py index 8ac2f5e..cc00419 100644 --- a/tests/test_rule_contracts.py +++ b/tests/test_rule_contracts.py @@ -553,7 +553,8 @@ def test_orchestration_rules_define_closed_executor_result_and_direct_review_bou "distinct implementation reviewer", "exact frozen candidate", "canonical `accepted-task-result-v1`", - "one compact final workflow review", + "one compact final audit", + "controller/orchestrator", "Keep finalization mechanical", ): assert current in review @@ -583,9 +584,9 @@ def test_execution_and_review_skills_define_current_optional_direct_review_flow( for current in ( "exact frozen commit or worktree candidate", - "implementation-review-v1", - "accepted implementation review when required", - "one compact final workflow review", + "implementation-review-v2", + "implementation review advice when required", + "controller/orchestrator", "they do not reconstruct review history", "Do not reread source for code quality or repeat implementation review", ):