feat: add core types, error definitions, and configuration#11
Conversation
Add foundation layer: AppState type, tagged error classes using better-result, app directory paths, and settings file I/O with Zod validation.
…andling - Add SettingsSchema with Zod for runtime validation of settings JSON - Add chmod(0o600) to secure settings file after writes - Create OUTPUT_DIR in ensureAppDir() with secure permissions - Add owner-only (0o700) permissions to APP_DIR and OUTPUT_DIR - Improve error handling: distinguish ENOENT from real failures - Delete duplicate AppState type (unused, kept Config as source of truth) - Handle null/undefined JSON payloads gracefully - Validate settings input before merge/save operations - Add proper error cause chaining for debugging Resolves 8 security and reliability issues from code review.
- Only ignore known unsupported cases (ENOTSUP, EINVAL) for chmod - Return ExportError when settings file permissions cannot be secured - Propagate real permission errors in ensureAppDir() instead of swallowing - Ensures security issues are surfaced instead of being hidden
- Fix inverted error handling logic that swallowed non-standard errors - Align with loadConfig pattern: only swallow ENOENT, throw all other errors - Prevent silent data loss when errors lack code property - Ensure permission/disk errors are properly propagated
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummary by CodeRabbitChores
New Features
WalkthroughThis PR introduces foundational infrastructure modules: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR cherry-picks the PR #2-specific commits cleanly, introducing the foundational infrastructure for The implementation is well-structured — railway-oriented error handling via
Confidence Score: 4/5Safe to merge — solid infrastructure foundation with only minor style and clarity concerns, no correctness or security issues. All three new source files are well-structured, errors are properly handled with railway-oriented results, and file permissions are secured. The three flagged items (chmod duplication, misleading error label, undefined-spread behaviour) are all non-blocking P2 style concerns that don't affect correctness in the normal path. Score of 4 rather than 5 because the undefined-spread issue could silently delete user settings under a specific (unlikely but realistic) caller pattern. src/config.ts — three minor style concerns worth addressing before the next layer of features builds on top of this API. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Caller]) -->|loadConfig| B[readSettingsFile]
B --> C{File exists?}
C -->|No| D[Return empty SettingsJson]
C -->|Yes| E[Parse JSON]
E --> F{Valid JSON?}
F -->|SyntaxError| D
F -->|Yes| G[Zod safeParse]
G -->|Invalid| D
G -->|Valid| H[Return SettingsJson]
D --> I[Resolve env vars DISCORD_BOT_TOKEN etc.]
H --> I
I --> J{token present?}
J -->|No| K[Err: ConfigError]
J -->|Yes| L[Ok: Config]
A2([Caller]) -->|saveSettings| M[Validate input Zod safeParse]
M -->|Invalid| N[Err: ExportError]
M -->|Valid| O[ensureAppDir]
O --> P[readSettingsFile existing]
P --> Q[Merge: existing + new values]
Q --> R[Bun.write SETTINGS_FILE]
R --> S[chmod 0o600]
S --> T{chmod error?}
T -->|ENOTSUP / EINVAL| U[Ok: void - ignore]
T -->|Other error| V[Err: ExportError]
T -->|Success| U
Reviews (1): Last reviewed commit: "fix: .gitignore patterns, type inference..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| await ensureAppDir(); | ||
|
|
||
| const existing = await readSettingsFile(); |
There was a problem hiding this comment.
readSettingsFile() is called inside the outer try/catch, so any I/O error it throws (e.g. permission denied) will be caught at line 123 and wrapped as "Failed to save settings: ...". This is technically handled, but the error message will mislead callers — the failure was a read, not a write. Consider explicitly catching and re-wrapping it with a more accurate message:
const existing = await readSettingsFile().catch((cause) => {
throw new ExportError({
message: `Failed to read existing settings before saving: ${cause instanceof Error ? cause.message : String(cause)}`,
cause,
});
});| // Secure file permissions (owner-only) | ||
| try { | ||
| await chmod(SETTINGS_FILE, 0o600); | ||
| } catch (err) { | ||
| if ( | ||
| err && | ||
| typeof err === "object" && | ||
| "code" in err && | ||
| (err.code === "ENOTSUP" || err.code === "EINVAL") | ||
| ) { | ||
| // Ignore known unsupported cases (e.g., Windows) | ||
| } else { | ||
| return new Err( | ||
| new ExportError({ | ||
| message: `Failed to secure settings file permissions: ${err instanceof Error ? err.message : String(err)}`, | ||
| cause: err, | ||
| }) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
paths.ts already contains a private chmodSafe helper with the exact same error-suppression logic for ENOTSUP/EINVAL. Since this block duplicates it verbatim, consider exporting chmodSafe from paths.ts and reusing it here:
// in paths.ts — export it
export const chmodSafe = async (path: string, mode: number): Promise<void> => { ... };Then in config.ts:
import { ensureAppDir, SETTINGS_FILE, chmodSafe } from "@/paths.ts";
// ...
await chmodSafe(SETTINGS_FILE, 0o600);This keeps the two implementations in sync and simplifies saveSettings.
|
|
||
| const existing = await readSettingsFile(); | ||
|
|
||
| const merged = { ...existing, ...validationResult.data }; |
There was a problem hiding this comment.
undefined silently deletes keys
{ ...existing, ...validationResult.data } will overwrite any key in existing with undefined if the caller passes that field explicitly as undefined (e.g. saveSettings({ token: "abc", guildId: undefined })). JSON.stringify will then omit that key, effectively deleting guildId from the saved file — which is likely unintended when the intent was only to update the token.
Consider filtering out undefined values before merging:
const patch = Object.fromEntries(
Object.entries(validationResult.data).filter(([, v]) => v !== undefined)
);
const merged = { ...existing, ...patch };1 participant
Summary
mainAppStatetype for application state managementbetter-resultfor railway-oriented error handling~/.discord-search)Context
PR #2 (
init/02-core-types) was accidentally merged with PR #1 commits mixed in, then reverted. This PR brings over only the PR #2-specific commits cleanly ontomain.Test plan
bun run typecheckpassesbun run checkpasses linting