chore(deps): add hypothesis + pre-commit to dev deps + document local hook activation - #1590
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates pre-commit and hypothesis into the development workflow by updating the CONTRIBUTING.md documentation and pyproject.toml dependency lists. Feedback was provided to ensure that comments in the configuration file remain accurate regarding tool defaults and avoid including ephemeral historical data like specific pull request numbers or timestamps.
| # Property-based testing — generates thousands of random inputs to | ||
| # find counterexamples the hand-written positive tests miss. Used | ||
| # opt-in (sprinkle ``@given(...)`` on a test); zero runtime cost on | ||
| # tests that don't use it. |
There was a problem hiding this comment.
The comment mentions that Hypothesis generates "thousands" of random inputs. By default, Hypothesis runs 100 examples per test. To ensure accuracy and consistency with the CONTRIBUTING.md file (which mentions "hundreds"), consider updating this description.
| # Property-based testing — generates thousands of random inputs to | |
| # find counterexamples the hand-written positive tests miss. Used | |
| # opt-in (sprinkle ``@given(...)`` on a test); zero runtime cost on | |
| # tests that don't use it. | |
| # Property-based testing — generates various random inputs to | |
| # find counterexamples the hand-written positive tests miss. Used | |
| # opt-in (sprinkle ``@given(...)`` on a test); zero runtime cost on | |
| # tests that don't use it. |
References
- Comments describing tool behavior should accurately reflect default settings to avoid misleading developers.
| # Pre-commit framework — gates every local ``git commit`` on the | ||
| # same ruff checks CI runs. Required activation step in | ||
| # CONTRIBUTING.md: ``pre-commit install`` once per clone. Without | ||
| # this, contributors silently bypass the lint gate locally and only | ||
| # discover the failure at CI time (which caused the 4 AM | ||
| # ruff-version-mismatch on PRs #1579 / #1584 on 2026-05-22). |
There was a problem hiding this comment.
Avoid including ephemeral details like specific PR numbers, timestamps, or "4 AM" in the codebase. This information is better suited for the commit history. The comment should focus on the purpose of the dependency and how to activate it.
| # Pre-commit framework — gates every local ``git commit`` on the | |
| # same ruff checks CI runs. Required activation step in | |
| # CONTRIBUTING.md: ``pre-commit install`` once per clone. Without | |
| # this, contributors silently bypass the lint gate locally and only | |
| # discover the failure at CI time (which caused the 4 AM | |
| # ruff-version-mismatch on PRs #1579 / #1584 on 2026-05-22). | |
| # Pre-commit framework — ensures local commits pass the same linting | |
| # checks as CI. Requires a one-time activation step per clone (see | |
| # CONTRIBUTING.md): ``pre-commit install``. This prevents | |
| # contributors from accidentally bypassing the lint gate locally. |
References
- Code comments should focus on the 'why' and 'how' of the current state, rather than historical context like PR numbers or specific dates, which are better tracked in version control.
igorls
left a comment
There was a problem hiding this comment.
Verified locally against feat/cli-read-verb's sibling branch — uv sync --extra dev resolves cleanly, both new deps install (hypothesis 6.152.9, pre-commit 4.6.0), and pre-commit run --all-files is clean. CI is green and the change is low-risk. Two small asks before merge:
1. Drop the ephemeral PR refs from the pre-commit comment in pyproject.toml (lines 64–70 of the diff). The CLAUDE.md guidance is to keep current-task/PR-number/incident-date references in the PR description (which already has them) rather than the source tree, since they rot as the codebase evolves. The structural reason for the dep is what belongs in the comment. Aligns with what gemini-code-assist flagged. Suggested rewrite:
# Pre-commit framework — gates every local ``git commit`` on the
# same ruff checks CI runs. Required activation step in
# CONTRIBUTING.md: ``pre-commit install`` once per clone. Without
# this, contributors silently bypass the lint gate locally and only
# discover failures at CI time.2. Minor: "thousands of random inputs" → align with CONTRIBUTING.md. Hypothesis defaults to 100 examples per test (max_examples). CONTRIBUTING.md already says "hundreds," so the pyproject.toml comment is the outlier — either "many" or "hundreds" would be more accurate.
Nothing else stood out. The duplication between [project.optional-dependencies].dev and [dependency-groups].dev already existed pre-PR and isn't this PR's problem.
|
ok added al the changes. please review when you can. |
…x group + subprocess CLI tests Addresses the remaining unresolved gemini-code-assist findings and the ``argparse.add_mutually_exclusive_group``/CLI-test follow-ups raised by @igorls on the initial review. Storage scaling --------------- * ``_chunked_get(col, ids, include, batch=500)`` helper splits any ``col.get(ids=...)`` call into chunks of at most ``batch`` IDs and merges the per-batch results, keeping each call comfortably below SQLite's ``SQLITE_MAX_VARIABLE_NUMBER`` bind limit (default 999). Returns the same ``{ids, documents, metadatas}`` shape ``col.get`` produces so callers can drop it in transparently. Empty-input short-circuit avoids a pointless ``col.get(ids=[])`` call. * ``_resolve_by_source`` fallback scan switched from a single unbounded ``col.get(include=["metadatas"])`` to a paginated loop that mirrors ``palace.bulk_check_mined`` exactly: ``col.count()`` bound, ``limit=1000`` per page, terminate on empty batch. Avoids loading every drawer's metadata into one Python list on 100K+ palaces. * The matched-IDs docs fetch then goes through ``_chunked_get`` so even a basename match against thousands of duplicate filenames (``notes.md``, ``index.ts``, etc.) cannot trip the SQLite bind limit. * ``_resolve_by_ids`` also routes through ``_chunked_get`` for uniformity. Closet pointers are typically ≤ 3 IDs in practice, so this is defensive-never-exercised, but matching the helper makes the two call sites symmetric and future-proofs against any caller that might pass a larger list. CLI ergonomics -------------- * ``--drawer`` and ``--all`` registered via ``argparse.add_mutually_exclusive_group()``. The constraint now shows up in ``mempalace read --help`` (``[--drawer DRAWER | --all]``) and is enforced at parse time, so the manual post-parse check in ``cmd_read`` is gone. * ``cmd_read`` TTY-guard regression repaired. A prior commit-suggestion application had introduced a duplicate ``if pointer is None or pointer == "-":`` block that dropped the ``sys.stdin is None`` defensive check (used to handle Windows ``pythonw.exe`` and some detached-daemon launch contexts). Restored to a single block with both ``is None`` and ``isatty()`` guards and a properly-placed ``sys.exit(1)``. Tests added / rewritten ----------------------- * ``TestChunkedGet`` (4 tests): empty-input no-call guard, single-chunk passthrough, multi-chunk split with full and partial last batch, merged-result aggregation contract. * ``TestCmdReadCLISubprocess`` (3 tests): real end-to-end smokes that mine a fixture file, shell out to ``mempalace read``, and assert surgical-slice correctness (the kind of contract the unit-test suite can't enforce with mocks alone). Includes a positive smoke asserting ``[3]``–``[7]`` only, a negative smoke for garbage-pointer exit codes, and a ``--help`` smoke asserting the mutex group surfaces AND ``--drawer 1 --all <ptr>`` is rejected at parse time with the argparse "not allowed with argument" message. * ``TestResolveBySourceTwoStepFetch`` (3 tests rewritten): brittle hardcoded ``side_effect`` lists replaced with the upgraded ``_fake_collection`` helper that supports ``limit``/``offset`` for paginated scans plus ``col.count()``. Contracts pinned: every metadata-scan call must NOT request ``"documents"``; the docs-fetch may issue any number of chunked ``col.get(ids=...)`` calls, but combined they request EXACTLY the matched IDs (no non-matches leaked); when zero matches survive the scan, ZERO docs-fetch calls happen. * ``_fake_collection`` helper upgraded with ``col.count.return_value`` and ``limit``/``offset``-aware fall-through, so any test against the new paginated path works through the established fixture pattern. Hygiene ------- * Stripped ``Task #87`` references from ``reader.py`` docstrings (module header + ``parse_pointer``) and ``tests/test_reader.py`` module header. Same CLAUDE.md guidance as PR #1590. Verification ------------ * Full mempalace suite (macOS local): 2191 passed, 3 skipped. * Reader test suite on Python 3.9 / 3.11 / 3.13 via OrbStack: 45/45. * ``ruff 0.15.9`` check + ``format --check`` both clean. * End-user smoke against a real mined palace: $ mempalace --palace /tmp/p mine /tmp/src $ mempalace --palace /tmp/p read "2024-11-08:L3-L7 chat.md" [3] line 3 — alpha [4] line 4 — beta [5] line 5 — gamma [6] line 6 — delta [7] line 7 — epsilon * ``mempalace read --help`` now shows ``[--drawer DRAWER | --all]`` in the usage line; ``--drawer 1 --all <ptr>`` is rejected with exit code 2 and ``argument --all: not allowed with argument --drawer``.
… hook activation Adds two test-discipline tools to the dev extras and closes a real contributor-onboarding gap in CONTRIBUTING.md that allowed PR #1579's 2026-05-22 4 AM ruff-version-mismatch lint failure. Adds to ``[project.optional-dependencies].dev`` (and matching ``[dependency-groups].dev`` for uv users): - **``hypothesis>=6.0``** — property-based testing framework. Generates hundreds of random inputs per test and shrinks failing cases to a minimal counterexample. Used opt-in (sprinkle ``@given(...)`` on a test); zero runtime cost on tests that don't use it. Would have caught the Tier 6a dateutil-fuzzy hallucination on PR #1584 with one property test. - **``pre-commit>=3.0``** — the pre-commit framework itself. ``.pre-commit-config.yaml`` already lives in the repo (committed by @igorls on 2026-05-18, pinned to ruff 0.15.9 in lockstep with CI). What was missing was making the framework a declared dev dependency so ``pip install -e .[dev]`` / ``uv sync --extra dev`` actually pulls it in. Adds a ``pre-commit install`` line to the Getting Started bash block plus a short paragraph explaining why this step is required (the hook file at ``.git/hooks/pre-commit`` is per-machine and NOT tracked by git, so the repo's ``.pre-commit-config.yaml`` only takes effect after each developer runs ``pre-commit install`` once locally). Adds an optional "Property-based tests" subsection under Running Tests showing the minimal ``@given(...)`` pattern, so contributors who want to reach for the new tool know it's available. On 2026-05-22 at 4:30 AM, PR #1579 (Tier 6a) hit a CI lint failure caused by a ruff version mismatch: my local machine had ruff 0.4.10, CI runs ruff 0.15.9 (pinned in pyproject.toml). The two versions produce different ``ruff format`` output for the same code. The repo HAD ``.pre-commit-config.yaml`` pinning ruff 0.15.9 — but the local git hook had never been wired on my machine because nothing in CONTRIBUTING.md said to run ``pre-commit install``. The protection existed at the project layer; the activation gap was at the contributor-onboarding layer. This commit closes that gap structurally. Anyone cloning the repo from now on sees ``pre-commit install`` as part of the Getting Started flow and is protected from the same failure. - **No ``mutmut`` in dev deps.** Mutation testing is heavier (runs the whole test suite per mutation) and useful periodically rather than every commit. Contributors who want to run it can install manually. Adding it to dev deps would bloat the install footprint for every contributor when most will never use it. - **No new property tests.** This PR ships the TOOL, not new test coverage. Property tests should land alongside the specific functions they cover, in their own PRs. - **No changes to ``.pre-commit-config.yaml``.** That file is correct as Igor wrote it. The fix here is purely making the framework installable + documenting the activation step. ruff check . → All checks passed. pre-commit run --all-files (locally) → ruff (legacy alias): Passed → ruff format: Passed OrbStack triple-Linux verify (Py 3.9 / 3.11 / 3.13) → ``pip install -e .[dev]`` resolves cleanly; hypothesis + pre_commit import successfully; existing test suite unaffected.
… gap caught by gemini on PR #1588)
Lock-step with pyproject.toml `[project.optional-dependencies].dev` (ruff bumped from 0.15.9 → 0.15.14 via PR #1583). The `.pre-commit-config.yaml` header explicitly requires this rev to match the pyproject pin — without this bump, contributors who run `pre-commit install` will hit the same version-mismatch debacle this PR was opened to prevent.
a08e20c to
4655a30
Compare
|
Привет. |
Adds two test-discipline tools to the dev extras and closes a real contributor-onboarding gap in CONTRIBUTING.md that allowed PR #1579's 2026-05-22 4 AM ruff-version-mismatch lint failure.
Changes
pyproject.tomlAdds to
[project.optional-dependencies].dev(and matching[dependency-groups].devfor uv users):hypothesis>=6.0— property-based testing framework. Generates hundreds of random inputs per test and shrinks failing cases to a minimal counterexample. Used opt-in (sprinkle@given(...)on a test); zero runtime cost on tests that don't use it. Would have caught the Tier 6a dateutil-fuzzy hallucination on PR feat(closets): Tier 6a — date+line locators with content-date hierarchy #1584 with one property test.pre-commit>=3.0— the pre-commit framework itself..pre-commit-config.yamlalready lives in the repo (committed by @igorls on 2026-05-18, pinned to ruff 0.15.9 in lockstep with CI). What was missing was making the framework a declared dev dependency sopip install -e .[dev]/uv sync --extra devactually pulls it in.CONTRIBUTING.mdAdds a
pre-commit installline to the Getting Started bash block plus a short paragraph explaining why this step is required (the hook file at.git/hooks/pre-commitis per-machine and NOT tracked by git, so the repo's.pre-commit-config.yamlonly takes effect after each developer runspre-commit installonce locally).Adds an optional "Property-based tests" subsection under Running Tests showing the minimal
@given(...)pattern, so contributors who want to reach for the new tool know it's available.Why this matters — the 2026-05-22 4 AM debacle
On 2026-05-22 at 4:30 AM, PR #1579 (Tier 6a) hit a CI lint failure caused by a ruff version mismatch: my local machine had ruff 0.4.10, CI runs ruff 0.15.9 (pinned in pyproject.toml). The two versions produce different
ruff formatoutput for the same code. The repo HAD.pre-commit-config.yamlpinning ruff 0.15.9 — but the local git hook had never been wired on my machine because nothing in CONTRIBUTING.md said to runpre-commit install. The protection existed at the project layer; the activation gap was at the contributor-onboarding layer.This commit closes that gap structurally. Anyone cloning the repo from now on sees
pre-commit installas part of the Getting Started flow and is protected from the same failure.Out of scope (deliberate)
No
mutmutin dev deps. Mutation testing is heavier (runs the whole test suite per mutation) and useful periodically rather than every commit. Contributors who want to run it can install manually. Adding it to dev deps would bloat the install footprint for every contributor when most will never use it.No new property tests. This PR ships the TOOL, not new test coverage. Property tests should land alongside the specific functions they cover, in their own PRs.
No changes to
.pre-commit-config.yaml. That file is correct as Igor wrote it. The fix here is purely making the framework installable + documenting the activation step.Verification
ruff check . → All checks passed.
pre-commit run --all-files (locally) → ruff (legacy alias): Passed → ruff format: Passed
OrbStack triple-Linux verify (Py 3.9 / 3.11 / 3.13) →
pip install -e .[dev]resolves cleanly; hypothesis + pre_commit import successfully; existing test suite unaffected.What does this PR do?
Adds
hypothesis>=6.0andpre-commit>=3.0to dev deps + documentspre-commit installin CONTRIBUTING.mdCloses the contributor-onboarding gap that caused PR #1579's 4 AM ruff-version-mismatch on 2026-05-22 — the repo had
.pre-commit-config.yamlpinnedcorrectly, but nothing told new contributors to run
pre-commit installlocally. Pure tooling/docs PR; independent of any feature PR.How to test
pip install -e .[dev](oruv sync --extra dev) → bothhypothesisandpre-commitshould installpre-commit install,git commitautomatically runs ruff before allowing the commitpython -c "import hypothesis"should succeedChecklist
python -m pytest tests/ -v) — no functional changes, existing tests untouchedruff check .)