refactor: replace session hook with ${CLAUDE_SESSION_ID} template variable - #14
Conversation
…able
- Delete .claude/hooks/session-start.sh and get-session-id.sh
- SKILL.md now uses ${CLAUDE_SESSION_ID} (built-in template variable
replaced at skill load time) instead of a bash script + hook pipeline
- Replace curl with node for handoff API call (fixes JSON/unicode
encoding issues on Windows)
- Remove ~120 lines of hook install/uninstall/upsert code from skill.ts
- Improve handoff 403 error message to guide user to refresh connection
There was a problem hiding this comment.
Pull request overview
This PR refactors the /handoff Claude Code skill to stop relying on a SessionStart hook + helper scripts for session ID detection, and instead uses the built-in ${CLAUDE_SESSION_ID} template variable. It also updates the skill’s handoff invocation guidance to use node -e rather than curl, and improves server-side error messaging for Teams auth-expired scenarios.
Changes:
- Remove the session-start hook pipeline and
get-session-id.sh, switching session ID sourcing to${CLAUDE_SESSION_ID}. - Simplify CLI install/uninstall by removing hook install/uninstall/upsert logic and no longer installing helper scripts.
- Improve
/api/handofferror response messaging for 401/403 cases to guide users to refresh the Teams connection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/index.ts |
Improves error handling messaging for Teams auth-expired responses during handoff send. |
src/cli/utils.ts |
Removes ensureExecutable helper (no longer needed after dropping scripts/hooks). |
src/cli/skill.ts |
Removes hook install/uninstall logic and only installs SKILL.md for the handoff skill. |
.claude/skills/handoff/SKILL.md |
Switches to ${CLAUDE_SESSION_ID} and documents node -e based handoff call. |
.claude/skills/handoff/get-session-id.sh |
Deleted legacy session ID helper script. |
.claude/hooks/session-start.sh |
Deleted legacy SessionStart hook script. |
Comments suppressed due to low confidence (1)
src/cli/skill.ts:18
removeSessionStartHookis no longer exported from this module, but the test suite still imports it (seetests/cli-utils.test.ts). This will cause typecheck/test failures. Either update/remove the tests and any remaining callers, or keep a backward-compatible export (even if it becomes a no-op) for one release to avoid breaking consumers.
export function getConversationRefsPath(): string {
return path.join(TEAMS_BOT_DATA_DIR, "conversation-refs.json");
}
export async function maybeInstallSkillPrompt(): Promise<void> {
// Check where it's currently installed
const globalPath = path.join(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const status = | ||
| (err as { response?: { status?: number } })?.response?.status; |
There was a problem hiding this comment.
The error-status detection in this catch block only checks err.response.status, which is an Axios-style shape; errors from @microsoft/teams.* typically expose HTTP status as statusCode (or a different field). As a result, 401/403 auth-expired cases may still fall through to the generic 500 response. Consider normalizing the status extraction to also check common fields like statusCode (and/or nested body/error shapes) so the improved message reliably triggers.
| const status = | |
| (err as { response?: { status?: number } })?.response?.status; | |
| const errorWithStatus = err as { | |
| status?: number; | |
| statusCode?: number; | |
| response?: { status?: number }; | |
| body?: { statusCode?: number; error?: { statusCode?: number } }; | |
| error?: { statusCode?: number; response?: { status?: number } }; | |
| }; | |
| const status = | |
| errorWithStatus?.response?.status ?? | |
| errorWithStatus?.statusCode ?? | |
| errorWithStatus?.status ?? | |
| errorWithStatus?.body?.statusCode ?? | |
| errorWithStatus?.body?.error?.statusCode ?? | |
| errorWithStatus?.error?.statusCode ?? | |
| errorWithStatus?.error?.response?.status; |
| @@ -118,10 +118,20 @@ expressAdapter.post( | |||
| res.json({ success: true }); | |||
| } catch (err) { | |||
| console.error(`[HANDOFF] ${err}`); | |||
There was a problem hiding this comment.
console.error([HANDOFF] ${err}) stringifies the error and often loses stack/structured details (frequently prints [object Object]). Logging the error object directly (or including err instanceof Error ? err.stack : err) will preserve the stack trace and make production debugging much easier.
| console.error(`[HANDOFF] ${err}`); | |
| console.error("[HANDOFF]", err); |
| function installSkillFiles(destinationDir: string, sourceDir: string): void { | ||
| fs.mkdirSync(destinationDir, { recursive: true }); | ||
|
|
||
| const files = ["SKILL.md", "get-session-id.sh"]; | ||
| for (const fileName of files) { | ||
| const source = path.join(sourceDir, fileName); | ||
| const destination = path.join(destinationDir, fileName); | ||
| fs.copyFileSync(source, destination); | ||
| ensureExecutable(destination); | ||
| } | ||
| const source = path.join(sourceDir, "SKILL.md"); | ||
| const destination = path.join(destinationDir, "SKILL.md"); | ||
| fs.copyFileSync(source, destination); | ||
| } |
There was a problem hiding this comment.
installSkillFiles now only overwrites SKILL.md and leaves any previously-installed files in place (e.g., get-session-id.sh from older installs). If the user chooses “Reinstall / change scope”, the destination directory may retain stale files indefinitely. Consider cleaning the destination directory (or explicitly removing the legacy script) before copying the new file set.
| export async function uninstallSkill(): Promise<void> { | ||
| const skillDirs = [ | ||
| path.join(homeDir, ".claude", "skills", "handoff"), | ||
| path.join(process.cwd(), ".claude", "skills", "handoff"), | ||
| ]; | ||
|
|
||
| for (const skillDir of skillDirs) { | ||
| if (fs.existsSync(skillDir)) { | ||
| fs.rmSync(skillDir, { recursive: true, force: true }); | ||
| console.log(`Removed skill from ${skillDir}`); | ||
| } | ||
| } | ||
|
|
||
| const settingsFiles = [ | ||
| path.join(homeDir, ".claude", "settings.json"), | ||
| path.join(process.cwd(), ".claude", "settings.json"), | ||
| ]; | ||
|
|
||
| for (const settingsFile of settingsFiles) { | ||
| if (!fs.existsSync(settingsFile)) { | ||
| continue; | ||
| } | ||
|
|
||
| const settings = readJson(settingsFile); | ||
| const removed = removeSessionStartHook(settings); | ||
|
|
||
| if (removed) { | ||
| writeJson(settingsFile, settings); | ||
| console.log(`Removed hook from ${settingsFile}`); | ||
| } | ||
| } | ||
|
|
||
| console.log("Uninstalled /handoff skill and hook."); | ||
| console.log("Uninstalled /handoff skill."); | ||
| } |
There was a problem hiding this comment.
uninstallSkill no longer removes legacy SessionStart hook entries from settings.json. For users upgrading from previous versions, this can leave a broken hook configured that points at a removed session-start.sh, potentially causing noisy failures every session start. Consider keeping a one-time cleanup of the legacy hook config during uninstall (or during install/upgrade) even if new installs no longer add hooks.
| allowedTools: | ||
| - Bash(curl*) | ||
| - Bash(*/get-session-id.sh) | ||
| - Bash(echo*) | ||
| - Bash(ps*) | ||
| - "Bash(node -e *)" | ||
| --- | ||
|
|
||
| # Handoff to Teams | ||
|
|
||
| When the user runs `/handoff`: | ||
|
|
||
| 1. Get the session ID using the helper script: | ||
| Session ID: ${CLAUDE_SESSION_ID} | ||
|
|
||
| ```bash | ||
| SKILL_DIR="$(dirname "$(readlink -f ~/.claude/skills/handoff/SKILL.md 2>/dev/null || echo .claude/skills/handoff/SKILL.md)")" | ||
| SID=$("$SKILL_DIR/get-session-id.sh") | ||
| echo "SESSION_ID=${SID:-not found}" | ||
| ``` | ||
| When the user runs `/handoff`: | ||
|
|
||
| 2. If session ID is empty, ask the user to run /status and paste their Session ID. | ||
| 1. The session ID is already available above via template variable. If it shows as empty or literal `${CLAUDE_SESSION_ID}`, ask the user to run /status and paste their Session ID. | ||
|
|
||
| 3. **Generate a session summary** before calling the API. Based on the current conversation, prepare these fields in the **same language as the conversation** (do NOT default to English): | ||
| - `workDir`: the current working directory (use `pwd`) | ||
| 2. **Generate a session summary** before calling the API. Based on the current conversation, prepare these fields in the **same language as the conversation** (do NOT default to English): | ||
| - `workDir`: the current working directory (use `pwd`, always use forward slashes) | ||
| - `sessionId`: the session ID from above |
There was a problem hiding this comment.
allowedTools now only allows Bash(node -e *), but step 2 instructs using pwd to obtain workDir. If Claude enforces allowedTools strictly, the skill won’t be able to run pwd. Either allow Bash(pwd*) as well, or adjust the instructions/script to derive workDir without calling pwd (e.g., from the Node process cwd).
| node -e " | ||
| const http = require('http'); | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const os = require('os'); | ||
| const payload = JSON.stringify({ | ||
| workDir: 'WORK_DIR_HERE', | ||
| sessionId: 'SESSION_ID_HERE', | ||
| title: 'TITLE_HERE', | ||
| summary: 'SUMMARY_HERE', | ||
| todos: TODOS_ARRAY_HERE, | ||
| buttonText: 'BUTTON_TEXT_HERE' | ||
| }); | ||
| const url = new URL(process.env.TEAMS_BOT_URL || 'http://localhost:3978'); | ||
| let token; | ||
| try { token = process.env.HANDOFF_TOKEN || fs.readFileSync(path.join(os.homedir(), '.claude/teams-bot/handoff-token'), 'utf8').trim(); } catch { token = ''; } | ||
| const req = http.request({ | ||
| hostname: url.hostname, port: url.port || 3978, path: '/api/handoff', | ||
| method: 'POST', |
There was a problem hiding this comment.
The Node snippet always uses the http module even if TEAMS_BOT_URL is https://..., which will fail for TLS endpoints. Consider selecting http vs https based on new URL(...).protocol (and ideally preserving any non-root base path from TEAMS_BOT_URL when building the request path).
- Remove removeSessionStartHook tests (function was deleted) - SKILL.md: use allowed-tools (kebab-case per docs), curl + heredoc - Clean up leftover allowedTools key from frontmatter
- Replace teamsApp.send() with direct Bot Framework REST API call (SDK's app.send had token scope issues causing 403) - Add specific error messages for conversation expiry, auth failure, and token issues - Clean stale destination dir on skill reinstall - Add legacy hook cleanup in uninstall for upgrade path - Revert unused saveServiceUrl in store.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const settings = readJson(settingsFile); | ||
| const removed = removeSessionStartHook(settings); | ||
|
|
||
| if (removed) { | ||
| const hooks = settings.hooks as Record<string, unknown> | undefined; | ||
| if (!hooks?.SessionStart) continue; | ||
| const groups = hooks.SessionStart as Array<Record<string, unknown>>; | ||
| const filtered = groups.filter((g) => { | ||
| const gh = Array.isArray(g.hooks) ? g.hooks : []; | ||
| return !gh.some( | ||
| (h: Record<string, unknown>) => | ||
| typeof h.command === "string" && | ||
| h.command.includes("session-start.sh"), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
In uninstallSkill(), SessionStart hook cleanup assumes settings.hooks.SessionStart is an array and that each group/hook is an object. If the settings file contains a different shape (e.g., SessionStart is an object/string, or the array contains nulls), this will throw at runtime (groups.filter, g.hooks, etc.) and prevent uninstall. Add defensive checks (e.g., verify hooks is a plain object, Array.isArray(SessionStart), and validate group/hook entries before property access) before filtering/removing legacy session-start.sh entries.
| // Clean up legacy SessionStart hooks that pointed to the now-removed session-start.sh | ||
| const settingsFiles = [ | ||
| path.join(homeDir, ".claude", "settings.json"), | ||
| path.join(process.cwd(), ".claude", "settings.json"), | ||
| ]; | ||
|
|
||
| for (const settingsFile of settingsFiles) { | ||
| if (!fs.existsSync(settingsFile)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (!fs.existsSync(settingsFile)) continue; | ||
| const settings = readJson(settingsFile); | ||
| const removed = removeSessionStartHook(settings); | ||
|
|
||
| if (removed) { | ||
| const hooks = settings.hooks as Record<string, unknown> | undefined; | ||
| if (!hooks?.SessionStart) continue; | ||
| const groups = hooks.SessionStart as Array<Record<string, unknown>>; | ||
| const filtered = groups.filter((g) => { | ||
| const gh = Array.isArray(g.hooks) ? g.hooks : []; | ||
| return !gh.some( | ||
| (h: Record<string, unknown>) => | ||
| typeof h.command === "string" && | ||
| h.command.includes("session-start.sh"), | ||
| ); | ||
| }); | ||
| if (filtered.length < groups.length) { | ||
| if (filtered.length === 0) delete hooks.SessionStart; | ||
| else hooks.SessionStart = filtered; | ||
| if (Object.keys(hooks).length === 0) delete settings.hooks; | ||
| writeJson(settingsFile, settings); | ||
| console.log(`Removed hook from ${settingsFile}`); | ||
| console.log(`Removed legacy hook from ${settingsFile}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new legacy SessionStart hook cleanup logic in uninstallSkill() isn’t covered by tests, and the prior unit tests for hook removal were deleted along with removeSessionStartHook(). Add vitest coverage for uninstallSkill’s hook cleanup (including mixed hook groups and malformed settings shapes) to prevent regressions/crashes when uninstalling.
| - Bash(*/get-session-id.sh) | ||
| - Bash(echo*) | ||
| - Bash(ps*) | ||
| allowed-tools: Bash(curl *) |
There was a problem hiding this comment.
The skill front matter changed from allowedTools: to allowed-tools: and the value is now a single string Bash(curl *). This likely won’t be recognized by Claude Code’s skill loader (and the space in curl * may prevent matching). Consider keeping the expected allowedTools key and YAML list format (and include any commands actually used, e.g., curl invocation that reads the token from a file). If the loader expects a different schema, this file should match that schema consistently.
| allowed-tools: Bash(curl *) | |
| allowedTools: | |
| - Bash(curl *) |
| 3. Send via curl with a single-quoted heredoc (no shell expansion, safe for unicode and special chars): | ||
|
|
||
| ```bash | ||
| curl -s --ipv4 -w "\nHTTP_STATUS:%{http_code}" -X POST "${TEAMS_BOT_URL:-http://localhost:3978}/api/handoff" \ | ||
| curl -s -X POST "${TEAMS_BOT_URL:-http://localhost:3978}/api/handoff" \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "x-handoff-token: ${HANDOFF_TOKEN:-$(cat "$HOME/.claude/teams-bot/handoff-token" 2>/dev/null)}" \ | ||
| -d '{ ... your JSON here ... }' | ||
| ``` | ||
|
|
||
| IMPORTANT: You must fill in the actual summary, todos, and buttonText values based on the conversation context. Do NOT use placeholders. | ||
|
|
||
| 5. If the response contains `"success":true` or HTTP_STATUS is 200: | ||
|
|
||
| ``` | ||
| Handoff sent! A forked session has been created on Teams — check Teams to continue. | ||
| You can keep working here — both sides work independently on the same codebase. | ||
| -H "x-handoff-token: $(cat "$HOME/.claude/teams-bot/handoff-token" 2>/dev/null)" \ | ||
| -w "\nHTTP_STATUS:%{http_code}" \ | ||
| -d @- <<'EOF' | ||
| YOUR_JSON_HERE | ||
| EOF | ||
| ``` |
There was a problem hiding this comment.
PR description says the handoff API call was switched from curl to node, but SKILL.md still instructs using curl for the request. Either update the skill instructions to use the new node approach or adjust the PR description so it matches the actual behavior/documentation.
| 3. Send via curl with a single-quoted heredoc (no shell expansion, safe for unicode and special chars): | ||
|
|
||
| ```bash | ||
| curl -s --ipv4 -w "\nHTTP_STATUS:%{http_code}" -X POST "${TEAMS_BOT_URL:-http://localhost:3978}/api/handoff" \ | ||
| curl -s -X POST "${TEAMS_BOT_URL:-http://localhost:3978}/api/handoff" \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "x-handoff-token: ${HANDOFF_TOKEN:-$(cat "$HOME/.claude/teams-bot/handoff-token" 2>/dev/null)}" \ | ||
| -d '{ ... your JSON here ... }' | ||
| ``` | ||
|
|
||
| IMPORTANT: You must fill in the actual summary, todos, and buttonText values based on the conversation context. Do NOT use placeholders. | ||
|
|
||
| 5. If the response contains `"success":true` or HTTP_STATUS is 200: | ||
|
|
||
| ``` | ||
| Handoff sent! A forked session has been created on Teams — check Teams to continue. | ||
| You can keep working here — both sides work independently on the same codebase. | ||
| -H "x-handoff-token: $(cat "$HOME/.claude/teams-bot/handoff-token" 2>/dev/null)" \ | ||
| -w "\nHTTP_STATUS:%{http_code}" \ | ||
| -d @- <<'EOF' | ||
| YOUR_JSON_HERE | ||
| EOF | ||
| ``` | ||
|
|
||
| 6. If the API call fails or HTTP_STATUS is not 200: | ||
| IMPORTANT: Replace `YOUR_JSON_HERE` with the actual JSON from step 2. The heredoc is single-quoted — content is passed verbatim, no escaping needed. |
There was a problem hiding this comment.
The text says the curl step uses a single-quoted heredoc with “no shell expansion”, but the command still performs parameter expansion (${TEAMS_BOT_URL:-...}) and command substitution ($(cat ...)). Reword this to clarify that the JSON body is passed verbatim (no expansion) rather than implying the entire command has no shell expansion.
- Move rate limiter, token acquisition, REST API call, and error handling from index.ts into dedicated handoff/api.ts module - Fix TS2783: remove duplicate 'type' in activity spread - SKILL.md: show specific error messages from bot response - Revert unused saveServiceUrl in store.ts
- Add tests/handoff-api.test.ts: auth, token failure, successful send, conversation expiry, 403 rejection, rate limiting (7 tests) - Remove copied rate limiter algorithm from security.test.ts — now tested via the real code in handoff/api.ts
Summary
session-start.shhook andget-session-id.sh— no longer needed${CLAUDE_SESSION_ID}(built-in template variable, replaced at skill load time) instead of bash script + hook pipelinecurlwithnodefor handoff API call — fixes JSON/unicode encoding and Windows path escaping issuesskill.tsWhy
The old approach required:
SessionStarthook to write session ID to~/.claude/current-session-idget-session-id.sh) to read it back (or walk the process tree)jqorpythonfor JSON parsing in the hookThis broke on Windows (Git Bash
pslacks-o, hook didn't fire,jqnot installed) and was fragile on all platforms (--resumewithout args has no UUID in cmdline).${CLAUDE_SESSION_ID}is a built-in template variable that Claude Code replaces at skill load time — zero dependencies, cross-platform, works with all launch modes.Test plan
${CLAUDE_SESSION_ID}correctly replaced when/handoffis invokednode -eHTTP call sends valid JSON with unicode (中文) contentteams-bot install-skillno longer installs hooksteams-bot uninstall-skillno longer tries to remove hooks