Skip to content

feat: include api-proxy token logs in firewall audit artifact#1549

Merged
lpcox merged 1 commit intomainfrom
feat/token-logs-artifact
Apr 1, 2026
Merged

feat: include api-proxy token logs in firewall audit artifact#1549
lpcox merged 1 commit intomainfrom
feat/token-logs-artifact

Conversation

@lpcox
Copy link
Copy Markdown
Collaborator

@lpcox lpcox commented Apr 1, 2026

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 existing firewall-audit-logs artifact upload in GitHub Actions workflows.

Before:

/tmp/gh-aw/sandbox/firewall/
├── logs/              ← uploaded as artifact ✅
│   └── access.log
├── audit/             ← uploaded as artifact ✅
│   └── audit.jsonl
└── api-proxy-logs/    ← NOT uploaded ❌
    └── token-usage.jsonl

Fix

Move api-proxy logs from a sibling to a subdirectory inside proxyLogsDir:

After:

/tmp/gh-aw/sandbox/firewall/
├── logs/              ← uploaded as artifact ✅
│   ├── access.log
│   └── api-proxy-logs/  ← now included automatically ✅
│       └── token-usage.jsonl
└── audit/             ← uploaded as artifact ✅
    └── audit.jsonl

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: Changed apiProxyLogsPath from path.join(path.dirname(proxyLogsDir), 'api-proxy-logs') to path.join(proxyLogsDir, 'api-proxy-logs') in 3 locations (compose generation, writeConfigs, cleanup)
  • src/docker-manager.test.ts: Updated 3 tests to match new subdirectory path

Testing

  • 275/278 docker-manager tests pass (3 pre-existing failures unrelated)
  • Behavior unchanged when --proxy-logs-dir is not specified (uses workDir/api-proxy-logs as before)

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>
@lpcox lpcox requested a review from Mossaka as a code owner April 1, 2026 15:48
Copilot AI review requested due to automatic review settings April 1, 2026 15:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 82.67% 82.77% 📈 +0.10%
Statements 82.34% 82.43% 📈 +0.09%
Functions 81.22% 81.22% ➡️ +0.00%
Branches 75.94% 76.00% 📈 +0.06%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 85.8% → 86.2% (+0.41%) 85.3% → 85.7% (+0.40%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

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

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/apiProxyLogsDir to use path.join(proxyLogsDir, 'api-proxy-logs') when proxyLogsDir is 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.

Comment on lines 1606 to 1610
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 });
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

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);
    }

Copilot uses AI. Check for mistakes.
const apiProxyLogsDir = path.join(proxyLogsDir, 'api-proxy-logs');
if (fs.existsSync(apiProxyLogsDir)) {
try {
execa.sync('chmod', ['-R', 'a+rX', apiProxyLogsDir]);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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]);
}

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

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
✅ Playwright — github.com title contains "GitHub"
✅ File write — /tmp/gh-aw/agent/smoke-test-claude-23857526368.txt created
✅ Bash verify — file contents confirmed

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude for issue #1549

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

🤖 Smoke test results for @lpcox:

Overall: PASS

📰 BREAKING: Report filed by Smoke Copilot for issue #1549

@github-actions github-actions bot mentioned this pull request Apr 1, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ No
Node.js v24.14.0 v20.20.1 ❌ No
Go go1.22.12 go1.22.12 ✅ Yes

Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot for issue #1549

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

Smoke test matrix:

  • PR: fix: disable IPv6 in agent container to prevent squid proxy bypass
  • PR: feat: add token usage tracking to api-proxy sidecar
  • GitHub MCP merged PR review: ✅
  • safeinputs-gh PR query: ❌
  • Playwright title contains "GitHub": ❌
  • Tavily web search result present: ❌
  • File write + bash cat + npm ci/build: ✅
  • Discussion oracle comment: ❌
    Overall status: FAIL

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

Smoke Test: GitHub Actions Services Connectivity ✅

All checks passed.

Service Check Result
Redis host.docker.internal:6379 PING PONG
PostgreSQL host.docker.internal:5432 pg_isready ✅ accepting connections
PostgreSQL smoketest db SELECT 1 ✅ returned 1

Note: redis-cli was not available (no apt access in sandbox), but Redis connectivity was verified via raw socket (+PONG response).

🔌 Service connectivity validated by Smoke Services

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

🏗️ 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 All passed ✅ PASS
Node.js execa All passed ✅ PASS
Node.js p-limit All passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Notes
  • Java: Maven local repo was root-owned (/home/runner/.m2/ owned by root), so -Dmaven.repo.local=/tmp/gh-aw/agent/m2-repo was used to work around the permission issue. Both gson and caffeine compiled and tested successfully.
  • C++: Both fmt and json built successfully with CMake + make (no test targets defined, build success recorded as pass).
  • .NET: hello-world printed Hello, World!; json-parse parsed and printed JSON correctly.

Generated by Build Test Suite for issue #1549 ·

@lpcox lpcox merged commit 505e51e into main Apr 1, 2026
68 of 69 checks passed
@lpcox lpcox deleted the feat/token-logs-artifact branch April 1, 2026 16:13
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.

2 participants