From 06fe3916e70561abe6a203898144dd411e1c0b8a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 21:52:22 +0200 Subject: [PATCH] ci: Update CLAUDE.md and add pre-push-review skill. Remove astrolabe from docker-compose.yml volume mount --- .claude/skills/pre-push-review/SKILL.md | 380 ++++++++++++++++++++++++ CLAUDE.md | 12 +- docker-compose.yml | 2 +- nextcloud_mcp_server/server/semantic.py | 17 +- 4 files changed, 406 insertions(+), 5 deletions(-) create mode 100644 .claude/skills/pre-push-review/SKILL.md diff --git a/.claude/skills/pre-push-review/SKILL.md b/.claude/skills/pre-push-review/SKILL.md new file mode 100644 index 00000000..f2214bf7 --- /dev/null +++ b/.claude/skills/pre-push-review/SKILL.md @@ -0,0 +1,380 @@ +--- +name: pre-push-review +description: | + Self-review the current branch's diff against nextcloud-mcp-server review patterns + before pushing a PR. Runs ruff/ty/tests on changed files and produces a punch list + of likely review-round findings, calibrated to issues that have repeatedly surfaced + in this repo's automated PR reviews. Use when the user is about to push, says "ready + to push", "review my work", "check before PR", or invokes /pre-push-review. + Report-only — does not modify code. +allowed-tools: + - Bash + - Read + - Grep + - Glob +--- + +# Pre-Push Review + +## Purpose + +Catch the issues that have repeatedly surfaced in PR reviews on this repo (#733–#750) +*before* the human reviewer or the automated `claude` PR-review bot sees them. Output is +a labelled punch list. The main loop decides what to fix. + +This skill is calibrated to **this repo's recurring patterns** — not a generic code +review. The point is to short-circuit the review-round-N loop, not to replace human +judgment. + +## When to Use + +**Trigger when:** +- User says "review my work", "ready to push", "check before PR", "pre-push", or + invokes `/pre-push-review`. +- After a substantive change (new module, new tool, refactor, behavior change) and + before `git push`. +- Before opening or updating a PR. + +**Skip when:** +- Tiny diffs (typo fix, README tweak, dependency bump only). +- User has explicitly said "just push it" / "skip the review". +- Branch is `master` or has zero commits ahead of base. + +## Workflow + +### Phase 1 — Establish scope (≤ 30s) + +Determine the base branch and diff range. Default base is `master`. + +```bash +git fetch origin master --quiet +BASE=$(git merge-base HEAD origin/master) +git rev-list --count $BASE..HEAD # commits ahead +git diff --stat $BASE..HEAD # files touched +git log --format="%h %s" $BASE..HEAD # commit list +``` + +If the user names a different base (e.g. `main`, a stacked branch), use that instead. + +**Identify the change shape** — these classifications drive scope-aware checks (Phase 3): + +| Touches | Treat as | +|---|---| +| `nextcloud_mcp_server/auth/`, `*scope*`, `*token*`, `*verifier*` | **security-sensitive** (escalate B, J, K to blocking) | +| `nextcloud_mcp_server/server/*.py` (new `@mcp.tool`) | **MCP surface** (D4, I1 mandatory) | +| `nextcloud_mcp_server/client/*.py` | **client layer** (B1–B4, H1, retry/backoff) | +| `nextcloud_mcp_server/models/*.py` | **schema** (D1–D3, validators for PHP quirks) | +| `nextcloud_mcp_server/search/`, `nextcloud_mcp_server/vector/` | **vector subsystem** (E*, F3, latency budget) | +| Webhooks, `nextcloud_mcp_server/auth/webhook_*` | **webhook handler** (J3, J4, K1) | +| Tests only | **test scope** (skip A–E, run F1–F4) | +| Docs / ADR only | **docs scope** (run G3, G4 only) | + +**Report up front:** branch, base, commits ahead, files changed, change shape. + +### Phase 2 — Automated checks (in parallel, ≤ 30s) + +Run these from the project root with separate `Bash` calls in **one message** so they +run concurrently: + +```bash +uv run ruff check # lint +uv run ruff format --check # formatting +uv run ty check -- nextcloud_mcp_server # types +uv run pytest tests/unit/ -x --no-header -q # unit tests +``` + +If the diff touches `nextcloud_mcp_server/server/` or `tests/server/`, also run: + +```bash +uv run pytest -m smoke -x --no-header -q +``` + +**If any automated check fails: stop and report.** Don't run the checklist on top of a +broken build — fixing the failures may eliminate findings or change the diff. + +### Phase 3 — Read the diff and run the project checklist (2–5min) + +```bash +git diff $BASE..HEAD # full diff +git diff $BASE..HEAD -- '*.py' | head -2000 # python only, capped +``` + +Read the **whole diff** before composing findings. Cross-file patterns (test symmetry, +single-source-of-truth, deduplicated logic) are only visible in aggregate. + +For each violation, capture: +- **Severity label** (🔴 / 🟡 / 🟢 — see Severity Guide below) +- **Category tag** (A1, B2, J3, etc.) +- **File:line** (new-side line number) +- **One-sentence note** — what's wrong, with a concrete fix direction + +### Phase 4 — Output the punch list + +Format follows the template in [§ Output Template](#output-template). Group by +severity. Include a brief **Strengths** section to balance the findings — this +matches the tone of the bot review on PRs #733–#747 and avoids the impression that +the review is purely negative. + +**Hard rules:** +- Do **not** edit any files. +- Do **not** propose patches inline (one-line fix direction is OK; full code blocks are not). +- Do **not** repeat findings the linter or type checker already surfaces. +- If a finding is style-only and CLAUDE.md doesn't mandate it, demote to 🟢. + +--- + +## Severity Guide + +| Label | Meaning | Examples | +|---|---|---| +| 🔴 **blocking** | Must fix before push. Real bug, security issue, or violates an ADR/CLAUDE.md mandate. | Cache-hit bypass on auth gate; raw `List[Dict]` from MCP tool; missing `await` on async call; HMAC compare with strings instead of bytes. | +| 🟡 **important** | Should fix. Not a bug today but a footgun that has caused review-rounds before. | Field description mismatches value; unbounded fan-out without semaphore; `int()` cast inside catch-all `except`; missing `openWorldHint=True`. | +| 🟢 **nit** | Polish. Not blocking, would be nice. | `Optional[X]` when surrounding file uses `X | None`; redundant regex anchors; missing test docstring; pluralization in error messages. | +| 💡 **suggestion** | Alternative approach to consider. No fix expected. | "Could use `frozenset` here for O(1) lookup"; "Could centralise this rewrite in `_make_request`". | +| 🎉 **praise** | Notable good pattern. Worth calling out for visibility. | Cache-hit AND cache-miss paths both enforce the gate; CI-guard test for registry exhaustiveness; explicit `*` keyword-only separator. | + +**Escalation rules** (apply per change shape from Phase 1): +- Security-sensitive scope → all J* and K* findings are at least 🟡; auth bypass risks are 🔴. +- MCP surface → D4 (annotations) and I1 (response wrapping) are 🔴. +- Client layer → H1 (duplicate round-trips) is 🟡 minimum. + +--- + +## Project Checklist + +Each item is tagged for use in the punch list (e.g. `[B1]`). Items are mined from +PRs #733, #734, #735, #736, #737, #741, #745, #746, #747, #750. + +### A. Style & typing (CLAUDE.md) + +- **A1** — PEP 604 unions: `X | None`, never `Optional[X]`. (#737, #745, #735) +- **A2** — Lowercase generics: `dict[str, Any]`, `list[T]`, never `Dict`/`List`. (#736, #745) +- **A3** — `anyio` not `asyncio`: `anyio.create_task_group`, `anyio.Lock`, `anyio.run`. Flag any new `asyncio.gather`/`asyncio.Lock`/`asyncio.run`. +- **A4** — Lazy `%`-style logging: `logger.warning("msg %s", var)`, not f-strings, in any file touched on this branch. (memory: feedback_lazy_logging) +- **A5** — Typed signatures: every new/modified function has parameter and return type hints. (#733) +- **A6** — Stay consistent with surrounding file: if a touched file uses legacy `Dict`/`Optional`, flag the introduced inconsistency, not pre-existing debt. (#736, #735, #745) + +### B. Error handling specificity + +- **B1** — Hoist parsing/casts before network calls: `int()`, `json.loads()`, etc. in their own `try/except (TypeError, ValueError)` *before* the network call. Catch-all `except Exception` over the parse + network produces "unexpected error" instead of a specific log line. (#750-r3, #750-r5) +- **B2** — Narrow exception types: `except RuntimeError`, not bare `except Exception`, when only one failure mode is documented. (#750-r6) +- **B3** — Specific error messages with context: include the problematic value and its surrounding context (e.g. `f"expected int id for {doc_type}, got {type(value).__name__}: {value!r}"`), not opaque re-raises. (#750-r6) +- **B4** — Fail-open vs fail-closed is a deliberate choice: any new fail-open branch (transient error → keep going) needs a log line and a brief comment on *why* fail-open is correct here. (#750-r2) +- **B5** — Except clause order: when catching a subclass and superclass in the same `try`, the subclass must come first. `except TimeoutError` before `except Exception` (TimeoutError is OSError → Exception subtype). (#747) +- **B6** — Defensive parsing envelope: when extracting fields from external payloads, wrap in `try/except (KeyError, TypeError, ValueError)` and return a sentinel. Bubbling `ValueError` from `int("not-a-number")` to the caller is a finding. (#747) + +### C. Comment honesty & documentation + +- **C1** — Comments must match behavior: don't say "background" if the code runs inline; don't say "evicts asynchronously" if it `await`s. (#750-r1) +- **C2** — Document all return-None / sentinel cases: if a function returns `None` for both 404 *and* malformed XML, the docstring must enumerate both. (#750-r6) +- **C3** — Cross-references point at canonical/earlier definition: "Mirrors `_verify_X`" should reference the function defined first. (#750-r1) +- **C4** — TODOs include WHY: `# TODO(perf): get_items(batch_size=-1) fetches all items at query time` ✓; `# TODO: optimize` ✗. (#750-r3) +- **C5** — Defensive code carries its reason: `.get(key, default)` that "shouldn't fire in practice" needs a comment saying so — otherwise readers treat it as load-bearing. (#750-r6) +- **C6** — Surprising-on-first-read code needs a one-liner: e.g. `password=token` for bearer auth is surprising; a `# caldav reuses the password slot for bearer tokens` line saves readers from suspecting copy-paste bugs. (#734) +- **C7** — Mark deliberate omissions: when a code path *intentionally* skips a guard (e.g. DELETE returns empty body, no shape guard), say so explicitly. Otherwise future maintainers will treat it as an oversight. (#736) + +### D. API & type boundaries + +- **D1** — Public response models narrow, internal types may widen: if `SearchResult.id: int | str` for forward-compat, `SemanticSearchResult.id: int` and convert at the boundary with an explicit cast that fails loudly. (#750-r3) +- **D2** — Field descriptions match the value: a field described as "unique documents" must hold a unique-document count, not a chunk count. Watch for `len(results)` assigned to a field whose docstring implies dedup. (#750-r1, #735) +- **D3** — Symmetry between related fields: `verified_count` and `dropped_count` should count at the same granularity. (#750-r6) +- **D4** — MCP tool annotations (ADR-017): tools that hit Nextcloud have `openWorldHint=True`. Read-only ops have `readOnlyHint=True`. Destructive ops have `destructiveHint=True` + `idempotentHint=True`. Create ops have `idempotentHint=False`. Update ops have `idempotentHint=False` (etag changes mean different inputs). (#741) +- **D5** — Keyword-only separators: use `*` in signatures with multiple optional params of the same/related types, e.g. `def __init__(self, *, password=None, token=None)` to prevent positional misuse. (#734) +- **D6** — Defaults match documented constraints: if the docstring says "max 1000 characters", add `Field(max_length=1000)` — don't rely on the server returning 400. (#737) +- **D7** — Create/update tools return `*Response` wrappers, not raw models: any new `@mcp.tool` returning a raw `BaseModel` (not `BaseResponse` subclass) is a finding. (#737, CLAUDE.md MCP Response Patterns) +- **D8** — Pagination metadata: a field called `total` should be the server-side total, not page count. Use `count` for "returned in this page" or add `has_more`. (#737) + +### E. Concurrency, resource bounds & lifespan + +- **E1** — Bound concurrency: any new `asyncio.gather` / task-group fan-out over user data needs an `anyio.Semaphore` cap (default 20 in this repo). (#750-r1) +- **E2** — Bound over-fetching: `limit * K` for filtering must have a fixed `K`. Unbounded N-types or unbounded `K` is a finding. (#750-r1) +- **E3** — Don't snapshot mutable singletons: `eviction_task_group` set during lifespan startup must be read via `@property`/accessor, not snapshotted into another object's `__init__` (order-sensitive race). (#750-r5) +- **E4** — Comment safety of dict/list mutation across tasks: when multiple concurrent tasks write distinct keys to a shared dict, add a one-line comment explaining cooperative-multitasking safety. (#750-r1) +- **E5** — Lifespan context symmetry: when adding fields to one of `AppContext` / `OAuthAppContext`, mirror them in the other to prevent silent regressions in OAuth mode. (#746) +- **E6** — Security gates apply on cache-hit AND cache-miss paths: an allowlist or scope check must run after both branches resolve. A test asserting cache-hit enforcement is mandatory. (#745) + +### F. Tests + +- **F1** — Symmetry across verifiers/handlers: if you added a 403 test for the notes verifier, add one for files/deck/news too. Asymmetric coverage in a registry-style module is a finding. (#750-r5, #741) +- **F2** — Fail-open paths have a test: any verifier or handler with a fail-open branch (non-numeric id, malformed payload) has a unit test that exercises it. (#750-r5, #750-r6) +- **F3** — CI guard tests for registries: registries (e.g. `INDEXED_DOC_TYPES → verifier`) should have a test that the registry is exhaustive. (#750-r2) +- **F4** — New public functions have tests: any new MCP tool, client method, or Pydantic model without a unit test is a 🟡 finding (or 🟢 if integration-only). +- **F5** — `caplog` is logger-scoped: prefer `caplog.set_level(logging.WARNING, logger="nextcloud_mcp_server.client.notes")` over plain `caplog.at_level("WARNING")` to avoid false positives from other loggers. (#736) +- **F6** — Wire-through tests at the right layer: pure-unit tests on the leaf class are great, but a test that the parent (`NextcloudClient.from_env`) actually threads the param through is a common gap. (#734) +- **F7** — Production-evidence regressions get a unit test: any bug found via CloudWatch / production logs (not in tests) needs a unit test added in the fix PR. (#746) +- **F8** — Test docstring consistency: if 4 of 5 new tests have docstrings, give the 5th one too. (#735) + +### G. Configuration, docs & ADRs + +- **G1** — New config knobs go through `Settings` + dynaconf validator: any new env var read directly via `os.getenv` in module code (rather than via `Settings`) is a finding. (#750-r5) +- **G2** — Resolve config lazily: `default = settings.X` at function-call time, not at decoration / module-import time, so test overrides work. (#750-r5) +- **G3** — User-facing docs cover non-obvious costs: new code paths with measurable latency (unbounded fetches, per-item round-trips) need a note in `docs/configuration.md` or the relevant ADR. (#750-r2, #750-r6) +- **G4** — ADRs match shipped interface: example code blocks in any modified ADR reflect the actual module's public API. If the ADR shows `Verifier` but the code ships `BatchVerifier`, the ADR is wrong. (#750-r3) +- **G5** — Tool docstrings document hidden round-trips: if an MCP tool now does an extra fetch per result (e.g. post-verification race guard), the docstring says so. (#750-r6) +- **G6** — Migration notes for breaking changes: a PR that silently changes failure modes (e.g. previously-allowed clients now get 401) needs a migration note in the PR description and ideally a startup log line. (#745) +- **G7** — Priority order in env var fallbacks: if a setting reads from `WEBHOOK_INTERNAL_URL` → `NEXTCLOUD_MCP_SERVER_URL` → `/.dockerenv` → localhost, document the order in code comments. (#747) + +### H. Performance hygiene + +- **H1** — No duplicate round-trips: if data needed by a verifier/handler is already in `SearchResult.metadata` or a passed-in object, don't re-scroll/re-fetch it. (#750-r1) +- **H2** — Single source of truth: hardcoded lists of doc types / scopes / app names in multiple files should reference one constant (e.g. `INDEXED_DOC_TYPES`). (#750-r2) +- **H3** — Limit clamping at the right layer: `min(max(1, limit), 200)` at the client layer gives a clear error rather than a confusing server-side mismatch. (#741) +- **H4** — Avoid empty JSON bodies in HTTP requests: `json=body or None` (instead of `json=body`) prevents sending `{}` and a spurious `Content-Type: application/json` header for bodyless calls. (#741) + +### I. MCP response patterns (CLAUDE.md) + +- **I1** — No raw `List[Dict]` from MCP tools: tools return Pydantic models inheriting from `BaseResponse`. FastMCP mangles raw lists into dicts with numeric string keys. **🔴 blocking when violated.** +- **I2** — Co-author trailer in commits: each AI-assisted commit ends with `Co-Authored-By: Claude ...`. Missing trailer on AI-touched commits is a 🟢 note. + +### J. Security & input validation + +- **J1** — Validate untrusted identifiers before HTTP layer: room tokens, paths, IDs that go into URLs must be validated against an allowlist regex (e.g. `r"[A-Za-z0-9]+"` with `fullmatch`) *before* the request is built — not after. Path traversal (`../etc/passwd`) is the canonical example. (#741) +- **J2** — `hmac.compare_digest` on bytes: both sides encoded to `bytes` before comparison. Comparing strings is incorrect. (#747) +- **J3** — HTML-escape user-controlled values in HTML responses: even when the input is gated by an upstream check (e.g. `get_preset(preset_id)` returning `None` for unknown), escape on the success path too. Defense-in-depth, not single-layer. (#747) +- **J4** — Static-config-but-still-rendered fields should be escaped consistently: if dynamic fields are escaped and static fields aren't, the inconsistency itself is the smell. (#747) +- **J5** — Privacy defaults: defaults for polling/read-receipt parameters should minimize side effects (e.g. `no_status_update=True` for chat polling so users don't appear "online" on every poll). (#741) +- **J6** — Server-layer hardening even when client supports it: if an MCP tool calls a client with hardcoded safe values (e.g. `look_into_future=False`), don't expose the unsafe option as a tool parameter. (#741) +- **J7** — Redundant regex anchors with `fullmatch()`: `re.compile(r"^[A-Za-z0-9]+$").fullmatch(...)` — the `^`/`$` are redundant and misleading. Use `re.compile(r"[A-Za-z0-9]+")` with `fullmatch`. (#741) + +### K. Trust boundaries + +- **K1** — Comment where input comes from at trust boundaries: webhook payloads, request body fields, query params — say "user-controlled" or "static config" so future readers know the threat model. (#747) +- **K2** — Differentiate HTTP status codes by retry semantics: 503 for "try again" (queue saturated, dependency down), 500 for "broken" (genuine bug). 401 without `WWW-Authenticate` is correct when the caller has no auth state machine (e.g. Nextcloud webhook delivery). (#747) +- **K3** — Trust-boundary comments at API entrypoints: when a `uri` field in a request body is taken at face value (e.g. Astrolabe provides its own webhook URI), document the trust assumption. (#747) + +### L. Sentinel values & PHP API quirks + +- **L1** — `0` as a valid value vs `None` as "absent": fields like `lastReadMessage` or `expirationTimestamp` may use `0` for "no messages read" or "never expires". Don't truthy-test them. Document semantics in field docstrings. (#741) +- **L2** — PHP empty-array-as-list quirks: spreed and other PHP backends serialize empty objects as `[]`. Pydantic validators must coerce `[] → {}` for object-shaped fields and `[] → None` for nullable single-object fields. (#741) +- **L3** — `isinstance(value, (date, datetime))` is redundant: `datetime` is a subclass of `date`. Either drop `datetime` from the tuple, or distinguish behavior between the two. (#735) +- **L4** — Field description matches serialized form: a field documented as "ISO date format" but accepting `datetime` will serialize as `"2000-01-01T12:30:00"`, not `"2000-01-01"`. Either coerce to date or update the description. (#735) + +### M. Defensive code patterns & symmetry + +- **M1** — `getattr(obj, "field", None)` masks future typos: when an attribute is *expected* to exist post-fix, prefer direct access so a typo (`document_recieve_stream`) fails loudly. The defensive `getattr` is the right call only when the attribute is genuinely optional. (#746) +- **M2** — Both-credentials / multiple-mode case has explicit precedence: when two mutually-exclusive params can both be set, either raise `ValueError` or `logger.warning` — silent precedence is a footgun. (#734) +- **M3** — Defensive dict construction over passing `None`: `kwargs = {"password": password, "auth_type": "basic"} if password else {"auth_type": "bearer"}` — don't pass `None` into constructors that don't accept it. (#734) +- **M4** — Speculative defensive code without observed cause: if you add an unwrap for `[{...}] → {...}` "in case the API returns it", cite the version/route where it's been observed. Otherwise it silently masks contract changes. (#736) +- **M5** — Idempotency of URL/path transforms: `_resolve_url` should be safe to call twice. If it adds `/index.php` only when missing, document and test the idempotency. (#733) + +--- + +## What NOT to Flag + +- **Pre-existing technical debt** outside the diff. Note as 💡 if relevant, but don't list as a blocking finding. +- **Style violations the linter already catches.** ruff and ty are authoritative for what they cover. Manual style checklist is for things they don't cover (PEP 604 unions, lazy logging, `Optional` vs `|`). +- **Code formatting** — `ruff format --check` is the source of truth. +- **Bikeshed-tier preferences** (variable naming taste, line breaks, comment phrasing). +- **Anything you'd phrase as "I would have done it differently"** without a concrete repo-pattern violation. +- **Findings that duplicate the bot's previous review** if there's an open PR with one already (use `gh pr view --comments` to check). + +If a category has zero applicable findings, omit it from the punch list. Don't pad. + +--- + +## Output Template + +``` +# Pre-push review for + +**Base:** · **Commits:** · **Files changed:** +**Change shape:** + +## Automated checks +- ruff: ✅ / ❌ +- format: ✅ / ❌ +- ty: ✅ / ❌ +- unit tests: ✅ / ❌ (X passed, Y failed) +- smoke (if run): ✅ / ❌ + +[If any check failed, stop here and report. Otherwise continue.] + +## Findings + +### 🔴 Blocking () + +1. **[J1]** `nextcloud_mcp_server/client/talk.py:42` — Token interpolated into + URL before validation; path-traversal risk. Move `_validate_token(token)` + above the `httpx.URL(...)` call. + +### 🟡 Important () + +1. **[D2]** `nextcloud_mcp_server/models/semantic.py:87` — `verified_count` + description says "unique documents" but value is `len(verified_results)` + (chunk count). Either reword or dedup the value. + +2. **[F1]** `tests/unit/search/test_verification.py` — 403 tests exist for + notes/deck verifiers but not for files/news. Add for symmetry. + +### 🟢 Nits () + +1. **[A1]** `nextcloud_mcp_server/server/deck.py:118` — `Optional[int]` → + `int | None` per CLAUDE.md. + +### 💡 Suggestions + +1. `nextcloud_mcp_server/auth/client_registry.py` — Pre-existing legacy + `Dict`/`Optional` typing throughout. If touching anyway, opportunity + to modernize. + +## Strengths + +- 🎉 **[E6]** Allowlist enforced on both cache-hit and cache-miss paths in + `verify_token_for_management_api`, with a dedicated test + (`test_cache_hit_also_enforces_allowlist`). Exemplary security gate. +- 🎉 **[F3]** New `test_supported_doc_types_covers_indexed_types` enforces + registry exhaustiveness — will catch any future doc_type added without a + verifier. + +## Notes + +- 5 commits on branch; all have `Co-Authored-By` trailer ✓ +- ADR-019 implementation checklist all marked [x] (G4 ✓) +``` + +### Clean diff (no findings) + +``` +# Pre-push review for fix/notes-etag-handling + +**Base:** master · **Commits:** 2 · **Files changed:** 3 +**Change shape:** client layer + +## Automated checks +- ruff: ✅ +- format: ✅ +- ty: ✅ +- unit tests: ✅ (412 passed) + +## Findings + +No checklist violations found. + +## Strengths + +- 🎉 **[B1]** ETag parsing hoisted into its own try/except before the + network call — error logs will be specific to malformed ETag, not + generic "unexpected error". + +Safe to push. +``` + +--- + +## Notes for the Running Model + +- Read the **whole diff** before composing the report. Cross-file patterns + (F1 symmetry, H2 single-source-of-truth, E5 lifespan symmetry) are only + visible in aggregate. +- Keep findings **specific and short**. One sentence each plus a fix direction. + The point is to feed them back into the main loop for fixing, not to write + a treatise. +- If an automated check fails, **stop and report** — don't run the checklist + on top of a broken build. +- The checklist is calibrated to *this repo's* recurring review feedback. If + the diff is in an unrelated area (CI config, random docs), most items won't + apply — say so rather than padding. +- **Don't fix anything.** This skill reports; the main loop fixes. Even if a + finding is a one-character change, leave it for the user to action. +- Use `gh pr view --comments` (not `gh api`) when checking for prior bot + reviews on an existing PR — see memory feedback_use_gh_pr. diff --git a/CLAUDE.md b/CLAUDE.md index 8b93202e..96fb337c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,11 +23,21 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ``` ### Code Quality -- **Run ruff and ty before committing**: +- **Before committing or pushing, invoke the `pre-push-review` skill**: + - Runs `ruff check`, `ruff format --check`, `ty check`, and unit tests, then audits the + branch diff against this repo's recurring PR-review patterns (mined from PRs #733–#750). + - Output is a labelled punch list (🔴 blocking / 🟡 important / 🟢 nit). The main loop + fixes; the skill reports. + - Skill location: `.claude/skills/pre-push-review/SKILL.md`. Invoke via the `Skill` tool + with `skill="pre-push-review"`, or when the user types `/pre-push-review`. + - Skip only for tiny diffs (typo, README tweak, single-line dependency bump) or when the + user explicitly says "just push it". +- **Manual fallback** (if the skill is unavailable): ```bash uv run ruff check uv run ruff format uv run ty check -- nextcloud_mcp_server + uv run pytest tests/unit/ -x -q ``` - **Ruff configuration** in pyproject.toml (extends select: ["I"] for import sorting) diff --git a/docker-compose.yml b/docker-compose.yml index 8f3bda13..8d993c6d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -36,7 +36,7 @@ services: # Mount OIDC development directory outside /var/www/html to avoid rsync conflicts # The post-installation hook will register /opt/apps as an additional app directory #- ./third_party:/opt/apps:ro - - ./third_party/astrolabe:/opt/apps/astrolabe:ro + #- ./third_party/astrolabe:/opt/apps/astrolabe:ro #- ./third_party/oidc:/opt/apps/oidc:ro environment: - NEXTCLOUD_TRUSTED_DOMAINS=app diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 977851cc..5bdb6fbb 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -177,8 +177,16 @@ def configure_semantic_tools(mcp: FastMCP): # are about to be dropped. Pass the lifespan-owned task group so # eviction of dropped points is fire-and-forget (does not block # the response). - eviction_task_group = getattr( - ctx.request_context.lifespan_context, "eviction_task_group", None + # Direct attribute access — both AppContext and OAuthAppContext + # expose ``eviction_task_group`` as a @property (see app.py), + # reading dynamically from the module-level VectorSyncState + # singleton. A defensive ``getattr(..., None)`` here would mask + # typos; if a future lifespan-context type forgets the property, + # AttributeError surfaces during the first search rather than + # silently degrading to inline eviction for the life of the + # process. + eviction_task_group = ( + ctx.request_context.lifespan_context.eviction_task_group ) verified_results, dropped_count = await verify_search_results( client, @@ -507,8 +515,11 @@ def configure_semantic_tools(mcp: FastMCP): """Fetch full content for a single document (parallel with semaphore).""" async with semaphore: if result.doc_type == "note": + # SemanticSearchResult.id is typed `int` (Pydantic enforces + # at construction); no defensive cast is needed here. The + # catch-all below covers only the verify-then-delete race. try: - note = await client.notes.get_note(int(result.id)) + note = await client.notes.get_note(result.id) content = note.get("content", "") accessible_results[index] = result full_contents[index] = content