fix: v1 release readiness — authorization, data integrity, and dialect support - #64
Merged
Conversation
…estructive db ops The Permission union had no member for vault, secret, config-write, truncate, teardown, lock force or debug writes, so those operations had nothing to gate against and shipped ungated. Adds the missing permissions and their matrix rows so the owning modules can call assertPolicy.
…nstant Buffer.from(str, 'hex') never throws — it stops at the first invalid pair and truncates odd lengths. Every malformed private key therefore collapsed to a zero-length HKDF input, so deriveStateKey returned the same 32 bytes regardless of input: a constant recomputable from this source. State written under it was readable by anyone holding no key material at all. isValidKeyHex existed but was called only from the CI env path. Guard the three entry points that actually feed StateManager: deriveStateKey (the chokepoint every state read and write passes through), loadPrivateKey (a corrupted or partially-synced identity.key is otherwise undetectable, since nothing verifies it against identity.pub), and setKeyOverride.
SettingsManager merged every ambient NOORM_* var into #settings on load, and save() serialised that same object — so NOORM_VAULT_TOKEN, NOORM_DB_PASSWORD and NOORM_API_KEY were written verbatim into the git-tracked .noorm/settings.yml by any mutation, including an unrelated one. @logosdx/utils merge() mutates and returns its target, so there was no copy anywhere in the path. Split the persisted document from the resolved view: #document holds what goes to disk, #settings holds document + env overlay and backs every accessor. Mutators write the document and recompute the overlay.
needsMigration tested for an `identity` field migrateState deliberately never writes, so the predicate was permanently true and every command -- including read-only ones -- re-encrypted and rewrote the whole state file. migrateState also rebuilt State from a fixed allowlist, silently dropping any top-level field a newer version had added; the drop was persisted immediately, so a single downgrade was permanent loss. Carry unknown fields through instead, still dropping legacy `identity` so pre-move key material does not get re-persisted into state.enc.
MySQL has no RETURNING clause, so every build, run, change and revert died before executing a single file. Read the generated key off the insert result instead of issuing LAST_INSERT_ID() as a second query — that function is per-connection and Kysely pools connections between statements.
A file's prior success licensed a skip forever, so every apply->revert-> apply cycle reported success over an untouched database. Retire that success when the operation was reverted/torn down, or when an operation ran the other way since. `ff`/`next` also filtered `stale` out of pending work, so teardown had no recovery path even once the files would run. Both defects had to be fixed together to make `db teardown` -> `change ff` rebuild anything.
npm dist-tags.latest was interpolated verbatim. compareVersions parses loosely enough that junk still compares greater, so a poisoned tag reached the release URL — fetch normalises `..`, relocating the binary AND its checksums.txt to an attacker repo, where verification then passes.
`ci identity enroll` matched an existing identity on hash alone, then propagated the vault key to whatever public key that row held. The enrollment hash is SHA256(email\0name\0publicKey\0'env'), and the documented air-gapped flow circulates the bot's public key in the clear — so its only secret input is published by design. Anyone able to INSERT into the identities table, vault access not required, could pre-register the hash under their own key and receive the vault, while the operator's command reported success and echoed their own correct key back. Compare the stored public_key against the presented one and refuse on mismatch. Also validate --public-key up front: an invalid key was INSERTed before the propagation that then failed, leaving a row no retry could repair, under an error message promising an idempotent retry.
Overwriting a config rewrote its access block with no authorization check, so `config import escalate.json --force` promoted a viewer config to admin/admin in one command — and could flip the `mcp: false` invisibility an operator set so agents could not see the config. The config being replaced now decides, via the config:write permission: viewer denies, operator and admin require --yes.
The state encryption key is HKDF over the identity private key, so regenerating the keypair orphans every state.enc on the machine — configs, secrets and database passwords — with no recovery path and nothing that re-encrypts existing state. The only guard was --force, documented as "Overwrite existing identity". Require --yes on top of --force, copy the previous key files aside first (owner-only, timestamped), and say what is actually being destroyed. Uses the raw flag rather than isYesMode() so an ambient NOORM_YES set for unattended runs cannot destroy every project's state as a side effect.
Rotation renamed the file but kept the old fd, so every later entry went to the rotated file, the log path never reappeared, and needsRotation on a missing path stayed false — rotation fired once per process and the rotated file then grew unbounded. Log files are now created 0600, not 0644.
`run build` stored the raw file hash while `needsRun` compared the rendered hash, so no `.sql.tmpl` could ever match and every template re-executed on every build — failing outright on non-idempotent DDL. Rendered output is the canonical dedup key: it is what reaches the database, it is what `run file` already used, and hashing raw bytes would silently skip a template whose data file or secrets changed. Consequence to note: a checksum now derives from SQL that may embed a rendered secret. SHA-256 discloses nothing, but it is a guess oracle for anyone holding both DB read access and a candidate value.
`rewind` declared --dry-run/--force and advertised them in its examples but called the SDK with neither, so the flag whose contract is "touch nothing" reverted for real. The SDK had no options parameter to pass them through. The documented `rewind <N>` form also never worked: citty yields the positional as a string and the manager branches on typeof number, so a count fell through to name lookup and failed with no reason attached.
Init wrote a bare `# noorm` comment with no patterns under it, so nothing
was ignored, and both surfaces then skipped on `includes('# noorm')` —
the empty block could never be repaired. Write `.noorm/state/` (state.enc
plus the log file) and key the skip on that entry so existing projects get
repaired on the next init.
The test asserted only that the header landed, which passed against a
block that ignored nothing; it now asserts the entry.
It asserted nothing and printed the PID and NOORM_* key list into CI group 1's output.
getIdentityForConfig is what the SDK calls to decide noorm.change.executed_by, and it passed only the config's identity string. The identity that actually authenticated to the vault therefore never reached the audit trail: an enrolled CI bot's changes were recorded against the runner's git user or OS username, and a bare NOORM_IDENTITY env var — unauthenticated free text — outranked the cryptographic identity outright. The TUI passed cryptoIdentity and the CLI/SDK did not, so one command attributed differently per surface. Read the process-wide CI identity override here. An explicit config identity still wins, since that override is deliberate.
The hash encodes email|name|machine|os. `identity edit` spread the new values over the old record and saved it, leaving a hash that still meant the previous person — the value the database joins on and that vault access is granted against — while `identity list` displayed the new one. The TUI's edit screen already recomputed it, so one operation had opposite outcomes per surface. Route through createIdentityForExistingKeys, carrying `machine` over so the only hash inputs that move are the ones the user asked to change, and report the fingerprint change since it costs vault access.
The overlay merged in after parseSettings had run, so nothing validated it. NOORM_BUILD_INCLUDE=00_tables put a string in an array field, which the build walker then iterated character by character — it matched no files and still reported success with exit 0. Validate the merged view so a scalar-for-array fails loudly instead.
transferData's assertPolicy only rejects `allowed: false`, so a confirm-tier destination reached the screen's plain <Confirm> and was writable with one keypress while the CLI hard-blocked. Gate the write target (destination for db-to-db, active config for import) the way the SDK does, and route the confirm through SmartConfirm. - truncateFirst had a setter that was never called and no control, so the TUI always transferred with it false; it is now a visible toggle - globalModes.dryRun was never passed, so the DRY badge lied here
`canRevert` collapsed a broken history read into the same "no" as "already reverted", and the executor turned every such no into `status: success` with no files — so reverting against a damaged tracking table reported success over an untouched schema. Note SQLite never raises here: a double-quoted identifier that matches no column is re-read as a string literal, so the status comes back as garbage rather than an error. Both paths now carry an error.
request() awaited a response event with no rejection path, so a crashed compute thread left the caller pending forever. OrderBuffer likewise accepted indices that could never drain, stranding every later item.
LIMIT/OFFSET with no ORDER BY has no stable window: a write to the source between pages dropped and duplicated rows while still reporting the full count. The drain loops spun on a counter a failed dispatch never decremented, so one bad row hung the pipeline forever.
The debug screens delete rows from noorm.vault and noorm.identities, and nothing checked authorization on the way — a viewer config could drop vault rows. The gate goes at the core seam rather than in the screens so a second surface can't inherit the tables without inheriting the check. createDebugOperations now requires a policy context; reads take debug:read, deletes take debug:write. Bulk delete authorizes before the empty-list short circuit so a denied caller isn't handed a plausible-looking 0.
EXPLAIN was a terminal `read` verdict on both classification paths, so `EXPLAIN (ANALYZE) DELETE FROM t` deleted rows under a viewer role on postgres, via the CLI and over MCP. It now inherits the class of the statement it wraps, and the CST scan walks every nested statement node instead of an enumerated list that had no DDL entry. Quote tracking modelled `'` only, so an MSSQL bracket identifier holding an odd apostrophe swallowed the following `;` and hid a whole statement. One dialect-aware masking pass replaces the four independent scanners.
state.enc was rewritten whole from an in-memory snapshot with no lock and no atomicity, so two processes writing at once silently lost one of the writes -- ten parallel `secret set` runs left five secrets, all exiting 0. Writes now take an O_EXCL lock beside the file, stage into a temp file and rename over the target, and fsync both. A lock older than 30s is assumed to belong to a dead process and broken. Locking alone is not enough: the second writer still holds a stale snapshot, so it would serialize its own clobber. Before writing, a writer now compares the file against the fingerprint it last saw and, if it changed, reconciles three-way against the snapshot it loaded. Three-way is required to tell "we never touched this key" from "we deleted this key". Each write also copies the previous generation to state.enc.bak. There was no backup of any kind, so a damaged state.enc meant unrecoverable loss of every config, secret and DB password in the project.
`config export` handed over the connection password in plaintext with no authorization check — a viewer config that refused `config rm` would still print its own password to stdout and exit 0. It was the only config operation with no gate at all.
Neither stub declared a json arg, so a headless caller in the config group got prose on stderr where every sibling command returns JSON. Both now emit a JSON error and name `config import` as the headless route, which was already the working path but was documented nowhere.
enabled, file, maxSize and maxFiles were hardcoded, so every logging setting was dead for all ~79 commands and the Shift+L overlay read a path the CLI never wrote. Disabling logging now stops the file only — the Logger still carries --json result output.
Two ways untrusted code ran with no opt-in: - Any `.js`/`.mjs`/`.ts` beside a SQL file was imported while building the context, unreferenced, during `preview`, `inspect` and `--dry-run`. Scripts now load only when the template names their key; inert formats still auto-load. - `include()` and the `$helpers` walk tested containment with a string prefix, so a `<root>-evil` sibling counted as inside the project.
The existing suite only exercised roles the connection was already granted, against a container user that is a superuser — so postgres would have allowed any SET ROLE and the tests could not have failed on an escalation. Add a second context connected as an unprivileged login role to exercise a real membership check, plus a nonexistent principal and a username carrying SQL metacharacters.
A config with no explicit `access` gave the agent channel `admin`, so
every stock project let an MCP client write, run DDL, and drop databases
— making the rest of the matrix decorative there.
BREAKING CHANGE: default access is now `{ user: 'admin', mcp: 'viewer' }`.
Agent writes require an explicit `access.mcp` opt-in. Configs that already
store an explicit `access` are untouched.
performProjectInit writes it only when absent; the TUI's reimplementation overwrote it every time, discarding user edits.
db truncate and db teardown now require confirmation, and the SDK equivalent is options.yes. These tests invoked them bare and asserted success, which encoded the defect the gating fixes: teardown dropped 63 objects and exited 0 with no confirmation. Passing --yes/yes states the intent rather than restoring the old behaviour.
Enrollment ends in a vault grant, so it is a config-scoped action, but it called the ungated propagateVaultKeyTo and never consulted the config's access. A viewer config — denied vault:read outright — could still hand the vault to a new identity, and no role was ever asked to confirm a grant that cannot be revoked. Build the gate from ctx.noorm.config, the same source the vault commands use. Authorization is checked before any read or write, so a denied role cannot probe the identities table. Confirmation is asked later, once the target is known, so the operator sees the identity they are granting to — and is skipped when that identity already holds access, since confirming a no-op is just noise.
scope.revert() set `reverted` before awaiting revertFn, so a revert that threw still marked the scope done: the retry the error message invites returned early, the revert SQL never ran, and the pooled connection the scope holds was never released. Set the flag only once revertFn resolves, and share the in-flight promise so concurrent callers still run it once. Idempotency after a successful revert is unchanged. Explicit-mode scopes also held a pooled connection with nothing to release it on teardown, so a caller who never reverted — an early return, a thrown error, a forgotten call — left disconnect() awaiting a pool drain that could never finish. Track the holders and release them before destroy(). The revert SQL is skipped there deliberately: the pool is going away, so no later query can inherit the identity.
getUsersWithoutVaultAccess returns its own [users, err] tuple, but the vault screen still wrapped it in attempt(), nesting one tuple inside another. The branch did not typecheck at HEAD. Not this slice's file — the collision landed between two merges, and fixing it here was the only way to get a green typecheck to verify against.
The insert-and-get-id logic was copy-pasted into runner/tracker and twice into change/history. All three emitted RETURNING on MySQL, which has no such clause, so fixing one copy left the others shipping broken.
Compiles the insert per dialect and reads the statement back, so a RETURNING on MySQL or a per-connection LAST_INSERT_ID follow-up fails without needing a live server to notice.
Spec body, MCP guide, and both migration notes described the old admin/admin default. Changeset marks the break: MCP writes now need an explicit `access.mcp` opt-in.
getUsersWithoutVaultAccess returns [users, error] so a failed lookup cannot read as "nobody is missing access". VaultScreen still called it through attempt(), nesting one tuple inside another, and derived its recipient type from the union. Only surfaced once both branches landed.
Comments and wiki signals still named the superseded default and the old `OPEN_ACCESS` constant.
`2` meant partial failure on the run/change batch commands, "there was nothing to release" on `lock force`, and — on those same batch commands — total failure as well, since `status === 'success' ? 0 : 2` collapsed `failed` and `partial` into one code. A pipeline could not tell "retry this" from "a human has to look at the half-written database", so in practice everyone tested `!= 0` and threw the distinction away. `src/cli/_exit.ts` now fixes the meanings: 0 success, 1 total failure, 2 the invocation named something that isn't there, 3 partial. Confirmation and `--force` refusals deliberately stay at 1 rather than becoming usage errors, so the code means one thing across every command. BREAKING CHANGE: a script testing `[ $? -eq 2 ]` for a partial apply must now test `-eq 3`. "Named target does not exist" moves from 1 to 2.
Success payloads had four incompatible shapes — `{version,…}`,
`{configs}`, a bare array from the list commands, `{success,path}` — and
only errors were uniform, so `jq -e '.success'` was absent on most
successes and errored outright on the arrays. The exit code was the only
cross-command discriminator, and it was inconsistent too.
Every `--json` payload is now an object carrying a top-level boolean
`success`, injected once in `outputResult` rather than at ~70 call sites
that would drift. `success` is derived from the payload's `status` where
there is one, so a `partial` batch can no longer announce itself as a
success, and it always agrees with the exit code.
`config export --json` gets the envelope; its default output is still the
bare artifact `config import` reads, so `config export dev > dev.json` is
unchanged. `noorm update --json` stopped printing two documents for one
error and reporting `success: true` on a failed install.
BREAKING CHANGE: commands that returned a top-level array now return a
named object — `change list` → `.changes`, `change history` → `.history`,
`db explore` lists → `.tables` / `.views` / `.indexes` / `.foreignKeys` /
`.functions` / `.procedures` / `.types` / `.triggers`.
`run inspect sql/nope.sql.tmpl` returned a fully-populated context report and exit 0 for a file that was never on disk — `buildContext` only reads the template's *directory*, so nothing on that path ever checked. `run dir` had two variants of the same shape: a missing directory surfaced as a generic SQL failure, and a directory holding no SQL files came back as `status: 'success'` over zero files, which is how a mistyped path passes a pipeline silently. All three now fail with the usage code and a message naming the path. `run preview` gained the same up-front check so both template commands agree, and its `--json` error field carries the message only — the stack trace it used to emit embedded absolute filesystem paths.
`run exec` was the only command that hard-required the `Bun` global, so it died with `Bun is not defined` on the documented Node dev entry — and in the `tests/cli` harness, which spawns `node dist/cli/index.js`, which is why the command had no coverage at all. Glob expansion falls back to `node:fs/promises`; the compiled binary keeps Bun's own matcher so its matching semantics are untouched. `lock acquire` gained `--timeout` and `--reason`, which the TUI has collected since it shipped. Without them a CI-acquired lock was always default-timeout and reasonless, so whoever it blocked had no way to see what they were waiting on. Both are omitted rather than passed as `undefined`, because LockManager merges over its defaults and an explicit `undefined` erases them.
…ract The envelope was never specified anywhere, which is why four shapes could coexist without anyone noticing. `docs/headless.md` now states it, names what breaks for existing consumers, and the exit-code table carries all four codes instead of three. Stale `jq '.[]'` examples against the commands that used to return bare arrays are corrected in the guide, the dev docs, and the noorm skill.
…voked (#66) * feat(policy): recognise agent harnesses from the environment they export An agent denied a write over MCP can see noorm on the PATH and shell out; the CLI hardcoded the user channel, so that second attempt ran with the human's role. Detection is the input to resolving the channel from provenance rather than from which binary was invoked. Allowlist only, from variables the harnesses set for their own children. TERM_PROGRAM, CI and TTY state are deliberately excluded: they describe the terminal or the pipeline, not the caller, and a false positive locks an operator out of their own CLI. * feat(identity): stamp agent provenance on audit rows executed_by recorded who ran an operation and nothing about what drove the session, so an agent-applied change was indistinguishable from a human one — the first question asked when a migration goes wrong. Folded into the identity string rather than a dedicated column: the audit question is binary, and a suffix answers it on all four dialects without a schema migration or a CLI/database version skew window. * feat(cli): show detected agent harness in noorm info Harness detection silently changes the channel an operation is authorised on, and a permission denial with no visible cause is bad to debug. Reports the markers actually set, which is what an operator would unset to be treated as human. * chore(changeset): describe agent provenance in audit rows * fix(connection): connect to MSSQL by IP address Tedious guards the SNI ServerName against IP literals on its TDS 8.0 path but not on the PRELOGIN path `encrypt: true` takes, so Node rejected every IP connection. Encryption is never downgraded to work around it: validating a certificate against an IP now fails, naming the new tlsServerName config field as the fix. * feat(policy)!: resolve the access channel from who is driving Channel named the transport, so the CLI hardcoded `user` at every policy call site. An agent refused a write over MCP could see noorm on the PATH, shell out, and run the same operation with the human's role. Measured on a stock config: sql:write, sql:ddl, db:create, run:build and vault:read all went deny -> allow, and db:destroy dropped to a confirm that --yes satisfies. Channel is now 'user' | 'agent' and ConfigAccess is { user, agent }. The CLI resolves it via resolveChannel(): NOORM_CHANNEL when explicitly user/agent, else the agent-harness allowlist, else user. `mcp serve` still passes 'agent' literally, above the override. `agent: false` hides a config on both transports — `config list` now filters it the way `list_configs` already did. Stored access.mcp migrates to access.agent at state schema v3, verbatim. The TUI keeps a literal 'user': it needs a TTY and an interactive human. NOORM_CHANNEL is excluded from the NOORM_* config/settings env mapping, or it would invent a `channel` key on every config. BREAKING CHANGE: Config.access.mcp is now Config.access.agent, and createContext's channel option takes 'agent' instead of 'mcp'. Agents shelling out to the CLI now get the agent role rather than the human's.
…sertions fail These four assertions all reduce to "did createCliLogger read settings.yml", and a bare expected/received cannot tell a missing file from a stale singleton from a load that threw. They fail on CI and pass on macOS and Linux locally — including a faithful replay of the CI step order — so the failure needs to describe itself.
bun's mock.module registry is process-global and re-registering the real
module does not restore it — measured. Two init-screen tests replace the
SettingsManager class, so getSettingsManager constructs a mock instance and
createCliLogger reads settings: {} instead of settings.yml. Their afterAll
restores are no-ops.
Load order decides who wins: root files before subdirectories on macOS, the
reverse on Linux, which is why this passed locally and failed only on CI. A
separate process is the only reliable isolation, matching tests/core/transfer.
Also corrects CLAUDE.md: the documented config env-snapshot contamination
source does not reproduce (memoizeOpts: false), and mock.module restores are
recorded as ineffective.
The e2e step asserting that a bad build does not report success was passing for the wrong reason. Its fixture claimed an intentional syntax error from a missing comma, but 'username display_name' is a valid implicit alias, and the 'deleted_at' column it selected is accepted too since SQLite does not resolve a view's columns until the view is queried. Both verified against bun:sqlite. The file only ever failed because it held two statements and SQLite's prepare rejects the trailing one. Once the runner learned to split statements, both ran cleanly and the build passed. Replaced with a real parse error, and updated the expected code to 3 (partial: one file applied, one failed). Exit 2 now means a usage error.
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.
Twelve agents audited one vertical slice each and found 201 confirmed defects — 26 critical — with every test suite green throughout. Seventeen agents then fixed them. Full audit and per-domain reports in
.claude/.scratchpad/2026-07-29-domain-audit/.What was actually wrong
Authorization did not work as designed.
core/policyintends one gate at the core seam that every surface inherits. In practice each surface re-implemented it and several cells were empty:db createwas ungated on the CLI while the TUI refused it (reproduced independently by three agents), and vault and secrets had no permission type at all. On top of that, the SQL classifier readEXPLAIN (ANALYZE) DELETE FROM tas a read — so aviewerdeleted rows through the CLI and over a live MCP session.And underneath all of it: a config with no explicit
accessresolved tomcp: 'admin', so every stock project already handed agents full admin. The classifier bypass didn't need a bypass.Operations reported success over damage. Export lost 14,149 rows and duplicated 13,387 under concurrent writes, reporting
rowsExported: 50000.change runafter a revert: 44 files, 44 skipped, 0 objects created, exit 0.db teardown < /dev/nulldropped 63 objects and exited 0, against docs promising refusal. Nine of twelve agents found instances independently.MySQL did not work. Both the runner and the change module emitted
RETURNING, which MySQL has no support for — zero files executed, zero changes applied.Breaking changes
access.mcpdefaults toviewer. Theuserchannel is unchanged; agents keepexploreand read-only SQL. Configs with explicitaccessare preserved — including ones already holdingmcp: adminfrom an earlier migration, whichnoorm config listnow displays so you can lower them.--jsonenvelope. Commands that returned a bare array now return a named object (change list→.changes,db explore→.tables, etc.).jq '.[]'on those must name the key.0success,1total failure,2usage,3partial. Previously2meant partial and total.Verification
4,503 tests pass across all four CI groups, zero failures — up from 3,821. Roughly 680 new tests, each confirmed failing before its fix. Integration suites run against live postgres, mysql and mssql.
Known gaps
admin/adminon disk and are indistinguishable from a deliberate opt-in, so they are not narrowed.noorm config listsurfaces them.exit 4for policy denials was skipped: only 9 command files gate locally, the rest inherit an untypedError. Needs a typedPolicyErrorin core first.checksums.txthas never shipped — the generating step landed one day after the last release, so the checksum-verifyingpostinstallhas never run in production.build-binariesalso runs afterchangeset publish, leaving a window where the npm package exists without its assets. Worth resolving before tagging.