Skip to content

Stop missing GitHub token references in workflow fields - #367

Open
lox wants to merge 2 commits into
expression/abstract-authorityfrom
expression/workflow-program
Open

Stop missing GitHub token references in workflow fields#367
lox wants to merge 2 commits into
expression/abstract-authorityfrom
expression/workflow-program

Conversation

@lox

@lox lox commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

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:

env:
  API_TOKEN: ${{ github.token }}

steps:
  - uses: owner/action@v1
    with:
      token: ${{ github.token }}

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.token reference 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.

lox commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

@buildsworth-bk review

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread internal/program/evaluate.go Outdated

lox commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

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.

lox commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

@buildsworth-bk review

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lox commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — for review 4998691269: confirmed the result-type fix on head 28c2cc5; no further changes are needed.

ampagent and others added 2 commits August 24, 2026 02:52
@lox
lox force-pushed the expression/workflow-program branch from 28c2cc5 to a843b83 Compare August 24, 2026 03:05

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lox lox changed the title Plan every workflow expression from one model Stop missing GitHub token references in workflow fields Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants