You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add imcp2-session-api crate: the session canister's Candid API - #235
A types-only crate for the canister that imcp2 backends will share
sessions through: every type of imcp2_session.did (session records,
tombstones, the change feed, key checks, configuration, status and the
error variants), the method names, the timing constants both sides
must agree on (code TTL, clock skew, consume window, abandonment, grant
caps, idle timeout bounds), and the shared rules (touch granularity,
idle deadline, when idle purge and touches take effect).
The hand-written .did is the contract: tests pin golden wire bytes for
every request and response, check that payloads without later `opt`
fields still decode and that unknown variants are refused, type the
.did against the Rust types, and check the invariants between the
constants.
It joins the workspace at the current version with `publish = false`:
nothing published depends on it yet, and its first release needs a
placeholder on crates.io before the lockstep publish can upload it.
blind-oracle
changed the title
Add imcp2-session-api: the session canister's Candid contract
Add imcp2-session-api crate: the session canister's Candid API
Nov 10, 2026
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Idle-timeout and reporting rules can prematurely purge active sessions, while zero-limit pagination, ambiguous hash encodings, and incomplete sealed-value headers create additional correctness risks.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The eviction logic can remove a just-resumed session while its latest activity is still buffered, leaving active sessions vulnerable to premature eviction.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Prevent eviction before a resumed session's touch is acknowledged
crates/imcp2-session-api/src/rules.rs:184
This can evict a just-resumed session while its new touch is still buffered: under a seven-day timeout, a stored activity report 16,320 seconds old already qualifies. Adding flush intervals to the old report's age cannot account for activity that resumed just before this check. Require an acknowledged touch before serving requests for an eviction-eligible session, or define a coordinated eviction protocol.
Document that snapshot restores can rewind Seq positions
crates/imcp2-session-api/src/types.rs:239
The Seq docs say snapshot loads rewind head, so this unconditional monotonicity guarantee can mislead backends that compare feed positions across a restore. State the restore exception here and in imcp2_session.did:105, and direct readers to check epoch before reusing their position.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
A purge request containing only a misspelled targeting field can decode to an empty filter and unintentionally purge every namespace.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Reject empty purge filters to prevent accidental global purges
crates/imcp2-session-api/src/types.rs:283
With every field optional, a targeted Candid purge containing only an unrecognized field such as namspace decodes to an empty filter, which this contract defines as purging every namespace. Reject empty filters and require an explicit full-purge choice so a targeting typo cannot become a global purge. The same filter is declared in imcp2_session.did:125–131.
Candid drops record fields a type does not declare, and dfx annotates a
textual argument with the canister's published interface before sending
it. With every PurgeFilter field optional, a misspelled one
(`namspace = ...`) silently became "no constraint": alone it purged
every session in every namespace, and next to correct fields it widened
the purge.
Every field is now required, as `variant { Any; Is : T }` (the new
`Match<T>`). A misspelled or missing field, a misspelled tag, or the old
`opt` form is refused before the call is made and when it is decoded;
purging everything has to be written out as every field `Any`.
This changes the wire format of admin_purge_sessions and PurgeJob
(nothing is released yet); the golden vectors are re-pinned. A new test
runs textual arguments through the same annotation dfx applies and
checks that every typo is refused.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Critical concerns omitted from the published review require human assessment of purge safety, revocation preservation after snapshot restore, and tombstone growth beyond live-session caps.
- Every method takes one record, so arguments can grow: touch_sessions
(TouchSessionsArg), get_sessions (GetSessionsArg), admin_set_key_check
(SetKeyCheckArg, with `replace : opt bool`), admin_retire_kid
(RetireKidArg); get_key_checks and get_status take `record {}`.
- Errors are records, `{ err_code; err_case : opt <Case>; err_str }`,
instead of variants: an older reader decodes a case it does not know
as `err_case = None` (Candid's opt rule), so cases can be added. One
shared code space (error_code); `Other(ErrorInfo)` is gone, and
`purge_in_progress` is the PurgeInProgress case.
- Methods a backend acts on are updates. get_sessions and list_changes
get explicit `*_non_replicated` query twins for the hint path;
get_key_checks and get_status are update-only.
- The method-name constants (`method`) are removed. The canister's
export_service test is the interface check; this crate checks its
types against the .did.
- rules.rs defines its terms (activity reports, idle purge, eviction)
and says what each rule returns, in which units;
reporting_matters is now should_report_session_activity, and
report_promptly should_report_session_activity_promptly.
- Each method's purpose is documented in the .did (consume_code
included). The crate docs no longer refer to another repository.
The wire format changes (nothing is released yet); the golden vectors
are re-pinned, and a new test pins that an unknown error case decodes
as None.
The design record and the runbook now describe:
- the sealed envelope's minimum length;
- the corrected touches_effective boundary;
- the shared reporting and prompt-report rules;
- the canister recording the exchange as the session's first activity;
- that a snapshot load can rewind the feed's head;
- the residual eviction race at a capacity cap, with its trade-off and
a new open item.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Critical authorization gaps can revive purged sessions through stale replicas or snapshot restores and allow authorization-code reuse through incorrectly scoped retry IDs.
- should_report_session_activity_promptly allows two clock skews, not one.
The canister caps a report at its own time, so it may hold an activity up
to CLOCK_SKEW earlier than the one a backend sent; with the backend's
clock then CLOCK_SKEW behind, one skew left the session evictable up to a
minute before the periodic flush landed. The race test now models that
worst case (and fails under the old threshold).
- Purge filters: created_from and created_to are inclusive bounds on
created_at, as the canister already matches them; Match says so.
- The contract states what was already true:
- a backend never uses a hint for a key whose deletion it has read from
an update, so a stale replica can delay a deletion, never undo it;
- exchange_id's retry boundary: only the verifier's holder can retry,
only while the code is accepted, and a retry gets the identical token;
- a snapshot load restores deleted sessions until operators replay the
revocations from their admin journal.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human judgment is needed to determine whether the maximum timer interval’s potential hour-long cleanup delay is acceptable relative to the much shorter abandonment and idle thresholds.
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
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.
A types-only crate for the canister that
imcp2backends will share sessions through. Plus tests.