diff --git a/.rules/python-00.md b/.rules/python-00.md new file mode 100644 index 000000000..845de8938 --- /dev/null +++ b/.rules/python-00.md @@ -0,0 +1,133 @@ +# Python 3.14 code style guidelines (with Ruff, Pyright, and pytest) + +## Naming Conventions + +- **Directories:** Use *snake_case* for top-level features or modules (e.g., + `data_pipeline`, `user_auth`). +- **Files:** Use *snake_case.py*; name for contents (e.g., `http_client.py`, + `task_queue.py`). +- **Classes:** Use *PascalCase*. +- **Variables & Functions:** Use *snake_case*. +- **Constants:** Use *UPPER_SNAKE_CASE* for module-level constants. +- **Private/Internal:** Prefix with a single underscore (`_`) for non-exported + helpers or internal APIs. + +## Python Typing Practices + +- **Use typing everywhere.** Enable and maintain full static type coverage. Use + Pyright for type-checking. +- **Use `TypedDict` or `Dataclass` for structured data where appropriate.** For + internal-only usage, prefer `@dataclass(slots=True)`. +- **Avoid `Any`.** Prefer concrete types, `object`, generics, or type variables. + Use a documented `cast()` or `Any` only when the boundary requires it. +- **Be explicit with returns.** Use `-> None`, `-> str`, etc., for all public + functions and class methods. + - **Favour immutability.** Prefer tuples to lists, and `frozendict` or + `types.MappingProxyType` where appropriate. + +## Tooling and Runtime Practices + +- **Enable Ruff.** Use Ruff to lint for performance, security, consistency, and + style issues. Enable fixers and formatters. +- Use `pyproject.toml` to configure tools like Ruff, Pyright, and Pytest. +- **Enforce `strict` in Pyright.** Treat all Pyright warnings as CI errors. Use + `# pyright: ignore` sparingly and with explanation. +- **Avoid side effects at import time.** Modules should not modify global state + or perform actions on import. +- **Use `.env` or settings modules** for environment-specific configuration. + Never hardcode secrets. + +## Linting and Formatting + +- **Use Ruff for linting** (replacing flake8, isort, pyflakes, etc.). +- **Use Ruff for formatting**. Let Ruff handle whitespace and formatting + entirely—don't fight it. + +## Documentation + +- **Use docstrings.** Document public functions, classes, and modules using + NumPy format. For example: + +```python +def scale(values: list[float], factor: float) -> list[float]: + """ + Scale a list of numbers by a given factor. + + Parameters + ---------- + values : list of float + The list of numeric values to scale. + factor : float + The multiplier to apply to each value. + + Returns + ------- + list of float + The scaled numeric values. + """ + return [v * factor for v in values] +``` + +- **Explain tricky code.** Use inline comments for non-obvious logic or + decisions. +- **Colocate documentation.** Keep README.md or `docs/` near reusable packages; + include usage examples. + +## Testing with pytest + +- **Colocate unit tests with code** using an `unittests` subdirectory and a + `test_` prefix. This keeps logic and its tests together: + +```text +user_auth/ + models.py + login_flow.py + unittests/ + test_models.py + test_login_flow.py +``` + +- **Structure integration tests separately.** When tests span multiple + components, use `tests/integration/`: + + ```text + tests/ + integration/ + test_login_flow.py + test_user_onboarding.py + ``` + +- **Use `pytest` idioms.** Prefer fixtures over setup/teardown methods. + Parametrize broadly. Avoid unnecessary mocks. + +- **Group related tests** using `class` with method names prefixed by `test_`. + +- **Write tests from a user's perspective.** Test public behaviour, not + internals. + +- **Avoid mocking too much.** Prefer test doubles only for external services or + non-deterministic behaviours. + +## Example + +```python +# login_flow.py +def login_user(username: str, password: str) -> bool: + """Return True if the user is authenticated.""" + ... + + +# login_flow_test.py +def test_login_success(): + assert login_user("alice", "correct-password") is True + + +def test_login_failure(): + assert not login_user("alice", "wrong-password") +``` + +______________________________________________________________________ + +This style guide aims to foster clean, consistent, and maintainable Python 3.14 +code with modern tooling. The priority is correctness, clarity, and developer +empathy. diff --git a/.rules/python-context-managers.md b/.rules/python-context-managers.md new file mode 100644 index 000000000..9a1e729b6 --- /dev/null +++ b/.rules/python-context-managers.md @@ -0,0 +1,111 @@ +# Using Context Managers for Cleanup and Resource Management + +Use context managers to encapsulate setup and teardown logic cleanly and +safely. This reduces the risk of forgetting to release resources (files, locks, +connections, etc.) and simplifies error handling. + +Context managers can be written either with `contextlib.contextmanager` (for +simple procedural control flow) or by implementing `__enter__` and `__exit__` +in a class (for more complex or stateful use cases). + +## Why Use Context Managers? + +- **Safety:** Ensures cleanup occurs even if an exception is raised. +- **Clarity:** Reduces boilerplate and visually scopes side effects. +- **Reuse:** Common setup/teardown logic becomes reusable and composable. + +______________________________________________________________________ + +## Example: Using `contextlib.contextmanager` + +Use this for straightforward procedural setup/teardown: + +```python +from contextlib import contextmanager + + +@contextmanager +def managed_file(path: str, mode: str): + f = open(path, mode) + try: + yield f + finally: + f.close() + + +# Usage: +with managed_file("/tmp/data.txt", "w") as f: + f.write("hello") +``` + +This avoids repeating `try/finally` in every file access. + +______________________________________________________________________ + +## Example: Using a Class-Based Context Manager + +Use this when state or lifecycle logic spans methods: + +```python +class Resource: + def __enter__(self): + self.conn = connect() + return self.conn + + def __exit__(self, exc_type, exc_val, exc_tb): + self.conn.close() + + +# Usage: +with Resource() as conn: + conn.send("ping") +``` + +This keeps state encapsulated and makes testing easier. + +______________________________________________________________________ + +## When to Use Which + +- Use `@contextmanager` when control flow is linear and no persistent state is + required. + +- Use a class when: + + - There is internal state or methods tied to the resource lifecycle. + - You need to support re-entry or more advanced context features. + +______________________________________________________________________ + +## Common Use Cases + +- File or network resource handling +- Lock acquisition and release +- Temporary resources and isolated environment changes (e.g., `patch`, + `tempfile`) +- Logging scope control or tracing +- Transaction control in databases or services + +______________________________________________________________________ + +## Don't Do This + +```python +f = open("file.txt") +try: + process(f) +finally: + f.close() +``` + +## Do This Instead + +```python +with open("file.txt") as f: + process(f) +``` + +Context managers make intent and error handling explicit. Prefer them over +manual `try/finally` for clearer, safer code. For directory-dependent work, +inject the directory explicitly or run a child process with its working +directory set; do not change the process working directory with `os.chdir`. diff --git a/.rules/python-exception-design-raising-handling-and-logging.md b/.rules/python-exception-design-raising-handling-and-logging.md new file mode 100644 index 000000000..8daa231a7 --- /dev/null +++ b/.rules/python-exception-design-raising-handling-and-logging.md @@ -0,0 +1,308 @@ +# Python exception design, raising, handling, and logging — Ruff TRY/BLE/EM/LOG, N818, PERF203 + +This guide distils the intent behind Ruff’s Tryceratops (TRY), Blind Except +(BLE), flake8‑errmsg (EM), flake8‑logging (LOG), pep8‑naming N818, and Perflint +PERF203, aligned with practical engineering practice. + +## 1) Design a coherent exception hierarchy (N818 + practice) + +**Principle:** model failure semantics with a small tree of domain exceptions; +suffix concrete error classes with `Error` (N818). A single package‑level base +class enables callers to catch all domain failures without vendor leakage. + +```python +class PaymentsError(Exception): + """All payment-layer errors.""" + + +class CardDeclinedError(PaymentsError): # ✅ ends with Error (N818) + def __init__(self, code: str, *, retry_after: int | None = None): + super().__init__(f"Card declined ({code})") + self.code = code + self.retry_after = retry_after +``` + +**Practice notes:** group exceptions under a common base; add structured +attributes (codes, identifiers, retry hints) so that business logic need not +parse free‑form strings. + +## 2) Raise the right thing, with the right cause (TRY003/TRY004/TRY200/TRY201) + +### Prefer specific built‑ins or domain errors over “vanilla” exceptions + +```python +# ❌ Avoid +raise Exception("Bad input") + +# ✅ Prefer +raise ValueError("Percent must be between 0 and 100") +# …or a domain error +raise CardDeclinedError("insufficient_funds") +``` + +TRY003 discourages raising `Exception` directly. TRY004 encourages appropriate +built‑ins (`TypeError` for wrong types, `ValueError` for bad values, etc.) or +domain‑specific classes. + +### Preserve causal chains with `raise … from …` + +```python +try: + token = decode_jwt(payload) +except jwt.InvalidTokenError as exc: + raise AuthenticationError("Invalid session token") from exc # ✅ TRY201 +``` + +When transforming low‑level failures into domain errors, `raise … from …` +retains traceback lineage (TRY201). Avoid discarding causes in contexts +expected to preserve them (TRY200). + +## 3) Catch narrowly; avoid blind handlers (BLE001), and use `else` for the happy path (TRY300) + +### Avoid blind `except` + +```python +# ❌ BLE001: masks defects and unrelated failures +try: + process(row) +except Exception: + pass + +# ✅ Catch only actionable failures +try: + process(row) +except (TimeoutError, RateLimitError) as exc: + backoff_and_retry(exc) +``` + +BLE001 warns on `except:` and `except Exception:`. Handlers should target +exceptions that can be meaningfully handled. + +### Separate success flow with `else` + +```python +def reciprocal(n: float) -> float: + try: + result = 1 / n + except ZeroDivisionError: + log.warning("n was zero") + return float("inf") + else: # ✅ TRY300 + return result +``` + +`else` emphasizes the happy path and avoids odd control‑flow within `try` +blocks. + +## 4) Message construction for raises (EM101/EM102) and logging practice (LOG004/LOG007/LOG009/LOG014/LOG015, TRY401) + +### Exception messages: construct once, pass once + +```python +name = user.name +# ❌ EM102: f-string passed directly into constructor +raise RuntimeError(f"User {name!r} not found") + +# ✅ Build the message, then pass a single object +msg = f"User {name!r} not found" +raise RuntimeError(msg) +``` + +EM101/EM102 prefer a single message object; this reduces duplication and +clarifies intent. + +### Logging: parameterized messages, module loggers, correct APIs + +```python +import logging + +logger = logging.getLogger(__name__) + +# ❌ LOG issues +logging.warning(f"failed for {user_id}") # f-string (LOG004/LOG014) +logging.warning("failed for %s" % user_id) # %-formatting (LOG007) +logging.warn("deprecated") # warn() (LOG009) +logging.error("bad root logger") # root logger usage (LOG015) + +# ✅ Correct +logger.warning("Failed for user_id=%s", user_id) # lazy interpolation +logger.error("Task %s crashed", task_id) +``` + +### Logging exceptions: no duplication + +```python +try: + risky() +except ValueError: + logger.exception("Risky operation failed") # ✅ includes traceback; no %s with exc +``` + +`logger.exception` records the active exception and traceback; appending the +exception object to the format arguments is redundant (TRY401). + +**Operational note:** log once at a boundary (e.g., request or worker entry +point). Inner layers should handle or re‑raise without logging to avoid +duplicate noise. + +## 5) Performance considerations in loops (PERF203) + +```python +# ❌ try/except inside a tight loop +for item in items: + try: + parse(item) + except ParseError: + handle_parse_failure() + continue + +# ✅ preserve per-item handling when processing must continue +for item in items: + try: + parse(item) + except ParseError: + handle_parse_failure() +``` + +Exception handling carries overhead on the exceptional path. If profiling +justifies changing the loop structure, document whether the operation is +allowed to fail fast; hoisting the `try` block changes the handling semantics +when later items must still be processed. + +## 6) Testing: assert specific failures (B017) + +```python +# ❌ Overly broad; test may pass for the wrong reason +with pytest.raises(Exception): + parse("not-json") + +# ✅ Narrow and expressive +with pytest.raises(JSONDecodeError, match=r"Expecting value"): + parse("not-json") +``` + +B017 flags overly broad exception assertions. Tests should specify the expected +type and, when useful, constrain the message via regex. + +## 7) Practical patterns and anti‑patterns + +**Handle vs bubble:** handle locally when the code can correct the condition +(retry, substitute, degrade) or add essential context and re‑raise with `from`. +Otherwise, allow bubbling to a layer capable of policy decisions (transaction +rollback, HTTP 5xx, CLI exit code). + +**No “log and re‑raise” chains:** log exactly once at a suitable boundary. +Intermediate layers should either resolve the problem or propagate it. + +**Built‑ins with intent:** `ValueError` for bad values, `TypeError` for wrong +types, `NotImplementedError` for abstract methods; avoid `RuntimeError` as a +catch‑all where a domain error or specific built‑in communicates intent better. + +## 8) Reference examples (good vs bad) + +### Wrapping vendor errors into domain errors + +```python +def charge(amount_pennies: int, card_token: str) -> str: + try: + return gateway.charge(amount_pennies, card_token) + except gateway.Timeout as exc: + raise PaymentsError("Gateway timeout") from exc # ✅ TRY201 + except gateway.CardDeclined as exc: + raise CardDeclinedError(exc.code, retry_after=60) from exc +``` + +### Boundary logging (single place) + +```python +def worker_main() -> None: + try: + process_job() + except PaymentsError: + logger.exception("Job failed due to payments error") # log once, then propagate + raise +``` + +### Building exception messages (EM) and logging payloads (LOG) + +```python +def must_have_key(d: dict, key: str) -> None: + if key not in d: + msg = f"Missing required key: {key!r}" + raise KeyError(msg) + + +order_id = "ord_123" +shop_id = "shop_456" +logger.info("Dispatching order_id=%s to shop_id=%s", order_id, shop_id) # structured +``` + +### `try/except` in loops (PERF203) and `else` usage (TRY300) + +```python +def parse_all(raw_items: list[str]) -> list[Record]: + parsed: list[Record] = [] + try: # ✅ PERF203 hoist + for raw in raw_items: + rec = parse_record(raw) + parsed.append(rec) + except ParseError: + logger.exception("Parsing aborted") + else: # ✅ TRY300: success-only post-processing + logger.info("Parsed %s records", len(parsed)) + return parsed +``` + +### Tests with specific exceptions (B017) + +```python +def test_amount_must_be_int() -> None: + with pytest.raises(TypeError, match="amount_pennies"): + charge("12.34", "tok_abc") # wrong type triggers TypeError +``` + +## 9) Minimal Ruff configuration to enforce these rules + +```toml +# pyproject.toml +[tool.ruff] +target-version = "py311" + +[tool.ruff.lint] +select = [ + "TRY", # Tryceratops + "BLE", # blind-except + "EM", # flake8-errmsg + "LOG", # flake8-logging + "N818", # exception names end with Error + "PERF203", # try/except in loop + "B017", # assert-raises-exception +] +``` + +## 10) One‑page policy for repositories + +> **Exceptions are part of the public API.** Define a small hierarchy with a +> package base and `*Error` suffix; raise specific types; wrap external +> failures with `raise … from …`; catch only what can be handled; use `else` +> for the happy path; avoid `try/except` in hot loops; never format log +> messages directly; log exceptions once at a boundary via `logger.exception`. +> Enforce with Ruff (TRY/BLE/EM/LOG/N818/PERF203/B017). + +## 11) References + +- Ruff rules: Tryceratops (TRY), Blind Except (BLE001), flake8‑errmsg + (EM101/EM102), flake8‑logging (LOG004/LOG007/LOG009/LOG014/LOG015), N818, + PERF203, B017. + - [https://docs.astral.sh/ruff/rules/#tryceratops-try](https://docs.astral.sh/ruff/rules/#tryceratops-try) + - [https://docs.astral.sh/ruff/rules/blind-except/](https://docs.astral.sh/ruff/rules/blind-except/) + - [https://docs.astral.sh/ruff/rules/assert-raises-exception/](https://docs.astral.sh/ruff/rules/assert-raises-exception/) + - [https://docs.astral.sh/ruff/rules/#flake8-errmsg-em](https://docs.astral.sh/ruff/rules/#flake8-errmsg-em) + - [https://docs.astral.sh/ruff/rules/#flake8-logging-log](https://docs.astral.sh/ruff/rules/#flake8-logging-log) + - [https://docs.astral.sh/ruff/rules/error-suffix-on-exception-name/](https://docs.astral.sh/ruff/rules/error-suffix-on-exception-name/) + - [https://docs.astral.sh/ruff/rules/try-except-in-loop/](https://docs.astral.sh/ruff/rules/try-except-in-loop/) +- Gui Commits practice notes: + - Exception structure: + [https://guicommits.com/how-to-structure-exception-in-python-like-a-pro/](https://guicommits.com/how-to-structure-exception-in-python-like-a-pro/) + - Logging guidance: + [https://guicommits.com/how-to-log-in-python-like-a-pro/](https://guicommits.com/how-to-log-in-python-like-a-pro/) diff --git a/.rules/python-generators.md b/.rules/python-generators.md new file mode 100644 index 000000000..2e802a1a0 --- /dev/null +++ b/.rules/python-generators.md @@ -0,0 +1,94 @@ +# Prefer Generators Over Complex Loop Logic + +Using generators improves readability, composability, and memory efficiency. +Functions built as generators are often simpler to test, debug, and refactor. +This guidance encourages breaking apart complex `for`-loops into generator +expressions or functions using `yield`. + +## Why Prefer Generators? + +- **Clarity:** Isolating data flow from control flow clarifies logic. +- **Efficiency:** Generators are lazy; they avoid building intermediate data + structures unless needed. +- **Composability:** Generators can be pipelined with other iterators using + `itertools` or comprehensions. + +## Example: Filtering and Transforming + +### Complex Loop (harder to read/test) + +```python +def get_names(users): + result = [] + for user in users: + if user.active and user.name: + result.append(user.name.upper()) + return result +``` + +### Generator-Based Version (clearer) + +```python +def iter_user_names(users): + for user in users: + if user.active and user.name: + yield user.name.upper() + + +def get_names(users): + return list(iter_user_names(users)) +``` + +Or with a comprehension: + +```python +def get_names(users): + return [user.name.upper() for user in users if user.active and user.name] +``` + +## Example: Chaining Filters and Mappings + +```python +from itertools import islice + + +def top_active_emails(users): + emails = ( + user.email.lower() for user in users if user.active and user.email is not None + ) + return list(islice(emails, 10)) +``` + +## Use Generators When + +- Code iterates over and filters/maps data. +- Early returns or short-circuit behaviour need to be clearer. +- The function logically produces a sequence over time. + +## Avoid Overcomplicating + +Do not convert everything into generators unnecessarily. Use them to simplify +logic—not obscure it. + +### BAD + +```python +def iter_numbers(): + yield from (x * 2 for x in range(10) if x % 2 == 0) +``` + +### BETTER + +```python +def iter_even_doubles(): + for x in range(10): + if x % 2 == 0: + yield x * 2 +``` + +______________________________________________________________________ + +**Rule of thumb:** When a `for` loop has multiple branches, mutations, or is +hard to explain in one sentence, consider rewriting it as a generator. + +Prefer clear, linear data flows over deeply nested conditionals and loop bodies. diff --git a/.rules/python-pyproject.md b/.rules/python-pyproject.md new file mode 100644 index 000000000..bb160b5fa --- /dev/null +++ b/.rules/python-pyproject.md @@ -0,0 +1,401 @@ +# 1. overview of `uv` and `pyproject.toml` + +Astral's `uv` is a Rust-based project and package manager that uses +`pyproject.toml` as its central configuration file. After a project is +initialized, commands such as `uv init`, `uv sync` or `uv run` cause `uv` to: + +1. Look for a `pyproject.toml` in the project root and keep a lockfile + (`uv.lock`) in sync with it. +2. Create a virtual environment (`.venv`) if one does not already exist. +3. Read dependency specifications (and any build-system directives) to install + or update packages accordingly. (Astral Docs[^1], RidgeRun.ai[^2]) + +In other words, the project's `pyproject.toml` drives everything—from metadata +to dependencies to build instructions—without needing `requirements.txt` or a +separate `setup.py` file. (Level Up Coding[^3], Python Packaging[^4]) + +______________________________________________________________________ + +## 2. The `[project]` table (PEP 621) + +The `[project]` table is defined by PEP 621 and is now the canonical place to +declare metadata (name, version, authors, etc.) and runtime dependencies. At +minimum, PEP 621 requires a statically declared `name`. The `version` may be +declared statically or listed in `dynamic` when supplied by the build backend: + +- `name` +- `version`, or `dynamic = ["version"]` + +However, projects generally benefit from including at least the following +additional fields for clarity and compatibility: + +```toml +[project] +name = "my_project" # Project name (PEP 621 requirement) +version = "0.1.0" # PEP 440-compatible version +description = "A brief overview" # Short summary +readme = "README.md" # Path to the project README (automatically included) +requires-python = ">=3.10" # Restrict Python versions, if needed +license = "MIT" # SPDX licence expression +license-files = ["LICENSE"] +authors = [ + { name = "Alice Example", email = "alice@example.org" } +] +keywords = ["uv", "astral", "example"] # (Optional) for metadata registries +classifiers = [ + "Programming Language :: Python :: 3", + "Operating System :: OS Independent" +] +dependencies = [ + "requests>=2.25", # Runtime dependency + "numpy>=1.23" +] +``` + +- **`name`:** A statically declared project name is mandatory per PEP 621. + `version` is also required unless it is listed in `dynamic` and supplied by + the build backend. (Python Packaging[^4], Reddit[^5]) +- **`description` and `readme`:** Although not mandatory, they help with + indexing and packaging tools; `readme = "README.md"` tells `uv` (and PyPI) to + include the project README as the long description. (Astral Docs[^1], Python + Packaging[^4]) +- **`requires-python`:** Constrains which Python interpreters the package + supports (e.g. `>=3.10`). (Python Packaging[^4], Reddit[^5]) +- **`license` and `license-files`:** Specify an SPDX licence expression such as + `license = "MIT"` and identify distributed licence files with + `license-files = ["LICENSE"]`. (Python Packaging[^4], Reddit[^5]) +- **`authors`:** A list of tables with `name` and `email`. Many registries + (e.g., PyPI) pull this for display. (Python Packaging[^4], Reddit[^5]) +- **`keywords` and `classifiers`:** These help search engines and package + indexes. Classifiers must follow the exact trove list defined by PyPA. + (Python Packaging[^4], Reddit[^5]) +- **`dependencies`:** A list of PEP 508 constraints (e.g., + `"requests>=2.25"`) that `uv` resolves to selected versions recorded in + `uv.lock`. `uv sync` installs the versions resolved in that lockfile rather + than the literal requirement strings. (Astral Docs[^1], RidgeRun.ai[^2]) + +______________________________________________________________________ + +## 3. Runtime vs. development dependencies + +`uv` (via PEP 621 and PEP 735) exposes three dependency fields. Choosing the +right one decides whether a dependency ships to every end user or only ever +exists on a contributor's machine. + +Table 1. Dependency field selection. + +| Field | Installed for | Use it for | +| ------------------------------- | --------------------------------- | ----------------------------------------------- | +| `project.dependencies` | Everyone who installs the package | Libraries the shipped code imports at runtime | +| `project.optional-dependencies` | End users who opt into an *extra* | Optional runtime *features* (`package[extra]`) | +| `dependency-groups` | Local development only | Test, lint, type-check, docs, and other tooling | + +### required runtime dependencies — `project.dependencies` + +Packages the shipped code imports unconditionally. They are published in the +wheel metadata and installed for every consumer. Use PEP 508 specifiers with +bounded ranges, and add them with `uv add `: + +```toml +[project] +dependencies = [ + "httpx>=0.27,<1", +] +``` + +### optional runtime features — `project.optional-dependencies` + +Published "extras" that an *end user* opts into to enable an optional feature +of the package, requested with `package[extra]` syntax (for example, +`pandas[excel]`). Reach for this only when the extra dependency powers +user-facing functionality that not everyone needs — never for development +tooling. Add them with `uv add --optional `: + +```toml +[project.optional-dependencies] +# Opt-in feature: end users request it with my_project[feature]. +feature = [ + "some-runtime-lib>=1.2,<2", +] +``` + +### development-time dependencies — `dependency-groups` + +Tooling only contributors need: test frameworks, linters, type checkers, +documentation builders, and property or mutation testers. These are +**local-only** — PEP 735 dependency groups are *not* included in published +package metadata (they are not part of the wheel), so they must live here +rather than in `project.optional-dependencies`. Add them with +`uv add --dev ` (the `dev` group) or `uv add --group `: + +```toml +[dependency-groups] +dev = [ + "pytest<9.1", + "ruff", + "ty", +] +``` + +**`uv` installs the `dev` group automatically by default.** `uv run` and +`uv sync` include the `dev` group with no extra flags, so a bare `uv sync` +gives a contributor the full toolchain. Adjust this with: + +- `--no-dev` to exclude only the `dev` group. +- `--no-default-groups` to disable configured default groups while still + permitting explicit selection of other groups. +- `--group ` or `--only-group ` to include or isolate a + non-default group. +- `[tool.uv].default-groups` to change which groups sync by default: + +```toml +[tool.uv] +default-groups = ["dev", "docs"] # or "all" +``` + +Groups may nest via `{ include-group = "..." }`, and by default `uv` resolves +every group together into a single `uv.lock`, so groups must be mutually +compatible unless incompatible sets are declared explicitly under +`[tool.uv].conflicts`. (Astral Docs[^6]) + +> **Rule of thumb:** if an end user needs it to *run* the code, it belongs +> in `project.dependencies` (always) or `project.optional-dependencies` +> (an opt-in feature). If only a contributor needs it to *develop, test, +> lint, type-check, or document* the code, it belongs in +> `dependency-groups`. + +______________________________________________________________________ + +## 4. Entry points and scripts + +To expose command-line interfaces (CLIs) or GUIs through a package, PEP 621 +provides the `[project.scripts]` and `[project.gui-scripts]` tables: + +```toml +[project.scripts] +mycli = "my_project.cli:main" + +[project.gui-scripts] +mygui = "my_project.gui:start" +``` + +- **`[project.scripts]`:** Defines console scripts. When `uv run mycli` is run, + `uv` will invoke the `main` function in `my_project/cli.py`. (Astral Docs[^7]) +- **`[project.gui-scripts]`:** On Windows, `uv` will wrap these in a GUI + executable; on Unix-like systems, they behave like normal console scripts. + (Astral Docs[^7]) +- **Plugin Entry Points:** If the project supports plugins, use + `[project.entry-points.'group.name']` to register them. (Astral Docs[^7]) + +______________________________________________________________________ + +## 5. Declaring a build system + +PEP 517/518 strongly recommends a `[build-system]` table to tell tools how to +build and install the project, but it is not universally required. When the +table is present, `uv` uses the declared backend and packages the current +project by default. When the table is omitted, `uv` does not install the +current project unless `tool.uv.package = true`; with that setting, `uv` uses +the legacy setuptools backend to package the project. Dependency resolution and +installation remain `uv` responsibilities and do not come from that legacy +backend. A common setuptools configuration specifies `setuptools>=64.0`, which +supports compatible PEP 660 editable installs without a `setup.py` stub, or +uses a lighter alternative such as `flit_core`. Below is the typical setup +using setuptools: + +```toml +[build-system] +requires = ["setuptools>=64.0"] +build-backend = "setuptools.build_meta" +``` + +- **`requires`:** A list of packages needed at build time. + `setuptools>=64.0` supplies PEP 660 support for compatible editable installs + in `uv`; `wheel` is not required. (Python Packaging[^4], Astral Docs[^7]) + - **`build-backend`:** The entry point for the project's build backend. + `setuptools.build_meta` is the PEP 517-compliant backend for setuptools. + (Python Packaging[^4], Astral Docs[^7]) +- **Note:** When `[build-system]` is omitted, `uv` uses the legacy setuptools + backend to package the current project only when `tool.uv.package = true` + (see next section). Without that setting, the current project is not + installed; `uv` continues to resolve and install dependencies independently. + (Astral Docs[^7]) + +______________________________________________________________________ + +## 6. `uv`-specific configuration (`[tool.uv]`) + +Astral `uv` allows projects to define their own settings in `[tool.uv]`. The +most common option is: + +```toml +[tool.uv] +package = true +``` + +- **`tool.uv.package = true`:** Forces `uv` to build and install the project + into its virtual environment every time `uv sync` or `uv run` is run. When + `[build-system]` is omitted, this setting selects the legacy setuptools + backend for packaging the current project. (Astral Docs[^7]) +- **Additional `uv`-specific keys:** Declare options such as custom indexes and + resolver policies under `[tool.uv]`; `package` is the most common key. + (Python Packaging[^4], Astral Docs[^7]) + +______________________________________________________________________ + +## 7. Putting it all together: example `pyproject.toml` + +Below is a complete example that demonstrates all sections. Adjust values as +needed for the project itself. + +```toml +[project] +name = "my_project" +version = "0.1.0" # PEP 440-compatible version +description = "An illustrative example for Astral uv" +readme = "README.md" +requires-python = ">=3.10" +license = "MIT" +license-files = ["LICENSE"] +authors = [ + { name = "Alice Example", email = "alice@example.org" } +] +keywords = ["astral", "uv", "pyproject", "example"] +classifiers = [ + "Programming Language :: Python :: 3", + "Operating System :: OS Independent" +] +dependencies = [ + "requests>=2.25", + "numpy>=1.23" +] + +# Opt-in runtime feature; end users install it with my_project[fast]. +[project.optional-dependencies] +fast = [ + "orjson>=3.9" +] + +# Development-only tooling (PEP 735); never shipped to end users. +[dependency-groups] +dev = [ + "pytest>=7.0", + "ruff", + "mypy>=1.0" +] +docs = [ + "sphinx>=5.0", + "sphinx-rtd-theme" +] + +[project.scripts] +mycli = "my_project.cli:main" + +[build-system] +requires = ["setuptools>=64.0"] +build-backend = "setuptools.build_meta" + +[tool.uv] +package = true +``` + +**Explanation of key points:** + +1. **Metadata under `[project]`:** + + - `name` (mandatory), and `version` unless supplied through `dynamic` (Python + Packaging[^4], Reddit[^5]) + - `description`, `readme`, `requires-python`: provide clarity about the + project and help tools like PyPI. (Python Packaging[^4], Reddit[^5]) + - `license`, `authors`, `keywords`, `classifiers`: standardized metadata, + which improves discoverability. (Python Packaging[^4], Reddit[^5]) + - `dependencies`: runtime requirements, expressed in PEP 508 syntax. + (Astral Docs[^1], RidgeRun.ai[^2]) + +2. **Optional features vs. development tooling:** + + - `[project.optional-dependencies]` declares an opt-in runtime *extra* + (`fast`), installed by end users via `my_project[fast]`. (Python + Packaging[^4]) + - `[dependency-groups]` (PEP 735) holds development-only tooling (`dev`, + `docs`) that is never published; `uv sync` installs the `dev` group by + default. (Astral Docs[^6]) + +3. **Entry Points (`[project.scripts]`):** + + - Defines a console command `mycli` that maps to `my_project/cli.py:main`. + Invoking `uv run mycli` will run the `main()` function. (Astral Docs[^7]) + +4. **Build System:** + + - `setuptools>=64.0` provides PEP 660 editable-install support for + compatible project layouts. ✱ Newer versions of setuptools support PEP 660 + editable installs without a `setup.py` stub. (Python Packaging[^4], Astral + Docs[^7]) + - `build-backend = "setuptools.build_meta"` tells `uv` how to compile the + package. (Python Packaging[^4], Astral Docs[^7]) + +5. **`[tool.uv]`:** + + - `package = true` ensures that `uv sync` will build and install the project + (in editable mode) every time dependencies change. Otherwise, `uv` treats + the project as a collection of scripts only (no package). (Astral Docs[^7]) + +______________________________________________________________________ + +## 8. Additional tips & best practices + +1. **Keep `pyproject.toml` Human-Readable:** Edit it by hand when possible. + Modern editors (VS Code, PyCharm) offer TOML syntax highlighting and PEP 621 + autocompletion. (Python Packaging[^4]) + +2. **Lockfile Discipline:** After modifying `dependencies` or any `[project]` + fields, always run `uv sync` (or `uv lock`) to update `uv.lock`. This + guarantees reproducible environments. (Astral Docs[^1]) + +3. **Versioning:** Use PEP 440-compatible version values for `version`. If the + version is generated by the build backend, list it in `dynamic` instead. + (Python Packaging[^4]) + +4. **Keep build constraints minimal:** If the project does not need editable + installs, `[build-system]` may be omitted; `uv` will not install the current + project unless `tool.uv.package = true`, which enables the legacy setuptools + backend for packaging. (Astral Docs[^7]) + +5. **Use Exact or Bounded Ranges for Dependencies:** Rather than `requests`, use + `requests>=2.25, <3.0` to avoid unexpected major bumps. (DevsJC[^8]) + +6. **Consider dynamic fields sparingly:** Declare fields such as + `dynamic = ["version"]` if the version is computed at build time (e.g. via + `setuptools_scm`). When doing so, ensure the build backend supports dynamic + metadata. (Python Packaging[^4]) + +______________________________________________________________________ + +## 9. Summary + +A "modern" `pyproject.toml` for an Astral `uv` project should: + +- Use the PEP 621 `[project]` table for metadata and runtime `dependencies`. +- Declare opt-in runtime *features* as extras under + `[project.optional-dependencies]`, and development-only tooling under + `[dependency-groups]` (the `dev` group installs by default). +- Define any CLI or GUI entry points under `[project.scripts]` or + `[project.gui-scripts]`. +- Declare a PEP 517 `[build-system]` (e.g. `setuptools>=64.0` with + `setuptools.build_meta`) to support compatible PEP 660 editable installs, or + omit it and rely on `tool.uv.package = true`. +- Include a `[tool.uv]` section, at minimum `package = true` to have `uv` build + and install the project package. + +Following these conventions ensures that the project is fully PEP-compliant, +easy to maintain, and integrates seamlessly with Astral `uv`. + +[^1]: [Working on projects | uv - Astral Docs](https://docs.astral.sh/uv/guides/projects/) +[^2]: [UV Tutorial: A Fast Python Package and Project Manager](https://www.ridgerun.ai/post/uv-tutorial-a-fast-python-package-and-project-manager) +[^3]: [Modern Python Development with pyproject.toml and UV](https://levelup.gitconnected.com/modern-python-development-with-pyproject-toml-and-uv-405dfb8b6ec8) +[^4]: [Writing your pyproject.toml – Python Packaging User Guide](https://packaging.python.org/en/latest/guides/writing-pyproject-toml/) +[^5]: [Anyone used UV package manager in production? (Reddit)](https://www.reddit.com/r/Python/comments/1ixryec/anyone_used_uv_package_manager_in_production/) +[^6]: [Managing dependencies | uv - Astral Docs](https://docs.astral.sh/uv/concepts/projects/dependencies/) +[^7]: [Configuring projects | uv - Astral Docs](https://docs.astral.sh/uv/concepts/projects/config/) +[^8]: [The Complete Guide to pyproject.toml – devsjc blogs](https://devsjc.github.io/blog/20240627-the-complete-guide-to-pyproject-toml/) diff --git a/.rules/python-return.md b/.rules/python-return.md new file mode 100644 index 000000000..5cb7a2856 --- /dev/null +++ b/.rules/python-return.md @@ -0,0 +1,137 @@ +# flake8-return style guide (Python 3.14) + +The `flake8-return` rules ensure consistent and explicit return behaviour, +ensuring functions are clear in intent and free from unnecessary control flow. +Follow these rules: + +## R501 — Avoid explicit `return None` if it's the only return + +```python +# BAD: +def func(): + return None + + +# GOOD: +def func(): + return +``` + +Use `return` alone instead of `return None` when the function's only result is +`None`. + +______________________________________________________________________ + +## R502 — Avoid implicit `None` in functions that may return a value + +```python +# BAD: +def func(x): + if x > 0: + return x + # implicitly returns None (bad) + + +# GOOD: +def func(x): + if x > 0: + return x + return 0 +``` + +Ensure all branches explicitly return a value if any branch does. + +______________________________________________________________________ + +## R503 — Add an explicit return at the end when a function may return a value + +```python +# BAD: +def func(x): + if x > 0: + return x + # missing terminal return (bad) + + +# GOOD: +def func(x): + if x > 0: + return x + return -1 +``` + +Don't rely on implicit `None` if the function may return a value +elsewhere—always return something at the end. + +Functions whose only possible result is `None` do not need a final bare +`return`: + +```python +# GOOD: +def func(): + do_something() + # implicit None is fine here +``` + +______________________________________________________________________ + +## R504 — Avoid redundant variable assignment before `return` + +```python +# BAD: +def func(): + result = compute() + return result + + +# GOOD: +def func(): + return compute() +``` + +Inline return expressions unless the variable is reused meaningfully before +returning. + +______________________________________________________________________ + +## R505–R508 — Eliminate unnecessary `else` after terminal statements + +Avoid `else` after `return`, `raise`, `break`, or `continue`. These statements +already exit control flow. + +```python +# BAD: +if cond: + return x +else: + return y + +# GOOD: +if cond: + return x +return y +``` + +This applies similarly for `raise`, `break`, and `continue`. + +```python +# BAD: +for x in xs: + if x > 0: + break + else: + log() + +# GOOD: +for x in xs: + if x > 0: + break + log() +``` + +These rules apply to regular and `async def` functions alike. + +______________________________________________________________________ + +Use the `flake8-return` rules to enforce predictable and clean return logic, +enhancing readability and correctness. diff --git a/.rules/python-typing.md b/.rules/python-typing.md new file mode 100644 index 000000000..fb2c217d0 --- /dev/null +++ b/.rules/python-typing.md @@ -0,0 +1,221 @@ +# Advanced typing and language features (Python 3.14) + +> This section documents forward-looking Python 3.14 typing features and best +> practices to improve clarity, correctness, and tooling support. Use these +> features to write expressive, modern Python. + +## `enum.Enum`, `enum.IntEnum`, `enum.StrEnum` + +Use `Enum` for fixed sets of related constants. Use `enum.auto()` to avoid +repeating values manually. Use `IntEnum` or `StrEnum` when interoperability +with integers or strings is required (e.g. for database or JSON serialization). + +```python +import enum + + +class Status(enum.Enum): + PENDING = enum.auto() + COMPLETE = enum.auto() + + +class ErrorCode(enum.IntEnum): + OK = 0 + NOT_FOUND = 404 + + +class Role(enum.StrEnum): + ADMIN = enum.auto() + GUEST = enum.auto() +``` + +Use `auto()` when exact values are unimportant and you want to avoid +duplication. Avoid `auto()` in `IntEnum` where numeric meaning matters. + +## `match` / `case` (structural pattern matching) + +Use structural pattern matching for branching over structured data. This is +especially useful for enums, discriminated unions, or pattern-rich data +structures. + +```python +def handle_status(status: Status) -> str: + match status: + case Status.PENDING: + return "Still processing" + case Status.COMPLETE: + return "Done" +``` + +## Generic class declarations (PEP 695) + +Use bracketed class-level type variables directly for generic class +declarations. + +```python +class Box[T]: + def __init__(self, value: T): + self.value = value +``` + +This is cleaner and avoids the indirection of separate `TypeVar` declarations. + +## `Self` type (PEP 673) + +Use `Self` in fluent interfaces and builder-style APIs to indicate the method +returns the same instance. + +```python +import typing + + +class Builder: + def __init__(self) -> None: + self.values: list[int] = [] + + def add(self, value: int) -> typing.Self: + self.values.append(value) + return self +``` + +This improves tool support and enforces correct chaining semantics. + +## `@override` decorator (PEP 698) + +Use `@override` to indicate that a method overrides one from a superclass. This +enables static analysis tools to detect typos and signature mismatches. + +```python +import typing + + +class Base: + def run(self) -> None: ... + + +class Child(Base): + @typing.override + def run(self) -> None: + print("Running") +``` + +This decorator is a no-op at runtime but improves tooling correctness. + +## `TypeGuard` (PEP 647) + +Use `TypeGuard[T]` to define custom runtime type guards that narrow types in +type checkers. + +```python +import typing + + +def is_str_list(val: list[object]) -> typing.TypeGuard[list[str]]: + return all(isinstance(x, str) for x in val) +``` + +Unlike `isinstance`, this informs the type checker that `val` is now +`list[str]`. + +## Defaults for type variables (PEP 696) + +Allow generic classes/functions to fall back to default types when no specific +type is provided. + +```python +class Box[T = int]: + def __init__(self, value: T | None = None): + # Do not construct an arbitrary T; retain the missing-value state. + self.value: T | None = value +``` + +The default type makes `Box()` equivalent to `Box[int]()` for type checking, +but a generic implementation must not assume that every possible `T` can be +constructed as an `int`. Keeping the fallback as `None` preserves type safety. + +## Standard library generics (PEP 585) + +Use built-in generics from the standard library (`list`, `dict`, `tuple`, etc.) +instead of `typing.List`, `typing.Dict`, etc. + +```python +names: list[str] = ["Alice", "Bob"] +``` + +This reduces imports and reflects the modern style. + +## Union syntax and optional (PEP 604) + +Use `|` to write union types, and `A | None` instead of `Optional[A]`. + +```python +value: int | None = None +``` + +This is more concise and readable, especially for nested types. + +## Type aliases using `TypeAlias` + +Use an annotated `TypeAlias` declaration for named aliases. + +```python +StrDict: typing.TypeAlias = dict[str, str] +``` + +This declares `TypeAlias` in the annotation rather than assigning it as a +value, while preserving the `dict[str, str]` type. + +When compatibility with Python < 3.12 is required, keep the older +`typing.TypeAlias` syntax and add `# noqa: UP040` so `ruff` does not flag it. +Place alias definitions after the import block and group shared aliases in +`bournemouth.types` to avoid duplication. + +## `from __future__ import annotations` + +Python 3.14 defers annotation evaluation by default, so this import is no +longer required in project modules. + +```python +from __future__ import annotations +``` + +For this repository, do not add `from __future__ import annotations` in new or +modified files. The project baseline is `>=3.14`. + +Repository exception: existing pytest-bdd step modules under +`tests/steps/test_*_steps.py` deliberately keep this import because step +discovery inspects annotations at runtime and deferred evaluation would cause +`NameError` for types imported only under `TYPE_CHECKING` blocks, so +`from __future__ import annotations` must be retained in those step modules. + +Use this import only in external or legacy code that must remain compatible +with Python versions earlier than 3.14. + +## `if typing.TYPE_CHECKING` + +Use this conditional to guard imports required only for static typing. + +```python +import typing + +if typing.TYPE_CHECKING: + from mypackage.internal import InternalType +``` + +This avoids runtime import costs or circular imports. + +## Standard aliases + +Use the following import aliases consistently: + +```python +import datetime as dt +import collections.abc as cabc +``` + +This simplifies common types such as `dt.datetime`, `cabc.Iterable`, +`cabc.Callable`, and helps disambiguate usage. + +______________________________________________________________________ + +These conventions promote clarity, tool compatibility, and future-ready Python. diff --git a/Makefile b/Makefile index ec06ebc55..c00491f27 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help all clean test test-nextest doctest test-workflow-contracts test-markdown-format test-typos-config build release lint lint-clippy lint-whitaker lint-python doc-coverage doc-coverage-test fmt check-fmt typecheck typecheck-python markdownlint spelling spelling-config spelling-helper-test nixie install-kani kani-check kani-full kani-ir install-verus verus formal-pr install-dev-fast dev-fast-check dev-build dev-test bench-build bench-config-load +.PHONY: help all clean test test-nextest doctest test-workflow-contracts test-markdown-format test-typos-config build release lint lint-clippy lint-whitaker lint-python doc-coverage doc-coverage-test fmt check-fmt typecheck typecheck-python markdownlint spelling spelling-config spelling-helper-test nixie install-kani kani-check kani-full kani-ir install-verus verus formal-pr install-dev-fast dev-fast-check dev-build dev-test bench-build bench-config-load bench-glob-expansion RUST_TOOLCHAIN_FILE ?= rust-toolchain.toml # Export this path before shell probes expand it, so Make does not interpolate @@ -95,7 +95,7 @@ DF12_PYTHON_LINTS_REF ?= v0.3.0 DF12_PYTHON_LINTS = git+https://github.com/leynos/df12-python-lints.git@$(DF12_PYTHON_LINTS_REF) DF12_PYLINT_MESSAGES = R9101,C9102,R9103,R9104,C9105,C9106,C9107,R9108,R9109,R9110,R9111,R9112,C9112 DF12_PYLINT = $(UV_ENV) $(UV) tool run --python $(PYTHON_BASELINE) \ - --from '$(DF12_PYTHON_LINTS)' pylint \ + --from 'pylint' --with '$(DF12_PYTHON_LINTS)' pylint \ --disable=all --load-plugins=df12_python_lints \ --enable=$(DF12_PYLINT_MESSAGES) AMBRLEAKS = $(UV_ENV) $(UV) tool run --python $(PYTHON_BASELINE) \ @@ -123,7 +123,6 @@ MD_FILES_FIND = find . -type f -name '*.md' \ PROVER_TOOLS_SOURCE ?= git+https://github.com/leynos/rust-prover-tools@b07ef696f8373d54ae68e517d39d47a5d27a5bd5 PROVER_TOOLS ?= uv tool run --from $(PROVER_TOOLS_SOURCE) prover-tools RUSTDOC_FLAGS ?= --cfg docsrs -D warnings -export RUSTDOC_FLAGS VERUS_FLAGS ?= VERUS_INSTALL_FLAGS ?= WHITAKER ?= whitaker @@ -204,7 +203,10 @@ fmt: ## Format Rust, Python, and Markdown sources check-fmt: ## Verify formatting $(CARGO) fmt --all -- --check $(RUFF) format --check $(PYTHON_SOURCES) - @$(MD_FILES_FIND) | xargs -0 -r scripts/check-markdown-format.sh + @$(MD_FILES_FIND) | xargs -0 sh -c '\ + if [ "$$#" -gt 0 ]; then \ + scripts/check-markdown-format.sh "$$@"; \ + fi' sh typecheck: typecheck-python ## Typecheck all targets and features RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings" $(CARGO) check --all-targets --all-features $(BUILD_JOBS) @@ -220,7 +222,7 @@ typecheck-python: ## Typecheck the Python sources with ty --extra-search-path scripts $(PYTHON_SOURCES) markdownlint: spelling ## Lint Markdown and enforce en-GB-oxendict spelling - $(MDLINT) "**/*.md" + @unset FORCE_COLOR; $(MDLINT) "**/*.md" spelling: spelling-config ## Enforce en-GB-oxendict spelling in Markdown prose @PYTHONPATH=scripts $(UV_ENV) $(UV) run --no-project --python $(PYTHON_BASELINE) scripts/typos_rollout_check.py --repository . @@ -228,7 +230,7 @@ spelling: spelling-config ## Enforce en-GB-oxendict spelling in Markdown prose $(UV) tool run typos@$(TYPOS_VERSION) --config typos.toml --force-exclude spelling-config: spelling-helper-test ## Generate and validate the spelling configuration - @$(UV_ENV) $(UV) run --no-project scripts/generate_typos_config.py + @$(UV_ENV) $(UV) run --no-project --python $(PYTHON_BASELINE) scripts/generate_typos_config.py @git ls-files --error-unmatch typos.toml >/dev/null @git diff --exit-code -- typos.toml @@ -299,6 +301,9 @@ bench-build: dev-fast-check ## Time clean and incremental debug builds for both bench-config-load: ## Benchmark cached configuration loading without layer copies $(CARGO) bench --bench config_load_cached_merge +bench-glob-expansion: ## Benchmark manifest glob expansion with an injected base + $(CARGO) bench --bench glob_expansion + help: ## Show available targets @grep -E '^[a-zA-Z_-]+:.*?##' $(MAKEFILE_LIST) | \ awk 'BEGIN {FS=":"; printf "Available targets:\n"} {printf " %-20s %s\n", $$1, $$2}' diff --git a/benches/glob_expansion.rs b/benches/glob_expansion.rs new file mode 100644 index 000000000..110ccf45e --- /dev/null +++ b/benches/glob_expansion.rs @@ -0,0 +1,81 @@ +//! Benchmark base-anchored manifest glob expansion. +//! +//! The fixture is created before timing begins. The two benches compare the +//! injected-base form used by manifest parsing with an equivalent absolute +//! pattern, retaining each result through [`test::black_box`]. + +#![feature(test)] + +extern crate test; + +use anyhow::{Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; +use netsuke::manifest::glob_paths; +use tempfile::TempDir; +use test::{Bencher, black_box}; +use test_support::fs as test_fs; + +/// Number of directories in the deterministic benchmark tree. +const DIRECTORY_COUNT: usize = 128; +/// Number of text files in each benchmark directory. +const FILES_PER_DIRECTORY: usize = 32; + +/// Retain the temporary tree and both pattern forms needed by the benchmarks. +struct GlobFixture { + /// Keep the fixture alive for each timed iteration. + _temporary_directory: TempDir, + /// Canonical UTF-8 directory supplied to the manifest-oriented call. + base: Utf8PathBuf, + /// Equivalent absolute pattern used as an unbased comparison. + absolute_pattern: String, +} + +/// Build a deterministic nested fixture tree outside the timed loop. +fn benchmark_fixture() -> Result { + let temporary_directory = tempfile::tempdir().context("create benchmark directory")?; + let base_directory = temporary_directory.path().join("workspace"); + test_fs::create_dir(&base_directory).context("create benchmark workspace")?; + for directory_index in 0..DIRECTORY_COUNT { + let directory = base_directory.join(format!("directory-{directory_index:03}")); + test_fs::create_dir(&directory) + .with_context(|| format!("create {}", directory.display()))?; + for file_index in 0..FILES_PER_DIRECTORY { + let file = directory.join(format!("file-{file_index:03}.txt")); + test_fs::write(&file, "fixture") + .with_context(|| format!("write {}", file.display()))?; + } + } + let base = Utf8Path::from_path(&base_directory) + .context("benchmark paths must be UTF-8")? + .to_path_buf(); + Ok(GlobFixture { + absolute_pattern: base.join("**/*.txt").to_string(), + _temporary_directory: temporary_directory, + base, + }) +} + +/// Benchmark the manifest-oriented relative pattern anchored to an injected base. +#[bench] +fn expands_relative_pattern_under_injected_base(bencher: &mut Bencher) { + let fixture = benchmark_fixture().unwrap_or_else(|error| panic!("build fixture: {error}")); + bencher.iter(|| { + let paths = glob_paths( + "**/*.txt", + Some(Utf8Path::new(black_box(fixture.base.as_str()))), + ) + .unwrap_or_else(|error| panic!("expand base-anchored glob: {error}")); + black_box(paths); + }); +} + +/// Benchmark the equivalent absolute pattern without an injected base. +#[bench] +fn expands_equivalent_absolute_pattern(bencher: &mut Bencher) { + let fixture = benchmark_fixture().unwrap_or_else(|error| panic!("build fixture: {error}")); + bencher.iter(|| { + let paths = glob_paths(black_box(&fixture.absolute_pattern), None) + .unwrap_or_else(|error| panic!("expand absolute glob: {error}")); + black_box(paths); + }); +} diff --git a/clippy.toml b/clippy.toml index 71afa5ad1..9112497b8 100644 --- a/clippy.toml +++ b/clippy.toml @@ -19,4 +19,5 @@ disallowed-methods = [ { path = "std::env::vars_os", reason = "inject an environment reader" }, { path = "std::env::set_var", reason = "use a stub environment in tests" }, { path = "std::env::remove_var", reason = "use a stub environment in tests" }, + { path = "std::env::set_current_dir", reason = "inject a base-directory seam; confine CWD changes to Command::current_dir" }, ] diff --git a/docs/adr-004-explicit-config-selection-outside-orthoconfig.md b/docs/adr-004-explicit-config-selection-outside-orthoconfig.md index 7f9ffdf40..5035c298b 100644 --- a/docs/adr-004-explicit-config-selection-outside-orthoconfig.md +++ b/docs/adr-004-explicit-config-selection-outside-orthoconfig.md @@ -114,6 +114,19 @@ Netsuke resolves explicit configuration paths in `src/cli/discovery.rs`. - Future changes to selector precedence must update `discovery.rs`, the developer guide, the design document, and this ADR together. +## Addendum — 2026-08-30 + +The original decision above remains unchanged: explicit configuration selection +belongs to the Netsuke CLI adapter rather than OrthoConfig. The current +selector contract is explicit about precedence and directory handling: +`--config` takes precedence over `NETSUKE_CONFIG`, and either explicit selector +bypasses automatic discovery. Relative selectors retain process-working- +directory semantics independently of `-C/--directory`; absolute selectors +remain unchanged. The `-C/--directory` value anchors automatic project +discovery and manifest lookup only. The production implementation is +`selector::resolve_config_selector` in `src/cli/discovery_selector.rs`, with +`src/cli/discovery.rs` owning the layer-loading boundary. + ## Related documents - [`docs/developers-guide.md`](developers-guide.md) diff --git a/docs/adr-008-environment-seam-taxonomy.md b/docs/adr-008-environment-seam-taxonomy.md index f1303d09e..b72f0cd32 100644 --- a/docs/adr-008-environment-seam-taxonomy.md +++ b/docs/adr-008-environment-seam-taxonomy.md @@ -198,6 +198,15 @@ resolution entirely rather than setting the variable for a child to read. ## Addendum +### 2026-08-30: Manifest glob base seam + +Manifest parsing owns a separate base-directory seam: it passes the manifest +directory or workspace root to `glob_paths(pattern, base)` and internal +`expand_glob(pattern, base)`. Relative glob patterns, including parent-relative +ones, resolve from that injected root and retain their pattern-relative result +spelling; absolute patterns remain absolute. This path neither reads nor +mutates process-global working-directory state during expansion. + ### 2026-08-26: EnvLock retirement `EnvLock` is retired rather than hardened. Production signatures must inject diff --git a/docs/adr-014-base-directory-seam-and-dir-anchoring.md b/docs/adr-014-base-directory-seam-and-dir-anchoring.md new file mode 100644 index 000000000..c58f8ea30 --- /dev/null +++ b/docs/adr-014-base-directory-seam-and-dir-anchoring.md @@ -0,0 +1,100 @@ +# Architecture decision record (ADR): base-directory seam and `-C` anchoring + +## Status + +Accepted. + +## Date + +2026-08-27 + +## Context and problem statement + +Manifest resolution and glob expansion previously read the process working +directory (`std::env::current_dir`) deep inside the library, and tests mutated +that working directory (`CwdGuard`) or coordinated process-global environment +state (`EnvLock`) to influence resolution. Under the AGENTS.md environment +mandate (see ADR-008) that ambient coupling is unacceptable: parallel test +execution cannot isolate a mutable process CWD, and a library's correct result +should not depend on where the invoking process happens to sit. + +Separately, the CLI contract for `-C/--directory` needed a precise statement: +the flag anchors automatic project discovery and manifest lookup, but an +explicit `--config` (or `NETSUKE_CONFIG`) selector is resolved against the +shell's original working directory and is deliberately independent of `-C`. + +## Decision + +- **Capture the working directory at the composition boundary, once, as data.** + The command-line entry points read `std::env::current_dir()` and pass the + value onward as an explicit base directory; manifest workspace resolution and + glob expansion accept that base as a parameter and never read the process CWD + themselves. `expand_glob`/`glob_paths` thread the base through to + `strip_base`, which removes it from matches to restore pattern-relative + spellings. +- **An ambient fallback exists only where no manifest root is available**, and + that read is confined to the composition boundary, not to resolution + internals. +- **Explicit selectors are independent of `-C`.** A relative `--config ` + or `NETSUKE_CONFIG` resolves against the shell's original working directory. + `-C` scopes automatic project discovery and manifest lookup; it never + re-anchors an explicit selector. See ADR-004 for the selection machinery and + `src/cli/discovery.rs` for the implementation. +- **In-process environment mutation is banned and gated.** `clippy.toml` and + `test_support/clippy.toml` disallow `std::env::set_var`, `remove_var`, and + `set_current_dir` across the workspace targets, and `make lint` runs Clippy + with those restrictions. `Command::env`/`Command::env_clear`/ + `Command::current_dir` — the child-process configuration builders — remain + the sanctioned route and are deliberately not disallowed. + +## Consequences + +- Manifest and glob resolution is deterministic: results depend on the injected + base, not on where the test or process was launched. +- Tests no longer need `EnvLock`/`CwdGuard`; `test_support/src/env_lock.rs` and + `cwd_guard.rs` were deleted, and suites pass explicit base directories. +- A contributor who reintroduces in-process mutation immediately fails the + Clippy stage of `make lint` (disallowed-methods) with a reason string telling + them what to do instead. +- Explicit `--config` behaviour is documented identically in the user guide, + the design document, and this ADR, fixing a stale passage that claimed + `-C`-anchoring. + +## Alternatives considered + +- **Threading a CWD value through every query.** Rejected: ADR-008's taxonomy + prefers capture-once-at-the-boundary over parameter threading everywhere. +- **Allowing sanctioned sites for `set_current_dir` in tests.** Rejected: that + would recreate the very coupling the seam removes. + +## Implementation references + +- Base seam: [`src/manifest/glob/mod.rs`](../src/manifest/glob/mod.rs) + (`expand_glob`, `glob_paths`, `strip_base`) and + [`src/manifest/workspace.rs`](../src/manifest/workspace.rs) + (`resolve_absolute_workspace_root`). +- Composition boundary: `src/runner/mod.rs` and `src/runner/help_query.rs`. +- Explicit-selector independence: + [`src/cli/discovery.rs`](../src/cli/discovery.rs); ADR-004. +- Gate: `make lint` runs Clippy with `clippy.toml` and + `test_support/clippy.toml`, which contain the disallowed-method policy. + +## Addendum — 2026-08-30 + +The accepted decision above remains the historical rationale for the seam. Its +current implementation has these clarified contracts: + +- Manifest parsing supplies the manifest directory or workspace root to + `glob_paths(pattern, base)` and `expand_glob(pattern, base)`. Relative + patterns, including parent-relative patterns, resolve from that injected root + and retain their pattern-relative result spelling; absolute patterns remain + absolute. +- Explicit `--config` and `NETSUKE_CONFIG` selectors remain independent of + `-C/--directory`: relative selectors resolve from the process working + directory and absolute selectors remain unchanged. `-C` anchors automatic + project discovery and manifest lookup. +- The environment-mutation enforcement is Clippy-only. `clippy.toml` and + `test_support/clippy.toml` reject the forbidden process-global mutation + methods across workspace targets; child-process configuration through + `Command::env`, `Command::env_clear`, and `Command::current_dir` remains + allowed. diff --git a/docs/contents.md b/docs/contents.md index 879d87dec..06c131482 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -75,12 +75,16 @@ operator, user, and contributor references are easier to find. - [ADR-013](adr-013-application-owned-configuration-observability.md): Application-owned configuration-load metrics, verbose snapshots, and bounded label vocabulary. -- [ADR-014](adr-014-backend-text-escaping-seam.md): +- [ADR-014: backend text escaping](adr-014-backend-text-escaping-seam.md): Ninja backend escaping boundary decision record, preserving ordinary shell dollar syntax in manifests without coupling the IR to Ninja. - [ADR-015](adr-015-use-bounded-git-cli-for-change-detection.md): Feature-private, bounded Git CLI queries for standard-library change detection. +- [ADR-014: base-directory seam]( + adr-014-base-directory-seam-and-dir-anchoring.md): Base-directory seam for + manifest and glob resolution, explicit-selector independence from `-C`, and + the in-process environment-mutation gate. ## Proposals @@ -128,6 +132,9 @@ operator, user, and contributor references are easier to find. evolution log, and principled refusals. - [documentation-style-guide.md](documentation-style-guide.md): Documentation conventions, roadmap-writing rules, and Markdown requirements. +- [scripting-standards.md](scripting-standards.md): Python scripting standards + for repository automation scripts, covering the Cyclopts CLI pattern, + `cuprum` command execution, `pathlib` usage, and pytest coverage rules. - [execplans/](execplans/): Execution plans and implementation handoff notes. ## Testing and quality references diff --git a/docs/developers-guide.md b/docs/developers-guide.md index ad69c1bb5..0732ca49c 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -649,51 +649,27 @@ workflow-specific projections and assertions, so parsing and structural validation remain consistent across the workflows under test. `make test` runs the non-doctest suite through -[cargo-nextest](https://nexte.st/) and then runs the doctests separately. CI -pins the runner version in `NEXTEST_VERSION` in `.github/workflows/ci.yml`. -Install that same version locally so local runs match CI; read the pin from the -workflow rather than copying the number, so the two cannot drift: +[cargo-nextest](https://nexte.st/) and the doctests separately. CI pins the +runner version in `NEXTEST_VERSION` in `.github/workflows/ci.yml`. Install that +same version locally so local runs match CI; read the pin from the workflow +rather than copying the number, so the two cannot drift: ```bash NEXTEST_VERSION="$(sed -n "s/.*NEXTEST_VERSION: '\(.*\)'.*/\1/p" \ .github/workflows/ci.yml)" cargo install cargo-nextest --locked --version "$NEXTEST_VERSION" - # or, for a prebuilt binary: cargo binstall --no-confirm --locked \ "cargo-nextest@$NEXTEST_VERSION" ``` -`make check-fmt` verifies Markdown formatting as well as Rust formatting, and -needs `mdtablefix` on `PATH`. CI pins the version in `MDTABLEFIX_VERSION` in -`.github/workflows/ci.yml`. Install that same version locally so local runs -match CI; read the pin from the workflow rather than copying the number, so the -two cannot drift: - -```bash -MDTABLEFIX_VERSION="$(sed -n "s/.*MDTABLEFIX_VERSION: '\(.*\)'.*/\1/p" \ - .github/workflows/ci.yml)" -cargo install --locked mdtablefix --version "$MDTABLEFIX_VERSION" -# or, for a prebuilt binary: -cargo binstall --no-confirm --locked \ - "mdtablefix@$MDTABLEFIX_VERSION" -``` - -Version drift matters here beyond reproducibility: a different `mdtablefix` -version may reflow prose differently, which would make `make check-fmt` fail on -an otherwise clean tree. - -CI pins the Whitaker installer version in `WHITAKER_INSTALLER_VERSION` in -`.github/workflows/ci.yml`. Install that same version locally so local linting -matches CI; read the pin from the workflow rather than copying the number, so -the two cannot drift: +Install the separately versioned Whitaker installer with: ```bash -WHITAKER_INSTALLER_VERSION="$(sed -n \ - "s/.*WHITAKER_INSTALLER_VERSION: '\\(.*\\)'.*/\\1/p" \ +WHITAKER_INSTALLER_VERSION="$(sed -n "s/.*WHITAKER_INSTALLER_VERSION: '\\(.*\\)'.*/\\1/p" \ .github/workflows/ci.yml)" -cargo install --locked whitaker-installer \ - --version "$WHITAKER_INSTALLER_VERSION" +# Build from crates.io: +cargo install --locked whitaker-installer --version "$WHITAKER_INSTALLER_VERSION" # or, for a prebuilt binary: cargo binstall --no-confirm --locked \ "whitaker-installer@$WHITAKER_INSTALLER_VERSION" @@ -725,8 +701,8 @@ so the staged libraries must be recent enough to include it. Libraries staged from an older checkout ignore `excluded_paths` silently — the exemptions stop applying with no error, and the lint reports the modules they covered. Re-run `whitaker-installer` to restage from HEAD. If that checkout has been left on a -detached HEAD, the install fails at its `git pull`; put it back on the default -branch and re-run. +detached HEAD, the installation fails during its `git pull`; put it back on the +default branch and re-run. [whitaker-pr-315]: https://github.com/leynos/whitaker/pull/315 @@ -810,7 +786,7 @@ backend. For a faster inner loop between gate runs, see For documentation changes, also run `make fmt`, `make markdownlint`, and `make nixie`. -### Workflow pins and Dependabot +## Workflow pins and Dependabot Dependabot owns the upgrade of GitHub Actions and reusable workflows, including calls into `leynos/shared-actions`. Contract tests that assert a caller's exact @@ -853,7 +829,7 @@ shared-action revision: the caller would keep working across any upstream bump, so pinning the SHA in a test buys nothing and costs a manual edit per bump. `tests/workflow_contracts/mutation_testing_test.py` is the canonical example. -#### Exception: the Polonius shared-action contract +### Exception: the Polonius shared-action contract The four workflows described under [Polonius CI shared-action contract](#polonius-ci-shared-action-contract) do @@ -1015,6 +991,14 @@ every command in this completion checklist: `markdownlint-cli2 --fix`. `mdtablefix` owns table padding and paragraph wrapping; `make markdownlint` then verifies the result. +`make check-fmt` runs the Rust and Python formatter checks, then passes tracked +Markdown files to `scripts/check-markdown-format.sh`. The wrapper skips the +Markdown check when the file list is empty, so the command remains portable +across hosts. The checker requires `mdtablefix` version `0.5.0`, the version +pinned by `MDTABLEFIX_VERSION` in the CI workflow; verify an installation with +`mdtablefix --version`. Run `make test-markdown-format` to exercise the +checker, including its empty-input behaviour, before changing the wrapper. + markdownlint's `MD060` (table-column-style) checks that table pipes align using a display-width model that treats CJK characters and emoji as double-width. That model disagrees with `mdtablefix`'s padding for right-to-left scripts, @@ -1031,37 +1015,9 @@ Contributors should prefer a file-scoped `markdownlint-disable-file` directive (or a narrower `markdownlint-disable-next-line`) over disabling a rule repository-wide, and should record the reason in a comment beside the directive. -`make check-fmt` verifies Markdown formatting as well as Rust formatting. It -runs `scripts/check-markdown-format.sh`, which compares each file against -`mdtablefix`'s output for that file and reports any that differ. `mdtablefix` -has no check-only mode, and the script never modifies tracked files. - -`mdtablefix` emits LF line endings, but Git can check files out with CRLF on -Windows. The script therefore accepts either the formatter's exact LF output or -its exact CRLF rendering; it does not accept mixed line endings or other text -differences. - -The script deliberately does not replay the `markdownlint-cli2 --fix` pass that -`make fmt` performs after `mdtablefix`. `make markdownlint` already rejects any -lint violation, so on a passing tree that pass has nothing to change. - -Checking against `mdtablefix` alone has a second benefit: it surfaces documents -where `mdtablefix` and markdownlint would fight. A heading nested inside an -ordered list is the common case: `mdtablefix` treats the heading as ending the -list and restarts the numbering at 1, while `MD029: ordered` renumbers it to -continue. Because `--fix` runs last, `make fmt` leaves such a file passing lint -but permanently unstable, so the check flags it. The remedy is to restructure -the document, for example by replacing the nested heading with a bold lead-in, -rather than to relax the check. - -Contributors who change the `mdtablefix` flags in `mdformat-all` must change -them in `scripts/check-markdown-format.sh` to match. - -The repository's Markdown is now held in canonical form, and `make check-fmt` -enforces it, so `make fmt` should no longer produce unrelated reflow. If it -does, that indicates a formatter version change or a file that has drifted, and -the resulting diff belongs in its own commit rather than being reverted -piecemeal. +Note that `mdformat-all` rewraps every Markdown file it finds, not only the +files a change touches. Revert the unrelated reflow before committing so a +change stays reviewable. ## Spelling enforcement @@ -1572,6 +1528,14 @@ on the repository's then-pinned `nightly-2026-06-25` supplying Cranelift between the two rows is the durable signal, not the seconds; the run below is representative of three consecutive runs that agreed to within 0.4 s. +`make bench-glob-expansion` measures `glob_paths("**/*.txt", Some(base))` +against its equivalent absolute, unbased pattern. Its deterministic fixture is +created before timing starts and each result is passed to `test::black_box`, so +the benchmark measures expansion rather than fixture construction or an +optimized-away query. Use it when changing glob-base preparation, path +rebasing, or separator formatting; compare the two cases on the same machine, +not their absolute timings across hosts. + | Variant | Clean build (s) | Incremental build (s) | | ------------------------------- | --------------- | --------------------- | | Default (LLVM, platform linker) | 11.6 | 0.8 | @@ -1748,7 +1712,7 @@ Cargo home plus Kani support-file home. ## Test execution -`make test` is the canonical entry point and composes two passes: +`make test` is the canonical entry point and composes two stages: - `make test-nextest` — `cargo nextest run --workspace --all-targets --all-features`, with @@ -1762,7 +1726,7 @@ Cargo home plus Kani support-file home. doctests, so they need their own pass; the separate target is what makes a broken documentation example fail the gate. -If either pass fails, `make test` fails. Run the individual targets when +If any stage fails, `make test` fails. Run the individual targets when iterating, but treat `make test` as the gate. ### Required real-Ninja coverage @@ -1805,10 +1769,10 @@ governs the non-doctest pass only, and deliberately stays small: nextest runs each test in its own process, but the codebase does not rely on that isolation for environment safety. Tests pass environment values through explicit configuration seams or configure a child with `env_clear()` followed by -`Command::env`. The BDD suite carries no environment or CWD lock: its steps -run inside the generated test-harness process, not inside an `assert_cmd` child. -`EnvLock` and `CwdGuard` remain only for direct tests that deliberately -exercise process working-directory behaviour outside that suite. +`Command::env`. Working-directory behaviour is exercised by injecting a base +directory through the manifest and discovery seams rather than changing the +process working directory, because the in-process coverage runner shares that +state. ### Runners not covered by this configuration @@ -2242,19 +2206,15 @@ cargo-nextest alongside every other test (see point (`world: TestWorld`) to each generated scenario test. nextest runs each generated scenario in its own process. That reinforces the -per-scenario isolation policy below rather than conflicting with it. The -scenario's steps still execute in that generated test-harness process, so an -in-process BDD step does not qualify for subprocess isolation. +per-scenario isolation policy below rather than conflicting with it: scenario +state cannot leak across process boundaries, so the policy's requirement to +recreate state per test is enforced by the runner as well as by convention. ### State and isolation policy - Scenario isolation is the default: scenario state must be recreated per test. - Shared process-wide state is avoided unless infrastructure cost requires controlled reuse. -- Route A drives an end-to-end `netsuke` child with `assert_cmd` and configures - only that child with `Command::env`. -- Route B calls a library entry point with its injected environment and keeps - assertions on in-process values such as `Cli`, `Manifest`, or `BuildGraph`. - Use `Slot` for optional or replaceable scenario values. - Use typed wrappers in `tests/bdd/types.rs` for step parameters to avoid ambiguous string-heavy signatures. @@ -2388,6 +2348,36 @@ character, non-ASCII byte, or other punctuation returns a MiniJinja Jinja-specific: `glob_paths` retains its filesystem-query contract and returns all matching UTF-8 file paths without applying shell-safety validation. +### Base-directory seam + +`glob_paths(pattern, base)` and the internal `expand_glob(pattern, base)` take +an optional injected `Utf8Path` base. The manifest parse boundary owns the +workspace-root decision and passes that root to the query closure; a +manifest-rooted parse therefore neither reads nor mutates process-global +working-directory state during glob expansion. + +- A relative pattern, including a parent-relative pattern, resolves from the + manifest directory or workspace root. The resolved base is stripped only + after matching, so results retain the spelling relative to the original + pattern (`../shared/file.txt` remains parent-relative). +- An absolute pattern does not use or strip the injected base. +- `PreparedGlob` canonicalizes a valid relative base to preserve symlinked + workspace behaviour, escapes that base as a literal for glob compilation, and + retains the unescaped path for result rebasing. `open_root_dir` receives + `None` for this prepared search because the base is already embedded. + +The focused base and property tests cover no-double-base, symlinked base, +base-path metacharacters, canonicalization failure, nested and parent-relative +results, absolute patterns, and forward-slash output. Run +`make bench-glob-expansion` alongside the relevant tests when changing this hot +path. + +The adjacent configuration-discovery seam keeps its ownership boundary clear: +`-C/--directory` anchors manifest lookup and automatic project discovery. +Explicit `--config` and `NETSUKE_CONFIG` selectors remain independent. Their +relative paths resolve from the process working directory and their absolute +paths are unchanged. + ### Capability scope The metadata check that filters directories out of a glob's results goes @@ -2447,10 +2437,11 @@ directory, and matches dropped because a symbolic link cannot be resolved through the capability, including an unreadable link within the prefix. It aggregates every skipped entry while retaining at most the first four unreachable-symlink paths as a trace sample. The `src/manifest/mod.rs` adapter -records those observations after the query at the Jinja `glob` helper's -orchestration boundary, via `glob::record_expansion`. Keeping recording there -leaves the expansion query free of metrics and tracing side effects while -keeping a degraded expansion visible without having to reproduce it. +records those observations and the whole expansion duration at the Jinja `glob` +helper's orchestration boundary, via `glob::expand_manifest_template_glob`. +Keeping recording there leaves the expansion query free of metrics and tracing +side effects while keeping a degraded or failed template expansion visible +without having to reproduce it. - **Metrics** — `netsuke_manifest_glob_expansions_total`, labelled `outcome` (`matched`, `unopenable_prefix`), and @@ -2461,15 +2452,28 @@ keeping a degraded expansion visible without having to reproduce it. rule in `AGENTS.md`. The Jinja adapter additionally records `netsuke_manifest_glob_rejections_total` with `outcome=unsafe_path` and `error_category=shell_quoting_required` when its shell-safety boundary - rejects a match. + rejects a match. It also records + `netsuke_manifest_template_glob_expansions_total`, labelled with the closed + `base_mode` (`absolute_pattern`, `relative_without_base`, + `relative_with_base`) and `outcome` (`matched`, `unopenable_prefix`, + `invalid_pattern`, `base_canonicalization_failure`, `utf8_conversion_failure`, + `capability_root_io_failure`, `glob_entry_processing_failure`) sets, plus + the unlabelled `netsuke_manifest_template_glob_expansion_duration_seconds` + histogram. The base mode classifies the pattern and manifest-root context; + absolute patterns bypass the configured root without resolving it. Direct + `glob_paths` queries remain pure and emit no metrics or tracing. + Template-boundary tracing uses the same bounded mode and outcome fields, with + caller-controlled paths and error text redacted. - **Tracing** — every caller-controlled path field is replaced with the stable `` marker: patterns, prefixes, and sampled relative matches. A skipped unreachable-symlink event is emitted only for the retained sample, with no more than four such events per expansion. Metrics retain only bounded aggregate status and reason data; errors may retain the caller's original - original pattern so invalid input can be explained precisely. Adapter - rejection events use the same `` path marker and carry only the - bounded outcome and error category. + pattern so invalid input can be explained precisely. Adapter rejection events + use the same `` path marker and carry only the bounded outcome and + error category. Template-expansion success, unopenable prefix, and error + events carry only the same bounded mode and outcome fields; failures use the + closed outcome set documented above. ## Test isolation utilities @@ -2477,12 +2481,10 @@ Environment variable mutations and working-directory changes are process-global side effects that can cause data races when tests run in parallel. Tests inject environment readers where the API supports them, and configure child processes with `env_clear()` followed by `Command::env` where ambient discovery is part -of the contract. BDD steps must not change either process-global value: use an -injected environment and absolute paths for in-process library assertions, or -an isolated `assert_cmd` child for end-to-end behaviour. `CwdGuard` is the RAII -utility for the few direct CWD tests that deliberately exercise it. For -locale-sensitive snapshot tests, use the `EnLocalizer` scoped pattern -documented in the +of the contract. Working-directory behaviour is exercised by injecting a base +directory (the manifest glob base, or `project_scope_file`'s directory) rather +than changing the process working directory. For locale-sensitive snapshot +tests, use the `EnLocalizer` scoped pattern documented in the [snapshot testing guide](snapshot-testing-in-netsuke-using-insta.md#locale-pinned-snapshot-tests). `src/snapshot_test_support.rs` owns output-oriented unit-test fixtures; @@ -2716,14 +2718,18 @@ section for the fixture's intended usage. ### Enforcing the environment mandate -`clippy.toml` disallows the six process-environment entry points, so +`clippy.toml` disallows the seven process-environment entry points, so `make lint` rejects a new one: ```toml disallowed-methods = [ { path = "std::env::var", reason = "inject an environment reader" }, + { path = "std::env::var_os", reason = "inject an environment reader" }, + { path = "std::env::vars", reason = "inject an environment reader" }, + { path = "std::env::vars_os", reason = "inject an environment reader" }, { path = "std::env::set_var", reason = "use a stub environment in tests" }, - # ... var_os, vars, vars_os, remove_var + { path = "std::env::remove_var", reason = "use a stub environment in tests" }, + { path = "std::env::set_current_dir", reason = "inject a base-directory seam" }, ] ``` @@ -2982,88 +2988,34 @@ process. - The reader answers by name only. It must not enumerate, and it must not mutate. -#### Manifest workspace base seam - -`resolve_absolute_workspace_root` and `open_manifest_workspace` in -`src/manifest/workspace.rs` anchor the workspace directory containing a -manifest. Both are `pub(super)` and are not exported from -`src/manifest/mod.rs`: the seam is a working-directory injection point, not a -general path-configuration API. - -Ownership and permitted call sites: - -- Production passes `None`; the sole caller is `from_path_with_registration` in - `src/manifest/query.rs`, so ambient current-directory resolution is unchanged. -- Tests inject a temporary directory or a relative base such as - `Some(Path::new("."))`; the seam must not become a general path-configuration - API. - -Composition rules: - -- An absolute parent wins outright; `base` is ignored. -- A relative parent joins onto `base`; `None` falls back to the process - current directory. -- A relative `base` (for example `Path::new(".")`) is itself anchored at the - current directory before joining, so `ManifestWorkspace::root` always stays - absolute. -- Both the `None` fallback and relative `base` anchoring call - `env::current_dir()`; either failure reports the - `MANIFEST_RESOLVE_WORKSPACE_ROOT` localization key. - -This is a working-directory seam, distinct from the three environment-variable -shapes in [ADR-008](adr-008-environment-seam-taxonomy.md); it mirrors the -`which` resolver's `cwd_override` precedent in -[`EnvSnapshot::capture`](#which-environment-capture). - -### `EnvLock` - -`test_support::env_lock::EnvLock` serializes the few tests that change the -process working directory. It is retired and retained only until those callers -migrate; do not add callers or tests. -[ADR-008](adr-008-environment-seam-taxonomy.md) records its retirement. -Environment-variable callers migrate to injected `mockable::Env` seams in -production signatures. Current-working-directory callers migrate to the -existing working-directory seam, such as an injected base or current-directory -path, or use absolute paths or `-C/--directory` instead. Until the current -CWD-only callers migrate, acquire it before `CwdGuard` so restoration occurs -before the lock is released: - -```rust -use test_support::env_lock::EnvLock; - -let _env_lock = EnvLock::acquire(); -``` - -Do not use this lock to justify process-environment mutation. Environment -access must remain injected, or confined to a spawned child process. BDD -scenarios must not acquire this lock: they are in-process tests, and a lock -would serialize the suite rather than isolate an ambient dependency. - -### `CwdGuard` - -Tests that call `std::env::set_current_dir` must restore the original working -directory after the test. `CwdGuard` is available from `test_support`; it -captures the current directory on construction and restores it on drop: - -```rust -use test_support::CwdGuard; -use test_support::env_lock::EnvLock; - -let _env_lock = EnvLock::acquire(); -let _cwd_guard = CwdGuard::acquire()?; -std::env::set_current_dir(temp.path())?; -``` - -Acquire `EnvLock` and then `CwdGuard` so Rust drops them in reverse declaration -order: `CwdGuard` restores the CWD first, and `EnvLock` releases second. - -These direct CWD tests are the narrow exception for exercising CWD-dependent -code itself. BDD scenarios instead retain an absolute manifest path or pass -`-C/--directory` into the CLI; neither approach changes the harness process CWD. - -This legacy ordering remains only for current CWD-only callers while they -migrate; do not add `EnvLock` callers or tests. -[ADR-008](adr-008-environment-seam-taxonomy.md) records its retirement. +### Retired process-environment mutation utilities + +The `EnvLock`, `CwdGuard`, and `EnvVarGuard` utilities that once serialized +process working-directory and environment mutation were retired from +`test_support`. Tests and harnesses inject data through seams instead: the +manifest glob base directory anchors relative globs, `project_scope_file` +accepts an explicit directory for configuration discovery, and environment +readers are injected into the functions that need them. None of these depend on +the process working directory or process-global environment state. + +`make lint` runs rustdoc, Clippy, and Whitaker. Clippy's workspace-wide +`disallowed-methods` configuration rejects `std::env::set_var`, +`std::env::remove_var`, and `std::env::set_current_dir` in every target kind +with warnings denied. Child-process configuration stays confined to the +`Command` builders: `Command::env`, `Command::env_clear`, and +`Command::current_dir`. + +### Scripting standards for automation scripts + +Python scripts under `scripts/` follow the repository's +[scripting standards](scripting-standards.md): a `uv` script block with a +Python 3.14 floor, Cyclopts for parameterized CLIs, `cuprum` for subprocess +execution, `pathlib` for filesystem access, and pytest coverage in +`scripts/tests/` mirroring each script's name. The house Python style rules in +`.rules/` (naming, typing, exception design, context managers, generators, and +returns) apply to every script and its tests. Refer to +[`docs/scripting-standards.md`](scripting-standards.md) before introducing or +changing an automation script. ### Injected and child-process environments @@ -3088,25 +3040,15 @@ appropriate injected seam, such as `run_with_ninja_program`, `StdlibConfig::with_path_override`, `StdlibConfig::with_home_override`, or `StdlibConfig::with_command_path_override`. End-to-end tests may call `env_clear()` and then apply values with `Command::env`, because the mutation -is confined to the child. This defines two BDD routes: Route A drives the -compiled binary with `assert_cmd` and configures only its child environment; -Route B calls library entry points with an injected environment and keeps the -scenario's in-process assertions on `Cli`, `Manifest`, `BuildGraph`, or render -state. +is confined to the child. ### Ordering rules 1. Inject environment-dependent inputs whenever the API supports them. 2. Use an isolated child process for APIs whose contract is ambient discovery. -3. In BDD, choose Route A for an end-to-end binary assertion or Route B for an - injected in-process library assertion. -4. Retain absolute paths or pass `-C/--directory` instead of changing the BDD - harness CWD. -5. For current legacy CWD-only callers, acquire `EnvLock` and then `CwdGuard`. -6. Add no `EnvLock` callers or tests; - [ADR-008](adr-008-environment-seam-taxonomy.md) records its retirement, and - issue #494 tracks removal. -7. Never mutate the harness process environment. +3. Inject a base directory through the manifest/glob seams for + working-directory-sensitive tests. +4. Never mutate the harness process environment. ### `tracing_capture` @@ -3180,18 +3122,18 @@ maintenance. Table: Scenario state groups and fields -| Group | Fields | Purpose | -| :----------------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------- | -| CLI state | `cli`, `cli_error` | Parsed CLI configuration and parse error capture. | -| Manifest state | `manifest`, `manifest_error` | Parsed manifest and error capture. | -| IR state | `build_graph`, `removed_action_id`, `generation_error` | Build graph, negative-test identifiers, generation errors. | -| Ninja state | `ninja_content`, `ninja_error` | Generated Ninja file content and errors. | -| Process state | `run_status`, `run_error`, `command_stdout`, `command_stderr`, `temp_dir`, `workspace_path`, `command_env` | Process results, workspace paths, child environment. | -| Stdlib state | `stdlib_root`, `stdlib_output`, `stdlib_error`, `stdlib_state`, `stdlib_command`, `stdlib_policy`, `stdlib_path_override`, `stdlib_fetch_max_bytes`, `stdlib_command_max_output_bytes`, `stdlib_command_stream_max_bytes`, `stdlib_text` | Stdlib rendering, network policy, and size constraints. | -| Localization state | `localization_lock`, `localization_guard`, `locale_config`, `locale_env`, `locale_cli_override`, `locale_system`, `resolved_locale`, `locale_message` | Scenario-level localizer overrides and resolution state. | -| HTTP server state | `http_server`, `stdlib_url` | Test HTTP server fixture for fetch scenarios. | -| Output state | `output_mode`, `simulated_no_color`, `simulated_term`, `output_prefs`, `simulated_no_emoji`, `rendered_prefix` | Accessibility and output preference resolution. | -| Environment state | `env_vars_forward` | Child environment map for Route A scenarios. | +| Group | Fields | Purpose | +| :----------------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :----------------------------------------------------------- | +| CLI state | `cli`, `cli_error` | Parsed CLI configuration and parse error capture. | +| Manifest state | `manifest`, `manifest_error` | Parsed manifest and error capture. | +| IR state | `build_graph`, `removed_action_id`, `generation_error` | Build graph, negative-test identifiers, generation errors. | +| Ninja state | `ninja_content`, `ninja_error` | Generated Ninja file content and errors. | +| Process state | `run_status`, `run_error`, `command_stdout`, `command_stderr`, `temp_dir`, `workspace_path`, `command_env` | Process results, workspace paths, child environment. | +| Stdlib state | `stdlib_root`, `stdlib_output`, `stdlib_error`, `stdlib_state`, `stdlib_command`, `stdlib_policy`, `stdlib_path_override`, `stdlib_fetch_max_bytes`, `stdlib_command_max_output_bytes`, `stdlib_command_stream_max_bytes`, `stdlib_text` | Stdlib rendering, network policy, and size constraints. | +| Localization state | `localization_lock`, `localization_guard`, `locale_config`, `locale_env`, `locale_cli_override`, `locale_system`, `resolved_locale`, `locale_message` | Scenario-level localizer overrides and resolution state. | +| HTTP server state | `http_server`, `stdlib_url` | Test HTTP server fixture for fetch scenarios. | +| Output state | `output_mode`, `simulated_no_color`, `simulated_term`, `output_prefs`, `simulated_no_emoji`, `rendered_prefix` | Accessibility and output preference resolution. | +| Environment state | `env_vars_forward` | Child process environment map forwarded to spawned commands. | ### Key `TestWorld` methods @@ -3413,9 +3355,10 @@ uses the bare `EnvProvider` name. Tests for injected configuration discovery should provide a map-backed `ConfigEnvProvider`. End-to-end tests of the ambient `ConfigStdEnvProvider` adapter must run in an isolated child configured with `env_clear()` followed by -`Command::env`. BDD configuration steps use the injected route; direct CWD -tests alone may use `EnvLock` alongside `CwdGuard`. Neither guard justifies -environment mutation. +`Command::env`. The retired `EnvLock`/`CwdGuard` utilities are gone; +working-directory-dependent config tests inject the anchor directory (for +example `project_scope_file` with an explicit directory) instead of changing +the process environment. Unit tests that only need to verify explicit config path precedence should test `explicit_config_path_with_env` with an injected provider instead of mutating @@ -4260,48 +4203,6 @@ context rather than resolving output or process configuration again; tests should inject the program through `run_with_ninja_program` when they need a deterministic child executable. -### Module: `runner::generation` - -`src/runner/generation.rs` owns the runner's reusable, in-memory generation -pipeline. It separates manifest loading, IR construction, and Ninja bundle -synthesis from command reporting and process execution. The read-only pipeline -is `load_manifest` (optionally observing manifest stages), then `build_graph`, -then `ninja_text`. Its final value is `GeneratedNinja`, including any dyndep -sidecars, rather than a materialized file or a running Ninja process. - -`load_manifest` uses the manifest-query registration: it permits only its -read-only helpers and rejects template access to the environment, filesystem, -network, clock, and shell. `load_manifest_for_build` is a separate, explicitly -effectful loader for build, clean, generate, and graph commands. It receives a -network policy and enables the full build stdlib; it is not a dry-run or -background-query primitive. - -#### Generation reuse boundary - -- **Ownership:** `runner::generation` is a private runner submodule. It owns - the three read-only generation steps, the explicitly effectful build loader, - their input/output hand-offs, and the manifest and IR error contexts. It does - not own `StatusReporter` updates, command dispatch, dyndep publication, or - Ninja execution. -- **Permitted call-sites:** `runner::generate_ninja` composes the complete - build pipeline through `load_manifest_for_build` for build, clean, and - generate commands. `runner::graph::handle_graph` may stop after `build_graph` - to render the graph, and `runner::help_query` uses `load_manifest` for its - read-only target catalogue. Runner unit tests may compose the read-only steps - directly. New dry-run or background-generation work may use `load_manifest`, - `build_graph`, and `ninja_text` only within the runner boundary; a public or - cross-subsystem consumer requires an explicit application boundary rather - than widening these internal helpers. -- **Composition rules:** command adapters report stages before or after the - relevant step and wrap `ninja_text` with runner-owned generation telemetry. - Only `load_manifest_with_stage_reporting` translates `StageObserver` events - into status updates and selects the effectful build loader. Consumers must - not call manifest parsing, IR generation, or `ninja_gen::generate_bundle` - directly in parallel with this pipeline. Before an adapter writes or executes - a returned bundle, it must use the existing capability-injected - dyndep-publication path to materialize its sidecars; the read-only steps - never write files, start processes, or invoke effectful template helpers. - ### Module: `runner::reporter` `src/runner/reporter.rs` owns construction of the run's `StatusReporter` from diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index c7d9c7641..624e01a02 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1152,15 +1152,23 @@ providing a secure bridge to the underlying system. these are normalized to the host platform before matching. Results contain only files (directories are ignored) and path separators are normalized to `/`. Leading-dot entries are matched by wildcards. Empty results are - represented as `[]`. Invalid patterns surface as `SyntaxError`; filesystem - iteration errors surface as `InvalidOperation`, matching minijinja error - semantics. On Unix, backslash escapes for glob metacharacters (`[`, `]`, `{`, - `}`, `*`, `?`) are preserved during separator normalization. A backslash - before `*` or `?` is kept only when the wildcard is trailing or followed by - an alphanumeric, `_`, or `-`; otherwise it becomes a path separator so - `config\*.yml` maps to `config/*.yml`. On Windows, backslash escapes are not - supported. This provides globbing support not available in Ninja itself, - which does not support globbing.[^3] + represented as `[]`. The manifest parse boundary supplies the manifest + directory or workspace root to `glob_paths(pattern, base)` and internal + `expand_glob(pattern, base)`: relative patterns, including parent-relative + ones, resolve from that root and retain their pattern-relative spelling after + base stripping, while absolute patterns remain absolute. With an injected + manifest root, this expansion does not read or mutate process-global + working-directory state. `from_str_with_env` passes `manifest_root: None`, so + unbased relative patterns resolve from the process working directory. Invalid + patterns surface as `SyntaxError`; filesystem iteration errors surface as + `InvalidOperation`, matching minijinja error semantics. On Unix, backslash + escapes for glob metacharacters (`[`, `]`, `{`, `}`, `*`, `?`) are preserved + during separator normalization. A backslash before `*` or `?` is kept only + when the wildcard is trailing or followed by an alphanumeric, `_`, or `-`; + otherwise it becomes a path separator so `config\*.yml` maps to + `config/*.yml`. On Windows, backslash escapes are not supported. This + provides globbing support not available in Ninja itself, which does not + support globbing.[^3] The Rust query returns those UTF-8 paths unchanged. The Jinja `glob()` adapter adds the narrower command-safety boundary: it exposes a result only @@ -3327,7 +3335,7 @@ flowchart LR ``` Netsuke configuration discovery is implemented in `src/cli/discovery.rs`. -Explicit file selection is handled by `explicit_config_path_with_env(...)`, +Explicit file selection is handled by `selector::resolve_config_selector(...)`, which applies the precedence `--config` > `NETSUKE_CONFIG`. `discover_file_layers(...)` performs one overall discovery pass, applying the `-C/--directory` flag as the project-discovery root. Its automatic path first @@ -3412,8 +3420,10 @@ arguments then override the file layers. 1. **Explicit override**: `--config ` and `NETSUKE_CONFIG` are evaluated in that precedence order before discovery. These explicit selectors bypass - automatic discovery and ignore the project-root anchor supplied by - `-C/--directory`. + automatic discovery. A relative explicit selector resolves from the process + working directory and is not rebased by `-C/--directory`; an absolute + selector remains unchanged. The `-C/--directory` project-root anchor applies + to manifest lookup and automatic project configuration discovery. 2. **Project scope**: Configuration files in the current working directory (or the directory specified via `-C/--directory`): @@ -3456,9 +3466,9 @@ manual flag repetition. **Implementation notes**: -- The `explicit_config_path_with_env(...)` helper resolves explicit config - selectors before automatic discovery so missing or invalid explicit files - remain hard errors. +- `selector::resolve_config_selector(...)` resolves explicit config selectors + before automatic discovery so missing or invalid explicit files remain hard + errors. - The `merge_with_config_and_env()` function in `src/cli/merge.rs` performs discovery and delegates to the ordinary merge query, which discards its collected events. The application startup boundary replays retained bounded @@ -3489,10 +3499,11 @@ manual flag repetition. automatic discovery. If an explicit selector is set, the selected file is loaded directly and bypasses discovery, but still participates in the normal precedence ladder: defaults < file < environment < CLI. -- Relative paths passed to `--config` are resolved against the process current - working directory, not the `-C/--directory` anchor. This keeps config-file - selection aligned with normal shell path semantics while `-C` continues to - scope project discovery and manifest lookup. +- Relative paths passed to `--config` (and `NETSUKE_CONFIG`) resolve from the + process working directory independently of `-C/--directory`. Absolute + selectors retain their original spelling. `-C` continues to anchor project + discovery and manifest lookup. Pass an absolute path when the selector must + not depend on the invoking directory (see ADR-004). ### 8.5 Manual Pages diff --git a/docs/roadmap.md b/docs/roadmap.md index a6ec31e2a..e64e79f2a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -170,16 +170,16 @@ and agents. selected (user-over-system and system-only merge-through cases are covered by the regression tests added in issue #385). -- [ ] 3.11.5. Retire `EnvLock` rather than harden its synchronization tests. - - [ ] Ensure production environment-variable callers accept injected +- [x] 3.11.5. Retire `EnvLock` rather than harden its synchronization tests. + - [x] Ensure production environment-variable callers accept injected `mockable::Env` seams. - - [ ] Ensure tests use `mockable::MockEnv` or isolated child processes. - - [ ] Ensure CWD-only callers use the existing working-directory seam, + - [x] Ensure tests use `mockable::MockEnv` or isolated child processes. + - [x] Ensure CWD-only callers use the existing working-directory seam, absolute paths, or `-C/--directory`. - [x] Migrate the remaining callers under issue #491. - [x] Migrate the remaining callers under issue #492. - [x] Migrate the remaining callers under issue #493. - - [ ] Remove `EnvLock` under issue #494 after every remaining `EnvLock` + - [x] Remove `EnvLock` under issue #494 after every remaining `EnvLock` caller has migrated. - See [ADR-008](adr-008-environment-seam-taxonomy.md). diff --git a/docs/scripting-standards.md b/docs/scripting-standards.md new file mode 100644 index 000000000..7dbf620d2 --- /dev/null +++ b/docs/scripting-standards.md @@ -0,0 +1,694 @@ +# Scripting standards + +Project scripts must prioritize clarity, reproducibility, and testability. + +Cyclopts is the default command‑line interface (CLI) framework for new and +updated scripts. This document supersedes prior guidance that recommended Typer +as a default. + +## Rationale for adopting Cyclopts + +- Environment‑first configuration without glue. Cyclopts reads environment + variables with a defined prefix (for example, `INPUT_`) and maps them to + parameters directly. Bash argument assembly and bespoke parsing can be + removed. +- Typed lists and paths from env. Parameters annotated as `list[str]` or + `list[pathlib.Path]` are populated from whitespace‑ or delimiter‑separated + environment values. Custom split/trim helpers are unnecessary. +- Clear precedence model. CLI flags override environment variables, which + override code defaults. Behaviour is predictable in both CI and local runs. +- Small API surface. The API is explicit and integrates cleanly with type + hints, aiding readability and testing. +- Backwards‑compatible migration. Option aliases and per‑parameter + environment variable names permit preservation of existing interfaces while + removing shell glue. + +## Language and runtime + +- Target Python 3.14 for all new scripts. Older versions may only be used when + integration constraints require them, and any exception must be documented + inline. +- Each script starts with an `uv` script block so runtime and dependency + expectations travel with the file. Prefer the shebang + `#!/usr/bin/env -S uv run --script` followed by the metadata block shown in + the example below. `uv run --script` reads the PEP 723 inline metadata block + and installs its declared dependencies before execution; `uv run python` + invokes the interpreter directly and silently ignores the metadata block, so + a directly executed script (`./script.py`) fails at import time because its + dependencies were never installed. +- External processes are invoked via + [`cuprum`](https://github.com/leynos/cuprum/) to provide typed, + allowlist-based command execution rather than ad‑hoc shell strings. Cuprum's + catalogue system ensures only registered programs can be executed, preventing + accidental shell access. +- File‑system interactions use `pathlib.Path`. Higher‑level operations (for + example, copying or removing trees) go through the `shutil` standard library + module. + +### Cyclopts CLI pattern (environment‑first) + +Employ Cyclopts when a script requires parameters, particularly under CI with +`INPUT_*` variables. + +```python +from __future__ import annotations + +from pathlib import Path +from typing import Optional, Annotated + +import cyclopts +from cyclopts import App, Parameter +from cuprum import Catalogue, sh + +# Map INPUT_ → function parameter without additional glue +app = App(config=cyclopts.config.Env("INPUT_", command=False)) + + +@app.default +def default( + *, + # Required parameters + bin_name: Annotated[str, Parameter(required=True)], + version: Annotated[str, Parameter(required=True)], + + # Optional scalars + package_name: Optional[str] = None, + target: Optional[str] = None, + outdir: Optional[Path] = None, + dry_run: bool = False, + + # Lists (whitespace/newline separated by default) + formats: list[str] | None = None, + man_paths: Annotated[list[Path] | None, Parameter(env_var="INPUT_MAN_PATHS")] = None, + deb_depends: list[str] | None = None, + rpm_depends: list[str] | None = None, +): + name = package_name or bin_name + + project_root = Path(__file__).resolve().parents[1] + build_dir = (outdir or (project_root / "dist")) / name + + if dry_run: + print({ + "name": name, + "version": version, + "target": target, + "formats": formats, + "man_paths": [str(p) for p in (man_paths or [])], + "deb_depends": deb_depends, + "rpm_depends": rpm_depends, + "build_dir": str(build_dir), + }) + return + + build_dir.mkdir(parents=True, exist_ok=True) + catalogue = Catalogue.from_programs("tofu") + with sh.scoped(catalogue): + result = sh.make("tofu")("plan", cwd=build_dir).run_sync() + if result.exit_code != 0: + raise SystemExit(result.exit_code) + +def main(): + """CLI Entrypoint""" + app() + + +if __name__ == "__main__": + main() + +``` + +Guidance: + +- Parameter names should be descriptive and stable. Where a legacy flag name + must remain available, add an alias: + + ```python + package_name: Annotated[Optional[str], Parameter(aliases=["--name"])] = None + ``` + +- Where a specific delimiter is required for an environment list (for example, + comma‑separated `formats`), specify it per parameter: + + ```python + formats: Annotated[list[str] | None, Parameter(env_var_split=",")] = None + ``` + +- Per‑parameter environment names can be pinned for backwards compatibility: + + ```python + config_out: Annotated[Optional[Path], Parameter(env_var="INPUT_CONFIG_PATH")] = None + ``` + +## cuprum: typed command execution + +Cuprum provides allowlist-based command execution with built-in observability. +Programs must be registered in a catalogue before they can be executed, +preventing accidental shell access. + +### Shared vs local catalogues + +For application code in a multi-script repository, use a shared catalogue in a +common module (for example, `project/utils/commands.py`). This centralizes the +list of allowed programs and ensures consistent access control across the +codebase: + +```python +from project.utils.commands import PROJECT_CATALOGUE +from cuprum import Catalogue, sh + +with sh.scoped(PROJECT_CATALOGUE): + # All project code uses the shared catalogue + ... +``` + +For standalone scripts and tests, define a local catalogue scoped to that +file's requirements. This keeps scripts self-contained and avoids coupling to +the main application: + +```python +# In a standalone script or test file +CATALOGUE = Catalogue.from_programs("git", "cargo") +``` + +### Catalogue and allowlisting + +```python +from cuprum import Catalogue, sh + +# Define allowed programs for this script +CATALOGUE = Catalogue.from_programs("git", "cargo", "grep") + +# Commands can only be constructed within a scoped catalogue +with sh.scoped(CATALOGUE): + git = sh.make("git") + result = git("--no-pager", "log", "-1", "--pretty=%H").run_sync() + last_commit = result.stdout.strip() +``` + +### Capturing output and handling failures + +```python +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("git", "grep") + +with sh.scoped(CATALOGUE): + git = sh.make("git") + + # run_sync() returns CommandResult with exit_code, stdout, stderr + result = git("status").run_sync() + if result.exit_code != 0: + # handle gracefully; result.stderr is available for logging + ... + + # Pipelines via the | operator with backpressure handling + log_cmd = git("--no-pager", "log", "--oneline") + grep_cmd = sh.make("grep")("fix") + shortlog = (log_cmd | grep_cmd).run_sync().stdout +``` + +### Working directory and environment management + +```python +from pathlib import Path +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("git") +repo_dir = Path(__file__).resolve().parents[1] + +with sh.scoped(CATALOGUE): + git = sh.make("git") + + # Working directory via cwd parameter + result = git("tag", "--list", cwd=repo_dir).run_sync() + tags = result.stdout + + # Read-only environment-sensitive command via env parameter + result = git( + "var", "GIT_AUTHOR_IDENT", + env={"GIT_AUTHOR_NAME": "CI", "GIT_AUTHOR_EMAIL": "ci@example.org"}, + ).run_sync() +``` + +### Keyword arguments as flags + +Cuprum transforms keyword arguments into `--flag=value` format automatically, +with underscores converted to hyphens: + +```python +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("cargo") + +with sh.scoped(CATALOGUE): + cargo = sh.make("cargo") + # Equivalent to: cargo build --release --target=x86_64-unknown-linux-gnu + result = cargo("build", release=True, target="x86_64-unknown-linux-gnu").run_sync() +``` + +### Observability hooks + +```python +import logging +from cuprum import Catalogue, sh, Hook + +LOGGER = logging.getLogger(__name__) +CATALOGUE = Catalogue.from_programs("cargo") + +def log_before(event): + LOGGER.info("Executing: %s", event.command) + +def log_after(event): + LOGGER.info("Completed with exit code %d", event.result.exit_code) + +with sh.scoped(CATALOGUE): + with sh.observe(Hook(before=log_before, after=log_after)): + sh.make("cargo")("check").run_sync() +``` + +### Async execution + +For I/O-bound workflows, Cuprum supports async execution: + +```python +import asyncio +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("cargo") + +async def run_checks(): + with sh.scoped(CATALOGUE): + cargo = sh.make("cargo") + # Async execution with run() + result = await cargo("check", "--all-targets").run() + return result.exit_code == 0 + +asyncio.run(run_checks()) +``` + +#### Task lifetime and `asyncio.gather` + +When multiple async commands run concurrently, each task's lifetime must be +bounded by the enclosing coroutine. Await the tasks with `asyncio.gather`, or +with `asyncio.TaskGroup` on Python 3.11 and later, so background command work +cannot escape the calling coroutine. + +```python +import asyncio +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("cargo", "python") + +async def run_all(): + with sh.scoped(CATALOGUE): + cargo = sh.make("cargo") + python = sh.make("python") + results = await asyncio.gather( + cargo("check", "--all-targets").run(), + python("-m", "pytest", "--tb=short").run(), + return_exceptions=True, + ) + return results +``` + +`return_exceptions=True` prevents a single task failure from cancelling sibling +tasks. Callers must inspect each result individually. + +#### Cancellation handling + +`asyncio.CancelledError` is not suppressed by Cuprum. If a task running `run()` +is cancelled, for example by a timeout or external signal, the coroutine raises +`CancelledError` as normal. Authors must not catch `CancelledError` silently. + +```python +async def check_with_timeout(): + with sh.scoped(CATALOGUE): + cargo = sh.make("cargo") + try: + result = await asyncio.wait_for( + cargo("build", "--release").run(), timeout=120.0 + ) + except asyncio.TimeoutError: + # Handle or re-raise; do not swallow CancelledError + raise + return result +``` + +#### Error propagation + +`run()` returns a `CommandResult` and does not raise on non-zero exit codes. +Subprocess errors are propagated as field values such as `exit_code` and +`stderr`, not as exceptions. Callers must check `result.exit_code` explicitly. +The exceptions raised are those from the Python event loop itself, such as +`CancelledError` and `TimeoutError`, or from catalogue violations such as +`UnknownProgramError`. + +#### Catalogue safety across concurrent tasks + +A `Catalogue` instance is safe to share across concurrent tasks because it is +read-only after construction. `sh.scoped(CATALOGUE)` is a context manager that +binds the catalogue for the current execution scope. Authors must not mutate +the catalogue inside a concurrent task. Construct the catalogue once at module +level and re-use it. + +#### Concurrent testing patterns with cmd-mox + +Concurrent async script paths use the same catalogue and scoped context in +tests as they do in production code. `cmd-mox` intercepts at the catalogue +boundary regardless of whether `run()` or `run_sync()` is used. + +```python +import pytest + + +@pytest.mark.asyncio +async def test_concurrent_commands_all_succeed(mock_catalogue): + mock_catalogue.register("cargo", exit_code=0, stdout="ok\n") + mock_catalogue.register("python", exit_code=0, stdout="passed\n") + + results = await run_all() # function under test + + assert all(r.exit_code == 0 for r in results) + + +@pytest.mark.asyncio +async def test_gather_continues_after_one_failure(mock_catalogue): + mock_catalogue.register("cargo", exit_code=1, stderr="error\n") + mock_catalogue.register("python", exit_code=0, stdout="passed\n") + + results = await run_all() + + exit_codes = [r.exit_code for r in results] + assert 1 in exit_codes + assert 0 in exit_codes +``` + +The `mock_catalogue` fixture replaces the real `CATALOGUE`. Authors must inject +it via a parameter or monkeypatch rather than relying on the module-level +constant directly. + +## pathlib: robust path manipulation + +### Project roots, joins, and ensuring directories + +```python +from __future__ import annotations +from pathlib import Path + +PROJECT_ROOT = Path(__file__).resolve().parents[1] +DIST = PROJECT_ROOT / "dist" +(DIST / "artifacts").mkdir(parents=True, exist_ok=True) + +# Portable joins and normalisation +cfg = PROJECT_ROOT.joinpath("config", "release.toml").resolve() +``` + +### Reading / writing files and atomic updates + +```python +from pathlib import Path +import tempfile + +f = Path("./dist/version.txt") + +# Text I/O +f.write_text("1.2.3\n", encoding="utf-8") +version = f.read_text(encoding="utf-8").strip() + +# Atomic write pattern (tmp → replace) +with tempfile.NamedTemporaryFile("w", delete=False, dir=f.parent, encoding="utf-8") as tmp: + tmp.write("new-contents\n") + tmp_path = Path(tmp.name) + +tmp_path.replace(f) # atomic on POSIX +``` + +### Globbing, filtering, and safe deletion + +```python +from pathlib import Path + +# Recursive glob +md_files = sorted(Path("docs").glob("**/*.md")) + +# Filter by suffix / size +small_md = [p for p in md_files if p.stat().st_size < 4096 and p.suffix == ".md"] + +# Safe deletion (ignore missing) +try: + (Path("build") / "temp.bin").unlink() +except FileNotFoundError: + pass +``` + +## Cyclopts + cuprum + pathlib together (reference script) + +```python +#!/usr/bin/env -S uv run --script +# /// script +# requires-python = ">=3.14" +# dependencies = ["cyclopts>=2.9", "cuprum", "cmd-mox"] +# /// + +from __future__ import annotations +from pathlib import Path +from typing import Optional, Annotated + +import cyclopts +from cyclopts import App, Parameter +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("git") + +app = App(config=cyclopts.config.Env("INPUT_", command=False)) + +@app.default +def main( + *, + bin_name: Annotated[str, Parameter(required=True)], + version: Annotated[str, Parameter(required=True)], + formats: list[str] | None = None, + outdir: Optional[Path] = None, + dry_run: bool = False, +): + project_root = Path(__file__).resolve().parents[1] + dist = (outdir or (project_root / "dist")) / bin_name + dist.mkdir(parents=True, exist_ok=True) + + if not dry_run: + with sh.scoped(CATALOGUE): + git = sh.make("git") + git("tag", f"v{version}", cwd=project_root).run_sync() + + print({ + "bin_name": bin_name, + "version": version, + "formats": formats or [], + "dist": str(dist), + }) + +if __name__ == "__main__": + app() +``` + +## Testing expectations + +- Automated coverage via `pytest` is required for every script. Fixtures from + `pytest-mock` support Python‑level mocking; `cmd-mox` simulates external + executables without touching the host system. +- Behavioural flows that map cleanly to scenarios should adopt Behaviour‑Driven + Development (BDD) via `pytest-bdd` so that intent is captured in + human‑readable Given/When/Then narratives. +- Tests reside in `scripts/tests/`, mirroring script names. For example, + `scripts/bootstrap_doks.py` pairs with `scripts/tests/test_bootstrap_doks.py`. +- Where scripts rely on environment variables, both happy paths and failure + modes must be asserted; tests should demonstrate graceful error handling + rather than opaque stack traces. + +### Mocking Python dependencies (pytest-mock) and environment (monkeypatch) + +```python +import os +from pathlib import Path +from cyclopts.testing import invoke +from scripts.package import app + + +def test_reads_env_and_defaults(monkeypatch, tmp_path): + # Arrange env for Cyclopts + monkeypatch.setenv("INPUT_BIN_NAME", "demo") + monkeypatch.setenv("INPUT_VERSION", "1.2.3") + monkeypatch.setenv("INPUT_FORMATS", "deb rpm") # whitespace or newlines + + # Exercise + result = invoke(app, []) + + # Assert + assert result.exit_code == 0 + assert '"version": "1.2.3"' in result.stdout + + +def test_patch_python_dependency(mocker): + # Example: patch a helper function used by the script + from scripts import helpers + + mocker.patch.object(helpers, "compute_checksum", return_value="deadbeef") + assert helpers.compute_checksum(b"abc") == "deadbeef" +``` + +### Mocking external executables with cmd-mox (record → replay → verify) + +Enable the plugin in `conftest.py`: + +```python +pytest_plugins = ("cmd_mox.pytest_plugin",) +``` + +```python +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("git") + + +def test_git_tag_happy_path(cmd_mox, tmp_path): + + # Mock external command behaviour + cmd_mox.mock("git").with_args("tag", "v1.2.3").returns(exit_code=0) + + # Run the code under test while shims are active + cmd_mox.replay() + with sh.scoped(CATALOGUE): + sh.make("git")("tag", "v1.2.3", cwd=tmp_path).run_sync() + cmd_mox.verify() + + +def test_git_tag_failure_surface_error(cmd_mox, tmp_path): + + cmd_mox.mock("git").with_args("tag", "v1.2.3").returns(exit_code=1, stderr="denied") + + cmd_mox.replay() + with sh.scoped(CATALOGUE): + result = sh.make("git")("tag", "v1.2.3", cwd=tmp_path).run_sync() + assert result.exit_code == 1 + assert "denied" in result.stderr + cmd_mox.verify() +``` + +### Spies and passthrough capture (turn real calls into fixtures) + +```python +from cuprum import Catalogue, sh + +CATALOGUE = Catalogue.from_programs("echo") + + +def test_spy_and_record(cmd_mox, tmp_path): + + # Spy records actual usage; passthrough runs the real command + spy = cmd_mox.spy("echo").passthrough() + + cmd_mox.replay() + with sh.scoped(CATALOGUE): + sh.make("echo")("hello world", cwd=tmp_path).run_sync() + cmd_mox.verify() + + # Inspect what happened + spy.assert_called() + assert spy.call_count == 1 + args = spy.invocations[0].argv[1:] + assert args == ["hello world"] +``` + +## Operational guidelines + +- Scripts must be idempotent. Re‑running should converge state without + destructive side effects. Guard conditions (for example, checking the secrets + manager for existing secrets) should precede writes or rotations. +- Pure functions that accept configuration objects are preferred over global + state so that tests can exercise logic deterministically. +- Exit codes should follow UNIX conventions: `0` for success, non‑zero for + actionable failures. Human‑friendly error messages should highlight + remediation steps. +- Dependencies must remain minimal. Any new package should be added to the `uv` + block and the rationale documented within the script or companion tests. + +## Migration guidance (Typer → Cyclopts) + +1. Dependencies: replace Typer with Cyclopts in the script's `uv` block. +2. Entry point: replace `app = typer.Typer(...)` with `app = App(...)` and + configure `Env("INPUT_", command=False)` where environment variables are + authoritative in CI. +3. Parameters: replace `typer.Option(...)` with annotations and + `Parameter(...)`. Mark required options with `required=True`. Map any + non‑matching environment names via `env_var=...`. +4. Lists: remove custom split/trim code. Use list‑typed parameters; add + `env_var_split=","` where a non‑whitespace delimiter is required. +5. Compatibility: retain legacy flag names using `aliases=["--old-name"]`. +6. Bash glue: delete argument arrays and conditional appends in GitHub + Actions. Export `INPUT_*` environment variables and call `uv run` on the + script. + +## Migration guidance (plumbum → cuprum) + +**Important semantic change:** Plumbum raises `ProcessExecutionError` on +non-zero exit codes by default, whereas Cuprum's `run_sync()` always returns a +`CommandResult` without raising. Code that relied on exception handling for +failure detection must be rewritten to check `result.exit_code` explicitly. +This shift improves predictability but requires careful attention when porting +existing error handling logic. + +1. Dependencies: replace `plumbum` with `cuprum` in `pyproject.toml` or the + script's `uv` block. +2. Define a catalogue: create a `Catalogue.from_programs(...)` listing all + executables the script requires. +3. Scope execution: wrap command construction in `with sh.scoped(CATALOGUE):`. +4. Command construction: replace `local["git"]["args"]` with + `sh.make("git")("args")`. +5. Execution: replace `command()` with `command.run_sync()` and access + `result.stdout`, `result.stderr`, `result.exit_code`. +6. Non‑raising execution: replace `.run(retcode=None)` patterns with + `run_sync()` and check `result.exit_code` explicitly. Note that this is now + the default behaviour, not a special case. +7. Working directory: replace `with local.cwd(path):` context manager with + `cwd=path` parameter on the command. +8. Environment: replace `with local.env(VAR=value):` with `env={"VAR": value}` + parameter on the command. +9. Pipelines: the `|` operator works identically; ensure both commands are + constructed via `sh.make()`. +10. Error handling: replace `CommandNotFound` with cuprum's + `UnknownProgramError`; replace `ProcessExecutionError` with exit code + checks on `CommandResult`. + +## CI wiring: GitHub Actions (Cyclopts‑first) + +```yaml +- name: Build + shell: bash + working-directory: ${{ inputs.project-dir }} + env: + INPUT_BIN_NAME: ${{ inputs.bin-name }} + INPUT_VERSION: ${{ inputs.version }} + INPUT_FORMATS: ${{ inputs.formats }} # multiline or space‑sep + INPUT_OUTDIR: ${{ inputs.outdir }} + run: | + set -euo pipefail + uv run "${GITHUB_ACTION_PATH}/scripts/package.py" +``` + +## Notes and gotchas + +- Newline‑separated lists are preferred for CI inputs to avoid shell quoting + issues across platforms. +- Cuprum's `run_sync()` always returns a `CommandResult`; check `exit_code` + explicitly rather than relying on exceptions for non‑zero exits. +- Production code should present friendly error messages; tests may assert raw + behaviours (non‑zero exits, stderr contents) via `cmd-mox`. +- On Windows, newline‑separated lists are recommended for `list[Path]` to + sidestep `;`/`:` semantics. +- Cuprum's catalogue must include all programs used by the script; attempting + to construct a command for an unregistered program raises + `UnknownProgramError`. + +This document should be referenced when introducing or updating automation +scripts to maintain a consistent developer experience across the repository. diff --git a/docs/users-guide.md b/docs/users-guide.md index 72dbd2c44..91a9bdd5c 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -11,7 +11,7 @@ may change before 1.0. Pin the Netsuke version in automated workflows. ## Install Netsuke Netsuke requires [Ninja](https://ninja-build.org/) on `PATH`. A source build -also requires the dated Rust nightly toolchain pinned in `rust-toolchain.toml`, +also requires the dated Rust nightly toolchain pinned in `rust-toolchain.toml` because Netsuke builds with the Polonius borrow checker, which nightly enables by default. @@ -540,22 +540,27 @@ expansion per matching source. Matching is case-sensitive. `*` and `?` do not cross directory separators; use `**` to descend into subdirectories. Directories are excluded, so only files -are returned. The [quick-start guide](quickstart.md) shows a complete runnable -example. - -Patterns may be absolute or relative to the working directory, including -parent-relative patterns such as `glob('../shared/*.h')`. Expansion is scoped -to the pattern's longest literal directory prefix — the text up to the first -`*`, `?`, `[` or `{`, trimmed back to the last separator, so `src/` for -`src/**/*.c`. If that prefix does not exist, or names something that is not a -directory, the call returns an empty list rather than failing. A symbolic-link -literal prefix, such as `src/link/*.c`, cannot establish the capability and -causes expansion to fail. A match is skipped rather than reported as an error -when the metadata lookup cannot resolve a symbolic link — the match itself or a -directory reached on the way to it — because it is unreadable within the -prefix, dangling, or resolves outside that prefix. A cyclic symbolic link is -reported as an error rather than skipped, since it describes a broken tree -rather than a missing file. +are returned. Relative patterns resolve against the directory containing the +manifest — the workspace root — independent of the directory Netsuke is invoked +from, so `glob('src/*.c')` in a `Netsukefile` at the project root matches +`/src/*.c`. The [quick-start guide](quickstart.md) shows a complete +runnable example. + +Patterns may be absolute or relative to the manifest directory, including +parent-relative patterns such as `glob('../shared/*.h')`. Relative results +retain their pattern-relative spelling after Netsuke removes the workspace +base; absolute patterns remain absolute. Expansion is scoped to the pattern's +longest literal directory prefix — the text up to the first `*`, `?`, `[` or +`{`, trimmed back to the last separator, so `src/` for `src/**/*.c`. If that +prefix does not exist, or names something that is not a directory, the call +returns an empty list rather than failing. A symbolic-link literal prefix, such +as `src/link/*.c`, cannot establish the capability and causes expansion to +fail. A match is skipped rather than reported as an error when the metadata +lookup cannot resolve a symbolic link — the match itself or a directory reached +on the way to it — because it is unreadable within the prefix, dangling, or +resolves outside that prefix. A cyclic symbolic link is reported as an error +rather than skipped, since it describes a broken tree rather than a missing +file. Patterns with unmatched braces are rejected during validation. When an opening brace remains unclosed, the diagnostic points to the outermost unmatched @@ -570,6 +575,11 @@ from becoming shell syntax when `item` is interpolated into a `command` or validation; each caller must validate or escape matched paths before passing them to a command sink. +Rust callers use `manifest::glob_paths(pattern, base)` with an optional base. +`Some(&Utf8Path)` anchors relative patterns and strips that base from results; +absolute patterns ignore the base, while `None` resolves relative patterns +against the process working directory. + ### Define reusable macros Macros return rendered text and can accept default arguments: @@ -950,8 +960,11 @@ relative output paths: netsuke --directory /path/to/project build ``` -An explicit `--config` path remains relative to the shell's original working -directory. +`--directory` affects manifest lookup, automatic project-configuration +discovery, and relative output paths. It does not rebase an explicit `--config` +path or `NETSUKE_CONFIG` value: a relative selector resolves from the process +working directory, while an absolute selector remains unchanged. Pass an +absolute path when the selector must not depend on the invoking directory. ### Generate and inspect artefacts diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index ecc17b005..3e77d4ec5 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -84,6 +84,14 @@ The Rust `manifest::glob_paths` query retains its previous contract and continues to return any matching UTF-8 file path because its callers own their downstream escaping boundary. +Relative manifest glob patterns, including parent-relative patterns, now +resolve from the manifest directory or workspace root rather than the process +working directory. Relative results retain their pattern-relative spelling +after the workspace base is stripped; absolute patterns remain absolute. The +manifest parse boundary supplies this base, so glob expansion does not read or +mutate process-global working-directory state. Callers of the Rust +`glob_paths(pattern, base)` API can supply the same base explicitly. + ## Policy enum parsing The public policy enums no longer implement `clap::ValueEnum`. This removes the diff --git a/src/cli/discovery.rs b/src/cli/discovery.rs index c96b650f5..8e63e0d1f 100644 --- a/src/cli/discovery.rs +++ b/src/cli/discovery.rs @@ -3,11 +3,10 @@ //! This module locates `OrthoConfig` file layers by scanning for config files //! through [`ConfigDiscovery`], handling explicit paths from CLI flags and //! environment variables, and loading TOML chains into [`MergeLayer`] values. - use ortho_config::{MapEnv, MergeLayer, OrthoResult, SharedEnvSource, load_config_file_as_chain}; use std::borrow::Cow; use std::io; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::sync::Arc; use super::command::Cli; @@ -24,6 +23,9 @@ mod json; mod layers; #[path = "discovery_paths.rs"] mod paths; + +#[path = "discovery_selector.rs"] +mod selector; #[path = "discovery_trace.rs"] mod trace; @@ -32,6 +34,12 @@ mod telemetry; use diagnostics::{BoundedConfigPath, ConfigLoadFailureKind, ConfigLoadWarning}; use layers::collect_file_layers_with_normalizer_and_trace; use paths::{FsPathNormalizer, PathNormalizer}; +#[cfg(test)] +use selector::{ + ConfigPathResolution, env_config_path, explicit_config_path_with_env, resolve_config_selector, +}; +#[cfg(test)] +use std::path::PathBuf; /// Record the discovery series for an already-timed phase at the boundary. pub use telemetry::record_discovery_outcome; use trace::{DiscoveryDiagnostics, DiscoveryTrace, FileLayerTrace}; @@ -179,7 +187,7 @@ fn collect_file_layers_with_env( Option, OrthoResult>>, ) { - let resolution = resolve_config_selector(cli.config.clone(), env); + let resolution = selector::resolve_config_selector(cli.config.clone(), env); let (file_layers, load_warning, outcome) = resolution.path.as_deref().map_or_else( || { let (project_scope, outcome) = collect_file_layers_with_normalizer_and_trace( @@ -190,6 +198,10 @@ fn collect_file_layers_with_env( (FileLayerTrace::Automatic { project_scope }, None, outcome) }, |path| { + // Explicit selectors are independent of `-C/--directory`. + // Relative paths retain their selector spelling and resolve + // against the process working directory at load time; absolute + // paths remain unchanged. let (load_warning, outcome) = load_layers_from_path_with_warning(path); ( FileLayerTrace::Explicit { @@ -221,68 +233,6 @@ pub(crate) fn discovery_env_source(env: &impl EnvProvider) -> SharedEnvSource { } Arc::new(source) } - -/// Select an explicit config path, giving `--config` precedence over `env`. -/// -/// A thin wrapper over [`resolve_config_selector`] for callers that need only -/// the winning path. Like that query it performs no tracing; discovery returns -/// bounded diagnostics for composition boundaries to emit later. -/// -/// Production code takes the richer [`ConfigPathResolution`] so it can trace the -/// environment lookups, leaving this as a convenience for precedence tests. -#[cfg(test)] -pub(crate) fn explicit_config_path_with_env(cli: &Cli, env: &impl EnvProvider) -> Option { - resolve_config_selector(cli.config.clone(), env).path -} - -/// Describes the result of the pure explicit-path selection query. -/// -/// Records the winning selector, its optional path, and every environment -/// lookup evaluated to reach the decision, so a caller can emit diagnostics -/// afterwards without giving the query tracing side effects. -#[derive(Debug, PartialEq, Eq)] -struct ConfigPathResolution { - /// Configuration selector that resolved the path. - selector: &'static str, - /// Bounded resolved configuration path, or `None` when unset. - path: Option, - /// Environment variables consulted during resolution, with their results. - environment_lookups: Vec<(&'static str, Option)>, -} - -/// Select a config path from the CLI flag, then `NETSUKE_CONFIG` via `env`. -/// -/// `cli_config` wins when present, in which case no environment lookup is -/// recorded because none is performed. This query emits no tracing. -fn resolve_config_selector( - cli_config: Option, - env: &impl EnvProvider, -) -> ConfigPathResolution { - if let Some(path) = cli_config { - return ConfigPathResolution { - selector: "cli_flag", - path: Some(path), - environment_lookups: Vec::new(), - }; - } - - let primary_path = env_config_path(env, CONFIG_ENV_VAR); - ConfigPathResolution { - selector: primary_path.as_ref().map_or("none", |_| CONFIG_ENV_VAR), - environment_lookups: vec![(CONFIG_ENV_VAR, primary_path.clone())], - path: primary_path, - } -} -/// Read a non-empty config path from `var_name` through `env`. -/// -/// Returns `None` when the variable is unset or empty, so discovery still runs. -/// This query emits no tracing. -fn env_config_path(env: &impl EnvProvider, var_name: &str) -> Option { - env.get(var_name) - .filter(|value| !value.is_empty()) - .map(PathBuf::from) -} - /// Load the configuration chain rooted at an explicit file path. /// /// Unlike discovery, a missing explicit file is an error because the caller @@ -361,12 +311,11 @@ mod helper_proptests; #[path = "discovery_layer_replay_tests.rs"] mod layer_replay_tests; #[cfg(test)] +#[path = "discovery_layer_selector_tests.rs"] +mod layer_selector_tests; +#[cfg(test)] #[path = "discovery_layer_tests.rs"] mod layer_tests; -#[cfg(test)] -#[path = "discovery_path_selection_tests.rs"] -mod path_selection_tests; - #[cfg(test)] #[path = "discovery_replay_proptests.rs"] mod replay_proptests; diff --git a/src/cli/discovery_layer_selector_tests.rs b/src/cli/discovery_layer_selector_tests.rs new file mode 100644 index 000000000..76fafeaaa --- /dev/null +++ b/src/cli/discovery_layer_selector_tests.rs @@ -0,0 +1,140 @@ +//! Tests for explicit configuration-selector independence from `-C`. +//! +//! A relative `--config` or `NETSUKE_CONFIG` selector resolves from the +//! process working directory even when `-C/--directory` is supplied; an +//! absolute selector is always used unchanged. End-to-end tests place a +//! decoy at the `-C` location to prove the selector is not rebased. +use super::paths::{FsPathNormalizer, normalized_path_key}; +use super::*; +use crate::cli::test_support::TestEnv; +use anyhow::{Context, Result, ensure}; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +/// Assert that `discovered` loaded exactly `expected_path` and nothing else. +fn assert_single_layer( + discovered: &DiscoveryOutcome, + expected_path: &std::path::Path, +) -> Result<()> { + ensure!( + discovered.first_error().is_none(), + "the explicit selector should load: {:?}", + discovered.first_error() + ); + let paths = discovered + .layers() + .iter() + .filter_map(|layer| layer.path().map(|path| path.as_str().to_owned())) + .collect::>(); + let expected = normalized_path_key(&FsPathNormalizer, &expected_path.to_string_lossy()) + .context("canonicalise the expected selector path")? + .to_string_lossy() + .into_owned(); + assert_eq!(paths, vec![expected]); + Ok(()) +} + +/// An absolute `--config` selector is used unchanged, even with `-C`. +/// +/// `-C/--directory` never re-anchors an absolute selector: the operator +/// pointed at an exact file. A decoy at the `-C`-joined path with different +/// content proves that rebasing an absolute selector would be caught. +#[test] +fn explicit_absolute_config_ignores_cli_directory() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let selector = temp.path().join("selector.toml"); + test_support::fs::write(&selector, "theme = \"ascii\"\n").context("write selector config")?; + let cli_directory = temp.path().join("cli-dir"); + test_support::fs::create_dir(&cli_directory).context("create -C directory")?; + // A decoy at the `-C`-joined path: if the selector were anchored to `-C`, + // this is what would actually load instead of `selector`. + test_support::fs::write(cli_directory.join("selector.toml"), "theme = \"dark\"\n") + .context("write -C decoy config")?; + + let cli = Cli { + config: Some(selector.clone()), + directory: Some(cli_directory), + ..Cli::default() + }; + let discovered = discover_file_layers(&cli, &TestEnv::default()); + + assert_single_layer(&discovered, &selector) +} + +/// A relative `--config` selector keeps its process-working-directory spelling. +#[test] +fn explicit_relative_config_ignores_cli_directory() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli_directory = temp.path().join("cli-dir"); + test_support::fs::create_dir(&cli_directory).context("create -C directory")?; + + let cli = Cli { + config: Some(PathBuf::from("relative.toml")), + directory: Some(cli_directory), + ..Cli::default() + }; + + assert_eq!( + explicit_config_path_with_env(&cli, &TestEnv::default()), + Some(PathBuf::from("relative.toml")) + ); + Ok(()) +} + +/// A relative `--config` selector without `-C` is returned unchanged. +/// +/// The pure query keeps the spelling as written; at load time it then +/// resolves against the process working directory, which the end-to-end +/// binary coverage in `tests/config_discovery_e2e_tests.rs` proves. +#[test] +fn explicit_relative_config_without_directory_stays_as_written() { + let cli = Cli { + config: Some(PathBuf::from("relative.toml")), + ..Cli::default() + }; + assert_eq!( + explicit_config_path_with_env(&cli, &TestEnv::default()), + Some(PathBuf::from("relative.toml")) + ); +} + +/// `--config` keeps precedence over `NETSUKE_CONFIG` regardless of `-C`. +#[test] +fn cli_selector_wins_over_environment_with_directory() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli_directory = temp.path().join("cli-dir"); + test_support::fs::create_dir(&cli_directory).context("create -C directory")?; + + let cli = Cli { + config: Some(PathBuf::from("cli.toml")), + directory: Some(cli_directory), + ..Cli::default() + }; + let env = TestEnv::default().with_var(CONFIG_ENV_VAR, "env.toml"); + + assert_eq!( + explicit_config_path_with_env(&cli, &env), + Some(PathBuf::from("cli.toml")) + ); + Ok(()) +} + +/// A relative `NETSUKE_CONFIG` selector also ignores `-C`. +#[test] +fn env_config_selector_ignores_cli_directory() -> Result<()> { + let temp = tempdir().context("create temp dir")?; + let cli_directory = temp.path().join("cli-dir"); + test_support::fs::create_dir(&cli_directory).context("create -C directory")?; + + let cli = Cli { + directory: Some(cli_directory), + ..Cli::default() + }; + let env = TestEnv::default().with_var(CONFIG_ENV_VAR, "env-selector.toml"); + + assert_eq!( + explicit_config_path_with_env(&cli, &env), + Some(PathBuf::from("env-selector.toml")) + ); + Ok(()) +} diff --git a/src/cli/discovery_path_selection_tests.rs b/src/cli/discovery_path_selection_tests.rs deleted file mode 100644 index 3f5dd5461..000000000 --- a/src/cli/discovery_path_selection_tests.rs +++ /dev/null @@ -1,42 +0,0 @@ -//! Tests for explicit configuration-path selection. -//! -//! These tests keep explicit relative selectors anchored to the process CWD -//! instead of rebasing them beneath the CLI directory. - -use super::*; -use crate::cli::test_support::TestEnv; -use anyhow::{Context, Result, ensure}; -use tempfile::tempdir; - -/// An explicit relative configuration file does not use the CLI directory. -#[test] -fn explicit_relative_config_does_not_use_cli_directory() -> Result<()> { - let temp = tempdir().context("create temp dir")?; - let unique_dir_name = temp - .path() - .file_name() - .and_then(std::ffi::OsStr::to_str) - .context("temporary directory has a UTF-8 name")?; - let config_name = format!("{unique_dir_name}-relative-config.toml"); - let config_path = temp.path().join(&config_name); - test_support::fs::write(&config_path, "emoji = \"always\"\n") - .context("write explicit config")?; - let cli = Cli { - config: Some(config_name.into()), - directory: Some(temp.path().to_path_buf()), - ..Cli::default() - }; - - let discovered = discover_file_layers(&cli, &TestEnv::default()); - let error = discovered - .first_error() - .context("relative explicit config must not load from the CLI directory")?; - - ensure!( - error - .to_string() - .contains("explicit configuration file not found"), - "expected missing explicit config error, got {error}" - ); - Ok(()) -} diff --git a/src/cli/discovery_selector.rs b/src/cli/discovery_selector.rs new file mode 100644 index 000000000..a051a8087 --- /dev/null +++ b/src/cli/discovery_selector.rs @@ -0,0 +1,82 @@ +//! Explicit configuration-selector resolution. +//! +//! The selector query answers one question: which configuration file did the +//! operator select, and at what path? `--config` outranks `NETSUKE_CONFIG`. +//! The selected path remains as written: absolute paths stay absolute, and +//! relative paths resolve from the process working directory at load time. +//! `-C/--directory` affects automatic discovery, not explicit selection. +//! The query is pure and emits no tracing; discovery traces the result later. + +use std::path::PathBuf; + +use super::{CONFIG_ENV_VAR, environment::EnvProvider}; + +/// Describes the result of the pure explicit-path selection query. +/// +/// Records the winning selector, its optional path, and every environment +/// lookup evaluated to reach the decision, so a caller can emit diagnostics +/// afterwards without giving the query tracing side effects. +#[derive(Debug, PartialEq, Eq)] +pub(super) struct ConfigPathResolution { + /// Configuration selector that resolved the path. + pub(super) selector: &'static str, + /// Bounded resolved configuration path, or `None` when unset. + pub(super) path: Option, + /// Environment variables consulted during resolution, with their results. + pub(super) environment_lookups: Vec<(&'static str, Option)>, +} + +/// Select an explicit config path, giving `--config` precedence over `env`. +/// +/// A thin wrapper over [`resolve_config_selector`] for callers that +/// need only the winning path. Like that query it performs no tracing; +/// discovery returns bounded diagnostics for composition boundaries to emit +/// later. +/// +/// Production code takes the richer [`ConfigPathResolution`] so it can trace +/// the environment lookups, leaving this as a convenience for precedence +/// tests. +#[cfg(test)] +pub(super) fn explicit_config_path_with_env( + cli: &super::Cli, + env: &impl EnvProvider, +) -> Option { + resolve_config_selector(cli.config.clone(), env).path +} + +/// Select a config path from the CLI flag, then `NETSUKE_CONFIG` via `env`, +/// independently of `-C/--directory`. +/// +/// `cli_config` wins when present, in which case no environment lookup is +/// recorded because none is performed. The winning path is used exactly as +/// selected: absolute paths remain unchanged and relative paths resolve from +/// the process working directory when loaded. This query emits no tracing. +pub(super) fn resolve_config_selector( + cli_config: Option, + env: &impl EnvProvider, +) -> ConfigPathResolution { + if let Some(path) = cli_config { + return ConfigPathResolution { + selector: "cli_flag", + path: Some(path), + environment_lookups: Vec::new(), + }; + } + + let primary_path = env_config_path(env, CONFIG_ENV_VAR); + ConfigPathResolution { + selector: primary_path.as_ref().map_or("none", |_| CONFIG_ENV_VAR), + environment_lookups: vec![(CONFIG_ENV_VAR, primary_path.clone())], + path: primary_path, + } +} + +/// Read a non-empty config path from `var_name` through `env`. +/// +/// Returns `None` when the variable is unset or empty, so discovery still runs. +/// This query emits no tracing. +pub(super) fn env_config_path(env: &impl EnvProvider, var_name: &str) -> Option { + env.get(var_name) + .filter(|value| !value.is_empty()) + .map(PathBuf::from) +} diff --git a/src/cli/discovery_trace.rs b/src/cli/discovery_trace.rs index 9636e3eaa..e78f2d52a 100644 --- a/src/cli/discovery_trace.rs +++ b/src/cli/discovery_trace.rs @@ -6,12 +6,12 @@ use tracing::debug; -use super::ConfigPathResolution; use super::diagnostics::{ BoundedConfigPath, ConfigLoadWarning, debug_config_path_from_fields, trace_config_path_variable_from_fields, }; use super::layers::ProjectScopeTrace; +use super::selector::ConfigPathResolution; /// Bounded selector and layer-branch diagnostics retained after discovery. #[derive(Clone, Debug)] diff --git a/src/manifest/glob/base.rs b/src/manifest/glob/base.rs new file mode 100644 index 000000000..3b16bbff4 --- /dev/null +++ b/src/manifest/glob/base.rs @@ -0,0 +1,256 @@ +//! Canonicalize injected bases before relative glob compilation. +//! +//! This module owns only the filesystem-to-path conversion at the injected +//! base seam. Pattern preparation owns joining and escaping the resulting +//! path, while the walker owns opening its literal prefix. + +use super::{ + GlobPattern, diagnostics, + errors::{GlobErrorContext, GlobErrorType, create_glob_error}, + escape::escape_glob_literal_path, +}; +use camino::{Utf8Path, Utf8PathBuf}; +use minijinja::Error; +use std::{ + sync::{Mutex, MutexGuard}, + time::Instant, +}; + +/// Resolve an injected base for a relative pattern to a canonical UTF-8 path. +/// +/// A workspace reached through a symbolic link must still expand relative +/// globs. `dunce` retains canonicalization while simplifying safe Windows +/// verbatim disk prefixes, which the `glob` crate deliberately does not +/// enumerate. +/// +/// # Errors +/// +/// Propagates canonicalization and UTF-8 conversion failures as +/// [`GlobErrorType::IoError`]. +pub(super) fn resolve_relative_glob_base( + base: &Utf8Path, +) -> std::result::Result { + resolve_relative_glob_base_for_template(base).map_err(super::GlobExpansionFailure::into_error) +} + +/// Resolve an injected base while retaining a bounded preparation failure. +fn resolve_relative_glob_base_for_template( + base: &Utf8Path, +) -> std::result::Result { + let canonical = dunce::canonicalize(base.as_std_path()).map_err(|error| { + super::GlobExpansionFailure::BaseCanonicalization(create_base_error( + base, + error.to_string(), + )) + })?; + Utf8PathBuf::from_path_buf(canonical).map_err(|path| { + super::GlobExpansionFailure::Utf8Conversion(create_base_error( + base, + format!("canonical base path is not valid UTF-8: {}", path.display()), + )) + }) +} + +/// Build a glob I/O error describing an unusable injected base. +fn create_base_error(base: &Utf8Path, detail: String) -> Error { + create_glob_error( + &GlobErrorContext { + pattern: base.to_string(), + error_char: char::from(0), + position: 0, + error_type: GlobErrorType::IoError, + }, + Some(detail), + ) +} + +/// Cache a manifest parse's injected glob base after its first relative use. +/// +/// This type belongs only to the manifest parse boundary. Direct +/// [`super::glob_paths`] callers continue to provide their optional base per +/// query; Jinja's closure owns one cache so multiple relative `glob()` calls +/// do not repeat filesystem canonicalization. +pub(in crate::manifest) struct GlobBaseCache { + /// Base supplied by the manifest workspace, before filesystem preparation. + base: Option, + /// Successfully canonicalized base retained for the rest of the parse. + resolved: Mutex>, +} + +impl GlobBaseCache { + /// Create an empty cache around an optional manifest workspace base. + pub(in crate::manifest) const fn new(base: Option) -> Self { + Self { + base, + resolved: Mutex::new(None), + } + } + + /// Classify a pattern and parse context for template expansion telemetry. + /// + /// The returned values are the closed `base_mode` label set used only by + /// manifest-template expansion diagnostics. Absolute patterns bypass the + /// configured base and therefore remain distinct from relative patterns. + pub(super) fn mode(&self, pattern: &Utf8Path) -> &'static str { + if pattern.is_absolute() { + "absolute_pattern" + } else if self.base.is_some() { + "relative_with_base" + } else { + "relative_without_base" + } + } + + /// Resolve and retain the configured base when one is available. + /// + /// # Errors + /// + /// Returns the canonicalization error from the injected base on its first + /// relative use. + fn resolve(&self) -> std::result::Result, super::GlobExpansionFailure> { + let _span = tracing::debug_span!("manifest.glob_base", operation = "resolve").entered(); + let Some(base) = self.base.as_deref() else { + diagnostics::record_base_cache_bypass(); + return Ok(None); + }; + let cached = self + .lock_resolved(base) + .map_err(super::GlobExpansionFailure::BaseCanonicalization)? + .clone(); + if cached.is_some() { + diagnostics::record_base_cache_hit(); + return Ok(cached); + } + + let started = Instant::now(); + let canonical = match resolve_relative_glob_base_for_template(base) { + Ok(canonical) => { + diagnostics::record_base_cache_miss(started.elapsed()); + canonical + } + Err(error) => { + diagnostics::record_base_cache_error(started.elapsed()); + return Err(error); + } + }; + let mut resolved = self + .lock_resolved(base) + .map_err(super::GlobExpansionFailure::BaseCanonicalization)?; + if let Some(published) = resolved.as_ref() { + return Ok(Some(published.clone())); + } + resolved.replace(canonical.clone()); + Ok(Some(canonical)) + } + + /// Lock the resolved-base cache and preserve a contextual poisoning error. + fn lock_resolved( + &self, + base: &Utf8Path, + ) -> std::result::Result>, Error> { + self.resolved.lock().map_err(|error| { + create_glob_error( + &GlobErrorContext { + pattern: base.to_string(), + error_char: char::from(0), + position: 0, + error_type: GlobErrorType::IoError, + }, + Some(format!("manifest glob-base cache lock poisoned: {error}")), + ) + }) + } +} + +/// Hold a validated glob pattern, search text, and optional rebase path. +/// +/// Pattern preparation lives beside base resolution because both constructors +/// decide whether a relative pattern needs a filesystem-prepared base. +pub(super) struct PreparedGlob { + /// Validated pattern and its normalised spelling. + pub(super) pattern: GlobPattern, + /// Search text handed to `glob_with`, owned only when it embeds a base. + search: Option, + /// Canonicalized base stripped from matches, present exactly when relative. + pub(super) strip: Option, +} + +impl PreparedGlob { + /// Prepare `pattern` and any relative injected `base` for filesystem matching. + /// + /// Canonicalizes a relative injected base through + /// [`resolve_relative_glob_base`] before embedding its escaped literal + /// spelling into the search text. + /// + /// # Errors + /// + /// Returns an error when the pattern fails brace validation or when the + /// injected base cannot be canonicalized. + pub(super) fn new(pattern: &str, base: Option<&Utf8Path>) -> std::result::Result { + let pattern_state = GlobPattern::new(pattern)?; + let normalized = pattern_state.normalized(); + let resolved_base = match base { + Some(dir) if !Utf8Path::new(normalized).is_absolute() => { + Some(resolve_relative_glob_base(dir)?) + } + _ => None, + }; + Ok(Self::from_pattern_and_base(pattern_state, resolved_base)) + } + + /// Prepare `pattern` using a manifest parse's cached injected base. + /// + /// # Errors + /// + /// Returns an error when the pattern fails brace validation or its first + /// relative use cannot canonicalize the injected base. + #[cfg(test)] + pub(super) fn new_with_base_cache( + pattern: &str, + base: &GlobBaseCache, + ) -> std::result::Result { + Self::new_with_base_cache_for_template(pattern, base) + .map_err(super::GlobExpansionFailure::into_error) + } + + /// Prepare `pattern` while retaining a bounded template failure outcome. + pub(super) fn new_with_base_cache_for_template( + pattern: &str, + base: &GlobBaseCache, + ) -> std::result::Result { + let pattern_state = + GlobPattern::new(pattern).map_err(super::GlobExpansionFailure::InvalidPattern)?; + let resolved_base = (!Utf8Path::new(pattern_state.normalized()).is_absolute()) + .then(|| base.resolve()) + .transpose()? + .flatten(); + Ok(Self::from_pattern_and_base(pattern_state, resolved_base)) + } + + /// Build glob search text from a validated pattern and resolved base. + fn from_pattern_and_base(pattern: GlobPattern, base: Option) -> Self { + let (search, strip) = base.map_or_else( + || (None, None), + |dir| { + let escaped = escape_glob_literal_path(&dir); + let separator = std::path::MAIN_SEPARATOR; + ( + Some(format!("{escaped}{separator}{}", pattern.normalized())), + Some(dir), + ) + }, + ); + Self { + pattern, + search, + strip, + } + } + + /// Borrow the compiled search, reusing the normalized pattern when unbased. + pub(super) fn search(&self) -> &str { + self.search + .as_deref() + .unwrap_or_else(|| self.pattern.normalized()) + } +} diff --git a/src/manifest/glob/diagnostics.rs b/src/manifest/glob/diagnostics.rs index c706b61cd..da855541d 100644 --- a/src/manifest/glob/diagnostics.rs +++ b/src/manifest/glob/diagnostics.rs @@ -4,7 +4,9 @@ //! erroneous, so neither reaches the top-level diagnostics: a literal prefix //! that names no directory, and a match that the capability cannot resolve //! because a symbolic link escapes the prefix. Both are recorded here so a -//! degraded expansion is visible without having to reproduce it. +//! degraded expansion is visible without having to reproduce it. The module +//! also records the manifest-scoped injected-base cache so canonicalization +//! cost and cache outcomes remain visible. //! The Jinja adapter also records paths rejected at its shell-safety boundary. //! //! What is recorded is deliberately bounded and redacted. @@ -21,9 +23,11 @@ //! with the same redaction, so its relative form cannot disclose a filename //! selected by the pattern. -use super::{GlobExpansion, GlobOutcome, GlobSkippedEntries}; -use metrics::{counter, describe_counter}; -use std::sync::Once; +use super::{GlobBaseCache, GlobExpansion, GlobExpansionFailure, GlobOutcome, GlobSkippedEntries}; +use camino::Utf8Path; +use metrics::{counter, describe_counter, describe_histogram, histogram}; +use minijinja::Error; +use std::{sync::Once, time::Duration}; /// Metric name counting glob expansions by outcome. const EXPANSIONS_TOTAL: &str = "netsuke_manifest_glob_expansions_total"; @@ -31,6 +35,16 @@ const EXPANSIONS_TOTAL: &str = "netsuke_manifest_glob_expansions_total"; const ENTRIES_SKIPPED_TOTAL: &str = "netsuke_manifest_glob_entries_skipped_total"; /// Metric name counting paths rejected by the Jinja glob adapter. const REJECTIONS_TOTAL: &str = "netsuke_manifest_glob_rejections_total"; +/// Metric name counting injected-base cache outcomes. +const BASE_CACHE_TOTAL: &str = "netsuke_manifest_glob_base_cache_total"; +/// Metric name recording injected-base canonicalization latency. +const BASE_CANONICALIZATION_DURATION: &str = + "netsuke_manifest_glob_base_canonicalization_duration_seconds"; +/// Metric name counting manifest-template glob expansion results. +const TEMPLATE_EXPANSIONS_TOTAL: &str = "netsuke_manifest_template_glob_expansions_total"; +/// Metric name recording end-to-end manifest-template glob expansion latency. +const TEMPLATE_EXPANSION_DURATION: &str = + "netsuke_manifest_template_glob_expansion_duration_seconds"; /// Stable marker replacing caller-controlled paths in tracing events. const REDACTED_PATH: &str = ""; @@ -55,9 +69,162 @@ fn describe_metrics() { "Counts paths rejected by the Jinja glob adapter, labelled by a \ bounded outcome and error category." ); + describe_counter!( + BASE_CACHE_TOTAL, + "Counts injected manifest glob-base cache outcomes labelled by \ + outcome: bypass, hit, miss, or error." + ); + describe_histogram!( + BASE_CANONICALIZATION_DURATION, + "Records the duration in seconds of injected manifest glob-base \ + canonicalization." + ); + describe_counter!( + TEMPLATE_EXPANSIONS_TOTAL, + "Counts manifest-template glob expansion results labelled by \ + base_mode (absolute_pattern, relative_without_base, or \ + relative_with_base) and outcome (matched, unopenable_prefix, \ + invalid_pattern, base_canonicalization_failure, \ + utf8_conversion_failure, capability_root_io_failure, or \ + glob_entry_processing_failure)." + ); + describe_histogram!( + TEMPLATE_EXPANSION_DURATION, + "Records the end-to-end duration in seconds of manifest-template \ + glob expansion." + ); }); } +/// Record a relative glob that has no injected manifest base to prepare. +pub(super) fn record_base_cache_bypass() { + describe_metrics(); + counter!(BASE_CACHE_TOTAL, "outcome" => "bypass").increment(1); + tracing::debug!( + operation = "glob_base_cache", + outcome = "bypass", + "manifest glob base preparation bypassed" + ); +} + +/// Record a relative glob that reuses a canonicalized manifest base. +pub(super) fn record_base_cache_hit() { + describe_metrics(); + counter!(BASE_CACHE_TOTAL, "outcome" => "hit").increment(1); + tracing::debug!( + operation = "glob_base_cache", + outcome = "hit", + "manifest glob base cache hit" + ); +} + +/// Record a successful first canonicalization of the manifest base. +pub(super) fn record_base_cache_miss(duration: Duration) { + record_base_cache_canonicalization("miss", duration); + tracing::debug!( + operation = "glob_base_cache", + outcome = "miss", + "manifest glob base canonicalized and cached" + ); +} + +/// Record a failed canonicalization of the manifest base. +pub(super) fn record_base_cache_error(duration: Duration) { + record_base_cache_canonicalization("error", duration); + tracing::debug!( + operation = "glob_base_cache", + outcome = "error", + error_category = "base_resolution", + "manifest glob base preparation failed" + ); +} + +/// Record the metric-only observations for one base canonicalization. +fn record_base_cache_canonicalization(outcome: &'static str, duration: Duration) { + describe_metrics(); + counter!(BASE_CACHE_TOTAL, "outcome" => outcome).increment(1); + record_base_canonicalization_duration(duration); +} + +/// Record the elapsed duration of one injected-base canonicalization. +fn record_base_canonicalization_duration(duration: Duration) { + histogram!(BASE_CANONICALIZATION_DURATION).record(duration.as_secs_f64()); +} + +/// Expand and observe a manifest-template glob without instrumenting queries. +/// +/// This adapter is the only expansion path that emits whole-operation +/// telemetry. Direct [`super::glob_paths`] callers remain pure so library +/// users can query the filesystem without installing observability backends. +pub(in crate::manifest) fn expand_manifest_template_glob( + pattern: &str, + base: &GlobBaseCache, +) -> std::result::Result { + let normalized = super::normalize::normalize_separators(pattern); + let base_mode = base.mode(Utf8Path::new(&normalized)); + let span = tracing::debug_span!( + "manifest.template_glob", + operation = "expand", + base_mode, + outcome = tracing::field::Empty, + ); + let _guard = span.enter(); + let started = std::time::Instant::now(); + let result = super::expand_glob_with_base_cache(pattern, base); + let outcome = record_template_expansion(&result, started.elapsed(), base_mode); + span.record("outcome", outcome); + result.map_err(GlobExpansionFailure::into_error) +} + +/// Record one completed or failed manifest-template glob expansion. +fn record_template_expansion( + result: &std::result::Result, + duration: Duration, + base_mode: &'static str, +) -> &'static str { + describe_metrics(); + histogram!(TEMPLATE_EXPANSION_DURATION).record(duration.as_secs_f64()); + match result { + Ok(expansion) => { + record(expansion); + let outcome = match expansion.outcome { + GlobOutcome::Matched => "matched", + GlobOutcome::UnopenablePrefix => "unopenable_prefix", + }; + counter!( + TEMPLATE_EXPANSIONS_TOTAL, + "base_mode" => base_mode, + "outcome" => outcome + ) + .increment(1); + tracing::debug!( + operation = "manifest_template_glob_expansion", + base_mode, + outcome, + "manifest template glob expansion completed" + ); + outcome + } + Err(failure) => { + let outcome = failure.outcome(); + counter!( + TEMPLATE_EXPANSIONS_TOTAL, + "base_mode" => base_mode, + "outcome" => outcome + ) + .increment(1); + tracing::debug!( + operation = "manifest_template_glob_expansion", + base_mode, + outcome, + error_category = "expansion_failure", + "manifest template glob expansion failed" + ); + outcome + } + } +} + /// Record a path rejected by the manifest-template shell-safety adapter. pub(super) fn record_template_path_rejection() { describe_metrics(); diff --git a/src/manifest/glob/errors.rs b/src/manifest/glob/errors.rs index d70d82014..79a90a5bc 100644 --- a/src/manifest/glob/errors.rs +++ b/src/manifest/glob/errors.rs @@ -3,6 +3,48 @@ use minijinja::{Error, ErrorKind}; use crate::localization::{self, keys}; +/// Classify a manifest-template glob failure before it becomes a render error. +/// +/// The variants are deliberately the closed `outcome` label set emitted by the +/// manifest-template telemetry boundary. They retain the existing rendered +/// error so direct callers continue to receive the same diagnostic detail. +pub(super) enum GlobExpansionFailure { + /// A pattern was rejected before or during glob compilation. + InvalidPattern(Error), + /// Resolving an injected base could not canonicalize its filesystem path. + BaseCanonicalization(Error), + /// A resolved filesystem path could not be represented as UTF-8. + Utf8Conversion(Error), + /// Opening the capability-scoped literal prefix failed unexpectedly. + CapabilityRootIo(Error), + /// Processing an entry returned by the glob walker failed. + GlobEntryProcessing(Error), +} + +impl GlobExpansionFailure { + /// Return the bounded metric outcome for this failure. + pub(super) const fn outcome(&self) -> &'static str { + match self { + Self::InvalidPattern(_) => "invalid_pattern", + Self::BaseCanonicalization(_) => "base_canonicalization_failure", + Self::Utf8Conversion(_) => "utf8_conversion_failure", + Self::CapabilityRootIo(_) => "capability_root_io_failure", + Self::GlobEntryProcessing(_) => "glob_entry_processing_failure", + } + } + + /// Recover the existing render error after recording its bounded outcome. + pub(super) fn into_error(self) -> Error { + match self { + Self::InvalidPattern(error) + | Self::BaseCanonicalization(error) + | Self::Utf8Conversion(error) + | Self::CapabilityRootIo(error) + | Self::GlobEntryProcessing(error) => error, + } + } +} + /// Context describing a glob pattern failure. #[derive(Debug)] pub(super) struct GlobErrorContext { diff --git a/src/manifest/glob/escape.rs b/src/manifest/glob/escape.rs new file mode 100644 index 000000000..1c44d647a --- /dev/null +++ b/src/manifest/glob/escape.rs @@ -0,0 +1,77 @@ +//! Escape injected glob-base paths without corrupting platform roots. +//! +//! This module owns only the private translation from a resolved filesystem +//! path to glob search text. [`super::PreparedGlob`] is its sole caller; it +//! supplies a canonical base, while this module preserves path syntax and +//! escapes only ordinary component names. + +use camino::{Utf8Component, Utf8Path}; + +/// Escape normal path components for glob compilation without changing roots. +/// +/// Windows canonicalization can yield an extended-length prefix such as +/// `\\?\C:`. The `?` in that prefix is path syntax rather than a glob token, +/// so escaping the complete path would invalidate it. Prefix and root +/// components therefore remain verbatim; only ordinary component names are +/// escaped. +pub(super) fn escape_glob_literal_path(path: &Utf8Path) -> String { + let separator = std::path::MAIN_SEPARATOR; + let mut escaped = String::new(); + let mut needs_separator = false; + + for component in path.components() { + match component { + Utf8Component::Prefix(prefix) => { + escaped.push_str(prefix.as_str()); + needs_separator = false; + } + Utf8Component::RootDir => { + escaped.push(separator); + needs_separator = false; + } + Utf8Component::CurDir => { + append_glob_path_component(&mut escaped, ".", separator, &mut needs_separator); + } + Utf8Component::ParentDir => { + append_glob_path_component(&mut escaped, "..", separator, &mut needs_separator); + } + Utf8Component::Normal(name) => { + let literal = glob::Pattern::escape(name); + append_glob_path_component(&mut escaped, &literal, separator, &mut needs_separator); + } + } + } + escaped +} + +/// Append one escaped component while preserving host-native path semantics. +fn append_glob_path_component( + path: &mut String, + component: &str, + separator: char, + needs_separator: &mut bool, +) { + if *needs_separator { + path.push(separator); + } + path.push_str(component); + *needs_separator = true; +} + +#[cfg(all(test, windows))] +mod tests { + //! Covers prefix-safe escaping for Windows glob search paths. + + use super::escape_glob_literal_path; + use camino::Utf8Path; + + /// Preserve a Windows extended-length prefix while escaping ordinary components. + #[test] + fn preserves_verbatim_prefix() { + let path = Utf8Path::new(r"\\?\C:\work\literal[ab]base"); + assert_eq!( + escape_glob_literal_path(path), + r"\\?\C:\work\literal[[]ab[]]base" + ); + } +} diff --git a/src/manifest/glob/mod.rs b/src/manifest/glob/mod.rs index b9ffc4030..9c1991ecd 100644 --- a/src/manifest/glob/mod.rs +++ b/src/manifest/glob/mod.rs @@ -6,9 +6,10 @@ //! paths in the order the `glob` crate yields them, with directories filtered //! out. //! -//! The work is split across four private submodules: +//! The work is split across seven private submodules: //! //! - `validate` rejects unbalanced braces before any filesystem access. +//! - `base` canonicalizes an injected base into a glob-compatible path. //! - `normalize` maps separators onto the platform's and, on Unix, rewrites //! backslash escapes into the bracket classes the `glob` crate understands. //! [`GlobPattern`] pairs the caller's text with that normalized form. @@ -17,9 +18,14 @@ //! runs the metadata check that filters each match. //! - `diagnostics` records the bounded data the pure expansion query returns //! at the manifest orchestration boundary. -//! - The manifest adapter exposes only paths that are portable unquoted shell -//! words. The public [`glob_paths`] query remains a filesystem API and does -//! not impose that template-specific command-safety policy. +//! - `errors` centralizes context-rich failures from validation and filesystem +//! access. +//! - `escape` preserves platform roots while quoting injected base components +//! for glob compilation. +//! +//! The manifest adapter exposes only paths that are portable unquoted shell +//! words. The public [`glob_paths`] query remains a filesystem API and does +//! not impose that template-specific command-safety policy. //! //! Matching itself belongs to the `glob` crate, which traverses the filesystem //! ambiently; only the metadata check is capability-scoped. `walk`'s module @@ -30,11 +36,16 @@ use minijinja::Error; mod diagnostics; mod errors; +mod escape; mod normalize; mod validate; mod walk; -use errors::{GlobErrorContext, GlobErrorType, create_glob_error}; +pub(super) use base::GlobBaseCache; +use base::PreparedGlob; +use camino::{Utf8Path, Utf8PathBuf}; +pub(super) use diagnostics::expand_manifest_template_glob; +use errors::{GlobErrorContext, GlobErrorType, GlobExpansionFailure, create_glob_error}; use normalize::normalize_separators; use validate::validate_brace_matching; use walk::{open_root_dir, process_glob_entry}; @@ -157,8 +168,8 @@ impl GlobSkippedEntries { /// Entry selected by the capability-scoped metadata query. #[derive(Debug)] pub(super) enum GlobEntry { - /// A matched regular file path with separators normalized to `/`. - Path(String), + /// A matched regular file path retained until final result formatting. + Path(Utf8PathBuf), /// A symlink the capability cannot resolve, given relative to the prefix. UnreachableSymlink(camino::Utf8PathBuf), /// The match does not name a regular file. @@ -241,7 +252,7 @@ fn is_shell_inert_path(path: &str) -> bool { /// /// ``` /// use netsuke::manifest::glob_paths; -/// let _: fn(&str) -> _ = glob_paths; +/// let _: fn(&str, Option<&camino::Utf8Path>) -> _ = glob_paths; /// ``` /// /// The module's internals are not. `GlobEntryResult` is a private alias inside @@ -254,12 +265,45 @@ fn is_shell_inert_path(path: &str) -> bool { /// The passing example above is the control for this rejection: it fails /// instead if the rustdoc harness wiring breaks, so the `compile_fail` block /// cannot pass vacuously. -pub fn glob_paths(pattern: &str) -> std::result::Result, Error> { - expand_glob(pattern).map(GlobExpansion::into_paths) +pub fn glob_paths( + pattern: &str, + base: Option<&Utf8Path>, +) -> std::result::Result, Error> { + expand_glob(pattern, base).map(GlobExpansion::into_paths) } /// Expand a pattern and return its bounded diagnostic data without recording it. -pub(super) fn expand_glob(pattern: &str) -> std::result::Result { +/// +/// `base` anchors relative patterns: when supplied and the pattern is not +/// absolute, the pattern is joined onto `base` before matching and the base is +/// stripped from the returned paths, so results keep their pattern-relative +/// spelling. `None` falls back to the process current directory, the +/// composition-root behaviour retained for string parsing. +pub(super) fn expand_glob( + pattern: &str, + base: Option<&Utf8Path>, +) -> std::result::Result { + let prepared = PreparedGlob::new(pattern, base)?; + expand_prepared_glob(&prepared).map_err(GlobExpansionFailure::into_error) +} + +/// Expand a pattern using a manifest-owned injected-base cache. +/// +/// The cache retains a successful canonicalization across `glob()` calls in +/// one manifest parse. It is consulted only after the normalized pattern has +/// been found relative, so absolute patterns still avoid base resolution. +fn expand_glob_with_base_cache( + pattern: &str, + base: &GlobBaseCache, +) -> std::result::Result { + let prepared = PreparedGlob::new_with_base_cache_for_template(pattern, base)?; + expand_prepared_glob(&prepared) +} + +/// Expand one already-prepared glob search and collect its diagnostic data. +fn expand_prepared_glob( + prepared: &PreparedGlob, +) -> std::result::Result { use glob::{MatchOptions, glob_with}; let opts = MatchOptions { @@ -268,29 +312,32 @@ pub(super) fn expand_glob(pattern: &str) -> std::result::Result std::result::Result paths.push(path), + match process_glob_entry(entry, &prepared.pattern, &root) + .map_err(GlobExpansionFailure::GlobEntryProcessing)? + { + GlobEntry::Path(path) => paths.push(strip_base(prepared.strip.as_deref(), &path)), GlobEntry::UnreachableSymlink(relative) => { skipped.record_unreachable_symlink(relative); } @@ -320,14 +369,22 @@ pub(super) fn expand_glob(pattern: &str) -> std::result::Result, path: &Utf8Path) -> String { + let relative = base + .and_then(|dir| path.strip_prefix(dir).ok()) + .unwrap_or(path); + // Format once, after any lexical base stripping, so a matched path does + // not first allocate a normalized String only to allocate again to rebase. + relative.as_str().replace('\\', "/") } - #[cfg(test)] mod tests; + +mod base; diff --git a/src/manifest/glob/tests/base.rs b/src/manifest/glob/tests/base.rs new file mode 100644 index 000000000..50de8baa9 --- /dev/null +++ b/src/manifest/glob/tests/base.rs @@ -0,0 +1,77 @@ +//! Tests for the injected base directory that anchors relative glob patterns. +//! +//! [`super::glob_paths`] accepts an optional base: relative patterns are joined +//! onto it before matching and the base is stripped from the results. These +//! tests cover the two invariants of that anchoring — the base is not applied +//! twice, and a symlinked base is followed rather than rejected. +#[cfg(unix)] +use super::super::glob_paths; +#[cfg(unix)] +use anyhow::{Context, Result, ensure}; +#[cfg(unix)] +use camino::Utf8Path; +#[cfg(unix)] +use tempfile::{Builder, tempdir}; +#[cfg(unix)] +use test_support::fs as test_fs; + +/// A relative injected base is not reopened under itself. +/// +/// `expand_glob` joins the base onto the pattern to build the search text +/// (`base.join(pattern)`); the capability root must then be opened from that +/// combined path rather than from the base a second time. Passing the base to +/// `open_root_dir` again would open `base` and then traverse the `base` +/// component once more, doubling the path and failing to match anything. +#[cfg(unix)] +#[test] +fn glob_paths_relative_base_is_not_doubled() -> Result<()> { + let cwd = std::env::current_dir().context("read the process working directory")?; + let temp = Builder::new() + .prefix("f1-relative-base-") + .tempdir_in(&cwd) + .context("create a temporary directory under the working directory")?; + let base = temp + .path() + .strip_prefix(&cwd) + .context("the temporary directory must live under the working directory")?; + test_fs::write(temp.path().join("a.txt"), "a")?; + + let results = glob_paths( + "*.txt", + Some(Utf8Path::from_path(base).expect("temp paths are UTF-8")), + )?; + ensure!( + results == vec!["a.txt".to_owned()], + "a relative base must not be doubled, got {results:?}" + ); + Ok(()) +} + +/// A symlinked base is followed, but a symlink inside the pattern's own +/// literal prefix is still rejected. +/// +/// The injected base is the workspace root the manifest was opened through, +/// which the shell may reach via a symbolic link. Anchoring the capability at +/// that base must follow the link; the pattern's literal prefix components are +/// still walked without following symlinks, so `glob_paths` with a symlinked +/// base expands matches rather than rejecting the whole glob. +#[cfg(unix)] +#[test] +fn glob_paths_follows_a_symlinked_base() -> Result<()> { + let temp = tempdir()?; + let target = temp.path().join("target"); + test_fs::create_dir(&target)?; + test_fs::write(target.join("a.txt"), "a")?; + let link = temp.path().join("link"); + test_fs::symlink("target", &link)?; + + let results = glob_paths( + "*.txt", + Some(Utf8Path::from_path(&link).expect("temp paths are UTF-8")), + )?; + ensure!( + results == vec!["a.txt".to_owned()], + "expected the match relative to the symlinked base, got {results:?}" + ); + Ok(()) +} diff --git a/src/manifest/glob/tests/base_property.rs b/src/manifest/glob/tests/base_property.rs new file mode 100644 index 000000000..55dceb17c --- /dev/null +++ b/src/manifest/glob/tests/base_property.rs @@ -0,0 +1,281 @@ +//! Property tests for injected-base glob expansion through the production +//! [`super::glob_paths`] boundary. +//! +//! The fixed cases in the sibling modules pin individual anchoring shapes. +//! These cover the invariants those shapes are examples of, across arbitrary +//! safe nesting: a relative pattern under `Some(base)` resolves to exactly +//! the fixture files spelled relative to the pattern, two distinct bases +//! never cross-contaminate, an absolute pattern ignores the base, a +//! parent-relative pattern keeps its `..` spelling, `None` retains the +//! unbased behaviour, and produced paths always use forward slashes. +//! +//! Each case builds a disposable temporary fixture tree and invokes the +//! production `glob_paths` function; the environment and working directory of +//! the test process are never mutated. + +use super::super::glob_paths; +use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; +use minijinja::ErrorKind; +use proptest::collection; +use proptest::prelude::*; +use std::collections::BTreeSet; +use tempfile::tempdir; +use test_support::fs as test_fs; + +/// Generate a small tree of safe (lowercase ASCII) path segments. +fn segments() -> impl Strategy> { + collection::vec("[a-z]{1,5}", 0..4) +} + +/// The expected pattern-relative spelling of `segments` joined to `leaf.txt`. +fn expected_relative(segments: &[String]) -> String { + let mut path = String::new(); + for segment in segments { + if !path.is_empty() { + path.push('/'); + } + path.push_str(segment); + } + if !path.is_empty() { + path.push('/'); + } + path.push_str("leaf.txt"); + path +} + +/// Install `segments/leaf.txt` under `root` and return the expected spelling. +fn install_leaf(root: &std::path::Path, segments: &[String]) -> Result { + let mut dir = root.to_path_buf(); + for segment in segments { + dir = dir.join(segment); + test_fs::create_dir(&dir).with_context(|| format!("create {dir:?}"))?; + } + let leaf = dir.join("leaf.txt"); + test_fs::write(&leaf, "x").with_context(|| format!("write {leaf:?}"))?; + Ok(expected_relative(segments)) +} + +proptest! { + /// A relative pattern under `Some(base)` returns exactly the fixture + /// files spelled relative to the pattern, independent of the base's + /// absolute location. + #[test] + fn relative_pattern_under_base_returns_pattern_relative_paths( + segments in segments(), + ) { + let temp = + tempdir().context("create a temporary directory").expect("temp dir must be creatable"); + let expected = install_leaf(temp.path(), &segments).expect("fixture must install"); + let base = Utf8Path::from_path(temp.path()).expect("temp paths are UTF-8"); + let mut results = glob_paths("**/*.txt", Some(base)).expect("relative glob must succeed"); + results.sort(); + let mut want = vec![expected]; + want.sort(); + prop_assert_eq!(results, want); + } + + /// The same relative pattern under two distinct bases returns only each + /// base's own files; the bases never cross-contaminate. + #[test] + fn distinct_bases_do_not_cross_contaminate( + first in segments(), + second in segments(), + ) { + let temp = + tempdir().context("create a temporary directory").expect("temp dir must be creatable"); + let base_a = temp.path().join("a"); + test_fs::create_dir(&base_a).expect("base A dir must be creatable"); + let base_b = temp.path().join("b"); + test_fs::create_dir(&base_b).expect("base B dir must be creatable"); + let expected_a = install_leaf(&base_a, &first).expect("fixture A must install"); + let expected_b = install_leaf(&base_b, &second).expect("fixture B must install"); + let pattern = "**/*.txt"; + + let results_a = + glob_paths(pattern, Some(Utf8Path::from_path(&base_a).expect("UTF-8"))) + .expect("glob under base A must succeed"); + let results_b = + glob_paths(pattern, Some(Utf8Path::from_path(&base_b).expect("UTF-8"))) + .expect("glob under base B must succeed"); + prop_assert_eq!( + results_a.into_iter().collect::>(), + BTreeSet::from([expected_a]), + "base A must not see base B's files" + ); + prop_assert_eq!( + results_b.into_iter().collect::>(), + BTreeSet::from([expected_b]), + "base B must not see base A's files" + ); + } + + /// An absolute pattern under `Some(base)` is not anchored: the base is + /// neither prepended nor stripped, and the pattern-relative suffix is + /// retained. + #[test] + fn absolute_pattern_ignores_the_base(nested in segments()) { + let temp = + tempdir().context("create a temporary directory").expect("temp dir must be creatable"); + let concrete = temp.path().join("concrete"); + test_fs::create_dir(&concrete).expect("concrete dir must be creatable"); + let expected = install_leaf(&concrete, &nested).expect("fixture must install"); + let absolute_pattern = format!("{}/**/*.txt", concrete.display()); + // A decoy base that must neither be joined nor stripped. + let decoy = temp.path().join("decoy"); + test_fs::create_dir(&decoy).expect("decoy dir must be creatable"); + test_fs::write(decoy.join("stray.txt"), "s").expect("decoy file must be writable"); + let found = glob_paths( + &absolute_pattern, + Some(Utf8Path::from_path(&decoy).expect("UTF-8")), + ) + .expect("absolute glob must succeed"); + let results = found.into_iter().collect::>(); + prop_assert!( + results.len() == 1, + "absolute pattern must match only concrete's files: {results:?}" + ); + let got = results.iter().next().expect("one result was asserted above"); + // Compare the suffix after the concrete base directory. + let suffix = format!("/{expected}"); + prop_assert!( + got.ends_with(&suffix), + "absolute result {got:?} must retain suffix {suffix:?}" + ); + } +} + +/// A parent-relative pattern keeps its `..` spelling in the result. +/// +/// The base's parent — here the temporary directory — is isolated, so the +/// result is deterministic; this is the same contract the integration tests +/// pin through a manifest workspace root. +#[test] +fn parent_relative_pattern_preserves_dot_dot() -> Result<()> { + let temp = tempdir()?; + let sub = temp.path().join("sub"); + test_fs::create_dir(&sub)?; + test_fs::write(temp.path().join("out.txt"), "out")?; + + let results = glob_paths( + "../*.txt", + Some(Utf8Path::from_path(&sub).expect("temp paths are UTF-8")), + )?; + ensure!( + results == vec!["../out.txt".to_owned()], + "expected the parent-relative match, got {results:?}" + ); + Ok(()) +} + +/// `None` retains the unbased behaviour: an absolute pattern returns the +/// matched absolute path with no base stripping. +#[test] +fn none_base_keeps_absolute_results() -> Result<()> { + let temp = tempdir()?; + let concrete = temp.path().join("concrete"); + test_fs::create_dir(&concrete)?; + test_fs::write(concrete.join("leaf.txt"), "x")?; + + let pattern = format!("{}/leaf.txt", concrete.display()); + let results = glob_paths(&pattern, None)?; + let result = results + .first() + .context("absolute pattern must return its one matching file")?; + let resolved_result = dunce::canonicalize(result)?; + let expected = dunce::canonicalize(concrete.join("leaf.txt"))?; + ensure!( + results.len() == 1 && Utf8Path::new(result).is_absolute() && resolved_result == expected, + "None must return the unstripped absolute file, got {results:?}" + ); + Ok(()) +} + +/// Every produced path uses forward slashes on every platform. +#[test] +fn results_use_forward_slashes() -> Result<()> { + let temp = tempdir()?; + let base = temp.path().join("base"); + test_fs::create_dir(&base)?; + let nested = base.join("nested"); + test_fs::create_dir(&nested)?; + test_fs::write(nested.join("leaf.txt"), "x")?; + let pattern = "**/*.txt"; + let results = glob_paths( + pattern, + Some(Utf8Path::from_path(&base).expect("temp paths are UTF-8")), + )?; + ensure!( + results == vec!["nested/leaf.txt".to_owned()], + "expected forward-slash spelling, got {results:?}" + ); + Ok(()) +} + +/// Assert that a metacharacter in an injected base remains literal. +fn assert_injected_base_metacharacter_is_literal(base_name: &str, decoy_name: &str) -> Result<()> { + let temp = tempdir()?; + let base = temp.path().join(base_name); + let decoy = temp.path().join(decoy_name); + test_fs::create_dir(&base)?; + test_fs::create_dir(&decoy)?; + test_fs::write(base.join("wanted.txt"), "wanted")?; + test_fs::write(decoy.join("decoy.txt"), "decoy")?; + + let results = glob_paths( + "*.txt", + Some(Utf8Path::from_path(&base).context("temporary paths must be UTF-8")?), + )?; + ensure!( + results == vec!["wanted.txt".to_owned()], + "base {base_name:?} must not match decoy {decoy_name:?}: {results:?}" + ); + Ok(()) +} + +/// Treat glob metacharacters in an injected base as literal path components. +/// +/// Each neighbouring decoy would match `*.txt` only if the base were compiled +/// as glob syntax instead of being escaped before the user pattern is joined. +#[cfg(unix)] +#[rstest::rstest] +#[case("literal*base", "literalxbase")] +#[case("literal?base", "literalxbase")] +fn injected_base_star_and_question_mark_are_literal( + #[case] base_name: &str, + #[case] decoy_name: &str, +) -> Result<()> { + assert_injected_base_metacharacter_is_literal(base_name, decoy_name) +} + +/// Cover an injected-base metacharacter that Windows permits in directory names. +/// +/// Windows reserves `*` and `?` in filesystem components, so the non-Windows +/// cases above retain that matcher coverage while this test exercises the legal +/// bracket spelling on every supported platform. +#[rstest::rstest] +#[case("literal[ab]base", "literalabase")] +fn injected_base_metacharacters_are_literal( + #[case] base_name: &str, + #[case] decoy_name: &str, +) -> Result<()> { + assert_injected_base_metacharacter_is_literal(base_name, decoy_name) +} + +/// Propagate a missing injected base as the glob I/O error rather than +/// silently searching from an unrelated fallback directory. +#[test] +fn missing_injected_base_is_an_io_error() -> Result<()> { + let temp = tempdir()?; + let missing = temp.path().join("missing"); + let error = glob_paths( + "*.txt", + Some(Utf8Path::from_path(&missing).expect("temp paths are UTF-8")), + ) + .expect_err("missing injected base must not fall back to another directory"); + ensure!( + error.kind() == ErrorKind::InvalidOperation, + "missing base must preserve the glob I/O error policy, got {error:?}" + ); + Ok(()) +} diff --git a/src/manifest/glob/tests/capability.rs b/src/manifest/glob/tests/capability.rs index c8ecaa5aa..321b22aa2 100644 --- a/src/manifest/glob/tests/capability.rs +++ b/src/manifest/glob/tests/capability.rs @@ -9,8 +9,6 @@ use camino::{Utf8Path, Utf8PathBuf}; use minijinja::ErrorKind; use rstest::{fixture, rstest}; use tempfile::{TempDir, tempdir}; -use test_support::cwd_guard::CwdGuard; -use test_support::env_lock::EnvLock; use test_support::fs as test_fs; /// A tree with one file inside `scoped/` and one sibling outside it. @@ -62,13 +60,14 @@ fn open_root_dir_declines_unopenable_prefix( ) -> Result<()> { let temp = scoped_tree?; let pattern = GlobPattern::new(&format!("{}/{prefix}/*.txt", temp.path().display()))?; - let root = open_root_dir(&pattern).with_context(|| format!("open root for {desc}"))?; + let root = open_root_dir(pattern.normalized(), None) + .with_context(|| format!("open root for {desc}"))?; ensure!( root.is_none(), "{desc} prefix should yield no capability at all" ); ensure!( - glob_paths(pattern.raw())?.is_empty(), + glob_paths(pattern.raw(), None)?.is_empty(), "{desc} prefix should expand to no matches" ); Ok(()) @@ -87,7 +86,7 @@ fn open_root_dir_scopes_past_an_escaped_metacharacter() -> Result<()> { test_fs::write(temp.path().join("out.txt"), "out")?; let pattern = GlobPattern::new(&format!(r"{}/\*x/*.txt", temp.path().display()))?; - let root = open_root_dir(&pattern) + let root = open_root_dir(pattern.normalized(), None) .context("open capability root")? .ok_or_else(|| anyhow!("the escaped directory exists, so a root was expected"))?; @@ -102,7 +101,7 @@ fn open_root_dir_scopes_past_an_escaped_metacharacter() -> Result<()> { "the escaped directory's contents must be reachable" ); - let results = glob_paths(pattern.raw())?; + let results = glob_paths(pattern.raw(), None)?; ensure!( results.iter().all(|p| p.ends_with("in.txt")) && results.len() == 1, "expected only the file inside the escaped directory: {results:?}" @@ -123,11 +122,11 @@ fn open_root_dir_rejects_a_symlinked_literal_prefix() -> Result<()> { let pattern = GlobPattern::new(&format!("{}/link/*.c", temp.path().display()))?; ensure!( - open_root_dir(&pattern).is_err(), + open_root_dir(pattern.normalized(), None).is_err(), "a symbolic-link prefix must not receive a capability" ); ensure!( - glob_paths(pattern.raw()).is_err(), + glob_paths(pattern.raw(), None).is_err(), "a symbolic-link prefix must fail rather than traverse its target" ); Ok(()) @@ -161,7 +160,7 @@ fn open_root_dir_propagates_an_unreadable_prefix() -> Result<()> { let _restore = ModeGuard(Utf8PathBuf::try_from(locked.clone())?); let pattern = GlobPattern::new(&format!("{}/inner/*.txt", locked.display()))?; - let Err(err) = open_root_dir(&pattern) else { + let Err(err) = open_root_dir(pattern.normalized(), None) else { // A privileged user bypasses the mode, so there is nothing to observe. tracing::warn!("skipping: the mode-000 prefix stayed readable"); return Ok(()); @@ -172,7 +171,7 @@ fn open_root_dir_propagates_an_unreadable_prefix() -> Result<()> { kind = err.kind() ); - let expansion = glob_paths(pattern.raw()) + let expansion = glob_paths(pattern.raw(), None) .expect_err("an unreadable prefix must fail the expansion, not silently match nothing"); ensure!( expansion.kind() == ErrorKind::InvalidOperation, @@ -189,7 +188,7 @@ fn open_root_dir_scopes_capability_to_literal_prefix(scoped_tree: Result let root_path = Utf8PathBuf::try_from(temp.path().to_path_buf()).context("temp dir path is not UTF-8")?; let pattern = GlobPattern::new(&format!("{root_path}/scoped/*.txt"))?; - let root = open_root_dir(&pattern) + let root = open_root_dir(pattern.normalized(), None) .context("open capability root")? .ok_or_else(|| anyhow!("literal prefix exists, so a root was expected"))?; @@ -245,7 +244,7 @@ fn glob_root_relativises_matches_against_the_prefix(scoped_tree: Result fn glob_paths_matches_only_within_literal_prefix(scoped_tree: Result) -> Result<()> { let temp = scoped_tree?; let pattern = format!("{}/scoped/*.txt", temp.path().display()); - let results = glob_paths(&pattern)?; + let results = glob_paths(&pattern, None)?; ensure!( results.iter().all(|p| p.ends_with("in.txt")), "only files under the literal prefix should match: {results:?}" @@ -256,19 +255,20 @@ fn glob_paths_matches_only_within_literal_prefix(scoped_tree: Result) - #[test] fn glob_paths_matches_parent_relative_patterns() -> Result<()> { - // Scoping the capability at the literal prefix also reaches patterns that - // ascend past the working directory: a `..` component in a match used to - // be rejected by the working-directory handle as a sandbox escape. + // Injecting a subdirectory as the glob base reaches patterns that ascend + // out of it: a `..` component in a match used to require changing the + // process working directory, which the sandbox-escape rejection cannot + // allow. The base's parent — here the temporary directory — is isolated, + // so the result is deterministic. let temp = tempdir()?; let sub = temp.path().join("sub"); test_fs::create_dir(&sub)?; test_fs::write(temp.path().join("out.txt"), "out")?; - let _lock = EnvLock::acquire(); - let _guard = CwdGuard::acquire()?; - std::env::set_current_dir(&sub).context("switch to the subdirectory")?; - - let results = glob_paths("../*.txt")?; + let results = glob_paths( + "../*.txt", + Some(Utf8Path::from_path(&sub).expect("temp paths are UTF-8")), + )?; ensure!( results == vec!["../out.txt".to_owned()], "expected the parent-relative match, got {results:?}" @@ -306,7 +306,8 @@ fn glob_paths_skips_symlinks_escaping_the_prefix( test_fs::symlink(link_target, src.join(link_name))?; let pattern = format!("{}/{pattern_tail}", temp.path().display()); - let results = glob_paths(&pattern).context("an escaping symlink must not abort the walk")?; + let results = + glob_paths(&pattern, None).context("an escaping symlink must not abort the walk")?; ensure!( results.iter().any(|p| p.ends_with(kept)), "valid matches should be preserved: {results:?}" @@ -330,7 +331,8 @@ fn glob_paths_skips_dangling_symlinks() -> Result<()> { test_fs::symlink("nowhere.c", src.join("dangling.c"))?; let pattern = format!("{}/src/*.c", temp.path().display()); - let results = glob_paths(&pattern).context("a dangling symlink must not abort the walk")?; + let results = + glob_paths(&pattern, None).context("a dangling symlink must not abort the walk")?; ensure!( results.iter().any(|p| p.ends_with("real.c")), "valid matches should be preserved: {results:?}" @@ -353,7 +355,7 @@ fn glob_paths_reports_symlink_loops() -> Result<()> { test_fs::symlink("loop.c", src.join("loop.c"))?; let pattern = format!("{}/src/*.c", temp.path().display()); - let err = glob_paths(&pattern).expect_err("a symlink loop should surface as an error"); + let err = glob_paths(&pattern, None).expect_err("a symlink loop should surface as an error"); ensure!( err.kind() == ErrorKind::InvalidOperation, "unexpected error kind {kind:?}", @@ -368,7 +370,7 @@ fn open_root_dir_falls_back_to_cwd_without_a_literal_prefix() -> Result<()> { // component, so the capability stays scoped to the working directory — // the pre-existing behaviour for relative patterns. let pattern = GlobPattern::new("*.txt")?; - let root = open_root_dir(&pattern) + let root = open_root_dir(pattern.normalized(), None) .context("open capability root")? .ok_or_else(|| anyhow!("the working directory always exists"))?; ensure!( diff --git a/src/manifest/glob/tests/diagnostics.rs b/src/manifest/glob/tests/diagnostics.rs index 1191dbc8f..6b57c3862 100644 --- a/src/manifest/glob/tests/diagnostics.rs +++ b/src/manifest/glob/tests/diagnostics.rs @@ -1,84 +1,127 @@ //! Tests for the counters and tracing events glob expansion records. //! -//! Each case records data returned by the pure expansion query through a -//! subscriber scoped to the call. The recorder and subscriber are both +//! Each case exercises the manifest-template adapter or the pure query through +//! a subscriber scoped to the call. The recorder and subscriber are both //! thread-local, so no test-wide lock is needed. #[cfg(unix)] use super::super::MAX_UNREACHABLE_SYMLINK_SAMPLES; -use super::super::{expand_glob, glob_paths, record_expansion}; -use anyhow::{Context, Result, ensure}; -use metrics::SharedString; -use metrics_util::{ - CompositeKey, MetricKind, - debugging::{DebugValue, DebuggingRecorder}, +use super::super::{ + GlobBaseCache, GlobExpansion, PreparedGlob, expand_manifest_template_glob, glob_paths, +}; +use super::diagnostics_support::{ + BASE_CACHE, EXPANSIONS, SKIPPED, Snapshot, TEMPLATE_EXPANSION_DURATION, TEMPLATE_EXPANSIONS, + counter_value, counter_value_with_labels, has_histogram, recorded, }; +use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; use rstest::rstest; use tempfile::tempdir; use test_support::fs as test_fs; -use tracing::level_filters::LevelFilter; -type Snapshot = Vec<( - CompositeKey, - Option, - Option, - DebugValue, -)>; - -/// Run `expand` with a local metrics recorder and a capturing subscriber. -fn recorded(expand: impl FnOnce() -> T) -> (T, Vec, Snapshot) { - let recorder = DebuggingRecorder::new(); - let snapshotter = recorder.snapshotter(); - let (value, events) = metrics::with_local_recorder(&recorder, || { - crate::test_tracing_capture::with_test_subscriber(LevelFilter::DEBUG, |captured| { - let value = expand(); - (value, captured.snapshot()) - }) - }); - (value, events, snapshotter.snapshot().into_vec()) -} +#[path = "template_modes.rs"] +mod template_modes; /// Expand and record at the manifest adapter's telemetry boundary. fn expand_and_record(pattern: &str) -> Result> { - let expansion = expand_glob(pattern)?; - record_expansion(&expansion); + let base = GlobBaseCache::new(None); + let expansion = expand_manifest_template_glob(pattern, &base)?; Ok(expansion.into_paths()) } -/// Value of the counter `name` carrying the label `label = value`. -fn counter_value(snapshot: &Snapshot, name: &str, label: (&str, &str)) -> Option { - snapshot.iter().find_map(|(key, _, _, debug_value)| { - if key.kind() != MetricKind::Counter || key.key().name() != name { - return None; - } - let carries_label = key - .key() - .labels() - .any(|found| found.key() == label.0 && found.value() == label.1); - match debug_value { - DebugValue::Counter(count) if carries_label => Some(*count), - _ => None, - } +/// Expand and record a manifest-template glob anchored at `temporary_directory`. +fn expand_and_record_with_injected_base( + temporary_directory: &tempfile::TempDir, + pattern: &str, +) -> (Result>, Vec, Snapshot) { + recorded(|| -> Result> { + let base = Utf8Path::from_path(temporary_directory.path()) + .context("temporary directory should have a UTF-8 path")? + .to_path_buf(); + let cache = GlobBaseCache::new(Some(base)); + let expansion = expand_manifest_template_glob(pattern, &cache)?; + Ok(GlobExpansion::into_paths(expansion)) }) } -const EXPANSIONS: &str = "netsuke_manifest_glob_expansions_total"; -const SKIPPED: &str = "netsuke_manifest_glob_entries_skipped_total"; +#[rstest] +fn base_cache_records_bypass_hit_miss_and_error_outcomes() -> Result<()> { + let temporary_directory = tempdir()?; + let base = Utf8Path::from_path(temporary_directory.path()) + .context("temporary directory should have a UTF-8 path")? + .to_path_buf(); + let unconfigured = GlobBaseCache::new(None); + let configured = GlobBaseCache::new(Some(base.clone())); + let missing = GlobBaseCache::new(Some(base.join("missing"))); + + let (result, events, snapshot) = recorded(|| -> Result<()> { + PreparedGlob::new_with_base_cache("*.txt", &unconfigured)?; + PreparedGlob::new_with_base_cache("*.txt", &configured)?; + PreparedGlob::new_with_base_cache("*.txt", &configured)?; + ensure!( + PreparedGlob::new_with_base_cache("*.txt", &missing).is_err(), + "a missing injected base must fail preparation" + ); + Ok(()) + }); + result?; + + for outcome in ["bypass", "miss", "hit", "error"] { + ensure!( + counter_value(&snapshot, BASE_CACHE, ("outcome", outcome)) == Some(1), + "expected one base-cache {outcome} outcome: {snapshot:?}" + ); + } + ensure!( + events + .iter() + .any(|event| event.contains("operation=\"glob_base_cache\"") + && event.contains("outcome=\"miss\"")), + "expected the cache miss trace event: {events:?}" + ); + ensure!( + !events + .iter() + .any(|event| event.contains(&temporary_directory.path().display().to_string())), + "base-cache trace events must not disclose the base path: {events:?}" + ); + Ok(()) +} #[rstest] fn a_completed_expansion_counts_its_matches() -> Result<()> { let temp = tempdir()?; test_fs::write(temp.path().join("a.txt"), "a")?; test_fs::write(temp.path().join("b.txt"), "b")?; - let pattern = format!("{}/*.txt", temp.path().display()); - let (results, events, snapshot) = recorded(|| expand_and_record(&pattern)); + let (results, events, snapshot) = expand_and_record_with_injected_base(&temp, "*.txt"); ensure!(results?.len() == 2, "both files should match"); ensure!( counter_value(&snapshot, EXPANSIONS, ("outcome", "matched")) == Some(1), "a completed expansion should count once as matched: {snapshot:?}" ); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[("base_mode", "relative_with_base"), ("outcome", "matched")] + ) == Some(1), + "the template boundary should record one based matched result: {snapshot:?}" + ); + ensure!( + has_histogram(&snapshot, TEMPLATE_EXPANSION_DURATION), + "the template boundary should record its duration: {snapshot:?}" + ); + ensure!( + events.iter().any(|event| { + event.contains("operation=\"manifest_template_glob_expansion\"") + && event.contains("base_mode=\"relative_with_base\"") + && event.contains("outcome=\"matched\"") + && event.contains("manifest template glob expansion completed") + }), + "the template boundary should emit a bounded matched trace: {events:?}" + ); let expansion_event = events .iter() .find(|event| event.contains("glob expansion complete")) @@ -97,9 +140,9 @@ fn a_completed_expansion_counts_its_matches() -> Result<()> { #[rstest] fn an_unopenable_prefix_counts_and_names_the_prefix() -> Result<()> { let temp = tempdir()?; - let pattern = format!("{}/no-such-dir/*.txt", temp.path().display()); - let (results, events, snapshot) = recorded(|| expand_and_record(&pattern)); + let (results, events, snapshot) = + expand_and_record_with_injected_base(&temp, "no-such-dir/*.txt"); ensure!(results?.is_empty(), "a missing prefix should match nothing"); ensure!( @@ -110,6 +153,30 @@ fn an_unopenable_prefix_counts_and_names_the_prefix() -> Result<()> { counter_value(&snapshot, EXPANSIONS, ("outcome", "matched")).is_none(), "an expansion that never ran must not count as matched: {snapshot:?}" ); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[ + ("base_mode", "relative_with_base"), + ("outcome", "unopenable_prefix"), + ] + ) == Some(1), + "the template boundary should record one based unopenable result: {snapshot:?}" + ); + ensure!( + has_histogram(&snapshot, TEMPLATE_EXPANSION_DURATION), + "the template boundary should record its duration: {snapshot:?}" + ); + ensure!( + events.iter().any(|event| { + event.contains("operation=\"manifest_template_glob_expansion\"") + && event.contains("base_mode=\"relative_with_base\"") + && event.contains("outcome=\"unopenable_prefix\"") + && event.contains("manifest template glob expansion completed") + }), + "the template boundary should emit a bounded unopenable trace: {events:?}" + ); let prefix_event = events .iter() .find(|event| event.contains("glob literal prefix names no directory")) @@ -126,6 +193,44 @@ fn an_unopenable_prefix_counts_and_names_the_prefix() -> Result<()> { Ok(()) } +#[rstest] +fn a_failed_template_expansion_records_a_bounded_error_outcome() -> Result<()> { + let cache = GlobBaseCache::new(None); + + let (result, events, snapshot) = recorded(|| expand_manifest_template_glob("[", &cache)); + ensure!(result.is_err(), "an invalid pattern must fail expansion"); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[ + ("base_mode", "relative_without_base"), + ("outcome", "invalid_pattern"), + ] + ) == Some(1), + "the template boundary should record one unbased error result: {snapshot:?}" + ); + ensure!( + has_histogram(&snapshot, TEMPLATE_EXPANSION_DURATION), + "the failed template expansion should record its duration: {snapshot:?}" + ); + ensure!( + events.iter().any(|event| { + event.contains("operation=\"manifest_template_glob_expansion\"") + && event.contains("base_mode=\"relative_without_base\"") + && event.contains("outcome=\"invalid_pattern\"") + && event.contains("error_category=\"expansion_failure\"") + && event.contains("manifest template glob expansion failed") + }), + "the failed template expansion should emit only its bounded trace: {events:?}" + ); + ensure!( + !events.iter().any(|event| event.contains('[')), + "the failed template trace must not disclose the pattern: {events:?}" + ); + Ok(()) +} + /// A skipped match is counted by reason and traced without its path. #[cfg(unix)] #[rstest] @@ -259,7 +364,7 @@ fn glob_paths_is_a_pure_query() -> Result<()> { test_fs::write(temp.path().join("a.txt"), "a")?; let pattern = format!("{}/*.txt", temp.path().display()); - let (results, events, snapshot) = recorded(|| glob_paths(&pattern)); + let (results, events, snapshot) = recorded(|| glob_paths(&pattern, None)); ensure!(results?.len() == 1, "the file should match"); ensure!( events.is_empty(), diff --git a/src/manifest/glob/tests/diagnostics_support.rs b/src/manifest/glob/tests/diagnostics_support.rs new file mode 100644 index 000000000..534191557 --- /dev/null +++ b/src/manifest/glob/tests/diagnostics_support.rs @@ -0,0 +1,81 @@ +//! Shared recorder and assertion helpers for glob diagnostics tests. +//! +//! The helpers keep the manifest-template observability cases focused on their +//! outcome contracts while ensuring every recorder and tracing subscriber stays +//! scoped to the individual test. + +use metrics::SharedString; +use metrics_util::{ + CompositeKey, MetricKind, + debugging::{DebugValue, DebuggingRecorder}, +}; +use tracing::level_filters::LevelFilter; + +/// Hold the metric-recorder snapshot captured for one test invocation. +pub(super) type Snapshot = Vec<( + CompositeKey, + Option, + Option, + DebugValue, +)>; + +/// Name the counter reporting base-cache observations. +pub(super) const BASE_CACHE: &str = "netsuke_manifest_glob_base_cache_total"; +/// Name the counter reporting completed glob expansions. +pub(super) const EXPANSIONS: &str = "netsuke_manifest_glob_expansions_total"; +/// Name the counter reporting skipped glob entries. +pub(super) const SKIPPED: &str = "netsuke_manifest_glob_entries_skipped_total"; +/// Name the counter reporting manifest-template glob results. +pub(super) const TEMPLATE_EXPANSIONS: &str = "netsuke_manifest_template_glob_expansions_total"; +/// Name the histogram reporting manifest-template glob duration. +pub(super) const TEMPLATE_EXPANSION_DURATION: &str = + "netsuke_manifest_template_glob_expansion_duration_seconds"; + +/// Run `operation` with a local metrics recorder and a capturing subscriber. +pub(super) fn recorded(operation: impl FnOnce() -> T) -> (T, Vec, Snapshot) { + let recorder = DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + let (value, events) = metrics::with_local_recorder(&recorder, || { + crate::test_tracing_capture::with_test_subscriber(LevelFilter::DEBUG, |captured| { + let value = operation(); + (value, captured.snapshot()) + }) + }); + (value, events, snapshotter.snapshot().into_vec()) +} + +/// Return a counter value carrying the requested `label`. +pub(super) fn counter_value(snapshot: &Snapshot, name: &str, label: (&str, &str)) -> Option { + counter_value_with_labels(snapshot, name, &[label]) +} + +/// Return a counter value carrying every requested label. +pub(super) fn counter_value_with_labels( + snapshot: &Snapshot, + name: &str, + labels: &[(&str, &str)], +) -> Option { + snapshot.iter().find_map(|(key, _, _, debug_value)| { + if key.kind() != MetricKind::Counter || key.key().name() != name { + return None; + } + let carries_labels = labels.iter().all(|expected| { + key.key() + .labels() + .any(|found| found.key() == expected.0 && found.value() == expected.1) + }); + match debug_value { + DebugValue::Counter(count) if carries_labels => Some(*count), + _ => None, + } + }) +} + +/// Report whether `snapshot` contains a sample for histogram `name`. +pub(super) fn has_histogram(snapshot: &Snapshot, name: &str) -> bool { + snapshot.iter().any(|(key, _, _, debug_value)| { + key.kind() == MetricKind::Histogram + && key.key().name() == name + && matches!(debug_value, DebugValue::Histogram(_)) + }) +} diff --git a/src/manifest/glob/tests/expansion.rs b/src/manifest/glob/tests/expansion.rs index e711e1282..460ac407c 100644 --- a/src/manifest/glob/tests/expansion.rs +++ b/src/manifest/glob/tests/expansion.rs @@ -23,7 +23,7 @@ fn glob_paths_filters_directories() -> Result<()> { test_fs::write(&file, "data")?; let pattern = format!("{}/dir/*", temp.path().display()); - let results = glob_paths(&pattern)?; + let results = glob_paths(&pattern, None)?; ensure!( results.iter().any(|p| p.ends_with("file.txt")), "expected file match" @@ -37,13 +37,13 @@ fn glob_paths_filters_directories() -> Result<()> { #[test] fn glob_paths_rejects_unmatched_brace() { - let err = glob_paths("foo{bar").expect_err("brace mismatch should error"); + let err = glob_paths("foo{bar", None).expect_err("brace mismatch should error"); assert_eq!(err.kind(), ErrorKind::SyntaxError); } #[rstest] fn glob_paths_rejects_an_invalid_pattern_before_a_missing_prefix() { - let err = glob_paths("missing/[").expect_err("an invalid pattern should error"); + let err = glob_paths("missing/[", None).expect_err("an invalid pattern should error"); assert_eq!(err.kind(), ErrorKind::SyntaxError); } @@ -88,7 +88,7 @@ fn glob_paths_accepts_escaped_braces_and_matches_files() -> Result<()> { "unexpected normalized pattern: {}", normalized.normalized() ); - let results = glob_paths(&pattern)?; + let results = glob_paths(&pattern, None)?; ensure!( results.iter().any(|p| p.ends_with("{file}.txt")), "escaped brace pattern should match literal braces" diff --git a/src/manifest/glob/tests/mod.rs b/src/manifest/glob/tests/mod.rs index 267ce819a..e3f6375eb 100644 --- a/src/manifest/glob/tests/mod.rs +++ b/src/manifest/glob/tests/mod.rs @@ -3,12 +3,19 @@ //! Split by concern: [`pattern`] covers normalisation and brace validation, //! [`expansion`] covers the matches [`super::glob_paths`] returns, //! [`capability`] covers the capability handle the metadata checks run -//! through, [`diagnostics`] covers the counters and events it records, and +//! through, [`diagnostics`] covers the counters and events it records, //! [`property`] covers the prefix and relativisation invariants the fixed -//! cases are examples of. +//! cases are examples of, [`base`] covers the injected base directory that +//! anchors relative patterns, and [`base_property`] exercises those anchoring +//! invariants through the production [`super::glob_paths`] boundary across +//! arbitrary safe nesting. +#[cfg(unix)] +mod base; +mod base_property; mod capability; mod diagnostics; +mod diagnostics_support; mod expansion; mod pattern; mod property; diff --git a/src/manifest/glob/tests/pattern.rs b/src/manifest/glob/tests/pattern.rs index b93fefdb0..9e1fe84c0 100644 --- a/src/manifest/glob/tests/pattern.rs +++ b/src/manifest/glob/tests/pattern.rs @@ -1,13 +1,15 @@ //! Tests for glob pattern normalisation and brace validation. -use super::super::GlobPattern; #[cfg(unix)] use super::super::normalize::force_literal_escapes; use super::super::normalize::normalize_separators; use super::super::validate::validate_brace_matching; +use super::super::{GlobPattern, PreparedGlob}; use crate::localization::{self, keys}; use anyhow::{Context, Result, anyhow, ensure}; +use camino::Utf8Path; use minijinja::ErrorKind; use rstest::rstest; +use tempfile::tempdir; use test_support::fluent::normalize_fluent_isolates; /// Helper to assert that a pattern produces a syntax error. @@ -179,3 +181,28 @@ fn glob_pattern_new_rejects_invalid_braces() { let err = GlobPattern::new("foo{").expect_err("invalid brace pattern must fail"); assert_eq!(err.kind(), ErrorKind::SyntaxError); } + +/// Verify that a prepared relative search uses the platform's single path separator. +#[test] +fn prepared_relative_search_uses_one_host_separator() -> Result<()> { + let temp = tempdir()?; + let base = Utf8Path::from_path(temp.path()).context("temporary paths must be UTF-8")?; + let prepared = PreparedGlob::new("nested/*.txt", Some(base))?; + let separator = std::path::MAIN_SEPARATOR; + let suffix = format!("{separator}nested{separator}*.txt"); + ensure!( + prepared.search().ends_with(&suffix), + "prepared search must end with {suffix:?}, got {:?}", + prepared.search() + ); + let prefix = prepared + .search() + .strip_suffix(&suffix) + .context("prepared search must retain its expected nested-pattern suffix")?; + ensure!( + !prefix.ends_with(separator), + "prepared search must use exactly one base-pattern separator, got {:?}", + prepared.search() + ); + Ok(()) +} diff --git a/src/manifest/glob/tests/template_modes.rs b/src/manifest/glob/tests/template_modes.rs new file mode 100644 index 000000000..3533c9616 --- /dev/null +++ b/src/manifest/glob/tests/template_modes.rs @@ -0,0 +1,157 @@ +//! Tests for manifest-template glob telemetry classifications. +//! +//! These cases exercise the template boundary's bounded base-mode and failure +//! outcomes without widening the telemetry-free direct query tests. + +use super::super::super::{GlobBaseCache, GlobExpansion, expand_manifest_template_glob}; +use super::super::diagnostics_support::{ + BASE_CACHE, TEMPLATE_EXPANSION_DURATION, TEMPLATE_EXPANSIONS, counter_value, + counter_value_with_labels, has_histogram, recorded, +}; +use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; +use rstest::rstest; +use tempfile::tempdir; +use test_support::fs as test_fs; + +/// Verify that absolute patterns do not prepare the configured injected base. +#[rstest] +fn an_absolute_pattern_bypasses_an_injected_base_cache() -> Result<()> { + let temp = tempdir()?; + test_fs::write(temp.path().join("a.txt"), "a")?; + let base = Utf8Path::from_path(temp.path()) + .context("temporary directory should have a UTF-8 path")? + .to_path_buf(); + let cache = GlobBaseCache::new(Some(base)); + let pattern = format!("{}/*.txt", temp.path().display()); + + let (results, events, snapshot) = recorded(|| -> Result> { + let expansion = expand_manifest_template_glob(&pattern, &cache)?; + Ok(GlobExpansion::into_paths(expansion)) + }); + ensure!( + results?.len() == 1, + "the absolute pattern should match its file" + ); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[("base_mode", "absolute_pattern"), ("outcome", "matched")] + ) == Some(1), + "the template boundary should classify the absolute pattern: {snapshot:?}" + ); + ensure!( + counter_value(&snapshot, BASE_CACHE, ("outcome", "miss")).is_none() + && !events + .iter() + .any(|event| event.contains("manifest glob base canonicalized and cached")), + "an absolute pattern must not canonicalize an injected base: {events:?} {snapshot:?}" + ); + Ok(()) +} + +/// Verify that unbased relative patterns report their process-rooted mode. +#[rstest] +fn a_relative_pattern_without_a_base_records_its_base_mode() -> Result<()> { + let pattern = "glob-diagnostics-relative-without-base/no-such-dir/*.txt"; + let cache = GlobBaseCache::new(None); + + let (results, _events, snapshot) = recorded(|| -> Result> { + let expansion = expand_manifest_template_glob(pattern, &cache)?; + Ok(GlobExpansion::into_paths(expansion)) + }); + ensure!( + results?.is_empty(), + "the missing prefix should match nothing" + ); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[ + ("base_mode", "relative_without_base"), + ("outcome", "unopenable_prefix"), + ] + ) == Some(1), + "the template boundary should classify the unbased relative pattern: {snapshot:?}" + ); + Ok(()) +} + +/// Verify that injected-base preparation failures still complete telemetry. +#[rstest] +fn an_unresolvable_injected_base_records_one_terminal_failure() -> Result<()> { + let temp = tempdir()?; + let base = Utf8Path::from_path(temp.path()) + .context("temporary directory should have a UTF-8 path")? + .join("missing"); + let cache = GlobBaseCache::new(Some(base)); + + let (result, _events, snapshot) = recorded(|| expand_manifest_template_glob("*.txt", &cache)); + ensure!( + result.is_err(), + "a missing injected base must fail expansion" + ); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[ + ("base_mode", "relative_with_base"), + ("outcome", "base_canonicalization_failure"), + ] + ) == Some(1), + "the preparation failure should record one terminal counter: {snapshot:?}" + ); + ensure!( + has_histogram(&snapshot, TEMPLATE_EXPANSION_DURATION), + "the preparation failure should record one duration: {snapshot:?}" + ); + Ok(()) +} + +/// Verify that a symlinked literal prefix reports a capability-root failure. +#[cfg(unix)] +#[rstest] +fn a_symlinked_literal_prefix_records_a_capability_root_failure() -> Result<()> { + let temp = tempdir()?; + let target = temp.path().join("target"); + test_fs::create_dir(&target)?; + test_fs::write(target.join("a.txt"), "a")?; + test_fs::symlink("target", temp.path().join("link"))?; + let pattern = format!("{}/link/*.txt", temp.path().display()); + let cache = GlobBaseCache::new(None); + + let (result, events, snapshot) = recorded(|| expand_manifest_template_glob(&pattern, &cache)); + ensure!( + result.is_err(), + "a symlinked literal prefix must fail expansion" + ); + ensure!( + counter_value_with_labels( + &snapshot, + TEMPLATE_EXPANSIONS, + &[ + ("base_mode", "absolute_pattern"), + ("outcome", "capability_root_io_failure"), + ] + ) == Some(1), + "the template boundary should classify the capability failure: {snapshot:?}" + ); + ensure!( + events.iter().any(|event| { + event.contains("operation=\"manifest_template_glob_expansion\"") + && event.contains("outcome=\"capability_root_io_failure\"") + && event.contains("manifest template glob expansion failed") + }), + "the template boundary should trace the bounded capability failure: {events:?}" + ); + ensure!( + !events + .iter() + .any(|event| event.contains(&temp.path().display().to_string())), + "capability failure events must not disclose the literal prefix: {events:?}" + ); + Ok(()) +} diff --git a/src/manifest/glob/walk.rs b/src/manifest/glob/walk.rs index c944a0dbe..8b6b6b90f 100644 --- a/src/manifest/glob/walk.rs +++ b/src/manifest/glob/walk.rs @@ -256,12 +256,10 @@ pub(super) fn unescape_literal_escapes(prefix: &str) -> String { /// Open the directory used as the capability root for the glob. /// -/// Returns `Ok(None)` when the literal prefix does not exist (or is not a -/// directory); the pattern can match nothing in that case, mirroring the -/// empty result the matcher would produce. -pub(super) fn open_root_dir(pattern: &GlobPattern) -> io::Result> { - let prefix = literal_dir_path(pattern); - match open_literal_prefix(Utf8Path::new(&prefix)) { +/// Returns `Ok(None)` when the literal prefix does not exist (or is not a directory). +pub(super) fn open_root_dir(search: &str, base: Option<&Utf8Path>) -> io::Result> { + let prefix = literal_dir_path(search); + match open_literal_prefix(Utf8Path::new(&prefix), base) { Ok(dir) => Ok(Some(GlobRoot { dir, prefix: Utf8PathBuf::from(prefix), @@ -273,12 +271,9 @@ pub(super) fn open_root_dir(pattern: &GlobPattern) -> io::Result io::Result { +/// The ambient opening establishes the filesystem root for an absolute prefix; +/// a relative one is opened at the injected `base` (`.` when none). +fn open_literal_prefix(prefix: &Utf8Path, injected_base: Option<&Utf8Path>) -> io::Result { let (base, remainder) = if prefix.is_absolute() { let root = prefix.ancestors().last().ok_or_else(|| { io::Error::new( @@ -294,7 +289,7 @@ fn open_literal_prefix(prefix: &Utf8Path) -> io::Result { })?; (root, remainder) } else { - (Utf8Path::new("."), prefix) + (injected_base.unwrap_or_else(|| Utf8Path::new(".")), prefix) }; let mut dir = Dir::open_ambient_dir(base, ambient_authority())?.into_std_file(); @@ -325,9 +320,9 @@ fn open_literal_prefix(prefix: &Utf8Path) -> io::Result { Ok(Dir::from_std_file(dir)) } -/// Return the filesystem path represented by a pattern's literal prefix. -fn literal_dir_path(pattern: &GlobPattern) -> String { - unescape_literal_escapes(literal_dir_prefix(pattern.normalized())) +/// Return the filesystem path represented by a normalised pattern's literal prefix. +fn literal_dir_path(normalized: &str) -> String { + unescape_literal_escapes(literal_dir_prefix(normalized)) } /// Report whether `err` means the literal prefix names no usable directory. /// @@ -382,19 +377,19 @@ pub(super) fn process_glob_entry( "glob matched a non-UTF-8 path".to_owned(), ) })?; - names_a_file(root, &utf_path) + names_a_file(root, utf_path) .map_err(|err| create_io_error(pattern, pattern.raw().len(), err.to_string())) } /// Classify whether a match names a regular file reachable through the /// capability, returning a bounded reason when it does not. -fn names_a_file(root: &GlobRoot, path: &Utf8Path) -> io::Result { - let relative = root.relativise(path)?; +fn names_a_file(root: &GlobRoot, path: Utf8PathBuf) -> io::Result { + let relative = root.relativise(&path)?; let Some(metadata) = root.metadata_relative(relative)? else { return Ok(GlobEntry::UnreachableSymlink(relative.to_path_buf())); }; if metadata.is_file() { - return Ok(GlobEntry::Path(path.as_str().replace('\\', "/"))); + return Ok(GlobEntry::Path(path)); } Ok(GlobEntry::NotAFile) } diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index 759bbd7e4..bb3c42e26 100644 --- a/src/manifest/mod.rs +++ b/src/manifest/mod.rs @@ -13,14 +13,10 @@ //! [`ManifestSource`] so callers pass domain-specific types instead of raw //! strings. //! -//! The optional `vars` section must deserialize into a JSON object; a list or -//! scalar is rejected with the localized `manifest.vars.not_object` diagnostic. -//! YAML mappings with non-string or composite keys cannot be represented as -//! JSON at all, so they fail earlier, during the initial `serde_saphyr` parse, -//! with the YAML parse diagnostic. Keys colliding with the built-in `env` and -//! `glob` helpers are rejected with the localized `manifest.vars.reserved_name` -//! diagnostic, since `MiniJinja` keeps functions and global variables in a -//! single namespace. +//! The optional `vars` section must be a JSON object; lists and scalars fail +//! with `manifest.vars.not_object`; non-string or composite keys fail during +//! the initial `serde_saphyr` parse. Reserved `env`/`glob` names fail with +//! `manifest.vars.reserved_name` because `MiniJinja` shares a global namespace. use crate::{ ast::{EMPTY_COMMAND_LIST_ERROR, NetsukeManifest}, @@ -94,6 +90,9 @@ struct ManifestParse<'a> { stdlib_registration: Option, /// Environment reader backing the `env()` helper. env_reader: &'a EnvReader, + /// Manifest workspace root, anchoring relative `glob()` patterns; `None` + /// falls back to the process current directory at the composition root. + manifest_root: Option, } /// Selects the stdlib surface available while rendering a manifest. @@ -103,7 +102,10 @@ enum StdlibRegistration { /// The read-only stdlib used to inspect manifest discovery metadata. ManifestQuery, } -/// Parse a manifest string, running the full YAML, Jinja and expansion pipeline. +/// Parse, render, and validate a manifest with injected glob-base data. +/// +/// Render Jinja values, anchor relative `glob()` patterns at `manifest_root`, +/// and validate recipes after expansion so rendered rules are checked. fn from_str_named( yaml: &str, parse: ManifestParse<'_>, @@ -113,6 +115,7 @@ fn from_str_named( name, stdlib_registration, env_reader, + manifest_root, } = parse; let is_manifest_query = matches!(stdlib_registration, Some(StdlibRegistration::ManifestQuery)); notify_stage(on_stage, ManifestLoadStage::InitialYamlParsing); @@ -129,9 +132,9 @@ fn from_str_named( jinja.add_function("env", move |var_name: String| { env_var_with(&var_name, |key| reader(key)) }); - jinja.add_function("glob", |pattern: String| { - let expansion = glob::expand_glob(&pattern)?; - glob::record_expansion(&expansion); + let glob_base = glob::GlobBaseCache::new(manifest_root); + jinja.add_function("glob", move |pattern: String| { + let expansion = glob::expand_manifest_template_glob(&pattern, &glob_base)?; expansion.into_template_paths(&pattern) }); let _stdlib_state = match stdlib_registration { @@ -299,6 +302,7 @@ pub fn from_str_with_env(yaml: &str, env_reader: &EnvReader) -> Result StdlibRegistration::Full(Box::new( StdlibConfig::new(workspace.dir)? - .with_workspace_root_path(workspace.root)? + .with_workspace_root_path(&workspace.root)? .with_network_policy(policy), )), ManifestLoadMode::ManifestQuery => StdlibRegistration::ManifestQuery, }; + let manifest_root = Some(workspace.root); from_str_named( &data, ManifestParse { name: &name, stdlib_registration: Some(stdlib_registration), env_reader, + manifest_root, }, &mut on_stage, ) diff --git a/test_support/clippy.toml b/test_support/clippy.toml index 6c60d7c39..b6d2ea087 100644 --- a/test_support/clippy.toml +++ b/test_support/clippy.toml @@ -21,4 +21,5 @@ disallowed-methods = [ { path = "std::env::vars_os", reason = "inject an environment reader" }, { path = "std::env::set_var", reason = "use a stub environment in tests" }, { path = "std::env::remove_var", reason = "use a stub environment in tests" }, + { path = "std::env::set_current_dir", reason = "inject a base-directory seam; confine CWD changes to Command::current_dir" }, ] diff --git a/test_support/src/cwd_guard.rs b/test_support/src/cwd_guard.rs deleted file mode 100644 index 05a348daa..000000000 --- a/test_support/src/cwd_guard.rs +++ /dev/null @@ -1,95 +0,0 @@ -//! Restore the process working directory after tests mutate it. -//! -//! Provides a RAII guard that captures the current working directory and -//! restores it on drop so tests do not leak CWD changes into other cases. - -use std::path::PathBuf; - -/// Guard that restores the original current working directory when dropped. -#[derive(Debug)] -pub struct CwdGuard(PathBuf); - -impl CwdGuard { - /// Capture the current working directory for later restoration. - /// - /// # Errors - /// - /// Returns an error if the current directory cannot be read. - pub fn acquire() -> std::io::Result { - Ok(Self(std::env::current_dir()?)) - } - - /// Alias for [`CwdGuard::acquire`] to support existing test call sites. - /// - /// # Errors - /// - /// Returns an error if the current directory cannot be read. - pub fn new() -> std::io::Result { - Self::acquire() - } -} - -impl Drop for CwdGuard { - fn drop(&mut self) { - drop(std::env::set_current_dir(&self.0)); - } -} - -#[cfg(test)] -mod tests { - //! Unit tests for the working-directory guard. - - use super::*; - use crate::env_lock::EnvLock; - use rstest::{fixture, rstest}; - use std::io; - - #[fixture] - fn env_lock() -> EnvLock { - EnvLock::acquire() - } - - #[fixture] - fn original_dir(env_lock: EnvLock) -> io::Result<(EnvLock, std::path::PathBuf)> { - Ok((env_lock, std::env::current_dir()?)) - } - - #[rstest] - #[case(CwdGuard::acquire)] - #[case(CwdGuard::new)] - fn constructor_captures_current_directory( - #[from(original_dir)] original_dir_result: io::Result<(EnvLock, std::path::PathBuf)>, - #[case] ctor: fn() -> io::Result, - ) -> anyhow::Result<()> { - let (_lock, original_dir) = original_dir_result?; - let guard = ctor()?; - anyhow::ensure!( - guard.0 == original_dir, - "guard should capture the directory that was current at acquire time" - ); - Ok(()) - } - - #[rstest] - fn drop_restores_original_directory( - #[from(original_dir)] original_dir_result: io::Result<(EnvLock, std::path::PathBuf)>, - ) -> anyhow::Result<()> { - let (_lock, original_dir) = original_dir_result?; - let temp = tempfile::tempdir()?; - - { - let _guard = CwdGuard::acquire()?; - std::env::set_current_dir(temp.path())?; - anyhow::ensure!( - std::env::current_dir()? != original_dir, - "CWD should be temp dir inside the guard scope" - ); - } - - anyhow::ensure!( - std::env::current_dir()? == original_dir, - "CWD should be restored after guard is dropped" - ); - Ok(()) - } -} diff --git a/test_support/src/env_lock.rs b/test_support/src/env_lock.rs deleted file mode 100644 index 4e6e879f1..000000000 --- a/test_support/src/env_lock.rs +++ /dev/null @@ -1,303 +0,0 @@ -//! Serialize legacy process-global environment and CWD mutations. -//! -//! `EnvLock` is a thread-bound, re-entrant global lock for legacy tests that -//! mutate process-global environment or current-working-directory state. It -//! prevents those mutations from interfering while the legacy callers run. -//! The seam is retired and remains only until its callers migrate: use an -//! injected `mockable::Env` for environment-variable access, and the existing -//! working-directory seam, absolute paths, or `-C/--directory` for CWD access. -//! Issue #494 tracks removal. Add no callers or tests; the -//! [ADR-008](../../docs/adr-008-environment-seam-taxonomy.md) records the -//! decision. - -use std::cell::RefCell; -use std::marker::PhantomData; -use std::rc::Rc; -use std::sync::{Mutex, MutexGuard}; -use std::{fmt, fmt::Formatter}; - -/// Global mutex serializing all environment mutations. -static ENV_LOCK: Mutex<()> = Mutex::new(()); - -thread_local! { - static ENV_LOCK_STATE: RefCell = const { RefCell::new(LockState { - depth: 0, - guard: None, - }) }; -} - -/// Per-thread bookkeeping for the global environment lock. -struct LockState { - /// Number of live `EnvLock` guards on this thread. - depth: usize, - /// The held mutex guard, present only while `depth` is non-zero. - guard: Option>, -} - -/// RAII guard that holds the global environment lock. -/// -/// The guard is thread-bound because its underlying mutex guard is stored in -/// thread-local state. Moving it to another thread would leave the acquiring -/// thread's lock depth and guard out of sync. -/// -/// ```compile_fail -/// use test_support::env_lock::EnvLock; -/// -/// let guard = EnvLock::acquire(); -/// std::thread::spawn(move || drop(guard)); -/// ``` -pub struct EnvLock { - /// Marker making the guard `!Send`, keeping the lock thread-bound. - _not_send: PhantomData>, -} - -impl fmt::Debug for EnvLock { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - f.debug_struct("EnvLock").finish_non_exhaustive() - } -} - -impl EnvLock { - /// Acquire the global lock serializing environment mutations. - #[must_use] - pub fn acquire() -> Self { - ENV_LOCK_STATE.with(|lock_state| { - let mut state_ref = lock_state.borrow_mut(); - if state_ref.depth == 0 { - state_ref.guard = Some( - ENV_LOCK - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner), - ); - } - state_ref.depth += 1; - }); - Self { - _not_send: PhantomData, - } - } -} - -impl Drop for EnvLock { - fn drop(&mut self) { - ENV_LOCK_STATE.with(|lock_state| { - let mut state_ref = lock_state.borrow_mut(); - state_ref.depth = state_ref.depth.saturating_sub(1); - if state_ref.depth == 0 { - drop(state_ref.guard.take()); - } - }); - } -} - -#[cfg(test)] -mod tests { - //! Unit tests for reentrant and contended environment locking. - - use super::*; - use proptest::prelude::{Just, Strategy, prop_oneof}; - use std::{sync::mpsc, time::Duration}; - - /// Read the thread-local lock state without asserting. - /// - /// Callers holding a live [`EnvLock`] need this: asserting while the guard - /// is alive would drop it during unwind, and dropping a `MutexGuard` while - /// panicking poisons the mutex. - fn current_thread_lock_is_held() -> bool { - ENV_LOCK_STATE.with(|lock_state| { - let state = lock_state.borrow(); - state.depth > 0 && state.guard.is_some() - }) - } - - fn assert_current_thread_lock_is_released(message: &str) { - ENV_LOCK_STATE.with(|lock_state| { - let state = lock_state.borrow(); - assert!(state.depth == 0 && state.guard.is_none(), "{message}"); - }); - } - - #[test] - fn reentrant_env_lock_nested_acquire_and_release() { - { - let _outer = EnvLock::acquire(); - let _inner = EnvLock::acquire(); - } - - // Observations are captured while the guards live and asserted only once - // they are released: a failing assertion under a live guard would drop it - // mid-unwind and poison `ENV_LOCK`, burying the real cause. - let outer = EnvLock::acquire(); - let held_with_nested = { - let _inner = EnvLock::acquire(); - current_thread_lock_is_held() - }; - let held_after_nested_drop = current_thread_lock_is_held(); - - drop(outer); - assert!( - held_with_nested, - "ENV_LOCK should remain locked while nested EnvLock guards are alive" - ); - assert!( - held_after_nested_drop, - "ENV_LOCK should remain locked until the outer EnvLock guard is dropped" - ); - assert_current_thread_lock_is_released( - "ENV_LOCK should be unlocked after final EnvLock guard is dropped", - ); - } - - #[test] - fn reentrant_env_lock_stays_locked_when_outer_drops_first() { - let outer = EnvLock::acquire(); - let inner = EnvLock::acquire(); - - drop(outer); - let held_with_inner_alive = current_thread_lock_is_held(); - - drop(inner); - assert!( - held_with_inner_alive, - "ENV_LOCK should remain locked while an inner EnvLock guard is alive" - ); - assert_current_thread_lock_is_released( - "ENV_LOCK should be unlocked after the final out-of-order guard drops", - ); - } - - #[test] - fn env_lock_serializes_contending_threads_until_release() { - let outer = EnvLock::acquire(); - let (attempting_tx, attempting_rx) = mpsc::channel(); - let (acquired_tx, acquired_rx) = mpsc::channel(); - let contender = std::thread::spawn(move || { - assert!( - attempting_tx.send(()).is_ok(), - "main test thread should await the contention attempt" - ); - let _guard = EnvLock::acquire(); - assert!( - acquired_tx.send(()).is_ok(), - "main test thread should await lock acquisition" - ); - }); - - assert!( - attempting_rx.recv_timeout(Duration::from_secs(1)).is_ok(), - "contending thread should attempt lock acquisition" - ); - assert_eq!( - acquired_rx.recv_timeout(Duration::from_millis(20)), - Err(mpsc::RecvTimeoutError::Timeout), - "contending thread must remain blocked while the outer guard lives" - ); - - drop(outer); - assert!( - acquired_rx.recv_timeout(Duration::from_secs(1)).is_ok(), - "contending thread should acquire the lock after release" - ); - assert!(contender.join().is_ok(), "contending thread should finish"); - } - - /// One step of the generated transition sequence. - #[derive(Clone, Copy, Debug)] - enum LockOp { - Acquire, - /// Drop the guard at `selector`, clamped to the live range, covering - /// nested and out-of-order release; ignored while no guard is live. - Drop(usize), - } - - fn assert_state_matches(live_guards: usize) { - ENV_LOCK_STATE.with(|lock_state| { - let state = lock_state.borrow(); - assert_eq!( - state.depth, live_guards, - "thread-local depth must equal the number of live guards" - ); - assert_eq!( - state.guard.is_some(), - live_guards > 0, - "the mutex guard must be held exactly while guards are live" - ); - }); - } - - proptest::proptest! { - /// Any bounded interleaving of acquires and arbitrary-index drops - /// keeps the thread-local depth equal to the number of live guards, - /// and holds the underlying mutex exactly while that count is - /// non-zero. - #[test] - fn lock_state_tracks_any_acquire_and_drop_interleaving( - ops in proptest::collection::vec( - prop_oneof![ - Just(LockOp::Acquire), - (0usize..8).prop_map(LockOp::Drop), - ], - 1..24, - ), - ) { - let mut live: Vec = Vec::new(); - for op in ops { - match op { - LockOp::Acquire => live.push(EnvLock::acquire()), - LockOp::Drop(selector) => { - if let Some(last) = live.len().checked_sub(1) { - drop(live.remove(selector.min(last))); - } - } - } - assert_state_matches(live.len()); - } - - while let Some(guard) = live.pop() { - drop(guard); - assert_state_matches(live.len()); - } - assert_current_thread_lock_is_released( - "dropping every generated guard must release the lock", - ); - } - } - - /// Probes `ENV_LOCK` directly, so it requires per-test process isolation - /// (the suite runs under `cargo nextest`, which forks each test). Under a - /// thread-parallel runner it would race any other test touching the global - /// mutex. - /// - /// No assertion here can leave process-global poisoning behind, though the - /// reason differs either side of `ENV_LOCK.clear_poison()`: - /// - /// - The `join` assertion runs before the flag is cleared, so `ENV_LOCK` is - /// still poisoned at that point — deliberately, since that is the state - /// under test. It is safe regardless: the poisoned guard belonged to the - /// spawned thread, and this thread holds no `EnvLock`, so a failure has no - /// live guard to drop. - /// - The later assertions run once the flag is cleared, and the held state is - /// captured before its guard is dropped, so an unwinding assertion cannot - /// drop a live `MutexGuard` and poison the mutex afresh. - #[test] - fn env_lock_recovers_after_mutex_poisoning() { - let poisoner = std::thread::spawn(|| { - let _guard = EnvLock::acquire(); - panic!("poison ENV_LOCK deliberately"); - }); - - assert!(poisoner.join().is_err(), "poisoning thread should panic"); - let was_poisoned = ENV_LOCK.is_poisoned(); - ENV_LOCK.clear_poison(); - assert!(was_poisoned, "the panic should poison the underlying mutex"); - - let recovered_guard = EnvLock::acquire(); - let held_while_live = current_thread_lock_is_held(); - drop(recovered_guard); - assert!( - held_while_live, - "recovered EnvLock guard should hold ENV_LOCK" - ); - assert_current_thread_lock_is_released("recovered ENV_LOCK should be released normally"); - } -} diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 3fa2294fb..fa27b6694 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -8,8 +8,6 @@ //! - computing SHA-256 hashes for cache keys (hash module) //! - spawning lightweight HTTP servers for network tests (http module) //! - sandboxing PATH and HOME for the dev-fast target tests (`dev_fast` module) -//! - retaining the legacy `env_lock`/`EnvLock` seam until its callers migrate -//! ([ADR-008](../../docs/adr-008-environment-seam-taxonomy.md)) //! //! All items are intended for use in tests within this workspace; avoid using //! them in production code. @@ -19,12 +17,9 @@ pub mod check_ninja; pub mod command_helper; pub mod config_metrics; -pub mod cwd_guard; - #[cfg(unix)] pub mod dev_fast; pub mod env; -pub mod env_lock; pub mod exec; pub mod fixture; pub mod fluent; @@ -41,9 +36,6 @@ pub mod stdlib_assert; /// Re-export the SHA-256 helper for concise call sites. pub use hash::sha256_hex; -/// Re-export of [`cwd_guard::CwdGuard`] for ergonomics in tests. -pub use cwd_guard::CwdGuard; - /// Re-export localizer helpers for integration tests. pub use localizer::{ EnLocalizer, LocalizerGuard, en_localizer, localizer_test_lock, set_en_localizer, diff --git a/test_support/src/localizer.rs b/test_support/src/localizer.rs index ee38a01a2..daecaa972 100644 --- a/test_support/src/localizer.rs +++ b/test_support/src/localizer.rs @@ -141,8 +141,7 @@ pub fn en_localizer() -> EnLocalizer { // guards nothing but the ordering of localizer installation, and // `set_en_localizer` below re-establishes the global state unconditionally, // so recovering the guard is safe. Panicking here would instead fail every - // subsequent test that takes this fixture. `crate::env_lock` recovers from - // poisoning the same way. + // subsequent test that takes this fixture. let lock = localizer_test_lock().unwrap_or_else(PoisonError::into_inner); EnLocalizer { _guard: RestoreProbe::new(set_en_localizer()), diff --git a/tests/bdd/steps/ir.rs b/tests/bdd/steps/ir.rs index 86e9a53b6..806a91e4c 100644 --- a/tests/bdd/steps/ir.rs +++ b/tests/bdd/steps/ir.rs @@ -213,8 +213,9 @@ fn graph_target_implicit_deps(world: &TestWorld, target: &str, paths: &str) -> R /// Compile a manifest file to IR, storing result or error in state. fn compile_manifest_impl(world: &TestWorld, path: &str) { - // The manifest path is absolute, while the process CWD stays at the - // project root, so relative glob patterns continue to resolve correctly. + // The manifest path is absolute; manifest parsing injects the manifest + // directory as the glob base, so relative glob patterns resolve against + // that directory regardless of the process working directory. let resolved = if std::path::Path::new(path).is_relative() && path.starts_with("tests/") { let manifest_dir = env!("CARGO_MANIFEST_DIR"); std::path::Path::new(manifest_dir) diff --git a/tests/bdd/steps/manifest/mod.rs b/tests/bdd/steps/manifest/mod.rs index 43a51a0f7..11ca7af99 100644 --- a/tests/bdd/steps/manifest/mod.rs +++ b/tests/bdd/steps/manifest/mod.rs @@ -68,8 +68,9 @@ pub(super) fn get_string_from_string_or_list( } fn parse_manifest_inner(world: &TestWorld, path: &ManifestPath) { - // Convert relative test data paths to absolute while the process CWD stays - // at the project root, where relative glob patterns resolve correctly. + // Convert relative test data paths to absolute; manifest parsing injects + // the manifest directory as the glob base, so relative glob patterns + // resolve against that directory rather than the process working directory. let manifest_path = if std::path::Path::new(path.as_str()).is_relative() && path.as_str().starts_with("tests/") { diff --git a/tests/clippy_env_policy_ui_tests.rs b/tests/clippy_env_policy_ui_tests.rs new file mode 100644 index 000000000..82a0223bd --- /dev/null +++ b/tests/clippy_env_policy_ui_tests.rs @@ -0,0 +1,75 @@ +//! Contract coverage for the Clippy environment-mutation configuration. +//! +//! The configuration disallows every process-global environment mutation in +//! each workspace crate. The `lint-clippy` target invokes Clippy across the +//! workspace, all target kinds, and all features, covering production and +//! test compilation surfaces without duplicating Clippy's own lint tests. + +use anyhow::{Context, Result, ensure}; +use camino::{Utf8Path, Utf8PathBuf}; +use test_support::fs as test_fs; +use toml::Value; + +/// List process-global environment mutations that the project forbids. +const FORBIDDEN_GLOBAL_ENV_MUTATIONS: [&str; 3] = [ + "std::env::set_var", + "std::env::remove_var", + "std::env::set_current_dir", +]; + +/// List per-crate Clippy configurations that enforce the policy. +const CLIPPY_POLICY_FILES: [&str; 2] = ["clippy.toml", "test_support/clippy.toml"]; + +/// Return a repository-relative path rooted at the workspace manifest. +fn repository_path(path: &str) -> Utf8PathBuf { + Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(path) +} + +/// Return disallowed method paths from one Clippy configuration file. +fn disallowed_method_paths(policy_path: &Utf8Path) -> Result> { + let policy: Value = test_fs::read_to_string(policy_path) + .with_context(|| format!("read Clippy policy at {policy_path}"))? + .parse() + .with_context(|| format!("parse Clippy policy at {policy_path}"))?; + let methods = policy + .get("disallowed-methods") + .and_then(Value::as_array) + .context("Clippy policy should declare disallowed-methods")?; + Ok(methods + .iter() + .filter_map(|method| method.get("path").and_then(Value::as_str)) + .map(str::to_owned) + .collect()) +} + +/// Keep every workspace Clippy policy configured for global mutation bans. +#[test] +fn clippy_configurations_disallow_every_global_environment_mutation() -> Result<()> { + for policy_file in CLIPPY_POLICY_FILES { + let policy_path = repository_path(policy_file); + let paths = disallowed_method_paths(&policy_path)?; + for required_path in FORBIDDEN_GLOBAL_ENV_MUTATIONS { + ensure!( + paths.iter().any(|path| path == required_path), + "{policy_file} must disallow {required_path}, found {paths:?}" + ); + } + } + Ok(()) +} + +/// Keep the Clippy gate scoped to every workspace target and feature. +#[test] +fn clippy_gate_covers_every_workspace_target_and_feature() -> Result<()> { + let makefile = test_fs::read_to_string(repository_path("Makefile"))?; + ensure!( + makefile + .contains("CLIPPY_FLAGS ?= --workspace --all-targets --all-features -- -D warnings"), + "CLIPPY_FLAGS must cover every workspace target and feature with warnings denied" + ); + ensure!( + makefile.contains("$(CARGO) clippy $(CLIPPY_FLAGS)"), + "lint-clippy must invoke Cargo Clippy with the workspace-wide contract" + ); + Ok(()) +} diff --git a/tests/config_discovery_e2e_tests.rs b/tests/config_discovery_e2e_tests.rs index 125f208c2..9dec04444 100644 --- a/tests/config_discovery_e2e_tests.rs +++ b/tests/config_discovery_e2e_tests.rs @@ -1,8 +1,12 @@ -//! End-to-end configuration discovery failure coverage. +//! End-to-end configuration-discovery coverage through the real binary. //! -//! These tests run the real binary in a child process with a closed -//! environment, proving that missing configuration still permits the normal -//! workflow while a malformed discovered file fails before manifest handling. +//! These tests run `netsuke` in a child process with a closed environment and +//! an explicit invocation directory, proving the explicit-selector contract of +//! ADR-014: a relative `--config ` resolves against the process working +//! directory even when `-C/--directory` is supplied; an absolute selector is +//! unchanged. The +//! parent process environment and working directory are never mutated; all +//! child-process configuration flows through the `Command` builders. use anyhow::{Context, Result, ensure}; use assert_cmd::cargo::cargo_bin_cmd; @@ -76,31 +80,40 @@ fn malformed_discovered_config_fails_the_binary_workflow() -> Result<()> { Ok(()) } -/// Assert that an explicit selector does not rebase beneath `-C`. -fn assert_explicit_relative_config_ignores_directory_anchor( +/// Prove explicit-selector independence from `-C` through the binary. +/// +/// The child runs from `invocation` with `-C project` and an explicit +/// selector. The invocation-directory copy enables early JSON output while +/// the `-C`-anchored copy does not, so the response shape names which file +/// loaded. A relative selector must load the invocation-directory copy; +/// an absolute selector must also remain unchanged. +fn assert_explicit_config_selection( selector: ExplicitSelector, selector_path_kind: SelectorPathKind, project_name: &str, config_name: &str, ) -> Result<()> { - let outer = tempdir().context("create invoking directory")?; - let project = outer.path().join(project_name); + let invocation = tempdir().context("create invoking directory")?; + let project = invocation.path().join(project_name); test_fs::create_dir(&project).context("create directory-anchored project")?; test_fs::copy("tests/data/minimal.yml", project.join("Netsukefile")) .context("write project manifest")?; - test_fs::write(outer.path().join(config_name), "json = true\n") - .context("write invoking-directory config")?; - test_fs::write(project.join(config_name), "json = false\n") + // The invocation-directory copy wins for every explicit selector. The + // `-C`-anchored copy is a decoy, while the invocation copy enables early + // JSON output so the response shape names which file loaded. + test_fs::write(invocation.path().join(config_name), "json = true\n") + .context("write invocation-directory config")?; + test_fs::write(project.join(config_name), "color = \"never\"\n") .context("write directory-anchored config")?; - let outer_path = utf8_workspace_path(&outer)?; + let invocation_path = utf8_workspace_path(&invocation)?; let selector_path = match selector_path_kind { - // Relative explicit selectors stay anchored to the child CWD. + // A relative explicit selector remains relative to the child CWD. SelectorPathKind::Relative => Utf8PathBuf::from(config_name), - // Absolute explicit selectors remain unchanged. - SelectorPathKind::Absolute => outer_path.join(config_name), + // An absolute explicit selector remains unchanged. + SelectorPathKind::Absolute => invocation_path.join(config_name), }; - let mut command = isolated_netsuke_command(&outer_path); + let mut command = isolated_netsuke_command(&invocation_path); command.args(["-C", project_name]); match selector { ExplicitSelector::Cli => { @@ -119,8 +132,12 @@ fn assert_explicit_relative_config_ignores_directory_anchor( output.status.success(), "generate should succeed: {output:?}" ); - let document: Value = serde_json::from_slice(&output.stdout) - .context("explicit selected config should enable JSON output")?; + let ninja = String::from_utf8_lossy(&output.stdout).into_owned(); + // The invocation-directory copy set `json = true`, so stdout is the JSON + // envelope around the generated artefact. This proves `-C` did not rebase + // the explicit relative selector onto its decoy. + let document: Value = serde_json::from_str(&ninja) + .with_context(|| format!("explicit selector should load the JSON config: {ninja}"))?; ensure!( document .pointer("/result/content") @@ -131,10 +148,10 @@ fn assert_explicit_relative_config_ignores_directory_anchor( Ok(()) } -/// An explicit relative CLI selector stays anchored to the child process CWD. +/// A relative CLI selector ignores `-C/--directory`. #[test] fn cli_explicit_relative_config_ignores_directory_anchor() -> Result<()> { - assert_explicit_relative_config_ignores_directory_anchor( + assert_explicit_config_selection( ExplicitSelector::Cli, SelectorPathKind::Relative, "project", @@ -142,10 +159,10 @@ fn cli_explicit_relative_config_ignores_directory_anchor() -> Result<()> { ) } -/// A relative environment selector stays anchored to the child process CWD. +/// A relative environment selector ignores `-C/--directory`. #[test] fn environment_explicit_relative_config_ignores_directory_anchor() -> Result<()> { - assert_explicit_relative_config_ignores_directory_anchor( + assert_explicit_config_selection( ExplicitSelector::Environment, SelectorPathKind::Relative, "project", @@ -156,7 +173,7 @@ fn environment_explicit_relative_config_ignores_directory_anchor() -> Result<()> /// An absolute CLI selector remains unchanged when `-C` is present. #[test] fn cli_explicit_absolute_config_ignores_directory_anchor() -> Result<()> { - assert_explicit_relative_config_ignores_directory_anchor( + assert_explicit_config_selection( ExplicitSelector::Cli, SelectorPathKind::Absolute, "project", @@ -167,7 +184,7 @@ fn cli_explicit_absolute_config_ignores_directory_anchor() -> Result<()> { /// An absolute environment selector remains unchanged when `-C` is present. #[test] fn environment_explicit_absolute_config_ignores_directory_anchor() -> Result<()> { - assert_explicit_relative_config_ignores_directory_anchor( + assert_explicit_config_selection( ExplicitSelector::Environment, SelectorPathKind::Absolute, "project", @@ -175,12 +192,53 @@ fn environment_explicit_absolute_config_ignores_directory_anchor() -> Result<()> ) } +/// Without `-C`, a relative explicit selector resolves against the process +/// working directory. +#[test] +fn cli_explicit_relative_config_without_directory_uses_working_directory() -> Result<()> { + let invocation = tempdir().context("create invoking directory")?; + test_fs::copy( + "tests/data/minimal.yml", + invocation.path().join("Netsukefile"), + ) + .context("write invocation manifest")?; + test_fs::write(invocation.path().join("relative.toml"), "json = true\n") + .context("write invocation-directory config")?; + + let invocation_path = utf8_workspace_path(&invocation)?; + let output = isolated_netsuke_command(&invocation_path) + .arg("--config") + .arg("relative.toml") + .arg("generate") + .output() + .context("run generate with an unanchored relative config")?; + + ensure!( + output.status.success(), + "generate should succeed: {output:?}" + ); + let document: Value = serde_json::from_slice(&output.stdout).with_context(|| { + format!( + "the relative selector should load the JSON invocation-directory config: {}", + String::from_utf8_lossy(&output.stdout) + ) + })?; + ensure!( + document + .pointer("/result/content") + .and_then(Value::as_str) + .is_some(), + "JSON output should contain the generated Ninja artefact: {document}", + ); + Ok(()) +} + proptest! { #![proptest_config(ProptestConfig::with_cases(32))] - /// Generated selector paths preserve explicit-selection anchoring. + /// Generated selector paths preserve the ADR-014 independence contract. #[test] - fn explicit_config_never_rebases_under_directory( + fn explicit_config_selection_ignores_directory_anchor( selector in prop_oneof![ Just(ExplicitSelector::Cli), Just(ExplicitSelector::Environment), @@ -193,7 +251,7 @@ proptest! { config_stem in "[a-z]{1,12}", ) { let config_name = format!("{config_stem}.toml"); - let result = assert_explicit_relative_config_ignores_directory_anchor( + let result = assert_explicit_config_selection( selector, selector_path_kind, &project_name, diff --git a/tests/data/glob.yml b/tests/data/glob.yml index e0cfa4b18..4c784b841 100644 --- a/tests/data/glob.yml +++ b/tests/data/glob.yml @@ -1,5 +1,5 @@ netsuke_version: 1.0.0 targets: - - foreach: glob('tests/data/glob_files/*.txt') - name: "{{ item | replace('tests/data/glob_files/', '') | replace('.txt', '.out') }}" + - foreach: glob('glob_files/*.txt') + name: "{{ item | replace('glob_files/', '') | replace('.txt', '.out') }}" command: "echo {{ item }}" diff --git a/tests/data/glob_windows.yml b/tests/data/glob_windows.yml index 984183df3..a4d61be7a 100644 --- a/tests/data/glob_windows.yml +++ b/tests/data/glob_windows.yml @@ -1,5 +1,5 @@ netsuke_version: 1.0.0 targets: - - foreach: glob('tests\\data\\glob_files\\*.txt') - name: "{{ item | replace('tests/data/glob_files/', '') | replace('.txt', '.out') }}" + - foreach: glob('glob_files\\*.txt') + name: "{{ item | replace('glob_files/', '') | replace('.txt', '.out') }}" command: "echo {{ item }}" diff --git a/tests/env_path_tests.rs b/tests/env_path_tests.rs index 21914b1c0..569b59a4f 100644 --- a/tests/env_path_tests.rs +++ b/tests/env_path_tests.rs @@ -3,7 +3,7 @@ //! Composition is pure (`test_support::env::prepend_path_value`) and the //! runner applies the result as data via `CommandEnv`, so nothing here //! mutates the parent process: no test carries `#[serial]` and none needs -//! `EnvLock`. +//! process-global environment or working-directory coordination. //! //! These are the named cases; the invariants they instantiate live in //! `env_path_property_tests.rs`, which Cargo builds as its own target. diff --git a/tests/makefile_test_target.rs b/tests/makefile_test_target.rs index 37a3333be..456415a65 100644 --- a/tests/makefile_test_target.rs +++ b/tests/makefile_test_target.rs @@ -3,7 +3,7 @@ //! //! `make test` is the single command local development and continuous //! integration (CI) both run. These tests pin the runner contract it encodes: -//! non-doctest tests go through cargo-nextest, and doctests run separately +//! non-doctest tests go through cargo-nextest and doctests run separately //! because nextest cannot execute them. //! //! They also pin the `RUSTFLAGS` contract shared by every recipe that sets the @@ -24,21 +24,20 @@ mod makefile; use anyhow::{Context, Result, ensure}; use camino::Utf8Path; -use makefile::{read_repo_file, target_prerequisites, target_recipe}; +use makefile::{phony_targets, read_repo_file, target_prerequisites, target_recipe}; use toml::Value; +/// Verify that `make test` orders the nextest pass before the doctest pass. #[test] fn behavioural_make_test_composes_the_nextest_and_doctest_passes() -> Result<()> { let makefile = read_repo_file(Utf8Path::new("Makefile"))?; let prerequisites = target_prerequisites(&makefile, "test").context("Makefile should declare a test target")?; - for expected in ["test-nextest", "doctest"] { - ensure!( - prerequisites.iter().any(|name| name == expected), - "make test should depend on {expected}, found {prerequisites:?}" - ); - } + ensure!( + prerequisites == ["test-nextest", "doctest"], + "make test must depend on nextest and doctests, found {prerequisites:?}" + ); let nextest_recipe = target_recipe(&makefile, "test-nextest") .context("Makefile should declare a test-nextest target")?; @@ -84,6 +83,55 @@ fn behavioural_make_test_composes_the_nextest_and_doctest_passes() -> Result<()> Ok(()) } +/// Keep the glob-expansion benchmark available through the Makefile. +#[test] +fn benchmark_glob_expansion_target_is_phony_and_runs_the_expected_bench() -> Result<()> { + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + let phony = phony_targets(&makefile); + ensure!( + phony.contains(&"bench-glob-expansion"), + ".PHONY must include bench-glob-expansion, found {phony:?}" + ); + let recipe = target_recipe(&makefile, "bench-glob-expansion") + .context("Makefile should declare a bench-glob-expansion recipe")?; + ensure!( + recipe.contains("$(CARGO) bench --bench glob_expansion"), + "bench-glob-expansion must invoke the glob_expansion bench, found {recipe:?}" + ); + Ok(()) +} + +/// Verify that the formatter recipe handles an empty Markdown file set portably. +#[test] +fn check_fmt_portably_skips_markdown_validation_without_files() -> Result<()> { + let makefile = read_repo_file(Utf8Path::new("Makefile"))?; + let recipe = + target_recipe(&makefile, "check-fmt").context("Makefile should declare check-fmt")?; + ensure!( + !recipe + .lines() + .filter(|line| line.contains("xargs")) + .flat_map(str::split_whitespace) + .any(|argument| { + argument == "-r" + || argument == "--no-run-if-empty" + || argument.strip_prefix('-').is_some_and(|short_flags| { + !short_flags.starts_with('-') && short_flags.contains('r') + }) + }), + "check-fmt must not rely on GNU-only xargs -r, found {recipe:?}" + ); + ensure!( + recipe.contains("if [ \"$$#\" -gt 0 ]"), + "check-fmt must guard against an empty Markdown input, found {recipe:?}" + ); + ensure!( + recipe.contains("scripts/check-markdown-format.sh \"$$@\""), + "check-fmt must validate every discovered Markdown path, found {recipe:?}" + ); + Ok(()) +} + #[path = "makefile_test_target/rustflags.rs"] mod rustflags; diff --git a/tests/manifest_glob_tests/capability_scope.rs b/tests/manifest_glob_tests/capability_scope.rs index 12bdb37df..8d26e7a4f 100644 --- a/tests/manifest_glob_tests/capability_scope.rs +++ b/tests/manifest_glob_tests/capability_scope.rs @@ -9,7 +9,6 @@ use super::{manifest_yaml, target_names, temp_dir}; use anyhow::{Context, Result, ensure}; use rstest::rstest; use std::path::Path; -use test_support::{cwd_guard::CwdGuard, env_lock::EnvLock}; /// Build a manifest with one `foreach` target per glob match. fn glob_manifest(pattern: &str) -> String { @@ -52,15 +51,17 @@ fn unopenable_prefix_yields_no_targets( /// A parent-relative pattern expands against the working directory. #[rstest] fn parent_relative_pattern_expands(temp_dir: tempfile::TempDir) -> Result<()> { + // Loading the manifest through its own path anchors relative glob + // patterns to the manifest's workspace root (`sub/`), so `../*.txt` + // ascends to the temporary directory. This preserves the parent-relative + // coverage without changing the process working directory. let sub = temp_dir.path().join("sub"); test_support::fs::create_dir(&sub)?; test_support::fs::write(temp_dir.path().join("out.txt"), "out")?; + let manifest_path = sub.join("Netsukefile"); + test_support::fs::write(&manifest_path, glob_manifest("../*.txt"))?; - let _lock = EnvLock::acquire(); - let _guard = CwdGuard::acquire()?; - std::env::set_current_dir(&sub).context("switch to the subdirectory")?; - - let manifest = netsuke::manifest::from_str(&glob_manifest("../*.txt"))?; + let manifest = netsuke::manifest::from_path(&manifest_path)?; ensure!( target_names(&manifest)? == vec!["../out.txt".to_owned()], "expected the parent-relative match" diff --git a/tests/support/makefile.rs b/tests/support/makefile.rs index f4ad62162..f2731dd02 100644 --- a/tests/support/makefile.rs +++ b/tests/support/makefile.rs @@ -123,6 +123,21 @@ pub fn target_prerequisites(contents: &str, target: &str) -> Option> }) } +/// Returns every target declared by a single-line `.PHONY` directive. +/// +/// # Examples +/// +/// ``` +/// assert_eq!(phony_targets(".PHONY: test lint\n"), ["test", "lint"]); +/// ``` +pub fn phony_targets(contents: &str) -> Vec<&str> { + contents + .lines() + .filter_map(|line| line.strip_prefix(".PHONY:")) + .flat_map(str::split_whitespace) + .collect() +} + /// Returns the tab-indented recipe lines for `target`, joined by newlines. /// /// A target with no recipe yields an empty string; an absent target yields @@ -157,7 +172,9 @@ mod tests { //! These also keep every helper used from each including crate, so a //! consumer needing only part of the surface does not trip `dead_code`. - use super::{parse_rule, read_repo_file, repo_root, target_prerequisites, target_recipe}; + use super::{ + parse_rule, phony_targets, read_repo_file, repo_root, target_prerequisites, target_recipe, + }; use camino::Utf8Path; const SAMPLE: &str = concat!( @@ -207,6 +224,11 @@ mod tests { assert_eq!(target_prerequisites(SAMPLE, "missing"), None); } + #[test] + fn phony_targets_reads_each_declared_target() { + assert_eq!(phony_targets(SAMPLE), ["alpha", "beta"]); + } + #[test] fn target_recipe_spans_blank_lines_and_stops_at_the_next_rule() { assert_eq!(