diff --git a/actions/setup/js/comment_memory_helpers.cjs b/actions/setup/js/comment_memory_helpers.cjs index 779bcf66079..e4ad0be8ddf 100644 --- a/actions/setup/js/comment_memory_helpers.cjs +++ b/actions/setup/js/comment_memory_helpers.cjs @@ -10,6 +10,8 @@ const COMMENT_MEMORY_EXTENSION = ".md"; const MAX_MEMORY_ID_LENGTH = 128; const COMMENT_MEMORY_MAX_SCAN_PAGES = 50; const COMMENT_MEMORY_MAX_SCAN_EMPTY_PAGES = 5; +const COMMENT_MEMORY_MAX_FILE_BYTES = 16 * 1024; +const COMMENT_MEMORY_MAX_TOTAL_BYTES = 48 * 1024; const COMMENT_MEMORY_PROMPT_START_MARKER = ""; const COMMENT_MEMORY_PROMPT_END_MARKER = ""; const COMMENT_MEMORY_CODE_FENCE = "``````"; @@ -147,6 +149,8 @@ module.exports = { COMMENT_MEMORY_EXTENSION, COMMENT_MEMORY_MAX_SCAN_PAGES, COMMENT_MEMORY_MAX_SCAN_EMPTY_PAGES, + COMMENT_MEMORY_MAX_FILE_BYTES, + COMMENT_MEMORY_MAX_TOTAL_BYTES, COMMENT_MEMORY_PROMPT_START_MARKER, COMMENT_MEMORY_PROMPT_END_MARKER, COMMENT_MEMORY_CODE_FENCE, diff --git a/actions/setup/js/setup_comment_memory_files.cjs b/actions/setup/js/setup_comment_memory_files.cjs index 40bbc060285..96505e0267f 100644 --- a/actions/setup/js/setup_comment_memory_files.cjs +++ b/actions/setup/js/setup_comment_memory_files.cjs @@ -11,6 +11,8 @@ const { COMMENT_MEMORY_DIR, COMMENT_MEMORY_MAX_SCAN_PAGES, COMMENT_MEMORY_MAX_SCAN_EMPTY_PAGES, + COMMENT_MEMORY_MAX_FILE_BYTES, + COMMENT_MEMORY_MAX_TOTAL_BYTES, COMMENT_MEMORY_PROMPT_START_MARKER, COMMENT_MEMORY_PROMPT_END_MARKER, extractCommentMemoryEntries, @@ -144,6 +146,19 @@ async function collectCommentMemoryFiles(githubClient, commentMemoryConfig) { } catch (err) { throw new Error(`Failed to create directory ${COMMENT_MEMORY_DIR}: ${String(err)}`, { cause: err }); } + let totalBytes = 0; + for (const [memoryId, content] of memoryMap.entries()) { + const writtenContent = `${content}\n`; + const contentBytes = Buffer.byteLength(writtenContent, "utf8"); + if (contentBytes > COMMENT_MEMORY_MAX_FILE_BYTES) { + throw new Error(`${ERR_VALIDATION}: comment_memory file '${memoryId}.md' is ${contentBytes} bytes, exceeding max ${COMMENT_MEMORY_MAX_FILE_BYTES} bytes`); + } + totalBytes += contentBytes; + if (totalBytes > COMMENT_MEMORY_MAX_TOTAL_BYTES) { + throw new Error(`${ERR_VALIDATION}: comment_memory total size ${totalBytes} bytes exceeds max ${COMMENT_MEMORY_MAX_TOTAL_BYTES} bytes`); + } + } + const writtenFiles = []; for (const [memoryId, content] of memoryMap.entries()) { const filePath = path.join(COMMENT_MEMORY_DIR, `${memoryId}.md`); diff --git a/actions/setup/js/setup_comment_memory_files.test.cjs b/actions/setup/js/setup_comment_memory_files.test.cjs index 5325d33117f..7ba49da9aff 100644 --- a/actions/setup/js/setup_comment_memory_files.test.cjs +++ b/actions/setup/js/setup_comment_memory_files.test.cjs @@ -220,4 +220,76 @@ describe("setup_comment_memory_files", () => { ); expect(global.core.warning).not.toHaveBeenCalledWith(expect.stringContaining("E004")); }); + + it("warns and skips writing when a memory entry exceeds per-file size cap", async () => { + fs.writeFileSync(CONFIG_PATH, JSON.stringify({ "comment-memory": { target: "triggering" } })); + const oversizedBody = `${"a".repeat(16 * 1024 + 1)}`; + global.github = { + rest: { + issues: { + listComments: vi.fn().mockResolvedValue({ + data: [{ body: `\`\`\`\`\`\`gh-aw-comment-memory:default\n${oversizedBody}\n\`\`\`\`\`\`\n` }], + }), + }, + }, + }; + + const module = await import("./setup_comment_memory_files.cjs"); + await module.main(); + + expect(fs.existsSync(path.join(COMMENT_MEMORY_DIR, "default.md"))).toBe(false); + expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("exceeding max 16384 bytes")); + }); + + it("counts the trailing newline when enforcing the per-file size cap", async () => { + // Content of exactly 16384 bytes passes the old check (16384 > 16384 is false) + // but the written file would be 16385 bytes (content + \n). The fix measures the + // written string, so 16385 > 16384 triggers the cap correctly. + fs.writeFileSync(CONFIG_PATH, JSON.stringify({ "comment-memory": { target: "triggering" } })); + const atCapBody = "a".repeat(16 * 1024); // 16384 bytes — exactly at the cap before \n + global.github = { + rest: { + issues: { + listComments: vi.fn().mockResolvedValue({ + data: [{ body: `\`\`\`\`\`\`gh-aw-comment-memory:default\n${atCapBody}\n\`\`\`\`\`\`\n` }], + }), + }, + }, + }; + + const module = await import("./setup_comment_memory_files.cjs"); + await module.main(); + + expect(fs.existsSync(path.join(COMMENT_MEMORY_DIR, "default.md"))).toBe(false); + expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("exceeding max 16384 bytes")); + }); + + it("warns and skips writing when total memory size exceeds cap", async () => { + fs.writeFileSync(CONFIG_PATH, JSON.stringify({ "comment-memory": { target: "triggering" } })); + const chunk = "a".repeat(12 * 1024 + 1); + global.github = { + rest: { + issues: { + listComments: vi.fn().mockResolvedValue({ + data: [ + { + body: + `\`\`\`\`\`\`gh-aw-comment-memory:one\n${chunk}\n\`\`\`\`\`\`\n` + + `\`\`\`\`\`\`gh-aw-comment-memory:two\n${chunk}\n\`\`\`\`\`\`\n` + + `\`\`\`\`\`\`gh-aw-comment-memory:three\n${chunk}\n\`\`\`\`\`\`\n` + + `\`\`\`\`\`\`gh-aw-comment-memory:four\n${chunk}\n\`\`\`\`\`\`\n`, + }, + ], + }), + }, + }, + }; + + const module = await import("./setup_comment_memory_files.cjs"); + await module.main(); + + expect(fs.existsSync(path.join(COMMENT_MEMORY_DIR, "one.md"))).toBe(false); + expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("total size")); + expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("exceeds max 49152 bytes")); + }); }); diff --git a/docs/adr/27325-organize-workflow-helpers-by-semantic-responsibility.md b/docs/adr/27325-organize-workflow-helpers-by-semantic-responsibility.md index da1fcbe23af..21ea6c8fea6 100644 --- a/docs/adr/27325-organize-workflow-helpers-by-semantic-responsibility.md +++ b/docs/adr/27325-organize-workflow-helpers-by-semantic-responsibility.md @@ -1,7 +1,7 @@ # ADR-27325: Organize pkg/workflow Helpers by Semantic Responsibility **Date**: 2026-04-20 -**Status**: Draft +**Status**: Accepted **Deciders**: pelikhan, Copilot --- @@ -46,6 +46,10 @@ A dedicated sub-package would enforce a hard import boundary and make the API ta - No public API surface changes; `GetCopilotAPITarget`, `GetGeminiAPITarget`, and `DefaultGeminiAPITarget` retain their signatures and are re-exported from the new file. - The refactor is behavior-preserving: empty-string filtering and type coercion semantics are maintained across all moved and renamed helpers. +### Norms + +- CI **MUST** enforce this file-organization boundary with `TestAWFHelpersDoesNotContainEngineAPITargetHelpers` in `pkg/workflow/engine_api_targets_file_organization_test.go`. + --- ## Part 2 — Normative Specification (RFC 2119) @@ -71,4 +75,4 @@ An implementation is considered conformant with this ADR if it satisfies all **M --- -*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24667638374) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* +*Accepted after implementation and conformance validation.* diff --git a/docs/adr/27327-parser-utility-test-strategy.md b/docs/adr/27327-parser-utility-test-strategy.md index 6ef41dc16b2..c01e8fa22f4 100644 --- a/docs/adr/27327-parser-utility-test-strategy.md +++ b/docs/adr/27327-parser-utility-test-strategy.md @@ -1,7 +1,7 @@ # ADR-27327: Test Strategy for Parser Package Utility Functions **Date**: 2026-04-20 -**Status**: Draft +**Status**: Accepted **Deciders**: pelikhan, Copilot --- @@ -41,6 +41,7 @@ Parser utilities could be tested exclusively at the workflow-loading integration #### Neutral - The renamed function `TestIsNotFoundError_RemoteNested` in `import_remote_nested_test.go` disambiguates the two test functions but changes the test name string reported in CI output and `go test -v` listings. - No production code is modified by this decision; all changes are confined to `_test.go` files. +- Conformance is implemented in `pkg/parser/frontmatter_utils_test.go` and `pkg/parser/import_remote_nested_test.go`. --- @@ -71,4 +72,4 @@ An implementation is considered conformant with this ADR if it satisfies all **M --- -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* +*Accepted after implementation and conformance validation.* diff --git a/docs/adr/27387-filter-non-frontmatter-markdown-during-compile-all.md b/docs/adr/27387-filter-non-frontmatter-markdown-during-compile-all.md index 7cdd8b83d0c..ff8adfd76f7 100644 --- a/docs/adr/27387-filter-non-frontmatter-markdown-during-compile-all.md +++ b/docs/adr/27387-filter-non-frontmatter-markdown-during-compile-all.md @@ -1,7 +1,7 @@ # ADR-27387: Filter Non-Frontmatter Markdown Files During compile-all Discovery **Date**: 2026-04-20 -**Status**: Draft +**Status**: Accepted **Deciders**: pelikhan, Copilot --- @@ -44,6 +44,7 @@ Workflow Markdown files could be required to follow a specific naming pattern (e #### Neutral - The first-line check is intentionally strict (`bytes.Equal(firstLine, []byte("---"))`); any leading whitespace or UTF-8 BOM before `---` will cause the file to be skipped. This is consistent with how YAML frontmatter is defined in the gh-aw spec. - Both compile pipelines now share a single filtering function, reducing future drift risk. +- Error propagation behavior is covered by `TestCompileAllWorkflowFiles/compile all propagates frontmatter filter I/O errors` in `pkg/cli/commands_file_watching_test.go`. --- @@ -75,4 +76,4 @@ An implementation is considered conformant with this ADR if it satisfies all **M --- -*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24679557357) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* +*Accepted after implementation and conformance validation.* diff --git a/docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md b/docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md index 1b994d6f538..354397bdfb9 100644 --- a/docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md +++ b/docs/adr/27476-extend-safe-outputs-dependencies-via-needs-field.md @@ -1,7 +1,7 @@ # ADR-27476: Extend `safe_outputs` Job Dependencies via `safe-outputs.needs` Field **Date**: 2026-04-21 -**Status**: Draft +**Status**: Accepted **Deciders**: pelikhan --- @@ -41,6 +41,16 @@ The compiler could parse expression strings inside `safe-outputs` config values - A new validation function (`validateSafeOutputsNeeds`) is added to the compiler pipeline, following the same structure as the existing `validateSafeJobNeeds` validator. - The schema change (`main_workflow_schema.json`) affects all schema-validation tooling and IDE integrations that consume the schema. +### Safeguards + +- Dependency cycles introduced through `safe-outputs.needs` are prevented by `JobManager.ValidateDependencies`, which validates the fully assembled job graph (including all `safe_outputs` edges) as a required compile-time validation gate in `pkg/workflow/compiler_yaml.go`. +- Negative validation coverage for reserved and unknown `safe-outputs.needs` targets is required and implemented in `pkg/workflow/safe_jobs_needs_validation_test.go`. + +### Norms + +- Changes to `safe-outputs.needs` semantics **MUST** update `pkg/parser/schemas/main_workflow_schema.json` and preserve `uniqueItems: true`. +- Any future extension of this field **MUST** include a schema version update and corresponding IDE/schema-consumer sync. + --- ## Part 2 — Normative Specification (RFC 2119) @@ -65,6 +75,11 @@ The compiler could parse expression strings inside `safe-outputs` config values 2. A `safe-outputs.needs` entry **MUST NOT** reference a job name that is not declared in the workflow's top-level `jobs:` map. 3. When a violation of rules 1 or 2 is detected, the compiler **MUST** emit a compile-time error and **MUST NOT** produce a compiled workflow artifact. +### Safeguards and Schema Sync + +1. `JobManager.ValidateDependencies` **MUST** run after `safe-outputs.needs` merge/validation so that the fully assembled job graph is checked for cycles before a compiled workflow artifact is emitted. +2. Any schema change to `safe-outputs.needs` semantics **MUST** update `pkg/parser/schemas/main_workflow_schema.json` and related IDE/schema-consumer documentation in the same release. + ### Import Merge Behavior 1. When a workflow imports another workflow, the importer's `safe-outputs.needs` **MUST** be merged with the imported workflow's `safe-outputs.needs` as a deduplicated union. @@ -76,4 +91,4 @@ An implementation is considered conformant with this ADR if it satisfies all **M --- -*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24701927396) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* +*Accepted after implementation and conformance validation.* diff --git a/docs/adr/27479-comment-memory-file-based-agent-memory-with-github-persistence.md b/docs/adr/27479-comment-memory-file-based-agent-memory-with-github-persistence.md index dde29715bde..aa8a718d268 100644 --- a/docs/adr/27479-comment-memory-file-based-agent-memory-with-github-persistence.md +++ b/docs/adr/27479-comment-memory-file-based-agent-memory-with-github-persistence.md @@ -1,7 +1,7 @@ # ADR-27479: Comment Memory — File-Based Agent Memory with GitHub Comment Persistence **Date**: 2026-04-21 -**Status**: Draft +**Status**: Accepted **Deciders**: pelikhan, copilot-swe-agent --- @@ -49,6 +49,16 @@ Memory could be stored in a dedicated GitHub issue (a "memory issue") or externa - Existing safe-output handler manager gains automatic file-based sync logic that runs unconditionally after each agent turn - The W3C-style safe outputs specification is updated to formally document the `comment_memory` type and end-to-end data flow +### Safeguards + +- Pre-agent comment-memory materialization enforces hard size caps: each memory file **MUST** be ≤16 KiB and the total materialized directory **MUST** be ≤48 KiB. +- If either limit is exceeded, setup **MUST** fail safely (warning + skip persistence setup) rather than materializing oversized memory into prompts. + +### Norms + +- The size-cap constants are source-of-truth values in `actions/setup/js/comment_memory_helpers.cjs` and **MUST** be kept aligned with the safe-outputs specification. +- Any change to these caps **MUST** update both implementation tests (`setup_comment_memory_files.test.cjs`) and the W3C-style safe-outputs spec. + --- ## Part 2 — Normative Specification (RFC 2119) @@ -62,6 +72,12 @@ Memory could be stored in a dedicated GitHub issue (a "memory issue") or externa 3. Memory files **MUST** be writable by the agent process at runtime. 4. Memory files **MUST NOT** include the XML marker tags or footer content — only the user-editable body of the managed block **SHALL** be materialized. +### Memory Size Limits + +1. Each materialized memory file **MUST NOT** exceed 16 KiB. +2. The total size of all materialized memory files under `/tmp/gh-aw/comment-memory/` **MUST NOT** exceed 48 KiB. +3. When either size limit is exceeded, setup **MUST** fail safely and **MUST NOT** materialize oversized memory into the agent prompt context. + ### Agent Interaction with Memory 1. Agents **MUST NOT** call a `comment_memory` safe-output tool to persist memory; memory persistence **SHALL** occur exclusively via the automatic file-sync mechanism. @@ -94,4 +110,4 @@ An implementation is considered conformant with this ADR if it satisfies all **M --- -*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24706352108) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* +*Accepted after implementation and conformance validation.* diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 27ce9b6d562..8c4f3678239 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -1678,6 +1678,7 @@ safe-outputs: ``` Use the single `safe-outputs.needs` field for all explicit custom dependencies. +The field is schema-defined in `pkg/parser/schemas/main_workflow_schema.json`; update local editor schema integrations when upgrading gh-aw schema versions. Validation rules: diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index b72b2c363f0..e5471b80e94 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -994,6 +994,7 @@ for (const op of issueOps) { When `tools.comment-memory` is enabled, implementations MUST support this additional data-flow path: 1. **GitHub comment → local files (pre-agent setup)**: A setup step reads the managed comment body from the target issue or pull request, extracts content between `` and ``, and writes one file per memory entry under `/tmp/gh-aw/comment-memory/.md`. + - Setup MUST enforce memory-size safeguards: each materialized file MUST be ≤16 KiB and the aggregate materialized directory MUST be ≤48 KiB. Exceeding either limit MUST emit a warning and skip writing all memory files, leaving the comment-memory directory empty. 2. **Local files → agent**: The prompt MUST include instructions that memory files are edited directly in `/tmp/gh-aw/comment-memory/`. 3. **Agent → artifact**: The unified agent artifact MUST include `/tmp/gh-aw/comment-memory/` when comment memory is enabled. 4. **Artifact → threat detection**: Threat-detection prompt setup MUST include discovered comment-memory files in analysis context. @@ -2514,6 +2515,7 @@ This section provides complete definitions for all remaining safe output types. - `memory_id` MUST be validated as `[A-Za-z0-9_-]+` with path traversal patterns rejected. - Managed comment scan MUST be bounded by a maximum page limit. +- Materialized comment-memory files MUST enforce a per-file cap of 16 KiB and an aggregate cap of 48 KiB during setup. - Body content MUST undergo sanitization and comment size/mention/link limit validation before upsert. - Cross-repository targets MUST be validated against `allowed-repos`. - Only content within managed marker tags is treated as editable memory; footer/provenance text MUST NOT be imported into editable files. For example, in `MEMORY\n\n`, only `MEMORY` is editable/imported. diff --git a/pkg/cli/commands_file_watching_test.go b/pkg/cli/commands_file_watching_test.go index ce631b3c94f..d7164b19d80 100644 --- a/pkg/cli/commands_file_watching_test.go +++ b/pkg/cli/commands_file_watching_test.go @@ -225,6 +225,32 @@ func TestCompileAllWorkflowFiles(t *testing.T) { assert.NoFileExists(t, docsLockFile, "Should not emit lock file for documentation markdown without frontmatter") }) + t.Run("compile all propagates frontmatter filter I/O errors", func(t *testing.T) { + tempDir := testutil.TempDir(t, "test-*") + workflowsDir := filepath.Join(tempDir, ".github/workflows") + err := os.MkdirAll(workflowsDir, 0o755) + require.NoError(t, err) + + validWorkflow := filepath.Join(workflowsDir, "valid.md") + validContent := "---\non: push\nengine: claude\n---\n# Valid Workflow\n\nContent" + err = os.WriteFile(validWorkflow, []byte(validContent), 0o644) + require.NoError(t, err) + + // Create a directory named broken.md — os.Open succeeds but a subsequent Read + // call returns an EISDIR error ("is a directory"), exercising the + // frontmatter-filter I/O propagation path without relying on process privileges + // or file-permission portability. + brokenWorkflow := filepath.Join(workflowsDir, "broken.md") + err = os.MkdirAll(brokenWorkflow, 0o755) + require.NoError(t, err) + + compiler := workflow.NewCompiler() + _, err = compileAllWorkflowFiles(context.Background(), compiler, workflowsDir, false) + require.Error(t, err) + require.ErrorContains(t, err, "failed to filter markdown files") + require.ErrorContains(t, err, "failed to read workflow file") + }) + t.Run("compile all handles glob error", func(t *testing.T) { // Use a malformed glob pattern that will cause filepath.Glob to error invalidDir := "/tmp/gh-aw/[invalid" diff --git a/pkg/workflow/engine_api_targets_file_organization_test.go b/pkg/workflow/engine_api_targets_file_organization_test.go new file mode 100644 index 00000000000..b66daf79bc8 --- /dev/null +++ b/pkg/workflow/engine_api_targets_file_organization_test.go @@ -0,0 +1,34 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestAWFHelpersDoesNotContainEngineAPITargetHelpers(t *testing.T) { + content, err := os.ReadFile(filepath.Join("awf_helpers.go")) + require.NoError(t, err) + + forbiddenFunctionSignatures := []string{ + "func extractAPITargetHost(", + "func extractAPIBasePath(", + "func extractAPITargetAuthHeader(", + "func GetCopilotAPITarget(", + "func extractLiteralEngineEnvHost(", + "func GetCopilotAllowlistTargets(", + "func GetAntigravityAPITarget(", + "func GetGeminiAPITarget(", + "func getEngineAPIHosts(", + "const DefaultAntigravityAPITarget", + "const DefaultGeminiAPITarget", + } + + for _, signature := range forbiddenFunctionSignatures { + require.NotContains(t, string(content), signature, "awf_helpers.go must not define %s", signature) + } +}