Skip to content

refactor: split forecast and report cli flows#143

Closed
ndycode wants to merge 1 commit intoplan/02-cli-help-and-login-splitfrom
plan/03-cli-diagnostics-split
Closed

refactor: split forecast and report cli flows#143
ndycode wants to merge 1 commit intoplan/02-cli-help-and-login-splitfrom
plan/03-cli-diagnostics-split

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • Continue the CLI modularization stack by moving auth forecast and auth report into a dedicated command module.

What Changed

  • Added lib/codex-manager/forecast-report-commands.ts for forecast/report argument parsing, JSON serialization, live quota probe flow, and report file output.
  • Kept lib/codex-manager.ts focused on dispatch plus shared dependency wiring for the extracted commands.

Validation

  • npm run lint
  • npm run typecheck
  • npm test
  • npm test -- test/documentation.test.ts
  • npm run build

Docs and Governance Checklist

  • README updated (if user-visible behavior changed) - not needed; internal refactor only
  • docs/getting-started.md updated (if onboarding flow changed) - not needed; onboarding behavior unchanged
  • docs/features.md updated (if capability surface changed) - not needed; capability surface unchanged
  • relevant docs/reference/* pages updated (if commands/settings/paths changed) - not needed; command behavior unchanged
  • docs/upgrade.md updated (if migration behavior changed) - not needed; no migration change
  • SECURITY.md and CONTRIBUTING.md reviewed for alignment

Risk and Rollback

  • Risk level: Low. This is a behavior-preserving extraction of the forecast/report CLI path with the same validation surface rerun after the split.
  • Rollback plan: Revert commit d1d2a7a.

Additional Notes

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this PR extracts the auth forecast and auth report CLI flows from the monolithic lib/codex-manager.ts into a dedicated lib/codex-manager/forecast-report-commands.ts module, following the same dependency-injection pattern already used by auth-commands.ts. behavior is preserved: the riskTone/availabilityTone helpers are correctly inlined as ternaries, and the joinStyledSegments replacement (rowParts.join(deps.stylePromptText(" | ", "muted"))) is functionally identical since joinStyledSegments also joined with " | ".

key concerns:

  • no vitest coverage for the new module; parseForecastArgs, parseReportArgs, runForecast, and runReport are all individually testable now but have no dedicated test file — this leaves the extracted logic uncovered by unit tests.
  • three local type redeclarations (PromptTone, QuotaEmailFallbackState, QuotaCacheAccountRef) are copied from codex-manager.ts instead of being moved to a shared types file; silent drift risk if upstream definitions change.
  • windows path safety: the --out value is resolved directly via resolve(process.cwd(), outPath) with no cwd-boundary check; a relative traversal path (../../etc/foo) will silently write outside the working directory on any platform.
  • pre-existing asymmetry preserved: runReport (live mode) always calls queuedRefresh for every account without first checking hasUsableAccessToken, unlike runForecast which guards the refresh. this is not a regression introduced here, but now that the code is isolated and unit-testable it's a good time to align the two.

Confidence Score: 4/5

  • safe to merge — behavior is preserved, no regressions introduced; minor tech-debt items should be tracked
  • clean, verified extraction with matching lint/typecheck/test passes; the issues flagged (missing tests, local type copies, path validation) are all pre-existing patterns or non-blocking style concerns, not regressions or new logic bugs
  • lib/codex-manager/forecast-report-commands.ts — missing test coverage, local type redeclarations, and --out path traversal

Important Files Changed

Filename Overview
lib/codex-manager/forecast-report-commands.ts new 622-line module containing the extracted forecast/report CLI flows; behavior is preserved, but local type redeclarations (PromptTone, QuotaEmailFallbackState, QuotaCacheAccountRef), no vitest coverage, and an unsanitized --out path (windows traversal risk) need attention
lib/codex-manager.ts clean removal of forecast/report code; new forecastReportCommandDeps wiring object correctly passes all required helpers; thin shim wrappers for runForecast/runReport look correct

Sequence Diagram

sequenceDiagram
    participant CLI as runCodexMultiAuthCli
    participant CM as codex-manager.ts
    participant FRC as forecast-report-commands.ts
    participant Storage as storage.js
    participant Refresh as refresh-queue.js
    participant Quota as quota-probe.js
    participant Cache as quota-cache.js

    CLI->>CM: auth forecast / auth report
    CM->>FRC: runForecastCommand(args, deps)
    FRC->>Storage: setStoragePath(null) + loadAccounts()
    Storage-->>FRC: AccountStorageV3

    alt --live flag set
        FRC->>Cache: loadQuotaCache()
        Cache-->>FRC: QuotaCacheData
        loop each enabled account
            FRC->>Refresh: queuedRefresh(refreshToken)
            Refresh-->>FRC: TokenResult
            FRC->>Quota: fetchCodexQuotaSnapshot(accountId, accessToken, model)
            Quota-->>FRC: QuotaSnapshot
            FRC->>Cache: updateQuotaCacheForAccount(...)
        end
        FRC->>Cache: saveQuotaCache(workingCache)
    end

    FRC->>FRC: evaluateForecastAccounts(inputs)
    FRC->>FRC: summarizeForecast + recommendForecastAccount

    alt --json flag
        FRC->>CLI: console.log(JSON)
    else --out path (report only)
        FRC->>Storage: fs.writeFile(outputPath, JSON)
        FRC->>CLI: console.log(human summary)
    else human output
        FRC->>CLI: console.log(styled rows)
    end
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/codex-manager/forecast-report-commands.ts
Line: 28-37

Comment:
**local type redeclarations that can silently drift**

`PromptTone`, `QuotaEmailFallbackState`, and `QuotaCacheAccountRef` are copied verbatim from `codex-manager.ts` rather than imported. since `codex-manager.ts` doesn't export them today, the duplication is forced — but that's the root problem. if either definition changes upstream (e.g. a new `QuotaEmailFallbackState` field), this module won't pick it up and the compiler won't complain because the shapes currently match.

the cleaner fix is to move these shared types into a `lib/codex-manager/types.ts` (or `lib/types.ts`) and export from both call sites. that pattern already exists in the codebase for `TokenFailure` and `ModelFamily`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/codex-manager/forecast-report-commands.ts
Line: 1251-1254

Comment:
**windows filesystem risk: unsanitized user-supplied `--out` path**

`resolve(process.cwd(), options.outPath)` is passed straight from argv. on windows, UNC paths (`\\server\share\...`) and drive-relative paths (`C:report.json`) resolve differently than posix paths — `resolve` handles them correctly, but the real concern is path traversal: a user (or a script piping args) can pass `../../sensitive/path` and write outside the intended working directory with no validation or confirmation.

consider validating that the resolved path stays within the cwd subtree, or at minimum add a note in `printReportUsage` warning that the `--out` value is written as-is:

```suggestion
	if (options.outPath) {
		const outputPath = resolve(process.cwd(), options.outPath);
		const cwd = process.cwd();
		if (!outputPath.startsWith(cwd + path.sep) && outputPath !== cwd) {
			console.error(`--out path must be within the current working directory: ${cwd}`);
			return 1;
		}
		await fs.mkdir(dirname(outputPath), { recursive: true });
		await fs.writeFile(outputPath, `${JSON.stringify(report, null, 2)}\n`, "utf-8");
	}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: lib/codex-manager/forecast-report-commands.ts
Line: 1-5

Comment:
**missing vitest coverage for the extracted module**

no new test file was added alongside this extraction. the original logic lived inside the large `codex-manager.ts` where it was covered indirectly via `test/codex-manager-cli.test.ts`, but the newly exported functions (`parseForecastArgs`, `parseReportArgs`, `runForecast`, `runReport`) are now individually unit-testable with simple mocked `ForecastReportCommandDeps`.

per the test/AGENTS.md coverage thresholds (80%+ across statements/branches/functions/lines), a `test/codex-manager/forecast-report-commands.test.ts` should be added that at minimum covers:
- `parseForecastArgs` / `parseReportArgs` arg edge cases (`--model=`, missing value, unknown flag)
- `runForecast` and `runReport` with a fully-mocked deps object (no accounts, json mode, live probe)
- the `--help` early-exit path for both commands

the concurrent sequential-refresh loop in `runReport` (live mode always re-refreshes every account token, unlike `runForecast` which guards with `hasUsableAccessToken`) is particularly worth a dedicated test to ensure it stays correct after future refactors.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "refactor: split fore..."

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b05b2d6b-22a0-4761-8200-00f70f322cbe

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch plan/03-cli-diagnostics-split
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch plan/03-cli-diagnostics-split

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant