Skip to content

feat(sync): add verifiable global-sync foundation#1985

Open
gltanaka wants to merge 99 commits into
mainfrom
codex/issue-1932-verifiable-goal
Open

feat(sync): add verifiable global-sync foundation#1985
gltanaka wants to merge 99 commits into
mainfrom
codex/issue-1932-verifiable-goal

Conversation

@gltanaka

Copy link
Copy Markdown
Contributor

Summary

Adds the canonical pdd.sync_core foundation and routes the global-sync compatibility surface through its identity, manifest, classifier, reporting, transaction, and certificate APIs. The certificate binds the candidate wheel SHA and rejects unsafe protected mutations.

pdd reconcile and existing global-sync commands retain their current compatibility behavior while using the canonical proof boundaries.

Rollout status

The full global certificate remains red pending rollout: protected signer and release/workflow deployment, transactional staging, profile/evidence migration, and seven real nightly attestations are still unavailable. This PR intentionally does not claim global-certificate completion.

Verification

  • conda run -n pdd pytest -q tests/test_sync_core_*.py --timeout=120
  • Four coverage-target compatibility tests in tests/test_commands_maintenance.py
  • conda run -n pdd pytest -q tests/e2e/test_issue_1932_continuous_sync_guarantee.py --timeout=120
  • git diff --check
  • conda run -n pdd pylint pdd/sync_core reports only R0801 duplicate-code; score 10.00/10.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Investigation summary

  • Rebased the branch from merge base 9a0d413 onto origin/main at 49504fc.
  • Main already contains the deterministic continuous-sync reconcile lineage: 2cec8f122 fix(sync): add deterministic continuous sync reconcile (#1954), including the later discovery, deletion, hook-worktree, and CI metadata behavior.
  • Sibling sync history reviewed: 89f1b5d51, 484b1d6f6, e96345276, 3fcd1f3cf, and 2cec8f122. The established convention is a deterministic, read-only reconcile/report surface with legacy command adapters, rather than independent sync classification implementations.
  • The rebased branch preserves that current-main behavior and layers the canonical pdd.sync_core proof/certificate work over it. Historical duplicate reconcile patches were either resolved in favor of the current-main superset or dropped by rebase as already upstream.
  • No additional same-root-cause sibling sites were changed outside this canonical global-sync surface; unrelated sync callers remain compatibility adapters.
  • Earlier commits and their prior red runs predated this runbook invocation. No preserved red-output transcript exists, so none is claimed here.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Verification evidence

Fresh local results on rebased head 8cd3e257:

  • Green: conda run -n pdd pytest -q tests/test_sync_core_*.py --timeout=120 -> 291 passed.
  • Green: the four coverage-target compatibility tests in tests/test_commands_maintenance.py -> 4 passed.
  • Green: conda run -n pdd pytest -q tests/e2e/test_issue_1932_continuous_sync_guarantee.py --timeout=120 completed successfully.
  • Green: git diff --check.
  • conda run -n pdd pylint pdd/sync_core returns nonzero only for R0801 duplicate-code (shared JSON parsing shape in reporting.py, verification.py, and waivers.py); pylint otherwise reports 10.00/10.

The full global certificate is intentionally still red pending protected signer/release workflow deployment, transactional staging, profile/evidence migration, and real nightly history.

Comment thread pdd/sync_core/includes.py Fixed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8cd3e257ed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread pdd/sync_core/released_checker.py Outdated
cli = importlib.import_module("pdd.cli").cli

cli.main(
args=["sync", "certify", *sys.argv[1:]],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Dispatch to the registered certify command

When the protected pdd-sync-checker entry point is used, this dispatches to pdd sync with certify as the optional basename. In this commit certify is registered as a top-level command, while sync is a plain command rather than a group, so global checker invocations with options such as --repos or --merge-group are rejected before certification runs. Invoke the registered command path or register a real sync certify subgroup; otherwise the released checker cannot produce global certificates.

Useful? React with 👍 / 👎.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer CI triage

Acknowledged findings:

  1. Normal <include>docs/prompting_guide.md</include> preprocessing omits content in source and installed-wheel paths. I will trace canonical include routing and sibling call sites before adding regression coverage.
  2. pdd/ci_drift_heal.py:1897 may log secret-bearing exception data. I will find the established redaction pattern and matching sibling logs.
  3. pdd/sync_core/includes.py:138 uses a polynomial regex on uncontrolled include text. I will replace it with a bounded/linear parser while preserving supported syntax and add adversarial coverage.

I will follow the strict TDD runbook: mine sibling/history evidence, add and push a red focused-test commit, then implement and verify with targeted tests, lint, regression-public, and a built-wheel smoke. xhigh review remains pending due to service quota.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer investigation evidence

Pre-fix sibling/history hunt completed before tests.

  • Include routing: get_file_path() is the sole expansion resolver used by XML, backtick, include-many, and semantic-query include call sites. Its intended sibling API is PathResolver.resolve_include() (cwd -> package_root -> repo_root). Commit da457b0bf introduced that convention specifically for bundled docs/installed-package resolution. Commit f39dd6090 replaced it with PathPolicy(project_root), which makes an isolated wheel smoke directory authoritative and misses packaged pdd/docs/prompting_guide.md. I will restore the established resolver route without changing each sibling call site.
  • Secret logging: the reported JSON dry-run emission at ci_drift_heal.py:1897 is the relevant full-report sink. Other ci_drift_heal exception/stderr logs are distinct command-failure diagnostics and outside this finding. The established repo-wide redaction convention is context_snapshot.redact_snapshot_text() (introduced in 8626b38b1); I will apply it before serializing the dry-run report.
  • Include parser: parse_include_references() is the canonical consumer used by sync closure/order and preprocess user-intent extraction. The XML and include-many regexes are the shared uncontrolled-text surface. There are no parallel canonical-parser siblings to patch; I will replace that parser stage with a bounded linear scan, preserving body, path=, self-closing, include-many, and backtick syntax.

Scope decision: fix the sole routing function, sole full-report JSON sink, and canonical parser only; no unrelated ci_drift_heal stderr logs or independent preprocess expansion mechanics. Next: add focused red tests and the public regression assertion, commit/push before implementation.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Red TDD evidence

Pushed test-only commit 03890f9e1 (test(sync): reproduce foundation CI regressions).

Command run:

pytest -q tests/test_preprocess.py::test_preprocess_resolves_bundled_prompting_guide_outside_checkout tests/test_ci_drift_heal.py::TestMainDryRunJson::test_dry_run_json_redacts_secret_bearing_failure_data tests/test_sync_core_includes.py::test_malformed_include_text_is_bounded

Actual result: FF. / 2 failed, 1 passed.

  • test_preprocess_resolves_bundled_prompting_guide_outside_checkout produced [File not found: docs/prompting_guide.md].
  • test_dry_run_json_redacts_secret_bearing_failure_data emitted the test GitHub token in the JSON report.
  • The malformed-input parser test passed under its one-second bound on the pre-fix code. It is adversarial coverage for the CodeQL regex finding, not a fabricated CodeQL result; CodeQL output was not run or claimed.

Proceeding to the production fixes.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Green CI regression evidence

Pushed:

  • 03890f9e1 test(sync): reproduce foundation CI regressions
  • 8a8cf5137 fix(sync): resolve foundation CI regressions

Passed verification:

pytest -q tests/test_preprocess.py::test_preprocess_resolves_bundled_prompting_guide_outside_checkout tests/test_ci_drift_heal.py::TestMainDryRunJson tests/test_sync_core_includes.py
# 23 passed

pylint pdd/sync_core/includes.py
# 10.00/10

git diff --check
# clean

make regression-public
# passed, including bundled docs CLI preprocess assertion

python -m build --wheel --outdir dist-smoke
python -m venv --system-site-packages "$SMOKE_DIR/venv"
"$SMOKE_DIR/venv/bin/python" -m pip install --no-deps dist-smoke/*.whl
cd "$SMOKE_DIR"
printf "<include>docs/prompting_guide.md</include>\n" > test_include_python.prompt
"$SMOKE_DIR/venv/bin/pdd" --quiet preprocess test_include_python.prompt --output preprocessed.txt
grep -q "PDD Mental Model" preprocessed.txt
# built-wheel preprocess smoke passed

I also ran direct pylint on pdd/preprocess.py and pdd/ci_drift_heal.py; those legacy modules retain existing broad/module-size diagnostics, with no new lint diagnostic introduced by this change.

Final branch is clean and pushed (codex/issue-1932-verifiable-goal); no merge performed. xhigh review remains pending due to service quota.

Comment thread pdd/ci_drift_heal.py Fixed
Comment thread pdd/sync_core/includes.py Fixed
@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to remaining CodeQL alerts

P1 #1: clear-text logging of sensitive data — pdd/ci_drift_heal.py:1899

Status: FIXING
Plan: make the affected log sink accept only stable, locally selected error categories and context, then audit sibling exception logging on the same canonical path.

P1 #2: polynomial regex on uncontrolled data — pdd/sync_core/includes.py:105

Status: FIXING
Plan: audit the parser's remaining regular expressions and replace the vulnerable parsing path with bounded or linear logic while preserving include syntax.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Red TDD evidence — remaining CodeQL alerts

Pushed 7e5dd71c6 (test(sync): reproduce remaining CodeQL alerts).

pytest -q tests/test_sync_core_includes.py::test_attribute_parser_is_bounded_for_repeated_word_characters tests/test_ci_drift_heal.py::TestMainDryRunJson --timeout=1
3 failed, 1 passed
FAILED test_attribute_parser_is_bounded_for_repeated_word_characters
  Failed: Timeout (>1.0s) in _ATTRIBUTE_PATTERN.finditer(raw)
FAILED TestMainDryRunJson.test_dry_run_json_outputs_report
  expected stable consumer context, but current JSON omits it
FAILED TestMainDryRunJson.test_dry_run_json_excludes_exception_bearing_failure_data
  candidate exception text: ssh-private-key is present in output

Investigation: the only remaining regexes in pdd/sync_core/includes.py are the attribute matcher and boolean-token searches, both reached from untrusted include text. The related CodeQL hardening conventions are 58e53914f and de8e0e184; this round replaces the parser regexes with forward scans. In the canonical dry-run path, serializing the whole report allows exception-derived unit/failure strings to reach stdout, so the fix will emit a fixed summary schema only.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer evidence — remaining CodeQL alerts

Pushed f78bad5e8 (fix(sync): eliminate remaining CodeQL alerts).

  • pdd/ci_drift_heal.py: dry-run JSON now has a fixed ok/consumer/count-summary schema. It never serializes report units, failures, paths, or exception-derived reasons.
  • pdd/sync_core/includes.py: replaced the attribute matcher and both boolean-token regex searches with forward-only scanners; no regexes remain in the shared include parser.
pytest -q tests/test_sync_core_includes.py tests/test_ci_drift_heal.py::TestMainDryRunJson --timeout=5
23 passed, 1 warning in 0.21s

pylint pdd/sync_core/includes.py
10.00/10

pylint --disable=all --enable=C0123 pdd/ci_drift_heal.py
10.00/10

make regression-public
PASS

isolated built-wheel preprocess smoke
PASS (installed wheel imported from site-packages; bundled docs include resolved)

git diff --check
PASS

A full pylint invocation over the legacy ci_drift_heal.py and its test module still reports pre-existing diagnostics; the touched parser is clean and the changed helper has no newly introduced C0123 diagnostic. Branch is clean and pushed. Monitoring the newly queued CodeQL run; I will not claim it green until GitHub reports it.

@gltanaka

Copy link
Copy Markdown
Contributor Author

CodeQL follow-up

The new CodeQL run completed successfully:

CodeQL: pass
Analyze (python): pass (1m28s)
Analyze (actions): pass
Analyze (javascript-typescript): pass

No remaining CodeQL annotations were reported for the two prior alerts. Branch remains clean and fully pushed at f78bad5e8.

@gltanaka

gltanaka commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Foundation CI fix round 3 evidence

Red commit: cba9b622c

The full unit lane exposed 37 failures:

Fix commit: 63d31004f

The fix restores the boundary:

  • legacy preprocessing accepts explicit local absolute paths;
  • canonical sync graph/path policy still rejects absolute managed paths;
  • nested source checkouts cannot shadow missing active-project includes;
  • CI JSON emits only basename, language, and classification per unit, excluding paths, reasons, and failure payloads;
  • canonical include parsing accepts outer whitespace but still rejects multiline/markup paths.

Verification:

@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial review — PR #1985 @ 63d3100

Verdict: DON'T MERGE
Findings: 5 total (3 P1, 2 P2)

P1 (blocking)

  • [P1] Remote signer capabilities leak into untrusted candidate/test subprocesses — pdd/sync_core/runner.py:351-358
    The environments described as credential-free only filter names containing API_KEY, CREDENTIAL, PASSWORD, SECRET, SIGNING_KEY, or TOKEN. That still passes capability-bearing variables such as PDD_ATTESTATION_SIGNER_COMMAND, PDD_CERTIFICATE_SIGNER_COMMAND, PDD_ATTESTATION_PUBLIC_KEY, and PDD_CERTIFICATE_ISSUER into pytest validator children; the lifecycle launcher and candidate-wheel harness use the same pattern at pdd/sync_core/lifecycle.py:19-31 and pdd/sync_core/scenario_harness.py:143-151. A candidate-authored test or candidate wheel can therefore discover and invoke the protected remote signer command instead of merely proving behavior in a credential-free child, which breaks the trust boundary required by the global-sync predicate.
    Suggested fix: Replace the denylist with a small allowlist for untrusted child environments, or at minimum strip every PDD_*ATTESTATION*, PDD_*CERTIFICATE*, PDD_*SIGNER*, PDD_RELEASED_CHECKER*, and CI identity/signing capability variable; add regression tests that set signer-command env vars and prove pytest/candidate children cannot read or invoke them.
  • [P1] Pytest support closure misses ImportFrom alias helpers, allowing self-certification through changed support code — pdd/sync_core/runner.py:91-123
    _local_module_paths() records node.module for from X import Y, but it never resolves the imported aliases. A protected test or conftest.py that says from tests import helper only binds tests.py/tests/__init__.py; tests/helper.py is omitted from the support digest and from the protected_paths diff/quarantine set at pdd/sync_core/runner.py:663-675. A candidate can then change tests/helper.py to alter fixtures/assertions while the runner reports trusted PASS, so tests can certify the code without the helper being bound to the attestation.
    Suggested fix: Resolve ImportFrom aliases to repository-local module/file candidates as well as node.module, add coverage for from tests import helper and relative from . import helper, and fail closed for unresolved local support imports that can affect collection or execution.
  • [P1] Lifecycle scenario contract can certify green without the documented lifecycle injections — pdd/sync_core/scenario_contract.py:3-18
    The required scenario set does not encode the full success predicate from the issue attachment: actual public prompt-only/code-only/test-only/include-only repair runs, simultaneous edit repair/block behavior, and post-repair zero-write reruns are not required. The current source-edit-matrix scenario only calls the pure classifier on synthetic snapshots (pdd/sync_core/scenario_harness.py:167-185), and the candidate public scenarios cover a red unbaselined report plus transaction recovery, not the end-to-end mutation/repair matrix. Because certificate._scan_predicate() only checks aggregate lifecycle counters and missing_scenarios, a future protected checker using this contract can sign ok: true while the lifecycle matrix required by the global-sync predicate was never exercised.
    Suggested fix: Expand the protected REQUIRED_SCENARIOS contract to one scenario per required lifecycle injection and make those scenarios drive the released candidate through the public report/recovery/repair commands, including immediate second-run no-write checks; until those scenarios exist, force the lifecycle predicate to remain red.

P2 (non-blocking)

  • [P2] Candidate wheel lifecycle install is not an exact dependency environment — pdd/sync_core/lifecycle.py:71-84
    The released checker installs the candidate wheel with normal dependency resolution from the ambient package index, then the certificate binds only the candidate wheel SHA. The same wheel can therefore be tested against different dependency bytes over time, or a candidate can alter dependency metadata and have unbound third-party code participate in lifecycle results. That weakens the “exact candidate wheel in a clean environment” evidence even if no secrets are present.
    Suggested fix: Install from a protected, hash-pinned wheelhouse or use --no-index with a locked dependency set, and bind the resulting installed distribution set (names, versions, hashes) into the lifecycle/certificate payload.
  • [P2] pdd sync certify is reserved before the existing sync command can receive certify as a module name — pdd/core/cli.py:353-357
    The root parser rewrites any leading sync certify into the new root certify command. That makes an existing or future module named certify unsyncable via the documented pdd sync BASENAME form, and options intended for sync will be parsed by the certificate command instead.
    Suggested fix: Expose the certificate command as a real subgroup or separate root command without pre-parse rewriting, or only rewrite when a disambiguating certificate-only option such as --repos/--merge-group is present.

Notes

Reviewed against actual base 49504fc9aa11a9ee70539c93a6f1afe64badad94 and head 63d31004f3e9633d11683cf13e2573f9fb8a2d39 from live gh pr view metadata. I did not treat the still-red global certificate rollout itself as a finding; the blocking issues above are in the foundation and predicate code shipped by this PR.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to Codex xhigh review

@codex Re: #1985 (comment)

P1 #1: Remote signer capabilities leak into untrusted candidate/test subprocesses

Status: FIXING in this PR
Plan: replace the current denylist-style environment inheritance for pytest, lifecycle, and candidate-wheel scenario children with a small untrusted-child environment that preserves only execution basics and strips signer/certificate/attestation/released-checker capabilities; add regression coverage proving PDD_*SIGNER*/certificate capability variables are absent from those children.

P1 #2: Pytest support closure misses ImportFrom alias helpers

Status: FIXING in this PR
Plan: resolve from package import helper and relative from . import helper aliases to repository-local .py and package candidates, include them in the support digest/protected path set, and add red coverage for both absolute and relative helper imports.

P1 #3: Lifecycle scenario contract can certify green without documented lifecycle injections

Status: FIXING in this PR
Plan: fail closed by adding explicit required lifecycle injection scenario identities for prompt/code/test/include/simultaneous/zero-write behavior and keeping the protected predicate red until released checker scenarios implement those public CLI repair/no-write runs.

P2 #1: Candidate wheel lifecycle install is not an exact dependency environment

Status: FIXING in this PR
Rationale: this is a real evidence gap because pip install can resolve unbound dependency bytes from ambient indexes. I will first bind dependency-environment evidence into lifecycle results/certificates, and if the local harness allows it, also prevent ambient dependency resolution for candidate install.

P2 #2: pdd sync certify is reserved before existing sync command can receive certify as a module name

Status: FIXING in this PR
Rationale: the pre-parser rewrite is an observable CLI ambiguity for a valid pdd sync BASENAME spelling. I will remove that rewrite and expose certification through the existing root pdd certify command path, with a regression proving pdd sync certify reaches the sync command semantics instead of the certificate parser.

@chatgpt-codex-connector

Copy link
Copy Markdown

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

@gltanaka

Copy link
Copy Markdown
Contributor Author

Red TDD evidence for high-review remediation

Review: #1985 (comment)
Pushed test-only commit: f465988ed (test(sync): reproduce high review boundary gaps)

Local focused red run (excluding Linux-only sandbox case and the deliberately blocking signer probe):

7 failed, 1 warning in 2.17s

Failures reproduce:

  • unresolved dynamic product loading accepted by runner identity;
  • runtime/checker changes absent from runner identity;
  • recovery false-COMMITTED race;
  • linked-worktree deleted and renamed policy bypasses;
  • real path-qualified module selection returning zero units;
  • sync preflight not called before logging/locking.

The real detached-session signer test also fails its 1.5-second bound: the exact-head implementation waits for the detached pipe holder (about 30 seconds in this probe).

The randomized worker-output forgery test is test-only updated to enumerate collection-*.json and execution-*.xml. It is skipped/fail-closed on macOS by design and will run in the focused Ubuntu protected-runner lane.

Proceeding to production implementation only after this pushed red commit.

@gltanaka

Copy link
Copy Markdown
Contributor Author

/heal

@gltanaka

Copy link
Copy Markdown
Contributor Author

Final evidence — high-review remediation complete

Review addressed: #1985 (comment)

Final pushed head: 543bf6d5992258e85293ac8a4634ad5a104f658a
Local and remote branch SHAs match; worktree is clean; git diff --check passes.

TDD commits

  • Red: f465988edtest(sync): reproduce high review boundary gaps
  • Fix: 872e7acedfix(sync): close high review verification boundaries
  • Red CLI follow-up: 84d54285ctest(sync): reproduce CLI preflight state writes
  • Fix CLI follow-up: 543bf6d59fix(sync): preflight CLI before state bookkeeping

Red evidence: #1985 (comment)

Outcomes

  1. Candidate collection/execution outputs no longer share the enumerable worker directory; the Ubuntu adversarial randomized-output forgery test cannot turn a protected failure into PASS.
  2. Product closure fails closed on unresolved dynamic loaders.
  3. File-form linked-worktree Git markers preserve committed-policy enforcement after delete/rename.
  4. Recovery performs full desired-state CAS and conditional rollback conflict handling.
  5. Normal and CLI sync preflight before logs, locks, dispatch, or state bookkeeping.
  6. Runner identity binds stable Python implementation/version/ABI/platform/machine and checker bytes without installation paths.
  7. Canonical selection matches prompt-root-relative, language-stripped qualified module identities.
  8. Signer timeout tracks and kills marked descendants across detached sessions and bounds post-timeout communication.

Verification

  • Focused local remediation tests: 10 passed.
  • CLI compatibility/preflight tests: 4 passed.
  • Real linked-worktree CLI zero-write probe: nonzero exit; before/after Git status digests identical with committed active policy deleted from the worktree.
  • Changed sync-core pylint: 10.00/10.
  • Local broad changed-area: 616 passed, 56 skipped; five unrelated macOS coverage subprocess cases failed locally, while exact-head Ubuntu broad CI is green.
  • Ubuntu focused protected-runner lane: 127 passed.
  • Ubuntu broad unit suite: 13,321 passed, 35 skipped, 1 xfailed.
  • CodeQL, package preprocess smoke, public CLI regression, story regression, repo-bloat Docker E2E, and auto-heal: passed.

CI: https://github.com/promptdriven/pdd/actions/runs/29184616907
Auto-heal: https://github.com/promptdriven/pdd/actions/runs/29184803588

@gltanaka

Copy link
Copy Markdown
Contributor Author

/heal

@gltanaka

Copy link
Copy Markdown
Contributor Author

Exhaustive read-only code-quality review

Reviewed SHA: 543bf6d5992258e85293ac8a4634ad5a104f658a
Base SHA: 49504fc9aa11a9ee70539c93a6f1afe64badad94
Verdict: CHANGES_REQUESTED
Findings: 4 total (2 P1, 2 P2)

P1

  1. Candidate code can still enumerate and forge both protected pytest result channelspdd/sync_core/runner.py:711-714, pdd/sync_core/runner.py:762-765, pdd/sync_core/runner.py:805-818, pdd/sync_core/runner.py:848-883, pdd/sync_core/supervisor.py:31-40, pdd/sync_core/supervisor.py:216-224

    Moving the randomized worker files into randomized subdirectories does not create an access boundary. _runtime_roots() always includes the sandbox cwd, so bubblewrap read-mounts the entire trusted temporary directory. The exact worker and authoritative result files are then bind-mounted writable. Candidate product code receives a worker script path under that directory in sys.argv[0], can recursively enumerate its parent, and can write both the randomized worker file and the fixed junit.xml / node-ids.json channel before calling os._exit(0). The checker still copies/accepts candidate-authored bytes and a zero worker status.

    The remediation test at tests/test_sync_core_runner.py:870-888 is not adversarial against the implemented layout: it runs directory.glob('collection-*.json') / glob('execution-*.xml') only in the worker's parent, while the files now live one random directory deeper. A recursive glob or direct write to the fixed authoritative filename exercises the bypass.

    Required fix: derive authoritative collection/execution outcomes in a protected process/namespace or channel that candidate code cannot enumerate, inspect, write, signal, or terminate. Add a Linux adversarial product that recursively discovers the actual nested channels (and fixed authoritative outputs), forges both phases, exits zero, and prove a protected failing assertion cannot become PASS.

  2. The dynamic-product closure remains bypassable through ordinary Python loader aliasespdd/sync_core/runner.py:195-218, pdd/sync_core/runner.py:324-359, pdd/sync_core/runner.py:451-467

    fail_on_dynamic=True only helps when the AST denylist recognizes a loader. For example, this valid product code is not marked dynamic:

    loader = __builtins__["__import__"]
    value = loader("helper").value

    The assignment is a subscript rather than the recognized Name('__import__'), and the later call is to an untracked alias. An exact-head temporary-repository probe changed only helper.py across two commits and obtained identical runner_identity_digest() values, while pytest would execute the changed helper bytes. The added test covers only the directly recognized importlib.import_module() spelling.

    Required fix: replace the loader-pattern denylist with a complete measured runtime closure or a defensible fail-closed policy that cannot be bypassed by aliasing/reflection. Add stale-evidence tests for indirect __import__ aliases and equivalent reflective loaders.

P2

  1. Runner identity still omits material checker and dependency bytespdd/sync_core/runner.py:525-592, pdd/sync_core/runner.py:514-522

    _checker_artifact_digest() hashes only runner.py and pytest_probe.py. Outcome derivation also materially depends on supervisor.py, isolation.py, Git/path helpers, and the installed pytest/plugin dependency bytes. Changing any of those while leaving the two hashed files and Python tuple unchanged preserves the signed runner digest even though sandboxing, collection, or execution semantics can change. The new test monkeypatches _checker_artifact_digest() itself; it does not prove that changing an actual transitive checker module or pytest installation invalidates evidence.

    Required fix: bind an independently released checker wheel/module closure and locked runtime dependency environment (or their verified digests), excluding only installation prefixes. Test that changing supervisor.py and pytest bytes rejects old evidence while identical released bytes across prefixes remain portable.

  2. Signer timeout returns within a bound but can leak an untracked detached descendantpdd/sync_core/signer_process.py:14-61, pdd/sync_core/signer_process.py:64-96

    Descendant discovery relies only on the inherited PDD_SIGNER_PROCESS_TOKEN. A helper can remove that variable before spawning a detached session. On timeout, the process group kill misses the detached child and _terminate_marked() cannot find it; the bounded second communicate() closes local pipes and returns while the child remains alive. An exact-head probe returned in 0.652 seconds but confirmed the unmarked detached child was still running and wrote a marker afterward. The regression at tests/test_sync_core_trust.py:125-134 checks elapsed time only and leaves the token intact, so it does not verify complete reaping.

    Required fix: use OS containment or stable process-tree tracking that does not trust a removable child environment marker, and assert no detached child survives for both remote signer adapters.

Prior review verification

Finding from the specified 82f88d0a review Exact-head status
P1 candidate result-channel forgery Not fixed: nested random files and fixed authoritative files remain enumerable/writable.
P1 dynamic product closure Not fixed: recognized loader spelling is rejected, but trivial loader aliasing preserves stale identity.
P1 linked-worktree deleted/renamed policy Fixed: valid file-form Git markers reach committed-policy enforcement, with linked-worktree coverage.
P1 recovery false-COMMITTED race Fixed: recovery now performs the desired-state sweep and conditional rollback handling.
P1 sync writes before preflight Fixed: orchestration and CLI paths preflight before log/lock/state bookkeeping.
P2 measured runtime/checker identity Partially fixed: Python tuple and two files are bound; the transitive checker/dependency closure is not.
P2 path-qualified module selection Fixed: real manifest selection uses prompt-root-relative, language-stripped identity.
P2 detached signer liveness Partially fixed: caller latency is bounded, but unmarked detached descendants survive.

Verification and scope

  • Read repository instructions, base-to-head diff and changed source/tests/workflow surfaces, complete PR discussion/formal reviews/inline threads, the specified prior review, and remediation commits/evidence.
  • Read-only local changed-area run: 407 passed, 56 skipped, 4 failed; the four failures are the known macOS fail-closed finalizer cases. git diff --check passed and the worktree remained clean.
  • Exact-head probes reproduced unchanged dynamic-helper identity and post-timeout signer descendant survival. The result-channel bypass follows directly from the Linux bind layout; the relevant Linux test is falsely non-recursive.
  • Live checks are green except auto-heal, which is still pending. Immediately before posting, the PR remained open/non-draft and live head/base exactly matched the SHAs above.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to high review

@codex Re: #1985 (comment)

P1 #1: Candidate code can enumerate and forge protected pytest result channels

Status: FIXING
Plan: Move authoritative derivation outside candidate-visible writable paths and add a Linux adversarial product that recursively discovers nested/fixed outputs, forges them, exits zero, and still cannot turn a protected failure into PASS.

P1 #2: Dynamic-product closure is bypassable through loader aliases

Status: FIXING
Plan: Replace spelling-based dynamic-loader recognition with a complete measured/fail-closed product runtime policy, including __builtins__ alias and reflective-loader stale-evidence tests.

P2 #1: Runner identity omits checker and dependency bytes

Status: FIXING
Plan: Bind the released checker module closure and locked pytest/runtime dependency bytes into runner identity, with tests that mutate actual supervisor and pytest bytes and preserve prefix portability for identical bytes.

P2 #2: Signer timeout leaks untracked detached descendants

Status: FIXING
Plan: Add OS/stable process-tree containment independent of inherited environment markers and verify, for both signer adapters, that an env-cleared detached descendant is gone rather than merely waiting a bounded time.

I will push the adversarial tests as a separate red commit and post their failing output before implementing the fixes.

@chatgpt-codex-connector

Copy link
Copy Markdown

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

@gltanaka

Copy link
Copy Markdown
Contributor Author

Red TDD evidence @ d59656392

Failing adversarial tests were committed and pushed before implementation.

Local command:

conda run -n pdd pytest -q tests/test_sync_core_runner.py tests/test_sync_core_trust.py -k "recursively_forge or aliased_reflective or actual_supervisor or actual_pytest or env_cleared_detached"

Result on macOS: 4 failed, 3 skipped, 90 deselected. The failures prove both __builtins__ alias/reflection cases preserve stale identity and actual supervisor.py / pytest byte changes do not alter runner identity. The three Linux containment cases were collected and skipped locally and are delegated to the focused Linux lane: recursive/fixed result-channel forgery plus env-cleared detached signer survival through both remote adapters.

No implementation changes are present in this commit.

@gltanaka

gltanaka commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Final CI evidence: preserved XPASS diagnostics

Head: 4a750400ff04d6449e36be752ef34fe45766322a
Fix commit: 4a750400f fix(sync): preserve protected pytest diagnostics

Diagnosis and focused fix

The protected worker terminates through os._exit(80 + status) to prevent candidate finalizers from affecting the checker. On the failing Linux run, pytest terminal XPASS diagnostic remained buffered when that immediate exit bypassed normal interpreter teardown. The checker therefore saw a valid JUnit report with one test and no failures, but no XPASS text, and normalized the result incorrectly to PASS.

The fix flushes the restored worker stdout and stderr immediately after pytest.main(...) and before os._exit(...) in both protected collection and execution workers. This preserves the existing isolated/randomized result channels and process-exit boundary; it only makes pytest already-derived diagnostic observable to the checker.

The existing Linux regression test_non_strict_xpass_cannot_pass was the red reproducer: it failed on 37f69af2 with PASS instead of XFAIL.

Exact-head verification

  • Focused Ubuntu protected-runner lane: 132 passed, 1 warning, 73.44s. This includes test_non_strict_xpass_cannot_pass. Run Unit Tests
  • Broad Ubuntu suite at the same SHA: 13,328 passed, 35 skipped, 1 xfailed, 795 warnings, 1452.27s. Unit Tests workflow
  • Required auto-heal: passed. auto-heal workflow

Final repository state: git diff --check passed, worktree is clean, and local HEAD exactly matches origin/codex/issue-1932-verifiable-goal at the SHA above.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Exhaustive read-only software quality review

Reviewed SHA: 4a750400ff04d6449e36be752ef34fe45766322a
Base SHA: 49504fc9aa11a9ee70539c93a6f1afe64badad94
Verdict: CHANGES_REQUESTED
Findings: 4 total (2 P1, 2 P2)

P1

  1. Candidate code can still forge both protected pytest result channels by reading its worker sourcepdd/sync_core/runner.py:752-796, pdd/sync_core/runner.py:806-830, pdd/sync_core/runner.py:863-867, pdd/sync_core/runner.py:930-934, tests/test_sync_core_runner.py:896-916

    Moving the writable result to a randomized sibling directory does not hide it from candidate code. Each candidate-readable worker script embeds the absolute worker_output / worker_junit pathname, and candidate product code knows that script as sys.argv[0]. It can read the script, extract the /results-.../collection-....json or /results-.../execution-....xml literal, write a passing payload to that explicitly writable bind, and call os._exit(80). The checker accepts return code 80 and copies those candidate-authored bytes into the authoritative file. The new recursive regression searches only the controller directory (trusted.rglob("*")); the writable result is its sibling, and the test never reads the worker source, so it misses the implemented disclosure.

    Required fix: derive outcomes through a channel whose address and contents candidate code cannot read or write, and do not treat a same-interpreter candidate-authored JUnit/node-list file as authoritative. Add the exact source-reading adversarial product for both collection and execution and prove a failing assertion cannot become PASS.

  2. The product runtime closure is still incomplete; non-Python and installed inputs preserve stale evidencepdd/sync_core/runner.py:471-480, pdd/sync_core/runner.py:625-634, pdd/sync_core/supervisor.py:31-40, pdd/sync_core/supervisor.py:216-224

    The alias fix hashes every committed path ending in .py, but protected pytest can read the entire repository and sys.prefix. Arbitrary product code can therefore consume json, templates, native extensions, shared libraries, or installed distributions that are absent from support_closure_digest. An exact-head temporary-repository probe used product.py to read helper.json; changing only helper.json changed test behavior while runner_identity_digest() remained byte-for-byte identical. The same stale-evidence issue applies when product code imports an installed dependency outside the four checker distributions hashed below.

    Required fix: bind every readable product runtime input (including non-Python repository artifacts, native/installed dependencies, and interpreter/runtime files) or restrict the sandbox to an explicit measured closure. Add stale-evidence tests for a data file and an installed/native dependency.

P2

  1. Runner identity still does not bind the actual protected checker runtimepdd/sync_core/runner.py:589-621, pdd/sync_core/supervisor.py:31-57, pdd/sync_core/supervisor.py:190-230

    _checker_artifact_digest() now covers four local modules and four Python distributions, but outcome semantics also depend on the Python executable/stdlib, libc/loader, bubblewrap, sudo, setpriv, mount behavior, and other files exposed by _runtime_roots(). _measured_python_runtime() records only version strings. Changing any of those bytes preserves the signed runner digest while changing isolation or result derivation. The new tests cover supervisor.py and pytest only, not the released transitive runtime requested by the prior review.

    Required fix: bind a released checker/runtime closure by logical names and content digests, including interpreter/stdlib and sandbox executables, while excluding installation prefixes. Verify changed runtime/sandbox bytes reject old evidence and identical bytes remain prefix-portable.

  2. Linux signer containment only reaps descendants if its SIGTERM handler runspdd/sync_core/signer_process.py:14-49, pdd/sync_core/signer_process.py:103-142, tests/test_sync_core_trust.py:142-194

    The subreaper calls descendants() only in stop(). After a normal child return, lines 47-49 immediately exit without checking adopted descendants, so a signer can spawn a detached child with stdio redirected, return a signature, and leak that child. Because the signer and wrapper share a UID, the signer can also SIGSTOP or SIGKILL its parent wrapper; the outer timeout path sends only SIGTERM on Linux, then closes pipes and calls wait(timeout=.5) without a SIGKILL/process-tree fallback. The added test forces a cooperative sleeping parent through the handler and does not cover either escape.

    Required fix: use containment the signer cannot signal or outlive (for example, a protected cgroup/pid-namespace boundary with kill-all semantics), clean the scope on every exit path, and retain a bounded hard-kill fallback. Test normal-return detached leakage and a stopped/killed containment leader for both adapters.

Prior review verification

Prior item at 543bf6d5992258e85293ac8a4634ad5a104f658a Exact-head status
P1 result-channel forgery Not fixed: randomized worker paths are disclosed in candidate-readable source.
P1 aliased dynamic product closure Partially fixed: committed .py aliases are bound; other readable runtime inputs are not.
P2 checker/dependency identity Partially fixed: selected modules/distributions are bound; the protected runtime is not.
P2 detached signer liveness Partially fixed: cooperative timeout is reaped; normal exit and wrapper-signal escapes remain.

Previously closed linked-worktree policy, recovery CAS, mutation preflight, path-qualified selection, and seven-night minimum findings remain resolved at this SHA.

Verification

  • Read repository guidance, issue/PR description, complete base-to-head diff, affected implementation/tests/workflow, commit history, 79 issue comments, 3 formal reviews, and 4 inline threads. Reviewed the red/fix commits after the prior exhaustive review.
  • Exact-head probes reproduced the unchanged digest for a changed helper.json and confirmed that both candidate-readable workers contain their exact randomized writable result path.
  • Focused remediation selection: 4 passed, 3 skipped on macOS; the three Linux-only tests are green in exact-head CI but do not exercise the bypasses above.
  • Broader serial changed-area run: 1415 passed, 59 skipped, 8 failed; four are the known macOS fail-closed sandbox finalizer cases, and four ci_drift_heal order-dependent failures pass when that file is run independently (201 passed). Exact-head GitHub checks are all green.
  • git diff --check passed; worktree remained clean. Immediately before posting, the PR was open/non-draft and live head/base exactly matched the SHAs above.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to Sol-high review

@sol-high Re: #1985 (comment)

P1 #1: Candidate-readable workers disclose protected result channels

Status: FIXING
Plan: remove candidate-visible result addresses and derive collection/execution outcomes through a trusted post-sandbox channel; add source-reading forgery regressions for both phases.

P1 #2: Product runtime closure is incomplete

Status: FIXING
Plan: replace the partial source digest with an explicit measured/readable runtime closure that covers repository data plus installed/native runtime inputs, and prove data/dependency mutations invalidate evidence.

P2 #1: Runner identity does not bind the actual checker/runtime closure

Status: FIXING
Plan: make the signed identity include a logical-name/content-digest manifest for the checker interpreter/runtime and sandbox executables, preserving prefix-independent equality for byte-identical closures.

P2 #2: Linux signer containment misses normal return and leader-loss escapes

Status: FIXING
Plan: make Linux containment own a kill-all scope, clean it on every exit path, and retain a bounded SIGKILL fallback; cover detached normal return and stopped/killed leaders for both adapters.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Red TDD evidence

Pushed test-only commit $(git rev-parse HEAD) before implementation.

Focused red run:

3 failed, 7 skipped
FAILED test_runner_identity_binds_product_readable_repository_data
FAILED test_released_runtime_digest_binds_installed_native_dependency
FAILED test_released_runtime_digest_binds_runtime_and_sandbox_bytes_prefix_portably

The source-reading result-channel and signer containment cases are Linux/bubblewrap-gated and will run in CI; the current macOS protected runner fails closed by design.

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.

2 participants