-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add optional lastModified timestamp to Annotated #140
base: main
Are you sure you want to change the base?
feat: add optional lastModified timestamp to Annotated #140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
Can you share a bit more about how you might use this to truncate context, just so I have a greater understanding of the use case? I think it probably makes sense, but would love more detail.
Also left a couple of minor comments for discussion. 🙏
schema/schema.ts
Outdated
@@ -832,6 +832,13 @@ export interface Annotated { | |||
* @maximum 1 | |||
*/ | |||
priority?: number; | |||
|
|||
/** | |||
* The timestamp indicating when this annotation was created or last updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be about the annotation (only) or about the thing that the annotation is attached to? I lean toward the latter—maybe we could update the phrasing to reflect this?
schema/schema.ts
Outdated
* | ||
* Should be an ISO 8601 formatted string (e.g., "2025-01-12T15:00:58Z"). | ||
*/ | ||
timestamp?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to have other timestamps in future, so perhaps a more specific name like:
timestamp?: string; | |
lastModified?: string; |
* upstream/main: docs: update README links chore: add markdown format check workflow and format all docs chore: add prettier and format command for markdown files Move schema files to versioned directory Fix broken links. 2024-11-05 schema.ts/schema.json points to main/schema/2024-11-05/schema.ts in anticipation of PR modelcontextprotocol#145 being merged. update generate json after merge update protocol revision to draft on sampling page updated schema protocol version to reflect draft. change to relative link in spec point documentation changes to "draft" Revert schema/schema.ts to upstream/main, add drafts folder. Update validate and build scripts to build drafts too. fix: update draft page reference and add alias for better navigation fix: incorrect link for "Specification (Draft)" on nav bar Update schema/schema.ts remove spurious semicolon validation, simplify wording. Add optional "size" attribute to Resource, include in Documentation Update docs/specification/client/sampling.md Updated Specification and Docs to support Audio Modality.
@jspahrsummers sorry for the delay on this one. I updated the name and added some examples to the docstring. Generally, the idea here is that since resources are application controlled, it's useful to pass in the lastModified timestamp of the attached data especially for truncation when we exceed context limits (eg. evict files that have no recent activity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me, thanks for adding this
schema/draft/schema.ts
Outdated
|
||
/** | ||
* The timestamp when the resource was last modified. | ||
* Examples: last activity timestamp in an open file, timestamp when resource is attached, etc. | ||
* | ||
* Should be an ISO 8601 formatted string (e.g., "2025-01-12T15:00:58Z"). | ||
*/ | ||
lastModified?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we annotate this with a date? https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-validation-00#rfc.section.7.3.1
82ab529
Co-authored-by: David Soria Parra <[email protected]>
* | ||
* Should be an ISO 8601 formatted string (e.g., "2025-01-12T15:00:58Z"). | ||
*/ | ||
lastModified?: Date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea. While this is usually the most intuitive way to express a point in time. However, I assume a client application will want to use this for comparison of the resource has changed. Notable ISO 8601 are not guarantee to be monotonically increasing. The following issues can appear:
- Leap Seconds
- Clock Adjustment
- Daylight Savings
- Clock Drift or Time Sync issues between client and server (particularly in a remote scenario)
There are a few ways to go about this:
- We can include more metadata (e,g, rsync uses size + mtime, git uses mtime + hashes) and use unix timestamps, to make educated guesses.
- We can enforce a monotonically increasing time, e.g. the watchman uses a random "point in time" stamp, that will always increase. It doesn't have to represent time, but for example things like "since the system started'.
- We can define lastModified to be purely informational and must not be used for comparision
Open to any suggestions. THrowing it back to you. If it's intended to be purely informational, I am happy to accept as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, unix timestamps + some additional information might be proibably the easiest.
Adds an optional timestamp field to Annotated interface.
Motivation and Context
It's useful to know when a Resource is created or updated for context truncation.
How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist
Additional context