feat: add deprecated btca config model compatibility alias#195
Open
bmdavis419 wants to merge 1 commit intocodex/pr-e-auth-robustness-docsfrom
Open
feat: add deprecated btca config model compatibility alias#195bmdavis419 wants to merge 1 commit intocodex/pr-e-auth-robustness-docsfrom
bmdavis419 wants to merge 1 commit intocodex/pr-e-auth-robustness-docsfrom
Conversation
Comment on lines
+22
to
+23
| const updated = await updateModel(server.url, options.provider, options.model); | ||
| server.stop(); |
Contributor
There was a problem hiding this comment.
Server not stopped on updateModel failure
If updateModel throws, server.stop() on line 23 is never reached because the exception propagates out of the tryPromise callback immediately. This leaves an auto-started server running and can cause port conflicts for subsequent commands.
Consider wrapping the server.stop() call in a try/finally block:
Suggested change
| const updated = await updateModel(server.url, options.provider, options.model); | |
| server.stop(); | |
| try { | |
| const updated = await updateModel(server.url, options.provider, options.model); | |
| console.warn( | |
| 'Deprecation: "btca config model" will be removed in a future release. Use "btca connect --provider ... --model ...".' | |
| ); | |
| console.log(`Model updated: ${updated.provider}/${updated.model}`); | |
| } finally { | |
| server.stop(); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/commands/config.ts
Line: 22-23
Comment:
**Server not stopped on `updateModel` failure**
If `updateModel` throws, `server.stop()` on line 23 is never reached because the exception propagates out of the `tryPromise` callback immediately. This leaves an auto-started server running and can cause port conflicts for subsequent commands.
Consider wrapping the `server.stop()` call in a `try/finally` block:
```suggestion
try {
const updated = await updateModel(server.url, options.provider, options.model);
console.warn(
'Deprecation: "btca config model" will be removed in a future release. Use "btca connect --provider ... --model ...".'
);
console.log(`Model updated: ${updated.provider}/${updated.model}`);
} finally {
server.stop();
}
```
How can I resolve this? If you propose a fix, please make it concise.
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
Issues
configcommand accidentally removed in refactoring? #151Greptile Summary
This PR adds a
btca config modelbackward-compatible alias command that delegates to the sameupdateModelpath asbtca connect, prints a deprecation notice on success, and documents the removal path. The registration inindex.tsis clean and the--helptest covers the new nested subcommand correctly. No plan files were found in the repository.Key changes:
apps/cli/src/commands/config.tsexposes aconfig modelsubcommand as a deprecated shim overupdateModel.apps/cli/src/index.tsregistersconfigCommandin the configuration commands block.apps/cli/src/index.test.tsadds a--helpsmoke test for the nested subcommand.apps/docs/guides/cli-reference.mdxdocuments the compatibility alias and corrects the--branchoption description ("default main"→"auto-detected when omitted").Issues found:
config.ts,server.stop()is only reached ifupdateModelsucceeds. IfupdateModelthrows, the auto-started server is never stopped, which can cause port conflicts for subsequent CLI invocations.Confidence Score: 3/5
config.ts— whereserver.stop()is skipped whenupdateModelthrows — is a real functional bug that can leave a dangling process behind.server.stop()call needs to be moved into afinallyblock.Important Files Changed
btca config modelcompatibility alias. Contains a server resource leak —server.stop()is not called ifupdateModelthrows.configCommand.configis correctly included inknownCommandsbefore the unknown-command check, so typo-suggestion logic works properly for the new command.btca config model --helpexits with code 0 and outputs the expected usage line. No issues found.btca config modelsection and updates the--branchdescription from "defaultmain" to "auto-detected when omitted". No issues found.Last reviewed commit: 66b4cd8