From b725788954d8beaa26491b9581fbc7aee755a495 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:28:55 -0400 Subject: [PATCH] docs(plans): R3-01 execution notes (T1-T9) - filter gotcha, T4/T5 fixture scope, observed cookie-challenge signature --- .../R3-01-mesh-grpc-allowanonymous-plan.md | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md b/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md index a0400ede..4f552be4 100644 --- a/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md +++ b/archreview/plans/R3-01-mesh-grpc-allowanonymous-plan.md @@ -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 :` 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.