fix: Remove sys___init requirement - sessions always auto-created#831
fix: Remove sys___init requirement - sessions always auto-created#831
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the requirement to call sys___init before other tool calls by making session creation automatic regardless of the DIFC setting, aligning behavior with standard MCP client expectations.
Changes:
- Update
UnifiedServer.requireSession()to always auto-create missing sessions (including when DIFC is enabled). - Adjust unit tests and documentation/flag help text to reflect the new session behavior.
- Pin
ghcr.io/github/gh-aw-mcpgimage in select locked workflows tov0.0.103instead oflatest.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/unified.go | Always auto-creates sessions in requireSession() and creates per-session payload directories. |
| internal/server/unified_test.go | Updates session-requirement tests to assert auto-creation behavior under DIFC. |
| internal/cmd/flags_difc.go | Updates --enable-difc flag description to remove sys init requirement language. |
| config.example.toml | Updates DIFC configuration comment to reflect policy enforcement behavior. |
| README.md | Updates CLI flag documentation to match new DIFC/session semantics. |
| .github/workflows/nightly-mcp-stress-test.lock.yml | Pins gh-aw-mcpg image to v0.0.103. |
| .github/workflows/large-payload-tester.lock.yml | Pins gh-aw-mcpg image to v0.0.103. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if session == nil { | ||
| log.Printf("Session not found for ID: %s. Available sessions: %v", sessionID, us.getSessionKeys()) | ||
| return fmt.Errorf("sys___init must be called before any other tool calls") | ||
| // Need to create session - acquire write lock | ||
| us.sessionMu.Lock() | ||
| // Double-check after acquiring write lock to avoid race condition | ||
| if us.sessions[sessionID] == nil { | ||
| log.Printf("Auto-creating session for ID: %s", sessionID) | ||
| us.sessions[sessionID] = NewSession(sessionID, "") | ||
| log.Printf("Session auto-created for ID: %s", sessionID) | ||
|
|
||
| // Ensure session directory exists in payload mount point | ||
| // This is done after releasing the lock to avoid holding it during I/O | ||
| us.sessionMu.Unlock() | ||
| if err := us.ensureSessionDirectory(sessionID); err != nil { | ||
| logger.LogWarn("client", "Failed to create session directory for session=%s: %v", sessionID, err) | ||
| // Don't fail - payloads will attempt to create the directory when needed | ||
| } | ||
| return nil |
There was a problem hiding this comment.
requireSession() now auto-creates sessions and calls ensureSessionDirectory(sessionID) based on the session ID from context. Since ensureSessionDirectory() uses filepath.Join(us.payloadDir, sessionID) without validating/sanitizing sessionID, a malicious or malformed session ID containing path separators, .., or an absolute path could cause directory creation outside the intended payload directory. Please validate the session ID before using it in filesystem paths (e.g., reject absolute paths, clean and ensure the result stays within us.payloadDir, and/or restrict to a safe character set).
| if session == nil { | ||
| log.Printf("Session not found for ID: %s. Available sessions: %v", sessionID, us.getSessionKeys()) | ||
| return fmt.Errorf("sys___init must be called before any other tool calls") | ||
| // Need to create session - acquire write lock | ||
| us.sessionMu.Lock() | ||
| // Double-check after acquiring write lock to avoid race condition | ||
| if us.sessions[sessionID] == nil { | ||
| log.Printf("Auto-creating session for ID: %s", sessionID) | ||
| us.sessions[sessionID] = NewSession(sessionID, "") | ||
| log.Printf("Session auto-created for ID: %s", sessionID) | ||
|
|
||
| // Ensure session directory exists in payload mount point | ||
| // This is done after releasing the lock to avoid holding it during I/O | ||
| us.sessionMu.Unlock() | ||
| if err := us.ensureSessionDirectory(sessionID); err != nil { | ||
| logger.LogWarn("client", "Failed to create session directory for session=%s: %v", sessionID, err) | ||
| // Don't fail - payloads will attempt to create the directory when needed | ||
| } | ||
| return nil |
There was a problem hiding this comment.
With sessions now always auto-created, any client that can influence the session ID can force unbounded growth of us.sessions and corresponding directories under the payload mount (disk usage / inode exhaustion). Consider adding a cap/TTL cleanup for us.sessions, or deferring directory creation until a payload actually needs to be written, to reduce DoS risk.
| await determineAutomaticLockdown(github, context, core); | ||
| - name: Download container images | ||
| run: bash /opt/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.13.12 ghcr.io/github/gh-aw-firewall/squid:0.13.12 ghcr.io/github/gh-aw-mcpg:latest ghcr.io/github/github-mcp-server:v0.30.2 ghcr.io/github/github-mcp-server:v0.30.3 mcp/brave-search mcp/duckduckgo mcp/everart mcp/fetch mcp/filesystem mcp/gdrive mcp/git mcp/google-maps mcp/hackernews-mcp mcp/kubernetes mcp/memory mcp/puppeteer mcp/sentry mcp/sequentialthinking mcp/slack mcp/sqlite mcp/time mcp/wikipedia-mcp mcp/youtube-transcript mcr.microsoft.com/playwright/mcp mcr.microsoft.com/playwright:v1.49.1-noble node:lts-alpine | ||
| run: bash /opt/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.13.12 ghcr.io/github/gh-aw-firewall/squid:0.13.12 ghcr.io/github/gh-aw-mcpg:v0.0.103 ghcr.io/github/github-mcp-server:v0.30.2 ghcr.io/github/github-mcp-server:v0.30.3 mcp/brave-search mcp/duckduckgo mcp/everart mcp/fetch mcp/filesystem mcp/gdrive mcp/git mcp/google-maps mcp/hackernews-mcp mcp/kubernetes mcp/memory mcp/puppeteer mcp/sentry mcp/sequentialthinking mcp/slack mcp/sqlite mcp/time mcp/wikipedia-mcp mcp/youtube-transcript mcr.microsoft.com/playwright/mcp mcr.microsoft.com/playwright:v1.49.1-noble node:lts-alpine |
There was a problem hiding this comment.
This workflow pins ghcr.io/github/gh-aw-mcpg to v0.0.103, but other locked workflows in the repo still reference ghcr.io/github/gh-aw-mcpg:latest (e.g. smoke-copilot.lock.yml). This inconsistency can make CI behavior drift across workflows; consider updating the remaining workflows to the same pinned version (or centralizing the version in one place) so tests run against the same gateway image.
| await determineAutomaticLockdown(github, context, core); | ||
| - name: Download container images | ||
| run: bash /opt/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.13.12 ghcr.io/github/gh-aw-firewall/squid:0.13.12 ghcr.io/github/gh-aw-mcpg:latest ghcr.io/github/github-mcp-server:v0.30.2 ghcr.io/github/github-mcp-server:v0.30.3 mcp/filesystem node:lts-alpine | ||
| run: bash /opt/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.13.12 ghcr.io/github/gh-aw-firewall/squid:0.13.12 ghcr.io/github/gh-aw-mcpg:v0.0.103 ghcr.io/github/github-mcp-server:v0.30.2 ghcr.io/github/github-mcp-server:v0.30.3 mcp/filesystem node:lts-alpine |
There was a problem hiding this comment.
This workflow pins ghcr.io/github/gh-aw-mcpg to v0.0.103, but other locked workflows in the repo still reference ghcr.io/github/gh-aw-mcpg:latest (e.g. smoke-copilot.lock.yml). This inconsistency can make CI behavior drift across workflows; consider updating the remaining workflows to the same pinned version (or centralizing the version in one place) so tests run against the same gateway image.
|
fix: Remove sys___init requirement - sessions always auto-created ✅
|
1 participant
No description provided.