Skip to content

feat: add session supervisor config plumbing#212

Open
ndycode wants to merge 1 commit intodevfrom
git-clean/pr147-supervisor-plumbing
Open

feat: add session supervisor config plumbing#212
ndycode wants to merge 1 commit intodevfrom
git-clean/pr147-supervisor-plumbing

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 21, 2026

Summary

  • extract the low-risk supervisor plumbing out of feat: add experimental Codex session supervisor #207 into its own PR
  • add the config/schema flag and abort-aware quota probe support needed by the supervisor runtime
  • cover the new config and quota abort behavior with focused tests

Validation

  • npm exec vitest run test/plugin-config.test.ts test/quota-probe.test.ts
  • npm run typecheck

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

clean plumbing PR that wires the codexCliSessionSupervisor boolean flag through schema, config default, env override accessor, and snapshot tests, then adds AbortSignal support to fetchCodexQuotaSnapshot with proper listener cleanup and abort re-checks around every async suspension point.

  • lib/schemas.ts + lib/config.ts: codexCliSessionSupervisor added as an optional zod boolean, defaults to false, overridable via CODEX_AUTH_CLI_SESSION_SUPERVISOR. accessor is synchronous, side-effect free, no token or filesystem exposure — consistent with existing resolveBooleanSetting pattern.
  • lib/quota-probe.ts: signal?: AbortSignal added to ProbeCodexQuotaOptions. the inner AbortController correctly forwards the caller signal via an { once: true } listener, handles the pre-aborted edge case, and cleans up in finally. abort guards are placed at every async boundary (before instructions load, after instructions load, after unsupported-model response). no filesystem access or token leak introduced.
  • test/plugin-config.test.ts: all four supervisor override paths covered; env var lifecycle correctly cleaned up per test. existing config snapshot assertions updated.
  • test/quota-probe.test.ts: four focused abort scenarios cover pre-aborted signal, mid-fetch abort, post-instructions abort, and abort during unsupported fallback — good vitest coverage for the new behavior.
  • one minor redundancy: throwIfQuotaProbeAborted at line 349 (inside try) is a no-op duplicate of line 347 (outside try) — js is single-threaded so the signal cannot flip between two synchronous lines. no runtime impact; see inline comment.

Confidence Score: 5/5

  • safe to merge — all abort paths are correct and tested, no token or filesystem risk introduced.
  • config flag is trivially safe (boolean, env-overridable, no i/o). abort plumbing is mechanically correct: listener cleanup via finally + { once: true } prevents leaks, pre-abort detection guards every async suspension point, and the catch correctly re-surfaces AbortErrors. the only finding is a redundant synchronous guard (line 349) that is dead code but harmless. test coverage is solid across all four new abort scenarios.
  • no files require special attention

Important Files Changed

Filename Overview
lib/config.ts adds codexCliSessionSupervisor boolean flag to DEFAULT_PLUGIN_CONFIG and a clean getCodexCliSessionSupervisor accessor that delegates to existing resolveBooleanSetting with CODEX_AUTH_CLI_SESSION_SUPERVISOR env override. no filesystem i/o, no token exposure, side-effect free.
lib/quota-probe.ts adds optional signal?: AbortSignal to ProbeCodexQuotaOptions and wires abort-awareness throughout fetchCodexQuotaSnapshot. abort propagation and listener cleanup are correct. one redundant synchronous guard at line 349 (inside try) duplicates the check at line 347 (outside try) — dead code, no runtime impact.
lib/schemas.ts adds codexCliSessionSupervisor: z.boolean().optional() to PluginConfigSchema — minimal, consistent with the surrounding schema entries.
test/plugin-config.test.ts correctly adds codexCliSessionSupervisor: false to all existing config-snapshot assertions and adds a focused describe block with four cases covering default, config override, env disable, and env enable. full env var lifecycle (delete, set, delete) is properly managed to avoid state leakage.
test/quota-probe.test.ts adds four targeted abort tests covering: mid-fetch signal fire, pre-aborted signal, signal abort between instructions load and fetch, and abort during unsupported-model fallback chain. covers all meaningful abort checkpoints in the new code. minor: missing blank line separator before the existing "parses reset-at values" test.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant fetchCodexQuotaSnapshot
    participant AbortController
    participant fetch as fetch(Codex API)

    Caller->>fetchCodexQuotaSnapshot: call({ signal })
    loop for each model
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: throwIfAborted(signal) [line 347 – outside try]
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: await getCodexInstructions(model)
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: throwIfAborted(signal) [post-instructions]
        fetchCodexQuotaSnapshot->>AbortController: new AbortController()
        alt signal already aborted
            fetchCodexQuotaSnapshot->>AbortController: controller.abort(signal.reason)
        else
            Caller-->>fetchCodexQuotaSnapshot: signal "abort" event → onAbort fires
            fetchCodexQuotaSnapshot->>AbortController: controller.abort(signal.reason)
        end
        fetchCodexQuotaSnapshot->>fetch: fetch(..., { signal: controller.signal })
        fetch-->>fetchCodexQuotaSnapshot: response / AbortError
        fetchCodexQuotaSnapshot->>fetchCodexQuotaSnapshot: clearTimeout + removeEventListener
        alt signal aborted in catch
            fetchCodexQuotaSnapshot-->>Caller: throw AbortError
        end
    end
    alt signal aborted after loop
        fetchCodexQuotaSnapshot-->>Caller: throw AbortError
    end
    fetchCodexQuotaSnapshot-->>Caller: CodexQuotaSnapshot | lastError
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/quota-probe.ts
Line: 347-349

Comment:
**Redundant consecutive abort guard**

lines 347 and 349 both call `throwIfQuotaProbeAborted` with zero async work between them. js is single-threaded, so `signal.aborted` cannot flip between two synchronous lines. the guard at line 347 (outside the `try`) is the effective early-exit point — it propagates cleanly to the caller without being swallowed by the catch. line 349 (inside `try`) is dead code: if the signal were somehow aborted here the catch would re-throw it anyway, but it can never be aborted here without line 347 having already fired.

```suggestion
	for (const model of models) {
		throwIfQuotaProbeAborted(options.signal);
		try {
			const instructions = await getCodexInstructions(model);
```

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

Reviews (2): Last reviewed commit: "feat: add session supervisor config plum..." | Re-trigger Greptile

@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 21, 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: e3b07675-cfba-463a-901d-6a02b1ab64ee

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 git-clean/pr147-supervisor-plumbing
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch git-clean/pr147-supervisor-plumbing

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 ndycode added the needs-human-review Flagged by automated PR quality screening; maintainer review required. label Mar 21, 2026
@ndycode ndycode added passed and removed passed labels Mar 21, 2026
@ndycode ndycode changed the base branch from main to dev March 21, 2026 19:57
@ndycode ndycode removed the needs-human-review Flagged by automated PR quality screening; maintainer review required. label Mar 22, 2026
@ndycode ndycode deleted the branch dev March 24, 2026 18:37
@ndycode ndycode closed this Mar 24, 2026
@ndycode ndycode deleted the git-clean/pr147-supervisor-plumbing branch March 24, 2026 18:37
@ndycode ndycode restored the git-clean/pr147-supervisor-plumbing branch March 24, 2026 18:40
@ndycode ndycode reopened this Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant