docs(devlog): close the Devin ACP removal unit - #4417
Conversation
Both work phases landed on dev (#4411 as 2d3c05f, #4415 as 213065e), so the unit moves to _fin with an outcome record. It records what the removal actually corrected: the ACP adapter was written on the untested assumption that OpenCodex could not hold the installed CLI's credential, then kept on a second assumption that leaving it registered was harmless because routing pinned the registry id away from it. That was true for the registry id and false for the custom-named row the documentation itself recommended, which is the row that would have broken on removal. It also corrects two beliefs this unit started with: devin-cli has not run over ACP since the account-login unit, and its token usage is reported, not missing. What is genuinely absent is account quota, which Cognition exposes only through Enterprise-scoped endpoints.
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. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
documentation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds planning and outcome records for retiring the Devin ACP adapter, migrating saved configurations, adding a tool-catalog prompt nudge, documenting verification requirements, and recording residual compatibility details. ChangesDevin ACP retirement
Estimated code review effort: 1 (Trivial) | ~4 minutes Change: Other ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Maintainer integration decision (dev, self-merge without a second approval). Recorded per
|
리뷰 · 우선순위 30 / 80이 PR은 Devin ACP 제거 유닛을 아웃컴이 말하는 핵심은 단순하다. ACP 어댑터는 “OpenCodex가 설치 CLI 자격증명을 못 든다”는 검증 안 된 가정으로 생겼고, 나중에는 “레지스트리 id만 핀 해 두면 등록을 남겨도 안전하다”는 두 번째 가정으로 남았다. 두 번째는 레지스트리 id 행에는 맞았지만, 문서가 권장하던 커스텀 이름 행에는 틀렸다. 그래서 #4415가 ACP를 지우고 유닛이 처음 믿었던 두 가지도 아웃컴이 바로잡는다. (1) account-login 이후 남은 잔여도 코드와 맞다. 작은 정리 포인트만 있다. 표의 wp1=#4411 / wp2=#4415 SHA·링크는 맞지만, git 부모 순서는 #4415가 먼저이고 #4411이 그 위 HEAD다. 페이즈 번호와 머지 시각이 어긋나 보여도 SHA가 정확하면 막히는 문제는 아니다. “독립 리뷰 4건” 아래에 접힌 항목이 다섯 줄인 것도 세는 방식 차이일 뿐, 내용 자체는 #4415/#4411과 모순되지 않는다. types.ts/config.ts 분할과도 무관하다.
메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
![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: 06c8fb288c
ℹ️ 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".
| `Unknown adapter`. Left as-is deliberately: config validation already rejects an unknown adapter | ||
| id at load, and folding case or whitespace here would hide a typo rather than repair a known |
There was a problem hiding this comment.
For the hand-edited case/whitespace variants discussed here, config loading does not reject the adapter ID: providerConfigSchema accepts any non-empty string at src/config.ts:587, and loadConfig() returns a successfully parsed configuration. The unknown adapter is rejected only later by createRegisteredAdapter() at src/adapters/registry.ts:180-181, so this outcome records a false rationale for leaving the migration exact-match-only. Either add actual load-time validation or state that these variants load and fail when a request constructs the adapter.
Useful? React with 👍 / 👎.
1 participant
Summary
dev— fix(codex): refresh the catalog when paginated history refuses injection #4411 as2d3c05fa9eand refactor(devin): retire the ACP adapter and give the shared adapter the tool-catalog nudge #4415 as213065e30b— so the planning unit moves fromdevlog/_plan/todevlog/_fin/with an outcome record, which is whatAGENTS.mdasks for once a unit's changes are visible in public git history.routedProviderConfigpinned the registry id away from it. That was true for the registry id and false for the custom-named row the documentation itself recommended, which is exactly the row that would have broken on removal.devin-clihas not run over ACP since the account-login unit, and its token usage is reported rather than missing. What is genuinely absent is account quota, which Cognition exposes only through Enterprise-scoped endpoints.projectDevinCliAuthModematches the retired adapter id by exact string, so a hand-edited"devin-cli "or"Devin-CLI"is not rewritten. Folding case or whitespace there would hide a typo instead of repairing a known historical value, and config validation already rejects an unknown adapter id at load.Documentation only. Nothing in the build, typecheck, or test path reads from
devlog/.Verification
bun run privacy:scan— pass.bun run structure:check— pass.bun test(full suite) — NOT RUN locally, per maintainer instruction; hosted CI on this exact head is the acceptance evidence. The focused suites for the code this unit describes were green on the mergeddevtip: 49 pass, 0 fail acrosscodex-sync-api,devin-adapter,devin-cli-authmode-migration,adapter-registry-authority,adapter-tool-conformanceandtest-layout.Checklist
Summary by CodeRabbit