fix(hn-monitor): guard against cron double-post (interim for cloud#1990)

.gitignore

@@ -3,3 +3,7 @@ node_modules/
 */.workforce/build/
 */persona.json
 .claude/
+
+# Relay VFS runtime state — rewritten on every reconcile cycle, never commit it
+.relay/
+memory/workspace/.relay/

hn-monitor/agent.ts

@@ -10,7 +10,7 @@
 import { defineAgent, type WorkforceCtx } from '@agentworkforce/runtime';
 import { slackClient } from '@relayfile/relay-helpers';
 
-interface Story {
+export interface Story {
   id: number;
   title: string;
   url: string;
@@ -37,11 +37,25 @@ export default defineAgent({
     return;
   }
 
-  await slackClient({ writebackTimeoutMs: 15_000 }).post(channel, await summarize(ctx, fresh));
-  await saveSeen(ctx, [...seen, ...fresh.map((s) => s.id)].slice(-200));
+  await postFreshStories(ctx, channel, seen, fresh);
   }
 });
 
+export async function postFreshStories(ctx: WorkforceCtx, channel: string, seen: number[], fresh: Story[]): Promise<void> {
+  // Claim the stories as seen BEFORE any long work. Cron delivery is
+  // at-least-once: a single tick can re-invoke this handler (cloud re-runs a
+  // delivery whose lease expires before it reports done — see
+  // AgentWorkforce/cloud#1990). Recording first means the re-invocation loads
+  // these ids as already-seen and stays silent, instead of posting the digest
+  // twice. The trade is that a failed summary/post drops that digest rather
+  // than risking a duplicate — the right call for a low-stakes twice-daily
+  // summary. (This is a stopgap; the durable fix is idempotent cron delivery in
+  // cloud#1990.)
+  await saveSeen(ctx, [...seen, ...fresh.map((s) => s.id)].slice(-200));
+  const digest = await summarize(ctx, fresh);
+  await slackClient({ writebackTimeoutMs: 15_000 }).post(channel, digest);
+}
+
 /** Top ~30 front-page stories via the public HN Algolia API. Returns [] on
  *  any network/parse failure so a transient outage doesn't crash the run. */
 async function fetchFrontPage(): Promise<Story[]> {

review/agent.ts

@@ -234,17 +234,58 @@ async function reviewAndFix(ctx: WorkforceCtx, pr: Pr): Promise<void> {
   // bot/reviewer comments resolved, nothing left for the agent to fix (the
   // READY sentinel). Every in-progress pass — opened, new commits, failing CI,
   // unresolved bot threads — stays silent so the channel isn't a play-by-play.
-  const channel = input(ctx, 'SLACK_CHANNEL');
-  if (channel && ready) {
-    const who = `<https://github.com/${pr.author}|@${pr.author}>`; // the PR opener
-    await postSlackPrUpdate(
-      ctx,
-      pr,
-      `:white_check_mark: ${who} — PR #${pr.number} in *${pr.owner}/${pr.repo}* is ready for your review: ${pr.url}`
-    );
+  if (ready) {
+    await announceReadyOnce(ctx, pr);
   }
 }
 
+// Announce "ready for your review" at most once per head commit. Re-reviews
+// fire on many webhooks (a later check completing, a new bot comment) and the
+// PR is no more "ready" than the last time we said so — re-announcing is the
+// duplicate-reminder noise. A genuinely new head SHA (fresh commits that pass)
+// is worth a new note, and postSlackPrUpdate threads it under the PR's first
+// message so the channel stays a single conversation per PR.
+const READY_ANNOUNCED_TAG = 'pr-reviewer:ready-announced';
+
+async function announceReadyOnce(ctx: WorkforceCtx, pr: Pr): Promise<void> {
+  const channel = input(ctx, 'SLACK_CHANNEL');
+  if (!channel) return;
+  if (pr.headSha && (await alreadyAnnouncedReady(ctx, pr))) return;
+  const who = `<https://github.com/${pr.author}|@${pr.author}>`; // the PR opener
+  await postSlackPrUpdate(
+    ctx,
+    pr,
+    `:white_check_mark: ${who} — PR #${pr.number} in *${pr.owner}/${pr.repo}* is ready for your review: ${pr.url}`
+  );
+  if (pr.headSha) await rememberReadyAnnounced(ctx, pr);
+}
+
+function readyAnnouncedTags(pr: Pr): string[] {
+  return [READY_ANNOUNCED_TAG, `pr:${pr.owner}/${pr.repo}#${pr.number}`];
+}
+
+async function alreadyAnnouncedReady(ctx: WorkforceCtx, pr: Pr): Promise<boolean> {
+  const items = await ctx.memory.recall(`pr-reviewer ready announced for ${pr.owner}/${pr.repo}#${pr.number}`, {
+    scope: 'workspace',
+    tags: readyAnnouncedTags(pr),
+    limit: 5,
+  });
+  return items.some((item) => {
+    try {
+      return (JSON.parse(item.content) as { headSha?: string }).headSha === pr.headSha;
+    } catch {
+      return false;
+    }
+  });
+}
+
+async function rememberReadyAnnounced(ctx: WorkforceCtx, pr: Pr): Promise<void> {
+  await ctx.memory.save(JSON.stringify({ headSha: pr.headSha }), {
+    scope: 'workspace',
+    tags: readyAnnouncedTags(pr),
+  });
+}
+
 export function reviewHarnessPrompt(pr: { owner: string; repo: string; number: number }): string {
   return [
     `Review pull request #${pr.number} in ${pr.owner}/${pr.repo}. The PR code is checked out in the current directory.`,
@@ -259,6 +300,12 @@ export function reviewHarnessPrompt(pr: { owner: string; repo: string; number: n
     `are often stale (already fixed by a later push). Reproduce the problem in the code as it is now, or skip it.`,
     `Make the smallest fix that addresses a demonstrated problem. Do not rewrite, restructure, or "harden" working`,
     `code beyond what the finding requires.`,
+    `Account for every bot and reviewer comment explicitly in your output under an "## Addressed comments" heading:`,
+    `one bullet per comment naming the bot/reviewer and what they raised, followed by either the file:line where you`,
+    `fixed it (e.g. "fixed in src/foo.ts:42") or, if you did not change anything, a one-line reason (stale —`,
+    `already handled by a later commit, or invalid because <reason>). This is how the comment authors and the human`,
+    `see that each thread was handled and exactly where, so be specific with the path and line; do not say a comment`,
+    `was addressed without pointing to the fix.`,
     `Verify every edit before you finish: run the repo's tests for the files you touched (install dependencies if`,
     `needed). When you change code that GENERATES commands, scripts, or queries, also execute a sample of the`,
     `generated output against a throwaway fixture — tests that only assert on the generated string prove nothing`,
@@ -285,9 +332,15 @@ async function verifyReadyForHumanReview(ctx: WorkforceCtx, pr: Pr): Promise<boo
       '--repo',
       `${pr.owner}/${pr.repo}`,
       '--json',
-      'mergeable,statusCheckRollup',
+      'state,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup,headRefOid',
     ], { cwd: ctx.sandbox.cwd, encoding: 'utf8', maxBuffer: 1024 * 1024 });
     const state = parsePrReadyState(stdout);
+    // Populate the head SHA from GitHub's authoritative current head. Some
+    // webhook payloads (e.g. check_run.completed) don't carry it, and without a
+    // SHA the ready-announce dedupe can't key on the commit and would re-ping.
+    if (typeof state.headRefOid === 'string' && state.headRefOid.trim()) {
+      pr.headSha = state.headRefOid.trim();
+    }
     const ready = prReadyStateAllowsHumanReview(state);
     if (!ready) {
       ctx.log?.('warn', 'pr-reviewer.ready-sentinel.downgraded', {
@@ -310,40 +363,83 @@ async function verifyReadyForHumanReview(ctx: WorkforceCtx, pr: Pr): Promise<boo
 }
 
 interface PullRequestReadyState {
+  state?: unknown;
   mergeable?: unknown;
+  mergeStateStatus?: unknown;
+  reviewDecision?: unknown;
   statusCheckRollup?: unknown;
+  headRefOid?: unknown;
 }
 
 function parsePrReadyState(stdout: string): PullRequestReadyState {
   const parsed = JSON.parse(stdout) as unknown;
   return parsed && typeof parsed === 'object' ? parsed as PullRequestReadyState : {};
 }
 
+/** Whether the PR is still open. A missing `state` is treated as open so the
+ *  check stays backward-compatible with callers that don't query it; only an
+ *  explicit non-OPEN value (MERGED/CLOSED) blocks the ready ping. */
+function prIsOpen(state: PullRequestReadyState): boolean {
+  const s = normalizeState(state.state);
+  return s === undefined || s === 'OPEN';
+}
+
 export function prReadyStateAllowsHumanReview(state: PullRequestReadyState): boolean {
+  // Never announce "ready for review" on a PR that merged or closed between the
+  // harness run and this check — that's the stale "ready" ping on a done PR.
+  if (!prIsOpen(state)) return false;
+  // A draft PR isn't up for review yet.
+  if (normalizeState(state.mergeStateStatus) === 'DRAFT') return false;
+  // No merge conflicts.
   if (state.mergeable !== 'MERGEABLE') return false;
+  // A reviewer or bot still asking for changes means it isn't the human's turn.
+  if (normalizeState(state.reviewDecision) === 'CHANGES_REQUESTED') return false;
+  // Checks must all be complete and passing. An *empty* rollup is ambiguous: it
+  // can mean "no CI configured" (fine to proceed) or "checks queued but not yet
+  // registered" (still pending — the original bug where `every` passed
+  // vacuously). Disambiguate with GitHub's own mergeStateStatus: only an empty
+  // rollup on a CLEAN PR (nothing blocking) counts as ready; a transient
+  // pre-registration window reports UNSTABLE/BLOCKED/UNKNOWN, not CLEAN.
   const checks = Array.isArray(state.statusCheckRollup) ? state.statusCheckRollup : [];
+  if (checks.length === 0) return normalizeState(state.mergeStateStatus) === 'CLEAN';
   return checks.every(checkPassedAndComplete);
 }
 
+// A check that's complete and not blocking. SUCCESS and NEUTRAL pass; SKIPPED
+// passes too — a conditionally-skipped job is GitHub's "not applicable", not a
+// failure, and must not pin the PR out of "ready" forever (it also mirrors the
+// trigger's ciFailed(), which treats skipped as non-failing).
 function checkPassedAndComplete(check: unknown): boolean {
   if (!check || typeof check !== 'object') return false;
   const record = check as Record<string, unknown>;
   const state = normalizeState(record.state);
-  if (state) return state === 'SUCCESS' || state === 'NEUTRAL';
+  if (state) return state === 'SUCCESS' || state === 'NEUTRAL' || state === 'SKIPPED';
   const status = normalizeState(record.status);
   const conclusion = normalizeState(record.conclusion);
-  return status === 'COMPLETED' && (conclusion === 'SUCCESS' || conclusion === 'NEUTRAL');
+  return status === 'COMPLETED' && (conclusion === 'SUCCESS' || conclusion === 'NEUTRAL' || conclusion === 'SKIPPED');
 }
 
 function normalizeState(value: unknown): string | undefined {
   return typeof value === 'string' ? value.trim().toUpperCase() : undefined;
 }
 
 function describeNotReadyState(state: PullRequestReadyState): string {
+  if (!prIsOpen(state)) {
+    return `state=${String(state.state ?? 'missing')}`;
+  }
+  if (normalizeState(state.mergeStateStatus) === 'DRAFT') {
+    return 'mergeStateStatus=DRAFT';
+  }
   if (state.mergeable !== 'MERGEABLE') {
     return `mergeable=${String(state.mergeable ?? 'missing')}`;
   }
+  if (normalizeState(state.reviewDecision) === 'CHANGES_REQUESTED') {
+    return 'reviewDecision=CHANGES_REQUESTED';
+  }
   const checks = Array.isArray(state.statusCheckRollup) ? state.statusCheckRollup : [];
+  if (checks.length === 0) {
+    return `no status checks reported and mergeStateStatus=${String(state.mergeStateStatus ?? 'missing')} (not CLEAN)`;
+  }
   const blocked = checks.find((check) => !checkPassedAndComplete(check));
   if (!blocked || typeof blocked !== 'object') {
     return 'statusCheckRollup contains a non-passing check';

tests/hn-monitor.test.mjs

@@ -0,0 +1,36 @@
+import assert from 'node:assert/strict';
+import test from 'node:test';
+
+import { postFreshStories } from '../.test-build/hn-monitor/agent.js';
+
+test('postFreshStories claims fresh story ids before summarizing', async () => {
+  const events = [];
+  const saved = [];
+  const ctx = {
+    memory: {
+      async save(content, opts) {
+        events.push('save');
+        saved.push({ content, opts });
+      },
+    },
+    llm: {
+      async complete() {
+        events.push('llm');
+        throw new Error('summary failed');
+      },
+    },
+  };
+
+  await assert.rejects(
+    postFreshStories(ctx, 'C123', [10], [
+      { id: 20, title: 'Agent Workforce cron leases', url: 'https://example.com/20', points: 42 },
+    ]),
+    /summary failed/,
+  );
+
+  assert.deepEqual(events, ['save', 'llm']);
+  assert.deepEqual(saved, [{
+    content: JSON.stringify([10, 20]),
+    opts: { tags: ['hn-monitor:seen'], scope: 'workspace' },
+  }]);
+});

tests/review-agent.test.mjs

@@ -152,6 +152,67 @@ test('prReadyStateAllowsHumanReview requires mergeable PRs with only completed p
   }), false);
 });
 
+test('prReadyStateAllowsHumanReview never reports a merged or closed PR ready', () => {
+  const passingChecks = [{ __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }];
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'MERGED', mergeable: 'MERGEABLE', statusCheckRollup: passingChecks,
+  }), false);
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'CLOSED', mergeable: 'MERGEABLE', statusCheckRollup: passingChecks,
+  }), false);
+  // An explicit OPEN state still passes when everything else is green.
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: passingChecks,
+  }), true);
+});
+
+test('prReadyStateAllowsHumanReview treats an empty (not-yet-registered) check rollup as not ready', () => {
+  // Empty rollup + not CLEAN = checks queued but not yet registered → pending.
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'BLOCKED', statusCheckRollup: [],
+  }), false);
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'UNKNOWN',
+  }), false);
+  // No mergeStateStatus at all is also not-ready (can't confirm nothing's pending).
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [],
+  }), false);
+});
+
+test('prReadyStateAllowsHumanReview allows a no-CI repo (empty rollup) only when GitHub reports CLEAN', () => {
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'CLEAN', statusCheckRollup: [],
+  }), true);
+});
+
+test('prReadyStateAllowsHumanReview treats skipped checks as non-blocking', () => {
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', statusCheckRollup: [
+      { __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' },
+      { __typename: 'CheckRun', name: 'e2e-conditional', status: 'COMPLETED', conclusion: 'SKIPPED' },
+      { __typename: 'StatusContext', context: 'optional-gate', state: 'SKIPPED' },
+    ],
+  }), true);
+});
+
+test('prReadyStateAllowsHumanReview holds back drafts and changes-requested PRs', () => {
+  const passingChecks = [{ __typename: 'CheckRun', name: 'unit', status: 'COMPLETED', conclusion: 'SUCCESS' }];
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', mergeStateStatus: 'DRAFT', statusCheckRollup: passingChecks,
+  }), false);
+  assert.equal(prReadyStateAllowsHumanReview({
+    state: 'OPEN', mergeable: 'MERGEABLE', reviewDecision: 'CHANGES_REQUESTED', statusCheckRollup: passingChecks,
+  }), false);
+});
+
+test('reviewHarnessPrompt requires accounting for each bot/reviewer comment with a location', () => {
+  const prompt = reviewHarnessPrompt({ owner: 'AgentWorkforce', repo: 'agents', number: 7 });
+  assert.match(prompt, /## Addressed comments/);
+  assert.match(prompt, /file:line where you/);
+  assert.match(prompt, /do not say a comment\s+was addressed without pointing to the fix/);
+});
+
 // Cloud only mounts an integration's relayfile subtree from its `scope` (or
 // from triggers — and this persona has github triggers only). A scope-less
 // `slack: {}` mounts nothing, so every slackClient() post was written to