diff --git a/.github/workflows/advisor-hooks.yml b/.github/workflows/advisor-hooks.yml new file mode 100644 index 000000000..dff275ccf --- /dev/null +++ b/.github/workflows/advisor-hooks.yml @@ -0,0 +1,18 @@ +name: Test Advisor hooks + +on: + pull_request: + paths: + - "advisor/hooks/**" + - ".github/workflows/advisor-hooks.yml" + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 20 + - name: Test consult persistence + run: node --test advisor/hooks/record-consult.test.mjs diff --git a/advisor/hooks/record-consult.sh b/advisor/hooks/record-consult.sh index 4b9f6be57..8205c1161 100755 --- a/advisor/hooks/record-consult.sh +++ b/advisor/hooks/record-consult.sh @@ -24,11 +24,11 @@ if [[ "$SUBAGENT_TYPE" != "advisor-subagent" || "$STATUS" != "completed" ]]; the fi NOW=$(date -u +%Y-%m-%dT%H:%M:%SZ) -advisor_state_update '.consults = ((.consults // 0) + 1) | .last_consult_at = $now' --arg now "$NOW" -rm -f "$PENDING_FILE" - DESCRIPTION=$(jq -r '.description // "Advisor consult"' <<< "$HOOK_INPUT") -SUMMARY=$(jq -r '.summary // empty' <<< "$HOOK_INPUT" | head -c 6000) +# Consume jq fully before bounding the text. With pipefail, piping a long +# summary to head can terminate jq with SIGPIPE and lose the consult record. +SUMMARY=$(jq -r '.summary // empty' <<< "$HOOK_INPUT") +SUMMARY=$(head -c 6000 <<< "$SUMMARY") { printf '## %s — %s\n\n' "$NOW" "$DESCRIPTION" if [[ -n "$SUMMARY" ]]; then @@ -36,6 +36,10 @@ SUMMARY=$(jq -r '.summary // empty' <<< "$HOOK_INPUT" | head -c 6000) else printf '_No summary captured._\n\n' fi -} >> "$LOG_FILE" +} >> "$LOG_FILE" || exit 1 + +# Mark the consult complete only after its advice was persisted successfully. +advisor_state_update '.consults = ((.consults // 0) + 1) | .last_consult_at = $now' --arg now "$NOW" +rm -f "$PENDING_FILE" exit 0 diff --git a/advisor/hooks/record-consult.test.mjs b/advisor/hooks/record-consult.test.mjs new file mode 100644 index 000000000..c69d2fa40 --- /dev/null +++ b/advisor/hooks/record-consult.test.mjs @@ -0,0 +1,64 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, existsSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { spawnSync } from "node:child_process"; + +const hook = join(dirname(fileURLToPath(import.meta.url)), "record-consult.sh"); +function consult(t, input, { brokenLog = false } = {}) { + const root = mkdtempSync(join(tmpdir(), "advisor-consult-")); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, ".cursor", "advisor"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "state.json"), JSON.stringify({ enabled: true, conversation_id: "current", consults: 0 })); + writeFileSync(join(dir, "pending"), ""); + if (brokenLog) mkdirSync(join(dir, "log.md")); + const result = spawnSync("bash", [hook], { + input: JSON.stringify({ conversation_id: "current", subagent_type: "advisor-subagent", status: "completed", ...input }), + encoding: "utf8", env: { ...process.env, CURSOR_PROJECT_DIR: root }, timeout: 5000, + }); + return { ...result, dir, state: JSON.parse(readFileSync(join(dir, "state.json"))), pending: existsSync(join(dir, "pending")) }; +} + +for (const [name, summary] of [ + ["short", "Advice remains intact"], + ["long", "A".repeat(500_000)], + ["unicode", "é".repeat(3000) + "tail"], +]) { + test(`persists ${name} consult before marking it complete`, (t) => { + const result = consult(t, { summary, description: "Review" }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.state.consults, 1); + assert.equal(result.pending, false); + const log = readFileSync(join(result.dir, "log.md")); + const expected = Buffer.from(summary).subarray(0, 6000); + assert.equal(log.includes(expected), true); + assert.equal(log.subarray(log.indexOf(Buffer.from("Review\n\n")) + 8).length, expected.length + 2); + }); +} + +test("records absent summaries without dropping the consult", (t) => { + const result = consult(t, {}); + assert.equal(result.status, 0, result.stderr); + assert.match(readFileSync(join(result.dir, "log.md"), "utf8"), /No summary captured/); + assert.equal(result.state.consults, 1); +}); + +test("failed log writes leave the consult pending and uncounted", (t) => { + const result = consult(t, { summary: "advice" }, { brokenLog: true }); + assert.notEqual(result.status, 0); + assert.equal(result.state.consults, 0); + assert.equal(result.pending, true); +}); + +for (const input of [{ status: "error" }, { status: "aborted" }, { conversation_id: "other" }, { subagent_type: "worker" }]) { + test(`ignores unrelated or unfinished consult ${JSON.stringify(input)}`, (t) => { + const result = consult(t, { summary: "advice", ...input }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.state.consults, 0); + assert.equal(result.pending, true); + assert.equal(existsSync(join(result.dir, "log.md")), false); + }); +}