Repair Squid log ownership in-container before reading logs and teardown - #8251
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The test does not currently prove that permission repair occurs before access.log is read.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Repairs Squid log ownership and permissions before diagnostics and teardown, improving log access and artifact preservation.
Changes:
- Adds reusable in-container
chown/chmodrepair. - Runs repair before log inspection and shutdown.
- Makes post-preservation
chmodbest-effort. - Updates related tests and documentation.
File summaries
| File | Description |
|---|---|
src/run-agent-command.test.ts |
Tests lifecycle repair behavior; ordering assertion must verify repair precedes log reads. |
src/log-directory-setup.ts |
Updates permission-layer comments. |
src/container-stop.ts |
Implements reusable permission repair. |
src/container-stop.test.ts |
Updates shutdown repair tests. |
src/container-stop-coverage.test.ts |
Updates branch coverage expectations. |
src/container-lifecycle.ts |
Repairs logs before inspection. |
src/artifact-preservation.ts |
Makes post-rename permission repair best-effort. |
src/artifact-preservation-errors.test.ts |
Tests preservation despite denied chmod. |
docs/logging_quickref.md |
Documents both permission-repair points. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| expect(mockExecaFn).toHaveBeenCalledWith( | ||
| 'docker', | ||
| expect.arrayContaining([ | ||
| 'exec', | ||
| '--user', | ||
| 'root', | ||
| SQUID_CONTAINER_NAME, | ||
| 'sh', | ||
| '-c', | ||
| 'chown -R "$TUID:$TGID" /var/log/squid 2>/dev/null; chmod -R a+rX /var/log/squid', | ||
| ]), | ||
| expect.objectContaining({ reject: false }), | ||
| ); |
|
@copilot address review feedback |
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
✅ Build Test Suite completed successfully!
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
❌ Smoke Gemini reports failed. Facets need polishing...
|
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.
|
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
❌ Contribution Check failed. Please review the logs for details.
|
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
|
🔌 Smoke Services — All services reachable! ✅
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation...
|
|
❌ Smoke Claude failed Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.anthropic.com"See Network Configuration for more information.
|
github-actions
Bot
added
the
smoke-copilot-network-isolation
|
EGRESS_RESULT allow=pass deny=pass ✅ Allowed domain (api.github.com): reachable (HTTP 200) Overall: PASS cc Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
|
Smoke Test Results —
Overall: PASS
|
|
Smoke Test Results: GitHub Actions Services Connectivity
Overall: FAIL — DNS resolution for
|
|
|
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 93.97% | 93.95% | 📉 -0.02% |
| Statements | 92.62% | 92.59% | 📉 -0.03% |
| Functions | 93.25% | 93.20% | 📉 -0.05% |
| Branches | 86.45% | 86.35% | 📉 -0.10% |
📁 Per-file Coverage Changes (5 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/enclave/dynamic-registry.ts |
95.7% → 92.6% (-3.18%) | 94.3% → 90.9% (-3.36%) |
src/artifact-preservation.ts |
95.2% → 94.6% (-0.59%) | 95.2% → 94.7% (-0.58%) |
src/container-lifecycle.ts |
96.7% → 96.7% (+0.02%) | 96.3% → 96.3% (+0.02%) |
src/enclave/manager.ts |
87.8% → 87.8% (+0.08%) | 86.2% → 86.3% (+0.09%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Chroot Runtime Version Comparison
Result: Not all tests passed — Node.js version differs between host and chroot environment, so the
|
Smoke Test: API Proxy OpenTelemetry Tracing — Results
Overall: All 5 scenarios passed. No unexpected failures found; OTEL tracing integration in the api-proxy sidecar is working end-to-end (module → tests → env propagation → hook point → export).
|
|
Smoke Test: Copilot BYOK (Direct) Mode ✅ PASS
All smoke tests passed. BYOK credential path confirmed working.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — PASS Notes:
|
|
Repair Squid log ownership in-container before reading logs and teardown
|
Squid writes its logs as UID 13, but AWF only repaired permissions inside the container at shutdown. Post-run diagnostics that read
access.logtherefore failed withEACCES, and host-sidechmodduring artifact preservation failed withOperation not permitted— masking the fact that the logs had actually been preserved:Changes
src/container-stop.ts—fixSquidLogPermissionsBeforeShutdown()is now the exported, reusablefixSquidLogPermissions(). It alsochowns to the runner UID/GID (passed viadocker exec -e, mirroringfixArtifactPermissionsForRootless()), not justchmod:Ownership transfer is what makes the later host-side
chmodviable;chmodalone left files owned by UID 13.src/container-lifecycle.ts—runAgentCommand()invokes the repair beforecheckSquidLogs(), while Squid is still running, so blocked-domain diagnostics can readaccess.log. The call is best-effort and non-throwing, so a failed repair cannot alter the agent exit code.src/artifact-preservation.ts— the post-renamechmodfor preserved Squid logs moved out of the rename'stryblock. Preservation is now reported accurately, and a deniedchmodis classified through the existingisBenignArtifactPermissionError()path and logged at debug.Docs/comments — layer 3 of the squid-log permission defense in
docs/logging_quickref.mdandsrc/log-directory-setup.tsupdated to reflect thechown+chmodrepair and its two call sites.Tests
Updated the existing
container-stopsuites for the new command shape, plus new cases: the repair runs beforeaccess.logis read, the agent exit code survives a failed repair, and Squid logs are still reported as preserved when the host-sidechmodis denied.