fix: extract sandbox.agent.memory from frontmatter so --memory-limit reaches AWF - #49448
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes sandbox.agent.memory propagation so compiled AWF commands receive --memory-limit.
Changes:
- Extracts and validates agent memory limits.
- Adds extraction and validation tests.
- Documents memory configuration and troubleshooting.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/frontmatter_extraction_security.go |
Extracts the memory value. |
pkg/workflow/frontmatter_extraction_security_test.go |
Tests memory extraction. |
pkg/workflow/sandbox_validation.go |
Validates memory-limit syntax. |
pkg/workflow/sandbox_validation_test.go |
Tests validation behavior. |
docs/src/content/docs/reference/sandbox.md |
Documents memory limits. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
|
|
||
| // validateAgentMemoryLimit checks that a sandbox.agent.memory string has the correct format. | ||
| func validateAgentMemoryLimit(memory string) error { | ||
| if !memoryLimitPattern.MatchString(memory) { |
| return NewValidationError( | ||
| "sandbox.agent.memory", | ||
| memory, | ||
| "memory value is not a valid limit. Expected a positive number followed by a unit: b, k, m, or g (e.g. \"4g\", \"512m\")", |
| config := compiler.extractAgentSandboxConfig(agentObj) | ||
| require.NotNil(t, config, "Should extract agent sandbox config") | ||
| assert.Equal(t, "48g", config.Memory, "Should extract sandbox.agent.memory") |
| When omitted, AWF's own default memory limit applies. Specifying an invalid format (e.g., `48gb` or `48`) is rejected at compile time. | ||
|
|
||
| > [!NOTE] | ||
| > If your build tools (MSBuild, `csc`, large JVM processes) are killed with exit code 137, the container is hitting its memory limit. Set `memory` to match your runner's available RAM. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: fix: extract sandbox.agent.memory from frontmatter
The fix correctly adds the missing extraction and validation path. Four pre-existing inline comments call out issues that should be addressed:
- Schema/validator mismatch (
sandbox_validation.go:457) —main_workflow_schema.jsonstill accepts formats (e.g.48gb,0m,08g) that the new validator rejects. These need to be aligned. - Error message (
sandbox_validation.go:461) — the guidance text does not mention the no-leading-zeros rule, so08glooks valid to users. - Test coverage gap (
frontmatter_extraction_security_test.go:201) — no end-to-end test from workflow frontmatter to the compiled--memory-limitflag; adding one would prevent regression. - Exit 137 docs (
sandbox.md:163) — exit 137 is SIGKILL, not conclusively a container memory limit hit; the note should be qualified.
Item 1 (schema mismatch) is the most impactful: it causes silent divergence between editor validation and compile-time validation.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.5 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested. Four inline comments have been added (by a prior review pass) covering the most impactful issues; this review summarises the overall picture.
📋 Key Themes & Highlights
Issues to Address
- Schema/validator mismatch:
memoryLimitPatternrejects values (48gb,0m,08g) that the JSON schema still accepts — users will get a compile-time error with no schema-level warning in editors. - Error message clarity: The error message does not call out the leading-zero restriction, so
08glooks like it satisfies the guidance. - Integration test gap: Extraction tests call
extractAgentSandboxConfigdirectly; no test compiles a full workflow and confirms--memory-limitappears in the generated lock file (the regression path from the linked issue). - Docs: exit 137 wording: Exit 137 = SIGKILL, which has multiple causes; the note should qualify that OOM is one possible cause, not the definitive one.
Positive Highlights
- ✅ Root cause properly fixed — extraction was the missing link, not the arg builder
- ✅ Mirrors the existing
mounts/runtimepattern cleanly - ✅ Comprehensive unit test matrix (8 format cases + integration)
- ✅ Compile-time rejection of invalid formats is the right UX
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 18.8 AIC · ⊞ 7K
Comment /matt to run again
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 90/100 — Excellent
Analyzed 14 test(s): 14 design, 0 implementation, 0 violation(s).
📊 Metrics (14 tests)
| Metric | Value |
|---|---|
| Analyzed | 14 (Go: 14, JS: 0) |
| ✅ Design | 14 (100%) |
| 0 (0%) | |
| Edge/error coverage | 14 (100%) |
| Duplicate clusters | 0 |
| Inflation | Yes (3.6:1 ratio) |
| 🚨 Violations | 0 |
| Test | File | Classification | Coverage |
|---|---|---|---|
TestExtractAgentSandboxConfigMemory (3 subtests) |
frontmatter_extraction_security_test.go | design_test | Happy path (present value), edge cases (missing, wrong type) |
TestValidateAgentMemoryLimit (9 table cases) |
sandbox_validation_test.go | design_test | Format validation: valid units, missing units, invalid suffixes, leading zeros, zero value, empty string |
TestValidateSandboxConfigMemory (3 subtests) |
sandbox_validation_test.go | design_test | Integration: valid memory, invalid format with error message, absent memory |
Analysis
All tests are design-level contract tests verifying user-facing behavior:
- ✅ Extraction logic correctly reads and ignores non-string values
- ✅ Validation enforces strict format (lowercase/uppercase units, no leading zeros, positive)
- ✅ Integration tests confirm validation is wired into the sandbox config pipeline
Test inflation note: The 3.6:1 ratio (101 test lines / 28 production lines) is justified—this is a critical security and container-tuning feature requiring comprehensive edge-case coverage across extraction, validation, and integration layers. No redundant tests detected.
Code quality signal: All assertions include descriptive failure messages (per Go convention). No mock libraries used. Boundary testing is thorough.
Verdict
✅ passed. 0% implementation tests (threshold: 30%). 14/14 design tests verify critical sandbox memory feature contracts.
…mpile-time validation
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (129 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 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.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: Request changes
Core fix (wiring sandbox.agent.memory extraction to the already-existing AWF --memory-limit arg builder) is correct and well-tested at the unit level, but the new validation regex conflicts with the published JSON schema, and non-string YAML values silently bypass validation entirely.
💡 Themes
- Schema/validator drift (blocking):
memoryLimitPattern(^[1-9][0-9]*[bkmgBKMG]$) rejects48gb/0m/08g/two-letter units, whilemain_workflow_schema.jsonstill accepts them — editors andgh aw compilewill disagree on the same input, and the docs' own "invalid" example is schema-valid. - Silent failure mode (blocking-adjacent): a non-string
memoryvalue (e.g. unquotedmemory: 48) is dropped with no error and no validation, silently disabling the feature this PR exists to fix. - Minor:
validateAgentMemoryLimitduplicatesvalidateBoundedQueryMemoryLimitalmost verbatim — worth consolidating to avoid future drift (as already demonstrated here). - Tests are solid at the extraction/validation unit level but don't exercise the full frontmatter → compile → AWF-args regression path the linked issue was about.
🔎 Code quality review by PR Code Quality Reviewer · auto · 40.2 AIC · ⊞ 7.8K
Comment /review to run again
| } | ||
|
|
||
| // validateAgentMemoryLimit checks that a sandbox.agent.memory string has the correct format. | ||
| func validateAgentMemoryLimit(memory string) error { |
There was a problem hiding this comment.
The new Go regex is stricter than — and inconsistent with — the published JSON schema for the same field, so identical values pass one gate and fail the other.
💡 Details
memoryLimitPattern here is ^[1-9][0-9]*[bkmgBKMG]$ (single-letter units, no leading zero, no zero value), but pkg/parser/schemas/main_workflow_schema.json for sandbox.agent.memory still uses ^[0-9]+(b|k|m|g|kb|mb|gb|B|K|M|G|KB|MB|GB)$, which accepts 48gb, 0m, 08g, and two-letter units (kb/mb/gb) that the Go validator rejects, and also accepts 0m/08g which Go rejects.
Result: editor/IDE JSON-schema validation (via the schema) will happily accept memory: 48gb or memory: 0m, but gh aw compile then rejects them with a Go-level error — a confusing split-brain UX. The docs' own "rejected" example (48gb) is actually schema-valid, compounding the confusion.
Fix: align the schema pattern with memoryLimitPattern, or relax the Go validator to accept the schema's broader set (kb/mb/gb suffixes), and update the docs example accordingly so all three sources of truth agree.
| } | ||
|
|
||
| // Extract memory (memory limit for the AWF container) | ||
| if memoryVal, hasMemory := agentObj["memory"]; hasMemory { |
There was a problem hiding this comment.
Non-string memory values (e.g. unquoted YAML memory: 48) are silently dropped — no warning, no error — and validation is skipped entirely.
💡 Details
if memoryStr, ok := memoryVal.(string); ok { ... } silently no-ops when the type assertion fails. A very plausible authoring mistake — writing memory: 48 instead of memory: "48g" — parses as a YAML int, fails the type check, and agentConfig.Memory stays empty. Since validateAgentMemoryLimit is only invoked when Memory != "", this bad input never triggers the intended validation error; the workflow just compiles with the default AWF memory limit and no diagnostic at all, silently defeating the entire feature this PR is meant to fix.
Suggested fix: when hasMemory is true but the type assertion fails, return/record a validation error (mirroring how invalid string values are already caught by validateAgentMemoryLimit) rather than silently ignoring it.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
I also ran local validation ( |
|
🎉 This pull request is included in a new release. Release: |
sandbox.agent.memorywas declared, schema-documented, and consumed by the AWF arg builder, butextractAgentSandboxConfignever assigned it — causing--memory-limitto silently disappear from every compiled lock file.Changes
frontmatter_extraction_security.go): addmemoryextraction block inextractAgentSandboxConfig, mirroring themounts/runtimepatternsandbox_validation.go): addvalidateAgentMemoryLimit(reuses existingmemoryLimitPattern) and call it fromvalidateSandboxConfig— invalid formats (e.g.48gb,48) are rejected at compile timefrontmatter_extraction_security_test.go,sandbox_validation_test.go): extraction cases (valid string, absent, non-string), format validation cases, and integration throughvalidateSandboxConfigdocs/reference/sandbox.md): documentsandbox.agent.memorywith valid format examples and a note on exit 137Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30680808859