diff --git a/.agents/memory/INBOX.md b/.agents/memory/INBOX.md index bd6fd0ab..350c9d98 100644 --- a/.agents/memory/INBOX.md +++ b/.agents/memory/INBOX.md @@ -13,42 +13,8 @@ One note per PR that hit friction, four lines: Cost: ``` -- 2026-09-24 #108 skill: implement-issue - What went wrong: the new no-tls-bypass lint rule's red tests covered only the literal spellings in the issue, so `vi.stubEnv(...)` and `globalAgent.options.rejectUnauthorized = false` got through. - Would have prevented it: when an issue asks a lint rule to catch "equivalent" forms, write a red test for each way the pattern can be spelled (assignment, call argument, member assignment, object property) before implementing. - Cost: review round - Seen: 2026-09-28 -- 2026-09-25 #118 skill: implement-issue - What went wrong: the `app.events()` overloads put the uncapped signature first, so options with `payloadMaxBytes` passed through a variable (no excess-property check) resolved to it and `e.payload` compiled unnarrowed. - Would have prevented it: when an optional field switches a return type, make the other overload forbid it (`field?: undefined`) and add a `@ts-expect-error` test that passes the options through a variable. - Cost: review round - Seen: 2026-09-28 -- 2026-09-25 #119 skill: implement-issue - What went wrong: the implementer left out `payloadMaxBytes` on `waitForEvent`, which the issue explicitly asks for. Moving each wait onto its own stream then broke `close()` three times over: pending waits were not closed, a wait whose stream was still opening escaped `close()`, and a failed open rejected with a raw socket error instead of `connection_error`. - Would have prevented it: tick off every bullet of the issue's Expected outcome, not just the numbered criteria. When a call moves off a shared resource onto its own, list what the shared resource did for free (close on shutdown, error typing) and test each against the new resource, including `close()` with no tick before it and an open that rejects. - Cost: three review rounds, fix-loop limit hit, issue blocked with one should-fix open - Seen: 2026-09-28 -- 2026-09-29 #140 skill: implement-issue - What went wrong: the app closing its own socket for the background went through `onSocketLost`, which emits an `error` event for any close other than 1000, so every app switch fired the app's error listener. - Would have prevented it: when adding a deliberate close or disconnect, list every event the existing loss path emits and write a red test for each one that must not fire. - Cost: review round -- 2026-10-02 #143 skill: implement-issue - What went wrong: the new event-descriptors.json conformance fixture left out the non-object and description-non-string vectors that tool-descriptors.json has, and had no non-ASCII name to pin the 4096 limit in UTF-16 code units. - Would have prevented it: when adding a descriptor fixture, start from every vector class in the nearest existing fixture and add one boundary vector in non-BMP characters for any length limit. - Cost: review round -- 2026-10-02 #145 skill: implement-issue - What went wrong: the Swift event registry sent the post-ack snapshot from an unstructured Task, so later deltas could overtake it, and the shared frames fixture pinned an order the SDK did not guarantee (test flaked 9 in 25). - Would have prevented it: when a fixture pins frame order across an async boundary, route those frames through one ordered queue and run the fixture test 25 times before committing. - Cost: review round -- 2026-10-02 #146 skill: implement-issue - What went wrong: in Kotlin, a registerEvent called from a session-change listener during ack handling sent its delta before the ack's snapshot, which then erased it on the daemon. - Would have prevented it: when an SDK sends a snapshot on ack, test a declaration made from a listener and from another thread during ack handling, not just before and after it. - Cost: review round - 2026-10-02 #147 skill: implement-issue What went wrong: the shipped skill's writing-tools.md and docs/TOOLS.md described declaring events for React Native only, and claimed dev warnings that only the React Native SDK gives. Would have prevented it: when a feature ships in several SDKs, write the user docs with one snippet per SDK and scope each behaviour claim to the SDKs that have it. Cost: review round -- 2026-10-02 #143 skill: implement-issue - What went wrong: adding --limit/--offset to `events ls` "mirroring tools ls" copied the flags but not tools' empty-page message or footer rule, so a page past the end printed "No events declared." for a session that had events. - Would have prevented it: when mirroring another command's paging, port its renderer cases too (empty page with total > 0, last page footer) and test each against the original's output. - Cost: review round + Seen: 2026-10-05 diff --git a/.agents/memory/LESSONS.md b/.agents/memory/LESSONS.md index a545c9ef..3f0ad106 100644 --- a/.agents/memory/LESSONS.md +++ b/.agents/memory/LESSONS.md @@ -26,6 +26,12 @@ Caps: 10 entries per section, 40 in total. Over the cap, the next review merges - 2026-09-28 (#117) Match user-supplied wildcard patterns in the daemon without building a regex (split on `*` and `indexOf` each piece, as `daemon/event-bus.ts` does), and add a many-star timing test with the red tests. Evidence: a `.*`-per-star regex backtracked exponentially and could block the single-threaded daemon; caught only in review. Mechanism: #132 +- 2026-10-05 (#119, #140) When a change moves a call onto its own resource or adds a deliberate close, list everything the existing path did for free or emitted (close on shutdown, error typing, `error` events) and write a red test for each, including `close()` with no tick before it and an open that rejects. + Evidence: #119 broke `close()` three ways and hit the fix-loop limit; #140 fired the app's error listener on every app switch. +- 2026-10-05 (#145, #146) When an SDK sends a snapshot on ack, route everything sent around it through one ordered queue and test a declaration made from a listener and from another thread during ack handling; run a fixture that pins frame order 25 times before committing. + Evidence: Swift deltas overtook the snapshot (flaked 9 in 25) and a Kotlin delta sent before the snapshot was erased on the daemon. +- 2026-10-05 (#108, #143) When a lint rule, fixture or command mirrors an existing one, start from every case of the nearest existing one (vector classes, renderer cases such as an empty page with total > 0) and write a red test per spelling of the pattern; add a non-BMP vector for any length limit. + Evidence: the no-tls-bypass rule missed `vi.stubEnv` and `globalAgent.options` forms, the events fixture lacked vectors tools had, and `events ls` printed "No events declared." past the last page; each cost a review round. ## review-pr