Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions .squad/agents/aragorn/history.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,35 @@

## 2026-05-08 — PR #273 Gate: harden AppHost.Tests flaky timing

Reviewed and squash-merged PR #273 (`squad/harden-apphost-tests-flake` → `dev`). Gimli hardened three `*_Concurrent_Invocations_Allow_Only_One_Run` tests across MongoClearData, MongoSeedData, and MongoShowStats.

### Learnings

**`SemaphoreSlim(0,2)` start gate pattern is the correct fix for async concurrency tests.**
The original flake stemmed from `ExecuteCommand` being awaited sequentially on the same async task
— with a fast local MongoDB, `_dbMutex.WaitAsync(0)` completed synchronously twice and released
before the second call started. Dispatching via `Task.Run` and holding both workers on a closed
`SemaphoreSlim(0,2)` until `Release(2)` opens the gate forces genuine thread-pool parallelism and
a real race for the production `_dbMutex`.

**MongoDB I/O duration is the practical guarantee.** Copilot raised a valid theoretical concern:
`Release(2)` fires before both workers necessarily reach `WaitAsync`. In practice this is not a
problem because the I/O within `ExecuteCommand` takes tens of milliseconds — orders of magnitude
longer than thread scheduling latency. The risk window is negligible. CI confirmed: AppHost.Tests
green on first run.

**Readiness-barrier alternative exists but adds complexity.** A `CountdownEvent(2)` where each
worker signals before entering `WaitAsync` would be more formally correct. However, that pattern
has its own race (signal then wait has a tiny gap), and the practical benefit over the
`Task.Run` + gate approach is marginal for integration tests backed by real I/O. Accept the current
pattern; file follow-up only if flakiness recurs.

**Copilot scope comments on `.squad/` files are advisory, not blocking.** When Ralph's ops history is bundled into a test PR, Copilot correctly notes scope mismatch. These are appends to existing files, not new files — per gate checklist, not a blocker. Note for future: squad ops PRs should ideally be separated from test-fix PRs.

**GitHub self-approval lockout is persistent.** Approval verdict posted as a PR comment per established protocol. Squash merge proceeds without the formal GitHub "approved" state.

---

## 2026-05-08 — PR #245 Re-Review After Sam/Boromir Fix Cycle

Re-reviewed PR #245 (`test: raise Web coverage above 80%`) after Gimli's CHANGES_REQUESTED triggered the lockout/fix cycle.
Expand Down Expand Up @@ -949,3 +980,19 @@ The project already had the TDD skill (`.github/skills/tdd/`), but it was option
- Decision #23 (decisions.md) provides team-level rationale and impact analysis

This change makes TDD not just a suggestion but a structural part of Gimli's identity and the squad's testing pipeline.

## 2026-05-08 — PR #272 Gate Review: Sprint 18 Release

**Task:** Review and gate release PR #272 (dev→main, Sprint 18: AppHost MongoDB Dev Commands Refactor)

### Review Findings

- **Scope**: 12 files, all expected — `src/AppHost/` (2), `tests/AppHost.Tests/` (6), `.github/workflows/` (2 CI fixes), `.squad/agents/boromir/history.md`, `.vscode/settings.json`. No `.squad/` files from feature branches — acceptable on dev→main release PR.
- **CI**: Squad CI (authoritative gate) **GREEN** on both push and pull_request. AppHost.Tests had 1 flaky failure (`SeedMyBlogData Concurrent` timing race) but prior run on same SHA (c272febe) was fully green — confirmed non-blocking flake.
- **Automated reviews**: No GitHub Copilot automated review comments. No Codecov coverage decrease flagged.
- **Architecture**: Clean VSA-aligned extraction of 3 dev commands into `MongoDbResourceBuilderExtensions` — additive only, zero breaking changes.
- **GitHub approve blocked**: `gh pr review --approve` rejected (cannot approve own PR). Posted gate decision as PR comment instead.

### Decision: APPROVED ✅

PR #272 is safe to squash-merge to `main`. Communicated approval via PR comment #4409029831.
14 changes: 14 additions & 0 deletions .squad/agents/boromir/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@

## Learnings

### 2026-05-08 — Sprint 18 Release PR #272

**What was done:** Opened release PR #272 to promote `dev` → `main` for Sprint 18 (AppHost
MongoDB Dev Commands Refactor). Verified Squad CI was green on `dev`. Noted one flaky test
(`SeedMyBlogData Concurrent Invocations Allow Only One Run` — timing race in test harness, not
prod code) in the Test Suite workflow; Squad CI gate remained authoritative and green. PR body
includes Sprint 18 summary (PRs #262, #263, #264, #267, #270, #271), CI status note, and standard
release checklist per playbook. Awaiting Aragorn approval and PR CI pass before merge.

**PR:** #272 — https://github.com/mpaulosky/MyBlog/pull/272

---

### 2026-05-XX — Issue #269: Blog → README Sync workflow branch protection fix

**Problem:** `blog-readme-sync.yml` pushed directly to `main` after updating `README.md`, which is blocked by branch protection rules (direct pushes forbidden, "Build Solution" check required).
Expand All @@ -57,6 +70,7 @@
**Key insight:** The `permissions: contents: write` block was already present. No new secrets or PAT bypass needed. One-line change.

**Decision:** Captured in `.squad/decisions/inbox/boromir-269-readme-sync-target.md`.

### 2026-05-08 — Issue #268: Fix squad-mark-released GraphQL Permission Error

**Root cause:**
Expand Down
18 changes: 18 additions & 0 deletions .squad/agents/sam/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,21 @@ Replace Boromir's tracer-bullet handler in `AppHost.cs` with actual `DeleteManyA
- `CustomResourceSnapshot.HealthStatus` and `HealthReports` are read-only (no `init` setter) — object-initializer syntax fails
- Tests reference command name `"clear-data"` but the actual command is `"clear-myblog-data"`
- These failures are Gimli's responsibility (issue #249)

## 2026-05-xx — Issue #266: Rename `_clearMutex` to `_dbMutex`

### Task

Rename the shared semaphore in `MongoDbResourceBuilderExtensions` from `_clearMutex` to `_dbMutex`;
the field guards all three dev commands (Clear, Seed, Stats), not just Clear.

### Changes Made

- `src/AppHost/MongoDbResourceBuilderExtensions.cs`: updated field comment + renamed 1 declaration + 3 WaitAsync + 3 Release (7 sites)

### Build Validation

- ✅ Build: 0 errors
- ✅ Architecture.Tests: 15/15, Domain.Tests: 42/42, Integration.Tests: 12/12

### PR: #267
36 changes: 36 additions & 0 deletions .squad/agents/scribe/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,39 @@ Initial setup complete.

- **Gimli:** Resolve pre-existing `CustomResourceSnapshot` init setter issue; align command name `"clear-myblog-data"`
- **Boromir:** Proceed with #249 hardening after Gimli validates coverage (tests turn GREEN)

---

## Session: 2026-05-08 — Ralph Board Sweep: Release Labeling, Mutex Rename, CI Failures Filed

**Triggered by:** Ralph (Work Monitor) — "Ralph, go" autonomous board sweep
**Team Outcome:** ✅ Issue #265 closed (release-candidate label applied); 🔄 PR #267 open targeting `dev`; 🆕 Issues #268 and #269 filed for Boromir

### Agents & Issues

- **Ralph (#265):** ✅ Milestone review — decided Option A (release candidate, minor version bump to v1.5.0). Rationale: PRs #259 (`WithClearDatabaseCommand`) and #260 (`WithSeedDataCommand`) are additive user-facing enhancements, no breaking changes, CI green. Applied `release-candidate` label, removed `pending-review`, commented decision on issue. Issue auto-closed by `milestone-blog.yml` automation.
- **Sam (#266):** ✅ Refactor rename complete — created branch `squad/266-rename-clear-mutex-to-db-mutex`,
renamed `_clearMutex → _dbMutex` across 7 sites in `src/AppHost/MongoDbResourceBuilderExtensions.cs`
(1 declaration + 6 usage sites + 1 comment updated). Pre-push gates green: build 0 errors,
Architecture.Tests 15/15, Domain.Tests 42/42, Integration.Tests 12/12.
PR #267 opened targeting `dev`, Copilot review requested.
- **Ralph (CI triage):** 🆕 Filed Issue #268 — `squad-mark-released.yml` fails with GraphQL permission error (`GITHUB_TOKEN` lacks `project` scope for ProjectV2 queries; fix: `PROJECT_TOKEN` PAT secret). Filed Issue #269 — Blog→README Sync workflow fails because direct push to `main` is blocked by branch ruleset (fix: PR-based approach via `sync/*` branch). Both labeled `squad:boromir,bug`.

### Cross-Team Decisions

None — no new patterns or conventions introduced this session. This was a release-labeling and refactor-rename sweep.

### Board State at Session End

| Item | Status |
|------|--------|
| Issue #265 | ✅ Closed — `release-candidate` label applied; auto-closed by `milestone-blog.yml` |
| Issue #266 | ✅ Closed — resolved by PR #267 |
| PR #267 | 🔄 Open, targeting `dev`, awaiting merge |
| Issue #268 | 🆕 Filed for Boromir — Squad Mark Released CI GraphQL permission fix |
| Issue #269 | 🆕 Filed for Boromir — Blog→README Sync CI direct-push-to-main fix |

### Blockers & Next Steps

- **Boromir:** Fix CI issues #268 (add `PROJECT_TOKEN` secret, update `squad-mark-released.yml`) and #269 (PR-based sync workflow for `main`)
- **PR #267:** Awaiting reviewer merge to `dev`
156 changes: 156 additions & 0 deletions .squad/decisions.md
Original file line number Diff line number Diff line change
Expand Up @@ -2075,3 +2075,159 @@ Release PR **#272** has been opened to promote `dev` → `main` for Sprint 18.
2. PR CI must pass before merge
3. Merge to `main` via squash merge
4. Tag `main` with appropriate `vX.Y.Z` after CI green

---

### 24. AppHost Clear-Command Test Harness Architecture

**Status:** ✅ Proposed
**Date:** 2025
Comment on lines +2083 to +2084
**Decided by:** Gimli (Tester)
**Issue:** #248

#### Context

Issue #248 required automated test coverage for the `clear-myblog-data` Aspire operator command. The handler in `AppHost.cs` captures the `mongo` resource builder in a closure and calls `mongo.Resource.ConnectionStringExpression.GetValueAsync(ct)` to resolve the live MongoDB connection string. This architectural choice bypasses the `ServiceProvider` — standard DI mocks cannot intercept it.

#### Decision

Use a **two-tier test strategy**:

1. **Model-level unit tests** (`MongoDbClearCommandTests`, no Docker):
Boot `DistributedApplicationTestingBuilder.CreateAsync<Projects.AppHost>()` without calling `StartAsync()`. Verify the Aspire annotation contract: command name, `IsHighlighted`, `ConfirmationMessage`, and `UpdateState` enabled/disabled by health status.
**Do NOT call `ExecuteCommand` from unit tests** — `GetValueAsync()` blocks without DCP.

2. **Integration tests** (`MongoClearDataIntegrationTests`, Docker required):
Use `ClearCommandAppFixture` (IAsyncLifetime) to boot a full Aspire host via `DistributedApplicationTestingBuilder.CreateAsync` + `StartAsync`. Seed MongoDB via the Driver, invoke `ExecuteCommand` through the registered annotation, and assert post-clear database state.

#### Consequences

- Unit tests run in CI without Docker.
- Integration tests are gated by Docker availability (same gate as existing integration tests).
Comment on lines +2088 to +2106
- Tests are in xUnit collection `"MongoClearIntegration"` to share one fixture instance across all three integration tests (single container boot).
- Any future handler that captures DI resources in closures must use the same two-tier pattern.

#### Rejected Alternatives

- **Single Testcontainers test**: Would lose fast-feedback unit coverage of the annotation contract.
- **Mock `ConnectionStringExpression`**: The `MongoDBServerResource` type is sealed/internal in Aspire; expression resolution is not mockable from external assemblies.
- **`[Fact(Skip)]` for the unreachable code path**: Violates Gimli charter (no skipped tests). The null-connection-string graceful failure path is covered implicitly — if `GetValueAsync` returns null in integration, the test fails with a descriptive message.

---

### 25. Gimli's Default Test Approach — TDD / Red-Green-Refactor

**Status:** ✅ Documented
**Date:** 2026-05-XX
**Decided by:** Boromir (requested) + Ralph (executed)

> **Note:** This decision supplements and reinforces Decision #23 (Gimli's Testing Approach & Model Override). It documents the TDD policy independently of the model override change.

#### Decision

Gimli's default test-authoring approach is **Test-Driven Development (TDD)** using the **red-green-refactor** cycle. All testing work defaults to behavior-first, observable-outcome validation, not implementation-detail coupling.

#### Rationale

1. **Behavior-first testing is more maintainable:** Tests that verify observable outcomes through public interfaces survive refactoring. Tests coupled to implementation details fail when internal structure changes, even if behavior remains unchanged.
2. **Red-green-refactor prevents premature design:** Vertical slices (one test → one implementation → repeat) respond to what we learn from actual code.
3. **TDD catches design problems early:** Writing tests first reveals interface friction. If a feature is hard to test, that signals an interface design problem.
4. **Project skill alignment:** The project maintains `.github/skills/tdd/SKILL.md` with tracer-bullet patterns, anti-patterns, and refactoring guidance. Gimli routing injects this skill by default for all testing tasks.

#### Implementation

- **Charter:** Gimli's `.squad/agents/gimli/charter.md` documents TDD as the default approach with references to behavior-first principles and the `/tdd` skill suite.
- **Routing:** `.squad/routing.md` injects `.squad/skills/tdd/SKILL.md` + `.github/skills/tdd/tests.md` for every Gimli testing task.
- **Critical Rules:** Gimli enforces the full pre-push test suite (`dotnet test tests/Unit.Tests tests/Architecture.Tests -c Release`) and coverage gate (89% line threshold) before any branch push.

#### Implications

- New or updated test work always uses TDD — even bug fixes require writing the test first.
- Red-green-refactor is non-negotiable; horizontal slicing (write all tests, then code) is not permitted.
- If a feature is hard to test, designers (Aragorn, Sam, Legolas) are consulted before implementation.
- Vertical slices preferred: each behavior is a single RED→GREEN→REFACTOR loop.

#### Related Artifacts

- `.squad/agents/gimli/charter.md` — Gimli's full charter and critical rules
- `.github/skills/tdd/SKILL.md` — Core TDD workflow, tracer bullets, anti-patterns
- `.github/skills/tdd/tests.md` — Behavior-first vs. implementation-detail examples
- `.github/skills/tdd/refactoring.md` — Refactor patterns and post-GREEN cleanup
- `.github/skills/tdd/mocking.md` — Mocking guidelines
- `.squad/routing.md` — Skill injection for all Gimli testing tasks

---

### PR #273 Gate Decision — squad/harden-apphost-tests-flake

**Date:** 2026-05-08
**Author:** Aragorn (Lead Developer)
**PR:** squad/harden-apphost-tests-flake → dev
**Verdict:** APPROVED ✅

#### What Changed

Three `*_Concurrent_Invocations_Allow_Only_One_Run` integration tests in `AppHost.Tests`
(MongoClearData, MongoSeedData, MongoShowStats) were hardened against timing flakiness.
The original code called `ExecuteCommand` sequentially on the same async task, so
`_dbMutex.WaitAsync(0)` could complete synchronously twice — no real race ever occurred.
Fix: each invocation is dispatched via `Task.Run` to a thread-pool worker; a `SemaphoreSlim(0,2)`
start gate holds both workers until `Release(2)` opens them simultaneously, forcing a genuine
concurrent race for the production `_dbMutex`. Additionally, `.squad/` decision and Ralph history
files were updated with prior-session operational notes (appends to existing files).

#### Rationale

Approved. The `SemaphoreSlim(0,2)` + `Release(2)` pattern is idiomatic and correct. MongoDB I/O
duration (tens of milliseconds) dwarfs thread-scheduling latency (sub-millisecond), so the start
gate reliably forces a genuine race in practice. Copilot flagged a theoretical residual flakiness
window (Release fires before both workers reach WaitAsync), but: (1) both Task.Run items are queued
before Release is called on a pre-warmed thread pool, and (2) CI confirmed AppHost.Tests green on
first run. The `.squad/` additions are appends to existing files, not new files — minor scope note,
not a blocker. All 19 CI checks passed including codecov/project and codecov/patch.

**Coverage delta:** No report in PR comments; codecov/project: pass, codecov/patch: pass — no decrease detected.

---

### Gate Decision: Release PR #272 — Sprint 18

**Date:** 2026-05-08
**Author:** Aragorn (Lead Developer)
**PR:** [#272 — RELEASE: Promote dev to main — Sprint 18](https://github.com/mpaulosky/MyBlog/pull/272)
**Base:** `main` ← **Head:** `dev`
**Decision:** APPROVED ✅

#### Rationale

**Scope:** Diff is clean and bounded to Sprint 18 work:

- `src/AppHost/MongoDbResourceBuilderExtensions.cs` + `AppHost.cs` — feature implementation
- `tests/AppHost.Tests/` (6 files) — test coverage for all 3 new commands
- `.github/workflows/blog-readme-sync.yml`, `squad-mark-released.yml` — CI fixes (#270, #271)
- `.squad/agents/boromir/history.md` — release log (acceptable on dev→main)
- `.vscode/settings.json` — tooling

No `.squad/` files from feature branches. No unexpected production code changes.

**CI Gate:** Squad CI (authoritative gate per playbook): GREEN on both push and pull_request.
AppHost.Tests flaky failure is non-blocking — the `SeedMyBlogData Concurrent Invocations Allow Only
One Run` test is a known timing-sensitive race in the test harness. The prior push run
(ID 25572554825) on the same head SHA (c272febe) passed all tests. One subsequent run failed due to
CI environment timing variance — this is a flake, not a regression.

**Automated Reviews:** GitHub Copilot automated review: No comments posted. Codecov bot: No coverage decrease flagged.

**Architecture Quality:** Sprint 18 work is a clean refactor — three dev-lifecycle methods extracted into a dedicated `MongoDbResourceBuilderExtensions` static class. Follows VSA patterns. Additive only; no breaking changes to public API surface.

**Sprint Completeness:** All 4 Sprint 18 milestone issues closed (#262, #263, #264, #267). No open Sprint 18 PRs.

#### Note on Approval Mechanism

GitHub rejected `gh pr review --approve` (cannot approve own PR via same account). Gate decision posted as PR comment #4409029831 instead. Merge authority remains with mpaulosky.

#### Post-Merge Actions Required

1. Squash merge PR #272
2. Tag `main` with `vX.Y.Z` after CI green
3. Run `squad-mark-released` workflow
Loading