docs: say which reads a superseded run keeps on its pass #38

jercik · 2026-10-04T22:44:23Z

jercik commented

2026-10-04 22:44:23 +00:00

Stacked on #36 (docs/readme-pass-pointers); merge #36 first.

#36 ends the README's supersession paragraph with "the run does not switch to the newer pass". Review 01M44GNBZGWES4QQP6295GRM6H on #36 pointed out that a reader can take this to mean every read stays on the pinned pass. Only the requests that take a pass id do: getCoverage and getReport always receive the pinned pass id, while getReview, getFindings and listClaims read review-wide state.

The paragraph now states that split and points at ReviewSession in src/contract/types.ts for which requests take a pass id, rather than listing them, so the README doesn't restate a set the interface defines.

🤖 Generated with Claude Code

Stacked on #36 (`docs/readme-pass-pointers`); merge #36 first. #36 ends the README's supersession paragraph with "the run does not switch to the newer pass". Review `01M44GNBZGWES4QQP6295GRM6H` on #36 pointed out that a reader can take this to mean every read stays on the pinned pass. Only the requests that take a pass id do: `getCoverage` and `getReport` always receive the pinned pass id, while `getReview`, `getFindings` and `listClaims` read review-wide state. The paragraph now states that split and points at `ReviewSession` in `src/contract/types.ts` for which requests take a pass id, rather than listing them, so the README doesn't restate a set the interface defines. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-04 22:44:23 +00:00

docs: say which reads a superseded run keeps on its pass

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

Details

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

Details

Review / Review (pull_request_target) Has been cancelled

Details

54c59d6d6a

The old clause implied every read stayed on the pinned pass. Only requests that take a
pass id do; the run's other reads return review-wide state.

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

jercik referenced this pull request

2026-10-04 22:44:33 +00:00

docs: drop the pass-bound operation list from the README #36

forgejo-actions commented

2026-10-04 22:54:47 +00:00

Review 01M45PHHD2G6A5J522MPPMXFVA — head dedc05a6dc2fa7556f0054b05025564e3e96cacc

Review — j4k-oss/review-wrapper @ `d4e1d10f79`

Scope: diff against base tree 397321d6ad0c
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 (3)

medium — The environment table duplicates the required credential-variable set

claim: 01M45PWDGDAH20P0XA5N2GQMNN
anchor: README.md (snippet)

| `REVIEW_SERVICE_URL`                 | Base URL of the review service (an org-managed variable). `serviceOrigin` in `src/credentials.ts` gives the form it must take; the client appends the `/v1` API prefix itself.                                  |
| `REVIEW_CAPABILITY_TOKEN`            | Capability token for the service's `/v1` API (an org secret).                                                                                                                                                   |
| `FORGEJO_TOKEN`                      | The job's own task token, used for every forge write.                                                                                                                                                           |
| `GITHUB_API_URL`                     | Forge API base, supplied by the runner.                                                                                                                                                                         |

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M45PZVKG4AS62CQ34F9KD84T · valid: The grounded table enumerates credential variables. The reviewer names src/credentials.ts and its forgeCredentials/reviewCredentials requireEnv calls, lists the source members and copy members, and reports that the lists agree. This supplies the essential restated-set comparison; agreement makes the duplication medium rather than disproving it. No source-governing or inaccessible-source exception is evidenced. The proposed source pointer and relocation of org-variable/org-secret annotations preserve the setup and security facts without maintaining a second inventory.
disposition: none

The credential rows create a second inventory of required variables that must be edited whenever credential acquisition changes. The copy currently agrees with the code, but a future change can silently leave setup readers relying on an obsolete inventory. The writing standard's “One Idea, One Place” guidance prefers the environment's authoritative definitions, and the restated-sets guideline treats a matching copy as a defect.

The source is the requireEnv calls in forgeCredentials and reviewCredentials in src/credentials.ts. For required credential variables, it defines REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, and GITHUB_API_URL. The anchored rows cover REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, and GITHUB_API_URL. No member occurs in only one list. The optional action-origin input is a separate setting, already documented through the README's pointer to action.yml.

Replace these rows with: “Credential variables and their validation are defined by forgeCredentials and reviewCredentials in src/credentials.ts.” Put the org-managed variable and org-secret annotations beside their corresponding reads in that source file; the task-token annotation already appears in forgeCredentials. The remaining environment discussion can keep the fork-gate requirement and runner-specific behavior without repeating a credential inventory. This preserves the configuration and security meaning while giving each variable's definition and annotation one home.

I read the full README, src/credentials.ts, src/credentials.test.ts, src/main.ts, src/wrapper/event-context.ts, and action.yml. This is a static comparison of the table against the actual credential readers; no runtime failure is asserted.

The decisive evidence is that both credential readers select the same required variables independently of the Markdown table. The claim would be refuted if the table generated or governed those reads, or if its intended readers could not open the named source; neither relationship appears in the examined files.

medium — The security summary repeats token-reader and internal-import counts

claim: 01M45PY6SDQYEVGTNQVTQ91K6J
anchor: README.md (snippet)

- The capability token is read in exactly one module with zero internal imports
  (`src/credentials.ts`) and flows only into the `Authorization` header of requests to
  `REVIEW_SERVICE_URL`. That variable is parsed as an `https:` URL before the token is
  read, and pinning `expected-service-origin` in the calling workflow makes a repointed
  org variable fail the run rather than post the token somewhere else.

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M45PZVKG4AS62CQ34F9KD84T · valid: The exact-grounded paragraph copies counts of token-reader modules and internal imports. The reviewer identifies src/credentials.ts as the token-reader source, lists its singleton reader set and empty internal-import set, and reports that the counts match. The counts describe source-defined inventories, not numerical premises of a capacity argument; tests checking isolation do not make the prose authoritative. Pointing to token acquisition and its isolation check removes the matching copies while retaining the Authorization flow, HTTPS-before-read ordering, and origin-pinning rationale. This is valid at medium severity.
disposition: none

The security explanation maintains numerical copies of source-defined inventories. They currently agree, but changes to token acquisition or the module's imports require a separate README edit to keep its security account accurate. The restated-sets guideline requires replacing matching copies with their source; the writing standard's “One Idea, One Place” guidance makes that correction useful without weakening the security boundary.

The paragraph says the capability token is read in “exactly one module” and that the module has “zero internal imports.” The production token-acquisition source found under src is src/credentials.ts: reviewCredentials calls requireEnv with REVIEW_CAPABILITY_TOKEN there. The source token-reader set therefore contains src/credentials.ts, and the prose's count of one matches. That file imports node:process and has no internal imports, so its internal-import set is empty and the prose's count of zero matches. Neither comparison exposes a missing or extra member. The isolation assertions in src/credentials.test.ts check the implementation; they do not make the README counts authoritative.

Replace the opening inventory with “Token acquisition is defined in src/credentials.ts, whose module-isolation boundary is checked by src/credentials.test.ts.” Continue with the existing statement about the Authorization header and retain the HTTPS-before-token-read ordering and expected-service-origin rationale. This keeps the security mechanism, its enforcement location, and the origin-pinning reason while removing both independently maintained counts.

I read the full README, src/credentials.ts, src/credentials.test.ts, and src/main.ts, and searched production TypeScript under src for REVIEW_CAPABILITY_TOKEN using rg. The matches outside credentials.ts describe errors or remedies rather than reading the token. The comparison is static; I did not execute the tests or inspect the external capability-client implementation.

The decisive evidence is the token read and import declarations in src/credentials.ts. This claim would be refuted if the numerical prose governed or generated those declarations, rather than describing them; no such connection appears in the examined files.

low — The README repeats internal request-scoping behavior

claim: 01M45PTWYTA0XX8ZZQ6MQ7NMNX
anchor: README.md (snippet)

Requests that take a pass
id keep the selected pass, and the run's other reads return review-wide state;
`ReviewSession` in `src/contract/types.ts` shows which requests take one.

lens: project-docs · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M45PZVKG4AS62CQ34F9KD84T · valid: The exact-grounded README passage describes internal request scoping. The reviewer reports that the diff adds it and supplies the matching ReviewSession signatures and reconcile calls: getReport takes the pinned pass, while getFindings takes only the review id. That concrete comparison supports a touched-prose placement violation under project-docs' division of labor and consolidation rules. The passage gives no installation or user procedure, decision rationale, or recorded README convention that would invoke an exception. Removing this implementation explanation leaves its authoritative home in code. Valid at low severity.
disposition: none

The edited passage adds a second description of the session's internal request scoping, requiring maintainers to keep onboarding prose synchronized with the request interface and its callers. This violates project-docs' Division of labor rule that how the system works belongs in code, and its Consolidating stray documentation rule to delete touched prose that restates code.

The diff expands the supersession paragraph with: “Requests that take a pass
id keep the selected pass, and the run's other reads return review-wide state;
ReviewSession in src/contract/types.ts shows which requests take one.” The existing home is the code: ReviewSession declares readonly getReport: (reviewId: string, passId: string) => Promise<string>; and readonly getFindings: (reviewId: string) => Promise<FindingsResponse>;. In src/wrapper/orchestrator.ts, reconcile calls const report = await session.getReport(reviewId, pinnedPassId); and const findings = known ?? (await session.getFindings(reviewId));. These establish the same distinction directly.

Remove the added per-request scoping explanation from README.md and keep that knowledge in src/contract/types.ts and the callers. The README exception permits human onboarding, installation and usage; this passage describes internal method arguments and read behavior, without an installation step, user action or decision rationale. Neither README.md nor AGENTS.md records a convention assigning implementation explanations to the README. This is a placement finding, not a claim that the description is false.

I read README.md in full, the complete diff, AGENTS.md, ReviewSession and WaitResult in src/contract/types.ts, waitForSettle and readSettle in src/wrapper/wait.ts, createReviewSessionWith in src/review/session.ts, and pinPass, review and reconcile in src/wrapper/orchestrator.ts. The conclusion is based on static inspection; no runtime reproduction was needed.

A recorded repository convention explicitly keeping internal behavior descriptions in the README would refute this placement finding. The inspected documentation provides no such exception.

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (0)

Coverage

Coverage pass: 01M45PHHGV3B7YB237PJHWYGJ6
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	no-claims	1	no
project-docs	whole	default	claims-emitted	1	no

**Review** `01M45PHHD2G6A5J522MPPMXFVA` — head `dedc05a6dc2fa7556f0054b05025564e3e96cacc` # Review — j4k-oss/review-wrapper @ d4e1d10f7991 Scope: diff against base tree `397321d6ad0c` 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 (3) ### medium — The environment table duplicates the required credential-variable set - claim: `01M45PWDGDAH20P0XA5N2GQMNN` - anchor: `README.md` (snippet) ``` | `REVIEW_SERVICE_URL` | Base URL of the review service (an org-managed variable). `serviceOrigin` in `src/credentials.ts` gives the form it must take; the client appends the `/v1` API prefix itself. | | `REVIEW_CAPABILITY_TOKEN` | Capability token for the service's `/v1` API (an org secret). | | `FORGEJO_TOKEN` | The job's own task token, used for every forge write. | | `GITHUB_API_URL` | Forge API base, supplied by the runner. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M45PZVKG4AS62CQ34F9KD84T` · valid: The grounded table enumerates credential variables. The reviewer names src/credentials.ts and its forgeCredentials/reviewCredentials requireEnv calls, lists the source members and copy members, and reports that the lists agree. This supplies the essential restated-set comparison; agreement makes the duplication medium rather than disproving it. No source-governing or inaccessible-source exception is evidenced. The proposed source pointer and relocation of org-variable/org-secret annotations preserve the setup and security facts without maintaining a second inventory. - disposition: none > The credential rows create a second inventory of required variables that must be edited whenever credential acquisition changes. The copy currently agrees with the code, but a future change can silently leave setup readers relying on an obsolete inventory. The writing standard's “One Idea, One Place” guidance prefers the environment's authoritative definitions, and the restated-sets guideline treats a matching copy as a defect. > > The source is the requireEnv calls in forgeCredentials and reviewCredentials in src/credentials.ts. For required credential variables, it defines REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, and GITHUB_API_URL. The anchored rows cover REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, and GITHUB_API_URL. No member occurs in only one list. The optional action-origin input is a separate setting, already documented through the README's pointer to action.yml. > > Replace these rows with: “Credential variables and their validation are defined by forgeCredentials and reviewCredentials in src/credentials.ts.” Put the org-managed variable and org-secret annotations beside their corresponding reads in that source file; the task-token annotation already appears in forgeCredentials. The remaining environment discussion can keep the fork-gate requirement and runner-specific behavior without repeating a credential inventory. This preserves the configuration and security meaning while giving each variable's definition and annotation one home. > > I read the full README, src/credentials.ts, src/credentials.test.ts, src/main.ts, src/wrapper/event-context.ts, and action.yml. This is a static comparison of the table against the actual credential readers; no runtime failure is asserted. > > The decisive evidence is that both credential readers select the same required variables independently of the Markdown table. The claim would be refuted if the table generated or governed those reads, or if its intended readers could not open the named source; neither relationship appears in the examined files. ### medium — The security summary repeats token-reader and internal-import counts - claim: `01M45PY6SDQYEVGTNQVTQ91K6J` - anchor: `README.md` (snippet) ``` - The capability token is read in exactly one module with zero internal imports (`src/credentials.ts`) and flows only into the `Authorization` header of requests to `REVIEW_SERVICE_URL`. That variable is parsed as an `https:` URL before the token is read, and pinning `expected-service-origin` in the calling workflow makes a repointed org variable fail the run rather than post the token somewhere else. ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M45PZVKG4AS62CQ34F9KD84T` · valid: The exact-grounded paragraph copies counts of token-reader modules and internal imports. The reviewer identifies src/credentials.ts as the token-reader source, lists its singleton reader set and empty internal-import set, and reports that the counts match. The counts describe source-defined inventories, not numerical premises of a capacity argument; tests checking isolation do not make the prose authoritative. Pointing to token acquisition and its isolation check removes the matching copies while retaining the Authorization flow, HTTPS-before-read ordering, and origin-pinning rationale. This is valid at medium severity. - disposition: none > The security explanation maintains numerical copies of source-defined inventories. They currently agree, but changes to token acquisition or the module's imports require a separate README edit to keep its security account accurate. The restated-sets guideline requires replacing matching copies with their source; the writing standard's “One Idea, One Place” guidance makes that correction useful without weakening the security boundary. > > The paragraph says the capability token is read in “exactly one module” and that the module has “zero internal imports.” The production token-acquisition source found under src is src/credentials.ts: reviewCredentials calls requireEnv with REVIEW_CAPABILITY_TOKEN there. The source token-reader set therefore contains src/credentials.ts, and the prose's count of one matches. That file imports node:process and has no internal imports, so its internal-import set is empty and the prose's count of zero matches. Neither comparison exposes a missing or extra member. The isolation assertions in src/credentials.test.ts check the implementation; they do not make the README counts authoritative. > > Replace the opening inventory with “Token acquisition is defined in src/credentials.ts, whose module-isolation boundary is checked by src/credentials.test.ts.” Continue with the existing statement about the Authorization header and retain the HTTPS-before-token-read ordering and expected-service-origin rationale. This keeps the security mechanism, its enforcement location, and the origin-pinning reason while removing both independently maintained counts. > > I read the full README, src/credentials.ts, src/credentials.test.ts, and src/main.ts, and searched production TypeScript under src for REVIEW_CAPABILITY_TOKEN using rg. The matches outside credentials.ts describe errors or remedies rather than reading the token. The comparison is static; I did not execute the tests or inspect the external capability-client implementation. > > The decisive evidence is the token read and import declarations in src/credentials.ts. This claim would be refuted if the numerical prose governed or generated those declarations, rather than describing them; no such connection appears in the examined files. ### low — The README repeats internal request-scoping behavior - claim: `01M45PTWYTA0XX8ZZQ6MQ7NMNX` - anchor: `README.md` (snippet) ``` Requests that take a pass id keep the selected pass, and the run's other reads return review-wide state; `ReviewSession` in `src/contract/types.ts` shows which requests take one. ``` - lens: project-docs · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M45PZVKG4AS62CQ34F9KD84T` · valid: The exact-grounded README passage describes internal request scoping. The reviewer reports that the diff adds it and supplies the matching ReviewSession signatures and reconcile calls: getReport takes the pinned pass, while getFindings takes only the review id. That concrete comparison supports a touched-prose placement violation under project-docs' division of labor and consolidation rules. The passage gives no installation or user procedure, decision rationale, or recorded README convention that would invoke an exception. Removing this implementation explanation leaves its authoritative home in code. Valid at low severity. - disposition: none > The edited passage adds a second description of the session's internal request scoping, requiring maintainers to keep onboarding prose synchronized with the request interface and its callers. This violates project-docs' Division of labor rule that how the system works belongs in code, and its Consolidating stray documentation rule to delete touched prose that restates code. > > The diff expands the supersession paragraph with: “Requests that take a pass > id keep the selected pass, and the run's other reads return review-wide state; > `ReviewSession` in `src/contract/types.ts` shows which requests take one.” The existing home is the code: `ReviewSession` declares <code>readonly getReport: (reviewId: string, passId: string) => Promise<string>;</code> and <code>readonly getFindings: (reviewId: string) => Promise<FindingsResponse>;</code>. In `src/wrapper/orchestrator.ts`, `reconcile` calls `const report = await session.getReport(reviewId, pinnedPassId);` and `const findings = known ?? (await session.getFindings(reviewId));`. These establish the same distinction directly. > > Remove the added per-request scoping explanation from README.md and keep that knowledge in src/contract/types.ts and the callers. The README exception permits human onboarding, installation and usage; this passage describes internal method arguments and read behavior, without an installation step, user action or decision rationale. Neither README.md nor AGENTS.md records a convention assigning implementation explanations to the README. This is a placement finding, not a claim that the description is false. > > I read README.md in full, the complete diff, AGENTS.md, ReviewSession and WaitResult in src/contract/types.ts, waitForSettle and readSettle in src/wrapper/wait.ts, createReviewSessionWith in src/review/session.ts, and pinPass, review and reconcile in src/wrapper/orchestrator.ts. The conclusion is based on static inspection; no runtime reproduction was needed. > > A recorded repository convention explicitly keeping internal behavior descriptions in the README would refute this placement finding. The inspected documentation provides no such exception. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M45PHHGV3B7YB237PJHWYGJ6 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 | no-claims | 1 | no | | project-docs | whole | default | claims-emitted | 1 | no |

🚀 1

jercik commented

2026-10-04 23:40:22 +00:00

Replying to comment #124044

This is review 01M44HE24F4WZG8A902EQV15C7 of 54c59d6. No pass has delivered all five slots yet: runs 64118, 64227 and 64315 lost slots to model capacity, and dispatch run 64427 lost test-trimming and project-docs to sandbox infrastructure failures. I'll re-ask once the sandbox recovers. Every finding so far is about README text this PR doesn't change, and the PR that owns the text already handles it:

01M44HQ90E89BEP5ZTDNXYDJT6 (required environment variables copied into a list) and 01M44HSV19PCQSVGSNAFH4FRWZ (composition paragraph lists the reconcilers) are valid. #32 replaces the list with pointers to forgeCredentials, readWrapperInputs and src/main.ts, and the reconcilers with a pointer to WrapperDeps.
01M44HS3NYF71ZPHYVD1C3Z4RM (action-input table) and 01M44HTYDVZHTWXW1P5PWGTP4W (recovery table) are valid. #34 points the README at inputs in action.yml and at the remedy printed on the failure log line.

The eight duplicate claims are covered by the findings they duplicate. Triage rejected 01M44JGSK19ZBKVHC2N42RBVYP and 01M44M14MBQVYPBV8EKSK7J2RW (the environment table copies the variables the wrapper reads). Replacing that table is held in any case, as 01M44DH5HKQFA3T9JJSGAEFP8M was on #30: it is the only place that tells a calling workflow which variables to set and what each must hold.

> Replying to comment #124044 This is review `01M44HE24F4WZG8A902EQV15C7` of `54c59d6`. No pass has delivered all five slots yet: runs 64118, 64227 and 64315 lost slots to model capacity, and dispatch run 64427 lost test-trimming and project-docs to sandbox infrastructure failures. I'll re-ask once the sandbox recovers. Every finding so far is about README text this PR doesn't change, and the PR that owns the text already handles it: - `01M44HQ90E89BEP5ZTDNXYDJT6` (required environment variables copied into a list) and `01M44HSV19PCQSVGSNAFH4FRWZ` (composition paragraph lists the reconcilers) are valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 replaces the list with pointers to `forgeCredentials`, `readWrapperInputs` and `src/main.ts`, and the reconcilers with a pointer to `WrapperDeps`. - `01M44HS3NYF71ZPHYVD1C3Z4RM` (action-input table) and `01M44HTYDVZHTWXW1P5PWGTP4W` (recovery table) are valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34 points the README at `inputs` in `action.yml` and at the remedy printed on the failure log line. The eight duplicate claims are covered by the findings they duplicate. Triage rejected `01M44JGSK19ZBKVHC2N42RBVYP` and `01M44M14MBQVYPBV8EKSK7J2RW` (the environment table copies the variables the wrapper reads). Replacing that table is held in any case, as `01M44DH5HKQFA3T9JJSGAEFP8M` was on #30: it is the only place that tells a calling workflow which variables to set and what each must hold.

jercik commented

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

Replying to comment #124044

Dispatch run 64933 delivered all five slots for 54c59d6 and added nothing: the report still has the same four findings, two rejected claims and eight duplicates. The dispositions in #38 (comment) stand.

> Replying to comment #124044 Dispatch run 64933 delivered all five slots for `54c59d6` and added nothing: the report still has the same four findings, two rejected claims and eight duplicates. The dispositions in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/38#issuecomment-124796 stand.

jercik commented

2026-10-05 01:07:10 +00:00

Replying to comment #124044

Replacing the environment table is declined, not held. Triage rejected 01M44JGSK19ZBKVHC2N42RBVYP and 01M44M14MBQVYPBV8EKSK7J2RW, and the table stays for the reason given on #30: it is the usage contract for someone wiring the action into another repository, and no single definition exists to point at.

> Replying to comment #124044 Replacing the environment table is declined, not held. Triage rejected `01M44JGSK19ZBKVHC2N42RBVYP` and `01M44M14MBQVYPBV8EKSK7J2RW`, and the table stays for the reason given on https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30: it is the usage contract for someone wiring the action into another repository, and no single definition exists to point at.

jercik changed target branch from docs/readme-pass-pointers to main

2026-10-05 09:32:52 +00:00

jercik force-pushed docs/supersession-pass-scope from 54c59d6d6a

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

Details

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

Details

Review / Review (pull_request_target) Has been cancelled

Details

to dedc05a6dc

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

Details

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

Details

Review / Review (pull_request_target) Successful in 9m55s

Details

2026-10-05 09:32:55 +00:00

Compare

jercik merged commit dc0fad6c8c into main

2026-10-05 09:33:37 +00:00

jercik referenced this pull request from a commit

2026-10-05 09:33:38 +00:00

docs: say which reads a superseded run keeps on its pass (#38)

forgejo-actions reviewed

2026-10-05 09:42:49 +00:00

README.md

Lines 83-85

					
				@ -82,3 +82,4 @@

				Supersession, a newer dispatch pass becoming latest during the wait, is a flag on

				`WaitResult`, not an outcome. It never changes the exit code, and the run does not switch

				to the newer pass.

				`WaitResult`, not an outcome. It never changes the exit code. Requests that take a pass

				id keep the selected pass, and the run's other reads return review-wide state;

				`ReviewSession` in `src/contract/types.ts` shows which requests take one.

low — The README repeats internal request-scoping behavior

The edited passage adds a second description of the session's internal request scoping, requiring maintainers to keep onboarding prose synchronized with the request interface and its callers. This violates project-docs' Division of labor rule that how the system works belongs in code, and its Consolidating stray documentation rule to delete touched prose that restates code.

The diff expands the supersession paragraph with: “Requests that take a pass
id keep the selected pass, and the run's other reads return review-wide state;
ReviewSession in src/contract/types.ts shows which requests take one.” The existing home is the code: ReviewSession declares readonly getReport: (reviewId: string, passId: string) => Promise<string>; and readonly getFindings: (reviewId: string) => Promise<FindingsResponse>;. In src/wrapper/orchestrator.ts, reconcile calls const report = await session.getReport(reviewId, pinnedPassId); and const findings = known ?? (await session.getFindings(reviewId));. These establish the same distinction directly.

Remove the added per-request scoping explanation from README.md and keep that knowledge in src/contract/types.ts and the callers. The README exception permits human onboarding, installation and usage; this passage describes internal method arguments and read behavior, without an installation step, user action or decision rationale. Neither README.md nor AGENTS.md records a convention assigning implementation explanations to the README. This is a placement finding, not a claim that the description is false.

I read README.md in full, the complete diff, AGENTS.md, ReviewSession and WaitResult in src/contract/types.ts, waitForSettle and readSettle in src/wrapper/wait.ts, createReviewSessionWith in src/review/session.ts, and pinPass, review and reconcile in src/wrapper/orchestrator.ts. The conclusion is based on static inspection; no runtime reproduction was needed.

A recorded repository convention explicitly keeping internal behavior descriptions in the README would refute this placement finding. The inspected documentation provides no such exception.

lens project-docs · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M45PTWYTA0XX8ZZQ6MQ7NMNX of review 01M45PHHD2G6A5J522MPPMXFVA

**low** — The README repeats internal request-scoping behavior > The edited passage adds a second description of the session's internal request scoping, requiring maintainers to keep onboarding prose synchronized with the request interface and its callers. This violates project-docs' Division of labor rule that how the system works belongs in code, and its Consolidating stray documentation rule to delete touched prose that restates code. > > The diff expands the supersession paragraph with: “Requests that take a pass > id keep the selected pass, and the run's other reads return review-wide state; > `ReviewSession` in `src/contract/types.ts` shows which requests take one.” The existing home is the code: `ReviewSession` declares <code>readonly getReport: (reviewId: string, passId: string) => Promise<string>;</code> and <code>readonly getFindings: (reviewId: string) => Promise<FindingsResponse>;</code>. In `src/wrapper/orchestrator.ts`, `reconcile` calls `const report = await session.getReport(reviewId, pinnedPassId);` and `const findings = known ?? (await session.getFindings(reviewId));`. These establish the same distinction directly. > > Remove the added per-request scoping explanation from README.md and keep that knowledge in src/contract/types.ts and the callers. The README exception permits human onboarding, installation and usage; this passage describes internal method arguments and read behavior, without an installation step, user action or decision rationale. Neither README.md nor AGENTS.md records a convention assigning implementation explanations to the README. This is a placement finding, not a claim that the description is false. > > I read README.md in full, the complete diff, AGENTS.md, ReviewSession and WaitResult in src/contract/types.ts, waitForSettle and readSettle in src/wrapper/wait.ts, createReviewSessionWith in src/review/session.ts, and pinPass, review and reconcile in src/wrapper/orchestrator.ts. The conclusion is based on static inspection; no runtime reproduction was needed. > > A recorded repository convention explicitly keeping internal behavior descriptions in the README would refute this placement finding. The inspected documentation provides no such exception. lens `project-docs` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M45PTWYTA0XX8ZZQ6MQ7NMNX` of review `01M45PHHD2G6A5J522MPPMXFVA`

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

Rows
Columns