Repository navigation
Record safe job and script outcomes in unified sessions - #72146
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@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.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new safe-job URI collector accepts arbitrary absolute URL schemes via new URL(), including executable or local schemes such as (redacted) (redacted) and file:. Restrict schemes before persisting and rendering resource evidence.
The specialist pass was unavailable because its file-read invocation was denied; this review is based on the pre-fetched diff and targeted source inspection. Inline publication was blocked by workflow permissions, so no inline comments were published.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (290 new lines in 📄 Draft ADR committed: Decision captured in the draft: Add an opt-in outcome contract. Safe jobs can declare
📋 What to do next
After the PR body links an ADR, this gate will run again and check that the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design to review the safe-job/script outcome recording work. No actionable issues found — the change is well-scoped and heavily tested.
📋 Key Themes & Highlights
What was reviewed
actions/setup/js/unified_session.cjs(safe-job outcome projection): validates job names, status values, and resource URLs with explicit fail-closed warnings (invalid_safe_job_names,invalid_safe_job_status,invalid_safe_job_url) rather than silently accepting forged data. Authoritative Actions jobresultalways overrides a reportedsuccessfromoutputs.status, which correctly prevents a compromised step from claiming success after a job failure.pkg/workflow/notify_comment.go(buildUsageSafeJobResultsEnv): projects only declaredstatus/urloutputs per job into the conclusion env, keeping privateneedsoutputs and unrelated jobs out of scope — confirmed byTestSafeJobOutcomeProjectionOnlyReferencesDeclaredOutputsandTestConclusionSafeJobOutcomeAllowlist.actions/setup/js/safe_output_manifest.cjs(extractCreatedItemFromResult): correctly excludesskipped/deferred/cancelled/staged/success:falseresults from being counted as created resources, even when a URL is present — this closes a real gap where a staged/failed operation could previously have looked like a successful creation.- URL values pass through existing
redactManifestValue/secret-redaction before being written to the manifest, and the unified-session collector rejects URLs with embedded control characters or userinfo (uri.username/uri.password), guarding against injection via forged step outputs.
Test coverage
- Go:
TestSafeJobOutcomeOutputsPropagation,TestConclusionSafeJobOutcomeAllowlist,TestConclusionSafeJobOutcomesPreserveAllJobResults,TestSafeJobOutcomeProjectionOnlyReferencesDeclaredOutputs, andpkg/parser/schema_safe_job_outputs_test.goall pass locally. - JS: new
actions/setup/js/safe_output_script_outcomes.test.cjsand additions tounified_session.test.cjscover malformed/forged status, URL redaction, and legacy-shape compatibility (could not execute vitest in this sandbox due to a network/cert restriction onnpm install, so JS coverage was verified by inspection only). - Docs (
safe-outputs-runtime.md,shared-safe-jobs.md,custom-safe-outputs.md) and skill files (developer-internals,operational-value-designer) were updated consistently with the newoutputs.status/outputs.urlcontract.
Positive highlights
- ✅ Fail-closed validation throughout (name/status/URL) with warnings instead of exceptions or silent corruption
- ✅ Authoritative job result cannot be overridden by a forged
successstatus - ✅ Non-applied outcomes (skipped/deferred/cancelled/staged/failed) never register as created resources, even with a URL present
- ✅ Minimal, surgical projection of only declared outputs into the conclusion job env
No blocking issues identified; approving.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~81 AIC · ⌖ ~19 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Outcome handling can miscount artifacts and label preparation as successful execution, and an affected compiler golden remains stale.
4 open issues
What changed in this PR
Updates gh-aw’s compiler and safe-output runtime to capture custom job and script execution outcomes in unified conclusion sessions.
Changes:
- Adds optional safe-job status and resource URI mappings, projecting only declared outputs.
- Persists applied script outcomes and displays execution status in session summaries.
- Adds regression coverage, authoring guidance, and updated workflow locks.
| File | Description |
|---|---|
pkg/workflow/safe_jobs.go |
Parses and propagates safe-job output mappings. |
pkg/workflow/safe_job_outcomes_test.go |
Tests output propagation and conclusion projection. |
pkg/workflow/notify_comment.go |
Supplies configured safe-job outcomes to conclusion. |
pkg/parser/schemas/main_workflow_schema.json |
Defines optional status and URL mappings. |
pkg/parser/schema_safe_job_outputs_test.go |
Tests accepted and rejected output configurations. |
docs/src/content/docs/reference/custom-safe-outputs.md |
Documents job and script outcome contracts. |
actions/setup/js/unified_session.test.cjs |
Tests outcome collection and validation. |
actions/setup/js/unified_session.cjs |
Collects safe-job result events. |
actions/setup/js/unified_session_render.test.cjs |
Tests status and URI rendering. |
actions/setup/js/unified_session_render.cjs |
Displays safe-output execution status. |
actions/setup/js/types/safe-output-script.d.ts |
Documents script returns and permits void results. |
actions/setup/js/types/handler-factory.d.ts |
Defines resource URI and non-applied result flags. |
actions/setup/js/safe_output_script_outcomes.test.cjs |
Tests script outcome persistence and suppression. |
actions/setup/js/safe_output_manifest.test.cjs |
Updates successful manifest expectations. |
actions/setup/js/safe_output_manifest.cjs |
Records explicit success and suppresses non-applied results. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Updates finalized-review manifest expectations. |
.github/workflows/smoke-copilot.lock.yml |
Adds safe-job outcome collection variables. |
.github/workflows/smoke-copilot-arm.lock.yml |
Adds safe-job outcome collection variables. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Adds safe-job outcome collection variables. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Adds safe-job outcome collection variables. |
.github/workflows/mcp-inspector.lock.yml |
Adds Slack-job outcome collection variables. |
.github/workflows/daily-choice-test.lock.yml |
Adds environment-job outcome collection variables. |
.github/workflows/aw-issue-clustering.lock.yml |
Adds publishing-job outcome collection variables. |
.github/skills/operational-value-designer/SKILL.md |
Explains outcome evidence for graders. |
.github/skills/developer-internals/SKILL.md |
Documents internal outcome collection contracts. |
.github/aw/shared-safe-jobs.md |
Adds shared-job evidence guidance. |
.github/aw/safe-outputs-runtime.md |
Describes job mappings and script return values. |
Review effort: Max
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 2a1e0ae
|
…come-status-exposure 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>
Integrate origin/main at e03dba1, including the deterministic setup-action resolver tests. Regenerate compiled workflows from the merged sources and preserve both safe-job outcome and memory-save session evidence. Give script outcome tests explicit GitHub runtime fixtures and verify closed pull requests still suppress script effects. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Expose execution status and canonical resource URIs from custom safe jobs and scripts so conclusion can include them in the unified session.
outputs.statusandoutputs.urlmappings.{ success: true, url }persist successful execution evidence in the manifest.safe_output.resultevents.Safe-job mappings reference execution-step outputs:
statusacceptssuccess,failure,cancelled, orskipped; absent status falls back to the Actions job result.urlidentifies the canonical resource returned by the service, not an agent-supplied target.