Fix context propagation and goroutine panic recovery in privileged MCP tools

[Copilot]

Two correctness issues in the privileged MCP logs tool's subprocess context management: context.Background() dropped all context values, and the context-watcher goroutine had no panic containment.

Changes

• context.WithoutCancel(ctx) instead of context.Background() — preserves context values (trace IDs, etc.) from the incoming request context while still detaching the deadline, so the MCP gateway's 60 s RPC deadline doesn't kill the subprocess prematurely.

• Panic recovery in goroutine — adds a top-level defer/recover to the context-watcher goroutine, consistent with the pattern used elsewhere in the codebase, so unexpected panics are logged rather than crashing the process.

// Before
subCtx, subCancel := context.WithTimeout(
    context.Background(),        // drops all context values
    time.Duration(timeoutValue)*time.Minute,
)
go func() {
    select { ... }  // no panic recovery
}()

// After
subCtx, subCancel := context.WithTimeout(
    context.WithoutCancel(ctx),  // preserves values, detaches deadline
    time.Duration(timeoutValue)*time.Minute,
)
go func() {
    defer func() {
        if r := recover(); r != nil {
            mcpLog.Printf("Panic in MCP logs context-watcher goroutine (recovered): %v", r)
        }
    }()
    select { ... }
}()

• Fixes [lint-monster] Fix privileged MCP CLI context propagation and goroutine panic containment #49028

[Copilot]

Pull request overview

Preserves request context values for privileged MCP log subprocesses while safely containing watcher goroutine panics.

Changes:

• Uses context.WithoutCancel(ctx) before applying the subprocess timeout.
• Adds panic recovery and logging to the context-watcher goroutine.

Show a summary per file

File	Description
pkg/cli/mcp_tools_privileged.go	Improves subprocess context propagation and goroutine safety.

Review details

Tip

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

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

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Warning

threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.

Details

The threat detection engine failed to produce results.

Review the workflow run logs for details.

No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #49031 only modified production code (pkg/cli/mcp_tools_privileged.go).

[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 (11 additions detected).

[github-actions]

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

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

The two changes are correct and well-scoped:

• context.WithoutCancel(ctx) (Go 1.21+) properly preserves context values while detaching the gateway deadline — no compatibility issue given go 1.26.5.
• The defer/recover in the goroutine is well-formed and consistent with the codebase pattern.
• The cancellation guard at ctx.Err() == context.Canceled correctly avoids propagating DeadlineExceeded from the gateway into the subprocess.

No actionable issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.2 AIC · ⌖ 4.77 AIC · ⊞ 5.3K

[github-actions]

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 20.7 AIC · ⌖ 4.81 AIC · ⊞ 7K
_{Comment /matt to run again}

[github-actions]

Verdict: COMMENT — no blocking issues found

Solid, well-scoped fix. context.WithoutCancel(ctx) correctly preserves context values while detaching the RPC deadline, and the watcher goroutine's cancellation-forwarding logic (only propagating context.Canceled, ignoring DeadlineExceeded) matches the stated intent and has no leak — it always terminates via one of the two Done() channels since subCtx carries a timeout.

💡 Notes considered but not blocking

• The defer recover() added to the watcher goroutine is defensive but effectively inert: the goroutine body is just a select over two channels and a call to subCancel() (an idempotent context cancel func) — none of these can panic under normal Go semantics. It's harmless boilerplate, not a bug, so not requesting a change here.
• No race conditions: subCancel() may be invoked concurrently from both the deferred call and the goroutine, but context.CancelFunc is documented as safe for concurrent/repeated calls.
• Comment accuracy: the updated comment correctly describes the new behavior and matches the code.

🔎 Code quality review by PR Code Quality Reviewer · aut00 · 37.6 AIC · ⌖ 4.5 AIC · ⊞ 7.8K
_{Comment /review to run again}