From c31baef1b86b8d56bf7411521d77f31240596d7a Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 9 Jul 2026 13:07:23 -0700 Subject: [PATCH 1/4] fix: log stderr from rootless permission repair and make chroot-home removal non-fatal - Capture and log stderr from the Docker-based permission repair container so failures are diagnosable (previously only exit code was logged) - Downgrade chroot-home removal failures from warn to debug since the post-step cleanup (install_copilot_cli.sh) handles it via sudo - This prevents permission repair failures from causing exit code 1 when the agent task itself completed successfully - Add tests for stderr logging behavior Closes #6070 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/artifact-permissions.test.ts | 35 ++++++++++++++++++++++++++++++++ src/artifact-permissions.ts | 6 +++++- src/artifact-preservation.ts | 9 +++++--- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/artifact-permissions.test.ts b/src/artifact-permissions.test.ts index 892767e8f..15ad957a1 100644 --- a/src/artifact-permissions.test.ts +++ b/src/artifact-permissions.test.ts @@ -39,6 +39,41 @@ describe('artifact-permissions', () => { } }); + it('logs stderr when permission repair fails', () => { + const auditDir = makeTempDir(); + try { + getuidSpy = jest.spyOn(process, 'getuid').mockReturnValue(1001); + const warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExecaSync.mockReturnValue({ stdout: '', stderr: 'no such image: agent:latest', exitCode: 1 }); + fixArtifactPermissionsForRootless([auditDir], undefined, undefined, undefined, undefined); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('no such image: agent:latest'), + ); + warnSpy.mockRestore(); + } finally { + fs.rmSync(auditDir, { recursive: true, force: true }); + } + }); + + it('logs exit code without stderr when stderr is empty', () => { + const auditDir = makeTempDir(); + try { + getuidSpy = jest.spyOn(process, 'getuid').mockReturnValue(1001); + const warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExecaSync.mockReturnValue({ stdout: '', stderr: '', exitCode: 1 }); + fixArtifactPermissionsForRootless([auditDir], undefined, undefined, undefined, undefined); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringMatching(/failed.*exit 1/i), + ); + // Should NOT contain a colon suffix when stderr is empty + const warnCall = warnSpy.mock.calls.find(c => typeof c[0] === 'string' && /exit 1/.test(c[0])); + expect(warnCall?.[0]).not.toMatch(/exit 1\):/); + warnSpy.mockRestore(); + } finally { + fs.rmSync(auditDir, { recursive: true, force: true }); + } + }); + it('runs rootless permission repair with translated mount paths', () => { const auditDir = makeTempDir(); try { diff --git a/src/artifact-permissions.ts b/src/artifact-permissions.ts index 46845f4f8..0ab545658 100644 --- a/src/artifact-permissions.ts +++ b/src/artifact-permissions.ts @@ -79,7 +79,11 @@ export function fixArtifactPermissionsForRootless( ); if (typeof result.exitCode === 'number' && result.exitCode !== 0) { - logger.warn(`Rootless artifact permission repair failed for ${dir} (exit ${result.exitCode})`); + const stderr = result.stderr?.trim(); + logger.warn( + `Rootless artifact permission repair failed for ${dir} (exit ${result.exitCode})` + + (stderr ? `: ${stderr}` : ''), + ); } } catch (error) { logger.warn(`Rootless artifact permission repair failed for ${dir}:`, error); diff --git a/src/artifact-preservation.ts b/src/artifact-preservation.ts index 00c638e8b..c23ae3ba0 100644 --- a/src/artifact-preservation.ts +++ b/src/artifact-preservation.ts @@ -247,11 +247,14 @@ export function removeWorkDirectories(workDir: string, options: RemoveWorkDirect ); try { fs.rmSync(chrootHomeDir, { recursive: true, force: true }); - } catch (retryError) { - logger.warn('Failed to remove chroot home directory after permission repair:', retryError); + } catch { + // Non-fatal: chroot-home will be cleaned by the post-step + // (install_copilot_cli.sh's sudo cleanup) or runner infrastructure. + logger.debug(`Could not remove chroot home directory after permission repair: ${chrootHomeDir}`); } } else { - logger.warn('Failed to remove chroot home directory:', error); + // Non-fatal: same reasoning — defer to post-step cleanup. + logger.debug('Failed to remove chroot home directory:', error); } } } From f45923a7495763202727d2b727db4ad6e9f7eeb7 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 9 Jul 2026 19:05:08 -0700 Subject: [PATCH 2/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/artifact-preservation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/artifact-preservation.ts b/src/artifact-preservation.ts index c23ae3ba0..441cd7563 100644 --- a/src/artifact-preservation.ts +++ b/src/artifact-preservation.ts @@ -247,10 +247,10 @@ export function removeWorkDirectories(workDir: string, options: RemoveWorkDirect ); try { fs.rmSync(chrootHomeDir, { recursive: true, force: true }); - } catch { + } catch (retryError) { // Non-fatal: chroot-home will be cleaned by the post-step // (install_copilot_cli.sh's sudo cleanup) or runner infrastructure. - logger.debug(`Could not remove chroot home directory after permission repair: ${chrootHomeDir}`); + logger.debug(`Could not remove chroot home directory after permission repair: ${chrootHomeDir}`, retryError); } } else { // Non-fatal: same reasoning — defer to post-step cleanup. From feb46c3311562e93f33712c7680610789517bc8b Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 9 Jul 2026 19:05:14 -0700 Subject: [PATCH 3/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/artifact-permissions.test.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/artifact-permissions.test.ts b/src/artifact-permissions.test.ts index 15ad957a1..670aaf8d2 100644 --- a/src/artifact-permissions.test.ts +++ b/src/artifact-permissions.test.ts @@ -41,16 +41,15 @@ describe('artifact-permissions', () => { it('logs stderr when permission repair fails', () => { const auditDir = makeTempDir(); + let warnSpy: jest.SpyInstance | undefined; try { getuidSpy = jest.spyOn(process, 'getuid').mockReturnValue(1001); - const warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); mockExecaSync.mockReturnValue({ stdout: '', stderr: 'no such image: agent:latest', exitCode: 1 }); fixArtifactPermissionsForRootless([auditDir], undefined, undefined, undefined, undefined); - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('no such image: agent:latest'), - ); - warnSpy.mockRestore(); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('no such image: agent:latest')); } finally { + warnSpy?.mockRestore(); fs.rmSync(auditDir, { recursive: true, force: true }); } }); From c255e15b8c1c57ce965cf1f83ebc644cce80d473 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 9 Jul 2026 19:05:21 -0700 Subject: [PATCH 4/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/artifact-permissions.test.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/artifact-permissions.test.ts b/src/artifact-permissions.test.ts index 670aaf8d2..20549b9b6 100644 --- a/src/artifact-permissions.test.ts +++ b/src/artifact-permissions.test.ts @@ -56,19 +56,18 @@ describe('artifact-permissions', () => { it('logs exit code without stderr when stderr is empty', () => { const auditDir = makeTempDir(); + let warnSpy: jest.SpyInstance | undefined; try { getuidSpy = jest.spyOn(process, 'getuid').mockReturnValue(1001); - const warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); mockExecaSync.mockReturnValue({ stdout: '', stderr: '', exitCode: 1 }); fixArtifactPermissionsForRootless([auditDir], undefined, undefined, undefined, undefined); - expect(warnSpy).toHaveBeenCalledWith( - expect.stringMatching(/failed.*exit 1/i), - ); + expect(warnSpy).toHaveBeenCalledWith(expect.stringMatching(/failed.*exit 1/i)); // Should NOT contain a colon suffix when stderr is empty const warnCall = warnSpy.mock.calls.find(c => typeof c[0] === 'string' && /exit 1/.test(c[0])); expect(warnCall?.[0]).not.toMatch(/exit 1\):/); - warnSpy.mockRestore(); } finally { + warnSpy?.mockRestore(); fs.rmSync(auditDir, { recursive: true, force: true }); } });