docs: say the fork gate guards only the review credentials #33
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/fork-gate-boundary"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Corrects the Security model bullet flagged in the review of #32 (claim
01M44DH0VFHYT6MWYWH96G8Z8Y):FORGEJO_TOKENis read before the fork gate on every path, and onlyreviewCredentials()waits for it.Two code comments made the same error and get the same correction: the header of
src/wrapper/event-context.tsand theisForkdoc comment insrc/contract/types.ts.🤖 Generated with Claude Code
Review
01M44FMS560BRRJR823QSZRW2E— head67a520252f78dec2bcfb96460a998604e04b241dReview — j4k-oss/review-wrapper @
209cb1127fScope: diff against base tree
e69943819640Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection
Computed under:
Findings (8)
medium — The action input table duplicates the input declared in action.yml
01M44FWM69QYH21FMRAYX73KZDREADME.md(snippet)01M44G2XZ0AJBP8KMP9MDVW8X9· valid: The exact README table names expected-service-origin as an action input. The reviewer reports that the runner-read action.yml inputs mapping defines that same sole member and its origin-pin behavior. The table is therefore a second editable copy of the input contract, even though the members currently agree. Point to action.yml and retain the caveat in its input description. Historical matching verdicts have no concrete refutation.01M44HX1VSNH8MET4Q0243HBNG· valid: The exact README table lists expected-service-origin and its behavior. The reviewer reports that runner-read action.yml defines that same sole input and description. The second catalog can silently stale even though it agrees today. Point to action.yml for the input contract and retain any distinct security rationale. Historical matches offer no concrete refutation.01M44HVZRQB3CNWKBEZS6Y25JB(writing-quality)medium — The outcome table restates the outcomes and exits defined in code
01M44FXC8EWSST2AF4EADZXNJXREADME.md(snippet)01M44G2XZ0AJBP8KMP9MDVW8X9· valid: The exact README table enumerates outcomes and exit codes. The reviewer lists the same outcome members in OUTCOMES and reports the matching OUTCOME_EXIT mapping in src/contract/types.ts. Superseded is expressly labeled a flag and is not an extra outcome. This handwritten table copies code-defined membership and exits; point to the definitions and preserve distinct recovery guidance without an exhaustive roster. No current mismatch is reported, so medium severity fits.01M44HX1VSNH8MET4Q0243HBNG· valid: The exact README table enumerates outcomes and exits. The reviewer lists the same eleven outcomes in OUTCOMES and the corresponding exits in OUTCOME_EXIT at src/contract/types.ts; superseded is expressly a flag, not an extra outcome. This is a second editable set and exit map. Point to those definitions and preserve distinct operational guidance without an exhaustive roster or row numbering. No current mismatch is established, so medium fits.01M44HSEXSK05FNT7J4KM79MSA(writing-quality)medium — The composition paragraph copies the reconciler count and members
01M44FXXFPF39NW64Q3QVSFQM0README.md(snippet)01M44G2XZ0AJBP8KMP9MDVW8X9· valid: The exact README paragraph counts and names summary, inline, and dispositions as the three reconcilers. The reviewer reports the same members in WrapperDeps in src/contract/types.ts and the wiring in src/main.ts. That is a complete copy of the code-defined collaborator set with no current mismatch. Refer to WrapperDeps while keeping the useful wiring and process-exit explanation. Historical matching verdicts supply no refutation.01M44HX1VSNH8MET4Q0243HBNG· valid: The exact README paragraph counts and names summary, inline, and dispositions as the three reconcilers. The reviewer reports that WrapperDeps in src/contract/types.ts defines precisely those collaborators and src/main.ts supplies them. The count and member list copy a code-defined set that can drift. Refer to WrapperDeps while retaining the wiring and process-exit explanation. No current member mismatch is reported.01M44HTPWS8WS1WGTC6R50VBKQ(writing-quality)medium — The recovery table restates the remedies already emitted in job logs
01M44HT0GJRH67A2T0S35D09N0README.md(snippet)01M44HX1VSNH8MET4Q0243HBNG· valid: The exact README table gives a complete inventory of recovery messages and actions. The reviewer identifies four selected codes plus an unknown fallback in src/wrapper/remedies.ts, and reports that formatExecutionRecovery already emits the corresponding remedies in job logs; the bundled @j4k/review formatter supplies the labels. The copied set can stale even though no difference is reported now. Direct operators to the emitted recovery text and maintainers to the formatter.medium — The environment contract copies the required-variable inventory
01M44HVQDW8RY9BSA3AD87VP0CREADME.md(snippet)01M44HX1VSNH8MET4Q0243HBNG· valid: The exact README passage presents a required-variable inventory. The reviewer lists the requireEnv inputs in readWrapperInputs, forgeCredentials, and reviewCredentials and reports that the README covers those names, using GITHUB_EVENT_* shorthand, with no established missing member. It is a separately maintained copy that can silently stale. Point to the source functions for membership, while keeping the fork-path requirement and immediate-failure rule as non-inventory guidance. Medium fits absent a proved current mismatch.medium — The WrapperInputs header falsely says its fields are never computed
01M44HRFXHK0QP5RCK8NKVCP1Jsrc/contract/types.ts(snippet)01M44HX1VSNH8MET4Q0243HBNG· valid: The exact header claims the inputs are never computed. The reviewer reports that readWrapperInputs computes isFork, normalizes the forge host, parses the attempt, and constructs reTriageAskKey. These concrete operations contradict the blanket wording and could mislead a maintainer about the contract. Remove the false phrase while retaining a useful contract pointer; the unavailable external section does not undo the observed conflict in the subject code.medium — Input derivation comment duplicates the two PR metadata sources
01M44FWJB8RYQ4ZV29426J4CPNsrc/wrapper/event-context.ts(snippet)01M44G2XZ0AJBP8KMP9MDVW8X9· valid: The exact comment presents event payload and forge as the complete PR metadata source set. The reviewer identifies the two readWrapperInputs branches in src/wrapper/event-context.ts, with readEventPayload and forge.getPull defining those same members. This adjacent comment copies the code-defined set and can drift; delete its source-list sentence. No current member mismatch is reported, so medium severity fits.01M44HX1VSNH8MET4Q0243HBNG· valid: The exact adjacent comment says PR metadata comes from the event payload or forge. The reviewer identifies readEventPayload and forge.getPull branches in src/wrapper/event-context.ts as the code-defined two-source set, with no current difference. This comment restates that set and can drift; delete the source-list sentence while keeping distinct intent. Medium severity fits.low — README security paragraph duplicates internal credential call order and test coverage
01M44FWA4NTNW284GH2SQT810MREADME.md(snippet)01M44G2XZ0AJBP8KMP9MDVW8X9· valid: The exact README excerpt documents internal credential call order and two test checks. The reviewer reports that this passage was changed in the diff and traces forgeCredentials, readWrapperInputs, the fork return, and the deferred openSession call. Those details are implementation and test inventory in a human onboarding README; the project-docs rule puts how it works in code. Keep the user-facing fork guarantee as a short security summary. Low severity fits a wrong-home finding.01M44HX1VSNH8MET4Q0243HBNG· valid: The exact README excerpt narrates credential helper order and names two tests. The reviewer reports this passage was edited and traces forgeCredentials before readWrapperInputs, then the fork return before the deferred reviewCredentials call. That is internal implementation and test inventory in a human-onboarding README; the project-docs skill assigns how-it-works detail to code and tests. Keep the user-facing fork credential guarantee as a short summary. Low severity fits the wrong-home finding.01M44HVDF2E0TB2KVFQ1YWN547(project-docs)Other claims
01M44HSEXSK05FNT7J4KM79MSAmedium — The outcomes table duplicates the code-defined outcome set →01M44FXC8EWSST2AF4EADZXNJX01M44HTPWS8WS1WGTC6R50VBKQmedium — The composition paragraph copies the reconciler set from main.ts →01M44FXXFPF39NW64Q3QVSFQM001M44HVDF2E0TB2KVFQ1YWN547low — README security model duplicates the internal credential call sequence →01M44FWA4NTNW284GH2SQT810M01M44HVZRQB3CNWKBEZS6Y25JBmedium — The Action inputs table repeats action.yml →01M44FWM69QYH21FMRAYX73KZDCoverage
Coverage pass: 01M44HJR3QV5JN9PKQ4J2X163F
Accounting: complete
Slot health: healthy
@ -50,3 +49,2 @@credentials module is ever called. On the fork path the capability token environmentvariable is never read — enforced by a static isolation test and an orchestrator unittest.code-path ordering: the fork gate is evaluated from the event payload **before**`reviewCredentials()` is called. The forge task token (`FORGEJO_TOKEN`) is read beforemedium — Security paragraph attributes every fork decision to the event payload
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M44F37NYJZ9G995T77RXWWW2of review01M44EVAMWZ9QXBSWC339418SGFixed in
67a5202. The bullet now saysreadWrapperInputsdecides whether the pull request is a fork beforereviewCredentials()is called, without claiming the verdict always comes from the payload. Onworkflow_dispatchit comes from the fetched pull request.@ -52,1 +49,4 @@test.code-path ordering: the fork gate is evaluated from the event payload **before**`reviewCredentials()` is called. The forge task token (`FORGEJO_TOKEN`) is read beforethe gate, on every path. On the fork path `REVIEW_SERVICE_URL` and`REVIEW_CAPABILITY_TOKEN` are never read: a static isolation test restricts themedium — Fork security paragraph copies the review credential variable set
lens
restated-sets· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M44F3SBF4RFT93KDQPMT68T4of review01M44EVAMWZ9QXBSWC339418SGFixed in
67a5202. The bullet now says a fork run returns at the gate, so nothingreviewCredentials()reads is read on that path. It no longer lists the variables.@ -53,0 +50,4 @@`reviewCredentials()` is called. The forge task token (`FORGEJO_TOKEN`) is read beforethe gate, on every path. On the fork path `REVIEW_SERVICE_URL` and`REVIEW_CAPABILITY_TOKEN` are never read: a static isolation test restricts thecredentials module's importers to `src/main.ts`, and an orchestrator unit test checksmedium — Security model copies the credentials module importer set
lens
restated-sets· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M44F2NHWHVC7556RHN72P4AYof review01M44EVAMWZ9QXBSWC339418SGFixed in
67a5202. The bullet now points at the isolation test insrc/credentials.test.tsfor which modules may import the credentials module, instead of naming the importer.@ -2,7 +2,7 @@// the event payload or fetched from the forge — nothing here is computed: no// merge base, no digest, no scope canonicalization, and the PR title and body// are never read. The fork verdict is derived here so the orchestrator can// evaluate it before any credential module is ever called.// evaluate it before reviewCredentials() is ever called.low — Input comment says nothing is computed despite deriving wrapper fields
lens
writing-quality· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M44F97BWXVM7TQVQCNGYMPX6of review01M44EVAMWZ9QXBSWC339418SGFixed in
67a5202. The header now says PR metadata is read from the payload or fetched from the forge, and that merge base, digest and scope canonicalization are left to the service. The absolute "nothing here is computed" is gone.This is review
01M44EVAMWZ9QXBSWC339418SGofa4e7bd0. Its four inline findings are fixed in67a5202and answered in their threads. The four summary-only findings are about README text this PR doesn't change, and each is already handled by the PR that owns that text:01M44F667G03T6M1T1XWP37C32("Environment contract overstates the required event variables") is valid. #32 replaces that list, wildcard included, with a pointer toforgeCredentials,readWrapperInputsandsrc/main.ts.01M44F4QH9RYXHZQHM1XGBBJVK("Outcome table restates the code-defined outcome set and exit mapping") is valid. #30 replaces the table with a pointer toOUTCOMESandOUTCOME_EXIT.01M44F72T06SY944TY7EP2AVCV("Composition paragraph restates the reconciler set") is valid. #32 points atWrapperDepsinstead.01M44F84QVWWW9MGXBWTR4ASHS("Recovery table restates the selected execution failure remedies") is valid. #34 replaces the table with the remedy printed on the failure log line.@ -49,4 +49,6 @@code-path ordering: the fork gate is evaluated from the event payload **before** thecredentials module is ever called. On the fork path the capability token environmentvariable is never read — enforced by a static isolation test and an orchestrator unittest.code-path ordering: `readWrapperInputs` decides whether the pull request is a fork**before** `reviewCredentials()` is called. `forgeCredentials()`, which reads the forgetask token, runs before the gate on every path. A fork run returns at the gate, sonothing `reviewCredentials()` reads is read on that path. The isolation test in`src/credentials.test.ts` checks which modules import the credentials module, and anorchestrator unit test checks that a fork run never opens a review session.low — README security paragraph duplicates internal credential call order and test coverage
lens
project-docs· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M44FWA4NTNW284GH2SQT810Mof review01M44FMS560BRRJR823QSZRW2E@ -3,3 +1,2 @@// merge base, no digest, no scope canonicalization, and the PR title and body// are never read. The fork verdict is derived here so the orchestrator can// evaluate it before any credential module is ever called.// Input derivation (roadmap 1.2, contract §2). PR metadata is read from the// event payload or fetched from the forge. Merge base, digest and scopemedium — Input derivation comment duplicates the two PR metadata sources
lens
restated-sets· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M44FWJB8RYQ4ZV29426J4CPNof review01M44FMS560BRRJR823QSZRW2EValid. The two branches of
readWrapperInputsright below the comment already show where the PR metadata comes from, so the sentence is a second copy. This PR is in its second review round and the sentence doesn't make its fork-gate correction wrong, so #39, stacked on this PR, deletes it.Valid. The README needs the guarantee, not the call order or the names of the tests that check it, and the Composition section already shows where the gate sits in code. This PR is in its second review round and the extra detail is accurate, so #39, stacked on this PR, cuts the bullet to the guarantee: a fork run returns at the fork gate without opening a review session, so it never reads
REVIEW_CAPABILITY_TOKEN.This is review
01M44FMS560BRRJR823QSZRW2Eof67a5202. Dispatch run 64134 completed it after run 63897 lost its writing-quality slot to model capacity. The two findings with threads are answered there. The other six are summary-only, about text this PR doesn't change:01M44FWM69QYH21FMRAYX73KZD(action-input table) is valid. #34 points the README atinputsinaction.yml.01M44HT0GJRH67A2T0S35D09N0(recovery table) is valid. #34 replaces the table with the remedy printed on the failure log line.01M44FXC8EWSST2AF4EADZXNJX(outcome table) is valid. #30 replaces the table with a pointer toOUTCOMESandOUTCOME_EXIT.01M44FXXFPF39NW64Q3QVSFQM0(composition paragraph) is valid. #32 points atWrapperDeps.01M44HRFXHK0QP5RCK8NKVCP1J(the inputs header says "never computed") is valid:readWrapperInputscomputesisForkandreTriageAskKeyand lowercases the forge host. Tracked in #40, which drops the phrase. It targetsmain, because the header is older than this PR.01M44HVQDW8RY9BSA3AD87VP0C(the environment contract copies the required variables) is split. #32 replaces the variable list in the sentence after the table with pointers to the functions that read them. Replacing the table itself is held, as01M44DH5HKQFA3T9JJSGAEFP8Mwas on #30. The 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. Replacing it changes what the README promises those workflows, so it waits for that decision.The four duplicate claims,
01M44HSEXSK05FNT7J4KM79MSA,01M44HTPWS8WS1WGTC6R50VBKQ,01M44HVDF2E0TB2KVFQ1YWN547and01M44HVZRQB3CNWKBEZS6Y25JB, are covered by the findings they duplicate.The half of
01M44HVQDW8RY9BSA3AD87VP0Cheld 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, becausesrc/credentials.ts,src/wrapper/event-context.tsandsrc/main.tseach read part of the set.The other half stands. #32 replaces the variable list in the sentence after the table with pointers.