fix: stop deleting reused branches, and finish the glm-5.3-flash correction - #2691
Merged
Conversation
Contributor
|
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 |
github-actions
Bot
added
the
intake: hygiene-blocked
Contributor
|
Contributor
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
3 tasks
github-actions
Bot
removed
the
intake: hygiene-blocked
3 tasks
Owner
Author
리뷰 · 우선순위 70 / 80설명 이 풀 리퀘스트는 #2689 위에 쌓인 후속입니다. 베이스가 닫힌 PR 브랜치 청소 스크립트가 이름만 보고 지우던 문제를 고칩니다. 지금 팁 SHA 가 닫힌 PR 의 head SHA 와 같을 때만 삭제하고, SHA 를 모르면 남깁니다. 워크플로가 또한 #2687 로 심은 CI 는 test 샤드 실패가 보이고 mergeable_state 는 unstable 입니다. #2689 와 같이 Linux shard 이슈(#2695) 영향을 받을 수 있습니다. 미리보기 배포는 계획에 없습니다. .github/scripts/closed-pr-branch-cleanup.cjs - 이름만 맞추던 삭제에 tip SHA 가드를 넣은 방향이 맞습니다 메인테이너의 판단이 필요한 지점
너의 추천 #2689 가 이 댓글은 grok-bot이 작성했습니다 |
7 tasks
lidge-jun
force-pushed
the
codex/pkgtree-503-false-positive
branch
from
August 26, 2026 17:53
de78afb to
8c83b6f
Compare
lidge-jun
force-pushed
the
codex/branch-cleanup-sha-guard
branch
from
August 26, 2026 17:53
cbeda14 to
0bbc4d1
Compare
added 6 commits
August 27, 2026 02:55
The package-tree guard compared the inode CHANGE time, which moves for metadata writes that replace nothing. A chmod, a chown, a touch, an editor normalizing permissions, a backup tool restoring modes - each left device, inode and size identical, so the guard's own definition of "replaced" was met by a file nobody had replaced. Every /v1/* request then answered 503 with "restart OpenCodex before retrying", and since a negative reading is deliberately never cached, there was no recovery short of a restart. Comparing the CONTENT modification time instead. Measured on macOS before changing anything: a chmod moves ctime and leaves mtime alone, an in-place rewrite of the same byte length moves mtime while inode and size hold, and an atomic write-then-rename install changes the inode as well. So mtime drops the false positive and keeps every real detection. The three new tests drive the real filesystem rather than a hand-built observation, because the defect was in which stat field was read and a synthetic fixture cannot tell the two apart - it would have passed before and after. Confirmed by mutation: putting ctimeNs back turns the permission test red and leaves the two replacement tests green. The guard's comment claimed it was detecting "an event that happens at most once per install". That is true of a tree replacement and was never true of a ctime change.
/api/codex-prompt writes the user's config.toml - the file that decides what the model reads - and its write verbs accepted the raw admin token. The auth gate checks that token before it ever consults the session table, so any process that can read ~/.opencodex/admin-api-token could rewrite a prompt. AGENTS.md is explicit that this is the case the session requirement exists to stop. Mutating verbs now require the gui-session principal, the same check the star endpoint uses and for the same reason. Reads stay open to the admin token: describing the layer stack changes nothing and the CLI parity path depends on it. The honest limit, which the code comment states rather than implies: a process running as the user can mint its own session from the loopback dashboard bootstrap, and can edit config.toml directly without going through this proxy at all. This removes the casual path - an agent that would have PUT here because the endpoint existed and the token was lying on disk - and makes the refusal legible. The real boundary is normative. The new test drives all four mutating verbs with an admin-token principal, asserts the 403 and the code, and then asserts config.toml is byte-identical: a refusal that wrote something on the way to refusing is not a refusal. The route harness now passes an explicit principal, which is what surfaced this in the first place - 32 of its cases were silently exercising the untrusted path. The GUI is unaffected: gui/src/api.ts:95-109 authenticates with an ocx_session_ token, which resolves to gui-session.
Two ways the composer could damage a config file it had just reported writing. A UTF-8 BOM is only legal at byte 0, and setProjection inserted its two generated lines at line index 0 - ahead of the BOM. The write verified its own bytes and reported success; Codex could then no longer parse the file. Editors on Windows emit that byte routinely, so this was not an exotic input. The BOM is now split off before line editing and restored after, in all three editors that insert lines: the projection block and both boolean setters. The new tests parse the RESULT rather than asserting the bytes we meant to write. That distinction is the whole lesson here - the old write path was self-consistent and still produced an unloadable file. Bun.TOML is not the parser Codex uses, so a pass is not proof Codex accepts the file, but a failure is proof it does not, and that is the direction the assertion needs to hold in. Separately, only the CONFIG was checked for readability before the transaction began. An unwritable STORE - a directory on its path, a mode change, a full disk - threw out of durableWrite after the config had already been renamed into place. The exception escaped the transaction entirely, so rollback never ran: the caller saw a raw error, the config carried a projection whose store did not exist, and the orphaned journal made every later write fail recovery_required. The write steps are now wrapped, and a throw rolls back to the recorded pre-state and drops the journal. That failure gets its own error, write_failed, mapped to 500 rather than folded into write_superseded. The two are not the same: superseded means another writer won a race, while here nobody won and nothing landed, and retrying the same request unchanged will fail identically until the path or the disk is fixed. Both fixes confirmed by mutation. Removing the BOM handling turns exactly the projection test red; rethrowing instead of rolling back turns exactly the store test red. The two exhaustiveness guards on the write-error status map caught the new error on their own, which is what they were written for.
…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.
lidge-jun
force-pushed
the
codex/pkgtree-503-false-positive
branch
from
August 26, 2026 17:56
8c83b6f to
583f5a2
Compare
lidge-jun
force-pushed
the
codex/branch-cleanup-sha-guard
branch
from
August 26, 2026 17:56
0bbc4d1 to
f25dab8
Compare
lidge-jun
changed the base branch from
codex/pkgtree-503-false-positive
to
dev
August 26, 2026 18:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #2689. Three more findings from the same dev-versus-main review, none
of them in the request path.
The closed-PR branch cleanup could delete a branch someone was using. It
matched branches by NAME alone: any branch whose same-name historical pull
requests were all closed became a deletion candidate, with no check that the
branch still pointed at one of their head commits. A
codex/-style name reusedfor new work inherited the closed history of every PR that had ever carried that
label, and the commits it was carrying 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 side's SHA is unknown.
That script had no test of any kind, which is how this shipped. It has eight now,
including one that walks every pre-existing safety rule so the tip check cannot be
reordered ahead of them.
glm-5.3-flash was still going through the vision sidecar on eight providers.
The earlier correction 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 received a text description of a picture it could have read itself: no error,
a worse answer, an extra call. It was also missing from
ZAI_GLM_53_MODELS, so onZ.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
maxdefault and 131072tokens.
The Moonshot
$refnormalizer relaxed constraints instead of intersectingthem. Its comment claimed the node "narrows the target", which is true only when
the node happens to be narrower. A node declaring
minLength: 1beside a targetdeclaring
minLength: 5emittedminLength: 1— weaker than either side askedfor. Under 2020-12
$refis an in-place applicator: both apply, so the emittedschema is their intersection. Lower bounds now take the max, upper bounds the min.
Verification
Mutation checks:
The Moonshot fix has a test in both directions — a looser sibling must not relax
the target, and a tighter one must still win — so it cannot be satisfied by
always preferring the target, which would discard a genuine narrowing.
The parity assertion for the sidecar classification is written across the whole
registry rather than per provider. The defect was not one entry being wrong, it
was a set of entries drifting apart, and only a global assertion catches the next
provider to seed that id. It also asserts that
glm-5.3stays text-only, so itcannot pass by declaring every GLM row a VLM.
Two of these three findings are mine, from work merged earlier the same day.
No GUI files are touched, so no screenshot applies.
Checklist
The workflow change is the sensitive one. It passes two additional SHAs into an
existing script, adds no permission, and changes the job's behaviour only in the
direction of deleting less.