diff --git a/.claude/memory/MEMORY.md b/.claude/memory/MEMORY.md index 1f8b851..ee93d40 100644 --- a/.claude/memory/MEMORY.md +++ b/.claude/memory/MEMORY.md @@ -7,12 +7,12 @@ Keep this file under 200 lines — anything longer is content bloat, not memory. - [learnings/sentry-mcp-no-comment-tool](learnings/2026-05-26-sentry-mcp-no-comment-tool.md) — Sentry MCP can't comment on issues; back-link from Linear only - [learnings/vercel-blocks-unknown-author-email](learnings/2026-05-26-vercel-blocks-unknown-author-email.md) — Vercel preview deploys block when commit author email has no GitHub account; use the noreply alias - [learnings/sandbox-cant-clone-private-repo](learnings/2026-05-26-sandbox-cant-clone-private-repo.md) — Don't `git clone` from sandbox bash; the `github_repository` resource is auth'd, raw `git clone` is not -- [learnings/github-mcp-strips-html-comments](learnings/2026-05-26-github-mcp-strips-html-comments.md) — `update_pull_request` silently strips `` from PR bodies; session-id marker can't be set by agent (ENG-25) +- [learnings/github-mcp-strips-html-comments](learnings/2026-05-26-github-mcp-strips-html-comments.md) — both `update_pull_request` AND `create_pull_request` strip `` from PR bodies; routed around in ENG-25 by switching to plain-text marker ## Decisions - [decisions/mcp-for-small-writes-checkout-for-big](decisions/2026-05-26-mcp-for-small-writes-checkout-for-big.md) — Single-file writes go through GitHub MCP; multi-file or test-needing changes use the mounted checkout + `git push` - [decisions/two-agent-builder-reviewer](decisions/2026-05-25-two-agent-builder-reviewer.md) — Separate agents for build vs. review so the reviewer reads diffs cold ## Conventions -- [conventions/pr-session-id-marker](conventions/pr-session-id-marker.md) — PR body MUST end with `` (or legacy `sthr_...`) so webhooks can resume +- [conventions/pr-session-id-marker](conventions/pr-session-id-marker.md) — PR body MUST end with plain-text `session-id: sesn_...` line so webhooks can resume (HTML-comment shape was stripped by MCP — ENG-25) - [conventions/agent-review-marker](conventions/agent-review-marker.md) — Reviewer's verdict goes on the first line as `AGENT_REVIEW: APPROVED|REQUEST_CHANGES|ESCALATE — ` diff --git a/.claude/memory/conventions/pr-session-id-marker.md b/.claude/memory/conventions/pr-session-id-marker.md index 4bad662..cbeea62 100644 --- a/.claude/memory/conventions/pr-session-id-marker.md +++ b/.claude/memory/conventions/pr-session-id-marker.md @@ -1,28 +1,49 @@ # PR body session-id marker -Every PR opened by the manager MUST end with this HTML comment as the -**last line** of the PR body: +Every PR opened by the manager MUST end its body with a session-id +marker. The `/api/github-webhook` route extracts this when a webhook +fires for the PR (e.g. `issue_comment.created` with +`AGENT_REVIEW: APPROVED`) and resumes the original manager session — +full context, no re-explaining. + +## Shape (current) + +A plain-text line, on its own, at the end of the PR body: ``` - +session-id: sesn_xxxxxxxxxxxxxxxx ``` -The `/api/github-webhook` route extracts this marker when a webhook -fires for the PR (e.g. `issue_comment.created` with -`AGENT_REVIEW: APPROVED`). With it, the webhook resumes the original -manager session — full implementation context, no re-explaining. +Per ENG-25 the GitHub MCP `update_pull_request` tool strips +`` HTML comments from PR bodies before persisting, so the +HTML-comment shape that used to be the convention is unwritable from +agent sessions. A plain-text `session-id:` line passes through +unchanged and is what every agent-opened PR should now use. + +If your kickoff message or system-prompt example still references the +HTML-comment shape, follow this file instead — it is the canonical +convention. + +## Prefix -The webhook regex accepts both the legacy `sthr_` prefix and the -current `sesn_` prefix returned by `client.beta.sessions.create()`. -Use whichever prefix the kickoff `user.message` carries. +Use whichever prefix the kickoff `user.message` carries. The current +Anthropic SDK returns `sesn_...`; older sessions had `sthr_...`. The +webhook regex accepts both. + +## Legacy shape (matched but do not write) + +For back-compat the webhook regex also matches the old HTML-comment +shape: + +``` + +``` -Without it, the webhook falls back to creating a fresh session. The -fresh session loses all design rationale and re-derives everything. -Functional but wasteful. +This is matched only so that markers a human landed via the GitHub +web API on an older PR continue to resume. Do not emit it from a new +PR — MCP will strip it on save. -The kickoff `user.message` includes the actual session id. Substitute -it verbatim; don't paraphrase or omit. +## Without the marker -The regex in `app/api/github-webhook/route.ts` accepts either -`sesn_` (current SDK prefix) or `sthr_` (legacy) — both are valid. -Use whatever the kickoff hands you. +The webhook falls back to creating a fresh manager session. Functional +but wasteful — the fresh session re-derives all design context. diff --git a/.claude/memory/learnings/2026-05-26-github-mcp-strips-html-comments.md b/.claude/memory/learnings/2026-05-26-github-mcp-strips-html-comments.md index 5e8fde0..bd92317 100644 --- a/.claude/memory/learnings/2026-05-26-github-mcp-strips-html-comments.md +++ b/.claude/memory/learnings/2026-05-26-github-mcp-strips-html-comments.md @@ -1 +1 @@ -The GitHub MCP `update_pull_request` tool unconditionally strips `` HTML comments from the `body` field before persisting — the PATCH fires (other prose changes land, `updated_at` advances), but any HTML comment line silently disappears. Confirmed twice on PR #12 review-feedback round 2 with both `sesn_...` and `sthr_...` prefixes; the filter isn't gated on content. This makes the `pr-session-id-marker` convention unenforceable by the agent via the standard transport — there's no `GITHUB_TOKEN` in the sandbox env and `api.github.com` is outside the egress allowlist, so you can't route around MCP with a direct PATCH. Don't waste review rounds on it — see ENG-25 for the fix proposal and the open question of whether `create_pull_request` shares the same filter (worth opportunistic testing whenever you open a new PR). +Both GitHub MCP `update_pull_request` AND `create_pull_request` unconditionally strip `` HTML comments from the PR `body` field before persisting — even inside fenced code blocks. The PATCH/POST fires (other prose changes land, `updated_at` advances), but every HTML comment silently disappears. Confirmed three times: PR #12 review-feedback round 2 with both `sesn_...` and `sthr_...` prefixes (`update_pull_request`); PR #18 open with a sentinel id (`create_pull_request`). Filter is not gated on content or prefix. No transport workaround from sandbox — no `GITHUB_TOKEN`, `api.github.com` outside egress allowlist, local git proxy is git-only, no `gh`. Resolved by ENG-25 (PR #18): canonical session-id marker is now a plain-text `session-id: sesn_...` line, which passes through unchanged. Webhook regex still matches the legacy HTML-comment shape for back-compat with anything a human landed via the web API. diff --git a/app/api/github-webhook/route.ts b/app/api/github-webhook/route.ts index 15fc659..8243be9 100644 --- a/app/api/github-webhook/route.ts +++ b/app/api/github-webhook/route.ts @@ -17,8 +17,20 @@ function verifySignature(rawBody: string, sigHeader: string | null, secret: stri function extractSessionId(text: string | undefined | null): string | null { if (!text) return null; - const m = text.match(//); - return m?.[1] ?? null; + // Preferred shape (ENG-25): a plain-text line on its own. + // session-id: sesn_xxxxxxxx + // The GitHub MCP `update_pull_request` tool strips `` + // HTML comments from PR bodies, so the agent can only land a marker + // in a non-comment shape. + const plain = text.match( + /(?:^|\n)\s*session-id:\s*((?:sthr_|sesn_)[A-Za-z0-9]+)\s*(?=\n|$)/i, + ); + if (plain) return plain[1]; + // Back-compat: the original HTML-comment shape, still accepted so + // any marker landed via the GitHub web API or a human PATCH on an + // older PR continues to resume correctly. + const html = text.match(//); + return html?.[1] ?? null; } async function fetchPrBody(prNumber: number, token: string): Promise {