feat: loop mode + poetic closing (5-min EventBridge loop, haiku, "Reminds me of" suffix) - #7
Conversation
Replaces the once-daily schedule with a 3-minute loop, makes the message more varied (date + location + weather + crypto + closest-match context, ending with a haiku), and adds curl commands to start and stop the loop. One writer, dedup made optional via env var.
- Clarify MessageFormatter signature change is internal (not a public surface) - Correct loop-start/loop-stop publish-failure semantics: change is lost on conditional-write failure (same posture as runFetch), not retried by the next tick which hydrates from S3 - Make the FETCH_DEDUP=true regression test scope explicit
- Drop the FETCH_DEDUP env var and the dedup code path in runFetch. The writer always posts; the loop is short-lived and the toggle is YAGNI complexity for a tutorial demo. - Drop the agent_settings table. EventBridge rule state is now the source of truth for 'is the loop running'. - loop-start calls EventBridge.EnableRuleCommand; loop-stop calls DisableRuleCommand. A stopped loop actually stops the recurring AWS invocations, not just makes the Lambda a no-op. - Add LOOP_RULE_NAME env var (set by CDK at synth) and events:EnableRule / events:DisableRule IAM on the rule ARN. - Status endpoint unchanged (no loopEnabled field). - Scope: no RAG corpus cap; loop is short-lived in practice.
The two env vars travel together — the CDK stack sets both or neither. Add a single cross-field check in loadConfig so a half-configured deploy fails at startup rather than at the first loop-start/loop-stop call.
- Restructure runFetch: fetch all sources up front, run per-source RAG lookups, format ONCE with a combined LoopContext, post ONCE, then write per-source agent_notifications rows (each carrying the same combined formatted_message). One Discord message per tick. - LoopContext replaces the per-source FormatContext; carries weatherValue + cryptoValue + per-source similarPast pairs. The SimilarPastResult interface gains notificationId and distance so the writer can populate the per-source nearest_match columns without a second RAG lookup. - LOOP_TOKEN is auto-generated at synth time (crypto.randomBytes) and exposed as a CloudFormation output. The start/stop scripts read FUNCTION_URL and LOOP_TOKEN from stack outputs, so the user just runs the script — no secret to copy. - Budget note updated: ~480 Bedrock calls/day (1 post per tick instead of 2 per source). - Open concerns section notes the per-source RAG behavior when one source fails.
The 3-minute window is too tight against Bedrock + Titan + S3 + Discord latency. 5 minutes gives comfortable headroom and halves the Bedrock cost (288 ticks/day vs 480) and the RAG corpus growth (576 rows/day vs 960). All references throughout the spec updated.
The previous design stored the full posted message (with 'Reminds me of'
suffix) as the RAG match's text, and the corpus embed was on that same
posted text. Each tick's suffix therefore included the previous tick's
full posted text recursively, growing past Discord's 2000-char limit
in ~13 ticks. A Discord 400 then skipped the inserts, the loop jammed
permanently, and the same oversized past row was retrieved every 5 min.
Fix: add a new 'base_message' column on agent_notifications holding
the LLM's pre-suffix output. The formatted_message column keeps the
full posted message (with optional suffix). The RAG embed is on
base_message, and findNearestMatch returns the past tick's
base_message for the suffix, never its formatted_message. The posted
message is bounded at one base + one suffix (~300 chars), no chain
growth, no jamming.
- New ALTER TABLE migration via the existing PRAGMA table_info guard
in bootstrap() (mirrors the RAG spec's column-addition pattern).
- findNearestMatch return shape changes from {formattedMessage} to
{baseMessage}; called out in section 4.5.
- New 'snowball regression test' in section 7: 20 simulated ticks
with every finalMessage.length < 500.
- Section 9 'Reminds me of message length' concern resolved (was the
open concern; now the bounded-length property is by construction).
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change replaces daily fetches with five-minute loop execution. Each tick combines successful weather and crypto readings, generates one message with optional global RAG context, posts once, and stores per-source notification and embedding records. ChangesLoop mode
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EventBridge
participant runHandler
participant runFetch
participant BedrockFormatter
participant DiscordPoster
participant SQLiteDatabase
EventBridge->>runHandler: invoke fetch target every five minutes
runHandler->>runFetch: pass weatherLocation and sources
runFetch->>BedrockFormatter: format combined LoopContext
runFetch->>SQLiteDatabase: find global nearest match and store embeddings
runFetch->>DiscordPoster: post one combined message
runFetch->>SQLiteDatabase: persist per-source notifications
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/format.test.ts (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the helper return value as
LoopContext.The helper builds a structural match for
LoopContextbut is not annotated. An added or renamed field insrc/format/types.tswould not fail this test file. Annotate the return type so the tests track the interface.♻️ Proposed annotation
+import type { LoopContext } from '../src/format/types.js'; + -const ctx = (readings: Array<{ source: 'weather' | 'crypto'; value: string }>, date = '2026-08-09', location = 'NYC') => ({ +const ctx = ( + readings: Array<{ source: 'weather' | 'crypto'; value: string }>, + date = '2026-08-09', + location = 'NYC', +): LoopContext => ({ date, location, readings, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/format.test.ts` around lines 5 - 9, Annotate the return value of the ctx helper as LoopContext so its structural object is checked against the interface defined in the format types. Keep the existing parameters and returned fields unchanged.tests/db.test.ts (1)
237-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert that the legacy row survives the second bootstrap.
The comment says the re-run "must not alter the column", but the test only counts columns. Add a check that the
NULLbase_messagerow is still present and stillNULL. That pins the migration's data-preservation behavior, not only the schema shape.♻️ Proposed additional assertion
const colsAfter = db.prepare(`PRAGMA table_info(agent_notifications)`).all() as Array<{ name: string }>; expect(colsAfter.filter((c) => c.name === 'base_message')).toHaveLength(1); + const legacy = db + .prepare(`SELECT base_message FROM agent_notifications`) + .all() as Array<{ base_message: string | null }>; + expect(legacy).toHaveLength(1); + expect(legacy[0]?.base_message).toBeNull();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/db.test.ts` around lines 237 - 240, Extend the second-bootstrap assertions in the test around bootstrap(db) to query the existing legacy row and verify it remains present with a NULL base_message value. Keep the current single-column assertion, and ensure the added check validates data preservation rather than only schema shape.tests/similarity.test.ts (1)
91-111: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the KNN-window behavior for legacy rows.
Add a test with
KNN_CANDIDATES + 1NULLrows closer than one valid row. Assert the current post-filter behavior (findNearestMatchreturnsnull), or change the query if valid rows must remain discoverable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/similarity.test.ts` around lines 91 - 111, Update the similarity tests around findNearestMatch to add KNN_CANDIDATES + 1 legacy notifications with NULL base_message values closer than a valid notification, then assert the intended post-filter behavior: findNearestMatch returns null. If the intended contract is to keep valid rows discoverable, instead adjust the query implementation and assert that the valid notification is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/07-budget-protection.md`:
- Around line 24-31: Update the document’s introductory count and the concluding
reference from “three” to “four” to reflect the added fourth cost driver, while
leaving the surrounding explanations unchanged.
In `@docs/superpowers/specs/2026-08-09-loop-mode-poetry-design.md`:
- Around line 191-198: Update the schedule instruction in section 4.7 to use
cdk.Duration.minutes(5) instead of cdk.Duration.minutes(3), while leaving the
other infrastructure changes unchanged.
In `@README.md`:
- Around line 45-50: Update the opening description in the “Loop mode” section
to state that the default EventBridge schedule runs every 5 minutes, rather than
describing a once-daily schedule. Clarify that loop-start is only needed after
loop-stop, while preserving the existing deployment and shell-toggle
instructions.
In `@src/agent/fetch.ts`:
- Around line 168-175: Update the final-message construction in the fetch flow
around finalMessage and match.baseMessage to enforce Discord’s 2,000-character
limit before params.poster.post is called. Preserve the full preMessage when it
fits, and include the “Reminds me of” suffix only when it fits; otherwise omit
or truncate the suffix so the posted message never exceeds 2,000 characters.
Ensure match.baseMessage cannot cause an oversized finalMessage.
In `@src/handler.ts`:
- Line 188: Update runHandler to accept loop-start and loop-stop alongside fetch
and status, validate the LOOP_TOKEN for those operations, and invoke the
corresponding EventBridge rule enable/disable commands before entering the fetch
path. Preserve the existing fetch and status behavior.
In `@tests/fetch.test.ts`:
- Around line 111-117: Update the explanatory comment beside the r2Ctx date
assertion in the fetch test to state that the configured now callback returns
2000, which is interpreted as epoch milliseconds and therefore produces
1970-01-01; do not describe the clock as unoverridden.
In `@tests/handler.test.ts`:
- Around line 230-258: Update the handler test around runHandler to stub the
outbound Titan InvokeModelCommand and Discord fetch calls in addition to
ConverseCommand. Configure successful responses for both stubs, then assert each
expected call was made so the test cannot pass after an unstubbed outbound
failure is swallowed by runFetch.
---
Nitpick comments:
In `@tests/db.test.ts`:
- Around line 237-240: Extend the second-bootstrap assertions in the test around
bootstrap(db) to query the existing legacy row and verify it remains present
with a NULL base_message value. Keep the current single-column assertion, and
ensure the added check validates data preservation rather than only schema
shape.
In `@tests/format.test.ts`:
- Around line 5-9: Annotate the return value of the ctx helper as LoopContext so
its structural object is checked against the interface defined in the format
types. Keep the existing parameters and returned fields unchanged.
In `@tests/similarity.test.ts`:
- Around line 91-111: Update the similarity tests around findNearestMatch to add
KNN_CANDIDATES + 1 legacy notifications with NULL base_message values closer
than a valid notification, then assert the intended post-filter behavior:
findNearestMatch returns null. If the intended contract is to keep valid rows
discoverable, instead adjust the query implementation and assert that the valid
notification is returned.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa3adafd-5dcf-4d7c-a812-fb3f7bef7687
📒 Files selected for processing (21)
README.mddocs/07-budget-protection.mddocs/superpowers/specs/2026-08-09-loop-mode-poetry-design.mdinfra/stack.tspackage.jsonscripts/loop-start.shscripts/loop-stop.shsrc/agent/fetch.tssrc/db/bootstrap.tssrc/format/bedrock.tssrc/format/local.tssrc/format/types.tssrc/handler.tssrc/localFetch.tssrc/rag/similarity.tstests/bedrock.test.tstests/db.test.tstests/fetch.test.tstests/format.test.tstests/handler.test.tstests/similarity.test.ts
- Enforce Discord 2000-char limit in fetch.ts (post: buildFinalMessageForDiscord helper; preserves preMessage, clips suffix with '...' when it doesn't fit, drops the suffix when there's not even room for a clip marker). Adds six unit tests covering the boundary cases. - README: align Loop mode opening with the 5-min default schedule; clarify loop-start is only needed after loop-stop. - docs/07-budget-protection.md: 'three' -> 'four' for cost-driver count. - tests/db.test.ts: assert the legacy row survives the second bootstrap (data preservation, not only schema shape). - tests/format.test.ts: annotate the ctx helper return value as LoopContext. - tests/handler.test.ts: stub InvokeModelCommand (Titan) and global fetch (Discord) in beforeEach; the two-source test now asserts both outbound calls happen, so runFetch cannot silently pass after a swallowed failure. - tests/similarity.test.ts: pin KNN-window behavior for legacy rows (51 null rows + 1 far valid row -> findNearestMatch returns null). - tests/fetch.test.ts: correct the 'epoch date' comment to reflect that now: () => 2000 is 2000ms after the epoch. Skipped: CodeRabbit suggested adding loop-start/loop-stop operations to runHandler. The scripts call 'aws events enable-rule' / 'aws events disable-rule' directly and never invoke the Lambda (see PR body), so the HTTP-path addition would not be wired to anything and would add a token / IAM surface the spec deliberately omits.
/fix-pr follow-upCommit: Review resolution
Verification
|
There was a problem hiding this comment.
Pull request overview
This PR implements “loop mode + poetic closing” for the sqlite-s3 agent tutorial: it shifts the deployed fetch cadence to a 5‑minute EventBridge loop, changes the writer to emit one combined Discord message per tick (with haiku), and adds a snowball-safe “Reminds me of” suffix driven by a global RAG KNN match over a new base_message column.
Changes:
- Replace the once-daily EventBridge schedule with
rate(5 minutes)and add start/stop scripts that enable/disable the deployed rule via AWS CLI. - Restructure
runFetchto fetch-all → format-once (LoopContext) → embed-once → global KNN → append mechanical suffix → post-once, while persisting per-source rows and embeddings without suffix snowball. - Update schema/RAG/formatters/tests/docs to support
base_message, global matching, and the new combined-message prompt contract.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/similarity.test.ts | Updates similarity tests for global KNN + baseMessage behavior and NULL filtering. |
| tests/handler.test.ts | Adds multi-source “one tick → one formatter/post” assertions and stubs Titan/embed + Discord post. |
| tests/format.test.ts | Updates local formatter tests to use LoopContext. |
| tests/fetch.test.ts | Rewrites writer tests for combined-message flow, suffix bounding, and snowball regression. |
| tests/db.test.ts | Verifies base_message column migration is added and idempotent. |
| tests/bedrock.test.ts | Updates Bedrock formatter tests for haiku prompt + LoopContext payload. |
| src/rag/similarity.ts | Switches nearest-match query to global KNN and returns baseMessage (filters NULL base_message). |
| src/localFetch.ts | Threads weatherLocation into runFetch. |
| src/handler.ts | Threads weatherLocation into runFetch in the Lambda handler path. |
| src/format/types.ts | Introduces LoopContext/LoopReading and changes MessageFormatter.format signature. |
| src/format/local.ts | Updates deterministic local formatter to emit a combined message from LoopContext. |
| src/format/bedrock.ts | Updates system/user prompts for friendly comment + haiku based on LoopContext. |
| src/db/bootstrap.ts | Adds PRAGMA-guarded base_message column migration to agent_notifications. |
| src/agent/fetch.ts | Implements the combined-message writer, two-step RAG, suffix bounding helper, and per-source persistence. |
| scripts/loop-stop.sh | Adds script to disable the deployed EventBridge rule via AWS CLI. |
| scripts/loop-start.sh | Adds script to enable the deployed EventBridge rule via AWS CLI. |
| README.md | Documents loop mode usage and redeploy caveat. |
| package.json | Exposes npm run loop-start / npm run loop-stop. |
| infra/stack.ts | Changes schedule to 5 minutes, increases Lambda timeout, and outputs LoopRuleName. |
| docs/superpowers/specs/2026-08-09-loop-mode-poetry-design.md | Adds design spec describing loop mode, combined message flow, and snowball prevention. |
| docs/superpowers/plans/2026-08-09-loop-mode-poetry.md | Adds implementation plan for the feature set. |
| docs/07-budget-protection.md | Adds loop-mode cost/growth note for budget protection. |
| docs/02-rehydration.md | Documents /tmp storage ceiling considerations for long-running loop usage. |
Suppressed comments (1)
src/agent/fetch.ts:242
db.close()is called before delegating topublish(), butpublish()also closes the database. Remove the extra close so the failure path doesn't depend onclose()being idempotent.
db.close();
return publish(db, params, priorEtag, runId, now, 0, errors);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- fetch.ts: drop redundant db.close() before delegating to publish() in the formatter-failure and post-failure early-return paths — publish() owns the close. - fetch.ts: rewrite publish() JSDoc to describe the actual lifecycle (closes passed-in DB; reopens only on the error path) and document the caller invariant (caller must not close first). - bootstrap.ts: fix migration comment that referenced a 'LIKE exclusion' — the real filter is WHERE n.base_message IS NOT NULL. - handler.test.ts: tighten the InvokeModelCommand assertion to toHaveLength(1) so a per-source embed regression fails loudly. - docs/02-rehydration.md: clarify that ENOSPC happens on the local writeFileSync of the hydrated snapshot, not inside s3.GetObject.
/fix-pr follow-upCommit: Review resolution
Verification
|
Read-only companion to loop-start/loop-stop. Fetches LoopRuleName from stack outputs and prints the rule's State, ScheduleExpression, and Arn via aws events describe-rule. Same PROFILE/REGION/STACK_NAME conventions. README: add to the command block, document it as read-only, and point the redeploy-gotcha paragraph at it (run loop-status after a redeploy to confirm state before deciding whether to re-run loop-stop). package.json: expose as npm run loop-status. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/agent/fetch.ts:214
- If embedding succeeds but
findNearestMatchthrows,preVectoris already populated, so the later insert loop still writes embeddings despite this catch promising to skip them. Reset it here so lookup failures follow the documented all-or-nothing RAG failure path.
} catch (error: unknown) {
const message = error instanceof Error ? error.message : String(error);
errors.push(`rag: ${message}`);
// preVector stays null — step 13's embedding insert is skipped for this tick.
README.md:59
- The implementation has no similarity-distance threshold: any eligible KNN result is appended. Saying “close enough” therefore misstates the visible behavior once the corpus contains any non-null
base_message.
friendly comment drawn from today's date, weather, and crypto price, ending with a
README.md:47
- This new 5-minute default leaves the later “The daily
fetchrun” sentence at line 73 stale. Update that sentence to say “scheduled” (or “5-minute”) so the README does not describe two different cadences.
The deployed EventBridge schedule runs every 5 minutes by default. Use `loop-stop` to
docs/02-rehydration.md:83
- A fresh container does not recover from this condition: it immediately rehydrates the same oversized S3 snapshot and hits the same 512 MB ceiling. Recovery requires increasing ephemeral storage or pruning/replacing the persisted snapshot.
until a redeploy resurfaces a fresh container with an empty `/tmp`.
docs/superpowers/specs/2026-08-09-loop-mode-poetry-design.md:204
- This section contradicts the PR:
src/handler.tsnow passesconfig.weatherLocationintorunFetch. Keep the no-new-control-plane-ops statement, but document the actual fetch-path wiring.
### 4.6 `src/handler.ts` — unchanged
No changes. Loop start/stop never reaches the Lambda (§2), so there are no new ops, no `EventBridgeClient` in `InjectedClients`, and no handler-level token check to get right or wrong.
docs/superpowers/specs/2026-08-09-loop-mode-poetry-design.md:303
- This says truncation is unnecessary, but
buildFinalMessageForDiscordexplicitly truncates an oversized base output and clips or omits an oversized suffix. Since this spec is marked implemented, document the actual cap behavior rather than asserting the opposite.
- **"Reminds me of" message length.** With the `base_message` separation, the suffix is always built from the past tick's pre-suffix text (a single LLM output, ~150 chars), never from a past `formatted_message` that includes a prior "Reminds me of" line. The posted message is bounded at one base + one suffix (~300 chars typical), well under Discord's 2000-char limit. No chain growth, no truncation needed.
Implements docs/superpowers/plans/2026-08-09-loop-mode-poetry.md per docs/superpowers/specs/2026-08-09-loop-mode-poetry-design.md.
What's in this PR
Replaces the once-daily schedule with a 5-minute loop, restructures the writer (
runFetch) to one combined message per tick (date + location + readings + haiku), and addsloop-start.sh/loop-stop.shscripts that toggle the EventBridge rule directly viaaws events.Writer (
src/agent/fetch.ts)The biggest single change. The per-source format+post loop is replaced with: fetch all → skip if all-failed → format once with a clean
LoopContext(no RAG fields) → embed the LLM's pre-suffix output once → run a global KNN lookup → mechanically append\n\nReminds me of: <past.baseMessage>if a match exists → post once → per-sourceagent_notificationsrows with the same combined message → reuse the pre-vector to insert per-source embeddings (one Titan call per tick, not per source).Schema (
src/db/bootstrap.ts)Adds a new
base_message TEXTcolumn toagent_notifications(nullable, PRAGMA-guarded). The RAG corpus embeds this column, andfindNearestMatchreturns it for the "Reminds me of" suffix — never the postedformatted_message— so the suffix cannot snowball.RAG (
src/rag/similarity.ts)findNearestMatchloses itssourceparam (global KNN, not per-source), addsWHERE n.base_message IS NOT NULL(required: without it, legacy rows would surface as"null"in the suffix), and returnsbaseMessageinstead offormattedMessage.Formatter (
src/format/*)New
LoopContextshape withdate,location, andreadings: [{source, value}]. Source-agnostic — a third source added perdocs/04-extending.mdshows up in the prompt automatically. The Bedrock system prompt now asks for a short friendly comment + closing haiku (5-7-5). The "closest past reading" mention is gone; RAG is appended mechanically after the LLM call.Infra (
infra/stack.ts)rate(5 minutes)(wasrate(1 day)).60s(was30s) — gives the two-step RAG flow headroom against transient Bedrock latency.LoopRuleNameCfnOutput exposingfetchSchedule.ruleNameso the scripts can resolve the rule without hardcoding.Scripts (
scripts/loop-start.sh,scripts/loop-stop.sh)Plain
aws events enable-rule/aws events disable-rule, reading the rule name from theLoopRuleNamestack output. No Lambda involved, no token, no extra IAM. Bothchmod +xand exposed asnpm run loop-start/npm run loop-stop.Docs
README.md— new "Loop mode" subsection with start/stop commands and the redeploy caveat (anpm run deployafterloop-stopre-enables the rule, since the CDK stack declares itenabled: true).docs/07-budget-protection.md— new bullet: loop mode drives ~576 Bedrock calls/day and growsagent_notifications+agent_embeddingsby ~1,152 rows/day combined.Test plan
npm run typecheck && npm test→ 119/119 tests pass across 14 files.Out of scope (per spec §9)
RAG corpus bloat over long runs, per-source dedup, and the "Reminds me of" near-echo at short cadence — all called out in the spec as deliberate trade-offs for a short-lived watched loop. Future spec if needed.
Follow-up:
loop-status(commita77ec71)Added a read-only companion to
loop-start/loop-stop:scripts/loop-status.sh— new,chmod +x, syntax-checked. FetchesLoopRuleNamefrom the stack the same way the other two scripts do, then callsaws events describe-ruleand prints the rule'sState(ENABLED/DISABLED),ScheduleExpression, andArn. Verdict line tells the operator which ofloop-startorloop-stopis the right next call. Read-only — does not enable or disable anything.package.json— exposed asnpm run loop-status.README.md(Loop mode section) — added to the command block, the prose now refers to "all three scripts" and notesloop-statusis read-only, and the redeploy caveat now points operators atnpm run loop-statusto confirm state after a redeploy before deciding whether to re-runloop-stop.This was the one gap the original PR left: there was no way to check loop state without either remembering to re-run
loop-stopafter every redeploy or falling back to a rawaws events describe-ruleinvocation.loop-statusmakes the right sequence self-documenting.🤖 Generated with Claude Code