Campaign bundle: PDD end-to-end staging fixes#1998
Open
Serhan-Asad wants to merge 88 commits into
Open
Conversation
…ace contract (#1900) The public-surface gate compared OLD-vs-NEW generated code, so it could not distinguish an intended interface change from generator drift, and its repair directive had no stable target ("restore compatible signatures" — compatible with the very code being regenerated). This dead-ended `pdd change -> pdd sync` whenever a module's declared interface legitimately evolved (live case pdd_cloud#2971: an added symbol plus unrelated `list_secret_metadata` drift). Make `_verify_public_surface_regression` declaration-aware, per-symbol: - Symbols declared in the prompt's <pdd-interface> (`type: module`) are validated against the DECLARED signature (a stable target) via `signature_entries_compatible`, with defaults resolved in the generated namespace (#1558). Binding-kind/async — which <pdd-interface> cannot express — stay anchored to the prior generation so async<->sync / function<->class drift is still caught; `BREAKING-CHANGE: change signature` remains the escape hatch for those. - Undeclared symbols keep the old-code baseline (+ `BREAKING-CHANGE`), so backward-compat protection for helpers/re-exports is unchanged (no regression). - `PublicSurfaceRegressionError` carries structured `signature_detail:` lines (full expected-vs-actual, no truncation); the agentic subprocess parser recovers them into the rebuilt repair directive and hard-failure block. `removed:` / `signature_changed:` lines stay byte-identical (cloud parser + tests depend on them). Scoped to `type: module` function declarations; cli/command names and dotted `Class.__init__`/new-class presence stay owned by the conformance gate. Adds tests/test_issue_1900_surface_contract.py (18 gate-level tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…enerator_main PDD-Auto-Heal-Checkpoint: success
… / parser hardening Follow-up to the #1900 public-surface declaration contract (the preceding auto-heal bot commit for examples/architecture/metadata is left untouched): - Declared dotted names (`Class.method`, `Class.__init__`) are presence-only in the surface gate: `_snapshot_public_signatures` receiver-strips `self`/`cls`, so comparing a declared method signature that includes `self` false-positived on UNCHANGED code. Their parameter contract stays with the `<pdd-interface>` signature gate; top-level functions keep full declared-signature validation. - `BREAKING-CHANGE: change signature <sym>` now relaxes ONLY the un-declarable binding-kind/async for a declared symbol, never its declared params/return — an added-required-param that violates the declaration is no longer bypassable by prose. - `_parse_signature_detail_lines` tolerates ` | ` inside a signature/default (dropped the ordered-index precheck; right-anchored rsplit + try/except only), so union types / delimiter-like defaults keep the stable declared repair target. - Sync README + the `code_generator_main` generator prompt (5b) to the declaration-aware behavior (incl. `PublicSurfaceRegressionError.signature_details`). Adds 4 regression tests (22 total in tests/test_issue_1900_surface_contract.py). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…code baseline (#1900) Review follow-up: making dotted-method / non-paren-class declared symbols presence-only ALSO removed them from the old-code baseline, dropping the signature protection they had before #1900 — a declared method could silently gain a required arg or flip binding kind, and a declared class's constructor ABI could drift. Only symbols actually validated against the declared signature (top-level names with a parseable paren signature) are now recorded in `declared_validated` and excluded from the old-code baseline; every other declared symbol (dotted methods, `class Service`-style non-paren declarations) falls back to the previous-generation old-vs-new comparison, which catches added-required-params, binding-kind flips, and constructor-ABI drift, and still honors BREAKING-CHANGE + #1558. The receiver-strip false positive stays fixed (the old-code path compares receiver-stripped snapshot entries on both sides). README + generator prompt (5b) wording synced to match. Adds 4 regression tests (method +required, staticmethod->instance flip, class ctor drift, BREAKING-CHANGE opt-out) and strengthens the presence-only test that had encoded the over-skip. 26 tests in tests/test_issue_1900_surface_contract.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…face too (#1900) Review follow-up: dotted methods and constructors were presence-only with an old-code fallback, which left NEW methods/ctors unchecked (an added required arg passed) and kept the `pdd change → sync` dead-end for method contract changes. Make them first-class declared-contract citizens, exactly like top-level functions. `_declared_signature_to_entry` gains `strip_receiver`: a declared method/ctor signature is receiver-stripped (leading `self`/`cls` dropped) to match `_snapshot_public_signatures`; `Class.__init__` maps to the class `[class]` entry (constructor ABI); binding-kind/async come from the pre-generation entry when present (else the generated one) so a `@staticmethod`→instance flip is still caught; and `BREAKING-CHANGE: change signature` relaxes only those un-declarable facets, never the declared params. Only symbols actually compared against the declaration join `declared_validated` (keyed on the snapshot key) and are excluded from the old-code baseline; description-only / non-paren declarations stay presence-only and fall back to old-vs-new so no pre-#1900 protection is lost. Net: a new declared method/ctor with an added required arg now raises; editing a declared method contract is authorized without a `BREAKING-CHANGE` permit; the declared-`self` false positive stays fixed. README + generator prompt (5b) synced. Adds 3 tests and reworks 2 (29 in tests/test_issue_1900_surface_contract.py). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The auto-heal bot commit (3632d6a) regenerated context/agentic_sync_runner_example.py in a way that dropped the top-level AsyncSyncRunner shim (breaking other prompts' <include select="class:AsyncSyncRunner">), un-mocked _sync_one_module (so the example spawned real pdd sync subprocesses), and left project_dependencies.csv stale with trailing whitespace. The #1900 change is internal (surface gate + detail parser) and does not alter the public interfaces these examples demonstrate, so restore main's known-good generated artifacts. PR diff is now purely the #1900 source/tests/docs.
…allable, fix repair advice (#1900) Review follow-up (three real gaps in the declared-interface surface contract): - Declared `_`-prefixed helpers (e.g. agentic_common's `_extract_step_report`) were false-positived as removed: `declared_missing` checked presence against the public surface, which excludes underscore names. The declaration is authoritative (like `__all__`), so union the declared symbols actually defined in the code into the `patch_targets` fed to `_snapshot_public_surface` / `_snapshot_public_signatures` for both before and after — a present declared `_helper` is no longer flagged, a genuinely removed one still is, and its signature is now validated too. - A declared callable regenerated as a non-callable (`def f` → `f = 1` / `from pkg import f as f`) passed: `signature_entries_compatible` returns None and the symbol was still marked validated, so the old-code baseline was skipped. Now only add to `declared_validated` when the comparison is conclusive (`compat is not None`); a None result falls through to the old-code baseline, which flags the `[function]`→`[assignment]`/`[import]` break (and does not false-positive on an unchanged callable-assignment). - The hard-failure / repair advice told users to add `BREAKING-CHANGE`, which for a declared symbol only relaxes binding-kind/async, never params — a dead-end. Advice is now declaration-aware: a declared-param violation points at editing the `<pdd-interface>` declaration; the `BREAKING-CHANGE` guidance stays for undeclared/removed symbols. Adds 7 tests (36 in tests/test_issue_1900_surface_contract.py); the generator prompt's `<include>` now lists the new helpers; README deduped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ines, underscore-class ctor (#1900) Review follow-up (three edge gaps in the declared-interface surface contract): - A declared callable regenerated as a non-callable (`def f` → `f = 1` / `from pkg import f as f`) slipped through when the prompt also carried `BREAKING-CHANGE: change signature f` — the old-code fallback skipped it via `allowed_signature_changes`. Now, when the declared comparison is inconclusive (`compat is None`) and the OLD entry was a callable contract, flag the declared-callable-became-non-callable DIRECTLY, regardless of `BREAKING-CHANGE` (which authorizes parameter changes, not de-callable-ing a declared callable — use `BREAKING-CHANGE: remove` for that). An unchanged callable-assignment (`f = lambda: x`) still passes. - `signature_detail:` lines are now JSON-encoded (`json.dumps` of `{symbol, expected, actual, source}`) at both emit sites and parsed via `json.loads`, ending the recurring field corruption when a signature default contains the ` | actual: ` / ` | source: ` delimiter substring. - Declared underscore-CLASS constructor drift (`_Service.__init__`) was missed because the patch-target signature capture only built entries for top-level functions. Extracted `_class_constructor_signature` (shared with `_walk_class`) and used it so a patch-target class gets its `[class]` constructor-ABI entry; a declared underscore class / constructor is now validated like a public one. Adds 5 tests (41 in tests/test_issue_1900_surface_contract.py); the generator prompt spec notes the JSON detail-line format. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sses; sync self-hosting metadata (#1900) Review follow-up: - Declared NESTED constructors whose class path contains an underscore behaved inconsistently — `_Outer.Inner.__init__` passed silently, `Outer._Inner.__init__` was mischaracterized as `removed`. They now behave like the public control: `_symbol_exists_in_module` resolves nested class paths (not just methods), `_resolve_class_node` walks dotted segments to a `ClassDef`, and the patch-target signature capture emits the `[class]` constructor-ABI entry for a nested declared class (keyed `Outer.Inner`) — so ctor drift raises a `changed` signature with a `pdd-interface` detail. Surgical: only DECLARED/patch-target nested classes are captured; the public `_walk_class` recursion (undeclared underscore-nested) is untouched, so no protection is broadened. `test_split_validation.py` confirms the `_symbol_exists_in_module` generalization didn't regress sibling-test patch resolution. - Self-hosting metadata synced to the #1900 behavior so future self-regeneration keeps it: the `agentic_sync_runner` prompt documents the JSON `signature_detail:` parsing + declaration-aware repair; the `code_generator_main` prompt `<include>` lists the new helpers (`_class_constructor_signature`, `_resolve_class_node`, `_symbol_exists_in_module`, `_patch_target_signature_entry`); and architecture.json's `code_generator_main` + `agentic_sync_runner` descriptions carry the declared-contract / JSON-detail behavior. Adds 4 tests (45 in tests/test_issue_1900_surface_contract.py). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cal test path (#1903) PDD derived a module's test-output path purely from `.pddrc`/defaults (`tests/test_{name}{ext}`), blind to the project's real test runner. On a jest/Next.js project it maintained a root `tests/` shadow while the co-located `__test__/*.test.tsx` the runner collects went stale — `generate`/`change`/`sync` reported "tests pass" (PDD ran its own shadow) while CI failed. A `.tsx` module also derived a `.ts` test extension. PDD now adopts a single, unambiguous, existing co-located test as its canonical test path so generate/change/sync target the real test CI runs: - content_selector: pure `find_collocated_test` (Python siblings + JS/TS jest/vitest/Next.js conventions; single match else None; suffix-gated; never raises), the `resolve_test_output_path` adopter, and presence-based explicit-vs-default provenance (`_pins_test_output_location` / `configured_test_output_pinned`). - sync_determine_operation: `get_pdd_file_paths` wraps the raw derivation and adopts across every return branch, threading a single authoritative explicit-vs-default signal read from the raw `.pddrc` context (never the `resolved_config` construct_paths pollutes). - cmd_test_main: adopts for direct `pdd test`, keeps the native/cloud generation destination in sync with the adopted write target, and pins sync's merge-into-existing write to the real test (no numbered `page.test_1.tsx` shadow). - code_generator_main: `_is_test_output_path` recognizes adopted `.test/.spec` `.mjs/.cjs` and the singular `__test__` dir, so adopted human tests stay behind the TestChurnError guard. Explicit user pins (CLI `--output`, `PDD_TEST_OUTPUT_PATH`, explicit `.pddrc test_output_path`/`outputs.test.path`) are always honored; the standard Python `src/foo.py -> tests/test_foo.py` flow is unchanged. Regression tests drive the real `construct_paths`/`architecture.json`. Source-of-truth: the four owning `prompts/*_python.prompt` files (content_selector, sync_determine_operation, cmd_test_main, code_generator_main) are updated to describe this behavior so PDD's regeneration / drift-heal reproduces it (in this repo the `.py` is generated from the prompt). Adoption is applied consistently across the write paths AND the `pdd test --evidence` manifest resolver, so evidence/replay audits point at the file PDD actually writes, not the shadow. Part of #1903 (scoped: adopts an existing co-located test; greenfield runner-config detection and never-block-on-churn are tracked as follow-ups #1916 / #1917). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on (#1926) Introduces a single `FingerprintTransaction` context manager that enforces the 'artifact write ⇒ fingerprint write' invariant across all mutating PDD commands (sync, generate, update, auto-deps, fix, CI heal). Replaces scattered `save_fingerprint` call sites with one commit-or-fail code path featuring null-hash guards, atomic temp-file+rename persistence, and `FingerprintFinalizeError` propagation to Click (non-zero exit on failure). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-fresh (#1938) pdd sync regenerated mature modules "rebirth-shaped" (full regeneration), dropping declared public symbols. PDD already ships edit-shaped generation (incremental_code_generator + code_patcher_LLM), but sync hardcoded force_incremental_flag=False, so it deferred to the deliberately-conservative diff_analyzer_LLM ("prefer complete regeneration"), which rebirthed modules on ordinary small prompt deltas and lost symbols. Make sync prefer surgical generation for mature modules, with a --fresh opt-out: - New `pdd sync --fresh` flag (default off), threaded sync -> sync_main -> sync_orchestration and the one-session generate loop. - Both sync generate call sites now pass force_incremental_flag=(not fresh): default -> surgical/edit-shaped (existing code + prompt delta -> minimal edit, declared symbols preserved); --fresh -> standard generation. - Reuses existing infra only. Safety nets preserved: new/empty modules still full-generate; undeterminable original prompt falls back to full generation; conformance repair retries still force full regeneration (#1724); the public-surface / declared-interface gate (#1900) is unchanged and remains the guarantee. - --fresh is single-module only: raises UsageError on global/agentic sync paths (mirrors --snapshot-context) instead of silently no-op'ing. Prompt sources edited in tandem with generated .py (pdd is self-hosted); sync_main's architecture.json signature aligned; README documents the flag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ily is down (#1941) The review loop dead-locked instead of degrading when only one AI provider family was alive. With `--reviewer codex --fixer claude --fallback-reviewer-on-failure`, a codex hard-failure promoted claude (the fixer's own role) to fallback-review the PR; when claude reported real findings the loop hit `break` with "secondary reviewer ... reported findings (fallback)" and never ran a fix round — even though a fresh same-family claude session could execute the concrete findings and be re-reviewed. Fix: in the fallback-reviewer "findings" branch of `run_checkup_review_loop`, instead of terminating, hand the actionable findings to the fixer on the next iteration (mirrors the existing fallback-clean gate-findings handoff just above), reassign the active reviewer to the fallback role, and let the normal fix + fresh-verify path run. Disclose the weaker guarantee honestly: - new `ReviewLoopState.role_independence` (`"independent"` | `"degraded (<role> unavailable)"`), rendered as a `role-independence:` report line and a `role_independence` key in final-state.json; - `same_role_review_fix` is stamped true for the degraded run; - the fix-failure stop reason names the vacancy explicitly instead of a bare "could not address". The superseded primary already renders `(optional, superseded by <fallback>)`, so the pdd_cloud verdict adapter drops it from the required-reviewer set and ships on the degraded-but-clean result — no cloud change needed. Behavior is unchanged when both families are alive. Adds regression tests: the deadlock→ship path (asserts the review-loop Machine Verdict passes + final-state disclosure), both-families-alive stays independent, and degraded-fixer-also-fails names the vacancy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review of #1942: the code change added runtime same-role degradation and a new `role_independence` state/report/final-state field, but the prompt-driven source-of-truth surfaces still described the pre-#1941 contract. Left unreconciled, `pdd sync` / the auto-heal bot would regenerate the code from the stale prompt and drop the fix. Reconcile all four surfaces with the implemented behavior: - pdd/prompts/checkup_review_loop_python.prompt: specify the auto-degrade rule under `fallback_reviewer_on_failure` (fallback-reviewer findings → fresh same-family fix + required verify instead of dead-end), the `role-independence:` report header line, the `role_independence` final-state.json key + revised `same_role_review_fix` semantics (config-time OR runtime), the appended-field ordering, and the degraded fix-failure stop-reason suffix. - architecture.json: add the #1941 auto-degrade sideEffect + `role_independence` to the final-state.json field list for run_checkup_review_loop. - README.md: document the auto-degrade behavior + disclosure under `--fallback-reviewer-on-failure`. - context/checkup_review_loop_example.py: add `role_independence` to the ReviewLoopState example, the final-state payload, the report-header comment, and the rendered-report example. No code/behavior change; docs-only reconciliation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gnostics (#1941) Review hardening for #1942: the cloud verdict adapter reads a `role-independence:` header line to route a degraded run to `ship_degraded`, but untrusted reviewer stderr rendered in `### Reviewer Diagnostics` could echo `role-independence: independent` and — since the adapter previously took the last match in the whole report — override the authoritative header, silently downgrading a degraded disclosure back to a plain ship (or the reverse). Add `_defang_role_independence_inline` to `_defang_adapter_trip_wires` so any `role-independence:`/`role_independence=` marker leaking from reviewer stderr is neutralized at the render boundary, exactly like the existing `reviewer-status:` / `fresh-final-review:` / `max-*-reached:` markers. The on-disk `final-state.json` and `reviewer_status_details` keep the original text. Prompt spec (27b) updated to list the new marker. Companion adapter-side defense (header-scoped parse) is in promptdriven/pdd_cloud#3126. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#1957) The checkup source-of-truth guards reconstructed each module's owning prompt path by string-joining a hardcoded `pdd/prompts/` prefix onto the `filename` stored in `architecture.json`. That prefix only ever resolved on pdd's own self-hosted checkout (prompts live under `pdd/prompts`, with a `prompts -> pdd/prompts` symlink). On every OTHER repo the guard probed a nonexistent path, classified the owning prompt as `missing_prompt`, and refused to repair — blocking the bug→fix→checkup chain on all external repos (observed on staging E2E against promptdriven/test_repo #4234/#4235). `_load_prompt_source_map` and `_extract_arch_pairs` now resolve the base prompts root via `_resolve_target_prompts_root(worktree)`, which reuses the same `.pddrc` discovery/parse helpers generate/sync use (`construct_paths._find_pddrc_file` / `_load_pddrc_config`) and falls back to `prompts` then `pdd/prompts`, picking the first existing directory and resolving symlinks to the real git-tracked path. A prompt is only reported deleted when it is missing from the target repo's resolved root. Owning prompt `pdd/prompts/checkup_review_loop_python.prompt` updated to match (10a/10b mapping rules + new resolver spec). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Agentic provider selection already fell through to the next provider on a
permanent error *within a single run_agentic_task call*, but each call
rebuilt its candidate list from scratch. A permanently-dead provider (e.g.
Codex whose staged ChatGPT auth is present but expired) was therefore
re-attempted on every step of a multi-step workflow, and a call whose only
feasible candidate was the dead provider had nowhere to fall through to —
so cloud one-session sync died on codex's 401 despite healthy anthropic/
google credentials in the same run.
Add a run-scoped permanent-failure registry in the single provider-selection
source of truth (run_agentic_task):
- mark_provider_permanently_failed(provider, classification) records a
provider when _classify_permanent_error fires; get_disabled_providers()
exposes the {provider: classification} map; reset_disabled_providers()
clears it. The registry is mirrored into the PDD_AGENTIC_DISABLED_PROVIDERS
env var so re-entrant `pdd` subprocesses inherit the same skip-list.
- run_agentic_task drops disabled providers from candidates AFTER the
PDD_AGENTIC_PROVIDER hard filter and BEFORE the single_provider_attempt
truncation, so a known-dead first provider no longer consumes the single
attempt and later steps skip it instead of re-paying its timeout.
- When every feasible provider was already disabled this run, fail fast with
an aggregated "All agent providers failed permanently earlier this run:
<provider>: <class>; ..." message instead of re-burning or the generic
"no providers available" line.
The registry is subordinate to PDD_AGENTIC_PROVIDER: it only ever removes a
provider that proved dead, never adds one, so a staged-auth hint cannot
re-pin a provider once its auth is known bad. No behavior change when the
first provider works.
Updates the owning prompt (agentic_common_python.prompt) with the new
exports and the registry requirement, and adds regression tests plus
conftest isolation for the new env var / registry.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rough Run-scoped provider permanent-failure fall-through (#1936). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prompt-declared <pdd-interface> as the public-surface contract (#1900). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Surgical (edit-shaped) regeneration for mature modules + --fresh (#1938). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) PR#1940 made `pdd sync` prefer surgical (edit-shaped) regeneration for mature modules, which needs an "original" prompt to diff the current prompt against. In the cloud sync-after-change flow the prompt edit is already committed on the branch (on-disk == HEAD) and the pre-change version is not reliably found by the local history search, so incremental resolution silently degraded to full regeneration and re-dropped declared symbols (live failure: test_repo#4232, 5 consecutive surface-gate blocks). Add a last-resort fallback in code_generator_main's original-prompt resolution: when no explicit original, no uncommitted-vs-HEAD delta, and no distinct prior version in local history resolve an original, read it from the base branch via the existing get_git_content_at_ref() helper. Base ref is configurable via PDD_SYNC_BASE_REF (default origin/main) and is only used when it differs from the current prompt. This replaces the prior degenerate fallback that set the "original" to the current HEAD content (identical prompts), which produced a no-op incremental patch that was then discarded for full regeneration anyway. When the base ref also yields nothing usable, can_attempt_incremental now stays False and an honest full regeneration runs, surfaced by a single unconditional (non-verbose) degradation notice so operators are not blind to mature-module full regens. Tests cover: base-ref resolves a distinct original -> incremental used; base-ref missing (None) -> full generation + loud notice, no crash; base-ref identical to current -> not used; PDD_SYNC_BASE_REF override honored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-resilience Auto-degrade to same-family fixer when one provider family is down (#1941). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…esilience Resolve owning-prompt paths against the repo-under-repair layout, not pdd's self-hosted pdd/prompts (#1957). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p-resilience Adopt the runner-collected co-located test as PDD's canonical test path (#1903). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… CodeQL) CodeQL flagged 4 py/path-injection alerts (#339-#342) on the new adoption code: candidates derived from a caller-supplied code-file path — issue- influenced in the hosted/agentic flow — flowed into resolve()/write-target selection without containment, so a traversal-shaped input could adopt a test path outside the repo. find_collocated_test now rejects candidates that resolve outside cwd, and both adoption sinks (resolve_test_output_path, _adopt_collocated_test) re-assert containment before retargeting. Traversal inputs degrade to no-adoption (derived path unchanged). Adds 4 containment regression tests; 5 existing tests updated to run with cwd=the project like the rest of the suite (matches real CLI invocation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Second review — reviewed commit: IntentThis latest revision preserves the campaign bundle's end-to-end PDD fixes while closing the first review's lifecycle gaps: revalidate generated mock contracts after CI/pre-checkup agents edit the worktree, isolate permanent-provider failures to a logical workflow while still propagating them to child processes, and record fail-closed fingerprint-finalization errors as failed operations rather than successful audit entries. Failure modes
PerformanceI rescanned the current full diff's provider selection/scope path, TUI worker boundary, remediation loops, mock validator, and metadata logging/finalization. The ContextVar implementation adds no workflow-long lock or GIL-heavy work. The new pre-commit checks can perform O(repository-files) schema/source scans once per CI or pre-checkup repair attempt (plus the existing final check), but they remain terminal remediation work rather than request-handler/event-loop hot paths; no new network/subprocess loop was introduced beyond the existing CI retries. I found no separate release-blocking performance regression, though large-monorepo scan time should remain monitored. Nits
Verdict: changes requestedThe three original direct-path defects are fixed, but the boundary cases above remain actionable. Verification in the repository's
|
Collaborator
Author
|
Addressed the second review pass in
Validation:
|
Collaborator
Author
|
Follow-up CI remediation landed in The instruction is now in the prompt body, the canonical fingerprint is refreshed, |
Collaborator
Author
|
Third review — reviewed commit: IntentThis revision keeps the full campaign bundle while completing the prior lifecycle repairs: make CI/pre-checkup validation and commit operate on one workflow-owned file allowlist, preserve provider-failure state across direct orchestrator and Textual worker boundaries without leaking it into independent workflows, and filter known-dead providers before a routing policy pins its preferred harness so healthy providers remain usable. Failure modes
PerformanceI scanned the revised allowlist/commit loop, provider scope/decorator path, Textual worker boundary, routing selection, mock-contract scans, and metadata finalization. The provider changes use ContextVars without workflow-long locks; the allowlist adds only O(number-of-dirty-files) set/filter work around existing Git subprocesses. Mock-contract validation remains O(repository files) and can run per remediation attempt, but it is terminal workflow work rather than an event-loop/request hot path. No separate release-blocking performance regression was found. NitsNone beyond the behavioral findings above. Verdict: changes requestedThe previous three findings are fixed on their cited paths, but the adjacent publisher/scope/logging cases above remain actionable. Verification against
|
Collaborator
Author
|
Addressed the third review pass in
Validation:
|
Collaborator
Author
|
Fourth review — reviewed commit: IntentThis current head completes the campaign bundle and closes the remaining audit gaps: keep workflow commits scoped away from unrelated staged entries, give direct one-session sync one provider-health epoch across repair attempts, emit routing telemetry even when all feasible providers are already disabled, and restore the canonical Failure modesI found no actionable findings. I explicitly exercised exactly these three concrete production failure modes from the prior pass:
PerformanceI scanned the final commit/publish path, one-session retry loop, provider ContextVar scopes, routing selection/filtering, mock-contract validation, metadata finalization, and prompt/include validation. The new commit scoping changes only the existing Git argv; the one-session decorator adds constant-time ContextVar entry/exit; routing still filters the bounded provider list once. No new unbounded collection loop, network/disk I/O in a hot callback, event-loop blocking, or lock contention was introduced. The pre-existing mock-contract repository scan remains terminal remediation work rather than a request/event-loop hot path. NitsNone. Verdict: readyThe three previously reported findings are fully resolved, the metadata-header fix is consistent with the prompt contract, and I found no new correctness, security, concurrency, API, logging, data-loss, or performance regression. Verification at
|
…ed-test churn Two gaps in the issue #1903 implementation surfaced in review of PR #1998: 1. Greenfield (§A) was unimplemented: with no existing co-located test, PDD kept its runner-blind `tests/` shadow. Add `find_runner_collected_test_path` (content_selector): when the project configures a jest/vitest runner (config file or package.json declaration, walking up to the repo root) and the module is JS/TS, write the FIRST test to the runner-collected `{module_dir}/__test__/{stem}.test.{ext}` instead of a `tests/` orphan. Wired into `resolve_test_output_path`'s no-sibling branch, so `pdd generate`/`change`/`sync` all target the collected path. Python keeps its pytest-idiomatic `tests/` default; user pins are still honored; total/ never-raising with CWE-022 containment reused. 2. Never-block (§B.4) was not honored: an unreconcilable (coverage-losing) adopted human test hard-failed the issue-driven workflow. In the agentic sync runner, on test-churn exhaustion (the child already restored the human test and one-session's #2208 coverage-preserving auto-accept already refused) keep the original test, emit `PDD_TEST_CHURN_NEEDS_REVIEW`, set `ModuleState.needs_review`, and report the module synced so the PR opens with the test flagged "needs review" in the progress comment / PR body. Scoped to the issue-driven runner only — standalone `pdd test`/`pdd sync <module>` keep the strict hard-fail (the safe default outside a PR flow). Prompts, fingerprints, README (§1122), and regression tests updated together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The #1941 same-family auto-degrade only ran inside the opt-in `_maybe_run_fallback_reviewer` else-branch, so it never fired for the accepted/live config (reviewer=codex, fixer=codex, reviewer_fallback=claude, codex family down). There the EXPLICIT reviewer_fallback branch promotes claude to reviewer but falls straight through to the fix round, which then targets the dead configured fixer (codex) and re-deadlocks with "findings remain". Apply the degradation after the explicit reviewer_fallback promotion as well: when the configured fixer IS the just-failed primary's identity (its family is the one that went down) and the fallback review carries actionable findings, promote the surviving fallback family to a fresh same-family fixer (`state.active_fixer`), stamp `same_role_review_fix=true` and `role_independence="degraded (<primary> unavailable)"`, and let the normal fix + required fresh-verify run. Automatic for every consumer — no `--fallback-reviewer-on-failure` opt-in. Trigger is narrow (fixer == the demonstrably-failed identity) so an independent cross-family fixer is still preferred and never bypassed. Adds a regression for the exact acceptance config the prior #1941 test missed (it used fixer=claude, which already reached the else-branch). Prompt and fingerprint updated alongside the generated module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Addressed the three P1 review findings on Finding 1 — greenfield runner-blind tests (#1903 §A): added Finding 2 — unreconcilable adopted test blocked the workflow (#1903 §B.4): scoped to the agentic issue-driven runner (per maintainer decision — standalone Finding 3 — #1941 degrade attached to the wrong path: the same-family auto-degrade now also fires after the explicit Each fix updates prompt + generated module + fingerprint + tests together; fingerprints verified self-consistent with Note: the previously-flagged red CI ( |
…ft-resolution Fix nested checkup drift input resolution
…lock Address three follow-up review refinements on the #1903 fixes: 1. Honor custom jest/vitest config (§A). `find_runner_collected_test_path` no longer hardcodes `__test__/{stem}.test.{ext}`. It reads JSON-readable config (package.json `jest`/`vitest`, `jest.config.json`, `.jestrc`) and honors `testMatch`/`testRegex` (choose `.test`/`.spec` + `__test__`/`__tests__`), `roots`/`rootDir` (containment), and `testPathIgnorePatterns` (exclusion) via a small micromatch→regex subset. If NO co-located candidate would be collected (centralized layout, roots excluding the module), it falls back to the derived path — never emits an uncollected test. JS-only configs (unparseable) keep the jest-default convention, which the default testMatch collects. 2. Scope the never-block to ADOPTED co-located tests (§B.4). The test-churn never-block now fires only when the churned test is a co-located human test (`_is_adopted_collocated_test_path`: `.test.`/`.spec.` or under `__test__`/`__tests__`). A PDD-owned `tests/test_*.py` shadow keeps the strict hard-fail even in the issue-driven runner, so coverage loss on a PDD-owned test is never silently swallowed. 3. Persist `ModuleState.needs_review` in `_save_state`/`_load_state` so a durable resume does not drop the flag. Prompts, fingerprints, README (§1108/§1122), and regressions updated (glob engine + resolver validated against real jest configs; PDD-shadow-keeps-hardfail and resume-persists-flag cases added). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Re: the last review pass — the head had advanced to 1. Custom jest/vitest rootDir/roots/testMatch (§A). 2. Never-block scoped to adopted tests (§B.4). The relief now fires only when the churned test is a co-located human test ( 3. Prompts, fingerprints (self-consistent with |
…riven gate Address the round-3 review of the greenfield/never-block fixes: 1. Centralized layouts no longer false-green. When co-location is not collected (a `roots`/`rootDir` or `testMatch` fixed-directory layout), the resolver now DERIVES a collected path *under* the configured directory instead of falling back to the runner-blind `tests/` shadow. New tests assert the derived path satisfies the configured testMatch/roots. 2. Custom JS/TS config files are respected. A `jest.config.js` is unparseable in Python, so its text is inspected: if it references a discovery key (testMatch/roots/rootDir/testRegex/testPathIgnorePatterns) the resolver conservatively REFUSES to claim a path is collected (falls back to derived) rather than assuming jest defaults; a plain JS config with no discovery keys keeps the default convention (which jest collects). 3. Never-block gated to the issue-driven workflow. The relief now requires `self.issue_url` (a real GitHub issue -> PR sync) in addition to the adopted co-located classifier. Project-wide `pdd sync` (issue_url=None, no PR) keeps the strict test-churn hard-fail, closing the silent-bypass path. New tests cover the global-sync-hard-fail case; existing never-block/shadow tests now chdir to isolate the resume-state file. Prompts, README (§1108/§1122), architecture.json (content_selector + agentic_sync_runner descriptions), and fingerprints updated together. Micromatch subset + resolver validated against real jest configs (default, centralized, roots include/exclude, spec-only, opaque JS custom vs plain). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Round-3 review addressed in 1. Centralized layouts no longer false-green (P1-1). When co-location isn't collected (a 2. Custom JS/TS configs respected (P1-2). A 3. Never-block gated to the issue-driven workflow (P1-3). Confirmed: On the provenance sub-point (infer ownership from filename): with the Also updated: both prompts, README §1108/§1122, and the stale |
…rovenance, prompt altitude Address gpt-5.6-sol (xhigh) review findings on the #1903/#1941 changes: #1903 correctness (content_selector greenfield): - Centralized layouts no longer collapse same-stem modules onto one path: derive the centralized candidate under the anchor MIRRORING the module's relative dir (`_module_rel_dir`), so `src/a/util.ts` and `src/b/util.ts` get distinct collected tests — never fork/overwrite. - Cover the vitest dialect and jest negation: the opaque-JS discovery scan now matches jest AND vitest keys (whole-word, so `__dirname` doesn't trip); `_candidate_is_collected` applies jest's ordered include/exclude semantics (a leading-`!` glob removes matches); parseable configs using `projects`/ `include`/`exclude` we can't resolve are refused rather than false-greened. #1903 provenance (agentic_sync_runner never-block classifier): - `_is_adopted_collocated_test_path` now rejects absolute paths, `..` traversal (CWE-022), and PDD-owned `tests/` shadows (JS `tests/foo.test.ts` as well as `tests/test_*.py`) — closing the over-acceptance the `.test.`/`.spec.` substring heuristic allowed. It remains a NECESSARY (not sufficient) gate composed with the issue_url guard + coverage-preserving auto-accept. #1941 prompting-guide altitude: - Rewrite the reviewer_fallback auto-degrade spec at behavior level per docs/prompting_guide.md (Concision Rules 3/8, Back-Propagation Altitude): drop private helper names, exact state assignments, and internal step ordering; keep the observable trigger, outcome, disclosure, and failure reason. Prompts, README §1108, fingerprints, and regressions updated in lockstep (centralized-collision, vitest-custom/plain, jest-negation, jest-projects, classifier traversal/absolute/JS-shadow). Local suites: 691 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ge cases, prompt altitude Address the 8 gpt-5.6-sol (xhigh) findings on the #1903 changes: Never-block classifier (agentic_sync_runner): - CRITICAL: production churn `output:` paths are ABSOLUTE (construct_paths); the round-4 classifier rejected all absolutes and broke the real never-block. Now accept an absolute path only after canonical containment in the worktree root (passed from `_sync_one_module`), reduce to repo-relative, then apply the traversal/shadow/co-location checks. Regressions for absolute in-root (never- block) and absolute out-of-root (hard-fail). Greenfield discovery (content_selector) — eliminate false-greens: - Delegating/composed JS configs (`require`/`import`/spread/`preset`/`extends`/ function) are unresolvable -> refuse (a plain literal is required to assume defaults). - Character-class negation `[!x]` now translates to `[^x]` (was "! or x"). - `testMatch` is evaluated with jest's true SEQUENTIAL order (a later positive re-includes after an earlier negation; last match wins). - rootDir-only centralized layouts derive a path UNDER rootDir (previously fell through to the runner-blind shadow). - Default discovery only co-locates for js/jsx/ts/tsx (jest ignores mjs/cjs). Prompt altitude (#1903 §A, content_selector prompt): - Rewrite the greenfield spec at behavior/contract level per docs/prompting_guide.md (Concision Rules 3/8): drop private-helper recipes, glob/regex mechanics, and internal ordering; keep observable invariants (never emit an uncollected path, refuse when unresolvable, honor collection rules, centralized non-collision, total). Prompts, README §1108, fingerprints, and regressions updated in lockstep. Local suites: 700 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Full test-authorship provenance threading (issue #1903 §B.4 never-block): - Capture "adopted from an existing HUMAN co-located test (unpinned)" at PATH RESOLUTION time (before generation makes greenfield vs adopted indistinguishable): new `content_selector.was_test_adopted`. Thread it as `adopted_human` through `_verify_test_churn` -> `TestChurnError` (new field + `adopted:` message line) at the cmd_test_main and one_session_sync churn sites, into `build_test_churn_hard_failure_from_error`'s block. - The parent runner now requires THREE guards to never-block: issue_url + structured `adopted: true` provenance (`_extract_test_churn_adopted`) + the in-repo co-located path classifier. A pinned/greenfield/older-child test reads false -> strict hard-fail. Codex round-3 correctness (content_selector greenfield): - Ambiguous (>1) existing co-located tests are no longer treated as greenfield (would fork a third file) — `resolve_test_output_path` keys on cardinality. - Explicit `testMatch` that matches nothing (or an unevaluable rule) FAILS CLOSED instead of falling through to an uncollected default path. - Refuse vitest `root`/`dir` discovery and non-literal/call-expression configs (`module.exports = buildConfig()`, delegating requires) — can't prove default. - Character-class negation `[!x]` -> `[^x]` (was "! or x"). - Default-extension gate is dialect-aware (vitest's default collects mjs/cjs). - Churn-block field extraction is scoped to the block and fails closed on conflicting `output:`/`adopted:` values (no unrelated-line leakage). #1941: the explicit-fallback auto-degrade no longer fires in review-only mode (which runs no fixer) — `not config.review_only` guard. Prompts, README, fingerprints (6 modules), and regressions updated in lockstep. Local suites: 1243 passed, 2 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…handling, prompt altitude
- SECURITY (ReDoS): repo-controlled testMatch/testRegex/testPathIgnorePatterns
are now matched through a timeout-bounded, fail-closed helper (the `regex`
module's per-match timeout + length caps), so a catastrophic-backtracking
runner pattern can never stall sync. All three pattern evaluations route
through it. Adversarial regression added.
- Jest-version-aware default extensions: Jest 30's default testMatch collects
.mjs/.cjs (Jest <=29 does not); detect the declared jest major from
package.json and widen the default-ext set only for jest>=30 or vitest;
unknown version stays strict.
- Stricter opaque-config refusal: computed property keys (`{['test'+'Match']:…}`)
and non-literal/identifier exports (`module.exports = config`,
`export default defineConfig(...)`) are no longer assumed default — refuse.
- Prompt altitude: rewrite the churn-block extraction spec at behavior level
(drop exact regexes / lock placement / internal call structure) per
docs/prompting_guide.md.
(Round-4 finding #5 "stale fingerprints" was a false positive — the stored hash
matches PDD's canonical calculate_prompt_hash; the reviewer compared a plain
sha256.) Fingerprints refreshed; regressions added. Local suites: 391 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…und, docs lockstep
Correctness (content_selector greenfield):
- bare `roots` entries resolve against the effective `rootDir` (jest semantics),
not the config dir — no longer certifies a test jest would exclude.
- explicit-empty `testMatch`/`testRegex` (`[]`) matches nothing -> refuse; a
config with BOTH testMatch and testRegex (jest rejects) -> refuse.
- runner detection no longer false-positives on any script containing the
substring "jest"/"vitest": only `test`/`test:*` scripts, real command tokens.
- extglob vs brace alternation: commas are literal inside `@(...)`/`+(...)`
extglobs (pipe alternates) and only alternate inside `{...}` braces.
Security/DoS:
- aggregate discovery work is bounded (pattern-count + candidate caps) so a
hostile large config can't stall an issue-driven sync for minutes even with
the per-match ReDoS timeouts.
Lockstep/docs:
- the runner's OWN structured churn block now renders the `adopted:` provenance
field (not just the standalone builder).
- content_selector prompt + README document the version-aware mjs/cjs rule
(jest 30+/vitest collect; jest<=29/unknown refuse); README + architecture.json
now describe all THREE never-block guards (issue scope + adoption provenance +
co-located shape).
Regressions added for each. Local suites: 397 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…undness, DoS budget, symlink containment Security: - Forgeable never-block signal: the churn `output:`/`adopted:` provenance is now required to be UNANIMOUS across EVERY churn block in the captured child output (still scoped to churn blocks). Since the never-block branch is only entered on a REAL churn, an injected/forged block that disagrees causes a conflict -> fail closed (strict hard-fail); an attacker cannot flip a real `adopted:false` to `true`. (A fully in-band signal is not attacker-proof; this defaults to the safe outcome and raises the bar substantially.) - Relative churn paths are now canonicalized against the worktree root and containment-checked (rejecting a symlink segment that escapes the root), not just lexically screened for `..` (CWE-022). - Aggregate discovery work is bounded by a single wall-clock BUDGET across the whole call (plus tighter per-match timeout and element caps), so a flood of near-timeout patterns can't accumulate minutes. Correctness: - Opaque JS config detection fails closed on dynamic construction (method calls, template literals, computed keys, identifier/call exports) that a token blacklist missed — a config that builds `testMatch` dynamically is refused. - Jest/vitest dialect is resolved for the mjs/cjs default: jest<=29 + a vitest dependency is AMBIGUOUS -> refuse; only jest>=30 or unambiguous vitest widens. Regressions added for each. Local suites: 402 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rship, ambiguous-source refusal
- Opaque JS config detection is now a positive WHITELIST: default discovery is
assumed ONLY for a single plain-object-literal export (`module.exports = {…}` /
`export default {…}`) with no discovery keys and no dynamic/composition
constructs — refusing blacklist-evading shapes like a second-statement mutation
`module.exports = {}; module.exports['test'+'Match'] = […]` (strings/comments
stripped before analysis).
- Greenfield OWNERSHIP provenance: PDD now records tests it greenfield-creates
(`.pdd/pdd_created_tests.json`) and `was_test_adopted` excludes them, so a
PDD-owned greenfield test is never reclassified as human-adopted on a later run
(would otherwise let coverage-losing churn take the never-block). Recorded at
the cmd_test_main and one_session creation sites.
- Ambiguous runner config: multiple config sources at the same directory level
(e.g. an inline package.json jest block AND a vitest.config.ts) -> fail closed,
so an inline block can't hide an active custom config file.
Prompts (content_selector/cmd_test_main/one_session_sync), fingerprints, and
regressions updated in lockstep. Local suites: 570 passed, 1 skipped.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e/tracked provenance Seven of eight round-8 findings (the eighth, forgeable in-band provenance, is a cross-process-channel design decision pending): - #1 Quoted discovery keys: `"testMatch"` was erased by string-stripping and slipped past the opaque-config whitelist. Discovery keys are now matched on the RAW config text (quoted or not) before the trivial-literal check. - #2 Multiple config FILES: the collector kept only the first JS/TS config, so `jest.config.js` + `vitest.config.ts` read as one source. It now counts every config file at a level -> >1 source is ambiguous -> refuse. - #3 Python adopted siblings: the never-block path classifier rejected `src/test_foo.py` / `src/foo_test.py`, hard-failing a supported #1903 Python adoption. It now accepts Python sibling conventions outside the top-level `tests/` shadow, alongside JS `.test.`/`.spec.`/`__tests__`. - #5 Ownership durability: the greenfield-ownership manifest moved from the routinely-gitignored `.pdd/pdd_created_tests.json` to the TRACKED `.pdd/meta/pdd_created_tests.json` (committed with the per-module fingerprints) so a fresh checkout / durable worktree keeps PDD's ownership; the legacy path is still read as a fallback. - #6 Durable resume dropped the flag: `DurableSyncRunner` rebuilds state from git checkpoint trailers only. The adopted-test `needs_review` note is now encoded into the trailer (base64url) and restored on resume, so a resumed run no longer reports a clean success and silently swallows coverage loss. - #7 ReDoS preprocessing: oversized raw `testMatch` globs are now rejected BEFORE translation/literal-dir extraction, and the hint scan is bounded, closing the gap where expansion ran outside the regex timeout. - #8 Prompt lockstep: the agentic prompt said "last churn header"; the code parses ALL churn blocks with unanimous values. Prompt updated to match. Prompts (agentic_sync_runner/durable_sync_runner), fingerprints, and regressions updated in lockstep. Local suites: 225 + 408 passed across the touched files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ance Closes the recurring forgeable-provenance finding (rounds 5, 6, 8). The never-block read failure-kind + adoption provenance from the child's in-band stdout/stderr, which a hostile project test can print — so a self-consistent forged churn block could downgrade a coverage-loss hard-fail to a "needs review" PR. The round-6 cross-block unanimity only defeated a CONFLICTING injected block, not a lone self-consistent one. Now the parent authenticates provenance over a channel untrusted test output cannot reach: - The runner mints a secret per-run nonce and hands it to each child sync ONLY over a non-inherited pipe FD (`pass_fds`; `PDD_CHURN_NONCE_FD` names it). The child inherits the FD; the grandchild test processes it later spawns do not (subprocess `close_fds` default), so a hostile test can read the env var but not the FD it names — it cannot learn the nonce. - `TestChurnError` reads that nonce once (cached) and stamps `nonce: <token>` into the churn block. Standalone `pdd test`/`sync` (no channel) omit it and never never-block. - `_extract_test_churn_adopted` / `_extract_test_churn_output_path` take an `expected_nonce`; the never-block trusts a block's `adopted:`/`output:` ONLY when its nonce matches (constant-time). A block without the nonce is dropped, not out-voted, so a forgery falls through to the strict hard-fail. Prompts (agentic_sync_runner/code_generator_main), fingerprints, and regressions updated in lockstep (forgery-defeat, wrong-nonce, no-nonce, child FD round-trip). Local suites: 975 passed, 1 skipped across the touched files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rrency, lockstep Six findings from round-9 review: - #1 Adoption must consult collection: `resolve_test_output_path` adopted an existing co-located sibling by filename alone, so a stale `src/__test__/x.test.ts` under a PARSEABLE Jest `testMatch: ["qa/**/*.spec.ts"]` was adopted and verified even though the runner excludes it (false-green preserved). It now redirects away from a provably-excluded sibling to the runner-collected location (or derived); `was_test_adopted` is tightened to require the resolved path BE that sibling, so a redirect target is never miscounted as a human adoption. - #2 Globstar: `**` was translated to `.*` everywhere, but micromatch treats `**` as a separator-crossing globstar ONLY as a whole path segment. Embedded `**` (`qa**/`, `**bar`) is now a single-segment `[^/]*`, so a pattern can no longer certify a path Jest rejects. - #3 Manifest race: parallel child processes did an unlocked read-modify-write of the ownership manifest and clobbered each other. Now under an interprocess `flock` + temp-file + atomic `os.replace`. - #4 State-save race: `_save_state` serialized only the snapshot, not the write. The whole snapshot->write->replace now runs under a dedicated `_save_lock` so a stale save cannot overwrite a newer one (dropping a needs-review note on resume). - #5 Prompt lockstep: content_selector prompt named the obsolete `.pdd/pdd_created_tests.json`; updated to the canonical tracked `.pdd/meta/pdd_created_tests.json` (legacy read-only fallback). - #6 Prompt/README/architecture lockstep: documented the Python `test_<stem>.py` / `<stem>_test.py` never-block classifier forms added in round 8. Prompts, README, architecture.json, fingerprints, and regressions updated in lockstep. Local suites: 980 passed, 1 skipped across the touched files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… node_modules ignore, per-block fail-closed, prompt altitude Six findings from round-10 review: - #1 Ownership manifest durability (recurring): `.pdd/meta/pdd_created_tests.json` was still gitignored and never staged into durable checkpoints, so a fresh checkout / durable worktree lost provenance. Added a `.gitignore` exception and taught the durable runner to force-add the shared (non-module-prefixed) manifest and accept it in checkpoint-staging validation. - #2 Jest semver: `_detected_jest_major` took the first integer, so `<30` read as 30 and `^30 || ^29` as 30 — enabling jest-30 `.mjs`/`.cjs` defaults on installs that could be <=29 (false-green). Now computes a conservative guaranteed-MINIMUM major across the full `||` range; jest-30 defaults enable only when the whole range provably guarantees major >= 30. - #3 Default ignore: greenfield discovery accepted a co-located `.test.*` candidate under `node_modules`, which both jest and vitest exclude by default. Such candidates are now refused. - #4 Per-block fail-closed: the churn extractors aggregated fields across trusted blocks into a set, so a trusted block MISSING output/adopted was covered for by a complete one. Each trusted block is now validated independently (exactly one value) before cross-block unanimity, honoring "any absence/conflict fails closed". - #5 Prompt altitude: requirement 9e transcribed private helpers, `pass_fds`, the FD/env mechanics, constant-time compare, and lock placement — against the prompting guide's Back-Propagation Altitude. Rewritten as concise behavioral rules (unforgeable provenance, fail-closed parsing, persisted review, observable output) with the mechanics dropped from the helper note too. - #6 Prompt accuracy: content_selector prompt claimed centralized runner paths are re-adopted by `find_collocated_test`; they are not — reworded to the actual behavior (deterministic re-derivation + ownership-manifest tracking). Prompts, .gitignore, fingerprints, and regressions updated in lockstep. Local suites: 985 passed, 1 skipped across the touched files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| f"repair — kept the existing test (`{kept}`) unchanged and " | ||
| f"flagged it for review (issue #1903); the PR still ships." | ||
| ) | ||
| print(f"{_TEST_CHURN_NEEDS_REVIEW_MARKER}: {kept}", flush=True) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles the PDD CLI fixes discovered while driving the six campaign issues end to end through the staging workflow. Individual fix PRs were accumulated on
stack/e2e-staging-v2; nothing from this session was merged directly tomain.Newly finalized in this pass:
The branch also preserves the previously validated campaign stack fixes for sync, provider fallback, and checkup behavior.
Verification
pdd checkup --validate-arch-includes: passed.make regression-public: passed.cmd_test_mainfinal-tree checkpoint: 56 passed, 1 skipped, 81.67% coverage.-11cases are an inherited local baseline reproduced on untouchedmain; no new failure signature.git diff --check: passed.Issues resolved by this bundle
Closes #1900
Closes #1903
Closes #1926
Closes #1936
Closes #1938
Closes #1939
Closes #1941
Closes #1957
Closes #1968
Landing order
pdd_cloudcampaign bundle after this one.Manager review/merge is required. This PR intentionally does not auto-merge.