fix: agent security and linting improvements

[Piyush0049]

Description

This PR introduces critical fixes for agent stability and defense in depth security, alongside resolving native Windows linting warnings.

Key Changes

• RAG Goroutine Leak: Added a cancellable child context and synchronization channel to the RAG ToolSet. This ensures the forwardEvents and StartFileWatcher goroutines are cleanly terminated when Stop() is called, preventing silent memory and file descriptor leaks across tool reloads.
• Execve Environment Truncation: Added \x00 validation to static environment variables in script_shell.go. This prevents malicious or malformed config.yaml files from injecting NUL bytes that would silently truncate the process environment at the kernel execve boundary.
• Linting Cleanups: Resolved native errorlint, noctx, and bodyclose warnings in Windows specific files (using errors.As, context.Background(), and explicit body closure) and safely managed gosec directives to ensure a perfectly clean golangci-lint run state.

Testing Instructions

• Verified golangci-lint run reports 0 issues.
• Verified task test passes successfully.

[aheritier]

Same here @Piyush0049

Please provide more details about the issue you are trying to solve. Is it something which happens often? What are the symptoms?

Right now the PR doesn't provide enough context to understand if you are fixing a bug or if these are just code reviews by an LLM which could easily hallucinate bugs.

Thanks

[Piyush0049]

@aheritier here is a detailed file-by-file breakdown of the 6 files modified in this PR:

1. Bug Fixes & Security (2 Files)

• pkg/tools/builtin/rag/rag.go:
  • Symptom: RAG file watcher and event goroutines remained running silently in the background after ToolSet.Stop(), causing memory and file handle leaks across tool reloads.
  • Fix: Tied watcher goroutines to a child context (watchCtx) that is canceled in Stop(), and waited for watcherDone completion.
• pkg/tools/builtin/shell/script_shell.go:
  • Symptom: OS kernel process execution (execve) treats \0 (NUL byte) as a string terminator. Env vars containing \0 would cause silent environment block truncation.
  • Fix: Added strings.ContainsRune(val, 0) validation to reject malformed environment variables before shell execution.

2. Windows Cross-Platform Linter Cleanups (4 Files)

• pkg/selfupdate/exec_windows.go: Fixed errorlint (%w error wrapping) and noctx (using exec.CommandContext).
• pkg/desktop/transport/transport_test.go: Fixed bodyclose warning by checking non-nil response body before closing.
• pkg/tools/builtin/backgroundjobs/cmd_windows.go & pkg/tools/builtin/shell/cmd_windows.go: Added //nolint:gosec annotations documenting why Windows API process group setup requires unsafe.Pointer and uint32(proc.Pid) conversions.

Summary (Before vs After)

Feature / Subsystem	Before	After
RAG Tool Lifecycle	Stop() left background goroutines running, causing goroutine & handle leaks.	Stop() cancels watchCtx and waits for watcherDone, cleanly exiting.
Env Var Security	Env vars with NUL (\0) bytes were silently truncated by OS kernel execve.	Rejected with an explicit error before process execution.
Windows Linters	Warnings triggered for errorlint, noctx, bodyclose, and gosec.	Clean golangci-lint execution on Windows with zero warnings.

[docker-agent]

Assessment: 🔴 CRITICAL

One high-severity confirmed bug was found in the new goroutine-lifecycle code added by this PR.

[dgageot]

Thanks for the PR! One thing needs fixing before this can go in: ToolSet.Stop() deadlocks forever when Start() failed.

In pkg/tools/builtin/rag/rag.go, t.watcherDone is only created after manager.Initialize() succeeds, but Stop() gates the receive on cancelWatcher != nil, which is set unconditionally:

watchCtx, cancel := context.WithCancel(ctx)
t.cancelWatcher = cancel            // set unconditionally
if err := t.manager.Initialize(ctx); err != nil {
    cancel()
    return ...                      // watcherDone is still nil
}
t.watcherDone = make(chan struct{})

// Stop():
if t.cancelWatcher != nil {
    t.cancelWatcher()
    <-t.watcherDone                 // nil channel -> blocks forever
}

This matters because StartableToolSet.Stop() (pkg/tools/startable.go:212) calls the inner Stop() unconditionally, while holding s.mu — it does not check whether Start() succeeded. So a RAG toolset that fails to initialize (unreachable embedding model, bad DB, bad doc path) hangs on teardown and wedges every subsequent Start/Stop/IsStarted on that mutex. That trades a silent goroutine leak for a hard hang.

Reproduced locally on this branch with a mock strategy whose Initialize returns an error, then calling Stop() in a goroutine:

--- FAIL: TestStopAfterFailedStart (3.00s)
    Stop() deadlocked after a failed Start()

Note that commit ec033bb is what introduced it: it moved make(chan struct{}) from before Initialize to after it. The previous revision deadlocked too (channel created, never closed on the error path), which suggests this path isn't covered yet.

Minimal fix:

if t.watcherDone != nil {
    <-t.watcherDone
}

Cleaner: create the channel up front and close() it on every exit path, so the invariant is "non-nil implies eventually closed" instead of depending on statement order. Either way, please add a regression test that calls Stop() after a failed Start() — it's a two-line guard, but it's exactly the case that got missed twice.

[Piyush0049]

@dgageot I have pushed a commit addressing the changes you asked to do. Please do let me know if any other changes are required.