feat(cli): add [auth.smtp] section to config-as-code with env() password resolution#123
Open
tonychang04 wants to merge 4 commits into
Open
feat(cli): add [auth.smtp] section to config-as-code with env() password resolution#123tonychang04 wants to merge 4 commits into
tonychang04 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds SMTP email configuration support to the InsForge CLI config system, extending the existing config-as-code pipeline to handle SMTP fields in ChangesSMTP Configuration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/config/apply.ts (1)
102-113:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreflight SMTP secret resolution before applying any change to prevent partial applies.
With mixed batches,
auth.allowed_redirect_urlscan be PUT first, then fail on SMTP secret resolution later. That leaves the run partially applied.Suggested direction
+// 1) Pre-resolve all secret refs for planned SMTP changes before any PUT. +const resolvedSmtpPasswords = new Map<DiffChange, string>(); +for (const change of result.changes) { + if (change.section === 'auth.smtp' && change.passwordEnvRef) { + const value = await resolveEnvRef(`env(${change.passwordEnvRef})`, 'auth.smtp.password'); + resolvedSmtpPasswords.set(change, value); + } +} + // 2) Apply changes only after preflight succeeds. for (const change of result.changes) { ... - await applyChange(change); + await applyChange(change, resolvedSmtpPasswords.get(change)); applied.push(change); }And update
applyChangeto accept an optional pre-resolved password and avoid resolving during mutation.Also applies to: 186-193
🤖 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.ts` around lines 102 - 113, Preflight resolution of SMTP secrets for every change before any mutation: iterate result.changes and for each change whose path may require secrets (e.g., SMTP/password fields) resolve the secret ahead of time and collect failures into skipped so no partial applies occur; then call applyChange(change, preResolvedSecret?) instead of resolving inside applyChange. Update the applyChange function signature to accept an optional preResolvedPassword (or credentials object) and ensure it uses that value if provided and does not attempt resolution during the mutation. Apply the same preflight logic to the second batch processing block that also iterates result.changes (the other loop around applyChange) so both places resolve SMTP secrets before making any PUTs and mark failing resolutions as skipped.
🧹 Nitpick comments (1)
src/lib/config-secrets.test.ts (1)
83-137: ⚡ Quick winAdd a malformed-JSON lookup test for
resolveEnvRef.You’re asserting major failure modes already; adding a
200+ invalid JSON case will lock in stable error behavior for backend/proxy anomalies.Proposed test case
describe('resolveEnvRef', () => { + it('throws SECRET_LOOKUP_FAILED when 200 response is not valid JSON', async () => { + ossFetchMock.mockResolvedValueOnce(new Response('not-json', { status: 200 })); + await expect( + resolveEnvRef('env(SMTP_PASSWORD)', 'auth.smtp.password'), + ).rejects.toMatchObject({ code: 'SECRET_LOOKUP_FAILED' }); + });🤖 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-secrets.test.ts` around lines 83 - 137, Add a test inside the describe('resolveEnvRef') block that simulates a 200 response with malformed JSON from ossFetchMock (e.g., ossFetchMock.mockResolvedValueOnce(new Response('invalid', { status: 200 })) ) when calling resolveEnvRef('env(BROKEN)', 'auth.smtp.password') and assert it rejects with the same error shape used for HTTP lookup failures (e.g., toMatchObject({ code: 'SECRET_LOOKUP_FAILED' })); reference resolveEnvRef, ossFetchMock and the existing SECRET_LOOKUP_FAILED assertions to mirror the error expectation and placement alongside the other cases.
🤖 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/export.ts`:
- Around line 83-84: The guard using "'smtpConfig' in authSlice" can throw if
authSlice is not an object; update the conditional in export command (the block
referencing authSlice and smtpConfig) to first verify authSlice is a non-null
object (e.g., typeof authSlice === 'object' && authSlice !== null) before using
the 'in' operator and accessing authSlice.smtpConfig, so malformed metadata
yields a controlled CLI error path rather than a runtime TypeError.
In `@src/lib/config-format.ts`:
- Around line 51-53: The note rendering incorrectly nests env(...) when
c.passwordEnvRef already includes env(...) — normalize c.passwordEnvRef before
pushing the line: check and strip any surrounding "env(" and trailing ")" (or
detect if it already starts with "env(") so you always produce a single env(...)
wrapper; update the code around the lines.push call that references
c.passwordEnvRef to use the normalized value (e.g., compute a local
normalizedPasswordRef and then use lines.push(` (password force-resent from
env(${normalizedPasswordRef}))`)) so duplicate env(...) is avoided.
In `@src/lib/config-schema.ts`:
- Around line 113-118: The SMTP port validation currently only checks for
integer type in the block handling 'port' and allows out-of-range values; update
the check in the same conditional (the code that assigns out.port) to validate
that obj.port is an integer within the TCP port range 1..65535 and throw
ConfigValidationError('auth.smtp.port', '<message>') when it's not; keep the
existing type/integer validation but extend it to enforce the range before
assigning out.port so invalid values like -1 or 70000 are rejected.
In `@src/lib/config-secrets.ts`:
- Around line 130-131: The JSON parsing for the secret response is unguarded:
wrap the call to res.json() in a try/catch and validate HTTP status before
trusting the body so parsing errors or non-2xx responses produce a consistent
CLIError; specifically, in the function that handles the fetch response (the
block using res and const body = (await res.json()) as { value?: string }),
first check res.ok and throw a CLIError with context if not ok, then try to
parse res.json() inside a try/catch and on any exception or if body.value is not
a non-empty string throw a CLIError describing the failure to retrieve/parse the
secret (include res.status/res.statusText or the caught error.message for
debugging).
In `@src/lib/config-toml.ts`:
- Around line 56-61: The emitted guidance hardcodes "SMTP_PASSWORD" even though
smtp.password contains the actual env reference; update the logic that builds
the comment (where lines.push is called for smtp.password in config-toml.ts) to
extract the secret/env name from smtp.password (the env(...) string) and
interpolate that name into the comment instead of the literal SMTP_PASSWORD,
falling back to a generic message if extraction fails. Ensure you reference and
parse smtp.password (the env(...) value) to compute the secret name used in the
comment and then push the comment using that extracted name.
---
Outside diff comments:
In `@src/commands/config/apply.ts`:
- Around line 102-113: Preflight resolution of SMTP secrets for every change
before any mutation: iterate result.changes and for each change whose path may
require secrets (e.g., SMTP/password fields) resolve the secret ahead of time
and collect failures into skipped so no partial applies occur; then call
applyChange(change, preResolvedSecret?) instead of resolving inside applyChange.
Update the applyChange function signature to accept an optional
preResolvedPassword (or credentials object) and ensure it uses that value if
provided and does not attempt resolution during the mutation. Apply the same
preflight logic to the second batch processing block that also iterates
result.changes (the other loop around applyChange) so both places resolve SMTP
secrets before making any PUTs and mark failing resolutions as skipped.
---
Nitpick comments:
In `@src/lib/config-secrets.test.ts`:
- Around line 83-137: Add a test inside the describe('resolveEnvRef') block that
simulates a 200 response with malformed JSON from ossFetchMock (e.g.,
ossFetchMock.mockResolvedValueOnce(new Response('invalid', { status: 200 })) )
when calling resolveEnvRef('env(BROKEN)', 'auth.smtp.password') and assert it
rejects with the same error shape used for HTTP lookup failures (e.g.,
toMatchObject({ code: 'SECRET_LOOKUP_FAILED' })); reference resolveEnvRef,
ossFetchMock and the existing SECRET_LOOKUP_FAILED assertions to mirror the
error expectation and placement alongside the other cases.
🪄 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: c19f54ee-fe1e-4af7-91cf-bbe4d02adda8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
package.jsonsrc/commands/config/apply.test.tssrc/commands/config/apply.tssrc/commands/config/export.test.tssrc/commands/config/export.tssrc/lib/config-capabilities.tssrc/lib/config-diff.test.tssrc/lib/config-diff.tssrc/lib/config-format.tssrc/lib/config-schema.tssrc/lib/config-secrets.test.tssrc/lib/config-secrets.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
Comment on lines
+51
to
+53
| if (c.passwordEnvRef) { | ||
| lines.push(` (password force-resent from env(${c.passwordEnvRef}))`); | ||
| } |
There was a problem hiding this comment.
Normalize passwordEnvRef before rendering the force-resend note.
Current formatting can produce env(env(...)) depending on what passwordEnvRef carries.
Proposed fix
if (c.passwordEnvRef) {
- lines.push(` (password force-resent from env(${c.passwordEnvRef}))`);
+ const ref = c.passwordEnvRef.startsWith('env(')
+ ? c.passwordEnvRef
+ : `env(${c.passwordEnvRef})`;
+ lines.push(` (password force-resent from ${ref})`);
}🤖 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-format.ts` around lines 51 - 53, The note rendering
incorrectly nests env(...) when c.passwordEnvRef already includes env(...) —
normalize c.passwordEnvRef before pushing the line: check and strip any
surrounding "env(" and trailing ")" (or detect if it already starts with "env(")
so you always produce a single env(...) wrapper; update the code around the
lines.push call that references c.passwordEnvRef to use the normalized value
(e.g., compute a local normalizedPasswordRef and then use lines.push(`
(password force-resent from env(${normalizedPasswordRef}))`)) so duplicate
env(...) is avoided.
Comment on lines
+113
to
+118
| if ('port' in obj) { | ||
| if (typeof obj.port !== 'number' || !Number.isInteger(obj.port)) { | ||
| throw new ConfigValidationError('auth.smtp.port', 'must be an integer'); | ||
| } | ||
| out.port = obj.port; | ||
| } |
There was a problem hiding this comment.
Constrain SMTP port to valid TCP range.
auth.smtp.port accepts any integer right now, including invalid values like -1 or 70000.
Proposed fix
if ('port' in obj) {
- if (typeof obj.port !== 'number' || !Number.isInteger(obj.port)) {
- throw new ConfigValidationError('auth.smtp.port', 'must be an integer');
+ if (
+ typeof obj.port !== 'number' ||
+ !Number.isInteger(obj.port) ||
+ obj.port < 1 ||
+ obj.port > 65535
+ ) {
+ throw new ConfigValidationError('auth.smtp.port', 'must be an integer between 1 and 65535');
}
out.port = obj.port;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ('port' in obj) { | |
| if (typeof obj.port !== 'number' || !Number.isInteger(obj.port)) { | |
| throw new ConfigValidationError('auth.smtp.port', 'must be an integer'); | |
| } | |
| out.port = obj.port; | |
| } | |
| if ('port' in obj) { | |
| if ( | |
| typeof obj.port !== 'number' || | |
| !Number.isInteger(obj.port) || | |
| obj.port < 1 || | |
| obj.port > 65535 | |
| ) { | |
| throw new ConfigValidationError('auth.smtp.port', 'must be an integer between 1 and 65535'); | |
| } | |
| out.port = obj.port; | |
| } |
🤖 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-schema.ts` around lines 113 - 118, The SMTP port validation
currently only checks for integer type in the block handling 'port' and allows
out-of-range values; update the check in the same conditional (the code that
assigns out.port) to validate that obj.port is an integer within the TCP port
range 1..65535 and throw ConfigValidationError('auth.smtp.port', '<message>')
when it's not; keep the existing type/integer validation but extend it to
enforce the range before assigning out.port so invalid values like -1 or 70000
are rejected.
Comment on lines
+130
to
+131
| const body = (await res.json()) as { value?: string }; | ||
| if (typeof body.value !== 'string' || body.value.length === 0) { |
There was a problem hiding this comment.
Guard JSON parsing for successful secret lookups.
A 2xx response with invalid/empty JSON will currently throw an untyped parse error instead of a consistent CLIError, which makes config apply failure handling brittle.
Proposed fix
- const body = (await res.json()) as { value?: string };
+ let body: { value?: string };
+ try {
+ body = (await res.json()) as { value?: string };
+ } catch {
+ throw new CLIError(
+ `failed to resolve env(${secretName}) for ${fieldPath}: invalid response body`,
+ 1,
+ 'SECRET_LOOKUP_FAILED',
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const body = (await res.json()) as { value?: string }; | |
| if (typeof body.value !== 'string' || body.value.length === 0) { | |
| let body: { value?: string }; | |
| try { | |
| body = (await res.json()) as { value?: string }; | |
| } catch { | |
| throw new CLIError( | |
| `failed to resolve env(${secretName}) for ${fieldPath}: invalid response body`, | |
| 1, | |
| 'SECRET_LOOKUP_FAILED', | |
| ); | |
| } | |
| if (typeof body.value !== 'string' || body.value.length === 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-secrets.ts` around lines 130 - 131, The JSON parsing for the
secret response is unguarded: wrap the call to res.json() in a try/catch and
validate HTTP status before trusting the body so parsing errors or non-2xx
responses produce a consistent CLIError; specifically, in the function that
handles the fetch response (the block using res and const body = (await
res.json()) as { value?: string }), first check res.ok and throw a CLIError with
context if not ok, then try to parse res.json() inside a try/catch and on any
exception or if body.value is not a non-empty string throw a CLIError describing
the failure to retrieve/parse the secret (include res.status/res.statusText or
the caught error.message for debugging).
Comment on lines
+56
to
+61
| if (smtp.password !== undefined) { | ||
| // password is always an env() ref at this point (schema validator rejects | ||
| // literals at parse time). Emit a comment about how to set the actual | ||
| // value so the user can discover the workflow from a fresh export. | ||
| lines.push('# password is managed via secrets — run `insforge secrets add SMTP_PASSWORD <value>`'); | ||
| lines.push(`password = ${JSON.stringify(smtp.password)}`); |
There was a problem hiding this comment.
Avoid hardcoding SMTP_PASSWORD in emitted guidance.
The rendered comment can disagree with the actual password = "env(...)" value when users choose a different secret name.
Proposed fix
- lines.push('# password is managed via secrets — run `insforge secrets add SMTP_PASSWORD <value>`');
+ lines.push('# password is managed via secrets — run `insforge secrets add <NAME> <value>`');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (smtp.password !== undefined) { | |
| // password is always an env() ref at this point (schema validator rejects | |
| // literals at parse time). Emit a comment about how to set the actual | |
| // value so the user can discover the workflow from a fresh export. | |
| lines.push('# password is managed via secrets — run `insforge secrets add SMTP_PASSWORD <value>`'); | |
| lines.push(`password = ${JSON.stringify(smtp.password)}`); | |
| if (smtp.password !== undefined) { | |
| // password is always an env() ref at this point (schema validator rejects | |
| // literals at parse time). Emit a comment about how to set the actual | |
| // value so the user can discover the workflow from a fresh export. | |
| lines.push('# password is managed via secrets — run `insforge secrets add <NAME> <value>`'); | |
| lines.push(`password = ${JSON.stringify(smtp.password)}`); |
🤖 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 56 - 61, The emitted guidance hardcodes
"SMTP_PASSWORD" even though smtp.password contains the actual env reference;
update the logic that builds the comment (where lines.push is called for
smtp.password in config-toml.ts) to extract the secret/env name from
smtp.password (the env(...) string) and interpolate that name into the comment
instead of the literal SMTP_PASSWORD, falling back to a generic message if
extraction fails. Ensure you reference and parse smtp.password (the env(...)
value) to compute the secret name used in the comment and then push the comment
using that extracted name.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 15 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/export.ts">
<violation number="1" location="src/commands/config/export.ts:83">
P2: Guard `authSlice` as an object before using the `in` operator to avoid runtime `TypeError` on malformed metadata responses.</violation>
</file>
<file name="src/lib/config-secrets.ts">
<violation number="1" location="src/lib/config-secrets.ts:102">
P1: `resolveEnvRef` relies on `ossFetch` returning 404 responses, but `ossFetch` throws on non-2xx, so the 404/`!res.ok` branches here are unreachable and missing secrets are misclassified as `SECRET_LOOKUP_FAILED`.</violation>
</file>
<file name="src/lib/config-toml.ts">
<violation number="1" location="src/lib/config-toml.ts:60">
P3: Avoid hardcoding `SMTP_PASSWORD` in the emitted guidance so instructions don't conflict with a custom `env(...)` secret name.</violation>
</file>
<file name="src/lib/config-schema.ts">
<violation number="1" location="src/lib/config-schema.ts:114">
P2: Validate `auth.smtp.port` against the valid TCP range (1-65535); integer-only validation currently allows invalid ports like -1 and 70000.</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.
Comment on lines
+114
to
+115
| if (typeof obj.port !== 'number' || !Number.isInteger(obj.port)) { | ||
| throw new ConfigValidationError('auth.smtp.port', 'must be an integer'); |
There was a problem hiding this comment.
P2: Validate auth.smtp.port against the valid TCP range (1-65535); integer-only validation currently allows invalid ports like -1 and 70000.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/config-schema.ts, line 114:
<comment>Validate `auth.smtp.port` against the valid TCP range (1-65535); integer-only validation currently allows invalid ports like -1 and 70000.</comment>
<file context>
@@ -62,5 +84,83 @@ function validateAuth(input: unknown): AuthConfig {
+ }
+
+ if ('port' in obj) {
+ if (typeof obj.port !== 'number' || !Number.isInteger(obj.port)) {
+ throw new ConfigValidationError('auth.smtp.port', 'must be an integer');
+ }
</file context>
Suggested change
| if (typeof obj.port !== 'number' || !Number.isInteger(obj.port)) { | |
| throw new ConfigValidationError('auth.smtp.port', 'must be an integer'); | |
| if ( | |
| typeof obj.port !== 'number' || | |
| !Number.isInteger(obj.port) || | |
| obj.port < 1 || | |
| obj.port > 65535 | |
| ) { | |
| throw new ConfigValidationError('auth.smtp.port', 'must be an integer between 1 and 65535'); |
| // password is always an env() ref at this point (schema validator rejects | ||
| // literals at parse time). Emit a comment about how to set the actual | ||
| // value so the user can discover the workflow from a fresh export. | ||
| lines.push('# password is managed via secrets — run `insforge secrets add SMTP_PASSWORD <value>`'); |
There was a problem hiding this comment.
P3: Avoid hardcoding SMTP_PASSWORD in the emitted guidance so instructions don't conflict with a custom env(...) secret name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/config-toml.ts, line 60:
<comment>Avoid hardcoding `SMTP_PASSWORD` in the emitted guidance so instructions don't conflict with a custom `env(...)` secret name.</comment>
<file context>
@@ -32,7 +37,36 @@ export function stringifyConfigToml(config: InsforgeConfig): string {
+ // password is always an env() ref at this point (schema validator rejects
+ // literals at parse time). Emit a comment about how to set the actual
+ // value so the user can discover the workflow from a fresh export.
+ lines.push('# password is managed via secrets — run `insforge secrets add SMTP_PASSWORD <value>`');
+ lines.push(`password = ${JSON.stringify(smtp.password)}`);
+ }
</file context>
Suggested change
| lines.push('# password is managed via secrets — run `insforge secrets add SMTP_PASSWORD <value>`'); | |
| lines.push('# password is managed via secrets — run `insforge secrets add <NAME> <value>`'); |
ossFetch throws on any non-2xx response, swallowing the status code. The
resolveEnvRef path that previously checked res.status === 404 never fired
— missing secrets always landed in SECRET_LOOKUP_FAILED.
Recover the signal from the thrown error's message instead (the backend's
NOT_FOUND path uses "Secret not found: NAME" wording). Verified against
live local backend on the SMTP branch:
$ insforge config apply # with env(NONEXISTENT) in TOML
code: SECRET_NOT_FOUND
msg: auth.smtp.password references env(NONEXISTENT) but no such secret
exists.
fix: insforge secrets add NONEXISTENT "<value>"
Unit tests updated to mock ossFetch as throwing (matches real behavior).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/config/apply.test.ts (1)
24-29: ⚡ Quick winMake missing-secret mock error shape closer to real
ossFetchfailures.The mocked throw message (
Secret not found: ...) is a bit synthetic compared to non-2xx throw paths. Aligning it with the real thrown format (including 404 signal) will better guard against regressions inresolveEnvReferror parsing.🤖 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 24 - 29, The mock in src/commands/config/apply.test.ts should throw an error shaped like real ossFetch non-2xx failures so resolveEnvRef can parse the 404 signal; change the thrown Error in the test (currently "Secret not found: ${key}") to include the HTTP status indicator (e.g. include "404" or "status: 404" and a Not Found phrase) so error-parsing paths in resolveEnvRef and any ossFetch-related logic will behave the same as real failures.
🤖 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 93-97: The shared test state variable nextMetadataResponse is not
reset between tests; update the beforeEach block (the one that calls
vi.clearAllMocks(), secretsStore.clear(), and sets tmp) to also reset
nextMetadataResponse (e.g. set to undefined/null or its initial value) so each
test starts with a clean metadata state and cannot inherit prior test responses.
In `@src/lib/config-secrets.test.ts`:
- Around line 124-127: The test in src/lib/config-secrets.test.ts that currently
does ossFetchMock.mockResolvedValueOnce(new Response('boom', { status: 500 }))
should instead mock the fetch as a rejected promise to mirror runtime behavior;
replace that resolved Response with ossFetchMock.mockRejectedValueOnce(new
Error('boom')) (or an Error-like rejection) so the test exercising the
SECRET_LOOKUP_FAILED mapping uses a thrown error like the real ossFetch and
accurately reflects the code paths in the functions that handle ossFetch
failures.
---
Nitpick comments:
In `@src/commands/config/apply.test.ts`:
- Around line 24-29: The mock in src/commands/config/apply.test.ts should throw
an error shaped like real ossFetch non-2xx failures so resolveEnvRef can parse
the 404 signal; change the thrown Error in the test (currently "Secret not
found: ${key}") to include the HTTP status indicator (e.g. include "404" or
"status: 404" and a Not Found phrase) so error-parsing paths in resolveEnvRef
and any ossFetch-related logic will behave the same as real failures.
🪄 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: 715005df-00aa-491b-9ed3-213fc682fdd2
📒 Files selected for processing (3)
src/commands/config/apply.test.tssrc/lib/config-secrets.test.tssrc/lib/config-secrets.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/config-secrets.ts
Comment on lines
93
to
97
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| secretsStore.clear(); | ||
| tmp = mkdtempSync(join(tmpdir(), 'insforge-apply-test-')); | ||
| }); |
There was a problem hiding this comment.
Reset nextMetadataResponse in beforeEach to prevent cross-test coupling.
nextMetadataResponse is shared mutable state but isn’t reset per test. Add a reset in beforeEach so future tests can’t accidentally inherit previous metadata.
Suggested patch
beforeEach(() => {
vi.clearAllMocks();
secretsStore.clear();
+ nextMetadataResponse = {};
tmp = mkdtempSync(join(tmpdir(), 'insforge-apply-test-'));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| secretsStore.clear(); | |
| tmp = mkdtempSync(join(tmpdir(), 'insforge-apply-test-')); | |
| }); | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| secretsStore.clear(); | |
| nextMetadataResponse = {}; | |
| tmp = mkdtempSync(join(tmpdir(), 'insforge-apply-test-')); | |
| }); |
🤖 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 93 - 97, The shared test
state variable nextMetadataResponse is not reset between tests; update the
beforeEach block (the one that calls vi.clearAllMocks(), secretsStore.clear(),
and sets tmp) to also reset nextMetadataResponse (e.g. set to undefined/null or
its initial value) so each test starts with a clean metadata state and cannot
inherit prior test responses.
Comment on lines
+124
to
+127
| it('throws SECRET_LOOKUP_FAILED on a non-404 HTTP error', async () => { | ||
| ossFetchMock.mockResolvedValueOnce( | ||
| new Response('boom', { status: 500 }), | ||
| ); |
There was a problem hiding this comment.
Mock non-404 failures as rejected promises to mirror runtime behavior.
This test currently simulates HTTP 500 via a resolved Response, but runtime behavior (per this PR’s own contract) is that ossFetch throws on non-2xx. Keeping this as a rejection makes the test representative and avoids false confidence in SECRET_LOOKUP_FAILED mapping.
Suggested test adjustment
it('throws SECRET_LOOKUP_FAILED on a non-404 HTTP error', async () => {
- ossFetchMock.mockResolvedValueOnce(
- new Response('boom', { status: 500 }),
- );
+ ossFetchMock.mockRejectedValueOnce(
+ new Error('Request failed with status 500'),
+ );
await expect(
resolveEnvRef('env(WHATEVER)', 'auth.smtp.password'),
).rejects.toMatchObject({ code: 'SECRET_LOOKUP_FAILED' });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('throws SECRET_LOOKUP_FAILED on a non-404 HTTP error', async () => { | |
| ossFetchMock.mockResolvedValueOnce( | |
| new Response('boom', { status: 500 }), | |
| ); | |
| it('throws SECRET_LOOKUP_FAILED on a non-404 HTTP error', async () => { | |
| ossFetchMock.mockRejectedValueOnce( | |
| new Error('Request failed with status 500'), | |
| ); | |
| await expect( | |
| resolveEnvRef('env(WHATEVER)', 'auth.smtp.password'), | |
| ).rejects.toMatchObject({ code: 'SECRET_LOOKUP_FAILED' }); | |
| }); |
🤖 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-secrets.test.ts` around lines 124 - 127, The test in
src/lib/config-secrets.test.ts that currently does
ossFetchMock.mockResolvedValueOnce(new Response('boom', { status: 500 })) should
instead mock the fetch as a rejected promise to mirror runtime behavior; replace
that resolved Response with ossFetchMock.mockRejectedValueOnce(new
Error('boom')) (or an Error-like rejection) so the test exercising the
SECRET_LOOKUP_FAILED mapping uses a thrown error like the real ossFetch and
accurately reflects the code paths in the functions that handle ossFetch
failures.
Mirrors the existing guard on the allowed_redirect_urls branch — without it, a malformed metadata response (e.g. auth: "x") would TypeError on 'smtpConfig' in authSlice instead of cleanly landing in skipped[]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Step 2 of the SMTP config-as-code initiative. Adds
[auth.smtp]to the declarative TOML surface with secret-aware password handling.Companion PR: InsForge/InsForge#1258 (backend — exposes
smtpConfigin admin/api/metadata). Required for this CLI feature to work — without it,config applycorrectly puts SMTP changes in `skipped[]` per the capability gate.What ships
Schema
[auth.smtp]section ininsforge.tomlwith 8 fields (enabled, host, port, username, password, sender_email, sender_name, min_interval_seconds)passwordfield validated asenv(NAME)ref only — literal values rejected at parse time (the validator infrastructure from PR feat(cli): config-as-code (export/plan/apply) + env() validation + compute port fix #109 finally gets its first consumer)Capability gate
metadataSupports()extended to proberaw.auth.smtpConfigpresenceskipped[]with the upgrade message; no PUT issued — same protocol as redirect URLsenv() ref resolution
resolveEnvRef(envRef, fieldPath)insrc/lib/config-secrets.ts/api/secrets/:namepre-flight before any PUT — missing/inactive secrets fail fast with actionable error (SECRET_NOT_FOUND/SECRET_EMPTY) naming the exactsecrets addcommandDiff layer
DiffChangediscriminated union:auth(redirect URLs) |auth.smtp(whole-object upsert)password = \"env(NAME)\", every `apply` resolves the current secrets-store value and re-sends. Reason: backend ciphertext can't be diffed against client-side, and re-sending matches user mental model ("I just rotated SMTP_PASSWORD, re-apply should propagate it")Export
Apply
Version bump
0.1.75 → 0.1.76
Tests
Test plan
Design notes captured in code
The trickier decisions are documented inline:
Followups (out of scope here)
🤖 Generated with Claude Code
Summary by cubic
Adds
[auth.smtp]toinsforge.tomlwith secret-awareenv(NAME)password handling and CLI apply/export. Also fixes secret lookup to returnSECRET_NOT_FOUNDand adds a safe typeof-guard in export to avoid crashes on malformed metadata.New Features
[auth.smtp]withenabled,host,port,username,password(must beenv(NAME)),sender_email,sender_name,min_interval_seconds; literal passwords are rejected.env(NAME)resolves only from the InsForge secrets store before apply; clear errors for missing (SECRET_NOT_FOUND) or inactive (SECRET_EMPTY); no local env usage./api/auth/smtp-config; omittingpasswordpreserves the current encrypted value; whenpassword = "env(NAME)"is present, it is force-resent on each apply; plan output uses opaque markers only.password = "env(SMTP_PASSWORD)"with a discovery comment; otherwise omits the line./api/metadataexposesauth.smtpConfig; otherwise they appear inskipped[].Bug Fixes
ossFetchthrows: missing secrets surface asSECRET_NOT_FOUNDwith aninsforge secrets add NAME "<value>"hint.authbefore checking forsmtpConfigto avoid a TypeError on malformed metadata; falls back toskipped[]as intended.Written for commit 88842d8. Summary will update on new commits.
Summary by CodeRabbit