Normalize sandbox frontmatter fields to kebab-case - #64302
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The codemod can create duplicate keys or fail to migrate valid YAML syntax, and several references remain outdated.
Review effort: Balanced
Findings: 1
Open (5)
What changed in this PR
Normalizes sandbox frontmatter to kebab-case while retaining camelCase in generated AWF runtime JSON.
Changes:
- Renames schema, parser, validation, and autocomplete fields.
- Adds a
gh aw fixmigration codemod. - Updates documentation, tests, workflows, and generated lock metadata.
| File | Description |
|---|---|
specs/awf-config-sources-spec.md |
Updates frontmatter mappings. |
pkg/workflow/strict_mode_sandbox_validation.go |
Updates strict-mode field names. |
pkg/workflow/strict_mode_sandbox_validation_test.go |
Updates strict-mode assertions. |
pkg/workflow/sandbox.go |
Changes YAML tags to kebab-case. |
pkg/workflow/sandbox_agent_images.go |
Maps image roles to AWF camelCase. |
pkg/workflow/frontmatter_extraction_security.go |
Extracts normalized sandbox fields. |
pkg/workflow/firewall.go |
Updates field documentation. |
pkg/workflow/engine_api_targets.go |
Updates target-field documentation. |
pkg/workflow/copilot_byok_extra_fields_compilation_test.go |
Tests kebab-case BYOK fields. |
pkg/workflow/compiler_validators.go |
Updates warning text. |
pkg/workflow/compiler_validators_test.go |
Updates warning assertions. |
pkg/parser/schemas/main_workflow_schema.json |
Enforces kebab-case sandbox fields. |
pkg/parser/schema_test.go |
Tests acceptance and rejection behavior. |
pkg/cli/fix_codemods.go |
Registers the migration codemod. |
pkg/cli/fix_codemods_test.go |
Verifies codemod registration order. |
pkg/cli/codemod_sandbox_fields.go |
Implements sandbox-field migration. |
pkg/cli/codemod_sandbox_fields_test.go |
Tests migration and preservation. |
docs/src/content/docs/reference/self-hosted-runners.md |
Updates image-role example. |
docs/src/content/docs/reference/sandbox.md |
Updates sandbox examples. |
docs/src/content/docs/reference/frontmatter-full.md |
Refreshes generated reference content. |
docs/src/content/docs/reference/faq.md |
Updates image-role example. |
docs/public/editor/autocomplete-data.json |
Updates editor metadata. |
.github/workflows/step-name-alignment.md |
Migrates sandbox frontmatter. |
.github/workflows/step-name-alignment.lock.yml |
Refreshes generated metadata. |
.github/workflows/smoke-pi.md |
Migrates sandbox frontmatter. |
.github/workflows/smoke-pi.lock.yml |
Refreshes generated metadata. |
.github/workflows/smoke-claude.md |
Migrates sandbox frontmatter. |
.github/workflows/smoke-claude.lock.yml |
Refreshes generated metadata. |
.changeset/normalize-sandbox-fields.md |
Documents the breaking change. |
| if replacement := sandboxFieldReplacement(parent, key); replacement != "" { | ||
| line, _ = findAndReplaceInLine(line, key, replacement) | ||
| key = replacement | ||
| modified = true | ||
| } |
There was a problem hiding this comment.
Fixed in eecaf4e: the codemod now detects a canonical sibling and returns a guided error without modifying the workflow; block and flow collision cases are covered.
| squid: registry.example.com/approved/squid:v0.28.8@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| agent: registry.example.com/approved/agent:v0.28.8@sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| apiProxy: registry.example.com/approved/api-proxy:v0.28.8@sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| api-proxy: registry.example.com/approved/api-proxy:v0.28.8@sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef |
There was a problem hiding this comment.
Updated the role examples to api-proxy, cli-proxy, and build-tools in eecaf4e.
| squid: registry.example.com/approved/squid:v0.28.4@sha256:<64-hex-digest> | ||
| agent: registry.example.com/approved/agent:v0.28.4@sha256:<64-hex-digest> | ||
| apiProxy: registry.example.com/approved/api-proxy:v0.28.4@sha256:<64-hex-digest> | ||
| api-proxy: registry.example.com/approved/api-proxy:v0.28.4@sha256:<64-hex-digest> |
There was a problem hiding this comment.
Updated the supported and required image role names to kebab-case in eecaf4e.
| squid: registry.example.com/approved/squid:v0.28.8@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| agent: registry.example.com/approved/agent:v0.28.8@sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| apiProxy: registry.example.com/approved/api-proxy:v0.28.8@sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| api-proxy: registry.example.com/approved/api-proxy:v0.28.8@sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef |
There was a problem hiding this comment.
Updated the role references to cli-proxy and build-tools in eecaf4e.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Attempted to submit a PR review for #64302 twice via safeoutputs create_pull_request_review_comment and submit_pull_request_review, but both calls failed with: Permission denied and could not request permission from user. No GitHub write was recorded; continuity memory was updated locally at /tmp/gh-aw/comment-memory/pr-code-quality-reviewer.md.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
🏗️ ADR Required - draft added for PR #64302I added a draft ADR at Evidence used
Gate result
Next actionPlease review and refine the draft ADR, then keep it with this architectural change before merge.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs (via pr-triage) — requesting changes: a confirmed logic bug in the codemod plus lingering doc inconsistencies from an unresolved earlier review pass.
📋 Key Themes & Highlights
Key Themes
- Codemod correctness (
/tdd): reproduced the previously-flagged duplicate-key bug inpkg/cli/codemod_sandbox_fields.go— when both the old and new spelling of a field already coexist (e.g.allowWriteandallow-write), the rename produces two identicalallow-write:keys, which is invalid/ambiguous YAML. No test currently covers this partially-migrated scenario. - Documentation consistency (
/grill-with-docs):docs/src/content/docs/reference/sandbox.md,docs/src/content/docs/reference/faq.md, anddocs/src/content/docs/reference/self-hosted-runners.mdstill list the now-rejected camelCase image role names (apiProxy,cliProxy,buildTools, etc.) even though the adjacent examples were migrated to kebab-case. These are the same gaps an earlier automated review pass already flagged and remain unaddressed in the current diff.
Positive Highlights
- ✅ The core schema/parser/compiler rename is comprehensive and consistent (
pkg/workflow/frontmatter_extraction_security.go,strict_mode_sandbox_validation.go, schema, tests all aligned). - ✅
sandboxImageRoleFromFrontmattercleanly bridges the kebab-case frontmatter spelling to AWF's required camelCase runtime JSON without leaking the old spelling into the public API. - ✅ New schema tests explicitly assert camelCase fields are rejected — good regression coverage for the breaking change itself.
Please fix the codemod duplicate-key handling (add a collision check + regression test) and sweep the remaining doc files for leftover camelCase role names before merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 64.6 AIC · ⌖ 13.7 AIC · ⊞ 10K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/codemod_sandbox_fields.go:121
[/tdd] Confirmed via a quick reproduction test: when a mapping already has both spellings (e.g. allowWrite and allow-write), this codemod renames the old key without checking for a collision, producing two identical allow-write: keys in the output YAML.
<details>
<summary>💡 Reproduction and fix idea</summary>
Running the codemod against:
sandbox:
agent:
config:
filesystem:
allowWrite: [/workspace]
allow-write: [/already-migrated]produces:
``…
docs/src/content/docs/reference/sandbox.md:248
[/grill-with-docs] The example above uses api-proxy, but this role list and the required-role rule on the next line still teach readers the rejected camelCase spellings (apiProxy, cliProxy, buildTools, dohProxy, enclaveScript, enclaveAgent, enclaveMcpServer, dindStaging). A reader copying these names will get a schema-validation error.
<details>
<summary>💡 Suggested fix</summary>
Replace with: squid, agent, api-proxy, cli-proxy, build-tools, doh-proxy, `encl…
docs/src/content/docs/reference/faq.md:203
[/grill-with-docs] This line still lists the rejected camelCase role names apiProxy, cliProxy, buildTools even though the example three lines above was migrated to api-proxy. Update the prose to the kebab-case spellings so the doc is internally consistent.
@copilot please address this.
docs/src/content/docs/reference/self-hosted-runners.md:242
[/grill-with-docs] cliProxy and buildTools here are now-rejected spellings; use cli-proxy and build-tools so this cross-reference matches the migrated schema.
@copilot please address this.
There was a problem hiding this comment.
Impeccable Skills Review — Normalize sandbox frontmatter fields to kebab-case
Change type: refactor_cleanup (schema/parser field rename). This diff is backend Go/JSON/docs with no UI surface, so Impeccable's UI-focused modes (distill/extract/critique/audit) don't map directly; I applied a correctness/security-focused review instead per the fallback rule.
Verification performed
- Built the repo (
go build ./...) — succeeds. - Ran targeted unit tests (
pkg/clicodemod tests,pkg/parserschema tests) — all pass. - Wrote ad-hoc regression tests to probe the codemod's line-transform logic:
- Confirmed the duplicate-key bug already flagged in the existing review thread on
pkg/cli/codemod_sandbox_fields.go:125: when a mapping has bothallowWriteandallow-writesiblings, the codemod renames the old key unconditionally, producing two identicalallow-write:keys in the emitted YAML (verified reproduction, invalid YAML output). This is a real, blocking defect for partially-migrated workflows and should be fixed (e.g., skip the rename or drop/warn when the target key already exists as a sibling) before merge. - Sibling-key handling (
filesystem+networkunder the same parent) works correctly — no false positives there. - Sequence/list items (
targets:as a list of maps) are correctly left untouched since the codemod tracks mapping stacks, not list items — no additional bug found here. - Confirmed
pkg/workflow/sandbox_agent_images.go'ssandboxImageRoleFromFrontmatterand the AWF JSON output structs (AWFAPITargetConfigetc. inawf_config.go) correctly retain camelCase for the generated AWF runtime config, matching the PR's stated design (kebab-case frontmatter, camelCase preserved only in generated AWF JSON). - Checked
docs/src/content/docs/reference/mcp-gateway.md'sentrypointArgsreferences — these document the generated MCP gateway config JSON (a different schema fromsandbox.mcp.entrypoint-argsfrontmatter), so they are correctly left as camelCase and are not stale.
- Confirmed the duplicate-key bug already flagged in the existing review thread on
Outstanding issues (already surfaced by existing review comments — not duplicated here)
The existing Copilot review thread already flags, and I independently confirmed as accurate:
pkg/cli/codemod_sandbox_fields.go:125— duplicate-key bug (confirmed reproducible, blocking).docs/src/content/docs/reference/faq.md:223,docs/src/content/docs/reference/sandbox.md:243,docs/src/content/docs/reference/self-hosted-runners.md:236— stale camelCase role names (apiProxy,cliProxy,buildTools, etc.) left in prose describing the now-rejected schema.pkg/workflow/sandbox.go:122— staleextraHeadersexample in a doc comment.
No new high-signal issues found beyond these. Recommending Request Changes until the duplicate-key codemod bug (#1) is fixed, since it can silently corrupt frontmatter for any workflow with a partially-migrated sandbox block.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 158.2 AIC · ⌖ 13.4 AIC · ⊞ 8.1K
| line, _ = findAndReplaceInLine(line, key, replacement) | ||
| key = replacement | ||
| modified = true | ||
| } |
There was a problem hiding this comment.
Confirmed via reproduction: when a mapping already has both spellings (e.g. allowWrite and allow-write as siblings), normalizeSandboxFieldLines renames allowWrite -> allow-write unconditionally without checking whether the target key already exists in the same mapping. This produces invalid YAML with two duplicate allow-write: keys, which will silently corrupt any partially-migrated workflow file when gh aw fix runs.
Repro (frontmatter):
sandbox:
agent:
config:
filesystem:
allowWrite: [/workspace]
allow-write: [/other]Output after codemod:
allow-write: [/workspace]
allow-write: [/other]Suggested fix: before renaming, check if replacement already exists as a sibling key in the current mapping (track keys seen at each stack level) and skip/warn instead of blindly overwriting.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 94834af: gh aw fix now reports a guided error when old and canonical sandbox keys coexist, leaving the file unchanged. Added block- and flow-mapping collision tests.
|
@copilot run pr-finisher skill |
|
@copilot PR #64302 still needs forward progress. Please review the open feedback, refresh the branch, and run the Open review signals:
Branch refresh:
|
|
Congratulations
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran the
Addressed the inline YAML migration gap and outstanding documentation/collision feedback in 94834af. The branch already contains current |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Sandbox frontmatter now consistently uses kebab-case. Camel-cased fields are rejected without parser compatibility aliases;
gh aw fixprovides migration.Parser and schema
allowWrite,authHeader, andentrypointArgs.Migration
Documentation and workflows
Run: https://github.com/github/gh-aw/actions/runs/36600355521