Skip to content

Commit 35c1f42

Browse files
kriszypclaude
andcommitted
fix(cli): refresh expired agent tokens; fix --once approval hang
Address heskew's two remaining non-blocking review notes on #1553: - `harper agent` hard-failed on an expired stored operation token instead of self-healing via the refresh_token, unlike cliOperations.ts. Extract the refresh logic into a shared `refreshExpiredOperationToken` helper in cliOperations.ts and call it from both cliOperations and agentCli, so the two transports can't drift again. - `--once` against a real TTY drains stdin via readAllStdin() before the first turn; if that turn then needed approval, resolveApprovals() built a new readline on the already-ended stdin and question() never resolved. Track actual stdin consumption (opts.stdinConsumed) instead of relying on isTTY, and fail loudly in that case like the non-TTY path already does. Refs #1553 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent e58094e commit 35c1f42

2 files changed

Lines changed: 56 additions & 36 deletions

File tree

bin/agentCli.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import * as readline from 'node:readline';
1818
import { loadCredentials, normalizeTarget } from './cliCredentials.ts';
19+
import { refreshExpiredOperationToken } from './cliOperations.ts';
1920
import { httpRequest } from '../utility/common_utils.ts';
2021
import { getHdbPid } from '../utility/processManagement/processManagement.js';
2122
import { initConfig, getConfigPath } from '../config/configUtils.ts';
@@ -32,6 +33,7 @@ interface CliOptions {
3233
json: boolean;
3334
once: boolean;
3435
message?: string;
36+
stdinConsumed?: boolean;
3537
}
3638

3739
interface Connection {
@@ -65,7 +67,7 @@ export async function runAgentCli(argv: string[]): Promise<number> {
6567

6668
let connection: Connection;
6769
try {
68-
connection = resolveConnection(opts);
70+
connection = await resolveConnection(opts);
6971
} catch (err) {
7072
console.error((err as Error).message);
7173
return 1;
@@ -78,6 +80,7 @@ export async function runAgentCli(argv: string[]): Promise<number> {
7880
} else if (opts.once || !process.stdin.isTTY) {
7981
// Piped input / --once: treat all of stdin as a single prompt.
8082
const piped = await readAllStdin();
83+
opts.stdinConsumed = true;
8184
if (!piped.trim()) {
8285
console.error('No prompt provided.');
8386
return 1;
@@ -139,7 +142,7 @@ function parseArgs(argv: string[]): CliOptions {
139142
* or Bearer auth, else the local domain socket. Mirrors the core of `cliOperations` without the
140143
* deploy-specific transport concerns.
141144
*/
142-
function resolveConnection(opts: CliOptions): Connection {
145+
async function resolveConnection(opts: CliOptions): Promise<Connection> {
143146
const credentials = loadCredentials();
144147
const rawTarget =
145148
opts.target || process.env.HARPER_CLI_TARGET || process.env.CLI_TARGET || (credentials && credentials.last_target);
@@ -170,6 +173,7 @@ function resolveConnection(opts: CliOptions): Connection {
170173
} else {
171174
const tokens = credentials?.targets?.[resolved];
172175
if (tokens?.operation_token) {
176+
await refreshExpiredOperationToken(options, tokens, resolved);
173177
options.headers.Authorization = `Bearer ${tokens.operation_token}`;
174178
} else if (username) {
175179
options.headers.Authorization = `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`;
@@ -325,9 +329,10 @@ class AgentClient {
325329
private async resolveApprovals(session: any, rl?: readline.Interface): Promise<boolean> {
326330
const pending = (session.pendingApprovals || []).filter((a: any) => a && !a.resolved);
327331
if (!pending.length) return false;
328-
// One-shot/piped paths have already read stdin to EOF (or there is no TTY), so a fresh readline
329-
// on process.stdin would never resolve `question()` and the turn would hang forever. Fail loudly.
330-
if (!rl && !process.stdin.isTTY) {
332+
// One-shot/piped paths have already read stdin to EOF (no TTY, or `--once` drained a real
333+
// TTY via readAllStdin), so a fresh readline on process.stdin would never resolve
334+
// `question()` and the turn would hang forever. Fail loudly instead.
335+
if (!rl && (this.opts.stdinConsumed || !process.stdin.isTTY)) {
331336
throw new Error(
332337
'Tool approval required, but no interactive terminal is available; re-run interactively to approve.'
333338
);

bin/cliOperations.ts

Lines changed: 46 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ function redactCredentials(req: any): any {
249249
return redacted;
250250
}
251251

252-
export { cliOperations, buildRequest, redactCredentials };
252+
export { cliOperations, buildRequest, redactCredentials, refreshExpiredOperationToken };
253253
const PREPARE_OPERATION: any = {
254254
deploy_component: async (req) => {
255255
if (req.package) {
@@ -327,6 +327,50 @@ function resolveTarget(req, allCredentials) {
327327
);
328328
}
329329

330+
/**
331+
* If `tokens.operation_token` is expired and a `refresh_token` is on hand, refreshes it via
332+
* `refresh_operation_token`, persisting the new token to the credentials file and updating
333+
* `tokens.operation_token` in place. Shared by `cliOperations` and any other CLI transport
334+
* (e.g. `harper agent`) authenticating with stored `harper login` tokens, so refresh behavior
335+
* stays in one place instead of drifting between callers.
336+
*/
337+
async function refreshExpiredOperationToken(
338+
options: any,
339+
tokens: { operation_token: string; refresh_token: string },
340+
lookupKey: string
341+
): Promise<void> {
342+
if (!tokens.refresh_token || !isJWTExpired(tokens.operation_token)) return;
343+
console.error('Operation token expired, attempting to refresh...');
344+
try {
345+
// Always use the standard operation timeout for this call, even when the caller's
346+
// own options carry the longer SSE timeout (e.g. a deploy_component retry) — the
347+
// refresh call itself is a small, fast request, not the streaming operation.
348+
const refreshOptions = { ...options, timeout: CLI_OPERATION_TIMEOUT_MS };
349+
refreshOptions.headers = { ...options.headers, Authorization: `Bearer ${tokens.refresh_token}` };
350+
const refreshResponse = await httpRequest(refreshOptions, {
351+
operation: 'refresh_operation_token',
352+
});
353+
if (refreshResponse.statusCode === 200) {
354+
const refreshData = JSON.parse(refreshResponse.body);
355+
if (refreshData.operation_token) {
356+
tokens.operation_token = refreshData.operation_token;
357+
saveCredentials(lookupKey, {
358+
operation_token: tokens.operation_token,
359+
refresh_token: tokens.refresh_token,
360+
});
361+
console.error('Operation token refreshed successfully.');
362+
}
363+
} else if (refreshResponse.statusCode === 401) {
364+
console.error('Refresh token expired or invalid. Please run harper login again.');
365+
process.exit(1);
366+
} else {
367+
console.error(`Failed to refresh operation token: ${refreshResponse.statusCode}`);
368+
}
369+
} catch (refreshErr) {
370+
console.error(`Error refreshing operation token: ${refreshErr.message}`);
371+
}
372+
}
373+
330374
/**
331375
* Using a unix domain socket will send a request to hdb operations API server
332376
* @param req
@@ -403,36 +447,7 @@ async function cliOperations(req: any, skipResponseLog = false) {
403447
}
404448

405449
if (tokens?.operation_token) {
406-
if (tokens.refresh_token && isJWTExpired(tokens.operation_token)) {
407-
console.error('Operation token expired, attempting to refresh...');
408-
try {
409-
const refreshOptions = { ...options, timeout: CLI_OPERATION_TIMEOUT_MS };
410-
refreshOptions.headers = { ...options.headers, Authorization: `Bearer ${tokens.refresh_token}` };
411-
const refreshResponse = await httpRequest(refreshOptions, {
412-
operation: 'refresh_operation_token',
413-
});
414-
if (refreshResponse.statusCode === 200) {
415-
const refreshData = JSON.parse(refreshResponse.body);
416-
if (refreshData.operation_token) {
417-
tokens.operation_token = refreshData.operation_token;
418-
saveCredentials(lookupKey || target?.resolvedTarget, {
419-
operation_token: tokens.operation_token,
420-
refresh_token: tokens.refresh_token,
421-
});
422-
console.error('Operation token refreshed successfully.');
423-
// Update the original request's authorization header with the new token
424-
options.headers.Authorization = `Bearer ${tokens.operation_token}`;
425-
}
426-
} else if (refreshResponse.statusCode === 401) {
427-
console.error('Refresh token expired or invalid. Please run harper login again.');
428-
process.exit(1);
429-
} else {
430-
console.error(`Failed to refresh operation token: ${refreshResponse.statusCode}`);
431-
}
432-
} catch (refreshErr) {
433-
console.error(`Error refreshing operation token: ${refreshErr.message}`);
434-
}
435-
}
450+
await refreshExpiredOperationToken(options, tokens, lookupKey || target?.resolvedTarget);
436451
options.headers.Authorization = `Bearer ${tokens.operation_token}`;
437452
}
438453
}

0 commit comments

Comments
 (0)