feat: implement the review wrapper action #4

Merged
jercik merged 22 commits from feat/wrapper into main 2026-08-08 13:47:55 +00:00
Owner

Composes the eight wrapper modules into a working action: src/main.ts builds 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 committed dist/index.mjs is 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, so REVIEW_CAPABILITY_TOKEN is never read on a fork pull request. The static isolation test and the importer allowlist still hold with src/main.ts as 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-await and promise/prefer-await-to-then fire on .then, on void 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 through require(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 --check rejects. A one-line .gitattributes marks dist/index.mjs -whitespace. No source formatting was touched.

Review-driven fixes

Two rounds of review feedback are folded in.

7abe25e closed reconciliation gaps: listReviews fails closed on an unusable X-Total-Count instead 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 a fixed → reopened → fixed sequence is not suppressed by the earlier marker. b9c4ce2 made the committed bundle byte-identical across machines.

f7d380f closed the second round: head.repo is nullable end to end (a deleted source fork classifies as a fork instead of throwing before the gate) and workflow_dispatch parses pr_number properly; snippet anchors are end-anchored on the last covered line and capped at Forgejo's 50-line MAX_CODE_COMMENT_LINES, so a placement always lands on a covered display line and can never 422 the batched review; getRawFile rejects 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 one walkClaims that fails on a non-advancing cursor; remaining review pages are fetched sequentially; unresolve verifies the returned resolver; the bundle targets node24 to match action.yml; ListClaimsQuery.destination is typed Destination; 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/review deep import (a devDependency inlined by esbuild at build time, behind the one-file src/review/api.ts seam).

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), and pnpm dedupe --check --ignore-scripts --ignore-pnpmfile — all green on f7d380f.

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:

  1. A scratch PR on a throwaway public j4k-oss repo, so the tokenless service can forge-fetch the tree.
  2. Red path end-to-end against dist/index.mjs with a crafted pull_request_target event: create, ask, pinned wait, reconcile, exit 1 on the row-7 abandoned-coverage summary.
  3. Ask replay with the same GITHUB_RUN_ID: the ask guard skips and the reconcile is idempotent.
  4. Fork skip with a foreign head.repo.full_name: exit 0 with the marker comment updated, re-run without REVIEW_CAPABILITY_TOKEN set to prove the isolation.
  5. Resolve and unresolve against the -j4k instance 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 v1 tag to the merged commit. Consumers pin the action at v1.

Composes the eight wrapper modules into a working action: `src/main.ts` builds 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 committed `dist/index.mjs` is 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, so `REVIEW_CAPABILITY_TOKEN` is never read on a fork pull request. The static isolation test and the importer allowlist still hold with `src/main.ts` as 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-await` and `promise/prefer-await-to-then` fire on `.then`, on `void 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 through `require(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 --check` rejects. A one-line `.gitattributes` marks `dist/index.mjs` `-whitespace`. No source formatting was touched. ## Review-driven fixes Two rounds of review feedback are folded in. `7abe25e` closed reconciliation gaps: `listReviews` fails closed on an unusable `X-Total-Count` instead 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 a `fixed → reopened → fixed` sequence is not suppressed by the earlier marker. `b9c4ce2` made the committed bundle byte-identical across machines. `f7d380f` closed the second round: `head.repo` is nullable end to end (a deleted source fork classifies as a fork instead of throwing before the gate) and `workflow_dispatch` parses `pr_number` properly; snippet anchors are end-anchored on the last covered line and capped at Forgejo's 50-line `MAX_CODE_COMMENT_LINES`, so a placement always lands on a covered display line and can never 422 the batched review; `getRawFile` rejects 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 one `walkClaims` that fails on a non-advancing cursor; remaining review pages are fetched sequentially; `unresolve` verifies the returned `resolver`; the bundle targets `node24` to match `action.yml`; `ListClaimsQuery.destination` is typed `Destination`; 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/review` deep import (a devDependency inlined by esbuild at build time, behind the one-file `src/review/api.ts` seam). ## 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), and `pnpm dedupe --check --ignore-scripts --ignore-pnpmfile` — all green on `f7d380f`. ## 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: 1. A scratch PR on a throwaway public `j4k-oss` repo, so the tokenless service can forge-fetch the tree. 2. Red path end-to-end against `dist/index.mjs` with a crafted `pull_request_target` event: create, ask, pinned wait, reconcile, exit 1 on the row-7 abandoned-coverage summary. 3. Ask replay with the same `GITHUB_RUN_ID`: the ask guard skips and the reconcile is idempotent. 4. Fork skip with a foreign `head.repo.full_name`: exit 0 with the marker comment updated, re-run without `REVIEW_CAPABILITY_TOKEN` set to prove the isolation. 5. Resolve and unresolve against the `-j4k` instance 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 `v1` tag to the merged commit. Consumers pin the action at `v1`.
Lays down every shared seam the wrapper's modules build against: the contract
types and outcome table, the contract constants, the isolated credentials
module, the one-file @j4k/review seam, the Forgejo wire shapes and the
conversation partition, action.yml, and the esbuild bundle machinery with a
freshness test. src/main.ts is a stub that fails loudly until the modules are
composed.

Also regenerates the align-managed renders for the repo's new vitest and
typescript-scripts traits.
Wraps the capability client behind the ReviewSession interface. The create
and ask writes retry transient faults on a bounded ladder; every status the
contract names maps to its failure row, and the reads leave transient
tolerance to the wait module's poll loop.
Stage 1 polls the pinned pass's coverage; stage 2 waits on the review-level
triage settle, flagging supersession without ending the wait. A settled review
still holds until grounding-pending claims clear within the grace window, and a
settle that regresses falls back to the stage-2 dispatch.
Implements ForgeClient against Forgejo v16: task-token auth, per-segment
percent-encoded paths, the X-Total-Count completeness check on issue
comments, full page/limit review pagination behind a cached
/settings/api limit, a cached -j4k version probe that degrades to
false, and conversation resolution that flags a 404 as an absent
resolution API.
feat: compose the wrapper entry point
Some checks failed
commit-msg / commitlint (pull_request) Successful in 20s
Checks / quality-checks (pull_request) Failing after 23s
Dedupe check / dedupe-check (pull_request) Successful in 27s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m34s
PR Review / forgejo-review-approach-luna-1 generator (pull_request_target) Successful in 3m4s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Successful in 3m4s
PR Review / forgejo-review-approach-luna-2 generator (pull_request_target) Successful in 3m21s
PR Review / forgejo-review-approach-luna-3 generator (pull_request_target) Successful in 4m23s
PR Review / forgejo-review-code-luna generator (pull_request_target) Successful in 11m57s
PR Review / forgejo-review-code-luna-2 generator (pull_request_target) Successful in 12m9s
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 12m46s
PR Review / forgejo-review-code-luna-3 generator (pull_request_target) Successful in 14m3s
PR Review / Dispatch and observe exact review writers (pull_request_target) Has been cancelled
6e06bc97ff
Wires the merged modules together in src/main.ts and rebuilds the committed bundle.
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctYXBwcm9hY2gtbHVuYS0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5MzgzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImM2NjFjMDQzLTg4ZDQtNGNmYy1iMmIyLWY4ODdlMDRjZjYxYyJ9 -->
@ -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.ts is 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.

`api.ts` is 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.
Author
Owner

Not taking this. @j4k/review is a devDependency here and the deep import is resolved at build time by esbuild into the committed dist/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 ships src/ with exports: 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.ts exists 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.

<!-- gh-feedback:reply-to:46291 --> Not taking this. `@j4k/review` is a devDependency here and the deep import is resolved at build time by esbuild into the committed `dist/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 ships `src/` with `exports: 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.ts` exists 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.
forgejo-actions left a comment

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 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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctYXBwcm9hY2gtc21hcnQtMSIsInNvdXJjZVdvcmtmbG93UnVuSWQiOiIxOTM4MyIsInNvdXJjZUdlbmVyYXRvckF0dGVtcHQiOiIxIiwicmVzdWx0SWQiOiIyMWMyZWUxZi05ZDJkLTQzMTgtYmY5OS01MjA1YzBmZTAwNjEifQ== -->
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctYXBwcm9hY2gtbHVuYS0yIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5MzgzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjY5MGY2MWMyLTUxOTEtNDI5My1hYjQ4LWQ4MmY2YjNkMmVlYSJ9 -->
@ -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.

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.
Author
Owner

Not taking this. @j4k/review is a devDependency here and the deep import is resolved at build time by esbuild into the committed dist/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 ships src/ with exports: 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.ts exists 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.

<!-- gh-feedback:reply-to:46295 --> Not taking this. `@j4k/review` is a devDependency here and the deep import is resolved at build time by esbuild into the committed `dist/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 ships `src/` with `exports: 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.ts` exists 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.
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctYXBwcm9hY2gtbHVuYS0zIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5MzgzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjQ4Y2ZlYTc0LTZmMWYtNGQxOS1iODk3LTI2YzY2ZGUwNWZlYyJ9 -->
@ -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)));

rest is 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 bounded p-map/p-limit concurrency (or sequential page reads) for this collection.

`rest` is 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 bounded `p-map`/`p-limit` concurrency (or sequential page reads) for this collection.
Author
Owner

Fixed in f7d380f. The remaining review pages are fetched sequentially in a for loop instead of Promise.all over a server-derived page count, so a long review history cannot fan out into a request burst.

<!-- gh-feedback:reply-to:46299 --> Fixed in f7d380f. The remaining review pages are fetched sequentially in a `for` loop instead of `Promise.all` over a server-derived page count, so a long review history cannot fan out into a request burst.
jercik marked this conversation as resolved
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5MzgzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjU2NzQzOGZiLTkxYjItNGFjNS1hZTZjLWUwMDIyZTRhZWMzYSJ9 -->
README.md Outdated
@ -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() calls createIssueComment/editIssueComment. A read-only token will 403 on the only fork path, so the run cannot return the documented green fork-skip result. Document the required write permission or change the fork outcome to avoid a write.

🟡 **Medium:** This says fork PRs use a read-only Forge token and that read mode suffices, but `reconcileForkSkip()` calls `createIssueComment`/`editIssueComment`. A read-only token will 403 on the only fork path, so the run cannot return the documented green `fork-skip` result. Document the required write permission or change the fork outcome to avoid a write.
Author
Owner

The premise is wrong, checked against the Forgejo source rather than the docs page: GetActionRepoPermission gives a fork pull_request_target task token AccessModeRead, and CreateIssueComment requires 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, and canUserEditComment allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites GetActionRepoPermission and both rules so the pairing no longer reads as self-contradictory (f7d380f).

<!-- gh-feedback:reply-to:46326 --> The premise is wrong, checked against the Forgejo source rather than the docs page: `GetActionRepoPermission` gives a fork `pull_request_target` task token `AccessModeRead`, and `CreateIssueComment` requires 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, and `canUserEditComment` allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites `GetActionRepoPermission` and 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: editIssueComment goes through call(), which unconditionally parses the response body as JSON. Forgejo/Gitea permits this PATCH endpoint to return 204 No Content; an otherwise successful summary update then throws Unexpected end of JSON input and aborts reconciliation. Use a no-content-aware request path or accept an empty response here.

🟡 **Medium:** `editIssueComment` goes through `call()`, which unconditionally parses the response body as JSON. Forgejo/Gitea permits this PATCH endpoint to return `204 No Content`; an otherwise successful summary update then throws `Unexpected end of JSON input` and aborts reconciliation. Use a no-content-aware request path or accept an empty response here.
Author
Owner

Checked against the Forgejo source rather than inferred: editIssueComment in routers/api/v1/repo/issue_comment.go answers a content edit with ctx.JSON(http.StatusOK, convert.ToAPIComment(...)). The 204 path exists only for non-content comment types (the delete route and the label/assignee-style comments), which this endpoint never reaches with a body payload. So the PATCH used here always returns a JSON body and call() is correct; adding a no-content branch would be dead code.

<!-- gh-feedback:reply-to:46323 --> Checked against the Forgejo source rather than inferred: `editIssueComment` in `routers/api/v1/repo/issue_comment.go` answers a content edit with `ctx.JSON(http.StatusOK, convert.ToAPIComment(...))`. The `204` path exists only for non-content comment types (the delete route and the label/assignee-style comments), which this endpoint never reaches with a `body` payload. So the PATCH used here always returns a JSON body and `call()` 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, but unresolve() 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 a reopened disposition remains resolved. Check the returned resolver consistently.

🟡 **Medium:** `resolve()` verifies that the returned anchor has a resolver, but `unresolve()` 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 a `reopened` disposition remains resolved. Check the returned resolver consistently.
Author
Owner

Fixed in f7d380f. unresolve() now checks the returned anchor the same way resolve() does and throws reopening did not stick for comment <id> when resolver is still non-null. Verified against the live instance first: DELETE .../resolution returns 200 with the updated comment JSON whose resolver is null, so the check is testing a real field rather than an assumed shape. Recorder support added in src/reconcile/dispositions.test.ts.

<!-- gh-feedback:reply-to:46325 --> Fixed in f7d380f. `unresolve()` now checks the returned anchor the same way `resolve()` does and throws `reopening did not stick for comment <id>` when `resolver` is still non-null. Verified against the live instance first: `DELETE .../resolution` returns 200 with the updated comment JSON whose `resolver` is `null`, so the check is testing a real field rather than an assumed shape. Recorder support added in `src/reconcile/dispositions.test.ts`.
jercik marked this conversation as resolved
@ -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.

🟡 **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.
Author
Owner

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.

<!-- gh-feedback:reply-to:46324 --> 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_target payloads can have head.repo null when the source repository has been deleted. This dereference crashes before isFork can 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 reading full_name.

🟠 **High:** `pull_request_target` payloads can have `head.repo` null when the source repository has been deleted. This dereference crashes before `isFork` can 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 reading `full_name`.
Author
Owner

Fixed in f7d380f. PullRequestTargetEvent.head.repo is now typed { full_name: string } | null (matching RawPull in src/forge/client.ts) and the fork verdict reads isFork: (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 in src/wrapper/event-context.test.ts.

<!-- gh-feedback:reply-to:46321 --> Fixed in f7d380f. `PullRequestTargetEvent.head.repo` is now typed `{ full_name: string } | null` (matching `RawPull` in `src/forge/client.ts`) and the fork verdict reads `isFork: (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 in `src/wrapper/event-context.test.ts`.
jercik marked this conversation as resolved
@ -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.

🟠 **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.
Author
Owner

Not a defect: report, findings and claims are review-level by construction in the service contract (ADR 0021 and the wrapper spec) — GET /report and the findings/claims reads take no pass_id because 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 a pass_id on the report read, which is service-side work.

<!-- gh-feedback:reply-to:46322 --> Not a defect: report, findings and claims are review-level by construction in the service contract (ADR 0021 and the wrapper spec) — `GET /report` and the findings/claims reads take no `pass_id` because 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 a `pass_id` on the report read, which is service-side work.
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTIiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTkzODMiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiYmNhNDM5YzAtZThiNi00Zjc3LTkxNDktM2ZkN2FjMWZiMTRlIn0= -->
README.md Outdated
@ -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 without REVIEW_CAPABILITY_TOKEN. Clarify that the capability token is required only for the same-repository service path.

🟢 **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 without `REVIEW_CAPABILITY_TOKEN`. Clarify that the capability token is required only for the same-repository service path.
Author
Owner

Fixed in f7d380f. The README's environment contract now splits the requirement per path: the forge/runner variables are required on every path, while REVIEW_SERVICE_URL and REVIEW_CAPABILITY_TOKEN are required only on the same-repository path, because a fork pull request returns at the gate without opening a service session.

<!-- gh-feedback:reply-to:46337 --> Fixed in f7d380f. The README's environment contract now splits the requirement per path: the forge/runner variables are required on every path, while `REVIEW_SERVICE_URL` and `REVIEW_CAPABILITY_TOKEN` are required only on the same-repository path, because a fork pull request returns at the gate without opening a service session.
jercik marked this conversation as resolved
@ -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-Count as zero and silently returns only page 1. listReviews feeds 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.

🟡 **Medium:** Review pagination treats a missing or invalid `X-Total-Count` as zero and silently returns only page 1. `listReviews` feeds 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.
Author
Owner

Already fixed before this round, in 7abe25e: listReviews now fails closed — an absent or non-finite X-Total-Count throws incomplete review listing for #<n>: unusable X-Total-Count ... instead of truncating to page 1. (f7d380f additionally made the remaining pages sequential.)

<!-- gh-feedback:reply-to:46335 --> Already fixed before this round, in 7abe25e: `listReviews` now fails closed — an absent or non-finite `X-Total-Count` throws `incomplete review listing for #<n>: unusable X-Total-Count ...` instead of truncating to page 1. (f7d380f additionally made the remaining pages sequential.)
jercik marked this conversation as resolved
@ -0,0 +15,4 @@
type DispositionRecord = NonNullable<ProjectedClaim["disposition"]["current"]>;
function claimIdOf(conversation: Conversation): string | undefined {

🟠 High: claimIdOf returns only one claim from a conversation. inline.ts batches all findings into one Forge review, so two findings on the same path/display line share the review id and are grouped by deriveConversations; 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.

🟠 **High:** `claimIdOf` returns only one claim from a conversation. `inline.ts` batches all findings into one Forge review, so two findings on the same path/display line share the review id and are grouped by `deriveConversations`; 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.
Author
Owner

Already fixed before this round, in 7abe25e: dispositions no longer key on a single claimIdOf per 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.

<!-- gh-feedback:reply-to:46334 --> Already fixed before this round, in 7abe25e: dispositions no longer key on a single `claimIdOf` per 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.
jercik marked this conversation as resolved
@ -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 getPull returns and before reconcile performs its summary, inline, and disposition writes. The old run then posts findings for the previous head onto the new head despite the head-moved rule. Guard mutations with an expected-head/conditional check or otherwise revalidate ownership at the write boundary.

🟡 **Medium:** The head check is a time-of-check/time-of-use race: the PR can synchronize after `getPull` returns and before `reconcile` performs its summary, inline, and disposition writes. The old run then posts findings for the previous head onto the new head despite the `head-moved` rule. Guard mutations with an expected-head/conditional check or otherwise revalidate ownership at the write boundary.
Author
Owner

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.

<!-- gh-feedback:reply-to:46336 --> 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 the try/catch, so the intended green fork-skip result never completes. Grant fork runs issue-comment write permission, or make the fork path no-write and handle the failure explicitly.

🔴 **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 the `try`/`catch`, so the intended green `fork-skip` result never completes. Grant fork runs issue-comment write permission, or make the fork path no-write and handle the failure explicitly.
Author
Owner

Half agreed and fixed in f7d380f: the fork branch is now inside the try, so any failure of reconcileForkSkip reports 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 AccessModeRead from GetActionRepoPermission, 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.

<!-- gh-feedback:reply-to:46332 --> Half agreed and fixed in f7d380f: the fork branch is now inside the `try`, so any failure of `reconcileForkSkip` reports 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 `AccessModeRead` from `GetActionRepoPermission`, 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.
jercik marked this conversation as resolved
@ -0,0 +81,4 @@
graceDeadlineAt: number,
): Promise<GroundingOutcome> {
const outcome = await poll<SettleOutcome | "regressed">(clock, graceDeadlineAt, async () => {
if (!(await hasPendingGrounding(session, reviewId))) {

🟠 High: When hasPendingGrounding returns false, grounding polling returns settled without calling readSettle. If triage regresses from settled to due or stalled while 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.

🟠 **High:** When `hasPendingGrounding` returns false, grounding polling returns `settled` without calling `readSettle`. If triage regresses from `settled` to `due` or `stalled` while 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.
Author
Owner

Fixed in f7d380f. src/wrapper/wait.ts now re-reads settle state on every grounding cycle before consulting the pending list: readSettle runs first, stalled and a regressed/absent pass short-circuit, and only then does an empty pending list count as settled. Two new tests cover a pass that moved on and a triage regression during the final grounding probe.

<!-- gh-feedback:reply-to:46333 --> Fixed in f7d380f. `src/wrapper/wait.ts` now re-reads settle state on every grounding cycle *before* consulting the pending list: `readSettle` runs first, `stalled` and a regressed/absent pass short-circuit, and only then does an empty pending list count as `settled`. Two new tests cover a pass that moved on and a triage regression during the final grounding probe.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 2 high issues and 1 medium issue.

Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)

**Summary:** Found 2 high issues and 1 medium issue. _Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5MzgzIiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjQ4NDRlZWE2LThlZTEtNGJhOS04ODJiLWQ3YmYzNWE2NDU4YSJ9 -->
@ -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: extraLines is 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 when extra_lines_count + 1 exceeds UI.MAX_CODE_COMMENT_LINES (50 by default). Because all findings are sent in one createInlineReview, 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.

🟠 **High:** `extraLines` is 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 when `extra_lines_count + 1` exceeds `UI.MAX_CODE_COMMENT_LINES` (50 by default). Because all findings are sent in one `createInlineReview`, 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.
Author
Owner

Fixed in f7d380f. src/reconcile/anchor-map.ts now caps a snippet range at MAX_COMMENT_LINES = 50 (Forgejo's setting.UI.MaxCodeCommentLines, enforced by ValidateCodeCommentLineRange): the range is end-anchored on the last covered line and the start is pulled forward to end - 49, so extra_lines_count can never exceed the instance limit and one long anchor can no longer 422 the whole batched review.

<!-- gh-feedback:reply-to:46341 --> Fixed in f7d380f. `src/reconcile/anchor-map.ts` now caps a snippet range at `MAX_COMMENT_LINES = 50` (Forgejo's `setting.UI.MaxCodeCommentLines`, enforced by `ValidateCodeCommentLineRange`): the range is end-anchored on the last covered line and the start is pulled forward to `end - 49`, so `extra_lines_count` can never exceed the instance limit and one long anchor can no longer 422 the whole batched review.
jercik marked this conversation as resolved
@ -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 old fixed marker makes this branch skip the new reply, so the PR thread displays stale disposition details even though resolution state changes. Include current.id or content_key in the marker/deduplication key so only the exact record is suppressed.

🟡 **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 old `fixed` marker makes this branch skip the new reply, so the PR thread displays stale disposition details even though resolution state changes. Include `current.id` or `content_key` in the marker/deduplication key so only the exact record is suppressed.
Author
Owner

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 earlier fixed marker.

<!-- gh-feedback:reply-to:46343 --> 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 earlier `fixed` marker.
jercik marked this conversation as resolved
@ -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: findOurComment trusts 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 in IssueComment and only reuse comments owned by the automation identity; otherwise create a new summary comment.

🟠 **High:** `findOurComment` trusts 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 in `IssueComment` and only reuse comments owned by the automation identity; otherwise create a new summary comment.
Author
Owner

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 IssueComment would add a field the wrapper cannot authenticate against an actions identity in a way an owner cannot also produce.

<!-- gh-feedback:reply-to:46342 --> 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 `IssueComment` would add a field the wrapper cannot authenticate against an actions identity in a way an owner cannot also produce.
forgejo-actions left a comment

Summary: Found 2 high and 1 medium correctness/integration issues.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 2 high and 1 medium correctness/integration issues. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6IjZlMDZiYzk3ZmZiMzZiZmQ0MDk5OTA1YWFiZDAzNDQ4YWY2YThmNzMiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTMiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTkzODMiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiYjcwOTlhZjEtZjBiOS00OWEwLTg5NmUtZjMyZDUzMTc5NDk3In0= -->
@ -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 gives FORGEJO_TOKEN read-only permissions for fork pull_request_target runs, so this returns 403 and the promised green fork-skip path 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.

🟠 **High:** The fork branch still calls `upsert`, which POSTs a new issue comment or PATCHes an existing one. Forgejo gives `FORGEJO_TOKEN` read-only permissions for fork `pull_request_target` runs, so this returns 403 and the promised green `fork-skip` path 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.
Author
Owner

The premise is wrong, checked against the Forgejo source rather than the docs page: GetActionRepoPermission gives a fork pull_request_target task token AccessModeRead, and CreateIssueComment requires 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, and canUserEditComment allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites GetActionRepoPermission and both rules so the pairing no longer reads as self-contradictory (f7d380f).

<!-- gh-feedback:reply-to:46347 --> The premise is wrong, checked against the Forgejo source rather than the docs page: `GetActionRepoPermission` gives a fork `pull_request_target` task token `AccessModeRead`, and `CreateIssueComment` requires 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, and `canUserEditComment` allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites `GetActionRepoPermission` and 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_name is dereferenced before isFork is computed. head.repo can 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 with pull.head.repo?.full_name ?? "".

🟠 **High:** `pull.head.repo.full_name` is dereferenced before `isFork` is computed. `head.repo` can 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 with `pull.head.repo?.full_name ?? ""`.
Author
Owner

Fixed in f7d380f. PullRequestTargetEvent.head.repo is now typed { full_name: string } | null (matching RawPull in src/forge/client.ts) and the fork verdict reads isFork: (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 in src/wrapper/event-context.test.ts.

<!-- gh-feedback:reply-to:46348 --> Fixed in f7d380f. `PullRequestTargetEvent.head.repo` is now typed `{ full_name: string } | null` (matching `RawPull` in `src/forge/client.ts`) and the fork verdict reads `isFork: (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 in `src/wrapper/event-context.test.ts`.
jercik marked this conversation as resolved
@ -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 classify and reconcile perform 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.

🟡 **Medium:** The head-moved check is a one-time snapshot, but `classify` and `reconcile` perform 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.
Author
Owner

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.

<!-- gh-feedback:reply-to:46349 --> 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.
fix: close the reconciliation gaps found in pre-handoff review
Some checks failed
commit-msg / commitlint (pull_request) Successful in 31s
Checks / quality-checks (pull_request) Failing after 42s
Dedupe check / dedupe-check (pull_request) Successful in 45s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m4s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-luna generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-code-luna-2 generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-code-luna-3 generator (pull_request_target) Has been cancelled
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Has been cancelled
PR Review / Dispatch and observe exact review writers (pull_request_target) Has been cancelled
7abe25e7b6
The superseded sweep replied to every prior thread on every new review, so a
finding surviving N pushes collected N replies. It now labels a thread once —
only while the thread is open and carries no superseded label from any review.

A disposition marker keyed by kind and claim id alone hid the second half of a
fix, reopen, fix cycle: the stale marker suppressed the new rationale while the
thread was resolved anyway. The marker now carries the disposition record id.

Two claims whose anchors land on one display line share one Forgejo
conversation. Only the first claim's disposition was projected; a reopened
sibling could sit under a resolved thread. Every claim marker in a conversation
is now projected, and a reopened claim wins the resolution verdict.

listReviews truncated to page one when X-Total-Count was missing or garbage,
which silently blinds every marker-based dedupe. It throws now, like the issue
comment listing.

Resolve and unresolve degraded to reply-only on 404 but aborted the run on 403
— the shape a task token without the fork's resolution permission produces.
Both statuses degrade now, off the error's own fields rather than a substring
match on its message.

Also records what row 11 does not buy: the report read takes no pass id, so a
mid-wait supersession renders the newer pass in a summary whose exit code
follows the pinned one.
fix: make the committed bundle byte-identical across machines
All checks were successful
commit-msg / commitlint (pull_request) Successful in 25s
Dedupe check / dedupe-check (pull_request) Successful in 47s
Checks / quality-checks (pull_request) Successful in 58s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m26s
PR Review / forgejo-review-approach-luna-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 9m2s
PR Review / forgejo-review-code-luna generator (pull_request_target) Successful in 11m21s
PR Review / forgejo-review-code-luna-3 generator (pull_request_target) Successful in 12m31s
PR Review / forgejo-review-code-luna-2 generator (pull_request_target) Successful in 15m0s
PR Review / Dispatch and observe exact review writers (pull_request_target) Successful in 15m22s
b9c4ce2861
esbuild labels each bundled module with its path relative to the outfile. For
dependencies pnpm resolves through its content-addressed store that path is the
store location, which differs per machine, so the committed artifact both leaked
the build host's layout and could never satisfy the freshness test in the
environment that did not produce it.

The build script now normalizes those comments to node_modules/<pkg>/... before
writing, and the freshness test applies the same function to its in-memory build,
so local and CI compare identical bytes while a source change still moves them.
forgejo-actions left a comment

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; the pull_request_target payload 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, intersects accepts 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_modules is 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)

**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; the `pull_request_target` payload 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, `intersects` accepts 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_modules` is 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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImI5YzRjZTI4NjFjZTgyNjVmY2M5ZTk5MTFkN2M1NTI3MjVjOGU3NTkiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5NDI4Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImY0OTcwN2Q2LWZhNzUtNDI1My04N2QxLWUwNGIxNmYyZTAzZSJ9 -->
README.md Outdated
@ -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 for pull_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.

🟡 **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 for `pull_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.
Author
Owner

The premise is wrong, checked against the Forgejo source rather than the docs page: GetActionRepoPermission gives a fork pull_request_target task token AccessModeRead, and CreateIssueComment requires 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, and canUserEditComment allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites GetActionRepoPermission and both rules so the pairing no longer reads as self-contradictory (f7d380f).

<!-- gh-feedback:reply-to:46410 --> The premise is wrong, checked against the Forgejo source rather than the docs page: `GetActionRepoPermission` gives a fork `pull_request_target` task token `AccessModeRead`, and `CreateIssueComment` requires 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, and `canUserEditComment` allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites `GetActionRepoPermission` and 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 node26 while action.yml declares runs.using: node24, so the runtime executing dist/index.mjs is 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. Set target: "node24" to match the declared runtime (.node-version/engines can stay at 26 for the toolchain).

🟢 **Low:** the bundle targets `node26` while `action.yml` declares `runs.using: node24`, so the runtime executing `dist/index.mjs` is 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. Set `target: "node24"` to match the declared runtime (`.node-version`/`engines` can stay at 26 for the toolchain).
Author
Owner

Fixed in f7d380f. src/bundle-options.ts now sets target: "node24", with a comment naming action.yml's runs.using: node24 as the reason; the toolchain's own Node version is unaffected.

<!-- gh-feedback:reply-to:46413 --> Fixed in f7d380f. `src/bundle-options.ts` now sets `target: "node24"`, with a comment naming `action.yml`'s `runs.using: node24` as the reason; the toolchain's own Node version is unaffected.
jercik marked this conversation as resolved
@ -0,0 +92,4 @@
query: {
after: string | undefined;
limit: number | undefined;
destination: string | undefined;

🟢 Low: destination is typed string, so the only caller's literal ("grounding-pending" in src/wrapper/wait.ts) is unchecked — a typo would silently query unfiltered claims and make hasPendingGrounding always true, i.e. every run ends grounding-pending. Destination is already re-exported from src/review/api.ts and otherwise unused; use it here.

🟢 **Low:** `destination` is typed `string`, so the only caller's literal (`"grounding-pending"` in `src/wrapper/wait.ts`) is unchecked — a typo would silently query unfiltered claims and make `hasPendingGrounding` always true, i.e. every run ends `grounding-pending`. `Destination` is already re-exported from `src/review/api.ts` and otherwise unused; use it here.
Author
Owner

Fixed in f7d380f. ListClaimsQuery.destination in src/contract/types.ts is now Destination | undefined (the type is imported alongside the others), so the "grounding-pending" literal in src/wrapper/wait.ts is checked at compile time.

<!-- gh-feedback:reply-to:46415 --> Fixed in f7d380f. `ListClaimsQuery.destination` in `src/contract/types.ts` is now `Destination | undefined` (the type is imported alongside the others), so the `"grounding-pending"` literal in `src/wrapper/wait.ts` is checked at compile time.
jercik marked this conversation as resolved
@ -0,0 +97,4 @@
return fetched.text;
},
getRawFile: async (path, ref) => {
const segments = path.split("/").map((segment) => encodeURIComponent(segment));

🟡 Medium: encodeURIComponent does not escape ., so a claim anchor path of ../../../../admin/users survives segment encoding and the WHATWG URL parser then resolves the dot segments away:

.../repos/j4k/review-wrapper/raw/../../../../admin/users?ref=main
→ https://code.j4k.dev/api/v1/admin/users?ref=main

The anchor path is service-supplied (ultimately model-derived), so an authenticated GET carrying FORGEJO_TOKEN can be steered to an arbitrary endpoint under GITHUB_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 with new URL() and verify the resulting pathname still starts with the repo raw prefix).

🟡 **Medium:** `encodeURIComponent` does not escape `.`, so a claim anchor path of `../../../../admin/users` survives segment encoding and the WHATWG URL parser then resolves the dot segments away: ``` .../repos/j4k/review-wrapper/raw/../../../../admin/users?ref=main → https://code.j4k.dev/api/v1/admin/users?ref=main ``` The anchor path is service-supplied (ultimately model-derived), so an authenticated GET carrying `FORGEJO_TOKEN` can be steered to an arbitrary endpoint under `GITHUB_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 with `new URL()` and verify the resulting pathname still starts with the repo raw prefix).
Author
Owner

Fixed in f7d380f. getRawFile now rejects any path with an empty, . or .. segment before building the URL (a warning, then undefined — 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 in src/forge/client.test.ts.

<!-- gh-feedback:reply-to:46406 --> Fixed in f7d380f. `getRawFile` now rejects any path with an empty, `.` or `..` segment before building the URL (a warning, then `undefined` — 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 in `src/forge/client.test.ts`.
jercik marked this conversation as resolved
@ -0,0 +94,4 @@
return undefined;
}
const located = locate(fileText, anchor.snippet);
if (located === undefined || !intersects(covered, located.start, located.span)) {

🟡 Medium: intersects accepts an overlap anywhere in the located range, but by this module's own display-line rule (AnchorPlacement.extraLines doc and src/forge/conversations.ts) Forgejo buckets the thread at new_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 with new_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.

🟡 **Medium:** `intersects` accepts an overlap anywhere in the located range, but by this module's own display-line rule (`AnchorPlacement.extraLines` doc and `src/forge/conversations.ts`) Forgejo buckets the thread at `new_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 with `new_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.
Author
Owner

Fixed in f7d380f. The intersects check 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 in src/reconcile/anchor-map.test.ts.

<!-- gh-feedback:reply-to:46408 --> Fixed in f7d380f. The `intersects` check 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 in `src/reconcile/anchor-map.test.ts`.
jercik marked this conversation as resolved
@ -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 IssueComment does not carry the author at all. A PR author can post a comment whose body starts with <!-- review:summary --> before the run; upsert then 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's user through IssueComment and matching on the wrapper's own identity closes it.

🟢 **Low:** the marker match is position-anchored but not author-anchored, and `IssueComment` does not carry the author at all. A PR author can post a comment whose body starts with `<!-- review:summary -->` before the run; `upsert` then 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's `user` through `IssueComment` and matching on the wrapper's own identity closes it.
Author
Owner

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 IssueComment would add a field the wrapper cannot authenticate against an actions identity in a way an owner cannot also produce.

<!-- gh-feedback:reply-to:46414 --> 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 `IssueComment` would 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.repo is nullable when the fork repo has been deleted — src/forge/client.ts types it { full_name: string } | null and maps it to "", and there is a test for it. On this path the payload is only as-cast, so the same PR read from the event payload throws TypeError: Cannot read properties of null (reading 'full_name') instead of deriving isFork: true. Since nothing catches it (see the src/main.ts note), 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 a workflow_dispatch payload without an inputs object throws a TypeError rather than the intended pr_number message.

🟡 **Medium:** `pull.head.repo` is nullable when the fork repo has been deleted — `src/forge/client.ts` types it `{ full_name: string } | null` and maps it to `""`, and there is a test for it. On this path the payload is only `as`-cast, so the same PR read from the event payload throws `TypeError: Cannot read properties of null (reading 'full_name')` instead of deriving `isFork: true`. Since nothing catches it (see the `src/main.ts` note), 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 a `workflow_dispatch` payload without an `inputs` object throws a `TypeError` rather than the intended `pr_number` message.
Author
Owner

Fixed in f7d380f. PullRequestTargetEvent.head.repo is now typed { full_name: string } | null (matching RawPull in src/forge/client.ts) and the fork verdict reads isFork: (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 in src/wrapper/event-context.test.ts.

The second half is fixed too: workflow_dispatch payloads go through parsePrNumber(payload.inputs?.pr_number), which rejects a missing inputs object, a non-integer, and a non-positive number with the intended message instead of a TypeError.

<!-- gh-feedback:reply-to:46407 --> Fixed in f7d380f. `PullRequestTargetEvent.head.repo` is now typed `{ full_name: string } | null` (matching `RawPull` in `src/forge/client.ts`) and the fork verdict reads `isFork: (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 in `src/wrapper/event-context.test.ts`. The second half is fixed too: `workflow_dispatch` payloads go through `parsePrNumber(payload.inputs?.pr_number)`, which rejects a missing `inputs` object, a non-integer, and a non-positive number with the intended message instead of a `TypeError`.
jercik marked this conversation as resolved
@ -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). hasPendingGrounding in src/wrapper/wait.ts has the same shape. Bound the page count, or stop when page.next_after === after.

🟢 **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). `hasPendingGrounding` in `src/wrapper/wait.ts` has the same shape. Bound the page count, or stop when `page.next_after === after`.
Author
Owner

Fixed in f7d380f. Both cursor walks are now one function, walkClaims in src/review/claims-walk.ts, which throws claims cursor did not advance past <cursor> when the service returns the same next_after twice. A non-advancing cursor is a service defect, so it fails loudly rather than paging forever.

<!-- gh-feedback:reply-to:46411 --> Fixed in f7d380f. Both cursor walks are now one function, `walkClaims` in `src/review/claims-walk.ts`, which throws `claims cursor did not advance past <cursor>` when the service returns the same `next_after` twice. A non-advancing cursor is a service defect, so it fails loudly rather than paging forever.
jercik marked this conversation as resolved
@ -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.ts deliberately leaves transient tolerance to the wait loop, but these three reads run after it, and per session.test.ts a 5xx on a read is rethrown unchanged. One transient blip on GET /report after a 45-minute wait discards the whole reconciliation and turns a green outcome red, with nothing written. Consider reusing the bounded ladder from withRetries for the reads inside reconcile.

🟢 **Low:** the reconcile reads carry no retry — `session.ts` deliberately leaves transient tolerance to the wait loop, but these three reads run *after* it, and per `session.test.ts` a 5xx on a read is rethrown unchanged. One transient blip on `GET /report` after a 45-minute wait discards the whole reconciliation and turns a green outcome red, with nothing written. Consider reusing the bounded ladder from `withRetries` for the reads inside `reconcile`.
Author
Owner

Deliberate placement, not an oversight. withRetries belongs 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 on GET /report is 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.

<!-- gh-feedback:reply-to:46412 --> Deliberate placement, not an oversight. `withRetries` belongs 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 on `GET /report` is 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 of reconcileForkSkip (403, forge 5xx, network) escapes runWrapper and — since src/main.ts awaits 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 the FAILURE_REMEDIES text. 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.

🟡 **Medium:** the fork branch sits outside the `try`, so any failure of `reconcileForkSkip` (403, forge 5xx, network) escapes `runWrapper` and — since `src/main.ts` awaits 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 the `FAILURE_REMEDIES` text. 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.
Author
Owner

Fixed in f7d380f — the fork branch now sits inside the try, 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 before deps.openSession, which is the actual security boundary.

On the 403 premise: a fork pull_request_target task token is AccessModeRead (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.

<!-- gh-feedback:reply-to:46409 --> Fixed in f7d380f — the fork branch now sits inside the `try`, 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 before `deps.openSession`, which is the actual security boundary. On the 403 premise: a fork `pull_request_target` task token is `AccessModeRead` (`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.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 2 high, 1 medium, and 1 low issue.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 2 high, 1 medium, and 1 low issue. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImI5YzRjZTI4NjFjZTgyNjVmY2M5ZTk5MTFkN2M1NTI3MjVjOGU3NTkiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5NDI4Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjljMzRjMmNiLWYzMjktNGM4Zi1hNmM5LWUxYmJhMjM1MjU5ZiJ9 -->
README.md Outdated
@ -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_URL or REVIEW_CAPABILITY_TOKEN, and only need Forge credentials to attempt the skip comment. Document the required variables separately for fork and same-repository paths.

🟢 **Low:** “Every variable is required” contradicts the fork-skip implementation: fork runs intentionally never read `REVIEW_SERVICE_URL` or `REVIEW_CAPABILITY_TOKEN`, and only need Forge credentials to attempt the skip comment. Document the required variables separately for fork and same-repository paths.
Author
Owner

Fixed in f7d380f. The README's environment contract now splits the requirement per path: the forge/runner variables are required on every path, while REVIEW_SERVICE_URL and REVIEW_CAPABILITY_TOKEN are required only on the same-repository path, because a fork pull request returns at the gate without opening a service session.

<!-- gh-feedback:reply-to:46426 --> Fixed in f7d380f. The README's environment contract now splits the requirement per path: the forge/runner variables are required on every path, while `REVIEW_SERVICE_URL` and `REVIEW_CAPABILITY_TOKEN` are required only on the same-repository path, because a fork pull request returns at the gate without opening a service session.
jercik marked this conversation as resolved
@ -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 in X-Total-Count (API pagination). Once a PR has more comments than the default page, distinct !== total throws, so summary and fork reconciliation fail instead of finding the marker. Page through this endpoint, as listReviews does, before checking completeness.

🟠 **High:** This performs one request without `page`/`limit`, but Forgejo list endpoints paginate and expose the full size in `X-Total-Count` ([API pagination](https://forgejo.org/docs/latest/user/api-usage/)). Once a PR has more comments than the default page, `distinct !== total` throws, so summary and fork reconciliation fail instead of finding the marker. Page through this endpoint, as `listReviews` does, before checking completeness.
Author
Owner

Checked against the Forgejo source: ListIssueComments in routers/api/v1/repo/issue_comment.go builds its FindCommentsOptions with no ListOptions, so the endpoint ignores page/limit and returns every comment in one response — which is why the client asserts distinct-ids == X-Total-Count instead of paging. The generic pagination docs describe the endpoints that set ListOptions; this one does not. Paging it would add requests that return the same full set each time.

<!-- gh-feedback:reply-to:46423 --> Checked against the Forgejo source: `ListIssueComments` in `routers/api/v1/repo/issue_comment.go` builds its `FindCommentsOptions` with no `ListOptions`, so the endpoint ignores `page`/`limit` and returns every comment in one response — which is why the client asserts distinct-ids == `X-Total-Count` instead of paging. The generic pagination docs describe the endpoints that set `ListOptions`; 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.

🟡 **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.
Author
Owner

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.

<!-- gh-feedback:reply-to:46425 --> 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 fork FORGEJO_TOKEN as 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.

🟠 **High:** The fork gate always calls `reconcileForkSkip`, which can POST or PATCH an issue comment. The security model documents the fork `FORGEJO_TOKEN` as read-only, but Forgejo requires the write issue scope for those operations ([token scopes](https://forgejo.org/docs/latest/user/token-scope/)); 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.
Author
Owner

The premise is wrong, checked against the Forgejo source rather than the docs page: GetActionRepoPermission gives a fork pull_request_target task token AccessModeRead, and CreateIssueComment requires 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, and canUserEditComment allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites GetActionRepoPermission and both rules so the pairing no longer reads as self-contradictory (f7d380f).

<!-- gh-feedback:reply-to:46424 --> The premise is wrong, checked against the Forgejo source rather than the docs page: `GetActionRepoPermission` gives a fork `pull_request_target` task token `AccessModeRead`, and `CreateIssueComment` requires 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, and `canUserEditComment` allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites `GetActionRepoPermission` and both rules so the pairing no longer reads as self-contradictory (f7d380f).
forgejo-actions left a comment

Summary: Found 3 issues (2 high, 1 medium).

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 3 issues (2 high, 1 medium). _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImI5YzRjZTI4NjFjZTgyNjVmY2M5ZTk5MTFkN2M1NTI3MjVjOGU3NTkiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTMiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTk0MjgiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiODg5NWE5MzUtZDViNi00ZmNmLWI1YTUtMmE5NGFjZDZlMmM2In0= -->
@ -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_target payloads can have head.repo: null after 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).

🟠 **High:** `pull_request_target` payloads can have `head.repo: null` after 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).
Author
Owner

Fixed in f7d380f. PullRequestTargetEvent.head.repo is now typed { full_name: string } | null (matching RawPull in src/forge/client.ts) and the fork verdict reads isFork: (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 in src/wrapper/event-context.test.ts.

<!-- gh-feedback:reply-to:46429 --> Fixed in f7d380f. `PullRequestTargetEvent.head.repo` is now typed `{ full_name: string } | null` (matching `RawPull` in `src/forge/client.ts`) and the fork verdict reads `isFork: (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 in `src/wrapper/event-context.test.ts`.
jercik marked this conversation as resolved
@ -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, but reconcileForkSkip can 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 returning fork-skip. Grant the token write:issue on the base repository or make the fork path truly read-only and remove the marker-write requirement.

🟠 **High:** The documented fork path uses a read-only `FORGEJO_TOKEN`, but `reconcileForkSkip` can 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 returning `fork-skip`. Grant the token `write:issue` on the base repository or make the fork path truly read-only and remove the marker-write requirement.
Author
Owner

The premise is wrong, checked against the Forgejo source rather than the docs page: GetActionRepoPermission gives a fork pull_request_target task token AccessModeRead, and CreateIssueComment requires 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, and canUserEditComment allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites GetActionRepoPermission and both rules so the pairing no longer reads as self-contradictory (f7d380f).

<!-- gh-feedback:reply-to:46430 --> The premise is wrong, checked against the Forgejo source rather than the docs page: `GetActionRepoPermission` gives a fork `pull_request_target` task token `AccessModeRead`, and `CreateIssueComment` requires 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, and `canUserEditComment` allows the poster regardless of unit mode. So row 1 completes green under the documented read-only token. The README bullet now cites `GetActionRepoPermission` and 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 hasPendingGrounding is false, this branch returns settled without calling readSettle again. If a newer (including vacuous/no-claim) pass starts during that final probe, latest_pass and a triage regression are never observed, leaving superseded false despite the row-11 contract. Re-read settle state before accepting the no-pending result.

🟡 **Medium:** When `hasPendingGrounding` is false, this branch returns `settled` without calling `readSettle` again. If a newer (including vacuous/no-claim) pass starts during that final probe, `latest_pass` and a triage regression are never observed, leaving `superseded` false despite the row-11 contract. Re-read settle state before accepting the no-pending result.
Author
Owner

Fixed in f7d380f. src/wrapper/wait.ts now re-reads settle state on every grounding cycle before consulting the pending list: readSettle runs first, stalled and a regressed/absent pass short-circuit, and only then does an empty pending list count as settled. Two new tests cover a pass that moved on and a triage regression during the final grounding probe.

<!-- gh-feedback:reply-to:46431 --> Fixed in f7d380f. `src/wrapper/wait.ts` now re-reads settle state on every grounding cycle *before* consulting the pending list: `readSettle` runs first, `stalled` and a regressed/absent pass short-circuit, and only then does an empty pending list count as `settled`. Two new tests cover a pass that moved on and a triage regression during the final grounding probe.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 2 medium issues.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 2 medium issues. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImI5YzRjZTI4NjFjZTgyNjVmY2M5ZTk5MTFkN2M1NTI3MjVjOGU3NTkiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTIiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTk0MjgiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiMjExYmRjOWUtODVkOC00OWM5LWFjZjctOWViMGI0OTE0MmIxIn0= -->
@ -0,0 +68,4 @@
path: placement.path,
body: commentBody(claim, reviewId),
new_position: placement.newPosition,
extra_lines_count: placement.extraLines,

🟡 Medium: placement.extraLines is passed straight through as extra_lines_count, but no Forgejo line-range limit is enforced. A valid service snippet can span more than the instance's MAX_CODE_COMMENT_LINES (50 by default), causing this single batched createInlineReview request 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.

🟡 **Medium:** `placement.extraLines` is passed straight through as `extra_lines_count`, but no Forgejo line-range limit is enforced. A valid service snippet can span more than the instance's `MAX_CODE_COMMENT_LINES` (50 by default), causing this single batched `createInlineReview` request 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.
Author
Owner

Fixed in f7d380f. src/reconcile/anchor-map.ts now caps a snippet range at MAX_COMMENT_LINES = 50 (Forgejo's setting.UI.MaxCodeCommentLines, enforced by ValidateCodeCommentLineRange): the range is end-anchored on the last covered line and the start is pulled forward to end - 49, so extra_lines_count can never exceed the instance limit and one long anchor can no longer 422 the whole batched review.

<!-- gh-feedback:reply-to:46433 --> Fixed in f7d380f. `src/reconcile/anchor-map.ts` now caps a snippet range at `MAX_COMMENT_LINES = 50` (Forgejo's `setting.UI.MaxCodeCommentLines`, enforced by `ValidateCodeCommentLineRange`): the range is end-anchored on the last covered line and the start is pulled forward to `end - 49`, so `extra_lines_count` can never exceed the instance limit and one long anchor can no longer 422 the whole batched review.
jercik marked this conversation as resolved
@ -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, reconcile fetches current report/diff/file data and then writes the summary, inline review, and disposition replies using inputs.headSha; if the PR synchronizes in that window, stale findings can be written to the new head despite the documented head-moved "no writes" outcome. Recheck the head immediately before each write phase or make writes conditional on the expected SHA.

🟡 **Medium:** This is only a snapshot check. Once it returns, `reconcile` fetches current report/diff/file data and then writes the summary, inline review, and disposition replies using `inputs.headSha`; if the PR synchronizes in that window, stale findings can be written to the new head despite the documented `head-moved` "no writes" outcome. Recheck the head immediately before each write phase or make writes conditional on the expected SHA.
Author
Owner

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.

<!-- gh-feedback:reply-to:46434 --> 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.
fix: close the defects the second review round found
All checks were successful
commit-msg / commitlint (pull_request) Successful in 22s
Dedupe check / dedupe-check (pull_request) Successful in 42s
Checks / quality-checks (pull_request) Successful in 54s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 1m29s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 9m11s
PR Review / forgejo-review-code-luna generator (pull_request_target) Successful in 15m28s
PR Review / forgejo-review-code-luna-2 generator (pull_request_target) Successful in 16m22s
PR Review / forgejo-review-code-luna-3 generator (pull_request_target) Successful in 16m39s
PR Review / Dispatch and observe exact review writers (pull_request_target) Successful in 16m59s
f7d380fd28
Treats a deleted head repository as a fork instead of dereferencing null,
and rejects a workflow_dispatch payload with no inputs object with the
documented message.

Anchors a snippet comment on the last covered line of its range and caps
the range at the forge's 50-line code-comment limit, so one long anchor
can no longer fail the whole batched inline review.

Refuses a raw-file path whose segments could resolve out of the repo's
raw route, pages the review listing sequentially, verifies that a reopen
stuck before reporting success, runs the fork gate inside the run's error
boundary, re-reads triage state before accepting an empty pending-claims
list, and bounds the claims walk on a cursor that stops advancing.

The bundle now targets node24, the runtime action.yml declares.
Author
Owner

Replying to review #10801

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, and src/review/api.ts is the deliberate one-file migration seam for the stable client entry point being prepared in the service repository. No change from this review.

> Replying to review #10801 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, and `src/review/api.ts` is the deliberate one-file migration seam for the stable client entry point being prepared in the service repository. No change from this review.
Author
Owner

Replying to review #10803

Acknowledged — no findings. The lazy review-session thunk this review calls out is the fork credential boundary and stays as-is.

> Replying to review #10803 Acknowledged — no findings. The lazy review-session thunk this review calls out is the fork credential boundary and stays as-is.
Author
Owner

Replying to review #10804

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.

> Replying to review #10804 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.
Author
Owner

Replying to review #10806

Acknowledged. Its inline finding (#46299, unbounded review-page fan-out) is fixed in f7d380f — the remaining pages are now fetched sequentially.

> Replying to review #10806 Acknowledged. Its inline finding (#46299, unbounded review-page fan-out) is fixed in f7d380f — the remaining pages are now fetched sequentially.
Author
Owner

Replying to review #10814

Acknowledged; all six inline findings are answered on their threads. Fixed in f7d380f: #46321 (null head.repo) and #46325 (unresolve now verifies the returned resolver). Declined with evidence: #46322 (report/findings/claims are review-level by contract), #46323 (Forgejo's editIssueComment returns 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 is AccessModeRead and comment creation needs only issues-read — README clarified in f7d380f).

> Replying to review #10814 Acknowledged; all six inline findings are answered on their threads. Fixed in f7d380f: #46321 (null `head.repo`) and #46325 (unresolve now verifies the returned `resolver`). Declined with evidence: #46322 (report/findings/claims are review-level by contract), #46323 (Forgejo's `editIssueComment` returns 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 is `AccessModeRead` and comment creation needs only issues-read — README clarified in f7d380f).
Author
Owner

Replying to review #10816

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 in 7abe25e: #46334, #46335. Declined with evidence: #46336 (Forgejo exposes no conditional-write primitive; concurrency: cancel-in-progress plus 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.

> Replying to review #10816 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 in 7abe25e: #46334, #46335. Declined with evidence: #46336 (Forgejo exposes no conditional-write primitive; `concurrency: cancel-in-progress` plus 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.
Author
Owner

Replying to review #10817

Acknowledged. Fixed in f7d380f: #46341 (snippet ranges are now capped at Forgejo's 50-line MAX_CODE_COMMENT_LINES). Already fixed in 7abe25e: #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).

> Replying to review #10817 Acknowledged. Fixed in f7d380f: #46341 (snippet ranges are now capped at Forgejo's 50-line `MAX_CODE_COMMENT_LINES`). Already fixed in 7abe25e: #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).
Author
Owner

Replying to review #10818

Acknowledged. Fixed in f7d380f: #46348 (null head.repo). Declined with evidence: #46347 (a fork task token is AccessModeRead and creating an issue comment needs only issues-read, per GetActionRepoPermission and CreateIssueComment), #46349 (head TOCTOU — no conditional-write primitive exists; workflow concurrency and marker-idempotent writes cover it).

> Replying to review #10818 Acknowledged. Fixed in f7d380f: #46348 (null `head.repo`). Declined with evidence: #46347 (a fork task token is `AccessModeRead` and creating an issue comment needs only issues-read, per `GetActionRepoPermission` and `CreateIssueComment`), #46349 (head TOCTOU — no conditional-write primitive exists; workflow concurrency and marker-idempotent writes cover it).
Author
Owner

Replying to review #10823

Acknowledged; every inline finding is answered on its thread. Fixed in f7d380f: #46406 (dot-segment path guard), #46407 (nullable head.repo plus a real pr_number parse), #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 (Destination type 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).

> Replying to review #10823 Acknowledged; every inline finding is answered on its thread. Fixed in f7d380f: #46406 (dot-segment path guard), #46407 (nullable `head.repo` plus a real `pr_number` parse), #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 (`Destination` type 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).
Author
Owner

Replying to review #10827

Acknowledged. Fixed in f7d380f: #46426 (README env-var requirements split per path). Declined with evidence: #46423 (Forgejo's ListIssueComments sets no ListOptions, so the endpoint returns every comment in one response), #46424 (fork task token is AccessModeRead and comment creation needs only issues-read), #46425 (the review workflow's concurrency: cancel-in-progress prevents the overlapping upsert, and Forgejo offers no conditional create).

> Replying to review #10827 Acknowledged. Fixed in f7d380f: #46426 (README env-var requirements split per path). Declined with evidence: #46423 (Forgejo's `ListIssueComments` sets no `ListOptions`, so the endpoint returns every comment in one response), #46424 (fork task token is `AccessModeRead` and comment creation needs only issues-read), #46425 (the review workflow's `concurrency: cancel-in-progress` prevents the overlapping upsert, and Forgejo offers no conditional create).
Author
Owner

Replying to review #10829

Acknowledged. Fixed in f7d380f: #46429 (null head.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).

> Replying to review #10829 Acknowledged. Fixed in f7d380f: #46429 (null `head.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).
Author
Owner

Replying to review #10830

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).

> Replying to review #10830 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).
forgejo-actions left a comment

Summary: Reviewed the composition in src/main.ts and all eight modules against the outcome table in README.md. The fork gate holds as documented — reviewCredentials() is reachable only through the openSession thunk, and src/main.ts is its sole importer, so REVIEW_CAPABILITY_TOKEN is never read on a fork path. Path handling (encodeSlug, the dot-segment guard in getRawFile), the pinned-pass wait loop's deadline bounds, and the claims-cursor walk all look sound; the committed bundle's tail matches src/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-Count on issue comments) and the -j4k resolution routes; node_modules is absent, so the gates listed in the PR body were not re-run.

Code review by Claude Code Opus (opus)

**Summary:** Reviewed the composition in `src/main.ts` and all eight modules against the outcome table in `README.md`. The fork gate holds as documented — `reviewCredentials()` is reachable only through the `openSession` thunk, and `src/main.ts` is its sole importer, so `REVIEW_CAPABILITY_TOKEN` is never read on a fork path. Path handling (`encodeSlug`, the dot-segment guard in `getRawFile`), the pinned-pass wait loop's deadline bounds, and the claims-cursor walk all look sound; the committed bundle's tail matches `src/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-Count` on issue comments) and the `-j4k` resolution routes; `node_modules` is absent, so the gates listed in the PR body were not re-run. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImY3ZDM4MGZkMjgwZTBiNDg5ZjgxMjg0MWE3ZjRjMmIzMzM0Mjc1ZDIiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5NDc5Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjVlYjFhZDRkLTI3NTktNGFmMi1hZGZmLTE0ZGE5NzYzYmMxZSJ9 -->
@ -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: call parses the body unconditionally, and unresolveConversation routes a DELETE through it. If that route answers 204/empty — the usual shape for a DELETE, and the PR body lists this route as unverified against a real -j4k instance — JSON.parse("") throws a bare SyntaxError, which isResolutionDegradation does not recognise (no status, no resolutionUnsupported), so it escapes unresolve and 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 dereferencing updated.resolver.

🟢 **Low:** `call` parses the body unconditionally, and `unresolveConversation` routes a DELETE through it. If that route answers `204`/empty — the usual shape for a DELETE, and the PR body lists this route as unverified against a real `-j4k` instance — `JSON.parse("")` throws a bare `SyntaxError`, which `isResolutionDegradation` does not recognise (no `status`, no `resolutionUnsupported`), so it escapes `unresolve` and 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 dereferencing `updated.resolver`.
Author
Owner

Not a defect, and the premise is settled by a live probe rather than the usual DELETE convention: I ran POST then DELETE .../reviews/{id}/comments/{comment}/resolution against this -j4k instance (comment 46334) before writing the check — the DELETE answers 200 with the updated comment JSON, resolver: null. So call() has a body to parse and updated.resolver is 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-up listReviewComments would be dead code guarding a shape this server does not produce.

<!-- gh-feedback:reply-to:46600 --> Not a defect, and the premise is settled by a live probe rather than the usual DELETE convention: I ran `POST` then `DELETE .../reviews/{id}/comments/{comment}/resolution` against this `-j4k` instance (comment 46334) before writing the check — the DELETE answers `200` with the updated comment JSON, `resolver: null`. So `call()` has a body to parse and `updated.resolver` is 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-up `listReviewComments` would 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: settle resolves the conversation whenever none of currents is reopened, but currents holds only the claims that already have a disposition — the flatMap at lines 171-174 drops every claim whose disposition.current is null/undefined. A conversation carrying one fixed claim and one still-undisposed claim therefore gets resolved, hiding a live finding's thread in the UI.

Sharing a conversation is not exotic: deriveConversations buckets by (review id, path, display line), and mapAnchor maps every scope: "path" anchor for a file to smallest(covered) with extraLines: 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 claimIds into settle and compare claimIds.filter((id) => claimById.has(id)).length with currents.length).

🟡 **Medium:** `settle` resolves the conversation whenever none of `currents` is `reopened`, but `currents` holds only the claims that already have a disposition — the `flatMap` at lines 171-174 drops every claim whose `disposition.current` is `null`/`undefined`. A conversation carrying one `fixed` claim and one still-undisposed claim therefore gets resolved, hiding a live finding's thread in the UI. Sharing a conversation is not exotic: `deriveConversations` buckets by (review id, path, display line), and `mapAnchor` maps *every* `scope: "path"` anchor for a file to `smallest(covered)` with `extraLines: 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 `claimIds` into `settle` and compare `claimIds.filter((id) => claimById.has(id)).length` with `currents.length`).
Author
Owner

Fixed in dccfc20. settle now 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 in src/reconcile/dispositions.test.ts.

<!-- gh-feedback:reply-to:46599 --> Fixed in dccfc20. `settle` now 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 in `src/reconcile/dispositions.test.ts`.
jercik marked this conversation as resolved
@ -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: findOurComment matches on the body prefix alone, with no check that the comment is the action's own. listIssueComments returns 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, and upsert PATCHes 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 editIssueComment on another user's comment is rejected, the ForgeRequestError escapes reconcileForkSkip, and runWrapper'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.

🟡 **Medium:** `findOurComment` matches on the body prefix alone, with no check that the comment is the action's own. `listIssueComments` returns 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, and `upsert` PATCHes 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 `editIssueComment` on another user's comment is rejected, the `ForgeRequestError` escapes `reconcileForkSkip`, and `runWrapper`'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.
Author
Owner

Fixed in dccfc20. IssueComment now carries user.id and findOurComment adopts a marked comment only when that id is the Forgejo actions system user (-2, the ActionsUserID constant 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 no GET /user round trip because the id is a server constant, not a per-instance identity. Test added in src/reconcile/summary.test.ts; the README security-model bullet records it.

<!-- gh-feedback:reply-to:46598 --> Fixed in dccfc20. `IssueComment` now carries `user.id` and `findOurComment` adopts a marked comment only when that id is the Forgejo actions system user (`-2`, the `ActionsUserID` constant 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 no `GET /user` round trip because the id is a server constant, not a per-instance identity. Test added in `src/reconcile/summary.test.ts`; the README security-model bullet records it.
jercik marked this conversation as resolved
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImY3ZDM4MGZkMjgwZTBiNDg5ZjgxMjg0MWE3ZjRjMmIzMzM0Mjc1ZDIiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5NDc5Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6ImI4MGJlNmFlLTg3ZmQtNGNmNS05OTFkLThiZjU3ZGY2ODBjZSJ9 -->
@ -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, causing walkClaims to issue requests forever. waitForSettle awaits 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.

🟡 **Medium:** This only rejects a cursor repeated immediately. A malformed service response can cycle cursors such as `A → B → A`, causing `walkClaims` to issue requests forever. `waitForSettle` awaits 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.
Author
Owner

Fixed in dccfc20. walkClaims now tracks every cursor it has followed in a Set and throws claims cursor repeated <cursor> on any repeat, so an A → B → A cycle terminates at the first repeat instead of paging forever inside the grounding probe. New cycle test in src/wrapper/orchestrator.test.ts.

<!-- gh-feedback:reply-to:46632 --> Fixed in dccfc20. `walkClaims` now tracks every cursor it has followed in a `Set` and throws `claims cursor repeated <cursor>` on any repeat, so an `A → B → A` cycle terminates at the first repeat instead of paging forever inside the grounding probe. New cycle test in `src/wrapper/orchestrator.test.ts`.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 1 high and 1 medium issue.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 1 high and 1 medium issue. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImY3ZDM4MGZkMjgwZTBiNDg5ZjgxMjg0MWE3ZjRjMmIzMzM0Mjc1ZDIiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTIiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTk0NzkiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiMzBjMDAxNzUtMmExNy00YjFiLWJiZWEtOTgyYTg1NzAwZjY5In0= -->
@ -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: findOurComment treats 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, so upsert calls editIssueComment, 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 in IssueComment) or ignore unowned marker comments and create the wrapper comment.

🟠 **High:** `findOurComment` treats 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, so `upsert` calls `editIssueComment`, 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 in `IssueComment`) or ignore unowned marker comments and create the wrapper comment.
Author
Owner

Fixed in dccfc20. IssueComment now carries user.id and findOurComment adopts a marked comment only when that id is the Forgejo actions system user (-2, the ActionsUserID constant 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 no GET /user round trip because the id is a server constant, not a per-instance identity. Test added in src/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.

<!-- gh-feedback:reply-to:46634 --> Fixed in dccfc20. `IssueComment` now carries `user.id` and `findOurComment` adopts a marked comment only when that id is the Forgejo actions system user (`-2`, the `ActionsUserID` constant 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 no `GET /user` round trip because the id is a server constant, not a per-instance identity. Test added in `src/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.
jercik marked this conversation as resolved
@ -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 calls readSettle, so if another run creates a newer pass while this pinned pass remains incomplete until the 30-minute timeout, superseded stays false. The timeout then proceeds through classify/reconcile and can write the incomplete/latest review to the PR even though the newer pass owns the surface. Observe latest_pass during stage-1 polling (or suppress reconciliation when it changes) before returning the timeout.

🟡 **Medium:** Stage 1 only polls `getCoverage`; it never calls `readSettle`, so if another run creates a newer pass while this pinned pass remains incomplete until the 30-minute timeout, `superseded` stays false. The timeout then proceeds through `classify`/`reconcile` and can write the incomplete/latest review to the PR even though the newer pass owns the surface. Observe `latest_pass` during stage-1 polling (or suppress reconciliation when it changes) before returning the timeout.
Author
Owner

Fixed in dccfc20. On the stage-1 coverage timeout the wait now performs one readSettle before returning, so latest_pass is observed and superseded is 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.

<!-- gh-feedback:reply-to:46635 --> Fixed in dccfc20. On the stage-1 coverage timeout the wait now performs one `readSettle` before returning, so `latest_pass` is observed and `superseded` is 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.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 1 critical and 2 high issues.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 1 critical and 2 high issues. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImY3ZDM4MGZkMjgwZTBiNDg5ZjgxMjg0MWE3ZjRjMmIzMzM0Mjc1ZDIiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTMiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTk0NzkiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiMTM1ZTBmYzMtZjUxOC00YmIyLTliMTQtYzFjOGJlZWQzZmU1In0= -->
@ -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: findOurComment trusts 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 in existingClaimMarkers/hasMarker lets 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.

🔴 **Critical:** `findOurComment` trusts 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 in `existingClaimMarkers`/`hasMarker` lets 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.
Author
Owner

Fixed in dccfc20. IssueComment now carries user.id and findOurComment adopts a marked comment only when that id is the Forgejo actions system user (-2, the ActionsUserID constant 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 no GET /user round trip because the id is a server constant, not a per-instance identity. Test added in src/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.

<!-- gh-feedback:reply-to:46637 --> Fixed in dccfc20. `IssueComment` now carries `user.id` and `findOurComment` adopts a marked comment only when that id is the Forgejo actions system user (`-2`, the `ActionsUserID` constant 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 no `GET /user` round trip because the id is a server constant, not a per-instance identity. Test added in `src/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.
jercik marked this conversation as resolved
@ -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, after keeps 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.

🟠 **High:** This only rejects an immediately repeated cursor. If the service returns a valid-looking cycle such as `A → B → A`, `after` keeps 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.
Author
Owner

Fixed in dccfc20. walkClaims now tracks every cursor it has followed in a Set and throws claims cursor repeated <cursor> on any repeat, so an A → B → A cycle terminates at the first repeat instead of paging forever inside the grounding probe. New cycle test in src/wrapper/orchestrator.test.ts.

<!-- gh-feedback:reply-to:46639 --> Fixed in dccfc20. `walkClaims` now tracks every cursor it has followed in a `Set` and throws `claims cursor repeated <cursor>` on any repeat, so an `A → B → A` cycle terminates at the first repeat instead of paging forever inside the grounding probe. New cycle test in `src/wrapper/orchestrator.test.ts`.
jercik marked this conversation as resolved
@ -0,0 +88,4 @@
known: FindingsResponse | undefined,
): Promise<void> {
const { inputs } = deps;
const report = await session.getReport(reviewId);

🟠 High: When wait reports superseded, this reconciliation still reads review-level/unpinned getReport, getFindings, and listClaims, while the exit and coverage decision used pinnedPassId. 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.

🟠 **High:** When `wait` reports `superseded`, this reconciliation still reads review-level/unpinned `getReport`, `getFindings`, and `listClaims`, while the exit and coverage decision used `pinnedPassId`. 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.
Author
Owner

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 /report and the findings/claims reads take no pass_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_id on the report read) that closes it.

<!-- gh-feedback:reply-to:46638 --> 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 /report` and the findings/claims reads take no `pass_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_id` on the report read) that closes it.
fix: close the defects the third review round found
All checks were successful
commit-msg / commitlint (pull_request) Successful in 22s
Dedupe check / dedupe-check (pull_request) Successful in 44s
Checks / quality-checks (pull_request) Successful in 54s
PR Review / Prepare immutable review tools (pull_request_target) Successful in 3m1s
PR Review / forgejo-review-approach-smart-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-1 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-3 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-approach-luna-2 generator (pull_request_target) Has been skipped
PR Review / forgejo-review-code-smart-1 generator (pull_request_target) Successful in 12m10s
PR Review / forgejo-review-code-luna-3 generator (pull_request_target) Successful in 13m1s
PR Review / forgejo-review-code-luna-2 generator (pull_request_target) Successful in 13m46s
PR Review / forgejo-review-code-luna generator (pull_request_target) Successful in 25m2s
PR Review / Dispatch and observe exact review writers (pull_request_target) Successful in 25m21s
dccfc205e9
Author
Owner

Replying to review #10850

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.

> Replying to review #10850 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.
Author
Owner

Replying to review #10852

Acknowledged. #46632 is fixed in dccfc20 — walkClaims now records every cursor it follows and throws on any repeat, so a cursor cycle cannot spin inside the grounding probe.

> Replying to review #10852 Acknowledged. #46632 is fixed in dccfc20 — `walkClaims` now records every cursor it follows and throws on any repeat, so a cursor cycle cannot spin inside the grounding probe.
Author
Owner

Replying to review #10853

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 so superseded is observed on that path).

> Replying to review #10853 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 so `superseded` is observed on that path).
Author
Owner

Replying to review #10854

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.

> Replying to review #10854 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.
forgejo-actions left a comment

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-pending without 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:

  • ListIssueComments really does take no ListOptions, and its X-Total-Count is CountComments over the same options — the single-fetch listing plus distinct-id equality check in src/forge/client.ts is sound.
  • ListPullReviews paginates and sets X-Total-Count, so failing closed on a missing header is right.
  • ValidateCodeCommentLineRange rejects extra_lines_count + 1 > MaxCodeCommentLines; the 50-line cap in mapAnchor yields at most extraLines = 49, so it stays inside the limit.
  • PullReviewComment carries position, original_position, extra_lines_count and resolver, and CreatePullReviewComment maps old_position > 0 to a negative line — replyAt and displayLine match the server's bucketing.
  • CreateIssueComment needs only CanReadIssuesOrPulls plus an unlocked issue, and EditIssueComment permits 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_modules is absent and @j4k/review lives on a private registry — so nothing here is based on executed tests.

Code review by Claude Code Opus (opus)

**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-pending` without 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: - `ListIssueComments` really does take no `ListOptions`, and its `X-Total-Count` is `CountComments` over the same options — the single-fetch listing plus distinct-id equality check in `src/forge/client.ts` is sound. - `ListPullReviews` paginates and sets `X-Total-Count`, so failing closed on a missing header is right. - `ValidateCodeCommentLineRange` rejects `extra_lines_count + 1 > MaxCodeCommentLines`; the 50-line cap in `mapAnchor` yields at most `extraLines = 49`, so it stays inside the limit. - `PullReviewComment` carries `position`, `original_position`, `extra_lines_count` and `resolver`, and `CreatePullReviewComment` maps `old_position > 0` to a negative line — `replyAt` and `displayLine` match the server's bucketing. - `CreateIssueComment` needs only `CanReadIssuesOrPulls` plus an unlocked issue, and `EditIssueComment` permits 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_modules` is absent and `@j4k/review` lives on a private registry — so nothing here is based on executed tests. _Code review by Claude Code Opus (opus)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImRjY2ZjMjA1ZTljYjc0YWEwMGI0ZGVjZmE0ZmFkNGMwMjJkNzIzNzEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1zbWFydC0xIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5NDk4Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6Ijk2Y2JhYjU2LWFjYzgtNDkxYS1iZTYyLTdlMTNhM2U4ODBiMiJ9 -->
@ -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 to FALLBACK_PAGE_LIMIT (50) because /settings/api was unreadable, and the instance has max_response_items configured below 50, Forgejo clamps limit server-side (utils.GetListOptions) while pages is still computed against 50. With max_response_items = 20 and 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 listReviews fails 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 total and the last page was non-empty, instead of capping at 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 to `FALLBACK_PAGE_LIMIT` (50) because `/settings/api` was unreadable, and the instance has `max_response_items` configured *below* 50, Forgejo clamps `limit` server-side (`utils.GetListOptions`) while `pages` is still computed against 50. With `max_response_items = 20` and 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 `listReviews` fails 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 `total` and the last page was non-empty, instead of capping at `ceil(total / limit)`.
jercik marked this conversation as resolved
@ -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.

commentBody puts claim.title and claim.body into the comment (the > quoting on line 12-15 does not neutralize an HTML comment), and claim.body is model-generated prose about a diff. A claim whose body reproduces a <!-- review:claim:… --> sequence — quoting code that contains one, for instance — makes this includes match 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:31 already 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.

🟢 **Low:** The idempotency check is a substring match against bodies that embed service-supplied text verbatim. `commentBody` puts `claim.title` and `claim.body` into the comment (the `> ` quoting on line 12-15 does not neutralize an HTML comment), and `claim.body` is model-generated prose about a diff. A claim whose body reproduces a `<!-- review:claim:… -->` sequence — quoting code that contains one, for instance — makes this `includes` match 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:31` already 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.
jercik marked this conversation as resolved
@ -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_target payload is cast, not parsed, while the workflow_dispatch branch right below validates its one field.

readEventPayload(...) as PullRequestTargetEvent asserts a shape onto JSON.parse output. A payload without pull_request (or without head/base) fails as TypeError: Cannot read properties of undefined rather than the fail-fast message this module uses everywhere else — and because src/main.ts:27 awaits readWrapperInputs outside runWrapper, that surfaces as a module-evaluation rejection with a raw stack, not a described outcome.

prNumber is 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 of parsePrNumber. A z.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.

🟢 **Low:** The `pull_request_target` payload is cast, not parsed, while the `workflow_dispatch` branch right below validates its one field. `readEventPayload(...) as PullRequestTargetEvent` asserts a shape onto `JSON.parse` output. A payload without `pull_request` (or without `head`/`base`) fails as `TypeError: Cannot read properties of undefined` rather than the fail-fast message this module uses everywhere else — and because `src/main.ts:27` awaits `readWrapperInputs` *outside* `runWrapper`, that surfaces as a module-evaluation rejection with a raw stack, not a described outcome. `prNumber` is 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 of `parsePrNumber`. A `z.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.
jercik marked this conversation as resolved
@ -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-pending without ever checking whether grounding is still pending.

graceDeadlineAt is set once, at the first settled observation (line 120), and deliberately not restarted. But poll is a while (clock.now() < deadlineAt) loop, so when the grace has already elapsed the step body never runs and hasPendingGrounding is never called — the ?? "grounding-pending" fallback fires unconditionally.

Concrete sequence, all inside the 15-minute stage-2 budget:

  1. t=0 settle reads settled → graceDeadlineAt = 300_000.
  2. Grounding is pending; polling continues.
  3. t=105s settle regresses to due → pollGrounding returns regressed.
  4. Stage 2 re-polls; at t=350s settle reads settled again.
  5. pollGrounding is entered with graceDeadlineAt = 300_000 < 350_000 → returns grounding-pending immediately.

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 hasPendingGrounding probe before falling back), so a spent grace budget stops the waiting without skipping the observation.

🟡 **Medium:** After a triage regression, an exhausted grace window makes this return `grounding-pending` without ever checking whether grounding is still pending. `graceDeadlineAt` is set once, at the first settled observation (line 120), and deliberately not restarted. But `poll` is a `while (clock.now() < deadlineAt)` loop, so when the grace has already elapsed the step body never runs and `hasPendingGrounding` is never called — the `?? "grounding-pending"` fallback fires unconditionally. Concrete sequence, all inside the 15-minute stage-2 budget: 1. `t=0` settle reads `settled` → `graceDeadlineAt = 300_000`. 2. Grounding is pending; polling continues. 3. `t=105s` settle regresses to `due` → `pollGrounding` returns `regressed`. 4. Stage 2 re-polls; at `t=350s` settle reads `settled` again. 5. `pollGrounding` is entered with `graceDeadlineAt = 300_000 < 350_000` → returns `grounding-pending` immediately. 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 `hasPendingGrounding` probe before falling back), so a spent grace budget stops the *waiting* without skipping the *observation*.
jercik marked this conversation as resolved
forgejo-actions left a comment

Summary: Found 2 actionable issues: 1 high and 1 medium.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** Found 2 actionable issues: 1 high and 1 medium. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImRjY2ZjMjA1ZTljYjc0YWEwMGI0ZGVjZmE0ZmFkNGMwMjJkNzIzNzEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTMiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTk0OTgiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiMmFjNjVlMmItMDJkMi00OWQzLTk5M2UtZjEwNDI0YzViMzJmIn0= -->
@ -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: claimIdsOf parses claim markers from every comment in a conversation, and hasMarker likewise 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, and settle can 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.

🟠 **High:** `claimIdsOf` parses claim markers from every comment in a conversation, and `hasMarker` likewise 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, and `settle` can 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.
Author
Owner

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 into PullReviewComment would buy that nuisance case a check whose threat model is a person with push access lying to themselves.

<!-- gh-feedback:reply-to:46674 --> 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 into `PullReviewComment` would 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: existingClaimMarkers collects 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.

🟡 **Medium:** `existingClaimMarkers` collects 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.
Author
Owner

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 into PullReviewComment would buy that nuisance case a check whose threat model is a person with push access lying to themselves.

<!-- gh-feedback:reply-to:46675 --> 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 into `PullReviewComment` would buy that nuisance case a check whose threat model is a person with push access lying to themselves.
forgejo-actions left a comment

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)

**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)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImRjY2ZjMjA1ZTljYjc0YWEwMGI0ZGVjZmE0ZmFkNGMwMjJkNzIzNzEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hLTIiLCJzb3VyY2VXb3JrZmxvd1J1bklkIjoiMTk0OTgiLCJzb3VyY2VHZW5lcmF0b3JBdHRlbXB0IjoiMSIsInJlc3VsdElkIjoiODY4OWRkZDgtNjk3Mi00NDc4LTk3NDgtNjZkZTczOGFiYTkzIn0= -->
@ -0,0 +119,4 @@
throw error;
}
},
listIssueComments: async (prNumber) => {

🟠 High: Forgejo paginates GET /issues/{index}/comments; omitting page/limit returns only the default-sized first page while X-Total-Count is 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.

🟠 **High:** Forgejo paginates `GET /issues/{index}/comments`; omitting `page`/`limit` returns only the default-sized first page while `X-Total-Count` is 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](https://forgejo.org/docs/latest/user/api-usage/).
Author
Owner

Verified against the deployed Forgejo source, not the pagination docs: ListIssueComments (routers/api/v1/repo/issue_comment.go, lines 92-97) builds FindCommentsOptions{IssueID, Since, Before, Type} with no ListOptions, and FindComments applies a page only when one is set — so this endpoint ignores page/limit and returns every comment in one response, with X-Total-Count from CountComments over the same options. That is exactly why the client asserts distinct-ids == X-Total-Count rather 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 set ListOptions. Same finding as #46423 last round, with the same source evidence.

<!-- gh-feedback:reply-to:46678 --> Verified against the deployed Forgejo source, not the pagination docs: `ListIssueComments` (`routers/api/v1/repo/issue_comment.go`, lines 92-97) builds `FindCommentsOptions{IssueID, Since, Before, Type}` with **no** `ListOptions`, and `FindComments` applies a page only when one is set — so this endpoint ignores `page`/`limit` and returns every comment in one response, with `X-Total-Count` from `CountComments` over the same options. That is exactly why the client asserts distinct-ids == `X-Total-Count` rather 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 set `ListOptions`. 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 end does not ensure newPosition is itself a covered diff line. For example, the long-snippet test maps a 60-line snippet with only line 60 covered to newPosition: 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.

🟠 **High:** Capping the range from `end` does not ensure `newPosition` is itself a covered diff line. For example, the long-snippet test maps a 60-line snippet with only line 60 covered to `newPosition: 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.
Author
Owner

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 by CodeCommentLines + extraLinesCount; a CutDiffAroundLine failure 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 is ValidateCodeCommentLineRange, which checks extra_lines_count + 1 <= MaxCodeCommentLines and nothing about coverage. LineBlame is 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.

<!-- gh-feedback:reply-to:46679 --> 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 by `CodeCommentLines + extraLinesCount`; a `CutDiffAroundLine` failure 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 is `ValidateCodeCommentLineRange`, which checks `extra_lines_count + 1 <= MaxCodeCommentLines` and nothing about coverage. `LineBlame` is 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.
forgejo-actions left a comment

Summary: No actionable issues found.

Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)

**Summary:** No actionable issues found. _Code review by Codex GPT-5.6 Luna (gpt-5.6-luna)_ <!-- axrecipe-review:v1:eyJzY2hlbWFWZXJzaW9uIjoxLCJzdGF0ZSI6InB1Ymxpc2hlZCIsInJlcG9zaXRvcnkiOiJqNGsvcmV2aWV3LXdyYXBwZXIiLCJudW1iZXIiOiI0IiwiaGVhZFNoYSI6ImRjY2ZjMjA1ZTljYjc0YWEwMGI0ZGVjZmE0ZmFkNGMwMjJkNzIzNzEiLCJzbG90IjoiZm9yZ2Vqby1yZXZpZXctY29kZS1sdW5hIiwic291cmNlV29ya2Zsb3dSdW5JZCI6IjE5NDk4Iiwic291cmNlR2VuZXJhdG9yQXR0ZW1wdCI6IjEiLCJyZXN1bHRJZCI6IjE4ODA1MDI5LTRhY2EtNDIwOS1hY2Q1LWEyMzljNzE4ODE2MyJ9 -->
Author
Owner

Replying to review comment #46669

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 — graceDeadlineAt is set at the first settled observation, so a regression that resolves after the grace has expired re-enters pollGrounding with a spent budget and returns grounding-pending without 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.

> Replying to review comment #46669 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 — `graceDeadlineAt` is set at the first settled observation, so a regression that resolves after the grace has expired re-enters `pollGrounding` with a spent budget and returns `grounding-pending` without 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.
Author
Owner

Replying to review comment #46670

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/api to be unreadable and the instance's real max_response_items to be below 50 — on code.j4k.dev it 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.

> Replying to review comment #46670 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/api` to be unreadable *and* the instance's real `max_response_items` to be below 50 — on `code.j4k.dev` it 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.
Author
Owner

Replying to review comment #46671

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.

> Replying to review comment #46671 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.
Author
Owner

Replying to review comment #46672

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_target payload with Zod is the right end state per the repo's parse-don't-validate rule; the concrete crash this round raised (a null head.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.

> Replying to review comment #46672 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_target` payload with Zod is the right end state per the repo's parse-don't-validate rule; the concrete crash this round raised (a null `head.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.
Author
Owner

Replying to review #10859

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.

> Replying to review #10859 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.
Author
Owner

Replying to review #10860

Acknowledged. Both findings are declined with source evidence on their threads: #46678 — Forgejo's ListIssueComments sets no ListOptions, so the endpoint returns every comment in one response and the distinct-ids/X-Total-Count assertion is the completeness proof; #46679 — Forgejo centers a code comment's hunk on the display line and tolerates a failed CutDiffAroundLine, so an uncovered range start is leading context, not a rejection.

> Replying to review #10860 Acknowledged. Both findings are declined with source evidence on their threads: #46678 — Forgejo's `ListIssueComments` sets no `ListOptions`, so the endpoint returns every comment in one response and the distinct-ids/`X-Total-Count` assertion is the completeness proof; #46679 — Forgejo centers a code comment's hunk on the display line and tolerates a failed `CutDiffAroundLine`, so an uncovered range start is leading context, not a rejection.
Author
Owner

Replying to review #10861

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.

> Replying to review #10861 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.
Author
Owner

Replying to review #10863

Acknowledged — no actionable issues.

> Replying to review #10863 Acknowledged — no actionable issues.
jercik merged commit 90bdc069c1 into main 2026-08-08 13:47:55 +00:00
jercik deleted branch feat/wrapper 2026-08-08 13:47:55 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
j4k-oss/review-wrapper!4
No description provided.