Preserve resolvable safe-output targets

[Copilot]

When an update-pull-request.target referenced a job in safe-outputs.needs, the compiler blanked the target in the agent config—even if the agent depended on that job. On workflow_dispatch, this incorrectly treated the target as triggering and rejected the update.

• Dependency-aware sanitization: Preserve needs.<job> expressions for jobs the agent directly depends on; continue neutralizing references the agent cannot resolve.
• Regression coverage: Verify a fixed PR target survives while unrelated safe-output dependency references are still sanitized.

safe-outputs:
  needs: [setup]
  update-pull-request:
    target: ${{ needs.setup.outputs.pull_request_number }}

• Fixes update-pull-request target from a safe-outputs.needs job output is blanked in the agent config and rejected on workflow_dispatch #64021

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #64176

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[Copilot]

Copilot review overview

🟢 Approval recommended

The dependency propagation and selective sanitization correctly address the reported regression with focused coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Preserves resolvable safe-output targets by making sanitization aware of the agent job’s direct dependencies.

Changes:

• Records agent job dependencies before safe-output config generation.
• Sanitizes only unresolvable needs.<job> expressions.
• Adds regression coverage for preserved and sanitized references.

File	Description
pkg/​workflow/​workflow_data.go	Stores resolved agent dependencies.
pkg/​workflow/​safe_outputs_config_generation.go	Filters sanitization to unresolvable jobs.
pkg/​workflow/​safe_outputs_config_generation_test.go	Tests mixed resolvable and unresolvable references.
pkg/​workflow/​compiler_main_job.go	Resolves dependencies before generating agent steps.
pkg/​workflow/​compiler_main_job_helpers.go	Publishes resolved dependencies to workflow data.
pkg/​workflow/​compiler_jobs_engine_env_test.go	Verifies dependencies remain available during config generation.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Comment Memory

reviewed_at: 2026-09-29T05:23:29.941Z
review_event: COMMENT
top_themes:
  - no blocking issues found in changed lines
  - dependency ordering fix for safe-outputs target sanitization
  - regression coverage for resolvable vs unresolvable needs expressions
files_reviewed:
  - pkg/workflow/compiler_jobs_engine_env_test.go
  - pkg/workflow/compiler_main_job.go
  - pkg/workflow/compiler_main_job_helpers.go
  - pkg/workflow/safe_outputs_config_generation.go
  - pkg/workflow/safe_outputs_config_generation_test.go
  - pkg/workflow/workflow_data.go
comment_count: 0

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 33.9 AIC · ⌖ 6.95 AIC · ⊞ 20.2K · ◷
_{Comment /review to run again}

[github-actions]

Verdict

No blocking issues found in the changed lines; the ordering fix now makes the agents direct dependencies available before safe-outputs config generation, which is the behavior this regression needed.

Reviewed themes

• Verified the new AgentJobNeeds plumbing is populated before generateMCPSetup() consumes safe-outputs config.
• Checked the sanitization change only preserves needs.<job> expressions for jobs the agent actually depends on and still neutralizes unrelated safe-output dependencies.
• Reviewed the added regression tests for both the preserved target and the still-sanitized unrelated dependency path.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 33.9 AIC · ⌖ 6.95 AIC · ⊞ 20.2K
_{Comment /review to run again}

[github-actions]

Impeccable Skills Review

This is a Go compiler internals fix (dependency-aware sanitization of safe-output needs expressions), not a UI/frontend change, so Impeccable's UI-focused modes (audit, critique, harden, distill, extract, clarify) don't apply. I performed a standard correctness/security review instead.

Verification performed:

• go build ./... — passes
• Targeted tests (TestGenerateSafeOutputsConfig*, TestBuildMainJobEngineEnvNeedsExpression, TestSanitizeAgentSafeOutputsConfig*) — all pass
• Full pkg/workflow suite — 2 unrelated pre-existing failures (TestCloudHypervisorSetupBundleScriptExecutesAgainstFixtures, TestBuildDynamicEnclaveExpiryScriptResolvesMinOfConfiguredAndJobExpiry), confirmed present on the base commit before this PR too (time-sensitive/flaky, untouched files)

Findings:

• data.AgentJobNeeds is populated in buildMainJobDependencies before generateSafeOutputsConfig consumes it (via the reorder in buildMainJob), so the resolvable-target check has correct data at generation time.
• Reordering buildMainJobDependencies/warnBuiltinJobEnvReferences earlier in buildMainJob is safe — neither jobCondition nor step generation depend on depends/engineEnvContent.
• Direct unit-test callers of generateSafeOutputsConfig that don't set AgentJobNeeds safely default to nil → full sanitization (equivalent to prior behavior), so no regression for those tests.
• New regression test (TestGenerateSafeOutputsConfigPreservesResolvableNeedsExpressions) correctly covers both the fixed case (needs.setup resolvable) and the still-sanitized case (needs.approval_allowlist unresolvable).

No blocking or actionable issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 69.3 AIC · ⌖ 13.2 AIC · ⊞ 8.1K

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design and /tdd to review this bug fix. This is a small, well-scoped, correctly-tested change — no blocking issues found.

📋 Key Themes & Highlights

Positive Highlights

• ✅ Root cause fix, not symptom patching: the previous code sanitized all jobs in safe-outputs.needs regardless of whether the agent job actually depended on them. The fix correctly threads through the agent job's real dependency list (AgentJobNeeds) so only genuinely unresolvable needs.<job> references are neutralized.
• ✅ Good regression coverage: TestGenerateSafeOutputsConfigPreservesResolvableNeedsExpressions was extended to cover both a resolvable (setup, in AgentJobNeeds) and an unresolvable (approval_allowlist, not in AgentJobNeeds) job in the same test, verifying the fix doesn't regress the original neutralization behavior. TestBuildMainJobEngineEnvNeedsExpression also asserts AgentJobNeeds is populated as a side effect.
• ✅ Reasoning documented in comments: the updated comment in safe_outputs_config_generation.go clearly explains why only jobs not in AgentJobNeeds are neutralized.
• ✅ Verified other callers of generateSafeOutputsConfig (evals, threat-detection) pass SafeOutputs: nil, so the new logic path is never reached there — no unintended side effects.

Minor Observations (non-blocking)

• The new WorkflowData.AgentJobNeeds field is populated as a mutation side effect inside buildMainJobDependencies, consumed later (indirectly, via generateMainJobSteps → generateMCPSetup → generateSafeOutputsConfig) in the same buildMainJob call. This is consistent with existing patterns in the file (e.g., other derived fields set during job construction), and call-order was verified to be correct, but it does add an implicit ordering dependency between buildMainJobDependencies and the later config-generation step that isn't obvious from the type signature alone. A short doc comment on the field noting "must be set before generateSafeOutputsConfig is called" could help future maintainers avoid an ordering regression.
• No changeset file was included in this PR. If this repo's release process expects one for user-facing compiler behavior changes, it may be worth adding.

All targeted tests (TestGenerateSafeOutputsConfig*, TestBuildMainJobDependencies, TestBuildMainJobEngineEnv*) pass, and go build ./... succeeds.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 142.4 AIC · ⌖ 14.5 AIC · ⊞ 10K
_{Comment /matt to run again}

[gh-aw-bot]

@copilot Please review the remaining feedback/checks, refresh the branch if needed, and run the pr-finisher skill.

Generated by 👨‍🍳 PR Sous Chef · pi · gpt54 · 10.8 AIC · ⌖ 8.72 AIC · ⊞ 9.5K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot Please review the remaining feedback/checks, refresh the branch if needed, and run the pr-finisher skill.

Generated by [👨‍🍳 PR Sous Chef](https://github.com/github/gh-aw/actions/...

Reviewed the remaining feedback and checks. No unresolved review threads or actionable findings remain; the branch already includes current main, so no refresh was needed. make fmt, make lint, make test-unit, make test, and make recompile all passed. No code changes were needed; the verified head is 0e815e6. The current-head CI checks completed successfully apart from the agent workflow still running; the cancelled CGO attempt has a successful run on the same head.