feat(codex): show the commit-attribution prompt layer - #2692
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c220e2335a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| * `Generated with Codex.` PR marker, disabled sends an explicit countermand. So the | ||
| * row's condition line must name the policy rather than claiming "always on". | ||
| */ | ||
| { id: "git-attribution", class: "runtime-conditional", key: null, default: null, order: null }, |
There was a problem hiding this comment.
On installations using the official 121_openai-codex build, this unconditional descriptor advertises a prompt layer that does not exist: the upstream audit explicitly records that this checkout lacks ext/git-attribution (devlog/_plan/260827_dev_regression_and_prompt_variants/000_plan.md:8-10). Because serialize() sends the complete static inventory and the GUI renders every descriptor, neither the probe result nor the account policy suppresses the row. Detect the installed binary's capability/version and include this descriptor only for Codex variants that actually provide the extension.
Useful? React with 👍 / 👎.
| * `Generated with Codex.` PR marker, disabled sends an explicit countermand. So the | ||
| * row's condition line must name the policy rather than claiming "always on". | ||
| */ | ||
| { id: "git-attribution", class: "runtime-conditional", key: null, default: null, order: null }, |
There was a problem hiding this comment.
Adding this user-visible row leaves docs-site/src/content/docs/guides/codex-prompt.md:20-28 and its translations directly contradicting the dashboard: they still say extension layers cannot be listed and describe only an “Always on” category rather than an account-controlled runtime condition. Update the prompt-layer guide and localized copies so users can interpret the new row and its neutral ordering marker correctly.
AGENTS.md reference: AGENTS.md:L279-L280
Useful? React with 👍 / 👎.
c220e23 to
3684a6b
Compare
리뷰 · 우선순위 58 / 80설명 이 풀 리퀘스트는 #2691 위에 쌓여 있습니다. 베이스는
초안이 아닙니다. 다만 스택 의존이라 #2689→#2691 이 먼저입니다. mergeable_state 는 unstable 이고 테스트 샤드 실패가 보입니다. GUI 스크린샷이 포함되어 있습니다. src/codex/prompt-layers.ts - git-attribution 을 inventory 에 넣는 방향은 HEAD 대비 맞습니다 메인테이너의 판단이 필요한 지점
너의 추천 스택(#2689, #2691)이 이 댓글은 grok-bot이 작성했습니다 |
cbeda14 to
0bbc4d1
Compare
3684a6b to
6ce9bb5
Compare
…ection Three more findings from the dev-versus-main review. The closed-PR branch cleanup matched branches by NAME alone. Any branch whose same-name historical pull requests were all closed became a deletion candidate, without checking that the branch still pointed at one of their head commits. A `codex/`-style name reused for new work inherited the closed history of every PR that had ever carried that label, and the commits it carried had never been in a pull request at all. The planner now requires the current tip to match a closed head SHA, and keeps the branch when either SHA is unknown. The script had no test of any kind, which is how this shipped; it has eight now, including one per existing safety rule so the tip check cannot be reordered ahead of them. glm-5.3-flash was still routed through the vision sidecar on eight providers. The correction commit fixed the two Alibaba entries and left umans, cline-pass, nvidia, zai, zhipu-bigmodel-coding, both Volcengine plans and ollama-cloud behind. That list means "the proxy describes this model's images", so a native VLM sitting in it got a text description of a picture it could read itself: no error, a worse answer, an extra call. The parity assertion is now written across the whole registry rather than per provider, because the defect was entries drifting apart rather than one being wrong. It was also missing from ZAI_GLM_53_MODELS, so on Z.AI it advertised a 1M context with a null effort ladder, no default effort and no output cap while its siblings carried three tiers, a max default and 131072 tokens. The parity test pinned that gap in place because it was written from the incomplete state instead of from the family definition. And the Moonshot `$ref` normalizer overwrote numeric bounds with the sibling's instead of intersecting them. Its comment claimed the node "narrows the target", which is true only when the node happens to be narrower - a node declaring minLength 1 beside a target declaring minLength 5 emitted minLength 1, a contract weaker than either side asked for. That is the same failure the required/properties composition already fixed for set-valued keywords. Lower bounds now take the max, upper bounds the min, and a test covers both directions so the fix cannot become "always prefer the target", which would discard a real narrowing. Each fix is mutation-verified: disabling the tip comparison turns the reused-branch test red on its own, and the looser-bound test was red before the intersection landed and green after.
The hygiene gate flagged the new eslint-disable for no-require-imports, and it was right to: the repo already reaches CommonJS helpers through await import() (ci-workflows.test.ts:5030), so the suppression was covering for a spelling choice rather than an unavoidable constraint. The interop shim is deliberate rather than defensive. A .cjs module reached through ESM can arrive either directly or under default depending on the loader, and picking whichever object actually carries the planner keeps the test honest about what it is calling instead of asserting against undefined.
…state Both were written from what the registry happened to serve rather than from the family definition, so adding glm-5.3-flash to ZAI_GLM_53_MODELS broke them. codex-catalog expected modelSupportsReasoningSummaries to hold exactly the four older 5.3 ids. That map is derived from the family constant, and flash belongs in it: the reasoning-summary question and the vision-sidecar question have different answers for this model, and only the second one excludes it. cline-pass had my own editing mistake in it - I had added the id to a noVisionModels literal that is computed from CLINE_PASS_IMAGE_MODELS, so the expectation contradicted the derivation it was checking. Removed; the list already excludes flash on its own.
…g always-on The prompt panel was under-reporting the prompt by a whole developer section. ext/git-attribution contributes one - the instruction to add a Co-authored-by: Codex trailer to commits the model writes and a Generated with Codex. line to pull requests it opens - and LAYER_INVENTORY had no entry for it. It is runtime-conditional, not a config toggle. lib.rs:33-80 resolves enablement from the auth server and caches it per thread, and features/src/lib.rs:277 records the old config flag as removed, so there is no key for this GUI to write and nothing in [features] to point a user at. The row therefore renders with no switch at all, which is the existing rule for a layer Codex cannot suppress: a disabled control would claim a capability that does not exist. Its order is null because it registers through extensions.context_contributors() rather than appearing in world_state.rs, so its position is registration-order dependent. That exposed a sorting bug: the panel collapsed a null order to 0, which put such a layer at the very TOP and claimed it is assembled first. Null now sorts last. Rendering the page in a real browser then caught something a unit test had not. The row said "Always on" while its dialog described a condition - and so did plugins, which has been shipping that contradiction since the panel landed. The condition map existed and only the dialog read it. The row reads it now, so the two surfaces cannot disagree about the same layer. The probe reports this layer as not-exposed rather than guessing a tag. The Rust source names a <git_attribution> marker pair, but a world-state section is diff-rendered and emits nothing on a turn where its state has not changed: live codex debug prompt-input at 0.145.0 returned 32978 bytes with no such block. The probe header already records that inferring tags from Rust constants produced a wrong mapping once, so this id goes in the unmapped list. Nine locales carry the three new strings. The about text says what a user needs to decide with: that Codex resolves this from the account, and that turning it off sends the opposite instruction rather than sending nothing.
0bbc4d1 to
f25dab8
Compare
6ce9bb5 to
bbfdd65
Compare
1 participant
Summary
Stacked on #2691. The prompt panel was under-reporting the prompt by a whole
developer section.
ext/git-attributioncontributes one — the instruction to add aCo-authored-by: Codextrailer to commits the model writes and aGenerated with Codex.line to pull requests it opens — andLAYER_INVENTORYhad no entry for it. This is the layer the request called "annotation": it lives
in the
120_codex-clicheckout, not in121_openai-codex, which is why a searchof the latter turns up only MCP tool annotations and
url_citationannotations.It is runtime-conditional, not a config toggle.
lib.rs:33-80resolvesenablement from the auth server and caches it per thread;
features/src/lib.rs:277records the old config flag as removed. So there is nokey for this GUI to write and nothing in
[features]to link to, and the rowrenders with no switch at all — the existing rule for a layer Codex cannot
suppress, since a disabled control would claim a capability that does not exist.
Two bugs the work surfaced.
Its
orderis null, because it registers throughextensions.context_contributors()rather than appearing inworld_state.rs. Thepanel collapsed a null order to
0, which put such a layer at the very TOP of thestack and claimed it is assembled first. Null now sorts last.
Rendering the real page in a browser caught the second one, which no unit test
had: the row said "Always on" while its dialog described a condition. So did
plugins, which has been shipping that contradiction since the panel landed. Thecondition map existed and only the dialog read it. The row reads it now.
The row sits last in the state list, carries the neutral position marker rather
than an invented number, has no switch, and states the real condition.
Verification
Live verification against the running proxy, not only the test doubles:
That DOM dump is what found the always-on bug; the API check is what confirms the
descriptor reaches a real client rather than a fixture.
The probe reports this layer as
not-exposedrather than guessing a tag. The Rustsource names a
<git_attribution>marker pair, but a world-state section isdiff-rendered and emits nothing on a turn where its state has not changed — live
codex debug prompt-inputat 0.145.0 returned 32978 bytes with no such block. Theprobe's own header records that inferring tags from Rust constants produced a wrong
mapping once, so this id goes in the unmapped list instead.
Nine locales carry the three new strings; locale parity stays at zero gaps.
Checklist
Nothing here touches auth or credentials. The row is read-only by construction:
the id is absent from
TOGGLE_KEYS, so the write allowlist cannot emit a key forit, and the route's existing table-driven guard already refuses every
non-config-toggle id.