-
Notifications
You must be signed in to change notification settings - Fork 475
fix: extract sandbox.agent.memory from frontmatter so --memory-limit reaches AWF #49448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
416352e
86b42d2
6b78efa
ca408b0
d70f34d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # ADR-49448: Extract sandbox.agent.memory from Frontmatter with Compile-Time Format Validation | ||
|
|
||
| **Date**: 2026-08-01 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The `sandbox.agent.memory` field was declared in the AWF workflow schema and consumed by the AWF arg builder to produce `--memory-limit`. However, `extractAgentSandboxConfig` never assigned the field, so `--memory-limit` was silently dropped from every compiled lock file. Users who set `sandbox.agent.memory` saw no error, no warning, and no effect — the field appeared functional in documentation but was effectively broken. Without memory overrides, large-memory build tools (MSBuild, JVM processes) hit the default container memory cap and were killed with exit code 137. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will complete the extraction of `sandbox.agent.memory` inside `extractAgentSandboxConfig`, mirroring the existing `mounts`/`runtime` extraction pattern, and add a dedicated `validateAgentMemoryLimit` function that reuses the existing `memoryLimitPattern` regex to reject malformed values (e.g. `48gb`, `48`) at compile time rather than at execution time. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Leave the silent drop in place (status quo) | ||
|
|
||
| The field remains schema-documented but has no effect. Users who configure memory limits see no error and receive no benefit. This is the current accidental state — it is not a deliberate choice and provides no value; keeping it active would be actively misleading. | ||
|
|
||
| #### Alternative 2: Extract the field but defer validation to AWF at execution time | ||
|
|
||
| The extraction step is added but no compile-time validation is performed; AWF is responsible for rejecting invalid format strings when it starts the container. This delays error detection to workflow execution, producing a runtime failure rather than a compile-time failure, and makes the error message less actionable since it surfaces far from the authoring step. | ||
|
|
||
| #### Alternative 3: Extract and validate at compile time (chosen) | ||
|
|
||
| Extraction is added alongside a compile-time validator that matches the pattern already used for bounded-query memory limits. Invalid formats are rejected at `gh aw compile` time with a descriptive error pointing to the docs. This is consistent with how other `sandbox` sub-fields are validated and provides the best developer experience. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - `sandbox.agent.memory` now propagates correctly to AWF as `--memory-limit`, making the feature functional. | ||
| - Malformed memory values are caught at compile time with a clear, actionable error message, consistent with the existing validation philosophy for sandbox fields. | ||
| - The fix reuses the existing `memoryLimitPattern`, so no new regex is introduced. | ||
|
|
||
| #### Negative | ||
| - `validateAgentMemoryLimit` is a thin wrapper around `memoryLimitPattern`; if the pattern's semantics change, callers must be updated consistently. | ||
| - Any workflow that previously silently had `sandbox.agent.memory` set to an invalid string will now fail at compile time — a breaking change for malformed configs, though this surfaces a pre-existing latent error. | ||
|
|
||
| #### Neutral | ||
| - Test coverage is added for the extraction path (valid string, absent, non-string type) and for the validation function (valid units, invalid suffixes, leading zeros, zero value). | ||
| - Documentation in `docs/reference/sandbox.md` now includes valid format examples and an exit-137 note. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,13 @@ func validateSandboxConfig(workflowData *WorkflowData) error { | |
| } | ||
| } | ||
|
|
||
| // Validate memory format if specified in agent config | ||
| if agentConfig != nil && agentConfig.Memory != "" { | ||
| if err := validateAgentMemoryLimit(agentConfig.Memory); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Validate gVisor runtime compatibility | ||
| if agentConfig != nil && agentConfig.Runtime == AgentRuntimeGVisor { | ||
| // gVisor is incompatible with ARC/DinD topology: the runner has no access to the | ||
|
|
@@ -445,6 +452,19 @@ func validateBoundedQueryMemoryLimit(memoryLimit string) error { | |
| return nil | ||
| } | ||
|
|
||
| // validateAgentMemoryLimit checks that a sandbox.agent.memory string has the correct format. | ||
| func validateAgentMemoryLimit(memory string) error { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Result: editor/IDE JSON-schema validation (via the schema) will happily accept Fix: align the schema pattern with |
||
| if !memoryLimitPattern.MatchString(memory) { | ||
|
|
||
| return NewValidationError( | ||
| "sandbox.agent.memory", | ||
| memory, | ||
| "memory value is not a valid limit. Expected a positive integer without leading zeros followed by a unit: b, k, m, or g (e.g. \"4g\", \"512m\")", | ||
| fmt.Sprintf("Use a valid memory limit format:\n\nsandbox:\n agent:\n memory: 4g # examples: 512m, 4g, 8g\n\nSee: %s", constants.DocsSandboxURL), | ||
| ) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func getSandboxDisableJustification(workflowData *WorkflowData) (string, error) { | ||
| if workflowData == nil || workflowData.Features == nil { | ||
| return "", errors.New("dangerously-disable-sandbox-agent feature is missing") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-string
memoryvalues (e.g. unquoted YAMLmemory: 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 — writingmemory: 48instead ofmemory: "48g"— parses as a YAML int, fails the type check, andagentConfig.Memorystays empty. SincevalidateAgentMemoryLimitis only invoked whenMemory != "", 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
hasMemoryis true but the type assertion fails, return/record a validation error (mirroring how invalid string values are already caught byvalidateAgentMemoryLimit) rather than silently ignoring it.