From 48d7ae4bb7cb3b04cc1ac2ea6b4471e1c28a0c7d Mon Sep 17 00:00:00 2001 From: Bowei Feng Date: Tue, 30 Jun 2026 11:34:57 +0800 Subject: [PATCH 1/4] feat: implement stat point allocation on level-up stat_points_available was already a column in the DB schema but was never wired up. This activates it end-to-end: - awardXp now grants 10 stat points per level gained - new buddy_allocate tool lets you spend points on a specific stat (DEBUGGING, PATIENCE, CHAOS, WISDOM, SNARK), capped at 100 - buddy_status appends a notice when unspent points are available --- src/server/index.ts | 67 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/src/server/index.ts b/src/server/index.ts index 2c2386d..2cac524 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -73,6 +73,11 @@ function awardXp(companionId: string, eventType: string): { newXp: number; newLe db.prepare("UPDATE companions SET xp = ?, level = ? WHERE id = ?").run(newXp, newLevel, companionId); + if (leveledUp) { + const levelsGained = newLevel - (row?.level || 1); + db.prepare("UPDATE companions SET stat_points_available = stat_points_available + ? WHERE id = ?").run(levelsGained * 10, companionId); + } + return { newXp, newLevel, leveledUp }; } @@ -172,6 +177,25 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { properties: {}, }, }, + { + name: "buddy_allocate", + description: "Spend an available stat point (earned from leveling up) to permanently raise one of your companion's five stats by 1, up to a cap of 100. Call buddy_status to see how many points are available.", + inputSchema: { + type: "object", + properties: { + stat: { + type: "string", + enum: ["DEBUGGING", "PATIENCE", "CHAOS", "WISDOM", "SNARK"], + description: "Which stat to raise." + }, + points: { + type: "number", + description: "How many available points to spend on this stat (default 1)." + } + }, + required: ["stat"], + }, + }, { name: "buddy_observe", description: "Call after every coding task with a 1-sentence summary. Returns your buddy's in-character reaction + XP. In guard mode, also pass claims/edges/cwd (schemas below); the observer prompt restates extraction guidance each turn.", @@ -354,7 +378,11 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { writeBuddyStatus(companion); - return { content: [{ type: "text", text: "DISPLAY VERBATIM: Show the full stat card below in a code block. Do not summarize.\n\n" + statusCard }] }; + const pointsNote = companion.availablePoints > 0 + ? `\n\n${companion.name} has ${companion.availablePoints} stat point${companion.availablePoints === 1 ? '' : 's'} available — use buddy_allocate to spend them.` + : ''; + + return { content: [{ type: "text", text: "DISPLAY VERBATIM: Show the full stat card below in a code block. Do not summarize.\n\n" + statusCard + pointsNote }] }; } if (name === "buddy_remember") { @@ -414,6 +442,43 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { }; } + if (name === "buddy_allocate") { + const { stat, points = 1 } = args as { stat: string; points?: number }; + const row = db.prepare("SELECT * FROM companions LIMIT 1").get() as any; + if (!row) { + return { content: [{ type: "text", text: "No companion hatched yet! Use buddy_hatch first." }] }; + } + + if (!STAT_NAMES.includes(stat as any)) { + return { content: [{ type: "text", text: `Unknown stat "${stat}". Choose from: ${STAT_NAMES.join(', ')}.` }] }; + } + + const available = row.stat_points_available || 0; + if (available <= 0) { + return { content: [{ type: "text", text: `${row.name} has no stat points available. Level up to earn more!` }] }; + } + + const currentValue = row[stat.toLowerCase()] ?? 0; + const canSpend = Math.min(points, available, 100 - currentValue); + if (canSpend <= 0) { + return { content: [{ type: "text", text: `${stat} is already at 100 — can't go higher!` }] }; + } + + db.prepare(`UPDATE companions SET ${stat.toLowerCase()} = ${stat.toLowerCase()} + ?, stat_points_available = stat_points_available - ? WHERE id = ?`) + .run(canSpend, canSpend, row.id); + + const updated = db.prepare("SELECT * FROM companions LIMIT 1").get() as any; + const companion = loadCompanion(updated)!; + const statusCard = renderCard(companion); + const remaining = updated.stat_points_available; + + const summary = remaining > 0 + ? `Spent ${canSpend} point${canSpend === 1 ? '' : 's'} on ${stat}. ${remaining} point${remaining === 1 ? '' : 's'} remaining.` + : `Spent ${canSpend} point${canSpend === 1 ? '' : 's'} on ${stat}. No points remaining.`; + + return { content: [{ type: "text", text: `${summary}\n\nDISPLAY VERBATIM: Show the full stat card below in a code block. Do not summarize.\n\n${statusCard}` }] }; + } + if (name === "buddy_observe") { const { summary, mode: modeArg, user_id, claims, edges, cwd } = args as { summary: string; From cfbec9d98489898f19d5bef327217f09ba37f6d7 Mon Sep 17 00:00:00 2001 From: Bowei Feng Date: Sat, 4 Jul 2026 00:39:03 +0800 Subject: [PATCH 2/4] fix(allocate): correct column prefix, NULL safety, integer validation, status refresh Four bugs addressed per maintainer review: 1. Wrong column names: handler read/wrote `debugging` instead of `stat_debugging`. Fix: extract applyStatAllocation to lib/allocate.ts which uses the correct stat_ prefix throughout. 2. NULL arithmetic: rescued companions have NULL stat columns; SQL `NULL + n = NULL` silently wiped points. Fix: resolve current value via loadCompanion() (which applies the bones fallback), then write a concrete integer. 3. Non-positive/fractional points: schema used type:"number" allowing 1.5 or -3. Fix: schema uses type:"integer" + minimum:1; applyStatAllocation also validates and returns { ok:false, reason:'invalid_points' } for early rejection. 4. Status file stale after allocation: writeBuddyStatus() was never called. Fix: called immediately after the companion is reloaded post-update. Adds 16 tests in src/__tests__/allocate.test.ts covering fresh companions, rescued companions with NULL stat columns, stat caps, clamping, invalid point amounts, and multi-level point gains. Co-Authored-By: Claude Sonnet 4.6 --- src/__tests__/allocate.test.ts | 214 +++++++++++++++++++++++++++++++++ src/lib/allocate.ts | 49 ++++++++ src/server/index.ts | 38 +++--- 3 files changed, 283 insertions(+), 18 deletions(-) create mode 100644 src/__tests__/allocate.test.ts create mode 100644 src/lib/allocate.ts diff --git a/src/__tests__/allocate.test.ts b/src/__tests__/allocate.test.ts new file mode 100644 index 0000000..5d606fe --- /dev/null +++ b/src/__tests__/allocate.test.ts @@ -0,0 +1,214 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { initDb, db } from '../db/schema.js'; +import { createCompanion, rescueCompanion, loadCompanion } from '../lib/companion.js'; +import { applyStatAllocation } from '../lib/allocate.js'; + +beforeEach(() => { + initDb(); + db.prepare('DELETE FROM xp_events').run(); + db.prepare('DELETE FROM memories').run(); + db.prepare('DELETE FROM sessions').run(); + db.prepare('DELETE FROM evolution_history').run(); + db.prepare('DELETE FROM companions').run(); +}); + +function givePoints(id: string, pts: number) { + db.prepare('UPDATE companions SET stat_points_available = ? WHERE id = ?').run(pts, id); +} + +function getRow(id: string): any { + return db.prepare('SELECT * FROM companions WHERE id = ?').get(id); +} + +// ─── fresh companions ──────────────────────────────────────────────────────── + +describe('fresh companion', () => { + it('raises the target stat by the requested amount', () => { + const { id } = createCompanion({ userId: 'fresh-1' }); + givePoints(id, 5); + const before = loadCompanion(getRow(id))!; + const result = applyStatAllocation(id, 'SNARK', 3); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.spent).toBe(3); + expect(result.newValue).toBe(before.stats.SNARK + 3); + expect(result.remaining).toBe(2); + }); + + it('writes the correct stat_ column name (not the bare stat name)', () => { + const { id } = createCompanion({ userId: 'fresh-col' }); + givePoints(id, 3); + applyStatAllocation(id, 'DEBUGGING', 2); + const row = getRow(id); + // stat_debugging must be updated; the bare "debugging" column does not exist + expect(typeof row.stat_debugging).toBe('number'); + expect(row.stat_debugging).toBeGreaterThan(0); + // sanity: stat_points_available must be decremented + expect(row.stat_points_available).toBe(1); + }); + + it('does not touch other stat columns', () => { + const { id } = createCompanion({ userId: 'fresh-notouch' }); + givePoints(id, 5); + const before = loadCompanion(getRow(id))!; + applyStatAllocation(id, 'CHAOS', 2); + const after = loadCompanion(getRow(id))!; + expect(after.stats.DEBUGGING).toBe(before.stats.DEBUGGING); + expect(after.stats.PATIENCE).toBe(before.stats.PATIENCE); + expect(after.stats.WISDOM).toBe(before.stats.WISDOM); + expect(after.stats.SNARK).toBe(before.stats.SNARK); + }); + + it('returns no_points when none are available', () => { + const { id } = createCompanion({ userId: 'fresh-nopoints' }); + const result = applyStatAllocation(id, 'WISDOM', 1); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('no_points'); + }); +}); + +// ─── rescued companions (NULL stat columns) ────────────────────────────────── + +describe('rescued companion (NULL stat columns)', () => { + it('initialises a NULL stat column rather than producing NULL', () => { + const { id } = rescueCompanion({ name: 'Nullbert', species: 'Lava Salamander' }); + // rescueCompanion does not set stat_* columns, so they are NULL + expect(getRow(id).stat_snark).toBeNull(); + + givePoints(id, 5); + + const companion = loadCompanion(getRow(id))!; // bones fallback resolves NULL + const expectedNew = companion.stats.SNARK + 2; + + const result = applyStatAllocation(id, 'SNARK', 2); + expect(result.ok).toBe(true); + if (!result.ok) return; + + const row = getRow(id); + expect(row.stat_snark).toBe(expectedNew); // must be a number, not NULL + expect(result.newValue).toBe(expectedNew); + }); + + it('deducts stat_points_available correctly even with NULL stat columns', () => { + const { id } = rescueCompanion({ name: 'Nullbert2' }); + givePoints(id, 8); + applyStatAllocation(id, 'CHAOS', 3); + expect(getRow(id).stat_points_available).toBe(5); + }); + + it('returns the correct remaining count after multiple allocations on a rescued companion', () => { + const { id } = rescueCompanion({ name: 'Nullbert3' }); + givePoints(id, 10); + applyStatAllocation(id, 'PATIENCE', 4); + const result = applyStatAllocation(id, 'WISDOM', 2); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.remaining).toBe(4); + }); +}); + +// ─── stat cap ──────────────────────────────────────────────────────────────── + +describe('stat cap', () => { + it('returns at_cap when the target stat is already 100', () => { + const { id } = createCompanion({ userId: 'cap-1' }); + givePoints(id, 5); + db.prepare('UPDATE companions SET stat_debugging = 100 WHERE id = ?').run(id); + const result = applyStatAllocation(id, 'DEBUGGING', 1); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('at_cap'); + }); + + it('clamps spending so the stat does not exceed 100', () => { + const { id } = createCompanion({ userId: 'cap-clamp' }); + givePoints(id, 10); + db.prepare('UPDATE companions SET stat_patience = 98 WHERE id = ?').run(id); + const result = applyStatAllocation(id, 'PATIENCE', 5); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.newValue).toBe(100); + expect(result.spent).toBe(2); // clamped from 5 to 2 + expect(result.remaining).toBe(8); // 10 - 2 + }); +}); + +// ─── invalid point amounts ─────────────────────────────────────────────────── + +describe('invalid point amounts', () => { + it('rejects 0', () => { + const { id } = createCompanion({ userId: 'inv-zero' }); + givePoints(id, 5); + const result = applyStatAllocation(id, 'SNARK', 0); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('invalid_points'); + }); + + it('rejects negative numbers', () => { + const { id } = createCompanion({ userId: 'inv-neg' }); + givePoints(id, 5); + const result = applyStatAllocation(id, 'CHAOS', -2); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('invalid_points'); + }); + + it('rejects fractional numbers', () => { + const { id } = createCompanion({ userId: 'inv-frac' }); + givePoints(id, 5); + const result = applyStatAllocation(id, 'WISDOM', 1.5); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('invalid_points'); + }); + + it('does not mutate the DB when points are invalid', () => { + const { id } = createCompanion({ userId: 'inv-nomut' }); + givePoints(id, 5); + const before = getRow(id); + applyStatAllocation(id, 'SNARK', -1); + const after = getRow(id); + expect(after.stat_points_available).toBe(before.stat_points_available); + }); +}); + +// ─── multi-level gains ─────────────────────────────────────────────────────── + +describe('multi-level point gains', () => { + it('allows spending points earned across multiple levels', () => { + const { id } = createCompanion({ userId: 'multilevel-1' }); + // Simulate 3 level-ups (10 points each) + givePoints(id, 30); + const r1 = applyStatAllocation(id, 'SNARK', 10); + const r2 = applyStatAllocation(id, 'CHAOS', 15); + expect(r1.ok).toBe(true); + expect(r2.ok).toBe(true); + if (!r1.ok || !r2.ok) return; + expect(r1.spent).toBe(10); + expect(r2.spent).toBe(15); + expect(r2.remaining).toBe(5); + }); + + it('each allocation is reflected in subsequent loadCompanion calls', () => { + const { id } = createCompanion({ userId: 'multilevel-2' }); + givePoints(id, 20); + const base = loadCompanion(getRow(id))!.stats.DEBUGGING; + applyStatAllocation(id, 'DEBUGGING', 5); + expect(loadCompanion(getRow(id))!.stats.DEBUGGING).toBe(base + 5); + applyStatAllocation(id, 'DEBUGGING', 3); + expect(loadCompanion(getRow(id))!.stats.DEBUGGING).toBe(base + 8); + }); + + it('stat_points_available from a level-up is honoured by the allocator', () => { + // Simulate what awardXp does when a companion levels up + const { id } = createCompanion({ userId: 'levelup-sim' }); + db.prepare('UPDATE companions SET stat_points_available = stat_points_available + 10 WHERE id = ?').run(id); + expect(getRow(id).stat_points_available).toBe(10); + const result = applyStatAllocation(id, 'WISDOM', 10); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.remaining).toBe(0); + }); +}); diff --git a/src/lib/allocate.ts b/src/lib/allocate.ts new file mode 100644 index 0000000..52ced64 --- /dev/null +++ b/src/lib/allocate.ts @@ -0,0 +1,49 @@ +import { db } from '../db/schema.js'; +import { loadCompanion } from './companion.js'; +import type { StatName } from './types.js'; + +export type AllocateResult = + | { ok: true; spent: number; newValue: number; remaining: number } + | { ok: false; reason: 'no_companion' | 'no_points' | 'at_cap' | 'invalid_points' }; + +/** + * Spend `points` stat points on `stat` for the companion identified by + * `companionId`. + * + * Resolves NULL stat columns (rescued/legacy companions that predate the stat + * migration) via loadCompanion's bones fallback before writing. This prevents + * NULL + n = NULL corruption that SQL-level increment would produce. + * + * `points` must be a positive integer; non-integer or non-positive values + * return { ok: false, reason: 'invalid_points' }. + */ +export function applyStatAllocation(companionId: string, stat: StatName, points: number): AllocateResult { + if (!Number.isInteger(points) || points < 1) { + return { ok: false, reason: 'invalid_points' }; + } + + const row = db.prepare('SELECT * FROM companions WHERE id = ?').get(companionId) as any; + if (!row) return { ok: false, reason: 'no_companion' }; + + const available = row.stat_points_available ?? 0; + if (available <= 0) return { ok: false, reason: 'no_points' }; + + // loadCompanion resolves NULL stat columns to deterministic bones values so + // a rescued companion with missing stat_ rows is handled correctly. + const companion = loadCompanion(row)!; + const currentValue = companion.stats[stat]; + if (currentValue >= 100) return { ok: false, reason: 'at_cap' }; + + const canSpend = Math.min(points, available, 100 - currentValue); + const newValue = currentValue + canSpend; + + // Column names are stat_debugging / stat_patience / etc. (not the bare stat name). + // Write a concrete value rather than `col = col + ?` so NULL-column rows are + // initialised to the bones value rather than producing NULL. + const dbCol = `stat_${stat.toLowerCase()}`; + db.prepare(`UPDATE companions SET ${dbCol} = ?, stat_points_available = stat_points_available - ? WHERE id = ?`) + .run(newValue, canSpend, companionId); + + const after = db.prepare('SELECT stat_points_available FROM companions WHERE id = ?').get(companionId) as any; + return { ok: true, spent: canSpend, newValue, remaining: after.stat_points_available ?? 0 }; +} diff --git a/src/server/index.ts b/src/server/index.ts index 2cac524..6738b7f 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -24,6 +24,7 @@ import { readFileSync, unlinkSync, mkdirSync } from "fs"; import { join, dirname } from "path"; import { homedir } from "os"; import { loadCompanion, writeBuddyStatus, createCompanion } from "../lib/companion.js"; +import { applyStatAllocation } from "../lib/allocate.js"; import { renderCard, hatchAnimation } from "../lib/card.js"; import { captureSnapshot } from "../lib/snapshot.js"; import { BUDDY_STATUS_PATH } from "../lib/constants.js"; @@ -189,7 +190,8 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { description: "Which stat to raise." }, points: { - type: "number", + type: "integer", + minimum: 1, description: "How many available points to spend on this stat (default 1)." } }, @@ -448,33 +450,33 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { if (!row) { return { content: [{ type: "text", text: "No companion hatched yet! Use buddy_hatch first." }] }; } - if (!STAT_NAMES.includes(stat as any)) { return { content: [{ type: "text", text: `Unknown stat "${stat}". Choose from: ${STAT_NAMES.join(', ')}.` }] }; } - - const available = row.stat_points_available || 0; - if (available <= 0) { - return { content: [{ type: "text", text: `${row.name} has no stat points available. Level up to earn more!` }] }; + if (!Number.isInteger(points) || points < 1) { + return { content: [{ type: "text", text: `points must be a positive integer (got ${points}).` }] }; } - const currentValue = row[stat.toLowerCase()] ?? 0; - const canSpend = Math.min(points, available, 100 - currentValue); - if (canSpend <= 0) { - return { content: [{ type: "text", text: `${stat} is already at 100 — can't go higher!` }] }; + const result = applyStatAllocation(row.id, stat as any, points); + if (!result.ok) { + if (result.reason === 'no_points') { + return { content: [{ type: "text", text: `${row.name} has no stat points available. Level up to earn more!` }] }; + } + if (result.reason === 'at_cap') { + return { content: [{ type: "text", text: `${stat} is already at 100 — can't go higher!` }] }; + } + return { content: [{ type: "text", text: "Something went wrong — no companion found." }] }; } - db.prepare(`UPDATE companions SET ${stat.toLowerCase()} = ${stat.toLowerCase()} + ?, stat_points_available = stat_points_available - ? WHERE id = ?`) - .run(canSpend, canSpend, row.id); - - const updated = db.prepare("SELECT * FROM companions LIMIT 1").get() as any; - const companion = loadCompanion(updated)!; + const updatedRow = db.prepare("SELECT * FROM companions WHERE id = ?").get(row.id) as any; + const companion = loadCompanion(updatedRow)!; + writeBuddyStatus(companion); const statusCard = renderCard(companion); - const remaining = updated.stat_points_available; + const { spent, remaining } = result; const summary = remaining > 0 - ? `Spent ${canSpend} point${canSpend === 1 ? '' : 's'} on ${stat}. ${remaining} point${remaining === 1 ? '' : 's'} remaining.` - : `Spent ${canSpend} point${canSpend === 1 ? '' : 's'} on ${stat}. No points remaining.`; + ? `Spent ${spent} point${spent === 1 ? '' : 's'} on ${stat}. ${remaining} point${remaining === 1 ? '' : 's'} remaining.` + : `Spent ${spent} point${spent === 1 ? '' : 's'} on ${stat}. No points remaining.`; return { content: [{ type: "text", text: `${summary}\n\nDISPLAY VERBATIM: Show the full stat card below in a code block. Do not summarize.\n\n${statusCard}` }] }; } From 2c72761584a9b565c2f52e7b2f0bd7e4c093f331 Mon Sep 17 00:00:00 2001 From: Bowei Feng Date: Sat, 4 Jul 2026 00:48:21 +0800 Subject: [PATCH 3/4] harden(allocate): validate stat inside the lib fn, make read-check-write atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit after a closer look: - applyStatAllocation() now re-validates `stat` against STAT_NAMES itself instead of trusting the caller. It interpolates the stat name into a SQL column reference (stat_${stat.toLowerCase()}), so leaving that check only in the MCP handler meant any other future caller of this now-exported lib function could hand it an unvalidated string straight into SQL. Adds 'invalid_stat' to AllocateResult. - Wraps the SELECT -> compute -> UPDATE -> SELECT sequence in db.transaction(), matching the existing repairDuplicateCompanions convention in db/schema.ts. The maintainer's review asked for the NULL fallback to be applied "atomically"; the previous commit fixed the NULL bug but left the four statements unwrapped. The MCP server and the UserPromptSubmit hook both hold this DB file open, so this closes a real (if narrow) cross-process race window, not just a theoretical one. - index.ts: removed the now-duplicated stat/points validation ahead of the call (applyStatAllocation does it internally) and replaced the two-if fallback with an exhaustive switch over all 5 AllocateResult reasons. - Tests: added invalid-stat-name coverage, including a SQL-injection-shaped stat string, verified to be rejected without mutating the row or dropping the table. Also fixed a latent flakiness bug in the original test file — several tests picked an arbitrary stat and asserted success without pinning its starting value, which breaks deterministically for any userId whose rarity roll happens to land its peak stat at the 100 cap (this is exactly what "multilevel-jump" did on first run). Fresh/multi-level tests now pin the target stat before allocating; rescued-companion tests dynamically pick a stat with headroom instead, since pinning would defeat the point of testing the NULL-column fallback. Co-Authored-By: Claude Sonnet 4.6 --- src/__tests__/allocate.test.ts | 113 +++++++++++++++++++++++++++++---- src/lib/allocate.ts | 60 +++++++++++------ src/server/index.ts | 24 +++---- 3 files changed, 153 insertions(+), 44 deletions(-) diff --git a/src/__tests__/allocate.test.ts b/src/__tests__/allocate.test.ts index 5d606fe..9c3fd97 100644 --- a/src/__tests__/allocate.test.ts +++ b/src/__tests__/allocate.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { initDb, db } from '../db/schema.js'; import { createCompanion, rescueCompanion, loadCompanion } from '../lib/companion.js'; import { applyStatAllocation } from '../lib/allocate.js'; +import { STAT_NAMES } from '../lib/types.js'; beforeEach(() => { initDb(); @@ -20,11 +21,20 @@ function getRow(id: string): any { return db.prepare('SELECT * FROM companions WHERE id = ?').get(id); } +// Deterministic rolls can put a rarity's "peak stat" at 100 already, which +// would spuriously trip the at_cap path in tests that just want to exercise +// plain allocation bookkeeping. Pin a stat to a known-safe baseline so those +// tests don't depend on which stat a given userId happens to roll as peak. +function pinStat(id: string, stat: string, value: number) { + db.prepare(`UPDATE companions SET stat_${stat.toLowerCase()} = ? WHERE id = ?`).run(value, id); +} + // ─── fresh companions ──────────────────────────────────────────────────────── describe('fresh companion', () => { it('raises the target stat by the requested amount', () => { const { id } = createCompanion({ userId: 'fresh-1' }); + pinStat(id, 'SNARK', 50); givePoints(id, 5); const before = loadCompanion(getRow(id))!; const result = applyStatAllocation(id, 'SNARK', 3); @@ -37,21 +47,23 @@ describe('fresh companion', () => { it('writes the correct stat_ column name (not the bare stat name)', () => { const { id } = createCompanion({ userId: 'fresh-col' }); + pinStat(id, 'DEBUGGING', 50); givePoints(id, 3); applyStatAllocation(id, 'DEBUGGING', 2); const row = getRow(id); // stat_debugging must be updated; the bare "debugging" column does not exist - expect(typeof row.stat_debugging).toBe('number'); - expect(row.stat_debugging).toBeGreaterThan(0); + expect(row.stat_debugging).toBe(52); // sanity: stat_points_available must be decremented expect(row.stat_points_available).toBe(1); }); it('does not touch other stat columns', () => { const { id } = createCompanion({ userId: 'fresh-notouch' }); + pinStat(id, 'CHAOS', 50); // ensure the target actually has headroom to write givePoints(id, 5); const before = loadCompanion(getRow(id))!; - applyStatAllocation(id, 'CHAOS', 2); + const result = applyStatAllocation(id, 'CHAOS', 2); + expect(result.ok).toBe(true); const after = loadCompanion(getRow(id))!; expect(after.stats.DEBUGGING).toBe(before.stats.DEBUGGING); expect(after.stats.PATIENCE).toBe(before.stats.PATIENCE); @@ -69,39 +81,55 @@ describe('fresh companion', () => { }); // ─── rescued companions (NULL stat columns) ────────────────────────────────── +// +// These tests can't pin the target stat -- the whole point is to exercise the +// NULL-column bones-fallback path. Instead they dynamically pick any stat +// that currently has headroom (a rescued companion has at most one stat +// pinned near a cap by its rarity roll, so 4 of 5 are virtually guaranteed to +// have room). describe('rescued companion (NULL stat columns)', () => { it('initialises a NULL stat column rather than producing NULL', () => { const { id } = rescueCompanion({ name: 'Nullbert', species: 'Lava Salamander' }); - // rescueCompanion does not set stat_* columns, so they are NULL - expect(getRow(id).stat_snark).toBeNull(); + // rescueCompanion does not set stat_* columns, so they are all NULL + const row0 = getRow(id); + for (const s of STAT_NAMES) { + expect(row0[`stat_${s.toLowerCase()}`]).toBeNull(); + } givePoints(id, 5); const companion = loadCompanion(getRow(id))!; // bones fallback resolves NULL - const expectedNew = companion.stats.SNARK + 2; + const stat = STAT_NAMES.find(s => companion.stats[s] < 100)!; + const expectedNew = companion.stats[stat] + 2; - const result = applyStatAllocation(id, 'SNARK', 2); + const result = applyStatAllocation(id, stat, 2); expect(result.ok).toBe(true); if (!result.ok) return; const row = getRow(id); - expect(row.stat_snark).toBe(expectedNew); // must be a number, not NULL + expect(row[`stat_${stat.toLowerCase()}`]).toBe(expectedNew); // must be a number, not NULL expect(result.newValue).toBe(expectedNew); }); it('deducts stat_points_available correctly even with NULL stat columns', () => { const { id } = rescueCompanion({ name: 'Nullbert2' }); givePoints(id, 8); - applyStatAllocation(id, 'CHAOS', 3); + const companion = loadCompanion(getRow(id))!; + const stat = STAT_NAMES.find(s => companion.stats[s] < 98)!; // headroom for 3 points + const result = applyStatAllocation(id, stat, 3); + expect(result.ok).toBe(true); expect(getRow(id).stat_points_available).toBe(5); }); it('returns the correct remaining count after multiple allocations on a rescued companion', () => { const { id } = rescueCompanion({ name: 'Nullbert3' }); givePoints(id, 10); - applyStatAllocation(id, 'PATIENCE', 4); - const result = applyStatAllocation(id, 'WISDOM', 2); + const companion = loadCompanion(getRow(id))!; + const [statA, statB] = STAT_NAMES.filter(s => companion.stats[s] < 96); // headroom for 4 then 2 + const r1 = applyStatAllocation(id, statA, 4); + const result = applyStatAllocation(id, statB, 2); + expect(r1.ok).toBe(true); expect(result.ok).toBe(true); if (!result.ok) return; expect(result.remaining).toBe(4); @@ -134,6 +162,44 @@ describe('stat cap', () => { }); }); +// ─── invalid stat names ────────────────────────────────────────────────────── + +describe('invalid stat names', () => { + it('rejects a stat name outside STAT_NAMES', () => { + const { id } = createCompanion({ userId: 'badstat-1' }); + givePoints(id, 5); + const result = applyStatAllocation(id, 'NOTASTAT' as any, 1); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('invalid_stat'); + }); + + it('rejects a lowercase stat name (must match STAT_NAMES exactly)', () => { + const { id } = createCompanion({ userId: 'badstat-2' }); + givePoints(id, 5); + const result = applyStatAllocation(id, 'snark' as any, 1); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('invalid_stat'); + }); + + it('safely rejects a SQL-injection-shaped stat string without touching the DB', () => { + const { id } = createCompanion({ userId: 'badstat-injection' }); + givePoints(id, 5); + const before = getRow(id); + + const result = applyStatAllocation(id, 'snark = 999; DROP TABLE companions; --' as any, 1); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.reason).toBe('invalid_stat'); + + // Table must still exist and this row must be untouched. + const after = getRow(id); + expect(after).toEqual(before); + expect(db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='companions'").get()).toBeTruthy(); + }); +}); + // ─── invalid point amounts ─────────────────────────────────────────────────── describe('invalid point amounts', () => { @@ -179,6 +245,8 @@ describe('invalid point amounts', () => { describe('multi-level point gains', () => { it('allows spending points earned across multiple levels', () => { const { id } = createCompanion({ userId: 'multilevel-1' }); + pinStat(id, 'SNARK', 50); + pinStat(id, 'CHAOS', 50); // Simulate 3 level-ups (10 points each) givePoints(id, 30); const r1 = applyStatAllocation(id, 'SNARK', 10); @@ -193,6 +261,7 @@ describe('multi-level point gains', () => { it('each allocation is reflected in subsequent loadCompanion calls', () => { const { id } = createCompanion({ userId: 'multilevel-2' }); + pinStat(id, 'DEBUGGING', 50); givePoints(id, 20); const base = loadCompanion(getRow(id))!.stats.DEBUGGING; applyStatAllocation(id, 'DEBUGGING', 5); @@ -204,6 +273,7 @@ describe('multi-level point gains', () => { it('stat_points_available from a level-up is honoured by the allocator', () => { // Simulate what awardXp does when a companion levels up const { id } = createCompanion({ userId: 'levelup-sim' }); + pinStat(id, 'WISDOM', 50); db.prepare('UPDATE companions SET stat_points_available = stat_points_available + 10 WHERE id = ?').run(id); expect(getRow(id).stat_points_available).toBe(10); const result = applyStatAllocation(id, 'WISDOM', 10); @@ -211,4 +281,25 @@ describe('multi-level point gains', () => { if (!result.ok) return; expect(result.remaining).toBe(0); }); + + it('a single award spanning 3 levels (levelsGained * 10) can be fully spent across stats', () => { + // Mirrors awardXp's `stat_points_available += levelsGained * 10` for a + // 3-level jump from one XP award, then spends all 30 across 3 stats. + const { id } = createCompanion({ userId: 'multilevel-jump' }); + pinStat(id, 'DEBUGGING', 50); + pinStat(id, 'PATIENCE', 50); + pinStat(id, 'SNARK', 50); + const levelsGained = 3; + db.prepare('UPDATE companions SET stat_points_available = stat_points_available + ? WHERE id = ?') + .run(levelsGained * 10, id); + expect(getRow(id).stat_points_available).toBe(30); + + const r1 = applyStatAllocation(id, 'DEBUGGING', 10); + const r2 = applyStatAllocation(id, 'PATIENCE', 10); + const r3 = applyStatAllocation(id, 'SNARK', 10); + expect(r1.ok && r2.ok && r3.ok).toBe(true); + if (!r1.ok || !r2.ok || !r3.ok) return; + expect(r3.remaining).toBe(0); + expect(getRow(id).stat_points_available).toBe(0); + }); }); diff --git a/src/lib/allocate.ts b/src/lib/allocate.ts index 52ced64..f50b3b0 100644 --- a/src/lib/allocate.ts +++ b/src/lib/allocate.ts @@ -1,10 +1,10 @@ import { db } from '../db/schema.js'; import { loadCompanion } from './companion.js'; -import type { StatName } from './types.js'; +import { STAT_NAMES, type StatName } from './types.js'; export type AllocateResult = | { ok: true; spent: number; newValue: number; remaining: number } - | { ok: false; reason: 'no_companion' | 'no_points' | 'at_cap' | 'invalid_points' }; + | { ok: false; reason: 'no_companion' | 'no_points' | 'at_cap' | 'invalid_points' | 'invalid_stat' }; /** * Spend `points` stat points on `stat` for the companion identified by @@ -14,36 +14,54 @@ export type AllocateResult = * migration) via loadCompanion's bones fallback before writing. This prevents * NULL + n = NULL corruption that SQL-level increment would produce. * + * `stat` is re-validated against STAT_NAMES here (not just by the MCP handler) + * because it gets interpolated into the UPDATE column list below — this + * function, not its caller, is what has to guarantee that's safe. + * * `points` must be a positive integer; non-integer or non-positive values * return { ok: false, reason: 'invalid_points' }. + * + * The read-check-write runs inside db.transaction() (same convention as + * repairDuplicateCompanions in db/schema.ts) so a concurrent writer on this + * row — the MCP server and the UserPromptSubmit hook share one DB file — + * can't land a write between the SELECT and the UPDATE. */ export function applyStatAllocation(companionId: string, stat: StatName, points: number): AllocateResult { + if (!STAT_NAMES.includes(stat)) { + return { ok: false, reason: 'invalid_stat' }; + } if (!Number.isInteger(points) || points < 1) { return { ok: false, reason: 'invalid_points' }; } - const row = db.prepare('SELECT * FROM companions WHERE id = ?').get(companionId) as any; - if (!row) return { ok: false, reason: 'no_companion' }; + const allocate = db.transaction((): AllocateResult => { + const row = db.prepare('SELECT * FROM companions WHERE id = ?').get(companionId) as any; + if (!row) return { ok: false, reason: 'no_companion' }; + + const available = row.stat_points_available ?? 0; + if (available <= 0) return { ok: false, reason: 'no_points' }; - const available = row.stat_points_available ?? 0; - if (available <= 0) return { ok: false, reason: 'no_points' }; + // loadCompanion resolves NULL stat columns to deterministic bones values so + // a rescued companion with missing stat_ rows is handled correctly. + const companion = loadCompanion(row)!; + const currentValue = companion.stats[stat]; + if (currentValue >= 100) return { ok: false, reason: 'at_cap' }; - // loadCompanion resolves NULL stat columns to deterministic bones values so - // a rescued companion with missing stat_ rows is handled correctly. - const companion = loadCompanion(row)!; - const currentValue = companion.stats[stat]; - if (currentValue >= 100) return { ok: false, reason: 'at_cap' }; + const canSpend = Math.min(points, available, 100 - currentValue); + const newValue = currentValue + canSpend; - const canSpend = Math.min(points, available, 100 - currentValue); - const newValue = currentValue + canSpend; + // Column names are stat_debugging / stat_patience / etc. (not the bare stat + // name). `stat` is guaranteed to be one of STAT_NAMES at this point, so this + // interpolation can't produce an arbitrary column reference. + // Write a concrete value rather than `col = col + ?` so NULL-column rows are + // initialised to the bones value rather than producing NULL. + const dbCol = `stat_${stat.toLowerCase()}`; + db.prepare(`UPDATE companions SET ${dbCol} = ?, stat_points_available = stat_points_available - ? WHERE id = ?`) + .run(newValue, canSpend, companionId); - // Column names are stat_debugging / stat_patience / etc. (not the bare stat name). - // Write a concrete value rather than `col = col + ?` so NULL-column rows are - // initialised to the bones value rather than producing NULL. - const dbCol = `stat_${stat.toLowerCase()}`; - db.prepare(`UPDATE companions SET ${dbCol} = ?, stat_points_available = stat_points_available - ? WHERE id = ?`) - .run(newValue, canSpend, companionId); + const after = db.prepare('SELECT stat_points_available FROM companions WHERE id = ?').get(companionId) as any; + return { ok: true, spent: canSpend, newValue, remaining: after.stat_points_available ?? 0 }; + }); - const after = db.prepare('SELECT stat_points_available FROM companions WHERE id = ?').get(companionId) as any; - return { ok: true, spent: canSpend, newValue, remaining: after.stat_points_available ?? 0 }; + return allocate(); } diff --git a/src/server/index.ts b/src/server/index.ts index 6738b7f..3673513 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -450,22 +450,22 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { if (!row) { return { content: [{ type: "text", text: "No companion hatched yet! Use buddy_hatch first." }] }; } - if (!STAT_NAMES.includes(stat as any)) { - return { content: [{ type: "text", text: `Unknown stat "${stat}". Choose from: ${STAT_NAMES.join(', ')}.` }] }; - } - if (!Number.isInteger(points) || points < 1) { - return { content: [{ type: "text", text: `points must be a positive integer (got ${points}).` }] }; - } const result = applyStatAllocation(row.id, stat as any, points); if (!result.ok) { - if (result.reason === 'no_points') { - return { content: [{ type: "text", text: `${row.name} has no stat points available. Level up to earn more!` }] }; - } - if (result.reason === 'at_cap') { - return { content: [{ type: "text", text: `${stat} is already at 100 — can't go higher!` }] }; + switch (result.reason) { + case 'no_points': + return { content: [{ type: "text", text: `${row.name} has no stat points available. Level up to earn more!` }] }; + case 'at_cap': + return { content: [{ type: "text", text: `${stat} is already at 100 — can't go higher!` }] }; + case 'invalid_stat': + return { content: [{ type: "text", text: `Unknown stat "${stat}". Choose from: ${STAT_NAMES.join(', ')}.` }] }; + case 'invalid_points': + return { content: [{ type: "text", text: `points must be a positive integer (got ${points}).` }] }; + case 'no_companion': + default: + return { content: [{ type: "text", text: "No companion hatched yet! Use buddy_hatch first." }] }; } - return { content: [{ type: "text", text: "Something went wrong — no companion found." }] }; } const updatedRow = db.prepare("SELECT * FROM companions WHERE id = ?").get(row.id) as any; From 75b27faf22263d025f802f4d9fc3fc621d3eb39e Mon Sep 17 00:00:00 2001 From: Bowei Feng Date: Sat, 4 Jul 2026 00:59:25 +0800 Subject: [PATCH 4/4] fix(award): reload companion after level-up grants points; BEGIN IMMEDIATE for allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the "after awarding" half of the review's point 4 — the previous commits only fixed the allocating half: - awardXpAndRefresh built the companion by patching xp/level/mood onto the caller's pre-award row snapshot, so on the turn a level-up granted stat points, companion.availablePoints was stale (usually 0). observe and pet then wrote that stale value into the status file. Now re-reads the row after awarding so the returned companion — and the status file written from it — reflect the freshly granted points. Exported for testing (same precedent as recalcMood / self-healing.test.ts). - applyStatAllocation now runs its transaction as BEGIN IMMEDIATE. With two processes on one WAL DB (MCP server + UserPromptSubmit hook), a deferred read-then-write transaction can fail its write upgrade with SQLITE_BUSY_SNAPSHOT instead of waiting on busy_timeout; taking the write lock up front avoids that. Also reworded the transaction comment, which referenced a function that only exists on the fix/hatch branch. - Tests: two awardXpAndRefresh cases (multi-level jump grants levelsGained*10 fresh points; pre-existing unspent points survive a no-level-up award), plus an afterAll wipe so the xp_events rows these tests insert don't leak into later test files that clean up with a bare DELETE FROM companions (foreign_keys is ON; leaking broke doctor-insight, mode-orthogonality, and respawn-cleanup in the shared-DB suite run). Co-Authored-By: Claude Fable 5 --- src/__tests__/allocate.test.ts | 63 ++++++++++++++++++++++++++++++++-- src/lib/allocate.ts | 12 ++++--- src/server/index.ts | 9 +++-- 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/__tests__/allocate.test.ts b/src/__tests__/allocate.test.ts index 9c3fd97..ab5df24 100644 --- a/src/__tests__/allocate.test.ts +++ b/src/__tests__/allocate.test.ts @@ -1,16 +1,33 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterAll } from 'vitest'; import { initDb, db } from '../db/schema.js'; import { createCompanion, rescueCompanion, loadCompanion } from '../lib/companion.js'; import { applyStatAllocation } from '../lib/allocate.js'; import { STAT_NAMES } from '../lib/types.js'; +// Safe to import: server/index.ts only auto-starts when run directly +// (same pattern as self-healing.test.ts importing recalcMood). +import { awardXpAndRefresh } from '../server/index.js'; +import { XP_REWARDS, levelFromXp } from '../lib/leveling.js'; -beforeEach(() => { - initDb(); +function wipeCompanionData() { + // Children before parents: foreign_keys is ON (initReasoningSchema enables it). db.prepare('DELETE FROM xp_events').run(); db.prepare('DELETE FROM memories').run(); db.prepare('DELETE FROM sessions').run(); db.prepare('DELETE FROM evolution_history').run(); db.prepare('DELETE FROM companions').run(); +} + +beforeEach(() => { + initDb(); + wipeCompanionData(); +}); + +// Test files share one BUDDY_DB_PATH file (see vitest.config.ts). The +// awardXpAndRefresh tests insert xp_events rows; leave the DB empty so later +// files whose cleanup only does `DELETE FROM companions` don't hit the +// xp_events foreign key. +afterAll(() => { + wipeCompanionData(); }); function givePoints(id: string, pts: number) { @@ -303,3 +320,43 @@ describe('multi-level point gains', () => { expect(getRow(id).stat_points_available).toBe(0); }); }); + +// ─── refresh after awarding (awardXpAndRefresh) ────────────────────────────── +// +// The maintainer's review: "Reload the companion and update the status file +// after awarding or allocating points; current refresh logic uses stale point +// data." The allocating side is covered above; these cover the awarding side. +// awardXpAndRefresh used to patch xp/level/mood onto the caller's pre-award +// row snapshot, so on a level-up turn the returned companion carried the +// stale stat_points_available (observe/pet then wrote it to the status file). + +describe('awardXpAndRefresh point freshness', () => { + it('returns fresh availablePoints on the turn a level-up happens', () => { + const { id } = createCompanion({ userId: 'award-refresh' }); + // Park XP so the next observe event crosses at least one level boundary, + // while the DB still says level 1 — exactly the state awardXp levels from. + db.prepare('UPDATE companions SET xp = 500, level = 1 WHERE id = ?').run(id); + const staleRow = getRow(id); // handler-style snapshot taken BEFORE the award + + const { companion, xpResult } = awardXpAndRefresh(staleRow, 'observe'); + + expect(xpResult.leveledUp).toBe(true); + const levelsGained = levelFromXp(500 + XP_REWARDS.observe) - 1; + expect(levelsGained).toBeGreaterThan(0); + // The returned companion must reflect the points awardXp just granted — + // the same object the observe/pet handlers pass to writeBuddyStatus. + expect(companion.availablePoints).toBe(levelsGained * 10); + expect(getRow(id).stat_points_available).toBe(levelsGained * 10); + }); + + it('reflects pre-existing unspent points even when no level-up occurs', () => { + const { id } = createCompanion({ userId: 'award-noref' }); + givePoints(id, 7); + const staleRow = getRow(id); + + const { companion, xpResult } = awardXpAndRefresh(staleRow, 'session'); // +3 xp, stays level 1 + + expect(xpResult.leveledUp).toBe(false); + expect(companion.availablePoints).toBe(7); + }); +}); diff --git a/src/lib/allocate.ts b/src/lib/allocate.ts index f50b3b0..4be5a58 100644 --- a/src/lib/allocate.ts +++ b/src/lib/allocate.ts @@ -21,10 +21,12 @@ export type AllocateResult = * `points` must be a positive integer; non-integer or non-positive values * return { ok: false, reason: 'invalid_points' }. * - * The read-check-write runs inside db.transaction() (same convention as - * repairDuplicateCompanions in db/schema.ts) so a concurrent writer on this - * row — the MCP server and the UserPromptSubmit hook share one DB file — - * can't land a write between the SELECT and the UPDATE. + * The read-check-write runs as one BEGIN IMMEDIATE transaction. Two processes + * hold this DB file open (the MCP server and the UserPromptSubmit hook), and + * under WAL a deferred transaction that reads first can fail its write upgrade + * (SQLITE_BUSY_SNAPSHOT) if the other process commits in between — immediate + * mode takes the write lock up front, so busy_timeout does the waiting and the + * SELECT-then-UPDATE pair is atomic. */ export function applyStatAllocation(companionId: string, stat: StatName, points: number): AllocateResult { if (!STAT_NAMES.includes(stat)) { @@ -63,5 +65,5 @@ export function applyStatAllocation(companionId: string, stat: StatName, points: return { ok: true, spent: canSpend, newValue, remaining: after.stat_points_available ?? 0 }; }); - return allocate(); + return allocate.immediate(); } diff --git a/src/server/index.ts b/src/server/index.ts index 3673513..8b98dda 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -85,11 +85,16 @@ function awardXp(companionId: string, eventType: string): { newXp: number; newLe /** * Award XP, recalculate mood, update DB, and load companion — shared by observe + pet. */ -function awardXpAndRefresh(row: any, eventType: string, userIdOverride?: string) { +export function awardXpAndRefresh(row: any, eventType: string, userIdOverride?: string) { const xpResult = awardXp(row.id, eventType); const newMood = recalcMood(row.id, xpResult.leveledUp); db.prepare("UPDATE companions SET mood = ? WHERE id = ?").run(newMood, row.id); - const companion = loadCompanion({ ...row, mood: newMood, xp: xpResult.newXp, level: xpResult.newLevel }, userIdOverride)!; + // Re-read the row instead of patching xp/level/mood onto the caller's + // snapshot: awardXp may have just granted stat points for a level-up, and + // the stale snapshot's stat_points_available would otherwise flow into the + // returned companion (and from there into the status file the caller writes). + const freshRow = db.prepare("SELECT * FROM companions WHERE id = ?").get(row.id); + const companion = loadCompanion(freshRow, userIdOverride)!; return { companion, xpResult }; }