Stop missing GitHub token references in workflow fields - #367
Conversation
|
@buildsworth-bk review |
There was a problem hiding this comment.
This touches credential authority planning, so it needs human review beyond my approval ceiling. I left one non-blocking adapter-semantic note 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 19521, 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.
|
Thanks — for review 4998679704: fixed the adapter-semantic note in 28c2cc5 by enforcing each site’s recorded result type and testing invalid Boolean and numeric controls. |
|
@buildsworth-bk review |
There was a problem hiding this comment.
The earlier adapter-semantic concern is fixed; I found no new issues. This still changes credential authority planning, so it needs human review beyond my approval ceiling.
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 19526, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
Thanks — for review 4998691269: confirmed the result-type fix on head 28c2cc5; no further changes are needed. |
Co-authored-by: Lachlan Donald <lachlan@buildkite.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a01de5-1b16-7159-8756-fd0d777e7069
Amp-Thread-ID: https://ampcode.com/threads/T-01a01de5-1b16-7159-8756-fd0d777e7069 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
28c2cc5 to
a843b83
Compare
There was a problem hiding this comment.
I found no new issues after checking the rebased secret-mapping and token-alias paths. This still changes credential authority planning, so it needs human review beyond my approval ceiling.
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 19938, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Stacked on #365; review this PR after that foundation.
Why
The compiler currently has to remember every place an expression can appear: job environment variables and outputs, step conditions and commands, action inputs, containers, services, and more. It then separately copies those fields into the runtime plan.
For example, both of these references need the same token authority even though they live in different parts of a workflow:
Today, adding another expression-bearing field means updating parsing, token and secret scanning, plan construction, and runtime handling independently. Missing one produces the recurring field-by-field fixes described in the expression authority design.
What
Lower each expanded workflow job into one normalized execution program before resolving actions and authorizing secrets. The program records where each expression came from, which expression rules apply, what type it must produce, and where it runs. It covers job and step conditions, environments, defaults, containers, services, typed controls, commands, action calls, and outputs.
Secret and token planning now walks that program instead of maintaining a separate field list. This also covers less common service fields such as commands and entrypoints; for example, a
github.tokenreference in a service command can no longer be omitted from authority planning.This is slice 2 of the design. It deliberately projects the normalized program back into the existing plan format, so the runtime contract does not change yet. Resolved action metadata comes in the next slice, followed by the final plan-schema cutover.