fix: address 17 open issues across auth, isolation, idempotency, and docs#52
Merged
Conversation
…docs Security & isolation: - BearerAuth ignores client-supplied principal; only the server-side token mapping is authoritative (#34). - Resume replay is scoped to the calling session via EventLog::replayAfterForSession (#44). - Subscriptions with no session_id filter default to the calling session instead of fanning out across sessions (#45). - Artifact fetch/release reject cross-session artifact ids with PERMISSION_DENIED (#46). - LeaseManager records the granting session; tool.invoke rejects leases granted to another session (#47). Correctness: - Idempotent retries now replay the original tool.result/tool.error instead of returning a bare Ack (#35). - ResultChunkAssembler enforces sequence contiguity, terminal-chunk delivery, and duplicate consistency (#36). - CostBudget tolerates small float costs (scientific notation), rejects NaN/Inf, and rejects over-precise budget patterns (#37). - ArtifactPut validates the supplied sha256 digest against payload bytes; client putArtifact() accepts and surfaces digest errors (#38). - ARCPClient ping/putArtifact map Nack responses through ErrorMapper consistently with the other helpers (#39). - StdioTransport drains and decodes a final unterminated frame on EOF instead of dropping it (#48). - InMemoryCredentialStore now reports supportsDurableRevocation() as false; runtime correctly refuses to pair it with a CredentialProvisioner (#49). - JobManager retains terminal jobs for a bounded retention window so session.list_jobs can surface recent history (#50). Documentation: - README event type names match the actual catalog (#40). - Auth guide notes server-authoritative bearer principal (#41). - @throws docs added to ARCPClient, JobContext, ARCPRuntime, and Transport public APIs (#42). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThis PR enforces session isolation across artifact, lease, and event replay operations; upgrades bearer authentication to ignore client-supplied principals; validates artifact SHA-256 digests and result chunk streams; extends job retention; and improves cost budget float handling. ChangesSession isolation, authentication security, and artifact management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.54)PHPStan was skipped because the config uses disallowed Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Internal/Runtime/ArtifactDispatcher.php`:
- Around line 45-51: The error message in the SHA-256 validation is misleading
because $normalized is derived with strtolower(), so uppercase hex is accepted;
update the lifecycle->nack call (the error string passed in the artifact.put
SHA-256 validation block using preg_match('/^[0-9a-f]{64}$/', $normalized)) to a
neutral message such as "artifact.put sha256 must be 64 hexadecimal characters"
(or similar) that does not claim lowercase-only hex.
In `@src/Internal/Runtime/CredentialLifecycle.php`:
- Around line 33-37: The code currently allows a null Session to fall back to
leases->get(...) which bypasses session ownership checks; update
leaseFromArguments (and the other method(s) that call leases->get around the
102–113 area) to require a non-null Session: if $session is null throw an
InvalidArgumentException (or specific domain exception) instead of calling
leases->get; and when retrieving a lease use a session-aware lookup or enforce
ownership by verifying the returned LeaseGranted's session id matches
$session->id (e.g. replace leases->get(...) with a session-aware call or add a
post-check that LeaseGranted->getSessionId() === $session->id and return
null/throw if it does not). Ensure you reference leaseFromArguments and the code
paths that currently call leases->get to locate and harden all fallbacks.
In `@src/Runtime/JobContext.php`:
- Line 326: Replace the nullable session reference with the authoritative
JobContext session id when registering leases: in JobContext (where the call to
$this->runtime->leases->register(...) occurs), use $this->sessionId instead of
$this->session->sessionId so the register call uses the non-null JobContext
session identifier; update the register invocation in that method (and any
nearby calls in the same class) to pass $this->sessionId.
In `@src/Transport/Transport.php`:
- Around line 33-35: Update the Transport class/interface header to match the
receive() method contract: change the top-level wording that currently says
connection drops are returned as null to state that clean closes return null
while unexpected transport failures throw \Arcp\Errors\TransportClosedException;
ensure this aligns with the receive() docblock which already documents returning
null on clean close and throwing TransportClosedException on unexpected failure,
and keep the \Amp\CancelledException note for `$cancellation` consistent.
In `@tests/Integration/ResumeTest.php`:
- Around line 137-166: The test currently has two consumers reading from the
same transport (ARCPClient's internal read-loop started by ARCPClient::open and
the direct $clientTA->receive() calls), which causes nondeterministic races; fix
by choosing one consumer path and removing the other: either stop using
ARCPClient::open and drive the test via the raw transport client ($clientTA)
only (send the Resume envelope with $clientTA->send and assert/read all replies
with $clientTA->receive), or stop using $clientTA->receive() and assert
replay/ack via the ARCPClient instance (use ARCPClient methods/events or its
receive/queueing API) so only ARCPClient reads the transport; adjust the code
around Envelope/Resume handling and checks that reference
$clientA->session->sessionId, $clientTA->send, and the receive loop to use the
single chosen consumer.
In `@tests/Integration/SubscriptionTest.php`:
- Around line 149-154: The test only performs negative checks and can pass when
$aObservedSessions is empty; add a positive assertion that A actually observed
its own session by asserting $aSessionId is present in $aObservedSessions (e.g.
call self::assertArrayHasKey($aSessionId, $aObservedSessions) or
self::assertContains($aSessionId, array_keys($aObservedSessions))) before
computing $foreign so the test verifies the subscription delivered at least one
envelope for A.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dffff26a-ed36-493a-8e2a-53a9fb08ab7c
📒 Files selected for processing (35)
README.mddocs/guides/auth.mdsrc/Auth/BearerAuth.phpsrc/Client/ARCPClient.phpsrc/Client/ResultChunkAssembler.phpsrc/Internal/Runtime/ArtifactDispatcher.phpsrc/Internal/Runtime/CredentialLifecycle.phpsrc/Internal/Runtime/LifecycleHandler.phpsrc/Internal/Runtime/SubscriptionRouter.phpsrc/Internal/Runtime/ToolInvocationHandler.phpsrc/Runtime/ARCPRuntime.phpsrc/Runtime/ArtifactStore.phpsrc/Runtime/CostBudget.phpsrc/Runtime/Credentials/InMemoryCredentialStore.phpsrc/Runtime/Job.phpsrc/Runtime/JobContext.phpsrc/Runtime/JobManager.phpsrc/Runtime/LeaseManager.phpsrc/Runtime/SubscriptionManager.phpsrc/Store/EventLog.phpsrc/Transport/StdioTransport.phpsrc/Transport/Transport.phptests/Integration/ArtifactTest.phptests/Integration/CredentialLifecycleTest.phptests/Integration/HandshakeTest.phptests/Integration/JobLifecycleTest.phptests/Integration/ResumeTest.phptests/Integration/StdioTransportTest.phptests/Integration/SubscriptionTest.phptests/Support/FakeDurableCredentialStore.phptests/Unit/AuthTest.phptests/Unit/Client/ResultChunkAssemblerTest.phptests/Unit/LeaseManagerTest.phptests/Unit/Runtime/CostBudgetTest.phptests/Unit/Runtime/Credentials/InMemoryCredentialStoreTest.php
Comment on lines
+45
to
+51
| if (preg_match('/^[0-9a-f]{64}$/', $normalized) !== 1) { | ||
| $this->lifecycle->nack( | ||
| $session, | ||
| $env, | ||
| 'INVALID_ARGUMENT', | ||
| 'artifact.put sha256 must be 64 lowercase hex chars', | ||
| ); |
There was a problem hiding this comment.
Adjust SHA-256 validation error text to match actual behavior.
Line 45 normalizes with strtolower(...), so uppercase hex is accepted. The current message says “must be 64 lowercase hex chars,” which is misleading.
Suggested fix
- 'artifact.put sha256 must be 64 lowercase hex chars',
+ 'artifact.put sha256 must be 64 hex characters',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (preg_match('/^[0-9a-f]{64}$/', $normalized) !== 1) { | |
| $this->lifecycle->nack( | |
| $session, | |
| $env, | |
| 'INVALID_ARGUMENT', | |
| 'artifact.put sha256 must be 64 lowercase hex chars', | |
| ); | |
| if (preg_match('/^[0-9a-f]{64}$/', $normalized) !== 1) { | |
| $this->lifecycle->nack( | |
| $session, | |
| $env, | |
| 'INVALID_ARGUMENT', | |
| 'artifact.put sha256 must be 64 hex characters', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Internal/Runtime/ArtifactDispatcher.php` around lines 45 - 51, The error
message in the SHA-256 validation is misleading because $normalized is derived
with strtolower(), so uppercase hex is accepted; update the lifecycle->nack call
(the error string passed in the artifact.put SHA-256 validation block using
preg_match('/^[0-9a-f]{64}$/', $normalized)) to a neutral message such as
"artifact.put sha256 must be 64 hexadecimal characters" (or similar) that does
not claim lowercase-only hex.
Comment on lines
+33
to
+37
| public function leaseFromArguments( | ||
| array $arguments, | ||
| ResolvedTool $resolved, | ||
| ?Session $session = null, | ||
| ): ?LeaseGranted { |
There was a problem hiding this comment.
Null-session fallback weakens lease isolation guarantees.
At Line 36 and Line 109–Line 113, a missing session falls back to leases->get(...), which bypasses session ownership enforcement. This leaves a reachable unsafe path if a caller omits the session argument.
Suggested hardening
- public function leaseFromArguments(
+ public function leaseFromArguments(
array $arguments,
ResolvedTool $resolved,
- ?Session $session = null,
+ Session $session,
): ?LeaseGranted {
@@
- private function baseLease(mixed $leaseArg, ?Session $session): ?LeaseGranted
+ private function baseLease(mixed $leaseArg, Session $session): ?LeaseGranted
{
$leaseId = $this->extractLeaseId($leaseArg);
if ($leaseId === null) {
return null;
}
- $sessionId = $session?->sessionId;
- if ($sessionId !== null) {
- return $this->runtime->leases->getForSession($leaseId, $sessionId);
- }
- return $this->runtime->leases->get($leaseId);
+ $sessionId = $session->sessionId
+ ?? throw new InvalidArgumentException('lease lookup requires authenticated session');
+ return $this->runtime->leases->getForSession($leaseId, $sessionId);
}Also applies to: 102-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Internal/Runtime/CredentialLifecycle.php` around lines 33 - 37, The code
currently allows a null Session to fall back to leases->get(...) which bypasses
session ownership checks; update leaseFromArguments (and the other method(s)
that call leases->get around the 102–113 area) to require a non-null Session: if
$session is null throw an InvalidArgumentException (or specific domain
exception) instead of calling leases->get; and when retrieving a lease use a
session-aware lookup or enforce ownership by verifying the returned
LeaseGranted's session id matches $session->id (e.g. replace leases->get(...)
with a session-aware call or add a post-check that LeaseGranted->getSessionId()
=== $session->id and return null/throw if it does not). Ensure you reference
leaseFromArguments and the code paths that currently call leases->get to locate
and harden all fallbacks.
| expiresAt: $expiresAt, | ||
| ); | ||
| $this->runtime->leases->register($granted); | ||
| $this->runtime->leases->register($granted, $this->session->sessionId); |
There was a problem hiding this comment.
Use the authoritative JobContext session id when registering granted leases.
Line 326 currently pulls from $this->session->sessionId, which can be nullable. This weakens the new session-ownership guarantee if a nullable session object slips through. Use the non-null $this->sessionId already carried by JobContext.
Suggested fix
- $this->runtime->leases->register($granted, $this->session->sessionId);
+ $this->runtime->leases->register($granted, $this->sessionId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $this->runtime->leases->register($granted, $this->session->sessionId); | |
| $this->runtime->leases->register($granted, $this->sessionId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Runtime/JobContext.php` at line 326, Replace the nullable session
reference with the authoritative JobContext session id when registering leases:
in JobContext (where the call to $this->runtime->leases->register(...) occurs),
use $this->sessionId instead of $this->session->sessionId so the register call
uses the non-null JobContext session identifier; update the register invocation
in that method (and any nearby calls in the same class) to pass
$this->sessionId.
Comment on lines
+33
to
+35
| * @throws \Arcp\Errors\TransportClosedException on unexpected | ||
| * transport failure (distinct from a clean close, which returns null). | ||
| * @throws \Amp\CancelledException when `$cancellation` fires. |
There was a problem hiding this comment.
Align top-level transport close semantics with this method contract.
The receive() doc now correctly distinguishes clean close (null) from unexpected failure (TransportClosedException), but the interface header still says connection drops are seen as null. Please update that header text to avoid contradictory API expectations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Transport/Transport.php` around lines 33 - 35, Update the Transport
class/interface header to match the receive() method contract: change the
top-level wording that currently says connection drops are returned as null to
state that clean closes return null while unexpected transport failures throw
\Arcp\Errors\TransportClosedException; ensure this aligns with the receive()
docblock which already documents returning null on clean close and throwing
TransportClosedException on unexpected failure, and keep the
\Amp\CancelledException note for `$cancellation` consistent.
Comment on lines
+137
to
+166
| $clientA = new ARCPClient($clientTA); | ||
| $clientB = new ARCPClient($clientTB); | ||
| $clientA->open(Auth::none(), new PeerInfo('cli-a', '0.1'), new Capabilities(anonymous: true)); | ||
| $clientB->open(Auth::none(), new PeerInfo('cli-b', '0.1'), new Capabilities(anonymous: true)); | ||
|
|
||
| // Both sessions push some events that get recorded in the event log. | ||
| $clientA->ping(); | ||
| $clientB->ping(); | ||
| $clientA->ping(); | ||
|
|
||
| // Session A asks for a full replay from the beginning. The bug | ||
| // pre-fix would forward session B's envelopes too. | ||
| $resumeEnv = new Envelope( | ||
| id: MessageId::random(), | ||
| payload: new Resume(afterMessageId: ''), | ||
| timestamp: new \DateTimeImmutable('now'), | ||
| sessionId: $clientA->session->sessionId, | ||
| ); | ||
| $clientTA->send($resumeEnv); | ||
|
|
||
| $foreignSessionId = (string) $clientB->session->sessionId; | ||
| $sawForeign = false; | ||
| $sawAck = false; | ||
| $loops = 0; | ||
| while (!$sawAck && $loops < 200) { | ||
| ++$loops; | ||
| $env = $clientTA->receive(); | ||
| if (!$env instanceof Envelope) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Avoid dual consumers on the same transport in this test.
After open(), the ARCPClient read-loop is already consuming envelopes, but Line 163 also reads directly from $clientTA. That race makes this test nondeterministic (ack/replay envelopes may be consumed by either reader, causing flakes/hangs). Please drive assertions through a single consumer path (e.g., raw transport-only test client, or ARCPClient-managed pending/router path exclusively).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Integration/ResumeTest.php` around lines 137 - 166, The test currently
has two consumers reading from the same transport (ARCPClient's internal
read-loop started by ARCPClient::open and the direct $clientTA->receive()
calls), which causes nondeterministic races; fix by choosing one consumer path
and removing the other: either stop using ARCPClient::open and drive the test
via the raw transport client ($clientTA) only (send the Resume envelope with
$clientTA->send and assert/read all replies with $clientTA->receive), or stop
using $clientTA->receive() and assert replay/ack via the ARCPClient instance
(use ARCPClient methods/events or its receive/queueing API) so only ARCPClient
reads the transport; adjust the code around Envelope/Resume handling and checks
that reference $clientA->session->sessionId, $clientTA->send, and the receive
loop to use the single chosen consumer.
Comment on lines
+149
to
+154
| $aSessionId = (string) $clientA->session->sessionId; | ||
| $bSessionId = (string) $clientB->session->sessionId; | ||
| self::assertArrayNotHasKey($bSessionId, $aObservedSessions); | ||
| // Allow A to see its own envelopes (e.g. backfill marker). | ||
| $foreign = array_diff(array_keys($aObservedSessions), [$aSessionId]); | ||
| self::assertEmpty($foreign); |
There was a problem hiding this comment.
Test can pass without proving the subscription actually delivered any envelope.
At Line 151–Line 154, the assertions are only negative checks. If $aObservedSessions is empty, this still passes. Add a positive assertion that A observed at least its own session id.
Suggested patch
$aSessionId = (string) $clientA->session->sessionId;
$bSessionId = (string) $clientB->session->sessionId;
+ self::assertArrayHasKey($aSessionId, $aObservedSessions, 'expected at least one envelope for subscriber session');
self::assertArrayNotHasKey($bSessionId, $aObservedSessions);
// Allow A to see its own envelopes (e.g. backfill marker).
$foreign = array_diff(array_keys($aObservedSessions), [$aSessionId]);
self::assertEmpty($foreign);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $aSessionId = (string) $clientA->session->sessionId; | |
| $bSessionId = (string) $clientB->session->sessionId; | |
| self::assertArrayNotHasKey($bSessionId, $aObservedSessions); | |
| // Allow A to see its own envelopes (e.g. backfill marker). | |
| $foreign = array_diff(array_keys($aObservedSessions), [$aSessionId]); | |
| self::assertEmpty($foreign); | |
| $aSessionId = (string) $clientA->session->sessionId; | |
| $bSessionId = (string) $clientB->session->sessionId; | |
| self::assertArrayHasKey($aSessionId, $aObservedSessions, 'expected at least one envelope for subscriber session'); | |
| self::assertArrayNotHasKey($bSessionId, $aObservedSessions); | |
| // Allow A to see its own envelopes (e.g. backfill marker). | |
| $foreign = array_diff(array_keys($aObservedSessions), [$aSessionId]); | |
| self::assertEmpty($foreign); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Integration/SubscriptionTest.php` around lines 149 - 154, The test only
performs negative checks and can pass when $aObservedSessions is empty; add a
positive assertion that A actually observed its own session by asserting
$aSessionId is present in $aObservedSessions (e.g. call
self::assertArrayHasKey($aSessionId, $aObservedSessions) or
self::assertContains($aSessionId, array_keys($aObservedSessions))) before
computing $foreign so the test verifies the subscription delivered at least one
envelope for A.
This was referenced May 25, 2026
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
Closes #34, #35, #36, #37, #38, #39, #40, #41, #42, #44, #45, #46, #47, #48, #49, #50.
Security & session isolation
BearerAuthno longer trusts the client-suppliedPeerInfo.principal; the token → principal mapping is always authoritative.Resumereplay is scoped to the calling session via a newEventLog::replayAfterForSession(). A foreignafter_message_idis rejected withDATA_LOSS.session_idfilter default to the requesting session id instead of fanning out across all sessions.ArtifactStore::fetch()/release()reject cross-session artifact ids withPERMISSION_DENIED(newgetForSession-style overload).LeaseManager::register()records the granting session; newgetForSession()is used inCredentialLifecyclesotool.invokecan no longer borrow another session's lease scope.Correctness
tool.result/tool.error(looked up via newEventLog::findByMessageId()), instead of returning a bareAckand leaving the client to fail/time out.ResultChunkAssembler::assemble()now enforces terminal-chunk delivery, contiguous0..Nsequence, and duplicate consistency.CostBudgetaccepts small floats that PHP stringifies as scientific notation, rejectsNaN/Inf, and rejects over-precise budget patterns.ArtifactPut.sha256is validated against payload bytes; the clientputArtifact()accepts and surfaces digest errors.ARCPClient::ping()/putArtifact()mapNackthroughErrorMapperconsistently with the other helpers and reject unexpected response types.StdioTransport::receive()drains and decodes a final unterminated frame at EOF instead of dropping it.InMemoryCredentialStore::supportsDurableRevocation()returnsfalse; runtime correctly refuses to pair it with aCredentialProvisioner. A smallFakeDurableCredentialStoretest double is added for tests that need to exercise the provisioner path.JobManagerretains terminal jobs for a bounded retention window (default 15 min).session.list_jobscan now surface recent completed/failed/cancelled jobs.Documentation
tool.invoke,tool.result,job.progress,job.result_chunk,artifact.ref, etc.).@throwsdocumentation toARCPClient::{open,invokeTool,subscribe,listJobs,ping,putArtifact,fetchArtifact,releaseArtifact},JobContext::{requestHumanInput,requestHumanChoice,requestPermission},ARCPRuntime::setDefaultToolVersion, and theTransportinterface.Test plan
composer test— 360 tests, 803 assertions, OKcomposer lint— cleancomposer stan— no errorscomposer psalm— no errors🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Security Improvements
Bug Fixes
Documentation