Repository navigation
Require successful redaction for all agent artifact uploads - #70875
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ 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.
|
|
✅ 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.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (106 new lines under 📄 Draft ADR committed: Decision captured: the
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs MatterADRs 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
No actionable blocking issues found in the changed lines. The artifact gating change is internally consistent with the documented security requirement, and the updated tests cover AWF/no-AWF configurations plus custom redaction outcomes. I did not run code or tests per the static-review constraint.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.23 AIC · ⌖ 0.544 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd per pr-triage classification (bug_fix). The fix is sound: redact_secrets is unconditionally emitted in the agent job, so always gating on secretRedactionSuccessCondition(data) is correct and closes the real gap (no-AWF workflows previously skipped the gate). Test coverage is strong — the new sandbox/tc nested matrix exercises both AWF and non-AWF paths against every hook outcome, and asserts exactly one redact_secrets step id, which is exactly the kind of regression-test discipline /tdd calls for.
📋 Key Themes & Highlights
Key Themes
- One actionable finding: removing the requireSuccessfulRedaction parameter also removed the last caller of hasSafeJobArtifactPaths (pkg/workflow/safe_jobs.go:285), leaving it unused dead code. Flagged inline on compiler_yaml_post_agent.go:306.
- No other logic gaps found in the diff: generateAgentOutputFallbackUpload and generateUnifiedArtifactUpload both now apply the same unconditional condition, and the doc update in artifacts.md accurately describes the new behavior.
Positive Highlights
- ✅ Good regression-test restructuring: wrapping the existing table in an outer sandbox table instead of duplicating cases keeps the matrix DRY while doubling coverage.
- ✅ The PR body is unusually careful about scope (explicitly not claiming to fix the heap-exhaustion/permission issues referenced in #69800/#69180), which is exactly the kind of precise root-cause framing /diagnosing-bugs encourages.
- ✅ Simplification of generateUnifiedArtifactUpload's signature (dropping the boolean parameter) is a net readability win.
No blocking issues; the dead-code cleanup is a nice-to-have, not a correctness concern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 54.2 AIC · ⌖ 13.9 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
An unused helper introduces a full-scan lint failure that diff-only checks miss.
1 open issue
What changed in this PR
This PR makes agent artifact uploads require successful built-in and custom redaction, including workflows without the agent firewall.
Changes:
- Apply the redaction guard to both upload paths.
- Expand regression coverage across firewall configurations and hook outcomes.
- Document upload eligibility and add a patch changeset.
| File | Description |
|---|---|
pkg/workflow/redaction_upload_gating_test.go |
Expands redaction-gate regression coverage. |
pkg/workflow/compiler_yaml_post_agent.go |
Updates the unified upload call. |
pkg/workflow/compiler_yaml_artifacts.go |
Requires successful redaction for both uploads. |
docs/src/content/docs/reference/artifacts.md |
Documents upload eligibility. |
.changeset/patch-gate-all-agent-uploads-on-redaction.md |
Adds a patch release note. |
Review effort: Max
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Reply to comment 6315455518: No additional change is needed for this completion notice; the review summary reports no actionable blocking issues. The separately reported unused helper was removed in this batch. |
|
Reply to comment 6315455545: Accepted the actionable finding from the skills-based review: removed the unused hasSafeJobArtifactPaths helper. The unconditional redaction guards and their existing outcome coverage are unchanged. |
|
Reply to comment 6315457732: Accepted the ADR follow-up: reviewed and completed the bot-authored draft with the decision rationale, alternatives, and consequences. The finalized ADR is now linked in the PR body. |
|
Reply to comment 6315460172: No additional code change is needed for this reported_no_action notice because it contains no specific test finding. The existing AWF/no-AWF and custom-hook outcome regressions are retained. |
|
Reply to comment 6315462457: No change is needed for this reported_no_action notice because it contains no actionable finding. It is not treated as proof of correctness; the batch is checked independently. |
Summary
agentoragent-output-fallbackuploads, including workflows without AWF or custom artifact paths.continue-on-error. An engine failure alone still permits diagnostic uploads after successful redaction.hasSafeJobArtifactPathshelper identified in review. Full-package unused-only lint, without diff filtering, reports zero issues.ADR: ADR-70875: Require Successful Redaction for All Agent Artifact Uploads
The ADR is finalized as Proposed for maintainer acceptance. It records the rationale, alternatives, fail-closed trade-offs, and limitations rather than claiming to solve the underlying runtime failures.
Investigation and scope
Refs #69800 and #69180; this PR intentionally does not close either historical report automatically.
redact_secretsID described in AWF artifact upload guards reference missing redact_secrets step #69180 is already covered by current source and existing AWF regressions. This PR addresses the remaining no-AWF upload-guard gap instead.Validation
make buildandPATH="/opt/homebrew/bin:$PATH" make fmtpassed.go test ./pkg/workflow -run 'TestCustomSecretMasking(StepIDs|UploadGates|WorkQueueUploadGates)$|TestAgentOutputFallbackArtifact|TestAgentArtifactExcludesUserGeneratedDirectory' -count=1, including after the review cleanup.golangci-lint run --enable-only unused ./pkg/workflowpassed for the review cleanup, without diff filtering.make agent-report-progressran impacted unit tests successfully once. Schema freshness and change-scoped custom Go lint passed.golangci-lint run --timeout=30m --concurrency=2 --allow-parallel-runners --new-from-rev=HEAD ./pkg/workflowpassed with zero new issues before the initial implementation commit. Review correctly identified that diff-only lint did not catch the subsequently removed unused helper.make check-workflow-driftpassed and restored all generated files.Found unknown linters in //nolint directives: largefunc, uncheckedsliceindexwarning as a non-diagnostic failure while filtering unchanged-file findings. Attempts also encountered a shared linter lock and timeout. This is not presented as a fully green wrapper run.Security review
A single read-only reviewer passed all ten scoped safety questions for the production changes and authored documentation, with no confirmed vulnerabilities. Their reviewed upload-generator and artifact-reference hashes still match the committed content. Afterwards, only test indentation and a compile-only fixture's required
threat-detection: falsesetting changed; runtime tools rejected follow-up to the synchronous reviewer, so the corrected fixture has passing local coverage but no renewed reviewer verdict. No replacement reviewer or live execution was used. The review-follow-up batch removes dead code and finalizes the non-executable ADR; it does not change the upload guards.Authored Markdown documents the stronger upload eligibility and its rationale, and announces it in the patch changeset; it introduces no executable permissions, credential destinations, or sandbox exceptions.