diff --git a/src/__tests__/hook-dispatch-cli.test.ts b/src/__tests__/hook-dispatch-cli.test.ts new file mode 100644 index 00000000..65e4091f --- /dev/null +++ b/src/__tests__/hook-dispatch-cli.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; + +import { parseStdin } from '../hook-dispatch-cli.js'; + +describe('parseStdin', () => { + it('degrades malformed JSON to an empty object instead of null', () => { + // RED BASELINE: before the fix this returns null (short-circuiting all + // dispatch). After the fix it degrades to {} so non-stdin-dependent + // background handlers still run, and records diagnostics to debug.log. + const result = parseStdin('{broken', 'stop'); + expect(result).not.toBeNull(); + expect(result).toBeTypeOf('object'); + expect(result.hook_event_name).toBe('Stop'); + }); + + it('returns an empty object (plus event name) for blank STDIN', () => { + const result = parseStdin('', 'stop'); + expect(result).toEqual({ hook_event_name: 'Stop' }); + }); + + it('parses well-formed JSON and keeps its fields (regression)', () => { + const result = parseStdin('{"transcript_path":"/x"}', 'stop'); + expect(result.transcript_path).toBe('/x'); + expect(result.hook_event_name).toBe('Stop'); + }); + + it('maps lower-case event aliases to their canonical hook names', () => { + const result = parseStdin('', 'session-start'); + expect(result.hook_event_name).toBe('SessionStart'); + }); + + it('degrades JSON `null` to {} instead of throwing', () => { + // RED BASELINE: before the fix, JSON.parse('null') returns null, and the + // subsequent `stdin.hook_event_name` access throws TypeError in ESM strict + // mode, short-circuiting all dispatch (the very failure mode the original + // malformed-JSON fix was meant to prevent). + const result = parseStdin('null', 'stop'); + expect(Array.isArray(result)).toBe(false); + expect(result.hook_event_name).toBe('Stop'); + }); + + it('degrades JSON number to {} instead of throwing', () => { + // RED BASELINE: before the fix, JSON.parse('123') returns 123, and + // assigning a property on a number primitive throws TypeError in ESM + // strict mode. + const result = parseStdin('123', 'stop'); + expect(Array.isArray(result)).toBe(false); + expect(result.hook_event_name).toBe('Stop'); + }); + + it('degrades JSON array to a plain object (arrays are not records)', () => { + // RED BASELINE: before the fix, JSON.parse('[1,2]') returns an array, + // which is typeof 'object' but not a plain record — downstream handlers + // indexing string keys would misbehave. + const result = parseStdin('[1,2]', 'stop'); + expect(Array.isArray(result)).toBe(false); + expect(result).toEqual({ hook_event_name: 'Stop' }); + }); +}); diff --git a/src/__tests__/votes.test.ts b/src/__tests__/votes.test.ts index 420245f3..68101f20 100644 --- a/src/__tests__/votes.test.ts +++ b/src/__tests__/votes.test.ts @@ -190,6 +190,90 @@ describe('mergeDeltas', () => { expect(merged.votes['doc-a'].upvoted_count).toBe(0); expect(merged.votes['doc-a'].recalled_count).toBe(1); }); + + it('does not keep last_upvoted_at when upvoted_count is 0', () => { + const local: UserVotesV2 = { + version: 2, + votes: { + 'doc-bug': { + recalled_count: 1, + upvoted_count: 0, + last_recalled_at: '2026-09-15T00:00:00Z', + last_upvoted_at: '2026-09-15T00:00:00Z', + }, + }, + deltas: { 'doc-bug': { recalled_delta: 1, upvoted_delta: 0 } }, + }; + const remote: UserVotesV2 = { + version: 2, + votes: { + 'doc-bug': { recalled_count: 0, upvoted_count: 0, last_recalled_at: '' }, + }, + deltas: {}, + }; + + const merged = mergeDeltas(local, remote); + expect(merged.votes['doc-bug'].upvoted_count).toBe(0); + expect(merged.votes['doc-bug'].last_upvoted_at).toBeUndefined(); + }); + + it('does not keep last_recalled_at when recalled_count is 0', () => { + const local: UserVotesV2 = { + version: 2, + votes: { + 'doc-bug': { + recalled_count: 0, + upvoted_count: 0, + last_recalled_at: '2026-09-15T00:00:00Z', + }, + }, + deltas: { 'doc-bug': { recalled_delta: -1, upvoted_delta: 0 } }, + }; + const remote: UserVotesV2 = { + version: 2, + votes: { + 'doc-bug': { recalled_count: 1, upvoted_count: 0, last_recalled_at: '2026-09-05T00:00:00Z' }, + }, + deltas: {}, + }; + + const merged = mergeDeltas(local, remote); + expect(merged.votes['doc-bug'].recalled_count).toBe(0); + expect(merged.votes['doc-bug'].last_recalled_at).toBe(''); + }); + + it('preserves timestamps when counts stay positive (regression)', () => { + const local: UserVotesV2 = { + version: 2, + votes: { + 'doc-a': { + recalled_count: 5, + upvoted_count: 2, + last_recalled_at: '2026-06-10T00:00:00Z', + last_upvoted_at: '2026-06-10T00:00:00Z', + }, + }, + deltas: { 'doc-a': { recalled_delta: 3, upvoted_delta: 1 } }, + }; + const remote: UserVotesV2 = { + version: 2, + votes: { + 'doc-a': { + recalled_count: 2, + upvoted_count: 1, + last_recalled_at: '2026-06-05T00:00:00Z', + last_upvoted_at: '2026-06-04T00:00:00Z', + }, + }, + deltas: {}, + }; + + const merged = mergeDeltas(local, remote); + expect(merged.votes['doc-a'].recalled_count).toBe(5); + expect(merged.votes['doc-a'].upvoted_count).toBe(2); + expect(merged.votes['doc-a'].last_recalled_at).toBe('2026-06-10T00:00:00Z'); + expect(merged.votes['doc-a'].last_upvoted_at).toBe('2026-06-10T00:00:00Z'); + }); }); describe('syncVotesToTeam', () => { @@ -281,4 +365,24 @@ describe('recallFeedback', () => { // Should not throw await expect(recallFeedback({ negative: 'nonexistent' })).resolves.not.toThrow(); }); + + it('negative deletes last_upvoted_at when upvoted_count reaches 0', async () => { + const votesDir = path.join(tmpDir, '.teamai', 'votes'); + fs.mkdirSync(votesDir, { recursive: true }); + const votePath = path.join(votesDir, 'testuser.yaml'); + + // Set up: recalled_count=1, upvoted_count=1 with last_upvoted_at populated + await incrementRecalled(votePath, ['doc-zero']); + await incrementUpvoted(votePath, ['doc-zero']); + + const before = await loadUserVotes(votePath); + expect(before.votes['doc-zero'].upvoted_count).toBe(1); + expect(before.votes['doc-zero'].last_upvoted_at).toBeTruthy(); + + await recallFeedback({ negative: 'doc-zero' }); + + const after = YAML.parse(fs.readFileSync(votePath, 'utf-8')) as UserVotesV2; + expect(after.votes['doc-zero'].upvoted_count).toBe(0); + expect(after.votes['doc-zero'].last_upvoted_at).toBeUndefined(); + }); }); diff --git a/src/hook-dispatch-cli.ts b/src/hook-dispatch-cli.ts index 11e5a268..09cda79e 100644 --- a/src/hook-dispatch-cli.ts +++ b/src/hook-dispatch-cli.ts @@ -108,17 +108,37 @@ function spawnBackground( } /** Parse STDIN JSON and normalize the event name for downstream handlers. */ -function parseStdin(raw: string, event: string): Record | null { +export function parseStdin(raw: string, event: string): Record { let stdin: Record = {}; if (raw.trim()) { try { stdin = JSON.parse(raw); } catch { - log.debug(`hook-dispatch: failed to parse STDIN JSON for event=${event}`); - return null; + // Degrade to {} instead of short-circuiting: handlers that depend on + // stdin fields (votes-sync, contribute-check) self-skip when + // transcript_path is absent, while background handlers that don't read + // stdin (version-check, etc.) still get to run. Include a bounded + // preview so concurrent STDIN corruption is diagnosable in debug.log. + const preview = raw.length > 160 + ? `${raw.slice(0, 80)}...${raw.slice(-80)}` + : raw; + log.debug( + `hook-dispatch: failed to parse STDIN JSON for event=${event}` + + ` (len=${raw.length}, body=${JSON.stringify(preview)})`, + ); } } + // JSON.parse succeeds for non-object values (`null`, numbers, booleans, + // strings, arrays) that are not valid hook payloads. Without this guard, + // the next `stdin.hook_event_name` access/assign throws TypeError in ESM + // strict mode, which the outer try/catch swallows and short-circuits the + // whole dispatch — the exact failure the malformed-JSON path above was + // meant to prevent. Degrade any non-plain-object to {}. + if (!stdin || typeof stdin !== 'object' || Array.isArray(stdin)) { + stdin = {}; + } + // WorkBuddy/CodeBuddy may pass hook_event_name: "" — normalize to the // CLI-derived event name so downstream handlers (parseHookEvent, etc.) // can correctly determine the event type. @@ -168,7 +188,6 @@ export async function hookDispatchCli( try { const raw = await readStdin(); const stdin = parseStdin(raw, event); - if (stdin === null) return; // Provider-config gate: HTTP-only teams must not receive git-provider-only // hook prompts (contribute / mr-hint / votes). Prefer the project-scope diff --git a/src/votes.ts b/src/votes.ts index c308e6a5..ac7e9994 100644 --- a/src/votes.ts +++ b/src/votes.ts @@ -116,6 +116,12 @@ export async function incrementUpvoted(votePath: string, docIds: string[]): Prom /** * Merge local deltas into a remote votes snapshot. * Returns merged result with empty deltas. + * + * Scope note: this is a delta-merge, not a full-snapshot consistency sweep. + * The zeroed-counter timestamp cleanup below only runs for docs that appear + * in `local.deltas` — pure-remote docs with no local delta are copied as-is + * and are NOT re-validated. Callers needing a full consistency pass must run + * it separately; do not assume mergeDeltas sanitizes the entire result. */ export function mergeDeltas(local: UserVotesV2, remote: UserVotesV2): UserVotesV2 { const votes: Record = {}; @@ -144,6 +150,14 @@ export function mergeDeltas(local: UserVotesV2, remote: UserVotesV2): UserVotesV votes[docId].last_upvoted_at = localEntry.last_upvoted_at; } } + + // Consistency constraint: a zeroed counter must not retain a timestamp. + if (votes[docId].recalled_count === 0) { + votes[docId].last_recalled_at = ''; + } + if (votes[docId].upvoted_count === 0) { + delete votes[docId].last_upvoted_at; + } } return { version: 2, votes, deltas: {} }; @@ -204,11 +218,17 @@ export async function recallFeedback(opts: { positive?: string; negative?: strin return; } const existingDelta = data.deltas[opts.negative] ?? { recalled_delta: 0, upvoted_delta: 0 }; + const decrementedCount = entry.upvoted_count - 1; + const updatedEntry: VoteEntryV2 = { ...entry, upvoted_count: decrementedCount }; + // Consistency: a zeroed counter must not retain a timestamp. + if (decrementedCount === 0) { + delete updatedEntry.last_upvoted_at; + } const updated: UserVotesV2 = { ...data, votes: { ...data.votes, - [opts.negative]: { ...entry, upvoted_count: entry.upvoted_count - 1 }, + [opts.negative]: updatedEntry, }, deltas: { ...data.deltas,