Skip to content

Support runtime GitHub event payload access - #366

Open
lox wants to merge 4 commits into
mainfrom
support-retained-github-event
Open

Support runtime GitHub event payload access#366
lox wants to merge 4 commits into
mainfrom
support-retained-github-event

Conversation

@lox

@lox lox commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Why

Whole or runtime-selected github.event access fails during compilation because job plans keep only event identity and a payload digest. Buildkite retains the upstream payload only briefly, so runtime jobs and retries cannot recover it later. Improving the error would preserve a compatibility gap that GitHub Actions users do not expect.

Supersedes #362.

What

Keep reducing ordinary scalar github.event.* values at compile time. When a job still needs whole, projected, or dynamically indexed event access, retain the immutable payload only in that job plan and expose it through the runtime GitHub context. Verify retained payloads against the existing digest and limit them to 25 MiB.

Keep action references static and keep issued tokens, resolved secrets, credentials, and internal admission metadata outside the event snapshot. Document the plan and build retention boundary in the security and compatibility guides.

Amp-Thread-ID: https://ampcode.com/threads/T-01a026c2-4076-71ed-b812-148e0a5c338a
Co-authored-by: Lachlan Donald <lachlan@buildkite.com>

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.

Action metadata still has two end-to-end gaps: composite expressions don't request the payload, and dynamic or projected input defaults remain rejected. I also noted two retention and validation edge cases 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 19518, 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/compiler/actions.go Outdated
Comment thread internal/compiler/actions.go
Comment thread internal/compiler/actions.go
Comment thread internal/expression/condition.go Outdated
Amp-Thread-ID: https://ampcode.com/threads/T-01a026c2-4076-71ed-b812-148e0a5c338a
Co-authored-by: Lachlan Donald <lachlan@buildkite.com>

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 retention-boundary, metadata-scan, and index-validation fixes are in place. Action defaults still stop in token-reference analysis, while the two new inline comments cover condition evaluation and whole-context serialization.

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 19524, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

Comment thread internal/compiler/plan_builder.go
Comment thread internal/expression/condition.go
Amp-Thread-ID: https://ampcode.com/threads/T-01a026c2-4076-71ed-b812-148e0a5c338a
Co-authored-by: Lachlan Donald <lachlan@buildkite.com>

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 condition-evaluation and action-default fixes are in place. Whole-context retention is still incomplete for composed expressions; the existing thread has the remaining case.

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 19531, 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

@buildsworth-bk review

@buildsworth-bk-app
buildsworth-bk-app Bot dismissed their stale review August 22, 2026 02:46

Previous concerns have been addressed.

@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 composed whole-context retention concern is fixed; the scan now finds nested toJSON(github) calls and the bundle test covers the reported case. I’m leaving this comment-only because retaining event payloads changes the immutable compiler/runtime plan data boundary.

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 19535, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

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