Remove duplicate logging in session.go, fix logUnified misuse#2788
Remove duplicate logging in session.go, fix logUnified misuse#2788
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/40199423-b62b-412a-8fcd-11b74d8467cf Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…sion directory creation Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/40199423-b62b-412a-8fcd-11b74d8467cf Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Removes duplicate session-related log entries and aligns session directory logging to use the session-scoped logger, reducing noisy/double-written logs in the server session lifecycle.
Changes:
- Remove duplicate
log.Printflines ingetSessionIDandrequireSession. - Switch
ensureSessionDirectoryfromlogUnified/stdlib log duplicates tologSessiononly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, err := os.Stat(sessionDir); err == nil { | ||
| // Directory already exists | ||
| logUnified.Printf("Session directory already exists: %s", sessionDir) | ||
| logSession.Printf("Session directory already exists: %s", sessionDir) | ||
| return nil |
There was a problem hiding this comment.
sessionDir includes the raw sessionID (which is the API key per spec) and is being logged verbatim. This can leak credentials into logs; prefer logging auth.TruncateSessionID(sessionID) and avoid printing the full path that embeds the secret.
| if err := os.MkdirAll(sessionDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create session directory: %w", err) | ||
| } | ||
|
|
||
| logUnified.Printf("Created session directory: %s", sessionDir) | ||
| log.Printf("Created payload directory for session: %s", auth.TruncateSessionID(sessionID)) | ||
| logSession.Printf("Created session directory: %s", sessionDir) |
There was a problem hiding this comment.
sessionID comes from the client and is used directly in filepath.Join(us.payloadDir, sessionID) and os.MkdirAll(sessionDir, ...). As written, a session ID containing .. (or an absolute path) can escape payloadDir and create directories elsewhere. Consider normalizing/validating the session ID (e.g., reject path traversal segments / absolute paths) and/or verify after filepath.Clean/filepath.Abs that the resulting path stays under us.payloadDir before calling MkdirAll.
session.gohad three dual-logging antipatterns where the same event was written to bothlogSession/logUnified(debug logger) andlog(stdlib), causing double log entries and split authority over session log messages.Changes
getSessionID): droppedlog.Printf("Extracted session ID from context: …")— identical to thelogSession.Printfabove itrequireSession): droppedlog.Printf("Checking session for ID: …")— same event aslogSession.PrintfensureSessionDirectory): droppedlog.Printf("Created payload directory for session: …")— redundant after switching tologSessionensureSessionDirectory): changedlogUnified.Printf(…)→logSession.Printf(…)— session-scoped events belong on the session logger, not the unified server loggerBefore:
After:
Non-duplicate
log.Printfcalls (no session ID in context, auto-creating/auto-created session, session validated) are retained.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build3212929701/b334/launcher.test /tmp/go-build3212929701/b334/launcher.test -test.testlogfile=/tmp/go-build3212929701/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� tion_pool.go er.go x_amd64/compile(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build3212929701/b319/config.test /tmp/go-build3212929701/b319/config.test -test.testlogfile=/tmp/go-build3212929701/b319/testlog.txt -test.paniconexit0 -test.timeout=10m0s is ternal/engine/wazevo/wazevoapi/debug_options.go ternal/engine/wazevo/wazevoapi/exitcode.go x_amd64/compile(dns block)nonexistent.local/tmp/go-build3212929701/b334/launcher.test /tmp/go-build3212929701/b334/launcher.test -test.testlogfile=/tmp/go-build3212929701/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� tion_pool.go er.go x_amd64/compile(dns block)slow.example.com/tmp/go-build3212929701/b334/launcher.test /tmp/go-build3212929701/b334/launcher.test -test.testlogfile=/tmp/go-build3212929701/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� tion_pool.go er.go x_amd64/compile(dns block)this-host-does-not-exist-12345.com/tmp/go-build3212929701/b343/mcp.test /tmp/go-build3212929701/b343/mcp.test -test.testlogfile=/tmp/go-build3212929701/b343/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3212929701/b334/_pkg_.a ache/go/1.25.8/x64/src/runtime/c-c=4 73cQFc0nv x_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.