Skip to content

fix(oauth): normalize imported token expiry - #335

Open
feniix wants to merge 4 commits into
openclaw:mainfrom
feniix:fix/vault-relative-expiry
Open

fix(oauth): normalize imported token expiry#335
feniix wants to merge 4 commits into
openclaw:mainfrom
feniix:fix/vault-relative-expiry

Conversation

@feniix

@feniix feniix commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • normalize tokens imported by vault set through the canonical persistence preparation path
  • derive expires_at from expires_in without changing explicit expiry aliases
  • preserve the existing atomic token and client-info vault write
  • document the expiry import contract and cover both fresh and delayed imports

Closes #334

Real behavior proof

One harness, three runs, all driving the real built CLI (node dist/cli.js, not the test runner) against a loopback fake OAuth provider + MCP server, with isolated HOME and XDG roots so the developer vault is never touched. The provider counts every POST /token and records every Authorization header presented at /mcp; it honors both the imported and the rotated access token, so nothing below is an artifact of the fixture rejecting a valid credential. All credentials are non-secret fixtures and are redacted anyway.

Run 1 and 2 differ only by the patch. Run 3 covers the delayed-import case raised in review.

1. Before — main ae3d900: the fresh token is redeemed on first use


$ mcporter vault set demo --stdin   # payload: expires_in 3600, refresh_token present, no expires_at
Saved OAuth credentials for 'demo' to $TMP/data/mcporter/credentials.json
(exit 0)

$ cat $XDG_DATA_HOME/mcporter/credentials.json   # persisted tokens (secrets redacted)
{
  "access_token": "<redacted>",
  "token_type": "Bearer",
  "refresh_token": "<redacted>",
  "expires_in": 3600,
  "expires_at": "(absent)"
}
-> NO absolute expiry persisted (expires_at absent)

$ mcporter list demo   # first use of the freshly imported token
demo

  /**
   * Return pong.
   */
  function ping();

  Examples:
    mcporter call demo.ping()

  1 tool · 51ms · HTTP http://127.0.0.1:58356/mcp
(exit 0)

provider-side observation:
  POST /token requests during first use : 1
    - grant_type=refresh_token refresh_token=<the freshly imported one>
  Authorization headers seen at /mcp    : Bearer <rotated access token>

RESULT: the fresh token was redeemed immediately (1 refresh request(s) to the provider).

2. After — this branch c5f11eb: the imported token is used as-is


$ mcporter vault set demo --stdin   # payload: expires_in 3600, refresh_token present, no expires_at
Saved OAuth credentials for 'demo' to $TMP/data/mcporter/credentials.json
(exit 0)

$ cat $XDG_DATA_HOME/mcporter/credentials.json   # persisted tokens (secrets redacted)
{
  "access_token": "<redacted>",
  "token_type": "Bearer",
  "refresh_token": "<redacted>",
  "expires_in": 3600,
  "expires_at": 1787629745
}
-> expires_at persisted: 3600s in the future

$ mcporter list demo   # first use of the freshly imported token
demo

  /**
   * Return pong.
   */
  function ping();

  Examples:
    mcporter call demo.ping()

  1 tool · 35ms · HTTP http://127.0.0.1:58339/mcp
(exit 0)

provider-side observation:
  POST /token requests during first use : 0
  Authorization headers seen at /mcp    : Bearer <imported access token>

RESULT: the imported token was used as-is; the provider was never asked to redeem the refresh token.

3. After — delayed import declaring its real expiry: still refreshes first


$ mcporter vault set demo --stdin   # payload: expires_in 3600 AND expires_at = now + 30s (issued 59.5 min ago)
Saved OAuth credentials for 'demo' to $TMP/data/mcporter/credentials.json
(exit 0)

$ cat $XDG_DATA_HOME/mcporter/credentials.json   # persisted tokens (secrets redacted)
{
  "access_token": "<redacted>",
  "token_type": "Bearer",
  "refresh_token": "<redacted>",
  "expires_in": 3600,
  "expires_at": 1787626175
}
-> expires_at persisted: 30s in the future (the supplied value, kept verbatim; the relative reading would have said ~3600s)

$ mcporter list demo   # first use of the freshly imported token
demo

  /**
   * Return pong.
   */
  function ping();

  Examples:
    mcporter call demo.ping()

  1 tool · 52ms · HTTP http://127.0.0.1:58342/mcp
(exit 0)

provider-side observation:
  POST /token requests during first use : 1
    - grant_type=refresh_token refresh_token=<the freshly imported one>
  Authorization headers seen at /mcp    : Bearer <rotated access token>

RESULT: the near-expiry token was refreshed before use (1 refresh request(s)) — the conservative path still runs when the payload declares a real expiry.

What the transcripts show

1. main ae3d900, fresh import 2. branch c5f11eb, fresh import 3. branch c5f11eb, delayed import
payload expiry expires_in: 3600 expires_in: 3600 expires_in: 3600 + expires_at: now+30
expires_at persisted absent now + 3600s now + 30s (verbatim)
POST /token on first use 1 0 1
bearer presented at /mcp rotated the imported token rotated
mcporter list demo ok ok ok

Run 1 is the P1 defect observed end to end: a one-hour-valid grant is spent on its very first use, because shouldRefreshCachedToken falls back to "expires_in + refresh_token ⇒ due" when no absolute expiry was persisted (src/oauth-token-refresh.ts:188). Note that the command still succeeds — that is what makes it worth fixing rather than merely noisy. Nothing surfaces to the user, but with rotating refresh tokens (RFC 9700 §4.14.2) the grant the operator just imported has already been consumed.

Run 2 is the fix: the absolute expiry is persisted, the timestamp comparison wins, and the provider is never contacted.

Run 3 answers the review finding directly. A payload that declares its real remaining lifetime keeps that value verbatim and still takes the conservative refresh path before the token is used — the patch does not blanket-suppress refresh for imported credentials, it suppresses it only where the payload says the token is genuinely live.

Expiry import contract (addresses the delayed-import finding)

expires_in is relative to token issuance, but vault set imports credentials that may already be held, so the payload carries no issuance timestamp. Normalization can only read a relative expiry as lifetime remaining at import — which over-extends a stale response, and readExplicitRefreshableBearerToken (src/oauth-token-refresh.ts:456) then returns that token directly instead of refreshing.

The mechanism to express a real expiry already exists and is untouched by this patch: withStoredExpiry returns tokens verbatim when expires_at or expiresAt is present, and validateOAuthTokens accepts both aliases. What was missing is that this was neither documented nor guarded, so this branch adds both:

  • docs/config.md now states the contract — an explicit expires_at / expiresAt (Unix seconds) is stored verbatim and is how delayed credentials should be imported; expires_in alone means lifetime remaining at import. Scripts that replay a stored token response should convert once at capture time.
  • tests/vault-command.test.ts covers a delayed import through both aliases: a token issued 55 minutes before import keeps its real 5-minutes-remaining expiry and is not rewritten to now + 3600. Run 3 above is the same case through the real CLI.

Both unit guards were mutation-checked — deleting the alias short-circuit in withStoredExpiry fails them, so they are not vacuous.

This keeps the #334 fix intact for the fresh-import case rather than reverting to the unconditional conservative refresh, which would reintroduce the reported bug.

Reproduction harness (drop in tmp/vault-expiry-proof.mjs, then pnpm build && node tmp/vault-expiry-proof.mjs . after-fix fresh / ... delayed)
/**
 * Real-behavior proof for mcporter#335.
 *
 * Runs the actual built CLI (dist/cli.js) against a local fake OAuth provider +
 * MCP server, and reports whether the provider's /token endpoint is hit on the
 * FIRST use of a freshly imported, still-valid (expires_in: 3600) token.
 *
 * Usage: node vault-expiry-proof.mjs <path-to-repo-checkout> <label>
 */
import { execFile } from 'node:child_process';
import fs from 'node:fs/promises';
import { createServer } from 'node:http';
import os from 'node:os';
import path from 'node:path';
import { Readable } from 'node:stream';
import { promisify } from 'node:util';
import { createMcpHandler, McpServer } from '@modelcontextprotocol/server';
import { z } from 'zod';

const execFileAsync = promisify(execFile);
const repo = path.resolve(process.argv[2] ?? process.cwd());
const label = process.argv[3] ?? 'run';
const scenario = process.argv[4] ?? 'fresh';
const CLI = path.join(repo, 'dist', 'cli.js');

const SEEDED_ACCESS = 'seeded-access-token';
const SEEDED_REFRESH = 'seeded-refresh-token';

const tokenEndpointHits = [];
const mcpAuthorizations = [];

function sendJson(response, body, status = 200) {
  const payload = JSON.stringify(body);
  response.writeHead(status, { 'content-type': 'application/json' });
  response.end(payload);
}

async function readBody(request) {
  const chunks = [];
  for await (const chunk of request) chunks.push(chunk);
  return Buffer.concat(chunks).toString('utf8');
}

function toWebRequest(request, origin) {
  const url = new URL(request.url ?? '/', origin);
  const headers = new Headers();
  for (const [key, value] of Object.entries(request.headers)) {
    if (Array.isArray(value)) for (const item of value) headers.append(key, item);
    else if (value !== undefined) headers.set(key, value);
  }
  const hasBody = request.method !== 'GET' && request.method !== 'HEAD';
  return new Request(url, {
    method: request.method,
    headers,
    ...(hasBody ? { body: Readable.toWeb(request), duplex: 'half' } : {}),
  });
}

async function writeWebResponse(response, webResponse) {
  const headers = {};
  webResponse.headers.forEach((value, key) => {
    headers[key] = value;
  });
  response.writeHead(webResponse.status, headers);
  if (!webResponse.body) return response.end();
  for await (const chunk of Readable.fromWeb(webResponse.body)) response.write(chunk);
  response.end();
}

const mcpHandler = createMcpHandler(() => {
  const server = new McpServer({ name: 'proof-fixture', version: '1.0.0' }, { capabilities: { tools: {} } });
  server.registerTool(
    'ping',
    { description: 'Return pong.', inputSchema: z.object({}) },
    async () => ({ content: [{ type: 'text', text: 'pong' }] })
  );
  return server;
});

let origin = '';
const server = createServer((request, response) => {
  const url = new URL(request.url ?? '/', origin || 'http://127.0.0.1');

  if (url.pathname.includes('.well-known/oauth-protected-resource')) {
    return sendJson(response, { resource: `${origin}/mcp`, authorization_servers: [origin] });
  }
  if (url.pathname.includes('.well-known/oauth-authorization-server') || url.pathname.includes('openid-configuration')) {
    return sendJson(response, {
      issuer: origin,
      authorization_endpoint: `${origin}/authorize`,
      token_endpoint: `${origin}/token`,
      registration_endpoint: `${origin}/register`,
      response_types_supported: ['code'],
      grant_types_supported: ['authorization_code', 'refresh_token'],
      token_endpoint_auth_methods_supported: ['none'],
      code_challenge_methods_supported: ['S256'],
    });
  }
  if (url.pathname === '/token' && request.method === 'POST') {
    return void readBody(request).then((raw) => {
      const params = new URLSearchParams(raw);
      tokenEndpointHits.push({
        grant_type: params.get('grant_type'),
        presented_refresh_token: params.get('refresh_token'),
      });
      sendJson(response, {
        access_token: 'rotated-access-token',
        token_type: 'Bearer',
        refresh_token: 'rotated-refresh-token',
        expires_in: 3600,
      });
    });
  }
  if (url.pathname === '/mcp') {
    const authorization = request.headers.authorization ?? '(none)';
    mcpAuthorizations.push(authorization);
    if (authorization !== `Bearer ${SEEDED_ACCESS}` && authorization !== 'Bearer rotated-access-token') {
      response.writeHead(401, {
        'www-authenticate': `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource"`,
      });
      return response.end('unauthorized');
    }
    return void mcpHandler
      .fetch(toWebRequest(request, origin))
      .then((webResponse) => writeWebResponse(response, webResponse))
      .catch(() => {
        if (!response.headersSent) response.writeHead(500);
        response.end();
      });
  }
  response.writeHead(404).end('not found');
});

await new Promise((resolve, reject) => {
  server.once('error', reject);
  server.listen(0, '127.0.0.1', resolve);
});
origin = `http://127.0.0.1:${server.address().port}`;

const root = await fs.mkdtemp(path.join(os.tmpdir(), `mcporter-vault-proof-${label}-`));
const env = {
  ...process.env,
  HOME: path.join(root, 'home'),
  XDG_CONFIG_HOME: path.join(root, 'config'),
  XDG_DATA_HOME: path.join(root, 'data'),
  XDG_STATE_HOME: path.join(root, 'state'),
  XDG_CACHE_HOME: path.join(root, 'cache'),
  NO_COLOR: '1',
};
await fs.mkdir(path.join(root, 'config', 'mcporter'), { recursive: true });
await fs.writeFile(
  path.join(root, 'config', 'mcporter', 'mcporter.json'),
  `${JSON.stringify({ mcpServers: { demo: { baseUrl: `${origin}/mcp`, auth: 'oauth' } } }, null, 2)}\n`
);

// 'fresh'   : a token response imported right after issuance (relative expiry only).
// 'delayed' : the same one-hour response, imported 59.5 minutes after it was
//             issued, declaring its true remaining lifetime via expires_at.
const trueExpiry = Math.floor(Date.now() / 1000) + 30;
const payload = {
  tokens: {
    access_token: SEEDED_ACCESS,
    token_type: 'Bearer',
    refresh_token: SEEDED_REFRESH,
    expires_in: 3600,
    ...(scenario === 'delayed' ? { expires_at: trueExpiry } : {}),
  },
  clientInfo: { client_id: 'proof-client', redirect_uris: [`${origin}/callback`] },
};

async function runCli(args, input) {
  try {
    const child = execFileAsync(process.execPath, [CLI, ...args], { env, timeout: 60_000 });
    if (input !== undefined) {
      child.child.stdin.end(input);
    }
    const { stdout, stderr } = await child;
    return { code: 0, stdout, stderr };
  } catch (error) {
    return { code: error.code ?? 1, stdout: error.stdout ?? '', stderr: error.stderr ?? String(error) };
  }
}

const out = [];
const say = (line = '') => out.push(line);

say(`### ${label} (scenario: ${scenario})`);
say(`repo: ${repo}`);
say(`git HEAD: ${(await execFileAsync('git', ['rev-parse', '--short', 'HEAD'], { cwd: repo })).stdout.trim()}`);
say(`fake provider + MCP server: ${origin}`);
say();

say(
  scenario === 'delayed'
    ? `$ mcporter vault set demo --stdin   # payload: expires_in 3600 AND expires_at = now + 30s (issued 59.5 min ago)`
    : `$ mcporter vault set demo --stdin   # payload: expires_in 3600, refresh_token present, no expires_at`
);
const setResult = await runCli(['vault', 'set', 'demo', '--stdin'], JSON.stringify(payload));
say(setResult.stdout.trim().replace(root, '$TMP'));
if (setResult.stderr.trim()) say(setResult.stderr.trim().replace(root, '$TMP'));
say(`(exit ${setResult.code})`);
say();

const vaultPath = path.join(root, 'data', 'mcporter', 'credentials.json');
const vault = JSON.parse(await fs.readFile(vaultPath, 'utf8'));
const entry = Object.values(vault.entries)[0];
say(`$ cat $XDG_DATA_HOME/mcporter/credentials.json   # persisted tokens (secrets redacted)`);
say(
  JSON.stringify(
    {
      access_token: '<redacted>',
      token_type: entry.tokens.token_type,
      refresh_token: '<redacted>',
      expires_in: entry.tokens.expires_in,
      expires_at: entry.tokens.expires_at ?? '(absent)',
    },
    null,
    2
  )
);
const nowSeconds = Math.floor(Date.now() / 1000);
say(
  entry.tokens.expires_at
    ? `-> expires_at persisted: ${entry.tokens.expires_at - nowSeconds}s in the future` +
      (scenario === 'delayed'
        ? ` (the supplied value, kept verbatim; the relative reading would have said ~3600s)`
        : ``)
    : `-> NO absolute expiry persisted (expires_at absent)`
);
say();

const hitsBefore = tokenEndpointHits.length;
say(`$ mcporter list demo   # first use of the freshly imported token`);
const listResult = await runCli(['list', 'demo']);
say(listResult.stdout.trim().replace(root, '$TMP') || '(no stdout)');
if (listResult.stderr.trim()) say(listResult.stderr.trim().replace(root, '$TMP'));
say(`(exit ${listResult.code})`);
say();

const refreshHits = tokenEndpointHits.slice(hitsBefore);
say(`provider-side observation:`);
say(`  POST /token requests during first use : ${refreshHits.length}`);
for (const hit of refreshHits) {
  say(`    - grant_type=${hit.grant_type} refresh_token=${hit.presented_refresh_token === SEEDED_REFRESH ? '<the freshly imported one>' : '<other>'}`);
}
say(`  Authorization headers seen at /mcp    : ${
  mcpAuthorizations.length === 0
    ? '(none)'
    : [...new Set(mcpAuthorizations)]
        .map((value) =>
          value === `Bearer ${SEEDED_ACCESS}`
            ? 'Bearer <imported access token>'
            : value === 'Bearer rotated-access-token'
              ? 'Bearer <rotated access token>'
              : value
        )
        .join(', ')
}`);
say();
say(
  refreshHits.length === 0
    ? 'RESULT: the imported token was used as-is; the provider was never asked to redeem the refresh token.'
    : scenario === 'delayed'
      ? `RESULT: the near-expiry token was refreshed before use (${refreshHits.length} refresh request(s)) — the conservative path still runs when the payload declares a real expiry.`
      : `RESULT: the fresh token was redeemed immediately (${refreshHits.length} refresh request(s) to the provider).`
);

console.log(out.join('\n'));

await new Promise((resolve) => server.close(resolve));
await mcpHandler.close?.();
await fs.rm(root, { recursive: true, force: true });
process.exit(0);

Tests

  • pnpm exec vitest run tests/vault-command.test.ts (8 passed)
  • pnpm exec vitest run tests/vault-command.test.ts tests/vault-cli.integration.test.ts tests/oauth-persistence-stores.test.ts (22 passed: 8 + 9 + 5)
  • pnpm lint:oxlint (passed)
  • pnpm exec oxfmt --check docs/config.md tests/vault-command.test.ts src/cli/vault-command.ts src/oauth-persistence-stores.ts (passed)
  • pnpm test (1,637 passed, 26 skipped; 1 pre-existing failure in oauth-refresh-process.integration.test.ts, untouched by this branch)
  • pnpm check (fails at format:check on tests/cli-list-stdio-logs.test.ts and tests/list-inline-stdio.test.ts — both pre-existing on main and not among the four files this branch changes)

@clawsweeper

clawsweeper Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 25, 2026
@clawsweeper

clawsweeper Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 25, 2026, 5:31 AM ET / 09:31 UTC.

ClawSweeper review

What this changes

The branch normalizes OAuth tokens imported through vault set, documents relative versus absolute expiry semantics, and adds regression coverage for fresh and delayed imports.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

This PR remains necessary: current main still bypasses the canonical expiry normalizer for vault set, while the linked OAuth bug remains open. The patch is focused and correct; its documented delayed-import compatibility boundary needs ordinary maintainer merge review.

Priority: P1
Reviewed head: 272f53af8a1a66fa6760cbe4eaeb06fd75c00b2e
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch is focused, tested, and backed by real CLI evidence; the remaining consideration is its explicit replay-compatibility contract.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The PR body provides redacted after-fix built-CLI output through a loopback OAuth provider and MCP server, including fresh and near-expiry cases.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body provides redacted after-fix built-CLI output through a loopback OAuth provider and MCP server, including fresh and near-expiry cases.
Evidence reviewed 5 items Main still lacks the fix: The branch adds the missing normalization call at the vault-write boundary; current main writes the payload tokens directly.
Canonical normalizer behavior: The reused helper preserves explicit expires_at/expiresAt values and otherwise derives expires_at from finite expires_in values.
Refresh-path compatibility: Current main already reads both absolute-expiry aliases before applying the relative-expiry fallback, so the new stored values are consumed by the established refresh decision.
Findings None None.
Security None None.

Live Verification

Command: pnpm exec vitest run tests/vault-command.test.ts

Result: FAIL (partial) — step 2 expect_output vault command: expected terminal output was not visible within 30 seconds: "vault command"

pnpm exec vitest run tests/vault-command.test.ts
runner@runnervm76f27:/tmp/clawsweeper-live-proof-335-IxL56U/target$ pnpm exec vitest run tests/vault-command.test.ts

 RUN  v4.1.10 /tmp/clawsweeper-live-proof-335-IxL56U/target

 ✓ tests/vault-command.test.ts (8 tests) 22ms

 Test Files  1 passed (1)
      Tests  8 passed (8)
   Start at  09:31:38
   Duration  244ms (transform 83ms, setup 24ms, import 88ms, tests 22ms, environment 0ms)

runner@runnervm76f27:/tmp/clawsweeper-live-proof-335-IxL56U/target$ pnpm exec vitest run tests/vault-command.test.ts

 RUN  v4.1.10 /tmp/clawsweeper-live-proof-335-IxL56U/target

 ✓ tests/vault-command.test.ts (8 tests) 22ms

 Test Files  1 passed (1)
      Tests  8 passed (8)
   Start at  09:31:39
   Duration  236ms (transform 88ms, setup 24ms, import 89ms, tests 22ms, environment 0ms)

runner@runnervm76f27:/tmp/clawsweeper-live-proof-335-IxL56U/target$



























Assertions:

  • FAIL expect_output: vault command

How this fits together

vault set writes imported OAuth credentials to MCPorter's shared vault. Later OAuth and refreshable-bearer connections read those credentials to decide whether to send the access token or refresh it first.

flowchart LR
A[Token JSON input] --> B[Vault set command]
B --> C[Expiry normalization]
C --> D[Shared credential vault]
D --> E[Token freshness check]
E --> F[Use bearer token]
E --> G[Refresh token request]
Loading

Decision needed

Question Recommendation
Is the documented expires_in-only replay compatibility boundary acceptable for existing vault-import automation? Accept the documented import contract: Treat expires_in as remaining lifetime at import and require an explicit absolute expiry for delayed credentials.

Why: A token payload has no issuance timestamp, so the implementation cannot distinguish a fresh response from a replayed one without choosing between immediate refresh and treating expires_in as remaining lifetime.

Before merge

  • Resolve merge risk (P1) - Existing automation that replays an old token response containing only expires_in will now receive a derived future expiry; such automation must provide expires_at or expiresAt as the new documentation explains.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 2 source files +3/-2, tests +39/-1, docs +6 The implementation reuses existing persistence behavior and defines the input contract with focused regression coverage.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #334
Summary: This PR is the focused candidate fix for the open vault-import expiry defect.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Accept the documented expiry contract (recommended)
    Merge with the explicit absolute-expiry path for replayed credentials, accepting that old expires_in-only replay scripts must be updated.
  2. Preserve the former conservative behavior
    Pause this change if maintaining automatic refresh for every expires_in-only import is the required compatibility policy.

Technical review

Best possible solution:

Merge the shared-normalizer reuse while retaining the documented absolute-expiry escape hatch for delayed or replayed token responses.

Do we have a high-confidence way to reproduce the issue?

Yes. The PR body supplies a real built-CLI loopback reproduction on main and after-fix runs that observe both token-endpoint redemption and the bearer sent to the MCP server.

Is this the best way to solve the issue?

Yes. Reusing the existing persistence normalizer fixes the divergent write path without introducing a parallel expiry implementation, while the docs state the necessary delayed-import constraint.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ae3d9000c320.

Labels

Label justifications:

  • P1: The defect can prematurely redeem a fresh OAuth refresh token in a normal credential-import workflow.
  • merge-risk: 🚨 compatibility: The PR intentionally changes how existing expires_in-only vault payloads are interpreted after delayed replay.
  • merge-risk: 🚨 auth-provider: The changed expiry decision controls whether MCPorter sends an imported bearer or redeems its refresh token.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides redacted after-fix built-CLI output through a loopback OAuth provider and MCP server, including fresh and near-expiry cases.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides redacted after-fix built-CLI output through a loopback OAuth provider and MCP server, including fresh and near-expiry cases.

Evidence

What I checked:

  • Main still lacks the fix: The branch adds the missing normalization call at the vault-write boundary; current main writes the payload tokens directly. (src/cli/vault-command.ts:52, 272f53af8a1a)
  • Canonical normalizer behavior: The reused helper preserves explicit expires_at/expiresAt values and otherwise derives expires_at from finite expires_in values. (src/oauth-persistence-stores.ts:35, e53ef107e4c9)
  • Refresh-path compatibility: Current main already reads both absolute-expiry aliases before applying the relative-expiry fallback, so the new stored values are consumed by the established refresh decision. (src/oauth-token-refresh.ts:160, ae3d9000c320)
  • Focused regression coverage: Tests freeze time, assert derived expiry for fresh imports, and retain each explicit absolute-expiry alias for delayed imports. (tests/vault-command.test.ts:116, 272f53af8a1a)
  • After-fix behavior proof: The PR body records redacted built-CLI loopback runs showing a fresh import performs zero token redemptions and an explicitly near-expiry import still refreshes before use. (272f53af8a1a)

Likely related people:

  • Peter Steinberger: Current-main blame attributes the expiry-normalization and refresh-decision source to this release commit. (role: current-main OAuth persistence introducer; confidence: high; commits: e53ef107e4c9; files: src/oauth-persistence-stores.ts, src/oauth-token-refresh.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Confirm the documented expires_in-only replay contract during maintainer merge review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (5 earlier review cycles)
  • reviewed 2026-08-25T00:43:31.077Z sha e54d64a :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-25T01:03:24.779Z sha a8c8772 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-25T01:27:25.182Z sha a8c8772 :: found issues before merge. :: [P1] Preserve a safe expiry for delayed vault imports
  • reviewed 2026-08-25T02:56:10.945Z sha 272f53a :: needs maintainer review before merge. :: none
  • reviewed 2026-08-25T03:38:37.612Z sha 272f53a :: needs maintainer review before merge. :: none

@feniix

feniix commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Added the requested after-fix real CLI behavior proof to the PR body at head a8c8772.

The isolated loopback run used the built CLI for vault set followed by the first list. It confirmed:

  • expires_in: 3600 was stored with expires_at derived at now + 3600s
  • the first cached read completed successfully
  • the MCP endpoint observed the cached bearer
  • the counting token endpoint observed 0 redemptions

All credentials were non-secret fixtures and their values were omitted from the transcript. This directly covers the P1 auth-provider merge risk.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 25, 2026
feniix and others added 2 commits August 24, 2026 23:46
`expires_in` is relative to token issuance, but `vault set` imports
credentials that may already be held, so the payload carries no issuance
timestamp. Normalization can only read a relative expiry as lifetime
remaining at import, which over-extends a stale response and lets
refreshable_bearer send a dead token instead of refreshing.

State that contract in the config guide: an explicit `expires_at` /
`expiresAt` is stored verbatim and is the way to import delayed
credentials, while `expires_in` alone means remaining lifetime. Cover
both aliases with a delayed-import regression so the passthrough cannot
regress into the relative reading.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`expires_at` and `expiresAt` are persistence-only fields, so reading them
straight off `OAuthTokens` failed `tsc` on CI. Narrow the loaded entry to
a local shape the way tests/oauth-persistence.test.ts already does, and
assert the effective stored expiry equals the supplied value rather than
merely differing from the relative one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@feniix

feniix commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vault set treats relative expires_in as immediately due and can invalidate fresh grants

1 participant