Keep nested reusable workflow details together during compilation - #349
Keep nested reusable workflow details together during compilation#349lox wants to merge 1 commit into
Conversation
|
@lox @wolfeidau: please provide explicit sign-off on this ownership refactor once you are satisfied with the design and preserved nested reusable-workflow semantics. @wolfeidau can approve via review; because @lox authored the PR, GitHub requires @lox to sign off explicitly in a comment. |
There was a problem hiding this comment.
The secret-inheritance and GITHUB_TOKEN permission authority paths make this a high-impact ownership refactor that needs human review. I found no specific issue to add.
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 18737, 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.
|
Acknowledged Buildsworth review 4980516167: it classified the authority paths as requiring human review and reported no specific issue. Human sign-off remains requested from @lox and @wolfeidau. |
|
Acknowledged for review 4980516167: no specific issue was reported; the requested human authority review remains pending. |
Amp-Thread-ID: https://ampcode.com/threads/T-01a01e23-fc89-77cf-bcf4-1a26ffbf5835 Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
45b5b8b to
a93c6f8
Compare
|
@lox @wolfeidau: this PR is now rebased onto current |
There was a problem hiding this comment.
I found no specific issue in the current rebased head. The secret-inheritance and GITHUB_TOKEN permission authority paths still need human review.
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 19611, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Why
The compiler replaces each reusable-workflow call with the jobs inside that workflow. Those jobs must keep several details from every caller: where the job came from, which secrets and token permissions it may use, inputs that depend on an earlier job, required jobs and outputs, and conditions attached to each call.
Previously, recursive workflow loading, job expansion, and plan creation each carried and rebuilt different parts of that context. A change in one stage could preserve a nested job but accidentally lose its caller's condition, secret mapping, or dependency in another stage.
For example:
If
release.ymlcalls another workflow, every resulting job still needs the original condition, deferredversioninput, dependency onbuild, source workflow, andrelease_tokenmapping. Keeping those details together makes that rule explicit and reduces the number of places a future reusable-workflow change must update.What
Add one private reusable-job component that owns this context from recursive workflow loading until each concrete job plan is built. It now:
The graph expander and plan builder now coordinate these steps instead of independently interpreting reusable-workflow calls. The compiler's JSON and serialized plan formats remain unchanged.
An end-to-end nested-workflow test verifies source paths, secret and token permissions, deferred outputs, status-only dependencies, call conditions, and plan encoding together. Existing explicit secret-forwarding tests also cover the behavior added to
mainwhile this branch was open.