docs: correct the service URL and PR-input rows of the environment table #37

Merged
jercik merged 3 commits from docs/env-table-accuracy into main 2026-10-05 09:32:51 +00:00
Owner

Answers the review of #34 (claims 01M44EVPVQHRSTQHN71HRTHTSK and 01M44EX41M27Y04WKDRRFP5MMH): two rows of the environment contract table claimed more than the code does.

REVIEW_SERVICE_URL is rejected when its path ends in /v1/ as well as /v1. On workflow_dispatch the event payload supplies only pr_number; the head SHA, branches, repository and state come from the forge.

🤖 Generated with Claude Code

Answers the [review of #34](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34#issuecomment-123243) (claims `01M44EVPVQHRSTQHN71HRTHTSK` and `01M44EX41M27Y04WKDRRFP5MMH`): two rows of the environment contract table claimed more than the code does. `REVIEW_SERVICE_URL` is rejected when its path ends in `/v1/` as well as `/v1`. On `workflow_dispatch` the event payload supplies only `pr_number`; the head SHA, branches, repository and state come from the forge. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
docs: correct the service URL and PR-input rows of the environment table
Some checks failed
commit-msg / commitlint (pull_request) Successful in 24s
Checks / quality-checks (pull_request) Successful in 49s
Review / Review (pull_request_target) Failing after 7m24s
6c997aa578
`REVIEW_SERVICE_URL` is rejected when its path ends in `/v1` or `/v1/`, not
only `/v1`. PR inputs come from the event payload on `pull_request_target`;
`workflow_dispatch` reads only `pr_number` there and fetches the rest from
the forge.

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

Review 01M44K67TVFXDNS7B3NBWNEKB8 — head 3f7917649d79a22a45854022bfcae70f9f6cf023

Review — j4k-oss/review-wrapper @ bed47dc989

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

medium — Outcome table restates the executable result and exit-code set

  • claim: 01M44NTXBKSY1RFQ9C2QVKM4TV
  • anchor: README.md (snippet)
| 1   | `fork-skip`           | Explicit "not reviewed" block; no `/v1` call precedes it.                                                                                                     | 0    |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The grounded README row is part of a reported table that copies all eleven OUTCOMES members and their OUTCOME_EXIT values from src/contract/types.ts. A separate superseded flag row does not undo the duplicated outcome set. The copies currently agree, so medium fits; point to the constants and retain only behavior they do not explain.
  • disposition: none

The outcomes table makes readers and maintainers rely on a second, manually maintained registry of the wrapper's results and exit codes. A new result or changed exit code can leave this reference stale while the job follows the executable constants.

src/contract/types.ts defines the source set in OUTCOMES and its exit codes in OUTCOME_EXIT: fork-skip, service-unreachable, auth-failed, forge-host-mismatch, ask-conflict, stuck, partial-coverage, grounding-pending, clean-zero-findings, clean-findings, and head-moved. The README table copies those same eleven members and adds a separate row for the superseded flag; no outcome member appears on only one side. The table also repeats the exit values and code comments. This is a restated set under the writing standard's One Idea, One Place and authoritative-lookup guidance, even while the copies agree.

Replace the inventory with a pointer to OUTCOMES and OUTCOME_EXIT in src/contract/types.ts. Keep behavior that the source does not explain, such as the timing of a head move, in the surrounding prose or beside its outcome definition. This preserves the operational guidance without a second list to update.

I read the full README, src/contract/types.ts, src/wrapper/classify-outcome.ts, and src/wrapper/orchestrator.ts; this is a static comparison, not a runtime test. The claim would be refuted if the README table were generated from those constants or read as an execution source; the tree shows a hand-written Markdown table and TypeScript constants used by the wrapper.

medium — Environment contract repeats the variables read by the wrapper

  • claim: 01M44NVTCNYZBJXSB7XHSA1MZ7
  • anchor: README.md (snippet)
The forge and runner variables — `FORGEJO_TOKEN`, `GITHUB_API_URL`, `GITHUB_EVENT_*`,
`GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, `GITHUB_REPOSITORY`, `GITHUB_SERVER_URL` — are
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The grounded README sentence and reported table repeat the environment names read in src/credentials.ts, src/wrapper/event-context.ts, and src/main.ts. Expanding GITHUB_EVENT_* yields the reported source members; the body initially says nine but explicitly counts ten, a harmless count slip in its explanation. Point to the readers while keeping the fork-gate and failure behavior. The earlier same-anchor verdict supplies no contrary evidence.
  • disposition: none

The environment contract names the complete required forge and runner variable set a second time after the table. A future change to an environment read can leave this hand-maintained inventory wrong even though the wrapper continues using the code-defined set.

The source reads in src/credentials.ts, src/wrapper/event-context.ts, and src/main.ts define nine variable names: REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, GITHUB_API_URL, GITHUB_EVENT_NAME, GITHUB_EVENT_PATH, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, GITHUB_REPOSITORY, and GITHUB_SERVER_URL. That is ten names, counting the two GITHUB_EVENT variables individually. The README's table and following required-on-every-path sentence cover the same ten: their GITHUB_EVENT_* shorthand covers NAME and PATH, and the table's ellipsis is made explicit in the following sentence. No member appears on only one side. The listing is a restated set under One Idea, One Place; the code's reads are the authoritative lookup.

Replace the complete inventory in the table and following sentence with pointers to those readers. Retain the distinct facts that service credentials are read only after the fork gate, that missing required values fail immediately, and that the run-attempt value controls re-triage. This preserves operational constraints without another list of names.

I read the README, the three named code files, and the input tests. This is a static comparison; the claim would be refuted by generation from the source or by the README being consumed as the environment schema. Neither is present in the tree.

medium — Action inputs table copies the manifest input

  • claim: 01M44NWD3WEGT6QN7XK59DECMZ
  • anchor: README.md (snippet)
| `expected-service-origin` | Origin `REVIEW_SERVICE_URL` must match exactly, e.g. `https://review.j4k.dev`. Left unset — or set to the empty string an undefined `${{ vars.X }}` expands to — the pin is off and only the `https:` scheme is enforced. |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The grounded README row lists expected-service-origin, and the reported comparison says action.yml defines that sole input and repeats its description. The manifest is the input source, so the README table is a second editable set. Point to action.yml. Earlier same-anchor allegations about URL validation concern a separate wording defect and do not refute this duplication.
  • disposition: none

The Action inputs table duplicates the action's input declaration and description. A workflow author who trusts this copy could miss a future change to the executable input contract.

action.yml defines the set of action inputs; its sole member is expected-service-origin. The README table covers the same sole member, with no member appearing on only one side, and repeats the unset/empty-string behavior already in the manifest description. This is a restated set under the writing standard's authoritative-lookup and One Idea, One Place guidance.

Replace the table with a pointer to action.yml, which exposes the name and help text to consumers. This preserves where to find the pin's behavior without maintaining a second input definition.

I compared the full README section with action.yml in the subject tree. This is a static comparison. Generation of the Markdown table from the manifest, or a runtime that reads the README as its input schema, would refute the duplication concern; neither is evident in the tree.

medium — Recovery table restates the failure remedy map

  • claim: 01M44NX49DRD0K5WWFPPH4FFEE
  • anchor: README.md (snippet)
| `provider session limit reached`              | Wait for the session limit to reset or ask the operator to lift it.                                                    |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The reported executionRemedies map in src/wrapper/remedies.ts defines four named failure remedies plus a fallback, while the grounded README table and reported comparison cover the displayed counterparts. That is a second remedy inventory. The external display-label mapping is unverified, but either a matching or mismatching label leaves this duplicated or stale table defective; follow the emitted remedy and point to formatExecutionRecovery. Medium fits the reported agreement.
  • disposition: none

The recovery table maintains a second cause-to-remedy mapping for operators. If the wrapper changes its recognized failure codes or remedy text, a reader following this table can choose stale recovery advice rather than the diagnostic the job actually emits.

executionRemedies in src/wrapper/remedies.ts is the executable source. Its named entries are provider-session-limit, provider-capacity, agent-execution-failed, and infrastructure-failure; formatExecutionRecovery also supplies a fallback for unrecognized codes. The README covers the corresponding four displayed causes plus the unrecognized-reason fallback, so no recognized source entry is visibly absent or extra. The exact display-label mapping comes from @j4k/review's formatExecutionFailure, outside this subject tree, and could not be compared here. This is a restated remedy set under One Idea, One Place even if the labels currently agree.

Replace the table with a pointer to formatExecutionRecovery and tell operators to follow the remedy printed with their failure. Preserve the retry distinction after that pointer: rerun failed triage, and use the printed dispatch re-ask for failed lens slots. The correction keeps the actionable instruction and removes a second map.

I read the README, src/wrapper/remedies.ts, src/wrapper/classify-outcome.ts, and the orchestrator tests for emitted diagnostics. This is static tracing, not a run against the service. A generated README table or a label mismatch in the external formatter would change the finding's exact comparison; the formatter source is the unresolved proof gap.

medium — Composition summary restates the reconciler set

  • claim: 01M44NXG523X5YEG67PQVVG1W1
  • anchor: README.md (snippet)
`GITHUB_REPOSITORY`, builds the forge client, derives the run's inputs from the event
payload, and hands the orchestrator its collaborators: the wait protocol, the clock, and
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The grounded composition excerpt and reported source comparison show README counts and names summary, inline, and dispositions as the complete reconciler set wired in src/main.ts and WrapperDeps. A code change can leave that count and list stale. Point to the wiring and retain the distinct orchestration and exit-code facts.
  • disposition: none

The composition summary hard-codes the number and names of the reconcilers beside the code that wires them. Adding or removing one in src/main.ts would leave this overview silently stale for a maintainer tracing the run.

The source set in src/main.ts has three reconciler fields passed to runWrapper: summary, inline, and dispositions. The README phrase 'the three reconcilers (summary, inline, dispositions)' covers those same three, with no member on only one side. The nearby WrapperDeps interface in src/contract/types.ts confirms those fields. The set count and members are a restatement under the writing standard's One Idea, One Place guidance.

Refer to the reconciler wiring in src/main.ts without counting or enumerating it. Keep the useful current fact that src/main.ts supplies the orchestrator and that its exit code becomes the process exit code. This gives readers the right entry point without a second list to maintain.

I read the complete composition paragraph, src/main.ts, and WrapperDeps. This is a static source comparison. The concern would be refuted if this README sentence were generated from the wiring or if it only gave nonexhaustive examples; its count and parenthetical present the complete set.

medium — Dispatch description omits the required PR-number input

  • claim: 01M44NYTXV5Y34CWS7D96BEY3H
  • anchor: README.md (snippet)
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. It identifies the pull request; a `workflow_dispatch` run fetches the PR's current state from the forge. `readWrapperInputs` in `src/wrapper/event-context.ts` derives the PR inputs. |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The grounded row says a workflow_dispatch run fetches the PR from the forge, while the reported readWrapperInputs trace first requires payload.inputs.pr_number and rejects its absence. A workflow author could omit that prerequisite and get a failed manual run. Add only that required input before the fetch description; the code pointer alone does not communicate it.
  • disposition: none

A workflow author can read 'runner event environment ... identifies the pull request' as meaning a manual dispatch needs no explicit PR-number input. A workflow_dispatch run without that input fails before it can fetch the PR, so the added sentence leaves out the prerequisite for the behavior it promises.

The changed row says the dispatch run fetches the PR's current state from the forge. In readWrapperInputs, the dispatch arm first reads payload.inputs?.pr_number, passes it to parsePrNumber, and only then calls forge.getPull(prNumber). The parser throws when the input is absent or is not a positive integer; src/wrapper/event-context.test.ts has the no-input case. The pointer to the reader is useful, but it does not state the caller's required input.

Add the smallest prerequisite to this row: 'A workflow_dispatch payload must provide inputs.pr_number; readWrapperInputs then fetches that PR's current state from the forge.' This preserves the current-state fact and prevents a failed manual run without copying the whole event schema.

I traced readWrapperInputs and its no-input test and read the README's rerun section, which only shows a specific re-ask command. I did not inspect a consuming workflow outside this tree. A caller contract that always supplies and documents pr_number would reduce the practical impact; the subject alone does not establish one.

medium — Empty origin pin still enforces more than the HTTPS scheme

  • claim: 01M44NZSG9QBYDHYT0A2YFHB4K
  • anchor: README.md (snippet)
| `expected-service-origin` | Origin `REVIEW_SERVICE_URL` must match exactly, e.g. `https://review.j4k.dev`. Left unset — or set to the empty string an undefined `${{ vars.X }}` expands to — the pin is off and only the `https:` scheme is enforced. |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44P1PDV8E80FJQNBT4DZ626 · valid: The grounded row says an empty origin pin leaves only HTTPS enforcement. The reported call trace says reviewCredentials always calls serviceOrigin first, and it rejects an HTTPS URL ending in /v1 even with no pin; the reported test covers that rejection. The row therefore overstates what disabling the pin allows. Say it skips only the origin match and keep URL validation at its source. Earlier same-anchor input-inventory claims describe a different defect.
  • disposition: none

A workflow author can leave the origin pin empty, supply an HTTPS service URL ending in /v1, and expect this input description to allow it. The run instead fails during credential validation before making a service request. The word 'only' therefore understates the remaining URL rules.

The README row says that with expected-service-origin unset or empty, 'only the https: scheme is enforced.' In src/credentials.ts, reviewCredentials calls serviceOrigin(serviceUrl) before checking the optional expected origin. That function rejects a pathname ending in /v1 even for an HTTPS URL; src/credentials.test.ts tests that rejection. The same inaccurate sentence is also the source description in action.yml.

Change the source description in action.yml to say that an empty input disables the origin pin while serviceOrigin still validates the URL, and have the README refer to that source. This preserves the useful pin-off behavior and avoids repeating the validator's rule set.

I read the input declaration, the full serviceOrigin and reviewCredentials call path, and the URL-validation tests. This is static tracing backed by tests in the tree; I did not run the action. A different deployed bundle could alter the live behavior, but the reviewed source and tests agree on this rejection.

Other claims

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

Coverage

Coverage pass: 01M44NHF06BM7C3G4SANAGC4BE
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 no-claims 1 no
<!-- review:summary --> **Review** `01M44K67TVFXDNS7B3NBWNEKB8` — head `3f7917649d79a22a45854022bfcae70f9f6cf023` # Review — j4k-oss/review-wrapper @ bed47dc98919 Scope: diff against base tree `e69943819640` 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 (7) ### medium — Outcome table restates the executable result and exit-code set - claim: `01M44NTXBKSY1RFQ9C2QVKM4TV` - anchor: `README.md` (snippet) ``` | 1 | `fork-skip` | Explicit "not reviewed" block; no `/v1` call precedes it. | 0 | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The grounded README row is part of a reported table that copies all eleven OUTCOMES members and their OUTCOME_EXIT values from src/contract/types.ts. A separate superseded flag row does not undo the duplicated outcome set. The copies currently agree, so medium fits; point to the constants and retain only behavior they do not explain. - disposition: none > The outcomes table makes readers and maintainers rely on a second, manually maintained registry of the wrapper's results and exit codes. A new result or changed exit code can leave this reference stale while the job follows the executable constants. > > `src/contract/types.ts` defines the source set in `OUTCOMES` and its exit codes in `OUTCOME_EXIT`: fork-skip, service-unreachable, auth-failed, forge-host-mismatch, ask-conflict, stuck, partial-coverage, grounding-pending, clean-zero-findings, clean-findings, and head-moved. The README table copies those same eleven members and adds a separate row for the `superseded` flag; no outcome member appears on only one side. The table also repeats the exit values and code comments. This is a restated set under the writing standard's One Idea, One Place and authoritative-lookup guidance, even while the copies agree. > > Replace the inventory with a pointer to `OUTCOMES` and `OUTCOME_EXIT` in `src/contract/types.ts`. Keep behavior that the source does not explain, such as the timing of a head move, in the surrounding prose or beside its outcome definition. This preserves the operational guidance without a second list to update. > > I read the full README, `src/contract/types.ts`, `src/wrapper/classify-outcome.ts`, and `src/wrapper/orchestrator.ts`; this is a static comparison, not a runtime test. The claim would be refuted if the README table were generated from those constants or read as an execution source; the tree shows a hand-written Markdown table and TypeScript constants used by the wrapper. ### medium — Environment contract repeats the variables read by the wrapper - claim: `01M44NVTCNYZBJXSB7XHSA1MZ7` - anchor: `README.md` (snippet) ``` The forge and runner variables — `FORGEJO_TOKEN`, `GITHUB_API_URL`, `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, `GITHUB_REPOSITORY`, `GITHUB_SERVER_URL` — are ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The grounded README sentence and reported table repeat the environment names read in src/credentials.ts, src/wrapper/event-context.ts, and src/main.ts. Expanding GITHUB_EVENT_* yields the reported source members; the body initially says nine but explicitly counts ten, a harmless count slip in its explanation. Point to the readers while keeping the fork-gate and failure behavior. The earlier same-anchor verdict supplies no contrary evidence. - disposition: none > The environment contract names the complete required forge and runner variable set a second time after the table. A future change to an environment read can leave this hand-maintained inventory wrong even though the wrapper continues using the code-defined set. > > The source reads in `src/credentials.ts`, `src/wrapper/event-context.ts`, and `src/main.ts` define nine variable names: REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, GITHUB_API_URL, GITHUB_EVENT_NAME, GITHUB_EVENT_PATH, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, GITHUB_REPOSITORY, and GITHUB_SERVER_URL. That is ten names, counting the two GITHUB_EVENT variables individually. The README's table and following required-on-every-path sentence cover the same ten: their `GITHUB_EVENT_*` shorthand covers NAME and PATH, and the table's ellipsis is made explicit in the following sentence. No member appears on only one side. The listing is a restated set under One Idea, One Place; the code's reads are the authoritative lookup. > > Replace the complete inventory in the table and following sentence with pointers to those readers. Retain the distinct facts that service credentials are read only after the fork gate, that missing required values fail immediately, and that the run-attempt value controls re-triage. This preserves operational constraints without another list of names. > > I read the README, the three named code files, and the input tests. This is a static comparison; the claim would be refuted by generation from the source or by the README being consumed as the environment schema. Neither is present in the tree. ### medium — Action inputs table copies the manifest input - claim: `01M44NWD3WEGT6QN7XK59DECMZ` - anchor: `README.md` (snippet) ``` | `expected-service-origin` | Origin `REVIEW_SERVICE_URL` must match exactly, e.g. `https://review.j4k.dev`. Left unset — or set to the empty string an undefined `${{ vars.X }}` expands to — the pin is off and only the `https:` scheme is enforced. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The grounded README row lists expected-service-origin, and the reported comparison says action.yml defines that sole input and repeats its description. The manifest is the input source, so the README table is a second editable set. Point to action.yml. Earlier same-anchor allegations about URL validation concern a separate wording defect and do not refute this duplication. - disposition: none > The Action inputs table duplicates the action's input declaration and description. A workflow author who trusts this copy could miss a future change to the executable input contract. > > `action.yml` defines the set of action inputs; its sole member is `expected-service-origin`. The README table covers the same sole member, with no member appearing on only one side, and repeats the unset/empty-string behavior already in the manifest description. This is a restated set under the writing standard's authoritative-lookup and One Idea, One Place guidance. > > Replace the table with a pointer to `action.yml`, which exposes the name and help text to consumers. This preserves where to find the pin's behavior without maintaining a second input definition. > > I compared the full README section with `action.yml` in the subject tree. This is a static comparison. Generation of the Markdown table from the manifest, or a runtime that reads the README as its input schema, would refute the duplication concern; neither is evident in the tree. ### medium — Recovery table restates the failure remedy map - claim: `01M44NX49DRD0K5WWFPPH4FFEE` - anchor: `README.md` (snippet) ``` | `provider session limit reached` | Wait for the session limit to reset or ask the operator to lift it. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The reported executionRemedies map in src/wrapper/remedies.ts defines four named failure remedies plus a fallback, while the grounded README table and reported comparison cover the displayed counterparts. That is a second remedy inventory. The external display-label mapping is unverified, but either a matching or mismatching label leaves this duplicated or stale table defective; follow the emitted remedy and point to formatExecutionRecovery. Medium fits the reported agreement. - disposition: none > The recovery table maintains a second cause-to-remedy mapping for operators. If the wrapper changes its recognized failure codes or remedy text, a reader following this table can choose stale recovery advice rather than the diagnostic the job actually emits. > > `executionRemedies` in `src/wrapper/remedies.ts` is the executable source. Its named entries are provider-session-limit, provider-capacity, agent-execution-failed, and infrastructure-failure; `formatExecutionRecovery` also supplies a fallback for unrecognized codes. The README covers the corresponding four displayed causes plus the unrecognized-reason fallback, so no recognized source entry is visibly absent or extra. The exact display-label mapping comes from `@j4k/review`'s `formatExecutionFailure`, outside this subject tree, and could not be compared here. This is a restated remedy set under One Idea, One Place even if the labels currently agree. > > Replace the table with a pointer to `formatExecutionRecovery` and tell operators to follow the remedy printed with their failure. Preserve the retry distinction after that pointer: rerun failed triage, and use the printed dispatch re-ask for failed lens slots. The correction keeps the actionable instruction and removes a second map. > > I read the README, `src/wrapper/remedies.ts`, `src/wrapper/classify-outcome.ts`, and the orchestrator tests for emitted diagnostics. This is static tracing, not a run against the service. A generated README table or a label mismatch in the external formatter would change the finding's exact comparison; the formatter source is the unresolved proof gap. ### medium — Composition summary restates the reconciler set - claim: `01M44NXG523X5YEG67PQVVG1W1` - anchor: `README.md` (snippet) ``` `GITHUB_REPOSITORY`, builds the forge client, derives the run's inputs from the event payload, and hands the orchestrator its collaborators: the wait protocol, the clock, and ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The grounded composition excerpt and reported source comparison show README counts and names summary, inline, and dispositions as the complete reconciler set wired in src/main.ts and WrapperDeps. A code change can leave that count and list stale. Point to the wiring and retain the distinct orchestration and exit-code facts. - disposition: none > The composition summary hard-codes the number and names of the reconcilers beside the code that wires them. Adding or removing one in `src/main.ts` would leave this overview silently stale for a maintainer tracing the run. > > The source set in `src/main.ts` has three reconciler fields passed to `runWrapper`: summary, inline, and dispositions. The README phrase 'the three reconcilers (summary, inline, dispositions)' covers those same three, with no member on only one side. The nearby `WrapperDeps` interface in `src/contract/types.ts` confirms those fields. The set count and members are a restatement under the writing standard's One Idea, One Place guidance. > > Refer to the reconciler wiring in `src/main.ts` without counting or enumerating it. Keep the useful current fact that `src/main.ts` supplies the orchestrator and that its exit code becomes the process exit code. This gives readers the right entry point without a second list to maintain. > > I read the complete composition paragraph, `src/main.ts`, and `WrapperDeps`. This is a static source comparison. The concern would be refuted if this README sentence were generated from the wiring or if it only gave nonexhaustive examples; its count and parenthetical present the complete set. ### medium — Dispatch description omits the required PR-number input - claim: `01M44NYTXV5Y34CWS7D96BEY3H` - anchor: `README.md` (snippet) ``` | `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. It identifies the pull request; a `workflow_dispatch` run fetches the PR's current state from the forge. `readWrapperInputs` in `src/wrapper/event-context.ts` derives the PR inputs. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The grounded row says a workflow_dispatch run fetches the PR from the forge, while the reported readWrapperInputs trace first requires payload.inputs.pr_number and rejects its absence. A workflow author could omit that prerequisite and get a failed manual run. Add only that required input before the fetch description; the code pointer alone does not communicate it. - disposition: none > A workflow author can read 'runner event environment ... identifies the pull request' as meaning a manual dispatch needs no explicit PR-number input. A `workflow_dispatch` run without that input fails before it can fetch the PR, so the added sentence leaves out the prerequisite for the behavior it promises. > > The changed row says the dispatch run fetches the PR's current state from the forge. In `readWrapperInputs`, the dispatch arm first reads `payload.inputs?.pr_number`, passes it to `parsePrNumber`, and only then calls `forge.getPull(prNumber)`. The parser throws when the input is absent or is not a positive integer; `src/wrapper/event-context.test.ts` has the no-input case. The pointer to the reader is useful, but it does not state the caller's required input. > > Add the smallest prerequisite to this row: 'A `workflow_dispatch` payload must provide `inputs.pr_number`; `readWrapperInputs` then fetches that PR's current state from the forge.' This preserves the current-state fact and prevents a failed manual run without copying the whole event schema. > > I traced `readWrapperInputs` and its no-input test and read the README's rerun section, which only shows a specific re-ask command. I did not inspect a consuming workflow outside this tree. A caller contract that always supplies and documents `pr_number` would reduce the practical impact; the subject alone does not establish one. ### medium — Empty origin pin still enforces more than the HTTPS scheme - claim: `01M44NZSG9QBYDHYT0A2YFHB4K` - anchor: `README.md` (snippet) ``` | `expected-service-origin` | Origin `REVIEW_SERVICE_URL` must match exactly, e.g. `https://review.j4k.dev`. Left unset — or set to the empty string an undefined `${{ vars.X }}` expands to — the pin is off and only the `https:` scheme is enforced. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44P1PDV8E80FJQNBT4DZ626` · valid: The grounded row says an empty origin pin leaves only HTTPS enforcement. The reported call trace says reviewCredentials always calls serviceOrigin first, and it rejects an HTTPS URL ending in /v1 even with no pin; the reported test covers that rejection. The row therefore overstates what disabling the pin allows. Say it skips only the origin match and keep URL validation at its source. Earlier same-anchor input-inventory claims describe a different defect. - disposition: none > A workflow author can leave the origin pin empty, supply an HTTPS service URL ending in `/v1`, and expect this input description to allow it. The run instead fails during credential validation before making a service request. The word 'only' therefore understates the remaining URL rules. > > The README row says that with `expected-service-origin` unset or empty, 'only the `https:` scheme is enforced.' In `src/credentials.ts`, `reviewCredentials` calls `serviceOrigin(serviceUrl)` before checking the optional expected origin. That function rejects a pathname ending in `/v1` even for an HTTPS URL; `src/credentials.test.ts` tests that rejection. The same inaccurate sentence is also the source description in `action.yml`. > > Change the source description in `action.yml` to say that an empty input disables the origin pin while `serviceOrigin` still validates the URL, and have the README refer to that source. This preserves the useful pin-off behavior and avoids repeating the validator's rule set. > > I read the input declaration, the full `serviceOrigin` and `reviewCredentials` call path, and the URL-validation tests. This is static tracing backed by tests in the tree; I did not run the action. A different deployed bundle could alter the live behavior, but the reviewed source and tests agree on this rejection. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M44NHF06BM7C3G4SANAGC4BE 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 | no-claims | 1 | no |
README.md Outdated
@ -22,1 +15,3 @@
| `GITHUB_RUN_ATTEMPT` | Forgejo run attempt (1-based). Required on every path. The wrapper re-triages stalled or failed triage only when this is ≥ 2. |
| Variable | Meaning |
| ------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `REVIEW_SERVICE_URL` | Base URL of the review service (an org-managed variable). Must parse as an `https:` URL whose path does not end in `/v1` or `/v1/`, because the client appends `/v1`. |

medium — The service URL guidance copies the forbidden path suffixes

The environment table makes readers rely on a second copy of the service URL validation rule. If the accepted or rejected suffixes change in code, this configuration guidance can silently become wrong.

API_PREFIX_SUFFIX in src/credentials.ts defines the rejected suffixes through /\/v1\/?$/u: /v1 and /v1/. The README copy covers /v1 and /v1/; no member appears in only one list, so it still agrees. This is a repository-defined validation set, not a fixed standard, an example, a generated source, or a dated record; readers can open the implementation.

Point the table to serviceOrigin in src/credentials.ts for the URL validation rule, and keep the reason for the API-prefix guard beside its regex.

I read the full README, src/credentials.ts, src/credentials.test.ts, and the src/main.ts call path. The tests exercise both listed suffix forms. This is a static source comparison, not a runtime reproduction. A demonstrated generation path that derives this README cell from API_PREFIX_SUFFIX would refute the independent-copy risk; the package scripts and repository scripts I checked show no such path.

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

<!-- review:claim:01M44FQEVVASXA9MEHF4ZEM43V --> **medium** — The service URL guidance copies the forbidden path suffixes > The environment table makes readers rely on a second copy of the service URL validation rule. If the accepted or rejected suffixes change in code, this configuration guidance can silently become wrong. > > `API_PREFIX_SUFFIX` in `src/credentials.ts` defines the rejected suffixes through `/\/v1\/?$/u`: `/v1` and `/v1/`. The README copy covers `/v1` and `/v1/`; no member appears in only one list, so it still agrees. This is a repository-defined validation set, not a fixed standard, an example, a generated source, or a dated record; readers can open the implementation. > > Point the table to `serviceOrigin` in `src/credentials.ts` for the URL validation rule, and keep the reason for the API-prefix guard beside its regex. > > I read the full README, `src/credentials.ts`, `src/credentials.test.ts`, and the `src/main.ts` call path. The tests exercise both listed suffix forms. This is a static source comparison, not a runtime reproduction. A demonstrated generation path that derives this README cell from `API_PREFIX_SUFFIX` would refute the independent-copy risk; the package scripts and repository scripts I checked show no such path. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44FQEVVASXA9MEHF4ZEM43V` of review `01M44FFYXGJXA6XHKADPDH94A0`
Author
Owner

Fixed in c02a237

<!-- gh-feedback:reply-to:123440 --> Fixed in c02a237
jercik marked this conversation as resolved
README.md Outdated
@ -23,0 +18,4 @@
| `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. |
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. `pull_request_target` reads PR details from the payload; `workflow_dispatch` reads only `pr_number` and fetches the rest from the forge. |

medium — The event environment guidance copies both supported event modes

The environment table gives a second, manually maintained account of the wrapper's event modes and their payload handling. If readWrapperInputs adds or changes a mode, the table can misstate which payload is read and where PR details come from.

src/contract/types.ts defines WrapperInputs.eventName as pull_request_target | workflow_dispatch, and readWrapperInputs in src/wrapper/event-context.ts branches on those same two modes and rejects others. The README copy names pull_request_target and workflow_dispatch; neither list has an extra or missing member, and the described payload behavior currently agrees. This is the wrapper's chosen subset of runner events, not the third party's full event set, and the README passage is neither a source, a dated record, nor a contents table.

Point the table to readWrapperInputs in src/wrapper/event-context.ts for event-specific input derivation, retaining behavior details beside the corresponding branches where the implementation can carry them.

I read the full README, the event-name union, readWrapperInputs, its tests, and the src/main.ts caller. This is a static comparison; I did not execute a runner event. A source-generated README cell would remove the independent-copy risk, but no such generation path appears in the package or scripts I checked.

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

<!-- review:claim:01M44FQXBGDK887K5NNM7ZB7SP --> **medium** — The event environment guidance copies both supported event modes > The environment table gives a second, manually maintained account of the wrapper's event modes and their payload handling. If `readWrapperInputs` adds or changes a mode, the table can misstate which payload is read and where PR details come from. > > `src/contract/types.ts` defines `WrapperInputs.eventName` as `pull_request_target | workflow_dispatch`, and `readWrapperInputs` in `src/wrapper/event-context.ts` branches on those same two modes and rejects others. The README copy names `pull_request_target` and `workflow_dispatch`; neither list has an extra or missing member, and the described payload behavior currently agrees. This is the wrapper's chosen subset of runner events, not the third party's full event set, and the README passage is neither a source, a dated record, nor a contents table. > > Point the table to `readWrapperInputs` in `src/wrapper/event-context.ts` for event-specific input derivation, retaining behavior details beside the corresponding branches where the implementation can carry them. > > I read the full README, the event-name union, `readWrapperInputs`, its tests, and the `src/main.ts` caller. This is a static comparison; I did not execute a runner event. A source-generated README cell would remove the independent-copy risk, but no such generation path appears in the package or scripts I checked. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44FQXBGDK887K5NNM7ZB7SP` of review `01M44FFYXGJXA6XHKADPDH94A0`

medium — The dispatch guidance copies the sole payload input

The word only turns this README sentence into a complete, separately maintained list of fields the dispatch path reads from the event payload. Adding another payload input in code would leave this guidance wrong until someone edits it too.

WorkflowDispatchEvent in src/wrapper/event-context.ts defines the payload input set as pr_number, and the dispatch branch of readWrapperInputs reads payload.inputs?.pr_number before fetching the pull from the forge. The README copy covers pr_number; the source set and copy each have that one member and no difference. This is the wrapper's selected payload field, not the complete third-party event schema, and the prose is not a generated source, dated record, or an example.

Point readers to the workflow_dispatch branch of readWrapperInputs in src/wrapper/event-context.ts for the fields it takes from the event payload. The branch already carries the behavior detail.

I read the full README, src/wrapper/event-context.ts, its dispatch tests, and the src/main.ts caller. The code trace establishes current agreement; I did not run a dispatch event. A demonstrated generator tying this README sentence to the input type and branch would refute the independent-copy concern, and the package scripts I checked do not provide one.

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

<!-- review:claim:01M44FS7MGTRS78178F7M3XCSR --> **medium** — The dispatch guidance copies the sole payload input > The word `only` turns this README sentence into a complete, separately maintained list of fields the dispatch path reads from the event payload. Adding another payload input in code would leave this guidance wrong until someone edits it too. > > `WorkflowDispatchEvent` in `src/wrapper/event-context.ts` defines the payload input set as `pr_number`, and the dispatch branch of `readWrapperInputs` reads `payload.inputs?.pr_number` before fetching the pull from the forge. The README copy covers `pr_number`; the source set and copy each have that one member and no difference. This is the wrapper's selected payload field, not the complete third-party event schema, and the prose is not a generated source, dated record, or an example. > > Point readers to the `workflow_dispatch` branch of `readWrapperInputs` in `src/wrapper/event-context.ts` for the fields it takes from the event payload. The branch already carries the behavior detail. > > I read the full README, `src/wrapper/event-context.ts`, its dispatch tests, and the `src/main.ts` caller. The code trace establishes current agreement; I did not run a dispatch event. A demonstrated generator tying this README sentence to the input type and branch would refute the independent-copy concern, and the package scripts I checked do not provide one. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44FS7MGTRS78178F7M3XCSR` of review `01M44FFYXGJXA6XHKADPDH94A0`
Author
Owner

Fixed in c02a237

<!-- gh-feedback:reply-to:123441 --> Fixed in c02a237
Author
Owner

Fixed in c02a237

<!-- gh-feedback:reply-to:123442 --> Fixed in c02a237
jercik marked this conversation as resolved
Lines 23-24
@ -23,3 +21,5 @@
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. `pull_request_target` reads PR details from the payload; `workflow_dispatch` reads only `pr_number` and fetches the rest from the forge. |
| `GITHUB_RUN_ATTEMPT` | Forgejo run attempt (1-based). Required on every path. The wrapper re-triages stalled or failed triage only when this is ≥ 2. |
The forge and runner variables — `FORGEJO_TOKEN`, `GITHUB_API_URL`, `GITHUB_EVENT_*`,
`GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, `GITHUB_REPOSITORY`, `GITHUB_SERVER_URL` — are

medium — The required environment-variable list duplicates the input checks

The paragraph restates the complete set of variables required on every path. When a new required runner variable is added to the input parser, this list can remain unchanged and send workflow maintainers to an incomplete setup checklist.

The actual required names are FORGEJO_TOKEN and GITHUB_API_URL in forgeCredentials, GITHUB_REPOSITORY in main.ts, and GITHUB_EVENT_NAME, GITHUB_EVENT_PATH, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, and GITHUB_SERVER_URL in readWrapperInputs. The paragraph covers those eight through six explicit names plus GITHUB_EVENT_* as shorthand for the two event variables; no required source member is absent from that reading of the copy. The wildcard itself is less precise than the source.

Replace the enumeration with a pointer to forgeCredentials, main.ts, and readWrapperInputs as the required-variable checks, while retaining the separate fork-path exception for service credentials. This preserves the consequential requirement and exception without a second member list.

I traced main.ts, forgeCredentials, and readWrapperInputs and read the surrounding environment contract. This is static reasoning; I did not run a workflow. Those calls to requireEnv and the initial repository check are the decisive source for the set.

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

<!-- review:claim:01M44GZQPJCBT3V27HCJ8KY3GT --> **medium** — The required environment-variable list duplicates the input checks > The paragraph restates the complete set of variables required on every path. When a new required runner variable is added to the input parser, this list can remain unchanged and send workflow maintainers to an incomplete setup checklist. > > The actual required names are `FORGEJO_TOKEN` and `GITHUB_API_URL` in `forgeCredentials`, `GITHUB_REPOSITORY` in `main.ts`, and `GITHUB_EVENT_NAME`, `GITHUB_EVENT_PATH`, `GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, and `GITHUB_SERVER_URL` in `readWrapperInputs`. The paragraph covers those eight through six explicit names plus `GITHUB_EVENT_*` as shorthand for the two event variables; no required source member is absent from that reading of the copy. The wildcard itself is less precise than the source. > > Replace the enumeration with a pointer to `forgeCredentials`, `main.ts`, and `readWrapperInputs` as the required-variable checks, while retaining the separate fork-path exception for service credentials. This preserves the consequential requirement and exception without a second member list. > > I traced `main.ts`, `forgeCredentials`, and `readWrapperInputs` and read the surrounding environment contract. This is static reasoning; I did not run a workflow. Those calls to `requireEnv` and the initial repository check are the decisive source for the set. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44GZQPJCBT3V27HCJ8KY3GT` of review `01M44FFYXGJXA6XHKADPDH94A0`
jercik marked this conversation as resolved
Author
Owner

Replying to review comment #123845

Valid. The list of required variables predates this PR, which changes only two rows of the table above it. #32 replaces the list with pointers to forgeCredentials, readWrapperInputs and src/main.ts.

> Replying to review comment #123845 Valid. The list of required variables predates this PR, which changes only two rows of the table above it. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 replaces the list with pointers to `forgeCredentials`, `readWrapperInputs` and `src/main.ts`.
docs: point the environment table at the URL and event-input code
All checks were successful
commit-msg / commitlint (pull_request) Successful in 23s
Checks / quality-checks (pull_request) Successful in 51s
Review / Review (pull_request_target) Successful in 11m40s
c02a237c2f
Two cells restated sets the code defines. They now name serviceOrigin and readWrapperInputs,
so the README cannot drift from the checks and the supported events.

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

Replying to comment #123439

This is review 01M44FFYXGJXA6XHKADPDH94A0 of 6c997aa. Dispatch run 64023 completed it after run 63863 lost its writing-quality slot to model capacity. The three findings on this PR's two table rows are fixed in c02a237 and answered in their threads, and #123845 is answered in its thread. The four summary-only findings are about README text this PR doesn't change, and the PR that owns each text already handles it:

  • 01M44GYJ3E9AJ5D1B7BM6RVJTJ (outcome table) is valid. #30 replaces the table with a pointer to OUTCOMES and OUTCOME_EXIT.
  • 01M44GZA3SC497K2CQ6KTZ55KP (action-input table) is valid. #34 points the README at action.yml.
  • 01M44H03Q3R3CW3Q54S7WVKBSR (composition paragraph) is valid. #32 points at WrapperDeps.
  • 01M44H13H1D9ZVDJVWZQV5TJS4 (recovery table) is valid. #34 replaces the table with the remedy printed on the failure log line.

The four duplicate claims, 01M44GVCF6NTE12F7BW1NWSAYZ, 01M44GX697RYJDZ04F855R941Q, 01M44GVN20E2HERV6AXANA7KH6 and 01M44GWSQB5Y1SSKK2119KW503, are covered by the threads of the findings they duplicate.

> Replying to comment #123439 This is review `01M44FFYXGJXA6XHKADPDH94A0` of 6c997aa. Dispatch run 64023 completed it after run 63863 lost its writing-quality slot to model capacity. The three findings on this PR's two table rows are fixed in c02a237 and answered in their threads, and #123845 is answered in its thread. The four summary-only findings are about README text this PR doesn't change, and the PR that owns each text already handles it: - `01M44GYJ3E9AJ5D1B7BM6RVJTJ` (outcome table) is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30 replaces the table with a pointer to `OUTCOMES` and `OUTCOME_EXIT`. - `01M44GZA3SC497K2CQ6KTZ55KP` (action-input table) is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34 points the README at `action.yml`. - `01M44H03Q3R3CW3Q54S7WVKBSR` (composition paragraph) is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 points at `WrapperDeps`. - `01M44H13H1D9ZVDJVWZQV5TJS4` (recovery table) is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34 replaces the table with the remedy printed on the failure log line. The four duplicate claims, `01M44GVCF6NTE12F7BW1NWSAYZ`, `01M44GX697RYJDZ04F855R941Q`, `01M44GVN20E2HERV6AXANA7KH6` and `01M44GWSQB5Y1SSKK2119KW503`, are covered by the threads of the findings they duplicate.
README.md Outdated
@ -23,0 +18,4 @@
| `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. |
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. `readWrapperInputs` in `src/wrapper/event-context.ts` derives the PR inputs from it for each event the wrapper supports. |

low — Environment table misstates where workflow dispatch gets PR state

For a workflow_dispatch run, this sentence tells readers that the runner event environment supplies the PR inputs, including the head and fork state. The event payload supplies only pr_number; readWrapperInputs calls forge.getPull(prNumber) and takes headSha, baseBranch, headBranch, and isFork from the returned pull summary. A reader using this table to diagnose a dispatch run would look for head data in the event payload instead of the forge response. State that the event identifies the PR and that dispatch fetches its current state from Forgejo. I traced readWrapperInputs in src/wrapper/event-context.ts, its workflow dispatch test in src/wrapper/event-context.test.ts, and the pr_number input in .forgejo/workflows/review.yml; this is a static source trace. A dispatch event payload containing these head fields, or a code path using them instead of getPull, would refute the claim.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44HW31SG0SQ794AF4N3BKQP of review 01M44HQNME6B9VAE2MGYCZKPXJ

<!-- review:claim:01M44HW31SG0SQ794AF4N3BKQP --> **low** — Environment table misstates where workflow dispatch gets PR state > For a `workflow_dispatch` run, this sentence tells readers that the runner event environment supplies the PR inputs, including the head and fork state. The event payload supplies only `pr_number`; `readWrapperInputs` calls `forge.getPull(prNumber)` and takes `headSha`, `baseBranch`, `headBranch`, and `isFork` from the returned pull summary. A reader using this table to diagnose a dispatch run would look for head data in the event payload instead of the forge response. State that the event identifies the PR and that dispatch fetches its current state from Forgejo. I traced `readWrapperInputs` in `src/wrapper/event-context.ts`, its workflow dispatch test in `src/wrapper/event-context.test.ts`, and the `pr_number` input in `.forgejo/workflows/review.yml`; this is a static source trace. A dispatch event payload containing these head fields, or a code path using them instead of `getPull`, would refute the claim. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44HW31SG0SQ794AF4N3BKQP` of review `01M44HQNME6B9VAE2MGYCZKPXJ`
Author
Owner

Fixed in 3f79176. The row now says the event environment identifies the pull request, and that a workflow_dispatch run fetches the PR's current state from the forge. It points at readWrapperInputs for how the inputs are derived.

<!-- gh-feedback:reply-to:124196 --> Fixed in 3f79176. The row now says the event environment identifies the pull request, and that a `workflow_dispatch` run fetches the PR's current state from the forge. It points at `readWrapperInputs` for how the inputs are derived.
jercik marked this conversation as resolved
Lines 23-24
@ -23,3 +21,5 @@
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. `readWrapperInputs` in `src/wrapper/event-context.ts` derives the PR inputs from it for each event the wrapper supports. |
| `GITHUB_RUN_ATTEMPT` | Forgejo run attempt (1-based). Required on every path. The wrapper re-triages stalled or failed triage only when this is ≥ 2. |
The forge and runner variables — `FORGEJO_TOKEN`, `GITHUB_API_URL`, `GITHUB_EVENT_*`,
`GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, `GITHUB_REPOSITORY`, `GITHUB_SERVER_URL` — are

medium — Required environment variables are copied into README prose

A maintainer can add a required runner variable in the wrapper while this separate “required on every path” list stays unchanged, leaving operators with an incomplete setup contract. The paragraph presents the complete required forge and runner environment set rather than pointing to the reads that enforce it.

The source reads in forgeCredentials (src/credentials.ts), readWrapperInputs (src/wrapper/event-context.ts), and src/main.ts require FORGEJO_TOKEN, GITHUB_API_URL, GITHUB_EVENT_NAME, GITHUB_EVENT_PATH, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, GITHUB_REPOSITORY, and GITHUB_SERVER_URL. Expanding the README’s GITHUB_EVENT_* to the two variables currently read yields the same members; neither side has an unmatched member.

Replace the enumeration with a pointer to the functions that require the variables. Keep the separate, useful fact that reviewCredentials() runs only after the fork gate, so service credentials are required only on the same-repository path. This follows the writing standard’s instruction to use the environment and source code as the authority instead of copying a set.

I compared the README section with those three source files and traced the fork gate in runWrapper; this is a static comparison, not a run. A generated or otherwise authoritative environment contract that drives these reads would refute the duplicate-source concern; none appears in the reviewed tree.

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

<!-- review:claim:01M44HZ794Q1MQD16Q0FH8VFAX --> **medium** — Required environment variables are copied into README prose > A maintainer can add a required runner variable in the wrapper while this separate “required on every path” list stays unchanged, leaving operators with an incomplete setup contract. The paragraph presents the complete required forge and runner environment set rather than pointing to the reads that enforce it. > > The source reads in `forgeCredentials` (`src/credentials.ts`), `readWrapperInputs` (`src/wrapper/event-context.ts`), and `src/main.ts` require `FORGEJO_TOKEN`, `GITHUB_API_URL`, `GITHUB_EVENT_NAME`, `GITHUB_EVENT_PATH`, `GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, `GITHUB_REPOSITORY`, and `GITHUB_SERVER_URL`. Expanding the README’s `GITHUB_EVENT_*` to the two variables currently read yields the same members; neither side has an unmatched member. > > Replace the enumeration with a pointer to the functions that require the variables. Keep the separate, useful fact that `reviewCredentials()` runs only after the fork gate, so service credentials are required only on the same-repository path. This follows the writing standard’s instruction to use the environment and source code as the authority instead of copying a set. > > I compared the README section with those three source files and traced the fork gate in `runWrapper`; this is a static comparison, not a run. A generated or otherwise authoritative environment contract that drives these reads would refute the duplicate-source concern; none appears in the reviewed tree. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44HZ794Q1MQD16Q0FH8VFAX` of review `01M44HQNME6B9VAE2MGYCZKPXJ`
jercik marked this conversation as resolved
docs: say a dispatch run fetches the PR's state from the forge
Some checks failed
commit-msg / commitlint (pull_request) Successful in 17s
Checks / quality-checks (pull_request) Successful in 48s
Review / Review (pull_request_target) Failing after 5m10s
3f7917649d
The cell said the PR inputs come from the runner event environment for
every event, but a workflow_dispatch payload carries only the PR number.
The head SHA, branches and fork status come from forge.getPull.

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

Replying to review comment #124195

Valid. The sentence predates this PR, which changes only two cells of the table above it. #32 already replaces the variable list with pointers to forgeCredentials, readWrapperInputs and src/main.ts.

> Replying to review comment #124195 Valid. The sentence predates this PR, which changes only two cells of the table above it. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 already replaces the variable list with pointers to `forgeCredentials`, `readWrapperInputs` and `src/main.ts`.
Author
Owner

Replying to comment #123439

This is review 01M44HQNME6B9VAE2MGYCZKPXJ of c02a237. The two findings with threads are answered there. The event row's dispatch error is fixed in 3f79176, and the duplicate claim 01M44J5WZVJ4SF3J565Y5C1WKE is covered by that thread. The other six are summary-only, about README text this PR doesn't change:

  • 01M44HZS0KRSTSY2G3CBCT79VW (action-input table) and 01M44J1TARJ7VX7DT4DJNJHGRB (recovery table) are valid. #34 points the README at inputs in action.yml and at the remedy printed on the failure log line.
  • 01M44J0F1W6H9S1VC9JC0AHSJN (outcome table) is valid. #30 replaces the table with a pointer to OUTCOMES and OUTCOME_EXIT.
  • 01M44J18M9V43V0RPG7MKTG48R (composition paragraph) is valid. #32 points at WrapperDeps.
  • 01M44J46M5G05CVCBP3JFRC5R5 (the fork-gate bullet says no credentials module runs before the gate) is valid: forgeCredentials() reads the forge token first. #33 corrects the bullet to name reviewCredentials().
  • 01M44J54BBK4B8V4BDJEWYP7XH (an unset origin pin still validates the URL) is valid: reviewCredentials() always calls serviceOrigin(), which rejects a /v1 suffix with or without the pin. #34 removes the README's copy of that sentence, and #41 corrects the original in action.yml. It targets main, because the sentence is older than this PR.
> Replying to comment #123439 This is review `01M44HQNME6B9VAE2MGYCZKPXJ` of `c02a237`. The two findings with threads are answered there. The event row's dispatch error is fixed in 3f79176, and the duplicate claim `01M44J5WZVJ4SF3J565Y5C1WKE` is covered by that thread. The other six are summary-only, about README text this PR doesn't change: - `01M44HZS0KRSTSY2G3CBCT79VW` (action-input table) and `01M44J1TARJ7VX7DT4DJNJHGRB` (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. - `01M44J0F1W6H9S1VC9JC0AHSJN` (outcome table) is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30 replaces the table with a pointer to `OUTCOMES` and `OUTCOME_EXIT`. - `01M44J18M9V43V0RPG7MKTG48R` (composition paragraph) is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 points at `WrapperDeps`. - `01M44J46M5G05CVCBP3JFRC5R5` (the fork-gate bullet says no credentials module runs before the gate) is valid: `forgeCredentials()` reads the forge token first. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/33 corrects the bullet to name `reviewCredentials()`. - `01M44J54BBK4B8V4BDJEWYP7XH` (an unset origin pin still validates the URL) is valid: `reviewCredentials()` always calls `serviceOrigin()`, which rejects a `/v1` suffix with or without the pin. #34 removes the README's copy of that sentence, and https://code.j4k.dev/j4k-oss/review-wrapper/pulls/41 corrects the original in `action.yml`. It targets `main`, because the sentence is older than this PR.
@ -23,0 +18,4 @@
| `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. |
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. It identifies the pull request; a `workflow_dispatch` run fetches the PR's current state from the forge. `readWrapperInputs` in `src/wrapper/event-context.ts` derives the PR inputs. |

medium — Dispatch description omits the required PR-number input

A workflow author can read 'runner event environment ... identifies the pull request' as meaning a manual dispatch needs no explicit PR-number input. A workflow_dispatch run without that input fails before it can fetch the PR, so the added sentence leaves out the prerequisite for the behavior it promises.

The changed row says the dispatch run fetches the PR's current state from the forge. In readWrapperInputs, the dispatch arm first reads payload.inputs?.pr_number, passes it to parsePrNumber, and only then calls forge.getPull(prNumber). The parser throws when the input is absent or is not a positive integer; src/wrapper/event-context.test.ts has the no-input case. The pointer to the reader is useful, but it does not state the caller's required input.

Add the smallest prerequisite to this row: 'A workflow_dispatch payload must provide inputs.pr_number; readWrapperInputs then fetches that PR's current state from the forge.' This preserves the current-state fact and prevents a failed manual run without copying the whole event schema.

I traced readWrapperInputs and its no-input test and read the README's rerun section, which only shows a specific re-ask command. I did not inspect a consuming workflow outside this tree. A caller contract that always supplies and documents pr_number would reduce the practical impact; the subject alone does not establish one.

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

<!-- review:claim:01M44NYTXV5Y34CWS7D96BEY3H --> **medium** — Dispatch description omits the required PR-number input > A workflow author can read 'runner event environment ... identifies the pull request' as meaning a manual dispatch needs no explicit PR-number input. A `workflow_dispatch` run without that input fails before it can fetch the PR, so the added sentence leaves out the prerequisite for the behavior it promises. > > The changed row says the dispatch run fetches the PR's current state from the forge. In `readWrapperInputs`, the dispatch arm first reads `payload.inputs?.pr_number`, passes it to `parsePrNumber`, and only then calls `forge.getPull(prNumber)`. The parser throws when the input is absent or is not a positive integer; `src/wrapper/event-context.test.ts` has the no-input case. The pointer to the reader is useful, but it does not state the caller's required input. > > Add the smallest prerequisite to this row: 'A `workflow_dispatch` payload must provide `inputs.pr_number`; `readWrapperInputs` then fetches that PR's current state from the forge.' This preserves the current-state fact and prevents a failed manual run without copying the whole event schema. > > I traced `readWrapperInputs` and its no-input test and read the README's rerun section, which only shows a specific re-ask command. I did not inspect a consuming workflow outside this tree. A caller contract that always supplies and documents `pr_number` would reduce the practical impact; the subject alone does not establish one. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44NYTXV5Y34CWS7D96BEY3H` of review `01M44K67TVFXDNS7B3NBWNEKB8`
jercik marked this conversation as resolved
Lines 23-24
@ -23,3 +21,5 @@
| `GITHUB_EVENT_*`, `GITHUB_RUN_ID`, … | Runner event environment. It identifies the pull request; a `workflow_dispatch` run fetches the PR's current state from the forge. `readWrapperInputs` in `src/wrapper/event-context.ts` derives the PR inputs. |
| `GITHUB_RUN_ATTEMPT` | Forgejo run attempt (1-based). Required on every path. The wrapper re-triages stalled or failed triage only when this is ≥ 2. |
The forge and runner variables — `FORGEJO_TOKEN`, `GITHUB_API_URL`, `GITHUB_EVENT_*`,
`GITHUB_RUN_ID`, `GITHUB_RUN_ATTEMPT`, `GITHUB_REPOSITORY`, `GITHUB_SERVER_URL` — are

medium — Environment contract repeats the variables read by the wrapper

The environment contract names the complete required forge and runner variable set a second time after the table. A future change to an environment read can leave this hand-maintained inventory wrong even though the wrapper continues using the code-defined set.

The source reads in src/credentials.ts, src/wrapper/event-context.ts, and src/main.ts define nine variable names: REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, GITHUB_API_URL, GITHUB_EVENT_NAME, GITHUB_EVENT_PATH, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, GITHUB_REPOSITORY, and GITHUB_SERVER_URL. That is ten names, counting the two GITHUB_EVENT variables individually. The README's table and following required-on-every-path sentence cover the same ten: their GITHUB_EVENT_* shorthand covers NAME and PATH, and the table's ellipsis is made explicit in the following sentence. No member appears on only one side. The listing is a restated set under One Idea, One Place; the code's reads are the authoritative lookup.

Replace the complete inventory in the table and following sentence with pointers to those readers. Retain the distinct facts that service credentials are read only after the fork gate, that missing required values fail immediately, and that the run-attempt value controls re-triage. This preserves operational constraints without another list of names.

I read the README, the three named code files, and the input tests. This is a static comparison; the claim would be refuted by generation from the source or by the README being consumed as the environment schema. Neither is present in the tree.

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

<!-- review:claim:01M44NVTCNYZBJXSB7XHSA1MZ7 --> **medium** — Environment contract repeats the variables read by the wrapper > The environment contract names the complete required forge and runner variable set a second time after the table. A future change to an environment read can leave this hand-maintained inventory wrong even though the wrapper continues using the code-defined set. > > The source reads in `src/credentials.ts`, `src/wrapper/event-context.ts`, and `src/main.ts` define nine variable names: REVIEW_SERVICE_URL, REVIEW_CAPABILITY_TOKEN, FORGEJO_TOKEN, GITHUB_API_URL, GITHUB_EVENT_NAME, GITHUB_EVENT_PATH, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT, GITHUB_REPOSITORY, and GITHUB_SERVER_URL. That is ten names, counting the two GITHUB_EVENT variables individually. The README's table and following required-on-every-path sentence cover the same ten: their `GITHUB_EVENT_*` shorthand covers NAME and PATH, and the table's ellipsis is made explicit in the following sentence. No member appears on only one side. The listing is a restated set under One Idea, One Place; the code's reads are the authoritative lookup. > > Replace the complete inventory in the table and following sentence with pointers to those readers. Retain the distinct facts that service credentials are read only after the fork gate, that missing required values fail immediately, and that the run-attempt value controls re-triage. This preserves operational constraints without another list of names. > > I read the README, the three named code files, and the input tests. This is a static comparison; the claim would be refuted by generation from the source or by the README being consumed as the environment schema. Neither is present in the tree. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44NVTCNYZBJXSB7XHSA1MZ7` of review `01M44K67TVFXDNS7B3NBWNEKB8`
jercik marked this conversation as resolved
Author
Owner

Replying to review comment #125247

Valid for the sentence after the table, which predates this PR. #32 already replaces its variable list with pointers to forgeCredentials, readWrapperInputs and src/main.ts.

Replacing the table itself is held, not applied, as 01M44DH5HKQFA3T9JJSGAEFP8M was on #30: the table is the only place that tells a calling workflow which variables to set and what each must hold.

> Replying to review comment #125247 Valid for the sentence after the table, which predates this PR. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 already replaces its variable list with pointers to `forgeCredentials`, `readWrapperInputs` and `src/main.ts`. Replacing the table itself is held, not applied, as `01M44DH5HKQFA3T9JJSGAEFP8M` was on #30: the table is the only place that tells a calling workflow which variables to set and what each must hold.
Author
Owner

Replying to review comment #125248

Valid: readWrapperInputs reads inputs.pr_number on a workflow_dispatch event and parsePrNumber throws when it is absent, and the README never names the input. #43 adds the requirement to the Notes paragraph that already says what a dispatch run needs and refuses. It targets main, so it doesn't wait for this PR.

> Replying to review comment #125248 Valid: `readWrapperInputs` reads `inputs.pr_number` on a `workflow_dispatch` event and `parsePrNumber` throws when it is absent, and the README never names the input. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/43 adds the requirement to the Notes paragraph that already says what a dispatch run needs and refuses. It targets `main`, so it doesn't wait for this PR.
Author
Owner

Replying to comment #123439

This is review 01M44K67TVFXDNS7B3NBWNEKB8 of 3f79176. Dispatch run 64696 completed it after an event run and two dispatch runs lost slots to model capacity and sandbox failures. The two findings with threads are answered there: the dispatch row's missing pr_number prerequisite goes to #43, and the variable list after the table to #32. The other five are summary-only. Each repeats a finding of the previous review about README text this PR doesn't change:

  • 01M44NTXBKSY1RFQ9C2QVKM4TV (outcome table): #30 replaces the table with a pointer to OUTCOMES and OUTCOME_EXIT.
  • 01M44NWD3WEGT6QN7XK59DECMZ (action-input table) and 01M44NX49DRD0K5WWFPPH4FFEE (recovery table): #34 points the README at inputs in action.yml and at the remedy printed on the failure log line.
  • 01M44NXG523X5YEG67PQVVG1W1 (composition paragraph): #32 points at WrapperDeps.
  • 01M44NZSG9QBYDHYT0A2YFHB4K (an unset origin pin still validates the URL): #34 removes the README's copy of that sentence, and #41 corrects the original in action.yml.
> Replying to comment #123439 This is review `01M44K67TVFXDNS7B3NBWNEKB8` of `3f79176`. Dispatch run 64696 completed it after an event run and two dispatch runs lost slots to model capacity and sandbox failures. The two findings with threads are answered there: the dispatch row's missing `pr_number` prerequisite goes to https://code.j4k.dev/j4k-oss/review-wrapper/pulls/43, and the variable list after the table to #32. The other five are summary-only. Each repeats a finding of the previous review about README text this PR doesn't change: - `01M44NTXBKSY1RFQ9C2QVKM4TV` (outcome table): https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30 replaces the table with a pointer to `OUTCOMES` and `OUTCOME_EXIT`. - `01M44NWD3WEGT6QN7XK59DECMZ` (action-input table) and `01M44NX49DRD0K5WWFPPH4FFEE` (recovery table): 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. - `01M44NXG523X5YEG67PQVVG1W1` (composition paragraph): https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 points at `WrapperDeps`. - `01M44NZSG9QBYDHYT0A2YFHB4K` (an unset origin pin still validates the URL): #34 removes the README's copy of that sentence, and https://code.j4k.dev/j4k-oss/review-wrapper/pulls/41 corrects the original in `action.yml`.
Author
Owner

Replying to review comment #125247

The half held above is declined: the environment table stays. It is the usage contract for someone wiring the action into another repository, and no single definition exists to point at, because src/credentials.ts, src/wrapper/event-context.ts and src/main.ts each read part of the set. The sentence after the table still gets its pointers in #32.

> Replying to review comment #125247 The half held above is declined: the environment table stays. It is the usage contract for someone wiring the action into another repository, and no single definition exists to point at, because `src/credentials.ts`, `src/wrapper/event-context.ts` and `src/main.ts` each read part of the set. The sentence after the table still gets its pointers in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32.
jercik merged commit e0fc604a4b into main 2026-10-05 09:32:51 +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!37
No description provided.