fix: build against @j4k/review 2.15.0 and drop the same-diff request extension #49

jercik · 2026-10-05T10:36:53Z

jercik commented

2026-10-05 10:36:53 +00:00

Bumps @j4k/review from 2.8.0 to 2.15.0, which declares attach_same_diff on the create request, and deletes the WithSameDiff extension that existed to fail tsc once it did.

The rebuilt bundle also brings in the service client's new transport retry (j4k/review#118). Transient connection failures and gateway errors (502/503/504 from a proxy, plus the service's own 503 while it restarts) now retry within a 12 s window, backing off from 250 ms to 4 s. Each attempt times out after 120 s. This runs under the wrapper's own retries (5 attempts, 10 s apart), and the comment in src/review/constants.ts now points at the client's retry policy instead of restating it.

No repository runs this until its workflow pins the merge commit.

🤖 Generated with Claude Code

Bumps `@j4k/review` from 2.8.0 to 2.15.0, which declares `attach_same_diff` on the create request, and deletes the `WithSameDiff` extension that existed to fail `tsc` once it did. The rebuilt bundle also brings in the service client's new transport retry (j4k/review#118). Transient connection failures and gateway errors (502/503/504 from a proxy, plus the service's own 503 while it restarts) now retry within a 12 s window, backing off from 250 ms to 4 s. Each attempt times out after 120 s. This runs under the wrapper's own retries (5 attempts, 10 s apart), and the comment in `src/review/constants.ts` now points at the client's retry policy instead of restating it. No repository runs this until its workflow pins the merge commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-05 10:36:53 +00:00

fix: build against @j4k/review 2.15.0 and drop the same-diff request extension

commit-msg / commitlint (pull_request) Successful in 30s

Details

Dedupe check / dedupe-check (pull_request) Successful in 47s

Details

Checks / quality-checks (pull_request) Successful in 1m5s

Details

Review / Review (pull_request_target) Successful in 3s

Details

844e07166d

The published create-review request now declares `attach_same_diff`, so the
`WithSameDiff` extension is gone and `CreateReviewRequest` is re-exported
from `@j4k/review` directly. The rebuilt bundle also carries the client's
transport retry window (502/503/504 and connection failures, 12 s).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

forgejo-actions commented

2026-10-05 10:46:14 +00:00

Review 01M45TVGSBBGJ86613RSATKBWW — head 42e92433da8e036d26d7df8de96ed8f53b2794b3

Review — j4k-oss/review-wrapper @ `03e359d93b`

Scope: diff against base tree 8869a4f97f96
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

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

Findings (2)

medium — The unsupported-event message copies the accepted event set

claim: 01M45V3MFE5TSP35JTDGZ9Q78Y
anchor: dist/index.mjs (snippet)

    `unsupported GITHUB_EVENT_NAME ${eventName} (expected pull_request_target or workflow_dispatch)`

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M45V7PATAEQ7ECC7P79RHW1V · valid: The exact-grounded diagnostic lists pull_request_target and workflow_dispatch as expected values. The reviewer names readWrapperInputs in src/wrapper/event-context.ts as the runtime source and reports those same successful branches, with no unmatched member. The wrapper's selection is not the forge's externally defined event catalog, and no audience unable to open the source is evidenced. Diagnostic prose is covered by the restated-sets rule. The source-pointer correction retains the rejected value; correcting the reported source template and rebuilding respects bundle generation. The historical claims and their comparisons were reassessed, rather than inheriting their verdicts: they provide no concrete refutation or changed condition. Medium fits the agreeing copy.
disposition: none

Changing the wrapper’s supported events requires a separate edit to this diagnostic. The message currently agrees with the implementation, but can silently give workflow authors a stale list after another event branch is added or removed. The writing standard’s “One Idea, One Place” guidance and the restated-sets rule require a reference to the authoritative definition.

readWrapperInputs in src/wrapper/event-context.ts defines the accepted set through its eventName branches: pull_request_target and workflow_dispatch. The diagnostic lists pull_request_target and workflow_dispatch as the expected values. Neither list has an extra or missing member. This is the wrapper’s selection of runner events, rather than the runner’s externally defined event catalog.

In src/wrapper/event-context.ts, replace the diagnostic with “unsupported GITHUB_EVENT_NAME ${eventName}; see readWrapperInputs in src/wrapper/event-context.ts for supported events”, then rebuild dist/index.mjs. Keep the interpolation as a template expression. This preserves the rejected value and gives the workflow author a current source to consult without maintaining another event list.

I read readWrapperInputs and its terminal error in both src/wrapper/event-context.ts and dist/index.mjs. README.md identifies the committed bundle as generated, and scripts/build-bundle.ts builds it from the entry point configured in src/bundle-options.ts. The correction belongs in the source template, followed by regeneration. This is a static comparison; no workflow was run.

The separately written list in the diagnostic and the accepted-event branches establish the duplication. A diagnostic that points to the definition, or obtains its list from that definition, would remove it.

medium — The retry comment duplicates the failure predicate

claim: 01M45V1G1WCNTNYBBR1EKAFDGA
anchor: src/review/constants.ts (snippet)

// Those calls collapse on their idempotency keys, so retrying transport-level
// failures and 5xx/429 responses is safe. The client's own transport retries
// first, under the policy in @j4k/review/src/client/retry-policy.ts; this ladder
// sits above it.

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M45V7PATAEQ7ECC7P79RHW1V · valid: The exact-grounded comment enumerates transport failures and 5xx/429 responses. The reviewer reports the defining predicate isTransientServiceFailure in src/review/session.ts and supplies its cases: TransportError, ServiceError status 429, and ServiceError status >= 500, with no unmatched category for valid HTTP statuses. This supplies the comparison required by the restated-sets rule; the wrapper selects the set, and the client-policy pointer does not exempt the enumeration beside it. The proposed source pointer preserves the idempotency rationale and retry-layer relationship. Medium fits an agreeing copy that requires a separate update.
duplicates: 01M45V39G0ESEY3EBRNVJWMPNF (restated-sets)
disposition: none

Maintainers must update this comment separately whenever the wrapper changes which failures it retries. Its list currently agrees with the retry logic for HTTP responses, but can silently become stale. The writing standard’s “One Idea, One Place” guidance and the restated-sets rule call for a reference to the definition instead.

The source is isTransientServiceFailure in src/review/session.ts. Its cases are TransportError, ServiceError with status 429, and ServiceError with status >= 500. The comment covers transport-level failures, 5xx responses, and 429 responses; none of those categories is missing or extra for valid HTTP statuses. withRetries calls that predicate before deciding whether to retry.

Replace the first two sentences with: “Those calls collapse on their idempotency keys, so replay is safe. isTransientServiceFailure in session.ts selects retryable failures.” Keep the following reference to the client’s retry-policy.ts and the explanation that this ladder sits above it. This preserves the reason replay is safe and the relationship between retry layers without copying the selected failures.

I read src/review/constants.ts, traced withRetries and isTransientServiceFailure in src/review/session.ts, and checked the corresponding bundled functions in dist/index.mjs. This is a static comparison, not a runtime failure claim.

The separate prose list and executable predicate establish the duplicate source of truth. Removing the list or replacing it with the predicate reference resolves it.

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (1)
- 01M45V39G0ESEY3EBRNVJWMPNF medium — Retry comment duplicates the transient-failure set → 01M45V1G1WCNTNYBBR1EKAFDGA
unadjudicated (0)

Coverage

Coverage pass: 01M45TVGTWK87G3DCER0Z677ZN
Accounting: complete
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	no-claims	1	no
writing-quality	whole	default	claims-emitted	1	no
test-trimming	whole	default	no-claims	1	no
restated-sets	whole	default	claims-emitted	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M45TVGSBBGJ86613RSATKBWW` — head `42e92433da8e036d26d7df8de96ed8f53b2794b3` # Review — j4k-oss/review-wrapper @ 03e359d93b04 Scope: diff against base tree `8869a4f97f96` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (2) ### medium — The unsupported-event message copies the accepted event set - claim: `01M45V3MFE5TSP35JTDGZ9Q78Y` - anchor: `dist/index.mjs` (snippet) ``` `unsupported GITHUB_EVENT_NAME ${eventName} (expected pull_request_target or workflow_dispatch)` ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M45V7PATAEQ7ECC7P79RHW1V` · valid: The exact-grounded diagnostic lists pull_request_target and workflow_dispatch as expected values. The reviewer names readWrapperInputs in src/wrapper/event-context.ts as the runtime source and reports those same successful branches, with no unmatched member. The wrapper's selection is not the forge's externally defined event catalog, and no audience unable to open the source is evidenced. Diagnostic prose is covered by the restated-sets rule. The source-pointer correction retains the rejected value; correcting the reported source template and rebuilding respects bundle generation. The historical claims and their comparisons were reassessed, rather than inheriting their verdicts: they provide no concrete refutation or changed condition. Medium fits the agreeing copy. - disposition: none > Changing the wrapper’s supported events requires a separate edit to this diagnostic. The message currently agrees with the implementation, but can silently give workflow authors a stale list after another event branch is added or removed. The writing standard’s “One Idea, One Place” guidance and the restated-sets rule require a reference to the authoritative definition. > > readWrapperInputs in src/wrapper/event-context.ts defines the accepted set through its eventName branches: pull_request_target and workflow_dispatch. The diagnostic lists pull_request_target and workflow_dispatch as the expected values. Neither list has an extra or missing member. This is the wrapper’s selection of runner events, rather than the runner’s externally defined event catalog. > > In src/wrapper/event-context.ts, replace the diagnostic with “unsupported GITHUB_EVENT_NAME ${eventName}; see readWrapperInputs in src/wrapper/event-context.ts for supported events”, then rebuild dist/index.mjs. Keep the interpolation as a template expression. This preserves the rejected value and gives the workflow author a current source to consult without maintaining another event list. > > I read readWrapperInputs and its terminal error in both src/wrapper/event-context.ts and dist/index.mjs. README.md identifies the committed bundle as generated, and scripts/build-bundle.ts builds it from the entry point configured in src/bundle-options.ts. The correction belongs in the source template, followed by regeneration. This is a static comparison; no workflow was run. > > The separately written list in the diagnostic and the accepted-event branches establish the duplication. A diagnostic that points to the definition, or obtains its list from that definition, would remove it. ### medium — The retry comment duplicates the failure predicate - claim: `01M45V1G1WCNTNYBBR1EKAFDGA` - anchor: `src/review/constants.ts` (snippet) ``` // Those calls collapse on their idempotency keys, so retrying transport-level // failures and 5xx/429 responses is safe. The client's own transport retries // first, under the policy in @j4k/review/src/client/retry-policy.ts; this ladder // sits above it. ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M45V7PATAEQ7ECC7P79RHW1V` · valid: The exact-grounded comment enumerates transport failures and 5xx/429 responses. The reviewer reports the defining predicate isTransientServiceFailure in src/review/session.ts and supplies its cases: TransportError, ServiceError status 429, and ServiceError status >= 500, with no unmatched category for valid HTTP statuses. This supplies the comparison required by the restated-sets rule; the wrapper selects the set, and the client-policy pointer does not exempt the enumeration beside it. The proposed source pointer preserves the idempotency rationale and retry-layer relationship. Medium fits an agreeing copy that requires a separate update. - duplicates: `01M45V39G0ESEY3EBRNVJWMPNF` (restated-sets) - disposition: none > Maintainers must update this comment separately whenever the wrapper changes which failures it retries. Its list currently agrees with the retry logic for HTTP responses, but can silently become stale. The writing standard’s “One Idea, One Place” guidance and the restated-sets rule call for a reference to the definition instead. > > The source is isTransientServiceFailure in src/review/session.ts. Its cases are TransportError, ServiceError with status 429, and ServiceError with status >= 500. The comment covers transport-level failures, 5xx responses, and 429 responses; none of those categories is missing or extra for valid HTTP statuses. withRetries calls that predicate before deciding whether to retry. > > Replace the first two sentences with: “Those calls collapse on their idempotency keys, so replay is safe. isTransientServiceFailure in session.ts selects retryable failures.” Keep the following reference to the client’s retry-policy.ts and the explanation that this ladder sits above it. This preserves the reason replay is safe and the relationship between retry layers without copying the selected failures. > > I read src/review/constants.ts, traced withRetries and isTransientServiceFailure in src/review/session.ts, and checked the corresponding bundled functions in dist/index.mjs. This is a static comparison, not a runtime failure claim. > > The separate prose list and executable predicate establish the duplicate source of truth. Removing the list or replacing it with the predicate reference resolves it. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (1) - `01M45V39G0ESEY3EBRNVJWMPNF` medium — Retry comment duplicates the transient-failure set → `01M45V1G1WCNTNYBBR1EKAFDGA` - unadjudicated (0) ## Coverage Coverage pass: 01M45TVGTWK87G3DCER0Z677ZN Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | claims-emitted | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

forgejo-actions reviewed

2026-10-05 10:46:15 +00:00

src/review/constants.ts Outdated

					
				@ -37,7 +37,8 @@ export const GROUNDING_GRACE_MS = 5 * 60_000;

				// Bounded retries for the create, ask, and re-triage writes (failure row 2).

medium — The retry-limit comment duplicates the session’s retried-operation set

Changing which session operations receive wrapper-level retries requires a separate edit to this comment. The copy currently agrees with the implementation, but it can silently become an incorrect guide for a maintainer adding or removing a retried operation.

The passage presents create, ask, and re-triage as the operations covered by these retry limits. In src/review/session.ts, createReviewSessionWith defines the set through its calls to withRetries: createOrAttach, ask, and retriage. Those correspond to the comment's create, ask, and re-triage; no member appears in only one set. The implementation, rather than this descriptive comment, determines which operations receive retries.

Replace the sentence with // Wrapper-level retry limits for withRetries in session.ts (failure row 2). This preserves the purpose of the constants and the contract reference while making the reader follow the authoritative function instead of maintaining a duplicate operation list. The writing skill's One Idea, One Place guidance and the restated-sets rule call for that pointer even while the copy agrees.

I read src/review/constants.ts and the whole src/review/session.ts, including each operation returned by createReviewSessionWith and the withRetries loop's use of SERVICE_RETRY_ATTEMPTS and SERVICE_RETRY_BACKOFF_MS. This is a static comparison; no runtime test is needed to establish this duplicated description.

The decisive evidence is the set of createReviewSessionWith operations that invoke withRetries. This claim would not apply if the prose were an input that generated that set, or if it merely named the authoritative function; neither is true here.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M45TF21YVT99M3VTAHJP03J0 of review 01M45T6P4HNMEHFMWJTY4MJV24

**medium** — The retry-limit comment duplicates the session’s retried-operation set > Changing which session operations receive wrapper-level retries requires a separate edit to this comment. The copy currently agrees with the implementation, but it can silently become an incorrect guide for a maintainer adding or removing a retried operation. > > The passage presents create, ask, and re-triage as the operations covered by these retry limits. In src/review/session.ts, createReviewSessionWith defines the set through its calls to withRetries: createOrAttach, ask, and retriage. Those correspond to the comment's create, ask, and re-triage; no member appears in only one set. The implementation, rather than this descriptive comment, determines which operations receive retries. > > Replace the sentence with `// Wrapper-level retry limits for withRetries in session.ts (failure row 2).` This preserves the purpose of the constants and the contract reference while making the reader follow the authoritative function instead of maintaining a duplicate operation list. The writing skill's One Idea, One Place guidance and the restated-sets rule call for that pointer even while the copy agrees. > > I read src/review/constants.ts and the whole src/review/session.ts, including each operation returned by createReviewSessionWith and the withRetries loop's use of SERVICE_RETRY_ATTEMPTS and SERVICE_RETRY_BACKOFF_MS. This is a static comparison; no runtime test is needed to establish this duplicated description. > > The decisive evidence is the set of createReviewSessionWith operations that invoke withRetries. This claim would not apply if the prose were an input that generated that set, or if it merely named the authoritative function; neither is true here. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M45TF21YVT99M3VTAHJP03J0` of review `01M45T6P4HNMEHFMWJTY4MJV24`

👍 1

Fixed in 42e92433da. I confirmed withRetries wraps exactly createReview, ask and retriage in src/review/session.ts; the comment now points at withRetries instead of listing those operations.

Fixed in 42e92433da8e036d26d7df8de96ed8f53b2794b3. I confirmed `withRetries` wraps exactly `createReview`, `ask` and `retriage` in `src/review/session.ts`; the comment now points at `withRetries` instead of listing those operations.

jercik marked this conversation as resolved

src/review/constants.ts Outdated

Lines 40-41

					
				@ -38,3 +38,4 @@

				// Those calls collapse on their idempotency keys, so retrying transport-level

				// failures and 5xx/429 responses is safe; the client's own transport already

				// retries connection-level rejections (3 attempts) — this ladder sits above it.

				// retries connection failures and 502/503/504 responses within a 12-second window

				// (backoff from 250 ms to 4 s) — this ladder sits above it.

high — The retry comment restates gateway statuses as unconditional transport retries

A maintainer reading this comment is told that the client's transport retries the listed gateway responses, but an application/problem+json response can bypass those retries. The comment also creates a second copy of the status selection that must be updated when the transport policy changes.

The defining source is GATEWAY_STATUSES and transientGatewayResponse in dist/index.mjs, bundled from @j4k/review/src/client/retry-policy.ts. The source's status members are 502, 503, and 504; the comment covers 502, 503, and 504. No status appears in only one list. The member behavior already disagrees, however: transientGatewayResponse returns false for a problem JSON response whose status is not 503, and for status 503 it returns true only when body.type is /problems/service-stopping. Thus a 502 problem JSON response is not retried despite the comment's unqualified description. fetchWithRetry uses this predicate to decide whether to return the response immediately.

Replace the copied list with a source pointer, for example: "the client's own transport applies its retry policy in @j4k/review/src/client/retry-policy.ts — this ladder sits above it." The response-specific detail already belongs to that predicate. No exception applies: this is a maintained implementation comment, not a dated record, test name, example, external standard, or input read by the system. The policy is available in the committed bundle, and @j4k/review belongs to the owner's j4k/review repository, so it is not a third-party set.

I read src/review/constants.ts, src/review/api.ts, src/review/session.ts, and the bundled transientGatewayResponse, fetchWithRetry, createTransport, and createCapabilityClient call path. This is a static trace, not a live service reproduction. The bundle directly establishes both the copied membership and the excluded response behavior; evidence that the deployed action uses a different transport policy would require reassessing the behavior mismatch.

lens restated-sets · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M45TD9KCHP520CF87VX7CY3B of review 01M45T6P4HNMEHFMWJTY4MJV24

**high** — The retry comment restates gateway statuses as unconditional transport retries > A maintainer reading this comment is told that the client's transport retries the listed gateway responses, but an application/problem+json response can bypass those retries. The comment also creates a second copy of the status selection that must be updated when the transport policy changes. > > The defining source is GATEWAY_STATUSES and transientGatewayResponse in dist/index.mjs, bundled from @j4k/review/src/client/retry-policy.ts. The source's status members are 502, 503, and 504; the comment covers 502, 503, and 504. No status appears in only one list. The member behavior already disagrees, however: transientGatewayResponse returns false for a problem JSON response whose status is not 503, and for status 503 it returns true only when body.type is /problems/service-stopping. Thus a 502 problem JSON response is not retried despite the comment's unqualified description. fetchWithRetry uses this predicate to decide whether to return the response immediately. > > Replace the copied list with a source pointer, for example: "the client's own transport applies its retry policy in `@j4k/review/src/client/retry-policy.ts` — this ladder sits above it." The response-specific detail already belongs to that predicate. No exception applies: this is a maintained implementation comment, not a dated record, test name, example, external standard, or input read by the system. The policy is available in the committed bundle, and @j4k/review belongs to the owner's j4k/review repository, so it is not a third-party set. > > I read src/review/constants.ts, src/review/api.ts, src/review/session.ts, and the bundled transientGatewayResponse, fetchWithRetry, createTransport, and createCapabilityClient call path. This is a static trace, not a live service reproduction. The bundle directly establishes both the copied membership and the excluded response behavior; evidence that the deployed action uses a different transport policy would require reassessing the behavior mismatch. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M45TD9KCHP520CF87VX7CY3B` of review `01M45T6P4HNMEHFMWJTY4MJV24`

👍 1

Fixed in 42e92433da. The comment no longer lists gateway statuses: it names @j4k/review/src/client/retry-policy.ts as the policy (a problem+json 502 or 504 is not retried, and a 503 only when its type is service-stopping). I verified that in transientGatewayResponse at 2.15.0, and the PR body now says "gateway errors" with that exception.

Fixed in 42e92433da8e036d26d7df8de96ed8f53b2794b3. The comment no longer lists gateway statuses: it names `@j4k/review/src/client/retry-policy.ts` as the policy (a problem+json 502 or 504 is not retried, and a 503 only when its type is `service-stopping`). I verified that in `transientGatewayResponse` at 2.15.0, and the PR body now says "gateway errors" with that exception.

jercik marked this conversation as resolved

jercik added 1 commit

2026-10-05 10:48:16 +00:00

fix: point the retry-limit comment at the retry policies it describes

commit-msg / commitlint (pull_request) Successful in 22s

Details

Dedupe check / dedupe-check (pull_request) Successful in 36s

Details

Checks / quality-checks (pull_request) Successful in 49s

Details

Review / Review (pull_request_target) Successful in 8m38s

Details

42e92433da

The comment restated which gateway responses the client transport retries
(problem+json 502/504 are not) and which operations `withRetries` wraps.
It now names the files that define both.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

forgejo-actions reviewed

2026-10-05 10:56:53 +00:00

src/review/constants.ts

Lines 38-41

					
				@ -38,3 +37,5 @@

				// Retry limits for the calls `withRetries` wraps in session.ts (failure row 2).

				// Those calls collapse on their idempotency keys, so retrying transport-level

				// failures and 5xx/429 responses is safe; the client's own transport already

				// retries connection-level rejections (3 attempts) — this ladder sits above it.

				// failures and 5xx/429 responses is safe. The client's own transport retries

				// first, under the policy in @j4k/review/src/client/retry-policy.ts; this ladder

				// sits above it.

medium — The retry comment duplicates the failure predicate

Maintainers must update this comment separately whenever the wrapper changes which failures it retries. Its list currently agrees with the retry logic for HTTP responses, but can silently become stale. The writing standard’s “One Idea, One Place” guidance and the restated-sets rule call for a reference to the definition instead.

The source is isTransientServiceFailure in src/review/session.ts. Its cases are TransportError, ServiceError with status 429, and ServiceError with status >= 500. The comment covers transport-level failures, 5xx responses, and 429 responses; none of those categories is missing or extra for valid HTTP statuses. withRetries calls that predicate before deciding whether to retry.

Replace the first two sentences with: “Those calls collapse on their idempotency keys, so replay is safe. isTransientServiceFailure in session.ts selects retryable failures.” Keep the following reference to the client’s retry-policy.ts and the explanation that this ladder sits above it. This preserves the reason replay is safe and the relationship between retry layers without copying the selected failures.

I read src/review/constants.ts, traced withRetries and isTransientServiceFailure in src/review/session.ts, and checked the corresponding bundled functions in dist/index.mjs. This is a static comparison, not a runtime failure claim.

The separate prose list and executable predicate establish the duplicate source of truth. Removing the list or replacing it with the predicate reference resolves it.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M45V1G1WCNTNYBBR1EKAFDGA of review 01M45TVGSBBGJ86613RSATKBWW

**medium** — The retry comment duplicates the failure predicate > Maintainers must update this comment separately whenever the wrapper changes which failures it retries. Its list currently agrees with the retry logic for HTTP responses, but can silently become stale. The writing standard’s “One Idea, One Place” guidance and the restated-sets rule call for a reference to the definition instead. > > The source is isTransientServiceFailure in src/review/session.ts. Its cases are TransportError, ServiceError with status 429, and ServiceError with status >= 500. The comment covers transport-level failures, 5xx responses, and 429 responses; none of those categories is missing or extra for valid HTTP statuses. withRetries calls that predicate before deciding whether to retry. > > Replace the first two sentences with: “Those calls collapse on their idempotency keys, so replay is safe. isTransientServiceFailure in session.ts selects retryable failures.” Keep the following reference to the client’s retry-policy.ts and the explanation that this ladder sits above it. This preserves the reason replay is safe and the relationship between retry layers without copying the selected failures. > > I read src/review/constants.ts, traced withRetries and isTransientServiceFailure in src/review/session.ts, and checked the corresponding bundled functions in dist/index.mjs. This is a static comparison, not a runtime failure claim. > > The separate prose list and executable predicate establish the duplicate source of truth. Removing the list or replacing it with the predicate reference resolves it. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M45V1G1WCNTNYBBR1EKAFDGA` of review `01M45TVGSBBGJ86613RSATKBWW`

🚀 1

jercik marked this conversation as resolved

jercik commented

2026-10-05 10:57:58 +00:00

Replying to review comment #128713

Not changed here. The sentence about idempotency keys and 5xx/429 responses is unchanged from main, and it agrees with isTransientServiceFailure in src/review/session.ts today, so it does not break what this PR delivers. Pointing it at the predicate is a separate documentation change. No follow-up PR is opened from this PR.

> Replying to review comment #128713 Not changed here. The sentence about idempotency keys and 5xx/429 responses is unchanged from `main`, and it agrees with `isTransientServiceFailure` in `src/review/session.ts` today, so it does not break what this PR delivers. Pointing it at the predicate is a separate documentation change. No follow-up PR is opened from this PR.

jercik commented

2026-10-05 10:57:58 +00:00

Replying to review summary comment #128598, finding 01M45V3MFE5TSP35JTDGZ9Q78Y (the unsupported-event message)

Not changed here. The message in src/wrapper/event-context.ts dates from the initial implementation (#4), this PR does not touch it, and its two events match the parser's branches today. Rewording it changes the bundle's runtime output and a test, which does not belong in a dependency bump. No follow-up PR is opened from this PR.

> Replying to review summary comment #128598, finding `01M45V3MFE5TSP35JTDGZ9Q78Y` (the unsupported-event message) Not changed here. The message in `src/wrapper/event-context.ts` dates from the initial implementation (#4), this PR does not touch it, and its two events match the parser's branches today. Rewording it changes the bundle's runtime output and a test, which does not belong in a dependency bump. No follow-up PR is opened from this PR.

jercik merged commit 49be49d39e into main

2026-10-05 10:58:40 +00:00

jercik referenced this pull request from a commit

2026-10-05 10:58:41 +00:00

fix: build against `@j4k/review` 2.15.0 and drop the same-diff request extension (#49)

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!49

				`@ -37,7 +37,8 @@ export const GROUNDING_GRACE_MS = 5 * 60_000;`
				`// Bounded retries for the create, ask, and re-triage writes (failure row 2).`

Rows
Columns

fix: build against @j4k/review 2.15.0 and drop the same-diff request extension #49

Review — j4k-oss/review-wrapper @ 03e359d93b

Findings (2)

medium — The unsupported-event message copies the accepted event set

medium — The retry comment duplicates the failure predicate

Other claims

Coverage

fix: build against `@j4k/review` 2.15.0 and drop the same-diff request extension #49

Review — j4k-oss/review-wrapper @ `03e359d93b`