Repository navigation
Conversation
jsam
changed the title
…oints The workflow ran on every matching PR update and again after merge. It now runs on PRs carrying the release label, on release/** branches, on v* tags and on demand; make vc-gate covers local runs.
A failed commit whose cut-back also fails is repaired only in memory, so a crash before the next write or a clean shutdown recovers it. The guides and the append contract claimed a failed write is never recovered; correct them and pin the behaviour with a test.
… did converged_mutations counted every sent mutation, including those whose subscriptions diverged (as with --fault drop-retractions). Judge convergence per mutation across all subscriptions and time it from the converged arrivals only.
It runs formatting, clippy and the tests that apply (workspace tests plus the snapshot categories affected since the branch left main) in parallel, then the performance gate. CONTRIBUTING, TESTING.md and the PR template require it. test-affected.sh now joins categories with basic-grep alternation, so a change touching several categories selects them all instead of none.
…suite One release-checkpoint policy for every expensive check: both become full-suite jobs behind its release gate, and the separate Verified Completions workflow is removed. The fast PR gate stays format, clippy and unit + integration tests.
The gate applies to each push, not only to opening the PR. pre-pr also skips the workspace tests when no Rust input changed since the branch left main, so docs-only pushes run only the checks that apply.
load(path, mode) sends .load to the engine again instead of reading a client-local file; over the WebSocket the engine refuses it as client-only, which now rejects with QueryError(unsupported) rather than passing silently. The SDK guides also flag insert()'s newly-stored count as a behaviour change.
- auth: serialize every credential mutation (validation, storage and registry publication) so a key revoked mid-create cannot be published, and fail closed when a password or role change deletes the old record but cannot write the new one. - ws: make the outbound fence's final revocation check and the frame enqueue atomic with respect to revocation on every data-frame path. - sdk-js: drop the client-side vectorSearch rewrite and the implicit switch to the default knowledge graph before a drop; the server's typed error surfaces instead. - perf-gate: delta and interference writers are open loop, sending on schedule regardless of acknowledgements, with latency measured from the scheduled time. - make pre-pr routes changed paths to their own fast checks (Python SDK, JS SDK, perf-gate tool, Rust tests and snapshots) in parallel before the performance gate. - The mid-proof revocation test waits for the revocation to land before judging the reply.
They run in the full suite at release checkpoints; the fast gate stays format, clippy and unit + integration tests.
Main's atomic programs report a rejected insert as 'Insert rejected for <relation>' and no longer re-export the schema types through the handler module.
…ergence accounting
…e orphaned grants
…come replacement docs
jsam
changed the title
Contributor
Author
|
Superseded by #225 (same fixes rebased onto main, merged with CI green). |
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
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.
Intent
Do not run CI redundantly; it is okay to move quickly and break things, then run QC at certain checkpoints. Call them release checkpoints, where the entire CI and QC verification runs. To move quickly, run and test things locally, make sure the relevant checks run in parallel before making the commit, and only run the checks that make sense, not everything. The performance policy should be done as a pipeline which is always run before a PR commit. Make sure the reactive expectations are part of the e2e quality checks pipeline. Always follow clean code principles and clean code architecture. After every big PR run QC agents that verify the issue or feature has been fully implemented or resolved. Documentation changes needed also have to always be kept up to date. Never put Claude/AI co-author or generated-with trailers in commits or PRs.
This change is the checkpoint-1 review fixes on top of main. Main already contains the release checkpoint 1 items (data-dir lock, SDK typed errors, credential revocation, capped subscriptions, recursive-read fix, perf gate, e2e reactive pipeline, GenBI benchmark, differential oracle, Verified Completions gate, transaction WAL) and now also API key expiry/rotation (#204); this change adds only the fixes their review required: the revoked-credential race (credential mutations serialized; a failed password/role replacement revokes everything, including persisted keys, so nothing rebinds after restart), an atomic revocation check at the outbound frame fence, removal of the JS SDK's client-side vectorSearch rewrite, the implicit switch to the default KG before a drop and the local-file load (the server-file .load API is restored), an open-loop perf-gate writer, the GenBI converged-mutation count (missing truth is not convergence), the WAL rollback-failure contract text with its test, CI gating of the perf-gate tool checks and the Verified Completions gate to release checkpoints (full-suite only, off the fast PR gate), and make pre-pr, which requires the merge base with origin/main (or an explicit PRE_PR_BASE), routes changed paths to their own fast checks in parallel (reusing the unit-test target) and then runs the perf gate before every push to a PR. When rebasing onto main, keep #204's credential design (expiry, rotation grace, principal module) and re-apply these fixes on top of it. The post-commit flush durability defects in the WAL commit path are fixed separately against main's code, not here.
What Changed
ws/outbound.rs), the server checks that the credential is still live using a newPrincipal::admit. That check is atomic with revocation and with expiry being brought forward. Admin bootstrap now writes abootstrap_keysrecord, so it never issues a second API key, and that record is backfilled for existing data directories. Tests cover all of this incredential_mutation_tests.rsandtests/credential_revocation_tests.rs. The WAL rollback-failure contract is now documented, with a test.vectorSearchsendstop_k/within_radiusqueries to the server again instead of sorting and slicing results in the client.dropKnowledgeGraphno longer switches todefaultfirst.load(path, mode?)is back to the server-file.loadcommand. The perf-gate writer is now open-loop: it sends on a fixed schedule and reads replies concurrently. GenBI counts a mutation as converged only when every agent converged, so missing truth no longer counts as convergence.full-suite.yml, which runs at release checkpoints. The standaloneverified-completions.ymlworkflow is deleted. A newmake pre-prtarget needs a merge base withorigin/main(or an explicitPRE_PR_BASE). It sends each changed path to its own fast checks in parallel (fmt, lint,unit-test, affected snapshots, Python/JS tests, perf-gate checks), then runs the perf gate. A newscripts/test_pre_pr.pytests that routing. CONTRIBUTING, TESTING.md, the PR template and the SDK, auth and persistence guides are updated to match.Risk Assessment
✅ Low: The latest fix round only moved a doc comment back to
fn now_ms, gave BOOTSTRAP_KEY_LABEL an accurate doc, and qualified the docs sentence. That sentence now matches replace_user_row's CommitError::Unknown handling and CommitError::Unknown's documented semantics, and it introduces no behavioral change.Testing
I drove the change against real server processes in isolated /tmp data directories, which I deleted afterwards. - Credentials over WebSocket (all checks passed): - A password change ends only the password session, with a credential_revoked notice and close. - A role change takes effect on the next request without closing the session. - Revoking your own key withholds the reply. - A dropped and recreated user does not get its old keys or grants back. - A revoked bootstrap key stays rejected after restart, even with INPUTLAYER_BOOTSTRAP_API_KEY still set. This includes an upgrade from a data directory created by the base (main) server. - JS SDK integration suite on a live server: 46 of 47 pass, including the typed rejections for vectorSearch,.loadand dropping the current graph. The one failure (unsupportedvsnot_found) reproduces identically on the base server, so it predates this change. - make pre-pr: the routing tests pass, and the real Makefile refuses a missing, invalid or self base before running any check. - Targeted Rust tests: the credential mutation, outbound fence, WAL rollback, credential-revocation and perf-gate/GenBI tests all pass. These are in-process tests, so the scenarios they cover are marked untested live. - Perf gate: a quick delta_single/interference run completed with its correctness checks. The full releasemake perf-gatecould not run because the host disk was nearly full (as little as 127 MB free), filled by other processes. - Not driven live: the failed-replacement fault injection, the outbound-fence revocation race, the GenBI benchmark (needs an external genbi-trust checkout), the WAL crash-after-failed-cut-back case, and the GitHub Actions gating (needs GitHub's runners). For the workflows I parsed the YAML instead.Evidence: Live WS credential session scenarios (candidate server)
Evidence: Bootstrap key revoke + restart (supplied key never rebinds)
Evidence: Upgrade from base-server data dir: backfill blocks re-issue of revoked bootstrap key
Evidence: JS SDK integration suite against live candidate server
Evidence: Same partial-failure JS test failing on base (main) server
Evidence: make pre-pr base guard transcript
Evidence: pre-pr routing tests
Evidence: Targeted Rust test results
Evidence: Perf-gate quick delta/interference run
Evidence: Perf-gate run file
Evidence: CI workflow gating (parsed YAML)
~/.no-mistakes/evidence/01M41XHZ3S11JK8J4VFFXZSJBA/drive.mjs)Pipeline
Updates from git push no-mistakes
... (6 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
🔧 **Review** - 3 issues found → auto-fixed (3) ✅
🔧 Fix applied.
7 issues (4 warnings, 3 infos) still open:
src/protocol/handler.rs:1344- The Argon2 password hash is now computed while the global storage write lock is held.handle_user_createacquiresself.storage.write()at :1339 and only then evaluatesauth::hash_password(password)?at :1344.handle_user_passworddoes the same: it takes the write lock at :1485 and hashes at :1509. The base code held a read lock here, so other readers kept running. Now every query and write on every knowledge graph stalls for the full Argon2 run, which is tens of milliseconds in release builds and much longer in debug builds, on each.user createor.user password. The hash depends only on the plaintext, not on any locked state. Fix: compute it before taking the lock (or beforecreate_user) so the serialized section covers only validation, persistence and publication.src/protocol/handler.rs:1017- Bootstrap now issues the API key only when_internaldid not exist before this boot.create_api_key = !list_knowledge_graphs().contains(_internal)also suppresses the key in a case that has nothing to do with recovering a revoked admin. Example: on first boot,ensure_knowledge_graphcreates_internal, then savingcredentials.tomlfails with a generated password (orhash_passwordorcreate_userfails), and the function returns before any user exists. On the next boot_internalexists with no users, so a fresh random key is generated, saved tocredentials.tomland printed as 'Admin API key: ****xxxx', but it is never stored. The operator is handed a key that cannot authenticate. The same happens when an operator deletescredentials.tomlduring recovery: a new key is generated and printed but not issued. Only a supplied or persisted plaintext can match a revoked key; a freshly generated one cannot. A narrower rule would suppress issuance only for supplied or persisted keys when recovering an existing graph and still issue newly generated keys. That changes the documented rule in authentication.mdx ('Automatic key issuance happens only when the internal auth graph is first created'). Alternatively, do not generate, save or print a key that will not be issued. Either remedy changes documented bootstrap behaviour, so it needs your decision.src/protocol/handler.rs:1393-create_userdeletes a recreated user's orphanedapi_keysrows but not the old user'skg_aclsgrants. A failed password or role replacement deletes theusersrow and stops (the failure paths are at :1522 and :1579). The ACL cleanup exists only inhandle_user_drop(:1468), so the old user'skg_aclsrows stay. Example: bob has owner on KGfinance;.user role bob viewerfails after the delete; an admin then runs.user create bob pw viewer. The new bob silently inherits owner onfinance. Keys are cleaned at this boundary but grants are not. The code and docs do not say whether a recreated username should inherit orphaned per-KG grants. Shouldcreate_useralso delete orphanedkg_aclsrows in the same transaction, or is inheritance intended?src/protocol/handler.rs:1047- This fix round introduced the problem. The F2 change now decides bootstrap key issuance only from the newbootstrap_keysrelation, and nothing backfills that relation for data directories created before this change. Those directories include every deployment of main/auth: API key expiry, rotation grace and batched last-used (#49) #204, whose_internalalready holds an admin user and a bootstrap API key but nobootstrap_keysrows. Failing sequence: (1) such a deployment upgrades; (2) the operator revokes thebootstrapkey, as the docs recommend; (3) a later.user password admin …or.user role admin …deletes the admin row and then fails to insert the replacement, so the admin row and its keys are deleted; (4) on restart,seed_admin_credentialsfinds no users and nobootstrap_keysrows, soissue_api_keyis true.resolvethen returns the old plaintext fromcredentials.toml, or from INPUTLAYER_BOOTSTRAP_API_KEY if it is still set. The server stores the same hash again, so the revoked key authenticates as admin. The previous rule (issue only when_internalis first created) blocked this, so the round moved the R1/rebase-2 invariant ('nothing rebinds after restart, including via the admin bootstrap') rather than closing it. A smallest fix: on boot, when the admin user exists andbootstrap_keysis empty, record the hash of the supplied or persisted bootstrap key. Alternatively, treat a pre-existing_internalwith nobootstrap_keysrows as already issued. Both add a one-time migration write, so the remedy needs authorization. The new testpartial_first_boot_issues_a_working_key_on_the_next_bootand the admin case oforphaned_keys_cannot_rebind_after_recreation_or_bootstrapstart from a fresh data directory, so neither exercises this path.src/protocol/handler.rs:1172- This is a gap left by the F2 fix round. The new key is saved tocredentials.tomland printed (masked) at :1093-1110, before thebootstrap_keysrecord (:1172) andstore_api_key(:1188) succeed. If either insert fails, the operator still gets a printed, saved key that cannot authenticate. The operator's F2 decision says 'never generate, print or save a key that was not stored'. The next boot will not repair this because the admin user already exists. Fix: print the key only after the key row is stored, or remove the saved key fromcredentials.tomlwhen an insert fails. Reaching this needs a storage failure right after the admin insert succeeds, so it is rare.src/protocol/handler.rs:245- This fix round insertedBOOTSTRAP_KEY_LABEL,record_bootstrap_keyandstore_bootstrap_keybetween the doc comment/// Current epoch milliseconds.andfn now_ms. As a result,BOOTSTRAP_KEY_LABELnow carries a two-line doc that begins 'Current epoch milliseconds.', andnow_mshas no doc comment. Fix: move that doc line back abovefn now_ms.docs/content/docs/guides/authentication.mdx:229- The new docs say a failed password or role replacement means 'nothing changes, and the user's row, API keys and live sessions stay as they were'. That does not hold forCommitError::Unknown. In that casereplace_user_row(src/protocol/handler.rs:1614-1617) callscredentials.remove_user, which revokes the user's live sessions and keys in memory until restart, because the write is durable but was not applied. Fix: qualify the sentence, for example 'unless the write's outcome is unknown, in which case the user's live credentials are revoked until restart'.🔧 Fix applied.
✅ Re-checked - no issues remain.
packages/inputlayer-js/tests/integration.test.ts:846- One JS SDK integration test fails against a live server: 'a partial failure names each failed statement' gets code 'unsupported' where it expects 'not_found' for.rel drop no_such_relinside a 3-statement program. It fails identically on the base (main 9ed61b4) server, and this branch does not modify the test, so the failure predates this change (evidence: js-partial-failure-on-base-server.log).make perf-gate(approved baseline vs candidate, judged) could not run because the host disk was nearly full (as little as 127 MB free), filled by other processes. Only a quick-profile debug run of the delta fixtures ran. To produce the judged report pre-pr requires, free disk space and runmake perf-gate.python3 -m unittest scripts/test_pre_pr.py -v(real Makefile in disposable git repos: routing, merge-base use, missing/invalid/self base refusal, parallel overlap, perf gate last)make pre-pr PRE_PR_BASE=HEAD|no-such-ref|''in the real worktree (fails before any check runs)cargo test --lib -- credential_mutation_tests revocation_waits_for_final_frame_enqueue crash_before_a_failed_cut_back_is_repairedcargo test --test credential_revocation_tests(14 WS end-to-end revocation tests)cargo test --manifest-path perf-gate/Cargo.toml --target-dir target(includes the GenBI convergence and open-loop delta tests)Live: candidate inputlayer-server + node WS driver (drive.mjs sessions): password change, role change, own-key revocation, user drop/recreate, key and grant non-inheritanceLive: revoke the bootstrap key, restart with INPUTLAYER_BOOTSTRAP_API_KEY still set, check the revoked key and the dropped user's old key are rejectedLive upgrade: base 9ed61b49 server creates the data dir with admin and bootstrap key, candidate server boots on it, the key is revoked and the server restarted; the revoked key never authenticatesLive:npx vitest run tests/integration.test.tsagainst the candidate server (47 tests)Live: the same failing JS test against the base server to classify the failureLive:perf-gate run --profile quick --rounds 1 --fixtures delta_single,interferenceagainst the candidate debug serverParsed .github/workflows with PyYAML to confirm perf-gate-check and vc-gate run only in full-suite.yml✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.