docs: drop the false "never computed" from the inputs header #40

Merged

jercik merged 1 commit from docs/wrapper-inputs-header into main 2026-10-05 09:34:46 +00:00

Conversation 2 Commits 1 Files changed 1 +1 -1

jercik commented 2026-10-04 23:03:05 +00:00

Owner

Copy link

Follow-up to #33, from review 01M44FMS560BRRJR823QSZRW2E (summary-only finding 01M44HRFXHK0QP5RCK8NKVCP1J).

The WrapperInputs section header in src/contract/types.ts said the inputs are "derived from the event, never computed". readWrapperInputs computes isFork and reTriageAskKey, lowercases the forge host and parses the run attempt, and on workflow_dispatch it fetches the head from the forge. The header now keeps only the contract pointer.

It targets main, because the header is older than #33.

🤖 Generated with Claude Code

Follow-up to #33, from review `01M44FMS560BRRJR823QSZRW2E` (summary-only finding `01M44HRFXHK0QP5RCK8NKVCP1J`). The `WrapperInputs` section header in `src/contract/types.ts` said the inputs are "derived from the event, never computed". `readWrapperInputs` computes `isFork` and `reTriageAskKey`, lowercases the forge host and parses the run attempt, and on `workflow_dispatch` it fetches the head from the forge. The header now keeps only the contract pointer. It targets `main`, because the header is older than #33. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit 2026-10-04 23:03:05 +00:00

docs: drop the false "never computed" from the inputs header ... 1798291543

readWrapperInputs computes isFork, lowercases the forge host, parses the run attempt and
builds reTriageAskKey, and on workflow_dispatch it fetches the head from the forge, so
"derived from the event, never computed" is false.

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

jercik referenced this pull request 2026-10-04 23:04:10 +00:00

docs: say the fork gate guards only the review credentials #33

forgejo-actions commented 2026-10-04 23:05:39 +00:00

Copy link

Review 01M44JG9B4Q3392GK4XWBNRQK9 — head 1798291543612c750da7ee661fb6a611f3171e4a

Review — j4k-oss/review-wrapper @ ed41ca6caa

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 (4)

medium — Fork verdict comment claims it precedes all credential reads

• claim: 01M44JXG0TSH55KMB30BY85HVH
• anchor: src/contract/types.ts (snippet)

  /** head.repo.full_name !== slug — evaluated before any credential is read. */

• lens: writing-quality · arm: default
• verdicts: 1 valid / 0 invalid / 0 uncertain
  • pass 01M44K2M9XV6KCQ1PDT74JKQ3Q · valid: The exact grounded comment promises that the fork comparison runs before any credential read. The reviewer's specific call trace places forgeCredentials() before readWrapperInputs() establishes isFork, while reviewCredentials() runs only after the fork gate. That makes the broad ordering guarantee false; limiting it to the review capability token preserves the actual guarantee.
• disposition: none

The comment gives security reviewers the wrong ordering guarantee: a fork run reads the Forgejo task token before it determines isFork. A reader could therefore assume the fork gate protects that token as well as the review capability token.

main.ts calls forgeCredentials() while creating the forge client, then calls readWrapperInputs(forge). That function sets isFork from the event payload or forge.getPull; runWrapper checks it later. The actual protected boundary is reviewCredentials(), which is called only by openSession after the fork check.

Change the comment to say the fork verdict is established before reviewCredentials() reads the capability token. This preserves the intended security guarantee without implying that no credential has been read.

I traced main.ts, forgeCredentials() in src/credentials.ts, readWrapperInputs() in src/wrapper/event-context.ts, and the fork branch in src/wrapper/orchestrator.ts. This is a static call-order finding; a trace showing forgeCredentials() runs after isFork would refute it.

medium — Inline reconciler contract promises a thread per finding

• claim: 01M44K0VQRN7Y3M9EDNWXDQRFG
• anchor: src/contract/types.ts (snippet)

  /** One thread per promoted finding whose anchor maps onto the PR diff;
   *  unmappable findings stay summary-only (never dropped silently). */

• lens: writing-quality · arm: default
• verdicts: 1 valid / 0 invalid / 0 uncertain
  • pass 01M44K2M9XV6KCQ1PDT74JKQ3Q · valid: The grounded contract says one thread per mapped finding. The reviewer's concrete trace reports that the reconciler posts an inline comment per finding but conversation-markers groups claims on the same display line into one conversation and dispositions settles them together. The comment therefore names the wrong unit; 'one inline comment' is the narrower accurate contract.
• disposition: none

This contract promises one thread for each mapped finding, but two findings anchored to the same display line can share a Forgejo conversation. A maintainer relying on the comment could treat thread resolution as a one-claim decision, which would mishandle the other claim in that conversation.

createInlineReconciler() adds one inline comment for each mapped finding. src/reconcile/conversation-markers.ts explicitly says that claims mapped to the same display line share one conversation, and src/reconcile/dispositions.ts settles all claims in a conversation together.

Say One inline comment per promoted finding whose anchor maps onto the PR diff while keeping the summary-only statement. That names the unit the code actually creates and preserves the guarantee that unmappable findings remain in the summary.

I traced createInlineReconciler(), claimIdsOf(), and the conversation resolution path. The conclusion is based on the repository's conversation model; a Forgejo guarantee of separate conversations for comments at one display line would refute the premise.

medium — Head SHA comment incorrectly pins dispatch runs to event time

• claim: 01M44K1Z23FBMBS7ZXYYQEDM2Y
• anchor: src/contract/types.ts (snippet)

  /** PR head SHA as of the event (workflow_dispatch: fetched via the forge). */

• lens: writing-quality · arm: default
• verdicts: 1 valid / 0 invalid / 0 uncertain
  • pass 01M44K2M9XV6KCQ1PDT74JKQ3Q · valid: The grounded comment says the PR head SHA is as of the event even for workflow_dispatch. The reviewer's branch trace says dispatch obtains pull.headSha from a later forge.getPull() call, whereas pull_request_target reads the event payload. The dispatch value can therefore reflect a later head, so the proposed source-specific wording fixes a real timing ambiguity.
• disposition: none

For workflow_dispatch, this wording makes the head SHA sound fixed at event creation. If the PR head changes before the action runs, the wrapper reviews the head returned by its later forge lookup instead. A reader investigating which commit a manual run reviewed could infer the wrong snapshot.

readWrapperInputs() reads pull.headSha from forge.getPull(prNumber) for dispatch, while the pull_request_target branch reads pull.head.sha from the event payload. The comment's parenthetical mentions the lookup but does not correct the event-time assertion.

Say PR head SHA from the event payload for pull_request_target, or from the forge lookup for workflow_dispatch. This preserves both input sources and makes their different timing explicit.

I traced both branches of readWrapperInputs() in src/wrapper/event-context.ts and its call from src/main.ts. The finding is based on source order; evidence that Forgejo supplies an event-time snapshot to getPull() would refute the dispatch timing consequence.

low — Service session heading points to a nonexistent module

• claim: 01M44JYTM6XFMG5EG6CWK9401J
• anchor: src/contract/types.ts (snippet)

// ---- Service session (module: service-session) ----

• lens: writing-quality · arm: default
• verdicts: 1 valid / 0 invalid / 0 uncertain
  • pass 01M44K2M9XV6KCQ1PDT74JKQ3Q · valid: The grounded heading points readers to a service-session module. The reviewer reports a path and occurrence search finding no such module and identifies createReviewSession() in src/review/session.ts as the implementation. On that concrete account, the pointer is misleading, and removing or correcting the module name preserves the useful heading.
• disposition: none

A maintainer following this module pointer cannot find the service session implementation: the tree has no service-session module. The incorrect name makes this contract harder to trace to its implementation.

The interface is implemented by createReviewSession() in src/review/session.ts, which src/main.ts imports. The adjacent section headings use actual module names such as wait and orchestrator.

Remove the parenthetical module name, leaving the Service session heading, or point to src/review/session.ts. The heading still groups the session contract without sending readers to a nonexistent module.

I checked the tree's file paths and searched for service-session; this comment is its only occurrence. A file or importable module actually named service-session in the reviewed tree would refute the finding.

Other claims

• grounding-pending (0)
• ungrounded (0)
• rejected (0)
• duplicate-of (0)
• unadjudicated (1)
  • 01M44JZWKKWYZG8WNV9JTQFT9V medium — Outcome header restates the set already defined by OUTCOMES

Coverage

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

lens	part	arm	unit status	runs	loss
general-bug	whole	default	no-claims	1	no
writing-quality	whole	default	no-claims	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** `01M44JG9B4Q3392GK4XWBNRQK9` — head `1798291543612c750da7ee661fb6a611f3171e4a` # Review — j4k-oss/review-wrapper @ ed41ca6caac8 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 (4) ### medium — Fork verdict comment claims it precedes all credential reads - claim: `01M44JXG0TSH55KMB30BY85HVH` - anchor: `src/contract/types.ts` (snippet) ``` /** head.repo.full_name !== slug — evaluated before any credential is read. */ ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44K2M9XV6KCQ1PDT74JKQ3Q` · valid: The exact grounded comment promises that the fork comparison runs before any credential read. The reviewer's specific call trace places forgeCredentials() before readWrapperInputs() establishes isFork, while reviewCredentials() runs only after the fork gate. That makes the broad ordering guarantee false; limiting it to the review capability token preserves the actual guarantee. - disposition: none > The comment gives security reviewers the wrong ordering guarantee: a fork run reads the Forgejo task token before it determines `isFork`. A reader could therefore assume the fork gate protects that token as well as the review capability token. > > `main.ts` calls `forgeCredentials()` while creating the forge client, then calls `readWrapperInputs(forge)`. That function sets `isFork` from the event payload or `forge.getPull`; `runWrapper` checks it later. The actual protected boundary is `reviewCredentials()`, which is called only by `openSession` after the fork check. > > Change the comment to say the fork verdict is established before `reviewCredentials()` reads the capability token. This preserves the intended security guarantee without implying that no credential has been read. > > I traced `main.ts`, `forgeCredentials()` in `src/credentials.ts`, `readWrapperInputs()` in `src/wrapper/event-context.ts`, and the fork branch in `src/wrapper/orchestrator.ts`. This is a static call-order finding; a trace showing `forgeCredentials()` runs after `isFork` would refute it. ### medium — Inline reconciler contract promises a thread per finding - claim: `01M44K0VQRN7Y3M9EDNWXDQRFG` - anchor: `src/contract/types.ts` (snippet) ``` /** One thread per promoted finding whose anchor maps onto the PR diff; * unmappable findings stay summary-only (never dropped silently). */ ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44K2M9XV6KCQ1PDT74JKQ3Q` · valid: The grounded contract says one thread per mapped finding. The reviewer's concrete trace reports that the reconciler posts an inline comment per finding but conversation-markers groups claims on the same display line into one conversation and dispositions settles them together. The comment therefore names the wrong unit; 'one inline comment' is the narrower accurate contract. - disposition: none > This contract promises one thread for each mapped finding, but two findings anchored to the same display line can share a Forgejo conversation. A maintainer relying on the comment could treat thread resolution as a one-claim decision, which would mishandle the other claim in that conversation. > > `createInlineReconciler()` adds one inline comment for each mapped finding. `src/reconcile/conversation-markers.ts` explicitly says that claims mapped to the same display line share one conversation, and `src/reconcile/dispositions.ts` settles all claims in a conversation together. > > Say `One inline comment per promoted finding whose anchor maps onto the PR diff` while keeping the summary-only statement. That names the unit the code actually creates and preserves the guarantee that unmappable findings remain in the summary. > > I traced `createInlineReconciler()`, `claimIdsOf()`, and the conversation resolution path. The conclusion is based on the repository's conversation model; a Forgejo guarantee of separate conversations for comments at one display line would refute the premise. ### medium — Head SHA comment incorrectly pins dispatch runs to event time - claim: `01M44K1Z23FBMBS7ZXYYQEDM2Y` - anchor: `src/contract/types.ts` (snippet) ``` /** PR head SHA as of the event (workflow_dispatch: fetched via the forge). */ ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44K2M9XV6KCQ1PDT74JKQ3Q` · valid: The grounded comment says the PR head SHA is as of the event even for workflow_dispatch. The reviewer's branch trace says dispatch obtains pull.headSha from a later forge.getPull() call, whereas pull_request_target reads the event payload. The dispatch value can therefore reflect a later head, so the proposed source-specific wording fixes a real timing ambiguity. - disposition: none > For `workflow_dispatch`, this wording makes the head SHA sound fixed at event creation. If the PR head changes before the action runs, the wrapper reviews the head returned by its later forge lookup instead. A reader investigating which commit a manual run reviewed could infer the wrong snapshot. > > `readWrapperInputs()` reads `pull.headSha` from `forge.getPull(prNumber)` for dispatch, while the `pull_request_target` branch reads `pull.head.sha` from the event payload. The comment's parenthetical mentions the lookup but does not correct the event-time assertion. > > Say `PR head SHA from the event payload for pull_request_target, or from the forge lookup for workflow_dispatch`. This preserves both input sources and makes their different timing explicit. > > I traced both branches of `readWrapperInputs()` in `src/wrapper/event-context.ts` and its call from `src/main.ts`. The finding is based on source order; evidence that Forgejo supplies an event-time snapshot to `getPull()` would refute the dispatch timing consequence. ### low — Service session heading points to a nonexistent module - claim: `01M44JYTM6XFMG5EG6CWK9401J` - anchor: `src/contract/types.ts` (snippet) ``` // ---- Service session (module: service-session) ---- ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M44K2M9XV6KCQ1PDT74JKQ3Q` · valid: The grounded heading points readers to a service-session module. The reviewer reports a path and occurrence search finding no such module and identifies createReviewSession() in src/review/session.ts as the implementation. On that concrete account, the pointer is misleading, and removing or correcting the module name preserves the useful heading. - disposition: none > A maintainer following this module pointer cannot find the service session implementation: the tree has no `service-session` module. The incorrect name makes this contract harder to trace to its implementation. > > The interface is implemented by `createReviewSession()` in `src/review/session.ts`, which `src/main.ts` imports. The adjacent section headings use actual module names such as `wait` and `orchestrator`. > > Remove the parenthetical module name, leaving the `Service session` heading, or point to `src/review/session.ts`. The heading still groups the session contract without sending readers to a nonexistent module. > > I checked the tree's file paths and searched for `service-session`; this comment is its only occurrence. A file or importable module actually named `service-session` in the reviewed tree would refute the finding. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (1) - `01M44JZWKKWYZG8WNV9JTQFT9V` medium — Outcome header restates the set already defined by OUTCOMES ## Coverage Coverage pass: 01M44KE4D3NZVZM6MTEMA0315Z Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | no-claims | 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 |

🚀 1

jercik referenced this pull request 2026-10-04 23:21:43 +00:00

docs: correct three contract comments in types.ts #42

jercik commented 2026-10-04 23:22:43 +00:00

Author

Owner

Copy link

Replying to comment #124268

This is review 01M44JG9B4Q3392GK4XWBNRQK9 of 1798291. Run 64246 and dispatch run 64281 each lost slots to model capacity, so dispatch run 64381 re-asks the general-bug, test-trimming and restated-sets slots. All five claims are about comments in src/contract/types.ts that this PR doesn't change:

• 01M44JXG0TSH55KMB30BY85HVH (the isFork comment says the verdict comes before any credential read) is valid: main.ts reads the forge token before readWrapperInputs sets isFork. #33 changes the comment to say the verdict comes before reviewCredentials() is called.
• 01M44K0VQRN7Y3M9EDNWXDQRFG (the inline reconciler promises a thread per finding), 01M44K1Z23FBMBS7ZXYYQEDM2Y (the headSha comment says "as of the event" for dispatch runs) and 01M44JYTM6XFMG5EG6CWK9401J (the session heading names a service-session module that doesn't exist) are valid. #42 corrects all three. It targets main, because the comments are older than this PR.
• 01M44JZWKKWYZG8WNV9JTQFT9V (the outcomes header restates the rows OUTCOMES defines), which the review left unadjudicated, is valid. #30 drops the row list from that header.

> Replying to comment #124268 This is review `01M44JG9B4Q3392GK4XWBNRQK9` of `1798291`. Run 64246 and dispatch run 64281 each lost slots to model capacity, so dispatch run 64381 re-asks the general-bug, test-trimming and restated-sets slots. All five claims are about comments in `src/contract/types.ts` that this PR doesn't change: - `01M44JXG0TSH55KMB30BY85HVH` (the `isFork` comment says the verdict comes before any credential read) is valid: `main.ts` reads the forge token before `readWrapperInputs` sets `isFork`. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/33 changes the comment to say the verdict comes before `reviewCredentials()` is called. - `01M44K0VQRN7Y3M9EDNWXDQRFG` (the inline reconciler promises a thread per finding), `01M44K1Z23FBMBS7ZXYYQEDM2Y` (the `headSha` comment says "as of the event" for dispatch runs) and `01M44JYTM6XFMG5EG6CWK9401J` (the session heading names a `service-session` module that doesn't exist) are valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/42 corrects all three. It targets `main`, because the comments are older than this PR. - `01M44JZWKKWYZG8WNV9JTQFT9V` (the outcomes header restates the rows `OUTCOMES` defines), which the review left unadjudicated, is valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30 drops the row list from that header.

jercik merged commit 389a2b0075 into main 2026-10-05 09:34:46 +00:00

jercik referenced this pull request from a commit 2026-10-05 09:34:46 +00:00

docs: drop the false "never computed" from the inputs header (#40)

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

Subscribe

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

No description provided.