Expose memory stores to on.steps in pre-activation for deterministic dispatch gating#44015
on.steps in pre-activation for deterministic dispatch gating#44015Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
on.steps in pre-activation for deterministic dispatch gating
🤖 PR Triage · Run §28868091227
Score breakdown: Impact 15 · Urgency 10 · Quality 13 Rationale: Draft. Adds memory-store hydration to Labels:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the workflow compiler so that workflows using on.steps can access previously persisted memory state during the pre_activation job, enabling deterministic dispatch gating without needing an LLM turn to reconstruct state.
Changes:
- Adds pre-activation memory hydration via a new
buildPreActivationMemoryRestoreStepshook that runs beforeinjectPreActivationOnSteps. - Extends
on_stepstests to validate memory restore steps are present and ordered before theon.stepsgate, and that explicit write-back steps are absent. - Updates a generated workflow lock file to reflect the new pre-activation repo-memory clone step.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_pre_activation_job.go | Adds pre-activation memory restore step generation before on.steps injection. |
| pkg/workflow/on_steps_test.go | Adds coverage asserting memory restore steps appear in pre_activation before the on.steps gate. |
| .github/workflows/daily-cli-performance.lock.yml | Regenerates lock output to include the new pre-activation repo-memory restore step. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| var memorySteps strings.Builder | ||
|
|
||
| generateCacheMemorySteps(&memorySteps, data) | ||
| generateRepoMemorySteps(&memorySteps, data) |
| // 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") |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (111 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
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test Quality Sentinel: 90/100. 0% implementation tests (threshold: 30%). One new design_test with behavioral_contract coverage; minor: 4 assert calls lack failure-context messages; inflation ratio 2.47:1 (expected for compiler integration tests). No guideline violations.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on one correctness issue (cache write-back) and one test-coverage gap.
📋 Key Themes & Highlights
Key Themes
- Cache write-back risk (correctness):
generateCacheMemoryStepscan emitactions/cache(notactions/cache/restore) whenRestoreOnlyis false and threat detection is disabled.actions/cacheauto-saves via its post-action, meaning a workflow with plaincache-memory: truewill silently write back the cache from pre-activation — contradicting the PR's read-only guarantee. - Test coverage gap (
/tdd): TheNotContainsassertions check for post-agent step names ("Commit cache-memory changes","Push repo-memory changes") but do not verify which cache action is used. The real write-back mechanism (actions/cacheauto-save) would go undetected by the current tests. - Section extractor fragility (minor): The YAML section parser relies on an indentation heuristic to bound the
pre_activationblock; unusual YAML formatting could cause it to include sibling job content and produce false-positives.
Positive Highlights
- ✅ Clean, well-scoped extension point — hydration is gated behind
len(data.OnSteps) == 0so workflows withouton.stepsare unaffected. - ✅ Good test structure: arranges full workflow frontmatter, compiles, reads lock file, and asserts ordering constraints.
- ✅
generateRepoMemoryStepsis genuinely read-only (clone only, no push step), so the repo-memory part of the contract holds. - ✅ The comment-memory path is hand-rolled in the function itself and correctly omits any write-back.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 97.3 AIC · ⌖ 5.66 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| var memorySteps strings.Builder | ||
|
|
||
| generateCacheMemorySteps(&memorySteps, data) |
There was a problem hiding this comment.
[/codebase-design] generateCacheMemorySteps is not purely read-only — when cache.RestoreOnly is false and threat detection is disabled, it emits actions/cache (not actions/cache/restore), which auto-saves via its post-action. This violates the stated read-only contract for pre-activation.
💡 Suggested fix
Force restore-only when calling from the pre-activation context. Options:
- Add a dedicated
generateCacheMemoryRestoreOnlyStepsthat always usesactions/cache/restore. - Temporarily override each cache entry with
RestoreOnly: truebefore delegating togenerateCacheMemorySteps.
The test should also assert that actions/cache/restore (not actions/cache) appears in the pre_activation section to catch a regression here.
@copilot please address this.
| 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") |
There was a problem hiding this comment.
[/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.
| lockContentStr := string(lockContent) | ||
| var preActivationSectionBuilder strings.Builder | ||
| inPreActivation := false | ||
| for line := range strings.SplitSeq(lockContentStr, "\n") { |
There was a problem hiding this comment.
[/tdd] The pre_activation section extractor parses YAML by indentation heuristic rather than by a known job terminator. The termination condition (HasPrefix(" ") && !HasPrefix(" ") && HasSuffix(":")) will silently miss the end of the section if the next sibling job has a name that doesn't end in : at exactly 2-space indentation (e.g. if YAML comment lines or flow-style keys appear between jobs). This fragility could cause the section to grow beyond pre_activation and produce false-positive assertions.
💡 Consider a more robust extractor
Use a known sibling job name to bound the search, or parse the full YAML with gopkg.in/yaml.v3 and extract the jobs map directly. A simpler short-term fix is to assert on the first occurrence of a known post-pre-activation job ID (e.g. agent: or push_repo_memory:) as the terminator.
@copilot please address this.
| memorySteps.WriteString(" await main();\n") | ||
| } | ||
|
|
||
| if memorySteps.Len() > 0 { |
There was a problem hiding this comment.
[/codebase-design] All three memory types are accumulated into a single strings.Builder and then appended as one YAML block. If generateCacheMemorySteps or generateRepoMemorySteps emits content but the comment-memory block does not, the final append fires correctly — but if only comment-memory is configured (no cache/repo), this function silently skips even the comment-memory step because the Len() > 0 guard only fires if something was written. Wait — that is actually fine since comment-memory is written directly before the guard. But the design couples three independent memory types through a single builder with no per-type gating, making it harder to conditionally suppress one type later. Minor design note, not blocking.
💡 Suggestion
Consider appending each memory type as a separate steps element (consistent with how other buildPreActivation* helpers work), so each is independently optional and individually traceable in the step list.
@copilot please address this.
There was a problem hiding this comment.
Review: Expose memory stores to on.steps in pre-activation
The goal and structure are sound — restoring memory state before on.steps gates run is the right design. Three issues need attention before merge:
🔴 Blocking – cache is not actually read-only in pre-activation
buildPreActivationMemoryRestoreSteps delegates to generateCacheMemorySteps (the same function used for the agent job). That function emits actions/cache (auto-save on job exit) instead of actions/cache/restore when cache.RestoreOnly is false and threat-detection is disabled — i.e., the default configuration for most workflows. The post-action hook will silently write the (unmodified) cache back at the end of every pre-activation job, wasting quota and risking cache corruption if the pre-activation job exits mid-run. See inline comment on line 243.
🟡 Non-blocking – steps appended as a single blob violates the slice convention
All other helpers in the file append individual YAML lines as separate slice elements. The new function packs all memory steps into one strings.Builder and appends a single string, making len(job.Steps) misleading. See inline comment on line 259.
🟡 Non-blocking – test does not assert the cache action variant
The new test checks write-back step names are absent but does not assert that actions/cache/restore (not actions/cache) is used, so it would not catch the bug above. See inline comment on on_steps_test.go:304.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 151.9 AIC · ⌖ 6.31 AIC · ⊞ 4.8K
|
|
||
| var memorySteps strings.Builder | ||
|
|
||
| generateCacheMemorySteps(&memorySteps, data) |
There was a problem hiding this comment.
Bug: generateCacheMemorySteps may emit actions/cache (not actions/cache/restore) in pre-activation, breaking the read-only guarantee.
generateCacheMemorySteps selects between actions/cache and actions/cache/restore based on cache.RestoreOnly || IsDetectionJobEnabled(data.SafeOutputs) (see cache.go:568-569). When neither flag is set (the common case), it emits actions/cache, whose post-action hook auto-saves the cache at job end. Calling this function from buildPreActivationMemoryRestoreSteps therefore silently writes back the cache at the end of every pre_activation run — directly contradicting the comment on line 235 ("This is a read-only surface: it restores/loads memory data but does not emit write-back or commit steps").
This can corrupt the cache for the actual agent job (a pre-activation run that fails mid-step still saves a partial/stale cache) and wastes cache quota.
Suggested fix: Set cache.RestoreOnly = true on a shallow copy of each CacheMemoryEntry before delegating to generateCacheMemorySteps, or extract a dedicated generateCacheMemoryRestoreOnlySteps helper that always passes actions/cache/restore.
@copilot please address this.
| } | ||
|
|
||
| if memorySteps.Len() > 0 { | ||
| steps = append(steps, memorySteps.String()) |
There was a problem hiding this comment.
Maintainability: entire multi-step content appended as one slice element, inconsistent with the rest of the codebase.
All other step-building helpers in this file (e.g., appendPreActivationSkipRolesStep, appendPreActivationSkipIfMatchStep) append each YAML line or logical step as an individual string entry in the []string slice. Here, all three memory sources are flushed into a single strings.Builder and appended as one large blob (line 259). The renderer at jobs.go:391-394 iterates over elements and writes each one verbatim, so the output is correct, but the deviation from convention makes the slice element count misleading (e.g., len(job.Steps) understates the actual step count logged in jobs.go:207).
Suggested fix: Follow the existing pattern — call helpers directly on the steps slice (or append each resulting block separately). This keeps len(job.Steps) accurate and makes ordering logic transparent.
@copilot please address this.
| 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") |
There was a problem hiding this comment.
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.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed unresolved review feedback in commits
Reran checks locally on the updated branch: Note: CI for the new head commit still needs a maintainer-triggered rerun. |
|
@copilot please run the
|
…-stores-on-steps Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. I ran Local checks passed: CI on this new head did not auto-rerun from agent pushes, so a maintainer-triggered rerun is still required. |
on.stepsexecutes inpre_activation, but memory stores were only restored in the agent job. This prevented deterministic pre-activation filters from using prior memory state without spending an LLM turn.Pre-activation memory hydration
buildPreActivationMemoryRestoreStepsand invoked it beforeinjectPreActivationOnSteps.cache-memory(restore/setup path),repo-memory(clone path),comment-memory(materialized files viasetup_comment_memory_files.cjs).on.steps.Read-only behavior in pre-activation
pre_activation.Coverage for memory-aware pre-activation dispatch
on_stepstest case that validates:pre_activation,on.stepsgate step,pre_activation.