fix: address PR review blockers (plan-usage units, legacy auth backup, Windows dsh preset)

- claude: parse rate_limit_event resetsAt as epoch seconds (the CLI emits
  seconds, not millis) and prefer rateLimitType over the reset-delta
  heuristic for 5h/7d window classification
- claude: scale over-capacity utilization (>1.0) to a real percent instead
  of clamping it to ~1%
- codex: back up an unmanaged auth.json before the first post-upgrade
  provider transition overwrites it, and preserve it when leaving a legacy
  managed provider without a backup
- dsh: spawn --dump-config through resolveCliSpawn so Windows .cmd shims
  launch via cmd.exe instead of silently failing with EINVAL
This commit is contained in:
zhukunpenglinyutong
2026-08-26 15:01:22 +08:00
parent 664f2148a3
commit 3b5feff1da
5 changed files with 224 additions and 18 deletions
+12 -6
View File
@@ -29,7 +29,7 @@ import { homedir } from 'os';
import { existsSync, readFileSync, readdirSync } from 'fs';
import { join, dirname } from 'path';
import { pathToFileURL } from 'url';
import { enrichPathWithBinDirs, commonCliBinDirs } from '../../utils/cli-path.js';
import { enrichPathWithBinDirs, commonCliBinDirs, resolveCliSpawn } from '../../utils/cli-path.js';
/**
* The shipped DSH agent presets plus user-installed ones curated by the CC
@@ -380,8 +380,8 @@ export function buildPresetOverlay({ presetId, presetText, baseIds, presetDir =
* `<dshHome>/.agent-presets/<id>/` where dshHome is `${DSH_HOME:-$HOME/.dsh}`
* — the same root the `dsh-agent-presets` plugin scans, so anything the DSH
* web app can select is found here too. From the resolved spawn command:
* - Windows `.cmd` shim: `resolveDshSpawnCommand` yields `node <pkgRoot>/lib/bin.js`
* → pkgRoot = dirname(dirname(script)).
* - Windows `.cmd` shim: `resolveCliSpawn` launches it via `cmd.exe`, so the
* preset directory is located from the bin path candidates below instead.
* - POSIX: try `dirname(dirname(bin))` (bin under `<pkgRoot>/bin/`) and the
* npm prefix layout `<prefix>/lib/node_modules/@deepseek-ai/dsh`.
*
@@ -434,12 +434,18 @@ export function getHeadlessBaseIds(spawnCmd, options = {}) {
const home = process.env.HOME || process.env.USERPROFILE || homedir();
enrichPathWithBinDirs(env, commonCliBinDirs(home));
try {
const result = spawnSync(bin, [...(spawnCmd.args || []), '--profile', 'headless', '--dump-config'], {
encoding: 'utf8',
// Route through resolveCliSpawn so Windows `.cmd`/`.bat` shims launch via
// cmd.exe (CVE-2024-27980) — a raw spawn of a shim fails with EINVAL and
// would silently drop the preset overlay.
const invocation = resolveCliSpawn(bin, [...(spawnCmd.args || []), '--profile', 'headless', '--dump-config'], {
env,
windowsHide: true,
});
const result = spawnSync(invocation.file, invocation.args, {
...invocation.options,
encoding: 'utf8',
timeout: 60_000,
maxBuffer: 32 * 1024 * 1024,
shell: spawnCmd.shell === true,
});
if (result.error || result.status !== 0) {
logDebug('dump-config failed:', result.error?.message || `status ${result.status}`);
@@ -363,11 +363,18 @@ public final class ClaudePlanUsageService {
if (utilization == null || !Double.isFinite(utilization)) {
return null;
}
double pct = clampPct(utilization <= 1.0 ? utilization * 100.0 : utilization);
// The CLI documents utilization as a fraction of the window (0-1, and
// exceeding 1 when over capacity), so scale it to a percent. The <= 10
// guard only protects against a hypothetical already-percent payload
// (0-100) from being scaled twice.
double pct = clampPct(utilization <= 10.0 ? utilization * 100.0 : utilization);
Long resetsAtMs = asLong(rateLimitInfo, "resetsAt", "resets_at", "resetAt");
// resetsAt is unix epoch SECONDS in the CLI schema (the CLI computes
// `resetsAt - Date.now()/1000`), not millis — convert before use.
Long resetsAtSec = asLong(rateLimitInfo, "resetsAt", "resets_at", "resetAt");
Long resetsAtMs = resetsAtSec != null ? resetsAtSec * 1000L : null;
String resetAt = resetsAtMs != null ? Instant.ofEpochMilli(resetsAtMs).toString() : null;
String periodType = resetsAtMs != null ? periodTypeFromResetMs(resetsAtMs) : "5h";
String periodType = periodTypeFromRateLimit(rateLimitInfo, resetsAtMs);
JsonObject window = new JsonObject();
window.addProperty("id", periodType);
@@ -397,6 +404,27 @@ public final class ClaudePlanUsageService {
return out;
}
/**
* Window classification prefers the CLI-provided {@code rateLimitType}
* ({@code five_hour} / {@code seven_day} / {@code seven_day_sonnet} / …) over
* the reset-delta heuristic, which only survives as a fallback.
*/
static String periodTypeFromRateLimit(JsonObject rateLimitInfo, Long resetsAtMs) {
String type = asString(rateLimitInfo, "rateLimitType");
if (type == null) {
type = asString(rateLimitInfo, "rate_limit_type");
}
if (type != null) {
if (type.startsWith("five_hour")) {
return "5h";
}
if (type.startsWith("seven_day")) {
return "7d";
}
}
return resetsAtMs != null ? periodTypeFromResetMs(resetsAtMs) : "5h";
}
static String periodTypeFromResetMs(long resetsAtMs) {
long deltaMs = resetsAtMs - System.currentTimeMillis();
if (deltaMs <= 6L * 60 * 60 * 1000) {
@@ -227,6 +227,7 @@ public class CodexSettingsManager {
IoAction commitProviderState) throws IOException {
Map<String, Object> previousFragment = parseProviderFragment(previousProvider, false);
Map<String, Object> nextFragment = parseProviderFragment(nextProvider, true);
JsonObject previousAuth = parseProviderAuth(previousProvider);
JsonObject nextAuth = parseProviderAuth(nextProvider);
synchronized (CONFIG_FILE_LOCK) {
@@ -263,7 +264,7 @@ public class CodexSettingsManager {
}
writeConfigTomlUnlocked(nextConfig);
if (nextProvider != null) {
if (previousProvider == null) {
if (shouldBackupLocalAuthUnlocked(cliAuthBackupPath, previousAuth)) {
backupLocalAuthUnlocked(cliAuthBackupPath);
}
if (nextAuth != null) {
@@ -272,7 +273,7 @@ public class CodexSettingsManager {
Files.deleteIfExists(authPath);
}
} else if (previousProvider != null) {
restoreLocalAuthUnlocked(cliAuthBackupPath);
restoreLocalAuthUnlocked(cliAuthBackupPath, previousAuth);
}
if (nextProvider == null) {
Files.deleteIfExists(configBaselinePath);
@@ -570,6 +571,27 @@ public class CodexSettingsManager {
return value;
}
/**
* Decides whether the current auth.json must be preserved as the user's local
* credential before a provider overwrites it. A backup is taken only when no
* backup exists yet and the current auth.json is not the outgoing provider's
* own managed credential — the latter covers legacy installs whose active
* provider was applied by older code that never took a backup.
*/
private boolean shouldBackupLocalAuthUnlocked(Path backupPath, JsonObject previousAuth) throws IOException {
if (Files.exists(backupPath)) {
return false;
}
if (previousAuth == null) {
// Either no previous provider (entering managed mode from a purely
// local state) or the outgoing provider owns no credential — in both
// cases the current auth.json is unmanaged and worth preserving.
return true;
}
JsonObject currentAuth = readAuthJsonUnlocked();
return currentAuth != null && !previousAuth.equals(currentAuth);
}
private void backupLocalAuthUnlocked(Path backupPath) throws IOException {
Path authPath = getAuthJsonPath();
if (Files.exists(authPath)) {
@@ -580,12 +602,20 @@ public class CodexSettingsManager {
}
}
private void restoreLocalAuthUnlocked(Path backupPath) throws IOException {
private void restoreLocalAuthUnlocked(Path backupPath, JsonObject previousAuth) throws IOException {
if (Files.exists(backupPath)) {
writeStringAtomically(getAuthJsonPath(), Files.readString(backupPath, StandardCharsets.UTF_8));
Files.deleteIfExists(backupPath);
return;
}
// No backup on file (legacy installs never took one): the current
// auth.json may still be the user's own `codex login` session — only
// delete it when it is the outgoing provider's managed credential.
JsonObject currentAuth = readAuthJsonUnlocked();
if (currentAuth != null && (previousAuth == null || !previousAuth.equals(currentAuth))) {
LOG.info("[CodexSettingsManager] No auth backup found; preserving unmanaged local credentials");
return;
}
Files.deleteIfExists(getAuthJsonPath());
}
@@ -11,19 +11,24 @@ import static org.junit.Assert.assertTrue;
public class ClaudePlanUsageServiceTest {
private static JsonObject info(double utilization, long resetsAtMs, String status) {
/** resetsAt is epoch SECONDS in the CLI rate_limit_info schema. */
private static JsonObject info(double utilization, long resetsAtSec, String status) {
JsonObject o = new JsonObject();
o.addProperty("utilization", utilization);
o.addProperty("resetsAt", resetsAtMs);
o.addProperty("resetsAt", resetsAtSec);
if (status != null) {
o.addProperty("status", status);
}
return o;
}
private static long nowSec() {
return System.currentTimeMillis() / 1000L;
}
@Test
public void buildCapacityPayload_fractionUtilization_mapsToPercentWith5hWindow() {
long resetsAt = System.currentTimeMillis() + 3L * 60 * 60 * 1000; // ~3h out → 5h bucket
long resetsAt = nowSec() + 3L * 60 * 60; // ~3h out → 5h bucket
JsonObject payload = ClaudePlanUsageService.buildCapacityPayload(info(0.42, resetsAt, "allowed_warning"));
assertEquals(42.0, payload.get("capacity_pct").getAsDouble(), 0.01);
@@ -41,8 +46,29 @@ public class ClaudePlanUsageServiceTest {
}
@Test
public void buildCapacityPayload_percentUtilizationAboveOne_treatedAsPercent() {
long resetsAt = System.currentTimeMillis() + 5L * 24 * 60 * 60 * 1000; // ~5d → 7d bucket
public void buildCapacityPayload_epochSecondsResetAt_convertsToMillis() {
long resetsAtSec = nowSec() + 2L * 60 * 60; // 2h out
JsonObject payload = ClaudePlanUsageService.buildCapacityPayload(info(0.1, resetsAtSec, null));
String resetAt = payload.get("reset_at").getAsString();
long parsedMs = java.time.Instant.parse(resetAt).toEpochMilli();
assertEquals(resetsAtSec * 1000L, parsedMs);
// 2h out must classify as the 5h window — with the old millis misread
// this landed in 1970 and misclassified everything.
assertEquals("5h", payload.get("period_type").getAsString());
}
@Test
public void buildCapacityPayload_overLimitFraction_clampsToHundred() {
// utilization 1.3 = 130% used (over capacity) — must surface as ~100%,
// not as a tiny "1.3%" reading.
JsonObject payload = ClaudePlanUsageService.buildCapacityPayload(info(1.3, nowSec() + 3600, null));
assertEquals(100.0, payload.get("capacity_pct").getAsDouble(), 0.01);
}
@Test
public void buildCapacityPayload_percentUtilizationAboveTen_treatedAsPercent() {
long resetsAt = nowSec() + 5L * 24 * 60 * 60; // ~5d → 7d bucket
JsonObject payload = ClaudePlanUsageService.buildCapacityPayload(info(87.0, resetsAt, "rejected"));
assertEquals(87.0, payload.get("capacity_pct").getAsDouble(), 0.01);
@@ -50,10 +76,26 @@ public class ClaudePlanUsageServiceTest {
assertEquals("rejected", payload.get("rate_limit_status").getAsString());
}
@Test
public void buildCapacityPayload_rateLimitTypeWinsOverDeltaHeuristic() {
// A seven_day window whose reset happens to be <6h out must still be 7d.
JsonObject o = info(0.5, nowSec() + 2L * 60 * 60, null);
o.addProperty("rateLimitType", "seven_day");
assertEquals("7d", ClaudePlanUsageService.buildCapacityPayload(o).get("period_type").getAsString());
JsonObject sonnet = info(0.5, nowSec() + 2L * 60 * 60, null);
sonnet.addProperty("rateLimitType", "seven_day_sonnet");
assertEquals("7d", ClaudePlanUsageService.buildCapacityPayload(sonnet).get("period_type").getAsString());
JsonObject fiveHour = info(0.5, nowSec() + 5L * 24 * 60 * 60, null);
fiveHour.addProperty("rateLimitType", "five_hour");
assertEquals("5h", ClaudePlanUsageService.buildCapacityPayload(fiveHour).get("period_type").getAsString());
}
@Test
public void buildCapacityPayload_missingUtilization_returnsNull() {
JsonObject noUtil = new JsonObject();
noUtil.addProperty("resetsAt", System.currentTimeMillis() + 1000L);
noUtil.addProperty("resetsAt", nowSec() + 1L);
assertNull(ClaudePlanUsageService.buildCapacityPayload(noUtil));
}
@@ -403,6 +403,106 @@ public class CodexSettingsManagerTomlRoundTripTest {
assertFalse(Files.exists(codexDir.resolve("auth.json.cli_backup")));
}
@Test
public void shouldBackupLegacyLocalAuthOnFirstPostUpgradeTransition() throws Exception {
// Legacy state: provider A (config-only, no authJson) was applied by old
// code which never took an auth backup, so auth.json still holds the
// user's genuine `codex login` session and no cli_backup exists.
Path tempHome = Files.createTempDirectory("codex-provider-legacy-backup-home");
useTemporaryHomeDirectory(tempHome);
Path codexDir = tempHome.resolve(".codex");
Files.createDirectories(codexDir);
Files.writeString(
codexDir.resolve("auth.json"),
"{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"legacy-oauth\"}}",
StandardCharsets.UTF_8
);
CodexSettingsManager manager = new CodexSettingsManager(new Gson());
JsonObject legacyProvider = new JsonObject();
legacyProvider.addProperty("id", "legacy-config-only");
legacyProvider.addProperty("configToml", "model = \"legacy-model\"\n");
JsonObject nextProvider = provider(
"next",
"model = \"next-model\"\n",
"{\"OPENAI_API_KEY\":\"next-key\"}"
);
manager.transitionProvider(legacyProvider, nextProvider, false, () -> { });
// The user's OAuth session must have been stashed before the overwrite.
assertTrue(Files.exists(codexDir.resolve("auth.json.cli_backup")));
assertEquals("next-key", manager.readAuthJson().get("OPENAI_API_KEY").getAsString());
// Leaving managed mode restores the legacy OAuth session.
manager.transitionProvider(nextProvider, null, true, () -> { });
assertEquals("legacy-oauth", manager.readAuthJson()
.getAsJsonObject("tokens").get("access_token").getAsString());
assertFalse(Files.exists(codexDir.resolve("auth.json.cli_backup")));
}
@Test
public void shouldPreserveUnmanagedAuthWhenLeavingLegacyManagedProvider() throws Exception {
// Legacy state as above; the user goes straight from the legacy managed
// provider to CLI-login without ever switching under the new code.
Path tempHome = Files.createTempDirectory("codex-provider-legacy-deactivate-home");
useTemporaryHomeDirectory(tempHome);
Path codexDir = tempHome.resolve(".codex");
Files.createDirectories(codexDir);
Files.writeString(
codexDir.resolve("auth.json"),
"{\"auth_mode\":\"chatgpt\",\"tokens\":{\"access_token\":\"legacy-oauth\"}}",
StandardCharsets.UTF_8
);
CodexSettingsManager manager = new CodexSettingsManager(new Gson());
JsonObject legacyProvider = new JsonObject();
legacyProvider.addProperty("id", "legacy-config-only");
legacyProvider.addProperty("configToml", "model = \"legacy-model\"\n");
manager.transitionProvider(legacyProvider, null, true, () -> { });
// The unmanaged local credential survives — no backup existed to restore,
// and the current auth.json is not the provider's own credential.
assertEquals("legacy-oauth", manager.readAuthJson()
.getAsJsonObject("tokens").get("access_token").getAsString());
}
@Test
public void shouldNotBackupOutgoingProvidersOwnCredential() throws Exception {
// Transition A→B where auth.json currently holds A's managed credential:
// there is no user credential to preserve, so no backup must appear
// (backing up A's credential would later restore it as if it were local).
Path tempHome = Files.createTempDirectory("codex-provider-owned-auth-home");
useTemporaryHomeDirectory(tempHome);
Path codexDir = tempHome.resolve(".codex");
Files.createDirectories(codexDir);
CodexSettingsManager manager = new CodexSettingsManager(new Gson());
JsonObject providerA = provider(
"a",
"model = \"a-model\"\n",
"{\"OPENAI_API_KEY\":\"a-key\"}"
);
JsonObject providerB = provider(
"b",
"model = \"b-model\"\n",
"{\"OPENAI_API_KEY\":\"b-key\"}"
);
// Simulate legacy: A applied without a backup (write its auth directly).
manager.transitionProvider(null, providerA, false, () -> { });
Files.deleteIfExists(codexDir.resolve("auth.json.cli_backup"));
manager.transitionProvider(providerA, providerB, false, () -> { });
assertFalse(Files.exists(codexDir.resolve("auth.json.cli_backup")));
assertEquals("b-key", manager.readAuthJson().get("OPENAI_API_KEY").getAsString());
// Deactivating B with no backup removes B's managed credential.
manager.transitionProvider(providerB, null, true, () -> { });
assertFalse(Files.exists(codexDir.resolve("auth.json")));
}
@Test
public void shouldNotConfuseProviderOAuthWithLocalCredentialBackup() throws Exception {
Path tempHome = Files.createTempDirectory("codex-provider-oauth-ownership-home");