Fix CLI port mismatch and centralize local dev URLs#1988
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 8c79615 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
🟠 1) agents-cli/src/__tests__/commands/profile.test.ts Missing URL validation tests for Custom path
Issue: The new profileAddCommand tests cover the happy paths for Local, Cloud, and Custom, but don't test the URL validation logic at lines 98-105 and 117-124 in profile.ts. These validators reject empty/whitespace URLs with "URL is required" and invalid URL formats with "Invalid URL format".
Why: Without testing URL validation, a regression could allow users to create profiles with invalid or empty URLs. These profiles would fail at runtime when the CLI attempts to use them for API calls or authentication flows. The validation logic has two distinct error conditions that should both be verified.
Fix: Add validation tests to the "Custom remote type" describe block:
it('should validate API URL format', async () => {
vi.mocked(p.select).mockResolvedValueOnce('custom');
let validateFn: ((value: string) => string | undefined) | undefined;
vi.mocked(p.text).mockImplementation(async (options: any) => {
if (options.message === 'Agents API URL:') {
validateFn = options.validate;
return 'https://api.example.com';
}
if (options.message === 'Manage UI URL:') return 'https://manage.example.com';
if (options.message === 'Environment name:') return 'production';
if (options.message === 'Credential reference:') return 'inkeep-test';
return '';
});
vi.mocked(p.confirm).mockResolvedValueOnce(false);
await profileAddCommand('test');
expect(validateFn?.('')).toBe('URL is required');
expect(validateFn?.(' ')).toBe('URL is required');
expect(validateFn?.('not-a-url')).toBe('Invalid URL format');
expect(validateFn?.('https://valid.com')).toBe(undefined);
});Refs:
🟡 Minor (2) 🟡
🟡 1) agents-cli/src/__tests__/commands/profile.test.ts Missing tests for edge cases in profile creation
Issue: The tests don't cover: (a) profile name prompt validation when name argument is omitted (lines 45-64), (b) duplicate profile name rejection (lines 67-71), or (c) the "switch to profile" confirmation flow (lines 203-212).
Why: These are defensive code paths that prevent invalid profile names, accidental overwrites, and ensure the switch-profile UX works correctly. Without tests, regressions in these areas would go undetected.
Fix: Add tests for:
- Profile name validation (
/^[a-z0-9-]+$/regex) - Duplicate profile rejection (mock
getProfileto return existing profile) - Profile switch when user confirms (mock
p.confirmto returntrue)
Refs:
Inline Comments:
- 🟡 Minor:
profile.ts:78Local hint could clarify no-auth behavior - 🟡 Minor:
setup-profile.mdx:33-37Step numbering incomplete (missing credential prompt) - 🟡 Minor:
cli-reference.mdx:59-60--local description omits Manage UI URL default
💭 Consider (2) 💭
💭 1) agents-docs/content/troubleshooting.mdx Add cross-link to CLI reference
Issue: The new CLI Issues section is well-structured but doesn't link back to the profile management commands in the CLI reference.
Why: Users landing on troubleshooting via search may not know where to learn more about inkeep profile add and inkeep profile current.
Fix: Add a brief cross-reference at the end: "For detailed profile management, see the CLI Reference profile commands."
💭 2) agents-cli/src/__tests__/commands/profile.test.ts:167 Verify warning message is logged
Issue: Test "should check keychain for credential and warn if missing" verifies checkCredentialExists is called but doesn't assert the warning message is actually logged.
Fix: Add expect(console.log).toHaveBeenCalledWith(expect.anything(), expect.stringContaining('not found in keychain')) to verify user-facing warning.
💡 APPROVE WITH SUGGESTIONS
Summary: This PR correctly fixes the port mismatch bug and centralizes localhost URLs via LOCAL_REMOTE — a solid fix for a real onboarding blocker. The Cloud/Local/Custom split in profile add is a sensible UX improvement. The main suggestions are around test coverage for the new validation logic and minor documentation completeness. The core bug fix and refactoring are clean and well-executed. 🎉
Discarded (9)
| Location | Issue | Reason Discarded |
|---|---|---|
vercel.mdx:188-203 |
Uses stale agentsManageApi/agentsRunApi field names |
Outside PR scope — pre-existing issue not introduced by this PR |
profile.ts:92-93 |
Behavioral difference between init --local and profile add -> Local | Intentional design per PR description — init allows URL customization for first-time setup |
profile.ts:95-107 |
Custom placeholder uses example.com domain | Explicitly designed this way per PR description to prevent accidentally accepting wrong defaults |
| 7 consistency findings | Positive confirmations that patterns are consistent | Not issues — confirmations that the PR maintains consistency |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
6 | 2 | 1 | 0 | 0 | 0 | 3 |
pr-review-product |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-docs |
3 | 0 | 1 | 0 | 2 | 0 | 0 |
pr-review-consistency |
8 | 0 | 0 | 0 | 0 | 0 | 8 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 21 | 2 | 2 | 0 | 3 | 0 | 14 |
Note: pr-review-consistency returned 7 positive confirmations + 1 out-of-scope finding; pr-review-standards found no issues (clean code quality).
agents-cli/src/commands/profile.ts
Outdated
| options: [ | ||
| { value: 'cloud', label: 'Inkeep Cloud', hint: 'Default cloud deployment' }, | ||
| { value: 'custom', label: 'Custom', hint: 'Local or self-hosted deployment' }, | ||
| { value: 'local', label: 'Local', hint: 'Local development (localhost)' }, |
There was a problem hiding this comment.
🟡 Minor: Local hint could clarify no-auth behavior
Issue: The hint says "Local development (localhost)" but doesn't convey that this path also auto-sets credential: 'none' and skips credential prompts entirely.
Why: Users who want local dev with authentication (e.g., testing auth flows) might select Local expecting to configure credentials, only to find they can't.
Fix: (1-click apply)
| { value: 'local', label: 'Local', hint: 'Local development (localhost)' }, | |
| { value: 'local', label: 'Local', hint: 'Local development (localhost, no auth)' }, |
Refs:
| Use `--local` for local development: | ||
| - Prompts for tenant ID and API URL (defaults to `localhost:3002`) |
There was a problem hiding this comment.
🟡 Minor: --local description omits Manage UI URL default
Issue: The description mentions the API URL default (localhost:3002) but omits the Manage UI URL (localhost:3000). The source code shows both URLs are set when creating local profiles.
Why: Users may not realize the local profile also sets a Manage UI URL, which matters for understanding where device-code authentication redirects.
Fix: (1-click apply)
| Use `--local` for local development: | |
| - Prompts for tenant ID and API URL (defaults to `localhost:3002`) | |
| Use `--local` for local development: | |
| - Prompts for tenant ID and API URL (defaults to `localhost:3002`); Manage UI defaults to `localhost:3000` |
Refs:
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
Re-review scoped to delta: 1 commit, 3 files, +3/-2 lines since the last automated review. The delta appears to directly address all 3 prior inline comments. No new reviewers dispatched — changes are minor text/documentation updates.
✅ Prior Inline Comments Addressed
All 3 inline comments from the prior review have been addressed by commit c2a8604b:
| Prior Comment | Status | Change |
|---|---|---|
profile.ts:78 — Local hint should clarify no-auth |
✅ Addressed | Hint now says "Local development (localhost, no auth)" |
setup-profile.mdx:33-37 — Step numbering incomplete |
✅ Addressed | Added Step 3 for credential reference (Cloud and Custom only) |
cli-reference.mdx:59-60 — --local omits Manage UI URL |
✅ Addressed | Now mentions "Manage UI defaults to localhost:3000" |
🕐 Pending Recommendations (2)
These items were raised in the prior review and remain unaddressed in the delta:
- 🟠
profile.test.tsMissing URL validation tests for Custom path — the URL validators at profile.ts:98-105 and :117-124 reject empty/invalid URLs but have no test coverage - 🟡
profile.test.tsMissing tests for edge cases — profile name validation (lines 45-64), duplicate profile rejection (lines 67-71), and switch-profile confirmation flow (lines 203-212) lack coverage
💡 APPROVE WITH SUGGESTIONS
Summary: The delta successfully addresses all prior inline feedback — the Local hint now clarifies no-auth behavior, the docs include the credential prompt step, and the --local description mentions Manage UI defaults. The core bug fix and refactoring from the original PR are solid. The remaining suggestions around URL validation test coverage are nice-to-haves for defensive testing but don't block merge. Ship it! 🚀
Discarded (0)
No items discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: No reviewers dispatched for this delta — changes were minor text/documentation updates that directly addressed prior inline feedback. Pending recommendations carried forward from the prior review.
- init.ts: replace 4 hardcoded localhost URLs with LOCAL_REMOTE imports (fixes manageUi using wrong port 3001 instead of 3000) - profile.ts: split 'profile add' into Cloud/Local/Custom paths with audience-appropriate defaults; add credential !== 'none' guard - config.ts: use LOCAL_REMOTE.api instead of hardcoded fallback URL - profile-config.ts: import LOCAL_REMOTE for fallback defaults - profiles/types.ts: remove dead DEFAULT_LOCAL_PROFILE constant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- init.test.ts: use LOCAL_REMOTE constants consistently for all mock return values and assertions (api + manageUi) - profile.test.ts: add 9 tests covering Cloud, Local, and Custom paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- cli-reference.mdx: fix profile YAML example, add login/logout sections, update push options/env vars - workspace-configuration.mdx: fix CLI flags, env vars, code examples - setup-profile.mdx: describe Cloud/Local/Custom profile options - troubleshooting.mdx: add CLI issues section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ageUi - profile.ts: add "no auth" to Local option hint - setup-profile.mdx: add credential reference as step 3 - cli-reference.mdx: mention Manage UI default in --local description Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
eb84ec1 to
8c79615
Compare
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scoped to delta: 1 commit, 1 file (+5 lines) since the last automated review. The only change is adding a changeset file.
✅ Delta Review
The delta since the last automated review contains only:
| Commit | Change |
|---|---|
8c79615d |
Added changeset file .changeset/quixotic-tomato-grouse.md |
Changeset content:
---
"@inkeep/agents-cli": patch
---
Fix CLI port mismatch by centralizing local dev URLs via --local flagThis is a valid changeset that correctly:
- Targets
@inkeep/agents-cli(the package being modified) - Uses
patchversion bump (appropriate for a bug fix) - Provides a clear, user-facing description of the change
No issues found in the delta.
🕐 Pending Recommendations (2)
These items were raised in the prior review and remain unaddressed:
- 🟠
profile.test.tsMissing URL validation tests for Custom path — the URL validators at profile.ts:98-105 and :117-124 reject empty/invalid URLs but have no test coverage - 🟡
profile.test.tsMissing tests for edge cases — profile name validation (lines 45-64), duplicate profile rejection (lines 67-71), and switch-profile confirmation flow (lines 203-212) lack coverage
✅ APPROVE
Summary: The delta adds only a changeset file, which is correctly formatted. The core bug fix and refactoring from the original PR were already reviewed and approved. Human approval from @amikofalvy confirms the PR is ready. The pending test coverage suggestions are nice-to-haves but don't block merge. Ship it! 🚢
Discarded (0)
No items discarded in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: No reviewers dispatched for this delta — the only change was a changeset file addition. Pending recommendations carried forward from the prior review.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
Problem
inkeep init --localcreates a CLI profile withmanageUi: 'http://localhost:3001', but the management dashboard runs on port 3000. This causes the device-code authentication flow to fail for every new developer running local setup — the CLI directs the browser tohttp://localhost:3001/device?user_code=XXXwhich doesn't exist, resulting in ERR_CONNECTION_REFUSED and eventually "Device code expired."How it happened
112b5c765(PR #1395)--localflag. CreatedLOCAL_REMOTEconstant intypes.tsAND hardcodedmanageUi: 'http://localhost:3001'inline ininit.ts. Both used port 3001. No mismatch yet.b63b786c7(PR #1458)init.tssurvived untouched.96de89839(PR #1752)LOCAL_REMOTE.manageUifrom 3001 → 3000. Did not touchinit.ts. The inline hardcoded'http://localhost:3001'was missed.Root cause:
init.tshardcodes the URL inline instead of importing theLOCAL_REMOTEconstant, so updating the constant had no effect on the actual profile creation.Beyond the immediate bug, the same localhost URLs are hardcoded as string literals in 6+ files across the CLI package.
LOCAL_REMOTEalready exists intypes.tsbut was barely used, creating a class of bug where one file gets updated and others don't.Changes
1. Fix the bug — use
LOCAL_REMOTEininit.tsImport
LOCAL_REMOTEfrom the profiles barrel and replace all 4 hardcoded localhost URLs:manageUi: 'http://localhost:3001'→LOCAL_REMOTE.manageUi(the bug)2. Centralize runtime fallbacks
profile-config.tsandconfig.tsboth had hardcoded localhost URL fallbacks. Replaced withLOCAL_REMOTEreferences so they stay in sync with the constant.3. Split
profile addinto Cloud/Local/CustomThe old "Custom" option served both local dev AND remote/staging users with conflicting defaults (pre-filled localhost URLs confused self-hosted users). Split into three options with audience-appropriate defaults:
LOCAL_REMOTE, credential auto-set to'none', no URL/credential prompts)Key decisions:
'none'— local dev servers don't require auth, andinit --localalready does this.https://your-agents-api.example.com) with no pre-filled default — prevents accidentally accepting wrong defaults. Validation rejects empty input.credential === 'none'—'none'is a sentinel meaning "no auth needed", warning about it missing from keychain is noise.4. Remove dead
DEFAULT_LOCAL_PROFILEconstantExported from
types.tsbut never imported by any file since creation in PR #1395. Removed to avoid misleading future developers.5. Tests
manageUi: 'http://localhost:3001'to'http://localhost:3000'LOCAL_REMOTEconstants consistently in all mock return values and assertionsprofileAddCommandcovering all three paths (Local, Cloud, Custom)6. Documentation
cli-reference.mdxmanageApi→api, removerunApi, credentialnone). Add login/logout command sections. Update profile add description. Fix init --local description. Fix push options/env vars/CI table (--agents-manage-api-url→--agents-api-url, env vars updated).workspace-configuration.mdxagentsApi.url,INKEEP_AGENTS_API_URL,--agents-api-url).setup-profile.mdxtroubleshooting.mdxNot changing
utils/url.tsline 34 — Dead code (buildAgentViewUrlhas zero callers). Removal tracked in PR Remove dead buildAgentViewUrl utility #1984.config.tslines 32, 37 — JSDoc@defaultannotations consumed by fumadocs-typescript for the docs site. Values are already correct (localhost:3002,localhost:3000). Can't reference constants in JSDoc tags (they're parsed as plain text).Test plan
Automated
pnpm test— 632 tests pass (5 skipped), 41 test files, 0 failuresManual end-to-end (built from source)
inkeep init --local --no-interactive→manageUi: 'http://localhost:3000'in profile,agentsApi.url: 'http://localhost:3002'in configinkeep profile add→ Local → no URL/credential prompts, env defaults todevelopment, profile stored withmanageUi: http://localhost:3000inkeep profile add→ Custom → placeholder hints visible, empty validation rejects with "URL is required", credential keychain warning shownlogin.tsconstructs URL fromprofile.remote.manageUi→http://localhost:3000/device?user_code=...for local profiles🤖 Generated with Claude Code