fix(markdown-formatter): harden hook + simplify plumbing - #11
Merged
Conversation
…list - Collapse the triplicated data_json envelope builder into one build_data_json helper (skipped / ok-clean / ok-findings paths). - Build FINDINGS_JSON in a single jq pass instead of re-spawning jq per matched line (was O(n) processes and O(n^2) re-serialization). - Drop a dead TOOL default that the line above always assigns. - Derive the ci-status RESULTS check from the needs graph (join(needs.*.result, ' ')) instead of a hand-maintained second list that had to be kept in sync by hand. Behavior-neutral; test suites unchanged and green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPusuNL65RexZQQQ4SqL7t
Correctness fixes from an adversarially-verified review: - read_file_path: anchor the project-membership check on a path-segment boundary so a sibling directory that merely shares the prefix (e.g. repo-backup/ vs repo/) is no longer admitted as in-project. - normalize_path: case-fold the whole Windows path (not just the drive letter) so a case-only difference between file_path and CLAUDE_PROJECT_DIR doesn't skip a genuinely in-repo file. POSIX paths pass through unchanged (case-sensitive). - tests: exercise the jq-absence guard via an empty PATH — a shell-function shadow left command -v jq succeeding, so the guard was never hit; remove the dead readonly-unset/re-source. - tests: replace racy fixed sleeps with a bounded wait_for_sink poll so the fire-and-forget sink assertions stop racing process-spawn latency. - tests: use a circular hour distance for the UTC alignment check so it doesn't false-fail once per day at the 23->00 boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPusuNL65RexZQQQ4SqL7t
kyle-sexton
force-pushed
the
fix/markdown-formatter-hook-correctness
branch
from
June 27, 2026 18:59
06a63f3 to
f859176
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06a63f369b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The unconditional ${rest,,} fold collapsed /c/Repo and /c/repo to the same
C:/repo on case-sensitive POSIX hosts, so hook::read_file_path could admit a
markdown file in a sibling directory outside CLAUDE_PROJECT_DIR. Gate the
drive-letter fold on OSTYPE (msys/cygwin/win32) so it applies only where the
filesystem is actually case-insensitive; POSIX paths pass through unchanged.
Correct the function comment, which wrongly claimed the drive regex never
matches on macOS/Linux. Add OSTYPE-aware unit tests covering both host classes
and the membership-guard regression.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HR4FCVWrioJ1dvfLJk1mcV
The /home/u/Pj test literal tripped the machine-specific-paths hygiene lane (flagged as a Linux user path). Swap it for /opt/App/Sub — still a mixed-case absolute POSIX path proving the no-fold pass-through, without a user-home shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HR4FCVWrioJ1dvfLJk1mcV
This was referenced Jul 17, 2026
kyle-sexton
added a commit
that referenced
this pull request
Jul 20, 2026
…ss when git clean failed (#395) (#590) ## Summary `git-tree-reset.sh --apply` printed `AppliedClean: git clean -fdx …` **unconditionally**, regardless of whether `git clean` actually succeeded — a false success signal that misleads any operator or automation keying off that line to conclude the tree reached a known-good state. Scope note: the sibling `AppliedReset` false-success and the unresolvable-`@{u}` gate that issue #395 references were already closed by #460 (reset-failure aborts clean, exit 5) and #542 (upstream-unresolved block, exit 6). This PR fixes the one remaining unguarded case — `AppliedClean` — so #395's acceptance criteria are fully met. ## Fix - Capture `git clean -fdx`'s exit status (`CLEAN_RC`), read on the line immediately after the capture so `$?` isn't clobbered (the script runs `set -uo pipefail`, no `-e`). - Gate the `AppliedClean` success line on the real outcome. A non-zero clean exit whose cause is **locked/in-use files** is the expected non-fatal case — `git clean` returns non-zero when it fails to remove any path, and that case is already reported honestly via `Unremovable:` — so it is **not** treated as failure (avoids regressing the deliberate locked-file handling on Windows). Only a non-zero exit with **no** `failed to remove` warnings is a genuine failure: it emits an explicit `FAILED:` line, `AppliedClean: failed`, and exits **7** instead of a success line. - Emit the `AppliedReset:` success line as soon as `reset --hard` genuinely succeeds — **before** `clean` — so a subsequent clean failure still surfaces the truthful reset outcome rather than swallowing it. - Run the reparse-point restore guard on the failure path too (data-loss guard: a clean that errored mid-run may still have deleted tracked files first; safe because `reset --hard` ran first). - Documented exit 7 in the script header, `usage()`, and the `clean` skill's `context/git-tree-reset.md` gates list. ## Verification New test case #11 (`git-tree-reset.test.sh`) forces a genuine clean failure via a `PATH` shim that intercepts only `git clean` (delegating every other subcommand — crucially `reset` — to the real git, so the reset genuinely succeeds). The full suite passes, including the 6 new assertions: ``` PASS: [38] clean failure exits 7 PASS: [39] clean failure reports failure PASS: [40] clean failure emits AppliedClean: failed PASS: [41] clean failure emits no clean success line PASS: [42] clean failure still reports the successful reset PASS: [43] clean failure leaves untracked intact (clean did not silently succeed) OK: git-tree-reset.sh tests passed ``` (All 43 tests pass; the pre-existing reset-failure suite #22–27 and upstream-unresolved suite #28–37 continue to pass — no regression. `shellcheck` is clean on both files.) Live `--apply` run against a repo where `git clean` fails after a successful reset — the report is now honest and exits non-zero: ``` AppliedReset: git reset --hard main FAILED: git clean -fdx exited 1 (non-locked-file cause) — untracked removal incomplete; reset --hard already applied. fatal: simulated git clean failure AppliedClean: failed RestoredTracked: 0 EXIT=7 scratch.txt still present (clean did NOT silently succeed): YES ``` Before this fix the same run printed `AppliedClean: git clean -fdx` and exited `0`. Closes #395 ## Related - #395 — this PR. - #396 — sibling issue on the same script (dry-run does not validate `@{u}` upstream). **Not addressed here** to avoid a merge collision on the same lines; a separate cycle picks it up. 🤖 Generated with a Claude Code implementation subagent (issue #395) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 20, 2026
fix(disk-hygiene): reason about volume-root rejection instead of blanket-denying non-OS drives
#1026
Closed
kyle-sexton
added a commit
that referenced
this pull request
Jul 30, 2026
…on recovery (#1780) ## The defect `/session-flow:handoff` wrote the save-point to an absolute location but emitted a **rootless** path in the copy-paste resume prompt. This was contract-specified, not a model slip: `reference/save-point.md` defined the directive as `Read @<handoffs-dir>/<TS>-handoff-<topic>.md` where `<handoffs-dir>` is "the path the write step actually used" — and `<memory_dir>` is repo-relative by contract, so the one artifact an operator carries across `/clear` lost the root the file hangs off. Pasted into a session whose cwd is not the worked-in repository root, the `@`-reference resolves somewhere else. When that somewhere else has its own `.work/handoffs/` — true of any home directory that has run `/handoff` before — the failure presents as *"the file is missing"* rather than *"the path has no root"*, which #1644 correctly calls the most expensive shape to diagnose. It is not only a cross-repo problem: a resuming session sitting in a **subdirectory** of the right repo fails the same way. `/session-flow:find-handoff`, the skill that exists to recover exactly this, carried the same single-root assumption — its transcript rung found the correct directive, resolved it against the source transcript's `cwd`, and then **discarded** the candidate on the existence check. ## The change **Producer (`reference/save-point.md`).** - The directive now carries the **absolute**, forward-slash-normalized path. Forward slashes are specified rather than left to the model: the directive survives into transcript JSONL, where a backslash is escaped again, and `find-handoff` greps that record. - A `Handoff origin:` line inside the rails names the repository identity and the repo-relative path, so a resume on a different machine or checkout can re-resolve. It is **computed at emit time** from the repository actually written into — deliberately *not* a stored frontmatter field, which is what kept this change out of schema territory. It sits inside the rails because the copy region is what travels; below the rail it would be lost on paste. - The `@` mention is documented as an **accelerator, not the mechanism**. Official docs state an `@` reference's path "can be relative or absolute" ([common-workflows](https://code.claude.com/docs/en/common-workflows#reference-files-and-directories)), but document no drive-letter or whitespace-bearing form — so expansion is treated as unverified there, and the directive is written to stay actionable without it (the same line states the full absolute path, which a resuming session reads directly). That is what makes rooting a strict improvement rather than a trade. Absolute is not new to this engine: `reference/topic-docs.md` already lands no-project-root handoffs under `${CLAUDE_PLUGIN_DATA}/topic-docs/handoffs/` "with the absolute path announced prominently". Same condition, reached a different way. **Consumer (`skills/find-handoff/SKILL.md`).** - The detection contract accepts **both** forms. Every handoff written before this shipped states a repo-relative path and is still on disk, so a detector that recognizes only rooted directives would stop recovering the entire existing corpus. Matching happens on the `…handoffs/<TS>-handoff-…` shape both forms share; they diverge only at the existence check. - **A path that resolves to nothing is UNRESOLVED, never discarded — on BOTH forms.** A rootless miss because resolving against the producer's `cwd` is an inference; a rooted miss because an absolute path is machine-local and a resume on another machine or checkout cannot satisfy it. The rooted miss is exactly what `Handoff origin:` exists for, so the existence check reads that line and re-resolves against the repository it names before giving up. Either way: one bounded, read-only widening over repository roots already in hand, then surfaced at the confirm gate with the directive verbatim — and the gate **names which failure it was**, because "the path has no root" and "nothing is at that absolute path on this machine" send an operator to different places, while "missing" sends them nowhere. Discarding on miss is the specific behavior that made the recovery ladder unable to recover the failure it was written for. - **`Handoff origin:` is a resolution input, not a detection signal.** It cannot admit or reject a candidate, so it is not a fourth key — it is read only after a candidate qualifies, at the existence check. The signal summary says so rather than listing three signals while the ladder depends on a fourth thing. - The Gotcha keeping rootless resolution alive is preserved, not replaced. An independent fresh-context review of this diff caught the rooted-miss hole: the first revision consulted `Handoff origin:` only on the rootless branch — where the contract says it can never appear, since the line shipped with the rooted form — leaving the one case it exists for as the one case nothing handled, which fell through to discard-on-miss and reintroduced the defect one path over. That, and the signal-label collision with the `/loop` re-arm note's existing "fourth signal", are fixed in the second commit. ## What this PR deliberately does NOT do It does **not** fully satisfy #1644's line 176 ("rung 1's missing repo-correlation check should be closed in the same change"). Closing it needs durable repository identity **stored in the handoff file** — a new frontmatter field — which is a cross-cutting schema change every handoff already on disk would lack, and which every consumer must then tolerate the absence of. That is a decision on its own merits, now filed as #1778 with the options laid out. A weaker substitute (read the repository off the producer transcript) was considered and rejected on evidence: it depends on a transcript that may be absent — while `find-handoff`'s own Gotchas say transcripts are the reliable index *precisely because the filesystem is not* — and it returns nothing for every rootless legacy handoff, i.e. exactly where the check is needed. Shipping it would have produced a check that looks closed and is not. Instead, rung 1 now **states the gap in place**, so the next reader does not mistake it for closed. ## Verification All run locally in the PR worktree against `origin/main`: - `scripts/check-changelog-parity.sh --check` — every versioned plugin has a CHANGELOG.md - `scripts/check-changelog-parity.sh --check-bump origin/main` — 0.17.19 → 0.17.20 has its `## [0.17.20]` entry, newly added - `scripts/check-changelog-parity.sh --check-order` — all 71 changelogs newest-first, no duplicates - `scripts/check-changed-skills.sh origin/main` — 2 skills checked, 0 failed (find-handoff carries the pre-existing >200-line soft warning; cap is 500, it is at 394) - `scripts/check-skill-portability.sh origin/main` — no unexcused coupling tokens - `scripts/validate-plugins.sh` — all plugin manifests + catalog valid - `scripts/check-orphaned-fixtures.sh --check`, `check-contract-slice-prune.sh --check-diff origin/main`, `check-cross-plugin-source-drift.sh --check` — all pass - `markdownlint-cli2 "plugins/session-flow/**/*.md"` — 36 files, 0 errors - session-flow's own contract tests (`parse-transcript.test.sh`, `observer.test.sh`) — PASS - Both touched `evals.json` files validate against `plugins/skill-quality/reference/evals.schema.json`. Eval coverage added for the producer emitting a rooted path when cwd is not the worked-in repo (handoff #4), both-forms acceptance (find-handoff #9), the rootless UNRESOLVED path (#10, with its prompt pinned to the pre-rooted directive so the branch is unambiguous), and the rooted miss re-resolved via `Handoff origin:` on another machine (#11). find-handoff eval #1's cwd-resolution expectation was made form-aware so it no longer asserts the old single-form behavior. The harness claim this change rests on was verified against current official docs this session rather than recalled, per the repo's fresh-docs mandate; the drive-letter and whitespace edge is explicitly flagged as *not* covered by those docs and is handled by not relying on expansion. ## Related - Fixes #1644 - #1778 — the split-out repo-identity / rung-1 correlation decision this PR states in place - #1677 — the topic-docs resolution-order gap (no rung for a session working in a repository that is not cwd's project root); adjacent cause, separately tracked, not required by this fix - #1086 — same class on a different surface: a skill resolving a repository from cwd when cwd is not the repository in play 🤖 Generated with [Claude Code](https://claude.com/claude-code) <https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improvements to the markdown-formatter hook from a
/simplifypass and a whole-repo/code-review(each finding adversarially verified before inclusion). Two commits, by concern.fix:correctness (verified bugs)hook-utils.shread_file_path!= "$proj"*is a prefix glob with no boundary — a sibling likerepo-backup/is admitted as in-project, so the hook would format.mdfiles outside the consumer reporoot/*, trailing slash stripped)hook-utils.shnormalize_pathfile_path/CLAUDE_PROJECT_DIRdiffer only in body casing → in-repo file silently skippedhook-utils.test.shjqwith a function —command -v jqstill succeeded, so the absence guard was never exercised (false coverage); dead readonly-unset/re-sourcePATH; remove the dead linessleep→ intermittent failures on Windowswait_for_sinkpoll (slow-sink timing test untouched)hook-utils.test.shrefactor:simplify (behavior-neutral)data_jsonbuilder into one helper.FINDINGS_JSONin a singlejqpass (was O(n) processes / O(n²) re-serialization).TOOLdefault.ci-statusRESULTSfrom theneedsgraph (join(needs.*.result, ' ')) instead of a hand-synced second list.Validation
hook-utils.test.sh27/0;markdown-format.test.sh40 pass (the 1 "fail" is a pre-existing slow-sink timing assertion that fails on Windows Git Bash before any of these changes). Shellcheck clean. Membership/normalize fixes verified against live temp-dir scenarios.🤖 Generated with Claude Code
https://claude.ai/code/session_01KPusuNL65RexZQQQ4SqL7t