Allow composite shell steps to use github.token - #388
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-01a031e9-19a0-706f-bc67-b74f65d4d427 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The direct composite-shell case is covered, but known input and provider guards still grant token authority in several runtime-unreachable paths. Details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20210, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only when repository policy or a verified grant allows it; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
Amp-Thread-ID: https://ampcode.com/threads/T-01a031e9-19a0-706f-bc67-b74f65d4d427 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
|
@buildsworth-bk review |
There was a problem hiding this comment.
Three previous concerns are fixed. The condition-reachability thread still has a known-false status-function path, so this remains blocked.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20223, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Amp-Thread-ID: https://ampcode.com/threads/T-01a031e9-19a0-706f-bc67-b74f65d4d427 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
|
Fixed review 5005591975 in 9b7bb29: status functions now remain abstract condition values, so a later known-false input or provider guard can still prove the composite step unreachable. |
Previous concerns have been addressed.
There was a problem hiding this comment.
The status-function reachability concern is fixed. Two other known-false paths still acquire token authority; details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20229, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Amp-Thread-ID: https://ampcode.com/threads/T-01a031e9-19a0-706f-bc67-b74f65d4d427 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
|
@buildsworth-bk review |
Previous concerns have been addressed.
There was a problem hiding this comment.
The two previous concerns are fixed. Three other known-false paths still acquire token authority; details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20246, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Amp-Thread-ID: https://ampcode.com/threads/T-01a031e9-19a0-706f-bc67-b74f65d4d427 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
|
@buildsworth-bk review |
Previous concerns have been addressed.
There was a problem hiding this comment.
The three previous concerns are fixed. Two new token-planning paths remain incorrect: nested with evaluation can omit required authority, and concrete matrix guards can reject tokenless jobs. Details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20778, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Amp-Thread-ID: https://ampcode.com/threads/T-01a031e9-19a0-706f-bc67-b74f65d4d427 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
|
@buildsworth-bk review |
Previous concerns have been addressed.
There was a problem hiding this comment.
Both previous concerns are fixed. Two root-invocation paths remain incorrect: sibling with values can be evaluated in the action scope, and concrete matrix inputs are not supplied to action analysis. Details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20799, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| "job.check_run_id": "", | ||
| } | ||
| if callerReferences == nil { | ||
| callerReferences = known |
There was a problem hiding this comment.
Blocking: For a root workflow invocation, this aliases the caller references to known, which the loop then mutates with each action input. Runtime evaluates every with template in the workflow caller scope. If caller inputs.a is true and the action receives a: false plus b: ${{ inputs.a }}, planning resolves b to false from the sibling action input and can omit a github.token branch that runtime reaches. Keep the root caller map separate; unavailable workflow inputs should stay unknown.
| for i, stepIndex := range actionIndexes { | ||
| reachable[i] = compositeStepMayRun(steps[stepIndex].Condition, knownReferences) | ||
| } | ||
| compiled, err := compileReachableActionInvocations(b.ctx, instance.RepositoryRoot, b.actionSource, plan.EventServerURL(b.ir.Event.Provider), actionRefs, actionInputs, reachable) |
There was a problem hiding this comment.
Blocking: The concrete matrix values collected above only classify the workflow step's if; they are not passed into root action input analysis here. For matrix.enabled: [false], with: {enabled: '${{ matrix.enabled }}'}, permissions: {}, and a composite shell token branch guarded by inputs.enabled, planning treats the input as unknown and rejects the tokenless job, while runtime resolves it to false. Pass the immutable matrix caller references into the root invocation analysis.
2 participants
Why
Composite actions such as
taiki-e/install-action@v2can put a guarded token in a shell-step environment:The compiler did not inspect that resolved metadata, so it produced a tokenless job and runtime evaluation failed.
What
Plan token authority for reachable composite shell-step
env,run, andworking-directorytemplates using the shared expression semantics. Known input and provider guards stay tokenless; runtime-dependent branches remain conservative.Nested action inputs and JavaScript or Docker lifecycle boundaries do not change. This also removes the obsolete action-program and plan-schema cutover plan after moving the lasting compatibility boundary into the product documentation.