test(oauth): clean up post-IPC migration test debt from #30257#30260
Conversation
![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: a9fbed45e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * What was deleted in this file (and from `oauth/__tests__/*.test.ts`): | ||
| * - ~10 describe.skip suites mocking pre-IPC oauth-store + connect-orchestrator | ||
| * code paths. Their daemon-side equivalents are covered by | ||
| * `oauth-commands-routes.test.ts`, `oauth-store.test.ts`, and | ||
| * `connection-routes-vs-cli-parity.test.ts`. |
There was a problem hiding this comment.
The new file header documents what “was deleted” and cites migration/PR history, which violates the mandatory comment rule in assistant/AGENTS.md (“do not reference code that has been removed”). This creates maintenance debt because these notes become stale and force readers to understand past implementations instead of current behavior; please rewrite this comment to describe only the present test scope without removal history.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Comment references removed code, violating assistant/AGENTS.md code comments rule
The new file-level JSDoc comment narrates history of removed code: "Tests that previously seeded the DB and asserted on table output / JSON shape now belong on the daemon route side — they were skipped pending IPC mock rewrite (#30257)" (lines 20-22). Per assistant/AGENTS.md § "Code comments": "do not reference code that has been removed" and "Avoid phrases like... 'previously used Y', or 'was removed in PR Z'".
(Refers to lines 20-22)
Prompt for agents
The file-level JSDoc comment in usage-cli.test.ts (lines 15-27) narrates history of removed code at lines 20-22 ('Tests that previously seeded the DB and asserted on table output / JSON shape now belong on the daemon route side — they were skipped pending IPC mock rewrite (#30257)'). Per assistant/AGENTS.md code comments rule, remove the history-narrating portions. The comment should simply state that this file tests CLI argument validation for the usage command, and note the follow-up opportunity to add IPC mock tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * After the IPC migration (#30238–#30251) the actual `sendMessage` call runs | ||
| * inside the daemon; the CLI shells out via `cliIpcCall(...)`. Tests that | ||
| * previously asserted on provider invocations (response body, system prompt | ||
| * threading, profile overrides, --json output shape, llm/inference alias | ||
| * equivalence) covered the pre-IPC in-process path and were skipped pending | ||
| * IPC mock rewrite (#30257). |
There was a problem hiding this comment.
🟡 Comment references removed code, violating assistant/AGENTS.md code comments rule
The new file-level JSDoc comment narrates history of removed code ("Tests that previously asserted on provider invocations", "covered the pre-IPC in-process path and were skipped pending IPC mock rewrite (#30257)", "the old in-process provider abstraction"). Per assistant/AGENTS.md § "Code comments": "do not reference code that has been removed. Comments should describe the current state of the codebase, not narrate its history. Avoid phrases like 'no longer does X', 'previously used Y', or 'was removed in PR Z'." The comment should describe what the file tests today without referencing what was removed.
Prompt for agents
The file-level JSDoc comment in inference-send.test.ts (lines 1-19) references removed code and narrates codebase history, violating the code comments rule in assistant/AGENTS.md. Phrases like 'Tests that previously asserted on...', 'covered the pre-IPC in-process path and were skipped pending IPC mock rewrite (#30257)', and 'the old in-process provider abstraction' should be removed. The comment should only describe what the file tests today (CLI surface tests: help rendering, argument validation, no-message guard) and what could be added. Remove lines 4-9 and 17-18 (the history-narrating portions) and keep only lines 1-3, 11-16 which describe the current state.
Was this helpful? React with 👍 or 👎 to provide feedback.
| getNestedValue: () => undefined, | ||
| setNestedValue: () => {}, |
There was a problem hiding this comment.
🟡 Comment references removed code, violating assistant/AGENTS.md code comments rule
The new file-level JSDoc comment references removed code and narrates history: "What was deleted in this file (and from oauth/__tests__/*.test.ts): ~10 describe.skip suites mocking pre-IPC oauth-store + connect-orchestrator code paths" (lines 45-49), and "They were out of scope for the post-#30257 cleanup" (lines 57-58). Per assistant/AGENTS.md § "Code comments": comments should describe the current state, not narrate history or reference removed code.
(Refers to lines 45-58)
Prompt for agents
The file-level JSDoc comment in oauth-cli.test.ts (lines 31-59) has sections that reference removed code and narrate history, violating assistant/AGENTS.md code comments rule. Lines 45-49 ('What was deleted...') explicitly list removed code. Lines 57-58 ('They were out of scope for the post-#30257 cleanup') narrate history. Remove these sections. Keep only the 'What lives here today' section (lines 41-43) and the 'Follow-up worth doing' items (lines 51-55) which describe current state and future work.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * 9 endpoints had no route-level unit tests — the CLI surface tests in | ||
| * `oauth-cli.test.ts` and `oauth/__tests__/*` covered the pre-IPC in-process | ||
| * implementations and were skipped pending rewrite (#30257). These tests are |
There was a problem hiding this comment.
🟡 Comment references removed code, violating assistant/AGENTS.md code comments rule
The new file-level JSDoc comment references removed test implementations: "the CLI surface tests in oauth-cli.test.ts and oauth/__tests__/* covered the pre-IPC in-process implementations and were skipped pending rewrite (#30257)" (lines 7-9). Per assistant/AGENTS.md § "Code comments": comments should describe the current state, not reference removed code or narrate history.
Prompt for agents
The file-level JSDoc comment in oauth-commands-routes.test.ts (lines 1-14) references removed code at lines 7-9 ('the CLI surface tests in oauth-cli.test.ts and oauth/__tests__/* covered the pre-IPC in-process implementations and were skipped pending rewrite (#30257)'). Per assistant/AGENTS.md code comments rule, remove lines 6-9. The comment should describe what THIS file tests (route handler validation, dispatch, payload shapes) and where deeper coverage lives, without referencing removed test files.
Was this helpful? React with 👍 or 👎 to provide feedback.
Follow-up to #30257, which merged with describe.skip on most of the oauth/usage/inference CLI tests to unblock CI after the CLI→IPC migration cascade (#30238–#30251). This PR closes out the debt: ADD daemon-side coverage for the IPC endpoints introduced in #30251: - assistant/src/__tests__/oauth-commands-routes.test.ts (+711 lines, 36 tests across all 9 endpoints: oauth_disconnect, oauth_mode_get, oauth_mode_set, oauth_status, oauth_ping, oauth_token, oauth_request, oauth_managed_connect_start, oauth_managed_connect_poll). These handlers had no route-level unit tests since the migration — that gap predates #30257 but needed to close before the CLI dupes could be deleted. DELETE outright (every top-level describe was describe.skip'd against pre-IPC in-process logic that no longer exists): - 9 × assistant/src/cli/commands/oauth/__tests__/*.test.ts (connect, disconnect, mode, ping, status, token, providers-{register,update,delete}) - assistant/src/__tests__/credentials-cli.test.ts (1232 lines) - assistant/src/cli/commands/__tests__/routes.test.ts (576 lines) TRIM down to passing CLI-plumbing tests only: - oauth-cli.test.ts: 2103 → 135 lines (kept requirePlatformConnection) - usage-cli.test.ts: 153 → 85 lines (kept invalid-arg validation) - inference-send.test.ts: 643 → 236 lines (kept help text + no-message guard + --max-tokens validation) Net: 15 files changed, +797 / -9910. Coverage check: the underlying business logic that the deleted tests nominally covered already runs through oauth-store.test.ts, credential-vault.test.ts, oauth-apps-routes.test.ts, oauth-providers-routes.test.ts, oauth-connect-routes.test.ts, and usage-routes.test.ts. The new oauth-commands-routes.test.ts file closes the only real coverage gap. Follow-ups (not blocking, called out in file-level docstrings): - CLI plumbing tests for exitFromIpcResult exit-code mapping, shouldOutputJson/writeOutput formatting, and the oauth token shell-lockdown guard - IPC-mock-based tests for the trimmed inference-send / usage paths if anyone wants stronger end-to-end coverage 🔥 Generated with [Credence](https://vellum.ai)
credence-the-bot
Bot
force-pushed
the
credence/cli-test-cleanup
branch
from
a9fbed4 to
774b550
Compare
|
Both bots flagged the same issue: docstrings narrated history of removed code, which violates Force-pushed Specific docstring lines flagged are gone:
PR description still carries the migration narrative; git log carries the diff. The files describe themselves now. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Follow-up to #30257, which merged with
describe.skipon most of the oauth/usage/inference CLI tests to unblock CI after the CLI→IPC migration cascade (#30238–#30251).This PR closes out the debt.
Add — daemon route coverage
assistant/src/__tests__/oauth-commands-routes.test.ts(new, 711 lines, 36 tests).Exercises all 9 IPC endpoints introduced in #30251:
oauth_disconnect,oauth_mode_get,oauth_mode_set,oauth_status,oauth_ping,oauth_token,oauth_request,oauth_managed_connect_start,oauth_managed_connect_poll.Coverage per endpoint: route-registry sanity, missing-arg validation, unknown-provider rejection, managed-vs-BYO dispatch, and the most-trafficked happy / error paths. This handler-level gap predates #30257 but needed to close before the CLI dupes could be deleted.
Delete — pre-IPC dead tests
Every top-level describe in these files was
describe.skip'd against in-process code paths that no longer exist after the IPC migration:assistant/src/cli/commands/oauth/__tests__/*.test.ts(connect,disconnect,mode,ping,status,token,providers-register,providers-update,providers-delete)assistant/src/__tests__/credentials-cli.test.ts(1232 lines)assistant/src/cli/commands/__tests__/routes.test.ts(576 lines)Trim — keep only living CLI plumbing
oauth-cli.test.tsrequirePlatformConnectionhelper (3 tests)usage-cli.test.ts--group-byvalidationinference-send.test.ts--max-tokensvalidationEach trimmed file gained a top-of-file docstring explaining what was removed and why, plus follow-up bullets for IPC-mocked CLI plumbing tests if anyone wants to recover deeper coverage at the CLI layer.
Coverage check
The business logic that the deleted CLI tests nominally covered already runs through:
oauth-store.test.ts(provider/connection persistence)credential-vault.test.ts(token-manager /withValidToken)oauth-apps-routes.test.ts,oauth-providers-routes.test.ts,oauth-connect-routes.test.ts(the other 3 oauth route surfaces)usage-routes.test.ts(usage aggregation)oauth-commands-routes.test.ts(the 9 command endpoints)No coverage regression in moved logic. The only new coverage is the daemon-side route layer this PR adds.
Diff stat
Follow-ups (not blocking)
Called out in each trimmed file's docstring:
exitFromIpcResultexit-code mapping,shouldOutputJson/writeOutputoutput formatting, and theoauth tokenshell-lockdown guard (VELLUM_UNTRUSTED_SHELL=1)🔥 Generated with Credence