Skip to content

fix(sync): bound global dry-run path failures#1996

Open
gltanaka wants to merge 29 commits into
codex/issue-1932-verifiable-goalfrom
codex/issue-1932-cloud-dry-run-liveness
Open

fix(sync): bound global dry-run path failures#1996
gltanaka wants to merge 29 commits into
codex/issue-1932-verifiable-goalfrom
codex/issue-1932-cloud-dry-run-liveness

Conversation

@gltanaka

Copy link
Copy Markdown
Contributor

Stacked on codex/issue-1932-verifiable-goal.\n\nTests: focused regression; report and maintenance suites; credential-free clone E2E with JSON parse and zero writes.

@chatgpt-codex-connector

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

Pre-fix investigation and TDD evidence

Sibling hunt. routes in directly to , bypassing . The report hard-coded and then classified every discovered unit independently. called the mutating , while resolution errors were caught per unit, which is the mechanism behind repeated failure output. This PR fixes only that report/discovery route and root JSON emission; it does not touch runners/adapters, alias/transaction, certificate/provenance, or rollout data.

History. I reviewed (global sync foundation), (canonical proof boundaries), and (CWD-independent path alignment). The established convention is to anchor path resolution at the owning project/config root and preserve canonical activation as an explicit protected-policy gate. This PR follows that convention. Non-active canonical policy remains non-authoritative legacy reporting.

Red commit: was pushed before the fix. Its focused test failed because stdout began with before the JSON payload, and the report returned for an absolute configured prompt root. The failure is recorded by .

Fix evidence: makes the same test pass. The test asserts one parseable JSON document, two discovered configured-root units, and no project writes.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Corrected Investigation and TDD Evidence

The preceding comment was corrupted by shell interpolation; this is the authoritative evidence.

Sibling hunt. sync --dry-run --json routes in pdd/commands/maintenance.py directly to continuous_sync.build_report, bypassing run_global_sync. The report hard-coded <project>/prompts and classified every discovered unit independently. classify_unit called the mutating get_fingerprint_path, while resolution errors were caught per unit. This PR fixes only the report/discovery route and root JSON emission. It does not touch runners/adapters, alias/transaction, certificate/provenance, or rollout data.

History. Reviewed 4351d7320 (global sync foundation), f39dd6090 (canonical proof boundaries), and ea6528c04 (CWD-independent path alignment). The established convention is to anchor resolution at the owning project/config root and retain canonical activation as an explicit protected-policy gate. Non-active canonical policy remains on the non-authoritative legacy reporting path.

Red commit. 88a374c1a was pushed before the fix. tests/test_cloud_global_dry_run.py failed because stdout began with the onboarding message before the JSON payload, and the report returned summary.total == 0 for an absolute configured prompt root.

Fix evidence. 208b609df makes the same test pass. It asserts exactly one parseable JSON document, two configured-root units, and no project writes.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Verification Evidence

  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 python -m pytest -q tests/test_cloud_global_dry_run.py: passed (1 test).
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 python -m pytest -q tests/test_sync_core_reporting.py tests/test_commands_maintenance.py -q: passed.
  • python -m pylint pdd/continuous_sync.py pdd/core/cli.py tests/test_cloud_global_dry_run.py: pdd/core/cli.py has pre-existing baseline diagnostics; no new diagnostic was introduced by this one-line JSON guard. pdd/continuous_sync.py retains an existing broad-exception warning outside this diff.
  • git diff --check: passed.

Credential-free clone E2E: cloned the branch into a disposable local repository, configured an external absolute prompt root containing two prompts, then ran python -m pdd --no-core-dump sync --dry-run --json with local mode and auto-update disabled. jq validated summary.total == 2 and two units. Git status immediately before and after the command was identical; no writes occurred.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Follow-up Read-only Hardening

The adversarial review trace exposed one additional report-path concern: the legacy read_fingerprint helper creates .pdd/meta. This follow-up removes it from report classification and decodes the already-read fingerprint payload directly. Commit: HEAD.

Re-ran: python -m pytest -q tests/test_cloud_global_dry_run.py tests/test_sync_core_reporting.py tests/test_commands_maintenance.py, git diff --check, and focused pylint for the report/test modules.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Final E2E Evidence

Repeated the disposable credential-free clone E2E at final head f9d63951d: python -m pdd --no-core-dump sync --dry-run --json produced two units; jq parsed the single JSON document; Git status was identical before and after.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from f9d6395 to 5e12694 Compare July 11, 2026 02:41
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial review — PR #1996 @ 5e12694

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

P1 (blocking)

  • [P1] Absolute configured prompt roots are traversed without any safety bound — pdd/continuous_sync.py:151-177,333-339
    _configured_prompt_roots() now accepts every prompts_dir from project .pddrc, resolves absolute values as-is, and discover_units() recursively runs rglob("*_*.prompt") over each configured directory. A candidate-controlled config can point prompts_dir at /, a home directory, a mounted checkout cache, or another huge/sensitive tree, causing pdd sync --dry-run --json to hang or enumerate paths outside the repository. The regression test proves one benign absolute directory works, but it does not bound traversal or enforce a trust boundary for untrusted PR configs.
    Suggested fix: Reject dangerous absolute roots or require them to be under an approved workspace/project boundary, skip hidden/system directories, and enforce a maximum traversal budget with a JSON failure when exceeded; add tests for //home-like roots and large trees.

P2 (non-blocking)

  • [P2] Missing fingerprints now mask path-resolution failures — pdd/continuous_sync.py:617-638
    classify_unit() returns UNBASELINED as soon as the fingerprint JSON is missing, before calling get_pdd_file_paths(). Before this change, path-resolution errors were surfaced as FAILURE even for unbaselined units. Invalid architecture or ambiguous module configuration can now be hidden behind a generic missing-fingerprint result, which weakens the report as a diagnostic and makes the new path-failure tests narrower than the behavior they claim to protect.
    Suggested fix: Use a read-only fingerprint path helper, but still run bounded path resolution and report path failures distinctly; add a regression where a discovered prompt has no fingerprint and an invalid/ambiguous path config.

Notes

  • Reviewed against actual base codex/issue-1932-verifiable-goal @ 63d31004f3e9633d11683cf13e2573f9fb8a2d39, not main.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to Codex review

@codex Re: #1996 (comment)

P1 #1: Absolute configured prompt roots are traversed without any safety bound

Status: FIXING
Plan: Add a trust/bounds check for configured absolute prompt roots, skip unsafe/system traversal, enforce a traversal budget with JSON-visible failure, and cover dangerous absolute roots plus large trees.

P2 #1: Missing fingerprints now mask path-resolution failures

Status: FIXING
Plan: Treat this as a real diagnostic regression; keep fingerprint reads read-only but perform bounded path resolution before returning unbaselined status and add a missing-fingerprint path-failure regression.

@chatgpt-codex-connector

Copy link
Copy Markdown

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

@gltanaka

Copy link
Copy Markdown
Contributor Author

Follow-up fix evidence

Fixed Codex xhigh findings from #1996 (comment).

Commits:

  • 19577d0c7 test(sync): reproduce dry-run discovery safety gaps — pushed before the fix. Red run: PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 python -m pytest -q tests/test_cloud_global_dry_run.py failed with 3 expected failures: unsafe absolute prompt root accepted, missing MAX_PROMPT_DISCOVERY_FILES, and missing fingerprint masking path resolution failure.
  • 7f0c74cbf fix(sync): bound dry-run prompt discovery — validates configured absolute prompt roots against the workspace boundary, skips hidden/system traversal, enforces a prompt discovery budget surfaced as JSON failure, and performs path resolution before returning missing-fingerprint UNBASELINED without leaving dry-run-created empty directories.

Verification:

  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 python -m pytest -q tests/test_cloud_global_dry_run.py -> 4 passed.
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 python -m pytest -q tests/test_cloud_global_dry_run.py tests/test_sync_core_reporting.py tests/test_commands_maintenance.py -> 58 passed.
  • python -m pylint pdd/continuous_sync.py tests/test_cloud_global_dry_run.py reports only existing pdd/continuous_sync.py module-size and broad-exception diagnostics; no new discovery/classification warnings remain.
  • git diff --check passed before commit.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from 7f0c74c to 93647b9 Compare July 11, 2026 06:12
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial re-review — PR #1996 @ 93647b9

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

P1 (blocking)

  • [P1] Candidate prompts_dir: .. still escapes the project and enumerates sibling trees — pdd/continuous_sync.py:204-259
    The unsafe-root finding is only partially resolved. _configured_prompt_roots() resolves relative config values before validation, and _is_safe_prompt_root() accepts any resolved path below base.resolve().parent. A candidate .pddrc can therefore set prompts_dir: .. and make sync --dry-run --json walk the whole parent workspace, including sibling repositories or prompt trees outside the project. A focused probe against this head with workspace/project/.pddrc set to prompts_dir: .. caused the report to discover workspace/sibling-secret/secret_python.prompt with no failure. That still violates the dry-run trust boundary and can leak or stall on sibling content.
    Suggested fix: After resolving both relative and absolute configured roots, accept roots inside the project by default and require any external prompt root to come from a protected, non-candidate allowlist or explicit trusted workspace policy. Add regressions for prompts_dir: .., ../sibling, and an absolute path equal to the workspace parent.

P2 (non-blocking)

  • [P2] Traversal budget counts prompt files, not traversal work — pdd/continuous_sync.py:158-180
    _bounded_prompt_paths() only stops after more than MAX_PROMPT_DISCOVERY_FILES matching prompt files. A huge configured tree with few or no *_*.prompt files still walks every directory and filename before returning, so the large-tree liveness part of the prior finding remains. Skipping a few hidden/system directory names helps common cases but does not bound arbitrary workspace or repository trees.
    Suggested fix: Track visited directories and/or directory entries as part of the traversal budget, fail closed when either budget is exceeded, and surface os.walk errors through JSON instead of silently continuing.

  • [P2] Per-unit cleanup scans the whole project twice and can mutate concurrent empty directories — pdd/continuous_sync.py:558-576, pdd/continuous_sync.py:752-781
    classify_unit() now snapshots every directory under the project before every unit and then scans the entire tree again to remove newly empty directories. That makes dry-run classification O(units * project directories), so a report with many discovered prompts can still stall even when prompt discovery is capped. It also means a read-only dry-run may rmdir() any empty directory created by another process between the two scans, not just directories produced by the legacy path helper.
    Suggested fix: Replace the directory-creating legacy path helper with a read-only resolver for reports, or track only exact directories created by the helper and remove only those after verifying they are still empty.

Prior findings verified

  • Absolute prompt-root traversal: not fully resolved; relative and parent-workspace roots still escape the project, and traversal work is not fully bounded.
  • Missing fingerprints masking path-resolution failures: resolved; classify_unit() now resolves paths before returning UNBASELINED for missing/invalid fingerprints.

Notes

  • Reviewed live open PR state against actual stacked base codex/issue-1932-verifiable-goal @ 9bd278dec20482e4d8be66b2202a6a3a025f2e03 and head 93647b9f912ec62afac9e8acde09e8aa11fa9e02.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Re-review follow-up evidence

Fixed latest xhigh re-review findings from #1996 (comment).

Commits:

  • 8a7a74f4f test(sync): reproduce dry-run rereview gaps — red tests covered prompts_dir: .., ../sibling, absolute workspace-parent roots, directory-entry traversal budget, and racy cleanup removing a concurrent empty directory.
  • 951a51183 fix(sync): keep dry-run discovery project-contained — validates configured roots inside the project boundary after resolution, adds MAX_PROMPT_DISCOVERY_ENTRIES, surfaces traversal errors in JSON, and replaces dry-run classification path resolution with a read-only report resolver instead of whole-project cleanup scans.

Verification:

  • Red run before fix: tests/test_cloud_global_dry_run.py failed 6 expected cases for escaped roots, missing entry budget, old reason text, and racy cleanup.
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py -> 9 passed.
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py tests/test_sync_core_reporting.py tests/test_commands_maintenance.py -> 63 passed.
  • conda run -n pdd pylint pdd/continuous_sync.py tests/test_cloud_global_dry_run.py reports only the existing pdd/continuous_sync.py module-size baseline (too-many-lines).
  • git diff --check passed.
  • Credential-free CLI smoke: disposable project-contained prompts were discovered; prompts_dir: .. produced unsafe_prompt_root; sibling prompt was not reported; command output parsed as JSON. DRY_RUN_E2E=PASS.

Branch pushed; no merge performed.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from 951a511 to 37e976f Compare July 11, 2026 06:52
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial re-review round 3 — PR #1996 @ 37e976f

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

P1 (blocking)

  • None.

P2 (non-blocking)

  • [P2] Missing-prompt repair still performs an unbounded whole-project traversal — pdd/continuous_sync.py:711-724, pdd/continuous_sync.py:868-874
    Prompt discovery is now bounded, but classify_unit() can still bypass those limits when legacy metadata exists for a missing prompt. In that path _repair_missing_fingerprinted_paths() calls _find_matching_artifact(), which uses root.rglob(filename) over the entire project. It filters .git, .pdd, and __pycache__ only after Path.rglob has already descended into them, and it has no directory-entry budget. A focused probe against this head showed classify_unit() visiting .git/objects/missing.py through this repair path, so a stale fingerprint can still make sync --dry-run --json traverse huge internal or dependency trees despite the new discovery caps.
    Suggested fix: Replace _find_matching_artifact() with the same bounded os.walk style used for prompt discovery, prune skipped directories before descent, enforce an entry budget, and surface a JSON FAILURE when repair search exceeds it. Alternatively, disable whole-project repair in machine JSON dry-run. Add a regression with stale metadata for a missing prompt and a large .git/node_modules tree.

Prior Findings Verified

  • Dangerous absolute and parent/sibling prompts_dir roots: resolved for discovery. Resolved roots must be under the project root, and unsafe roots surface as unsafe_prompt_root failures.
  • Prompt discovery budget counted only prompt files: resolved for discovery. _bounded_prompt_paths() now counts directory entries as well as matching prompt files.
  • Missing fingerprints masking path-resolution failures: resolved. classify_unit() resolves paths before returning UNBASELINED for missing/invalid fingerprints.
  • Per-unit dry-run cleanup mutating concurrent empty directories: resolved. The report path now uses read-only path resolution instead of whole-project cleanup scans.

Notes

  • Reviewed live open PR state against actual stacked base codex/issue-1932-verifiable-goal @ b8f05dcd93d3c18067facaedde7bbc81216c4c6e and head 37e976f89af2716843198d8350c6af378cab72b4.
  • Focused probe was read-only with respect to the repository and used only disposable temporary files.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to Codex re-review round 3

@codex Re: #1996 (comment)

P2 #1: Missing-prompt repair still performs an unbounded whole-project traversal

Status: FIXING
Plan: Replace the missing-artifact repair rglob with pruned bounded traversal, fail deterministically when the repair budget is exceeded, and add regressions for stale metadata with large skipped/internal trees so machine JSON dry-run cannot bypass the discovery caps.

@chatgpt-codex-connector

Copy link
Copy Markdown

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

@gltanaka

Copy link
Copy Markdown
Contributor Author

Round-3 follow-up evidence

Fixed latest xhigh re-review finding from #1996 (comment).

Commits:

  • 730724c86 test(sync): reproduce missing-artifact repair traversal gap — red test proved stale fingerprint repair had no bounded repair traversal budget.
  • e5cd646d5 fix(sync): bound missing-artifact repair traversal — replaces missing-artifact rglob with pruned os.walk, skips hidden/system directories before descent, enforces MAX_REPAIR_DISCOVERY_ENTRIES, surfaces deterministic repair_traversal_budget / repair_traversal_error failures, and preserves read-only classification behavior.

Verification:

  • Red test now passes: test_missing_prompt_repair_uses_bounded_pruned_traversal.
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py tests/test_sync_core_reporting.py tests/test_commands_maintenance.py -> 64 passed.
  • conda run -n pdd pylint pdd/continuous_sync.py tests/test_cloud_global_dry_run.py reports only the existing pdd/continuous_sync.py module-size baseline (too-many-lines).
  • Credential-free CLI smoke/E2E: disposable project-contained sync --dry-run --json parsed as JSON and reported one discovered unit (DRY_RUN_E2E=PASS).
  • git diff --check passed.

Branch pushed; no merge performed.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from e5cd646 to ce27337 Compare July 11, 2026 07:35
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial re-review round 4 — PR #1996 @ ce27337

Verdict: MERGE
Findings: 0 total (0 P1, 0 P2)

P1 (blocking)

  • None.

P2 (non-blocking)

  • None.

Prior Findings Verified

  • Dangerous absolute and parent/sibling prompts_dir roots: resolved. Configured roots are resolved and must remain under the project root; unsafe roots surface as unsafe_prompt_root failures and are not traversed.
  • Prompt discovery budget counted only prompt files: resolved. _bounded_prompt_paths() counts directory entries and matching prompt files, prunes hidden/system directories before descent, and surfaces traversal failures in JSON.
  • Missing fingerprints masking path-resolution failures: resolved. classify_unit() resolves report paths before returning UNBASELINED for missing/invalid fingerprints.
  • Per-unit dry-run cleanup mutating concurrent empty directories: resolved. The report path uses read-only path resolution and no longer scans/removes project directories after classification.
  • Missing-prompt repair unbounded traversal: resolved. _find_matching_artifact() now uses pruned os.walk, counts repair traversal entries, and returns deterministic repair_traversal_budget / repair_traversal_error failures.

Read-only Probes

  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py -> 10 passed.
  • Disposable safe-root CLI smoke: project-contained prompts_dir: prompts returned JSON with one discovered widget unit.
  • Disposable unsafe-root CLI smoke: prompts_dir: ../sibling returned one unsafe_prompt_root failure and did not report the sibling prompt.

Notes

  • Reviewed live open PR state against actual base codex/issue-1932-verifiable-goal @ 87aea93087dc246c9224c2df4fd3b6fd8b102dcf and head ce27337e6bd3883554b43d318891f71264469911.
  • Local inspection/probes used a disposable /tmp clone only; no repository files or PR branches were modified.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from ce27337 to 366388d Compare July 11, 2026 08:46
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial re-review round 5 — PR #1996 @ 366388d

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

P1 (blocking)

  • [P1] Stale-metadata repair still reads and accepts symlinked artifacts outside the project — pdd/continuous_sync.py:712-788, pdd/continuous_sync.py:896-952
    The root and traversal-budget fixes keep configured prompt discovery project-contained, but the missing-prompt repair path still follows symlink files. _find_matching_artifact() walks the project, then calls candidate.is_file() and calculate_sha256(candidate) for a matching filename; both follow symlinks. A candidate can leave stale .pdd/meta/widget_python.json, make the prompt missing, and place project/widget.py -> /outside/file. If the stale hash matches, classify_unit() repairs the code path to that symlink and returns IN_SYNC, so sync --dry-run --json still reads and validates outside-checkout content despite the project-contained discovery boundary. A disposable probe at this head reproduced {"classification":"IN_SYNC","failure":null} for a project-local symlink pointing to an outside file.
    Suggested fix: In repair search and final classification hashing, reject symlink artifact candidates before hashing or require candidate.resolve(strict=True) to remain under the project root; surface a deterministic JSON failure instead of repairing to the symlink. Add a regression with stale metadata, a missing prompt, and widget.py symlinked outside the project.

P2 (non-blocking)

  • None.

Prior Findings Verified

  • Dangerous absolute and parent/sibling prompts_dir roots: resolved for configured prompt discovery. Resolved roots must remain under the project root, and unsafe roots surface as unsafe_prompt_root failures.
  • Prompt discovery budget counted only prompt files: resolved for discovery. _bounded_prompt_paths() counts directory entries and matching prompt files, prunes hidden/system directories before descent, and surfaces traversal errors in JSON.
  • Missing fingerprints masking path-resolution failures: resolved. classify_unit() resolves report paths before returning UNBASELINED for missing/invalid fingerprints.
  • Per-unit dry-run cleanup mutating concurrent empty directories: resolved. The report path uses read-only path resolution and no longer scans/removes project directories after classification.
  • Missing-prompt repair unbounded directory traversal: mostly resolved. _find_matching_artifact() now uses pruned bounded os.walk, but the symlink-following bug above leaves an outside-project read/validation escape.

Read-only Probes

  • Live base/head: codex/issue-1932-verifiable-goal @ f1dca1995daf794f5d9889c9d640fc0c392ff698; head codex/issue-1932-cloud-dry-run-liveness @ 366388da1d7e7b49f2d243863ee4848112c20b0e.
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py tests/test_sync_core_reporting.py tests/test_commands_maintenance.py -> 64 passed.
  • Disposable symlink probe: stale fingerprint + missing prompt + project/widget.py symlink to an outside file with matching hash returned IN_SYNC instead of failure.
  • git diff --check f1dca1995daf794f5d9889c9d640fc0c392ff698 366388da1d7e7b49f2d243863ee4848112c20b0e -> clean.
  • Current GitHub checks: auto-heal pass; heal pass. Merge state: CLEAN / MERGEABLE.

Notes

  • Reviewed the full live diff and surrounding code in a disposable /tmp checkout. No repository files or PR branches were modified.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from 366388d to 0544203 Compare July 11, 2026 09:27
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial re-review round 6 — PR #1996 @ 0544203

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

P1 (blocking)

  • [P1] Legacy include-dependency hashes still read outside-project paths before dry-run safety checks — pdd/continuous_sync.py:979-1035, pdd/sync_determine_operation.py:1813-1821, pdd/sync_determine_operation.py:1927-1936
    The new dry-run safety gate validates prompt/code/example/test paths before calculate_current_hashes(), but it does not validate fingerprint.include_deps. classify_unit() then passes stored_include_deps=fingerprint.include_deps into the legacy hash helper. When the prompt currently has no <include> tags, calculate_prompt_hash() re-hashes every stored dependency path as absolute or relative to CWD, follows symlinks, accepts outside-project regular files, and can block on special files before any project-boundary or traversal-budget check. A disposable probe with .pdd/meta/widget_python.json containing an absolute outside include dependency returned classification == "IN_SYNC", so the dry-run can still read and validate outside-checkout content despite the new artifact/symlink checks.
    Suggested fix: Validate stored include dependency paths before hashing: reject absolute paths outside the project, symlinks, non-regular files, and missing/broken paths with a deterministic JSON failure such as unsafe_include_deps / unsafe_artifacts. Prefer a safe report-only wrapper around include hashing so calculate_current_hashes() cannot read untrusted legacy dependency paths before boundary checks. Add regressions for absolute outside include deps, symlink include deps, and a special-file path that must not be opened.

P2 (non-blocking)

  • None.

Prior Findings Verified

  • Dangerous absolute and parent/sibling prompts_dir roots: resolved for configured prompt discovery. Resolved roots must remain under the project root and unsafe roots surface as unsafe_prompt_root failures.
  • Prompt discovery budget counted only prompt files: resolved for discovery. _bounded_prompt_paths() counts directory entries and matching prompt files, prunes hidden/system directories before descent, and surfaces traversal errors in JSON.
  • Missing fingerprints masking path-resolution failures: resolved. classify_unit() resolves report paths before returning UNBASELINED for missing/invalid fingerprints.
  • Per-unit dry-run cleanup mutating concurrent empty directories: resolved. The report path uses read-only path resolution and no longer scans/removes project directories after classification.
  • Missing-prompt repair unbounded traversal: resolved for directory traversal. _find_matching_artifact() uses pruned os.walk, counts repair entries, and reports repair traversal budget/error failures.
  • Symlinked generated artifact repair/read escape from round 5: resolved for prompt/code/example/test artifacts. _artifact_path_violation() rejects symlink artifacts before hashing, and the e2e symlink regression passes.
  • Stored include-dependency outside-project read/hang escape: not resolved; finding above.

Read-only Probes

  • Live base/head: codex/issue-1932-verifiable-goal @ d25f0fbc927ae1c102e54093b6bd33d315a5a37b; head codex/issue-1932-cloud-dry-run-liveness @ 0544203a9fb505cd2ed0e0e102700142d1e5685f.
  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py tests/test_continuous_sync_path_policy.py tests/test_sync_core_fingerprint_store.py tests/e2e/test_issue_1932_continuous_sync_guarantee.py::test_issue_1996_symlinked_generated_artifact_is_failure_not_in_sync tests/e2e/test_issue_1932_continuous_sync_guarantee.py::test_issue_1932_deleted_canonical_artifact_is_not_masked_by_duplicate tests/e2e/test_issue_1932_continuous_sync_guarantee.py::test_issue_1932_deleted_prompt_stays_discovered_from_metadata -> 24 passed.
  • Disposable include-deps probe: legacy fingerprint with absolute outside include dependency and matching prompt hash returned IN_SYNC instead of failure.
  • git diff --check d25f0fbc927ae1c102e54093b6bd33d315a5a37b 0544203a9fb505cd2ed0e0e102700142d1e5685f -> clean.
  • Current GitHub checks: auto-heal pass; heal pass. Merge state: CLEAN / MERGEABLE.

Notes

  • Reviewed the full live diff and surrounding code in a disposable /tmp checkout. No repository files or PR branches were modified.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from 4feebc0 to 06aa903 Compare July 11, 2026 12:49
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex xhigh adversarial review round 7 — PR #1996 @ 06aa903

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

P1 (blocking)

  • [P1] Relative legacy include dependencies are validated against the project root but hashed against process CWD — pdd/continuous_sync.py:582-620,1087-1106; pdd/sync_determine_operation.py:1813-1821,1927-1936
    _include_dep_violation() interprets a relative fingerprint key as root / key, but both legacy hashing paths interpret that same key from Path.cwd(). Since project_root() intentionally supports invocation from a project subdirectory, the guard can approve one regular file and then hash a different subdirectory-relative file. That second path can be a symlink outside the checkout or a special file, reopening the exact outside-read/hang class from round 6. All new dependency tests invoke classification with CWD at the project root or use absolute keys, so they do not cover this mismatch.
    Fix: resolve each stored dependency once under an explicit project-root base and use that same checked path for hashing while preserving the original key in the prompt digest; do not pass unnormalized relative keys into the CWD-sensitive legacy helper. Add an existing-suite regression that invokes from a nested project directory and ensures the alternate CWD-relative path is never opened.

  • [P1] Symlinked metadata ancestors still escape the checkout before fingerprint safety checks — pdd/continuous_sync.py:422-428,541-550,975-992
    _metadata_identities() globs base/.pdd/meta without validating that directory, and _load_fingerprint_json() only lstat()s the final JSON path. If .pdd, .pdd/meta, or another ancestor is a symlink, lstat() sees the outside target file as a regular leaf and read_text() reads it; discovery can also enumerate the outside metadata directory. The direct-leaf symlink test and FingerprintStore.read_legacy() containment check do not protect this separate continuous-sync reader.
    Fix: validate the metadata root and every path component with lstat, require the resolved metadata directory and fingerprint file to remain under base, and surface a deterministic unsafe_metadata JSON failure before globbing or reading. Add coverage for a regular fingerprint reached through a symlinked .pdd/meta ancestor.

P2 (meaningful robustness)

  • [P2] Non-dict include_deps bypasses the new guard and can abort machine JSON — pdd/continuous_sync.py:557-570,622-627,1087-1106; pdd/sync_determine_operation.py:1813,1930
    _fingerprint_from_payload() accepts the raw JSON value, while _unsafe_include_deps() treats every non-dict value as no violations. A truthy list/string then reaches dict(stored_deps) or .items() and raises instead of producing the promised JSON report.
    Fix: accept only null or a dictionary of non-empty string paths to string digests; classify every other shape as invalid/unsafe metadata, and convert hash-layer exceptions into a per-unit FAILURE.

  • [P2] Invalid .pddrc shapes are inconsistently hidden or allowed to crash dry-run JSON — pdd/continuous_sync.py:240-267
    Loader ValueErrors are silently converted to an empty config and fallback prompts/, while a syntactically valid contexts list reaches .values() and a non-dict defaults reaches .get(). Thus candidate configuration errors either scan an unintended root or prevent any JSON document from being emitted.
    Fix: validate contexts and defaults types and turn parse/shape failures into a deterministic discovery FAILURE instead of defaulting or raising.

Prior review items verified from existing evidence

  • Dangerous absolute/parent/sibling prompt roots, prompt file/entry traversal budgets, and JSON-visible traversal errors: resolved for configured prompt discovery.
  • Missing-fingerprint path-resolution masking and racy whole-project cleanup: resolved; report path resolution is read-only and runs before UNBASELINED.
  • Missing-artifact repair traversal: resolved; traversal is pruned, entry-bounded, and reports deterministic failures.
  • Generated artifact symlink escape: resolved for prompt/code/example/test paths and repair candidates.
  • Round-6 stored include dependency escape: absolute outside, missing, direct symlink, and FIFO cases are resolved, but relative keys remain unsafe because validation and hashing use different bases (P1 above).

Existing verification

  • PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 conda run -n pdd pytest -q tests/test_cloud_global_dry_run.py tests/test_continuous_sync_path_policy.py tests/test_sync_core_fingerprint_store.py [focused issue-1932/1996 e2e tests] tests/test_sync_core_reporting.py tests/test_commands_maintenance.py -> 83 passed.
  • git diff --check origin/codex/issue-1932-verifiable-goal...HEAD -> clean.
  • GitHub checks: auto-heal pass; heal pass.
  • Reviewed against origin/codex/issue-1932-verifiable-goal @ 72ad46c4b3972e47471a1e24bd89be992b217a0c; no custom scripts, repositories, payloads, or probes were created, and the worktree stayed clean.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to Codex xhigh review round 7

@codex Re: #1996 (comment)

P1 #1: Relative legacy include dependencies use mismatched validation and hash bases

Status: FIXING
Plan: Resolve stored relative dependency keys once against the explicit project root, validate and hash that exact path, preserve the original key in the digest, and add a nested-CWD regression proving the alternate CWD-relative path is never opened.

P1 #2: Symlinked metadata ancestors escape before fingerprint checks

Status: FIXING
Plan: Validate metadata-root components before globbing and fingerprint components before reading, enforce resolved containment under the project base, report deterministic unsafe_metadata, and add a symlinked-ancestor regression.

P2 #1: Malformed include_deps can bypass validation and abort JSON output

Status: FIXING
Plan: Accept only null or a mapping of non-empty string paths to string digests, classify all other shapes as unsafe metadata, keep hash failures unit-scoped, and add malformed-shape regressions.

P2 #2: Invalid .pddrc shapes are hidden or crash dry-run JSON

Status: FIXING
Plan: Validate parsed configuration plus contexts and defaults object shapes, convert parse/shape errors into deterministic discovery failures, and add regressions ensuring JSON is emitted without scanning fallback roots.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Round 7 implementation evidence

Implemented all four findings from #1996 (comment).

Investigation

  • Sibling paths: both calculate_prompt_hash() and the stored-dependency fallback in calculate_current_hashes() resolved relative legacy keys from process CWD. Continuous sync now supplies an explicit project-root base to both while retaining each original key in the composite digest. Existing callers keep the legacy default.
  • Metadata convention: reused the repository's component-by-component lstat() safety pattern from artifact/include validation; metadata ancestors and leaves are checked before glob/read and resolved containment is enforced.
  • Config convention: discovery failures already have a machine-readable DiscoveryFailure path, so .pddrc parse and shape errors now use it and fail closed before prompt/metadata fallback discovery.
  • Prior commits inspected: 06aa9031c / 8d8d70a6a (round-6 include-dependency guard and tests), 21a79fa92 (symlinked artifact rejection), and 26eebf9e2 (project-contained dry-run discovery).

Strict TDD commits

  • RED test commit (pushed before implementation): 0e00b79bf test(sync): cover round-7 metadata safety gaps
  • GREEN fix commit / new head: f8ccfd3fd fix(sync): fail closed on unsafe legacy metadata

Red evidence:

  • Explicit-root regression: TypeError: calculate_prompt_hash() got an unexpected keyword argument 'dependency_root' (1 failed).
  • Real CLI regressions: 7 failed — malformed include_deps remained IN_SYNC, symlinked metadata lacked unsafe_metadata, and invalid .pddrc either emitted no JSON or silently fell back.

Green evidence:

  • conda run -n pdd pytest -q tests/test_sync_determine_operation.py::TestFingerprintIncludeDependencies tests/e2e/test_issue_1932_continuous_sync_guarantee.py tests/test_continuous_sync_path_policy.py tests/test_cloud_global_dry_run.py53 passed.
  • Focused round-7 + adjacent round-6 cases through the real python -m pdd sync --dry-run --json harness → 13 passed.
  • conda run -n pdd pylint pdd/continuous_sync.py10.00/10.
  • conda run -n pdd pylint --disable=all --enable=E,F pdd/sync_determine_operation.py tests/e2e/test_issue_1932_continuous_sync_guarantee.py10.00/10. Full unrestricted lint of sync_determine_operation.py remains noisy from pre-existing repository-wide style debt; no new error/fatal diagnostics.
  • git diff --check → clean.

Push was a normal fast-forward; no rebase or force push was used. New SHA: f8ccfd3fd.

@gltanaka gltanaka force-pushed the codex/issue-1932-cloud-dry-run-liveness branch from a1162b1 to a2f9d5a Compare July 11, 2026 20:07
@gltanaka

Copy link
Copy Markdown
Contributor Author

Codex adversarial review — PR #1996 @ a2f9d5a

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

P1 (blocking)

  • [P1] An unresolved live include falls back to the unsafe ancestor resolver — pdd/sync_determine_operation.py:1765-1787,1910-1940,2052-2063
    _validated_report_live_includes() returns (True, None) when a declared dependency is missing from the two project-contained candidates. calculate_current_hashes() passes that None onward, and calculate_prompt_hash() interprets it as “validation was not supplied,” then calls _legacy_dependency_path() from a synthetic project-root prompt and walks every ancestor. A focused probe with <include>ancestor-only.txt</include> absent from the project but present in its parent read the outside file bytes and returned PROMPT_CHANGED instead of a safety failure. This reopens the round-8/13 pre-read validation finding.
    Suggested fix: Represent “validated but unresolved” distinctly from “no validation requested,” fail closed on any unresolved live reference, and make the report path incapable of invoking _legacy_dependency_path() after validation starts.

  • [P1] Report hashing breaks legacy v1 alias compatibility — pdd/sync_determine_operation.py:1910-1915
    Legacy v1 hashes sort and deduplicate the declared strings before resolving them. The report-only branch instead resolves first and runs sorted(set(resolved_dependencies)). Thus aliases such as dep.txt and ./dep.txt historically contribute the same file twice, but the new dry-run contributes it once. A focused probe produced different hashes from calculate_prompt_hash(..., hash_version=1) and calculate_current_hashes(..., dependency_root=root) for unchanged bytes, so a valid legacy fingerprint is falsely reported as prompt drift. The added compatibility test covers only duplicate identical declarations, not aliases.
    Suggested fix: Carry (declared_reference, validated_path) pairs and preserve v1's exact sorted(set(declared_strings)) ordering/deduplication before reading the corresponding validated paths.

  • [P1] Nested architecture reports certify different example/test paths than live sync — pdd/continuous_sync.py:1069-1143; pdd/sync_determine_operation.py:1219-1325
    _resolve_report_paths() correctly anchors a matched code filepath at architecture_path.parent, but always anchors legacy example/test directories at the top-level base. The live architecture branch anchors code, example, and test paths at the architecture owner. In a reproduced packages/store/architecture.json case, both resolvers chose packages/store/src/pages/cart.tsx, while the report chose top-level examples/ and tests/ and live sync chose packages/store/examples/ and packages/store/tests/. The current nested-architecture test asserts only the code path. This can yield false failures or a green report over duplicate top-level artifacts while live sync uses changed nested artifacts.
    Suggested fix: Factor the complete architecture result, including derived stems and all artifact roots, into one pure resolver shared by report and live callers; add paired assertions for code, example, test, and test_files under nested architectures.

P2 (robustness / liveness)

  • [P2] Metadata enumeration still materializes a wide directory before enforcing the budget — pdd/continuous_sync.py:627-670
    list(islice(meta_root.iterdir(), remaining + 1)) looks bounded, but on the required Python 3.12 runtime Path.iterdir() iterates an os.listdir(self) result, which materializes every entry before yielding the first one. A single very wide .pdd/meta directory therefore retains the round-11 time/memory failure despite the post-enumeration allowance. The test replaces Path.iterdir() with a lazy test generator and does not exercise this runtime behavior.
    Suggested fix: Use the same explicit os.scandir per-entry debit pattern as prompt and repair traversal, stopping at allowance + 1 without os.listdir or sorting an unbounded inventory.

  • [P2] Config ownership work remains multiplicative outside the traversal budget — pdd/continuous_sync.py:231-289,320-404,1034-1066
    Every discovered prompt reparses its nearest .pddrc and scans all contexts in _prompt_ownership(), and classification reparses the owning config again. MAX_PROMPT_DISCOVERY_ROOTS is applied only after root deduplication, so a config may contain an unbounded number of contexts sharing one root. With up to 10,000 accepted prompts, candidate-controlled config parsing/context scans become O(prompts × contexts) and can dominate runtime after traversal is bounded.
    Suggested fix: Validate and cache each logical config once per report, cap ownership/context work, precompute root-to-context ownership, and pass the parsed config into path resolution.

Prior-Finding Closure Audit

Notes

@gltanaka

Copy link
Copy Markdown
Contributor Author

Implementer response to Codex adversarial review

@codex Re: #1996 (comment)

P1 #1: An unresolved live include falls back to the unsafe ancestor resolver

Status: FIXING in this PR
Plan: Carry an explicit validated-live-include result whose unresolved state fails closed, so report hashing and dependency extraction cannot enter _legacy_dependency_path() after validation begins.

P1 #2: Report hashing breaks legacy v1 alias compatibility

Status: FIXING in this PR
Plan: Carry declared-reference/validated-path pairs and apply v1's exact declared-string sort/dedup semantics before reading only the paired validated paths.

P1 #3: Nested architecture reports certify different example/test paths than live sync

Status: FIXING in this PR
Plan: Extract one pure complete architecture artifact resolver and use its code, example, test, and test_files result in both report and live resolution.

P2 #1: Metadata enumeration still materializes a wide directory before enforcing the budget

Status: FIXING in this PR
Plan: Replace Path.iterdir() with explicit os.scandir(), debit each yielded entry, and stop at the shared allowance without materializing the directory.

P2 #2: Config ownership work remains multiplicative outside the traversal budget

Status: FIXING in this PR
Plan: Add a report-scoped validated config/ownership cache, cap total context definitions, precompute root ownership once, and pass cached parsed config through classification/path resolution.

Pre-fix sibling investigation

  • The unsafe ancestor fallback is confined to the validated report-live branch in calculate_current_hashes(); stored-dependency and non-report legacy callers still intentionally use _legacy_dependency_path() and will remain separate.
  • The declaration/path identity mismatch also affects extract_include_deps() inputs, so the same paired validated result will feed both hashing and emitted dependency metadata.
  • Architecture-derived code/example/test/test_files logic is duplicated between _resolve_report_paths() and the architecture branch of get_pdd_file_paths(); both sites are in scope because partial anchoring cannot provide parity.
  • Prompt and repair traversal already use direct per-entry os.scandir(); metadata is the sole remaining Path.iterdir() budget implementation in the report discovery path and will adopt that established pattern.
  • Config reparsing occurs in both _prompt_ownership() and _configured_output_defaults(); context scanning also occurs once per prompt. Both are in scope under one report-scoped resolver state. Unrelated general CLI config loaders remain out of scope.

Prior-commit investigation

  • 4171cf5a1 established fail-closed unsafe legacy dependency handling; a2f9d5ac5 established pre-read validated live includes. This fix preserves their project-bound component validation while making validation state unambiguous.
  • 826bf31ee introduced discovered-context ownership and nested architecture anchoring, including the current duplicated ownership loads; this round will retain its ownership semantics while caching validated results.
  • d8e4c6565 and a2f9d5ac5 established bounded candidate generation and explicit os.scandir() for prompt/repair traversal; metadata will follow the same per-entry debit convention.
  • Existing architecture behavior in get_pdd_file_paths() anchors every derived artifact at the owning architecture.json directory. The shared pure result will preserve that convention rather than define a third resolution rule.

I will append one strict red test(sync) commit covering all five findings, push and post its failing output before any production edit, then append a separate fix(sync) commit and green evidence. No rebase, retarget, amend, force push, or work outside this worktree.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Round 14 strict TDD red evidence

Red commit pushed before production changes: 191ad2c09test(sync): reproduce round-14 liveness gaps.

Exact command:

PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 pytest -q tests/test_cloud_global_dry_run.py -k 'metadata_traversal_stops_scandir or nested_architecture_report_matches or unresolved_live_include or validated_v1_hash or report_caches_config'

Exact result: 5 failed, 40 deselected, 3 warnings in 0.78s.

  • Metadata: expected allowance + 1 entries through os.scandir; observed 0 because Path.iterdir/os.listdir remained in use.
  • Architecture: report returned top-level examples/cart_example.tsx; live returned packages/store/examples/cart_example.tsx (and corresponding test paths).
  • Unresolved include: prompt_hash was non-null and the ancestor fallback was read instead of failing closed.
  • v1 aliases: validated and legacy hashes differed for dep.txt plus ./dep.txt.
  • Ownership: one config with two prompts was parsed 5 times instead of once; the same test also requires a visible invalid_pddrc failure once the context cap is exceeded.

The red commit is the live PR head. Production implementation starts only after this comment.

@gltanaka

Copy link
Copy Markdown
Contributor Author

Round 14 implementation evidence

Implemented and pushed at 77df2c8cc56fa8255956206befb5b420865ecd70 in response to #1996 (comment).

Strict append-only TDD

  • Red: 191ad2c09test(sync): reproduce round-14 liveness gaps
  • Green: 77df2c8ccfix(sync): close round-14 liveness gaps

The red commit was pushed and its five-failure output posted at #1996 (comment) before any production edit.

Focused green command:

PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 pytest -q tests/test_cloud_global_dry_run.py -k 'metadata_traversal_stops_scandir or nested_architecture_report_matches or unresolved_live_include or validated_v1_hash or report_caches_config'
5 passed, 40 deselected, 4 warnings in 0.49s

Resolved findings

  • Validated-but-unresolved live includes now have an explicit empty validated result, return a null prompt hash, emit no dependency map, and never re-enter ancestor legacy resolution.
  • Validated live includes carry declared-reference/path pairs. v1 applies exact declared-string sorting/deduplication before reading only validated paths, preserving alias multiplicity.
  • One shared non-mutating architecture artifact resolver supplies code, example, test, and test_files to report and live callers, anchored at the owning nested architecture root.
  • Metadata enumeration uses direct os.scandir, consumes allowance + 1 entries, and never invokes Python 3.12 Path.iterdir/os.listdir materialization.
  • Parsed validated configs are cached on discovered units, reused during classification, and context definitions are capped at 1,000 with visible invalid_pddrc failure.

Green verification

Full relevant suite, including subprocess CLI E2E:

PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 pytest -q tests/e2e/test_issue_1932_continuous_sync_guarantee.py tests/test_cloud_global_dry_run.py tests/test_continuous_sync_path_policy.py tests/test_sync_determine_operation.py
271 passed, 26 warnings in 259.58s (0:04:19)

Static/compile/diff:

pylint --errors-only pdd/continuous_sync.py pdd/sync_determine_operation.py tests/test_cloud_global_dry_run.py tests/e2e/test_issue_1932_continuous_sync_guarantee.py
(exit 0)

ruff check --select E9,F63,F7,F82 pdd/continuous_sync.py pdd/sync_determine_operation.py tests/test_cloud_global_dry_run.py
All checks passed!

python -m compileall -q pdd tests/test_cloud_global_dry_run.py tests/e2e/test_issue_1932_continuous_sync_guarantee.py
(exit 0)

git diff --check
(exit 0)

Real CLI report E2E used a disposable project containing prompts/widget_python.prompt:

PYTHONPATH=<worktree> PDD_AUTO_UPDATE=false PDD_FORCE_LOCAL=1 python -m pdd --no-core-dump sync --dry-run --json

Result: exit 0; one UNBASELINED widget; 0 failures; project-relative code, example, test, and test_files paths.

Final state

  • Local HEAD, origin branch, and live PR head: 77df2c8cc56fa8255956206befb5b420865ecd70
  • Live PR base: ed0a56d04b4830edc70e9d5c1743db2d6609efd1
  • Branch is clean and has no unpushed commits.
  • No rebase, retarget, amend, force push, or sibling-worktree operation was performed.

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.

1 participant