Skip to content

fix(verification): keep frozen check identities across resumes and name conflicts (#393) - #404

Open
datacas wants to merge 2 commits into
mainfrom
fix/393-frozen-check-identity
Open

datacas wants to merge 2 commits into
mainfrom
fix/393-frozen-check-identity

Conversation

@datacas

@datacas datacas commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Closes #393

Work item: GitHub issue #393 (#393).

Diagnosis

Symptom. Resuming with --from resolve or --from rereview stopped before any dispatch with the generic cannot rewrite frozen check identity. The stop named no check and no field.

Hypothesis in the item. It listed several candidates and kept the cause open: stale runtime, a restore failure, a legacy polluted plan, a metadata mismatch, or something else.

Reproduction. tests.test_run_cycle.NativeExecutionPlanTests.test_observed_pre_dispatch_collision_names_the_check_and_its_fields uses a sanitized legacy revision with the documented shape:

  • two configured IDs (configured-test and configured-test-<digest>) with one command, runtime executor and profile, project context, and the projection's runtime provenance and reason;
  • agent checks, one of which has a command today's policy no longer authorizes.

The strict restore fails (HistoryError), so the resume keeps the fingerprinted plan. The pre-#387 reconciliation derived configured-test-<digest> again. Extending the plan with that spec raises the observed error. The actual check is configured-test-<16 hex of the command digest>. The fields that differ are:

  • command: persisted fingerprint against the plain command;
  • provenance and reason: normalized to runtime by the projection.

executor, profile, context and level are equal. All the new regressions were run against the pre-fix scripts/ (tests from this branch, scripts from main). They failed with 10 failures and 5 errors, including the three-resume test. They pass with the fix.

Installed runtime against source. The plan modules in ~/.code-cycle/runtime are byte-identical to main at 0032676 today, with no git diff --no-index differences for cycle_plan.py, verification_plan.py, cycle_history.py, cycle_dispatch.py, execution_plan.py and run_cycle.py. They were reinstalled on 2026-11-03 at 16:37 +0100, after the reported failures. toolkit.identity records version=0.6.1 with an empty commit, so the runtime that actually ran the failing resumes can't be recovered from here. With the current reconciliation, the pre-dispatch collision for this exact shape no longer happens, because #387 recognizes the configured digest. The same root cause still broke the current code after dispatch. The restored configured check carried provenance/reason: runtime, and after a partial restore its command was a fingerprint. So a correct report for configured-test, for example with provenance: project as the skill instructs, or with the plain command, was rejected. _normalize_test_payload swallowed that rejection, so the stage's evidence was silently discarded as not_verified. This stage could not read the persisted plan snapshot from the history database: the implement stage's command table denies python3 -c and helper scripts, and the TMPDIR policy allows only fixed file names. The sanitized fixture uses the facts confirmed during triage instead.

Cause and invariant. The broken invariant: a frozen check's identity is what it executes (command, executor, profile, context, level). extend compared every field, including the descriptive ones that persistence normalizes. On top of that, the partial-restore path left every command, authorized or not, as a fingerprint. The cause matches the item's "metadata mismatch + restoration" candidates. The "stale runtime" candidate explains only the pre-dispatch form.

Related work (widened search: bug label, recurrence after #385). #385 / PR #387 (closed, same symptom, configured-digest recognition), #388, #382, #330 (independent, per the item). No other open item is addressed.

Fix shape.

  • VerificationPlan.extend compares only IDENTITY_FIELDS. It keeps the frozen provenance and reason, and treats a stored ref-<sha256> executor, profile or context as equal to the value it came from.
  • A genuine change is rejected with the check ID and every differing field. Commands appear only as fingerprint prefixes, marked unbound while unbound, and other non-token values as references. Demotion names the check and the lost obligation.
  • The partial-restore path binds the commands today's policy authorizes and keeps the rest as inert fingerprints, retaining every check, obligation and result.
  • A rejected stage report keeps not_verified and adds a sanitized report note. Before dispatch, the diagnostic is the BLOCKED stop reason (readiness_unconfirmed).
  • Enforcement is not relaxed. A command is never matched by fingerprint in extend, so an agent can't bind a hidden command.

Work units

Unit Behaviour Files Tests Verification Rollback
WU-1 Frozen check identity reconciliation, partial restore binding, conflict and demotion diagnostics, docs scripts/verification_plan.py, scripts/cycle_history.py, scripts/cycle_plan.py, scripts/cycle_dispatch.py, scripts/run_cycle.py, tests/test_run_cycle.py, docs/verification.md, docs/execution-plan.md 7 new tests in NativeExecutionPlanTests see below independent (no schema or data migration: the stored projection format is unchanged)

Acceptance criteria coverage

AC Check
AC-1 test_observed_pre_dispatch_collision_names_the_check_and_its_fields (installed-runtime/source comparison in Diagnosis)
AC-2 test_three_resumes_keep_identities_obligations_and_history (resolve, rereview, resolve)
AC-3 test_three_resumes_… and test_partial_restore_binds_only_authorized_commands (legacy plan with both configured IDs)
AC-4 test_persisted_metadata_is_not_an_identity_conflict; configuration and checkout paths are not part of any runtime-owned identity
AC-5 test_genuine_identity_changes_are_rejected_and_borrow_no_evidence, test_an_unauthorized_non_manual_command_still_fails_restore
AC-6 test_three_resumes_… (new HEAD per resume, history retained, unbound/manual/unexecuted checks stay unverified, cap not_verified)
AC-7 test_pre_dispatch_identity_stop_names_check_and_fields, test_rejected_report_note_is_safe_for_a_terminal
AC-8 docs updated; suites below

Verification (agent-reported)

Command Exit Result
python3 -m unittest tests.test_run_cycle.NativeExecutionPlanTests 0 29 tests OK
python3 -m unittest discover -s tests (configured-test) 0 2185 tests OK, 1 skipped
python3 scripts/validate-package.py 0 VALID: 16 skills (7 cycle, 9 supporting), shared sections in sync, cross-agent manifests aligned
python3 -m unittest 0 0 tests (no top-level package; the configured suite is the discover run)
basedpyright not run unverified: denied by the implement stage's command table. The editor's BasedPyright diagnostics for the touched files showed only the existing classes (implicit relative imports, Any, import cycles). The new from verification_plan import persisted_token follows the module's existing import style.

Environment: the tests use temporary SQLite stores and Git fixtures with their own identity, and isolate_host_environment keeps out the global Git config, credentials and the user's history database. Local runs and CI share Python stdlib-only dependencies. They differ in the host: WSL2, Python 3.10.12.

Security

cc-security-review ran on the diff. SEC-001 (low): the new report note could carry an agent-chosen check ID with terminal control bytes. Fixed: the note goes through readable(), and a test covers it. No other finding: partial binding grants nothing beyond today's authorization, a fingerprint can't be bound from a report, and the diagnostics don't reveal command text.

Documentation

docs/verification.md (identity fields, partial restore, diagnostics format) and docs/execution-plan.md (reconciliation) are updated. The idempotent-resume behaviour from #387 was already documented. This change extends that section.

…me conflicts (#393)

A check's execution identity is now its command, executor, profile, context
and level. Provenance and reason are descriptive: the history projection
normalizes them to `runtime`, so a report or reconciliation that describes a
frozen check differently keeps the frozen description instead of failing with
"cannot rewrite frozen check identity". An executor, profile or context kept by
the store as a `ref-<sha256>` reference matches the value it came from.

When a resume cannot bind every stored command, `restore_plan(partial=True)`
still binds the commands today's policy authorizes and leaves the rest as inert
fingerprints, keeping every check, obligation and result. The configured check
therefore keeps one identity, and agent reports for it are no longer rejected
because the frozen command was left as a fingerprint.

A rejected identity names the check and each differing field, showing commands
only as fingerprint prefixes (marked `unbound` while unbound) and non-token
values as references. Before dispatch, that text is the BLOCKED stop reason. A
rejected stage report keeps `not_verified` and adds a report note. Demotion
errors name the check and the lost obligation.

Regression tests use a sanitized legacy plan with two configured IDs. They
cover three resumes (resolve, rereview, resolve), partial restore, normalized
metadata, genuine identity changes, an unbound fingerprint and the pre-dispatch
stop.

Closes #393
@datacas

datacas commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Implement stage summary

Status: IMPLEMENTED. Head: 6a5e5f942495b230370aa9d435122e92d693b0c8. Diagnosis decision: implement (cause_mismatch, with the corrected cause inside the item's scope).

  • Cause. A frozen check's identity compared descriptive fields that the history projection normalizes (provenance, reason become runtime). The partial-restore path also left even authorized commands as fingerprints.
  • Fix.
    • Identity is now command, executor, profile, context and level. The frozen description is kept.
    • On restore, commands authorized today are bound and the others stay inert fingerprints.
    • Conflicts and demotions name the check and its fields without revealing commands.
  • Verification (agent-reported).
    • python3 -m unittest discover -s tests: exit 0, 2185 OK, 1 skipped.
    • python3 scripts/validate-package.py: exit 0, VALID.
    • python3 -m unittest: exit 0, 0 tests.
    • Focused NativeExecutionPlanTests: exit 0, 29 OK.
    • basedpyright: unverified, denied by the stage command table.
  • Security. cc-security-review ran. SEC-001 (low, terminal control bytes in the new note) was fixed in this head.
  • Checks of the pushed head (6a5e5f9): none reported yet (rollup empty, read once under the parallel CI policy).

Run: implement · profile deep_coder · requested anthropic/claude-opus-5-5 · resolved ? · effort high · schema:2

@datacas

datacas commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author

Code Cycle initial review — PR #404

[CCR-20261103-001] · senior_reviewer · anthropic/claude-opus-5-5→? · high · schema:2

Verdict: REVIEW_PASSED_AWAITING_CI. The review of e00284b7711eddae08378a0487b1119f3f229e20 is complete and no finding blocks approval. One required check is still pending, so this is not an approval. The cycle's CI gate decides.

Scope

  • Accumulated diff origin/main...e00284b (merge base 6cda9d99), 539 lines. Every hunk was read.
  • Files: scripts/verification_plan.py, scripts/cycle_history.py, scripts/cycle_plan.py, scripts/cycle_dispatch.py, scripts/run_cycle.py, tests/test_run_cycle.py, docs/verification.md, docs/execution-plan.md.
  • Surrounding code read to check the fix: Check, VerificationPlan.validate/extend, normalize_tests, plan_projection, restore_plan, CycleHistoryStore.latest_plan (digest check), PlanRuntime.prepare_plan, authorized_bindings, record_plan_stage, the dispatch normalization path, readable() and scripts/runtime.manifest.

Summary

The fix restores the invariant that a frozen check's identity is what it executes (command, executor, profile, context, level).

  • extend keeps the frozen description (provenance, reason) instead of comparing it.
  • A stored ref-<sha256> executor, profile or context matches the value it was derived from.
  • The partial-restore path now binds the commands today's policy authorizes, through the same authorized_bindings() set the strict restore uses. It keeps the rest as inert fingerprints.
  • Conflict and demotion errors name the check and the differing fields. They never include command text.
  • A command is never matched by its fingerprint inside extend, so a report cannot bind a hidden command. Enforcement is not relaxed.
  • restore_plan now also runs the projection-version and digest check on the partial path. latest_plan already enforces the digest, so this changes no behaviour.

Findings

[REV-001] · low · open · - · blocks:no — A persisted ref- check ID is not matched back to the reported ID

scripts/verification_plan.py:272 (extend), scripts/cycle_history.py:241

plan_projection stores a check id that is not a plain token, for example one with a space or more than 128 characters, as ref-<sha256>, the same as executor, profile and context. identity_conflict now maps ref- references back for executor, profile and context, but extend looks checks up with checks.get(check.id) only. After a restore, an agent that reports such a check under its original ID gets a new, second check. The frozen ref- check stays required with no evidence on the new HEAD, so cap() holds the conclusion at not_verified on every later resume. This is a duplicate obligation of the kind AC-2 excludes, though not for the configured check.

The behaviour predates this PR and the toolkit's own IDs are plain tokens, so the finding does not block approval. Suggested follow-up: resolve a reported ID through persisted_token() when the exact ID is absent, or restrict reported check IDs to the token grammar. Evidence: code inspection only. The review command table denies running an ad-hoc reproduction (see Unverified).

Security triage and audit

  • Deterministic rule: .code-cycle.yml declares no security_review.always_when. No changed path or file matches the mandatory minimum (CI and automation, secrets and keys, agent/toolkit configuration), and the PR has no labels.
  • Reviewer judgement: the change touches security tooling, namely command-authorization binding on restore, which evidence a report may record, and diagnostics shown in a terminal. The runtime plan also requires the audit (security_required: true). cc-security-review ran on this head.
  • Result: no confirmed vulnerability and no potential risk above informational.
    • Partial binding grants nothing beyond today's policy: it uses the same authorized_bindings() set as the strict restore.
    • Fingerprints are compared exactly, so a report cannot bind one.
    • ref- matching would need a SHA-256 preimage to abuse.
    • The tests.rejected text is not persisted: it reaches only report.note, through readable(…, 300), which removes control bytes. Pre-dispatch stop text is built from safe_value() tokens and 12-hex fingerprint prefixes. The full digests were already in the store.

Executed verification (this review, local clone at e00284b)

Command Exit Result
python3 -m unittest tests.test_run_cycle.NativeExecutionPlanTests 0 29 tests OK, including the 7 new #393 regressions
python3 -m unittest discover -s tests 0 Ran 2185 tests in 558.400s, OK (skipped=1)
python3 scripts/validate-package.py 0 VALID: 16 skills (7 cycle, 9 supporting), shared sections in sync, cross-agent manifests aligned
python3 -m unittest 0 Ran 0 tests (no top-level package; the configured suite is the discover run)
basedpyright (5 changed scripts/ modules) 1 99 errors, 2724 warnings. None of the errors is on a changed line range except cycle_history.py:51:6, which is reportImplicitRelativeImport for the new from verification_plan import persisted_token. That is the same class as the existing import on line 52 and the rest of the module. The other diagnostics are pre-existing and reported separately, as AC-8 asks, not as a finding.

Acceptance criteria

AC State Evidence
AC-1 met test_observed_pre_dispatch_collision_names_the_check_and_its_fields passes. The installed-runtime/source comparison is recorded in the PR description.
AC-2 met test_three_resumes_keep_identities_obligations_and_history (resolve, rereview, resolve) passes. REV-001 is a non-configured edge case outside the tested shape.
AC-3 met The legacy fixture with both configured IDs passes, and test_partial_restore_binds_only_authorized_commands passes.
AC-4 met test_persisted_metadata_is_not_an_identity_conflict passes.
AC-5 met test_genuine_identity_changes_are_rejected_and_borrow_no_evidence passes.
AC-6 met The three-resume test checks per-HEAD unverified and the not_verified cap. CI does not feed cap().
AC-7 met test_pre_dispatch_identity_stop_names_check_and_fields and test_rejected_report_note_is_safe_for_a_terminal pass.
AC-8 met docs/verification.md and docs/execution-plan.md are updated. The suites pass, and the type-check diagnostics are reported above.

Unverified

  • REV-001 is supported by code inspection only. Running an ad-hoc helper (python3 $TMPDIR/report.md) was denied by this stage's command table ('python3' is not in the review stage's command table). This review did not bypass it.
  • basedpyright was run on the changed modules, not on the whole repository.

CI (read once under the parallel policy; not waited on)

Check State
Skills, manifests, and Bash installer pass
Windows test shard (windows-1) pass
Windows test shard (windows-2) pass
Windows test shard (windows-4) pass
Windows test shard (windows-3) pending

Residual risk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resume blocks before dispatch on frozen check identity: recover persisted plans and name conflicting fields

1 participant