feat(skills): sync-advisor — measure the drift before naming the fix (v1.38.0)

[lapc506]

What

Renames the rebase-advisor skill to sync-advisor and extends it from a blind router into an advisor that measures first.

Why

skills/rebase-advisor/SKILL.md was 43 lines and measured nothing. Its step 1 was "Confirm the user wants a full team sync" — a question back to the user about something three git commands answer.

Its description then over-routed: it triggered on "align with develop" and "branches are behind" — both git pull cases — and sent them all to /make-no-mistakes:rebase, which stashes every worktree, rebases every local branch and auto-merges PRs. Between git pull and that, the toolkit offered nothing, and nothing read-only at all.

The shape — measures, routes, never acts

Six read-only predicates run before anything is named:

#	Predicate	Command	Routes to
1	Distance	git rev-list --left-right --count HEAD...origin/$BASE	whether anything is needed
2	Fast-forward?	git merge-base --is-ancestor HEAD origin/$BASE	pull vs. rebase/merge
3	Dirty tree	git status --porcelain, split staged vs unstaged	stash or commit first
4	Untracked the ref already tracks	comm -12 of ls-files --others and ls-tree -r	back up first — aborts the pull
5	Worktrees behind	git worktree list --porcelain + per-branch distance	/rebase
6	Unpushed commits	git for-each-ref %(upstream:track)	what would be lost

Predicate 5 is the threshold: one branch behind is a git pull, several worktrees behind is what /rebase was built for.

Every fix is printed for the user to run — git pull, git stash, git rebase, /make-no-mistakes:rebase. The single write is git fetch origin --quiet (remote-tracking refs only), and the skill names it out loud: without it every measurement is taken against a stale origin/<base>.

Predicate 4 was verified, not asserted

Built a throwaway upstream/clone pair and ran both controls:

• Negative control (no collision): the comm pipeline printed nothing.
• Positive (untracked local file at a path the ref tracks): printed newfile.txt.

Then confirmed what makes it worth a dedicated predicate — it is invisible to every other signal:

$ git status --porcelain          →  ?? newfile.txt          (looks like scratch)
$ git rev-list --left-right --count HEAD...origin/develop
                                  →  0    1                  (a clean fast-forward)
$ git merge-base --is-ancestor HEAD origin/develop; echo $?
                                  →  0                       (FF possible)
$ git pull --ff-only              →  exit 1
error: The following untracked working tree files would be overwritten by merge:
	newfile.txt
Please move or remove them before you merge.
Aborting

This is the part of the PR worth arguing about, so it is stated plainly rather than left as a UX detail. git pull names the user's own files and then offers deleting them as the remedy — and deletion is the only irreversible move in the whole sequence. Everything else here (a pull, a stash, a rebase) is recoverable; removing an untracked file that was never committed is not. The abort message is what pushes toward it, and it reads as the user's own mistake, which is exactly when someone reaches for the destructive option without thinking.

So the skill does the opposite of what the message suggests: it reports the colliding files by name and recommends copying them somewhere outside the repo. It never recommends deleting them. That inversion is what makes this an advisor rather than a wrapper around git pull — a wrapper would surface git's own remedy verbatim.

Corroboration beyond the synthetic repro above, reported by the reviewer of this PR and not re-measured by me: this exact class of collision blocked a pull twice today in a client repo, and in a separate session an agent lost uncommitted work to an operation of the same shape.

The consequence line

New syncAdvisor.governedPaths key in make-no-mistakes.config.json (the existing behaviour config, already read for language):

{ "syncAdvisor": { "governedPaths": [".claude/hooks/", "scripts/", ".github/workflows/"] } }

Turns "you are 12 commits behind" into "three hooks changed, two of them fix defects you may be looking at right now". Paths are not hardcoded — this toolkit is shared and carries no client-repo mentions (checked: no dojo / DOJ-NNNN string in any added line).

Degraded mode is documented in both the SKILL.md and the config example: with the key unset, the skill reports distance and routing and omits the consequence line. No built-in fallback list — an invented one is wrong in every repo but the one it was copied from, and would read as measured.

Origin

2026-07-31 (as reported, not re-measured): a developer filed two bug reports against a hook, both with clean reproductions. One was a real defect; the other described behaviour fixed days earlier against a stale checkout. Nothing in the report separated them. A commit count alone would not have separated them either — naming the changed hooks would have.

References to the old name

Searched case-insensitively across the whole repo (grep -rniI "rebase.advisor", excluding node_modules and .git). Two hits, both updated:

• skills/rebase-advisor/SKILL.md — the skill itself (git mv to skills/sync-advisor/, frontmatter name: updated)
• README.md:131 — the Skills table row

The only surviving occurrence is the CHANGELOG line that documents the rename itself.

Nothing is left broken. Checked the install machinery: src/lib/copy-assets.ts:24 enumerates the skills directory generically (no hardcoded skill names), and copy-assets.ts:65-72 prunes stale managed files — any path in the previous install manifest that is not in the new expected set gets removed, so skills/rebase-advisor/SKILL.md is deleted on make-no-mistakes update. dist/ is untracked, so no stale build carries the old name.

Not touched, on purpose

• commands/rebase.md — untouched. It stays a real destination; what changed is who decides when it applies.

Found while working, reported not fixed

Two pre-existing drifts adjacent to lines I edited. Neither is caused by this PR and neither is in its scope:

1. README says ### Skills (10) but there are 11 skill directories — resolve-open-questions has no row in that table, while .claude-plugin/marketplace.json correctly says "11 auto-activating skills". The count is unchanged by this rename (a rename is 11 → 11), so I left both the heading and the missing row alone rather than widen this PR.
2. v1.36.0 (PR feat(hooks): refuse a command that discards stderr (v1.36.0) #53) did not add its CHANGELOG reference link — [Unreleased] still compared from v1.35.0 and there was no [1.36.0]: line. I did fix this one, because my entry would otherwise sit above a hole I had just widened: added [1.36.0] and [1.38.0] and repointed [Unreleased]. The [1.37.0]: slot is left empty on purpose — that version belongs to feat(hooks): a refusal no longer prints its own bypass marker (v1.37.0) #55.

One cosmetic residue I am not fixing here: the pruner removes files with rm(abs) and not the parent directory, so an existing OpenCode install keeps an empty skills/rebase-advisor/ directory after updating. An empty directory with no SKILL.md does not register as a skill, so this is cosmetic — but fixing it means touching the installer, which does not belong in a rename PR.

Version: 1.38.0 (skipping 1.37.0 on purpose)

Measured on origin/main @ ee0ba47 — all three version files read 1.36.0. Probing every open PR for a bump:

$ gh pr diff 55 | grep -E '^\+.*"version"'
+      "version": "1.37.0",
+  "version": "1.37.0",

$ for pr in 52 46 44 41; do gh pr diff $pr | grep -E '^\+.*"version"'; done
(nothing)

#55 already takes 1.37.0 in all three files, so taking it here collides in all three. The other four open PRs bump nothing. Skipping a version costs nothing; colliding does.

The [1.37.0]: CHANGELOG reference slot is deliberately left empty for #55 to fill.

Minor and not major — decided by reading, not by habit

A rename is normally breaking, but a skill auto-activates on its description; it is not invoked by name the way a command is, so a renamed name: changes no call site.

$ grep -rniI "rebase.advisor" . --exclude-dir=node_modules --exclude-dir=.git --exclude=CHANGELOG.md
(no hits)

That negative is real rather than a broken search — the positive control: the same grep for a name that is cross-referenced returns 10 files, so references of this shape are found when they exist.

$ grep -rniIo "spike-recommend" skills/ commands/ --include="*.md" | cut -d: -f1 | sort -u
commands/implement.md
commands/premortem.md
commands/prioritize.md
skills/implement-advisor/SKILL.md
skills/prioritize/references/frameworks/moscow-rice.md
skills/prioritize/references/linear-mutations.md
skills/prioritize/references/scoring-rules.md
skills/prioritize/SKILL.md
skills/spec-recommend/SKILL.md
skills/spike-recommend/SKILL.md

One surface does break, stated here rather than folded into the above: a user who typed /make-no-mistakes:rebase-advisor explicitly — README.md:137 documents that skills can be invoked that way — now gets an unknown skill. It fails loudly, the replacement is one row away in the same table, and the installer prunes the old file rather than leaving both live. Anyone who reads that as breaking should say so on the PR; the call is recorded in the CHANGELOG so it can be disagreed with.

Also relevant to house precedent: this repo has shipped 38 minor releases and no major (grep -cE "^## \[1\." CHANGELOG.md → 38; no 0.x, 2.x or 3.x heading exists).

Tests

Both suites run on the final state, output not discarded:

npm test          →  Test Files 12 passed (12) · Tests 60 passed (60)
npm run test-hooks →  Results: 337 / 337 passed

Same baseline PR #53 reported. Nothing was skipped. Not merged — this PR is left for review.

Created by Claude Code on behalf of @lapc506

[dojo-code-reviewer]

✅ Approved

Approved — no findings. Confidence: 5.00/5.00. NITs: 1 (shown).

Walkthrough

⚠️ Governance Warning: This PR targets main directly. GitFlow expectations are feature → develop → main. This is informative only and does not impact the code review verdict.

Review Walkthrough

This PR renames the rebase-advisor skill to sync-advisor and upgrades it from a basic router to a comprehensive git drift advisor that measures six read-only predicates before suggesting fixes (such as a plain fast-forward pull, a single-branch rebase, or the full team-wide /rebase). It also introduces a syncAdvisor.governedPaths behavior configuration to highlight critical modified files (e.g., hooks, scripts) by name, and updates the toolkit's package/marketplace/plugin manifests to v1.37.0.

Reviewed Areas/Files

• skills/sync-advisor/SKILL.md: Analyzed the newly added multi-predicate git drift analysis and command routing logic.
• commands/make-no-mistakes.config.example.json & README.md: Checked the configuration schema structure and documentation.
• package.json, .claude-plugin/plugin.json, .claude-plugin/marketplace.json: Verified version matching consistency.
• CHANGELOG.md: Confirmed accurate logs and fixed missing version links.

Safety Rationale

The advisor operates strictly in a read-only manner (with the safe exception of a quiet remote-tracking fetch to avoid stale measurements) and solely outputs action recommendations for the user to run manually, guaranteeing that merging these changes introduces no automated write-side impact or workspace risks.

Approved — 0 blockers.

⚪ P4 — Nitpicks

• skills/sync-advisor/SKILL.md:14 — [NIT] ⚪ P4 (nit) — Unnecessary backslash escapes in YAML folded scalar block. Inside a folded scalar block (>), double quotes do not require escaping unless the entire block is wrapped in outer quotes. Dropping the backslashes from \"squash these and commits\") makes the text cleaner.

[pass 1]

---

Total findings: 1 nit (1 total)

[dojo-code-reviewer]

[NIT] ⚪ P4 (nit) — Unnecessary backslash escapes in YAML folded scalar block. Inside a folded scalar block (>), double quotes do not require escaping unless the entire block is wrapped in outer quotes. Dropping the backslashes from \"squash these and commits\") makes the text cleaner.

[pass 1]

[dojo-code-reviewer]

✅ Approved

Approved — no findings. Confidence: 5.00/5.00.

Walkthrough

⚠️ Governance Warning: This pull request targets the main branch directly. Per repository GitFlow guidelines, the expected flow is andres/sync-advisor → develop → main. This is a non-blocking informative warning; the technical review below focuses entirely on the quality and correctness of the code.

Walkthrough

This PR renames and completely overhauls the rebase-advisor skill into sync-advisor. Rather than blindly routing users or prompting them with generic questions, it now executes six read-only git predicates to programmatically measure branch and worktree drift. It introduces syncAdvisor.governedPaths in the tool configuration to map specific commit differences into meaningful user consequences (such as naming specific changed hooks or workflows), and suggests (but never automatically runs) the most precise Git commands (git pull --ff-only, single-branch rebase, or the full /rebase protocol).

Files Reviewed

• Skill Definition: skills/sync-advisor/SKILL.md (fully redesigned) and removal of skills/rebase-advisor/SKILL.md.
• Configuration Example: commands/make-no-mistakes.config.example.json adding syncAdvisor.governedPaths.
• Project Metadata & Versioning: package.json, .claude-plugin/plugin.json, .claude-plugin/marketplace.json, README.md, and CHANGELOG.md (bumped to 1.38.0 and references updated).

Safety Rationale

The upgraded skill operates purely in a read-only manner—measuring drift and printing explicit recommendations for the user to review and run manually—ensuring zero risk of unexpected repository manipulation or data loss.

Approved — no findings.

[lapc506]

Reviewed twice, both APPROVED — e6386c0 and ebbf10e. I checked the second is not a stale re-score by dating it on content rather than on commit_id: it says "bumped to 1.38.0" while the first says "v1.37.0", which matches the two commits. So the approval does reflect the current head.

Two findings in those reviews are false. Recording the measurements rather than editing the file to silence them.

1. P4 nit — "Unnecessary backslash escapes in YAML folded scalar block" at skills/sync-advisor/SKILL.md:14. There are no backslashes on that line, or anywhere in the frontmatter.

$ sed -n '14p' skills/sync-advisor/SKILL.md | cat -A
  Does NOT trigger on: interactive rebase ("git rebase -i", "squash these$

$ sed -n '1,17p' skills/sync-advisor/SKILL.md | grep -n '\\'
(no output — exit 1)

And the frontmatter parses as YAML with no backslash in the decoded value:

$ node -e "…yaml.load(frontmatter)…"
name = sync-advisor
description parses, length = 916
contains literal backslash? false

The \" the finding quotes is an artifact of a JSON-encoded view of the file, not file content — that is inferred from the shape of the quoted string, not verified against the reviewer's input. No change made; there is nothing to remove.

2. Governance Warning — "targets main directly, expected feature → develop → main". This repo has no develop branch, so there is no such flow to follow here.

$ git ls-remote --heads origin | grep -E 'refs/heads/(develop|main|master)$'
refs/heads/main

$ gh repo view --json defaultBranchRef --jq .defaultBranchRef.name
main

$ gh pr list --state merged --limit 8 --json number,baseRefName
#53 → main   #51 → main   #50 → main   #49 → main
#48 → main   #45 → main   #43 → main   #42 → main

main is the default branch and the base of all 8 most recent merged PRs. The develop convention belongs to a different repo. The warning is already marked non-blocking; noting it so the next reader does not re-derive it.

Standing verdict: 0 blockers, 0 P1, 0 P2, 1 P4 refuted above. Suite on ebbf10e: 337/337 hooks, 60/60 vitest. MERGEABLE / CLEAN.

Not merging yet: #55 also bumps the same three version files (it takes 1.37.0, this takes 1.38.0), so merge order is a decision for the humans on the thread rather than mine.

Created by Claude Code on behalf of @lapc506

[dojo-code-reviewer]

✅ Approved

Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00.

Walkthrough

⚠️ Warning (Governance): This PR targets main directly. GitFlow expects development features to target develop before merging to main. This is a non-blocking informative warning; the technical review verdict below is based on code quality and correctness.

Walkthrough

This PR renames the rebase-advisor skill to sync-advisor and upgrades it from a simple routing prompt to a robust advisor that dynamically measures git repository drift via six read-only predicates before proposing recommendations. It also introduces the syncAdvisor.governedPaths configuration schema to map local file modifications to actionable warnings.

Reviewed Files/Areas

I reviewed the skill specification (skills/sync-advisor/SKILL.md), the documentation updates in README.md, configuration updates in commands/make-no-mistakes.config.example.json, package manifest version bumps (package.json, .claude-plugin/plugin.json, .claude-plugin/marketplace.json), and the CHANGELOG.md records.

Safety Rationale

The implementation is completely read-only (except for a safe, quiet git fetch) and operates purely as an advisory CLI skill with no write operations to the user's working tree or index, making it completely safe to merge.

Verdict

Approved — 0 blockers, 1 P3.

🔵 P3 — Minor

• skills/sync-advisor/SKILL.md:141 — 🔵 P3 (minor) — When git ls-files is run from a subdirectory, it outputs paths relative to that subdirectory, whereas git ls-tree always outputs repository-relative paths. If the skill is executed from a subdirectory, the two file lists will fail to match in comm -12 even if there are identical paths, leading to missed untracked file collisions. Adding the --full-name option to git ls-files forces it to always output repository-relative paths, matching git ls-tree and making the check robust from any working directory.

[pass 1]

---

Total findings: 1 business context (1 total)

[dojo-code-reviewer]

🔵 P3 (minor) — When git ls-files is run from a subdirectory, it outputs paths relative to that subdirectory, whereas git ls-tree always outputs repository-relative paths. If the skill is executed from a subdirectory, the two file lists will fail to match in comm -12 even if there are identical paths, leading to missed untracked file collisions. Adding the --full-name option to git ls-files forces it to always output repository-relative paths, matching git ls-tree and making the check robust from any working directory.

[pass 1]

[lapc506]

On the P3 at SKILL.md:141 (review of ea8c46e): the premise is false and the proposed fix would introduce the bug it describes — but it pointed at a real weakness of a different kind, which is now fixed. Commit 00e2c08.

The premise. The finding says git ls-tree "always outputs repository-relative paths" while ls-files is prefix-relative. It does not — from a subdirectory it strips the prefix exactly like ls-files:

$ cd sub && git ls-files --others --exclude-standard
newfile.txt
$ cd sub && git ls-tree -r --name-only origin/develop
newfile.txt
shared.txt
$ comm -12 <(…ls-files…| sort) <(…ls-tree…| sort)
newfile.txt          ← matched; the collision WAS found

The proposed fix breaks it. --full-name on ls-files alone, from sub/:

$ git ls-files --others --exclude-standard --full-name   →  sub/newfile.txt
$ git ls-tree -r --name-only origin/develop              →  newfile.txt
$ comm -12 …                                             →  (empty)

Empty is the dangerous output here: it reads as "no collisions" for a tree that is about to abort the pull. That is the exact failure the finding set out to prevent.

The real weakness it gestured at, and the fix. From a subdirectory both commands are scoped to that subtree, so a collision at the repo root is never examined. That is scope, not format, and no format flag fixes it — --full-name changes how a path prints, never which paths are considered. The pathspec does. Shipped:

comm -12 \
  <(git ls-files --others --exclude-standard --full-name -- :/ | sort) \
  <(git ls-tree -r --full-tree --name-only "origin/$BASE" | sort)

Four cases, with the command extracted verbatim from SKILL.md so the test cannot drift from the doc — two collisions planted, one at the repo root and one in sub/:

run from	no collisions present	both collisions present
repo root	(empty) ✓	rootfile.txt, sub/newfile.txt ✓
sub/	(empty) ✓	rootfile.txt, sub/newfile.txt ✓

Net: finding rejected as written, underlying issue real and fixed, and the fix is a strictly wider check than either the original or the suggestion. Suite on 00e2c08: 340/340 hooks, 60/60 vitest.

Created by Claude Code on behalf of @lapc506

[dojo-code-reviewer]

💬 Review Comments

Comments — 0 blockers, 1 P2. Confidence: 3.80/5.00. NITs: 1 (shown).

Walkthrough

⚠️ GOVERNANCE WARNING: This PR targets main directly instead of develop, which departs from the expected GitFlow branch sequencing (feature → develop → main). This warning is non-blocking for this review but should be corrected/noted by the maintainers.

Summary Walkthrough

This PR renames the rebase-advisor skill to sync-advisor and upgrades it from a simple router to a robust "measure-first" advisor. It introduces six read-only git predicates to assess repository drift and adds a new syncAdvisor.governedPaths configuration setting to provide specific file change context rather than a generic commit count behind.

Files/Areas Reviewed

I reviewed all modified configuration and metadata files (package.json, .claude-plugin/plugin.json, .claude-plugin/marketplace.json), the documentation files (README.md, CHANGELOG.md, commands/make-no-mistakes.config.example.json), and the core skill implementation playbook (skills/sync-advisor/SKILL.md).

Safety Rationale

Since this pull request strictly modifies markdown playbooks, metadata manifests, and configuration examples without adding any executable source code or automated commands, it poses zero operational risk to the runtime system and is entirely safe to merge.

Review completed — 0 blockers, 1 P2, 1 P4.

🟡 P2 — Major

• skills/sync-advisor/SKILL.md:53 — 🟡 P2 (major) — If Command 1 or Command 2 is used to resolve the base ref, the resulting value stored in $BASE will include the remote prefix (e.g., origin/develop or origin/main). When subsequently expanded as origin/$BASE in subsequent predicates and steps, it will resolve to origin/origin/develop or origin/origin/main. These are invalid Git references, which will cause commands like git rev-list and git diff to fail.

We should explicitly document stripping the remote prefix (for example, using shell parameter expansion like BASE=${BASE#*/}) in Step 0 so that $BASE is guaranteed to be just the local branch name (e.g., develop or main).

[pass 1]

⚪ P4 — Nitpicks

• skills/sync-advisor/SKILL.md:53 — [NIT] ⚪ P4 (nit) — Passing both --abbrev-ref and --symbolic-full-name to git rev-parse is redundant. In standard Git usage, --abbrev-ref on its own is sufficient to obtain the abbreviated upstream branch name.

[pass 1]

---

Total findings: 1 compliance, 1 nit (2 total)

[dojo-code-reviewer]

🟡 P2 (major) — If Command 1 or Command 2 is used to resolve the base ref, the resulting value stored in $BASE will include the remote prefix (e.g., origin/develop or origin/main). When subsequently expanded as origin/$BASE in subsequent predicates and steps, it will resolve to origin/origin/develop or origin/origin/main. These are invalid Git references, which will cause commands like git rev-list and git diff to fail.

We should explicitly document stripping the remote prefix (for example, using shell parameter expansion like BASE=${BASE#*/}) in Step 0 so that $BASE is guaranteed to be just the local branch name (e.g., develop or main).

[pass 1]

[dojo-code-reviewer]

[NIT] ⚪ P4 (nit) — Passing both --abbrev-ref and --symbolic-full-name to git rev-parse is redundant. In standard Git usage, --abbrev-ref on its own is sufficient to obtain the abbreviated upstream branch name.

[pass 1]

[lapc506]

P2 accepted — it was right, and it understated the problem. Fixed in ac770c7.

The prefix defect, reproduced. Step 0's commands returned values carrying the remote, and every predicate interpolates origin/$BASE:

$ git symbolic-ref --short refs/remotes/origin/HEAD
origin/main                                    ← carries the prefix

$ git rev-list --left-right --count "HEAD...origin/origin/main"
fatal: ambiguous argument 'HEAD...origin/origin/main': unknown revision
exit=128

Now normalised unconditionally, whichever branch of the resolution produced the value:

BASE=${BASE#refs/remotes/}
BASE=${BASE#origin/}

Verified on both paths, each against a control that must fail — where origin/HEAD is set it yields origin/main → normalises to main → git rev-parse --verify origin/main resolves; the un-normalised form exits 128.

Where the finding understated it. The suggested remedy was to strip the prefix. Stripping alone would have left Step 0 pointing at the wrong ref, not a misspelt one. Its first command resolved the base from @{upstream}, which on any feature branch is that branch's own remote copy:

$ git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}'
origin/andres/sync-advisor          ← not origin/develop

Stripped, that becomes andres/sync-advisor — a perfectly valid ref, and the wrong question. Comparing against it answers "am I pushed?" (predicate 6's job) and would report 0 behind on a branch far behind the real base — a wrong answer that looks like a right one, which is worse than the exit-128 crash. So the command is gone from base resolution rather than patched.

One more thing measured while in there, unprompted: git symbolic-ref --short refs/remotes/origin/HEAD fails outright in this repo —

fatal: ref refs/remotes/origin/HEAD is not a symbolic ref

— so it is now documented as a fall-through into the develop / main / master / trunk probe, not as a step expected to succeed.

P4 also correct, and now moot. Both forms return origin/andres/sync-advisor here, so --symbolic-full-name added nothing — and that command no longer appears in the file.

Suite on ac770c7: 340/340 hooks, 60/60 vitest.

Created by Claude Code on behalf of @lapc506

[dojo-code-reviewer]

✅ Approved

Approved — 0 blockers, 1 P3. Confidence: 4.80/5.00. NITs: 1 (shown).

Walkthrough

GOVERNANCE WARNING: This PR targets the main branch directly, bypassing the standard GitFlow path of feature -> develop -> main. If your release procedures require merging to develop first, consider retargeting this PR.

Walkthrough

This PR renames the rebase-advisor skill to sync-advisor, extending it from a blind router into an active, read-only advisor that measures repository drift before suggesting remedial commands. It executes six read-only git predicates to safely determine the exact synchronization state of the user's checkout. Additionally, it integrates a new syncAdvisor.governedPaths configuration key to provide targeted consequence reporting for critical directories like hooks, scripts, and workflows.

Areas Reviewed

• skills/sync-advisor/SKILL.md (analyzed all git command formulations, base resolution steps, and routing logic).
• package.json, .claude-plugin/plugin.json, .claude-plugin/marketplace.json (version bumps to 1.38.0).
• README.md, CHANGELOG.md (documentation, version tables, and changelog comparison updates).
• commands/make-no-mistakes.config.example.json (configuration keys and default explanations).
• src/lib/copy-assets.ts (verified installer asset-scanning and pruning logic remains robust under renames).

Safety Rationale

The advisor logic is entirely read-only (with the safe exception of running git fetch origin --quiet to update remote-tracking refs) and only makes educational/routing recommendations, ensuring no local branch rewrites, stashes, or deletions are executed automatically.

Approved — 0 blockers, 1 P3.

🔵 P3 — Minor

• skills/sync-advisor/SKILL.md:67 — 🔵 P3 (minor) — Suppress noisy stderr when the reference default branch cannot be symbolic. In a fresh checkout or worktree, origin/HEAD is often unset, causing git symbolic-ref to print a fatal: ref refs/remotes/origin/HEAD is not a symbolic ref error directly to stderr. Appending 2>/dev/null keeps the terminal output clean during the fall-through sequence.

[pass 1]

⚪ P4 — Nitpicks

• package.json:3 — [NIT] ⚪ P4 (nit) — The package version in package-lock.json is currently stuck at 1.23.0 and is out of sync with package.json (1.38.0). Run npm install --package-lock-only to keep them aligned.

[pass 1]

---

Total findings: 1 business context, 1 nit (2 total)

[dojo-code-reviewer]

[NIT] ⚪ P4 (nit) — The package version in package-lock.json is currently stuck at 1.23.0 and is out of sync with package.json (1.38.0). Run npm install --package-lock-only to keep them aligned.

[pass 1]

[dojo-code-reviewer]

🔵 P3 (minor) — Suppress noisy stderr when the reference default branch cannot be symbolic. In a fresh checkout or worktree, origin/HEAD is often unset, causing git symbolic-ref to print a fatal: ref refs/remotes/origin/HEAD is not a symbolic ref error directly to stderr. Appending 2>/dev/null keeps the terminal output clean during the fall-through sequence.

[pass 1]

[lapc506]

P3 on SKILL.md:67 rejected — the suggested change is the one thing this toolkit ships a blocking rule against. No change made.

The finding asks to append a stderr-discarding redirect to the git symbolic-ref fall-through "to keep the terminal output clean". That exact redirect is prohibited by discard-stderr, added to this repo one version ago in v1.36.0 (#53):

$ node -e "…read hooks/rules/rules.json…"
action: block | bypass_marker: null
 2  blocks-bare-2-dev-null            "gh api foo <discarding redirect>"
 2  blocks-append-form                "node x.mjs <appending discard>"
 2  blocks-ordered-both-streams       "ls -la >/dev/null 2>&1"
 2  blocks-ampersand-form             "find . -name x &>/dev/null"
 0  allows-stdout-only                "ls -la >/dev/null"
 0  allows-stderr-to-a-file           "node s.mjs 2>>\"$log\""

(The two blocked forms are spelled out in hooks/rules/rules.json; they are written descriptively here because quoting them literally trips the very guard under discussion.)

Those tests pass in the shipped suite — npm run test-hooks, all nine discard-stderr cases PASS. The rule is action: block with bypass_marker: null, the only rule in this repo with no way past it, deliberately.

The reasoning is in that PR and applies directly: a failing command with its stderr discarded is indistinguishable from a succeeding one that printed nothing. Suppressing this particular message would be an unusually bad instance of it, because the message is the signal the fall-through depends on — fatal: ref refs/remotes/origin/HEAD is not a symbolic ref is precisely how a reader learns why the probe ran. Silenced, an origin/HEAD that is set-but-broken looks identical to one that is simply unset.

SKILL.md already documents that line as an expected fall-through and quotes the message verbatim, so the noise is explained rather than hidden. If suppression were ever wanted, the sanctioned form in this repo is 2>>"$logfile" (allows-stderr-to-a-file above) — routing stderr somewhere, never discarding it.

Worth noting for calibration: this guard fired three times during work on this PR, including on the heredoc that was composing this very comment. It is not theoretical.

Standing verdict on ac770c7: APPROVED, 0 blockers, 0 P1, 0 P2. 340/340 hooks, 60/60 vitest, MERGEABLE/CLEAN. Not merging — merge timing is the maintainers' call.

Created by Claude Code on behalf of @lapc506