fix: address PR review comments

- Add error logging to CodexProvider auth check instead of silent failure
- Fix cachedAt timestamp to return actual cache time instead of request time
- Replace misleading hardcoded rate limit values (100) with sentinel value (-1)
- Fix unused parameter warning in codex routes

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Shirone
2026-01-10 16:26:12 +01:00
parent 604f98b08f
commit c99883e634
4 changed files with 31 additions and 10 deletions

View File

@@ -21,6 +21,7 @@ import {
extractTextFromContent,
classifyError,
getUserFriendlyErrorMessage,
createLogger,
} from '@automaker/utils';
import type {
ExecuteOptions,
@@ -658,6 +659,8 @@ async function loadCodexInstructions(cwd: string, enabled: boolean): Promise<str
.join('\n\n');
}
const logger = createLogger('CodexProvider');
export class CodexProvider extends BaseProvider {
getName(): string {
return 'codex';
@@ -1045,7 +1048,7 @@ export class CodexProvider extends BaseProvider {
return { authenticated: true, method: 'oauth' };
}
} catch (error) {
// Silent fail
logger.warn('Error running login status command during auth check:', error);
}
}

View File

@@ -12,7 +12,7 @@ export function createCodexRoutes(
const router = Router();
// Get current usage (attempts to fetch from Codex CLI)
router.get('/usage', async (req: Request, res: Response) => {
router.get('/usage', async (_req: Request, res: Response) => {
try {
// Check if Codex CLI is available first
const isAvailable = await usageService.isAvailable();
@@ -60,7 +60,7 @@ export function createCodexRoutes(
router.get('/models', async (req: Request, res: Response) => {
try {
const forceRefresh = req.query.refresh === 'true';
const models = await modelCacheService.getModels(forceRefresh);
const { models, cachedAt } = await modelCacheService.getModelsWithMetadata(forceRefresh);
if (models.length === 0) {
res.status(503).json({
@@ -74,7 +74,7 @@ export function createCodexRoutes(
res.json({
success: true,
models,
cachedAt: Date.now(),
cachedAt,
});
} catch (error) {
logger.error('Error fetching models:', error);

View File

@@ -86,6 +86,24 @@ export class CodexModelCacheService {
return this.refreshModels();
}
/**
* Get models with cache metadata
*
* @param forceRefresh - If true, bypass cache and fetch fresh data
* @returns Object containing models and cache timestamp
*/
async getModelsWithMetadata(
forceRefresh = false
): Promise<{ models: CodexModel[]; cachedAt: number }> {
const models = await this.getModels(forceRefresh);
// Try to get the actual cache timestamp
const cached = await this.loadFromCache();
const cachedAt = cached?.cachedAt ?? Date.now();
return { models, cachedAt };
}
/**
* Refresh models from app-server and update cache
*

View File

@@ -157,9 +157,9 @@ export class CodexUsageService {
if (rateLimitsResult?.rateLimits?.primary) {
const primary = rateLimitsResult.rateLimits.primary;
result.rateLimits!.primary = {
limit: 100, // Not provided by API, using placeholder
used: primary.usedPercent,
remaining: 100 - primary.usedPercent,
limit: -1, // Not provided by API
used: -1, // Not provided by API
remaining: -1, // Not provided by API
usedPercent: primary.usedPercent,
windowDurationMins: primary.windowDurationMins,
resetsAt: primary.resetsAt,
@@ -170,9 +170,9 @@ export class CodexUsageService {
if (rateLimitsResult?.rateLimits?.secondary) {
const secondary = rateLimitsResult.rateLimits.secondary;
result.rateLimits!.secondary = {
limit: 100,
used: secondary.usedPercent,
remaining: 100 - secondary.usedPercent,
limit: -1, // Not provided by API
used: -1, // Not provided by API
remaining: -1, // Not provided by API
usedPercent: secondary.usedPercent,
windowDurationMins: secondary.windowDurationMins,
resetsAt: secondary.resetsAt,