feat(api,cli): bot-owned GitHub attachments comment (phase 2 PR B)#290
Conversation
…fixture drift guard
… a rendered comment body gatherCommentBody lists the calling workspace's own R2 objects under the stable gh key prefix and its own galleries linked to the PR/issue coordinate, then renders the managed comment body via attachmentsCommentBody (Task 1). Takes the workspace record and workspace slug as separate parameters, since WorkspaceRecord has no name field and routes hold the slug separately as workspaceName.
… comment
Workspace-authed route (files:read) that gathers the calling workspace's
own attachments/galleries for a PR or issue and upserts the managed bot
comment via the GitHub App install. Never 5xxs on an integration failure
(bot problems -> 200 { posted:false, reason }); only malformed requests
use the standard AppError envelope.
…n comment types syncAttachmentsComment now returns a via: "bot" | "gh" field, but the screenshot command's human-mode print and --comment help text weren't updated to match the three commands.ts call sites. Also adds the missing via field to two stale local type annotations (screenshot.ts, mcp/tools.ts) that compiled fine but no longer matched the real return shape.
- parallelize gatherCommentBody's independent attachment + gallery reads; hydrate each page's galleries concurrently (parity with the CLI gather) - collapse upsertBotComment's duplicated create/patch branch - extract a shared route jsonBody helper (dedupes galleries.ts) and align the comment route's repo regex with public-files.ts - inline syncAttachmentsComment's single-use gh-path closure; export a shared AttachmentsCommentResult type + commentViaSuffix helper used across the four call sites; type the client's decline reason as a literal union
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub App-backed managed attachment and gallery comments, with an authenticated API route, deterministic rendering, typed client support, and CLI fallback to local ChangesManaged GitHub comments
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
uploads-api | 2ccc48c | Commit Preview URL Branch Preview URL |
Jul 20 2026, 07:54 PM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
uploads-auth | fb8c51c | Commit Preview URL Branch Preview URL |
Jul 20 2026, 07:46 PM |
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
apps/api/src/routes/github-comment-route.test.ts (1)
56-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a happy-path route test.
Coverage here is limited to
not_installedand a malformed-body 400. There's no test exercising the full route wiring end-to-end (installed app → gather →upsertBotComment→{posted:true, action, count, commentUrl}response), which would catch integration issues betweenparseTarget,gatherCommentBody, andupsertBotCommentthat per-unit tests ingithub-comment.tscan't surface. ThefakeFetchpattern already used ingithub-comment.test.tscould be reused here (the route callsupsertBotCommentwith the default globalfetch, sovi.spyOn(globalThis, "fetch")would work).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/github-comment-route.test.ts` around lines 56 - 69, The route tests need a happy-path end-to-end case covering an installed app through parseTarget, gatherCommentBody, and upsertBotComment. Extend the POST /v1/:workspace/github/comment suite using the existing fakeFetch setup or a global fetch spy, then assert the successful response includes posted: true, action, count, and commentUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/github-comment.ts`:
- Around line 143-156: Update the existing-marker lookup around listRes and
existing to paginate through all GitHub issue comments, following the
cursor-based approach used by gatherAttachments and gatherGalleries. Continue
requesting pages until the attachments marker is found or no further comments
remain, while preserving the existing 403/unavailable handling and reuse of the
discovered comment for upserts.
- Around line 138-139: Update the comment lookup flow around installationToken
and the issue-comment retrieval logic to paginate through all comment pages, not
just the first 100 results, before deciding whether to create or update a
managed comment. Preserve the existing managed-comment matching and upsert
behavior once all pages have been searched.
In `@apps/api/src/index.ts`:
- Around line 127-129: Update the GitHub comment route registration in the API
router to include the existing writeRateLimit middleware alongside the
workspaceAuth guard before githubComment. Keep the route path and handler
unchanged, and reuse the established write-rate-limit symbol used by other
mutating routes.
In `@apps/api/src/routes/github-comment.ts`:
- Around line 37-64: Update the githubComment route middleware chain to include
writeRateLimit keyed by workspace alongside requireScope("files:read"). Reuse
the existing writeRateLimit middleware and preserve the handler’s current
behavior.
- Around line 25-31: Update the num parsing and validation in the request
validation flow to accept only values whose original body.num type is number;
remove coercion via Number(body.num), while preserving the existing safe-integer
and positive-value checks.
- Line 18: Update REPO_RE validation to reject dot-only owner or repository
segments such as “.” and “..”, preventing path traversal values before GitHub
URL construction. Preserve acceptance of valid alphanumeric, underscore, hyphen,
and embedded-dot repository names, and ensure the validated segments are safely
encoded or otherwise cannot alter the intended API path.
---
Nitpick comments:
In `@apps/api/src/routes/github-comment-route.test.ts`:
- Around line 56-69: The route tests need a happy-path end-to-end case covering
an installed app through parseTarget, gatherCommentBody, and upsertBotComment.
Extend the POST /v1/:workspace/github/comment suite using the existing fakeFetch
setup or a global fetch spy, then assert the successful response includes
posted: true, action, count, and commentUrl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 737f952f-91e5-4cce-a8ec-5787b7b4edc9
📒 Files selected for processing (19)
.changeset/github-bot-comment.mdapps/api/src/github-comment-render.test.tsapps/api/src/github-comment-render.tsapps/api/src/github-comment.test.tsapps/api/src/github-comment.tsapps/api/src/index.tsapps/api/src/routes/galleries.tsapps/api/src/routes/github-comment-route.test.tsapps/api/src/routes/github-comment.tsapps/api/src/routes/json-body.tspackages/uploads/src/client.tspackages/uploads/src/commands.tspackages/uploads/src/commands/screenshot.tspackages/uploads/src/mcp/tools.tspackages/uploads/test/client-github-comment.test.tspackages/uploads/test/commands-comment.test.tspackages/uploads/test/github-render-golden.test.tspackages/uploads/test/mcp.test.tstest/fixtures/github-comment-golden.json
| const token = await installationToken(env, cfg, installationId, fetchImpl); | ||
| if (!token) return { degrade: "unavailable" }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' apps/api/src/github-comment.tsRepository: buildinternet/uploads
Length of output: 6960
🏁 Script executed:
sed -n '1,260p' apps/api/src/github-app.tsRepository: buildinternet/uploads
Length of output: 5253
🏁 Script executed:
rg -n "ATTACHMENTS_MARKER|issues/.*/comments|per_page=100|nextCursor|page=.*comments" apps/api/srcRepository: buildinternet/uploads
Length of output: 2604
🏁 Script executed:
sed -n '130,340p' apps/api/src/github-comment.test.tsRepository: buildinternet/uploads
Length of output: 6138
Search all comment pages before deciding to create
apps/api/src/github-comment.ts:145-156 only checks the first 100 issue comments, so an existing managed comment on a later page will be missed and every later upsert will create a duplicate instead of patching it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/github-comment.ts` around lines 138 - 139, Update the comment
lookup flow around installationToken and the issue-comment retrieval logic to
paginate through all comment pages, not just the first 100 results, before
deciding whether to create or update a managed comment. Preserve the existing
managed-comment matching and upsert behavior once all pages have been searched.
| let listRes: Response; | ||
| try { | ||
| listRes = await githubFetch(fetchImpl, `${base}/issues/${target.num}/comments?per_page=100`, { | ||
| headers: githubHeaders(token), | ||
| }); | ||
| } catch { | ||
| return { degrade: "unavailable" }; | ||
| } | ||
| if (listRes.status === 403) return { degrade: "forbidden" }; | ||
| if (!listRes.ok) return { degrade: "unavailable" }; | ||
| const comments = (await listRes.json().catch(() => [])) as GhComment[]; | ||
| const existing = Array.isArray(comments) | ||
| ? comments.find((c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER)) | ||
| : undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Existing-marker lookup only checks the first 100 comments — no pagination.
per_page=100 is fetched once with no loop, unlike gatherAttachments/gatherGalleries in this same file which correctly page via cursors. If an issue/PR already has more than 100 comments before the bot's marker was created (or would fall past the first page), existing will never be found, and every future upsert creates a new duplicate marker comment instead of updating the real one.
Separately (lower priority, worth confirming): there's a list-then-write TOCTOU window here — two concurrent calls for the same target could both see no existing comment and both POST, creating two marker comments. Might be an accepted tradeoff given typical usage, but worth confirming intentionally.
🐛 Proposed fix (pagination)
- let listRes: Response;
- try {
- listRes = await githubFetch(fetchImpl, `${base}/issues/${target.num}/comments?per_page=100`, {
- headers: githubHeaders(token),
- });
- } catch {
- return { degrade: "unavailable" };
- }
- if (listRes.status === 403) return { degrade: "forbidden" };
- if (!listRes.ok) return { degrade: "unavailable" };
- const comments = (await listRes.json().catch(() => [])) as GhComment[];
- const existing = Array.isArray(comments)
- ? comments.find((c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER))
- : undefined;
+ let existing: GhComment | undefined;
+ for (let page = 1; ; page++) {
+ let listRes: Response;
+ try {
+ listRes = await githubFetch(
+ fetchImpl,
+ `${base}/issues/${target.num}/comments?per_page=100&page=${page}`,
+ { headers: githubHeaders(token) },
+ );
+ } catch {
+ return { degrade: "unavailable" };
+ }
+ if (listRes.status === 403) return { degrade: "forbidden" };
+ if (!listRes.ok) return { degrade: "unavailable" };
+ const comments = (await listRes.json().catch(() => [])) as GhComment[];
+ if (!Array.isArray(comments) || comments.length === 0) break;
+ existing = comments.find((c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER));
+ if (existing || comments.length < 100) break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let listRes: Response; | |
| try { | |
| listRes = await githubFetch(fetchImpl, `${base}/issues/${target.num}/comments?per_page=100`, { | |
| headers: githubHeaders(token), | |
| }); | |
| } catch { | |
| return { degrade: "unavailable" }; | |
| } | |
| if (listRes.status === 403) return { degrade: "forbidden" }; | |
| if (!listRes.ok) return { degrade: "unavailable" }; | |
| const comments = (await listRes.json().catch(() => [])) as GhComment[]; | |
| const existing = Array.isArray(comments) | |
| ? comments.find((c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER)) | |
| : undefined; | |
| let existing: GhComment | undefined; | |
| for (let page = 1; ; page++) { | |
| let listRes: Response; | |
| try { | |
| listRes = await githubFetch( | |
| fetchImpl, | |
| `${base}/issues/${target.num}/comments?per_page=100&page=${page}`, | |
| { headers: githubHeaders(token) }, | |
| ); | |
| } catch { | |
| return { degrade: "unavailable" }; | |
| } | |
| if (listRes.status === 403) return { degrade: "forbidden" }; | |
| if (!listRes.ok) return { degrade: "unavailable" }; | |
| const comments = (await listRes.json().catch(() => [])) as GhComment[]; | |
| if (!Array.isArray(comments) || comments.length === 0) break; | |
| existing = comments.find((c) => typeof c.body === "string" && c.body.includes(ATTACHMENTS_MARKER)); | |
| if (existing || comments.length < 100) break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/github-comment.ts` around lines 143 - 156, Update the
existing-marker lookup around listRes and existing to paginate through all
GitHub issue comments, following the cursor-based approach used by
gatherAttachments and gatherGalleries. Continue requesting pages until the
attachments marker is found or no further comments remain, while preserving the
existing 403/unavailable handling and reuse of the discovered comment for
upserts.
| // Bot-owned managed comment (phase 2 PR B). Workspace-authed (unlike the | ||
| // HMAC-public /v1/github/webhook above) — behind the workspaceAuth guard. | ||
| .route("/v1/:workspace/github", githubComment) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/api/src/index.ts outline =="
ast-grep outline apps/api/src/index.ts --view expanded || true
echo
echo "== apps/api/src/index.ts relevant slice =="
nl -ba apps/api/src/index.ts | sed -n '100,150p'
echo
echo "== locate githubComment definition/usages =="
rg -n "githubComment|writeRateLimit|requireScope\\(\"files:read\"\\)" apps/api/src -SRepository: buildinternet/uploads
Length of output: 400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/api/src/routes search =="
fd -a "github-comment.ts|githubComment|writeRateLimit" apps/api/src || true
echo
echo "== likely route file outline candidates =="
for f in $(fd -a -t f "github.*\.ts$|.*comment.*\.ts$" apps/api/src 2>/dev/null | head -20); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded || true
doneRepository: buildinternet/uploads
Length of output: 6870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/api/src/index.ts relevant slice =="
nl -ba apps/api/src/index.ts | sed -n '115,140p'
echo
echo "== writeRateLimit and githubComment definitions/usages =="
rg -n "writeRateLimit|githubComment|requireScope\\(\"files:read\"\\)" apps/api/src -SRepository: buildinternet/uploads
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/api/src/index.ts relevant slice =="
sed -n '115,140p' apps/api/src/index.ts | cat -n
echo
echo "== apps/api/src/routes/github-comment.ts relevant slice =="
sed -n '1,120p' apps/api/src/routes/github-comment.ts | cat -n
echo
echo "== writeRateLimit locations in routes =="
rg -n "writeRateLimit" apps/api/src/routes apps/api/src/index.ts -SRepository: buildinternet/uploads
Length of output: 6387
Add writeRateLimit to the GitHub comment route. apps/api/src/routes/github-comment.ts exposes a mutating /comment endpoint and calls GitHub with the shared App installation token, but it only uses requireScope("files:read"). This should be rate-limited like the other write paths so one workspace can’t drain the shared GitHub API budget for everyone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/index.ts` around lines 127 - 129, Update the GitHub comment
route registration in the API router to include the existing writeRateLimit
middleware alongside the workspaceAuth guard before githubComment. Keep the
route path and handler unchanged, and reuse the established write-rate-limit
symbol used by other mutating routes.
Source: Coding guidelines
|
|
||
| // Same owner/name grammar as public-files.ts's deriveGithubContext, so a repo | ||
| // string validates identically across the API. | ||
| const REPO_RE = /^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how target.repo/owner+name are used to build outbound GitHub API requests.
fd -a 'github-app.ts' apps/api/src
fd -a 'github-comment.ts' apps/api/src
rg -n -B2 -A10 'function installationForRepo' apps/api/src/github-app.ts
rg -n -B2 -A15 'function upsertBotComment' apps/api/src/github-comment.ts
rg -n "api\.github\.com|repos/\$\{|fetch\(" apps/api/src/github-app.ts apps/api/src/github-comment.tsRepository: buildinternet/uploads
Length of output: 1929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' apps/api/src/routes/github-comment.ts
printf '\n--- github-comment.ts ---\n'
sed -n '1,240p' apps/api/src/github-comment.ts
printf '\n--- github-app.ts ---\n'
sed -n '1,220p' apps/api/src/github-app.tsRepository: buildinternet/uploads
Length of output: 14705
🏁 Script executed:
node - <<'JS'
const urls = [
'https://api.github.com/repos/owner/name/installation',
'https://api.github.com/repos/../../installation',
'https://api.github.com/repos/owner/../issues/1/comments',
'https://api.github.com/repos/././issues/1/comments',
];
for (const u of urls) {
const parsed = new URL(u);
console.log(u, '=>', parsed.href);
}
JSRepository: buildinternet/uploads
Length of output: 562
Reject dot-only repo segments before building GitHub URLs
REPO_RE still admits ../.., owner/.., and similar values. Those strings are interpolated directly into https://api.github.com/repos/${repo}/..., so .. segments collapse to different GitHub API paths under the installation token. Split owner/name and encode each segment, or reject dot-only segments here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/github-comment.ts` at line 18, Update REPO_RE validation
to reject dot-only owner or repository segments such as “.” and “..”, preventing
path traversal values before GitHub URL construction. Preserve acceptance of
valid alphanumeric, underscore, hyphen, and embedded-dot repository names, and
ensure the validated segments are safely encoded or otherwise cannot alter the
intended API path.
| const repo = typeof body.repo === "string" ? body.repo : ""; | ||
| const num = typeof body.num === "number" ? body.num : Number(body.num); | ||
| const kind = body.kind; | ||
| if (!REPO_RE.test(repo)) | ||
| throw new ValidationError("repo must be owner/name.", { code: "invalid_repo" }); | ||
| if (!Number.isSafeInteger(num) || num < 1) | ||
| throw new ValidationError("num must be a positive integer."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten num validation to reject type-confused inputs.
Number(body.num) coerces non-number JSON values, e.g. num: true → 1, num: [5] → 5, both of which then pass Number.isSafeInteger and are accepted as valid targets even though they aren't numbers.
🛡️ Proposed fix
- const num = typeof body.num === "number" ? body.num : Number(body.num);
+ const num = typeof body.num === "number" ? body.num : NaN;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const repo = typeof body.repo === "string" ? body.repo : ""; | |
| const num = typeof body.num === "number" ? body.num : Number(body.num); | |
| const kind = body.kind; | |
| if (!REPO_RE.test(repo)) | |
| throw new ValidationError("repo must be owner/name.", { code: "invalid_repo" }); | |
| if (!Number.isSafeInteger(num) || num < 1) | |
| throw new ValidationError("num must be a positive integer."); | |
| const repo = typeof body.repo === "string" ? body.repo : ""; | |
| const num = typeof body.num === "number" ? body.num : NaN; | |
| const kind = body.kind; | |
| if (!REPO_RE.test(repo)) | |
| throw new ValidationError("repo must be owner/name.", { code: "invalid_repo" }); | |
| if (!Number.isSafeInteger(num) || num < 1) | |
| throw new ValidationError("num must be a positive integer."); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/github-comment.ts` around lines 25 - 31, Update the num
parsing and validation in the request validation flow to accept only values
whose original body.num type is number; remove coercion via Number(body.num),
while preserving the existing safe-integer and positive-value checks.
| export const githubComment = new Hono<WorkspaceVars>().post( | ||
| "/comment", | ||
| requireScope("files:read"), | ||
| async (c) => { | ||
| const target = parseTarget(await jsonBody(c)); | ||
| const cfg = githubAppConfig(c.env); | ||
| if (!cfg) return c.json({ posted: false, reason: "app_unconfigured" }); | ||
| const installId = await installationForRepo(c.env, cfg, target.repo); | ||
| if (installId === null) return c.json({ posted: false, reason: "not_installed" }); | ||
|
|
||
| const gathered = await gatherCommentBody( | ||
| c.env, | ||
| c.get("workspace"), | ||
| c.get("workspaceName"), | ||
| target, | ||
| ); | ||
| if (gathered.skip) return c.json({ posted: true, action: "skipped", count: 0 }); | ||
|
|
||
| const result = await upsertBotComment(c.env, cfg, installId, target, gathered.body); | ||
| if ("degrade" in result) return c.json({ posted: false, reason: result.degrade }); | ||
| return c.json({ | ||
| posted: true, | ||
| action: result.action, | ||
| count: gathered.count, | ||
| commentUrl: result.commentUrl, | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add writeRateLimit middleware — this route performs an external mutation without it.
This handler creates/updates a GitHub comment via the app's installation token (a real side-effecting write), but only requireScope("files:read") is wired in; there's no writeRateLimit. As per coding guidelines, "Mutating routes must use the writeRateLimit middleware keyed by workspace; the middleware may no-op only when the WRITE_LIMITER binding is absent." Unbounded repeated calls here can hammer the GitHub API on behalf of the installation token.
🔒️ Proposed fix
export const githubComment = new Hono<WorkspaceVars>().post(
"/comment",
requireScope("files:read"),
+ writeRateLimit,
async (c) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const githubComment = new Hono<WorkspaceVars>().post( | |
| "/comment", | |
| requireScope("files:read"), | |
| async (c) => { | |
| const target = parseTarget(await jsonBody(c)); | |
| const cfg = githubAppConfig(c.env); | |
| if (!cfg) return c.json({ posted: false, reason: "app_unconfigured" }); | |
| const installId = await installationForRepo(c.env, cfg, target.repo); | |
| if (installId === null) return c.json({ posted: false, reason: "not_installed" }); | |
| const gathered = await gatherCommentBody( | |
| c.env, | |
| c.get("workspace"), | |
| c.get("workspaceName"), | |
| target, | |
| ); | |
| if (gathered.skip) return c.json({ posted: true, action: "skipped", count: 0 }); | |
| const result = await upsertBotComment(c.env, cfg, installId, target, gathered.body); | |
| if ("degrade" in result) return c.json({ posted: false, reason: result.degrade }); | |
| return c.json({ | |
| posted: true, | |
| action: result.action, | |
| count: gathered.count, | |
| commentUrl: result.commentUrl, | |
| }); | |
| }, | |
| ); | |
| export const githubComment = new Hono<WorkspaceVars>().post( | |
| "/comment", | |
| requireScope("files:read"), | |
| writeRateLimit, | |
| async (c) => { | |
| const target = parseTarget(await jsonBody(c)); | |
| const cfg = githubAppConfig(c.env); | |
| if (!cfg) return c.json({ posted: false, reason: "app_unconfigured" }); | |
| const installId = await installationForRepo(c.env, cfg, target.repo); | |
| if (installId === null) return c.json({ posted: false, reason: "not_installed" }); | |
| const gathered = await gatherCommentBody( | |
| c.env, | |
| c.get("workspace"), | |
| c.get("workspaceName"), | |
| target, | |
| ); | |
| if (gathered.skip) return c.json({ posted: true, action: "skipped", count: 0 }); | |
| const result = await upsertBotComment(c.env, cfg, installId, target, gathered.body); | |
| if ("degrade" in result) return c.json({ posted: false, reason: result.degrade }); | |
| return c.json({ | |
| posted: true, | |
| action: result.action, | |
| count: gathered.count, | |
| commentUrl: result.commentUrl, | |
| }); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/github-comment.ts` around lines 37 - 64, Update the
githubComment route middleware chain to include writeRateLimit keyed by
workspace alongside requireScope("files:read"). Reuse the existing
writeRateLimit middleware and preserve the handler’s current behavior.
Source: Coding guidelines
Cache the managed comment's id in GITHUB_CACHE keyed by repo#num so re-editing attachment media is a single PATCH with no listing. The id is an optimization only — a 404 (comment deleted) drops it and falls back to the marker hunt, which now pages through the whole thread instead of just the first 100 comments. The CLI gh fallback gains --paginate for the same reason, so both paths edit in place on busy PRs rather than posting a duplicate.
…oute
Address review of the bot-comment endpoint:
- apply writeRateLimit (per-workspace) since it drives an outbound GitHub
write, matching the file/gallery mutating routes
- reject dot-only repo segments (".", "..") — the repo string is
interpolated into a server-side api.github.com path where "../" traverses
- num accepts only a JSON number (drop Number() coercion)
- add a happy-path e2e route test through parseTarget -> gatherCommentBody
-> upsertBotComment asserting { posted, action, count, commentUrl }
The paginated marker lookup the review also flagged already landed in the
prior commit.
📎 AttachmentsMaintained by uploads.sh — re-uploading a file with the same name updates it everywhere it is embedded. |
* fix(uploads): warn on stderr when linked dev CLI's dist/ is stale The globally-linked uploads binary imports packages/uploads/dist/cli.js, which only gets rebuilt by an explicit build step. After pulling new source, the linked CLI can keep running old compiled code while --version still reports the current package version, misleading local debugging (see #295). Compares src/ vs dist/ mtimes at startup and prints a one-line warning without blocking execution. The check is a no-op for published npm installs: no src/ tree ships in the tarball, so the first existsSync check short-circuits. * docs(uploads): update stale "via local gh auth" copy in MCP tool descriptions The put --comment and comment MCP tool descriptions still described posting via local gh auth as the primary mechanism, left over from before the bot-comment path shipped in #290. Match the wording the comment command's own help already uses: server-side bot comment first (uploads-sh[bot]), local gh fallback second. Fixes #294.
1 participant
GitHub App phase 2, PR B — bot-owned attachments comment
Closes the second half of #284 (phase 2). PR A (#288) added webhooks + cache invalidation; this PR adds the bot-owned managed comment.
When the "uploads.sh" GitHub App is installed on the target repo, the managed PR/issue attachments comment is now posted server-side as
uploads-sh[bot]via an installation token. Uploaders no longer need a locally authenticatedghfor--comment/uploads comment. Where the App isn't installed, the CLI falls back to its existing local-ghpath, unchanged.How it works
POST /v1/:workspace/github/comment(workspace-authed) gathers the calling workspace's own attachments (gh/…key prefix) + galleries for the coordinate, renders the marker comment body server-side, and — when the App is installed with write — upserts it as the bot. Returns{ posted: true, action, count, commentUrl }or{ posted: false, reason }.posted: trueit's done, otherwise (not installed /403/ self-hosted 404 / any error) it runs its existing gather → render →ghupsert. Human output distinguishes bot vsgh.GITHUB_CACHEkeyed byrepo#num, so re-editing attachment media is a singlePATCHwith no listing. The id is an optimization only — a404(comment deleted) drops it and falls back to the marker hunt, which now pages through the whole thread (a busy PR can push the comment past the first 100). Theghfallback gains--paginatefor the same reason, so both paths edit the one comment in place on 100+ comment threads instead of posting a second.Before → after
Before — every uploader needs an authenticated local
gh; the comment is authored by whoever ran the command:flowchart LR U["uploads comment --pr N"] --> CLI["CLI"] CLI --> G["gather attachments + galleries"] G --> R["render comment body"] R --> GH{"local gh<br/>authenticated?"} GH -- yes --> POST["gh upserts comment<br/>(authored by the user)"] GH -- no --> FAIL["command fails —<br/>no comment posted"]After — the server posts as
uploads-sh[bot]when the App is installed; the local-ghpath stays as the fallback, so nothing regresses:flowchart TB U["uploads comment --pr N"] --> CLI["CLI"] CLI --> EP["POST /v1/:workspace/github/comment<br/>(workspace-authed, sends only repo + num + kind)"] EP --> SRV["server gathers the workspace's<br/>OWN attachments + galleries"] SRV --> SR["server renders comment body"] SR --> INST{"App installed<br/>with write?"} INST -- yes --> CACHE{"cached comment id?<br/>(GITHUB_CACHE: repo#num)"} CACHE -- hit --> PATCH["PATCH that comment directly<br/>(one call, no listing)"] CACHE -- "miss / 404 deleted" --> HUNT["hunt marker comment,<br/>paginated → PATCH or POST,<br/>cache the id"] PATCH --> DONE(["posted: true — edited in place"]) HUNT --> DONE INST -- "no / 403 / 404 / error" --> DECL["posted: false, reason"] DECL --> FB["CLI falls back to existing gather →<br/>render → gh upsert (paginated marker hunt)"] FB --> DONE2(["posted via gh<br/>(unchanged path)"])Design decisions
{ repo, num, kind }, never a caller-supplied body. This is the trust boundary: since the App is installed org-wide, a client-supplied body would let any workspace token make the bot post arbitrary markdown to any org PR. Rendering strictly from the workspace's own uploads prevents that.@uploads/*package, so the renderer is a deliberate copy in the api worker. A shared golden fixture (test/fixtures/github-comment-golden.json) is asserted byte-for-byte from both sides, so the copies can't drift.ghpath stays fully intact as the deepest fallback — self-hosters (App not configured, or an older worker without this route) keep working exactly as before.403(write not yet approved) falls through togh, so this is safe to merge before the live-App permission upgrade.Deploy step (out-of-band, after merge)
The bot path stays dormant until the App has write access:
buildinternetorg install.uploads comment --pr <n>from an environment withoutghauth against a repo where the App is installed; the comment should appear asuploads-sh[bot].No new secrets/bindings — the added permissions travel with the existing installation token.
Testing
github-comment.tsunits (gather scoping + cross-workspace isolation, create/patch, 403 → forbidden, token/network → unavailable, empty → skipped); route tests (auth, validation,not_installed).syncAttachmentsCommentprefers bot onposted: true(noghcall), falls back onposted: falseand on a thrown endpoint call; client method request shape.jsonBodyroute helper, sharedAttachmentsCommentResulttype +viasuffix helper, tightened result/reason types).Notes
@buildinternet/uploads(CLI). api/web-only changes remain changeset-ignored.issue_commentwebhook events (drift reconciliation); no new event subscription until that handler lands.