mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 10:51:15 +00:00
Oleksandr Bezdieniezhnykh
e31f59211d
Adds retro_2026-05-12_cycle4.md, structure_2026-05-12_cycle4.md, and the deploy_cycle4.md report that was dropped from the Steps 12-15 sync commit. Appends 3 new lessons to LESSONS.md (12/15 ring buffer) on transitive major-version bumps, exposed pre-existing bugs, and single-task-cycle metric framing. State advances to cycle 5 / step 9 (awaiting next New Task invocation). Co-authored-by: Cursor <cursoragent@cursor.com>
64 lines
8.2 KiB
Markdown
64 lines
8.2 KiB
Markdown
# Engineering Lessons
|
|
|
|
Recurring bugs, surprising library behaviors, and process insights extracted from completed cycles. Newest at the top. Keep entries short — this is for fast scanning at the start of new cycles, not exhaustive history.
|
|
|
|
This file has two layers:
|
|
|
|
- **Deep engineering lessons** (L-NNN): library bugs, architectural insights, multi-paragraph context. Persist forever.
|
|
- **Ring buffer** at the bottom: most recent 15 single-sentence lessons emitted by the retrospective skill, consumed by `new-task` / `plan` / `decompose` / `autodev` Step 0. Oldest entries drop off the top.
|
|
|
|
Categories: `estimation` · `architecture` · `testing` · `dependencies` · `tooling` · `process`
|
|
|
|
---
|
|
|
|
## L-001 — Dapper `TypeHandler<T>` is bypassed for enum types during read deserialization
|
|
|
|
**Cycle**: 1 (AZ-484)
|
|
**Discovered by**: integration test failure (`Error parsing column 12 (source=google_maps - String)`); root-caused via web search to long-standing Dapper issue [#259](https://github.com/DapperLib/Dapper/issues/259).
|
|
**Affects**: Dapper 2.1.35 (and most other versions until the proposed `Settings.PreferTypeHandlersForEnums` opt-in in PR #2200, not yet merged).
|
|
|
|
**What happens**
|
|
Registering `SqlMapper.AddTypeHandler(new MyEnumHandler())` for an `enum` type — even via `SqlMapper.TypeHandler<TEnum>` — works for **writes** (the handler's `SetValue` is invoked for parameter binding) but is **silently bypassed for reads**. Dapper's IL-emitted deserializer checks `IsEnum` first and falls back to `Enum.TryParse(string, ignoreCase: true)`.
|
|
|
|
**Why this is dangerous**
|
|
If the enum's wire string happens to match a member name case-insensitively (e.g., `RegionStatus.Failed` ↔ `"failed"`), the bypass goes unnoticed and round-trip works *accidentally*. The bug only surfaces when the wire format diverges from the C# member name (e.g., `TileSource.GoogleMaps` ↔ `"google_maps"` — `Enum.TryParse("google_maps")` does not match `GoogleMaps` because of the underscore).
|
|
|
|
**Recommended approach**
|
|
- Do **not** rely on `SqlMapper.TypeHandler<TEnum>` for read-side enum mapping unless the wire values match the enum member names case-insensitively.
|
|
- For enums whose wire format diverges (snake_case, kebab-case, custom IDs), store the entity field as `string` and provide an explicit converter (`*Converter.ToWireValue` / `FromWireValue`) for use at the service-layer boundary. This is what AZ-484 does for `TileEntity.Source` ↔ `TileSourceConverter`.
|
|
- Unit-test the converter directly. Do not assume that round-tripping through Dapper proves anything for enums.
|
|
|
|
**Detection**
|
|
- Unit tests of the type handler in isolation will pass even when the handler is bypassed at runtime.
|
|
- Failure surfaces only at integration-test time when the actual SELECT runs.
|
|
- If you must keep an enum-typed field, write at minimum one integration test that reads the enum back through Dapper from a real database row.
|
|
|
|
---
|
|
|
|
## Ring buffer (last 15 entries — newest at top)
|
|
|
|
- [2026-05-12] [dependencies] Major-version bumps of direct deps cascade through transitives; the task spec must list the transitive packages whose major version changes as a result OR explicitly note "transitive major-version drift not analyzed in spec" — verify with `dotnet restore --dry-run` against a scratch branch before writing the spec (cycle 4: AZ-500 surprise-bumped `Microsoft.OpenApi` 1.x → 2.x via the `Microsoft.AspNetCore.OpenApi` 8.0.25 → 10.0.7 path; forced an unscheduled Swashbuckle bump + Program.cs refactor mid-implementation).
|
|
Source: _docs/06_metrics/retro_2026-05-12_cycle4.md
|
|
- [2026-05-12] [process] When a scope-protected task newly *exposes* a pre-existing bug elsewhere in the codebase (vs. introducing a new one), surface it as a recommended follow-up PBI in the batch report AND list it as a "newly exposed bug" separate from "newly introduced findings" in the deploy report — bugs that already existed don't count as cycle-introduced regressions, but they must not be silently re-buried (cycle 4: AZ-500's bootstrap fix unmasked the pre-existing `scripts/run-performance-tests.sh:417` `grep -o | wc -l` + `pipefail` bug).
|
|
Source: _docs/06_metrics/retro_2026-05-12_cycle4.md
|
|
- [2026-05-12] [process] When a cycle has a single non-functional task (migration / refactor / dependency hygiene), the retro must reframe the metric set around continuity (0 regressions), forward-resolution (prior findings closed by the bump itself), and unblocking (capabilities now exercisable end-to-end) — task count + complexity points read as misleading flatlines that look like under-productivity (cycle 4: AZ-500 alone delivered 5 SP vs cycle 3's 18 SP, but the cycle's value was forward-resolving 2 cycle-3 advisories and finally executing PT-01..PT-08 end-to-end against the migrated build).
|
|
Source: _docs/06_metrics/retro_2026-05-12_cycle4.md
|
|
- [2026-05-12] [process] For cross-team blockers (admin team must supply config values, etc.), prefer an Option-B forcing function (ship the validation/scaffolding with prod-empty config that fails-fast at deploy) over deferring the entire task — the fail-fast contract makes the cross-team conversation impossible to skip and ships the in-workspace work in the current cycle (cycle 3: AZ-494 shipped iss/aud validation with empty prod appsettings so deploy must supply real values).
|
|
Source: _docs/06_metrics/retro_2026-05-12_cycle3.md
|
|
- [2026-05-12] [process] ACs that prescribe a specific measurement or sentinel mechanism (e.g. "per-item latency < 50ms", "guard fires when DB name contains _test") should also prescribe — or explicitly defer — the path for collecting / enforcing it, or implementations will substitute proxies / equivalents that look like spec drift in review (cycle 3: AZ-492 PT-08 per-item gate cost became a derived proxy; AZ-493 DB-name guard became Host-allowlist).
|
|
Source: _docs/06_metrics/retro_2026-05-12_cycle3.md
|
|
- [2026-05-12] [process] ACs that require cross-repo writes should be tagged with the target workspace and rendered separately in the traceability matrix — mixing them with in-workspace ACs makes "correctly deferred" indistinguishable from "incomplete work" (cycle 3: AZ-494 AC-7 deferred for the suite-repo write; matrix renders as `◐ deferred` which is ambiguous).
|
|
Source: _docs/06_metrics/retro_2026-05-12_cycle3.md
|
|
- [2026-05-11] [testing] Test helpers shared across unit + integration projects must live in one consolidated location — duplicate near-identical copies will diverge and require parallel fixes (cycle 2: `JwtTokenFactory.cs` and `JwtTestHelpers.cs` had the same `Expires < NotBefore` bug fixed in separate commits).
|
|
Source: _docs/06_metrics/retro_2026-05-11_cycle2.md
|
|
- [2026-05-11] [process] Deferred-status NFR entries are allowed at most ONCE per NFR — if a Deferred NFR has not landed by the end of the cycle that follows the one in which it was deferred, the harness work must be promoted to a real PBI before any new NFR is accepted as Deferred (cycle 2 inherited cycle 1's PT-07 + added PT-08 + JWT-attach script-rot).
|
|
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).
|
|
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
|