Configuring frontmatter for bundled artifact folders - #50108
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review changes and finish work |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed the changes and pushed follow-up commit 5bb3e80 to restore ambient folders after custom checkout steps, with a regression test. Validation passed: |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 80/100 — Excellent
Analyzed 3 test(s): 3 design, 0 implementation, 0 violation(s).
📊 Metrics (3 tests)
| Metric | Value |
|---|---|
| Analyzed | 3 (Go: 3, JS: 0) |
| ✅ Design | 3 (100%) |
| 0 (0%) | |
| Edge/error coverage | 2 (67%) |
| Duplicate clusters | 0 |
| Inflation | YES — import_field_extractor_test.go: 43 test lines / 10 prod lines ≈ 4.3:1 |
| 🚨 Violations | 0 |
| Test | File | Classification | Issues |
|---|---|---|---|
TestAmbientFoldersExtractedFromMdImport |
pkg/parser/import_field_extractor_test.go:248 |
design_test, high_value | assert.Equal without descriptive failure message (minor) |
TestAmbientFoldersIncludedInActivationArtifact |
pkg/workflow/compiler_artifacts_test.go:320 |
design_test, high_value | Happy-path only |
TestAmbientFoldersRestoredAfterCustomCheckout |
pkg/workflow/compiler_artifacts_test.go:386 |
design_test, high_value | None |
⚠️ Flagged Tests (1)
TestAmbientFoldersExtractedFromMdImport (pkg/parser/import_field_extractor_test.go:248) — The final assert.Equal lacks a descriptive failure message. Consider adding "ambient folders should be deduplicated and ordered" as the third argument.
Inflation note: import_field_extractor_test.go 4.3:1 ratio is above the 2:1 threshold, but the test covers non-trivial multi-import deduplication behavior — justified.
Verdict
✅ passed. 0% implementation tests (threshold: 30%). Score 80/100 — Excellent. No guideline violations.
There was a problem hiding this comment.
Review: Configuring frontmatter for bundled artifact folders
This PR introduces on.ambient-folders — a well-designed feature that replaces the previous squad-state artifact approach with a generic, reusable mechanism.
Correctness and security look solid:
- Path validation in
normalizeAmbientFoldersusesfilepath.Cleanplus an allowlist regex (^[A-Za-z0-9._/-]+$) that prevents traversal, absolute paths, and whitespace-separated injection in the shellforloop. - Schema validation in
main_workflow_schema.jsonmirrors the same constraints (pattern,notwith absolute/traversal patterns). - Deduplication via
ambientFoldersSetin the accumulator is correct and the union-over-imports semantics are clear. - The
IsImportSafeSharedWorkflowOnhelper correctly gates shared-workflow detection so a file withon: { ambient-folders: [...] }and no trigger is treated as a shared component. - Duplicate restore placement (once after artifact download, once after custom checkouts) is intentional and handled cleanly via
postLastCheckoutSteps. awfnow excludesACTIONS_ID_TOKEN_REQUEST_TOKEN/ACTIONS_ID_TOKEN_REQUEST_URLfrom container env — unrelated but a good hardening change.
Tests cover the key paths (import merging, artifact inclusion, custom-checkout ordering). No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 45.6 AIC · ⌖ 7.53 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Adds import-safe on.ambient-folders configuration for transferring activation-generated folders into agent workspaces.
Changes:
- Parses, validates, merges, and normalizes ambient-folder configuration.
- Stages folders in the activation artifact and restores them after checkouts.
- Migrates Squad state transfer to the shared activation artifact.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Stores resolved ambient folders. |
pkg/workflow/event_validation.go |
Allows the new on key. |
pkg/workflow/compiler_yaml_runtime_setup.go |
Restores folders around custom checkouts. |
pkg/workflow/compiler_yaml_ai_execution.go |
Restores folders after base content. |
pkg/workflow/compiler_orchestrator_workflow.go |
Resolves ambient-folder configuration. |
pkg/workflow/compiler_orchestrator_frontmatter.go |
Recognizes import-safe on blocks. |
pkg/workflow/compiler_artifacts_test.go |
Tests artifact and checkout placement. |
pkg/workflow/compiler_activation_outputs.go |
Stages and uploads ambient folders. |
pkg/workflow/compiler_activation_job.go |
Adds folders to sparse checkout. |
pkg/workflow/ambient_folders.go |
Implements validation, staging, and restoration. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the new schema property. |
pkg/parser/schema_validation.go |
Allows ambient folders in shared workflows. |
pkg/parser/import_processor.go |
Carries merged folder configuration. |
pkg/parser/import_field_extractor.go |
Extracts folders from imports. |
pkg/parser/import_field_extractor_test.go |
Tests imported-folder merging. |
docs/src/content/docs/reference/imports.md |
Documents shared-component support. |
docs/src/content/docs/reference/frontmatter.md |
Documents the frontmatter key. |
.github/workflows/squad-game-planner.lock.yml |
Regenerates the compiled workflow. |
.github/workflows/shared/squad.md |
Migrates Squad to ambient folders. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/ambient_folders.go:123
- The restore script also relies on POSIX shell syntax but inherits the runner's default shell. On supported Windows runners this is parsed as PowerShell and fails before restoration; select Bash explicitly.
" run: |",
- Files reviewed: 19/19 changed files
- Comments generated: 5
- Review effort level: Balanced
| " if [ -e \"$src\" ]; then", | ||
| " mkdir -p \"$(dirname \"$dst\")\"", | ||
| " rm -rf \"$dst\"", | ||
| " cp -a \"$src\" \"$dst\"", | ||
| " fi", |
| " # poutine:ignore untrusted_checkout_exec\n", | ||
| " run: |\n", |
| if clean == "." || clean == ".." || strings.HasPrefix(clean, "../") || filepath.IsAbs(value) || strings.HasPrefix(value, "/") { | ||
| return nil, fmt.Errorf("on.ambient-folders entry %q must be a relative folder path within the repository", folder) | ||
| } |
| var sharedWorkflowForbiddenFields = buildForbiddenFieldsMap() | ||
|
|
||
| var sharedWorkflowAllowedOnFieldList = []string{ | ||
| "ambient-folders", |
| "ambient-folders": { | ||
| "type": "array", | ||
| "description": "Workspace-relative folders to bundle in the activation artifact and restore before the agent runs. Useful for activation steps that generate reusable prompt, skill, or agent context.", |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting on interface consistency and test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Shell word-splitting invariant —
GH_AW_AMBIENT_FOLDERSis space-joined and iterated with an unquotedforloop. The path validator prevents spaces today, but the coupling is undocumented and fragile. - Asymmetric generator API —
generateStageAmbientFoldersStepreturns[]stringwhilegenerateRestoreAmbientFoldersStepwrites to astrings.Builder, making the two harder to test and compose consistently. IsImportSafeSharedWorkflowOnrepurposing — the function is now used for shared-component detection as well as import safety, which conflates two separate responsibilities.- Test ordering not asserted — the artifact tests check presence but not ordering of stage/restore steps relative to upload/agent steps.
Positive Highlights
- ✅ Clean new
ambient_folders.gomodule — validation, normalization, and generation are well-separated. - ✅ Deduplication via
seenset and properfilepath.Cleannormalization. - ✅ Good test coverage for import merging (
TestAmbientFoldersExtractedFromMdImport) and end-to-end compilation (TestAmbientFoldersIncludedInActivationArtifact). - ✅ Sparse-checkout correctly extended to include ambient folders so the activation job can access them.
- ✅ Lock file and schema updated consistently with the implementation.
- ✅
poutine:ignore untrusted_checkout_execannotations present on shell steps.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 55.4 AIC · ⌖ 7.49 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/ambient_folders.go:495
[/codebase-design] The shell loop for folder in $GH_AW_AMBIENT_FOLDERS word-splits on spaces — safe today because the regex ^[A-Za-z0-9._/-]+$ forbids them, but the coupling is implicit and fragile.
Add a comment near the pattern definition or the generated script to document why space-splitting is safe, so a future relaxation of the regex does not silently break path handling.
@copilot please address this.
pkg/workflow/ambient_folders.go:476
[/codebase-design] filepath.IsAbs(value) is checked alongside strings.HasPrefix(value, "/"). On Windows filepath.IsAbs also catches C:\... style paths, but the regex ^[A-Za-z0-9._/-]+$ already blocks : and \, making the IsAbs call redundant. Remove the duplicate check or add a comment explaining the dual guard is intentional for cross-platform safety.
@copilot please address this.
pkg/workflow/compiler_orchestrator_frontmatter.go:738
[/codebase-design] parser.IsImportSafeSharedWorkflowOn is called here to decide whether a file with an on: block should still be treated as a shared component. The function name "IsImportSafe" describes a different concept (can this on: block be safely merged via imports?) rather than "has no real trigger events". The dual meaning — import-safe fields vs. shared-component detection — may confuse future maintainers. Consider a dedicated predicate like onBlockContainsOnlySharedFields …
pkg/workflow/compiler_artifacts_test.go:648
[/tdd] TestAmbientFoldersIncludedInActivationArtifact uses strings.Contains for all assertions. This makes the test pass even if the step appears in the wrong position (e.g., stage step after the upload). Consider asserting ordering: Stage ambient folders must appear before Upload activation artifact, and Restore ambient folders must appear before the agent execution step.
@copilot please address this.
pkg/workflow/ambient_folders.go:519
[/codebase-design] generateRestoreAmbientFoldersStep writes directly to a *strings.Builder while generateStageAmbientFoldersStep returns a []string. The asymmetry makes both functions harder to test in isolation and forces callers to use different patterns. Align them — either both return []string (like the stage variant) or both accept a builder. The restoreAmbientFoldersSteps helper already exists; generateRestoreAmbientFoldersStep could just loop over it the same way caller…
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (403 new lines in 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. Why ADRs MatterADRs 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
|
|
@copilot PR #50108 still needs follow-up before a maintainer can investigate quickly. Please address these unresolved review items first, newest first:
Please refresh the branch if needed, resolve the open review feedback, rerun Run: https://github.com/github/gh-aw/actions/runs/30862559128
|
|
@copilot PR #50108 still needs follow-up before a maintainer can investigate quickly. Please address these unresolved review items first, newest first:
Please refresh the branch if needed, resolve the open review feedback, rerun Run: https://github.com/github/gh-aw/actions/runs/30864142711
|
| GH_AW_AMBIENT_FOLDERS: ".squad .github/agents" | ||
| # poutine:ignore untrusted_checkout_exec | ||
| run: | | ||
| mkdir -p /tmp/gh-aw/ambient-folders |
There was a problem hiding this comment.
@copilot refactor to shell script file in actions/setup/sh
| GH_AW_AMBIENT_FOLDERS: ".squad .github/agents" | ||
| # poutine:ignore untrusted_checkout_exec | ||
| run: | | ||
| for folder in $GH_AW_AMBIENT_FOLDERS; do |
There was a problem hiding this comment.
@copilot refactor into shell script file in actions/setup/sh
|
🎉 This pull request is included in a new release. Release: |
Pull request created by AI Agent