From 4b045f4b5a13ef7317b8f4fb44fd94ec9a414db7 Mon Sep 17 00:00:00 2001 From: Stefan Preu Date: Sat, 31 Oct 2026 02:54:52 +0100 Subject: [PATCH 1/6] fix(sdk): keep closure clocks when an update repeats an item's terminal state An update naming the close status was routed to the closure writer without checking whether the item was already closed. With force, the writer re-stamped closed_at and the store stamped updated_at on every retry, so a repeated identical closure moved the recorded closure time (GH-1628); the same payload on an open item was a no-op. commitItemMutations reuses update(), so transactions shared the defect. routeCloseStatusUpdate now reads the stored item first. When the requested close status, trimmed reason and completed_at already describe it, the remaining fields take the ordinary skipNoop update path and the result reports terminal_state_unchanged; a bare identical update returns no changed fields without --force. A different reason is still a repeat closure behind the existing force gate. The helpers live in repeated-terminal-state.ts to keep update.ts under the line ceiling. Regression tests cover the forced identical update, the unforced identical update with a padded reason, other fields applied without re-stamping closed_at, the preserved repeat-closure route, and a mixed-batch commitItemMutations retry; three of the four update tests fail without the fix. --- src/sdk/lifecycle/repeated-terminal-state.ts | 131 ++++++++++++++++++ src/sdk/lifecycle/update.ts | 38 ++++- .../commands/lifecycle/update-command.spec.ts | 125 +++++++++++++++++ tests/unit/sdk/item-transaction.spec.ts | 48 +++++++ 4 files changed, 341 insertions(+), 1 deletion(-) create mode 100644 src/sdk/lifecycle/repeated-terminal-state.ts diff --git a/src/sdk/lifecycle/repeated-terminal-state.ts b/src/sdk/lifecycle/repeated-terminal-state.ts new file mode 100644 index 000000000..6c7adca54 --- /dev/null +++ b/src/sdk/lifecycle/repeated-terminal-state.ts @@ -0,0 +1,131 @@ +/** + * @module sdk/lifecycle/repeated-terminal-state + * + * An update that names the close status for an item that already holds that + * status with the same reason is not a transition. Routing it to the closure + * writer re-stamped `closed_at` and `updated_at` on every retry (GH-1628) + * while the same payload on an open item was a no-op. These helpers detect + * the repeated state and apply only the remaining fields. + */ +import { + buildItemNotFoundError, + locateItem, + normalizeStatusInput, + readLocatedItem, + toItemRecord, + transferMutationStdinTokenPolicy, + type GlobalOptions, + type RuntimeStatusRegistry, +} from "../runtime-primitives.js"; +import type { ItemMetadata } from "../../types/index.js"; +import type { CloseRouteContext, UpdateCommandOptions, UpdateResult } from "./update.js"; + +/** Receipt for an update that repeats the item's current terminal state, so no closure clock is re-stamped (GH-1628). */ +export const TERMINAL_STATE_UNCHANGED_WARNING = "terminal_state_unchanged"; + +function normalizeTerminalReasonInput( + value: string | undefined, +): string | undefined { + const trimmed = value?.trim(); + return trimmed !== undefined && trimmed.length > 0 ? trimmed : undefined; +} + +/** + * True when the requested close status, reason and completion time already + * describe the stored item. Such an update is not a transition: routing it to + * the closure writer would re-stamp `closed_at` and `updated_at` on every + * retry (GH-1628), while the same identical payload on an open item is a + * no-op. + */ +export function describesCurrentTerminalState( + metadata: ItemMetadata, + options: UpdateCommandOptions, + statusRegistry: RuntimeStatusRegistry, +): boolean { + if ( + normalizeStatusInput(metadata.status, statusRegistry) !== + statusRegistry.close_status + ) { + return false; + } + const requestedReason = normalizeTerminalReasonInput(options.closeReason); + if ( + requestedReason !== undefined && + requestedReason !== normalizeTerminalReasonInput(metadata.close_reason) + ) { + return false; + } + return ( + options.completedAt === undefined || + options.completedAt === metadata.completed_at + ); +} + +/** Read the stored metadata of the item an update is routing, refusing with the shared not-found guidance. */ +export async function readRoutedItemMetadata( + context: CloseRouteContext, +): Promise { + const located = await locateItem( + context.pmRoot, + context.id, + context.settings.id_prefix, + context.settings.item_format, + context.typeRegistry.type_to_folder, + ); + if (!located) { + throw await buildItemNotFoundError( + context.pmRoot, + context.id, + context.settings.id_prefix, + context.typeRegistry.type_to_folder, + ); + } + const { document } = await readLocatedItem(located, { + schema: context.settings.schema, + }); + return document.metadata; +} + +/** + * Apply the non-terminal fields of an update whose close status, reason and + * completion time already match the stored item. The closure clocks and the + * terminal history stay untouched; the remaining fields take the ordinary + * update path, which skips the write when nothing changes. + */ +export async function applyRepeatedTerminalStateUpdate( + context: CloseRouteContext, + metadata: ItemMetadata, + otherFieldKeys: readonly string[], + runUpdate: ( + id: string, + options: UpdateCommandOptions, + global: GlobalOptions, + ) => Promise, +): Promise { + if (otherFieldKeys.length === 0) { + return { + item: toItemRecord(metadata), + changed_fields: [], + warnings: [ + ...context.workflowTransitionWarnings, + TERMINAL_STATE_UNCHANGED_WARNING, + ], + }; + } + const remainingOptions = transferMutationStdinTokenPolicy(context.options, { + ...context.options, + status: undefined, + closeReason: undefined, + completedAt: undefined, + }); + const result = await runUpdate(context.id, remainingOptions, context.global); + return { + item: result.item, + changed_fields: result.changed_fields, + warnings: [ + ...context.workflowTransitionWarnings, + ...result.warnings, + TERMINAL_STATE_UNCHANGED_WARNING, + ], + }; +} diff --git a/src/sdk/lifecycle/update.ts b/src/sdk/lifecycle/update.ts index 5c4b25de8..ad172c87d 100644 --- a/src/sdk/lifecycle/update.ts +++ b/src/sdk/lifecycle/update.ts @@ -3,6 +3,11 @@ * * Implements the pm update command surface and its agent-facing runtime behavior. */ +import { + applyRepeatedTerminalStateUpdate, + describesCurrentTerminalState, + readRoutedItemMetadata, +} from "./repeated-terminal-state.js"; import { assertInitializedTracker } from "../environment/tracker-preflight.js"; import { isLiteralMutationInput } from "../../core/item/literal-mutation-input.js"; import { resolveMutationStdinTokenFields } from "../../core/item/parse.js"; @@ -1333,13 +1338,26 @@ interface UpdateMutationContext { pmRoot: string; } -interface CloseRouteContext { +/** Inputs shared by the close-status routing of an update and its repeated-terminal-state check. */ +export interface CloseRouteContext { + /** Normalized update options of the invocation. */ options: UpdateCommandOptions; + /** Which update fields the invocation set, keyed by option name. */ fieldFlags: Record; + /** Runtime status registry that names the configured close status. */ statusRegistry: RuntimeStatusRegistry; + /** Workflow transition warnings collected before routing. */ workflowTransitionWarnings: readonly string[]; + /** Global CLI options forwarded to nested commands. */ global: GlobalOptions; + /** Target item id. */ id: string; + /** Resolved tracker root. */ + pmRoot: string; + /** Tracker settings in effect. */ + settings: Awaited>; + /** Item type registry used to locate the item document. */ + typeRegistry: ReturnType; } // `rank` is a legacy alias for `order`; keep it out of the scalar loop if the @@ -2081,6 +2099,21 @@ async function routeCloseStatusUpdate( key !== "completedAt", ) .map(([key]) => key); + const currentMetadata = await readRoutedItemMetadata(context); + if ( + describesCurrentTerminalState( + currentMetadata, + context.options, + context.statusRegistry, + ) + ) { + return applyRepeatedTerminalStateUpdate( + context, + currentMetadata, + otherFieldKeys, + runUpdate, + ); + } const routeWarnings: string[] = []; let preChangedFields: string[] = []; if (otherFieldKeys.length > 0) { @@ -3343,6 +3376,9 @@ async function runUpdateWithContext( workflowTransitionWarnings, global, id, + pmRoot, + settings, + typeRegistry, }); if (routedClose) { return routedClose; diff --git a/tests/unit/commands/lifecycle/update-command.spec.ts b/tests/unit/commands/lifecycle/update-command.spec.ts index 0f77eac02..f13d82eee 100644 --- a/tests/unit/commands/lifecycle/update-command.spec.ts +++ b/tests/unit/commands/lifecycle/update-command.spec.ts @@ -810,6 +810,131 @@ describe("runUpdate", () => { }); }); + describe("repeated terminal state updates (GH-1628)", () => { + async function readTerminalSnapshot(context: TempPmContext, id: string) { + const settings = await readSettings(context.pmPath); + const located = await locateItem( + context.pmPath, + id, + settings.id_prefix, + settings.item_format, + ); + if (!located) { + throw new Error(`item ${id} not located`); + } + const bytes = await readFile(located.itemPath, "utf8"); + const history = context.runCli(["history", id, "--json", "--full"], { expectJson: true }); + expect(history.code).toBe(0); + const persisted = context.runCli(["get", id, "--json"], { expectJson: true }); + expect(persisted.code).toBe(0); + const item = (persisted.json as { item: Record }).item; + return { + bytes, + historyLength: (history.json as { history: unknown[] }).history.length, + closed_at: item.closed_at as string, + updated_at: item.updated_at as string, + close_reason: item.close_reason as string, + title: item.title as string, + }; + } + + async function createClosedTask(context: TempPmContext, title: string): Promise { + const id = createTask(context, title); + const closed = await runUpdate( + id, + { status: "closed", closeReason: "done the first time" }, + { path: context.pmPath }, + ); + expect(closed.warnings).toContain("auto_routed_from_update_to_close"); + // Make a re-stamped clock observable. + await new Promise((resolve) => setTimeout(resolve, 5)); + return id; + } + + it("keeps closure and mutation clocks when a forced update repeats the current terminal state", async () => { + await withTempPmPath(async (context) => { + const id = await createClosedTask(context, "forced-repeat-closed"); + const before = await readTerminalSnapshot(context, id); + const result = await runUpdate( + id, + { + title: before.title, + status: "closed", + closeReason: before.close_reason, + force: true, + }, + { path: context.pmPath }, + ); + expect(result.changed_fields).toEqual([]); + expect(result.warnings).toContain("terminal_state_unchanged"); + expect(result.warnings).not.toContain("auto_routed_from_update_to_close"); + const after = await readTerminalSnapshot(context, id); + expect(after.closed_at).toBe(before.closed_at); + expect(after.updated_at).toBe(before.updated_at); + expect(after.historyLength).toBe(before.historyLength); + expect(after.bytes).toBe(before.bytes); + }); + }); + + it("treats an identical terminal update without --force as a no-op instead of a refused re-closure", async () => { + await withTempPmPath(async (context) => { + const id = await createClosedTask(context, "unforced-repeat-closed"); + const before = await readTerminalSnapshot(context, id); + const result = await runUpdate( + id, + { status: "closed", closeReason: ` ${before.close_reason} ` }, + { path: context.pmPath }, + ); + expect(result.changed_fields).toEqual([]); + expect(result.warnings).toContain("terminal_state_unchanged"); + const after = await readTerminalSnapshot(context, id); + expect(after.closed_at).toBe(before.closed_at); + expect(after.updated_at).toBe(before.updated_at); + expect(after.historyLength).toBe(before.historyLength); + }); + }); + + it("applies the other fields of a repeated terminal update without re-stamping closed_at", async () => { + await withTempPmPath(async (context) => { + const id = await createClosedTask(context, "repeat-closed-with-title"); + const before = await readTerminalSnapshot(context, id); + const result = await runUpdate( + id, + { status: "closed", closeReason: before.close_reason, title: "renamed after closure", force: true }, + { path: context.pmPath }, + ); + expect(result.changed_fields).toEqual(["title"]); + expect(result.warnings).toContain("terminal_state_unchanged"); + expect(result.warnings).not.toContain("auto_routed_from_update_to_close"); + const after = await readTerminalSnapshot(context, id); + expect(after.title).toBe("renamed after closure"); + expect(after.closed_at).toBe(before.closed_at); + expect(after.updated_at).not.toBe(before.updated_at); + expect(after.historyLength).toBe(before.historyLength + 1); + }); + }); + + it("still routes a repeat closure with a different reason through the closure writer", async () => { + await withTempPmPath(async (context) => { + const id = await createClosedTask(context, "repeat-closed-new-reason"); + const before = await readTerminalSnapshot(context, id); + await expect( + runUpdate(id, { status: "closed", closeReason: "a different reason" }, { path: context.pmPath }), + ).rejects.toMatchObject({ exitCode: EXIT_CODE.CONFLICT }); + const forced = await runUpdate( + id, + { status: "closed", closeReason: "a different reason", force: true }, + { path: context.pmPath }, + ); + expect(forced.warnings).toContain("auto_routed_from_update_to_close"); + expect(forced.changed_fields).toEqual(expect.arrayContaining(["close_reason"])); + const after = await readTerminalSnapshot(context, id); + expect(after.close_reason).toBe("a different reason"); + expect(after.historyLength).toBe(before.historyLength + 1); + }); + }); + }); + it("refuses a reasonless auto-routed terminal transition under required-reason governance", async () => { await withTempPmPath(async (context) => { const id = createTask(context, "auto-route-close-no-reason"); diff --git a/tests/unit/sdk/item-transaction.spec.ts b/tests/unit/sdk/item-transaction.spec.ts index 45c2b04a3..240a84dbb 100644 --- a/tests/unit/sdk/item-transaction.spec.ts +++ b/tests/unit/sdk/item-transaction.spec.ts @@ -490,6 +490,54 @@ describe("SDK bulk item-mutation transactions (GH-613)", () => { }); }); + it("retries an identical closed update in a mixed batch without touching its closure clocks (GH-1628)", async () => { + await withTempPmPath(async (context) => { + const closedTarget = createSeedItem(context, "bulk-identical-closed"); + const preClose = context.runCli( + ["close", closedTarget, "Closed before the batch", "--author", "bulk-seed", "--json"], + { expectJson: true }, + ); + expect(preClose.code).toBe(0); + const openTarget = createSeedItem(context, "bulk-identical-open"); + const before = await readItem(context.pmPath, closedTarget); + await new Promise((resolve) => setTimeout(resolve, 5)); + + const mutations = [ + { + op: "update" as const, + id: closedTarget, + options: { + title: before.title, + status: "closed", + closeReason: before.close_reason, + force: true, + }, + }, + { op: "update" as const, id: openTarget, options: { title: "Renamed in the batch" } }, + ]; + const first = await commitItemMutations({ + pmRoot: context.pmPath, + transactionId: "bulk-batch-identical-closed", + author: "bulk-agent", + mutations, + }); + expect(first.status).toBe("committed"); + const retried = await commitItemMutations({ + pmRoot: context.pmPath, + transactionId: "bulk-batch-identical-closed-retry", + author: "bulk-agent", + mutations, + }); + expect(retried.status).toBe("committed"); + + const after = await readItem(context.pmPath, closedTarget); + expect(after.closed_at).toBe(before.closed_at); + expect(after.updated_at).toBe(before.updated_at); + expect(after.close_reason).toBe("Closed before the batch"); + expect((await readItem(context.pmPath, openTarget)).title).toBe("Renamed in the batch"); + }); + }); + it("records evidence, closes, and releases one claimed item atomically", async () => { await withTempPmPath(async (context) => { const target = createSeedItem(context, "atomic-completion"); From 83f4ce01b122565734a199ea1e8e4c0b4280079f Mon Sep 17 00:00:00 2001 From: Stefan Preu Date: Sat, 31 Oct 2026 02:54:53 +0100 Subject: [PATCH 2/6] fix(cli): disclose the whole pm next response under its ceiling and encode null columns by position pm next reported filters.token_budget 640 while nothing enforced it, and --token-budget replaced the universal default ceiling instead of bounding the selection beside it, so the complete response (9,148 bytes on this tracker) carried no cost receipt at all (GH-1627). The TOON codec lost the compact table for ready and containers because omitting one all-null column emitted the full fifteen-name column_order list, so those sections fell back to escaped tag JSON. --token-budget is no longer a read-output cost alias for next; the universal default ceiling keeps binding and a requested selection budget audits the complete envelope, so read_output reports budget_source default, budget_tokens and estimated_tokens beside truncation.ready_budget unless an enforced intent receipt already discloses the cost. filters.token_budget is the requested selection ceiling or null. PmTableCellEncoding records omitted null columns as null_column_index and decoders still accept the legacy column_order. Measured on the real tracker: pm next --limit 5 drops from 9,148 to 8,718 bytes and from 182 to 76 escaped-quote sequences, every section list-encodes tags, and --token-budget 640 --json carries the receipt (budget 6000, estimated 4119). The contracts snapshot drops the stale --output-budget alias on next. --- docs/AGENT_OUTPUT_ENCODINGS.md | 12 +++++ src/sdk/output/table-rows.ts | 28 ++++++++-- src/sdk/query/next.ts | 24 +++++++-- src/sdk/read-output-contracts.ts | 21 ++++++++ src/sdk/read-output/surfaces.ts | 3 +- tests/fixtures/contracts/full.json | 9 +--- .../unit/commands/query/next-command.spec.ts | 54 +++++++++++++++++++ tests/unit/sdk/output/table-rows.spec.ts | 42 +++++++++++++++ 8 files changed, 176 insertions(+), 17 deletions(-) diff --git a/docs/AGENT_OUTPUT_ENCODINGS.md b/docs/AGENT_OUTPUT_ENCODINGS.md index 8160d000c..2b6135f0f 100644 --- a/docs/AGENT_OUTPUT_ENCODINGS.md +++ b/docs/AGENT_OUTPUT_ENCODINGS.md @@ -24,6 +24,10 @@ including scalar cells, so strings that resemble JSON remain distinguishable. `absent` contains `[row_index, column_name]` pairs for missing fields. Null padding does not imply an explicit null in the original object. Column order follows first occurrence; row order, ranking, identities, and nested facts are preserved. +A compact candidate omits columns that are null in every row and names them in +`null_columns`; `null_column_index` records each omitted column's original +position, so restoration is exact without repeating the full column list +(decoders still accept the legacy `column_order`). A producer-owned `_encoding` field prevents this transformation. Package authors can use `encodePmTableRows` and `decodePmTableRows` from @@ -117,3 +121,11 @@ answer returns an explicit omission receipt and recovery; it does not pretend that omitted work is an empty population. Use `--output-budget unbounded` to request the complete result. Markdown, NDJSON, and other command-specific streaming budgets remain separate contracts. + +`pm next --token-budget` is a selection ceiling for `recommended` and `ready`, +not a response cost: it never replaces the universal ceiling, and requesting it +audits the complete envelope under that ceiling, so `read_output` discloses +`budget_source: default`, `budget_tokens`, and the measured `estimated_tokens` +beside `truncation.ready_budget`. `filters.token_budget` is `null` unless a +selection budget was requested. An enforced `--for` intent receipt already +discloses the whole-response cost and suppresses the duplicate audit. diff --git a/src/sdk/output/table-rows.ts b/src/sdk/output/table-rows.ts index 7c0ecf7dc..bd9f1f4f8 100644 --- a/src/sdk/output/table-rows.ts +++ b/src/sdk/output/table-rows.ts @@ -15,7 +15,13 @@ export interface PmTableCellEncoding { list_columns?: Record; /** Columns omitted from every encoded row because every source value is explicit null. */ null_columns?: string[]; - /** Original first-occurrence column order when omitted null columns need reinsertion. */ + /** Zero-based original position of each omitted null column, parallel to `null_columns`. */ + null_column_index?: number[]; + /** + * Legacy full column list accepted on decode. Encoders emit + * `null_column_index` instead, so one omitted column no longer costs the + * whole column list in metadata. + */ column_order?: string[]; /** Missing field coordinates, expressed as [zero-based row, column name]. */ absent: Array<[number, string]>; @@ -74,10 +80,26 @@ export function encodePmTableRows( ); return { rows: encoded, encoding: { json_columns: jsonColumns, absent, ...(listColumns.length ? { list_columns: Object.fromEntries(listColumns.map(column => [column, ";"])) } : {}), - ...(nullColumns.length ? { null_columns: nullColumns, column_order: columns } : {}), + ...(nullColumns.length + ? { null_columns: nullColumns, null_column_index: nullColumns.map((column) => columns.indexOf(column)) } + : {}), } }; } +/** Reinsert omitted null columns at their recorded original positions. */ +function restoreColumnOrder(encodedColumns: readonly string[], encoding: PmTableCellEncoding): string[] { + const nullColumns = encoding.null_columns ?? []; + const positions = encoding.null_column_index ?? []; + const columns = [...encodedColumns]; + const insertions = nullColumns + .map((column, index) => ({ column, position: positions[index] })) + .filter((entry): entry is { column: string; position: number } => typeof entry.position === "number") + .sort((left, right) => left.position - right.position); + for (const { column, position } of insertions) columns.splice(Math.min(position, columns.length), 0, column); + for (const column of nullColumns) if (!columns.includes(column)) columns.push(column); + return columns; +} + /** * Restore rows emitted by {@link encodePmTableRows}. Metadata is explicit, so * strings resembling JSON stay strings unless their column declares JSON. @@ -103,7 +125,7 @@ export function decodePmTableRows( }), ); for (const column of table.encoding.null_columns ?? []) cells.set(column, null); - const columns = table.encoding.column_order ?? [...cells.keys()]; + const columns = table.encoding.column_order ?? restoreColumnOrder(Object.keys(row), table.encoding); return Object.fromEntries(columns.filter(column => cells.has(column)).map(column => [column, cells.get(column)!])); }); } diff --git a/src/sdk/query/next.ts b/src/sdk/query/next.ts index 68d9738dd..348d61e52 100644 --- a/src/sdk/query/next.ts +++ b/src/sdk/query/next.ts @@ -198,7 +198,12 @@ export interface NextResult { include_decisions: boolean; include_gates?: boolean; include_containers?: boolean; - token_budget: number; + /** + * Requested recommended+ready selection ceiling, or null when none was + * requested. The complete response is bounded by the universal output + * budget and disclosed by its `read_output` receipt (GH-1627). + */ + token_budget: number | null; }; /** Value that configures or reports suggestions for this contract. */ suggestions?: string[]; @@ -599,7 +604,7 @@ async function finalizeNextResult(params: { author: string; packingBudget: number; featureStore: ContextSignalStoreReadResult; - tokenBudget?: number; + tokenBudget: number | null; hasIntentBudget: boolean; }): Promise { const warnings = [ @@ -616,7 +621,7 @@ async function finalizeNextResult(params: { params.blockedRows, params.gateCount, ); - if (params.tokenBudget !== undefined) { + if (params.tokenBudget !== null) { applyNextReadyBudget(params.result, params.tokenBudget, params.hasIntentBudget); } if (params.explainRanking) { @@ -736,6 +741,14 @@ function resolveNextCallerAuthor( ); } +/** Echo the requested selection ceiling; null when the caller requested none. */ +function requestedSelectionBudget( + options: NextOptions, + resolved: number, +): number | null { + return options.tokenBudget === undefined ? null : resolved; +} + /** Implements `pm next`: computes the ranked ready/blocked actionable queues and a single recommended next item with rationale for the public runtime surface. */ export async function runNext( options: NextOptions, @@ -755,6 +768,7 @@ export async function runNext( options.tokenBudget, Math.max(192, limit * 128), ); + const requestedTokenBudget = requestedSelectionBudget(options, tokenBudget); const blockedLimit = parseNextLimit( options.blockedLimit, "--blocked-limit", @@ -885,7 +899,7 @@ export async function runNext( include_decisions: options.includeDecisions === true, include_gates: options.includeGates === true, include_containers: options.includeContainers === true, - token_budget: tokenBudget, + token_budget: requestedTokenBudget, }, truncation: Object.keys(truncation).length > 0 ? truncation : undefined, }; @@ -903,7 +917,7 @@ export async function runNext( author: callerAuthor, packingBudget: tokenBudget, featureStore, - tokenBudget: options.tokenBudget === undefined ? undefined : tokenBudget, + tokenBudget: requestedTokenBudget, hasIntentBudget: typeof options.for === "string", }); } diff --git a/src/sdk/read-output-contracts.ts b/src/sdk/read-output-contracts.ts index bb0e394b6..6e7c7f6c5 100644 --- a/src/sdk/read-output-contracts.ts +++ b/src/sdk/read-output-contracts.ts @@ -1562,6 +1562,26 @@ function hasContextIntentBudgetReceipt( ); } +/** + * A `next` selection budget (`--token-budget`) bounds only the recommended and + * ready rows, so the complete envelope is audited under the universal ceiling + * whenever one is requested. An enforced intent receipt already discloses the + * whole-response cost and suppresses the duplicate audit (GH-1627). + */ +function requiresNextSelectionBudgetAudit( + resolved: PmResolvedReadOutputDimensions, + options: Record, + result: Record, +): boolean { + if (resolved.command !== "next") return false; + if ((options.tokenBudget ?? options.token_budget) === undefined) return false; + return !( + isRecord(result.context_intent) && + result.context_intent.command === "next" && + result.context_intent.within_budget === true + ); +} + /** Disclose ceilings not otherwise carried by a canonical, intent, or session control. */ function readOutputBudgetReceiptFields( budget: @@ -1649,6 +1669,7 @@ function requiresReadOutputAudit( (resolved.command === "context" && resolved.cost?.source === "legacy" && !hasContextIntentBudgetReceipt(result, resolved.cost.value)) || + requiresNextSelectionBudgetAudit(resolved, options, result) || options.outputRowContract === true || options.output_row_contract === true || resolved.canonical_options_used!.includes("--output-include") || diff --git a/src/sdk/read-output/surfaces.ts b/src/sdk/read-output/surfaces.ts index 8afaf2976..0bcdfd294 100644 --- a/src/sdk/read-output/surfaces.ts +++ b/src/sdk/read-output/surfaces.ts @@ -40,9 +40,10 @@ export const LEGACY_FLAGS_BY_COMMAND: Readonly< cost: ["--token-budget"], encoding: ["--format"], }, + // `--token-budget` on next is the recommended+ready selection ceiling, not a + // response cost dimension; the universal default ceiling keeps binding. next: { amount: ["--blocked-limit", "--limit"], - cost: ["--token-budget"], encoding: ["--format"], }, health: { diff --git a/tests/fixtures/contracts/full.json b/tests/fixtures/contracts/full.json index e1f5f4ad0..f9ad56201 100644 --- a/tests/fixtures/contracts/full.json +++ b/tests/fixtures/contracts/full.json @@ -44280,14 +44280,7 @@ "canonical_option": "--output-budget", "dimension": "cost", "inapplicable_reason": null, - "legacy_aliases": [ - { - "flag": "--token-budget", - "migration_hint": "--token-budget is a compatibility alias; prefer --output-budget .", - "semantics": "replacement", - "visibility": "hidden_alias" - } - ] + "legacy_aliases": [] }, "encoding": { "applicable": true, diff --git a/tests/unit/commands/query/next-command.spec.ts b/tests/unit/commands/query/next-command.spec.ts index ae11f8916..7c983ac8a 100644 --- a/tests/unit/commands/query/next-command.spec.ts +++ b/tests/unit/commands/query/next-command.spec.ts @@ -149,6 +149,60 @@ describe("runNext", () => { }); }); + it("reports a null selection budget unless requested and audits the complete envelope under the universal ceiling (GH-1627)", async () => { + await withTempPmPath(async (context) => { + createItem(context, { title: "Audited ready work", priority: "1" }); + createItem(context, { title: "Audited second work", priority: "2" }); + const unbudgeted = await runNext({}, { path: context.pmPath }); + expect(unbudgeted.filters.token_budget).toBeNull(); + expect(unbudgeted.truncation?.ready_budget).toBeUndefined(); + const budgeted = await runNext({ tokenBudget: 5000 }, { path: context.pmPath }); + expect(budgeted.filters.token_budget).toBe(5000); + const plain = context.runCli(["next", "--json"], { expectJson: true }); + expect(plain.code).toBe(0); + expect((plain.json as { read_output?: unknown }).read_output).toBeUndefined(); + for (const format of ["json", "toon"] as const) { + const audited = context.runCli( + ["next", "--token-budget", "5000", "--format", format], + { expectJson: format === "json" }, + ); + expect(audited.code).toBe(0); + if (format === "json") { + const envelope = audited.json as { + read_output: Record; + filters: { token_budget: number | null }; + }; + expect(envelope.filters.token_budget).toBe(5000); + expect(envelope.read_output).toMatchObject({ + command: "next", + budget_source: "default", + within_budget: true, + result_omitted: false, + }); + expect(envelope.read_output.legacy_aliases_used).not.toContain("--token-budget"); + expect(typeof envelope.read_output.budget_tokens).toBe("number"); + expect(envelope.read_output.estimated_tokens as number).toBeGreaterThan(0); + expect(envelope.read_output.estimated_tokens as number).toBeLessThanOrEqual( + envelope.read_output.budget_tokens as number, + ); + } else { + expect(audited.stdout).toContain("read_output:"); + expect(audited.stdout).toContain('budget_source: "default"'); + expect(audited.stdout).toContain("token_budget: 5000"); + } + } + const intent = context.runCli(["next", "--for", "execute", "--json"], { expectJson: true }); + expect(intent.code).toBe(0); + const intentEnvelope = intent.json as { + context_intent?: { within_budget?: boolean }; + read_output?: unknown; + }; + if (intentEnvelope.context_intent?.within_budget === true) { + expect(intentEnvelope.read_output).toBeUndefined(); + } + }); + }); + it("resolves caller identity through settings and unknown fallbacks", () => { const previousAuthor = process.env.PM_AUTHOR; delete process.env.PM_AUTHOR; diff --git a/tests/unit/sdk/output/table-rows.spec.ts b/tests/unit/sdk/output/table-rows.spec.ts index 894330eb8..f0bf50a96 100644 --- a/tests/unit/sdk/output/table-rows.spec.ts +++ b/tests/unit/sdk/output/table-rows.spec.ts @@ -180,6 +180,48 @@ describe("lossless SDK table cells", () => { }); }); + it("encodes omitted null columns by position so one null column no longer costs the whole column list (GH-1627)", () => { + const rows = Array.from({ length: 5 }, (_, index) => ({ + id: `pm-${index}`, + title: `Ready ${index}`, + priority: 1, + order: null, + deadline: index === 1 ? "2099-01-01T00:00:00.000Z" : null, + assignee: index === 2 ? "agent" : null, + tags: ["agent-ux", "github"], + rank: index + 1, + })); + const compact = encodePmTableRows(rows, { compact: true }); + expect(compact.encoding.null_columns).toEqual(["order"]); + expect(compact.encoding.null_column_index).toEqual([3]); + expect(compact.encoding.column_order).toBeUndefined(); + expect(decodePmTableRows(compact)).toEqual(rows); + expect(Object.keys(decodePmTableRows(compact)[0]!)).toEqual(Object.keys(rows[0]!)); + const plain = encodePmTableRows(rows, { compact: false }); + const size = (table: PmEncodedTableRows): number => + Buffer.byteLength(encode({ ready: table.rows, ready_encoding: table.encoding })); + expect(size(compact)).toBeLessThan(size(plain)); + const rendered = formatBuiltInOutput({ ready: rows }, "toon"); + expect(rendered).toContain("list_columns"); + expect(rendered).toContain("null_column_index"); + expect(rendered).not.toContain("column_order"); + expect(restoreTables(decode(rendered))).toEqual({ ready: rows }); + }); + + it("still restores rows that carry legacy column_order metadata", () => { + const table: PmEncodedTableRows = { + rows: [{ b: 1 }], + encoding: { json_columns: [], absent: [], null_columns: ["a", "c"], column_order: ["a", "b", "c"] }, + }; + expect(decodePmTableRows(table)).toEqual([{ a: null, b: 1, c: null }]); + expect(Object.keys(decodePmTableRows(table)[0]!)).toEqual(["a", "b", "c"]); + const positional: PmEncodedTableRows = { + rows: [{ b: 1 }], + encoding: { json_columns: [], absent: [], null_columns: ["a", "c"], null_column_index: [0, 2] }, + }; + expect(Object.keys(decodePmTableRows(positional)[0]!)).toEqual(["a", "b", "c"]); + }); + it("compacts agenda null columns and safe string lists with exact restoration", () => { const rows = Array.from({ length: 10 }, (_, index) => ({ item_id: `pm-${index}`, kind: "deadline", event_title: null, event_end: null, From b453457f4365ae05fd11f0440112d0f991808247 Mon Sep 17 00:00:00 2001 From: Stefan Preu Date: Sat, 31 Oct 2026 02:54:53 +0100 Subject: [PATCH 3/6] feat(cli): compact lean refusals and expose the claim holder as typed fields Refusals printed a single-line message twice (headline and What happened; title and detail in JSON), kept the generic requirement, rationale and pm --help examples under --lean, and echoed the caller's own argv, so a lean text refusal was larger than the default one (813 against 798 bytes for a claim conflict). The holder of a claimed item existed only inside the sentence (GH-1610). The SDK declares the constant boilerplate once (PM_GENERIC_REFUSAL_GUIDANCE, PM_RECOVERY_ECHO_FIELDS, isGenericRefusal*, projectLeanRecoveryPayload), and both lean projections consume it: lean text keeps the headline, next steps, a specific requirement or rationale, verifier findings and specific recovery; lean JSON drops generic required/examples and the recovery echo on top of title/why. A single-line message is no longer repeated under What happened in any mode. already_claimed_by carries item_id, claimed_by, claimed_at (latest assignee change in history) and claim_source (claim|assignment) through PmCliErrorContext, the JSON envelope and the classification; text renders Held by: ( since