Move comment-memory preparation before user steps: in agent job#43952
Conversation
- Add `generateActivationArtifactAndCommentMemorySteps` to emit the
activation artifact download, a minimal comment-memory config write,
and the "Prepare comment memory files" step BEFORE `emitCustomSteps`
in Phase 2 (`generateRuntimeAndWorkspaceSetupSteps`)
- Add `generateCommentMemoryEarlyConfigStep` to write a minimal
`${RUNNER_TEMP}/gh-aw/safeoutputs/config.json` at compile time so
`setup_comment_memory_files.cjs` can read comment_memory config before
the full MCP-setup "Generate Safe Outputs Config" step runs
- Remove activation artifact download and comment-memory step from Phase 3
(`generateEngineInstallAndPreAgentSteps`) — they now run earlier
- Add `TestCommentMemoryBeforeCustomSteps` ordering test
- Update `TestCheckoutRuntimeOrderInCustomSteps` for new 9-step ordering
- Regenerate all 258 workflow `.lock.yml` golden files and wasm golden files
Fixes: comment-memory files were never populated from prior history because
`loadSafeOutputsConfig()` read an empty config (the config file wasn't
written yet when the step ran in Phase 3).
Closes #43944
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
steps: in agent job
🤖 PR Triage — §28848998303
📝 DRAFT/WIP — Move comment-memory preparation before user steps in agent job. Checklist incomplete (0 files committed yet). Await completion before triage.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR fixes comment-memory initialization in compiled agent jobs by moving the activation artifact download + comment-memory config/file preparation earlier in the agent job—specifically before any user-provided steps:—so /tmp/gh-aw/comment-memory/*.md can be populated from prior comment history deterministically.
Changes:
- Adds a Phase 2 emission path in the YAML main-job generator to (1) download the activation artifact and, when comment-memory is enabled, (2) write an early minimal safe-outputs config and (3) run the
setup_comment_memory_files.cjspreparation step before user custom steps. - Removes/reorders the previous activation artifact download placement so downstream workflows/goldens reflect the new ordering.
- Updates/extends ordering tests and regenerates workflow lockfiles + wasm golden outputs to match the new step sequence.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml_main_job.go | Reorders activation artifact download and comment-memory setup to run before user steps:; adds early comment-memory config writer. |
| pkg/workflow/compiler_presteps_test.go | Adds/updates tests asserting comment-memory setup steps occur before custom steps. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Golden update reflecting earlier activation artifact download step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Golden update reflecting earlier activation artifact download step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Golden update reflecting earlier activation artifact download step. |
| .github/workflows/*.lock.yml | Regenerated lockfiles reflecting the updated agent-job step ordering (activation artifact download moved earlier). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 270/270 changed files
- Comments generated: 1
- Review effort level: Low
| func (c *Compiler) generateCommentMemoryEarlyConfigStep(yaml *strings.Builder, data *WorkflowData) { | ||
| builder := handlerRegistry["comment_memory"] | ||
| if builder == nil { | ||
| return | ||
| } | ||
| cfg := builder(data.SafeOutputs) | ||
| if cfg == nil { | ||
| return | ||
| } | ||
| configMap := map[string]any{"comment_memory": cfg} | ||
| jsonBytes, err := json.Marshal(configMap) | ||
| if err != nil { | ||
| compilerYamlLog.Printf("Warning: failed to marshal comment-memory config: %v", err) | ||
| return | ||
| } | ||
| configJSON := string(jsonBytes) | ||
| delimiter := GenerateHeredocDelimiterFromContent("COMMENT_MEMORY_CONFIG", configJSON) | ||
| yaml.WriteString(" - name: Write comment-memory configuration\n") | ||
| yaml.WriteString(" run: |\n") | ||
| yaml.WriteString(" mkdir -p \"${RUNNER_TEMP}/gh-aw/safeoutputs\"\n") | ||
| fmt.Fprintf(yaml, " cat > \"${RUNNER_TEMP}/gh-aw/safeoutputs/config.json\" << '%s'\n", delimiter) | ||
| fmt.Fprintf(yaml, " %s\n", configJSON) | ||
| fmt.Fprintf(yaml, " %s\n", delimiter) | ||
| } |
![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: 92/100 — Excellent. 0% implementation tests (threshold: 30%).
Both tests are pure design tests verifying behavioral contracts for step ordering:
- TestCheckoutRuntimeOrder: Updated to verify new 9-step sequence with comment-memory preparation
- TestCommentMemoryBeforeCustomSteps: New test ensuring comment-memory setup precedes custom steps
No violations: proper build tags, no mock libraries, all assertions include descriptive messages. Test inflation reasonable (1.7:1 ratio).
… user steps Captures the architectural decision to move the activation artifact download and comment-memory file preparation to Phase 2 of the workflow compiler, fixing the empty-config race that prevented comment-memory files from being populated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (251 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
|
There was a problem hiding this comment.
Two correctness issues require fixes before merge
The core ordering logic is sound and the root cause fix is correct. Two issues need addressing before this is safe to ship.
### Blocking issues (2)
1. TestCommentMemoryBeforeCustomSteps does not assert configIdx < prepareIdx (compiler_presteps_test.go:362)
The test verifies all three comment-memory steps appear before the user custom step, but never checks their mutual ordering. A regression that swaps Write comment-memory configuration and Prepare comment memory files would pass all existing assertions while reproducing the original bug (prepare runs before config is written → resolveCommentMemoryConfig({}) → null → silent no-op).
2. generateCommentMemoryEarlyConfigStep returns silently without gating the prepare step (compiler_yaml_main_job.go:399)
If the function returns early (nil registry entry, nil builder output, or marshal error), Prepare comment memory files is still unconditionally emitted and will run against a missing config.json → silent no-op. This is the exact failure mode this PR was written to fix. The fix: return a bool from generateCommentMemoryEarlyConfigStep and gate the prepare step on it.
### Non-blocking observation
Undocumented divergence between early config path and full MCP config generation (compiler_yaml_main_job.go:426) — the early write is intentionally minimal and correct today, but both paths write to the same config.json with no documented scope constraint. A brief comment would prevent future drift.
🔎 Code quality review by PR Code Quality Reviewer · 271.5 AIC · ⌖ 5.64 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
|
|
||
| t.Logf("Step order verified: download(%d), config(%d), prepare(%d) all < custom(%d)", | ||
| downloadIdx, configIdx, prepareIdx, customIdx) |
There was a problem hiding this comment.
Test is missing the critical intra-step ordering assertion — configIdx < prepareIdx is never checked, so a regression that swaps the config-write and prepare steps would not be caught.
💡 Why this matters and suggested fix
The entire correctness guarantee of this PR is that Write comment-memory configuration runs before Prepare comment memory files, because the JS script reads the config file written by the prior step. The test verifies that each step appears before the custom user step, but never asserts their mutual ordering.
If generateCommentMemoryEarlyConfigStep were accidentally called after the prepare step was emitted, the test would still pass — the prepare step would silently call resolveCommentMemoryConfig({}) → null → no-op, reproducing the original bug this PR is fixing.
Add the missing assertions after the existing ordering checks:
if downloadIdx >= configIdx {
t.Errorf("Activation artifact download (%d) must come before config write (%d)", downloadIdx, configIdx)
}
if configIdx >= prepareIdx {
t.Errorf("Config write (%d) must come before prepare step (%d)", configIdx, prepareIdx)
}| // Write a minimal comment-memory config so setup_comment_memory_files.cjs can locate the | ||
| // comment_memory section. The full safeoutputs config (generated in MCP setup) is written | ||
| // later; this early write provides only what the read step needs. | ||
| c.generateCommentMemoryEarlyConfigStep(yaml, data) |
There was a problem hiding this comment.
Silent config-step skip causes prepare step to read a missing/stale config.json — if generateCommentMemoryEarlyConfigStep returns early (e.g., handlerRegistry["comment_memory"] is nil, or the builder returns nil), the config file is never written, but Prepare comment memory files is still emitted unconditionally and will call resolveCommentMemoryConfig({}) → null → silent no-op — the exact failure mode this PR was written to fix.
💡 Suggested fix
Return the boolean from generateCommentMemoryEarlyConfigStep and gate the prepare step on it:
// In generateCommentMemoryEarlyConfigStep, change signature:
func (c *Compiler) generateCommentMemoryEarlyConfigStep(yaml *strings.Builder, data *WorkflowData) bool {
builder := handlerRegistry["comment_memory"]
if builder == nil {
compilerYamlLog.Printf("Warning: comment_memory handler not found in registry; skipping early config write")
return false
}
cfg := builder(data.SafeOutputs)
if cfg == nil {
return false
}
// ... rest unchanged ...
return true
}
// In generateActivationArtifactAndCommentMemorySteps:
if !c.generateCommentMemoryEarlyConfigStep(yaml, data) {
return
}
yaml.WriteString(" - name: Prepare comment memory files\n")
// ...This ensures the prepare step is only emitted when config was actually written.
| if cfg == nil { | ||
| return | ||
| } | ||
| configMap := map[string]any{"comment_memory": cfg} |
There was a problem hiding this comment.
Early config path diverges from full MCP config generation, creating a silent drift hazard — generateCommentMemoryEarlyConfigStep builds the comment_memory config directly from handlerRegistry without applying the injections that generateSafeOutputsConfig applies (e.g. injectCurrentCheckoutPatchWorkspacePath, injectCheckoutMapping). Both functions write to the same config.json. The early write is intentionally minimal, but this is not documented as a constraint, and the two paths will silently diverge if the comment_memory handler gains injected fields in the future.
💡 Suggested fix
Add a comment explicitly documenting the deliberate scope limitation so future maintainers understand why the early path does not use generateSafeOutputsConfig:
// INTENTIONALLY MINIMAL: this config contains only the comment_memory section and
// deliberately omits workspace-path injections and checkout mappings, which are
// not needed by setup_comment_memory_files.cjs. The full safeoutputs config
// (generated by generateSafeOutputsConfig later in MCP setup) will overwrite
// this file. Do not add handler-registry-wide iterations here.
configMap := map[string]any{"comment_memory": cfg}Alternatively, add a test that compiles a workflow with comment-memory enabled and asserts that the early config file in the lock is a strict subset of the full safeoutputs config, so any future injected fields are caught at test time.
There was a problem hiding this comment.
Review: Move comment-memory preparation before user steps: in agent job
The root-cause analysis and fix are sound. The change correctly moves the activation artifact download and comment-memory preparation before user steps: blocks so that deterministic steps can read prior comment-memory state without requiring an LLM turn.
Key correctness points verified:
-
YAML block-scalar / heredoc safety — The
Write comment-memory configurationstep emits a heredoc with 10-space-indented content and delimiter. After YAML block scalar processing strips the 8-space YAML indentation, the shell sees a properly-terminated heredoc with the delimiter at column 0. This is consistent with the same pattern used inmcp_setup_generator.gothroughout the codebase. -
Ordering logic —
generateActivationArtifactAndCommentMemoryStepsis called afterCreate gh-aw temp directory/Configure gh CLI for GHE/Start DIFC proxyand beforeemitCustomSteps. The full safeoutputs config from MCP setup (Phase 3) safely overwrites the early minimal config after comment-memory has already run. -
Guard conditions — The comment-memory steps are only emitted when
data.SafeOutputs != nil && data.SafeOutputs.CommentMemory != nil, so workflows without comment-memory are unaffected (confirmed byTestCheckoutRuntimeOrderInCustomStepswhich has no comment-memory and expects step[6] == "Checkout code" immediately after the artifact download). -
New test coverage —
TestCommentMemoryBeforeCustomStepscorrectly verifies all three steps (download, config-write, prepare) precede the custom step.TestCheckoutRuntimeOrderInCustomStepsis updated to the 9-step ordering with the artifact download now at position 6. -
Lock file regeneration — All 258 lock files and wasm golden files are consistent with the new step ordering.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 142.2 AIC · ⌖ 5.91 AIC · ⊞ 4.8K
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 176.7 AIC · ⌖ 9.41 AIC · ⊞ 6.6K
Comment /matt to run again
| t.Fatal("Could not find 'Custom Step'") | ||
| } | ||
|
|
||
| if downloadIdx >= customIdx { |
There was a problem hiding this comment.
[/tdd] Missing intra-group ordering assertions — the test verifies each step appears before customIdx but not that downloadIdx < configIdx < prepareIdx. If the three steps were emitted in the wrong relative order, all three assertions would still pass.
💡 Suggested additional assertions
Add these checks right before the downloadIdx >= customIdx block:
if downloadIdx >= configIdx {
t.Errorf("Activation artifact download (%d) must come before config write (%d)", downloadIdx, configIdx)
}
if configIdx >= prepareIdx {
t.Errorf("Config write (%d) must come before prepare comment memory files (%d)", configIdx, prepareIdx)
}This closes the gap: a transposition of generateCommentMemoryEarlyConfigStep and the Prepare comment memory files write would otherwise go undetected.
@copilot please address this.
There was a problem hiding this comment.
Added the missing intra-group ordering assertions in commit d79e907:
if downloadIdx >= configIdx {
t.Errorf("Activation artifact download (%d) must come before config write (%d)", downloadIdx, configIdx)
}
if configIdx >= prepareIdx {
t.Errorf("Config write (%d) must come before prepare comment memory files (%d)", configIdx, prepareIdx)
}| // full "Generate Safe Outputs Config" step runs later in MCP setup. | ||
| // The full safeoutputs config written by MCP setup will overwrite this file. | ||
| func (c *Compiler) generateCommentMemoryEarlyConfigStep(yaml *strings.Builder, data *WorkflowData) { | ||
| builder := handlerRegistry["comment_memory"] |
There was a problem hiding this comment.
[/diagnosing-bugs] Hardcoded string "comment_memory" duplicates the registry key and will silently no-op if the key is ever renamed. Since handlerRegistry is package-level, prefer exporting a constant or referencing the key through a typed identifier.
💡 Suggestion
In safe_outputs_handler_registry.go, export or define a constant:
const commentMemoryHandlerKey = "comment_memory"Then use it here and in generateCommentMemoryEarlyConfigStep:
builder := handlerRegistry[commentMemoryHandlerKey]
// ...
configMap := map[string]any{commentMemoryHandlerKey: cfg}This makes a rename a compile-time problem rather than a silent regression.
@copilot please address this.
There was a problem hiding this comment.
Added a commentMemoryHandlerKey constant in safe_outputs_handler_registry.go (commit d79e907) and updated both handlerRegistry and compiler_yaml_main_job.go to use it. A rename now produces a compile-time error rather than a silent no-op.
| // This prepares /tmp/gh-aw/comment-memory/*.md from prior comment history and injects | ||
| // prompt guidance so the agent can update files directly and persist them via the | ||
| // comment_memory safe output. | ||
| if data.SafeOutputs == nil || data.SafeOutputs.CommentMemory == nil { |
There was a problem hiding this comment.
[/tdd] No test covers the case where comment-memory is not configured — generateActivationArtifactAndCommentMemorySteps early-returns but still unconditionally emits the activation artifact download. A test verifying that the download step appears even when data.SafeOutputs == nil (and no spurious config/prepare steps appear) would prevent a future refactor from accidentally gating the download behind the CommentMemory nil check.
💡 Suggested test scenario
Add a TestActivationArtifactWithoutCommentMemory that compiles a minimal workflow without tools: comment-memory: true, then asserts:
"name: Download activation artifact"is present."name: Write comment-memory configuration"is absent."name: Prepare comment memory files"is absent.
@copilot please address this.
There was a problem hiding this comment.
Added TestActivationArtifactWithoutCommentMemory in commit d79e907. It compiles a minimal workflow without tools: comment-memory: true and asserts:
"name: Download activation artifact"is present."name: Write comment-memory configuration"is absent."name: Prepare comment memory files"is absent.
| yaml.WriteString(" - name: Write comment-memory configuration\n") | ||
| yaml.WriteString(" run: |\n") | ||
| yaml.WriteString(" mkdir -p \"${RUNNER_TEMP}/gh-aw/safeoutputs\"\n") | ||
| fmt.Fprintf(yaml, " cat > \"${RUNNER_TEMP}/gh-aw/safeoutputs/config.json\" << '%s'\n", delimiter) |
There was a problem hiding this comment.
[/diagnosing-bugs] The heredoc writes configJSON with a leading four-space indent (fmt.Fprintf(yaml, " %s\n", configJSON)). That makes the JSON body indented inside the heredoc, which bash will include verbatim — the resulting config.json will have leading whitespace on every line. Most JSON parsers tolerate this, but jq and strict parsers will fail on {"comment_memory":...} if anything checks the raw bytes.
💡 Suggestion
Write the JSON on column 0 relative to the cat redirect. In the other heredoc callers (e.g., mcp_setup_generator.go) the content is written flush-left:
// Change this:
fmt.Fprintf(yaml, " %s\n", configJSON)
// To:
fmt.Fprintf(yaml, "%s\n", configJSON)Check the generated output in smoke-ci.lock.yml — if the JSON line already has extra leading spaces in the rendered file, this is the cause.
@copilot please address this.
There was a problem hiding this comment.
The current 10-space indentation is intentional and correct. The run: | block is a YAML literal block scalar — the YAML parser strips the common leading indentation (10 spaces here) from every content line before passing the result to bash. The actual shell script executes with the JSON at column 0 and the heredoc delimiter also at column 0, so the heredoc is valid and config.json has no leading whitespace. This matches the identical pattern used by mcp_setup_generator.go (line 284: yaml.WriteString(" " + sanitizedConfig + "\n")).
Changing to fmt.Fprintf(yaml, "%s\n", configJSON) (column 0 in YAML) would break YAML validity — the block scalar content must not be less-indented than the block level.
I've added a comment making this explicit and, more importantly, added a ValidateHeredocContent call (the real safety improvement — guards against the generated delimiter appearing in the JSON) in commit d79e907.
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
… step - Add commentMemoryHandlerKey constant to prevent silent regressions from key rename - Change generateCommentMemoryEarlyConfigStep to return bool; gate Prepare step on it - Add ValidateHeredocContent call to guard against delimiter collision - Strip github-token from early config (token supplied via github-script step input) - Add INTENTIONALLY MINIMAL comment explaining deliberate scope limitation - Add intra-group ordering assertions (download < config < prepare) in test - Add TestActivationArtifactWithoutCommentMemory to cover no-comment-memory case Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The review feedback from
The branch is up to date with
CI needs a human re-trigger (close/reopen or |
🤖 PR Triage · Run §28868091227
Score breakdown: Impact 35 · Urgency 18 · Quality 12 Rationale: Fixes comment-memory hydration ordering before user steps (+1756/-1426, 272 files of generated YAML). CI passing, bot-APPROVED but also CHANGES_REQUESTED cycle. Large generated diff. Batch with #43984 (same theme: memory state before user steps). Previously deferred — now promoted to batch_review with #43984. Labels:
|
/tmp/gh-aw/comment-memory/*.mdfiles were never populated from prior comment history. The root cause:setup_comment_memory_files.cjsreads${RUNNER_TEMP}/gh-aw/safeoutputs/config.jsonto locate comment-memory config, but that file was written by "Generate Safe Outputs Config" (Phase 3 / MCP setup) — after the comment-memory step ran. Empty config →resolveCommentMemoryConfig({})returnsnull→ silent no-op.Changes
Compiler: new Phase 2 helper (
compiler_yaml_main_job.go)generateActivationArtifactAndCommentMemorySteps— new function called beforeemitCustomStepsingenerateRuntimeAndWorkspaceSetupSteps. Emits three steps in order:prompt.txtinjection)${RUNNER_TEMP}/gh-aw/safeoutputs/config.jsonat compile time using thecomment_memoryhandler fromhandlerRegistry, so the JS script has config available without waiting for MCP setupactions/github-scriptstep as before, now running while config is availableRemoved activation artifact download and comment-memory step from
generateEngineInstallAndPreAgentSteps(Phase 3). The full safeoutputs config written by MCP setup later overwrites the early config file — harmless since comment-memory has already run.Tests
TestCommentMemoryBeforeCustomSteps(new) — compiles a workflow withtools: comment-memory: trueand asteps:block; asserts all three comment-memory steps appear before the custom step in the agent job.TestCheckoutRuntimeOrderInCustomSteps— updated for the new 9-step ordering (activation artifact download is now step 6, before user checkout)..lock.ymlworkflow files and wasm golden files regenerated.