Repository navigation
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ch path uses
plan_codex_live_write decided official-ness from provider.category alone. The
rest of the switch path — including its own post-switch auth.json cleanup —
asks is_codex_official_provider, which also recognises the fixed official row
and any provider bound to a managed codex_oauth account, whatever their stored
category says.
A card official to the predicate but not to the string was planned as a
third-party switch: write_full_auth stayed false, so its ChatGPT login never
reached auth.json, and remove_auth_file became !preserve_official_login — true
by default — so the live login was deleted. Switching to your own official card
signed you out, and the post-switch cleanup, which does use the predicate, then
found nothing left to clean.
One managed-account path in services::proxy already hardcoded Some("official")
to sidestep this. codex_live_write_category names the rule once, and both the
preflight and the write now read it, so the two cannot disagree about which
provider they are judging.
d368164 to
13f1c06
Compare
|
@ntdatt812 感谢您的提交,「写入计划必须和切换路径的其他部分用同一个 official-ness 定义」这个不变量意识非常好——两套判定并存正是会出这类 bug 的地方! 这个 PR 的目标已经在 main 中实现,将以此为依据关闭本 PR。 main 中的对应实现PR 针对的 pub(crate) fn is_official(provider: &Provider) -> bool {
provider.category.as_deref() == Some("official")
|| crate::proxy::providers::is_codex_official_provider(provider)
}模块头注释明确「官方卡: 您的工作没有白做「两套 official-ness 定义并存会出 bug」的分析完全正确——main 的重构正是把判定收敛成一处;绑定托管 如果您在最新 main 上复现出「绑定托管账号的卡片被写入路径当第三方」的场景,欢迎开 issue 附复现步骤,我们按场景处理。 我将关闭这个 PR,再次感谢您的时间和耐心! |
Two definitions of "official", and the live write uses the weaker one
plan_codex_live_writedecides official-ness from one thing:The rest of the switch path asks a different question.
services/provider/mod.rs— including the block that cleans upauth.jsonafter the very write this plan produces — uses:And
is_codex_official_provideris broader on purpose. It returns true for the fixed official row, and it returns true for any provider bound to a managedcodex_oauthaccount, regardless of whatcategoryholds:So a card that every other part of the switch treats as official can reach the plan and be judged third-party.
What that costs
The third-party branch does two things the official branch explicitly does not:
write_full_authcodex_auth_has_login_material(auth)falseremove_auth_filefalse!preserve_official_loginPreservation is opt-in and defaults to false, so for such a card:
auth, is never written toauth.json— the plan is config-only; andauth.jsonis deleted.Switching to your own official card signs you out. Then
services/provider/mod.rsrunsclear_stale_codex_live_auth_after_official_switch— which does use the broad predicate — finds no file, and returnsOk(false). Nothing is logged, because from that block's point of view there was nothing stale to clean.record_codex_managed_oauth_live_auth, which runs right after the write, does not rescue it: it writes the ownership marker file, notauth.json.The fix
The evidence that this is a known-shaped problem is already in the tree — one managed-account path in
services/proxy.rshardcodes the category:That is the right call made in one place. This PR names the rule once instead:
and routes the five sites that build a Codex live write through it —
services/config.rs,services/provider/live.rs(both the preflight and the write), and the twoservices/proxy.rssites.Including the preflight is the part I would not want dropped.
plan_codex_live_write's own doc says validation and execution live in one builder so they cannot drift; that guarantee is only worth anything if both are handed the same provider identity. A preflight that judged a card third-party while the write judged it official would refuse switches that would have succeeded, or pass ones that then behave differently.I deliberately did not touch
is_codex_official_provideritself. Widening the predicate would change proxy routing and base-URL resolution, which is a much larger blast radius than the write-planning bug actually needs.Verification
Two tests, and the first is red against
main.The harm is asserted before the mechanism on purpose — with the assertions the other way round, the failure reads "expected
Some("official"), gotSome("codex")", which says what a helper returned rather than what breaks for the user. It also opens with a precondition assert thatis_codex_official_provideralready calls the fixture official, so the test cannot quietly become vacuous if that predicate changes.The second test pins the other direction: a genuine third-party card keeps its own category, still deletes
auth.jsonwith preservation off, and stays config-only. Its fixture carries a real[model_providers.relay]table — my first attempt used an empty config, which is refused outright (provider.codex.config.missing) and would have passed for the wrong reason.cargo test --lib: 2713 passed, 2 failed. Both failures are pre-existing and environmental —resolve_catalog_rejects_symlink_escaping_config_dirandsymlink_cycle_does_not_cause_stack_overflowboth die onOs { code: 1314, "A required privilege is not held by the client." }, this Windows box having no symlink privilege. Neither is in a file this branch touches.cargo clippy --all-targets -- -D warnings→ clean.cargo fmt --allapplied.Scope note
This is not a fix for #6925, which reports the reverse direction (official → third-party with preservation on). I went looking for that one and could not reproduce it in the plan —
remove_auth_file = !preserve_official_loginis correct there and covered by existing tests — so I am not claiming it. This is a different reachable path to the same symptom, found on the way.