fix(harness): sanitize session ids used in workspace file names - #2952
fix(harness): sanitize session ids used in workspace file names#2952wylovelyi wants to merge 1 commit into
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Buktal
left a comment
Buktal
left a comment
There was a problem hiding this comment.
静默替换有点隐式了， a:b 和 a-b 本来是两个不同的 sessionId，统一把保留字符换成 - 容易有 ID 冲突或者覆盖文件的风险
交给应用层来规避处理应该更合适一些
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @wylovelyi please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR replaces selected Windows-reserved characters in session-derived local filenames and adds path-name assertions. I verified the affected session and task persistence paths, but the lossy replacement aliases distinct session IDs and can mix their transcripts and task records; the raw ID also remains in the filesystem mirror path, and Windows control characters are still unhandled. These correctness and session-isolation issues need resolution before the change is safe to merge.
The concerns above are significant enough that this would normally be a change request; noting that the CLA is also not signed yet (see the CLA reminder comment), this review is posted as a comment. Please address the session-ID aliasing and mirror-path issues, and sign the CLA, then feel free to @mention me for a re-review.
Automated review by github-manager-bot
| * <p>Forward slash and backslash are included as well: an id is a single file-name segment | ||
| * here, so either would otherwise be read as a directory separator. | ||
| */ | ||
| private static final Pattern UNSAFE_SEGMENT_CHARS = Pattern.compile("[<>:\"/\\\\|?*]"); |
There was a problem hiding this comment.
The pattern omits ASCII control characters (U+0000 through U+001F), which Windows also forbids in file-name segments. A caller-supplied id such as a\u0001b is therefore unchanged and still fails when the Windows path is constructed. Include those characters in the transformation and cover them in the test.
| public Path resolveSessionContextFile(RuntimeContext rc, String agentId, String sessionId) { | ||
| return getSessionDir(rc, agentId) | ||
| .resolve(sessionId + WorkspaceConstants.SESSION_CONTEXT_EXT); | ||
| .resolve(safeSegment(sessionId) + WorkspaceConstants.SESSION_CONTEXT_EXT); |
There was a problem hiding this comment.
This only changes the local path. SessionTranscriptWriter still builds contextRelativePath from the raw session id, and SessionTree uses that override for mirror reads and uploads. With a Windows-backed LocalFilesystem, the raw colon still reaches Path.of(...); moreover the remote context key and local sanitized key now differ. Derive the mirror key from the same sanitized segment (and test a transcript write with a filesystem configured).
| * hint that the caller-supplied id is the problem. | ||
| */ | ||
| private static String safeSegment(String segment) { | ||
| return segment == null ? null : UNSAFE_SEGMENT_CHARS.matcher(segment).replaceAll("-"); |
There was a problem hiding this comment.
Replacing every unsafe character with - is lossy: distinct allowed IDs such as a:b and a-b resolve to the same context, log, and task-record files. Their transcript history and task maps are then shared (or task records overwritten), while sessions.json still has separate raw-ID entries. Use a collision-free filename encoding or a persisted ID-to-filename mapping.
4 participants
AgentScope-Java Version
2.0.3-SNAPSHOT(currentmain)Description
Closes #2937.
Background.
RuntimeContext.sessionIdis caller-supplied and carries no documentedcharacter constraint, yet
WorkspaceManageruses it verbatim to build file names(
agents/{agentId}/sessions/{sessionId}.jsonl,agents/{agentId}/tasks/{sessionId}.json).A namespaced id such as
agent:abc-123:main:main-9f3cis perfectly legal as an identifier,but the colon is reserved by Windows/NTFS, so path construction fails on Windows with a bare
InvalidPathExceptionthrown from deep inside the workspace layer — with no hint that thecaller-supplied id is the problem.
Reproduced locally on Windows 11 / JDK 17:
Change. Reserved characters are replaced with
-in the file-name segment rather thanrejected, so existing ids keep working unchanged on every platform:
UNSAFE_SEGMENT_CHARSpattern[<>:"/\\|?*]plus a smallsafeSegment(String)helpersessionIdbecomes part of a file name:resolveSessionFile,resolveSessionContextFile,resolveSessionLogFile,taskRecordPath/and\are included deliberately:sessionIdis a single file-name segment here, so eitherone would otherwise be read as a directory separator and silently write outside the session
directory. This follows the same approach as the already-merged #1031 and #2921, which stripped
Windows-reserved characters from skill source paths.
No on-disk data changes shape. Writes (
SessionTranscriptWriter) and reads(
SessionSearchTool) both go through these same helpers, so the sanitized name is symmetric —there is no way to write a transcript that can no longer be found. Ids that work today contain
no reserved character and are returned unchanged; ids that do contain one could not produce a
file on Windows in the first place, since they threw before any file was created.
Scope is kept to
sessionId, as the issue describes.agentIdreaches path construction thesame way and would benefit from the same treatment — happy to extend this PR if you would
prefer the two handled together.
How to test.
WorkspaceManagerPathSafetyTestgains two cases, both platform-independent(they assert the replacement behaviour rather than relying on a Windows-only crash):
sessionFileNamesReplaceWindowsReservedCharacters— the#2937id shape plus every reservedcharacter and both separators
sessionFileNamesLeaveOrdinaryIdsUnchanged— regression guard: an ordinary id is untouchedVerified locally on Windows 11 / JDK 17.0.18 / Maven 3.9.4:
mvn -pl agentscope-harness spotless:checkpasses, and the full harness suite(
mvn -pl agentscope-harness test) is green — 895 tests, 0 failures, 0 errors.Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)