feat(link): install skills only when called with no args#120
Conversation
When `link` is invoked without any of --project-id, --org-id, --api-base-url, --api-key, --template, or --auth, skip the auth and project picker entirely and just install the InsForge agent skills into the current directory. The agent walks the user through sign-in and project provisioning later, when it actually needs backend services. This makes the new-user "try InsForge with my agent" path one short, sign-in-free command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThe ChangesSkills-only fast path for link command
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/projects/link.ts (1)
103-103: ⚡ Quick winPrefer explicit option check for robustness.
The current check using
Object.values(opts).every((v) => v === undefined)works but is fragile. If a boolean flag option is added in the future, it would default tofalserather thanundefined, causing this check to fail unexpectedly.♻️ Proposed fix for more explicit and maintainable check
-const isSkillsOnly = Object.values(opts).every((v) => v === undefined); +const isSkillsOnly = !opts.projectId && !opts.orgId && !opts.template && !opts.auth && !opts.apiBaseUrl && !opts.apiKey;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/projects/link.ts` at line 103, Replace the fragile all-values-undefined test with an explicit check of known option keys: instead of const isSkillsOnly = Object.values(opts).every((v) => v === undefined), create a list of option names you expect (e.g., const optionKeys = ['projectId','path','force', ...]) and set isSkillsOnly = optionKeys.every(k => opts[k] === undefined); update any logic using isSkillsOnly accordingly so future boolean or new options don't break this detection; reference the existing opts object and isSkillsOnly constant in src/commands/projects/link.ts when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commands/projects/link.ts`:
- Line 103: Replace the fragile all-values-undefined test with an explicit check
of known option keys: instead of const isSkillsOnly =
Object.values(opts).every((v) => v === undefined), create a list of option names
you expect (e.g., const optionKeys = ['projectId','path','force', ...]) and set
isSkillsOnly = optionKeys.every(k => opts[k] === undefined); update any logic
using isSkillsOnly accordingly so future boolean or new options don't break this
detection; reference the existing opts object and isSkillsOnly constant in
src/commands/projects/link.ts when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95dc3879-ffbe-44a8-9491-41c34eb5c1d8
📒 Files selected for processing (1)
src/commands/projects/link.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Adds a clean "skills-only fast path" to projects link — when called with no args it installs agent skills without requiring auth, org/project picker, or writing .insforge/project.json. The intent is clear and the implementation is mostly correct, but it ships without any tests.
Requirements context
No /docs/superpowers/ directory exists in this repo. Review assessed against the PR description alone, cross-referenced with the project's TDD convention (insforge-development skill) which requires every new behavior to have at least one test.
Findings
Critical
[Software Engineering] No test coverage for the new fast path
src/commands/projects/link.ts (lines 103–126)
There is no link.test.ts file anywhere in the repo (confirmed via glob). The project has test files for create, all branch/* subcommands, and various lib/ modules — the link command is the only command-level entrypoint with zero tests. This PR adds an entirely new execution branch (skills-only mode) with no tests covering:
- The happy path (no args →
installSkillscalled, correct output) - The
--jsonflag variant (no args +--json→{ success: true, skills_only: true }) - The error path (
installSkillsthrows →reportCliUsage(false)+handleErrorcalled)
Per the project's TDD convention: "Every new behavior has at least one test." Please add a link.test.ts (or equivalent) before merging.
Suggestion
[Functionality] isSkillsOnly detection is fragile
src/commands/projects/link.ts:103
const isSkillsOnly = Object.values(opts).every((v) => v === undefined);This works correctly today because every declared option uses a <value> placeholder and defaults to undefined. But it will silently break if a future contributor adds:
- a boolean flag with no placeholder (e.g.
.option('--verbose')→ defaults tofalse, notundefined) - an option with an explicit default value
A more robust form makes the intent explicit and is immune to future option additions:
const isSkillsOnly =
opts.projectId === undefined &&
opts.orgId === undefined &&
opts.template === undefined &&
opts.auth === undefined &&
opts.apiBaseUrl === undefined &&
opts.apiKey === undefined;Information
[Software Engineering] Double shutdownAnalytics() in the skills-only error path
src/commands/projects/link.ts:122 and src/commands/projects/link.ts:537
When installSkills throws, shutdownAnalytics() is called explicitly at line 122, then again in the outer finally at line 537. This is consistent with the existing pattern for the direct-link error path (lines 300–303) so it's not a regression, but it's a pre-existing code smell worth tracking.
Verdict
request_changes — one Critical finding (missing tests for new behavior).
- Replace `Object.values(opts).every(...)` with explicit per-option
undefined checks. Same behavior today, but immune to future options
that ship with a default value or a boolean flag.
- Add `src/commands/projects/link.test.ts` with four cases:
* no-args: installSkills called, trackCommand + reportCliUsage fire,
auth and org picker stay untouched
* --json: emits a single skills_only success payload
* installSkills throws: reportCliUsage('cli.link_skills_only', false)
and exit code 1
* --project-id provided: skills-only fast path is bypassed and the
OAuth path is entered
Addresses review feedback on #120.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Code Review — feat(link): install skills only when called with no args
Summary: The PR adds a clean "skills-only fast path" to projects link that skips auth, org/project selection, and config writing when the command is invoked with no options, installing agent skills immediately instead.
Requirements context: No /docs/superpowers/ directory exists in this repo. Review assessed against the PR title, body, and the implementation itself.
Findings
Critical
(none)
Suggestion
Software engineering — isSkillsOnly is a growing enumeration that can silently exclude future flags
src/commands/projects/link.ts:103–109
The fast-path gate enumerates every option explicitly:
const isSkillsOnly =
opts.projectId === undefined &&
opts.orgId === undefined &&
opts.template === undefined &&
opts.auth === undefined &&
opts.apiBaseUrl === undefined &&
opts.apiKey === undefined;This is correct today, but any new option added to link in the future won't automatically be excluded from the fast path — the maintainer must remember to extend this list. A short comment (// extend this check when adding new options) would prevent a silent regression. Alternatively, you could guard with an assertion in the option definitions themselves, but the comment is the lower-friction fix.
Software engineering — process.exit monkey-patching in tests
src/commands/projects/link.test.ts:112–128, 141–153
The error and bypass tests monkey-patch process.exit by direct assignment:
(process.exit as unknown) = (code?: number) => { … throw new Error('__exit__'); };This pattern works, but vitest provides vi.spyOn(process, 'exit').mockImplementation(…) which is more idiomatic and automatically restores on vi.clearAllMocks() / vi.restoreAllMocks() in the beforeEach. The current try/finally restore is correct, but vi.spyOn would be safer if an earlier line inside try throws before reaching the mutation.
Software engineering — dynamic module imports inside test bodies
src/commands/projects/link.test.ts:88–93, 103–104, 118, 134–135, 148–149
After parseAsync, each test does:
const { installSkills } = await import('../../lib/skills.js');Since vi.mock is hoisted, the mock reference is stable and can be imported once at the top of the file with a const { installSkills } = await import(…) or by capturing the return value of vi.mock. The repeated dynamic imports add noise without benefit and are mildly confusing to future readers.
Information
Functionality — return after handleError in the inner catch is dead code
src/commands/projects/link.ts:129–131
handleError(err, json);
return;handleError calls process.exit(1), so the return is never reached. Harmless, and consistent with the existing link_direct error path, so no action required — just noting it.
Functionality — trackCommand is not awaited
src/commands/projects/link.ts:114
trackCommand('link', 'skills-only', { skills_only: true });Not awaited, matching the pattern in all other link paths (lines 291, 402). Consistent with the existing convention; fine as-is.
Functionality — outer finally handles shutdownAnalytics on success
The skills-only success path calls return (line 125), which exits the inner try and still triggers the outer finally at line 543 — so shutdownAnalytics() is correctly called on both success and error without duplication on the success path. The error path does call it twice (inner catch + outer finally), but shutdownAnalytics is idempotent. No action needed.
Verdict
Approved (informational — no Critical findings; human sign-off still required via the approve flow).
The fast path logic is correct, the isSkillsOnly gate is explicit and tested, and the 4 tests cover the key behaviors (no-args, --json, error/exit-1, bypass via --project-id). The suggestions above are polish-level; none block merging.
2 participants
Summary by cubic
Running
projects linkwith no args now installs InsForge agent skills only, skipping auth and project linking. This creates a fast, sign-in-free quick start; the agent will guide provisioning later.linkinstalls skills in the current directory and exits; with--jsonit returns{ success: true, skills_only: true }..insforge/project.json; providing flags like--project-idbypasses this path.trackCommand,reportCliUsage) and exits with code 1 on install errors; detection uses explicit per-option checks with tests for no-args,--json, error, and bypass cases.Written for commit 1a0648c. Summary will update on new commits.
Summary by CodeRabbit
Documentation
New Features