diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 212e883..d8788f8 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,7 +8,7 @@ "name": "dk-workflows", "source": "./", "description": "dk-workflows", - "version": "1.3.0" + "version": "1.4.0" } ] } diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 4ab16db..6b2c437 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "dk-workflows", - "version": "1.3.0", + "version": "1.4.0", "description": "Portable repository workflows, configurable reviewers, and guarded drafting.", "author": { "name": "Dmitry Kropachev" diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index 79e0c9e..dce0c9e 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "dk-workflows", - "version": "1.3.0", + "version": "1.4.0", "description": "Portable repository workflows, configurable reviewers, and guarded drafting.", "author": { "name": "Dmitry Kropachev" diff --git a/PROVENANCE.json b/PROVENANCE.json index 16116b1..f6ce814 100644 --- a/PROVENANCE.json +++ b/PROVENANCE.json @@ -72,6 +72,11 @@ "source": "dk-code-factory/dk-workflows", "version": "1.2.0", "description": "Incremental preparation, conclusion graph, immutable evidence lineage, refresh validation and regression coverage." + }, + { + "source": "dk-code-factory/dk-workflows", + "version": "1.4.0", + "description": "Fresh workflow metadata contracts: consistent plugin versions, revision-checked Decisions and publication ownership, engineering compatibility identity, durable captures and isolated checkouts, host guidance and regression coverage." } ] } diff --git a/README.md b/README.md index 2cd08ab..cf354b9 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # dk-workflows -Independent MIT plugin for Claude Code and Codex. Version 1.3.0. +Independent MIT plugin for Claude Code and Codex. Version 1.4.0. ## Install @@ -237,7 +237,7 @@ when the host removes the original installation during lookup. Entry failures without a usable runtime receipt stop work; offline release checks still return a retained runtime and token and remain nonblocking. -Preparation v2 retains stable conclusion dependencies and immutable evidence snapshots. +Preparation v3 retains stable conclusion dependencies and immutable evidence snapshots. `prepare --previous-preparation FILE` compares complete Git entries and governing sources, reuses proven unchanged conclusions, and batches affected conclusions into one reassessment. Identical and supported operational refreshes use zero model calls; @@ -248,10 +248,41 @@ Decisions synchronization and iteration budgets retain their existing gates. See Decisions can persist across independent runs by setting `DK_PR_DECISIONS_HOME` to an absolute private directory shared by those runs. It is the complete cache root: -`////pr-.json`, including fork PRs. +`/v3////pr-.json`, including fork PRs. When unset, the default remains `${XDG_DATA_HOME:-~/.local/share}/dk-pr-decisions`. Explicit stores are exclusive; invalid or repository-local paths fail without fallback. Hosts provide persistent storage and access. See the [Decisions storage contract](runtime/dk-pr-decisions/references/storage.md) for seeding, migration, locking and interruption recovery. Recovered choices require live context refresh and engineering preparation before dependent work. + + +Fresh workflows allocate durable private directories under +`${XDG_STATE_HOME:-~/.local/state}/dk-code-factory/workflows/`. Generated contexts, +isolated checkouts, manifests, evidence captures and conflict proposals belong there; +use the returned paths in handoffs. No automatic deletion policy applies. Preparation +gates read owned captures; arbitrary external evidence defaults to live checks, so +missing required rules or instructions still block. Live head/body, configuration +and local-choice checks remain mandatory. + +`preparation_id` fingerprints the entire immutable artifact. Runtime-computed +`engineering_id` fingerprints its normalized governing evidence, tree/base, scope, +Decisions, ordered reviewer descriptors, analysis, readiness and policy. Supported +operational counters/status/receipts, storage paths, refresh reports and verified +lineage do not invalidate engineering compatibility; unknown fields still govern. +Session v2 checkpoints, review-result v3 coverage, scope assessments and completion +verification v2 can survive an operational refresh with the same engineering ID. +Interrupted resume also requires the same head. Tree-equivalent cleanup retains its +separate guarded reconciliation path. Provenance keeps exact preparation references. + +New Decisions storage uses `dk-pr-decisions/v3/` under the private data home. Each +write checks a monotonic revision under the private-state lock; network/model work +runs outside it. A stale write returns API v2 `status: error` with +`error.code: metadata_conflict`, a retained proposal path and an explicit recovery +action. The stable CLI reports a workflow block. Publication intent is committed +before GitHub mutation; pending intent blocks other writers until its owner finishes +or `publish --reconcile` verifies the outcome without replaying the edit. A validated +legacy snapshot can be imported without changing its original file, including any +pending intent. Legacy active iteration runs require their retained runtime; a fresh +request after a terminal legacy run preserves history under the existing start rules. +There is no bulk migration of old runs, results or checkpoints. diff --git a/claude/skills/dk-pr-decisions/SKILL.md b/claude/skills/dk-pr-decisions/SKILL.md index 0119f85..43bef6f 100644 --- a/claude/skills/dk-pr-decisions/SKILL.md +++ b/claude/skills/dk-pr-decisions/SKILL.md @@ -10,3 +10,5 @@ Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then Read [the shared procedure](../../../runtime/dk-pr-decisions/procedure.md) and perform the requested workflow. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. On preparation refresh, select the active artifact with `prepare --previous-preparation FILE`; follow [the incremental preparation contract](../../../runtime/dk-pr-decisions/references/preparation.md). Preserve review budgets and exact session/coverage gates. + +Fresh workflows use preparation v3, session v2, review-result v3 and completion-verification v2. Preserve both preparation and engineering IDs. Use returned private workflow paths for all generated evidence and child handoffs. Operational refresh may preserve engineering identity; validate current evidence and retained reviewer bindings before reuse. Resume legacy active runs only through their retained runtime. A Decisions `metadata_conflict` blocks dependent work: read its retained proposal and explicitly reconcile; never merge or replay publication automatically. diff --git a/claude/skills/dk-pr-iter/SKILL.md b/claude/skills/dk-pr-iter/SKILL.md index d31163f..8c7e50d 100644 --- a/claude/skills/dk-pr-iter/SKILL.md +++ b/claude/skills/dk-pr-iter/SKILL.md @@ -10,3 +10,5 @@ Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then Read [the coordinator entry](../../../runtime/dk-pr-iter/procedure.md), then read each linked stage contract before acting in that stage. Give child agents only their assigned role contracts and pinned evidence paths. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. On preparation refresh, select the active artifact with `prepare --previous-preparation FILE`; follow [the incremental preparation contract](../../../runtime/dk-pr-decisions/references/preparation.md). Preserve review budgets and exact session/coverage gates. + +Fresh workflows use preparation v3, session v2, review-result v3 and completion-verification v2. Preserve both preparation and engineering IDs. Use returned private workflow paths for all generated evidence and child handoffs. Operational refresh may preserve engineering identity; validate current evidence and retained reviewer bindings before reuse. Resume legacy active runs only through their retained runtime. A Decisions `metadata_conflict` blocks dependent work: read its retained proposal and explicitly reconcile; never merge or replay publication automatically. diff --git a/claude/skills/dk-pr-review/SKILL.md b/claude/skills/dk-pr-review/SKILL.md index 48940cc..b3ccf99 100644 --- a/claude/skills/dk-pr-review/SKILL.md +++ b/claude/skills/dk-pr-review/SKILL.md @@ -10,3 +10,5 @@ Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then Read [the shared procedure](../../../runtime/dk-pr-review/procedure.md) and perform the requested workflow. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. On preparation refresh, select the active artifact with `prepare --previous-preparation FILE`; follow [the incremental preparation contract](../../../runtime/dk-pr-decisions/references/preparation.md). Preserve review budgets and exact session/coverage gates. + +Fresh workflows use preparation v3, session v2, review-result v3 and completion-verification v2. Preserve both preparation and engineering IDs. Use returned private workflow paths for all generated evidence and child handoffs. Operational refresh may preserve engineering identity; validate current evidence and retained reviewer bindings before reuse. Resume legacy active runs only through their retained runtime. A Decisions `metadata_conflict` blocks dependent work: read its retained proposal and explicitly reconcile; never merge or replay publication automatically. diff --git a/codex/skills/dk-pr-decisions/SKILL.md b/codex/skills/dk-pr-decisions/SKILL.md index 4b030b5..35bedd6 100644 --- a/codex/skills/dk-pr-decisions/SKILL.md +++ b/codex/skills/dk-pr-decisions/SKILL.md @@ -10,3 +10,5 @@ Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then Read [the shared procedure](../../../runtime/dk-pr-decisions/procedure.md) and perform the requested workflow. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. On preparation refresh, select the active artifact with `prepare --previous-preparation FILE`; follow [the incremental preparation contract](../../../runtime/dk-pr-decisions/references/preparation.md). Preserve review budgets and exact session/coverage gates. + +Fresh workflows use preparation v3, session v2, review-result v3 and completion-verification v2. Preserve both preparation and engineering IDs. Use returned private workflow paths for all generated evidence and child handoffs. Operational refresh may preserve engineering identity; validate current evidence and retained reviewer bindings before reuse. Resume legacy active runs only through their retained runtime. A Decisions `metadata_conflict` blocks dependent work: read its retained proposal and explicitly reconcile; never merge or replay publication automatically. diff --git a/codex/skills/dk-pr-iter/SKILL.md b/codex/skills/dk-pr-iter/SKILL.md index 0ba05be..155daa8 100644 --- a/codex/skills/dk-pr-iter/SKILL.md +++ b/codex/skills/dk-pr-iter/SKILL.md @@ -10,3 +10,5 @@ Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then Read [the coordinator entry](../../../runtime/dk-pr-iter/procedure.md), then read each linked stage contract before acting in that stage. Give child agents only their assigned role contracts and pinned evidence paths. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. On preparation refresh, select the active artifact with `prepare --previous-preparation FILE`; follow [the incremental preparation contract](../../../runtime/dk-pr-decisions/references/preparation.md). Preserve review budgets and exact session/coverage gates. + +Fresh workflows use preparation v3, session v2, review-result v3 and completion-verification v2. Preserve both preparation and engineering IDs. Use returned private workflow paths for all generated evidence and child handoffs. Operational refresh may preserve engineering identity; validate current evidence and retained reviewer bindings before reuse. Resume legacy active runs only through their retained runtime. A Decisions `metadata_conflict` blocks dependent work: read its retained proposal and explicitly reconcile; never merge or replay publication automatically. diff --git a/codex/skills/dk-pr-review/SKILL.md b/codex/skills/dk-pr-review/SKILL.md index 9bbc5db..aa10500 100644 --- a/codex/skills/dk-pr-review/SKILL.md +++ b/codex/skills/dk-pr-review/SKILL.md @@ -10,3 +10,5 @@ Before work, read [the lifecycle procedure](../../../common/lifecycle.md), then Read [the shared procedure](../../../runtime/dk-pr-review/procedure.md) and perform the requested workflow. Resolve paths beginning with `PLUGIN_ROOT/` against this plugin's root. Preserve the shared procedure's validation and publication guards. On preparation refresh, select the active artifact with `prepare --previous-preparation FILE`; follow [the incremental preparation contract](../../../runtime/dk-pr-decisions/references/preparation.md). Preserve review budgets and exact session/coverage gates. + +Fresh workflows use preparation v3, session v2, review-result v3 and completion-verification v2. Preserve both preparation and engineering IDs. Use returned private workflow paths for all generated evidence and child handoffs. Operational refresh may preserve engineering identity; validate current evidence and retained reviewer bindings before reuse. Resume legacy active runs only through their retained runtime. A Decisions `metadata_conflict` blocks dependent work: read its retained proposal and explicitly reconcile; never merge or replay publication automatically. diff --git a/conftest.py b/conftest.py index a6a7bb6..1378848 100644 --- a/conftest.py +++ b/conftest.py @@ -13,3 +13,8 @@ def run(argv, *args, **kwargs): raise AssertionError('Fixture attempted an unmocked model execution') return original(argv, *args, **kwargs) monkeypatch.setattr(subprocess, 'run', run) + + +@pytest.fixture(autouse=True) +def private_workflow_state(tmp_path_factory, monkeypatch): + monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path_factory.mktemp("workflow-state"))) diff --git a/runtime/dk-pr-decisions/references/api.md b/runtime/dk-pr-decisions/references/api.md index 01ced95..01593bf 100644 --- a/runtime/dk-pr-decisions/references/api.md +++ b/runtime/dk-pr-decisions/references/api.md @@ -55,8 +55,8 @@ All former top-level command results now live under `data`; publication results `prepare --input-manifest FILE --output-dir PRIVATE_DIR [--previous-preparation FILE] [--full-analysis]` delegates to `scripts/preparation.py` without a discussion scan or any publication. It preserves -this API v2 envelope; `data` contains `artifact`, `preparation_id`, `readiness` and a `refresh` report. -The artifact has its own `preparation_version: 2`; read +this API v2 envelope; `data` contains `artifact`, `preparation_id`, `engineering_id`, `readiness` and a `refresh` report. +The artifact has its own `preparation_version: 3`; read [the preparation contract](preparation.md) for complete evidence, semantic analysis, question retention, placement, proportionate tests, and downstream gates. `verified: true` retains its original provenance/freshness meaning. @@ -74,3 +74,24 @@ results retain `status: pending` with `publication.reason`, even if the pending array is empty. Dependent iteration stays `sync_pending` until live publication, context and preparation agree. Existing scan/list/context/add/replace/suggest commands and unified decision records retain their contracts. + + +## Revision conflicts and pending publication + +The API envelope remains v2. Fresh records use private `dk-pr-decisions/v3/` storage +and a monotonically increasing `revision`. Every commit compares the loaded revision +under the existing file lock. Network and model calls run outside the lock. A stale +writer receives `status: error`, `error.code: metadata_conflict`, +`error.proposal_path` and `error.next_action`. The proposal retains the attempted +update, expected/current revisions, record path and diagnostics. The stable CLI +classifies it as a workflow block. Read the proposal and current state, reconcile +any pending intent, and explicitly reapply the intended change against the current +revision. Never merge, retry or replay publication automatically. + +Publication commits its owning operation's intent before GitHub mutation. A pending +intent rejects competing writers, including refresh, additions and suggestions. +Its owner can finish; after interruption use `publish --reconcile` to verify applied +or unapplied state without replaying the mutation. Pending legacy intents remain +blockers after import. Import validates the selected legacy snapshot and leaves its +original file untouched. Existing interrupted workflows use their retained runtime; +there is no bulk migration of active runs or immutable evidence. diff --git a/runtime/dk-pr-decisions/references/preparation-contract.md b/runtime/dk-pr-decisions/references/preparation-contract.md index e4af341..5632ea0 100644 --- a/runtime/dk-pr-decisions/references/preparation-contract.md +++ b/runtime/dk-pr-decisions/references/preparation-contract.md @@ -113,7 +113,7 @@ remain significant. Exact supported exceptions: ## Stored artifact and verification -Immutable `preparation_version:2` artifacts retain `analysis`, graph/retired IDs, +Immutable `preparation_version:3` artifacts retain `analysis`, graph/retired IDs, input fingerprints, reviewer descriptors, readiness, PR/base/head/body/Decisions identity, parent path/ID/hash and migration provenance. Snapshots are fingerprinted and read-only outside the repo. Historical validation checks retained integrity and @@ -126,10 +126,27 @@ binding/identity changes, affected/reused conclusions, explanations and actual remain outside binding analysis. Tree-equivalent review reuse preserves original provenance only when existing scope/rules/Decisions/instruction/base checks pass. -Completion verification has `verification_version:1`, current `preparation_id`, +Completion verification has `verification_version:2`, originating `preparation_id`, +runtime-computed `engineering_id`, `pr_url`, `head_sha`, and `checks` for `behavior`, `placement`, `test_sufficiency`, `compatibility`, `complexity`. Each check contains boolean `satisfied`, `rationale`, and nonempty `evidence`: `{"path":"ABSOLUTE_PATH","sha256":"FILE_HASH"}`. Inspection reports, code and test results qualify; missing, false or stale evidence blocks completion. Both stages use the same current preparation, verification and mutable run record; an immutable history snapshot cannot replace live choices. + + +The complete artifact fingerprint is `preparation_id`; its runtime-computed +`engineering_id` is the compatibility key. Store both. The latter excludes supported +operational metadata, storage paths, reports and verified lineage, retaining unknown +fields as governing. Reviewer order is explicit in `reviewer_order`. Sources retain +`path`, `sha256`, immutable `snapshot`, and `mode` (`capture` or `live`). See the +preparation procedure for capture defaults and manifest `source_modes` overrides. +Never accept a supplied engineering ID without recomputation and current evidence +and binding validation. Completion verification v2 remains reusable across an +operational refresh with identical engineering identity and head; its preparation +reference remains unchanged. Use workflow-owned evidence captures for verification +reports intended to survive original export removal. Legacy verification v1 belongs +to the retained legacy runtime. Fresh artifacts never rewrite historical checkpoints. + +`recovery_manifest` and `recovery_context` retain verified path/hash references to runnable private recovery inputs. Captured sources record `origin_identity`; recovery manifests carry `source_origins` to preserve identity while relocating captures. These declared storage fields do not govern engineering compatibility. diff --git a/runtime/dk-pr-decisions/references/preparation.md b/runtime/dk-pr-decisions/references/preparation.md index daab9c3..75d45a6 100644 --- a/runtime/dk-pr-decisions/references/preparation.md +++ b/runtime/dk-pr-decisions/references/preparation.md @@ -25,7 +25,7 @@ defines response fields, evidence normalization and stored artifacts. python3 dk-pr-decisions/scripts/decisions.py prepare --input-manifest MANIFEST.json --output-dir PRIVATE_DIR [--previous-preparation FILE] [--full-analysis] ``` -The API v2 envelope returns `data.artifact`, `preparation_id`, `readiness` and +The API v2 envelope returns `data.artifact`, `preparation_id`, `engineering_id`, `readiness` and `refresh`. Consumers validate readiness and all current fingerprints, including choices, rules and reviewer sources. Never substitute local analysis to pass a gate. @@ -65,7 +65,7 @@ or unverifiable selected parents block; old source paths use verified retained c Preserve retained values and stable IDs; the runtime assigns and retires IDs, rebuilds locations, adds mandatory dependencies and validates the merged graph. - `--full-analysis`, changed base/mode/policy or missing prior Git objects require - full reassessment. Version 1 migrates through fresh analysis into separate v2 storage. + full reassessment. Version 1 migrates through fresh analysis into separate v3 storage. Old-policy parents retain integrity/structural checks before current-policy analysis. - Unbounded impact may request one full fallback. Each model phase allows one correction; another invalid response blocks. No per-conclusion calls or scheduler. @@ -143,3 +143,46 @@ cleanup. For version-2 review provenance, reconcile directly against the origina reviewed preparation using the published assessment; version-1 coverage keeps its exact-scope verification. Refresh engineering verification and require exact-head CI. Preserve the existing run, cycle count and five-cycle limit. + + +## Fresh workflow storage and compatibility + +Preparation v3 writes new versions beneath a private workflow directory under +`${XDG_STATE_HOME:-~/.local/state}/dk-code-factory/workflows/`. The context collector +returns `workflow_dir` and a durable `context_file`. Use these paths for exports, +manifests, scope/rule handoffs, validation reports and verification files. `prepare` +retains the legacy `--output-dir` argument for caller compatibility and validates +that it is outside the repository; the returned `artifact` path in workflow storage +is authoritative. Existing artifacts and interrupted runtimes are never rewritten. + +Each source retains original path/hash plus a read-only `snapshot` and a `mode`: +`capture` or `live`. Generated manifest/context/Decisions/metadata/patch, reviewer +descriptors, prior preparation, complete bundled feedback, and pinned checkout +production/test files are captures. Arbitrary external evidence defaults to live. +Optional manifest `source_modes` maps exact indexed source keys (for example +`run_history/0`) to `capture` or `live`. Mark generated exports as captures; preserve +required external rules and instruction sources as live. Captures survive removal +of their original export paths. Every live source must remain present and match. +The workflow owns a separate clean pinned checkout. Gates read the captured context, +metadata, Decisions and patch, and validate that checkout and all retained bindings. +Current live PR head/body, configuration and mutable local choices still gate work. + +Retain both IDs. `preparation_id` covers all artifact fields. Runtime-computed +`engineering_id` covers normalized governing inputs, tree/base, PR scope, Decisions, +ordered reviewer descriptors, analysis, readiness and policy. Declared operational +fields, storage paths, refresh reports and verified lineage bookkeeping are excluded; +unknown fields govern. Counter-only refresh can preserve checkpoints, assessments, +results and verification while retaining their original preparation IDs as provenance. +Interrupted session resume additionally requires unchanged head. Governing changes +invalidate reuse. Tree-equivalent series cleanup still requires its guarded path. +Legacy active runs must use their retained runtime; starting a fresh request after a +terminal run preserves history and counters under the existing start-run rules. + + +Each new artifact also retains `recovery_manifest` and `recovery_context` path/hash +references. After original exports disappear, use `recovery_manifest.path` with +`prepare --previous-preparation ARTIFACT`. It references the owned context, checkout +and captured evidence while retaining live source paths. Its `source_origins` map +preserves captured source identities across relocation; this bookkeeping does not +replace governing content fingerprints. Retain these files unchanged; construct a +new manifest when current evidence changes. Required live inputs still block if lost. diff --git a/runtime/dk-pr-decisions/references/storage.md b/runtime/dk-pr-decisions/references/storage.md index 4b9b05e..6c42441 100644 --- a/runtime/dk-pr-decisions/references/storage.md +++ b/runtime/dk-pr-decisions/references/storage.md @@ -3,7 +3,7 @@ Set `DK_PR_DECISIONS_HOME` to an absolute private directory accessible to every run that should share agreements. This is the root of the complete Decisions cache, not its parent. Records use -`////pr-.json`. The PR URL supplies the +`/v3////pr-.json`. The PR URL supplies the base identity, including when the checkout and head belong to a fork. Hosts, repositories and PRs have independent records. The comments report, suggestion evidence and publication receipts use the same store. @@ -16,9 +16,10 @@ silent fallback or merging with another store. Hosts must provide persistent storage, sandbox access and a filesystem supporting process locks, atomic rename and synchronization. No Code Factory configuration is changed by this plugin. -Every modifying command holds a per-PR process lock from live snapshot refresh -through analysis, local changes and publication bookkeeping. PRs can proceed -independently. New directories are private (0700); records and locks use 0600. +Every modifying command reads a monotonic revision before analysis and checks it +under a per-PR process lock at commit. GitHub and model calls run outside the lock. +Stale writers return `metadata_conflict` with their retained proposal and request; +reconcile explicitly without automatic merging or retries. PRs proceed independently. New directories are private (0700); records and locks use 0600. Atomic replacement synchronizes the file and containing directory. Interruption leaves the previous complete record or its committed replacement. Temporary files from a killed process are not read as records. Do not delete lock files while @@ -45,12 +46,14 @@ through `add` or `replace`. Saving and recovering cache records never mutates GitHub. Publication remains an explicit separate action (`publish` or `scan --publish`). A pending publication -receipt survives interruption and refresh. Use `publish --reconcile` to observe +receipt survives interruption and blocks competing refreshes and writers. Use `publish --reconcile` to observe live state before any retry; do not discard uncertain receipts or replay writes. ## Migration -Existing Decisions JSON formats and API v2 records remain readable. With the +Existing Decisions JSON formats and API v2 records remain readable. Fresh writes use +the versioned `v3/` namespace. A validated snapshot from the same configured root +may seed it without altering the legacy file. Pending publication remains blocking. With the default root, the former `dk-pr-read-comments` cache is still migrated on refresh. An explicit root never searches the default or former report store. diff --git a/runtime/dk-pr-decisions/scripts/decision_storage.py b/runtime/dk-pr-decisions/scripts/decision_storage.py index 4a51c51..a3d9f7e 100644 --- a/runtime/dk-pr-decisions/scripts/decision_storage.py +++ b/runtime/dk-pr-decisions/scripts/decision_storage.py @@ -75,9 +75,11 @@ def check_access(path, *, directory=False): 'and directory traversal access to DK_PR_DECISIONS_HOME') -def cache_path(pr_url): +def cache_path(pr_url, *, versioned=True): relative = identity(pr_url) base = root() + if versioned: + base = base / 'v3' path = validate_location(base / relative) if not path.is_relative_to(base): raise ValueError('Decisions record escapes the configured root through a symlink; ' diff --git a/runtime/dk-pr-decisions/scripts/decisions.py b/runtime/dk-pr-decisions/scripts/decisions.py index 8b90262..99aee0e 100644 --- a/runtime/dk-pr-decisions/scripts/decisions.py +++ b/runtime/dk-pr-decisions/scripts/decisions.py @@ -18,9 +18,32 @@ import subprocess import sys import tempfile +import copy +import uuid +from contextvars import ContextVar from pathlib import Path from urllib.parse import quote, urlparse from shared.github_reads import run as github_run +from shared.private_state import mutate, write +from shared.workflow_storage import allocate + +_OPERATION = ContextVar('decisions_operation', default=None) +_REQUEST = ContextVar('decisions_request', default=None) + + +class MetadataConflict(ValueError): + code = 'metadata_conflict' + + def __init__(self, state, path, current, reason): + directory = allocate() + self.proposal_path = str(directory / 'decisions-proposal.json') + self.next_action = ('Read the retained proposal and current Decisions record; reconcile pending ' + 'publication with publish --reconcile, then explicitly reapply the intended update ' + 'against the current revision. Do not replay publication.') + write(Path(self.proposal_path), {'proposal': state, 'current_revision': current.get('revision', 0), + 'expected_revision': state.get('revision', 0), 'record': str(path), + 'request': _REQUEST.get(), 'reason': reason, 'next_action': self.next_action}) + super().__init__(reason) GH_HOST = "github.com" @@ -521,14 +544,13 @@ def render(pr, viewer, state): @contextmanager def transaction(pr_target=None): - # Discovery only locates the lock. Fetch the working snapshot under the lock. + # Resolve the operation identity; only the final revision check holds a lock. storage.root() pr_url = pr_target if not isinstance(pr_url, str) or not pr_url.startswith('https://'): pr_url = gh_json(['pr', 'view', *([str(pr_target)] if pr_target else []), '--json', 'url'])['url'] - with storage.lock(pr_url): - yield pr_url + yield pr_url def scan(pr_target=None): @@ -561,16 +583,19 @@ def _scan(pr_target): if not viewer: raise RuntimeError("Could not identify the authenticated GitHub user") state_path = cache_path(pr['url']) - # Legacy report migration is default-store only. An explicit root is exclusive. legacy_path = None if 'DK_PR_DECISIONS_HOME' not in os.environ: - legacy_path = storage.root().parent / 'dk-pr-read-comments' / storage.identity(pr['url']) - legacy_path = storage.validate_location(legacy_path) - source_path = state_path - if not state_path.exists() and legacy_path is not None and legacy_path.exists(): - source_path = legacy_path + legacy_path = storage.validate_location(storage.root().parent / 'dk-pr-read-comments' / storage.identity(pr['url'])) + previous_path = storage.cache_path(pr['url'], versioned=False) + candidates = [state_path, previous_path] + ([legacy_path] if legacy_path is not None else []) + source_path = next((p for p in candidates if p.exists()), state_path) prior = storage.read(source_path, pr['url'], missing=True) + importing = source_path != state_path and source_path.exists() migrating = source_path == legacy_path + if importing: + prior['revision'] = 0 + if (prior.get('publication_intent') or {}).get('status') == 'pending': + raise MetadataConflict(prior, state_path, prior, 'Publication is pending; explicitly reconcile before refreshing') if migrating: discussion_urls = {comment.get('url') for comment in prior.get('comments', {}).values()} prior['decisions'] = [item for item in prior.get('decisions', []) @@ -623,7 +648,8 @@ def _scan(pr_target): manual = [item for item in prior.get('manual_decisions', []) if decision_id(item['text']) not in superseded] decisions = decisions + [item for item in manual if item not in decisions] - state = {"version": 2, "run_number": run_number, "pr": pr, "viewer": viewer, + state = {**prior, "version": 2, "revision": prior.get('revision', 0), + "run_number": run_number, "pr": pr, "viewer": viewer, "linked_issues": context["linked_issues"], "comments": comments, "threads": threads, "decisions": decisions, "summaries": {k: v for k, v in summaries.items() if k in threads}, @@ -639,7 +665,7 @@ def _scan(pr_target): "code_suggestions": prior.get('code_suggestions', {}) if prior.get('pr', {}).get('headRefOid') == pr['headRefOid'] else {}} retain_agreements(state) # Commit only after every API page, body, and analysis has succeeded. - atomic_write(state_path, json.dumps(state, ensure_ascii=False, indent=2) + "\n") + save_state(state, state_path) return state, state_path @@ -723,7 +749,17 @@ def compose_publication(state, decisions, description=None, description_review_s def publish(state, decisions, description=None, description_review_state=None, *, persist=None, reconcile=False): + token = _OPERATION.set({'id': uuid.uuid4().hex, 'reconcile': reconcile}) + try: + return _publish(state, decisions, description, description_review_state, persist=persist, reconcile=reconcile) + finally: + _OPERATION.reset(token) + + +def _publish(state, decisions, description=None, description_review_state=None, *, persist=None, reconcile=False): """One compare-before-write body publisher, with durable intent and live reconciliation.""" + if state.get('storage_version') == 3 and persist is None: + raise ValueError('Fresh Decisions publication requires durable intent persistence') superseded = {r['id'] for r in decision_records(state) if r['status'] == 'superseded'} decisions = [item for item in decisions if decision_id(item['text']) not in superseded] pending_intent = state.get('publication_intent') @@ -776,7 +812,8 @@ def same(snapshot, body): if reconciled or reconcile: return {'updated': False, 'pending': decisions, 'reason': 'Current choices or description remain pending. Publish explicitly after review.'} - intent = {'snapshot': {key: pr[key] for key in ('body', 'title', 'headRefOid') if key in pr}, + intent = {'operation_id': _OPERATION.get()['id'], + 'snapshot': {key: pr[key] for key in ('body', 'title', 'headRefOid') if key in pr}, 'body': updated, 'status': 'pending'} intent['id'] = hashlib.sha256((json.dumps(intent, sort_keys=True) + str(len(state.get('publication_history', [])))).encode()).hexdigest()[:20] @@ -793,6 +830,8 @@ def same(snapshot, body): '--json', 'body,title,headRefOid']) if not same(intent['snapshot'], updated): raise RuntimeError('Publication could not be verified against live head, title and body') + except MetadataConflict: + raise except Exception as exc: intent['error'] = str(exc) save() @@ -926,7 +965,7 @@ def suggest_from_code(state, path): raise RuntimeError(f'{exc}. Analysis response: {diagnostic}{response_note} ' f'Code evidence retained at {evidence_dir / "manifest.json"}.') from None state['code_suggestions'] = {'head': head, 'items': result['decisions']} - atomic_write(path, json.dumps(state, ensure_ascii=False, indent=2) + '\n') + save_state(state, path) return result['decisions'] @@ -1073,13 +1112,49 @@ def apply_replacements(body, replacements): return body +def validated_cache(path, pr_url): + state = storage.read(path, pr_url) + if type(state.get('revision', 0)) is not int or state.get('revision', 0) < 0: + raise ValueError('Invalid Decisions revision; reconcile through its retained runtime') + decision_records(state) + return state + + def load_cache(pr_url): path = cache_path(pr_url) - return storage.read(path, pr_url), path + if not path.exists(): + legacy = storage.cache_path(pr_url, versioned=False) + if not legacy.exists(): + raise ValueError('No cached decisions; run scan first') + state = validated_cache(legacy, pr_url) + state['revision'] = 0 + save_state(state, path) + return validated_cache(path, pr_url), path def save_state(state, path): - atomic_write(path, json.dumps(state, ensure_ascii=False, indent=2) + '\n') + expected = state.get('revision', 0) + if type(expected) is not int or expected < 0: + raise ValueError('Decisions revision must be a nonnegative integer') + proposed = copy.deepcopy(state) + proposed.setdefault('version', 2) + operation = _OPERATION.get() or {} + def commit(current): + pending = current.get('publication_intent') or {} + if current.get('revision', 0) != expected: + raise MetadataConflict(proposed, path, current, 'Decisions revision changed during this operation') + if (pending.get('status') == 'pending' and not operation.get('reconcile') + and (not operation.get('id') or pending.get('operation_id') != operation['id'])): + raise MetadataConflict(proposed, path, current, 'Publication is pending and owned by another operation') + current.clear() + current.update(proposed, storage_version=3, revision=expected + 1) + return current['revision'] + path = storage.validate_location(Path(path)) + storage.check_access(path) + storage.private_directory(path.parent) + state['revision'] = mutate(path, {}, commit) + storage.sync_directory(path.parent) + state['storage_version'] = 3 def active_choices(state): @@ -1197,7 +1272,7 @@ def error(self, message): raise ValueError(message) -def cli(): +def _cli(): parser = Parser(description='Analyze and explicitly publish PR decisions') parser.add_argument('--pr', help='Full PR URL (required for list and publish)') sub = parser.add_subparsers(dest='command', required=True) @@ -1226,6 +1301,7 @@ def cli(): if name == 'replace': add.add_argument('--id', required=True, help='Active decision ID to supersede') args = parser.parse_args() + _REQUEST.set(vars(args)) if args.command == 'prepare': sys.path.insert(0, str(Path(__file__).resolve().parent)) from preparation import prepare @@ -1287,11 +1363,22 @@ def execute_command(args): print(json.dumps(envelope(args.command, data, publication), ensure_ascii=False)) +def cli(): + token = _REQUEST.set(None) + try: + return _cli() + finally: + _REQUEST.reset(token) + + def main(): try: cli() except (RuntimeError, ValueError, KeyError, TypeError, OSError) as exc: - print(json.dumps(envelope(None, error={'code': type(exc).__name__, 'message': str(exc)}), ensure_ascii=True)) + error = {'code': getattr(exc, 'code', type(exc).__name__), 'message': str(exc)} + if isinstance(exc, MetadataConflict): + error.update(proposal_path=exc.proposal_path, next_action=exc.next_action) + print(json.dumps(envelope(None, error=error), ensure_ascii=True)) return 1 return 0 diff --git a/runtime/dk-pr-decisions/scripts/preparation.py b/runtime/dk-pr-decisions/scripts/preparation.py index af19084..0c5d7ed 100644 --- a/runtime/dk-pr-decisions/scripts/preparation.py +++ b/runtime/dk-pr-decisions/scripts/preparation.py @@ -15,8 +15,10 @@ import tempfile from shared import preparation_graph as graph_api +from shared.engineering_identity import engineering_id, compatible_reference +from shared.workflow_storage import allocate, owned -VERSION = 2 +VERSION = 3 DIMENSIONS = ('correctness', 'remediation_depth', 'performance', 'compatibility', 'complexity') EVIDENCE = ('issues', 'feedback', 'choices', 'user_instructions', 'run_history', 'questions', 'prior_preparation', 'rules', 'production', 'tests', 'ci', 'reviewers') @@ -108,6 +110,28 @@ def path(value): raise ValueError('preparation requires a clean isolated checkout') sources = {key: {'path': str(Path(value).resolve()), 'sha256': file_hash(value)} for key, value in paths.items()} + modes = manifest.get('source_modes', {}) + if (not isinstance(modes, dict) or set(modes) - set(sources) + or any(mode not in {'capture', 'live'} for mode in modes.values())): + raise ValueError('source_modes must map indexed source keys to capture or live') + for key, source in sources.items(): + category = key.split('/')[0] + captured = (key in {'manifest', 'context', 'decisions', 'metadata', 'patch'} + or category in {'reviewers', 'prior_preparation'} + or category in {'production', 'tests'} and + Path(source['path']).is_relative_to(Path(context['checkout']))) + if category == 'feedback': + value = read(source['path']) + captured |= value.get('complete') is True + source['mode'] = modes.get(key, 'capture' if captured else 'live') + origins = manifest.get('source_origins', {}) + if (not isinstance(origins, dict) or set(origins) - set(sources) + or any(not text(value) or sources[key]['mode'] != 'capture' for key, value in origins.items())): + raise ValueError('source_origins requires captured source keys and nonempty original identities') + for key, source in sources.items(): + if source['mode'] == 'capture': + source['origin_identity'] = origins.get(key) or graph_api.source_identity( + key, source, context['checkout'], Path(source['path']).read_bytes()) prior = [] for value in evidence['prior_preparation']: prior_path = path(value) @@ -242,8 +266,9 @@ def readiness(analysis): def policy(): + import shared.engineering_identity as engineering return digest({str(p.name): file_hash(p) for p in ( - Path(__file__), Path(graph_api.__file__), + Path(__file__), Path(graph_api.__file__), Path(engineering.__file__), Path(__file__).resolve().parents[1] / 'references/preparation.md', Path(__file__).resolve().parents[1] / 'references/preparation-contract.md')}) @@ -292,6 +317,7 @@ def reassessment_prompt(prompt, parent_path, parent, paths, changed_sources, sel def prepare(manifest_path, output_dir, analyzer=None, *, previous_preparation=None, full_analysis=False): manifest, context, decisions, sources, descriptors, prior = collect(manifest_path) + collection_sources = dict(sources) # Capture collection order before rebinding citation slots. Manifest aliases # can disappear after publication; retained paths must not resolve them again. reviewer_paths = [source['path'] for key, source in sources.items() if key.startswith('reviewers/')] @@ -467,7 +493,7 @@ def unchanged(): graph_api.conclusions(parent['analysis'])[parent['graph'][k]['location']]), 'model_call_count': calls, 'phases': phases if parent and not migration else []} # Return the selected immutable artifact only when every live binding is exact. - if (parent and not force_full and mode == 'operational' and parent['identity'] == identity + if (parent and parent['preparation_version'] == VERSION and not force_full and mode == 'operational' and parent['identity'] == identity and parent['inputs'] == current and parent.get('reviewer_paths') == reviewer_paths and {k: (v['path'], v['sha256']) for k, v in parent['sources'].items()} == @@ -475,9 +501,18 @@ def unchanged(): validate(parent, context, decisions, require_ready=False) report['mode'] = 'unchanged' return {'artifact': str(Path(previous_preparation).resolve()), 'preparation_id': parent['preparation_id'], - 'readiness': parent['readiness'], 'refresh': report} - version_dir = Path(tempfile.mkdtemp(prefix='preparation-', dir=_output_directory(output_dir))) + 'engineering_id': engineering_id(parent), 'readiness': parent['readiness'], 'refresh': report} + workflow_dir = (owned(parent['workflow_dir']) if parent and parent.get('workflow_dir') else + owned(context['workflow_dir']) if context.get('workflow_dir') else + allocate(repository=context.get('repository', context['checkout']))) + outside(workflow_dir, context.get('repository', context['checkout'])) + outside(workflow_dir, context['checkout']) + version_dir = Path(tempfile.mkdtemp(prefix='preparation-', dir=workflow_dir)) try: + workspace = version_dir / 'checkout' + git(context['checkout'], 'clone', '--quiet', '--no-hardlinks', '--no-checkout', + context['checkout'], str(workspace)) + git(workspace, 'checkout', '--quiet', '--detach', context['head_sha']) for key, source in sources.items(): target = version_dir / 'evidence' / key target.parent.mkdir(parents=True, exist_ok=True, mode=0o700) @@ -487,18 +522,56 @@ def unchanged(): target.write_bytes(content) os.chmod(target, 0o400) source['snapshot'] = str(target) + # Retain a directly usable collection manifest, not only its old aliases. + # Live inputs keep their original paths; captured inputs use private copies. + from shared.private_state import write + recovered_context = {**context, 'checkout': str(workspace), + 'metadata': sources['metadata']['snapshot'], 'patch': sources['patch']['snapshot'], + 'workflow_dir': str(workflow_dir), 'context_file': str(version_dir / 'context.json')} + context_path = version_dir / 'context.json' + write(context_path, recovered_context) + recovery = {**manifest, 'context': str(context_path), 'decisions': sources['decisions']['snapshot'], + 'evidence': {}, 'source_modes': {}, 'source_origins': {}} + for key in ('manifest', 'context', 'decisions', 'metadata', 'patch'): + recovery['source_modes'][key] = sources[key]['mode'] + if sources[key]['mode'] == 'capture': + recovery['source_origins'][key] = sources[key]['origin_identity'] + for category, values in manifest['evidence'].items(): + recovery['evidence'][category] = [] + for index, value in enumerate(values): + original = str((Path(manifest_path).resolve().parent / value).resolve()) + key = category + '/' + str(index) + source = collection_sources[key] + recovery['evidence'][category].append(source['snapshot'] if source['mode'] == 'capture' else original) + recovery['source_modes'][key] = source['mode'] + if source['mode'] == 'capture': + recovery['source_origins'][key] = source['origin_identity'] + # Reviewer instruction sources are collected from the preserved descriptor. + for key, source in collection_sources.items(): + if key.startswith('reviewer-source/'): + recovery['source_modes'][key] = source['mode'] + if source['mode'] == 'capture': + recovery['source_origins'][key] = source['origin_identity'] + manifest_target = version_dir / 'manifest.json' + write(manifest_target, recovery) + context_path.chmod(0o400) + manifest_target.chmod(0o400) artifact = {'preparation_version': VERSION, 'mode': manifest['mode'], 'identity': identity, + 'workflow_dir': str(workflow_dir), 'workspace': str(workspace), + 'recovery_manifest': {'path': str(manifest_target), 'sha256': file_hash(manifest_target)}, + 'recovery_context': {'path': str(context_path), 'sha256': file_hash(context_path)}, + 'reviewer_order': list(descriptors), 'sources': sources, 'reviewers': descriptors, 'reviewer_paths': reviewer_paths, 'analysis': result, 'graph': graph, 'retired_ids': retired, 'inputs': current, 'parent': ({'preparation_id': parent['preparation_id'], 'path': str(Path(previous_preparation).resolve()), 'sha256': parent_hash, 'migration': migration} if parent else None), 'refresh': report, 'readiness': readiness(result)} + artifact['engineering_id'] = engineering_id(artifact) artifact['preparation_id'] = digest(artifact) validate(artifact, context, decisions, require_ready=False) unchanged() target = version_dir / 'preparation.json' - from shared.private_state import write write(target, artifact) os.chmod(target, 0o400) except BaseException: @@ -506,16 +579,11 @@ def unchanged(): shutil.rmtree(version_dir) raise return {'artifact': str(target), 'preparation_id': artifact['preparation_id'], - 'readiness': artifact['readiness'], 'refresh': report} - - -def _output_directory(path): - path.mkdir(parents=True, exist_ok=True, mode=0o700) - return path + 'engineering_id': engineering_id(artifact), 'readiness': artifact['readiness'], 'refresh': report} def validate(artifact, context, decisions, *, require_ready=True, check_sources=True): - if not isinstance(artifact, dict) or artifact.get('preparation_version') not in {1, VERSION}: + if not isinstance(artifact, dict) or artifact.get('preparation_version') not in {1, 2, VERSION}: raise ValueError('unsupported preparation artifact') if artifact.get('mode') not in {'standalone', 'iteration'}: raise ValueError('invalid preparation mode') @@ -542,7 +610,7 @@ def validate(artifact, context, decisions, *, require_ready=True, check_sources= if not isinstance(descriptor, dict) or descriptor.get('reviewer') != name or descriptor.get('complete') is not True: raise ValueError('preparation reviewer adaptation is incomplete') validate_analysis(artifact['analysis'], artifact['sources'], artifact['reviewers']) - if artifact['preparation_version'] == VERSION: + if artifact['preparation_version'] >= 2: if 'reviewer_paths' in artifact: paths = artifact['reviewer_paths'] expected_paths = [source['path'] for key, source in sources.items() if key.startswith('reviewers/')] @@ -570,7 +638,8 @@ def validate(artifact, context, decisions, *, require_ready=True, check_sources= raise ValueError('invalid preparation governing input fingerprint') if same_policy: content = Path(source['snapshot']).read_bytes() - expected_input = {'identity': graph_api.source_identity(key, source, context['checkout'], content), + original_context = read(sources['context']['snapshot']) + expected_input = {'identity': graph_api.source_identity(key, source, original_context['checkout'], content), 'governing': digest(graph_api.semantic(key, content))} if expected_input != fingerprint: raise ValueError('preparation governing input fingerprint mismatch') @@ -592,11 +661,42 @@ def validate(artifact, context, decisions, *, require_ready=True, check_sources= if file_hash(context[field]) != sources[field]['sha256']: raise ValueError(f'preparation source identity mismatch: supplied {field} differs from prepared evidence') for source in artifact['sources'].values(): + if artifact['preparation_version'] >= 3 and source.get('mode') == 'capture': + continue if file_hash(source['path']) != source['sha256']: raise ValueError('preparation source changed; refresh preparation') + if artifact['preparation_version'] >= 3: + for key in ('recovery_manifest', 'recovery_context'): + reference = artifact.get(key, {}) + if not Path(reference.get('path', '')).is_absolute() or file_hash(reference['path']) != reference.get('sha256'): + raise ValueError('workflow recovery capture changed: ' + key) + if artifact.get('engineering_id') != engineering_id(artifact): + raise ValueError('preparation engineering identity mismatch') + if (set(artifact.get('reviewer_order', [])) != set(artifact['reviewers']) + or len(artifact['reviewer_order']) != len(artifact['reviewers'])): + raise ValueError('invalid ordered reviewer identity') + if any(source.get('mode') not in {'capture', 'live'} for source in sources.values()): + raise ValueError('preparation source requires capture/live classification') + if check_sources: + from shared.bindings import pinned_bindings + with pinned_bindings(artifact['reviewers']): + pass return artifact +def source_path(artifact, key): + source = artifact['sources'][key] + return source['snapshot'] if artifact.get('preparation_version', 1) >= 3 else source['path'] + + +def context_for(artifact): + context = read(source_path(artifact, 'context')) + if artifact.get('preparation_version', 1) >= 3: + context.update(checkout=artifact['workspace'], + patch=source_path(artifact, 'patch'), metadata=source_path(artifact, 'metadata')) + return context + + def load(path, context, decisions, **kwargs): outside(path, context.get('repository', context['checkout'])) outside(path, context['checkout']) @@ -606,8 +706,8 @@ def load(path, context, decisions, **kwargs): def gate(path, *, pr_url, head_sha, body=None, require_ready=True): """For work/completion gates: recover authoritative context from retained sources.""" artifact = read(path) - context = read(artifact['sources']['context']['path']) - raw = read(artifact['sources']['decisions']['path']) + context = context_for(artifact) + raw = read(source_path(artifact, 'decisions')) decisions = raw.get('data', raw) if context['url'] != pr_url or context['head_sha'] != head_sha: raise ValueError('preparation PR or head mismatch') @@ -631,7 +731,7 @@ def retained(path): or file_hash(target) != source['sha256']): raise ValueError('retained preparation evidence changed: ' + key) copies[key] = {'path': str(target), 'sha256': source['sha256']} - context = read(copies['context']['path']) + context = context_for(artifact) raw = read(copies['decisions']['path']) decisions = raw.get('data', raw) outside(path, context['repository']) @@ -642,11 +742,13 @@ def retained(path): def verify_completion(path, artifact): """Require actual engineering assessment, independent of reviewer convergence and CI.""" - context = read(artifact['sources']['context']['path']) + context = context_for(artifact) outside(path, context.get('repository', context['checkout'])) value = read(path) - if (value.get('verification_version') != 1 - or value.get('preparation_id') != artifact['preparation_id'] + version = 2 if artifact.get('preparation_version', 1) >= 3 else 1 + if (value.get('verification_version') != version + or not compatible_reference(value, artifact) + or not text(value.get('preparation_id')) or value.get('head_sha') != artifact['identity']['head_sha'] or value.get('pr_url') != artifact['identity']['pr_url']): raise ValueError('engineering verification identity is stale') diff --git a/runtime/dk-pr-decisions/tests/preparation_fixtures.py b/runtime/dk-pr-decisions/tests/preparation_fixtures.py index 3d0d4f7..45d8dbf 100644 --- a/runtime/dk-pr-decisions/tests/preparation_fixtures.py +++ b/runtime/dk-pr-decisions/tests/preparation_fixtures.py @@ -3,6 +3,7 @@ import importlib.util import json from pathlib import Path +from shared.workflow_storage import allocate import subprocess ROOT = Path(__file__).resolve().parents[2] @@ -74,9 +75,33 @@ def artifact(context, decisions, sources, reviewers=('example',), result=None): return value +def promote(value, context, decisions): + """Use the fresh contract in tests of the former run-record CLI adapter.""" + if value['preparation_version'] >= 3: + return + directory = Path(context['patch']).parent / 'fresh-inputs' + directory.mkdir(exist_ok=True) + def put(name, content): + path = directory / name + path.write_text(json.dumps(content)) + return str(path) + decisions.setdefault('decisions', []) + absence = put('absence.json', {'verified_absence': True, 'reason': 'No other fixture evidence'}) + evidence = {name: [absence] for name in prep.EVIDENCE} + evidence['production'] = [str(Path(context['checkout']) / 'x.py')] + evidence['reviewers'] = [put('reviewer-' + str(i) + '.json', d) for i, d in enumerate(value['reviewers'].values())] + manifest = put('manifest.json', {'mode': value['mode'], 'context': put('context.json', context), + 'decisions': put('decisions.json', decisions), 'evidence': evidence}) + saved = prep.prepare(manifest, directory / 'output', analyzer=lambda *_: response(value['analysis'])) + current = prep.read(saved['artifact']) + value.clear() + value.update(current) + + class Package: def __init__(self, root): self.root = Path(root) + self.versions = allocate() self.repo = self.root / 'checkout' self.repo.mkdir() def git(*args): @@ -89,7 +114,7 @@ def git(*args): git('commit', '-qm', 'Transform fixture') self.head = git('rev-parse', 'HEAD') self.context = {'url': 'https://github.com/example/fixture/pull/1', 'base_sha': self.head, - 'head_sha': self.head, 'checkout': str(self.repo), 'repository': str(self.repo), + 'head_sha': self.head, 'checkout': str(self.repo), 'repository': str(self.repo), 'workflow_dir': str(self.versions), 'patch': str(self.root / 'diff.patch'), 'metadata': str(self.root / 'metadata.json')} (self.root / 'diff.patch').write_text('diff --git a/transform.py b/transform.py\n+++ b/transform.py\n@@ -1 +1 @@\n-old\n+new\n') (self.root / 'metadata.json').write_text(json.dumps({'body': '', 'title': 'Transform fix'})) @@ -113,5 +138,5 @@ def write(self, name, value): return path def prepare(self, result=None): - return prep.prepare(self.manifest_path, self.root / 'versions', + return prep.prepare(self.manifest_path, self.versions, analyzer=lambda prompt, checkout: response(result or analysis())) diff --git a/runtime/dk-pr-decisions/tests/test_incremental_preparation.py b/runtime/dk-pr-decisions/tests/test_incremental_preparation.py index c8ff50f..db92baa 100644 --- a/runtime/dk-pr-decisions/tests/test_incremental_preparation.py +++ b/runtime/dk-pr-decisions/tests/test_incremental_preparation.py @@ -23,7 +23,7 @@ def setUp(self): self.package = Package(self.temp.name) def refresh(self, parent, result=None, **kwargs): - return prep.prepare(self.package.manifest_path, self.package.root / 'versions', + return prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=parent['artifact'], analyzer=lambda *_: result, **kwargs) @@ -54,7 +54,7 @@ def commit(self, changes, *, amend=False, message='Reassessed fixture'): def test_identical_inputs_reuse_artifact_without_model(self): first = self.package.prepare() with patch.object(prep, 'analyze') as model: - refreshed = prep.prepare(self.package.manifest_path, self.package.root / 'versions', + refreshed = prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact']) model.assert_not_called() self.assertEqual(refreshed['artifact'], first['artifact']) @@ -192,11 +192,11 @@ def test_changed_input_requires_all_affected_and_one_batch(self): invalid = retain(parent) invalid['dispositions'].pop() with patch('shared.executors.execute', side_effect=[json.dumps(invalid), json.dumps(retain(parent))]) as model: - result = prep.prepare(self.package.manifest_path, self.package.root / 'versions', previous_preparation=first['artifact']) + result = prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact']) self.assertEqual(model.call_count, 2) self.assertEqual(result['refresh']['model_call_count'], 2) with patch('shared.executors.execute', return_value=json.dumps(retain(parent))) as model: - result = prep.prepare(self.package.manifest_path, self.package.root / 'versions', previous_preparation=first['artifact']) + result = prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact']) self.assertEqual(model.call_count, 1) self.assertEqual(result['refresh']['model_call_count'], 1) self.assertEqual(prep.read(result['artifact'])['analysis'], parent['analysis']) @@ -214,7 +214,7 @@ def test_graph_closure_cycles_and_selective_reassessment(self): node['depends_on'] = [performance] elif node['location'] == performance: node['depends_on'] = [correctness] - first = prep.prepare(self.package.manifest_path, self.package.root / 'versions', analyzer=lambda *_: value) + first = prep.prepare(self.package.manifest_path, self.package.versions, analyzer=lambda *_: value) parent = prep.read(first['artifact']) self.commit({'new/caller.py': 'from transform import transform\n'}) # The prompt descriptor is unchanged: only the added file is substantive. @@ -243,13 +243,13 @@ def test_explicit_escalation_allows_only_one_full_fallback(self): self.package.write('user.json', {'instruction': 'Cross-cutting change'}) escalation = {'full_reassessment': True, 'reason': 'Cannot bound the scope of this interaction'} with patch('shared.executors.execute', side_effect=[json.dumps(escalation), json.dumps(retain(parent))]) as model: - result = prep.prepare(self.package.manifest_path, self.package.root / 'versions', previous_preparation=first['artifact']) + result = prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact']) self.assertEqual(model.call_count, 2) self.assertEqual(result['refresh']['mode'], 'full_fallback') self.assertEqual(result['refresh']['model_call_count'], 2) with patch('shared.executors.execute', return_value=json.dumps(escalation)) as model: with self.assertRaisesRegex(ValueError, 'cannot escalate again'): - prep.prepare(self.package.manifest_path, self.package.root / 'versions', previous_preparation=first['artifact']) + prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact']) self.assertEqual(model.call_count, 3) # Initial escalation, full phase and its one correction. def test_new_conclusions_get_fresh_ids_deleted_ids_stay_retired(self): @@ -451,7 +451,7 @@ def mutate_before_hash(path): with self.assertRaisesRegex(ValueError, 'changed during analysis'): package.prepare() self.assertTrue(mutated) - self.assertEqual(list((package.root / 'versions').glob('*/preparation.json')), []) + self.assertEqual(list((package.versions).glob('*/preparation.json')), []) def test_reviewer_descriptor_change_between_parse_and_hash_cannot_publish(self): package = self.package @@ -473,7 +473,7 @@ def mutate_before_hash(path): with self.assertRaisesRegex(ValueError, 'changed during analysis'): package.prepare() self.assertTrue(mutated) - self.assertEqual(list((package.root / 'versions').glob('*/preparation.json')), []) + self.assertEqual(list((package.versions).glob('*/preparation.json')), []) def test_stale_inputs_interruptions_and_concurrent_activation(self): package = self.package @@ -490,11 +490,11 @@ def stale(*_): package.write('user.json', {'instruction': 'Changed during reassessment'}) return retain(parent) with self.assertRaisesRegex(ValueError, 'source changed during analysis'): - prep.prepare(package.manifest_path, package.root / 'versions', previous_preparation=first['artifact'], analyzer=stale) + prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact'], analyzer=stale) with self.assertRaises(KeyboardInterrupt): - prep.prepare(package.manifest_path, package.root / 'versions', previous_preparation=first['artifact'], + prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact'], analyzer=lambda *_: (_ for _ in ()).throw(KeyboardInterrupt())) - self.assertEqual(list((package.root / 'versions').glob('*/preparation.json')), [Path(first['artifact'])]) + self.assertEqual(list((package.versions).glob('*/preparation.json')), [Path(first['artifact'])]) second = self.refresh(first, retain(parent)) other = self.refresh(first, retain(parent)) state.attach_preparation(record, second['artifact'], package.context['url'], package.head) @@ -585,10 +585,10 @@ def test_v1_migration_uses_fresh_analysis_and_separate_storage(self): 'identity': {'pr_url': context['url'], 'base_sha': context['base_sha'], 'head_sha': context['head_sha'], 'body_hash': decisions['body_hash'], 'decisions_hash': prep.digest(decisions)}, - 'sources': sources, 'reviewers': reviewers, 'analysis': analysis(), 'readiness': 'ready'} + 'sources': {k: {f: v for f, v in s.items() if f not in {'mode', 'origin_identity'}} for k, s in sources.items()}, 'reviewers': reviewers, 'analysis': analysis(), 'readiness': 'ready'} old['preparation_id'] = prep.digest(old) legacy_dir = package.root / 'legacy-v1' / old['preparation_id'][:20] - for key, source in sources.items(): + for key, source in old['sources'].items(): self.assertEqual(set(source), {'path', 'sha256'}) target = legacy_dir / 'evidence' / key target.parent.mkdir(parents=True, exist_ok=True) @@ -603,7 +603,7 @@ def test_v1_migration_uses_fresh_analysis_and_separate_storage(self): def model(prompt, checkout): prompts.append(prompt) return response(fresh_analysis) - result = prep.prepare(package.manifest_path, package.root / 'versions', + result = prep.prepare(package.manifest_path, package.versions, previous_preparation=old_path, analyzer=model) migrated = prep.read(result['artifact']) self.assertEqual(len(prompts), 1) @@ -611,12 +611,12 @@ def model(prompt, checkout): self.assertIn('"preparation_version": 1', prompts[0]) self.assertEqual(result['refresh']['mode'], 'migration') self.assertEqual(result['refresh']['model_call_count'], 1) - self.assertEqual(migrated['preparation_version'], 2) + self.assertEqual(migrated['preparation_version'], 3) self.assertEqual(migrated['analysis'], fresh_analysis) self.assertTrue(migrated['parent']['migration']) self.assertEqual(migrated['parent']['preparation_id'], old['preparation_id']) self.assertEqual(migrated['parent']['path'], str(old_path)) - self.assertEqual(Path(result['artifact']).parent.parent, package.root / 'versions') + self.assertEqual(Path(result['artifact']).parent.parent, package.versions) self.assertEqual(prep.historical(old_path), old) self.assertEqual({path.relative_to(legacy_dir): path.read_bytes() for path in legacy_dir.rglob('*') if path.is_file()}, before) @@ -627,11 +627,11 @@ def stale_migration(*_): return response(fresh_analysis) try: with self.assertRaisesRegex(ValueError, 'invalid selected parent'): - prep.prepare(package.manifest_path, package.root / 'versions', + prep.prepare(package.manifest_path, package.versions, previous_preparation=old_path, analyzer=stale_migration) finally: snapshot.write_bytes(original) - self.assertEqual(list((package.root / 'versions').glob('*/preparation.json')), [Path(result['artifact'])]) + self.assertEqual(list((package.versions).glob('*/preparation.json')), [Path(result['artifact'])]) def test_graph_rejects_invalid_watches_dangling_references_and_missing_coverage(self): for mutate in (lambda v: v['graph'].pop(), @@ -646,7 +646,7 @@ def test_graph_rejects_invalid_watches_dangling_references_and_missing_coverage( value = response() mutate(value) with self.subTest(value=value), self.assertRaises(ValueError): - prep.prepare(self.package.manifest_path, self.package.root / 'versions', analyzer=lambda *_: value) + prep.prepare(self.package.manifest_path, self.package.versions, analyzer=lambda *_: value) def test_fifth_review_then_tree_equal_cleanup_completes_without_preparation_model(self): from shared import executors, review_sessions, review_coverage, review_reconciliation @@ -679,7 +679,7 @@ def test_fifth_review_then_tree_equal_cleanup_completes_without_preparation_mode self.assertNotEqual(package.head, reviewed_head) self.assertEqual(prep.git(package.repo, 'rev-parse', 'HEAD^{tree}'), reviewed_tree) with patch.object(prep, 'analyze') as model: - fresh = prep.prepare(package.manifest_path, package.root / 'versions', previous_preparation=first['artifact']) + fresh = prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact']) model.assert_not_called() self.assertEqual(fresh['refresh']['model_call_count'], 0) state.attach_preparation(record, fresh['artifact'], package.context['url'], package.head, @@ -694,7 +694,7 @@ def test_fifth_review_then_tree_equal_cleanup_completes_without_preparation_mode source = package.repo / 'transform.py' check = {'satisfied': True, 'rationale': 'Inspected current transform and validation evidence', 'evidence': [{'path': str(source), 'sha256': prep.file_hash(source)}]} - verification = package.write('verification.json', {'verification_version': 1, + verification = package.write('verification.json', {'verification_version': 2, 'engineering_id': fresh['engineering_id'], 'preparation_id': fresh['preparation_id'], 'pr_url': package.context['url'], 'head_sha': package.head, 'checks': {name: check for name in ('behavior', 'placement', 'test_sufficiency', 'compatibility', 'complexity')}}) tree = prep.git(package.repo, 'rev-parse', 'HEAD^{tree}') @@ -753,8 +753,8 @@ def verdict(key=None): else: self.commit({'new.py': 'a = 1\n'}) invalid['identity']['head_sha'] = package.head - guard = ('preparation source identity is stale' if changed == 'tree' else - 'reconciliation does not cover this review and preparation') + guard = ('preparation PR or head mismatch' if changed == 'tree' else + 'preparation artifact identity mismatch') with self.subTest(changed=changed), self.assertRaisesRegex(ValueError, guard): state.verify_review(record_path, invalid) @@ -968,7 +968,7 @@ def test_missing_original_feedback_content_blocks_before_model(self): package.write('conversation.json', conversation) with self.subTest(number=number), patch.object(prep, 'analyze') as model: with self.assertRaisesRegex(ValueError, 'feedback|comment'): - prep.prepare(package.manifest_path, package.root / 'versions') + prep.prepare(package.manifest_path, package.versions) model.assert_not_called() package.write('inline.json', self.inline_feedback()) package.write('conversation.json', self.conversation_feedback()) @@ -1091,7 +1091,7 @@ def test_feedback_schema_marker_requires_the_supported_integer_type(self): package.write('feedback.json', {**self.inline_feedback(), 'schemaVersion': marker}) with self.subTest(marker=marker), patch.object(prep, 'analyze') as model: with self.assertRaisesRegex(ValueError, 'unsupported complete feedback schema'): - prep.prepare(package.manifest_path, package.root / 'versions') + prep.prepare(package.manifest_path, package.versions) model.assert_not_called() def test_unknown_complete_claims_in_other_categories_keep_exact_bytes(self): @@ -1177,10 +1177,10 @@ def copying(path, content): with self.subTest(target=target, phase=phase, defect=defect), patch.object(Path, 'write_bytes', copying), \ patch.object(prep, 'policy', return_value=parent['inputs']['policy'] if phase == 'analyzer' else 'new-policy'): with self.assertRaisesRegex(ValueError, 'invalid selected parent'): - prep.prepare(package.manifest_path, package.root / 'versions', + prep.prepare(package.manifest_path, package.versions, previous_preparation=second['artifact'], full_analysis=True, analyzer=analyzer) self.assertEqual(len(damaged), 1) - self.assertEqual(set((package.root / 'versions').glob('*/preparation.json')), + self.assertEqual(set((package.versions).glob('*/preparation.json')), {Path(first['artifact']), Path(second['artifact'])} - ({snapshot} if defect == 'delete' else set())) finally: @@ -1207,7 +1207,7 @@ def changed_parent(*_): Path(first['artifact']).write_text('changed selected parent') return retain(parent) with self.assertRaisesRegex(ValueError, 'selected parent changed'): - prep.prepare(self.package.manifest_path, self.package.root / 'versions', + prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact'], analyzer=changed_parent) def test_review_session_resume_still_requires_exact_identity(self): @@ -1352,9 +1352,9 @@ def test_second_invalid_reassessment_blocks_without_publishing(self): for invalid in ({'dispositions': []}, {'dispositions': [{'id': []}], 'additions': [], 'impacts': []}): with patch('shared.executors.execute', return_value=json.dumps(invalid)) as model: with self.assertRaisesRegex(ValueError, 'invalid after one correction'): - prep.prepare(self.package.manifest_path, self.package.root / 'versions', previous_preparation=first['artifact']) + prep.prepare(self.package.manifest_path, self.package.versions, previous_preparation=first['artifact']) self.assertEqual(model.call_count, 2) - self.assertEqual(list((self.package.root / 'versions').glob('*/preparation.json')), [Path(first['artifact'])]) + self.assertEqual(list((self.package.versions).glob('*/preparation.json')), [Path(first['artifact'])]) def test_reviewer_instruction_changes_and_new_rules_invalidate_governing_conclusions(self): package = self.package diff --git a/runtime/dk-pr-decisions/tests/test_metadata_stability.py b/runtime/dk-pr-decisions/tests/test_metadata_stability.py new file mode 100644 index 0000000..9dca53c --- /dev/null +++ b/runtime/dk-pr-decisions/tests/test_metadata_stability.py @@ -0,0 +1,369 @@ +"""Fresh-contract regressions for identity, durable evidence and competing writers.""" +import copy +import importlib.util +import json +import multiprocessing +from pathlib import Path +import shutil +import sys +from unittest.mock import patch + +import pytest + +from preparation_fixtures import Package, analysis, descriptor, prep, response, retain +from test_preparation import module +from shared import review_coverage, review_sessions +from shared.command_results import classification +from shared.engineering_identity import engineering_id +from shared.private_state import read, write + +api = module('metadata_decisions', 'dk-pr-decisions/scripts/decisions.py') +state_api = module('metadata_run', 'dk-pr-iter/scripts/run_state.py') +runner = module('metadata_runner', 'dk-pr-review/scripts/run_reviewers.py') +completion = module('metadata_completion', 'dk-pr-iter/scripts/completion.py') + + +def state(url='https://github.com/o/r/pull/1'): + return {'version': 2, 'pr': {'url': url, 'number': 1, 'headRefOid': 'a' * 40, + 'title': 'Fixture', 'body': 'Scope'}, 'decisions': [], 'manual_decisions': [], + 'comments': {}, 'threads': {}, 'summaries': {}, 'publication_history': []} + + +def competing_writer(path, ready, proceed, queue): + current = read(Path(path), {}) + current['decisions'].append({'text': 'Stale choice', 'references': []}) + ready.set() + assert proceed.wait(10) + try: + api.save_state(current, Path(path)) + queue.put({'error': 'stale writer committed'}) + except api.MetadataConflict as exc: + queue.put({'code': exc.code, 'proposal': exc.proposal_path}) + + +def test_stale_process_preserves_choices_intents_and_history(tmp_path): + path = tmp_path / 'decisions.json' + initial = state() + api.save_state(initial, path) + ctx = multiprocessing.get_context('fork') + ready, proceed, queue = ctx.Event(), ctx.Event(), ctx.Queue() + process = ctx.Process(target=competing_writer, args=(str(path), ready, proceed, queue)) + process.start() + try: + assert ready.wait(10) + current = read(path, {}) + current['decisions'] = [{'text': 'Winning choice', 'references': []}] + current['publication_intent'] = {'id': 'intent', 'operation_id': 'owner', 'status': 'pending'} + current['publication_history'] = [dict(current['publication_intent'])] + api.save_state(current, path) + committed = path.read_bytes() + proceed.set() + result = queue.get(timeout=10) + process.join(10) + assert process.exitcode == 0 + assert result['code'] == 'metadata_conflict' + assert path.read_bytes() == committed + proposal = read(Path(result['proposal']), {}) + assert proposal['proposal']['decisions'][0]['text'] == 'Stale choice' + assert proposal['current_revision'] == 2 and proposal['expected_revision'] == 1 + assert proposal['next_action'] + with pytest.raises(api.MetadataConflict): + api.save_state(read(path, {}), path) + assert path.read_bytes() == committed + finally: + proceed.set() + process.join(10) + if process.is_alive(): + process.terminate() + process.join() + + +@pytest.mark.parametrize('applied', [False, True]) +def test_publication_interruption_requires_explicit_reconciliation_without_replay(tmp_path, applied): + path = tmp_path / 'state.json' + current = state() + current['decisions'] = [{'text': 'Preserve API', 'references': []}] + api.save_state(current, path) + live = {**current['pr']} + mutations = [] + def github(args): + if args[0] == 'api': + mutations.append(args) + if applied: + live['body'] = args[-1][5:] + raise KeyboardInterrupt + return dict(live) + with patch.object(api, 'gh_json', side_effect=github): + with pytest.raises(KeyboardInterrupt): + api.publish(current, current['decisions'], persist=lambda: api.save_state(current, path)) + pending = read(path, {}) + assert pending['publication_intent']['status'] == 'pending' + assert pending['publication_history'][0]['status'] == 'pending' + with pytest.raises(api.MetadataConflict): + api.save_state(pending, path) + recovered = read(path, {}) + result = api.publish(recovered, recovered['decisions'], reconcile=True, + persist=lambda: api.save_state(recovered, path)) + assert len(mutations) == 1 + assert not result['updated'] + saved = read(path, {}) + assert saved['publication_intent']['status'] == ('verified' if applied else 'not_applied') + assert saved['publication_history'][0] == saved['publication_intent'] + assert saved['revision'] > pending['revision'] + + +def test_legacy_decisions_import_leaves_pending_original_untouched(tmp_path, monkeypatch): + monkeypatch.setenv('XDG_DATA_HOME', str(tmp_path)) + path = api.cache_path('https://github.com/o/r/pull/1') + legacy = tmp_path / 'dk-pr-decisions/github.com/o/r/pr-1.json' + current = state() + current['publication_intent'] = {'id': 'uncertain', 'status': 'pending', + 'body': 'Scope with the pending choice', 'snapshot': dict(current['pr'])} + legacy.parent.mkdir(parents=True, exist_ok=True) + write(legacy, current) + before = legacy.read_bytes() + loaded, target = api.load_cache(current['pr']['url']) + assert target == path and loaded['revision'] == 1 + assert legacy.read_bytes() == before + with pytest.raises(api.MetadataConflict): + api.save_state(loaded, target) + assert loaded['publication_intent']['id'] == 'uncertain' + + +def test_conflict_envelope_is_a_workflow_block(tmp_path, monkeypatch, capsys): + path = tmp_path / 'state.json' + current = state() + api.save_state(current, path) + stale = copy.deepcopy(current) + api.save_state(current, path) + monkeypatch.setattr(api, 'cli', lambda: api.save_state(stale, path)) + assert api.main() == 1 + result = json.loads(capsys.readouterr().out) + assert result['schema_version'] == 2 and result['status'] == 'error' + assert result['error']['code'] == 'metadata_conflict' + assert Path(result['error']['proposal_path']).exists() + assert classification('decisions', 1, result)[0:2] == ('blocked', 'metadata_conflict') + + +def test_full_workflow_operational_refresh_resume_and_completion(tmp_path, monkeypatch): + package = Package(tmp_path) + names = ['example', 'second'] + package.manifest['mode'] = 'iteration' + package.manifest['evidence']['reviewers'].append(str(package.write('second.json', descriptor('second', package.repo / 'transform.py')))) + history = {'preparation_history_version': 1, 'governing': {'questions': []}, 'operational': {'cycles': 0}} + package.manifest['evidence']['run_history'] = [str(package.write('history.json', history))] + package.write('manifest.json', package.manifest) + first = package.prepare(analysis(names)) + prepared = prep.read(first['artifact']) + record, record_path = {}, tmp_path / 'run.json' + state_api.start_run(record) + state_api.attach_preparation(record, first['artifact'], package.context['url'], package.head) + state_api.next_cycle(record, package.head) + state_api.classify(record, 'feedback', package.head, 'valid', 'source', 'Confirmed') + state_api.assess(record, 'feedback', package.head, first['preparation_id'], 'within_scope', 'source', 'Local repair') + binding = state_api.review_session(record, package.head, record_path) + write(record_path, record) + monkeypatch.setattr(runner, 'describe_reviewer', lambda name: prepared['reviewers'][name]) + monkeypatch.setattr(runner, 'skill_path', lambda name: package.repo / 'transform.py') + session = review_sessions.Session(package.context, prepared, names, path=binding['session_file']) + with session.execution(), patch.object(runner, 'run_skill_reviewer', side_effect=[{'findings': []}, KeyboardInterrupt]): + with pytest.raises(KeyboardInterrupt): + runner.review(package.context, names, '', package.decisions, '', prepared, session=session) + old = read(session.path, {}) + checkpoint = Path(old['reviews'][names[0]]['checkpoint']['path']) + checkpoint_bytes = checkpoint.read_bytes() + check = {'satisfied': True, 'rationale': 'Inspected supported transform and evidence', + 'evidence': [{'path': prepared['sources']['production/0']['snapshot'], 'sha256': prepared['sources']['production/0']['sha256']}]} + verification = package.write('verification.json', {'verification_version': 2, + 'preparation_id': first['preparation_id'], 'engineering_id': first['engineering_id'], + 'pr_url': package.context['url'], 'head_sha': package.head, + 'checks': {key: check for key in ('behavior', 'placement', 'test_sufficiency', 'compatibility', 'complexity')}}) + history['operational']['cycles'] = 1 + package.write('history.json', history) + with patch.object(prep, 'analyze') as model: + fresh = prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact']) + model.assert_not_called() + assert first['preparation_id'] != fresh['preparation_id'] + assert first['engineering_id'] == fresh['engineering_id'] + prepared = prep.read(fresh['artifact']) + state_api.attach_preparation(record, fresh['artifact'], package.context['url'], package.head) + assert state_api.fix_plan(record, 'feedback', package.head)['action'] == 'fix' + assert record['assessments'][0]['preparation_id'] == first['preparation_id'] + resumed = review_sessions.Session(package.context, prepared, names, path=session.path, resume=True) + with resumed.execution(), patch.object(runner, 'run_skill_reviewer', return_value={'findings': []}) as calls: + result = runner.review(package.context, names, '', package.decisions, '', prepared, session=resumed) + item = resumed.finish(result) + assert calls.call_count == 1 and result['reused_reviewers'] == names[:1] + assert checkpoint.read_bytes() == checkpoint_bytes + assert read(session.path, {})['review_history'][:len(old['review_history'])] == old['review_history'] + state_api.register_review(record, {'review_artifact': item}, package.head) + write(record_path, record) + assert review_coverage.load(item)['version'] == 3 + assert read(session.path, {})['session_version'] == 2 + assert prep.verify_completion(verification, prepared)['preparation_id'] == first['preparation_id'] + assert completion.completion_gate(package.head, package.head, package.head, True, False, 'success', + stage='review', preparation=fresh['artifact'], run_record=record_path, + pr_url=package.context['url'], body='', verification=verification) == 'clean' + assert len(read(record_path, {})['cycles']) == 1 + # A completed immutable result and the earlier verification survive another counter refresh. + history['operational']['cycles'] = 2 + package.write('history.json', history) + final = prep.prepare(package.manifest_path, package.versions, previous_preparation=fresh['artifact']) + assert final['engineering_id'] == first['engineering_id'] + state_api.attach_preparation(record, final['artifact'], package.context['url'], package.head) + write(record_path, record) + assert state_api.verify_review(record_path, prep.read(final['artifact']))['preparation_id'] == fresh['preparation_id'] + assert completion.completion_gate(package.head, package.head, package.head, True, False, 'success', + stage='review', preparation=final['artifact'], run_record=record_path, + pr_url=package.context['url'], body='', verification=verification) == 'clean' + assert len(record['cycles']) == 1 + + +def test_durable_captures_survive_export_and_checkout_removal_live_rules_still_block(tmp_path): + package = Package(tmp_path) + # This fixture's descriptor is a live instruction; retain it separately from the disposable checkout. + instruction = package.write('instruction.json', {'procedure': 'Review the supported transform'}) + package.write('reviewer.json', descriptor('example', instruction)) + package.manifest['source_modes'] = {name + '/0': 'capture' for name in prep.EVIDENCE + if name not in {'reviewers', 'rules', 'user_instructions'}} + package.write('manifest.json', package.manifest) + result = package.prepare() + prepared = prep.read(result['artifact']) + live_paths = {s['path'] for s in prepared['sources'].values() if s['mode'] == 'live'} + for key, source in prepared['sources'].items(): + if source['mode'] == 'capture' and source['path'] not in live_paths: + Path(source['path']).unlink(missing_ok=True) + shutil.rmtree(package.repo) + assert prep.gate(result['artifact'], pr_url=package.context['url'], head_sha=package.head) == prepared + session = review_sessions.Session(package.context, prepared, ['example']) + assert session.path.is_relative_to(Path(prepared['workflow_dir'])) + refreshed = prep.prepare(prepared['recovery_manifest']['path'], package.versions, previous_preparation=result['artifact']) + assert refreshed['refresh']['model_call_count'] == 0 + assert refreshed['engineering_id'] == result['engineering_id'] + review_sessions.Session(prep.context_for(prep.read(refreshed['artifact'])), prep.read(refreshed['artifact']), + ['example'], path=session.path, resume=True) + instruction.unlink() + with pytest.raises((ValueError, OSError)): + prep.gate(result['artifact'], pr_url=package.context['url'], head_sha=package.head) + + +@pytest.mark.parametrize('category', ['issues', 'choices', 'user_instructions', 'rules', 'questions', 'feedback', 'run_history']) +def test_each_governing_source_invalidates_interrupted_reuse(tmp_path, category): + package = Package(tmp_path) + source = package.write('governing.json', {'verified_absence': True, 'reason': 'No initial obligation'}) + package.manifest['evidence'][category] = [str(source)] + package.write('manifest.json', package.manifest) + first = package.prepare() + old = prep.read(first['artifact']) + session = review_sessions.Session(package.context, old, ['example']) + session.record('example', 'completed', {'findings': []}) + before = session.path.read_bytes() + package.write('governing.json', {'verified_absence': True, 'reason': 'Changed governing obligation'}) + fresh = prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact'], + analyzer=lambda *_: retain(old, unmapped=['source:' + category + '/0'] if category in {'feedback', 'run_history'} else [])) + assert fresh['engineering_id'] != first['engineering_id'] + with pytest.raises(ValueError, match='changed evidence'): + review_sessions.Session(package.context, prep.read(fresh['artifact']), ['example'], path=session.path, resume=True) + assert session.path.read_bytes() == before + + +def test_unknown_fields_govern_reports_and_verified_lineage_do_not(tmp_path): + package = Package(tmp_path) + prepared = prep.read(package.prepare()['artifact']) + expected = engineering_id(prepared) + for key in ('refresh', 'parent'): + operational = copy.deepcopy(prepared) + operational[key] = {'bookkeeping': 'changed'} + assert engineering_id(operational) == expected + unknown = copy.deepcopy(prepared) + unknown['new_constraint'] = 'Must govern' + assert engineering_id(unknown) != expected + unknown = copy.deepcopy(prepared) + unknown['sources']['rules/0']['new_constraint'] = 'Must govern' + assert engineering_id(unknown) != expected + + +def test_legacy_active_runs_are_untouched_and_terminal_history_survives(tmp_path): + record = {'iteration_run': {'id': 'legacy', 'status': 'active'}, 'cycles': [{'number': 5, 'head': 'a' * 40}], + 'events': [{'kind': 'published'}], 'local_decisions': [{'text': 'Keep API', 'source': 'user'}]} + before = copy.deepcopy(record) + with pytest.raises(ValueError, match='retained runtime'): + state_api.start_run(record, resume=True) + assert record == before + record['iteration_run']['status'] = 'completed' + state_api.start_run(record) + assert record['iteration_run']['contract_version'] == 2 + assert record['run_history'][0]['cycles'] == before['cycles'] + assert record['local_decisions'] == before['local_decisions'] + assert record['events'] == before['events'] + + +def test_invalid_retained_binding_blocks_before_consuming_another_cycle(tmp_path): + from shared import bindings + from shared.runtime_bundles import retained_binding + provider = tmp_path / 'provider.py' + provider.write_text('print("review fixture")\n') + selected = {'command': [sys.executable, str(provider), '{handoff}']} + name = bindings.key(selected) + retained = retained_binding({'binding': selected, 'identity': name, 'executor': 'command'}) + (tmp_path / 'evidence').mkdir() + package = Package(tmp_path / 'evidence') + package.manifest['mode'] = 'iteration' + procedure = Path(retained['argv'][1]) + value = descriptor(name, procedure) + value['execution'] = retained + package.write('reviewer.json', value) + package.write('manifest.json', package.manifest) + result = package.prepare(analysis([name])) + record = {} + state_api.start_run(record) + state_api.attach_preparation(record, result['artifact'], package.context['url'], package.head) + state_api.next_cycle(record, package.head) + before = copy.deepcopy(record) + procedure.chmod(0o600) + procedure.write_text('print("tampered")\n') + with pytest.raises((ValueError, RuntimeError)): + state_api.next_cycle(record, package.head) + assert record == before and len(record['cycles']) == 1 + + +def test_conflict_during_refresh_retains_the_requested_choice(tmp_path, monkeypatch, capsys): + path = tmp_path / 'state.json' + current = state() + api.save_state(current, path) + stale = copy.deepcopy(current) + api.save_state(current, path) + request = {'text': 'Keep the selected API', 'references': []} + monkeypatch.setattr(sys, 'argv', ['decisions.py', '--pr', current['pr']['url'], 'add', + '--json', json.dumps(request), '--user-instruction', 'Keep the API']) + monkeypatch.setattr(api, 'scan', lambda *_: api.save_state(stale, path)) + assert api.main() == 1 + result = json.loads(capsys.readouterr().out) + proposal = read(Path(result['error']['proposal_path']), {}) + assert json.loads(proposal['request']['json']) == request + assert proposal['request']['user_instruction'] == 'Keep the API' + + +def test_unknown_prior_evidence_cannot_hide_as_lineage_bookkeeping(tmp_path): + package = Package(tmp_path) + first = package.prepare() + parent = prep.read(first['artifact']) + package.manifest['evidence']['prior_preparation'] = [str(package.write('prior.json', { + 'verified_absence': True, 'reason': 'No prior run', 'unresolved_constraint': 'Preserve compatibility'}))] + package.write('manifest.json', package.manifest) + fresh = prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact'], + analyzer=lambda *_: retain(parent, unmapped=['source:prior_preparation/0'])) + assert fresh['engineering_id'] != first['engineering_id'] + + +def test_artifact_storage_and_verified_lineage_do_not_change_engineering_identity(tmp_path): + package = Package(tmp_path) + first = package.prepare() + second = package.prepare() + assert second['preparation_id'] != first['preparation_id'] + assert second['engineering_id'] == first['engineering_id'] + package.manifest['evidence']['prior_preparation'] = [first['artifact']] + package.write('manifest.json', package.manifest) + grown = prep.prepare(package.manifest_path, package.versions, previous_preparation=first['artifact']) + assert grown['refresh']['model_call_count'] == 0 + assert grown['engineering_id'] == first['engineering_id'] diff --git a/runtime/dk-pr-decisions/tests/test_persistent_storage.py b/runtime/dk-pr-decisions/tests/test_persistent_storage.py index 8271482..4faefa1 100644 --- a/runtime/dk-pr-decisions/tests/test_persistent_storage.py +++ b/runtime/dk-pr-decisions/tests/test_persistent_storage.py @@ -109,7 +109,7 @@ def update(self, **fields): live.write_text(json.dumps(data)) def path(self): - return Path(env['DK_PR_DECISIONS_HOME']) / core.storage.identity(URL) + return Path(env['DK_PR_DECISIONS_HOME']) / 'v3' / core.storage.identity(URL) result = Harness() result.env, result.live, result.root = env, live, tmp_path @@ -150,7 +150,7 @@ def test_isolation_and_exclusive_root(harness): other = str(harness.root / 'empty-store') assert harness.invoke('list', DK_PR_DECISIONS_HOME=other, code=1)['status'] == 'error' assert not Path(other).exists() - assert harness.path() == harness.root / 'persistent/github.com/base/project/pr-34.json' + assert harness.path() == harness.root / 'persistent/v3/github.com/base/project/pr-34.json' def test_competing_adds_and_refreshes_do_not_lose_changes(harness): @@ -160,12 +160,26 @@ def test_competing_adds_and_refreshes_do_not_lose_changes(harness): '--user-instruction', f'Exact instruction {i}', TEST_SCAN_DELAY='.04')) if i % 2 == 0: processes.append(harness.start('scan', TEST_SCAN_DELAY='.04')) + conflicts = [] for process in processes: - harness.finish(process) + out, err = process.communicate(timeout=15) + assert not err, err + result = json.loads(out) + assert process.returncode in (0, 1), result + if process.returncode: + assert result['error']['code'] == 'metadata_conflict' + proposal = json.loads(Path(result['error']['proposal_path']).read_text()) + conflicts.append(proposal) + # Explicitly inspect and reapply retained choices against the latest revision. + for proposal in conflicts: + request = proposal['request'] + if request['command'] == 'add': + choice = json.loads(request['json']) + harness.add(choice['text'], request['user_instruction']) records = harness.invoke('context')['data']['decisions'] assert {r['text'] for r in records} == {f'Choice {i}' for i in range(8)} assert {r['source'] for r in records} == {f'Exact instruction {i}' for i in range(8)} - assert json.loads(harness.path().read_text())['run_number'] == 13 + assert json.loads(harness.path().read_text())['revision'] > 0 def test_competing_replacements_preserve_one_chain_across_restarts(harness): @@ -238,7 +252,8 @@ def test_uncertain_publication_receipt_survives_kill_and_refresh(harness): intent = state['publication_intent'] assert intent['status'] == 'pending' assert state['publication_history'][-1] == intent - harness.invoke('scan') + blocked = harness.invoke('scan', code=1) + assert blocked['error']['code'] == 'metadata_conflict' assert json.loads(harness.path().read_text())['publication_intent'] == intent result = harness.invoke('publish', '--reconcile') assert result['publication']['reconciled'] == 'published' @@ -292,7 +307,7 @@ def test_temporary_imports_and_report_share_storage_without_publication(harness) def test_offline_list_has_no_writes_or_lock_creation(harness): harness.add('Saved') - harness.path().with_suffix('.lock').unlink() + harness.path().with_name(harness.path().name + '.lock').unlink() root = harness.root / 'persistent' before = {p: (p.read_bytes(), p.stat().st_mtime_ns) for p in root.rglob('*') if p.is_file()} bin_dir = harness.root / 'bin' @@ -360,7 +375,7 @@ def test_default_legacy_paths_and_private_synchronized_writes(tmp_path, monkeypa monkeypatch.delenv('DK_PR_DECISIONS_HOME', raising=False) monkeypatch.setenv('XDG_DATA_HOME', str(tmp_path / 'data')) path = core.cache_path(URL) - assert path == tmp_path / 'data/dk-pr-decisions/github.com/base/project/pr-34.json' + assert path == tmp_path / 'data/dk-pr-decisions/v3/github.com/base/project/pr-34.json' with patch.object(core.storage, 'sync_directory', wraps=core.storage.sync_directory) as sync: core.atomic_write(path, '{}\n') assert path.stat().st_mode & 0o777 == 0o600 @@ -369,4 +384,4 @@ def test_default_legacy_paths_and_private_synchronized_writes(tmp_path, monkeypa assert sync.call_args_list[-1].args == (path.parent,) monkeypatch.delenv('XDG_DATA_HOME') with patch.object(Path, 'home', return_value=tmp_path): - assert core.cache_path(URL) == tmp_path / '.local/share/dk-pr-decisions' / core.storage.identity(URL) + assert core.cache_path(URL) == tmp_path / '.local/share/dk-pr-decisions/v3' / core.storage.identity(URL) diff --git a/runtime/dk-pr-decisions/tests/test_preparation.py b/runtime/dk-pr-decisions/tests/test_preparation.py index 53100da..ef41e0f 100644 --- a/runtime/dk-pr-decisions/tests/test_preparation.py +++ b/runtime/dk-pr-decisions/tests/test_preparation.py @@ -509,7 +509,7 @@ def test_completion_cannot_bypass_pending_or_stale_preparation(self): package.write('run-record.json', record) self.assertEqual(completion.completion_gate(*args, stage='review', preparation=saved['artifact'], pr_url=package.context['url']), 'run_record_missing') - verification = {'verification_version': 1, 'preparation_id': saved['preparation_id'], + verification = {'verification_version': 2, 'engineering_id': saved['engineering_id'], 'preparation_id': saved['preparation_id'], 'pr_url': package.context['url'], 'head_sha': package.head, 'checks': {name: {'satisfied': True, 'rationale': 'Inspected current transform and unit evidence', 'evidence': [{'path': str(package.repo / 'transform.py'), @@ -553,7 +553,7 @@ def test_completion_rechecks_live_run_record_after_same_head_human_choice(self): check = {'satisfied': True, 'rationale': 'Inspected the supported behavior and its boundaries', 'evidence': [{'path': str(source), 'sha256': prep.file_hash(source)}]} verification_path = package.write('verification.json', { - 'verification_version': 1, 'preparation_id': saved['preparation_id'], + 'verification_version': 2, 'engineering_id': saved['engineering_id'], 'preparation_id': saved['preparation_id'], 'pr_url': package.context['url'], 'head_sha': package.head, 'checks': {name: check for name in ('behavior', 'placement', 'test_sufficiency', 'compatibility', 'complexity')}}) args = (package.head, package.head, package.head, True, False, 'success') diff --git a/runtime/dk-pr-iter/references/iteration-preflight.md b/runtime/dk-pr-iter/references/iteration-preflight.md index 401d153..0983edb 100644 --- a/runtime/dk-pr-iter/references/iteration-preflight.md +++ b/runtime/dk-pr-iter/references/iteration-preflight.md @@ -4,7 +4,7 @@ Read and follow [metadata readiness](metadata.md) first. Complete the tool, conf 1. Identify the PR's base repository and its head repository, branch, and SHA. Use the PR URL for GitHub queries and CI waits so a checkout of a fork cannot select another PR by branch name. Run `$dk-config --pr PR_URL refresh`, read applicable repository rules and `AGENTS.md`. Read the linked issue, PR scope, and live `## Decisions` section. Check the worktree and push access to the head repository, including forks. 2. Fetch the **complete** unresolved-thread snapshot using [dk-pr-address-reviews](../../dk-pr-address-reviews/procedure.md)'s deterministic fetcher. Require successful exit and `complete: true`. Use its PR title, description, head repository and branch, base and head SHAs, and closing issue references as the saved context; avoid redundant `gh pr view` reads of unchanged fields. Always fetch top-level reviews and general PR comments with that skill's `fetch_pr_conversation.py --snapshot-file SNAPSHOT_FILE --cwd REPO`, even when the unresolved inline list is empty. Require successful exit, `complete: true`, and the same PR URL and head. Use the inline snapshot's `items` plus the conversation artifact's `items` as the feedback worklist. Account for each `reference` using the full conversation, current PR head, and base. Read all later top-level comments before deciding whether an earlier request needs a reply; these comments have no resolved flag. Recheck head SHA before each mutation; an external head change stops the current agent or CI wait and requires fresh preflight and classification. -3. Create or inspect the per-PR record outside the repository using `scripts/run_state.py`. For a new request after a terminal run, use `start-run`; this archives the prior run and starts a distinct run with a fresh five-cycle budget. Use `resume-run` only for interrupted active work, preserving its counters. Never treat a completed run as a resume. An active run blocks `start-run`; reconcile and record its actual terminal outcome before a later user request can start another. For a legacy record, use `adopt-run` to preserve its counters, then explicitly record the known outcome; never silently reset an exhausted budget. Do not finish and restart within one request to evade a cap. Preserve publication events, reply-controller history, local decisions, and per-head rerun limits across runs. Record thread classifications, evidence URLs, decisions with sources (`decision --text TEXT --source SOURCE`), deferred items, pushes, replies, CI attempts, and cycle counts. Keep this record as operational history. Immediately persist agreed PR-level choices through Decisions `add` or explicit `replace`, preserving the exact user instruction or source evidence; a run-record entry alone does not persist a choice in Decisions. Restore `local_decisions` from this record on resume as temporary context, reconciling them against active persistent Decisions so superseded choices cannot reactivate. Export `dk-pr-decisions/scripts/decisions.py --pr PR_URL context --local-decisions-file RUN_RECORD.json` to a JSON file outside the repository. Require `schema_version: 2`, `status: ok`, and `data.verified: true`. Read active entries in `data.decisions`; superseded entries are history. Pass its path and a concise decision summary to every code fixing, review cycle, and CI repair agent. Include the preparation path/ID once assembled in step 4. Refresh it after a new decision, PR body edit, or head change. Missing, unreadable, malformed, stale, or conflicting context blocks dependent work until reconciled. Before any publication, reconcile the record with live GitHub state so an interrupted run cannot duplicate a push, decision, reply, or rerun. +3. Create or inspect the per-PR record outside the repository using `scripts/run_state.py`. For a new request after a terminal run, use `start-run`; this archives the prior run and starts a distinct run with a fresh five-cycle budget. Use `resume-run` only for interrupted active work, preserving its counters. Never treat a completed run as a resume. An active run blocks `start-run`; reconcile and record its actual terminal outcome before a later user request can start another. For a legacy active record, resume through its retained runtime; the new contract cannot resume it. Never silently reset an exhausted budget. Do not finish and restart within one request to evade a cap. Preserve publication events, reply-controller history, local decisions, and per-head rerun limits across runs. Record thread classifications, evidence URLs, decisions with sources (`decision --text TEXT --source SOURCE`), deferred items, pushes, replies, CI attempts, and cycle counts. Keep this record as operational history. Immediately persist agreed PR-level choices through Decisions `add` or explicit `replace`, preserving the exact user instruction or source evidence; a run-record entry alone does not persist a choice in Decisions. Restore `local_decisions` from this record on resume as temporary context, reconciling them against active persistent Decisions so superseded choices cannot reactivate. Export `dk-pr-decisions/scripts/decisions.py --pr PR_URL context --local-decisions-file RUN_RECORD.json` to a JSON file outside the repository. Require `schema_version: 2`, `status: ok`, and `data.verified: true`. Read active entries in `data.decisions`; superseded entries are history. Pass its path and a concise decision summary to every code fixing, review cycle, and CI repair agent. Include the preparation path/ID once assembled in step 4. Refresh it after a new decision, PR body edit, or head change. Missing, unreadable, malformed, stale, or conflicting context blocks dependent work until reconciled. Before any publication, reconcile the record with live GitHub state so an interrupted run cannot duplicate a push, decision, reply, or rerun. 4. Before handling local changes, rebasing, initial fixes or review, follow [engineering preparation](../../dk-pr-decisions/references/preparation.md). Pin an isolated remote checkout with `dk-pr-review/scripts/review_context.py`, discover configured effective reviewer descriptors, and assemble the complete original-evidence manifest including exact user instructions, local choices, questions and prior preparation. Run Decisions `prepare` in `iteration` mode. Require `ready`; register it with `run_state.py --repo HOST/OWNER/REPO --pr NUMBER preparation --file PREPARATION.json --pr-url PR_URL --head SHA`. Before dependent mutations and initial/CI fix handoffs run `work-gate --head SHA` and recheck live PR scope/head. Refresh after code, scope, answer or instruction changes without resetting counters or assessments. Retain immutable snapshots for `run_history` evidence so routine operational events do not invalidate their own preparation; work gates separately detect changed local decisions. A material proposal or pending synchronization blocks affected work even after clean reviews. 5. Before rebasing, inspect staged, unstaged, and untracked changes (`git status --porcelain=v1 --untracked-files=all`); read their contents and compare each change with the PR scope, current head, settled Decisions, and any review request it may address. Validate in-scope changes, stage only relevant files or hunks, and commit cohesive work to the PR branch using `dk-commit-message`; include those commits in the rebase, review, and publication flow. Discard only changes proven to contain no unique work, such as generated artifacts or edits already present on the PR head, and only after preserving a recoverable copy outside the repository; inspect and remove staged and untracked content explicitly rather than using a blanket clean or reset. Preserve unrelated work and continue from a clean linked worktree at the verified PR head when possible. If ownership, scope, or whether a change is safe to discard remains uncertain, stop and ask the human before touching it. Report how every initial change was handled. Stop on an unsafe or ambiguous branch, incomplete evidence, or missing reviewers. Fetch the target branch from the base repository, verify its remote tip and the PR head against the head repository again, and rebase the PR commits onto that tip before classifying or reviewing. Resolve conflicts against the PR's intended scope and validate the result. Publish the rewritten branch only to the verified head repository with `git push --force-with-lease=:` after rechecking both remote refs; a lease failure or external head change requires fresh preflight. Recheck the base target tip before each review cycle and before publishing a code change; if it advanced, rebase again, validate, and restart the affected review or CI observation against the new head. Never rewrite unrelated commits or use a plain `--force` push. Refresh the thread snapshot, decision context and preparation against the resulting head before classifying or reviewing. @@ -13,3 +13,5 @@ Keep large evidence outside the conversation: save complete thread snapshots, co Spawn agents for this workflow with `fork_turns="none"`. Give each a short, self-contained handoff: its task and permitted actions, PR URL and repository path, pinned head and any relevant base, applicable user constraints, required skill or procedure, artifact paths, and the specific references or failure excerpts it needs. The agent reads full evidence from those files when needed. Do not paste the parent conversation, broad diffs, thread snapshots, or run record into the handoff. Include the governing preparation path/ID, selected remedy, principal production/test locations, reusable helpers/fixtures, behavior and test scenarios, commands and stopping criteria. Workers record routine adjustments with rationale and return material ownership, interaction, design or validation changes to the coordinator before dependent work. Include the selected completion prompt verbatim when handing off completion. Include the waiting and message policy below in every self-contained handoff, including when delegating this entire workflow; it applies to the caller, workflow agent, and nested workers. For an interrupted invocation entering a repaired runtime, follow [the retained-run recovery procedure](run-record.md#recovery-into-the-repaired-runtime) before resuming; preserve original reviewer packages, counters and cleanup recovery evidence. + +Fresh contracts use returned private workflow paths for generated exports and handoffs. Keep required external rules/instructions live, and select capture mode only for generated evidence. Retain both preparation and engineering IDs. Treat `metadata_conflict` as a block requiring explicit proposal reconciliation. diff --git a/runtime/dk-pr-iter/references/run-record.md b/runtime/dk-pr-iter/references/run-record.md index 4500cb0..d777eec 100644 --- a/runtime/dk-pr-iter/references/run-record.md +++ b/runtime/dk-pr-iter/references/run-record.md @@ -79,7 +79,7 @@ reassessment. A parentless artifact cannot replace an existing binding; an expli idempotent only while that same artifact remains active. Registration remains one locked mutation and preserves the existing run and review budgets. `assessments` retains immutable scope dispositions keyed by feedback reference, code -head and preparation ID, with sources and rationale. A later preparation can change +head and engineering ID for fresh runs, with exact preparation provenance, sources and rationale. A later preparation can change scope at the same code head without erasing the previous assessment or classification. Validity remains in `threads`; an excluded valid defect never becomes `unsupported`. Only a `fix-plan` action of `fix` permits the fix handoff. It returns principal code/test @@ -190,6 +190,8 @@ Report review readiness separately from cleanup and completion readiness. ## Recovery into the repaired runtime +Legacy active runs without `contract_version: 2` must continue through their original retained runtime. This recovery procedure applies to compatible contract-v2 runs only. + First verify that the old invocation and all its children have stopped. Inspect their retained lifecycle records with `update.py inspect --host HOST`, and explicitly complete each stopped child's token before its parent using the normal @@ -197,7 +199,7 @@ explicitly complete each stopped child's token before its parent using the norma records; never delete them or infer liveness from age or a launcher PID alone. Enter the repaired installed package normally and use its returned retained -`runtime_root`, `completion_script` and new lifecycle token. Resume the existing +`runtime_root`, `completion_script` and new lifecycle token. For a run already marked `contract_version: 2`, resume the existing active iteration with `resume-run`; do not start a replacement run or reset the five-cycle budget. Validate the retained reviewer packages, original session and result, original preparation and evidence copies, current preparation, candidate, @@ -213,3 +215,14 @@ provenance or disagreement remains a blocker. After verified publication, refres final-head preparation, reconcile against the original review with the published assessment, refresh engineering verification and exact-head CI, and run the complete gate. Retain the original artifacts and counters throughout recovery. + + +New `iteration_run` records carry `contract_version: 2` and a private `workflow_dir`. +Fresh runs require preparation v3. `resume-run`, work and preparation activation +reject legacy active runs with an instruction to use their retained runtime; they +never rewrite those records. After a terminal legacy run, normal `start-run` archives +history and preserves cross-run publication, choice and rerun records as before. +Before allocating a review cycle, validate retained reviewer bindings. A failed +validation consumes no new cycle and never refunds previously consumed cycles. +Operational preparation refresh preserves assessment/result/verification reuse by +runtime-computed engineering ID while retaining original preparation references. diff --git a/runtime/dk-pr-iter/scripts/completion.py b/runtime/dk-pr-iter/scripts/completion.py index ffb44d4..715fc32 100644 --- a/runtime/dk-pr-iter/scripts/completion.py +++ b/runtime/dk-pr-iter/scripts/completion.py @@ -42,7 +42,7 @@ def current_preparation(path, run_record, pr_url, head, body): if not isinstance(record, dict): raise ValueError('iteration run record must be an object') current = run_state_api.work_gate(record, head) - if (current['preparation_id'] != prepared['preparation_id'] + if (preparation_api.engineering_id(current) != preparation_api.engineering_id(prepared) or current['identity']['pr_url'] != pr_url): raise ValueError('completion preparation differs from the active run record') return prepared diff --git a/runtime/dk-pr-iter/scripts/run_state.py b/runtime/dk-pr-iter/scripts/run_state.py index c62984a..0e768e1 100644 --- a/runtime/dk-pr-iter/scripts/run_state.py +++ b/runtime/dk-pr-iter/scripts/run_state.py @@ -23,6 +23,9 @@ from shared.private_state import root, read, write, mutate from shared.flaky_tests import Registry, record_flaky from shared import review_coverage, review_sessions, review_reconciliation +from shared.engineering_identity import engineering_id, compatible_reference +from shared import bindings +from shared.workflow_storage import allocate _spec = importlib.util.spec_from_file_location('iteration_preparation', next(p for p in Path(__file__).resolve().parents if (p / 'shared').is_dir()) / 'dk-pr-decisions/scripts/preparation.py') @@ -36,6 +39,8 @@ def start_run(record, *, resume=False): if resume: if not active or active['status'] != 'active': raise ValueError('no active iteration run to resume') + if active.get('contract_version') != 2: + raise ValueError('legacy active run; resume through its retained runtime') return active if active and active['status'] == 'active': raise ValueError('iteration run is active; resume or finish it before starting another') @@ -47,7 +52,8 @@ def start_run(record, *, resume=False): for field in ('cycles', 'cycle_retries', 'repairs', 'review_results', 'review_sessions', 'review_reconciliations', 'cleanup_prechecks'): record[field] = [] record.pop('preparation', None) - record['iteration_run'] = {'id': uuid.uuid4().hex, 'status': 'active'} + record['iteration_run'] = {'id': uuid.uuid4().hex, 'status': 'active', + 'contract_version': 2, 'workflow_dir': str(allocate())} return record['iteration_run'] @@ -69,7 +75,12 @@ def finish_run(record, status, reason): def attach_preparation(record, path, pr_url, head, *, expected_parent=None): + run = record.get('iteration_run', {}) + if run and run.get('contract_version') != 2: + raise ValueError('legacy active run; use its retained runtime') artifact = preparation_api.gate(path, pr_url=pr_url, head_sha=head, require_ready=False) + if run.get('contract_version') == 2 and artifact.get('preparation_version') != 3: + raise ValueError('fresh runs require preparation v3; legacy runs use their retained runtime') parent = artifact.get('parent') expected = parent['preparation_id'] if parent else None active = record.get('preparation', {}).get('id') @@ -84,11 +95,12 @@ def attach_preparation(record, path, pr_url, head, *, expected_parent=None): raise ValueError('active preparation changed; refresh from the current preparation') if artifact['mode'] != 'iteration': raise ValueError('iteration requires iteration preparation') - raw = preparation_api.read(artifact['sources']['decisions']['path']) + raw = preparation_api.read(preparation_api.source_path(artifact, 'decisions')) decisions = raw.get('data', raw) if any(choice not in decisions['local_decisions'] for choice in record.get('local_decisions', [])): raise ValueError('preparation omits run-record local choices') item = {'path': str(Path(path).resolve()), 'id': artifact['preparation_id'], + 'engineering_id': engineering_id(artifact), 'head': head, 'pr_url': pr_url, 'local_decisions_hash': preparation_api.digest(record.get('local_decisions', []))} if run.get('id'): @@ -101,6 +113,9 @@ def attach_preparation(record, path, pr_url, head, *, expected_parent=None): def work_gate(record, head, *, require_ready=True): + active = record.get('iteration_run', {}) + if active and active.get('contract_version') != 2: + raise ValueError('legacy active run; use its retained runtime') if record.get('iteration_run') and record['iteration_run']['status'] != 'active': raise ValueError('iteration run is finished; start a new run') item = record.get('preparation') @@ -124,22 +139,23 @@ def assess(record, ref, head, preparation_id, disposition, evidence, reason): if disposition not in {'within_scope', 'outside_scope', 'decision_blocked'} or not all((ref, evidence, reason)): raise ValueError('scope assessment requires disposition, evidence and rationale') item = {'reference': ref, 'head': head, 'preparation_id': preparation_id, + 'engineering_id': engineering_id(artifact), 'disposition': disposition, 'evidence': evidence, 'reason': reason} entries = record.setdefault('assessments', []) - old = next((entry for entry in entries if all(entry[k] == item[k] - for k in ('reference', 'head', 'preparation_id'))), None) - if old and old != item: + old = next((entry for entry in entries if entry['reference'] == ref and entry['head'] == head + and compatible_reference(entry, artifact)), None) + if old and any(old[k] != item[k] for k in ('disposition', 'evidence', 'reason')): raise ValueError('assessment already recorded; refresh preparation to reassess') if not old: entries.append(item) - return item + return old or item def fix_plan(record, ref, head): artifact = work_gate(record, head, require_ready=False) assessment = next((item for item in record.get('assessments', []) if item['reference'] == ref and item['head'] == head - and item['preparation_id'] == artifact['preparation_id']), None) + and compatible_reference(item, artifact)), None) if not assessment: raise ValueError('scope and remediation-depth assessment required before a fix') # Scope is evaluated first. A small pre-existing defect never overrides an exclusion. @@ -194,7 +210,9 @@ def append_unique(record, field, item, key): def next_cycle(record, head): require_head(head) - work_gate(record, head) + prepared = work_gate(record, head) + with bindings.pinned_bindings(prepared['reviewers']): + pass number = len(record.setdefault("cycles", [])) + 1 if number > 5: raise ValueError("five review cycles exhausted") @@ -221,8 +239,9 @@ def review_session(record, head, record_path): entries = record.setdefault('review_sessions', []) item = next((v for v in entries if all(v[k] == value for k, value in identity.items())), None) if item is None: + directory = record.get('iteration_run', {}).get('workflow_dir', str(Path(record_path).resolve().parent)) item = {**identity, 'head': head, 'session_file': str( - Path(record_path).resolve().parent / 'review-sessions' / (uuid.uuid4().hex + '.json'))} + Path(directory) / 'review-sessions' / (uuid.uuid4().hex + '.json'))} entries.append(item) if item['head'] != head: raise ValueError('iteration review session head changed') @@ -244,7 +263,7 @@ def register_review(record, output, head): for key in ('path', 'sha256'))): raise ValueError('review_artifact requires nonempty string path and sha256') artifact = review_sessions.result_artifact(binding['session_file'], item) - if (artifact['head'] != head or artifact['preparation_id'] != prepared['preparation_id'] + if (artifact['head'] != head or not compatible_reference(artifact, prepared) or artifact['pr_url'] != prepared['identity']['pr_url']): raise ValueError('review result differs from the active preparation') registered = {**item, **identity, 'session_file': binding['session_file']} @@ -345,7 +364,9 @@ def verify_findings(record, artifact): classification = record.get('threads', {}).get(ref, {}).get(artifact['head']) assessment = next((a for a in record.get('assessments', []) if a['reference'] == ref and a['head'] == artifact['head'] - and a['preparation_id'] == artifact['preparation_id']), None) + and (a.get('engineering_id') == artifact.get('engineering_id') + if artifact.get('version') == 3 else + a['preparation_id'] == artifact['preparation_id'])), None) if not classification or not assessment: raise ValueError('review finding lacks validity and scope assessment: ' + ref) if assessment['disposition'] == 'decision_blocked' or ( @@ -361,7 +382,7 @@ def legacy_review_options(record_path, context, prepared, reviewers, scope, rule head = context['head_sha'] def allocate(record): current = work_gate(record, head) - if current['preparation_id'] != prepared['preparation_id']: + if engineering_id(current) != engineering_id(prepared): raise ValueError('legacy review preparation differs from the active iteration record') item = review_session(record, head, record_path) if stats_cycle is not None and stats_cycle != item['cycle']: @@ -382,6 +403,8 @@ def allocate(record): result['reused_reviewers'] = result['executed_reviewers'] + result['reused_reviewers'] result['executed_reviewers'] = [] result.update(review_session={'id': session['id'], 'path': str(path)}, review_artifact=item) + if artifact.get('version') == 3: + result.update(preparation_id=artifact['preparation_id'], engineering_id=artifact['engineering_id']) options['result'] = result return options @@ -389,7 +412,7 @@ def allocate(record): def legacy_register_review(record_path, result, head): record_path = Path(record_path) prepared = work_gate(read(record_path, {}), head) - context = read(Path(prepared['sources']['context']['path']), {}) + context = preparation_api.context_for(prepared) return mutate(review_coverage.storage_path(record_path, context), {}, lambda record: register_review(record, result, head)) @@ -592,7 +615,8 @@ def main(): elif a.command == 'preparation': result = mutate(path, default, lambda d: attach_preparation(d, a.file, a.pr_url, a.head, expected_parent=a.expected_parent)) elif a.command == 'work-gate': artifact = work_gate(read(path, default), a.head) - result = {'gate': 'ready', 'preparation_id': artifact['preparation_id']} + result = {'gate': 'ready', 'preparation_id': artifact['preparation_id'], + 'engineering_id': engineering_id(artifact)} elif a.command == 'assess': result = mutate(path, default, lambda d: assess(d, a.ref, a.head, a.preparation_id, a.disposition, a.evidence, a.reason)) elif a.command == 'fix-plan': result = fix_plan(read(path, default), a.ref, a.head) elif a.command == "thread": result = mutate(path, default, lambda d: classify(d, a.ref, a.head, a.classification, a.evidence, a.reason, a.production_lines)) diff --git a/runtime/dk-pr-iter/tests/test_iter.py b/runtime/dk-pr-iter/tests/test_iter.py index c7533ec..b210365 100644 --- a/runtime/dk-pr-iter/tests/test_iter.py +++ b/runtime/dk-pr-iter/tests/test_iter.py @@ -160,7 +160,7 @@ def observer(*args): def prepared_boundaries(monkeypatch): # These existing tests isolate CI/series/counter policy. Preparation's real # gates and resume interactions are exercised in test_preparation.py. - monkeypatch.setattr(state, 'work_gate', lambda record, head: {'readiness': 'ready'}) + monkeypatch.setattr(state, 'work_gate', lambda record, head: {'readiness': 'ready', 'reviewers': {}}) monkeypatch.setattr(completion, 'current_preparation', lambda *a: {'mode': 'iteration'}) monkeypatch.setattr(completion.run_state_api, 'verify_review', lambda *a, **kw: {}) monkeypatch.setattr(completion, 'live_preparation_scope', lambda *a: {'body': '', 'headRefOid': SHA}) diff --git a/runtime/dk-pr-iter/tests/test_review_reconciliation.py b/runtime/dk-pr-iter/tests/test_review_reconciliation.py index f56a90a..c73374f 100644 --- a/runtime/dk-pr-iter/tests/test_review_reconciliation.py +++ b/runtime/dk-pr-iter/tests/test_review_reconciliation.py @@ -201,7 +201,7 @@ def run(self): def verification(self, prepared=None): prepared = prepared or self.current return self.package.write('verification.json', { - 'verification_version': 1, 'preparation_id': prepared['preparation_id'], + 'verification_version': 2, 'engineering_id': prepared['engineering_id'], 'preparation_id': prepared['preparation_id'], 'head_sha': prepared['identity']['head_sha'], 'pr_url': prepared['identity']['pr_url'], 'checks': {name: {'satisfied': True, 'rationale': 'Current transform and tests inspected', 'evidence': [prepared['sources']['production/0']]} @@ -409,7 +409,7 @@ def test_tampered_or_stale_evidence_never_supplies_coverage(scenario, kind): elif kind.startswith('assessment'): target = s.assessment_path elif kind == 'current_source': - target = Path(s.current['sources']['feedback/0']['path']) + target = Path(s.current['sources']['feedback/0']['snapshot']) else: record = read(s.path, {}) if kind == 'run': record['iteration_run']['id'] = 'changed' @@ -433,8 +433,12 @@ def test_movement_during_comparison_registers_nothing(scenario, kind): s = scenario s.refresh() def move(): - if kind == 'head': s.git('commit', '--allow-empty', '-qm', 'Concurrent movement') - if kind == 'source': s.package.write('feedback.json', {'new': 'obligation'}) + if kind == 'head': + coverage.git(s.current['workspace'], '-c', 'user.name=Test', '-c', 'user.email=test@example.test', 'commit', '--allow-empty', '-qm', 'Concurrent movement') + if kind == 'source': + target = Path(s.current['sources']['feedback/0']['snapshot']) + target.chmod(0o600) + target.write_text('{}') if kind == 'assessment': s.assessment_path.write_text('{}') if kind == 'run': state.mutate(s.path, {}, lambda record: record['iteration_run'].update(id='changed')) @@ -582,7 +586,7 @@ def test_equal_scope_refresh_requires_source_comparison(scenario, source, equiva s.package.write('history.json', {'obligation': 'Inspect the newly reported failure'}) s.package.manifest['evidence'][source] = ['history.json'] s.refresh(published=False, change=lambda value: value.update(sync_reason='')) - assert coverage.scope(s.original) == coverage.scope(s.current) + assert coverage.scope(s.original) != coverage.scope(s.current) with pytest.raises(ValueError, match='scope lacks review coverage'): state.verify_review(s.path, s.current) if not equivalent: @@ -603,7 +607,7 @@ def test_equal_scope_reordered_reviewers_cannot_retain_coverage(tmp_path, monkey s = Scenario(tmp_path, monkeypatch, reviewers=('second', 'example')) s.package.manifest['evidence']['reviewers'].reverse() s.refresh(published=False, change=lambda value: value.update(sync_reason='')) - assert coverage.scope(s.original) == coverage.scope(s.current) + assert coverage.scope(s.original) != coverage.scope(s.current) assert s.run()['status'] == 'blocked' with pytest.raises(ValueError): state.verify_review(s.path, s.current) @@ -646,6 +650,15 @@ def test_exact_reviewed_preparation_honors_its_own_blocked_comparison(scenario): assert s.run()['status'] == 'blocked' with pytest.raises(ValueError, match='not semantically equivalent'): state.verify_review(s.path, s.original) + refreshed = prep.prepare(s.original['recovery_manifest']['path'], s.package.versions, + previous_preparation=s.original_path) + current = prep.read(refreshed['artifact']) + assert current['preparation_id'] != s.original['preparation_id'] + assert current['engineering_id'] == s.original['engineering_id'] + state.mutate(s.path, {}, lambda record: state.attach_preparation( + record, refreshed['artifact'], s.package.context['url'], s.reviewed_head)) + with pytest.raises(ValueError, match='not semantically equivalent'): + state.verify_review(s.path, current) @pytest.mark.parametrize('refreshed', [True, False]) @@ -1022,10 +1035,10 @@ def change(artifact): artifact['result']['findings'] = [{'path': 'transform.py', 'line': 1, 'message': 'Unassessed defect', 'reviewers': s.reviewers}] retain_legacy_review(s, change) - if kind in {'scope', 'discussion', 'tree'}: + if kind in {'scope', 'discussion', 'tree', 'reviewer', 'finding'}: result = run() assert result['status'] == 'blocked' - assert ('unchanged review scope' if kind != 'tree' else 'original reviewed tree') in result['reason'] + assert 'unchanged review scope' in result['reason'] else: reason = {'source': 'source changed', 'run': 'iteration run/cycle', 'cycle': 'iteration run/cycle', 'reviewer': 'execution is incomplete', 'finding': 'lacks validity and scope assessment', @@ -1053,7 +1066,7 @@ def prepare(package, *args, **kwargs): run, candidate, path, _ = setup_precheck(s, monkeypatch) shutil.rmtree(s.package.historical_checkout) if missing_object: - checkout = Path(s.package.context['checkout']) + checkout = Path(prep.context_for(s.current)['checkout']) (checkout / '.git/objects' / s.reviewed_head[:2] / s.reviewed_head[2:]).unlink() with pytest.raises(subprocess.CalledProcessError): run() @@ -1188,7 +1201,10 @@ def test_stale_precheck_never_authorizes_publication(scenario, monkeypatch, kind write(path, guard) if kind == 'candidate': s.git('commit', '--allow-empty', '-qm', 'Concurrent change') - if kind == 'source': s.package.write('feedback.json', {'changed': True}) + if kind == 'source': + target = Path(s.current['sources']['feedback/0']['snapshot']) + target.chmod(0o600) + target.write_text('{}') if kind == 'run': state.mutate(s.path, {}, lambda r: r['iteration_run'].update(id='other')) if kind == 'attempt': state.mutate(s.path, {}, lambda r: r['cycle_retries'].append({'number': 5})) if kind == 'receipt': Path(receipt['path']).write_text('{}') @@ -1317,15 +1333,17 @@ def test_reply_precheck_guarded_cleanup_and_final_completion(scenario, monkeypat if legacy: retain_legacy_review(s) run, candidate, path, data = setup_precheck(s, monkeypatch, no_op=no_op, discussion=not legacy) - receipt = run()['precheck'] if legacy: - assert state.verify_review(s.path, s.current)['version'] == 1 - saved = read(Path(receipt['path']), {}) - assert saved['reconciliation_version'] == 1 - assert not {'session_file', 'attempt'} & saved['binding'].keys() - with pytest.raises(ValueError, match='cannot supply final review coverage'): - coverage.verify(s.reference, s.current, reconciliation={ - 'artifact': receipt, 'binding': saved['binding']}) + # A synthetic v1 result cannot cover refreshed v3 engineering inputs. + blocked = run() + assert blocked['status'] == 'blocked' and 'unchanged review scope' in blocked['reason'] + assert len(read(s.path, {})['cycles']) == 5 + return + receipt = run()['precheck'] + saved = read(Path(receipt['path']), {}) + with pytest.raises(ValueError, match='cannot supply final review coverage'): + coverage.verify(s.reference, s.current, reconciliation={ + 'artifact': receipt, 'binding': saved['binding']}) assert completion.completion_gate(s.reviewed_head, s.reviewed_head, s.reviewed_head, True, False, 'success', stage='review', preparation=s.current_path, pr_url=s.package.context['url'], body='', run_record=s.path, verification=s.verification(), cleanup_precheck=receipt['path']) == 'clean' @@ -1367,24 +1385,15 @@ def invoke(*args): coverage.git(isolated, 'checkout', '-q', '--detach', candidate) # Final preparation must consume the same verified discussion, now at the # published candidate head. It receives a new comparison and verification. - if legacy: - s.refresh(published=False, change=lambda result: result.update(sync_reason='')) - else: - restatement_refresh(s, published=False) + restatement_refresh(s, published=False) s.assessment_path = final_assessment s.assessment = reconcile.series.load(final_assessment, s.package.repo) - if legacy: - with pytest.raises(ValueError, match='requires version-2'): - s.run() - else: - assert s.run()['status'] == 'reconciled' + assert s.run()['status'] == 'reconciled' assert completion.completion_gate(s.reviewed_head, candidate, candidate, True, False, {'head': candidate, 'result': 'success'}, preparation=s.current_path, pr_url=s.package.context['url'], body='', run_record=s.path, verification=s.verification(), assessment=s.assessment, series_snapshot=reconcile.series.live_snapshot(s.package.context['url'], isolated)) == 'clean' - assert s.calls == ([0] if legacy else [0, 0]) and len(read(s.path, {})['cycles']) == 5 - if legacy: - assert read(s.path, {})['review_sessions'] == [] + assert s.calls == [0, 0] and len(read(s.path, {})['cycles']) == 5 assert Path(s.reference['path']).read_bytes() == s.original_bytes assert not path.exists() and s.git('rev-parse', data['recovery_ref']) == s.reviewed_head assert all(path.read_bytes() == before for path, before in s.retained_bytes.items()) diff --git a/runtime/dk-pr-iter/tests/test_review_result_diagnostics.py b/runtime/dk-pr-iter/tests/test_review_result_diagnostics.py index b4ee347..f22a6c6 100644 --- a/runtime/dk-pr-iter/tests/test_review_result_diagnostics.py +++ b/runtime/dk-pr-iter/tests/test_review_result_diagnostics.py @@ -29,7 +29,7 @@ def test_malformed_review_result_cli_has_diagnostic_without_mutating_record( tmp_path, monkeypatch, capsys, output): head = 'a' * 40 # Preparation gating is independent of parsing external runner output. - monkeypatch.setattr(state, 'work_gate', lambda *args: {'preparation_id': 'ready'}) + monkeypatch.setattr(state, 'work_gate', lambda *args: {'preparation_id': 'ready', 'reviewers': {}}) record = {} state.start_run(record) state.next_cycle(record, head) diff --git a/runtime/dk-pr-read-comments/tests/test_report_pr_comments.py b/runtime/dk-pr-read-comments/tests/test_report_pr_comments.py index 87665f1..de4e81d 100644 --- a/runtime/dk-pr-read-comments/tests/test_report_pr_comments.py +++ b/runtime/dk-pr-read-comments/tests/test_report_pr_comments.py @@ -152,7 +152,7 @@ def setUp(self): self.env = patch.dict(os.environ, {"XDG_DATA_HOME": str(self.data)}) self.env.start() self.addCleanup(self.env.stop) - self.pr = {"number": 3, "title": "Fix", "body": "", "url": "https://github.com/acme/project/pull/3", "headRepository": {"nameWithOwner": "acme/project"}, "headRefOid": "abc"} + self.pr = {"number": 3, "title": "Fix", "body": "", "url": "https://github.com/acme/project/pull/3", "headRepository": {"nameWithOwner": "acme/project"}, "headRefOid": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"} self.meta = {"i1": {**comment("i1"), "kind": "inline", "thread_id": "t"}} self.threads = {"t": {"id": "t", "resolved": False, "path": "x.py", "line": 4, "comment_ids": ["i1"]}} self.analysis_count = 0 @@ -180,7 +180,7 @@ def fake_run(self, args, **kwargs): self.last_payload = payload head_repo = (self.pr.get("headRepository") or {}).get("nameWithOwner") or "acme/project" self.assertEqual(payload["code_permalinks"], { - "prefix": f"https://github.com/{head_repo}/blob/abc/", "paths": {"x.py": [[4, 4]]}}) + "prefix": f"https://github.com/{head_repo}/blob/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/", "paths": {"x.py": [[4, 4]]}}) return json.dumps({"decisions": [{"text": "Keep a cache", "references": [comment("i1")["url"]]}], "thread_summaries": [{"thread_id": t["id"], "summary": "Discusses the cache."} for t in payload["threads_to_summarize"]]}) @@ -205,11 +205,11 @@ def fetch_bodies(self, ids): return {cid: f"Text for {cid}" for cid in ids} def state_path(self): - return self.data / "dk-pr-decisions" / "github.com" / "acme" / "project" / "pr-3.json" + return mod.cache_path(self.pr['url']) def test_compact_context_reuses_current_checkpoint(self): first = json.loads(self.invoke(context_json=True)) - self.assertEqual(first['head_sha'], 'abc') + self.assertEqual(first['head_sha'], 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa') self.assertEqual(first['threads'][0]['summary'], 'Discusses the cache.') self.assertEqual(len(first['decisions']), 1) self.assertEqual(self.analysis_count, 1) @@ -305,13 +305,13 @@ def test_fork_code_links_use_head_repository(self): self.pr["headRepository"] = {"nameWithOwner": "contributor/project"} self.invoke() self.assertEqual(self.last_payload["code_permalinks"]["prefix"], - "https://github.com/contributor/project/blob/abc/") + "https://github.com/contributor/project/blob/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/") def test_legacy_cache_migrates_without_code_decisions(self): self.invoke() state = json.loads(self.state_path().read_text()) state['decisions'].append({'text': 'Code-derived choice', 'references': [ - 'https://github.com/acme/project/blob/abc/x.py#L4']}) + 'https://github.com/acme/project/blob/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/x.py#L4']}) legacy = self.data / 'dk-pr-read-comments' / 'github.com' / 'acme' / 'project' / 'pr-3.json' legacy.parent.mkdir(parents=True) legacy.write_text(json.dumps(state)) diff --git a/runtime/dk-pr-review/references/sessions.md b/runtime/dk-pr-review/references/sessions.md index d5094fc..ba925d4 100644 --- a/runtime/dk-pr-review/references/sessions.md +++ b/runtime/dk-pr-review/references/sessions.md @@ -107,3 +107,23 @@ mapped, source-verified discussion restatements and operational changes with the same engineering meaning; all other protected inputs remain strict. Legacy version-1 attestations retain their original restrictions. Final-head refresh and reconciliation against the published assessment are still required. + + +Fresh preparation v3 uses session v2 and review-result v3. The session compatibility +identity contains the runtime-computed engineering ID, exact head, base/tree, scope, +caller scope/rules text and ordered reviewers. Each result retains its exact +preparation ID plus engineering ID; session provenance records each preparation +used. An operational refresh with equal engineering identity can reuse completed +checkpoints without modifying their bytes or earlier history. Validate current live +evidence, immutable captures and retained provider bindings first. Changed governing +sources, Decisions, code, instructions or reviewer order block reuse. A changed head +blocks interruption resume even with an equal tree; use guarded cleanup reconciliation +for completed coverage. Legacy sessions cannot resume through the new contract and +must continue through their retained runtime. Fresh default sessions and their +checkpoints/results live in the private workflow directory. + +Registry and retained execution use the same version validator: missing versions +are accepted on either side; declared registry and manifest versions must match. +Package hashes and exact procedure identity remain mandatory even without a version. +Invalid retained bindings block cycle allocation before counters advance; consumed +cycles are never refunded or reset. diff --git a/runtime/dk-pr-review/scripts/review_context.py b/runtime/dk-pr-review/scripts/review_context.py index 86cca74..2c4a06f 100755 --- a/runtime/dk-pr-review/scripts/review_context.py +++ b/runtime/dk-pr-review/scripts/review_context.py @@ -14,6 +14,7 @@ from urllib.parse import urlparse from shared.github_reads import run as github_run from shared.pr_identity import resolve +from shared.workflow_storage import allocate FIELDS = 'number,url,title,body,baseRefName,baseRefOid,headRefName,headRefOid,headRepository,isCrossRepository,files' LARGE_DIFF_ERROR = re.compile(r'diff exceeded the maximum number of lines|20,?000\s+lines|PullRequest\.diff too_large', re.I) @@ -156,7 +157,7 @@ def fetch(cwd, pr=None): local_head = local.stdout.strip() if local.returncode == 0 else None status = subprocess.run(['git', 'status', '--porcelain=v1'], cwd=cwd, text=True, capture_output=True) worktree_clean = status.returncode == 0 and not status.stdout.strip() - folder = Path(tempfile.mkdtemp(prefix='codex-pr-review-')) + folder = allocate(repository=root) checkout = folder/'checkout' command(['git', 'clone', '--quiet', '--no-hardlinks', '--no-checkout', str(root), str(checkout)], cwd) head = metadata['headRefOid'] @@ -188,11 +189,15 @@ def fetch(cwd, pr=None): raise RuntimeError('PR head or base changed during checkout; retry from a new snapshot') (folder/'metadata.json').write_text(json.dumps(metadata, indent=2, ensure_ascii=False)+'\n') (folder/'diff.patch').write_text(patch) - return {'url': metadata['url'], 'repository': str(root), 'base_sha': metadata['baseRefOid'], 'head_sha': head, + context = {'url': metadata['url'], 'repository': str(root), 'base_sha': metadata['baseRefOid'], 'head_sha': head, 'local_head': local_head, 'worktree_clean': worktree_clean, 'local_matches_pr': worktree_clean and local_head == metadata['headRefOid'], 'metadata': str(folder/'metadata.json'), 'patch': str(folder/'diff.patch'), - 'checkout': str(checkout)} + 'checkout': str(checkout), 'workflow_dir': str(folder), 'context_file': str(folder / 'context.json')} + (folder / 'context.json').write_text(json.dumps(context, indent=2) + '\n') + for path in (folder / 'metadata.json', folder / 'diff.patch', folder / 'context.json'): + path.chmod(0o600) + return context def main(): diff --git a/runtime/dk-pr-review/scripts/run_reviewers.py b/runtime/dk-pr-review/scripts/run_reviewers.py index b501b3a..2d7161e 100644 --- a/runtime/dk-pr-review/scripts/run_reviewers.py +++ b/runtime/dk-pr-review/scripts/run_reviewers.py @@ -240,6 +240,15 @@ def describe_reviewer(name): def review(context, reviewers, scope, decisions, rules, preparation, stats=None, session=None): + if preparation.get('preparation_version', 1) >= 3: + if any(context[k] != preparation['identity'][field] for k, field in ( + ('url', 'pr_url'), ('head_sha', 'head_sha'), ('base_sha', 'base_sha'))): + raise ValueError('supplied context has changed evidence; refresh preparation') + for field in ('patch', 'metadata'): + supplied = Path(context[field]) + if supplied.exists() and preparation_api.file_hash(supplied) != preparation['sources'][field]['sha256']: + raise ValueError('preparation source identity mismatch: supplied ' + field + ' differs from prepared evidence') + context = preparation_api.context_for(preparation) decisions = validate_decisions(context, decisions) preparation = preparation_api.validate(preparation, context, decisions) with bindings.pinned_bindings(preparation['reviewers']): @@ -251,6 +260,8 @@ def _review(context, reviewers, scope, decisions, rules, preparation, stats=None reviewers = list(provider_map) if any(name not in preparation['reviewers'] for name in reviewers): raise ValueError('preparation omits a configured reviewer') + if preparation.get('preparation_version', 1) >= 3 and reviewers != preparation['reviewer_order']: + raise ValueError('configured reviewer order changed; refresh preparation') if not reviewers: raise ValueError('no PR review agent is configured; use dk-config reviewers set SKILL') patch = Path(context['patch']).read_text() @@ -392,7 +403,17 @@ def main(): parser.add_argument('--resume', action='store_true', help='Explicitly resume an interrupted session with identical evidence') parser.add_argument('--run-record', help='Compatibility adapter for earlier iteration callers') args = parser.parse_args() - context = json.loads(Path(args.context).read_text()) + preparation = preparation_api.read(args.preparation_file) if args.preparation_file else None + fresh = preparation is not None and preparation.get('preparation_version', 1) >= 3 + context = preparation_api.context_for(preparation) if fresh else json.loads(Path(args.context).read_text()) + if fresh and Path(args.context).exists(): + supplied = preparation_api.read(args.context) + if any(supplied[k] != context[k] for k in ('url', 'head_sha', 'base_sha')): + raise ValueError('supplied context has changed evidence; refresh preparation') + for field in ('metadata', 'patch'): + if (Path(supplied[field]).exists() and preparation_api.file_hash(supplied[field]) != + preparation['sources'][field]['sha256']): + raise ValueError('supplied ' + field + ' differs from prepared evidence') if not args.describe and (not args.decisions_file or not args.preparation_file): parser.error('--decisions-file and --preparation-file are required to launch reviewers') config = json.loads(run(['python3', str(ROOT/'dk-config/scripts/repo_rules.py'), @@ -403,7 +424,11 @@ def main(): if args.describe: print(json.dumps({'reviewers': [describe_reviewer(name) for name in config['reviewers']]}, ensure_ascii=True)) return - decisions = validate_decisions(context, json.loads(Path(args.decisions_file).read_text())) + decisions = validate_decisions(context, preparation_api.read(preparation_api.source_path(preparation, 'decisions')) + if fresh else json.loads(Path(args.decisions_file).read_text())) + if fresh and Path(args.decisions_file).exists(): + if validate_decisions(context, preparation_api.read(args.decisions_file)) != decisions: + raise ValueError('supplied Decisions changed; refresh preparation') preparation = preparation_api.load(args.preparation_file, context, decisions) def optional(path): return Path(path).read_text() if path else '' @@ -440,6 +465,9 @@ def optional(path): artifact = session.finish(result.copy()) result['review_session'] = {'id': session.id, 'path': str(session.path)} result['review_artifact'] = artifact + if fresh: + result.update(preparation_id=preparation['preparation_id'], + engineering_id=preparation_api.engineering_id(preparation)) if legacy: legacy.legacy_register_review(args.run_record, result, context['head_sha']) print(json.dumps(result, ensure_ascii=True)) diff --git a/runtime/dk-pr-review/tests/test_prepared_delivery.py b/runtime/dk-pr-review/tests/test_prepared_delivery.py index c1a2513..2a4e4e5 100644 --- a/runtime/dk-pr-review/tests/test_prepared_delivery.py +++ b/runtime/dk-pr-review/tests/test_prepared_delivery.py @@ -99,7 +99,7 @@ def configured(args,cwd=None): self.assertEqual(review_coverage.verify(wire['review_artifact'],artifact),retained) received=json.loads(self.received.read_bytes().decode('utf-8')) self.assertEqual(os.fsencode(received['preparation']['analysis']['placement']['production']['locations'][0]),b'odd-\xff.py') - sidecar=json.loads((self.package.root/'review-stats.json').read_bytes().decode('utf-8')) + sidecar=json.loads((Path(runner.preparation_api.context_for(artifact)['patch']).parent/'review-stats.json').read_bytes().decode('utf-8')) self.assertEqual(sidecar['run']['reviews'][self.name]['status'],'completed') stats_results=[json.loads(Path(path).read_bytes().decode('utf-8')) for path in sidecar['run']['reviews'][self.name]['artifacts'] if Path(path).suffix=='.json'] diff --git a/runtime/dk-pr-review/tests/test_review_entrypoints.py b/runtime/dk-pr-review/tests/test_review_entrypoints.py index d21eb86..9dbb6d7 100644 --- a/runtime/dk-pr-review/tests/test_review_entrypoints.py +++ b/runtime/dk-pr-review/tests/test_review_entrypoints.py @@ -54,6 +54,10 @@ def iteration_record(tmp_path, context, prepared, cycle=2): preparation_path = tmp_path / 'preparation.json' write(preparation_path, prepared) record = {} + from preparation_fixtures import promote + decisions = json.loads(Path(prepared["sources"]["decisions"]["path"]).read_text()) + promote(prepared, context, decisions) + write(preparation_path, prepared) state.start_run(record) state.attach_preparation(record, preparation_path, context['url'], context['head_sha']) for _ in range(cycle): @@ -147,7 +151,10 @@ def test_main_resumes_checkpoint_after_interrupt_before_aggregate_finish( artifact = result_artifact(session_path, result['review_artifact']) assert coverage.verify(result['review_artifact'], prepared) == artifact assert artifact['result'] == {key: value for key, value in result.items() - if key not in {'review_session', 'review_artifact'}} + if key not in {'review_session', 'review_artifact', 'preparation_id', 'engineering_id'}} + if artifact['version'] == 3: + assert result['preparation_id'] == artifact['preparation_id'] + assert result['engineering_id'] == artifact['engineering_id'] if route == 'legacy': record = read(run_path, {}) assert record['iteration_run'] == original_record['iteration_run'] diff --git a/runtime/dk-pr-review/tests/test_review_provenance.py b/runtime/dk-pr-review/tests/test_review_provenance.py index 4916e3c..1f9d74f 100644 --- a/runtime/dk-pr-review/tests/test_review_provenance.py +++ b/runtime/dk-pr-review/tests/test_review_provenance.py @@ -153,6 +153,9 @@ def select(plugin): preparation_path = root / 'preparation.json' write(preparation_path, prepared) record = {} + from preparation_fixtures import promote + promote(prepared, context, decisions) + write(preparation_path, prepared) state.start_run(record) state.attach_preparation(record, preparation_path, context['url'], context['head_sha']) state.next_cycle(record, context['head_sha']) diff --git a/runtime/dk-pr-review/tests/test_review_storage.py b/runtime/dk-pr-review/tests/test_review_storage.py index 4530e64..a0a4789 100644 --- a/runtime/dk-pr-review/tests/test_review_storage.py +++ b/runtime/dk-pr-review/tests/test_review_storage.py @@ -202,6 +202,7 @@ def test_legacy_main_rejects_record_lock_before_allocation_even_with_wrong_cycle storage_layout, main_call, tmp_path, root): context, decisions, prepared, names = storage_layout path = iteration_record(tmp_path, context, prepared, cycle=2) + context = runner.preparation_api.context_for(prepared) before_record = path.read_bytes() before_roots = {key: (Path(context[key]) / 'x.py').read_bytes() for key in ('repository', 'checkout')} @@ -221,6 +222,7 @@ def test_legacy_main_rechecks_record_lock_before_registration( storage_layout, main_call, tmp_path, root): context, decisions, prepared, names = storage_layout path = iteration_record(tmp_path, context, prepared, cycle=2) + context = runner.preparation_api.context_for(prepared) before_roots = {key: (Path(context[key]) / 'x.py').read_bytes() for key in ('repository', 'checkout')} allocated_record = None diff --git a/runtime/shared/bindings.py b/runtime/shared/bindings.py index 4cb7f19..f885ae1 100644 --- a/runtime/shared/bindings.py +++ b/runtime/shared/bindings.py @@ -60,6 +60,13 @@ def codex_plugins(): _PINNED_BINDINGS = ContextVar('dk_pinned_bindings', default={}) +def validate_version(registry_version, manifest): + """A version is optional on either side; compare only declared versions.""" + declared = manifest.get('version') + if registry_version is not None and 'version' in manifest and registry_version != declared: + raise ValueError('Installed version and manifest disagree; refresh the host installation') + + @contextmanager def pinned_bindings(descriptors): from shared.runtime_bundles import verify, verify_binding @@ -82,8 +89,8 @@ def pinned_bindings(descriptors): raise ValueError('invalid retained reviewer binding') procedure, manifest = manifest_skill(Path(value['root']), value['executor'], *value['binding']['skill'].split(':')) - if (str(procedure) != value['procedure'] or (value.get('version') is not None - and manifest.get('version', value['version']) != value['version'])): + validate_version(value.get('version'), manifest) + if str(procedure) != value['procedure']: raise ValueError('retained reviewer procedure or version changed') pinned[name] = value token = _PINNED_BINDINGS.set(pinned) @@ -207,8 +214,7 @@ def resolve(value): f'Install the plugin for {host}, or remove ambiguous installations.') identity, root, record = matches[0] procedure, manifest = manifest_skill(root, host, namespace, name) - if record.get('version') and record['version'] != manifest.get('version', record['version']): - raise ValueError('Installed version and manifest disagree; refresh the host installation') + validate_version(record.get('version'), manifest) return {'binding': binding, 'identity': key(binding), 'executor': host, 'plugin_id': identity, 'root': str(root.resolve()), 'procedure': str(procedure), 'version': record.get('version')} diff --git a/runtime/shared/command_results.py b/runtime/shared/command_results.py index ec6b984..1ccd080 100644 --- a/runtime/shared/command_results.py +++ b/runtime/shared/command_results.py @@ -71,6 +71,9 @@ def classification(command, code, data): elif command == 'replies' and value.get('action') in {'stop', 'reconcile'}: reason = 'reply_' + value['action'] elif command == 'decisions': + error = value.get('error') or {} + if value.get('status') == 'error' and error.get('code') == 'metadata_conflict': + return 'blocked', 'metadata_conflict', error.get('next_action', 'Reconcile the retained proposal explicitly.') payload = value.get('data') if isinstance(payload, dict) and payload.get('readiness') in {'needs_decision', 'sync_pending'}: reason = payload['readiness'] diff --git a/runtime/shared/engineering_identity.py b/runtime/shared/engineering_identity.py new file mode 100644 index 0000000..e47d2ce --- /dev/null +++ b/runtime/shared/engineering_identity.py @@ -0,0 +1,98 @@ +"""Engineering compatibility is computed, never trusted from a caller's ID.""" +import copy +import hashlib +import json +from pathlib import Path + +from shared.preparation_graph import digest + + +def descriptor(value): + value = copy.deepcopy(value) + replacements = {} + execution = value.get('execution') or {} + for key in ('root', 'origin_root'): + if execution.get(key): + replacements[execution[key]] = '' + for index, bundle in enumerate(execution.get('bundles', [])): + replacements[bundle['root']] = '' + for index, source in enumerate(value.get('sources', [])): + replacements[source['path']] = '' + # Known paths also appear in the adapter's declared effective prompt. + def normalize(item): + if isinstance(item, str): + for path, token in sorted(replacements.items(), key=lambda v: -len(v[0])): + item = item.replace(path, token).replace(json.dumps(path)[1:-1], token) + return item + if isinstance(item, list): + return [normalize(v) for v in item] + if isinstance(item, dict): + return {k: normalize(v) for k, v in item.items()} + return item + return normalize(value) + + +def engineering_id(prepared): + if prepared.get('preparation_version', 1) < 3: + return prepared['preparation_id'] + # Only these declared fields are operational, derived, or verified lineage. + excluded = {'preparation_id', 'engineering_id', 'sources', 'reviewer_paths', + 'parent', 'refresh', 'retired_ids', 'workflow_dir', 'workspace', + 'identity', 'inputs', 'reviewers', 'recovery_manifest', 'recovery_context'} + content = {k: v for k, v in prepared.items() if k not in excluded} + graph = prepared.get('graph', {}) + content['graph'] = {node['location']: {**node, + 'depends_on': sorted(graph[key]['location'] for key in node['depends_on'])} + for node in graph.values()} + content['identity'] = {k: v for k, v in prepared['identity'].items() + if k not in {'head_sha', 'decisions_hash'}} + content['inputs'] = {**prepared['inputs'], 'sources': { + key: {**value, 'identity': key if prepared['sources'][key].get('mode') == 'capture' else value['identity']} + for key, value in prepared['inputs']['sources'].items() if not key.startswith('prior_preparation/')}} + # Preserve explicit order independently of JSON object key ordering. + order = prepared.get('reviewer_order', list(prepared['reviewers'])) + content['reviewers'] = [descriptor(prepared['reviewers'][name]) for name in order] + content['source_contracts'] = {key: {k: v for k, v in source.items() + if k not in {'path', 'snapshot', 'sha256', 'origin_identity'}} + for key, source in prepared['sources'].items() + if not key.startswith('prior_preparation/')} + # Only proven ancestors and the declared absence fields are bookkeeping. + # Unrelated prior preparations and unknown absence fields still govern. + lineage, parent = set(), prepared.get('parent') + while isinstance(parent, dict) and {'path', 'sha256', 'preparation_id'} <= parent.keys(): + if parent['preparation_id'] in lineage: + raise ValueError('cyclic preparation lineage') + try: + raw = Path(parent['path']).read_bytes() + ancestor = json.loads(raw) + if (hashlib.sha256(raw).hexdigest() != parent['sha256'] + or ancestor.get('preparation_id') != parent['preparation_id']): + raise ValueError('preparation parent fingerprint mismatch') + except (OSError, ValueError, TypeError, AttributeError, KeyError) as exc: + raise ValueError('invalid selected parent preparation: ' + str(exc)) from exc + lineage.add(parent['preparation_id']) + parent = ancestor.get('parent') + prior_inputs = {} + for key, source in prepared['sources'].items(): + if not key.startswith('prior_preparation/'): + continue + prior = json.loads(Path(source['snapshot']).read_bytes()) + value = {k: v for k, v in source.items() + if k not in {'path', 'snapshot', 'sha256', 'origin_identity', 'mode'}} + if 'analysis' in prior: + if prior.get('preparation_id') not in lineage: + value['preparation'] = engineering_id(prior) + else: + value['extensions'] = {k: v for k, v in prior.items() if k not in {'verified_absence', 'reason'}} + if not value['extensions']: + value.pop('extensions') + if value: + prior_inputs[key] = value + content['prior_governing'] = prior_inputs + return digest(content) + + +def compatible_reference(item, prepared): + return (item.get('engineering_id') == engineering_id(prepared) + if prepared.get('preparation_version', 1) >= 3 + else item.get('preparation_id') == prepared['preparation_id']) diff --git a/runtime/shared/preparation_graph.py b/runtime/shared/preparation_graph.py index c8106bb..e0ebb66 100644 --- a/runtime/shared/preparation_graph.py +++ b/runtime/shared/preparation_graph.py @@ -48,8 +48,10 @@ def relative(path, checkout): def source_identity(key, source, checkout, content): + if source.get('mode') == 'capture' and source.get('origin_identity'): + return source['origin_identity'] category = key.split('/')[0] - if '/' not in key or category == 'prior_preparation': + if '/' not in key or category in {'prior_preparation', 'reviewer-source'}: return key repo_path = relative(source['path'], checkout) if repo_path is not None: @@ -98,11 +100,12 @@ def semantic(key, content): value = decode(content) if key == 'manifest': return {'mode': value['mode'], **{k: v for k, v in value.items() - if k not in {'mode', 'context', 'decisions', 'evidence'}}} + if k not in {'mode', 'context', 'decisions', 'evidence', + 'source_modes', 'source_origins'}}} if key == 'context': return {k: v for k, v in value.items() if k not in {'head_sha', 'checkout', 'repository', 'patch', 'metadata', - 'local_head', 'worktree_clean', 'local_matches_pr'}} + 'local_head', 'worktree_clean', 'local_matches_pr', 'workflow_dir', 'context_file'}} if key == 'decisions': if 'data' in value: return {**value, 'data': {k: v for k, v in value['data'].items() if k != 'head_sha'}} @@ -130,7 +133,8 @@ def semantic(key, content): return {'fields': {k: v for k, v in value.items() if k != 'operational'}, 'operational_unknown': operational_unknown(value.get('operational', {}))} if category == 'reviewers': - return value + from shared.engineering_identity import descriptor + return descriptor(value) return {'opaque_sha256': hashlib.sha256(content).hexdigest()} diff --git a/runtime/shared/review_coverage.py b/runtime/shared/review_coverage.py index 5fa727f..220e68e 100644 --- a/runtime/shared/review_coverage.py +++ b/runtime/shared/review_coverage.py @@ -5,6 +5,7 @@ from pathlib import Path from shared.private_state import read, mutate +from shared.engineering_identity import engineering_id PROTECTED_SOURCE_CATEGORIES = frozenset({'issues', 'choices', 'user_instructions', 'rules'}) @@ -18,6 +19,8 @@ def git(cwd, *args): def scope(prepared, *, decisions=None): + if prepared.get('preparation_version', 1) >= 3: + return engineering_id(prepared) raw = read(Path(prepared['sources']['decisions']['path']), {}) if decisions is None else decisions decisions = raw.get('data', raw) # Head changes alone are allowed for proven tree-equivalent regrouping. @@ -28,6 +31,26 @@ def scope(prepared, *, decisions=None): if key.split('/')[0] in PROTECTED_SOURCE_CATEGORIES}}) +def preparation_api(): + import importlib.util + path = Path(__file__).resolve().parents[1] / 'dk-pr-decisions/scripts/preparation.py' + spec = importlib.util.spec_from_file_location('coverage_preparation', path) + api = importlib.util.module_from_spec(spec) + spec.loader.exec_module(api) + return api + + +def validate_current(prepared): + api = preparation_api() + context = api.context_for(prepared) + raw = api.read(api.source_path(prepared, 'decisions')) + api.validate(prepared, context, raw.get('data', raw)) + if (git(context['checkout'], 'rev-parse', 'HEAD') != context['head_sha'] + or git(context['checkout'], 'status', '--porcelain=v1')): + raise ValueError('prepared isolated checkout changed') + return context + + def outside(path, context): path = Path(path).resolve() for key in ('repository', 'checkout'): @@ -50,14 +73,17 @@ def retain(session_path, context, prepared, result, session_id): session_path = storage_path(session_path, context) directory = outside(session_path.resolve().parent / 'review-results', context) session = read(session_path, {}) - if session.get('id') != session_id or session.get('session_version') != 1: + version = 3 if prepared.get('preparation_version', 1) >= 3 else 2 + if session.get('id') != session_id or session.get('session_version') != version - 1: raise ValueError('review result requires its originating session') - artifact = {'version': 2, 'session_id': session_id, + artifact = {'version': version, 'session_id': session_id, 'session_identity': session.get('identity'), 'pr_url': context['url'], 'head': context['head_sha'], 'tree': git(context['checkout'], 'rev-parse', f"{context['head_sha']}^{{tree}}"), 'scope': scope(prepared), 'preparation_id': prepared['preparation_id'], 'result': result} + if version == 3: + artifact['engineering_id'] = engineering_id(prepared) validate_session_identity(artifact) fingerprint = digest(artifact) path = storage_path(directory / (fingerprint + '.json'), context) @@ -72,6 +98,8 @@ def validate_session_identity(artifact): identity = artifact.get('session_identity') fields = {'pr_url', 'head', 'base', 'preparation_id', 'tree', 'scope', 'scope_text', 'rules_text', 'reviewers'} + reference = 'engineering_id' if artifact.get('version') == 3 else 'preparation_id' + fields = (fields - {'preparation_id'}) | {reference} if (not isinstance(identity, dict) or set(identity) != fields or any(not isinstance(identity[key], str) or not identity[key] for key in fields - {'reviewers'}) @@ -80,15 +108,15 @@ def validate_session_identity(artifact): or any(not isinstance(reviewer, str) or not reviewer for reviewer in identity['reviewers']) or len(set(identity['reviewers'])) != len(identity['reviewers']) or any(identity[key] != artifact[key] - for key in ('pr_url', 'head', 'preparation_id', 'tree', 'scope'))): + for key in ('pr_url', 'head', reference, 'tree', 'scope'))): raise ValueError('review result session identity is missing or inconsistent') def load(item): artifact = read(Path(item['path']), {}) - if digest(artifact) != item['sha256'] or artifact.get('version') not in {1, 2}: + if digest(artifact) != item['sha256'] or artifact.get('version') not in {1, 2, 3}: raise ValueError('review result artifact changed or has an unsupported version') - if artifact['version'] == 2: + if artifact['version'] >= 2: validate_session_identity(artifact) return artifact @@ -111,14 +139,26 @@ def verify(item, prepared, *, accepted_failures=(), reconciliation=None, checkou if reconciliation and reconciliation['binding'].get('purpose') == 'cleanup_precheck': raise ValueError('cleanup prechecks cannot supply final review coverage') artifact = load(item) - context = read(Path(prepared['sources']['context']['path']), {}) + if prepared.get('preparation_version', 1) >= 3: + if checkout is None: + context = validate_current(prepared) + else: + # Cleanup checks original coverage from its immutable captures while + # separately validating current preparation and the candidate tree. + api = preparation_api() + context = api.context_for(prepared) + raw = api.read(api.source_path(prepared, 'decisions')) + api.validate(prepared, context, raw.get('data', raw), check_sources=False) + else: + context = read(Path(prepared['sources']['context']['path']), {}) tree = git(checkout or context['checkout'], 'rev-parse', f"{prepared['identity']['head_sha']}^{{tree}}") current_scope = scope(prepared) - if artifact['scope'] != current_scope and reconciliation is not None: + changed_head = artifact.get('version') == 3 and artifact['head'] != prepared['identity']['head_sha'] + if (artifact['scope'] != current_scope or changed_head) and reconciliation is not None: from shared import review_reconciliation binding = reconciliation['binding'] review_reconciliation.load(reconciliation['artifact'], binding) - if (artifact['version'] != 2 or binding['review_artifact'] != {k: item[k] for k in ('path', 'sha256')} + if (artifact['version'] not in {2, 3} or binding['review_artifact'] != {k: item[k] for k in ('path', 'sha256')} or binding['original_scope'] != artifact['scope'] or binding['original_preparation_id'] != artifact['preparation_id'] or binding['current_preparation_id'] != prepared['preparation_id'] @@ -126,7 +166,7 @@ def verify(item, prepared, *, accepted_failures=(), reconciliation=None, checkou or binding['final_head'] != prepared['identity']['head_sha']): raise ValueError('reconciliation does not cover this review and preparation') if (artifact['pr_url'] != prepared['identity']['pr_url'] - or artifact['tree'] != tree or (artifact['scope'] != current_scope and reconciliation is None)): + or artifact['tree'] != tree or ((artifact['scope'] != current_scope or changed_head) and reconciliation is None)): raise ValueError('final tree or scope lacks review coverage') result = artifact['result'] completed = result['executed_reviewers'] + result['reused_reviewers'] diff --git a/runtime/shared/review_reconciliation.py b/runtime/shared/review_reconciliation.py index 075d0c0..5e6edf2 100644 --- a/runtime/shared/review_reconciliation.py +++ b/runtime/shared/review_reconciliation.py @@ -149,7 +149,9 @@ def without_head(value): else: from shared import cleanup_precheck _, accepted = api.review_acceptances(record, reference, result) - reviewed = coverage.verify(reference, {**original, 'sources': old_sources}, + reviewed_preparation = (original if original.get('preparation_version', 1) >= 3 + else {**original, 'sources': old_sources}) + reviewed = coverage.verify(reference, reviewed_preparation, accepted_failures=accepted, checkout=checkout) api.verify_findings(record, reviewed) assessment_binding = cleanup_precheck.identity(precheck, context, result, current, live=live) @@ -376,6 +378,18 @@ def coverage_for(record, prepared, api, *, assessment=None, assessment_path=None # Only the current preparation can supply retained coverage; unrelated # historical evidence is never a fallback for a damaged current entry. if item.get('current_preparation_id') != prepared['preparation_id']: + if prepared.get('preparation_version', 1) >= 3: + from shared.engineering_identity import engineering_id + prior = next((p for p in record.get('preparations', []) + if p['id'] == item.get('current_preparation_id')), {}) + if prior.get('engineering_id') == engineering_id(prepared): + saved = read(Path(item['path']), {}) + if coverage.digest(saved) != item['sha256'] or not saved.get('binding'): + raise ValueError('reconciliation evidence is missing or malformed') + if saved['binding'].get('review_artifact') == reference: + load(item, saved['binding'], require_equivalent=False) + if not saved['comparison']['equivalent']: + raise ValueError('preparations are not semantically equivalent: ' + saved['comparison']['rationale']) continue saved = read(Path(item['path']), {}) if (not isinstance(saved, dict) or coverage.digest(saved) != item['sha256'] @@ -397,12 +411,14 @@ def coverage_for(record, prepared, api, *, assessment=None, assessment_path=None or api.preparation_api.file_hash(assessment_path) != binding['assessment']['sha256']): raise ValueError('completion must use the same fingerprinted cleanup assessment') if assessment is not None and assessment != series.load(binding['assessment']['path'], - api.preparation_api.read(prepared['sources']['context']['path'])['checkout']): + api.preparation_api.context_for(prepared)['checkout']): raise ValueError('completion must use the reconciled cleanup assessment') return {'artifact': item, 'binding': binding} - # Equal scope does not fingerprint feedback, operational sources or reviewer - # order. Refreshed preparations require reconciliation even at identical scope. - if (result.get('preparation_id') == prepared['preparation_id'] + # Legacy scope omits feedback and reviewer order. Fresh engineering identity + # covers them, so an unchanged-head operational refresh can retain coverage. + from shared.engineering_identity import compatible_reference + if (compatible_reference(result, prepared) + and (result.get('version') != 3 or result['head'] == prepared['identity']['head_sha']) and result['scope'] == coverage.scope(prepared)): return None raise ValueError('final tree or scope lacks review coverage; reconcile after cleanup or review within the existing budget') diff --git a/runtime/shared/review_sessions.py b/runtime/shared/review_sessions.py index 6996d38..6d35c2d 100644 --- a/runtime/shared/review_sessions.py +++ b/runtime/shared/review_sessions.py @@ -7,6 +7,7 @@ from shared.private_state import root, read, mutate from shared import review_coverage as coverage +from shared.engineering_identity import engineering_id def directory(pr_url): @@ -23,8 +24,9 @@ def outside(path, context): def identity(context, prepared, reviewers, scope='', rules=''): + field = 'engineering_id' if prepared.get('preparation_version', 1) >= 3 else 'preparation_id' return {'pr_url': context['url'], 'head': context['head_sha'], - 'base': prepared['identity']['base_sha'], 'preparation_id': prepared['preparation_id'], + 'base': prepared['identity']['base_sha'], field: engineering_id(prepared), 'tree': coverage.git(context['checkout'], 'rev-parse', f"{context['head_sha']}^{{tree}}"), 'scope': coverage.scope(prepared), 'scope_text': coverage.digest(scope), 'rules_text': coverage.digest(rules), 'reviewers': list(reviewers)} @@ -32,22 +34,36 @@ def identity(context, prepared, reviewers, scope='', rules=''): class Session: def __init__(self, context, prepared, reviewers, *, path=None, resume=False, scope='', rules=''): + version = 2 if prepared.get('preparation_version', 1) >= 3 else 1 + if version == 2: + if (any(context[k] != prepared['identity'][field] for k, field in ( + ('url', 'pr_url'), ('head_sha', 'head_sha'), ('base_sha', 'base_sha'))) + or list(reviewers) != prepared['reviewer_order']): + raise ValueError('review session has changed evidence; refresh preparation') + context = coverage.validate_current(prepared) self.context, self.prepared = context, prepared if resume and path is None: raise ValueError('resuming requires --session-file') - self.path = outside(path or directory(context['url']) / (uuid.uuid4().hex + '.json'), context) + base = Path(prepared['workflow_dir']) / 'review-sessions' if version == 2 else directory(context['url']) + self.path = outside(path or base / (uuid.uuid4().hex + '.json'), context) self.execution_lock = self.path.with_name(self.path.name + '.execution.lock') self.validate_storage() self.identity = identity(context, prepared, reviewers, scope, rules) - default = {'session_version': 1, 'id': uuid.uuid4().hex, 'identity': self.identity, + default = {'session_version': version, 'id': uuid.uuid4().hex, 'identity': self.identity, 'status': 'active', 'reviews': {}, 'results': []} def initialize(old): if resume: - if (old.get('session_version') != 1 or old.get('identity') != self.identity + if old.get('session_version') != version: + raise ValueError('legacy session contract; resume through its retained runtime') + if (old.get('identity') != self.identity or old.get('status') not in {'active', 'incomplete'}): raise ValueError('review session is missing, finished or has changed evidence; start fresh') elif old != default: raise ValueError('session file already exists; explicitly resume interrupted work') + if version == 2: + reference = {'preparation_id': prepared['preparation_id'], 'engineering_id': engineering_id(prepared)} + if reference not in old.setdefault('preparations', []): + old['preparations'].append(reference) return old['id'] self.id = mutate(self.path, {} if resume else default, initialize) @@ -146,11 +162,11 @@ def save(data): def result_artifact(session_path, item): session = read(Path(session_path), {}) - if (session.get('session_version') != 1 or not session.get('results') + if (session.get('session_version') not in {1, 2} or not session.get('results') or session['results'][-1] != item): raise ValueError('result is not the latest registered result of this review session') artifact = coverage.load(item) - if (artifact.get('version') != 2 + if (artifact.get('version') != session['session_version'] + 1 or artifact['session_id'] != session.get('id') or artifact['session_identity'] != session.get('identity') or session.get('status') != ('completed' if artifact['result']['complete'] else 'incomplete')): diff --git a/runtime/shared/workflow_storage.py b/runtime/shared/workflow_storage.py new file mode 100644 index 0000000..e28cab9 --- /dev/null +++ b/runtime/shared/workflow_storage.py @@ -0,0 +1,25 @@ +"""Private durable storage for fresh workflows. Nothing here is auto-deleted.""" +import os +from pathlib import Path +import tempfile + + +def allocate(*, repository=None): + base = Path(os.environ.get('XDG_STATE_HOME') or Path.home() / '.local/state') + if not base.is_absolute(): + raise ValueError('XDG_STATE_HOME must be absolute') + base = base / 'dk-code-factory/workflows' + from shared.repository_scope import inside_checkout + if (base.resolve().is_relative_to(Path(__file__).resolve().parents[2]) + or inside_checkout(base, repository or Path.cwd())): + raise ValueError('private workflow storage must stay outside the plugin and repository') + base.mkdir(parents=True, exist_ok=True, mode=0o700) + return Path(tempfile.mkdtemp(prefix='workflow-', dir=base)) + + +def owned(path): + base = Path(os.environ.get('XDG_STATE_HOME') or Path.home() / '.local/state') + path = Path(path).resolve() + if not path.is_relative_to((base / 'dk-code-factory/workflows').resolve()): + raise ValueError('workflow directory must be in private workflow storage') + return path diff --git a/tests/test_bindings.py b/tests/test_bindings.py index cfb12fd..c10e44a 100644 --- a/tests/test_bindings.py +++ b/tests/test_bindings.py @@ -289,3 +289,48 @@ def test_malformed_registry_response_not_cached(monkeypatch, invalid): with pytest.raises(ValueError, match='installed array'): b.codex_plugins() assert b.codex_plugins() == [] + + +@pytest.mark.parametrize('declared,registered,valid', [ + (None, '1.2.3', True), ('1.2.3', None, True), (None, None, True), + ('1.2.3', '1.2.3', True), ('9.9.9', '1.2.3', False), +]) +def test_discovery_and_retained_execution_share_version_contract(tmp_path, monkeypatch, declared, registered, valid): + from shared.runtime_bundles import retained_binding + root = plugin(tmp_path) + manifest = root / '.claude-plugin/plugin.json' + value = json.loads(manifest.read_text()) + if declared is None: + value.pop('version') + else: + value['version'] = declared + manifest.write_text(json.dumps(value)) + registry(tmp_path, root, monkeypatch, {'third-party@test': [ + {'installPath': str(root), **({'version': registered} if registered else {})}]}) + binding = {'skill': 'third-party:review', 'host': 'claude'} + if not valid: + with pytest.raises(ValueError, match='disagree'): + b.resolve(binding) + # Retained validation compares the same pair independently of discovery. + resolved = {'binding': binding, 'identity': b.key(binding), 'executor': 'claude', + 'root': str(root), 'procedure': str(root / 'native/skills/review/SKILL.md'), + 'version': registered} + retained = retained_binding(resolved) + with pytest.raises(ValueError, match='disagree'): + with b.pinned_bindings({b.key(binding): {'execution': retained}}): + pytest.fail('mismatched retained version accepted') + return + retained = retained_binding(b.resolve(binding)) + descriptor = {b.key(binding): {'execution': retained}} + with b.pinned_bindings(descriptor): + assert b.resolve(binding) == retained + monkeypatch.setattr(e, 'execute_process', lambda *a, **kw: '{"findings":[]}') + handoff = tmp_path / 'handoff.json' + handoff.write_text('{}') + assert json.loads(e.invoke(binding, handoff, tmp_path, 'Review')) == {'findings': []} + source = Path(retained['procedure']) + source.chmod(0o600) + source.write_text('Tampered instructions') + with pytest.raises((ValueError, RuntimeError), match='changed'): + with b.pinned_bindings(descriptor): + pytest.fail('tampered retained package accepted') diff --git a/tests/test_runtime_bundles.py b/tests/test_runtime_bundles.py index fd85a37..3bcddb2 100644 --- a/tests/test_runtime_bundles.py +++ b/tests/test_runtime_bundles.py @@ -223,7 +223,7 @@ def run(argv, **kwargs): with pytest.raises(ValueError, match='disagree'): runner.describe_reviewer(selection) incompatible = {'execution': runtime_bundles.retained_binding(resolved)} - with pytest.raises(ValueError, match='procedure or version changed'): + with pytest.raises(ValueError, match='disagree'): with bindings.pinned_bindings({bindings.key(selection): incompatible}): pass manifest_path.write_text(json.dumps(manifest))