docs(plans): R3-01 execution notes (T1-T9) - filter gotcha, T4/T5 fixture scope, observed cookie-challenge signature
This commit is contained in:
Joseph Doherty
@@ -352,3 +352,87 @@ T10 last — and its step-0 control runs on master BEFORE the branch is merged
|
||||
```
|
||||
|
||||
Overall effort: **~half a dev-day code+tests, plus the two-pass rig session.** The Critical itself is five lines; everything else in this plan exists because the five lines were invisible to a seven-phase live-gate program.
|
||||
|
||||
---
|
||||
|
||||
## Execution notes (T1–T9, 2026-08-20)
|
||||
|
||||
Branch `fix/r3-01-mesh-grpc-allowanonymous`, one commit per task as specified. T1–T9 all landed;
|
||||
T10 (live gate) left `pending` for the orchestrator per the run brief. Every code-touching task
|
||||
(T1–T7) was verified red-before-fix by hand — for T2–T5 by asserting against the un-fixed
|
||||
`MeshGrpcEndpoints.cs`/`Program.cs` (via a temporary `git show <parent-commit>:<path>` swap,
|
||||
rebuild, run, then restore — confirmed byte-identical restore via `git diff --stat` each time)
|
||||
and observing the expected failure before reapplying the fix. Two things surfaced during that
|
||||
process that are worth recording:
|
||||
|
||||
1. **The census's endpoint filter could not stay a "route pattern starts with `/`" heuristic once
|
||||
T3's empty-prefix `MapGroup` landed.** `RealMethodEndpoints` in `MeshGrpcAnonymousMetadataTests`
|
||||
originally filtered on `RoutePattern.RawText?.StartsWith('/')` to exclude grpc-dotnet's own
|
||||
per-app "Unimplemented service" catch-all and per-service "Unimplemented method for X"
|
||||
fallback (neither carries a route pattern starting with `/` in the ungrouped case). Once
|
||||
LocalDbSync moved behind `app.MapGroup(string.Empty).AllowAnonymous().MapZbLocalDbSync()`,
|
||||
those SAME fallback patterns *also* gained a leading `/` inside the group, so the heuristic
|
||||
started over-counting (`DriverOnly_WithASyncPort_AlsoMapsTheLocalDbSyncEndpoint` and
|
||||
`LocalDbSync_AllowAnonymousGroup_LeavesTheRoutePatternByteIdentical` both went red for the
|
||||
wrong reason — 3 endpoints found where 1 was expected). Fixed by matching against the exact
|
||||
known set of five method route-pattern literals instead of a shape heuristic. Not a plan defect
|
||||
— the plan didn't specify the filter mechanism — but a genuine gotcha for anyone extending this
|
||||
census: **grpc-dotnet's route-pattern-shape for its own fallback endpoints is not stable across
|
||||
a plain `MapGrpcService` vs. a grouped one**, so don't filter on shape once any mesh map moves
|
||||
behind a group.
|
||||
|
||||
2. **T4's fixture does not map or wire `LocalDbSyncService`'s real dependencies.** The plan's
|
||||
fixture description says "maps the mesh services through `MapOtOpcUaMeshGrpcServices`"; T4's
|
||||
`MeshGrpcAuthStackTests` calls it with `syncListenPort: 0` (LocalDbSync NOT mapped by the
|
||||
helper) and instead hand-maps the library's `LocalDbSyncService` directly, with no
|
||||
`.AllowAnonymous()`, purely as the negative control. This is a minimal, deliberate narrowing:
|
||||
no assertion in T4 or T5 exercises LocalDbSync's real RPC (the plan's own positive/negative
|
||||
lists never name it), and standing up its DI graph (`AddZbLocalDb`/`AddZbLocalDbReplication`,
|
||||
a temp-file SQLite `ILocalDb`) would have added fixture weight with nothing to verify — the
|
||||
authorization middleware refuses an unauthorized call before the service is ever constructed,
|
||||
so the DI graph doesn't need to exist for the negative control to be meaningful. Recorded here
|
||||
because it's a case where the fixture is deliberately narrower than a literal reading of the
|
||||
plan's prose, even though every named assertion is still covered.
|
||||
|
||||
3. **The positive/negative service lists T4 and T5 assert are asymmetric, and that asymmetry is
|
||||
the plan's own, not introduced here.** T4's positive list is {`DeploymentArtifactService.Fetch`,
|
||||
`CentralCommandService.ReportApply`, `NodeCommandService.Execute`} — Telemetry is not asserted
|
||||
positively there. T5's no-PSK detail-string list is {Config-serve, Telemetry stream, Node
|
||||
command} — `CentralCommandService`'s own detail string
|
||||
(`"Central command authentication failed."`) is never asserted anywhere in T4/T5. Implemented
|
||||
exactly as specified (no scope widening per the run brief); flagging the gap in case a
|
||||
follow-up wants a `NoOrWrongPsk_CentralCommandReportApply_FailsWithTheInterceptorsDetail` case
|
||||
for full symmetry — trivial to add given the fixture already maps and keys that service.
|
||||
|
||||
4. **Observed cookie-challenge signature (T5 step 2), captured directly against this repo's
|
||||
fixture stack (macOS, Kestrel h2c, `Grpc.Net.Client` over `HttpClient`/`SocketsHttpHandler`):**
|
||||
every blocked positive-test call in the red state failed as
|
||||
`RpcException(StatusCode.Internal, "Error starting gRPC call. HttpRequestException: The HTTP/2
|
||||
server reset the stream. HTTP/2 error code 'PROTOCOL_ERROR' (0x1). (HttpProtocolError)
|
||||
HttpProtocolException: The HTTP/2 server reset the stream. HTTP/2 error code 'PROTOCOL_ERROR'
|
||||
(0x1). (HttpProtocolError)")` — never `Unauthenticated`/`PermissionDenied`. The cookie handler's
|
||||
redirect/401 breaks the prior-knowledge h2c stream before any gRPC trailers are written, so the
|
||||
client sees a transport-level reset rather than a clean gRPC status. **The live gate (T10)
|
||||
should grep node/central logs and browser network traces for `PROTOCOL_ERROR`, `/login`, `302`
|
||||
and `401` together** — not for `401` alone, and not for a gRPC `Unauthenticated`/
|
||||
`PermissionDenied` status, which this environment never actually produces for the blocked case.
|
||||
(A real reverse-proxy in front of a production deployment could still surface a clean HTTP 401
|
||||
instead of a stream reset, depending on how it terminates HTTP/2 — the plan's own multi-signal
|
||||
grep advice already covers that; this note is the empirical confirmation of one concrete shape.)
|
||||
|
||||
5. **T6 passes identically on both the pre-fix and post-fix mapper (confirmed by the same
|
||||
temporary-swap procedure).** Driver-only nodes never register `AddOtOpcUaAuth` at all, so
|
||||
`.AllowAnonymous()` presence or absence on the mesh maps has zero effect on that shape — T6 is
|
||||
a structural pin/regression-control test, not a red-then-green TDD test for a behaviour T1–T3
|
||||
changed. This matches the plan's own "standard (test-only)" classification; recorded because
|
||||
the run brief asked for red-before-fix confirmation to be checked (and reported) for every task,
|
||||
and this is the one task where "red" never existed to begin with.
|
||||
|
||||
6. **T8's `docker compose config` verification ran locally on macOS (OrbStack-backed Docker
|
||||
Compose v5.1.2)** — no Windows-VM `lmxopcua-fix` helper needed, consistent with the Mac-path
|
||||
guidance in CLAUDE.md. Confirmed both branches: unset `OTOPCUA_DISABLE_LOGIN` resolves
|
||||
`Security__Auth__DisableLogin: "true"` on both central-1/central-2; `OTOPCUA_DISABLE_LOGIN=false`
|
||||
resolves `"false"` on both.
|
||||
|
||||
No other deviations from the plan's design or task breakdown. T10 was not run — left `pending`
|
||||
per the run brief, for the orchestrator.
|
||||
|
||||
Reference in New Issue
Block a user