fix: address external review feedback (title-caps fact, testkit ZSet fidelity, mock-leak, doctor semver, replay seeding)

HANDOFF.md

@@ -178,6 +178,16 @@ git hooks (simple-git-hooks): pre-commit → lint-staged ;  pre-push → typeche
 - ✅ acceptance G1에 devvit.json `$schema` + cron 5-field 체크 추가. `docs/new-mod-checklist.md` 신규.
 - ✅ **버그 수정**: 미사용 변수 2건(`SAFE_ACTIONS` import, `catch (err)`) — ESLint가 잡음.
 
+**세션 3 (2026-05-12) — 외부 리뷰(gemini-code-assist / coderabbit / codex) 반영 (PR #3)**:
+
+- ✅ `r_shouting_title` starter rule이 **body** uppercase ratio를 보던 버그 → 새 fact `content.title.upperCaseRatio` 추가 (rule-schema FactPaths + fact-bag 계산 + system-prompt 자동 포함). 이제 ALL-CAPS **제목** 신호를 정확히 본다. (codex P2)
+- ✅ `devvit-testkit.ts`: `zAdd` 중복 멤버 허용 → 실제 Redis ZADD처럼 멤버별 score 갱신; `zRange`가 `by:'score'`일 때 `reverse` 무시하던 것 수정 + 구조 정리. (gemini)
+- ✅ `devvit-doctor.ts`: node 버전 비교가 major.minor만 보던 것 → major.minor.patch. (gemini)
+- ✅ `system-prompt.test.ts`: clarification 예시의 빈 `suggestedAnswers []`가 통과하던 것 → `length > 0` 검증 추가. (coderabbit)
+- ✅ `test/setup.ts` `beforeEach`: `vi.clearAllMocks()`만으로는 mock **구현**이 누수 → `getPostById`/`getCommentById` `mockReset()` + 모든 reddit 더블 기본 impl 재설정. (coderabbit)
+- ✅ `replay-runner`: `redisHashes`/`redisZsets` fixture 필드 추가 (이전엔 string 키만 seed 가능) + replay용 generic thing stub. `fixtures/undo-action.json` 신규 (rollback 라운드트립 replay).
+- 152 tests pass (1 skipped). lint/format/tsc clean. acceptance 4/4.
+
 **아직 남음 (= 사용자 Devvit wizard 단계 + 이후)**:
 
 - `npm run dev` 실기 playtest로만 검증되는 gate (Compose 메뉴 렌더, OpenAI compile 라운드트립, undo 라운드트립) — acceptance/doctor 출력의 MANUAL/soft 섹션 참조

fixtures/undo-action.json

@@ -0,0 +1,21 @@
+{
+  "route": "/internal/menu/undo-action",
+  "mod": true,
+  "body": { "location": "post", "targetId": "t3_target" },
+  "redisZsets": {
+    "testsub:audit": [{ "member": "a_undo_demo", "score": 1730000000000 }]
+  },
+  "redisHashes": {
+    "testsub:audit:a_undo_demo": {
+      "action": "remove",
+      "outcome": "applied",
+      "thingId": "t3_target",
+      "thingType": "post",
+      "authorName": "spammer",
+      "ruleSourceNL": "remove posts from accounts under 50 karma"
+    }
+  },
+  "redis": {
+    "testsub:rollback:a_undo_demo": "{\"entry\":{\"actionId\":\"a_undo_demo\",\"ruleId\":\"r_low_karma\",\"ruleSourceNL\":\"remove posts from accounts under 50 karma\",\"thingId\":\"t3_target\",\"thingType\":\"post\",\"action\":\"remove\",\"params\":{},\"authorName\":\"spammer\",\"ts\":1730000000000,\"outcome\":\"applied\"},\"reverseParams\":{\"wasRemoved\":false,\"action\":\"remove\"}}"
+  }
+}

scripts/devvit-doctor.ts

@@ -118,13 +118,18 @@ try {
   const have = process.versions.node;
   if (!want) warn('package.json has no engines.node');
   else {
-    const min = want
-      .replace(/[^\d.]/g, '')
-      .split('.')
-      .map(Number);
-    const cur = have.split('.').map(Number);
-    const geq = cur[0] > min[0] || (cur[0] === min[0] && (cur[1] ?? 0) >= (min[1] ?? 0));
-    if (geq) ok(`node ${have} satisfies engines.node "${want}"`);
+    // Compare major.minor.patch. Only handles a single ">=x.y.z" floor — fine
+    // for the engines fields we write; not a full semver-range parser.
+    const parts = (v: string) =>
+      v
+        .replace(/[^\d.]/g, '')
+        .split('.')
+        .map((n) => Number(n) || 0);
+    const min = parts(want);
+    const cur = parts(have);
+    let cmp = 0;
+    for (let i = 0; i < 3 && cmp === 0; i++) cmp = (cur[i] ?? 0) - (min[i] ?? 0);
+    if (cmp >= 0) ok(`node ${have} satisfies engines.node "${want}"`);
     else fail(`node ${have} does NOT satisfy engines.node "${want}"`);
   }
 } catch {

scripts/replay.ts

@@ -17,7 +17,9 @@
 //     "mod":     true,                      // caller is a moderator of the playtest sub (default: true)
 //     "settings":{ "openaiApiKey": "sk-x" },// values returned by settings.get(...)
 //     "openai":  { ...ruleJson... },        // canned OpenAI compile result (sets a fake fetch)
-//     "redis":   { "testsub:rules:active": "{...json...}" }  // pre-seed redis string keys
+//     "redis":      { "testsub:rules:active": "{...json...}" },          // pre-seed string keys
+//     "redisHashes":{ "testsub:audit:<id>": { "action": "remove", ... } }, // pre-seed hash keys
+//     "redisZsets": { "testsub:audit": [ { "member": "<id>", "score": 1730000000000 } ] }  // pre-seed zsets
 //   }
 //
 // This shells out to vitest so it reuses test/setup.ts's mocks verbatim; the

src/server/evaluator.test.ts

@@ -21,6 +21,7 @@ function facts(overrides: Partial<Record<string, string | number | boolean>> = {
     'content.containsRegex': 'hello world join my discord.gg/abc',
     'content.title.length': 12,
     'content.title.contains': 'My Cool Title',
+    'content.title.upperCaseRatio': 0.25,
     'content.url': 'https://discord.gg/abc',
     'content.urlDomain': 'discord.gg',
     'sub.weeklyActiveUsers': 1000,

src/server/fact-bag.test.ts

@@ -36,6 +36,17 @@ describe('buildPostFactBag', () => {
     expect(bag['content.containsRegex']).toBe(POST.body);
   });
 
+  it('computes content.title.upperCaseRatio from the title, independent of the body', async () => {
+    const shouty = await buildPostFactBag({ ...POST, title: 'BUY MY COURSE NOW', body: 'a perfectly calm body' });
+    expect(shouty['content.title.upperCaseRatio']).toBe(1);
+    expect(shouty['content.upperCaseRatio']).toBeLessThan(0.2); // body ratio unaffected
+    const calm = await buildPostFactBag({ ...POST, title: 'a perfectly calm title', body: 'WHATEVER' });
+    expect(calm['content.title.upperCaseRatio']).toBe(0);
+    // numeric-only title → 0, not NaN
+    const numeric = await buildPostFactBag({ ...POST, title: '2024 results' });
+    expect(numeric['content.title.upperCaseRatio']).toBe(0);
+  });
+
   it('passes through subreddit context', async () => {
     const bag = await buildPostFactBag(POST);
     expect(bag['sub.weeklyActiveUsers']).toBe(1234);

src/server/fact-bag.ts

@@ -29,12 +29,19 @@ interface CommentInput {
   sub?: { weeklyActiveUsers?: number; over18?: boolean };
 }
 
+// Fraction of A–Z letters in `s` that are uppercase. 0 when `s` has no letters
+// (so a link post with an empty body / a numeric title scores 0, not NaN).
+function upperCaseRatioOf(s: string): number {
+  const letters = s.replace(/[^A-Za-z]/g, '');
+  return letters.length === 0 ? 0 : (letters.match(/[A-Z]/g)?.length ?? 0) / letters.length;
+}
+
 export async function buildPostFactBag(p: PostInput, reportsCount = 0): Promise<FactBag> {
   const a = await getAuthorFacts(p.authorId, p.authorName);
   const linkRegex = /https?:\/\/[^\s)]+/gi;
   const links = p.body?.match(linkRegex) ?? [];
-  const upper = (p.body ?? '').replace(/[^A-Za-z]/g, '');
-  const upperCaseRatio = upper.length === 0 ? 0 : (upper.match(/[A-Z]/g)?.length ?? 0) / upper.length;
+  const upperCaseRatio = upperCaseRatioOf(p.body ?? '');
+  const titleUpperCaseRatio = upperCaseRatioOf(p.title ?? '');
   let urlDomain = '';
   try {
     if (p.url) urlDomain = new URL(p.url).hostname;
@@ -59,6 +66,7 @@ export async function buildPostFactBag(p: PostInput, reportsCount = 0): Promise<
     'content.containsRegex': p.body ?? '',
     'content.title.length': p.title?.length ?? 0,
     'content.title.contains': p.title ?? '',
+    'content.title.upperCaseRatio': titleUpperCaseRatio,
     'content.url': p.url ?? '',
     'content.urlDomain': urlDomain,
 
@@ -74,8 +82,7 @@ export async function buildCommentFactBag(c: CommentInput, reportsCount = 0): Pr
   const a = await getAuthorFacts(c.authorId, c.authorName);
   const linkRegex = /https?:\/\/[^\s)]+/gi;
   const links = c.body.match(linkRegex) ?? [];
-  const upper = c.body.replace(/[^A-Za-z]/g, '');
-  const upperCaseRatio = upper.length === 0 ? 0 : (upper.match(/[A-Z]/g)?.length ?? 0) / upper.length;
+  const upperCaseRatio = upperCaseRatioOf(c.body);
 
   return {
     'author.accountAgeHours': a.accountAgeHours,
@@ -93,6 +100,7 @@ export async function buildCommentFactBag(c: CommentInput, reportsCount = 0): Pr
     'content.containsRegex': c.body,
     'content.title.length': 0,
     'content.title.contains': '',
+    'content.title.upperCaseRatio': 0,
     'content.url': '',
     'content.urlDomain': '',
 

src/shared/rule-schema.ts

@@ -39,10 +39,11 @@ export const FactPaths = [
   'content.length',
   'content.linkCount',
   'content.imageCount',
-  'content.upperCaseRatio',
+  'content.upperCaseRatio', // body's A–Z uppercase ratio (0 for link posts / empty bodies)
   'content.containsRegex', // requires .params.regex
   'content.title.length',
   'content.title.contains', // requires .params.needle
+  'content.title.upperCaseRatio', // title's A–Z uppercase ratio — the "ALL CAPS TITLE" signal
   'content.url', // full URL (post link)
   'content.urlDomain', // hostname only
 

src/shared/starter-rules.test.ts

@@ -71,6 +71,7 @@ describe('starter rules behave as intended against representative fact bags', ()
     'content.containsRegex': 'a normal post',
     'content.title.length': 20,
     'content.title.contains': 'A Normal Title',
+    'content.title.upperCaseRatio': 0.05,
     'content.url': '',
     'content.urlDomain': '',
     'sub.weeklyActiveUsers': 500,
@@ -98,15 +99,25 @@ describe('starter rules behave as intended against representative fact bags', ()
     expect(out.map((r) => r.id)).toContain('r_low_karma_link_drop');
   });
 
-  it('an ALL-CAPS title matches the shouting-title rule', () => {
+  it('an ALL-CAPS title matches the shouting-title rule (title ratio, not body ratio)', () => {
     const out = selectMatchingRules(rules, 'onPostSubmit', {
       ...base,
-      'content.upperCaseRatio': 0.95,
+      'content.title.upperCaseRatio': 0.95,
       'content.title.length': 30,
     });
     expect(out.map((r) => r.id)).toContain('r_shouting_title');
   });
 
+  it('a normal-cased title with an ALL-CAPS body does NOT trigger the shouting-title rule', () => {
+    const out = selectMatchingRules(rules, 'onPostSubmit', {
+      ...base,
+      'content.upperCaseRatio': 0.95, // body is shouty, title is not
+      'content.title.upperCaseRatio': 0.05,
+      'content.title.length': 30,
+    });
+    expect(out.map((r) => r.id)).not.toContain('r_shouting_title');
+  });
+
   it('a long all-caps comment matches the wall-of-caps rule', () => {
     const out = selectMatchingRules(rules, 'onCommentSubmit', {
       ...base,

src/shared/starter-rules.ts

@@ -48,7 +48,7 @@ const STARTER_RULE_SEEDS: readonly SeedRule[] = [
     on: ['onPostSubmit'],
     when: {
       all: [
-        { fact: 'content.upperCaseRatio', op: 'gt', value: 0.7 },
+        { fact: 'content.title.upperCaseRatio', op: 'gt', value: 0.7 },
         { fact: 'content.title.length', op: 'gte', value: 12 },
       ],
     },

src/shared/system-prompt.test.ts

@@ -66,6 +66,7 @@ describe('FEW_SHOT_EXAMPLES', () => {
       if (!('needsClarification' in ex.assistant)) continue;
       expect((ex.assistant.question ?? '').length).toBeGreaterThan(0);
       expect(Array.isArray(ex.assistant.suggestedAnswers)).toBe(true);
+      expect((ex.assistant.suggestedAnswers ?? []).length).toBeGreaterThan(0);
     }
   });
 });

test/devvit-testkit.ts

@@ -70,21 +70,22 @@ export function makeFakeRedis(): FakeRedis {
     },
     hGetAll: async (k: string) => ({ ...(hashes.get(k) ?? {}) }),
     zAdd: async (k: string, entry: { member: string; score: number }) => {
-      const arr = zsets.get(k) ?? [];
+      // Real Redis ZADD updates the score of an existing member, not a duplicate.
+      const arr = (zsets.get(k) ?? []).filter((e) => e.member !== entry.member);
       arr.push(entry);
       zsets.set(k, arr);
     },
     zRange: async (k: string, start: number, stop: number, opts?: { by?: 'rank' | 'score'; reverse?: boolean }) => {
-      let arr = [...(zsets.get(k) ?? [])].sort((a, b) => a.score - b.score);
+      const arr = [...(zsets.get(k) ?? [])].sort((a, b) => a.score - b.score);
+      if (opts?.reverse) arr.reverse();
       if (opts?.by === 'score') {
-        arr = arr.filter((e) => e.score >= start && e.score <= stop);
-      } else {
-        if (opts?.reverse) arr.reverse();
-        const end = stop < 0 ? arr.length : stop + 1;
-        arr = arr.slice(start, end);
-        return arr;
+        // start/stop are score bounds (low..high); honour them regardless of order.
+        const [lo, hi] = start <= stop ? [start, stop] : [stop, start];
+        return arr.filter((e) => e.score >= lo && e.score <= hi);
       }
-      return arr;
+      // by rank (default): start/stop are indices into the (possibly reversed) array.
+      const end = stop < 0 ? arr.length : stop + 1;
+      return arr.slice(start, end);
     },
     zCount: async (k: string, min: number, max: number | '+inf') => {
       const hi = max === '+inf' ? Infinity : max;

test/replay-runner.test.ts

@@ -27,7 +27,9 @@ type Fixture = {
   mod?: boolean;
   settings?: Record<string, unknown>;
   openai?: unknown;
-  redis?: Record<string, string>;
+  redis?: Record<string, string>; // string keys, e.g. `${sub}:rules:active`
+  redisHashes?: Record<string, Record<string, string>>; // hash keys, e.g. `${sub}:audit:<id>`
+  redisZsets?: Record<string, Array<{ member: string; score: number }>>; // zset keys, e.g. `${sub}:audit`
 };
 
 function snapshotRedisKeys() {
@@ -48,11 +50,27 @@ describe.skipIf(!FIXTURE)('replay', () => {
       );
     const body = 'body' in fx && fx.body !== undefined ? fx.body : fx;
 
+    // Generic Reddit "thing" stub so action/rollback flows complete in a replay
+    // (tests use precise mocks; here we just want to observe the control flow).
+    const stubThing = {
+      approve: async () => {},
+      unlock: async () => {},
+      lock: async () => {},
+      remove: async () => {},
+      removed: false,
+      flair: null as { text: string } | null,
+    };
+    fakeReddit.getPostById.mockResolvedValue(stubThing);
+    fakeReddit.getCommentById.mockResolvedValue(stubThing);
+
     // Apply fixture-driven mock state.
     if (fx.mod ?? true) fakeReddit.getModerators.mockResolvedValue(fakeListing([{ username: 'caller' }]));
     if (fx.settings) fakeSettings.get.mockImplementation(async (k: string) => fx.settings![k]);
     if (fx.openai !== undefined) fakeFetch.mockResolvedValue(openaiResponse(fx.openai));
     if (fx.redis) for (const [k, v] of Object.entries(fx.redis)) await fakeRedis.set(k, v);
+    if (fx.redisHashes) for (const [k, h] of Object.entries(fx.redisHashes)) await fakeRedis.hSet(k, h);
+    if (fx.redisZsets)
+      for (const [k, members] of Object.entries(fx.redisZsets)) for (const m of members) await fakeRedis.zAdd(k, m);
 
     const before = snapshotRedisKeys();
     const res = await app.fetch(

test/setup.ts

@@ -41,11 +41,21 @@ beforeEach(() => {
   fakeRedis.hashes.clear();
   fakeRedis.zsets.clear();
   vi.clearAllMocks();
+  // clearAllMocks only wipes call history — re-establish every default *implementation*
+  // here so a `.mockResolvedValue(...)` / `.mockImplementation(...)` in one test can't
+  // leak into the next. (getPostById/getCommentById have no factory default → mockReset.)
+  fakeReddit.getPostById.mockReset();
+  fakeReddit.getCommentById.mockReset();
   fakeReddit.getCurrentSubreddit.mockResolvedValue({ id: 't5_testsub', name: 'testsub' });
   fakeReddit.getCurrentUser.mockResolvedValue({ id: 't2_caller', username: 'caller' });
   fakeReddit.getUserByUsername.mockResolvedValue(null);
   fakeReddit.getUserKarmaFromCurrentSubreddit.mockResolvedValue({ fromComments: 0, fromPosts: 0 });
   fakeReddit.getModerators.mockResolvedValue({ all: async () => [] });
+  fakeReddit.report.mockResolvedValue({});
+  fakeReddit.setPostFlair.mockResolvedValue(undefined);
+  fakeReddit.banUser.mockResolvedValue(undefined);
+  fakeReddit.muteUser.mockResolvedValue(undefined);
+  fakeReddit.unbanUser.mockResolvedValue(undefined);
   fakeReddit.modMail.createModNotification.mockResolvedValue('modmail_conv_1');
   fakeScheduler.runJob.mockResolvedValue('job_1');
   fakeSettings.get.mockResolvedValue(undefined);