Add unit tests closing coverage gaps in pure-logic modules - #320
Merged
Conversation
Adds 138 direct unit tests across 8 new files, targeting previously untested (or only indirectly-tested) pure-logic, correctness-sensitive modules. No production code changed; all tests run without ES/Postgres. - test_typedsheets.py: value casting/parsing helpers (cast, parse_*, convert, cast_row_values, remove_nulls) incl. null/empty asymmetry. - test_authorization.py: groupfinder synthetic + user/access-key lookup branches, _create_principals, is_admin_request (security-sensitive). - test_etag.py: ETag/conditional-GET decorators incl. 304 short-circuit and per-principal ETag distinctness. - test_schema_formats.py: is_uuid / is_accession regex validators. - test_server_defaults_misc.py: all add_last_modified branches. - test_predicates.py: SubpathSegments / AdditionalPermission predicates. - test_typeinfo.py: extract_schema_links recursive generator. - test_util_helpers.py: deduplicate_list, ensurelist, gunzip_content, convert_integer_to_comma_string, simple_path_ids, recursively_process_field, resolve_file_path, and merge_calculated_into_properties edge branches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Patch bump for the test-only coverage additions in the previous commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
willronchetti
added a commit
that referenced
this pull request
Jul 6, 2026
…nd flaky marks - Fix the CHANGELOG entry heading left as 11.31.1 by the master merge while pyproject.toml said 11.31.7 -- this mismatch was failing both the Static Checks and UNIT CI jobs on the changelog checker. - Add unit tests for the gaps PR #320's own audit listed as deliberate follow-ups: local_roles (principal expansion, role->principal reverse mapping, LocalRolesAuthorizationPolicy), the calculated-property registry (schema default guard, copy+mark, condition/attr dispatch, MRO-ordered props_for, category isolation, ItemNamespace memoization), json_renderer adapters (UUID/set/frozenset/datetime, request-less dumps, BinaryFromJSON, JSONResult.serializer), ManagerLRUCache (no-op outside a request, settings capacity, afterCompletion flush), and the untested EDWHash branches (verify round-trip, bytes/str equivalence, password-too-long boundary). - Re-enable @pytest.mark.flaky(max_runs=3, rerun_filter=delay_rerun) on test_aggregated_items and add it to test_indexer_namespacing and test_indexer_queue_adds_telemetry_id, which use the same SQS/ES polling pattern as their already-protected neighbors (known CI flakiness fix). - Bump version to 11.31.8 (checked open PRs: #321 is at 11.30.8, #308 at 11.24.0 -- no collision) and add a changelog entry. - Record the flaky-mark convention and the ItemNamespace.__getattr__ request=None recursion sharp edge in AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 tasks
willronchetti
added a commit
that referenced
this pull request
Jul 7, 2026
#321) * Fix self-registration privilege-escalation in create_unauthorized_user POST /create-unauthorized-user ran with request.remote_user = 'EMBED' (restricted_fields write permission) while validating/creating the new User, but took the caller-submitted body almost as-is - letting a caller self-assign privileged fields (e.g. "groups": ["admin"]) on their own new account. Whitelist (not blocklist) the fields the endpoint accepts, since snovault is consumed by apps with different User schemas and a hardcoded blocklist of "dangerous" field names only protects against names its author thought to enumerate. pending_lab is a documented exception since it is only a self-declared request requiring later admin promotion to lab, not an immediate grant of access. Also fixes a TestRoot ACL drift (missing EMBED -> restricted_fields, present in production SnovaultRoot) needed to exercise this endpoint in tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Bump version to 11.30.8 to avoid collision with PR #318 (11.30.5) PR #318 already merged at 11.30.5; PR #320 claims 11.30.7. Next free. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 138 direct unit tests across 8 new files (initial audit), plus a follow-up
round of ~50 more tests across 4 new files + 2 extended files closing the gaps the
initial audit had explicitly deferred. No production code was changed; the new tests
run without ElasticSearch or Postgres. Also folds in a known CI-flakiness fix
(flaky-rerun decorators in
test_indexing.py) and repairs a version/changelog mismatchintroduced by the master merge.
What I audited (round 1)
I mapped the 30 existing
test_*.pyfiles against the 60 top-levelsnovault/*.pymodules, ran a targeted read of the pure-logic candidates, and checked each for
(a) testability without live services, (b) correctness-sensitivity, and (c) current
coverage. I confirmed gaps by grepping the whole test tree for each target function
and by exercising actual runtime behavior before pinning it in assertions.
Gaps found and closed (round 1 — 8 new files, 138 tests)
test_typedsheets.pytypedsheets.pynull/empty-string asymmetry, right-to-left cast ordering, array/object parsing, and row generators.test_authorization.pyauthorization.pygroupfinderhad only indirect coverage. Covers all syntheticremoteuser.*results, user + access-key lookup branches,_create_principals, andis_admin_request.test_etag.pyetag.pytest_schema_formats.pyschema_formats.pyis_uuid/is_accessionidentity validators, untested; documents the knownre.matchlaxness.test_server_defaults_misc.pyserver_defaults_misc.pyadd_last_modifiedbranches covered via mockedget_userid/get_now.test_predicates.pypredicates.pytext/phash, and__call__matching.test_typeinfo.pytypeinfo.extract_schema_linkslinkTocases plus theKeyErroredge.test_util_helpers.pyutil.py(pure helpers)deduplicate_list,ensurelist,gunzip_content,simple_path_ids,recursively_process_field,merge_calculated_into_propertiesedge branches, and more.Round 2 — follow-up gaps implemented (4 new files, 2 extended)
These are the items round 1 listed under "deliberately left as follow-up", now done:
test_local_roles.py(new)local_roles.pylocal_principals(lineage accumulation, callable roles, string-role normalization,__ac_local_roles_block__stopping ancestors but keeping the blocking node's own roles, identity-return when nothing matches),merged_local_principals(reverse role→principal mapping), andLocalRolesAuthorizationPolicy.permits/principals_allowed_by_permissionend-to-end against a real ACL.test_calculated_registry.py(new)calculated.pyCalculatedPropertyschemadefaultguard (ValueError), schema copy +calculatedProperty: Truemarking, condition gating,attrdispatch to context methods, fn-arg resolution from the namespace;ItemNamespaceper-fn memoization and string-name resolution;CalculatedProperties.props_forMRO ordering (subclass overrides base), instance-or-class input, and category isolation.test_json_renderer.py(new)json_renderer.pydumps()working outside any request (the indexer calls it that way),JSONResult.serializer→BinaryFromJSONUTF-8 chunk encoding.test_manager_lru_cache.py(new)cache.pyManagerLRUCachedegrading to a no-op with no threadlocal stack, capacity resolution from registry settings vs. default, and theafterCompletiontransaction flush (stale-entry protection for retried transactions).test_edw_hash.py(extended)edw_hash.pyverify()round-trip, bytes/str secret equivalence, and the exact "Password too long" boundary (454-char max secret, computed from the salt layout).CI flakiness fix folded in (test-only, known issue)
@pytest.mark.flaky(max_runs=3, rerun_filter=delay_rerun)ontest_aggregated_items(was commented out).test_indexer_namespacingandtest_indexer_queue_adds_telemetry_id, which use the same SQS/ES polling pattern astheir already-protected neighbors but never had it.
AGENTS.md.Post-merge repair
The master merge (
6b7ef67c9) resolvedpyproject.tomlto11.31.7but left this PR'sCHANGELOG entry headed
11.31.1, which failed the changelog checker in both theStatic Checks and UNIT CI jobs. Fixed the heading, then bumped to 11.31.8 for the new
work (checked open PRs for collisions: #321 is at 11.30.8, #308 at 11.24.0 — none).
Notes / observations (no action taken)
ItemNamespace.__getattr__sharp edge (not a production bug): if anItemNamespaceis constructed withrequest=None(or a request lacking.registry),looking up an unknown attribute recurses to
RecursionErrorinstead of raising a cleanAttributeError, because thereify-basedregistry/_propertiesproperties raiseAttributeErrorinside__getattr__and re-enter it. Unreachable in production(
calculate_propertiesalways passes a real request); documented inAGENTS.mdforfuture test authors.
AGENTS.md/CLAUDE.md; since master now has them (via Add per-type track_revisions opt-out for Postgres revision history #319),Fix self-registration privilege-escalation in create_unauthorized_user #321 will need conflict resolution on those files when it takes a master merge — noted
here because this PR's merge already integrated master's copies.
Testing
pytest -m "not es and not indexing and not performance"→790 passed, 28 skipped, with the single pre-existing environment-specific failure
(
test_postgresql_fixture.py::test_snovault_db_test_port), which fails identically ona clean baseline.
test_indexing.py(ES-marked, not runnable locally) collects cleanly with the newdecorators;
delay_rerunwas already imported in that file.🤖 Generated with Claude Code