Conversation
gltanaka
left a comment
There was a problem hiding this comment.
Change Request
Blocking Issues
1. CI gate (validate-arch-includes) is high-risk with no proof it passes today
The PR adds pdd validate-arch-includes to .github/workflows/unit-tests.yml before unit tests. The root architecture.json is 276KB with dozens of entries. If any entry's dependencies disagree with its prompt's <include> tags — which is likely given normal development drift — CI breaks for everyone on merge. Please either:
- Demonstrate this command passes on
maintoday, or - Ship it as a warning-only step (non-blocking) initially, or
- Remove it from CI and add it in a follow-up once validated
2. Duplicate guard checks git HEAD instead of file content — blocks legitimate re-runs
The most common reason to re-run pdd sync is after editing a prompt file. But editing a prompt doesn't change git HEAD until you commit. So the guard blocks the typical workflow:
pdd sync foo
vim prompts/foo.prompt # edit
pdd sync foo # BLOCKED — same argv, same HEAD
Users will learn to always pass --force, making the guard dead weight. Consider fingerprinting the mtimes or content hashes of relevant prompt files instead of (or in addition to) git HEAD.
3. _last_run_path uses cwd instead of project root
def _last_run_path(cwd: str) -> Path:
return Path(cwd) / ".pdd" / _LAST_RUN_FILENAMEIf a user runs from a subdirectory, the record is written there. The next run from project root won't find it. This should use find_project_root() (which this PR already moves to architecture_registry.py) to ensure the guard always reads/writes the same file.
4. Broad except Exception in auto-deps integration hides bugs
In auto_deps_main.py:
except Exception as arch_exc:
rprint(f"[yellow]Warning: Could not update architecture.json...")Every possible bug (KeyError, TypeError, AttributeError) in the new 177-line auto_deps_architecture.py becomes a silently ignored yellow warning. Please catch specific expected exceptions (OSError, json.JSONDecodeError) and let real bugs propagate.
Non-blocking Issues (address or explain)
5. Architecture.json reformatting risk — merge_auto_deps_includes_into_architecture re-serializes the entire file with json.dumps(arch_data, indent=2). On the 276KB root architecture.json, this can produce a massive diff if the existing formatting differs. Consider writing back only the modified entry or preserving the original formatting.
6. Cross-module import of private function — auto_deps_architecture.py imports _resolve_architecture_prompt_path from architecture_include_validation.py. The underscore prefix means private. Either rename it to make it public or extract it to a shared location.
7. No way to suppress validation warnings without --quiet — The new architecture/include warnings fire on every sync. During normal development where drift is expected, this is noisy. Consider a dedicated flag (e.g., --no-arch-warnings) or only emitting these in --verbose mode.
8. Unrelated prompt change — The BUG_STEP_TIMEOUTS addition to agentic_bug_orchestrator_python.prompt is unrelated to any feature in this PR. Consider splitting it into its own commit at minimum.
|
target 4/10 |
Ok thanks for the review. I have made these changes.
|
…tes, verbose warnings
3c67f5b to
60f92b8
Compare
E2E Validation ResultsCherry-picked all 5 commits onto Unit Tests
Manual E2E Tests (all pass)
Gaps Filed as Issues
All three are follow-up items, not merge blockers. |
Description
Adds a duplicate-run guard for expensive CLI entry points: sync, generate, and fix.
After a guarded command finishes through the normal CLI summary path, we persist a small record at .pdd/last_run.json (normalized argv tail after the program name, cwd, git rev-parse HEAD, subcommand, timestamp). Before the next run of the same subcommand, if argv, cwd, HEAD, and subcommand match the last record and the timestamp is inside a configurable window (default 15 minutes via PDD_DUPLICATE_WINDOW_MIN, or PDD_DUPLICATE_WINDOW_SEC for overrides), we treat it as a likely duplicate LLM run:
CI=1: print a warning to stderr and continue (no scripted CI break by default).
Interactive TTY (stdin and stdout TTY, not --quiet): warning plus Continue anyway? [y/N]; declining aborts the run.
Otherwise: non-interactive failure with click.UsageError unless bypassed.
Bypass: global --force or PDD_ALLOW_DUPLICATE_RUN=1. Disable entirely: PDD_DISABLE_DUPLICATE_GUARD=1.
Under pytest, the guard is off unless PDD_TEST_DUPLICATE_GUARD=1, so the existing suite is unaffected.
Wiring: check_duplicate_before_subcommand runs from the root CLI callback (with stream restore on block/abort); record_after_guarded_command runs from process_commands after a successful guarded flow.
Testing
uv run pytest tests/test_duplicate_cli_guard.py -v — covers guard on/off under pytest, CI warn-only path, non-interactive block, TTY prompt (yes/no), --force / PDD_ALLOW_DUPLICATE_RUN, different git HEAD, time window expiry, and post-run recording behavior.
Run two same commands back to back (manual)
Checklist
Fixes #751