Skip to content

Refactor container cleanup to deduplicate log/session preservation paths#3361

Merged
lpcox merged 2 commits into
mainfrom
copilot/fix-duplicate-code-container-cleanup
May 18, 2026
Merged

Refactor container cleanup to deduplicate log/session preservation paths#3361
lpcox merged 2 commits into
mainfrom
copilot/fix-duplicate-code-container-cleanup

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 18, 2026

cleanup() in src/container-cleanup.ts repeated the same chmod-or-rename preservation flow across four directories (session-state, api-proxy-logs, cli-proxy-logs, squid-logs). This made behavior changes error-prone and required editing multiple near-identical blocks.

  • What changed

    • Added a single internal helper, preserveDirectory(...), to encapsulate:
      • runtime-directory path handling (chmod -R a+rX + "... available at" log)
      • default workDir fallback (rename to timestamped /tmp + "... preserved at" log)
      • per-path error message variants
      • optional chmod after move (used by squid logs)
  • Where duplication was removed

    • Replaced four inline blocks in cleanup() with helper calls for:
      • agent session state (agent-session-state)
      • API proxy logs (api-proxy-logs)
      • CLI proxy logs (cli-proxy-logs)
      • squid logs (squid-logs)
  • Behavior preserved

    • Existing labels, destination naming, and debug/error message wording remain aligned with prior behavior.
    • Squid keeps its special post-rename permission normalization via helper option (chmodPreservedDir).
preserveDirectory({
  runtimeDir: proxyLogsDir,
  runtimeSubdir: 'api-proxy-logs',
  workDir,
  workSubdir: 'api-proxy-logs',
  destinationBaseName: 'api-proxy-logs',
  timestamp,
  availableLabel: 'API proxy logs',
  preservedLabel: 'API proxy logs',
  permissionErrorMessage: 'Could not fix api-proxy log permissions:',
  preserveErrorMessage: 'Could not preserve api-proxy logs:',
});

Copilot AI changed the title [WIP] Refactor duplicated code in container-cleanup.ts Refactor container cleanup to deduplicate log/session preservation paths May 18, 2026
Copilot finished work on behalf of lpcox May 18, 2026 22:30
Copilot AI requested a review from lpcox May 18, 2026 22:30
@lpcox lpcox marked this pull request as ready for review May 18, 2026 22:48
Copilot AI review requested due to automatic review settings May 18, 2026 22:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors cleanup() in src/container-cleanup.ts to remove four near-identical chmod-or-rename log/session-state preservation blocks by introducing a single preserveDirectory() helper with per-call options.

Changes:

  • Adds PreserveDirectoryOptions type and preserveDirectory() helper covering both the runtime-directory chmod path and the workDir→/tmp rename path.
  • Replaces four inline preservation blocks (agent session state, api-proxy logs, cli-proxy logs, squid logs) with helper calls.
  • Preserves squid-specific post-rename chmod via chmodPreservedDir, and squid's no-existsSync runtime chmod via runtimeDirMustExist: false.
Show a summary per file
File Description
src/container-cleanup.ts Introduce preserveDirectory helper and replace four duplicated preservation blocks in cleanup().

Copilot's findings

Tip

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

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actions
Copy link
Copy Markdown
Contributor

🧪 Smoke Test Results

Test Status
GitHub MCP connectivity
GitHub.com HTTP connectivity ⚠️ (pre-step data unavailable — template not expanded)
File write/read ⚠️ (pre-step data unavailable — template not expanded)

Overall: PARTIAL — MCP test passed; connectivity and file tests could not be verified due to unexpanded template variables.

PR: Refactor container cleanup to deduplicate log/session preservation paths — author @Copilot, assignees @lpcox @Copilot

📰 BREAKING: Report filed by Smoke Copilot

@github-actions
Copy link
Copy Markdown
Contributor

🔥 Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP connectivity
GitHub.com HTTP ⚠️ pre-step data unavailable (template vars not expanded)
File write/read ⚠️ pre-step data unavailable (template vars not expanded)
BYOK inference (agent → api-proxy → api.githubcopilot.com)

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com.

PR: "Refactor container cleanup to deduplicate log/session preservation paths" — author: @Copilot, assignees: @lpcox, @Copilot

Overall: PARTIAL (BYOK path ✅, pre-step outputs not available)

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results

✅ GitHub API: Confirmed 2 PR entries in recent-prs.json
✅ Playwright: GitHub homepage title contains "GitHub"
✅ File verify: Verification file exists at /tmp/gh-aw/agent/smoke-test-claude-26064198640.txt

Result: PASS — All smoke tests passed.

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: FAIL

Merged PRs: ✅ chore: recompile all workflow lock files; refactor: split token-tracker.js into four focused modules
Safeinputs GH: ❌ tool unavailable
Playwright: ✅ GitHub title verified
Tavily: ❌ no tools advertised / search rejected
File + bash: ✅ file created and read
Discussion: ✅ commented on #3362
Build: ✅ npm ci && npm run build

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

Smoke test FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • localhost

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

💎 Faceted by Smoke Gemini

@github-actions
Copy link
Copy Markdown
Contributor

Chroot Smoke Test Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ NO
Node.js v24.15.0 v20.20.2 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot.

Tested by Smoke Chroot

@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx passed ✅ PASS
Node.js execa passed ✅ PASS
Node.js p-limit passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #3361 · ● 6.5M ·

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: GitHub Actions Services Connectivity

Check Result
Redis PING ❌ Timeout/no response
PostgreSQL pg_isready ❌ No response
PostgreSQL SELECT 1 ❌ Timeout/no response

Overall: FAILhost.docker.internal is unreachable from this runner environment. Service containers are not accessible.

🔌 Service connectivity validated by Smoke Services

@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 18, 2026

@copilot address review feedback

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 18, 2026

@copilot address review feedback

I checked this PR and don’t see any actionable review comments or unresolved review threads to address right now. Please share the specific feedback/thread you want me to apply, and I’ll update it right away.

Copilot finished work on behalf of lpcox May 18, 2026 22:58
@lpcox lpcox merged commit 3283a09 into main May 18, 2026
66 of 73 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-code-container-cleanup branch May 18, 2026 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] Log-directory preservation pattern repeated four times in container-cleanup.ts

3 participants