Skip to content

Allow composite shell steps to use github.token - #388

Open
lox wants to merge 6 commits into
mainfrom
fix/composite-shell-token-planning
Open

Allow composite shell steps to use github.token#388
lox wants to merge 6 commits into
mainfrom
fix/composite-shell-token-planning

Evaluate guards in their caller context

8ee5482
Select commit
Loading
Failed to load commit list.
buildsworth-bk-app / Buildsworth review succeeded Aug 25, 2026 in 5m 22s

Review submitted — changes requested

Review summary

Change

PR #388 extends token-authority planning to direct github.token references in reachable composite shell-step run, env, and working-directory templates. It adds abstract expression truthiness, condition reachability, known action-input reduction, and workflow-invocation reachability so the compiler can distinguish known-false guards from runtime-dependent branches. It also updates token diagnostics and compatibility documentation and removes the completed expression-authority planning document.

The latest revision addresses the prior review by evaluating nested action with templates against their parent references and by including concrete matrix values when classifying a workflow action step's if condition.

Review coverage

I read the complete PR diff and the changed compiler, expression, program, CLI, test, and compatibility files. I traced:

  • workflow action planning through planBuilder.buildActions, compileReachableActionInvocations, actionNode.inspectInvocation, and knownInputReferences
  • runtime with evaluation and input-default resolution through evaluate(step.With, eval) and resolveActionInputs
  • abstract logical, case(), computed-index, and token-effect behavior
  • direct and nested composite shell-template inspection
  • token authorization, admission diagnostics, and action attribution
  • the tests for provider guards, known inputs, nested actions, computed indexes, unreachable invocations, and matrix guards

No Linear issue ID appears in the branch name, PR title, or PR description, so no Linear acceptance criteria were available.

Buildkite build #1848 passed, including the shell smoke jobs and public-actions proof. Socket checks also passed. I could not run local tests because the session did not provide the pinned Go/mise toolchain.

Findings

The submitted review requests changes for two root-invocation paths:

  1. internal/compiler/actions.go: when no caller map is supplied, callerReferences aliases the child action's mutable known map. Later root with templates can therefore read earlier sibling action inputs instead of the workflow caller's inputs. Planning can incorrectly prove a token branch unreachable and emit a tokenless plan that fails at runtime.
  2. internal/compiler/plan_builder.go: concrete matrix values are used only for the workflow action step's if. They are not supplied when reducing root action with templates. A false matrix value passed into a composite input remains abstractly unknown, can request github.token, and can make an otherwise valid permissions: {} job fail compilation.

The prior nested-with and matrix-step-condition concerns are fixed, acknowledged in their existing threads, and the superseded review was dismissed before posting the new review.

Verdict

Changes requested. The current revision can both omit required token authority and reject a valid tokenless matrix job. The GitHub review contains two inline blocking findings with concrete reproductions and local correction boundaries.

Trigger source: comment.