Skip to content

v1.22.0: two steps that reported success having done nothing - #19

Merged
wan-huiyan merged 3 commits into
mainfrom
fix/24c-overwrite-and-24b-scope
Aug 7, 2026
Merged

v1.22.0: two steps that reported success having done nothing#19
wan-huiyan merged 3 commits into
mainfrom
fix/24c-overwrite-and-24b-scope

Conversation

@wan-huiyan

@wan-huiyan wan-huiyan commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Two steps in this skill reported success while having done nothing — the failure the skill exists to catch, in its own text. Running the repo's own gate to ship them turned up a third, in the gate.

1 — Step 24c silently destroyed step 24d's work

Mechanism, verified in the text. 24c writes ~/.claude/usage-tracking/<date>_<sid8>_<project>.{json,md} with >, never >> — both the cctime-fork branch and the session_metrics.py fallback. 24d then merges review_findings / review_summary into that same JSON and appends a table to that same .md. 24c's own subagent sanity-check instructs a re-run ("redirect its output over the fork's files"), and the numbers keep moving as a session continues, so refreshing near the end is the normal thing to do. The refresh replaces both files wholesale and nothing warns.

The record afterwards looks complete — tokens, models, transcript counts all present. It simply has no findings in it, and the cross-session roll-up they exist for is gone. Observed 2026-08-07: a session refreshed to pick up later subagents and wiped fifteen merged findings, caught only by listing the JSON's top-level keys afterwards.

Fix. 24c opens with the overwrite stated plainly and the ordering (24c first, then 24d, never the reverse; re-running 24c means re-applying 24d). The fence itself makes the refresh non-destructive: cp the record aside before the write, then a small python block — same shape as 24d's — merges the two fields back and prints the top-level keys. The print is the verification, not a comment. The Markdown table has no carry and must be re-appended by hand; that is said rather than glossed. 24d gains the matching ordering note and a read-it-back verification bullet, since review_findings merged: ... proves what was written, not what is still on disk.

Exercised end to end (24c write -> 24d merge -> 24c refresh):

top-level keys: ['model', 'review_findings', 'review_summary', 'subagents', 'tokens']

and the same refresh with the carry removed:

['tokens']

The new subagent totals and the findings both survive; without the carry only tokens does.

2 — The freshness audit passes without opening the file you edited

The mechanism is not the one first reported, so it is written down as measured. skill_freshness_audit.py has scanned both install roots since v1.17.0 — ~/.claude/skills/ and ~/.claude/plugins/cache/ — so "it only looks at ~/.claude/skills" is no longer true. The real gap is a third location: a plugin skill's source repo, a marketplace checkout under ~/.claude/plugins/marketplaces/<marketplace>/ or a clone anywhere on disk. That is the copy step 24e explicitly tells you to edit, and the audit never opens it.

Measured on the author's machine while making this very change:

  • 247 skills audited across the two install roots
  • session-handoff reported from ~/.claude/plugins/cache/wan-huiyan-session-handoff/session-handoff/1.20.0/SKILL.md
  • zero results from any marketplaces/ path
  • the source repo being edited was at 1.21.0

It passed, on the wrong file, one version behind. Two further reasons the pass means less than it looks: the trigger is git diff ... | grep SKILL.md in the current repo, so editing a plugin skill in a separate repo does not fire the audit at all; and the cache copy's age comes from mtime, which is the install time, so it reads 0d whatever the file contains.

Fix. 24b now states its scope, and gives the check that closes the gap when the edited SKILL.md is a plugin skill: verify it directly — the version bumped in every place that plugin's own repo records it, established by grepping the repo and reading its manifest test rather than from memory (five places here: SKILL.md frontmatter, plugin.json, marketplace.json, package.json, and the newest README Version History entry, all five enforced by tests/manifest-consistency.test.mjs) — and that repo's own gates run. Then the general form, which is the more useful half:

An audit that reports "all clean" without naming what it examined has told you nothing.

Prefer a check that prints its scope, and compare that scope against the path of the thing you changed. skill_freshness_audit.py --human does print its roots on its first line — read that rather than the "All skills fresh." at the bottom. Same failure this skill already carries twice from v1.17.0.

3 — The pre-push gate was corrupting the repo it guards

Not in the original brief. Found by running this repo's own gate to ship the two fixes above, and it is the same shape as both: green, confident, and destructive.

Git exports GIT_DIR, GIT_INDEX_FILE and friends into every hook process, and a child's cwd does not override an inherited GIT_DIR. The suite's fixture repos are driven by spawnSync("git", a, { cwd: repo }), which inherits process.env — so npm test run from .githooks/pre-push handed those variables to the fixtures, and their git init / config / commit wrote into the real repo instead.

What the first push in this PR left behind:

  • core.bare=true, user.name=t, user.email=t@e.st, submodule.active=. in .git/config
  • the pushed branch ref moved to the fixture's remove lessons commit
  • an emptied index, after which git status answered fatal: this operation must be run in a work tree

The push succeeded and the commit was intact on origin, so nothing was lost — but the tests were green, the hook printed "tests passed", and nothing reported the damage. It is invisible until the next git command.

Two layers: the fixture helper now scrubs every GIT_* variable before spawning git, which is where the damage actually happened, and the hook unsets them too for anything else the suite spawns. Reproduced against a throwaway repo with GIT_DIR/GIT_INDEX_FILE set exactly as a hook sets them — before the fix its HEAD moved to remove lessons, user.name became t and git ls-files came back empty; after the fix HEAD, author and index are all unchanged. The second push in this PR left the clone clean.

Scope and gates

Fixes 1 and 2 are skill text only — no script behaviour changed and nothing existing was restructured. Fix 3 touches the hook and one test helper, no assertions. Version bumped 1.21.0 -> 1.22.0 in all five recorded places.

npm test: 79 tests, 77 pass, 0 fail, 2 skipped (eval-suite.json absent, as on main) — run before the edits for a baseline, after each change, and by the pre-push hook on both pushes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UCqwdBqETB9SeEVR2fSTfK

wan-huiyan and others added 3 commits August 7, 2026 13:42
Both are the failure this skill exists to catch, in its own text.

24c silently destroyed 24d's work. 24c writes the usage record with `>`,
24d merges review_findings/review_summary into that same JSON and appends
a table to the same .md, and 24c's own subagent sanity-check tells you to
"redirect its output over the fork's files". So a refresh — the normal
thing to do, since the numbers keep moving — replaced both files wholesale
with no warning. The record still had tokens, models and transcript counts;
it just had no findings, and the cross-session roll-up they exist for was
gone. 2026-08-07: a session refreshed near the end to pick up later
subagents and wiped fifteen merged findings, caught only by listing the
JSON's top-level keys afterwards.

24c now names the overwrite, fixes the order (24c then 24d, never the
reverse), carries 24d's two fields across the refresh in the fence itself,
and ends by printing the top-level keys — the print is the verification.
The Markdown table has no carry and must be re-appended by hand; that is
stated rather than glossed. Exercised end to end: with the carry the
refreshed record keeps review_findings alongside the new subagent totals;
without it only `tokens` survives.

24b's freshness audit passes without opening the file you edited. It scans
the two roots a skill is INSTALLED under; a plugin skill's SOURCE — the
marketplace checkout or clone that step 24e explicitly tells you to edit —
is a third place it never looks. Measured here: 247 skills audited,
session-handoff reported from the plugin cache at 1.20.0, zero results from
any marketplaces/ path, while the repo being edited was at 1.21.0. It
passed, on the wrong file, one version behind. Also: the trigger greps
`git diff` in the CURRENT repo, so editing a plugin skill in a separate repo
never fires it, and the cache copy's age comes from mtime — the install
time — so it reads 0d regardless of contents.

24b now states its scope, gives the direct check that closes the gap
(version bumped in every place that repo records it, verified by grep rather
than memory — five places here — and that repo's own gates run), and states
the general form: an audit that reports "all clean" without naming what it
examined has told you nothing. Prefer a check that prints its scope and
compare that scope against what you changed.

Version recorded in five places, all bumped: SKILL.md frontmatter,
plugin.json, marketplace.json, package.json, README Version History.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCqwdBqETB9SeEVR2fSTfK
Found by running this repo's own gate for the change in this PR.

Git exports GIT_DIR, GIT_INDEX_FILE and friends into every hook process,
and a child's `cwd` does not override an inherited GIT_DIR. The suite's
fixture repos are driven by `spawnSync("git", a, { cwd: repo })`, which
inherits process.env — so `npm test` run FROM .githooks/pre-push handed
those variables to the fixtures and their `git init` / `config` / `commit`
wrote into the real repo instead.

Observed 2026-08-07 pushing this branch from a worktree: core.bare=true,
user.name=t and user.email=t@e.st in .git/config, submodule.active=., the
pushed branch ref moved to the fixture's "remove lessons" commit, and an
emptied index — after which `git status` answered "fatal: this operation
must be run in a work tree". The push had succeeded and the commit was
intact on origin, so nothing was lost; but the tests were green, the hook
printed "tests passed", and nothing anywhere reported the damage. It is
invisible until the next git command.

Two layers. The fixture helper now scrubs every GIT_* variable before
spawning git, which is where the damage actually happened; the hook unsets
them too, for anything else the suite spawns.

Reproduced and verified against a throwaway repo with GIT_DIR/GIT_INDEX_FILE
set, exactly as a hook sets them. Before: HEAD moved to "remove lessons",
user.name became "t", `git ls-files` empty. After: HEAD, author and index
all unchanged. Suite still 79 tests, 0 fail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCqwdBqETB9SeEVR2fSTfK
A contributor reads Version History to know what a release changed, and the
gate fix is the part that affects anyone who has run the documented
`git config core.hooksPath .githooks`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCqwdBqETB9SeEVR2fSTfK
@wan-huiyan
wan-huiyan merged commit e6a0ff4 into main Aug 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant