Skip to content

docs(agents): record what a green pull request does not tell you - #5034

Merged
lidge-jun merged 1 commit into
devfrom
codex/agents-composition-traps
Sep 18, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/agents-composition-traps

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 18, 2026

Copy link
Copy Markdown
Owner

Summary

AGENTS.md does not mention the file-size ratchet at all, so every contributor and every agent rediscovers it by failing CI. This records the three composition traps that produced ten defects in one round, including an hour of red dev.

The facts behind it, measured against this tip rather than remembered:

  • 39 of the 51 ratcheted files sit at exactly their cap, and three more are within five lines. src/server/index.ts, src/config.ts, src/server/responses/core.ts, gui/src/pages/Models.tsx, codex-catalog.test.ts and openai-responses-passthrough.test.ts are among them. One added line fails the ratchet for that branch and every branch cut from dev afterwards.
  • Two branches can each stay under a cap alone and sum over it together — test(combos): hold the zero-output bare-error case in a sibling file #4908, test(server): hold the prototype-named override case in a sibling file #5011 and test: hold the newest catalog and provider cases in sibling files #5018 are the three instances.
  • A missing member of an exhaustive union is not one red suite. typecheck precedes every job, so when a closed translation namespace still listed nine locales after a tenth landed, it failed fourteen checks including docker smoke and all three npm-global smokes.
  • Counts drift silently because both sides write a plausible number: two branches each added one CLI capability and each wrote 47 when the truth was 48; two each added one provider preset and each wrote 94 when the registry had 95.

One observation is included because it cost real time and is not obvious: a moved test is not automatically the same test. A case moved out of codex-v2-gate.test.ts failed in isolation, and then failed again in place once unrelated blocks moved around it, because its final assertion was reading catalog state earlier cases had warmed rather than the contract it documented.

No behaviour changes. This is documentation of mechanisms that already exist and already enforce themselves.

Verification

Local verification was not run because this lane forbids local suites, builds, typechecks, installs and ocx invocations; hosted CI is the executable verification.

The headroom figures were computed by reading tests/fixtures/file-size-baseline.json and counting lines in each tracked path at this tip, which is the same comparison scripts/file-size-ratchet.ts makes.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Documentation
    • Added guidance explaining limitations that may not be apparent from a successful pull request.
    • Documented how file-size thresholds are maintained, including limited remaining headroom and potential merge conflicts.
    • Added cautions for exhaustive updates involving locale catalogs, capability lists, provider presets, and generated counts.
    • Included examples of how incomplete updates can cause checks to fail or leave documented counts out of sync.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 18, 2026 06:59
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-18T07:01:44.015162Z 1c8daea PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: bec2be4d-8524-4618-a7a3-65c3eb8452f8

📥 Commits

Reviewing files that changed from the base of the PR and between baae905 and 1c8daea.

📒 Files selected for processing (1)
  • AGENTS.md

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

AGENTS.md adds guidance about downward-only file-size caps and exhaustive structures over unions. It documents merge-related failure cases, affected test-layout files, and examples of count drift.

Changes

Repository review guidance

Layer / File(s) Summary
File-size and union-exhaustiveness guidance
AGENTS.md:235-283
Adds a section explaining that Math.min keeps file-size caps from increasing, causing additions to fail when files reach their caps. It documents sibling-file registration requirements and moved-test behavior. It also describes union merge failures, translation check failures, and count drift for CLI capabilities and provider presets. It recommends derived counts instead of duplicated counts.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 1c8da

This documentation-only change does not alter runtime behavior and presents no concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and clearly summarizes the main change: documenting what a green pull request does not tell you in AGENTS.md.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 76 / 80

이 PR은 코드를 바꾸지 않습니다. AGENTS.md「초록색 PR이 알려주지 않는 것」 섹션을 하나 넣어서, 각자 브랜치 CI는 통과했는데 합치면 dev가 빨개지는 합성(composition) 함정을 에이전트와 기여자에게 미리 적어 둡니다. 지금 dev 끝은 baae9057b (#5023 catalog contextWindow 백필, 패키지 2.59.0)이고, 이 문서는 그 위에 이미 돌아가는 메커니즘만 설명합니다. 동작·스키마·라벨 변경은 없습니다.

왜 지금이냐면, 파일 크기 래칫과 합집합(union)·카운트 드리프트가 에이전트 작업에서 반복해서 dev를 한 시간씩 빨갛게 만들었기 때문입니다. PR 본문이 말하는 대로, 한 라운드에서 그런 결함이 열 개까지 나왔고 #4908 / #5011 / #5018처럼 각자 캡 아래인데 합치면 캡을 넘는 사례가 이미 있습니다. AGENTS.md에는 지금까지 이 래칫이 거의 안 적혀 있어서, 기여마다 CI에서 다시 배우는 비용이 컸습니다.

문서가 가리키는 사실을 현재 체크아웃에서 다시 쟀습니다. tests/fixtures/file-size-baseline.json에 추적 파일이 51개이고, 그중 39개가 캡과 줄 수가 정확히 같고, 5줄 이내인 파일이 3개입니다. 문서에 예시로 든 src/server/index.ts(893/893), src/config.ts(460/460), src/server/responses/core.ts(210/210), gui/src/pages/Models.tsx(2792/2792), tests/codex-integration/codex-catalog.test.ts(7985/7985), tests/responses/openai-responses-passthrough.test.ts(4809/4809)도 전부 캡에 딱 붙어 있습니다. scripts/file-size-ratchet.tsupdateBaselineMath.min으로만 캡을 내리는 것도 문서 설명과 같습니다. 삽입 위치는 「Do not rerun passing checks…」 바로 다음, 「Minimal containers…」 앞이라 읽기 흐름도 맞습니다.

문서가 가르치는 해결도 맞습니다. 캡을 숫자로 올리는 게 아니라, 케이스를 형제 파일로 옮기고 scripts/test-layout/layout.jsontests/fixtures/test-layout-expected.json 둘 다 등록하라는 것, 그리고 옮긴 테스트가 예전 케이스가 데운 카탈로그 상태에 기대면 색이 바뀐다는 경고는 합성 실패를 줄이는 실질적인 안내입니다. 합집합·로케일·손수 쓴 카운트(47→48, 94→95) 쪽도 「한 쪽만 통과한 브랜치가 합치면 typecheck부터 쓰러진다」는 교훈을 초등학생도 따라갈 말로 정리했습니다.

작은 주의만 있습니다. 「작성 시점」 숫자(39/51, 47·94 예시)는 dev가 움직일수록 낡습니다. 메커니즘 설명은 오래가고, 스냅샷 숫자는 예시로 남기면 됩니다. CI는 아직 pending이라 mergeable_state가 blocked인 상태입니다. 문서만 49줄 추가이므로 동작 회귀 위험은 사실상 없습니다.

라인 238 - 새 섹션 제목·본문이 AGENTS 흐름에 잘 맞음 (통과 체크 재실행 금지 바로 다음)
라인 248251 - 래칫 헤드룸·예시 경로가 현재 baseline과 일치함
라인 253
257 - 합성 캡 초과 시 형제 파일 이동 + layout 이중 등록 안내가 정확함
경로/tests/fixtures/file-size-baseline.json - 「작성 시점」 39/51 숫자는 시간이 지나면 어긋날 수 있음 (예시로 읽으면 됨)
경로/CI - 호스트 CI 아직 pending; 로컬 스위트 생략은 이 레포 문서 레인 규칙과 맞음

메인테이너의 판단이 필요한 지점

  • 「작성 시점」 헤드룸 숫자를 이대로 둘지, 「거의 모든 추적 파일이 캡에 붙어 있다」처럼 더 오래가는 문장으로 바꿀지
  • 이 내용을 AGENTS.md에만 둘지, 사람 기여자용 CONTRIBUTING에도 한 줄 링크를 둘지
  • requested_reviewers에 Ingwannu가 있으니, 문서 레인 승인 경로를 그 리뷰에 맡길지 바로 머지할지

너의 추천
CI(특히 hygiene / changes)가 초록이 되면 머지하세요. types/config 스플릿에 무효화될 코드도 없고, 중복 PR도 아니며, 합성으로 dev가 빨개지는 비용을 줄이는 문서입니다. 숫자 스냅샷이 거슬리면 머지 전에 「거의 전부 캡에 붙어 있다」 정도로만 부드럽게 고쳐도 됩니다.

이 댓글은 grok-bot이 작성했습니다

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Merging with every expensive leg reporting skipped, and recording that rather than letting the green aggregate speak for itself.

This change touches one file, AGENTS.md, which is not in the ci path filter. So gates and all four test shards skipped and the aggregate ci check reports success without having verified anything. That is the gap recorded in #5001, and it is the same shape that made #4999 unverifiable by the pull-request event.

I am not dispatching a full lane=all run for this one, and the reason is that the evidence such a run could produce is not evidence about this change. The diff is English prose in a markdown file. Nothing in the build, typecheck or test path reads it, it declares no configuration, and it contains no credential or path that privacy:scan could have an opinion about. A full cross-platform matrix would prove that the rest of the repository still works, which nobody doubts, at the cost of an hour of queue while eight other pull requests are waiting for runners.

What the change asserts is factual rather than behavioural, and those facts were computed against this tip: 39 of 51 ratcheted files sit at exactly their cap, three more are within five lines, and the named files are the ones the baseline actually lists. If any of those numbers drift, the text goes stale rather than the repository breaking.

@lidge-jun
lidge-jun merged commit cc90bed into dev Sep 18, 2026
21 of 22 checks passed
@lidge-jun
lidge-jun deleted the codex/agents-composition-traps branch September 18, 2026 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant