Repository navigation
feat: encrypted secrets, a served /_storage, and a boot that says what it needs - #51
Conversation
…t it needs
Three things an app hit in production, and one the ceiling forced.
**Storage was silently local.** `startStorage` never read an external binding:
`s3Driver()` existed and nothing called it, so an S3- or R2-configured deploy
wrote to a container-local disk that the next restart destroyed. It now honours
the binding, and the embedded path fails at boot with `X_STORAGE_UNWRITABLE`
naming both real options instead of a bare `EROFS` out of `mkdirSync` — the
failure that CrashLooped a hardened container 22 times with nothing in the log
that mentioned storage.
**`grantUpload` minted URLs nothing answered.** `/_storage` is mounted now, in
`x dev` and in `serve.ts`, with `isTenantScoped` deciding what a signed key may
reach.
**Secrets.** `x secrets init|edit|show`, an encrypted file committed with the
repo and a master key that is not — the two halves of `x secrets edit` that
belong to the terminal (`X_SECRETS_EDITOR_MISSING`, `X_SECRETS_EDIT_FAILED`)
are the CLI's; every code about the envelope is core's.
**`packages/cli/src/errors.ts` split at 528 lines**, table into `error-codes.ts`
and classes into `errors.ts`, the shape `@ultimat3/core` already uses. That
exposed a real defect in the gate: `isCodeRegistry` tested the *filename*, so a
classes-only `errors.ts` still counted as its package's registry and outranked
the package that owns the code — `X_NOT_IMPLEMENTED` moved from `core` to `cli`
in the manifest the moment the file split. It now reads the file's own
`export const <PKG>_ERROR_{CODES,TITLES}` declaration, which also corrects a
misattribution that predates the split: `X_FORBIDDEN` is `policy`'s, and was
credited to `action` for a bare literal in a `??` fallback.
`bun run verify`: all 17 steps green. Reference-app ratchet: 10 of 17, 7 pinned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change adds encrypted secret management, policy-authorized storage reads, configurable storage bindings, and a separated CLI error-code registry. It also adds tests, public exports, manifest entries, command wiring, and documentation. ChangesEncrypted secrets management
Policy-authorized storage reads
Error-code ownership and scanning
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 15
🤖 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 `@packages/cli/src/cmd-secrets.test.ts`:
- Around line 135-140: Update the test covering key-file overwrite protection to
use a root containing only .secrets.key and no secrets.enc.json, so execution
reaches the key-file guard in secretsCommand.init. Keep the existing
initialized-root scenario as a separate test for the sealed-file conflict, and
assert X_GENERATE_CONFLICT for both cases.
In `@packages/cli/src/cmd-secrets.ts`:
- Around line 159-162: Update the signal handlers around shred so SIGINT and
SIGTERM each restore the signal’s default termination behavior by re-raising the
same signal after cleanup. In the edit flow’s finally block, remove the matching
handlers to avoid retaining listeners once io.launch completes, while preserving
shred’s cleanup behavior.
In `@packages/cli/src/dev-runtime.test.ts`:
- Around line 356-363: Update the test for an unwritable embedded root by
skipping it when the current process runs as root, before creating or chmod’ing
the temporary directory. Preserve the existing assertions and storage setup for
non-root processes so the test continues validating the X_STORAGE_UNWRITABLE
behavior.
In `@packages/cli/src/dev-runtime.ts`:
- Line 133: Update the S3 configuration parsing near forcePathStyle so
S3_FORCE_PATH_STYLE=true and other usual truthy spellings enable path-style
requests, rather than only the literal "1"; alternatively, validate the value
and reject unknown spellings with a coded error. Preserve explicit configuration
semantics and avoid silently treating declared truthy values as false.
In `@packages/cli/src/dev-storage.test.ts`:
- Around line 47-50: Remove the type-narrowing `throw new Error('no route')`
from `call`; instead, narrow the route through an assertion or equivalent
control flow and use the narrowed `matched` route when invoking its handler. Do
not introduce a bare error or unrelated error subclass.
In `@packages/cli/src/dev-storage.ts`:
- Around line 82-97: Update readStorageObject and the StorageDriver contract to
use a metadata-only operation for response validation and a range-capable stream
for body delivery instead of get() buffering the full object. Propagate the
metadata and requested range through local and S3 drivers, and have
storageResponse() use stream() so 304, 416, and 206 responses are determined
before reading and large bodies are not materialized in memory.
In `@packages/cli/src/errors.ts`:
- Around line 329-333: Update StorageUnwritableError to accept a named options
object instead of two positional string parameters, matching the constructors of
CatalogExistsError, CliNotImplementedError, and the secrets error classes.
Preserve the existing cause, fix, error code, and docs behavior, and update
every new StorageUnwritableError call site to pass the corresponding named
properties.
In `@packages/cli/src/mcp-errors.ts`:
- Line 83: Update the remediation for X_SECRETS_EDIT_FAILED in CLI_FIXES and
SecretsEditFailedError to use the same machine-readable command, such as x
secrets edit --json, while retaining the trailing # note only where it serves as
documentation.
In `@packages/cli/src/ts-scan.ts`:
- Around line 174-186: Update isCodeRegistry in packages/cli/src/ts-scan.ts
(lines 174-186) to apply CODE_TABLE to the existing comment- and
string-literal-masked source view, so only executable registry declarations are
detected. Add negative test cases in packages/cli/src/ts-scan.test.ts (lines
219-223) covering table-shaped text inside a comment and a string literal.
In `@packages/core/src/secrets-errors.ts`:
- Around line 57-67: Update the SecretsKeyInvalidError constructor’s fix message
to branch on input.at: use the key-file cat command only for file-based sources,
and provide a runnable platform/environment-specific command for
ULTIMATE_SECRETS_KEY. Preserve the stable error code, cause, and source metadata
while ensuring every emitted fix names and works for the failing source.
In `@packages/core/src/secrets-store.ts`:
- Around line 45-56: Update findMasterKey and readSecretsFile to catch
filesystem read failures and rethrow the appropriate typed
SecretsKeyInvalidError or SecretsFileInvalidError, preserving the original
exception through the sourceError field. Ensure both errors extend the
established UltimateError hierarchy and expose the expected X_* code and
remediation details instead of allowing bare filesystem Errors to escape.
- Around line 86-91: Update writeMasterKeyFile to call chmodSync(path,
SECRETS_KEY_MODE) after writeFileSync so rewritten master-key files always
regain restrictive permissions. Add an overwrite test in the secrets-store test
suite that verifies existing key files receive SECRETS_KEY_MODE.
In `@packages/core/src/secrets.ts`:
- Around line 21-28: Update the key-id validation around KEY_ID and the related
error message near the referenced validation logic to derive both the regex
length and human-readable length from SECRETS_KEY_ID_LENGTH. Remove the
hard-coded 16-character values while preserving the existing validation behavior
and message intent.
In `@wiki/CLI-Reference.md`:
- Around line 399-401: Update the `secrets init --json` example in the wiki so
its `summary` exactly matches the string rendered by `cli.secrets.init` in
`packages/cli/src/messages.ts`, including the `.gitignore` clause after the
master key ID. Preserve the existing JSON structure and ensure the displayed
output uses the correctly quoted, complete summary.
In `@wiki/Error-Codes.md`:
- Line 269: Move the X_STORAGE_UNWRITABLE row from the “Routes, render and
budgets” table into the “Storage” table, placing it immediately after
X_STORAGE_UPLOAD_FAILED. Preserve the row’s existing description, causes, and
remediation text unchanged.
🪄 Autofix
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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff4eb03b-26ed-43de-b011-57917de82f5e
📒 Files selected for processing (40)
framework.manifest.jsonpackages/cli/CLAUDE.mdpackages/cli/src/cmd-dev.tspackages/cli/src/cmd-secrets.test.tspackages/cli/src/cmd-secrets.tspackages/cli/src/dev-runtime.test.tspackages/cli/src/dev-runtime.tspackages/cli/src/dev-storage.test.tspackages/cli/src/dev-storage.tspackages/cli/src/error-catalog.test.tspackages/cli/src/error-codes.tspackages/cli/src/error-contract.test.tspackages/cli/src/error-contract.tspackages/cli/src/errors.tspackages/cli/src/index.tspackages/cli/src/mcp-errors.test.tspackages/cli/src/mcp-errors.tspackages/cli/src/messages.tspackages/cli/src/registry.tspackages/cli/src/serve.tspackages/cli/src/test-shards.tspackages/cli/src/ts-scan.test.tspackages/cli/src/ts-scan.tspackages/cli/src/verify-test-run.tspackages/core/CLAUDE.mdpackages/core/README.mdpackages/core/src/index.tspackages/core/src/secrets-errors.tspackages/core/src/secrets-store.test.tspackages/core/src/secrets-store.tspackages/core/src/secrets.test.tspackages/core/src/secrets.tspackages/storage/CLAUDE.mdpackages/storage/README.mdpackages/storage/src/index.tspackages/storage/src/path.test.tspackages/storage/src/path.tsscripts/verify.test.tswiki/CLI-Reference.mdwiki/Error-Codes.md
| test('it refuses to overwrite an existing key file', async () => { | ||
| const root = await initialized('conflict'); | ||
| await expect(secretsCommand.run(context(['secrets', 'init'], root))).rejects.toBeUltimateError( | ||
| 'X_GENERATE_CONFLICT', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test never reaches the key-file branch it names.
initialized('conflict') produces a root that has both secrets.enc.json and .secrets.key. init checks secretsFileExists(root) first (packages/cli/src/cmd-secrets.ts Line 116), so the first guard fires. Both guards throw X_GENERATE_CONFLICT, so the assertion passes without the key-file guard at packages/cli/src/cmd-secrets.ts Lines 119-121 ever running.
That guard is the one protecting an unrecoverable loss. Cover it with a root that has a key file and no sealed file.
💚 Proposed fix: split the two guards into two tests
test('it refuses to overwrite an existing key file', async () => {
const root = await initialized('conflict');
await expect(secretsCommand.run(context(['secrets', 'init'], root))).rejects.toBeUltimateError(
'X_GENERATE_CONFLICT',
);
+ // The sealed file is found first above, so the key-file guard needs a root that has only a key.
+ const orphan = await appRoot('conflict-key');
+ await Bun.write(join(orphan, SECRETS_KEY_FILE), generateMasterKey());
+ await expect(
+ secretsCommand.run(context(['secrets', 'init'], orphan)),
+ ).rejects.toBeUltimateError('X_GENERATE_CONFLICT');
});📝 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.
| test('it refuses to overwrite an existing key file', async () => { | |
| const root = await initialized('conflict'); | |
| await expect(secretsCommand.run(context(['secrets', 'init'], root))).rejects.toBeUltimateError( | |
| 'X_GENERATE_CONFLICT', | |
| ); | |
| }); | |
| test('it refuses to overwrite an existing key file', async () => { | |
| const root = await initialized('conflict'); | |
| await expect(secretsCommand.run(context(['secrets', 'init'], root))).rejects.toBeUltimateError( | |
| 'X_GENERATE_CONFLICT', | |
| ); | |
| // The sealed file is found first above, so the key-file guard needs a root that has only a key. | |
| const orphan = await appRoot('conflict-key'); | |
| await Bun.write(join(orphan, SECRETS_KEY_FILE), generateMasterKey()); | |
| await expect( | |
| secretsCommand.run(context(['secrets', 'init'], orphan)), | |
| ).rejects.toBeUltimateError('X_GENERATE_CONFLICT'); | |
| }); |
🤖 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 `@packages/cli/src/cmd-secrets.test.ts` around lines 135 - 140, Update the test
covering key-file overwrite protection to use a root containing only
.secrets.key and no secrets.enc.json, so execution reaches the key-file guard in
secretsCommand.init. Keep the existing initialized-root scenario as a separate
test for the sealed-file conflict, and assert X_GENERATE_CONFLICT for both
cases.
| const shred = (): void => rmSync(dir, { recursive: true, force: true }); | ||
| // A `finally` does not run for a signal, and Ctrl-C inside an editor kills this process too. | ||
| process.once('SIGINT', shred); | ||
| process.once('SIGTERM', shred); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find existing signal handling in the CLI package and the exit-code convention it uses.
rg -nP -C4 "process\.(on|once)\s*\(\s*'SIG" --type=ts packages/
rg -nP -C3 "process\.exit\s*\(" --type=ts packages/cli/src/Repository: developerz-ai/ultimate
Length of output: 2890
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cmd-secrets.ts ---'
sed -n '120,195p' packages/cli/src/cmd-secrets.ts
printf '%s\n' '--- io.launch definition and call sites ---'
rg -n -C5 "launch\s*[:=]|async launch|launch\(" packages/cli/src packages -g '*.ts' | head -240
printf '%s\n' '--- project runtime and signal conventions ---'
rg -n -C3 "interface.*IO|type.*IO|SIGINT|SIGTERM|exitCode|process\.exit" CLAUDE.md AGENTS.md packages/cli/src packages/cli/test 2>/dev/null | head -300Repository: developerz-ai/ultimate
Length of output: 41711
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- declared runtimes ---'
rg -n '"packageManager"|"engines"|bun' package.json packages/cli/package.json bun.lockb bun.lock 2>/dev/null | head -80 || true
command -v bun || true
command -v node || true
printf '%s\n' '--- standalone Node signal probe ---'
python3 - <<'PY'
import os
import signal
import subprocess
import sys
import time
script = r"""
process.once('SIGTERM', () => {
process.stdout.write('handled\n');
});
setTimeout(() => {
process.stdout.write('alive-after-signal\n');
process.exit(0);
}, 150);
"""
child = subprocess.Popen(
['node', '-e', script],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
time.sleep(0.03)
os.kill(child.pid, signal.SIGTERM)
stdout, stderr = child.communicate(timeout=2)
print(f'code={child.returncode}')
print(stdout, end='')
if stderr:
print(f'stderr={stderr}', file=sys.stderr)
print('--- parent-only signal probe with a separate child ---')
parent_script = r"""
const { spawn } = require('node:child_process');
const child = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 1000)']);
process.once('SIGTERM', () => {
process.stdout.write(`parent-handled child=${child.exitCode === null}\n`);
setTimeout(() => {
process.kill(child.pid, 'SIGKILL');
process.exit(0);
}, 100);
});
setTimeout(() => {}, 1000);
"""
parent = subprocess.Popen(
['node', '-e', parent_script],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
time.sleep(0.05)
os.kill(parent.pid, signal.SIGTERM)
stdout, stderr = parent.communicate(timeout=2)
print(f'code={parent.returncode}')
print(stdout, end='')
if stderr:
print(f'stderr={stderr}', file=sys.stderr)
PYRepository: developerz-ai/ultimate
Length of output: 1925
🌐 Web query:
Node.js process signal events registered listener default action SIGTERM documentation
💡 Result:
On non-Windows platforms, the default action of the Node.js process when receiving a SIGTERM signal is to reset the terminal mode and exit the process with an exit code of 128 + the signal number [1][2]. If a listener for the SIGTERM event is registered (e.g., using process.on('SIGTERM',...)), the default behavior is removed, and the Node.js process will no longer exit automatically upon receiving the signal [1][2]. In this scenario, it becomes the responsibility of the registered listener to handle the signal, which often involves performing necessary cleanup tasks and manually exiting the process if desired [3]. It is important to note the following platform-specific details: - SIGTERM is not supported on Windows [1][2]. - The behavior described above applies to non-Windows platforms [1][2]. - SIGKILL, another common signal, cannot have a listener installed and will always cause the process to terminate unconditionally on all platforms [1][2].
Citations:
- 1: https://nodejs.org/api/process.html
- 2: https://nodejs.org/api/process.md
- 3: https://stackoverflow.com/questions/65677272/what-is-the-default-behaviour-of-node-js-on-receiving-sigterm
🌐 Web query:
Bun process SIGTERM signal handler default action documentation
💡 Result:
The default action for a SIGTERM signal in the Bun runtime depends on whether a JavaScript listener has been registered for that signal [1]. When no JavaScript listener is registered for SIGTERM, Bun follows the standard operating system disposition for POSIX signals [1]. In this state, the runtime restores the default signal handler (SIG_DFL) and re-raises the signal, which results in the immediate termination of the process with an exit code of 128 + signal number [1]. This behavior is designed to match the signal handling patterns found in Node.js [1]. If a JavaScript listener is registered using process.on("SIGTERM", callback), Bun executes the registered handler instead of the default termination behavior [1][2]. Developers can utilize this mechanism to perform graceful shutdowns [3][4]. In summary: - If no listener is registered: The process terminates by default (standard POSIX behavior) [1]. - If a listener is registered: The process runs the custom listener callback [1][2].
Citations:
- 1: fix: apply default signal disposition when no JS listeners are registered oven-sh/bun#27311
- 2: https://bun.com/docs/guides/process/os-signals
- 3: https://bun.com/reference/node/process
- 4: Bun does not support
SIGTERMlisteners oven-sh/bun#1657
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import signal
import subprocess
import sys
import time
listener_script = r"""
process.once('SIGTERM', () => {
process.stdout.write('handled\n');
});
process.stdout.write('ready\n');
setTimeout(() => {
process.stdout.write('alive-after-signal\n');
process.exit(0);
}, 250);
"""
child = subprocess.Popen(
['node', '-e', listener_script],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
bufsize=1,
)
ready = child.stdout.readline()
print(f'ready={ready.strip()}')
os.kill(child.pid, signal.SIGTERM)
stdout, stderr = child.communicate(timeout=2)
print(f'listener-code={child.returncode}')
print(stdout, end='')
if stderr:
print(f'listener-stderr={stderr}', file=sys.stderr)
reraised_script = r"""
const signal = 'SIGTERM';
process.once(signal, () => {
process.removeAllListeners(signal);
process.kill(process.pid, signal);
});
process.stdout.write('ready\n');
setTimeout(() => {}, 1000);
"""
child = subprocess.Popen(
['node', '-e', reraised_script],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
bufsize=1,
)
ready = child.stdout.readline()
print(f'ready={ready.strip()}')
os.kill(child.pid, signal.SIGTERM)
stdout, stderr = child.communicate(timeout=2)
print(f're-raise-code={child.returncode}')
print(stdout, end='')
if stderr:
print(f're-raise-stderr={stderr}', file=sys.stderr)
PYRepository: developerz-ai/ultimate
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import signal
import subprocess
import sys
listener_script = r"""
process.once('SIGTERM', () => {
process.stdout.write('handled\n');
});
process.stdout.write('ready\n');
setTimeout(() => {
process.stdout.write('alive-after-signal\n');
process.exit(0);
}, 250);
"""
child = subprocess.Popen(
['node', '-e', listener_script],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
bufsize=1,
)
ready = child.stdout.readline()
print(f'ready={ready.strip()}')
os.kill(child.pid, signal.SIGTERM)
stdout, stderr = child.communicate(timeout=2)
print(f'listener-code={child.returncode}')
print(stdout, end='')
if stderr:
print(f'listener-stderr={stderr}', file=sys.stderr)
reraised_script = r"""
const signal = 'SIGTERM';
process.once(signal, () => {
process.removeAllListeners(signal);
process.kill(process.pid, signal);
});
process.stdout.write('ready\n');
setTimeout(() => {}, 1000);
"""
child = subprocess.Popen(
['node', '-e', reraised_script],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
bufsize=1,
)
ready = child.stdout.readline()
print(f'ready={ready.strip()}')
os.kill(child.pid, signal.SIGTERM)
stdout, stderr = child.communicate(timeout=2)
print(f're-raise-code={child.returncode}')
print(stdout, end='')
if stderr:
print(f're-raise-stderr={stderr}', file=sys.stderr)
PYRepository: developerz-ai/ultimate
Length of output: 245
Restore default signal termination after shredding. Registered SIGTERM listeners suppress Bun’s default exit, so a supervisor can leave edit awaiting io.launch until SIGKILL. Re-raise the same signal after shred, and remove the matching handler in finally.
🤖 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 `@packages/cli/src/cmd-secrets.ts` around lines 159 - 162, Update the signal
handlers around shred so SIGINT and SIGTERM each restore the signal’s default
termination behavior by re-raising the same signal after cleanup. In the edit
flow’s finally block, remove the matching handlers to avoid retaining listeners
once io.launch completes, while preserving shred’s cleanup behavior.
| test('an unwritable embedded root is X_STORAGE_UNWRITABLE, not a bare EROFS', () => { | ||
| // 0o500 = r-x: the directory exists and mkdir inside it is refused, which is what a | ||
| // readOnlyRootFilesystem does to the app root. The old code let Bun's own EROFS escape with | ||
| // no code, no fix, and no mention of storage. | ||
| const parent = mkdtempSync(join(tmpdir(), 'x-storage-ro-')); | ||
| const denied = join(parent, 'nested'); | ||
| mkdirSync(denied, { recursive: true }); | ||
| chmodSync(denied, 0o500); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
This case fails when the suite runs as root. 0o500 does not stop uid 0, so mkdirSync succeeds, startStorage returns a Storage, and line 373 fails. Container CI commonly runs as root, so the failure reports the runner rather than the code. Skip the case when the process is root.
🐛 Proposed fix
- test('an unwritable embedded root is X_STORAGE_UNWRITABLE, not a bare EROFS', () => {
+ // Root ignores the mode bits, so the only honest options are to skip or to lie about coverage.
+ const asUser = process.getuid?.() === 0 ? test.skip : test;
+ asUser('an unwritable embedded root is X_STORAGE_UNWRITABLE, not a bare EROFS', () => {📝 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.
| test('an unwritable embedded root is X_STORAGE_UNWRITABLE, not a bare EROFS', () => { | |
| // 0o500 = r-x: the directory exists and mkdir inside it is refused, which is what a | |
| // readOnlyRootFilesystem does to the app root. The old code let Bun's own EROFS escape with | |
| // no code, no fix, and no mention of storage. | |
| const parent = mkdtempSync(join(tmpdir(), 'x-storage-ro-')); | |
| const denied = join(parent, 'nested'); | |
| mkdirSync(denied, { recursive: true }); | |
| chmodSync(denied, 0o500); | |
| // Root ignores the mode bits, so the only honest options are to skip or to lie about coverage. | |
| const asUser = process.getuid?.() === 0 ? test.skip : test; | |
| asUser('an unwritable embedded root is X_STORAGE_UNWRITABLE, not a bare EROFS', () => { | |
| // 0o500 = r-x: the directory exists and mkdir inside it is refused, which is what a | |
| // readOnlyRootFilesystem does to the app root. The old code let Bun's own EROFS escape with | |
| // no code, no fix, and no mention of storage. | |
| const parent = mkdtempSync(join(tmpdir(), 'x-storage-ro-')); | |
| const denied = join(parent, 'nested'); | |
| mkdirSync(denied, { recursive: true }); | |
| chmodSync(denied, 0o500); |
🤖 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 `@packages/cli/src/dev-runtime.test.ts` around lines 356 - 363, Update the test
for an unwritable embedded root by skipping it when the current process runs as
root, before creating or chmod’ing the temporary directory. Preserve the
existing assertions and storage setup for non-root processes so the test
continues validating the X_STORAGE_UNWRITABLE behavior.
| endpoint: binding.url, | ||
| ...(env['S3_REGION'] === undefined ? {} : { region: env['S3_REGION'] }), | ||
| // MinIO needs path style; R2 and AWS do not. Declared, never sniffed from the endpoint. | ||
| forcePathStyle: env['S3_FORCE_PATH_STYLE'] === '1', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
S3_FORCE_PATH_STYLE=true is silently false. Only the literal 1 enables it. Against MinIO that produces a virtual-hosted request with no diagnostic, which contradicts the "declared, never sniffed" comment above it. Accept the usual truthy spellings, or reject unknown values with a coded error.
🐛 Proposed fix
- forcePathStyle: env['S3_FORCE_PATH_STYLE'] === '1',
+ forcePathStyle: ['1', 'true', 'yes'].includes(
+ (env['S3_FORCE_PATH_STYLE'] ?? '').toLowerCase(),
+ ),📝 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.
| forcePathStyle: env['S3_FORCE_PATH_STYLE'] === '1', | |
| forcePathStyle: ['1', 'true', 'yes'].includes( | |
| (env['S3_FORCE_PATH_STYLE'] ?? '').toLowerCase(), | |
| ), |
🤖 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 `@packages/cli/src/dev-runtime.ts` at line 133, Update the S3 configuration
parsing near forcePathStyle so S3_FORCE_PATH_STYLE=true and other usual truthy
spellings enable path-style requests, rather than only the literal "1";
alternatively, validate the value and reject unknown spellings with a coded
error. Preserve explicit configuration semantics and avoid silently treating
declared truthy values as false.
| async function call(routes: readonly Route[], init: CallInit = {}): Promise<Response> { | ||
| const route = routes[0]; | ||
| expect(route?.path).toBe(`${STORAGE_BASE_PATH}/:disk/*key`); | ||
| if (route === undefined) throw new Error('no route'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Bare Error at line 50. The rule holds in tests too: never throw new Error, subclass UltimateError with a code, a cause, and a runnable fix:. Here the throw is only type narrowing, so delete it.
As per coding guidelines: "Never throw a bare Error; subclass UltimateError with a stable X_SCREAMING_SNAKE code, cause, and executable fix: instruction."
🐛 Proposed fix
async function call(routes: readonly Route[], init: CallInit = {}): Promise<Response> {
- const route = routes[0];
- expect(route?.path).toBe(`${STORAGE_BASE_PATH}/:disk/*key`);
- if (route === undefined) throw new Error('no route');
+ const [route] = routes;
+ expect(route?.path).toBe(`${STORAGE_BASE_PATH}/:disk/*key`);
+ expect(route).toBeDefined();
+ const matched = route as NonNullable<typeof route>;Then use matched.handler(...) at the return.
🤖 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 `@packages/cli/src/dev-storage.test.ts` around lines 47 - 50, Remove the
type-narrowing `throw new Error('no route')` from `call`; instead, narrow the
route through an assertion or equivalent control flow and use the narrowed
`matched` route when invoking its handler. Do not introduce a bare error or
unrelated error subclass.
Source: Coding guidelines
| export function findMasterKey( | ||
| root: string, | ||
| env: EnvRecord = process.env, | ||
| ): MasterKeyRef | undefined { | ||
| const fromEnv = env[SECRETS_KEY_ENV]; | ||
| if (fromEnv !== undefined && fromEnv.trim().length > 0) { | ||
| return { hex: fromEnv.trim(), source: 'env', at: SECRETS_KEY_ENV }; | ||
| } | ||
| const path = masterKeyPath(root); | ||
| if (!existsSync(path)) return undefined; | ||
| return { hex: readFileSync(path, 'utf-8').trim(), source: 'file', at: path }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A filesystem error here escapes as a bare Error.
existsSync passes, then readFileSync can still throw EACCES, EISDIR or ELOOP — a key file owned by another user is the common one. That error has no X_* code, no cause and no fix, so the boot failure is unreadable to an agent. The same hole exists at line 72 in readSecretsFile.
Wrap both reads and re-throw a typed error, for example SecretsKeyInvalidError / SecretsFileInvalidError with sourceError set.
As per coding guidelines: "Errors | subclass UltimateError; never throw new Error" for packages/core/**/*.ts.
🤖 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 `@packages/core/src/secrets-store.ts` around lines 45 - 56, Update
findMasterKey and readSecretsFile to catch filesystem read failures and rethrow
the appropriate typed SecretsKeyInvalidError or SecretsFileInvalidError,
preserving the original exception through the sourceError field. Ensure both
errors extend the established UltimateError hierarchy and expose the expected
X_* code and remediation details instead of allowing bare filesystem Errors to
escape.
Source: Coding guidelines
| /** Write the master key at 0600. Callers must have made the ignore rule true first. */ | ||
| export function writeMasterKeyFile(root: string, keyHex: string): string { | ||
| const path = masterKeyPath(root); | ||
| writeFileSync(path, `${keyHex}\n`, { encoding: 'utf-8', mode: SECRETS_KEY_MODE }); | ||
| return path; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Node.js fs.writeFileSync mode option apply when the file already exists?
💡 Result:
No, the mode option in Node.js fs.writeFileSync (and related file-writing methods) does not apply when the file already exists [1][2][3]. The mode option is only used to set the file permissions and sticky bits when a new file is created [1][2][3]. If the file already exists, its existing permissions remain unchanged [1][2][3].
Citations:
- 1: https://nodejs.org/api/fs.md
- 2: https://beta.docs.nodejs.org/fs.html
- 3: https://nodejs.org/docs/latest-v24.x/api/fs.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n packages/core/src/secrets-store.ts | sed -n '1,140p'
printf '%s\n' '--- related call sites and tests ---'
rg -n -C 3 'writeMasterKeyFile|SECRETS_KEY_MODE|secrets rotate|master key' packages/core packages/cli --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' 2>/dev/null || true
printf '%s\n' '--- package guidance ---'
rg -n -C 2 'secrets-store|Bun-only|node:fs|chmod|0600' CLAUDE.md AGENTS.md packages/core 2>/dev/null || trueRepository: developerz-ai/ultimate
Length of output: 31448
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
source = Path('packages/core/src/secrets-store.ts').read_text()
cli = Path('packages/cli/src/cmd-secrets.ts').read_text()
write = re.search(
r'export function writeMasterKeyFile\(.*?\n\}',
source,
re.S,
)
assert write, 'writeMasterKeyFile was not found'
body = write.group(0)
print('writeMasterKeyFile has writeFileSync:', 'writeFileSync(' in body)
print('writeMasterKeyFile reapplies permissions:', bool(re.search(r'\bchmodSync\s*\(', body)))
print('writeMasterKeyFile passes mode:', bool(re.search(r'\bmode:\s*SECRETS_KEY_MODE\b', body)))
print('rotate calls writeMasterKeyFile:', bool(re.search(
r'writeMasterKeyFile\s*\(\s*root\s*,\s*next\.hex\s*\)', cli
)))
print('overwrite test present:', 'overwrite' in Path('packages/core/src/secrets-store.test.ts').read_text())
PYRepository: developerz-ai/ultimate
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 2 'overwrite|existing|0600|writeMasterKeyFile' packages/core/src/secrets-store.test.tsRepository: developerz-ai/ultimate
Length of output: 1116
Reapply restrictive permissions when rewriting the key
writeFileSync applies mode only when it creates a file. x secrets rotate rewrites the existing .secrets.key, so loose permissions remain unchanged. Call chmodSync(path, SECRETS_KEY_MODE) after writeFileSync, and add an overwrite case to packages/core/src/secrets-store.test.ts.
🤖 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 `@packages/core/src/secrets-store.ts` around lines 86 - 91, Update
writeMasterKeyFile to call chmodSync(path, SECRETS_KEY_MODE) after writeFileSync
so rewritten master-key files always regain restrictive permissions. Add an
overwrite test in the secrets-store test suite that verifies existing key files
receive SECRETS_KEY_MODE.
| /** Truncated to 64 bits: enough to name a key, far too little to attack the 256-bit key behind it. */ | ||
| export const SECRETS_KEY_ID_LENGTH = 16; | ||
|
|
||
| /** A secret's name is the env var it becomes — there is no second namespace. */ | ||
| export const SECRET_NAME = /^[A-Z][A-Z0-9_]*$/; | ||
|
|
||
| const HEX_KEY = /^[0-9a-f]+$/; | ||
| const KEY_ID = /^[0-9a-f]{16}$/; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Project the key-id length; do not restate it.
SECRETS_KEY_ID_LENGTH is 16, and KEY_ID restates {16}, and line 234 restates 16-character. Three copies of one fact drift. Derive the regex and the message from the constant.
♻️ Derive the pattern from the constant
-const KEY_ID = /^[0-9a-f]{16}$/;
+const KEY_ID = new RegExp(`^[0-9a-f]{${SECRETS_KEY_ID_LENGTH}}$`);- throw new SecretsFileInvalidError({ at, reason: 'has no 16-character hex "kid" field' });
+ throw new SecretsFileInvalidError({
+ at,
+ reason: `has no ${SECRETS_KEY_ID_LENGTH}-character hex "kid" field`,
+ });As per path instructions: "Define once, project everywhere — a fact stated in two places will drift; generate it."
Also applies to: 232-235
🤖 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 `@packages/core/src/secrets.ts` around lines 21 - 28, Update the key-id
validation around KEY_ID and the related error message near the referenced
validation logic to derive both the regex length and human-readable length from
SECRETS_KEY_ID_LENGTH. Remove the hard-coded 16-character values while
preserving the existing validation behavior and message intent.
Source: Path instructions
| $ x secrets init --json | ||
| {"ok":true,"command":"secrets","summary":"sealed secrets.enc.json — master key 4f2a…", | ||
| "data":{"path":"secrets.enc.json","keyPath":".secrets.key","keyId":"4f2a…","gitignore":"added"}} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The sample --json summary does not match the catalog string.
cli.secrets.init in packages/cli/src/messages.ts Line 106 renders sealed {path} — master key {kid}, and .gitignore now covers the key file. The sample stops after the key id and drops the .gitignore clause.
Nothing catches this. The wiki has no build or validation step, and x verify reads only wiki/Error-Codes.md, so the sample can drift from the catalog without failing the gate. An agent that pattern-matches this page against real output gets a miss.
📝 Proposed fix: quote the rendered summary
$ x secrets init --json
-{"ok":true,"command":"secrets","summary":"sealed secrets.enc.json — master key 4f2a…",
+{"ok":true,"command":"secrets",
+ "summary":"sealed secrets.enc.json — master key 4f2a…, and .gitignore now covers the key file",
"data":{"path":"secrets.enc.json","keyPath":".secrets.key","keyId":"4f2a…","gitignore":"added"}}Based on the learning that the repository wiki is plain Markdown synchronized to GitHub with no documentation build or validation step, and that x verify reads only wiki/Error-Codes.md from the wiki.
📝 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.
| $ x secrets init --json | |
| {"ok":true,"command":"secrets","summary":"sealed secrets.enc.json — master key 4f2a…", | |
| "data":{"path":"secrets.enc.json","keyPath":".secrets.key","keyId":"4f2a…","gitignore":"added"}} | |
| $ x secrets init --json | |
| {"ok":true,"command":"secrets", | |
| "summary":"sealed secrets.enc.json — master key 4f2a…, and .gitignore now covers the key file", | |
| "data":{"path":"secrets.enc.json","keyPath":".secrets.key","keyId":"4f2a…","gitignore":"added"}} |
🤖 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 `@wiki/CLI-Reference.md` around lines 399 - 401, Update the `secrets init
--json` example in the wiki so its `summary` exactly matches the string rendered
by `cli.secrets.init` in `packages/cli/src/messages.ts`, including the
`.gitignore` clause after the master key ID. Preserve the existing JSON
structure and ensure the displayed output uses the correctly quoted, complete
summary.
Source: Learnings
| | `X_ROUTE_UNNORMALIZED` | a route was registered without `defineRoute` | `registerRoute({ config })` was handed the author's own object, so `meta` and `budget` were never normalized and every descriptor reader would read them wrong | wrap it: `registerRoute({ file, config: defineRoute({ … }) })` | | ||
| | `X_ROUTE_DUPLICATE` | two route files resolve to one URL | a copied page directory | delete or rename one | | ||
| | `X_ROUTE_FILE_INVALID` | a route file is not named for its surface | `site/pricing.tsx` or `site/blog/index.tsx` instead of `<dir>/page.tsx` | `mkdir -p <dir> && git mv <file> <dir>/page.tsx` — `route.ts` under `api/` | | ||
| | `X_STORAGE_UNWRITABLE` | the storage disk this process needs cannot be written to | a hardened container (`readOnlyRootFilesystem: true`) with no writable volume at the embedded disk's root, or `S3_ENDPOINT` set with `S3_BUCKET` empty | mount a writable volume at the path in the message, or set `S3_ENDPOINT` + `S3_BUCKET` to use object storage. `x doctor --json` shows which binding this process resolved | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
X_STORAGE_UNWRITABLE is in the wrong table.
This row sits in "Routes, render and budgets", between two X_ROUTE_* rows. Every other X_STORAGE_* code is in the "Storage" table at lines 232-245. Move the row there so the section groups all storage codes.
📄 Move the row
| `X_ROUTE_FILE_INVALID` | a route file is not named for its surface | `site/pricing.tsx` or `site/blog/index.tsx` instead of `<dir>/page.tsx` | `mkdir -p <dir> && git mv <file> <dir>/page.tsx` — `route.ts` under `api/` |
-| `X_STORAGE_UNWRITABLE` | the storage disk this process needs cannot be written to | a hardened container (`readOnlyRootFilesystem: true`) with no writable volume at the embedded disk's root, or `S3_ENDPOINT` set with `S3_BUCKET` empty | mount a writable volume at the path in the message, or set `S3_ENDPOINT` + `S3_BUCKET` to use object storage. `x doctor --json` shows which binding this process resolved |
| `X_ROUTE_LOAD_INVALID` | a route declared a `load` that is not a function | `load: someData` instead of `load: () => someData` | make `load` a function of `({ params, url })` returning the page data, or remove it |Add the same row after X_STORAGE_UPLOAD_FAILED at line 245.
🤖 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 `@wiki/Error-Codes.md` at line 269, Move the X_STORAGE_UNWRITABLE row from the
“Routes, render and budgets” table into the “Storage” table, placing it
immediately after X_STORAGE_UPLOAD_FAILED. Preserve the row’s existing
description, causes, and remediation text unchanged.
|
🚀 Shipped v1.5.0 — encrypted secrets, a served 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
1 participant
Storage was silently local
startStoragenever read an external binding.s3Driver()existed and nothing called it, so an S3- or R2-configured deploy wrote to a container-local disk the next restart destroyed — a data-loss bug with no symptom until the restart.It now honours the binding. The embedded path fails at boot with
X_STORAGE_UNWRITABLE, naming both real options (a writable volume, orS3_ENDPOINT+S3_BUCKET), instead of the bareEROFSout of Bun'smkdirSyncthat CrashLooped areadOnlyRootFilesystem: truecontainer 22 times with nothing in the log that mentioned storage.grantUploadminted URLs nothing answered/_storageis mounted now — inx devand inserve.ts— withisTenantScopeddeciding what a signed key may reach.Secrets
x secrets init | edit | show: an encrypted file committed with the repo, a master key that is not. The two halves ofx secrets editthat belong to the terminal are the CLI's (X_SECRETS_EDITOR_MISSING,X_SECRETS_EDIT_FAILED); every code about the envelope itself is@ultimat3/core's.A gate defect the 500-line ceiling exposed
packages/cli/src/errors.tshit 528 lines and split — table intoerror-codes.ts, classes intoerrors.ts, the shape@ultimat3/corealready uses.That surfaced a real bug:
isCodeRegistrytested the filename, so the classes-onlyerrors.tsstill counted as its package's registry and outranked the package that owns the code.X_NOT_IMPLEMENTEDmoved fromcoretocliinframework.manifest.jsonthe moment the file split, and nothing butscripts/manifest.test.tswould have caught it.It now reads the file's own
export const <PKG>_ERROR_{CODES,TITLES}declaration. That also corrects a misattribution predating the split:X_FORBIDDENispolicy's — its registry says so in a comment — and was credited toactionfor a bare literal in a??fallback.Gates
bun run verify— all 17 steps greenbun run scripts/reference-app-gate.ts— 10 of 17 pass, 7 pinned, ratchet holds🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
x secretsfor initializing, viewing, editing, setting, and rotating encrypted secrets.Bug Fixes
Documentation