feat: implement the review wrapper action #4
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/wrapper"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Composes the eight wrapper modules into a working action:
src/main.tsbuilds the forge client from the run's credentials, derives the PR inputs from the event payload, and hands the orchestrator its wait protocol, clock, and three reconcilers. The committeddist/index.mjsis rebuilt from those sources.The review session is passed as a thunk, not a value. That is the fork gate in code —
reviewCredentials()runs only when the orchestrator opens a session, which the fork path never does, soREVIEW_CAPABILITY_TOKENis never read on a fork pull request. The static isolation test and the importer allowlist still hold withsrc/main.tsas the sole importer.Two composition-surfaced changes
The spec's entry uses top-level await, which the shared oxlint config rejects (
node/no-top-level-await). Every promise-chain alternative is rejected too —unicorn/prefer-top-level-awaitandpromise/prefer-await-to-thenfire on.then, onvoid run(), and on an async IIFE. The two rules cannot both be satisfied, so the entry keeps the spec's shape behind a scoped/* eslint-disable node/no-top-level-await */. This is a bundled ESM action node runs directly, never throughrequire(esm), so the rule's stated hazard does not apply here. The durable fix is in@j4k/oxlint-config: the node preset should not enable a rule that a sibling preset's rule contradicts.The bundle now pulls in dependency code carrying trailing whitespace, which the pre-commit
git diff --checkrejects. A one-line.gitattributesmarksdist/index.mjs-whitespace. No source formatting was touched.Review-driven fixes
Two rounds of review feedback are folded in.
7abe25eclosed reconciliation gaps:listReviewsfails closed on an unusableX-Total-Countinstead of truncating to page 1, dispositions project every claim marker in a conversation rather than the first, and a projection marker carries the disposition record's identity so afixed → reopened → fixedsequence is not suppressed by the earlier marker.b9c4ce2made the committed bundle byte-identical across machines.f7d380fclosed the second round:head.repois nullable end to end (a deleted source fork classifies as a fork instead of throwing before the gate) andworkflow_dispatchparsespr_numberproperly; snippet anchors are end-anchored on the last covered line and capped at Forgejo's 50-lineMAX_CODE_COMMENT_LINES, so a placement always lands on a covered display line and can never 422 the batched review;getRawFilerejects empty and dot path segments; the fork branch moved inside the orchestrator's error boundary so a failed skip comment reports as a described outcome; the wait loop re-reads settle state before accepting an empty pending-claims list; the two claims-cursor walks became onewalkClaimsthat fails on a non-advancing cursor; remaining review pages are fetched sequentially;unresolveverifies the returnedresolver; the bundle targetsnode24to matchaction.yml;ListClaimsQuery.destinationis typedDestination; and the README splits its environment contract per path and cites the Forgejo rules behind the read-only fork token.Findings declined with evidence are answered on their PR threads — chiefly the fork-token 403 premise (a fork task token is
AccessModeRead, and creating an issue comment needs only issues-read plus an unlocked issue), the head-synchronize TOCTOU (Forgejo has no conditional-write primitive; workflow concurrency and marker-idempotent writes cover it), and the@j4k/reviewdeep import (a devDependency inlined by esbuild at build time, behind the one-filesrc/review/api.tsseam).Gates
pnpm knip,pnpm format:check,pnpm typecheck,pnpm lint,pnpm fta,pnpm run test(206 tests, 14 files, including bundle freshness and credentials isolation), andpnpm dedupe --check --ignore-scripts --ignore-pnpmfile— all green onf7d380f.Staging smoke: deferred
The tier-2 local staging smoke has not run. It needs an operator-minted capability token for identity
forgejo-ci-wrapper, which is not available to this branch. When the token exists, the smoke covers:j4k-ossrepo, so the tokenless service can forge-fetch the tree.dist/index.mjswith a craftedpull_request_targetevent: create, ask, pinned wait, reconcile, exit 1 on the row-7 abandoned-coverage summary.GITHUB_RUN_ID: the ask guard skips and the reconcile is idempotent.head.repo.full_name: exit 0 with the marker comment updated, re-run withoutREVIEW_CAPABILITY_TOKENset to prove the isolation.-j4kinstance with a personal token, recording whether the job task token can call the resolution route at all.The fixture-seeded green path (rows 9/10, disposition projection, superseded sweep) stays deferred behind the cluster spool blocker; it is Phase 4's validation protocol, not a gate here.
After merge, and on the user's go: move the
v1tag to the merged commit. Consumers pin the action atv1.Approach review: The orchestration and security boundary are coherent. One maintainability concern remains: the review-service adapter reaches into unpublished package source paths; see the inline comment for a more stable package boundary.
Approach review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +3,4 @@// because the published package ships sources with no exports map (guarded by// a test in j4k/review); when that changes, this file is the one-file migration.export { createCapabilityClient } from "@j4k/review/src/client/capability-client.ts";api.tsis a facade over@j4k/review/src/...internals and depends on that package shipping raw TypeScript without an exports map. That couples the wrapper build to the service repository's source layout, despite the wrapper's stated HTTP-contract boundary. Prefer publishing a stable client/wire entry point (or a small dedicated client package) and import that here; service refactors and package export changes then do not break this action or require a compatibility test in the wrapper.Not taking this.
@j4k/reviewis a devDependency here and the deep import is resolved at build time by esbuild into the committeddist/index.mjs— nothing at runtime resolves the package, so a later export-map change cannot break a released action, only a future rebuild. The package deliberately shipssrc/withexports: null(it is the service repository, not a published client), and adding a stable client entry point is work in that repository, tracked on its packaging-guard branch.src/review/api.tsexists precisely as the one-file migration seam that will change when that entry point lands; the HTTP contract boundary is unaffected because only types and constants cross it.Approach review: The approach looks good. The thin composition shell keeps forge access, review-service behavior, waiting, and reconciliation separated, while the lazy review-session factory cleanly preserves the fork credential boundary.
Approach review by Codex GPT-5.6 SOL (gpt-5.6-sol)
Approach review: The composition, fork gate, staged wait, and reconciliation boundaries are coherent. One material maintenance concern is noted inline below.
Approach review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +3,4 @@// because the published package ships sources with no exports map (guarded by// a test in j4k/review); when that changes, this file is the one-file migration.export { createCapabilityClient } from "@j4k/review/src/client/capability-client.ts";This boundary still deep-imports
@j4k/review/src/...implementation files. That couples the action to the service package's internal layout, so a compatible package update can break bundle resolution without changing the HTTP contract. Prefer exposing a stable public client/wire entry point (or dedicated contract package) and importing that here; the current seam only localizes the eventual migration after a break.Not taking this.
@j4k/reviewis a devDependency here and the deep import is resolved at build time by esbuild into the committeddist/index.mjs— nothing at runtime resolves the package, so a later export-map change cannot break a released action, only a future rebuild. The package deliberately shipssrc/withexports: null(it is the service repository, not a published client), and adding a stable client entry point is work in that repository, tracked on its packaging-guard branch.src/review/api.tsexists precisely as the one-file migration seam that will change when that entry point lands; the HTTP contract boundary is unaffected because only types and constants cross it.Approach review: The module boundaries and deferred review-credential thunk fit the action's security and reconciliation flow. One scaling alternative is noted inline.
Approach review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +144,4 @@get(`${pull(prNumber)}/reviews?page=${n}&limit=${limit}`);const first = await page(1);const rest = Math.max(Math.ceil((totalCount(first.response) ?? 0) / limit) - 1, 0);const others = await Promise.all(Array.from({ length: rest }, (_x, i) => page(i + 2)));restis derived from server-controlled pagination metadata, so this launches every remaining review page at once. A PR with a large review history can create an unbounded request burst and hit Forgejo or rate limits; use a boundedp-map/p-limitconcurrency (or sequential page reads) for this collection.Fixed in
f7d380f. The remaining review pages are fetched sequentially in aforloop instead ofPromise.allover a server-derived page count, so a long review history cannot fan out into a request burst.Summary: Found 6 actionable issues: deleted-head handling, superseded-pass pinning, no-content Forgejo responses, marker spoofing, unverified reopen state, and fork-token documentation.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -11,0 +41,4 @@(`src/credentials.ts`) and flows only into the `Authorization` header of requests to`REVIEW_SERVICE_URL`.- Forge writes use the job's own task token: write-mode on same-repo pull requests,read-only on fork pull requests. The fork path performs only the labeled-skip issue🟡 Medium: This says fork PRs use a read-only Forge token and that read mode suffices, but
reconcileForkSkip()callscreateIssueComment/editIssueComment. A read-only token will 403 on the only fork path, so the run cannot return the documented greenfork-skipresult. Document the required write permission or change the fork outcome to avoid a write.The premise is wrong, checked against the Forgejo source rather than the docs page:
GetActionRepoPermissiongives a forkpull_request_targettask tokenAccessModeRead, andCreateIssueCommentrequires only read access on the issues unit plus an unlocked issue — comment creation is not gated on write. The re-run edit path edits a comment the same actions identity posted, andcanUserEditCommentallows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now citesGetActionRepoPermissionand both rules so the pairing no longer reads as self-contradictory (f7d380f).@ -0,0 +137,4 @@createIssueComment: (prNumber, body) =>call("POST", `${repo}/issues/${prNumber}/comments`, { body }),editIssueComment: (commentId, body) =>call("PATCH", `${repo}/issues/comments/${commentId}`, { body }),🟡 Medium:
editIssueCommentgoes throughcall(), which unconditionally parses the response body as JSON. Forgejo/Gitea permits this PATCH endpoint to return204 No Content; an otherwise successful summary update then throwsUnexpected end of JSON inputand aborts reconciliation. Use a no-content-aware request path or accept an empty response here.Checked against the Forgejo source rather than inferred:
editIssueCommentinrouters/api/v1/repo/issue_comment.goanswers a content edit withctx.JSON(http.StatusOK, convert.ToAPIComment(...)). The204path exists only for non-content comment types (the delete route and the label/assignee-style comments), which this endpoint never reaches with abodypayload. So the PATCH used here always returns a JSON body andcall()is correct; adding a no-content branch would be dead code.@ -0,0 +93,4 @@async function unresolve(prNumber: number, conversation: Conversation): Promise<boolean> {try {await forge.unresolveConversation(prNumber, conversation.reviewId, conversation.anchor.id);🟡 Medium:
resolve()verifies that the returned anchor has a resolver, butunresolve()treats any fulfilled DELETE as success and discards the returned comment. If the server returns a stale non-null resolver, the wrapper reports success while areopeneddisposition remains resolved. Check the returned resolver consistently.Fixed in
f7d380f.unresolve()now checks the returned anchor the same wayresolve()does and throwsreopening did not stick for comment <id>whenresolveris still non-null. Verified against the live instance first:DELETE .../resolutionreturns 200 with the updated comment JSON whoseresolverisnull, so the check is testing a real field rather than an assumed shape. Recorder support added insrc/reconcile/dispositions.test.ts.@ -0,0 +47,4 @@for (const item of findings.items) {const { claim } = item;const marker = claimMarker(claim.id);if (bodies.some((body) => body.includes(marker))) {🟡 Medium: Any comment body containing
<!-- review:claim:<id> -->is treated as wrapper-owned. A PR participant can post that marker after seeing the claim ID and suppress the inline finding on reruns; the same unverified marker is also used for disposition correlation. Verify wrapper authorship/metadata or use an unforgeable ownership token before skipping or acting on a thread.Not taking this. On a fork pull request the wrapper returns at the gate and never reaches inline reconciliation, so the only PR participants who can reach this path are same-repository authors — and they already hold write access to the branch and the workflow. A participant who wants to suppress a finding does not need a forged marker; the threat model here is not "a person with push access lies to themselves". The finding also never disappears: the summary comment lists every finding regardless of inline placement, so a forged marker can at most suppress a duplicate inline thread. Forgejo exposes no unforgeable per-comment metadata for actions identities, so the alternative would be a second write path with no security gain.
@ -0,0 +67,4 @@headSha: pull.head.sha,baseBranch: pull.base.ref,headBranch: pull.head.ref,isFork: pull.head.repo.full_name !== slug,🟠 High:
pull_request_targetpayloads can havehead.reponull when the source repository has been deleted. This dereference crashes beforeisForkcan classify the PR, so the fork-skip path does not produce its promised explicit result. Treat a missing repo as fork/untrusted (or safely re-fetch the head repo) before readingfull_name.Fixed in
f7d380f.PullRequestTargetEvent.head.repois now typed{ full_name: string } | null(matchingRawPullinsrc/forge/client.ts) and the fork verdict readsisFork: (pull.head.repo?.full_name ?? "") !== slug, so a deleted source fork classifies as a fork and reaches the skip path instead of throwing. Covered by a new case insrc/wrapper/event-context.test.ts.@ -0,0 +108,4 @@known: FindingsResponse | undefined,): Promise<void> {const { inputs } = deps;const report = await session.getReport(reviewId);🟠 High: The superseded flag is only logged; reconciliation still fetches the report and findings through review-level methods with no
pinnedPassId. Once a newer pass becomes current, this older run can publish the newer pass's report/findings while claiming to reconcile its pinned pass. Preserve a pass-specific snapshot/selector through reconciliation, or suppress stale writes when that is unavailable.Not a defect: report, findings and claims are review-level by construction in the service contract (ADR 0021 and the wrapper spec) —
GET /reportand the findings/claims reads take nopass_idbecause the service exposes no pass-scoped projection of them. There is nothing pass-specific to preserve on the wrapper side. The wrapper already pins what the contract lets it pin (the wait and the coverage read), and the README's row-11 note documents exactly this residue: the summary may render the newer pass while the exit code follows the pinned one, and closing it needs apass_idon the report read, which is service-side work.Summary: Found 6 actionable issues (1 critical, 2 high, 2 medium, 1 low).
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -11,0 +20,4 @@| `GITHUB_API_URL` | Forge API base, supplied by the runner. || `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Standard runner event environment; the wrapper's PR inputs come from the event payload. |Every variable is required. A missing one is a configuration defect and fails the run🟢 Low: This says every environment variable is required, but the fork gate intentionally never calls
reviewCredentials()and the documented isolation scenario relies on fork runs withoutREVIEW_CAPABILITY_TOKEN. Clarify that the capability token is required only for the same-repository service path.Fixed in
f7d380f. The README's environment contract now splits the requirement per path: the forge/runner variables are required on every path, whileREVIEW_SERVICE_URLandREVIEW_CAPABILITY_TOKENare required only on the same-repository path, because a fork pull request returns at the gate without opening a service session.@ -0,0 +143,4 @@const page = (n: number): Promise<Fetched> =>get(`${pull(prNumber)}/reviews?page=${n}&limit=${limit}`);const first = await page(1);const rest = Math.max(Math.ceil((totalCount(first.response) ?? 0) / limit) - 1, 0);🟡 Medium: Review pagination treats a missing or invalid
X-Total-Countas zero and silently returns only page 1.listReviewsfeeds both inline-marker discovery and the superseded sweep, so a proxy/API response without that header causes duplicate inline comments and leaves older threads unprocessed. Fail closed on an unusable count instead of silently truncating the listing.Already fixed before this round, in
7abe25e:listReviewsnow fails closed — an absent or non-finiteX-Total-Countthrowsincomplete review listing for #<n>: unusable X-Total-Count ...instead of truncating to page 1. (f7d380fadditionally made the remaining pages sequential.)@ -0,0 +15,4 @@type DispositionRecord = NonNullable<ProjectedClaim["disposition"]["current"]>;function claimIdOf(conversation: Conversation): string | undefined {🟠 High:
claimIdOfreturns only one claim from a conversation.inline.tsbatches all findings into one Forge review, so two findings on the same path/display line share the review id and are grouped byderiveConversations; path anchors on one file can produce this routinely. Only the first finding gets a disposition projection (and resolving it can resolve the shared conversation). Track all claim markers or create separate review/thread ids per finding.Already fixed before this round, in
7abe25e: dispositions no longer key on a singleclaimIdOfper conversation — every claim marker in the thread is collected, so two findings that share a review id, path and display line each get their own projection.@ -0,0 +143,4 @@const pinnedPassId = await pinPass(deps, session, reviewId);const result = await deps.wait(session, reviewId, pinnedPassId, deps.clock);const pull = await deps.forge.getPull(inputs.prNumber);🟡 Medium: The head check is a time-of-check/time-of-use race: the PR can synchronize after
getPullreturns and beforereconcileperforms its summary, inline, and disposition writes. The old run then posts findings for the previous head onto the new head despite thehead-movedrule. Guard mutations with an expected-head/conditional check or otherwise revalidate ownership at the write boundary.Deliberate, and not closable the way the finding suggests. Forgejo exposes no conditional-write primitive on comments — there is no if-match on a comment create or edit — so "make writes conditional on the expected SHA" is not implementable against this API. The design closes the window from the other side: the workflow template uses
concurrency: { group: review-<pr>, cancel-in-progress: true }, so a synchronize cancels the older run, and every write is marker-idempotent, so the newer run rewrites the same summary comment and skips threads that already carry their claim marker. The worst case is a brief interval where the summary shows the previous head, which the next run corrects.@ -0,0 +162,4 @@export const runWrapper: RunWrapper = async (deps) => {if (deps.inputs.isFork) {await deps.summary.reconcileForkSkip({ prNumber: deps.inputs.prNumber });🔴 Critical: The fork gate calls
reconcileForkSkip, which lists and then creates or edits an issue comment. Those are Forge writes, so this contradicts the documented read-only fork token; a normal fork run will hit 401/403 and this branch is outside thetry/catch, so the intended greenfork-skipresult never completes. Grant fork runs issue-comment write permission, or make the fork path no-write and handle the failure explicitly.Half agreed and fixed in
f7d380f: the fork branch is now inside thetry, so any failure ofreconcileForkSkipreports as a described outcome instead of an unhandled rejection.The 401/403 premise is not right, checked against the Forgejo source rather than the docs: a fork task token gets
AccessModeReadfromGetActionRepoPermission, and creating an issue comment requires only read on the issues unit with the issue unlocked; the re-run edit path edits a comment the same actions identity posted, and the poster is always allowed to edit its own comment. So row 1 completes green under the documented read-only token. The README's security-model bullet now spells that out instead of leaving it to inference.@ -0,0 +81,4 @@graceDeadlineAt: number,): Promise<GroundingOutcome> {const outcome = await poll<SettleOutcome | "regressed">(clock, graceDeadlineAt, async () => {if (!(await hasPendingGrounding(session, reviewId))) {🟠 High: When
hasPendingGroundingreturns false, grounding polling returnssettledwithout callingreadSettle. If triage regresses fromsettledtodueorstalledwhile the pending-claims list is empty, the wrapper still proceeds to reconciliation and can report a green result for a non-terminal review. Re-read triage state on every grounding cycle before accepting the empty list.Fixed in
f7d380f.src/wrapper/wait.tsnow re-reads settle state on every grounding cycle before consulting the pending list:readSettleruns first,stalledand a regressed/absent pass short-circuit, and only then does an empty pending list count assettled. Two new tests cover a pass that moved on and a triage regression during the final grounding probe.Summary: Found 2 high issues and 1 medium issue.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -0,0 +97,4 @@if (located === undefined || !intersects(covered, located.start, located.span)) {return undefined;}return { path: anchor.path, newPosition: located.start, extraLines: located.span - 1 };🟠 High:
extraLinesis the full snippet span with no Forge-side bound. Valid service snippets may contain more than 50 lines (the schema limits bytes, not lines), but Forgejo rejects a review comment whenextra_lines_count + 1exceedsUI.MAX_CODE_COMMENT_LINES(50 by default). Because all findings are sent in onecreateInlineReview, one long anchor makes the reconciliation fail with 422 and skips later disposition projection. Fall back to a one-line covered anchor, or otherwise cap ranges to a negotiated limit, before building the batch.Fixed in
f7d380f.src/reconcile/anchor-map.tsnow caps a snippet range atMAX_COMMENT_LINES = 50(Forgejo'ssetting.UI.MaxCodeCommentLines, enforced byValidateCodeCommentLineRange): the range is end-anchored on the last covered line and the start is pulled forward toend - 49, soextra_lines_countcan never exceed the instance limit and one long anchor can no longer 422 the whole batched review.@ -0,0 +113,4 @@current: DispositionRecord,supported: boolean,): Promise<boolean> {if (!hasMarker(conversation, dispositionMarker(current.kind, claimId))) {🟡 Medium: The projection marker is keyed only by disposition kind and claim ID. A later disposition record can have the same kind but a new rationale or
fix_ref(for example, fixed → reopened → fixed); the oldfixedmarker makes this branch skip the new reply, so the PR thread displays stale disposition details even though resolution state changes. Includecurrent.idorcontent_keyin the marker/deduplication key so only the exact record is suppressed.Already fixed before this round, in
7abe25e: the projection marker carries the disposition record's identity, not just its kind and claim id, so a fixed → reopened → fixed sequence posts the new reply instead of being suppressed by the earlierfixedmarker.@ -0,0 +20,4 @@/** Position-anchored: a human reply that quotes the marker mid-body is not* ours, so the match must start at byte 0. */function findOurComment(comments: readonly IssueComment[]): IssueComment | undefined {return comments.find((comment) => comment.body.startsWith(SUMMARY_MARKER));🟠 High:
findOurCommenttrusts only a public, fixed body prefix and does not verify who authored the comment. Any PR participant can pre-create a comment starting with this marker; the wrapper will either fail trying to patch someone else’s comment or overwrite a comment that remains attacker-owned and can later be edited to falsify the displayed review summary. Include the Forge comment author inIssueCommentand only reuse comments owned by the automation identity; otherwise create a new summary comment.Not taking this, for the same reason as the inline marker: a fork pull request returns at the gate before any reconciliation, so the only participants able to pre-create the marker comment are same-repository authors, who already have write access to the workflow and branch. Under the fork path's read-mode task token an attempt to edit a foreign comment is a 403, not a silent takeover. And the summary is never the sole record of a finding — the exit code and the check conclusion come from the outcome, not from the comment body. Carrying the author through
IssueCommentwould add a field the wrapper cannot authenticate against an actions identity in a way an owner cannot also produce.Summary: Found 2 high and 1 medium correctness/integration issues.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +43,4 @@// Failure row 1: a real green pass with visible text, never a bare skipped// status — and reachable with no /v1 call and no capability credential.reconcileForkSkip: async ({ prNumber }) => {await upsert(forge, prNumber, FORK_SKIP_BODY);🟠 High: The fork branch still calls
upsert, which POSTs a new issue comment or PATCHes an existing one. Forgejo givesFORGEJO_TOKENread-only permissions for forkpull_request_targetruns, so this returns 403 and the promised greenfork-skippath never completes. Either skip all writes on forks or run the marker update in a separately authorized trusted job; a read-only token cannot create or edit it.The premise is wrong, checked against the Forgejo source rather than the docs page:
GetActionRepoPermissiongives a forkpull_request_targettask tokenAccessModeRead, andCreateIssueCommentrequires only read access on the issues unit plus an unlocked issue — comment creation is not gated on write. The re-run edit path edits a comment the same actions identity posted, andcanUserEditCommentallows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now citesGetActionRepoPermissionand both rules so the pairing no longer reads as self-contradictory (f7d380f).@ -0,0 +67,4 @@headSha: pull.head.sha,baseBranch: pull.base.ref,headBranch: pull.head.ref,isFork: pull.head.repo.full_name !== slug,🟠 High:
pull.head.repo.full_nameis dereferenced beforeisForkis computed.head.repocan be null when a source fork has been deleted; the Forge client already models this same response as nullable. An event for that PR therefore throws before the fork gate and produces no skip result. Treat a missing head repository as a fork, for example withpull.head.repo?.full_name ?? "".Fixed in
f7d380f.PullRequestTargetEvent.head.repois now typed{ full_name: string } | null(matchingRawPullinsrc/forge/client.ts) and the fork verdict readsisFork: (pull.head.repo?.full_name ?? "") !== slug, so a deleted source fork classifies as a fork and reaches the skip path instead of throwing. Covered by a new case insrc/wrapper/event-context.test.ts.@ -0,0 +143,4 @@const pinnedPassId = await pinPass(deps, session, reviewId);const result = await deps.wait(session, reviewId, pinnedPassId, deps.clock);const pull = await deps.forge.getPull(inputs.prNumber);🟡 Medium: The head-moved check is a one-time snapshot, but
classifyandreconcileperform later service and Forge writes. If a new commit lands after this GET, the old run can still post its report, inline comments, and dispositions onto the new PR head despite the newer-run ownership guarantee. Re-check immediately before writes or serialize/cancel overlapping runs.Deliberate, and not closable the way the finding suggests. Forgejo exposes no conditional-write primitive on comments — there is no if-match on a comment create or edit — so "make writes conditional on the expected SHA" is not implementable against this API. The design closes the window from the other side: the workflow template uses
concurrency: { group: review-<pr>, cancel-in-progress: true }, so a synchronize cancels the older run, and every write is marker-idempotent, so the newer run rewrites the same summary comment and skips threads that already carry their claim marker. The worst case is a brief interval where the summary shows the previous head, which the next run corrects.Summary: Reviewed the whole wrapper composition (entry, credentials, event context, service session, wait protocol, forge client, three reconcilers) against the outcome table in the README. The structure holds up — the fork gate really does precede every capability-token read, the failure-row mapping matches the table, and the unit tests are unusually thorough.
Found 5 medium and 5 low issues. The medium ones: an unescaped
..in the raw-file path lets a service-supplied anchor redirect an authenticated GET anywhere under the API base; thepull_request_targetpayload is cast rather than parsed, so a deleted fork repo (head.repo: null) crashes a path the forge client already handles; multi-line snippet placement checks the wrong line against the diff (Forgejo buckets at the range end,intersectsaccepts any overlap); the fork branch sits outside the orchestrator's error boundary, so a failed skip comment becomes an unhandled rejection instead of row 1; and the README's "read-only token ... which read mode suffices for" cannot be true of an issue-comment POST — one half of that sentence is wrong, and it is the one path the staging smoke has not run.I could not execute the suite (
node_modulesis absent in the review sandbox), so the gate results in the PR body are taken as reported, not verified.Code review by Claude Code Opus (opus)
@ -11,0 +41,4 @@(`src/credentials.ts`) and flows only into the `Authorization` header of requests to`REVIEW_SERVICE_URL`.- Forge writes use the job's own task token: write-mode on same-repo pull requests,read-only on fork pull requests. The fork path performs only the labeled-skip issue🟡 Medium: this pairing looks self-contradictory: creating the labeled-skip comment goes through
POST /repos/{slug}/issues/{n}/comments, which is a write — a genuinely read-only token answers 403, and row 1 then never posts its "not reviewed" block (and, per the orchestrator note, dies with an unhandled rejection rather than exit 0). Either the fork run's task token is not read-only forpull_request_target(in which case this bullet overstates the boundary) or row 1 is unimplementable as written. The PR body lists this exact path as an unrun staging smoke, so it is worth settling before merge and correcting whichever half is wrong.The premise is wrong, checked against the Forgejo source rather than the docs page:
GetActionRepoPermissiongives a forkpull_request_targettask tokenAccessModeRead, andCreateIssueCommentrequires only read access on the issues unit plus an unlocked issue — comment creation is not gated on write. The re-run edit path edits a comment the same actions identity posted, andcanUserEditCommentallows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now citesGetActionRepoPermissionand both rules so the pairing no longer reads as self-contradictory (f7d380f).@ -0,0 +11,4 @@bundle: true,platform: "node",format: "esm",target: "node26",🟢 Low: the bundle targets
node26whileaction.ymldeclaresruns.using: node24, so the runtime executingdist/index.mjsis the runner's Node 24. Nothing in the current sources or in bundled zod emits Node 26-only syntax, so this is latent rather than broken today — but the first use of syntax esbuild considers safe for 26 and not 24 becomes a parse error on the runner, not a build failure. Settarget: "node24"to match the declared runtime (.node-version/enginescan stay at 26 for the toolchain).Fixed in
f7d380f.src/bundle-options.tsnow setstarget: "node24", with a comment namingaction.yml'sruns.using: node24as the reason; the toolchain's own Node version is unaffected.@ -0,0 +92,4 @@query: {after: string | undefined;limit: number | undefined;destination: string | undefined;🟢 Low:
destinationis typedstring, so the only caller's literal ("grounding-pending"insrc/wrapper/wait.ts) is unchecked — a typo would silently query unfiltered claims and makehasPendingGroundingalways true, i.e. every run endsgrounding-pending.Destinationis already re-exported fromsrc/review/api.tsand otherwise unused; use it here.Fixed in
f7d380f.ListClaimsQuery.destinationinsrc/contract/types.tsis nowDestination | undefined(the type is imported alongside the others), so the"grounding-pending"literal insrc/wrapper/wait.tsis checked at compile time.@ -0,0 +97,4 @@return fetched.text;},getRawFile: async (path, ref) => {const segments = path.split("/").map((segment) => encodeURIComponent(segment));🟡 Medium:
encodeURIComponentdoes not escape., so a claim anchor path of../../../../admin/userssurvives segment encoding and the WHATWG URL parser then resolves the dot segments away:The anchor path is service-supplied (ultimately model-derived), so an authenticated GET carrying
FORGEJO_TOKENcan be steered to an arbitrary endpoint underGITHUB_API_URL. The slug is already defended against exactly this (encodeSlug+ the hostile-slug test); the path is not. Reject any segment that is"","."or".."before building the URL (or build withnew URL()and verify the resulting pathname still starts with the repo raw prefix).Fixed in
f7d380f.getRawFilenow rejects any path with an empty,.or..segment before building the URL (a warning, thenundefined— a claim path that cannot be read is already a non-fatal case), so percent-encoded dot segments can no longer be resolved away by the URL parser onto another API route. Test added insrc/forge/client.test.ts.@ -0,0 +94,4 @@return undefined;}const located = locate(fileText, anchor.snippet);if (located === undefined || !intersects(covered, located.start, located.span)) {🟡 Medium:
intersectsaccepts an overlap anywhere in the located range, but by this module's own display-line rule (AnchorPlacement.extraLinesdoc andsrc/forge/conversations.ts) Forgejo buckets the thread atnew_position + extra_lines_count— the range end. So a snippet located at lines 10–14 where only line 10 is covered passes the check and is posted withnew_position: 10, extra_lines_count: 4, anchoring the thread at line 14, which the PR diff does not render. The existing multi-line test only covers the case where the end line happens to be covered.Require the display line to be covered — e.g. check
covered.has(located.start + located.span - 1), or shrink the range to end on the last covered line inside it.Fixed in
f7d380f. Theintersectscheck is gone:lastCovered()returns the last covered line inside the located range and the placement is end-anchored on it (newPosition = max(start, end - 49),extraLines = end - start), so the display line Forgejo buckets the thread at is covered by construction. New cases insrc/reconcile/anchor-map.test.ts.@ -0,0 +20,4 @@/** Position-anchored: a human reply that quotes the marker mid-body is not* ours, so the match must start at byte 0. */function findOurComment(comments: readonly IssueComment[]): IssueComment | undefined {return comments.find((comment) => comment.body.startsWith(SUMMARY_MARKER));🟢 Low: the marker match is position-anchored but not author-anchored, and
IssueCommentdoes not carry the author at all. A PR author can post a comment whose body starts with<!-- review:summary -->before the run;upsertthen edits their comment instead of creating the wrapper's, and they can edit it afterwards to any text they like — the check still passes green. Carrying the comment'suserthroughIssueCommentand matching on the wrapper's own identity closes it.Not taking this, for the same reason as the inline marker: a fork pull request returns at the gate before any reconciliation, so the only participants able to pre-create the marker comment are same-repository authors, who already have write access to the workflow and branch. Under the fork path's read-mode task token an attempt to edit a foreign comment is a 403, not a silent takeover. And the summary is never the sole record of a finding — the exit code and the check conclusion come from the outcome, not from the comment body. Carrying the author through
IssueCommentwould add a field the wrapper cannot authenticate against an actions identity in a way an owner cannot also produce.@ -0,0 +67,4 @@headSha: pull.head.sha,baseBranch: pull.base.ref,headBranch: pull.head.ref,isFork: pull.head.repo.full_name !== slug,🟡 Medium:
pull.head.repois nullable when the fork repo has been deleted —src/forge/client.tstypes it{ full_name: string } | nulland maps it to"", and there is a test for it. On this path the payload is onlyas-cast, so the same PR read from the event payload throwsTypeError: Cannot read properties of null (reading 'full_name')instead of derivingisFork: true. Since nothing catches it (see thesrc/main.tsnote), the job dies with a raw stack.Read it defensively (
pull.head.repo?.full_name ?? ""), or better, parse the payload with a Zod schema per the repo's parse-don't-validate rule — the same cast also means aworkflow_dispatchpayload without aninputsobject throws aTypeErrorrather than the intendedpr_numbermessage.Fixed in
f7d380f.PullRequestTargetEvent.head.repois now typed{ full_name: string } | null(matchingRawPullinsrc/forge/client.ts) and the fork verdict readsisFork: (pull.head.repo?.full_name ?? "") !== slug, so a deleted source fork classifies as a fork and reaches the skip path instead of throwing. Covered by a new case insrc/wrapper/event-context.test.ts.The second half is fixed too:
workflow_dispatchpayloads go throughparsePrNumber(payload.inputs?.pr_number), which rejects a missinginputsobject, a non-integer, and a non-positive number with the intended message instead of aTypeError.@ -0,0 +87,4 @@): Promise<ClaimsPageResponse["items"]> {const items: ClaimsPageResponse["items"] = [];let after: string | undefined = undefined;for (;;) {🟢 Low: the cursor walk has no bound: a service that keeps returning a non-null
next_after, or repeats the same cursor, spins forever accumulating pages with no deadline above it (unlike the wait loop).hasPendingGroundinginsrc/wrapper/wait.tshas the same shape. Bound the page count, or stop whenpage.next_after === after.Fixed in
f7d380f. Both cursor walks are now one function,walkClaimsinsrc/review/claims-walk.ts, which throwsclaims cursor did not advance past <cursor>when the service returns the samenext_aftertwice. A non-advancing cursor is a service defect, so it fails loudly rather than paging forever.@ -0,0 +108,4 @@known: FindingsResponse | undefined,): Promise<void> {const { inputs } = deps;const report = await session.getReport(reviewId);🟢 Low: the reconcile reads carry no retry —
session.tsdeliberately leaves transient tolerance to the wait loop, but these three reads run after it, and persession.test.tsa 5xx on a read is rethrown unchanged. One transient blip onGET /reportafter a 45-minute wait discards the whole reconciliation and turns a green outcome red, with nothing written. Consider reusing the bounded ladder fromwithRetriesfor the reads insidereconcile.Deliberate placement, not an oversight.
withRetriesbelongs to the wait loop because that is where transient tolerance has a deadline to live inside; the reconcile reads run after it and have no bound of their own, so a retry ladder there would silently extend a run past the wait budget. A red run from a transient 5xx onGET /reportis re-runnable and every write in reconcile is marker-idempotent, so the recovery path is a re-run that costs one more read — not a partially-written surface. Failing loudly on an unexpected read failure is the intended behaviour.@ -0,0 +162,4 @@export const runWrapper: RunWrapper = async (deps) => {if (deps.inputs.isFork) {await deps.summary.reconcileForkSkip({ prNumber: deps.inputs.prNumber });🟡 Medium: the fork branch sits outside the
try, so any failure ofreconcileForkSkip(403, forge 5xx, network) escapesrunWrapperand — sincesrc/main.tsawaits it at top level with no boundary — surfaces as an unhandled rejection: exit 1 with a raw stack instead of row 1's controlled outcome, and none of theFAILURE_REMEDIEStext. Every other outcome in the table gets a described error.Move the fork branch inside the
try(or wrap the whole body), so a failed skip comment reports through the same path as the rest.Fixed in
f7d380f— the fork branch now sits inside thetry, so a failed skip comment reports through the same outcome/remedy path as everything else rather than escaping as an unhandled rejection. The gate still runs beforedeps.openSession, which is the actual security boundary.On the 403 premise: a fork
pull_request_targettask token isAccessModeRead(GetActionRepoPermission), and Forgejo's create-issue-comment path requires only read access on the issues unit plus an unlocked issue, so the skip comment is not a 403 — the re-run edit path edits a comment the same actions identity posted, which the poster is always permitted to edit. README now states this explicitly.Summary: Found 2 high, 1 medium, and 1 low issue.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -11,0 +20,4 @@| `GITHUB_API_URL` | Forge API base, supplied by the runner. || `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Standard runner event environment; the wrapper's PR inputs come from the event payload. |Every variable is required. A missing one is a configuration defect and fails the run🟢 Low: “Every variable is required” contradicts the fork-skip implementation: fork runs intentionally never read
REVIEW_SERVICE_URLorREVIEW_CAPABILITY_TOKEN, and only need Forge credentials to attempt the skip comment. Document the required variables separately for fork and same-repository paths.Fixed in
f7d380f. The README's environment contract now splits the requirement per path: the forge/runner variables are required on every path, whileREVIEW_SERVICE_URLandREVIEW_CAPABILITY_TOKENare required only on the same-repository path, because a fork pull request returns at the gate without opening a service session.@ -0,0 +112,4 @@},listIssueComments: async (prNumber) => {const url = `${repo}/issues/${prNumber}/comments`;const { response, text } = await get(url);🟠 High: This performs one request without
page/limit, but Forgejo list endpoints paginate and expose the full size inX-Total-Count(API pagination). Once a PR has more comments than the default page,distinct !== totalthrows, so summary and fork reconciliation fail instead of finding the marker. Page through this endpoint, aslistReviewsdoes, before checking completeness.Checked against the Forgejo source:
ListIssueCommentsinrouters/api/v1/repo/issue_comment.gobuilds itsFindCommentsOptionswith noListOptions, so the endpoint ignorespage/limitand returns every comment in one response — which is why the client asserts distinct-ids ==X-Total-Countinstead of paging. The generic pagination docs describe the endpoints that setListOptions; this one does not. Paging it would add requests that return the same full set each time.@ -0,0 +24,4 @@}async function upsert(forge: ForgeClient, prNumber: number, body: string): Promise<void> {const existing = findOurComment(await forge.listIssueComments(prNumber));🟡 Medium: This is a check-then-create upsert. Two overlapping re-runs or dispatches can both observe no marker and both POST, leaving duplicate summary comments; the inline marker check has the same race. Use an idempotent/conditional create or re-check after a create conflict.
The race is already closed outside this function: the review workflow template sets
concurrency: { group: review-<pr>, cancel-in-progress: true }, so two overlapping runs for the same PR cannot both reach the upsert — the older one is cancelled. Forgejo also offers no conditional/idempotent comment create (no if-match, no client-supplied key), so a re-check after a create would be a second read that still cannot prevent the duplicate, only notice it. A dispatch run for a PR whose review run is in flight is a manual action outside the design's concurrency envelope.@ -0,0 +154,4 @@console.error(`pass ${pinnedPassId} was superseded mid-wait; reconciling the pinned pass (the newer pass gets its own run)`,);}🟠 High: The fork gate always calls
reconcileForkSkip, which can POST or PATCH an issue comment. The security model documents the forkFORGEJO_TOKENas read-only, but Forgejo requires the write issue scope for those operations (token scopes); with that token every fork run fails before returning the row-1 green outcome. Grant the minimal comment-write permission or make fork skip a no-write result.The premise is wrong, checked against the Forgejo source rather than the docs page:
GetActionRepoPermissiongives a forkpull_request_targettask tokenAccessModeRead, andCreateIssueCommentrequires only read access on the issues unit plus an unlocked issue — comment creation is not gated on write. The re-run edit path edits a comment the same actions identity posted, andcanUserEditCommentallows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now citesGetActionRepoPermissionand both rules so the pairing no longer reads as self-contradictory (f7d380f).Summary: Found 3 issues (2 high, 1 medium).
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +67,4 @@headSha: pull.head.sha,baseBranch: pull.base.ref,headBranch: pull.head.ref,isFork: pull.head.repo.full_name !== slug,🟠 High:
pull_request_targetpayloads can havehead.repo: nullafter a source repository is deleted; this dereference then throws before the fork gate and turns the intended safe skip into a failed run. Make the event type nullable and treat a missing head repository as a foreign/fork head (the Forge API client already handles the same case).Fixed in
f7d380f.PullRequestTargetEvent.head.repois now typed{ full_name: string } | null(matchingRawPullinsrc/forge/client.ts) and the fork verdict readsisFork: (pull.head.repo?.full_name ?? "") !== slug, so a deleted source fork classifies as a fork and reaches the skip path instead of throwing. Covered by a new case insrc/wrapper/event-context.test.ts.@ -0,0 +162,4 @@export const runWrapper: RunWrapper = async (deps) => {if (deps.inputs.isFork) {await deps.summary.reconcileForkSkip({ prNumber: deps.inputs.prNumber });🟠 High: The documented fork path uses a read-only
FORGEJO_TOKEN, butreconcileForkSkipcan create or edit an issue comment here. Those POST/PATCH operations require issue-write permission, so normal fork skips will fail with 403 instead of returningfork-skip. Grant the tokenwrite:issueon the base repository or make the fork path truly read-only and remove the marker-write requirement.The premise is wrong, checked against the Forgejo source rather than the docs page:
GetActionRepoPermissiongives a forkpull_request_targettask tokenAccessModeRead, andCreateIssueCommentrequires only read access on the issues unit plus an unlocked issue — comment creation is not gated on write. The re-run edit path edits a comment the same actions identity posted, andcanUserEditCommentallows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now citesGetActionRepoPermissionand both rules so the pairing no longer reads as self-contradictory (f7d380f).@ -0,0 +81,4 @@graceDeadlineAt: number,): Promise<GroundingOutcome> {const outcome = await poll<SettleOutcome | "regressed">(clock, graceDeadlineAt, async () => {if (!(await hasPendingGrounding(session, reviewId))) {🟡 Medium: When
hasPendingGroundingis false, this branch returnssettledwithout callingreadSettleagain. If a newer (including vacuous/no-claim) pass starts during that final probe,latest_passand a triage regression are never observed, leavingsupersededfalse despite the row-11 contract. Re-read settle state before accepting the no-pending result.Fixed in
f7d380f.src/wrapper/wait.tsnow re-reads settle state on every grounding cycle before consulting the pending list:readSettleruns first,stalledand a regressed/absent pass short-circuit, and only then does an empty pending list count assettled. Two new tests cover a pass that moved on and a triage regression during the final grounding probe.Summary: Found 2 medium issues.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +68,4 @@path: placement.path,body: commentBody(claim, reviewId),new_position: placement.newPosition,extra_lines_count: placement.extraLines,🟡 Medium:
placement.extraLinesis passed straight through asextra_lines_count, but no Forgejo line-range limit is enforced. A valid service snippet can span more than the instance'sMAX_CODE_COMMENT_LINES(50 by default), causing this single batchedcreateInlineReviewrequest to be rejected; because inline reconciliation is awaited before dispositions, one oversized finding prevents all remaining projections. Bound or skip oversized anchors before batching and keep them in the summary.Fixed in
f7d380f.src/reconcile/anchor-map.tsnow caps a snippet range atMAX_COMMENT_LINES = 50(Forgejo'ssetting.UI.MaxCodeCommentLines, enforced byValidateCodeCommentLineRange): the range is end-anchored on the last covered line and the start is pulled forward toend - 49, soextra_lines_countcan never exceed the instance limit and one long anchor can no longer 422 the whole batched review.@ -0,0 +143,4 @@const pinnedPassId = await pinPass(deps, session, reviewId);const result = await deps.wait(session, reviewId, pinnedPassId, deps.clock);const pull = await deps.forge.getPull(inputs.prNumber);🟡 Medium: This is only a snapshot check. Once it returns,
reconcilefetches current report/diff/file data and then writes the summary, inline review, and disposition replies usinginputs.headSha; if the PR synchronizes in that window, stale findings can be written to the new head despite the documentedhead-moved"no writes" outcome. Recheck the head immediately before each write phase or make writes conditional on the expected SHA.Deliberate, and not closable the way the finding suggests. Forgejo exposes no conditional-write primitive on comments — there is no if-match on a comment create or edit — so "make writes conditional on the expected SHA" is not implementable against this API. The design closes the window from the other side: the workflow template uses
concurrency: { group: review-<pr>, cancel-in-progress: true }, so a synchronize cancels the older run, and every write is marker-idempotent, so the newer run rewrites the same summary comment and skips threads that already carry their claim marker. The worst case is a brief interval where the summary shows the previous head, which the next run corrects.Acknowledged. The one inline finding (#46291, the
@j4k/review/src/...deep import) is declined with evidence on the thread: the package is a devDependency inlined by esbuild at build time, so nothing resolves it at runtime, andsrc/review/api.tsis the deliberate one-file migration seam for the stable client entry point being prepared in the service repository. No change from this review.Acknowledged — no findings. The lazy review-session thunk this review calls out is the fork credential boundary and stays as-is.
Acknowledged. Its inline finding (#46295) is the same package-boundary concern as #46291 and is declined with the same evidence on the thread. No change from this review.
Acknowledged. Its inline finding (#46299, unbounded review-page fan-out) is fixed in
f7d380f— the remaining pages are now fetched sequentially.Acknowledged; all six inline findings are answered on their threads. Fixed in
f7d380f: #46321 (nullhead.repo) and #46325 (unresolve now verifies the returnedresolver). Declined with evidence: #46322 (report/findings/claims are review-level by contract), #46323 (Forgejo'seditIssueCommentreturns 200 JSON for a content edit; the 204 path is other comment types), #46324 (fork PRs never reach inline reconciliation; same-repo participants already hold write access), #46326 (a fork task token isAccessModeReadand comment creation needs only issues-read — README clarified inf7d380f).Acknowledged; all six inline findings are answered on their threads. Fixed in
f7d380f: #46332 (the fork branch now sits inside the error boundary), #46333 (settle state re-read before the pending-claims list), #46337 (README env-var requirements split per path). Already fixed in7abe25e: #46334, #46335. Declined with evidence: #46336 (Forgejo exposes no conditional-write primitive;concurrency: cancel-in-progressplus marker-idempotent writes close it from the other side). The critical rating on #46332 rested on a 403 premise that the Forgejo source contradicts — see that thread.Acknowledged. Fixed in
f7d380f: #46341 (snippet ranges are now capped at Forgejo's 50-lineMAX_CODE_COMMENT_LINES). Already fixed in7abe25e: #46343. Declined with evidence: #46342 (summary-marker authorship — fork PRs return at the gate, so only same-repo authors can reach it, and they already hold write access).Acknowledged. Fixed in
f7d380f: #46348 (nullhead.repo). Declined with evidence: #46347 (a fork task token isAccessModeReadand creating an issue comment needs only issues-read, perGetActionRepoPermissionandCreateIssueComment), #46349 (head TOCTOU — no conditional-write primitive exists; workflow concurrency and marker-idempotent writes cover it).Acknowledged; every inline finding is answered on its thread. Fixed in
f7d380f: #46406 (dot-segment path guard), #46407 (nullablehead.repoplus a realpr_numberparse), #46408 (placement end-anchored on the last covered line), #46409 (fork branch inside the error boundary), #46411 (non-advancing claims cursor now throws), #46413 (target: "node24"), #46415 (Destinationtype on the query). Declined with evidence: #46410 (read-mode token can create the skip comment — README now cites the Forgejo rules), #46412 (retry deliberately belongs to the wait loop, which owns the deadline), #46414 (summary-marker authorship, same reasoning as #46342).Acknowledged. Fixed in
f7d380f: #46426 (README env-var requirements split per path). Declined with evidence: #46423 (Forgejo'sListIssueCommentssets noListOptions, so the endpoint returns every comment in one response), #46424 (fork task token isAccessModeReadand comment creation needs only issues-read), #46425 (the review workflow'sconcurrency: cancel-in-progressprevents the overlapping upsert, and Forgejo offers no conditional create).Acknowledged. Fixed in
f7d380f: #46429 (nullhead.repo), #46431 (settle re-read before accepting an empty pending list). Declined with evidence: #46430 (read-mode fork token suffices for the skip comment — see the thread and the clarified README bullet).Acknowledged. Fixed in
f7d380f: #46433 (snippet ranges capped at Forgejo's 50-line limit). Declined with evidence: #46434 (head TOCTOU — no conditional-write primitive; workflow concurrency plus marker-idempotent writes cover it).Summary: Reviewed the composition in
src/main.tsand all eight modules against the outcome table inREADME.md. The fork gate holds as documented —reviewCredentials()is reachable only through theopenSessionthunk, andsrc/main.tsis its sole importer, soREVIEW_CAPABILITY_TOKENis never read on a fork path. Path handling (encodeSlug, the dot-segment guard ingetRawFile), the pinned-pass wait loop's deadline bounds, and the claims-cursor walk all look sound; the committed bundle's tail matchessrc/main.ts.Found 2 medium and 1 low issue:
src/reconcile/summary.ts— the summary comment is matched by marker prefix without checking authorship, so a pre-seated comment hijacks the slot and turns the fork-skip row (documented exit 0) into a red check under the read-only fork token.src/reconcile/dispositions.ts— a conversation shared by a disposed and an undisposed claim is resolved, hiding a live finding; claims sharing a thread is routine for path-scoped anchors.src/forge/client.ts— the DELETE on the resolution route is parsed as JSON, so an empty body would abort the run after the surface has already been written.Not verifiable here: the Forgejo API response shapes (
extra_lines_count,X-Total-Counton issue comments) and the-j4kresolution routes;node_modulesis absent, so the gates listed in the PR body were not re-run.Code review by Claude Code Opus (opus)
@ -0,0 +54,4 @@async function call<T>(method: string, url: string, payload: unknown): Promise<T> {const { text } = await request(method, url, payload);return JSON.parse(text) as T;🟢 Low:
callparses the body unconditionally, andunresolveConversationroutes a DELETE through it. If that route answers204/empty — the usual shape for a DELETE, and the PR body lists this route as unverified against a real-j4kinstance —JSON.parse("")throws a bareSyntaxError, whichisResolutionDegradationdoes not recognise (nostatus, noresolutionUnsupported), so it escapesunresolveand fails the run after the summary comment and inline threads have already been written. Every other degrade on this route is deliberately swallowed.Handle the empty body explicitly rather than letting the parse throw: on empty text, confirm the state with a follow-up read (
listReviewComments) instead of dereferencingupdated.resolver.Not a defect, and the premise is settled by a live probe rather than the usual DELETE convention: I ran
POSTthenDELETE .../reviews/{id}/comments/{comment}/resolutionagainst this-j4kinstance (comment 46334) before writing the check — the DELETE answers200with the updated comment JSON,resolver: null. Socall()has a body to parse andupdated.resolveris a real field, not an assumption. The PR body's "unverified" line was stale on that point; the route is verified for the personal-token case. Adding an empty-body branch plus a follow-uplistReviewCommentswould be dead code guarding a shape this server does not produce.@ -0,0 +88,4 @@if (currents.some((entry) => entry.current.kind === "reopened")) {return conversation.anchor.resolver === null ? true : unresolve(prNumber, conversation);}if (currents.length === 0 || conversation.anchor.resolver !== null) {🟡 Medium:
settleresolves the conversation whenever none ofcurrentsisreopened, butcurrentsholds only the claims that already have a disposition — theflatMapat lines 171-174 drops every claim whosedisposition.currentisnull/undefined. A conversation carrying onefixedclaim and one still-undisposed claim therefore gets resolved, hiding a live finding's thread in the UI.Sharing a conversation is not exotic:
deriveConversationsbuckets by (review id, path, display line), andmapAnchormaps everyscope: "path"anchor for a file tosmallest(covered)withextraLines: 0, so two path-scoped findings on the same file always land on one thread.Mirror the reopened rule and require unanimity before closing: only resolve when every claim id of the conversation that belongs to the current review has a current disposition (e.g. pass
claimIdsintosettleand compareclaimIds.filter((id) => claimById.has(id)).lengthwithcurrents.length).Fixed in
dccfc20.settlenow takes the count of the conversation's claims that belong to the current review and resolves only when every one of them has a current disposition — a claim still unadjudicated keeps the thread open instead of being closed by its sibling. Test added insrc/reconcile/dispositions.test.ts.@ -0,0 +20,4 @@/** Position-anchored: a human reply that quotes the marker mid-body is not* ours, so the match must start at byte 0. */function findOurComment(comments: readonly IssueComment[]): IssueComment | undefined {return comments.find((comment) => comment.body.startsWith(SUMMARY_MARKER));🟡 Medium:
findOurCommentmatches on the body prefix alone, with no check that the comment is the action's own.listIssueCommentsreturns every user's comments, so anyone who posts a comment whose body starts with<!-- review:summary -->before the wrapper's first run permanently owns the summary slot, andupsertPATCHes that foreign comment instead of creating its own.On the fork path that turns a documented green outcome red: the task token is read-only there (README "Security model"), so
editIssueCommenton another user's comment is rejected, theForgeRequestErrorescapesreconcileForkSkip, andrunWrapper's catch-all returns 1 — row 1 is specified as exit 0, and a fork PR author can trigger it with one comment. It also breaks the README's stated invariant that the re-run "edits one the same actions identity posted, which the poster is always allowed to edit".Fix: carry the poster on
IssueComment(user.login/user.id) and adopt a marked comment only when it matches the authenticated identity (GET /user), otherwise create a fresh one.Fixed in
dccfc20.IssueCommentnow carriesuser.idandfindOurCommentadopts a marked comment only when that id is the Forgejo actions system user (-2, theActionsUserIDconstant every task token posts as) — a value no human account can hold, so it is the ownership proof the visible marker is not. A marker comment posted by anyone else is ignored and the wrapper creates its own. This needs noGET /userround trip because the id is a server constant, not a per-instance identity. Test added insrc/reconcile/summary.test.ts; the README security-model bullet records it.Summary: Found 1 medium issue: claims pagination can loop forever on a cursor cycle.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +21,4 @@if (page.next_after === null) {return items;}if (page.next_after === after) {🟡 Medium: This only rejects a cursor repeated immediately. A malformed service response can cycle cursors such as
A → B → A, causingwalkClaimsto issue requests forever.waitForSettleawaits this inside the grounding probe, so its grace/stage deadlines are never rechecked and the action can remain hung. Track all previously seen cursors (and reject any repeat, with an optional page bound) before continuing.Fixed in
dccfc20.walkClaimsnow tracks every cursor it has followed in aSetand throwsclaims cursor repeated <cursor>on any repeat, so anA → B → Acycle terminates at the first repeat instead of paging forever inside the grounding probe. New cycle test insrc/wrapper/orchestrator.test.ts.Summary: Found 1 high and 1 medium issue.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +21,4 @@* ours, so the match must start at byte 0. */function findOurComment(comments: readonly IssueComment[]): IssueComment | undefined {return comments.find((comment) => comment.body.startsWith(SUMMARY_MARKER));}🟠 High:
findOurCommenttreats any issue comment beginning with the predictable marker as wrapper-owned. On a fork PR, the author can post<!-- review:summary -->before the run; the read-mode task token can list/create comments but cannot edit the author's comment, soupsertcallseditIssueComment, receives 403, and the row-1 fork-skip run exits 1 instead of green. Select only a comment authored by the action identity (include poster data inIssueComment) or ignore unowned marker comments and create the wrapper comment.Fixed in
dccfc20.IssueCommentnow carriesuser.idandfindOurCommentadopts a marked comment only when that id is the Forgejo actions system user (-2, theActionsUserIDconstant every task token posts as) — a value no human account can hold, so it is the ownership proof the visible marker is not. A marker comment posted by anyone else is ignored and the wrapper creates its own. This needs noGET /userround trip because the id is a server constant, not a per-instance identity. Test added insrc/reconcile/summary.test.ts; the README security-model bullet records it.Your fork-path framing is what made this worth fixing over the earlier round's answer: the read-mode task token may edit its own comment but not a foreign one, so a pre-posted marker turned row 1's green skip into a 403. The ownership check removes that path entirely.
@ -0,0 +87,4 @@const pin: Pin = { superseded: false };const coverageDeadlineAt = clock.now() + STAGE1_DEADLINE_MS;const complete = await poll(clock, coverageDeadlineAt, async () => {🟡 Medium: Stage 1 only polls
getCoverage; it never callsreadSettle, so if another run creates a newer pass while this pinned pass remains incomplete until the 30-minute timeout,supersededstays false. The timeout then proceeds throughclassify/reconcileand can write the incomplete/latest review to the PR even though the newer pass owns the surface. Observelatest_passduring stage-1 polling (or suppress reconciliation when it changes) before returning the timeout.Fixed in
dccfc20. On the stage-1 coverage timeout the wait now performs onereadSettlebefore returning, solatest_passis observed andsupersededis set on that path too. The read is tolerant — supersession is a label on the result, never a gate, so a failed final read costs the label rather than the outcome. Two tests cover the moved-on pass and the failed read.Summary: Found 1 critical and 2 high issues.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +20,4 @@/** Position-anchored: a human reply that quotes the marker mid-body is not* ours, so the match must start at byte 0. */function findOurComment(comments: readonly IssueComment[]): IssueComment | undefined {return comments.find((comment) => comment.body.startsWith(SUMMARY_MARKER));🔴 Critical:
findOurCommenttrusts a visible body marker as proof that a comment belongs to the wrapper. Any commenter can post<!-- review:summary -->first, so the next run will overwrite that user's comment; the same marker-only trust inexistingClaimMarkers/hasMarkerlets a spoofed claim or disposition marker suppress automated writes. Include the Forge author/identity in the client model and require the wrapper author (or another non-spoofable ownership check) before editing or treating a marker as reconciled.Fixed in
dccfc20.IssueCommentnow carriesuser.idandfindOurCommentadopts a marked comment only when that id is the Forgejo actions system user (-2, theActionsUserIDconstant every task token posts as) — a value no human account can hold, so it is the ownership proof the visible marker is not. A marker comment posted by anyone else is ignored and the wrapper creates its own. This needs noGET /userround trip because the id is a server constant, not a per-instance identity. Test added insrc/reconcile/summary.test.ts; the README security-model bullet records it.Scope note: the fix covers the summary comment, the one surface where a foreign marker caused a real failure (an edit of someone else's comment, 403 on the read-mode fork path). Claim and disposition markers live on inline review comments inside a review the wrapper created; a fork PR never reaches that code, so forging one requires the write access a same-repository participant already has, and the summary still lists every finding regardless of inline state.
@ -0,0 +24,4 @@if (page.next_after === after) {throw new Error(`claims cursor did not advance past ${after}`);}after = page.next_after;🟠 High: This only rejects an immediately repeated cursor. If the service returns a valid-looking cycle such as
A → B → A,afterkeeps changing and this loop never terminates, blocking reconciliation until the workflow is killed. Track every cursor seen and throw on any repeat before following it.Fixed in
dccfc20.walkClaimsnow tracks every cursor it has followed in aSetand throwsclaims cursor repeated <cursor>on any repeat, so anA → B → Acycle terminates at the first repeat instead of paging forever inside the grounding probe. New cycle test insrc/wrapper/orchestrator.test.ts.@ -0,0 +88,4 @@known: FindingsResponse | undefined,): Promise<void> {const { inputs } = deps;const report = await session.getReport(reviewId);🟠 High: When
waitreportssuperseded, this reconciliation still reads review-level/unpinnedgetReport,getFindings, andlistClaims, while the exit and coverage decision usedpinnedPassId. A newer pass can therefore be incomplete or produce different findings, yet its report/inline/disposition data are posted under the older pass and the run exits for the old result. Abort reconciliation on supersession or add/pass through a pass-scoped read for every reconciliation surface.Same answer as #46322, and it is a contract limit rather than an oversight: report, findings and claims are review-level by construction in the service contract (ADR 0021) —
GET /reportand the findings/claims reads take nopass_id, so there is no pass-scoped read to pass through. Aborting reconciliation on supersession is worse than the current behaviour: it would leave the PR with no summary at all for a head whose review did complete, while the newer run rewrites the same marker-idempotent surface moments later anyway. The README's row-11 note documents the residue and names the service-side change (pass_idon the report read) that closes it.Acknowledged. Its findings are answered on their threads: #46598 (summary-comment ownership) and #46599 (a conversation with an unadjudicated claim) are fixed in
dccfc20; #46600 (DELETE body shape) is declined with a live probe against this instance.Acknowledged. #46632 is fixed in
dccfc20—walkClaimsnow records every cursor it follows and throws on any repeat, so a cursor cycle cannot spin inside the grounding probe.Acknowledged. Both are fixed in
dccfc20: #46634 (a marked summary comment is adopted only when the actions system user authored it — the concrete fork-path 403 this raised) and #46635 (the coverage timeout now performs one settle read sosupersededis observed on that path).Acknowledged. Fixed in
dccfc20: #46637 (marker ownership on the summary comment) and #46639 (cursor-cycle bound). Declined with evidence: #46638 — report/findings/claims are review-level by contract, so there is no pass-scoped read to thread through, and aborting reconciliation on supersession would leave a completed head with no summary at all; the README's row-11 note names the service-side change that closes the residue.Summary: Reviewed the composed wrapper end to end. Found 1 medium and 3 low issues; no critical or high defects.
The medium one is a wait-protocol edge: once the grounding grace is spent, a triage regression followed by a re-settle returns
grounding-pendingwithout ever re-checking pending claims, so a healthy review can land a red check.Checks I verified against Forgejo's current sources rather than assuming:
ListIssueCommentsreally does take noListOptions, and itsX-Total-CountisCountCommentsover the same options — the single-fetch listing plus distinct-id equality check insrc/forge/client.tsis sound.ListPullReviewspaginates and setsX-Total-Count, so failing closed on a missing header is right.ValidateCodeCommentLineRangerejectsextra_lines_count + 1 > MaxCodeCommentLines; the 50-line cap inmapAnchoryields at mostextraLines = 49, so it stays inside the limit.PullReviewCommentcarriesposition,original_position,extra_lines_countandresolver, andCreatePullReviewCommentmapsold_position > 0to a negative line —replyAtanddisplayLinematch the server's bucketing.CreateIssueCommentneeds onlyCanReadIssuesOrPullsplus an unlocked issue, andEditIssueCommentpermits the poster, so the read-mode fork path in the README holds.The fork gate ordering, the credentials thunk, the retry/failure-row mapping and the committed bundle's freshness markers all look correct. I could not run
pnpm test/typecheck—node_modulesis absent and@j4k/reviewlives on a private registry — so nothing here is based on executed tests.Code review by Claude Code Opus (opus)
@ -0,0 +151,4 @@}// Sequential: the page count comes from a server-supplied total, so a// long review history must not fan out into one request per page at once.const pages = Math.ceil(total / limit);🟢 Low: The page count is derived from the requested limit, which the server may clamp below what is asked.
When
probePageLimit()falls back toFALLBACK_PAGE_LIMIT(50) because/settings/apiwas unreadable, and the instance hasmax_response_itemsconfigured below 50, Forgejo clampslimitserver-side (utils.GetListOptions) whilepagesis still computed against 50. Withmax_response_items = 20and 45 reviews,pages = ceil(45/50) = 1, so 20 of 45 reviews are returned and the caller is told the listing is complete.That truncation is silent, and it is exactly what
listReviewsfails closed on elsewhere: the callers (existingClaimMarkers, the disposition sweep) use this listing for marker-based idempotency, so a truncated result re-posts inline threads that already exist and mis-classifies existing conversations as human threads.Deriving the stop condition from the responses rather than the requested limit closes it — e.g. keep paging while the accumulated row count is short of
totaland the last page was non-empty, instead of capping atceil(total / limit).@ -0,0 +47,4 @@for (const item of findings.items) {const { claim } = item;const marker = claimMarker(claim.id);if (bodies.some((body) => body.includes(marker))) {🟢 Low: The idempotency check is a substring match against bodies that embed service-supplied text verbatim.
commentBodyputsclaim.titleandclaim.bodyinto the comment (the>quoting on line 12-15 does not neutralize an HTML comment), andclaim.bodyis model-generated prose about a diff. A claim whose body reproduces a<!-- review:claim:… -->sequence — quoting code that contains one, for instance — makes thisincludesmatch for a claim that has no thread, so a real finding is silently never posted inline.The same substring exposure exists in
claimIdsOf(src/reconcile/conversation-markers.ts:22), where a quoted marker makes an unrelated conversation look wrapper-owned.src/reconcile/summary.ts:31already solves this for the summary marker by anchoring the match at byte 0. Anchoring here the same way — the wrapper always writes its marker as the first line — makes a quoted marker inert without changing any legitimate match.@ -0,0 +60,4 @@const forgeHost = new URL(requireEnv("GITHUB_SERVER_URL")).hostname.toLowerCase();if (eventName === "pull_request_target") {const { pull_request: pull } = readEventPayload(eventPath) as PullRequestTargetEvent;🟢 Low: The
pull_request_targetpayload is cast, not parsed, while theworkflow_dispatchbranch right below validates its one field.readEventPayload(...) as PullRequestTargetEventasserts a shape ontoJSON.parseoutput. A payload withoutpull_request(or withouthead/base) fails asTypeError: Cannot read properties of undefinedrather than the fail-fast message this module uses everywhere else — and becausesrc/main.ts:27awaitsreadWrapperInputsoutsiderunWrapper, that surfaces as a module-evaluation rejection with a raw stack, not a described outcome.prNumberis the field worth pinning down: it is interpolated straight into every forge route (${repo}/issues/${prNumber}/comments,${pull(pr)}/reviews), and unlike the dispatch path it goes through no equivalent ofparsePrNumber. Az.object({...})parse of the payload here (per the repo's parse-don't-validate rule) gives both branches the same guarantee and keeps the failure a named configuration defect.@ -0,0 +80,4 @@}return (await hasPendingGrounding(session, reviewId)) ? undefined : "settled";});return outcome ?? "grounding-pending";🟡 Medium: After a triage regression, an exhausted grace window makes this return
grounding-pendingwithout ever checking whether grounding is still pending.graceDeadlineAtis set once, at the first settled observation (line 120), and deliberately not restarted. Butpollis awhile (clock.now() < deadlineAt)loop, so when the grace has already elapsed the step body never runs andhasPendingGroundingis never called — the?? "grounding-pending"fallback fires unconditionally.Concrete sequence, all inside the 15-minute stage-2 budget:
t=0settle readssettled→graceDeadlineAt = 300_000.t=105ssettle regresses todue→pollGroundingreturnsregressed.t=350ssettle readssettledagain.pollGroundingis entered withgraceDeadlineAt = 300_000 < 350_000→ returnsgrounding-pendingimmediately.The review may have finished grounding during step 4, and the wrapper still reports row 8 and exits 1 — a red "Review" check for a healthy review.
A minimal fix is to always run the step at least once (do/while, or an explicit final
hasPendingGroundingprobe before falling back), so a spent grace budget stops the waiting without skipping the observation.Summary: Found 2 actionable issues: 1 high and 1 medium.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +19,4 @@export function claimIdsOf(conversation: Conversation): string[] {const ids: string[] = [];for (const comment of conversation.comments) {for (const match of comment.body.matchAll(CLAIM_MARKER_PATTERN)) {🟠 High:
claimIdsOfparses claim markers from every comment in a conversation, andhasMarkerlikewise trusts unowned disposition markers. A human can inject a marker into an otherwise human thread; the projector then treats it as wrapper-owned, may post a disposition/superseded reply, andsettlecan resolve or reopen that human thread without a real wrapper finding or disposition. Include comment/review author ownership in the Forge types and only parse or suppress markers from wrapper-created comments.Examined and declined. A fork pull request returns at the gate before any inline reconciliation, so the only actor who can plant a marker in a review comment is a same-repository participant — who already holds write access to the branch and the workflow, and needs no forged marker to change what the wrapper does. There is no privilege gain here, only self-deception.
The summary surface was different and is now fixed (
dccfc20): there a foreign marker caused a real failure — the read-mode fork token editing someone else's comment, 403, and row 1's green skip lost. Inline comments have no equivalent failure: a spurious suppression at most drops a duplicate inline thread while the summary comment still lists every finding, and a stray resolve on a human thread is reversible from the UI. Carrying the poster intoPullReviewCommentwould buy that nuisance case a check whose threat model is a person with push access lying to themselves.@ -0,0 +47,4 @@for (const item of findings.items) {const { claim } = item;const marker = claimMarker(claim.id);if (bodies.some((body) => body.includes(marker))) {🟡 Medium:
existingClaimMarkerscollects every review comment body, so any occurrence of a marker is treated as proof that the wrapper already posted the claim. A reviewer can add<!-- review:claim:<id> -->to a normal reply (the claim id is exposed in wrapper bodies), causing retries to skip the real inline finding while only the summary remains. Track and verify wrapper actor/review ownership before suppressing a post, as the summary reconciler does.Examined and declined. A fork pull request returns at the gate before any inline reconciliation, so the only actor who can plant a marker in a review comment is a same-repository participant — who already holds write access to the branch and the workflow, and needs no forged marker to change what the wrapper does. There is no privilege gain here, only self-deception.
The summary surface was different and is now fixed (
dccfc20): there a foreign marker caused a real failure — the read-mode fork token editing someone else's comment, 403, and row 1's green skip lost. Inline comments have no equivalent failure: a spurious suppression at most drops a duplicate inline thread while the summary comment still lists every finding, and a stray resolve on a human thread is reversible from the UI. Carrying the poster intoPullReviewCommentwould buy that nuisance case a check whose threat model is a person with push access lying to themselves.Summary: Found 2 high-severity correctness issues: issue-comment pagination can abort reconciliation on PRs over Forgejo's default page size, and long snippet anchors can submit ranges starting outside the diff.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
@ -0,0 +119,4 @@throw error;}},listIssueComments: async (prNumber) => {🟠 High: Forgejo paginates
GET /issues/{index}/comments; omittingpage/limitreturns only the default-sized first page whileX-Total-Countis the global total. This code then deliberately throws on that mismatch, so any PR with more comments than the page size fails summary/fork-skip and marker reconciliation instead of reading all markers. Walk the pages (using the configured limit) before checking completeness. Forgejo pagination.Verified against the deployed Forgejo source, not the pagination docs:
ListIssueComments(routers/api/v1/repo/issue_comment.go, lines 92-97) buildsFindCommentsOptions{IssueID, Since, Before, Type}with noListOptions, andFindCommentsapplies a page only when one is set — so this endpoint ignorespage/limitand returns every comment in one response, withX-Total-CountfromCountCommentsover the same options. That is exactly why the client asserts distinct-ids ==X-Total-Countrather than paging: the assertion is the completeness proof, and it cannot fire on a large PR. The generic pagination guide describes the endpoints that do setListOptions. Same finding as #46423 last round, with the same source evidence.@ -0,0 +114,4 @@if (end === undefined) {return undefined;}const start = Math.max(located.start, end - (MAX_COMMENT_LINES - 1));🟠 High: Capping the range from
enddoes not ensurenewPositionis itself a covered diff line. For example, the long-snippet test maps a 60-line snippet with only line 60 covered tonewPosition: 11, extraLines: 49; that start is outside the displayed hunk, and Forgejo can reject the whole batched review. Choose a covered/contiguous start (or fall back to a single-line comment) before sending the range.Checked against the Forgejo source: an uncovered start line is not rejected.
CreateCodeCommentKnownReviewID(services/pull/review.go) centers the stored hunk on the display line —displayLine := UnsignedDisplayLine(), i.e.line + extra_lines_count— and expands context byCodeCommentLines + extraLinesCount; aCutDiffAroundLinefailure is caught and logged, storing the comment without context rather than failing the request (log.Warn("CreateCodeComment: storing comment without diff context")). The only range validation isValidateCodeCommentLineRange, which checksextra_lines_count + 1 <= MaxCodeCommentLinesand nothing about coverage.LineBlameis called at the start line, which is a real file line by construction, and even its not-enough-lines error is tolerated.So the guarantee the wrapper needs is the covered display line, which end-anchoring gives it; a start above the visible hunk renders as leading context, not a 422. Making the start covered as well would either shrink a legitimate range or drop it to one line, losing the snippet the finding is about.
Summary: No actionable issues found.
Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)
Acknowledged without a fix under the round gate this PR is now under (round 4: fix only clear, severe, demonstrably real bugs). The trace is right —
graceDeadlineAtis set at the first settled observation, so a regression that resolves after the grace has expired re-enterspollGroundingwith a spent budget and returnsgrounding-pendingwithout one probe. The consequence is a red run on a possibly-grounded review, which is re-runnable and writes nothing wrong; that is not the caliber the gate still fixes this late. Worth a follow-up (a do/while, as you suggest) rather than another round on this PR.Acknowledged without a fix under the round gate this PR is now under (round 4: fix only clear, severe, demonstrably real bugs). The clamp case needs
/settings/apito be unreadable and the instance's realmax_response_itemsto be below 50 — oncode.j4k.devit is exactly 50 and the probe succeeds — and the effect is a truncated review listing, not a wrong write. Deriving the stop condition from the responses is the better shape; it belongs to a follow-up.Acknowledged without a fix under the round gate this PR is now under (round 4: fix only clear, severe, demonstrably real bugs). Anchoring the inline marker match at byte 0 is a sensible tightening, but the exploit needs a same-repository participant — a fork PR returns at the gate before inline reconciliation — and the worst outcome is a suppressed duplicate inline thread while the summary still lists the finding. Follow-up, not another round here.
Acknowledged without a fix under the round gate this PR is now under (round 4: fix only clear, severe, demonstrably real bugs). Parsing the
pull_request_targetpayload with Zod is the right end state per the repo's parse-don't-validate rule; the concrete crash this round raised (a nullhead.repo) is already typed and handled, and what remains is a malformed-payload shape that only a broken runner produces. Deferred to a follow-up rather than reworking the entry contract in round 4.Acknowledged. This PR is now in round 4, so the fix gate narrows to clear, severe, demonstrably real bugs. #46669, #46670, #46671 and #46672 are acknowledged without a fix on that basis, each with its reasoning on the thread; none is contested on merits and all are follow-up material.
Acknowledged. Both findings are declined with source evidence on their threads: #46678 — Forgejo's
ListIssueCommentssets noListOptions, so the endpoint returns every comment in one response and the distinct-ids/X-Total-Countassertion is the completeness proof; #46679 — Forgejo centers a code comment's hunk on the display line and tolerates a failedCutDiffAroundLine, so an uncovered range start is leading context, not a rejection.Acknowledged. #46674 and #46675 are declined with evidence: a fork PR returns at the gate before inline reconciliation, so only a same-repository participant — who already holds write access to the branch and workflow — can plant an inline marker, and the worst outcome is a suppressed duplicate thread while the summary still lists the finding. The surface where a foreign marker did cause a real failure, the summary comment, is identity-checked as of
dccfc20.Acknowledged — no actionable issues.
@j4k/review2.15.0 and drop the same-diff request extension #49