Skip to content

fix(devin): fold counter-read findings into the supportsImages propagation - #4557

Merged
lidge-jun merged 1 commit into
devfrom
codex/260914-l2-devin-modalities-fold
Sep 13, 2026
Merged

fix(devin): fold counter-read findings into the supportsImages propagation#4557
lidge-jun merged 1 commit into
devfrom
codex/260914-l2-devin-modalities-fold

Conversation

@lidge-jun

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

Copy link
Copy Markdown
Owner

Summary

  • Follow-up to feat(devin): propagate catalog supportsImages to the advertised catalog #4556 (merged as 72335fc): the adversarial counter-read of that diff returned after the merge and its findings are folded here. No behavior change.
  • The advertised-catalog tests in tests/providers/devin-live-models.test.ts now stub globalThis.fetch to throw, so a seeded-catalog cache miss fails the test instead of dialling Cognition — the "never touches the network" claim is now enforced rather than assumed.
  • The structure/catalog.md precedence sentence is scoped to inputModalities and moved out of the TTL paragraph: the broader claim (covering live contextWindow and reasoningEfforts) was not literally true, because those fields have their own configured sources that overwrite the live values through the same hints pass.
  • The structure/adapters/registry.md collapse wording now covers the whole EFFORT_TOKENS suffix set (-1m, -fast, -priority included), matching what collapseDevinModelUid actually strips.

Verification

  • Local product suite, typecheck, build, and install were NOT RUN (repository policy for this delivery round). The only proof is hosted Cross-platform CI at the exact head SHA, queued by the push on the pull_request event: run 34783132657 at exact head 1fdf02fdc6cc6360f0cc53b2b5a1e356e6ac78a9 (in progress at PR open time).
  • Debugging-only, not proof: bun run structure:check passed locally on the same content before the follow-up branch was cut.
  • No GUI change; no screenshot.

Checklist

  • Scope stays focused and avoids unrelated cleanup. (Three files, test hardening plus doc truthfulness only.)
  • Docs or release notes were updated when needed. (The two structure sentences this PR corrects are the docs.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (No auth/credential/workflow/release surface; test-only network stub.)

Summary by CodeRabbit

  • Documentation

    • Clarified how capability information is aggregated across Devin live models, including effort variants and tier rows.
    • Updated live-model catalog guidance to clarify which measured values are retained and how catalog-provided values take precedence.
  • Tests

    • Improved test isolation for live-model catalog lookups by detecting unexpected network requests and restoring test environment state after each test.

The adversarial counter-read of 5d95dbf returned two should-fix findings
and one nit, all folded here: the advertised-catalog tests now stub
globalThis.fetch to throw, so a seeded-cache miss fails the test instead
of dialling Cognition; the catalog.md precedence sentence is scoped to
inputModalities (live contextWindow and reasoningEfforts have their own
configured sources and the broader claim was not literally true) and moved
out of the TTL paragraph; and the registry.md collapse wording covers the
whole EFFORT_TOKENS suffix set rather than only effort variants.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 13, 2026 21:12
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 13, 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-13T21:14:43.023913Z 1fdf02f 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.

@github-actions github-actions Bot added the bug Something isn't working label Sep 13, 2026
@coderabbitai

coderabbitai Bot commented Sep 13, 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: d5f8e45f-23bf-4163-9047-1292eedd5238

📥 Commits

Reviewing files that changed from the base of the PR and between 72335fc and 1fdf02f.

📒 Files selected for processing (3)
  • structure/adapters/registry.md
  • structure/catalog.md
  • tests/providers/devin-live-models.test.ts

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


📝 Walkthrough

Walkthrough

The changes clarify Devin live-model documentation and add network isolation to the Devin live-model test lifecycle. The documentation covers capability aggregation and live-row input-modality precedence.

Changes

Devin live models

Layer / File(s) Summary
Live-model aggregation and merge documentation
structure/adapters/registry.md, structure/catalog.md
The registry documentation covers capability aggregation across each base model’s collapsed UID, including effort suffixes and tier rows. The catalog documentation states that only measured inputModalities are spread before provider-config hints.
Test network isolation
tests/providers/devin-live-models.test.ts
The test setup saves and replaces globalThis.fetch with a failing stub. Cleanup restores the original fetch and retains catalog and model-cache reset behavior.

Priority: ⬇️ Low

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

Change: Other

Suggested reviewers: olddonkey

Merge Risk: ⚪ Minimal · up to 1fdf0

The changes are limited to accurate documentation and scoped test network isolation, with no concrete merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is related to the Devin changes. It identifies the follow-up counter-read findings and the supportsImages propagation topic covered by the documentation and test updates.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/260914-l2-devin-modalities-fold

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

리뷰 · 우선순위 58 / 80

이 PR은 방금 dev에 들어간 #4556(72335fc, Devin 카탈로그 supportsImages → 광고 카탈로그 전파)의 카운터리드 후속입니다. 동작은 바꾸지 않습니다. #4556이 남긴 세 가지 헐거움을 고칩니다. (1) tests/providers/devin-live-models.test.ts가 “네트워크를 안 탄다”고만 가정하고 있었고, (2) structure/catalog.md가 라이브 우선순위를 contextWindow·reasoningEfforts·inputModalities 전부에 걸친 것처럼 넓게 적었고, (3) structure/adapters/registry.md가 접기 대상을 “effort 변형”으로만 적어 EFFORT_TOKENS 전체(특히 -1m·-fast·-priority)와 어긋났습니다.

지금 로컬 dev HEAD는 72335fc6a입니다. 스냅샷과 같고, 직전 랜딩이 바로 #4556입니다. 패키지는 2.55.0 라인입니다. 이 PR은 새 기능을 더하는 칸이 아니라, 방금 연 전파 레이어의 문서·테스트 진실성을 같은 라운드에서 잠그는 칸입니다. base는 dev입니다.

왜 중요하냐면, #4556 리뷰·문서가 “광고 카탈로그 테스트는 시드 캐시만 본다”·“라이브가 힌트 앞에 펼쳐지되 선언/사이드카가 이긴다”·“접기는 effort 변형 투표”라고 말해 두었는데, 코드·문서가 그 문장을 문자 그대로 지키지 않으면 다음 기여자가 틀린 계약을 읽습니다. 특히 catalog.md의 넓은 문장은 사실이 아닙니다. src/codex/catalog/provider-fetch.ts에서 contextWindow는 발견값을 설정 cap이 누르는 경로이고, reasoningEffortsconfiguredReasoningEfforts가 다시 채웁니다. 라이브가 “살아남는” 우선순위 이야기의 진짜 주인공은 inputModalities뿐입니다. 이 PR이 그 문장을 좁히고 TTL 단락 밖으로 빼서, #4556이 실제로 한 일과 문서를 맞춥니다.

테스트 쪽 변경은 작지만 의미가 큽니다. beforeEach에서 globalThis.fetch를 던져 버리게 바꿔, 시드 캐시 미스가 Cognition(GetCascadeModelConfigs)으로 새면 케이스가 바로 실패합니다. afterEach에서 원래 fetch를 되돌립니다. 주석도 “캐시 미스는 버그”라고 못 박습니다. 지금까지는 setCachedCatalogForTests + 고유 KEY/HOST 매칭에만 기대 “안 탈 것”이었고, 시드가 빠지거나 키가 어긋나면 조용히 실네트워크로 갈 수 있었습니다. 이제는 그 가정이 강제됩니다.

registry.md 문구는 src/adapters/devin/live-models.tsEFFORT_TOKENS(low/medium/high/xhigh/max/none/fast/priority/1m)와 collapseDevinModelUid가 실제로 벗기는 접미사 집합에 맞춥니다. “effort variants”만 말하면 서비스 티어·컨텍스트 변형(-fast/-priority/-1m)이 투표 바구니 밖처럼 읽히는데, 코드는 그들도 같은 UID 접기로 모읍니다. REASONING_RUNG_TOKENS와 접기 집합이 다르다는 기존 주석과도 이제 문서가 충돌하지 않습니다.

변경 파일은 세 개뿐입니다. 프로덕션 런타임 경로(live-models.ts / provider-fetch.ts / catalog 파서)는 손대지 않았습니다. types.ts/config.ts 분할과도 무관하고, 닫을 중복 PR도 아닙니다. 라벨은 bug인데, 성격은 “#4556 직후 문서·테스트 정합”에 가깝습니다. 로컬 product/typecheck/build/install은 이번 라운드 정책상 안 돌렸고, 증거는 호스트 Cross-platform CI입니다. 리뷰 작성 시점 기준 resolve-pr/enforce-target/hygiene/changes/label 등은 통과했고, test·gates·docker smoke·keyring·api usage 등은 아직 pending입니다.

tests/providers/devin-live-models.test.ts beforeEach fetch 스텁 - 캐시 미스를 실패로 바꾸는 방향은 맞습니다. 다만 스텁이 Cognition만이 아니라 모든 fetch를 던집니다. 이 파일 안에 나중에 다른 HTTP 픽스처가 들어오면 같이 깨집니다. 지금은 시드-only라 괜찮고, 범위가 파일 전체 beforeEach인 점만 기억하면 됩니다.

tests/providers/devin-live-models.test.ts afterEach 복원 - realFetch를 beforeEach에서 대입합니다. beforeEach가 대입 전에 죽으면 afterEach가 undefined를 넣을 수 있는 아주 얇은 모서리입니다. 실무 영향은 거의 없고, 원하면 모듈 로드 시점에 한 번 캡처해도 됩니다.

structure/catalog.md - inputModalities만 남긴 문장은 provider-fetch.ts Devin 라이브 맵의 스프레드 + 힌트 덮어쓰기와 일치합니다. contextWindow/reasoningEfforts를 뺀 것이 맞는 수정입니다.

structure/adapters/registry.md - “EFFORT_TOKENS 접미사, -1m 같은 티어 행 포함” 표현이 collapseDevinModelUid와 맞습니다. “effort variants”보다 정확합니다.

CI - 로컬 스위트를 안 돌린 라운트라 머지 게이트의 자동 증거는 호스트 CI뿐입니다. pending 샤드가 그린이 되기 전에 랜딩하지 마세요.

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

  • 라벨을 bug로 둘지, 문서/테스트 정합용으로 documentation·후속 태그로 바꿀지
  • fetch 스텁을 파일 전역 throw로 둘지, Cognition URL만 막도록 좁힐지(지금은 전역이 feat(devin): propagate catalog supportsImages to the advertised catalog #4556 계약 강제에 더 확실함)
  • 이 후속을 #4556과 같은 2.55.0 Devin polish 묶음으로 바로 넣을지, CI만 보고 단독 랜딩할지
  • Cross-platform CI(test/gates/docker 포함) 그린을 머지 필수 게이트로 고정할지

너의 추천
CI 그린 확인 뒤 이 PR은 머지하세요. 동작 변경 없이 #4556이 약속한 세 문장(네트워크 차단 강제, inputModalities만의 우선순위, EFFORT_TOKENS 전체 접기)을 문서·테스트에 잠그는 정확한 후속입니다. types/config 분할과도 충돌하지 않습니다. 머지 전에만 pending CI를 보고, 랜딩 후에는 추가 후속이 필요 없습니다.

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

@lidge-jun

Copy link
Copy Markdown
Owner Author

Integrating through the maintainer self-integration path in MAINTAINERS.md and recording that choice here.

Exact-head evidence: Cross-platform CI run 34783132657 completed success at 1fdf02fdc6cc6360f0cc53b2b5a1e356e6ac78a9. Local product suite, typecheck, build and install NOT RUN.

These are the counter-read folds that raced the #4556 merge. Nothing here changes behavior, but two of the three are worth naming because they are corrections of claims rather than of code.

The test now stubs fetch so a seeded-cache miss fails loudly instead of quietly dialling Cognition — a test that reaches the network when its fixture is wrong is a test that can pass for the wrong reason. The structure/catalog.md precedence sentence was scoped down to inputModalities: as written it also claimed the ordering for contextWindow and reasoningEfforts, which have their own configured sources and do not follow it. And the registry.md wording was widened to the full EFFORT_TOKENS suffix set rather than the subset it happened to name.

Recording the process fact too, since it is the third sighting tonight: the devin/swe-2 counter-read for this layer hung on a transient rate limit and was re-dispatched on grok-4.6, which returned findings with zero must-fix. The folds above are the should-fix items from that pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant