mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 07:01:15 +00:00
[AZ-505] Cycle 6 Step 17: retrospective + close cycle
Single-task cycle delivering AZ-505 (3 SP); 1 batch, PASS verdict after a single auto-fix round (ComputeLocationHash duplication consolidated into Uuidv5.LocationHashForTile). Step 14 Security Audit skipped; Step 15 Performance Test PASS (8/8, exit 0) and closes the cycle-3 perf-harness leftover that carried across cycles 3-5. Top 3 lessons appended to LESSONS.md ring buffer: - Kestrel Http1AndHttp2 requires TLS for ALPN; spec-time decision - Dapper-bypassing test paths own the Npgsql type contract - Test fixtures naming specific schema artifacts need migration awareness Cycle 7 opens at Step 9 (New Task). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,204 @@
|
|||||||
|
# Retrospective — Cycle 6 (2026-05-12)
|
||||||
|
|
||||||
|
**Tasks**: AZ-505 — Tile inventory endpoint (`POST /api/satellite/tiles/inventory`) + HTTP/2 enablement on the dev listener + Leaflet covering index (`tiles_leaflet_path`), 3 SP. Single-task cycle; ships the consumer-facing half of the AZ-503 epic that was deferred from cycle 5.
|
||||||
|
**Mode**: cycle-end (autodev Step 17)
|
||||||
|
**Previous retro**: `retro_2026-05-12_cycle5.md`
|
||||||
|
**Cycle shape**: small follow-up cycle — first cycle to deliver an HTTP/2-enabled endpoint to consumers, first cycle to ship two contract artifacts simultaneously (new `tile-inventory.md` + joint-freeze `tile-storage.md` major bump), first cycle since cycle 1 to close a cross-cycle leftover (cycle-3 perf-harness leftover, carried across cycles 3 → 4 → 5).
|
||||||
|
|
||||||
|
## 1. Implementation Metrics
|
||||||
|
|
||||||
|
| Metric | Cycle 6 | Δ vs cycle 5 |
|
||||||
|
|--------|---------|--------------|
|
||||||
|
| Tasks implemented | **1** (AZ-505) | -1 |
|
||||||
|
| Batches executed | **1** | -1 |
|
||||||
|
| Avg tasks / batch | 1.0 | unchanged |
|
||||||
|
| Total complexity delivered | **3 SP** | -1 SP |
|
||||||
|
| Avg complexity / batch | 3 SP | +1 SP |
|
||||||
|
| Tasks at-or-below 5 SP cap | **1 of 1 (100%)** | unchanged |
|
||||||
|
| Tasks split mid-cycle | **0** | -1 (cycle 5 split AZ-503 → AZ-503-foundation + AZ-505; cycle 6 was the planned consumption of that split — no further split needed) |
|
||||||
|
| Tasks above cap | 0 | unchanged |
|
||||||
|
| Cumulative reviews | **0** (trigger is every 3 batches; cycle 6 has 1 batch) | unchanged |
|
||||||
|
| Cross-cycle leftovers carried in | **1** (cycle-3 perf-harness leftover, replays #1-#5 across cycles 3-5) | unchanged at cycle entry |
|
||||||
|
| Cross-cycle leftovers carried out | **0** (cycle-3 leftover CLOSED by clean exit-0 perf run) | -1 |
|
||||||
|
|
||||||
|
**Sequencing**: single batch — AZ-505 landed clean in batch 01 (1 auto-fix round on a Medium / Maintainability finding: `ComputeLocationHash` duplication consolidated into `Uuidv5.LocationHashForTile`). The cycle completed in **7 dev commits**: Step 9 new-task chore + refine, Step 10 batch 01, autodev-state chore, Step 11 AC-5 TLS fix, Steps 12-13 test-spec + docs sync, and Steps 15-16 perf + deploy report. One more commit than cycle 5's 6-commit count, driven entirely by the post-merge AC-5 TLS pivot (see §5 below) — which was a single-commit post-merge correction, not a re-implementation, so the additional cycle cost was small.
|
||||||
|
|
||||||
|
## 2. Quality Metrics
|
||||||
|
|
||||||
|
### Code Review Results
|
||||||
|
|
||||||
|
| Verdict | Count | Percentage |
|
||||||
|
|---------|-------|-----------|
|
||||||
|
| PASS | **1** (batch 01) | **100%** |
|
||||||
|
| PASS_WITH_WARNINGS | 0 | 0% |
|
||||||
|
| FAIL | 0 | 0% |
|
||||||
|
|
||||||
|
### Findings by Severity (per-batch code review)
|
||||||
|
|
||||||
|
| Severity | Cycle 6 | Δ vs cycle 5 |
|
||||||
|
|----------|---------|--------------|
|
||||||
|
| Critical | 0 | unchanged |
|
||||||
|
| High | 0 | unchanged |
|
||||||
|
| Medium | **1 auto-fixed in-review** (`ComputeLocationHash` duplication) | +1 |
|
||||||
|
| Low | **0** | -1 (cycle 5 had 1 Low — `contentSha256` soft-NULL guard) |
|
||||||
|
| Remaining post-review | **0** | unchanged |
|
||||||
|
|
||||||
|
### Findings by Category
|
||||||
|
|
||||||
|
| Category | Count | Top Files |
|
||||||
|
|----------|-------|-----------|
|
||||||
|
| Maintainability | **1 auto-fixed** | `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` + `SatelliteProvider.Services.TileDownloader/TileService.cs` (consolidated into `SatelliteProvider.Common/Utils/Uuidv5.cs`) |
|
||||||
|
| Bug | 0 | — |
|
||||||
|
| Spec-Gap | 0 | — |
|
||||||
|
| Security | 0 (Step 14 skipped this cycle) | — |
|
||||||
|
| Performance | 0 | — |
|
||||||
|
| Style | 0 | — |
|
||||||
|
| Scope | 0 | unchanged |
|
||||||
|
|
||||||
|
**Note on the Medium auto-fix**: the `ComputeLocationHash` duplication was caused by AZ-505's first-pass implementation introducing a 3-line helper in two places at once (repository + service) rather than reaching for the existing `Uuidv5` utility module. This is the same cross-call-site duplication pattern flagged by AZ-491 retro lesson L-002. The review auto-fix landed the canonical `Uuidv5.LocationHashForTile` helper in `Common.Utils` (already the cross-repo identity owner) and updated all 4 call sites + 6 test sites; no production behaviour change. Pattern is unchanged from previous cycles — the lesson is *known* but the pattern still occurred. See §5 for the proposed reinforcement.
|
||||||
|
|
||||||
|
### Security audit (cycle 6)
|
||||||
|
|
||||||
|
| Metric | Value | Δ vs cycle 5 |
|
||||||
|
|--------|-------|--------------|
|
||||||
|
| Verdict | **SKIPPED** (user skipped optional gate) | new |
|
||||||
|
| Reason | No new dependencies, no schema-level security-sensitive change, dev-only TLS cert (gitignored, never promoted past dev), all SQL parameterised, new endpoint `.RequireAuthorization()` | — |
|
||||||
|
| New Critical / High | n/a (no audit) | — |
|
||||||
|
| Carry-overs (still OPEN, unchanged from cycle 5) | 3 (Microsoft.NET.Test.Sdk 17.8.0 transitive flag; Microsoft.IdentityModel.Tokens / System.IdentityModel.Tokens.Jwt 7.0.3 NU1902; Serilog.AspNetCore 8.0.3 fallback) | unchanged |
|
||||||
|
|
||||||
|
**Justification for skip**: cycle 6's change set is narrow and audit-light: a new GET-like POST endpoint (parameterised SQL, `.RequireAuthorization()`), a covering index (no privilege change, no extension change), a TLS dev affordance (cert gitignored, dev-only, never promoted), and contract artifacts. The cycle-5 audit baseline still applies — no new dependency adds, no new privileges granted, no new attack surface beyond what AZ-503-foundation already exposed. The cycle-5 carry-overs are unchanged and re-listed in `deploy_cycle6.md` for visibility. Recommendation: a full audit at cycle 7 if any cross-cutting auth, schema, or dependency change lands.
|
||||||
|
|
||||||
|
### Performance gate (cycle 6)
|
||||||
|
|
||||||
|
| Metric | Value |
|
||||||
|
|--------|-------|
|
||||||
|
| Verdict | **PASS** |
|
||||||
|
| Scenarios | **8 Pass · 0 Warn · 0 Fail · 0 Unverified** (single default-parameter run; no manual re-run needed) |
|
||||||
|
| Script exit code | **0** (first ever clean exit-0 for the perf harness — closes the cycle-3 leftover) |
|
||||||
|
| AZ-505 NFR-1 (inventory p95 ≤ 200ms at coords≤500) | **MET inline** — `TileInventoryTests.PerformanceBudget_AC4` against a seeded 1000-row table: median 5-8ms, p95 well under threshold. Not a standalone PT scenario; verified by the integration suite. |
|
||||||
|
| AZ-505 NFR-2 (HTTP/2 multiplexing, single TLS connection) | **MET inline** — `Http2MultiplexingTests`: 8 concurrent GETs over one HTTP/2 connection with `HttpVersion = 2.0` asserted on every response, cumulative wall time < 5s. |
|
||||||
|
| Existing PT-01..PT-08 regressions | None. PT-08 batch p95 = 544ms (vs 2000ms threshold; vs cycle-5 117ms). The increase is per-curl TLS handshake overhead on the host→api loopback measurement leg (the harness opens a fresh connection per `curl`); not an application-latency change. |
|
||||||
|
| Cycle-3 perf-harness leftover | **CLOSED** this cycle. The deletion criterion ("default-parameter `./scripts/run-performance-tests.sh` exits 0 against an api built from `dev`") is satisfied. File deleted in the cycle-6 perf+deploy commit. |
|
||||||
|
|
||||||
|
## 3. Trend Comparison (cycle 5 → cycle 6)
|
||||||
|
|
||||||
|
| Trend | Direction | Notes |
|
||||||
|
|-------|-----------|-------|
|
||||||
|
| Findings volume | **Stable** (1 Medium auto-fix; 0 Low; 0 carry into deploy) | cycle 5 had 0 Medium + 1 Low post-review; cycle 6 has 1 Medium auto-fixed + 0 Low. Net post-review delta: 0. |
|
||||||
|
| Code-review pass rate | **+50pp** (50% → 100%) | cycle 5's PASS/PASS_WITH_WARNINGS split was due to 1 Low not auto-fixed; cycle 6 went PASS clean. |
|
||||||
|
| Leftovers carried out | **-1** (1 → 0) | first cycle since cycle 1 to fully close a cross-cycle leftover. |
|
||||||
|
| Architectural cycles introduced | **0** (unchanged) | no new component edges; the new endpoint lives entirely within established layering (Api → Common/DataAccess/TileDownloader). |
|
||||||
|
| Contract artifacts produced | **+1** (1 → 2) | cycle 5 produced 1 minor bump (`uav-tile-upload.md` 1.0.0 → 1.1.0); cycle 6 produced 1 new contract (`tile-inventory.md` v1.0.0) + 1 major bump (`tile-storage.md` 1.0.0 → 2.0.0). |
|
||||||
|
| Migrations | **+1** (1 → 1) | cycle 5: migration 014 (additive columns + index swap). cycle 6: migration 015 (index-only — create covering + drop superseded). |
|
||||||
|
| Step 14 (Security Audit) outcome | **Skipped** (vs cycle 5 PASS_WITH_WARNINGS) | first skip since the gate was added; trade-off documented in §2 above. |
|
||||||
|
| Step 15 (Performance Test) script exit | **0** (vs cycle 5: 1) | first clean exit-0 since the perf harness was introduced; closes the cycle-3 leftover that motivated AZ-504. |
|
||||||
|
| Cross-cycle bug-introduction pattern | **0 new** | nothing newly broken this cycle. |
|
||||||
|
| Mid-cycle scope decisions (split / defer / re-spec) | **0** | cycle 5 split AZ-503 into AZ-503-foundation + AZ-505; cycle 6 consumed the planned half without further scope adjustment. |
|
||||||
|
| Post-merge correction commits | **+1** (0 → 1) | new this cycle: the AC-5 TLS pivot was discovered during Step 11 (Run Tests) and landed as a single follow-up commit. See §4. |
|
||||||
|
|
||||||
|
**Cumulative LESSONS reuse**: 3 lessons from previous cycles were directly applicable this cycle:
|
||||||
|
|
||||||
|
- **L-002 from AZ-491 (cross-call-site duplication)** — the `ComputeLocationHash` duplication is the same pattern; the lesson exists but the pattern recurred. Reinforcement proposed below.
|
||||||
|
- **2026-05-12 dependencies lesson on Major-version bumps** — confirmed valid; the deferred Serilog 10.x recheck stayed deferred because no 10.x line is published yet, exactly as the lesson predicts.
|
||||||
|
- **2026-05-12 process lesson on cross-PBI dependency capture via blocked-link** — cycle 6 consumed an AZ-503-foundation → AZ-505 blocked-link cleanly; the split + tracker-link pattern from cycle 5 worked end-to-end.
|
||||||
|
|
||||||
|
## 4. Patterns and Insights
|
||||||
|
|
||||||
|
### Pattern 1 — Kestrel "Http1AndHttp2 + plaintext = silent HTTP/1.1 only" surprise (new this cycle)
|
||||||
|
|
||||||
|
When AZ-505 first landed, Kestrel was configured `HttpProtocols.Http1AndHttp2` on a plaintext listener (`http://+:8080`). The expectation: HTTP/2 clients would get HTTP/2; HTTP/1.1 clients would get HTTP/1.1. The actual behaviour: Kestrel logged `HTTP/2 is not enabled for [::]:8080 ... TLS is not enabled` at INFO level and silently served only HTTP/1.1. ALPN — the protocol-negotiation mechanism `Http1AndHttp2` relies on — runs only over TLS. The test that catches this (`Http2MultiplexingTests`) failed with `HTTP_1_1_REQUIRED` (0xd) — a clear signal once observed, but the Kestrel log was at INFO (not WARN), so a casual reading of the API container logs did not surface it.
|
||||||
|
|
||||||
|
**Why this matters for future cycles**: any "enable HTTP/2" task that doesn't explicitly include TLS in its definition-of-done will produce the same silent downgrade. The Kestrel log line is information-only; the only reliable detector is an integration test that asserts `HttpVersion == 2.0` on a response — which AZ-505 had, but only as the *last* gate, after the implementation was already considered "done" by the spec.
|
||||||
|
|
||||||
|
**Insight**: the spec did say "ALPN negotiates h2 / http/1.1", but didn't say "therefore TLS is required". The pivot to TLS+ALPN with a self-signed dev cert + `update-ca-certificates` in the test container was the right resolution, but it cost one re-implementation pass on AZ-505 AC-5 (~1 dev commit, ~20 file touches across compose, scripts, tests, perf-script, docs). Catching this at spec-write time would have avoided the post-merge correction. Lesson L-005 captures the prevention.
|
||||||
|
|
||||||
|
### Pattern 2 — `Npgsql.DateTime.Kind=Utc` vs `timestamp without time zone` mismatch in test seeders (new instance)
|
||||||
|
|
||||||
|
Three test files (`TileInventoryTests` x2 seeding paths + `LeafletPathIndexOnlyTests` seed) all hit the same Npgsql 6+ error: `Cannot write DateTime with Kind=UTC to PostgreSQL type 'timestamp without time zone'`. Production paths go through Dapper, which transparently rewrites the kind; tests that bypass Dapper for control over the bind site (raw `NpgsqlCommand` for performance fixtures or array-typed parameter binding) must set `DateTime.Kind=Unspecified` explicitly.
|
||||||
|
|
||||||
|
**Insight**: the bug surfaces only when a new test path bypasses Dapper. AZ-505 introduced two such paths in the same PR (the integer-array `ANY($1::uuid[])` parameter binding, which Dapper can't express, plus the perf-fixture seeders that needed deterministic timestamps). The lesson is: **when a test bypasses Dapper to gain access to a feature Dapper doesn't expose, the test author owns the type-conversion contract that Dapper used to handle silently**. Lesson L-006 captures the rule.
|
||||||
|
|
||||||
|
### Pattern 3 — Drop-and-replace index migrations need test-fixture co-updates in the same PR
|
||||||
|
|
||||||
|
Cycle 5's `MigrationTests.Az503NewUniqueIndexCoversIntegerKeyAndFlightId` asserted that `idx_tiles_location_hash` exists after migration 014. Cycle 6's migration 015 *drops* that index and replaces it with `tiles_leaflet_path` (which has the same leading column + INCLUDE columns — semantically superior). The test asserted the old name; broadening the assertion to accept either index name kept the AZ-503 AC-9 intent ("a location_hash-keyed access path exists") verifiable.
|
||||||
|
|
||||||
|
**Insight**: tests that assert specific schema artifacts (index names, constraint names, column names) need cross-migration awareness — a migration that drops/renames any of them must update the assertion in the same PR. The MigrationTests assertion was deliberately narrow when written (named the exact AZ-503 index because that was the cycle-5 artifact). Cycle 6's migration 015 was foreseeable from cycle 5's split note; the assertion could have been written more abstractly (e.g., "any index whose first column is `location_hash`") but wasn't.
|
||||||
|
|
||||||
|
**Insight²**: the lesson is similar to the cross-call-site duplication pattern (L-002) but on the *test-fixture* axis rather than the source-code axis. Test fixtures that name specific implementation artifacts age the same way source code does: they require maintenance when the implementation evolves. Lesson L-007 captures the rule.
|
||||||
|
|
||||||
|
### Pattern 4 — Closing a cross-cycle leftover by reaching its trigger condition (positive pattern)
|
||||||
|
|
||||||
|
The cycle-3 perf-harness leftover carried replay-attempt entries across cycles 3 → 4 → 5, recording five distinct contexts in which the deletion criterion (`exit 0` on a default-parameter run) was attempted and didn't land. Cycle 6 didn't *target* the leftover; the trigger condition was reached as a side-effect of the AC-5 TLS work + the cycle 6 perf-script adaptation (`--cacert` plumbing). The leftover was deleted in the cycle-6 perf+deploy commit without any new PBI being opened for it.
|
||||||
|
|
||||||
|
**Insight**: the leftover system worked exactly as designed — the precondition for deletion was recorded, was checked at every replay, and was finally satisfied by an unrelated cycle's work. The `_docs/_process_leftovers/` folder is now empty as of end of cycle 6. This is the first time the project has been fully leftover-free since cycle 3 introduced the mechanism.
|
||||||
|
|
||||||
|
## 5. Top 3 Improvement Actions
|
||||||
|
|
||||||
|
### Action 1 — Reinforce L-002 (cross-call-site duplication detection) by adding it to the implement-skill review checklist
|
||||||
|
|
||||||
|
**Why**: the AZ-491 cross-call-site-duplication lesson is real and the pattern keeps recurring (cycle 2: JWT factories; cycle 5: `BuildTileEntity` `idName` + `locationHashName`; cycle 6: `ComputeLocationHash` x2). The pattern is **always** caught at code-review time, but **never** caught at implement time. The auto-fix cost is low (a single 1-round consolidation), but it's still 1 extra review round per occurrence. A pre-review check would save it.
|
||||||
|
|
||||||
|
**Action**: add an explicit step to `.cursor/skills/implement/SKILL.md` Phase "Self-verification before review": grep new source files for ≥2 occurrences of the same `Uuidv5.Create(...)` / `HashAlgorithm.HashData(...)` / DTO-construction helper, and consolidate before the review handoff. ~1 SP.
|
||||||
|
|
||||||
|
**Cost**: ~30 minutes of skill-author work. Counted as 1 SP because it touches the implement skill (cross-cutting tool).
|
||||||
|
|
||||||
|
### Action 2 — Add an HTTP/2 spec-writing checklist to plan / new-task skills
|
||||||
|
|
||||||
|
**Why**: pattern 1 above. The TLS+ALPN requirement for `Http1AndHttp2` is a Kestrel implementation detail that surprised AZ-505's spec author. Future tasks that mention "enable HTTP/2", "negotiate h2", or "multiplex over a single connection" will hit the same Kestrel surprise unless the spec explicitly resolves TLS-or-h2c at spec-write time.
|
||||||
|
|
||||||
|
**Action**: in `.cursor/skills/plan/steps/` and `.cursor/skills/new-task/SKILL.md`, when the task mentions HTTP/2 / h2 / multiplexing / ALPN, the skill must surface the TLS-vs-h2c choice as a spec-writing decision (with A/B/C):
|
||||||
|
|
||||||
|
- **A** — TLS+ALPN on the dev listener; dev cert plumbing; production terminates at ingress (current AZ-505 path).
|
||||||
|
- **B** — h2c on the dev listener; production via cleartext to ingress's HTTP/2 backend.
|
||||||
|
- **C** — Both; dev listener supports h2c for browser-less programmatic clients AND TLS+ALPN for browsers.
|
||||||
|
|
||||||
|
The chosen path goes into the task spec's "Implementation notes" before implementation starts. ~2 SP.
|
||||||
|
|
||||||
|
**Cost**: ~1 hour of skill-author work. Counted as 2 SP because it touches two skills (plan + new-task) and the wording needs to be precise (HTTP/2 is one of those topics where slightly-wrong precision is worse than vague).
|
||||||
|
|
||||||
|
### Action 3 — Add a `_docs/02_document/tests/` lint that flags test-fixture assertions naming specific schema artifacts
|
||||||
|
|
||||||
|
**Why**: pattern 3 above. The `MigrationTests.Az503NewUniqueIndexCoversIntegerKeyAndFlightId` assertion locked in the index name `idx_tiles_location_hash`; the next migration that drops/renames that index made the assertion stale in a way that didn't fail until the next test run. A doc-side lint that flags assertions matching `idx_<name>` / `pk_<name>` / `fk_<name>` / specific column-name strings as "fragile" would catch the same class of fixture-rot at test-spec review time.
|
||||||
|
|
||||||
|
**Action**: add a `_docs/02_document/tests/<test>.md` review step (in `.cursor/skills/test-spec/` cycle-update mode) that surfaces assertions referencing specific named DB artifacts; the review prompts the test author to consider whether the assertion is *about the existence of the artifact* or *about the capability the artifact provides* and to phrase the assertion at the abstraction level matching the intent. ~2 SP.
|
||||||
|
|
||||||
|
**Cost**: ~1 hour of skill-author work. Counted as 2 SP because it adds review prompts to the test-spec skill (cross-cutting). This is a process check, not a runtime lint — the goal is to catch the fragile pattern at the spec-writing stage, not at runtime.
|
||||||
|
|
||||||
|
## 6. Carry-overs (status this cycle)
|
||||||
|
|
||||||
|
| Item | Status | Notes |
|
||||||
|
|------|--------|-------|
|
||||||
|
| Cycle-3 perf-harness leftover | **CLOSED** this cycle | First cycle to satisfy the deletion criterion. Deleted in the cycle-6 perf+deploy commit. |
|
||||||
|
| Microsoft.NET.Test.Sdk 17.8.0 transitive `NuGet.Frameworks` NU1902 | OPEN (carried from cycles 4 + 5) | No cycle-6 touch. Re-listed in deploy_cycle6.md. |
|
||||||
|
| Microsoft.IdentityModel.Tokens / System.IdentityModel.Tokens.Jwt 7.0.3 NU1902 | OPEN (carried from cycles 3-5) | No cycle-6 touch. Re-listed in deploy_cycle6.md. |
|
||||||
|
| Serilog.AspNetCore 8.0.3 → 10.x recheck | OPEN (carried from cycles 4 + 5; no 10.x published) | No cycle-6 touch. Re-listed in deploy_cycle6.md. |
|
||||||
|
| ASPDEPR002 `WithOpenApi(...)` deprecation | OPEN (carried from cycles 4 + 5) | No cycle-6 touch. Re-listed in deploy_cycle6.md. |
|
||||||
|
| Admin team `iss/aud` confirmation (carried from cycle 3) | OPEN (still required before promoting beyond `dev`) | Re-listed in deploy_cycle6.md. |
|
||||||
|
| `metadata.flightId` authenticated provenance (F1-cy5) | OPEN (long-term, not actionable until flight registry exists) | Re-listed in deploy_cycle6.md. |
|
||||||
|
| `pgcrypto` ops gap (F2-cy5) | OPEN (doc-only fix, ~30 min) | Recommended follow-up PBI re-listed in deploy_cycle6.md. |
|
||||||
|
|
||||||
|
**New leftovers carried out of cycle 6**: **none**. First cycle since cycle 3 with zero new leftovers.
|
||||||
|
|
||||||
|
## 7. Suggested Rule / Skill Updates
|
||||||
|
|
||||||
|
| Target file | Change | Rationale |
|
||||||
|
|-------------|--------|-----------|
|
||||||
|
| `.cursor/skills/implement/SKILL.md` Phase "Self-verification before review" | Add: "Before handing off to the code-review skill, grep new source files for ≥2 occurrences of the same `Uuidv5.Create(...)` / `HashAlgorithm.HashData(...)` / cross-component DTO helper; if found, consolidate into the canonical home in `Common.Utils` before the review pass. Skip only if the duplicate sites are deliberately variant (e.g. different namespace, different normalization rule)." | Pattern 1 above. Cross-call-site duplication has occurred in cycles 2, 5, 6; each time it cost an auto-fix review round. Pre-empting it at implement-time is cheap. |
|
||||||
|
| `.cursor/skills/plan/steps/` + `.cursor/skills/new-task/SKILL.md` | Add: "When the task mentions HTTP/2 / h2 / multiplexing / ALPN / multiplex-over-single-connection, the spec MUST resolve the TLS-vs-h2c choice via A/B/C and capture the decision in the task spec's Implementation Notes BEFORE implementation begins. Default: TLS+ALPN with a dev-only self-signed cert; production terminates at ingress." | Pattern 1 above. The TLS-requirement-for-ALPN surprise cost ~1 dev commit's worth of post-merge correction this cycle. |
|
||||||
|
| `.cursor/skills/test-spec/` cycle-update mode | Add: "When updating an existing test scenario, flag any assertion that references a specific named DB artifact (index, constraint, FK name) and surface to the user whether the assertion is *about the existence of the named artifact* (fragile to migration renames) or *about the capability the artifact provides* (robust). Recommend phrasing at the capability abstraction level when possible." | Pattern 3 above. Cycle-6 MigrationTests assertion broke because cycle-5's wording was at the artifact-name level, not the capability level. |
|
||||||
|
|
||||||
|
## 8. Validations and Sources
|
||||||
|
|
||||||
|
- **All cycle-6 implementation artifacts parsed**: 1 batch report (`batch_01_cycle6_report.md`), 1 review file (`reviews/batch_01_cycle6_review.md`), 1 implementation report (`implementation_report_tile_inventory_cycle6.md`), 1 completeness gate report (`implementation_completeness_cycle6_report.md`), 1 deploy report (`deploy_cycle6.md`), 1 perf report (`perf_2026-05-12_cycle6.md`).
|
||||||
|
- **Cycle-5 retro compared explicitly**: see §3 trend table.
|
||||||
|
- **Cross-cycle dependency tracking**: AZ-503-foundation → AZ-505 blocked-link captured in the cycle-5 task spec + delivered in cycle 6; the dependency-link mechanism worked end-to-end.
|
||||||
|
- **No skill-level escalations encountered**: no `retry_count: 3` failures in any sub-skill; no FAIL verdicts in any review or gate; no scope-discipline escalations.
|
||||||
|
|
||||||
|
## 9. Self-Verification
|
||||||
|
|
||||||
|
- [x] All cycle 6 implementation artifacts parsed (1 batch report, 1 review, 1 completeness gate report, 1 implementation report, 1 deploy report, 1 perf report).
|
||||||
|
- [x] Comparison with cycle-5 retro performed (§3 trend table).
|
||||||
|
- [x] Top 3 improvement actions concrete and actionable (§5).
|
||||||
|
- [x] Suggested rule/skill updates specific and tied to a target file (§7).
|
||||||
|
- [x] Cycle-3 perf-harness leftover deletion is recorded (§6 + §4 pattern 4).
|
||||||
|
- [x] LESSONS.md ring buffer to be appended with top 3 cycle-6 lessons (§4 patterns 1-3) — applied in next step.
|
||||||
+6
-6
@@ -37,6 +37,12 @@ If the enum's wire string happens to match a member name case-insensitively (e.g
|
|||||||
|
|
||||||
## Ring buffer (last 15 entries — newest at top)
|
## Ring buffer (last 15 entries — newest at top)
|
||||||
|
|
||||||
|
- [2026-05-12] [tooling] Kestrel `HttpProtocols.Http1AndHttp2` silently serves only HTTP/1.1 over a plaintext listener — ALPN requires TLS, so any "enable HTTP/2" task without TLS in its definition-of-done will downgrade transparently and the only log line is at INFO; tasks that mention HTTP/2 / h2 / multiplexing / ALPN MUST resolve the TLS-vs-h2c choice at spec-write time and the test gate MUST assert `HttpVersion == 2.0` not just a 200 (cycle 6: AZ-505 AC-5 first landed on h2c plaintext, required a post-merge TLS+ALPN pivot with dev-cert plumbing across compose/tests/perf/docs).
|
||||||
|
Source: _docs/06_metrics/retro_2026-05-12_cycle6.md
|
||||||
|
- [2026-05-12] [testing] When a test bypasses Dapper to gain access to a feature Dapper doesn't expose (e.g. `ANY($1::uuid[])` array params, raw `NpgsqlCommand` for performance fixtures), the test owns the Npgsql type-conversion contract that Dapper used to handle silently — `DateTime.Kind=Utc` must be converted to `Unspecified` before binding into a `timestamp without time zone` column (cycle 6: AZ-505 introduced two Dapper-bypassing paths and all three new test files hit the same `Cannot write DateTime with Kind=UTC` error until `DateTime.SpecifyKind(..., Unspecified)` was added at the bind sites).
|
||||||
|
Source: _docs/06_metrics/retro_2026-05-12_cycle6.md
|
||||||
|
- [2026-05-12] [testing] Tests that assert specific schema artifact names (`idx_<name>` / `pk_<name>` / `fk_<name>`) need cross-migration awareness — phrase assertions at the capability abstraction level ("any index whose first column is `location_hash`") rather than the artifact-name level when possible, otherwise drop/rename migrations require fixture co-updates in the same PR (cycle 6: `MigrationTests.Az503NewUniqueIndexCoversIntegerKeyAndFlightId` hardcoded `idx_tiles_location_hash` from migration 014; migration 015 dropped it, broke the assertion until broadened to accept either index name).
|
||||||
|
Source: _docs/06_metrics/retro_2026-05-12_cycle6.md
|
||||||
- [2026-05-12] [architecture] Cross-repo cryptographic invariants (UUID namespaces, deterministic-key formulas, base32/64 alphabets, tile-zoom conventions) MUST live as code-level constants in BOTH repos with reference-vector tests on BOTH sides — documentation alone is insufficient because constant drift surfaces only as 100% lookup misses in production, harder to detect than a unit-test failure (cycle 5: AZ-503 introduced `TileNamespace = 5b8d0c2e-7f1a-4d3b-9c5e-1f3a8e7d2b6c` which must byte-match the same constant in `gps-denied-onboard/components/c6_tile_cache/_uuid.py`; the satellite-provider side has the constant + 10 Python-generated reference vectors in `Uuidv5Tests.cs` and the sibling repo will mirror).
|
- [2026-05-12] [architecture] Cross-repo cryptographic invariants (UUID namespaces, deterministic-key formulas, base32/64 alphabets, tile-zoom conventions) MUST live as code-level constants in BOTH repos with reference-vector tests on BOTH sides — documentation alone is insufficient because constant drift surfaces only as 100% lookup misses in production, harder to detect than a unit-test failure (cycle 5: AZ-503 introduced `TileNamespace = 5b8d0c2e-7f1a-4d3b-9c5e-1f3a8e7d2b6c` which must byte-match the same constant in `gps-denied-onboard/components/c6_tile_cache/_uuid.py`; the satellite-provider side has the constant + 10 Python-generated reference vectors in `Uuidv5Tests.cs` and the sibling repo will mirror).
|
||||||
Source: _docs/06_metrics/retro_2026-05-12_cycle5.md
|
Source: _docs/06_metrics/retro_2026-05-12_cycle5.md
|
||||||
- [2026-05-12] [tooling] Local Docker/colima DNS cold-start is a recurring class of failure that contaminates the Step-15 perf gate — when the perf-mode "one re-run" rule fires twice across consecutive cycles with the same root-cause class (DNS / NTP / resolver), the harness must escalate from "re-run" to a deterministic fix at the harness layer (DNS pre-warm in script, OR move gate to CI), not just another re-run (cycle 5: PT-01 failed Run #1 on `tile.googleapis.com` cold-start, then Run #2 on `mt0.google.com` cold-start; the warmup probe between runs only touched the hostnames it explicitly named).
|
- [2026-05-12] [tooling] Local Docker/colima DNS cold-start is a recurring class of failure that contaminates the Step-15 perf gate — when the perf-mode "one re-run" rule fires twice across consecutive cycles with the same root-cause class (DNS / NTP / resolver), the harness must escalate from "re-run" to a deterministic fix at the harness layer (DNS pre-warm in script, OR move gate to CI), not just another re-run (cycle 5: PT-01 failed Run #1 on `tile.googleapis.com` cold-start, then Run #2 on `mt0.google.com` cold-start; the warmup probe between runs only touched the hostnames it explicitly named).
|
||||||
@@ -61,9 +67,3 @@ If the enum's wire string happens to match a member name case-insensitively (e.g
|
|||||||
Source: _docs/06_metrics/retro_2026-05-11_cycle2.md
|
Source: _docs/06_metrics/retro_2026-05-11_cycle2.md
|
||||||
- [2026-05-11] [testing] Integration tests must explicitly reset DB state at startup — relying on wallclock seeds or "tests probably won't collide" is a workaround, not isolation; the persistent Postgres volume in docker-compose makes test data accumulation the default state (cycle 2: `UavUploadTests._coordinateCounter` collision was patched with a wallclock seed instead of a real DB-reset hook).
|
- [2026-05-11] [testing] Integration tests must explicitly reset DB state at startup — relying on wallclock seeds or "tests probably won't collide" is a workaround, not isolation; the persistent Postgres volume in docker-compose makes test data accumulation the default state (cycle 2: `UavUploadTests._coordinateCounter` collision was patched with a wallclock seed instead of a real DB-reset hook).
|
||||||
Source: _docs/06_metrics/retro_2026-05-11_cycle2.md
|
Source: _docs/06_metrics/retro_2026-05-11_cycle2.md
|
||||||
- [2026-05-11] [testing] Persisted enums need a Dapper read-roundtrip integration test — unit-testing the type handler in isolation does not prove read-side behavior (see L-001).
|
|
||||||
Source: _docs/06_metrics/retro_2026-05-11.md
|
|
||||||
- [2026-05-11] [process] NFR test-spec additions must include the runner-script implementation in the same step, or be tagged "Deferred — harness work tracked in <ticket>"; otherwise scenarios accumulate as Unverified across cycles.
|
|
||||||
Source: _docs/06_metrics/retro_2026-05-11.md
|
|
||||||
- [2026-05-11] [estimation] Task-spec test-site-count estimates must be backed by an explicit grep evidence block, not pattern-matched against neighboring code (AZ-484 spec said ~3 sites in `RegionServiceTests`; actual = 0).
|
|
||||||
Source: _docs/06_metrics/retro_2026-05-11.md
|
|
||||||
|
|||||||
@@ -2,14 +2,14 @@
|
|||||||
|
|
||||||
## Current Step
|
## Current Step
|
||||||
flow: existing-code
|
flow: existing-code
|
||||||
step: 17
|
step: 9
|
||||||
name: Retrospective
|
name: New Task
|
||||||
status: not_started
|
status: not_started
|
||||||
sub_step:
|
sub_step:
|
||||||
phase: 0
|
phase: 0
|
||||||
name: awaiting-invocation
|
name: awaiting-invocation
|
||||||
detail: ""
|
detail: ""
|
||||||
retry_count: 0
|
retry_count: 0
|
||||||
cycle: 6
|
cycle: 7
|
||||||
tracker: jira
|
tracker: jira
|
||||||
auto_push: true
|
auto_push: true
|
||||||
|
|||||||
Reference in New Issue
Block a user