feat(agent): add bounded snapshot subscriptions - #97
ben-ranford wants to merge 21 commits into
Conversation
ben-ranford
added
area:agent
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c76bf29516
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Compatibility and subscription cleanup issues, plus the required changelog update, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an opt-in, negotiated full-snapshot subscription extension with bounded delivery, session integration, schemas, documentation, and lifecycle tests.
Changes:
- Adds subscription protocol types, negotiation, and capability support.
- Implements bounded coalescing delivery and cleanup.
- Adds session bridging, validation, documentation, and tests.
File summaries
| File | Reviewed change |
|---|---|
scripts/rigor/generated/public-api.txt |
Updates generated public API inventory. |
schema/protocol/subscription_schema_test.go |
Validates schema separation and references. |
schema/protocol/snapshot-subscriptions.json |
Defines the subscription wire schema. |
runtime/agent/subscription.go |
Implements bounded subscription state and production. |
runtime/agent/subscription_test.go |
Tests negotiation and baseline behavior. |
runtime/agent/subscription_lifecycle_test.go |
Tests lifecycle and failure cleanup. |
runtime/agent/subscription_delivery_test.go |
Tests coalescing and client isolation. |
runtime/agent/session_bridge.go |
Bridges sessions to snapshots and publications. |
runtime/agent/server.go |
Handles negotiation, delivery, and lifecycle cleanup. |
protocol/protocol.go |
Adds extension constants and wire types. |
docs/snapshot-subscriptions.md |
Documents subscription usage and responsibilities. |
docs/README.md |
Links the subscription guide. |
capability/capability.go |
Adds subscription capability negotiation data. |
Review details
Suppressed comments (4)
docs/snapshot-subscriptions.md:6
- This adds user-visible protocol behavior, but
CHANGELOG.md'sUnreleasedsection remains empty. The repository contribution rules require updating the changelog for user-visible behavior changes (CONTRIBUTING.md:38); please add the advertised subscription entry.
Snapshot subscriptions are an opt-in extension for automation clients that need
full semantic snapshots after state changes. They do not change Stave protocol
version `1.0`, the existing `stave.snapshot` polling request, or any existing
v1 payload.
runtime/agent/server.go:432
- This subscription-specific error path also discards a writer failure. With an active subscription, a failed response write here leaves
Servereading and the producer waiting instead of joining it; propagate the write error and enter the normal cleanup path.
_ = write(protocol.Response{JSONRPC: protocol.JSONRPC, ID: r.ID, Error: protocol.Errorf(protocol.InvalidRequest, "invalid snapshot unsubscribe request")})
runtime/agent/server.go:285
- When a regular
s.Notifywrite fails, the preceding error branch only records the failure and falls through; this cleanup runs only when the notifications channel closes. If the input remains open, an active subscription producer can keep waiting and snapshotting after the output failure, so the connection does not satisfy the required stop-and-join behavior. Treat the writer error as connection termination and close/join the current subscription before returning.
} else {
subscriptionMu.Lock()
if subscription != nil {
subscription.close()
subscription = nil
runtime/agent/server.go:421
Server.Closecan closes.notifywhile the baseline is being written. The writer's closed-channel branch may run before this assignment and observe no subscription; after that, this code installs the producer, but the localnotificationschannel is already nil and no later close signal will reach it. The producer can then remain alive until input EOF despite the server being closed. Coordinate installation with the close lifecycle or recheck the closed state after installing and immediately close/join the producer.
if response.Error == nil && baseline != nil {
subscriptionMu.Lock()
subscription = newSnapshotSubscription(ctx, *baseline, subscriptionWake, s.opt.SnapshotPublicationWaiter, func(waitCtx context.Context) (protocol.SnapshotResult, bool) {
result, err := s.subscriptionSnapshot(waitCtx)
return result, err == nil
})
subscriptionMu.Unlock()
- Files reviewed: 12/13 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f735dce3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e033631e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4f04a1f2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0387bb37b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a67025f1dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Sonar API auditReviewed commit: Live public SonarCloud API queries for PR #97 returned:
Audited via CLI at 2026-09-14T13:18:17.598128+00:00. Project visibility was verified as public. No findings were changed or suppressed. |
Summary
stave.snapshot.subscribe/v1extension. Each physical JSONL connection owns one subscription, writes its full baseline first, and coalesces later updates into one pending full snapshot. Unsubscribe, close, cancellation and output failure stop and join the producer.Closes #65
This PR targets
feat/65-subscription-dependencies, containing #78, #94 and #79, so its diff contains only #65. After those prerequisites land, rebase the #65 commits onto current main and retarget before the merge gate.Validation
make fastmake verifymake ci— final exit 0;/tmp/stave-pr97-handler-ci.logafter the handler cleanup.-race.Release Notes
Scope firewall: A review item belongs here only if it directly prevents #65 acceptance and is an incremental correction in the initially touched subscription, bridge, negotiation or schema surface. Shared files or wording do not justify adjacent work.