feat(cli): add alternative authentication for link command#24
feat(cli): add alternative authentication for link command#24jwfing merged 3 commits intoInsForge:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds direct linking via CLI options Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/commands/projects/link.ts (1)
39-40: Consider validating--api-base-urlformat.The
apiBaseUrlis used directly inoss_hostand later inreportCliUsagefor HTTP requests. A malformed URL will cause runtime failures. Consider basic validation (e.g., URL constructor check).Proposed validation
if (opts.apiBaseUrl && opts.apiKey) { + // Validate URL format + try { + new URL(opts.apiBaseUrl); + } catch { + throw new CLIError('Invalid --api-base-url: must be a valid URL.'); + } + // Direct OSS/Self-hosted linking bypasses OAuth const projectConfig: ProjectConfig = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/projects/link.ts` around lines 39 - 40, Validate the --api-base-url value before assigning it to oss_host and before calling reportCliUsage: in the code handling opts (where api_key: opts.apiKey and oss_host: opts.apiBaseUrl are set) add a basic URL validation (e.g., attempt to construct new URL(opts.apiBaseUrl) and catch errors) and reject/throw or surface a clear CLI error if invalid; ensure reportCliUsage and any HTTP client only run when opts.apiBaseUrl passes this check so malformed URLs cannot be used..gitignore (1)
4-4: Minor redundancy:.insforge/and.insforgeboth present.Line 4 already ignores
.insforge/(directory), and line 9 adds.insforge(without slash), which matches both files and directories. The second pattern subsumes the first. Consider removing the duplicate on line 4, or keep both if intentional for clarity.Also applies to: 9-9
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 4, The .gitignore contains duplicate patterns for the same path (".insforge/" and ".insforge"); remove the redundancy by deleting one of them—prefer keeping the simpler ".insforge" pattern and remove ".insforge/" (or vice versa if you prefer explicit directory notation) so only a single ignore entry for insforge remains; update the .gitignore accordingly to eliminate the duplicate entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/projects/link.ts`:
- Around line 31-56: The direct linking branch (where opts.apiBaseUrl &&
opts.apiKey creates projectConfig and calls saveProjectConfig,
installCliGlobally, installSkills, and reportCliUsage) needs its own try-catch
so failures report the correct telemetry; wrap the entire direct-link block in a
try/catch, and in the catch call reportCliUsage('cli.link_direct', false,
<error-code-or-default>) and rethrow or surface the error, while preserving the
existing success path that calls reportCliUsage('cli.link_direct', true, 6).
Ensure the catch references the same symbols: saveProjectConfig,
installCliGlobally, installSkills, and reportCliUsage.
- Around line 31-41: The code currently treats only the case where both
opts.apiBaseUrl and opts.apiKey are present; add a guard to detect when exactly
one is provided (i.e., XOR) and throw a CLIError with a clear message like "Both
--api-base-url and --api-key must be provided for direct linking" to avoid
silently falling back to OAuth; update the block around the existing check of
opts.apiBaseUrl and opts.apiKey (the ProjectConfig construction) to first if
(opts.apiBaseUrl && opts.apiKey) { ... } else if (opts.apiBaseUrl ||
opts.apiKey) { throw new CLIError('Both --api-base-url and --api-key must be
provided for direct linking'); } and ensure CLIError is imported where used.
---
Nitpick comments:
In @.gitignore:
- Line 4: The .gitignore contains duplicate patterns for the same path
(".insforge/" and ".insforge"); remove the redundancy by deleting one of
them—prefer keeping the simpler ".insforge" pattern and remove ".insforge/" (or
vice versa if you prefer explicit directory notation) so only a single ignore
entry for insforge remains; update the .gitignore accordingly to eliminate the
duplicate entry.
In `@src/commands/projects/link.ts`:
- Around line 39-40: Validate the --api-base-url value before assigning it to
oss_host and before calling reportCliUsage: in the code handling opts (where
api_key: opts.apiKey and oss_host: opts.apiBaseUrl are set) add a basic URL
validation (e.g., attempt to construct new URL(opts.apiBaseUrl) and catch
errors) and reject/throw or surface a clear CLI error if invalid; ensure
reportCliUsage and any HTTP client only run when opts.apiBaseUrl passes this
check so malformed URLs cannot be used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f402beac-2f9b-44a3-aefe-6a99b5a2bec6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.gitignoreskills-lock.jsonsrc/commands/projects/link.ts
skills-lock.json
Outdated
| @@ -0,0 +1,15 @@ | |||
| { | |||
There was a problem hiding this comment.
This file is involved by the execution of insforge link command, right? I don't think we need add it to repo.
| await reportCliUsage('cli.link_direct', true, 6); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing validation when only one of the two flags is provided
src/commands/projects/link.ts
Outdated
| try { | ||
| if (opts.apiBaseUrl && opts.apiKey) { | ||
| // Direct OSS/Self-hosted linking bypasses OAuth | ||
| const projectConfig: ProjectConfig = { |
There was a problem hiding this comment.
No URL validation on --api-base-url
| .insforge/ | ||
| .env | ||
| .env.* | ||
|
|
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
After linked a project with --api-key and --api-base-url, you run any of commands:
- insforge db tables
- insforge docs
- insforge storage
- etc.
It always requests to OAuth login first.
|
It's not a simple setting update, I think testing all commands is necessary for new initialized mothod(link with project metadata directly). |
|
Please tell us how you tested it! and address the ai concerns! |
|
Thanks for the review! @jwfing @tonychang04 I addressed the feedback: • Added validation to ensure both --api-base-url and --api-key must be provided together Testing performed:
Please let me know if additional changes are needed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/credentials.ts (1)
8-21: Consider adding a clarifying comment for the OSS credential bypass.The null-safe access via
projConfig?.project_idcorrectly handles the case whengetProjectConfig()returnsnull. The hardcoded credentials approach is reasonable for OSS projects that authenticate viaapi_keyinstead of OAuth tokens.Adding a brief comment explaining why these placeholder values are acceptable (i.e., OSS commands use
api_keyfrom project config rather than these tokens) would help future maintainers understand the design intent.Suggested documentation
const projConfig = getProjectConfig(); if (projConfig?.project_id === 'oss-project') { + // OSS/self-hosted projects authenticate via api_key from project config, + // not OAuth tokens. Return placeholder credentials to satisfy the type contract. return { access_token: 'oss-token', refresh_token: 'oss-refresh',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/credentials.ts` around lines 8 - 21, Add a short clarifying comment above the OSS bypass block that explains why getProjectConfig() is null-safe checked and why hardcoded placeholder tokens (returned when projConfig?.project_id === 'oss-project') are acceptable: note that OSS projects authenticate via api_key in project config (not OAuth tokens), so these access_token/refresh_token/user values are dummies used only for compatibility with callers; reference projConfig, getProjectConfig, and the 'oss-project' project_id so maintainers know the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/projects/link.ts`:
- Around line 31-71: Move the validation and direct-link flow into a single
try-catch so all errors for the direct-link path are handled and reported only
as 'cli.link_direct' (avoid letting CLIError thrown by validation bubble to the
outer catch). Specifically, include the checks of opts.apiBaseUrl and
opts.apiKey and the new URL() validation inside the same try block that creates
ProjectConfig, calls saveProjectConfig, installCliGlobally, installSkills, and
reportCliUsage('cli.link_direct', ...); in the catch for that block call
reportCliUsage('cli.link_direct', false) and then either handle/exit (so you
don’t re-throw into the outer catch) or re-throw a transformed error that the
outer handler will ignore for telemetry — the key is to ensure only
reportCliUsage('cli.link_direct', false) runs for direct-link errors and remove
the duplicate reporting from the outer 'cli.link' path.
---
Nitpick comments:
In `@src/lib/credentials.ts`:
- Around line 8-21: Add a short clarifying comment above the OSS bypass block
that explains why getProjectConfig() is null-safe checked and why hardcoded
placeholder tokens (returned when projConfig?.project_id === 'oss-project') are
acceptable: note that OSS projects authenticate via api_key in project config
(not OAuth tokens), so these access_token/refresh_token/user values are dummies
used only for compatibility with callers; reference projConfig,
getProjectConfig, and the 'oss-project' project_id so maintainers know the
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91fe6d56-4f1d-47ca-989d-693aacb1dfc6
📒 Files selected for processing (2)
src/commands/projects/link.tssrc/lib/credentials.ts
|
@git112 I think you changed the default behavior for create command: |
|
@git112 any updates? we can't change the behavior of |
|
I’ve identified the issue and I’m updating the auth handling. |
3 participants
Closes #20
Added
--api-base-urland--api-keyoptions to theinsforge linkcommand to bypass OAuth. This enables direct project linking for OSS/self-hosted instances, remote servers, CI, headless environments, and TUI tools.Summary by CodeRabbit
New Features
Chores