diff --git a/.agents/skills/basic-machines-review/SKILL.md b/.agents/skills/basic-machines-review/SKILL.md new file mode 100644 index 000000000..3305e7e4d --- /dev/null +++ b/.agents/skills/basic-machines-review/SKILL.md @@ -0,0 +1,68 @@ +--- +name: basic-machines-review +description: Use when reviewing Basic Machines code for house style, architecture risk, pre-merge hardening, or whether a change fits basic-memory/basic-memory-cloud conventions. +license: MIT +--- + +# Basic Machines Review + +Use this skill for repo-local review passes where ordinary code review needs Basic Machines +house style and architecture judgment. Report findings only; do not edit code unless the user +asks you to fix specific findings. + +## Scope + +Review the current diff or named files against: + +- The repo's `AGENTS.md` / `CLAUDE.md` +- `docs/ENGINEERING_STYLE.md` +- The touched code paths and tests + +Apply only the guidance for the active repo. In `basic-memory`, prioritize local-first +file/database/MCP boundaries. In `basic-memory-cloud`, prioritize tenant/workspace isolation, +cloud worker behavior, and web-v2 state/runtime boundaries. + +## Review Rubric + +Report only concrete, falsifiable risks: + +- **Cognitive load:** Is the change harder to understand than the problem requires? +- **Change propagation:** Will one product change force edits across unrelated layers? +- **Knowledge duplication:** Is the same rule encoded in multiple places that can drift? +- **Accidental complexity:** Did the change add abstractions, fallbacks, or state without need? +- **Dependency direction:** Are API/MCP/CLI, services, repositories, and UI stores respecting + their intended boundaries? +- **Domain model distortion:** Do names and types still match the product concept, or did a + transport/storage detail leak into the domain? +- **Test oracle quality:** Would the tests fail for the bug or regression the change claims to + protect against? + +## House Rules To Check Explicitly + +- No speculative `getattr(obj, "attr", default)` for unknown model shapes. +- No broad exception swallowing, warning-only failure paths, or hidden fallback behavior. +- No casts or `Any` that hide an unclear type relationship. +- Dataclasses for internal value/result objects; Pydantic at validation/serialization + boundaries. +- Narrow `Protocol`s when only a capability is needed. +- Explicit async/resource ownership, cancellation, and cleanup. +- Meaningful regression tests or verification for risky changes. +- Comments explain why, not what. + +## Reporting Format + +Lead with findings ordered by severity. Each finding should include: + +| Severity | Use for | +| -------- | ------- | +| `high` | A likely correctness, security, data-loss, or tenant/workspace isolation failure | +| `medium` | A concrete maintainability or boundary risk that can cause future defects | +| `low` | A minor consistency issue, ambiguous guidance, or review-only cleanup | + +```text +severity | file:line | risk category | claim +Why: concrete behavior or code path that proves the risk. +Fix: smallest practical change, or "none obvious" if the risk needs product input. +``` + +If there are no findings, say so and note any verification gaps that remain. diff --git a/.claude/skills/basic-machines-review b/.claude/skills/basic-machines-review new file mode 120000 index 000000000..48715ef5c --- /dev/null +++ b/.claude/skills/basic-machines-review @@ -0,0 +1 @@ +../../.agents/skills/basic-machines-review \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 0cb1efed4..80d00bc0e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,13 +108,34 @@ Before opening or updating a PR, run the checks that mirror the common required - Follow the repository pattern for data access - Tools communicate to api routers via the httpx ASGI client (in process) +### Programming Style + +See [docs/ENGINEERING_STYLE.md](docs/ENGINEERING_STYLE.md) for the fuller house style. The +short version for agents: + +- Prefer type-safe, explicit designs over object-heavy indirection. Use Python 3.12 `type` + aliases, full annotations, and narrow `Protocol`s when a caller only needs a capability. +- Use dataclasses for internal value objects and operation results; use Pydantic v2 at API, + CLI, MCP, and persistence boundaries where validation and serialization matter. +- Keep async boundaries obvious. Resource-owning code should use context managers, propagate + cancellation, and avoid hidden background work unless the lifecycle is explicit. +- Fail fast. Do not add silent fallback logic, broad exception swallowing, speculative + `getattr`, or casts that hide an unclear model shape. +- Keep control flow simple and local. Push branching decisions up, keep leaf helpers focused, + and name values after the domain concept they carry. +- Use evidence-first testing. Add or update meaningful regression tests for bugs and risky + behavior, prefer real code paths over mocks, and run the narrowest command that proves the + change before widening verification. +- Comments should explain why a branch, invariant, or constraint exists. Avoid comments that + merely narrate obvious code. + ### Code Change Guidelines - **Full file read before edits**: Before editing any file, read it in full first to ensure complete context; partial reads lead to corrupted edits - **Minimize diffs**: Prefer the smallest change that satisfies the request. Avoid unrelated refactors or style rewrites unless necessary for correctness -- **No speculative getattr**: Never use `getattr(obj, "attr", default)` when unsure about attribute names. Check the class definition or source code first -- **Fail fast**: Write code with fail-fast logic by default. Do not swallow exceptions with errors or warnings -- **No fallback logic**: Do not add fallback logic unless explicitly told to and agreed with the user +- **House style is canonical**: Follow the Programming Style section above for type-safe, + fail-fast code; do not hide unclear models with speculative attributes, broad exception + handling, casts, or unapproved fallback logic - **No guessing**: Do not say "The issue is..." before you actually know what the issue is. Investigate first. ### Literate Programming Style diff --git a/docs/ENGINEERING_STYLE.md b/docs/ENGINEERING_STYLE.md new file mode 100644 index 000000000..31497d35c --- /dev/null +++ b/docs/ENGINEERING_STYLE.md @@ -0,0 +1,64 @@ +# Basic Memory Engineering Style + +Style is how we make code easier to verify. Prefer explicit, typed, local-first code that +preserves the file system as the source of truth while keeping the database, API, and MCP +surfaces in sync. + +## Design Center + +- Basic Memory is local-first. Markdown files are the durable source; SQLite/Postgres indexes + are derived state that should be rebuilt or reconciled from files when needed. +- Keep the existing boundary order: CLI/MCP/API entrypoints compose dependencies, services own + business behavior, repositories own database access, and file services own filesystem writes. +- MCP tools should remain atomic and composable. They should call API routers through typed MCP + clients, not reach around into services. +- Prefer small, explicit abstractions that match a real domain boundary. Avoid object + hierarchies when a function, dataclass, type alias, or protocol describes the concept better. + +## Types And Data + +- Use full type annotations and Python 3.12 syntax. Introduce `type` aliases for repeated + structured shapes, callback signatures, or domain concepts that would otherwise become + anonymous `dict[str, Any]` values. +- Use dataclasses for internal values, operation inputs, and service results. Prefer + `frozen=True` when the value should not change and `slots=True` when identity/dynamic + attributes are not needed. +- Use Pydantic v2 at boundaries that validate, serialize, or deserialize data: API payloads, + CLI/MCP schemas, configuration, and persistence-adjacent schemas. +- Use narrow `Protocol`s when a caller needs a capability rather than a concrete repository or + service. Keep protocols small enough that fake implementations in tests are obvious. +- Avoid speculative `getattr`, broad casts, or `Any` as a way to paper over uncertainty. Read + the model or schema definition and make the type relationship explicit. + +## Control Flow And Resources + +- Fail fast when an invariant is broken. Do not swallow exceptions, add warning-only error + handling, or introduce fallback behavior unless the user explicitly agrees to that behavior. +- Keep control flow simple and close to the domain decision. Push `if` statements up into the + function that owns orchestration; keep leaf helpers focused on computation or one side effect. +- Make async/resource boundaries visible with context managers and explicit lifecycles. Do not + start background work without a clear owner, cancellation story, and verification path. +- Keep file mutations centralized through the existing file utilities/services so checksum, + atomic write, and index synchronization behavior stays coherent. + +## Testing And Verification + +- Use evidence-first testing, not mechanical TDD. For bugs and risky behavior, add or update a + regression test that would catch the failure. For small documentation-only edits, use the + relevant doc/repo hygiene checks. +- Prefer tests that exercise real code paths. Use mocks, doubles, or `monkeypatch` only when + the external boundary would be slow, nondeterministic, or impossible to trigger directly. +- Keep coverage at 100% for new code. Use `# pragma: no cover` only for code that would require + disproportionate mocking and is covered through an integration or runtime path. +- Start with targeted commands, then widen as risk grows: focused pytest, `just fast-check`, + `just doctor`, package checks for agent packaging changes, and full SQLite/Postgres gates + when behavior crosses shared boundaries. + +## Comments And Names + +- Name values after the domain concept they carry: project, entity, permalink, tenant, route, + checksum, observation, relation, batch, or index state. +- Comments should say why a branch, invariant, retry, lifecycle, or compatibility constraint + exists. Section headers are useful when a function or file has clear phases. +- Avoid comments that restate the code. If a comment cannot explain a decision, simplify the + code or improve the name instead.