Skip to content

fix(session): sanitize '/' and '\' in session keys so forum topic key…#1330

Merged
alexhoshina merged 1 commit intosipeed:mainfrom
statxc:fix/session-key-sanitize-slash
Mar 11, 2026
Merged

fix(session): sanitize '/' and '\' in session keys so forum topic key…#1330
alexhoshina merged 1 commit intosipeed:mainfrom
statxc:fix/session-key-sanitize-slash

Conversation

@statxc
Copy link
Contributor

@statxc statxc commented Mar 10, 2026

📝 Description

Session keys can contain a slash when they represent a composite ID, for example:

  • Telegram forum topics: agent:main:telegram:group:-1003822706455/12 (chat ID + thread ID)
  • Slack threads: agent:main:slack:channel:C01234/1234567890.123456 (channel + thread TS)

Previously, only the colon (:) was replaced with _ when building session file paths. The slash was left as-is, so the path was interpreted as a directory separator and the code tried to open a file like sessions/agent_main_telegram_group_-1003822706455/12.jsonl. The subdirectory was never created, which led to "no such file or directory" when adding messages.

This PR updates the session key sanitization in both the JSONL memory store and the legacy session manager: / and \ are now replaced with _ so that:

  • Session files always live in a single flat directory under sessions/.
  • Composite IDs (forum topics, Slack threads) work without path errors.
  • Behaviour stays safe on Windows where \ is a path separator.

The logical session key (e.g. agent:main:telegram:group:-1003822706455/12) is unchanged in memory and in the bus; only the filename used on disk is sanitized.

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

#1270

📚 Technical Context (Skip for Docs)

  • Reference URL: N/A
  • Reasoning: Session keys are turned into filenames by replacing unsafe characters. Colons were already sanitized for cross-platform paths; slashes were not, so composite IDs used by Telegram forum topics and Slack threads produced invalid paths. Sanitizing / and \ keeps a single-file-per-session layout and fixes the error.

🧪 Test Environment

  • OS: Ubuntu
  • Model/Provider: optional
  • Channels: Telegram (forum topics), Slack (threads) — any channel using composite chat IDs

📸 Evidence (Optional)

Click to view Logs/Screenshots

Before: open .picoclaw/workspace/sessions/agent_main_telegram_group_-1003822706455/12.jsonl: no such file or directory
After: session file is created as agent_main_telegram_group_-1003822706455_12.jsonl under sessions/ and messages are stored correctly.

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly. (No user-facing docs change required; behaviour is internal.)

@statxc
Copy link
Contributor Author

statxc commented Mar 10, 2026

@alexhoshina @dimonb I'd appreciate you review this PR

@sipeed-bot sipeed-bot bot added type: bug Something isn't working go Pull requests that update go code labels Mar 10, 2026
Copy link

@nikolasdehor nikolasdehor left a comment

Choose a reason for hiding this comment

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

Clean, well-scoped fix. The session key sanitization for '/' and '\' is the right approach to prevent composite IDs (Telegram forum topics, Slack threads) from being interpreted as directory separators.

A few notes:

  1. Collision risk acknowledged -- the existing comment about lossy mapping ("telegram:123" vs "telegram_123") was removed. The same concern applies to '/' being flattened to '_'. Two keys like "group:-100/12" and "group:-100_12" would now map to the same file. This is acceptable given the real-world key formats, but worth keeping in mind.

  2. Test coverage is good -- the new test case for the Telegram forum topic key and the updated path traversal tests (now verifying that "foo/bar" is sanitized rather than rejected) are exactly right.

  3. Both code paths updated -- good that both pkg/memory/jsonl.go and pkg/session/manager.go were updated in lockstep, maintaining the mirror contract mentioned in the comments.

  4. The removal of the strings.ContainsAny(filename, "/\\") guard in Save() is correct since sanitizeFilename now handles this before the check.

LGTM.

@statxc
Copy link
Contributor Author

statxc commented Mar 10, 2026

Now telegram forum works well without any issues. Me and @dimonb confirmed. It's okay to merge this PR if code has no problem anymore. Thanks

Copy link
Collaborator

@alexhoshina alexhoshina left a comment

Choose a reason for hiding this comment

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

LGTM

@alexhoshina alexhoshina merged commit 755fa32 into sipeed:main Mar 11, 2026
4 checks passed
@statxc statxc deleted the fix/session-key-sanitize-slash branch March 11, 2026 10:54
fishtrees pushed a commit to fishtrees/picoclaw that referenced this pull request Mar 12, 2026
…lash

fix(session): sanitize '/' and '\' in session keys so forum topic key…
@Orgmar
Copy link
Contributor

Orgmar commented Mar 13, 2026

@statxc Great investigation on this one. Composite IDs from Telegram forum topics and Slack threads breaking session file paths is the kind of bug that's tricky to catch until it hits production. Sanitizing both / and \ while keeping the logical key unchanged is the right approach.

We're running a PicoClaw Dev Group on Discord for contributors to connect. If you'd like to join, email [email protected] with the subject [Join PicoClaw Dev Group] + your GitHub account and we'll send you the invite!

dj-oyu pushed a commit to dj-oyu/picoclaw that referenced this pull request Mar 14, 2026
…lash

fix(session): sanitize '/' and '\' in session keys so forum topic key…
dj-oyu pushed a commit to dj-oyu/picoclaw that referenced this pull request Mar 16, 2026
…lash

fix(session): sanitize '/' and '\' in session keys so forum topic key…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update go code type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants