fix(#433): promote /handover MCP reindex to named step + add advisory hook by atlas-apex · Pull Request #439 · me2resh/apexyard

atlas-apex · 2026-05-28T16:54:27Z

Summary

Promotes the post-clone MCP reindex from prose comment to named sub-step 1.5-reindex — putting it structurally on the same footing as 1.5-clone above and 1.5 (topology) below so the agent's eye doesn't pass over it during execution
Replaces "skip silently" language with explicit non-silent failure semantics — emit a one-line warning, set REINDEX_STATUS=unavailable, continue. Silent skips are indistinguishable between "server down" and "agent forgot" — the second is the failure mode this PR exists to prevent
Adds an advisory PostToolUse hook (suggest-mcp-reindex-after-clone.sh) that fires after the matching git clone … workspace/<name> and emits a one-line reminder. Exit 0 always, non-blocking — same shape as detect-role-trigger.sh and check-upstream-drift.sh
Wires the hook in .claude/settings.json under PostToolUse Bash with if: "Bash(git clone *)"
11-case test suite under .claude/hooks/tests/ — 5 should-fire cases (workspace clones, quoted paths, absolute paths via portfolio helper, banner content, project-name extraction) + 6 should-not (non-workspace targets, failed clones with exit code != 0, wrong tool, empty input). All 11 pass

Why this matters

The /handover skill clones the target repo into workspace/<name>/ at step 1.5-clone (default behaviour since #417). The whole point is that subsequent reads in steps 2–6 use the local clone via search_code / search_docs and the MCP index — described as "3-15× cheaper per query" than grep + Read. That savings is conditional on the MCP index actually containing the cloned repo, which requires the reindex call.

Before this PR the reindex was a prose comment inside the bash block, framed as "best-effort — skip silently if the MCP server is not available." In practice that phrase reads as permission to skip unconditionally. The reindex got skipped even when the server was running, all deep-dive reads ran against an empty index, and the savings vanished.

Two changes together fix it: structural promotion (so the step is harder to miss in the SKILL.md), and an advisory hook (so even if it IS missed, the operator gets a visible reminder right after the clone command).

Testing

bash .claude/hooks/tests/test_suggest_mcp_reindex_after_clone.sh — 11 PASS, 0 FAIL
jq empty .claude/settings.json — valid JSON
Visual: render .claude/skills/handover/SKILL.md and confirm the new 1.5-reindex section sits between 1.5-clone and 1.5 (topology) with the warning + status-marker example blocks
Manual repro path: run /handover <url> against a URL, observe step 1.5-clone clone, observe the hook banner emitted after the clone, observe step 1.5-reindex triggering the MCP reindex tool call
On MCP unavailable: confirm the one-line warning prints and $REINDEX_STATUS=unavailable is set; subsequent steps fall back to grep + Read without further apology

Backwards compatibility

No new dependencies (the hook is pure bash + jq, already required)
Hook silently no-ops on non-workspace clones, failed clones, and non-Bash tool calls — adopters who don't use /handover see no change
The SKILL.md edit removes one prose comment block and adds one numbered sub-step; existing operators who already do the reindex by habit are unaffected (the new step formalises what they were already doing)

Closes #433.

Glossary

Term	Definition
1.5-clone	Step in `/handover` SKILL.md (added in #417) that clones the target repo into `workspace/<name>/` immediately when a Git URL is given. Default behaviour; cheapest moment to clone before any reads.
1.5-reindex	New step added by this PR — call `mcp__apexyard-search__reindex(scope="project", project="<name>")` after a successful clone so `search_code` / `search_docs` return results during the deep-dive phases (steps 2–6) that follow.
`REINDEX_STATUS`	New marker variable set by step 1.5-reindex: `"indexed"` on success, `"unavailable"` when the MCP server can't be reached, `"skipped"` when `CLONE_STATUS` isn't `"cloned"`. Steps 2–6 read it to decide between MCP search and `grep` + `Read`.
Advisory hook	Non-blocking PostToolUse hook that emits a banner to stderr and exits 0. Same pattern as `detect-role-trigger.sh` / `check-upstream-drift.sh` — removes the "I forgot the rule applied here" failure mode without changing tool behaviour.
PostToolUse	Claude Code hook event that fires after a tool call completes. Carries the tool input AND the response (exit code, output) — lets the hook condition on whether the operation succeeded before suggesting follow-ups.

… hook Bug: the post-clone MCP reindex in /handover was buried in a prose comment inside step 1.5-clone, surrounded by the phrase "skip silently if the MCP server is not available". In practice that reads as permission to skip unconditionally, so the reindex got skipped even when the server was running. All subsequent reads in steps 2-6 then fell back to grep + Read against a stale index, defeating the 3-15x token-cost benefit of cloning early. Fix: - Promote the reindex to its own named, numbered sub-step 1.5-reindex in handover SKILL.md, structurally parallel to 1.5-clone above and 1.5 (topology) below. Naming it makes it visually harder to skip during agent execution. - Replace the "skip silently" language with explicit non-silent failure semantics: emit a one-line warning, set REINDEX_STATUS=unavailable, then continue. Silent skips are the failure mode the change is trying to prevent. - Add a PostToolUse Bash hook (suggest-mcp-reindex-after-clone.sh) that fires after the matching git clone command and emits a one-line reminder banner. Advisory shape: exit 0 always, same pattern as detect-role-trigger.sh and check-upstream-drift.sh. Recognises both the literal workspace/<name> form (single-fork default) and absolute paths ending in /workspace/<name> (the helper-resolved form used in split-portfolio v2). Skips on failed clones (exit code != 0). - Wire the hook in .claude/settings.json under PostToolUse Bash with matcher `if: "Bash(git clone *)"`. - Add 11-case test suite at .claude/hooks/tests/test_suggest_mcp_reindex_after_clone.sh: 5 should-fire (workspace clones with various forms, banner content checks, project-name extraction) and 6 should-not (non-workspace clones, failed clones, non-clone commands, wrong tool, empty input). All 11 pass. Closes #433. ## Glossary | Term | Definition | |------|------------| | 1.5-clone | Step in /handover SKILL.md (added in #417) that clones the target repo into workspace/<name>/ immediately when a Git URL is given. Default behaviour; cheapest moment to clone before any reads. | | 1.5-reindex | New step added by this PR — call mcp__apexyard-search__reindex(scope="project", project="<name>") after a successful clone so search_code / search_docs return results during the deep-dive phases (steps 2-6) that follow. | | REINDEX_STATUS | New marker variable set by step 1.5-reindex: "indexed" on success, "unavailable" when the MCP server can't be reached, "skipped" when CLONE_STATUS isn't "cloned". Steps 2-6 read it to decide between MCP search and grep + Read. | | Advisory hook | Non-blocking PostToolUse hook that emits a banner to stderr and exits 0. Same pattern as detect-role-trigger.sh / check-upstream-drift.sh — removes the "I forgot the rule applied here" failure mode without changing tool behaviour. | | PostToolUse | Claude Code hook event that fires after a tool call completes. Carries the tool input AND the response (exit code, output) — lets the hook condition on whether the operation succeeded before suggesting follow-ups. | Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

atlas-apex

Code Review: PR #439

Commit: 6381f88efaaf397075dad529d36ccaab8469001e

Summary

Two coordinated fixes for the failure mode in #433: the /handover skill's post-clone MCP reindex was buried in a prose comment with "skip silently if MCP unavailable" language, and agents read that as permission to skip unconditionally. This PR (a) promotes the reindex to a named numbered sub-step 1.5-reindex between 1.5-clone and 1.5 (topology), with explicit non-silent failure semantics (REINDEX_STATUS=unavailable + one-line warning), and (b) adds an advisory PostToolUse hook (suggest-mcp-reindex-after-clone.sh) that fires after git clone … workspace/<name> and emits a one-line reminder banner — same shape as detect-role-trigger.sh / check-upstream-drift.sh.

Checklist Results

Architecture & Design: Pass — advisory hook follows the established non-blocking PostToolUse pattern; SKILL.md restructure cleanly slots the new step between two existing numbered headings.
Code Quality: Pass — bash syntax clean (bash -n OK), hook is well-commented, JSON parsing via jq matches sibling hooks, banner is concise (~280 chars, within the ≤600 budget the file documents).
Testing: Pass — 11-case test suite (5 should-fire + 6 should-not). Verified locally: PASS: 11, FAIL: 0. Covers single-fork form, quoted targets, absolute paths via portfolio helper, project-name extraction, non-workspace targets, failed clones (exit 1 and 128), and wrong-tool input.
Security: Pass — no secrets, no user-supplied input executed; the hook only reads from stdin JSON and writes to stderr.
Performance: Pass — hook is short, runs once per git clone PostToolUse, no shelling out beyond grep/sed/jq.
PR Description & Glossary: Pass — glossary present with 5 well-defined terms (1.5-clone, 1.5-reindex, REINDEX_STATUS, advisory hook, PostToolUse). Ticket linked (Closes #433). Testing instructions present.
Summary Bullet Narrative: Pass — every bullet answers what changed AND why it matters (especially the "skip silently is indistinguishable from server down vs agent forgot" framing on bullet 2).
Technical Decisions (AgDR): N/A — this is a tooling/skill polish PR. The "promote prose comment to named step" + "add advisory hook" pair follows existing framework patterns (detect-role-trigger.sh, check-upstream-drift.sh) and doesn't introduce a new technology, dependency, or architectural pattern that warrants AgDR ceremony.
Adopter Handbooks: Pass with one advisory finding (see below).

Issues Found

One non-blocking CI failure — site-counts drift detection fails because the PR adds a new hook (hooks count: 35 → 36) and site/*.html, site/*.md.gen, site/llms.txt, site/llms-full.txt, site/skill.md still claim 35. The CI log explicitly calls this out as the expected failure mode for "added a new hook in this PR — refresh the counts in site/". This needs fixing before merge per the no-red-CI rule (.claude/rules/pr-quality.md § "No Red CI Before Merge") — surface as a finding but it's a 7-file count update, not a substantive code change. Either fold the count updates into this PR or land a follow-up commit on the same branch before merge.

Drift list from the CI log:

site/architecture.html:545 — claims 35 shell gates, actual 36
site/index.md.gen:78 — claims 35 hooks, actual 36
site/architecture.md.gen:28 — claims 35 shell gates, actual 36
site/llms.txt:6 — claims 35 hooks, actual 36
site/llms.txt:57 — claims 35 shell hooks, actual 36
site/llms-full.txt:6 — claims 35 hooks, actual 36
site/skill.md:46 — claims 35 shell hooks, actual 36

Handbook Findings

Commit-message quality — handbooks/general/commit-message-quality.md (advisory)

Commit 6381f88 has subject fix(#433): promote /handover MCP reindex to named step + add advisory hook (good — descriptive, under 70 chars), but the commit body is empty for a 202-line diff across 4 files. Per the handbook § "What Rex flags" #2: substantive PRs (>50 lines) deserve a commit body explaining the WHY independently of the PR body. A future git blame reader reaches the commit message, not the PR description. Consider amending with a short body that captures the load-bearing rationale from the PR body's "Why this matters" section (the "skip silently is indistinguishable" framing especially). Non-blocking; PR-body narrative is already solid.

Suggestions

Hook robustness — workspace-name with hyphens via portfolio helper: the absolute-path branch of the regex (/workspace/[A-Za-z0-9._-]+) handles hyphens, and the literal-path branch via workspace/[^[:space:]"'…]+ accepts them too. Verified by the "absolute path via portfolio helper" test case extracting bar. No change needed — flagging as confirmed coverage.
Banner closing line: the third sentence — "If the MCP server is unavailable, print one-line warning + set REINDEX_STATUS=unavailable and continue" — duplicates what SKILL.md § 1.5-reindex says in more detail. Could be tightened to a one-liner ("On error: see § 1.5-reindex") to stay closer to the ~280-char banner budget; the current shape is still within budget so this is purely a polish nit.
SKILL.md cross-reference back to the hook: the closing paragraph of § 1.5-reindex mentions the hook by filename ("A PostToolUse hook (suggest-mcp-reindex-after-clone.sh) fires …"). Good — this gives the reader the symmetric link from skill ↔ hook, matching the framework's pattern (e.g. require-active-ticket.sh referenced from /start-ticket).

Verdict

COMMENT — the diff is well-shaped, well-tested, and addresses the documented failure mode cleanly. Merge blocked by the failing site-counts drift detection check (no-red-CI rule); update the 7 site/ files to reflect the new 36-hook total in the same branch and the gate clears. Once CI is green, this is ready to merge from the code-review side.

(Posted as COMMENT — author is self, GitHub blocks self-approve. Per the agent rules, no approval marker written on a COMMENT verdict.)

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 6381f88efaaf397075dad529d36ccaab8469001e

Adds the count refresh that the site-counts-check CI workflow requires when a new hook ships in the same PR. Touches the six files the drift test scans: - site/llms.txt — top-line "> 35 hooks" claim + bullet "35 shell hooks" - site/llms-full.txt — same two surfaces - site/index.md.gen — H3 "Guardrails that stop bad code from shipping · 35 hooks" - site/architecture.md.gen — Layer-1 bullet "35 shell gates" - site/architecture.html — SVG text label ".claude/hooks/ · 35 shell gates" - site/skill.md — "35 shell hooks enforce SDLC rules" Verified locally: bash .claude/hooks/tests/test_site_counts.sh skills: 55 hooks: 36 roles: 19 PASS: site framework counts match actuals across all scanned files. Refs #433.

atlas-apex

Code Re-Review: PR #439 (commit `ccd06d7`)

Re-review trigger: new commit ccd06d7 pushed since the last review at 6381f88 to address the site-counts-check CI failure.

Diff scope of the new commit — clean

The fixup commit ccd06d7 touches exactly 6 files, all under site/, all single-line 35 → 36 replacements. No scope creep — the SKILL.md, hook script, hook test, and settings.json from the original commit are untouched:

File	Change
`site/architecture.html`	SVG text label `.claude/hooks/ · 35 shell gates` → `36`
`site/architecture.md.gen`	Hooks bullet `35 shell gates` → `36`
`site/index.md.gen`	H3 `Guardrails … · 35 hooks` → `36`
`site/llms-full.txt`	Two surfaces: top-line claim + capability-layer bullet
`site/llms.txt`	Two surfaces: top-line claim + capability-layer bullet
`site/skill.md`	Body line `35 shell hooks enforce SDLC rules` → `36`

Each file's diff is exactly +1 −1 (or +2 −2 for the two-surface txt files). The commit message names all six files and includes the local verification output (hooks: 36 ✓).

CI status — all green

Check	Status
Verify Ticket ID	pass
lychee (link check)	pass
markdownlint-cli2	pass
shellcheck .claude/hooks	pass
site-counts drift detection	pass (was the failure I flagged)

The lychee transient-flake concern didn't materialise — it passed cleanly on this run. The site-counts gate is now satisfied because both sides of its equality (the count in the source files vs the actual hook count under .claude/hooks/) agree at 36.

Combined PR head SHA

ccd06d7be4eebdd0cd287b4f99a574bbcba9719b

Matches the SHA you named. mergeable: MERGEABLE.

Remaining issues

None blocking. Everything I flagged in the prior review at 6381f88 is now resolved:

✅ site-counts-check CI failure: fixed by the deterministic count refresh
✅ No scope creep into the substantive PR files
✅ Glossary section present and complete
✅ Summary bullets are narrative (what + why), no label-only nits
✅ AgDR not required (advisory hook + SKILL.md restructure, no new dependency / framework / architecture decision)

Verdict

APPROVED (modulo the self-author constraint — posting as --comment instead of --approve, no approval marker written).

The fix is the right shape: count refresh isolated in its own commit, deterministic local verification command in the commit body, scope strictly limited to the six files the drift test scans. Ready for the merge-gate hand-off once a second reviewer takes a look.

🤖 Re-reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: ccd06d7be4eebdd0cd287b4f99a574bbcba9719b

… hook (#439) * fix(#433): promote /handover MCP reindex to named step + add advisory hook Bug: the post-clone MCP reindex in /handover was buried in a prose comment inside step 1.5-clone, surrounded by the phrase "skip silently if the MCP server is not available". In practice that reads as permission to skip unconditionally, so the reindex got skipped even when the server was running. All subsequent reads in steps 2-6 then fell back to grep + Read against a stale index, defeating the 3-15x token-cost benefit of cloning early. Fix: - Promote the reindex to its own named, numbered sub-step 1.5-reindex in handover SKILL.md, structurally parallel to 1.5-clone above and 1.5 (topology) below. Naming it makes it visually harder to skip during agent execution. - Replace the "skip silently" language with explicit non-silent failure semantics: emit a one-line warning, set REINDEX_STATUS=unavailable, then continue. Silent skips are the failure mode the change is trying to prevent. - Add a PostToolUse Bash hook (suggest-mcp-reindex-after-clone.sh) that fires after the matching git clone command and emits a one-line reminder banner. Advisory shape: exit 0 always, same pattern as detect-role-trigger.sh and check-upstream-drift.sh. Recognises both the literal workspace/<name> form (single-fork default) and absolute paths ending in /workspace/<name> (the helper-resolved form used in split-portfolio v2). Skips on failed clones (exit code != 0). - Wire the hook in .claude/settings.json under PostToolUse Bash with matcher `if: "Bash(git clone *)"`. - Add 11-case test suite at .claude/hooks/tests/test_suggest_mcp_reindex_after_clone.sh: 5 should-fire (workspace clones with various forms, banner content checks, project-name extraction) and 6 should-not (non-workspace clones, failed clones, non-clone commands, wrong tool, empty input). All 11 pass. Closes #433. ## Glossary | Term | Definition | |------|------------| | 1.5-clone | Step in /handover SKILL.md (added in #417) that clones the target repo into workspace/<name>/ immediately when a Git URL is given. Default behaviour; cheapest moment to clone before any reads. | | 1.5-reindex | New step added by this PR — call mcp__apexyard-search__reindex(scope="project", project="<name>") after a successful clone so search_code / search_docs return results during the deep-dive phases (steps 2-6) that follow. | | REINDEX_STATUS | New marker variable set by step 1.5-reindex: "indexed" on success, "unavailable" when the MCP server can't be reached, "skipped" when CLONE_STATUS isn't "cloned". Steps 2-6 read it to decide between MCP search and grep + Read. | | Advisory hook | Non-blocking PostToolUse hook that emits a banner to stderr and exits 0. Same pattern as detect-role-trigger.sh / check-upstream-drift.sh — removes the "I forgot the rule applied here" failure mode without changing tool behaviour. | | PostToolUse | Claude Code hook event that fires after a tool call completes. Carries the tool input AND the response (exit code, output) — lets the hook condition on whether the operation succeeded before suggesting follow-ups. | Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(#433): bump site hook count 35 → 36 to match the new advisory hook Adds the count refresh that the site-counts-check CI workflow requires when a new hook ships in the same PR. Touches the six files the drift test scans: - site/llms.txt — top-line "> 35 hooks" claim + bullet "35 shell hooks" - site/llms-full.txt — same two surfaces - site/index.md.gen — H3 "Guardrails that stop bad code from shipping · 35 hooks" - site/architecture.md.gen — Layer-1 bullet "35 shell gates" - site/architecture.html — SVG text label ".claude/hooks/ · 35 shell gates" - site/skill.md — "35 shell hooks enforce SDLC rules" Verified locally: bash .claude/hooks/tests/test_site_counts.sh skills: 55 hooks: 36 roles: 19 PASS: site framework counts match actuals across all scanned files. Refs #433. --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

atlas-apex commented May 28, 2026

View reviewed changes

atlas-apex merged commit 86cde71 into me2resh:dev May 28, 2026
5 checks passed

This was referenced May 28, 2026

fix(/handover): MCP reindex step is easily skipped — promote to named sub-step + add mechanical backstop #433

Closed

release(#452): v2.2.0 #453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#433): promote /handover MCP reindex to named step + add advisory hook#439

fix(#433): promote /handover MCP reindex to named step + add advisory hook#439
atlas-apex merged 2 commits into
me2resh:devfrom
atlas-apex:fix/#433-handover-mcp-reindex

atlas-apex commented May 28, 2026

Uh oh!

atlas-apex left a comment

Uh oh!

atlas-apex left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

atlas-apex commented May 28, 2026

Summary

Why this matters

Testing

Backwards compatibility

Glossary

Uh oh!

atlas-apex left a comment

Choose a reason for hiding this comment

Code Review: PR #439

Summary

Checklist Results

Issues Found

Handbook Findings

Suggestions

Verdict

Uh oh!

atlas-apex left a comment

Choose a reason for hiding this comment

Code Re-Review: PR #439 (commit ccd06d7)

Diff scope of the new commit — clean

CI status — all green

Combined PR head SHA

Remaining issues

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Code Re-Review: PR #439 (commit `ccd06d7`)