From 834e72845461d138da273227238d7a5e31d8a296 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:22:10 -0400 Subject: [PATCH 1/6] docs(pr4): implementation plan for reader state-invariants + doc cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the post-merge review of PR3: - Fix the 2 state-inconsistency bugs in src/agent/status.ts (swap order; reset cachedEtag on HEAD-succeeds-GET-fails) - Add spec §4.3.1 documenting the partial-failure state invariants - Add docs/06-discord-webhook-setup.md (new reader got stuck on this) - Remove aws-cloud-agent / core-llm-wiki references from public docs and spec Co-Authored-By: Claude --- ...e-s3-agent-tutorial-pr4-reader-bugfixes.md | 897 ++++++++++++++++++ 1 file changed, 897 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md diff --git a/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md b/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md new file mode 100644 index 0000000..dcddc7f --- /dev/null +++ b/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md @@ -0,0 +1,897 @@ +# SQLite S3 Agent Tutorial — PR4: Reader State-Invariant Fixes + Doc Cleanup + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Depends on:** PR3 must be merged. This plan assumes `src/agent/status.ts`, `src/handler.ts`, the `status` reader path, and the tutorial docs (`docs/01-architecture.md` through `docs/05-from-tutorial-to-prod.md`) all exist and pass their tests. + +**Goal:** Fix two state-inconsistency bugs in the reader's cache-miss branch that the post-merge review surfaced, and clean up the public docs so the tutorial no longer references a private sibling repo or leaves Discord webhook setup as an unexplained exercise. The fix shape is the smallest one that preserves the existing retry-loop behavior on a corrupted or missing S3 object — the bug is the inconsistency, not the loop. + +**Architecture:** The two bugs share one root cause: the cache-miss branch in `src/agent/status.ts` updates `state.cachedEtag` and `state.db` in an order that allows a partial failure to leave them in an inconsistent combination. The invariant the cache must preserve is "either both fields are populated and the pair is valid, or both are null/undefined." A failing `openReadOnlyDatabase` should leave `cachedEtag` at its prior value (not the new one). The HEAD-succeeds-but-GET-fails branch should reset `cachedEtag` to `null` so the next call retries cleanly. The retry loop on a permanently broken S3 object is unchanged — it terminates when the operator or the writer fixes the underlying object. + +**Tech Stack:** No new dependencies. Reuses the existing test tooling (Vitest, `aws-sdk-client-mock`, `mkdtempSync` for hermetic `/tmp` directories). + +--- + +## File Structure (changes to PR3) + +``` +sqlite-s3-agent-tutorial/ +├── docs/ +│ ├── 01-architecture.md # MODIFY: remove aws-cloud-agent reference +│ ├── 02-rehydration.md # MODIFY: link to new Discord webhook doc +│ ├── 05-from-tutorial-to-prod.md # MODIFY: remove aws-cloud-agent references, generalize framing +│ ├── 06-discord-webhook-setup.md # NEW: step-by-step Discord webhook creation +│ ├── bedrock-model-comparison.md # MODIFY: remove aws-cloud-agent / core-llm-wiki references +│ └── superpowers/ +│ └── specs/ +│ └── 2026-08-08-sqlite-s3-agent-tutorial-design.md # MODIFY: add §4.3.1 +│ state-invariants +│ subsection; +│ update Status line; +│ remove ~14 aws-cloud-agent +│ references +├── src/ +│ ├── agent/ +│ │ └── status.ts # MODIFY: swap order; reset cachedEtag on GET-null +│ └── format/ +│ └── families.ts # MODIFY: remove aws-cloud-agent reference +└── tests/ + └── status.test.ts # MODIFY: add regression tests for failure modes +``` + +--- + +## Task 1: Fix Bug 1 — swap `state.db` and `state.cachedEtag` assignment order + +**Files:** +- Modify: `src/agent/status.ts` + +**Bug:** `src/agent/status.ts:119-120` assigns `state.cachedEtag = object.etag` *before* `state.db = openReadOnlyDatabase(dbPath)`. If `openReadOnlyDatabase` throws (corrupted snapshot, non-SQLite bytes, disk-full-mid-write), `state.cachedEtag` is left holding the new ETag while `state.db` stays `undefined`. This violates the invariant the spec describes in §4.3.1 (added in Task 4). + +**Fix:** Swap the order. Assign `state.db` first; only assign `state.cachedEtag` after `openReadOnlyDatabase` succeeds. On failure, `state.cachedEtag` stays at its prior value (or `null` on a cold start) and `state.db` stays `undefined`, which is the same state the reader was in before the call. + +- [ ] **Step 1: Modify `src/agent/status.ts`** + +In `src/agent/status.ts`, replace the three lines: + +```typescript + writeFileSync(dbPath, object.body); + state.cachedEtag = object.etag; + state.db = openReadOnlyDatabase(dbPath); +``` + +with: + +```typescript + writeFileSync(dbPath, object.body); + // Assign cachedEtag only after openReadOnlyDatabase succeeds — if open throws, + // cachedEtag stays at its prior value (or null) and the next call retries cleanly + // instead of leaving (cachedEtag=newEtag, db=undefined), an invalid state per + // spec §4.3.1. + state.db = openReadOnlyDatabase(dbPath); + state.cachedEtag = object.etag; +``` + +- [ ] **Step 2: Verify the type-check passes** + +Run: `npx tsc -p tsconfig.check.json` +Expected: exit 0, no errors. + +--- + +## Task 2: Fix Bug 2 — reset `state.cachedEtag` on HEAD-succeeds-GET-fails + +**Files:** +- Modify: `src/agent/status.ts` + +**Bug:** `src/agent/status.ts:110-116` handles the HEAD-succeeds-GET-returns-null branch (race between HEAD and GET, typically a transient S3 inconsistency). The current code returns the empty-state JSON but leaves `state.cachedEtag` at whatever value it held before — typically the prior valid ETag from a previous successful hydration. This violates the invariant `cachedEtag === null ⟺ db === undefined`. + +**Fix:** Reset `state.cachedEtag` to `null` before returning the empty-state JSON. The next call sees `cachedEtag === null`, evaluates cache miss, and re-runs the full cache-miss path. + +- [ ] **Step 1: Modify `src/agent/status.ts`** + +In `src/agent/status.ts`, replace: + +```typescript + const object = await store.get(storeKey); + if (object === null) { + // HEAD succeeded but GET raced a delete between the two calls — treat as + // no-snapshot rather than throwing, since the outcome the caller cares about + // (nothing to query) is identical to the head === null branch above. + return { snapshotVersion: null, sources: [], recentNotifications: [] }; + } +``` + +with: + +```typescript + const object = await store.get(storeKey); + if (object === null) { + // HEAD succeeded but GET raced a delete between the two calls — treat as + // no-snapshot rather than throwing, since the outcome the caller cares about + // (nothing to query) is identical to the head === null branch above. + // Reset cachedEtag so the next call retries cleanly rather than leaving + // (cachedEtag=oldEtag, db=undefined), an invalid state per spec §4.3.1. + state.cachedEtag = null; + return { snapshotVersion: null, sources: [], recentNotifications: [] }; + } +``` + +- [ ] **Step 2: Verify the type-check passes** + +Run: `npx tsc -p tsconfig.check.json` +Expected: exit 0, no errors. + +--- + +## Task 3: Add regression tests for the failure modes + +**Files:** +- Modify: `tests/status.test.ts` + +**A note on test design.** Both bugs are *state-consistency* bugs — the cache-miss branch leaves `(cachedEtag, db)` in an invalid combination, but the function's externally observable behavior is identical in the buggy and fixed versions. The cacheHit check fails in both cases (`state.db === undefined` is enough to force a re-download), so GET calls happen at the same cadence. + +The tests below verify that the failure modes propagate correctly (open failure throws, GET-null returns empty state) and that the function recovers cleanly when the underlying S3 object becomes valid. They serve as regression tests and documentation of the expected behavior, not as a demonstration of the bug. The invariant itself is enforced by the spec (Task 4) and code review, not by these tests. + +- [ ] **Step 1: Add the test for Bug 1 — open failure propagates and recovers** + +Append to `tests/status.test.ts`: + +```typescript + it('propagates openReadOnlyDatabase failures and recovers when the snapshot becomes valid', async () => { + // An open failure on a corrupted snapshot must: + // (a) propagate as a rejection (the Lambda returns 500), + // (b) not poison the reader — once the underlying S3 object is fixed, the next + // call downloads valid bytes and returns the seeded data. + let validBytes: Buffer | null = null; + let corrupt = true; + const adaptiveStore = { + ...ctx.store, + async get(key: string) { + if (corrupt) { + // Bytes that aren't a valid SQLite file — openReadOnlyDatabase will throw. + return { body: Buffer.from('not a sqlite file'), etag: 'corrupt-etag' }; + } + return { body: validBytes as Buffer, etag: 'fixed-etag' }; + }, + async head(key: string) { + return { etag: corrupt ? 'corrupt-etag' : 'fixed-etag' }; + }, + }; + + // Seed a valid snapshot so we have bytes to recover with. + const seeded = await seedSnapshot(ctx.dbPath, ctx.store); + validBytes = seeded.body; + + const readerDbPath = join(ctx.dir, 'reader-copy.db'); + const reader = createStatusReader(readerDbPath); + + // First call: open throws on the corrupted bytes. + await expect(reader.getStatus(adaptiveStore, 'memory.db')).rejects.toThrow(); + + // Snapshot becomes valid (operator or writer fixes the S3 object). + corrupt = false; + + // Second call: cache miss, downloads valid bytes, opens successfully, returns + // the seeded data. Without the fix, state would be (cachedEtag='corrupt-etag', + // db=undefined) — invalid per spec §4.3.1 — but the test would still pass because + // the cacheHit check fails for the same reason in both versions. The test documents + // the recovery behavior; the invariant is enforced by code review. + const recovered = await reader.getStatus(adaptiveStore, 'memory.db'); + expect(recovered.snapshotVersion).toBe('fixed-etag'); + expect(recovered.sources).toEqual([ + { name: 'weather', lastValue: '72F', lastFetchedAt: 1000, lastPostedAt: 1000 }, + ]); + }); +``` + +- [ ] **Step 2: Add the test for Bug 2 — GET-null returns empty state and recovers** + +Append to `tests/status.test.ts`: + +```typescript + it('returns empty state and recovers on the next call when HEAD succeeds but GET returns null', async () => { + // The HEAD-succeeds-GET-fails branch (object deleted between HEAD and GET) must: + // (a) return the empty-state JSON (treating it as no-snapshot, not throwing), + // (b) reset cachedEtag so the next call retries cleanly, + // (c) once the underlying object exists, return the seeded data without being + // stuck in a HEAD→GET→empty cycle. + let objectExists = false; + let validBytes: Buffer | null = null; + const adaptiveStore = { + ...ctx.store, + async get(key: string) { + if (!objectExists) return null; // HEAD-succeeds-GET-fails + return { body: validBytes as Buffer, etag: 'fixed-etag' }; + }, + async head(key: string) { + return { etag: 'fixed-etag' }; + }, + }; + + const seeded = await seedSnapshot(ctx.dbPath, ctx.store); + validBytes = seeded.body; + + const readerDbPath = join(ctx.dir, 'reader-copy.db'); + const reader = createStatusReader(readerDbPath); + + // First call: HEAD says the object exists, GET says it doesn't. Empty state. + const first = await reader.getStatus(adaptiveStore, 'memory.db'); + expect(first).toEqual({ snapshotVersion: null, sources: [], recentNotifications: [] }); + + // Object materializes (operator creates it out-of-band, or writer runs). + objectExists = true; + + // Second call: HEAD still says fixed-etag, GET returns valid bytes, returns data. + // The fix resets cachedEtag to null on the first call, so the second call sees + // (cachedEtag=null, db=undefined) — cache miss, fresh GET, success. + const recovered = await reader.getStatus(adaptiveStore, 'memory.db'); + expect(recovered.snapshotVersion).toBe('fixed-etag'); + expect(recovered.sources).toEqual([ + { name: 'weather', lastValue: '72F', lastFetchedAt: 1000, lastPostedAt: 1000 }, + ]); + }); +``` + +- [ ] **Step 3: Run the tests to verify they pass** + +Run: `npx vitest run tests/status.test.ts` +Expected: all tests pass — both the existing tests and the two new ones. + +If a test fails, the bug is more severe than the review identified. Report the failure with the full test output before proceeding. + +--- + +## Task 4: Update the spec to document the partial-failure state behavior + +**Files:** +- Modify: `docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md` + +The spec's §4.3 currently describes the cache-miss happy path and the `NoSuchKey` branch but does not document the partial-failure state behavior. Without this, the next implementation pass will make the same bug. + +- [ ] **Step 1: Add a new subsection §4.3.1 to the spec** + +In `docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md`, after §4.3 (the closing line "the tutorial teaches it explicitly."), insert a new subsection: + +```markdown +### 4.3.1 Partial-failure state invariants + +The reader's `ReaderState` has two fields — `cachedEtag` and `db` — and exactly two valid combinations: + +- `(cachedEtag: , db: )` — the cache holds a valid snapshot. +- `(cachedEtag: null, db: undefined)` — the cache is empty. + +Any other combination is a bug. The cache-miss branch must preserve this invariant across every failure mode: + +- **`openReadOnlyDatabase` throws** (corrupted snapshot, non-SQLite bytes, disk error): `state.db` stays `undefined`, `state.cachedEtag` stays at its prior value (or `null` on a cold start). The error propagates up — the Lambda returns 500, the next call retries the cache-miss path from scratch. +- **`GetObject` returns `null` after a successful `HEAD`** (transient S3 race, object deleted between calls): `state.cachedEtag` is reset to `null`, the empty-state JSON is returned. The next call retries cleanly. +- **`GetObject` returns `NoSuchKey`** (no snapshot yet — `fetch` has never run): identical to the `null` case above. + +The retry loop on a permanently broken S3 object is unavoidable — it terminates when the operator or the writer fixes the underlying object. The invariant is what prevents the loop from behaving incorrectly (e.g., returning stale data from a closed handle) while it runs. +``` + +- [ ] **Step 2: Update the "Status" line at the top of the spec** + +The spec's Status line may read either `**Status:** Implemented` (the merged-to-main +version after PR3) or `**Status:** Approved (ready for implementation planning)` (the +in-progress version, which is what the spec author sees in their IDE before merging +PR3). The search-and-replace below is robust to both: detect which variant is present +and replace it with the new value. + +**If the current line is `**Status:** Implemented`:** + +Replace: + +```markdown +**Status:** Implemented +``` + +with: + +```markdown +**Status:** Implemented (PR3) + state-invariants hardened (PR4) +``` + +**If the current line is `**Status:** Approved (ready for implementation planning)`:** + +Replace: + +```markdown +**Status:** Approved (ready for implementation planning) +``` + +with: + +```markdown +**Status:** Implemented (PR3) + state-invariants hardened (PR4) +``` + +**If neither variant is present**, leave the line alone and add a note to the PR +description explaining that the Status line was already in some other state. The spec +text below Status is unchanged either way. + +--- + +## Task 5: Remove `aws-cloud-agent` references from the spec file + +**Files:** +- Modify: `docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md` + +The spec references `aws-cloud-agent` and `core-llm-wiki` in roughly 14 places. These are framing references ("same toolchain family as", "same pattern as", "the sibling project") that leak implementation lineage which doesn't belong in a public tutorial. Each occurrence should be removed or generalized so the standalone constraint (line 17) is actually honored. + +This task is the spec-cleanup equivalent of Task 6, which does the same for the public docs. + +- [ ] **Step 1: Update "Standalone" and "TypeScript, Node 24, ESM" constraints** + +In `docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md`, replace the two adjacent bullet points (lines 17-18): + +```markdown +- **Standalone.** No coupling to `aws-cloud-agent`, `core-llm-wiki`, or any other sibling repo. Reusable as a starting point for any agent with persistent state. +- **TypeScript, Node 24, ESM.** Same toolchain family as `aws-cloud-agent`. +``` + +with: + +```markdown +- **Standalone.** Reusable as a starting point for any agent with persistent state. No coupling to any external repo. +- **TypeScript, Node 24, ESM.** Modern, ESM-native, strict. +``` + +- [ ] **Step 2: Replace the §4.3 closing sentence** + +Replace (line 125): + +```markdown +The version cache (§4.3) is what makes warm invocations cheap. `setup()` here is just opening a SQLite file, so the payoff is smaller than `aws-cloud-agent`'s MiniSearch rebuild — but the *mechanism* is the same, and the tutorial teaches it. +``` + +with: + +```markdown +The version cache (§4.3) is what makes warm invocations cheap. The mechanism is the same one any warm-cache pattern uses: hold a pointer to the last loaded resource, validate it against the source's current version on each access, and reload only when it has changed. The tutorial teaches that mechanism in its simplest form. +``` + +- [ ] **Step 3: Replace the "Why close-and-reopen" sentence** + +Replace (line 161): + +```markdown +**Why close-and-reopen rather than reuse the open handle?** `better-sqlite3` keeps a page cache in memory. If the file on disk changes underneath an open handle, the cache describes a file that no longer exists — silently wrong answers, no error. The mechanism is the same one `aws-cloud-agent` uses for the same reason. +``` + +with: + +```markdown +**Why close-and-reopen rather than reuse the open handle?** `better-sqlite3` keeps a page cache in memory. If the file on disk changes underneath an open handle, the cache describes a file that no longer exists — silently wrong answers, no error. Closing the handle before overwrite is what prevents that. +``` + +- [ ] **Step 4: Replace the §5 prefix note** + +Replace (line 169): + +```markdown +Three tables, prefixed `agent_` (matching `aws-cloud-agent`'s convention) so future migrations stay collision-free. +``` + +with: + +```markdown +Three tables, prefixed `agent_` so future migrations stay collision-free. +``` + +- [ ] **Step 5: Replace the `outcome`/`error` nullable rationale** + +Replace (line 210): + +```markdown +**`outcome` and `error` are nullable on purpose.** A run row inserted at step 4 of the writer lifecycle (§3.1) and never updated is itself a record: "this run started and never finished." That signal is lost if the columns default to a fake value. Same pattern as `aws-cloud-agent`'s `agent_runs`. +``` + +with: + +```markdown +**`outcome` and `error` are nullable on purpose.** A run row inserted at step 4 of the writer lifecycle (§3.1) and never updated is itself a record: "this run started and never finished." That signal is lost if the columns default to a fake value. +``` + +- [ ] **Step 6: Replace the §6 principle prose** + +Replace (line 238): + +```markdown +**The principle: every error category has exactly one right answer, and it's the same answer every time.** `aws-cloud-agent`'s design calls this out explicitly in its §6 prose; the tutorial does the same because it teaches a habit, not just a pattern. +``` + +with: + +```markdown +**The principle: every error category has exactly one right answer, and it's the same answer every time.** The tutorial teaches a habit, not just a pattern. +``` + +- [ ] **Step 7: Replace the §7 testing rationale** + +Replace (line 246): + +```markdown +Tests run against a **real SQLite file** with a real `better-sqlite3` handle — no mocks of the database. `aws-cloud-agent`'s design calls this out explicitly ("the library's actual behaviour is the thing under test") and the tutorial adopts the same principle. Network boundaries are mocked because Discord and external APIs are out of our control. +``` + +with: + +```markdown +Tests run against a **real SQLite file** with a real `better-sqlite3` handle — no mocks of the database. The library's actual behaviour is the thing under test, not a re-implementation of it. Network boundaries are mocked because Discord and external APIs are out of our control. +``` + +- [ ] **Step 8: Replace the §7.1 mocking-table entry** + +Replace (line 255): + +```markdown +| S3 client | Yes | `aws-sdk-client-mock` (matches `aws-cloud-agent`). | +``` + +with: + +```markdown +| S3 client | Yes | `aws-sdk-client-mock`. | +``` + +- [ ] **Step 9: Replace the §7.3 smoke-test rationale** + +Replace (line 273): + +```markdown +`scripts/smoke.sh` invokes the deployed `fetch` op, waits for the run, then invokes `status` and asserts the JSON contains a `weather` source with a `lastValue`. Matches `aws-cloud-agent`'s smoke pattern; tutorial readers can run it after deploy to verify end-to-end. +``` + +with: + +```markdown +`scripts/smoke.sh` invokes the deployed `fetch` op, waits for the run, then invokes `status` and asserts the JSON contains a `weather` source with a `lastValue`. Tutorial readers can run it after deploy to verify end-to-end. +``` + +- [ ] **Step 10: Replace the §8 out-of-scope entry** + +Replace (line 284): + +```markdown +- **A `social` retrieval profile, episodic tiers, ontology, outbox.** All `aws-cloud-agent`-specific concepts, all intentionally omitted. +``` + +with: + +```markdown +- **An ontology, semantic retrieval profiles, episodic memory tiers, outbox patterns.** All project-specific concepts that grew up around richer knowledge-graph workloads, intentionally omitted. +``` + +- [ ] **Step 11: Replace the §10 repo-layout entries** + +Replace (line 321): + +```markdown +│ └── 05-from-tutorial-to-prod.md # the deltas vs aws-cloud-agent +``` + +with: + +```markdown +│ └── 05-from-tutorial-to-prod.md # the deltas vs running this in production +``` + +And replace (line 360): + +```markdown +├── Dockerfile # arm64 cross-compile block (§8.5 of aws-cloud-agent) +``` + +with: + +```markdown +├── Dockerfile # arm64 cross-compile block +``` + +- [ ] **Step 12: Replace the §10 closing prose** + +Replace (line 367): + +```markdown +`docs/01-architecture.md` is the tutorial's *narrative*. The code is the working example; the docs explain why each piece exists. `docs/05-from-tutorial-to-prod.md` is a closing piece that points at `aws-cloud-agent` for readers who outgrow the tutorial — making the lineage explicit. +``` + +with: + +```markdown +`docs/01-architecture.md` is the tutorial's *narrative*. The code is the working example; the docs explain why each piece exists. `docs/05-from-tutorial-to-prod.md` is a closing piece that describes what changes when this tutorial's defaults are no longer the right trade-offs — a checklist for readers who outgrow it. +``` + +- [ ] **Step 13: Replace the §12.5 model-swap reference** + +Replace (line 457): + +```markdown +4. If the family is not `zai`, add a registry entry — *only after a live probe* of accepted prefixes and request shape. The probe procedure is documented in the sibling repo (`aws-cloud-agent/docs/superpowers/specs/2026-08-02-model-provider-adapter-design.md` §5). The four-step procedure is mandatory, including the negative control: some families accept unknown request fields silently, so "the request did not 400" is not evidence a field is supported. +``` + +with: + +```markdown +4. If the family is not `zai`, add a registry entry — *only after a live probe* of accepted prefixes and request shape. The four-step procedure is mandatory, including the negative control: some families accept unknown request fields silently, so "the request did not 400" is not evidence a field is supported. +``` + +- [ ] **Step 14: Verify no remaining `aws-cloud-agent` references in the spec** + +Run: `grep -n "aws-cloud-agent\|core-llm-wiki" docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md` +Expected: no output. + +--- + +## Task 6: Remove `aws-cloud-agent` references from public docs + +**Files:** +- Modify: `docs/01-architecture.md` +- Modify: `docs/05-from-tutorial-to-prod.md` +- Modify: `docs/bedrock-model-comparison.md` +- Modify: `src/format/families.ts` + +The tutorial is public; `aws-cloud-agent` is a private repo. References to it produce dead links and leak implementation lineage that doesn't belong in a tutorial. Wherever the docs say "this mirrors `aws-cloud-agent`'s pattern" or "the sibling project," rewrite to either drop the reference or frame the choice generically. + +The existing `*-plan*.md` files in `docs/superpowers/plans/` are historical implementation records and may be kept as-is — they describe the project's lineage at the time each PR was merged, and removing references would falsify the record. + +- [ ] **Step 1: Update `docs/01-architecture.md`** + +In the "Why one Lambda, not two" section, replace: + +```markdown +`aws-cloud-agent`, the sibling project this tutorial is drawn from, uses two Lambdas — a +writer and a reader — because its reader also runs semantic search backed by a vector +index that needs its own warm-container lifecycle tuning. This tutorial's reader is a +much smaller job: query two tables and return JSON. Splitting it into a second Lambda +would mean a second container image, a second set of IAM grants, and a second cold-start +budget — for a query that returns in single-digit milliseconds once hydrated. One function +with an `op` field is simpler and the tutorial's job is to teach the storage pattern, not +Lambda topology. +``` + +with: + +```markdown +A more ambitious agent might split the reader into its own Lambda — say, when the reader +also runs semantic search backed by a vector index that needs its own warm-container +lifecycle tuning. This tutorial's reader is a much smaller job: query two tables and +return JSON. Splitting it into a second Lambda would mean a second container image, a +second set of IAM grants, and a second cold-start budget — for a query that returns in +single-digit milliseconds once hydrated. One function with an `op` field is simpler and +the tutorial's job is to teach the storage pattern, not Lambda topology. +``` + +- [ ] **Step 2: Update `docs/05-from-tutorial-to-prod.md`** + +In the "What stays the same" section, replace: + +```markdown +The rehydration protocol — bootstrap, conditional writes, version-cached reads — doesn't +change shape as the system grows. That's the point of the pattern: it's the same mechanism +whether the payload is a two-table dedup cache or full knowledge graph +with a vector index. What changes is how much work happens between hydrate and publish, +not how hydrate and publish themselves work. +``` + +with: + +```markdown +The rehydration protocol — bootstrap, conditional writes, version-cached reads — doesn't +change shape as the system grows. That's the point of the pattern: it's the same mechanism +whether the payload is a two-table dedup cache or a larger state file with more tables and +indices. What changes is how much work happens between hydrate and publish, not how +hydrate and publish themselves work. +``` + +Also in the section "The single Lambda split," replace: + +```markdown +This tutorial's `fetch` and `status` share one function because the reader's query is +cheap. If your reader starts doing real work — search, aggregation, anything with its own +latency and memory profile — split it into its own function. The two functions still share the storage pattern in this +tutorial's `docs/02-rehydration.md`; only the deployment topology changes. +``` + +with: + +```markdown +This tutorial's `fetch` and `status` share one function because the reader's query is +cheap. If your reader starts doing real work — search, aggregation, anything with its own +latency and memory profile — split it into its own function. The two functions still share +the storage pattern in this tutorial's `docs/02-rehydration.md`; only the deployment +topology changes. +``` + +Also in the section "More than one writer path," replace: + +```markdown +This tutorial has exactly one thing that writes to the snapshot: the `fetch` op, on a +fixed daily schedule. A production agent is more likely to need multiple write paths — a +scheduled job and a manually-triggered one, say — which raises the question of whether +`reservedConcurrentExecutions: 1` is still sufficient once two *different* Lambda +functions might both want to write. It isn't, on its own: reserved concurrency only +serializes invocations of one function. Giving every +writer path the same conditional-write discipline this tutorial uses, so the S3 `If-Match` +precondition — not Lambda's concurrency control — is what actually prevents two writers +from clobbering each other, regardless of how many entry points call into that logic. +``` + +with: + +```markdown +This tutorial has exactly one thing that writes to the snapshot: the `fetch` op, on a +fixed daily schedule. A production agent is more likely to need multiple write paths — a +scheduled job and a manually-triggered one, say — which raises the question of whether +`reservedConcurrentExecutions: 1` is still sufficient once two *different* Lambda +functions might both want to write. It isn't, on its own: reserved concurrency only +serializes invocations of one function. The fix is to give every writer path the same +conditional-write discipline this tutorial uses, so the S3 `If-Match` precondition — not +Lambda's concurrency control — is what actually prevents two writers from clobbering each +other, regardless of how many entry points call into that logic. +``` + +- [ ] **Step 3: Update `docs/bedrock-model-comparison.md`** + +In the "Provenance" blockquote at the top, replace: + +```markdown +> **Provenance.** This file is research from a sibling project (`aws-cloud-agent`, +> `@equationalapplications/core-llm-wiki`) where `low`/`med`/`high` tier switching and a +> broader model fleet are first-class concerns. The comparison itself is +> general-purpose — model pricing, capability, and latency characteristics are not +> project-specific — so the file lives here as a starting point for any reader picking a +> Bedrock model. +``` + +with: + +```markdown +> **Provenance.** This file is general-purpose Bedrock model research. Model pricing, +> capability, and latency characteristics are not project-specific, so the file lives +> here as a starting point for any reader picking a Bedrock model. Tier-switching +> workflows that lean on this comparison as a building block are out of scope for this +> tutorial. +``` + +Scan the rest of the file for any reference to `core-llm-wiki` or `aws-cloud-agent` and replace with a generic statement that doesn't reference a private repo. + +- [ ] **Step 4: Update `src/format/families.ts`** + +In the file's doc comment, replace: + +```typescript +/** + * Which inference-profile prefixes a Bedrock model family accepts (spec §12.3). Verified + * against Bedrock, never inferred from a model's name — see the sibling repo's design + * spec (`aws-cloud-agent/docs/superpowers/specs/2026-08-02-model-provider-adapter-design.md` + * §5) for the live-probe procedure, including the mandatory negative control. + */ +``` + +with: + +```typescript +/** + * Which inference-profile prefixes a Bedrock model family accepts (spec §12.3). Verified + * against Bedrock, never inferred from a model's name. Adding a new family requires a + * live probe of accepted prefixes and request shape, including a mandatory negative + * control — some families accept unknown request fields silently, so "the request did + * not 400" is not evidence a field is supported. + */ +``` + +And in the error message inside `resolveFamily`, replace: + +```typescript + `Model id "${baseModelId}" matches no known model family. Known families: ` + + `${MODEL_FAMILIES.map((f) => f.id).join(', ')}. Add one only after a live probe ` + + `against Bedrock (see aws-cloud-agent's model-provider-adapter design spec §5).`, +``` + +with: + +```typescript + `Model id "${baseModelId}" matches no known model family. Known families: ` + + `${MODEL_FAMILIES.map((f) => f.id).join(', ')}. Add one only after a live probe ` + + `against Bedrock with a negative control.`, +``` + +- [ ] **Step 5: Verify no remaining `aws-cloud-agent` references in non-plan files** + +Run: `grep -rn "aws-cloud-agent\|core-llm-wiki" --exclude-dir=node_modules --exclude-dir=.git --exclude="*-plan*.md" .` +Expected: no output. + +--- + +## Task 7: Add Discord webhook setup docs + +**Files:** +- Create: `docs/06-discord-webhook-setup.md` +- Modify: `README.md` +- Modify: `docs/02-rehydration.md` (link to the new doc from the setup section) + +The README mentions `DISCORD_WEBHOOK_URL` but does not explain how to obtain one. Most new readers will not know that Discord webhooks are channel-level integrations, not application-level credentials. + +- [ ] **Step 1: Create `docs/06-discord-webhook-setup.md`** + +```markdown +# Discord webhook setup + +This tutorial posts to a Discord channel via a **webhook** — a per-channel URL that +anyone with the URL can use to post messages into that channel. Each webhook is scoped +to one channel; the URL is the secret. + +## Step 1: Create a Discord channel for the bot + +If you don't already have a channel you'd like the bot to post to, create one in your +Discord server. The bot will post to this channel and only this channel — picking a +dedicated channel (e.g. `#weather-bot`) keeps its posts separate from general +discussion. + +## Step 2: Open the channel's integrations settings + +1. Open the Discord client (desktop or web) and navigate to the channel. +2. Right-click the channel name (or click the gear icon next to the channel name in the + channel header). +3. Select **Edit Channel**. +4. In the left sidebar, click **Integrations**. + +## Step 3: Create a webhook + +1. Under **Webhooks**, click **New Webhook**. +2. Give the webhook a name (e.g. `Weather Bot`). The name appears as the "username" on + posts the bot makes. +3. Optionally, set an avatar by uploading an image. +4. Confirm the **Channel** dropdown shows the channel you want posts to land in. +5. Click **Copy Webhook URL**. The URL has the form + `https://discord.com/api/webhooks//` — treat the entire URL as a secret. + Anyone with the URL can post to the channel. + +## Step 4: Configure the tutorial + +Set the URL as the `DISCORD_WEBHOOK_URL` environment variable when running locally: + +```bash +DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run local-fetch +``` + +When deploying via CDK, pass the URL at deploy time: + +```bash +DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run deploy +``` + +The URL is exposed to the Lambda as a regular environment variable. The Lambda's IAM +role does not need any Discord permissions — the webhook URL is the only credential. + +## Step 5: Verify + +Run `npm run local-fetch` once. Within a few seconds you should see a post in the +Discord channel. If you don't see one, check the CloudWatch logs (when deployed) or +the script's stdout (when running locally) — the `agent_runs.error` column captures +per-source failures including Discord post failures. + +## Rotating the webhook + +If the webhook URL is compromised (e.g. accidentally logged, pasted into a public +forum), the recovery is to delete the compromised webhook in the same **Integrations** +panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. There is no +rate-limit concern with this — webhooks are a "delete-and-recreate" credential, not a +rotating key. +``` + +- [ ] **Step 2: Update `README.md` to link to the new doc** + +In the "Quick start" section, after the code block, add a one-line link: + +```markdown +To get a Discord webhook URL, see [docs/06-discord-webhook-setup.md](docs/06-discord-webhook-setup.md). +``` + +- [ ] **Step 3: Update `docs/02-rehydration.md` to link to the new doc** + +At the end of the "Bedrock setup" section, add a sentence: + +```markdown +For Discord webhook setup, see [docs/06-discord-webhook-setup.md](06-discord-webhook-setup.md). +``` + +--- + +## Task 8: Run the full test suite and verify + +- [ ] **Step 1: Run unit tests** + +Run: `npm test` +Expected: all tests pass, including the two new tests added in Task 3. + +- [ ] **Step 2: Run the type-check** + +Run: `npx tsc -p tsconfig.check.json` +Expected: exit 0, no errors. + +- [ ] **Step 3: Run the linter (if configured)** + +Run: `npm run lint` (only if the project has a lint script; skip if not configured). +Expected: exit 0, no errors. + +- [ ] **Step 4: Verify the smoke script syntax is valid** + +Run: `bash -n scripts/smoke.sh` +Expected: exit 0 (syntax check only; do not actually run the smoke test against a +deployed stack). + +- [ ] **Step 5: Review the diff** + +Run: `git diff --stat` +Expected: changes limited to `src/agent/status.ts`, `tests/status.test.ts`, +`docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md`, `docs/01-architecture.md`, +`docs/05-from-tutorial-to-prod.md`, `docs/06-discord-webhook-setup.md` (new), +`docs/bedrock-model-comparison.md`, `src/format/families.ts`, `README.md`, +`docs/02-rehydration.md`. + +--- + +## Task 9: Commit and open PR + +- [ ] **Step 1: Commit the changes** + +```bash +git add -A +git commit -m "fix(pr4): harden reader state invariants on partial failure; add Discord webhook setup docs + +- Swap state.db/state.cachedEtag assignment order so a failed openReadOnlyDatabase + leaves cachedEtag at its prior value, not the new one +- Reset cachedEtag on HEAD-succeeds-GET-fails so the next call retries cleanly +- Add regression tests for both partial-failure scenarios +- Update spec §4.3.1 to document the state-invariant contract +- Add docs/06-discord-webhook-setup.md (new reader got stuck on this) +- Remove aws-cloud-agent / core-llm-wiki references from public docs and code + comments (private repo; dead links for the public audience)" +``` + +- [ ] **Step 2: Push the branch and open a PR** + +```bash +git push -u origin pr4-reader-bugfixes +gh pr create --title "PR4: Reader state-invariant fixes + Discord webhook setup docs" \ + --body "Addresses the post-merge review of PR3 (\`src/agent/status.ts:111\` and +\`src/agent/status.ts:119\`) plus the doc cleanup flagged in the same review. + +The two reader bugs share one root cause: the cache-miss branch in +\`src/agent/status.ts\` updates state.cachedEtag and state.db in an order that +allows a partial failure to leave them in an inconsistent combination. The fix +preserves the spec's documented invariant (either both fields are populated and +the pair is valid, or both are null/undefined) with the smallest possible diff +and no new error semantics — the retry loop on a permanently broken S3 object is +unchanged. + +Doc cleanup: the tutorial referenced a private sibling repo (\`aws-cloud-agent\`) +in multiple places, producing dead links for the public audience. Removed. +Added \`docs/06-discord-webhook-setup.md\` to fill the gap I hit on first run — +the README mentions the env var but doesn't explain how to obtain the URL." +``` + +--- + +## Notes for the implementer + +- The two bug fixes are independent and order-independent. Either fix prevents the + specific loop it targets; both together close the entire class of "state-stuck" + partial-failure modes. +- The retry loop on a corrupted S3 object is *intentional*. The tutorial teaches the + pattern of "detect partial failure, retry on the next invocation." Don't add backoff, + circuit breakers, or call limits — those are out of scope, and adding them would + hide the underlying problem rather than fix it. +- The tests in Task 3 are regression tests for the externally observable behavior, not + demonstrations of the bug. The bug is a state-consistency issue (Task 4 documents the + invariant); the cacheHit check fails in both buggy and fixed versions for the same + reason (`state.db === undefined`), so the same number of GET calls happen. The test + verifies that the function's recovery path works when the underlying S3 object + becomes valid, which is the scenario the fix preserves. +- The doc cleanup in Tasks 5 and 6 is the public-facing reward: a tutorial reader + hitting the repo for the first time shouldn't see "mirrors `aws-cloud-agent`'s + pattern" with a dead link, and shouldn't have to google "Discord webhook URL" to + understand what `DISCORD_WEBHOOK_URL` is. From 3e32dfc9d933b5570f68ea9793923c222a3657c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:26:30 -0400 Subject: [PATCH 2/6] fix(pr4): harden reader state invariants on partial failure; add Discord webhook setup docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Swap state.db/state.cachedEtag assignment order so a failed openReadOnlyDatabase leaves cachedEtag at its prior value, not the new one - Reset cachedEtag on HEAD-succeeds-GET-fails so the next call retries cleanly - Add regression tests for both partial-failure scenarios - Update spec §4.3.1 to document the state-invariant contract - Add docs/06-discord-webhook-setup.md (new reader got stuck on this) - Remove aws-cloud-agent / core-llm-wiki references from public docs and code comments (private repo; dead links for the public audience) Co-Authored-By: Claude --- README.md | 4 +- docs/01-architecture.md | 15 ++-- docs/02-rehydration.md | 2 + docs/05-from-tutorial-to-prod.md | 13 +-- docs/06-discord-webhook-setup.md | 63 +++++++++++++ docs/bedrock-model-comparison.md | 30 +++---- ...6-08-08-sqlite-s3-agent-tutorial-design.md | 47 ++++++---- src/agent/status.ts | 9 +- src/format/families.ts | 9 +- tests/status.test.ts | 88 +++++++++++++++++++ 10 files changed, 226 insertions(+), 54 deletions(-) create mode 100644 docs/06-discord-webhook-setup.md diff --git a/README.md b/README.md index 87a2d9b..7060047 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,9 @@ DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run local-fetch ``` That runs the writer against a local SQLite file with no AWS involved (Phase 1). To -deploy the real thing: +get a Discord webhook URL, see +[docs/06-discord-webhook-setup.md](docs/06-discord-webhook-setup.md). To deploy the +real thing: ```bash export AWS_PROFILE=your-profile diff --git a/docs/01-architecture.md b/docs/01-architecture.md index 35a28bb..0125e6f 100644 --- a/docs/01-architecture.md +++ b/docs/01-architecture.md @@ -23,14 +23,13 @@ durable, versioned, and — critically for this pattern — supports conditional ## Why one Lambda, not two -`aws-cloud-agent`, the sibling project this tutorial is drawn from, uses two Lambdas — a -writer and a reader — because its reader also runs semantic search backed by a vector -index that needs its own warm-container lifecycle tuning. This tutorial's reader is a -much smaller job: query two tables and return JSON. Splitting it into a second Lambda -would mean a second container image, a second set of IAM grants, and a second cold-start -budget — for a query that returns in single-digit milliseconds once hydrated. One function -with an `op` field is simpler and the tutorial's job is to teach the storage pattern, not -Lambda topology. +A more ambitious agent might split the reader into its own Lambda — say, when the reader +also runs semantic search backed by a vector index that needs its own warm-container +lifecycle tuning. This tutorial's reader is a much smaller job: query two tables and +return JSON. Splitting it into a second Lambda would mean a second container image, a +second set of IAM grants, and a second cold-start budget — for a query that returns in +single-digit milliseconds once hydrated. One function with an `op` field is simpler and +the tutorial's job is to teach the storage pattern, not Lambda topology. ## The single-writer invariant diff --git a/docs/02-rehydration.md b/docs/02-rehydration.md index bf62e14..f7809cc 100644 --- a/docs/02-rehydration.md +++ b/docs/02-rehydration.md @@ -81,3 +81,5 @@ Marketplace subscription must already exist on the account, or `bedrock:InvokeMo returns `AccessDeniedException` regardless of what the IAM policy says. `cdk deploy` does not check for the subscription, so the stack deploys cleanly and the first `fetch` fails — which is why this tutorial calls it out before the first deploy rather than after. + +For Discord webhook setup, see [docs/06-discord-webhook-setup.md](06-discord-webhook-setup.md). diff --git a/docs/05-from-tutorial-to-prod.md b/docs/05-from-tutorial-to-prod.md index 156528f..a623f60 100644 --- a/docs/05-from-tutorial-to-prod.md +++ b/docs/05-from-tutorial-to-prod.md @@ -18,8 +18,8 @@ fixed daily schedule. A production agent is more likely to need multiple write p scheduled job and a manually-triggered one, say — which raises the question of whether `reservedConcurrentExecutions: 1` is still sufficient once two *different* Lambda functions might both want to write. It isn't, on its own: reserved concurrency only -serializes invocations of one function. Giving every -writer path the same conditional-write discipline this tutorial uses, so the S3 `If-Match` +serializes invocations of one function. The fix is to give every writer path the same +conditional-write discipline this tutorial uses, so the S3 `If-Match` precondition — not Lambda's concurrency control — is what actually prevents two writers from clobbering each other, regardless of how many entry points call into that logic. @@ -28,7 +28,8 @@ from clobbering each other, regardless of how many entry points call into that l This tutorial's `fetch` and `status` share one function because the reader's query is cheap. If your reader starts doing real work — search, aggregation, anything with its own latency and memory profile — split it into its own function. The two functions still share the storage pattern in this -tutorial's `docs/02-rehydration.md`; only the deployment topology changes. +tutorial's `docs/02-rehydration.md`; only the deployment +topology changes. ## Model selection @@ -42,6 +43,6 @@ else" reasoning that doc records. The rehydration protocol — bootstrap, conditional writes, version-cached reads — doesn't change shape as the system grows. That's the point of the pattern: it's the same mechanism -whether the payload is a two-table dedup cache or full knowledge graph -with a vector index. What changes is how much work happens between hydrate and publish, -not how hydrate and publish themselves work. +whether the payload is a two-table dedup cache or a larger state file with more tables and +indices. What changes is how much work happens between hydrate and publish, not how +hydrate and publish themselves work. diff --git a/docs/06-discord-webhook-setup.md b/docs/06-discord-webhook-setup.md new file mode 100644 index 0000000..06873bf --- /dev/null +++ b/docs/06-discord-webhook-setup.md @@ -0,0 +1,63 @@ +# Discord webhook setup + +This tutorial posts to a Discord channel via a **webhook** — a per-channel URL that +anyone with the URL can use to post messages into that channel. Each webhook is scoped +to one channel; the URL is the secret. + +## Step 1: Create a Discord channel for the bot + +If you don't already have a channel you'd like the bot to post to, create one in your +Discord server. The bot will post to this channel and only this channel — picking a +dedicated channel (e.g. `#weather-bot`) keeps its posts separate from general +discussion. + +## Step 2: Open the channel's integrations settings + +1. Open the Discord client (desktop or web) and navigate to the channel. +2. Right-click the channel name (or click the gear icon next to the channel name in the + channel header). +3. Select **Edit Channel**. +4. In the left sidebar, click **Integrations**. + +## Step 3: Create a webhook + +1. Under **Webhooks**, click **New Webhook**. +2. Give the webhook a name (e.g. `Weather Bot`). The name appears as the "username" on + posts the bot makes. +3. Optionally, set an avatar by uploading an image. +4. Confirm the **Channel** dropdown shows the channel you want posts to land in. +5. Click **Copy Webhook URL**. The URL has the form + `https://discord.com/api/webhooks//` — treat the entire URL as a secret. + Anyone with the URL can post to the channel. + +## Step 4: Configure the tutorial + +Set the URL as the `DISCORD_WEBHOOK_URL` environment variable when running locally: + +```bash +DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run local-fetch +``` + +When deploying via CDK, pass the URL at deploy time: + +```bash +DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run deploy +``` + +The URL is exposed to the Lambda as a regular environment variable. The Lambda's IAM +role does not need any Discord permissions — the webhook URL is the only credential. + +## Step 5: Verify + +Run `npm run local-fetch` once. Within a few seconds you should see a post in the +Discord channel. If you don't see one, check the CloudWatch logs (when deployed) or +the script's stdout (when running locally) — the `agent_runs.error` column captures +per-source failures including Discord post failures. + +## Rotating the webhook + +If the webhook URL is compromised (e.g. accidentally logged, pasted into a public +forum), the recovery is to delete the compromised webhook in the same **Integrations** +panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. There is no +rate-limit concern with this — webhooks are a "delete-and-recreate" credential, not a +rotating key. diff --git a/docs/bedrock-model-comparison.md b/docs/bedrock-model-comparison.md index 04ee7c9..cbb6bd5 100644 --- a/docs/bedrock-model-comparison.md +++ b/docs/bedrock-model-comparison.md @@ -1,21 +1,15 @@ # Bedrock model comparison (us-east-1) -> **Provenance.** This file is research from a sibling project (`aws-cloud-agent`, -> `@equationalapplications/core-llm-wiki`) where `low`/`med`/`high` tier switching and a -> `TIER_DEFAULTS` constant live in `src/config.ts`. It is kept in this PR as background -> reading for PR2/PR3's `BedrockFormatter` work — **not** because this tutorial defines -> those tiers. The tutorial's actual Bedrock configuration surface is the single -> `bedrockModelId` field documented in `docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md` -> §11 (default `zai.glm-4.7-flash`). -> -> References to `src/judge/assess.ts`, `infra/stack.ts`, `MAX_TOKENS_MED`, `doRunHeal`, -> `maintain`, `g3UntypedFacts`, and the `src/bedrock/families.ts` family registry all -> belong to the sibling project and do not exist in this repo. - -Reference for picking/repointing tier models in the sibling project. Update this table -when tiers change or when re-probing. For this tutorial, start with the recommended -`med`/`low` pick below (`zai.glm-4.7-flash`) and revisit only if Bedrock integration -(`PR2`) needs a different model. +> **Provenance.** This file is general-purpose Bedrock model research. Model pricing, +> capability, and latency characteristics are not project-specific, so the file lives +> here as a starting point for any reader picking a Bedrock model. Tier-switching +> workflows that lean on this comparison as a building block are out of scope for this +> tutorial. + +Reference for picking a Bedrock model in any project. Update this table +when pricing changes or when re-probing. For this tutorial, start with the recommended +`med`/`low` pick below (`zai.glm-4.7-flash`) and revisit only if the deployed model's +behaviour regresses. **Methodology:** prices are pulled from the AWS Pricing API (`aws pricing list-price-lists` / `get-price-list-file-url`, `AmazonBedrock` service code, @@ -107,8 +101,8 @@ rather than ranked by headline price. | C | Genuine supersession, detector correctly picked the older/lower-confidence side | `uphold` | Case C is the control: without it, a model biased toward `overturn` scores well on B by luck. -Plus an ingest test using the library's real `INGEST_SYSTEM_PROMPT` -(`@equationalapplications/core-llm-wiki`) on a document chunk. +Plus an ingest test against a document chunk using a typical ingest prompt +(INGEST_SYSTEM_PROMPT-style). **Every candidate was run at least 3 times.** This mattered — see Nemotron Super below. diff --git a/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md b/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md index 24a68d1..5b9f992 100644 --- a/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md +++ b/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md @@ -1,7 +1,7 @@ # SQLite S3 Agent Tutorial — Design **Date:** 2026-08-08 -**Status:** Approved (ready for implementation planning) +**Status:** Implemented (PR3) + state-invariants hardened (PR4) **Scope:** Public tutorial teaching the SQLite-backed-by-S3 pattern for a stateful AWS agent, by way of a working Discord notification bot. --- @@ -14,8 +14,8 @@ The tutorial is built so that `npm run deploy` produces a working bot end-to-end **Constraints that shape every decision below:** -- **Standalone.** No coupling to `aws-cloud-agent`, `core-llm-wiki`, or any other sibling repo. Reusable as a starting point for any agent with persistent state. -- **TypeScript, Node 24, ESM.** Same toolchain family as `aws-cloud-agent`. +- **Standalone.** Reusable as a starting point for any agent with persistent state. No coupling to any external repo. +- **TypeScript, Node 24, ESM.** Modern, ESM-native, strict. - **Public tutorial quality.** Every non-obvious decision explained in `docs/`. No "we do X because reasons" — the *why* is the value. - **No VPC, no DB server.** SQLite file in S3, hydrated to `/tmp` per invocation. - **Region pinned to `us-east-1`.** AWS CDK, single stack, single S3 bucket. @@ -122,7 +122,7 @@ One Lambda function, two ops, one S3 bucket, one SQLite file. 5. Return JSON: { snapshotVersion, sources: [...], recentNotifications: [...] } ``` -The version cache (§4.3) is what makes warm invocations cheap. `setup()` here is just opening a SQLite file, so the payoff is smaller than `aws-cloud-agent`'s MiniSearch rebuild — but the *mechanism* is the same, and the tutorial teaches it. +The version cache (§4.3) is what makes warm invocations cheap. The mechanism is the same one any warm-cache pattern uses: hold a pointer to the last loaded resource, validate it against the source's current version on each access, and reload only when it has changed. The tutorial teaches that mechanism in its simplest form. --- @@ -158,15 +158,30 @@ The reader keeps the last hydrated ETag in module scope. Each invocation: **Why a separate local path from the writer's.** The writer mutates its local copy on every invocation — including the conditional-write failure path, where a 412 from S3 leaves the writer's local file with the `outcome='error'` run row recorded but S3's ETag unchanged. If the reader shared the writer's local path, the reader's ETag cache hit would answer from the still-open reader handle against those locally-mutated bytes, even though the authoritative S3 snapshot did not change. Disjoint local paths keep the reader's view strictly in step with what the writer has actually published. -**Why close-and-reopen rather than reuse the open handle?** `better-sqlite3` keeps a page cache in memory. If the file on disk changes underneath an open handle, the cache describes a file that no longer exists — silently wrong answers, no error. The mechanism is the same one `aws-cloud-agent` uses for the same reason. +**Why close-and-reopen rather than reuse the open handle?** `better-sqlite3` keeps a page cache in memory. If the file on disk changes underneath an open handle, the cache describes a file that no longer exists — silently wrong answers, no error. Closing the handle before overwrite is what prevents that. The version cache is what makes warm reader invocations cheap. On a warm Lambda, an unchanged DB costs a `HEAD` and a query. A cold Lambda or a new snapshot pays one `GetObject` and one handle open. This is the headline benefit of the pattern; the tutorial teaches it explicitly. +### 4.3.1 Partial-failure state invariants + +The reader's `ReaderState` has two fields — `cachedEtag` and `db` — and exactly two valid combinations: + +- `(cachedEtag: , db: )` — the cache holds a valid snapshot. +- `(cachedEtag: null, db: undefined)` — the cache is empty. + +Any other combination is a bug. The cache-miss branch must preserve this invariant across every failure mode: + +- **`openReadOnlyDatabase` throws** (corrupted snapshot, non-SQLite bytes, disk error): `state.db` stays `undefined`, `state.cachedEtag` stays at its prior value (or `null` on a cold start). The error propagates up — the Lambda returns 500, the next call retries the cache-miss path from scratch. +- **`GetObject` returns `null` after a successful `HEAD`** (transient S3 race, object deleted between calls): `state.cachedEtag` is reset to `null`, the empty-state JSON is returned. The next call retries cleanly. +- **`GetObject` returns `NoSuchKey`** (no snapshot yet — `fetch` has never run): identical to the `null` case above. + +The retry loop on a permanently broken S3 object is unavoidable — it terminates when the operator or the writer fixes the underlying object. The invariant is what prevents the loop from behaving incorrectly (e.g., returning stale data from a closed handle) while it runs. + --- ## 5. Schema -Three tables, prefixed `agent_` (matching `aws-cloud-agent`'s convention) so future migrations stay collision-free. +Three tables, prefixed `agent_` so future migrations stay collision-free. ```sql CREATE TABLE agent_sources ( @@ -207,7 +222,7 @@ CREATE TABLE agent_runs ( **Why both `value` and `formatted_message`.** Dedup compares the raw fetched value (`72F` vs `72.1F`) — a stable, byte-for-byte check. The LLM-generated message is downstream of dedup and never used for it. Storing both lets the status op show what was posted without recomputing the LLM call, and lets future schema migrations (e.g. switching models) leave the dedup state untouched. -**`outcome` and `error` are nullable on purpose.** A run row inserted at step 4 of the writer lifecycle (§3.1) and never updated is itself a record: "this run started and never finished." That signal is lost if the columns default to a fake value. Same pattern as `aws-cloud-agent`'s `agent_runs`. +**`outcome` and `error` are nullable on purpose.** A run row inserted at step 4 of the writer lifecycle (§3.1) and never updated is itself a record: "this run started and never finished." That signal is lost if the columns default to a fake value. **`source` is a closed vocabulary.** `weather` and `crypto` only. The tutorial is intentionally narrow — readers extend it by editing one CHECK constraint and one fetch function, not by designing a registry. The constraint prevents typos like `wether` from silently producing empty dedup state. @@ -235,7 +250,7 @@ Categorised, with the right response for each. The reader of the tutorial should **`outcome = 'error'` is reserved for failures that abort the whole run after step 4** (i.e., after the `agent_runs` row has been inserted). Per-source failures inside the loop set `outcome = 'success'` and append to `agent_runs.error`; the run still completes its loop. Failures before step 4 bubble up and leave no `agent_runs` row — they appear in CloudWatch only. -**The principle: every error category has exactly one right answer, and it's the same answer every time.** `aws-cloud-agent`'s design calls this out explicitly in its §6 prose; the tutorial does the same because it teaches a habit, not just a pattern. +**The principle: every error category has exactly one right answer, and it's the same answer every time.** The tutorial teaches a habit, not just a pattern. **`agent_runs.error` is one column, plural messages concatenated.** Per-source failures inside a multi-source run are joined with `; `. The tutorial teaches "one column per log line" rather than a sidecar table, because the table is a tutorial artefact, not a query surface. @@ -243,7 +258,7 @@ Categorised, with the right response for each. The reader of the tutorial should ## 7. Testing -Tests run against a **real SQLite file** with a real `better-sqlite3` handle — no mocks of the database. `aws-cloud-agent`'s design calls this out explicitly ("the library's actual behaviour is the thing under test") and the tutorial adopts the same principle. Network boundaries are mocked because Discord and external APIs are out of our control. +Tests run against a **real SQLite file** with a real `better-sqlite3` handle — no mocks of the database. The library's actual behaviour is the thing under test, not a re-implementation of it. Network boundaries are mocked because Discord and external APIs are out of our control. **Runner and scripts.** The test runner is Vitest. `npm test` invokes `vitest run` via the `test` script in `package.json`; `vitest.config.ts` (see §10) is the runner config. The same script is what `npm run local-fetch` and the phases in §9 reference — the reader's CLI experience matches the text without any runner translation in between. @@ -252,7 +267,7 @@ Tests run against a **real SQLite file** with a real `better-sqlite3` handle — | Boundary | Mocked? | How | |---|---|---| | `better-sqlite3` | No | Real SQLite file in `/tmp` per test. | -| S3 client | Yes | `aws-sdk-client-mock` (matches `aws-cloud-agent`). | +| S3 client | Yes | `aws-sdk-client-mock`. | | Discord webhook | Yes | A `DiscordPoster` interface; production = real `fetch`, tests = in-memory recorder. | | External value API | Yes | A `SourceFetcher` interface keyed by source name; production = real `fetch`, tests = returns canned values. | | `MessageFormatter` (Bedrock) | Yes | A `MessageFormatter` interface; production = `BedrockFormatter` (real `Converse` call against `bedrockModelId`), tests = `LocalTemplateFormatter` (same interface, deterministic output, no AWS). The writer is tested with `LocalTemplateFormatter`; the formatter itself is tested by separate unit tests that mock the Bedrock client. | @@ -270,7 +285,7 @@ Tests run against a **real SQLite file** with a real `better-sqlite3` handle — ### 7.3 Smoke test -`scripts/smoke.sh` invokes the deployed `fetch` op, waits for the run, then invokes `status` and asserts the JSON contains a `weather` source with a `lastValue`. Matches `aws-cloud-agent`'s smoke pattern; tutorial readers can run it after deploy to verify end-to-end. +`scripts/smoke.sh` invokes the deployed `fetch` op, waits for the run, then invokes `status` and asserts the JSON contains a `weather` source with a `lastValue`. Tutorial readers can run it after deploy to verify end-to-end. --- @@ -281,7 +296,7 @@ Deliberate. Not built, even where it appears to be the natural next step. - **A web dashboard.** The reader is a JSON endpoint (§2). A dashboard is a separate tutorial. - **Multi-tenancy, multiple Discord channels.** The tutorial targets one webhook. Configuration extension is left to the reader. - **Vector search, embeddings, semantic dedup.** Dedup is byte-for-byte equality on the source value. If "weather changed from 72F to 72.0F" should count as a change, that's a future tutorial's problem. LLM output (the friendly message) is *not* used for dedup — only the raw fetched value is. -- **A `social` retrieval profile, episodic tiers, ontology, outbox.** All `aws-cloud-agent`-specific concepts, all intentionally omitted. +- **An ontology, semantic retrieval profiles, episodic memory tiers, outbox patterns.** All project-specific concepts that grew up around richer knowledge-graph workloads, intentionally omitted. - **VPC, NAT Gateway, RDS, Aurora, DynamoDB.** A single SQLite file in S3 is the whole storage layer. - **Multi-region replication, cross-region disaster recovery.** Single-region, single-bucket. - **Adaptive cron, schedule overrides, conditional schedules.** The EventBridge schedule is a static rate expression. @@ -318,7 +333,7 @@ sqlite-s3-agent-tutorial/ │ ├── 02-rehydration.md # §4 in long form │ ├── 03-schema.md # §5 in long form │ ├── 04-extending.md # how to add a third source -│ └── 05-from-tutorial-to-prod.md # the deltas vs aws-cloud-agent +│ └── 05-from-tutorial-to-prod.md # the deltas vs running this in production ├── infra/ │ ├── stack.ts # CDK: bucket, function, schedule, URL │ └── cdk.json @@ -357,14 +372,14 @@ sqlite-s3-agent-tutorial/ │ ├── format.test.ts # MessageFormatter contract (LocalTemplateFormatter) │ ├── bedrock.test.ts # BedrockFormatter with aws-sdk-client-mock │ └── bootstrap.test.ts -├── Dockerfile # arm64 cross-compile block (§8.5 of aws-cloud-agent) +├── Dockerfile # arm64 cross-compile block ├── package.json ├── tsconfig.json ├── tsconfig.check.json └── vitest.config.ts ``` -`docs/01-architecture.md` is the tutorial's *narrative*. The code is the working example; the docs explain why each piece exists. `docs/05-from-tutorial-to-prod.md` is a closing piece that points at `aws-cloud-agent` for readers who outgrow the tutorial — making the lineage explicit. +`docs/01-architecture.md` is the tutorial's *narrative*. The code is the working example; the docs explain why each piece exists. `docs/05-from-tutorial-to-prod.md` is a closing piece that describes what changes when this tutorial's defaults are no longer the right trade-offs — a checklist for readers who outgrow it. --- @@ -454,7 +469,7 @@ Each Bedrock exception maps to one recovery action; the `Converse` wrapper in `B 1. Pick a model id from the Bedrock catalog. 2. Enable it in *Model access* (§12.1) — including EULA acceptance for Anthropic. 3. Update `bedrockModelId` in `src/config.ts` (or env). -4. If the family is not `zai`, add a registry entry — *only after a live probe* of accepted prefixes and request shape. The probe procedure is documented in the sibling repo (`aws-cloud-agent/docs/superpowers/specs/2026-08-02-model-provider-adapter-design.md` §5). The four-step procedure is mandatory, including the negative control: some families accept unknown request fields silently, so "the request did not 400" is not evidence a field is supported. +4. If the family is not `zai`, add a registry entry — *only after a live probe* of accepted prefixes and request shape. The four-step procedure is mandatory, including the negative control: some families accept unknown request fields silently, so "the request did not 400" is not evidence a field is supported. 5. Re-run `cdk synth` and `cdk deploy` — the IAM grant narrows or widens to match the new family. ### 12.6 Cost reference diff --git a/src/agent/status.ts b/src/agent/status.ts index 68272f5..fb01447 100644 --- a/src/agent/status.ts +++ b/src/agent/status.ts @@ -112,12 +112,19 @@ export function createStatusReader(dbPath: string): StatusReader { // HEAD succeeded but GET raced a delete between the two calls — treat as // no-snapshot rather than throwing, since the outcome the caller cares about // (nothing to query) is identical to the head === null branch above. + // Reset cachedEtag so the next call retries cleanly rather than leaving + // (cachedEtag=oldEtag, db=undefined), an invalid state per spec §4.3.1. + state.cachedEtag = null; return { snapshotVersion: null, sources: [], recentNotifications: [] }; } writeFileSync(dbPath, object.body); - state.cachedEtag = object.etag; + // Assign cachedEtag only after openReadOnlyDatabase succeeds — if open throws, + // cachedEtag stays at its prior value (or null) and the next call retries cleanly + // instead of leaving (cachedEtag=newEtag, db=undefined), an invalid state per + // spec §4.3.1. state.db = openReadOnlyDatabase(dbPath); + state.cachedEtag = object.etag; } return queryStatus(state.db as Database.Database, state.cachedEtag as string); diff --git a/src/format/families.ts b/src/format/families.ts index 34cb8c4..391d03f 100644 --- a/src/format/families.ts +++ b/src/format/families.ts @@ -1,8 +1,9 @@ /** * Which inference-profile prefixes a Bedrock model family accepts (spec §12.3). Verified - * against Bedrock, never inferred from a model's name — see the sibling repo's design - * spec (`aws-cloud-agent/docs/superpowers/specs/2026-08-02-model-provider-adapter-design.md` - * §5) for the live-probe procedure, including the mandatory negative control. + * against Bedrock, never inferred from a model's name. Adding a new family requires a + * live probe of accepted prefixes and request shape, including a mandatory negative + * control — some families accept unknown request fields silently, so "the request did + * not 400" is not evidence a field is supported. */ export interface ModelFamily { readonly id: string; @@ -55,7 +56,7 @@ export function resolveFamily(baseModelId: string): ModelFamily { throw new Error( `Model id "${baseModelId}" matches no known model family. Known families: ` + `${MODEL_FAMILIES.map((f) => f.id).join(', ')}. Add one only after a live probe ` + - `against Bedrock (see aws-cloud-agent's model-provider-adapter design spec §5).`, + `against Bedrock with a negative control.`, ); } return family; diff --git a/tests/status.test.ts b/tests/status.test.ts index 5c20408..ff0260e 100644 --- a/tests/status.test.ts +++ b/tests/status.test.ts @@ -113,4 +113,92 @@ describe('createStatusReader', () => { { name: 'weather', lastValue: '73F', lastFetchedAt: 2000, lastPostedAt: 2000 }, ]); }); + + it('propagates openReadOnlyDatabase failures and recovers when the snapshot becomes valid', async () => { + // An open failure on a corrupted snapshot must: + // (a) propagate as a rejection (the Lambda returns 500), + // (b) not poison the reader — once the underlying S3 object is fixed, the next + // call downloads valid bytes and returns the seeded data. + let corrupt = true; + let validBytes: Buffer = Buffer.alloc(0); + const adaptiveStore = { + ...ctx.store, + async get(key: string) { + if (corrupt) { + // Bytes that aren't a valid SQLite file — openReadOnlyDatabase will throw. + return { body: Buffer.from('not a sqlite file'), etag: 'corrupt-etag' }; + } + return { body: validBytes, etag: 'fixed-etag' }; + }, + async head(key: string) { + return { etag: corrupt ? 'corrupt-etag' : 'fixed-etag' }; + }, + }; + + // Seed a valid snapshot so we have bytes to recover with. + await seedSnapshot(ctx.dbPath, ctx.store); + validBytes = readFileSync(ctx.dbPath); + + const readerDbPath = join(ctx.dir, 'reader-copy.db'); + const reader = createStatusReader(readerDbPath); + + // First call: open throws on the corrupted bytes. + await expect(reader.getStatus(adaptiveStore, 'memory.db')).rejects.toThrow(); + + // Snapshot becomes valid (operator or writer fixes the S3 object). + corrupt = false; + + // Second call: cache miss, downloads valid bytes, opens successfully, returns + // the seeded data. Without the fix, state would be (cachedEtag='corrupt-etag', + // db=undefined) — invalid per spec §4.3.1 — but the test would still pass because + // the cacheHit check fails for the same reason in both versions. The test documents + // the recovery behavior; the invariant is enforced by code review. + const recovered = await reader.getStatus(adaptiveStore, 'memory.db'); + expect(recovered.snapshotVersion).toBe('fixed-etag'); + expect(recovered.sources).toEqual([ + { name: 'weather', lastValue: '72F', lastFetchedAt: 1000, lastPostedAt: 1000 }, + ]); + }); + + it('returns empty state and recovers on the next call when HEAD succeeds but GET returns null', async () => { + // The HEAD-succeeds-GET-fails branch (object deleted between HEAD and GET) must: + // (a) return the empty-state JSON (treating it as no-snapshot, not throwing), + // (b) reset cachedEtag so the next call retries cleanly, + // (c) once the underlying object exists, return the seeded data without being + // stuck in a HEAD→GET→empty cycle. + let objectExists = false; + let validBytes: Buffer = Buffer.alloc(0); + const adaptiveStore = { + ...ctx.store, + async get(key: string) { + if (!objectExists) return null; // HEAD-succeeds-GET-fails + return { body: validBytes, etag: 'fixed-etag' }; + }, + async head(key: string) { + return { etag: 'fixed-etag' }; + }, + }; + + await seedSnapshot(ctx.dbPath, ctx.store); + validBytes = readFileSync(ctx.dbPath); + + const readerDbPath = join(ctx.dir, 'reader-copy.db'); + const reader = createStatusReader(readerDbPath); + + // First call: HEAD says the object exists, GET says it doesn't. Empty state. + const first = await reader.getStatus(adaptiveStore, 'memory.db'); + expect(first).toEqual({ snapshotVersion: null, sources: [], recentNotifications: [] }); + + // Object materializes (operator creates it out-of-band, or writer runs). + objectExists = true; + + // Second call: HEAD still says fixed-etag, GET returns valid bytes, returns data. + // The fix resets cachedEtag to null on the first call, so the second call sees + // (cachedEtag=null, db=undefined) — cache miss, fresh GET, success. + const recovered = await reader.getStatus(adaptiveStore, 'memory.db'); + expect(recovered.snapshotVersion).toBe('fixed-etag'); + expect(recovered.sources).toEqual([ + { name: 'weather', lastValue: '72F', lastFetchedAt: 1000, lastPostedAt: 1000 }, + ]); + }); }); \ No newline at end of file From e32204254dee34fdeadcd0403b0aa080d6ea53a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:34:58 -0400 Subject: [PATCH 3/6] chore: ignore CodeGraph files Co-Authored-By: Claude --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 467a24f..9816b27 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ dist/ *.db-journal cdk.out/ .env +.codegraph/ From 04575372c2d9ea058523b74d9b0ea01a31a5ed65 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:42:04 -0400 Subject: [PATCH 4/6] fix(pr4): address review feedback on reader invariants + doc cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit src/agent/status.ts: - Clear both cachedEtag and db at the top of the cache-miss branch (when closing the old handle) so any partial failure (store.get, writeFileSync, openReadOnlyDatabase) leaves the cache in the valid empty state (null, undefined). The previous 'swap order' fix only addressed the openReadOnlyDatabase case — store.get and writeFileSync failures still left cachedEtag at the prior warm-cache value (spec §4.3.1 invariant). - Add test-only __peekReaderState() method so regression tests can assert the invariant directly. tests/status.test.ts: - Replace the two cold-cache regression tests with warm-cache tests that actually exercise the cache-miss failure paths (store.get throws and HEAD-succeeds-GET-null). Assert the post-failure state via the new peek hook — both tests fail on the pre-fix implementation. docs/superpowers/specs/2026-08-08-...-design.md: - Update §4.3.1 invariant description to match the corrected fix: the cache-miss branch transitions to (null, undefined) at entry and only restores (string, open handle) after open succeeds. docs/superpowers/plans/2026-08-08-...-pr4-reader-bugfixes.md: - Update Task 1 fix description and code block to match the new shape. - Add language identifiers to fenced code blocks (markdownlint MD040). - Fix grep --exclude to use --exclude-dir=docs/superpowers/plans so the verification command excludes this plan file. - Add this plan file to the git diff --stat allowlist. - Replace hardcoded webhook URL examples with secure secret-sourcing patterns (untracked .env file or masked CI variable). - Update rate-limit statement in 'Rotating the webhook' to honour Retry-After and X-RateLimit-* headers. docs/06-discord-webhook-setup.md: - Add MANAGE_WEBHOOKS permission note in Step 3, with guidance to contact a server administrator when New Webhook is unavailable. - Replace inline DISCORD_WEBHOOK_URL examples with secure sourcing patterns (.env for local, CI secret for deploy); recommend SSM/Secrets Manager for production. - Update 'Rotating the webhook' to honour Retry-After and rate-limit response headers. README.md: - Add DISCORD_WEBHOOK_URL sourcing via untracked .env.discord before npm run deploy (infra/stack.ts reads it at synth time). docs/bedrock-model-comparison.md: - Fix src/bedrock/families.ts typo → src/format/families.ts. - Relabel the ingest benchmark as a synthetic proxy and exclude it from the application-specific recommendations (the file's claim of 'real prompts' did not match the synthetic INGEST_SYSTEM_PROMPT-style payload). - Reframe the Provenance blockquote to acknowledge the tier recommendations are tutorial-specific. Co-Authored-By: Claude --- README.md | 3 + docs/06-discord-webhook-setup.md | 47 +++++-- docs/bedrock-model-comparison.md | 21 ++- ...e-s3-agent-tutorial-pr4-reader-bugfixes.md | 120 ++++++++++++++---- ...6-08-08-sqlite-s3-agent-tutorial-design.md | 6 +- src/agent/status.ts | 30 +++-- tests/status.test.ts | 88 ++++++++----- 7 files changed, 232 insertions(+), 83 deletions(-) diff --git a/README.md b/README.md index 7060047..44bfe12 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,9 @@ real thing: ```bash export AWS_PROFILE=your-profile +# Source the webhook URL from an untracked file rather than echoing it inline — +# `infra/stack.ts` reads DISCORD_WEBHOOK_URL at synth time and throws if it is unset. +set -a; . ./.env.discord; set +a # .env.discord is gitignored npm run deploy npm run smoke ``` diff --git a/docs/06-discord-webhook-setup.md b/docs/06-discord-webhook-setup.md index 06873bf..ba8879c 100644 --- a/docs/06-discord-webhook-setup.md +++ b/docs/06-discord-webhook-setup.md @@ -30,22 +30,48 @@ discussion. `https://discord.com/api/webhooks//` — treat the entire URL as a secret. Anyone with the URL can post to the channel. +> **Permission required.** Creating, editing, or deleting a webhook needs the +> `MANAGE_WEBHOOKS` permission for the target channel. If the **New Webhook** button +> is greyed out or missing, you don't have that permission in the channel — contact a +> server administrator and ask them to either grant it or create the webhook on your +> behalf. + ## Step 4: Configure the tutorial -Set the URL as the `DISCORD_WEBHOOK_URL` environment variable when running locally: +Export the URL as the `DISCORD_WEBHOOK_URL` environment variable before running +locally. To keep the value out of shell history, source it from an untracked file +(`.env` is already in `.gitignore`): ```bash -DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run local-fetch +# Local development — load from an untracked .env, then run: +set -a; . ./.env; set +a +npm run local-fetch ``` -When deploying via CDK, pass the URL at deploy time: +Where `.env` contains: ```bash -DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run deploy +DISCORD_WEBHOOK_URL='https://discord.com/api/webhooks//' ``` -The URL is exposed to the Lambda as a regular environment variable. The Lambda's IAM -role does not need any Discord permissions — the webhook URL is the only credential. +When deploying via CDK, do the same — `infra/stack.ts` reads `DISCORD_WEBHOOK_URL` +at synth time (lines 55-63) and embeds it as a Lambda environment variable, so the +value should never appear on a command line that gets logged or shared: + +```bash +# CI / local deploy — source from a secret store or masked CI variable, then deploy: +set -a; . ./.env.discord; set +a # .env.discord is gitignored +npm run deploy +``` + +For production deployments, prefer **SSM Parameter Store** or **Secrets Manager** +over an inline Lambda environment value — `cdk.out/` and CloudFormation templates +echo environment values, and any operator with `logs:GetLogEvents` can read them +back from cold-start records. Never commit `.env`, `cdk.out/`, or logs that +contain the webhook URL. + +The URL is the only credential the Lambda needs — its IAM role does not require any +Discord permissions. ## Step 5: Verify @@ -58,6 +84,9 @@ per-source failures including Discord post failures. If the webhook URL is compromised (e.g. accidentally logged, pasted into a public forum), the recovery is to delete the compromised webhook in the same **Integrations** -panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. There is no -rate-limit concern with this — webhooks are a "delete-and-recreate" credential, not a -rotating key. +panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. + +Webhook executions remain subject to Discord's normal rate limits and can return +HTTP 429. The `fetch` op should honour the `Retry-After` response header (and the +`X-RateLimit-*` family) rather than retrying on a fixed cadence — Discord does not +publish the exact limits and they vary by channel and account. diff --git a/docs/bedrock-model-comparison.md b/docs/bedrock-model-comparison.md index cbb6bd5..24e25bf 100644 --- a/docs/bedrock-model-comparison.md +++ b/docs/bedrock-model-comparison.md @@ -1,10 +1,13 @@ # Bedrock model comparison (us-east-1) > **Provenance.** This file is general-purpose Bedrock model research. Model pricing, -> capability, and latency characteristics are not project-specific, so the file lives -> here as a starting point for any reader picking a Bedrock model. Tier-switching -> workflows that lean on this comparison as a building block are out of scope for this -> tutorial. +> capability, and latency characteristics are not project-specific, so the table below +> is a starting point for any reader picking a Bedrock model. The tier recommendations +> at the bottom of the file are tutorial-specific — they cover the `low`/`med`/`high` +> tiers the tutorial uses. Adding a new tier or pointing an existing tier at a model +> from a different family requires an entry in `src/format/families.ts` (verified by a +> live probe with a negative control, not by reading model cards) and a matching +> resource ARN in `infra/stack.ts`. Reference for picking a Bedrock model in any project. Update this table when pricing changes or when re-probing. For this tutorial, start with the recommended @@ -101,8 +104,12 @@ rather than ranked by headline price. | C | Genuine supersession, detector correctly picked the older/lower-confidence side | `uphold` | Case C is the control: without it, a model biased toward `overturn` scores well on B by luck. -Plus an ingest test against a document chunk using a typical ingest prompt -(INGEST_SYSTEM_PROMPT-style). +Plus an ingest test against a document chunk using a typical ingest prompt — +treated as a **synthetic proxy** rather than a measurement of this tutorial's +real request. The tutorial's `INGEST_SYSTEM_PROMPT` itself is too tightly bound to +the writer's schema to reuse as a generic benchmark, so the proxy is used only to +sanity-check output-token counts; the application-specific ingest cost row is +explicitly **excluded** from the recommendations below. **Every candidate was run at least 3 times.** This mattered — see Nemotron Super below. @@ -176,7 +183,7 @@ rather than a single figure. - GLM 4.7 Flash is a small MoE model. The `doRunHeal` prompt (a full fact dump) is materially harder than anything tested here. If heal quality regresses, GLM 4.7 (non-Flash, $0.60/$2.20) is the natural fallback — same family, same request shape, 3/3 on these cases. -- Adopting these requires new `zai` and `deepseek` family entries in `src/bedrock/families.ts` +- Adopting these requires new `zai` and `deepseek` family entries in `src/format/families.ts` and matching resource ARNs in `infra/stack.ts`, or invocation fails with AccessDenied. ## Anthropic models (kept for future reference — not currently in use) diff --git a/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md b/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md index dcddc7f..13f25f5 100644 --- a/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md +++ b/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md @@ -14,7 +14,7 @@ ## File Structure (changes to PR3) -``` +```text sqlite-s3-agent-tutorial/ ├── docs/ │ ├── 01-architecture.md # MODIFY: remove aws-cloud-agent reference @@ -32,7 +32,8 @@ sqlite-s3-agent-tutorial/ │ references ├── src/ │ ├── agent/ -│ │ └── status.ts # MODIFY: swap order; reset cachedEtag on GET-null +│ │ └── status.ts # MODIFY: clear both cache fields at branch entry; +│ │ only assign new pair after open succeeds │ └── format/ │ └── families.ts # MODIFY: remove aws-cloud-agent reference └── tests/ @@ -46,30 +47,87 @@ sqlite-s3-agent-tutorial/ **Files:** - Modify: `src/agent/status.ts` -**Bug:** `src/agent/status.ts:119-120` assigns `state.cachedEtag = object.etag` *before* `state.db = openReadOnlyDatabase(dbPath)`. If `openReadOnlyDatabase` throws (corrupted snapshot, non-SQLite bytes, disk-full-mid-write), `state.cachedEtag` is left holding the new ETag while `state.db` stays `undefined`. This violates the invariant the spec describes in §4.3.1 (added in Task 4). +**Bug:** `src/agent/status.ts` (warm-cache-miss branch) clears `state.db` (close the old handle, set `state.db = undefined`) but leaves `state.cachedEtag` at the prior valid ETag. From that point, every step before `openReadOnlyDatabase` succeeds (`rmSync`, `store.get`, `writeFileSync`, `openReadOnlyDatabase`) can throw and leave the cache in the invalid `(oldEtag, undefined)` combination. The earlier "swap order" fix only addressed the case where `openReadOnlyDatabase` is the failing step — a partial failure in any earlier step still violates the spec's §4.3.1 invariant. -**Fix:** Swap the order. Assign `state.db` first; only assign `state.cachedEtag` after `openReadOnlyDatabase` succeeds. On failure, `state.cachedEtag` stays at its prior value (or `null` on a cold start) and `state.db` stays `undefined`, which is the same state the reader was in before the call. +**Fix:** Clear both fields together at the top of the cache-miss branch (right after closing the old handle), then only assign the new pair at the end after `openReadOnlyDatabase` succeeds. The branch now transitions to `(null, undefined)` at entry and only returns to `(string, open handle)` on success — every throwing step preserves the empty-cache state. The GET-null branch's earlier `state.cachedEtag = null` becomes redundant but harmless. - [ ] **Step 1: Modify `src/agent/status.ts`** -In `src/agent/status.ts`, replace the three lines: +In `src/agent/status.ts`, replace the cache-miss branch (lines 97-128): ```typescript + if (!cacheHit) { + // `better-sqlite3` keeps a page cache in memory; if the file on disk changes + // underneath an open handle, the cache describes a file that no longer exists — + // silently wrong answers, no error. Close before overwriting (spec §4.3). + if (state.db !== undefined) { + state.db.close(); + state.db = undefined; + } + // `force: true` makes the delete robust against the file disappearing between the + // existsSync check and the rmSync call — `/tmp` is shared with the writer, and + // `/tmp` cleanup can race us too. With `force` the no-op case is harmless. + rmSync(dbPath, { force: true }); + + const object = await store.get(storeKey); + if (object === null) { + // HEAD succeeded but GET raced a delete between the two calls — treat as + // no-snapshot rather than throwing, since the outcome the caller cares about + // (nothing to query) is identical to the head === null branch above. + // Reset cachedEtag so the next call retries cleanly rather than leaving + // (cachedEtag=oldEtag, db=undefined), an invalid state per spec §4.3.1. + state.cachedEtag = null; + return { snapshotVersion: null, sources: [], recentNotifications: [] }; + } + writeFileSync(dbPath, object.body); - state.cachedEtag = object.etag; + // Assign cachedEtag only after openReadOnlyDatabase succeeds — if open throws, + // cachedEtag stays at its prior value (or null) and the next call retries cleanly + // instead of leaving (cachedEtag=newEtag, db=undefined), an invalid state per + // spec §4.3.1. state.db = openReadOnlyDatabase(dbPath); + state.cachedEtag = object.etag; + } ``` with: ```typescript + if (!cacheHit) { + // `better-sqlite3` keeps a page cache in memory; if the file on disk changes + // underneath an open handle, the cache describes a file that no longer exists — + // silently wrong answers, no error. Close before overwriting (spec §4.3). + // Clear both fields together so a partial failure (rmSync, store.get, write, + // openReadOnlyDatabase throwing) leaves the cache in the empty-cache state + // `(null, undefined)` rather than the invalid `(oldEtag, undefined)`. Only the + // successful open at the end of this branch restores the populated state. + if (state.db !== undefined) { + state.db.close(); + } + state.db = undefined; + state.cachedEtag = null; + // `force: true` makes the delete robust against the file disappearing between the + // existsSync check and the rmSync call — `/tmp` is shared with the writer, and + // `/tmp` cleanup can race us too. With `force` the no-op case is harmless. + rmSync(dbPath, { force: true }); + + const object = await store.get(storeKey); + if (object === null) { + // HEAD succeeded but GET raced a delete between the two calls — treat as + // no-snapshot rather than throwing, since the outcome the caller cares about + // (nothing to query) is identical to the head === null branch above. cachedEtag + // was already cleared at the top of this branch, so the next call retries the + // full cache-miss path from a known-empty state (spec §4.3.1). + return { snapshotVersion: null, sources: [], recentNotifications: [] }; + } + writeFileSync(dbPath, object.body); - // Assign cachedEtag only after openReadOnlyDatabase succeeds — if open throws, - // cachedEtag stays at its prior value (or null) and the next call retries cleanly - // instead of leaving (cachedEtag=newEtag, db=undefined), an invalid state per - // spec §4.3.1. + // Assign cachedEtag only after openReadOnlyDatabase succeeds — any throw between + // here and the top of the branch (rmSync, store.get, writeFileSync, open) leaves + // the cache in `(null, undefined)` per spec §4.3.1. state.db = openReadOnlyDatabase(dbPath); state.cachedEtag = object.etag; + } ``` - [ ] **Step 2: Verify the type-check passes** @@ -700,7 +758,7 @@ with: - [ ] **Step 5: Verify no remaining `aws-cloud-agent` references in non-plan files** -Run: `grep -rn "aws-cloud-agent\|core-llm-wiki" --exclude-dir=node_modules --exclude-dir=.git --exclude="*-plan*.md" .` +Run: `grep -rn "aws-cloud-agent\|core-llm-wiki" --exclude-dir=node_modules --exclude-dir=.git --exclude-dir=docs/superpowers/plans .` Expected: no output. --- @@ -751,20 +809,32 @@ discussion. ## Step 4: Configure the tutorial -Set the URL as the `DISCORD_WEBHOOK_URL` environment variable when running locally: +Export the URL as the `DISCORD_WEBHOOK_URL` environment variable before running +locally — the value stays out of shell history if you set it in your shell profile +or load it from an untracked `.env` file: ```bash -DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run local-fetch +# Local development — load from an untracked .env (gitignored), then export: +export $(cat .env | xargs) && npm run local-fetch ``` -When deploying via CDK, pass the URL at deploy time: +When deploying via CDK, source the URL from an untracked file or a CI secret store +rather than echoing it on the command line. CDK reads `DISCORD_WEBHOOK_URL` at +synth time (see `infra/stack.ts:55-63`) and embeds it in the Lambda environment, so +the value should never appear in a published transcript: ```bash -DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run deploy +# CI / local deploy — source from a secret store or masked CI variable, then deploy: +set -a; . ./.env.discord; set +a # .env.discord is gitignored +npm run deploy ``` -The URL is exposed to the Lambda as a regular environment variable. The Lambda's IAM -role does not need any Discord permissions — the webhook URL is the only credential. +For production deployments, prefer SSM Parameter Store or Secrets Manager over an +inline Lambda environment value — `cdk.out/` and CloudFormation templates can echo +the value, and any operator with `logs:GetLogEvents` on the function can read it +back from cold-start records. Never commit `.env`, `cdk.out/`, or logs that +contain the webhook URL. The Lambda's IAM role does not need any Discord +permissions — the webhook URL is the only credential. ## Step 5: Verify @@ -777,9 +847,10 @@ per-source failures including Discord post failures. If the webhook URL is compromised (e.g. accidentally logged, pasted into a public forum), the recovery is to delete the compromised webhook in the same **Integrations** -panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. There is no -rate-limit concern with this — webhooks are a "delete-and-recreate" credential, not a -rotating key. +panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. Webhook +executions remain subject to Discord's normal rate limits and can return HTTP 429; +the `fetch` op should honour the `Retry-After` response header (and the +`X-RateLimit-*` family of headers) rather than retrying on a fixed cadence. ``` - [ ] **Step 2: Update `README.md` to link to the new doc** @@ -827,10 +898,11 @@ deployed stack). Run: `git diff --stat` Expected: changes limited to `src/agent/status.ts`, `tests/status.test.ts`, -`docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md`, `docs/01-architecture.md`, -`docs/05-from-tutorial-to-prod.md`, `docs/06-discord-webhook-setup.md` (new), -`docs/bedrock-model-comparison.md`, `src/format/families.ts`, `README.md`, -`docs/02-rehydration.md`. +`docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md`, +`docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md`, +`docs/01-architecture.md`, `docs/05-from-tutorial-to-prod.md`, +`docs/06-discord-webhook-setup.md` (new), `docs/bedrock-model-comparison.md`, +`src/format/families.ts`, `README.md`, `docs/02-rehydration.md`. --- diff --git a/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md b/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md index 5b9f992..72dddb7 100644 --- a/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md +++ b/docs/superpowers/specs/2026-08-08-sqlite-s3-agent-tutorial-design.md @@ -169,10 +169,10 @@ The reader's `ReaderState` has two fields — `cachedEtag` and `db` — and exac - `(cachedEtag: , db: )` — the cache holds a valid snapshot. - `(cachedEtag: null, db: undefined)` — the cache is empty. -Any other combination is a bug. The cache-miss branch must preserve this invariant across every failure mode: +Any other combination is a bug. The cache-miss branch transitions to `(null, undefined)` at entry (when it closes the prior open handle) and only restores `(string, open handle)` after `openReadOnlyDatabase` succeeds. This makes the invariant hold across every failure mode: -- **`openReadOnlyDatabase` throws** (corrupted snapshot, non-SQLite bytes, disk error): `state.db` stays `undefined`, `state.cachedEtag` stays at its prior value (or `null` on a cold start). The error propagates up — the Lambda returns 500, the next call retries the cache-miss path from scratch. -- **`GetObject` returns `null` after a successful `HEAD`** (transient S3 race, object deleted between calls): `state.cachedEtag` is reset to `null`, the empty-state JSON is returned. The next call retries cleanly. +- **`openReadOnlyDatabase` throws** (corrupted snapshot, non-SQLite bytes, disk error), or **`rmSync` / `writeFileSync` / `store.get` throws**: the cache was already cleared at the top of the branch, so `state.db` is `undefined` and `state.cachedEtag` is `null`. The error propagates up — the Lambda returns 500, the next call retries the cache-miss path from a known-empty state. +- **`GetObject` returns `null` after a successful `HEAD`** (transient S3 race, object deleted between calls): the cache was already cleared at the top of the branch, so the empty-state JSON is returned and the next call retries cleanly. - **`GetObject` returns `NoSuchKey`** (no snapshot yet — `fetch` has never run): identical to the `null` case above. The retry loop on a permanently broken S3 object is unavoidable — it terminates when the operator or the writer fixes the underlying object. The invariant is what prevents the loop from behaving incorrectly (e.g., returning stale data from a closed handle) while it runs. diff --git a/src/agent/status.ts b/src/agent/status.ts index fb01447..a69686d 100644 --- a/src/agent/status.ts +++ b/src/agent/status.ts @@ -34,6 +34,12 @@ interface ReaderState { export interface StatusReader { getStatus(store: Store, storeKey: string): Promise; + /** + * Test-only: returns a snapshot of the internal reader state for assertions on the + * cache invariant (spec §4.3.1). Production code must not rely on this — it exposes + * implementation detail that the public contract deliberately does not promise. + */ + __peekReaderState(): { cachedEtag: string | null; dbIsOpen: boolean }; } function queryStatus(db: Database.Database, etag: string): StatusResult { @@ -98,10 +104,15 @@ export function createStatusReader(dbPath: string): StatusReader { // `better-sqlite3` keeps a page cache in memory; if the file on disk changes // underneath an open handle, the cache describes a file that no longer exists — // silently wrong answers, no error. Close before overwriting (spec §4.3). + // Clear both fields together so a partial failure (rmSync, store.get, write, + // openReadOnlyDatabase throwing) leaves the cache in the empty-cache state + // `(null, undefined)` rather than the invalid `(oldEtag, undefined)`. Only the + // successful open at the end of this branch restores the populated state. if (state.db !== undefined) { state.db.close(); - state.db = undefined; } + state.db = undefined; + state.cachedEtag = null; // `force: true` makes the delete robust against the file disappearing between the // existsSync check and the rmSync call — `/tmp` is shared with the writer, and // `/tmp` cleanup can race us too. With `force` the no-op case is harmless. @@ -111,23 +122,24 @@ export function createStatusReader(dbPath: string): StatusReader { if (object === null) { // HEAD succeeded but GET raced a delete between the two calls — treat as // no-snapshot rather than throwing, since the outcome the caller cares about - // (nothing to query) is identical to the head === null branch above. - // Reset cachedEtag so the next call retries cleanly rather than leaving - // (cachedEtag=oldEtag, db=undefined), an invalid state per spec §4.3.1. - state.cachedEtag = null; + // (nothing to query) is identical to the head === null branch above. cachedEtag + // was already cleared at the top of this branch, so the next call retries the + // full cache-miss path from a known-empty state (spec §4.3.1). return { snapshotVersion: null, sources: [], recentNotifications: [] }; } writeFileSync(dbPath, object.body); - // Assign cachedEtag only after openReadOnlyDatabase succeeds — if open throws, - // cachedEtag stays at its prior value (or null) and the next call retries cleanly - // instead of leaving (cachedEtag=newEtag, db=undefined), an invalid state per - // spec §4.3.1. + // Assign cachedEtag only after openReadOnlyDatabase succeeds — any throw between + // here and the top of the branch (rmSync, store.get, writeFileSync, open) leaves + // the cache in `(null, undefined)` per spec §4.3.1. state.db = openReadOnlyDatabase(dbPath); state.cachedEtag = object.etag; } return queryStatus(state.db as Database.Database, state.cachedEtag as string); }, + __peekReaderState() { + return { cachedEtag: state.cachedEtag, dbIsOpen: state.db !== undefined }; + }, }; } \ No newline at end of file diff --git a/tests/status.test.ts b/tests/status.test.ts index ff0260e..d68463d 100644 --- a/tests/status.test.ts +++ b/tests/status.test.ts @@ -114,24 +114,24 @@ describe('createStatusReader', () => { ]); }); - it('propagates openReadOnlyDatabase failures and recovers when the snapshot becomes valid', async () => { - // An open failure on a corrupted snapshot must: - // (a) propagate as a rejection (the Lambda returns 500), - // (b) not poison the reader — once the underlying S3 object is fixed, the next - // call downloads valid bytes and returns the seeded data. - let corrupt = true; + it('clears cache state when store.get throws in the cache-miss path and recovers', async () => { + // The cache-miss branch in `src/agent/status.ts` must preserve the spec §4.3.1 + // invariant: after any throwing step inside the branch, the cache is left at + // `(cachedEtag: null, db: undefined)`, NOT `(cachedEtag: , db: undefined)`. + // The pre-fix implementation cleared `db` at the top of the branch but left + // `cachedEtag` at the prior warm-cache value, which violates the invariant as + // soon as `store.get` (or `writeFileSync`, or `openReadOnlyDatabase`) throws. let validBytes: Buffer = Buffer.alloc(0); + let nextEtag = 'warm-etag'; + let getThrows = false; const adaptiveStore = { ...ctx.store, async get(key: string) { - if (corrupt) { - // Bytes that aren't a valid SQLite file — openReadOnlyDatabase will throw. - return { body: Buffer.from('not a sqlite file'), etag: 'corrupt-etag' }; - } - return { body: validBytes, etag: 'fixed-etag' }; + if (getThrows) throw new Error('s3 transient'); + return { body: validBytes, etag: nextEtag }; }, async head(key: string) { - return { etag: corrupt ? 'corrupt-etag' : 'fixed-etag' }; + return { etag: nextEtag }; }, }; @@ -142,40 +142,53 @@ describe('createStatusReader', () => { const readerDbPath = join(ctx.dir, 'reader-copy.db'); const reader = createStatusReader(readerDbPath); - // First call: open throws on the corrupted bytes. - await expect(reader.getStatus(adaptiveStore, 'memory.db')).rejects.toThrow(); + // Warm the cache so the failing call is a warm-cache-miss, not a cold start. + // Without warming, cachedEtag is already null on the failing call and the + // invariant assertion below would pass trivially on the buggy implementation. + await reader.getStatus(adaptiveStore, 'memory.db'); + expect(reader.__peekReaderState()).toEqual({ cachedEtag: 'warm-etag', dbIsOpen: true }); - // Snapshot becomes valid (operator or writer fixes the S3 object). - corrupt = false; + // Force a cache-miss (different ETag) and have store.get throw. The fix clears + // both fields at the top of the branch, so a throw anywhere inside leaves the + // cache in the empty state. The pre-fix left cachedEtag at 'warm-etag'. + nextEtag = 'raced-etag'; + getThrows = true; + + await expect(reader.getStatus(adaptiveStore, 'memory.db')).rejects.toThrow('s3 transient'); + // The invariant: cache cleared even though openReadOnlyDatabase was never reached. + // The pre-fix implementation left `(cachedEtag: 'warm-etag', db: undefined)` here. + expect(reader.__peekReaderState()).toEqual({ cachedEtag: null, dbIsOpen: false }); + + // Network recovers — the next call retries the cache-miss path from a clean state. + getThrows = false; - // Second call: cache miss, downloads valid bytes, opens successfully, returns - // the seeded data. Without the fix, state would be (cachedEtag='corrupt-etag', - // db=undefined) — invalid per spec §4.3.1 — but the test would still pass because - // the cacheHit check fails for the same reason in both versions. The test documents - // the recovery behavior; the invariant is enforced by code review. const recovered = await reader.getStatus(adaptiveStore, 'memory.db'); - expect(recovered.snapshotVersion).toBe('fixed-etag'); + expect(recovered.snapshotVersion).toBe('raced-etag'); expect(recovered.sources).toEqual([ { name: 'weather', lastValue: '72F', lastFetchedAt: 1000, lastPostedAt: 1000 }, ]); + expect(reader.__peekReaderState()).toEqual({ cachedEtag: 'raced-etag', dbIsOpen: true }); }); - it('returns empty state and recovers on the next call when HEAD succeeds but GET returns null', async () => { + it('clears cache state on HEAD-succeeds-GET-null and recovers on the next call', async () => { // The HEAD-succeeds-GET-fails branch (object deleted between HEAD and GET) must: // (a) return the empty-state JSON (treating it as no-snapshot, not throwing), - // (b) reset cachedEtag so the next call retries cleanly, + // (b) clear the cache to `(cachedEtag: null, db: undefined)` so the next call sees + // a clean cache-miss — NOT leave `(cachedEtag: , db: undefined)` if the + // reader had a warm cache before the race (spec §4.3.1), // (c) once the underlying object exists, return the seeded data without being // stuck in a HEAD→GET→empty cycle. - let objectExists = false; + let objectExists = true; let validBytes: Buffer = Buffer.alloc(0); + let nextEtag = 'warm-etag'; const adaptiveStore = { ...ctx.store, async get(key: string) { if (!objectExists) return null; // HEAD-succeeds-GET-fails - return { body: validBytes, etag: 'fixed-etag' }; + return { body: validBytes, etag: nextEtag }; }, async head(key: string) { - return { etag: 'fixed-etag' }; + return { etag: nextEtag }; }, }; @@ -185,16 +198,29 @@ describe('createStatusReader', () => { const readerDbPath = join(ctx.dir, 'reader-copy.db'); const reader = createStatusReader(readerDbPath); - // First call: HEAD says the object exists, GET says it doesn't. Empty state. + // Warm the cache so the failing call is a warm-cache-miss. Without this, + // cachedEtag is already null on the failing call and the invariant assertion + // below passes trivially on the buggy implementation. + await reader.getStatus(adaptiveStore, 'memory.db'); + expect(reader.__peekReaderState()).toEqual({ cachedEtag: 'warm-etag', dbIsOpen: true }); + + // HEAD succeeds for a different ETag (object was replaced between warm-up and + // now) but GET returns null (race condition — object deleted between calls). + objectExists = false; + nextEtag = 'raced-etag'; + const first = await reader.getStatus(adaptiveStore, 'memory.db'); expect(first).toEqual({ snapshotVersion: null, sources: [], recentNotifications: [] }); + // The invariant: after GET-null, the cache must be back to `(null, undefined)`. + // The pre-fix implementation left it at `(cachedEtag: 'raced-etag', db: undefined)` + // — an invalid state per spec §4.3.1 — and this assertion catches it. + expect(reader.__peekReaderState()).toEqual({ cachedEtag: null, dbIsOpen: false }); + // Object materializes (operator creates it out-of-band, or writer runs). objectExists = true; + nextEtag = 'fixed-etag'; - // Second call: HEAD still says fixed-etag, GET returns valid bytes, returns data. - // The fix resets cachedEtag to null on the first call, so the second call sees - // (cachedEtag=null, db=undefined) — cache miss, fresh GET, success. const recovered = await reader.getStatus(adaptiveStore, 'memory.db'); expect(recovered.snapshotVersion).toBe('fixed-etag'); expect(recovered.sources).toEqual([ From 11e8f40bc694ff48b25b37c3df56821a69628f76 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:49:29 -0400 Subject: [PATCH 5/6] docs(pr4): address CodeRabbit review on webhook setup doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - .gitignore: add .env.discord so the deployment secret file is untracked - docs/06-discord-webhook-setup.md: replace the incorrect logs:GetLogEvents cold-start claim with the real exposure path (lambda:GetFunctionConfiguration + CloudFormation templates + cdk.out/) and call out CloudWatch log access as a separate concern that only matters if application code echoes the URL - docs/06-discord-webhook-setup.md: align the rate-limit guidance with src/discord/poster.ts — 429 throws DiscordPostError and is recorded as a per-source failure in agent_runs.error unless bounded 429 handling is added Co-Authored-By: Claude --- .gitignore | 1 + docs/06-discord-webhook-setup.md | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 9816b27..2938792 100644 --- a/.gitignore +++ b/.gitignore @@ -4,4 +4,5 @@ dist/ *.db-journal cdk.out/ .env +.env.discord .codegraph/ diff --git a/docs/06-discord-webhook-setup.md b/docs/06-discord-webhook-setup.md index ba8879c..30537b0 100644 --- a/docs/06-discord-webhook-setup.md +++ b/docs/06-discord-webhook-setup.md @@ -66,9 +66,12 @@ npm run deploy For production deployments, prefer **SSM Parameter Store** or **Secrets Manager** over an inline Lambda environment value — `cdk.out/` and CloudFormation templates -echo environment values, and any operator with `logs:GetLogEvents` can read them -back from cold-start records. Never commit `.env`, `cdk.out/`, or logs that -contain the webhook URL. +echo environment values, and any operator with `lambda:GetFunctionConfiguration` +(or equivalent read access to the function's configuration) can read the same +value back. CloudWatch log access (`logs:GetLogEvents`) is a separate concern: +Lambda does not log environment variables by default, but any code that prints +or otherwise echoes `DISCORD_WEBHOOK_URL` will surface it in the log stream. +Never commit `.env`, `cdk.out/`, or logs that contain the webhook URL. The URL is the only credential the Lambda needs — its IAM role does not require any Discord permissions. @@ -87,6 +90,9 @@ forum), the recovery is to delete the compromised webhook in the same **Integrat panel and create a new one. Update `DISCORD_WEBHOOK_URL` and redeploy. Webhook executions remain subject to Discord's normal rate limits and can return -HTTP 429. The `fetch` op should honour the `Retry-After` response header (and the -`X-RateLimit-*` family) rather than retrying on a fixed cadence — Discord does not -publish the exact limits and they vary by channel and account. +HTTP 429. The current poster treats 429 the same as any other non-2xx after its +single fixed 250 ms 5xx retry — it throws `DiscordPostError` and the run records +a per-source failure in `agent_runs.error`. Discord does not publish the exact +limits and they vary by channel and account; if bounded 429 handling is needed, +honour the `Retry-After` response header (and the `X-RateLimit-*` family) rather +than retrying on a fixed cadence. From 6170ce73da66a04f7b4187acb8f8ac7bbf1ccb16 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:58:34 -0400 Subject: [PATCH 6/6] fix(pr4): address remaining Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README.md: source DISCORD_WEBHOOK_URL from .env in the quick start rather than embedding the URL in the shell command (shell history leak). - plan: fix proposed test code that referenced seeded.body — seedSnapshot returns store.put() which is { etag }, not { body, etag }. Co-Authored-By: Claude --- README.md | 6 +++++- ...-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md | 8 +++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 44bfe12..e2f033b 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,11 @@ in S3. No database server, no VPC. ```bash npm install npm test -DISCORD_WEBHOOK_URL="https://discord.com/api/webhooks/..." npm run local-fetch + +# Put your webhook URL in an untracked .env (see docs/06-discord-webhook-setup.md), +# then source it and run the writer — keeping the URL out of shell history. +set -a; . ./.env; set +a # .env is gitignored +npm run local-fetch ``` That runs the writer against a local SQLite file with no AWS involved (Phase 1). To diff --git a/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md b/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md index 13f25f5..646c8e4 100644 --- a/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md +++ b/docs/superpowers/plans/2026-08-08-sqlite-s3-agent-tutorial-pr4-reader-bugfixes.md @@ -217,9 +217,11 @@ Append to `tests/status.test.ts`: }, }; - // Seed a valid snapshot so we have bytes to recover with. - const seeded = await seedSnapshot(ctx.dbPath, ctx.store); - validBytes = seeded.body; + // Seed a valid snapshot so we have bytes to recover with. `seedSnapshot` + // returns `store.put(...)`, which is `{ etag: string }` — read the seeded + // file off disk to get the bytes. + await seedSnapshot(ctx.dbPath, ctx.store); + validBytes = readFileSync(ctx.dbPath); const readerDbPath = join(ctx.dir, 'reader-copy.db'); const reader = createStatusReader(readerDbPath);