fix(work-items): resolve consumer-local gh-bot.sh wrapper independent of adapter location

[kyle-sexton]

Summary

• The GitHub adapter's WIT_GH_BOT resolved the bot wrapper relative to the adapter's own directory (${CLAUDE_PLUGIN_ROOT}/tools/github-auth/gh-bot.sh in the normal bundled path), never checking the consumer-local override path CONTRACT.md already documented (${CLAUDE_PROJECT_DIR}/tools/github-auth/gh-bot.sh).
• As a result, a consuming repo's own bot wrapper was never found, and all tracker writes (item create, lease/reclaim comments) silently fell back to the ambient gh (session-user) identity instead of bot attribution.

Fix

• common.sh: extracted wit_gh_resolve_bot_wrapper, which now resolves the bot wrapper consumer-local-first, plugin-bundled fallback — checking ${CLAUDE_PROJECT_DIR}/tools/github-auth/gh-bot.sh first (independent of where the adapter itself resolved from, so a shadowed consumer-local adapter still finds the consumer's wrapper), then falling back to the bundled path beside the seam tree. This mirrors the existing two-rule adapter resolution documented in CONTRACT.md's "Adapter resolution" section.
• CONTRACT.md: updated the "Identity routing (GitHub adapter)" section to document the corrected consumer-local-first/plugin-bundled-fallback resolution order (previously it documented the buggy adapter-relative-only behavior).
• common.test.sh: added regression coverage for wit_gh_resolve_bot_wrapper — resolves the consumer-local wrapper when present, falls back to the bundled path when the consumer has none, and falls back when CLAUDE_PROJECT_DIR is unset.

Verification

Full adapter + dispatcher test suite run, all green:

$ bash adapters/github/common.test.sh
PASS: [1] common.sh exposes gh_write
PASS: [2] common.sh exposes wit_run_gh
PASS: [3] common.sh exposes wit_resolve_repo
PASS: [4] common.sh exposes wit_emit_item
PASS: [5] common.sh exposes wit_lease_json
PASS: [6] common.sh exposes wit_lease_is_live
PASS: [7] common.sh exposes wit_list_lease_comments
PASS: [8] common.sh exposes wit_help_if_requested
PASS: [9] common.sh exposes wit_map_gh_error
PASS: [10] lease marker constant
PASS: [11] 404 → not-found (5)
PASS: [12] rate limit → unavailable (8)
PASS: [13] resolves consumer-local wrapper first when present
PASS: [14] falls back to bundled path when consumer has none
PASS: [15] falls back to bundled path when CLAUDE_PROJECT_DIR unset

All other adapters/github/*.test.sh files and work-item-tracker.test.sh (including its own consumer-local/adapter-resolution cases) also pass unchanged.

shellcheck on both modified shell files: no findings.

Closes #365

Related

• work-items: github adapter should resolve a consumer-local gh-bot.sh wrapper independent of adapter location #365
• Originates from the feat(work-items): bundle work-item-tracker seam in plugin (shape A) #224 review thread (deferred dependency chore: initialize marketplace scaffold #1, option (c))

[claude]

Claude finished @kyle-sexton's task in 1m 53s —— View job

---

PR Review — fix(work-items): consumer-local gh-bot.sh wrapper resolution

• Gather context and understand the request
• Review changed files
  • common.sh — new wit_gh_resolve_bot_wrapper function
  • common.test.sh — regression test coverage
  • CONTRACT.md — documentation update
  • plugin.json — version bump
  • CHANGELOG.md — changelog entry
• Assess correctness, security, and alignment with project guidelines
• Post final review

---

Assessment: Approve

The fix is correct, minimal, and architecturally consistent. The bug (adapater-relative-only resolution of gh-bot.sh bypassing the documented consumer-local override path) is cleanly fixed without scope creep.

---

common.sh — wit_gh_resolve_bot_wrapper

common.sh L44–56

The implementation is correct:

• Reads CLAUDE_PROJECT_DIR dynamically (not captured at function definition time), so subshell overrides in tests work cleanly.
• The ${CLAUDE_PROJECT_DIR:-} guard avoids a set -u failure when the variable is unset — correct, matches how the adapter resolution guard handles the same variable.
• The bundled fallback $WIT_GH_ADAPTER_DIR/../../../github-auth/gh-bot.sh is deliberately non-normalized (matching the prior behavior); [[ -f ... ]] in gh_write handles .. correctly.
• Split from readonly WIT_GH_BOT=... to WIT_GH_BOT="$(...)"; readonly WIT_GH_BOT is the standard Bash pattern for assigning a computed value and immediately making it read-only.

One observation worth noting: WIT_GH_BOT is resolved once at source time and cached as readonly. The _WIT_GH_COMMON_LOADED guard means if common.sh is re-sourced in the same shell, the previously resolved value is kept — correct, since CLAUDE_PROJECT_DIR is expected stable within a session.

No issues.

---

common.test.sh — regression coverage

common.test.sh L30–52

Three cases, covering the contract exactly:

• Consumer-local present (L39–44): uses -ef (inode-based comparison) rather than string equality — correctly handles any normalisation differences between the path the function returns and the path constructed in the test.
• Consumer root exists but has no wrapper (L46–47): string-equals the bundled fallback. BUNDLED is constructed with the same $WIT_GH_ADAPTER_DIR/../../../... form the function emits, so the comparison is stable.
• CLAUDE_PROJECT_DIR unset (L49–50): runs in a subshell ($()), so unset is scoped to the subshell and doesn't affect the parent test environment. Correct.

Temp dirs are cleaned up (L52). Tests don't add wit_gh_resolve_bot_wrapper to the public-API declaration list — correct, it's an internal helper.

No issues.

---

CONTRACT.md

CONTRACT.md L286–295

The updated "Identity routing" section accurately reflects the implemented behaviour: consumer-local-first, plugin-bundled fallback, independent of where the adapter resolved from. The cross-reference to "Adapter resolution" makes the design intent clear. The "zero vendoring" phrasing is a useful new clarification for consuming-repo authors.

No issues.

---

plugin.json / CHANGELOG.md

Patch version bump (0.18.1 → 0.18.2) is appropriate — this is a bug fix, not a feature addition. CHANGELOG entry is thorough and consistent with the format of surrounding entries (no dates, which matches the rest of the file).

No issues.

---

Summary

Clean, targeted fix that closes the gap between documented and implemented bot-wrapper resolution. All three critical resolution paths are covered by tests. No security concerns (the wrapper path is validated with [[ -f ... ]] before execution; the wrapper lives in the consumer's own repo tree, which is already trusted). No scope creep. Good to merge.

[claude]

Claude finished @kyle-sexton's task in 2m 20s —— View job

---

Security Review

• Gather PR diff and changed files
• Analyze common.sh for shell injection, path traversal, and credential handling
• Analyze common.test.sh for test-introduced risks
• Analyze CONTRACT.md and plugin.json for config/permission risks
• Synthesize and post findings

---

Findings

SUGGESTION — CLAUDE_PROJECT_DIR is not canonicalized before path construction · Confidence: Low

File: plugins/work-items/tools/work-item-tracker/adapters/github/common.sh

wit_gh_resolve_bot_wrapper() {
  if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
    local consumer_wrapper="$CLAUDE_PROJECT_DIR/tools/github-auth/gh-bot.sh"
    if [[ -f "$consumer_wrapper" ]]; then
      printf '%s\n' "$consumer_wrapper"

The constructed path is later passed to bash "$WIT_GH_BOT" (line 98). Because CLAUDE_PROJECT_DIR is not canonicalized (e.g., via realpath), a value containing .. segments could resolve outside the project root. A theoretical attacker who controls CLAUDE_PROJECT_DIR (e.g., via environment injection in a multi-tenant CI scenario) could cause an arbitrary file to be executed as the bot wrapper.

Confidence is low because in normal Claude Code operation CLAUDE_PROJECT_DIR is set by the platform harness and is not consumer-controllable. The rest of the plugin already uses CLAUDE_PROJECT_DIR without canonicalization, so this is consistent with the established trust model rather than a regression. Defense-in-depth would call for CLAUDE_PROJECT_DIR="$(realpath -- "$CLAUDE_PROJECT_DIR")" at the top of the function, but it is not strictly necessary given the platform trust model.

Fix this →

---

Summary

No CRITICAL or IMPORTANT security issues were found in this PR.

• Shell injection / command injection: arguments to gh_write are passed through as "$@" — properly double-quoted throughout. No injection surface.
• Credential/token handling: unchanged. The bot wrapper executes with the wrapper script's own ambient credentials; WIT_GH_BOT is captured at source time via readonly, preventing late mutation.
• Path traversal: the constructed path flows through [[ -f ... ]] before bash execution. The check is necessary for the consumer-local-first fallback logic to work correctly. The pre-existing ../ sequences in the bundled fallback path are resolved by the shell naturally and were present before this PR.
• Trust model: this change correctly implements the behavior already documented in CONTRACT.md's "Identity routing" section. The consumer controlling ${CLAUDE_PROJECT_DIR}/tools/github-auth/gh-bot.sh is an intentional capability — a consumer who can write to their own project root already has full code execution there.
• Test code (common.test.sh): mktemp -d + set -u + cleanup with rm -rf is correct. The [[ -ef ]] (same-inode) comparison for path resolution testing is the right idiom here.
• plugin.json / CONTRACT.md: version bump and documentation update only — no security-relevant configuration changes.

[kyle-sexton]

Post-green review classification (babysit lane):

Both automated PR reviews returned Approve / Good to merge with no blocking findings. Both automated security-review runs raised one identical Low-confidence defense-in-depth item, classified below.

#	Finding	Classification	Evidence	Reacted
1	CLAUDE_PROJECT_DIR not canonicalized before building consumer_wrapper in wit_gh_resolve_bot_wrapper (raised by both automated security-review runs)	VALID (defer)	Claim is technically true — no realpath at common.sh L45-46 — but non-actionable in isolation. The reviewer itself marks it Low confidence and "not strictly necessary given the platform trust model": CLAUDE_PROJECT_DIR is platform-set (not consumer-controllable in normal operation), and a consumer who can write ${CLAUDE_PROJECT_DIR}/tools/github-auth/gh-bot.sh already holds code execution in their own trusted tree. No other CLAUDE_PROJECT_DIR consumer in the plugin canonicalizes (grep: zero realpath in the adapter tree), so hardening only this one call site would be an inconsistent band-aid, not a root-cause fix. Deferred as a repo-wide hardening candidate: canonicalize consistently across all CLAUDE_PROJECT_DIR consumers, or not at all. Not a regression this diff introduces.	👍

No blocking or defer-forbidden item applies. No inline review threads exist on this PR (GraphQL query returned empty) — nothing to resolve.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @kyle-sexton's task in 1m 55s —— View job

---

Security Review

• Gather context and understand the request
• Read PR diff and changed files
• Analyze common.sh for shell injection, path traversal, credential handling
• Analyze common.test.sh for test-introduced risks
• Analyze CONTRACT.md, plugin.json, CHANGELOG.md for config/permission risks
• Synthesize and post findings

---

Findings

SUGGESTION — CLAUDE_PROJECT_DIR not canonicalized before path construction · Confidence: Low

File: plugins/work-items/tools/work-item-tracker/adapters/github/common.sh L45–52

wit_gh_resolve_bot_wrapper() {
  if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then
    local consumer_wrapper="$CLAUDE_PROJECT_DIR/tools/github-auth/gh-bot.sh"
    if [[ -f "$consumer_wrapper" ]]; then
      printf '%s\n' "$consumer_wrapper"
      return 0
    fi
  fi
  printf '%s\n' "$WIT_GH_ADAPTER_DIR/../../../github-auth/gh-bot.sh"
}

The resolved path is subsequently passed to bash "$WIT_GH_BOT" "$@". Because CLAUDE_PROJECT_DIR is not canonicalized (e.g., via realpath), a value containing .. segments could resolve the wrapper to a path outside the project root. In a theoretical multi-tenant CI environment where CLAUDE_PROJECT_DIR is attacker-controlled, this could cause an arbitrary file to be executed as the bot wrapper.

Confidence is low for two reasons: (1) in normal Claude Code operation CLAUDE_PROJECT_DIR is set by the platform harness and is not consumer-controllable, and (2) this is fully consistent with how CLAUDE_PROJECT_DIR is used throughout the rest of the plugin — this PR introduces no regression in the trust model.

Defense-in-depth fix: add local project_dir; project_dir="$(realpath -- "$CLAUDE_PROJECT_DIR" 2>/dev/null || printf '%s' "$CLAUDE_PROJECT_DIR")" and use $project_dir in place of $CLAUDE_PROJECT_DIR when building consumer_wrapper.

Fix this →

---

No other security issues found

Independent review confirms the prior automated security review's conclusions:

• Shell / command injection: "$@" is properly double-quoted throughout gh_write and wit_run_gh. No injection surface is introduced by this PR.
• Arbitrary file execution surface: The attack chain (CLAUDE_PROJECT_DIR → consumer_wrapper → bash "$WIT_GH_BOT") requires controlling a platform-set variable or writing a file to the consumer's own project root — both already in the trusted perimeter. The [[ -f "$WIT_GH_BOT" ]] guard at L97 is load-bearing for the fallback logic; it does not create a new execution surface.
• Credential / token handling: WIT_GH_BOT is captured once at source time via readonly. The _WIT_GH_COMMON_LOADED guard ensures the resolved path cannot be changed by a second source. No credentials flow through the new code path.
• bash hardcoded as interpreter for the wrapper (L98): pre-existing behavior, unchanged by this PR.
• Test code (common.test.sh): mktemp -d paths are properly double-quoted throughout; rm -rf targets are known mktemp outputs; [[ -ef ]] (inode comparison) is the correct idiom for path-resolution assertions; unset CLAUDE_PROJECT_DIR is correctly scoped inside $(...) so it does not pollute the parent test environment.
• plugin.json / CHANGELOG.md / CONTRACT.md: version bump and documentation only — no security-relevant configuration changes.

No CRITICAL or IMPORTANT findings. The SUGGESTION above is consistent with an existing codebase convention, not a regression this diff introduces.

[claude]

Claude finished @kyle-sexton's task in 1m 33s —— View job

---

PR Review — fix(work-items): consumer-local gh-bot.sh wrapper resolution

• Gather context and review changed files
• Assess correctness, security, and alignment with project guidelines
• Post final review

---

Assessment: Approve

This is a clean, targeted bug fix. The root cause (adapter-relative-only WIT_GH_BOT resolution silently bypassing the documented consumer-local path) is correctly identified and fixed without scope creep.

---

common.sh — wit_gh_resolve_bot_wrapper

common.sh L44–56

Correctness. All three resolution paths are correctly handled:

• CLAUDE_PROJECT_DIR set and consumer wrapper exists → returns consumer path.
• CLAUDE_PROJECT_DIR set but no consumer wrapper → falls through to bundled path.
• CLAUDE_PROJECT_DIR unset → ${CLAUDE_PROJECT_DIR:-} safely evaluates to empty under set -u; falls through to bundled path.

Bundled fallback path. $WIT_GH_ADAPTER_DIR/../../../github-auth/gh-bot.sh navigates three levels up from adapters/github/ to reach tools/, then into tools/github-auth/gh-bot.sh — which is the correct plugin-bundled location. The path is deliberately non-normalized (matching the prior behavior); [[ -f "$WIT_GH_BOT" ]] in gh_write (L97) handles .. correctly.

readonly assignment pattern. Splitting WIT_GH_BOT="$(...)"; readonly WIT_GH_BOT (L55–56) is the correct Bash idiom. Under set -e, assigning directly into readonly WIT_GH_BOT=$(...) would suppress the subshell exit code; the split form preserves it.

Source guard. The _WIT_GH_COMMON_LOADED guard at L8–9 means wit_gh_resolve_bot_wrapper is called exactly once. Since WIT_GH_BOT is readonly, re-sourcing in the same shell would fail at the assignment if the guard were absent — the guard makes this safe.

No issues.

---

common.test.sh — regression coverage

common.test.sh L30–52

Three cases cover the contract exactly:

• Consumer-local present (L39–44): uses -ef (inode comparison) — correctly handles any normalisation difference between the returned path and the test-constructed path.
• Consumer root exists, no wrapper (L46–47): BUNDLED is built with the same $WIT_GH_ADAPTER_DIR/../../../... form the function emits, so the string comparison is stable.
• CLAUDE_PROJECT_DIR unset (L49–50): unset runs inside $(...) so it's scoped to the subshell; the parent test environment is unaffected. Correct.

Temp dirs are cleaned up (L52). wit_gh_resolve_bot_wrapper is correctly absent from the public-API declaration list (L12–18) — it is an internal helper.

One minor observation: the test sources common.sh at L10, which captures WIT_GH_BOT via wit_gh_resolve_bot_wrapper and marks it readonly. The three new test cases then call wit_gh_resolve_bot_wrapper directly (not via the cached constant), which is intentional — the tests are exercising the resolution function, not the cached value. This is the right approach for regression coverage.

No issues.

---

CONTRACT.md — Identity routing section

CONTRACT.md L286–295

The updated "Identity routing" section now accurately documents the implemented behaviour: consumer-local-first, plugin-bundled fallback, independent of adapter location. The "zero vendoring" phrasing is a useful addition for consuming-repo authors. The cross-reference to "Adapter resolution" makes the design symmetry explicit.

No issues.

---

plugin.json / CHANGELOG.md

Patch version bump (0.18.1 → 0.18.2) is appropriate for a bug fix. CHANGELOG entry is thorough, accurate, and consistent with the format of surrounding entries.

No issues.

---

Summary

No correctness, security, or guideline concerns. The fix is minimal, all three resolution paths have regression tests, and the documentation is updated to match. Good to merge.

[kyle-sexton]

Tower merge-sit: merge-drive lane dark ~5.5h (rate-limit hold since ~10:36Z); tower sitting merges per stall protocol. Gate-verified live: CLEAN, 0 unresolved threads. This was generated by AI (control tower, session 6).