Skip to content

fix(authz): route checkAccess/can/batchCheck through the instrumented path (F3) - #25

Merged
ilpanich merged 2 commits into
mainfrom
claude/improvements-run5-benchmark-def-bazzei
Aug 10, 2026
Merged

fix(authz): route checkAccess/can/batchCheck through the instrumented path (F3)#25
ilpanich merged 2 commits into
mainfrom
claude/improvements-run5-benchmark-def-bazzei

Conversation

@ilpanich

Copy link
Copy Markdown
Owner

The example found a real bug

Writing the F3 example for this SDK found a defect in the already-merged D5 work — which is the whole reason the example exists.

examples/telemetry_hook.php drives the ordinary public API. Its first run, against an unreachable server, printed:

--- telemetry ---
  retries: (none)
  refreshes: 0

No retries. No request events. One attempt.

Cause

checkAccess(), can() and batchCheck() posted directly through private helpers with no §16 retry budget, no §17 memo and no §19 request pair. Only checkAccessDecision() was reachable from the instrumented path — and batchCheckDecisions() was itself uninstrumented, so batch had no retry on either surface despite §16.2 naming batch_check as eligible.

The D5 conformance suite drives checkAccessDecision exclusively, so it was green throughout.

That is precisely what §16.7 was written about:

"A tested surface nobody calls is worse than an absent one: the passing tests are exactly what stop anyone from looking."

…reproduced in the very fan-out that introduced the clause, and visible only from the surface a caller actually reaches. The clause said to count requests on the wire through the public surface; the suite counted them through a public surface, and picked the one callers use least.

Fix

  • checkAccess → delegates to checkAccessDecision, takes .allowed
  • batchCheck → delegates to batchCheckDecisions
  • batchCheckDecisions → now wraps its send in RetryPolicy::execute with a §19 span (new sendBatch, mirroring sendCheck) and its own BATCH_PATH template
  • dead decodeAllowed helper removed

Delegating rather than duplicating the instrumentation is the point: there is now one instrumented path per operation and no second one to forget.

The memo is deliberately still not applied to batch — the §17 key is per-check, so a batch would split into n entries with n keys, changing what a partial hit means. §17 says nothing about batch, so this takes the conservative reading rather than inventing semantics.

Tests

Six cases added, all counting requests on the wire through the bare-bool surface:

Case Asserts
checkAccess retries 2 requests on a 503→200 script
checkAccess attempt cap exactly MAX_ATTEMPTS on persistent 503
can alias retries an alias that didn't would be a hole in the policy
checkAccess telemetry start, end, retry, start, end
batchCheck retries 2 requests
batchCheck telemetry operation batchCheck, path template /api/v1/authz/check/batch

Verification

PHPUnit cannot be installed in this sandbox — composer still fails Could not authenticate against github.com through the proxy. CI is the verifier for the suite, PHPStan and the docblock gate.

The fix itself is demonstrated directly by re-running the example:

--- telemetry ---
  checkAccess/failure: count=3 mean=3ms
  retries checkAccess: 2
  refreshes: 0

Three attempts, two retries, events emitted — matching the C, C++ and C# SDKs. For this defect that is the stronger evidence anyway: the bug was that the ordinary path was uninstrumented, and the example is the only check that exercises the ordinary path.

I also verified every touched method still sits directly under its own docblock — the orphaned-docblock mistake that has broken the docblock gate in this repo before.


Generated by Claude Code

claude added 2 commits August 10, 2026 10:34
… path (F3)

Writing the F3 example for this SDK found a real defect in the merged D5 work,
which is the whole reason the example exists.

examples/telemetry_hook.php drives the ordinary public API. Its first run, on
an unreachable server, printed:

  --- telemetry ---
    retries: (none)
    refreshes: 0

No retries. No request events. One attempt.

CAUSE. checkAccess(), can() and batchCheck() posted directly through private
helpers with no §16 retry budget, no §17 memo and no §19 request pair. Only
checkAccessDecision() and batchCheckDecisions() were reachable from the
instrumented path — and batchCheckDecisions was itself uninstrumented, so
batch had no retry on either surface despite §16.2 naming batch_check as
eligible.

The D5 conformance suite drives checkAccessDecision exclusively, so it was
green throughout. That is precisely what §16.7 was written about — "a tested
surface nobody calls is worse than an absent one, because the passing tests
are what stop anyone from looking" — reproduced in the SDK fan-out that
introduced the clause, and visible only from the surface a caller reaches.

FIX. checkAccess delegates to checkAccessDecision and takes .allowed;
batchCheck delegates to batchCheckDecisions; batchCheckDecisions now wraps its
send in RetryPolicy::execute with a §19 span (new sendBatch, mirroring
sendCheck) and its own BATCH_PATH template. Delegating rather than duplicating
the instrumentation is the point: there is now one instrumented path per
operation and no second one to forget. The dead decodeAllowed helper is gone.

The memo is deliberately still not applied to batch: the §17 key is per-check,
so a batch would split into n entries with n keys, which changes what a partial
hit means. §17 says nothing about batch, so this takes the conservative reading
rather than inventing semantics.

TESTS. Six cases added, all counting requests on the wire through the bare-bool
surface: checkAccess retries and honours the attempt cap, `can` inherits the
policy (an alias that did not would be a hole in it), checkAccess emits the
start/end/retry/start/end sequence, batchCheck retries, and batchCheck's
telemetry carries the batch path template rather than the single-check one.

VERIFICATION. PHPUnit cannot be installed in this sandbox — composer still
fails "Could not authenticate against github.com" through the proxy, so CI is
the verifier for the suite, PHPStan and the docblock gate. The fix itself is
demonstrated directly by re-running the example, which now prints:

  --- telemetry ---
    checkAccess/failure: count=3 mean=3ms
    retries checkAccess: 2
    refreshes: 0

Three attempts, two retries, events emitted — matching the C, C++ and C# SDKs.
For this defect that is the stronger evidence anyway: the bug was that the
ordinary path was uninstrumented, and the example is the only check that
exercises the ordinary path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ubrFbqsMkBqC5gwadPsDu
…ttempt

CI caught eight failures from the previous commit, all one root cause: tests
that queue ONE mock response for a retryable failure. checkAccess now retries,
so attempts 2-3 hit "Mock queue is empty" and the test fails for a reason
unrelated to what it asserts.

That is the correct failure. Those tests encoded the old single-attempt
behaviour, which was the bug.

Two different fixes, because the tests want two different things.

Six MAPPING tests (AuthzRestClientErrorTest x5, CoverageEdgeCasesTest x1) are
named for a translation — a malformed 200 fails closed as NetworkError, a
ConnectException maps to NetworkError, a 500 maps to NetworkError. Retry is
orthogonal to all of them, so they now construct with `retry: false` and keep
asserting exactly one thing. §16's behaviour has its own wire-count tests in
D5ConformanceTest; duplicating it into every mapping test would only mean more
places to update next time the policy moves.

Two GUARD tests (AccessEnforcerTest) are a different case, and disabling retry
there would have thrown away the interesting part. §16.4 says the retry budget
is spent FIRST and the guard denies when it is exhausted — the budget is not
extended because the caller is a guard, and the guard does not admit a request
because retries were attempted. Queueing the whole budget makes those two
tests assert that composed behaviour instead of the pre-§16 one, so they are
stronger than before rather than merely repaired.

VERIFICATION — and a correction to the previous commit message. PHPUnit is
installable here after all: composer cannot authenticate against github.com,
but the PHAR from phar.phpunit.de downloads fine, so the suite CAN be run
locally and my earlier "CI is the verifier" was a limit I accepted too
readily.

  AuthzRestClientErrorTest  10/10 OK
  CoverageEdgeCasesTest      9/9  OK
  D5ConformanceTest         28/28 OK  (22 before, +6 for the bool surface)
  AccessEnforcerTest        blocked locally — symfony/http-foundation is a dev
                            dependency the partial vendor/ lacks, so every case
                            errors on JsonResponse regardless of this change.
                            CI has it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ubrFbqsMkBqC5gwadPsDu
@ilpanich
ilpanich merged commit a4f5e18 into main Aug 10, 2026
6 checks passed
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.

2 participants