Skip to content

PB-3005: Run source-checked local actions from public reusable workflows - #330

Open
zhming0 wants to merge 16 commits into
mainfrom
pb-3005-source-checked-local-actions
Open

PB-3005: Run source-checked local actions from public reusable workflows#330
zhming0 wants to merge 16 commits into
mainfrom
pb-3005-source-checked-local-actions

Conversation

@zhming0

@zhming0 zhming0 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Why

GitHub resolves ./... actions inside remote reusable workflows against the caller job workspace. SLSA v2.1.0 checks its pinned builder repository out to the literal __BUILDER_CHECKOUT_DIR__ path before invoking local actions there, but eager composite traversal tried to read that future path from the caller checkout and failed.

Resolves PB-3005.

What

  • Recognize source-checked local actions by their general workspace alias and repository-relative path, and bind their locks to the remote workflow's repository, exact commit, resolved branch or tag namespace, and tree digest.
  • Rewrite the matching public checkout to the pinned commit, require the populated workspace action to match immutable source, and execute the verified source-store copy.
  • Add hermetic SLSA-shaped coverage for deferred needs inputs, nested composites, checkout, top-level and nested local actions, unavailable paths, tampering, and branch/tag ambiguity.
  • Document caller-workspace resolution and the immutable source boundary.

@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 inline comments are non-blocking. This changes the immutable-source execution and checkout credential boundary, so it still needs human approval.

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 18197, 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/runtime/checkout.go
Comment thread docs/security.md Outdated

zhming0 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed Buildsworth review 4970335564 in f82c129: source checkouts now reject case-insensitive duplicate inputs before lookup, and the security documentation includes the anonymous provenance-bound checkout boundary.

zhming0 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in f82c129 for review 4970335564: reject case-insensitive duplicate source-checkout inputs before lookup and document the anonymous provenance-bound checkout boundary.

@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.

One blocking case remains: a checkout authored directly in the remote reusable-workflow job still fails compiler adapter validation. Details 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 18201, then answer my questions about the findings.

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

Comment thread internal/compiler/plan_builder.go

zhming0 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 596f8db for review 4970395103: compiler adapter validation now admits provenance-bound direct source checkouts and hermetic coverage exercises direct checkout → local action.

@buildsworth-bk-app
buildsworth-bk-app Bot dismissed their stale review August 19, 2026 09:13

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 previous blocking checkout-validation concern is resolved. I left one non-blocking documentation note. This still changes the immutable-source execution and checkout credential boundary, so it remains above the L2 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 18202, then answer my questions about the findings.

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

Comment thread docs/compatibility.md Outdated

zhming0 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed Buildsworth review 4970490842 in 700c1c1: clarified the provenance-bound checkout exception in the checkout contract, replied inline, and resolved the thread.

@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.

One blocking provenance case remains: bare remote-workflow refs currently make branch and tag checkouts indistinguishable. Details 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 18203, then answer my questions about the findings.

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

Comment thread internal/compiler/plan_builder.go Outdated

@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 previous namespace-provenance concern is still open because shared repository memoization can drop the resolved namespace. The existing thread has the order-dependent failure.

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

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

@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 memoized namespace propagation is fixed, but the existing provenance thread remains open because a bound alias can still fall through to event-checkout semantics.

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

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

@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 bound-alias provenance concern remains open on case-insensitive macOS workspaces; details are in the existing thread.

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

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

@buildsworth-bk-app
buildsworth-bk-app Bot dismissed their stale review August 19, 2026 10:12

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 previous case-only alias issue is fixed. Two blocking alias-classification cases remain; details 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 18209, then answer my questions about the findings.

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

Comment thread internal/compiler/plan_builder.go Outdated
Comment thread internal/runtime/checkout.go Outdated

@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 empty-alias case is fixed. The normalization concern remains open at runtime for a non-ASCII checkout spelling that aliases an ASCII source path; details are in the existing thread. This still changes immutable-source execution and checkout credential boundaries, so it needs human approval.

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

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

@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 runtime alias-normalization concern remains open in the existing thread; I found no new issues in this pass.

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

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

@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 latest change catches the Kelvin-sign case, but the existing alias-normalization concern remains open for canonical equivalents that aren't case folds; details are in the thread. I found no new issues in this pass.

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

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

@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 NFD change covers canonical equivalence, but the existing alias-classification concern remains open for APFS's multi-rune case folds; details are in the thread. I found no new issues in this pass.

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

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

@buildsworth-bk-app
buildsworth-bk-app Bot dismissed their stale review August 19, 2026 11:04

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 previous alias-normalization concern is fixed. I left one non-blocking input-name ambiguity. The immutable-source execution and checkout credential boundary remains above the L2 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 18219, then answer my questions about the findings.

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

Comment thread internal/action/integration/checkout.go Outdated
@buildsworth-bk-app
buildsworth-bk-app Bot dismissed their stale review August 19, 2026 11:14

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 previous input-name ambiguity is fixed. One new blocking resource-bound issue remains; details 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 18224, then answer my questions about the findings.

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

Comment thread internal/action/integration/checkout.go
@buildsworth-bk-app
buildsworth-bk-app Bot dismissed their stale review August 19, 2026 11:22

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 previous input-validation resource bound is fixed, and I found no new issues in this pass. This still changes immutable-source execution and checkout credential boundaries, so it needs human approval.

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