Repository navigation
test(proxy): drive the Copilot client cases through the built CLI - #1909
Merged
Merged
Conversation
- T3885676104-a (PR 1593): the Copilot post-apply note, the model-id default and the env-script content are now asserted on what `node dist/cli/index.js proxy start` prints and writes in a seeded throwaway HOME, through a `seed` hook on startProxyForTests. A dist whose CLI stops doing this now fails them; the src-importing versions could not see it. The suite header no longer says a throwaway HOME needs a launchd unit (startProxyForTests sets NEUROLINK_PROXY_IGNORE_LAUNCHD=1). Not done: restore-on-shutdown through the CLI (a proxy restores only on SIGINT or through the fail-open guard, which is racy to assert); the sourced-profile detection matrix stays in the existing determinism-exception case, since one spawned proxy per profile text is the cost of moving it; no dist-freshness guard was added to this suite. Verification: test:proxy (184 passed, 7 skipped, same 7 live-API skips as before); mutation proof on a deliberately broken dist (the old src-importing cases pass, the three CLI cases fail; a good rebuild passes all); build, check, lint, check:tools-tests, check:deps, test:provider-structure, test:model-manifests.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
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 |
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
|
🎉 This PR is included in version 12.47.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
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
The three Copilot client cases in
test/continuous-test-suite-proxy.tsimportedsrc/cli/proxy-clients/{registry,copilot}.jsdirectly, so a brokendist/cli/index.jscould pass them. The post-apply note, the model-id default and the env-script content now come from whatnode dist/cli/index.js proxy startprints and writes in a throwaway HOME that holds a.copilotdirectory. Adistwhose CLI stops doing any of those fails the cases.Only the test suite changes: one file, no
src/change.It sits on top of the change that rewrote Copilot's profile detection (#1905, merged) and leaves the profile-line table case that change added untouched.
What changed in the suite
startProxyForTestsnow also pointsGROK_HOMEat the throwaway HOME.proxy startconfigures every client it finds, the migrated cases now run that whole roster, and an inheritedGROK_HOMEwould otherwise send the spawned proxy at a real Grok config directory.startProxyForTeststakes an optionalseed(home)callback that runs on the throwaway HOME before the child starts, andProxyTestInstancegainsoutput(), the tail of what the child printed. Existing callers are unaffected.startCopilotProxy(profile?)starts a proxy whose HOME holds.copilot(and a.zshrcwhen a profile is given) and waits forRestart Copilot CLI, which is printed right after the post-apply note./healthanswers before the clients are configured, so waiting on health alone would race.Proxy clients: Copilot reports when its script is not sourced(name kept) spawns twice: with no profile the output must report the configured client and ask for the profile line; with the documented profile line it must report the configured client and not ask.Proxy clients: Copilot env script sets a model id(name kept) reads the script the built CLI wrote and checks the model id and its:-override form.Proxy clients: the built CLI writes a sourceable Copilot env scriptis new. It checks the three exports and the realhttp://127.0.0.1:<port>/v1of the spawned proxy. It replacesCopilot emits a sourceable env script.Proxy clients: Copilot detects an absent CLI and restore removes the env scriptkeeps the parts that stay direct calls:detect()with and without~/.copilot,apply()writing the script, andrestore()removing it.proxy-infrabecause they spawn a proxy; the direct-call case stays underproxy-config.startProxyForTestssetsNEUROLINK_PROXY_IGNORE_LAUNCHD=1), and the Copilot positive paths are listed as driven through the built CLI.Every red assertion of the three old cases, and where it lives now, is mapped in the table below.
Auto-configured Copilot CLI settingsis in the outputCOPILOT_PROVIDER_TYPE,_BASE_URL,_API_KEY, the/v1URL)The one behaviour that no longer exists is the closing
restoreAllClientscall of case 1: the throwaway HOME is deleted bystopProxyForTests, so nothing outside it is touched.Tests
pnpm run test:proxyon the base before any edit: 183 passed, 7 skipped, 0 failed. After: 184 passed, 7 skipped (the same seven live-API cases), 0 failed. The extra pass is the old third case split in two.There is no source fix to revert, so red proof is by mutation. A deliberately broken
dist/cliwas built from a mutatedsrc/cli/proxy-clients/copilot.ts, thensrc/was put back to the verified original (hash checked) without rebuilding, and each suite copy was run with only the Copilot cases selected:src/)COPILOT_PROVIDER_TYPErenamedThe first row is the gap the thread described: the old cases stay green against a broken CLI. Each failure was read to confirm it is the intended assertion, and each prints a cross mark, not a skip. A one-assertion break-on-purpose in a scratch copy also prints a cross mark and exits non-zero.
Cost: case 1 (two spawns) 8 s wall including tsx start-up, case 2 4 s, case 3 3 s; all five Copilot cases run together in 6 s. The per-case bound is 180 s.
The suite was run again after the
GROK_HOMEline was added: 184 passed, 7 skipped.Also run on this commit:
pnpm run build,check,lint(0 errors; the warnings are in other files),check:tools-tests,check:deps,test:provider-structure,test:model-manifests.Notes for review
proxy-infracase does, when the suite detects a launchd-managed proxy on the machine. They spawn withNEUROLINK_PROXY_IGNORE_LAUNCHD=1and do not need that guard, so this is an extra skip path on such a machine, not a coverage gain; it matches the neighbouringCLI:cases.diston disk, sopnpm run build:clihas to be current beforetest:proxy; a stale build fails them in a way that points atsrc.assertDistFresh). Adistolder thansrc/still runs here; the new cases fail only when the stale build behaves differently. Adding the guard is a separate decision, so it is not in this change.[ -f ~/.neurolink/copilot-env.sh ] && . ~/.neurolink/copilot-env.shas a source command. It does, and the direct-call profile table already pins the same line.Not done
Could not verify
pnpm run test:providers-mockedwas not run: this change touches only the proxy suite, which that job does not run.Source thread: #1593 (comment)