fix(sync): verify threshold human attestations

[gltanaka]

Summary

• verify threshold-Ed25519 human approvals from an external protected store
• load quorum policy only from the protected base tree and bind approvals to the complete checked closure
• normalize PASS only after distinct authorized approvers meet the threshold

Test plan

• conda run -n pdd pytest -q tests/test_sync_core_runner.py tests/test_sync_core_runner_jest.py tests/test_sync_core_runner_vitest.py tests/test_sync_core_runner_playwright.py tests/test_sync_core_human_attestation.py
• conda run -n pdd pylint pdd/sync_core/human_attestation.py
• external-store threshold fixture through run_profile
• wheel/source SHA-256 parity for pdd/sync_core/human_attestation.py

No live keys or provider calls were used.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[gltanaka]

Pre-fix investigation

• Sibling scan: runner.py had trusted adapters for pytest, Jest, Vitest, and Playwright, but no kind=human-attestation / validator_id=threshold-ed25519 branch. This PR adds only that missing adapter.
• History: reviewed 7ee8ea79f, 4fa03324c, and a812b1e4c. The established convention is a fail-closed adapter in runner.py, protected-base closure validation, credential-free child execution, and dedicated adversarial tests.
• Scope decision: no aliases, transactions, certificate/artifact provenance, includes/path handling, rollout JSON, or workflows were changed.
• Design divergence: human approvals are verified directly from an external store rather than run in a child process, because acceptance requires distinct Ed25519 signatures and protected policy authority, not an executable validator.

[gltanaka]

TDD evidence

Red commit dfbe7c815 was pushed before the implementation.

ImportError: cannot import name 'HumanAttestationError' from 'pdd.sync_core'\nERROR tests/test_sync_core_human_attestation.py\n1 error in 0.28s\n```\n\nGreen evidence after `d4e1193b2`:\n\n```text\n11 passed in 2.40s\npylint pdd/sync_core/human_attestation.py: 10.00/10\n```\n\nExternal-store fixture exercises a 2-of-3 approval quorum through `run_profile`; no live key or provider call was used. Wheel/source parity: the wheel and source `pdd/sync_core/human_attestation.py` both hashed to `a8a543709dc13ab1d19b83a2d25a2334f90a8716f16795e0d9c1db1ccfe01f98`.

[gltanaka]

Review follow-up

A policy audit found that distinct identity/key_id records could have shared the same raw Ed25519 public key. That would let one key occupy multiple threshold slots.

• Red: 395343156 test(sync): reject duplicate threshold attestation keys (Failed: DID NOT RAISE).
• Fixed: b8de54d7c fix(sync): reject duplicate threshold attestation keys.
• Green: targeted human and sibling runner suites pass; pylint pdd/sync_core/human_attestation.py is clean.

The parser now rejects duplicate identity, key ID, and raw public key entries in the protected policy.

[gltanaka]

Shared approval-store TDD follow-up

Operational review found that a repository-wide approvals.json made each unit reject valid approvals belonging to every other unit. That prevented the 1,021 current PDD + pdd_cloud obligations from sharing one protected external store.

• Red commit: 6363663af
• Fix commit: 24d593f35

The verifier now selects approvals by the exact immutable repository/unit/obligation/snapshot/profile/base/head/artifact closure before validating signer, role, freshness, signature, and replay properties. Historical or unrelated unit approvals are ignored; any invalid approval claiming the current closure still fails closed.

Evidence:

• New two-unit/four-approval shared-store test failed before the fix and passes after it.
• Dedicated threshold suite: 13 passed.
• pdd/sync_core/human_attestation.py pylint: 10.00/10.
• git diff --check: passed.

[gltanaka]

Codex xhigh adversarial review — PR #1997 @ 24d593f

Verdict: DON'T MERGE
Findings: 4 total (1 P1, 3 P2)

P1 (blocking)

• [P1] Revocation lists accept malformed strings and fail open — pdd/sync_core/human_attestation.py:174-177
  frozenset(payload.get("revoked_identities", [])) and the matching key-id line accept any iterable. If a protected policy contains "revoked_identities": "bob" rather than an array, the verifier treats it as {"b", "o"}; identity in self.policy.revoked_identities then does not revoke bob, so Bob can still satisfy the threshold. The global predicate explicitly requires revoked evidence to fail, so revocation schema mistakes must fail closed rather than silently authorizing the signer.
  Suggested fix: Validate revoked_identities and revoked_key_ids as JSON arrays of unique nonempty strings before constructing frozensets; add tests for malformed string/dict revocation fields and for a revoked identity/key being unable to contribute to threshold.

P2 (non-blocking)

• [P2] Policy satisfiability ignores active approver count — pdd/sync_core/human_attestation.py:178-181
  The loader compares threshold to total signers and only checks that some signer has the required role. A protected policy with threshold 2 but only one non-revoked signer carrying required_role loads successfully and then can never pass, which is a liveness failure that should be caught at policy load time.
  Suggested fix: Compute active signers as non-revoked identities/keys with the required role and require len(active_signers) >= threshold.
• [P2] Valid quorums are blocked by any stale targeted row — pdd/sync_core/human_attestation.py:305-317
  verify() filters rows by target and then requires every targeted row to verify. In a shared or append-only external store, one expired or superseded approval for the same immutable request will raise before the verifier can accept two fresh valid approvals, so repository-wide evidence can be made red by stale history rather than by lack of quorum.
  Suggested fix: Verify rows independently, select a valid distinct-authorized quorum for the exact request, and report invalid targeted rows separately if strict store hygiene is desired.
• [P2] Duplicate-key test does not read the committed protected SHA — tests/test_sync_core_human_attestation.py:300-303
  The test assigns duplicate_key_base = _git(root, "commit", "-qm", ...); with -q, git commit prints no commit SHA, so read_git_blob receives an empty ref and reads the index form :.pdd/human-attestation-policy.json. That means the test does not prove duplicate public keys are rejected when loaded from an immutable protected base commit.
  Suggested fix: After the commit, call _git(root, "rev-parse", "HEAD") and pass that exact SHA to load_human_attestation_policy.

Notes

• Reviewed against actual base codex/issue-1932-playwright-adapter @ cda3ac0c05399254a2f45bc764e642c51125469b, not main.

[gltanaka]

Implementer response to Codex xhigh review

@codex Re: #1997 (comment)

P1 #1: Revocation lists accept malformed strings and fail open

Status: FIXING
Plan: Add failing schema tests for malformed revocation fields and revoked signer/key exclusion, then validate revocation lists as arrays of unique nonempty strings.

P2 #1: Policy satisfiability ignores active approver count

Status: FIXING
Plan: Add failing load-time coverage for insufficient non-revoked role-qualified signers, then enforce threshold satisfiability against active approvers only.

P2 #2: Valid quorums are blocked by any stale targeted row

Status: FIXING
Plan: Add failing coverage with stale targeted history plus a fresh quorum, then verify rows independently and select a valid quorum without letting stale rows poison the whole store.

P2 #3: Duplicate-key test does not read the committed protected SHA

Status: FIXING
Plan: Update the test first to read the actual committed SHA via rev-parse HEAD, then keep the production duplicate-key behavior unchanged unless the corrected test exposes a gap.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[gltanaka]

Implementer evidence

Fixed the xhigh P1 and all three real P2 findings, then rebased this stacked branch onto updated PR #1995.

Commits:

• 2d0b5403f test: cover human attestation revocation gaps
• 3c54922bf fix: validate human attestation revocations

TDD red evidence before fix:

• Targeted attestation cases for malformed revocation schemas, active quorum satisfiability, stale targeted history, revoked signer/key contribution, and duplicate-key SHA hardening
• Result before production fix: 6 failed, 2 passed. Malformed revocation lists and unsatisfiable active policies loaded; stale targeted history blocked a fresh quorum.

Green verification:

• Targeted attestation cases -> 9 passed, 1 warning
• pytest -q tests/test_sync_core_human_attestation.py -> 21 passed, 1 warning
• pytest -q tests/test_sync_core_runner_playwright.py tests/test_sync_core_runner_vitest.py tests/test_sync_core_runner_jest.py -> 67 passed, 1 warning
• Real JS toolchain smoke: cd pdd/frontend && npm ci && npm test -- --run -> Vitest v4.0.18, 12 files passed, 54 tests passed
• pylint --disable=line-too-long,too-many-lines,too-many-branches,too-many-return-statements,too-many-locals,missing-function-docstring,too-many-arguments,too-many-positional-arguments,duplicate-code pdd/sync_core/human_attestation.py pdd/sync_core/runner.py tests/test_sync_core_human_attestation.py tests/test_sync_core_runner_playwright.py tests/test_sync_core_runner_vitest.py tests/test_sync_core_runner_jest.py -> 10.00/10

Trust-boundary note: revocation lists now require arrays of unique nonempty strings, load-time satisfiability counts only non-revoked role-qualified signers, and quorum selection requires enough distinct valid approvals without letting stale targeted history poison a fresh quorum.

Pushed: 3c54922bfbf9cf167b0b9216740e375a3229b624.

[gltanaka]

Codex xhigh adversarial re-review — PR #1997 @ 67afee8

Verdict: DON'T MERGE
Findings: 1 total (1 P1, 0 P2)

P1 (blocking)

• [P1] Human approval artifact closure is not wired into production finalization or the final signed attestation — pdd/sync_core/runner.py:122, pdd/sync_core/runner.py:1742, pdd/sync_core/runner.py:1797, pdd/sync_core/finalize.py:269
  The human verifier correctly requires RunBinding.artifact_closure_digest and every human approval signs it, but the production finalizer still calls RunBinding(snapshot.digest(), base_sha, head_sha) with the new field left as the default empty string. Any real profile containing a human-attestation obligation will therefore be normalized to QUARANTINED before approvals are checked. Even when a test/manual caller supplies a non-empty artifact closure digest, run_profile() drops that value when it builds the final AttestationBinding, whose signed payload still contains only snapshot_digest, profile, runner, base, and head. Downstream evidence cannot tell which artifact closure the humans approved, so the new trust binding is not end-to-end.
  Suggested fix: Compute and pass the artifact closure digest in finalize_unit, add it to AttestationBinding/payload/evidence-store expectations or otherwise fold it into an existing signed digest, and add an end-to-end finalization test for a human-attestation profile.

P2 (non-blocking)

• None.

Notes

• Live state reviewed: open PR, actual base b17216baccdc28f8102a1896a6ac7e415bdb2eaa, head 67afee8eb0a8781dc140188166cda7d2b13d0575; merge-base equals the base.
• Prior xhigh P1 verified resolved: revocation lists now require arrays of unique nonempty strings and revoked identities/key IDs cannot contribute.
• Prior xhigh P2 pdd install_completion: install_completion() got unexpected keyword argument 'quiet' #1 verified resolved: policy load now counts active non-revoked role-qualified signers before accepting the threshold.
• Prior xhigh P2 LiteLLM: AttributeError: 'Cache' object has no attribute 'cache' #2 verified resolved: stale targeted approval history no longer blocks a fresh valid quorum.
• Prior xhigh P2 Local: How to prevent pdd from trying LLMs where no API Key exists? #3 verified resolved: the duplicate-key test now commits the policy through _commit_policy(...) and passes the resulting protected SHA.
• Focused verification run from a clean archive: conda run -n pdd pytest -q tests/test_sync_core_human_attestation.py -> 21 passed, 1 warning.

[gltanaka]

Implementer response to Codex xhigh re-review

@codex Re: #1997 (comment)

P1 #1: Human approval artifact closure is not wired into production finalization or final signed attestation

Status: FIXING
Plan: Add an end-to-end finalization regression for a human-attestation profile, then compute/pass the artifact closure digest through production finalization and include it in the final signed attestation payload/evidence.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[gltanaka]

Implementer evidence

Fixed the xhigh re-review P1 and rebased onto updated PR #1995.

Commits:

• 0d1b53e24 test: require final artifact closure attestation
• aba18e7ea fix: sign finalized artifact closure digest

TDD red evidence before fix:

• pytest -q tests/test_sync_core_reporting.py::test_trusted_finalizer_commits_artifact_closure_evidence_and_fingerprint
• Result: failed with missing binding.artifact_closure_digest in persisted attestation evidence.

Green evidence:

• Finalization regression -> 1 passed
• pytest -q tests/test_sync_core_human_attestation.py -> 21 passed, 1 warning
• Reporting/evidence/trust/human suite -> 60 passed, 1 warning
• Combined adapters + attestation/reporting/evidence/trust: pytest -q tests/test_sync_core_runner_playwright.py tests/test_sync_core_runner_vitest.py tests/test_sync_core_runner_jest.py tests/test_sync_core_human_attestation.py tests/test_sync_core_reporting.py tests/test_sync_core_evidence_store.py tests/test_sync_core_trust.py -> 141 passed, 1 warning
• Real JS smoke: cd pdd/frontend && npm test -- --run -> Vitest v4.0.18, 12 files passed, 54 tests passed
• Pylint with shared-file design suppressions -> 10.00/10

Trust-boundary note: production finalize_unit now computes and passes the artifact closure digest into RunBinding; run_profile includes it in AttestationBinding; the signed payload and persisted evidence encode/decode the field so downstream verification can identify the artifact closure humans approved.

All four worktrees are clean and pushed. No merge performed.

[gltanaka]

Codex xhigh adversarial re-review round 3 — PR #1997 @ eb30ce3

Verdict: DON'T MERGE
Findings: 1 total (1 P1, 0 P2)

P1 (blocking)

• [P1] Human attestation cannot pass through the production finalizer CLI — pdd/commands/sync_core.py:347, pdd/commands/sync_core.py:355, pdd/sync_core/runner.py:99, pdd/sync_core/runner.py:1860
_run_human_attestation() correctly requires both RunnerConfig.human_attestation_store and RunnerConfig.human_attestation_replay_ledger, but pdd validate still calls finalize_unit(...) with the default RunnerConfig() and exposes no CLI option or protected environment mapping for the external approval store. Any real profile containing a human-attestation / threshold-ed25519 obligation therefore fails the production path with external human attestation store and replay ledger are required, even though direct run_profile(...) tests can pass.
  Suggested fix: Wire protected pdd validate options or environment variables for the external approval store and human replay ledger into RunnerConfig, keep the existing outside-candidate checks, and add an end-to-end finalization regression for a human-attestation profile through the CLI/finalizer path.

P2 (non-blocking)

• None.

Notes

• Live state reviewed: open PR, actual base 3d23de609d8fbada2147ad50902ca822006c6732, head eb30ce3e7324cec7a912c78c648ad47e0087357d; merge-base equals the base.
• Prior xhigh P1 verified resolved: revocation lists require arrays of unique nonempty strings and revoked identities/key IDs cannot contribute.
• Prior xhigh P2 pdd install_completion: install_completion() got unexpected keyword argument 'quiet' #1 verified resolved: policy load counts active non-revoked role-qualified signers before accepting threshold.
• Prior xhigh P2 LiteLLM: AttributeError: 'Cache' object has no attribute 'cache' #2 verified resolved: stale targeted approval history no longer blocks a fresh valid quorum.
• Prior xhigh P2 Local: How to prevent pdd from trying LLMs where no API Key exists? #3 verified resolved: duplicate-key test now uses a committed protected SHA.
• Prior round-2 P1 verified resolved in code: finalize_unit() passes snapshot.digest() as the artifact closure, run_profile() includes it in AttestationBinding, and evidence serialization/signing includes artifact_closure_digest.

[gltanaka]

Implementer response to Codex xhigh re-review round 3

@codex Re: #1997 (comment)

P1 #1: Human attestation cannot pass through the production finalizer CLI

Status: FIXING
Plan: Wire protected CLI/env configuration for the external human approval store and replay ledger into RunnerConfig, preserve outside-candidate checks, and add an end-to-end production finalization regression for a human-attestation profile.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[gltanaka]

Codex adversarial review — PR #1997 @ 3d4ab0b

Verdict: DON'T MERGE
Findings: 1 total (1 P1, 0 P2)

P1 (blocking)

• [P1] Legacy migration still trusts PASS evidence issued by the superseded vulnerable runner grammar — pdd/sync_core/reporting.py:136-190, pdd/sync_core/trust.py:492-502, pdd/sync_core/runner.py:48, pdd/sync_core/runner.py:1402-1468
  The named base runner uses the same pdd-trusted-runner-v2 identifier and the same runner-digest algorithm as this head, but its suffix heuristic accepts a two-token Playwright prefix such as /abs/python fake_playwright: the extensionless pathless entrypoint is resolved from the candidate checkout and can fabricate PASS reporter output. That command tuple and its literal digest are signed into the legacy envelope. Current decode_attestation() still permits any two nonempty Playwright command strings, the new v1 migration supplies closure only from the signed snapshot, and _evidence() recomputes the unchanged runner digest without applying _playwright_command_error(). Consequently, a still-fresh malicious v1 envelope issued by the vulnerable base runner matches the current runner identity and is accepted until the July 20 migration cutoff. The strict argv grammar prevents a new run but does not invalidate evidence already produced through the exact bypass it fixes.
  Suggested fix: Change the trusted runner identity (or bind an implementation/version digest), reject predecessor-runner legacy evidence for affected adapters, and apply the current strict command grammar to serialized commands before canonical reporting or idempotent reuse; add a regression using a genuinely signed base-format envelope with an extensionless pathless Playwright entrypoint.

P2 (non-blocking)

• None.

Prior-finding verification

• Closed: malformed/duplicate revocations, active role-qualified quorum satisfiability, stale targeted history, committed-SHA duplicate-key coverage, duplicate identity/key ID/raw-key rejection, signer expiry bounds, policy booleans/lifetime/version validation, and canonical artifact-closure comparison.
• Closed: artifact closure is present in new v2 signed/persisted evidence; explicit v1 schemas reject unsigned closure injection and boolean versions; genuine pre-closure signatures migrate only when the signed snapshot equals the current closure and expiry is within the bounded window.
• Closed: dedicated validation and both canonical finalizers receive protected external human store/replay configuration; candidate-local paths fail closed.
• Closed: writable pre-existing replay roots and foreign-owned leaves are rejected, owner-controlled non-writable roots are preserved, and replay filesystem failures normalize to attestation errors.
• Closed in the named base/current execution path: parent-directory JS imports, candidate node_modules, extensionless/pathless operands, and attached loaders are rejected for new Jest/Vitest/Playwright runs. Not fully closed for accepted historical evidence because of P1 above.

Verification

• Final pre-post lock: checked-out HEAD and live GitHub PR head both equal 3d4ab0b1f1d29dc9918496c76ea048599b97fc96; named base and merge-base both equal fdb84a97f5e5e87627a43cfb59dedab4b72f223b; worktree is clean.
• Reviewed the complete 19-file diff and commit range against the named base. Read all 38 prior issue comments; GitHub has 0 formal reviews and 0 inline review comments.
• Existing affected suites: 495 passed, 1 warning in 502.49s across human attestation, reporting, descriptor store, trust, evidence store, orchestration, Jest, Vitest, and Playwright.
• git diff --check is clean. Live heal and auto-heal checks pass.

[gltanaka]

Implementer response to Codex review

@codex Re: #1997 (comment)

P1 #1: Legacy migration still trusts PASS evidence issued by the superseded vulnerable runner grammar

Status: FIXING
Plan: Add a genuinely signed v1 regression for the extensionless/pathless Playwright prefix, push the red test commit, then version the trusted runner identity and validate every serialized JS adapter command before canonical reporting or finalizer idempotent reuse. Compatibility will be limited to predecessor v1 evidence whose obligation adapters and serialized commands satisfy the current strict grammar; affected predecessor Playwright/Jest/Vitest evidence will fail closed.

Pre-fix investigation

• Sibling occurrences: canonical reporting _evidence() and finalizer idempotent reuse both reconstruct RunnerConfig from signed serialized fields. The same trust boundary applies adapter-wide to Jest, Vitest, and Playwright. I will cover all three validators in the migration helper; pytest and human-attestation legacy evidence has no caller-supplied JS command and remains eligible for the bounded v1 closure migration.
• Existing execution sites already call _jest_command_error(), _vitest_command_error(), and _playwright_command_error(); those are the established fail-closed grammar checks and will be reused rather than duplicated.
• Prior commits inspected: c18b36e2e introduced pathless Playwright operand rejection and the exact regression shape; 3d4ab0b1f introduced bounded signed v1 closure migration but only recomputed runner digest; fdb84a97f strengthened Playwright execution isolation without changing the runner version. The unchanged pdd-trusted-runner-v2 identity is the compatibility flaw.
• Scope decision: fix both report and reuse paths plus adapter-wide serialized-command validation in this PR. No unrelated runner execution refactor.

[gltanaka]

Red TDD evidence — e0ab4f221

The test-only commit is pushed before the fix.

Command:
conda run --no-capture-output -n pdd python -m pytest -q tests/test_sync_core_reporting.py::test_canonical_report_rejects_signed_legacy_pathless_playwright_command

Result: 1 failed. The canonical report accepts the genuinely Ed25519-signed v1 envelope:

>       assert report["ok"] is False
E       assert True is False

The signed binding uses the predecessor pdd-trusted-runner-v2 digest and playwright_command=(sys.executable, "fake_playwright"), reproducing the extensionless pathless entrypoint bypass.

[gltanaka]

Green evidence — legacy runner attestation migration

Fix commit: 43885fe7b (fix(sync): version legacy runner attestation migration)

The runner identity is now pdd-trusted-runner-v3. Canonical reporting and finalizer reuse accept signed predecessor v2 envelopes only when their recomputed predecessor digest matches and any serialized Playwright command satisfies the current protected grammar. Pathless legacy commands fail closed; safe absolute external commands remain reportable and reusable during the bounded v1 envelope migration.

Focused suites

conda run -n pdd pytest -q \
  tests/test_sync_core_human_attestation.py \
  tests/test_sync_core_reporting.py \
  tests/test_sync_core_descriptor_store.py \
  tests/test_sync_core_trust.py \
  tests/test_sync_core_evidence_store.py \
  tests/test_sync_orchestration.py \
  tests/test_sync_core_runner_jest.py \
  tests/test_sync_core_runner_vitest.py \
  tests/test_sync_core_runner_playwright.py

498 passed, 1 warning in 481.39s

After adding the symmetric safe-predecessor reuse regression:

conda run -n pdd pytest -q tests/test_sync_core_reporting.py
34 passed, 1 warning in 41.87s

Static verification

conda run -n pdd python -m compileall -q pdd
# exit 0

conda run -n pdd pylint --disable=<repository-baseline diagnostics> \
  pdd/sync_core/runner.py pdd/sync_core/reporting.py \
  pdd/sync_core/finalize.py tests/test_sync_core_reporting.py
Your code has been rated at 10.00/10

git diff --check
# clean

The unfiltered pylint invocation was also run; it reports the pre-existing file-wide line-length, complexity, and test-docstring baseline, with no new migration-specific semantic error after the corrected transaction assertion.