diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index 3e729a6e..ec8d6226 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -251,7 +251,7 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `pyproject.toml` | Tool config — `[tool.ruff]`, `[tool.pytest.ini_options]`, `[tool.coverage.*]`, `[tool.bandit]`, and `[tool.importlinter]`: ADR-7's layer contracts with today's recorded exceptions | | `.gitattributes` | Forces LF on `scripts/scaleway/` — those files run on, or are handed to, a Linux host, and a Windows checkout with `core.autocrlf=true` otherwise ships them CRLF (a cloud-init once arrived as `#cloud-config` + CR). Exempts `static/vendor/` from any end-of-line conversion, so the vendored builds stay byte-identical to their recorded integrity | | `docker-compose.yml` | Full stack: app + PostgreSQL + Redis + Ollama; `--profile mcp` adds MCP servers | -| `Dockerfile` | Multi-stage build on Docker Hardened Images — `dhi.io/python:3.12-dev` builds the venv, `dhi.io/python:3.12` runs it as uid 65532 with no shell or package manager | +| `Dockerfile` | Multi-stage build on Docker Hardened Images — `dhi.io/python:3.12-dev` builds the venv, `dhi.io/python:3.12` runs it as uid 65532 with no shell or package manager; source and venv owned by root, only `logs/`, `uploads/` and `data/` writable (R-11) | | `docker-entrypoint.py` | Container entrypoint — expands `SERVER_PORT`/`UVICORN_WORKERS`/`UVICORN_TIMEOUT` (the hardened base has no shell to do it) and `exec`s uvicorn so it stays PID 1 | | `docs/DEPLOYMENT.md` | Docker Compose deployment: secrets, resource limits, upgrade/rollback, TLS, security checklist | | `docs/grafana-dashboard.json` | Importable Grafana dashboard (uid `localchat-rag-v1`, 20 panels; the Model Usage row is OBS-1a) | diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 69d917fa..25315191 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -666,6 +666,28 @@ jobs: fi echo "OK: uid 65532" + - name: The runtime user can write only its own directories + # R-11: the source and the venv are root's, so code execution in the app cannot + # persist by rewriting a module; logs, uploads and data are the app's. + run: | + docker run --rm --entrypoint python localchat:smoke -c ' + import os, site, sys + own = ("/app/data", "/app/logs", "/app/uploads") + writable = [ + p + for top in ("/app", "/opt/venv") + for root, dirs, files in os.walk(top) + if not root.startswith(own) + for p in [root, *(os.path.join(root, f) for f in files)] + if os.access(p, os.W_OK) + ] + missing = [p for p in own if not os.access(p, os.W_OK)] + on_path = [p for p in sys.path if p and os.access(p, os.W_OK)] + problems = writable[:5] + missing + on_path + (["ENABLE_USER_SITE"] if site.ENABLE_USER_SITE else []) + sys.exit("::error::write access not as expected: " + ", ".join(problems) if problems else 0) + ' + echo "OK: /app and /opt/venv read-only but for logs, uploads, data; nothing writable on sys.path" + - name: No shell and no package manager run: | docker run --rm --entrypoint python localchat:smoke -c ' diff --git a/CHANGELOG.md b/CHANGELOG.md index ffe587a7..ec0756b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ OpenAI, `https://api.openai.com/v1`) and drop any `provider/` prefix from `CLOUD - **`litellm` is gone from the image (QP-11, ADR-4).** It carried CVE-2026-114612, in a proxy server LocalChat never ran, and every fixed version pulls `boto3` into the image. The `pip-audit` suppression for it is removed; `SECURITY.md` item 12 is resolved. +- **The image's code is no longer writable by the process that runs it (R-11).** The source and the virtual environment were owned by uid 65532, so code execution in the app could persist by rewriting a module. Both now belong to root. The runtime user writes only `/app/logs`, `/app/uploads` and a new `/app/data`. - **The public webhook receiver no longer reveals which connector ids exist.** It answered 404 for an unknown id and 400 for a connector of another type before checking the secret, so an anonymous caller could probe for ids and for which were webhooks. Every refusal @@ -45,6 +46,7 @@ OpenAI, `https://api.openai.com/v1`) and drop any `provider/` prefix from `CLOUD `CLOUD_PRICE_OUTPUT_PER_MTOK`; without both, only tokens are counted. A provider that is not OpenAI-shaped (Anthropic's native API, Vertex) is no longer reachable, as ADR-4 accepted. +- **Settings and fine-tuned rerankers live in `/app/data`, on a new `app_data` volume.** `STATE_FILE` (new, default `app_state.json`) and `RERANKER_MODEL_PATH` point there in the image, and a fine-tune now writes its versions beside `RERANKER_MODEL_PATH` instead of a fixed `./models/reranker`. The active model and RAG parameters therefore survive a container recreation, which they never did. **Upgrading:** the settings page's values held in the old container are not carried over; set them once more after the upgrade. A fine-tuned reranker in the old container's `/app/models` is likewise left behind and falls back to the base model until retrained; promoting or rolling back to a version trained there is refused rather than marked active. Host runs are unchanged. - **A purge destroys only what was retired first (R-9).** The single-object purges of a document, conversation, workspace, connector and user checked references and nothing else, so a live row could be hard-deleted in one call, skipping the reversible step diff --git a/Dockerfile b/Dockerfile index 125d6bce..236c6639 100644 --- a/Dockerfile +++ b/Dockerfile @@ -57,28 +57,37 @@ RUN python -m venv --copies /opt/venv && \ # Writable runtime directories, created here because the runtime stage has no # shell to mkdir with. COPY is the only way to introduce a directory there. -RUN mkdir -p /skel/logs /skel/uploads +RUN mkdir -p /skel/logs /skel/uploads /skel/data # ---- Stage 2: runtime ---------------------------------------- FROM dhi.io/python:3.12@sha256:7c247af7f603bba8197ad5c34595066e1e6b81644c5a37b576d157979ceb4ea6 AS runtime +# The base image already runs as 65532, and WORKDIR creates its directory as the +# current user — so without this, /app itself was the runtime user's, and a module +# dropped there would shadow any import (R-11). Switched back below. +USER 0:0 + # No libpq layer: psycopg[binary] vendors libpq inside the wheel, and the # hardened base has no apt to install a system copy with. Verified by importing # psycopg in the built image — see the docker-smoke job in tests.yml. -# Copy virtual environment from builder -COPY --from=builder --chown=65532:65532 /opt/venv /opt/venv +# Copy virtual environment from builder. Owned by root, like the source below: the +# runtime user reads and runs it but cannot rewrite it, so code execution in the app +# cannot persist by editing a module (QP-9 review, R-11). +COPY --from=builder /opt/venv /opt/venv ENV PATH="/opt/venv/bin:$PATH" WORKDIR /app -# Copy application source -COPY --chown=65532:65532 . /app +# Copy application source, owned by root (see the venv above) +COPY . /app -# Runtime directories (see the builder stage for why they arrive by COPY) +# The only directories the runtime user may write (see the builder stage for why they +# arrive by COPY). data/ holds the settings page's state and fine-tuned rerankers. COPY --from=builder --chown=65532:65532 /skel/logs /app/logs COPY --from=builder --chown=65532:65532 /skel/uploads /app/uploads +COPY --from=builder --chown=65532:65532 /skel/data /app/data # uid:gid of the hardened base image's own nonroot user. Numeric because the # image carries no /etc/passwd entry this file could refer to by name. @@ -90,6 +99,8 @@ USER 65532:65532 ENV PYTHONDONTWRITEBYTECODE=1 \ PYTHONUNBUFFERED=1 \ APP_ENV=production \ + STATE_FILE=/app/data/app_state.json \ + RERANKER_MODEL_PATH=/app/data/reranker/latest \ SERVER_HOST=0.0.0.0 \ SERVER_PORT=5000 \ LOG_FORMAT=json diff --git a/SECURITY.md b/SECURITY.md index ae1f3491..9dcbc189 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -336,7 +336,7 @@ fails on the code before it. | R-8 | Low | Feedback and annotations accept conversation and chunk ids from another workspace; a viewer can make any document un-purgeable | **Fixed** — every id a rating or annotation names is looked up in the caller's scope first, the global tier's chunks included except for a workspace API key; the message id too, which the review did not name. `tests/integration/test_object_authorization_over_the_wire.py`, `tests/integration/test_rating_ids_are_in_scope.py` | | R-9 | Low | Single-object purges do not require the row to be retired first | **Fixed** — each of the five purges locks its row and refuses unless it is retired, inside its own transaction: a live row is a 409 and survives, a missing one a 404. A user purge thereby cannot reach an active admin. `tests/unit/test_purge_requires_retired.py`, `tests/integration/test_purge_requires_retired.py` | | R-10 | Low | Admin user routes take raw dicts; the last-admin count ignores `is_active` | **Fixed** — the three routes validate a Pydantic body, so a string `"false"` no longer passes the last-admin rule (Postgres stored it as false), and the rule counts active admins only. The env-var admin's bootstrap check still counts live admins. `tests/unit/test_admin_user_bodies_are_validated.py`, `tests/integration/test_admin_count_is_of_active_admins.py`. Known residual: the rule reads the count and writes on separate connections, so the last two active admins deactivating each other at the same moment both succeed; the repair is SQL by hand (older than R-10) | -| R-11 | Low | The image's source is owned by the runtime user | **Open** | +| R-11 | Low | The image's source is owned by the runtime user | **Fixed** — the source and the venv belong to root; the runtime user writes only `/app/logs`, `/app/uploads` and `/app/data`, where the settings state and fine-tuned rerankers now live. A model the reranker loads must lie in that directory and may bring no code of its own (`trust_remote_code=False`). `docker-smoke` walks `/app` and `/opt/venv` and fails on anything else writable, or anything writable on `sys.path` | | R-12 | Low | GraphRAG's entity expansion is not scoped (off by default) | **Fixed** — a co-occurrence counts only through a live document of the caller's workspace; retired documents contributed too. `tests/integration/test_graph_expansion_scope.py` | | — | — | `GET /api/workspaces/active` and `POST /api/workspaces/switch` returned any named workspace, its system prompt included, to anyone signed in; found by R-5's new probes | **Fixed** (#510) — `tests/unit/test_active_workspace_is_authorised.py` | diff --git a/docker-compose.yml b/docker-compose.yml index 6349f5b1..35753b48 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -443,6 +443,7 @@ services: volumes: - app_uploads:/app/uploads - app_logs:/app/logs + - app_data:/app/data # MM-2: ingestion (chunk + batch-embed) is the spiky path here, so the # limit sits above the idle footprint with room for a large upload. deploy: @@ -479,6 +480,8 @@ volumes: driver: local app_logs: driver: local + app_data: + driver: local # ────────────────────────────────────────────────────────────── # Networks diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 9620e8c5..a8cbe35e 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -386,7 +386,7 @@ to empty, in which case the router falls back to the active model. | `RERANK_MIN_SCORE` | `-5.0` | Cross-encoder score below which a chunk is dropped | | `RERANK_LOW_RELEVANCE_LIMIT` | `3` | Chunks kept when everything scores below the threshold | | `RERANKER_WEIGHT` | `0.3` | The reranker score's share of the final blend | -| `RERANKER_MODEL_PATH` | `./models/reranker/latest` | Fine-tuned model directory; falls back to the base cross-encoder | +| `RERANKER_MODEL_PATH` | `./models/reranker/latest` (host-run); `/app/data/reranker/latest` (image) | Fine-tuned model directory; falls back to the base cross-encoder. A fine-tune writes its versions beside it | | `FEEDBACK_FINETUNE_MIN_PAIRS` | `50` | Feedback pairs required before a fine-tune run starts | | `EMBEDDING_CONCURRENT_BATCHES` | `2` | Parallel embedding batches during ingest | | `PDF_LOADER` | `auto` | PDF extraction backend | @@ -442,6 +442,7 @@ to empty, in which case the router falls back to the active model. | Variable | Default | Effect | |---|---|---| | `APP_VERSION` | `3.5.0` | Reported as `system.app_version` by `GET /api/settings/stats` (admin). See the note below | +| `STATE_FILE` | `app_state.json` (host-run); `/app/data/app_state.json` (image) | Where the settings page persists the active model and RAG parameters. In compose it lives on the `app_data` volume | | `MAX_CONTENT_LENGTH` | `16777216` | Upload ceiling in bytes (16 MB), enforced while the upload streams to disk | | `UVICORN_WORKERS` | `1` | Worker processes. **Only `1` is supported** — the app refuses to boot otherwise | | `UVICORN_TIMEOUT` | `600` | uvicorn's `--timeout-keep-alive`, applied by `docker-entrypoint.py`. **Not a request timeout**: it closes a connection that has been idle this long, which for an SSE stream means one that has produced nothing. Keep it ≥ `OLLAMA_EMBED_TIMEOUT`; behind the nginx overlay, `proxy_read_timeout 300s` in `nginx/nginx.conf` is the lower and therefore real ceiling | diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 93ba3ca2..c526c26e 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -135,8 +135,9 @@ into a **restart loop**: PermissionError: [Errno 13] Permission denied: '/app/logs/app.log' ``` -The runtime image runs as **uid 65532**, and the `Dockerfile` creates `/app/logs` and -`/app/uploads` owned by it. But both paths are **named volumes**, and a volume keeps the +The runtime image runs as **uid 65532**, and the `Dockerfile` creates `/app/logs`, +`/app/uploads` and `/app/data` owned by it — the only paths it may write; the source and +the venv belong to root (R-11). But logs and uploads are **named volumes** that may predate it, and a volume keeps the ownership it was created with — uid 1000 under the pre-hardening image. The image is correct; the volume predates it. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 39c1eaf3..613bc8a6 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1925,7 +1925,7 @@ From `SECURITY.md`'s QP-9 register, in this order: migration 0024). 3. ✅ R-10: admin user routes take raw dicts; the last-admin count ignores `is_active`. 4. ✅ R-5b: a ratchet over the `ALL_WORKSPACES` call sites, each with a recorded reason. -5. R-9 ✅, R-11, R-12 ✅: purge preconditions, the image's file ownership, GraphRAG's scope. +5. ✅ R-9, R-11, R-12: purge preconditions, the image's file ownership, GraphRAG's scope. #### QP-11 — Replace `litellm` (ADR-4) ✅ (built 2026-11-07, taken ahead of the rest of QP-10) @@ -2093,7 +2093,7 @@ Findings from the 2026-07 external code-quality audit that are real but delibera - **`STATE_FILE` (`app_state.json`, `src/config.py`) resolves relative to the working directory, not to a guaranteed-writable path.** `AppState.set_active_model()` / `set_rag_param()` write it; `_save_state()` catches and logs any failure rather than crashing, so every failure mode here is silent-degrade. **Rewritten 2026-08-27 — the evidence this entry was built on no longer exists.** It argued from the Helm chart's `readOnlyRootFilesystem: true`, concluded that "on a real Helm deployment today, every write fails", and set its re-review trigger as "before the Helm chart is used for a real deployment". **PG-0 deleted the Helm chart on 2026-08-05** (`8294604`), three weeks before this was last edited. The failure it described is now unreachable and the trigger it named can never fire — an entry that had quietly become undismissable rather than parked. **What is actually true under the two supported topologies**, re-derived from the code: - - **Docker.** `WORKDIR /app`, `/app` is `chown`ed to uid 65532, and nothing sets `read_only`. The write **succeeds**. But the `app` service mounts only `app_uploads:/app/uploads` and `app_logs:/app/logs`, so `/app/app_state.json` sits in the container's writable layer and is **lost on every container recreation** — `docker compose up -d --build`, an image bump, any `docker compose down`. The active model and RAG parameters silently revert to their config defaults. + - **Docker.** *Since R-11 (2026-11-08) `/app` belongs to root and the image sets `STATE_FILE=/app/data/app_state.json` on the `app_data` volume, so the state now survives recreation; the coherence argument below still stands.* Before that: `WORKDIR /app`, `/app` is `chown`ed to uid 65532, and nothing sets `read_only`. The write **succeeds**. But the `app` service mounts only `app_uploads:/app/uploads` and `app_logs:/app/logs`, so `/app/app_state.json` sits in the container's writable layer and is **lost on every container recreation** — `docker compose up -d --build`, an image bump, any `docker compose down`. The active model and RAG parameters silently revert to their config defaults. - **Host run.** Writes land in the working directory and persist. No failure. So the defect survived the chart's deletion, but it is *lost on redeploy*, not *fails on write* — a different symptom, a different trigger, and one that fires on the topology the project actually ships. **The fix is unchanged, and the reasoning for it is stronger.** `AppState` is shared mutable config; a writable path or a volume mount gives persistence without coherence. The destination is a small `app_settings` table in the Postgres this app already requires, read and written through `src/db/` like every other piece of durable state. That closes the redeploy loss, the multi-instance divergence in the first bullet of this section, and any future read-only-filesystem constraint, instead of patching each symptom separately. **Re-review trigger**: the first time a support question is "why did my active model reset?", or before any deployment where redeploys are routine — which is every containerised one, so this is closer to scheduled than parked. diff --git a/src/config.py b/src/config.py index 14655d28..c113b181 100644 --- a/src/config.py +++ b/src/config.py @@ -640,8 +640,9 @@ def _production_errors(errors: list[str]) -> None: # this project's log sanitiser. Set to DEBUG to get it back while diagnosing one. LOG_THIRD_PARTY_LEVEL: str = os.environ.get('LOG_THIRD_PARTY_LEVEL', 'WARNING') -# State persistence file -STATE_FILE: str = 'app_state.json' +# State persistence file. The image points it into /app/data, the one directory there +# the runtime user may write besides logs and uploads. +STATE_FILE: str = os.environ.get('STATE_FILE', 'app_state.json') # ============================================================================ # RERANKER CONFIGURATION (Feature 5.2) diff --git a/src/rag/feedback_pipeline.py b/src/rag/feedback_pipeline.py index 659ab370..c560d6e2 100644 --- a/src/rag/feedback_pipeline.py +++ b/src/rag/feedback_pipeline.py @@ -89,20 +89,23 @@ def persist_reranker_version(db: Any, result: dict) -> str | None: def promote_model(db: Any, version_id: str) -> bool: """Set *version_id* as active and update the latest.txt pointer. Returns success. - DB writes are delegated to the db-layer FeedbackRepository (HK-7); file-pointer update - and reranker reload remain here as non-DB concerns. + The model must still exist inside the model directory, and the pointer is written + before the row is activated: a version trained where the image can no longer write + (``./models`` before R-11) is refused rather than marked active while the old model + keeps serving. DB writes are delegated to the db-layer FeedbackRepository (HK-7). """ try: - model_path = db.feedback.activate_reranker_version(version_id) + from ..rag.reranker import confined_model_path, reload_reranker + + version = next((v for v in db.feedback.reranker_versions() if v["id"] == version_id), None) + model_path = confined_model_path(version["model_path"]) if version else None if model_path is None: + logger.warning("[Pipeline] Promote refused: no such version, or its model is gone or misplaced") + return False + (Path(model_path).parent / "latest.txt").write_text(model_path, encoding="utf-8") + if db.feedback.activate_reranker_version(version_id) is None: return False logger.info("[Pipeline] Promoted reranker version") - # Update latest.txt - if model_path: - pointer = Path(model_path).parent / "latest.txt" - pointer.parent.mkdir(parents=True, exist_ok=True) - pointer.write_text(model_path, encoding="utf-8") - from ..rag.reranker import reload_reranker reload_reranker(model_path) return True except Exception as exc: # noqa: BLE001 — promotion is reported as False; the running model is untouched @@ -118,7 +121,7 @@ def rollback_model(db: Any, version_id: str) -> bool: def finetune_reranker( pairs: list[dict], base_model: str = "cross-encoder/ms-marco-MiniLM-L-6-v2", - output_dir: str | Path = "./models/reranker", + output_dir: str | Path | None = None, epochs: int = 3, batch_size: int = 16, ) -> dict[str, Any]: @@ -136,7 +139,11 @@ def finetune_reranker( logger.info("[Pipeline] No training pairs — skipping fine-tune") return {"pair_count": 0, "skipped": True} - # Use a versioned subdirectory so previous models are preserved + # Use a versioned subdirectory so previous models are preserved. By default beside + # RERANKER_MODEL_PATH, whose .txt pointer promote_model writes and the reranker reads. + if output_dir is None: + from .. import config + output_dir = Path(config.RERANKER_MODEL_PATH).parent versioned_dir = _versioned_output_dir(output_dir) # Build sentence-transformers InputExample format @@ -156,7 +163,7 @@ def finetune_reranker( split = int(len(train_samples) * 0.8) train_set, eval_set = train_samples[:split], train_samples[split:] - model = CrossEncoder(base_model, num_labels=1) + model = CrossEncoder(base_model, num_labels=1, trust_remote_code=False) # Baseline NDCG on eval set (before fine-tuning) eval_labels = [int(s.label) for s in eval_set] @@ -232,7 +239,8 @@ def run(db: Any, days: int = 7, output: str | Path | None = None, finetune: bool parser.add_argument("--days", type=int, default=7, help="Days of feedback to export") parser.add_argument("--output", type=str, default=None, help="JSONL output path") parser.add_argument("--finetune", action="store_true", help="Run reranker fine-tuning") - parser.add_argument("--model-dir", type=str, default="./models/reranker", help="Output dir for fine-tuned model") + parser.add_argument("--model-dir", type=str, default=None, + help="Output dir for fine-tuned model (default: beside RERANKER_MODEL_PATH)") args = parser.parse_args() from src.db import db as _db diff --git a/src/rag/reranker.py b/src/rag/reranker.py index d51eea50..29bc3c65 100644 --- a/src/rag/reranker.py +++ b/src/rag/reranker.py @@ -55,6 +55,26 @@ def _to_safe_path(raw: str) -> str | None: real = os.path.realpath(raw) return real if os.path.exists(real) else None +def confined_model_path(raw: str | None) -> str | None: + """*raw* resolved, if it exists inside the directory ``RERANKER_MODEL_PATH`` sits in. + + A pointer file or a version row names the model to load; confining it means a + path written by anything else cannot pick what the reranker loads (R-11). + """ + safe = _to_safe_path(raw) if raw else None + if safe is None: + return None + root = os.path.realpath(os.path.dirname(config.RERANKER_MODEL_PATH) or ".") + try: + inside = os.path.commonpath([root, safe]) == root + except ValueError: # different drives on Windows + inside = False + if not inside: + logger.warning("[Reranker] Ignoring a model path outside the model directory") + return None + return safe + + _BASE_MODEL = "cross-encoder/ms-marco-MiniLM-L-6-v2" @@ -112,7 +132,7 @@ def _load(self, model_path: str | None) -> None: resolved = self._resolve_model_path(model_path) if resolved: try: - self._model = CrossEncoder(resolved) + self._model = CrossEncoder(resolved, trust_remote_code=False) self._model_path = resolved logger.info("[Reranker] Loaded fine-tuned model") return @@ -121,27 +141,26 @@ def _load(self, model_path: str | None) -> None: # Fall back to base model try: - self._model = CrossEncoder(_BASE_MODEL) + self._model = CrossEncoder(_BASE_MODEL, trust_remote_code=False) self._model_path = _BASE_MODEL logger.info(f"[Reranker] Loaded base model: {_BASE_MODEL}") except Exception as exc: # noqa: BLE001 — and if the base will not load either, the reranker is simply off logger.warning(f"[Reranker] Could not load base model: {exc}") def _resolve_model_path(self, model_path: str | None) -> str | None: - """Return an existing model directory path, or None.""" - candidates: list[str] = [] - if model_path: - candidates.append(model_path) + """Return an existing model directory path, or None. + + The configured path is the operator's; one named by a caller or by the pointer + file must lie beside it. + """ + candidates: list[str | None] = [confined_model_path(model_path)] cfg_path = config.RERANKER_MODEL_PATH if cfg_path: - candidates.append(cfg_path) + candidates.append(_to_safe_path(cfg_path)) safe_cfg = os.path.realpath(cfg_path) if '\x00' not in cfg_path else None if safe_cfg: - pointed = _read_latest_pointer(safe_cfg) - if pointed: - candidates.append(pointed) - for p in candidates: - safe = _to_safe_path(p) + candidates.append(confined_model_path(_read_latest_pointer(safe_cfg))) + for safe in candidates: if safe: return safe return None diff --git a/tests/unit/test_feedback.py b/tests/unit/test_feedback.py index 5dd98069..527ef065 100644 --- a/tests/unit/test_feedback.py +++ b/tests/unit/test_feedback.py @@ -15,6 +15,7 @@ import json import uuid +from pathlib import Path from unittest.mock import MagicMock, patch import pytest @@ -368,6 +369,22 @@ def test_finetune_reranker_builds_eval_pairs_from_input_examples(self, tmp_path) called_eval_pairs = mock_model.predict.call_args[0][0] assert all(isinstance(p, tuple) and len(p) == 2 for p in called_eval_pairs) + def test_finetune_reranker_writes_beside_the_model_the_reranker_loads(self, tmp_path): + """R-11: in the image only /app/data is writable, so the default follows RERANKER_MODEL_PATH.""" + from src import config + from src.rag.feedback_pipeline import finetune_reranker + + pairs = [{"query": f"q{i}", "chunk_text": f"c{i}", "label": i % 2} for i in range(10)] + mock_model = MagicMock() + mock_model.predict.return_value = [0.9, 0.1] + with patch.object(config, "RERANKER_MODEL_PATH", str(tmp_path / "reranker" / "latest")), \ + patch("sentence_transformers.CrossEncoder", return_value=mock_model): + result = finetune_reranker(pairs) + + written = Path(result["output_path"]) + assert (written.parent, (tmp_path / "reranker" / "latest.txt").read_text(encoding="utf-8")) == ( + tmp_path / "reranker", str(written)) + def test_write_jsonl_creates_file(self, tmp_path): from src.rag.feedback_pipeline import write_jsonl pairs = [{"query": "q1", "chunk_text": "c1", "label": 1}] diff --git a/tests/unit/test_reranker.py b/tests/unit/test_reranker.py index 47e86feb..81d37aa1 100644 --- a/tests/unit/test_reranker.py +++ b/tests/unit/test_reranker.py @@ -153,7 +153,8 @@ def cross_encoder_side_effect(path, **kwargs): mock_st = MagicMock() mock_st.CrossEncoder = mock_cls - with patch.dict("sys.modules", {"sentence_transformers": mock_st}): + # The fine-tuned model must sit beside RERANKER_MODEL_PATH to be tried at all (R-11). + with patch.dict("sys.modules", {"sentence_transformers": mock_st}), patch("src.rag.reranker.config.RERANKER_MODEL_PATH", str(tmp_path / "latest")): m = RerankerModel(model_path=str(fine_tuned_dir)) assert m.is_available() is True diff --git a/tests/unit/test_reranker_model_is_confined.py b/tests/unit/test_reranker_model_is_confined.py new file mode 100644 index 00000000..fcf6ff5a --- /dev/null +++ b/tests/unit/test_reranker_model_is_confined.py @@ -0,0 +1,83 @@ +"""R-11 (#529 review) — what the reranker loads stays inside its model directory. + +In the image only ``/app/data`` is writable, and the reranker's model directory lives +there. A pointer file or a version row that names a path elsewhere is ignored, and a +promotion is refused before the database marks a version active that cannot be loaded. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from src import config +from src.rag.feedback_pipeline import promote_model +from src.rag.reranker import RerankerModel, confined_model_path + +pytestmark = pytest.mark.unit + + +@pytest.fixture() +def model_dir(tmp_path): + base = tmp_path / "reranker" + (base / "v1").mkdir(parents=True) + outside = tmp_path / "elsewhere" + outside.mkdir() + with patch.object(config, "RERANKER_MODEL_PATH", str(base / "latest")): + yield base, outside + + +class TestTheConfinement: + def test_a_version_beside_the_configured_path_is_kept(self, model_dir): + base, _ = model_dir + assert confined_model_path(str(base / "v1")) == str((base / "v1").resolve()) + + @pytest.mark.parametrize("kind", ["outside", "dotdot", "missing"]) + def test_anything_else_is_refused(self, model_dir, kind): + base, outside = model_dir + raw = {"outside": str(outside), "dotdot": str(base / ".." / "elsewhere"), + "missing": str(base / "v9")}[kind] + assert confined_model_path(raw) is None + + def test_a_pointer_naming_another_directory_is_not_loaded(self, model_dir): + base, outside = model_dir + (base / "latest.txt").write_text(str(outside), encoding="utf-8") + st = MagicMock() + with patch.dict("sys.modules", {"sentence_transformers": st}): + RerankerModel() + assert [c.args[0] for c in st.CrossEncoder.call_args_list] == ["cross-encoder/ms-marco-MiniLM-L-6-v2"] + + def test_no_model_may_bring_its_own_code(self, model_dir): + base, _ = model_dir + (base / "latest.txt").write_text(str(base / "v1"), encoding="utf-8") + st = MagicMock() + with patch.dict("sys.modules", {"sentence_transformers": st}): + RerankerModel() + assert st.CrossEncoder.call_args.kwargs["trust_remote_code"] is False + + +def _db(model_path: str) -> MagicMock: + db = MagicMock() + db.feedback.reranker_versions.return_value = [{"id": "v", "model_path": model_path}] + db.feedback.activate_reranker_version.return_value = model_path + return db + + +class TestPromotion: + def test_a_version_outside_the_model_directory_is_not_activated(self, model_dir): + """A version trained into ./models before R-11 cannot be written beside any more.""" + _, outside = model_dir + db = _db(str(outside)) + with patch("src.rag.reranker.reload_reranker") as reload: + assert promote_model(db, "v") is False + assert (db.feedback.activate_reranker_version.called, reload.called) == (False, False) + + def test_a_version_inside_is_pointed_at_then_activated(self, model_dir): + base, _ = model_dir + db = _db(str(base / "v1")) + with patch("src.rag.reranker.reload_reranker") as reload: + assert promote_model(db, "v") is True + assert ((base / "latest.txt").read_text(encoding="utf-8"), reload.call_args.args[0]) == ( + str((base / "v1").resolve()), str((base / "v1").resolve())) + db.feedback.activate_reranker_version.assert_called_once_with("v")