From 9034c8e1ff9ae2431e4bf327c585b8855ada6f39 Mon Sep 17 00:00:00 2001 From: Self-Managing Codebase Manager <7004983+WillTaylor22@users.noreply.github.com> Date: Tue, 26 May 2026 10:04:16 +0000 Subject: [PATCH 1/2] ENG-25: plain-text session-id marker (HTML-comment shape stripped by MCP) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub MCP `update_pull_request` tool unconditionally strips `` HTML comments from PR bodies before persisting, so agent sessions cannot land the marker required by `conventions/pr-session-id-marker.md`. Every review-feedback round on an agent PR currently burns a round on the un-landable marker (PR #12 hit the 3-round ceiling on exactly this). Fix: change the canonical marker shape from an HTML comment to a plain-text line that MCP passes through unchanged. The webhook regex now matches both: - new: `session-id: sesn_xxxxxxxx` (on its own line) - legacy: `` (back-compat) So any old PR whose marker was landed by a human via the web API still resumes, but every new agent-opened PR can use the plain-text shape and the marker will actually persist. Files: - app/api/github-webhook/route.ts `extractSessionId` — tries plain-text shape first, falls back to the HTML-comment shape. The plain-text regex requires the marker to be on its own line (anchored with `(?:^|\n)` and `(?=\n|$)`) so it doesn't false-match prose like 'I noticed session-id: sesn_xxx is in this PR'. Validated against 10 cases including that negative. - .claude/memory/conventions/pr-session-id-marker.md — rewritten so the plain-text shape is canonical; legacy HTML-comment shape kept as 'matched but do not write'. - .claude/memory/MEMORY.md — one-line hook updated to reflect the new canonical shape and cross-ref ENG-25. Verification: - `npm run lint` — clean. - `npm run build` — green; `/api/github-webhook` still compiles as a dynamic route. - `extractSessionId` regression-tested in node against 10 cases (plain-text on its own line; trailing newline; trailing whitespace; case-insensitive label; legacy sthr_; HTML-comment back-compat; inline prose must NOT match; no marker; malformed prefix). All pass. Out of scope: - Underlying MCP defect (the strip filter itself) is not fixed — the learning at `learnings/github-mcp-strips-html-comments.md` stays accurate. We just route around it. - ENG-21/22/23/24 (other 'Credentials needed' gaps) are independent. --- .claude/memory/MEMORY.md | 2 +- .../conventions/pr-session-id-marker.md | 57 +++++++++++++------ app/api/github-webhook/route.ts | 16 +++++- 3 files changed, 54 insertions(+), 21 deletions(-) diff --git a/.claude/memory/MEMORY.md b/.claude/memory/MEMORY.md index 1f8b851..d9aba96 100644 --- a/.claude/memory/MEMORY.md +++ b/.claude/memory/MEMORY.md @@ -14,5 +14,5 @@ Keep this file under 200 lines — anything longer is content bloat, not memory. - [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/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 { From 079c949b47def856629a98bb27b65aaa4faef5cf Mon Sep 17 00:00:00 2001 From: Self-Managing Codebase Manager <7004983+WillTaylor22@users.noreply.github.com> Date: Tue, 26 May 2026 10:05:49 +0000 Subject: [PATCH 2/2] memory: confirm create_pull_request shares the HTML-comment strip filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probed on PR #18 (this PR): the body posted via create_pull_request included a sentinel marker AND literal examples inside fenced code blocks in the prose. After the PR opened, the body returned by the GitHub API has ALL of them stripped — not just the trailing marker. So create_pull_request shares the same body-strip filter as update_pull_request. ENG-25's 'Workaround' section flagged this as an open question worth testing on a new PR; the test was just run. Updates the learning entry to note the new datapoint and that the plain-text shape (this PR) is the resolution path. The webhook-regex back-compat branch for the HTML-comment shape stays — it only fires for markers a human landed via the web API on older PRs. --- .claude/memory/MEMORY.md | 2 +- .../learnings/2026-05-26-github-mcp-strips-html-comments.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude/memory/MEMORY.md b/.claude/memory/MEMORY.md index d9aba96..ee93d40 100644 --- a/.claude/memory/MEMORY.md +++ b/.claude/memory/MEMORY.md @@ -7,7 +7,7 @@ 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` 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.