Skip to content

Commit 254e511

Browse files
cemililikclaude
andcommitted
fix(infra): compare the gate's service sets, not their counts
Second review round on the PR. Nine findings were real; four were not, and are listed at the bottom with what settled them. **The readiness gate could pass a stack missing a declared service.** Its exit condition was `no unhealthy rows && observed == declared`. One service never created plus one orphan still running gives equal counts, so the gate broke out reporting success. I added the missing/extra sets in the previous commit for the diagnostic message and left the exit test on counts — the sets were computed one branch too late to be used for the decision. Proven on a live stack rather than argued: `meilisearch` removed, a healthy orphan `ghost` created by Compose itself under the same project, 14 observed against 14 declared, everything else green. Old condition → `exit 0`, "All compose services running; every healthcheck green". New condition → `exit 1`, "saw 14 of 14 declared services; never created: meilisearch; orphaned (docker compose down --remove-orphans): ghost". Healthy stack still exits 0. Getting that proof took two false starts worth recording, both the same shape as everything else this branch has been fixing. A hand-built orphan carrying Compose's labels is not counted by `docker compose ps`, so the scenario I first "reproduced" never existed. Then the old-condition copy ran from the scratchpad, where `REPO_ROOT` resolves relative to `BASH_SOURCE`, so it inspected an empty directory and failed for a reason that had nothing to do with the condition under test. **`TenantContextBehavior`'s comment described RLS as working.** It read "they are issued by `TransactionBehavior` at step 6". `grep -rn "set_config\|SET LOCAL" backend/src` returns nothing: no code in this repository sets the GUCs, and RLS is not enforced at runtime until Packet 7. The reasoning about *why* the GUCs belong inside the transaction is correct and stays; the tense does not. **ADR-0032's registration sample was still missing its guards** — the three `ArgumentNullException`/`ArgumentException` lines the shipped `ProviderResilienceRegistration` opens with. This is the same fence whose earlier comparison reported "13 lines, zero differences" because the normaliser dropped exactly the `ArgumentNullException` lines. The skill's copy has them; the ADR's did not. Recorded in Amendment 2; § Decision is byte-identical to `main` (same md5). **Phase 01's defect list said "All of these were remediated".** One of its six entries is branch protection, which was *deferred by maintainer decision*, not fixed. Now scoped, with the trigger named. **`DROP DATABASE keycloak` was documented with Keycloak still running.** PostgreSQL refuses to drop a database with a connected client, and Keycloak holds its pool for its whole lifetime. Stop first, `up -d` after. **`local-dev-setup` grew a fifth copy of the service inventory.** The previous commit rebuilt that table from `docker compose config` — accurate, and a second source of truth next to `infra/compose/README.md`, which carries image, endpoint and credentials for every service. Replaced with a link plus the three facts a reader setting up actually needs from it. Also: both canonical producer fences dropped `CohortId`, a field the command carries and the event declares, so anyone copying either silently lost it; README's "two images cannot carry one" conflated the gate's input (Compose service definitions) with the reason (`daprio/placement` and `daprio/daprd` ship a single binary on an empty base), and now names both. **The three new Standards 12 sections cite their derivation** — ADR-0002 for the compose-stack rules, ADR-0035 for development credentials. I did not mint an ADR: these tighten how an existing decision is operated rather than deciding anything new, and the file already derives from six ADRs. If you want a dedicated record for the dev-loop hygiene rules, that is your call to make. My own check caught that I had cited a "§ Network Exposure" heading that does not exist in Standards 11; it points at § Transport now. Not changed, with the reason: - **Convert the adapter's `text` flow diagram to mermaid.** CLAUDE.md's mermaid rule governs documentation diagrams and requires a readable text fallback — which is what this fence already is. Three skills use `text` flows and one uses mermaid; converting one of the three makes the corpus less consistent, not more. A sweep is its own change. - **Rewrite ADR-0017's remaining Identity references.** Deliberate. Amendment 2 carries the correction and dated banners sit above each stale passage; the body below them is the record of what was decided in May. Editing it would undo the governance choice the amendment exists to express. - **"`List<Entity<CourseId>>.Contains(b)` should say same-id true, different-id false."** The sentence is describing the *defect* a sneaky explicit re-implementation produces — `Contains` answering true for different ids — which is the measured behaviour the entry exists to warn about. Correct as written. - **Add `.env` as a prerequisite to `down` / `clean` / `logs` / `ps`.** Those targets work without it: every compose variable has a `${VAR:-default}` fallback and the project name is a literal in `dev.yml`. Creating a file as a side effect of tearing down is worse than the inconsistency. 227 tests green, build 0/0, format clean, 1337 links resolve, gate exercised against a live 14-service stack and torn down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 1f62d3d commit 254e511

10 files changed

Lines changed: 81 additions & 51 deletions

File tree

‎.claude/skills/add-integration-event/SKILL.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ await outbox.EnqueueAsync(new EnrollmentCreatedIntegrationEventV1
8787
EnrollmentId = enrollment.Id.Value,
8888
LearnerId = request.LearnerId.Value,
8989
CourseVersionId = request.CourseVersionId.Value,
90+
CohortId = request.CohortId?.Value,
9091
Source = request.Source.ToString().ToLowerInvariant(),
9192
}, cancellationToken);
9293

‎.claude/skills/add-mediatr-handler/SKILL.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ public sealed class CreateEnrollmentCommandHandler(
150150
EnrollmentId = enrollment.Id.Value,
151151
LearnerId = request.LearnerId.Value,
152152
CourseVersionId = request.CourseVersionId.Value,
153+
CohortId = request.CohortId?.Value,
153154
// OccurredAt is auto-populated by IntegrationEventBase — do not set
154155
// it manually. If you need it explicitly, inject IClock and use
155156
// clock.UtcNow per 02-backend-coding.md § Time.

‎.claude/skills/local-dev-setup/SKILL.md‎

Lines changed: 22 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -105,34 +105,28 @@ What `make dev` expands to:
105105
docker compose --env-file .env -f infra/compose/dev.yml up -d
106106
```
107107

108-
The components and their default ports:
109-
110-
| Component | Published port(s) | Purpose |
111-
|-----------|-------------------|---------|
112-
| Postgres | 5432 | Main DB. |
113-
| Valkey | 6379 | Cache + Dapr state. |
114-
| Kafka | *(none)* | Dapr pub/sub backend. Reached over the compose network only — nothing on the host speaks to it directly. |
115-
| Kafka UI | 8081 | Optional UI for topics. |
116-
| Vault (dev mode) | 8200 | Secrets backend; dev root token, **not** for production. |
117-
| Keycloak | 8080 | Two realms: `learnstack` (tenants) + `learnstack-hub` (operators). |
118-
| SeaweedFS | 9000 (S3 API) / 9001 (filer UI) / 9333 (master) / 8084 | Object storage (single dev binary: master + volume + filer + S3 gateway). |
119-
| Meilisearch | 7700 | Search. |
120-
| LiveKit | 7880 (API) / 7881 (TLS) / 7882 (RTC) / 50000-50100 (UDP media) | Live classroom. |
121-
| coturn | 3478 (UDP+TCP) / 5349 / 49152-49200 (UDP relay) | TURN for LiveKit. |
122-
| Mailpit | 8025 (UI) / 1025 (SMTP) | Captures outbound email in dev. |
123-
| Dapr sidecar (api) | 3500 (HTTP) / 50001 (gRPC) | Building-block runtime. |
124-
| Dapr placement | *(none)* | Actor placement; compose network only. |
125-
| APISIX | 9080 (HTTP gateway) / 9443 (HTTPS) / 9091 (Prometheus metrics) | File-driven standalone (`data_plane`) per ADR-0015 — no Admin API, no dashboard companion. `apisix.yaml` is the only source of truth. |
126-
127-
Every one of these binds to `127.0.0.1`, never `0.0.0.0` — see
128-
[Infrastructure Standards § Published ports](../../../docs/standards/12-infrastructure.md).
129-
Regenerate the table from the stack itself rather than by hand —
130-
`docker compose --env-file .env -f infra/compose/dev.yml config --format json`
131-
is the source of truth.
132-
133-
Neither application host is a compose service. `LearnStack.Api` runs on the
134-
workstation via `dotnet run` on the `ASPNETCORE_URLS` port in `.env.example`
135-
(5080), and `apps/web` runs via `pnpm dev` on 3000.
108+
The canonical service inventory — image, local endpoint and default credentials
109+
for every service in the stack — lives in
110+
[`infra/compose/README.md`](../../../infra/compose/README.md), grouped by the
111+
Phase 01 packet that introduced each one. It is not repeated here; a second copy
112+
is a second thing to keep true, and this file has no way to notice when
113+
`dev.yml` changes.
114+
115+
Three properties of that inventory matter while you are setting up:
116+
117+
- **Every published port binds `127.0.0.1`, never `0.0.0.0`** — see
118+
[Infrastructure Standards § Published ports](../../../docs/standards/12-infrastructure.md).
119+
- **Kafka and the Dapr placement service publish nothing.** They are reached
120+
over the compose network only; nothing on the host speaks to them directly.
121+
- **Neither application host is a compose service.** `LearnStack.Api` runs on
122+
the workstation via `dotnet run` on the `ASPNETCORE_URLS` port in
123+
`.env.example` (5080), and `apps/web` runs via `pnpm dev` on 3000.
124+
125+
To read the resolved truth rather than any document, ask the stack:
126+
127+
```bash
128+
docker compose --env-file .env -f infra/compose/dev.yml config --format json
129+
```
136130

137131
### Step 4: First-run bootstrap
138132

‎README.md‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,11 @@ make dev # bring local stack up
7373
make seed # verify health + print demo credentials
7474
```
7575

76-
> `make seed` exits 0 on a clean stack. The health gate used to time out on every run —
77-
> it flagged any service with no healthcheck, and two images cannot carry one.
76+
> `make seed` exits 0 on a clean stack. The health gate used to time out on every run:
77+
> it read every service's `healthcheck:` block in the Compose file and flagged any
78+
> service that declared none, but `dapr-placement` and `dapr-sidecar-api` cannot
79+
> declare one — their images ship a single binary on an empty base, with no shell
80+
> and no `wget`/`curl`/`nc` to probe with.
7881
> [Phase 02a Packet 3b](docs/roadmap/phase-02a-kernel-tenancy.md) repaired that along
7982
> with the rest of the Phase 01 development-loop debt.
8083

‎backend/src/LearnStack.Application/Pipeline/TenantContextBehavior.cs‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,17 @@ public Task<TResponse> Handle(
4949
return Task.FromResult(Result.FailFor<TResponse>(TenantMismatchError));
5050
}
5151

52-
// Nothing to do here for RLS. The GUCs are transaction-local
53-
// (set_config('app.tenant_id', ..., true)), and this behavior runs at
54-
// step 4 with no transaction open, so they are issued by
55-
// TransactionBehavior at step 6 as the first statement inside the
56-
// transaction — Security Standards § Tenant Context. A
57-
// DbConnectionInterceptor cannot do it either: it fires at connection
58-
// open, not at transaction start, so the value would be discarded
59-
// before the query it protects ever runs.
52+
// Nothing to do here for RLS, and nothing anywhere else yet: no code
53+
// in this repository issues set_config today. Packet 7 adds it to
54+
// TransactionBehavior, and until then RLS is not enforced at runtime.
55+
//
56+
// Why it belongs there and not here: the GUCs are transaction-local
57+
// (set_config('app.tenant_id', ..., true)) and this behavior runs at
58+
// step 4 with no transaction open, so the value would be discarded
59+
// before the query it protects ever runs. TransactionBehavior at step 6
60+
// issues them as the first statement inside the transaction — Security
61+
// Standards § Tenant Context. A DbConnectionInterceptor cannot do it
62+
// either: it fires at connection open, which precedes BEGIN.
6063

6164
return next();
6265
}

‎docs/decisions/0032-exception-handling-logging-and-observability.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -575,6 +575,10 @@ public static class ProviderResilienceRegistration
575575
// no ResilientProviderAdapter<TPort> can satisfy `: TPort`. Adapters take
576576
// IProviderResilience<TPort> as a collaborator and route outbound calls
577577
// through Pipeline.ExecuteAsync themselves.
578+
ArgumentNullException.ThrowIfNull(services);
579+
ArgumentNullException.ThrowIfNull(configuration);
580+
ArgumentException.ThrowIfNullOrWhiteSpace(portName);
581+
578582
var options = configuration
579583
.GetSection($"Resilience:{portName}")
580584
.Get<ResilienceOptions>() ?? new ResilienceOptions();

‎docs/roadmap/phase-01-repository-tooling.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,12 @@
111111
>
112112
> ### Known defects this phase shipped with, and where they are fixed
113113
>
114-
> All of these were remediated in **[Phase 02a Packet 3b](phase-02a-kernel-tenancy.md)**
115-
> — see its [delivery record](phase-02a-kernel-tenancy.md#delivery-record-packet-3b) —
114+
> All but one were remediated in **[Phase 02a Packet 3b](phase-02a-kernel-tenancy.md)**
115+
> — see its [delivery record](phase-02a-kernel-tenancy.md#delivery-record-packet-3b).
116+
> The exception is **branch protection**: the missing approval and
117+
> bypass settings are *deferred by maintainer decision*, not fixed, and stay
118+
> that way until a second contributor gains write access — see
119+
> [CONTRIBUTING § Branch protection](../../.github/CONTRIBUTING.md). Packet 3b
116120
> which exists precisely so that Phase 01's record does not have to be rewritten. The
117121
> list below is kept as written, because what a phase shipped with is part of its
118122
> record:

‎docs/standards/12-infrastructure.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,10 @@ otel-collector # Phase 11 (Production hardening — observability stack
8888

8989
### Healthchecks and the readiness gate
9090

91+
**Derives from:** [ADR-0002 Initial Architecture](../decisions/0002-initial-architecture.md)
92+
— these tighten how the local development stack that ADR describes is operated;
93+
they introduce no new architectural decision.
94+
9195
- **A service in the default compose profile declares a `healthcheck` when one can be
9296
written.** `make seed` waits on the default-profile set and treats a service that is
9397
running but not `healthy` as not ready.
@@ -106,6 +110,9 @@ otel-collector # Phase 11 (Production hardening — observability stack
106110

107111
### Published ports
108112

113+
**Derives from:** [ADR-0002 Initial Architecture](../decisions/0002-initial-architecture.md)
114+
and [Security Standards § Transport](11-security.md#transport).
115+
109116
- **Every published port binds `127.0.0.1`** — `"127.0.0.1:5432:5432"`, never
110117
`"5432:5432"`. A bare mapping listens on every interface, so a laptop on a café
111118
network publishes its development database, and `dev.yml` ships committed development
@@ -124,6 +131,10 @@ otel-collector # Phase 11 (Production hardening — observability stack
124131

125132
### Development credentials
126133

134+
**Derives from:** [ADR-0035 Demand-Gated Infrastructure](../decisions/0035-demand-gated-infrastructure.md)
135+
— `ConfigurationSecretProvider` is the shipped default, so `.env` is the local
136+
expression of the secret port rather than a parallel mechanism.
137+
127138
- **Compose files carry no bare credential literals.** Every credential is
128139
`${VAR:-fallback}` so `.env` can override it and the fallback is visibly a default.
129140
`.env.example` lists every such variable — it is the source of truth for what a

‎infra/keycloak/README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,11 +107,14 @@ default behaviour). To force a clean re-seed of the realm itself, wipe the
107107
Keycloak database first:
108108

109109
```bash
110+
# Stop Keycloak first: PostgreSQL refuses to drop a database that still has a
111+
# connected client, and Keycloak holds a pool open for its whole lifetime.
112+
docker compose --env-file .env -f infra/compose/dev.yml stop keycloak
110113
docker compose --env-file .env -f infra/compose/dev.yml exec postgres \
111114
psql -U learnstack -d learnstack -c "DROP DATABASE keycloak;"
112115
docker compose --env-file .env -f infra/compose/dev.yml exec postgres \
113116
psql -U learnstack -d learnstack -c "CREATE DATABASE keycloak OWNER learnstack;"
114-
docker compose --env-file .env -f infra/compose/dev.yml restart keycloak
117+
docker compose --env-file .env -f infra/compose/dev.yml up -d keycloak
115118
```
116119

117120
## What does NOT live here

‎scripts/seed.sh‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -116,21 +116,27 @@ while true; do
116116
# never got created fails the gate instead of passing by omission.
117117
declared=$(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" config --services | wc -l | tr -d ' ')
118118
observed=$(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" ps -a --format '{{.Service}}' | wc -l | tr -d ' ')
119-
if [[ -z "$not_healthy" && "$observed" -eq "$declared" ]]; then
119+
# Compare the SETS, not just the counts. One service missing plus one
120+
# orphan present gives observed == declared, and a count-only test would
121+
# break out of the loop reporting success on a stack that is missing a
122+
# declared service.
123+
missing=$(comm -13 \
124+
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" ps -a --format '{{.Service}}' | sort -u) \
125+
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" config --services | sort -u) \
126+
| xargs)
127+
extra=$(comm -23 \
128+
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" ps -a --format '{{.Service}}' | sort -u) \
129+
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" config --services | sort -u) \
130+
| xargs)
131+
132+
if [[ -z "$not_healthy" && -z "$missing" && -z "$extra" ]]; then
120133
break
121134
fi
122-
if [[ "$observed" -ne "$declared" ]]; then
135+
136+
if [[ -n "$missing" || -n "$extra" ]]; then
123137
if [[ -n "$not_healthy" ]]; then
124138
not_healthy="${not_healthy}"$'\n'
125139
fi
126-
missing=$(comm -13 \
127-
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" ps -a --format '{{.Service}}' | sort -u) \
128-
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" config --services | sort -u) \
129-
| xargs)
130-
extra=$(comm -23 \
131-
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" ps -a --format '{{.Service}}' | sort -u) \
132-
<(docker compose "${ENV_FILE_ARGS[@]+"${ENV_FILE_ARGS[@]}"}" -f "$COMPOSE_FILE" config --services | sort -u) \
133-
| xargs)
134140
detail="saw $observed of $declared declared services"
135141
if [[ -n "$missing" ]]; then
136142
detail="$detail; never created: $missing"

0 commit comments

Comments
 (0)