diff --git a/.github/workflows/daily-cli-performance.lock.yml b/.github/workflows/daily-cli-performance.lock.yml index 792cfd0d305..305a8bbae82 100644 --- a/.github/workflows/daily-cli-performance.lock.yml +++ b/.github/workflows/daily-cli-performance.lock.yml @@ -1861,6 +1861,16 @@ jobs: setupGlobals(core, github, context, exec, io, getOctokit); const { main } = require('${{ runner.temp }}/gh-aw/actions/check_membership.cjs'); await main(); + # Repo memory git-based storage configuration from frontmatter processed below + - name: Clone repo-memory branch (default) + env: + GH_TOKEN: ${{ github.token }} + GITHUB_SERVER_URL: ${{ github.server_url }} + BRANCH_NAME: memory/cli-performance + TARGET_REPO: ${{ github.repository }} + MEMORY_DIR: /tmp/gh-aw/repo-memory/default + CREATE_ORPHAN: true + run: bash "${RUNNER_TEMP}/gh-aw/actions/clone_repo_memory_branch.sh" - name: Detect recent compilation-related changes id: changes uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 diff --git a/docs/adr/44015-expose-memory-stores-to-on-steps-pre-activation.md b/docs/adr/44015-expose-memory-stores-to-on-steps-pre-activation.md new file mode 100644 index 00000000000..9b45fde9d11 --- /dev/null +++ b/docs/adr/44015-expose-memory-stores-to-on-steps-pre-activation.md @@ -0,0 +1,48 @@ +# ADR-44015: Expose Memory Stores to `on.steps` in Pre-Activation + +**Date**: 2026-07-07 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The gh-aw workflow engine compiles workflows into a `pre_activation` job and an agent job. Custom dispatch-gating steps defined via `on.steps` execute in `pre_activation`, but all three memory stores (cache-memory, repo-memory, comment-memory) were previously only restored in the agent job. This meant that any `on.steps` logic requiring prior memory state — for example, a gate step that reads a cached value to decide whether to proceed — either had to forgo memory access or consume an additional LLM turn in the agent phase to hydrate state. The inability to access memory in pre-activation limited the expressiveness of deterministic, non-LLM dispatch gates. + +### Decision + +We will restore all configured memory stores (cache-memory, repo-memory, comment-memory) in the pre-activation job before `on.steps` gates execute. The restore is **read-only**: it reuses the existing restore/load paths for each memory type and does not add cache-commit or repo-push write-back steps to pre-activation. Hydration is gated on the presence of `on.steps` in the workflow definition so that workflows without custom steps are unaffected. + +### Alternatives Considered + +#### Alternative 1: Execute `on.steps` in the Agent Job Instead of Pre-Activation + +Move the `on.steps` execution point from `pre_activation` to the agent job, where memory is already available. + +This would avoid the need to restore memory in a second location, but it would break the architectural contract that `on.steps` runs before the LLM is invoked. The entire purpose of `on.steps` is to gate activation deterministically and cheaply; running it in the agent job would consume a full LLM turn even when the gate rejects the request, negating the cost benefit. + +#### Alternative 2: Require Workflows to Declare Their Own Memory Restoration Steps + +Leave pre-activation unchanged and let workflow authors manually add memory restore steps before their `on.steps` gates via explicit YAML. + +This avoids centralizing the restore logic in the compiler, but it places the burden on every workflow author to know the correct restore incantation for each memory type. It also creates drift risk: if the restore steps change, all affected workflows must be updated. The compiler already owns the pattern for generating memory steps; extending it is the consistent approach. + +### Consequences + +#### Positive +- `on.steps` gates can now read prior memory state to make deterministic dispatch decisions without spending an LLM turn. +- Reusing the existing restore/load paths ensures memory-type-specific details (branch names, cache keys, token handling) remain in a single authoritative location. +- The read-only constraint prevents pre-activation from accidentally mutating memory, preserving the invariant that only the agent job writes back. + +#### Negative +- Every pre-activation run for a workflow with `on.steps` now executes additional restore steps, adding latency even when the gate does not use memory. +- The pre-activation job compiler gains a new code path (`buildPreActivationMemoryRestoreSteps`), increasing the surface area that must be maintained as memory store implementations evolve. + +#### Neutral +- The change is scoped to workflows that define `on.steps`; workflows without `on.steps` are compiled identically to before. +- The `.github/workflows/daily-cli-performance.lock.yml` lock file is regenerated as a side effect of the compiler change, reflecting the new restore step. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/workflow/compiler_pre_activation_job.go b/pkg/workflow/compiler_pre_activation_job.go index e40c0d14e05..09a2ab127b1 100644 --- a/pkg/workflow/compiler_pre_activation_job.go +++ b/pkg/workflow/compiler_pre_activation_job.go @@ -47,6 +47,7 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec steps = c.buildPreActivationSkipIfQuerySteps(data, steps, skipIfToken) steps = c.buildPreActivationSkipIfCheckFailingStep(data, steps) steps = c.buildPreActivationRolesBotsCmdSteps(data, steps) + steps = c.buildPreActivationMemoryRestoreSteps(data, steps) steps, onStepIDs, err := c.injectPreActivationOnSteps(data, steps, customSteps) if err != nil { return nil, err @@ -230,6 +231,75 @@ func (c *Compiler) buildPreActivationRolesBotsCmdSteps(data *WorkflowData, steps return steps } +// buildPreActivationMemoryRestoreSteps restores memory stores before on.steps run in pre-activation. +// This is a read-only surface: it restores/loads memory data but does not emit write-back or commit steps. +func (c *Compiler) buildPreActivationMemoryRestoreSteps(data *WorkflowData, steps []string) []string { + if len(data.OnSteps) == 0 { + return steps + } + + var cacheMemorySteps strings.Builder + generatePreActivationCacheMemoryRestoreSteps(&cacheMemorySteps, data) + if cacheMemorySteps.Len() > 0 { + steps = append(steps, cacheMemorySteps.String()) + } + + var repoMemorySteps strings.Builder + generateRepoMemorySteps(&repoMemorySteps, data) + if repoMemorySteps.Len() > 0 { + steps = append(steps, repoMemorySteps.String()) + } + + if data.SafeOutputs != nil && data.SafeOutputs.CommentMemory != nil { + var commentMemorySteps strings.Builder + commentMemorySteps.WriteString(" - name: Prepare comment memory files\n") + fmt.Fprintf(&commentMemorySteps, " uses: %s\n", getCachedActionPin("actions/github-script", data)) + commentMemorySteps.WriteString(" with:\n") + fmt.Fprintf(&commentMemorySteps, " github-token: %s\n", getEffectiveSafeOutputGitHubToken(data.SafeOutputs.CommentMemory.GitHubToken)) + commentMemorySteps.WriteString(" script: |\n") + commentMemorySteps.WriteString(" const { setupGlobals } = require('${{ runner.temp }}/gh-aw/actions/setup_globals.cjs');\n") + commentMemorySteps.WriteString(" setupGlobals(core, github, context, exec, io, getOctokit);\n") + commentMemorySteps.WriteString(" const { main } = require('${{ runner.temp }}/gh-aw/actions/setup_comment_memory_files.cjs');\n") + commentMemorySteps.WriteString(" await main();\n") + steps = append(steps, commentMemorySteps.String()) + } + + return steps +} + +func generatePreActivationCacheMemoryRestoreSteps(builder *strings.Builder, data *WorkflowData) { + if data.CacheMemoryConfig == nil || len(data.CacheMemoryConfig.Caches) == 0 { + return + } + + preActivationData := &WorkflowData{ + ParsedTools: data.ParsedTools, + SafeOutputs: data.SafeOutputs, + CacheMemoryConfig: &CacheMemoryConfig{ + Caches: make([]CacheMemoryEntry, len(data.CacheMemoryConfig.Caches)), + }, + } + for i, cache := range data.CacheMemoryConfig.Caches { + cacheCopy := CacheMemoryEntry{ + ID: cache.ID, + Key: cache.Key, + Description: cache.Description, + RestoreOnly: true, + Scope: cache.Scope, + } + if cache.AllowedExtensions != nil { + cacheCopy.AllowedExtensions = append([]string(nil), cache.AllowedExtensions...) + } + if cache.RetentionDays != nil { + retentionDays := *cache.RetentionDays + cacheCopy.RetentionDays = &retentionDays + } + preActivationData.CacheMemoryConfig.Caches[i] = cacheCopy + } + + generateCacheMemorySteps(builder, preActivationData) +} + func (c *Compiler) appendPreActivationSkipRolesStep(data *WorkflowData, steps []string) []string { steps = append(steps, " - name: Check skip-roles\n") steps = append(steps, fmt.Sprintf(" id: %s\n", constants.CheckSkipRolesStepID)) diff --git a/pkg/workflow/on_steps_test.go b/pkg/workflow/on_steps_test.go index 482eb2b9992..345144e73cd 100644 --- a/pkg/workflow/on_steps_test.go +++ b/pkg/workflow/on_steps_test.go @@ -5,6 +5,7 @@ package workflow import ( "os" "path/filepath" + "regexp" "strings" "testing" @@ -225,6 +226,90 @@ Test on.steps are appended after built-in checks t.Error("Expected membership check step to appear before on.steps gate step in pre_activation job") } }) + + t.Run("on_steps_with_memory_stores_restored_in_pre_activation", func(t *testing.T) { + workflowContent := `--- +on: + workflow_dispatch: null + roles: all + steps: + - name: Gate check + id: gate + run: echo "checking..." +tools: + cache-memory: true + repo-memory: true + comment-memory: true +engine: copilot +--- + +Test on.steps with memory stores restored in pre-activation +` + workflowFile := filepath.Join(tmpDir, "test-on-steps-memory-pre-activation.md") + if err := os.WriteFile(workflowFile, []byte(workflowContent), 0644); err != nil { + t.Fatal(err) + } + + err := compiler.CompileWorkflow(workflowFile) + if err != nil { + t.Fatalf("CompileWorkflow() returned error: %v", err) + } + + lockFile := stringutil.MarkdownToLockFile(workflowFile) + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockContentStr := string(lockContent) + preActivationStart := strings.Index(lockContentStr, "\n pre_activation:\n") + if preActivationStart == -1 { + preActivationStart = strings.Index(lockContentStr, " pre_activation:\n") + } + if preActivationStart == -1 { + t.Fatalf("Expected pre_activation section in lock file. Lock file:\n%s", lockContentStr) + } + + sectionBodyStart := preActivationStart + if strings.HasPrefix(lockContentStr[preActivationStart:], "\n") { + sectionBodyStart++ + } + jobStartPattern := regexp.MustCompile(`\n [A-Za-z0-9_-]+:\n`) + searchStart := sectionBodyStart + len(" pre_activation:\n") + nextJobLoc := jobStartPattern.FindStringIndex(lockContentStr[searchStart:]) + var preActivationSection string + if nextJobLoc == nil { + preActivationSection = lockContentStr[sectionBodyStart:] + } else { + preActivationSection = lockContentStr[sectionBodyStart : searchStart+nextJobLoc[0]+1] + } + if preActivationSection == "" { + t.Fatalf("Expected pre_activation section in lock file. Lock file:\n%s", lockContentStr) + } + + // Memory stores should be restored before on.steps run in pre-activation. + assert.Contains(t, preActivationSection, "Restore cache-memory file share data") + assert.Contains(t, preActivationSection, "Clone repo-memory branch (default)") + assert.Contains(t, preActivationSection, "Prepare comment memory files") + assert.Contains(t, preActivationSection, " id: gate") + + cacheRestoreIdx := strings.Index(preActivationSection, "Restore cache-memory file share data") + repoRestoreIdx := strings.Index(preActivationSection, "Clone repo-memory branch (default)") + commentMemoryIdx := strings.Index(preActivationSection, "Prepare comment memory files") + gateStepIdx := strings.Index(preActivationSection, " id: gate") + if cacheRestoreIdx == -1 || repoRestoreIdx == -1 || commentMemoryIdx == -1 || gateStepIdx == -1 { + t.Fatalf("Expected memory restore steps and gate step in pre_activation section. Section:\n%s", preActivationSection) + } + assert.Less(t, cacheRestoreIdx, gateStepIdx, "cache-memory should be restored before on.steps") + assert.Less(t, repoRestoreIdx, gateStepIdx, "repo-memory should be restored before on.steps") + assert.Less(t, commentMemoryIdx, gateStepIdx, "comment-memory should be restored before on.steps") + + // Pre-activation must not include write-back/commit steps. + assert.NotContains(t, preActivationSection, "Commit cache-memory changes") + assert.NotContains(t, preActivationSection, "Push repo-memory changes") + assert.Contains(t, preActivationSection, "uses: "+getActionPin("actions/cache/restore")) + assert.NotContains(t, preActivationSection, "uses: actions/cache@") + assert.NotContains(t, preActivationSection, "uses: "+getActionPin("actions/cache")) + }) } // TestExtractOnSteps tests the extractOnSteps function directly