Skip to content

fix(events): refuse an empty session id - #801

Draft
justintime4tea wants to merge 1 commit into
justingross/GH-778-run-events-carry-lifecycle-and-activityfrom
justingross/GH-800-session-id-refuses-empty
Draft

justintime4tea wants to merge 1 commit into
justingross/GH-778-run-events-carry-lifecycle-and-activityfrom
justingross/GH-800-session-id-refuses-empty

Conversation

@justintime4tea

@justintime4tea justintime4tea commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

SessionId accepted the empty string, and every store keyed by session inherited it (#800): clients sending a blank chat session id shared one skill log, and a session journal (#779) keyed by a blank would be shared by every run whose id resolved to one. This moves SessionId onto nonempty_string_newtype! so a blank cannot be held at all, and has each boundary that built one from a string treat a blank as "no session".

"Stacked" on #794 — draft until it lands

This builds on nonempty_string_newtype! from #794, so its base is #794's branch. The typed resolver builds on #795 (treat a blank chat session id as missing), which has merged into nightly; the branch is rebased onto #794 at the current nightly and no longer carries its own copy.

When #794 merges: retarget to nightly, mark ready.

Changes

  • aura-events: SessionId is generated by nonempty_string_newtype! — new returns Result<Self, EmptyId>, TryFrom replaces From, "" fails to deserialize. The macro gains non_empty(value) -> Option<Self> for boundaries where blank and absent mean the same.
  • chat handler: chat_session_id() returns a SessionId (blank sources still fall through, a fresh id is minted if none), and prepare_request takes &SessionId, so the guarantee travels with the value.
  • HITL scopes built by the builder and the orchestrator, and ScopeRecord → AgentScope on restoring a stored approval: a blank reads as None rather than a session or a decode failure.
  • standalone CLI (direct.rs): a blank session id is an error at the boundary; the Backend trait keeps its &str.
  • tests: refusal on construction and serde, non_empty, a blank in a stored scope restoring as None, and the existing resolver and skill-log tests under the typed id.

@greptile-apps

greptile-apps Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

SessionId now rejects empty strings during construction and deserialization.

  • Chat requests carry the typed identifier into prepare_request.
  • Empty optional session strings become None in approval scopes.
  • Standalone CLI requests reject an empty identifier.
  • Tests cover construction, deserialization, and stored scope restoration.

No new actionable issues were found. The earlier header-fixture finding was withdrawn, and the earlier comment changes are addressed.

Reviews (4) · Last reviewed commit: "fix(events): refuse an empty session id" · Reviewed by Greptile

Comment thread crates/aura-web-server/src/handlers.rs
Comment thread crates/aura/src/session_store/record.rs Outdated
@justintime4tea
justintime4tea force-pushed the justingross/GH-800-session-id-refuses-empty branch from 412cd98 to 7022782 Compare October 9, 2026 13:10
@justintime4tea
justintime4tea force-pushed the justingross/GH-778-run-events-carry-lifecycle-and-activity branch from cba59a3 to 245e703 Compare October 9, 2026 14:25
@justintime4tea
justintime4tea force-pushed the justingross/GH-800-session-id-refuses-empty branch from 7022782 to d23fd02 Compare October 9, 2026 14:26
`SessionId` was generated by `string_newtype!`, so `""` built one and
deserialized into one, while `ObserverId`, `CheckpointRef` and `RunId`
refuse their empty and nil values. Every store keyed by session
inherited the hole: clients sending a blank chat session id shared one
skill log, and a session journal keyed by it would be shared by every
run whose id resolved to a blank.

`SessionId` now comes from `nonempty_string_newtype!`: `new` returns a
`Result`, `TryFrom` replaces `From`, and `""` is refused on every path.
The macro gains `non_empty`, for a boundary where a blank and an absent
value mean the same, and every site that built a session from a string
decides what a blank means there, which is the same thing everywhere:
no session. The chat handler's resolver hands out a `SessionId`, so the
guarantee travels with the value; the builder's and the orchestrator's
HITL scopes, and a stored approval's scope on restore, read a blank as
`None`; the standalone CLI reports one as an error at its boundary.

Fixes: GH-800
@justintime4tea
justintime4tea force-pushed the justingross/GH-778-run-events-carry-lifecycle-and-activity branch from 245e703 to 5db77be Compare October 9, 2026 20:52
@justintime4tea
justintime4tea force-pushed the justingross/GH-800-session-id-refuses-empty branch from d23fd02 to 17ddf89 Compare October 9, 2026 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant