Propagate cancellation through audit analysis soft-failure paths

[Copilot]

This follow-up addresses a cancellation gap in the errgroup migration from #46973: some analysis goroutines could return nil after context cancellation, causing collectAuditAnalysisResults to report success. The change ensures cancellation is surfaced consistently even when a worker encounters a soft failure during in-flight shutdown.

• Context cancellation propagation

  • collectAuditAnalysisResults now checks ctx.Err() after g.Wait() and returns cancellation if set.
  • This closes the success-on-cancel window when group workers do not emit a hard error.

• Soft-failure behavior with canceled context

  • launchJobDetailsAnalysis now returns gctx.Err() when cancellation is active instead of swallowing errors as non-fatal.
  • runAuditAnalysis now does the same in its soft-failure branch, preserving existing soft-failure behavior only when the context is still valid.

• Regression coverage

  • Added a targeted concurrency test proving cancellation is returned when a soft-failing analysis is interrupted in-flight.

if err := g.Wait(); err != nil {
	return results, err
}
if ctx.Err() != nil {
	return results, ctx.Err()
}

[Copilot]

Pull request overview

Ensures audit analysis consistently propagates context cancellation through soft-failure paths.

Changes:

• Returns cancellation after analysis group completion.
• Propagates cancellation from job-detail and generic analysis workers.
• Adds regression coverage for in-flight cancellation.

Show a summary per file

File	Description
pkg/cli/audit.go	Propagates cancellation through audit analysis paths.
pkg/cli/audit_concurrency_test.go	Tests canceled soft-failure behavior.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Medium

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

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

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

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

[github-actions]

The fix is correct and well-targeted. The three changes complement each other: soft-failure paths in launchJobDetailsAnalysis and runAuditAnalysis now surface gctx.Err() when the context is cancelled, and the collectAuditAnalysisResults post-wait guard closes the remaining success-on-cancel window when all goroutines returned nil. The regression test validates the new behavior concurrently. No blocking issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.1 AIC · ⌖ 4.34 AIC · ⊞ 5K

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /diagnosing-bugs — one finding, overall solid work.

📋 Key Themes & Highlights

Finding

• Partial test coverage: runAuditAnalysis cancellation is now tested, but the identical soft-failure+cancel branch in launchJobDetailsAnalysis has no regression test.

Positive Highlights

• ✅ Clean, minimal fix — each change is a precise 3-line guard closing the identified gap.
• ✅ The post-g.Wait() ctx.Err() check in collectAuditAnalysisResults correctly handles the silent soft-fail-on-cancel scenario.
• ✅ New test uses a blocking channel to reliably reproduce the race without flakiness.
• ✅ PR description is clear and self-contained.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 18.7 AIC · ⌖ 4.5 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

[/tdd] The new test covers the runAuditAnalysis cancellation path well, but the analogous soft-failure cancellation path in launchJobDetailsAnalysis (audit.go line 749–751) has no equivalent test. That branch has the same shape and the same risk.

💡 Suggested test sketch

Add a test that sets up a fake fetchJobDetailsWithCounts that blocks until gctx is done, then returns an error, and asserts g.Wait() returns context.Canceled. The pattern is identical to TestRunAuditAnalysisReturnsCancellationForSoftFailuresWhenContextCanceled.

@copilot please address this.

[github-actions]

🧪 Test Quality Sentinel Report

❌ Test Quality Score: 80/100 — Needs Review (Build Tag Violation)

Analyzed 1 test: 1 design, 0 implementation, 1 hard violation.

📊 Metrics (1 test)

Metric	Value
Analyzed	1 (Go: 1, JS: 0)
✅ Design	1 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	1 (100%)
Duplicate clusters	0
Inflation (prod 9 lines → test 25 lines)	2.78:1 — Exceeds 2:1 threshold
🚨 Violations	1 (HARD)

⚠️ Build Tag Violation (BLOCKER)

File: pkg/cli/audit_concurrency_test.go

The test file is missing the mandatory //go:build tag on line 1. Go test files in this repository must declare either:

• //go:build !integration for unit tests
• //go:build integration for integration tests

Action required: Add one of the above tags as the first line of the file before the package declaration.

✅ Flagged Tests (1 design test — high value)

TestRunAuditAnalysisReturnsCancellationForSoftFailuresWhenContextCanceled (audit_concurrency_test.go:39-62)

Classification: Design test (behavioral contract), high value, excellent edge-case coverage.

Why this test matters: Verifies that context cancellation propagates through soft-failure code paths. Without this test, cancellation would be silently ignored when errors occur, breaking the contract that context.Canceled errors flow through the errgroup.

Test quality: Uses proper synchronization (started channel) to coordinate the cancellation timing, tests both that error is context.Canceled AND that the setter was not called. All assertions have descriptive failure messages.

Minor concern: Test file inflation (2.78:1 ratio) is acceptable here given the complexity of testing concurrent cancellation.

Verdict

❌ REQUEST_CHANGES. Hard violation: missing //go:build tag in new/modified test file. Despite strong test quality (100% design tests, excellent edge-case coverage), this coding-guideline violation must be addressed before merge.

🧪 Test quality analysis by Test Quality Sentinel · 15.4 AIC · ⌖ 11.5 AIC · ⊞ 7K · ◷
_{Comment /review to run again}

[github-actions]

❌ Test Quality Sentinel: 80/100. Hard violation detected: new/modified Go test file missing mandatory //go:build tag on line 1. Test quality is excellent (100% design tests, strong edge-case coverage), but the coding-guideline violation must be resolved before merge. See the sentinel comment above for details.

[github-actions]

Non-blocking observation

The post-g.Wait() context check is the correct and complete fix for the cancellation gap. The in-goroutine guards are a nice-to-have but only trigger when workers return a non-nil error — workers that return nil on graceful shutdown still rely on the outer check. See inline comment for details.

🔎 Code quality review by PR Code Quality Reviewer · 41 AIC · ⌖ 6.03 AIC · ⊞ 5.6K
_{Comment /review to run again}

Comments that could not be inline-anchored

pkg/cli/audit.go:11

Goroutine-level cancellation check is incomplete: If a worker's fn() returns nil on cancellation (by observing <-gctx.Done() internally and returning nil, zero), the checks at lines 20–21 and 30–31 are never reached — the post-g.Wait() check (line 9–11) is the only guard that catches this case.

<details>
<summary>💡 Details</summary>

The inner-goroutine pattern:

if gctx.Err() != nil {
    return gctx.Err()
}
auditLog.Printf(...)  // soft-failure path

only triggers when `…

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.15