Skip to content

Codex/loopx advisor#2406

Open
maxliux5 wants to merge 30 commits into
huangruiteng:mainfrom
maxliux5:codex/loopx-advisor
Open

Codex/loopx advisor#2406
maxliux5 wants to merge 30 commits into
huangruiteng:mainfrom
maxliux5:codex/loopx-advisor

Conversation

@maxliux5

Copy link
Copy Markdown

Summary

  • Add an opt-in LoopX Turn Advisor mode that uses a stronger model for bounded, read-only advice and a lower-cost model for execution.
  • Capture exact provider token usage for Advisor and executor phases without retaining prompts, raw model output, or trajectories.
  • Add fail-closed usage validation, CLI output budgets, deterministic E2E coverage, and paired live qualification.
  • A one-case provider-backed qualification passed both quality arms and reduced total tokens from 263,472 to 130,782 (50.36%).

Issue Or Task

  • Contributor task ID: issue-xq4gd4vfaykog

Validation

  • python3 -m py_compile loopx/*.py
  • loopx check --scan-root .
  • Other:
    • 987 passed, 2 skipped
    • Ruff and mypy passed
    • Advisor catalog canary passed
    • Provider-backed paired qualification passed

Boundary Checklist

  • I did not commit .loopx/, .codex/goals/, live ACTIVE_GOAL_STATE.md, credentials, private benchmark traces, verifier output, raw agent sessions, internal document links, or local machine
    paths.
  • I did not duplicate maintainer-owned benchmark work unless a maintainer split out a public issue for it.
  • I kept the change scoped to the linked issue/task.

@maxliux5

Copy link
Copy Markdown
Author
image 跑了benchmark,但是目前看Token优化效果一般,我还要优化一下

@maxliux5

Copy link
Copy Markdown
Author
image 整体可以节省 30.30% 的token

@huangruiteng

Copy link
Copy Markdown
Owner

Thanks for the additional benchmark work. I re-reviewed the latest head cfec6970 and both result screenshots.

I am revising my earlier scope recommendation: I do not require this to be split into three PRs. The underlying LoopX Turn path is already experimental, Advisor mode is off when the flag is omitted, and auto requires an explicit --advisor-mode auto. With those boundaries preserved, this can land as one explicitly opt-in experimental feature rather than waiting for production-grade automatic promotion evidence.

The new evidence is meaningful: five real-model cases used the same Sol baseline/Advisor and Luna executor; both arms passed independent validation in all 5 cases; 4/5 cases reduced tokens; aggregate usage fell from 1,166,651 to 813,209 (30.30%). That is enough to justify continued experimentation in main. It is not yet evidence for making Advisor automatic by default: each case is one Turn and one observed run, and arithmetic-fix still regressed by 41.25%. That distinction is fine for an experimental opt-in.

Before merge, please close these focused items without adding another framework.

1. Fix the usage receipt before treating the benchmark totals as exact

model_usage.py still has two correctness problems:

  • event_usage() tries last_token_usage before total_token_usage. When the same final provider event contains both, it records the last segment instead of the cumulative usage.
  • _non_negative_int() accepts a fractional value such as 1.9 and silently converts it to 1. An “exact provider token usage” contract should reject non-integral values rather than truncate them.

Please prefer a valid cumulative total when it is present, accept only real non-negative integers (excluding booleans and fractional numbers), and add regression fixtures containing both last/total usage plus a fractional counter. Then rerun the five-case qualification. The updated receipt, rather than the current screenshot totals, should be the cost evidence used for merge.

2. Keep auto explicitly experimental, not implicitly promoted

The current default-off behavior is the right gate and may remain in this PR. I am also okay with keeping the explicit auto mode and the Sol/Luna candidate profile. Please make the status unambiguous in the user-facing docs and selection evidence: this is an experimental opt-in profile, not a generally qualified/default policy. automatic_promotion_allowed=false in the qualification receipt is still correct; it means “do not turn this on implicitly,” not “the user may not explicitly request the experiment.”

A small wording/receipt adjustment is enough. Please do not build a new feature-flag service or promotion framework here.

3. Either harden the Advisor read boundary or describe it accurately

The temporary empty cwd, read-only, --ephemeral, and bounded literal file packet materially reduce accidental repository access. They are useful controls. They do not prove that the Advisor can read only the packet: Codex still loads the user's configuration and may have configured tools, MCP surfaces, or network access. The prompt instruction not to invoke tools is not a hard isolation boundary.

For this experimental merge, either disable those extra tool/config/network surfaces for the Advisor process, or narrow the documentation claim to say this is bounded context minimization rather than a confidentiality/security sandbox. The latter is acceptable for this PR and keeps the implementation small.

4. Rebase and restore current CI

The PR is currently conflicting with main and has no current GitHub check result. Please resolve the integration conflict and rerun the focused Turn suites plus normal CI on the resulting head.

Validation I ran

  • the focused Codex CLI/model-selection/Turn driver/executor pytest suite passed on cfec6970;
  • git diff --check origin/main...HEAD passed;
  • remote CI is currently absent because the PR head is conflicting.

Decision: hold for the focused correctness/integration fixes above, but no mandatory PR split. After those pass, I am comfortable merging this as a default-off, explicitly experimental Advisor feature.

@maxliux5

Copy link
Copy Markdown
Author
image 最新版的代码,token能够省12%

@maxliux5

Copy link
Copy Markdown
Author
image

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.

2 participants