Repository navigation
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require allowed-repos for repository expressions, keep wildcards exclusive to scoped allowlists, and propagate hard checkout rejections into agent failure issues. Fail compilation instead of silently dropping invalid main or imported checkout configurations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…pression Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@copilot merge main and do a deep review as a GitHub actions security expert and compare to the security architecture of AW. Be adverserial. |
|
@copilot add a max-repos to place a limit of the number of repos checked out. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged current Confirmed HIGH finding, fixed in Independent security re-review verified 33 host/setting combinations; focused code review found no further significant issues. Allowlist expressions must still come from trusted policy sources. Validation: 615 targeted JavaScript tests, type checking, focused Go checkout/credential/parser tests, and final impacted tests passed; secret scanning passed. The publication gate remains blocked by inherited custom Go lint findings. Automated review/CodeQL timed out twice generating the merge diff and is incomplete. No live authenticated Actions checkout or end-to-end disclosure test was performed. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (692 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 🔍 Evidence used
❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
🟡 Changes recommended
Dynamic allowlist expressions currently permit caller-controlled policy expansion, defeating the intended trust boundary.
1 open finding
What changed in this PR
Adds runtime-expanded, allowlisted repository checkouts while preserving credential isolation, manifest generation, and failure reporting.
Changes:
- Adds dynamic checkout validation, expansion, metadata checks, and credential cleanup.
- Propagates checkout parsing and runtime failures through compiler and failure-reporting paths.
- Updates schemas, documentation, migration notes, and tests.
Security review found one must-fix issue: caller-controlled expressions can currently define both repository selection and its allowlist.
| File | Description |
|---|---|
pkg/workflow/workflow_builder.go |
Propagates checkout parsing errors. |
pkg/workflow/side_repo_maintenance_integration_test.go |
Updates dynamic checkout fixture. |
pkg/workflow/pull_request_target_validation.go |
Revises trusted-checkout guidance. |
pkg/workflow/pull_request_target_validation_test.go |
Updates pull-request-target cases. |
pkg/workflow/pr_test.go |
Updates multi-checkout fixtures. |
pkg/workflow/pr_ready_for_review_checkout_test.go |
Simplifies workflow-repository checkout. |
pkg/workflow/notify_comment_conclusion_helpers.go |
Forwards dynamic checkout failures. |
pkg/workflow/inline_imports_test.go |
Handles builder errors. |
pkg/workflow/frontmatter_types_test.go |
Tests invalid checkout booleans. |
pkg/workflow/frontmatter_parsing.go |
Returns checkout parsing errors. |
pkg/workflow/compiler.go |
Validates runtime checkout configurations. |
pkg/workflow/compiler_string_api.go |
Propagates builder failures. |
pkg/workflow/compiler_safe_outputs_job.go |
Exposes safe-output checkout errors. |
pkg/workflow/compiler_orchestrator_workflow.go |
Handles builder errors. |
pkg/workflow/compiler_orchestrator_workflow_test.go |
Updates builder tests. |
pkg/workflow/compiler_main_job_helpers.go |
Exposes agent checkout errors. |
pkg/workflow/checkout_step_generator.go |
Generates dynamic checkout and manifest steps. |
pkg/workflow/checkout_path_validation.go |
Updates dynamic-path documentation. |
pkg/workflow/checkout_manifest_compile_test.go |
Tests dynamic manifest wiring. |
pkg/workflow/checkout_manager.go |
Stores runtime allowlists. |
pkg/workflow/checkout_dynamic.go |
Implements dynamic checkout generation. |
pkg/workflow/checkout_dynamic_test.go |
Covers compiler-side dynamic behavior. |
pkg/workflow/checkout_credentials_render.go |
Excludes dynamic entries from static cleanup. |
pkg/workflow/checkout_config_parser.go |
Parses and validates runtime lists. |
pkg/parser/schemas/main_workflow_schema.json |
Adds alias and allowlist schema. |
docs/src/content/docs/specs/checkout-behavior-specification.md |
Specifies runtime checkout behavior. |
docs/src/content/docs/reference/frontmatter-full.md |
Documents new frontmatter fields. |
docs/src/content/docs/reference/checkout.md |
Adds runtime checkout guidance. |
actions/setup/sh/clean_git_credentials.sh |
Supports explicit config cleanup. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests checkout failure reporting. |
actions/setup/js/handle_agent_failure.cjs |
Reports runtime checkout rejections. |
actions/setup/js/checkout_dynamic.test.cjs |
Tests runtime validation and cleanup. |
actions/setup/js/checkout_dynamic.cjs |
Implements runtime checkout expansion. |
actions/setup/js/build_checkout_manifest.cjs |
Incorporates dynamic checkout plans. |
.changeset/major-runtime-repository-checkout.md |
Records the breaking release change. |
🧠 Review effort: Balanced
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Request changes
The runtime checkout work is not wired end to end yet: the agent path knows about the dynamic manifest and cleanup rules, but the safe_outputs path still loses runtime-selected repositories and can keep a different credential than the PR handlers actually use.
Blocking themes
safe_outputsnever emits the checkout manifest for runtime-selected repos, so PR handlers fall back to the bounded workspace scan and can miss valid deep checkouts.- Dynamic
safe_outputscheckouts fall back topushToken, but that path does not carry the global safe-outputs GitHub App step output, so runtime-selected repos can fetch or push with the wrong credential.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 119.2 AIC · ⌖ 5.93 AIC · ⊞ 21.4K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design to this high-risk runtime-checkout feature. The design is unusually thorough (fail-closed metadata checks, SHA-pinned actions, path-traversal/symlink defense, credential cleanup verification, 592 passing tests), and the LFS/submodule rejection logic correctly blocks the direct allowlist-bypass vector.
📋 Key Themes & Highlights
Key Themes
- One open finding from GitHub Advanced Security (flagged on
checkout_dynamic.cjs:150): submodule rejection is based solely on thecheckout.submodulesfrontmatter setting; a repository-controlled.gitmodulesfile is never inspected. Today this is mitigated because the generatedactions/checkoutstep hard-codessubmodules: false, closing the direct bypass — but the surrounding comments/error text ("submodule URLs can bypass allowed-repos") promise a broader guarantee than is explicitly tested. Left an inline comment requesting either an explicit test/comment confirming no code path ever acts on.gitmodulesfor these checkouts, or a defense-in-depth guard if one exists. - Everything else reviewed (allowlist parsing/merging, path validation, credential cleanup, failure propagation, compiler validation ordering) is well-tested and consistent with existing patterns (
resolvedCheckout,CheckoutManager,writeYAMLEnvescaping).
Positive Highlights
- ✅ Fail-closed API visibility checks before any checkout action is generated, including canonical-name re-validation after redirects.
- ✅
allowedReposmerge semantics (first-seen wins) are explicitly tested against import-based policy broadening. - ✅ Credential cleanup (
cleanCheckoutCredentials) walks real filesystem state rather than relying on boundedfinddiscovery, closing the deep-prefix bypass described in the PR body, with a dedicated regression test for nested submodule configs beyond the old depth cap. - ✅ Thorough test coverage across both the Go compiler layer and the JS runtime layer for LFS/submodule/path/symlink edge cases.
Given the single open finding is already acknowledged by tooling and has a clear closure path, this does not block approval but should be resolved before merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 153.8 AIC · ⌖ 15.6 AIC · ⊞ 10.3K
Comment /matt to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added Updated schema, documentation, and regression tests. Focused Go tests, JavaScript type checking, and 628 final JavaScript tests passed. Independent code review found no significant issues; the publication gate still has inherited custom Go lint failures. Automated validation reported success but skipped unchanged review/Go analysis. |
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 4741e30
|
Restrict allowlist expression provenance, disable recursive submodule fetches, and wire safe-output manifests and effective checkout authentication with regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Align safe-output requirements with trusted runtime policies, bounded preflight, job-local manifests, credential finalization, staged isolation, and failure reporting. Add an executable bounded TLA+ model with checker integration, negative controls, and reachability witnesses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Why
Scheduled workers need to process a changing set of repositories without recompiling their workflows. This adds runtime checkout expansion while keeping repository selection constrained by an explicit policy and preserving per-checkout credentials.
Approach
checkout.repositoryor itsrepoalias, with mandatoryallowed-reposand a literal non-root path prefix for runtime expansion. The standalone${{ github.repository }}expression is exempt and retains normal single-repository checkout behavior without an allowlist or prefix. Scoped wildcard patterns filter selections; repository selectors and unrestricted wildcard policies are rejected.prefix/owner/repo, record the results in the checkout manifest, and share the layout between agent and safe-output jobs. Target metadata and follow-up fetches use the entry's credentials; agent checkouts remain credential-free.Security and migration
Repository expressions require
allowed-reposexcept for the standalone${{ github.repository }}expression. To retain the workflow repository at the workspace root, omitrepositoryor use that expression without an allowlist. Other expressions require a policy and prefix; compound expressions containinggithub.repositorydo not qualify for the exception. A major changeset and updated reference/specification docs describe the migration.Runtime lists reject
lfs: trueat compilation and runtime because repository-controlled LFS endpoints can bypass the allowlist using the checkout token. Generated dynamic actions explicitly disable LFS; static checkout support is unchanged. Public-host runtime checkouts also reject submodules whose visibility cannot be verified. Policy expressions must resolve from trusted configuration, not attacker-controlled selection data.Agent finalization explicitly cleans and verifies each expanded checkout's git config and nested submodule configs using validated checkout paths, independent of bounded filesystem discovery. This closes the deep-prefix credential-cleanup bypass, including with
force-clean-git-credentials: true. Missing configs, unavailable helpers, symlinked configs, or residual credentials stop the job. Safe-output jobs retain their intended write credentials.Validation
Focused Go compiler/parser/checkout and credential-verification tests, JavaScript type checking, and 592 runtime/manifest/failure-handler tests passed. Local Git regressions cover deep credential includes, nested submodules, failed and false-success cleanup, missing helpers/configs, symlinks, and safe-output credential retention. Standard Go lint, changed-file JavaScript lint, action shell checks, and schema freshness checks passed.
The publication gate remains blocked by pre-existing custom Go lint findings in changed files. Broader package testing also encountered macOS/Linux fixture limitations; no live GitHub Actions execution or authenticated end-to-end LFS reproduction was performed.