feat(cli): add auth verification + password policy to insforge.toml#127
Conversation
WalkthroughAdds a new metadata mapping module that projects raw /api/metadata into CLI LiveConfig and InsforgeConfig shapes, expands auth config (verification flags, reset/verify methods, password policy), wires those projections into config plan/export/apply flows, and updates diffs, TOML parsing/stringifying, capability probing, and tests accordingly. ChangesAuth + Metadata projection and CLI config flow
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
Greptile SummaryThis PR extends
Confidence Score: 5/5Safe to merge; all new auth/password fields route through well-tested, capability-gated code paths with correct wire-key mappings and defensive null handling. The centralization of metadata projection into config-metadata.ts is clean and the new per-field diff/apply/export logic is thorough and well-tested (343 passing tests). The only finding is a type-level inaccuracy in RawAuthMetadata.smtpConfig — the runtime handling via isPlainObject() is correct everywhere the field is read. src/lib/config-metadata.ts — the smtpConfig nullable type gap is worth fixing before this module grows further. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI
participant apply.ts
participant plan.ts
participant export.ts
participant config-metadata.ts
participant Backend
CLI->>Backend: GET /api/metadata
Backend-->>CLI: RawMetadataResponse
alt config apply
CLI->>apply.ts: applyCommand()
apply.ts->>config-metadata.ts: liveFromMetadata(raw)
config-metadata.ts-->>apply.ts: LiveConfig
apply.ts->>apply.ts: "diffConfig({live, file})"
loop each DiffChange (capability-gated)
apply.ts->>Backend: "PUT /api/auth/config {field: value}"
end
else config plan
CLI->>plan.ts: planCommand()
plan.ts->>config-metadata.ts: liveFromMetadata(raw)
config-metadata.ts-->>plan.ts: LiveConfig
plan.ts->>plan.ts: "diffConfig({live, file})"
plan.ts-->>CLI: formatted plan output
else config export
CLI->>export.ts: exportCommand()
export.ts->>config-metadata.ts: configFromMetadata(raw)
config-metadata.ts-->>export.ts: "{config, skipped[]}"
export.ts-->>CLI: insforge.toml written
end
Reviews (5): Last reviewed commit: "fix(config-metadata): shape-guard custom..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Code Review — feat(cli): add auth verification + password policy to insforge.toml
Summary: Well-structured extension of the config-as-code surface — per-key gating, default-keep semantics, and the liveFromMetadata centralisation are all solid design calls. No blocking issues found.
Requirements context
No /docs/superpowers/ directory exists in this repo. Review assessed against the PR description, stacked-on PR #123, and the inline design notes.
Findings
Critical
(none)
Suggestion
[Software Engineering] config-metadata.ts has no dedicated unit tests
liveFromMetadata (src/lib/config-metadata.ts:52-113) is the new single source of truth that both apply.ts and plan.ts route through — and the PR description correctly calls out that the old plan.ts had a latent bug that this fixes. Yet the 30 new tests are spread across config-diff, config-toml, config-capabilities, and export; none target liveFromMetadata directly.
The function has non-trivial edge cases:
- Presence-based detection of the password block (
'passwordMinLength' in a || 'requireNumber' in a || …) - Type-narrowing guards on
verifyEmailMethod/resetPasswordMethodthat silently fall back when the value is not'code' | 'link' - The per-field
?? defaultfallback for partial backend exposure
A dedicated config-metadata.test.ts covering at least: full payload, partial auth (password only), legacy backend (no auth fields), and an unexpected verifyEmailMethod value would lock in the correctness guarantee the PR claims.
[Functionality] export.ts does not call liveFromMetadata — the centralisation is incomplete for export
apply.ts and plan.ts both now route through liveFromMetadata. export.ts (src/commands/config/export.ts:56-134) has its own parallel projection. This is semantically defensible (export builds a TOML file rather than a diff baseline), but the PR description says "apply/plan/export agree on the live shape" — which is only two-thirds true.
The practical risk: if a field mapping is corrected in liveFromMetadata, the export path won't benefit automatically. The field-presence semantics between the two paths are consistent right now, but they could silently diverge as new fields are added. Worth at least a comment in export.ts noting the intentional divergence, or a future consolidation task.
[Functionality] Unknown enum values from a future backend are silently skipped and misreported
In both export.ts:69-80 and liveFromMetadata (config-metadata.ts:66-77), a verifyEmailMethod or resetPasswordMethod value that is not 'code' or 'link' causes the field to be treated as absent:
// export.ts
if (authObj && 'verifyEmailMethod' in authObj &&
(authObj.verifyEmailMethod === 'code' || authObj.verifyEmailMethod === 'link')) {
config.auth.verify_email_method = authObj.verifyEmailMethod;
} else {
skipped.push('auth.verify_email_method'); // ← also fires for unexpected values
}If a future backend ships a new method value (e.g. 'otp'), export would list the field as skipped (misleading — the backend does support it), and liveFromMetadata would leave verify_email_method as undefined, causing diffConfig to default the live value to 'code'. A user with verify_email_method = 'link' in their TOML would then see a false diff and apply would overwrite the backend's actual value.
Low probability today, but it's worth an inline comment explaining why unknown values fall back rather than being passed through, so a future maintainer adding a new enum value knows to update the CLI first.
Information
[Software Engineering] Exhaustiveness check is only in applyChange, not in metadataSupports
applyChange correctly ends with const _exhaustive: never = change (apply.ts:215). metadataSupports (config-capabilities.ts:42-83) ends with return false for unrecognised changes. Both are correct given their contracts (false-safe fallback for capabilities, hard error for apply). Just noting for visibility — the asymmetry is intentional and fine.
[Performance] Sequential PUTs for multi-field password policy
A TOML block that changes all five [auth.password] fields issues five sequential PUT /api/auth/config requests. These are administrative, low-frequency operations, so this is not a problem in practice. If latency ever becomes noticeable, the backend's partial-body PUT already supports sending multiple fields in one call — the CLI could batch them. Not worth changing now.
Verdict
approved (informational — no Critical findings). The implementation is correct, the capability-gating model is coherent, and the test coverage for the new TOML surface is solid. The two Suggestion items (missing config-metadata.ts unit tests and the incomplete centralisation in export) are the most actionable follow-ups before this pattern scales to more fields.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 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/export.ts">
<violation number="1" location="src/commands/config/export.ts:52">
P2: `configFromMetadata(raw)` can crash on malformed metadata because it assumes `raw.auth` is an object before using `'in'` checks. This makes `config export` less robust against unexpected API payloads.</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.
Extends config-as-code to cover the auth.config row beyond redirect URLs: - [auth] adds require_email_verification + verify_email_method + reset_password_method as direct keys - [auth.password] block: min_length + require_number / require_lowercase / require_uppercase / require_special_char Per-key DiffChange variants give true default-keep semantics — a partial [auth.password] block only touches the fields it names. All eight new fields route through the existing PUT /api/auth/config endpoint with per-key idempotent PUTs and per-field capability gating, so a backend that exposes them piecemeal still applies the supported subset. Also extracts liveFromMetadata into a shared lib/config-metadata.ts so apply / plan / export agree on the live shape — fixes a latent plan.ts bug that only read allowedRedirectUrls. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Use 'key' in fileAuth presence-check for verify_email_method and reset_password_method, matching the established pattern (truthy check happens to work today but would silently misbehave on a hypothetical future enum value) - Extract configFromMetadata into config-metadata.ts so export.ts shares the same field-presence detection as liveFromMetadata — adding a new field now touches one place rather than risking silent drift - Add config-metadata.test.ts covering full / partial / legacy / unknown- enum response shapes for both functions - Add inline comment explaining why unknown enum values fall back to "skipped" (parser would reject them on next apply, so we can't round-trip them) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ents cubic flagged that configFromMetadata could crash on a malformed metadata response — `'key' in primitive` throws TypeError, so a backend bug returning auth: "string" would take down `config export`. Guard both slices through isPlainObject() before reading any key, treating wrong-shaped input as "field not exposed" so the command degrades gracefully. Same guard applied to liveFromMetadata for symmetry; both paths now have test coverage for string/number/null/array auth slices. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bind raw.deployments to a local before the predicate check so TS can narrow it past `... | undefined`. Constrain the helper's type parameter to `extends object` so callers without an explicit type context still get a useful narrowing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
tonychang04
force-pushed
the
feat/insforge-toml-auth-policy
branch
from
0663ddf to
8b22aa2
Compare
![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-metadata.test.ts (1)
95-103: ⚡ Quick winAdd malformed inner-field regression cases for metadata projection.
Current malformed tests only cover non-object
auth. Add cases likeallowedRedirectUrls: 123(and similar wrong inner types) to lock defensive behavior and prevent crash regressions.Also applies to: 187-194
🤖 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-metadata.test.ts` around lines 95 - 103, Add additional malformed-inner-field cases to the metadata projection tests so non-object inner fields don't crash; in the test that calls liveFromMetadata (the 'treats a malformed auth slice as absent...' case) include examples where object fields contain wrong primitive types (e.g., { auth: { allowedRedirectUrls: 123 } }, { auth: { clientIds: "bad" } } etc.) and assert the function returns the safe projected form (e.g., auth: { allowedRedirectUrls: [], clientIds: [] } or an empty object as appropriate). Repeat the same pattern for the other test block that uses liveFromMetadata referenced later (the similar test around lines 187-194) so both slices and inner fields are guarded against malformed types. Ensure assertions reflect the expected defensive defaults from the projection logic.
🤖 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-metadata.ts`:
- Around line 65-67: The assignment to live.auth!.allowed_redirect_urls in
src/lib/config-metadata.ts trusts a.allowedRedirectUrls shape and can crash
later; update the logic (both where allowedRedirectUrls is set at the shown
block and the similar block at lines 149-152) to validate that
a.allowedRedirectUrls is an array of strings before assigning it to
live.auth!.allowed_redirect_urls (e.g., check Array.isArray and typeof elements
=== 'string'), and otherwise assign a safe fallback (empty array or undefined)
so downstream calls like normalizeUrlList/TOML rendering won't throw.
---
Nitpick comments:
In `@src/lib/config-metadata.test.ts`:
- Around line 95-103: Add additional malformed-inner-field cases to the metadata
projection tests so non-object inner fields don't crash; in the test that calls
liveFromMetadata (the 'treats a malformed auth slice as absent...' case) include
examples where object fields contain wrong primitive types (e.g., { auth: {
allowedRedirectUrls: 123 } }, { auth: { clientIds: "bad" } } etc.) and assert
the function returns the safe projected form (e.g., auth: { allowedRedirectUrls:
[], clientIds: [] } or an empty object as appropriate). Repeat the same pattern
for the other test block that uses liveFromMetadata referenced later (the
similar test around lines 187-194) so both slices and inner fields are guarded
against malformed types. Ensure assertions reflect the expected defensive
defaults from the projection logic.
🪄 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: 054afdfb-5de8-4333-8766-94fce0c655d7
📒 Files selected for processing (13)
src/commands/config/apply.tssrc/commands/config/export.test.tssrc/commands/config/export.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-metadata.test.tssrc/lib/config-metadata.tssrc/lib/config-schema.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
|
@cubic-dev-ai please re-review after rebase onto main + minor narrowing fix |
@tonychang04 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 13 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-metadata.ts">
<violation number="1" location="src/lib/config-metadata.ts:214">
P2: `configFromMetadata` incorrectly marks `auth.smtp` as unsupported when `smtpConfig` is present but null; this diverges from the repository’s presence-based capability checks and can produce incorrect export/skipped output.</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.
CodeRabbit (Major) flagged that allowedRedirectUrls was assigned without shape validation — a backend returning a string instead of an array would crash normalizeUrlList on the diff path. Coerce non-array values to [] in both projections via a new asStringArray helper; wrong-shaped values still count as "supported" since the key was present. cubic (P2) flagged that configFromMetadata used a truthy check on smtpConfig, so smtpConfig: null (legitimate "supported but blank" signal) was incorrectly reported as unsupported. Switch to a presence- based check matching metadataSupports() in config-capabilities.ts: emit the [auth.smtp] block only when there's real data, but keep the field out of the skipped[] list when the key exists with a null value. liveFromMetadata gets the same isPlainObject guard on smtpConfig for symmetry — the diff layer's empty-state defaults already cover the no-row case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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-metadata.ts`:
- Around line 126-128: liveFromMetadata() currently assigns
live.deployments.subdomain directly from raw.deployments.customSlug which can be
a non-string; validate that raw.deployments (d) has a string and non-empty
customSlug before assigning. Change the assignment in liveFromMetadata so it
sets live.deployments = { subdomain: typeof d.customSlug === 'string' &&
d.customSlug.trim() ? d.customSlug : null } (mirroring the guard used in
configFromMetadata()), ensuring malformed numeric or empty values degrade to
null.
🪄 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: 485532ee-eb3d-4701-90c4-1be945604d2a
📒 Files selected for processing (2)
src/lib/config-metadata.test.tssrc/lib/config-metadata.ts
CodeRabbit (Major): liveFromMetadata trusted deployments.customSlug
without a type check. A malformed payload like { customSlug: 123 }
would leak a number into live.deployments.subdomain, producing
nonsense diffs against the file's string-typed subdomain. Mirror the
string-shape guard already in configFromMetadata: non-string or empty
values degrade to null.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Well-scoped extension of the config-as-code surface: adds auth verification flags and password policy to insforge.toml, centralises the metadata projection that was previously duplicated across apply/plan/export, and ships with solid test coverage across all the new library layers.
Requirements context
No matching spec/plan found under /docs/superpowers/ — that directory does not exist in this repo. Assessment is against the PR description and linked design notes alone.
Findings
Critical
(none)
Suggestion
[Software Engineering] apply.test.ts not updated for the new dispatch branches — src/commands/config/apply.ts:135-173
The existing apply.test.ts tests allowedRedirectUrls, auth.smtp, and deployments.subdomain by asserting the exact PUT endpoint and body. The four new branches added in this PR (require_email_verification, verify_email_method, reset_password_method, and the auth.password dispatch via authPasswordWireKey) have no equivalent coverage.
The risk: a wire-key typo (e.g. requireEmailVerifcation) or wrong endpoint would pass all existing tests undetected. The authPasswordWireKey path is indirectly validated by config-capabilities.test.ts, but the applyChange call-site that wraps it is not. The test harness is already in place and the pattern is clear — each new branch deserves a test that asserts the PUT body matches the expected camelCase key.
[Software Engineering] Truthy guard after in-check on verify_email_method / reset_password_method — src/lib/config-diff.ts:211,227
if (fileAuth && 'verify_email_method' in fileAuth && fileAuth.verify_email_method) {After 'verify_email_method' in fileAuth, TypeScript narrows fileAuth.verify_email_method to VerificationMethod (non-optional 'code' | 'link'), so the trailing truthy guard is always true — the validator rejects empty strings at parse time. The inline comment acknowledges this awkwardness. Using !== undefined instead would be consistent with the require_email_verification branch immediately above and removes the "happens to work" fragility if VerificationMethod is ever extended.
Information
[Process] PR is stacked on #123 — merge order dependency
The PR description explicitly notes "Merge [#123] first; this is a clean fast-forward after." The base branch is main. Confirm #123 is merged before merging this one, or rebase and update the description.
[Software Engineering] EMPTY_PASSWORD_POLICY defaults must stay in sync with backend authConfigSchema — src/lib/config-diff.ts:418-425
const EMPTY_PASSWORD_POLICY: LivePasswordPolicy = {
min_length: 8,
require_number: false,
...
};The comment already calls this out. A drift between this constant and the backend's actual defaults would cause a spurious diff on every first config plan against a fresh project. Consider adding a pointer to the shared-schemas package version (or a TODO with the issue number) so a future maintainer knows where to look if the backend defaults ever change.
Verdict
approved (informational — explicit GitHub approval is a separate human action)
No critical findings. The two Suggestions are worth addressing before this lands, particularly the apply.test.ts gap, but neither blocks the correctness of the feature as shipped. The centralization of liveFromMetadata / configFromMetadata is a clear win — it closes a real plan false-positive bug and makes future field additions a single-file change.
2 participants
Summary
Extends config-as-code to cover the rest of the auth.config row that the SMTP work depends on — declaratively configure verification flows + password policy.
Stacked on top of #123 (the SMTP / deployments PR). Merge that first; this is a clean fast-forward after.
New TOML surface
```toml
[auth]
require_email_verification = true
verify_email_method = "link" # "code" | "link"
reset_password_method = "code"
[auth.password]
min_length = 12
require_number = true
require_lowercase = true
require_uppercase = true
require_special_char = true
```
Design notes
Drive-by fix
Extracted `liveFromMetadata` into `src/lib/config-metadata.ts` so apply/plan/export agree on the live shape. Fixes a latent bug where `plan.ts` only read `allowedRedirectUrls` from metadata — SMTP/deployments/new-field diffs in the plan output would have shown false positives.
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds auth verification flags and password policy to
insforge.tomlwith per-field diffs and applies. Centralizes metadata handling and hardens parsing to prevent crashes, bogus diffs, and false skips.New Features
require_email_verification,verify_email_method("code" | "link"),reset_password_method("code" | "link").min_length,require_number,require_lowercase,require_uppercase,require_special_char; per-key diffs and idempotent per-fieldPUT /api/auth/configusing a centralized wire-key map; capability-gated via flat/api/metadatakeys.config-metadatato emit only supported fields; unknown enum values are dropped.Bug Fixes
liveFromMetadata+configFromMetadatakeep plan/export/apply in sync and fix plan false positives.allowedRedirectUrlsto[]; treatsmtpConfig: nullas supported-but-empty; shape-guarddeployments.customSlug(non-strings →null) to avoid bogus diffs.verify_email_methodandreset_password_methodavoid silent mismatches when live is missing.Written for commit c4e086e. Summary will update on new commits.
Note
Add auth verification flags and password policy support to
insforge.tomlinsforge.tomlschema withrequire_email_verification,verify_email_method,reset_password_method, and an[auth.password]sub-table (withmin_lengthandrequire_*boolean fields).config plan,config apply, andconfig exportto diff, apply, and emit these new fields by issuingPUT /api/auth/configwith the relevant fields.liveFromMetadataandconfigFromMetadatahelpers in config-metadata.ts, removing duplicated inline implementations from each command.config plannow diffs additional live fields; backends missing these fields default tofalse(verification flag) or'code'(method fields) rather than being ignored.Changes since #127 opened
asStringArrayutility function and updatedliveFromMetadataandconfigFromMetadatafunctions to coerce wrong-shapedallowedRedirectUrlsvalues to empty arrays [78325f8]liveFromMetadataandconfigFromMetadatafunctions to only populate SMTP configuration blocks whensmtpConfigis a plain object, treating null values as absent [78325f8]allowedRedirectUrlscoercion andsmtpConfignull-handling in bothliveFromMetadataandconfigFromMetadatafunctions [78325f8]liveFromMetadatafunction inconfig-metadatato validatedeployments.customSlugtype and value, ensuringdeployments.subdomainis set to null unlesscustomSlugis a non-empty string, and added corresponding test coverage for non-string input scenarios [c4e086e]Macroscope summarized 8b22aa2.
Summary by CodeRabbit
New Features
Tests