j4k-oss/review-wrapper
Watch
2
0
Fork
You've already forked review-wrapper
 0
Code Issues 12 Pull requests Releases Packages Activity Actions

docs: point to the outcome set instead of restating it #30

Merged
jercik merged 1 commit from docs/outcome-set-pointers into main 2026-10-05 09:29:08 +00:00
Conversation 9 Commits 1 Files changed 2 +23 -31
jercik commented 2026-10-04 20:56:36 +00:00
Owner
Copy link

Follow-up to #29 for its review comments 119844, 119845, 119846, and 119847.

Stacked on #29; merge after it.

🤖 Generated with Claude Code

Follow-up to #29 for its review comments [119844](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/29#issuecomment-119844), [119845](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/29#issuecomment-119845), [119846](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/29#issuecomment-119846), and [119847](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/29#issuecomment-119847). Stacked on #29; merge after it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
docs: point to the outcome set instead of restating it
Some checks failed
commit-msg / commitlint (pull_request) Successful in 22s
Details
Checks / quality-checks (pull_request) Successful in 48s
Details
Review / Review (pull_request_target) Has been cancelled
Details
11bcf572f3
 
Replace the README outcomes table, the 422 problem-type list, and the
types.ts header's row enumeration with pointers to `OUTCOMES`,
`OUTCOME_EXIT`, `FAILURE_REMEDIES`, and `toFailure`. Per-outcome detail
the README table carried moves into the `OUTCOMES` entry comments.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
docs: point at prefixFor instead of copying the log prefixes
All checks were successful
commit-msg / commitlint (pull_request) Successful in 16s
Details
Checks / quality-checks (pull_request) Successful in 33s
Details
Review / Review (pull_request_target) Successful in 9m24s
Details
c74baebf8c
 
The previous commit moved the two job-log prefixes into the OUTCOMES
comments, a second copy of the strings `prefixFor` builds. The README
now names `prefixFor` and the comments drop the copies.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
forgejo-actions commented 2026-10-04 21:11:52 +00:00
Copy link

Review 01M44D7VYFRQP4VJNS6FQ10MFF — head 9895834673b5b0c10f9369a3d02c286313ddabee

Review — j4k-oss/review-wrapper @ 61c3b02b72

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

medium — Supersession note copies the pass-bound operations from code

  • claim: 01M44DFBM0FHYZ5WA97BQNM831
  • anchor: README.md (snippet)
Supersession leaves execution waiting, coverage classification, and report accounting
pinned to one dispatch pass.
  • lens: restated-sets · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44DNBQFE0NHV9XK076S4TK7 · valid: The grounded README sentence enumerates three pass-bound operations. The reported trace names src/wrapper/orchestrator.ts and the wait, classification, and reconciliation paths using pinnedPassId; its source and prose member lists agree. This is a maintained copy of a code-defined set, so point to the pinnedPassId flow instead.
  • disposition: none

The Notes section enumerates the operations tied to a pinned dispatch pass. That copy can silently become incomplete when the pinned-pass dataflow changes, leaving readers with an outdated account of supersession.

The source defines the current members through pinnedPassId: review in src/wrapper/orchestrator.ts passes it to the wait, outcome classification, and reconciliation paths; waitForSettle in src/wrapper/wait.ts polls the pinned pass, classifyOutcome in src/wrapper/classify-outcome.ts reads its coverage, and reconcile requests its report. The README copy covers execution waiting, coverage classification, and report accounting respectively. None appears only in the source or only in the copy at this revision.

Replace this enumeration with a pointer to the pinnedPassId flow in src/wrapper/orchestrator.ts. This is undated README prose, the reader can open the defining code, and the sentence is neither an example nor a table of contents. I traced the named functions statically; I did not run the wrapper. The decisive check is whether every pass-bound operation follows this dataflow; a further operation would change the member comparison, while the existing calls establish that this is a maintained copy.

medium — Composition prose copies the reconciler set from WrapperDeps

  • claim: 01M44DFMH5QS6MV3BARXYXBA4A
  • anchor: README.md (snippet)
the three reconcilers (summary, inline, dispositions).
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44DNBQFE0NHV9XK076S4TK7 · valid: The grounded phrase counts and names the reconcilers. The current body identifies src/contract/types.ts as the WrapperDeps source and lists summary, inline, and dispositions on both sides. The copy can drift when that type changes; point to WrapperDeps. Earlier valid verdicts are consistent but do not determine this result.
  • disposition: none

The composition summary fixes the number and names of the reconcilers in prose. If WrapperDeps gains or drops a reconciler, this sentence can silently describe the wrong collaborators. The source set in src/contract/types.ts is summary, inline, and dispositions; the prose covers summary, inline, and dispositions, with no member present on only one side. The writing standard says to point to an authoritative definition instead of copying a set. Replace the phrase with “the reconcilers defined by WrapperDeps,” preserving that main.ts assembles the orchestrator dependencies. I read WrapperDeps and the object passed to runWrapper in src/main.ts; both show the same members. A changed WrapperDeps member without an edit to this sentence would establish the drift risk; a generated link from the type to this sentence would refute it, and I found none.

medium — The action input table restates action.yml

  • claim: 01M44DJ0KDFW02S42E948JP33G
  • anchor: README.md (snippet)
## Action inputs

| Input                     | Meaning                                                                                                                                                                                                                   |
| ------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `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 01M44DNBQFE0NHV9XK076S4TK7 · valid: The grounded README table names expected-service-origin as its sole action input. The body names action.yml as the runner-read manifest and reports that it defines the same sole member and origin-pin semantics. The table duplicates that contract and can drift; point to action.yml inputs. Prior matching verdicts do not supply the premise for this decision.
  • disposition: none

The README duplicates the action input definition and its origin-pin behavior, so a manifest change can leave users following stale setup guidance. action.yml defines the set with one member, expected-service-origin; the README table covers that same sole member, so neither side has a missing member today. The writing standard says to name the file that defines a set instead of copying it. Replace this table with a pointer to action.yml for the input contract; the security section already explains why pinning the service origin matters, and that reason stays. I read action.yml, the input parsing in reviewCredentials in src/credentials.ts, and the README security section. A build step that generates this table from action.yml would refute the maintenance concern; I found no such step in the subject.

medium — The recovery table duplicates the emitted failure remedies

  • claim: 01M44DJJTNTDKYW678SK206YZ4
  • anchor: README.md (snippet)
Before retrying, act on the reported cause:

| Message                                       | Recovery                                                                                                               |
| --------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- |
| `provider session limit reached`              | Wait for the session limit to reset or ask the operator to lift it.                                                    |
| `selected model at capacity`                  | Wait for the model to accept new work.                                                                                 |
| `agent execution failed`                      | Ask the operator to inspect the failed agent logs and correct the reported error.                                      |
| `sandbox infrastructure failed`               | Ask the operator to check sandbox provisioning, controller connectivity, and resources.                                |
| `execution failed for an unrecognized reason` | Copy the quoted code from `(code "…")` on that log line and ask the operator to look it up in the review service logs. |
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M44DNBQFE0NHV9XK076S4TK7 · valid: The grounded table lists five displayed failure cases and remedies. The reported trace names src/wrapper/remedies.ts and its four selected cases plus fallback, with no member mismatch, and says the wrapper emits the remedies on failure log lines. The table copies that maintained recovery set; direct readers to the emitted remedy and retain the retry instruction. The exact dependency labels are not needed for this local set comparison.
  • disposition: none

The recovery table can silently disagree with the diagnosis and remedy that the wrapper actually prints, causing an operator to act on stale advice. formatExecutionRecovery in src/wrapper/remedies.ts selects provider-session-limit, provider-capacity, agent-execution-failed, infrastructure-failure, or its unknown-code fallback; the bundled formatExecutionFailure maps these to the README messages provider session limit reached, selected model at capacity, agent execution failed, sandbox infrastructure failed, and execution failed for an unrecognized reason. The table covers all five cases, with no member on only one side now. The writing standard says to point to the defining code rather than copy its set. Replace the table with “Follow the recovery action on the failure log line before retrying”; keep the next paragraph’s distinct rerun instruction. I compared the table with remedies.ts and the formatter bundled in dist/index.mjs, then traced the triage and coverage call sites in classifyOutcome. A generated table from those definitions would refute the drift concern; I found none.

Other claims

  • grounding-pending (0)
  • ungrounded (0)
  • rejected (0)
  • duplicate-of (0)
  • unadjudicated (2)
    • 01M44DH5HKQFA3T9JJSGAEFP8M medium — Required environment variables are copied into the README
    • 01M44DK5FQJDPCKECYQK6GRXSG low — The outcome source pointer promises behavior that the array does not define

Coverage

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

lens part arm unit status runs loss
general-bug whole default no-claims 1 no
writing-quality whole default claims-emitted 1 no
test-trimming whole default no-claims 1 no
restated-sets whole default claims-emitted 1 no
project-docs whole default no-claims 1 no
<!-- review:summary --> **Review** `01M44D7VYFRQP4VJNS6FQ10MFF` — head `9895834673b5b0c10f9369a3d02c286313ddabee` # Review — j4k-oss/review-wrapper @ 61c3b02b7258 Scope: diff against base tree `47c7ab2f663e` 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 (4) ### medium — Supersession note copies the pass-bound operations from code - claim: `01M44DFBM0FHYZ5WA97BQNM831` - anchor: `README.md` (snippet) ``` Supersession leaves execution waiting, coverage classification, and report accounting pinned to one dispatch pass. ``` - lens: restated-sets · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44DNBQFE0NHV9XK076S4TK7` · valid: The grounded README sentence enumerates three pass-bound operations. The reported trace names src/wrapper/orchestrator.ts and the wait, classification, and reconciliation paths using pinnedPassId; its source and prose member lists agree. This is a maintained copy of a code-defined set, so point to the pinnedPassId flow instead. - disposition: none > The Notes section enumerates the operations tied to a pinned dispatch pass. That copy can silently become incomplete when the pinned-pass dataflow changes, leaving readers with an outdated account of supersession. > > The source defines the current members through `pinnedPassId`: `review` in `src/wrapper/orchestrator.ts` passes it to the wait, outcome classification, and reconciliation paths; `waitForSettle` in `src/wrapper/wait.ts` polls the pinned pass, `classifyOutcome` in `src/wrapper/classify-outcome.ts` reads its coverage, and `reconcile` requests its report. The README copy covers execution waiting, coverage classification, and report accounting respectively. None appears only in the source or only in the copy at this revision. > > Replace this enumeration with a pointer to the `pinnedPassId` flow in `src/wrapper/orchestrator.ts`. This is undated README prose, the reader can open the defining code, and the sentence is neither an example nor a table of contents. I traced the named functions statically; I did not run the wrapper. The decisive check is whether every pass-bound operation follows this dataflow; a further operation would change the member comparison, while the existing calls establish that this is a maintained copy. ### medium — Composition prose copies the reconciler set from WrapperDeps - claim: `01M44DFMH5QS6MV3BARXYXBA4A` - anchor: `README.md` (snippet) ``` the three reconcilers (summary, inline, dispositions). ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44DNBQFE0NHV9XK076S4TK7` · valid: The grounded phrase counts and names the reconcilers. The current body identifies src/contract/types.ts as the WrapperDeps source and lists summary, inline, and dispositions on both sides. The copy can drift when that type changes; point to WrapperDeps. Earlier valid verdicts are consistent but do not determine this result. - disposition: none > The composition summary fixes the number and names of the reconcilers in prose. If WrapperDeps gains or drops a reconciler, this sentence can silently describe the wrong collaborators. The source set in src/contract/types.ts is summary, inline, and dispositions; the prose covers summary, inline, and dispositions, with no member present on only one side. The writing standard says to point to an authoritative definition instead of copying a set. Replace the phrase with “the reconcilers defined by WrapperDeps,” preserving that main.ts assembles the orchestrator dependencies. I read WrapperDeps and the object passed to runWrapper in src/main.ts; both show the same members. A changed WrapperDeps member without an edit to this sentence would establish the drift risk; a generated link from the type to this sentence would refute it, and I found none. ### medium — The action input table restates action.yml - claim: `01M44DJ0KDFW02S42E948JP33G` - anchor: `README.md` (snippet) ``` ## Action inputs | Input | Meaning | | ------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `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 `01M44DNBQFE0NHV9XK076S4TK7` · valid: The grounded README table names expected-service-origin as its sole action input. The body names action.yml as the runner-read manifest and reports that it defines the same sole member and origin-pin semantics. The table duplicates that contract and can drift; point to action.yml inputs. Prior matching verdicts do not supply the premise for this decision. - disposition: none > The README duplicates the action input definition and its origin-pin behavior, so a manifest change can leave users following stale setup guidance. action.yml defines the set with one member, expected-service-origin; the README table covers that same sole member, so neither side has a missing member today. The writing standard says to name the file that defines a set instead of copying it. Replace this table with a pointer to action.yml for the input contract; the security section already explains why pinning the service origin matters, and that reason stays. I read action.yml, the input parsing in reviewCredentials in src/credentials.ts, and the README security section. A build step that generates this table from action.yml would refute the maintenance concern; I found no such step in the subject. ### medium — The recovery table duplicates the emitted failure remedies - claim: `01M44DJJTNTDKYW678SK206YZ4` - anchor: `README.md` (snippet) ``` Before retrying, act on the reported cause: | Message | Recovery | | --------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | | `provider session limit reached` | Wait for the session limit to reset or ask the operator to lift it. | | `selected model at capacity` | Wait for the model to accept new work. | | `agent execution failed` | Ask the operator to inspect the failed agent logs and correct the reported error. | | `sandbox infrastructure failed` | Ask the operator to check sandbox provisioning, controller connectivity, and resources. | | `execution failed for an unrecognized reason` | Copy the quoted code from `(code "…")` on that log line and ask the operator to look it up in the review service logs. | ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44DNBQFE0NHV9XK076S4TK7` · valid: The grounded table lists five displayed failure cases and remedies. The reported trace names src/wrapper/remedies.ts and its four selected cases plus fallback, with no member mismatch, and says the wrapper emits the remedies on failure log lines. The table copies that maintained recovery set; direct readers to the emitted remedy and retain the retry instruction. The exact dependency labels are not needed for this local set comparison. - disposition: none > The recovery table can silently disagree with the diagnosis and remedy that the wrapper actually prints, causing an operator to act on stale advice. formatExecutionRecovery in src/wrapper/remedies.ts selects provider-session-limit, provider-capacity, agent-execution-failed, infrastructure-failure, or its unknown-code fallback; the bundled formatExecutionFailure maps these to the README messages provider session limit reached, selected model at capacity, agent execution failed, sandbox infrastructure failed, and execution failed for an unrecognized reason. The table covers all five cases, with no member on only one side now. The writing standard says to point to the defining code rather than copy its set. Replace the table with “Follow the recovery action on the failure log line before retrying”; keep the next paragraph’s distinct rerun instruction. I compared the table with remedies.ts and the formatter bundled in dist/index.mjs, then traced the triage and coverage call sites in classifyOutcome. A generated table from those definitions would refute the drift concern; I found none. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (2) - `01M44DH5HKQFA3T9JJSGAEFP8M` medium — Required environment variables are copied into the README - `01M44DK5FQJDPCKECYQK6GRXSG` low — The outcome source pointer promises behavior that the array does not define ## Coverage Coverage pass: 01M44D7W1HGECNQEB92JBMYYV2 Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | claims-emitted | 1 | no | | project-docs | whole | default | no-claims | 1 | no |
🚀 1
forgejo-actions reviewed 2026-10-04 21:11:52 +00:00
README.md Outdated
Lines 74-77
@ -86,2 +73,5 @@
| 12 | `head-moved` | No PR comment or conversation-resolution writes; the newer run owns the comment surface. See the timing below. | 0 |
| 13 | `diff-unreadable` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 |
The outcomes and their exit codes are `OUTCOMES` and `OUTCOME_EXIT` in
`src/contract/types.ts`, where each outcome's entry carries its behaviour. The remedy
that a failure's log line names is chosen by `FAILURE_REMEDIES` in
`src/wrapper/orchestrator.ts`, and a wait failure's log prefix by `prefixFor` in
`src/wrapper/remedies.ts`.
forgejo-actions commented 2026-10-04 21:11:52 +00:00
Copy link

medium — Failure-remedy pointer omits wait and coverage recovery code

The new pointer sends a reader looking for a failure log remedy to FAILURE_REMEDIES, but that map supplies text only when runWrapper catches WrapperFailureError. Wait, triage, and partial-coverage failures build their log remedies in formatStuckRemedy and formatExecutionRecovery. Someone changing the recovery instruction printed for a failed lens slot would edit or inspect the wrong place.

In runWrapper, FAILURE_REMEDIES is consulted in the WrapperFailureError catch. classifyOutcome instead calls formatStuckRemedy for unsettled and partial-coverage results; that function calls formatExecutionRecovery for recorded execution failures. prefixFor selects only the log prefix. The writing standard asks for precise, verifiable pointers to the actual source.

Narrow the first clause to WrapperFailureError remedies, and point wait and coverage recovery messages to formatStuckRemedy and formatExecutionRecovery in src/wrapper/remedies.ts. Keep prefixFor as the pointer for their prefix. I traced runWrapper, classifyOutcome, and the remedy functions; this is static reasoning about those call paths. A separate logger bypassing these functions could change the coverage of the revised pointer, but none appears in the paths I read.

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

<!-- review:claim:01M44BXBNSKYRF476WQ2FV9KNB --> **medium** — Failure-remedy pointer omits wait and coverage recovery code > The new pointer sends a reader looking for a failure log remedy to FAILURE_REMEDIES, but that map supplies text only when runWrapper catches WrapperFailureError. Wait, triage, and partial-coverage failures build their log remedies in formatStuckRemedy and formatExecutionRecovery. Someone changing the recovery instruction printed for a failed lens slot would edit or inspect the wrong place. > > In runWrapper, FAILURE_REMEDIES is consulted in the WrapperFailureError catch. classifyOutcome instead calls formatStuckRemedy for unsettled and partial-coverage results; that function calls formatExecutionRecovery for recorded execution failures. prefixFor selects only the log prefix. The writing standard asks for precise, verifiable pointers to the actual source. > > Narrow the first clause to WrapperFailureError remedies, and point wait and coverage recovery messages to formatStuckRemedy and formatExecutionRecovery in src/wrapper/remedies.ts. Keep prefixFor as the pointer for their prefix. I traced runWrapper, classifyOutcome, and the remedy functions; this is static reasoning about those call paths. A separate logger bypassing these functions could change the coverage of the revised pointer, but none appears in the paths I read. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44BXBNSKYRF476WQ2FV9KNB` of review `01M44BKEX0F9Z6R1V9DNFCF1R2`
👍 1
jercik commented 2026-10-04 21:34:26 +00:00
Author
Owner
Copy link

Fixed in 591c92f. The Outcomes section now sends a session failure's remedy to FAILURE_REMEDIES and a stuck or partial-coverage run's log line, remedy included, to formatStuckRemedy and formatExecutionRecovery in src/wrapper/remedies.ts.

<!-- gh-feedback:reply-to:122214 --> Fixed in 591c92f. The Outcomes section now sends a session failure's remedy to `FAILURE_REMEDIES` and a stuck or partial-coverage run's log line, remedy included, to `formatStuckRemedy` and `formatExecutionRecovery` in `src/wrapper/remedies.ts`.
jercik marked this conversation as resolved
README.md
@ -159,7 +151,7 @@ re-ask command printed in the partial-coverage diagnostic.
`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
the three reconcilers (summary, inline, dispositions). The orchestrator's exit code
forgejo-actions commented 2026-10-04 21:11:52 +00:00
Copy link

medium — Composition paragraph duplicates the reconciler set

The composition paragraph repeats the complete set of reconcilers wired in main.ts. A future change to that wiring can make the count and names here stale without any failure in the README, sending a maintainer to the wrong collaborators. The copy currently agrees with the code.

main.ts constructs createSummaryReconciler, createInlineReconciler, and createDispositionProjector and passes them as summary, inline, and dispositions. The README covers summary, inline, and dispositions; no member appears on only one side. The restated-sets rule and the writing standard’s One Idea, One Place guidance favor pointing at the wiring rather than maintaining the list here.

Say that main.ts builds the forge client and passes the collaborators it constructs to the orchestrator, without a count or member list. Keep the useful explanation that its returned exit code becomes the process exit code. I read the whole README composition paragraph and main.ts; this is a static source comparison. Reading any alternate entry point would establish whether the paragraph needs a wider scope, but main.ts is the entry point declared in action.yml.

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

<!-- review:claim:01M44BVNK6Z2MXP8KQH0V6PQ53 --> **medium** — Composition paragraph duplicates the reconciler set > The composition paragraph repeats the complete set of reconcilers wired in main.ts. A future change to that wiring can make the count and names here stale without any failure in the README, sending a maintainer to the wrong collaborators. The copy currently agrees with the code. > > main.ts constructs createSummaryReconciler, createInlineReconciler, and createDispositionProjector and passes them as summary, inline, and dispositions. The README covers summary, inline, and dispositions; no member appears on only one side. The restated-sets rule and the writing standard’s One Idea, One Place guidance favor pointing at the wiring rather than maintaining the list here. > > Say that main.ts builds the forge client and passes the collaborators it constructs to the orchestrator, without a count or member list. Keep the useful explanation that its returned exit code becomes the process exit code. I read the whole README composition paragraph and main.ts; this is a static source comparison. Reading any alternate entry point would establish whether the paragraph needs a wider scope, but main.ts is the entry point declared in action.yml. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44BVNK6Z2MXP8KQH0V6PQ53` of review `01M44BKEX0F9Z6R1V9DNFCF1R2`
🚀 1
jercik marked this conversation as resolved
docs: point stuck and coverage remedies at their formatters
Some checks failed
commit-msg / commitlint (pull_request) Successful in 17s
Details
Checks / quality-checks (pull_request) Successful in 49s
Details
Review / Review (pull_request_target) Has been cancelled
Details
591c92fc37
 
`FAILURE_REMEDIES` supplies a remedy only when the service session fails. A
stuck or partial-coverage run's log line comes from `formatStuckRemedy` and
`formatExecutionRecovery`, so the README now names those for that case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jercik commented 2026-10-04 21:34:27 +00:00
Author
Owner
Copy link

Replying to review comment #122213

Tracked in #32, which replaces the collaborator list in the composition paragraph with a pointer to WrapperDeps in src/contract/types.ts. It is stacked on this PR.

> Replying to review comment #122213 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32, which replaces the collaborator list in the composition paragraph with a pointer to `WrapperDeps` in `src/contract/types.ts`. It is stacked on this PR.
jercik commented 2026-10-04 21:34:28 +00:00
Author
Owner
Copy link

Replying to comment #122212

The unadjudicated claim 01M44BTESXJKH5PCCHGSW3K4W5 is valid: the paragraph under the environment table listed the forge and runner variables a second time, and nothing would fail when the two lists drift. Tracked in #32, which replaces that list with a pointer to forgeCredentials, readWrapperInputs and src/main.ts, the code that reads those variables. It is stacked on this PR.

> Replying to comment #122212 The unadjudicated claim `01M44BTESXJKH5PCCHGSW3K4W5` is valid: the paragraph under the environment table listed the forge and runner variables a second time, and nothing would fail when the two lists drift. Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32, which replaces that list with a pointer to `forgeCredentials`, `readWrapperInputs` and `src/main.ts`, the code that reads those variables. It is stacked on this PR.
forgejo-actions reviewed 2026-10-04 21:41:16 +00:00
README.md
@ -159,7 +151,7 @@ re-ask command printed in the partial-coverage diagnostic.
`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
the three reconcilers (summary, inline, dispositions). The orchestrator's exit code
forgejo-actions commented 2026-10-04 21:41:16 +00:00
Copy link

medium — Composition prose copies the reconciler set from WrapperDeps

The composition summary fixes the number and names of the reconcilers in prose. If WrapperDeps gains or drops a reconciler, this sentence can silently describe the wrong collaborators. The source set in src/contract/types.ts is summary, inline, and dispositions; the prose covers summary, inline, and dispositions, with no member present on only one side. The writing standard says to point to an authoritative definition instead of copying a set. Replace the phrase with “the reconcilers defined by WrapperDeps,” preserving that main.ts assembles the orchestrator dependencies. I read WrapperDeps and the object passed to runWrapper in src/main.ts; both show the same members. A changed WrapperDeps member without an edit to this sentence would establish the drift risk; a generated link from the type to this sentence would refute it, and I found none.

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

<!-- review:claim:01M44DFMH5QS6MV3BARXYXBA4A --> **medium** — Composition prose copies the reconciler set from WrapperDeps > The composition summary fixes the number and names of the reconcilers in prose. If WrapperDeps gains or drops a reconciler, this sentence can silently describe the wrong collaborators. The source set in src/contract/types.ts is summary, inline, and dispositions; the prose covers summary, inline, and dispositions, with no member present on only one side. The writing standard says to point to an authoritative definition instead of copying a set. Replace the phrase with “the reconcilers defined by WrapperDeps,” preserving that main.ts assembles the orchestrator dependencies. I read WrapperDeps and the object passed to runWrapper in src/main.ts; both show the same members. A changed WrapperDeps member without an edit to this sentence would establish the drift risk; a generated link from the type to this sentence would refute it, and I found none. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44DFMH5QS6MV3BARXYXBA4A` of review `01M44D7VYFRQP4VJNS6FQ10MFF`
🚀 1
jercik marked this conversation as resolved
README.md Outdated
Lines 161-162
@ -170,3 +162,2 @@
Row 11 pins execution waiting, coverage classification, and report accounting to one
dispatch pass. The report request carries that `pass_id` even if another dispatch
becomes latest. Claims, findings, and triage settlement remain review-wide facts.
Supersession leaves execution waiting, coverage classification, and report accounting
pinned to one dispatch pass. The report request carries that `pass_id` even if another
forgejo-actions commented 2026-10-04 21:41:16 +00:00
Copy link

medium — Supersession note copies the pass-bound operations from code

The Notes section enumerates the operations tied to a pinned dispatch pass. That copy can silently become incomplete when the pinned-pass dataflow changes, leaving readers with an outdated account of supersession.

The source defines the current members through pinnedPassId: review in src/wrapper/orchestrator.ts passes it to the wait, outcome classification, and reconciliation paths; waitForSettle in src/wrapper/wait.ts polls the pinned pass, classifyOutcome in src/wrapper/classify-outcome.ts reads its coverage, and reconcile requests its report. The README copy covers execution waiting, coverage classification, and report accounting respectively. None appears only in the source or only in the copy at this revision.

Replace this enumeration with a pointer to the pinnedPassId flow in src/wrapper/orchestrator.ts. This is undated README prose, the reader can open the defining code, and the sentence is neither an example nor a table of contents. I traced the named functions statically; I did not run the wrapper. The decisive check is whether every pass-bound operation follows this dataflow; a further operation would change the member comparison, while the existing calls establish that this is a maintained copy.

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

<!-- review:claim:01M44DFBM0FHYZ5WA97BQNM831 --> **medium** — Supersession note copies the pass-bound operations from code > The Notes section enumerates the operations tied to a pinned dispatch pass. That copy can silently become incomplete when the pinned-pass dataflow changes, leaving readers with an outdated account of supersession. > > The source defines the current members through `pinnedPassId`: `review` in `src/wrapper/orchestrator.ts` passes it to the wait, outcome classification, and reconciliation paths; `waitForSettle` in `src/wrapper/wait.ts` polls the pinned pass, `classifyOutcome` in `src/wrapper/classify-outcome.ts` reads its coverage, and `reconcile` requests its report. The README copy covers execution waiting, coverage classification, and report accounting respectively. None appears only in the source or only in the copy at this revision. > > Replace this enumeration with a pointer to the `pinnedPassId` flow in `src/wrapper/orchestrator.ts`. This is undated README prose, the reader can open the defining code, and the sentence is neither an example nor a table of contents. I traced the named functions statically; I did not run the wrapper. The decisive check is whether every pass-bound operation follows this dataflow; a further operation would change the member comparison, while the existing calls establish that this is a maintained copy. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44DFBM0FHYZ5WA97BQNM831` of review `01M44D7VYFRQP4VJNS6FQ10MFF`
🚀 1
jercik marked this conversation as resolved
jercik commented 2026-10-04 21:57:08 +00:00
Author
Owner
Copy link

Replying to review comment #122779

Valid. The phrase names the three reconcilers that WrapperDeps in src/contract/types.ts already defines, and nothing would flag the copy if that type changed. Tracked in #32, which rewrites the Composition paragraph to say src/main.ts hands the orchestrator "the collaborators that WrapperDeps in src/contract/types.ts names", with no list. It is stacked on this PR.

> Replying to review comment #122779 Valid. The phrase names the three reconcilers that `WrapperDeps` in `src/contract/types.ts` already defines, and nothing would flag the copy if that type changed. Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32, which rewrites the Composition paragraph to say `src/main.ts` hands the orchestrator "the collaborators that `WrapperDeps` in `src/contract/types.ts` names", with no list. It is stacked on this PR.
jercik commented 2026-10-04 21:59:58 +00:00
Author
Owner
Copy link

Replying to review comment #122778

Valid. Tracked in #36, which replaces the list of pass-bound operations with a pointer to the pinnedPassId that review in src/wrapper/orchestrator.ts pins and hands to the steps that take a pass id. Checking the source also showed the old sentence overstated the wait: inside waitForSettle, only the stage-1 coverage poll is scoped to the pinned pass, and the wait ends on the review-level triage settle. It is stacked on this PR.

> Replying to review comment #122778 Valid. Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/36, which replaces the list of pass-bound operations with a pointer to the `pinnedPassId` that `review` in `src/wrapper/orchestrator.ts` pins and hands to the steps that take a pass id. Checking the source also showed the old sentence overstated the wait: inside `waitForSettle`, only the stage-1 coverage poll is scoped to the pinned pass, and the wait ends on the review-level triage settle. It is stacked on this PR.
jercik commented 2026-10-04 21:59:59 +00:00
Author
Owner
Copy link

Replying to comment #122212

This is review 01M44D7VYFRQP4VJNS6FQ10MFF of 591c92f. Two of its findings are summary-only, and it left two claims unadjudicated.

  • 01M44DJ0KDFW02S42E948JP33G ("The action input table restates action.yml") and 01M44DJJTNTDKYW678SK206YZ4 ("The recovery table duplicates the emitted failure remedies") are valid. The review of #32 raised the same two points. Tracked in #34, which points at inputs in action.yml and at the remedy printed on the failure log line. It targets main, because both tables are older than this PR.
  • 01M44DK5FQJDPCKECYQK6GRXSG ("The outcome source pointer promises behavior that the array does not define") is valid. OUTCOMES holds only names, with a comment beside each entry that summarizes its behaviour. Tracked in #36, which rewords the pointer to promise only that. It is stacked on this PR.
  • 01M44DH5HKQFA3T9JJSGAEFP8M ("Required environment variables are copied into the README") is held, not applied. The environment table is the only place that tells a calling workflow which variables to set and what each must hold, and no single source file defines that set: src/credentials.ts, src/wrapper/event-context.ts and src/main.ts each read part of it. Replacing the table with pointers changes what the README promises the workflows that call this action, so it waits for that decision.
> Replying to comment #122212 This is review `01M44D7VYFRQP4VJNS6FQ10MFF` of `591c92f`. Two of its findings are summary-only, and it left two claims unadjudicated. - `01M44DJ0KDFW02S42E948JP33G` ("The action input table restates action.yml") and `01M44DJJTNTDKYW678SK206YZ4` ("The recovery table duplicates the emitted failure remedies") are valid. The review of #32 raised the same two points. Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/34, which points at `inputs` in `action.yml` and at the remedy printed on the failure log line. It targets `main`, because both tables are older than this PR. - `01M44DK5FQJDPCKECYQK6GRXSG` ("The outcome source pointer promises behavior that the array does not define") is valid. `OUTCOMES` holds only names, with a comment beside each entry that summarizes its behaviour. Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/36, which rewords the pointer to promise only that. It is stacked on this PR. - `01M44DH5HKQFA3T9JJSGAEFP8M` ("Required environment variables are copied into the README") is held, not applied. The environment table is the only place that tells a calling workflow which variables to set and what each must hold, and no single source file defines that set: `src/credentials.ts`, `src/wrapper/event-context.ts` and `src/main.ts` each read part of it. Replacing the table with pointers changes what the README promises the workflows that call this action, so it waits for that decision.
jercik commented 2026-10-05 01:07:09 +00:00
Author
Owner
Copy link

Replying to comment #122212

01M44DH5HKQFA3T9JJSGAEFP8M (the README copies the required environment variables) is declined; it was held above. The table is the usage contract for someone wiring the action into another repository: which variables to set and what each must hold. 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.

> Replying to comment #122212 `01M44DH5HKQFA3T9JJSGAEFP8M` (the README copies the required environment variables) is declined; it was held above. The table is the usage contract for someone wiring the action into another repository: which variables to set and what each must hold. 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.
jercik changed target branch from feat/same-diff-attach to main 2026-10-05 09:24:23 +00:00
jercik force-pushed docs/outcome-set-pointers from 591c92fc37
Some checks failed
commit-msg / commitlint (pull_request) Successful in 17s
Details
Checks / quality-checks (pull_request) Successful in 49s
Details
Review / Review (pull_request_target) Has been cancelled
Details
to 9895834673
All checks were successful
Review / Review (pull_request_target) Successful in 5s
Details
commit-msg / commitlint (pull_request) Successful in 17s
Details
Checks / quality-checks (pull_request) Successful in 32s
Details
2026-10-05 09:24:27 +00:00 Compare
jercik merged commit 36f2c5a3a2 into main 2026-10-05 09:29:08 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!30
No description provided.