-
Notifications
You must be signed in to change notification settings - Fork 21
fix(init): normalize custom base URL #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a1f5ca3
d3ae4d9
66b2c2b
7d0a862
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,10 @@ import { installPrepareCommitMsgHook } from '../git/hook.js'; | |
|
|
||
| const CUSTOM_KEY = '__custom__'; | ||
|
|
||
| export function normalizeBaseUrl(value: string): string { | ||
| return value.replace(/\/+$/, ''); | ||
| } | ||
|
|
||
| export function buildApiKeyPrompt(existingKey: string, apiKeyEnv: string) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM RISK This function is significantly over complexity and length limits (CCN 47), which makes the initialization flow difficult to maintain and test. Consider breaking down the various prompt configurations into a map or separate factory functions based on the provider. |
||
| return { | ||
| message: `Enter your API key (will be stored in config), or leave blank to use ${pc.cyan(`$${apiKeyEnv}`)} env var:`, | ||
|
|
@@ -78,7 +82,7 @@ export async function initCommand(options: { installHook?: boolean } = {}): Prom | |
| outro('Setup cancelled.'); | ||
| return; | ||
| } | ||
| baseUrl = urlResult; | ||
| baseUrl = normalizeBaseUrl(urlResult); | ||
| apiKeyEnv = 'CUSTOM_API_KEY'; | ||
| needsApiKey = true; | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import test from 'node:test'; | ||
|
|
||
| import { initCommand, normalizeBaseUrl } from '../dist/commands/init.js'; | ||
|
|
||
| test('handles single trailing slash', () => { | ||
| assert.equal(normalizeBaseUrl('https://api.example.com/v1/'), 'https://api.example.com/v1'); | ||
| }); | ||
|
|
||
| test('trims multiple trailing slashes from custom base URLs', () => { | ||
| assert.equal(normalizeBaseUrl('https://api.example.com/v1///'), 'https://api.example.com/v1'); | ||
| }); | ||
|
|
||
| test('preserves custom base URLs without trailing slashes', () => { | ||
| assert.equal(normalizeBaseUrl('https://api.example.com/v1'), 'https://api.example.com/v1'); | ||
| }); | ||
|
|
||
| test('initCommand saves normalized custom provider base URL', async () => { | ||
| const savedConfigs = []; | ||
| const selectedValues = ['__custom__', 'custom-model']; | ||
| const textValues = ['https://api.example.com/v1/', 'test-api-key', '50']; | ||
| const spinnerStub = { | ||
| start() {}, | ||
| stop() {}, | ||
| }; | ||
|
|
||
| await initCommand( | ||
| {}, | ||
| { | ||
| intro() {}, | ||
| outro() {}, | ||
| select: async () => selectedValues.shift(), | ||
| text: async () => textValues.shift(), | ||
| confirm: async () => false, | ||
| spinner: () => spinnerStub, | ||
| isCancel: () => false, | ||
| configExists: () => false, | ||
| fetchModels: async (provider, baseUrl) => { | ||
| assert.equal(provider, '__custom__'); | ||
| assert.equal(baseUrl, 'https://api.example.com/v1'); | ||
| return ['custom-model']; | ||
| }, | ||
| saveConfig: async (config) => { | ||
| savedConfigs.push(config); | ||
| }, | ||
| testConnection: async (config) => { | ||
| assert.equal(config.baseUrl, 'https://api.example.com/v1'); | ||
| return config.model; | ||
| }, | ||
| }, | ||
|
Comment on lines
+27
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "== initCommand signature =="
ast-grep outline src/commands/init.ts --view expanded | sed -n '/initCommand/,+6p'
echo
sed -n '22,90p' src/commands/init.ts
echo
echo "== test call site =="
sed -n '18,56p' tests/init-base-url.test.mjsRepository: 404-PF/commit-echo Length of output: 3366 🏁 Script executed: #!/bin/bash
set -e
echo "== init.ts imports and helper usage =="
ast-grep outline src/commands/init.ts --view expanded | sed -n '1,220p'
echo
echo "== init.ts relevant sections =="
sed -n '1,260p' src/commands/init.ts
echo
echo "== tests that mock command dependencies =="
rg -n "initCommand\\(|fetchModels: async|saveConfig: async|testConnection: async|configExists:" tests src -g '!**/node_modules/**'Repository: 404-PF/commit-echo Length of output: 8995 🏁 Script executed: #!/bin/bash
set -e
echo "== init-base-url.test.mjs =="
sed -n '1,220p' tests/init-base-url.test.mjs
echo
echo "== any module-mocking helpers used in tests =="
rg -n "mock\\(|unstable_mockModule|proxyquire|sinon|vi\\.mock|jest\\.mock" tests src -g '!**/node_modules/**'Repository: 404-PF/commit-echo Length of output: 2070 Use a real mocking seam for 🤖 Prompt for AI Agents |
||
| ); | ||
|
Comment on lines
+27
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 HIGH RISK The |
||
|
|
||
| assert.equal(savedConfigs.length, 1); | ||
| assert.equal(savedConfigs[0].provider, '__custom__'); | ||
| assert.equal(savedConfigs[0].baseUrl, 'https://api.example.com/v1'); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The current implementation fails to remove trailing slashes if the string ends with whitespace. Trimming the input first ensures robust normalization.