feat(cli): add insforge config export/plan/apply (MVP)#109
Open
tonychang04 wants to merge 12 commits intomainfrom
Open
feat(cli): add insforge config export/plan/apply (MVP)#109tonychang04 wants to merge 12 commits intomainfrom
insforge config export/plan/apply (MVP)#109tonychang04 wants to merge 12 commits intomainfrom
Conversation
WalkthroughThis PR introduces a config management system for the InsForge CLI. It adds TOML parsing via ChangesConfig Management System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@src/commands/config/apply.ts`:
- Around line 85-88: The applyChange function currently declares an unused
parameter file; remove file from the applyChange signature (async function
applyChange(change: DiffChange): Promise<void>) and update all callers to stop
passing that argument, and update any related overload/alternate applyChange
declaration that also lists file to match (remove the unused file parameter
there as well); ensure TypeScript types and imports remain consistent after the
signature change.
- Around line 42-44: The code emits two JSON objects when the json flag is set
(see the if (json) { console.log(JSON.stringify(result, null, 2)); } else { ...
} blocks and the similar block at lines 69-71), breaking callers expecting a
single JSON payload; change both places to avoid multiple prints by composing a
single JSON document (e.g., create a combined object containing plan and apply
fields or replace the earlier plan print with storing plan data into a shared
result object) and call JSON.stringify exactly once at the end when json is
true, leaving the non-json branches unchanged; update the branches that
reference json, result, and any planResult/applyResult variables so only one
console.log(JSON.stringify(...)) is executed per invocation.
In `@src/commands/config/export.ts`:
- Around line 26-35: The current export flow prompts via p.confirm (and checks
p.isCancel) when the target exists, which will hang in non-interactive runs when
opts.json is true; change the logic in the block that checks existsSync(target)
to first check opts.json and, if opts.json is set and opts.force is not true,
fail fast (emit an error/JSON error output or return non-zero) instead of
calling p.confirm; otherwise preserve the existing behavior (if not json and not
force, call p.confirm and handle p.isCancel). Ensure you update the branch that
references target, opts.out, opts.force, p.confirm and p.isCancel accordingly.
In `@src/lib/config-diff.ts`:
- Around line 40-42: The comparison of allowed_redirect_urls is order-sensitive
and can produce churn; before calling arrayEquals on fromV and toV, normalize
both allowed_redirect_urls (e.g., in the logic surrounding fromV/toV) by making
deterministic sets—sort the arrays (and optionally deduplicate) so order
differences don't trigger arrayEquals, and apply the same normalization for the
other occurrence referenced around lines 64-67; update the code that builds
fromV/toV to use the normalized arrays before comparing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9b09d7d-6581-47e7-9c99-46d96f39511d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.jsonsrc/commands/config/apply.tssrc/commands/config/export.tssrc/commands/config/index.tssrc/commands/config/plan.tssrc/index.tssrc/lib/config-diff.test.tssrc/lib/config-diff.tssrc/lib/config-format.test.tssrc/lib/config-format.tssrc/lib/config-schema.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/config/apply.ts">
<violation number="1" location="src/commands/config/apply.ts:43">
P2: `--json` apply emits two JSON documents (plan and applied result), which makes machine parsing unreliable.</violation>
<violation number="2" location="src/commands/config/apply.ts:53">
P2: The confirmation guard does not honor global `-y/--yes`, so non-interactive runs can still block on a prompt.</violation>
</file>
<file name="src/commands/config/export.ts">
<violation number="1" location="src/commands/config/export.ts:26">
P2: Skip the overwrite confirmation prompt in `--json` mode so export remains non-interactive and machine-readable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
3 tasks
- config-diff: normalize allowed_redirect_urls (sort + dedupe) before comparing so reordering or duplicating URLs in TOML produces no diff; emit normalized values so the rendered plan and applied request body match what's actually different. - apply: collapse --json output to a single document (plan + applied state) so jq and other consumers can parse one payload per invocation. - apply: honor the global -y/--yes flag (alongside --auto-approve), and in --json mode require explicit consent rather than silently applying or hanging on a TTY prompt. - apply: drop the unused `file` parameter from applyChange. - export: in --json mode, fail fast with an actionable error when the output path exists and --force isn't set, instead of hanging on a TTY confirm. Adds three tests covering reorder, dedupe, and normalized-diff output. All existing tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-project InsForge backends evolve independently from the CLI. A user can have a CLI from npm @latest pointed at a project on any prior backend version. Without a probe, `config apply` would silently drop fields on older backends that ignore unknown PUT bodies, or hang on schema mismatches. Adds `metadataSupports(raw, change)` in src/lib/config-capabilities.ts. Probes the RAW `/api/metadata` response (not the Zod-parsed object — defaults would mask absence) for the field a change targets. - apply: per-change gate. Supported changes apply, unsupported ones push to skipped[] with an actionable "upgrade backend" message. JSON output shape becomes { plan, applied, skipped }. No PUT issued for unsupported sections, so older permissive servers can't silently swallow the body. - plan: tags each change as supported/skipped, surfaces skip list up front so users aren't surprised by apply. - export: only emits TOML sections the backend exposes. The file represents what THIS backend can do; future apply cycles match. Skipped sections reported in JSON output. Tests: - 8 unit tests in config-capabilities.test.ts covering presence vs absence vs malformed inputs. - Integration tests for apply (modern/legacy/empty-array), export (skipped section omitted from output), plan (skipped surfaced). - All 210 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- config-capabilities.ts: use !== undefined && !== null instead of != null
(eqeqeq rule)
- config-toml.ts: attach the original parser error as { cause } so it
isn't swallowed (preserve-caught-error rule)
- *.test.ts: replace inline `typeof import('...')` with a top-level
`import type * as` (consistent-type-imports rule)
All 210 tests still pass; my files now have zero lint findings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/config/apply.ts">
<violation number="1" location="src/commands/config/apply.ts:103">
P2: `--json` output now uses inconsistent types for `applied` across branches (boolean vs array), which can break automation parsing idempotent/no-op runs.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic
|
|
||
| if (json) { | ||
| console.log( | ||
| JSON.stringify({ plan: result, applied, skipped }, null, 2), |
There was a problem hiding this comment.
P2: --json output now uses inconsistent types for applied across branches (boolean vs array), which can break automation parsing idempotent/no-op runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/config/apply.ts, line 103:
<comment>`--json` output now uses inconsistent types for `applied` across branches (boolean vs array), which can break automation parsing idempotent/no-op runs.</comment>
<file context>
@@ -78,19 +79,44 @@ export function registerConfigApplyCommand(cfg: Command): void {
if (json) {
console.log(
- JSON.stringify({ plan: result, applied: true, changes: result.changes }, null, 2),
+ JSON.stringify({ plan: result, applied, skipped }, null, 2),
);
} else {
</file context>
Suggested change
| JSON.stringify({ plan: result, applied, skipped }, null, 2), | |
| JSON.stringify({ plan: result, applied: applied.length > 0, changes: applied, skipped }, null, 2), |
Tip: Review your code locally with the cubic CLI to iterate faster.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@src/commands/config/apply.test.ts`:
- Around line 35-44: The test's vi.mock call is using a TypeScript "typeof
import('../../lib/errors.js')" annotation on the orig call which breaks CI;
change the mock to call the original factory without the typeof import
annotation (i.e. use await orig() to get the actual module) and keep the
override of handleError (vi.fn((err: unknown) => { throw err; })) intact so
tests can inspect thrown errors — update the code around the vi.mock(...) block
and the const actual assignment that currently uses typeof import(...) to
instead use await orig() or equivalent.
In `@src/commands/config/export.test.ts`:
- Around line 28-36: The vi.mock call is using the lint-problematic pattern
"await orig<typeof import(... )>()"; change it to get the original module
without the typeof-import generic and then cast the result to the module type
before spreading. Concretely, inside the vi.mock callback for the errors module,
replace the generic await orig<typeof import('../../lib/errors.js')>() with
const actual = await orig() and then treat actual as the proper module type
(e.g., cast to typeof import('../../lib/errors.js')) before returning
{...actual, handleError: vi.fn((err: unknown) => { throw err; })}; this removes
the typeof import() usage while preserving the handleError mock.
In `@src/commands/config/plan.test.ts`:
- Around line 28-36: Remove the inline typeof import() generic on the await orig
call and instead import the module type with a top-level type import (e.g. add
"import type * as ErrorsModule from '../../lib/errors.js'"), then call "const
actual = await orig()" (no generic) and cast it like "actual as unknown as
typeof ErrorsModule" before spreading and replacing handleError in the vi.mock
callback; keep references to vi.mock, orig, and handleError as in the diff.
In `@src/lib/config-capabilities.ts`:
- Around line 39-43: Replace the loose inequality in the predicate check: change
the `raw?.auth != null` usage to a strict check so ESLint's eqeqeq rule is
satisfied; update the condition at the start of the boolean return (the
`raw?.auth != null` expression) to use `!==` (or explicitly check both `!==
undefined` and `!== null`) while keeping the rest of the checks (`typeof
raw.auth === 'object'` and `'allowedRedirectUrls' in raw.auth`) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d25398f8-4638-4972-a0a6-ad7056325a1e
📒 Files selected for processing (8)
src/commands/config/apply.test.tssrc/commands/config/apply.tssrc/commands/config/export.test.tssrc/commands/config/export.tssrc/commands/config/plan.test.tssrc/commands/config/plan.tssrc/lib/config-capabilities.test.tssrc/lib/config-capabilities.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/commands/config/apply.test.ts (1)
76-79: ⚡ Quick winMove temp directory cleanup to
afterEachto avoid leaks on test failure.
rmSyncat the bottom of eachitblock is never reached when an assertion throws, leaving temp dirs behind in CI. Moving cleanup toafterEachis idiomatic and guarantees cleanup regardless of test outcome.♻️ Proposed refactor
+afterEach(() => { + if (tmp) rmSync(tmp, { recursive: true, force: true }); +}); + beforeEach(() => { vi.clearAllMocks(); tmp = mkdtempSync(join(tmpdir(), 'insforge-apply-test-')); });Then remove the three inline
rmSync(tmp, { recursive: true, force: true })calls from the end of eachitblock (lines 113, 144, 166).🤖 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/config/apply.test.ts` around lines 76 - 79, The tests create a temp directory in beforeEach via tmp = mkdtempSync(join(tmpdir(), 'insforge-apply-test-')), but cleanup is done with inline rmSync calls inside individual it blocks which are skipped when a test throws; move the recursive rmSync(tmp, { recursive: true, force: true }) call into an afterEach block to guarantee deletion, keep the mkdtempSync creation in beforeEach, and remove the three inline rmSync(...) invocations at the end of the individual it tests so cleanup is centralized and always executed.src/commands/config/export.test.ts (1)
68-71: ⚡ Quick winSame
afterEachcleanup gap as inapply.test.ts.
rmSyncat lines 98 and 121 is unreachable when an assertion fails, leaving temp dirs in CI.♻️ Proposed refactor
+afterEach(() => { + if (tmp) rmSync(tmp, { recursive: true, force: true }); +}); + beforeEach(() => { vi.clearAllMocks(); tmp = mkdtempSync(join(tmpdir(), 'insforge-export-test-')); });Then remove the inline
rmSynccalls from the end of eachitblock (lines 98 and 121).🤖 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/config/export.test.ts` around lines 68 - 71, The tests create a temp dir in beforeEach via mkdtempSync assigned to tmp but currently clean it up with inline rmSync calls at the end of individual it blocks (e.g., the rmSync calls referenced in the review), which are skipped on assertion failures; add a shared afterEach that safely removes tmp (use fs.rmSync or fs.rmdirSync with recursive:true or check fs.existsSync before removing and wrap in try/catch) to guarantee cleanup, and then remove the inline rmSync calls from the it blocks; reference the beforeEach/tmp initialization and update the test file’s afterEach cleanup to always run.
🤖 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/config/apply.test.ts`:
- Around line 76-79: The tests create a temp directory in beforeEach via tmp =
mkdtempSync(join(tmpdir(), 'insforge-apply-test-')), but cleanup is done with
inline rmSync calls inside individual it blocks which are skipped when a test
throws; move the recursive rmSync(tmp, { recursive: true, force: true }) call
into an afterEach block to guarantee deletion, keep the mkdtempSync creation in
beforeEach, and remove the three inline rmSync(...) invocations at the end of
the individual it tests so cleanup is centralized and always executed.
In `@src/commands/config/export.test.ts`:
- Around line 68-71: The tests create a temp dir in beforeEach via mkdtempSync
assigned to tmp but currently clean it up with inline rmSync calls at the end of
individual it blocks (e.g., the rmSync calls referenced in the review), which
are skipped on assertion failures; add a shared afterEach that safely removes
tmp (use fs.rmSync or fs.rmdirSync with recursive:true or check fs.existsSync
before removing and wrap in try/catch) to guarantee cleanup, and then remove the
inline rmSync calls from the it blocks; reference the beforeEach/tmp
initialization and update the test file’s afterEach cleanup to always run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6babb16-be41-4e63-bf77-02999c5472f3
📒 Files selected for processing (5)
src/commands/config/apply.test.tssrc/commands/config/export.test.tssrc/commands/config/plan.test.tssrc/lib/config-capabilities.tssrc/lib/config-toml.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/config-toml.ts
- src/commands/config/plan.test.ts
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to:
InsForge/InsForge#1216(backend half — extends `/api/metadata` to surface `auth.allowed_redirect_urls`)What
Adds three CLI commands that round-trip a single TOML field —
[auth] allowed_redirect_urls— between `insforge.toml` and the live project state:```bash
insforge config export # GET /api/metadata → write insforge.toml
insforge config plan # diff TOML vs metadata, print plan
insforge config apply # plan → confirm → PUT /api/auth/config
```
This is the smallest possible vertical slice of the config-as-code feature. Everything else (SMTP, OAuth providers, custom domains, etc.) is matrix-fill on top of this architecture.
Why
Agents can't make fine-grained config changes today — setting up a custom domain, configuring SMTP, changing an auth redirect URL all require dashboard clicks (invisible, not reproducible) or per-field CLI subcommands (sprawls fast, no PR diff). With ~100 config knobs across the platform, per-field CLI is the wrong shape.
This PR ships the TOML + apply loop with the smallest possible scope so the architecture proves out before we widen the surface. Full design in `spec-toml-mvp.md` and `proposal-insforge-config-toml.md`.
What this PR contains
9 commits, all on `feat/insforge-toml-mvp` off `main`:
Lib: 6 files in `src/lib/config-{schema,toml,diff,format}.ts` + tests
Commands: 4 files in `src/commands/config/{index,export,plan,apply}.ts`
Tests: 12 vitest tests, all green
What this PR does NOT do
Behaviors that distinguish this from Supabase `config push`
Smoke test (against mock backend)
End-to-end loop verified locally:
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds
insforge config export/plan/applyto manage[auth] allowed_redirect_urlsviainsforge.toml, now with backend capability probing. Unsupported fields are skipped safely; export only writes fields the backend exposes, and JSON output is a single, machine‑readable document.New Features
insforge config export: reads/api/metadata, writesinsforge.toml; emits only supported fields and reportsskipped.insforge config plan: diffs file vs live (human or JSON) and listsskippedpaths when the backend omits a field.insforge config apply: shows plan, prompts unless--auto-approve/-y, supports--dry-run; per-change gate applies only supported fields; writes viaPUT /api/auth/config.Bug Fixes
allowed_redirect_urls(sort + dedupe); reordering/duplicates produce no diff; emitted values are normalized.apply --jsonprints one JSON object{ plan, applied, skipped }, honors-y/--yes, and requires explicit consent in--json.export --jsonfails fast if the output file exists without--forceinstead of prompting.Written for commit d8d34a6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Chores