Fix CLI port mismatch and centralize local dev URLs#1987
Fix CLI port mismatch and centralize local dev URLs#1987nick-inkeep wants to merge 3 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
🟠⚠️ Major (3) 🟠⚠️
Inline Comments:
- 🟠 Major:
slack-api.ts:328Unrelated change (Slack CSV export "Last Used" column) appears to be accidental scope creep — may be reverting an intentional fix from 90 minutes prior - 🟠 Major:
profile.test.ts:38-40Missing test coverage for user cancellation flows (6p.isCancel()checks untested) - 🟠 Major:
profile.test.ts:22-24Missing test for "profile already exists" error path (data loss prevention check untested)
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
config.ts:87-92Config command generates legacy flatapiUrlformat, inconsistent withinit.tsnestedagentsApiformat
💭 Consider (2) 💭
Inline Comments:
- 💭 Consider:
setup-profile.mdx:33-37Missing guidance on Environment name prompt in docs - 💭 Consider:
cli-reference.mdx:786-804Example showslogin --profile localbut Local profiles don't require authentication
🧹 While You're Here (1) 🧹
🧹 1) vercel.mdx Outdated config format in deployment docs
Issue: The Vercel deployment documentation at agents-docs/content/deployment/vercel.mdx (lines 192-199) still uses the old split API config format (agentsManageApi, agentsRunApi) while this PR updates cli-reference.mdx and workspace-configuration.mdx to use the unified agentsApi format.
Why: Customers referencing different docs pages will encounter conflicting config examples.
Fix: Update vercel.mdx (and potentially other deployment guides: aws-ec2.mdx, gcp-compute-engine.mdx, hetzner.mdx) to use the unified agentsApi format.
Refs: vercel.mdx config example
💡 APPROVE WITH SUGGESTIONS
Summary: The core bug fix is solid — centralizing localhost URLs via LOCAL_REMOTE and splitting profile add into Cloud/Local/Custom paths are well-designed changes. However, please consider addressing:
- Remove the unrelated
slack-api.tschange — this appears to accidentally revert a prior intentional fix and doesn't belong in a CLI bugfix PR - Add test coverage for the two missing critical paths — cancellation handling and duplicate profile name validation prevent user confusion and data loss
The minor config format inconsistency and docs gaps are lower priority but worth addressing if you're doing another pass.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
profile.test.ts |
Test uses object spread differently than implementation | Stylistic preference, functionally equivalent |
cli-reference.mdx |
Env vars section doesn't mention INKEEP_API_KEY | Not introduced by this PR, separate concern |
profile.test.ts |
Missing test for switch profile confirmation | Lower priority, not a data integrity issue |
profile.test.ts |
Missing test for interactive name prompting | Covered by lower-level ProfileManager tests |
profile.test.ts |
Test doesn't verify warning message display | Low confidence, nice-to-have completeness |
troubleshooting.mdx |
Doesn't mention API port for push troubleshooting | Low confidence, minor completeness |
| Other deployment docs | Outdated env var references | Outside PR scope, pre-existing |
| Product positive feedback | Cloud/Local/Custom split, credential skipping, placeholder hints, env defaults | Positive observations, not issues |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
5 | 0 | 0 | 1 | 2 | 0 | 2 |
pr-review-tests |
6 | 0 | 0 | 0 | 2 | 0 | 4 |
pr-review-docs |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-product |
6 | 0 | 2 | 0 | 0 | 0 | 4 |
| Total | 18 | 0 | 2 | 1 | 4 | 0 | 11 |
| } | ||
|
|
||
| const headers = ['Slack Username', 'Slack Email', 'Slack User ID', 'Linked At']; | ||
| const headers = ['Slack Username', 'Slack Email', 'Slack User ID', 'Linked At', 'Last Used']; |
There was a problem hiding this comment.
🟠 MAJOR: Unrelated change — appears to be accidental scope creep
Issue: This file adds a "Last Used" column to the Slack CSV export, which is unrelated to the CLI port mismatch fix. Based on commit history, this column was intentionally removed 90 minutes prior in commit b9512cf2 because "The Last Used column was always empty."
Why: Including this revert in a CLI bugfix PR makes review, rollback, and bisection harder. If the "Last Used" fix needs to be rolled back independently, it can't be without also reverting the CLI port fix.
Fix: Consider reverting these lines and addressing the Slack CSV change in a separate PR if the lastUsedAt data is now being populated correctly.
Refs:
- PR description notes this file under "Not changing" for
utils/url.tsbut doesn't mentionslack-api.ts
| describe('Local remote type', () => { | ||
| it('should create profile with LOCAL_REMOTE URLs without URL prompts', async () => { | ||
| vi.mocked(p.select).mockResolvedValueOnce('local'); |
There was a problem hiding this comment.
🟠 MAJOR: Missing test coverage for user cancellation flows
Issue: The production code in profile.ts has 6 separate p.isCancel() checks (lines 58-61, 83-86, 109-112, 128-131, 151-154, 173-176) that exit when users cancel input prompts. None of these cancellation paths are tested.
Why: If a regression causes p.isCancel() to be mishandled (e.g., continuing execution after cancel), users could experience partial profile creation or confusing errors instead of graceful exit.
Fix: Add at least one cancellation test per prompt type. Example:
it('should exit gracefully when user cancels remote type selection', async () => {
vi.mocked(p.select).mockResolvedValueOnce(Symbol('cancel'));
vi.mocked(p.isCancel).mockReturnValueOnce(true);
await expect(profileAddCommand('test')).rejects.toThrow('process.exit called');
expect(p.cancel).toHaveBeenCalledWith('Profile creation cancelled');
expect(mockProfileManager.addProfile).not.toHaveBeenCalled();
});Refs:
| import { LOCAL_REMOTE } from '../../utils/profiles'; | ||
|
|
||
| describe('profileAddCommand', () => { |
There was a problem hiding this comment.
🟠 MAJOR: Missing test for "profile already exists" error path
Issue: Production code at profile.ts lines 67-71 checks if a profile name is already taken and exits with error code 1. This critical validation has no test coverage.
Why: A regression that removes or breaks this check could allow users to accidentally overwrite existing profiles, losing their configuration.
Fix: Add test:
describe('profile name validation', () => {
it('should reject profile creation when name already exists', async () => {
mockProfileManager.getProfile.mockReturnValueOnce({
remote: 'cloud',
credential: 'existing',
environment: 'production',
});
await expect(profileAddCommand('existing-profile')).rejects.toThrow('process.exit called');
expect(mockProfileManager.addProfile).not.toHaveBeenCalled();
});
});Refs:
| 1. **Remote type**: | ||
| - Select **Inkeep Cloud** if you have an Inkeep Cloud account (URLs are configured automatically) | ||
| - Select **Local** for local development (localhost defaults, no authentication required) | ||
| - Select **Custom** for self-hosted deployments, then enter your API and Manage UI URLs | ||
| 2. **Environment name**: Enter `production` (or your preferred name) |
There was a problem hiding this comment.
💭 Consider: Missing guidance on Environment name prompt
Issue: The docs simplify Step 1 to mention the three remote type options but don't explain what the Environment name prompt is for. Users see an undocumented prompt during the flow.
Why: Users following this guide may be confused when they see an "Environment name:" prompt not mentioned in the docs.
Fix: Consider expanding Step 2 to briefly explain the environment prompt:
2. **Environment name**: The CLI prompts for an environment name (defaults to `production` for Cloud/Custom, `development` for Local). This label helps organize profiles by deployment stage.
- 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>
nick-inkeep
force-pushed
the
fix/cli-port-mismatch-v2
branch
from
502196a to
49a7169
Compare
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
🟡 Minor (3) 🟡
🟡 1) init.ts, index.ts Comments and CLI help text conflate "local" with "self-hosted"
Issue: Three locations use "self-hosted" to describe the --local flag, but the PR explicitly separates these use cases: --local is for local development (localhost defaults, no auth), while self-hosted deployments should use inkeep profile add with Custom.
Locations:
init.ts:316— JSDoc says "Simple local init for self-hosted deployments"init.ts:58— Inline comment says "Check if user wants local init (self-hosted)"index.ts:70— CLI help says "Use local/self-hosted mode"
Why: Users running inkeep init --help will see "self-hosted" and use --local for their production self-hosted deployment, resulting in a profile with credential: 'none' that won't work with authenticated servers.
Fix: Update comments to clarify --local is for local development only:
- JSDoc: "Local init for development environments"
- Inline: "Check if user wants local development init or cloud init"
- CLI help: "Initialize for local development (localhost defaults, no auth)"
Refs:
- cli-reference.mdx line 47 — updated docs correctly say "local development"
🟡 2) cli-reference.mdx:786-804 Docs example shows login --profile local but Local profiles don't need login
Issue: The "Using profiles with authenticated deployments" section uses inkeep login --profile local as the example. However, Local profiles auto-set credential: 'none' and don't require login. The docs even state this on line 827: "Local profiles with credential: none do not require login."
Why: Users following this example may run inkeep login on their Local profile and see odd behavior, wondering if they misconfigured something.
Fix: Use a different example profile name that implies authentication:
# Authenticate against your secured deployment
inkeep login --profile stagingRefs:
- cli-reference.mdx line 827 — states Local profiles don't require login
🟡 3) setup-profile.mdx:33-37 Setup guide incomplete — missing credential prompt documentation
Issue: The setup guide documents the three remote type options but omits the credential prompt that follows for Cloud and Custom paths. Users selecting Cloud or Custom will encounter an undocumented "Credential reference:" prompt.
Why: Users may be confused when they see a prompt not mentioned in the documentation.
Fix: Expand the documentation to include the credential prompt:
1. **Remote type**:
- Select **Inkeep Cloud** if you have an Inkeep Cloud account
- Select **Local** for local development (no auth required)
- Select **Custom** for self-hosted deployments
2. **Environment name**: Enter `production` (or `development` for Local)
3. **Credential reference** (Cloud and Custom only): Name for storing credentials (defaults to `inkeep-<profile-name>`)Refs:
Inline Comments:
- 🟡 Minor:
config.ts:91Config generates flatapiUrlformat, inconsistent withinit.tsnestedagentsApiformat - 🟡 Minor:
profile.test.ts:146Missing test coverage for URL validation in Custom path - 🟡 Minor:
profile.test.ts:182Missing test for profile switch confirmation flow
🕐 Pending Recommendations (3)
- 🟠
profile.test.ts:22-24Missing test for "profile already exists" error path (data loss prevention check untested) - 🟠
profile.test.ts:38-40Missing test coverage for user cancellation flows (6p.isCancel()checks untested) - 💭
setup-profile.mdx:33-37Missing guidance on Environment name prompt in docs
💡 APPROVE WITH SUGGESTIONS
Summary: The core bug fix is solid — centralizing localhost URLs via LOCAL_REMOTE eliminates the port mismatch that broke device-code auth. The Cloud/Local/Custom split in profile add is a well-designed UX improvement that correctly separates use cases.
The remaining issues are primarily documentation and test coverage gaps:
- Comments still say "self-hosted" when they should say "local development" — these predate the PR but are worth updating for consistency with the new taxonomy
- Test coverage gaps for validation, cancellation, and error paths — the happy paths are well tested, but edge cases are missing
- Docs incomplete — credential prompt undocumented, and one example contradicts the stated behavior
Previous feedback about the slack-api.ts scope creep was addressed (file removed from diff). The 3 pending test coverage issues from the prior review remain unaddressed.
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
url.ts:34 |
Hardcoded localhost not using LOCAL_REMOTE |
PR explicitly documents as "Not changing" — tracked in PR #1984 |
login.ts:170-174 |
Different cloud fallback URLs than CLOUD_REMOTE |
Pre-existing issue, not introduced by this PR |
| Other test files | Hardcoded localhost values | Pre-existing, fixing would be scope creep |
| Docs positive feedback | Cloud/Local/Custom taxonomy well-designed | Positive observation, not an issue |
profile.ts:92-93 |
Local hard-codes ports without prompting | Design tradeoff acknowledged in PR, reasonable choice |
troubleshooting.mdx |
Could mention profile current shows credential status |
Low confidence, minor completeness suggestion |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
5 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-consistency |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-docs |
3 | 2 | 0 | 0 | 0 | 1 | 0 |
pr-review-tests |
3 | 0 | 0 | 0 | 2 | 2 | 0 |
pr-review-comments |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
| Total | 18 | 4 | 0 | 0 | 3 | 3 | 9 |
Note: pr-review-standards returned 0 findings because issues were already flagged in prior review. Multiple reviewers flagged the same config format inconsistency (deduplicated to 1 inline comment). Comment issues on unchanged lines couldn't be posted inline.
| export default defineConfig({ | ||
| tenantId: '${key === 'tenantId' ? value : ''}', | ||
| apiUrl: '${key === 'apiUrl' ? value : 'http://localhost:3002'}', | ||
| apiUrl: '${key === 'apiUrl' ? value : LOCAL_REMOTE.api}', |
There was a problem hiding this comment.
🟡 Minor: Config format inconsistency with init.ts
Issue: This template generates configs with flat apiUrl format, while init.ts generates the nested agentsApi: { url: ... } format (lines 441-443). Users running different CLI commands will see different config formats.
Why: A user who runs inkeep init --local then later uses inkeep config set will encounter two different config formats in their project, creating confusion about which is canonical.
Fix: Consider updating to use the nested format for consistency:
export default defineConfig({
tenantId: '${key === 'tenantId' ? value : ''}',
agentsApi: {
url: '${key === 'apiUrl' ? value : LOCAL_REMOTE.api}',
},
});Refs:
| credential: 'inkeep-staging', | ||
| environment: 'staging', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: Missing test for URL validation in Custom path
Issue: The production code at profile.ts lines 98-106 and 117-125 validates Custom URLs — rejecting empty input ('URL is required') and invalid formats ('Invalid URL format'). These validation branches have no test coverage.
Why: If URL validation regresses, users could create profiles with invalid/empty URLs that would fail at runtime during login or push operations.
Fix: Add validation tests to the Custom describe block:
it('should reject empty API URL with validation error', async () => {
vi.mocked(p.select).mockResolvedValueOnce('custom');
let apiValidate: ((v: string) => string | undefined) | undefined;
vi.mocked(p.text).mockImplementation(async (opts: any) => {
if (opts.message === 'Agents API URL:') {
apiValidate = opts.validate;
return 'https://api.example.com';
}
if (opts.message === 'Manage UI URL:') return 'https://manage.example.com';
if (opts.message === 'Environment name:') return 'production';
if (opts.message === 'Credential reference:') return 'inkeep-test';
return '';
});
vi.mocked(p.confirm).mockResolvedValueOnce(false);
await profileAddCommand('test');
expect(apiValidate!('')).toBe('URL is required');
expect(apiValidate!(' ')).toBe('URL is required');
expect(apiValidate!('not-a-url')).toBe('Invalid URL format');
});Refs:
| expect(mockProfileManager.checkCredentialExists).toHaveBeenCalledWith('inkeep-prod'); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: Missing test for profile switch confirmation
Issue: The production code at profile.ts lines 204-212 prompts the user to switch to the newly created profile. All 9 tests mock p.confirm to return false, so the setActiveProfile call after successful confirmation is never exercised.
Why: If the setActiveProfile call were removed or moved incorrectly, users who confirm switching would not actually have their active profile changed.
Fix: Add a test that confirms the switch:
it('should switch to new profile when user confirms', async () => {
vi.mocked(p.select).mockResolvedValueOnce('local');
vi.mocked(p.text).mockResolvedValueOnce('development');
vi.mocked(p.confirm).mockResolvedValueOnce(true); // User wants to switch
await profileAddCommand('new-local');
expect(mockProfileManager.setActiveProfile).toHaveBeenCalledWith('new-local');
});Refs:
|
Superseded by next PR (clean history). |
1 participant
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