feat: include api-proxy token logs in firewall audit artifact#1549
feat: include api-proxy token logs in firewall audit artifact#1549
Conversation
Move api-proxy logs from a sibling directory of proxyLogsDir to a subdirectory inside it. When --proxy-logs-dir is set (e.g., by gh-aw to /tmp/gh-aw/sandbox/firewall/logs), the api-proxy logs now land at .../logs/api-proxy-logs/ instead of .../api-proxy-logs/. This means the existing firewall-audit-logs artifact upload automatically captures token-usage.jsonl without any workflow changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR ensures api-proxy token usage logs (token-usage.jsonl) are captured by the existing “firewall-audit-logs” GitHub Actions artifact upload by relocating api-proxy logs from a sibling directory to a subdirectory within proxyLogsDir.
Changes:
- Update
apiProxyLogsPath/apiProxyLogsDirto usepath.join(proxyLogsDir, 'api-proxy-logs')whenproxyLogsDiris specified (compose generation, config writing, cleanup). - Update docker-manager tests to assert the new subdirectory location.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/docker-manager.ts |
Writes api-proxy logs under proxyLogsDir/api-proxy-logs and adjusts cleanup permission handling accordingly. |
src/docker-manager.test.ts |
Updates unit tests to match the new api-proxy logs path behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const apiProxyLogsDir = config.proxyLogsDir | ||
| ? path.join(path.dirname(config.proxyLogsDir), 'api-proxy-logs') | ||
| ? path.join(config.proxyLogsDir, 'api-proxy-logs') | ||
| : path.join(config.workDir, 'api-proxy-logs'); | ||
| if (!fs.existsSync(apiProxyLogsDir)) { | ||
| fs.mkdirSync(apiProxyLogsDir, { recursive: true, mode: 0o777 }); |
There was a problem hiding this comment.
writeConfigs now creates api-proxy-logs inside proxyLogsDir. If proxyLogsDir already exists but is not writable by the current AWF process (common when a previous root run chowned it to the Squid UID 13 with mode 0755), fs.mkdirSync(apiProxyLogsDir) will throw and abort the run. Consider catching EACCES here and either (a) ensuring proxyLogsDir is made writable (chmod/chown when possible) before creating the subdir, or (b) falling back to the previous sibling-directory location with a warning so non-root runs don’t regress.
See below for a potential fix:
let apiProxyLogsDir = config.proxyLogsDir
? path.join(config.proxyLogsDir, 'api-proxy-logs')
: path.join(config.workDir, 'api-proxy-logs');
if (!fs.existsSync(apiProxyLogsDir)) {
let created = false;
// When proxyLogsDir is configured, prefer creating the subdirectory inside it.
// If permissions are too restrictive (e.g., owned by Squid UID 13, mode 0755),
// handle EACCES gracefully and fall back to a sibling directory under workDir.
if (config.proxyLogsDir) {
try {
fs.mkdirSync(apiProxyLogsDir, { recursive: true, mode: 0o777 });
fs.chmodSync(apiProxyLogsDir, 0o777);
created = true;
} catch (err) {
const e = err as NodeJS.ErrnoException;
if (e.code === 'EACCES') {
// Try to relax permissions on proxyLogsDir and retry once
try {
fs.chmodSync(config.proxyLogsDir, 0o777);
fs.mkdirSync(apiProxyLogsDir, { recursive: true, mode: 0o777 });
fs.chmodSync(apiProxyLogsDir, 0o777);
created = true;
} catch (err2) {
const e2 = err2 as NodeJS.ErrnoException;
if (e2.code !== 'EACCES') {
// Non-permission error: rethrow to avoid masking real problems
throw err2;
}
// Fall back to previous behavior: use workDir/api-proxy-logs as a sibling directory.
const fallbackDir = path.join(config.workDir, 'api-proxy-logs');
apiProxyLogsDir = fallbackDir;
if (!fs.existsSync(fallbackDir)) {
fs.mkdirSync(fallbackDir, { recursive: true, mode: 0o777 });
fs.chmodSync(fallbackDir, 0o777);
}
logger.warn(
`proxyLogsDir (${config.proxyLogsDir}) is not writable; ` +
`using fallback API proxy logs directory at: ${fallbackDir}. ` +
`Original error: ${e2.message}`
);
created = true;
}
} else {
// Unexpected error: rethrow
throw err;
}
}
}
// If proxyLogsDir is not set or creation above did not run, create under workDir.
if (!created && !fs.existsSync(apiProxyLogsDir)) {
fs.mkdirSync(apiProxyLogsDir, { recursive: true, mode: 0o777 });
// Explicitly set permissions to 0o777 (not affected by umask)
fs.chmodSync(apiProxyLogsDir, 0o777);
}
| const apiProxyLogsDir = path.join(proxyLogsDir, 'api-proxy-logs'); | ||
| if (fs.existsSync(apiProxyLogsDir)) { | ||
| try { | ||
| execa.sync('chmod', ['-R', 'a+rX', apiProxyLogsDir]); |
There was a problem hiding this comment.
In cleanup(), when proxyLogsDir is set you chmod apiProxyLogsDir, and later in the same function you also chmod proxyLogsDir recursively. Since apiProxyLogsDir is a subdirectory of proxyLogsDir now, the second chmod already covers it; consider removing the extra chmod call (or skipping it when it’s nested) to avoid redundant chmod -R executions.
| execa.sync('chmod', ['-R', 'a+rX', apiProxyLogsDir]); | |
| // If apiProxyLogsDir is not nested under proxyLogsDir, fix its permissions explicitly. | |
| // Otherwise, rely on the later recursive chmod on proxyLogsDir to cover it. | |
| const relative = path.relative(proxyLogsDir, apiProxyLogsDir); | |
| const isNested = | |
| relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative); | |
| if (!isNested) { | |
| execa.sync('chmod', ['-R', 'a+rX', apiProxyLogsDir]); | |
| } |
|
Smoke test results — run 23857526368 ✅ GitHub MCP — last 2 closed PRs: #1544 fix: disable IPv6 in agent container, #1520 [WIP] Fix permissions for gh-aw config files Overall: PASS
|
|
🤖 Smoke test results for
Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
|
Smoke test matrix:
|
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed.
Note:
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS Notes
|
Problem
The api-proxy token usage logs (
token-usage.jsonl) are written to a sibling directory of--proxy-logs-dir, so they are NOT captured by the existingfirewall-audit-logsartifact upload in GitHub Actions workflows.Before:
Fix
Move api-proxy logs from a sibling to a subdirectory inside
proxyLogsDir:After:
No workflow changes needed — the existing artifact upload of
/tmp/gh-aw/sandbox/firewall/logs/now captures token usage logs automatically.Changes
src/docker-manager.ts: ChangedapiProxyLogsPathfrompath.join(path.dirname(proxyLogsDir), 'api-proxy-logs')topath.join(proxyLogsDir, 'api-proxy-logs')in 3 locations (compose generation, writeConfigs, cleanup)src/docker-manager.test.ts: Updated 3 tests to match new subdirectory pathTesting
--proxy-logs-diris not specified (usesworkDir/api-proxy-logsas before)