Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the .NET workflows file-system checkpoint store to safely persist checkpoints when sessionId contains path-forbidden/special characters by encoding the on-disk file name, and adjusts the persisted index format accordingly.
Changes:
- Encode checkpoint file names derived from
sessionId/checkpointIdto avoid invalid paths and directory traversal. - Change
index.jsonlentries to include bothCheckpointInfoand the corresponding on-disk file name. - Add/extend unit tests covering invalid/path traversal characters in
sessionId.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Workflows/Checkpointing/FileSystemJsonCheckpointStore.cs | Escapes checkpoint file names and writes a richer index entry format. |
| dotnet/src/Microsoft.Agents.AI.Workflows/WorkflowsJsonUtilities.cs | Adds source-gen JSON metadata for the new index entry type. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/FileSystemJsonCheckpointStoreTests.cs | Adds temp-dir helper and new tests for escaping/path traversal scenarios. |
You can also share your feedback on Copilot code review. Take the survey.
dotnet/src/Microsoft.Agents.AI.Workflows/Checkpointing/FileSystemJsonCheckpointStore.cs
Show resolved
Hide resolved
dotnet/src/Microsoft.Agents.AI.Workflows/Checkpointing/FileSystemJsonCheckpointStore.cs
Show resolved
Hide resolved
dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/FileSystemJsonCheckpointStoreTests.cs
Outdated
Show resolved
Hide resolved
dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/FileSystemJsonCheckpointStoreTests.cs
Show resolved
Hide resolved
5498b79 to
8d54402
Compare
The `sessionId`, an optional parameter when starting a new session when running a workflow is an arbitrary string. This allows consumers to support whatever ids are needed by other systems, but can result in errors when an OS special or forbidden character is included. The fix is to escape the paths, in a 1:1 manner. We rely on EncodeDataString to do this. * Also modifies the index file to make it easier to determine what the name of the file on disk is for a given `sessionId`.
8d54402 to
b66c61e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
The
sessionId, an optional parameter when starting a new session when running a workflow is an arbitrary string. This allows consumers to support whatever ids are needed by other systems, but can result in errors when an OS special or forbidden character is included.Description
The fix is to escape the paths, in a 1:1 manner. We rely on
Uri.EscapeDataStringto do this. This is a potential breaking change if users createdsessionIds containing DataString-encoded characters.sessionId.Contribution Checklist