diff --git a/.assets/provision/install_pwsh.sh b/.assets/provision/install_pwsh.sh index 1ee7b75f..179dba97 100755 --- a/.assets/provision/install_pwsh.sh +++ b/.assets/provision/install_pwsh.sh @@ -79,7 +79,13 @@ fedora) ;; debian | ubuntu) export DEBIAN_FRONTEND=noninteractive - [ "$SYS_ID" = 'debian' ] && apt-get update >&2 && apt-get install -y libicu76 >&2 2>/dev/null || true + . /etc/os-release + case "${VERSION_CODENAME:-}" in + trixie) libicu='libicu76' ;; + resolute) libicu='libicu78' ;; + *) libicu='' ;; + esac + [ -n "$libicu" ] && apt-get update >&2 2>/dev/null && apt-get install -y "$libicu" >&2 2>/dev/null || true # create temporary dir for the downloaded binary TMP_DIR=$(mktemp -d -p "$HOME") trap 'rm -fr "$TMP_DIR"' EXIT diff --git a/.assets/provision/install_uv.sh b/.assets/provision/install_uv.sh index 14ef3f4e..9039d18f 100755 --- a/.assets/provision/install_uv.sh +++ b/.assets/provision/install_uv.sh @@ -34,11 +34,21 @@ if [ -x "$HOME/.local/bin/uv" ]; then else # update uv using the self update command printf "\e[92mupdating \e[1m$APP\e[22m\n" >&2 + # build the env for self update: UV_SYSTEM_CERTS (uv 0.11.0+) supersedes the + # deprecated UV_NATIVE_TLS. Only add UV_NATIVE_TLS for legacy uv (< 0.11.0), + # which predates UV_SYSTEM_CERTS and still needs it for the TLS-verified update. + # An empty/unparseable VER means uv's output format changed - i.e. a newer uv - + # so treat it as new and skip the deprecated var to avoid the warning. + uv_env=(UV_SYSTEM_CERTS=true) + if [ -n "$VER" ] && [ "$VER" != '0.11.0' ] && + [ "$(printf '%s\n0.11.0\n' "$VER" | sort -V | head -n1)" = "$VER" ]; then + uv_env+=(UV_NATIVE_TLS=true) + fi # retry uv self update up to 5 times if it fails retry_count=0 max_retries=5 while [ $retry_count -le $max_retries ]; do - UV_SYSTEM_CERTS=true UV_NATIVE_TLS=true "$HOME/.local/bin/uv" self update >&2 + env "${uv_env[@]}" "$HOME/.local/bin/uv" self update >&2 [ $? -eq 0 ] && break || true ((retry_count++)) || true echo "retrying... $retry_count/$max_retries" >&2 diff --git a/.assets/provision/setup_profile_user.ps1 b/.assets/provision/setup_profile_user.ps1 index 6b0b2efb..b8f13484 100755 --- a/.assets/provision/setup_profile_user.ps1 +++ b/.assets/provision/setup_profile_user.ps1 @@ -15,6 +15,18 @@ $profileDir = [IO.Path]::GetDirectoryName($PROFILE) if (-not (Test-Path $profileDir -PathType Container)) { New-Item $profileDir -ItemType Directory | Out-Null } + +# *clean up obsolete modules superseded by a rename +# do-linux was renamed to do-unix; both export the same function/alias names, so a +# stale do-linux left in the user module path would shadow do-unix with duplicate +# commands. Remove it before the new module is installed. Only user scope is needed - +# do-linux was never installed AllUsers (do-common is the only system-wide module). +$staleModule = "$HOME/.local/share/powershell/Modules/do-linux" +if (Test-Path $staleModule -PathType Container) { + Write-Host 'removing obsolete do-linux module...' + Remove-Module -Name 'do-linux' -Force -ErrorAction SilentlyContinue + Remove-Item $staleModule -Recurse -Force +} # set up Microsoft.PowerShell.PSResourceGet and update installed modules if (Get-Module -Name Microsoft.PowerShell.PSResourceGet -ListAvailable) { if (-not (Get-PSResourceRepository -Name PSGallery).Trusted) { @@ -142,7 +154,7 @@ if (Test-Path "$HOME/$uvCli" -PathType Leaf) { # set up make completer $completerFunction = 'Register-MakeCompleter' -if (Get-Command $completerFunction -Module 'do-linux' -CommandType Function -ErrorAction SilentlyContinue) { +if (Get-Command $completerFunction -Module 'do-unix' -CommandType Function -ErrorAction SilentlyContinue) { if (-not ($profileContent | Select-String $completerFunction -SimpleMatch -Quiet)) { Write-Host 'adding make auto-completion...' $profileContent.AddRange( diff --git a/.assets/scripts/linux_setup.sh b/.assets/scripts/linux_setup.sh index ea748202..e3fee22a 100755 --- a/.assets/scripts/linux_setup.sh +++ b/.assets/scripts/linux_setup.sh @@ -262,7 +262,7 @@ if [ -f /usr/bin/pwsh ]; then sudo cp -rf modules/do-common /usr/local/share/powershell/Modules/ # determine current user scope modules to install - modules=('do-linux') + modules=('do-unix') grep -qw 'az' <<<$scope && modules+=(do-az) || true [ -f /usr/bin/git ] && modules+=(aliases-git) || true [ -f /usr/bin/kubectl ] && modules+=(aliases-kubectl) || true diff --git a/.assets/scripts/modules_update.ps1 b/.assets/scripts/modules_update.ps1 index 61c583b9..d2fb27cf 100755 --- a/.assets/scripts/modules_update.ps1 +++ b/.assets/scripts/modules_update.ps1 @@ -59,7 +59,7 @@ process { 'aliases-kubectl' 'do-az' 'do-common' - 'do-linux' + 'do-unix' 'psm-windows' ) foreach ($module in $modules) { diff --git a/.assets/scripts/vg_cacert_fix.ps1 b/.assets/scripts/vg_cacert_fix.ps1 index 14fe49aa..3e76e2de 100644 --- a/.assets/scripts/vg_cacert_fix.ps1 +++ b/.assets/scripts/vg_cacert_fix.ps1 @@ -43,7 +43,7 @@ process { } # intercept certificates from chain and filter out existing ones - $chain = Get-Certificate -Uri 'gems.hashicorp.com' -BuildChain | Select-Object -Skip 1 | Where-Object { + $chain = Get-Certificate -Uri 'gems.hashicorp.com' -PresentedChain | Select-Object -Skip 1 | Where-Object { $_.Thumbprint -notin $cacert.Thumbprint } diff --git a/.assets/scripts/vg_certs_add.ps1 b/.assets/scripts/vg_certs_add.ps1 index 68964b18..215554fd 100755 --- a/.assets/scripts/vg_certs_add.ps1 +++ b/.assets/scripts/vg_certs_add.ps1 @@ -75,7 +75,7 @@ $Path = Resolve-Path $Path $content = [IO.File]::ReadAllLines($Path) # intercept certificates from chain and filter out existing ones -$chain = Get-Certificate -Uri 'gems.hashicorp.com' -BuildChain | Select-Object -Skip 1 +$chain = Get-Certificate -Uri 'gems.hashicorp.com' -PresentedChain | Select-Object -Skip 1 # create installation script New-Item (Split-Path $scriptInstallRootCA) -ItemType Directory -ErrorAction SilentlyContinue | Out-Null diff --git a/.claude/prepare-pr.toml b/.claude/prepare-pr.toml new file mode 100644 index 00000000..6b728964 --- /dev/null +++ b/.claude/prepare-pr.toml @@ -0,0 +1,28 @@ +# prepare-pr per-repo config +# +# Consumed by .claude/skills/prepare-pr/scripts/extract_signals.py. Keeps the +# skill's script portable: repo-specific layout lives here, not in the script. +# Absent/partial config degrades to a safe no-op (the ARCHITECTURE staleness +# check simply finds nothing). See ARCHITECTURE.md and the skill's SKILL.md. + +# Doc scanned for staleness by the `architecture` subcommand (Phase 1.5c). +architecture_doc = "ARCHITECTURE.md" + +# Where extracted operational lessons are written (Phase 1.5b). +lessons_path = "design/lessons.md" + +# Maps an ARCHITECTURE.md section identifier -> path prefixes that, when touched +# by the branch diff, mark that section potentially stale. Keys are section names +# the skill echoes back in `stale_sections`; the agent then finds the matching +# heading in ARCHITECTURE.md (this repo's headings are numbered, e.g. +# `## 1. Host vs guest split`, and carry no parenthesized IDs). Only the doc's +# concrete inventories (tables/trees) are mapped - they are what actually drifts: +# host_guest_split -> § 1 (host/guest split table) +# hook_inventory -> § 3 (pre-commit hook inventory table) +# test_layout -> § 4 (test layout tree) +# powershell_modules-> § 6 (module inventory + sync list / install sites) +[architecture_sections] +host_guest_split = ["wsl/", ".assets/scripts/", ".assets/provision/", ".assets/trigger/"] +hook_inventory = [".pre-commit-config.yaml", "tests/hooks/"] +test_layout = ["tests/"] +powershell_modules = ["modules/", ".assets/scripts/modules_update.ps1"] diff --git a/.claude/skills/address-pr-review/SKILL.md b/.claude/skills/address-pr-review/SKILL.md new file mode 100644 index 00000000..17249079 --- /dev/null +++ b/.claude/skills/address-pr-review/SKILL.md @@ -0,0 +1,129 @@ +--- +name: address-pr-review +description: State-aware GitHub Copilot PR review handler. Detects review state (not triggered / in progress / has unresolved threads / clean), triggers Copilot via `gh pr edit --add-reviewer` when needed, polls until completion, classifies fresh unresolved comments as fix/resolve-only/skip, applies fixes, resolves threads via GraphQL, and pushes. Only exits when the fresh review (matching HEAD SHA) has no unresolved fresh threads. Use when the user types `/address-pr-review`, asks to address PR comments, wants to clear review findings, or says "check the PR review." Disabled for auto-invocation. +disable-model-invocation: true +--- + +# Address PR review + +State-aware Copilot PR review handler. Detects the current review state, drives toward "fresh review clean," and processes only fresh unresolved threads. Works with any reviewer that posts inline PR comments; Copilot is the default trigger target. + +## When to use + +- `/address-pr-review` - drive current branch's PR review to a clean state +- `/address-pr-review 37` - same, for a specific PR number +- "address the PR review comments" / "check the PR review" / "clear the review" - same as `/address-pr-review` + +## Prerequisites + +- `gh` CLI installed and authenticated. +- Copilot enabled on the repository. + +**Important:** always trigger Copilot via `pr_review.py trigger`, never via ad-hoc `gh pr edit --add-reviewer` invocations or raw GraphQL mutations from the agent. `pr_review.py trigger` wraps `gh pr edit --add-reviewer copilot-pull-request-reviewer` with the right reviewer login, idempotency handling, and uniform error reporting; one-off calls drift in subtle ways (wrong login alias, no idempotency, no JSON output for the skill's state machine to consume). If `trigger` fails, surface the error to the user - Copilot may need to be enabled in repo settings. + +## Review states + +The skill operates as a state machine. Four states are possible: + +| State | Fresh review exists? | Copilot requested? | Unresolved fresh threads? | Action | +| ----- | -------------------- | ------------------ | ------------------------- | ---------------- | +| **A** | No | No | N/A | Trigger + wait | +| **B** | No | Yes (in progress) | N/A | Wait | +| **C** | Yes | No | Yes | **Process** | +| **D** | Yes | No | No | **EXIT** (clean) | + +"Fresh review" = a Copilot review whose `commit.oid` matches the PR's current `headRefOid`. A review from a prior push is **stale** and ignored - the skill triggers a new one. + +"Fresh threads" = threads with `isResolved: false` AND `isOutdated: false`. Outdated unresolved threads are silently ignored - the fresh review re-evaluates the same code. + +**State D is the only clean exit.** Every other state drives toward it via trigger/wait/process. + +## Workflow + +### Phase 1 - detect state + +```bash +uv run --frozen python .claude/skills/address-pr-review/scripts/pr_review.py state --pr +``` + +Returns JSON with `state`, `headSha`, `freshReviewSha`, `copilotRequested`, and `unresolvedFreshThreads`. Exit code: `0`=D, `1`=C, `2`=B, `3`=A. + +If no PR is found on the current branch (or `--pr` is invalid), the script exits with a clear error. Surface it to the user and stop. + +### Phase 2 - drive to State D + +Branch on the state from Phase 1: + +- **State A** (not triggered): run `pr_review.py trigger --pr ` to request Copilot. Then `pr_review.py wait --pr ` to poll until the review completes. The `wait` subcommand returns when the state resolves to C or D. Continue from that state. +- **State B** (in progress): skip trigger, go straight to `pr_review.py wait --pr `. Same continuation. +- **State C** (unresolved fresh threads): proceed to Phase 3. +- **State D**: announce "PR review clean - no unresolved fresh threads. Exit." Done. + +`wait` polls every 30 seconds, up to 8 min total. On timeout (exit 4), surface to the user - Copilot may be queued or the service may be slow. Don't loop the wait; let the user decide whether to retry. + +### Phase 3 - process unresolved fresh threads (State C only) + +The `state` JSON already contains `unresolvedFreshThreads` with `{id, path, line, author, body}` for each. Present them as a summary table: + +```text +## PR #37 - N unresolved fresh threads + +| # | File:Line | Author | Summary | +|---|-----------|--------|---------| +| 1 | src/main.py:9 | copilot | Missing error handling on API call | +| 2 | docs/index.md:109 | copilot | Stale reference count | +``` + +**Known false positives** - auto-resolve without reading the file: + +- Comments flagging `ubuntu-slim` as an invalid or non-standard GitHub runner (e.g., "runs-on: ubuntu-slim is likely to fail"). `ubuntu-slim` is a valid GitHub-hosted runner that this repo uses intentionally. + +For each remaining thread, read the comment body + the referenced file at the specified line. Classify: + +- **`fix`** - the comment identifies a real issue (bug, missing code, inconsistency, stale reference). Read the referenced file, formulate a fix using Claude's knowledge of the codebase (not a copy-paste of the suggestion), apply via Edit. +- **`resolve-only`** - the comment is already addressed by a prior fix in this session, or the issue genuinely doesn't apply (e.g., stale on a specific line but the file changed elsewhere). Resolve silently. +- **`skip`** - the comment is a suggestion, design question, or judgment call that needs human input. + +Resolve `fix` and `resolve-only` threads via: + +```bash +uv run --frozen python .claude/skills/address-pr-review/scripts/pr_review.py resolve +``` + +After processing all `fix` and `resolve-only` items: + +- If any `skip` items remain: present them to the user via `AskUserQuestion`. For each, offer three options: "Fix it" (Claude fixes now), "Resolve without fix" (intentional choice), "Leave open" (for later / human reviewer to decide). Act on user's choices. +- Report: "N fixed, M resolved (stale/intentional), K surfaced to user." + +### Phase 4 - commit and push + +**Only runs in standalone mode** - when the skill is invoked directly, not as part of a larger workflow. The caller decides commit topology; the skill's job is to surface fixes and resolve threads. + +When invoked by a caller that manages its own commit flow (e.g., a consolidation skill that re-cuts commits), the caller should pass context indicating Phase 4 should be skipped. Without that context, Phase 4 runs by default. + +If any files were edited in Phase 3: + +1. Run `make lint` to validate. +2. Stage the changed files explicitly (never `git add -A`). +3. Commit: `git commit -m "fix: address PR review comments"` +4. Push: `git push` + +The push will trigger a new Copilot review automatically. The skill is **one-shot** - it does not re-invoke itself after pushing. The user decides whether to re-run. + +If no files were edited (all threads were `resolve-only` or `skip` → leave-open), skip the commit - just report the resolution results. + +## Anti-patterns + +- **Processing outdated threads.** Threads with `isOutdated: true` reference code that may no longer exist at that line. The fresh review re-evaluates the same code; if the issue persists, it appears as a fresh thread. Silently ignoring outdated threads is correct. +- **Treating a stale review as fresh.** A review whose `commit.oid` doesn't match the current `headRefOid` is from a prior push. Don't process its threads even if they're unresolved - trigger a new review instead. +- **Resolving `skip` items silently.** The human might want to act on them - a "consider X" suggestion might actually be a good idea. Surface, don't suppress. +- **Posting reply comments before resolving.** Silent resolve is the design choice - keeps the PR history clean. The fix is visible in the diff; the resolution is visible in the thread state. +- **Looping `wait` on timeout.** If `wait` exits 4 (timeout), don't immediately re-call it. Surface to the user; Copilot may be queued or rate-limited. +- **Copying the reviewer's suggested fix verbatim.** The reviewer (Copilot or human) suggests direction; Claude writes the actual fix using its knowledge of the codebase's patterns and accepted decisions. +- **Resolving threads for human reviewers' comments without checking.** The skill processes ALL unresolved fresh threads regardless of author. If a human reviewer left a comment expecting a human reply, resolving silently would be rude. Classify these as `skip` unless the fix is unambiguous. + +## Example invocations + +- `/address-pr-review` - address comments on current branch's PR +- `/address-pr-review 37` - address comments on PR #37 +- "Check the PR review and fix what you can" - same as `/address-pr-review` diff --git a/.claude/skills/address-pr-review/scripts/pr_review.py b/.claude/skills/address-pr-review/scripts/pr_review.py new file mode 100755 index 00000000..d5aff9f1 --- /dev/null +++ b/.claude/skills/address-pr-review/scripts/pr_review.py @@ -0,0 +1,362 @@ +#!/usr/bin/env -S uv run --frozen python +""" +pr_review.py - state-aware GitHub PR review thread management. + +Companion script for the /address-pr-review skill. Handles Copilot review +lifecycle: detect current state, trigger reviews, wait for completion, and +resolve threads via GraphQL. + +Review states +------------- +A = no fresh Copilot review exists, none requested -> trigger + wait +B = no fresh Copilot review exists, one is queued -> wait +C = fresh Copilot review exists, unresolved threads -> process them +D = fresh Copilot review exists, no unresolved -> DONE (only clean-exit) + +"Fresh" means a review whose commit SHA matches the PR's current HEAD SHA. +"In progress" means Copilot is in the PR's requested_reviewers list. + +Subcommands +----------- +state --pr N + Detect the current state. Prints JSON; exit code = state-specific: + 0 = D (clean), 1 = C (unresolved), 2 = B (in progress), 3 = A (none). + +trigger --pr N + Request Copilot review (gh pr edit --add-reviewer). Idempotent. + +wait --pr N [--interval 30] [--timeout 480] + Poll until state resolves to C or D. Same JSON shape as `state`. + Exit 1 = C, 0 = D, 4 = timeout. + +resolve + Resolve a single review thread via GraphQL. + +# :example +.claude/skills/address-pr-review/scripts/pr_review.py state --pr 37 +.claude/skills/address-pr-review/scripts/pr_review.py trigger --pr 37 +.claude/skills/address-pr-review/scripts/pr_review.py wait --pr 37 --timeout 480 +.claude/skills/address-pr-review/scripts/pr_review.py resolve PRRT_xxx +""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys +import time + +COPILOT_REVIEWER_LOGIN = "copilot-pull-request-reviewer" # author of submitted reviews +# The requested_reviewers users list may surface Copilot under either login +# depending on the API surface: "Copilot" (bot display login) or the same +# "copilot-pull-request-reviewer" slug used when triggering. Accept both so +# state detection doesn't misclassify an in-progress review as "not requested" +# and re-trigger in a loop. +COPILOT_REQUESTED_LOGINS = frozenset({"Copilot", COPILOT_REVIEWER_LOGIN}) + +STATE_QUERY = """ +query($owner: String!, $repo: String!, $pr: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + headRefOid + reviews(last: 50) { + nodes { + author { login } + submittedAt + commit { oid } + } + } + } + } +} +""" + +THREADS_PAGE_QUERY = """ +query($owner: String!, $repo: String!, $pr: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { endCursor hasNextPage } + nodes { + id + isResolved + isOutdated + comments(first: 1) { + nodes { + body + path + line + originalLine + author { login } + } + } + } + } + } + } +} +""" + +RESOLVE_MUTATION = """ +mutation($threadId: ID!) { + resolveReviewThread(input: {threadId: $threadId}) { + thread { isResolved } + } +} +""" + + +def _run_gh(cmd: list[str]) -> str: + """ + Run a gh command, returning stdout. Exit with a concise message on failure. + + Using check=True here would raise CalledProcessError and dump a Python + traceback on common, expected failures (gh not installed, not logged in, + missing scopes). Surface a readable one-line error the calling skill can + show the user instead. + """ + try: + result = subprocess.run(cmd, capture_output=True, text=True) + except FileNotFoundError: + print("gh CLI not found on PATH. Install and authenticate gh.", file=sys.stderr) + raise SystemExit(1) from None + if result.returncode != 0: + detail = result.stderr.strip() or result.stdout.strip() or "unknown error" + print(f"gh command failed ({' '.join(cmd[:3])} ...): {detail}", file=sys.stderr) + raise SystemExit(1) + return result.stdout + + +def _repo_info() -> tuple[str, str]: + """Return (owner, repo_name) from gh.""" + data = json.loads(_run_gh(["gh", "repo", "view", "--json", "owner,name"])) + return data["owner"]["login"], data["name"] + + +def _auto_pr() -> int: + """Auto-detect PR number from current branch.""" + try: + result = subprocess.run( + ["gh", "pr", "view", "--json", "number", "--jq", ".number"], + capture_output=True, + text=True, + ) + except FileNotFoundError: + print("gh CLI not found on PATH. Install and authenticate gh.", file=sys.stderr) + raise SystemExit(1) from None + if result.returncode != 0 or not result.stdout.strip(): + print("No open PR on this branch. Push first or specify --pr.", file=sys.stderr) + raise SystemExit(1) + return int(result.stdout.strip()) + + +def _graphql(query: str, **variables: str | int) -> dict: + """Run a GraphQL query via gh api.""" + cmd = ["gh", "api", "graphql", "-f", f"query={query}"] + for k, v in variables.items(): + flag = "-F" if isinstance(v, int) else "-f" + cmd.extend([flag, f"{k}={v}"]) + return json.loads(_run_gh(cmd)) + + +def _copilot_requested(owner: str, repo: str, pr: int) -> bool: + """Check if Copilot is in the PR's requested_reviewers list.""" + data = json.loads( + _run_gh(["gh", "api", f"repos/{owner}/{repo}/pulls/{pr}/requested_reviewers"]) + ) + return any( + u.get("login") in COPILOT_REQUESTED_LOGINS for u in data.get("users", []) + ) + + +def _flatten_thread(thread: dict) -> dict: + """Flatten a thread node into a simple dict for Claude to consume.""" + comments = thread["comments"]["nodes"] + first = comments[0] if comments else {} + return { + "id": thread["id"], + "isOutdated": thread["isOutdated"], + "path": first.get("path", ""), + "line": first.get("line") or first.get("originalLine"), + "author": (first.get("author") or {}).get("login", "unknown"), + "body": first.get("body", ""), + } + + +def _fetch_all_threads(owner: str, repo: str, pr: int) -> list[dict]: + """Fetch all review threads via cursor pagination (no first:N cap).""" + all_threads: list[dict] = [] + cursor: str | None = None + while True: + kwargs: dict[str, str | int] = {"owner": owner, "repo": repo, "pr": pr} + if cursor is not None: + kwargs["cursor"] = cursor + data = _graphql(THREADS_PAGE_QUERY, **kwargs) + block = data["data"]["repository"]["pullRequest"]["reviewThreads"] + all_threads.extend(block["nodes"]) + if not block["pageInfo"]["hasNextPage"]: + break + cursor = block["pageInfo"]["endCursor"] + return all_threads + + +def _detect_state(owner: str, repo: str, pr: int) -> dict: + """Run state detection. Returns a dict for JSON output + exit-code decisions.""" + data = _graphql(STATE_QUERY, owner=owner, repo=repo, pr=pr) + pr_node = (data.get("data") or {}).get("repository", {}).get("pullRequest") + if pr_node is None: + print( + f"PR #{pr} not found in {owner}/{repo} (invalid number or no access).", + file=sys.stderr, + ) + raise SystemExit(1) + head_sha = pr_node["headRefOid"] + + # Find the most-recent Copilot review matching HEAD SHA. GraphQL can return + # author: null (deleted user) or commit: null (edge cases), so guard both + # rather than indexing blindly. + copilot_reviews = [ + r + for r in pr_node["reviews"]["nodes"] + if (r.get("author") or {}).get("login") == COPILOT_REVIEWER_LOGIN + ] + copilot_reviews.sort(key=lambda r: r.get("submittedAt") or "", reverse=True) + fresh_review = next( + (r for r in copilot_reviews if (r.get("commit") or {}).get("oid") == head_sha), + None, + ) + fresh_review_sha = ( + (fresh_review.get("commit") or {}).get("oid") if fresh_review else None + ) + + # Only fetch review threads when a fresh review exists (states C/D). Without + # one (states A/B), the threads are irrelevant to classification, so skip the + # paginated fetch - it would add unnecessary GitHub API calls to every `wait` + # poll and risk rate limits. + fresh_threads: list[dict] = [] + if fresh_review_sha is not None: + all_threads = _fetch_all_threads(owner, repo, pr) + fresh_threads = [ + _flatten_thread(t) + for t in all_threads + if not t["isResolved"] and not t["isOutdated"] + ] + state = "C" if fresh_threads else "D" + # A fresh review exists, so state is C/D regardless of this flag. Report the + # real requested-reviewer status rather than assuming True, so the field + # stays semantically accurate for callers. + copilot_requested = _copilot_requested(owner, repo, pr) + else: + copilot_requested = _copilot_requested(owner, repo, pr) + state = "B" if copilot_requested else "A" + + return { + "state": state, + "headSha": head_sha, + "freshReviewSha": fresh_review_sha, + "copilotRequested": copilot_requested, + "unresolvedFreshThreads": fresh_threads, + } + + +def _state_exit_code(state: str) -> int: + """Map state letter to exit code (D=0, C=1, B=2, A=3).""" + return {"D": 0, "C": 1, "B": 2, "A": 3}[state] + + +def cmd_state(args: argparse.Namespace) -> int: + """Detect and print current review state.""" + pr = args.pr or _auto_pr() + owner, repo = _repo_info() + result = _detect_state(owner, repo, pr) + json.dump(result, sys.stdout, indent=2) + print() + return _state_exit_code(result["state"]) + + +def cmd_trigger(args: argparse.Namespace) -> int: + """Request Copilot review (idempotent).""" + pr = args.pr or _auto_pr() + # route through _run_gh for consistent, traceback-free errors (missing gh, + # auth failure, etc.) - it exits with a concise message on failure + _run_gh(["gh", "pr", "edit", str(pr), "--add-reviewer", COPILOT_REVIEWER_LOGIN]) + print(json.dumps({"triggered": True, "pr": pr})) + return 0 + + +def cmd_wait(args: argparse.Namespace) -> int: + """Poll until state resolves to C or D, or timeout.""" + pr = args.pr or _auto_pr() + owner, repo = _repo_info() + deadline = time.monotonic() + args.timeout + attempt = 0 + + while time.monotonic() < deadline: + attempt += 1 + result = _detect_state(owner, repo, pr) + + if result["state"] in ("C", "D"): + json.dump(result, sys.stdout, indent=2) + print() + return _state_exit_code(result["state"]) + + remaining = int(deadline - time.monotonic()) + print( + f"Poll #{attempt}: state={result['state']} " + f"(requested={result['copilotRequested']}, {remaining}s remaining)...", + file=sys.stderr, + ) + time.sleep(args.interval) + + # Timeout: emit the last-known state and exit 4. + final = _detect_state(owner, repo, pr) + json.dump(final, sys.stdout, indent=2) + print() + return 4 + + +def cmd_resolve(args: argparse.Namespace) -> int: + """Resolve a single thread.""" + data = _graphql(RESOLVE_MUTATION, threadId=args.thread_id) + resolved = data["data"]["resolveReviewThread"]["thread"]["isResolved"] + json.dump({"resolved": resolved}, sys.stdout) + print() + return 0 if resolved else 1 + + +def main(argv: list[str]) -> int: + """Parse args, dispatch to state/trigger/wait/resolve.""" + parser = argparse.ArgumentParser(description=__doc__.splitlines()[1]) + sub = parser.add_subparsers(dest="command", required=True) + + p_state = sub.add_parser("state", help="Detect current review state") + p_state.add_argument("--pr", type=int, help="PR number (auto-detect if omitted)") + + p_trigger = sub.add_parser("trigger", help="Request Copilot review") + p_trigger.add_argument("--pr", type=int, help="PR number (auto-detect if omitted)") + + p_wait = sub.add_parser("wait", help="Poll until state resolves to C or D") + p_wait.add_argument("--pr", type=int, help="PR number (auto-detect if omitted)") + p_wait.add_argument( + "--interval", type=int, default=30, help="Seconds between polls" + ) + p_wait.add_argument( + "--timeout", type=int, default=480, help="Total timeout in seconds" + ) + + p_resolve = sub.add_parser("resolve", help="Resolve a review thread") + p_resolve.add_argument("thread_id", help="GraphQL node ID (PRRT_*)") + + args = parser.parse_args(argv) + handlers = { + "state": cmd_state, + "trigger": cmd_trigger, + "wait": cmd_wait, + "resolve": cmd_resolve, + } + return handlers[args.command](args) + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) diff --git a/.claude/skills/prepare-pr/SKILL.md b/.claude/skills/prepare-pr/SKILL.md new file mode 100644 index 00000000..c6558689 --- /dev/null +++ b/.claude/skills/prepare-pr/SKILL.md @@ -0,0 +1,416 @@ +--- +name: prepare-pr +description: Wrap a feature branch up into a clean, shippable PR - consolidate WIP commits into a small set of Conventional Commits by prefix (feat/fix/chore/docs/test/refactor), run `make lint-diff` once at the end, push, then create or update a descriptive PR. Skips per-commit hooks via `--no-verify` and runs them in one pass after consolidation. Use when the user types `/prepare-pr`, says "wrap this branch up", "ship this branch", "open a PR for this", "consolidate the commits and PR", or similar end-of-branch language. Disabled for auto-invocation. +disable-model-invocation: true +--- + +# Prepare PR + +End-of-branch consolidation skill for this repo. Take a branch with ad-hoc WIP commits + lint side-effects accumulated during development, regroup the file-level changes by Conventional Commits prefix, run lint once, push, and open or update a descriptive PR. + +This repo ships versioned tooling - several PowerShell modules under `modules/`, each with a `ModuleVersion` in its `modules//.psd1` manifest (`do-common`, `do-unix`, `do-az`, `utils-install`, `utils-setup`, `psm-windows`, `aliases-git`, `aliases-kubectl`) - but it does not gate releases on a CHANGELOG or automated SemVer arithmetic, so this skill takes no version-bump argument. If a change warrants a module version bump, that's an explicit edit the author makes in the relevant commit; the skill just produces clean commits and a clear PR. (There is no Python/npm package here - no `pyproject.toml`/`package.json` - so the only versioned artifacts are the `.psd1` manifests.) + +## When to use + +- `/prepare-pr` - run the full pipeline against the current branch +- `/prepare-pr --skip-review` - skip Phase 1.5 (`/second-opinion`) and Phase 4.5 (`/address-pr-review`); use when Copilot is offline, or when you've already run both skills manually earlier in the session +- "wrap this branch up" / "ship this branch" / "open a PR for this" - same flow +- "update the PR with the new commits" - re-run; history rewritten + force-pushed, PR body updated + +## Workflow + +Four numbered phases plus two interstitials (Phase 1.5 heterogeneous-model review, Phase 4.5 PR review address). Stop and surface the error if any phase fails - do not paper over. + +### Phase 1: Survey and categorize + +Get the lay of the land *before* proposing any changes to history. + +1. **Sync the local trunk.** Run this **first**, before anything else touches the trunk - every later merge-base, diff, and lint compares against it, and a stale local trunk silently poisons all of them (a stale local `main` inflated the branch's `main..HEAD` from 15 to 33 commits in one real run, and would have made the Phase 2 soft-reset revert already-merged commits). `sync-trunk` fetches origin and fast-forwards the local trunk ref if it is behind - it never checks out trunk (the working tree is untouched; it moves the ref with `git update-ref`), so it's safe to run from the feature branch. + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py sync-trunk + ``` + + Read the JSON `action`: + - `up-to-date` / `fast-forwarded` / `no-local` / `no-origin` → proceed. + - `offline` (fetch failed) → non-fatal; announce it and proceed (origin/local refs are whatever's on disk). The `--force-with-lease` in Phase 4 is the backstop. + - `diverged` (exit 1) → **stop.** The local trunk has commits not on origin - a broken state a fast-forward can't resolve (fast-forwards never conflict; divergence means real local commits). Surface the JSON `reason` and tell the user to reconcile (`git checkout && git pull --ff-only`, or investigate the local commits) before re-running. + + This also fixes a repo-local footgun: this repo's `make lint-diff` hard-codes `--from-ref main` (the **local** ref), so without this step Phase 3's lint validates against a stale base too. + +2. **Detect the trunk branch.** This skill is a gallery item portable across repos - never assume `main`. The bundled `extract_signals.py` script resolves trunk via the fallback chain `git symbolic-ref refs/remotes/origin/HEAD` → `gh repo view --json defaultBranchRef` → probe `main`/`master`/`trunk`/`prod` against both local and remote refs. Each downstream subcommand auto-resolves trunk internally; only the agent-facing display below needs separate calls: + + - **`trunk-name`** - prints the bare branch name (`main`, `master`, ...). Pass this literal to `gh pr create --base `. + - **`trunk-ref`** - prints a fully-resolvable git ref (`origin/main` when a remote-tracking ref exists, else the local `main` branch). Pass this literal to `git log ..HEAD`, `git diff ..HEAD`, `git merge-base HEAD`, etc. It prefers `origin/` because that's authoritative - it's what the PR is computed against and stays current on fetch, whereas a local trunk branch often lags in a feature-branch workflow and would make every merge-base resolve too far back (pulling already-merged commits into the diff/reset). Run `git fetch` first so `origin/` is current. Many CI environments only have trunk at `refs/remotes/origin/`, so plain `main` would fail there anyway. + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py trunk-name + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py trunk-ref + ``` + + Substitute the printed values as **literal arguments** in subsequent commands - never `$VAR` interpolation, since shell variables don't persist across the agent's separate `Bash` tool calls. For example, if `trunk-ref` prints `main`, the next call is literally `git log --oneline main..HEAD`; if it prints `origin/main`, the next call is literally `git log --oneline origin/main..HEAD`. Sticking to plain commands (no `eval`, no `$(...)` substitution) keeps every `Bash` invocation statically analyzable by the permission matcher, so they don't trigger approval prompts. + + Non-zero exit + stderr message means trunk resolution failed - the typical remedy is `git remote set-head origin -a` (or `git fetch origin ` if the trunk's ref isn't available locally at all). Surface the error verbatim; silent misdetection is worse than a noisy abort. The script is bundled because the bash version of this chain has two famous footguns caught in PR review: `git symbolic-ref ... | sed ...` masks failures (the upstream `git symbolic-ref` exit code gets shadowed by `sed`'s always-zero exit), and the local-only `for c in ... do git show-ref refs/heads/$c` probe misses CI checkouts where trunk is only at `refs/remotes/origin/`. + +3. **Verify the branch is safe to rewrite.** Run `extract_signals.py branch-safety` - it refuses (non-zero exit, structured JSON output) if the current branch matches the resolved trunk name, is in `{main, master, develop, trunk, prod}`, or matches `^release/`. Belt-and-suspenders matters here because a repo can have a default branch named `main` *and* a long-lived `master` you also shouldn't rewrite. The check is fail-closed: if trunk resolution itself fails, the JSON returns `safe: false` rather than letting an unguarded rewrite through. On refusal, surface the JSON's `reason` field and stop; tell the user to switch to a feature branch. + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py branch-safety + ``` + +4. **Capture the current HEAD SHA** for emergency restore. Print it in the output so the user can `git reset --hard ` if anything goes wrong. + + ```bash + git rev-parse HEAD + git branch --show-current + ``` + +5. **List what's changed since trunk**, plus anything uncommitted or staged (lint side-effects often live in the index). Substitute the `trunk-ref` value from step 2 as a literal in the two trunk-relative commands: + + ```bash + git log --oneline ..HEAD + git diff --name-status ..HEAD + git diff --name-status # uncommitted (working tree) + git diff --cached --name-status # staged + ``` + +6. **Read the repo's commit-convention doc before classifying.** If a project doc exists (common names: `ARCHITECTURE.md`, `AGENTS.md`, `CONTRIBUTING.md`, `CLAUDE.md`, `.github/`), skim it for commits/conventions and versioning sections. **Repo-specific rules override the generic taxonomy below.** The generic prefixes are only a fallback for repos with no documented convention - never let them override a rule the repo actually states. In particular, watch for: per-scope prefixes and any dedicated release/version-bump commit form the repo prescribes, and versioned artifacts that must be committed separately (see the grouping rule below). + +7. **Classify each changed file into one Conventional Commits prefix, grouped by `(prefix, concern)` - not prefix alone:** + - `feat` - new functionality or a new file the project didn't have + - `fix` - bug fix; existing thing didn't work, now works + - `chore` - tooling, build config, dependency bumps, CI, hook tweaks, gitignore + - `docs` - vault content (`docs/**`), READMEs, design docs, skill bodies + - `test` - new or updated tests with no production change in the same file + - `refactor` - behavior-preserving cleanup + - For `docs` changes inside a specific vault, scope it: `docs(networking)`, `docs(networking/icp)`, `docs(tls-certificates-kb)`. For hook changes, scope `chore(hooks)` or `fix(hooks)`. Scope helps reviewers skim. + - **Two files sharing a prefix do NOT automatically share a commit.** A new module function and a new script that uses it are both `feat`, but if the repo separates them (see below) they are two commits. Group by the *logical change*, then apply the prefix - not the other way around. + - **Versioned artifacts get their own commit + version bump.** If the repo versions a module or package via a manifest (e.g. a PowerShell module's `ModuleVersion`, a `pyproject.toml`/`package.json` `version`) and any file **under that artifact's root** changed, then: (a) that commit contains **only** the artifact's files - never mixed with the scripts/docs/tooling that merely consume it; (b) the version bump lives in that same commit (SemVer by the largest change - new public surface = MINOR, fix = PATCH, breaking change = MAJOR); (c) the message follows whatever release form the repo prescribes (from step 6). Files that only *use* the artifact do not trigger a bump. If the repo documents no versioning rule, skip this - don't invent version bumps a repo doesn't ask for. + - **Bump only against *released* code - fold within-branch fixes into the commit that introduced them.** Before adding an increment, check whether the artifact code you're changing was introduced *earlier on this same branch* (still unreleased) vs. already exists on trunk. Run `git log --oneline ..HEAD -- ` (or `git log -S '' ..HEAD`): if the code being fixed first appeared in a branch commit that itself bumped the version, the fix is part of *that* unreleased change - fold it into that commit and keep its version, do **not** stack a second increment. A branch that adds a function at vX.Y.0 and then corrects it before merge ships as a single vX.Y.0, not vX.Y.0 + vX.Y.1. Only increment when changing code that is already on trunk (already released). This is the #1 versioning mistake: treating a pre-merge correction to just-introduced code as a new SemVer release. + +8. **Decide whether consolidation is worth doing.** If the existing commits on the branch are already clean (small N, conventional-commit titles, one logical change per commit, **and each commit respects the repo's separation/versioning rules from step 6-7**), skip Phase 2 entirely and jump to Phase 3. Heuristic: if the trunk-to-HEAD `git log --oneline` (from step 5) shows <= 3 commits AND each title starts with a conventional prefix AND the working tree + index are clean, consolidation adds noise rather than removing it. **But a low commit count is not sufficient** - a single commit that mixes a versioned artifact with its consumers (or bumps nothing when the versioned artifact changed) still warrants a re-cut, so inspect content, not just count. Otherwise propose the consolidation plan. + +9. **Produce a categorization plan**: which files go in which commit, in which order, with a draft commit message for each. Bring it to the user via `AskUserQuestion` (or a plain prompt if a single yes/no): **approve, modify, or abort**. + + Do not execute history rewrites until the user approves. The user knows things about their work that the file diffs don't capture. + +### Phase 1.5: Heterogeneous-model review (optional) + +Invoke `/second-opinion` for an author-time review by a different model family (GitHub Copilot CLI with GPT) before the destructive soft-reset. The point of running here, not after Phase 2: any review-driven fixes get absorbed into the still-WIP commit history and Phase 2's per-prefix consolidation cleans them up for free. Running this after the soft-reset would require a second reset cycle to re-cut the clean commits. + +1. **Skip-check.** If the user passed `--skip-review` in the slash-command args, announce the skip and proceed to Phase 2. Otherwise, run `command -v copilot >/dev/null` to verify the Copilot CLI is available. If it fails, log a warning and proceed to Phase 2 (never block the PR on Copilot CLI availability). Do NOT assume copilot is missing without running the check - it is installed at `~/.vscode-server/data/User/globalStorage/github.copilot-chat/copilotCli/copilot` in VS Code Server environments. +2. **Ensure the branch has reviewable history.** `/second-opinion` only sees committed state (`git diff ..HEAD`); any uncommitted work in the working tree is invisible to it. Two failure modes the preflight catches: + + - Branch at parity with trunk + dirty tree → empty review diff, "no findings" silently returned. + - Branch ahead of trunk + dirty tree → review sees the *committed* changes but **not** the dirty work added on top. + + In both cases the fix is the same: create one throwaway WIP commit so everything is committed. `extract_signals.py preflight-wip` is **self-applying** - it inspects the tree, and if a WIP commit is needed, it stages everything and commits with `--no-verify` on its own. The agent runs **one command, unconditionally, every time**: + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py preflight-wip + ``` + + Stdout will be either `created-wip` (a commit was made) or `skip` (tree was already clean). Never gate this call on your own assessment of the tree state - the script's whole purpose is to make the right call so the agent can't get it wrong. **Skipping this step silently degrades the review:** the prior failure mode that motivated this design was "agent saw a tidy two-commit branch, decided manually a WIP commit wasn't needed, ran `/second-opinion`, got 'No findings' on 8 hours of new work that wasn't in any commit." If you ever find yourself reasoning about whether to run preflight, the answer is: run it. + + The preflight script uses `git status --porcelain` (not `git diff --quiet HEAD`) so that **untracked** files trip the guard too - a brand-new file on a fresh branch is the most common shape of "I have work to review but no commits yet," and `git diff` is blind to untracked paths. The internal `git add -A` is the **only** legitimate one in the whole skill - the commit is throwaway. Phase 2's merge-base soft-reset dissolves it automatically while preserving the working tree, so review-driven fixes fold into the per-prefix commits like any other WIP. No explicit teardown needed. Pass `--dry-run` only when you want to inspect the decision without acting (tests, debugging) - never in the live skill flow. +3. **Invoke the skill.** Get the merge-base SHA (the same one Phase 2 will use) and pass it as the `base` to `/second-opinion` - the diff is exactly "what the branch adds since it diverged from the trunk": + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py phase2-base + ``` + + The printed SHA is the value to pass as `base=""` when invoking `/second-opinion`. Sharing the same source-of-truth subcommand for Phase 1.5's review base and Phase 2's reset target guarantees both phases see the same starting point. + +4. **Challenge every finding before acting.** The reviewer is a different model with no project context beyond `REVIEW-BRIEF.md`. It will flag intentional patterns, misread regex intent, and suggest "fixes" that break things. For each finding: + - Read the flagged code and understand the author's intent (check git blame, surrounding context, ARCHITECTURE.md, comments). + - If the finding is clearly wrong (flags a known pattern, misunderstands the regex/logic, contradicts a documented convention) - **dismiss it** and note why. + - If the finding is clearly right (genuine bug, obvious typo, provably broken logic) - fix it. + - If you cannot determine whether the finding is valid - **surface it to the user** via `AskUserQuestion` with the finding detail and your assessment of why it might or might not apply. Never auto-fix when uncertain. +5. **Present a summary** of all findings with your verdict for each: `fixed`, `dismissed (reason)`, or `needs-user-judgment`. The user should see what the reviewer flagged AND what you decided, not just the fixes. +6. **After fixes (if any).** Re-run `make lint` (lint stages modifications; Phase 2 will redo staging). Don't add commits for review fixes - they become part of the WIP state that Phase 2 consolidates. +7. **Verification rerun (if any fixes were applied).** Rerun `/second-opinion` scoped to only the files that were fixed - not the full branch diff. The fixed file list comes from your own context: you applied fixes via Edit in step 4, so you know exactly which files changed. Pass them as path arguments to `git diff`: + + ```bash + copilot -p "Review ONLY these files for remaining issues: . Run: git diff ..HEAD -- path/to/fixed1.py path/to/fixed2.py ..." ... + ``` + + This catches regressions from the fixes without re-reviewing the entire branch (which would repeat dismissed findings). Cap at 1 verification rerun. If the rerun finds new issues, fix them and proceed without a third pass. +8. **Proceed to Phase 1.5b.** The fixes become part of the WIP commits and get categorized into the right Conventional Commits prefix during Phase 2's consolidation. + +If `/second-opinion` reports "No findings.", announce it and proceed straight to Phase 1.5b. + +### Phase 1.5b: Extract learnings + +Mine the WIP commit history for operational lessons before Phase 2 destroys it. The bundled script does the deterministic signal detection; the agent generalizes and writes the prose. + +1. **Run the signal detector:** + + ```bash + uv run --frozen python /scripts/extract_signals.py signals + ``` + + Returns JSON with `signals` (array of `{type, items}`), `next_lesson_id`, `existing_entries`, and `lessons_exists`. Signal types: `repeated_edits` (files in 3+ WIP commits), `lint_fixes` (commit messages matching lint-fix patterns), `corrections` (commits starting with "fix:", "revert:", etc.). + +2. **Add review findings.** If Phase 1.5 fixed any `/second-opinion` findings (not dismissed), treat each as an additional signal. The script does not detect these - the agent knows them from Phase 1.5 context. + +3. **No signals? Skip silently.** If the JSON `signals` array is empty and no review findings were fixed, proceed to Phase 1.5c. Do not create `design/lessons.md` or announce anything. + +4. **Filter: only keep signals that could recur.** For each signal, ask: "Could this same mistake happen again in a different file, a different skill, or a different PR?" Drop signals where the fix is **self-enforcing** - infrastructure changes (Makefile targets, CI config, hook config, linter rules) that mechanically prevent the mistake from recurring. Those fixes live in the code; a lesson is redundant. Keep signals where the pattern could repeat in new code the agent writes in the future (e.g., "always redact env values in probe scripts" - the fix in one script doesn't prevent the same mistake in the next script). + +5. **Draft candidate entries.** For each surviving signal, draft a generalized rule: + + ```markdown + ## L-NNN - YYYY-MM-DD - short-tag + + **Source:** PR #TBD, branch `` + + + + --- + ``` + + - **Generalize.** The entry teaches a forward-looking rule, not a war story. + - **Cap at 3 entries per PR.** If more signals fire, pick the 3 with broadest applicability. + - **L-NNN numbering.** Use `next_lesson_id` from the script output. + - **Deduplicate.** Compare against `existing_entries` from the script. Drop duplicates. + +6. **Bootstrap or append.** If `lessons_exists` is false: + + ```bash + mkdir -p design + ``` + + Create `design/lessons.md` with this header: + + ```markdown + # Operational Lessons + + Generalized rules extracted from PR history by `/prepare-pr`. Each entry captures a pattern the agent learned the hard way and the rule that prevents recurrence. Reviewed in the PR diff before merge. + + --- + ``` + + Append candidate entries. If the file exists, append after the last entry. + +7. **Proceed to Phase 1.5c.** The `design/lessons.md` changes are uncommitted working-tree state; Phase 2 classifies them under `docs` (or `chore` if no other docs changes exist). + +### Phase 1.5c: Review ARCHITECTURE.md + +The bundled script detects which architecture-doc sections may be stale based on the branch diff. **This step is config-driven and repo-agnostic:** the section→path map lives in `.claude/prepare-pr.toml` at the repo root (see "Config" below), not in the script. A repo with no config gets a clean no-op here - `stale_sections` is always empty - so the skill degrades gracefully when ported. + +1. **Run the staleness checker:** + + ```bash + uv run --frozen python /scripts/extract_signals.py architecture + ``` + + Returns JSON with the same keys on every path: `stale_sections` (the config's section names whose path prefixes were touched), `architecture_exists`, `architecture_doc` (the doc name from config, default `ARCHITECTURE.md`), `config_present` (whether `.claude/prepare-pr.toml` was found), and `changed_files_count`. With no config, `stale_sections` is `[]` and `config_present` is `false`. + +2. **No stale sections? Skip silently.** If `stale_sections` is empty (including the no-config case), proceed to Phase 2. + +3. **Update stale sections.** Read the architecture doc and update only the flagged sections. The section names are whatever the repo's `.claude/prepare-pr.toml` defines; find the matching heading in the doc and refresh it. For **this repo** the map is: + - `host_guest_split` → § 1 (Host vs guest split) table if a script area appeared/moved under `wsl/`, `.assets/scripts/`, `.assets/provision/`, or `.assets/trigger/` (a new execution context or invocation path). + - `hook_inventory` → § 3 (Pre-commit hook inventory) table if `.pre-commit-config.yaml` or a hook under `tests/hooks/` changed (add/remove a row, refresh the "what it checks / why"). + - `test_layout` → § 4 (Test layout) tree if the `tests/` structure changed (new subdir, new hook module, changed `make test-*` wiring). + - `powershell_modules` → § 6 (PowerShell modules) if `modules/` gained/lost/renamed a module or `.assets/scripts/modules_update.ps1` changed - refresh the synced-vs-local table (§ 6.1), the sync list, and the install-sites table (§ 6.2). A synced-module rename (e.g. `do-linux` → `do-unix`) also touches every install site listed there. + +4. **The changes join the `docs` commit in Phase 2.** + +#### Config (`.claude/prepare-pr.toml`) + +The staleness map and the doc/lessons paths are **per-repo config**, not baked into the bundled script - that's what keeps the script portable enough to live in `~/.claude/skills/` and be shared across repos. The script reads `.claude/prepare-pr.toml` from the git repo root; when it's absent, malformed, or `tomllib` is unavailable (Python < 3.11), every consumer falls back to a safe default (the `architecture` check becomes a no-op, `lessons_path` defaults to `design/lessons.md`). To adopt the skill in a new repo, drop a file like: + +```toml +architecture_doc = "ARCHITECTURE.md" # doc scanned for staleness (default) +lessons_path = "design/lessons.md" # where Phase 1.5b writes lessons (default) + +[architecture_sections] # section name -> path prefixes that mark it stale +powershell_module = ["modules/"] +build_pipeline = ["pyproject.toml", "Makefile"] +# ... one entry per architecture-doc section you want staleness-checked +``` + +Omit `[architecture_sections]` entirely to disable the staleness check (Phase 1.5c becomes a silent no-op) - appropriate for repos with no architecture doc. + +### Phase 2: Soft-reset and recommit by (prefix, concern) + +Skip entirely if Phase 1 step 8 concluded the branch is already clean. + +1. **Clear the index** so we start from a known state: + + ```bash + git restore --staged . + ``` + +2. **Soft-reset to the merge-base with trunk** so all branch changes become unstaged but the working tree is preserved. Critically, target the **merge-base**, not the trunk tip - if trunk has advanced since the branch was cut and you reset to the tip, the next commits would diff against the new tip and look like they're reverting the unrelated trunk changes that happened in between: + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py phase2-base + # Substitute the printed SHA as a literal in the next command: + git reset --soft + git restore --staged . + ``` + +3. **For each group from the plan**, stage explicitly and commit with `--no-verify` (we'll validate via `make lint-diff` once in Phase 3, instead of paying the per-commit hook cost N times): + + ```bash + git add # explicit list, never `git add .` or `-A` + git commit --no-verify -m "(scope): " + ``` + + Order doesn't matter for the diff but does affect reviewer cognitive load. Suggested order: `feat` → `fix` → `refactor` → `test` → `chore` → `docs`. Foundations first, ergonomics last. + +4. **Commit message style** - terse, single-line, Conventional Commits. Match the project's existing style. To inspect prior commit titles for examples, run `extract_signals.py trunk-ref`, then `git log --oneline | head -20` (literal substitution, not shell variable). The body is optional and only adds value when the *why* is non-obvious from the file changes - skip the body for routine work. + + Examples from this repo: + - `feat(docs): publish tls-certificates-kb vault to mkdocs site` + - `docs(networking/icp): align with official ICP documentation` + - `fix(hooks): handle hidden directories in cspell ignorePaths` + - `chore(hooks): patch align_tables.py to respect '|' inside wikilinks` + +5. **Never use `git add .` or `git add -A`** in this phase. The active scope is what *you* want in each commit, not whatever happens to be in the working tree. Always explicit file lists. + +### Phase 3: Final lint + +Per-commit hooks were skipped via `--no-verify`; this is where we validate the consolidated state. + +```bash +make lint-diff +``` + +Use `make lint-diff` (not `make lint`) - post-commit there are no uncommitted changes for `make lint` to operate on; `make lint-diff` runs hooks against the diff to trunk, which is what we actually want to validate. Per-repo note: the Makefile target itself may hard-code its trunk ref (this repo's does, against `main`) - if you port this skill to a repo where the Makefile uses a different trunk name, the target needs updating too, separately from this skill. + +- **Passes + working tree clean** → done; proceed to Phase 4. +- **Passes + files modified by auto-fix hooks** (whitespace, table alignment, EOF, formatter side-effects) → amend the relevant commit: + + ```bash + git commit --amend --no-verify --no-edit -a + ``` + + Acceptable noise - the auto-fix output is uniform across hooks and doesn't change meaning. If the auto-fix touched files that conceptually belong to a different prefix than the last commit, split the fixup into a fresh `chore: lint fixups` commit instead of amending. +- **Fails non-recoverably** → stop. Surface the error. Do not push. The user fixes manually, then re-runs Phase 3 + 4. + +If `mkdocs build --strict` is part of the project's quality gate (it is, for vault changes), run `uv run mkdocs build --strict` as well and surface any warnings - strict-mode warnings fail the build. + +### Phase 4: Push and PR + +1. **Push.** If the branch has no upstream yet: + + ```bash + git push -u origin + ``` + + Otherwise (history was rewritten): + + ```bash + git push --force-with-lease + ``` + + `--force-with-lease` (not bare `--force`) refuses to clobber remote commits you haven't seen - safety net for the "someone else pushed in the last 5 minutes" case. + +2. **Create or update PR.** Probe first: + + ```bash + gh pr view --json number,title,body 2>/dev/null + ``` + + - **No PR** → create. Get the trunk name as a literal (no shell variables): + + ```bash + uv run --frozen python .claude/skills/prepare-pr/scripts/extract_signals.py trunk-name + # Substitute the printed name as a literal in the next command: + gh pr create --base \ + --title "(scope): " \ + --body "$(cat <<'EOF' + ## Summary + + - + - <...> + + ## Test plan + + - [x] `make lint` - all 12 pre-commit hooks green + - [x] `uv run mkdocs build --strict` - zero warnings (if docs touched) + - [ ] + + 🤖 Generated with [Claude Code](https://claude.com/claude-code) + EOF + )" + ``` + + - **PR exists** → update title + body only: + + ```bash + gh pr edit --title "..." --body "..." + ``` + + Never close/reopen, never re-request review, never touch labels or milestone unless the user explicitly asks. + + - **Backfill PR number in lessons.** If `design/lessons.md` was created or modified in this run, replace the placeholder: `sed -i "s/PR #TBD/PR #/g" design/lessons.md`, then amend the relevant commit: `git commit --amend --no-verify --no-edit design/lessons.md`. + +3. **Title style** - mirror the most significant commit. If the branch is multi-purpose, lead with the most user-visible scope. Keep under ~70 chars (the GitHub list view truncates). + +4. **Body style** - this repo's PR template is two sections: + - **`## Summary`** - short bullets, one per logical change. Lean on the consolidated commit messages; this is not the place to repeat them verbatim, but rather to surface the *why* a reviewer needs to know. + - **`## Test plan`** - checklist. Use `[x]` for what you actually ran (`make lint`, `mkdocs build --strict`), `[ ]` for reviewer-facing manual checks (browse the rendered site, click links, etc.). If the change is hooks/tooling, the reviewer checks usually run the hook against a contrived input. + +5. **Append the Claude Code attribution trailer** on the last line of the body (the convention used by other PRs in this repo). Do not put it inside a section. + +### Phase 4.5: Address PR review comments (optional) + +After Phase 4 pushes and creates/updates the PR, drive the GitHub Copilot **server-side PR review** to a clean state. This is **independent of the Copilot CLI** used in Phase 1.5 - it uses `pr_review.py` (not raw `gh` commands) to interact with GitHub's review API. + +**Entry point:** always use `pr_review.py` subcommands - never run `gh pr edit --add-reviewer` or raw `gh api graphql` mutations as one-offs. `pr_review.py` centralizes the trigger/wait/resolve logic so this phase's state machine has a single tested code path; ad-hoc `gh` calls drift from the script's idempotency, login-alias, and JSON-output guarantees. The correct sequence is: + +```bash +uv run --frozen python .claude/skills/address-pr-review/scripts/pr_review.py state # detect review state (A/B/C/D) +uv run --frozen python .claude/skills/address-pr-review/scripts/pr_review.py trigger # request Copilot review +uv run --frozen python .claude/skills/address-pr-review/scripts/pr_review.py wait --timeout 480 # poll until complete +uv run --frozen python .claude/skills/address-pr-review/scripts/pr_review.py resolve # resolve a thread +``` + +1. **Skip-check.** If the user passed `--skip-review`, announce the skip and stop. (Same flag skips Phase 1.5.) This is the **only** skip condition - unlike Phase 1.5, Phase 4.5 does not depend on the `copilot` CLI binary being on PATH. **Never skip because a raw `gh` command failed** - use `pr_review.py` which handles the API correctly. +2. **Iteration loop (cap at 2):** + - Use `pr_review.py state` to detect current state, then `trigger` + `wait` to get the review, then process unresolved fresh threads per the `/address-pr-review` skill (run only address-pr-review's Phase 1-3 - skip its Phase 4 commit; commits are handled by the prepare-pr Phase 2 re-cut below). Drive from any state (A/B/C/D) to either State D (clean - done) or "fixes applied, ready for re-cut." + - **If no fixes were applied** (State D reached without code edits, or all unresolved threads were `resolve-only`/`skip`-leave-open) → done. Exit. + - **If fixes were applied** → re-run Phase 2 (soft-reset + recommit, NO Phase 1.5) → re-run Phase 3 (lint-diff) → re-run Phase 4 (force-push + update PR body). The new push triggers a fresh Copilot review automatically. +3. **After 2 iterations:** if fixes are still being made, surface to user: "2 fix cycles complete. Run `/address-pr-review` manually if more comments arrive." Stop. + +Non-blocking: if `/address-pr-review` reports a timeout (Copilot didn't respond within 5 min) or fails non-recoverably, log the warning and continue. The PR is pushed; the user can address comments manually. + +The state-aware skill is the brain - Phase 4.5 just orchestrates the iteration cap and the Phase 2 re-cut. All freshness detection, polling, and resolution logic lives in `pr_review.py` where it can be tested independently. + +## Anti-patterns + +- **`git push --force`** without `--with-lease` - clobbers remote commits you haven't seen. +- **Force-pushing to the trunk branch or any well-known trunk name (`main`, `master`, `develop`, `trunk`, `prod`, `release/*`)** - Phase 1 step 2 guardrail must refuse. The literal list is a safety net for repos where trunk resolution somehow returned the wrong name. +- **Hard-coding `main` in any new `git`/`gh` invocation** - this skill is a gallery item, copied across repos that use `main`, `master`, `prod`, etc. Resolve trunk via `extract_signals.py trunk-name` / `trunk-ref` and substitute the printed value as a literal argument in the next command. Hard-coding `main` works here and silently breaks the next repo. +- **Using `eval`, `$(...)` substitution, or shell variables to plumb trunk values between commands.** The agent's `Bash` tool runs every command in a fresh shell, so any `eval`'d variable is gone by the next call. Worse: compound commands with `$(...)` or `eval` are flagged by the permission matcher as "shell syntax that cannot be statically analyzed" and trigger an approval prompt every time. Always run the script command first, **read the printed value from the output**, then issue the next command with that value as a literal argument. Plain `git`/`gh` commands without substitution match cleanly against the allow-list and run silently. +- **Resetting to the trunk *tip* (`git reset --soft `) in Phase 2 instead of the merge-base** (`git reset --soft `, obtained from `extract_signals.py phase2-base`). Tip-reset is only safe when nothing landed on trunk between branch-cut and now. If trunk advanced, the post-reset commits diff against the new tip and look like they're reverting the unrelated trunk changes that happened in between. The `phase2-base` helper always returns the merge-base; use it every time. +- **Skipping Phase 1 step 1 (`sync-trunk`).** A stale local trunk poisons every merge-base, diff, and `make lint-diff --from-ref main` downstream - the branch looks like it contains commits that are already merged, and the Phase 2 soft-reset can target a base far behind the real merge-base. `sync-trunk` fast-forwards the local trunk from origin before anything reads it. It's not optional and it's not the same as `phase2-base` preferring `origin/`: the `origin/` preference fixes the skill's *own* merge-base calls, but the repo's Makefile reads the *local* ref, so both the fetch/ff and the origin-preference are needed. On `diverged` (exit 1), stop - don't rewrite history on top of an out-of-sync trunk. +- **`git add .` or `git add -A`** in Phase 2 - the active scope is what *you* want in each commit, not whatever happens to be in working tree. Always explicit file lists. +- **Skipping `make lint-diff` after Phase 2.** Per-commit hooks were bypassed via `--no-verify`; `make lint-diff` is the validation gate. Skipping it means pushing unvalidated state. +- **Using `make lint` instead of `make lint-diff` after Phase 2.** Post-commit there are no uncommitted changes for `make lint` to operate on. `make lint-diff` runs hooks against the trunk-to-HEAD diff, which is the actual scope to validate. +- **Auto-running consolidation on a clean branch.** Adds noise (force-push for no benefit, lost commit timestamps) when the branch was already in PR shape. Phase 1 step 8 is the gate - trust it. +- **Squashing without explanation.** If you collapse 5 commits with distinct semantics into one `chore: misc updates`, the reviewer (and `git blame` six months later) can't tell what changed why. The whole point of the per-prefix consolidation is that each commit has a *clear scope*. +- **Stuffing tooling changes into a `docs(...)` commit.** Reviewers triage by scope; mixing scopes makes it harder to revert one change without the other. +- **Adding the Claude Code attribution trailer to commit messages, not just the PR body.** This repo's convention is the trailer goes in the PR body and (per the global Bash safety doc) in commits authored by Claude. Be consistent with the project's existing commits - check `git log` recent samples. +- **Re-requesting review on update.** Updating the PR body or pushing a new revision shouldn't ping the reviewer again. `gh pr edit` for body, plain `git push --force-with-lease` for code, nothing else. +- **Running `/second-opinion` after Phase 2 (soft-reset).** The soft-reset is the point of no return for commit topology; review-driven fixes after that need a second reset cycle. Phase 1.5 fires *before* Phase 2 by design - fixes land in WIP commits and Phase 2 absorbs them for free. +- **Invoking `/second-opinion` with no commits ahead of trunk.** `/second-opinion` reviews `git diff ..HEAD`. If the branch is at parity with the trunk and all work is uncommitted, the diff is empty, the reviewer reports "no findings," and the agent (and the user) thinks the code passed review when it was never actually reviewed. Phase 1.5 step 2's pre-flight WIP-commit + Phase 2's merge-base soft-reset is the workaround - never invoke `/second-opinion` on an empty `..HEAD` diff and treat success as meaningful. +- **Skipping Phase 1.5 step 2 (preflight-wip) when the branch already has committed changes ahead of trunk.** The preflight applies in BOTH "no commits, dirty tree" AND "some commits + dirty tree" cases - the script's whole purpose is to make the right call so the agent can't reason its way past it. The failure I saw firsthand: branch had 2 committed changes + 8 uncommitted files; agent skipped preflight ("there's already a diff for Copilot to see"); `/second-opinion` reviewed only the 2 old commits and returned "No findings," missing every line of the 8 uncommitted files. The script is self-applying for exactly this reason - always run `extract_signals.py preflight-wip` unconditionally in Phase 1.5, every time, no judgment call. +- **Skipping Phase 4.5 because `copilot` CLI is missing.** Phase 4.5 uses the GitHub server-side Copilot reviewer via `gh` CLI - it does NOT use the `copilot` CLI binary. Only `--skip-review` should skip it. Phase 1.5 is the one that needs the `copilot` CLI. +- **Assuming `copilot` CLI is not installed without running `command -v copilot`.** In VS Code Server / WSL environments, copilot lives at `~/.vscode-server/.../copilotCli/copilot` and is on PATH. Always run the check. +- **Auto-applying `/second-opinion` findings without challenge.** The reviewer is a different model with limited project context. It will misread intentional regex patterns, flag documented conventions, and suggest "fixes" that break scoping or logic. Every finding must be validated against the author's intent before acting. When uncertain, surface to the user - never auto-fix on faith. +- **Skipping Phase 1.5 or 4.5 based on your own judgment** ("it's just a new file", "small change", "no existing code modified"). The ONLY skip condition is `--skip-review`. New features are the MOST important case for review - fresh code has no prior review coverage and no test history. The `/second-opinion` run on `bootstrap-docs` caught two real bugs in brand-new code. Never invent skip reasons that aren't in the skill. +- **Writing session-specific notes in `design/lessons.md` instead of generalized rules.** Each entry is a forward-looking rule, not a journal entry. "When adding a new vault, create `.nav.yml` before content files" is a rule. "I forgot `.nav.yml` for tls-certificates-kb" is a journal entry. +- **Extracting more than 3 learnings per PR.** A noisy ledger gets ignored. Pick the 3 with broadest applicability. If you genuinely have 5+ signals, the branch was too large - note that as one of the 3. +- **Skipping Phase 1.5c (ARCHITECTURE.md review).** New skills, hooks, and vault structure changes are the most common sources of staleness. The review takes seconds; the staleness costs hours when a future agent acts on outdated information. + +## Example invocations + +- `/prepare-pr` - full pipeline against the current branch +- `/prepare-pr --skip-review` - skip Phase 1.5 (`/second-opinion`) and Phase 4.5 (`/address-pr-review`); use when Copilot is offline or you've already run reviews manually +- "wrap this branch up as a PR" - same workflow +- "update the PR with the new commits" - re-run; Phase 1 detects existing PR, Phase 4 takes the update path +- "the branch is already clean, just lint + push + PR" - skill detects this in Phase 1 step 8 and skips Phase 2 automatically; the user's framing just confirms the heuristic diff --git a/.claude/skills/prepare-pr/scripts/extract_signals.py b/.claude/skills/prepare-pr/scripts/extract_signals.py new file mode 100755 index 00000000..a6333458 --- /dev/null +++ b/.claude/skills/prepare-pr/scripts/extract_signals.py @@ -0,0 +1,796 @@ +#!/usr/bin/env -S uv run --frozen python +""" +Bundled helpers for /prepare-pr. + +Subcommands: sync-trunk, trunk, trunk-name, trunk-ref, branch-safety, +preflight-wip, phase2-base, signals, architecture. +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from pathlib import Path + +try: + import tomllib +except ModuleNotFoundError: # Python < 3.11 - config support degrades to no-op + tomllib = None # type: ignore[assignment] + +WELL_KNOWN_TRUNK_NAMES = ("main", "master", "trunk", "prod") +WELL_KNOWN_PROTECTED_BRANCHES = ("main", "master", "develop", "trunk", "prod") +PROTECTED_BRANCH_PATTERNS = (re.compile(r"^release/"),) + +LINT_FIX_RE = re.compile( + r"(?:fix|fixup|fix up)\s*(?:lint|hook|linter|format|whitespace|trailing)", + re.IGNORECASE, +) +CORRECTION_RE = re.compile( + r"^(?:fix:|revert:|actually|no,|oops|wrong)", + re.IGNORECASE, +) + +# Per-repo config keeps this script portable: repo-specific layout (the +# ARCHITECTURE staleness map, doc/lessons paths) lives in `.claude/prepare-pr.toml` +# at the repo root, NOT baked into this file. Absent config -> graceful no-op +# (the `architecture` staleness check simply finds nothing). See SKILL.md and the +# `[architecture_sections]` example in the repo's own `.claude/prepare-pr.toml`. +CONFIG_RELPATH = ".claude/prepare-pr.toml" +DEFAULT_LESSONS_PATH = "design/lessons.md" +DEFAULT_ARCHITECTURE_DOC = "ARCHITECTURE.md" + + +def _run(cmd: list[str], **kwargs: object) -> str: + try: + result = subprocess.run(cmd, capture_output=True, text=True, **kwargs) + except FileNotFoundError: + # Executable not on PATH - treat as soft failure so callers can fall + # back through the resolution chain (e.g., trunk detection probes + # well-known refs when `gh` is unavailable). + return "" + if result.returncode != 0: + return "" + return result.stdout.strip() + + +def _repo_root() -> Path: + """Return the git worktree root, or cwd if `git` can't resolve it.""" + top = _run(["git", "rev-parse", "--show-toplevel"]) + return Path(top) if top else Path.cwd() + + +def _load_config() -> dict[str, object]: + """ + Load `.claude/prepare-pr.toml` from the repo root, if present. + + Returns {} when the file is absent, unreadable, malformed, or when tomllib is + unavailable (Python < 3.11). Every consumer must treat {} as "no repo-specific + config" and fall back to defaults - this is what keeps the script portable to + repos that ship no config at all. + """ + if tomllib is None: + return {} + cfg_path = _repo_root() / CONFIG_RELPATH + if not cfg_path.is_file(): + return {} + try: + with cfg_path.open("rb") as fh: + return tomllib.load(fh) + except (OSError, tomllib.TOMLDecodeError): + return {} + + +def _config_architecture_sections(cfg: dict[str, object]) -> dict[str, list[str]]: + """ + Extract the {section_name: [path_prefix, ...]} map from config. + + Accepts only string keys mapping to lists of strings; silently drops malformed + entries so a partly-broken config still yields the usable sections rather than + erroring the whole run. + """ + raw = cfg.get("architecture_sections") + if not isinstance(raw, dict): + return {} + sections: dict[str, list[str]] = {} + for name, prefixes in raw.items(): + if isinstance(name, str) and isinstance(prefixes, list): + clean = [p for p in prefixes if isinstance(p, str) and p] + if clean: + sections[name] = clean + return sections + + +def _commits_since(base: str) -> list[dict[str, str]]: + log = _run(["git", "log", "--format=%H|%s", f"{base}..HEAD"]) + if not log: + return [] + commits = [] + for line in log.splitlines(): + if "|" not in line: + continue + sha, subject = line.split("|", 1) + commits.append({"sha": sha.strip(), "subject": subject.strip()}) + return commits + + +def _files_in_commit(sha: str) -> list[str]: + out = _run(["git", "diff-tree", "--no-commit-id", "--name-only", "-r", sha]) + return [f for f in out.splitlines() if f] if out else [] + + +def _changed_files_since(base: str) -> list[str]: + out = _run(["git", "diff", "--name-only", f"{base}..HEAD"]) + return [f for f in out.splitlines() if f] if out else [] + + +def _next_lesson_id(lessons_path: Path) -> int: + if not lessons_path.exists(): + return 1 + text = lessons_path.read_text(encoding="utf-8", errors="replace") + ids = [int(m.group(1)) for m in re.finditer(r"^## L-(\d{3})", text, re.MULTILINE)] + return (max(ids) + 1) if ids else 1 + + +def _existing_lessons(lessons_path: Path) -> list[str]: + if not lessons_path.exists(): + return [] + text = lessons_path.read_text(encoding="utf-8", errors="replace") + return re.findall(r"^## L-\d{3} - .+$", text, re.MULTILINE) + + +def _emit(payload: dict[str, object], code: int = 0) -> int: + """Emit JSON payload to stdout and return exit code.""" + json.dump(payload, sys.stdout, indent=2) + print() + return code + + +def _resolve_trunk_name() -> str: + """Resolve trunk name via the SKILL.md fallback chain.""" + ref = _run(["git", "symbolic-ref", "--short", "refs/remotes/origin/HEAD"]) + if ref: + return ref.removeprefix("origin/") + gh_default = _run( + [ + "gh", + "repo", + "view", + "--json", + "defaultBranchRef", + "-q", + ".defaultBranchRef.name", + ] + ) + if gh_default: + return gh_default + for name in WELL_KNOWN_TRUNK_NAMES: + if _ref_exists(f"refs/heads/{name}") or _ref_exists( + f"refs/remotes/origin/{name}" + ): + return name + return "" + + +def _ref_exists(ref: str) -> bool: + """Return True if ref resolves under `git show-ref --verify`.""" + try: + result = subprocess.run( + ["git", "show-ref", "--verify", "--quiet", ref], capture_output=True + ) + except FileNotFoundError: + return False + return result.returncode == 0 + + +def _run_ok(cmd: list[str]) -> bool: + """Run a command, returning True only on a zero exit (git binary present).""" + try: + result = subprocess.run(cmd, capture_output=True, text=True) + except FileNotFoundError: + return False + return result.returncode == 0 + + +def _is_ancestor(ancestor: str, descendant: str) -> bool: + """Return True if `ancestor` is an ancestor of `descendant`.""" + return _run_ok(["git", "merge-base", "--is-ancestor", ancestor, descendant]) + + +def _resolve_trunk_ref(name: str) -> str: + """ + Pick a usable trunk ref, preferring origin/ over the local branch. + + origin/ is authoritative - it's what the PR/merge is computed against, + and it stays current on fetch. A local trunk branch can lag behind (nobody + runs `git checkout main && git pull` on a feature-branch workflow), and using + a stale local ref makes every merge-base (phase2-base, architecture, signals) + resolve too far back, pulling already-merged commits into the diff/reset. + Fall back to the local branch only when there's no remote-tracking ref. + """ + if _ref_exists(f"refs/remotes/origin/{name}"): + return f"origin/{name}" + if _ref_exists(f"refs/heads/{name}"): + return name + return "" + + +def cmd_sync_trunk(args: argparse.Namespace) -> int: + """ + Fetch origin and fast-forward the local trunk branch if it is behind. + + Runs once at the very start of Phase 1, before any merge-base/diff/lint uses + the trunk. Fetching de-stales origin/; fast-forwarding the local branch + keeps consumers that read the LOCAL ref correct too (notably this repo's + `make lint-diff --from-ref main`). The working tree is untouched - trunk is + never checked out; the local ref is moved with `git update-ref`. + + Outcomes (JSON `action`): + up-to-date - local trunk already equals origin + fast-forwarded- local trunk moved forward to origin + diverged - local trunk has commits not on origin -> ABORT (exit 1) + no-local - no local trunk branch; origin/ is used directly + no-origin - no remote-tracking ref (nothing to sync against) + offline - fetch failed (no network/remote) -> warn, continue + """ + name = args.trunk_name or _resolve_trunk_name() + if not name: + return _emit( + { + "synced": False, + "action": "error", + "reason": ( + "could not resolve trunk name; run: git remote set-head origin -a" + ), + }, + code=1, + ) + + local_ref = f"refs/heads/{name}" + origin_ref = f"refs/remotes/origin/{name}" + has_local = _ref_exists(local_ref) + + # Fetch so origin/ is current. Non-fatal: offline dev must still work. + fetched = _run_ok(["git", "fetch", "origin", name]) + if not fetched: + return _emit( + { + "synced": False, + "action": "offline", + "name": name, + "reason": ( + f"git fetch origin {name} failed; proceeding with existing refs" + ), + } + ) + + if not _ref_exists(origin_ref): + return _emit({"synced": False, "action": "no-origin", "name": name}) + + origin_sha = _run(["git", "rev-parse", origin_ref]) + if not has_local: + return _emit( + {"synced": False, "action": "no-local", "name": name, "origin": origin_sha} + ) + + local_sha = _run(["git", "rev-parse", local_ref]) + if local_sha == origin_sha: + return _emit( + {"synced": True, "action": "up-to-date", "name": name, "local": local_sha} + ) + + # Behind: local is a strict ancestor of origin -> safe fast-forward. + if _is_ancestor(local_sha, origin_ref): + # Never fast-forward a checked-out branch; on a feature branch this is + # safe, but guard anyway so we don't desync the index/working tree. + current = _run(["git", "branch", "--show-current"]) + if current == name: + return _emit( + { + "synced": False, + "action": "on-trunk", + "name": name, + "reason": ( + f"currently on '{name}'; run `git pull --ff-only` yourself" + ), + }, + code=1, + ) + if not _run_ok(["git", "update-ref", local_ref, origin_sha]): + return _emit( + { + "synced": False, + "action": "error", + "name": name, + "reason": ( + f"git update-ref {local_ref} failed; local trunk left unchanged" + ), + }, + code=1, + ) + return _emit( + { + "synced": True, + "action": "fast-forwarded", + "name": name, + "local_before": local_sha, + "local_after": origin_sha, + } + ) + + # Diverged: local has commits not on origin. Abort per design. + return _emit( + { + "synced": False, + "action": "diverged", + "name": name, + "local": local_sha, + "origin": origin_sha, + "reason": ( + f"local '{name}' has commits not on origin/{name}; " + f"resolve manually (e.g. git checkout {name} && git pull --ff-only) " + "before rewriting history" + ), + }, + code=1, + ) + + +def cmd_trunk(_args: argparse.Namespace) -> int: + """Resolve the trunk branch into name + usable git ref (diagnostic JSON).""" + name = _resolve_trunk_name() + if not name: + return _emit( + { + "error": "Could not resolve trunk; run: git remote set-head origin -a", + }, + code=1, + ) + ref = _resolve_trunk_ref(name) + if not ref: + return _emit( + { + "name": name, + "error": ( + f"Trunk '{name}' has no local or remote ref; " + f"run: git fetch origin {name}" + ), + }, + code=1, + ) + return _emit({"name": name, "ref": ref}) + + +def cmd_trunk_name(_args: argparse.Namespace) -> int: + """Print just the trunk branch name (plain text, for gh pr create --base).""" + name = _resolve_trunk_name() + if not name: + print( + "Could not resolve trunk; run: git remote set-head origin -a", + file=sys.stderr, + ) + return 1 + print(name) + return 0 + + +def cmd_trunk_ref(_args: argparse.Namespace) -> int: + """Print just the trunk git ref (plain text, for git log/diff/merge-base/reset).""" + name = _resolve_trunk_name() + if not name: + print( + "Could not resolve trunk; run: git remote set-head origin -a", + file=sys.stderr, + ) + return 1 + ref = _resolve_trunk_ref(name) + if not ref: + print( + f"Trunk '{name}' has no local or remote ref; run: git fetch origin {name}", + file=sys.stderr, + ) + return 1 + print(ref) + return 0 + + +def cmd_branch_safety(args: argparse.Namespace) -> int: + """Refuse if current branch matches trunk or any protected name.""" + current = _run(["git", "branch", "--show-current"]) + if not current: + return _emit({"safe": False, "current": "", "reason": "detached HEAD"}, code=1) + trunk_name = args.trunk_name or _resolve_trunk_name() + if not trunk_name: + return _emit( + { + "safe": False, + "current": current, + "reason": ( + "could not resolve trunk name - fail-closed to prevent " + "destructive rewrite; pass --trunk-name or run: " + "git remote set-head origin -a" + ), + }, + code=1, + ) + if current == trunk_name: + return _emit( + { + "safe": False, + "current": current, + "trunk_name": trunk_name, + "reason": "current branch is the trunk", + }, + code=1, + ) + if current in WELL_KNOWN_PROTECTED_BRANCHES: + return _emit( + { + "safe": False, + "current": current, + "trunk_name": trunk_name, + "reason": "current branch is a well-known protected name", + }, + code=1, + ) + if any(p.match(current) for p in PROTECTED_BRANCH_PATTERNS): + return _emit( + { + "safe": False, + "current": current, + "trunk_name": trunk_name, + "reason": "current branch matches a release/* pattern", + }, + code=1, + ) + return _emit({"safe": True, "current": current, "trunk_name": trunk_name}) + + +def cmd_preflight_wip(args: argparse.Namespace) -> int: + """ + Self-applying WIP guard: print 'created-wip' or 'skip' and act accordingly. + + By default (no flag), this subcommand BOTH decides AND acts: if a WIP commit + is needed, it runs `git add -A && git commit --no-verify -m "WIP for review"` + itself, then prints 'created-wip' on stdout. If the tree is already clean or + the branch is at parity with trunk, it prints 'skip'. The agent's job is one + line: run the command. No stdout parsing, no conditional follow-up commands - + that's the failure mode this design eliminates. + + `--dry-run` restores the old behavior: print 'create-wip' or 'skip' but do + not commit. Useful for tests and for callers that want to inspect first. + """ + trunk_ref = args.trunk_ref or _resolve_trunk_ref(_resolve_trunk_name()) + if not trunk_ref: + print( + "could not resolve trunk_ref - pass --trunk-ref or run: " + "git remote set-head origin -a", + file=sys.stderr, + ) + return 1 + base = _run(["git", "merge-base", trunk_ref, "HEAD"]) + head = _run(["git", "rev-parse", "HEAD"]) + porcelain = _run(["git", "status", "--porcelain", "--untracked-files=all"]) + if not base or not head: + print("git merge-base or rev-parse failed", file=sys.stderr) + return 1 + dirty = bool(porcelain) + at_parity = base == head + if not dirty and at_parity: + print("skip") + print( + "branch clean and at parity with trunk - nothing to review", + file=sys.stderr, + ) + return 0 + if not dirty: + print("skip") + print( + "working tree clean - existing commits ARE the review scope", + file=sys.stderr, + ) + return 0 + reason = ( + "branch at parity with trunk AND working tree dirty" + if at_parity + else "branch has commits ahead of trunk AND working tree dirty - " + "WIP needed so /second-opinion sees committed + uncommitted together" + ) + if args.dry_run: + print("create-wip") + print(reason, file=sys.stderr) + return 0 + add_rc = subprocess.run( + ["git", "add", "-A"], capture_output=True, text=True, check=False + ) + if add_rc.returncode != 0: + print(f"git add -A failed: {add_rc.stderr.strip()}", file=sys.stderr) + return 1 + commit_rc = subprocess.run( + ["git", "commit", "--no-verify", "-m", "WIP for review"], + capture_output=True, + text=True, + check=False, + ) + if commit_rc.returncode != 0: + err = commit_rc.stderr.strip() or commit_rc.stdout.strip() + print(f"git commit failed: {err}", file=sys.stderr) + return 1 + print("created-wip") + print(f"{reason} - WIP commit created", file=sys.stderr) + return 0 + + +def cmd_phase2_base(args: argparse.Namespace) -> int: + """Print the merge-base SHA to soft-reset to in Phase 2 (plain text on stdout).""" + trunk_ref = args.trunk_ref or _resolve_trunk_ref(_resolve_trunk_name()) + if not trunk_ref: + print("could not resolve trunk_ref", file=sys.stderr) + return 1 + sha = _run(["git", "merge-base", trunk_ref, "HEAD"]) + if not sha: + print(f"git merge-base {trunk_ref} HEAD returned nothing", file=sys.stderr) + return 1 + print(sha) + return 0 + + +def cmd_signals(args: argparse.Namespace) -> int: + """Extract learning signals from WIP history.""" + if args.base: + base = args.base + else: + trunk_ref = _resolve_trunk_ref(_resolve_trunk_name()) + if not trunk_ref: + return _emit( + { + "error": ( + "Could not resolve trunk; pass --base or run: " + "git remote set-head origin -a" + ) + }, + code=1, + ) + base = _run(["git", "merge-base", trunk_ref, "HEAD"]) + if not base: + return _emit({"error": "Could not determine merge-base"}, code=1) + + if args.lessons: + # resolve a relative --lessons against the repo root too (absolute paths + # pass through unchanged), so behavior matches the config/default path and + # doesn't depend on the cwd the command happens to run from + lessons_path = _repo_root() / args.lessons + else: + cfg = _load_config() + cfg_lessons = cfg.get("lessons_path") + rel = ( + cfg_lessons + if isinstance(cfg_lessons, str) and cfg_lessons + else DEFAULT_LESSONS_PATH + ) + lessons_path = _repo_root() / rel + + commits = _commits_since(base) + if not commits: + json.dump( + { + "signals": [], + "next_lesson_id": _next_lesson_id(lessons_path), + "existing_entries": _existing_lessons(lessons_path), + "lessons_exists": lessons_path.exists(), + }, + sys.stdout, + indent=2, + ) + print() + return 0 + + file_counts: dict[str, int] = {} + for c in commits: + for f in _files_in_commit(c["sha"]): + file_counts[f] = file_counts.get(f, 0) + 1 + + signals = [] + + repeated = [ + {"file": f, "commit_count": n} + for f, n in sorted(file_counts.items(), key=lambda x: -x[1]) + if n >= 3 + ] + if repeated: + signals.append({"type": "repeated_edits", "items": repeated}) + + lint_fixes = [ + {"sha": c["sha"][:7], "subject": c["subject"]} + for c in commits + if LINT_FIX_RE.search(c["subject"]) + ] + if lint_fixes: + signals.append({"type": "lint_fixes", "items": lint_fixes}) + + corrections = [ + {"sha": c["sha"][:7], "subject": c["subject"]} + for c in commits + if CORRECTION_RE.match(c["subject"]) + ] + if corrections: + signals.append({"type": "corrections", "items": corrections}) + + next_id = _next_lesson_id(lessons_path) + existing = _existing_lessons(lessons_path) + + result = { + "signals": signals, + "next_lesson_id": next_id, + "existing_entries": existing, + "lessons_exists": lessons_path.exists(), + } + + json.dump(result, sys.stdout, indent=2) + print() + return 0 + + +def cmd_architecture(args: argparse.Namespace) -> int: + """Check ARCHITECTURE.md for staleness against the branch diff.""" + if args.base: + base = args.base + else: + trunk_ref = _resolve_trunk_ref(_resolve_trunk_name()) + if not trunk_ref: + return _emit( + { + "error": ( + "Could not resolve trunk; pass --base or run: " + "git remote set-head origin -a" + ) + }, + code=1, + ) + base = _run(["git", "merge-base", trunk_ref, "HEAD"]) + if not base: + return _emit({"error": "Could not determine merge-base"}, code=1) + + cfg = _load_config() + arch_doc = cfg.get("architecture_doc") + if not isinstance(arch_doc, str) or not arch_doc: + arch_doc = DEFAULT_ARCHITECTURE_DOC + sections = _config_architecture_sections(cfg) + + arch_path = _repo_root() / arch_doc + if not arch_path.exists(): + json.dump( + { + "stale_sections": [], + "architecture_exists": False, + "architecture_doc": arch_doc, + "config_present": bool(cfg), + "changed_files_count": len(_changed_files_since(base)), + }, + sys.stdout, + indent=2, + ) + print() + return 0 + + # No section map configured -> nothing to key staleness off. This is the + # portable default: a repo with no `[architecture_sections]` gets a clean + # no-op rather than an error, so Phase 1.5c simply finds nothing to update. + if not sections: + json.dump( + { + "stale_sections": [], + "architecture_exists": True, + "architecture_doc": arch_doc, + "config_present": bool(cfg), + "changed_files_count": len(_changed_files_since(base)), + }, + sys.stdout, + indent=2, + ) + print() + return 0 + + changed_files = _changed_files_since(base) + stale = [ + section_name + for section_name, path_prefixes in sections.items() + if any( + f.startswith(prefix) for prefix in path_prefixes for f in changed_files + ) + ] + + result = { + "stale_sections": stale, + "architecture_exists": True, + "architecture_doc": arch_doc, + "config_present": bool(cfg), + "changed_files_count": len(changed_files), + } + + json.dump(result, sys.stdout, indent=2) + print() + return 0 + + +def main() -> int: + """CLI entry point for /prepare-pr helpers.""" + parser = argparse.ArgumentParser(description=__doc__) + sub = parser.add_subparsers(dest="command", required=True) + + st = sub.add_parser( + "sync-trunk", + help="Fetch origin and fast-forward local trunk if behind (abort if diverged)", + ) + st.add_argument("--trunk-name", help="Override trunk name (default: autoresolve)") + + sub.add_parser("trunk", help="Resolve trunk name + ref (JSON, diagnostic)") + sub.add_parser( + "trunk-name", help="Print trunk branch name (plain text, for gh pr create)" + ) + sub.add_parser( + "trunk-ref", + help="Print trunk git ref (plain text, for git log/diff/merge-base/reset)", + ) + + bs = sub.add_parser( + "branch-safety", help="Verify current branch is safe to rewrite" + ) + bs.add_argument("--trunk-name", help="Override trunk name (default: autoresolve)") + + pf = sub.add_parser( + "preflight-wip", + help=( + "Self-applying WIP guard for Phase 1.5: commits dirty work itself " + "(use --dry-run to inspect without committing)." + ), + ) + pf.add_argument("--trunk-ref", help="Override trunk ref (default: autoresolve)") + pf.add_argument( + "--dry-run", + action="store_true", + help="Print 'create-wip'/'skip' but do NOT commit (legacy behavior).", + ) + + pb = sub.add_parser( + "phase2-base", + help="Emit the merge-base SHA to soft-reset to in Phase 2", + ) + pb.add_argument("--trunk-ref", help="Override trunk ref (default: autoresolve)") + + sig = sub.add_parser("signals", help="Extract learning signals from WIP history") + sig.add_argument( + "--base", help="Git ref to diff against (default: merge-base with trunk)" + ) + sig.add_argument( + "--lessons", help="Path to lessons.md (default: design/lessons.md)" + ) + + arch = sub.add_parser("architecture", help="Check ARCHITECTURE.md for staleness") + arch.add_argument( + "--base", help="Git ref to diff against (default: merge-base with trunk)" + ) + + args = parser.parse_args() + + dispatch = { + "sync-trunk": cmd_sync_trunk, + "trunk": cmd_trunk, + "trunk-name": cmd_trunk_name, + "trunk-ref": cmd_trunk_ref, + "branch-safety": cmd_branch_safety, + "preflight-wip": cmd_preflight_wip, + "phase2-base": cmd_phase2_base, + "signals": cmd_signals, + "architecture": cmd_architecture, + } + handler = dispatch.get(args.command) + if handler is None: + return 1 + return handler(args) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.claude/skills/second-opinion/SKILL.md b/.claude/skills/second-opinion/SKILL.md index 295fd281..d91b2104 100644 --- a/.claude/skills/second-opinion/SKILL.md +++ b/.claude/skills/second-opinion/SKILL.md @@ -28,7 +28,7 @@ The bias-control mechanism is **the process boundary itself**. Copilot runs as a Run the bundled check script to verify `REVIEW-BRIEF.md` targets this repo: ```bash -python3 /scripts/review_brief.py check +uv run --frozen python /scripts/review_brief.py check ``` Returns JSON with `match`, `brief_repo`, `current_repo`, `needs_update`. @@ -37,7 +37,7 @@ Returns JSON with `match`, `brief_repo`, `current_repo`, `needs_update`. - **Mismatch or missing `repo:` tag** → run discovery and offer a one-time rewrite: ```bash - python3 /scripts/review_brief.py discover + uv run --frozen python /scripts/review_brief.py discover ``` The discovery output includes detected stacks, context from `CLAUDE.md`/`AGENTS.md`/`README.md`, and the existing brief content. Use this context to rewrite `REVIEW-BRIEF.md` with: @@ -115,7 +115,7 @@ If Copilot exits non-zero, capture the error and surface to the user. Don't retr Save Copilot's raw output to a temp file and parse it with the bundled script: ```bash -python3 /scripts/review_brief.py parse /tmp/copilot-review.md +uv run --frozen python /scripts/review_brief.py parse /tmp/copilot-review.md ``` Returns JSON: `{"findings": [{id, severity, file, line, description, suggestion}, ...], "count": N}`. If count is 0, announce "No findings." and exit. diff --git a/.claude/skills/second-opinion/scripts/review_brief.py b/.claude/skills/second-opinion/scripts/review_brief.py index 0e384fa7..89bd1e64 100755 --- a/.claude/skills/second-opinion/scripts/review_brief.py +++ b/.claude/skills/second-opinion/scripts/review_brief.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python3 +#!/usr/bin/env -S uv run --frozen python """ Review brief management for the /second-opinion skill. @@ -8,10 +8,10 @@ parse - parse Copilot's raw output into structured JSON findings Usage: - python3 scripts/review_brief.py check - python3 scripts/review_brief.py discover - python3 scripts/review_brief.py parse - echo "" | python3 scripts/review_brief.py parse - + uv run --frozen python scripts/review_brief.py check + uv run --frozen python scripts/review_brief.py discover + uv run --frozen python scripts/review_brief.py parse + echo "" | uv run --frozen python scripts/review_brief.py parse - """ from __future__ import annotations @@ -25,7 +25,8 @@ BRIEF_FILENAME = "REVIEW-BRIEF.md" CONTEXT_FILES = [ - ("claude_md", "CLAUDE.md"), + ("claude_md", ".claude/CLAUDE.md"), + ("claude_md_root", "CLAUDE.md"), ("agents_md", "AGENTS.md"), ("architecture_md", "ARCHITECTURE.md"), ("readme", "README.md"), @@ -104,6 +105,38 @@ def _read_head(path: Path, max_lines: int = MAX_CONTEXT_LINES) -> str | None: return None +def _walk_pruned(root: Path): + """ + Recursively yield paths under root, skipping SKIP_DIRS subtrees. + + Plain Path.rglob() descends into vendored trees (.venv, node_modules, ...), + which can mean tens of thousands of irrelevant files on a stack probe. This + prunes those directories in place so discovery stays fast in real repos. + """ + stack = [root] + while stack: + current = stack.pop() + try: + entries = list(current.iterdir()) + except OSError: + continue + for entry in entries: + # Recurse into real subdirectories only. Skipping symlinked dirs (rather + # than following them) avoids accidental traversal of external trees and + # symlink cycles. is_dir() + not is_symlink() is the 3.12-compatible form + # of is_dir(follow_symlinks=False) (that kwarg is 3.13+). + if entry.is_dir() and not entry.is_symlink(): + if entry.name in SKIP_DIRS: + continue + stack.append(entry) + yield entry + + +def _any_match(root: Path, suffix: str) -> bool: + """True if any non-pruned file under root ends with suffix.""" + return any(p.name.endswith(suffix) and p.is_file() for p in _walk_pruned(root)) + + def _detect_stacks(root: Path) -> list[str]: """Quick stack detection (subset of bootstrap-hooks/detect_stack.py).""" stacks: list[str] = [] @@ -124,15 +157,16 @@ def _detect_stacks(root: Path) -> list[str]: stacks.append(stack) break - if any(root.rglob("*.tf")): + if _any_match(root, ".tf"): stacks.append("terraform") - if any(root.rglob("*.sh")): + if _any_match(root, ".sh"): stacks.append("shell") - if (root / ".obsidian").exists() or any( - (root / "docs").rglob(".obsidian") if (root / "docs").exists() else [] + docs = root / "docs" + if (root / ".obsidian").exists() or ( + docs.exists() and any(p.name == ".obsidian" for p in _walk_pruned(docs)) ): stacks.append("obsidian") - if (root / "docs").exists() and any((root / "docs").rglob("*.md")): + if docs.exists() and _any_match(docs, ".md"): stacks.append("markdown-docs") if (root / ".github" / "workflows").exists(): stacks.append("github-actions") @@ -187,7 +221,10 @@ def cmd_check(args: argparse.Namespace) -> int: json.dump(result, sys.stdout, indent=2) print() - return 0 if result["match"] else 1 + # Fail only when the brief genuinely needs updating (missing tag or a real + # mismatch). The indeterminate case - current repo can't be determined, e.g. + # gh CLI unavailable - sets needs_update: false and must not fail the check. + return 1 if result["needs_update"] else 0 def cmd_discover(args: argparse.Namespace) -> int: diff --git a/.github/workflows/repo_checks.yml b/.github/workflows/repo_checks.yml index a947f50d..945dfdf1 100644 --- a/.github/workflows/repo_checks.yml +++ b/.github/workflows/repo_checks.yml @@ -15,6 +15,13 @@ jobs: - name: Fetch base branch run: git fetch origin ${{ github.event.pull_request.base.ref }} --depth=1 + - name: Ensure Pester 6+ available + shell: pwsh + run: | + if (-not (Get-Module -ListAvailable -Name Pester | Where-Object { $_.Version -ge '6.0' })) { + Install-Module Pester -MinimumVersion 6.0 -Repository PSGallery -Force -SkipPublisherCheck -Scope CurrentUser + } + - name: Run pre-commit hooks uses: j178/prek-action@v2 with: diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ec543de3..95ca33e8 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -135,3 +135,39 @@ For functions installed system-wide via `/etc/profile.d/`, the file MUST start w ``` See `design/lessons.md` for the incident this guard prevents. + +## 6. PowerShell modules + +`modules/` holds the PowerShell modules installed into the guest's module path during provisioning. Each is a standard module directory (`/.psd1` manifest + `.psm1` root + `Functions/*.ps1`) with a `ModuleVersion`. + +### 6.1 Synced vs. repo-local + +Most modules are **not authored here** - they are mirrored from the upstream [`szymonos/ps-modules`](https://github.com/szymonos/ps-modules) repo. `.assets/scripts/modules_update.ps1` clones/refreshes `../ps-modules` (via `Invoke-GhRepoClone`) and copies a fixed list into `modules/`, overwriting the local copies. **Edit synced modules upstream, not here** - a local edit is silently clobbered on the next `modules_update.ps1` run. + +| Module | Origin | Notes | +| ----------------- | ---------- | --------------------------------------------------------------------------- | +| `do-common` | synced | Shared functions; installed **AllUsers** (system-wide), not per-user | +| `do-unix` | synced | Cross-platform (Linux + macOS) shell helpers; supersedes the old `do-linux` | +| `do-az` | synced | Azure helpers; installed only when the `az` scope is selected | +| `psm-windows` | synced | Windows-side helpers | +| `aliases-git` | synced | git aliases | +| `aliases-kubectl` | synced | kubectl aliases; installed only when the `k8s_base` scope is selected | +| `utils-install` | repo-local | Provisioning-time helpers (e.g. `Invoke-GhRepoClone`) - authored here | +| `utils-setup` | repo-local | Setup helpers - authored here | + +The synced list is the `$modules` array in `modules_update.ps1`; keep it in sync with what upstream ships. `do-unix` replaced `do-linux` (same module GUID, cross-platform `Get-SysInfo`); when adding or renaming a synced module, update **both** the sync list and every install site (see § 6.2). + +### 6.2 Install sites + +Provisioning copies modules into the guest's PowerShell module path from four places. All four must agree on the module name set - a rename (like `do-linux` → `do-unix`) touches every one: + +| Site | Context | Which modules | +| ------------------------------------------ | ------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `.assets/scripts/linux_setup.sh` | Linux host / guest | `do-common` AllUsers; `do-unix` + scoped extras CurrentUser | +| `wsl/wsl_setup.ps1` | Windows host → WSL | same split, driven by the `$scopes` selected | +| `vagrant/*/*/Vagrantfile` | Vagrant VMs | `aliases-git do-common do-unix` (12 files, one per box) | +| `.assets/provision/setup_profile_user.ps1` | user profile setup | references `do-unix` for the `Register-MakeCompleter` completer; also removes the obsolete user-scope `do-linux` module so a rename doesn't leave duplicate commands | + +### 6.3 Add a module function + +See § 5.2. For a **synced** module, make the change in the upstream `ps-modules` repo and re-run `modules_update.ps1`; only `utils-install` / `utils-setup` are edited directly in this repo. diff --git a/Makefile b/Makefile index ad6777c9..aa72b8d7 100644 --- a/Makefile +++ b/Makefile @@ -11,15 +11,13 @@ ifdef CA_CUSTOM export NODE_EXTRA_CA_CERTS := $(CA_CUSTOM) endif -# Stage all changes, run prek, then restore previously staged file paths. -# Note: only path-level staging is preserved; partially-staged hunks become fully -# staged after the round-trip. Auto-fixes from hooks land in the working tree. +# Run prek over the whole working tree without touching the user's staging: +# stage into a disposable scratch index (GIT_INDEX_FILE) so the real index is +# never modified. Auto-fixes land in the working tree for review. define PREK_RUN -sf=$$(mktemp); git diff --cached --name-only -z >$$sf; \ -git add --all && prek run $(HOOK) $(1); rc=$$?; \ -git reset -q HEAD; \ -if [ -s $$sf ]; then xargs -0 git add -- <$$sf 2>/dev/null; fi; \ -rm -f $$sf; exit $$rc +export GIT_INDEX_FILE="$$(git rev-parse --git-path index).prek"; \ +trap 'rm -f "$$GIT_INDEX_FILE" "$$GIT_INDEX_FILE.lock"' EXIT; \ +git read-tree HEAD && git add --all && prek run $(HOOK) $(1) endef .PHONY: help diff --git a/design/lessons.md b/design/lessons.md index 48cf87ff..4dccf7fd 100644 --- a/design/lessons.md +++ b/design/lessons.md @@ -39,3 +39,10 @@ When adding an entry, link the commit, the rule it produced (under `.claude/rule - **Symptom:** `git_resolve_branch ""` and `git_resolve_branch "d"` returned the literal regex pattern instead of resolving to `dev`/`devel`/`development`. Worked on Linux, failed silently on macOS. Surfaced by envy-nx's integration workflow running the full bats suite on `macos-15`. - **Root cause:** Case-arm patterns like `(|el|elop|elopment)` use an empty alternative at the start. GNU grep matches it; BSD grep on macOS doesn't, and yields no match. The functions returned the unmatched pattern as a literal string. - **Rule:** Never use empty-alternative regex (`(|foo|bar)`). Use the explicit-optional form `(foo|bar)?` - unambiguously correct on both GNU and BSD grep, and equivalent for purposes of "match nothing or one of the alternatives". This is also a useful test ground for the cross-platform bats suite: anything that worked on Linux but only Linux is an under-tested portability assumption. + +## 2026-07 - Synced-module parameter rename broke external call sites + +- **Commit:** `632d7f7` (follow-up to the `do-common` v2.0.0 sync) +- **Symptom:** After syncing `do-common` v2.0.0 from `ps-modules`, `wsl_certs_add.ps1`, `vg_cacert_fix.ps1`, and `vg_certs_add.ps1` still called `Get-Certificate -BuildChain`, a parameter the new module version had renamed to `-PresentedChain`. The scripts throw at runtime because the old parameter no longer binds. +- **Root cause:** Synced modules (see `ARCHITECTURE.md` § 6.1) are mirrored wholesale from upstream, so a renamed/removed public parameter or function lands here with no local diff to flag it. Consumers *outside* the module directory - provisioning scripts, other modules - keep the old call and break silently until run. +- **Rule:** When a `modules_update.ps1` sync changes a synced module's **public surface** (renamed/removed parameter, function, or alias), grep the whole repo for call sites *outside* `modules//` and update them in the same branch. `rg -F ''` across `.assets`, `wsl`, and sibling `modules/` is the completeness check. A synced version bump is a consumer-migration task, not just a file copy. diff --git a/modules/do-common/Functions/certs.ps1 b/modules/do-common/Functions/certs.ps1 index 0dad96af..e56362c4 100644 --- a/modules/do-common/Functions/certs.ps1 +++ b/modules/do-common/Functions/certs.ps1 @@ -174,16 +174,119 @@ function ConvertTo-PEM { } +<# +.SYNOPSIS +Split a Uri into its hostname and port components. + +.DESCRIPTION +Accepts a bare hostname ('example.com'), a host:port pair ('example.com:8443') +or a full Uri ('https://example.com:8443/path') and returns the hostname and +port. A port embedded in the Uri takes precedence over the Port parameter. + +.PARAMETER Uri +Uri, hostname or host:port string to parse. +.PARAMETER Port +Fallback port used when no port is embedded in the Uri. +#> +function Split-UriHostPort { + [CmdletBinding()] + [OutputType([System.Object[]])] + param ( + [Parameter(Mandatory, Position = 0)] + [string]$Uri, + + [Parameter(Position = 1)] + [ValidateRange(1, 65535)] + [int]$Port = 443 + ) + + # strip scheme, then path/query/fragment, then userinfo to leave the authority component + $authority = (($Uri -replace '^[a-zA-Z][\w+.-]*://') -split '[/?#]', 2)[0].Split('@')[-1] + + if ($authority -match '^\[(?.+)\](?::(?\d+))?$') { + # bracketed IPv6 literal, optionally with a port + $hostname = $Matches.host + if ($Matches.port) { $Port = [int]$Matches.port } + } elseif ($authority -match '^(?[^:]+):(?\d+)$') { + # host:port + $hostname = $Matches.host + $Port = [int]$Matches.port + } elseif ($authority -match '^[^:]+:[^:]*$') { + # single colon but a non-numeric/empty port - fail fast instead of mis-parsing as a hostname + throw "Invalid port in Uri '$Uri'. Expected 'host:'." + } else { + # bare hostname or bare (unbracketed) IPv6 literal + $hostname = $authority + } + + return $hostname, $Port +} + + +<# +.SYNOPSIS +Build a concise ErrorRecord for certificate retrieval failures. + +.DESCRIPTION +Unwraps nested .NET exceptions (e.g. the SocketException behind a +"Exception calling ..." wrapper) so the reported message states the actual +cause and prefixes it with the target host:port. + +.PARAMETER Exception +Exception thrown while connecting or performing the TLS handshake. +.PARAMETER Target +Target 'host:port' the certificate was requested from. +#> +function New-CertificateError { + [CmdletBinding()] + [OutputType([System.Management.Automation.ErrorRecord])] + param ( + [Parameter(Mandatory, Position = 0)] + [System.Exception]$Exception, + + [Parameter(Mandatory, Position = 1)] + [string]$Target + ) + + # drill down to the innermost exception for the root-cause message + $inner = $Exception + while ($inner.InnerException) { + $inner = $inner.InnerException + } + # pick the error category based on the underlying exception type + $category = $inner -is [System.Net.Sockets.SocketException] ` + ? [System.Management.Automation.ErrorCategory]::ConnectionError + : [System.Management.Automation.ErrorCategory]::NotSpecified + + return [System.Management.Automation.ErrorRecord]::new( + [System.Exception]::new("Failed to get certificate from '$Target'. $($inner.Message)", $Exception), + 'CertificateRetrievalError', + $category, + $Target + ) +} + + <# .SYNOPSIS Get certificate(s) from specified Uri. .PARAMETER Uri -Uri used for intercepting certificate. -.PARAMETER BuildChain -Switch whether to build full certificate chain. -.PARAMETER IgnoreValidation -Ignore validation errors for getting certificate/building chain. +Uri used for intercepting certificate. The port can be appended to the host +(e.g. 'example.com:8443'), analogous to `openssl s_client -connect host:port`. +.PARAMETER Port +Port used for the TLS connection. Defaults to 443 and is overridden by a port +embedded in the Uri. +.PARAMETER PresentedChain +Return the full certificate chain presented by the endpoint during the TLS +handshake (leaf first), instead of just the leaf certificate. The chain is +captured from the handshake itself, so it works even for endpoints with a +broken/untrusted chain or behind an inspecting proxy. + +.NOTES +The certificate presented during the handshake is always accepted, regardless of +its validity, so inspecting expired/self-signed/untrusted certificates never +throws. Only connection and transport failures raise an error. #> function Get-Certificate { [CmdletBinding()] @@ -192,42 +295,64 @@ function Get-Certificate { [Parameter(Mandatory, Position = 0)] [string]$Uri, - [switch]$BuildChain, + [Parameter(Position = 1)] + [ValidateRange(1, 65535)] + [int]$Port = 443, - [switch]$IgnoreValidation + [switch]$PresentedChain ) begin { - $tcpClient = [System.Net.Sockets.TcpClient]::new($Uri, 443) - if ($BuildChain) { - $chain = [System.Security.Cryptography.X509Certificates.X509Chain]::new() - } - if ($IgnoreValidation) { - $sslStream = [System.Net.Security.SslStream]::new($tcpClient.GetStream(), $false, { $true }) - if ($BuildChain) { - $chain.ChainPolicy.VerificationFlags = [System.Security.Cryptography.X509Certificates.X509VerificationFlags]::AllFlags - } - } else { - $sslStream = [System.Net.Security.SslStream]::new($tcpClient.GetStream()) + # parse hostname and port from the Uri ('host', 'host:port' or 'scheme://host:port/path') + try { + $hostname, $Port = Split-UriHostPort -Uri $Uri -Port $Port + } catch { + $PSCmdlet.ThrowTerminatingError((New-CertificateError -Exception $_.Exception -Target $Uri)) } } process { + # list to capture the presented chain from the handshake callback; the certs + # are deep-copied (via RawData) so they survive disposal of the SslStream + $presented = [System.Collections.Generic.List[System.Security.Cryptography.X509Certificates.X509Certificate2]]::new() + $validationCallback = { + param ($senderObj, $cert, $chain, $sslPolicyErrors) + # reset so a repeated callback invocation replaces rather than duplicates + $presented.Clear() + # $chain can be null in some handshake edge cases; guard before enumerating + if ($chain) { + foreach ($element in $chain.ChainElements) { + $presented.Add([System.Security.Cryptography.X509Certificates.X509Certificate2]::new($element.Certificate.RawData)) + } + } + # always accept - this function inspects certificates, it does not validate trust + return $true + } + + $tcpClient = $sslStream = $null try { - $sslStream.AuthenticateAsClient($Uri) - $certificate = $sslStream.RemoteCertificate + # open the TCP connection to the target host and port + $tcpClient = [System.Net.Sockets.TcpClient]::new($hostname, $Port) + + # perform the TLS handshake, accepting any certificate so even invalid + # certs can be inspected; the callback captures the full presented chain + $sslStream = [System.Net.Security.SslStream]::new($tcpClient.GetStream(), $false, $validationCallback) + $sslStream.AuthenticateAsClient($hostname) + # RemoteCertificate is typed X509Certificate; use GetRawCertData() (base + # method) and deep-copy so the leaf survives disposal of the SslStream + $rawLeaf = $sslStream.RemoteCertificate.GetRawCertData() + $leaf = [System.Security.Cryptography.X509Certificates.X509Certificate2]::new($rawLeaf) + } catch { + # surface a concise, categorized error instead of the raw .NET exception + $PSCmdlet.ThrowTerminatingError((New-CertificateError -Exception $_.Exception -Target "${hostname}:${Port}")) } finally { - $sslStream.Close() + if ($sslStream) { $sslStream.Dispose() } + if ($tcpClient) { $tcpClient.Dispose() } } - if ($BuildChain) { - $isChainValid = $chain.Build($certificate) - if ($isChainValid) { - $certificate = $chain.ChainElements.Certificate - } else { - Write-Warning 'SSL certificate chain validation failed.' - } - } + # return the presented chain (leaf first), or just the leaf certificate; + # fall back to the leaf if the callback couldn't populate the chain + $certificate = ($PresentedChain -and $presented.Count) ? $presented.ToArray() : $leaf } end { @@ -241,9 +366,15 @@ function Get-Certificate { Get certificate(s) from specified Uri using OpenSSL application. .PARAMETER Uri -Uri used for intercepting certificate. -.PARAMETER BuildChain -Switch whether to build full certificate chain. +Uri used for intercepting certificate. The port can be appended to the host +(e.g. 'example.com:8443'), analogous to `openssl s_client -connect host:port`. +.PARAMETER Port +Port used for the TLS connection. Defaults to 443 and is overridden by a port +embedded in the Uri. +.PARAMETER PresentedChain +Return the full certificate chain presented by the endpoint (leaf first), instead +of just the leaf certificate. Uses `openssl s_client -showcerts`, so the returned +certificates are the exact bytes the endpoint sent on the wire. #> function Get-CertificateOpenSSL { [CmdletBinding()] @@ -252,20 +383,42 @@ function Get-CertificateOpenSSL { [Parameter(Mandatory, Position = 0)] [string]$Uri, - [switch]$BuildChain + [Parameter(Position = 1)] + [ValidateRange(1, 65535)] + [int]$Port = 443, + + [switch]$PresentedChain ) begin { # check if OpenSSL is installed if (-not (Get-Command openssl -CommandType Application -ErrorAction SilentlyContinue)) { - Throw 'OpenSSL not found. Script execution halted.' + $er = [System.Management.Automation.ErrorRecord]::new( + [System.Exception]::new('OpenSSL not found. Install OpenSSL or omit the -OpenSSL switch.'), + 'OpenSSLNotFound', + [System.Management.Automation.ErrorCategory]::NotInstalled, + 'openssl' + ) + $PSCmdlet.ThrowTerminatingError($er) + } + + # parse hostname and port from the Uri ('host', 'host:port' or 'scheme://host:port/path') + try { + $hostname, $Port = Split-UriHostPort -Uri $Uri -Port $Port + } catch { + $PSCmdlet.ThrowTerminatingError((New-CertificateError -Exception $_.Exception -Target $Uri)) } + # bracket IPv6 literals (a bare colon in the host) for OpenSSL's host:port syntax + $connectHost = $hostname.Contains(':') ? "[$hostname]" : $hostname + # build the OpenSSL argument list [System.Collections.Generic.List[string]]$cmdArgs = @('s_client') $cmdArgs.Add('-connect') - $cmdArgs.Add("${Uri}:443") - if ($BuildChain) { + $cmdArgs.Add("${connectHost}:${Port}") + $cmdArgs.Add('-servername') + $cmdArgs.Add($hostname) + if ($PresentedChain) { $cmdArgs.Add('-showcerts') } } @@ -275,11 +428,17 @@ function Get-CertificateOpenSSL { # Use the call operator (&) to execute OpenSSL with arguments $opensslOutput = Out-Null | & openssl @cmdArgs 2>$null } catch { - Throw "Error executing OpenSSL: $_" + $PSCmdlet.ThrowTerminatingError((New-CertificateError -Exception $_.Exception -Target "${hostname}:${Port}")) } if (-not $opensslOutput) { - Throw "No output from OpenSSL. Possibly an unknown host: `"$Uri`"." + $er = [System.Management.Automation.ErrorRecord]::new( + [System.Exception]::new("Failed to get certificate from '${hostname}:${Port}'. No output from OpenSSL, possibly an unknown host."), + 'CertificateRetrievalError', + [System.Management.Automation.ErrorCategory]::ConnectionError, + "${hostname}:${Port}" + ) + $PSCmdlet.ThrowTerminatingError($er) } # Normalize the output: join array into one string and standardize line breaks @@ -290,7 +449,13 @@ function Get-CertificateOpenSSL { $reMatches = [regex]::Matches($outputText, $pemPattern) if ($reMatches.Count -eq 0) { - Throw "No certificates found in OpenSSL output for `"$Uri`"." + $er = [System.Management.Automation.ErrorRecord]::new( + [System.Exception]::new("Failed to get certificate from '${hostname}:${Port}'. No certificates found in OpenSSL output."), + 'CertificateRetrievalError', + [System.Management.Automation.ErrorCategory]::InvalidResult, + "${hostname}:${Port}" + ) + $PSCmdlet.ThrowTerminatingError($er) } # Convert each PEM block to an X509Certificate2 object @@ -317,6 +482,12 @@ function Get-RootCertificates { $sysId = (Select-String '(?<=^ID.+)(alpine|arch|fedora|debian|ubuntu|opensuse)' -List /etc/os-release).Matches.Value $certPath = $sysId -eq 'opensuse' ? '/etc/ssl/ca-bundle.pem' : '/etc/ssl/certs/ca-certificates.crt' ConvertFrom-PEM -Path $certPath + } elseif ($IsMacOS) { + # read trusted roots from the system keychain (includes MDM-pushed roots) + $pem = security find-certificate -a -p /System/Library/Keychains/SystemRootCertificates.keychain /Library/Keychains/System.keychain 2>$null + if ($pem) { + ConvertFrom-PEM -InputObject ([string]::Join("`n", $pem)) + } } } @@ -326,11 +497,16 @@ function Get-RootCertificates { Show certificate chain for a specified Uri. .PARAMETER Uri -Uri used for intercepting certificate chain. +Uri used for intercepting certificate chain. The port can be appended to the +host (e.g. 'example.com:8443'), analogous to `openssl s_client -connect host:port`. +.PARAMETER Port +Port used for the TLS connection. Defaults to 443 and is overridden by a port +embedded in the Uri. .PARAMETER InputObject Object from pipeline to show certificate properties. -.PARAMETER BuildChain -Build chain for certificate obtained from Uri. +.PARAMETER PresentedChain +Show the full certificate chain presented by the endpoint, instead of just the +leaf certificate. .PARAMETER Extended Switch, whether to show extended certificate properties. .PARAMETER Strip @@ -347,11 +523,15 @@ function Show-Certificate { [Parameter(Mandatory, Position = 0, ParameterSetName = 'FromUri')] [string]$Uri, + [Parameter(Position = 1, ParameterSetName = 'FromUri')] + [ValidateRange(1, 65535)] + [int]$Port = 443, + [Parameter(Mandatory, ValueFromPipeline, ParameterSetName = 'FromPipeline')] [System.Security.Cryptography.X509Certificates.X509Certificate2[]]$InputObject, [Parameter(ParameterSetName = 'FromUri')] - [switch]$BuildChain, + [switch]$PresentedChain, [switch]$Extended, @@ -363,8 +543,6 @@ function Show-Certificate { ) begin { - $WarningPreference = 'Stop' - # build properties for Show-Object function $showCertProp = if ($All) { @{ } @@ -394,11 +572,16 @@ function Show-Certificate { process { switch ($PsCmdlet.ParameterSetName) { FromUri { - $cert = if ($PSBoundParameters.OpenSSL) { - $PSBoundParameters.Remove('OpenSSL') | Out-Null - Get-CertificateOpenSSL @PSBoundParameters | Add-CertificateProperties - } else { - Get-Certificate @PSBoundParameters | Add-CertificateProperties + try { + $cert = if ($PSBoundParameters.OpenSSL) { + $PSBoundParameters.Remove('OpenSSL') | Out-Null + Get-CertificateOpenSSL @PSBoundParameters | Add-CertificateProperties + } else { + Get-Certificate @PSBoundParameters | Add-CertificateProperties + } + } catch { + # re-throw the concise error from Get-Certificate without the wrapper's call-site noise + $PSCmdlet.ThrowTerminatingError($_) } } FromPipeline { @@ -419,7 +602,11 @@ function Show-Certificate { Show certificate chain for a specified Uri. .PARAMETER Uri -Uri used for intercepting certificate chain. +Uri used for intercepting certificate chain. The port can be appended to the +host (e.g. 'example.com:8443'), analogous to `openssl s_client -connect host:port`. +.PARAMETER Port +Port used for the TLS connection. Defaults to 443 and is overridden by a port +embedded in the Uri. .PARAMETER Extended Switch, whether to show extended certificate properties. .PARAMETER Strip @@ -436,6 +623,10 @@ function Show-CertificateChain { [Parameter(Mandatory, Position = 0)] [string]$Uri, + [Parameter(Position = 1)] + [ValidateRange(1, 65535)] + [int]$Port = 443, + [Parameter(Mandatory, ParameterSetName = 'Extended')] [switch]$Extended, @@ -449,11 +640,16 @@ function Show-CertificateChain { ) begin { - $PSBoundParameters.Add('BuildChain', $true) + $PSBoundParameters.Add('PresentedChain', $true) } process { - Show-Certificate @PSBoundParameters + try { + Show-Certificate @PSBoundParameters + } catch { + # re-throw the concise error without the wrapper's call-site noise + $PSCmdlet.ThrowTerminatingError($_) + } } } diff --git a/modules/do-common/Functions/python.ps1 b/modules/do-common/Functions/python.ps1 index acdc07d1..2a1ae00f 100644 --- a/modules/do-common/Functions/python.ps1 +++ b/modules/do-common/Functions/python.ps1 @@ -11,13 +11,15 @@ function Invoke-CertifiFixFromChain { $cacertPaths = [System.Collections.Generic.HashSet[string]]::new() # get certifi/cacert.pem file path foreach ($package in @('certifi', 'pip')) { + # reset per package so a previous iteration's result never carries over + $showFiles = $null if (Get-Command 'uv' -CommandType Application -ErrorAction SilentlyContinue) { [string[]]$showFiles = uv pip show -f $package 2>$null } - if (-not $showFiles) { + if (-not $showFiles -and (Get-Command 'pip' -CommandType Application -ErrorAction SilentlyContinue)) { [string[]]$showFiles = pip show -f $package 2>$null } - if ($location = ($showFiles | Select-String '(?<=^Location: ).+$').Matches.Value) { + if ($showFiles -and ($location = ($showFiles | Select-String '(?<=^Location: ).+$').Matches.Value)) { if ($cacert = ($showFiles | Select-String '\S*\bcacert\.pem$').Matches.Value) { $cacert.ForEach({ $cacertPaths.Add(([IO.Path]::Combine($location, $_))) | Out-Null }) } @@ -28,34 +30,55 @@ function Invoke-CertifiFixFromChain { process { if ($cacertPaths) { # get intermediate and root certificates - $chain = Invoke-CommandRetry { - Get-Certificate 'www.python.org' -BuildChain | Select-Object -Skip 1 - } - # check if root certificate from chain is installed in the system - $rootCrts = Get-RootCertificates - if ($chain[-1].Thumbprint -in $rootCrts.Thumbprint) { + $chain = @(Invoke-CommandRetry { + Get-Certificate 'www.python.org' -PresentedChain | Select-Object -Skip 1 + }) + # bail out if the endpoint presented only a leaf (nothing left after -Skip 1); + # Write-Error terminates under $ErrorActionPreference = 'Stop' + if (-not $chain) { + Write-Error 'No intermediate/root certificates presented by the TLS endpoint.' + } + # check if the chain's root certificate is trusted by the system; build the + # chain natively against the OS trust store (incl. the macOS keychain) with + # revocation and AIA downloads off, so it's an OS-trust-only check with no + # surprise network access + $rootChain = [System.Security.Cryptography.X509Certificates.X509Chain]::new() + $rootChain.ChainPolicy.RevocationMode = [System.Security.Cryptography.X509Certificates.X509RevocationMode]::NoCheck + $rootChain.ChainPolicy.DisableCertificateDownloads = $true + try { + $isRootTrusted = $rootChain.Build($chain[-1]) + } finally { + $rootChain.Dispose() + } + if ($isRootTrusted) { foreach ($path in $cacertPaths) { Write-Verbose $path.Replace($HOME, '~') - $certifiCerts = ConvertFrom-PEM $path - # check if certs already added to cacert.pem - if ($certsToAdd = $chain.ForEach({ $_.Where({ $_.Thumbprint -notin $certifiCerts.Thumbprint }) })) { - # add certificates from chain to the certifi/cacert.pem - foreach ($cert in $certsToAdd) { - $msg = [string]::Join("`n", - "`e[1;92mThumbprint :`e[0m $($cert.Thumbprint)", - "`e[1;92mSubject :`e[0m $($cert.Subject)", - "`e[1;92mIssuer :`e[0m $($cert.Issuer)`n" - ) - Write-Host $msg - $pem = "`n$(ConvertTo-PEM $cert -AddHeader)" - if ($IsLinux -and (Get-ChildItem $path).User -eq 'root') { - sudo pwsh -nop -noni -c "[IO.File]::AppendAllText('$path', '$pem')" - } else { - [IO.File]::AppendAllText($path, $pem) + # a single unwritable cacert (e.g. a system-owned pip _vendor bundle) + # must not abort fixing the remaining, writable paths + try { + $certifiCerts = ConvertFrom-PEM $path + # check if certs already added to cacert.pem + if ($certsToAdd = $chain.Where({ $_.Thumbprint -notin $certifiCerts.Thumbprint })) { + # add certificates from chain to the certifi/cacert.pem + foreach ($cert in $certsToAdd) { + $msg = [string]::Join("`n", + "`e[1;92mThumbprint :`e[0m $($cert.Thumbprint)", + "`e[1;92mSubject :`e[0m $($cert.Subject)", + "`e[1;92mIssuer :`e[0m $($cert.Issuer)`n" + ) + Write-Host $msg + $pem = "`n$(ConvertTo-PEM $cert -AddHeader)" + if ($IsLinux -and (Get-ChildItem $path).User -eq 'root') { + sudo pwsh -nop -noni -c "[IO.File]::AppendAllText('$path', '$pem')" + } else { + [IO.File]::AppendAllText($path, $pem) + } } + } else { + Write-Verbose 'All certificates from TLS chain already added to the file.' } - } else { - Write-Verbose 'All certificates from TLS chain already added to the file.' + } catch { + Write-Warning "Skipped '$($path.Replace($HOME, '~'))': $($_.Exception.Message)" } } } else { diff --git a/modules/do-common/do-common.psd1 b/modules/do-common/do-common.psd1 index 5ce6db23..367e8b17 100644 --- a/modules/do-common/do-common.psd1 +++ b/modules/do-common/do-common.psd1 @@ -4,7 +4,7 @@ RootModule = 'do-common.psm1' # Version number of this module. - ModuleVersion = '1.8.3' + ModuleVersion = '2.0.0' # Supported PSEditions CompatiblePSEditions = @('Core') @@ -42,6 +42,7 @@ 'Show-Certificate' 'Show-CertificateChain' 'Show-ConvertedPem' + 'Split-UriHostPort' # cli 'Invoke-DigColored' # common diff --git a/modules/do-common/do-common.psm1 b/modules/do-common/do-common.psm1 index 33a7f302..b8e3fa65 100644 --- a/modules/do-common/do-common.psm1 +++ b/modules/do-common/do-common.psm1 @@ -20,6 +20,7 @@ $exportModuleMemberParams = @{ 'Show-Certificate' 'Show-CertificateChain' 'Show-ConvertedPem' + 'Split-UriHostPort' # cli 'Invoke-DigColored' # common diff --git a/modules/do-linux/Functions/common.ps1 b/modules/do-unix/Functions/common.ps1 similarity index 53% rename from modules/do-linux/Functions/common.ps1 rename to modules/do-unix/Functions/common.ps1 index e57b8f60..35e55ab0 100644 --- a/modules/do-linux/Functions/common.ps1 +++ b/modules/do-unix/Functions/common.ps1 @@ -1,40 +1,76 @@ <# .SYNOPSIS -Returns system information from /etc/os-release. +Returns system information for Linux (/etc/os-release, /proc) or macOS (sw_vers, sysctl). #> function Get-SysInfo { - # get os-release properties - $osr = Get-DotEnv '/etc/os-release' - # get cpu info - $cpu = @{} - (Select-String '^model name|^cpu cores|^siblings' '/proc/cpuinfo' -Raw | Select-Object -Unique).ForEach({ - $key, $value = $_.Split(':').Trim() - $cpu[$key] = $value + if ($IsLinux) { + # get os-release properties + $osr = Get-DotEnv '/etc/os-release' + # get cpu info + $cpu = @{} + (Select-String '^model name|^cpu cores|^siblings' '/proc/cpuinfo' -Raw | Select-Object -Unique).ForEach({ + $key, $value = $_.Split(':').Trim() + $cpu[$key] = $value + } + ) + # calculate memory usage + $mem = @{} + (Select-String '^MemTotal|^MemAvailable' '/proc/meminfo' -Raw).ForEach({ + $key, $value = $_.Split(':') + $mem[$key] = ($value -replace '[^0-9]') / 1MB + } + ) + $mem['MemUsed'] = $mem.MemTotal - $mem.MemAvailable + + # build system properties + $sysProp = [ordered]@{ + UserHost = "`e[1;34m$(id -un)`e[0m@`e[1;34m$([System.IO.File]::ReadAllLines('/proc/sys/kernel/hostname'))`e[0m" + OS = "`e[1;37m$($osr.NAME) $($osr.BUILD_ID ?? $osr.VERSION ?? $osr.VERSION_ID) $(uname -m)`e[0m" + Kernel = uname -r + Uptime = "$(Get-Uptime)" } - ) - # calculate memory usage - $mem = @{} - (Select-String '^MemTotal|^MemAvailable' '/proc/meminfo' -Raw).ForEach({ - $key, $value = $_.Split(':') - $mem[$key] = ($value -replace '[^0-9]') / 1MB + if ($env:WSL_DISTRO_NAME) { $sysProp['OS Host'] = 'Windows Subsystem for Linux' } + if ($env:WSL_DISTRO_NAME) { $sysProp['WSL Distro'] = $env:WSL_DISTRO_NAME } + if ($env:CONTAINER_ID) { $sysProp['DistroBox'] = $env:CONTAINER_ID } + } elseif ($IsMacOS) { + # get macOS version info + $productName = (sw_vers -productName) + $productVersion = (sw_vers -productVersion) + $buildVersion = (sw_vers -buildVersion) + # get cpu info + $cpuBrand = (sysctl -n machdep.cpu.brand_string) + $physCores = (sysctl -n hw.physicalcpu) + $logCores = (sysctl -n hw.logicalcpu) + # calculate memory usage (free + inactive + speculative = available) + $memTotal = [long](sysctl -n hw.memsize) / 1GB + $vmStat = vm_stat + $pageSize = if ($vmStat[0] -match 'page size of (\d+)') { [long]$Matches[1] } else { 16384 } + $pagesFree = if ($vmStat -match 'Pages free:\s+([\d]+)') { [long]$Matches[1] } else { 0 } + $pagesInactive = if ($vmStat -match 'Pages inactive:\s+([\d]+)') { [long]$Matches[1] } else { 0 } + $pagesSpeculative = if ($vmStat -match 'Pages speculative:\s+([\d]+)') { [long]$Matches[1] } else { 0 } + $memAvailable = ($pagesFree + $pagesInactive + $pagesSpeculative) * $pageSize / 1GB + $memUsed = $memTotal - $memAvailable + + # build system properties + $sysProp = [ordered]@{ + UserHost = "`e[1;34m$(id -un)`e[0m@`e[1;34m$([System.Net.Dns]::GetHostName())`e[0m" + OS = "`e[1;37m$productName $productVersion ($buildVersion) $(uname -m)`e[0m" + Kernel = uname -r + Uptime = "$(Get-Uptime)" } - ) - $mem['MemUsed'] = $mem.MemTotal - $mem.MemAvailable - - # build system properties - $sysProp = [ordered]@{ - UserHost = "`e[1;34m$(id -un)`e[0m@`e[1;34m$([System.IO.File]::ReadAllLines('/proc/sys/kernel/hostname'))`e[0m" - OS = "`e[1;37m$($osr.NAME) $($osr.BUILD_ID ?? $osr.VERSION ?? $osr.VERSION_ID) $(uname -m)`e[0m" - Kernel = uname -r - Uptime = "$(Get-Uptime)" } - if ($env:WSL_DISTRO_NAME) { $sysProp['OS Host'] = 'Windows Subsystem for Linux' } - if ($env:WSL_DISTRO_NAME) { $sysProp['WSL Distro'] = $env:WSL_DISTRO_NAME } - if ($env:CONTAINER_ID) { $sysProp['DistroBox'] = $env:CONTAINER_ID } + if ($env:TERM_PROGRAM) { $sysProp['Terminal'] = $env:TERM_PROGRAM } $sysProp['Shell'] = "PowerShell $($PSVersionTable.PSVersion)" - $sysProp['CPU'] = "$($cpu['model name']) ($($cpu['cpu cores'])/$($cpu['siblings']))" - $sysProp['Memory'] = '{0:n2} GiB / {1:n2} GiB ({2:p0})' -f $mem['MemUsed'], $mem['MemTotal'], ($mem['MemUsed'] / $mem['MemTotal']) + + if ($IsLinux) { + $sysProp['CPU'] = "$($cpu['model name']) ($($cpu['cpu cores'])/$($cpu['siblings']))" + $sysProp['Memory'] = '{0:n2} GiB / {1:n2} GiB ({2:p0})' -f $mem['MemUsed'], $mem['MemTotal'], ($mem['MemUsed'] / $mem['MemTotal']) + } elseif ($IsMacOS) { + $sysProp['CPU'] = "$cpuBrand ($physCores/$logCores)" + $sysProp['Memory'] = '{0:n2} GiB / {1:n2} GiB ({2:p0})' -f $memUsed, $memTotal, ($memUsed / $memTotal) + } + if ($env:LANG) { $sysProp['Locale'] = $env:LANG } return [PSCustomObject]$sysProp diff --git a/modules/do-linux/Functions/completers.ps1 b/modules/do-unix/Functions/completers.ps1 similarity index 100% rename from modules/do-linux/Functions/completers.ps1 rename to modules/do-unix/Functions/completers.ps1 diff --git a/modules/do-linux/do-linux.psd1 b/modules/do-unix/do-unix.psd1 similarity index 97% rename from modules/do-linux/do-linux.psd1 rename to modules/do-unix/do-unix.psd1 index 5ee8747e..7c5bf190 100644 --- a/modules/do-linux/do-linux.psd1 +++ b/modules/do-unix/do-unix.psd1 @@ -1,5 +1,5 @@ # -# Module manifest for module 'do-linux' +# Module manifest for module 'do-unix' # # Generated by: szymono # @@ -9,7 +9,7 @@ @{ # Script module or binary module file associated with this manifest. - RootModule = 'do-linux.psm1' + RootModule = 'do-unix.psm1' # Version number of this module. ModuleVersion = '0.6.0' @@ -30,7 +30,7 @@ Copyright = '(c) szymono. All rights reserved.' # Description of the functionality provided by this module - Description = 'This module is intended to streamline my workflow with PowerShell on Linux.' + Description = 'This module is intended to streamline my workflow with PowerShell on Linux and macOS.' # Minimum version of the PowerShell engine required by this module CompatiblePSEditions = @('Core') diff --git a/modules/do-linux/do-linux.psm1 b/modules/do-unix/do-unix.psm1 similarity index 100% rename from modules/do-linux/do-linux.psm1 rename to modules/do-unix/do-unix.psm1 diff --git a/tests/hooks/align_tables.py b/tests/hooks/align_tables.py old mode 100755 new mode 100644 index 6e18475f..bed68b8c --- a/tests/hooks/align_tables.py +++ b/tests/hooks/align_tables.py @@ -1,24 +1,26 @@ -#!/usr/bin/env python3 """ Auto-align markdown table columns. Ensures all pipe characters in each table are at the same column position across all rows (MD060 compliance). -Usage: - python3 -m tests.hooks.align_tables docs/*.md +# :example +uv run --frozen python -m tests.hooks.align_tables docs/*.md """ +import re import sys import unicodedata def _display_width(text: str) -> int: - """Return the monospace display width of *text*. + """ + Return the monospace display width of *text*. Wide characters (most emoji, CJK) count as 2 columns. Zero-width characters (combining marks, variation selectors, ZWJ) count as 0. - A base character followed by VS16 (U+FE0F) is forced to width 2 (emoji presentation). + A base character followed by VS16 (U+FE0F) is forced to width 2 + (emoji presentation). """ width = 0 chars = list(text) @@ -28,7 +30,7 @@ def _display_width(text: str) -> int: if cat.startswith("M") or cat == "Cf": continue # check if next char is VS16 (emoji presentation selector) - has_vs16 = i + 1 < len(chars) and chars[i + 1] == "️" + has_vs16 = i + 1 < len(chars) and chars[i + 1] == "\ufe0f" eaw = unicodedata.east_asian_width(ch) if eaw in ("W", "F") or has_vs16: width += 2 @@ -42,17 +44,94 @@ def _pad(text: str, target_width: int) -> str: return text + " " * (target_width - _display_width(text)) -def align_table(lines): +_ESCAPED_PIPE_SENTINEL = "\x00" +_WIKILINK_PIPE_SENTINEL = "\x01" +_WIKILINK_RE = re.compile(r"\[\[([^\[\]]+?)\]\]") + + +def _mask_wikilink_pipes(text: str) -> str: + """Replace '|' inside [[...]] with a sentinel so it survives the table-row split.""" + return _WIKILINK_RE.sub( + lambda m: "[[" + m.group(1).replace("|", _WIKILINK_PIPE_SENTINEL) + "]]", + text, + ) + + +def _split_cells(line: str) -> list[str]: + r""" + Split a markdown table row on '|', preserving wikilink and escape pipes. + + Two forms are treated as literal pipes (not column separators): + - '\|' - the standard markdown escape for a literal pipe + - '|' inside an Obsidian wikilink '[[target|display]]' + + Both forms appear in this repo: display-text wikilinks in table cells (e.g. + '[[01 Primer|Primer]]') would otherwise be chopped in half. The '\|' escape + still works for symmetry and for non-wikilink literal pipes. + """ + body = line.strip().strip("|").replace(r"\|", _ESCAPED_PIPE_SENTINEL) + body = _mask_wikilink_pipes(body) + return [ + c.strip() + .replace(_WIKILINK_PIPE_SENTINEL, "|") + .replace(_ESCAPED_PIPE_SENTINEL, r"\|") + for c in body.split("|") + ] + + +_SEPARATOR_CELL_RE = re.compile(r"^:?-+:?$") + + +def _is_separator_row(cells: list[str]) -> bool: + """Return True if every cell is a markdown separator cell (`:?-+:?`).""" + return bool(cells) and all( + _SEPARATOR_CELL_RE.fullmatch(cell.strip()) for cell in cells + ) + + +def _alignment_marker(cell: str) -> tuple[bool, bool]: + """Return (left, right) colon alignment flags from a separator cell.""" + cell = cell.strip() + return cell.startswith(":"), cell.endswith(":") + + +def _separator_cell(width: int, left: bool, right: bool) -> str: + """Build a separator cell preserving alignment colons, min 1 dash.""" + # GFM requires at least one hyphen in a delimiter cell (colons don't count). + # Reserve room for any alignment colons, then fill the rest with dashes. + dashes = max(1, width - int(left) - int(right)) + return (":" if left else "") + "-" * dashes + (":" if right else "") + + +def align_table(lines: list[str]) -> list[str]: """Align all pipes in a markdown table.""" rows = [] for line in lines: - cells = [c.strip() for c in line.strip().strip("|").split("|")] - rows.append(cells) + rows.append(_split_cells(line)) if len(rows) < 2: return lines - num_cols = len(rows[0]) + # Preserve the leading indentation of the first row (tables nested in list + # items are indented); emitting bare "| ..." rows would de-indent them and + # change how the table renders. + first = lines[0] + indent = first[: len(first) - len(first.lstrip())] + + # Only treat this block as a table if row 2 is a real separator row. Two + # consecutive pipe-prefixed lines that aren't a table (or a table missing + # its separator) would otherwise get a data row rewritten into dashes. + if not _is_separator_row(rows[1]): + return lines + + num_cols = max(len(row) for row in rows) + + # capture per-column alignment markers from the separator row before padding + sep_row = rows[1] + markers = [ + _alignment_marker(sep_row[j]) if j < len(sep_row) else (False, False) + for j in range(num_cols) + ] # Find max display width per column (skip separator row) widths = [0] * num_cols @@ -60,27 +139,41 @@ def align_table(lines): if i == 1: continue for j, cell in enumerate(row): - if j < num_cols: - widths[j] = max(widths[j], _display_width(cell)) + widths[j] = max(widths[j], _display_width(cell)) + + # The separator cell needs at least one dash plus room for any alignment + # colons. If a column's widest data cell is narrower than that, the + # separator row would render wider than the data cells and knock the pipes + # out of alignment (MD060). Raise each column to its separator minimum. + for j in range(num_cols): + left, right = markers[j] + widths[j] = max(widths[j], 1 + int(left) + int(right)) # Rebuild rows with aligned pipes result = [] for i, row in enumerate(rows): if i == 1: - parts = ["| " + "-" * widths[j] + " " for j in range(num_cols)] + parts = [ + "| " + _separator_cell(widths[j], *markers[j]) + " " + for j in range(num_cols) + ] else: parts = [ "| " + _pad(row[j] if j < len(row) else "", widths[j]) + " " for j in range(num_cols) ] - result.append("".join(parts) + "|") + result.append(indent + "".join(parts) + "|") return result -def process_file(path): +def process_file(path: str) -> bool: """Process a single markdown file. Return True if changes were made.""" - with open(path) as f: - original = f.read() + try: + with open(path, encoding="utf-8") as f: + original = f.read() + except (OSError, UnicodeDecodeError) as e: + print(f"align-tables: cannot read {path}: {e}", file=sys.stderr) + raise SystemExit(1) from e lines = original.splitlines() result = [] @@ -90,7 +183,7 @@ def process_file(path): for line in lines: stripped = line.strip() - if stripped.startswith("```") or stripped.startswith("~~~"): + if stripped.startswith(("```", "~~~")): in_code_block = not in_code_block is_table = ( not in_code_block and stripped.startswith("|") and "|" in stripped[1:] @@ -110,8 +203,12 @@ def process_file(path): new_content = "\n".join(result) + "\n" if new_content != original: - with open(path, "w") as f: - f.write(new_content) + try: + with open(path, "w", encoding="utf-8", newline="\n") as f: + f.write(new_content) + except OSError as e: + print(f"align-tables: cannot write {path}: {e}", file=sys.stderr) + raise SystemExit(1) from e return True return False diff --git a/tests/hooks/gremlins.py b/tests/hooks/gremlins.py index 8940a0dc..22a28557 100644 --- a/tests/hooks/gremlins.py +++ b/tests/hooks/gremlins.py @@ -2,14 +2,23 @@ Scan staged text files for unwanted Unicode characters. Auto-fixes characters with obvious ASCII replacements (dashes, smart quotes, -fancy spaces, etc.) and reports any remaining unfixable gremlins. +fancy spaces, etc.). + +Markup files (.md, .html, .htm) get a relaxed pass: EM DASH, NO-BREAK SPACE, +HORIZONTAL ELLIPSIS, and MIDDLE DOT are allowed there because they serve +legitimate typographic purposes. Everything else with a safe ASCII +replacement (smart quotes, EN DASH, most zero-width/invisible chars, etc.) is +still auto-fixed. Bidirectional control characters (LRM/RLM, embeddings, +overrides, isolates) have no safe replacement and are reported as unfixable - +the hook exits non-zero so they get manual attention. The gremlins VSCode +extension handles visual reporting for the allowed chars. When auto-fixes are applied the hook prints what changed and exits 0; pre-commit detects the modified file and blocks the commit so the user can review, re-stage, and commit again. # :example -python3 -m tests.hooks.gremlins wsl/wsl_setup.ps1 +uv run --frozen python -m tests.hooks.gremlins docs/*.md """ import os @@ -41,14 +50,42 @@ "\u200d": "", # ZERO WIDTH JOINER "\u00ad": "", # SOFT HYPHEN "\u000c": "", # FORM FEED + # Miscellaneous + "\u00b7": "-", # MIDDLE DOT + # BOM + "\ufeff": "", # BYTE ORDER MARK (UTF-8 BOM) } -# Characters that cannot be auto-fixed -- always reported for manual review. -REPORT_ONLY: tuple[str, ...] = ( - "\u00b7", # MIDDLE DOT +# Characters that should be flagged but have no safe auto-replacement. +UNFIXABLE: frozenset[str] = frozenset( + { + "\u200e", # LEFT-TO-RIGHT MARK + "\u200f", # RIGHT-TO-LEFT MARK + "\u202a", # LEFT-TO-RIGHT EMBEDDING + "\u202b", # RIGHT-TO-LEFT EMBEDDING + "\u202c", # POP DIRECTIONAL FORMATTING + "\u202d", # LEFT-TO-RIGHT OVERRIDE + "\u202e", # RIGHT-TO-LEFT OVERRIDE + "\u2066", # LEFT-TO-RIGHT ISOLATE + "\u2067", # RIGHT-TO-LEFT ISOLATE + "\u2068", # FIRST STRONG ISOLATE + "\u2069", # POP DIRECTIONAL ISOLATE + } ) -ALL_FORBIDDEN = set(AUTO_FIX) | set(REPORT_ONLY) +ALL_FORBIDDEN: frozenset[str] = frozenset(AUTO_FIX) | UNFIXABLE + +# Markup files allow a small set of typographic chars that are intentional. +# Everything NOT in this set is still auto-fixed. +MARKUP_EXTENSIONS = frozenset((".md", ".html", ".htm")) +MARKUP_ALLOW = frozenset( + { + "\u2014", # EM DASH + "\u00a0", # NO-BREAK SPACE + "\u2026", # HORIZONTAL ELLIPSIS + "\u00b7", # MIDDLE DOT + } +) def _char_label(ch: str) -> str: @@ -70,62 +107,83 @@ def is_text_file(path: str) -> bool: return False -def fix_and_report(path: str) -> tuple[list[str], list[str]]: - """Auto-fix what we can, report what we cannot. +def _is_markup(path: str) -> bool: + _, ext = os.path.splitext(path) + return ext.lower() in MARKUP_EXTENSIONS - Returns (fixed_reports, unfixable_reports). - """ + +def fix_and_report(path: str) -> tuple[list[str], list[str]]: + """Auto-fix gremlins in a file. Returns (fixes, errors).""" try: - with open(path, encoding="utf-8", errors="replace") as fh: + # surrogateescape (not replace) so invalid UTF-8 bytes round-trip on the + # in-place write below instead of being flattened to U+FFFD and lost. + with open(path, encoding="utf-8", errors="surrogateescape") as fh: content = fh.read() except OSError: return [], [] + if not (ALL_FORBIDDEN & set(content)): + return [], [] + + markup = _is_markup(path) + fixed_chars: set[str] = set() new_content = content for ch, replacement in AUTO_FIX.items(): + if markup and ch in MARKUP_ALLOW: + continue if ch in new_content: fixed_chars.add(ch) new_content = new_content.replace(ch, replacement) + fixes: list[str] = [] if fixed_chars: - with open(path, "w", encoding="utf-8") as fh: - fh.write(new_content) - - fixed_reports = [] - if fixed_chars: + try: + with open( + path, "w", encoding="utf-8", errors="surrogateescape", newline="\n" + ) as fh: + fh.write(new_content) + except OSError as e: + print(f"gremlins: cannot write {path}: {e}", file=sys.stderr) + raise SystemExit(1) from e labels = ", ".join(sorted(_char_label(ch) for ch in fixed_chars)) - fixed_reports.append(f"{path}: fixed {labels}") + fixes.append(f"{path}: fixed {labels}") - unfixable_reports = [] - for lineno, line in enumerate(new_content.splitlines(), start=1): - for ch in REPORT_ONLY: - if ch in line: - unfixable_reports.append(f"{path}:{lineno}: contains {_char_label(ch)}") + errors: list[str] = [] + found_unfixable = UNFIXABLE & set(new_content) + if found_unfixable: + # Report each occurrence with its 1-based line/column so the offending + # character can be located for manual cleanup, grouped under the file. + for line_no, line in enumerate(new_content.splitlines(), start=1): + for col, ch in enumerate(line, start=1): + if ch in found_unfixable: + errors.append( + f"{path}:{line_no}:{col}: unfixable {_char_label(ch)}" + ) - return fixed_reports, unfixable_reports + return fixes, errors def check_gremlins(argv: Iterable[str]) -> int: - files = list(argv) + """Scan files for invisible/gremlin characters and auto-fix them.""" all_fixed: list[str] = [] - all_unfixable: list[str] = [] + all_errors: list[str] = [] - for path in files: + for path in argv: if not os.path.exists(path) or not is_text_file(path): continue - fixed, unfixable = fix_and_report(path) - all_fixed.extend(fixed) - all_unfixable.extend(unfixable) + fixes, errors = fix_and_report(path) + all_fixed.extend(fixes) + all_errors.extend(errors) if all_fixed: print("Gremlin characters auto-fixed:", file=sys.stderr) for r in all_fixed: print(f" {r}", file=sys.stderr) - if all_unfixable: - print("Gremlin characters found (manual fix required):", file=sys.stderr) - for r in all_unfixable: + if all_errors: + print("Unfixable gremlin characters found:", file=sys.stderr) + for r in all_errors: print(f" {r}", file=sys.stderr) return 1 diff --git a/vagrant/hyperv/arch/Vagrantfile b/vagrant/hyperv/arch/Vagrantfile index 85c78f99..127c5437 100644 --- a/vagrant/hyperv/arch/Vagrantfile +++ b/vagrant/hyperv/arch/Vagrantfile @@ -50,7 +50,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/hyperv/debian/Vagrantfile b/vagrant/hyperv/debian/Vagrantfile index 04204ea6..52712b03 100644 --- a/vagrant/hyperv/debian/Vagrantfile +++ b/vagrant/hyperv/debian/Vagrantfile @@ -58,7 +58,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/hyperv/fedora/Vagrantfile b/vagrant/hyperv/fedora/Vagrantfile index 7840b4a9..043b645b 100644 --- a/vagrant/hyperv/fedora/Vagrantfile +++ b/vagrant/hyperv/fedora/Vagrantfile @@ -48,7 +48,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/hyperv/ubuntu/Vagrantfile b/vagrant/hyperv/ubuntu/Vagrantfile index 4cb0acb1..d23f3cfd 100644 --- a/vagrant/hyperv/ubuntu/Vagrantfile +++ b/vagrant/hyperv/ubuntu/Vagrantfile @@ -57,7 +57,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/libvirt/alpine/Vagrantfile b/vagrant/libvirt/alpine/Vagrantfile index 51a3ec02..a4e55091 100644 --- a/vagrant/libvirt/alpine/Vagrantfile +++ b/vagrant/libvirt/alpine/Vagrantfile @@ -41,7 +41,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/libvirt/arch/Vagrantfile b/vagrant/libvirt/arch/Vagrantfile index 048254df..2d3db575 100644 --- a/vagrant/libvirt/arch/Vagrantfile +++ b/vagrant/libvirt/arch/Vagrantfile @@ -43,7 +43,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/libvirt/debian/Vagrantfile b/vagrant/libvirt/debian/Vagrantfile index 452590df..5ec43e16 100644 --- a/vagrant/libvirt/debian/Vagrantfile +++ b/vagrant/libvirt/debian/Vagrantfile @@ -35,7 +35,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/libvirt/fedora/Vagrantfile b/vagrant/libvirt/fedora/Vagrantfile index 8b52ce8d..03516695 100644 --- a/vagrant/libvirt/fedora/Vagrantfile +++ b/vagrant/libvirt/fedora/Vagrantfile @@ -43,7 +43,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/libvirt/opensuse/Vagrantfile b/vagrant/libvirt/opensuse/Vagrantfile index fa6bc1c8..5b1e6963 100644 --- a/vagrant/libvirt/opensuse/Vagrantfile +++ b/vagrant/libvirt/opensuse/Vagrantfile @@ -44,7 +44,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/libvirt/ubuntu/Vagrantfile b/vagrant/libvirt/ubuntu/Vagrantfile index 004a92aa..dbca8ef4 100644 --- a/vagrant/libvirt/ubuntu/Vagrantfile +++ b/vagrant/libvirt/ubuntu/Vagrantfile @@ -47,7 +47,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/virtualbox/fedora/Vagrantfile b/vagrant/virtualbox/fedora/Vagrantfile index 506ac8a7..21ef3afb 100644 --- a/vagrant/virtualbox/fedora/Vagrantfile +++ b/vagrant/virtualbox/fedora/Vagrantfile @@ -43,7 +43,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/vagrant/virtualbox/ubuntu/Vagrantfile b/vagrant/virtualbox/ubuntu/Vagrantfile index be1362fc..050faf03 100644 --- a/vagrant/virtualbox/ubuntu/Vagrantfile +++ b/vagrant/virtualbox/ubuntu/Vagrantfile @@ -49,7 +49,7 @@ SCRIPT script_install_psmodules = <<~SCRIPT cd ~/source/repos/szymonos/linux-setup-scripts || exit 0 mkdir -p ~/.local/share/powershell/Modules - for module in aliases-git do-common do-linux; do + for module in aliases-git do-common do-unix; do rm -rf ~/.local/share/powershell/Modules/$module cp -rf modules/$module ~/.local/share/powershell/Modules/ done diff --git a/wsl/wsl_certs_add.ps1 b/wsl/wsl_certs_add.ps1 index c7ee91c5..0e8ac567 100644 --- a/wsl/wsl_certs_add.ps1 +++ b/wsl/wsl_certs_add.ps1 @@ -121,7 +121,7 @@ process { # intercept certificates from all uris foreach ($uri in $Uris) { try { - Get-Certificate -Uri $Uri -BuildChain | Select-Object -Skip 1 | ForEach-Object { + Get-Certificate -Uri $Uri -PresentedChain | Select-Object -Skip 1 | ForEach-Object { $certSet.Add($_) | Out-Null } } catch [System.Management.Automation.MethodInvocationException] { diff --git a/wsl/wsl_setup.ps1 b/wsl/wsl_setup.ps1 index 4d602f18..c401e156 100644 --- a/wsl/wsl_setup.ps1 +++ b/wsl/wsl_setup.ps1 @@ -628,7 +628,7 @@ process { $allUsersCmd = 'mkdir -p /usr/local/share/powershell/Modules && rm -rf /usr/local/share/powershell/Modules/do-common && cp -rf modules/do-common /usr/local/share/powershell/Modules/' wsl.exe --distribution $Distro --user root --exec sh -c $allUsersCmd # instantiate psmodules generic lists - $modules = [System.Collections.Generic.SortedSet[String]]::new([string[]]@('aliases-git', 'do-linux')) + $modules = [System.Collections.Generic.SortedSet[String]]::new([string[]]@('aliases-git', 'do-unix')) # determine modules to install if ('az' -in $scopes) { $modules.Add('do-az') | Out-Null