Repository navigation
Harden dry-run compilation against hosted execution effects - #70851
Conversation
Disable custom jobs and reusable calls without retaining credentials, reject unsupported host execution and OIDC declarations, fail on existing actionable scanner findings, and preserve trigger comments and reusable telemetry contracts. Document the breaking diagnostic-policy change and migration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines. The review was limited to static inspection of the pre-fetched diff; tests were not run per review instructions.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 1.87 AIC · ⌖ 0.668 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (633 new lines under 📄 Draft ADR committed: The draft captures the decision to make
📋 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
|
Expect existing actionable findings to remain blocking in synthetic and native ShellCheck regressions while asserting stable script-location identities and zero added findings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /improve-codebase-architecture (per pr-triage: architecture_change) focused on compiler_development_safety.go, compiler_safe_output_jobs.go, and the yaml.Node rewrite in compiler_development_telemetry.go.
Overall this is solid, well-tested security-hardening work — the new validateDryRunEffects/makeDryRunJobInert logic has thorough test coverage (compiler_development_safety_test.go), including the "builtin job name bypass" edge case, and the YAML trigger-preservation rewrite is backed by dedicated round-trip and idempotency tests. No blocking issues found; left 3 non-blocking suggestions.
📋 Key Themes
- Manually-enumerated field list drift risk (
compiler_development_safety.go): the rejected builtin-job-override fields (setup-steps, pre-steps, steps, permissions, uses, with, secrets) are a separate list from the augmentation allowlist incompiler_builtin_job_augmentation.go(needs, if, permissions, timeout-minutes, continue-on-error). Today there's no exploitable gap (no augmentation path readsenvfor builtin jobs), but the two lists aren't coupled, so a future augmentation addition could silently bypass this safety net. - Early-return divergence in
buildCallWorkflowJob's dry-run branch (compiler_safe_output_jobs.go): correct and well-tested, but the dry-run/normal branches now diverge more in control flow than before — a short comment explaining why skippingaddCallWorkflowSecrets/addCallWorkflowPermissionsis safe would help future maintainers. - Mixed YAML abstraction levels in
prepareDryRunWorkflowDispatch: the function now mixes rawyaml.Nodemanipulation with higher-levelgoyaml.Unmarshal/MarshalWithFieldOrdercalls in the same function. Justified by the comment-preservation goal and covered by tests, but worth a doc comment for future readers.
Positive Highlights
- ✅ Excellent new test coverage for both the safety-check logic and the YAML trigger rewrite (round-trip, idempotency, and rejection-of-invalid-input tests).
- ✅ Documentation (
.github/aw/debug-agentic-workflow.md,CHANGELOG.md, changeset) is updated consistently with the breaking-change scope. - ✅ Scanner gate tightening (existing+added findings) is a deliberate, well-documented breaking change with updated tests.
Investigation was bounded to the pre-fetched diff plus targeted cross-references into compiler_builtin_job_augmentation.go and the job schema; no remaining uncertainty beyond what's noted above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 236.2 AIC · ⌖ 15.4 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Authored host execution and OIDC grants still bypass dry-run rejection, and compilation regressions remain.
6 open issues
What changed in this PR
Hardens gh-aw dry-run compilation against retained execution and credentials, without treating diagnostic workflows as a sandbox.
Changes:
- Replace custom jobs and managed reusable calls with inert placeholders.
- Reject unsupported host effects and fail existing actionable scanner findings.
- Preserve trigger comments and update tests, documentation, and migration guidance.
| File | Description |
|---|---|
pkg/workflow/compiler_yaml.go |
Integrate safety validation and job disabling. |
pkg/workflow/compiler_safe_output_jobs.go |
Replace dry-run reusable calls. |
pkg/workflow/compiler_development_test.go |
Update disabled-call expectations. |
pkg/workflow/compiler_development_telemetry.go |
Preserve triggers during dispatch insertion. |
pkg/workflow/compiler_development_telemetry_test.go |
Cover telemetry and reusable-secret isolation. |
pkg/workflow/compiler_development_safety.go |
Validate effects and create inert jobs. |
pkg/workflow/compiler_development_safety_test.go |
Test rejection and job disabling. |
pkg/workflow/compiler_development_dispatch_test.go |
Test trigger and comment preservation. |
pkg/cli/mcp_tools_readonly.go |
Update the compile tool contract. |
pkg/cli/compile_scanner_differential.go |
Gate existing findings and normalize paths. |
pkg/cli/compile_scanner_differential_test.go |
Update scanner gate expectations. |
pkg/cli/compile_development.go |
Update blocking diagnostic wording. |
pkg/cli/compile_development_test.go |
Check authored OIDC rejection. |
pkg/cli/compile_development_report.go |
Report hardened scope and scanner gates. |
pkg/cli/compile_development_report_test.go |
Update report assertions. |
docs/src/content/docs/setup/cli.md |
Document dry-run restrictions. |
docs/src/content/docs/reference/compilation-process.md |
Explain boundaries and compatibility. |
cmd/gh-aw/main.go |
Update CLI help. |
CHANGELOG.md |
Record breaking behavior. |
.github/aw/debug-agentic-workflow.md |
Update hosted-debug safeguards. |
.changeset/major-fail-closed-dry-run-host-effects.md |
Declare major release and migration guidance. |
Review effort: Max
Reject remaining resolved host effects and fail closed on built-in overrides, preserve reusable output bindings and runner policy in inert jobs, separate compiler-owned stack gates from authored steps, and complete the ADR and regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to this review notice: No code change is needed for this completion notice; the review reported no actionable blocking issues. The actionable inline feedback from the other reviews is addressed separately in this batch. |
|
In response to this review notice: Accepted the three architecture suggestions: built-in overrides now use a fail-closed allowlist, and comments explain the reusable-call early return and node-level trigger normalization. Each inline thread has a specific response. |
|
In response to this review notice: No code change is needed for this no-action notification, and I am not treating it as validation evidence. The ShellCheck expectations and new guard/placeholder regressions were validated directly in this batch. |
|
In response to this review notice: Accepted the ADR follow-up: reviewed and completed ADR-70851 with the decision rationale, alternatives, migration consequences, and remaining sandbox limitations. The PR body now links the ADR. |
|
In response to this review notice: No code change is needed for this no-action notification; it does not establish correctness or publication success. The actionable review fixes were validated and published separately in this batch. |
|
In response to this review notice: Accepted: finalized ADR-70851 with Context, Decision, Alternatives Considered, and Consequences, and linked it in the PR body. It documents fail-closed admission and inert jobs without claiming a general hosted sandbox. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Why
The hosted-readiness audit found that staging built-in safe outputs did not prevent custom jobs, publishers, reusable workflows, or host-side extensions from retaining real execution and credentials. Successful compilation was not a sufficient signal for unattended hosted execution.
This change makes those diagnostic boundaries fail closed without claiming that dry-run is a general execution sandbox.
ADR: ADR-70851: Fail Closed in Dry-Run Compilation for Host Execution Effects
Changes
Compatibility and migration
Breaking change: diagnostic workflows containing unsupported effects now fail compilation rather than leaving those effects unverified. Custom jobs have empty outputs and can cause dependent jobs to skip. Use a separate diagnostic workflow without unsupported declarations and resolve actionable scanner findings before live testing.
Ordinary compilation is unchanged. Agent shell execution, custom engines, custom credentials, and hosted identity/runner behavior still require independent review and isolation. A major changeset, migration guidance, CLI/MCP descriptions, and reference documentation are included.
Validation
./gh-awfleet audit: ordinary compilation succeeded for all 346 workflows; hardened dry-run emitted 138 diagnostic locks and intentionally rejected 208 workflows with unsupported effects. These counts predate the additional review hardening and are historical audit results, not current fleet certification.Security review note
No credentials, action dependencies, or runtime privileges were added. Diagnostic output intentionally removes custom executable code and credential forwarding. Existing safe-update warnings about removed actions remain blocking rather than being approved or suppressed. Review follow-ups close the reported resolved-field and optional-built-in execution paths; this is not a claim that arbitrary agent code is sandboxed.