From 54cf0f999026241c65a7d3447529a1094431660e Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Fri, 24 Jul 2026 14:06:29 +0200 Subject: [PATCH 1/4] Add per-repo autocommit opt-in for /close archiving Route step 10's commit+push prompt straight to archive-task.sh commit-push when a repo commits .claude/work-system-close-autocommit, skipping the y/n ask in repos where archiving-to-main is the norm. Off by default; the flag is an authorization scoped to exactly that one narrow, already-guarded action (pathspec commit, ff-only, never force-pushes). - archive-task.sh: new `autocommit get|set|unset` subcommand - close/SKILL.md: check the flag before asking, same result reporting either way - test_archive_task.py, docs, CHANGELOG, work-system 1.9.4 -> 1.10.0 --- .claude-plugin/marketplace.json | 2 +- .claude/knowledge/_index.md | 2 +- .../features/task-archiving-on-close.md | 33 ++++++- CHANGELOG.md | 3 + .../work-system/.claude-plugin/plugin.json | 2 +- plugins/work-system/README.md | 10 ++ plugins/work-system/scripts/archive-task.sh | 40 +++++++- .../work-system/scripts/test_archive_task.py | 93 +++++++++++++++++++ plugins/work-system/skills/close/SKILL.md | 37 ++++++-- 9 files changed, 205 insertions(+), 17 deletions(-) create mode 100644 plugins/work-system/scripts/test_archive_task.py diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 6325a55..a3d1e73 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -18,7 +18,7 @@ "name": "work-system", "source": "./plugins/work-system", "description": "Generic task and worktree workflow system for Claude Code. Manage tasks as markdown files, run them in isolated git worktrees with a choice of worker agent (Claude, codex, or grok), and track progress across the define/kickoff/continue/status/close lifecycle.", - "version": "1.9.4" + "version": "1.10.0" }, { "name": "pr-flow", diff --git a/.claude/knowledge/_index.md b/.claude/knowledge/_index.md index a13e7bd..7a05672 100644 --- a/.claude/knowledge/_index.md +++ b/.claude/knowledge/_index.md @@ -17,7 +17,7 @@ - `features/herdr-close-automation.md` — `/close` in herdr: cwd-tab teardown, plugin SessionEnd hook, the one TUI-exit primitive, detached self-exit onto idle - `features/herdr-tab-glyphs.md` — Task-state glyphs (`○ ● ◇ ◆ ✓`) + main-root `◉` on herdr tab labels: `states` mode in the self-contained renderer, sync-vs-`--cached` PR refresh per caller, exact-cwd rename rules, soft pr-flow shim - `features/kickoff-agent-selection.md` — `/kickoff` worker choice: single committed per-repo default (no global/fallback/ranking) else picker; `agent-registry.sh` as SoT; bounded model-aware grok probe (inconclusive→trust-auth); non-claude "document, don't fake" degradation; announce-not-prompt for external defaults -- `features/task-archiving-on-close.md` — `/close` archives (not deletes) the task file; adaptive commit + ff-push to main +- `features/task-archiving-on-close.md` — `/close` archives (not deletes) the task file; adaptive commit + ff-push to main; per-repo `.claude/work-system-close-autocommit` opt-in skips the ask - `features/swarm-backend-adapter.md` — 0.6.0 read+web posture: OS secret-jail (denylist, worktree-aware, git-config-safe), per-voice fail-closed degrade, `jail` verb, prompt egress guard + residual risks; plus verified codex/grok CLI facts (schema JSON, effort mapping, model-aware readiness) - `features/swarm-review-pipeline.md` — `/swarm:review` pipeline: skill↔Workflow wiring, family-consensus, 0.5.0 lens clusters + design-kind verify, `--fix`/`--loop` (deterministic close-out via `loop-closeout.py`), `--pr` publish via deterministic `pr-post.py` diff --git a/.claude/knowledge/features/task-archiving-on-close.md b/.claude/knowledge/features/task-archiving-on-close.md index cf94b0f..8273533 100644 --- a/.claude/knowledge/features/task-archiving-on-close.md +++ b/.claude/knowledge/features/task-archiving-on-close.md @@ -1,10 +1,10 @@ --- title: "Task Archiving on /close" createdAt: 2026-06-29 -updatedAt: 2026-07-12 +updatedAt: 2026-07-24 createdFrom: "PR #19" -updatedFrom: "session: 2026-07-12" -pluginVersion: 1.8.2 +updatedFrom: "session: 2026-07-24" +pluginVersion: 1.10.0 prime: false reindexedAt: 2026-07-12 --- @@ -82,3 +82,30 @@ logic belongs in a tested script, not SKILL.md prose). See also [skill-compositi non-recursive `tasks/*.md`, which already excludes `tasks/archive/`. The "never persistent `cd`" footgun the helper's explicit paths avoid is a rule — see `.claude/rules/cwd-safety.md`. + +## Per-repo autocommit opt-in (1.10.0) + +- **The commit+push ask is skippable, per repo.** In a repo where + archiving-to-main is the norm (dotfiles, this repo), the always-yes y/n prompt + is pure friction — observed repeatedly across close runs. A committed + `.claude/work-system-close-autocommit` flag (content `yes`/`true`) routes + `/close` straight to `commit-push`, no `AskUserQuestion`, still reporting the + result exactly as the manual path does. Mirrors the `.claude/work-system-agent` + default precedent: a committed per-repo file, not a settings-plugin entry + (Phase-1, not yet consumed elsewhere) and not local git config (wouldn't travel + with the repo). `archive-task.sh` grew an `autocommit get|set|unset` subcommand + as the single source of truth for the flag, mirroring the script-owns-the-logic + split the rest of this feature already follows. +- **The global-rule framing.** The user's standing rule is "ask before pushing to + the default branch." This flag is the durable per-repo **authorization** for + exactly ONE narrow action — the archive-metadata commit+push in step 10 — not a + general loosening of that rule. What makes it safe to grant is that + `commit-push`'s existing guards (above) already bound the blast radius: an + exact pathspec commit (never a blanket `tasks/` add), fast-forward only, never + a force-push, and a hard refusal (`unpushed-history`) when `main` carries other + unpushed commits. The opt-in only removes the prompt; it does not weaken any of + those guards. +- **Off by default, per-repo only.** No global default and no shipped fallback — + the same shape as the agent-default. A repo that wants this opts in explicitly + by committing the flag file; a global toggle was explicitly deferred as a + separate follow-up, not built speculatively. diff --git a/CHANGELOG.md b/CHANGELOG.md index 359b922..f965404 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,9 @@ entries are grouped per plugin, newest first. ## work-system +### 1.10.0 — 2026-07-24 +- `/close` step 10's commit+push prompt is now skippable per repo: a committed `.claude/work-system-close-autocommit` flag (mirrors the `.claude/work-system-agent` default precedent) routes straight to `archive-task.sh commit-push` — no `AskUserQuestion` — and reports the result exactly as the manual path does. Off by default; unset repos keep today's ask-once behavior. Per-repo only, no global default. `archive-task.sh` grew an `autocommit get|set|unset` subcommand as the single source of truth for the flag (get returns `enabled=yes` only for content `yes`/`true`). + ### 1.9.4 — 2026-07-24 - Add two more herdr tab-glyph refresh triggers: `/kickoff` (after the launch step, so the freshly-created tab is included) and `/define` (after the task file is written). Both call the same best-effort, silent `herdr-tab-glyph.sh refresh --cached ` that `/close`/`/list`/`/status` already use — outside herdr, or on failure, it's a silent no-op. Fixes the Manager tab's `◉` hub mark sometimes not appearing until an unrelated `/close`/`/list`/`/status` happened to run in a repo where you mostly `/kickoff`. diff --git a/plugins/work-system/.claude-plugin/plugin.json b/plugins/work-system/.claude-plugin/plugin.json index f501141..c3bf4b9 100644 --- a/plugins/work-system/.claude-plugin/plugin.json +++ b/plugins/work-system/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "work-system", "description": "Generic task and worktree workflow system for Claude Code. Manage tasks as markdown files, run them in isolated git worktrees with a choice of worker agent (Claude, codex, or grok), and track progress across the define/kickoff/continue/status/close lifecycle.", - "version": "1.9.4", + "version": "1.10.0", "author": { "name": "gering" }, diff --git a/plugins/work-system/README.md b/plugins/work-system/README.md index 52d812a..8599994 100644 --- a/plugins/work-system/README.md +++ b/plugins/work-system/README.md @@ -149,6 +149,16 @@ once to commit **and** fast-forward-push it to `main` (so local `main` never diverges; a failed push just leaves the commit local). `/list` shows the archived count in its summary. +**Skip the prompt with a per-repo opt-in.** In a repo where archiving-to-main is +the established norm, the commit+push ask is pure friction. Commit +`.claude/work-system-close-autocommit` (content `yes`) and `/close` skips the +prompt and commits+pushes the archive directly — still reporting the outcome, and +still bounded by the same safety guards (archive-scoped commit, fast-forward +only, never force-pushes, refuses when `main` has other unpushed commits). This +is the durable authorization for exactly that one narrow action, nothing else. +Set it with `archive-task.sh autocommit set ` (unset with `unset`, check +with `get`); off by default, and per-repo only — no global default. + ## Worker agent selection `/kickoff` doesn't hardcode Claude as the worktree worker. Each agent is a diff --git a/plugins/work-system/scripts/archive-task.sh b/plugins/work-system/scripts/archive-task.sh index df82809..85b994c 100755 --- a/plugins/work-system/scripts/archive-task.sh +++ b/plugins/work-system/scripts/archive-task.sh @@ -34,6 +34,17 @@ # pathspec commit, so unrelated pre-staged work is never swept in), and # fast-forward push to origin. Refuses if the main repo isn't on # . Never force-pushes. +# autocommit get +# Print `enabled=yes` when the repo opts into skipping /close's commit+push +# prompt (committed `.claude/work-system-close-autocommit`, content `yes` or +# `true`), else `enabled=no` (missing file, any other content, or no such +# repo). Per-repo only — no global default. +# autocommit set +# Write `yes` to the repo's `.claude/work-system-close-autocommit` (creating +# `.claude/` if needed). This is itself a tracked change to commit. +# autocommit unset +# Remove the repo's `.claude/work-system-close-autocommit`, reverting to the +# default ask-once behavior. # # `archive` output: key=value lines (paths relative to the main repo) — # archived_path=tasks/archive/[-N].md @@ -248,8 +259,35 @@ commit_push() { fi } +autocommit() { + local op="${1:-}" repo="${2:-}" + local file="$repo/.claude/work-system-close-autocommit" + case "$op" in + get) + [ -n "$repo" ] || { echo "usage: ${0##*/} autocommit get " >&2; exit 2; } + local enabled="no" + if [ -f "$file" ]; then + local content; content="$(tr -d '[:space:]' < "$file" 2>/dev/null || true)" + case "$content" in yes|true) enabled="yes" ;; esac + fi + printf 'enabled=%s\n' "$enabled" + ;; + set) + [ -n "$repo" ] || { echo "usage: ${0##*/} autocommit set " >&2; exit 2; } + mkdir -p "$repo/.claude" + printf 'yes\n' > "$file" + ;; + unset) + [ -n "$repo" ] || { echo "usage: ${0##*/} autocommit unset " >&2; exit 2; } + rm -f "$file" + ;; + *) echo "usage: ${0##*/} autocommit {get|set|unset} " >&2; exit 2 ;; + esac +} + case "${1:-}" in archive) shift; archive "$@" ;; commit-push) shift; commit_push "$@" ;; - *) echo "usage: ${0##*/} {archive [--pr ] [--sha ] | commit-push }" >&2; exit 2 ;; + autocommit) shift; autocommit "$@" ;; + *) echo "usage: ${0##*/} {archive [--pr ] [--sha ] | commit-push | autocommit {get|set|unset} }" >&2; exit 2 ;; esac diff --git a/plugins/work-system/scripts/test_archive_task.py b/plugins/work-system/scripts/test_archive_task.py new file mode 100644 index 0000000..4608a37 --- /dev/null +++ b/plugins/work-system/scripts/test_archive_task.py @@ -0,0 +1,93 @@ +#!/usr/bin/env python3 +"""Tests for archive-task.sh's `autocommit` subcommand — run standalone +(`python3 test_archive_task.py`) or via scripts/check-structure.py's +"plugin tests" check. + +Scope: just the flag-routing logic behind /close step 10's opt-in +(`.claude/work-system-close-autocommit`) — the part that is actually +script-testable. The `archive`/`commit-push` subcommands are exercised +manually via /close; they need a real git repo and are covered by the +design rationale in `.claude/knowledge/features/task-archiving-on-close.md`. +""" +import subprocess +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).parent +SCRIPT = HERE / "archive-task.sh" + +FAILS = [] + + +def check(name, cond): + if not cond: + FAILS.append(name) + + +def run(*args): + return subprocess.run( + ["bash", str(SCRIPT), *args], capture_output=True, text=True + ) + + +def flag_path(repo): + return repo / ".claude" / "work-system-close-autocommit" + + +with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) / "repo" + repo.mkdir() + + # --- get: no flag file -> disabled by default ------------------------- # + r = run("autocommit", "get", str(repo)) + check("no flag file -> enabled=no", r.stdout.strip() == "enabled=no") + check("get exits 0", r.returncode == 0) + + # --- set: writes the file, creates .claude/ as needed ------------------ # + r = run("autocommit", "set", str(repo)) + check("set exits 0", r.returncode == 0) + check("set creates the flag file", flag_path(repo).is_file()) + check("set writes 'yes'", flag_path(repo).read_text().strip() == "yes") + + r = run("autocommit", "get", str(repo)) + check("after set -> enabled=yes", r.stdout.strip() == "enabled=yes") + + # --- unset: removes the file, reverts to disabled ----------------------- # + r = run("autocommit", "unset", str(repo)) + check("unset exits 0", r.returncode == 0) + check("unset removes the flag file", not flag_path(repo).exists()) + check("after unset -> enabled=no", run("autocommit", "get", str(repo)).stdout.strip() == "enabled=no") + + # --- content variants: only yes/true (whitespace-trimmed) count -------- # + flag_path(repo).parent.mkdir(parents=True, exist_ok=True) + for content, expected in [ + ("yes\n", "yes"), + ("true\n", "yes"), + (" yes \n", "yes"), + ("no\n", "no"), + ("1\n", "no"), + ("", "no"), + ]: + flag_path(repo).write_text(content) + got = run("autocommit", "get", str(repo)).stdout.strip() + check(f"content {content!r} -> enabled={expected}", got == f"enabled={expected}") + + # --- a repo that never had .claude/ at all ------------------------------ # + bare = Path(tmp) / "bare" + bare.mkdir() + check("no repo dir at all -> enabled=no", run("autocommit", "get", str(bare)).stdout.strip() == "enabled=no") + + # --- usage errors -------------------------------------------------------- # + check("get missing repo -> exit 2", run("autocommit", "get").returncode == 2) + check("set missing repo -> exit 2", run("autocommit", "set").returncode == 2) + check("unknown op -> exit 2", run("autocommit", "bogus", str(repo)).returncode == 2) + check("bare autocommit -> exit 2", run("autocommit").returncode == 2) + + +if FAILS: + print("FAIL:") + for f in FAILS: + print(" -", f) + sys.exit(1) +print("archive-task.sh autocommit: all tests passed") diff --git a/plugins/work-system/skills/close/SKILL.md b/plugins/work-system/skills/close/SKILL.md index 1294988..b00712f 100644 --- a/plugins/work-system/skills/close/SKILL.md +++ b/plugins/work-system/skills/close/SKILL.md @@ -172,19 +172,36 @@ Rules: at `tasks/.md`"; the rest of the cleanup (worktree/branch) already happened. - Read the helper's `key=value` output (`archived_path`, `collision`, `committable`): - **`committable=yes`** (there is a git change to commit — the archive isn't gitignored, - or the original task file was tracked so its removal needs recording): show - `git -C status --short tasks/archive/ tasks/.md` (scoped — not - the whole `tasks/`, which would surface unrelated pending tasks) and ask **once**: - "Commit the archived task file to `` and push? [y/n]" — one approval covers - both (the archive is metadata, and the push is what keeps local `` from - diverging and breaking the next `/close`'s step-5 sync). If yes, delegate the whole - stage→commit→fast-forward-push to the helper (all the git-stateful steps live there, not - here, so they can't drift; step 5 already synced local ``, so its commit is - a clean ff for origin — it never force-pushes): + or the original task file was tracked so its removal needs recording): check the repo's + opt-in flag first (per-repo only, off by default — never a global setting): + ```sh + bash "${CLAUDE_PLUGIN_ROOT}/scripts/archive-task.sh" autocommit get + ``` + - **`enabled=yes`** — the repo committed `.claude/work-system-close-autocommit`. That + file IS the durable per-repo authorization for exactly this one narrow, contained + action (archive-scoped pathspec commit, fast-forward only, never force-pushes, and it + refuses to push on `unpushed-history` — see the design rationale in + `.claude/knowledge/features/task-archiving-on-close.md`). Skip the AskUserQuestion below + and go straight to the commit+push. Print one short transparency line first — never + silent about an auto action: "Auto-committing archived task file to `` + (`.claude/work-system-close-autocommit` is set)…". + - **`enabled=no`** (the default, unchanged behavior) — show + `git -C status --short tasks/archive/ tasks/.md` (scoped — + not the whole `tasks/`, which would surface unrelated pending tasks) and ask **once**: + "Commit the archived task file to `` and push? [y/n]" — one approval covers + both (the archive is metadata, and the push is what keeps local `` from + diverging and breaking the next `/close`'s step-5 sync). Only continue below on yes. + + Either way (auto-enabled or approved), delegate the whole stage→commit→fast-forward-push + to the helper (all the git-stateful steps live there, not here, so they can't drift; step + 5 already synced local ``, so its commit is a clean ff for origin — it never + force-pushes): ```sh bash "${CLAUDE_PLUGIN_ROOT}/scripts/archive-task.sh" commit-push ``` - Report from its `result=` (and `archive_committed=` on the committed-* results — when + Report from its `result=` exactly the same way regardless of which entry path got here — + never claim "committed and pushed" unless `result=committed-pushed` (and + `archive_committed=` on the committed-* results — when `no`, the archive is gitignored and the commit recorded only the source file's removal, so say "task file removal committed; archive kept local (gitignored)" rather than claiming the archive itself was committed): From 22950a551304ef0b16172617e4c43003451c5082 Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Mon, 27 Jul 2026 16:55:23 +0200 Subject: [PATCH 2/4] Harden the /close autocommit opt-in after swarm review Three review rounds against the initial cut found the flag's trust model too weak and the write path unsafe. Presence no longer authorizes anything: the value is read from the committed object on , so neither a file a tool merely wrote nor a working-tree edit hidden by --assume-unchanged/--skip-worktree can waive /close's push prompt, and the verdict no longer depends on which branch the checkout sits on. - get: read from :; bare enabled=no when nothing is configured, else reason= + a ready-to-print note= (callers relay it instead of restating the vocabulary); locally-disabled is reported neutrally rather than told to revert itself - set/unset: delegate resolution to main-repo-path.sh and cross-check it is a real work tree; refuse a symlinked or wrong-typed flag/.claude and re-validate after mkdir; mktemp+rename instead of a PID-derived temp path; warn when .claude/ is gitignored; remind to commit the deletion - commit-push: :(literal) pathspecs so a task named `x*` cannot sweep in other archives (not for check-ignore, which rejects pathspec magic); archive() uses mktemp too - close/SKILL.md: pass , show the scoped status preview on the auto path, fail closed on any non-enabled=yes output - docs: state what the guard is and is NOT worth (it raises the bar to "a commit", not "human-reviewed"); CLAUDE.md marker -> v1.10.x Scope stated honestly in the knowledge entry: agent-registry.sh still open-codes the old resolver, and separate-git-dir layouts are refused rather than supported. --- .../features/task-archiving-on-close.md | 85 +++- CHANGELOG.md | 6 +- CLAUDE.md | 2 +- plugins/work-system/README.md | 22 +- plugins/work-system/scripts/archive-task.sh | 411 +++++++++++++++-- .../work-system/scripts/test_archive_task.py | 417 ++++++++++++++++-- plugins/work-system/skills/close/SKILL.md | 38 +- 7 files changed, 897 insertions(+), 84 deletions(-) diff --git a/.claude/knowledge/features/task-archiving-on-close.md b/.claude/knowledge/features/task-archiving-on-close.md index 8273533..45f3b4d 100644 --- a/.claude/knowledge/features/task-archiving-on-close.md +++ b/.claude/knowledge/features/task-archiving-on-close.md @@ -43,7 +43,9 @@ logic belongs in a tested script, not SKILL.md prose). See also [skill-compositi - **All git-stateful work is in `commit-push`, gated by `/close`.** Honoring the never-commit-without-approval rule, `archive-task.sh archive` only moves the file + appends the index. When `committable=yes`, `/close` asks **once** ("commit and - push?") and then delegates the entire stage→commit→push to `archive-task.sh + push?") — **unless the repo carries the committed per-repo autocommit opt-in, which + replaces that per-close ask with a durable authorization (see the 1.10.0 section + below)** — and then delegates the entire stage→commit→push to `archive-task.sh commit-push` — kept out of SKILL.md prose so the multi-step git logic can't drift (the project's prose-drift lesson). Two correctness guards earned in review: it commits with an explicit **pathspec** (`git commit -- @@ -109,3 +111,84 @@ persistent `cd`" footgun the helper's explicit paths avoid is a rule — see the same shape as the agent-default. A repo that wants this opts in explicitly by committing the flag file; a global toggle was explicitly deferred as a separate follow-up, not built speculatively. +- **Presence is not authorization; the committed value is (swarm-review).** The + first cut honored the flag if the file merely *existed* — too weak for a flag + whose job is waiving an approval gate for a push to the default branch: any + tool, or a prompt-injected worker agent in a task worktree, can write a file + with nobody approving a push. So `get` reads the value from the **committed + object** (`git show HEAD:`), never from the working tree. That one call is + both the tracked-check and the value read, and it is immune to the working-tree + tricks that fool a diff-based guard (`git update-index --assume-unchanged` / + `--skip-worktree` make `ls-files`/`diff --quiet` report a dirty file as clean — + round 2 of the review reproduced that bypass against the diff-based version). + A dirty flag additionally falls back to asking, so a local edit can still + *disable* the opt-in deliberately. Every unresolved case (not a git repo, + symlinked flag or `.claude`, unreadable git state) reports `enabled=no`, which + merely restores the prompt. +- **What the guard is worth — stated honestly, because the docs are the claim.** + It stops a flag that was merely *written*. It does **not** stop an actor who + can already commit in your repo: they can commit the flag like any other + change, and an agent-authored flag merged through a normal PR is indistinguishable + from a human one. The guard raises the bar from "any file write" to "a commit", + not to "reviewed by a human". What actually bounds the damage is `commit-push` + itself — archive-scoped pathspec, fast-forward only, never a force-push, refusal + on `unpushed-history` — so the worst case is inert archived markdown reaching + `main` unreviewed. **Accepted residual:** requiring the flag's commit to be an + ancestor of `origin/` would make "reviewed" true, but it breaks + origin-less repos, depends on freshly-fetched refs, and is disproportionate to + that blast radius. Revisit only if the flag ever authorizes more than this. +- **Resolution delegates to `main-repo-path.sh`** — the plugin's one resolver, and + the same helper that produces the `` `/close` passes in. Sharing + it is the point: an independent second resolver could disagree with `/close`'s, + so `set` would write a flag `get` never reads. An earlier cut open-coded a + `--git-common-dir` + `basename == .git` heuristic (copied from + `agent-registry.sh`) and broke on separate-git-dir/submodule layouts. The + resolver's answer is then **cross-checked** (`--is-inside-work-tree` + + `--show-toplevel`): under `git init --separate-git-dir`, `git worktree list` + reports the *git dir* as the first worktree (verified), and without the check + `set` cheerfully created `.claude/` inside `.git` — a flag that can never be + committed, reported as success. Now such a layout is refused with a diagnostic + naming it. **Caveat to a claim made earlier here:** `agent-registry.sh` still + uses the old heuristic, so the two per-repo files do *not* share one resolver + yet — migrating it is follow-up work, deliberately out of this task's scope + (it sits on the `/kickoff` hot path with its own tests). +- **Sharp edges the script owns** (the accepted-token list, `reason=` codes and + their meanings live in `archive-task.sh`'s header — do not restate them here or + in the README; that vocabulary drifted across five surfaces once already): + - Trim, *not* `tr -d '[:space:]'` — deleting all whitespace collapses `y e s` + into an accepted token, waiving the gate on undocumented content. + - `set`/`unset` refuse a symlinked flag **or `.claude` parent**, and re-validate + after `mkdir -p` — guarding only the leaf let a symlinked `.claude` make + `mkdir` succeed through the link and land the write outside the repo while the + reported path still read in-repo. Residual: a shell can't do this race-free + (no `openat`/`O_NOFOLLOW`); the re-checks narrow the window, they don't close it. + - `set` writes a `mktemp`'d file **in the target directory**, `chmod`s it *before* + the rename, and renames. A fixed `$file.tmp.$$` is PID-predictable (pre-plant + it as a symlink → the redirect clobbers an arbitrary file and `mv` installs the + symlink *as* the flag), and a post-rename `chmod` can follow a swapped-in link. + - `set` warns when `.claude/` is **gitignored** — otherwise `git add` refuses the + path and `get` reports `untracked` forever while `set` claims success. + - `unset` reminds you to commit the *deletion*: removing the file makes `get` + report disabled at once, but HEAD still carries the flag for every other clone. + - The dirty-check compares **blob hashes**, not `git diff --quiet`, so the index + bits (`--assume-unchanged`/`--skip-worktree`) can't hide a deliberate local + disable either. + - The verdict is read from **``**, not `HEAD` — the flag authorizes + a commit onto that branch and `commit_push` refuses any other, so reading HEAD + made the answer depend on which branch the main checkout happened to sit on + (a flag on a feature branch announced an auto-commit that then bailed out). + - `commit_push` passes `:(literal)` pathspecs: a task named `x*` would otherwise + glob `tasks/archive/x*.md` and commit every matching archive. **Not** for + `check-ignore`, which rejects pathspec magic outright — prefixing it there + silently broke the gitignored-archive detection (caught by a smoke test, not + the suite). + - A repo that never opted in must answer a **bare** `enabled=no`: callers relay + any `note=` verbatim, so ordering the symlink check before the existence check + made every close in a symlinked-`.claude` repo warn about an opt-in nobody + configured. + - Sibling helpers are addressed via an absolute `SCRIPT_DIR` captured at load: + a `$(dirname "$0")` expanded inside `( cd "$repo" && … )` resolves against the + *target* repo when `$0` is relative — and runs whatever sits there. +- **`get` ships the human sentence with the machine code.** Alongside `reason=` it + emits a ready-to-print `note=`, and callers relay that instead of re-spelling the + vocabulary — the prose-drift failure this project already recorded. diff --git a/CHANGELOG.md b/CHANGELOG.md index f965404..0cbe1a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,11 @@ entries are grouped per plugin, newest first. ## work-system ### 1.10.0 — 2026-07-24 -- `/close` step 10's commit+push prompt is now skippable per repo: a committed `.claude/work-system-close-autocommit` flag (mirrors the `.claude/work-system-agent` default precedent) routes straight to `archive-task.sh commit-push` — no `AskUserQuestion` — and reports the result exactly as the manual path does. Off by default; unset repos keep today's ask-once behavior. Per-repo only, no global default. `archive-task.sh` grew an `autocommit get|set|unset` subcommand as the single source of truth for the flag (get returns `enabled=yes` only for content `yes`/`true`). +- `/close` step 10's commit+push prompt is now skippable per repo: a committed `.claude/work-system-close-autocommit` flag (mirrors the `.claude/work-system-agent` default precedent) routes straight to `archive-task.sh commit-push` — no `AskUserQuestion` — and reports the result exactly as the manual path does. Off by default; unset repos keep today's ask-once behavior. Per-repo only, no global default. `archive-task.sh` grew an `autocommit get|set|unset` subcommand as the single source of truth for the flag. +- The flag is honored **only once committed**: `get` reads the value from the committed object (`git show HEAD:`), never from the working tree, so a file a tool or a worktree agent merely wrote cannot waive the prompt — and neither can a working-tree edit hidden behind `git update-index --assume-unchanged`/`--skip-worktree`, which fools a diff-based guard. A locally edited flag still falls back to asking, so deliberate local disabling works. Scope stated honestly: this raises the bar from "any file write" to "a commit", not to "human-reviewed"; `commit-push`'s own guards (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed history) are what bound the damage. +- The verdict is read from **``**, not `HEAD`: the flag authorizes a commit onto that branch (`commit-push` refuses any other), so a flag living only on some feature branch no longer announces an auto-commit that then fails. A repo that never opted in answers a bare `enabled=no`; every other non-honored case adds a machine `reason=` and a ready-to-print `note=` that callers relay verbatim instead of duplicating the vocabulary in prose. Accepted content is `yes`/`true`/`1`/`on` (case-insensitive, trimmed, single line); `no`/`false`/`0`/`off` disables, and a deliberate local disable is reported neutrally rather than being told to revert itself. `/close` treats **anything that is not literally `enabled=yes`** — including empty output from an older installed plugin without the subcommand — as "ask", so the check can never fail open, and it prints the same scoped `git status` preview on the auto path: standing authorization is not authorization to act unseen. +- `commit-push` now passes its paths as `:(literal)` pathspecs, so a task name containing glob characters (`x*`) can no longer sweep other archived files into the commit. Both writers in the script use `mktemp` rather than a PID-derived temp name. +- Path handling: resolution delegates to `main-repo-path.sh` (the same resolver `/close` uses, so `set` can't write where `get` won't look); `set`/`unset` refuse a symlinked flag **or `.claude` parent** and verify `.claude` resolves inside the repo; `set` writes via an exclusively-created `mktemp` file in the target directory instead of a PID-predictable `.tmp.$$` path, and echoes the resolved `flag=`. ### 1.9.4 — 2026-07-24 - Add two more herdr tab-glyph refresh triggers: `/kickoff` (after the launch step, so the freshly-created tab is included) and `/define` (after the task file is written). Both call the same best-effort, silent `herdr-tab-glyph.sh refresh --cached ` that `/close`/`/list`/`/status` already use — outside herdr, or on failure, it's a silent no-op. Fixes the Manager tab's `◉` hub mark sometimes not appearing until an unrelated `/close`/`/list`/`/status` happened to run in a repo where you mostly `/kickoff`. diff --git a/CLAUDE.md b/CLAUDE.md index 7a80021..bc42c91 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,7 +19,7 @@ This is a **Claude Code plugin marketplace** (monorepo) containing plugins that ## Current Plugins - **knowledge-system** (v1.9.x) — Knowledge management with three layers: Rules, Knowledge, Memory. Skills: `/init`, `/query`, `/curate`, `/reindex`, `/backfill-knowledge`, `/migrate`, `/statusline` -- **work-system** (v1.8.x) — Task and worktree workflow. Skills: `/define`, `/kickoff`, `/adopt`, `/continue`, `/status`, `/close`, `/list`, `/statusline` +- **work-system** (v1.10.x) — Task and worktree workflow. Skills: `/define`, `/kickoff`, `/adopt`, `/continue`, `/status`, `/close`, `/list`, `/statusline` - **pr-flow** (v1.3.x) — PR review feedback loop. Skills: `/open`, `/cycle`, `/check`, `/fix`, `/rebase`, `/merge` - **swarm** (v0.6.x) — Local mixture-of-agents code review (external `codex`/`grok` CLIs — grok-4.5 — plus Claude lenses: 11 in 4 clusters). Externals get file-read + web research under an OS secret-jail. P2: `/swarm:review` pipeline (scope→fan-out→merge→verify); P5: `--fix`/`--loop` apply the findings you agreed with. Skills: `/swarm:review`, `/swarm:agents` - **settings** (v0.1.x) — Per-plugin TOML config resolved over schema defaults; each plugin owns its `schema/settings.schema.json`. Skill: `/settings` (list/show/get/set/validate). Phase 1: config surface only. diff --git a/plugins/work-system/README.md b/plugins/work-system/README.md index 8599994..9d24dbe 100644 --- a/plugins/work-system/README.md +++ b/plugins/work-system/README.md @@ -154,10 +154,24 @@ the established norm, the commit+push ask is pure friction. Commit `.claude/work-system-close-autocommit` (content `yes`) and `/close` skips the prompt and commits+pushes the archive directly — still reporting the outcome, and still bounded by the same safety guards (archive-scoped commit, fast-forward -only, never force-pushes, refuses when `main` has other unpushed commits). This -is the durable authorization for exactly that one narrow action, nothing else. -Set it with `archive-task.sh autocommit set ` (unset with `unset`, check -with `get`); off by default, and per-repo only — no global default. +only, never force-pushes, refuses when `main` has other unpushed commits). + +Set it with `archive-task.sh autocommit set ` (`unset` to disable, `get` to +check — that subcommand's header documents the exact accepted values and status +codes). Off by default, per-repo only — no global default. The flag is honored +**only once it is committed**: the value is read from the committed version, not +the working tree, so a file that was merely written — by a tool, or by an agent +working in a worktree — never authorizes the skip. Anything unresolved reports +`enabled=no`, you simply get the prompt back, and the tool tells you why. Paths +resolve to the main checkout, so `set` from inside a worktree still writes where +`/close` reads. + +Be clear on what that buys you: it raises the bar from *"anything can write a +file"* to *"something had to commit"* — **not** proof a human reviewed it, since +whoever can commit in your repo can commit this too. What bounds the damage is +the commit+push itself: archive-scoped, fast-forward only, never a force-push, +refused when `main` carries other unpushed commits — so the worst case is inert +archived markdown reaching `main` unreviewed. ## Worker agent selection diff --git a/plugins/work-system/scripts/archive-task.sh b/plugins/work-system/scripts/archive-task.sh index 85b994c..2101ae6 100755 --- a/plugins/work-system/scripts/archive-task.sh +++ b/plugins/work-system/scripts/archive-task.sh @@ -9,9 +9,16 @@ # # Committability is adaptive and needs no .gitignore surgery: the archive simply # inherits whatever tasks/ does. If tasks/ is gitignored the archived file is -# ignored too (local-only); otherwise the move is a committable change. The -# script never commits *without /close's approval*: `archive` only moves+records -# and reports `committable`; `commit-push` runs only after the y/n gate. +# ignored too (local-only); otherwise the move is a committable change. +# +# `archive` NEVER commits: it only moves+records and reports `committable`. All +# git-stateful work is in `commit-push`, which /close runs after the y/n gate — +# OR, without asking, when the repo carries the committed per-repo opt-in flag +# `.claude/work-system-close-autocommit` (see the `autocommit` subcommand). That +# flag is the durable authorization for exactly this one narrow action; it is +# honored only when tracked + unmodified, and it changes nothing about +# commit-push's own guards (archive-scoped pathspec, ff-only, never force-push, +# refuses on unpushed history). # # CWD-safe: every path is explicit, the script never `cd`s (see cwd-safety rule). # @@ -34,17 +41,44 @@ # pathspec commit, so unrelated pre-staged work is never swept in), and # fast-forward push to origin. Refuses if the main repo isn't on # . Never force-pushes. -# autocommit get -# Print `enabled=yes` when the repo opts into skipping /close's commit+push -# prompt (committed `.claude/work-system-close-autocommit`, content `yes` or -# `true`), else `enabled=no` (missing file, any other content, or no such -# repo). Per-repo only — no global default. +# autocommit get [] +# Print `enabled=yes` iff the repo opts into skipping /close's commit+push +# prompt, else `enabled=no`. FAIL-SAFE: anything unresolved or suspicious +# reports `no`, which merely keeps the prompt. Pass : the flag +# authorizes a commit onto THAT branch (commit-push refuses any other), so +# it is read from that ref — reading HEAD would make the verdict depend on +# whichever branch the main checkout is parked on. Falls back to HEAD when +# omitted. The value is read from the COMMITTED object, never the worktree — +# presence alone is not authorization, and a working-tree edit can never +# silently enable the waiver (not even via `git update-index +# --assume-unchanged` / `--skip-worktree`, which fool a diff-based guard). +# What this does and does NOT buy you: it stops a flag that was merely +# WRITTEN (stray file, tool, an agent's file write); it does NOT stop an +# actor who can already commit in the repo. commit-push's own guards +# (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed +# history) are what bound the damage in that case. +# A repo that never opted in answers a BARE `enabled=no`. Otherwise two +# extra lines say why it was not honored: `reason=` one of +# symlink | not-a-file | untracked | modified | locally-disabled | +# unrecognized-content | git-error | unsupported-layout | not-a-repo, plus +# `note=` — callers relay the note rather than +# re-spelling this vocabulary in their own prose (it drifts). +# Accepted content (case-insensitive, leading/trailing whitespace trimmed, +# single line): yes/true/1/on → on; no/false/0/off → off. +# Per-repo only — no global default. # autocommit set -# Write `yes` to the repo's `.claude/work-system-close-autocommit` (creating -# `.claude/` if needed). This is itself a tracked change to commit. +# Write `yes` to the repo's `.claude/work-system-close-autocommit` via an +# exclusively-created mktemp file + rename (no PID-predictable temp path to +# pre-plant), refusing when the flag OR its `.claude` parent is a symlink or +# resolves outside the repo. Echoes `flag=` plus a reminder that it +# takes effect only once COMMITTED. # autocommit unset -# Remove the repo's `.claude/work-system-close-autocommit`, reverting to the -# default ask-once behavior. +# Remove the repo's `.claude/work-system-close-autocommit` (same path +# guards), reverting to the default ask-once behavior. Echoes `flag=`. +# All three resolve to the MAIN checkout by delegating to +# main-repo-path.sh — the same resolver /close uses, so `set` can never write +# where `get` doesn't look. A non-git path is a usage error for set/unset and a +# plain `enabled=no` for get. # # `archive` output: key=value lines (paths relative to the main repo) — # archived_path=tasks/archive/[-N].md @@ -60,6 +94,13 @@ # Exit 0 on success; 2 on a usage error; 3 when the task file does not exist. set -eu +# Absolute directory of THIS script, resolved once, before anything can change +# the working directory. Sibling helpers must be addressed through it: a +# `$(dirname "$0")` expanded inside a `( cd && … )` subshell would +# resolve against that other repo when $0 is relative — looking up (and running) +# whatever happens to sit at that path there. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + archive() { local repo="${1:-}" name="${2:-}" branch="${3:-}" if [ -z "$repo" ] || [ -z "$name" ] || [ -z "$branch" ]; then @@ -122,12 +163,23 @@ archive() { # interrupted write (disk full / signal) never leaves a truncated archive the # collision loop would later orphan as a real-looking file. On a write failure # the temp is removed and we abort with the source still intact. - local tmp="$dest.tmp.$$" + # + # mktemp, not a `$dest.tmp.$$` name: a PID-derived path is predictable, so + # anything able to create files in tasks/archive/ could pre-plant it as a + # symlink and have this redirect write through it. (Same reasoning as the + # autocommit writer below — keep the two consistent.) + local tmp + tmp="$(mktemp "$tasks_dir/archive/.archive.XXXXXX" 2>/dev/null || true)" + if [ -z "$tmp" ]; then + echo "failed to create a temp file in $tasks_dir/archive" >&2 + exit 1 + fi if ! { printf '%s\n\n' "$stamp"; cat "$src"; } > "$tmp"; then rm -f "$tmp" 2>/dev/null || true echo "failed to write archive $dest" >&2 exit 1 fi + chmod 644 "$tmp" 2>/dev/null || true # mktemp creates 600; the archive is repo content mv "$tmp" "$dest" # Remove the original BEFORE recording the index, rolling the moved archive back @@ -194,12 +246,25 @@ commit_push() { # would sweep in unrelated pending task files): the archived file and _index.md # only when not gitignored (`git add` errors on an ignored path), and the # original's removal only when it was tracked. + # + # The MUTATING commands get the `:(literal)` pathspec prefix. git otherwise + # reads these as GLOBS, and the task name flows in from the task file: a task + # named `x*` yields `tasks/archive/x*.md`, which would match — and commit — + # every archived file with that prefix. Literal magic makes the pathspec mean + # exactly the one file it names. (`lit()` keeps the raw path for comparisons.) + # + # NOT for `check-ignore`, which rejects pathspec magic outright ("pathspec + # magic not supported by this command") — prefixing it there made every probe + # fail, so `archive_ignored` was stuck at `no` and a gitignored archive would + # have hit `git add`'s error path instead. It is a read-only probe, so a glob + # there cannot commit anything. + lit() { printf ':(literal)%s' "$1"; } local archive_ignored=no - if git -C "$repo" check-ignore -q "$archived"; then archive_ignored=yes; fi + if git -C "$repo" check-ignore -q -- "$archived"; then archive_ignored=yes; fi local paths=() if [ "$archive_ignored" = no ]; then paths+=("$archived"); fi - git -C "$repo" check-ignore -q "tasks/archive/_index.md" || paths+=("tasks/archive/_index.md") - if git -C "$repo" ls-files --error-unmatch -- "$src_rel" >/dev/null 2>&1; then paths+=("$src_rel"); fi + git -C "$repo" check-ignore -q -- "tasks/archive/_index.md" || paths+=("tasks/archive/_index.md") + if git -C "$repo" ls-files --error-unmatch -- "$(lit "$src_rel")" >/dev/null 2>&1; then paths+=("$src_rel"); fi [ "${#paths[@]}" -eq 0 ] && { echo "result=nothing-to-commit"; return 0; } # Stage each path INDEPENDENTLY: `git add -A a b` aborts wholesale on one @@ -209,9 +274,9 @@ commit_push() { # in, and a bad/empty element is simply absent from the commit. local p staged=() archive_committed=no for p in "${paths[@]}"; do - git -C "$repo" add -A -- "$p" 2>/dev/null || true - if ! git -C "$repo" diff --cached --quiet -- "$p"; then - staged+=("$p") + git -C "$repo" add -A -- "$(lit "$p")" 2>/dev/null || true + if ! git -C "$repo" diff --cached --quiet -- "$(lit "$p")"; then + staged+=("$(lit "$p")") if [ "$p" = "$archived" ]; then archive_committed=yes; fi fi done @@ -259,29 +324,311 @@ commit_push() { fi } +# Resolve to the MAIN checkout root: the opt-in flag must live in the main +# worktree, not in a disposable linked worktree that /close later removes (a flag +# written there would never be read and would vanish with the worktree). +# +# DELEGATES to main-repo-path.sh — the plugin's one main-worktree resolver, and +# the very helper that produces the /close passes to `autocommit +# get`. Sharing it is the point: a second, independent resolver here could +# disagree with /close's, so `set` would write a flag `get` never reads — the +# exact failure this resolution exists to prevent. (An earlier cut open-coded a +# `--git-common-dir` + `basename == .git` heuristic; that duplicated +# agent-registry.sh and broke on separate-git-dir/submodule layouts.) +# +# The resolver's answer is CROSS-CHECKED before use, not trusted: under +# `git init --separate-git-dir`, `git worktree list --porcelain` reports the GIT +# DIR as the first worktree (verified), so an unchecked result would have `set` +# cheerfully create `.claude/` INSIDE `.git` — a flag that can never be committed, +# reported as success. `--is-inside-work-tree` + `--show-toplevel` costs one call +# and turns that silent-wrong-success into a clean refusal. (Fixing the exotic +# layout itself belongs in main-repo-path.sh; this only refuses to act on it.) +# +# The subshell `cd` is scoped (never a persistent cd — see the cwd-safety rule); +# main-repo-path.sh reads the current directory rather than taking a path. The +# helper is addressed via the absolute $SCRIPT_DIR — NOT `$(dirname "$0")`, which +# inside this subshell would resolve against "$repo" when $0 is relative. +# Prints the root, or nothing when does not resolve to a real work tree. +# Always returns 0, so a caller's `root="$(autocommit_root …)"` can't trip `set -e`. +autocommit_root() { + local repo="$1" root="" + [ -d "$repo" ] || return 0 + root="$( cd "$repo" 2>/dev/null && bash "$SCRIPT_DIR/main-repo-path.sh" path 2>/dev/null || true )" + [ -n "$root" ] && [ -d "$root" ] || return 0 + # Must be an actual checkout, and must agree with git's own idea of its top. + [ "$(git -C "$root" rev-parse --is-inside-work-tree 2>/dev/null || true)" = "true" ] || return 0 + local top; top="$(git -C "$root" rev-parse --show-toplevel 2>/dev/null || true)" + [ -n "$top" ] || return 0 + printf '%s\n' "$top" + return 0 +} + +# Explain WHY the main checkout could not be resolved. "Not a git repository" is +# wrong (and sends the user hunting in the wrong direction) when the path IS in a +# repo whose layout the resolver can't handle — e.g. a separate-git-dir checkout, +# where `git worktree list` reports the git dir rather than the work tree. +autocommit_no_root_error() { + local op="$1" repo="$2" + if [ "$(git -C "$repo" rev-parse --is-inside-work-tree 2>/dev/null || true)" = "true" ]; then + # Do NOT suggest placing the flag by hand: `get` cannot resolve this layout + # either, so a hand-placed flag would never be read. Say it is unavailable. + echo "autocommit $op: cannot resolve the main checkout for $repo — unsupported repository layout (e.g. separate-git-dir); the autocommit opt-in is unavailable here" >&2 + else + echo "autocommit $op: not a git repository: $repo" >&2 + fi +} + +# Refuse to write/delete through a redirected path. Guarding only the LEAF is not +# enough: with no `.claude` yet, a symlinked `.claude` DIRECTORY makes `mkdir -p` +# succeed through the link and the write land outside the repo entirely, while the +# reported path still looks in-repo. So check the parent as well, and verify the +# parent physically resolves INSIDE the repo root. Returns 1 (with a message) when +# the caller must abort. $3 is the subcommand name, for the message only. +autocommit_guard_paths() { + local root="$1" file="$2" op="$3" dir; dir="$(dirname "$file")" + if [ -L "$dir" ]; then + echo "autocommit $op: refusing — $dir is a symlink" >&2; return 1 + fi + if [ -L "$file" ]; then + echo "autocommit $op: refusing — $file is a symlink" >&2; return 1 + fi + # TYPE, not just symlink-ness. Otherwise a plain file at `.claude` let `mkdir + # -p` fail with a raw error and an undocumented exit 1, and a DIRECTORY at the + # flag path passed the guard entirely — `mv` then moved the temp file *into* it + # and `set` reported success for an opt-in that could never work. + if [ -e "$dir" ] && [ ! -d "$dir" ]; then + echo "autocommit $op: refusing — $dir exists and is not a directory" >&2; return 1 + fi + if [ -e "$file" ] && [ ! -f "$file" ]; then + echo "autocommit $op: refusing — $file exists and is not a regular file" >&2; return 1 + fi + # Physical containment: if .claude already exists, its resolved path must sit + # under the resolved repo root. Skipped when it doesn't exist yet (mkdir -p will + # create a real directory — the -L check above already ruled out a link). + if [ -d "$dir" ]; then + local rdir rroot + rdir="$( cd "$dir" 2>/dev/null && pwd -P || true )" + rroot="$( cd "$root" 2>/dev/null && pwd -P || true )" + if [ -z "$rdir" ] || [ -z "$rroot" ] || [ "${rdir#"$rroot"/}" = "$rdir" ]; then + echo "autocommit $op: refusing — $dir does not resolve inside $root" >&2; return 1 + fi + fi + return 0 +} + autocommit() { - local op="${1:-}" repo="${2:-}" - local file="$repo/.claude/work-system-close-autocommit" + local op="${1:-}" repo="${2:-}" mainbr="${3:-}" + case "$op" in + get|set|unset) ;; + *) echo "usage: ${0##*/} autocommit {get|set|unset} " >&2; exit 2 ;; + esac + if [ -z "$repo" ]; then + echo "usage: ${0##*/} autocommit $op " >&2; exit 2 + fi + + local rel=".claude/work-system-close-autocommit" + local root; root="$(autocommit_root "$repo")" + local file=""; [ -n "$root" ] && file="$root/$rel" + case "$op" in get) - [ -n "$repo" ] || { echo "usage: ${0##*/} autocommit get " >&2; exit 2; } - local enabled="no" - if [ -f "$file" ]; then - local content; content="$(tr -d '[:space:]' < "$file" 2>/dev/null || true)" - case "$content" in yes|true) enabled="yes" ;; esac + # FAIL-SAFE by construction: every unresolved or suspicious case reports + # enabled=no, which simply keeps /close's approval prompt. `reason=` (a + # machine code) plus `note=` (the ready-to-print human sentence, so callers + # never re-spell the vocabulary in prose) are emitted ONLY when a flag file + # exists but was not honored — the confusing case, where the feature would + # otherwise just look broken. A plainly absent flag stays a bare enabled=no. + if [ -z "$root" ]; then + # Not silent: `set` refuses these layouts with a diagnostic, so `get` + # must say the same thing rather than looking like a plain "not opted in" + # — otherwise the opt-in is unreachable AND unexplained. + if [ "$(git -C "$repo" rev-parse --is-inside-work-tree 2>/dev/null || true)" = "true" ]; then + printf 'enabled=no\nreason=unsupported-layout\nnote=%s\n' \ + "cannot resolve this repository's main checkout (unsupported layout) — the autocommit opt-in is unavailable here" + else + printf 'enabled=no\nreason=not-a-repo\nnote=%s\n' \ + "not a git repository — the autocommit opt-in is unavailable here" + fi + return 0 + fi + # NOTHING CONFIGURED comes first: a repo that never opted in must answer a + # bare `enabled=no`, with no reason/note. Testing the symlink before this + # made every /close in a repo with a symlinked `.claude` (a normal dotfiles + # layout) warn about an opt-in the user never set up — and callers are told + # a note means "an existing flag was not honored". + if [ ! -e "$file" ] && [ ! -L "$file" ]; then printf 'enabled=no\n'; return 0; fi + # A symlinked flag — or a symlinked `.claude` parent — means the on-disk + # state is redirected; never honor it. (The value itself comes from the + # committed object below, so this cannot change the answer, but a redirected + # checkout is exactly the situation to fall back to asking in.) + if [ -L "$file" ] || [ -L "$root/.claude" ]; then + printf 'enabled=no\nreason=symlink\nnote=%s\n' \ + "the autocommit flag (or its .claude directory) is a symlink — refusing to honor it" + return 0 fi - printf 'enabled=%s\n' "$enabled" + if [ ! -f "$file" ]; then + printf 'enabled=no\nreason=not-a-file\nnote=%s\n' \ + "the autocommit flag path is not a regular file — refusing to honor it" + return 0 + fi + + # PROVENANCE — presence alone is NOT authorization. Read the value from the + # COMMITTED object (`git show HEAD:`), never from the working tree: + # that single call is both the tracked-check (it fails when the path isn't + # in HEAD) and the value read, and it is immune to the working-tree tricks + # that fool a `diff`-based guard — `git update-index --assume-unchanged` / + # `--skip-worktree` make `ls-files`/`diff --quiet` report a modified file as + # clean, but they cannot change what HEAD contains. + # + # Scope of this guard (documented honestly — see the knowledge entry): it + # stops a flag that was merely WRITTEN (a tool, a stray file, an agent's + # file write) from waiving the prompt. It does NOT stop an actor who can + # already commit in your repo — that actor can commit the flag like any + # other change. What bounds the damage there is commit-push itself: + # archive-scoped pathspec, fast-forward only, never a force-push, and a + # refusal when carries other unpushed commits. + # WHICH REF: the archive lands on , and commit_push refuses to + # commit anywhere else, so the authorization must be read from that same + # branch — not from whatever the main checkout happens to be parked on. + # Reading HEAD made the verdict depend on the current branch: a checkout on + # an unrelated branch reported "not committed" for a flag that IS committed + # on main, and a checkout on a branch carrying the flag announced + # "auto-committing" before commit_push bailed out with wrong-branch. + # Falls back to HEAD when no branch is passed (standalone/manual use). + local ref="HEAD" + if [ -n "$mainbr" ] && git -C "$root" rev-parse --verify -q "$mainbr" >/dev/null 2>&1; then + ref="$mainbr" + fi + local head_val rc=0 + head_val="$(git -C "$root" show "$ref:$rel" 2>/dev/null)" || rc=$? + if [ "$rc" -ne 0 ]; then + # Not on that ref (never committed there), or git could not read it at + # all. Both are "not authorized"; distinguish them so debugging isn't + # misdirected. + if git -C "$root" rev-parse --verify -q "$ref" >/dev/null 2>&1; then + printf 'enabled=no\nreason=untracked\nnote=%s\n' \ + "the autocommit flag is not committed on $ref — it takes effect only once committed there (it is authorization, not a scratch file)" + else + printf 'enabled=no\nreason=git-error\nnote=%s\n' \ + "could not read the autocommit flag from git — falling back to asking" + fi + return 0 + fi + # The committed value is what authorizes, so a local edit can never silently + # ENABLE the skip. A local edit SHOULD still be able to disable it (that is + # a deliberate act by whoever holds the checkout), so a flag whose working + # copy differs from HEAD falls back to asking. + # + # Compare BLOB HASHES, not `git diff --quiet`: the index bits that hide a + # dirty file (`--assume-unchanged`, `--skip-worktree`) make diff report + # "clean", which would silently ignore a deliberate local disable. Hashing + # the file and the HEAD blob is index-independent, so both directions hold. + local head_oid work_oid + head_oid="$(git -C "$root" rev-parse "$ref:$rel" 2>/dev/null || true)" + work_oid="$(git -C "$root" hash-object -- "$file" 2>/dev/null || true)" + if [ -z "$work_oid" ] || [ "$work_oid" != "$head_oid" ]; then + # Deliberately disabling locally is a SUPPORTED move, so don't answer it + # with "commit or revert it to make it effective" — that tells the user to + # undo the thing they just did, on every single close. Only content that + # is neither the committed value nor an explicit off gets the corrective + # wording. + local local_val + local_val="$(sed 's/^[[:space:]]*//; s/[[:space:]]*$//' < "$file" 2>/dev/null | tr '[:upper:]' '[:lower:]' || true)" + case "$local_val" in + no|false|0|off) + printf 'enabled=no\nreason=locally-disabled\nnote=%s\n' \ + "the autocommit opt-in is switched off in your working copy (the committed flag is unchanged)" ;; + *) + printf 'enabled=no\nreason=modified\nnote=%s\n' \ + "the autocommit flag differs from the committed version — commit or revert it to make it effective" ;; + esac + return 0 + fi + + # TRIM, not "delete every space": `tr -d '[:space:]'` would collapse `y e s` + # (or a value split across lines) into an accepted token, honoring content + # that matches no documented value. Strip only leading/trailing whitespace, + # and treat anything multi-line as unrecognized. Case-insensitive, because + # the file is hand-edited by design and `Yes`/`TRUE` must not read as "off". + local content + case "$head_val" in + # Pure-shell newline test — a `grep -c ''` pipeline exits 1 on empty + # input, so an `|| echo 0` fallback fires on TOP of grep's own "0" and + # yields the two-line string "0\n0", which `[ -gt ]` then rejects with a + # bash error on stderr mid-close. + *" +"*) content="__multiline__" ;; + *) content="$(printf '%s' "$head_val" \ + | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ + | tr '[:upper:]' '[:lower:]')" ;; + esac + case "$content" in + yes|true|1|on) printf 'enabled=yes\n' ;; + no|false|0|off) printf 'enabled=no\n' ;; + *) printf 'enabled=no\nreason=unrecognized-content\nnote=%s\n' \ + "the autocommit flag's content is not a recognized value (expected yes/true/1/on)" ;; + esac ;; set) - [ -n "$repo" ] || { echo "usage: ${0##*/} autocommit set " >&2; exit 2; } - mkdir -p "$repo/.claude" - printf 'yes\n' > "$file" + if [ -z "$root" ]; then autocommit_no_root_error set "$repo"; exit 2; fi + autocommit_guard_paths "$root" "$file" set || exit 2 + mkdir -p "$root/.claude" + # RE-VALIDATE after mkdir: the guard above runs before the directory + # exists, so between the two a concurrent process could plant `.claude` as + # a symlink and `mkdir -p` would succeed straight through it, landing every + # later write outside the repo while `flag=` still reads in-repo. + autocommit_guard_paths "$root" "$file" set || exit 2 + # Write through a mktemp'd file IN THE TARGET DIRECTORY, then rename. A + # fixed `$file.tmp.$$` is PID-predictable: anything able to create files in + # .claude/ could pre-plant that path as a symlink, have the redirect write + # through it (clobbering an arbitrary file), and then have `mv` install the + # symlink AS the flag. mktemp creates the file exclusively (O_EXCL) with a + # random name, so there is nothing to pre-plant. Same directory keeps the + # final `mv` a rename, so the flag is REPLACED, never written through. + local tmp + tmp="$(mktemp "$root/.claude/.work-system-close-autocommit.XXXXXX" 2>/dev/null || true)" + if [ -z "$tmp" ]; then + echo "autocommit set: could not create a temp file in $root/.claude" >&2; exit 1 + fi + if ! printf 'yes\n' > "$tmp"; then + rm -f "$tmp" 2>/dev/null || true + echo "autocommit set: could not write $file" >&2; exit 1 + fi + # Set the mode on the TEMP file, before it has the well-known name: a + # `chmod` after the rename could follow a symlink swapped in behind it and + # change some other file's mode. (mktemp creates 600; the flag is ordinary + # repo content.) + chmod 644 "$tmp" 2>/dev/null || true + # Final re-check of both parent and leaf immediately before the rename. + autocommit_guard_paths "$root" "$file" set || { rm -f "$tmp" 2>/dev/null || true; exit 2; } + mv -f "$tmp" "$file" + printf 'flag=%s\n' "$file" + # A gitignored `.claude/` makes the whole opt-in impossible — `git add` + # refuses the path, so `get` would report `untracked` forever while `set` + # kept claiming success. Say so instead of sending the user in circles. + if git -C "$root" check-ignore -q -- "$rel" 2>/dev/null; then + printf 'note=%s\n' "$rel is gitignored — the opt-in cannot take effect until the path is un-ignored (or added with git add -f)" + else + printf 'note=%s\n' "commit this file — the opt-in takes effect only once it is committed" + fi ;; unset) - [ -n "$repo" ] || { echo "usage: ${0##*/} autocommit unset " >&2; exit 2; } + if [ -z "$root" ]; then autocommit_no_root_error unset "$repo"; exit 2; fi + # Guard the PARENT here too: `rm -f` through a symlinked .claude would + # delete a file outside the repo. (`rm` on a symlinked LEAF is safe — it + # removes the link, not its target — but a symlinked parent is not.) + autocommit_guard_paths "$root" "$file" unset || exit 2 + local was_committed=no + git -C "$root" cat-file -e "HEAD:$rel" 2>/dev/null && was_committed=yes rm -f "$file" + printf 'flag=%s\n' "$file" + # Deleting the file makes `get` report disabled immediately, which LOOKS + # done — but HEAD still carries the flag, so a fresh clone, a teammate, or a + # `git restore` re-enables it. Mirror set's reminder. + if [ "$was_committed" = yes ]; then + printf 'note=%s\n' "commit this deletion — the opt-in stays active for any checkout that still has the committed file" + fi ;; - *) echo "usage: ${0##*/} autocommit {get|set|unset} " >&2; exit 2 ;; esac } diff --git a/plugins/work-system/scripts/test_archive_task.py b/plugins/work-system/scripts/test_archive_task.py index 4608a37..1dbec9a 100644 --- a/plugins/work-system/scripts/test_archive_task.py +++ b/plugins/work-system/scripts/test_archive_task.py @@ -3,11 +3,15 @@ (`python3 test_archive_task.py`) or via scripts/check-structure.py's "plugin tests" check. -Scope: just the flag-routing logic behind /close step 10's opt-in +Scope: the flag-routing logic behind /close step 10's opt-in (`.claude/work-system-close-autocommit`) — the part that is actually script-testable. The `archive`/`commit-push` subcommands are exercised -manually via /close; they need a real git repo and are covered by the -design rationale in `.claude/knowledge/features/task-archiving-on-close.md`. +manually via /close; their design rationale lives in +`.claude/knowledge/features/task-archiving-on-close.md`. + +The tests run against REAL git repos (not bare temp dirs) because the flag is +only honored when git-tracked and unmodified: presence alone must never +authorize skipping /close's push-approval gate. """ import subprocess import sys @@ -18,6 +22,7 @@ SCRIPT = HERE / "archive-task.sh" FAILS = [] +REL = ".claude/work-system-close-autocommit" def check(name, cond): @@ -31,54 +36,396 @@ def run(*args): ) -def flag_path(repo): - return repo / ".claude" / "work-system-close-autocommit" +def git(repo, *args): + return subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True + ) + + +def kv(out): + """Parse key=value stdout lines into a dict.""" + d = {} + for line in out.splitlines(): + if "=" in line: + k, v = line.split("=", 1) + d[k] = v + return d + + +def new_repo(root, name="repo"): + """A real git repo with one commit, so HEAD exists for the clean-check.""" + repo = root / name + repo.mkdir(parents=True) + git(repo, "init", "-q") + git(repo, "config", "user.email", "test@example.com") + git(repo, "config", "user.name", "Test") + (repo / "seed.txt").write_text("seed\n") + git(repo, "add", "seed.txt") + git(repo, "commit", "-qm", "seed") + return repo + + +def commit_flag(repo, msg="add flag"): + git(repo, "add", REL) + git(repo, "commit", "-qm", msg) with tempfile.TemporaryDirectory() as tmp: - repo = Path(tmp) / "repo" - repo.mkdir() + # .resolve(): git reports the CANONICAL root (on macOS /tmp -> /private/tmp), + # so compare resolved paths or every `flag=` assertion fails on a symlinked + # temp dir. The script echoing git's canonical path is correct behavior. + root = Path(tmp).resolve() + repo = new_repo(root) + flag = repo / REL - # --- get: no flag file -> disabled by default ------------------------- # + # --- get: no flag file -> disabled by default, no reason line ------------ # r = run("autocommit", "get", str(repo)) - check("no flag file -> enabled=no", r.stdout.strip() == "enabled=no") + check("no flag -> enabled=no", kv(r.stdout).get("enabled") == "no") + check("no flag -> no reason line", "reason" not in kv(r.stdout)) check("get exits 0", r.returncode == 0) - # --- set: writes the file, creates .claude/ as needed ------------------ # + # --- set: writes the file and reports where + that it must be committed -- # r = run("autocommit", "set", str(repo)) check("set exits 0", r.returncode == 0) - check("set creates the flag file", flag_path(repo).is_file()) - check("set writes 'yes'", flag_path(repo).read_text().strip() == "yes") + check("set creates the flag", flag.is_file()) + check("set writes 'yes'", flag.read_text().strip() == "yes") + check("set echoes the resolved path", kv(r.stdout).get("flag") == str(flag)) + check("set reminds to commit", "commit" in kv(r.stdout).get("note", "")) + + # --- PROVENANCE: an uncommitted flag is NOT authorization ---------------- # + # This is the core guard: a file a tool/agent merely wrote must not waive + # /close's approval gate for a push to the default branch. + r = run("autocommit", "get", str(repo)) + check("untracked flag -> enabled=no", kv(r.stdout).get("enabled") == "no") + check("untracked flag -> reason=untracked", kv(r.stdout).get("reason") == "untracked") + check("untracked flag ships a human note", "committed" in kv(r.stdout).get("note", "")) + + # committed -> now honored + commit_flag(repo) + r = run("autocommit", "get", str(repo)) + check("committed flag -> enabled=yes", kv(r.stdout).get("enabled") == "yes") + check("enabled=yes carries no reason", "reason" not in kv(r.stdout)) + + # locally modified -> falls back to asking (deliberate local disabling works) + flag.write_text("no\n") + r = run("autocommit", "get", str(repo)) + check("locally disabled -> enabled=no", kv(r.stdout).get("enabled") == "no") + check("locally disabled -> reason=locally-disabled", + kv(r.stdout).get("reason") == "locally-disabled") + flag.write_text("yes\n") # back to the committed content + check("restored -> enabled=yes", + kv(run("autocommit", "get", str(repo)).stdout).get("enabled") == "yes") + + # a local edit cannot silently ENABLE a committed-off flag either + flag.write_text("no\n") + commit_flag(repo, "disable flag") + flag.write_text("yes\n") + r = run("autocommit", "get", str(repo)) + check("local enable of committed-off -> enabled=no", kv(r.stdout).get("enabled") == "no") + check("local enable -> reason=modified", kv(r.stdout).get("reason") == "modified") + + # REGRESSION: the index bits that make a dirty file look clean must not + # enable the waiver. A diff-based guard reports "unmodified" here; reading + # the value from HEAD is what actually holds the line. + for bit in ("--assume-unchanged", "--skip-worktree"): + git(repo, "update-index", bit, REL) # committed content is "no" + flag.write_text("yes\n") + r = run("autocommit", "get", str(repo)) + check(f"{bit} cannot enable the waiver", kv(r.stdout).get("enabled") == "no") + git(repo, "update-index", bit.replace("--", "--no-", 1), REL) + git(repo, "checkout", "--", REL) + flag.write_text("yes\n") + commit_flag(repo, "re-enable") + + # ...and the same bits must not hide a deliberate local DISABLE either. A + # `git diff --quiet` dirty-check reports "clean" here; comparing blob hashes + # is what keeps the escape hatch working. + for bit in ("--assume-unchanged", "--skip-worktree"): + git(repo, "update-index", bit, REL) # committed content is "yes" + flag.write_text("no\n") + r = run("autocommit", "get", str(repo)) + check(f"{bit} cannot hide a local disable", kv(r.stdout).get("enabled") == "no") + check(f"{bit} local disable -> reason=locally-disabled", + kv(r.stdout).get("reason") == "locally-disabled") + git(repo, "update-index", bit.replace("--", "--no-", 1), REL) + git(repo, "checkout", "--", REL) + # --- content variants (committed each time; case-insensitive) ------------ # + for content, expected, reason in [ + ("yes\n", "yes", None), + ("true\n", "yes", None), + ("YES\n", "yes", None), # case-insensitive: hand-edited by design + ("True\n", "yes", None), + ("1\n", "yes", None), + ("on\n", "yes", None), + (" yes \n", "yes", None), # whitespace-trimmed + ("no\n", "no", None), # recognized off -> no confusing reason + ("false\n", "no", None), + ("maybe\n", "no", "unrecognized-content"), + ("autocommit=yes\n", "no", "unrecognized-content"), # key=value form is NOT accepted + ("", "no", "unrecognized-content"), + # TRIM, not "delete all whitespace": `tr -d '[:space:]'` would collapse + # these into the accepted token `yes` and waive the gate on content + # matching no documented value. + ("y e s\n", "no", "unrecognized-content"), + ("ye\ns\n", "no", "unrecognized-content"), + ("yes\nno\n", "no", "unrecognized-content"), # multi-line is never accepted + ]: + flag.write_text(content) + commit_flag(repo, f"content {content!r}") + r = run("autocommit", "get", str(repo)) + got = kv(r.stdout) + check(f"content {content!r} -> enabled={expected}", got.get("enabled") == expected) + check(f"content {content!r} -> reason={reason}", got.get("reason") == reason) + # `get` is called mid-/close and its stderr is surfaced to the user, so a + # stray shell error (e.g. from a `grep -c ''` fallback yielding "0\n0") + # would read as the close itself breaking. + check(f"content {content!r} -> clean stderr", r.stderr == "") + + # --- symlink: never honored, never written through ----------------------- # + victim = root / "victim.txt" + victim.write_text("IMPORTANT\n") + flag.unlink() + flag.symlink_to(victim) r = run("autocommit", "get", str(repo)) - check("after set -> enabled=yes", r.stdout.strip() == "enabled=yes") + check("symlinked flag -> enabled=no", kv(r.stdout).get("enabled") == "no") + check("symlinked flag -> reason=symlink", kv(r.stdout).get("reason") == "symlink") + r = run("autocommit", "set", str(repo)) + check("set on symlink -> exit 2", r.returncode == 2) + check("set on symlink explains why", "symlink" in r.stderr) + check("set on symlink leaves victim intact", victim.read_text() == "IMPORTANT\n") + flag.unlink() + git(repo, "checkout", "--", REL) + + # REGRESSION: a symlinked `.claude` PARENT escapes the repo even though the + # leaf doesn't exist — `mkdir -p` succeeds through the link and the write + # lands outside while the reported path still looks in-repo. + outside = root / "outside" + outside.mkdir() + (outside / "work-system-close-autocommit").write_text("PRECIOUS\n") + prepo = new_repo(root, "prepo") + (prepo / ".claude").symlink_to(outside) + r = run("autocommit", "set", str(prepo)) + check("set through symlinked .claude -> exit 2", r.returncode == 2) + check("set through symlinked .claude explains why", "symlink" in r.stderr) + check("set through symlinked .claude leaves victim intact", + (outside / "work-system-close-autocommit").read_text() == "PRECIOUS\n") + r = run("autocommit", "unset", str(prepo)) + check("unset through symlinked .claude -> exit 2", r.returncode == 2) + check("unset through symlinked .claude deletes nothing", + (outside / "work-system-close-autocommit").is_file()) + check("get through symlinked .claude -> enabled=no", + kv(run("autocommit", "get", str(prepo)).stdout).get("enabled") == "no") + + # REGRESSION: the temp path must not be PID-predictable. A pre-planted + # `.tmp.` symlink used to be followed by the redirect (clobbering + # the target) and then installed AS the flag by `mv`. + # + # A behavioral test cannot cover this honestly: the vulnerable name embeds + # the script's own `$$`, which the test cannot know or control, so planting a + # guessed PID range proves nothing (an earlier version of this test planted + # PIDs 2-399 while real subprocess PIDs run in the thousands — reverting the + # fix left every assertion passing). So assert the PROPERTY at the source: the + # writer must not build a temp path out of `$$`. Crude, but unlike the + # behavioral version it actually fails when the bug comes back. + code = "\n".join( + ln for ln in SCRIPT.read_text().splitlines() + if not ln.lstrip().startswith("#") # comments explain the bug by name + ) + check("no PID-derived temp path in the writer", ".tmp.$$" not in code) + check("writer uses mktemp", "mktemp " in code) + # Behavioral half: the flag ends up a real file and nothing outside is touched. + trepo = new_repo(root, "trepo") + tvictim = root / "tempvictim.txt" + tvictim.write_text("PRECIOUS\n") + r = run("autocommit", "set", str(trepo)) + check("set succeeds", r.returncode == 0) + check("unrelated file untouched", tvictim.read_text() == "PRECIOUS\n") + check("flag is a regular file, not a symlink", + (trepo / REL).is_file() and not (trepo / REL).is_symlink()) + check("no temp files left behind", + not list((trepo / ".claude").glob(".work-system-close-autocommit.*"))) + + # --- linked worktree: set/get address the MAIN checkout ------------------ # + # A flag written into a disposable worktree would never be read (and /close + # deletes the worktree with it), so all three ops resolve to the main repo + # root. Uses a FRESH repo whose flag was never committed — otherwise + # `git worktree add` checks the tracked flag out into the worktree and a + # "no worktree-local flag" assertion would see git's copy, not a stray write. + wrepo = new_repo(root, "wrepo") + wflag = wrepo / REL + wt = root / "wt" + git(wrepo, "worktree", "add", "-q", "-b", "task/x", str(wt)) + if wt.is_dir(): + r = run("autocommit", "set", str(wt)) + check("set from worktree targets main repo", kv(r.stdout).get("flag") == str(wflag)) + check("set from worktree wrote the main repo flag", wflag.is_file()) + check("set from worktree writes no worktree-local flag", not (wt / REL).exists()) + # committed in the main repo -> a get from the worktree honors it + commit_flag(wrepo, "enable in main repo") + check("get from worktree reads main repo flag", + kv(run("autocommit", "get", str(wt)).stdout).get("enabled") == "yes") + else: + FAILS.append("could not create linked worktree for the resolution test") - # --- unset: removes the file, reverts to disabled ----------------------- # + # --- unset ---------------------------------------------------------------- # r = run("autocommit", "unset", str(repo)) check("unset exits 0", r.returncode == 0) - check("unset removes the flag file", not flag_path(repo).exists()) - check("after unset -> enabled=no", run("autocommit", "get", str(repo)).stdout.strip() == "enabled=no") - - # --- content variants: only yes/true (whitespace-trimmed) count -------- # - flag_path(repo).parent.mkdir(parents=True, exist_ok=True) - for content, expected in [ - ("yes\n", "yes"), - ("true\n", "yes"), - (" yes \n", "yes"), - ("no\n", "no"), - ("1\n", "no"), - ("", "no"), - ]: - flag_path(repo).write_text(content) - got = run("autocommit", "get", str(repo)).stdout.strip() - check(f"content {content!r} -> enabled={expected}", got == f"enabled={expected}") + check("unset removes the flag", not flag.exists()) + check("unset echoes the path", kv(r.stdout).get("flag") == str(flag)) + # removed but still tracked in HEAD -> the deletion is a modification + check("after unset -> enabled=no", + kv(run("autocommit", "get", str(repo)).stdout).get("enabled") == "no") + + # --- REGRESSION: relative invocation must not resolve the sibling helper --- # + # `$(dirname "$0")` expanded inside `( cd "$repo" && … )` resolves against the + # TARGET repo, so a script planted there would be run (and its stdout trusted + # as the repo root). Invoke through a relative path, from the scripts' own + # directory, with a decoy helper in the target repo. + drepo = new_repo(root, "drepo") + (drepo / "scripts").mkdir() + marker = root / "PWNED" + (drepo / "scripts" / "main-repo-path.sh").write_text( + f"#!/bin/sh\ntouch {marker}\necho {drepo}\n" + ) + (drepo / "scripts" / "main-repo-path.sh").chmod(0o755) + rel_run = subprocess.run( + ["bash", SCRIPT.name, "autocommit", "get", str(drepo)], + cwd=str(HERE), capture_output=True, text=True, + ) + check("relative invocation does not run the target repo's script", not marker.exists()) + check("relative invocation still answers", "enabled=" in rel_run.stdout) + + # --- REGRESSION: gitignored .claude/ must be called out -------------------- # + # Otherwise `git add` refuses the path and `get` says "untracked" forever + # while `set` keeps reporting success. + irepo = new_repo(root, "irepo") + (irepo / ".gitignore").write_text(".claude/\n") + git(irepo, "add", ".gitignore") + git(irepo, "commit", "-qm", "ignore .claude") + r = run("autocommit", "set", str(irepo)) + check("set in gitignored repo still exits 0", r.returncode == 0) + check("set warns the path is gitignored", "gitignored" in kv(r.stdout).get("note", "")) + + # --- REGRESSION: unset reminds you to commit the deletion ------------------ # + r = run("autocommit", "unset", str(repo)) + check("unset of a committed flag reminds to commit the deletion", + "commit" in kv(r.stdout).get("note", "")) + git(repo, "checkout", "--", REL) + # ...but not when there was nothing committed to begin with + nrepo = new_repo(root, "nrepo") + r = run("autocommit", "unset", str(nrepo)) + check("unset with no committed flag stays quiet", "note" not in kv(r.stdout)) + + # --- REGRESSION: unsupported layout is refused, never written into .git ---- # + # `git worktree list` reports the GIT DIR under --separate-git-dir, so an + # unchecked resolver would create .claude/ inside .git — uncommittable. + sgd_wt = root / "sgd" + sgd_git = root / "sgd.git" + subprocess.run(["git", "init", "-q", f"--separate-git-dir={sgd_git}", str(sgd_wt)], + capture_output=True, text=True) + if sgd_wt.is_dir(): + git(sgd_wt, "config", "user.email", "test@example.com") + git(sgd_wt, "config", "user.name", "Test") + (sgd_wt / "seed.txt").write_text("seed\n") + git(sgd_wt, "add", "seed.txt") + git(sgd_wt, "commit", "-qm", "seed") + # Assert the INVARIANT ("set and get agree"), not today's git quirk: the + # refusal only happens because `git worktree list` currently reports the + # git dir for such repos. A git release that reports the work tree + # correctly would make `set` succeed — an improvement that must not turn + # CI red. Either outcome is acceptable; a silent wrong-success is not. + r = run("autocommit", "set", str(sgd_wt)) + check("separate-git-dir never writes into the git dir", + not (sgd_git / ".claude").exists()) + if r.returncode == 0: + git(sgd_wt, "add", "-f", REL) + git(sgd_wt, "commit", "-qm", "flag") + check("separate-git-dir: if set succeeds, get honors it", + kv(run("autocommit", "get", str(sgd_wt)).stdout).get("enabled") == "yes") + else: + check("separate-git-dir refusal is the documented exit 2", r.returncode == 2) + check("separate-git-dir refusal names the layout", "layout" in r.stderr) + # get must explain it too, not answer a bare enabled=no that looks + # like "simply not opted in". + g = kv(run("autocommit", "get", str(sgd_wt)).stdout) + check("separate-git-dir get explains itself", g.get("reason") == "unsupported-layout") + check("separate-git-dir get ships a note", "unavailable" in g.get("note", "")) + + # --- REGRESSION: the verdict follows , not the current HEAD --- # + # The flag authorizes a commit onto the default branch, and commit-push + # refuses to commit anywhere else — so a flag living only on some feature + # branch must NOT announce an auto-commit. + brepo = new_repo(root, "brepo") # new_repo's first branch is the default + default_branch = git(brepo, "rev-parse", "--abbrev-ref", "HEAD").stdout.strip() + git(brepo, "checkout", "-q", "-b", "feature") + (brepo / ".claude").mkdir() + (brepo / REL).write_text("yes\n") + git(brepo, "add", REL) + git(brepo, "commit", "-qm", "flag on feature only") + check("flag only on a feature branch -> not honored for the default branch", + kv(run("autocommit", "get", str(brepo), default_branch).stdout).get("enabled") == "no") + check("...and says it is not committed there", + kv(run("autocommit", "get", str(brepo), default_branch).stdout).get("reason") == "untracked") + # committed on the default branch -> honored + git(brepo, "checkout", "-q", default_branch) + (brepo / ".claude").mkdir(exist_ok=True) # tracked only on `feature` until now + (brepo / REL).write_text("yes\n") + git(brepo, "add", REL) + git(brepo, "commit", "-qm", "flag on default") + check("flag on the default branch -> honored", + kv(run("autocommit", "get", str(brepo), default_branch).stdout).get("enabled") == "yes") + + # --- REGRESSION: a deliberate local disable gets neutral wording ---------- # + # "commit or revert it to make it effective" would tell the user to undo the + # very thing they just did, on every close. + (brepo / REL).write_text("no\n") + g = kv(run("autocommit", "get", str(brepo), default_branch).stdout) + check("local disable -> reason=locally-disabled", g.get("reason") == "locally-disabled") + check("local disable note is not corrective", "revert" not in g.get("note", "")) + (brepo / REL).write_text("garbage\n") + g = kv(run("autocommit", "get", str(brepo), default_branch).stdout) + check("local garbage -> reason=modified", g.get("reason") == "modified") + + # --- REGRESSION: a symlinked .claude in a repo that never opted in -------- # + # Must answer a BARE enabled=no: callers relay any note verbatim, so warning + # about an opt-in that was never configured fires on every close. + srepo = new_repo(root, "srepo") + empty_target = root / "empty-claude" + empty_target.mkdir() + (srepo / ".claude").symlink_to(empty_target) + g = kv(run("autocommit", "get", str(srepo)).stdout) + check("symlinked .claude, never opted in -> bare enabled=no", g.get("enabled") == "no") + check("symlinked .claude, never opted in -> no reason", "reason" not in g) + + # --- REGRESSION: path types other than symlink are refused too ------------ # + frepo = new_repo(root, "frepo") + (frepo / ".claude").write_text("i am a file\n") + r = run("autocommit", "set", str(frepo)) + check("regular file at .claude -> exit 2", r.returncode == 2) + check("regular file at .claude -> refusal message", "not a directory" in r.stderr) + drepo2 = new_repo(root, "drepo2") + (drepo2 / ".claude").mkdir() + (drepo2 / REL).mkdir() + r = run("autocommit", "set", str(drepo2)) + check("directory at flag path -> exit 2", r.returncode == 2) + check("directory at flag path -> refusal message", "not a regular file" in r.stderr) - # --- a repo that never had .claude/ at all ------------------------------ # - bare = Path(tmp) / "bare" - bare.mkdir() - check("no repo dir at all -> enabled=no", run("autocommit", "get", str(bare)).stdout.strip() == "enabled=no") + # --- non-git paths -------------------------------------------------------- # + plain = root / "plain" + plain.mkdir() + check("non-git get -> enabled=no", + kv(run("autocommit", "get", str(plain)).stdout).get("enabled") == "no") + r = run("autocommit", "set", str(plain)) + check("non-git set -> exit 2", r.returncode == 2) + check("non-git set explains why", "not a git repository" in r.stderr) + check("non-git set creates nothing", not (plain / ".claude").exists()) + r = run("autocommit", "set", str(root / "no" / "such" / "path")) + check("bogus path set -> exit 2", r.returncode == 2) + check("bogus path creates no stray tree", not (root / "no").exists()) + check("non-git unset -> exit 2", run("autocommit", "unset", str(plain)).returncode == 2) - # --- usage errors -------------------------------------------------------- # + # --- usage errors --------------------------------------------------------- # check("get missing repo -> exit 2", run("autocommit", "get").returncode == 2) check("set missing repo -> exit 2", run("autocommit", "set").returncode == 2) check("unknown op -> exit 2", run("autocommit", "bogus", str(repo)).returncode == 2) diff --git a/plugins/work-system/skills/close/SKILL.md b/plugins/work-system/skills/close/SKILL.md index b00712f..3ea18b3 100644 --- a/plugins/work-system/skills/close/SKILL.md +++ b/plugins/work-system/skills/close/SKILL.md @@ -175,22 +175,40 @@ Rules: or the original task file was tracked so its removal needs recording): check the repo's opt-in flag first (per-repo only, off by default — never a global setting): ```sh - bash "${CLAUDE_PLUGIN_ROOT}/scripts/archive-task.sh" autocommit get + bash "${CLAUDE_PLUGIN_ROOT}/scripts/archive-task.sh" autocommit get ``` - - **`enabled=yes`** — the repo committed `.claude/work-system-close-autocommit`. That - file IS the durable per-repo authorization for exactly this one narrow, contained - action (archive-scoped pathspec commit, fast-forward only, never force-pushes, and it - refuses to push on `unpushed-history` — see the design rationale in - `.claude/knowledge/features/task-archiving-on-close.md`). Skip the AskUserQuestion below - and go straight to the commit+push. Print one short transparency line first — never - silent about an auto action: "Auto-committing archived task file to `` - (`.claude/work-system-close-autocommit` is set)…". - - **`enabled=no`** (the default, unchanged behavior) — show + Pass ``: the flag authorizes a commit onto *that* branch, so it is read + from it. Without it the answer would follow whatever branch the main checkout happens + to be on, which can disagree with where `commit-push` will actually commit. + - **`enabled=yes`** — and *only* this exact literal — means the repo carries a + **committed** `.claude/work-system-close-autocommit`. That file IS the durable + per-repo authorization for exactly this one narrow, contained action (archive-scoped + pathspec commit, fast-forward only, never force-pushes, and it refuses to push on + `unpushed-history`). Skip the AskUserQuestion below and go straight to the + commit+push — but **show the same scoped preview first**, then say what you are + doing. An unattended commit+push to `` must never be invisible: the + user gave standing authorization for this action, not for doing it unseen. + ```sh + git -C status --short tasks/archive/ tasks/.md + ``` + Then one line — "Auto-committing archived task file to `` + (`.claude/work-system-close-autocommit` is set)…" — and proceed without asking. + - **`enabled=no` — the default, unchanged behavior:** show `git -C status --short tasks/archive/ tasks/.md` (scoped — not the whole `tasks/`, which would surface unrelated pending tasks) and ask **once**: "Commit the archived task file to `` and push? [y/n]" — one approval covers both (the archive is metadata, and the push is what keeps local `` from diverging and breaking the next `/close`'s step-5 sync). Only continue below on yes. + If a **`note=`** line accompanies `enabled=no`, a flag file exists but was not honored + (most often: `set` was run but never committed) — **relay that note verbatim** in one + line so the opt-in doesn't just look broken. Do not re-spell the `reason=` codes here; + the script owns that vocabulary and ships the human sentence with it. + - **No `enabled=` line at all → also ask**, and add one line that the opt-in check could + not run. This covers empty stdout, a usage error (an older installed plugin whose + `archive-task.sh` predates the `autocommit` subcommand), a non-zero exit, or any + unparseable output. **Fail-closed on purpose:** guessing "yes" here would commit and + push to `` with no prompt. Note this is *not* the ordinary `enabled=no` + case above — say "could not run" only when there is genuinely no verdict to read. Either way (auto-enabled or approved), delegate the whole stage→commit→fast-forward-push to the helper (all the git-stateful steps live there, not here, so they can't drift; step From 5b857171ba8892038e7f2b768aa96e5aac36f6c8 Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Mon, 27 Jul 2026 17:12:46 +0200 Subject: [PATCH 3/4] Fix tag-shadowing and fail-open paths in the autocommit opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of the swarm review found the authorization ref was read by bare name, so a tag named like the default branch shadowed refs/heads (git resolves refs/tags first, and tags auto-follow on fetch) — letting a value the branch never carried authorize an unattended commit+push. Verified, and now covered by a regression test. - get: read refs/heads/, fully qualified; fail CLOSED when the branch is unresolvable instead of falling back to HEAD; decline when the checkout is not on that branch (commit-push would refuse anyway, so enabled=yes only announced a commit that then bailed) - unset: take the same optional so the "commit this deletion" reminder uses the ref that actually grants authorization - one autocommit_normalize() for both the committed and working-tree values; they had drifted (only one rejected multi-line content) - archive: verify a real regular file is in place before removing the source; derive the mode from umask rather than forcing 644; use the shared lit() helper for its committable probe - lit() hoisted to top level, with the actual rule stated: prefix every command that interprets pathspec magic, omit for check-ignore, which rejects it - close/SKILL.md: authorize on a whole-line `enabled=yes` match, never a substring — note= can carry repo-controlled text - header/usage/CHANGELOG/knowledge corrected to match the code --- .../features/task-archiving-on-close.md | 12 +- CHANGELOG.md | 2 +- plugins/work-system/scripts/archive-task.sh | 175 +++++++++++------- .../work-system/scripts/test_archive_task.py | 47 ++++- plugins/work-system/skills/close/SKILL.md | 8 +- 5 files changed, 169 insertions(+), 75 deletions(-) diff --git a/.claude/knowledge/features/task-archiving-on-close.md b/.claude/knowledge/features/task-archiving-on-close.md index 45f3b4d..9aed694 100644 --- a/.claude/knowledge/features/task-archiving-on-close.md +++ b/.claude/knowledge/features/task-archiving-on-close.md @@ -173,10 +173,14 @@ persistent `cd`" footgun the helper's explicit paths avoid is a rule — see - The dirty-check compares **blob hashes**, not `git diff --quiet`, so the index bits (`--assume-unchanged`/`--skip-worktree`) can't hide a deliberate local disable either. - - The verdict is read from **``**, not `HEAD` — the flag authorizes - a commit onto that branch and `commit_push` refuses any other, so reading HEAD - made the answer depend on which branch the main checkout happened to sit on - (a flag on a feature branch announced an auto-commit that then bailed out). + - The verdict is read from **`refs/heads/`**, not `HEAD` and not the + bare branch name. Reading HEAD made the answer depend on which branch the main + checkout sat on; reading the BARE name was worse — git resolves `refs/tags/` + before `refs/heads/`, so a tag named like the default branch (tags auto-follow + on fetch) supplied an authorization value the branch never carried. Verified. + `get` also declines when the checkout is not on that branch, since + `commit_push` would refuse anyway, and fails **closed** when the branch cannot + be resolved rather than falling back to HEAD. - `commit_push` passes `:(literal)` pathspecs: a task named `x*` would otherwise glob `tasks/archive/x*.md` and commit every matching archive. **Not** for `check-ignore`, which rejects pathspec magic outright — prefixing it there diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cbe1a8..7a31ba0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,7 +54,7 @@ entries are grouped per plugin, newest first. ### 1.10.0 — 2026-07-24 - `/close` step 10's commit+push prompt is now skippable per repo: a committed `.claude/work-system-close-autocommit` flag (mirrors the `.claude/work-system-agent` default precedent) routes straight to `archive-task.sh commit-push` — no `AskUserQuestion` — and reports the result exactly as the manual path does. Off by default; unset repos keep today's ask-once behavior. Per-repo only, no global default. `archive-task.sh` grew an `autocommit get|set|unset` subcommand as the single source of truth for the flag. -- The flag is honored **only once committed**: `get` reads the value from the committed object (`git show HEAD:`), never from the working tree, so a file a tool or a worktree agent merely wrote cannot waive the prompt — and neither can a working-tree edit hidden behind `git update-index --assume-unchanged`/`--skip-worktree`, which fools a diff-based guard. A locally edited flag still falls back to asking, so deliberate local disabling works. Scope stated honestly: this raises the bar from "any file write" to "a commit", not to "human-reviewed"; `commit-push`'s own guards (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed history) are what bound the damage. +- The flag is honored **only once committed**: `get` reads the value from the committed object on the default branch (`git show refs/heads/:` — fully qualified, so a same-named tag cannot shadow the branch), never from the working tree, so a file a tool or a worktree agent merely wrote cannot waive the prompt — and neither can a working-tree edit hidden behind `git update-index --assume-unchanged`/`--skip-worktree`, which fools a diff-based guard. A locally edited flag still falls back to asking, so deliberate local disabling works. Scope stated honestly: this raises the bar from "any file write" to "a commit", not to "human-reviewed"; `commit-push`'s own guards (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed history) are what bound the damage. - The verdict is read from **``**, not `HEAD`: the flag authorizes a commit onto that branch (`commit-push` refuses any other), so a flag living only on some feature branch no longer announces an auto-commit that then fails. A repo that never opted in answers a bare `enabled=no`; every other non-honored case adds a machine `reason=` and a ready-to-print `note=` that callers relay verbatim instead of duplicating the vocabulary in prose. Accepted content is `yes`/`true`/`1`/`on` (case-insensitive, trimmed, single line); `no`/`false`/`0`/`off` disables, and a deliberate local disable is reported neutrally rather than being told to revert itself. `/close` treats **anything that is not literally `enabled=yes`** — including empty output from an older installed plugin without the subcommand — as "ask", so the check can never fail open, and it prints the same scoped `git status` preview on the auto path: standing authorization is not authorization to act unseen. - `commit-push` now passes its paths as `:(literal)` pathspecs, so a task name containing glob characters (`x*`) can no longer sweep other archived files into the commit. Both writers in the script use `mktemp` rather than a PID-derived temp name. - Path handling: resolution delegates to `main-repo-path.sh` (the same resolver `/close` uses, so `set` can't write where `get` won't look); `set`/`unset` refuse a symlinked flag **or `.claude` parent** and verify `.claude` resolves inside the repo; `set` writes via an exclusively-created `mktemp` file in the target directory instead of a PID-predictable `.tmp.$$` path, and echoes the resolved `flag=`. diff --git a/plugins/work-system/scripts/archive-task.sh b/plugins/work-system/scripts/archive-task.sh index 2101ae6..eefe757 100755 --- a/plugins/work-system/scripts/archive-task.sh +++ b/plugins/work-system/scripts/archive-task.sh @@ -57,10 +57,12 @@ # actor who can already commit in the repo. commit-push's own guards # (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed # history) are what bound the damage in that case. -# A repo that never opted in answers a BARE `enabled=no`. Otherwise two -# extra lines say why it was not honored: `reason=` one of +# A repo that never opted in answers a BARE `enabled=no` — that bare form +# is reserved for exactly that case. Every other non-honored outcome adds +# two lines saying why: `reason=` one of # symlink | not-a-file | untracked | modified | locally-disabled | -# unrecognized-content | git-error | unsupported-layout | not-a-repo, plus +# unrecognized-content | git-error | unsupported-layout | not-a-repo | +# unresolvable-branch | checkout-not-on-main, plus # `note=` — callers relay the note rather than # re-spelling this vocabulary in their own prose (it drifts). # Accepted content (case-insensitive, leading/trailing whitespace trimmed, @@ -72,13 +74,15 @@ # pre-plant), refusing when the flag OR its `.claude` parent is a symlink or # resolves outside the repo. Echoes `flag=` plus a reminder that it # takes effect only once COMMITTED. -# autocommit unset +# autocommit unset [] # Remove the repo's `.claude/work-system-close-autocommit` (same path -# guards), reverting to the default ask-once behavior. Echoes `flag=`. +# guards), reverting to the default ask-once behavior. Echoes `flag=`, +# plus a reminder to commit the deletion when the flag is still live on the +# authorization ref (pass so that check matches get's). # All three resolve to the MAIN checkout by delegating to # main-repo-path.sh — the same resolver /close uses, so `set` can never write -# where `get` doesn't look. A non-git path is a usage error for set/unset and a -# plain `enabled=no` for get. +# where `get` doesn't look. A non-git path is a usage error for set/unset, and +# for get an `enabled=no` carrying `reason=not-a-repo` + `note=`. # # `archive` output: key=value lines (paths relative to the main repo) — # archived_path=tasks/archive/[-N].md @@ -101,6 +105,17 @@ set -eu # whatever happens to sit at that path there. SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# `:(literal)` pathspec prefix. Task names flow in from task files and reach git +# as PATHSPECS, where `*`/`?`/`[` are globs: a task named `x*` yields +# `tasks/archive/x*.md`, matching every archived file with that prefix. The rule +# is NOT "mutating commands only" — it is: prefix every git command that +# INTERPRETS pathspec magic (add, commit, diff, ls-files), and omit it for +# `check-ignore`, which rejects magic outright ("pathspec magic not supported by +# this command") and would fail every probe if prefixed. Drifting either way is a +# real bug: unprefixed `ls-files` mis-detects a neighbouring file, prefixed +# `check-ignore` breaks gitignore detection entirely. +lit() { printf ':(literal)%s' "$1"; } + archive() { local repo="${1:-}" name="${2:-}" branch="${3:-}" if [ -z "$repo" ] || [ -z "$name" ] || [ -z "$branch" ]; then @@ -179,8 +194,19 @@ archive() { echo "failed to write archive $dest" >&2 exit 1 fi - chmod 644 "$tmp" 2>/dev/null || true # mktemp creates 600; the archive is repo content + # mktemp creates 600; the archive is ordinary repo content, so restore the mode + # the caller's umask would have produced for a plain redirect — do NOT force + # 644, which would override a deliberately stricter umask and make archived + # task files world-readable in a private checkout. + chmod "$(printf '%o' "$(( 0666 & ~0$(umask) ))")" "$tmp" 2>/dev/null || true mv "$tmp" "$dest" + # The source is removed next, so make sure a REAL archive is in place first: if + # the destination is not a regular file (a swapped-in symlink, an interrupted + # rename), removing the source would destroy the task with no copy left. + if [ ! -f "$dest" ] || [ -L "$dest" ]; then + echo "archive at $dest is not a regular file — refusing to remove the original" >&2 + exit 1 + fi # Remove the original BEFORE recording the index, rolling the moved archive back # if the remove fails (immutable/locked source, read-only parent). This keeps the @@ -212,7 +238,7 @@ archive() { if git -C "$repo" rev-parse --git-dir >/dev/null 2>&1; then if ! git -C "$repo" check-ignore -q "tasks/archive/$base"; then committable="yes" - elif git -C "$repo" ls-files --error-unmatch -- "tasks/$safe.md" >/dev/null 2>&1; then + elif git -C "$repo" ls-files --error-unmatch -- "$(lit "tasks/$safe.md")" >/dev/null 2>&1; then committable="yes" fi fi @@ -247,18 +273,7 @@ commit_push() { # only when not gitignored (`git add` errors on an ignored path), and the # original's removal only when it was tracked. # - # The MUTATING commands get the `:(literal)` pathspec prefix. git otherwise - # reads these as GLOBS, and the task name flows in from the task file: a task - # named `x*` yields `tasks/archive/x*.md`, which would match — and commit — - # every archived file with that prefix. Literal magic makes the pathspec mean - # exactly the one file it names. (`lit()` keeps the raw path for comparisons.) - # - # NOT for `check-ignore`, which rejects pathspec magic outright ("pathspec - # magic not supported by this command") — prefixing it there made every probe - # fail, so `archive_ignored` was stuck at `no` and a gitignored archive would - # have hit `git add`'s error path instead. It is a read-only probe, so a glob - # there cannot commit anything. - lit() { printf ':(literal)%s' "$1"; } + # Pathspec handling: see lit()'s contract at the top of the file. local archive_ignored=no if git -C "$repo" check-ignore -q -- "$archived"; then archive_ignored=yes; fi local paths=() @@ -363,6 +378,26 @@ autocommit_root() { return 0 } +# Normalize a flag value to one token: `on`, `off`, or `bad`. ONE implementation +# for both the committed value and the working-tree copy — they had drifted apart +# (only one rejected multi-line content), which is how a value accepted in one +# place and rejected in the other becomes possible. +# Trim leading/trailing whitespace ONLY: deleting all whitespace would collapse +# `y e s` into an accepted token. Multi-line is never a valid flag. +autocommit_normalize() { + local raw="$1" v + case "$raw" in + *" +"*) printf 'bad\n'; return 0 ;; + esac + v="$(printf '%s' "$raw" | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' | tr '[:upper:]' '[:lower:]')" + case "$v" in + yes|true|1|on) printf 'on\n' ;; + no|false|0|off) printf 'off\n' ;; + *) printf 'bad\n' ;; + esac +} + # Explain WHY the main checkout could not be resolved. "Not a git repository" is # wrong (and sends the user hunting in the wrong direction) when the path IS in a # repo whose layout the resolver can't handle — e.g. a separate-git-dir checkout, @@ -420,10 +455,10 @@ autocommit() { local op="${1:-}" repo="${2:-}" mainbr="${3:-}" case "$op" in get|set|unset) ;; - *) echo "usage: ${0##*/} autocommit {get|set|unset} " >&2; exit 2 ;; + *) echo "usage: ${0##*/} autocommit {get|set|unset} []" >&2; exit 2 ;; esac if [ -z "$repo" ]; then - echo "usage: ${0##*/} autocommit $op " >&2; exit 2 + echo "usage: ${0##*/} autocommit $op [] (branch omitted: get/unset fall back to HEAD)" >&2; exit 2 fi local rel=".claude/work-system-close-autocommit" @@ -490,14 +525,37 @@ autocommit() { # WHICH REF: the archive lands on , and commit_push refuses to # commit anywhere else, so the authorization must be read from that same # branch — not from whatever the main checkout happens to be parked on. - # Reading HEAD made the verdict depend on the current branch: a checkout on - # an unrelated branch reported "not committed" for a flag that IS committed - # on main, and a checkout on a branch carrying the flag announced - # "auto-committing" before commit_push bailed out with wrong-branch. - # Falls back to HEAD when no branch is passed (standalone/manual use). - local ref="HEAD" - if [ -n "$mainbr" ] && git -C "$root" rev-parse --verify -q "$mainbr" >/dev/null 2>&1; then - ref="$mainbr" + # + # FULLY QUALIFIED (`refs/heads/`), never the bare name: git resolves + # `refs/tags/` BEFORE `refs/heads/`, so a tag named like the + # default branch shadows it — and tags are auto-followed on fetch. A bare + # `$mainbr` therefore let a tag supply the authorization value that the + # branch itself never carried. Verified. + local ref="" + if [ -n "$mainbr" ]; then + if git -C "$root" rev-parse --verify -q "refs/heads/$mainbr" >/dev/null 2>&1; then + ref="refs/heads/$mainbr" + else + # FAIL CLOSED. Falling back to HEAD here would reinstate exactly the + # branch-dependent verdict this argument exists to remove — and in the + # failing direction (a flag on the parked branch would read as + # authorization for a branch that has none). + printf 'enabled=no\nreason=unresolvable-branch\nnote=%s\n' \ + "the authorization branch could not be resolved in this repository — falling back to asking" + return 0 + fi + # commit_push refuses unless the checkout is ON that branch, so an + # `enabled=yes` here would only announce an auto-commit the next command + # declines. Answer for what can actually happen. + local cur; cur="$(git -C "$root" branch --show-current 2>/dev/null || true)" + if [ "$cur" != "$mainbr" ]; then + printf 'enabled=no\nreason=checkout-not-on-main\nnote=%s\n' \ + "the main checkout is not on the branch the archive would be committed to — falling back to asking" + return 0 + fi + else + # No branch passed (standalone/manual use): HEAD is the only sensible ref. + ref="HEAD" fi local head_val rc=0 head_val="$(git -C "$root" show "$ref:$rel" 2>/dev/null)" || rc=$? @@ -533,40 +591,22 @@ autocommit() { # is neither the committed value nor an explicit off gets the corrective # wording. local local_val - local_val="$(sed 's/^[[:space:]]*//; s/[[:space:]]*$//' < "$file" 2>/dev/null | tr '[:upper:]' '[:lower:]' || true)" - case "$local_val" in - no|false|0|off) - printf 'enabled=no\nreason=locally-disabled\nnote=%s\n' \ - "the autocommit opt-in is switched off in your working copy (the committed flag is unchanged)" ;; - *) - printf 'enabled=no\nreason=modified\nnote=%s\n' \ - "the autocommit flag differs from the committed version — commit or revert it to make it effective" ;; - esac + local_val="$(autocommit_normalize "$(cat "$file" 2>/dev/null || true)")" + if [ "$local_val" = off ]; then + printf 'enabled=no\nreason=locally-disabled\nnote=%s\n' \ + "the autocommit opt-in is switched off in your working copy (the committed flag is unchanged)" + else + printf 'enabled=no\nreason=modified\nnote=%s\n' \ + "the autocommit flag differs from the committed version — commit or revert it to make it effective" + fi return 0 fi - # TRIM, not "delete every space": `tr -d '[:space:]'` would collapse `y e s` - # (or a value split across lines) into an accepted token, honoring content - # that matches no documented value. Strip only leading/trailing whitespace, - # and treat anything multi-line as unrecognized. Case-insensitive, because - # the file is hand-edited by design and `Yes`/`TRUE` must not read as "off". - local content - case "$head_val" in - # Pure-shell newline test — a `grep -c ''` pipeline exits 1 on empty - # input, so an `|| echo 0` fallback fires on TOP of grep's own "0" and - # yields the two-line string "0\n0", which `[ -gt ]` then rejects with a - # bash error on stderr mid-close. - *" -"*) content="__multiline__" ;; - *) content="$(printf '%s' "$head_val" \ - | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ - | tr '[:upper:]' '[:lower:]')" ;; - esac - case "$content" in - yes|true|1|on) printf 'enabled=yes\n' ;; - no|false|0|off) printf 'enabled=no\n' ;; - *) printf 'enabled=no\nreason=unrecognized-content\nnote=%s\n' \ - "the autocommit flag's content is not a recognized value (expected yes/true/1/on)" ;; + case "$(autocommit_normalize "$head_val")" in + on) printf 'enabled=yes\n' ;; + off) printf 'enabled=no\n' ;; + *) printf 'enabled=no\nreason=unrecognized-content\nnote=%s\n' \ + "the autocommit flag's content is not a recognized value (expected yes/true/1/on)" ;; esac ;; set) @@ -618,8 +658,15 @@ autocommit() { # delete a file outside the repo. (`rm` on a symlinked LEAF is safe — it # removes the link, not its target — but a symlinked parent is not.) autocommit_guard_paths "$root" "$file" unset || exit 2 + # Check the SAME ref `get` authorizes from — a HEAD-based check skipped the + # reminder in exactly the case that needs it (flag still live on the + # authorization branch while the checkout sits elsewhere). + local unset_ref="HEAD" + if [ -n "$mainbr" ] && git -C "$root" rev-parse --verify -q "refs/heads/$mainbr" >/dev/null 2>&1; then + unset_ref="refs/heads/$mainbr" + fi local was_committed=no - git -C "$root" cat-file -e "HEAD:$rel" 2>/dev/null && was_committed=yes + git -C "$root" cat-file -e "$unset_ref:$rel" 2>/dev/null && was_committed=yes rm -f "$file" printf 'flag=%s\n' "$file" # Deleting the file makes `get` report disabled immediately, which LOOKS @@ -636,5 +683,5 @@ case "${1:-}" in archive) shift; archive "$@" ;; commit-push) shift; commit_push "$@" ;; autocommit) shift; autocommit "$@" ;; - *) echo "usage: ${0##*/} {archive [--pr ] [--sha ] | commit-push | autocommit {get|set|unset} }" >&2; exit 2 ;; + *) echo "usage: ${0##*/} {archive [--pr ] [--sha ] | commit-push | autocommit {get|set|unset} []}" >&2; exit 2 ;; esac diff --git a/plugins/work-system/scripts/test_archive_task.py b/plugins/work-system/scripts/test_archive_task.py index 1dbec9a..23b454d 100644 --- a/plugins/work-system/scripts/test_archive_task.py +++ b/plugins/work-system/scripts/test_archive_task.py @@ -363,10 +363,18 @@ def commit_flag(repo, msg="add flag"): (brepo / REL).write_text("yes\n") git(brepo, "add", REL) git(brepo, "commit", "-qm", "flag on feature only") - check("flag only on a feature branch -> not honored for the default branch", - kv(run("autocommit", "get", str(brepo), default_branch).stdout).get("enabled") == "no") - check("...and says it is not committed there", - kv(run("autocommit", "get", str(brepo), default_branch).stdout).get("reason") == "untracked") + # Checked out ON the feature branch: commit_push would refuse anyway, so the + # unattended path must not be announced. + g = kv(run("autocommit", "get", str(brepo), default_branch).stdout) + check("flag only on a feature branch -> not honored", g.get("enabled") == "no") + check("...because the checkout is not on the authorization branch", + g.get("reason") == "checkout-not-on-main") + # Back on the default branch the flag file isn't even in the working tree + # (it lives only on `feature`), so this is a plain "never opted in" answer. + git(brepo, "checkout", "-q", default_branch) + g = kv(run("autocommit", "get", str(brepo), default_branch).stdout) + check("flag absent on the default branch -> bare enabled=no", g.get("enabled") == "no") + check("...with no reason line", "reason" not in g) # committed on the default branch -> honored git(brepo, "checkout", "-q", default_branch) (brepo / ".claude").mkdir(exist_ok=True) # tracked only on `feature` until now @@ -376,6 +384,37 @@ def commit_flag(repo, msg="add flag"): check("flag on the default branch -> honored", kv(run("autocommit", "get", str(brepo), default_branch).stdout).get("enabled") == "yes") + # --- REGRESSION: a TAG must not shadow the authorization branch ----------- # + # git resolves refs/tags/ BEFORE refs/heads/, and tags are + # auto-followed on fetch — so a bare refname let a tag supply an + # authorization value the branch never carried. + trepo2 = new_repo(root, "trepo2") + tbranch = git(trepo2, "rev-parse", "--abbrev-ref", "HEAD").stdout.strip() + (trepo2 / ".claude").mkdir() + (trepo2 / REL).write_text("yes\n") + git(trepo2, "add", REL) + git(trepo2, "commit", "-qm", "flag on") + git(trepo2, "tag", "flagged-on") + (trepo2 / REL).write_text("no\n") + git(trepo2, "add", REL) + git(trepo2, "commit", "-qm", "flag off") + # a tag named exactly like the branch, pointing at the "yes" commit + git(trepo2, "tag", "-f", tbranch, "flagged-on") + check("branch really says off", + git(trepo2, "show", f"refs/heads/{tbranch}:{REL}").stdout.strip() == "no") + check("a same-named tag really shadows the branch for a bare refname", + git(trepo2, "show", f"{tbranch}:{REL}").stdout.strip() == "yes") + check("tag cannot supply the authorization", + kv(run("autocommit", "get", str(trepo2), tbranch).stdout).get("enabled") == "no") + + # --- REGRESSION: an unresolvable authorization branch fails CLOSED -------- # + # Falling back to HEAD here would reinstate the branch-dependent verdict in + # the failing direction. + g = kv(run("autocommit", "get", str(brepo), "no-such-branch").stdout) + check("unresolvable branch -> enabled=no", g.get("enabled") == "no") + check("unresolvable branch -> reason=unresolvable-branch", + g.get("reason") == "unresolvable-branch") + # --- REGRESSION: a deliberate local disable gets neutral wording ---------- # # "commit or revert it to make it effective" would tell the user to undo the # very thing they just did, on every close. diff --git a/plugins/work-system/skills/close/SKILL.md b/plugins/work-system/skills/close/SKILL.md index 3ea18b3..d2a785e 100644 --- a/plugins/work-system/skills/close/SKILL.md +++ b/plugins/work-system/skills/close/SKILL.md @@ -180,8 +180,12 @@ Rules: Pass ``: the flag authorizes a commit onto *that* branch, so it is read from it. Without it the answer would follow whatever branch the main checkout happens to be on, which can disagree with where `commit-push` will actually commit. - - **`enabled=yes`** — and *only* this exact literal — means the repo carries a - **committed** `.claude/work-system-close-autocommit`. That file IS the durable + - **A line that is exactly `enabled=yes`** — match the **whole line**, never a + substring of the output: other lines (notably `note=`) can legitimately contain a + branch name or other repo-controlled text, and a substring scan would let that text + supply the authorization token. Anything short of a standalone `enabled=yes` line is + not authorization. It means the repo carries a **committed** + `.claude/work-system-close-autocommit`. That file IS the durable per-repo authorization for exactly this one narrow, contained action (archive-scoped pathspec commit, fast-forward only, never force-pushes, and it refuses to push on `unpushed-history`). Skip the AskUserQuestion below and go straight to the From 62d18db4a711a168c35a8b25a036991c69424ad8 Mon Sep 17 00:00:00 2001 From: Robert Gering Date: Tue, 28 Jul 2026 14:28:10 +0200 Subject: [PATCH 4/4] Distinguish a locally-deleted autocommit flag, and test the glob guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the external-only (codex + grok) control review. - get: "nothing configured" now means nothing here AND nothing on the authorization ref. A flag deleted locally but still committed there reports reason=locally-deleted with a note that other checkouts keep auto-committing — it was answering with the bare enabled=no the header reserves for never-opted-in repos - extract autocommit_ref(): one place resolving refs/heads/, reused by get and unset instead of two copies - rename head_val/head_oid -> ref_val/ref_oid and fix the comment that still claimed `git show HEAD:`; the value has come from the authorization branch since the tag-shadowing fix - test the :(literal) pathspec hardening: a task literally named `x*` plus decoy archives it would glob-match. The first cut of this test was vacuous (task named `x-star` — no metacharacter, passed with the hardening removed); the committed version fails when lit() is neutralized, which was verified both ways Not applied: the claim that the blob-hash dirty-check breaks under core.autocrlf/eol=crlf. `git hash-object ` applies that path's filters by default, so the hashes match — checked directly. --- .../features/task-archiving-on-close.md | 10 +- CHANGELOG.md | 2 +- plugins/work-system/scripts/archive-task.sh | 100 ++++++++++++------ .../work-system/scripts/test_archive_task.py | 45 +++++++- 4 files changed, 117 insertions(+), 40 deletions(-) diff --git a/.claude/knowledge/features/task-archiving-on-close.md b/.claude/knowledge/features/task-archiving-on-close.md index 9aed694..9532855 100644 --- a/.claude/knowledge/features/task-archiving-on-close.md +++ b/.claude/knowledge/features/task-archiving-on-close.md @@ -189,7 +189,15 @@ persistent `cd`" footgun the helper's explicit paths avoid is a rule — see - A repo that never opted in must answer a **bare** `enabled=no`: callers relay any `note=` verbatim, so ordering the symlink check before the existence check made every close in a symlinked-`.claude` repo warn about an opt-in nobody - configured. + configured. "Nothing configured" means nothing here **and** nothing on the + authorization ref — a flag deleted locally but still committed there is + `locally-deleted`, since every other clone keeps auto-committing until the + deletion is committed. + - `git hash-object ` applies that path's gitattributes/clean filters by + default (that is what `--no-filters` disables), so the blob-hash dirty-check + stays correct under `core.autocrlf`/`eol=crlf` — checked directly after a + review round claimed otherwise. Verify such claims against git rather than + taking them on trust; the reviewer was wrong here. - Sibling helpers are addressed via an absolute `SCRIPT_DIR` captured at load: a `$(dirname "$0")` expanded inside `( cd "$repo" && … )` resolves against the *target* repo when `$0` is relative — and runs whatever sits there. diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a31ba0..691b17e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,7 @@ entries are grouped per plugin, newest first. - `/close` step 10's commit+push prompt is now skippable per repo: a committed `.claude/work-system-close-autocommit` flag (mirrors the `.claude/work-system-agent` default precedent) routes straight to `archive-task.sh commit-push` — no `AskUserQuestion` — and reports the result exactly as the manual path does. Off by default; unset repos keep today's ask-once behavior. Per-repo only, no global default. `archive-task.sh` grew an `autocommit get|set|unset` subcommand as the single source of truth for the flag. - The flag is honored **only once committed**: `get` reads the value from the committed object on the default branch (`git show refs/heads/:` — fully qualified, so a same-named tag cannot shadow the branch), never from the working tree, so a file a tool or a worktree agent merely wrote cannot waive the prompt — and neither can a working-tree edit hidden behind `git update-index --assume-unchanged`/`--skip-worktree`, which fools a diff-based guard. A locally edited flag still falls back to asking, so deliberate local disabling works. Scope stated honestly: this raises the bar from "any file write" to "a commit", not to "human-reviewed"; `commit-push`'s own guards (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed history) are what bound the damage. - The verdict is read from **``**, not `HEAD`: the flag authorizes a commit onto that branch (`commit-push` refuses any other), so a flag living only on some feature branch no longer announces an auto-commit that then fails. A repo that never opted in answers a bare `enabled=no`; every other non-honored case adds a machine `reason=` and a ready-to-print `note=` that callers relay verbatim instead of duplicating the vocabulary in prose. Accepted content is `yes`/`true`/`1`/`on` (case-insensitive, trimmed, single line); `no`/`false`/`0`/`off` disables, and a deliberate local disable is reported neutrally rather than being told to revert itself. `/close` treats **anything that is not literally `enabled=yes`** — including empty output from an older installed plugin without the subcommand — as "ask", so the check can never fail open, and it prints the same scoped `git status` preview on the auto path: standing authorization is not authorization to act unseen. -- `commit-push` now passes its paths as `:(literal)` pathspecs, so a task name containing glob characters (`x*`) can no longer sweep other archived files into the commit. Both writers in the script use `mktemp` rather than a PID-derived temp name. +- `commit-push` now passes its paths as `:(literal)` pathspecs, so a task name containing glob characters (`x*`) can no longer sweep other archived files into the commit — covered by a regression test that fails when the prefix is removed. Both writers in the script use `mktemp` rather than a PID-derived temp name. - Path handling: resolution delegates to `main-repo-path.sh` (the same resolver `/close` uses, so `set` can't write where `get` won't look); `set`/`unset` refuse a symlinked flag **or `.claude` parent** and verify `.claude` resolves inside the repo; `set` writes via an exclusively-created `mktemp` file in the target directory instead of a PID-predictable `.tmp.$$` path, and echoes the resolved `flag=`. ### 1.9.4 — 2026-07-24 diff --git a/plugins/work-system/scripts/archive-task.sh b/plugins/work-system/scripts/archive-task.sh index eefe757..f8d27a3 100755 --- a/plugins/work-system/scripts/archive-task.sh +++ b/plugins/work-system/scripts/archive-task.sh @@ -58,11 +58,13 @@ # (archive-scoped pathspec, ff-only, never force-push, refusal on unpushed # history) are what bound the damage in that case. # A repo that never opted in answers a BARE `enabled=no` — that bare form -# is reserved for exactly that case. Every other non-honored outcome adds -# two lines saying why: `reason=` one of +# is reserved for exactly that case (nothing here AND nothing on the +# authorization ref; a flag deleted locally but still committed there is +# `locally-deleted`, not "never opted in"). Every other non-honored outcome +# adds two lines saying why: `reason=` one of # symlink | not-a-file | untracked | modified | locally-disabled | -# unrecognized-content | git-error | unsupported-layout | not-a-repo | -# unresolvable-branch | checkout-not-on-main, plus +# locally-deleted | unrecognized-content | git-error | unsupported-layout | +# not-a-repo | unresolvable-branch | checkout-not-on-main, plus # `note=` — callers relay the note rather than # re-spelling this vocabulary in their own prose (it drifts). # Accepted content (case-insensitive, leading/trailing whitespace trimmed, @@ -378,6 +380,22 @@ autocommit_root() { return 0 } +# Resolve the ref the authorization is read from. FULLY QUALIFIED +# (`refs/heads/`), never the bare name: git resolves `refs/tags/` +# BEFORE `refs/heads/`, so a tag named like the default branch shadows it — +# and tags are auto-followed on fetch, so a bare `$mainbr` let a tag supply an +# authorization value the branch never carried. Verified. +# Prints the ref, or NOTHING when a branch was passed but does not resolve (the +# caller fails closed on that). With no branch, HEAD — standalone/manual use. +autocommit_ref() { + local root="$1" mainbr="$2" + if [ -z "$mainbr" ]; then printf 'HEAD\n'; return 0; fi + if git -C "$root" rev-parse --verify -q "refs/heads/$mainbr" >/dev/null 2>&1; then + printf 'refs/heads/%s\n' "$mainbr" + fi + return 0 +} + # Normalize a flag value to one token: `on`, `off`, or `bad`. ONE implementation # for both the committed value and the working-tree copy — they had drifted apart # (only one rejected multi-line content), which is how a value accepted in one @@ -486,12 +504,31 @@ autocommit() { fi return 0 fi + # Resolve the authorization ref up front — the "nothing configured" test + # below needs it, not just the value read further down. + local ref; ref="$(autocommit_ref "$root" "$mainbr")" + local on_ref=no + if [ -n "$ref" ] && git -C "$root" cat-file -e "$ref:$rel" 2>/dev/null; then on_ref=yes; fi + # NOTHING CONFIGURED comes first: a repo that never opted in must answer a # bare `enabled=no`, with no reason/note. Testing the symlink before this # made every /close in a repo with a symlinked `.claude` (a normal dotfiles # layout) warn about an opt-in the user never set up — and callers are told # a note means "an existing flag was not honored". - if [ ! -e "$file" ] && [ ! -L "$file" ]; then printf 'enabled=no\n'; return 0; fi + # + # "Nothing configured" means nothing HERE *and* nothing on the authorization + # ref. A flag that is merely deleted locally while still committed there is + # not the same thing: this checkout is fail-safe, but every other clone keeps + # auto-committing until the deletion is committed — so say so instead of + # answering with the form reserved for never-opted-in repos. + if [ ! -e "$file" ] && [ ! -L "$file" ]; then + if [ "$on_ref" = yes ]; then + printf 'enabled=no\nreason=locally-deleted\nnote=%s\n' \ + "the autocommit flag is deleted here but still committed on the authorization branch — commit the deletion, or other checkouts keep auto-committing" + return 0 + fi + printf 'enabled=no\n'; return 0 + fi # A symlinked flag — or a symlinked `.claude` parent — means the on-disk # state is redirected; never honor it. (The value itself comes from the # committed object below, so this cannot change the answer, but a redirected @@ -508,12 +545,14 @@ autocommit() { fi # PROVENANCE — presence alone is NOT authorization. Read the value from the - # COMMITTED object (`git show HEAD:`), never from the working tree: - # that single call is both the tracked-check (it fails when the path isn't - # in HEAD) and the value read, and it is immune to the working-tree tricks - # that fool a `diff`-based guard — `git update-index --assume-unchanged` / - # `--skip-worktree` make `ls-files`/`diff --quiet` report a modified file as - # clean, but they cannot change what HEAD contains. + # COMMITTED object on the authorization ref (`git show "$ref:"`, where + # $ref is `refs/heads/`; HEAD only in standalone/no-branch + # use), never from the working tree: that single call is both the + # committed-check (it fails when the path isn't on that ref) and the value + # read, and it is immune to the working-tree tricks that fool a `diff`-based + # guard — `git update-index --assume-unchanged` / `--skip-worktree` make + # `ls-files`/`diff --quiet` report a modified file as clean, but they cannot + # change what the ref contains. # # Scope of this guard (documented honestly — see the knowledge entry): it # stops a flag that was merely WRITTEN (a tool, a stray file, an agent's @@ -526,16 +565,9 @@ autocommit() { # commit anywhere else, so the authorization must be read from that same # branch — not from whatever the main checkout happens to be parked on. # - # FULLY QUALIFIED (`refs/heads/`), never the bare name: git resolves - # `refs/tags/` BEFORE `refs/heads/`, so a tag named like the - # default branch shadows it — and tags are auto-followed on fetch. A bare - # `$mainbr` therefore let a tag supply the authorization value that the - # branch itself never carried. Verified. - local ref="" + # $ref was resolved above (see autocommit_ref for why it is fully qualified). if [ -n "$mainbr" ]; then - if git -C "$root" rev-parse --verify -q "refs/heads/$mainbr" >/dev/null 2>&1; then - ref="refs/heads/$mainbr" - else + if [ -z "$ref" ]; then # FAIL CLOSED. Falling back to HEAD here would reinstate exactly the # branch-dependent verdict this argument exists to remove — and in the # failing direction (a flag on the parked branch would read as @@ -553,12 +585,9 @@ autocommit() { "the main checkout is not on the branch the archive would be committed to — falling back to asking" return 0 fi - else - # No branch passed (standalone/manual use): HEAD is the only sensible ref. - ref="HEAD" fi - local head_val rc=0 - head_val="$(git -C "$root" show "$ref:$rel" 2>/dev/null)" || rc=$? + local ref_val rc=0 + ref_val="$(git -C "$root" show "$ref:$rel" 2>/dev/null)" || rc=$? if [ "$rc" -ne 0 ]; then # Not on that ref (never committed there), or git could not read it at # all. Both are "not authorized"; distinguish them so debugging isn't @@ -575,16 +604,19 @@ autocommit() { # The committed value is what authorizes, so a local edit can never silently # ENABLE the skip. A local edit SHOULD still be able to disable it (that is # a deliberate act by whoever holds the checkout), so a flag whose working - # copy differs from HEAD falls back to asking. + # copy differs from the authorization ref falls back to asking. # # Compare BLOB HASHES, not `git diff --quiet`: the index bits that hide a # dirty file (`--assume-unchanged`, `--skip-worktree`) make diff report # "clean", which would silently ignore a deliberate local disable. Hashing - # the file and the HEAD blob is index-independent, so both directions hold. - local head_oid work_oid - head_oid="$(git -C "$root" rev-parse "$ref:$rel" 2>/dev/null || true)" + # the file and the ref blob is index-independent, so both directions hold. + # `git hash-object ` applies that path's gitattributes/clean filters + # by default (that is what `--no-filters` turns off), so an eol=crlf or + # text=auto checkout still compares equal — verified, not assumed. + local ref_oid work_oid + ref_oid="$(git -C "$root" rev-parse "$ref:$rel" 2>/dev/null || true)" work_oid="$(git -C "$root" hash-object -- "$file" 2>/dev/null || true)" - if [ -z "$work_oid" ] || [ "$work_oid" != "$head_oid" ]; then + if [ -z "$work_oid" ] || [ "$work_oid" != "$ref_oid" ]; then # Deliberately disabling locally is a SUPPORTED move, so don't answer it # with "commit or revert it to make it effective" — that tells the user to # undo the thing they just did, on every single close. Only content that @@ -602,7 +634,7 @@ autocommit() { return 0 fi - case "$(autocommit_normalize "$head_val")" in + case "$(autocommit_normalize "$ref_val")" in on) printf 'enabled=yes\n' ;; off) printf 'enabled=no\n' ;; *) printf 'enabled=no\nreason=unrecognized-content\nnote=%s\n' \ @@ -661,10 +693,8 @@ autocommit() { # Check the SAME ref `get` authorizes from — a HEAD-based check skipped the # reminder in exactly the case that needs it (flag still live on the # authorization branch while the checkout sits elsewhere). - local unset_ref="HEAD" - if [ -n "$mainbr" ] && git -C "$root" rev-parse --verify -q "refs/heads/$mainbr" >/dev/null 2>&1; then - unset_ref="refs/heads/$mainbr" - fi + local unset_ref; unset_ref="$(autocommit_ref "$root" "$mainbr")" + [ -n "$unset_ref" ] || unset_ref="HEAD" local was_committed=no git -C "$root" cat-file -e "$unset_ref:$rel" 2>/dev/null && was_committed=yes rm -f "$file" diff --git a/plugins/work-system/scripts/test_archive_task.py b/plugins/work-system/scripts/test_archive_task.py index 23b454d..134fae5 100644 --- a/plugins/work-system/scripts/test_archive_task.py +++ b/plugins/work-system/scripts/test_archive_task.py @@ -273,9 +273,15 @@ def commit_flag(repo, msg="add flag"): check("unset exits 0", r.returncode == 0) check("unset removes the flag", not flag.exists()) check("unset echoes the path", kv(r.stdout).get("flag") == str(flag)) - # removed but still tracked in HEAD -> the deletion is a modification - check("after unset -> enabled=no", - kv(run("autocommit", "get", str(repo)).stdout).get("enabled") == "no") + # Deleted here but STILL COMMITTED on the authorization ref: fail-safe for + # this checkout, but every other clone keeps auto-committing. That must not + # answer with the bare form reserved for never-opted-in repos. + g = kv(run("autocommit", "get", str(repo)).stdout) + check("after unset -> enabled=no", g.get("enabled") == "no") + check("after unset -> reason=locally-deleted", g.get("reason") == "locally-deleted") + check("after unset -> warns other checkouts still auto-commit", + "other checkouts" in g.get("note", "")) + git(repo, "checkout", "--", REL) # --- REGRESSION: relative invocation must not resolve the sibling helper --- # # `$(dirname "$0")` expanded inside `( cd "$repo" && … )` resolves against the @@ -471,6 +477,39 @@ def commit_flag(repo, msg="add flag"): check("bare autocommit -> exit 2", run("autocommit").returncode == 2) + # --- REGRESSION: a glob-y task name must not widen the commit ------------- # + # `archive`/`commit-push` pass task-derived paths to git as PATHSPECS, where + # `*` is a glob: without the `:(literal)` prefix a task named `x*` would stage + # and commit every `tasks/archive/x*.md`. Under the autocommit opt-in that + # commit is pushed to the default branch with no prompt at all, so this is + # worth an automated guard rather than the manual smoke test it had. + grepo = new_repo(root, "grepo") + gbranch = git(grepo, "rev-parse", "--abbrev-ref", "HEAD").stdout.strip() + (grepo / "tasks" / "archive").mkdir(parents=True) + # The task name itself carries the glob metacharacter — that is the whole + # point. `x*` archives to `tasks/archive/x*.md`, a pathspec that matches every + # `x…​.md` unless it is passed as :(literal). + (grepo / "tasks" / "x*.md").write_text("# Star\n") + for decoy in ("x-decoy-1.md", "x-decoy-2.md"): # both match the glob x*.md + (grepo / "tasks" / "archive" / decoy).write_text("# decoy\n") + git(grepo, "add", "-A", "tasks") + git(grepo, "commit", "-qm", "task + decoys") + # dirty the decoys, so a glob-widened pathspec WOULD have something to sweep in + for decoy in ("x-decoy-1.md", "x-decoy-2.md"): + (grepo / "tasks" / "archive" / decoy).write_text("# decoy touched\n") + + r = run("archive", str(grepo), "x*", "task/x-star") + archived = kv(r.stdout).get("archived_path") + check("glob-y task archives to its own file", archived == "tasks/archive/x*.md") + run("commit-push", str(grepo), "x*", archived, gbranch) + committed = git(grepo, "show", "--name-only", "--format=", "HEAD").stdout.split() + check("commit-push did commit the archive", "tasks/archive/x*.md" in committed) + check("commit-push swept in no decoy", + not any(f.startswith("tasks/archive/x-decoy") for f in committed)) + check("decoy edits stay uncommitted", + "x-decoy-1.md" in git(grepo, "status", "--porcelain").stdout) + + if FAILS: print("FAIL:") for f in FAILS: