INS-305 Expand declarative config apply coverage#156
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtends insforge.toml support with auth.disable_signup and optional endpoint-backed sections (storage, realtime, schedules): schema/validation, TOML I/O, state loading, capability probing, diffs, command dispatchers, tests, and README docs. ChangesExtended Declarative Config Workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Greptile SummaryThis PR extends the declarative
Confidence Score: 5/5Safe to merge; all new code paths are covered by tests and the 404 classification logic correctly surfaces resource-level errors instead of silently swallowing them. The optional-endpoint probing is gated by TOML section presence in plan/apply and always-on in export, which matches the intent. The new isMissingOptionalEndpoint guard uses the freshly-added statusCode field on CLIError to distinguish route-level 404s from resource-level ones cleanly, addressing the previously flagged overly-broad message.includes check. Test coverage spans the happy path, route-level 404 skip, and resource-level 404 propagation for each section. No files require special attention; the two style observations (sequential vs parallel fetches in plan/apply, and the triplicated fetchOptionalConfig helper) are non-blocking. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI
participant metadata as /api/metadata
participant storageAPI as /api/storage/config
participant realtimeAPI as /api/realtime/config
participant schedulesAPI as /api/schedules/config
Note over CLI: config plan / config apply
CLI->>metadata: GET (always)
metadata-->>CLI: RawMetadataResponse
alt [storage] in TOML
CLI->>storageAPI: GET (sequential)
storageAPI-->>CLI: RawStorageConfig or 404
end
alt [realtime] in TOML
CLI->>realtimeAPI: GET (sequential)
realtimeAPI-->>CLI: RawRetentionConfig or 404
end
alt [schedules] in TOML
CLI->>schedulesAPI: GET (sequential)
schedulesAPI-->>CLI: RawRetentionConfig or 404
end
Note over CLI: liveFromMetadata + diffConfig + metadataSupports gate
alt "change.section == storage"
CLI->>storageAPI: PUT maxFileSizeMb
end
alt "change.section == realtime"
CLI->>realtimeAPI: PATCH retentionDays
end
alt "change.section == schedules"
CLI->>schedulesAPI: PATCH retentionDays
end
Note over CLI: config export
CLI->>metadata: GET (always)
CLI->>storageAPI: GET (always, parallel)
CLI->>realtimeAPI: GET (always, parallel)
CLI->>schedulesAPI: GET (always, parallel)
metadata-->>CLI: RawMetadataResponse
storageAPI-->>CLI: RawStorageConfig or 404
realtimeAPI-->>CLI: RawRetentionConfig or 404
schedulesAPI-->>CLI: RawRetentionConfig or 404
Note over CLI: configFromMetadata to insforge.toml
Reviews (7): Last reviewed commit: "bump version" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isOptionalEndpointUnsupported(err: unknown): boolean { | ||
| if (!(err instanceof CLIError)) return false; | ||
| const message = err.message.toLowerCase(); | ||
| return ( | ||
| err.code === 'NOT_FOUND' || | ||
| message.includes('oss request failed: 404') || | ||
| message.includes('not found') || | ||
| message.includes('not available') || | ||
| message.includes('not enabled') | ||
| ); | ||
| } |
There was a problem hiding this comment.
err.code === 'NOT_FOUND' branch is dead code. ossFetch always throws new CLIError(message) with no third argument, so err.code is always undefined and this condition never fires. The only active guards are the three message.includes(…) checks below it. Removing the dead branch makes the actual fallback logic easier to reason about.
| function isOptionalEndpointUnsupported(err: unknown): boolean { | |
| if (!(err instanceof CLIError)) return false; | |
| const message = err.message.toLowerCase(); | |
| return ( | |
| err.code === 'NOT_FOUND' || | |
| message.includes('oss request failed: 404') || | |
| message.includes('not found') || | |
| message.includes('not available') || | |
| message.includes('not enabled') | |
| ); | |
| } | |
| function isOptionalEndpointUnsupported(err: unknown): boolean { | |
| if (!(err instanceof CLIError)) return false; | |
| const message = err.message.toLowerCase(); | |
| return ( | |
| message.includes('oss request failed: 404') || | |
| message.includes('not found') || | |
| message.includes('not available') || | |
| message.includes('not enabled') | |
| ); | |
| } |
| function isOptionalEndpointUnsupported(err: unknown): boolean { | ||
| if (!(err instanceof CLIError)) return false; | ||
| const message = err.message.toLowerCase(); | ||
| return ( | ||
| err.code === 'NOT_FOUND' || | ||
| message.includes('oss request failed: 404') || | ||
| message.includes('not found') || | ||
| message.includes('not available') || | ||
| message.includes('not enabled') | ||
| ); | ||
| } |
There was a problem hiding this comment.
The message.includes('not found') check matches any backend 404 whose body contains that phrase — including resource-level errors such as "Storage config not found" or "Project not found". If the backend returns such a message for a real error (misconfigured project, auth failure that produces a 404, etc.), fetchOptionalJson silently returns undefined, the capability gate sees the section as unsupported, and config apply reports the change as skipped with "your backend doesn't expose…". The actual error is swallowed and never surfaced to the user. ossFetch already distinguishes route-level 404s from resource-level ones via isRouteLevel404 when constructing the fallback message ('OSS request failed: 404'), so the message.includes('oss request failed: 404') check is the safe one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/config-state.ts (1)
15-25: 💤 Low valueOptional: fetch
/api/metadataalongside the optional endpoints.
/api/metadatais awaited before the optional fetches begin, adding one serial round-trip. Since the optional fetches don't depend on the parsed metadata, you can issue all requests concurrently and parse metadata from its resolved response.🤖 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/lib/config-state.ts` around lines 15 - 25, In loadConfigState, the call to ossFetch('/api/metadata') is awaited before starting the other optional requests which causes a serial round-trip; instead start the metadata fetch without awaiting (keep the Promise in metadataRes), kick off the other fetches with Promise.all (fetchOptionalJson and fetchOptionalEmailTemplates) concurrently, then await metadataRes.json() after the Promise.all resolves to parse metadata; update references to metadataRes/metadata accordingly so parsing happens after concurrent requests complete.
🤖 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/lib/config-state.ts`:
- Around line 52-62: isOptionalEndpointUnsupported currently uses broad
substring checks that can mask real backend failures; update it to rely
primarily on the CLIError code check and remove the loose message matches.
Modify the function isOptionalEndpointUnsupported to first confirm err is a
CLIError and then return true only when err.code === 'NOT_FOUND' (you may keep a
very specific exact check like message.includes('oss request failed: 404') if
needed), but remove the generic message.includes checks for 'not found', 'not
available', and 'not enabled' so unrelated errors are not suppressed.
---
Nitpick comments:
In `@src/lib/config-state.ts`:
- Around line 15-25: In loadConfigState, the call to ossFetch('/api/metadata')
is awaited before starting the other optional requests which causes a serial
round-trip; instead start the metadata fetch without awaiting (keep the Promise
in metadataRes), kick off the other fetches with Promise.all (fetchOptionalJson
and fetchOptionalEmailTemplates) concurrently, then await metadataRes.json()
after the Promise.all resolves to parse metadata; update references to
metadataRes/metadata accordingly so parsing happens after concurrent requests
complete.
🪄 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: 1a8122c8-8bc6-4cff-b1a8-fbda4b022c8a
📒 Files selected for processing (18)
README.mdsrc/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.tssrc/lib/config-diff.test.tssrc/lib/config-diff.tssrc/lib/config-format.tssrc/lib/config-metadata.test.tssrc/lib/config-metadata.tssrc/lib/config-schema.tssrc/lib/config-state.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
| function isOptionalEndpointUnsupported(err: unknown): boolean { | ||
| if (!(err instanceof CLIError)) return false; | ||
| const message = err.message.toLowerCase(); | ||
| return ( | ||
| err.code === 'NOT_FOUND' || | ||
| message.includes('oss request failed: 404') || | ||
| message.includes('not found') || | ||
| message.includes('not available') || | ||
| message.includes('not enabled') | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect CLIError definition and any status/code properties
fd -t f 'errors.ts' | xargs -I{} sh -c 'echo "== {} =="; cat "{}"'
# How ossFetch constructs CLIError (status codes available?)
fd -t f 'oss.ts' -p 'api' | xargs -I{} sh -c 'echo "== {} =="; cat "{}"'Repository: InsForge/CLI
Length of output: 4651
Harden optional-endpoint suppression to avoid masking real backend failures
CLIError only carries exitCode and optional code (no HTTP/status field), so isOptionalEndpointUnsupported can’t truly gate on 404 via status today. Tighten the suppression logic to rely primarily on err.code === 'NOT_FOUND' and drop/replace broad message substring matches like not found / not available / not enabled to prevent unrelated non-404 errors that happen to include those phrases from being silently swallowed.
🤖 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/lib/config-state.ts` around lines 52 - 62, isOptionalEndpointUnsupported
currently uses broad substring checks that can mask real backend failures;
update it to rely primarily on the CLIError code check and remove the loose
message matches. Modify the function isOptionalEndpointUnsupported to first
confirm err is a CLIError and then return true only when err.code ===
'NOT_FOUND' (you may keep a very specific exact check like message.includes('oss
request failed: 404') if needed), but remove the generic message.includes checks
for 'not found', 'not available', and 'not enabled' so unrelated errors are not
suppressed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 18 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/lib/config-capabilities.ts">
<violation number="1" location="src/lib/config-capabilities.ts:132">
P1: Don't gate email-template support on an existing matching template. An empty `/api/auth/email-templates` list should still count as supported, otherwise first-time templates get skipped.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return slice !== undefined && slice !== null && typeof slice === 'object' && key in slice; | ||
| } | ||
|
|
||
| function hasEmailTemplate(state: RawConfigState | undefined, templateType: string): boolean { |
There was a problem hiding this comment.
P1: Don't gate email-template support on an existing matching template. An empty /api/auth/email-templates list should still count as supported, otherwise first-time templates get skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/config-capabilities.ts, line 132:
<comment>Don't gate email-template support on an existing matching template. An empty `/api/auth/email-templates` list should still count as supported, otherwise first-time templates get skipped.</comment>
<file context>
@@ -82,11 +106,36 @@ export function metadataSupports(raw: RawMetadata, change: DiffChange): boolean
+ return slice !== undefined && slice !== null && typeof slice === 'object' && key in slice;
+}
+
+function hasEmailTemplate(state: RawConfigState | undefined, templateType: string): boolean {
+ return (
+ Array.isArray(state?.emailTemplates) &&
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/config-toml.test.ts (1)
300-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetention normalization expectation conflicts with the stated contract.
This test expects
retention_days = 0to parse as0, but the PR contract says TOML0should normalize to internalnull. The current assertion will lock in the wrong behavior.Expected assertion shape
expect(parseConfigToml(toml)).toEqual({ storage: { max_file_size_mb: 100 }, - realtime: { retention_days: 0 }, + realtime: { retention_days: null }, schedules: { retention_days: 30 }, });🤖 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/lib/config-toml.test.ts` around lines 300 - 315, The test "parses storage and retention sections" asserts that a TOML retention_days = 0 yields numeric 0, but the contract for parseConfigToml states TOML 0 should be normalized to internal null; update the test expectation to reflect that normalization (e.g., expect realtime.retention_days to be null) and adjust any other retention assertions (schedules if applicable) so they match parseConfigToml's normalization behavior rather than asserting raw 0 values.src/lib/config-toml.ts (1)
161-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid serializing
retention_days: undefinedas0.Using
'retention_days' in configplus?? 0converts explicitundefinedinto0(disabled), which changes semantics from “unset” to “set”.Patch
function renderRetentionFields(config: RetentionConfig, lines: string[]): void { - if ('retention_days' in config) { + if (config.retention_days !== undefined) { // TOML has no null literal; 0 is our explicit "disabled" spelling. lines.push(`retention_days = ${config.retention_days ?? 0}`); } }🤖 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/lib/config-toml.ts` around lines 161 - 165, The code in renderRetentionFields currently uses `'retention_days' in config` plus `?? 0`, which serializes an explicit undefined into 0 and changes semantics; update the condition to only write the field when it is present and not undefined (e.g., check `('retention_days' in config && config.retention_days !== undefined)`) and remove the `?? 0` so you emit the actual numeric value when set, leaving the key absent when unset.
🧹 Nitpick comments (1)
src/lib/config-toml.test.ts (1)
329-343: ⚡ Quick winAdd deterministic ordering assertions for the new sections.
This test checks section presence but not the required fixed order, so ordering regressions could slip through.
Example assertions
const out = stringifyConfigToml(original); +const authIdx = out.indexOf('[auth]'); +const storageIdx = out.indexOf('[storage]'); +const realtimeIdx = out.indexOf('[realtime]'); +const schedulesIdx = out.indexOf('[schedules]'); +expect(authIdx).toBeGreaterThanOrEqual(0); +expect(storageIdx).toBeGreaterThan(authIdx); +expect(realtimeIdx).toBeGreaterThan(storageIdx); +expect(schedulesIdx).toBeGreaterThan(realtimeIdx);🤖 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/lib/config-toml.test.ts` around lines 329 - 343, The test currently verifies presence of sections but not their order; update the test around stringifyConfigToml and parseConfigToml to assert a deterministic section ordering by checking the string positions (e.g., use out.indexOf('[storage]') < out.indexOf('[realtime]') < out.indexOf('[schedules]') or equivalent comparisons) so ordering regressions fail the test while keeping the round-trip equality check intact.
🤖 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/lib/config-toml.ts`:
- Around line 24-27: The export lost rendering of auth.email_templates; add a
renderer (e.g., renderAuthEmailTemplates or include logic in
renderAuthSubTables) that serializes each template as its own TOML table named
[auth.email_templates."<type>"] and invoke it when building the auth section in
the deterministic order used by renderConfig (ensure email_templates is rendered
in the auth sub-table sequence between auth.password and auth.smtp as
described); reference the auth-related functions/variables in this file
(renderAuthSection / renderAuthSubTables / renderConfig or similarly named
helpers) and include the email template rendering call so exports include those
tables again.
---
Outside diff comments:
In `@src/lib/config-toml.test.ts`:
- Around line 300-315: The test "parses storage and retention sections" asserts
that a TOML retention_days = 0 yields numeric 0, but the contract for
parseConfigToml states TOML 0 should be normalized to internal null; update the
test expectation to reflect that normalization (e.g., expect
realtime.retention_days to be null) and adjust any other retention assertions
(schedules if applicable) so they match parseConfigToml's normalization behavior
rather than asserting raw 0 values.
In `@src/lib/config-toml.ts`:
- Around line 161-165: The code in renderRetentionFields currently uses
`'retention_days' in config` plus `?? 0`, which serializes an explicit undefined
into 0 and changes semantics; update the condition to only write the field when
it is present and not undefined (e.g., check `('retention_days' in config &&
config.retention_days !== undefined)`) and remove the `?? 0` so you emit the
actual numeric value when set, leaving the key absent when unset.
---
Nitpick comments:
In `@src/lib/config-toml.test.ts`:
- Around line 329-343: The test currently verifies presence of sections but not
their order; update the test around stringifyConfigToml and parseConfigToml to
assert a deterministic section ordering by checking the string positions (e.g.,
use out.indexOf('[storage]') < out.indexOf('[realtime]') <
out.indexOf('[schedules]') or equivalent comparisons) so ordering regressions
fail the test while keeping the round-trip equality check intact.
🪄 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: 9b792252-4457-43c1-aca6-6f6bf436e6df
📒 Files selected for processing (14)
README.mdsrc/commands/config/apply.test.tssrc/commands/config/apply.tssrc/commands/config/export.test.tssrc/lib/config-capabilities.test.tssrc/lib/config-capabilities.tssrc/lib/config-diff.test.tssrc/lib/config-diff.tssrc/lib/config-metadata.test.tssrc/lib/config-metadata.tssrc/lib/config-schema.tssrc/lib/config-state.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
💤 Files with no reviewable changes (9)
- src/lib/config-diff.test.ts
- src/lib/config-capabilities.test.ts
- src/commands/config/apply.ts
- src/commands/config/apply.test.ts
- src/lib/config-metadata.test.ts
- src/commands/config/export.test.ts
- src/lib/config-diff.ts
- src/lib/config-schema.ts
- src/lib/config-capabilities.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Code review — Expand declarative config apply coverage
Summary: Cleanly factored extension of config apply/plan/export to four new keys (auth.disable_signup, storage.max_file_size_mb, realtime.retention_days, schedules.retention_days) via a shared loadConfigState loader, well-tested at the unit level — but the "skip unsupported backends gracefully" path is fragile and effectively untested, which can regress the existing config commands on older backends.
Requirements context: No matching spec/plan found under docs/specs/ (the only specs there cover the diagnose and db-migrations commands) — assessed against the PR description and the surrounding code conventions alone. The PR description states two load-bearing requirements I held the code to: (a) "missing or unavailable endpoints are treated as absent and skipped gracefully" and (b) "keep unsupported backend versions on the existing skip path."
Critical
1. [functionality / software-engineering] The unsupported-endpoint detection is fragile and untested — it can break apply/plan/export on backends that lack the new endpoints. — src/lib/config-metadata.ts:70-110
loadConfigState unconditionally probes /api/storage/config, /api/realtime/config, and /api/schedules/config on every invocation (even an auth-only apply), and relies on isOptionalEndpointUnsupported to swallow the "endpoint doesn't exist" error. That guard is unreliable:
err.code === 'NOT_FOUND'is a dead branch.ossFetchonly ever throwsnew CLIError(message)with nocodeargument (src/lib/api/oss.ts:161,src/lib/errors.ts:3-12), soerr.codeis alwaysundefinedhere. Detection therefore rests entirely on the message-substring checks.- The substring checks miss the most likely route-miss shape.
ossFetchbuildsmessage = err.message ?? err.error ?? "OSS request failed: ${status}"(oss.ts:135). This file's ownisRouteLevel404 = !err.error || err.error === 'NOT_FOUND'(oss.ts:144) shows the backend returns{ "error": "NOT_FOUND" }for route-level 404s. When such a body has nomessagefield,messagebecomes the literal"NOT_FOUND"→ lowercased"not_found", which matches none of'oss request failed: 404','not found','not available','not enabled'(note the underscore vs. space). The error then propagates and the whole command fails. - Because the probe is unconditional, this isn't limited to the new sections: a user who upgrades the CLI but whose backend predates these endpoints would find
config export/plan/applycompletely broken, even when theirinsforge.tomlonly touches auth — a regression of existing behavior and a direct violation of requirements (a) and (b) above. - No test exercises the catch path. Every test mock (
apply.test.ts,export.test.ts,plan.test.ts) returns HTTP 200 — the "unsupported" cases simulate absence via an empty{}200 body, never a 404 throw. SofetchOptionalJson/isOptionalEndpointUnsupported— the exact fragile logic — has zero coverage.
Recommended fix: detect by HTTP status, not message text. Either have ossFetch attach the status code to the CLIError (it already reads res.status / err.statusCode) and check err.statusCode === 404 in fetchOptionalJson, or do a raw fetch in loadConfigState and branch on res.status === 404. Then add a regression test where the optional endpoint throws a 404 CLIError in both the { "error": "NOT_FOUND" } and empty-body shapes (see Suggestion 4).
Suggestion
2. [functionality / performance] Probe only the endpoints the operation needs. — src/lib/config-metadata.ts:70-88
loadConfigState fetches all three optional slices on every apply/plan, regardless of which sections the TOML contains. For an auth-only apply that's three wasted round-trips (they're parallelized via Promise.all, which is good, so it's latency not serialization), and it widens the blast radius of the Critical above. Consider probing only for sections present in file for apply/plan. export legitimately needs all three.
3. [functionality] Confirm the HTTP verbs match the backend routes. — src/commands/config/apply.ts:247-266
storage applies with PUT, while realtime/schedules use PATCH. That may be correct, but the asymmetry is worth a confirming note: the capability probe is a GET, so a write-verb mismatch (e.g. PUT to a PATCH-only route) wouldn't be caught by the gate and would only fail at apply time.
4. [software-engineering] Add a regression test for the graceful-skip path. A test where an optional endpoint throws a 404 CLIError (both the structured { "error": "NOT_FOUND" } and the no-body "OSS request failed: 404" shapes) would lock in the behavior the PR promises and would have caught the Critical. Current tests only cover the 200-with-empty-body case.
Information
5. [functionality] max_file_size_mb upper bound is hard-coded to 200. — src/lib/config-schema.ts (validateStorage)
If the backend's actual cap differs or changes, the CLI will reject otherwise-valid configs client-side. Consider documenting where 200 comes from, or letting the backend be the authority on the limit.
6. [functionality] retention_days = 0 is overloaded to mean "disable cleanup" (wire null). — src/lib/config-diff.ts (normalizeRetentionDays), README
Reasonable given TOML has no null literal, and it's documented in the README and code comments — just noting that 0 reads naturally as "delete immediately / keep 0 days," so the overload is a mild footgun. No change requested.
7. [software-engineering] Minor indentation drift in the reformatted expected object in src/lib/config-metadata.test.ts (around the password/disable_signup/smtp block) — valid but inconsistently indented. Cosmetic.
Security
No security-relevant concerns. New writes go through the existing ossFetch (Bearer api_key); all new TOML inputs are validated with type/range checks (config-schema.ts); analytics records only section path names, not values, so no secrets/PII are newly logged; no new dependencies. SMTP/secret handling is unchanged.
Performance
No N+1 or event-loop blocking. The only note is the three unconditional optional-endpoint probes per invocation (parallelized) — see Suggestion 2.
Verdict: request_changes
One Critical item (the fragile, untested unsupported-endpoint detection that can regress existing config commands on older backends). The rest of the change is well-structured and the unit-test coverage for the happy paths is solid. Resolving the Critical — status-based 404 detection plus a regression test — should make this mergeable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/config-state-loader.test.ts`:
- Around line 24-36: The test currently only checks the return value of
loadConfigState() which can mask removals of endpoint probes; update the test to
assert that ossFetchMock was actually invoked for the expected endpoints by
adding assertions that ossFetchMock was called with '/api/metadata' and
'/api/storage/config' (and any other optional probe paths your implementation
should hit) and that the sequence of calls equals the expected list of probe
paths (compare ossFetchMock.mock.calls to the expected array) so the test fails
if loadConfigState() stops calling those endpoints; reference loadConfigState
and ossFetchMock when adding these assertions.
🪄 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: a6552482-f7a0-4a5e-bf73-41d05416b76c
📒 Files selected for processing (3)
src/commands/config/plan.test.tssrc/lib/config-metadata.tssrc/lib/config-state-loader.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/config/plan.test.ts
- src/lib/config-metadata.ts
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: Expand declarative config apply coverage
Summary: Solid, well-factored extension of config apply/plan/export to four new declarative keys, but the new "optional endpoint" 404 detection matches a message string that real InsForge backends never emit, which will make all three commands throw against the exact older backends the graceful-skip path is meant to support.
Requirements context
No /docs/superpowers/ directory exists in this repo, and nothing under docs/specs/ (the three existing specs cover diagnose and db migrations) matches declarative config apply — so I assessed against the PR description/body and the repo's DEVELOPMENT.md + .claude/skills/cli-development conventions, plus the existing config-* code as the source of intent.
🔴 Critical
[Functionality] Optional-endpoint 404 detection matches a string the backend never sends → plan/apply/export crash on older backends — src/lib/config-metadata.ts:98-102, src/lib/api/oss.ts:127-161
fetchOptionalJson swallows a 404 only when isOptionalEndpointUnsupported(err) is true, which requires the CLIError.message to equal oss request failed: 404 (or start with oss request failed: 404\n):
return message === 'oss request failed: 404' || message.startsWith('oss request failed: 404\n');But that string is the CLI's client-side fallback, produced by ossFetch only when the 404 body has no message and no error field (oss.ts:135 — err.message ?? err.error ?? 'OSS request failed: ${res.status}'). A real InsForge route-level 404 returns a JSON body { error: 'NOT_FOUND' } — this is pinned by the repo's own existing test src/lib/api/oss.test.ts:69 and by the route-level handling in oss.ts:144 (const isRouteLevel404 = !err.error || err.error === 'NOT_FOUND'). For that body, ossFetch builds CLIError('NOT_FOUND'), so isOptionalEndpointUnsupported sees 'not_found', returns false, and the error propagates out of loadConfigState — failing the whole command.
Impact: before this PR, plan/apply/export hit only /api/metadata (always present). This PR adds three probes (/api/storage/config, /api/realtime/config, /api/schedules/config) to every invocation (config-metadata.ts:74-78). Any backend lacking even one of them now throws instead of skipping — a regression that directly contradicts the PR's stated behavior ("missing or unavailable endpoints are treated as absent and skipped gracefully" / "keep unsupported backend versions on the existing skip path"). The graceful path only fires for a bare-bodied/non-JSON 404 (e.g. an nginx/proxy 404), not the application's own NOT_FOUND response.
Recommended fix: stop string-matching the rendered message. ossFetch already parses statusCode (oss.ts:131-132) and knows res.status — attach the HTTP status to CLIError and gate on err.status === 404 in isOptionalEndpointUnsupported. (Cheap and also removes the brittleness flagged below.) Please also verify the actual 404 body shape these three endpoints return on a pre-feature backend before merging.
🟡 Suggestion
[Software engineering / testing] The graceful-skip test feeds an error the backend can't produce — src/lib/config-state-loader.test.ts:24-36
The "treats route-level missing optional endpoints as unsupported" test throws new CLIError('OSS request failed: 404'), i.e. the synthetic fallback, so it passes while the real {error:'NOT_FOUND'} path (the Critical above) goes untested — the test gives false green on the central new behavior. Add a case that drives a realistic route-level 404, ideally end-to-end through ossFetch's error mapping (mock fetch returning Response(JSON.stringify({error:'NOT_FOUND'}), {status:404}), as oss.test.ts:69 does) rather than hand-constructing the CLIError. That test would have caught this.
[Project convention] Agent-skills sync not mentioned — DEVELOPMENT.md §3
This PR adds a new user-facing declarative surface (auth.disable_signup, storage.max_file_size_mb, realtime.retention_days, schedules.retention_days). DEVELOPMENT.md §3 asks that command/flag/surface changes be mirrored in the insforge-cli skill in InsForge/agent-skills and that the skill PR be cross-referenced here. README is updated (good), but there's no mention of an agent-skills update — please confirm whether the skill needs the new keys, or note that it doesn't.
🔵 Information
[Design] Root cause of the brittle matching: CLIError carries no HTTP status — src/lib/api/oss.ts:127-161
statusCode is read off the body at oss.ts:131-132 and res.status is in scope, but neither is attached to the thrown CLIError, forcing consumers to recover intent by string-matching .message. This same pattern already appears in config-secrets.ts (matching for SECRET_NOT_FOUND). Carrying status on CLIError would let all of them gate on a code instead of a localized message — worth doing alongside the Critical fix.
[Functionality] asNumber silently drops non-integer values — src/lib/config-metadata.ts:223-229
asNumber/asRetentionDays require Number.isInteger, so a backend that returns maxFileSizeMb/retentionDays as a float or numeric string degrades to "unsupported/skipped" rather than surfacing a parse warning. Low blast radius given the integer-only schema, but worth a comment so it isn't read as a capability gap later.
[Consistency] Mixed HTTP verbs across the new endpoints — src/commands/config/apply.ts:247-266
storage uses PUT /api/storage/config while realtime/schedules use PATCH /api/{realtime,schedules}/config. Presumably matches each backend route's contract — just flagging to confirm it's intentional, not a copy-paste drift.
Dimension coverage notes
- Security: No findings. SMTP password stays an
env()ref, is force-resent and resolved at PUT time, and is never logged or round-tripped into TOML (config-diff.ts:449-465,apply.ts:229-237); the newdisable_signuptoggle is capability-gated like the rest; analytics properties stay limited to counts/section names perDEVELOPMENT.md§2. New endpoints reuse the existing Bearer-keyossFetchpath — no new injection surface. - Performance: No findings. The three new probes run via
Promise.allafter/api/metadata(config-metadata.ts:74-78); +3 parallel requests per invocation, documented in the PR body, no N+1 or hot-path concern.
Verdict: request_changes
One Critical (the 404-detection regression) blocks merge. The structure, type-safety (exhaustive DiffChange checks), round-trip discipline, and the rest of the test coverage are good — once the optional-endpoint detection is keyed to the HTTP status (and a realistic 404 test is added), this should be in good shape.
There was a problem hiding this comment.
1 issue found across 13 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/plan.ts">
<violation number="1" location="src/commands/config/plan.ts:107">
P3: Optional-endpoint fetch/error handling is duplicated across plan/apply/export instead of being centralized, increasing divergence risk.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }); | ||
| } | ||
|
|
||
| async function fetchOptionalConfig<T>(path: string): Promise<T | undefined> { |
There was a problem hiding this comment.
P3: Optional-endpoint fetch/error handling is duplicated across plan/apply/export instead of being centralized, increasing divergence risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/config/plan.ts, line 107:
<comment>Optional-endpoint fetch/error handling is duplicated across plan/apply/export instead of being centralized, increasing divergence risk.</comment>
<file context>
@@ -79,3 +103,21 @@ export function registerConfigPlanCommand(cfg: Command): void {
});
}
+
+async function fetchOptionalConfig<T>(path: string): Promise<T | undefined> {
+ try {
+ const res = await ossFetch(path);
</file context>
|
Addressed the review items in
I saw the new cubic P3 about centralizing the duplicated optional-endpoint helpers; I am intentionally leaving those local for now to keep this PR minimal and avoid reintroducing a wrapper abstraction after reverting the state-loader approach. |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: Expand declarative config apply coverage
Summary: Cleanly extends insforge.toml to cover auth.disable_signup, storage.max_file_size_mb, realtime.retention_days, and schedules.retention_days, with consistent capability-gating and solid test coverage — no blocking issues.
Requirements context
No /docs/superpowers/ directory exists in this repo, and the specs under docs/specs/ (diagnose, db-migrations) do not match this PR's scope — no matching spec/plan found; assessed against the PR description, the existing config-apply architecture, and DEVELOPMENT.md conventions. The change is well-scoped: every touched line serves the four stated new sections plus the supporting CLIError status/code plumbing.
Critical
(none)
No correctness bugs, security holes, data-loss risks, or broken existing behavior found in the new code.
Suggestions
1. Software engineering — duplicated optional-endpoint helpers (DRY).
fetchOptionalConfig<T>() and isMissingOptionalEndpoint() are copied verbatim into three files: src/commands/config/apply.ts:190-206, src/commands/config/plan.ts:107-123, and src/commands/config/export.ts:114-130. Since EndpointConfigResponses and the metadata projection already live in src/lib/config-metadata.ts, hoisting these two helpers into a shared module would keep the route-level-404 detection logic in one place (it must stay in sync with the isRouteLevel404 rule in src/lib/api/oss.ts:144). Non-blocking, but it's three copies of a security/compat-sensitive predicate.
2. Performance — optional config fetches are sequential in apply/plan.
export.ts:63-67 fetches the three optional config slices with Promise.all, but apply.ts:51-65 and plan.ts:43-57 await them one at a time. When a TOML declares all of storage + realtime + schedules, that's three sequential round-trips that could be parallelized like export does. Low blast radius (cold CLI path, ≤3 calls, gated on section presence), but worth aligning for consistency.
3. Software engineering — new schema validators are only covered indirectly.
validateStorage and validateRetentionSection (src/lib/config-schema.ts:116-163) have no dedicated config-schema.test.ts. They are exercised through parseConfigToml in config-toml.test.ts (the 201 over-max and -1 negative rejection cases are good), but the valid boundary values (max_file_size_mb = 1 and = 200, retention_days = null literal) aren't directly asserted. Coverage is reasonable as-is; adding the boundaries would harden the range checks against future edits.
Information
1. Functionality — EMPTY_STORAGE_CONFIG.max_file_size_mb = 50 default (src/lib/config-diff.ts:524-526). This fallback only surfaces as the from value in plan output for backends that don't expose /api/storage/config — and in exactly that case the change is capability-skipped, so it never drives an actual PUT. Harmless, and it mirrors the existing EMPTY_PASSWORD_POLICY pattern; just be aware the displayed from: 50 is a CLI-side guess, not a real backend read, when the section is unsupported.
2. Software engineering — CLIError widening is safely additive (src/lib/errors.ts:8, src/lib/api/oss.ts:161). ossFetch now passes err.error as code and res.status as statusCode on every thrown CLIError (previously both were undefined). I checked the codebase: all other .code === / .statusCode reads are on Node ErrnoExceptions (ENOENT/EEXIST/numeric exit codes), never on ossFetch's CLIError, so no existing branch changes behavior. The --json error output now also carries the backend's error code, which is an improvement.
3. Security — no concerns. The new sections carry only integers/booleans; no new user input reaches SQL/shell. SMTP secret handling (env() refs, force-resend, never round-tripping the value) is pre-existing and untouched. Analytics sections_changed records only section.key paths (changePath), never values — no PII/secret leakage.
4. Round-trip correctness verified. The retention_days = 0 ⇄ wire-null mapping (normalizeRetentionDays, config-diff.ts:420-424; renderRetentionFields, config-toml.ts:161-166) is symmetric and tested, and config-format.ts's generic formatChange fallback (:61) renders the new variants without modification. The 0 disables retention semantics is documented in the README.
Verdict
approved (informational — a human still gives the explicit GitHub approval). Zero Critical findings. The implementation matches the PR's stated scope, capability-gates each new section independently, handles optional-endpoint 404s correctly, and is backed by thorough apply/plan/export/diff/metadata tests. The suggestions above are non-blocking polish.
Fermionic-Lyu
changed the title
2 participants
Summary
Not included
Related
Test plan
Note
Summary
Not included
Related
Test plan
Changes since #156 opened