diff --git a/.claude/commands/dev-checklists.md b/.claude/commands/dev-checklists.md
deleted file mode 100644
index 4c70d6ab8..000000000
--- a/.claude/commands/dev-checklists.md
+++ /dev/null
@@ -1,176 +0,0 @@
----
-description: Development checklists for code changes (params, methodology, warnings, reviews, bugs)
-argument-hint: "[checklist-name]"
----
-
-# Development Checklists
-
-## Adding a New Parameter to Estimators
-
-When adding a new `__init__` parameter that should be available across estimators:
-
-1. **Implementation** (for each affected estimator):
- - [ ] Add to `__init__` signature with default value
- - [ ] Store as `self.param_name`
- - [ ] Add to `get_params()` return dict
- - [ ] Handle in `set_params()` (usually automatic via `hasattr`)
-
-2. **Consistency** - apply to all applicable estimators per the **Estimator inheritance** map in CLAUDE.md
-
-3. **Testing**:
- - [ ] Test `get_params()` includes new param
- - [ ] Test parameter affects estimator behavior
- - [ ] Test with non-default value
-
-4. **Downstream tracing**:
- - [ ] Before implementing: `grep -rn "self\." diff_diff/.py` to find ALL downstream paths
- - [ ] Parameter handled in ALL aggregation methods (simple, event_study, group)
- - [ ] Parameter handled in bootstrap inference paths
-
-5. **Documentation**:
- - [ ] Update docstring in all affected classes
- - [ ] Update CLAUDE.md if it's a key design pattern
-
-## Implementing Methodology-Critical Code
-
-When implementing or modifying code that affects statistical methodology (estimators, SE calculation, inference, edge case handling):
-
-1. **Before coding - consult the Methodology Registry**:
- - [ ] Read the relevant estimator section in `docs/methodology/REGISTRY.md`
- - [ ] Identify the reference implementation(s) listed
- - [ ] Note the edge case handling requirements
-
-2. **During implementation**:
- - [ ] Follow the documented equations and formulas
- - [ ] Match reference implementation behavior for standard cases
- - [ ] For edge cases: either match reference OR document deviation
-
-3. **When deviating from reference implementations**:
- - [ ] Add entry in `docs/methodology/REGISTRY.md` using a reviewer-recognized label:
- `**Note:**`, `**Deviation from R:**`, or `**Note (deviation from R):**`
- (see CLAUDE.md "Documenting Deviations" for full format reference)
- - [ ] Include rationale (e.g., "defensive enhancement", "R errors here")
- - [ ] Ensure the deviation is an improvement, not a bug
- - [ ] If deferring P2/P3 work: shippable items get a row in `TODO.md` (Actionable
- Backlog; columns `Issue | Location | Origin | Effort | Priority`); blocked
- items get a row in `DEFERRED.md` under the matching blocker section (columns
- `Issue | Location | PR | Priority`) — see CLAUDE.md "Tracking-file map"
-
-4. **Testing methodology-aligned behavior**:
- - [ ] Test that edge cases produce documented behavior (NaN, warning, etc.)
- - [ ] Assert warnings are raised (not just captured)
- - [ ] Assert the warned-about behavior actually occurred
- - [ ] For NaN results: assert `np.isnan()`, don't just check "no exception"
- - [ ] All inference fields computed via `safe_inference()` (not inline)
-
-## Adding Warning/Error/Fallback Handling
-
-When adding code that emits warnings or handles errors:
-
-1. **Consult Methodology Registry first**:
- - [ ] Check if behavior is documented in edge cases section
- - [ ] If not documented, add it before implementing
-
-2. **Verify behavior matches message**:
- - [ ] Manually trace the code path after warning/error
- - [ ] Confirm the stated behavior actually occurs
-
-3. **Write behavioral tests**:
- - [ ] Don't just test "no exception raised"
- - [ ] Assert the expected outcome occurred
- - [ ] For fallbacks: verify fallback behavior was applied
- - [ ] Example: If warning says "setting NaN", assert `np.any(np.isnan(result))`
-
-4. **Protect arithmetic operations**:
- - [ ] Wrap ALL related operations in `np.errstate()`, not just the final one
- - [ ] Include division, matrix multiplication, and any operation that can overflow/underflow
-
-## Reviewing New Features or Code Paths
-
-When reviewing PRs that add new features, modes, or code paths (learned from PR #97 analysis):
-
-1. **Edge Case Coverage**:
- - [ ] Empty result sets (no matching data for a filter condition)
- - [ ] NaN/Inf propagation through ALL inference fields (SE, t-stat, p-value, CI)
- - [ ] Parameter interactions (e.g., new param x existing aggregation methods)
- - [ ] Control/comparison group composition for all code paths
-
-2. **Documentation Completeness**:
- - [ ] All new parameters have docstrings with type, default, and description
- - [ ] Methodology docs match implementation behavior (equations, edge cases)
- - [ ] Edge cases documented in `docs/methodology/REGISTRY.md`
-
-3. **Logic Audit for New Code Paths**:
- - [ ] When adding new modes (like `base_period="varying"`), trace ALL downstream effects
- - [ ] Check aggregation methods handle the new mode correctly
- - [ ] Check bootstrap/inference methods handle the new mode correctly
- - [ ] Explicitly test control group composition in new code paths
-
-4. **Pattern Consistency**:
- - [ ] Search for similar patterns in codebase (e.g., `t_stat = x / se if se > 0 else ...`)
- - [ ] Ensure new code follows established patterns or updates ALL instances
- - [ ] If fixing a pattern, grep for ALL occurrences first:
- ```bash
- grep -n "if.*se.*> 0.*else" diff_diff/*.py
- ```
-
-## Fixing Bugs Across Multiple Locations
-
-When a bug fix involves a pattern that appears in multiple places (learned from PR #97 analysis):
-
-1. **Find All Instances First**:
- - [ ] Use grep/search to find ALL occurrences of the pattern before fixing
- - [ ] Document the locations found (file:line)
- - [ ] Example: `t_stat = effect / se if se > 0 else 0.0` appeared in 7 locations
-
-2. **Fix Comprehensively in One Round**:
- - [ ] Fix ALL instances in the same PR/commit
- - [ ] Create a test that covers all locations
- - [ ] Don't fix incrementally across multiple review rounds
-
-3. **Regression Test the Fix**:
- - [ ] Verify fix doesn't break other code paths
- - [ ] For early-return fixes: ensure downstream code still runs when needed
- - [ ] Example: Bootstrap early return must still compute per-effect SEs
-
-4. **Common Patterns to Watch For**:
- - `if se > 0 else 0.0` -> should be `else np.nan` for undefined statistics
- - `if len(data) > 0 else return` -> check what downstream code expects
- - `mask = (condition)` -> verify mask logic for all parameter combinations
-
-## Pre-Merge Review Checklist
-
-Final checklist before approving a PR:
-
-1. **Behavioral Completeness**:
- - [ ] Happy path tested
- - [ ] Edge cases tested (empty data, NaN inputs, boundary conditions)
- - [ ] Error/warning paths tested with behavioral assertions
-
-2. **Inference Field Consistency**:
- - [ ] If one inference field (SE, t-stat, p-value) can be NaN, all related fields handle it
- - [ ] Aggregation methods propagate NaN correctly
- - [ ] Bootstrap methods handle NaN in base estimates
-
-3. **Documentation Sync**:
- - [ ] Docstrings updated for all changed signatures
- - [ ] `diff_diff/guides/llms.txt` updated if a new estimator/feature appears in the public API (this is the AI-agent contract; it cascades to RTD)
- - [ ] `docs/api/*.rst` updated for new modules / signatures
- - [ ] `docs/references.rst` updated if a new scholarly source is cited
- - [ ] `README.md` updated ONLY if (a) new estimator catalog one-liner, (b) hero/badges/tagline change, or (c) top-level capability paragraph (Diagnostics & Sensitivity, Survey Support). Do NOT add usage examples, parameter tables, or per-estimator sections.
- - [ ] `REGISTRY.md` updated if methodology edge cases change
-
-## Quick Reference: Common Patterns to Check
-
-Before submitting methodology changes, verify these patterns:
-
-```bash
-# Find potential NaN handling issues (should use np.nan, not 0.0)
-grep -n "if.*se.*>.*0.*else 0" diff_diff/*.py
-
-# Find all t_stat calculations to ensure consistency
-grep -n "t_stat.*=" diff_diff/*.py
-
-# Find all inference field assignments
-grep -n "\(se\|t_stat\|p_value\|ci_lower\|ci_upper\).*=" diff_diff/*.py | head -30
-```
diff --git a/.claude/commands/pre-merge-check.md b/.claude/commands/pre-merge-check.md
index 4a5937729..b24b83bb4 100644
--- a/.claude/commands/pre-merge-check.md
+++ b/.claude/commands/pre-merge-check.md
@@ -21,93 +21,80 @@ git ls-files --others --exclude-standard
```
Categorize files into:
-- **Methodology files**: `diff_diff/*.py` (excluding `__init__.py`)
+- **Methodology files**: `diff_diff/**/*.py` (excluding `__init__.py`) — match
+ **recursively**. `diff_diff/` contains real packages (`visualization/`, `guides/`),
+ and a non-recursive `diff_diff/*.py` glob silently skips every file inside them,
+ so their pattern checks and test lookups never run.
- **Test files**: `tests/*.py`
- **Documentation files**: `*.md`, `*.rst`, `docs/**`
+- **Notebooks**: `docs/tutorials/*.ipynb`
-### 2. Run Automated Pattern Checks
+For a nested file, the name used for test discovery in Section 2.2 is the
+**package directory**, not the module: `diff_diff/visualization/_event_study.py`
+resolves on `visualization`.
-#### 2.1 Inference & Parameter Pattern Checks (for methodology files)
+### 2. Run Automated Pattern Checks (via the argv-safe helper)
-> **Canonical definitions** — This section is referenced by `/submit-pr` and `/push-pr-update`. Keep it as the single source of truth for methodology pattern checks.
+#### 2.1 Methodology pattern checks + test resolution — `premerge_scan.py`
-If any methodology files changed, run these pattern checks on the **changed methodology files only**:
+> **Canonical** — This section is referenced by `/submit-pr` and `/push-pr-update`.
+> The single source of truth for the methodology pattern checks is
+> `.claude/scripts/premerge_scan.py`. Do NOT hand-run greps over changed filenames.
-**Check A — Inline inference computation**:
-```bash
-grep -n "t_stat[[:space:]]*=[[:space:]]*[^#]*/ *se" | grep -v "safe_inference"
-```
-Flag each match: "Consider using `safe_inference()` from `diff_diff.utils` instead of inline t_stat computation."
+**Why a helper, not prose greps.** A filename is git-controlled data: git permits
+`$()`, backticks, quotes, spaces, and newlines in a path, so `grep pattern ` or
+`pytest ` over a changed path executes a payload like `diff_diff/$(touch x).py`.
+Prose "screens" leak (untracked paths, argument-vs-assignment forms, other commands).
+`premerge_scan.py` closes this structurally: it discovers changed files via
+`git … -z` through a subprocess **argv array** (never a shell), runs the pattern checks
+(A–D below) as **pure-Python regex over file content** (opening a file by path cannot
+execute a filename), resolves test coverage by matching names in Python, and **screens
+every path** — emitting only validated-safe run-lists. It is unit-tested
+(`tests/test_premerge_scan.py`) with staged *and* untracked `$(touch sentinel)`
+filenames asserting nothing executes.
-**Check B — Zero-SE fallback to 0.0 instead of NaN**:
-```bash
-grep -En "if.*(se|SE).*>.*0.*else[[:space:]]+(0\.0|0)"
-```
-Flag each match: "SE=0 should produce NaN, not 0.0, for inference fields."
+Run it:
-**Check C — New `self.X` in `__init__` not in `get_params()`**:
```bash
-# Extract new self.X assignments from diff
-git diff HEAD -- | grep "^+" | grep "self\.\w\+ = \w\+" | sed 's/.*self\.\(\w\+\).*/\1/' | sort -u
-# For each extracted param name, check if it appears in get_params()
-grep "get_params" -A 30 | grep ""
+SCRATCH="$(git rev-parse --git-path premerge-scan)"; mkdir -p "$SCRATCH"
+python3 .claude/scripts/premerge_scan.py --scratch "$SCRATCH"
```
-Flag missing params: "New parameter `X` stored in `__init__` but not found in `get_params()` return dict."
-
-**Check D — `compute_confidence_interval` called without NaN guard**:
-```bash
-grep -n "compute_confidence_interval" | grep -v "safe_inference\|if.*isfinite\|if.*se.*>"
-```
-Flag each match: "Verify CI computation handles non-finite SE (use `safe_inference()` or add guard)."
-
-**Report findings**: If matches found, list each with file:line and the recommended fix.
-
-#### 2.2 Test Existence Check
-For each changed methodology file, check if corresponding test file was also changed:
-
-| Methodology File | Expected Test File |
-|------------------|-------------------|
-| `diff_diff/staggered.py` | `tests/test_staggered.py` |
-| `diff_diff/estimators.py` | `tests/test_estimators.py` |
-| `diff_diff/twfe.py` | `tests/test_estimators.py` |
-| `diff_diff/synthetic_did.py` | `tests/test_estimators.py` |
-| `diff_diff/sun_abraham.py` | `tests/test_sun_abraham.py` |
-| `diff_diff/triple_diff.py` | `tests/test_triple_diff.py` |
-| `diff_diff/trop.py` | `tests/test_trop.py` |
-| `diff_diff/bacon.py` | `tests/test_bacon.py` |
-| `diff_diff/linalg.py` | `tests/test_linalg.py` |
-| `diff_diff/utils.py` | `tests/test_utils.py` |
-| `diff_diff/diagnostics.py` | `tests/test_diagnostics.py` |
-| `diff_diff/prep.py` | `tests/test_prep.py` |
-| `diff_diff/visualization.py` | `tests/test_visualization.py` |
-| `diff_diff/honest_did.py` | `tests/test_honest_did.py` |
-| `diff_diff/power.py` | `tests/test_power.py` |
-| `diff_diff/pretrends.py` | `tests/test_pretrends.py` |
-
-**Report**: List any methodology files without corresponding test changes.
+- If it **exits 4**, a git or file-read operation failed — the scan is **incomplete**
+ and its run-lists were truncated to empty. **Stop and report the error;** do NOT
+ continue (empty run-lists would silently run no tests and misstate coverage).
+- If it **exits 3**, it found a changed path containing shell metacharacters. It has
+ already excluded that path; surface the reported path(s) for manual review — a
+ methodology filename with `$()`/backticks is itself a red flag — and do not improvise
+ a shell command over it.
+- Report its pattern findings (file:line) to the user.
+- The safe, NUL-delimited test/notebook run-lists it writes
+ (`$SCRATCH/run-tests.z`, `$SCRATCH/run-notebooks.z`) are consumed argv-safely in
+ Section 4 — never re-derive filenames into a shell command yourself.
+
+The checks the helper performs (kept here as the human-readable spec; the helper is the
+executable source of truth):
+
+- **Check A** — inline inference: a line matching `t_stat = … / se` without
+ `safe_inference`. Fix: use `safe_inference()` from `diff_diff.utils`.
+- **Check B** — zero-SE fallback: `if se > 0 … else 0.0`. Fix: SE=0 → NaN, not 0.0.
+- **Check C** — a new `self.X` (from the diff) absent from `get_params()`.
+- **Check D** — `compute_confidence_interval` without a `safe_inference`/`isfinite`/
+ `if se >` guard.
+- **Test resolution** — for each changed methodology file, the test files whose name
+ contains its stem (leading underscore stripped) or its `doc-deps.yaml` group name;
+ reports any with no resolved suite so coverage is confirmed by hand rather than
+ silently skipped.
#### 2.3 Docstring Check (heuristic)
-For changed `.py` files, run a quick check for functions without docstrings:
-```bash
-# Find public function definitions (heuristic check)
-grep -n "^def [^_]" | head -10
-grep -n "^ def [^_]" | head -10
-```
-
-Note: This is a heuristic. Verify docstrings exist for new public functions.
-
-#### 2.4 Docstring Staleness Check (for changed .py files)
-
-For functions with changed signatures, verify their docstrings are up to date:
-
-```bash
-# Find functions with changed signatures
-git diff HEAD -- | grep "^+.*def " | head -10
-```
-
-For each changed function, flag: "Verify docstring Parameters section matches updated signature for: ``"
+Public functions in the changed `.py` files should have docstrings, and functions with
+changed signatures should have up-to-date `Parameters` sections. This is a heuristic —
+do **not** grep changed filenames in a shell (a path like `diff_diff/$(touch x).py`
+would execute). Read each changed methodology file (the safe list from Section 2.1)
+with the Read tool and scan for public `def`s lacking a docstring, and for `+`-added
+`def` lines in the diff whose docstring may be stale. Flag those for the user to
+confirm. Reading a file by path never executes its name.
#### 2.5 Documentation Impact Check
@@ -202,27 +189,84 @@ Based on your changes to:
### 4. Ask About Running Tests
-Use AskUserQuestion to offer test options:
+Use AskUserQuestion. **Targeted is the default** — run tests for the touched code
+areas, not the whole suite:
```
Would you like to run tests now?
Options:
-1. Yes - run full test suite (pytest)
-2. Yes - run only tests for changed files
-3. No - skip tests for now
+1. Yes - tests for changed files only (recommended)
+2. No - skip tests for now
+3. Yes - full test suite (slow; only when the change is broad)
```
-If user chooses option 1:
+For option 1, run the resolved suites `premerge_scan.py` wrote to
+`$SCRATCH/run-tests.z` (Section 2.1) — **argv-safe via `xargs -0`**, so a hostile test
+filename is passed as an argument, never reparsed by the shell:
+
```bash
-pytest
+SCRATCH="$(git rev-parse --git-path premerge-scan)"
+if [ -s "$SCRATCH/run-tests.z" ]; then
+ xargs -0 pytest < "$SCRATCH/run-tests.z"
+else
+ echo "No test files resolved for the changed modules."
+ # Offer: run the full suite (explicitly), name the tests to run, or skip.
+ # Do NOT run a bare `pytest` — it discovers and runs the ENTIRE suite, the opposite
+ # of the targeted run requested.
+fi
```
-If user chooses option 2, run targeted tests based on changed files:
+The list already includes every changed `tests/test_*.py` and the module-resolved
+suites (helper's job). Never run more than one pytest process at a time.
+
+### 4b. Validate Notebooks (only if notebooks changed)
+
+Skip this section entirely when no `docs/tutorials/*.ipynb` files changed.
+
+CI runs `pytest --nbmake docs/tutorials/` but is gated behind the `ready-for-ci`
+label, and the branch is frozen once that label is on — so notebook breakage has to
+be caught *here*, before submitting. Two notebooks are additionally excluded from
+the main CI job as too slow and are only ever executed locally:
+`06_power_analysis.ipynb` and `10_trop.ipynb`.
+
+Run the changed notebooks from the helper's safe list — **argv-safe via `xargs -0`**,
+never a filename pasted into the command:
+
```bash
-pytest tests/test_staggered.py tests/test_estimators.py # (example)
+SCRATCH="$(git rev-parse --git-path premerge-scan)"
+[ -s "$SCRATCH/run-notebooks.z" ] && \
+ xargs -0 pytest --nbmake --nbmake-timeout=600 < "$SCRATCH/run-notebooks.z"
```
+`run-notebooks.z` already contains **every** changed notebook, `06_power_analysis.ipynb`
+and `10_trop.ipynb` included — so this one command covers them; do **not** run those two
+again separately. The point about 06/10 is only *why* local validation matters (CI skips
+them), not that they need a second invocation.
+
+**Exception — `26_composition_drift_calibration.ipynb` needs a dependency the dev
+extra does not provide.** It imports `balance`, which is not in `[dev]`; CI runs it
+in a separate interop job that installs `balance>=0.21`. Running it in a normal dev
+environment fails on the import, which is an environment problem masquerading as a
+notebook regression.
+
+If that notebook is among the changed set, check first and ask before installing:
+
+```bash
+python -c "import balance" 2>/dev/null && echo "balance: present" || echo "balance: MISSING"
+```
+
+If missing, offer to `pip install "balance>=0.21"` or to skip that one notebook and
+let the CI interop job cover it. Do not install it silently — it is a heavy
+dependency that is deliberately absent from `[dev]`.
+
+`nbmake` executes notebooks **without writing outputs back**, so there is no
+clear-outputs cleanup step and nothing dirty can reach a commit. Use it rather than
+`jupyter nbconvert --execute --inplace`, which mutates files in place and is not a
+dependency of this project.
+
+Notebook runs take minutes. Ask before starting, and report per-notebook pass/fail.
+
### 5. Report Summary
```
@@ -246,7 +290,11 @@ Review the checklist items above before running /submit-pr
## Notes
-- This skill is read-only - it only analyzes and reports, doesn't modify files
+- Non-mutating: analyses and reports only. Running tests and `--nbmake` does not
+ modify tracked files.
- Run this BEFORE `/submit-pr` to catch issues early
- Pattern checks are heuristics - review flagged items manually to confirm
- If pattern checks find issues, fix them before submitting
+- Check B (zero-SE fallback) currently matches nothing repo-wide. That is the point:
+ it is a regression guard against reintroducing an anti-pattern that was eradicated,
+ not a dead check. Keep it.
diff --git a/.claude/commands/push-pr-update.md b/.claude/commands/push-pr-update.md
index 863a101eb..e2a53e8a2 100644
--- a/.claude/commands/push-pr-update.md
+++ b/.claude/commands/push-pr-update.md
@@ -1,17 +1,39 @@
---
-description: Push code revisions to an existing PR and trigger AI code review
-argument-hint: "[--message ] [--no-review]"
+description: Push code revisions to an existing PR
+argument-hint: "[--message ]"
---
# Push PR Update
-Push local changes to an existing pull request branch and optionally trigger AI code review.
+Push local changes to an existing pull request branch.
+
+For same-repo PRs, pushing **automatically starts the CI codex review** — this
+command must never post an `/ai-review` comment to trigger one. Doing so
+double-fires the reviewer and clutters the PR.
+
+For **fork** PRs the reviewer does not run at all: `.github/workflows/ai_pr_review.yml`
+gates `pull_request` runs on `head.repo.full_name == github.repository` to avoid
+untrusted checkout. Ask the PR itself which case applies:
+
+```bash
+gh pr view --json isCrossRepository --jq '.isCrossRepository'
+```
+
+`true` means a fork PR — say the reviewer is security-gated and point at
+`/ai-review-local`, rather than telling the user to poll for something that will
+never arrive.
+
+**Do not infer this by comparing `gh pr view --json headRepositoryOwner` against
+`gh repo view --json owner`.** In a fork checkout `gh repo view` resolves to the
+fork, so both sides return the fork owner and a genuine cross-repository PR is
+misreported as same-repo — precisely backwards. `isCrossRepository` is computed by
+GitHub from the PR's head and base repositories and does not depend on which
+checkout the CLI is run from.
## Arguments
`$ARGUMENTS` may contain:
- `--message ` (optional): Custom commit message. If omitted, auto-generate from changes.
-- `--no-review` (optional): Skip triggering AI review after push.
## Instructions
@@ -19,23 +41,29 @@ Push local changes to an existing pull request branch and optionally trigger AI
Parse `$ARGUMENTS` to extract:
- **--message**: Custom commit message (everything after `--message` until next flag or end)
-- **--no-review**: Boolean flag
### 2. Validate Current State
-1. **Get repository default branch**:
+> **Refs are data — resolve them into variables, never interpolate ``.**
+> A git ref name can contain `$()` or backticks (git accepts them), so pasting a
+> resolved default branch / comparison ref into a shell command executes it. Resolve
+> them into shell variables via command substitution and use only **quoted** forms
+> (`"$DEFAULT_BRANCH"`, `"$COMPARISON_REF..HEAD"`) everywhere below. Variables do not
+> persist across separate Bash tool calls, so re-run the two-line resolution at the top
+> of any later block that needs them (it is deterministic).
+
+1. **Resolve the default branch into a variable**:
```bash
- gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name'
+ DEFAULT_BRANCH="$(gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name')"
```
- Store as ``.
2. **Check current branch**:
```bash
- git branch --show-current
+ CURRENT="$(git branch --show-current)"
```
- - If current branch equals ``, abort:
+ - If `"$CURRENT"` equals `"$DEFAULT_BRANCH"`, abort:
```
- Error: Cannot push PR update from branch.
+ Error: Cannot push PR update from the default branch.
Switch to a feature branch or use /submit-pr to create a new PR.
```
@@ -59,46 +87,49 @@ Parse `$ARGUMENTS` to extract:
```bash
git rev-parse --abbrev-ref @{u} 2>/dev/null
```
- - If NO upstream exists:
- - Determine comparison ref (handles shallow/single-branch clones):
- - If `` exists locally (`git rev-parse --verify 2>/dev/null`): use ``
- - Else if `origin/` exists (`git rev-parse --verify origin/ 2>/dev/null`): use `origin/`
- - Else: fetch it first (`git fetch origin --depth=1 2>/dev/null || true`), then use `origin/`
- - Store as ``
- - Check if branch has commits ahead: `git rev-list --count ..HEAD 2>/dev/null || echo "0"`
- - If ahead count > 0:
- - **Scan for secrets in commits to push** (see Section 3a below)
- - Compute ``: `git diff --name-only ..HEAD | wc -l`
- - Proceed to Section 3a (secret scan), then 3b (methodology checks), then Section 4 (Push to Remote) — will push with `-u` to set upstream
- - If ahead count = 0: Abort (new branch with nothing to push):
- ```
- No changes detected. Working directory is clean and branch has no commits ahead of .
- Nothing to push.
- ```
- - If upstream EXISTS:
- - Check if branch is ahead: `git rev-list --count @{u}..HEAD`
- - If ahead count > 0:
- - **Scan for secrets in commits to push** (see Section 3a below)
- - Compute ``: `git diff --name-only @{u}..HEAD | wc -l`
- - Proceed to Section 3a (secret scan), then 3b (methodology checks), then Section 4 (Push to Remote) — there are committed changes to push
- - If ahead count = 0: Abort:
- ```
- No changes detected. Working directory is clean and branch is up to date.
- Nothing to push.
- ```
+ - **Resolve `COMPARISON_REF` into a variable** — quoted, deterministic, handles
+ shallow/single-branch clones. Prefer the upstream `@{u}`; else the local or
+ `origin/` default branch (fetched shallow if absent):
+ ```bash
+ DEFAULT_BRANCH="$(gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name')"
+ if UP="$(git rev-parse --abbrev-ref @{u} 2>/dev/null)"; then
+ COMPARISON_REF="$UP"
+ elif git rev-parse --verify "$DEFAULT_BRANCH" >/dev/null 2>&1; then
+ COMPARISON_REF="$DEFAULT_BRANCH"
+ elif git rev-parse --verify "origin/$DEFAULT_BRANCH" >/dev/null 2>&1; then
+ COMPARISON_REF="origin/$DEFAULT_BRANCH"
+ else
+ git fetch origin "$DEFAULT_BRANCH" --depth=1 2>/dev/null || true
+ COMPARISON_REF="origin/$DEFAULT_BRANCH"
+ fi
+ AHEAD="$(git rev-list --count "$COMPARISON_REF..HEAD" 2>/dev/null || echo 0)"
+ ```
+ - If `"$AHEAD"` > 0:
+ - **Scan for secrets in commits to push** (Section 3a)
+ - Files changed: `git diff --name-only "$COMPARISON_REF..HEAD" | wc -l`
+ - Proceed to Section 3a (secret scan), 3b (methodology), Section 4 (push with `-u`)
+ - If `"$AHEAD"` == 0: abort — "No changes detected; branch has no commits ahead of
+ the default branch. Nothing to push."
+ - If upstream EXISTS: `AHEAD="$(git rev-list --count "@{u}..HEAD")"`.
+ - `"$AHEAD"` > 0 → scan (3a), `git diff --name-only "@{u}..HEAD" | wc -l`, then
+ 3a → 3b → Section 4.
+ - `"$AHEAD"` == 0 → abort — "No changes detected; branch is up to date. Nothing
+ to push."
### 3a. Secret Scan for Already-Committed Changes (when skipping Section 3)
When the working tree is clean but commits are ahead, scan for secrets in the commits to be pushed before proceeding to Section 4:
-1. **Get diff range**: Use `..HEAD` (from Section 2.4 — either `@{u}`, ``, or `origin/`)
+1. **Re-resolve `COMPARISON_REF`** at the top of this block (variables do not persist
+ across tool calls) using the same deterministic snippet as Section 2.4, then use it
+ **quoted** — never a raw `` placeholder.
2. **Run pattern check** using the canonical patterns from `/pre-merge-check` Section 2.6:
```bash
- secret_files=$(git diff ..HEAD -G "" --name-only 2>/dev/null || true)
- sensitive_files=$(git diff --name-only ..HEAD | grep -iE "" || true)
+ secret_files=$(git diff "$COMPARISON_REF..HEAD" -G "" --name-only 2>/dev/null || true)
+ sensitive_files=$(git diff --name-only "$COMPARISON_REF..HEAD" | grep -iE "" || true)
```
- Read the actual regex values from `/pre-merge-check` Section 2.6 at execution time. Uses `-G` to search diff content but `--name-only` to output only file names.
+ Read the actual regex values from `/pre-merge-check` Section 2.6 at execution time. Uses `-G` to search diff content but `--name-only` to output only file names. (``/`` are the fixed literal regexes from Section 2.6, not git-controlled data.)
3. **If patterns detected** (i.e., `secret_files` or `sensitive_files` is non-empty), warn with AskUserQuestion:
```
@@ -115,18 +146,13 @@ When the working tree is clean but commits are ahead, scan for secrets in the co
When the working tree is clean but commits are ahead, check for methodology issues before pushing:
-1. **Detect methodology files in committed changes**:
- ```bash
- git diff --name-only ..HEAD | grep "^diff_diff/.*\.py$" | grep -v "__init__"
- ```
-
-2. If methodology files are present:
- 1. Read `/pre-merge-check` Section 2.1 for pattern check definitions.
- 2. Run **all four pattern checks (A through D)** on those methodology files.
- **Check C override**: The canonical Check C uses `git diff HEAD` which is empty on a clean working tree. For already-committed changes, substitute `git diff ..HEAD -- ` to extract new `self.X` assignments from the committed diff range.
- 3. For any matches, display the file:line and flag message from that section.
-
- If warnings are found, display them as warnings (non-blocking) since changes are already committed.
+1. **Methodology review of already-committed changes is deferred to `/pre-merge-check`.**
+ The changes here are already committed, so this pattern check is non-blocking, and
+ the pre-merge gate (run before committing) is the right place for it. Do **not**
+ interpolate a comparison ref into a scan command here — `/pre-merge-check` covers the
+ working-tree case safely via `premerge_scan.py`, and re-deriving a committed range in
+ prose is where injection creeps back in. If you want the committed range checked, run
+ `/pre-merge-check` on the branch before it was committed, or review the diff by eye.
3. **Documentation impact check**: Check which source files in `diff_diff/` are in the committed changes.
If source files are present, read `docs/doc-deps.yaml` and check which dependent
@@ -150,17 +176,16 @@ Note: Section 3b checks are informational warnings only — no AskUserQuestion p
git add -A
```
-2. **Quick pattern check** (if methodology files are staged):
+2. **Quick pattern check** — run the argv-safe helper, never a shell grep over
+ filenames:
```bash
- git diff --cached --name-only | grep "^diff_diff/.*\.py$" | grep -v "__init__"
+ SCRATCH="$(git rev-parse --git-path premerge-scan)"; mkdir -p "$SCRATCH"
+ python3 .claude/scripts/premerge_scan.py --scratch "$SCRATCH"
```
-
- If methodology files are present:
- 1. Read `/pre-merge-check` Section 2.1 for pattern check definitions.
- 2. Run **all four pattern checks (A through D)** on the staged methodology files.
- 3. For any matches, display the file:line and flag message from that section.
-
- If warnings are found:
+ Runs the methodology pattern checks (A–D) in pure Python; **exit 3** = a
+ metacharacter-bearing path, **exit 4** = a git/read failure (incomplete scan —
+ **stop and report**, do not push on an empty scan). See `/pre-merge-check`
+ Section 2.1. If it reports findings:
```
Pre-commit pattern check found N potential issues:
@@ -219,15 +244,30 @@ Note: Section 3b checks are informational warnings only — no AskUserQuestion p
- Run `git diff --cached --stat` to see what's being committed
- Analyze the changes and generate a descriptive commit message
- Use imperative mood ("Add", "Fix", "Update", "Refactor")
- - Format with HEREDOC and Co-Authored-By:
- ```bash
- git commit -m "$(cat <<'EOF'
-
-
- Co-Authored-By: Claude Opus 4.5
- EOF
- )"
- ```
+ - **Commit via `git commit --file`, never a heredoc.** `--message` here is raw user
+ input, and a `git commit -m "$(cat <<'EOF' … EOF)"` heredoc breaks if the message
+ contains a line that is exactly `EOF`: the heredoc closes early and the following
+ lines run as shell. The Write tool never invokes a shell, and `git commit --file`
+ reads the file verbatim. Do this as **three ordered operations, not one shell
+ block** (a Write tool call cannot run inside a Bash process):
+
+ 1. Derive and print the literal path (one Bash call):
+ ```bash
+ git rev-parse --git-path push-commit-msg.txt
+ ```
+ 2. **Write the message to that literal path with the Write tool.**
+ 3. Commit, then clean up **while preserving the commit's exit status** — do not let
+ `rm` become the block's successful last command and mask a failed commit (one
+ Bash call, re-deriving the path):
+ ```bash
+ MSG_FILE="$(git rev-parse --git-path push-commit-msg.txt)"
+ rc=0; git commit --file "$MSG_FILE" || rc=$?
+ rm -f "$MSG_FILE"
+ [ "$rc" -eq 0 ] || { echo "commit failed ($rc)"; exit "$rc"; }
+ ```
+ Do NOT append `Co-Authored-By`, `Claude-Session`, "Generated with Claude
+ Code", or any other authorship trailer. The commit message describes the
+ change, not who typed it.
### 4. Push to Remote
@@ -253,59 +293,28 @@ Note: Section 3b checks are informational warnings only — no AskUserQuestion p
git log -1 --oneline
```
-### 5. Trigger AI Review (unless `--no-review`)
+### 5. Report Results
-If `--no-review` flag was NOT provided:
-
-1. **Get repository owner and name**:
- ```bash
- gh repo view --json owner,name --jq '.owner.login + "/" + .name'
- ```
- Store as `/` (resolves to the current repo context, correct for fork workflows).
- Parse to extract `` and ``.
-
-2. **Add review comment using MCP tool**:
- ```
- mcp__github__add_issue_comment with parameters:
- - owner:
- - repo:
- - issue_number:
- - body: "/ai-review"
- ```
-
-### 6. Report Results
-
-**If AI review triggered:**
```
Changes pushed to PR #
Commit: -
Files changed:
-AI code review triggered. Results will appear shortly.
-When AI review is green, add the `ready-for-ci` label to trigger CI tests (if not already added).
-
PR URL:
-```
-**If `--no-review` was used:**
+
+CI tests require the `ready-for-ci` label, which the user adds (never Claude).
```
-Changes pushed to PR #
-Commit: -
-Files changed:
-
-PR URL:
-
-Tip: Run /ai-review to request AI code review.
-Note: CI tests require the `ready-for-ci` label on the PR.
-```
+- **Same-repo**: `AI code review started automatically on push — poll the PR for the bot's Overall Assessment rather than posting anything to request it.`
+- **Fork**: `The CI AI reviewer is security-gated and will NOT run on fork PRs. Use /ai-review-local instead.`
## Error Handling
### Not on a Feature Branch
```
-Error: Cannot push PR update from branch.
+Error: Cannot push PR update from the default branch.
Switch to a feature branch or use /submit-pr to create a new PR.
```
@@ -317,7 +326,7 @@ Nothing to push.
### No Changes to Commit or Push (no upstream, no commits ahead)
```
-No changes detected. Working directory is clean and branch has no commits ahead of .
+No changes detected. Working directory is clean and branch has no commits ahead of the default branch.
Nothing to push.
```
@@ -338,22 +347,17 @@ If the remote has new commits, try:
## Examples
```bash
-# Push changes with auto-generated commit message and trigger AI review
+# Push changes with auto-generated commit message
/push-pr-update
# Push with custom commit message
/push-pr-update --message "Address PR feedback: fix edge case handling"
-
-# Push without triggering AI review
-/push-pr-update --no-review
-
-# Both options together
-/push-pr-update --message "Fix typo in docstring" --no-review
```
## Notes
-- This skill is for updating existing PRs. Use `/submit-pr` to create new PRs.
+- This command is for updating existing PRs. Use `/submit-pr` to create new PRs.
- Always stages ALL changes (`git add -A`). Stage manually first for partial commits.
-- The `/ai-review` comment triggers the repository's AI review workflow (if configured).
+- Pushing auto-starts the CI codex review. Never post `/ai-review` to trigger it.
+- Uses the `gh` CLI throughout. The GitHub MCP server is NOT used in this repo.
- Uses the same secret scanning as `/submit-pr` to prevent accidental credential commits.
diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md
index 68e36870d..02ae2110d 100644
--- a/.claude/commands/review-plan.md
+++ b/.claude/commands/review-plan.md
@@ -79,9 +79,10 @@ Read the project's `CLAUDE.md` file to understand:
- Key design patterns (sklearn-like API, formula interface, results objects, etc.)
- Estimator inheritance map
- Testing conventions
-- Key reference file pointers (methodology registry, development checklists, etc.)
+- Key reference file pointers (methodology registry, etc.)
-Also read `.claude/commands/dev-checklists.md` for development checklists.
+Also read `CONTRIBUTING.md` for documentation requirements, test writing
+guidelines, and implementation guidelines.
If the plan modifies estimator math, standard error formulas, inference logic, or edge-case handling, also read `docs/methodology/REGISTRY.md` to understand the academic foundations and reference implementations for the affected estimator(s).
@@ -353,11 +354,16 @@ Present the review in the following format. Number each issue sequentially withi
---
-## Checklist Gaps
+## Convention Gaps
-Cross-reference against the relevant development checklists in `.claude/commands/dev-checklists.md`. List which checklist items are not addressed by the plan.
+Cross-reference against `CLAUDE.md` and `CONTRIBUTING.md`. List project conventions
+the plan does not account for.
-[Identify which checklist applies (e.g., "Adding a New Parameter to Estimators", "Implementing Methodology-Critical Code", "Fixing Bugs Across Multiple Locations") and list any items from that checklist that the plan doesn't cover.]
+[Draw on the conventions that actually apply to this plan — e.g. the estimator
+inheritance map and `get_params`/`set_params` propagation for a new parameter;
+`safe_inference()` for inference fields; the deviation-labelling rules for
+methodology changes; grep-all-sites-then-fix-in-one-PR for pattern bugs; the
+documentation surfaces in "README discipline". List only what the plan misses.]
**Registry Alignment** (if methodology files changed):
- [ ] Plan equations match REGISTRY.md (or deviations documented)
diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md
deleted file mode 100644
index bff66a80f..000000000
--- a/.claude/commands/review-pr.md
+++ /dev/null
@@ -1,252 +0,0 @@
----
-description: Review a GitHub PR for methodology, code quality, and maintainability
-argument-hint: " [--paper ] [--post]"
----
-
-# Review Pull Request
-
-Perform comprehensive code review on a GitHub PR.
-
-## Arguments
-
-`$ARGUMENTS` should contain:
-- PR number (required): e.g., "74", "#74"
-- `--paper ` (optional): Path to reference paper (PDF) for methodology validation
-- `--post` flag (optional): Post the review instead of generating draft
-
-## Modes
-
-### Draft Mode (default)
-`/review-pr ` - Generate review and save to file
-
-### Post Mode
-`/review-pr --post` - Post existing review to PR
-
-## Review Framework
-
-Analyze PRs across 6 dimensions:
-
-### 1. Methodology Review
-- Statistical/algorithmic correctness
-- Alignment with academic references (if `--paper` provided, compare implementation against paper)
-- Edge case handling
-- Mathematical accuracy of formulas/algorithms
-
-### 2. Code Quality
-- Test coverage for new code
-- Error handling
-- API consistency with existing codebase patterns
-
-### 3. Security Review
-- Input validation and sanitization
-- Injection risks (SQL, command, etc.)
-- Secrets/credentials exposure
-- Dependency vulnerabilities
-
-### 4. Documentation Review
-- Docstrings for new/modified functions
-- `diff_diff/guides/llms.txt` updated if a new public-API surface landed (AI-agent contract)
-- API documentation (RST files in `docs/api/`)
-- `docs/references.rst` updated for new scholarly citations
-- README updated ONLY for landing-page-relevant changes (catalog one-liner, hero/badges/tagline, top-level capability paragraph). Per CONTRIBUTING.md, README is not the place for usage examples or per-estimator sections.
-- Inline comments for complex logic
-
-### 5. Performance
-- Computational complexity
-- Memory usage
-- Unnecessary operations
-
-### 6. Maintainability & Tech Debt
-- Code duplication
-- Breaking changes
-- Backward compatibility
-
-## Instructions
-
-### 1. Parse Arguments
-
-Parse `$ARGUMENTS` to extract:
-- **PR number**: Required. Strip '#' prefix if present.
-- **--paper **: Optional. Path to reference paper PDF for methodology validation.
-- **--post**: Optional flag. If present, post the review instead of generating draft.
-
-If no PR number is provided, use AskUserQuestion to request it.
-
-### 2. Draft Mode Workflow (default, when --post is NOT present)
-
-1. **Fetch PR info** using GitHub MCP tools:
- - Use `mcp__github__get_pull_request` to get PR details (title, description, author, base branch)
- - Use `mcp__github__get_pull_request_files` to get list of changed files
-
-2. **Analyze changes**:
- - Read each changed file using the Read tool to understand the full context
- - For source files (.py): Review methodology correctness, code quality, security
- - For test files: Assess test coverage completeness for new functionality
- - For documentation files: Check accuracy and completeness
- - **If `--paper` was provided**:
- - Read the PDF using the Read tool
- - Compare implementation against algorithms/formulas in the paper
- - Note any deviations, simplifications, or potential errors
-
-3. **Generate review** following this template structure:
-
-```markdown
-# Code Review: PR #[number] - [title]
-
-**Author**: [author]
-**Branch**: [head] -> [base]
-**Files Changed**: [count]
-
----
-
-## Executive Summary
-
-[2-3 sentences describing what the PR does and your overall assessment]
-
----
-
-## Part 1: Methodology Review
-
-[For PRs with statistical/algorithmic changes, assess correctness]
-[If --paper was used, include comparison against the reference paper]
-[If not applicable, write "N/A - No methodology changes in this PR"]
-
----
-
-## Part 2: Issues Found
-
-### Critical Issues
-[Must fix before merge - security vulnerabilities, bugs, incorrect algorithms]
-
-### Medium Issues
-[Should fix - code quality, missing tests, documentation gaps]
-
-### Minor Issues
-[Nice to have - style suggestions, minor optimizations]
-
----
-
-## Part 3: Security Assessment
-
-[Assessment of security concerns, input validation, etc.]
-[If no security concerns, note "No security issues identified"]
-
----
-
-## Part 4: Documentation Assessment
-
-[Check docstrings, llms.txt for new public-API surfaces, API RST docs, references.rst for new citations, README only for landing-page-relevant changes]
-
----
-
-## Part 5: Performance Assessment
-
-[Computational complexity, memory usage, potential bottlenecks]
-
----
-
-## Part 6: Maintainability Assessment
-
-[Code duplication, backward compatibility, tech debt]
-
----
-
-## Recommendations
-
-### Must Fix (before merge)
-1. [item]
-
-### Should Fix
-1. [item]
-
-### Nice to Have
-1. [item]
-
----
-
-## Final Assessment
-
-| Category | Rating | Notes |
-|----------|--------|-------|
-| Methodology | [icon] | [brief note] |
-| Code Quality | [icon] | [brief note] |
-| Security | [icon] | [brief note] |
-| Documentation | [icon] | [brief note] |
-| Performance | [icon] | [brief note] |
-| Maintainability | [icon] | [brief note] |
-
-**Overall Verdict**: [Approved / Approved with Minor Changes / Needs Revision / Needs Major Revision]
-
----
-
-*Review generated by Claude Code*
-```
-
-Use these rating icons:
-- Pass/Good: checkmark
-- Warning/Minor issues: warning sign
-- Fail/Major issues: X mark
-
-4. **Save the review**:
- - Create `.claude/reviews/` directory if it doesn't exist (use Bash: `mkdir -p .claude/reviews`)
- - Write the review to `.claude/reviews/pr-{number}-review.md` using the Write tool
- - Overwrite any existing file (no confirmation needed)
-
-5. **Report to user**:
- - Show a summary of what was found
- - Tell them the file path
- - Instruct them to edit the file if needed, then run `/review-pr --post`
-
-### 3. Post Mode Workflow (when --post IS present)
-
-1. **Check for draft file**:
- - Try to read `.claude/reviews/pr-{number}-review.md`
- - If file doesn't exist, tell user to run without --post first to generate the draft
-
-2. **Post to PR**:
- - Use `mcp__github__add_issue_comment` to post the review content
- - The owner and repo should be determined from the current git repository (use `git remote get-url origin` and parse it)
-
-3. **Confirm success**:
- - Tell user the review was posted
- - Provide the PR URL
-
-## Output Examples
-
-### Draft Mode Output
-```
-Review generated for PR #74
-
-Draft saved to: .claude/reviews/pr-74-review.md
-
-Summary:
-- 5 files changed (3 source, 1 test, 1 docs)
-- Found 1 critical issue, 2 medium issues, 1 suggestion
-- Methodology: Correct
-- Overall: Needs Revision
-
-Edit the review file as needed, then run:
- /review-pr 74 --post
-```
-
-### Post Mode Output
-```
-Review posted to PR #74
-https://github.com/owner/repo/pull/74
-```
-
-### Error Output (no draft exists)
-```
-No review draft found for PR #74
-
-Run without --post first to generate the draft:
- /review-pr 74
-```
-
-## Notes
-
-- Reviews are saved as markdown files for easy editing before posting
-- The --post flag reads from the saved file, preserving any user edits
-- Draft mode always overwrites any existing draft for that PR number
-- The skill only reviews PRs from the current repository (no cross-repo support)
-- For methodology validation, the --paper flag accepts PDF files which Claude can read directly
diff --git a/.claude/commands/submit-pr.md b/.claude/commands/submit-pr.md
index d71b8807e..ef52fd5b8 100644
--- a/.claude/commands/submit-pr.md
+++ b/.claude/commands/submit-pr.md
@@ -17,114 +17,191 @@ Commit work, push to a new branch, and open a pull request with the project-spec
## Instructions
-### 1. Parse Arguments
+### 1. Parse Arguments and Resolve Safe Values
+
+Parse `$ARGUMENTS` to extract the raw **title** (optional), optional **--branch**,
+optional **--base** (default `main`), and the **--draft** flag. Determine a **change
+type** (`feature`/`fix`/`refactor`/`docs`) from a quick `git diff --stat` /
+`git status --porcelain`.
+
+**Title when none was given.** `/submit-pr` with no title is supported. If you want a
+descriptive title, derive it from **base-free** commands only — `git diff --cached
+--stat`, `git status --porcelain`, `git log -1 --format=%s` — and write it in step 1b.
+**Never build a title with `git log ..HEAD`**: at this point `` is the raw,
+unvalidated `--base`, and a malicious value would execute on that command line before
+the script ever sees it. If you write no title, `pr_prepare.py` generates a base-free
+fallback (the last commit subject), so the no-title path is safe either way.
+
+**The raw title/branch/base must never touch a shell — not even a `VAR="…"`
+assignment.** A backtick or `$(...)` in the title executes *at the assignment*,
+before any script runs (`RAW_TITLE="Fix `` `id` `` "` fires `id`). So materialise the
+untrusted values with the **Write tool** (which never invokes a shell) into files,
+then pass the *file paths* — which you control and which are metacharacter-free — to
+`.claude/scripts/pr_prepare.py`. That script is the single ingress that resolves and
+validates every dynamic value; it is unit-tested (`tests/test_pr_prepare.py`),
+including an end-to-end subprocess test that runs it with a `$(touch sentinel)` title
+and asserts nothing executes.
+
+**Do the three sub-steps in this exact order — they are not one shell block.**
+
+**1a. Derive and clear the scratch directory** (one Bash call). Use a *deterministic*
+path, not `mktemp -d`: shell variables do not persist across separate Bash tool calls,
+so a random path captured here is gone in the next call. `git rev-parse --git-path`
+recomputes the same path every call (and is correct inside worktrees). Clearing it
+first prevents a prior interrupted run's stale `raw-branch.txt`/`raw-base.txt` from
+being read as fresh explicit flags:
-Parse `$ARGUMENTS` to extract:
-- **title**: Everything before any `--` flags
-- **--branch**: Branch name (if provided)
-- **--base**: Base branch (default: `main`)
-- **--draft**: Boolean flag
+```bash
+SCRATCH="$(git rev-parse --git-path pr-prepare)" # e.g. .git/pr-prepare
+rm -rf "$SCRATCH" && mkdir -p "$SCRATCH"
+echo "$SCRATCH" # note the literal path for 1b
+```
-### 2. Detect Remote Configuration
+**1b. Write the raw values with the Write tool** (NOT `echo`/heredoc/`printf` — those
+re-invoke the shell on the content) into the literal path from 1a. Only write a file
+for a value that exists:
+- `/raw-title.txt` ← the title, only if the user gave one *or* you generated
+ a base-free one; otherwise omit it and the script generates a fallback
+- `/raw-branch.txt` ← the explicit `--branch`, only if one was given
+- `/raw-base.txt` ← the explicit `--base`, only if one was given
-Determine if this is a fork-based workflow:
+**1c. Run the script** (one Bash call — re-derive `SCRATCH`, do NOT clear it again).
+**Substitute the change type and draft flag as literals** — they are trusted values
+you determined during parsing (a fixed enum and a boolean), and shell variables set in
+an earlier tool call are not defined here. Write the literal `--change-type `,
+and include the literal `--draft` line only for a draft PR:
-1. **Check for upstream remote**:
- ```bash
- git remote get-url upstream 2>/dev/null
- ```
+```bash
+SCRATCH="$(git rev-parse --git-path pr-prepare)"
+python3 .claude/scripts/pr_prepare.py \
+ --title-file "$SCRATCH/raw-title.txt" \
+ --branch-file "$SCRATCH/raw-branch.txt" \
+ --base-file "$SCRATCH/raw-base.txt" \
+ --current-branch "$(git branch --show-current)" \
+ --change-type fix \
+ --draft \
+ --scratch "$SCRATCH"
+```
-2. **Set remote variables**:
- - If `upstream` exists → **fork workflow**:
- - `` = `upstream` (for base comparisons, PR target)
- - `` = `origin` (for pushing branches)
- - Extract `/` from upstream URL
- - Extract `` from origin URL
- - If `upstream` does not exist → **direct workflow**:
- - `` = `origin`
- - `` = `origin`
- - Extract `/` from origin URL
-
-3. **Fetch from base remote**:
- ```bash
- git fetch
- ```
+Replace `fix` with the resolved change type (`feature`/`fix`/`refactor`/`docs`), and
+**drop the `--draft` line entirely** when the user did not pass `--draft`. A
+missing/empty `--title-file`/`--branch-file`/`--base-file` means "not supplied".
+`--current-branch` is trusted git output; the script still validates it.
+
+If it exits non-zero it has **rejected** an unsafe or conflicting explicit
+`--branch`/`--base`, or a fork remote that would not parse (it fails closed) — surface
+its message and stop.
+
+**Because variables do not persist across tool calls, every later step re-derives
+`SCRATCH="$(git rev-parse --git-path pr-prepare)"` and re-reads the value files it
+needs** (`BASE_BRANCH="$(cat "$SCRATCH/pr-base.txt")"`, etc.) inside that same call.
+Do not assume a variable set in an earlier step is still defined.
+
+**Cleanup on every exit.** A `trap … EXIT` cannot span tool calls, so make it a rule:
+if you abort at any later step (rejection here, the user cancels the sync in step 3, a
+commit or push fails), re-derive `$SCRATCH` and `rm -rf` it before stopping. Step 10
+removes it on the success path.
+
+The script wrote one value per file under `$SCRATCH`:
+`pr-base.txt`, `pr-branch.txt`, `pr-headref.txt`, `pr-target-repo.txt`,
+`pr-is-fork.txt`, `pr-draft.txt`, `pr-title.txt`. **Re-read the ones a step needs at
+the top of that step** (variables do not carry over), always quoted:
+
+```bash
+SCRATCH="$(git rev-parse --git-path pr-prepare)"
+BASE_BRANCH="$(cat "$SCRATCH/pr-base.txt")"
+BRANCH_NAME="$(cat "$SCRATCH/pr-branch.txt")"
+```
+
+Every branch/base value matches `[A-Za-z0-9._/-]` and passed `git check-ref-format`;
+the title is the opaque file. None can carry a shell metacharacter.
+
+### 2. Resolve Remotes
+
+Remote *names* are fixed literals (`origin`/`upstream`), not user input, so they are
+safe to set in prose:
+
+- `IS_FORK == true` → **fork workflow**: `BASE_REMOTE=upstream`, `PUSH_REMOTE=origin`.
+ `TARGET_REPO` and `HEAD_REF` (`fork-owner:branch`) were already resolved by the
+ script from the remote URLs.
+- `IS_FORK == false` → **direct workflow**: `BASE_REMOTE=origin`, `PUSH_REMOTE=origin`.
+ `gh` infers the target repo; `HEAD_REF` is just `BRANCH_NAME`.
+
+Then fetch: `git fetch "$BASE_REMOTE"`.
### 3. Sync with Remote
-1. **Check if behind base branch**:
+1. **Check if behind base branch** (quoted variables throughout):
```bash
- git rev-list --count HEAD../
+ git rev-list --count "HEAD..$BASE_REMOTE/$BASE_BRANCH"
```
- If count > 0, we're behind. Warn user and offer options:
```
- Your branch is X commits behind /.
+ Your branch is X commits behind $BASE_REMOTE/$BASE_BRANCH.
Options:
- 1. Rebase first: git pull --rebase
+ 1. Rebase first: git pull --rebase "$BASE_REMOTE" "$BASE_BRANCH"
2. Continue anyway (may have merge conflicts in PR)
```
- Use AskUserQuestion to let user choose whether to continue or abort
-### 4. Check for Changes
+### 4. Reconcile branch state (idempotent)
-1. **Check for uncommitted changes**:
- ```bash
- git status --porcelain
- ```
- - If output is non-empty, there are staged or unstaged changes → proceed to step 5
+**submit-pr's goal is: this committed branch is pushed and has an open PR.** It is
+idempotent — safe to run whether or not the work is already committed or already
+pushed. The standard flow commits at `/ai-review-local` (which requires a commit) and
+runs `/pre-merge-check` before this, so by here the work is normally *already
+committed*; and a rebase/prep step may already have *pushed* it. The terminal state is
+**"a PR exists,"** not "there was something to push" — so a fully-committed,
+already-pushed branch still proceeds to open its PR (step 10), never dead-ends.
-2. **Check for unpushed commits** (if no uncommitted changes):
- ```bash
- git rev-list --count /..HEAD
- ```
- - If count > 0, there are unpushed commits → skip to step 7
- - If count == 0, inform user and exit:
+1. **Uncommitted changes?** `git status --porcelain`
+ - **Non-empty** → these have NOT been through `/ai-review-local`. Warn and
+ AskUserQuestion:
```
- No changes detected. Your working directory is clean and up-to-date with /.
- Nothing to submit.
+ N uncommitted file(s) have not been through /ai-review-local.
+ 1. Commit them anyway (goes in UNREVIEWED — bypasses the review step)
+ 2. Abort - commit and run /ai-review-local first
```
+ On option 1, do steps 5, 5b, 6 (create branch if needed, stage, commit). On
+ option 2, stop.
+ - **Empty** → continue to the zero-diff guard below.
-### 5. Resolve Branch Name (BEFORE any commits)
-
-**IMPORTANT**: Always resolve the branch name before staging or committing to avoid commits on the base branch.
-
-1. **Check current branch**:
+2. **Zero-diff guard — is there anything to submit at all?**
```bash
- git branch --show-current
+ git rev-list --count "$BASE_REMOTE/$BASE_BRANCH..HEAD"
```
+ - **0 commits ahead of base** (e.g. a clean `main` that equals `origin/main`):
+ there is genuinely nothing to submit. Check for an existing open PR (step 10's
+ query) — if one exists, report its URL; if not, exit with **"Nothing to submit —
+ no commits ahead of `$BASE_BRANCH`."** Do NOT create or push a branch (that would
+ leave remote clutter, then `gh pr create` would fail on an empty diff).
+ - **> 0 commits ahead** → real work exists. Continue to step 7 (push-if-needed) then
+ step 10 (ensure a PR exists). An already-*pushed* branch with commits and no PR
+ still proceeds — that is the idempotent path.
+
+### 5. Create the Branch (BEFORE any commits)
+
+`BRANCH_NAME` was already resolved and safety-validated by the script in step 1 —
+whitelist-sanitised if generated, or rejected outright if an unsafe explicit
+`--branch` was given. There is **no sanitisation to do here**; just use the quoted
+variable.
+
+1. **Check current branch**: `CURRENT="$(git branch --show-current)"`. This is empty
+ on a **detached HEAD**.
+
+2. **If on the base branch, OR detached** (`CURRENT` empty or equal to `$BASE_BRANCH`),
+ create and switch before staging, so no commit lands on the base or on a detached
+ HEAD:
+ ```bash
+ git checkout -b "$BRANCH_NAME"
+ ```
+ Detached HEAD must take this path: otherwise a commit lands on no branch, and step 7
+ would try to push a generated ref that was never created. (`$BRANCH_NAME` from step 1
+ is already the generated name in the detached case, since the script saw an empty
+ `--current-branch`.)
-2. **If on base branch (e.g., `main`)**:
- - Generate or use provided branch name
- - **Generate branch name** (if not provided via `--branch`):
- - Analyze changes to understand the change type:
- ```bash
- git diff --stat # Unstaged changes
- git diff --cached --stat # Staged changes
- git status --porcelain # All changes summary
- ```
- - **Sanitize the branch name** (from title or generated):
- 1. Lowercase the string
- 2. Replace spaces with hyphens
- 3. Remove invalid git ref characters: `:`, `?`, `*`, `[`, `]`, `^`, `~`, `\`, `@{`, `..`
- 4. Replace consecutive hyphens/underscores with single hyphen
- 5. Trim leading/trailing hyphens
- 6. Truncate to reasonable length (50 chars max for branch name portion)
- - Prefix based on change type: `feature/`, `fix/`, `refactor/`, `docs/`
- - **Validate with git**:
- ```bash
- git check-ref-format --branch ""
- ```
- - If validation fails, prompt user for a valid branch name
- - If no diff output and no title provided, prompt user for branch name
- - **Create and switch to the new branch BEFORE staging**:
- ```bash
- git checkout -b
- ```
-
-3. **If already on a feature branch**:
- - Use the current branch name
- - No need to create a new branch
+3. **Otherwise** (already on a feature branch), use it as-is — no new branch needed.
### 5b. Stage and Quick Pattern Check
@@ -133,17 +210,17 @@ Determine if this is a fork-based workflow:
git add -A
```
-2. **Quick pattern check** (if methodology files are staged):
+2. **Quick pattern check** — run the argv-safe helper, never a shell grep over
+ filenames (a changed path like `diff_diff/$(touch x).py` would execute):
```bash
- git diff --cached --name-only | grep "^diff_diff/.*\.py$" | grep -v "__init__"
+ SCRATCH="$(git rev-parse --git-path premerge-scan)"; mkdir -p "$SCRATCH"
+ python3 .claude/scripts/premerge_scan.py --scratch "$SCRATCH"
```
-
- If methodology files are present:
- 1. Read `/pre-merge-check` Section 2.1 for pattern check definitions.
- 2. Run **all four pattern checks (A through D)** on the staged methodology files.
- 3. For any matches, display the file:line and flag message from that section.
-
- If warnings are found:
+ It runs the methodology pattern checks (A–D) in pure Python over file content;
+ **exit 3** = a changed path carries shell metacharacters, **exit 4** = a git/read
+ failure (the scan is incomplete — **stop and report**, do not commit on an empty
+ scan). See `/pre-merge-check` Section 2.1 for the shared definition. If it reports
+ findings:
```
Pre-commit pattern check found N potential issues:
@@ -210,15 +287,22 @@ Determine if this is a fork-based workflow:
- Run `git diff --cached --stat` to see what's being committed
- Analyze the changes and generate a descriptive commit message
- Use imperative mood ("Add", "Fix", "Update", "Refactor")
- - Format with HEREDOC:
+ - **Write the message to a file with the Write tool, then `git commit --file` —
+ never a heredoc.** A `git commit -m "$(cat <<'EOF' … EOF)"` heredoc breaks if the
+ message body contains a line that is exactly `EOF`: the heredoc closes early and
+ the following lines execute as shell. This is the same shell-ingress class
+ `pr_prepare.py` exists to prevent; the Write tool never invokes a shell, and
+ `git commit --file` reads the file verbatim. Write the message to the literal
+ scratch path (the one printed in step 1a), then commit — **re-deriving `SCRATCH`
+ in this block**, since it does not carry over from an earlier tool call:
```bash
- git commit -m "$(cat <<'EOF'
-
-
- Co-Authored-By: Claude Opus 4.5
- EOF
- )"
+ SCRATCH="$(git rev-parse --git-path pr-prepare)"
+ # write the message to "$SCRATCH/commit-msg.txt" with the Write tool first
+ git commit --file "$SCRATCH/commit-msg.txt"
```
+ Do NOT append `Co-Authored-By`, `Claude-Session`, "Generated with Claude
+ Code", or any other authorship trailer. The commit message describes the
+ change, not who typed it.
### 7. Push Branch to Remote
@@ -227,37 +311,43 @@ Determine if this is a fork-based workflow:
git branch --show-current
```
-2. **Guard: Prevent pushing from base branch**:
- - If current branch equals `` (e.g., `main`):
- - This can happen when step 4 skipped to step 7 due to unpushed commits on base
- - **Must create a new branch before proceeding**:
- - Generate branch name from unpushed commits (analyze `git log /..HEAD`)
- - Use provided `--branch` name if available
- - Create and switch:
- ```bash
- git checkout -b
- ```
+2. **Guard: Prevent pushing from base branch or detached HEAD**:
+ - If the current branch is empty (detached HEAD) **or** equals `"$BASE_BRANCH"`:
+ - This can happen when step 4 skipped to step 7 due to unpushed commits on base,
+ or when HEAD was detached
+ - **Must switch to `$BRANCH_NAME` before proceeding** (already resolved safely
+ in step 1):
+ ```bash
+ git checkout -b "$BRANCH_NAME"
+ ```
- If branch creation fails or is declined, abort with error:
```
Error: Cannot create PR from base branch to itself.
Please create a feature branch first or provide --branch .
```
-3. **Push to push-remote** (always `origin`, even in fork workflows):
+3. **Push to push-remote only if needed** (always `origin`, even in fork workflows).
+ Push when there are unpushed commits or no upstream; it's a **no-op when the branch
+ is already fully pushed** — that is not an error, just continue to step 10:
```bash
- git push -u
+ if [ -z "$(git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null)" ]; then
+ git push -u "$PUSH_REMOTE" "$BRANCH_NAME" # no upstream yet
+ elif [ "$(git rev-list --count @{u}..HEAD)" -gt 0 ]; then
+ git push "$PUSH_REMOTE" "$BRANCH_NAME" # unpushed commits
+ fi
+ # else: already fully pushed — nothing to push, proceed to step 10.
```
### 8. Extract Commit Information for PR Body
1. Get commits on this branch (compare against base-remote to avoid stale data):
```bash
- git log /..HEAD --oneline
+ git log "$BASE_REMOTE/$BASE_BRANCH..HEAD" --oneline
```
2. Get changed files:
```bash
- git diff /..HEAD --stat
+ git diff "$BASE_REMOTE/$BASE_BRANCH..HEAD" --stat
```
3. Categorize changes for the template:
@@ -284,92 +374,136 @@ Fill in the template:
## Security / privacy
- Confirm no secrets/PII in this PR: Yes
-
----
-Generated with Claude Code
```
+Do not add an authorship footer to the PR body.
+
**Template logic:**
- **Methodology**: Mark "N/A" only if NO files changed in `diff_diff/`, `rust/src/`, or `docs/methodology/`. If methodology files changed, consult `docs/methodology/REGISTRY.md` for proper citations.
- **Validation**: List `test_*.py` files changed, note tutorial updates
- **Security**: Default "Yes", but warn if `.env`, credentials, or API key patterns detected
-### 10. Create Pull Request
+### 10. Ensure a PR exists
-Use the MCP GitHub tool to create the PR:
+The terminal state is "an open PR exists for this branch **that matches what was
+requested**," so **first check whether one already does** — this is what makes
+submit-pr idempotent and safe to re-run. Fetch enough to compare, not just the URL:
-```
-mcp__github__create_pull_request with parameters:
- - owner: # upstream-owner (fork) or owner (direct)
- - repo: # upstream-repo (fork) or repo (direct)
- - title:
- - head: # See below for fork vs direct
- - base:
- - body:
- - draft:
+```bash
+gh pr view --json url,state,baseRefName,isDraft \
+ --jq 'select(.state=="OPEN") | "\(.url)\t\(.baseRefName)\t\(.isDraft)"' 2>/dev/null
```
-**Head reference format**:
-- **Direct workflow**: `head: `
-- **Fork workflow**: `head: :`
+- **An open PR exists and its `baseRefName` == `$BASE_BRANCH` and its `isDraft`
+ matches the requested draft state** → report its URL; you are done. Do not open a
+ second one.
+- **An open PR exists but the base or draft state differs from what was requested**
+ → do NOT silently treat it as success. Report the mismatch (e.g. "an open PR for
+ this branch already targets `main`, but you asked for `--base develop`") and let the
+ user decide — retarget, or accept the existing PR.
+- **No open PR** → create it, as below.
-**Extract remote info**:
-```bash
-# For target (where PR is created)
-git remote get-url
-# Parse: git@github.com:owner/repo.git or https://github.com/owner/repo.git
+(Fork workflows: pass the target repo explicitly to `gh pr view --repo "$TARGET_REPO"`,
+since a bare `gh` resolves to the fork.)
-# For fork owner (if fork workflow)
-git remote get-url origin
-# Extract owner from URL
-```
+All dynamic values were resolved and safety-checked by the script in step 1 and are
+already loaded into `TITLE_FILE`/`BASE_BRANCH`/`BRANCH_NAME`/`HEAD_REF`/`TARGET_REPO`.
+There is **no sanitisation, no remote-URL parsing, and no placeholder substitution
+to do here** — that logic lives in `pr_prepare.py` where it is unit-tested. This step
+only writes the body and invokes `gh` with quoted variables.
-### 10b. Ensure PR ref exists
+1. **Write the body with the Write tool** (not a shell heredoc) to `$SCRATCH/pr-body.md`.
+ Using the tool means the body is never parsed by a shell and cannot collide with a
+ heredoc delimiter. `pr_prepare.py` already wrote `$SCRATCH/pr-title.txt`.
-After PR creation, GitHub may not immediately create `refs/pull//head` (especially
-when the PR is created via API). Push a no-op to the branch to force ref creation:
+2. **Invoke `gh`, re-deriving everything in this one call** (nothing persisted from
+ earlier steps), everything quoted. Draft state comes from the file the script
+ wrote, so a `--draft` request cannot be silently dropped:
-```bash
-git push
-```
+ ```bash
+ SCRATCH="$(git rev-parse --git-path pr-prepare)"
+ TITLE="$(cat "$SCRATCH/pr-title.txt")"
+ BODY_FILE="$SCRATCH/pr-body.md"
+ BASE_BRANCH="$(cat "$SCRATCH/pr-base.txt")"
+ HEAD_REF="$(cat "$SCRATCH/pr-headref.txt")"
+ TARGET_REPO="$(cat "$SCRATCH/pr-target-repo.txt")"
+ IS_FORK="$(cat "$SCRATCH/pr-is-fork.txt")"
+ DRAFT=(); [ "$(cat "$SCRATCH/pr-draft.txt")" = "true" ] && DRAFT=(--draft)
+
+ if [ "$IS_FORK" = "true" ]; then
+ gh pr create \
+ --repo "$TARGET_REPO" --head "$HEAD_REF" --base "$BASE_BRANCH" \
+ --title "$TITLE" --body-file "$BODY_FILE" "${DRAFT[@]}"
+ else
+ # gh infers the target repo from the remotes in the direct workflow.
+ gh pr create \
+ --base "$BASE_BRANCH" --title "$TITLE" --body-file "$BODY_FILE" "${DRAFT[@]}"
+ fi
+ ```
-Then verify:
-```bash
-git ls-remote refs/pull//head
-```
+ `DRAFT` is a bash array so an empty value expands to *no argument* (not an empty
+ string), which the earlier `${DRAFT:+…}` form got wrong under zsh.
-If still missing, push an empty commit:
-```bash
-git commit --allow-empty -m "Trigger PR ref creation"
-git push
-```
+3. **Clean up**, on success and failure alike: `rm -rf "$SCRATCH"`.
+
+`gh pr create` prints the PR URL on success — capture it for step 11 rather than
+reconstructing it. If it fails, surface the error verbatim; do not retry silently.
+
+**Regression coverage.** The safety property — no title/base/branch value can execute
+regardless of backticks or `$()` — is enforced by `tests/test_pr_prepare.py`
+(including a payload that would `touch` a sentinel), not by prose. If you change how
+values reach the shell here, that suite must still pass.
+
+### 10b. Do NOT force PR ref creation
+
+There is deliberately no step here that pushes again, verifies
+`refs/pull//head`, or creates an empty commit. An earlier version did, and
+it was wrong three ways:
+
+1. It probed `PUSH_REMOTE` for the ref. In a fork workflow that is the fork, while
+ `refs/pull//head` only ever exists in the **base** repository — so the
+ check came back empty even on success, every time.
+2. The fallback then committed `Trigger PR ref creation` and pushed it, putting a
+ junk empty commit in the history of every fork PR.
+3. That extra push re-triggers the CI AI reviewer, which already started on PR open.
+
+The workaround existed for PRs created through the raw API. `gh pr create` uses the
+normal path and GitHub creates the ref itself. If a ref genuinely appears missing,
+report it — do not modify the branch to provoke it.
### 11. Report Results
```
Pull request created successfully!
-Branch:
-PR: # -
-URL: https://github.com/<target-owner>/<target-repo>/pull/<number>
+Branch: $BRANCH_NAME
+PR: <the URL gh printed>
Changes included:
<list of changed files>
Next steps:
- Review the PR at the URL above
-- AI code review runs automatically on PR open
-- When AI review is green, add the `ready-for-ci` label to trigger CI tests
-- Run /review-pr <number> to get AI review
+- <AI review line — see below, depends on direct vs fork>
+- CI tests require the `ready-for-ci` label, which the user adds (never Claude)
```
+**The AI review line is conditional.** `.github/workflows/ai_pr_review.yml` runs
+`pull_request` reviews only when `head.repo.full_name == github.repository`, so fork
+PRs are skipped at the workflow level as an untrusted-checkout guard.
+
+- **Direct workflow**: `AI code review starts automatically on PR open — do NOT post /ai-review`
+- **Fork workflow**: `The CI AI reviewer is security-gated and will NOT run on fork PRs. Use /ai-review-local instead.`
+
+Telling a fork contributor to wait for a review that cannot appear is worse than
+telling them nothing.
+
## Error Handling
-### No Changes to Commit
-```
-No changes detected. Your working directory is clean.
-Nothing to submit.
-```
+### Nothing to do (idempotent success)
+A clean, fully-pushed branch that already has an open PR is not an error — report the
+existing PR URL. submit-pr never dead-ends with "nothing to submit" on a committed
+branch: if no PR exists yet it opens one (step 10), even when there is nothing to push.
### Branch Already Exists
```
@@ -402,6 +536,7 @@ Show the error and provide manual fallback commands.
- Always stages ALL changes (`git add -A`). Stage manually first for partial commits.
- Branch names auto-prefixed: feature/, fix/, refactor/, docs/
-- Uses MCP GitHub server for PR creation (requires PAT with repo access)
+- Uses the `gh` CLI for PR creation (requires `gh auth login`). The GitHub MCP
+ server is NOT used anywhere in this repo — `gh` is the only supported path.
- Git push uses SSH or HTTPS based on remote URL configuration
- **Fork workflows supported**: If `upstream` remote exists, PRs target upstream with `<fork-owner>:<branch>` head reference
diff --git a/.claude/commands/validate-tutorial.md b/.claude/commands/validate-tutorial.md
deleted file mode 100644
index ce60de2f7..000000000
--- a/.claude/commands/validate-tutorial.md
+++ /dev/null
@@ -1,78 +0,0 @@
----
-description: Validate that tutorial Jupyter notebooks execute without errors
-argument-hint: "[all | 01-10 | notebook-name]"
----
-
-# Validate Tutorial Notebooks
-
-Validate that tutorial Jupyter notebooks execute without errors.
-
-## Arguments
-
-The user may provide an optional argument: `$ARGUMENTS`
-
-- If empty or not provided: Ask the user which notebook(s) to validate
-- If "all": Validate all notebooks in `docs/tutorials/`
-- If a number (e.g., "01", "1", "10"): Validate that specific notebook
-- If a name (e.g., "basic_did", "trop"): Validate the matching notebook
-
-## Available Notebooks
-
-```
-01_basic_did.ipynb - Basic 2x2 DiD
-02_staggered_did.ipynb - Callaway-Sant'Anna staggered adoption
-03_synthetic_did.ipynb - Synthetic DiD
-04_parallel_trends.ipynb - Parallel trends testing
-05_honest_did.ipynb - Honest DiD sensitivity analysis
-06_power_analysis.ipynb - Power analysis for study design
-07_pretrends_power.ipynb - Pre-trends power analysis
-08_triple_diff.ipynb - Triple Difference (DDD)
-09_real_world_examples.ipynb - Real-world datasets
-10_trop.ipynb - TROP estimator
-```
-
-## Instructions
-
-1. **Parse the argument** to determine which notebook(s) to run:
- - If no argument provided, use AskUserQuestion to let user select:
- - Option 1: "All notebooks"
- - Option 2: "Select specific notebook" (then show numbered list)
- - If "all", run all notebooks
- - If a number, find the matching notebook (e.g., "01" or "1" matches "01_basic_did.ipynb")
- - If a partial name, find the matching notebook (e.g., "trop" matches "10_trop.ipynb")
-
-2. **Execute each selected notebook** using:
- ```bash
- jupyter nbconvert --to notebook --execute --inplace "docs/tutorials/NOTEBOOK.ipynb" 2>&1
- ```
-
- Or if nbconvert is not available, try:
- ```bash
- python -m jupyter nbconvert --to notebook --execute --inplace "docs/tutorials/NOTEBOOK.ipynb" 2>&1
- ```
-
-3. **Report results** for each notebook:
- - Success: "01_basic_did.ipynb - PASSED"
- - Failure: Show the error message and which cell failed
-
-4. **Clear outputs and metadata** after validation completes (regardless of pass/fail):
- ```bash
- jupyter nbconvert --ClearOutputPreprocessor.enabled=True --ClearMetadataPreprocessor.enabled=True --inplace "docs/tutorials/NOTEBOOK.ipynb"
- ```
-
- This ensures notebook outputs and execution metadata (timestamps, execution counts) are not committed to git.
-
-5. **Summary**: After all notebooks complete, show a summary like:
- ```
- Validation complete: 8/10 passed, 2 failed
- Failed: 05_honest_did.ipynb, 09_real_world_examples.ipynb
-
- All notebook outputs have been cleared.
- ```
-
-## Notes
-
-- Notebooks are executed to validate they work, then outputs and metadata are cleared
-- If a notebook fails, continue to the next one (don't stop early)
-- The working directory should be the project root
-- Some notebooks may take a while to run (especially those with bootstrap)
diff --git a/.claude/commands/worktree-new.md b/.claude/commands/worktree-new.md
index 093d3837b..cba306531 100644
--- a/.claude/commands/worktree-new.md
+++ b/.claude/commands/worktree-new.md
@@ -81,14 +81,135 @@ git worktree list
### 4. Create the Worktree
+**If base-ref was provided**, use it directly — the user stated their intent:
+
```bash
-# If base-ref provided (create new branch <name> starting at base-ref):
git worktree add -b "<name>" -- "$WORKTREE_PATH" "$BASE_REF"
+```
+
+**If no base-ref was provided**, do NOT silently branch from wherever HEAD happens
+to be. Resolve what the base would actually be first:
-# If no base-ref (create new branch from current HEAD):
-git worktree add -b "<name>" -- "$WORKTREE_PATH"
+```bash
+CURRENT_REF="$(git rev-parse --abbrev-ref HEAD)"
+# Base remote: upstream when this is a fork checkout, else origin — same resolution
+# /submit-pr uses. In the common (no-upstream) case this is just origin.
+BASE_REMOTE="$(git remote get-url upstream >/dev/null 2>&1 && echo upstream || echo origin)"
+# Resolve the remote's default branch from the remote itself. Do NOT use
+# `gh repo view "$BASE_REMOTE"` — gh treats the argument as an owner/repo, not a
+# remote alias, so it errors ("Could not resolve to a Repository with the name
+# '<you>/origin'") and silently falls back to `main`, breaking any repo whose
+# default is `master`/`develop`.
+DEFAULT_BRANCH="$(git ls-remote --symref "$BASE_REMOTE" HEAD 2>/dev/null \
+ | awk '/^ref:/ {sub("refs/heads/","",$2); print $2; exit}')"
+
+# Offline / remote unavailable? Fall back to the LOCAL record of the remote's HEAD
+# rather than inventing `main` — assuming `main` on a `develop`/`master` repo would
+# branch from the wrong ancestry.
+if [ -z "$DEFAULT_BRANCH" ]; then
+ DEFAULT_BRANCH="$(git symbolic-ref --quiet --short "refs/remotes/$BASE_REMOTE/HEAD" 2>/dev/null \
+ | sed "s#^$BASE_REMOTE/##")"
+fi
```
+If `$DEFAULT_BRANCH` is still empty, **do not guess `main`** — ask the user for the
+base branch (or to pass an explicit base-ref) and stop. A wrong default silently
+poisons every worktree spun off it.
+
+- If `$CURRENT_REF` equals `$DEFAULT_BRANCH`, check the base is actually current
+ before branching from it. A worktree spun off a stale local default branch means a
+ rebase later and phantom review findings in between:
+
+ Measure **both** directions. Behind-only is the obvious case, but ahead matters
+ just as much: local commits sitting unpushed on the default branch would be
+ inherited by the new worktree and end up contaminating an unrelated PR.
+
+ Distinguish "verified in sync" from "could not verify" — a failed fetch or a missing
+ tracking ref must NOT masquerade as "in sync", or a stale/divergent base slips
+ through silently:
+
+ ```bash
+ FRESH_OK=1
+ git fetch "$BASE_REMOTE" "$DEFAULT_BRANCH" --quiet 2>/dev/null || FRESH_OK=0
+ # Track ref EXISTENCE separately from freshness: a stale-but-present ref can be
+ # offered as a fallback; an absent ref cannot (branching from it just fails).
+ REF_EXISTS=1
+ git rev-parse --verify --quiet "$BASE_REMOTE/$DEFAULT_BRANCH" >/dev/null || { REF_EXISTS=0; FRESH_OK=0; }
+ # A failed rev-list must lower FRESH_OK — NOT silently become 0 ahead/behind (which
+ # would read as "verified in sync"). Propagate the failure, then default the value.
+ BEHIND=$(git rev-list --count "HEAD..$BASE_REMOTE/$DEFAULT_BRANCH" 2>/dev/null) || FRESH_OK=0
+ AHEAD=$(git rev-list --count "$BASE_REMOTE/$DEFAULT_BRANCH..HEAD" 2>/dev/null) || FRESH_OK=0
+ : "${BEHIND:=0}"; : "${AHEAD:=0}"
+ ```
+
+ - **`FRESH_OK=1` and `BEHIND == 0` and `AHEAD == 0`** — verified in sync:
+ **proceed silently, no prompt.** This is the common path and must stay frictionless.
+ ```bash
+ git worktree add -b "<name>" -- "$WORKTREE_PATH"
+ ```
+
+ - **`FRESH_OK=0`** — freshness could not be verified (offline, fetch failed, or the
+ tracking ref is absent). Do not assume sync. Surface it — and **only offer the
+ stale-tracking-ref option when `REF_EXISTS=1`**; when the ref is absent that option
+ would just fail:
+ ```
+ Could not verify <default-branch> against <base-remote>.
+ Branching from local <default-branch> may inherit stale or local-only commits.
+
+ Options:
+ 1. Branch from <base-remote>/<default-branch> anyway (may be stale) — ONLY if REF_EXISTS=1
+ 2. Branch from local <default-branch>
+ 3. Abort - I'll fetch first
+ ```
+
+ - **Otherwise** — verified but behind, ahead, or diverged — surface it before
+ creating anything. State whichever applies:
+ ```
+ Local <default-branch> and <base-remote>/<default-branch> differ:
+ behind: <BEHIND> commit(s) ahead: <AHEAD> commit(s)
+
+ Behind means you will rebase later. Ahead means those local-only commits
+ become part of the new branch.
+
+ Options:
+ 1. Branch from <base-remote>/<default-branch> (recommended)
+ 2. Branch from local <default-branch> anyway
+ 3. Abort
+ ```
+ Option 1 uses `git worktree add -b "<name>" -- "$WORKTREE_PATH" "$BASE_REMOTE/$DEFAULT_BRANCH"`;
+ option 2 uses the plain form above.
+
+ Never fetch-and-reset the user's checked-out default branch — this reads
+ `$BASE_REMOTE/*` and branches from it. The branch they are standing on is left alone.
+
+- If `$CURRENT_REF` is anything else, the new branch would inherit that branch's
+ commits. That is occasionally intended (stacking work) and usually not. **Surface it
+ with AskUserQuestion before creating anything:**
+
+ ```
+ No base branch given, and HEAD is on '<current-ref>', not '<default-branch>'.
+ A new worktree created now would inherit every commit on '<current-ref>'.
+
+ Options:
+ 1. Branch from <base-remote>/<default-branch> (recommended)
+ 2. Branch from current HEAD (<current-ref>) - stack on this work
+ 3. Abort - let me specify a base
+ ```
+
+ For option 1, the worktree add must be **conditional on the fetch succeeding** —
+ otherwise a failed fetch silently branches from a stale `$BASE_REMOTE/$DEFAULT_BRANCH`,
+ breaking the promise that the base is current:
+ ```bash
+ if git fetch "$BASE_REMOTE" "$DEFAULT_BRANCH" --quiet; then
+ git worktree add -b "<name>" -- "$WORKTREE_PATH" "$BASE_REMOTE/$DEFAULT_BRANCH"
+ else
+ echo "Fetch of $BASE_REMOTE/$DEFAULT_BRANCH failed; its tracking ref may be stale."
+ # Ask: branch from the (possibly stale) tracking ref, branch from current HEAD,
+ # or abort and fetch manually. Do NOT create the worktree on a stale base silently.
+ fi
+ ```
+ For option 2, use the no-base-ref form above. For option 3, stop.
+
### 5. Set Up Python Environment
Note to user: dependency installation may take a moment on a fresh venv.
@@ -104,16 +225,25 @@ Do NOT use `-q` — let pip output stream so the user sees progress.
### 6. Verify Rust Backend Rebuild Capability (best-effort)
Step 5 already compiled the Rust extension via maturin (the build backend).
-This step verifies that maturin is available in the venv for **future rebuilds**
-after Rust code changes. Use `--manifest-path` to avoid changing directories:
+This step verifies maturin can rebuild into **this worktree's** venv after future
+Rust changes.
+
+**`maturin develop` installs into the venv it detects from `VIRTUAL_ENV` / the
+current directory — NOT the venv implied by `--manifest-path`.** Passing only
+`--manifest-path` from another directory builds this worktree's Rust code into
+whatever venv happens to be active, silently. Pin both:
```bash
-"$WORKTREE_PATH/.venv/bin/maturin" develop --manifest-path "$WORKTREE_PATH/rust/Cargo.toml"
+(cd "$WORKTREE_PATH" && VIRTUAL_ENV="$WORKTREE_PATH/.venv" \
+ "$WORKTREE_PATH/.venv/bin/maturin" develop --release --features accelerate)
```
+`--release --features accelerate` matches the macOS build used elsewhere in this
+project (Apple Accelerate BLAS). On Linux substitute `--features openblas`; on
+Windows drop `--features` entirely.
+
If this step fails, the Rust extension was likely still built successfully by pip
-in step 5. Note that maturin rebuilds are available for future Rust code changes
-and continue. This is not an error.
+in step 5. Report it and continue — this is not an error.
### 7. Report
diff --git a/.claude/scripts/pr_prepare.py b/.claude/scripts/pr_prepare.py
new file mode 100644
index 000000000..b77507979
--- /dev/null
+++ b/.claude/scripts/pr_prepare.py
@@ -0,0 +1,347 @@
+#!/usr/bin/env python3
+"""Prepare and validate every dynamic value ``/submit-pr`` feeds to the shell.
+
+Why this exists
+---------------
+``/submit-pr`` builds a branch, syncs against a base, pushes, and opens a PR. The
+title, branch name, and base branch are user- or generator-influenced text. When any
+of them reaches a shell — even as ``VAR="…"`` — a backtick or ``$(...)`` executes.
+git *accepts* both in a ref name (``git check-ref-format --branch 'x-`whoami`'``
+passes), so an ordinary title like ``Fix `safe_inference` guard`` is a live payload.
+
+The safe boundary is therefore **not** "sanitise before building the command" — it is
+"never let the raw value touch a shell at all". So the untrusted values arrive here as
+**files** (the caller writes them with the Write tool, which never invokes a shell)
+and this script reads their contents with Python I/O. Nothing untrusted is ever an
+argv element or a shell assignment.
+
+This module:
+
+- **never** evaluates input, interpolates it into a shell, or touches the network;
+- reads the raw title / explicit branch / explicit base from files;
+- reduces branch and base to a form containing no shell metacharacters *and* valid as
+ a git ref — generated names by whitelist sanitisation + ref normalisation, explicit
+ names by *rejection* if unsafe (never a silent rewrite);
+- honours an already-checked-out feature branch instead of a title-derived name;
+- resolves fork-vs-direct context from read-only ``git remote`` queries.
+
+Outputs are written one value per file under ``--scratch``; the caller reads each with
+``"$(cat …)"``. Every emitted branch/base value matches ``[A-Za-z0-9._/-]+`` and passes
+``git check-ref-format``, so the caller may use them in git/gh commands safely, quoted.
+
+When the title file is empty the script **generates a fallback title from base-free
+git commands** (the last commit subject) rather than erroring — so the no-title
+``/submit-pr`` invocation works, and the caller never needs to run a base-referencing
+command like ``git log <base>..HEAD`` in prose, which would put a raw, unvalidated
+base on a shell command line.
+
+Exit non-zero with a message on any unsafe or malformed explicit input; the caller
+must abort rather than proceed.
+"""
+
+from __future__ import annotations
+
+import argparse
+import os
+import re
+import subprocess
+import sys
+
+# A ref name is shell-safe iff it contains only these characters. STRICTER than git,
+# which permits backticks, ``$``, ``(``/``)`` and more — all shell-dangerous. Uppercase
+# is allowed: it has no shell semantics, and real branches like ``Release/4.0`` or bases
+# like ``V4`` must not be rejected. Generated names are still lowercased by
+# :func:`sanitize_branch_portion`; this wider set only affects explicit/current refs.
+_SAFE_REF = re.compile(r"[A-Za-z0-9._/-]+")
+
+# For a *title-derived branch portion* (before the type prefix), '/' is also excluded
+# — the only slash in the final name is the one we prepend.
+_UNSAFE_PORTION_CHAR = re.compile(r"[^a-z0-9._-]")
+
+_BRANCH_PREFIXES = ("feature/", "fix/", "refactor/", "docs/")
+_MAX_PORTION = 50
+
+
+# ---------------------------------------------------------------------------
+# Pure helpers (no I/O) — the unit-test surface
+# ---------------------------------------------------------------------------
+
+
+def normalize_ref_portion(portion: str) -> str:
+ """Fix git-invalid ref forms in an already char-whitelisted portion.
+
+ ``git check-ref-format`` rejects ``..``, a component that starts or ends with
+ ``.``, and a trailing ``.lock``. A title like ``v1.2..final`` or ``foo.lock``
+ survives character-whitelisting but would make ``git checkout -b`` fail.
+ """
+ portion = re.sub(r"\.{2,}", ".", portion) # no ".."
+ portion = re.sub(r"\.lock$", "", portion) # no trailing ".lock"
+ portion = portion.strip(".-_") # no leading/trailing . - _
+ return portion
+
+
+def sanitize_branch_portion(text: str) -> str:
+ """Reduce free text (e.g. a PR title) to a safe branch portion, no prefix.
+
+ Lowercase; spaces to hyphens; every character outside ``[a-z0-9._-]`` — including
+ ``/`` — becomes a hyphen; collapse hyphen runs and underscore runs *separately* so
+ a lone underscore survives (``safe_inference`` stays intact); normalise git-invalid
+ dot forms; trim and truncate. The result always matches ``_SAFE_REF``.
+ """
+ s = text.lower().replace(" ", "-")
+ s = _UNSAFE_PORTION_CHAR.sub("-", s)
+ s = re.sub(r"-{2,}", "-", s)
+ s = re.sub(r"_{2,}", "_", s)
+ s = normalize_ref_portion(s)
+ return s[:_MAX_PORTION].strip(".-_")
+
+
+def is_shell_safe_ref(name: str) -> bool:
+ """True iff ``name`` contains only whitelist characters (no metacharacters)."""
+ return bool(name) and _SAFE_REF.fullmatch(name) is not None
+
+
+def owner_repo(url: str) -> "str | None":
+ """Extract ``owner/repo`` from any git remote URL, portably.
+
+ Handles ``git@host:owner/repo.git``, ``https://host/owner/repo(.git)``, and
+ ``ssh://git@host/owner/repo.git``. Returns ``None`` if it cannot be parsed.
+ """
+ # Strip trailing slash(es) FIRST, then the optional ``.git`` — a bare
+ # ``…/owner/repo/`` (no ``.git``) must still parse.
+ u = re.sub(r"\.git$", "", url.strip().rstrip("/"))
+ m = re.search(r"[/:]([^/]+/[^/]+)$", u)
+ return m.group(1) if m else None
+
+
+def build_head_ref(is_fork: bool, fork_owner: "str | None", branch: str) -> str:
+ """The ``--head`` value for ``gh pr create``.
+
+ Direct: just the branch. Fork: ``owner:branch`` built by concatenation, never
+ interpolation. Assumes ``branch`` is already validated safe.
+
+ In fork mode a missing ``fork_owner`` is a hard error, not a fall back to the
+ unqualified branch — ``gh pr create --repo <upstream> --head <branch>`` would then
+ pick a same-named branch in the *base* repo instead of the fork's.
+ """
+ if is_fork:
+ if not fork_owner:
+ raise ValueError(
+ "Fork workflow but could not parse the fork owner from origin's URL; "
+ "refusing to open a PR with an unqualified head ref."
+ )
+ return f"{fork_owner}:{branch}"
+ return branch
+
+
+def resolve_base(explicit: "str | None", default: str = "main") -> str:
+ """Return the base branch, or raise ``ValueError`` if an explicit base is unsafe.
+
+ The default applies only when no base was supplied — an explicit-but-invalid base
+ is an error, never a silent fall back to ``main``.
+ """
+ if explicit is None:
+ return default
+ if not is_shell_safe_ref(explicit):
+ raise ValueError(f"Refusing unsafe base branch {explicit!r}: only [A-Za-z0-9._/-] allowed.")
+ if not _git_ref_ok(explicit):
+ raise ValueError(f"Base branch {explicit!r} is not a valid git ref.")
+ return explicit
+
+
+def resolve_branch(
+ explicit: "str | None",
+ current_branch: "str | None",
+ base: str,
+ title: str,
+ change_type: str,
+) -> str:
+ """Decide the branch name to use, or raise ``ValueError``.
+
+ Precedence:
+
+ 1. **Explicit ``--branch``** — honoured only if shell-safe *and* a valid git ref,
+ else rejected (never silently rewritten). If it conflicts with a different
+ already-checked-out feature branch, that is an error the user must resolve.
+ 2. **An existing feature branch** (``current_branch`` set and not the base) — used
+ verbatim, so the command pushes and PRs the branch you are actually on rather
+ than a title-derived name. Rejected if it somehow carries unsafe characters.
+ 3. **Generated** from the title: sanitised, ref-normalised, prefixed by change
+ type, and required to pass ``git check-ref-format`` (falling back to a safe
+ ``<prefix>change`` if the title reduces to nothing valid).
+ """
+ on_feature = bool(current_branch) and current_branch != base
+
+ if explicit is not None:
+ if not is_shell_safe_ref(explicit):
+ raise ValueError(
+ f"Refusing unsafe branch name {explicit!r}: only [A-Za-z0-9._/-] allowed."
+ )
+ if not _git_ref_ok(explicit):
+ raise ValueError(f"Branch name {explicit!r} is not a valid git ref.")
+ if on_feature and current_branch != explicit:
+ raise ValueError(
+ f"On feature branch {current_branch!r} but --branch {explicit!r} was "
+ f"given. Switch branch or drop --branch; refusing to mix them."
+ )
+ return explicit
+
+ if on_feature:
+ assert current_branch is not None
+ if not is_shell_safe_ref(current_branch):
+ raise ValueError(f"Current branch {current_branch!r} has unsafe characters; rename it.")
+ return current_branch
+
+ prefix = change_type if change_type.endswith("/") else f"{change_type}/"
+ if prefix not in _BRANCH_PREFIXES:
+ prefix = "feature/"
+ portion = sanitize_branch_portion(title) or "change"
+ candidate = f"{prefix}{portion}"
+ if not _git_ref_ok(candidate):
+ candidate = f"{prefix}change"
+ return candidate
+
+
+# ---------------------------------------------------------------------------
+# Thin git wiring (read-only) and file I/O
+# ---------------------------------------------------------------------------
+
+
+def _git(*args: str) -> "subprocess.CompletedProcess[str]":
+ return subprocess.run(["git", *args], capture_output=True, text=True, check=False)
+
+
+def _git_ref_ok(name: str) -> bool:
+ return _git("check-ref-format", "--branch", name).returncode == 0
+
+
+def _remote_url(remote: str) -> "str | None":
+ r = _git("remote", "get-url", remote)
+ return r.stdout.strip() if r.returncode == 0 and r.stdout.strip() else None
+
+
+def generate_fallback_title() -> str:
+ """A default PR title from **base-free** git state, used when none was supplied.
+
+ Deliberately references no base branch: the alternative — the caller running
+ ``git log <base>..HEAD`` in prose to build a title — would place a raw, unvalidated
+ ``--base`` on a shell command line before validation, reopening the injection
+ boundary.
+
+ The last commit subject is only meaningful when the tree is **clean** (the commit
+ *is* the change). In ``/submit-pr``'s normal flow the title is resolved *before*
+ the commit, so the tree is dirty and ``git log -1`` would return the previous,
+ unrelated commit — use a neutral title in that case.
+ """
+ dirty = bool(_git("status", "--porcelain").stdout.strip())
+ if dirty:
+ return "Update working tree changes"
+ r = _git("log", "-1", "--format=%s")
+ subject = r.stdout.strip() if r.returncode == 0 else ""
+ return subject or "Update"
+
+
+def _read_file(path: "str | None") -> "str | None":
+ """Read a value file's content, or None if the path is absent/empty/missing.
+
+ Content is read as data — never parsed by a shell — so any payload is inert.
+ """
+ if not path or not os.path.isfile(path):
+ return None
+ with open(path, encoding="utf-8") as fh:
+ text = fh.read().strip()
+ return text or None
+
+
+def _write(path: str, content: str) -> None:
+ with open(path, "w", encoding="utf-8") as fh:
+ fh.write(content)
+
+
+def main(argv: "list[str] | None" = None) -> int:
+ p = argparse.ArgumentParser(description="Prepare safe PR-creation values.")
+ # Untrusted values arrive as FILES (written by the caller's Write tool), never as
+ # argv strings — so a backtick/$() in a title cannot execute reaching this process.
+ p.add_argument(
+ "--title-file",
+ default=None,
+ help="File holding the raw PR title; if absent/empty, a base-free title is generated",
+ )
+ p.add_argument("--branch-file", default=None, help="File holding an explicit branch")
+ p.add_argument("--base-file", default=None, help="File holding an explicit base")
+ p.add_argument(
+ "--current-branch",
+ default="",
+ help="Output of `git branch --show-current` (trusted git state; may be empty)",
+ )
+ p.add_argument(
+ "--change-type",
+ default="feature",
+ help="Prefix for a generated branch (feature|fix|refactor|docs)",
+ )
+ # --draft is a trusted boolean (flag presence), not untrusted text, so it is a
+ # normal argv flag. Emitting it as a value file keeps the caller's re-read pattern
+ # uniform, so a draft request cannot be silently dropped on the way to `gh`.
+ p.add_argument("--draft", action="store_true", help="Mark the PR as a draft")
+ p.add_argument("--scratch", required=True, help="Directory for output value files")
+ args = p.parse_args(argv)
+
+ os.makedirs(args.scratch, exist_ok=True)
+
+ title = _read_file(args.title_file)
+ if title is None:
+ # No title supplied: generate one from base-free git state rather than error,
+ # so `/submit-pr` with no title works and prose never runs a base-referencing
+ # title command. (title-file is optional-in-effect; the value is still opaque.)
+ title = generate_fallback_title()
+ explicit_branch = _read_file(args.branch_file)
+ explicit_base = _read_file(args.base_file)
+ current_branch = args.current_branch.strip() or None
+
+ try:
+ base = resolve_base(explicit_base)
+ branch = resolve_branch(explicit_branch, current_branch, base, title, args.change_type)
+
+ upstream = _remote_url("upstream")
+ is_fork = upstream is not None
+ if is_fork:
+ # Fail CLOSED: in fork mode both the upstream repo and the fork owner must
+ # parse, or we would target the wrong repository / an unqualified head.
+ target_repo = owner_repo(upstream)
+ if not target_repo:
+ raise ValueError(
+ f"Fork workflow but could not parse owner/repo from upstream URL "
+ f"{upstream!r}."
+ )
+ origin = _remote_url("origin")
+ origin_or = owner_repo(origin) if origin else None
+ fork_owner = origin_or.split("/")[0] if origin_or else None
+ head_ref = build_head_ref(True, fork_owner, branch) # raises if no fork_owner
+ else:
+ target_repo = ""
+ head_ref = build_head_ref(False, None, branch)
+ except ValueError as exc:
+ print(str(exc), file=sys.stderr)
+ return 2
+
+ # One value per file; caller reads each with "$(cat …)". Title is copied to a
+ # canonical name so the caller passes --title "$(cat …/pr-title.txt)".
+ _write(os.path.join(args.scratch, "pr-title.txt"), title)
+ _write(os.path.join(args.scratch, "pr-base.txt"), base)
+ _write(os.path.join(args.scratch, "pr-branch.txt"), branch)
+ _write(os.path.join(args.scratch, "pr-headref.txt"), head_ref)
+ _write(os.path.join(args.scratch, "pr-target-repo.txt"), target_repo or "")
+ _write(os.path.join(args.scratch, "pr-is-fork.txt"), "true" if is_fork else "false")
+ _write(os.path.join(args.scratch, "pr-draft.txt"), "true" if args.draft else "false")
+
+ print(f"base={base}")
+ print(f"branch={branch}")
+ print(f"is_fork={'true' if is_fork else 'false'}")
+ print(f"head_ref={head_ref}")
+ print(f"target_repo={target_repo or ''}")
+ print(f"draft={'true' if args.draft else 'false'}")
+ return 0
+
+
+if __name__ == "__main__":
+ raise SystemExit(main())
diff --git a/.claude/scripts/premerge_scan.py b/.claude/scripts/premerge_scan.py
new file mode 100644
index 000000000..1073e5c08
--- /dev/null
+++ b/.claude/scripts/premerge_scan.py
@@ -0,0 +1,361 @@
+#!/usr/bin/env python3
+"""Argv-safe pre-merge scanning over git-controlled filenames.
+
+Why this exists
+---------------
+`/pre-merge-check` (and the pattern checks `/submit-pr` and `/push-pr-update` share)
+run methodology greps, test resolution, and pytest/nbmake over the set of *changed
+files*. Git permits ``$()``, backticks, quotes, spaces, and newlines in a path, so any
+prose that pastes a changed filename into a shell command — ``grep pattern <files>``,
+``pytest <files>`` — executes a payload like ``diff_diff/$(touch x).py``. A prose
+"screen" is a blocklist and leaks (untracked paths, other commands, argument vs
+assignment forms).
+
+This helper closes the class structurally:
+
+- **Discovery** uses ``git … -z`` through ``subprocess.run([...])`` (an argv array, no
+ shell) and splits on NUL, so a filename is only ever *data*.
+- **Pattern checks** run as pure-Python regex / AST over file *content*. Opening a file
+ by path never invokes a shell, so a hostile filename cannot execute.
+- **Test resolution** matches names in Python; nothing is globbed through a shell.
+- Every changed path is **screened** for shell metacharacters; unsafe paths are
+ reported and excluded from the emitted run-lists, so the caller's ``pytest``/``nbmake``
+ only ever receives validated-safe paths.
+- Git failures **fail closed**: a nonzero git exit stops the scan with a nonzero status
+ rather than being reported as "0 findings".
+
+Modes
+-----
+Default scans the working tree (tracked-modified + staged + untracked). ``--range
+A..B`` scans an already-committed range instead — used by ``/push-pr-update`` when the
+tree is clean but commits are ahead.
+
+The caller runs the helper, reads its report, and runs pytest/nbmake over the
+NUL-delimited safe lists it writes, portably: ``xargs -0 pytest < run-tests.z``.
+"""
+
+from __future__ import annotations
+
+import argparse
+import ast
+import os
+import re
+import subprocess
+
+_SHELL_META = re.compile(r"""[`$;|&<>(){}\[\]"'*?\s\\]""")
+
+_CHECK_A = re.compile(r"t_stat\s*=\s*[^#]*/\s*se")
+_CHECK_B = re.compile(r"if.*(?:se|SE).*>.*0.*else\s+(?:0\.0|0)")
+_CHECK_D_GUARD = re.compile(r"if.*se.*>")
+_SELF_ASSIGN = re.compile(r"self\.(\w+)\s*=\s*\w")
+
+
+class GitError(RuntimeError):
+ """A git command failed — the scan must fail closed, not report empty."""
+
+
+# ---------------------------------------------------------------------------
+# Pure helpers (no I/O) — the unit-test surface
+# ---------------------------------------------------------------------------
+
+
+def is_safe_path(path: str) -> bool:
+ """True iff ``path`` has no character that could execute or reparse in a shell."""
+ return bool(path) and _SHELL_META.search(path) is None
+
+
+def categorize(paths: "list[str]") -> "dict[str, list[str]]":
+ """Bucket changed paths. Methodology = ``diff_diff/**/*.py`` minus ``__init__``."""
+ out: "dict[str, list[str]]" = {"methodology": [], "tests": [], "notebooks": [], "docs": []}
+ for p in paths:
+ base = os.path.basename(p)
+ if p.startswith("diff_diff/") and p.endswith(".py") and base != "__init__.py":
+ out["methodology"].append(p)
+ elif p.startswith("tests/") and p.endswith(".py"):
+ out["tests"].append(p)
+ elif p.startswith("docs/tutorials/") and p.endswith(".ipynb"):
+ out["notebooks"].append(p)
+ elif p.endswith((".md", ".rst")) or p.startswith("docs/"):
+ out["docs"].append(p)
+ return out
+
+
+def check_content_patterns(lines: "list[str]") -> "list[tuple[int, str]]":
+ """Run content checks A/B/D over a file's lines. Returns (line_no, message)."""
+ findings = []
+ for i, line in enumerate(lines, 1):
+ if _CHECK_A.search(line) and "safe_inference" not in line:
+ findings.append((i, "Check A: use safe_inference() instead of inline t_stat"))
+ if _CHECK_B.search(line):
+ findings.append((i, "Check B: SE=0 should produce NaN, not 0.0"))
+ if "compute_confidence_interval" in line and not (
+ "safe_inference" in line or "isfinite" in line or _CHECK_D_GUARD.search(line)
+ ):
+ findings.append((i, "Check D: guard compute_confidence_interval for non-finite SE"))
+ return findings
+
+
+def _init_self_assigns(cls: ast.ClassDef) -> "set[str]":
+ names: "set[str]" = set()
+ for node in cls.body:
+ if isinstance(node, ast.FunctionDef) and node.name == "__init__":
+ for sub in ast.walk(node):
+ if isinstance(sub, ast.Assign):
+ for tgt in sub.targets:
+ if (
+ isinstance(tgt, ast.Attribute)
+ and isinstance(tgt.value, ast.Name)
+ and tgt.value.id == "self"
+ ):
+ names.add(tgt.attr)
+ return names
+
+
+def _get_params_refs(cls: ast.ClassDef) -> "set[str]":
+ refs: "set[str]" = set()
+ for node in cls.body:
+ if isinstance(node, ast.FunctionDef) and node.name == "get_params":
+ for sub in ast.walk(node):
+ if isinstance(sub, ast.Attribute):
+ refs.add(sub.attr)
+ elif isinstance(sub, ast.Name):
+ refs.add(sub.id)
+ elif isinstance(sub, ast.Constant) and isinstance(sub.value, str):
+ refs.add(sub.value)
+ return refs
+
+
+def new_params_missing_from_get_params(
+ added_param_names: "list[str]", file_text: str
+) -> "list[str]":
+ """Check C (AST-based): of the ``added_param_names`` (new ``self.X`` from the diff),
+ return those that are assigned in some class ``__init__`` but never referenced in
+ that same class's ``get_params()``. Restricting to ``__init__`` and to the class's
+ own ``get_params`` avoids the old substring heuristic's false negatives."""
+ try:
+ tree = ast.parse(file_text)
+ except SyntaxError:
+ return []
+ added = set(added_param_names)
+ missing: "set[str]" = set()
+ for cls in (n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)):
+ assigned = _init_self_assigns(cls) & added
+ if not assigned:
+ continue
+ refs = _get_params_refs(cls)
+ missing |= assigned - refs
+ return sorted(missing)
+
+
+def param_names_from_added(added_lines: "list[str]") -> "list[str]":
+ return sorted({m.group(1) for ln in added_lines for m in _SELF_ASSIGN.finditer(ln)})
+
+
+def stem_of(path: str) -> str:
+ """Test-discovery stem: the module name (or package dir), leading underscore dropped."""
+ if path.startswith("diff_diff/"):
+ rest = path[len("diff_diff/") :]
+ first = rest.split("/", 1)[0]
+ stem = first[:-3] if first.endswith(".py") else first
+ else:
+ stem = os.path.basename(path)
+ stem = stem[:-3] if stem.endswith(".py") else stem
+ return stem[1:] if stem.startswith("_") else stem
+
+
+def resolve_tests(stem: str, test_files: "list[str]", group: "str | None" = None) -> "list[str]":
+ """Every test file whose name contains the (underscore-stripped) stem or group."""
+ keys = [k for k in (stem, group) if k]
+ return sorted({t for t in test_files for k in keys if k in os.path.basename(t)})
+
+
+def load_groups(yaml_text: str) -> "dict[str, str]":
+ """Parse the ``groups:`` block of doc-deps.yaml into a member-path → group-name map.
+ Regex-parsed (PyYAML is not a project dependency)."""
+ member2group: "dict[str, str]" = {}
+ m = re.search(r"^groups:\n((?: \S.*\n| - .*\n|\n)*)", yaml_text, re.M)
+ if not m:
+ return member2group
+ cur = None
+ for line in m.group(1).split("\n"):
+ gm = re.match(r"^ (\w[\w_]*):\s*$", line)
+ im = re.match(r"^\s+- (diff_diff/\S+)", line)
+ if gm:
+ cur = gm.group(1)
+ elif im and cur:
+ member2group[im.group(1)] = cur
+ return member2group
+
+
+# ---------------------------------------------------------------------------
+# Thin git wiring (argv, NUL-delimited) and I/O — fail closed on git errors
+# ---------------------------------------------------------------------------
+
+
+def _git_lines_z(*args: str) -> "list[str]":
+ r = subprocess.run(["git", *args], capture_output=True, text=True, check=False)
+ if r.returncode != 0:
+ raise GitError(f"git {' '.join(args)} failed (rc={r.returncode}): {r.stderr.strip()}")
+ return [p for p in r.stdout.split("\0") if p]
+
+
+def parse_name_status_z(tokens: "list[str]") -> "dict[str, str]":
+ """Parse `git diff --name-status -z` tokens → {path: 'A'|'M'|'D'}. Renames/copies
+ (``R``/``C``) carry two paths; the destination is treated as modified."""
+ status: "dict[str, str]" = {}
+ i = 0
+ while i < len(tokens):
+ code = tokens[i]
+ i += 1
+ if code and code[0] in ("R", "C"):
+ # old, new
+ if i + 1 < len(tokens):
+ status[tokens[i + 1]] = "M"
+ i += 2
+ else:
+ if i < len(tokens):
+ status[tokens[i]] = {"A": "A", "D": "D"}.get(code[:1], "M")
+ i += 1
+ return status
+
+
+def _name_status_z(*args: str) -> "dict[str, str]":
+ return parse_name_status_z(_git_lines_z(*args))
+
+
+def _changed_with_status(range_spec: "str | None") -> "dict[str, str]":
+ """{path: status} for the working tree (default) or a committed range. Status is
+ 'A' (added/untracked), 'M' (modified), or 'D' (deleted)."""
+ status: "dict[str, str]" = {}
+ if range_spec:
+ status.update(_name_status_z("diff", "--name-status", "-z", range_spec))
+ else:
+ status.update(_name_status_z("diff", "--name-status", "-z", "HEAD"))
+ status.update(_name_status_z("diff", "--cached", "--name-status", "-z"))
+ for p in _git_lines_z("ls-files", "--others", "--exclude-standard", "-z"):
+ status.setdefault(p, "A")
+ return status
+
+
+def _added_lines_for(path: str, range_spec: "str | None") -> "list[str]":
+ diff_arg = range_spec if range_spec else "HEAD"
+ r = subprocess.run(
+ ["git", "diff", diff_arg, "--", path], capture_output=True, text=True, check=False
+ )
+ if r.returncode != 0:
+ raise GitError(f"git diff for {path!r} failed (rc={r.returncode})")
+ return [
+ ln[1:] for ln in r.stdout.splitlines() if ln.startswith("+") and not ln.startswith("+++")
+ ]
+
+
+def _read_text(path: str) -> str:
+ with open(path, encoding="utf-8", errors="replace") as fh:
+ return fh.read()
+
+
+def main(argv: "list[str] | None" = None) -> int:
+ p = argparse.ArgumentParser(description="Argv-safe pre-merge scan over changed files.")
+ p.add_argument("--scratch", required=True, help="Directory for the safe run-lists")
+ p.add_argument("--range", dest="range_spec", default=None, help="Scan a committed range A..B")
+ p.add_argument("--groups-file", default="docs/doc-deps.yaml", help="doc-deps.yaml for groups")
+ p.add_argument("--test-files-list", default=None, help="Optional NUL file of test paths")
+ args = p.parse_args(argv)
+ os.makedirs(args.scratch, exist_ok=True)
+
+ # Truncate the run-lists up front. The scratch dir is deterministic and reused, and
+ # an early failure (exit 3/4) returns before the lists are (re)written — so clear
+ # them now, or a prior run's stale lists could drive pytest/nbmake over unrelated
+ # files. They are repopulated only on the success path below.
+ run_tests_path = os.path.join(args.scratch, "run-tests.z")
+ run_notebooks_path = os.path.join(args.scratch, "run-notebooks.z")
+ open(run_tests_path, "w").close()
+ open(run_notebooks_path, "w").close()
+
+ try:
+ status = _changed_with_status(args.range_spec)
+ except GitError as exc:
+ print(f"SCAN FAILED (git error): {exc}")
+ return 4 # fail closed — do NOT report "0 findings" on a broken scan
+
+ unsafe = [p for p in status if not is_safe_path(p)]
+ safe = {p: s for p, s in status.items() if is_safe_path(p)}
+ cats = categorize(list(safe))
+
+ print("== Pre-merge scan ==")
+ if unsafe:
+ print("UNSAFE PATHS (shell metacharacters) — excluded from checks, review by hand:")
+ for u in unsafe:
+ print(f" !! {u!r}")
+
+ findings = []
+ try:
+ for mpath in cats["methodology"]:
+ if safe[mpath] == "D":
+ continue # deleted — nothing to read or run
+ text = _read_text(mpath)
+ for line_no, msg in check_content_patterns(text.splitlines()):
+ findings.append(f"{mpath}:{line_no}: {msg}")
+ if safe[mpath] == "A":
+ # Untracked/added: `git diff HEAD` shows no added lines, so treat the
+ # whole file as new when extracting self.X params for Check C.
+ added_params = param_names_from_added(text.splitlines())
+ else:
+ added_params = param_names_from_added(_added_lines_for(mpath, args.range_spec))
+ for miss in new_params_missing_from_get_params(added_params, text):
+ findings.append(f"{mpath}: Check C: self.{miss} not found in get_params()")
+ except (GitError, OSError) as exc:
+ print(f"SCAN FAILED during pattern checks: {exc}")
+ return 4
+
+ print(f"\nPattern checks: {len(findings)} finding(s)")
+ for f in findings:
+ print(f" {f}")
+
+ # Test resolution. Seed with changed *collectable* tests (test_*.py only — a
+ # conftest.py/helper change is not a pytest target), then add module-resolved suites
+ # (name stem + doc-deps group). Deleted files are excluded from run-lists.
+ try:
+ test_files = (
+ [p for p in _read_text(args.test_files_list).split("\0") if p]
+ if args.test_files_list and os.path.isfile(args.test_files_list)
+ else _git_lines_z("ls-files", "-z", "tests/test_*.py")
+ )
+ except GitError as exc:
+ print(f"SCAN FAILED listing tests: {exc}")
+ return 4
+ groups = load_groups(_read_text(args.groups_file)) if os.path.isfile(args.groups_file) else {}
+
+ def collectable(t: str) -> bool:
+ return os.path.basename(t).startswith("test_") and safe.get(t) != "D"
+
+ to_run: "set[str]" = {t for t in cats["tests"] if collectable(t)}
+ non_test_changes = [t for t in cats["tests"] if not os.path.basename(t).startswith("test_")]
+ for mpath in cats["methodology"]:
+ # A deleted module still resolves surviving dependent tests to run.
+ hits = [
+ t
+ for t in resolve_tests(stem_of(mpath), test_files, groups.get(mpath))
+ if safe.get(t) != "D"
+ ]
+ if not hits and safe[mpath] != "D":
+ print(f"\n no test file resolved for {mpath} — confirm coverage manually")
+ to_run.update(hits)
+ if non_test_changes:
+ print(f"\n changed support files (not pytest targets): {non_test_changes} —")
+ print(" targeted resolution is insufficient; consider the full suite.")
+ ordered = sorted(to_run)
+
+ notebooks = [n for n in cats["notebooks"] if safe.get(n) != "D"]
+ with open(run_tests_path, "w", encoding="utf-8") as fh:
+ fh.write("\0".join(ordered))
+ with open(run_notebooks_path, "w", encoding="utf-8") as fh:
+ fh.write("\0".join(notebooks))
+
+ print(f"\nResolved {len(ordered)} test file(s) and {len(notebooks)} notebook(s).")
+ print("Run portably: xargs -0 pytest <", os.path.join(args.scratch, "run-tests.z"))
+ return 3 if unsafe else 0
+
+
+if __name__ == "__main__":
+ raise SystemExit(main())
diff --git a/CLAUDE.md b/CLAUDE.md
index 02aab92dd..73250b82a 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -198,8 +198,7 @@ When adding new functionality, the source of truth is:
| `docs/methodology/REGISTRY.md` | Academic foundations, equations, edge cases — **consult before methodology changes** |
| `docs/v4-design.md` + `docs/v4-deprecations.yaml` | 4.0 program design spec + CI-enforced deprecation ledger — **consult before any 4.0-program PR**; deviations must edit both in the same diff |
| `docs/doc-deps.yaml` | Source-to-documentation dependency map — **consult when any source file changes** |
-| `CONTRIBUTING.md` | Documentation requirements, test writing guidelines |
-| `.claude/commands/dev-checklists.md` | Checklists for params, methodology, warnings, reviews, bugs (run `/dev-checklists`) |
+| `CONTRIBUTING.md` | Documentation requirements, test writing guidelines, implementation guidelines |
| `.claude/memory.md` | Debugging patterns, tolerances, API conventions (git-tracked) |
| `diff_diff/guides/llms-practitioner.txt` | Baker et al. (2025) 8-step practitioner workflow for AI agents (accessible at runtime via `diff_diff.get_llm_guide("practitioner")`) |
| `docs/performance-plan.md` | Performance optimization details |
@@ -220,8 +219,8 @@ When adding new functionality, the source of truth is:
pending on the user.
- For non-trivial tasks, use `EnterPlanMode`. Consult `docs/methodology/REGISTRY.md` for methodology changes.
- When modifying source files in `diff_diff/`, consult `docs/doc-deps.yaml` to identify impacted documentation. Run `/docs-impact` to see the full list.
-- For bug fixes, grep for the pattern across all files before fixing.
-- Follow the relevant development checklists (run `/dev-checklists`).
+- For bug fixes, grep for the pattern across all files before fixing, and fix every
+ occurrence in the same PR (see CONTRIBUTING.md "Implementation Guidelines").
- Before submitting: run `/pre-merge-check`, then `/ai-review-local` for pre-PR AI review.
- Submit with `/submit-pr`.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 906a0ffb3..b86df4f4b 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -145,3 +145,54 @@ Example format (RST):
Use `assert_nan_inference()` from conftest.py to validate ALL inference fields are
NaN-consistent. Don't check individual fields separately.
+
+## Implementation Guidelines
+
+### Adding an estimator parameter
+
+Storing a parameter is the easy half. Before implementing, grep for every place it
+will have to be honoured:
+
+```bash
+grep -rn "self\.<param>" diff_diff/<module>.py
+```
+
+A new parameter is only complete when it is:
+
+- stored on `self` and returned by `get_params()` (and accepted by `set_params()`)
+- applied in **every** aggregation mode — `simple`, `event_study`, and `group`
+- applied in the **bootstrap/inference** paths, not just the analytical one
+- reflected on the result object, so `to_dict()`/`summary()` do not misreport it
+- propagated to the estimators that inherit it (see the inheritance map in
+ `CLAUDE.md` — subclasses of `DifferenceInDifferences` inherit automatically,
+ standalone estimators must each be updated)
+
+The recurring bug is a parameter that works in the default aggregation and is
+silently ignored in one of the others. The same trace applies when adding a new
+*mode* or code path: follow it through aggregation, bootstrap, and results before
+declaring it done.
+
+### Control/comparison-group composition on new code paths
+
+When you add a new mode or code path (e.g. `base_period="varying"`, a new
+control-selection rule), **verify the control/comparison group is composed correctly
+for that path**, not just the default. In staggered designs especially, confirm the
+"not-yet-treated" comparison group excludes the treatment cohort itself, and that
+parameter interactions (new mode × each aggregation method) select the intended
+group. A silently mis-composed comparison group produces a plausible but wrong effect
+with no error.
+
+### Protecting arithmetic
+
+Wrap **all** related operations in `np.errstate()`, not just the one that raised.
+A guard around the final division still lets an upstream matrix multiply or
+subtraction emit the warning. Include division, matrix multiplication, and any
+operation that can overflow or underflow.
+
+### Fixing a pattern that appears in more than one place
+
+Grep for every occurrence **before** fixing any of them, and fix them all in the
+same PR. Incremental fixes across review rounds are how a pattern bug survives:
+each round looks resolved, and the next reviewer finds the sibling you missed.
+If the same class of finding comes back a third time, stop patching sites and
+name the invariant instead.
diff --git a/TODO.md b/TODO.md
index 33ed91d3f..da804a633 100644
--- a/TODO.md
+++ b/TODO.md
@@ -47,3 +47,6 @@ generic sparse-FE, QR+SVD rank-detection redundancy, `check_finite` bypass — m
| MMM interop PR-B: calibration tutorial notebook (fit DiD/CS -> scope -> `to_pymc_marketing_lift_test` / `to_meridian_roi_prior`) + a `llms-practitioner.txt` Step 8 pointer to the exporters as the MMM hand-off. | `docs/tutorials/`, `diff_diff/guides/llms-practitioner.txt` | mmm-interop | Mid | Low |
| Tracking-file contract guard test: reject NEW active deferred-work pointers at `TODO.md` (deferred rows live in `DEFERRED.md`; allowlist for historical/past-tense prose and actionable-row pointers) and assert rows cross-linking a `docs/v4-deprecations.yaml` `M-xxx` id don't restate ledger status. Origin: tracking-split local review R2. | `tests/`, `TODO.md`, `DEFERRED.md` | tracking-split | Quick | Low |
| Real-data CI canary for dataset-backed replication tests: `test_methodology_lwdid.py`'s Prop 99 / Walmart goldens skip (visibly) when loaders fall back to synthetic; add a lane or canary asserting `df.attrs["source"] == "lwdid_ssc_ancillary"` in CI so network regressions cannot silently de-gate the replication tests. Pairs with the loader-fallback repair row above. | `tests/test_methodology_lwdid.py`, `.github/workflows/` | LWDiD validation suite | Quick | Low |
+| `worktree-rm` safety via a tested argv helper: the prose rewrite (ask-before-remove confirmation gate, detached-HEAD reachability/rescue, tip-identity force-delete guard) was reverted to the main version because editing the prose repeatedly reintroduced shell-injection (last: sourcing a state file built from a git-derived branch name). Restore those guards in a `worktree_rm.py` that takes the name via file ingress, invokes git through argv arrays, and has a metacharacter-branch/path injection regression test — the pattern that worked for `pr_prepare.py`/`premerge_scan.py`. | `.claude/commands/worktree-rm.md`, `.claude/scripts/` | skill-audit | Heavy | Medium |
+| `premerge_scan.py` should scan the staged blob (`git show :path`) for staged methodology files, not the working-tree copy — a stage-then-revert-working-copy edit currently reads the safe working version and misses the staged violation. Union staged-index findings with unstaged/untracked filesystem findings. | `.claude/scripts/premerge_scan.py` | skill-audit | Mid | Low |
+| Re-add committed-range methodology scanning to `/push-pr-update` §3b (clean tree, commits ahead) using `premerge_scan.py --range`, with the comparison ref passed as **data** (resolved into a quoted variable in one Bash call, never a raw `<placeholder>`). It was removed to avoid ref interpolation; the helper already implements and tests `--range`. | `.claude/commands/push-pr-update.md` | skill-audit | Quick | Low |
diff --git a/tests/test_command_contract.py b/tests/test_command_contract.py
new file mode 100644
index 000000000..f37a13595
--- /dev/null
+++ b/tests/test_command_contract.py
@@ -0,0 +1,263 @@
+"""Contract tests over the /submit-pr and /push-pr-update command markdown.
+
+The safety of these commands lives partly in prose (the model follows the steps), so
+`pr_prepare.py` and `git commit --file` being safe in isolation is not enough — the
+commands must actually use them. These lightweight checks read the command files and
+fail if a heredoc-based `git commit` is reintroduced, raw values are interpolated, or a
+base-referencing git command runs before the base is validated. They scan **fenced
+`bash` blocks only**, so prose references to an anti-pattern (in backticks, mid-
+sentence) are never mistaken for executable lines.
+"""
+
+import pathlib
+import re
+
+import pytest
+
+_COMMANDS = pathlib.Path(__file__).resolve().parent.parent / ".claude" / "commands"
+_FILES = ["submit-pr.md", "push-pr-update.md"]
+
+
+def _bash_block_lines(text):
+ """Yield (line_number, line) for every line inside a ```bash fenced block.
+
+ Line numbers are 1-indexed, matching editor display and the other checks here.
+ """
+ lines = text.splitlines()
+ i = 0
+ while i < len(lines):
+ if re.match(r"^\s*```bash\b", lines[i]):
+ j = i + 1
+ while j < len(lines) and not re.match(r"^\s*```\s*$", lines[j]):
+ yield j + 1, lines[j]
+ j += 1
+ i = j + 1
+ else:
+ i += 1
+
+
+# A real heredoc commit is a *command line* beginning with `git commit -m "$(cat`.
+_HEREDOC_COMMIT = re.compile(r"^\s*git commit -m \"\$\(cat")
+# A git command performing a `..` range (needs a base ref) — matched anywhere on the
+# line, so it also catches `TITLE="$(git log "$BASE"..HEAD)"`.
+_BASE_RANGE_CMD = re.compile(r"git (?:log|diff|rev-list)\b[^\n]*\.\.")
+# The actual helper invocation (not a prose mention).
+_PREPARE_INVOKE = re.compile(r"^\s*python3 \.claude/scripts/pr_prepare\.py\b")
+
+
+def _read(name):
+ if not _COMMANDS.is_dir():
+ # No command dir at all → installed distribution without repo tooling.
+ pytest.skip("command dir not present (installed distribution)")
+ path = _COMMANDS / name
+ # In a real checkout a missing hardened command is a lost safety boundary, not a
+ # reason to skip — fail loudly so deleting the file cannot leave the suite green.
+ assert path.exists(), f"{name} is missing from a repo checkout — safety contract lost"
+ return path.read_text()
+
+
+_REQUIRED_COMMANDS = ["submit-pr.md", "push-pr-update.md", "worktree-new.md", "worktree-rm.md"]
+
+
+@pytest.mark.parametrize("name", _REQUIRED_COMMANDS)
+def test_required_command_present_in_checkout(name):
+ if not _COMMANDS.is_dir():
+ pytest.skip("not a repo checkout")
+ assert (_COMMANDS / name).exists(), f"{name} deleted from a checkout — contract lost"
+
+
+@pytest.mark.parametrize("name", _FILES)
+def test_no_active_heredoc_commit(name):
+ offenders = [(n, ln) for n, ln in _bash_block_lines(_read(name)) if _HEREDOC_COMMIT.match(ln)]
+ assert not offenders, f"{name} has heredoc `git commit -m` line(s): {offenders}"
+
+
+@pytest.mark.parametrize("name", _FILES)
+def test_uses_git_commit_file(name):
+ assert "git commit --file" in _read(
+ name
+ ), f"{name} must commit via `git commit --file`, not a heredoc"
+
+
+def test_submit_pr_has_no_raw_value_interpolation():
+ """No raw title/branch/base pasted into a shell command; all flow through files."""
+ text = _read("submit-pr.md")
+ assert '--title "$RAW' not in text
+ assert not re.search(r'--(branch|base) "\$RAW', text)
+
+
+def test_submit_pr_has_zero_diff_guard():
+ """submit-pr must guard against a clean zero-commits-ahead branch (else it creates
+ and pushes a useless branch, then gh pr create fails on an empty diff). The idempotent
+ reframe removed the blanket 'nothing to submit' exit; the guard must be scoped to
+ 'zero commits ahead of base'."""
+ text = _read("submit-pr.md")
+ assert re.search(
+ r'rev-list --count "\$BASE_REMOTE/\$BASE_BRANCH\.\.HEAD"', text
+ ), "submit-pr must compare HEAD to the base to detect a zero-diff branch"
+ assert "Nothing to submit" in text, "submit-pr must still exit on a genuine zero-diff"
+
+
+def test_submit_pr_idempotency_check_compares_base_and_draft():
+ """The existing-PR idempotency check must not treat ANY open PR as success — it must
+ compare baseRefName/isDraft. Assert the fields are on the *executable* `gh pr view`
+ command line (its --json argument), not merely somewhere in the prose."""
+ view_lines = [
+ ln
+ for _, ln in _bash_block_lines(_read("submit-pr.md"))
+ if "gh pr view" in ln and "--json" in ln
+ ]
+ assert view_lines, "no executable `gh pr view --json` idempotency query found"
+ assert any(
+ "baseRefName" in ln and "isDraft" in ln for ln in view_lines
+ ), "the gh pr view idempotency query must fetch baseRefName AND isDraft to compare them"
+
+
+def test_submit_pr_no_base_range_command_before_validation():
+ """The raw --base must not reach a shell before pr_prepare.py validates it. No git
+ command performing a base `..` range may appear, in any bash block, before the
+ actual helper invocation — this is the exact P0 class where title generation ran
+ `git log <base>..HEAD` on the raw base. Scans fenced bash only (prose references are
+ not matched) and locates the real `python3 …pr_prepare.py` line, not a prose
+ mention of it."""
+ text = _read("submit-pr.md")
+ lines = text.splitlines()
+ inv_line = next((n for n, ln in enumerate(lines, 1) if _PREPARE_INVOKE.match(ln)), None)
+ assert inv_line is not None, "no actual `python3 …/pr_prepare.py` invocation found"
+ offenders = [
+ (n, ln) for n, ln in _bash_block_lines(text) if n < inv_line and _BASE_RANGE_CMD.search(ln)
+ ]
+ assert not offenders, f"base-range git command before validation: {offenders}"
+
+
+# ---------------------------------------------------------------------------
+# worktree-new safety guards. (worktree-rm's destructive-logic rewrite was reverted
+# to its main version in this PR — see PR notes — so it has no guards to assert here.)
+# ---------------------------------------------------------------------------
+
+_WORKTREE_NEW_REQUIRED = [
+ "ls-remote --symref", # default branch from the remote, not `gh repo view <alias>`
+ "FRESH_OK", # unverified freshness is surfaced, not treated as in-sync
+]
+
+
+@pytest.mark.parametrize("pattern", _WORKTREE_NEW_REQUIRED)
+def test_worktree_new_keeps_guard(pattern):
+ assert pattern in _read("worktree-new.md"), f"worktree-new.md lost guard: {pattern!r}"
+
+
+def test_worktree_new_no_gh_repo_view_alias():
+ # `gh repo view "$BASE_REMOTE"` treats a remote alias as owner/repo; must not recur.
+ # Skip `#` comment lines — the fix documents the anti-pattern in a bash comment.
+ offenders = [
+ (n, ln)
+ for n, ln in _bash_block_lines(_read("worktree-new.md"))
+ if not ln.lstrip().startswith("#") and re.search(r'gh repo view "\$BASE_REMOTE"', ln)
+ ]
+ assert not offenders, f"worktree-new uses gh repo view with a remote alias: {offenders}"
+
+
+# A shell assignment that pastes a raw <placeholder> value into shell source — the
+# class behind every injection finding this PR closed (title, base, filename stem).
+# A git filename/title with `$()` or backticks executes at such an assignment.
+_PLACEHOLDER_ASSIGN = re.compile(r'^\s*[A-Za-z_][A-Za-z0-9_]*="<[^>]+>"')
+
+# The workflow commands this PR hardens. (revise-plan.md also has one such line but is
+# untouched here and belongs to the separate plan-review effort.)
+_HARDENED = [
+ "submit-pr.md",
+ "push-pr-update.md",
+ "pre-merge-check.md",
+ "worktree-new.md",
+ "worktree-rm.md",
+]
+
+
+@pytest.mark.parametrize("name", _HARDENED)
+def test_no_raw_placeholder_assignment(name):
+ """Invariant: no command pastes a raw value into a shell assignment. Discovered
+ filenames, titles, and bases must be consumed as data (files, NUL streams, or the
+ model's reasoning), never `VAR="<placeholder>"` — a `$(...)`/backtick payload would
+ execute at the assignment. (This catches the ASSIGNMENT form; pre-merge-check, which
+ must pass filenames as command ARGUMENTS, is additionally guarded below.)"""
+ offenders = [
+ (n, ln) for n, ln in _bash_block_lines(_read(name)) if _PLACEHOLDER_ASSIGN.match(ln)
+ ]
+ assert not offenders, f"{name} pastes a raw value into a shell assignment: {offenders}"
+
+
+_SCAN_COMMANDS = ["pre-merge-check.md", "submit-pr.md", "push-pr-update.md"]
+
+
+@pytest.mark.parametrize("name", _SCAN_COMMANDS)
+def test_commands_invoke_premerge_scan(name):
+ """The methodology pattern checks over changed filenames must go through the
+ argv-safe helper (premerge_scan.py), never a shell grep over paths. All three
+ commands that run those checks must invoke it."""
+ assert "premerge_scan.py" in _read(
+ name
+ ), f"{name} must run pattern checks via premerge_scan.py, not a shell grep"
+
+
+def test_push_pr_update_no_raw_ref_interpolation():
+ """push-pr-update must not paste a git-controlled ref (`<comparison-ref>` /
+ `<default-branch>`) into an executable command — git accepts `$()`/backticks in ref
+ names. Refs are resolved into shell variables and used quoted. Scans bash blocks
+ only, so the prose guard mentioning the placeholder is not matched."""
+ offenders = [
+ (n, ln)
+ for n, ln in _bash_block_lines(_read("push-pr-update.md"))
+ if re.search(r"<(comparison-ref|default-branch)>", ln)
+ ]
+ assert not offenders, f"push-pr-update interpolates a raw ref placeholder: {offenders}"
+
+
+def test_push_pr_update_ref_vars_are_quoted():
+ """It is not enough to reject the raw placeholder — a *bare* `$COMPARISON_REF..HEAD`
+ (unquoted) would still let a `$()`/backtick in the ref's value execute. Assert every
+ `$COMPARISON_REF`/`$DEFAULT_BRANCH` use in a bash block is quoted (preceded by `"` or
+ inside a double-quoted span)."""
+ bare = []
+ for n, ln in _bash_block_lines(_read("push-pr-update.md")):
+ for m in re.finditer(r"\$(?:COMPARISON_REF|DEFAULT_BRANCH)\b", ln):
+ before = ln[: m.start()]
+ quoted = before.rstrip().endswith('"') or (before.count('"') % 2 == 1)
+ if not quoted:
+ bare.append((n, ln.strip()))
+ assert not bare, f"push-pr-update has BARE (unquoted) ref variable use: {bare}"
+
+
+def test_quoted_ref_variable_does_not_execute(tmp_path):
+ """The safety property behind the quoting: a ref *value* holding `$(...)` as data
+ (as it would if git ever returned such a ref name) is inert when used through a
+ quoted expansion — the exact shape the command uses, `git rev-list "$REF..HEAD"`.
+ Single-quote assignment holds the payload as literal data; the quoted use must not
+ re-execute it."""
+ import subprocess
+
+ script = "REF='x$(touch SHOULD_NOT)'; git rev-list --count \"$REF..HEAD\" 2>/dev/null; true"
+ subprocess.run(["bash", "-c", script], cwd=tmp_path, capture_output=True)
+ assert not (tmp_path / "SHOULD_NOT").exists(), "quoted ref-var use executed its payload"
+
+
+def test_pre_merge_check_has_no_filename_grep():
+ """No `grep/pytest/git diff` over a `<changed-…files>` placeholder may remain in
+ pre-merge-check — that was the filename-as-argument injection surface. Filenames now
+ flow only through the helper and `xargs -0` over its NUL-delimited safe lists."""
+ offenders = [
+ (n, ln)
+ for n, ln in _bash_block_lines(_read("pre-merge-check.md"))
+ if re.search(r"(grep|pytest|git diff|ls)\b[^\n]*<changed", ln)
+ ]
+ assert not offenders, f"pre-merge-check still greps changed filenames: {offenders}"
+
+
+def test_no_gnu_only_xargs_a():
+ """`xargs -a` is a GNU extension that BSD/macOS xargs rejects. The run-lists must be
+ fed over stdin (`xargs -0 pytest < file`) so pre-merge-check works on macOS."""
+ offenders = [
+ (n, ln)
+ for n, ln in _bash_block_lines(_read("pre-merge-check.md"))
+ if re.search(r"xargs\b[^\n]*\s-a\b", ln)
+ ]
+ assert not offenders, f"GNU-only `xargs -a` (breaks on macOS): {offenders}"
diff --git a/tests/test_commit_message_safety.py b/tests/test_commit_message_safety.py
new file mode 100644
index 000000000..b09c967d5
--- /dev/null
+++ b/tests/test_commit_message_safety.py
@@ -0,0 +1,44 @@
+"""Regression: /submit-pr and /push-pr-update must commit via `git commit --file`,
+not a `git commit -m "$(cat <<'EOF' … )"` heredoc.
+
+A quoted heredoc stops expansion *inside* the body, but a message whose body contains
+a line equal to the delimiter (`EOF`) closes the heredoc early — the following lines
+then run as shell. This test proves the mandated pattern (write the message to a file
+with a non-shell writer, then `git commit --file`) is immune to that, using a message
+that would `touch` a sentinel if any line were interpreted by a shell.
+"""
+
+import subprocess
+
+import pytest
+
+
+def _git(*args, cwd):
+ return subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True, check=False)
+
+
+@pytest.fixture()
+def repo(tmp_path):
+ _git("init", cwd=tmp_path)
+ _git("config", "user.email", "t@t.t", cwd=tmp_path)
+ _git("config", "user.name", "t", cwd=tmp_path)
+ (tmp_path / "f.txt").write_text("x")
+ _git("add", "-A", cwd=tmp_path)
+ return tmp_path
+
+
+def test_commit_file_does_not_execute_message_content(repo, tmp_path):
+ sentinel = tmp_path / "sentinel"
+ # A message that breaks a `<<'EOF'` heredoc and, if any line were run as shell,
+ # would create the sentinel.
+ malicious = f"Fix thing\n\nEOF\n$(touch {sentinel})\n`touch {sentinel}`\nend\n"
+ msg_file = repo / "commit-msg.txt"
+ msg_file.write_text(malicious) # non-shell write, mirrors the Write tool
+
+ r = _git("commit", "--file", str(msg_file), cwd=repo)
+ assert r.returncode == 0, r.stderr
+ assert not sentinel.exists(), "commit message content executed as shell"
+
+ # And the message is preserved verbatim (nothing stripped or interpreted).
+ body = _git("log", "-1", "--pretty=%B", cwd=repo).stdout
+ assert "$(touch" in body and "EOF" in body
diff --git a/tests/test_pr_prepare.py b/tests/test_pr_prepare.py
new file mode 100644
index 000000000..e90445045
--- /dev/null
+++ b/tests/test_pr_prepare.py
@@ -0,0 +1,365 @@
+"""Tests for .claude/scripts/pr_prepare.py — safe ingress for /submit-pr values.
+
+These guard the property that matters: no title, base, or branch value can carry a
+shell metacharacter or command substitution into a command line, an unsafe *explicit*
+branch/base is rejected rather than silently rewritten, generated names are valid git
+refs, and an already-checked-out feature branch is honoured. The suite is skipped when
+the script is absent (e.g. a pip-installed checkout without .claude/scripts/).
+"""
+
+import importlib.util
+import pathlib
+import subprocess
+import sys
+
+import pytest
+
+
+def _find_script() -> "pathlib.Path | None":
+ candidate = (
+ pathlib.Path(__file__).resolve().parent.parent / ".claude" / "scripts" / "pr_prepare.py"
+ )
+ if candidate.exists():
+ return candidate
+ try:
+ root = subprocess.check_output(
+ ["git", "rev-parse", "--show-toplevel"],
+ stderr=subprocess.DEVNULL,
+ text=True,
+ ).strip()
+ candidate = pathlib.Path(root) / ".claude" / "scripts" / "pr_prepare.py"
+ if candidate.exists():
+ return candidate
+ except (subprocess.CalledProcessError, FileNotFoundError):
+ pass
+ return None
+
+
+_SCRIPT_PATH = _find_script()
+
+pytestmark = pytest.mark.skipif(
+ _SCRIPT_PATH is None,
+ reason="pr_prepare.py not found (not in repo checkout)",
+)
+
+
+@pytest.fixture(scope="module")
+def mod():
+ spec = importlib.util.spec_from_file_location("pr_prepare", _SCRIPT_PATH)
+ module = importlib.util.module_from_spec(spec) # type: ignore[arg-type]
+ spec.loader.exec_module(module) # type: ignore[union-attr]
+ return module
+
+
+# ---------------------------------------------------------------------------
+# sanitize_branch_portion — the injection-neutralising core
+# ---------------------------------------------------------------------------
+
+_MALICIOUS = [
+ "Fix `safe_inference` and $(touch /tmp/sentinel)",
+ "Add ${IFS}payload; rm -rf .",
+ "Handle | pipe && chain > redirect",
+ "Quote 'single' and \"double\" and `back`",
+ "Newline\ninjection\rtest",
+]
+
+
+@pytest.mark.parametrize("title", _MALICIOUS)
+def test_sanitized_portion_is_shell_safe(mod, title):
+ out = mod.sanitize_branch_portion(title)
+ assert mod.is_shell_safe_ref(out), f"{out!r} still contains metacharacters"
+ for ch in "`$();|&><'\"\n\r/ ":
+ assert ch not in out
+
+
+def test_single_underscore_preserved(mod):
+ out = mod.sanitize_branch_portion("Fix safe_inference NaN guard")
+ assert "safe_inference" in out
+ assert "__" not in out
+
+
+def test_hyphen_runs_collapse(mod):
+ assert mod.sanitize_branch_portion("a---b c") == "a-b-c"
+
+
+def test_portion_truncated_and_trimmed(mod):
+ out = mod.sanitize_branch_portion("-" * 5 + "x" * 80)
+ assert len(out) <= 50
+ assert not out.startswith("-") and not out.endswith("-")
+
+
+def test_empty_after_sanitize(mod):
+ assert mod.sanitize_branch_portion("!!!///$$$") == ""
+
+
+# ---------------------------------------------------------------------------
+# normalize_ref_portion + generated refs are valid git refs
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+ "raw,forbidden",
+ [
+ ("a..b", ".."),
+ ("foo.lock", ".lock"),
+ (".hidden", None), # leading dot stripped
+ ("trailing.", None), # trailing dot stripped
+ ],
+)
+def test_normalize_fixes_git_invalid_forms(mod, raw, forbidden):
+ out = mod.normalize_ref_portion(raw)
+ if forbidden:
+ assert forbidden not in out
+ assert not out.startswith(".") and not out.endswith(".")
+
+
+@pytest.mark.parametrize(
+ "title",
+ ["Fix v1.2..final", "Release foo.lock", "...", "Handle a....b edge"],
+)
+def test_generated_branch_is_valid_git_ref(mod, title):
+ # These titles would produce git-invalid refs without normalization.
+ b = mod.resolve_branch(None, None, "main", title, "fix")
+ assert mod._git_ref_ok(b), f"generated {b!r} fails git check-ref-format"
+
+
+# ---------------------------------------------------------------------------
+# resolve_branch — precedence: explicit > existing feature branch > generated
+# ---------------------------------------------------------------------------
+
+
+def test_generated_branch_has_prefix_and_safe_portion(mod):
+ b = mod.resolve_branch(None, None, "main", "Fix `safe_inference` bug", "fix")
+ assert b.startswith("fix/")
+ assert mod.is_shell_safe_ref(b)
+ assert "`" not in b and "$" not in b
+
+
+def test_generated_branch_slash_only_from_prefix(mod):
+ b = mod.resolve_branch(None, None, "main", "touch /tmp/x", "fix")
+ assert b.count("/") == 1
+
+
+def test_existing_feature_branch_is_used_verbatim(mod):
+ # On a feature branch, no explicit --branch: use the branch we are ON, not a
+ # title-derived name (else push/HEAD_REF target the wrong branch).
+ b = mod.resolve_branch(None, "fix/already-here", "main", "some title", "fix")
+ assert b == "fix/already-here"
+
+
+def test_on_base_generates_from_title(mod):
+ b = mod.resolve_branch(None, "main", "main", "New thing", "feature")
+ assert b == "feature/new-thing"
+
+
+def test_explicit_branch_conflicts_with_feature_branch_rejected(mod):
+ with pytest.raises(ValueError):
+ mod.resolve_branch("fix/other", "fix/already-here", "main", "t", "fix")
+
+
+def test_unknown_change_type_falls_back_to_feature(mod):
+ b = mod.resolve_branch(None, None, "main", "whatever", "nonsense")
+ assert b.startswith("feature/")
+
+
+@pytest.mark.parametrize(
+ "bad",
+ ["evil`whoami`", "a$(id)b", "has space", "semi;colon", "pipe|x", 'quote"x'],
+)
+def test_explicit_unsafe_branch_rejected(mod, bad):
+ with pytest.raises(ValueError):
+ mod.resolve_branch(bad, None, "main", "title", "fix")
+
+
+def test_explicit_safe_branch_used_verbatim(mod):
+ assert (
+ mod.resolve_branch("fix/manual-branch_1.2", None, "main", "t", "fix")
+ == "fix/manual-branch_1.2"
+ )
+
+
+def test_base_default_when_absent(mod):
+ assert mod.resolve_base(None) == "main"
+
+
+def test_explicit_unsafe_base_rejected(mod):
+ with pytest.raises(ValueError):
+ mod.resolve_base("main`whoami`")
+
+
+def test_explicit_safe_base_used(mod):
+ assert mod.resolve_base("develop") == "develop"
+
+
+def test_uppercase_explicit_ref_accepted(mod):
+ # Uppercase has no shell semantics; real refs like Release/4.0 or V4 must pass.
+ assert mod.resolve_branch("Release/4.0", None, "main", "t", "fix") == "Release/4.0"
+ assert mod.resolve_base("V4") == "V4"
+
+
+def test_build_head_ref_fork_without_owner_raises(mod):
+ with pytest.raises(ValueError):
+ mod.build_head_ref(True, None, "fix/x")
+ with pytest.raises(ValueError):
+ mod.build_head_ref(True, "", "fix/x")
+
+
+# ---------------------------------------------------------------------------
+# owner_repo / head ref
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+ "url,expected",
+ [
+ ("git@github.com:owner/repo.git", "owner/repo"),
+ ("https://github.com/owner/repo.git", "owner/repo"),
+ ("https://github.com/owner/repo", "owner/repo"),
+ ("ssh://git@github.com/owner/repo.git", "owner/repo"),
+ ("git@github.com:owner/repo.git/", "owner/repo"),
+ ("https://github.com/owner/repo/", "owner/repo"), # trailing slash, no .git
+ ("git@github.com:owner/repo/", "owner/repo"),
+ ("https://github.com/owner/repo.git/", "owner/repo"),
+ ],
+)
+def test_owner_repo_parses(mod, url, expected):
+ assert mod.owner_repo(url) == expected
+
+
+def test_build_head_ref_direct_vs_fork(mod):
+ assert mod.build_head_ref(False, "", "fix/x") == "fix/x"
+ assert mod.build_head_ref(True, "forkowner", "fix/x") == "forkowner:fix/x"
+
+
+# ---------------------------------------------------------------------------
+# File-based input: values arrive as FILE CONTENT, never as shell/argv strings
+# ---------------------------------------------------------------------------
+
+
+def test_main_reads_title_from_file_and_executes_nothing(mod, tmp_path):
+ sentinel = tmp_path / "sentinel"
+ title_file = tmp_path / "raw-title.txt"
+ title_file.write_text(f"Fix `safe_inference` and $(touch {sentinel})")
+ scratch = tmp_path / "scratch"
+
+ rc = mod.main(
+ [
+ "--title-file",
+ str(title_file),
+ "--change-type",
+ "fix",
+ "--scratch",
+ str(scratch),
+ ]
+ )
+ assert rc == 0
+ assert not sentinel.exists()
+
+ assert (scratch / "pr-title.txt").read_text() == title_file.read_text().strip()
+ branch_out = (scratch / "pr-branch.txt").read_text()
+ assert mod.is_shell_safe_ref(branch_out)
+ assert "`" not in branch_out and "$" not in branch_out
+
+
+def test_main_rejects_unsafe_explicit_branch(mod, tmp_path):
+ title_file = tmp_path / "t.txt"
+ title_file.write_text("ok")
+ branch_file = tmp_path / "b.txt"
+ branch_file.write_text("evil`whoami`")
+ rc = mod.main(
+ [
+ "--title-file",
+ str(title_file),
+ "--branch-file",
+ str(branch_file),
+ "--scratch",
+ str(tmp_path / "s"),
+ ]
+ )
+ assert rc == 2
+
+
+def test_end_to_end_subprocess_no_execution(tmp_path):
+ """The reviewer's requirement: exercise the real process boundary, not just an
+ in-process call. A payload in the title FILE must not execute when the script is
+ run as a subprocess, and the resolved branch must be metacharacter-free."""
+ sentinel = tmp_path / "sentinel"
+ title_file = tmp_path / "raw-title.txt"
+ # Content includes backticks, $(), a semicolon, and a newline.
+ title_file.write_text(f"Fix `id`; $(touch {sentinel})\nsecond line")
+ scratch = tmp_path / "scratch"
+
+ result = subprocess.run(
+ [
+ sys.executable,
+ str(_SCRIPT_PATH),
+ "--title-file",
+ str(title_file),
+ "--change-type",
+ "fix",
+ "--scratch",
+ str(scratch),
+ ],
+ capture_output=True,
+ text=True,
+ )
+ assert result.returncode == 0, result.stderr
+ assert not sentinel.exists(), "payload executed via the subprocess boundary"
+ branch_out = (scratch / "pr-branch.txt").read_text()
+ for ch in "`$();\n":
+ assert ch not in branch_out
+
+
+def _init_repo(path):
+ path.mkdir()
+ subprocess.run(["git", "init", "-q"], cwd=path, check=True)
+ subprocess.run(["git", "config", "user.email", "t@t.t"], cwd=path, check=True)
+ subprocess.run(["git", "config", "user.name", "t"], cwd=path, check=True)
+
+
+def test_no_title_clean_tree_uses_last_commit_subject(mod, tmp_path, monkeypatch):
+ """No-title path must not error or need a base. On a CLEAN tree the last commit
+ subject is the change, so it is the title."""
+ repo = tmp_path / "repo"
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "f.txt").write_text("x")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "feat: my subject line"], cwd=repo, check=True)
+
+ scratch = repo / "s"
+ rc = mod.main(["--scratch", str(scratch)]) # no --title-file at all
+ assert rc == 0
+ assert (scratch / "pr-title.txt").read_text() == "feat: my subject line"
+
+
+def test_no_title_dirty_tree_uses_neutral_not_previous_commit(mod, tmp_path, monkeypatch):
+ """In /submit-pr's normal flow the title is resolved before the commit, so the tree
+ is dirty. The fallback must NOT reuse the previous, unrelated commit subject."""
+ repo = tmp_path / "repo"
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "f.txt").write_text("x")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "OLD unrelated subject"], cwd=repo, check=True)
+ # Now dirty the tree (uncommitted change), as at title-resolution time.
+ (repo / "g.txt").write_text("new work")
+
+ assert mod.generate_fallback_title() == "Update working tree changes"
+ assert "OLD unrelated subject" not in mod.generate_fallback_title()
+
+
+def test_fallback_title_never_empty(mod):
+ assert mod.generate_fallback_title() # non-empty in this repo
+
+
+def test_main_draft_flag_emitted(mod, tmp_path):
+ title_file = tmp_path / "t.txt"
+ title_file.write_text("Add thing")
+ scratch = tmp_path / "s"
+ # Without --draft
+ mod.main(["--title-file", str(title_file), "--scratch", str(scratch)])
+ assert (scratch / "pr-draft.txt").read_text() == "false"
+ # With --draft
+ mod.main(["--title-file", str(title_file), "--draft", "--scratch", str(scratch)])
+ assert (scratch / "pr-draft.txt").read_text() == "true"
diff --git a/tests/test_premerge_scan.py b/tests/test_premerge_scan.py
new file mode 100644
index 000000000..e7212315d
--- /dev/null
+++ b/tests/test_premerge_scan.py
@@ -0,0 +1,336 @@
+"""Tests for .claude/scripts/premerge_scan.py — argv-safe pre-merge scanning.
+
+The property that matters: a git-controlled filename — however hostile — is only ever
+consumed as data, never executed. Pattern checks run in pure Python over file content;
+discovery is NUL-delimited argv; unsafe paths are screened out. The integration tests
+place `$(touch sentinel)` filenames in the working tree (staged AND untracked) and
+assert nothing executes. Skipped when the script is absent (installed distribution).
+"""
+
+import importlib.util
+import pathlib
+import subprocess
+
+import pytest
+
+
+def _find_script():
+ cand = (
+ pathlib.Path(__file__).resolve().parent.parent / ".claude" / "scripts" / "premerge_scan.py"
+ )
+ if cand.exists():
+ return cand
+ try:
+ root = subprocess.check_output(
+ ["git", "rev-parse", "--show-toplevel"], stderr=subprocess.DEVNULL, text=True
+ ).strip()
+ cand = pathlib.Path(root) / ".claude" / "scripts" / "premerge_scan.py"
+ if cand.exists():
+ return cand
+ except (subprocess.CalledProcessError, FileNotFoundError):
+ pass
+ return None
+
+
+_SCRIPT = _find_script()
+pytestmark = pytest.mark.skipif(_SCRIPT is None, reason="premerge_scan.py not found")
+
+
+@pytest.fixture(scope="module")
+def mod():
+ spec = importlib.util.spec_from_file_location("premerge_scan", _SCRIPT)
+ m = importlib.util.module_from_spec(spec) # type: ignore[arg-type]
+ spec.loader.exec_module(m) # type: ignore[union-attr]
+ return m
+
+
+# ---------------------------------------------------------------------------
+# Pure helpers
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+ "path,safe",
+ [
+ ("diff_diff/staggered.py", True),
+ ("diff_diff/visualization/_event_study.py", True),
+ ("tests/test_foo.py", True),
+ ("diff_diff/$(touch x).py", False),
+ ("diff_diff/`whoami`.py", False),
+ ("tests/test_a;b.py", False),
+ ("diff_diff/has space.py", False),
+ ("diff_diff/quote'.py", False),
+ ],
+)
+def test_is_safe_path(mod, path, safe):
+ assert mod.is_safe_path(path) is safe
+
+
+def test_categorize(mod):
+ cats = mod.categorize(
+ [
+ "diff_diff/staggered.py",
+ "diff_diff/__init__.py",
+ "diff_diff/visualization/_event_study.py",
+ "tests/test_staggered.py",
+ "docs/tutorials/01_intro.ipynb",
+ "README.md",
+ ]
+ )
+ assert cats["methodology"] == [
+ "diff_diff/staggered.py",
+ "diff_diff/visualization/_event_study.py",
+ ]
+ assert cats["tests"] == ["tests/test_staggered.py"]
+ assert cats["notebooks"] == ["docs/tutorials/01_intro.ipynb"]
+ assert "README.md" in cats["docs"]
+
+
+def test_check_content_patterns(mod):
+ lines = [
+ "t_stat = effect / se", # A
+ "t_stat = safe_inference(effect, se)", # not A (has safe_inference)
+ "x = 1 if se > 0 else 0.0", # B
+ "compute_confidence_interval(se)", # D (no guard)
+ "if np.isfinite(se): compute_confidence_interval(se)", # not D (guard)
+ ]
+ msgs = [m for _, m in mod.check_content_patterns(lines)]
+ assert any("Check A" in m for m in msgs)
+ assert any("Check B" in m for m in msgs)
+ assert any("Check D" in m for m in msgs)
+ # The guarded/safe_inference lines must NOT produce A or D.
+ assert sum("Check A" in m for m in msgs) == 1
+ assert sum("Check D" in m for m in msgs) == 1
+
+
+def test_new_params_missing_from_get_params_ast(mod):
+ # AST-based: restricted to __init__ self-assigns vs the class's own get_params.
+ text = (
+ "class E:\n"
+ " def __init__(self):\n"
+ " self.new_param = 1\n"
+ " self.kept = 2\n"
+ " def get_params(self):\n"
+ " return {'kept': self.kept}\n"
+ )
+ added = mod.param_names_from_added([" self.new_param = 1", " self.kept = 2"])
+ assert mod.new_params_missing_from_get_params(added, text) == ["new_param"]
+
+
+def test_check_c_ignores_assignments_outside_init(mod):
+ # A self.X assigned in a *non*-__init__ method must not count as a param.
+ text = (
+ "class E:\n"
+ " def __init__(self):\n"
+ " self.real = 1\n"
+ " def fit(self):\n"
+ " self.other = 2\n" # not a param
+ " def get_params(self):\n"
+ " return {'real': self.real}\n"
+ )
+ # 'other' is not in __init__, so it is not a missing param; 'real' is present.
+ assert mod.new_params_missing_from_get_params(["real", "other"], text) == []
+
+
+def test_load_groups(mod):
+ yaml = (
+ "groups:\n"
+ " staggered:\n"
+ " - diff_diff/staggered.py\n"
+ " - diff_diff/staggered_bootstrap.py\n"
+ "\n"
+ "other:\n"
+ )
+ m = mod.load_groups(yaml)
+ assert m["diff_diff/staggered_bootstrap.py"] == "staggered"
+
+
+def test_stem_of(mod):
+ assert mod.stem_of("diff_diff/staggered.py") == "staggered"
+ assert mod.stem_of("diff_diff/_nprobust_port.py") == "nprobust_port" # leading _ dropped
+ assert mod.stem_of("diff_diff/visualization/_event_study.py") == "visualization" # package
+
+
+def test_resolve_tests(mod):
+ tests = [
+ "tests/test_staggered.py",
+ "tests/test_methodology_staggered.py",
+ "tests/test_other.py",
+ ]
+ hits = mod.resolve_tests("staggered", tests)
+ assert hits == ["tests/test_methodology_staggered.py", "tests/test_staggered.py"]
+
+
+# ---------------------------------------------------------------------------
+# Integration: hostile filenames — staged AND untracked — never execute
+# ---------------------------------------------------------------------------
+
+
+def _init_repo(path):
+ subprocess.run(["git", "init", "-q"], cwd=path, check=True)
+ subprocess.run(["git", "config", "user.email", "t@t.t"], cwd=path, check=True)
+ subprocess.run(["git", "config", "user.name", "t"], cwd=path, check=True)
+
+
+def test_hostile_filenames_do_not_execute(mod, tmp_path, monkeypatch):
+ repo = tmp_path / "repo"
+ (repo / "diff_diff").mkdir(parents=True)
+ (repo / "tests").mkdir()
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ # A base commit so `git diff HEAD` works.
+ (repo / "diff_diff" / "ok.py").write_text("x = 1\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+
+ # Sentinel is a bare name in the repo (no slashes), so it can be embedded in a
+ # filename. If any codepath runs `$(touch sentinel)` as shell, the file appears.
+ sentinel = repo / "sentinel"
+ # A STAGED hostile filename and an UNTRACKED hostile filename.
+ staged = repo / "diff_diff" / "$(touch sentinel).py"
+ staged.write_text("t_stat = effect / se\n")
+ subprocess.run(["git", "add", "--", str(staged)], cwd=repo, check=True)
+ untracked = repo / "tests" / "test_`touch sentinel`.py"
+ untracked.write_text("x\n")
+
+ scratch = repo / "s"
+ rc = mod.main(["--scratch", str(scratch)])
+
+ assert not sentinel.exists(), "a hostile filename executed"
+ assert rc == 3, "unsafe paths must make the scan exit non-zero so the caller stops"
+ # The safe run-lists must not contain the hostile names.
+ run_tests = (scratch / "run-tests.z").read_text()
+ assert "touch" not in run_tests
+
+
+def test_clean_repo_returns_zero(mod, tmp_path, monkeypatch):
+ repo = tmp_path / "clean"
+ (repo / "diff_diff").mkdir(parents=True)
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "diff_diff" / "ok.py").write_text("x = 1\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+ (repo / "diff_diff" / "ok.py").write_text("x = 2\n") # a clean change
+
+ rc = mod.main(["--scratch", str(repo / "s")])
+ assert rc == 0
+
+
+def test_changed_test_file_included_in_run_list(mod, tmp_path, monkeypatch):
+ """A test-only change must appear in run-tests.z (DT-1)."""
+ repo = tmp_path / "r"
+ (repo / "tests").mkdir(parents=True)
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "tests" / "test_thing.py").write_text("def test_x(): pass\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+ (repo / "tests" / "test_thing.py").write_text("def test_x(): assert True\n")
+
+ scratch = repo / "s"
+ assert mod.main(["--scratch", str(scratch)]) == 0
+ assert "tests/test_thing.py" in (scratch / "run-tests.z").read_text()
+
+
+def test_committed_range_scans_ahead_commit(mod, tmp_path, monkeypatch, capsys):
+ """--range scans a committed range even when the working tree is clean (CQ-1), and
+ the Check A finding is actually reported (not merely rc==0)."""
+ repo = tmp_path / "r"
+ (repo / "diff_diff").mkdir(parents=True)
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "diff_diff" / "m.py").write_text("x = 1\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+ subprocess.run(["git", "branch", "baseref"], cwd=repo, check=True)
+ (repo / "diff_diff" / "m.py").write_text("t_stat = effect / se\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "work"], cwd=repo, check=True)
+
+ scratch = repo / "s"
+ assert mod.main(["--scratch", str(scratch), "--range", "baseref..HEAD"]) == 0
+ out = capsys.readouterr().out
+ assert "diff_diff/m.py" in out and "Check A" in out
+
+
+def test_untracked_estimator_check_c(mod, tmp_path, monkeypatch, capsys):
+ """An untracked new estimator whose param is absent from get_params() must be
+ flagged by Check C — `git diff HEAD` shows no added lines for it (CQ round-21)."""
+ repo = tmp_path / "r"
+ (repo / "diff_diff").mkdir(parents=True)
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "diff_diff" / "base.py").write_text("x = 1\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+ # UNTRACKED new estimator with a param missing from get_params.
+ (repo / "diff_diff" / "new_est.py").write_text(
+ "class E:\n"
+ " def __init__(self):\n"
+ " self.new_param = 1\n"
+ " def get_params(self):\n"
+ " return {}\n"
+ )
+ assert mod.main(["--scratch", str(repo / "s")]) == 0
+ out = capsys.readouterr().out
+ assert "Check C" in out and "new_param" in out
+
+
+def test_deleted_methodology_file_does_not_crash(mod, tmp_path, monkeypatch):
+ """Deleting a methodology file must not raise/exit 4 (round-21)."""
+ repo = tmp_path / "r"
+ (repo / "diff_diff").mkdir(parents=True)
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "diff_diff" / "gone.py").write_text("x = 1\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+ subprocess.run(["git", "rm", "-q", "diff_diff/gone.py"], cwd=repo, check=True)
+
+ assert mod.main(["--scratch", str(repo / "s")]) == 0
+
+
+def test_conftest_not_a_pytest_target(mod, tmp_path, monkeypatch):
+ """A changed tests/conftest.py must not enter run-tests.z (would exit 5) (DT-1)."""
+ repo = tmp_path / "r"
+ (repo / "tests").mkdir(parents=True)
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "tests" / "conftest.py").write_text("import pytest\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+ (repo / "tests" / "conftest.py").write_text("import pytest # changed\n")
+
+ scratch = repo / "s"
+ assert mod.main(["--scratch", str(scratch)]) == 0
+ assert "conftest.py" not in (scratch / "run-tests.z").read_text()
+
+
+def test_parse_name_status_z(mod):
+ # A modified, an added, a deleted, and a rename (dest → M).
+ toks = ["M", "a.py", "A", "b.py", "D", "c.py", "R100", "old.py", "new.py"]
+ st = mod.parse_name_status_z(toks)
+ assert st == {"a.py": "M", "b.py": "A", "c.py": "D", "new.py": "M"}
+
+
+def test_git_error_fails_closed(mod, tmp_path, monkeypatch):
+ """A bad range (git error) must exit non-zero, not report 0 findings (CQ-2), and
+ must not leave STALE run-lists in the reused scratch dir (CI review P2)."""
+ repo = tmp_path / "r"
+ repo.mkdir()
+ _init_repo(repo)
+ monkeypatch.chdir(repo)
+ (repo / "f.py").write_text("x\n")
+ subprocess.run(["git", "add", "-A"], cwd=repo, check=True)
+ subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True)
+
+ scratch = repo / "s"
+ # Seed a STALE run-list a prior run might have left behind.
+ scratch.mkdir()
+ (scratch / "run-tests.z").write_text("tests/test_unrelated.py")
+
+ rc = mod.main(["--scratch", str(scratch), "--range", "nonexistent-ref..HEAD"])
+ assert rc == 4
+ # The stale list must have been truncated at startup, before the git error.
+ assert (scratch / "run-tests.z").read_text() == ""