docs: reduce the fork-gate bullet to its guarantee #39

jercik · 2026-10-04T23:03:03Z

jercik commented

2026-10-04 23:03:03 +00:00

Follow-up to #33, from review 01M44FMS560BRRJR823QSZRW2E (threads #123531 and #123532).

The Security model bullet about fork runs now states only the guarantee: a fork run returns at the fork gate without opening a review session, so it never reads REVIEW_CAPABILITY_TOKEN. The call order and the names of the tests that check it are gone. The Composition section already shows where the gate sits in code.

The opening comment of src/wrapper/event-context.ts drops the sentence that listed where PR metadata comes from. The two branches of readWrapperInputs right below it show that.

Stacked on #33, because both edits change text that #33 writes.

🤖 Generated with Claude Code

Follow-up to #33, from review `01M44FMS560BRRJR823QSZRW2E` (threads #123531 and #123532). The Security model bullet about fork runs now states only the guarantee: a fork run returns at the fork gate without opening a review session, so it never reads `REVIEW_CAPABILITY_TOKEN`. The call order and the names of the tests that check it are gone. The Composition section already shows where the gate sits in code. The opening comment of `src/wrapper/event-context.ts` drops the sentence that listed where PR metadata comes from. The two branches of `readWrapperInputs` right below it show that. Stacked on #33, because both edits change text that #33 writes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-04 23:03:04 +00:00

docs: reduce the fork-gate bullet to its guarantee

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

Details

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

Details

Review / Review (pull_request_target) Has been cancelled

Details

0886930d8d

The bullet keeps the guarantee and drops the call order and test inventory. The
event-context header drops the sentence that restated the two input sources.

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

jercik referenced this pull request

2026-10-04 23:03:47 +00:00

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

jercik referenced this pull request

2026-10-04 23:03:47 +00:00

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

forgejo-actions commented

2026-10-04 23:05:39 +00:00

Review 01M45PJYX9YGQXV07DPAN0YRPC — head f5990dc2ae36a040605ce6d11f43067029ec73b5

Review — j4k-oss/review-wrapper @ `41e47cf925`

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

No findings survived.

Reviewed:

general-bug (whole/default): unit-failed
writing-quality (whole/default): unit-failed
test-trimming (whole/default): unit-failed
restated-sets (whole/default): unit-failed
project-docs (whole/default): unit-failed

Other claims

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

Coverage

Coverage pass: 01M45PJYYT9EH7S95WXAP96D0S
Accounting: complete
Slot health: general-bug (whole/default): unit-failed; writing-quality (whole/default): unit-failed; test-trimming (whole/default): unit-failed; restated-sets (whole/default): unit-failed; project-docs (whole/default): unit-failed

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

general-bug run 01M45PJYYVV568373F4GPDEAVD: sandbox infrastructure failed.
writing-quality run 01M45PJYYW6QRYKBWA6MBHTJZ6: sandbox infrastructure failed.
test-trimming run 01M45PJYYYQDEJ51DP0Z3M1VP3: sandbox infrastructure failed.
restated-sets run 01M45PJYZ00Z0RD5HHQ82J9410: sandbox infrastructure failed.
project-docs run 01M45PJYZ1NF93BWX6FNVCXTMD: sandbox infrastructure failed.

**Review** `01M45PJYX9YGQXV07DPAN0YRPC` — head `f5990dc2ae36a040605ce6d11f43067029ec73b5` # Review — j4k-oss/review-wrapper @ 41e47cf925f9 Scope: diff against base tree `d4e1d10f7991` 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 (0) No findings survived. Reviewed: - general-bug (whole/default): unit-failed - writing-quality (whole/default): unit-failed - test-trimming (whole/default): unit-failed - restated-sets (whole/default): unit-failed - project-docs (whole/default): unit-failed ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M45PJYYT9EH7S95WXAP96D0S Accounting: complete Slot health: general-bug (whole/default): unit-failed; writing-quality (whole/default): unit-failed; test-trimming (whole/default): unit-failed; restated-sets (whole/default): unit-failed; project-docs (whole/default): unit-failed | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | unit-failed | 1 | no | | writing-quality | whole | default | unit-failed | 1 | no | | test-trimming | whole | default | unit-failed | 1 | no | | restated-sets | whole | default | unit-failed | 1 | no | | project-docs | whole | default | unit-failed | 1 | no | - general-bug run 01M45PJYYVV568373F4GPDEAVD: sandbox infrastructure failed. - writing-quality run 01M45PJYYW6QRYKBWA6MBHTJZ6: sandbox infrastructure failed. - test-trimming run 01M45PJYYYQDEJ51DP0Z3M1VP3: sandbox infrastructure failed. - restated-sets run 01M45PJYZ00Z0RD5HHQ82J9410: sandbox infrastructure failed. - project-docs run 01M45PJYZ1NF93BWX6FNVCXTMD: sandbox infrastructure failed.

🚀 1

forgejo-actions reviewed

2026-10-04 23:25:24 +00:00

README.md

Lines 45-46

					
				@ -55,2 +48,5 @@

				- Forgejo does pass secrets to fork `pull_request_target` runs, so the protection is in

				  the code. A fork run returns at the fork gate without opening a review session, so it

				  never reads `REVIEW_CAPABILITY_TOKEN`.

				- The capability token is read in exactly one module with zero internal imports

				  (`src/credentials.ts`) and flows only into the `Authorization` header of requests to

medium — Credential security bullet restates module and import counts

The security bullet counts the modules that read the capability token and the internal imports of that module. Those counts can silently drift as code changes, weakening a maintainer's understanding of the fork boundary even while the README still sounds definitive.

In the subject tree, src/credentials.ts is the one module with a REVIEW_CAPABILITY_TOKEN environment read, and it has no imports from wrapper modules. The README names that same module and gives counts of one and zero; both counts currently agree with the source. src/credentials.test.ts checks the import boundary, but that test does not make the README copy authoritative.

Point to reviewCredentials in src/credentials.ts and its import-boundary test without counting files or imports; retain the explanation that the token reaches the review service only after URL validation and origin pinning. This preserves the security reasoning while locating its mutable facts in code.

I read README.md, src/credentials.ts, src/credentials.test.ts, and the call path through src/main.ts and src/wrapper/orchestrator.ts; this is a static trace. A generated or enforced link from the source to this README sentence would refute the drift risk, but I found none. The writing skill's One Idea, One Place guidance supports a source pointer here.

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

**medium** — Credential security bullet restates module and import counts > The security bullet counts the modules that read the capability token and the internal imports of that module. Those counts can silently drift as code changes, weakening a maintainer's understanding of the fork boundary even while the README still sounds definitive. > > In the subject tree, `src/credentials.ts` is the one module with a `REVIEW_CAPABILITY_TOKEN` environment read, and it has no imports from wrapper modules. The README names that same module and gives counts of one and zero; both counts currently agree with the source. `src/credentials.test.ts` checks the import boundary, but that test does not make the README copy authoritative. > > Point to `reviewCredentials` in `src/credentials.ts` and its import-boundary test without counting files or imports; retain the explanation that the token reaches the review service only after URL validation and origin pinning. This preserves the security reasoning while locating its mutable facts in code. > > I read `README.md`, `src/credentials.ts`, `src/credentials.test.ts`, and the call path through `src/main.ts` and `src/wrapper/orchestrator.ts`; this is a static trace. A generated or enforced link from the source to this README sentence would refute the drift risk, but I found none. The writing skill's One Idea, One Place guidance supports a source pointer here. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44KEB4AFFE7VZQYK6JQPPHX` of review `01M44JG8KYBFS4CPN0YDC2YFZ3`

👎 1

@jercik I'm holding this one for your decision rather than applying it. The bullet states the token's isolation as a guarantee of the security model, and only half of it is enforced:

"Zero internal imports" can't drift silently. src/credentials.test.ts fails when src/credentials.ts imports anything but node: builtins.
"Exactly one module" has no test. src/credentials.ts is the only module that reads REVIEW_CAPABILITY_TOKEN today, but nothing fails if a second one starts reading it.

The finding would replace both counts with a pointer to reviewCredentials and its test. That changes what the security section promises a reader, the same question held on #30 for 01M44DH5HKQFA3T9JJSGAEFP8M. The other option keeps the guarantee and adds a test that fails when any other module reads REVIEW_CAPABILITY_TOKEN. The bullet is older than this PR, so either change goes in a follow-up.

@jercik I'm holding this one for your decision rather than applying it. The bullet states the token's isolation as a guarantee of the security model, and only half of it is enforced: - "Zero internal imports" can't drift silently. `src/credentials.test.ts` fails when `src/credentials.ts` imports anything but `node:` builtins. - "Exactly one module" has no test. `src/credentials.ts` is the only module that reads `REVIEW_CAPABILITY_TOKEN` today, but nothing fails if a second one starts reading it. The finding would replace both counts with a pointer to `reviewCredentials` and its test. That changes what the security section promises a reader, the same question held on #30 for `01M44DH5HKQFA3T9JJSGAEFP8M`. The other option keeps the guarantee and adds a test that fails when any other module reads `REVIEW_CAPABILITY_TOKEN`. The bullet is older than this PR, so either change goes in a follow-up.

I disagree with the pointer, and that closes my question above: the bullet keeps both counts.

The counts are the guarantee: one module reads REVIEW_CAPABILITY_TOKEN, and that module imports nothing internal. A pointer to reviewCredentials would drop what a reader of a security model needs to know. The gap is on the other side. Only the import count is tested, in src/credentials.test.ts. A follow-up stacked on this PR will add a test that fails when a second module reads the variable, and I'll link it here once it is open.

I disagree with the pointer, and that closes my question above: the bullet keeps both counts. The counts are the guarantee: one module reads `REVIEW_CAPABILITY_TOKEN`, and that module imports nothing internal. A pointer to `reviewCredentials` would drop what a reader of a security model needs to know. The gap is on the other side. Only the import count is tested, in `src/credentials.test.ts`. A follow-up stacked on this PR will add a test that fails when a second module reads the variable, and I'll link it here once it is open.

jercik marked this conversation as resolved

jercik commented

2026-10-04 23:28:06 +00:00

Replying to comment #124269

This is review 01M44JG8KYBFS4CPN0YDC2YFZ3 of 0886930. Dispatch run 64311 completed it after run 64243 lost slots to model capacity. The finding with a thread, 01M44KEB4AFFE7VZQYK6JQPPHX (the security bullet counts the modules that read the token), is held there for the owner's decision. The other five are summary-only, about README text this PR doesn't change:

01M44K7G6K3CZ27JABPC2M1Y5W (outcome table) is valid. #30 replaces the table with a pointer to OUTCOMES and OUTCOME_EXIT.
01M44K83DDMGM1PZA8EQ5A0NPR (action-input table) and 01M44KBHQWPBQGA64ZY8W03TAR (recovery table) are valid. #34 points the README at inputs in action.yml and at the remedy printed on the failure log line.
01M44KA0HM55EGQ3WJXEKQSJKX (required-variable list) and 01M44KC8JMJK26V9WMHQTK086D (composition paragraph) are valid. #32 replaces the list with pointers to forgeCredentials, readWrapperInputs and src/main.ts, and the reconcilers with a pointer to WrapperDeps.

> Replying to comment #124269 This is review `01M44JG8KYBFS4CPN0YDC2YFZ3` of `0886930`. Dispatch run 64311 completed it after run 64243 lost slots to model capacity. The finding with a thread, `01M44KEB4AFFE7VZQYK6JQPPHX` (the security bullet counts the modules that read the token), is held there for the owner's decision. The other five are summary-only, about README text this PR doesn't change: - `01M44K7G6K3CZ27JABPC2M1Y5W` (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`. - `01M44K83DDMGM1PZA8EQ5A0NPR` (action-input table) and `01M44KBHQWPBQGA64ZY8W03TAR` (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. - `01M44KA0HM55EGQ3WJXEKQSJKX` (required-variable list) and `01M44KC8JMJK26V9WMHQTK086D` (composition paragraph) are valid. https://code.j4k.dev/j4k-oss/review-wrapper/pulls/32 replaces the list with pointers to `forgeCredentials`, `readWrapperInputs` and `src/main.ts`, and the reconcilers with a pointer to `WrapperDeps`.

jercik referenced this pull request

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

test: fail when a second module reads the capability token #44

jercik commented

2026-10-05 05:10:26 +00:00

Replying to review comment #124568

The test is in #44, stacked on this PR. It fails when any non-test module under src/ other than src/credentials.ts reads REVIEW_CAPABILITY_TOKEN by name. It can't see a name built at run time.

> Replying to review comment #124568 The test is in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/44, stacked on this PR. It fails when any non-test module under `src/` other than `src/credentials.ts` reads `REVIEW_CAPABILITY_TOKEN` by name. It can't see a name built at run time.

jercik changed target branch from docs/fork-gate-boundary to main

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

jercik force-pushed docs/fork-gate-bullet-guarantee from 0886930d8d

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

Details

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

Details

Review / Review (pull_request_target) Has been cancelled

Details

to f5990dc2ae

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

Details

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

Details

Review / Review (pull_request_target) Failing after 2m20s

Details

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

Compare

jercik merged commit bd34f8bba0 into main

2026-10-05 09:34:43 +00:00

jercik referenced this pull request from a commit

2026-10-05 09:34:43 +00:00

docs: reduce the fork-gate bullet to its guarantee (#39)

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

Rows
Columns