Skip to content

Complete log path migration to StateDirResolver for adapters#22

Closed
stevendinggang wants to merge 1 commit intoraysonmeng:feat/bootstrap-commandfrom
stevendinggang:fix/complete-log-path-migration
Closed

Complete log path migration to StateDirResolver for adapters#22
stevendinggang wants to merge 1 commit intoraysonmeng:feat/bootstrap-commandfrom
stevendinggang:fix/complete-log-path-migration

Conversation

@stevendinggang
Copy link
Copy Markdown

Summary

Completes the logging path migration that was partially done in feat/bootstrap-command.

  • codex-adapter.ts and claude-adapter.ts still hardcoded /tmp/agentbridge.log
  • bridge.ts and daemon.ts had already been migrated to stateDir.logFile

This PR finishes the migration so all four components use the same resolved log path.

Closes #21

Changes

  • src/codex-adapter.ts: Remove hardcoded LOG_FILE, accept logFile via constructor (defaults to StateDirResolver().logFile)
  • src/claude-adapter.ts: Same pattern — remove LOG_FILE constant, accept injectable logFile
  • src/daemon.ts: Pass stateDir.logFile to CodexAdapter
  • src/bridge.ts: Add stateDir.ensure(), pass stateDir.logFile to ClaudeAdapter

Design notes

  • Constructor defaults use new StateDirResolver().logFile so existing callers (including tests) work without changes
  • No new dependencies — StateDirResolver was already in the codebase
  • Backward compatible — zero test changes needed

Test plan

  • bun run typecheck passes
  • bun test src passes

🤖 Generated with Claude Code

@raysonmeng
Copy link
Copy Markdown
Owner

Code Review / 代码审查

Thanks for this contribution! The log path migration logic is correct and well-structured.

感谢你的贡献!日志路径迁移的逻辑正确,结构清晰。

Approved with requested changes / 批准,需要以下修改

1. Rebase required / 需要 Rebase

This PR was created before PR #38 (server-to-client request passthrough) was merged. codex-adapter.ts has changed significantly since then — new PendingServerRequest interface, handleServerRequest() method, serverRequestToProxy map, etc. Please rebase onto current master and resolve conflicts.

这个 PR 创建时 PR #38 还没合并。codex-adapter.ts 之后有较大改动(新增 PendingServerRequest 接口、handleServerRequest() 方法等)。请 rebase 到最新 master 并解决冲突。

2. Base branch / 基础分支

The PR targets feat/bootstrap-command instead of master. Please change the base branch to master.

PR 的目标分支是 feat/bootstrap-command 而非 master,请修改为 master

3. Suggestion: constructor default / 建议:构造函数默认值

The default logFile = new StateDirResolver().logFile works but creates unnecessary coupling. Since both callers (bridge.ts and daemon.ts) always pass logFile explicitly, consider either making it required or using a simpler fallback:

默认值 logFile = new StateDirResolver().logFile 可以工作但引入了不必要的耦合。因为调用方都会显式传入 logFile,建议改为必传参数或使用更简单的 fallback:

// Option A: required parameter / 必传参数
constructor(logFile: string)

// Option B: simple fallback / 简单 fallback
constructor(logFile = "/tmp/agentbridge.log")

After rebasing and addressing these points, this is ready to merge!
Rebase 并修改后即可合并!

🤖 Reviewed by Claude + Codex via AgentBridge

@raysonmeng
Copy link
Copy Markdown
Owner

Superseded by PR #63 — 直接基于 master 重做,避免 feat/bootstrap-command 的冲突。

@raysonmeng raysonmeng closed this Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants