Fix audit findings (non-docs): wire format, spec-conformance, bugs, security#120
Merged
Conversation
Add custom JSON converters for JobStatus, LogLevel, ChunkEncoding, LeaseGrant (bare namespace map), CredentialConstraints (dotted keys), AgentInventory (plain array) and JobEventPayload (flat kind-specific body). Reorder Json.fs after Messages.fs so converters can reference the payload types. Add golden wire-format tests pinning the spec JSON shapes (§6.2, §7.3, §8.2, §8.4). Co-authored-by: Cursor <cursoragent@cursor.com>
…ncy, leases, credentials - session.close/session.closed wire types replace session.bye (#73) - idempotency replay compares param fingerprint, emits DUPLICATE_KEY on conflict (#76) - cancellation acks job.cancelled then job.error(CANCELLED) (#77, #100); job.cancel returns JOB_NOT_FOUND/PERMISSION_DENIED (#78) - EmitDelegateAsync validates lease subset, raises LEASE_SUBSET_VIOLATION (#81) - credential_rotated value redacted from subscribers (#82) - streamed job.result carries result_id; mixing inline+chunks errors (#83) - anonymous sessions never receive credentials; startup guard (#88) - EventLog.EvictExpired honors lastAckedSeq; periodic pruning timer + PruneEmpty (#75, #105) - terminal job records dispose CTS/watchdog, clear credentials, evicted after retention (#113) Co-authored-by: Cursor <cursoragent@cursor.com>
Add session.resume message type, move resume out of session.hello, retain disconnected sessions as resumable within the window, replay buffered events on reattach, rotate resume token, and return RESUME_WINDOW_EXPIRED for unknown/expired sessions. Add integration tests. Co-authored-by: Cursor <cursoragent@cursor.com>
…try, anonymous isolation - max_runtime_sec watchdog emits job.error(TIMEOUT)/timed_out (#101) - idempotent replay returns the original job.accepted verbatim (#102) - single terminal message per job via terminal gate (#103) - cost.budget.* telemetry no longer decrements budget (#104) - unique per-session anonymous principal id (#110) - malformed/unknown inbound -> session.error INVALID_REQUEST, session survives (#99) Co-authored-by: Cursor <cursoragent@cursor.com>
…n, client races - list_jobs stable ordering + cursor pagination + next_cursor; bare-name agent filter; limit applied via take+1 (#109, #91, #108) - job.subscribe history:true replays buffered events; replayed reflects reality (#115) - per-session gate serializes event_seq assignment with send (#112) - credential rotation keeps id outstanding for terminal revocation (#107) - client buffers job-addressed envelopes until handle registered (#95) - SubscribeAsync surfaces denials instead of a dead handle (#96) - receive-loop exit faults pending waiters and completes handles (#97) Co-authored-by: Cursor <cursoragent@cursor.com>
…, revocation surfacing - reject name@version without agent_versions; strip suffix from non-negotiated sessions (#79) - enforce chunk_seq monotonicity and single-stream completion (#84) - progress current>=0 (reject) and current<=total (clamp) (#85) - negative cost metrics rejected; negative non-cost metrics still emit (#86) - permanent credential revocation failures logged + exposed via RevocationFailures (#87) Co-authored-by: Cursor <cursoragent@cursor.com>
- crypto RNG for trace/span ids (#41) - handler resolved before job.accepted; shared unwind (#47) - lease/runtime watchdog callbacks observe exceptions (#48) - 3-state RevocationOutcome; permanent failures keep credential outstanding (#49) - auto-ack/receive-loop/dispose tasks observed; Completion exposed (#60,#61,#62) - JobErrorMapper parses details fields (#63) - AutoAck docs corrected to event-driven (#64) - ChunkAssembler + WebSocket message size/chunk caps (#65,#66) - Giraffe short-circuits ws/400 branches (#40) - CLI awaits enumerator dispose and honors --stdio (#38,#39) Co-authored-by: Cursor <cursoragent@cursor.com>
- watchdog skips terminal jobs (#89); wire retryable + Unknown arm (#90) - client observes cancellation tokens (#98); reject re-hello (#106) - session-scoped LastEventSeq via Interlocked; subscribed_from in subscriber space (#111) - enforce negotiated features on submit (#114); duplicate-currency budget subset sum (#116) - readEnvelope rejects null/empty envelopes (#117); ArcpResult avoids shadowing FSharp.Core Result (#118) - StaticBearerVerifier constant-time compare (#119); DevMode rejects whitespace (#54) - glob regex cache (#44); literalPrefix/isSubset cleanup (#43,#45); array retryDelays (#55) - single idempotency lookup (#52); credential revoke failure observable (#53) - dead-code removal (#51,#67); volatile stdio closed (#68); typed pending error (#69) - Connected member (#70); optional jobId mapping (#71); lazy Otel source from Version.Sdk (#42) Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 21 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (57)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
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
1 participant
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.
Summary
Addresses the full set of non-docs audit findings across
Arcp.Core,Arcp.Runtime,Arcp.Client,Arcp.Otel,Arcp.Giraffe, andArcp.Cli. Work is grouped by theme below. Build is clean (0 warnings/0 errors); 268 unit + 28 integration tests pass.Wire format (foundational)
job.eventbodies, lowercasefinal_status/level/encoding, bare-map leases, dotted credential-constraint keys, plaincapabilities.agentsarrays. Added golden wire-format tests. (Union payloads serialize with F# case-name wrappers and PascalCase tags, breaking the spec wire format (§6.2, §7.3, §8.2, §8.4) #94)Session lifecycle & resume
session.close/session.closedreplacesession.bye; runtime acks close. (Close uses 'session.bye' wire type; no 'session.closed' ack (§6.7) #73)session.resumemessage + buffered-event replay +RESUME_WINDOW_EXPIRED; resume modeled as its own message. (session.resume flow not implemented; replay never invoked (§6.3) #74, Resume modeled inside session.hello rather than session.resume (§6.3) #80)EvictExpiredandDrophave no callers, so per-session buffers leak forever #105)session.helloon an established session. (Repeatedsession.helloon one connection mints a new session and leaks the old ServerSessionContext #106)Jobs & idempotency
DUPLICATE_KEYon conflict; replays the originaljob.acceptedverbatim. (Idempotency replay does not compare params; never emits DUPLICATE_KEY (§7.2) #76, Idempotent replay returns a differentjob.acceptedpayload: credentials omitted, budget reflects current spend (§7.2) #102)job.cancelledthenjob.error(CANCELLED);job.cancelauthorizes/validates. (Cancel terminates with job.result(cancelled), not job.error(CANCELLED) (§7.4) #77, job.cancel silently no-ops on unauthorized/unknown id (§7.4) #78,job.cancelledacknowledgement does not exist in the message vocabulary; cancel is never acked (§7.4) #100)max_runtime_secenforced →TIMEOUT/timed_out. (max_runtime_secis accepted but never enforced; TIMEOUT is never produced (§7.1, §12) #101)job.result(cancelled)afterjob.error(LEASE_EXPIRED)(§7.3, §9.5) #103, Expiry watchdog re-emits LEASE_EXPIRED on already-terminated jobs (§9.5) #89)Leases, budget, credentials, events
EmitDelegateAsyncvalidates lease subset →LEASE_SUBSET_VIOLATION. (Runtime never validates delegated lease subset (§9.4) #81)credential_rotatedvalue redacted from subscribers; rotation keeps the id outstanding for terminal revocation. (credential_rotated status leaks new credential value to subscribers (§14) #82, Credential rotation erases the registry/store entry, so the rotated (new) value is never revoked at job termination (§9.8.2) #107)job.resultcarriesresult_id; inline+chunk mixing errors; chunk_seq monotonicity enforced. (job.result after streamed chunks omits required result_id (§8.4) #83, Runtime never enforces chunk_seq monotonicity or single result_id per stream (§8.4) #84)cost.budget.*telemetry no longer decrements; negative cost rejected. (Agent-reportedcost.budget.remainingtelemetry decrements the budget counter (§9.6) #104, Negative cost metric silently dropped instead of rejected (§9.6) #86)Spec conformance & security
session.error(INVALID_REQUEST), session survives. (Runtime silently drops malformed and unknown inbound messages instead of returning INVALID_REQUEST (§12) #99)agent_versionsgating forname@version. (Agent version pinning works without negotiating agent_versions (§6.2, §7.5) #79)list_jobsstable ordering + cursor pagination; bare-name agent filter; limit applied lazily. (session.list_jobspagination is not implemented: cursor ignored,next_cursoralways null, truncated jobs unreachable (§6.6) #109,session.list_jobsagent filter requires exactname@version; the spec's bare-name filter matches nothing (§6.6) #108, session.list_jobs materializes every visible job before applying the limit #91)job.subscribehistory:truereplays buffered events;replayedreflects reality. (job.subscribewithhistory: truenever replays buffered events but answersreplayed: true(§7.6) #115)event_seqassignment + send. (Event seq assignment and transport send are not atomic per session — events can hit the wire out of order (§8.3) #112)"anonymous"— cross-client job visibility and subscription (§6.6, §14) #110)Client
retryableviaUnknownarm; parse errordetails; optional jobId mapping. (Wireretryableignored when reconstructing client-side ARCPError (§12) #90, JobErrorMapper silently drops timeout/lease/resume fields #63, JobErrorMapper passes empty jobId from non-job context #71)Completion/Connected. (sendMessage Task swallowed via ignore; send errors lost #60, runReceiveLoop started with ignore; loop exceptions never observed #61, DisposeAsync fire-and-forget in finally swallows enumerator errors #62, connectedTcs is never observed externally #70)Cleanups & perf
JobErrorMapper,AutoAckdocs, glob regex cache,literalPrefix/isSubset/idempotency-lookup cleanups, array retry delays, dead-code removal, volatile stdio flag, typed pending error, lazy Otel source, Giraffe short-circuit, CLI--stdio+ awaited dispose. (AutoAckScheduler interval flush only fires on event arrival #64, ArcpActivitySource creates a static ActivitySource at module load #42, Lease.literalPrefix uses mutable scan instead of pattern matching #43, Lease.Glob.compile rebuilds regex on every match #44, Lease.isSubset uses nested match instead of Result.bind chain #45, JobManager constructor takes unused timeProvider parameter #50, SubscriptionFanout.byPrincipal field is never read or written #51, Repeated dictionary lookup for idempotency replay #52, Credential revocation failure in JobLauncher silently swallowed #53, retryDelays as F# list incurs O(n) indexed access #55, Memory transport binds unused infinite seq #67, Stdioclosedfield read without volatile/lock from receive task #68, PendingRegistry.Register throws failwithf from library code #69, Cli streamEventsAsync fires-and-forgets enumerator DisposeAsync #38, Cli serve command ignores the --stdio flag #39, Giraffe endpoint calls next after WebSocket session ends #40)ARCP.Core.Resultrenamed toArcpResultto stop shadowing FSharp.Core; null-envelope rejection; duplicate-currency budget subset sum. (ARCP.Core.Resultmodule shadows FSharp.Core'sResultfor every consumer that opens the namespace #118,Codec.readEnvelopeaccepts the JSON literalnull, yielding a null Envelope that crashes the session loop #117, Duplicate currencies in a childcost.budgetbypass the subset check while summing at acceptance (§9.4) #116)Skipped: #35 (labeled
audit/docs).Fixed issues
Fixes #38 Fixes #39 Fixes #40 Fixes #41 Fixes #42 Fixes #43 Fixes #44 Fixes #45 Fixes #47 Fixes #48 Fixes #49 Fixes #50 Fixes #51 Fixes #52 Fixes #53 Fixes #54 Fixes #55 Fixes #60 Fixes #61 Fixes #62 Fixes #63 Fixes #64 Fixes #65 Fixes #66 Fixes #67 Fixes #68 Fixes #69 Fixes #70 Fixes #71 Fixes #73 Fixes #74 Fixes #75 Fixes #76 Fixes #77 Fixes #78 Fixes #79 Fixes #80 Fixes #81 Fixes #82 Fixes #83 Fixes #84 Fixes #85 Fixes #86 Fixes #87 Fixes #88 Fixes #89 Fixes #90 Fixes #91 Fixes #94 Fixes #95 Fixes #96 Fixes #97 Fixes #98 Fixes #99 Fixes #100 Fixes #101 Fixes #102 Fixes #103 Fixes #104 Fixes #105 Fixes #106 Fixes #107 Fixes #108 Fixes #109 Fixes #110 Fixes #111 Fixes #112 Fixes #113 Fixes #114 Fixes #115 Fixes #116 Fixes #117 Fixes #118 Fixes #119
Test plan
dotnet buildclean (0 warnings/errors)dotnet testunit (268 passing) and integration (28 passing)