fix: validate the wrapper's inputs and stop reporting a refused resolution as unsupported #11

Merged
jercik merged 4 commits from fix/dispatch-input-validation into main 2026-09-01 05:53:44 +00:00
Owner

Four things the wrapper accepted or described wrongly. Each now fails loudly, and dist/index.mjs is rebuilt to match.

parsePrNumber ran Number(raw) past an integer check, so 007, 7.0, 7e0, 7, +7 and 0x7 all became 7. Dispatching 007 builds the concurrency group review-007, which supersedes nothing — two passes then write the same comment surface. The raw string must now match ^[1-9][0-9]*$, and the converted value must be a safe integer: Number("9007199254740993") is 9007199254740992, and enough digits give Infinity and a request for /pulls/Infinity.

REVIEW_SERVICE_URL was only checked for emptiness before the capability token rode an Authorization header to whatever it named. It must now parse as an https: URL whose path does not already end in /v1 — the client appends that prefix itself, so https://review.j4k.dev/v1 used to 404 every call as /v1/v1/reviews. The new optional expected-service-origin action input pins the origin exactly when a caller sets it; left unset, or set to the empty string an undefined vars expression expands to, only the scheme and prefix checks apply.

Nothing read the pull request's state, so a workflow_dispatch for a closed or merged PR was reviewed like any other. The dispatch arm now refuses anything that is not open and unmerged, and refuses with a separate message when the forge returns neither state nor merged rather than guessing.

A runtime 403 or 404 from the resolve endpoint was logged as "conversation resolution is unavailable on this instance" — false on a -j4k server whose version probe just confirmed the route exists (#8). The 403 now says the task token may not use the route. The 404 stays ambiguous on purpose: the probe matches the -j4k substring, which proves fork lineage and not that the build serves the endpoint, so the message names all three causes — an older build, a comment deleted since the sweep, or a refused token. Either way the pass degrades to replies instead of failing the run. That fixes the label, not the refusal — a real 403 still needs the Actions task token granted resolve permission server-side, so #8 stays open.

A follow-up in j4k/align bumps the pinned sha in the review workflow template and sets expected-service-origin: https://review.j4k.dev.

Four things the wrapper accepted or described wrongly. Each now fails loudly, and `dist/index.mjs` is rebuilt to match. `parsePrNumber` ran `Number(raw)` past an integer check, so `007`, `7.0`, `7e0`, ` 7`, `+7` and `0x7` all became 7. Dispatching `007` builds the concurrency group `review-007`, which supersedes nothing — two passes then write the same comment surface. The raw string must now match `^[1-9][0-9]*$`, and the converted value must be a safe integer: `Number("9007199254740993")` is 9007199254740992, and enough digits give `Infinity` and a request for `/pulls/Infinity`. `REVIEW_SERVICE_URL` was only checked for emptiness before the capability token rode an `Authorization` header to whatever it named. It must now parse as an `https:` URL whose path does not already end in `/v1` — the client appends that prefix itself, so `https://review.j4k.dev/v1` used to 404 every call as `/v1/v1/reviews`. The new optional `expected-service-origin` action input pins the origin exactly when a caller sets it; left unset, or set to the empty string an undefined vars expression expands to, only the scheme and prefix checks apply. Nothing read the pull request's state, so a `workflow_dispatch` for a closed or merged PR was reviewed like any other. The dispatch arm now refuses anything that is not open and unmerged, and refuses with a separate message when the forge returns neither `state` nor `merged` rather than guessing. A runtime 403 or 404 from the resolve endpoint was logged as "conversation resolution is unavailable on this instance" — false on a `-j4k` server whose version probe just confirmed the route exists (#8). The 403 now says the task token may not use the route. The 404 stays ambiguous on purpose: the probe matches the `-j4k` substring, which proves fork lineage and not that the build serves the endpoint, so the message names all three causes — an older build, a comment deleted since the sweep, or a refused token. Either way the pass degrades to replies instead of failing the run. That fixes the label, not the refusal — a real 403 still needs the Actions task token granted resolve permission server-side, so #8 stays open. A follow-up in `j4k/align` bumps the pinned sha in the review workflow template and sets `expected-service-origin: https://review.j4k.dev`.
fix: reject malformed dispatch inputs and name a refused resolution for what it is
All checks were successful
commit-msg / commitlint (pull_request) Successful in 22s
Checks / quality-checks (pull_request) Successful in 36s
Review / Review (pull_request_target) Successful in 9m22s
20b316a742
Four defects, all in code that accepted something it should have refused.

`parsePrNumber` ran `Number(raw)` past an integer check, so `007`, `7.0`, `7e0`,
` 7`, `+7` and `0x7` all became 7. A dispatch on `007` then computed the
concurrency group `review-007`, which supersedes nothing and lets two passes
write the same comment surface. The raw string now has to match
`^[1-9][0-9]*$` before it is parsed.

`REVIEW_SERVICE_URL` was only checked for emptiness before the capability token
rode an Authorization header to whatever it named. It now has to parse as an
`https:` URL, and the new optional `expected-service-origin` action input pins
the origin exactly when a caller sets it.

Nothing read the pull request's state, so a `workflow_dispatch` for a closed or
merged PR was reviewed like any other. The dispatch arm now refuses anything
that is not open and unmerged, and refuses with a distinct error when the forge
returns neither field rather than guessing.

A runtime 403 or 404 from the resolve endpoint was logged as "resolution is
unavailable on this instance", which is false on a `-j4k` server the version
probe just confirmed serves the route. The classification now follows the probe:
on a `-j4k` instance the failure is reported as a refusal, quoting the status and
the endpoint, and "unsupported" is left for instances whose probe found no
`-j4k`. Either way the pass still degrades to replies rather than failing the
run.

`dist/index.mjs` is rebuilt to match.

Review 01M1CCXN526GR602HEQ4P50821 — head 63e033375a4c58f9f146473b7aa39369fb9ef542

Review — j4k-oss/review-wrapper @ da2202054e

Scope: diff against base tree 017065182efc
Status: dispatched — coverage complete (3/3 slots terminal)

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v1",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v1",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v1"
}

Findings (0)

No findings survived.

Reviewed:

  • general-bug (whole/default): claims-emitted
  • comments-trimming (whole/default): no-claims
  • test-trimming (whole/default): no-claims

Other claims

  • grounding-pending (0)
  • ungrounded (0)
  • rejected (0)
  • duplicate-of (0)
  • unadjudicated (2)
    • 01M1CDAX3XN94FZSQSR6CCH8B2 medium — reviewCredentials validates a normalized URL but returns the raw value
    • 01M1CDC6XTQVM2JBHM2VEAC401 medium — workflow_dispatch does not recheck open state after the review wait

Coverage

lens part arm unit status runs loss
general-bug whole default claims-emitted 1 no
comments-trimming whole default no-claims 1 no
test-trimming whole default no-claims 1 no
<!-- review:summary --> **Review** `01M1CCXN526GR602HEQ4P50821` — head `63e033375a4c58f9f146473b7aa39369fb9ef542` # Review — j4k-oss/review-wrapper @ da2202054eb5 Scope: diff against base tree `017065182efc` Status: dispatched — coverage complete (3/3 slots terminal) Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v1", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v1", "tally": "tally-v1", "triage_settle": "triage-settle-v1" } ``` ## Findings (0) No findings survived. Reviewed: - general-bug (whole/default): claims-emitted - comments-trimming (whole/default): no-claims - test-trimming (whole/default): no-claims ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (2) - `01M1CDAX3XN94FZSQSR6CCH8B2` medium — reviewCredentials validates a normalized URL but returns the raw value - `01M1CDC6XTQVM2JBHM2VEAC401` medium — workflow_dispatch does not recheck open state after the review wait ## Coverage | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | claims-emitted | 1 | no | | comments-trimming | whole | default | no-claims | 1 | no | | test-trimming | whole | default | no-claims | 1 | no |
Lines 74-77
@ -64,0 +71,7 @@
if (shape.status !== 403 && shape.status !== 404) {
return undefined;
}
const endpoint = typeof shape.url === "string" ? shape.url : "an unrecorded endpoint";
if (!serverSupportsResolution) {
return {
reason: "unsupported",

low — classifyResolutionFailure's unsupported arm is unreachable: every production call passes probed === true
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

I traced every call site of classifyResolutionFailure through src/reconcile/dispositions.ts. I could not run vitest — there is no node_modules and no network in this sandbox — so this is a control-flow trace, not an executed reproduction.

The second argument is always pass.probed, and pass is built once per reconcile as const pass: Pass = { prNumber, probed, supported: probed } with probed declared readonly. classifyResolutionFailure is called only from the catch blocks of resolve() and unresolve(). Those two are reached only through:

  • projectClaims, which returns early on if (!pass.supported) { return; } before calling settle, and settle is the only caller of unresolve and one of the two callers of resolve;
  • projectSuperseded, which returns early on if (!pass.supported || !open) { return; } before its resolve.

pass.supported starts equal to probed and is only ever assigned false. So pass.supported === true at a resolve/unresolve call implies pass.probed === true, and serverSupportsResolution is a constant true at every reachable call.

What goes wrong: the if (!serverSupportsResolution) arm — its reason: "unsupported" value and its "this instance has no conversation-resolution API (HTTP …)" message — can never be produced at runtime. The two cases in the new src/reconcile/conversation-markers.test.ts that assert it ("calls a 404 on a server whose probe found no -j4k unsupported" and its 403 twin) exercise a branch the wrapper cannot enter. The reason field is never read anywhere either: grepping the tree for ResolutionDegradation and .reason matches only the declaration and the return type in this same file, so only message is consumed. The Pass.probed comment claims it "is the only thing that separates a refused resolution from an instance that never served the route", but an instance that never served the route is already diagnosed unconditionally in reconcile's if (!probed) branch and never reaches this code, so a maintainer reading the module will believe two diagnoses ship when only one can. Either drop the parameter and the dead arm, or move the pass.supported guards so an unprobed instance still attempts one resolution and can be classified.

What would refute this: a call to resolve/unresolve that is not guarded by pass.supported, or a write to pass.probed. I found neither in src/reconcile/dispositions.ts.

claim 01M1CA80TEG6VJT888NWT8NZKN of review 01M1C9TJ50AP66066Y2JKBA0FT

<!-- review:claim:01M1CA80TEG6VJT888NWT8NZKN --> **low** — classifyResolutionFailure's `unsupported` arm is unreachable: every production call passes probed === true lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I traced every call site of `classifyResolutionFailure` through `src/reconcile/dispositions.ts`. I could not run vitest — there is no `node_modules` and no network in this sandbox — so this is a control-flow trace, not an executed reproduction. > > The second argument is always `pass.probed`, and `pass` is built once per reconcile as `const pass: Pass = { prNumber, probed, supported: probed }` with `probed` declared `readonly`. `classifyResolutionFailure` is called only from the `catch` blocks of `resolve()` and `unresolve()`. Those two are reached only through: > > - `projectClaims`, which returns early on `if (!pass.supported) { return; }` before calling `settle`, and `settle` is the only caller of `unresolve` and one of the two callers of `resolve`; > - `projectSuperseded`, which returns early on `if (!pass.supported || !open) { return; }` before its `resolve`. > > `pass.supported` starts equal to `probed` and is only ever assigned `false`. So `pass.supported === true` at a `resolve`/`unresolve` call implies `pass.probed === true`, and `serverSupportsResolution` is a constant `true` at every reachable call. > > What goes wrong: the `if (!serverSupportsResolution)` arm — its `reason: "unsupported"` value and its "this instance has no conversation-resolution API (HTTP …)" message — can never be produced at runtime. The two cases in the new `src/reconcile/conversation-markers.test.ts` that assert it ("calls a 404 on a server whose probe found no -j4k unsupported" and its 403 twin) exercise a branch the wrapper cannot enter. The `reason` field is never read anywhere either: grepping the tree for `ResolutionDegradation` and `.reason` matches only the declaration and the return type in this same file, so only `message` is consumed. The `Pass.probed` comment claims it "is the only thing that separates a refused resolution from an instance that never served the route", but an instance that never served the route is already diagnosed unconditionally in `reconcile`'s `if (!probed)` branch and never reaches this code, so a maintainer reading the module will believe two diagnoses ship when only one can. Either drop the parameter and the dead arm, or move the `pass.supported` guards so an unprobed instance still attempts one resolution and can be classified. > > What would refute this: a call to `resolve`/`unresolve` that is not guarded by `pass.supported`, or a write to `pass.probed`. I found neither in `src/reconcile/dispositions.ts`. claim `01M1CA80TEG6VJT888NWT8NZKN` of review `01M1C9TJ50AP66066Y2JKBA0FT`
Author
Owner

Confirmed and fixed in 30972d873b.

The trace holds. pass.probed is readonly, pass.supported starts equal to it and is only ever assigned false, and both resolve and unresolve are reachable only past a !pass.supported guard (projectClaims line 125 before settle, projectSuperseded line 151 before its own resolve) — so serverSupportsResolution was a constant true and the unsupported arm was dead. Grepping confirmed the other half too: reason was read nowhere outside conversation-markers.test.ts.

Took the first branch you named. classifyResolutionFailure(error, probed) is now resolutionRefusalMessage(error) returning the message alone; ResolutionDegradation and its single-valued reason are gone, as is the now-unused Pass.probed field and the comment that claimed two diagnoses ship. The unprobed instance keeps its one diagnosis, unconditionally, in reconcile's !probed branch before any request goes out. The two tests asserting the dead branch are deleted; the rest were rewritten against the string return. Full gate set green (bundle, format:check, knip, typecheck, lint, fta, 249 tests).

<!-- gh-feedback:reply-to:74570 --> Confirmed and fixed in 30972d873bd96b0a1581aa448f9ffc4df6d65ddd. The trace holds. `pass.probed` is `readonly`, `pass.supported` starts equal to it and is only ever assigned `false`, and both `resolve` and `unresolve` are reachable only past a `!pass.supported` guard (`projectClaims` line 125 before `settle`, `projectSuperseded` line 151 before its own `resolve`) — so `serverSupportsResolution` was a constant `true` and the `unsupported` arm was dead. Grepping confirmed the other half too: `reason` was read nowhere outside `conversation-markers.test.ts`. Took the first branch you named. `classifyResolutionFailure(error, probed)` is now `resolutionRefusalMessage(error)` returning the message alone; `ResolutionDegradation` and its single-valued `reason` are gone, as is the now-unused `Pass.probed` field and the comment that claimed two diagnoses ship. The unprobed instance keeps its one diagnosis, unconditionally, in `reconcile`'s `!probed` branch before any request goes out. The two tests asserting the dead branch are deleted; the rest were rewritten against the string return. Full gate set green (bundle, format:check, knip, typecheck, lint, fta, 249 tests).
jercik marked this conversation as resolved
fix: drop the unreachable unsupported arm from the resolution refusal message
All checks were successful
commit-msg / commitlint (pull_request) Successful in 24s
Checks / quality-checks (pull_request) Successful in 43s
Review / Review (pull_request_target) Successful in 13m42s
30972d873b
Every resolve and unresolve call sits behind a `pass.supported` guard, and
`supported` starts at the version probe's verdict and only ever latches false —
so the classifier's `serverSupportsResolution` argument was a constant `true`
and its `unsupported` arm could not be reached. An instance that never served
the route is already diagnosed by `reconcile`'s `!probed` branch, before any
request. `classifyResolutionFailure` becomes `resolutionRefusalMessage(error)`
returning the message alone: the `reason` discriminant had one reachable value
and no reader outside its own tests.

Also document that an empty `expected-service-origin` — what an undefined vars
expression expands to — leaves the origin pin off, which the code has always
done and neither action.yml nor the README said.
Author
Owner

Unadjudicated claim 01M1CA8X0MB25483MPSWKCA7Z0 (low) — "An empty expected-service-origin input silently disables the origin pin, and neither action.yml nor the README says so"

The review pass listed this under ## Other claims → unadjudicated, so it never became a thread. Checked it anyway, and it is accurate: reviewCredentials() skips the origin assertion when the input is undefined or "" (src/credentials.ts, and src/credentials.test.ts pins the empty case deliberately), while both descriptions said only "Left unset". The gap matters because expected-service-origin: ${{ vars.SOMETHING }} expands to the empty string when the variable is undefined — the pin disappears with no error.

Documented in 30972d873b: action.yml and the README's action-inputs table now say an empty value leaves the pin off. Behaviour is unchanged.

> Unadjudicated claim `01M1CA8X0MB25483MPSWKCA7Z0` (low) — "An empty expected-service-origin input silently disables the origin pin, and neither action.yml nor the README says so" The review pass listed this under `## Other claims → unadjudicated`, so it never became a thread. Checked it anyway, and it is accurate: `reviewCredentials()` skips the origin assertion when the input is `undefined` *or* `""` (`src/credentials.ts`, and `src/credentials.test.ts` pins the empty case deliberately), while both descriptions said only "Left unset". The gap matters because `expected-service-origin: ${{ vars.SOMETHING }}` expands to the empty string when the variable is undefined — the pin disappears with no error. Documented in 30972d873bd96b0a1581aa448f9ffc4df6d65ddd: `action.yml` and the README's action-inputs table now say an empty value leaves the pin off. Behaviour is unchanged.
Lines 74-75
@ -64,0 +65,5 @@
if (shape.status !== 403 && shape.status !== 404) {
return undefined;
}
const endpoint = typeof shape.url === "string" ? shape.url : "an unrecorded endpoint";
return `review-wrapper: the forge refused conversation resolution with HTTP ${shape.status} from ${endpoint}; the version probe reports a -j4k instance, so the route exists and the task token is not allowed to use it — projecting replies only for the rest of this pass`;

low — Resolution 404s are misdiagnosed as token permission failures
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

I examined ForgeRequestError, probeResolution in src/forge/client.ts, resolutionRefusalMessage, and both resolve/unresolve catch sites. Every non-OK response becomes the same error shape, and the feature probe proves only that the version string contains -j4k; nevertheless any status 404 is logged as proof that the route exists and the task token is forbidden, without inspecting the response body or identifying the failure as an authorization response. A missing comment/review during the reconciliation sweep, or a -j4k build that does not carry this route, therefore produces a false permissions diagnosis and latches the rest of the pass into reply-only mode, sending an operator toward token changes instead of the missing resource/version. I could not inspect a server contract because none is present in the subject, so I am keeping this low severity; preserve the status/endpoint while describing 404 as ambiguous, or classify it only from a server-specific error code. A documented server guarantee that every 404 from this route on every -j4k version exclusively means token refusal would refute the finding.

claim 01M1CB65VHAYSQ2MD691DSW763 of review 01M1CAJG8RVF2X9P9R0N20S9Y6

<!-- review:claim:01M1CB65VHAYSQ2MD691DSW763 --> **low** — Resolution 404s are misdiagnosed as token permission failures lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I examined ForgeRequestError, probeResolution in src/forge/client.ts, resolutionRefusalMessage, and both resolve/unresolve catch sites. Every non-OK response becomes the same error shape, and the feature probe proves only that the version string contains `-j4k`; nevertheless any status 404 is logged as proof that the route exists and the task token is forbidden, without inspecting the response body or identifying the failure as an authorization response. A missing comment/review during the reconciliation sweep, or a -j4k build that does not carry this route, therefore produces a false permissions diagnosis and latches the rest of the pass into reply-only mode, sending an operator toward token changes instead of the missing resource/version. I could not inspect a server contract because none is present in the subject, so I am keeping this low severity; preserve the status/endpoint while describing 404 as ambiguous, or classify it only from a server-specific error code. A documented server guarantee that every 404 from this route on every `-j4k` version exclusively means token refusal would refute the finding. claim `01M1CB65VHAYSQ2MD691DSW763` of review `01M1CAJG8RVF2X9P9R0N20S9Y6`
Author
Owner

Agreed and fixed in 885f00f11e.

The probe is exactly as narrow as you describe — probeResolution in src/forge/client.ts reads /version and returns meta.version.includes("-j4k"), which proves fork lineage and nothing about the route. An older -j4k build predating the resolution endpoint passes it and 404s on the first resolve, and the anchor can also disappear between the sweep and the write. So the 403 diagnosis was being applied to a status that does not carry it.

Split by status rather than softening both: 403 keeps so the task token may not use the route, and 404 now reads the version probe reports a -j4k instance but not that this build serves the route, so the cause is that build, a comment deleted since the sweep, or a refused token. Status and endpoint stay verbatim in both, and both still degrade the pass to replies. resolutionRefusalMessage is renamed resolutionDegradationMessage to match. Full gate set green (252 tests).

<!-- gh-feedback:reply-to:74576 --> Agreed and fixed in 885f00f11e1d9144ce1ce4a22cafc4fd3369b106. The probe is exactly as narrow as you describe — `probeResolution` in `src/forge/client.ts` reads `/version` and returns `meta.version.includes("-j4k")`, which proves fork lineage and nothing about the route. An older `-j4k` build predating the resolution endpoint passes it and 404s on the first resolve, and the anchor can also disappear between the sweep and the write. So the 403 diagnosis was being applied to a status that does not carry it. Split by status rather than softening both: 403 keeps `so the task token may not use the route`, and 404 now reads `the version probe reports a -j4k instance but not that this build serves the route, so the cause is that build, a comment deleted since the sweep, or a refused token`. Status and endpoint stay verbatim in both, and both still degrade the pass to replies. `resolutionRefusalMessage` is renamed `resolutionDegradationMessage` to match. Full gate set green (252 tests).
jercik marked this conversation as resolved
Lines 43-48
@ -43,6 +48,6 @@
function parsePrNumber(raw: string | undefined): number {
const prNumber = Number(raw);
if (raw === undefined || !Number.isInteger(prNumber) || prNumber <= 0) {
if (raw === undefined || !PR_NUMBER.test(raw)) {
throw new Error(`workflow_dispatch input pr_number is not a positive integer: ${String(raw)}`);
}
return prNumber;
return Number(raw);
}

medium — Large dispatch PR numbers are silently rounded to another pull
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

I examined parsePrNumber in src/wrapper/event-context.ts, its workflow_dispatch caller, the accepted/rejected cases in event-context.test.ts, and the forge client route construction. The digit-only regex accepts values beyond JavaScript's safe-integer range and then Number(raw) rounds them (or produces Infinity) without another check. I ran printf '%s' '{"inputs":{"pr_number":"9007199254740993"}}' | GITHUB_EVENT_NAME=workflow_dispatch GITHUB_EVENT_PATH=/dev/stdin GITHUB_REPOSITORY=j4k/review GITHUB_RUN_ID=8814 GITHUB_SERVER_URL=https://code.j4k.dev node --experimental-strip-types --input-type=module -e '…readWrapperInputs…'; the fake forge was asked for 9007199254740992 and the resulting WrapperInputs carried that rounded number. A manual dispatch can therefore fetch and reconcile a different pull than the decimal ID the operator entered, while a sufficiently long value becomes /pulls/Infinity; convert once and require Number.isSafeInteger before calling getPull. A forge contract proving pull identifiers can never exceed Number.MAX_SAFE_INTEGER would lower the practical wrong-pull risk, while a real pull at either colliding ID would confirm it end to end.

claim 01M1CAVBM028NK5JRDP5WBNTQR of review 01M1CAJG8RVF2X9P9R0N20S9Y6

<!-- review:claim:01M1CAVBM028NK5JRDP5WBNTQR --> **medium** — Large dispatch PR numbers are silently rounded to another pull lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I examined parsePrNumber in src/wrapper/event-context.ts, its workflow_dispatch caller, the accepted/rejected cases in event-context.test.ts, and the forge client route construction. The digit-only regex accepts values beyond JavaScript's safe-integer range and then Number(raw) rounds them (or produces Infinity) without another check. I ran `printf '%s' '{"inputs":{"pr_number":"9007199254740993"}}' | GITHUB_EVENT_NAME=workflow_dispatch GITHUB_EVENT_PATH=/dev/stdin GITHUB_REPOSITORY=j4k/review GITHUB_RUN_ID=8814 GITHUB_SERVER_URL=https://code.j4k.dev node --experimental-strip-types --input-type=module -e '…readWrapperInputs…'`; the fake forge was asked for 9007199254740992 and the resulting WrapperInputs carried that rounded number. A manual dispatch can therefore fetch and reconcile a different pull than the decimal ID the operator entered, while a sufficiently long value becomes `/pulls/Infinity`; convert once and require Number.isSafeInteger before calling getPull. A forge contract proving pull identifiers can never exceed Number.MAX_SAFE_INTEGER would lower the practical wrong-pull risk, while a real pull at either colliding ID would confirm it end to end. claim `01M1CAVBM028NK5JRDP5WBNTQR` of review `01M1CAJG8RVF2X9P9R0N20S9Y6`
Author
Owner

Reproduced and fixed in 885f00f11e.

Confirmed against the regex directly: Number("9007199254740993") is 9007199254740992, Number("99999999999999999999999") is 1e+23, and 400 digits give Infinity — all three pass ^[1-9][0-9]*$. parsePrNumber now requires Number.isSafeInteger after the conversion and throws workflow_dispatch input pr_number is larger than any pull request number: <raw> otherwise, so nothing reaches getPull. New cases in event-context.test.ts pin both rejections and accept 9007199254740991 at the boundary.

I did not chase a forge contract for the identifier range — the fix does not need one, since a value the runtime cannot represent exactly is refused rather than assumed impossible. Full gate set green (252 tests).

<!-- gh-feedback:reply-to:74575 --> Reproduced and fixed in 885f00f11e1d9144ce1ce4a22cafc4fd3369b106. Confirmed against the regex directly: `Number("9007199254740993")` is `9007199254740992`, `Number("99999999999999999999999")` is `1e+23`, and 400 digits give `Infinity` — all three pass `^[1-9][0-9]*$`. `parsePrNumber` now requires `Number.isSafeInteger` after the conversion and throws `workflow_dispatch input pr_number is larger than any pull request number: <raw>` otherwise, so nothing reaches `getPull`. New cases in `event-context.test.ts` pin both rejections and accept `9007199254740991` at the boundary. I did not chase a forge contract for the identifier range — the fix does not need one, since a value the runtime cannot represent exactly is refused rather than assumed impossible. Full gate set green (252 tests).
jercik marked this conversation as resolved
fix: bound the dispatch pr_number and stop reading a resolution 404 as a token refusal
Some checks failed
commit-msg / commitlint (pull_request) Successful in 19s
Checks / quality-checks (pull_request) Successful in 46s
Review / Review (pull_request_target) Failing after 18m55s
885f00f11e
`^[1-9][0-9]*$` accepts any number of digits, and `Number(raw)` then rounds past
2^53: dispatching `9007199254740993` fetched and reconciled pull
9007199254740992, and a long enough value asked the forge for `/pulls/Infinity`.
The parse now requires `Number.isSafeInteger`.

The version probe matches the `-j4k` substring, which proves fork lineage and
not that the build serves the resolution route — so a 404 from that route can
equally be an older `-j4k` build, an anchor comment deleted since the sweep, or
a refused token. Only the 403 keeps the token diagnosis; the 404 message now
names all three causes. `resolutionRefusalMessage` becomes
`resolutionDegradationMessage`, which is what both statuses actually produce.
fix: reject a REVIEW_SERVICE_URL that already ends with the /v1 API prefix
All checks were successful
commit-msg / commitlint (pull_request) Successful in 25s
Checks / quality-checks (pull_request) Successful in 42s
Review / Review (pull_request_target) Successful in 13m52s
63e033375a
The capability client builds paths that start with `/v1` and resolves them
against `REVIEW_SERVICE_URL`, so `https://review.j4k.dev/v1` asks the service
for `/v1/v1/reviews` and every call 404s with nothing naming the cause. The
scheme check now has a companion: a base whose path ends in `/v1` is refused up
front. A base mounted under any other path still works.

`credentials.test.ts` used `https://review.j4k.dev/v1` as a valid fixture in
four cases — exactly the value that would have failed at runtime — so those move
to the origin.
Author
Owner

Unadjudicated claim 01M1CCE3JYB9M77XY5FC03EQKA (medium) — "An accepted service URL path duplicates the API prefix"

Round 3's pass listed this under ## Other claims → unadjudicated and never opened a thread for it, so here is the disposition. It reproduces: createCapabilityClient builds paths like /v1/reviews, and buildUrl in the transport resolves them with new URL(path.replace(/^\//, ""), base.endsWith("/") ? base : base + "/"). With REVIEW_SERVICE_URL=https://review.j4k.dev/v1 that gives https://review.j4k.dev/v1/v1/reviews — every call 404s and nothing names the cause.

The confusion was already in this repo: four credentials.test.ts cases used https://review.j4k.dev/v1 as a valid service URL.

Fixed in 63e033375a: serviceOrigin now refuses a base whose path ends in /v1, beside the existing https: check, and the README's environment-contract row says so. A base mounted under any other path (https://review.j4k.dev/review) still resolves correctly and is covered by a test. The four fixtures move to the origin.

Also recording the round-3 pass's coverage: its test-trimming slot came back abandoned and the run exited 1, so that lens did not review head 885f00f. This push starts a fresh pass over the same diff.

> Unadjudicated claim `01M1CCE3JYB9M77XY5FC03EQKA` (medium) — "An accepted service URL path duplicates the API prefix" Round 3's pass listed this under `## Other claims → unadjudicated` and never opened a thread for it, so here is the disposition. It reproduces: `createCapabilityClient` builds paths like `/v1/reviews`, and `buildUrl` in the transport resolves them with `new URL(path.replace(/^\//, ""), base.endsWith("/") ? base : base + "/")`. With `REVIEW_SERVICE_URL=https://review.j4k.dev/v1` that gives `https://review.j4k.dev/v1/v1/reviews` — every call 404s and nothing names the cause. The confusion was already in this repo: four `credentials.test.ts` cases used `https://review.j4k.dev/v1` as a valid service URL. Fixed in 63e033375a4c58f9f146473b7aa39369fb9ef542: `serviceOrigin` now refuses a base whose path ends in `/v1`, beside the existing `https:` check, and the README's environment-contract row says so. A base mounted under any other path (`https://review.j4k.dev/review`) still resolves correctly and is covered by a test. The four fixtures move to the origin. Also recording the round-3 pass's coverage: its `test-trimming` slot came back `abandoned` and the run exited 1, so that lens did not review head 885f00f. This push starts a fresh pass over the same diff.
Author
Owner

Adjudicating round 4's two unadjudicated claims. Neither opened a thread, so the dispositions are here. This is review round 4, where the rule is to fix only clear, severe, demonstrably real bugs and to defer the rest — every push costs another full pass over the whole diff.

01M1CDAX3XN94FZSQSR6CCH8B2 (medium) — "reviewCredentials validates a normalized URL but returns the raw value"

The security reading is refuted, and I checked it rather than deferring it, because the value decides where the capability token goes. serviceOrigin calls new URL(serviceUrl); the transport calls new URL(path, raw.endsWith("/") ? raw : raw + "/"). Same WHATWG parser, same raw string, so the origin cannot diverge. Ran the interesting inputs — leading whitespace, an explicit :443, embedded userinfo (https://user:pw@review.j4k.dev), and the backslash-authority trick (https://review.j4k.dev\@evil.example) — and the validated origin equals the request origin in every one.

What is left is hygiene, not a defect: returning parsed.href instead of the raw string would make the validated value the used value by construction. Deferred to a follow-up PR — the exact change is return { serviceUrl: parsed.href, … } in reviewCredentials (src/credentials.ts), which needs serviceOrigin to hand back the parsed URL rather than its origin.

01M1CDC6XTQVM2JBHM2VEAC401 (medium) — "workflow_dispatch does not recheck open state after the review wait"

Accurate as described. assertDispatchable runs once inside readWrapperInputs, before the orchestrator opens the session, and a pass takes minutes — so a PR merged or closed during the wait still gets its comment surface reconciled.

Acknowledged, not fixed, for two reasons. The round gate: writing comments onto a PR that closed mid-review is untidy, not corruption, and nothing is lost. And it is a design question rather than a defect — the pull_request_target arm has no state check at all and never did, so rechecking on only the dispatch arm would make the two paths disagree. If it should be fixed, the change belongs in the orchestrator after the wait returns, not in assertDispatchable, and it should cover both arms.

Adjudicating round 4's two unadjudicated claims. Neither opened a thread, so the dispositions are here. This is review round 4, where the rule is to fix only clear, severe, demonstrably real bugs and to defer the rest — every push costs another full pass over the whole diff. > `01M1CDAX3XN94FZSQSR6CCH8B2` (medium) — "reviewCredentials validates a normalized URL but returns the raw value" The security reading is refuted, and I checked it rather than deferring it, because the value decides where the capability token goes. `serviceOrigin` calls `new URL(serviceUrl)`; the transport calls `new URL(path, raw.endsWith("/") ? raw : raw + "/")`. Same WHATWG parser, same raw string, so the origin cannot diverge. Ran the interesting inputs — leading whitespace, an explicit `:443`, embedded userinfo (`https://user:pw@review.j4k.dev`), and the backslash-authority trick (`https://review.j4k.dev\@evil.example`) — and the validated origin equals the request origin in every one. What is left is hygiene, not a defect: returning `parsed.href` instead of the raw string would make the validated value the used value by construction. Deferred to a follow-up PR — the exact change is `return { serviceUrl: parsed.href, … }` in `reviewCredentials` (`src/credentials.ts`), which needs `serviceOrigin` to hand back the parsed URL rather than its origin. > `01M1CDC6XTQVM2JBHM2VEAC401` (medium) — "workflow_dispatch does not recheck open state after the review wait" Accurate as described. `assertDispatchable` runs once inside `readWrapperInputs`, before the orchestrator opens the session, and a pass takes minutes — so a PR merged or closed during the wait still gets its comment surface reconciled. Acknowledged, not fixed, for two reasons. The round gate: writing comments onto a PR that closed mid-review is untidy, not corruption, and nothing is lost. And it is a design question rather than a defect — the `pull_request_target` arm has no state check at all and never did, so rechecking on only the dispatch arm would make the two paths disagree. If it should be fixed, the change belongs in the orchestrator after the wait returns, not in `assertDispatchable`, and it should cover both arms.
jercik merged commit cf15181b5b into main 2026-09-01 05:53:44 +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!11
No description provided.