diff --git a/CLAUDE.md b/CLAUDE.md index 5b1f4b7..d06c623 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,6 +54,7 @@ Top-level packages under `src/whygraph/`: - `analyze/` — LLM-backed analysis. `description.py` / `llm_descriptor.py` produce per-commit diff descriptions; `rationale.py` / `rationale_generator.py` produce the 5-section rationale cards; `backfill.py` runs the lazy on-read backfill. Prompt templates live under `analyze/prompts/`. - `agents.py` — registry of supported LLM agents (Claude Code, Cursor, VS Code / Copilot, Codex, Claude Desktop) and the per-agent MCP config wiring (`write_snippet` / `render_snippet`). `whygraph init --agent X` reads from here. - `assets.py` + `assets/claude-code/` — bundled Claude Code assets (agents, commands, skills) copied into a project's `.claude/` by `whygraph init --agent claude`. Loaded at runtime via `importlib.resources.files("whygraph") / "assets" / "claude-code"`; same packaging precedent as `analyze/prompts/`. +- `hooks.py` — the auto-rescan git hooks (`post-commit` / `post-merge` / `post-rewrite` / `post-checkout`): the helper script, the sentinel-guarded dispatcher, and `sync_hooks()`. A top-level module for the same reason as `agents.py` / `assets.py` — an installed-by-`init` concern — and deliberately Click-free (it raises `HooksError`, not `ClickException`). - `__main__.py` — enables `python -m whygraph`. Console scripts in `pyproject.toml`: `whygraph` → `cli:main`, `whygraph-mcp` → `mcp.server:main`. Both must keep working — `.mcp.json` files written by `whygraph init` and the `uv tool install` path depend on them. @@ -89,7 +90,13 @@ CodeGraph indexing belongs to **`whygraph scan`, not `whygraph init`** — `init ### Auto-rescan git hooks -`whygraph hooks install` (opt-in; `cli/commands/hooks.py`) wires `post-commit` / `post-merge` / `post-rewrite` to keep the DBs current as the developer works — no daemon. Each hook execs a shared helper (`.whygraph/hooks/whygraph-scan`, gitignored) that runs `whygraph scan --skip-analyze --no-remote` (git history + `codegraph sync` only — fast, offline, no token; LLM descriptions stay on lazy backfill). The helper is **detached** (commits return instantly) and **single-flight + coalescing** (portable `mkdir` lock + a `pending` flag, since macOS has no `flock`), so rapid commits neither stack nor drop the latest `HEAD`. Installs are **sentinel-guarded** (`# >>> whygraph managed >>>`) and append to a foreign hook rather than clobber it. The `--no-remote` flag on `scan` exists for this path; `db/engine.py` sets `PRAGMA busy_timeout` so a background rescan and a manual scan don't collide. +`whygraph init` installs them (`hooks.py`, a top-level module beside `agents.py` / `assets.py` — there is **no** `hooks` CLI command — the group was removed). Four hooks — `post-commit` / `post-merge` / `post-rewrite` / `post-checkout` — keep the DBs current as the developer works, no daemon. Each execs a shared helper (`.whygraph/hooks/whygraph-scan`, gitignored) that runs `whygraph scan --skip-analyze --no-remote` (git history + `codegraph sync` only — fast, offline, no token; LLM descriptions stay on lazy backfill). The helper is **detached** (commits return instantly) and **single-flight + coalescing** (portable `mkdir` lock + a `pending` flag, since macOS has no `flock`), so rapid commits neither stack nor drop the latest `HEAD`. Installs are **sentinel-guarded** (`# >>> whygraph managed >>>`) and append to a foreign hook rather than clobber it. The dispatcher forwards `"$@"` because `post-checkout` is the only hook git invokes with arguments; the helper's arg gate skips a file checkout and a same-commit `git switch -c`. The `--no-remote` flag on `scan` exists for this path; `db/engine.py` sets `PRAGMA busy_timeout` so a background rescan and a manual scan don't collide. + +`[scan].hooks` is the only switch — a bool *or* a list of hook names — and **`init` is the reconciler**: `sync_hooks()` iterates all of `HOOK_NAMES` every run, installing what is listed and stripping the managed block from what is not, so shrinking the list removes the dropped hooks. It is one function rather than an install/uninstall pair precisely so the removal half cannot be forgotten. It is best-effort: an unwritable hooks dir or an unknown hook name warns and `init` still exits 0. + +### Branch membership + +`on_default_branch` is **computed, not assumed**. `Repository.default_branch_refs` resolves the default branch (`origin/HEAD` → `/main` → `/master`, overridable via `[scan].default_branch`) and unions it with the same-named *local* branch, so unpushed commits on local `main` still count. `GitCrawler` flags new rows against that SHA set and records `first_seen_ref` (a branch name, or `refs/pull//head` for a `PROriginEnricher` recovery; `NULL` means it was on the default branch) — and a **reconcile pass** recomputes the flag for existing rows on every scan, so the DB self-heals as branches merge or get rewritten. Two guards make a mass-demotion impossible: an unresolvable default branch and a shallow clone both skip the pass entirely. Rows are never deleted — an unreachable commit is still evidence. `first_seen_ref` has exactly one consumer beyond debugging: the rename-alias walks in `mcp/path_history.py` / `mcp/evidence.py` scope to *default branch **or** current branch*, which keeps an in-flight rename visible without letting an abandoned branch pollute path history forever. Deferred (net-new, not built yet): a project registry for cross-repo orchestration, a persistent/server mode, and per-branch CodeGraph/WhyGraph databases. diff --git a/docs/guide/scanning.md b/docs/guide/scanning.md index 905e7e5..8da4bee 100644 --- a/docs/guide/scanning.md +++ b/docs/guide/scanning.md @@ -47,29 +47,80 @@ whygraph scan --no-remote --skip-analyze ## Keep it fresh -Don't want to re-scan by hand? Install git hooks once, and new commits refresh WhyGraph and CodeGraph -on the fly: +You don't have to re-scan by hand. `whygraph init` installs git hooks that refresh WhyGraph and +CodeGraph in the background as you work - there's no daemon and no separate command to run. -```bash -whygraph hooks install -``` +Four hooks are wired, covering every git event that can change the tree or add commits: + +| Hook | Fires on | +|---|---| +| `post-commit` | `git commit`, `git commit --amend` | +| `post-merge` | `git pull`, `git merge` | +| `post-rewrite` | `git rebase`, including `git pull --rebase` | +| `post-checkout` | `git switch` / `git checkout` to another branch | -This wires `post-commit`, `post-merge`, and `post-rewrite` to run -`whygraph scan --no-remote --skip-analyze` **in the background**. Git history and a CodeGraph -`sync` only - no LLM, no remote calls - so commits stay instant and the scan is offline and -token-free. +Each runs `whygraph scan --no-remote --skip-analyze` **in the background**. Git history and a +CodeGraph `sync` only - no LLM, no remote calls - so commits stay instant and the scan is offline +and token-free. The hooks are detached and single-flight: rapid commits coalesce instead of stacking, and the latest `HEAD` always wins. An existing hook of your own is appended to behind a sentinel guard, never -overwritten. +overwritten. `post-checkout` skips the two cases that can't have changed anything - a file checkout +(`git checkout -- somefile`) and `git switch -c` at the current commit. -Check or remove them any time: +### Choosing which hooks to install -```bash -whygraph hooks status -whygraph hooks uninstall +`[scan].hooks` in `whygraph.toml` governs the set, and **`whygraph init` makes `.git/hooks` match +it exactly**. Edit the value, then re-run `whygraph init` - nothing changes until you do. + +```toml +[scan] +hooks = true # all four (the default) +# hooks = false # none +# hooks = ["post-commit", "post-merge"] # only these two ``` +The reconcile works in **both directions**. Shrinking the list *removes* the hooks you dropped - +you don't have to undo them by hand - and growing it adds them back. Setting `false` strips all +four and deletes the shared helper, leaving any foreign hook content of your own intact. + +Because the setting lives in the committed config, it survives re-runs and applies to everyone who +clones the repo. + !!! note "Hooks stay fast on purpose" The hooks deliberately skip the remote and LLM phases so they never slow a commit. For PRs, issues, and fresh descriptions, run a full `whygraph scan` now and then. + +## How WhyGraph sees branches + +WhyGraph records every commit it walks, but it distinguishes **shipped history** from work in +progress. Each commit row carries `on_default_branch`: `1` when the commit is reachable from the +default branch, `0` when it isn't. + +The default branch is resolved from `origin/HEAD`, falling back to `origin/main` then +`origin/master`, and is judged as the union of that remote-tracking ref *and* your local branch of +the same name - so commits you've made on `main` but haven't pushed still count as shipped. For a +repo on `develop` or `trunk` where `origin/HEAD` isn't set, name it explicitly: + +```toml +[scan] +default_branch = "develop" +``` + +The pre-scan panel shows what it resolved. If it says `unresolved`, branch flagging is off and every +commit is treated as on the default branch - the same behaviour as before this existed. + +**What this means in practice:** unmerged work on a feature branch is excluded by design from +velocity numbers, area history, and the chat statistics surface. It is still recorded, still +searchable, and still evidence - it just isn't counted as shipped. + +Membership is recomputed on **every** scan, so the database self-heals: + +- Merge a branch and the next scan promotes its commits to the default branch. +- Squash-merge it and the originals stay off-branch, correctly - a squash creates a *new* commit. +- Force-push a commit away and the next scan demotes it, with a warning naming the count. The row is + kept: a commit that no longer exists on any branch is still valid evidence for why the code looks + the way it does. + +Shallow clones (`git clone --depth=1`) skip the recompute entirely - a truncated view of history +would otherwise demote nearly everything. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 4af1195..8b096ea 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -7,7 +7,6 @@ own install. There are six commands. $ whygraph --help Commands: analyze Describe a commit's diff with the configured LLM. - hooks Manage opt-in git hooks that auto-rescan on new commits. init Initialize the WhyGraph database under .whygraph/whygraph.db. scan Run the source crawlers, then describe each commit with the LLM. serve Serve the WhyGraph Explorer panel for this repository. @@ -35,6 +34,12 @@ summary that masks every secret, asks *"Write these files?"*, then writes both ` defaulted. `--yes` (and any non-TTY invocation) skips the prompts, uses defaults, and never clobbers an existing `whygraph.toml`. +`init` also installs the auto-rescan git hooks and **reconciles them to `[scan].hooks` in both +directions** - installing what the config lists and stripping the managed block from what it +doesn't. Editing `[scan].hooks` and re-running `whygraph init` is the supported way to change hook +coverage; see [Keep it fresh](../guide/scanning.md#keep-it-fresh). A hooks directory that can't be +written is a warning, never a failed init. + `init` does **not** index CodeGraph. That happens on [`scan`](#whygraph-scan). With `--agent X`, it also wires the WhyGraph MCP server into that agent's config. All supported @@ -104,21 +109,3 @@ whygraph analyze [BASELINE] Every commit named on the command line must already exist in the WhyGraph database. Run `whygraph scan` before `whygraph analyze`. -## `whygraph hooks` - -Manage opt-in git hooks that auto-rescan on new commits. There's no daemon - the hooks run a fast, -background, offline scan as you commit. - -| Subcommand | Description | -|---|---| -| `install` | Install the auto-rescan hooks into the current repository. Idempotent and non-clobbering - it appends to a foreign hook behind a sentinel guard. | -| `status` | Report whether the auto-rescan hooks are installed. | -| `uninstall` | Remove the auto-rescan hooks, leaving any foreign hook content intact. | - -```bash -whygraph hooks install -``` - -The hooks wire `post-commit`, `post-merge`, and `post-rewrite` to run -`whygraph scan --no-remote --skip-analyze` in the background. See -[Keep it fresh](../guide/scanning.md#keep-it-fresh) for the details. diff --git a/docs/reference/index.md b/docs/reference/index.md index c47ccae..33ad51c 100644 --- a/docs/reference/index.md +++ b/docs/reference/index.md @@ -9,7 +9,7 @@ against the running code, so it stays in step with what `whygraph` actually does --- - Every command and flag, straight from `--help`: `init`, `scan`, `analyze`, `hooks`, `version`. + Every command and flag, straight from `--help`: `init`, `scan`, `analyze`, `serve`, `version`. [:octicons-arrow-right-24: CLI reference](cli.md) diff --git a/src/whygraph/chat/stats_sql.py b/src/whygraph/chat/stats_sql.py index 5be1632..719257e 100644 --- a/src/whygraph/chat/stats_sql.py +++ b/src/whygraph/chat/stats_sql.py @@ -114,8 +114,10 @@ def run_stats_query(sql: str, *, db_path: Path | None = None) -> dict: === FIVE REQUIRED RULES (each of these silently corrupts results) === 1. ALWAYS filter `on_default_branch = 1` on the commit table. Rows with 0 are - PR-origin commits recovered from squash merges; counting them double-counts - work that is already on the main walk. + NOT on the default branch — either unmerged local work scanned off a feature + branch, or a PR-origin commit recovered from a squash merge. Neither belongs + in a count of shipped work, and including them double-counts squashed PRs. + `first_seen_ref` tells the two apart if you ever need to. 2. For dates ALWAYS use SQLite's date functions — strftime(), date(), julianday() — and NEVER substr() on a timestamp. Timestamps are TEXT in @@ -187,6 +189,10 @@ def run_stats_query(sql: str, *, db_path: Path | None = None) -> dict: -- formatter sweep. Observed: 201 commits 0-24, -- 13 at 25-49, 5 at 50-74. Not a quality measure. on_default_branch INTEGER 0/1 -- see rule 1 + first_seen_ref TEXT NULL -- NULL = was on the default branch when scanned. + -- Otherwise the ref a flag-0 row came from: a branch + -- name (unmerged local work) or refs/pull//head + -- (a squash-merge recovery). scanned_at TEXT commit_file_change — one row per (commit, path AT THAT COMMIT) diff --git a/src/whygraph/cli/__init__.py b/src/whygraph/cli/__init__.py index 631e5d8..e2e095c 100644 --- a/src/whygraph/cli/__init__.py +++ b/src/whygraph/cli/__init__.py @@ -14,7 +14,6 @@ from whygraph.core import configure_logging, get_config from .commands.analyze import analyze_cmd -from .commands.hooks import hooks_cmd from .commands.init import init_cmd from .commands.install import install_cmd from .commands.scan import scan_cmd @@ -34,5 +33,4 @@ def main() -> None: main.add_command(scan_cmd) main.add_command(serve_cmd) main.add_command(analyze_cmd) -main.add_command(hooks_cmd) main.add_command(install_cmd) diff --git a/src/whygraph/cli/commands/hooks.py b/src/whygraph/cli/commands/hooks.py deleted file mode 100644 index 1c317fc..0000000 --- a/src/whygraph/cli/commands/hooks.py +++ /dev/null @@ -1,250 +0,0 @@ -"""The ``whygraph hooks`` command group — opt-in auto-rescan git hooks. - -Installs ``post-commit`` / ``post-merge`` / ``post-rewrite`` hooks that run -an incremental, offline ``whygraph scan`` (git history + CodeGraph, no LLM, -no remote) in the background whenever the developer adds commits — so the -WhyGraph and CodeGraph databases stay current without a manual scan or a -long-running daemon. - -The hooks are thin dispatchers that exec a shared helper -(``.whygraph/hooks/whygraph-scan``); the helper detaches the scan so commits -return instantly, and uses a portable ``mkdir`` lock plus a ``pending`` flag -so overlapping commits neither stack nor drop the latest ``HEAD``. - -Everything is **opt-in** (never installed by a bare ``whygraph init``) and -**non-clobbering** — managed content lives between sentinel comments, so a -pre-existing foreign hook is appended to, not overwritten. -""" - -from __future__ import annotations - -import re -import subprocess -from pathlib import Path - -import click - -from ..console import console - -SENTINEL = "# >>> whygraph managed >>>" -SENTINEL_END = "# <<< whygraph managed <<<" - -HELPER_RELPATH = Path(".whygraph") / "hooks" / "whygraph-scan" -"""Location of the shared helper, relative to the repo root.""" - -HOOK_NAMES = ("post-commit", "post-merge", "post-rewrite") -"""Git hooks that fire when commits land locally, via merge/pull, or via rebase/amend.""" - -_HELPER_SCRIPT = """\ -#!/bin/sh -# whygraph auto-rescan helper (managed by `whygraph hooks install`). -# After a commit/merge/rebase, runs an incremental, offline scan — git -# history + CodeGraph, no LLM, no remote — detached so the commit returns -# immediately. Single-flight + coalescing so rapid commits don't stack and -# the latest HEAD is never missed. Re-created on reinstall; edits are lost. -command -v whygraph >/dev/null 2>&1 || exit 0 -root=$(git rev-parse --show-toplevel 2>/dev/null) || exit 0 -mkdir -p "$root/.whygraph/logs" -lock="$root/.whygraph/scan.lock" -pending="$root/.whygraph/scan.pending" -log="$root/.whygraph/logs/hooks.log" -: > "$pending" -( - cd "$root" || exit 0 - while [ -e "$pending" ]; do - if mkdir "$lock" 2>/dev/null; then - trap 'rmdir "$lock" 2>/dev/null' EXIT INT TERM - rm -f "$pending" - whygraph scan --skip-analyze --no-remote >> "$log" 2>&1 - rmdir "$lock" 2>/dev/null - trap - EXIT INT TERM - else - # Another run holds the lock; it will see the re-armed pending flag. - break - fi - done -) /dev/null 2>&1 & -exit 0 -""" - -_HOOK_BLOCK = ( - f"{SENTINEL}\n" - 'helper="$(git rev-parse --show-toplevel 2>/dev/null)/.whygraph/hooks/whygraph-scan"\n' - '[ -x "$helper" ] && "$helper"\n' - f"{SENTINEL_END}\n" -) -"""The dispatcher block written into each git hook file.""" - -_BLOCK_RE = re.compile( - re.escape(SENTINEL) + r".*?" + re.escape(SENTINEL_END) + r"\n?", - re.DOTALL, -) - - -@click.group(name="hooks") -def hooks_cmd() -> None: - """Manage opt-in git hooks that auto-rescan on new commits.""" - - -@hooks_cmd.command(name="install") -def install_cmd() -> None: - """Install the auto-rescan hooks into the current repository. - - Idempotent and non-clobbering: writes the shared helper and adds a - sentinel-guarded dispatcher to each of :data:`HOOK_NAMES`, refreshing - an existing managed block in place or appending to a foreign hook. - - Raises - ------ - click.ClickException - If the current directory is not inside a git work tree. - """ - project_root = Path.cwd() - hooks_dir = _git_hooks_dir(project_root) - hooks_dir.mkdir(parents=True, exist_ok=True) - - helper = project_root / HELPER_RELPATH - helper.parent.mkdir(parents=True, exist_ok=True) - helper.write_text(_HELPER_SCRIPT) - helper.chmod(0o755) - console.print(f"Wrote rescan helper: {helper}") - - for name in HOOK_NAMES: - action = _install_hook(hooks_dir / name) - console.print(f" {name}: {action}") - - console.print( - "Auto-rescan hooks installed — new commits refresh WhyGraph + CodeGraph " - "in the background (git + CodeGraph only; run `whygraph scan` for PRs/issues " - "+ LLM descriptions). Remove with `whygraph hooks uninstall`." - ) - - -@hooks_cmd.command(name="uninstall") -def uninstall_cmd() -> None: - """Remove the auto-rescan hooks, leaving any foreign hook content intact. - - Raises - ------ - click.ClickException - If the current directory is not inside a git work tree. - """ - project_root = Path.cwd() - hooks_dir = _git_hooks_dir(project_root) - - removed_any = False - for name in HOOK_NAMES: - if _uninstall_hook(hooks_dir / name): - console.print(f" {name}: removed managed block") - removed_any = True - - helper = project_root / HELPER_RELPATH - if helper.exists(): - helper.unlink() - console.print(f"Removed rescan helper: {helper}") - removed_any = True - - console.print( - "Auto-rescan hooks uninstalled." - if removed_any - else "No WhyGraph hooks were installed." - ) - - -@hooks_cmd.command(name="status") -def status_cmd() -> None: - """Report whether the auto-rescan hooks are installed. - - Raises - ------ - click.ClickException - If the current directory is not inside a git work tree. - """ - project_root = Path.cwd() - hooks_dir = _git_hooks_dir(project_root) - - console.print(f"Hooks dir: {hooks_dir}") - helper = project_root / HELPER_RELPATH - console.print( - f"Helper: {'present' if helper.exists() else 'missing'} ({helper})" - ) - for name in HOOK_NAMES: - hp = hooks_dir / name - if hp.exists() and SENTINEL in hp.read_text(): - state = "managed" - elif hp.exists(): - state = "present (not managed by whygraph)" - else: - state = "missing" - console.print(f" {name}: {state}") - - -def _git_hooks_dir(project_root: Path) -> Path: - """Resolve the repository's hooks directory (worktree-aware). - - Uses ``git rev-parse --git-path hooks`` so the result is correct for - linked worktrees and a custom ``core.hooksPath``. - - Raises - ------ - click.ClickException - If ``git`` reports the directory is not a work tree. - """ - try: - result = subprocess.run( - ["git", "rev-parse", "--git-path", "hooks"], - cwd=project_root, - capture_output=True, - text=True, - check=True, - ) - except (OSError, subprocess.CalledProcessError) as exc: - raise click.ClickException( - "not a git repository (run `whygraph hooks` from inside a repo)" - ) from exc - p = Path(result.stdout.strip()) - return p if p.is_absolute() else (project_root / p) - - -def _install_hook(hook_path: Path) -> str: - """Write or refresh the managed dispatcher in one hook file; return the action taken.""" - if not hook_path.exists(): - hook_path.write_text("#!/bin/sh\n" + _HOOK_BLOCK) - hook_path.chmod(0o755) - return "created" - - text = hook_path.read_text() - if SENTINEL in text: - hook_path.write_text(_BLOCK_RE.sub(_HOOK_BLOCK, text)) - hook_path.chmod(0o755) - return "refreshed managed block" - - sep = "" if text.endswith("\n") else "\n" - hook_path.write_text(text + sep + _HOOK_BLOCK) - hook_path.chmod(0o755) - return "appended to existing hook" - - -def _uninstall_hook(hook_path: Path) -> bool: - """Strip the managed block from one hook file; return ``True`` if anything changed. - - If removing the block leaves only a bare ``#!/bin/sh`` shebang (i.e. the - hook was created by WhyGraph), the file is deleted; otherwise the - foreign remainder is kept. - """ - if not hook_path.exists(): - return False - text = hook_path.read_text() - if SENTINEL not in text: - return False - - stripped = _BLOCK_RE.sub("", text) - if stripped.strip() in ("", "#!/bin/sh"): - hook_path.unlink() - else: - hook_path.write_text(stripped) - hook_path.chmod(0o755) - return True - - -__all__ = ["hooks_cmd"] diff --git a/src/whygraph/cli/commands/init.py b/src/whygraph/cli/commands/init.py index b5ea25a..4dc03cd 100644 --- a/src/whygraph/cli/commands/init.py +++ b/src/whygraph/cli/commands/init.py @@ -126,6 +126,7 @@ def init_cmd( _scaffold_example_config(project_root, answers) _maybe_write_user_config(project_root, answers, write_user=interactive or yes) _ensure_gitignore(project_root) + _sync_hooks(project_root, answers) resolved_agent = answers.agent or agent_name if resolved_agent is None: @@ -175,10 +176,16 @@ def _gather_answers(project_root: Path, agent_name: str | None, *, interactive: Lazy-imports the interactive module so lightweight surfaces stay fast. """ - from whygraph.core.config import DEFAULT_ANSWERS, InitAnswers + from whygraph.core.config import DEFAULT_ANSWERS, InitAnswers, read_hooks_pref if not interactive: - return InitAnswers(agent=agent_name, reconfigure_toml=False) + # Seed hooks from any existing config so a non-interactive re-run + # never resurrects a rejection or widens a deliberate list. + return InitAnswers( + agent=agent_name, + reconfigure_toml=False, + scan_hooks=read_hooks_pref(project_root), + ) from whygraph.cli.interactive import InitAborted, prompt_for_init @@ -257,6 +264,40 @@ def _ensure_gitignore(project_root: Path) -> None: click.echo(".gitignore already covers WhyGraph entries") +def _sync_hooks(project_root: Path, answers) -> None: + """Reconcile the auto-rescan git hooks to ``[scan].hooks``. + + Makes ``.git/hooks`` match the configured value exactly, in both + directions — installing what is listed and stripping the managed + block from what is not, so shrinking the list drops the hooks it + dropped. + + Best-effort: a hooks directory we cannot resolve or write — or an + unknown hook name in the configured list — is a warning, never a + failed init. The DB, config, and agent wiring are already done and + remain valid. Mirrors the CodeGraph refresh contract in + ``scan/codegraph_crawler.py``. + """ + from whygraph import hooks + + try: + names = hooks.resolve_hook_names(answers.scan_hooks) + result = hooks.sync_hooks(project_root, names) + except hooks.HooksError as exc: + click.echo(f"Skipped git hooks — {exc}", err=True) + return + + if result.installed: + click.echo( + f"Installed auto-rescan git hooks: {', '.join(result.installed)}" + " — new commits refresh WhyGraph + CodeGraph in the background" + ) + if result.removed: + click.echo(f"Removed git hooks: {', '.join(result.removed)}") + if not result.installed and not result.removed: + click.echo("No git hooks installed ([scan].hooks is off)") + + def _ensure_db_initialized() -> Path: """Bootstrap the WhyGraph DB, lazy-importing the heavy chain. diff --git a/src/whygraph/cli/commands/scan.py b/src/whygraph/cli/commands/scan.py index e62aab7..cceab33 100644 --- a/src/whygraph/cli/commands/scan.py +++ b/src/whygraph/cli/commands/scan.py @@ -94,7 +94,7 @@ "Crawl the source-control remote (GitHub PRs / issues) per " "`[scan].provider`. `--no-remote` skips it for a fast, offline, " "token-free scan — git history + CodeGraph only. Used by the " - "auto-rescan git hooks (`whygraph hooks install`). Default: on." + "auto-rescan git hooks installed by `whygraph init`. Default: on." ), ) @click.option( @@ -130,7 +130,11 @@ def scan_cmd( db_path = ensure_initialized() config = get_config() - repository = Repository(Path.cwd(), origin_remote=config.scan_remote) + repository = Repository( + Path.cwd(), + origin_remote=config.scan_remote, + default_branch=config.scan_default_branch, + ) if remote: _apply_github_token(config) github_client = _select_github_client(config.scan_provider, repository) @@ -271,8 +275,10 @@ def scan_cmd( ) # ── Phase 4 · LLM descriptions — the slow, token-heavy long pole, - # run strictly last and alone. Only ever describes main-walk - # commits, so the recovered on_default_branch=0 rows stay lazy. ── + # run strictly last and alone. It describes commits reachable from + # the current branch — reachability, not on_default_branch — so + # PR-origin rows stay lazy because their objects are unreachable, + # while feature-branch commits are described like any other. ── if run_analyze: n += 1 console.rule( @@ -404,10 +410,18 @@ def _timing(title: str) -> str: # Structural row — git + GitHub combined into one phase row. structural = [c for c in (git, github) if c is not None] structural_summary = " · ".join(c.summary for c in structural if c.summary) or "—" + # A bulk demotion is the one way rows can *leave* the default-branch + # queries, so it gets the same ⚠ treatment as a CodeGraph skip rather + # than hiding inside the summary counts. + git_warning = getattr(git, "warning", None) + if git_warning: + structural_summary = f"{structural_summary}\n⚠ {git_warning}" grid.add_row( _ICON_STRUCTURAL, "Structural crawl", - _status_glyph(ok=all(c.error is None for c in structural)), + _status_glyph( + ok=all(c.error is None for c in structural), warn=bool(git_warning) + ), structural_summary, _timing("Structural crawl"), ) @@ -561,6 +575,7 @@ def _render_scan_panel( rows: list[tuple[str, object]] = [ ("Repository", repo_label), ("Branch", str(branch) if branch is not None else "unknown"), + ("Default branch", _default_branch_label(repository)), ("Database", str(db_path)), ( "CodeGraph", @@ -640,6 +655,26 @@ def _github_skip_reason(config: "Config", remote_enabled: bool = True) -> str: return f"skipped — {config.scan_remote!r} remote is not a GitHub remote" +def _default_branch_label(repository: "Repository") -> object: + """Render the pre-scan panel's **Default branch** row. + + Reports the refs branch membership will be judged against, or — when + nothing resolves — says so in yellow and names the config key that + fixes it. Silent degradation is the failure mode that wastes an + afternoon: with no default branch resolved, every commit is flagged + as on it, and nothing else in the output would hint why. + """ + refs = _best_effort(lambda: repository.default_branch_refs) + if refs is None: + return Text("unavailable", style="yellow") + if not refs: + return Text( + "unresolved — branch flagging disabled; set [scan].default_branch", + style="yellow", + ) + return ", ".join(refs) + + def _best_effort(fn: "Callable[[], _T]") -> "_T | None": """Run ``fn``; return its result, or ``None`` if it raised. diff --git a/src/whygraph/cli/interactive.py b/src/whygraph/cli/interactive.py index b39d381..4497fd2 100644 --- a/src/whygraph/cli/interactive.py +++ b/src/whygraph/cli/interactive.py @@ -40,6 +40,7 @@ OllamaConfig, OpenAIConfig, OpenRouterConfig, + read_hooks_pref, ) # Provider tags (hyphen form — matches the LLM factory tag). @@ -328,6 +329,31 @@ def _prompt_scan(prompter: Prompter) -> tuple[str, str | None]: return scan_provider, scan_token +def _prompt_hooks( + prompter: Prompter, existing: bool | tuple[str, ...] +) -> bool | tuple[str, ...]: + """Prompt for the auto-rescan git hooks, seeded from the existing config. + + A configured **list** skips the prompt entirely and is returned + verbatim: the list is an advanced, config-file-only shape, and a + Yes/No answer must never silently widen ``["post-commit"]`` back to + all four. The prompt runs only when the existing value is a bool or + absent, and defaults to that value — so a prior ``hooks = false`` is + not resurrected by a stray Enter. + """ + if not isinstance(existing, bool): + return existing + return bool( + _require( + prompter.confirm( + "Install git hooks that re-scan in the background after " + "commits, pulls and branch switches?", + default=existing, + ) + ) + ) + + def prompt_for_init( project_root: Path, *, @@ -389,13 +415,18 @@ def prompt_for_init( # does not touch whygraph.toml). agent = _prompt_agent(prompter, preset_agent) - # Steps 2-7 — only when (re)configuring whygraph.toml. + # Hook coverage is seeded from any existing config, so a prior opt-out + # or a deliberately narrowed list survives a re-run. + existing_hooks = read_hooks_pref(project_root) + + # Steps 2-8 — only when (re)configuring whygraph.toml. if reconfigure: analyze_provider, analyze_model, rationale_provider, rationale_model = ( _prompt_llm(prompter) ) api_keys = _prompt_api_keys(prompter, analyze_provider, rationale_provider) scan_provider, scan_token = _prompt_scan(prompter) + scan_hooks = _prompt_hooks(prompter, existing_hooks) answers = InitAnswers( agent=agent, analyze_provider=analyze_provider, @@ -405,12 +436,15 @@ def prompt_for_init( api_keys=api_keys, scan_provider=scan_provider, scan_token=scan_token, + scan_hooks=scan_hooks, reconfigure_toml=True, ) else: - # Keep the existing whygraph.toml; still refresh the example and - # wire the agent. - answers = InitAnswers(agent=agent, reconfigure_toml=False) + # Keep the existing whygraph.toml; still refresh the example, wire + # the agent, and reconcile hooks to what that config already says. + answers = InitAnswers( + agent=agent, scan_hooks=existing_hooks, reconfigure_toml=False + ) # Step 8 — review & confirm (the single gate before any write). will_write_user = (not user_path.exists()) or answers.reconfigure_toml diff --git a/src/whygraph/core/config.py b/src/whygraph/core/config.py index ee3f52f..4b2ca14 100644 --- a/src/whygraph/core/config.py +++ b/src/whygraph/core/config.py @@ -407,6 +407,37 @@ class LlmConfig: ) +def _parse_hooks(value: object) -> bool | tuple[str, ...]: + """Normalize the *shape* of ``[scan].hooks``. + + Accepts a bool verbatim, or a list of strings as a tuple (so the + frozen :class:`Config` stays hashable). An empty list collapses to + ``False`` — "install none" has one representation downstream. + + Hook **names** are deliberately not validated here: that would mean + importing :mod:`whygraph.hooks` from ``core``, inverting the + dependency direction of the cross-cutting leaf package. + :func:`whygraph.hooks.resolve_hook_names` validates at the point of + use, and ``whygraph init`` — the only command that acts on the value + — surfaces a typo as a warning. + + Raises + ------ + ConfigError + If the value is neither a bool nor a list of strings. + """ + if isinstance(value, bool): + return value + if isinstance(value, list): + if not all(isinstance(item, str) for item in value): + raise ConfigError("[scan].hooks list entries must all be strings") + return tuple(value) or False + raise ConfigError( + f"[scan].hooks must be a bool or a list of hook names, " + f"got {type(value).__name__}" + ) + + def _build_llm_config(raw: dict) -> LlmConfig: """Parse a raw ``[llm]`` dict into a typed :class:`LlmConfig`.""" sections: dict[str, object] = {} @@ -504,6 +535,22 @@ class Config: ambient ``GH_TOKEN`` / ``GITHUB_TOKEN`` environment variables (or an existing ``gh auth login`` session). Kept per-project so one shared scanning container can serve repos across different orgs. + scan_hooks : bool or tuple[str, ...] + Which auto-rescan git hooks ``whygraph init`` keeps installed. + ``True`` (default) → all of + :data:`whygraph.hooks.HOOK_NAMES`; ``False`` or an empty list → + none; a list of names → exactly those, with the rest removed. + Loaded from ``[scan].hooks``. Only the *shape* is validated here; + the names are checked by + :func:`whygraph.hooks.resolve_hook_names` at the point of use, so + ``core`` keeps no dependency on the hooks module. + scan_default_branch : str or None + Override the branch WhyGraph treats as shipped history, e.g. + ``"develop"``. Loaded from ``[scan].default_branch``; an empty + value is treated as ``None``, which auto-resolves from + ``origin/HEAD`` then ``origin/main`` / ``origin/master``. An + unresolvable value is not an error — it degrades to "cannot + judge" and is reported in the scan panel. whygraph_db : Path or None Override path to the WhyGraph SQLite DB. If ``None``, callers use the project-relative default ``.whygraph/whygraph.db``. @@ -538,6 +585,8 @@ class Config: scan_provider: str = "off" scan_remote: str = "origin" scan_token: str | None = None + scan_hooks: bool | tuple[str, ...] = True + scan_default_branch: str | None = None whygraph_db: Path | None = None codegraph_db: Path | None = None llm: LlmConfig = field(default_factory=LlmConfig) @@ -669,6 +718,11 @@ def from_toml(cls, path: Path) -> Config: if "token" in scan: token = (scan.pop("token") or "").strip() raw["scan_token"] = token or None + if "hooks" in scan: + raw["scan_hooks"] = _parse_hooks(scan.pop("hooks")) + if "default_branch" in scan: + branch = (scan.pop("default_branch") or "").strip() + raw["scan_default_branch"] = branch or None for unknown in scan: _log.warning("ignoring unknown key in [scan]: %r", unknown) @@ -754,6 +808,11 @@ class InitAnswers: scan_token : str or None Value for ``[scan].token``; rendered active **only** into ``whygraph.toml`` when present. + scan_hooks : bool or tuple[str, ...] + Value for ``[scan].hooks`` — which auto-rescan git hooks ``init`` + keeps installed. Rendered into **both** TOMLs, because the + written value is what the *next* ``init`` reads back: a hard-coded + literal here would resurrect a rejection the user just made. reconfigure_toml : bool ``True`` when the command should (over)write ``whygraph.toml``. ``False`` (default, and always in non-interactive runs) preserves @@ -768,6 +827,7 @@ class InitAnswers: api_keys: dict[str, str] = field(default_factory=dict) scan_provider: str = "off" scan_token: str | None = None + scan_hooks: bool | tuple[str, ...] = True reconfigure_toml: bool = False @@ -837,6 +897,20 @@ def _key_line(provider: str, answers: InitAnswers, include_tokens: bool) -> str: return _LLM_KEY_HINTS[provider] +def _render_hooks_value(value: bool | tuple[str, ...]) -> str: + """Render a ``[scan].hooks`` value as TOML: ``true``, ``false``, or an array. + + This must round-trip: ``whygraph init`` writes the file that the + *next* ``whygraph init`` reads back to decide whether to install. A + hard-coded ``hooks = true`` in the template would mean a user who + declined hooks gets a config claiming they wanted them, and the next + run silently reinstalls. + """ + if isinstance(value, bool): + return "true" if value else "false" + return "[" + ", ".join(f'"{name}"' for name in value) + "]" + + def render_config(answers: InitAnswers, *, include_tokens: bool) -> str: """Render ``whygraph.toml`` text from ``answers``. @@ -869,6 +943,7 @@ def render_config(answers: InitAnswers, *, include_tokens: bool) -> str: """ subs = { "scan_provider": answers.scan_provider, + "scan_hooks": _render_hooks_value(answers.scan_hooks), "analyze_provider": answers.analyze_provider, "rationale_provider": answers.rationale_provider, # Chat is not prompted for by `whygraph init` — the [chat] block @@ -914,6 +989,41 @@ def default_config_text() -> str: return render_config(DEFAULT_ANSWERS, include_tokens=False) +def read_hooks_pref( + project_root: Path, *, default: bool | tuple[str, ...] = True +) -> bool | tuple[str, ...]: + """Read ``[scan].hooks`` from an existing ``whygraph.toml``. + + Seeds ``whygraph init``'s hook reconcile so a prior opt-out is never + resurrected and a deliberately narrowed list is never widened — both + paths (interactive prompt and ``--yes``) start from the same value. + + Best-effort by design: a missing, unreadable, or invalid config + yields ``default`` rather than raising. ``init`` must not fail + because of a config it is about to rewrite. + + Parameters + ---------- + project_root : Path + Directory holding ``whygraph.toml``. + default : bool or tuple[str, ...], optional + Value to return when no usable preference is found. Default + ``True`` (install every hook). + + Returns + ------- + bool or tuple[str, ...] + The configured preference, or ``default``. + """ + path = project_root / CONFIG_FILENAME + if not path.exists(): + return default + try: + return Config.from_toml(path).scan_hooks + except (OSError, ConfigError, tomllib.TOMLDecodeError): + return default + + def write_example_config( project_root: Path, answers: InitAnswers = DEFAULT_ANSWERS ) -> Path: diff --git a/src/whygraph/core/default_config.toml.tmpl b/src/whygraph/core/default_config.toml.tmpl index 01e3a81..74584cf 100644 --- a/src/whygraph/core/default_config.toml.tmpl +++ b/src/whygraph/core/default_config.toml.tmpl @@ -19,6 +19,17 @@ ${scan_token_line} # token — handy when one shared container scans repos # across different orgs. whygraph.toml is gitignored, so a # token here is never committed. +# Auto-rescan git hooks, installed by `whygraph init`: +# true — all four: post-commit, post-merge, post-rewrite, post-checkout +# false — none (init removes any already installed) +# [list] — only these, e.g. ["post-commit", "post-merge"] +# They run a fast offline scan in the background so WhyGraph and CodeGraph +# track your commits. `whygraph init` makes .git/hooks match this value +# exactly — edit and re-run it to add or drop hooks. +hooks = ${scan_hooks} +# default_branch = "main" # override the branch WhyGraph treats as "shipped history". + # Default: resolved from origin/HEAD, else origin/main, + # else origin/master. Set for repos on develop / trunk. [analyze] # LLM that writes a per-commit "git diff" description during `whygraph scan`. diff --git a/src/whygraph/db/migrations/versions/e4c1b9d72f3a_add_first_seen_ref_to_commit.py b/src/whygraph/db/migrations/versions/e4c1b9d72f3a_add_first_seen_ref_to_commit.py new file mode 100644 index 0000000..3f553aa --- /dev/null +++ b/src/whygraph/db/migrations/versions/e4c1b9d72f3a_add_first_seen_ref_to_commit.py @@ -0,0 +1,43 @@ +"""add first_seen_ref to commit + +Revision ID: e4c1b9d72f3a +Revises: c7d4a1e8b3f2 +Create Date: 2026-08-03 10:00:00.000000 + +Adds the ``first_seen_ref`` provenance column on ``commit``. NULL — the +value every existing row gets — means "was on the default branch when +first scanned". A non-NULL value records the ref an off-default-branch +commit was first seen on: a local branch name for work scanned off a +feature branch, or ``refs/pull//head`` for a commit recovered by the +squash-origin enricher. Additive and nullable, so no data migration is +needed; the first post-upgrade scan's reconcile pass recomputes every +``on_default_branch`` value in one sweep. +""" + +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = "e4c1b9d72f3a" +down_revision: Union[str, Sequence[str], None] = "c7d4a1e8b3f2" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema. + + A plain ``add_column`` (native ``ALTER TABLE ADD COLUMN``) rather than a + batch recreate: SQLite adds a nullable column in place, and recreating + ``commit`` would trip the foreign key from ``commit_file_change`` on a + populated DB. + """ + op.add_column("commit", sa.Column("first_seen_ref", sa.Text(), nullable=True)) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_column("commit", "first_seen_ref") diff --git a/src/whygraph/db/models/commit.py b/src/whygraph/db/models/commit.py index 5edfc6e..0a9fc03 100644 --- a/src/whygraph/db/models/commit.py +++ b/src/whygraph/db/models/commit.py @@ -9,17 +9,20 @@ class Commit(WhygraphTable, table=True): - """One row per scanned Git commit (first-parent walk of the default branch). + """One row per scanned Git commit (reachable from the default branch). Notes ----- * ``on_default_branch`` is ``int`` (0/1), not ``bool``, to keep the declared SQLite affinity as INTEGER (same rationale as - :attr:`whygraph.db.models.PullRequest.draft`). ``1`` (default) marks a - commit on the first-parent main walk; ``0`` marks a PR-origin commit - recovered from a squash-merged PR (see ``scan/pr_origin_enricher.py``) - that must stay out of the main-walk-only queries (area-history, - refactor-walk). + :attr:`whygraph.db.models.PullRequest.draft`). ``1`` marks a commit + reachable from the default branch; ``0`` marks one that is not — + either unmerged local work scanned off a feature branch, or a + PR-origin commit recovered from a squash-merged PR (see + ``scan/pr_origin_enricher.py``). Both must stay out of the + default-branch-only queries (area-history, refactor-walk). + * ``first_seen_ref`` discriminates those two populations; see the + field comment. """ sha: str = Field(primary_key=True, nullable=True, sa_type=Text) @@ -41,8 +44,16 @@ class Commit(WhygraphTable, table=True): # uses it to drive ``git blame --ignore-rev`` walk-past so older # authorship surfaces through commits that would otherwise mask it. refactor_score: int = Field(default=0) - # 0 = PR-origin commit recovered from a squash-merged PR (not on the - # first-parent main walk); 1 = on the default-branch walk (the norm). + # 0 = not reachable from the default branch (unmerged local work, or a + # PR-origin commit recovered from a squash-merged PR); 1 = reachable + # (the norm). Recomputed on every scan by GitCrawler's reconcile pass. on_default_branch: int = Field( default=1, sa_column_kwargs={"server_default": text("1")} ) + # Ref this commit was first seen on when it was NOT on the default + # branch: a local branch name, or refs/pull//head for a PR-origin + # recovery. NULL means it was on the default branch when first scanned. + # Written once at insert and never rewritten — including by a 1 -> 0 + # demotion, where NULL correctly reads as "was on the default branch, + # no longer reachable". + first_seen_ref: str | None = Field(default=None, sa_type=Text) diff --git a/src/whygraph/hooks.py b/src/whygraph/hooks.py new file mode 100644 index 0000000..95f869a --- /dev/null +++ b/src/whygraph/hooks.py @@ -0,0 +1,322 @@ +"""Auto-rescan git hooks, installed by ``whygraph init``. + +Installs ``post-commit`` / ``post-merge`` / ``post-rewrite`` / +``post-checkout`` hooks that run an incremental, offline ``whygraph scan`` +(git history + CodeGraph, no LLM, no remote) in the background whenever +the developer commits, pulls, rebases, or switches branch — so the +WhyGraph and CodeGraph databases stay current without a manual scan or a +long-running daemon. + +The hooks are thin dispatchers that exec a shared helper +(``.whygraph/hooks/whygraph-scan``); the helper detaches the scan so +commits return instantly, and uses a portable ``mkdir`` lock plus a +``pending`` flag so overlapping git events neither stack nor drop the +latest ``HEAD``. ``post-checkout`` is the one hook git invokes with +arguments, so the dispatcher forwards ``"$@"`` and the helper filters out +the two cases that cannot have changed the tree. + +Hook coverage is governed by ``[scan].hooks`` and reconciled by +``whygraph init`` — see :func:`sync_hooks`. Managed content lives between +sentinel comments, so a pre-existing foreign hook is appended to, not +overwritten. + +This is a top-level module (like ``agents.py`` and ``assets.py``) rather +than a CLI command: it is an installed-by-``init`` concern, and it must +not depend on Click. +""" + +from __future__ import annotations + +import re +import subprocess +from collections.abc import Sequence +from dataclasses import dataclass +from pathlib import Path + +SENTINEL = "# >>> whygraph managed >>>" +SENTINEL_END = "# <<< whygraph managed <<<" + +HELPER_RELPATH = Path(".whygraph") / "hooks" / "whygraph-scan" +"""Location of the shared helper, relative to the repo root.""" + +HOOK_NAMES = ("post-commit", "post-merge", "post-rewrite", "post-checkout") +"""Every hook WhyGraph manages — the reconcile set. + +:func:`sync_hooks` considers **all** of these on every call, installing +the ones it is given and stripping the managed block from the rest. The +four cover every git event that can change the worktree or add commits: +``post-commit`` (commit, amend), ``post-merge`` (pull, merge), +``post-rewrite`` (rebase), ``post-checkout`` (branch switch). +""" + +_HELPER_SCRIPT = """\ +#!/bin/sh +# whygraph auto-rescan helper (managed by `whygraph init`). +# After a commit/merge/rebase/checkout, runs an incremental, offline scan — +# git history + CodeGraph, no LLM, no remote — detached so the git command +# returns immediately. Single-flight + coalescing so rapid commits don't +# stack and the latest HEAD is never missed. Re-created on every +# `whygraph init`; edits are lost. +command -v whygraph >/dev/null 2>&1 || exit 0 +root=$(git rev-parse --show-toplevel 2>/dev/null) || exit 0 +# post-checkout is the only hook invoked with 3 args: . +if [ "$#" -eq 3 ]; then + [ "$3" = "1" ] || exit 0 # file checkout (`git checkout -- path`) — nothing changed + [ "$1" != "$2" ] || exit 0 # `git switch -c` at the same commit — identical tree +fi +mkdir -p "$root/.whygraph/logs" +lock="$root/.whygraph/scan.lock" +pending="$root/.whygraph/scan.pending" +log="$root/.whygraph/logs/hooks.log" +: > "$pending" +( + cd "$root" || exit 0 + while [ -e "$pending" ]; do + if mkdir "$lock" 2>/dev/null; then + trap 'rmdir "$lock" 2>/dev/null' EXIT INT TERM + rm -f "$pending" + whygraph scan --skip-analyze --no-remote >> "$log" 2>&1 + rmdir "$lock" 2>/dev/null + trap - EXIT INT TERM + else + # Another run holds the lock; it will see the re-armed pending flag. + break + fi + done +) /dev/null 2>&1 & +exit 0 +""" + +_HOOK_BLOCK = ( + f"{SENTINEL}\n" + 'helper="$(git rev-parse --show-toplevel 2>/dev/null)/.whygraph/hooks/whygraph-scan"\n' + '[ -x "$helper" ] && "$helper" "$@"\n' + f"{SENTINEL_END}\n" +) +"""The dispatcher block written into each git hook file. + +Forwards ``"$@"`` because ``post-checkout`` carries +`` ``; the other three hooks +pass zero or one argument and the helper's arg gate ignores those. +""" + +_BLOCK_RE = re.compile( + re.escape(SENTINEL) + r".*?" + re.escape(SENTINEL_END) + r"\n?", + re.DOTALL, +) + + +class HooksError(RuntimeError): + """The hooks directory cannot be resolved or written, or a name is unknown.""" + + +@dataclass(frozen=True) +class HooksResult: + """What :func:`sync_hooks` did. + + Attributes + ---------- + helper : Path or None + Where the shared helper was written, or ``None`` when every hook + was removed and the helper deleted. + actions : dict[str, str] + Per-hook outcome, keyed by hook name and covering all of + :data:`HOOK_NAMES`. One of ``"created"``, ``"refreshed"``, + ``"appended"``, ``"removed"``, or ``"absent"``. + """ + + helper: Path | None + actions: dict[str, str] + + @property + def installed(self) -> tuple[str, ...]: + """Hook names that now carry the managed block.""" + return tuple( + name + for name, action in self.actions.items() + if action in ("created", "refreshed", "appended") + ) + + @property + def removed(self) -> tuple[str, ...]: + """Hook names whose managed block was stripped by this call.""" + return tuple( + name for name, action in self.actions.items() if action == "removed" + ) + + +def resolve_hook_names(value: bool | Sequence[str]) -> tuple[str, ...]: + """Normalize a ``[scan].hooks`` value to concrete hook names. + + Parameters + ---------- + value : bool or Sequence[str] + ``True`` → all of :data:`HOOK_NAMES`; ``False`` or an empty + sequence → none; a sequence of names → exactly those. + + Returns + ------- + tuple[str, ...] + The hooks to keep installed, in :data:`HOOK_NAMES` order so the + result is stable regardless of how the config listed them. + + Raises + ------ + HooksError + If a name is not one of :data:`HOOK_NAMES`. Validation lives here + rather than in ``core.config`` so the cross-cutting ``core`` + package keeps no dependency on this module. + """ + if isinstance(value, bool): + return HOOK_NAMES if value else () + unknown = [name for name in value if name not in HOOK_NAMES] + if unknown: + raise HooksError( + f"unknown hook name(s): {', '.join(sorted(unknown))} " + f"(valid: {', '.join(HOOK_NAMES)})" + ) + wanted = set(value) + return tuple(name for name in HOOK_NAMES if name in wanted) + + +def sync_hooks(project_root: Path, names: Sequence[str]) -> HooksResult: + """Reconcile the repo's git hooks to exactly ``names``. + + Installs or refreshes the managed block in each named hook, and + **strips it from every hook in** :data:`HOOK_NAMES` **that is not + named** — so shrinking the configured list removes the dropped hooks + rather than orphaning them. Writes the shared helper when ``names`` + is non-empty and deletes it when empty; ``sync_hooks(root, ())`` is + therefore the uninstall. Foreign hook content is never touched. + + Both directions live in one function deliberately: the removal half + is the part that is easy to forget on one branch of an + install/uninstall pair, and folding them together makes omitting it + structurally impossible. + + Parameters + ---------- + project_root : Path + The repository working tree. + names : Sequence[str] + Hook names to keep installed — normally the output of + :func:`resolve_hook_names`. + + Returns + ------- + HooksResult + The helper path (or ``None``) and the per-hook action taken. + + Raises + ------ + HooksError + If the hooks directory cannot be resolved or written. + """ + hooks_dir = _git_hooks_dir(project_root) + wanted = set(names) + + helper: Path | None = None + try: + if wanted: + hooks_dir.mkdir(parents=True, exist_ok=True) + helper = project_root / HELPER_RELPATH + helper.parent.mkdir(parents=True, exist_ok=True) + helper.write_text(_HELPER_SCRIPT) + helper.chmod(0o755) + + actions: dict[str, str] = {} + for name in HOOK_NAMES: + path = hooks_dir / name + if name in wanted: + actions[name] = _install_hook(path) + else: + actions[name] = "removed" if _uninstall_hook(path) else "absent" + + if not wanted: + stale = project_root / HELPER_RELPATH + if stale.exists(): + stale.unlink() + except OSError as exc: + raise HooksError(f"cannot write git hooks under {hooks_dir}: {exc}") from exc + + return HooksResult(helper=helper, actions=actions) + + +def _git_hooks_dir(project_root: Path) -> Path: + """Resolve the repository's hooks directory (worktree-aware). + + Uses ``git rev-parse --git-path hooks`` so the result is correct for + linked worktrees and a custom ``core.hooksPath``. + + Raises + ------ + HooksError + If ``git`` reports the directory is not a work tree. + """ + try: + result = subprocess.run( + ["git", "rev-parse", "--git-path", "hooks"], + cwd=project_root, + capture_output=True, + text=True, + check=True, + ) + except (OSError, subprocess.CalledProcessError) as exc: + raise HooksError( + f"not a git repository ({project_root}) — no hooks to manage" + ) from exc + p = Path(result.stdout.strip()) + return p if p.is_absolute() else (project_root / p) + + +def _install_hook(hook_path: Path) -> str: + """Write or refresh the managed dispatcher in one hook file; return the action.""" + if not hook_path.exists(): + hook_path.write_text("#!/bin/sh\n" + _HOOK_BLOCK) + hook_path.chmod(0o755) + return "created" + + text = hook_path.read_text() + if SENTINEL in text: + hook_path.write_text(_BLOCK_RE.sub(_HOOK_BLOCK, text)) + hook_path.chmod(0o755) + return "refreshed" + + sep = "" if text.endswith("\n") else "\n" + hook_path.write_text(text + sep + _HOOK_BLOCK) + hook_path.chmod(0o755) + return "appended" + + +def _uninstall_hook(hook_path: Path) -> bool: + """Strip the managed block from one hook file; return ``True`` if anything changed. + + If removing the block leaves only a bare ``#!/bin/sh`` shebang (i.e. the + hook was created by WhyGraph), the file is deleted; otherwise the + foreign remainder is kept. + """ + if not hook_path.exists(): + return False + text = hook_path.read_text() + if SENTINEL not in text: + return False + + stripped = _BLOCK_RE.sub("", text) + if stripped.strip() in ("", "#!/bin/sh"): + hook_path.unlink() + else: + hook_path.write_text(stripped) + hook_path.chmod(0o755) + return True + + +__all__ = [ + "HELPER_RELPATH", + "HOOK_NAMES", + "SENTINEL", + "SENTINEL_END", + "HooksError", + "HooksResult", + "resolve_hook_names", + "sync_hooks", +] diff --git a/src/whygraph/mcp/evidence.py b/src/whygraph/mcp/evidence.py index f5e6d6f..b1dcc6a 100644 --- a/src/whygraph/mcp/evidence.py +++ b/src/whygraph/mcp/evidence.py @@ -425,18 +425,21 @@ def _rename_events_for(path: str) -> list[tuple[str, str]]: """Return ``(rename_commit_sha, predecessor_path)`` for every rename in path's lineage.""" # Lazy import: path_history reuses _linked_prs / _linked_issues from # this module, so eager imports would create a cycle. - from .path_history import resolve_path_aliases + from .path_history import branch_scope, current_branch_scope, resolve_path_aliases + current_branch = current_branch_scope() out: list[tuple[str, str]] = [] with get_session() as session: - aliases = resolve_path_aliases(session, path) + aliases = resolve_path_aliases(session, path, current_branch=current_branch) if not aliases: return [] rows = session.exec( select(CommitFileChange.commit_sha, CommitFileChange.renamed_from) + .join(Commit, col(Commit.sha) == col(CommitFileChange.commit_sha)) .where(col(CommitFileChange.path).in_(aliases)) .where(col(CommitFileChange.change_type).in_(("R", "C"))) .where(col(CommitFileChange.renamed_from).is_not(None)) + .where(branch_scope(current_branch)) ).all() for row in rows: sha = row[0] diff --git a/src/whygraph/mcp/path_history.py b/src/whygraph/mcp/path_history.py index 344f27c..443dea7 100644 --- a/src/whygraph/mcp/path_history.py +++ b/src/whygraph/mcp/path_history.py @@ -29,9 +29,71 @@ from whygraph.core.utils import LIKE_ESCAPE_CHAR, like_escape from whygraph.db import get_session from whygraph.db.models import Commit, CommitFileChange +from whygraph.services.git import GitError, Repository +from .targets import repo_root -def resolve_path_aliases(session: Session, path: str) -> set[str]: + +def branch_scope(current_branch: str | None): + """SQL predicate for the commits an alias walk may see. + + The default branch, **plus** the branch you are standing on. Neither + extreme is right: filtering to the default branch alone blinds + WhyGraph to the rename you are making *right now* — close to the most + valuable moment for a rationale lookup — while no filter at all lets + an abandoned branch's renames pollute every path query forever. + + Self-cleaning, so no expiry is needed: merging promotes the rows to + ``on_default_branch = 1`` and they stay visible on their own merit, + and switching away drops the old branch's aliases immediately. + + Parameters + ---------- + current_branch : str or None + The checked-out branch, or ``None`` to scope to the default + branch only — the safe direction, used for a detached HEAD and + whenever git cannot be reached. + + Returns + ------- + ColumnElement[bool] + A predicate over the ``commit`` table; the caller must have + joined it. + """ + on_default = col(Commit.on_default_branch) == 1 + if current_branch is None: + return on_default + return on_default | (col(Commit.first_seen_ref) == current_branch) + + +def current_branch_scope() -> str | None: + """The checked-out branch, or ``None`` when it should not widen the scope. + + Returns ``None`` on a detached HEAD — :attr:`Repository.current_branch` + yields the literal ``"HEAD"`` there, which as a ``first_seen_ref`` + value would union in every commit ever scanned from a detached head — + and on any :class:`GitError`. Both degrade to default-branch-only. + + Deliberately **uncached**: the MCP server is long-lived and outlives + branch switches, so a cached value would serve exactly the stale + aliases this scoping exists to prevent. One + ``git rev-parse --abbrev-ref HEAD`` per tool call (~5 ms). + + Returns + ------- + str or None + The branch name, or ``None``. + """ + try: + branch = Repository(repo_root()).current_branch + except GitError: + return None + return None if branch == "HEAD" else branch + + +def resolve_path_aliases( + session: Session, path: str, *, current_branch: str | None = None +) -> set[str]: """Every historical name ``path`` has ever gone by, plus ``path`` itself. Walks ``commit_file_change.renamed_from`` edges one BFS layer at a @@ -46,6 +108,11 @@ def resolve_path_aliases(session: Session, path: str) -> set[str]: path : str The path to start from — typically the current HEAD path of the file the caller cares about. Returned in the result set. + current_branch : str or None, optional + Widen the walk to renames first seen on this branch, on top of + the default branch — see :func:`branch_scope`. Defaults to + ``None`` (default branch only), so a caller that does not thread + it gets the conservative behaviour rather than an error. Returns ------- @@ -59,8 +126,10 @@ def resolve_path_aliases(session: Session, path: str) -> set[str]: while frontier: rows = session.exec( select(CommitFileChange.renamed_from) + .join(Commit, col(Commit.sha) == col(CommitFileChange.commit_sha)) .where(col(CommitFileChange.path).in_(frontier)) .where(col(CommitFileChange.renamed_from).is_not(None)) + .where(branch_scope(current_branch)) ).all() next_layer = {row for row in rows if row and row not in aliases} if not next_layer: @@ -164,7 +233,9 @@ def area_history_commits( with get_session() as session: if include_renames: - aliases = resolve_path_aliases(session, path) + aliases = resolve_path_aliases( + session, path, current_branch=current_branch_scope() + ) else: aliases = {path} if not aliases: @@ -176,10 +247,12 @@ def area_history_commits( col(CommitFileChange.commit_sha) == col(Commit.sha), ) .where(col(CommitFileChange.path).in_(aliases)) - # Area-history is a main-walk-only view. Recovered PR-origin - # commits (on_default_branch=0) carry no commit_file_change - # rows so the join already excludes them; this makes the - # invariant explicit for a future broad consumer. + # Area-history is a default-branch-only view, and this filter + # is what enforces it. Flag-0 rows are no longer only PR-origin + # recoveries (which carry no commit_file_change rows): they now + # also include unmerged local work scanned off a feature + # branch, which *does* carry them. The alias set above may + # widen to the current branch; the commit set never does. .where(col(Commit.on_default_branch) == 1) ) if exclude_shas: diff --git a/src/whygraph/mcp/resources.py b/src/whygraph/mcp/resources.py index d059750..c9993cc 100644 --- a/src/whygraph/mcp/resources.py +++ b/src/whygraph/mcp/resources.py @@ -58,7 +58,7 @@ from .errors import WhyGraphError from .evidence import _json_list, _linked_prs -from .path_history import resolve_path_aliases +from .path_history import current_branch_scope, resolve_path_aliases _log = logging.getLogger(__name__) @@ -484,7 +484,13 @@ def _find_changes_resource( try: with get_session() as session: - aliases = resolve_path_aliases(session, path) if path else set() + aliases = ( + resolve_path_aliases( + session, path, current_branch=current_branch_scope() + ) + if path + else set() + ) shas = _find_changes_shas(session, terms, path, aliases) if not shas: return {"query": query, "path": path or None, "count": 0, "commits": []} diff --git a/src/whygraph/scan/git_crawler.py b/src/whygraph/scan/git_crawler.py index 8d23cbd..46a3cd1 100644 --- a/src/whygraph/scan/git_crawler.py +++ b/src/whygraph/scan/git_crawler.py @@ -14,14 +14,22 @@ checked independently of the commit row, so upgrading from a pre-Phase-2 WhyGraph DB and re-running ``whygraph scan`` backfills the index without needing a separate command. + +Branch membership is computed, not assumed. Every row records whether it +is reachable from the default branch (``on_default_branch``) and, when it +is not, the ref it was first seen on (``first_seen_ref``). A reconcile +pass at the end of each crawl recomputes the flag for *existing* rows too, +so the database self-heals as branches merge or get rewritten — see the +plan's §4.2 / §4.3. """ from __future__ import annotations +import logging from datetime import datetime, timezone from rich.progress import Progress -from sqlmodel import select +from sqlmodel import col, select, update from whygraph.db import get_session from whygraph.db.models.commit import Commit as CommitRow @@ -32,13 +40,21 @@ from .crawler import Crawler from .refactor_score import compute_refactor_score +_log = logging.getLogger(__name__) + +# SHAs per reconcile UPDATE. Well under SQLite's SQLITE_MAX_VARIABLE_NUMBER +# on every build (999 on older ones), so a bulk reflag never trips it. +_UPDATE_CHUNK = 500 + class GitCrawler(Crawler): """Crawl every commit on the repository's current branch. Sizes the progress bar from ``len(repository.commits)`` and inserts one row per new commit. SHAs already present in the ``commit`` table - are skipped without modification, so re-scans are idempotent. + are skipped without modification, so re-scans are idempotent — except + for the branch-membership reconcile pass, which is the one place a + re-scan deliberately rewrites existing rows. Parameters ---------- @@ -46,15 +62,26 @@ class GitCrawler(Crawler): Shared Progress instance owned by the orchestrator. repository : Repository The git repository to scan. Walks :attr:`Repository.current_branch`. + + Attributes + ---------- + warning : str or None + Message describing a bulk demotion, for the orchestrator to + surface after the crawl. ``None`` when nothing was demoted. + Mirrors :attr:`CodeGraphCrawler.warning`'s "surface after the + crawl, don't fail it" contract. """ def __init__(self, progress: Progress, *, repository: Repository) -> None: super().__init__("git", progress, total=None) self._repository = repository + self.warning: str | None = None def work(self) -> None: commits = self._repository.commits self.set_total(len(commits)) + default_shas = self._repository.default_branch_shas + branch = self._repository.current_branch with get_session() as session: existing_commits: set[str] = set(session.exec(select(CommitRow.sha)).all()) @@ -76,8 +103,20 @@ def work(self) -> None: ) if dc.sha not in existing_commits: + # An unresolvable default branch degrades to 1 for + # everything, which is exactly today's behaviour — no + # new failure mode for local-only or unborn repos. + on_default = ( + 1 if (not default_shas or dc.sha in default_shas) else 0 + ) session.add( - _to_row(dc, scanned_at=scanned_at, refactor_score=score) + _to_row( + dc, + scanned_at=scanned_at, + refactor_score=score, + on_default_branch=on_default, + first_seen_ref=None if on_default else branch, + ) ) inserted += 1 elif file_changes: @@ -91,10 +130,111 @@ def work(self) -> None: session.add(existing) self.advance(1) - self.summary = f"{len(commits)} commits ({inserted} new)" + promoted, demoted = _reconcile_branch_membership( + session, default_shas, skip=self._repository.is_shallow + ) + + parts = [f"{inserted} new"] + if promoted: + parts.append(f"{promoted} promoted") + if demoted: + parts.append(f"{demoted} demoted") + refs = ", ".join(self._repository.default_branch_refs) + self.warning = ( + f"{demoted} commits are no longer reachable from {refs} — " + "demoted to off-default-branch" + ) + self.summary = f"{len(commits)} commits ({', '.join(parts)})" + + +def _reconcile_branch_membership( + session, default_shas: frozenset[str], *, skip: bool +) -> tuple[int, int]: + """Recompute ``on_default_branch`` for every existing ``commit`` row. + + This is what makes the database self-heal: a feature commit whose + branch has since been merged is promoted, and a commit that was + force-pushed away is demoted. Rows are never deleted — an unreachable + commit is still valid evidence for why the code looks the way it does. + + ``first_seen_ref`` is deliberately **not** rewritten. On a demotion + its ``NULL`` correctly reads as "was on the default branch, no longer + reachable"; on a promotion the original ref stays as provenance. + + Parameters + ---------- + session : Session + The crawler's open session. Changes are staged, not committed — + the caller's context manager owns the transaction. + default_shas : frozenset[str] + Every SHA reachable from the default branch. An **empty** set + means the default branch could not be resolved, which is + "cannot judge", not "nothing is on it". + skip : bool + Skip the pass entirely (shallow clone). A truncated view of the + default branch would demote nearly every row. + + Returns + ------- + tuple[int, int] + ``(promoted, demoted)`` — the ``0 -> 1`` and ``1 -> 0`` counts. + Both guards return ``(0, 0)`` without touching a single row. + + Notes + ----- + The demoted SHAs are logged at ``INFO``, which + :func:`whygraph.core.logger.scan_log_redirect` lands in + ``.whygraph/scan.log``. The console warning stays a count — an + unbounded SHA list has no place in a one-line-per-phase panel. + """ + if not default_shas or skip: + return (0, 0) + + promote: list[str] = [] + demoted_shas: list[str] = [] + rows = session.exec(select(CommitRow.sha, CommitRow.on_default_branch)).all() + for sha, flag in rows: + want = 1 if sha in default_shas else 0 + if want == flag: + continue + (promote if want == 1 else demoted_shas).append(sha) + + _set_flag(session, promote, 1) + _set_flag(session, demoted_shas, 0) + + if demoted_shas: + _log.info( + "demoted %d commits off the default branch: %s", + len(demoted_shas), + " ".join(sorted(demoted_shas)), + ) + return (len(promote), len(demoted_shas)) + + +def _set_flag(session, shas: list[str], value: int) -> None: + """Stage ``on_default_branch = value`` for ``shas``, chunked. + + Chunked because the SHA list is unbounded — a first scan against a + pre-existing DB can move tens of thousands of rows — and SQLite caps + the number of bound parameters per statement. + """ + for start in range(0, len(shas), _UPDATE_CHUNK): + chunk = shas[start : start + _UPDATE_CHUNK] + session.exec( + update(CommitRow) + .where(col(CommitRow.sha).in_(chunk)) + .values(on_default_branch=value) + ) -def _to_row(dc: CommitDC, *, scanned_at: str, refactor_score: int = 0) -> CommitRow: +def _to_row( + dc: CommitDC, + *, + scanned_at: str, + refactor_score: int = 0, + on_default_branch: int = 1, + first_seen_ref: str | None = None, +) -> CommitRow: return CommitRow( sha=dc.sha, parent_shas=" ".join(dc.parent_shas), @@ -109,6 +249,8 @@ def _to_row(dc: CommitDC, *, scanned_at: str, refactor_score: int = 0) -> Commit deletions=dc.stats.deletions, scanned_at=scanned_at, refactor_score=refactor_score, + on_default_branch=on_default_branch, + first_seen_ref=first_seen_ref, ) diff --git a/src/whygraph/scan/pr_origin_enricher.py b/src/whygraph/scan/pr_origin_enricher.py index fa245c1..d7349f9 100644 --- a/src/whygraph/scan/pr_origin_enricher.py +++ b/src/whygraph/scan/pr_origin_enricher.py @@ -140,13 +140,25 @@ def _select_candidates( return candidates -def _to_origin_row(dc: CommitDC, *, scanned_at: str) -> CommitRow: +def _to_origin_row(dc: CommitDC, *, scanned_at: str, number: int) -> CommitRow: """Build an ``on_default_branch=0`` commit row from a git value object. Mirrors ``git_crawler._to_row`` but flags the row as a recovered PR-origin commit and leaves ``refactor_score`` at its default — origin commits carry no ``commit_file_change`` rows, so the refactor-walk never reaches them regardless. + + Parameters + ---------- + dc : Commit + The git value object read back from the pinned PR ref. + scanned_at : str + ISO timestamp stamped on every row this run inserts. + number : int + The PR this commit was recovered from. Recorded in + ``first_seen_ref`` as ``refs/pull//head``, which is what + distinguishes a recovery from unmerged local work — both of which + are ``on_default_branch=0``. """ return CommitRow( sha=dc.sha, @@ -162,6 +174,7 @@ def _to_origin_row(dc: CommitDC, *, scanned_at: str) -> CommitRow: deletions=dc.stats.deletions, scanned_at=scanned_at, on_default_branch=0, + first_seen_ref=f"refs/pull/{number}/head", ) @@ -245,7 +258,9 @@ def work(self) -> None: except GitError as exc: _log.warning("skipping origin commit %s: %s", oid[:9], exc) continue - session.add(_to_origin_row(dc, scanned_at=scanned_at)) + session.add( + _to_origin_row(dc, scanned_at=scanned_at, number=cand.number) + ) inserted.add(oid) self.advance(1) diff --git a/src/whygraph/services/git/commands.py b/src/whygraph/services/git/commands.py index 3a65331..1efb05f 100644 --- a/src/whygraph/services/git/commands.py +++ b/src/whygraph/services/git/commands.py @@ -8,7 +8,7 @@ from __future__ import annotations -from collections.abc import Iterator +from collections.abc import Iterator, Sequence from pathlib import Path from subprocess import CompletedProcess @@ -32,6 +32,19 @@ """``git rev-parse --abbrev-ref HEAD`` — the current branch name, or ``"HEAD"`` if detached.""" +GitIsShallowCmd = ShellCommand( + argv=["git", "rev-parse", "--is-shallow-repository"], + parse=lambda r: r.stdout.strip() == "true", +) +"""``git rev-parse --is-shallow-repository`` — ``True`` for a shallow clone. + +A shallow clone's reachability is truncated, so ``git rev-list`` over the +default branch returns only the grafted tip. Callers that judge branch +membership must skip that judgement entirely rather than act on a +partial answer. +""" + + def _parse_remote_url(result: CompletedProcess[str]) -> str | None: """Parse ``git remote get-url `` into a URL or ``None``. @@ -87,6 +100,85 @@ def parse(self, result: CompletedProcess[str]) -> int: return int(result.stdout.strip() or "0") +class GitSymbolicRefCmd(ShellCommand[str | None]): + """``git symbolic-ref --quiet `` — the ref it points at, or ``None``. + + Must be run with ``check=False``: an unset + ``refs/remotes//HEAD`` is a normal state — a plain + ``git fetch`` does not create it — not an error. Follows the same + "non-zero exit collapses to ``None``" idiom as + :class:`GitRemoteUrlCmd`. + + Parameters + ---------- + ref : str + The symbolic ref to resolve, e.g. + ``"refs/remotes/origin/HEAD"``. + """ + + def __init__(self, ref: str) -> None: + self.ref = ref + + def argv(self) -> list[str]: + return ["git", "symbolic-ref", "--quiet", self.ref] + + def parse(self, result: CompletedProcess[str]) -> str | None: + if result.returncode != 0: + return None + return result.stdout.strip() or None + + +class GitRefExistsCmd(ShellCommand[bool]): + """``git rev-parse --verify --quiet ^{commit}`` — does ``ref`` resolve? + + Must be run with ``check=False``: a ref that does not exist is the + question being asked, so its non-zero exit is a value (``False``), + not a failure. The ``^{commit}`` peel keeps a tag or a tree from + answering ``True`` for a query that means "is there a commit here?". + + Parameters + ---------- + ref : str + The ref to probe, e.g. ``"origin/main"``. + """ + + def __init__(self, ref: str) -> None: + self.ref = ref + + def argv(self) -> list[str]: + return ["git", "rev-parse", "--verify", "--quiet", f"{self.ref}^{{commit}}"] + + def parse(self, result: CompletedProcess[str]) -> bool: + return result.returncode == 0 + + +class GitRevListShasCmd(ShellCommand[frozenset[str]]): + """``git rev-list ...`` — every SHA reachable from the given refs. + + Multiple refs are **unioned**, which is exactly the semantics branch + membership needs: a commit on either the local or the + remote-tracking default branch counts as on the default branch. + Deliberately not ``--first-parent`` — a commit merged in via a merge + commit *is* reachable and must be included. + + Parameters + ---------- + refs : Sequence[str] + One or more commit-ishes. An empty sequence must not be passed: + bare ``git rev-list`` is an error, and callers with no refs to + offer have nothing to ask. + """ + + def __init__(self, refs: Sequence[str]) -> None: + self.refs = tuple(refs) + + def argv(self) -> list[str]: + return ["git", "rev-list", *self.refs] + + def parse(self, result: CompletedProcess[str]) -> frozenset[str]: + return frozenset(result.stdout.split()) + + class GitDiffCmd(ShellCommand[str]): """``git diff --no-color [-- ]`` — raw unified diff text. diff --git a/src/whygraph/services/git/repository.py b/src/whygraph/services/git/repository.py index c16eb78..35355b4 100644 --- a/src/whygraph/services/git/repository.py +++ b/src/whygraph/services/git/repository.py @@ -16,8 +16,12 @@ GitDiffCmd, GitDiffTreeFileChangesCmd, GitFetchRefsCmd, + GitIsShallowCmd, GitLogCommitCmd, + GitRefExistsCmd, GitRemoteUrlCmd, + GitRevListShasCmd, + GitSymbolicRefCmd, ) from .commit import Commit from .commits import Commits @@ -40,6 +44,11 @@ # distinct identities; the results are concatenated in input order. _MAILMAP_CHUNK = 500 +# Short branch names probed, in order, when ``refs/remotes//HEAD`` +# is unset — which is the common case, since a plain ``git clone`` sets it +# but a plain ``git fetch`` into an existing repo does not. +_DEFAULT_BRANCH_CANDIDATES = ("main", "master") + class Repository: """A git repository rooted at a specific working tree on disk. @@ -67,6 +76,12 @@ class Repository: Name of the git remote :attr:`origin_url` reads. Default ``"origin"``; override to inspect a differently-named remote (e.g. ``"upstream"``). + default_branch : str or None, optional + Short name of the branch to treat as the default (e.g. + ``"develop"``), overriding :attr:`default_branch_refs`' + auto-resolution outright. ``None`` (default) auto-resolves, which + is right for the overwhelming majority of repositories — see + that property for the chain. Attributes ---------- @@ -74,9 +89,16 @@ class Repository: The repository working tree (as supplied at construction). """ - def __init__(self, root: Path, *, origin_remote: str = "origin") -> None: + def __init__( + self, + root: Path, + *, + origin_remote: str = "origin", + default_branch: str | None = None, + ) -> None: self.root = root self._origin_remote = origin_remote + self._default_branch = default_branch self._shell = Shell() def __repr__(self) -> str: @@ -152,6 +174,135 @@ def origin_url(self) -> str | None: except ShellError as exc: raise GitError(f"failed to resolve origin URL at {self.root}") from exc + @cached_property + def default_branch_refs(self) -> tuple[str, ...]: + """Refs whose union defines "on the default branch". + + Resolution order — the first step that yields a short branch + name wins: + + 1. ``default_branch`` from construction, when supplied. It + **replaces** the probing below outright, for repos on + ``develop`` / ``trunk``. + 2. ``git symbolic-ref refs/remotes//HEAD``, which a + plain ``git clone`` sets to the forge's real default branch. + 3. ``/main``, then ``/master``. + + The resulting short name is then expanded into **both** the + remote-tracking ref and the same-named *local* branch, when each + exists — so the answer is typically ``("origin/main", "main")``. + The union matters in both directions: unpushed commits on local + ``main`` are absent from ``origin/main``, and a colleague's + fetched-but-unmerged work is absent from local ``main``. Judging + against only one of the two would misclassify one of those + populations. + + Returns + ------- + tuple[str, ...] + Refs to pass to ``git rev-list``, or an empty tuple when + nothing resolves — an unborn HEAD, no remote, or exotic + branch naming with no configured override. Callers must read + the empty tuple as "cannot judge" and leave flags alone + rather than treating every commit as off-branch. + + Notes + ----- + Never raises: any ``git`` failure degrades to the empty tuple, + because a wrong answer here would mass-reflag the database while + no answer merely preserves the status quo. + """ + short = self._resolve_default_branch_name() + if short is None: + return () + refs = [] + remote_ref = f"{self._origin_remote}/{short}" + if self._ref_exists(remote_ref): + refs.append(remote_ref) + if self._ref_exists(short): + refs.append(short) + return tuple(refs) + + @cached_property + def default_branch_shas(self) -> frozenset[str]: + """Every SHA reachable from :attr:`default_branch_refs`. + + One ``git rev-list`` over the whole ref union, so membership + testing is O(1) per commit afterwards. Full reachability, **not** + ``--first-parent``: a commit merged into the default branch via a + merge commit is on that branch and must be counted. + + Returns + ------- + frozenset[str] + The reachable SHAs, or an empty set when + :attr:`default_branch_refs` is empty or ``git`` fails. + """ + refs = self.default_branch_refs + if not refs: + return frozenset() + try: + return self._shell.run(GitRevListShasCmd(refs), cwd=self.root) + except ShellError: + return frozenset() + + @cached_property + def is_shallow(self) -> bool: + """``True`` for a shallow clone, whose reachability is truncated. + + In a ``--depth=1`` clone (the GitHub Actions default) + ``git rev-list origin/main`` returns a single SHA, so any caller + that judges branch membership from it would conclude that the + entire history is off-branch. Such callers must skip the + judgement when this is ``True``. + + Returns + ------- + bool + Whether the repository is shallow. A ``git`` failure returns + ``True`` — an unreadable answer is treated as truncated, + since skipping a reconcile is recoverable and a mass-reflag + is not. + """ + try: + return self._shell.run(GitIsShallowCmd, cwd=self.root) + except ShellError: + return True + + def _resolve_default_branch_name(self) -> str | None: + """Short name of the default branch, or ``None`` if unresolvable. + + Implements steps 1-3 of :attr:`default_branch_refs`' chain. The + configured override is returned verbatim without probing — an + unresolvable value then falls out as an empty ref tuple, which + the scan panel reports. + """ + if self._default_branch: + return self._default_branch + pointee = self._symbolic_ref(f"refs/remotes/{self._origin_remote}/HEAD") + if pointee: + prefix = f"refs/remotes/{self._origin_remote}/" + if pointee.startswith(prefix): + return pointee[len(prefix) :] + for candidate in _DEFAULT_BRANCH_CANDIDATES: + if self._ref_exists(f"{self._origin_remote}/{candidate}"): + return candidate + return None + + def _symbolic_ref(self, ref: str) -> str | None: + """Resolve a symbolic ref, or ``None`` if unset or unreadable.""" + try: + return self._shell.run(GitSymbolicRefCmd(ref), cwd=self.root, check=False) + except ShellError: + return None + + def _ref_exists(self, ref: str) -> bool: + """Whether ``ref`` resolves to a commit; ``False`` if git fails.""" + try: + return self._shell.run(GitRefExistsCmd(ref), cwd=self.root, check=False) + except ShellError: + return False + def diff(self, commit: Commit, *, pathspec: str | None = None) -> str: """Raw unified-diff text for ``commit`` against its first parent. diff --git a/tests/fixtures/default_config_golden.toml b/tests/fixtures/default_config_golden.toml index 604c9be..27ec06d 100644 --- a/tests/fixtures/default_config_golden.toml +++ b/tests/fixtures/default_config_golden.toml @@ -19,6 +19,17 @@ remote = "origin" # git remote whose URL is inspected for provider/a # token — handy when one shared container scans repos # across different orgs. whygraph.toml is gitignored, so a # token here is never committed. +# Auto-rescan git hooks, installed by `whygraph init`: +# true — all four: post-commit, post-merge, post-rewrite, post-checkout +# false — none (init removes any already installed) +# [list] — only these, e.g. ["post-commit", "post-merge"] +# They run a fast offline scan in the background so WhyGraph and CodeGraph +# track your commits. `whygraph init` makes .git/hooks match this value +# exactly — edit and re-run it to add or drop hooks. +hooks = true +# default_branch = "main" # override the branch WhyGraph treats as "shipped history". + # Default: resolved from origin/HEAD, else origin/main, + # else origin/master. Set for repos on develop / trunk. [analyze] # LLM that writes a per-commit "git diff" description during `whygraph scan`. diff --git a/tests/test_chat_stats_sql.py b/tests/test_chat_stats_sql.py index 7a02a45..85f751c 100644 --- a/tests/test_chat_stats_sql.py +++ b/tests/test_chat_stats_sql.py @@ -405,7 +405,7 @@ def test_schema_doc_carries_all_five_silent_corruption_rules() -> None: test can catch because the tool did exactly what it was asked. """ doc = stats_sql._SCHEMA_DOC - # Rule 1 — double-counted squash-recovered commits. + # Rule 1 — commits that are not on the default branch. assert "on_default_branch = 1" in doc # Rule 2 — mixed ISO-8601 forms; substr() does not normalise, strftime does. assert "strftime" in doc @@ -470,3 +470,27 @@ def test_schema_doc_routes_developer_grouping_through_the_author_table() -> None assert "do NOT also apply rule 1" in doc # Rule 1 is not a caveat on this path, and the count is not a scoreboard. assert "Never present a commit count as a measure of productivity" in doc + + +def test_schema_doc_describes_flag_zero_as_both_populations() -> None: + """Case 47e (audit A5) — rule 1's *reason* must match reality. + + Flag-0 used to mean only "PR-origin recovery, already on the main walk". + It now also means unmerged local work, which is emphatically *not* on the + main walk — a model reasoning from the old premise could decide to union + flag-0 rows back in when asked for "all work including squashed PRs". + + Asserts on the rule's substance, not its wording, so a future rewording + does not break the test. + """ + doc = stats_sql._SCHEMA_DOC + rule_one = doc.split("2. For dates")[0].split("1. ALWAYS")[1] + + # The instruction is unchanged and must stay. + assert "on_default_branch = 1" in rule_one + # Both populations named; the false "already on the main walk" claim gone. + assert "unmerged" in rule_one.lower() + assert "squash" in rule_one.lower() + assert "already on the main walk" not in rule_one.lower() + # The discriminator is documented in the commit schema block. + assert "first_seen_ref" in doc diff --git a/tests/test_cli_hooks.py b/tests/test_cli_hooks.py deleted file mode 100644 index cba1a17..0000000 --- a/tests/test_cli_hooks.py +++ /dev/null @@ -1,130 +0,0 @@ -"""Tests for the ``whygraph hooks`` command group. - -Exercise install / uninstall / status against a real (throwaway) git repo -via Click's ``CliRunner.isolated_filesystem``: the managed dispatcher is -sentinel-guarded, idempotent, and never clobbers a foreign hook, and the -generated shell is syntactically valid. -""" - -from __future__ import annotations - -import os -import subprocess -from pathlib import Path - -from click.testing import CliRunner - -from whygraph.cli.commands.hooks import ( - HELPER_RELPATH, - HOOK_NAMES, - SENTINEL, - hooks_cmd, -) - - -def _git_init() -> None: - subprocess.run(["git", "init", "-q"], check=True) - - -def _install(runner: CliRunner): - return runner.invoke(hooks_cmd, ["install"]) - - -def test_install_creates_helper_and_hooks() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): - _git_init() - result = _install(runner) - assert result.exit_code == 0, result.output - - helper = Path(HELPER_RELPATH) - assert helper.exists() - assert os.access(helper, os.X_OK) - - for name in HOOK_NAMES: - hook = Path(".git/hooks") / name - assert hook.exists(), name - assert SENTINEL in hook.read_text() - assert os.access(hook, os.X_OK) - - -def test_install_is_idempotent() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): - _git_init() - _install(runner) - _install(runner) # second run must not stack blocks - for name in HOOK_NAMES: - text = (Path(".git/hooks") / name).read_text() - assert text.count(SENTINEL) == 1, name - - -def test_install_appends_to_foreign_hook() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): - _git_init() - foreign = Path(".git/hooks/post-commit") - foreign.write_text("#!/bin/sh\necho custom-hook\n") - - _install(runner) - - text = foreign.read_text() - assert "echo custom-hook" in text # foreign content preserved - assert SENTINEL in text # ours appended - - -def test_uninstall_removes_ours_keeps_foreign() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): - _git_init() - foreign = Path(".git/hooks/post-commit") - foreign.write_text("#!/bin/sh\necho custom-hook\n") - - _install(runner) - result = runner.invoke(hooks_cmd, ["uninstall"]) - assert result.exit_code == 0, result.output - - # Foreign hook kept, our block stripped. - text = foreign.read_text() - assert "echo custom-hook" in text - assert SENTINEL not in text - # Hooks WhyGraph created outright are removed, as is the helper. - assert not (Path(".git/hooks") / "post-merge").exists() - assert not Path(HELPER_RELPATH).exists() - - -def test_status_reports_states() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): - _git_init() - before = runner.invoke(hooks_cmd, ["status"]) - assert before.exit_code == 0 - assert "missing" in before.output - - _install(runner) - after = runner.invoke(hooks_cmd, ["status"]) - assert "managed" in after.output - - -def test_not_a_git_repo_errors() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): # no git init - result = _install(runner) - assert result.exit_code != 0 - assert "not a git repository" in result.output - - -def test_generated_shell_is_valid() -> None: - runner = CliRunner() - with runner.isolated_filesystem(): - _git_init() - _install(runner) - # `sh -n` parses without executing — catches quoting/syntax errors. - for path in [ - Path(HELPER_RELPATH), - *(Path(".git/hooks") / n for n in HOOK_NAMES), - ]: - check = subprocess.run( - ["sh", "-n", str(path)], capture_output=True, text=True - ) - assert check.returncode == 0, f"{path}: {check.stderr}" diff --git a/tests/test_cli_init_hooks.py b/tests/test_cli_init_hooks.py new file mode 100644 index 0000000..d408b99 --- /dev/null +++ b/tests/test_cli_init_hooks.py @@ -0,0 +1,167 @@ +"""End-to-end tests for ``whygraph init``'s git-hook reconcile (plan §4.6). + +Drives the real command through :class:`click.testing.CliRunner` against a +throwaway git repo, with the DB and preflight stubbed out (same approach as +``tests/test_init_agents.py``) so these tests are about hook reconciliation +and nothing else. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from whygraph.cli.commands.init import init_cmd +from whygraph.hooks import HELPER_RELPATH, HOOK_NAMES, SENTINEL + + +@pytest.fixture +def stub_init(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): + """Neutralise the heavy init steps — DB bootstrap and host preflight.""" + + def _fake_db() -> Path: + db = tmp_path / ".whygraph" / "whygraph.db" + db.parent.mkdir(parents=True, exist_ok=True) + db.touch() + return db + + monkeypatch.setattr("whygraph.cli.commands.init._ensure_db_initialized", _fake_db) + monkeypatch.setattr("whygraph.cli.commands.init._run_preflight", lambda: None) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + root = tmp_path / "repo" + root.mkdir() + subprocess.run(["git", "init", "-q", str(root)], check=True) + return root + + +def _init(repo: Path, *args: str): + # `init_cmd` directly, not the `whygraph` group: the group callback runs + # `configure_logging`, which replaces the root logger's handlers and would + # silently disable `caplog` for every test that runs after this file. + runner = CliRunner() + return runner.invoke(init_cmd, list(args), catch_exceptions=False) + + +def _run_in(repo: Path, monkeypatch: pytest.MonkeyPatch, *args: str): + monkeypatch.chdir(repo) + return _init(repo, *args) + + +def _managed(repo: Path) -> set[str]: + """Hook names currently carrying the managed block.""" + hooks_dir = repo / ".git" / "hooks" + return { + name + for name in HOOK_NAMES + if (hooks_dir / name).exists() and SENTINEL in (hooks_dir / name).read_text() + } + + +def test_init_yes_installs_all_four_hooks( + stub_init, repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 35.""" + result = _run_in(repo, monkeypatch, "--yes") + + assert result.exit_code == 0, result.output + assert _managed(repo) == set(HOOK_NAMES) + assert (repo / HELPER_RELPATH).exists() + assert "Installed auto-rescan git hooks" in result.output + + +def test_existing_opt_out_is_not_resurrected( + stub_init, repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 36 / property 2 — a prior `hooks = false` survives a re-run.""" + (repo / "whygraph.toml").write_text("[scan]\nhooks = false\n") + + result = _run_in(repo, monkeypatch, "--yes") + + assert result.exit_code == 0, result.output + assert _managed(repo) == set() + assert not (repo / HELPER_RELPATH).exists() + + +def test_flipping_to_false_removes_hooks_and_helper( + stub_init, repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 37 — `init` is the reconciler, so it uninstalls too.""" + _run_in(repo, monkeypatch, "--yes") + assert _managed(repo) == set(HOOK_NAMES) + + (repo / "whygraph.toml").write_text("[scan]\nhooks = false\n") + result = _run_in(repo, monkeypatch, "--yes") + + assert result.exit_code == 0, result.output + assert _managed(repo) == set() + assert not (repo / HELPER_RELPATH).exists() + assert "Removed git hooks" in result.output + + +def test_shrinking_the_list_drops_the_others( + stub_init, repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 38 (D7 end-to-end) — the removal half, through the real command.""" + _run_in(repo, monkeypatch, "--yes") + + (repo / "whygraph.toml").write_text('[scan]\nhooks = ["post-commit"]\n') + result = _run_in(repo, monkeypatch, "--yes") + + assert result.exit_code == 0, result.output + assert _managed(repo) == {"post-commit"} + # The helper stays — post-commit still dispatches to it. + assert (repo / HELPER_RELPATH).exists() + + +def test_typo_in_hook_name_warns_and_installs_nothing( + stub_init, repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 14 / 41 — a bad name is a warning, not a failed init.""" + (repo / "whygraph.toml").write_text('[scan]\nhooks = ["post-comit"]\n') + + result = _run_in(repo, monkeypatch, "--yes") + + assert result.exit_code == 0, result.output + assert "post-comit" in result.output + assert _managed(repo) == set() + # The rest of init still completed. + assert "Initialized WhyGraph database" in result.output + + +def test_unwritable_hooks_dir_warns_but_init_succeeds( + stub_init, repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 16 / 42 — best-effort (§4.6 property 1).""" + hooks_dir = repo / ".git" / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + hooks_dir.chmod(0o500) + try: + result = _run_in(repo, monkeypatch, "--yes") + finally: + hooks_dir.chmod(0o700) + + assert result.exit_code == 0, result.output + assert "Skipped git hooks" in result.output + # The DB, config and gitignore work all completed regardless. + assert (repo / "whygraph.toml").exists() + assert (repo / "whygraph.example.toml").exists() + + +def test_not_a_git_repo_warns_but_init_succeeds( + stub_init, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`whygraph init` outside a repo still bootstraps everything else.""" + plain = tmp_path / "plain" + plain.mkdir() + + result = _run_in(plain, monkeypatch, "--yes") + + assert result.exit_code == 0, result.output + assert "Skipped git hooks" in result.output + assert (plain / "whygraph.toml").exists() diff --git a/tests/test_cli_init_interactive.py b/tests/test_cli_init_interactive.py index d1ff600..9307121 100644 --- a/tests/test_cli_init_interactive.py +++ b/tests/test_cli_init_interactive.py @@ -79,7 +79,7 @@ def test_rationale_defaults_to_analyze(tmp_path: Path) -> None: selects=["claude", "openai", DEFAULT, "off"], # analyze_model(default), rationale_model(default) texts=[DEFAULT, DEFAULT], - confirms=[True], # final "Write these files?" + confirms=[True, True], # install hooks? Yes ; Write these files? Yes ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -98,7 +98,7 @@ def test_api_key_prompted_once_when_shared(tmp_path: Path) -> None: selects=["claude", "openai", DEFAULT, "off"], texts=[DEFAULT, DEFAULT], passwords=["sk-shared"], - confirms=[True], + confirms=[True, True], ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -111,7 +111,7 @@ def test_api_key_prompted_twice_when_providers_differ(tmp_path: Path) -> None: selects=["claude", "openai", "deepseek", "off"], texts=[DEFAULT, DEFAULT], passwords=["sk-openai", "sk-deepseek"], - confirms=[True], + confirms=[True, True], ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -124,7 +124,7 @@ def test_no_key_prompt_for_non_key_bearing_providers(tmp_path: Path) -> None: prompter = ScriptedPrompter( selects=["claude", "claude-cli", DEFAULT, "off"], texts=[DEFAULT, DEFAULT], - confirms=[True], + confirms=[True, True], ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -139,7 +139,7 @@ def test_token_not_prompted_when_scan_off(tmp_path: Path) -> None: prompter = ScriptedPrompter( selects=["claude", "claude-cli", DEFAULT, "off"], texts=[DEFAULT, DEFAULT], - confirms=[True], + confirms=[True, True], ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -152,7 +152,7 @@ def test_token_prompted_for_github(tmp_path: Path) -> None: selects=["claude", "claude-cli", DEFAULT, "github"], texts=[DEFAULT, DEFAULT], passwords=["ghp_real"], - confirms=[True], + confirms=[True, True], ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -183,7 +183,7 @@ def test_overwrite_gate_yes_runs_full_flow(tmp_path: Path) -> None: prompter = ScriptedPrompter( selects=["claude", "openai", DEFAULT, "off"], texts=[DEFAULT, DEFAULT], - confirms=[True, True], # overwrite? Yes ; Write? Yes + confirms=[True, True, True], # overwrite? Yes ; hooks? Yes ; Write? Yes ) answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -195,7 +195,7 @@ def test_preset_agent_skips_agent_prompt(tmp_path: Path) -> None: prompter = ScriptedPrompter( selects=["openai", DEFAULT, "off"], # no agent select texts=[DEFAULT, DEFAULT], - confirms=[True], + confirms=[True, True], ) answers = prompt_for_init(tmp_path, preset_agent="cursor", prompter=prompter) @@ -216,7 +216,7 @@ def test_abort_on_declined_final_confirm(tmp_path: Path) -> None: prompter = ScriptedPrompter( selects=["claude", "openai", DEFAULT, "off"], texts=[DEFAULT, DEFAULT], - confirms=[False], # decline "Write these files?" + confirms=[True, False], # hooks? Yes ; decline "Write these files?" ) with pytest.raises(InitAborted): prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) @@ -275,7 +275,7 @@ def test_summary_hook_receives_text(tmp_path: Path) -> None: selects=["claude", "anthropic", DEFAULT, "off"], texts=[DEFAULT, DEFAULT], passwords=[""], # blank anthropic key → env fallback - confirms=[True], + confirms=[True, True], ) prompt_for_init( tmp_path, @@ -286,3 +286,62 @@ def test_summary_hook_receives_text(tmp_path: Path) -> None: assert len(seen) == 1 assert "Review" not in seen[0] # the body only; the panel title is the command's assert "Analyze:" in seen[0] + + +# ---------- 9. git hooks (plan §4.6, properties 2 and 3) --------------------- + + +def test_hooks_prompt_defaults_to_yes(tmp_path: Path) -> None: + prompter = ScriptedPrompter( + selects=["claude", "claude-cli", DEFAULT, "off"], + texts=[DEFAULT, DEFAULT], + confirms=[DEFAULT, True], # accept the hooks default ; Write? Yes + ) + answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) + + assert answers.scan_hooks is True + + +def test_declining_hooks_is_recorded(tmp_path: Path) -> None: + """Case 40 — a No must reach the answers, not just the prompt.""" + prompter = ScriptedPrompter( + selects=["claude", "claude-cli", DEFAULT, "off"], + texts=[DEFAULT, DEFAULT], + confirms=[False, True], # hooks? No ; Write? Yes + ) + answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) + + assert answers.scan_hooks is False + + +def test_hooks_prompt_seeds_from_existing_opt_out(tmp_path: Path) -> None: + """Property 2 — a prior `false` becomes the prompt's default, so a bare + Enter cannot resurrect it.""" + (tmp_path / "whygraph.toml").write_text("[scan]\nhooks = false\n") + prompter = ScriptedPrompter( + selects=["claude", "claude-cli", DEFAULT, "off"], + texts=[DEFAULT, DEFAULT], + # overwrite? Yes ; hooks? (accept default) ; Write? Yes + confirms=[True, DEFAULT, True], + ) + answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) + + assert answers.scan_hooks is False + + +def test_configured_list_skips_the_prompt_and_is_preserved(tmp_path: Path) -> None: + """Case 39 / property 3 — a Yes/No answer must never widen a list. + + The list is an advanced, config-file-only shape, so the prompt is + skipped entirely rather than asked and ignored. + """ + (tmp_path / "whygraph.toml").write_text('[scan]\nhooks = ["post-commit"]\n') + prompter = ScriptedPrompter( + selects=["claude", "claude-cli", DEFAULT, "off"], + texts=[DEFAULT, DEFAULT], + confirms=[True, True], # overwrite? Yes ; Write? Yes — no hooks prompt + ) + answers = prompt_for_init(tmp_path, preset_agent=None, prompter=prompter) + + assert answers.scan_hooks == ("post-commit",) + assert not any("git hooks" in msg for msg in _kinds(prompter, "confirm")) diff --git a/tests/test_cli_scan_phases.py b/tests/test_cli_scan_phases.py index 8a2405c..0954e38 100644 --- a/tests/test_cli_scan_phases.py +++ b/tests/test_cli_scan_phases.py @@ -301,3 +301,57 @@ def test_results_panel_is_defensive_and_total( assert "skipped" in out # absent pr-origins assert "Scan log" in out # R11: path row retained assert "done in" in out # total elapsed in the title + + +def test_results_panel_surfaces_a_git_demotion_warning( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Case 47 — a bulk demotion gets its own ⚠, not just a summary count.""" + buf = io.StringIO() + monkeypatch.setattr(scan_mod, "console", Console(file=buf, width=120)) + + git = _Fake( + "git", + summary="412 commits (3 new, 2 demoted)", + warning="2 commits are no longer reachable from origin/main, main" + " — demoted to off-default-branch", + ) + + scan_mod._render_results_panel( + ran=[git], + codegraph_crawler=None, + db_path=Path("/repo/.whygraph/whygraph.db"), + scan_log_path=Path("/repo/.whygraph/scan.log"), + phase_timings={"Structural crawl": 1.0}, + total_elapsed=1.0, + ) + + out = buf.getvalue() + assert "⚠" in out + assert "no longer reachable" in out + assert "2 demoted" in out + + +def test_default_branch_row_reports_an_unresolved_repo(tmp_path: Path) -> None: + """Case 47 / D6 — the empty case is named, not silently degraded.""" + + class _NoDefaultBranch: + default_branch_refs: tuple[str, ...] = () + + class _Exploding: + @property + def default_branch_refs(self) -> tuple[str, ...]: + raise RuntimeError("git is broken") + + unresolved = scan_mod._default_branch_label(_NoDefaultBranch()) + assert "set [scan].default_branch" in str(unresolved) + + # A git failure degrades this one row rather than aborting the command. + assert "unavailable" in str(scan_mod._default_branch_label(_Exploding())) + + +def test_default_branch_row_lists_the_resolved_refs() -> None: + class _Resolved: + default_branch_refs = ("origin/main", "main") + + assert scan_mod._default_branch_label(_Resolved()) == "origin/main, main" diff --git a/tests/test_core_config_scaffold.py b/tests/test_core_config_scaffold.py index 9b35934..1fe386e 100644 --- a/tests/test_core_config_scaffold.py +++ b/tests/test_core_config_scaffold.py @@ -159,3 +159,28 @@ def test_render_claude_cli_tag_parses(tmp_path: Path) -> None: cfg = Config.from_toml(path) assert cfg.analyze.provider == "claude-cli" assert cfg.rationale.provider == "claude-cli" + + +def test_hooks_choice_round_trips(tmp_path: Path) -> None: + """Case 22a (audit A1) — the written config reproduces the choice. + + Load-bearing, not cosmetic: `whygraph init` writes the file that the + *next* `whygraph init` reads back to decide whether to install. A + hard-coded `hooks = true` in the template would silently resurrect a + rejection the user just made. + """ + for value in (False, True, ("post-commit", "post-merge")): + answers = InitAnswers(scan_hooks=value, reconfigure_toml=True) + rendered = render_config(answers, include_tokens=False) + path = tmp_path / f"{value}.toml" + path.write_text(rendered) + + assert Config.from_toml(path).scan_hooks == value + + # And the literal TOML shapes, so a future renderer change is visible. + assert "hooks = false" in render_config( + InitAnswers(scan_hooks=False), include_tokens=False + ) + assert 'hooks = ["post-commit"]' in render_config( + InitAnswers(scan_hooks=("post-commit",)), include_tokens=False + ) diff --git a/tests/test_core_config_scan.py b/tests/test_core_config_scan.py index 05fa26c..44445fd 100644 --- a/tests/test_core_config_scan.py +++ b/tests/test_core_config_scan.py @@ -110,3 +110,82 @@ def test_empty_token_normalizes_to_none(tmp_path: Path, value: str) -> None: ) assert cfg.scan_token is None + + +# --- [scan].hooks and [scan].default_branch (plan §6) ------------------------ + + +@pytest.mark.parametrize("value, expected", [("true", True), ("false", False)]) +def test_hooks_bool_parses_verbatim(tmp_path: Path, value: str, expected: bool) -> None: + """Case 18.""" + cfg = Config.from_toml( + _write(tmp_path / "whygraph.toml", f"[scan]\nhooks = {value}\n") + ) + + assert cfg.scan_hooks is expected + + +def test_hooks_list_parses_to_tuple(tmp_path: Path) -> None: + """Case 19 — a list becomes a tuple; an empty list collapses to False.""" + cfg = Config.from_toml( + _write(tmp_path / "whygraph.toml", '[scan]\nhooks = ["post-commit"]\n') + ) + assert cfg.scan_hooks == ("post-commit",) + + empty = Config.from_toml(_write(tmp_path / "empty.toml", "[scan]\nhooks = []\n")) + assert empty.scan_hooks is False + + +@pytest.mark.parametrize("body", ["hooks = 5", 'hooks = ["post-commit", 7]']) +def test_hooks_wrong_shape_raises(tmp_path: Path, body: str) -> None: + """Case 20 — a shape error is a hard failure, like an invalid provider.""" + with pytest.raises(ConfigError, match=r"\[scan\].hooks"): + Config.from_toml(_write(tmp_path / "whygraph.toml", f"[scan]\n{body}\n")) + + +def test_hooks_names_are_not_validated_here(tmp_path: Path) -> None: + """A typo'd name parses fine — `whygraph.hooks` owns name validation, so + `core` keeps no dependency on it (§6).""" + cfg = Config.from_toml( + _write(tmp_path / "whygraph.toml", '[scan]\nhooks = ["post-comit"]\n') + ) + + assert cfg.scan_hooks == ("post-comit",) + + +def test_hooks_and_default_branch_defaults(tmp_path: Path) -> None: + """Case 21.""" + cfg = Config.from_toml(_write(tmp_path / "whygraph.toml", "")) + + assert cfg.scan_hooks is True + assert cfg.scan_default_branch is None + + +def test_default_branch_parses(tmp_path: Path) -> None: + cfg = Config.from_toml( + _write(tmp_path / "whygraph.toml", '[scan]\ndefault_branch = "develop"\n') + ) + + assert cfg.scan_default_branch == "develop" + + +@pytest.mark.parametrize("value", ['""', '" "']) +def test_empty_default_branch_normalizes_to_none(tmp_path: Path, value: str) -> None: + cfg = Config.from_toml( + _write(tmp_path / "whygraph.toml", f"[scan]\ndefault_branch = {value}\n") + ) + + assert cfg.scan_default_branch is None + + +def test_unknown_scan_key_still_only_warns(tmp_path: Path) -> None: + """Case 22 — the new `scan.pop("hooks")` must not break the warn loop.""" + cfg = Config.from_toml( + _write( + tmp_path / "whygraph.toml", + '[scan]\nhooks = false\ndefault_branch = "main"\nnonsense = 1\n', + ) + ) + + assert cfg.scan_hooks is False + assert cfg.scan_default_branch == "main" diff --git a/tests/test_db_plumbing.py b/tests/test_db_plumbing.py index 18c38bc..1141ff7 100644 --- a/tests/test_db_plumbing.py +++ b/tests/test_db_plumbing.py @@ -268,7 +268,10 @@ def test_chat_message_error_column_round_trips( session.commit() db_engine._reset_engine() - command.downgrade(alembic_config(), "-1") + # Downgrade to the parent of the revision that adds `error`, not a bare + # "-1" — that would only be the right target for as long as this stays + # the head revision, which it no longer is. + command.downgrade(alembic_config(), "f3582dfcc817") assert "error" not in _column_names(db_path, "chat_message") # The seeded rows survive the drop — only the column goes away. conn = sqlite3.connect(db_path) diff --git a/tests/test_git_crawler.py b/tests/test_git_crawler.py index 24cb602..b569e1c 100644 --- a/tests/test_git_crawler.py +++ b/tests/test_git_crawler.py @@ -2,8 +2,10 @@ from __future__ import annotations +import logging import subprocess import time +from contextlib import contextmanager from pathlib import Path from typing import Iterator @@ -318,3 +320,234 @@ def test_persisted_fields_match_in_memory_commit(repo_root: Path) -> None: assert row["insertions"] == dc.stats.insertions assert row["deletions"] == dc.stats.deletions assert row["scanned_at"] # set to a non-empty ISO string + + +# -------------------------------------------------------------------------- +# Branch membership (plan §4.2) and the reconcile pass (§4.3). +# -------------------------------------------------------------------------- + + +def _git_out(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(cwd), *args], + check=True, + capture_output=True, + text=True, + ).stdout + + +def _commit_file(root: Path, name: str, body: str = "x\n") -> str: + (root / name).write_text(body) + _git(root, "add", name) + _git(root, "commit", "-q", "-m", name) + return _git_out(root, "rev-parse", "HEAD").strip() + + +def _configure(root: Path) -> Path: + _git(root, "config", "user.email", "test@example.com") + _git(root, "config", "user.name", "Test User") + _git(root, "config", "commit.gpgsign", "false") + return root + + +@pytest.fixture +def cloned(tmp_path: Path) -> Path: + """A clone of a two-commit ``main`` upstream — ``origin/main`` resolves.""" + upstream = _make_repo(tmp_path / "upstream") + work = tmp_path / "work" + subprocess.run( + ["git", "clone", "-q", str(upstream), str(work)], + check=True, + capture_output=True, + ) + return _configure(work) + + +def _rows() -> dict[str, tuple[int, str | None]]: + """``{sha: (on_default_branch, first_seen_ref)}`` for every commit row.""" + with get_session() as session: + return { + r.sha: (r.on_default_branch, r.first_seen_ref) + for r in session.exec(select(CommitRow)).all() + } + + +def test_feature_branch_commit_is_off_default_branch(cloned: Path) -> None: + """Case 8 — unmerged work is flagged 0 and records the ref it came from.""" + _git(cloned, "switch", "-q", "-c", "feature/x") + sha = _commit_file(cloned, "feature.txt") + + GitCrawler(Progress(), repository=Repository(cloned)).run() + + assert _rows()[sha] == (0, "feature/x") + + +def test_default_branch_commit_is_flagged_one(cloned: Path) -> None: + """Case 9 — a commit reachable from origin/main is 1 with a NULL ref.""" + head = _git_out(cloned, "rev-parse", "HEAD").strip() + + GitCrawler(Progress(), repository=Repository(cloned)).run() + + assert _rows()[head] == (1, None) + + +def test_merge_promotes_feature_commits(cloned: Path) -> None: + """Case 10 + 16 — a true merge promotes the rows on the next scan.""" + _git(cloned, "switch", "-q", "-c", "feature/x") + sha = _commit_file(cloned, "feature.txt") + GitCrawler(Progress(), repository=Repository(cloned)).run() + assert _rows()[sha][0] == 0 + + _git(cloned, "switch", "-q", "main") + _git(cloned, "merge", "-q", "--no-ff", "-m", "merge feature/x", "feature/x") + + crawler = GitCrawler(Progress(), repository=Repository(cloned)) + crawler.run() + + # Promoted, but first_seen_ref is provenance and is never rewritten. + assert _rows()[sha] == (1, "feature/x") + assert "1 promoted" in crawler.summary + assert "demoted" not in crawler.summary + assert crawler.warning is None + + +def test_squash_merge_leaves_originals_off_branch(cloned: Path) -> None: + """Case 11 — a squash creates a *new* commit; the originals stay 0.""" + _git(cloned, "switch", "-q", "-c", "feature/x") + sha = _commit_file(cloned, "feature.txt") + GitCrawler(Progress(), repository=Repository(cloned)).run() + + _git(cloned, "switch", "-q", "main") + _git(cloned, "merge", "-q", "--squash", "feature/x") + _git(cloned, "commit", "-q", "-m", "squashed feature/x") + + GitCrawler(Progress(), repository=Repository(cloned)).run() + + # Exactly what PROriginEnricher would have produced — offline and free. + assert _rows()[sha] == (0, "feature/x") + + +def test_rewritten_history_demotes_and_warns(cloned: Path) -> None: + """Cases 12, 16, 17 — a demotion is counted, warned about, and applied.""" + sha = _commit_file(cloned, "local.txt") + GitCrawler(Progress(), repository=Repository(cloned)).run() + assert _rows()[sha][0] == 1 + + _git(cloned, "reset", "-q", "--hard", "HEAD~1") + + crawler = GitCrawler(Progress(), repository=Repository(cloned)) + crawler.run() + + # The row is retained as evidence, just excluded from default-branch queries. + assert _rows()[sha] == (0, None) + assert "1 demoted" in crawler.summary + assert crawler.warning is not None + assert "1 commits are no longer reachable from origin/main, main" in crawler.warning + + +@contextmanager +def _capture(logger_name: str) -> Iterator[list[str]]: + """Capture a single logger's INFO records, independently of the root. + + Deliberately not ``caplog``: several CLI tests earlier in the session + invoke the ``whygraph`` group, whose callback runs ``configure_logging`` + and replaces the root logger's handlers — taking pytest's capture handler + with it. Attaching to the module logger asserts exactly what D9 promises + (this logger emits the SHAs at INFO; ``scan_log_redirect`` does the rest) + without depending on global logging state. + """ + messages: list[str] = [] + + class _Collect(logging.Handler): + def emit(self, record: logging.LogRecord) -> None: + messages.append(record.getMessage()) + + logger = logging.getLogger(logger_name) + handler = _Collect(level=logging.INFO) + previous = logger.level + logger.addHandler(handler) + logger.setLevel(logging.INFO) + try: + yield messages + finally: + logger.removeHandler(handler) + logger.setLevel(previous) + + +def test_demoted_shas_are_logged(cloned: Path) -> None: + """Case 47d (D9) — the SHAs land in the scan log, not the panel.""" + sha = _commit_file(cloned, "local.txt") + GitCrawler(Progress(), repository=Repository(cloned)).run() + _git(cloned, "reset", "-q", "--hard", "HEAD~1") + + with _capture("whygraph.scan.git_crawler") as messages: + crawler = GitCrawler(Progress(), repository=Repository(cloned)) + crawler.run() + + assert any(sha in m for m in messages) + # The console line stays a count — the SHA belongs in the log only. + assert crawler.warning is not None + assert sha not in crawler.warning + + +def test_shallow_clone_skips_reconcile(tmp_path: Path) -> None: + """Case 13 — a truncated view must never mass-demote.""" + upstream = _make_repo(tmp_path / "upstream") + shallow = tmp_path / "shallow" + subprocess.run( + ["git", "clone", "-q", "--depth=1", upstream.as_uri(), str(shallow)], + check=True, + capture_output=True, + ) + _configure(shallow) + assert Repository(shallow).is_shallow is True + + # A pre-existing row for a commit the shallow clone cannot see. Without + # the guard the reconcile would demote it. + with get_session() as session: + session.add( + CommitRow( + sha="0" * 40, + parent_shas="", + author_name="A", + author_email="a@example.com", + authored_at="2026-01-01T00:00:00Z", + committed_at="2026-01-01T00:00:00Z", + subject="older than the graft point", + body="", + files_changed=0, + insertions=0, + deletions=0, + scanned_at="2026-01-01T00:00:00Z", + ) + ) + + crawler = GitCrawler(Progress(), repository=Repository(shallow)) + crawler.run() + + assert _rows()["0" * 40] == (1, None) + assert "demoted" not in crawler.summary + assert crawler.warning is None + + +def test_unresolvable_default_branch_flags_everything_one(repo_root: Path) -> None: + """Case 14 — no remote, no main/master override: today's behaviour exactly.""" + repo = Repository(repo_root) + assert repo.default_branch_refs == () + + GitCrawler(Progress(), repository=repo).run() + + assert all(row == (1, None) for row in _rows().values()) + + +def test_detached_head_records_the_literal_head(cloned: Path) -> None: + """Case 15 — a detached HEAD is stored verbatim; no special case.""" + _git(cloned, "switch", "-q", "-c", "feature/x") + sha = _commit_file(cloned, "feature.txt") + _git(cloned, "checkout", "-q", sha) + + repo = Repository(cloned) + assert repo.current_branch == "HEAD" + GitCrawler(Progress(), repository=repo).run() + + assert _rows()[sha] == (0, "HEAD") diff --git a/tests/test_git_default_branch.py b/tests/test_git_default_branch.py new file mode 100644 index 0000000..339f54b --- /dev/null +++ b/tests/test_git_default_branch.py @@ -0,0 +1,166 @@ +"""Tests for :class:`Repository`'s default-branch resolution (plan §4.1). + +Every case builds a real repository (and, where the remote-tracking refs +matter, a real clone) with ``subprocess`` — the resolution chain reads +``refs/remotes/*`` and shallow-clone state that no fake can reproduce +faithfully. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from whygraph.core import Shell +from whygraph.services.git import Repository +from whygraph.services.git.commands import GitRevListShasCmd + + +def _git(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(cwd), *args], + check=True, + capture_output=True, + text=True, + ).stdout + + +def _init(root: Path, branch: str = "main") -> Path: + root.mkdir(parents=True, exist_ok=True) + _git(root, "init", "-q", "-b", branch) + _git(root, "config", "user.email", "test@example.com") + _git(root, "config", "user.name", "Test User") + _git(root, "config", "commit.gpgsign", "false") + return root + + +def _commit(root: Path, name: str) -> str: + (root / name).write_text(f"{name}\n") + _git(root, "add", name) + _git(root, "commit", "-q", "-m", name) + return _git(root, "rev-parse", "HEAD").strip() + + +@pytest.fixture +def origin(tmp_path: Path) -> Path: + """A bare-ish upstream on ``main`` with two commits.""" + root = _init(tmp_path / "origin") + _commit(root, "one.txt") + _commit(root, "two.txt") + return root + + +def _clone(origin_root: Path, dest: Path, *extra: str) -> Path: + subprocess.run( + ["git", "clone", "-q", *extra, str(origin_root), str(dest)], + check=True, + capture_output=True, + ) + _git(dest, "config", "user.email", "test@example.com") + _git(dest, "config", "user.name", "Test User") + _git(dest, "config", "commit.gpgsign", "false") + return dest + + +def test_symbolic_ref_resolves_remote_head(origin: Path, tmp_path: Path) -> None: + """Case 1 — a plain clone sets ``origin/HEAD``; it is authoritative.""" + work = _clone(origin, tmp_path / "work") + + repo = Repository(work) + + assert "origin/main" in repo.default_branch_refs + + +def test_falls_back_to_remote_main_when_head_unset( + origin: Path, tmp_path: Path +) -> None: + """Case 2 — no ``origin/HEAD`` (the plain-fetch state) → ``origin/main``.""" + work = _clone(origin, tmp_path / "work") + # `update-ref -d` would deref the symref and delete origin/main instead. + _git(work, "symbolic-ref", "--delete", "refs/remotes/origin/HEAD") + + repo = Repository(work) + + assert repo.default_branch_refs == ("origin/main", "main") + + +def test_falls_back_to_remote_master(tmp_path: Path) -> None: + """Case 3 — a ``master`` upstream resolves through the second candidate.""" + upstream = _init(tmp_path / "upstream", branch="master") + _commit(upstream, "one.txt") + work = _clone(upstream, tmp_path / "work") + # `update-ref -d` would deref the symref and delete origin/main instead. + _git(work, "symbolic-ref", "--delete", "refs/remotes/origin/HEAD") + + repo = Repository(work) + + assert repo.default_branch_refs == ("origin/master", "master") + + +def test_union_includes_unpushed_local_commits(origin: Path, tmp_path: Path) -> None: + """Case 4 — a local commit not yet pushed is still on the default branch.""" + work = _clone(origin, tmp_path / "work") + local_sha = _commit(work, "three.txt") + + repo = Repository(work) + + assert repo.default_branch_refs == ("origin/main", "main") + assert local_sha in repo.default_branch_shas + # Guard the point of the union: the remote-tracking ref alone would miss it. + remote_only = Shell().run(GitRevListShasCmd(("origin/main",)), cwd=work) + assert local_sha not in remote_only + + +def test_no_remote_and_exotic_branch_resolves_nothing(tmp_path: Path) -> None: + """Case 5 — nothing to judge against degrades to the empty answer.""" + root = _init(tmp_path / "solo", branch="trunk") + _commit(root, "one.txt") + + repo = Repository(root) + + assert repo.default_branch_refs == () + assert repo.default_branch_shas == frozenset() + + +def test_configured_override_replaces_resolution(origin: Path, tmp_path: Path) -> None: + """Case 6 — the override wins outright, ignoring ``origin/HEAD``.""" + work = _clone(origin, tmp_path / "work") + _git(work, "switch", "-q", "-c", "develop") + develop_sha = _commit(work, "dev.txt") + + repo = Repository(work, default_branch="develop") + + assert repo.default_branch_refs == ("develop",) + assert develop_sha in repo.default_branch_shas + # origin/main is deliberately *not* consulted once the override is set. + assert "origin/main" not in repo.default_branch_refs + + +def test_configured_override_that_resolves_to_nothing( + origin: Path, tmp_path: Path +) -> None: + """An unresolvable override degrades to "cannot judge", never raises.""" + work = _clone(origin, tmp_path / "work") + + repo = Repository(work, default_branch="no-such-branch") + + assert repo.default_branch_refs == () + assert repo.default_branch_shas == frozenset() + + +def test_is_shallow(origin: Path, tmp_path: Path) -> None: + """Case 7 — ``--depth=1`` is shallow; a full clone is not.""" + # `--depth` is ignored for a plain local-path clone; file:// forces the + # real transport, which is the only way to get a genuinely shallow repo. + shallow = tmp_path / "shallow" + subprocess.run( + ["git", "clone", "-q", "--depth=1", origin.as_uri(), str(shallow)], + check=True, + capture_output=True, + ) + full = _clone(origin, tmp_path / "full") + + assert Repository(shallow).is_shallow is True + assert Repository(full).is_shallow is False diff --git a/tests/test_hooks.py b/tests/test_hooks.py new file mode 100644 index 0000000..1e825c4 --- /dev/null +++ b/tests/test_hooks.py @@ -0,0 +1,279 @@ +"""Tests for :mod:`whygraph.hooks` — the auto-rescan git hooks. + +Exercise :func:`sync_hooks` against a real (throwaway) git repo: the +managed dispatcher is sentinel-guarded, idempotent, never clobbers a +foreign hook, reconciles in **both** directions, and the generated shell +is syntactically valid. The ``post-checkout`` arg gate is tested by +running the helper under ``sh`` with git's real argument shapes. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from whygraph.hooks import ( + HELPER_RELPATH, + HOOK_NAMES, + SENTINEL, + HooksError, + resolve_hook_names, + sync_hooks, +) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True) + return tmp_path + + +@pytest.fixture(autouse=True) +def _stub_whygraph_on_path( + tmp_path_factory: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch +) -> None: + """Put a no-op ``whygraph`` on PATH for the helper's own guard. + + The helper exits early unless ``command -v whygraph`` succeeds, so the + arg-gate tests need *a* binary — but not the real one, which would + fork a detached scan into pytest's tmp dir and outlive the test. + """ + bin_dir = tmp_path_factory.mktemp("stub-bin") + stub = bin_dir / "whygraph" + stub.write_text("#!/bin/sh\nexit 0\n") + stub.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}{os.pathsep}{os.environ['PATH']}") + + +def _hook(repo: Path, name: str) -> Path: + return repo / ".git" / "hooks" / name + + +def _install_all(repo: Path): + return sync_hooks(repo, HOOK_NAMES) + + +# --- ported from the retired tests/test_cli_hooks.py ------------------------- + + +def test_install_creates_helper_and_hooks(repo: Path) -> None: + result = _install_all(repo) + + helper = repo / HELPER_RELPATH + assert result.helper == helper + assert helper.exists() + assert os.access(helper, os.X_OK) + + for name in HOOK_NAMES: + hook = _hook(repo, name) + assert hook.exists(), name + assert SENTINEL in hook.read_text() + assert os.access(hook, os.X_OK) + + +def test_install_is_idempotent(repo: Path) -> None: + _install_all(repo) + _install_all(repo) # second run must not stack blocks + + for name in HOOK_NAMES: + assert _hook(repo, name).read_text().count(SENTINEL) == 1, name + + +def test_install_appends_to_foreign_hook(repo: Path) -> None: + foreign = _hook(repo, "post-commit") + foreign.write_text("#!/bin/sh\necho custom-hook\n") + + _install_all(repo) + + text = foreign.read_text() + assert "echo custom-hook" in text # foreign content preserved + assert SENTINEL in text # ours appended + + +def test_uninstall_removes_ours_keeps_foreign(repo: Path) -> None: + """Case 26 — ``sync_hooks(root, ())`` *is* the uninstall.""" + foreign = _hook(repo, "post-commit") + foreign.write_text("#!/bin/sh\necho custom-hook\n") + _install_all(repo) + + result = sync_hooks(repo, ()) + + text = foreign.read_text() + assert "echo custom-hook" in text + assert SENTINEL not in text + # Hooks WhyGraph created outright are removed, as is the helper. + assert not _hook(repo, "post-merge").exists() + assert not (repo / HELPER_RELPATH).exists() + assert result.helper is None + assert set(result.removed) == set(HOOK_NAMES) + + +def test_states_are_reported_per_hook(repo: Path) -> None: + """The direct-inspection equivalent of the retired ``status`` command.""" + before = sync_hooks(repo, ()) + assert set(before.actions.values()) == {"absent"} + + after = _install_all(repo) + assert set(after.actions.values()) == {"created"} + assert set(after.installed) == set(HOOK_NAMES) + + +def test_not_a_git_repo_raises_hooks_error(tmp_path: Path) -> None: + """Case 34 — a ``HooksError``, never a ``ClickException``.""" + with pytest.raises(HooksError, match="not a git repository"): + sync_hooks(tmp_path, HOOK_NAMES) + + +def test_generated_shell_is_valid(repo: Path) -> None: + """Case 33 — ``sh -n`` parses without executing.""" + _install_all(repo) + + for path in [ + repo / HELPER_RELPATH, + *(_hook(repo, n) for n in HOOK_NAMES), + ]: + check = subprocess.run(["sh", "-n", str(path)], capture_output=True, text=True) + assert check.returncode == 0, f"{path}: {check.stderr}" + + +# --- new: the four-hook set and D7's two-directional reconcile --------------- + + +def test_all_four_hooks_are_managed(repo: Path) -> None: + """Case 23 — ``post-checkout`` joined the set.""" + _install_all(repo) + + assert "post-checkout" in HOOK_NAMES + assert SENTINEL in _hook(repo, "post-checkout").read_text() + + +def test_shrinking_the_list_removes_dropped_hooks(repo: Path) -> None: + """Case 24 (D7 shrink) — the half that is easy to forget.""" + _install_all(repo) + + result = sync_hooks(repo, ("post-commit", "post-merge")) + + assert SENTINEL in _hook(repo, "post-commit").read_text() + assert SENTINEL in _hook(repo, "post-merge").read_text() + assert not _hook(repo, "post-rewrite").exists() + assert not _hook(repo, "post-checkout").exists() + # The helper stays — two hooks still dispatch to it. + assert (repo / HELPER_RELPATH).exists() + assert set(result.removed) == {"post-rewrite", "post-checkout"} + + +def test_growing_the_list_restores_hooks(repo: Path) -> None: + """Case 25 (D7 grow) — the reverse direction.""" + sync_hooks(repo, ("post-commit",)) + assert not _hook(repo, "post-rewrite").exists() + + sync_hooks(repo, HOOK_NAMES) + + for name in HOOK_NAMES: + assert SENTINEL in _hook(repo, name).read_text(), name + + +def test_shrink_preserves_foreign_content_in_a_dropped_hook(repo: Path) -> None: + """Case 27 — only the managed block goes.""" + foreign = _hook(repo, "post-rewrite") + foreign.write_text("#!/bin/sh\necho mine\n") + _install_all(repo) + + sync_hooks(repo, ("post-commit",)) + + text = foreign.read_text() + assert "echo mine" in text + assert SENTINEL not in text + + +def test_resolve_hook_names(repo: Path) -> None: + """Case 28 — the bool-or-list shape, and a typo'd name.""" + assert resolve_hook_names(True) == HOOK_NAMES + assert resolve_hook_names(False) == () + assert resolve_hook_names(()) == () + assert resolve_hook_names(["post-commit"]) == ("post-commit",) + # Normalized to HOOK_NAMES order regardless of how the config listed them. + assert resolve_hook_names(["post-merge", "post-commit"]) == ( + "post-commit", + "post-merge", + ) + + with pytest.raises(HooksError, match="post-comit"): + resolve_hook_names(["post-comit"]) + + +# --- the post-checkout arg gate ---------------------------------------------- + + +def test_dispatcher_forwards_arguments(repo: Path) -> None: + """Case 29 — without ``"$@"`` the helper could not tell the cases apart.""" + _install_all(repo) + + assert '"$helper" "$@"' in _hook(repo, "post-checkout").read_text() + + +def _run_helper(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + """Run the helper with git's post-checkout argument shape.""" + return subprocess.run( + ["sh", str(repo / HELPER_RELPATH), *args], + cwd=repo, + capture_output=True, + text=True, + ) + + +def _scan_was_armed(repo: Path) -> bool: + """Whether the helper got as far as arming a scan. + + The pending flag is written immediately before the detached subshell, + and the arg gate sits above it — so its existence (or the log the + subshell creates) is the observable signal that the gate let the call + through, without depending on a `whygraph` binary being on PATH. + """ + return (repo / ".whygraph" / "logs").exists() or ( + repo / ".whygraph" / "scan.pending" + ).exists() + + +def test_file_checkout_is_skipped(repo: Path) -> None: + """Case 30 — ``git checkout -- path`` passes ``0`` as the third arg.""" + _install_all(repo) + + result = _run_helper(repo, "a" * 40, "b" * 40, "0") + + assert result.returncode == 0 + assert not _scan_was_armed(repo) + + +def test_same_point_branch_creation_is_skipped(repo: Path) -> None: + """Case 31 — ``git switch -c`` at the same commit: identical tree.""" + _install_all(repo) + sha = "c" * 40 + + result = _run_helper(repo, sha, sha, "1") + + assert result.returncode == 0 + assert not _scan_was_armed(repo) + + +def test_real_branch_switch_proceeds(repo: Path) -> None: + """Case 32 — a genuine branch switch passes the gate.""" + _install_all(repo) + + result = _run_helper(repo, "a" * 40, "b" * 40, "1") + + assert result.returncode == 0 + assert _scan_was_armed(repo) + + +def test_argless_hooks_proceed(repo: Path) -> None: + """post-commit passes no arguments; the gate must ignore it entirely.""" + _install_all(repo) + + result = _run_helper(repo) + + assert result.returncode == 0 + assert _scan_was_armed(repo) diff --git a/tests/test_mcp_area_history.py b/tests/test_mcp_area_history.py index 8596ef8..e0219c0 100644 --- a/tests/test_mcp_area_history.py +++ b/tests/test_mcp_area_history.py @@ -201,3 +201,100 @@ def test_area_history_tool_rejects_invalid_inputs() -> None: whygraph_area_history("") with pytest.raises(WhyGraphError, match="limit must be >= 1"): whygraph_area_history("foo.py", limit=0) + + +# --- D8: alias scope (default branch OR current branch) ---------------------- + + +def _seed_unmerged_rename(session) -> None: + """A rename made on an unmerged feature branch. + + ``old.py`` exists on the default branch; the rename to ``new.py`` was + made on ``feature/x`` and has not landed, so its commit is flag 0. + """ + session.add(_commit("c_main", subject="add old.py", committed_at="2026-06-01")) + session.add(_change(commit_sha="c_main", path="old.py", change_type="A")) + branch_commit = _commit( + "c_branch", subject="rename old.py -> new.py", committed_at="2026-06-02" + ) + branch_commit.on_default_branch = 0 + branch_commit.first_seen_ref = "feature/x" + session.add(branch_commit) + session.add( + _change( + commit_sha="c_branch", + path="new.py", + change_type="R", + renamed_from="old.py", + similarity=100, + ) + ) + session.commit() + + +def test_alias_scope_includes_the_current_branch(whygraph_db_initialized: Path) -> None: + """Case 47a(i) — the in-flight rename is visible while you are on it.""" + with get_session() as session: + _seed_unmerged_rename(session) + aliases = resolve_path_aliases(session, "new.py", current_branch="feature/x") + + assert aliases == {"new.py", "old.py"} + + +def test_alias_scope_excludes_another_branch(whygraph_db_initialized: Path) -> None: + """Case 47a(ii) — switching away drops the old branch's aliases at once.""" + with get_session() as session: + _seed_unmerged_rename(session) + aliases = resolve_path_aliases(session, "new.py", current_branch="feature/y") + + assert aliases == {"new.py"} + + +def test_alias_scope_excludes_on_detached_head(whygraph_db_initialized: Path) -> None: + """Case 47a(iii) — ``None`` (detached HEAD / GitError) is the safe direction.""" + with get_session() as session: + _seed_unmerged_rename(session) + aliases = resolve_path_aliases(session, "new.py", current_branch=None) + + assert aliases == {"new.py"} + + +def test_alias_scope_after_merge_is_branch_independent( + whygraph_db_initialized: Path, +) -> None: + """Case 47a — merging promotes the row, so the alias stands on its own.""" + with get_session() as session: + _seed_unmerged_rename(session) + # What the reconcile pass does once the branch lands. + merged = session.get(Commit, "c_branch") + merged.on_default_branch = 1 + session.add(merged) + session.commit() + + assert resolve_path_aliases(session, "new.py", current_branch=None) == { + "new.py", + "old.py", + } + assert resolve_path_aliases(session, "new.py", current_branch="unrelated") == { + "new.py", + "old.py", + } + + +def test_area_history_commit_set_stays_default_branch_only( + whygraph_db_initialized: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Case 47c — the alias set widens, the commit set does not.""" + with get_session() as session: + _seed_unmerged_rename(session) + + monkeypatch.setattr( + "whygraph.mcp.path_history.current_branch_scope", lambda: "feature/x" + ) + result = whygraph_area_history(path="new.py") + + shas = {item["commit"]["sha"] for item in result["evidence"]} + # c_main is reached only *through* the branch-local rename alias... + assert "c_main" in shas + # ...but the unmerged commit itself never enters a default-branch view. + assert "c_branch" not in shas diff --git a/tests/test_mcp_evidence_pr_origin.py b/tests/test_mcp_evidence_pr_origin.py index 8258f74..20fd718 100644 --- a/tests/test_mcp_evidence_pr_origin.py +++ b/tests/test_mcp_evidence_pr_origin.py @@ -121,25 +121,31 @@ def _build_squash_repo(root: Path) -> dict[str, str]: return {"squash": squash, "feat1": feat1, "feat2": feat2} -def _seed_squash_pr(shas: dict[str, str]) -> None: +def _seed_squash_pr(shas: dict[str, str], *, first_seen_ref: str | None = None) -> None: + """Seed the squash commit, its two originals, and the merged PR. + + ``first_seen_ref`` selects which population the flag-0 rows represent: + ``None`` (the default) leaves them as `PROriginEnricher` would write + them, and a branch name makes them locally-scanned feature commits — + the widening D4 deliberately accepts. + """ with get_session() as session: session.add( _squash_row(shas["squash"], committed_at="2026-04-01T00:00:00+00:00") ) - session.add( - _origin_row( - shas["feat1"], - subject="feat: first two lines", - committed_at="2026-03-01T00:00:00+00:00", - ) + first = _origin_row( + shas["feat1"], + subject="feat: first two lines", + committed_at="2026-03-01T00:00:00+00:00", ) - session.add( - _origin_row( - shas["feat2"], - subject="feat: third line", - committed_at="2026-03-02T00:00:00+00:00", - ) + second = _origin_row( + shas["feat2"], + subject="feat: third line", + committed_at="2026-03-02T00:00:00+00:00", ) + for row in (first, second): + row.first_seen_ref = first_seen_ref + session.add(row) session.add( PullRequest( number=1, @@ -289,3 +295,35 @@ def test_pr_origin_beats_area_but_loses_to_blame() -> None: assert _should_replace(_ev("s", "blame"), "pr-origin") is False # And pr-origin is not displaced by the weaker labels. assert _should_replace(_ev("s", "pr-origin"), "area") is False + + +def test_gate_also_fires_for_locally_scanned_feature_commits( + tmp_path: Path, + whygraph_db_initialized: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Case 44 (D4) — the gate stays wide, and that is the decision. + + Once ``GitCrawler`` flags unmerged work ``on_default_branch=0``, the + ``_enriched_squash_prs_for`` predicate ("at least one flag-0 oid + exists") matches locally-authored branches too. That widening is + deliberate: it buys squash-origin attribution on every branch this + machine developed, including on repos where the enricher never runs + (`provider = off`, or the `--no-remote` hook scans). Its cost is an + occasional wasted ``git blame``, which + ``test_attribute_squash_origins_degrades_on_bad_head_sha`` pins as + swallowed per-PR. + """ + shas = _build_squash_repo(tmp_path / "repo") + # Same rows, but provenance says "scanned locally off feature/x", not + # "fetched from refs/pull//head". + _seed_squash_pr(shas, first_seen_ref="feature/x") + + monkeypatch.chdir(tmp_path / "repo") + result = whygraph_evidence_for(path="sample.py", line_start=1, line_end=3) + + by_source: dict[str, set[str]] = {} + for item in result["evidence"]: + by_source.setdefault(item["source"], set()).add(item["commit"]["sha"]) + + assert by_source.get("pr-origin") == {shas["feat1"], shas["feat2"]} diff --git a/tests/test_scan_analyze_crawler.py b/tests/test_scan_analyze_crawler.py index f465987..f4c38ee 100644 --- a/tests/test_scan_analyze_crawler.py +++ b/tests/test_scan_analyze_crawler.py @@ -320,3 +320,33 @@ def test_one_failing_commit_does_not_block_the_rest( failed = [sha for sha, v in descs.items() if v[0] is None] assert len(described) == 2 # the other two still completed and committed assert len(failed) == 1 + + +def test_feature_branch_commits_are_still_described( + isolated_db: Path, repo_path: Path +) -> None: + """Case 47b (audit B4) — reachability, not the flag (plan §3 row 19). + + Once ``GitCrawler`` flags unmerged work ``on_default_branch=0``, the + naive reading of ``test_origin_commits_stay_lazy`` would be that such + work also stops getting described. It doesn't: ``AnalyzeCrawler`` + bounds its work by reachability from the current branch, and a + feature-branch commit *is* reachable. No LLM cost or coverage change. + """ + commits = _commits(repo_path) + _insert(commits) + # Flag them all off the default branch, exactly as a scan on an + # unmerged feature branch would. + with get_session() as session: + for row in session.exec(select(CommitRow)).all(): + row.on_default_branch = 0 + row.first_seen_ref = "feature/x" + session.add(row) + db_engine._reset_engine() + + crawler = _run(repo_path, _StubDescriptor()) + + assert crawler.error is None + descs = _descriptions() + for c in commits: + assert descs[c.sha][0] == "DESCRIPTION" diff --git a/tests/test_scan_pr_origin_enricher.py b/tests/test_scan_pr_origin_enricher.py index 793ff4c..0e75fb5 100644 --- a/tests/test_scan_pr_origin_enricher.py +++ b/tests/test_scan_pr_origin_enricher.py @@ -224,6 +224,34 @@ def test_work_inserts_origin_rows_and_fetches_only_candidates( assert squash_flag == 1 # the squash commit stays on the main walk +def test_origin_rows_record_their_own_pull_ref(whygraph_db_initialized: Path) -> None: + """Case 47f — ``first_seen_ref`` names the PR each row came from. + + Two candidates in one run: the number is threaded per-candidate, so a + shared ``scanned_at`` must not become a shared ref. + """ + with get_session() as session: + session.add(_commit("squash10", files_changed=40)) + session.add(_commit("squash11", files_changed=40)) + session.add(_pr(10, oids=["a10"], merge_commit_sha="squash10")) + session.add(_pr(11, oids=["a11"], merge_commit_sha="squash11")) + session.commit() + + repo = _StubRepo({"a10": _dc("a10"), "a11": _dc("a11")}) + enricher = PROriginEnricher( + Progress(), repository=repo, min_commits=5, large_commit_file_count=30 + ) + enricher.run() + + assert enricher.error is None + with get_session() as session: + refs = {sha: session.get(Commit, sha).first_seen_ref for sha in ("a10", "a11")} + # A commit on the main walk was never off it — no provenance to record. + assert session.get(Commit, "squash10").first_seen_ref is None + + assert refs == {"a10": "refs/pull/10/head", "a11": "refs/pull/11/head"} + + def test_work_no_candidates_makes_no_fetch(whygraph_db_initialized: Path) -> None: """When nothing is gated, the enricher never touches the network.""" with get_session() as session: diff --git a/tests/test_smoke.py b/tests/test_smoke.py index e3d706b..db09c0c 100644 --- a/tests/test_smoke.py +++ b/tests/test_smoke.py @@ -60,3 +60,10 @@ def test_mcp_server_registers_prompts() -> None: "whygraph_why_was_this_written", "whygraph_triage_commit", } + + +def test_cli_no_longer_registers_hooks_command() -> None: + """Case 46 — the group was removed; `[scan].hooks` + `init` replaced it.""" + from whygraph.cli import main + + assert "hooks" not in main.commands