Skip to content

Commit ca4f9fb

Browse files
HumanBean17claude
andcommitted
feat(config): resolve relative embedding.model against config dir / source_root
A relative `embedding.model` (e.g. `./models/minilm`) was handed to sentence-transformers verbatim and resolved against process CWD — unlike `index_dir` and `source_root`, which anchor on the config file's directory. That made a committed `.java-codebase-rag.yml` non-portable, and could let the CLI indexer and the MCP reader load different models from the same config when their CWDs differed. `maybe_expand_embedding_model_path` now takes optional `config_dir` / `source_root` / `source` kwargs and, after the existing `~` / `$VAR` expansion, resolves a result still `./` / `../`-prefixed to absolute — mirroring `_resolve_index_dir_path`: YAML values anchor on the config file's directory, CLI / env values on `source_root`. Hub ids, absolute paths, `~/`-expanded values, and an env var that already yielded an absolute path are all left untouched. With no base supplied, relative resolution is skipped, so the MCP runtime read (`resolved_sbert_model_for_process_env`) is byte-for-byte unchanged — it receives an already-absolute path from `apply_to_os_environ` in the normal flow. The #239 MCP YAML-loading fix is untouched. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent faeeb38 commit ca4f9fb

3 files changed

Lines changed: 216 additions & 12 deletions

File tree

docs/CONFIGURATION.md

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,13 +98,19 @@ index_dir: ./.java-codebase-rag
9898
embedding:
9999
# Hub id OR local directory containing the sentence-transformers model files.
100100
# - Hub id example: `sentence-transformers/all-MiniLM-L6-v2`
101-
# - Local path examples: `/opt/models/minilm`, `~/models/minilm`, `$MODEL_DIR/minilm`
101+
# - Local path examples: `/opt/models/minilm`, `~/models/minilm`, `$MODEL_DIR/minilm`, `./models/minilm`
102102
# - Resolution applies expanduser + expandvars when the value is path-shaped
103-
# (starts with `/`, `./`, `../`, `~`, or contains `$`). Same rule for
104-
# `SBERT_MODEL` and `--embedding-model` after precedence picks the string.
105-
# Plain `org/name` is treated as a hub id and passed through unchanged.
106-
# A relative path without `./` (e.g. `models/minilm`) is ambiguous with
107-
# hub-id shape — prepend `./` if you mean a local directory.
103+
# (starts with `/`, `./`, `../`, `~`, or contains `$`); a result still
104+
# `./` / `../`-prefixed after that expansion is then resolved to absolute.
105+
# Same rule for `SBERT_MODEL` and `--embedding-model` after precedence picks
106+
# the string. Plain `org/name` is treated as a hub id and passed through
107+
# unchanged. A relative path without `./` (e.g. `models/minilm`) is
108+
# ambiguous with hub-id shape — prepend `./` if you mean a local directory.
109+
# - Relative base (mirrors `index_dir`): a YAML `model` resolves against THIS
110+
# config file's directory; `SBERT_MODEL` / `--embedding-model` resolve
111+
# against the resolved `source_root`. So a committed `model: ./models/minilm`
112+
# is portable across machines and across the CLI indexer vs the MCP reader,
113+
# regardless of process CWD.
108114
# - Env: SBERT_MODEL. CLI: --embedding-model. Default: sentence-transformers/all-MiniLM-L6-v2
109115
model: sentence-transformers/all-MiniLM-L6-v2
110116

@@ -220,7 +226,7 @@ async_producer_overrides:
220226
| Field | Expanded? | Notes |
221227
|---|---|---|
222228
| `index_dir` | partial | `~` expanded; `$VAR` is NOT expanded. A YAML relative path resolves against the config file's directory (same base as `source_root`); the default `./.java-codebase-rag` sits beside the resolved `source_root`. |
223-
| `embedding.model` (when path-shaped) | yes | Path-shape = starts with `/`, `./`, `../`, `~`, or contains `$`. Plain `org/name` is treated as a hub id and passed through. Applies to the value after CLI > env > YAML > default precedence. Long-lived MCP hosts also apply the same expansion when reading `SBERT_MODEL` from the process environment (so table metadata and search agree with `index_common` defaults). |
229+
| `embedding.model` (when path-shaped) | yes | Path-shape = starts with `/`, `./`, `../`, `~`, or contains `$`; `~` / `$VAR` are expanded, then a result still `./` / `../`-prefixed is resolved to absolute. Plain `org/name` is treated as a hub id and passed through. Relative base (mirrors `index_dir`): a YAML `model` resolves against the config file's directory; `SBERT_MODEL` / `--embedding-model` resolve against `source_root`. Applies after CLI > env > YAML > default precedence. Long-lived MCP hosts also apply the same expansion when reading `SBERT_MODEL` from the process environment (so table metadata and search agree with `index_common` defaults). |
224230
| `embedding.device` | n/a | Device strings (`cpu`, `cuda`, `mps`) aren't paths. |
225231
| `microservice_roots[*]` | no | Each entry is a directory **name** relative to `source_root`, not an arbitrary path. |
226232
| Brownfield `path:` / `topic:` values | no | These are URL paths and Kafka topic names, not filesystem paths. Literal characters preserved. |

java_codebase_rag/config.py

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,36 @@ def cocoindex_subprocess_env_defaults() -> dict[str, str]:
5252
_UNRESOLVED_VAR_RE = re.compile(r"\$(\w+|\{[^}]+\})")
5353

5454

55-
def maybe_expand_embedding_model_path(value: str) -> str:
56-
"""Expand ``~`` and ``$VAR`` when *value* is path-shaped.
55+
def maybe_expand_embedding_model_path(
56+
value: str,
57+
*,
58+
config_dir: Path | None = None,
59+
source_root: Path | None = None,
60+
source: SettingSource | None = None,
61+
) -> str:
62+
"""Expand ``~`` / ``$VAR`` for path-shaped values and resolve relatives to absolute.
5763
5864
Path-shape: starts with ``/``, ``./``, ``../``, ``~``, or contains ``$``.
5965
Plain ``org/name`` (hub id) does not match and is passed through unchanged.
6066
61-
Used for ``embedding.model`` after precedence resolution and for runtime
62-
``SBERT_MODEL`` reads (e.g. MCP) so the string matches ``ResolvedOperatorConfig``.
67+
Relative resolution mirrors :func:`_resolve_index_dir_path` so a committed
68+
config is portable regardless of process CWD:
69+
70+
* YAML values (``source == "yaml"``) resolve against ``config_dir`` (the
71+
directory holding ``.java-codebase-rag.yml``).
72+
* CLI / env values resolve against ``source_root``.
73+
74+
Only a result that still starts with ``./`` or ``../`` *after* ``~`` /
75+
``$VAR`` expansion is re-based — so hub ids (``org/name``), absolute paths,
76+
``~/``-expanded paths, and an env var that already yielded an absolute path
77+
are all left untouched.
78+
79+
When no base is supplied (the runtime ``SBERT_MODEL`` read via
80+
:func:`resolved_sbert_model_for_process_env`), relative resolution is
81+
skipped: the value is returned ``expandvars`` / ``expanduser``-expanded but
82+
not re-based, matching the prior best-effort behavior. The main resolution
83+
path (:func:`resolve_operator_config`) supplies a base, so the absolute path
84+
it stores is what downstream loaders receive.
6385
"""
6486
needs_expand = value.startswith(("/", "./", "../", "~")) or "$" in value
6587
if not needs_expand:
@@ -70,9 +92,31 @@ def maybe_expand_embedding_model_path(value: str) -> str:
7092
f"java-codebase-rag: path-shaped model string contains unresolved variable: {expanded}",
7193
file=sys.stderr,
7294
)
95+
if expanded.startswith(("./", "../")):
96+
base = _embedding_model_base(
97+
source=source, config_dir=config_dir, source_root=source_root
98+
)
99+
if base is not None:
100+
return str((base / expanded).resolve())
73101
return expanded
74102

75103

104+
def _embedding_model_base(
105+
*,
106+
source: SettingSource | None,
107+
config_dir: Path | None,
108+
source_root: Path | None,
109+
) -> Path | None:
110+
"""Base directory for a relative ``embedding.model``.
111+
112+
Mirrors :func:`_resolve_index_dir_path`: YAML values anchor on the config
113+
file's directory; CLI / env values anchor on the resolved ``source_root``.
114+
"""
115+
if source == "yaml":
116+
return config_dir
117+
return source_root
118+
119+
76120
def resolved_sbert_model_for_process_env(import_time_default: str) -> str:
77121
"""``SBERT_MODEL`` from the process environment, with the same expansion as YAML/CLI resolution.
78122
@@ -387,7 +431,12 @@ def resolve_operator_config(
387431
yaml_path=("embedding", "model"),
388432
default=_DEFAULT_EMBEDDING_MODEL,
389433
)
390-
model = maybe_expand_embedding_model_path(model)
434+
model = maybe_expand_embedding_model_path(
435+
model,
436+
config_dir=config_dir,
437+
source_root=root,
438+
source=model_src,
439+
)
391440
device, device_src = _pick_optional_device(
392441
cli_val=cli_embedding_device,
393442
env_key="SBERT_DEVICE",

tests/test_config.py

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,155 @@ def test_existing_behavior_unchanged(self, tmp_path, monkeypatch):
292292
assert result.index_dir == tmp_path / ".java-codebase-rag"
293293

294294

295+
class TestEmbeddingModelRelativePath:
296+
"""``embedding.model`` relative paths resolve against a base directory.
297+
298+
Mirrors ``index_dir`` (see ``TestIndexDirRelativeToConfigDir``): a relative
299+
model path in YAML resolves against the config file's directory; a relative
300+
model path from CLI / env resolves against the resolved ``source_root``.
301+
This makes a committed ``.java-codebase-rag.yml`` portable — the model loads
302+
from the same absolute path for the CLI indexer and the MCP reader, instead
303+
of resolving against an unreliable process CWD.
304+
"""
305+
306+
def test_yaml_relative_model_resolves_against_config_dir(self, tmp_path, monkeypatch):
307+
"""``embedding.model: ./models/minilm`` (YAML) -> <config_dir>/models/minilm."""
308+
monkeypatch.delenv("SBERT_MODEL", raising=False)
309+
monkeypatch.delenv("JAVA_CODEBASE_RAG_SOURCE_ROOT", raising=False)
310+
311+
config_dir = tmp_path / "ctx"
312+
config_dir.mkdir()
313+
(config_dir / YAML_CONFIG_FILENAMES[0]).write_text(
314+
"embedding:\n model: ./models/minilm\n"
315+
)
316+
monkeypatch.chdir(config_dir)
317+
318+
result = resolve_operator_config(source_root=None)
319+
assert result.embedding_model == str((config_dir / "models/minilm").resolve())
320+
assert result.embedding_model_source == "yaml"
321+
322+
def test_yaml_double_dot_model_resolves_against_config_dir(self, tmp_path, monkeypatch):
323+
"""``embedding.model: ../shared/minilm`` (YAML) -> <config_dir>/../shared/minilm."""
324+
monkeypatch.delenv("SBERT_MODEL", raising=False)
325+
monkeypatch.delenv("JAVA_CODEBASE_RAG_SOURCE_ROOT", raising=False)
326+
327+
config_dir = tmp_path / "ctx"
328+
config_dir.mkdir()
329+
(config_dir / YAML_CONFIG_FILENAMES[0]).write_text(
330+
"embedding:\n model: ../shared/minilm\n"
331+
)
332+
monkeypatch.chdir(config_dir)
333+
334+
result = resolve_operator_config(source_root=None)
335+
assert result.embedding_model == str((tmp_path / "shared/minilm").resolve())
336+
337+
def test_env_relative_model_resolves_against_source_root(self, tmp_path, monkeypatch):
338+
"""``SBERT_MODEL=./models/minilm`` (env) -> <source_root>/models/minilm.
339+
340+
Config sets ``source_root: ../`` so source_root (tmp_path) differs from
341+
config_dir (tmp_path/ctx); the env-sourced model must anchor on
342+
source_root, not config_dir — matching ``index_dir``'s env base.
343+
"""
344+
monkeypatch.delenv("JAVA_CODEBASE_RAG_SOURCE_ROOT", raising=False)
345+
346+
config_dir = tmp_path / "ctx"
347+
config_dir.mkdir()
348+
(config_dir / YAML_CONFIG_FILENAMES[0]).write_text("source_root: ../\n")
349+
monkeypatch.chdir(config_dir)
350+
monkeypatch.setenv("SBERT_MODEL", "./models/minilm")
351+
352+
result = resolve_operator_config(source_root=None)
353+
assert result.source_root == tmp_path
354+
assert result.embedding_model == str((tmp_path / "models/minilm").resolve())
355+
assert result.embedding_model_source == "env"
356+
357+
def test_cli_relative_model_resolves_against_source_root(self, tmp_path, monkeypatch):
358+
"""``--embedding-model ./models/minilm`` (CLI) -> <source_root>/models/minilm."""
359+
monkeypatch.delenv("SBERT_MODEL", raising=False)
360+
361+
result = resolve_operator_config(
362+
source_root=tmp_path, cli_embedding_model="./models/minilm"
363+
)
364+
assert result.embedding_model == str((tmp_path / "models/minilm").resolve())
365+
assert result.embedding_model_source == "cli"
366+
367+
368+
class TestMaybeExpandEmbeddingModelPath:
369+
"""Unit tests pinning the expansion/resolution helper's contract."""
370+
371+
def test_no_base_leaves_relative_unchanged(self):
372+
"""Without a base dir, relative paths are NOT resolved.
373+
374+
``resolved_sbert_model_for_process_env`` (the MCP runtime read of
375+
``SBERT_MODEL``) calls this with no base; it must stay a no-op for
376+
relative values so MCP behavior is unchanged there. The main resolution
377+
path supplies a base, so the absolute path it produces is what reaches
378+
the lazy loader in practice.
379+
"""
380+
from java_codebase_rag.config import maybe_expand_embedding_model_path
381+
382+
assert maybe_expand_embedding_model_path("./models/minilm") == "./models/minilm"
383+
assert maybe_expand_embedding_model_path("../shared/minilm") == "../shared/minilm"
384+
385+
def test_hub_id_passthrough(self):
386+
from java_codebase_rag.config import maybe_expand_embedding_model_path
387+
388+
assert maybe_expand_embedding_model_path("org/name") == "org/name"
389+
assert (
390+
maybe_expand_embedding_model_path("sentence-transformers/all-MiniLM-L6-v2")
391+
== "sentence-transformers/all-MiniLM-L6-v2"
392+
)
393+
394+
def test_absolute_passthrough(self):
395+
from java_codebase_rag.config import maybe_expand_embedding_model_path
396+
397+
assert maybe_expand_embedding_model_path("/opt/models/minilm") == "/opt/models/minilm"
398+
399+
def test_env_var_expansion_preserved(self, monkeypatch):
400+
from java_codebase_rag.config import maybe_expand_embedding_model_path
401+
402+
monkeypatch.setenv("MODEL_DIR", "/opt/models")
403+
assert maybe_expand_embedding_model_path("${MODEL_DIR}/minilm") == "/opt/models/minilm"
404+
assert maybe_expand_embedding_model_path("$MODEL_DIR/minilm") == "/opt/models/minilm"
405+
406+
def test_tilde_expansion_preserved(self, monkeypatch):
407+
from java_codebase_rag.config import maybe_expand_embedding_model_path
408+
409+
monkeypatch.setenv("HOME", "/home/user")
410+
assert maybe_expand_embedding_model_path("~/models/minilm") == "/home/user/models/minilm"
411+
412+
def test_yaml_base_resolves_relative(self, tmp_path):
413+
from java_codebase_rag.config import maybe_expand_embedding_model_path
414+
415+
out = maybe_expand_embedding_model_path(
416+
"./models/minilm", config_dir=tmp_path, source="yaml"
417+
)
418+
assert out == str((tmp_path / "models/minilm").resolve())
419+
420+
def test_cli_env_base_is_source_root(self, tmp_path):
421+
from java_codebase_rag.config import maybe_expand_embedding_model_path
422+
423+
for src in ("cli", "env"):
424+
out = maybe_expand_embedding_model_path(
425+
"./models/minilm", source_root=tmp_path, source=src
426+
)
427+
assert out == str((tmp_path / "models/minilm").resolve())
428+
429+
def test_absolute_after_env_var_not_rebased(self, tmp_path, monkeypatch):
430+
"""An env var that already yields an absolute path is left absolute.
431+
432+
Guards the ``${HUB_ID}`` edge: only ``./`` / ``../``-prefixed results are
433+
re-based, so a var holding ``org/name`` or an absolute path is untouched.
434+
"""
435+
from java_codebase_rag.config import maybe_expand_embedding_model_path
436+
437+
monkeypatch.setenv("MODEL_DIR", "/opt/models")
438+
out = maybe_expand_embedding_model_path(
439+
"${MODEL_DIR}/minilm", config_dir=tmp_path, source="yaml"
440+
)
441+
assert out == "/opt/models/minilm"
442+
443+
295444
def test_cocoindex_subprocess_env_defaults_uses_real_inflight_env_var() -> None:
296445
"""The throttle must use CocoIndex's REAL env var name.
297446

0 commit comments

Comments
 (0)