Replace hand-rolled GitHub read tools with github-mcp-server (allowlisted subset)

[bokelley]

Problem

Addie's GitHub read tools have two functional gaps:

1. No diff / file / review-thread access. get_github_issue (server/src/addie/mcp/member-tools.ts:5395) only hits /repos/{org}/{repo}/issues/{number} — issue-shaped metadata + top-level comments. It cannot fetch:

   • PR diffs or changed files (/pulls/{N}/files, raw-diff content-type)
   • Inline review-thread comments (/pulls/{N}/comments)
   • Reviews (/pulls/{N}/reviews)
   • Commits (/pulls/{N}/commits, /commits/{sha})

   Result: when a user pastes a PR URL and asks Addie to review the change, she can read the description but not the actual code.

2. No fork access. parseAllowedRepo (member-tools.ts:425) hard-codes GITHUB_READ_ALLOWED_ORGS = ['adcontextprotocol', 'prebid'], so PRs from contributor forks (e.g. patmmccann/adcp) are rejected before the API call. Cross-fork PRs do live under the base repo's PR number, but that only helps once we have a diff endpoint to actually read.

Proposal

Mount the official github/github-mcp-server with an explicit read-only tool allowlist. ~6–8 tools mounted, not the full ~70.

Allowlist (initial):

• get_pull_request
• get_pull_request_diff
• get_pull_request_files
• get_pull_request_comments (review-thread comments)
• get_pull_request_reviews
• get_issue
• search_issues
• search_pull_requests

Auth: reuse the existing WorkOS Pipes GitHub connection per user (same path as create_github_issue). Reads inherit the user's repo permissions, so private forks work iff the user has access. No service-account token required.

Retire:

• get_github_issue (member-tools.ts:5395)
• list_github_issues (member-tools.ts:5479)

These become redundant once the GH MCP read tools land. Keep create_github_issue and draft_github_issue — they have specific WorkOS Pipes + draft-link UX that the GH MCP server doesn't replicate.

Trust boundary

Removing the org allowlist widens the prompt-injection surface: any PR body / review comment becomes attacker-controlled text in Addie's context. Existing mitigation is wrapUntrusted() around GitHub-sourced content. Audit task: confirm every github-mcp-server tool output is wrapped before reaching model context. If the GH MCP server returns structured JSON, add a thin output-mapper that wraps each user-controlled string field.

Out of scope (deferred)

• Single facade tool (github_read with action enum) — revisit if 6–8 tools clutter the catalog noticeably.
• Subagent-based isolation (read-only GH-reader agent) — revisit if PR-text injection in Addie's main loop becomes a real problem.

Acceptance

• github-mcp-server mounted with the allowlist above
• Tool outputs pass through wrapUntrusted (or equivalent) for any user-controlled field
• get_github_issue and list_github_issues removed; references updated
• Tool catalog (generated/tool-catalog.generated.ts) regenerates cleanly
• Manual smoke: PR diff on a fork PR, review threads on an adcontextprotocol/adcp PR

[bokelley]

Triage

Classification: Feature request
Bucket(s): addie, security-sensitive (no security-sensitive label exists in this repo — flagged in run summary)
Status: ready-for-human

What the experts said:

• security-reviewer: Blocker — wrapUntrusted() uses <untrusted-github-content> but neutralizeUntrustedTags() targets <untrusted_proposer_input>; the tags don't match, so an attacker embedding </untrusted_proposer_input> in a PR body already escapes the fence on the current path, and removing the org allowlist amplifies this across arbitrary public repos. MCP subprocess lifecycle (singleton vs. per-session) and per-user token injection path must be explicit before merge.
• prompt-engineer: Blocker — upstream github-mcp-server tool descriptions are uncontrolled (no usage_hints, no Addie routing conventions). The org allowlist is a blast-radius control, not a UX restriction; remove it separately with its own review. Eight new tools need explicit ALWAYS_AVAILABLE_TOOLS / TOOL_SETS anchoring or the "model hallucinates unavailability" pattern (Audit tool-set descriptions for always-available overlap to prevent Addie hallucinations #2998) recurs.

My take: Both experts agree the direction is sound but two design decisions must be resolved before implementation begins. The prompt-injection surface expansion makes this security-sensitive — it needs explicit sign-off regardless of implementation quality.

@bokelley, your call on two points:

1. Allowlist removal — keep or drop?

Option A — keep and expand case-by-case (experts prefer this; blast radius stays bounded):

- GITHUB_READ_ALLOWED_ORGS = ['adcontextprotocol', 'prebid']
+ GITHUB_READ_ALLOWED_ORGS = ['adcontextprotocol', 'prebid', 'patmmccann']

Option B — remove, rely on output-mapper as the sole gate (experts flag as insufficient alone):

- if (!GITHUB_READ_ALLOWED_ORGS.includes(owner)) throw new Error(...)
+ // all orgs permitted; output-mapper is merge gate

2. Description-override shim — the 8 upstream tool descriptions need usage_hints added and upstream phrasings audited before mounting. In scope for this PR, or a follow-up with a filed issue?

---

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

---

Generated by Claude Code

[bokelley]

Triage

Classification: Feature request
Bucket(s): addie, security-sensitive
Status: ready-for-human

What the experts said:

• security-reviewer: 4 pre-conditions before arbitrary-org reads are safe — (1) system-prompt anchor for the <untrusted-github-content> boundary is missing (the wrapper instruction currently arrives in the same turn as attacker-controlled content, with no prior anchor); (2) github-mcp-server returns structured JSON — an output-mapper that calls wrapUntrusted() on each user-controlled field must ship in the same PR; (3) removing the org allowlist opens per-user WorkOS Pipes tokens to any repo the user can access, including private ones — no equivalent of parseAllowedRepo is proposed; (4) sanitizeInline doesn't escape HTML in title/label fields that land inside the wrapper. Two medium concerns: WorkOS token scope is unbounded for read ops; no diff-size cap (current tools enforce GITHUB_BODY_MAX_CHARS = 4000).
• prompt-engineer: 3 blockers — (1) github-mcp-server default descriptions won't route correctly across Addie's 20+ tool catalog; all 8 need Addie-specific overrides with "when to use this" framing; (2) get_pull_request_diff vs get_pull_request_files vs get_pull_request_comments have no disambiguation rule — "review this PR" will over-call or under-call; (3) the output-mapper (per-field wrapUntrusted) is load-bearing and cannot be deferred — its absence is the biggest safety regression.

My take: Approach is sound; the safety scaffold isn't yet defined. Both experts converge on the output-mapper as non-negotiable in this PR. @bokelley — two decisions before a draft opens: (a) should wrapUntrusted in member-tools.ts be consolidated with wrapUntrustedInput from untrusted-input.ts first, or can the mapper ship using the local variant? (b) should the org allowlist be retained for diff/file tools (higher injection risk) while removed only for issue-read tools?

---

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

---

Generated by Claude Code

[bokelley]

Triage

Classification: Feature request
Bucket(s): addie, security-sensitive (label not found in repo — noted in run summary)
Status: ready-for-human

What the experts said:

• prompt-engineer: Two blockers — the wrapUntrusted() intercept point is undefined for a mounted MCP server (structured fields from github-mcp-server aren't covered by string-level wrapping applied after deserialization); and removing GITHUB_READ_ALLOWED_ORGS leaves no stated replacement for org/repo scoping at tool-call time. Also flags get_pull_request_diff as a significant capability step-up that should be tied to a concrete Addie user flow before inclusion.
• security-reviewer: Two blockers — the github-mcp-server process must be configured with per-user WorkOS Pipes tokens exclusively, not the shared GITHUB_TOKEN fallback (which would create a privilege escalation path); and the <untrusted-github-content> tag semantics need a standing system-prompt declaration rather than relying on injection at tool-call time.

My take: The direction is sound, but two implementation decisions need to be documented before a PR is meaningful: (1) the exact MCP adapter intercept point where all structured output fields are wrapped, and (2) what replaces org allowlist scoping at tool-call time. @bokelley — could you document both of those here so the implementation has a clear target?

---

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

---

Generated by Claude Code

[bokelley]

Triage

Classification: Feature request
Bucket(s): addie, security-sensitive (neither label exists in repo — noted in run summary)
Status: ready-for-human

What the experts said:

• security-reviewer: Two blockers — (1) github-mcp-server tool results bypass wrapUntrusted() entirely, and a pre-existing tag-name inconsistency (untrusted-github-content vs the underscore variant in the CI guard) means injection coverage has a gap today; (2) the proposed user OAuth token (reused from create_github_issue) may carry repo scope, making private repo diffs reachable in Addie's LLM context.
• prompt-engineer: Two additional blockers — upstream tool descriptions are generic and give Addie no principled basis for routing "review this PR" among three overlapping tools; get_pull_request_diff has no truncation and the existing GITHUB_BODY_MAX_CHARS cap lives in the hand-rolled path that github-mcp-server bypasses.

My take: Direction is well-scoped and the implementation notes in the issue are solid. Three blockers are implementation gates (output wrapper, description routing layer, diff truncation — no design decision required). One requires a call from you.

Auth scope — design choice:

Current read tools use process.env.GITHUB_TOKEN (server bot, public repos only). The proposal switches to the per-user WorkOS Pipes OAuth token. That token may carry repo scope (write-level), letting get_pull_request_diff pull private repo diffs into Addie's context.

Option A — Keep bot token for read tools:

- const token = await getGitHubAccessToken(workosUserId); // write-scoped OAuth
+ const token = process.env.GITHUB_TOKEN;                 // bot token, public only

No private repo access. Fork PRs from private forks remain unreachable (same as today).

Option B — User token with a read-only scope gate:

+ // Require a separate read-scoped connection distinct from the write-path token
+ const token = await getReadScopedGitHubToken(workosUserId); // public_repo read only

Enables user-permissioned reads (private forks accessible). Requires WorkOS Pipes to surface a separate, downscoped connection — can't reuse the write-path token as-is.

@bokelley, your call: A (simpler, public-only) or B (user-permissioned reads, requires Pipes work)?

---

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

---

Generated by Claude Code