fix(approvals): interpolate request id in fallback command

itsuzef · web-flow · commit beb499b4d1c0 · 2026-05-31T18:59:43.000+01:00
Fix approval fallback text so exec and plugin approval messages render a concrete request id in the chat copy-paste command instead of the literal <id> placeholder. This makes the Reply with: /approve ... line directly usable for owners while keeping the existing approval resolver contract unchanged. Proof: - git diff --check origin/main...HEAD - pnpm test src/infra/exec-approval-forwarder.test.ts src/infra/plugin-approval-forwarder.test.ts src/plugin-sdk/approval-renderers.test.ts - .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main - CI run 26720052738 passed Thanks @itsuzef.
diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts
@@ -602,7 +602,7 @@ describe("exec approval forwarder", () => {
     expect(text).toContain("🔒 Exec approval required");
     expect(text).toContain("Command: `echo hello`");
     expect(text).toContain("Expires in: 5s");
-    expect(text).toContain("Reply with: /approve <id> allow-once|allow-always|deny");
+    expect(text).toContain("Reply with: /approve req-1 allow-once|allow-always|deny");
   });
 
   it("includes command analysis warnings in fallback delivery text", () => {
@@ -639,7 +639,7 @@ describe("exec approval forwarder", () => {
     ).resolves.toBe(true);
     await Promise.resolve();
     const text = getFirstDeliveryText(deliver);
-    expect(text).toContain("Reply with: /approve <id> allow-once|deny");
+    expect(text).toContain("Reply with: /approve req-1 allow-once|deny");
     expect(text).not.toContain("allow-once|allow-always|deny");
     expect(text).toContain("Allow Always is unavailable");
   });
diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts
@@ -286,7 +286,7 @@ export function buildExecApprovalRequestMessage(request: ExecApprovalRequest, no
       ? "Background mode note: non-interactive runs cannot wait for chat approvals; use pre-approved policy (allow-always or ask=off)."
       : "Background mode note: non-interactive runs cannot wait for chat approvals; the effective policy still requires per-run approval unless ask=off.",
   );
-  lines.push(`Reply with: /approve <id> ${decisionText}`);
+  lines.push(`Reply with: /approve ${request.id} ${decisionText}`);
   if (!allowedDecisions.includes("allow-always")) {
     lines.push(
       "Allow Always is unavailable because the effective policy requires approval every time.",
diff --git a/src/infra/plugin-approval-forwarder.test.ts b/src/infra/plugin-approval-forwarder.test.ts
@@ -190,7 +190,7 @@ describe("plugin approval forwarding", () => {
       expect(result).toBe(true);
       await flushPendingDelivery();
       const payload = firstDeliveredPayload(deliver);
-      expect(payload?.text).toContain("Reply with: /approve <id> allow-once|deny");
+      expect(payload?.text).toContain("Reply with: /approve plugin-req-1 allow-once|deny");
       expect(payload?.text).not.toContain("allow-always");
       expect(payload?.presentation).toEqual({
         blocks: [
diff --git a/src/infra/plugin-approvals.ts b/src/infra/plugin-approvals.ts
@@ -108,9 +108,9 @@ export function buildPluginApprovalRequestMessage(
   const expiresIn = Math.max(0, Math.round((request.expiresAtMs - nowMsValue) / 1000));
   lines.push(`Expires in: ${expiresIn}s`);
   lines.push(
-    `Reply with: /approve <id> ${resolvePluginApprovalRequestAllowedDecisions(request.request).join(
-      "|",
-    )}`,
+    `Reply with: /approve ${request.id} ${resolvePluginApprovalRequestAllowedDecisions(
+      request.request,
+    ).join("|")}`,
   );
   return lines.join("\n");
 }
diff --git a/src/plugin-sdk/approval-renderers.test.ts b/src/plugin-sdk/approval-renderers.test.ts
@@ -142,7 +142,7 @@ describe("plugin-sdk/approval-renderers", () => {
         nowMs: 1_000,
       }),
       textExpected: (text: string) =>
-        expect(text).toContain("Reply with: /approve <id> allow-once|deny"),
+        expect(text).toContain("Reply with: /approve plugin-approval-123 allow-once|deny"),
       presentationExpected: {
         blocks: [
           {

Original file line number	Diff line number	Diff line change
`@@ -142,7 +142,7 @@ describe("plugin-sdk/approval-renderers", () => {`
`142`	`142`	`nowMs: 1_000,`
`143`	`143`	`}),`
`144`	`144`	`textExpected: (text: string) =>`
`145`		`- expect(text).toContain("Reply with: /approve <id> allow-once\|deny"),`
	`145`	`+ expect(text).toContain("Reply with: /approve plugin-approval-123 allow-once\|deny"),`
`146`	`146`	`presentationExpected: {`
`147`	`147`	`blocks: [`
`148`	`148`	`{`