diff --git a/packages/extension/dist/index.js b/packages/extension/dist/index.js index c71c5d8a..de350518 100644 --- a/packages/extension/dist/index.js +++ b/packages/extension/dist/index.js @@ -13603,6 +13603,8 @@ var TERMINAL_STATES = { }; var QUESTION_TRIAGE = "Answer via crew_send if run context suffices; escalate to the user only for genuinely human decisions."; var TWO_FAILURES_RULE = "Two consecutive failures on the same task require escalation to the user."; +var READ_THE_REPORT = 'Read it via crew_run { op: "result", runId }.'; +var READ_ANY_PARTIAL_OUTPUT = 'Any partial output it produced is readable via crew_run { op: "result", runId }, though there may be none.'; function capitalize(s) { return s.length === 0 ? s : `${s[0].toUpperCase()}${s.slice(1)}`; } @@ -13664,16 +13666,16 @@ function formatDigest(e, lookup) { const state = event.payload.state; if (state === "failed") { const reason = row?.latestActivity ?? "see runtime"; - return `${capitalize(who)} FAILED: ${reason}. ${TWO_FAILURES_RULE}`; + return `${capitalize(who)} FAILED: ${reason}. ${TWO_FAILURES_RULE} ${READ_ANY_PARTIAL_OUTPUT}`; } if (state === "succeeded") { - return `${capitalize(who)} succeeded.`; + return `${capitalize(who)} succeeded. ${READ_THE_REPORT}`; } if (state === "cancelled") { - return `${capitalize(who)} was cancelled.`; + return `${capitalize(who)} was cancelled. ${READ_ANY_PARTIAL_OUTPUT}`; } if (state === "lost") { - return `${capitalize(who)} was lost (worker process died).`; + return `${capitalize(who)} was lost (worker process died). ${READ_ANY_PARTIAL_OUTPUT}`; } if (state === "working") { return `${capitalize(who)} started working.`; @@ -13711,6 +13713,7 @@ function formatDigest(e, lookup) { function attachMilestoneBridge(pi, monitor) { const tracker = new MilestoneTracker; const send = pi.sendMessage; + let warnedMissingSendMessage = false; return monitor.subscribeEvents((e, meta) => { const milestone = tracker.isMilestone(e); if (!milestone || meta.replay) { @@ -13724,6 +13727,9 @@ function attachMilestoneBridge(pi, monitor) { } if (typeof send === "function") { send.call(pi, digest, { deliverAs: "followUp", triggerTurn: true }); + } else if (!warnedMissingSendMessage) { + warnedMissingSendMessage = true; + pi.logger.warn("crew milestone bridge: this omp build exposes no sendMessage on ExtensionAPI, so run milestones will not be delivered to the leader for the rest of this session"); } } catch (err) { pi.logger.error("crew milestone bridge: digest injection failed", { diff --git a/packages/extension/src/milestones.test.ts b/packages/extension/src/milestones.test.ts index 0c37acbf..a890debe 100644 --- a/packages/extension/src/milestones.test.ts +++ b/packages/extension/src/milestones.test.ts @@ -204,6 +204,33 @@ test("failed digest contains the two-consecutive-failures rule and reason", () = expect(digest).toContain("Two consecutive failures"); }); +test("succeeded digest tells the leader how to read the report", () => { + // The whole point of a terminal notification: it says the run ended, not + // what it produced. Nothing pushes the report, so a digest that names the + // state and stops leaves the leader holding none of the output. The + // settled-turn digest below has always carried this instruction and had a + // test pinning it; the terminal digests carried neither, which is how the + // omission survived. + const digest = formatDigest(run("run-1", "succeeded"), ROWS); + expect(digest).toBeDefined(); + expect(digest).toContain("succeeded"); + expect(digest).toContain('crew_run { op: "result"'); +}); + +test("non-succeeded terminal digests offer partial output without promising it", () => { + // `run/result` accepts every terminal state and returns whatever visible + // text the journal accumulated, which for a run that died is usually + // partial and is sometimes null. The wording must offer it without + // claiming it exists. + for (const state of ["failed", "cancelled", "lost"]) { + const rows: RunLookup = { "run-1": { ...ROWS["run-1"], latestActivity: "process exited 1" } as MonitorRow }; + const digest = formatDigest(run("run-1", state), rows); + expect(digest, `${state} must produce a digest`).toBeDefined(); + expect(digest, `${state} must point at run/result`).toContain('crew_run { op: "result"'); + expect(digest, `${state} must not promise output exists`).toContain("there may be none"); + } +}); + test("a settled turn (runFlagsEvent turnSettled:true) is a milestone, once per settle episode", () => { const t = tracker(); // First settle: milestone. @@ -329,6 +356,43 @@ test("bridge injects a digest for a milestone and stays silent for noise", () => expect(sent.length).toBe(1); }); +test("an omp build with no sendMessage warns once, not silently and not per event", () => { + // The only branch in the bridge that could fail silently. `send` is looked + // up through a cast so a renamed or removed omp method degrades to "no + // digest"; without this warning every milestone for the session is dropped + // with nothing logged, because the surrounding catch only covers digests + // that throw. An omp version bump could switch the leader's notifications + // off entirely and look exactly like a quiet run. + const warnings: string[] = []; + const fakePi = { + logger: { debug() {}, info() {}, warn: (m: string) => warnings.push(m), error() {} }, + // deliberately no sendMessage + } as unknown as { logger: { [k: string]: (...a: unknown[]) => void } }; + const listeners: Array<(e: EventEnvelope, meta: { replay: boolean }) => void> = []; + const controller = { + subscribeEvents(cb: (e: EventEnvelope, meta: { replay: boolean }) => void) { + listeners.push(cb); + return () => {}; + }, + getState() { + return { rows: ROWS }; + }, + } as unknown as MonitorController; + attachMilestoneBridge(fakePi as never, controller); + + // Nothing logged before a milestone is actually due. + expect(warnings.length).toBe(0); + + listeners[0]?.(run("run-1", "succeeded"), { replay: false }); + expect(warnings.length).toBe(1); + expect(warnings[0]).toContain("sendMessage"); + + // A second milestone must not repeat it -- a long run would otherwise + // flood the log with one unchanging fact. + listeners[0]?.(run("run-2", "failed"), { replay: false }); + expect(warnings.length).toBe(1); +}); + test("a replayed milestone never injects a digest (stale-failure guard)", () => { const { sent, dispatch } = fakeBridge(); diff --git a/packages/extension/src/milestones.ts b/packages/extension/src/milestones.ts index 8ea86b8a..8cd5a9e9 100644 --- a/packages/extension/src/milestones.ts +++ b/packages/extension/src/milestones.ts @@ -33,6 +33,33 @@ const QUESTION_TRIAGE = "Answer via crew_send if run context suffices; escalate /** The rule text appended to a failed-run digest. */ const TWO_FAILURES_RULE = "Two consecutive failures on the same task require escalation to the user."; +/** + * How a leader reads a finished run's report. + * + * A terminal digest is a *notification*: it says the run ended, not what it + * produced. The report itself is only reachable through `run/result`, which + * nothing pushes -- so a digest that names the state and stops leaves the + * leader knowing a run succeeded and holding none of its output. The + * settled-turn digest below has always carried this instruction; the + * terminal ones did not, which is the asymmetry this closes. + */ +const READ_THE_REPORT = 'Read it via crew_run { op: "result", runId }.'; + +/** + * The same pointer for a run that did NOT succeed. + * + * `run/result` accepts every terminal state, not just `succeeded` + * (`service::orchestration::run_result` gates on `is_terminal`), and returns + * whatever visible text the journal accumulated before the run ended + * (`query::run_result_events_op`'s `final_text.or(chunk_text)`). For a + * failed, cancelled or lost run that is usually partial output and is often + * the most useful diagnostic there is -- but it can also be null, when the + * worker produced nothing before it died. The wording therefore offers it + * without promising it: a digest that said "read the output" and returned + * nothing would be worse than one that never mentioned it. + */ +const READ_ANY_PARTIAL_OUTPUT = 'Any partial output it produced is readable via crew_run { op: "result", runId }, though there may be none.'; + /** * The monitor's run rows, keyed by run id. Lets the digest name the run's * adapter / task instead of emitting bare ids. @@ -123,16 +150,16 @@ export function formatDigest(e: EventEnvelope, lookup: RunLookup): string | unde const state = event.payload.state; if (state === "failed") { const reason = row?.latestActivity ?? "see runtime"; - return `${capitalize(who)} FAILED: ${reason}. ${TWO_FAILURES_RULE}`; + return `${capitalize(who)} FAILED: ${reason}. ${TWO_FAILURES_RULE} ${READ_ANY_PARTIAL_OUTPUT}`; } if (state === "succeeded") { - return `${capitalize(who)} succeeded.`; + return `${capitalize(who)} succeeded. ${READ_THE_REPORT}`; } if (state === "cancelled") { - return `${capitalize(who)} was cancelled.`; + return `${capitalize(who)} was cancelled. ${READ_ANY_PARTIAL_OUTPUT}`; } if (state === "lost") { - return `${capitalize(who)} was lost (worker process died).`; + return `${capitalize(who)} was lost (worker process died). ${READ_ANY_PARTIAL_OUTPUT}`; } if (state === "working") { return `${capitalize(who)} started working.`; @@ -205,6 +232,26 @@ export function attachMilestoneBridge(pi: ExtensionAPI, monitor: MonitorControll } ).sendMessage; + // Warned at most once per session, the first time a digest is actually + // due. Without this the unavailable-API path is the only silent branch in + // the bridge: `send` is looked up through a cast so a renamed or removed + // omp method "degrades to no digest", the `typeof` guard below is then + // false forever, and every milestone for the whole session is dropped + // with nothing logged -- the surrounding catch only covers digests that + // throw. An omp version bump could switch the leader's notifications off + // entirely and look identical to a quiet run. + // + // Logged lazily -- at the first digest actually due -- rather than at + // attach time, and the reason is where the reader will be, not noise: + // this warning exists to explain an ABSENCE to someone who has noticed a + // milestone did not arrive, and that person is reading the log around + // the moment it should have fired. Attach can be hours earlier, when + // nobody is troubleshooting anything, so a warning there is filed before + // the question exists. Lazily also means a session that never reaches a + // milestone stays quiet, and once-per-session means a long run cannot + // flood the log with one unchanging fact. + let warnedMissingSendMessage = false; + return monitor.subscribeEvents((e: EventEnvelope, meta: EventDeliveryMeta) => { const milestone = tracker.isMilestone(e); if (!milestone || meta.replay) { @@ -218,6 +265,9 @@ export function attachMilestoneBridge(pi: ExtensionAPI, monitor: MonitorControll } if (typeof send === "function") { void send.call(pi, digest, { deliverAs: "followUp", triggerTurn: true }); + } else if (!warnedMissingSendMessage) { + warnedMissingSendMessage = true; + pi.logger.warn("crew milestone bridge: this omp build exposes no sendMessage on ExtensionAPI, so run milestones will not be delivered to the leader for the rest of this session"); } } catch (err) { pi.logger.error("crew milestone bridge: digest injection failed", {