mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 05:41:14 +00:00
[AZ-494] [AZ-492] Cycle 3 Step 16: full test suite green; close batches
Final cumulative review for batches 04-05 (PASS_WITH_WARNINGS, 4 Low findings, all non-blocking). Combined with the prior 01-03 cumulative, this closes the per-cycle batch coverage with two PASS_WITH_WARNINGS verdicts. scripts/run-tests.sh --full green: format check + 13 cycle-3 unit tests (including the 4 new AZ-494 fail-fast cases for missing / empty iss / aud) + the full integration suite (including the 2 new WrongIssuer / WrongAudience 401 assertions). Fixed a stale "leave blank to fall back" comment in .env.example that contradicted the "REQUIRED" line right above it; the integration runner reads env vars directly with no appsettings fallback so blank values now fail-fast. Advanced _docs/_autodev_state.md to mark Step 10 (Implement) status: completed. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
+10
-7
@@ -22,11 +22,14 @@ JWT_SECRET=
|
||||
#
|
||||
# Production values MUST be confirmed by the admin team before deploy
|
||||
# (the admin API stamps the `iss` claim; satellite-provider validates
|
||||
# the `aud` claim). For local dev these can use the DEV-ONLY values
|
||||
# baked into appsettings.Development.json — leave blank to fall back.
|
||||
# the `aud` claim).
|
||||
#
|
||||
# Example values (NEVER use these in prod):
|
||||
# JWT_ISSUER=DEV-ONLY-iss-admin-azaion-local
|
||||
# JWT_AUDIENCE=DEV-ONLY-aud-satellite-provider
|
||||
JWT_ISSUER=
|
||||
JWT_AUDIENCE=
|
||||
# For local dev / CI: use the DEV-ONLY values below. The integration
|
||||
# test runner and scripts/run-tests.sh read these directly from the
|
||||
# environment (no appsettings fallback on the test side), so leaving
|
||||
# them blank will cause run-tests.sh to refuse to start.
|
||||
#
|
||||
# NEVER ship these DEV-ONLY values to prod — they exist only to make
|
||||
# local-dev mints validate against appsettings.Development.json:
|
||||
JWT_ISSUER=DEV-ONLY-iss-admin-azaion-local
|
||||
JWT_AUDIENCE=DEV-ONLY-aud-satellite-provider
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
# Cumulative Code Review — Batches 04–05 cycle 3
|
||||
|
||||
**Batch range**: 04-05 (cycle 3)
|
||||
**Cycle**: 3
|
||||
**Date**: 2026-05-12
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
**Trigger**: Implement skill Step 14.5 (final cumulative — closes cycle 3 batch coverage; first cumulative was for 01-03)
|
||||
|
||||
## Scope
|
||||
|
||||
| Batch | Tasks | Surfaces touched |
|
||||
|-------|-------|------------------|
|
||||
| 04 | AZ-492 | `SatelliteProvider.IntegrationTests/{PerfBootstrap.cs (added), Program.cs}`, `scripts/run-performance-tests.sh`, `_docs/02_document/{tests/performance-tests.md, tests/traceability-matrix.md, modules/tests_integration.md, module-layout.md}`, `_docs/06_metrics/retro_2026-05-11_cycle2.md`, `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md` (deleted) |
|
||||
| 05 | AZ-494 | `SatelliteProvider.Api/Authentication/AuthenticationServiceCollectionExtensions.cs`, `SatelliteProvider.TestSupport/JwtTokenFactory.cs`, `SatelliteProvider.IntegrationTests/{JwtTestHelpers,JwtIntegrationTests,UavUploadTests,PerfBootstrap,Program}.cs`, `SatelliteProvider.Tests/Authentication/AuthenticationServiceCollectionExtensionsTests.cs`, `SatelliteProvider.Api/appsettings*.json`, `.env.example`, `docker-compose{,.tests}.yml`, `scripts/run-{tests,performance-tests}.sh`, `_docs/02_document/{architecture, modules/{api_program,tests_integration,tests_unit}, tests/traceability-matrix}.md`, `_docs/03_implementation/deploy_cycle2.md`, `_docs/05_security/{security_report,owasp_review}.md`, `_docs/02_tasks/_dependencies_table.md` |
|
||||
|
||||
## Phase-by-Phase Summary (cumulative)
|
||||
|
||||
### Phase 1: Context Loading
|
||||
|
||||
The two batches share the theme **"operationalise what cycle-2 deferred"**. Both extend AZ-487 + AZ-491 / AZ-488 surfaces from cycle 2 rather than introduce new domains. AZ-492 makes the deferred PT-07 / PT-08 scenarios runnable; AZ-494 flips F-AUTH-2 from Medium-open to Resolved. The single point of overlap is `PerfBootstrap` + the perf harness's mint path — AZ-492 added them, AZ-494 had to thread the new iss/aud parameters through.
|
||||
|
||||
### Phase 2: Spec Compliance
|
||||
|
||||
- AZ-492: 6 / 6 ACs addressed at code level; AC-2 + AC-3 require live runtime measurement, gated at Step 16.
|
||||
- AZ-494: 6 / 7 ACs addressed; AC-7 (cross-repo suite contract write) explicitly deferred — outside this workspace's boundary. Documented in `deploy_cycle2.md` R3 follow-up.
|
||||
|
||||
No new spec-vs-reality findings emerged beyond what was already recorded in batches 01-03.
|
||||
|
||||
### Phase 3: Code Quality (cumulative)
|
||||
|
||||
- `JwtTokenFactory.Create` remains the single `new JwtSecurityToken(...)` site. AZ-494 added optional iss/aud parameters at the END of the signature (defaults `null`), so the cycle-2 hygiene wins survive.
|
||||
- `JwtTestHelpers` (runner-side, env-reading) and `JwtTokenFactory` (pure, side-effect-free) maintain the separation introduced by AZ-491. `MintAuthenticated` / `MintExpired` are 1-2-line wrappers that don't reintroduce duplicate mint logic.
|
||||
- The "resolve required value from env or config, throw fail-fast" pattern was extracted into `AuthenticationServiceCollectionExtensions.ResolveRequiredOrThrow`. `ResolveSecretOrThrow` stays separate because it has the ≥ 32-byte invariant from AZ-487. Two functions, two distinct invariants — acceptable.
|
||||
- `PerfBootstrap` lives in `IntegrationTests`, not `TestSupport`, intentionally — it depends on `SixLabors.ImageSharp`, which TestSupport must not pull in. Documented in `module-layout.md` per the AZ-492 review.
|
||||
- No verbose logging. No silent error suppression. No new comments that narrate code. No new annotations beyond what already existed.
|
||||
|
||||
### Phase 4: Security Quick-Scan (cumulative)
|
||||
|
||||
- AZ-494 closes F-AUTH-2 (Medium, cycle 2). `ValidateIssuer = true` + `ValidateAudience = true` are now wired against env-sourced values with fail-fast startup. Token reuse across satellite-suite services that share `JWT_SECRET` is now blocked.
|
||||
- AZ-492 raised no security findings of its own; the perf harness uses the same JWT mint surface as the integration runner.
|
||||
- One new Low finding noted in `owasp_review.md` A07: no token revocation list exists. That's a known residual gap, deliberately not in this cycle's scope.
|
||||
- DEV-ONLY placeholder values (`appsettings.Development.json`, `.env.example`) use the `DEV-ONLY-` prefix so a single grep surfaces every site that must be replaced in a real prod rollout.
|
||||
|
||||
### Phase 5: Test Coverage (cumulative)
|
||||
|
||||
- 4 new unit tests + 2 new integration scenarios from AZ-494 — all on a path that's runtime-gated at Step 16.
|
||||
- AZ-492's PT-07 + PT-08 will execute against a live API + Postgres at perf-test time. No new XUnit cases (perf harness is a bash script).
|
||||
- Existing integration suite migrated to `MintAuthenticated` / `MintExpired` — no test-fixture drift left for AZ-494's iss/aud validation to break.
|
||||
|
||||
### Phase 6: Cross-Task Consistency (cumulative)
|
||||
|
||||
- AZ-492 + AZ-494 both depend on AZ-491's `JwtTokenFactory` consolidation. Neither reintroduced a parallel mint path. The "single mint surface" invariant from batch 02 survives both batches.
|
||||
- AZ-492's `PerfBootstrap.MintToken` was updated in lock-step with AZ-494's contract change. Without this update the perf harness would have started emitting 401s. Caught in-batch.
|
||||
- The `.env` / `JWT_*` env-var convention is now uniform across `JWT_SECRET`, `JWT_ISSUER`, `JWT_AUDIENCE` — same fail-fast contract, same resolution precedence, same docs location, same compose plumbing. No drift between `scripts/run-tests.sh` and `scripts/run-performance-tests.sh`.
|
||||
|
||||
## Cumulative findings
|
||||
|
||||
### Carried over (still open, NOT regressions)
|
||||
|
||||
- L (AZ-492 batch 4): per-item gate cost reported as a derived proxy (`batch_p95 / batch_size`). True per-call timing needs server-side instrumentation — Low, documented as a future PBI.
|
||||
- L (AZ-492 batch 4): duplicate JPEG-factory pattern across `UavUploadTests.CreateValidJpeg` + `PerfBootstrap.GenerateUavFixture` + the upstream gen-uav-fixture utility — Low, image-fixture consolidation tracked as a future PBI.
|
||||
|
||||
### New (this cumulative)
|
||||
|
||||
- L (AZ-494 batch 5): cross-repo `suite/_docs/10_auth.md` write deferred (AC-7) — Low; tracked in `deploy_cycle2.md` R3.
|
||||
- L (AZ-494 batch 5): no token revocation list (residual after iss/aud validation) — Low; tracked in `owasp_review.md` A07 as a separate follow-up.
|
||||
|
||||
### Acknowledged operational gate (NOT a finding)
|
||||
|
||||
Option B for AZ-494 deliberately leaves `appsettings.json`'s `Jwt.Issuer` / `Jwt.Audience` empty so production deploy without `JWT_ISSUER` + `JWT_AUDIENCE` env vars fails at startup. This is the user-selected forcing function — by design, not a defect.
|
||||
|
||||
## Verdict
|
||||
|
||||
**PASS_WITH_WARNINGS**.
|
||||
|
||||
All 4 Low findings are non-blocking. The acknowledged operational gate is intentional. Cycle 3 batch implementation is complete; ready for Step 16 (full test suite gate) before promotion to Step 17 (Update Docs).
|
||||
@@ -4,11 +4,11 @@
|
||||
flow: existing-code
|
||||
step: 10
|
||||
name: Implement
|
||||
status: not_started
|
||||
status: completed
|
||||
sub_step:
|
||||
phase: 0
|
||||
name: awaiting-invocation
|
||||
detail: ""
|
||||
phase: 16
|
||||
name: full-test-suite-passed
|
||||
detail: "Step 16 PASS: ./scripts/run-tests.sh --full green. 5 batches landed (AZ-495+AZ-496, AZ-491, AZ-493, AZ-492, AZ-494). 2 cumulative reviews: 01-03 + 04-05 both PASS_WITH_WARNINGS. AZ-494 AC-1/2 wrong-iss/wrong-aud assertions both PASS in integration log line 650/653. 4 new fail-fast unit tests PASS. Cycle 3 implementation complete."
|
||||
retry_count: 0
|
||||
cycle: 3
|
||||
tracker: jira
|
||||
|
||||
Reference in New Issue
Block a user