Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/workflows/daily-cli-performance.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions docs/adr/44015-expose-memory-stores-to-on-steps-pre-activation.md
Original file line number Diff line number Diff line change
@@ -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.*
70 changes: 70 additions & 0 deletions pkg/workflow/compiler_pre_activation_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
Expand Down
85 changes: 85 additions & 0 deletions pkg/workflow/on_steps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package workflow
import (
"os"
"path/filepath"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -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")
Comment on lines +289 to +293

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The write-back absence checks (NotContains) are named after the post-agent job steps ("Commit cache-memory changes", "Push repo-memory changes") — but the real silent write risk in pre-activation is actions/cache (not actions/cache/restore) auto-saving on its post-action. These two step-name checks would pass even if the wrong cache action were used.

💡 Add the missing assertion
// Verify cache action is restore-only (not auto-saving)
assert.NotContains(t, preActivationSection, "actions/cache@",
    "pre_activation should use actions/cache/restore, not actions/cache")
assert.Contains(t, preActivationSection, "actions/cache/restore@",
    "pre_activation cache step should be restore-only")

This directly tests the read-only property the PR description promises.

@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage: no case exercises the actions/cache vs actions/cache/restore selection in pre-activation.

The test asserts that write-back steps (Commit cache-memory changes, Push repo-memory changes) are absent from pre_activation, but it does not assert that the cache restore step uses actions/cache/restore (and not the auto-saving actions/cache). This gap means the bug described on line 243 would not be caught by this test suite.

Suggested addition: add an assertion like:

assert.Contains(t, preActivationSection, "actions/cache/restore")
assert.NotContains(t, preActivationSection, "uses: actions/cache@")

(or the pinned variants returned by getActionPin). This would concretely enforce the read-only cache contract.

@copilot please address this.

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
Expand Down