diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4c207d78efba..17fcfd0b6074 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -949,6 +949,9 @@ jobs: - name: Import Great Expectations run: python -c "import great_expectations as gx; print('Successfully imported GX Version:', gx.__version__)" + - name: Check installed agent skills and schema catalogs + run: python ci/checks/check_installed_agent_skills.py + ci-required: # Single required status check for branch protection. # Passes when every upstream job either succeeded or was skipped diff --git a/ci/checks/check_installed_agent_skills.py b/ci/checks/check_installed_agent_skills.py new file mode 100644 index 000000000000..09ff7e0db0c0 --- /dev/null +++ b/ci/checks/check_installed_agent_skills.py @@ -0,0 +1,206 @@ +""" +Purpose: guard the installed package against silently dropping the bundled agent +skills or their schema catalogs. + +The skills and the two catalog indexes they depend on all ship through +`package_data` glob patterns rather than through code, which means nothing enforces +that the patterns stay in sync with what actually lives in the source tree: a pattern +narrowed by an unrelated edit, or a file added under a directory the patterns do not +reach, fails silently. `pip install .` and `import great_expectations` both succeed +either way, so nothing in the ordinary import-and-run check catches it. + +This script is meant to run after `pip install .`, against the resulting installed +package, and checks the properties a user actually depends on: + +* both bundled skills resolve the way an installed package resolves them -- through + the import system, not by checking that some files happen to exist; +* both schema catalog indexes are present, and each schema tree ships more than just + its index; +* the `skills list` subcommand names both skills; +* installing from the running package produces content that matches its own + ownership manifest; +* every file the source tree bundles for a skill actually made it into the installed + package, not just the files that happen to make the skill resolve. + +Run directly with `python ci/checks/check_installed_agent_skills.py` from the +repository root, with the package already installed in the active environment. +""" + +from __future__ import annotations + +import io +import sys +import tempfile +from contextlib import redirect_stdout +from pathlib import Path +from typing import Final + +import great_expectations +from great_expectations import __main__ as command_line +from great_expectations.agent_skills import installer +from great_expectations.agent_skills.installer import ( + SkillTarget, + install_skills, + iter_bundled_skills, + read_skill_manifest, +) + +#: The skills this package currently bundles, named explicitly rather than merely +#: counted -- a rename or a dropped skill is then reported by name instead of as an +#: unexplained count mismatch. +EXPECTED_SKILLS: Final = frozenset({"gx-configure-data-source", "gx-configure-expectations"}) + +#: The schema trees the package ships alongside the skills, each carrying its own +#: catalog index, relative to the installed package root. +SCHEMA_TREES: Final = ( + Path("expectations", "core", "schemas"), + Path("datasource", "fluent", "schemas"), +) + +INDEX_NAME: Final = "index.json" + + +def check_bundled_skills_resolve() -> list[Path]: + """Both skills must be found the way an installed package is found: through the + import system, not by checking that some files happen to exist. + + ``iter_bundled_skills`` is what every install and list run relies on to locate the + skills, and it is also what raises when a packaging pattern ships some of a + skill's files and drops others -- the shape a too-narrow glob produces. Calling it + here, rather than checking paths by hand, is what makes this a check on skill + *resolution*. + """ + skills = sorted(iter_bundled_skills(), key=lambda skill: skill.name) + names = {skill.name for skill in skills} + assert names == EXPECTED_SKILLS, ( + f"expected the installed package to bundle {sorted(EXPECTED_SKILLS)}, found {sorted(names)}" + ) + for skill in skills: + assert (skill / "SKILL.md").is_file(), f"{skill} has no SKILL.md" + return skills + + +def check_catalog_indexes(installed_root: Path) -> None: + """Both catalog indexes must ship at their documented location.""" + for tree in SCHEMA_TREES: + index = installed_root / tree / INDEX_NAME + assert index.is_file(), f"{index} was not found in the installed package" + + +def check_schema_counts_nonzero(installed_root: Path) -> dict[str, int]: + """Each schema tree must ship more than just its index.""" + counts: dict[str, int] = {} + for tree in SCHEMA_TREES: + directory = installed_root / tree + count = sum(1 for path in directory.rglob("*.json") if path.name != INDEX_NAME) + assert count > 0, f"no schema JSON files were found under {directory}" + counts[tree.as_posix()] = count + return counts + + +def check_skills_list_names_both(project_root: Path) -> str: + """The ``skills list`` subcommand must name both skills, run the way a user runs it. + + Invoked in-process through the same entry point ``python -m great_expectations`` + calls, rather than shelled out to, so the exact code path a user runs is exercised + without depending on how the interpreter running this script happens to be found. + """ + buffer = io.StringIO() + with redirect_stdout(buffer): + exit_code = command_line.main(["skills", "list", "--project-root", str(project_root)]) + output = buffer.getvalue() + assert exit_code == 0, f"'skills list' exited {exit_code}:\n{output}" + for name in EXPECTED_SKILLS: + assert name in output, f"'skills list' did not mention {name}:\n{output}" + return output + + +def check_installed_digests_match_manifest(project_root: Path) -> None: + """Installing from the running package must produce content matching its own + ownership manifest. + + That match is what lets a later run tell an untouched install apart from one the + user edited. Checking it here, against a genuinely packaged and installed + distribution, covers a case the test suite cannot reach: the suite exercises a + fixture tree or a source checkout under an editable install, neither of which is + the artifact a user actually receives. + + Hashing is done with the installer's own ``_tree_digest`` rather than a second, + independently written function: the manifest's ``content_sha256`` field is defined + as that function's output, so the only way to ask "does this destination still + match what its manifest recorded" is to recompute the same function and compare -- + a differently framed hash would disagree with the manifest even for byte-identical + content, and this check would fail on every run rather than only on a real + regression. That is not circular, because the two hashes are taken over different + trees: the manifest's value is computed from the installed package's own bundled + directory, while this recomputes over the copy placed in the project. Equality is + therefore a real property of the install pipeline -- that ``shutil.copytree`` + reproduced the source directory byte for byte. + """ + report = install_skills(project_root, targets=(SkillTarget.AGENTS, SkillTarget.CLAUDE)) + assert not report.failed, ( + f"installing into a scratch project reported failures: {report.failed}" + ) + assert report.installed, "installing into a scratch project installed nothing" + for destination in report.installed: + manifest = read_skill_manifest(destination) + assert manifest is not None, f"{destination} has no ownership manifest after install" + recorded = manifest.get("content_sha256") + actual = installer._tree_digest(destination) + assert actual == recorded, ( + f"{destination} hashes to {actual}, but its manifest records {recorded} -- the " + "installed content does not match what was written down for it" + ) + + +def check_every_bundled_file_shipped(source_root: Path, installed_root: Path) -> None: + """Every file under the source skills tree must exist at the same relative path in + the installed package. + + Every other check here can pass while a packaging pattern still drops a file that + is neither a ``SKILL.md`` nor a markdown reference -- a script or an image added to + a skill directory, say -- because nothing else compares the two trees file for + file. ``iter_bundled_skills`` would not notice: it only requires ``SKILL.md``. + """ + missing = [ + path.relative_to(source_root).as_posix() + for path in sorted(source_root.rglob("*")) + if path.is_file() + and not path.is_symlink() + and not (installed_root / path.relative_to(source_root)).is_file() + ] + assert not missing, ( + f"these files exist under {source_root} but were not found in the installed package " + f"at {installed_root} -- check the packaging patterns for the skills tree: {missing}" + ) + + +def main() -> None: + installed_root = Path(great_expectations.__file__).resolve().parent + repo_root = Path(__file__).resolve().parents[2] + source_skills_root = repo_root / "great_expectations" / ".agents" / "skills" + + try: + skills = check_bundled_skills_resolve() + check_catalog_indexes(installed_root) + counts = check_schema_counts_nonzero(installed_root) + + with tempfile.TemporaryDirectory(prefix="gx-installed-skills-guard-") as scratch: + project_root = Path(scratch) + check_skills_list_names_both(project_root) + check_installed_digests_match_manifest(project_root) + + check_every_bundled_file_shipped(source_skills_root, skills[0].parent) + except (AssertionError, OSError) as error: + print(f"[ERROR] {error}") + sys.exit(1) + + schema_summary = ", ".join(f"{count} under {tree}" for tree, count in counts.items()) + print( + f"Installed agent skills are complete: {len(skills)} skills " + f"({', '.join(sorted(skill.name for skill in skills))}), schemas {schema_summary}." + ) + + +if __name__ == "__main__": + main() diff --git a/docs/adr/0006-ship-agent-skills-with-the-package.md b/docs/adr/0006-ship-agent-skills-with-the-package.md new file mode 100644 index 000000000000..8a54b8bac357 --- /dev/null +++ b/docs/adr/0006-ship-agent-skills-with-the-package.md @@ -0,0 +1,134 @@ +# 6. Ship agent skills with the package + +Date: 2026-08-13 + +## Status + +Accepted + +## Context + +Data practitioners increasingly configure and validate data through a coding +agent rather than by writing every line of Python themselves. An agent's +general programming knowledge does not tell it the current, correct sequence +of calls for a specific library: which factory method to call for a given +connection type, in what order a validation suite has to be registered before +expectations are added to it, or how to handle a secret without ever printing +it to the conversation. Left to infer this from the source or from +out-of-date training data, an agent produces plausible-looking code that is +subtly wrong at least as often as it produces working code, and a user who +does not already know the right pattern has no way to tell the two apart. + +Closing that gap requires guidance that an agent can actually find and use. +That means it has to live where an agent's tooling already looks, in a form +the agent's platform already knows how to read, and it has to stay accurate +for whatever version of the library the user has installed — guidance +written against an API that has since changed is worse than no guidance, +because it is confidently wrong instead of visibly absent. + +## Decision + +We ship a set of "skills" — self-contained guidance documents for a coding +agent — as part of the `great_expectations` distribution, and give users a +command to place them where their agent looks for them. + +**Format.** Each skill is a directory containing one entry document, plus +supporting reference material one directory level below it. This is an open +format, not something specific to Great Expectations: multiple coding-agent +tools already read directories shaped this way, so publishing skills in this +form makes them usable by every agent whose platform speaks the format, +without our writing a separate integration per agent. A proprietary or +single-vendor shape would have bought nothing for the additional maintenance +of yet another format, and would have worked with only one agent. + +**Location.** The skill content lives inside the installed package itself, +not behind a URL the agent fetches at runtime and not something generated on +demand. The reason is version matching: the correct guidance for calling a +fluent factory method or registering a suite is a function of the exact +`great_expectations` release installed, and an install of the package is the +one artifact guaranteed to carry the version the guidance has to match. A +separately hosted copy can drift out of sync with any given install the +moment either one changes independently, silently handing an agent +instructions for an API surface that no longer matches what is on disk. +Shipping the content in the package ties its version to the code's version by +construction, so the normal act of installing or upgrading the package is +also what keeps the guidance current. + +**Command surface.** The command to place the bundled skills into a project +is invoked as a module, `python -m great_expectations …`, rather than through +a new console-script entry point installed onto the user's `PATH`. Great +Expectations previously shipped a console-script command-line interface and +removed it. Reintroducing one — even a minimal one — brings back the +packaging-level machinery a console script requires and the platform-specific +quirks of `PATH`-installed executables (name collisions, `PATH` not being set +up in every environment a Python package is used from, different behavior +across virtual environments and editable installs), to serve what is, in +substance, an occasional local file-management step for a library that is +not a command-line application. `python -m` needs none of that: it uses the +same import machinery already required to use the library at all, so it +behaves identically in every environment where `import great_expectations` +already works. + +**Install model.** The command places the bundled skills, by copying or on +request by linking, into the discovery directories a project's coding agent +reads — `.agents/skills` for Codex and Cursor, `.claude/skills` for Claude +Code and Cursor — alongside a small manifest recording what was installed and +a hash of its content. Several principles follow from treating the +destination as belonging to the user, not to the package: + +- A destination that already holds exactly what would be installed is left + completely alone. The manifest is what lets a repeat run tell "nothing to + do" apart from "something changed" without guessing from the file contents + alone, which is what makes the command safe to run again after every + upgrade, or simply on the suspicion that it was never run at all. +- The tool never silently overwrites something it did not create. A + directory with no record of having been installed by this package is left + alone unconditionally — there is no option that overwrites it — because + nothing in a directory it never wrote can be told apart from a user's own + work. +- Once a directory carries that record, the tool can tell its own untouched + copy apart from one the user has since edited, and refuses to replace the + latter without an explicit override. A command meant to be safe to run + again after every upgrade cannot also be a command that discards local + edits as a side effect of checking for updates. +- An upgrade builds the replacement in full beside the destination and moves + it into place, never rewriting files where they sit. A process that dies + partway through therefore never leaves behind a skill with some files at the + new version and some at the old — an agent reading a directory in that state + would follow guidance that no single release ever actually shipped, which is + worse than the outdated version it was replacing. + +## Consequences + +An agent whose platform reads this open format gets accurate, version-matched +guidance the moment the package is installed and the install command is run, +with no bespoke integration effort on our part and none required of the +agent's maintainers. The same content is available to any other tool that +scans installed packages for it, at no additional cost, because it sits at a +predictable path inside the package rather than behind custom retrieval +logic. + +The guidance now has to be kept in step with the fluent API it describes, the +same way any other part of the package does, or it degrades into the exact +failure mode — instructions for an API that no longer matches what is +installed — that shipping it in-package was meant to prevent. + +The install copies by default, so a package upgrade alone does not update +guidance already placed in a project; the install command has to be run again +to pick up a new version. That default exists because copying is the only +form every platform this content runs on is known to treat the same way a +real directory is treated, and the only one that survives the package being +upgraded or removed. Re-running the install command is the price of that +reliability, and it is cheap precisely because re-running it is always safe. +Linking directly to the package's own copy is available for users who want +guidance that tracks the installed version without re-running anything; +choosing it accepts, in exchange, that a project's guidance can change +without an explicit action, and that on a platform that will not create +links at all it is reported as a failure rather than falling back silently +to a copy. + +There is no globally installed executable to remember; the command is only +reachable through `python -m`, which requires knowing the package is +installed in the environment being used — a smaller surface than a +console script, and one that trades a small amount of discoverability for +never depending on how a user's `PATH` happens to be configured. diff --git a/great_expectations/.agents/skills/gx-configure-data-source/SKILL.md b/great_expectations/.agents/skills/gx-configure-data-source/SKILL.md new file mode 100644 index 000000000000..f7bf59de9bb5 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-data-source/SKILL.md @@ -0,0 +1,392 @@ +--- +name: gx-configure-data-source +description: Set up Great Expectations data access end to end — connect a data source, define a data asset, add a batch definition, and verify it by actually reading data through it. Use when a user asks to connect Great Expectations to their data (files, a SQL database or warehouse, or an in-memory dataframe), to add an asset or batch definition to a project they already have, or when other Great Expectations work is blocked because no working batch definition exists yet. +license: Apache-2.0 +--- + +# Configure a Great Expectations data source + +This skill takes a user from "here is my data" to a **verified working batch +definition** — a named, saved way of pulling a specific slice of their data +that you have proven works by actually reading through it. + +Three objects, in order, each built on the one before: + +- A **data source** holds the connection: a directory, a connection string, or + an in-memory handle. +- A **data asset** names the logical collection of data within it: a table, a + query, a family of files, a dataframe. +- A **batch definition** selects how much of that asset a single operation + reads: the whole thing, or one time window of it. + +Everything here goes through Great Expectations' public configuration API and +produces ordinary project artifacts. Nothing depends on this skill being +present afterwards. + +## The flow + +1. **Preflight** — find out which project (or in-memory session) you are + operating on, and tell the user. See `references/preflight.md`. +2. **Elicit** — the source type, the connection details, the asset, and the + batching cadence. +3. **Configure** — data source, then asset, then batch definition, each with + the reuse-first pattern below. +4. **Verify** — retrieve a batch and probe it. Retrieval alone proves nothing. +5. **Report** — say what was built and where it lives; offer write-out if the + session is in memory. + +Do not skip step 1, and do not stop before step 4. A configuration that was +never read through is not a result worth reporting. + +## Step 1 — Preflight + +Follow `references/preflight.md` in full before configuring anything. It +establishes whether you are working against a project on disk or an in-memory +session, tells you what to announce to the user, and covers the environment +problems that silently masquerade as "no project found". + +The outcome you carry forward is a `context` object and one fact: whether the +session is file-backed or in memory. Both are fully supported paths. + +## Step 2 — Elicit what you need + +Four things, in this order. Ask for them together where you can; don't +interrogate the user one field at a time. + +**1. Which data source type.** Read the shipped catalog rather than working +from memory — see `references/datasource-catalog.md`. It gives you the exact +factory name for each type, the arguments that type accepts, and which of them +are mandatory. It also covers how to steer the conversation when the user +describes their data rather than naming a backend. + +**2. The connection details** the chosen type's schema marks as required. Read +the secrets rule below before you write any of them down. + +**3. What the asset is.** A table name, a SQL query, a file-name pattern, or a +dataframe variable in the user's session. Which of these are available depends +on the type; the catalog reference enumerates them. + +**4. The batching cadence.** Whole collection, or sliced by day, month, or +year. Ask what the natural unit of the user's data is — if they check data +daily, a daily batch definition matches how they work. If the data has no +usable date column, the whole collection is the right answer and there is +nothing to apologize for. + +### Secrets: templates only, never values + +Connection strings routinely carry passwords, tokens, and account +identifiers. Great Expectations already has a mechanism for this, and it is +the only one to use. + +**Put a `${VARIABLE_NAME}` reference in the configuration; never the literal +value.** The reference is what gets stored, and it is resolved at connection +time: + +```python +connection_string = "postgresql+psycopg2://${DB_USER}:${DB_PASSWORD}@warehouse.internal:5432/analytics" +``` + +Three rules follow from this, and none of them bend: + +- **Never write a literal secret into a configuration, a file, or a message + back to the user.** Not as a placeholder, not "just for testing", not + inlined to get past an error. If you are holding a secret value, the only + correct thing to do with it is tell the user which environment variable to + put it in. +- **Never echo a resolved secret value.** Report the template + (`${DB_PASSWORD}`), never what it resolved to. +- **If a required credential is missing, say which variable is missing and how + to set it — then stop.** Do not guess a value, do not substitute a default, + and do not fall back to an unauthenticated connection. + +**Where the variable is read from depends on the session**, and the split +matters: + +- An **in-memory session** resolves `${VARIABLE_NAME}` from process + environment variables only. It has no project on disk, so it has no + uncommitted config file to read from. If the user wants to supply the value, + it has to be an environment variable. +- A **file-backed project** reads environment variables *and* an uncommitted + config-variables file inside the project directory. Either works; the + uncommitted file is the option for values that should be available to + anyone opening that project. + +Name whichever applies when you ask for a credential, so the user knows where +to put it. `references/write-out.md` covers what changes when an in-memory +session later becomes a project. + +## Step 3 — Configure, reusing what already exists + +The user may be adding to a project that already has some of this configured — +possibly from a teammate, a previous conversation, or an earlier run of this +same flow. **Fetch each object before creating it**, and create only what is +missing. + +Adding a duplicate is not an option the API offers: `add__asset` and +`add_batch_definition_*` raise `ValueError: "" already exists` on a +second call with the same name. There is no `add_or_update_*` variant for +assets or batch definitions. + +All three fetch calls signal absence with a `LookupError` subclass, so a +single `except LookupError` is the correct catch for each: + +| Fetch | Raises when missing | +| --- | --- | +| `context.data_sources.get(name)` | `KeyError` (a `LookupError`) | +| `datasource.get_asset(name)` | `LookupError` | +| `asset.get_batch_definition(name)` | `KeyError` (a `LookupError`) | + +### Never "update" a data source that already exists + +**`add_or_update_` replaces a data source wholesale.** It does not merge. +Calling it against a name that already exists drops every asset and batch +definition attached to that data source — including ones this session never +created. A run that "updates" a data source in order to add one asset to it +destroys the user's other assets silently: no error, no warning, and the flow +carries on and reports success. + +Verified directly: a data source carrying assets `['orders', 'products']` has +`[]` assets immediately after a second `add_or_update_sqlite` under the same +name. + +So: + +- Call the data-source factory **at most once per flow**, before any asset + step, and **only when the data source does not already exist**. +- If the user genuinely wants to change the connection configuration of an + existing data source, tell them first, in plain terms, that every asset and + batch definition on it will be dropped and have to be rebuilt — then let + them decide. + +Note also that the factory **tests the connection as part of the call**, so it +can be slow or hang on an unreachable host. Run it inside the duration-tracked +wrapper in `references/robustness.md` like any other data-touching call. + +### The pattern + +```python executable +DATASOURCE_NAME, ASSET_NAME, BATCH_DEFINITION_NAME = "warehouse", "orders", "by_month" + +# --- data source: create only if absent; never replace an existing one --- +try: + datasource = context.data_sources.get(DATASOURCE_NAME) +except LookupError: + datasource = context.data_sources.add_or_update_sqlite( + name=DATASOURCE_NAME, + connection_string="sqlite:///${WAREHOUSE_PATH}", + ) + +# --- asset: reuse, or delete and re-add when its configuration must change --- +replace_asset = False # set True only if the user wants different asset config +try: + asset = datasource.get_asset(ASSET_NAME) + asset_exists = True +except LookupError: + asset_exists = False + +if asset_exists and replace_asset: + datasource.delete_asset(ASSET_NAME) + asset_exists = False +if not asset_exists: + asset = datasource.add_table_asset(name=ASSET_NAME, table_name="orders") + +# --- batch definition: same shape --- +replace_batch_definition = False +try: + batch_definition = asset.get_batch_definition(BATCH_DEFINITION_NAME) + batch_definition_exists = True +except LookupError: + batch_definition_exists = False + +if batch_definition_exists and replace_batch_definition: + asset.delete_batch_definition(BATCH_DEFINITION_NAME) + batch_definition_exists = False +if not batch_definition_exists: + batch_definition = asset.add_batch_definition_monthly( + name=BATCH_DEFINITION_NAME, column="ordered_at" + ) +``` + +Two things to keep intact when you adapt this: + +- **The fetch and the create are separate statements, not a create inside the + `except` of the fetch's own `try`.** Keeping them apart means a failure in + the create is reported as a create failure rather than being swallowed by + the same handler. +- **Deleting and re-adding an asset affects only that asset.** Its siblings on + the same data source are untouched — this is exactly why the delete-and-add + pattern is safe for assets and batch definitions while the data-source + factory is not. + +Set `replace_asset` / `replace_batch_definition` from what the user actually +asked for. If they want a different table, different files, or a different +partitioning column than what is already configured under that name, the +existing object has to be replaced. If they described what is already there, +reuse it and say so. + +## Step 4 — Verify by reading through it + +**Retrieving a batch does not prove the configuration works.** For a SQL query +or table asset, building the batch touches nothing — `get_batch()` returns a +real `Batch` object even when the table does not exist. Reporting success on +that basis means telling the user their setup works when it does not. + +Always follow retrieval with a probe that actually reads data: + +```python executable +batch = batch_definition.get_batch() # add batch_parameters=... for a partitioned definition +head = batch.head(n_rows=5) # this is the step that touches the data +``` + +`head` is a small object with a `.data` attribute holding a pandas DataFrame +of the sampled rows; printing it renders the rows directly. + +**Run this probe inside the duration-tracked, exception-catching wrapper in +`references/robustness.md`.** Do not write your own — that reference already +handles the parts that are easy to get wrong: checking in with the user while +a slow query is still running rather than after it returns, never cancelling +work that continues to run and bill on the data platform, and recovering the +real database error out of the bare `KeyError` a broken probe raises. + +Four outcomes, and three of them are not failures: + +- **The probe returns rows** — the batch definition works. Proceed to step 5. +- **The probe returns zero rows**, with the expected column names — the + configuration works and the underlying collection is empty. This is what an + empty table or empty dataframe looks like: `head` returns a frame with the + right columns and no rows. Say both things plainly. Do not report it as a + configuration failure, and do not start changing the configuration to make + rows appear. +- **`get_batch()` raises `NoAvailableBatchesError: No available batches + found.`** — the configuration works, but the *window you asked for* holds no + data. Both partitioned SQL assets and file-based assets fail this way when + the requested year/month matches nothing, and it is the normal answer for a + window outside the data's range. Report it as an empty window, name the + batch parameters you used, and offer to try a window that exists — not as a + broken setup. +- **The probe raises anything else** — the configuration does not work. Report + it per `references/robustness.md`'s rules: what failed, why as far as it is + known, one concrete next step. A broken table or query surfaces here as a + bare `KeyError` with no readable message; that reference explains how to + recover the real database error behind it. Do not report success, and do not + retry with different parameters hoping something sticks. + +## Step 5 — Report, and offer write-out when in memory + +Tell the user, concretely: + +- The names of the data source, asset, and batch definition, and which of them + you created versus reused. +- Where the configuration lives: the project's configuration directory for a + file-backed session, or a plain statement that the session is in memory and + nothing is saved yet. +- That the batch definition is verified, and what the probe returned — a row + count and the columns is usually enough. Never paste a secret or a resolved + credential into this report. +- How to retrieve a batch again, including the `batch_parameters` the batch + definition needs. + +**If the session is in memory, offer to write it out** to a real project so +the work survives — see `references/write-out.md` for the procedure and for +what the user needs to know about dataframe assets, which carry configuration +but no data. Offer it; don't do it unprompted, and don't pick the location. + +## Where this flow ends + +**The verified batch definition is the end state.** Do not build a suite of +"smoke test" expectations to prove the setup works — the probe in step 4 has +already proven it, an invented expectation asserts something the user never +asked for, and a failing one would report a data-quality problem that is +really just a guess of yours. + +When the user wants to assert things about their data, that is expectation +work: hand off to the `gx-configure-expectations` skill with the batch +definition you just verified. If they ask for both in one breath, finish this +flow, report it, and then move on. + +## Worked examples + +Each of these shows the object chain — data source, asset, batch definition, +verified batch — for a **fresh** setup where none of these names exist yet. +Preflight comes first in all three, and step 3's fetch-first pattern still +applies: the moment there is any chance the project already holds a data +source by that name, wrap these calls in it rather than copying them as they +stand, or the data-source call will drop assets that are already there. + +Take the factory names and arguments for the user's actual backend from +`references/datasource-catalog.md`. + +### A file-based source: monthly CSV files + +The date lives in the file name, so the monthly batch definition takes a +`regex` with named groups. **Batch parameters for file-based definitions must +be strings** — passing `2024` instead of `"2024"` raises +`InvalidBatchRequestError`. + +```python +datasource = context.data_sources.add_or_update_pandas_filesystem( + name="sales_files", + base_directory="/data/sales", +) +asset = datasource.add_csv_asset(name="monthly_sales") +batch_definition = asset.add_batch_definition_monthly( + name="by_month", + regex=r"sales_(?P\d{4})-(?P\d{2})\.csv", +) + +batch = batch_definition.get_batch(batch_parameters={"year": "2024", "month": "02"}) +print(batch.head(n_rows=5)) +``` + +### A SQL source: a table partitioned by month + +The date lives in a column, so the monthly batch definition takes `column`. +**Batch parameters here are integers**, unlike the file-based case above. The +credential-bearing part of the connection string is a `${VARIABLE_NAME}` +reference, never a literal. + +```python +datasource = context.data_sources.add_or_update_postgres( + name="warehouse", + connection_string="postgresql+psycopg2://${DB_USER}:${DB_PASSWORD}@warehouse.internal:5432/analytics", +) +asset = datasource.add_table_asset(name="orders", table_name="orders") +batch_definition = asset.add_batch_definition_monthly(name="by_month", column="ordered_at") + +batch = batch_definition.get_batch(batch_parameters={"year": 2024, "month": 3}) +print(batch.head(n_rows=5)) +``` + +A query asset is the alternative when the logical collection is not a whole +table — `datasource.add_query_asset(name=..., query=...)`. Use it to express +what the data *is*, not to bolt a `LIMIT` onto a slow table; see +`references/robustness.md` for why row-limited query assets are a last-resort +exploration tool rather than a batching mechanism. + +### An in-memory dataframe + +A dataframe asset stores configuration only — the data itself is handed over +at retrieval time, every time, in this session and in every future one: + +```python executable +datasource = context.data_sources.add_or_update_pandas(name="in_memory") +asset = datasource.add_dataframe_asset(name="customers") +batch_definition = asset.add_batch_definition_whole_dataframe(name="all_rows") + +batch = batch_definition.get_batch(batch_parameters={"dataframe": df}) +print(batch.head(n_rows=5)) +``` + +Say this out loud to the user when you configure one: the dataframe is not +saved, and anything reading this asset later must supply a dataframe itself. + +## References + +- `references/preflight.md` — establishing and announcing the session context. +- `references/datasource-catalog.md` — every configurable type, its factory, + its arguments, and its asset and batch-definition surface, read from the + shipped catalog. +- `references/robustness.md` — the time-budget wrapper, scope-reduction + levers, and how to report a failure helpfully. +- `references/write-out.md` — turning an in-memory session into a real + project. diff --git a/great_expectations/.agents/skills/gx-configure-data-source/references/datasource-catalog.md b/great_expectations/.agents/skills/gx-configure-data-source/references/datasource-catalog.md new file mode 100644 index 000000000000..3267886aa8e1 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-data-source/references/datasource-catalog.md @@ -0,0 +1,226 @@ +# The datasource catalog: finding types, factories, and asset surfaces + +Great Expectations ships a machine-readable catalog of every data source type +it can configure, alongside the code, inside the installed package. Read it at +runtime. **Never work from a memorized or hand-written list of types** — one +goes stale the moment a release adds a backend, and a wrong guess sends the +user down a path that doesn't exist. The catalog is generated from the same +code that defines the factories, so it is correct for the version that is +actually installed in front of you. + +Everything below is derived from files inside the installed package. There is +nothing to download and no network call. + +## Locating the catalog + +The catalog lives under `datasource/fluent/schemas/` in the installed +`great_expectations` package. Reach it through `importlib.resources` rather +than by building a filesystem path from `great_expectations.__file__` — that +works whether the package is installed normally, installed in editable mode, +or imported from a zipped distribution: + +```python +import json +from importlib.resources import files + +SCHEMAS = files("great_expectations") / "datasource" / "fluent" / "schemas" +``` + +Three kinds of file live there: + +- `index.json` — the type index: one entry per configurable data source type. +- `Datasource.json` — one connection schema per type, naming the + arguments its factory accepts. +- `Datasource/` — a sibling directory per type, holding one schema per + asset type that data source supports. + +## Step 1: the type index gives you the factory name + +`index.json` maps each connection schema filename to the **exact** name of the +factory method on `context.data_sources` that creates that type: + +```python +index = json.loads((SCHEMAS / "index.json").read_text()) + +for schema_file, factory in sorted(index.items()): + print(f"{schema_file[:-len('Datasource.json')]:<28} context.data_sources.{factory}(...)") +``` + +**Read the factory name out of the index; never derive it from the type +name.** There is no naming rule that holds — the index exists precisely +because the mapping is irregular in both directions. `BigQueryDatasource.json` +maps to `add_or_update_bigquery`, not `add_or_update_big_query`; +`PandasAzureBlobStorageDatasource.json` maps to `add_or_update_pandas_abs`. +Any snake-casing rule you might infer from the regular cases will silently +produce a method name that doesn't exist for the irregular ones, and the +failure surfaces as a confusing `AttributeError` rather than "no such data +source type". + +Use the index for two things: to answer "what can Great Expectations connect +to?" for the user, and to turn the type they pick into a call you can actually +make. + +## Step 2: the connection schema gives you the arguments + +Once the user has picked a type, its `Datasource.json` schema names +exactly what its factory accepts and which of those are mandatory: + +```python +schema_file = "PostgresDatasource.json" # whichever type the user picked +schema = json.loads((SCHEMAS / schema_file).read_text()) + +print("factory: context.data_sources." + index[schema_file]) +print("required:", schema["required"]) +print("accepted:", sorted(schema["properties"])) +``` + +For `PostgresDatasource.json` that prints: + +```text +factory: context.data_sources.add_or_update_postgres +required: ['name', 'connection_string'] +accepted: ['assets', 'connection_string', 'create_temp_table', 'id', 'kwargs', 'name', 'type'] +``` + +`required` is what you must elicit from the user before you can make the call. +Treat `assets`, `id`, and `type` as reserved: `assets` is populated by the +asset factories rather than passed in, `id` is assigned, and `type` is implied +by the factory you chose. Each entry in `properties` carries its own +`description` and `type` where the source defines one — read those to the user +when they ask what a particular argument means, instead of guessing. + +`required` is a floor, not the whole story: some optional fields are filled in +from other values rather than left empty. A SQL table asset, for example, +lists only `name` as required, and its `table_name` defaults to the asset +name. If a default like that would be wrong for the user's data, pass the +field explicitly. + +## Step 3: the sibling directory gives you the asset types + +Each type's asset surface is the set of JSON files in the directory named +after its connection schema. Every asset schema carries the asset's type token +under `properties.type`, and the factory that creates it is **`add__asset` +on the data source object** — that derivation is exact for every asset schema +the package ships, unlike the data-source factory names in step 1: + +```python +asset_dir = SCHEMAS / schema_file.removesuffix(".json") + +for entry in sorted(p.name for p in asset_dir.iterdir() if p.name.endswith(".json")): + asset_schema = json.loads((asset_dir / entry).read_text()) + asset_type = asset_schema["properties"]["type"]["enum"][0] + print(f"datasource.add_{asset_type}_asset(...) required={asset_schema['required']}") +``` + +For `PostgresDatasource` that prints: + +```text +datasource.add_query_asset(...) required=['name', 'query'] +datasource.add_table_asset(...) required=['name'] +``` + +The same call against `PandasFilesystemDatasource` enumerates its file-format +assets (`add_csv_asset`, `add_parquet_asset`, `add_excel_asset`, and so on), +and against `PandasDatasource` it includes `add_dataframe_asset`. Read the +directory rather than assuming which formats a given backend supports — the +file-based backends do not all carry the same set, and the SQL backends carry +dialect-specific asset types in some cases. + +## Step 4: the asset object gives you the batch-definition surface + +Batch definitions are the last link in the chain, and their available shapes +depend on the asset you actually created. Ask the asset object directly, after +you have it: + +```python +batch_definition_factories = sorted( + name + for name in dir(asset) + if name.startswith("add_batch_definition_") +) +print(batch_definition_factories) +``` + +Three shapes come up constantly, and they show the pattern: + +| Asset | Available `add_batch_definition_*` | +| --- | --- | +| A file-format asset (e.g. a CSV asset) | `_daily`, `_monthly`, `_yearly`, `_path` | +| A SQL table or query asset | `_daily`, `_monthly`, `_yearly`, `_whole_table` | +| A dataframe asset | `_whole_dataframe` | + +Note the `startswith("add_batch_definition_")` filter, with the trailing +underscore. It deliberately excludes the bare `add_batch_definition` method, +which takes a partitioner object you would have to construct and import +yourself. Always go through the named `add_batch_definition_` factories +instead — they build the partitioner for you, and they are the supported way +to express batching. + +The time-based factories (`_daily`, `_monthly`, `_yearly`) need to know which +value to slice on, and that differs by asset family: a SQL asset takes the +`column` to partition on, while a file-format asset infers the date from the +file path and takes a `regex` describing it. The whole-collection shapes +(`_whole_table`, `_whole_dataframe`) take only a `name`; `_path` takes a name +and the specific `path` to pin the batch to. Don't guess between them — read +the factory's own signature, which is authoritative for the installed version +and costs nothing: + +```python +import inspect + +print(inspect.signature(asset.add_batch_definition_monthly)) +``` + +## Choosing a type with the user + +The catalog answers "what is possible"; the user answers "what do you have". +Three families cover the landscape, and naming them is usually enough to get a +decision quickly: + +- **Files** — data sitting in a directory, a bucket, or a filesystem, read + through pandas or Spark. Look for the types whose names carry a storage + location (filesystem, S3, Google Cloud Storage, Azure Blob Storage, DBFS). +- **SQL** — data in a database or warehouse, reached by connection string. + These types name the engine or dialect, plus a generic SQL type for anything + reachable through a SQLAlchemy connection string that has no dedicated type. +- **Dataframes** — data already in memory in the user's own process, as a + pandas or Spark dataframe. Use this when the user has the data loaded + already, and remember that the configuration written for it carries no data: + the dataframe is supplied per batch at retrieval time. + +If the user's backend has no dedicated type, the generic SQL type is the +fallback for anything with a SQLAlchemy connection string — say so plainly +rather than reporting the backend as unsupported. If it genuinely isn't +reachable any of these ways, say that too, and don't improvise a substitute. + +## When a type needs a driver or credentials it doesn't have + +Many types depend on an optional driver package, a client library, or ambient +credentials that Great Expectations does not install or provide. Adding the +data source is where this surfaces, because the factory tests the connection +as part of the call — so a type that is present in the catalog is not thereby +proven usable in this environment. + +The failure arrives as a `TestConnectionError` wrapping the underlying cause. +How informative that wrapped cause is varies a lot by backend, and it is worth +expecting both shapes: + +- **Precise and actionable**, which is the common case. Connecting to BigQuery + with no application credentials configured, for example, reports + `SQLAlchemyCreateEngineError("Unable to create SQLAlchemy Engine: due to + DefaultCredentialsError('Your default credentials were not found. ...')")`, + including a documentation link. Relay a cause like this close to verbatim — + it is more specific than anything you could paraphrase. +- **Terse to the point of empty.** Some backends surface only an exception type + with no message at all — a Spark data source on a machine with no working + Java runtime raises `TestConnectionError: ... due to PySparkRuntimeError()`, + where the actual problem (Java is missing) appeared only as unstructured + output from a subprocess. When the cause is this thin, say so honestly: + name the backend, say the connection test failed without a usable message, + and point at the most likely environment prerequisite for that backend + rather than inventing a specific diagnosis. + +Either way, follow `robustness.md`'s rule for reporting a failure — what +failed, why as far as it is known, and one concrete next step — and stop +there. Installing packages or provisioning credentials in the user's +environment is their call, not yours. diff --git a/great_expectations/.agents/skills/gx-configure-data-source/references/preflight.md b/great_expectations/.agents/skills/gx-configure-data-source/references/preflight.md new file mode 100644 index 000000000000..e3b3c4ae0597 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-data-source/references/preflight.md @@ -0,0 +1,142 @@ +# Session preflight + +Before configuring anything, establish what you're operating on: a project the +user already has on disk, or a temporary in-memory session. Do this exactly +once at the start of the flow, and tell the user the outcome before you do +anything else. + +## Enter with `cloud_mode=False` + +Call the context factory like this, not with a bare call: + +```python executable +import great_expectations as gx + +context = gx.get_context(cloud_mode=False) +``` + +A managed cloud offering that this factory can auto-connect to has been +retired. If a machine still carries leftover configuration for it (environment +variables, or a leftover config file), a bare `gx.get_context()` — and +`cloud_mode=True` — will detect that configuration, try to honor it, and raise +immediately with an error to that effect. That failure has nothing to do with +the user's local project or data; it's a stale-environment problem, and it +would happen before you ever got to look at a local project. Passing +`cloud_mode=False` explicitly skips that detection and always resolves to a +local file-backed project if one is found, or an in-memory session otherwise. + +**Check for stale cloud configuration yourself before you call it**, because +`cloud_mode=False` silently discards that configuration rather than reporting +it — there is no signal in the return value or in normal output that it was +there. Look for `GX_CLOUD_ACCESS_TOKEN`, `GX_CLOUD_ORGANIZATION_ID`, or +`GX_CLOUD_BASE_URL` in the environment. If any are set, tell the user plainly: +those variables were found but ignored, because that offering is no longer +reachable and today they should either unset them or ignore this message — +they have no effect on the session you're about to build. + +## Interpret what comes back + +`gx.get_context(cloud_mode=False)` returns one of two things. Branch on the +type: + +```python executable +from great_expectations.data_context import FileDataContext + +if isinstance(context, FileDataContext): + context_root = context.root_directory + # tell the user: "Using the existing project's configuration at + # ." +else: + # tell the user: "No project found — working in a temporary, in-memory + # session. Nothing here is saved until it's written out to a project + # (see write-out.md)." + ... +``` + +**A discovered project is not optional to announce.** Always state +`context_root` back to the user before doing anything else, so they know +exactly which project they're about to modify — this also makes +`add_or_update_*` updates against that project legible rather than a surprise. +Name it precisely as the project's *configuration directory*, not "the +project" on its own — `context_root` is the `gx` subdirectory that holds +`great_expectations.yml` and the stores, and its **parent** is the project +directory a user would think of as "the project". That parent is what +`project_root_dir` means everywhere it's accepted (`preflight.md`'s own +one-liner below, and `write-out.md`'s `gx.get_context(mode="file", +project_root_dir=...)`). Never feed `context_root` back in as a +`project_root_dir` — doing so nests a second `gx` directory inside the first +(`/gx/gx`) instead of reopening the same project. + +**An in-memory (ephemeral) session is not a degraded mode to apologize for.** +It's a normal, fully supported way to work — state plainly that the session is +temporary and that everything can be written out to a real project later (see +`write-out.md`). Do not stop, and do not treat the absence of a project as an +error condition. + +## Never scaffold a project yourself + +Standing up a new file-backed project is the user's decision, not something +to do on their behalf or without being asked. Two things follow from this: + +- Never call the file-context constructor without an explicit target + directory. Concretely: never call `gx.get_context(mode="file")` with no + `project_root_dir` — it does not raise or refuse when a project isn't found + at that implicit location; it silently creates one, into the current + working directory, with no confirmation. That is not something to do without + the user asking for it. +- If a user wants a new project created rather than an in-memory session, give + them the one-liner to run themselves and let them choose the location: + + ```python + context = gx.get_context(mode="file", project_root_dir="") + ``` + + Do not walk them through it interactively or pick the path for them. + +This applies during preflight. It does not apply to the write-out procedure in +`write-out.md`, which also calls `gx.get_context(mode="file", ...)` — there, +the target directory has already been confirmed with the user as an explicit +step, so the call is expected and disclosed rather than a silent side effect. + +## Never treat "no project" as a stop condition + +An in-memory session is a supported, first-class outcome of preflight, +covered above. Do not stop, refuse, or ask the user to go create a project +first just because discovery didn't find one — proceed with the ephemeral +session instead. + +## When discovery itself fails + +Two failure shapes are worth knowing by name, because neither produces an +obvious, self-explanatory error, and both are easy to misread as "no project +found" when they're actually a misconfiguration worth surfacing: + +**A stale `GX_HOME` environment variable.** If `GX_HOME` points at a directory +that either doesn't exist or doesn't contain a project config file, project +discovery does *not* raise — it silently falls back to the in-memory session, +exactly as if `GX_HOME` had never been set. If the user believes they have a +project and you get an in-memory session instead, this is the first thing to +check. Validate it yourself, since the factory won't: + +```python executable +import os +from pathlib import Path + +gx_home = os.environ.get("GX_HOME") +if gx_home is not None: + gx_home_path = Path(gx_home).expanduser() + if not (gx_home_path / "great_expectations.yml").is_file(): + # tell the user: "GX_HOME is set to , but no project config + # was found there. If you expected an existing project to be used, + # check the path — otherwise this variable can be ignored/unset." + ... +``` + +**Stale cloud configuration**, covered above — check for it before calling +the factory, since the factory itself won't tell you it was there. + +In both cases, the same shape of report applies: state what looked +misconfigured, state which value or file caused it, and give one concrete next +step (fix the path, unset the variable, or proceed with the in-memory +session). Never let a silent fallback pass as "everything's fine" when the +environment suggests the user expected otherwise. diff --git a/great_expectations/.agents/skills/gx-configure-data-source/references/robustness.md b/great_expectations/.agents/skills/gx-configure-data-source/references/robustness.md new file mode 100644 index 000000000000..bfa871b9b5cb --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-data-source/references/robustness.md @@ -0,0 +1,340 @@ +# Robustness: time budgets, scope reduction, and reading results correctly + +Real data is large, slow, and occasionally broken in ways that don't produce +clean errors. This document covers three things every data-touching step in +this skill needs: a time budget that doesn't pretend it can cancel anything, +concrete levers for cutting a query down to size, and how to read a result +correctly so an infrastructure failure never gets reported as a data-quality +finding. + +## The advisory time budget is a check-in, not a kill switch + +Wrap any potentially slow call — testing a connection, retrieving a batch, +probing it, running a validation — so you can check in with the user *while +it is still running*, not only after it returns. A budget measured after the +call has already completed can't do this — by the time you'd act on it, there +is nothing left to check in about. Instead, run the call on a worker thread +and poll it with a timeout, so you get control back at regular intervals +while the real call is still in flight: + +```python executable +import concurrent.futures +import time + +BUDGET_SECONDS = 60 # let the user adjust this for known-slow sources +POLL_SECONDS = 5 # how often to check whether the budget has passed + +executor = concurrent.futures.ThreadPoolExecutor(max_workers=1) +future = executor.submit(batch.head, n_rows=5) # or test_connection(), get_batch(), validate(), ... +start = time.monotonic() +checked_in = False +succeeded = False +result = None + +while True: + # Report state instead of racing an exception: on Python 3.11+, + # concurrent.futures.TimeoutError IS the builtin TimeoutError, so if the + # wrapped call itself raises a TimeoutError (a driver connect/statement + # timeout, for example — see lever 3 below), an `except + # concurrent.futures.TimeoutError` branch built on `future.result()` + # cannot tell "still running" apart from "finished, with that exact + # exception". Since a *finished* future's `.result()` returns instantly, + # that ambiguity turns into an unthrottled busy loop with no sleep in it. + # `concurrent.futures.wait(...)` sidesteps this entirely — it reports + # done/not-done without ever raising the wrapped call's own exception. + done, _ = concurrent.futures.wait([future], timeout=POLL_SECONDS) + elapsed = time.monotonic() - start + if not done: + # The real call has NOT returned yet — this branch runs while it is + # still executing on the worker thread, which is what makes an + # in-flight check-in possible at all. + if elapsed > BUDGET_SECONDS and not checked_in: + checked_in = True + # tell the user, right now, while the call is still running: + # "This has been running s, past the s + # budget, and is still going." Then follow the three steps below. + ... + continue + try: + result = future.result() + succeeded = True + except Exception as e: + # translate the exception — see "Reporting failures helpfully" below + result = None + break + +if succeeded and checked_in: + # tell the user it finished after all, at s, past the budget + ... +``` + +`succeeded` — not `result is not None` — is what distinguishes "the call +returned" from "the call failed": some wrapped calls (`test_connection()`, +for one) return `None` on success, so `result` alone can't tell the two +apart. + +**When the budget is exceeded, do not abandon the operation.** On most data +platforms, the query is running on the platform's own compute the moment it +was dispatched — closing the client connection or giving up on waiting does +not cancel it, and it keeps consuming (and costing) resources on the platform +regardless of whether anything is still waiting on the result. Killing the +client side only means losing visibility into a query that is still running +and still being billed. So the moment the budget check-in above fires: + +1. Tell the user the operation is still running and has passed the budget. +2. State plainly that it will keep running (and, on most platforms, keep + costing money) whether or not you keep waiting on it. +3. Ask whether to keep waiting, or to reduce the scope of data involved and + try again — never decide this unilaterally. + +**Keep the future alive; never cancel it.** Whatever the user picks, do not +call `future.cancel()` or `executor.shutdown(cancel_futures=True)` — the call +already dispatched to the platform's own compute, and cancelling the future +only makes this process stop watching it; it does not stop the remote work, +which is the point above about it continuing to run and cost money. If the +user chooses to keep waiting, stay in the polling loop shown above. If the +user chooses to reduce scope, stop polling this loop and leave `future` and +`executor` exactly as they are — still running, unattended — and submit the +reduced-scope attempt (using the levers below) as a new, separate call. Don't +block on the abandoned future first. + +## Scope-reduction levers, in preference order + +The goal each time is to make the specific operation touch less data, not to +change what the asset represents — an asset stays the durable, logical +collection of data a project points at; a batch definition is what selects +how much of it a given operation actually reads. + +**1. A narrower batch definition window, via the existing partitioner +factories.** This is the primary lever, and it should be tried first whenever +there's a usable date/time or otherwise partitionable column. Add (or reuse) a +partitioned batch definition, then request a specific window at fetch time: + +```python +# One-time setup: partition by month on a datetime column. +batch_definition = asset.add_batch_definition_monthly(name="monthly", column="event_time") + +# Per-attempt: fetch only one narrow window instead of the whole asset. +batch = batch_definition.get_batch(batch_parameters={"year": 2024, "month": 3}) +``` + +The equivalent daily/yearly variants exist too +(`add_batch_definition_daily`, `add_batch_definition_yearly`), as does a +directory-scoped daily/monthly/yearly split for file-based assets. Always +reach these through the asset's own `add_batch_definition_*` methods and +`batch_parameters` — never by constructing a partitioner object directly. + +**2. A row-limiting argument on the read itself, for local file-based +sources.** Where the underlying reader supports it (for example, a pandas +CSV asset), pass its native `nrows` option — a hard cap on how many rows get +read — as a keyword argument when adding the asset: + +```python +asset = pandas_datasource.add_csv_asset( + name="my_asset", + filepath_or_buffer="/path/to/large_file.csv", + nrows=1000, +) +``` + +This helps specifically with large local files rather than remote queries. + +**3. A driver-level connection or statement timeout**, for SQL sources. Pass +driver-specific timeout options through the datasource's `kwargs`, which are +forwarded straight to engine creation: + +```python +datasource = context.data_sources.add_or_update_postgres( + name="my_datasource", + connection_string="postgresql+psycopg2://${DB_USER}:${DB_PASSWORD}@host/db", + kwargs={"connect_args": {"connect_timeout": 10}}, +) +``` + +The exact keys are driver-specific (a `connect_timeout` for one driver may be +a different name for another) — this caps how long a *connection attempt* +can hang, not how much data a query returns, so treat it as a complement to +lever 1, not a substitute. + +**4. Last resort: a row limit on a query asset, for exploration only.** If no +column exists to partition on (no usable date/time or other partitionable +column), and the goal is just to inspect a sample of the data rather than +operate on the real batch, a query asset with an explicit `LIMIT` in its SQL +text is an acceptable stopgap: + +```python +asset = datasource.add_query_asset( + name="sample_events", + query="SELECT * FROM events LIMIT 1000", +) +``` + +Flag this to the user explicitly as temporary and exploration-only when you +use it. A query asset's row limit is baked into that one query's text — it +isn't a general batching mechanism, it doesn't compose with the partitioner +levers above, and it isn't something to reach for as a default way to make a +slow asset faster. If a project keeps needing this, the honest answer is that +the reduction lever it actually needs doesn't exist yet in the partitioner set +— that's a gap to name to the user, not one to paper over with query-asset +limits everywhere. + +## Reporting failures helpfully + +Whenever something in this flow raises, report three things and stop there — +never a raw traceback: **what failed** (the operation you were attempting), +**why, as far as it's known** (the clearest available cause), and **one +concrete next step** the user can take. + +Two exception shapes are common enough to call out by name: + +- **Connection and configuration failures** (a failed `test_connection()` + during `add_or_update_*`, or an import error for an optional driver + dependency) already carry an actionable message from Great Expectations + itself — relay it close to verbatim, plus one next step (fix the connection + string, install the missing driver package, check the credential). +- **A missing credential.** If a data operation fails because a referenced + `${ENV_VAR}` isn't set, say exactly which variable is missing and how to + provide it (set the environment variable, or add it to the project's + uncommitted config-variables file if working in a file-backed project). + Never hardcode a value or invent a placeholder to work around it. +- **A bare `KeyError` out of a batch probe.** Retrieving a batch off a broken + query or table does not itself fail — see "Why a retrieved batch doesn't + prove anything" below — but probing it can raise a plain `KeyError`, not a + Great Expectations exception, with no readable message of its own (its text + is just an internal cache key). The real underlying cause (the actual + database error, including which table or column it choked on) is emitted + during metric resolution as a **`WARNING`-level log record on the + `great_expectations.validator.metrics_calculator` logger** — not printed to + stdout. Attach a handler to that logger before the probe so you capture the + record directly, rather than trusting the `KeyError`'s own text or scanning + raw console output — this avoids depending on stdout/stderr being + visible, or on the *root* logger's own level. It does **not** avoid this + logger's own effective level: a handler only sees records the logger lets + past its own threshold, so if a host has raised + `great_expectations.validator.metrics_calculator` above `WARNING`, force it + down to `WARNING` for the duration of the probe and restore it afterward: + + ```python + import logging + import re + import ast + + class _CaptureHandler(logging.Handler): + def __init__(self): + super().__init__(level=logging.WARNING) + self.records: list[str] = [] + + def emit(self, record: logging.LogRecord) -> None: + self.records.append(record.getMessage()) + + def _extract_cause(message: str, ceiling: int = 500) -> str: + # The log message is a stringified dict of MetricConfigurationID -> + # {"exception_message": ..., "exception_traceback": ..., ...} — often + # several thousand characters, wrapping a full traceback with absolute + # filesystem paths. Never relay it verbatim (see the no-raw-traceback + # rule above). Pull out just the exception_message value(s); if the + # shape ever changes and the pattern doesn't match, fall back to a + # hard-truncated slice so a ceiling is guaranteed either way. + found = re.findall(r"'exception_message': (.+?), 'raised_exception':", message, re.DOTALL) + try: + # A match is not automatically a valid Python literal: an error text + # containing the terminator itself truncates the non-greedy match + # mid-literal. Fall through to truncation rather than raising from + # inside the error-reporting path. + cause = "; ".join(ast.literal_eval(m) for m in found) if found else message + except Exception: + cause = message + if len(cause) > ceiling: + cause = cause[:ceiling] + "... (truncated)" + return cause + + handler = _CaptureHandler() + metrics_logger = logging.getLogger("great_expectations.validator.metrics_calculator") + metrics_logger.addHandler(handler) + prior_level = metrics_logger.level + metrics_logger.setLevel(logging.WARNING) + try: + head = batch.head(n_rows=5) + except KeyError: + cause = _extract_cause(handler.records[-1]) if handler.records else "" + # report: the batch could not be read; cause holds the real database error + ... + finally: + metrics_logger.removeHandler(handler) + metrics_logger.setLevel(prior_level) + ``` + + Report to the user that the batch could not be read, relay the extracted + cause (a few hundred characters, not the raw log message), and point them + at checking the query or table name, and the connection, as the next step. +- **Resource exhaustion on a large dataset** (a `MemoryError`, or a + driver/engine error whose message names memory or disk space). This is not + a data-quality finding and not a connection problem — it means the + operation tried to hold more of the dataset in memory than the machine + running it has. Report plainly that it ran out of resources processing the + full dataset, then route straight to the scope-reduction levers above: a + narrower batch-definition window (lever 1) is the first thing to try, in + the same partitioner-first order used for a slow-but-successful call. + +## Why a retrieved batch doesn't prove anything, for SQL query assets + +`batch_definition.get_batch()` succeeds and returns a real `Batch` object even +when the underlying table or query is broken — for a SQL query asset, nothing +about building the batch touches the database. So retrieving a batch is not, +by itself, evidence of a working configuration. Always follow it with a cheap, +duration-tracked probe before declaring success: + +```python +batch = batch_definition.get_batch() +head = batch.head(n_rows=5) # this is what actually touches the data +``` + +A probe that raises means the configuration doesn't work — report per the +`KeyError` guidance above. A probe that returns means there's a real, working +batch definition. + +## Distinguishing an infrastructure failure from a real data-quality result + +After running an expectation (or a suite) against a batch, `success is False` +can mean two very different things, and they must be reported differently: + +- **The metric itself errored** — a broken column reference, a query that + fails, a type mismatch the engine can't evaluate. Great Expectations + reports this the same way it reports a real failure — `success: False` — + but the result payload is empty. Check `result.result`: an empty dict means + nothing was actually evaluated; there is no data-quality finding to report, + only a configuration problem to fix. Don't guess at what that problem is — + `result.exception_info` names it exactly. On this branch it's a dict with + one entry per metric that errored, keyed by the **string repr** of a + `MetricConfigurationID` (not a `MetricConfigurationID` instance itself — a + keyed lookup with one won't match), and each value is a dict whose + `exception_message` key holds the real cause verbatim, e.g. + `'Error: The column "nope" in BatchData does not exist.'`. Report that + message as the *why*, not a guess. Reached through the normal flow these + messages are short and clean, because the batch-definition probe catches a + broken table long before validation runs. Apply the same length ceiling + used for captured log output anyway: validating against a batch that was + never probed can surface an engine-level message carrying a full traceback, + and the no-raw-traceback rule holds on every path. +- **The data genuinely failed the expectation.** The result payload is + populated — counts, examples, percentages of the specific values that + violated the check. + +```python +result = batch.validate(expectation) +if not result.success and not result.result: + # a metric error: result.exception_info is a dict of + # {str(MetricConfigurationID): {"exception_message": ..., ...}}, one entry + # per metric that errored — report each exception_message verbatim as the + # why, not a data-quality finding. Keys are strings; iterating .items() + # works, but a keyed lookup with a MetricConfigurationID instance won't. + for metric_id, info in result.exception_info.items(): + # tell the user: info["exception_message"] + ... +elif not result.success: + # a genuine data-quality failure: report result.result's counts/examples + ... +``` + +Never report an empty-`result` failure to the user as "your data failed this +check" — it didn't get evaluated at all. diff --git a/great_expectations/.agents/skills/gx-configure-data-source/references/write-out.md b/great_expectations/.agents/skills/gx-configure-data-source/references/write-out.md new file mode 100644 index 000000000000..ee0174fd0a86 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-data-source/references/write-out.md @@ -0,0 +1,178 @@ +# Writing an in-memory session out to a project + +An in-memory (ephemeral) session, per `preflight.md`, holds everything only in +process memory — data sources, assets, batch definitions, and expectation +suites all disappear when the process ends. This procedure turns that session +into a real, file-backed project so the work survives and is reusable outside +this conversation. + +Offer this at a natural point — after a data source and batch definition are +verified working, or after a suite has been built and run — not as an +unprompted interruption mid-task. Only do it when the user agrees. + +## Confirm a target directory first + +Never guess a location. Ask the user where the project should live (an +absolute path is safest), and confirm it back before writing anything. + +## The procedure: public factories, not the built-in migrator + +Great Expectations ships a method that converts an in-memory context to a +file-backed one in place. Do not use it here. It resolves the target directory +from the current working directory rather than from an explicit path, it has +a known store-migration ordering issue, and its merge behavior does not +reliably overwrite objects that already exist at the destination — none of +which is acceptable when the target directory and the correctness of the +result both matter. Instead, build the file-backed project explicitly and +re-create each object in it through the same update-safe public factories used +everywhere else in this skill. That gives you a small, fully disclosed +sequence of steps, each independently retryable. + +**`add_or_update_` replaces the datasource wholesale — it is not +additive.** Calling it drops every asset and batch definition already +attached to that datasource, including ones that came from outside this +session: a prior conversation, a teammate, an earlier write-out. Opening a +project that already has a datasource with the name you're about to write and +calling `add_or_update_pandas` (or any `add_or_update_` factory) +under that same name silently destroys every other asset already on it — this +is true the very first time it's called in this project, not just on a +repeat. **Check whether the datasource already exists first, and skip adding +it if it does.** Only call `add_or_update_*` again if you specifically intend +to replace that datasource's connection configuration, and if so, warn the +user first that doing so will drop every other asset already attached to it. + +Build the file-backed project and re-create each object with the pattern +below. Wrap each object in its own try rather than the whole procedure in one +try block, and keep a running record of what succeeded and what didn't. The +asset step needs the datasource handle, and the batch-definition step needs +the asset handle — a zero-arg step that doesn't reflect this can't express +the chain. Have each step **re-fetch its dependency by name** from +`file_context` instead of closing over a variable from an earlier step: that +gets you the same handle without needing an earlier step to have succeeded in +the same function scope, and it makes failures cascade correctly — if the +datasource step failed, the asset step's own fetch of it fails too, with a +reason that points back at the real cause instead of a confusing `NameError`. + +The datasource step follows the same fetch-first-on-`LookupError` shape as +the asset and batch-definition steps below it — that's what makes it safe to +run once, unconditionally, without wiping a datasource that's already there. +List it exactly once, before any asset step, even when the session created +several assets on it: + +```python executable +import great_expectations as gx + +# 1. Open (or create) the file-backed project at the confirmed location. +file_context = gx.get_context(mode="file", project_root_dir="") + +def _add_datasource(): + try: + return file_context.data_sources.get("my_datasource") + except LookupError: + return file_context.data_sources.add_or_update_pandas(name="my_datasource") + +def _add_asset(): + datasource = file_context.data_sources.get("my_datasource") + try: + return datasource.get_asset("my_asset") + except LookupError: + return datasource.add_dataframe_asset(name="my_asset") + +def _add_batch_definition(): + asset = file_context.data_sources.get("my_datasource").get_asset("my_asset") + try: + return asset.get_batch_definition("my_batch_definition") + except LookupError: + return asset.add_batch_definition_whole_dataframe(name="my_batch_definition") + +def _add_suite(): + # `suite` here is the same ExpectationSuite object (or an equivalent one) + # built against the in-memory context earlier in the flow. + return file_context.suites.add_or_update(suite) + +steps = [ + ("data source my_datasource", _add_datasource), + ("asset my_asset", _add_asset), + ("batch definition my_batch_definition", _add_batch_definition), + ("suite my_suite", _add_suite), + # ... one entry per object to re-create: repeat the asset and + # batch-definition pattern (each its own function, closing over its own + # name) for every asset/batch definition created in the session, and the + # suite pattern for every suite. List the datasource step only once, even + # when the session created several assets on it. +] + +written = [] +failed = [] +for label, step in steps: + try: + step() + written.append(label) + except Exception as e: + failed.append((label, str(e))) +``` + +Note the fetch-first pattern in all three of the datasource, asset, and +batch-definition steps. `add_or_update_pandas` (and the other +`add_or_update_` factories) and `suites.add_or_update` are +update-safe on their own — calling either again just replaces that one +object's own content, which is harmless in isolation — but as just covered, +`add_or_update_` is not safe for what's attached underneath a +datasource, which is why the datasource step above fetches first too. There +is no `add_or_update_*` factory at all for a dataframe asset or a batch +definition — calling `add_dataframe_asset` or +`add_batch_definition_whole_dataframe` a second time with the same name +raises instead of updating. Fetching first and only adding on a `LookupError` +is what actually makes every step in this procedure safe to run again — not +the presence of `add_or_update_*` in some of the calls. + +Report both lists to the user explicitly: what was written successfully, and +what wasn't, with the reason for each failure. If an earlier step failed, a +later step that depends on it will fail too — report that as a consequence of +the earlier failure, not as a second, unrelated problem. Because every step +fetches first, re-running the whole procedure after fixing the cause of a +failure is safe — nothing already written gets duplicated, corrupted, or (per +the warning above) destroyed by running it again. + +## Report the written location + +When it completes, tell the user the absolute path that was written to, and +name what landed there (data source names, asset names, batch definition +names, suite names). Don't just say "done" — the point of write-out is that +the user can go find these files. + +## What "usable without modification" means, and its one exception + +Everything written out this way is a standard project artifact: a fresh +`gx.get_context(mode="file", project_root_dir=...)` against that directory +loads the same data sources, assets, batch definitions, and suites, and +`batch_definition.get_batch()` and `batch.validate(suite)` work exactly as +they did in the original session — with one exception. + +**Dataframe assets carry no data.** An in-memory dataframe (a pandas +`DataFrame` passed as the asset's data) is never serialized to disk — only the +asset's *configuration* is written out. After write-out, and in every future +session, retrieving a batch from a dataframe asset still requires passing the +dataframe explicitly at call time: + +```python executable +batch = batch_definition.get_batch(batch_parameters={"dataframe": df}) +``` + +State this to the user when a dataframe asset is part of what got written +out — it's easy to assume the data went with the config, and it didn't. + +## Secrets after write-out: the environment-vs-file split + +An in-memory session resolves `${ENV_VAR}`-style substitutions only from +process environment variables — it has no on-disk uncommitted config file to +read them from, because it has no disk footprint at all. A file-backed project +gains a second, additive source: an uncommitted config-variables file that +lives inside the project directory. Writing a session out to a project does +not change how any existing `${ENV_VAR}` reference resolves — it still comes +from the environment, exactly as before — but it does mean the user now has +the option to move any of those values into the project's uncommitted config +file for anyone else who works in that project without necessarily sharing +the same shell environment. Mention this as a follow-up option; do not do it +for them, and never write a resolved secret value into any file yourself — +only the `${ENV_VAR}`-style reference belongs in a persisted config. diff --git a/great_expectations/.agents/skills/gx-configure-expectations/SKILL.md b/great_expectations/.agents/skills/gx-configure-expectations/SKILL.md new file mode 100644 index 000000000000..8dbab37c64ed --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-expectations/SKILL.md @@ -0,0 +1,406 @@ +--- +name: gx-configure-expectations +description: Turn the data-quality checks a user describes in their own words into Great Expectations expectations — matched against the catalog shipped with the installed package, collected into a persisted suite, run against a batch, and reported per expectation. Use when a user wants to assert something about their data ("amounts should never be negative", "the customer column must never be empty"), to add checks to an existing suite, or to re-run a suite and explain the results. +license: Apache-2.0 +--- + +# Configure Great Expectations expectations + +This skill takes a user from "here is what I want to be true about my data" to +a **saved suite of expectations that has been run against a real batch**, with +every check reported individually. + +Two objects, and the relationship between them is where the traps live: + +- An **expectation** is a single check with typed parameters — one column, one + assertion, one set of bounds. +- An **expectation suite** is the named, persisted collection of them. + +Everything here goes through Great Expectations' public API and produces +ordinary project artifacts. Nothing depends on this skill being present +afterwards. + +## The flow + +1. **Preflight** — find out which project (or in-memory session) you are + operating on, and tell the user. See `references/preflight.md`. +2. **Check the precondition** — a working batch definition must already + exist. If none does, hand off and stop. +3. **Match** what the user described against the shipped expectation catalog. + Build only what they described. +4. **Register the suite first**, then add the matched expectations to it. +5. **Validate** against a batch and report each expectation's own outcome. +6. **Confirm persistence**, and offer write-out if the session is in memory. + +Steps 2 and 4 are the two places this flow fails silently rather than loudly. +Do not reorder them, and do not skip step 5 — a suite that was never run is +not a result worth reporting. + +## Step 1 — Preflight + +Follow `references/preflight.md` in full before anything else. It establishes +whether you are working against a project on disk or an in-memory session, +tells you what to announce to the user, and covers the environment problems +that silently masquerade as "no project found". + +The outcome you carry forward is a `context` object and one fact: whether the +session is file-backed or in memory. Both are fully supported paths, and the +difference matters at step 6. + +## Step 2 — The precondition: a working batch definition must exist + +Expectations are checks against a batch of data. Without a batch definition to +retrieve one through, there is nothing to validate against, and **there is no +acceptable way to improvise data access here**. Do not add a data source, do +not read a file directly with pandas, do not construct a batch by hand. + +Enumerate what the session already has: + +```python executable +def existing_batch_definitions(context): + """(data source, asset, batch definition) name triples available in this session.""" + found = [] + for datasource in context.data_sources.all().values(): + for asset in datasource.assets: + for batch_definition in asset.batch_definitions: + found.append((datasource.name, asset.name, batch_definition.name)) + return found +``` + +**If this returns an empty list, stop.** Tell the user that expectations need +a batch definition to run against, that their project (or session) has none +yet, and that setting one up is the `gx-configure-data-source` skill's job. +Hand off to it and end this flow — do not carry on and do not offer a +workaround. This is a hard stop, not a suggestion. + +If it returns more than one, name them and let the user choose; guessing which +slice of their data they meant to assert against is not a decision to make for +them. Then retrieve the batch: + +```python executable +datasource_name, asset_name, batch_definition_name = existing_batch_definitions(context)[0] +batch_definition = ( + context.data_sources.get(datasource_name) + .get_asset(asset_name) + .get_batch_definition(batch_definition_name) +) +batch = batch_definition.get_batch() # add batch_parameters=... for a partitioned definition +``` + +A dataframe asset needs its data supplied at retrieval time — +`get_batch(batch_parameters={"dataframe": df})` — so ask the user for the +dataframe rather than inventing one. + +**Retrieving a batch is not proof that it works.** For SQL assets, retrieval +touches nothing. If the batch definition has not been verified in this +session, probe it first with `batch.head(n_rows=5)` inside the wrapper in +`references/robustness.md`, exactly as the data-source flow does. A broken +batch definition discovered at validation time reports as a wall of metric +errors instead of one clear configuration problem. + +## Step 3 — Match what the user described, and only that + +Read the shipped catalog rather than working from memory — see +`references/expectation-catalog.md`. It covers locating the index inside the +installed package, matching a described check on type, description, and +data-quality category, filtering by the backend behind the batch definition, +and reading each expectation's parameters from its schema. + +Two rules govern this step. + +**Never invent an expectation type.** If nothing in the catalog matches what +the user described, say so, present the nearest candidates the catalog +actually contains, and offer the custom-expectation path. Report unmatched +checks separately as unbuilt, with the reason — never silently drop one, and +never substitute a different check that sounds similar. The catalog reference +gives the no-match procedure in full. + +**Build only what the user described.** Do not profile the data to find +things worth asserting. Do not scan the schema and propose a check per column. +Do not append "you might also want" suggestions to the suite. The user decides +what is asserted about their data; a check they did not ask for is a guess of +yours that will later fail and be read as a real data-quality problem. If they +describe something vaguely ("amounts should be reasonable"), ask what bound +they mean — do not pick one from the data. + +Inspecting a column to *inform a question you ask the user* is fine. Turning +what you observed into an expectation without being asked is not. + +## Step 4 — Register the suite first, then add expectations + +**Register the suite with the context before adding any expectation to it.** +This is the ordering rule, and getting it backwards loses work silently: + +```python executable +import great_expectations as gx + +SUITE_NAME = "orders_quality" + +# Reuse an existing suite; create one only when the name is genuinely new. +if SUITE_NAME in {suite.name for suite in context.suites.all()}: + suite = context.suites.get(SUITE_NAME) +else: + suite = context.suites.add(gx.ExpectationSuite(name=SUITE_NAME)) + +# Now add. Each add persists immediately, because the suite is registered. +suite.add_expectation(gx.expectations.ExpectColumnValuesToNotBeNull(column="customer")) +suite.add_expectation(gx.expectations.ExpectColumnValuesToBeBetween(column="amount", min_value=0)) +``` + +Three things this pattern gets right, each verified against real behavior: + +- **Work on the handle the factory returns.** `context.suites.add(...)` + re-reads the stored suite and returns a *different* object from the one you + passed in. The returned handle is the one wired to the store, so every + `add_expectation` on it is written through immediately, with no separate + save call. +- **An unregistered suite persists nothing.** Building + `gx.ExpectationSuite(name=...)`, adding expectations to it, and validating + with it all work — `batch.validate(suite)` runs happily against a suite the + context has never seen. Nothing raises, nothing warns, and afterwards + `context.suites.get(name)` raises `DataContextError: ExpectationSuite with + name was not found.` The entire suite is gone. This is the failure + mode the ordering rule exists to prevent. +- **`context.suites.add_or_update()` is destructive against an existing + name.** It replaces the stored suite wholesale rather than merging: calling + `context.suites.add_or_update(gx.ExpectationSuite(name=SUITE_NAME))` against + a suite already holding three expectations leaves it holding zero — no + error, no warning. Use the fetch-first pattern above instead. Reach for + `add_or_update` only when the user has asked to replace a suite's whole + contents, and tell them what is being discarded first. + +Reusing a suite is safe: fetching it returns its existing expectations intact, +and adding to it appends. Adding an expectation identical to one already +present is a no-op — the collection is set-like — so re-running the flow does +not accumulate duplicates. + +### Suite names: no dots + +**Never put a dot in a suite name.** The store treats dots as path +separators. A suite named `orders.quality` is written to +`gx/expectations/orders/quality.json`, and `a.b.c` to +`gx/expectations/a/b/c.json` — one nested directory per segment. The suite +still loads by its full dotted name, so nothing appears broken from the API, +but the project's expectations directory fans out into a tree where nobody +looking for a suite file will find it, and a plain listing of the directory no +longer shows the suites it holds. + +Use underscores or hyphens: `orders_quality`, `orders-daily`. If the user asks +for a dotted name, say why you are changing it rather than changing it +silently. + +## Step 5 — Validate, and report each expectation on its own terms + +```python executable +result = batch.validate(suite) +``` + +That is the whole validation call. It builds everything else it needs +internally. + +Validation is a data-touching operation, so run it inside the duration-tracked +wrapper in `references/robustness.md` — the same one the batch probe uses. A +suite over a large table can run for a long time, and the rules there about +checking in with the user rather than abandoning a query that keeps running on +the platform apply unchanged. + +### Pair results with expectations by configuration, never by position + +**`result.results` does not come back in the order the expectations were +added.** Validation regroups the suite before running it, in two ways. Both +are stable across runs, so neither ever looks like a bug: + +- **Expectations are grouped by the column they address.** Every check on one + column is evaluated together, in the order that column was first mentioned. + A suite added as `not_be_null(customer)`, `mean_to_be_between(amount)`, + `values_to_be_unique(customer)`, `max_to_be_between(amount)` comes back with + the two `customer` checks first and the two `amount` checks after. Checks + with no `column` argument at all, such as a table row count, form a group of + their own. +- **An expectation whose metric errored is moved ahead of every expectation + that ran**, so a broken parameter also changes the position of everything + else. + +What makes this dangerous is that plenty of suites *do* come back in the order +they were built: any suite written column by column already matches the +grouping, and so does a two- or three-expectation suite over one column. +Pairing your input list with the results by index therefore looks correct +while you are trying it out and mislabels every finding once the suite grows. + +Read the identity off each result instead: `each.expectation_config.type` and +`each.expectation_config.kwargs`. + +### Separate a metric error from a data failure + +`success is False` means two very different things, and reporting them the +same way tells the user their data is bad when their configuration is: + +```python executable +for each in result.results: + config = each.expectation_config + if each.success: + print(f"PASS {config.type} {config.kwargs}") + elif not each.result: + # The metric never evaluated — a configuration problem, not a finding. + for _metric_id, info in each.exception_info.items(): + print(f"ERROR {config.type} {config.kwargs}: {info['exception_message']}") + else: + # The data genuinely failed the check. + print(f"FAIL {config.type} {config.kwargs}: {each.result}") +``` + +**Both halves of `not each.success and not each.result` are load-bearing.** A +*passing* `expect_column_to_exist` also carries an empty `result` dict, so the +emptiness test alone would misclassify it. + +On the error branch, `exception_info` already names the cause exactly — do not +guess at it. It is a dict with one entry per metric that errored, keyed by the +**string** repr of a metric identifier, and each value carries an +`exception_message` holding the real cause verbatim: a mistyped column reports +`Error: The column "nope" in BatchData does not exist.` Relay that message as +the *why*, apply the length ceiling from `references/robustness.md`, and never +paste the accompanying `exception_traceback`. + +**Never report an empty-`result` failure as "your data failed this check."** +It was never evaluated. Report it as an expectation that could not run, name +the cause, and offer to fix the parameter and re-run. + +### Degenerate data does not raise + +Empty tables and all-null columns are ordinary results, not exceptions. +Anything in your report that treats them as errors is wrong. Verified +behavior, on a SQL batch: + +| Situation | Outcome | +| --- | --- | +| Empty table, `expect_column_values_to_not_be_null` | `success=True`, `element_count: 0` — vacuously true | +| Empty table, `expect_column_mean_to_be_between` | `success=False`, `{"observed_value": None}` | +| Empty table, `expect_table_row_count_to_be_between(min_value=1)` | `success=False`, `{"observed_value": 0}` | +| All-null column, `expect_column_values_to_not_be_null` | `success=False`, 100% unexpected | +| All-null column, `expect_column_values_to_be_between` | `success=True` — nulls count as missing, not as violations | +| All-null column, `expect_column_mean_to_be_between` | `success=False`, `{"observed_value": None}` | + +Two of these mislead if reported literally: + +- **`{"observed_value": None}` is a populated result**, so the discriminator + above correctly classifies it as a data failure rather than a metric error. + But it does not mean the mean was out of range — it means there were no + non-null values to compute over. Say that, and say which column. +- **A value-level check passing over an all-null column is not reassurance.** + Nulls are excluded from the unexpected count, so a range check over a column + of nothing but nulls succeeds. If the user's real question was whether the + column has usable data, pair it with a non-null check and say why. + +### Summarize honestly + +`result.describe()` returns a JSON string carrying overall `success`, a +`statistics` block (`evaluated_expectations`, `successful_expectations`, +`unsuccessful_expectations`, `success_percent`), and a per-expectation list of +`expectation_type` / `kwargs` / `success` / `result`. It is a good basis for a +written summary — but it does not distinguish a metric error from a data +failure. Use the loop above for that distinction and `describe()` for the +counts. + +Report, per expectation: what was checked, on which column, and what happened +— passed, failed with the observed numbers, or could not run with the cause. +Then the totals. Do not present a metric error inside the failure count as +though the data had been judged. + +## Step 6 — Persistence, and write-out when in memory + +**In a file-backed project the suite is already saved.** Registering it in +step 4 wrote it to `/expectations/.json`, and every +subsequent `add_expectation` was written through as it happened. There is no +save step to run and none to forget. Confirm it concretely: name the suite, +name the file, and say that a fresh session picks it up with +`context.suites.get("")`. + +**Validation results are not persisted by this flow.** `batch.validate()` +returns the result to you and writes nothing to the project. The suite is the +durable artifact; the report you give the user is the record of this run. Say +so rather than letting them assume the results are filed somewhere. + +**In an in-memory session, nothing survives the process.** A second in-memory +session sees no suites and no data sources at all. Say this plainly — it is a +supported way to work, not a degraded one — and **offer to write the session +out** to a real project, per `references/write-out.md`. That procedure covers +the suite along with the data source, asset, and batch definition it depends +on; writing out a suite without them leaves a project that cannot run it. +Offer it; don't do it unprompted, and don't pick the location. + +## Worked example + +A file-backed project that already holds a verified batch definition over an +`orders` table. The user asked for two things: customer must never be missing, +and amounts must never be negative. + +```python +import great_expectations as gx + +context = gx.get_context(cloud_mode=False) # step 1, per references/preflight.md + +batch_definition = ( # step 2, from what already exists + context.data_sources.get("warehouse") + .get_asset("orders") + .get_batch_definition("all_rows") +) +batch = batch_definition.get_batch() + +# step 3: both descriptions matched catalog entries; nothing else was invented +if "orders_quality" in {suite.name for suite in context.suites.all()}: + suite = context.suites.get("orders_quality") # step 4: register/fetch first +else: + suite = context.suites.add(gx.ExpectationSuite(name="orders_quality")) + +suite.add_expectation(gx.expectations.ExpectColumnValuesToNotBeNull(column="customer")) +suite.add_expectation(gx.expectations.ExpectColumnValuesToBeBetween(column="amount", min_value=0)) + +result = batch.validate(suite) # step 5 + +for each in result.results: + config = each.expectation_config + if each.success: + print(f"PASS {config.type} {config.kwargs}") + elif not each.result: + for _metric_id, info in each.exception_info.items(): + print(f"ERROR {config.type} {config.kwargs}: {info['exception_message']}") + else: + print(f"FAIL {config.type} {config.kwargs}: {each.result}") +``` + +Against a table of four rows where one `customer` is null and one `amount` is +`-5.0`, that prints: + +```text +FAIL expect_column_values_to_not_be_null {'batch_id': 'warehouse-orders', 'column': 'customer'}: {'element_count': 4, 'unexpected_count': 1, 'unexpected_percent': 25.0, 'partial_unexpected_list': [None], ...} +FAIL expect_column_values_to_be_between {'batch_id': 'warehouse-orders', 'column': 'amount', 'min_value': 0.0}: {'element_count': 4, 'unexpected_count': 1, 'unexpected_percent': 25.0, 'partial_unexpected_list': [-5.0], ...} +``` + +This two-expectation suite happens to come back in the order it was built — +which is exactly why the loop reads each result's own +`expectation_config` rather than trusting the position. Report it to the user +as *one of four rows has no customer, and one amount is negative (-5.0)*; the +`partial_unexpected_list` is what makes a finding actionable, so relay it. + +## Where this flow ends + +The saved, run, reported suite is the end state. Three things sit outside it: + +- **Setting up data access.** If the precondition in step 2 fails, that is + the `gx-configure-data-source` skill's work, not something to improvise + around. +- **Proposing expectations from the data.** The user describes; this flow + builds. See step 3. +- **Rendering results anywhere but this conversation.** Report what the run + found; do not build reporting surfaces the user did not ask for. + +## References + +- `references/preflight.md` — establishing and announcing the session context. +- `references/expectation-catalog.md` — locating the shipped catalog, matching + a described check against it, reading parameters, and what to do when + nothing matches. +- `references/robustness.md` — the time-budget wrapper, scope-reduction + levers, and how to report a failure helpfully. +- `references/write-out.md` — turning an in-memory session into a real + project. diff --git a/great_expectations/.agents/skills/gx-configure-expectations/references/expectation-catalog.md b/great_expectations/.agents/skills/gx-configure-expectations/references/expectation-catalog.md new file mode 100644 index 000000000000..6ad9e5671f75 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-expectations/references/expectation-catalog.md @@ -0,0 +1,260 @@ +# The expectation catalog: matching a described check to a real expectation + +Great Expectations ships a machine-readable catalog of its expectations, +alongside the code, inside the installed package. Read it at runtime. **Never +work from a memorized or hand-written list of expectation types** — one goes +stale the moment a release adds or renames a check, and a type you invent +fails at construction time with an error that reads like the user asked for +something impossible. The catalog is generated from the same models that +define the expectations, so it is correct for the version actually installed +in front of you. + +Everything below is derived from files inside the installed package. There is +nothing to download and no network call. + +## Locating the catalog + +The catalog lives under `expectations/core/schemas/` in the installed +`great_expectations` package. Reach it through `importlib.resources` rather +than by building a filesystem path from `great_expectations.__file__` — that +works whether the package is installed normally, installed in editable mode, +or imported from a zipped distribution: + +```python +import json +from importlib.resources import files + +SCHEMAS = files("great_expectations") / "expectations" / "core" / "schemas" +INDEX = json.loads((SCHEMAS / "index.json").read_text()) +CATALOG = INDEX["expectations"] +``` + +Two kinds of file live there: + +- `index.json` — the catalog: one entry per cataloged expectation, plus a + `documented_absent` list covered at the end of this document. +- `.json` — one schema per cataloged expectation, naming the + parameters it accepts and which of them are mandatory. + +## Step 1: what each index entry carries + +`INDEX["expectations"]` maps each `expectation_type` (the snake_case name that +appears in validation results) to four fields: + +```python +print(json.dumps(CATALOG["expect_column_values_to_be_between"], indent=2)) +``` + +```text +{ + "data_quality_issues": ["Numeric"], + "schema_file": "ExpectColumnValuesToBeBetween.json", + "short_description": "Expect the column entries to be between a minimum value and a maximum value (inclusive).", + "supported_data_sources": ["Pandas", "Spark", "SQLite", "PostgreSQL", ...] +} +``` + +Each field earns its place in matching: + +- **`expectation_type`** — the key. Its words are the vocabulary users + actually reach for: `null`, `unique`, `between`, `in_set`, `match_regex`, + `row_count`. +- **`short_description`** — one sentence of prose, which is what makes a + natural-language phrase matchable at all. +- **`data_quality_issues`** — a small controlled vocabulary for grouping. Read + the live set out of the index rather than assuming it — + `sorted({i for v in CATALOG.values() for i in v["data_quality_issues"]})` + currently gives `Completeness`, `Multi-source`, `Numeric`, `SQL`, `Schema`, + `Uniqueness`, `Validity`, `Volume`. Use it when the user describes a + *category* ("check the data is complete") rather than a specific check. +- **`supported_data_sources`** — the backends the expectation is known to work + on. Check it against the backend behind the user's batch definition before + offering a candidate, because the coverage is not uniform: the + `*_like_pattern*` family, `expect_query_results_to_match_comparison`, and + `expect_table_row_count_to_equal_other_table` are SQL-only and absent on + Spark. + +## Step 2: matching what the user described + +Search all three text-bearing fields together. Keep it simple — the point is +to produce candidates for the user to confirm, not to guess on their behalf: + +```python +def search(*terms: str) -> list[tuple[int, str, str]]: + """Rank catalog entries by how many of the user's terms they mention.""" + terms = [t.lower() for t in terms] + hits = [] + for expectation_type, entry in CATALOG.items(): + haystack = " ".join([ + expectation_type.replace("_", " "), + entry["short_description"], + " ".join(entry["data_quality_issues"]), + ]).lower() + score = sum(term in haystack for term in terms) + if score: + hits.append((score, expectation_type, entry["short_description"])) + return sorted(hits, reverse=True) +``` + +For "the customer column should never be empty", `search("null")` returns +`expect_column_values_to_not_be_null`, `expect_column_values_to_be_null`, and +`expect_column_proportion_of_non_null_values_to_be_between` — three real +candidates with visibly different meanings. **Show the candidates and their +descriptions and let the user pick** when more than one is plausible. The +difference between "never null" and "at least 95% non-null" is the user's +decision, not yours. + +Two filters are worth having alongside the text search: + +```python +# By category, when the user described a kind of problem rather than a check. +[e for e, v in CATALOG.items() if "Uniqueness" in v["data_quality_issues"]] + +# By backend, to drop candidates that won't run on this batch definition. +[e for e, v in CATALOG.items() if "Spark" in v["supported_data_sources"]] +``` + +## Step 3: from a matched entry to a constructed expectation + +The class name is the schema filename without its `.json` suffix, and every +cataloged expectation is exposed under that name on +`great_expectations.expectations`. That derivation is exact for every entry +the package ships: + +```python +import great_expectations as gx + +entry = CATALOG["expect_column_values_to_be_between"] +expectation_class = getattr(gx.expectations, entry["schema_file"].removesuffix(".json")) +``` + +The schema names the parameters and which are mandatory: + +```python +schema = json.loads((SCHEMAS / entry["schema_file"]).read_text()) +print("required:", schema["required"]) +print("accepted:", sorted(schema["properties"])) +``` + +```text +required: ['column'] +accepted: ['batch_id', 'catch_exceptions', 'column', 'condition_parser', 'description', + 'id', 'max_value', 'meta', 'metadata', 'min_value', 'mostly', 'notes', + 'rendered_content', 'result_format', 'row_condition', 'severity', + 'strict_max', 'strict_min', 'windows'] +``` + +Then construct it with keyword arguments: + +```python +expectation = expectation_class(column="amount", min_value=0, mostly=0.99) +``` + +Reading the schema rather than guessing matters for three reasons: + +- **`required` is short and the interesting parameters are optional.** + `expect_column_values_to_be_between` requires only `column`; `min_value` and + `max_value` are optional, and an expectation built with neither asserts + nothing useful. Elicit the bound the user actually meant. +- **Each entry in `properties` carries its own `description`.** Read it to the + user when they ask what a parameter means instead of paraphrasing from + memory. `mostly`, for example, is documented as "Successful if at least + `mostly` fraction of values match the Expectation" — a tolerance, not a + target. +- **Treat `batch_id`, `id`, `meta`, `metadata`, `rendered_content`, and + `windows` as reserved.** They are assigned by the library or belong to + surfaces outside this flow; the parameters to elicit are the ones that + describe the check. + +## The `documented_absent` list + +`INDEX["documented_absent"]` names expectations that are real and usable but +ship no schema file: + +```python +print(INDEX["documented_absent"]) +``` + +```text +['expect_column_values_to_be_dateutil_parseable', + 'expect_column_values_to_be_decreasing', + 'expect_column_values_to_be_increasing', + 'expect_column_values_to_be_json_parseable', + 'expect_column_values_to_match_json_schema'] +``` + +These are **not** unavailable. Each has a class on +`great_expectations.expectations` under the usual CamelCase name and can be +constructed, added to a suite, and validated exactly like any other. What they +lack is a catalog entry, so `search()` above will never surface them and there +is no schema to read parameters from. Fall back to the class itself, which is +authoritative for the installed version: + +```python +import inspect + +expectation_class = gx.expectations.ExpectColumnValuesToBeIncreasing +print(inspect.signature(expectation_class)) # accepted parameters and defaults +print(expectation_class.__doc__) # the same prose a description would carry +``` + +Mention them by hand when a user describes something they cover — a monotonic +sequence, a parseable date string, JSON-shaped values — since the text search +cannot. + +## When nothing matches + +Say so. **Never invent an expectation type**, never bend the user's +description onto a check that means something else, and never present a +candidate as though it were what they asked for. + +Produce nearest candidates from the catalog itself so the user has something +concrete to react to. `difflib` is in the standard library and is enough: + +```python +import difflib + +def nearest(phrase: str, n: int = 5) -> list[tuple[float, str]]: + corpus = { + expectation_type: f"{expectation_type.replace('_', ' ')} {entry['short_description']}" + for expectation_type, entry in CATALOG.items() + } + scored = sorted( + ( + (difflib.SequenceMatcher(None, phrase.lower(), text.lower()).ratio(), expectation_type) + for expectation_type, text in corpus.items() + ), + reverse=True, + ) + return [(round(ratio, 3), expectation_type) for ratio, expectation_type in scored[:n]] +``` + +For "every email address must be deliverable" — a real check, and one no +shipped expectation performs — `search("deliverable", "email")` returns +nothing and `nearest(...)` returns `expect_column_values_to_be_null`, +`expect_column_value_z_scores_to_be_less_than`, +`expect_column_pair_values_to_be_equal`, and so on, all scoring around 0.3. +**A weak score is information, not a match.** Report it as one: + +> Nothing in the installed catalog checks whether an email address is +> deliverable — that needs a live mail server, which is outside what an +> expectation does. The closest shipped checks are +> `expect_column_values_to_match_regex` (format only, not deliverability) and +> `expect_column_values_to_not_be_null`. Would either of those be useful, or +> should this check live outside Great Expectations? + +Two honest paths forward, and either is a better answer than a wrong match: + +- **A different shipped expectation that covers part of the intent**, named + with its limitation stated plainly — format instead of deliverability, + non-null instead of non-empty-string. +- **A custom expectation.** Great Expectations supports user-defined + expectations, and a check with no shipped equivalent is exactly what they + are for. Point the user at the custom-expectation documentation for the + version they have installed rather than sketching an implementation + mid-flow — authoring one is its own piece of work, not a step in this + conversation. + +Build the expectations that did match, run them, and report the unmatched ones +separately as unbuilt, with the reason. Do not silently drop them, and do not +substitute something else for them. diff --git a/great_expectations/.agents/skills/gx-configure-expectations/references/preflight.md b/great_expectations/.agents/skills/gx-configure-expectations/references/preflight.md new file mode 100644 index 000000000000..e3b3c4ae0597 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-expectations/references/preflight.md @@ -0,0 +1,142 @@ +# Session preflight + +Before configuring anything, establish what you're operating on: a project the +user already has on disk, or a temporary in-memory session. Do this exactly +once at the start of the flow, and tell the user the outcome before you do +anything else. + +## Enter with `cloud_mode=False` + +Call the context factory like this, not with a bare call: + +```python executable +import great_expectations as gx + +context = gx.get_context(cloud_mode=False) +``` + +A managed cloud offering that this factory can auto-connect to has been +retired. If a machine still carries leftover configuration for it (environment +variables, or a leftover config file), a bare `gx.get_context()` — and +`cloud_mode=True` — will detect that configuration, try to honor it, and raise +immediately with an error to that effect. That failure has nothing to do with +the user's local project or data; it's a stale-environment problem, and it +would happen before you ever got to look at a local project. Passing +`cloud_mode=False` explicitly skips that detection and always resolves to a +local file-backed project if one is found, or an in-memory session otherwise. + +**Check for stale cloud configuration yourself before you call it**, because +`cloud_mode=False` silently discards that configuration rather than reporting +it — there is no signal in the return value or in normal output that it was +there. Look for `GX_CLOUD_ACCESS_TOKEN`, `GX_CLOUD_ORGANIZATION_ID`, or +`GX_CLOUD_BASE_URL` in the environment. If any are set, tell the user plainly: +those variables were found but ignored, because that offering is no longer +reachable and today they should either unset them or ignore this message — +they have no effect on the session you're about to build. + +## Interpret what comes back + +`gx.get_context(cloud_mode=False)` returns one of two things. Branch on the +type: + +```python executable +from great_expectations.data_context import FileDataContext + +if isinstance(context, FileDataContext): + context_root = context.root_directory + # tell the user: "Using the existing project's configuration at + # ." +else: + # tell the user: "No project found — working in a temporary, in-memory + # session. Nothing here is saved until it's written out to a project + # (see write-out.md)." + ... +``` + +**A discovered project is not optional to announce.** Always state +`context_root` back to the user before doing anything else, so they know +exactly which project they're about to modify — this also makes +`add_or_update_*` updates against that project legible rather than a surprise. +Name it precisely as the project's *configuration directory*, not "the +project" on its own — `context_root` is the `gx` subdirectory that holds +`great_expectations.yml` and the stores, and its **parent** is the project +directory a user would think of as "the project". That parent is what +`project_root_dir` means everywhere it's accepted (`preflight.md`'s own +one-liner below, and `write-out.md`'s `gx.get_context(mode="file", +project_root_dir=...)`). Never feed `context_root` back in as a +`project_root_dir` — doing so nests a second `gx` directory inside the first +(`/gx/gx`) instead of reopening the same project. + +**An in-memory (ephemeral) session is not a degraded mode to apologize for.** +It's a normal, fully supported way to work — state plainly that the session is +temporary and that everything can be written out to a real project later (see +`write-out.md`). Do not stop, and do not treat the absence of a project as an +error condition. + +## Never scaffold a project yourself + +Standing up a new file-backed project is the user's decision, not something +to do on their behalf or without being asked. Two things follow from this: + +- Never call the file-context constructor without an explicit target + directory. Concretely: never call `gx.get_context(mode="file")` with no + `project_root_dir` — it does not raise or refuse when a project isn't found + at that implicit location; it silently creates one, into the current + working directory, with no confirmation. That is not something to do without + the user asking for it. +- If a user wants a new project created rather than an in-memory session, give + them the one-liner to run themselves and let them choose the location: + + ```python + context = gx.get_context(mode="file", project_root_dir="") + ``` + + Do not walk them through it interactively or pick the path for them. + +This applies during preflight. It does not apply to the write-out procedure in +`write-out.md`, which also calls `gx.get_context(mode="file", ...)` — there, +the target directory has already been confirmed with the user as an explicit +step, so the call is expected and disclosed rather than a silent side effect. + +## Never treat "no project" as a stop condition + +An in-memory session is a supported, first-class outcome of preflight, +covered above. Do not stop, refuse, or ask the user to go create a project +first just because discovery didn't find one — proceed with the ephemeral +session instead. + +## When discovery itself fails + +Two failure shapes are worth knowing by name, because neither produces an +obvious, self-explanatory error, and both are easy to misread as "no project +found" when they're actually a misconfiguration worth surfacing: + +**A stale `GX_HOME` environment variable.** If `GX_HOME` points at a directory +that either doesn't exist or doesn't contain a project config file, project +discovery does *not* raise — it silently falls back to the in-memory session, +exactly as if `GX_HOME` had never been set. If the user believes they have a +project and you get an in-memory session instead, this is the first thing to +check. Validate it yourself, since the factory won't: + +```python executable +import os +from pathlib import Path + +gx_home = os.environ.get("GX_HOME") +if gx_home is not None: + gx_home_path = Path(gx_home).expanduser() + if not (gx_home_path / "great_expectations.yml").is_file(): + # tell the user: "GX_HOME is set to , but no project config + # was found there. If you expected an existing project to be used, + # check the path — otherwise this variable can be ignored/unset." + ... +``` + +**Stale cloud configuration**, covered above — check for it before calling +the factory, since the factory itself won't tell you it was there. + +In both cases, the same shape of report applies: state what looked +misconfigured, state which value or file caused it, and give one concrete next +step (fix the path, unset the variable, or proceed with the in-memory +session). Never let a silent fallback pass as "everything's fine" when the +environment suggests the user expected otherwise. diff --git a/great_expectations/.agents/skills/gx-configure-expectations/references/robustness.md b/great_expectations/.agents/skills/gx-configure-expectations/references/robustness.md new file mode 100644 index 000000000000..bfa871b9b5cb --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-expectations/references/robustness.md @@ -0,0 +1,340 @@ +# Robustness: time budgets, scope reduction, and reading results correctly + +Real data is large, slow, and occasionally broken in ways that don't produce +clean errors. This document covers three things every data-touching step in +this skill needs: a time budget that doesn't pretend it can cancel anything, +concrete levers for cutting a query down to size, and how to read a result +correctly so an infrastructure failure never gets reported as a data-quality +finding. + +## The advisory time budget is a check-in, not a kill switch + +Wrap any potentially slow call — testing a connection, retrieving a batch, +probing it, running a validation — so you can check in with the user *while +it is still running*, not only after it returns. A budget measured after the +call has already completed can't do this — by the time you'd act on it, there +is nothing left to check in about. Instead, run the call on a worker thread +and poll it with a timeout, so you get control back at regular intervals +while the real call is still in flight: + +```python executable +import concurrent.futures +import time + +BUDGET_SECONDS = 60 # let the user adjust this for known-slow sources +POLL_SECONDS = 5 # how often to check whether the budget has passed + +executor = concurrent.futures.ThreadPoolExecutor(max_workers=1) +future = executor.submit(batch.head, n_rows=5) # or test_connection(), get_batch(), validate(), ... +start = time.monotonic() +checked_in = False +succeeded = False +result = None + +while True: + # Report state instead of racing an exception: on Python 3.11+, + # concurrent.futures.TimeoutError IS the builtin TimeoutError, so if the + # wrapped call itself raises a TimeoutError (a driver connect/statement + # timeout, for example — see lever 3 below), an `except + # concurrent.futures.TimeoutError` branch built on `future.result()` + # cannot tell "still running" apart from "finished, with that exact + # exception". Since a *finished* future's `.result()` returns instantly, + # that ambiguity turns into an unthrottled busy loop with no sleep in it. + # `concurrent.futures.wait(...)` sidesteps this entirely — it reports + # done/not-done without ever raising the wrapped call's own exception. + done, _ = concurrent.futures.wait([future], timeout=POLL_SECONDS) + elapsed = time.monotonic() - start + if not done: + # The real call has NOT returned yet — this branch runs while it is + # still executing on the worker thread, which is what makes an + # in-flight check-in possible at all. + if elapsed > BUDGET_SECONDS and not checked_in: + checked_in = True + # tell the user, right now, while the call is still running: + # "This has been running s, past the s + # budget, and is still going." Then follow the three steps below. + ... + continue + try: + result = future.result() + succeeded = True + except Exception as e: + # translate the exception — see "Reporting failures helpfully" below + result = None + break + +if succeeded and checked_in: + # tell the user it finished after all, at s, past the budget + ... +``` + +`succeeded` — not `result is not None` — is what distinguishes "the call +returned" from "the call failed": some wrapped calls (`test_connection()`, +for one) return `None` on success, so `result` alone can't tell the two +apart. + +**When the budget is exceeded, do not abandon the operation.** On most data +platforms, the query is running on the platform's own compute the moment it +was dispatched — closing the client connection or giving up on waiting does +not cancel it, and it keeps consuming (and costing) resources on the platform +regardless of whether anything is still waiting on the result. Killing the +client side only means losing visibility into a query that is still running +and still being billed. So the moment the budget check-in above fires: + +1. Tell the user the operation is still running and has passed the budget. +2. State plainly that it will keep running (and, on most platforms, keep + costing money) whether or not you keep waiting on it. +3. Ask whether to keep waiting, or to reduce the scope of data involved and + try again — never decide this unilaterally. + +**Keep the future alive; never cancel it.** Whatever the user picks, do not +call `future.cancel()` or `executor.shutdown(cancel_futures=True)` — the call +already dispatched to the platform's own compute, and cancelling the future +only makes this process stop watching it; it does not stop the remote work, +which is the point above about it continuing to run and cost money. If the +user chooses to keep waiting, stay in the polling loop shown above. If the +user chooses to reduce scope, stop polling this loop and leave `future` and +`executor` exactly as they are — still running, unattended — and submit the +reduced-scope attempt (using the levers below) as a new, separate call. Don't +block on the abandoned future first. + +## Scope-reduction levers, in preference order + +The goal each time is to make the specific operation touch less data, not to +change what the asset represents — an asset stays the durable, logical +collection of data a project points at; a batch definition is what selects +how much of it a given operation actually reads. + +**1. A narrower batch definition window, via the existing partitioner +factories.** This is the primary lever, and it should be tried first whenever +there's a usable date/time or otherwise partitionable column. Add (or reuse) a +partitioned batch definition, then request a specific window at fetch time: + +```python +# One-time setup: partition by month on a datetime column. +batch_definition = asset.add_batch_definition_monthly(name="monthly", column="event_time") + +# Per-attempt: fetch only one narrow window instead of the whole asset. +batch = batch_definition.get_batch(batch_parameters={"year": 2024, "month": 3}) +``` + +The equivalent daily/yearly variants exist too +(`add_batch_definition_daily`, `add_batch_definition_yearly`), as does a +directory-scoped daily/monthly/yearly split for file-based assets. Always +reach these through the asset's own `add_batch_definition_*` methods and +`batch_parameters` — never by constructing a partitioner object directly. + +**2. A row-limiting argument on the read itself, for local file-based +sources.** Where the underlying reader supports it (for example, a pandas +CSV asset), pass its native `nrows` option — a hard cap on how many rows get +read — as a keyword argument when adding the asset: + +```python +asset = pandas_datasource.add_csv_asset( + name="my_asset", + filepath_or_buffer="/path/to/large_file.csv", + nrows=1000, +) +``` + +This helps specifically with large local files rather than remote queries. + +**3. A driver-level connection or statement timeout**, for SQL sources. Pass +driver-specific timeout options through the datasource's `kwargs`, which are +forwarded straight to engine creation: + +```python +datasource = context.data_sources.add_or_update_postgres( + name="my_datasource", + connection_string="postgresql+psycopg2://${DB_USER}:${DB_PASSWORD}@host/db", + kwargs={"connect_args": {"connect_timeout": 10}}, +) +``` + +The exact keys are driver-specific (a `connect_timeout` for one driver may be +a different name for another) — this caps how long a *connection attempt* +can hang, not how much data a query returns, so treat it as a complement to +lever 1, not a substitute. + +**4. Last resort: a row limit on a query asset, for exploration only.** If no +column exists to partition on (no usable date/time or other partitionable +column), and the goal is just to inspect a sample of the data rather than +operate on the real batch, a query asset with an explicit `LIMIT` in its SQL +text is an acceptable stopgap: + +```python +asset = datasource.add_query_asset( + name="sample_events", + query="SELECT * FROM events LIMIT 1000", +) +``` + +Flag this to the user explicitly as temporary and exploration-only when you +use it. A query asset's row limit is baked into that one query's text — it +isn't a general batching mechanism, it doesn't compose with the partitioner +levers above, and it isn't something to reach for as a default way to make a +slow asset faster. If a project keeps needing this, the honest answer is that +the reduction lever it actually needs doesn't exist yet in the partitioner set +— that's a gap to name to the user, not one to paper over with query-asset +limits everywhere. + +## Reporting failures helpfully + +Whenever something in this flow raises, report three things and stop there — +never a raw traceback: **what failed** (the operation you were attempting), +**why, as far as it's known** (the clearest available cause), and **one +concrete next step** the user can take. + +Two exception shapes are common enough to call out by name: + +- **Connection and configuration failures** (a failed `test_connection()` + during `add_or_update_*`, or an import error for an optional driver + dependency) already carry an actionable message from Great Expectations + itself — relay it close to verbatim, plus one next step (fix the connection + string, install the missing driver package, check the credential). +- **A missing credential.** If a data operation fails because a referenced + `${ENV_VAR}` isn't set, say exactly which variable is missing and how to + provide it (set the environment variable, or add it to the project's + uncommitted config-variables file if working in a file-backed project). + Never hardcode a value or invent a placeholder to work around it. +- **A bare `KeyError` out of a batch probe.** Retrieving a batch off a broken + query or table does not itself fail — see "Why a retrieved batch doesn't + prove anything" below — but probing it can raise a plain `KeyError`, not a + Great Expectations exception, with no readable message of its own (its text + is just an internal cache key). The real underlying cause (the actual + database error, including which table or column it choked on) is emitted + during metric resolution as a **`WARNING`-level log record on the + `great_expectations.validator.metrics_calculator` logger** — not printed to + stdout. Attach a handler to that logger before the probe so you capture the + record directly, rather than trusting the `KeyError`'s own text or scanning + raw console output — this avoids depending on stdout/stderr being + visible, or on the *root* logger's own level. It does **not** avoid this + logger's own effective level: a handler only sees records the logger lets + past its own threshold, so if a host has raised + `great_expectations.validator.metrics_calculator` above `WARNING`, force it + down to `WARNING` for the duration of the probe and restore it afterward: + + ```python + import logging + import re + import ast + + class _CaptureHandler(logging.Handler): + def __init__(self): + super().__init__(level=logging.WARNING) + self.records: list[str] = [] + + def emit(self, record: logging.LogRecord) -> None: + self.records.append(record.getMessage()) + + def _extract_cause(message: str, ceiling: int = 500) -> str: + # The log message is a stringified dict of MetricConfigurationID -> + # {"exception_message": ..., "exception_traceback": ..., ...} — often + # several thousand characters, wrapping a full traceback with absolute + # filesystem paths. Never relay it verbatim (see the no-raw-traceback + # rule above). Pull out just the exception_message value(s); if the + # shape ever changes and the pattern doesn't match, fall back to a + # hard-truncated slice so a ceiling is guaranteed either way. + found = re.findall(r"'exception_message': (.+?), 'raised_exception':", message, re.DOTALL) + try: + # A match is not automatically a valid Python literal: an error text + # containing the terminator itself truncates the non-greedy match + # mid-literal. Fall through to truncation rather than raising from + # inside the error-reporting path. + cause = "; ".join(ast.literal_eval(m) for m in found) if found else message + except Exception: + cause = message + if len(cause) > ceiling: + cause = cause[:ceiling] + "... (truncated)" + return cause + + handler = _CaptureHandler() + metrics_logger = logging.getLogger("great_expectations.validator.metrics_calculator") + metrics_logger.addHandler(handler) + prior_level = metrics_logger.level + metrics_logger.setLevel(logging.WARNING) + try: + head = batch.head(n_rows=5) + except KeyError: + cause = _extract_cause(handler.records[-1]) if handler.records else "" + # report: the batch could not be read; cause holds the real database error + ... + finally: + metrics_logger.removeHandler(handler) + metrics_logger.setLevel(prior_level) + ``` + + Report to the user that the batch could not be read, relay the extracted + cause (a few hundred characters, not the raw log message), and point them + at checking the query or table name, and the connection, as the next step. +- **Resource exhaustion on a large dataset** (a `MemoryError`, or a + driver/engine error whose message names memory or disk space). This is not + a data-quality finding and not a connection problem — it means the + operation tried to hold more of the dataset in memory than the machine + running it has. Report plainly that it ran out of resources processing the + full dataset, then route straight to the scope-reduction levers above: a + narrower batch-definition window (lever 1) is the first thing to try, in + the same partitioner-first order used for a slow-but-successful call. + +## Why a retrieved batch doesn't prove anything, for SQL query assets + +`batch_definition.get_batch()` succeeds and returns a real `Batch` object even +when the underlying table or query is broken — for a SQL query asset, nothing +about building the batch touches the database. So retrieving a batch is not, +by itself, evidence of a working configuration. Always follow it with a cheap, +duration-tracked probe before declaring success: + +```python +batch = batch_definition.get_batch() +head = batch.head(n_rows=5) # this is what actually touches the data +``` + +A probe that raises means the configuration doesn't work — report per the +`KeyError` guidance above. A probe that returns means there's a real, working +batch definition. + +## Distinguishing an infrastructure failure from a real data-quality result + +After running an expectation (or a suite) against a batch, `success is False` +can mean two very different things, and they must be reported differently: + +- **The metric itself errored** — a broken column reference, a query that + fails, a type mismatch the engine can't evaluate. Great Expectations + reports this the same way it reports a real failure — `success: False` — + but the result payload is empty. Check `result.result`: an empty dict means + nothing was actually evaluated; there is no data-quality finding to report, + only a configuration problem to fix. Don't guess at what that problem is — + `result.exception_info` names it exactly. On this branch it's a dict with + one entry per metric that errored, keyed by the **string repr** of a + `MetricConfigurationID` (not a `MetricConfigurationID` instance itself — a + keyed lookup with one won't match), and each value is a dict whose + `exception_message` key holds the real cause verbatim, e.g. + `'Error: The column "nope" in BatchData does not exist.'`. Report that + message as the *why*, not a guess. Reached through the normal flow these + messages are short and clean, because the batch-definition probe catches a + broken table long before validation runs. Apply the same length ceiling + used for captured log output anyway: validating against a batch that was + never probed can surface an engine-level message carrying a full traceback, + and the no-raw-traceback rule holds on every path. +- **The data genuinely failed the expectation.** The result payload is + populated — counts, examples, percentages of the specific values that + violated the check. + +```python +result = batch.validate(expectation) +if not result.success and not result.result: + # a metric error: result.exception_info is a dict of + # {str(MetricConfigurationID): {"exception_message": ..., ...}}, one entry + # per metric that errored — report each exception_message verbatim as the + # why, not a data-quality finding. Keys are strings; iterating .items() + # works, but a keyed lookup with a MetricConfigurationID instance won't. + for metric_id, info in result.exception_info.items(): + # tell the user: info["exception_message"] + ... +elif not result.success: + # a genuine data-quality failure: report result.result's counts/examples + ... +``` + +Never report an empty-`result` failure to the user as "your data failed this +check" — it didn't get evaluated at all. diff --git a/great_expectations/.agents/skills/gx-configure-expectations/references/write-out.md b/great_expectations/.agents/skills/gx-configure-expectations/references/write-out.md new file mode 100644 index 000000000000..ee0174fd0a86 --- /dev/null +++ b/great_expectations/.agents/skills/gx-configure-expectations/references/write-out.md @@ -0,0 +1,178 @@ +# Writing an in-memory session out to a project + +An in-memory (ephemeral) session, per `preflight.md`, holds everything only in +process memory — data sources, assets, batch definitions, and expectation +suites all disappear when the process ends. This procedure turns that session +into a real, file-backed project so the work survives and is reusable outside +this conversation. + +Offer this at a natural point — after a data source and batch definition are +verified working, or after a suite has been built and run — not as an +unprompted interruption mid-task. Only do it when the user agrees. + +## Confirm a target directory first + +Never guess a location. Ask the user where the project should live (an +absolute path is safest), and confirm it back before writing anything. + +## The procedure: public factories, not the built-in migrator + +Great Expectations ships a method that converts an in-memory context to a +file-backed one in place. Do not use it here. It resolves the target directory +from the current working directory rather than from an explicit path, it has +a known store-migration ordering issue, and its merge behavior does not +reliably overwrite objects that already exist at the destination — none of +which is acceptable when the target directory and the correctness of the +result both matter. Instead, build the file-backed project explicitly and +re-create each object in it through the same update-safe public factories used +everywhere else in this skill. That gives you a small, fully disclosed +sequence of steps, each independently retryable. + +**`add_or_update_` replaces the datasource wholesale — it is not +additive.** Calling it drops every asset and batch definition already +attached to that datasource, including ones that came from outside this +session: a prior conversation, a teammate, an earlier write-out. Opening a +project that already has a datasource with the name you're about to write and +calling `add_or_update_pandas` (or any `add_or_update_` factory) +under that same name silently destroys every other asset already on it — this +is true the very first time it's called in this project, not just on a +repeat. **Check whether the datasource already exists first, and skip adding +it if it does.** Only call `add_or_update_*` again if you specifically intend +to replace that datasource's connection configuration, and if so, warn the +user first that doing so will drop every other asset already attached to it. + +Build the file-backed project and re-create each object with the pattern +below. Wrap each object in its own try rather than the whole procedure in one +try block, and keep a running record of what succeeded and what didn't. The +asset step needs the datasource handle, and the batch-definition step needs +the asset handle — a zero-arg step that doesn't reflect this can't express +the chain. Have each step **re-fetch its dependency by name** from +`file_context` instead of closing over a variable from an earlier step: that +gets you the same handle without needing an earlier step to have succeeded in +the same function scope, and it makes failures cascade correctly — if the +datasource step failed, the asset step's own fetch of it fails too, with a +reason that points back at the real cause instead of a confusing `NameError`. + +The datasource step follows the same fetch-first-on-`LookupError` shape as +the asset and batch-definition steps below it — that's what makes it safe to +run once, unconditionally, without wiping a datasource that's already there. +List it exactly once, before any asset step, even when the session created +several assets on it: + +```python executable +import great_expectations as gx + +# 1. Open (or create) the file-backed project at the confirmed location. +file_context = gx.get_context(mode="file", project_root_dir="") + +def _add_datasource(): + try: + return file_context.data_sources.get("my_datasource") + except LookupError: + return file_context.data_sources.add_or_update_pandas(name="my_datasource") + +def _add_asset(): + datasource = file_context.data_sources.get("my_datasource") + try: + return datasource.get_asset("my_asset") + except LookupError: + return datasource.add_dataframe_asset(name="my_asset") + +def _add_batch_definition(): + asset = file_context.data_sources.get("my_datasource").get_asset("my_asset") + try: + return asset.get_batch_definition("my_batch_definition") + except LookupError: + return asset.add_batch_definition_whole_dataframe(name="my_batch_definition") + +def _add_suite(): + # `suite` here is the same ExpectationSuite object (or an equivalent one) + # built against the in-memory context earlier in the flow. + return file_context.suites.add_or_update(suite) + +steps = [ + ("data source my_datasource", _add_datasource), + ("asset my_asset", _add_asset), + ("batch definition my_batch_definition", _add_batch_definition), + ("suite my_suite", _add_suite), + # ... one entry per object to re-create: repeat the asset and + # batch-definition pattern (each its own function, closing over its own + # name) for every asset/batch definition created in the session, and the + # suite pattern for every suite. List the datasource step only once, even + # when the session created several assets on it. +] + +written = [] +failed = [] +for label, step in steps: + try: + step() + written.append(label) + except Exception as e: + failed.append((label, str(e))) +``` + +Note the fetch-first pattern in all three of the datasource, asset, and +batch-definition steps. `add_or_update_pandas` (and the other +`add_or_update_` factories) and `suites.add_or_update` are +update-safe on their own — calling either again just replaces that one +object's own content, which is harmless in isolation — but as just covered, +`add_or_update_` is not safe for what's attached underneath a +datasource, which is why the datasource step above fetches first too. There +is no `add_or_update_*` factory at all for a dataframe asset or a batch +definition — calling `add_dataframe_asset` or +`add_batch_definition_whole_dataframe` a second time with the same name +raises instead of updating. Fetching first and only adding on a `LookupError` +is what actually makes every step in this procedure safe to run again — not +the presence of `add_or_update_*` in some of the calls. + +Report both lists to the user explicitly: what was written successfully, and +what wasn't, with the reason for each failure. If an earlier step failed, a +later step that depends on it will fail too — report that as a consequence of +the earlier failure, not as a second, unrelated problem. Because every step +fetches first, re-running the whole procedure after fixing the cause of a +failure is safe — nothing already written gets duplicated, corrupted, or (per +the warning above) destroyed by running it again. + +## Report the written location + +When it completes, tell the user the absolute path that was written to, and +name what landed there (data source names, asset names, batch definition +names, suite names). Don't just say "done" — the point of write-out is that +the user can go find these files. + +## What "usable without modification" means, and its one exception + +Everything written out this way is a standard project artifact: a fresh +`gx.get_context(mode="file", project_root_dir=...)` against that directory +loads the same data sources, assets, batch definitions, and suites, and +`batch_definition.get_batch()` and `batch.validate(suite)` work exactly as +they did in the original session — with one exception. + +**Dataframe assets carry no data.** An in-memory dataframe (a pandas +`DataFrame` passed as the asset's data) is never serialized to disk — only the +asset's *configuration* is written out. After write-out, and in every future +session, retrieving a batch from a dataframe asset still requires passing the +dataframe explicitly at call time: + +```python executable +batch = batch_definition.get_batch(batch_parameters={"dataframe": df}) +``` + +State this to the user when a dataframe asset is part of what got written +out — it's easy to assume the data went with the config, and it didn't. + +## Secrets after write-out: the environment-vs-file split + +An in-memory session resolves `${ENV_VAR}`-style substitutions only from +process environment variables — it has no on-disk uncommitted config file to +read them from, because it has no disk footprint at all. A file-backed project +gains a second, additive source: an uncommitted config-variables file that +lives inside the project directory. Writing a session out to a project does +not change how any existing `${ENV_VAR}` reference resolves — it still comes +from the environment, exactly as before — but it does mean the user now has +the option to move any of those values into the project's uncommitted config +file for anyone else who works in that project without necessarily sharing +the same shell environment. Mention this as a follow-up option; do not do it +for them, and never write a resolved secret value into any file yourself — +only the `${ENV_VAR}`-style reference belongs in a persisted config. diff --git a/great_expectations/__main__.py b/great_expectations/__main__.py new file mode 100644 index 000000000000..dcf7ff378a7a --- /dev/null +++ b/great_expectations/__main__.py @@ -0,0 +1,380 @@ +"""Command line entry point: ``python -m great_expectations``. + +Great Expectations is a library, not a command line application, and this module is +deliberately the whole of its command surface: the one thing a user cannot do from +Python is put files into their own project before an agent reads them, because by then +the decision of what to trust in that project has already been made. Everything here +therefore only prints and installs -- it never touches data. + +The subcommands wrap :mod:`great_expectations.agent_skills.installer`, which reports +per-skill problems rather than raising them. That shapes this envelope: a run that +installs three skills and refuses a fourth has to print all four and still exit +nonzero, so the whole outcome is visible in one screen and a script can tell it apart +from a clean run. The failures the installer *does* raise -- an unusable project +directory, a package that bundles no skills -- are answered with the message it wrote +and a nonzero exit, never a traceback: both are things the user can fix, and a +traceback would bury the sentence that says how. +""" + +from __future__ import annotations + +import argparse +import sys +import textwrap +from collections.abc import Callable, Sequence +from pathlib import Path +from typing import Any, Final + +import great_expectations +from great_expectations.agent_skills.installer import ( + MANIFEST_NAME, + InstallMode, + SkillFailureKind, + SkillInstallFailure, + SkillInstallReport, + SkillTarget, + install_skills, + iter_bundled_skills, + read_skill_manifest, +) + +#: The ``--target`` values, and the discovery directories each one selects. +_TARGETS: Final[dict[str, tuple[SkillTarget, ...]]] = { + "agents": (SkillTarget.AGENTS,), + "claude": (SkillTarget.CLAUDE,), + "all": (SkillTarget.AGENTS, SkillTarget.CLAUDE), +} + +#: Width the installer's reasons are wrapped to. Fixed rather than read from the +#: terminal so that the same command produces the same output everywhere, including +#: in a log or a pipe, where there is no terminal to read. +_REASON_WIDTH: Final = 88 + +_SKILLS_DESCRIPTION: Final = ( + "Great Expectations bundles skills that teach a coding agent to configure data " + "sources and expectations. Agents look for skills in directories inside your " + "project, so the skills have to be installed there before an agent can find them." +) + +_INSTALL_DESCRIPTION: Final = ( + "Install the bundled skills into a project. Safe to run again at any time: skills " + "already installed at this version are left byte-for-byte alone, and a directory " + "that Great Expectations did not install, or that you have edited since it was " + "installed, is reported rather than overwritten." +) + +_LIST_DESCRIPTION: Final = ( + "Show the skills this package bundles and, for each agent directory, whether the " + "skill is installed in the project and which version installed it." +) + +#: Printed once below the failed destinations, whatever went wrong with them. +_FAILURE_FOOTER: Final = "Nothing was changed at the paths above." + +#: Added when the run actually reported a skill as edited. The installer's reason for +#: that one says what to do; this says what "edited" means, because the answer has a +#: consequence nobody guesses: the whole directory is compared, so a file the user +#: never chose to put there counts. Its opening clause names the failures it belongs +#: to, since a run can report edited and unreadable destinations together and advice +#: meant for one would send the user hunting for the wrong thing at the other. +_LOCAL_EDIT_FOOTER: Final = ( + "Where a skill above is reported as edited: it counts as edited when anything " + "inside its directory differs from what was installed -- including a file put " + "there by an editor or by the operating system, such as .DS_Store -- because the " + "whole directory is compared against what was written." +) + + +def _unreportable_project_root_reason(project_root: Path) -> str: + return ( + f"Cannot report the skills installed in {project_root}: it is not an existing " + "directory. Pass the path of the project you want the skills reported for." + ) + + +def main(argv: Sequence[str] | None = None) -> int: + """Run a command and return its exit status. + + Returns: + ``0`` if everything asked for succeeded, ``1`` otherwise. Nothing failing is a + stricter condition than something succeeding: an install that put two skills in + place and refused a third exits nonzero, because a script that treated it as a + success would go on to run an agent that is missing a skill. + """ + parser = _build_parser() + arguments = parser.parse_args(argv) + run: Callable[[argparse.Namespace], int] = arguments.run + try: + return run(arguments) + except OSError as error: + # The installer's messages for these are written to be read by the person who + # typed the command -- they name the path, say what is wrong with it, and give + # the next step -- so the message is the whole of the output. + print(error, file=sys.stderr) + return 1 + + +def _build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + prog="python -m great_expectations", + description="Command line utilities for Great Expectations.", + ) + commands = parser.add_subparsers(metavar="command", required=True) + skills = commands.add_parser( + "skills", + help="install and inspect the agent skills bundled with this package", + description=_SKILLS_DESCRIPTION, + ) + skills_commands = skills.add_subparsers(metavar="command", required=True) + _add_install_parser(skills_commands) + _add_list_parser(skills_commands) + return parser + + +def _add_install_parser(commands: argparse._SubParsersAction) -> None: + install = commands.add_parser( + "install", + help="install the bundled skills into a project", + description=_INSTALL_DESCRIPTION, + ) + _add_project_root_argument(install, "install the skills into") + install.add_argument( + "--target", + choices=tuple(_TARGETS), + default="all", + help=( + "which agent directories to install into: 'agents' for .agents/skills, read " + "by Codex and Cursor; 'claude' for .claude/skills, read by Claude Code and " + "Cursor; 'all' for both, which is the default and serves every supported " + "agent from one run" + ), + ) + install.add_argument( + "--symlink", + action="store_true", + help=( + "link to the skills in the installed package instead of copying them, so " + "that they follow the package when it is upgraded. Not every platform " + "permits symlinks; where they cannot be created the skill is reported as " + "failed and installs normally without this option" + ), + ) + install.add_argument( + "--force", + action="store_true", + help=( + "overwrite skill directories that Great Expectations installed and that " + "have been edited since. A directory it did not install is never " + "overwritten, with or without this option" + ), + ) + install.set_defaults(run=_run_install) + + +def _add_list_parser(commands: argparse._SubParsersAction) -> None: + listing = commands.add_parser( + "list", + help="show the bundled skills and where they are installed", + description=_LIST_DESCRIPTION, + ) + _add_project_root_argument(listing, "report the installed skills of") + listing.set_defaults(run=_run_list) + + +def _add_project_root_argument(parser: argparse.ArgumentParser, purpose: str) -> None: + """Declare ``--project-root``, whose default is resolved later rather than here. + + The default is the working directory, but reading the working directory is a + filesystem call that fails outright once the directory has been deleted -- ordinary + after a build script removes its own directory or a container mount disappears. + Evaluating it while the arguments are merely being *defined* would make that failure + a traceback out of every command, including ``--help`` and a mistyped subcommand, + neither of which needs a working directory at all. + """ + parser.add_argument( + "--project-root", + type=Path, + default=None, + metavar="PATH", + help=f"the project to {purpose} (default: the current directory)", + ) + + +def _project_root(arguments: argparse.Namespace) -> Path: + """Return the project to act on, reading the working directory only if asked to.""" + if arguments.project_root is not None: + project_root: Path = arguments.project_root + return project_root + try: + return Path.cwd() + except OSError as error: + raise OSError(_unusable_working_directory_reason(error)) from error + + +def _unusable_working_directory_reason(error: OSError) -> str: + return ( + f"Cannot read the current directory: {error.strerror or error}. It has usually " + "been deleted or unmounted since this shell started. Change to a directory that " + "still exists, or pass --project-root with the path of the project." + ) + + +def _run_install(arguments: argparse.Namespace) -> int: + """Install the bundled skills and print what happened to every destination.""" + project_root = _project_root(arguments) + report = install_skills( + project_root, + targets=_TARGETS[arguments.target], + mode=InstallMode.SYMLINK if arguments.symlink else InstallMode.COPY, + force=arguments.force, + ) + _print_install_report(report, project_root) + return 1 if report.failed else 0 + + +def _print_install_report(report: SkillInstallReport, project_root: Path) -> None: + """Print every destination the run considered, grouped by what happened to it.""" + print(f"Great Expectations {great_expectations.__version__} skills in {project_root}") + _print_group("Installed", report.installed, project_root) + _print_group("Updated", report.replaced, project_root) + _print_group("Already up to date", report.up_to_date, project_root) + _print_failures(report.failed, project_root) + if not report.installed and not report.replaced and not report.failed: + # Saying so beats an empty run: the user asked for something to happen, and + # "nothing did, and that is the right answer" is the news. + print("\nEvery bundled skill was already installed at this version. Nothing to do.") + + +def _print_group(heading: str, destinations: Sequence[Path], project_root: Path) -> None: + if not destinations: + return + print(f"\n{heading} ({len(destinations)})") + for destination in destinations: + print(f" {_display_path(destination, project_root)}") + + +def _print_failures(failures: Sequence[SkillInstallFailure], project_root: Path) -> None: + """Print the failed destinations with the installer's reason for each. + + The reasons are reproduced whole. Each one already names the state the destination + is in and one thing to do about it, and shortening them here would leave the user + with a path and no way to act on it. + """ + if not failures: + return + print(f"\nFailed ({len(failures)})") + for failure in failures: + print(f" {_display_path(failure.destination, project_root)}") + print(_wrap(failure.reason, indent=" ")) + footer = _FAILURE_FOOTER + if any(failure.kind is SkillFailureKind.LOCALLY_MODIFIED for failure in failures): + # Asked of the report rather than of the filesystem: what a destination looks + # like afterwards cannot tell an edited directory from one that could not be + # read, and both leave the destination sitting there with a valid manifest. + footer = f"{footer} {_LOCAL_EDIT_FOOTER}" + print() + print(_wrap(footer)) + + +def _wrap(text: str, indent: str = "") -> str: + """Fill a paragraph without breaking a path across two lines. + + A wrapped path cannot be copied out of the terminal, and every message here exists + to be acted on. An over-long path is left to overflow instead. + """ + return textwrap.fill( + text, + width=_REASON_WIDTH, + initial_indent=indent, + subsequent_indent=indent, + break_long_words=False, + break_on_hyphens=False, + ) + + +def _run_list(arguments: argparse.Namespace) -> int: + """Print the bundled skills and what each agent directory of the project holds. + + Reporting only, so it succeeds even when the project is out of date: an install + that has not been re-run since an upgrade is the state this command exists to + make visible, not a failure of the command. + """ + project_root = _project_root(arguments) + if not project_root.is_dir(): + raise NotADirectoryError(_unreportable_project_root_reason(project_root)) + + version = great_expectations.__version__ + skills = list(iter_bundled_skills()) + print(f"Great Expectations {version} bundles {len(skills)} agent skills.") + print(f"Installed state in {project_root}:") + + stale = False + for skill in skills: + print(f"\n{skill.name}") + for target in SkillTarget: + state, is_stale = _installed_state(project_root / target.value / skill.name, version) + stale = stale or is_stale + print(f" {target.value:<15} {state}") + if stale: + print( + "\nSome skills were installed by a different version of Great Expectations.\n" + "Run 'python -m great_expectations skills install' to bring them up to date." + ) + return 0 + + +def _installed_state(destination: Path, version: str) -> tuple[str, bool]: + """Describe what is installed at one destination, and whether it is out of date. + + Ownership is read through the installer's own manifest reader, which treats a + manifest that is missing, unreadable, or not Great Expectations' as the same + answer. Anything else would let this command claim a directory the install command + would refuse to touch. + + Presence is decided without following links, so a link pointing at nothing -- what a + symlink install becomes when the package it pointed at is gone -- is reported as + something being there rather than as an empty destination. + """ + try: + destination.lstat() + except FileNotFoundError: + return "not installed", False + except OSError as error: + # Usually an unreadable parent directory. Not knowing is its own answer: this + # command is read from to decide whether to install, and "not installed" is a + # claim about a destination whose state was never actually seen. + return f"cannot be read: {error.strerror or error}", False + manifest = read_skill_manifest(destination) + if manifest is None: + return f"present, but not installed by Great Expectations (no {MANIFEST_NAME})", False + installed_version = _manifest_string(manifest, "gx_version") or "an unrecorded version" + mode = _manifest_string(manifest, "mode") + described = f"installed by {installed_version}" + if mode: + described = f"{described} ({mode})" + if installed_version == version: + return described, False + return f"{described} -- this package is {version}", True + + +def _manifest_string(manifest: dict[str, Any], key: str) -> str | None: + """Read one field of a manifest, tolerating a manifest that does not hold it. + + A manifest is a file in the user's project: it can be older than this code, or + hand-edited. A field this command cannot use is reported as unknown rather than + allowed to end the run. + """ + value = manifest.get(key) + return value if isinstance(value, str) else None + + +def _display_path(destination: Path, project_root: Path) -> str: + """Show a destination relative to the project, which is how the user thinks of it.""" + try: + return str(destination.relative_to(project_root)) + except ValueError: + return str(destination) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/great_expectations/agent_skills/__init__.py b/great_expectations/agent_skills/__init__.py new file mode 100644 index 000000000000..79024989ff63 --- /dev/null +++ b/great_expectations/agent_skills/__init__.py @@ -0,0 +1,5 @@ +"""Support for the agent skills bundled with this package. + +Deliberately empty of re-exports: the supported way to use what lives here is the +command line (``python -m great_expectations skills ...``), not an import. +""" diff --git a/great_expectations/agent_skills/installer.py b/great_expectations/agent_skills/installer.py new file mode 100644 index 000000000000..f914c3f670ee --- /dev/null +++ b/great_expectations/agent_skills/installer.py @@ -0,0 +1,693 @@ +"""Install the skills bundled in this package into a project's agent directories. + +Coding agents discover skills by reading well-known directories inside the project they +are working in, so guidance that ships inside an installed Python package is invisible +to them until it is copied (or linked) into one of those directories. This module is +that bridge, and the whole of its difficulty is that the destination belongs to the +user, not to Great Expectations: + +* **The install command has to be safe to re-run.** Users run it again after every + upgrade, and often just because they are not sure whether they ran it. A second run + must therefore be a no-op rather than a rewrite. +* **Great Expectations must only ever replace its own files.** A directory that this + package did not create, or one it created and the user has since edited, is not + the installer's to overwrite. Each installed skill directory therefore carries an ownership + manifest recording the version and a content hash, which is what lets a later run + tell "unchanged copy of an older version" (safe to replace) apart from "the user + edited this" (refuse) and "someone else's directory" (refuse, always). +* **A crash must not leave a half-written skill.** A skill directory whose entry + document survived but whose references did not is worse than no skill at all, + because an agent will happily follow the truncated remains. Every write is + therefore staged in a sibling directory and moved into place with a rename, so the + destination is only ever the complete old tree, absent, or the complete new tree. + +Problems with one skill never abort the others and are never raised: they are collected +in the returned report, each labelled with what went wrong, so the caller can disclose +every one of them and explain the ones that need explaining. Nothing here is part of +the public Python API -- the command-line entry point is the supported surface. +""" + +from __future__ import annotations + +import contextlib +import enum +import hashlib +import importlib.util +import json +import shutil +import uuid +from collections.abc import Iterator, Sequence +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Final + +#: Name of the ownership manifest written into every skill directory this module +#: installs. Its presence is what marks a directory as safe for a later run to +#: replace. +MANIFEST_NAME: Final = ".gx-skill.json" + +_MANAGED_BY: Final = "great_expectations" +_PACKAGE_NAME: Final = "great_expectations" + +#: Location of the bundled skills inside the installed package. +_BUNDLE_RELPATH: Final = (".agents", "skills") + +#: A directory is a skill if and only if it holds an entry document. +_ENTRY_DOCUMENT: Final = "SKILL.md" + +#: Prefix for staging directories. Reserved, recognizable, and swept at the start of +#: every run so that a directory left behind by an interrupted run is cleaned up. +_STAGING_PREFIX: Final = ".gx-tmp-" + + +class SkillTarget(enum.Enum): + """A project-relative directory that coding agents search for skills. + + Values are the directory each platform reads: Codex and the wider ecosystem read + ``.agents/skills``, Claude Code reads ``.claude/skills``, and Cursor reads both -- + which is why installing into both by default is what makes one command serve all + three. + """ + + AGENTS = ".agents/skills" + CLAUDE = ".claude/skills" + + +class InstallMode(enum.Enum): + """How an installed skill relates to the bundled copy in the package. + + ``COPY`` is the default because it is the only mode every platform is known to + support and the only one that survives the package being upgraded or removed. + ``SYMLINK`` trades that robustness for content that tracks the installed package + without re-running the command. + """ + + COPY = "copy" + SYMLINK = "symlink" + + +class SkillFailureKind(enum.Enum): + """What went wrong at a destination, as a fact rather than something to deduce. + + A caller that explains failures has to tell a refusal -- a destination this package + declines to touch because of what it holds -- apart from a destination it simply + could not read or write. The two call for opposite advice, and nothing observable + about the destination afterwards distinguishes them: a directory refused for local + edits and a directory whose files could not be read both still exist, both still + hold a valid manifest, and both leave the run's own filesystem state unchanged. The + only place that knows which happened is the code that decided, so it says so here. + """ + + #: Nothing there claims Great Expectations as its owner. Never replaced. + FOREIGN_DESTINATION = "foreign_destination" + #: Installed by this package and edited since. Replaced only under ``force``. + LOCALLY_MODIFIED = "locally_modified" + #: Could not be read, so whether it was safe to replace could not be decided. + UNREADABLE_DESTINATION = "unreadable_destination" + #: The new content could not be written or moved into place. + WRITE_FAILED = "write_failed" + #: Symlinks were asked for and the platform would not create them. + SYMLINKS_UNSUPPORTED = "symlinks_unsupported" + + +@dataclass(frozen=True) +class SkillInstallFailure: + """One destination the run left alone, and why.""" + + destination: Path + kind: SkillFailureKind + #: Text written to be read by the user: what happened, the state the destination is + #: in now, and one thing to do about it. + reason: str + + +@dataclass(frozen=True) +class SkillInstallReport: + """The outcome of an install run, one entry per skill per target directory. + + Every destination the run considered appears in exactly one of the four fields, so + a caller can report the whole run without inferring anything. + """ + + #: Destinations that did not exist and were created. + installed: tuple[Path, ...] + #: Destinations already holding this version of the skill; left untouched. + up_to_date: tuple[Path, ...] + #: Unmodified destinations from another version, replaced with the bundled skill. + replaced: tuple[Path, ...] + #: Destinations left alone: refusals and write failures alike, each labelled. + failed: tuple[SkillInstallFailure, ...] + + +class _Outcome(enum.Enum): + """Which report field a successfully handled destination belongs to.""" + + INSTALLED = "installed" + UP_TO_DATE = "up_to_date" + REPLACED = "replaced" + + +class _SkillRefusal(Exception): + """A problem with a single destination: reported to the user, never raised at them. + + The message is the text the user reads next to the destination path, so it says + what happened, what state the destination is in now, and what to do about it. The + kind travels with it because a caller that groups or explains failures cannot + recover it from the message without matching on prose. + """ + + def __init__(self, kind: SkillFailureKind, reason: str) -> None: + super().__init__(reason) + self.kind = kind + + +@dataclass(frozen=True) +class _InstallContext: + """The settings shared by every destination in a single run.""" + + mode: InstallMode + force: bool + version: str + + +_FOREIGN_DESTINATION_REASON: Final = ( + "Something already exists at this path that Great Expectations does not manage: it " + f"holds no {MANIFEST_NAME} manifest. It was left untouched. Move or delete it if " + "you want Great Expectations to install its skill here." +) + +_LOCALLY_MODIFIED_REASON: Final = ( + "Great Expectations installed this skill, but it has local edits: its contents no " + f"longer match the {MANIFEST_NAME} manifest recorded when it was installed. It was " + "left untouched, so no edits were lost. Save a copy of your changes elsewhere, or " + "re-run the install with --force to overwrite this directory with the bundled skill." +) + + +def _write_failure_reason(error: BaseException) -> str: + return ( + f"Could not write this skill into the project: {error}. The destination was " + "left as it was and any partly written files were removed. Check the free " + "space and write permissions on the destination, then run the install again." + ) + + +def _swap_failure_reason(error: BaseException) -> str: + return ( + f"Could not move this skill into place: {error}. The previous contents were " + f"restored where possible; a leftover {_STAGING_PREFIX}* directory beside this " + "path, if any, holds them and is removed by the next install run." + ) + + +def _symlink_failure_reason(error: BaseException) -> str: + return ( + f"Could not create the symlinks for this skill: {error}. Some platforms only " + "permit symlinks for privileged accounts. The destination was left as it was; " + "re-run the install without --symlink to install file copies instead." + ) + + +def _missing_bundle_reason(searched: Sequence[str]) -> str: + locations = ", ".join(searched) if searched else "the installed package" + return ( + "The installed great_expectations package bundles no agent skills: no " + f"{'/'.join(_BUNDLE_RELPATH)} directory was found in {locations}. Re-install " + "great_expectations, and if the problem persists, report it as a packaging bug." + ) + + +def _empty_bundle_reason(root: Path) -> str: + return ( + f"The installed great_expectations package bundles no agent skills: {root} " + f"holds no directory containing a {_ENTRY_DOCUMENT} file. A partly packaged " + "installation looks exactly like this. Re-install great_expectations, and if " + "the problem persists, report it as a packaging bug." + ) + + +def _unreadable_destination_reason(error: OSError) -> str: + path = error.filename or "a path inside this directory" + return ( + f"Could not read {path} while checking whether this skill is up to date: " + f"{error.strerror or error}. The destination was left untouched. Check the " + "permissions on that path -- an install run by another user can leave files " + "this one cannot read -- then run the install again." + ) + + +def _unreadable_bundle_reason(error: OSError) -> str: + path = error.filename or "a bundled skill file" + return ( + f"Cannot read {path} in the installed great_expectations package: " + f"{error.strerror or error}. The bundled skills have to be readable to be " + "installed, so this is a defect in the installation rather than in the " + "project. Re-install great_expectations, and if the problem persists, report " + "it as a packaging bug." + ) + + +def _unusable_project_root_reason(project_root: Path) -> str: + return ( + f"Cannot install skills into {project_root}: it is not an existing directory. " + "Pass the path of the project you want the skills installed into." + ) + + +def read_skill_manifest(directory: Path) -> dict[str, Any] | None: + """Return the ownership manifest of ``directory``, or ``None`` if it has none. + + ``None`` means the directory was not installed by this package -- whether because + the manifest is missing, unreadable, not valid JSON, or does not claim Great + Expectations as its owner. Every one of those cases has to be treated identically: + the only safe reading of a directory whose ownership cannot be proved is that it + belongs to someone else. + """ + try: + raw = (directory / MANIFEST_NAME).read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None + try: + manifest = json.loads(raw) + except json.JSONDecodeError: + return None + if not isinstance(manifest, dict) or manifest.get("managed_by") != _MANAGED_BY: + return None + return manifest + + +def iter_bundled_skills() -> Iterator[Path]: + """Yield the skill directories bundled in the installed package, ordered by name. + + Resolution goes through the import system rather than through this file's location, + so the same code finds the skills in a wheel install, an editable install and a + source checkout: in each of them the skills sit beside the package's ``__init__``, + wherever the import system says that is. + + Raises: + FileNotFoundError: if the installed package carries no usable bundled skills -- + whether it has no bundle directory at all or a bundle directory holding + nothing that qualifies as a skill. Both are defects in the installation + rather than problems the caller can do anything about per skill, and both + have to raise: returning no skills would let a caller report a run in which + nothing was installed as a run in which nothing went wrong. A partly + packaged installation -- reference files shipped, entry documents missed -- + produces exactly that second shape. + OSError: if the bundle itself cannot be read. An installation whose own files + are unreadable is defective in the same way, and saying so beats a bare + permission error naming a path in someone else's site-packages. + """ + root = _bundled_skills_root() + try: + skills = sorted(p for p in root.iterdir() if (p / _ENTRY_DOCUMENT).is_file()) + except OSError as error: + raise OSError(_unreadable_bundle_reason(error)) from error + if not skills: + raise FileNotFoundError(_empty_bundle_reason(root)) + return iter(skills) + + +def install_skills( + project_root: Path, + *, + targets: Sequence[SkillTarget] = (SkillTarget.AGENTS, SkillTarget.CLAUDE), + mode: InstallMode = InstallMode.COPY, + force: bool = False, +) -> SkillInstallReport: + """Install every bundled skill into each target directory under ``project_root``. + + Re-running this is always safe: destinations already holding this version are left + byte-for-byte alone, and a destination that Great Expectations did not install, or + that has been edited since it was installed, is refused rather than overwritten. + ``force`` opts into overwriting the edited ones; nothing opts into overwriting a + directory without an ownership manifest. + + Args: + project_root: the project the skills are installed into. + targets: the discovery directories to install into. Both, by default, which is + what makes one run serve every supported coding agent. + mode: whether to install copies of the skill files or symlinks to them. + force: replace skill directories that were installed by Great Expectations and + have been edited since. Never applies to directories this package did + not install. + + Returns: + A report with every destination in exactly one of its four fields. Problems + with individual skills are reported there, never raised. + + Raises: + OSError: if ``project_root`` is not an existing directory, or the installed + package bundles no skills, or the bundle cannot be read. Each makes the + whole run meaningless, unlike a per-destination problem, which is reported. + """ + root = Path(project_root) + if not root.is_dir(): + raise NotADirectoryError(_unusable_project_root_reason(root)) + + skills = list(iter_bundled_skills()) + try: + digests = {skill: _tree_digest(skill) for skill in skills} + except OSError as error: + raise OSError(_unreadable_bundle_reason(error)) from error + context = _InstallContext(mode=mode, force=force, version=_installed_gx_version()) + + outcomes: dict[_Outcome, list[Path]] = {outcome: [] for outcome in _Outcome} + failed: list[SkillInstallFailure] = [] + + for target in targets: + parent = root / target.value + try: + parent.mkdir(parents=True, exist_ok=True) + except OSError as error: + reason = _write_failure_reason(error) + failed.extend( + SkillInstallFailure(parent / skill.name, SkillFailureKind.WRITE_FAILED, reason) + for skill in skills + ) + continue + _clear_staging_remnants(parent) + for skill in skills: + destination = parent / skill.name + try: + outcome = _install_one(skill, destination, digests[skill], context) + except _SkillRefusal as refusal: + failed.append(SkillInstallFailure(destination, refusal.kind, str(refusal))) + else: + outcomes[outcome].append(destination) + + return SkillInstallReport( + installed=tuple(outcomes[_Outcome.INSTALLED]), + up_to_date=tuple(outcomes[_Outcome.UP_TO_DATE]), + replaced=tuple(outcomes[_Outcome.REPLACED]), + failed=tuple(failed), + ) + + +def _install_one( + source: Path, destination: Path, digest: str, context: _InstallContext +) -> _Outcome: + """Bring a single destination in line with a single bundled skill. + + The order of the checks is the contract: ownership before staleness, and staleness + only for destinations that still match their own manifest. Comparing versions first + would replace a directory the user had edited, on the very command that is supposed + to be safe to re-run. + """ + if not _lexists(destination): + _materialize(source, destination, digest, context) + return _Outcome.INSTALLED + + manifest = read_skill_manifest(destination) + if manifest is None: + raise _SkillRefusal(SkillFailureKind.FOREIGN_DESTINATION, _FOREIGN_DESTINATION_REASON) + + try: + unmodified = _is_unmodified(destination, manifest) + current = unmodified and _is_current(destination, source, manifest, digest, context) + except OSError as error: + # Inspecting a destination means reading it, and the destination belongs to the + # user: a path left unreadable by an install run as another user, or by a + # restrictive umask, has to cost this one destination and no more. + raise _SkillRefusal( + SkillFailureKind.UNREADABLE_DESTINATION, _unreadable_destination_reason(error) + ) from error + + if not unmodified and not context.force: + raise _SkillRefusal(SkillFailureKind.LOCALLY_MODIFIED, _LOCALLY_MODIFIED_REASON) + if current: + return _Outcome.UP_TO_DATE + + _materialize(source, destination, digest, context) + return _Outcome.REPLACED + + +def _is_unmodified(destination: Path, manifest: dict[str, Any]) -> bool: + """Report whether a destination still matches the manifest written when it was made. + + This is the question "has the user changed this?", which is asked of the + destination against its own recorded state -- not against the bundled skill, which + legitimately differs after an upgrade. + """ + if manifest.get("mode") == InstallMode.SYMLINK.value: + return _links_are_intact(destination) + return _tree_digest(destination) == manifest.get("content_sha256") + + +def _is_current( + destination: Path, + source: Path, + manifest: dict[str, Any], + digest: str, + context: _InstallContext, +) -> bool: + """Report whether a destination already holds exactly what this run would install.""" + if ( + manifest.get("gx_version") != context.version + or manifest.get("content_sha256") != digest + or manifest.get("mode") != context.mode.value + ): + return False + if context.mode is InstallMode.SYMLINK: + # The links serve the package's current content, but only if they still point + # at it: an upgrade can add a file, and a moved environment invalidates them. + return _links_point_at(destination, source) + return True + + +def _links_are_intact(destination: Path) -> bool: + """Report whether a symlink-mode destination is still nothing but links. + + Content edits cannot be detected by hashing here -- the content lives in the + package and the links follow it -- so what is checked is the structure the install + created. A link the user replaced with a real file is a local modification. + """ + try: + entries = [entry for entry in destination.iterdir() if entry.name != MANIFEST_NAME] + except OSError: + return False + return bool(entries) and all(entry.is_symlink() for entry in entries) + + +def _links_point_at(destination: Path, source: Path) -> bool: + """Report whether the destination links exactly mirror the bundled skill's entries.""" + try: + expected = {entry.name: entry for entry in source.iterdir()} + actual = { + entry.name: entry for entry in destination.iterdir() if entry.name != MANIFEST_NAME + } + if set(actual) != set(expected): + return False + return all(link.readlink() == expected[name] for name, link in actual.items()) + except OSError: + return False + + +def _materialize(source: Path, destination: Path, digest: str, context: _InstallContext) -> None: + """Build the skill in a staging directory, then move it onto the destination. + + Nothing is written at the destination until a complete tree exists beside it, so a + failure -- or a crash -- at any point here leaves the destination either untouched + or replaced whole, and at worst a staging directory the next run sweeps away. + """ + staging = _staging_path(destination) + try: + _stage(source, staging, context.mode) + _write_manifest(staging, digest, context) + except _SkillRefusal: + _remove(staging) + raise + except OSError as error: + _remove(staging) + raise _SkillRefusal(SkillFailureKind.WRITE_FAILED, _write_failure_reason(error)) from error + _swap_into_place(staging, destination) + + +def _stage(source: Path, staging: Path, mode: InstallMode) -> None: + """Assemble the skill's content in the staging directory. + + A symlink inside a bundled skill is copied as a symlink rather than followed. That + is what keeps the copy a copy: dereferencing would write content from outside the + bundle into the user's project, and it would put a real file where the digest of + the source recorded a link, so the destination could never match its own manifest + and every later run would report an untouched install as edited. + """ + if mode is InstallMode.SYMLINK: + try: + staging.mkdir(parents=True) + for entry in sorted(source.iterdir()): + (staging / entry.name).symlink_to(entry, target_is_directory=entry.is_dir()) + except (OSError, NotImplementedError) as error: + raise _SkillRefusal( + SkillFailureKind.SYMLINKS_UNSUPPORTED, _symlink_failure_reason(error) + ) from error + else: + try: + shutil.copytree(source, staging, symlinks=True) + except (OSError, shutil.Error) as error: + raise _SkillRefusal( + SkillFailureKind.WRITE_FAILED, _write_failure_reason(error) + ) from error + + +def _swap_into_place(staging: Path, destination: Path) -> None: + """Move a fully staged directory onto the destination. + + A rename onto a name that does not exist is atomic, and that is the whole of the + fresh-install case. Replacing an existing directory cannot be one rename, because + renaming onto a non-empty directory is not allowed, so it is two: the old tree is + renamed aside to a staging name first. The guarantee is therefore not that the + swap is a single atomic step, but that no intermediate state is ever a partly + written skill -- the destination is the old tree, then briefly absent, then the new + tree, and the tree renamed aside is swept by the next run if this one dies. + """ + if not _lexists(destination): + try: + staging.replace(destination) + except OSError as error: + _remove(staging) + raise _SkillRefusal( + SkillFailureKind.WRITE_FAILED, _write_failure_reason(error) + ) from error + return + + previous = _staging_path(destination) + try: + destination.replace(previous) + except OSError as error: + _remove(staging) + raise _SkillRefusal(SkillFailureKind.WRITE_FAILED, _write_failure_reason(error)) from error + try: + staging.replace(destination) + except OSError as error: + with contextlib.suppress(OSError): + previous.replace(destination) + _remove(staging) + raise _SkillRefusal(SkillFailureKind.WRITE_FAILED, _swap_failure_reason(error)) from error + _remove(previous) + + +def _write_manifest(directory: Path, digest: str, context: _InstallContext) -> None: + """Record what was installed, so a later run can tell this copy from an edited one.""" + manifest = { + "managed_by": _MANAGED_BY, + "gx_version": context.version, + "content_sha256": digest, + "mode": context.mode.value, + } + (directory / MANIFEST_NAME).write_text( + json.dumps(manifest, indent=2, sort_keys=True) + "\n", encoding="utf-8" + ) + + +def _tree_digest(root: Path) -> str: + """Hash the contents of a directory tree. + + Determinism across machines is the point -- the hash recorded on one machine is + compared against a tree on another after an upgrade -- so paths are ordered and + encoded in their platform-independent form, each entry is tagged by kind and + length-framed so that a rename cannot produce the same digest as an edit, and the + walk never descends through a symlinked directory, whose contents are not this + tree's to describe. The manifest itself is excluded because it holds this digest. + + A symlink is hashed by the path it points at rather than by what it resolves to. + Resolving would make a file and a link to an identical file indistinguishable, when + replacing one with the other is exactly the kind of change to an installed skill + that has to be noticed -- and it would make the digest of a tree depend on files + outside it. + """ + digest = hashlib.sha256() + for relpath, path in sorted(_walk(root)): + if relpath == MANIFEST_NAME: + continue + if path.is_symlink(): + digest.update(f"{relpath}\0L\0{path.readlink().as_posix()}\0".encode()) + elif path.is_file(): + payload = path.read_bytes() + digest.update(f"{relpath}\0F\0{len(payload)}\0".encode()) + digest.update(payload) + return digest.hexdigest() + + +def _walk(root: Path) -> Iterator[tuple[str, Path]]: + """Yield every entry below ``root`` as a relative path, without following links. + + Written out rather than delegated to a recursive glob because the standard + library's has followed symlinked directories in some versions and not in others, + and the digest built on top of this cannot afford to depend on which. + """ + pending = [root] + while pending: + for entry in pending.pop().iterdir(): + yield entry.relative_to(root).as_posix(), entry + if entry.is_dir() and not entry.is_symlink(): + pending.append(entry) + + +def _bundled_skills_root() -> Path: + """Locate the bundled skills in the installed package.""" + try: + spec = importlib.util.find_spec(_PACKAGE_NAME) + except (ImportError, ValueError): + spec = None + locations = list(spec.submodule_search_locations or ()) if spec is not None else [] + for location in locations: + candidate = Path(location).joinpath(*_BUNDLE_RELPATH) + if candidate.is_dir(): + return candidate + raise FileNotFoundError(_missing_bundle_reason(locations)) + + +def _installed_gx_version() -> str: + """Read the version of the package these skills were bundled with. + + Read at call time, and from the package rather than from distribution metadata, so + that it is right for an editable install as well as a released wheel. + """ + import great_expectations + + return great_expectations.__version__ + + +def _staging_path(destination: Path) -> Path: + """Return an unused, recognizably temporary sibling of the destination. + + A sibling, because a rename is only guaranteed to be cheap and atomic within one + filesystem, and the destination's parent is the only directory known to be on the + same one. + """ + return destination.parent / f"{_STAGING_PREFIX}{destination.name}-{uuid.uuid4().hex[:12]}" + + +def _clear_staging_remnants(parent: Path) -> None: + """Sweep staging directories left behind by an interrupted run. + + Best effort, including the listing itself: if the target directory cannot even be + read, that is worth reporting against each destination inside it rather than + aborting the run here, where there is nothing to report it against. Listing is + spelled with ``iterdir`` rather than a glob for the same reason the digest's walk + is: how the standard library's glob treats an unreadable directory is an + implementation detail, and this needs to behave the same way everywhere. + """ + with contextlib.suppress(OSError): + for entry in parent.iterdir(): + if entry.name.startswith(_STAGING_PREFIX): + _remove(entry) + + +def _remove(path: Path) -> None: + """Delete a file, link or tree, best effort: cleanup must not mask the real error.""" + with contextlib.suppress(OSError): + if path.is_dir() and not path.is_symlink(): + shutil.rmtree(path, ignore_errors=True) + else: + path.unlink(missing_ok=True) + + +def _lexists(path: Path) -> bool: + """Report whether anything is at ``path``, including a link to nothing.""" + try: + path.lstat() + except (OSError, ValueError): + return False + else: + return True diff --git a/setup.py b/setup.py index 285e22edd83a..f36ee3008da0 100644 --- a/setup.py +++ b/setup.py @@ -153,6 +153,15 @@ def get_extras_require(): # become part of the distribution. "expectations/core/schemas/*.json", "datasource/fluent/schemas/**/*.json", + # Agent-facing guidance, read by a coding agent rather than imported by + # Python. Matched by file rather than by a directory glob, because a + # setuptools package_data pattern only ever selects files -- an empty + # directory in the pattern's path is never itself a match, and wheels + # cannot record an empty directory anyway. Scoped to the skills tree + # rather than a blanket markdown glob, so an unrelated markdown file + # added elsewhere in the package does not silently become part of the + # distribution. + ".agents/skills/**/*.md", ] }, "name": "great_expectations", diff --git a/tests/agent_skills/__init__.py b/tests/agent_skills/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/agent_skills/test_installer.py b/tests/agent_skills/test_installer.py new file mode 100644 index 000000000000..907ffa415a3f --- /dev/null +++ b/tests/agent_skills/test_installer.py @@ -0,0 +1,2580 @@ +"""Filesystem tests for installing the bundled agent skills into a project. + +The install command writes into a directory that belongs to the user rather than to this +package, which makes most of what it does a promise about what it will *not* touch: a +second run must not rewrite an unchanged copy, an edited copy must survive being refused, +a directory Great Expectations never created must be byte-for-byte untouched even under +``--force``, and a write that fails must leave nothing behind. None of those promises can +be checked by reading the report a run returns -- a report is what the installer *says* +it did -- so every assertion below is made against the filesystem: bytes, link targets, +ownership manifests, modification times and inode numbers. + +Each check is a function returning a list of problems, and each one is paired with a test +that runs the same check against a deliberately broken build of the installer -- copying +that dereferences symlinks, an ownership check that trusts every directory, a staged +write that goes straight to the destination -- and asserts the check reports it. A +filesystem check that quietly stopped comparing anything would otherwise keep passing +forever while asserting nothing, which is the failure mode these tests exist to prevent. + +Everything runs against directories built under ``tmp_path``. Nothing here installs into +the checkout, and nothing reaches the network. +""" + +from __future__ import annotations + +import argparse +import contextlib +import dataclasses +import hashlib +import importlib.util +import itertools +import json +import pathlib +import shutil +import stat +import types +from collections.abc import Iterator, Sequence +from typing import Callable, Final + +import pytest + +import great_expectations +from great_expectations import __main__ as command_line +from great_expectations.agent_skills import installer +from great_expectations.agent_skills.installer import ( + MANIFEST_NAME, + InstallMode, + SkillFailureKind, + SkillInstallFailure, + SkillInstallReport, + SkillTarget, + install_skills, +) + +pytestmark = [pytest.mark.unit] + +PROJECT_ROOT: Final = pathlib.Path(__file__).parents[2] +BUNDLED_SKILLS_ROOT: Final = PROJECT_ROOT / "great_expectations" / ".agents" / "skills" + +ENTRY_DOCUMENT: Final = "SKILL.md" +REFERENCE_DIR: Final = "references" +STAGING_PREFIX: Final = ".gx-tmp-" + +ALL_TARGETS: Final = (SkillTarget.AGENTS, SkillTarget.CLAUDE) + +#: Number of skills the package is known to bundle. Guards the checks over the real +#: bundle against a discovery bug reducing them to nothing. +MIN_BUNDLED_SKILLS: Final = 2 + +#: Two synthetic skills into two target directories. Every check over the synthetic +#: bundle asserts it had at least this many destinations to look at, so a scenario that +#: silently stopped installing anything cannot pass by comparing empty sets. +MIN_DESTINATIONS: Final = 4 + +#: Versions the synthetic package claims. Neither is a real release, so a check that +#: read the real version by accident fails rather than passes. +INSTALLED_VERSION: Final = "1.3.0.test" +EARLIER_VERSION: Final = "1.2.0.test" + +FILE: Final = "file" +LINK: Final = "link" +DIRECTORY: Final = "directory" + +#: How deep the synthetic bundle really goes (``references/guide.md``). A walk that +#: descended through the symlink cycles below would report paths far deeper than this. +REAL_TREE_DEPTH: Final = 2 +#: Depth reached by a link-following walk before the demonstration below gives up. Well +#: past the real depth and well short of the point where the kernel refuses to resolve +#: any more symlink components. +CYCLE_DEPTH_EVIDENCE: Final = 8 +#: Entries the link-following demonstration is allowed to visit before it is cut off. A +#: walk that terminates on its own never reaches this. +CYCLE_WALK_LIMIT: Final = 400 + + +# --------------------------------------------------------------------------- +# Recording what is on disk, independently of the code under test. +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class Entry: + """One path in a tree, as the filesystem holds it. + + Identity is deliberately recorded twice over: ``kind`` and ``payload`` say what the + user would read, while ``mtime_ns`` and ``inode`` say whether it was written. A run + that rewrote a file with identical bytes changes the second pair and not the first, + and "already up to date" is a claim about both. + """ + + kind: str + #: Link target for a link, a hash of the bytes for a file, empty for a directory. + payload: str + size: int + mtime_ns: int + inode: int + + +def walk(root: pathlib.Path) -> Iterator[pathlib.Path]: + """Yield every path below ``root`` without following links. + + Spelled out here rather than borrowed from the installer: a test that measured the + filesystem with the code under test would report a broken walk as an unchanged tree. + """ + pending = [root] + while pending: + for entry in sorted(pending.pop().iterdir()): + yield entry + if entry.is_dir() and not entry.is_symlink(): + pending.append(entry) + + +def snapshot(root: pathlib.Path) -> dict[str, Entry]: + """Record a whole tree, keyed by path relative to ``root``.""" + recorded: dict[str, Entry] = {} + for path in walk(root): + stats = path.lstat() + if path.is_symlink(): + kind, payload, size = LINK, str(path.readlink()), 0 + elif path.is_file(): + payload_bytes = path.read_bytes() + kind, payload, size = ( + FILE, + hashlib.sha256(payload_bytes).hexdigest(), + len(payload_bytes), + ) + else: + kind, payload, size = DIRECTORY, "", 0 + recorded[path.relative_to(root).as_posix()] = Entry( + kind=kind, payload=payload, size=size, mtime_ns=stats.st_mtime_ns, inode=stats.st_ino + ) + return recorded + + +def contents(recorded: dict[str, Entry]) -> dict[str, tuple[str, str]]: + """Reduce a snapshot to what a reader would see, dropping when it was written.""" + return {name: (entry.kind, entry.payload) for name, entry in recorded.items()} + + +def describe(entry: Entry) -> str: + if entry.kind == LINK: + return f"a link to {entry.payload}" + if entry.kind == FILE: + return f"a file of {entry.size} bytes ({entry.payload[:12]})" + return "a directory" + + +# --------------------------------------------------------------------------- +# The synthetic package the scenarios install from. +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class Bundle: + """A stand-in for the skills directory inside an installed package.""" + + root: pathlib.Path + names: tuple[str, ...] + + @property + def skills(self) -> list[pathlib.Path]: + return [self.root / name for name in self.names] + + def source_for(self, destination: pathlib.Path) -> pathlib.Path: + return self.root / destination.name + + +@pytest.fixture +def bundle(tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch) -> Bundle: + """A package bundling two skills, with a directory that is not one alongside them. + + Synthetic rather than the real content because the scenarios need to change what the + package holds -- an upgrade adds a file, a defective packaging run drops an entry + document -- and because a check that reads the real bundle would change meaning every + time the shipped guidance is edited. The real bundle is covered separately. + """ + root = tmp_path / "site-packages" / "great_expectations" / ".agents" / "skills" + names = ("gx-first-skill", "gx-second-skill") + for index, name in enumerate(names): + skill = root / name + (skill / REFERENCE_DIR).mkdir(parents=True) + (skill / ENTRY_DOCUMENT).write_text( + f"---\nname: {name}\n---\n\n# {name}\n\nSee `{REFERENCE_DIR}/guide.md`.\n", + encoding="utf-8", + ) + (skill / REFERENCE_DIR / "guide.md").write_text(f"# guide {index}\n", encoding="utf-8") + not_a_skill = root / "shared-fragments" + not_a_skill.mkdir() + (not_a_skill / "fragment.md").write_text("# not a skill: no entry document\n", encoding="utf-8") + + monkeypatch.setattr(installer, "_bundled_skills_root", lambda: root) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + return Bundle(root=root, names=names) + + +@pytest.fixture +def bundle_with_links(bundle: Bundle) -> Bundle: + """The same package, with a skill holding an ordinary link and a dangling one. + + Both belong here: a link that resolves is what a build step or a packaging tool + leaves behind, and a link that does not is what an upgrade leaves behind when the + file it pointed at is dropped. Neither may abort an install, and neither may make an + installed copy hash differently from the skill it was copied from. + """ + references = bundle.root / bundle.names[0] / REFERENCE_DIR + (references / "shared.md").symlink_to(pathlib.Path("guide.md")) + (references / "dropped.md").symlink_to(pathlib.Path("gone.md")) + return bundle + + +@pytest.fixture +def project(tmp_path: pathlib.Path) -> pathlib.Path: + root = tmp_path / "project" + root.mkdir() + return root + + +def expected_destinations( + project: pathlib.Path, bundle: Bundle, targets: Sequence[SkillTarget] = ALL_TARGETS +) -> list[pathlib.Path]: + return [project / target.value / name for target in targets for name in bundle.names] + + +def stamp_manifest_version(destination: pathlib.Path, version: str) -> None: + """Rewrite the version an installed skill records, leaving its content alone. + + This is what an installed skill looks like after the package is upgraded: the tree + still matches the hash its own manifest recorded, so it is unmodified, but it is no + longer what this version of the package would install. The manifest is excluded from + the hash, so editing it here does not make the destination look edited. + """ + path = destination / MANIFEST_NAME + manifest = json.loads(path.read_text(encoding="utf-8")) + manifest["gx_version"] = version + path.write_text(json.dumps(manifest, indent=2, sort_keys=True) + "\n", encoding="utf-8") + + +@contextlib.contextmanager +def made_unreadable(path: pathlib.Path) -> Iterator[None]: + """Take away every permission on ``path`` for the duration, then give them back.""" + original = stat.S_IMODE(path.lstat().st_mode) + path.chmod(0o000) + try: + yield + finally: + path.chmod(original) + + +def is_readable(path: pathlib.Path) -> bool: + try: + list(path.iterdir()) + except OSError: + return False + return True + + +# --------------------------------------------------------------------------- +# Checks. Each returns the problems it found, so the same code can be asserted +# empty against the real installer and non-empty against a broken one. +# --------------------------------------------------------------------------- + + +def describe_report(report: SkillInstallReport) -> str: + failed = ", ".join(f"{failure.destination}: {failure.kind.value}" for failure in report.failed) + return ( + f"installed={[str(path) for path in report.installed]}," + f" up_to_date={[str(path) for path in report.up_to_date]}," + f" replaced={[str(path) for path in report.replaced]}," + f" failed=[{failed}]" + ) + + +def failure_for( + report: SkillInstallReport, destination: pathlib.Path +) -> SkillInstallFailure | None: + for failure in report.failed: + if failure.destination == destination: + return failure + return None + + +def partition_problems(report: SkillInstallReport, expected: Sequence[pathlib.Path]) -> list[str]: + """Every destination the run considered must appear in exactly one outcome. + + A caller reports the whole run by printing the four fields, so a destination in two + of them is reported twice and one in none of them is never mentioned at all. + """ + if not expected: + return ["the run considered no destinations, so its partition proves nothing"] + groups = { + "installed": tuple(report.installed), + "up_to_date": tuple(report.up_to_date), + "replaced": tuple(report.replaced), + "failed": tuple(failure.destination for failure in report.failed), + } + problems: list[str] = [] + for destination in sorted(set(expected).union(*(set(group) for group in groups.values()))): + appearances = [ + name for name, group in groups.items() for path in group if path == destination + ] + if len(appearances) > 1: + problems.append(f"{destination} is reported in {sorted(appearances)}, not in one.") + elif not appearances: + problems.append(f"{destination} is reported in no outcome; the run must place it.") + elif destination not in expected: + problems.append( + f"{destination} is reported as {appearances[0]} but is not a destination" + " this run had to consider." + ) + return problems + + +def manifest_problems( + destination: pathlib.Path, source: pathlib.Path, version: str, mode: InstallMode +) -> list[str]: + """The ownership manifest must record who installed this, at which version and how.""" + path = destination / MANIFEST_NAME + try: + manifest = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as error: + return [f"{path}: no readable ownership manifest ({error})."] + expected = { + "managed_by": "great_expectations", + "gx_version": version, + "content_sha256": installer._tree_digest(source), + "mode": mode.value, + } + return [ + f"{path}: {field} is {manifest.get(field)!r}, expected {value!r}." + for field, value in expected.items() + if manifest.get(field) != value + ] + + +def difference_problems( + source: pathlib.Path, + destination: pathlib.Path, + expected: dict[str, tuple[str, str]], + actual: dict[str, tuple[str, str]], +) -> list[str]: + problems: list[str] = [] + for name in sorted(set(expected) - set(actual)): + problems.append(f"{destination}: {name} is missing; {source} holds it.") + for name in sorted(set(actual) - set(expected)): + problems.append(f"{destination}: {name} was installed but is not part of {source}.") + for name in sorted(set(actual) & set(expected)): + if actual[name] != expected[name]: + problems.append( + f"{destination}: {name} is {actual[name][0]} ({actual[name][1][:12]}) but" + f" {source / name} is {expected[name][0]} ({expected[name][1][:12]})." + ) + return problems + + +def installed_copy_problems( + source: pathlib.Path, destination: pathlib.Path, version: str +) -> list[str]: + """A copy install must reproduce the bundled skill entry for entry. + + The digests are compared as well as the trees, because the digest is what every later + run decides on: a copy that reads the same but hashes differently is reported as + edited for ever afterwards. + """ + if destination.is_symlink() or not destination.is_dir(): + return [f"{destination} is not a directory; a copy install must create one."] + expected = contents(snapshot(source)) + if not expected: + return [f"{source} holds nothing, so comparing {destination} against it proves nothing."] + actual = contents(snapshot(destination)) + problems: list[str] = [] + if actual.pop(MANIFEST_NAME, None) is None: + problems.append( + f"{destination} holds no {MANIFEST_NAME}; nothing records who installed it." + ) + problems += difference_problems(source, destination, expected, actual) + problems += manifest_problems(destination, source, version, InstallMode.COPY) + installed_digest = installer._tree_digest(destination) + if installed_digest != installer._tree_digest(source): + problems.append( + f"{destination} hashes to {installed_digest[:12]} but {source} hashes to" + f" {installer._tree_digest(source)[:12]}; every later run would call this copy edited." + ) + return problems + + +def linked_install_problems( + source: pathlib.Path, destination: pathlib.Path, version: str +) -> list[str]: + """A symlink install must be a real directory of links into the installed package. + + Not a single link to the package: the ownership manifest is written into the + destination, and a link would put it inside the installed package instead. + """ + if destination.is_symlink() or not destination.is_dir(): + return [f"{destination} is not a directory; a symlink install must create one."] + expected = {entry.name for entry in source.iterdir()} + if not expected: + return [f"{source} holds nothing to link to."] + actual = {entry.name: entry for entry in destination.iterdir() if entry.name != MANIFEST_NAME} + problems: list[str] = [] + if set(actual) != expected: + problems.append(f"{destination} links {sorted(actual)}; {source} holds {sorted(expected)}.") + for name, entry in sorted(actual.items()): + if not entry.is_symlink(): + problems.append( + f"{entry} is a real file or directory rather than a link into {source}." + ) + elif entry.readlink() != source / name: + problems.append(f"{entry} links to {entry.readlink()}, not to {source / name}.") + problems += entry_document_readable_problems(source, destination) + problems += manifest_problems(destination, source, version, InstallMode.SYMLINK) + return problems + + +def entry_document_readable_problems(source: pathlib.Path, destination: pathlib.Path) -> list[str]: + """The entry document must read back through the destination, or no agent finds it.""" + try: + installed = (destination / ENTRY_DOCUMENT).read_bytes() + except OSError as error: + return [f"{destination / ENTRY_DOCUMENT} cannot be read: {error}."] + if installed != (source / ENTRY_DOCUMENT).read_bytes(): + return [f"{destination / ENTRY_DOCUMENT} does not read back as {source / ENTRY_DOCUMENT}."] + return [] + + +def unchanged_problems(destination: pathlib.Path, before: dict[str, Entry]) -> list[str]: + """Nothing under ``destination`` may have been read back differently, or rewritten.""" + if not before: + return [f"{destination} held nothing before the run, so comparing it proves nothing."] + if destination.is_symlink() or not destination.is_dir(): + return [ + f"{destination} is no longer a directory; it held {len(before)} entries" + " before the run and has to hold them still." + ] + after = snapshot(destination) + problems: list[str] = [] + for name in sorted(set(after) - set(before)): + problems.append( + f"{destination}: {name} appeared ({describe(after[name])}) in an untouched run." + ) + for name in sorted(set(before) - set(after)): + problems.append(f"{destination}: {name} was removed ({describe(before[name])}).") + for name in sorted(set(before) & set(after)): + old, new = before[name], after[name] + if (new.kind, new.payload) != (old.kind, old.payload): + problems.append( + f"{destination}: {name} was rewritten: {describe(old)} became {describe(new)}." + ) + elif (new.mtime_ns, new.inode) != (old.mtime_ns, old.inode): + problems.append( + f"{destination}: {name} was written again with the same content" + " (its modification time or inode changed); an untouched run writes nothing." + ) + return problems + + +def staging_remnant_problems(project: pathlib.Path) -> list[str]: + """No half-written tree may be left beside a destination once a run has returned.""" + problems: list[str] = [] + for target in ALL_TARGETS: + parent = project / target.value + if not parent.is_dir(): + continue + for entry in sorted(parent.iterdir()): + if entry.name.startswith(STAGING_PREFIX): + problems.append( + f"{entry} was left behind; a run must remove what it staged before returning." + ) + return problems + + +def reason_problems(failure: SkillInstallFailure, mentions: Sequence[str] = ()) -> list[str]: + problems: list[str] = [] + if not failure.reason.strip(): + problems.append(f"{failure.destination}: the failure carries no reason to show the user.") + problems += [ + f"{failure.destination}: the reason does not name {mention}: {failure.reason}" + for mention in mentions + if mention not in failure.reason + ] + return problems + + +def refusal_problems( + report: SkillInstallReport, + destination: pathlib.Path, + kind: SkillFailureKind, + mentions: Sequence[str] = (), +) -> list[str]: + failure = failure_for(report, destination) + if failure is None: + return [ + f"{destination} must be reported as failed with {kind.value};" + f" got {describe_report(report)}" + ] + if failure.kind is not kind: + return [f"{destination} was refused as {failure.kind.value}, expected {kind.value}."] + return reason_problems(failure, mentions) + + +# --------------------------------------------------------------------------- +# Scenarios: setup, one or two runs, and the filesystem afterwards. +# --------------------------------------------------------------------------- + + +def fresh_install_problems(project: pathlib.Path, bundle: Bundle) -> list[str]: + report = install_skills(project, targets=ALL_TARGETS) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + problems += partition_problems(report, expected) + if set(report.installed) != set(expected): + problems.append( + f"a first run must install every destination; got {describe_report(report)}" + ) + for destination in expected: + problems += installed_copy_problems( + bundle.source_for(destination), destination, INSTALLED_VERSION + ) + for target in ALL_TARGETS: + stray = project / target.value / "shared-fragments" + if stray.exists(): + problems.append(f"{stray}: a directory holding no {ENTRY_DOCUMENT} is not a skill.") + problems += staging_remnant_problems(project) + return problems + + +def non_vacuity_problems(expected: Sequence[pathlib.Path]) -> list[str]: + if len(expected) < MIN_DESTINATIONS: + return [ + f"expected at least {MIN_DESTINATIONS} destinations to check, got" + f" {[str(path) for path in expected]}" + ] + return [] + + +def idempotency_problems(project: pathlib.Path, bundle: Bundle, mode: InstallMode) -> list[str]: + first = install_skills(project, targets=ALL_TARGETS, mode=mode) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + if first.failed: + return [ + *problems, + f"the first run must succeed before idempotency means anything;" + f" got {describe_report(first)}", + ] + before = {destination: snapshot(destination) for destination in expected} + + second = install_skills(project, targets=ALL_TARGETS, mode=mode) + + problems += partition_problems(second, expected) + if set(second.up_to_date) != set(expected): + problems.append( + f"a second run must leave every destination alone; got {describe_report(second)}" + ) + for destination, recorded in before.items(): + problems += unchanged_problems(destination, recorded) + problems += staging_remnant_problems(project) + return problems + + +@dataclasses.dataclass(frozen=True) +class InstalledProject: + """A project with every skill installed, and what everything looked like then. + + Both sides are recorded, the project and the package, because the questions a + re-run answers are all comparisons between the two: what the destination held when + it was installed, and what the package ships now. + """ + + project: pathlib.Path + bundle: Bundle + destinations: list[pathlib.Path] + installed: dict[pathlib.Path, dict[str, Entry]] + bundled: dict[str, dict[str, tuple[str, str]]] + + +def install_and_record(project: pathlib.Path, bundle: Bundle) -> InstalledProject: + """Install every skill and record the state a later run will be measured against.""" + install_skills(project, targets=ALL_TARGETS) + destinations = expected_destinations(project, bundle) + return InstalledProject( + project=project, + bundle=bundle, + destinations=destinations, + installed={destination: snapshot(destination) for destination in destinations}, + bundled={skill.name: contents(snapshot(skill)) for skill in bundle.skills}, + ) + + +def rewrite_bundled_skills(bundle: Bundle) -> None: + """Change what the package ships, leaving its version alone. + + Kept separate from the version so that either can be moved without the other. A + release changes both; a source checkout or an editable install changes only this, + and that is the case in which the recorded hash is the only thing that notices. + """ + for skill in bundle.skills: + (skill / ENTRY_DOCUMENT).write_text( + f"---\nname: {skill.name}\n---\n\n# {skill.name}, rewritten since the install\n", + encoding="utf-8", + ) + (skill / REFERENCE_DIR / "added.md").write_text( + "# added since the install\n", encoding="utf-8" + ) + + +def replacement_problems( + state: InstalledProject, report: SkillInstallReport, content_changed: bool +) -> list[str]: + """Every destination must now hold what the package holds. + + ``content_changed`` is a dimension the caller sets, not something this decides: a + run can reach here because the version moved, because the content moved, or because + both did, and a check that assumed one of those would stop being able to tell the + others apart. It is verified before it is used -- a "content moved" run in which the + package did not actually change proves nothing, and neither does a "version only" + run in which the content moved as well. + """ + problems = non_vacuity_problems(state.destinations) + problems += partition_problems(report, state.destinations) + if set(report.replaced) != set(state.destinations): + problems.append(f"every installed skill must be replaced; got {describe_report(report)}") + for destination in state.destinations: + source = state.bundle.source_for(destination) + moved = contents(snapshot(source)) != state.bundled[source.name] + if moved is not content_changed: + problems.append( + f"{source} {'changed' if moved else 'did not change'} since the install," + f" which is not the run this is checking (content_changed={content_changed})." + ) + problems += installed_copy_problems(source, destination, great_expectations.__version__) + if content_changed: + problems += arrival_problems(destination, state.installed[destination]) + problems += staging_remnant_problems(state.project) + return problems + + +def arrival_problems(destination: pathlib.Path, before: dict[str, Entry]) -> list[str]: + """The new content has to have reached the destination, not just the manifest. + + A run that rewrote the ownership manifest and nothing else reports every destination + as replaced and leaves the user reading the skill they had before. + """ + was = {name: (entry.kind, entry.payload) for name, entry in before.items()} + now = contents(snapshot(destination)) + if {name: entry for name, entry in was.items() if name != MANIFEST_NAME} == { + name: entry for name, entry in now.items() if name != MANIFEST_NAME + }: + return [ + f"{destination} holds exactly what it held before the run: the changed skill" + " never reached the project, whatever the report says." + ] + return [] + + +@dataclasses.dataclass(frozen=True) +class EditedSkill: + """An installed skill the user has since edited, and the run's other destinations.""" + + project: pathlib.Path + bundle: Bundle + edited: pathlib.Path + others: list[pathlib.Path] + before: dict[str, Entry] + + +def install_and_edit(project: pathlib.Path, bundle: Bundle) -> EditedSkill: + """Install every skill, then add a file to one of the installed copies. + + The edit and the package version are deliberately left as two separate dimensions. + A setup that always bumped the version alongside the edit would leave "the user + edited this" and "this is from an older release" indistinguishable, and no test built + on it could show which of the two a refusal -- or a repair -- was really keyed on. + """ + install_skills(project, targets=ALL_TARGETS) + edited = project / SkillTarget.AGENTS.value / bundle.names[0] + (edited / REFERENCE_DIR / "notes.md").write_text("notes the user added\n", encoding="utf-8") + expected = expected_destinations(project, bundle) + return EditedSkill( + project=project, + bundle=bundle, + edited=edited, + others=[destination for destination in expected if destination != edited], + before=snapshot(edited), + ) + + +def edited_skill_problems( + state: EditedSkill, report: SkillInstallReport, force: bool, stale: bool +) -> list[str]: + """What a re-run must have done to an edited copy, and to everything around it. + + ``stale`` says only whether the package moved on since the install, which changes + what happens to the destinations that were *not* edited. The edited one's outcome is + the same either way, and that is the contract: ownership is decided before staleness, + so a run that compared versions first would replace an edited copy and lose the edits + on the very command that is supposed to be safe to re-run. + """ + expected = [state.edited, *state.others] + problems = non_vacuity_problems(expected) + problems += partition_problems(report, expected) + untouched = report.replaced if stale else report.up_to_date + if set(untouched) - {state.edited} != set(state.others): + expectation = "brought up to the new version" if stale else "left up to date" + problems.append( + f"the destinations that were not edited must be {expectation};" + f" got {describe_report(report)}" + ) + if force: + if state.edited not in report.replaced: + problems.append(f"--force must replace {state.edited}; got {describe_report(report)}") + problems += forced_repair_problems(state) + else: + problems += refusal_problems(report, state.edited, SkillFailureKind.LOCALLY_MODIFIED) + problems += unchanged_problems(state.edited, state.before) + problems += staging_remnant_problems(state.project) + return problems + + +def forced_repair_problems(state: EditedSkill) -> list[str]: + """After ``--force``, the edited copy must be the bundled skill and nothing else.""" + problems: list[str] = [] + if (state.edited / REFERENCE_DIR / "notes.md").exists(): + problems.append(f"{state.edited} still holds the edit --force was asked to overwrite.") + problems += installed_copy_problems( + state.bundle.source_for(state.edited), state.edited, great_expectations.__version__ + ) + return problems + + +#: An ownership manifest that parses, and names an owner that is not this package. +ANOTHER_OWNERS_MANIFEST: Final = json.dumps( + {"managed_by": "some-other-tool", "gx_version": INSTALLED_VERSION, "mode": "copy"}, + indent=2, + sort_keys=True, +) + +#: Bytes that are not text at all: what a truncated write, a compressed file restored +#: under the wrong name, or an editor saving in another encoding leaves behind. +UNDECODABLE_MANIFEST: Final = b'{"managed_by": "\xff\xfegreat_expectations"}' + +#: Every way a destination can fail to prove that Great Expectations installed it, one +#: per branch that decides it: absent or unreadable, bytes that will not decode, text +#: that will not parse, JSON that is not a mapping, and a mapping naming another owner. +#: Ownership that cannot be proved is ownership by someone else, so all of these have to +#: be refused identically -- and none of them may end the run, since the manifest is read +#: before the point at which a destination's problems start being caught. +UNPROVEN_OWNERSHIP: Final = [ + pytest.param(None, id="no_manifest"), + pytest.param(UNDECODABLE_MANIFEST, id="undecodable"), + pytest.param("{ this never parsed", id="unparseable"), + pytest.param('["great_expectations"]', id="not_a_mapping"), + pytest.param(ANOTHER_OWNERS_MANIFEST, id="another_owner"), +] + + +def undecodable_problems(manifest: bytes) -> list[str]: + """The bytes have to be undecodable, or the case they stand for is not being tested.""" + try: + manifest.decode("utf-8") + except UnicodeDecodeError: + return [] + return [f"{manifest!r} decodes as UTF-8, so it does not stand for a manifest that cannot."] + + +def foreign_destination_problems( + project: pathlib.Path, bundle: Bundle, force: bool, manifest: str | bytes | None = None +) -> list[str]: + """A destination this package did not install is never replaced, with or without force. + + ``manifest`` is what sits at the ownership manifest's path: nothing, or a file that + fails to prove ownership -- as text that cannot be decoded, cannot be parsed, or + parses into something that is not this package's. A directory another tool manages is + the case that looks most like one of ours, since it holds a manifest and that + manifest parses, so only the recorded owner tells them apart. + """ + foreign = project / SkillTarget.CLAUDE.value / bundle.names[0] + foreign.mkdir(parents=True) + (foreign / ENTRY_DOCUMENT).write_text("---\nname: mine\n---\n\n# a skill I wrote\n", "utf-8") + (foreign / "notes.md").write_text("notes of my own\n", encoding="utf-8") + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + if isinstance(manifest, bytes): + (foreign / MANIFEST_NAME).write_bytes(manifest) + problems += undecodable_problems(manifest) + elif manifest is not None: + (foreign / MANIFEST_NAME).write_text(manifest, encoding="utf-8") + before = snapshot(foreign) + + try: + report = install_skills(project, targets=ALL_TARGETS, force=force) + except Exception as error: + # Caught rather than allowed to end the test, because "the run did not survive + # this destination" is the finding, and the module's whole promise is that a + # problem with one skill is reported instead of raised. + return [ + *problems, + "a destination that cannot be read as one of ours must cost that destination" + f" and no more; the run raised {error!r}", + ] + + problems += partition_problems(report, expected) + problems += refusal_problems(report, foreign, SkillFailureKind.FOREIGN_DESTINATION) + problems += unchanged_problems(foreign, before) + others = [destination for destination in expected if destination != foreign] + if set(report.installed) != set(others): + problems.append( + f"refusing one destination must not stop the others; got {describe_report(report)}" + ) + for destination in others: + problems += installed_copy_problems( + bundle.source_for(destination), destination, INSTALLED_VERSION + ) + problems += staging_remnant_problems(project) + return problems + + +def write_failure_problems(project: pathlib.Path, bundle: Bundle) -> list[str]: + """A failed first write must disclose itself and leave nothing at the destination.""" + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + + report = install_skills(project, targets=ALL_TARGETS) + + problems += partition_problems(report, expected) + for destination in expected: + problems += refusal_problems(report, destination, SkillFailureKind.WRITE_FAILED) + if destination.exists() or destination.is_symlink(): + problems.append( + f"{destination} exists after a write that failed; a run that could not" + " finish must leave nothing an agent could read." + ) + problems += staging_remnant_problems(project) + return problems + + +def failed_replacement_problems( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +) -> list[str]: + """A failed replacement must leave the skill that was already installed intact.""" + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + install_skills(project, targets=ALL_TARGETS) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + before = {destination: snapshot(destination) for destination in expected} + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + break_writing(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS) + + problems += partition_problems(report, expected) + for destination in expected: + problems += refusal_problems(report, destination, SkillFailureKind.WRITE_FAILED) + problems += unchanged_problems(destination, before[destination]) + problems += staging_remnant_problems(project) + return problems + + +def swap_failure_problems( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch, restore: bool = True +) -> list[str]: + """A rename that fails while the destination is briefly absent must put it back. + + Replacing an existing skill cannot be one rename, because renaming onto a non-empty + directory is not allowed: the old tree is moved aside first, and between the two + renames the destination does not exist. A failure in that window is the only way this + module can lose a skill it was asked to update, so the previous contents are moved + back and the failure is reported against that destination alone. + """ + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + install_skills(project, targets=ALL_TARGETS) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + before = {destination: snapshot(destination) for destination in expected} + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + break_the_swap(monkeypatch, restore=restore) + + report = install_skills(project, targets=ALL_TARGETS) + + problems += partition_problems(report, expected) + for destination in expected: + # The reason names the staging directory the previous contents may be sitting in, + # which is what makes it worth reading next to a failure that never got that far. + problems += refusal_problems( + report, destination, SkillFailureKind.WRITE_FAILED, mentions=[STAGING_PREFIX] + ) + problems += unchanged_problems(destination, before[destination]) + problems += staging_remnant_problems(project) + return problems + + +def remnant_sweep_problems(project: pathlib.Path, bundle: Bundle) -> list[str]: + """A tree left behind by an interrupted run is cleaned up by the next one. + + Staging beside the destination is what keeps a crash from producing a half-written + skill, and it is only free of litter if the leftovers are swept: without this the + project accumulates a directory per interrupted run, for ever. + """ + install_skills(project, targets=ALL_TARGETS) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + remnant = project / SkillTarget.AGENTS.value / f"{STAGING_PREFIX}{bundle.names[0]}-9c1f0ae4b2d7" + (remnant / REFERENCE_DIR).mkdir(parents=True) + (remnant / ENTRY_DOCUMENT).write_text("half a skill, left by a crash\n", encoding="utf-8") + if not remnant.is_dir(): + return [f"{remnant} was not created, so sweeping it proves nothing."] + before = {destination: snapshot(destination) for destination in expected} + + report = install_skills(project, targets=ALL_TARGETS) + + problems += partition_problems(report, expected) + if set(report.up_to_date) != set(expected): + problems.append( + f"a run over an unchanged project must leave it alone; got {describe_report(report)}" + ) + problems += staging_remnant_problems(project) + for destination in expected: + problems += unchanged_problems(destination, before[destination]) + return problems + + +def unreadable_destination_problems(project: pathlib.Path, bundle: Bundle) -> list[str]: + """One path this user cannot read must cost one destination, not the whole run.""" + install_skills(project, targets=ALL_TARGETS) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + damaged = project / SkillTarget.AGENTS.value / bundle.names[0] + unreadable = damaged / REFERENCE_DIR + before = {destination: snapshot(destination) for destination in expected} + others = [destination for destination in expected if destination != damaged] + + report: SkillInstallReport | None = None + with made_unreadable(unreadable): + if is_readable(unreadable): + return [ + f"{unreadable} is still readable with no permissions at all, so this check" + " cannot mean anything; it has to run as a user the filesystem restricts." + ] + try: + report = install_skills(project, targets=ALL_TARGETS) + except OSError as error: + problems.append( + f"a path that cannot be read must cost one destination, not the run;" + f" the run raised {error!r}" + ) + if report is None: + return problems + + problems += partition_problems(report, expected) + problems += refusal_problems( + report, damaged, SkillFailureKind.UNREADABLE_DESTINATION, mentions=[str(unreadable)] + ) + if set(report.up_to_date) != set(others): + problems.append( + f"every other destination must still be handled; got {describe_report(report)}" + ) + for destination in expected: + problems += unchanged_problems(destination, before[destination]) + return problems + + +def empty_bundle_problems(project: pathlib.Path) -> list[str]: + """A package that bundles nothing must be refused, not reported as a clean run.""" + try: + report = install_skills(project, targets=ALL_TARGETS) + except FileNotFoundError as error: + problems = [] + if "bundles no agent skills" not in str(error): + problems.append(f"the refusal must say the package bundles no skills; got {error}") + if any(project.iterdir()): + problems.append(f"{project} must be left alone when there is nothing to install.") + return problems + return [ + "a package bundling no skills must be refused: a run in which nothing was installed" + f" was reported as a run in which nothing went wrong ({describe_report(report)})" + ] + + +def mode_switch_problems( + project: pathlib.Path, bundle: Bundle, installed_as: InstallMode, asked_for: InstallMode +) -> list[str]: + """Re-running in a different mode must convert what is installed, not skip it. + + The two modes hold the same content, the same version and the same recorded hash, so + the only thing that distinguishes an installed copy from an installed link is the + mode the manifest records. A run that did not compare it would tell a user who asked + for links that everything was already up to date, and leave them with copies. + """ + install_skills(project, targets=ALL_TARGETS, mode=installed_as) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + + report = install_skills(project, targets=ALL_TARGETS, mode=asked_for) + + problems += partition_problems(report, expected) + if set(report.replaced) != set(expected): + problems.append( + f"asking for {asked_for.value} where {installed_as.value} is installed must" + f" replace every destination; got {describe_report(report)}" + ) + check = linked_install_problems if asked_for is InstallMode.SYMLINK else installed_copy_problems + for destination in expected: + problems += check( + bundle.source_for(destination), destination, great_expectations.__version__ + ) + problems += staging_remnant_problems(project) + return problems + + +def symlink_mode_problems(project: pathlib.Path, bundle: Bundle) -> list[str]: + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + + report = install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + + problems += partition_problems(report, expected) + if set(report.installed) != set(expected): + problems.append( + f"a first run must install every destination; got {describe_report(report)}" + ) + for destination in expected: + problems += linked_install_problems( + bundle.source_for(destination), destination, INSTALLED_VERSION + ) + problems += staging_remnant_problems(project) + return problems + + +# --------------------------------------------------------------------------- +# Deliberately broken builds of the installer, each modelling a defect the +# checks above exist to catch. +# --------------------------------------------------------------------------- + + +def stop_writing_manifests(monkeypatch: pytest.MonkeyPatch) -> None: + """Installs the content but records no ownership, so no later run can tell it is ours.""" + monkeypatch.setattr(installer, "_write_manifest", lambda *arguments: None) + + +def dereference_symlinks(monkeypatch: pytest.MonkeyPatch) -> None: + """Copies what a link points at instead of the link, the way a plain copytree does.""" + real = shutil.copytree + + def copytree(source, destination, symlinks=False, *arguments, **keywords): + keywords.pop("symlinks", None) + return real(source, destination, False, *arguments, **keywords) + + monkeypatch.setattr(shutil, "copytree", copytree) + + +def ignore_the_version_stamp(monkeypatch: pytest.MonkeyPatch) -> None: + """Treats every destination as stale, so a second run rewrites what it just wrote.""" + monkeypatch.setattr(installer, "_is_current", lambda *arguments: False) + + +def treat_everything_as_current(monkeypatch: pytest.MonkeyPatch) -> None: + """Treats every destination as up to date, so an upgrade never reaches the project.""" + monkeypatch.setattr(installer, "_is_current", lambda *arguments: True) + + +def stop_comparing_the_recorded_hash(monkeypatch: pytest.MonkeyPatch) -> None: + """Compares the version and the mode, but not the content the manifest recorded. + + Two releases can ship identical skills, and a source checkout ships changed skills + under the version it was already carrying, so the recorded hash is the only thing + that notices content moving without the version moving. Without it, anyone + developing against an editable install re-runs the command, is told everything is + up to date, and never sees their change reach the project. + """ + + def is_current(destination, source, manifest, digest, context): + if ( + manifest.get("gx_version") != context.version + or manifest.get("mode") != context.mode.value + ): + return False + if context.mode is InstallMode.SYMLINK: + return installer._links_point_at(destination, source) + return True + + monkeypatch.setattr(installer, "_is_current", is_current) + + +def ignore_local_edits(monkeypatch: pytest.MonkeyPatch) -> None: + """Trusts every managed destination, so the user's edits are silently overwritten.""" + monkeypatch.setattr(installer, "_is_unmodified", lambda *arguments: True) + + +def install_nothing(monkeypatch: pytest.MonkeyPatch) -> None: + """Reports every destination as written and writes nothing at all.""" + monkeypatch.setattr(installer, "_materialize", lambda *arguments: None) + + +def claim_every_directory(monkeypatch: pytest.MonkeyPatch) -> None: + """Reads an ownership manifest out of any directory, including the user's own.""" + monkeypatch.setattr( + installer, + "read_skill_manifest", + lambda directory: {"managed_by": "great_expectations", "mode": InstallMode.COPY.value}, + ) + + +def stop_checking_the_owner(monkeypatch: pytest.MonkeyPatch) -> None: + """Accepts any manifest that parses, without asking whose it is. + + The narrower slip: a manifest that is missing, unreadable or not a mapping is still + rejected, so every destination that holds no manifest behaves exactly as before. Only + a directory another tool manages changes hands -- and it is the case that looks most + like one of ours. + """ + + def read_skill_manifest(directory): + try: + raw = (directory / MANIFEST_NAME).read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None + try: + manifest = json.loads(raw) + except json.JSONDecodeError: + return None + return manifest if isinstance(manifest, dict) else None + + monkeypatch.setattr(installer, "read_skill_manifest", read_skill_manifest) + + +def stop_swallowing_undecodable_manifests(monkeypatch: pytest.MonkeyPatch) -> None: + """Reads the manifest as text without allowing for bytes that are not text. + + Every other way of failing to prove ownership is answered with "not ours". This one + escapes instead -- and it escapes from the one call made before the caller starts + guarding, so it does not cost a destination, it ends the whole run: every other + skill and every other target with it, under a traceback rather than a report. + """ + + def read_skill_manifest(directory): + try: + raw = (directory / MANIFEST_NAME).read_text(encoding="utf-8") + except OSError: + return None + try: + manifest = json.loads(raw) + except json.JSONDecodeError: + return None + if not isinstance(manifest, dict) or manifest.get("managed_by") != "great_expectations": + return None + return manifest + + monkeypatch.setattr(installer, "read_skill_manifest", read_skill_manifest) + + +def ignore_edits_when_deciding_staleness(monkeypatch: pytest.MonkeyPatch) -> None: + """Decides staleness without first asking whether the copy was edited. + + Production asks "does this already hold what the run would install?" only of copies + that still match their own manifest. Dropping that one conjunction leaves an edited + copy at the current version looking up to date, so ``--force`` reports success, + writes nothing, and the edit stays where it is: a repair the command claims to have + made and did not. Everything else about this build is production's own code. + """ + + def install_one(source, destination, digest, context): + if not installer._lexists(destination): + installer._materialize(source, destination, digest, context) + return installer._Outcome.INSTALLED + manifest = installer.read_skill_manifest(destination) + if manifest is None: + raise installer._SkillRefusal( + SkillFailureKind.FOREIGN_DESTINATION, installer._FOREIGN_DESTINATION_REASON + ) + try: + unmodified = installer._is_unmodified(destination, manifest) + current = installer._is_current(destination, source, manifest, digest, context) + except OSError as error: + raise installer._SkillRefusal( + SkillFailureKind.UNREADABLE_DESTINATION, + installer._unreadable_destination_reason(error), + ) from error + if not unmodified and not context.force: + raise installer._SkillRefusal( + SkillFailureKind.LOCALLY_MODIFIED, installer._LOCALLY_MODIFIED_REASON + ) + if current: + return installer._Outcome.UP_TO_DATE + installer._materialize(source, destination, digest, context) + return installer._Outcome.REPLACED + + monkeypatch.setattr(installer, "_install_one", install_one) + + +def break_the_swap(monkeypatch: pytest.MonkeyPatch, restore: bool = True) -> None: + """Fails the rename that moves a staged tree into place. + + By then the previous tree has already been renamed aside, so this is the one moment + at which the destination does not exist. ``restore`` leaves the second attempt -- + the one that puts the previous tree back -- working, which is what production + promises; turning it off models a filesystem that fails that too, and shows what the + check would have to notice. + """ + real = pathlib.Path.replace + failed: set[str] = set() + + def replace(self, target): + target = pathlib.Path(target) + moving_into_place = self.name.startswith(STAGING_PREFIX) and not target.name.startswith( + STAGING_PREFIX + ) + if moving_into_place and (not restore or str(target) not in failed): + failed.add(str(target)) + raise OSError(16, "Device or resource busy", str(target)) + return real(self, target) + + monkeypatch.setattr(pathlib.Path, "replace", replace) + + +def break_writing(monkeypatch: pytest.MonkeyPatch) -> None: + """Fails partway through writing, the way a full disk does: some files, then an error.""" + + def copytree(source, destination, *arguments, **keywords): + source, destination = pathlib.Path(source), pathlib.Path(destination) + destination.mkdir(parents=True) + shutil.copy2(source / ENTRY_DOCUMENT, destination / ENTRY_DOCUMENT) + raise OSError(28, "No space left on device", str(destination)) + + monkeypatch.setattr(shutil, "copytree", copytree) + + +def leave_staging_behind(monkeypatch: pytest.MonkeyPatch) -> None: + """Never cleans up, so a failed write leaves its half-written tree in the project.""" + monkeypatch.setattr(installer, "_remove", lambda path: None) + + +def write_without_staging(monkeypatch: pytest.MonkeyPatch) -> None: + """Writes straight into the destination, so a failure leaves a half-written skill.""" + + def materialize(source, destination, digest, context): + installer._remove(destination) + installer._stage(source, destination, context.mode) + installer._write_manifest(destination, digest, context) + + monkeypatch.setattr(installer, "_materialize", materialize) + + +def abort_on_read_failure(monkeypatch: pytest.MonkeyPatch) -> None: + """Lets a read error end the whole run instead of costing one destination.""" + real = installer._install_one + + def install_one(source, destination, digest, context): + try: + return real(source, destination, digest, context) + except installer._SkillRefusal as refusal: + if refusal.kind is SkillFailureKind.UNREADABLE_DESTINATION: + raise OSError(str(refusal)) from refusal + raise + + monkeypatch.setattr(installer, "_install_one", install_one) + + +def treat_unreadable_as_unchanged(monkeypatch: pytest.MonkeyPatch) -> None: + """Answers "has the user edited this?" with "no" when it cannot read the answer.""" + real = installer._is_unmodified + + def is_unmodified(destination, manifest): + try: + return real(destination, manifest) + except OSError: + return True + + monkeypatch.setattr(installer, "_is_unmodified", is_unmodified) + + +def report_an_empty_bundle_as_success(monkeypatch: pytest.MonkeyPatch) -> None: + """Returns no skills where a defective installation should be refused outright.""" + monkeypatch.setattr(installer, "iter_bundled_skills", lambda: iter(())) + + +def ignore_the_install_mode(monkeypatch: pytest.MonkeyPatch) -> None: + """Compares the version and the content, but not how the skill was installed. + + Copies and links carry the same recorded hash -- it describes the package's content, + which is what both of them serve -- so without the mode a project installed one way + is reported up to date when the other way is asked for. + """ + + def is_current(destination, source, manifest, digest, context): + return ( + manifest.get("gx_version") == context.version + and manifest.get("content_sha256") == digest + ) + + monkeypatch.setattr(installer, "_is_current", is_current) + + +def trust_links_without_looking(monkeypatch: pytest.MonkeyPatch) -> None: + """Assumes installed links still point into the package, wherever it has got to.""" + monkeypatch.setattr(installer, "_links_point_at", lambda destination, source: True) + + +def copy_where_links_were_asked_for(monkeypatch: pytest.MonkeyPatch) -> None: + """Ignores symlink mode and copies, so the install stops tracking the package.""" + + def stage(source, staging, mode): + shutil.copytree(source, staging, symlinks=True) + + monkeypatch.setattr(installer, "_stage", stage) + + +def refuse_to_make_links(monkeypatch: pytest.MonkeyPatch) -> None: + """A platform that permits symlinks only for privileged accounts.""" + + def symlink_to(self, target, target_is_directory=False): + raise OSError(1, "Operation not permitted", str(self)) + + monkeypatch.setattr(pathlib.Path, "symlink_to", symlink_to) + + +def hide_failures(monkeypatch: pytest.MonkeyPatch) -> None: + """Drops the refusals from the report the command exits on.""" + real = command_line.install_skills + + def install(*arguments, **keywords): + return dataclasses.replace(real(*arguments, **keywords), failed=()) + + monkeypatch.setattr(command_line, "install_skills", install) + + +def hide_the_outcome_group(monkeypatch: pytest.MonkeyPatch, group: str) -> None: + """Empties one whole outcome out of the report the command prints. + + Indistinguishable, from the printing code's side, from a build that stopped printing + that group: the destinations are installed correctly either way, the command still + exits 0, and the only thing lost is the user being told. + """ + real = command_line.install_skills + + def install(*arguments, **keywords): + return dataclasses.replace(real(*arguments, **keywords), **{group: ()}) + + monkeypatch.setattr(command_line, "install_skills", install) + + +def label_failures_by_appearance(monkeypatch: pytest.MonkeyPatch) -> None: + """Derives the failure kind from the destination afterwards instead of recording it. + + This is the heuristic the typed failure kind replaced: a destination that exists and + holds an ownership manifest is called edited. A destination whose subdirectory could + not be read satisfies both conditions, which is exactly why the kind cannot be + recovered after the fact. + """ + real = command_line.install_skills + + def install(*arguments, **keywords): + report = real(*arguments, **keywords) + return dataclasses.replace( + report, + failed=tuple( + dataclasses.replace(failure, kind=SkillFailureKind.LOCALLY_MODIFIED) + if failure.destination.exists() + and installer.read_skill_manifest(failure.destination) is not None + else failure + for failure in report.failed + ), + ) + + monkeypatch.setattr(command_line, "install_skills", install) + + +# --------------------------------------------------------------------------- +# A first run. +# --------------------------------------------------------------------------- + + +def test_a_first_run_installs_every_skill_into_every_target(project: pathlib.Path, bundle: Bundle): + assert not fresh_install_problems(project, bundle) + + +def test_a_first_run_without_ownership_manifests_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + stop_writing_manifests(monkeypatch) + + problems = fresh_install_problems(project, bundle) + + assert [problem for problem in problems if MANIFEST_NAME in problem] + + +def test_the_skills_this_package_bundles_install_as_exact_copies(project: pathlib.Path): + """The real bundle, found the way an installed package is: through the import system. + + The digest comparison inside is the claim the rest of the installer rests on. If a + bundled skill does not hash to the same value once installed -- which is what happens + the moment a copy stops preserving something the hash describes -- then no run after + the first can tell an untouched install from one the user edited. + """ + skills = list(installer.iter_bundled_skills()) + assert len(skills) >= MIN_BUNDLED_SKILLS, f"found {[skill.name for skill in skills]}" + assert {skill.parent for skill in skills} == {BUNDLED_SKILLS_ROOT}, ( + f"the package resolved its skills to {sorted({str(skill.parent) for skill in skills})}," + f" not to {BUNDLED_SKILLS_ROOT}" + ) + + report = install_skills(project, targets=ALL_TARGETS) + + expected = [project / target.value / skill.name for target in ALL_TARGETS for skill in skills] + assert not partition_problems(report, expected) + assert set(report.installed) == set(expected), describe_report(report) + problems = [ + problem + for destination in expected + for problem in installed_copy_problems( + BUNDLED_SKILLS_ROOT / destination.name, + destination, + great_expectations.__version__, + ) + ] + assert not problems, "\n".join(problems) + + +def test_a_copy_that_dereferenced_a_link_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """Dereferencing is the one copying defect a report cannot show: every file arrives. + + What arrives is a real file where the package holds a link, so the copy hashes + differently from the skill it was copied from and every later run calls it edited. + """ + (bundle.root / bundle.names[0] / REFERENCE_DIR / "shared.md").symlink_to( + pathlib.Path("guide.md") + ) + dereference_symlinks(monkeypatch) + + problems = fresh_install_problems(project, bundle) + + assert [problem for problem in problems if "hashes to" in problem] + assert [problem for problem in problems if "shared.md" in problem] + + +def test_a_copy_that_dereferenced_a_dangling_link_is_reported( + project: pathlib.Path, bundle_with_links: Bundle, monkeypatch: pytest.MonkeyPatch +): + """The same defect meeting a link to a file the package no longer ships: it cannot + copy at all, and the skill never reaches the project. + """ + dereference_symlinks(monkeypatch) + + problems = fresh_install_problems(project, bundle_with_links) + + assert [problem for problem in problems if "is not a directory" in problem] + + +# --------------------------------------------------------------------------- +# The report accounts for every destination exactly once. +# --------------------------------------------------------------------------- + + +def test_every_destination_lands_in_exactly_one_outcome(project: pathlib.Path, bundle: Bundle): + """One run producing all four outcomes at once, which is when a partition can slip.""" + install_skills(project, targets=ALL_TARGETS) + first, second = bundle.names + edited = project / SkillTarget.AGENTS.value / first + (edited / REFERENCE_DIR / "notes.md").write_text("notes the user added\n", encoding="utf-8") + removed = project / SkillTarget.CLAUDE.value / first + shutil.rmtree(removed) + stale = project / SkillTarget.CLAUDE.value / second + stamp_manifest_version(stale, EARLIER_VERSION) + untouched = project / SkillTarget.AGENTS.value / second + + report = install_skills(project, targets=ALL_TARGETS) + + expected = expected_destinations(project, bundle) + assert not non_vacuity_problems(expected) + assert not partition_problems(report, expected) + assert report.installed == (removed,), describe_report(report) + assert report.up_to_date == (untouched,), describe_report(report) + assert report.replaced == (stale,), describe_report(report) + assert [failure.destination for failure in report.failed] == [edited], describe_report(report) + problems = [ + problem + for destination in (removed, stale, untouched) + for problem in installed_copy_problems( + bundle.source_for(destination), destination, INSTALLED_VERSION + ) + ] + assert not problems, "\n".join(problems) + + +def test_a_destination_reported_twice_is_caught(tmp_path: pathlib.Path): + destination = tmp_path / SkillTarget.AGENTS.value / "gx-first-skill" + report = SkillInstallReport( + installed=(destination,), up_to_date=(destination,), replaced=(), failed=() + ) + + problems = partition_problems(report, [destination]) + + assert [problem for problem in problems if "installed" in problem and "up_to_date" in problem] + + +def test_a_destination_reported_nowhere_is_caught(tmp_path: pathlib.Path): + destination = tmp_path / SkillTarget.AGENTS.value / "gx-first-skill" + report = SkillInstallReport(installed=(), up_to_date=(), replaced=(), failed=()) + + problems = partition_problems(report, [destination]) + + assert [problem for problem in problems if "no outcome" in problem] + + +# --------------------------------------------------------------------------- +# Running it again. +# --------------------------------------------------------------------------- + + +def test_a_second_run_leaves_every_installed_skill_untouched(project: pathlib.Path, bundle: Bundle): + assert not idempotency_problems(project, bundle, InstallMode.COPY) + + +def test_a_second_run_that_rewrote_the_same_content_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """A rewrite with identical bytes is invisible in the content and in the report.""" + ignore_the_version_stamp(monkeypatch) + + problems = idempotency_problems(project, bundle, InstallMode.COPY) + + assert [problem for problem in problems if "leave every destination alone" in problem] + assert [problem for problem in problems if "written again with the same content" in problem] + + +def test_a_skill_holding_links_installs_and_stays_up_to_date( + project: pathlib.Path, bundle_with_links: Bundle +): + """Ordinary and dangling links alike: copied as links, and a no-op on the next run.""" + assert not fresh_install_problems(project, bundle_with_links) + assert not idempotency_problems(project, bundle_with_links, InstallMode.COPY) + + +# --------------------------------------------------------------------------- +# Running it after an upgrade. +# --------------------------------------------------------------------------- + + +def test_a_new_version_alone_replaces_the_installed_skills( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """Two releases can ship byte-identical skills, and the install still has to say so: + what the destination records is which version put it there. + """ + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + state = install_and_record(project, bundle) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + + report = install_skills(project, targets=ALL_TARGETS) + + assert not replacement_problems(state, report, content_changed=False) + + +def test_changed_content_alone_replaces_the_installed_skills(project: pathlib.Path, bundle: Bundle): + """The everyday case for anyone working on the skills themselves: an editable + install or a source checkout ships changed content under an unchanged version, so + the version stamp cannot be what decides whether the project is out of date. + """ + state = install_and_record(project, bundle) + rewrite_bundled_skills(bundle) + + report = install_skills(project, targets=ALL_TARGETS) + + assert not replacement_problems(state, report, content_changed=True) + + +def test_content_that_never_reached_the_project_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """The check must fail against a build that compares everything but the content.""" + state = install_and_record(project, bundle) + rewrite_bundled_skills(bundle) + stop_comparing_the_recorded_hash(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS) + + problems = replacement_problems(state, report, content_changed=True) + assert [problem for problem in problems if "must be replaced" in problem] + assert [problem for problem in problems if "added.md is missing" in problem] + assert [problem for problem in problems if "never reached the project" in problem] + + +def test_an_upgrade_that_moves_both_replaces_the_installed_skills( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """A real release moves both at once, which must behave as either one alone does.""" + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + state = install_and_record(project, bundle) + rewrite_bundled_skills(bundle) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + + report = install_skills(project, targets=ALL_TARGETS) + + assert not replacement_problems(state, report, content_changed=True) + + +def test_an_upgrade_left_unapplied_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + state = install_and_record(project, bundle) + rewrite_bundled_skills(bundle) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + treat_everything_as_current(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS) + + problems = replacement_problems(state, report, content_changed=True) + assert [problem for problem in problems if "added.md is missing" in problem] + + +# --------------------------------------------------------------------------- +# Destinations the run must not overwrite. +# --------------------------------------------------------------------------- + + +def test_a_skill_edited_after_it_was_installed_is_refused(project: pathlib.Path, bundle: Bundle): + """The package has not moved on, so nothing but the edit can explain the refusal.""" + state = install_and_edit(project, bundle) + + report = install_skills(project, targets=ALL_TARGETS) + + assert not edited_skill_problems(state, report, force=False, stale=False) + + +def test_an_edited_skill_survives_an_upgrade( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """The dangerous case: the version moved on, so replacing would look justified.""" + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + state = install_and_edit(project, bundle) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + + report = install_skills(project, targets=ALL_TARGETS) + + assert not edited_skill_problems(state, report, force=False, stale=True) + + +def test_an_overwritten_local_edit_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + state = install_and_edit(project, bundle) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + ignore_local_edits(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS) + + problems = edited_skill_problems(state, report, force=False, stale=True) + assert [problem for problem in problems if "LOCALLY_MODIFIED" in problem.upper()] + assert [problem for problem in problems if "notes.md was removed" in problem] + + +def test_force_repairs_an_edited_skill_at_the_same_version(project: pathlib.Path, bundle: Bundle): + """``--force`` is a repair as much as an upgrade path. + + The edited copy is the version this package would install anyway, so nothing about it + is stale: the only thing to fix is the edit. A run that decided what to write by + comparing versions alone would find nothing to do here and say so, leaving the edit + in place under a command that reported success. + """ + state = install_and_edit(project, bundle) + + report = install_skills(project, targets=ALL_TARGETS, force=True) + + assert not edited_skill_problems(state, report, force=True, stale=False) + + +def test_a_forced_repair_that_wrote_nothing_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """The check must fail against a build that decides staleness before ownership.""" + state = install_and_edit(project, bundle) + ignore_edits_when_deciding_staleness(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS, force=True) + + problems = edited_skill_problems(state, report, force=True, stale=False) + assert [problem for problem in problems if "--force must replace" in problem] + assert [problem for problem in problems if "still holds the edit" in problem] + + +def test_force_replaces_an_edited_skill_across_an_upgrade( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + monkeypatch.setattr(great_expectations, "__version__", EARLIER_VERSION) + state = install_and_edit(project, bundle) + monkeypatch.setattr(great_expectations, "__version__", INSTALLED_VERSION) + + report = install_skills(project, targets=ALL_TARGETS, force=True) + + assert not edited_skill_problems(state, report, force=True, stale=True) + + +def test_a_forced_replacement_that_wrote_nothing_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """The report says replaced either way; only the destination shows which is true.""" + state = install_and_edit(project, bundle) + install_nothing(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS, force=True) + + problems = edited_skill_problems(state, report, force=True, stale=False) + assert [problem for problem in problems if "still holds the edit" in problem] + + +@pytest.mark.parametrize("manifest", UNPROVEN_OWNERSHIP) +@pytest.mark.parametrize("force", [False, True], ids=["without_force", "with_force"]) +def test_a_directory_great_expectations_did_not_install_is_never_overwritten( + project: pathlib.Path, bundle: Bundle, force: bool, manifest: str | bytes | None +): + assert not foreign_destination_problems(project, bundle, force=force, manifest=manifest) + + +def test_an_overwritten_foreign_directory_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + claim_every_directory(monkeypatch) + + problems = foreign_destination_problems(project, bundle, force=True) + + assert [problem for problem in problems if "notes.md was removed" in problem] + + +def test_a_manifest_that_ends_the_run_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """The check must fail against a build where one unreadable manifest ends everything. + + The ownership manifest is read before the point at which a destination's problems + start being caught, so a way of failing that raises instead of answering does not + cost one destination: it costs the run, and every skill that would have been + installed after it, under a traceback rather than a report. + """ + stop_swallowing_undecodable_manifests(monkeypatch) + + problems = foreign_destination_problems( + project, bundle, force=True, manifest=UNDECODABLE_MANIFEST + ) + + assert [problem for problem in problems if "cost that destination and no more" in problem] + + +def test_a_directory_managed_by_another_tool_being_adopted_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """A manifest that parses is not a manifest that belongs to this package. + + The check must fail against a build that accepts any well-formed manifest, because + such a directory is indistinguishable from one of ours by everything except the owner + it records -- and adopting it overwrites whatever the other tool put there. + """ + stop_checking_the_owner(monkeypatch) + + problems = foreign_destination_problems( + project, bundle, force=True, manifest=ANOTHER_OWNERS_MANIFEST + ) + + assert [problem for problem in problems if "FOREIGN_DESTINATION" in problem.upper()] + assert [problem for problem in problems if "notes.md was removed" in problem] + + +# --------------------------------------------------------------------------- +# Writes that fail. +# --------------------------------------------------------------------------- + + +def test_a_failed_write_leaves_nothing_at_the_destination( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + break_writing(monkeypatch) + + assert not write_failure_problems(project, bundle) + + +def test_a_half_written_tree_left_in_the_project_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + break_writing(monkeypatch) + leave_staging_behind(monkeypatch) + + problems = write_failure_problems(project, bundle) + + assert [problem for problem in problems if STAGING_PREFIX in problem] + + +def test_a_failed_replacement_leaves_the_installed_skill_intact( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + assert not failed_replacement_problems(project, bundle, monkeypatch) + + +def test_a_replacement_written_without_staging_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """Writing in place is what makes a failure visible to an agent as a truncated skill.""" + write_without_staging(monkeypatch) + + problems = failed_replacement_problems(project, bundle, monkeypatch) + + assert [problem for problem in problems if "was removed" in problem] + + +def test_a_failed_swap_puts_the_previous_skill_back( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + assert not swap_failure_problems(project, bundle, monkeypatch) + + +def test_a_failed_swap_that_lost_the_previous_skill_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """Without the restore the destination simply stops existing, which is the whole + reason the previous tree is moved aside rather than deleted. + """ + problems = swap_failure_problems(project, bundle, monkeypatch, restore=False) + + assert [problem for problem in problems if "no longer a directory" in problem] + + +def test_a_remnant_of_an_interrupted_run_is_swept(project: pathlib.Path, bundle: Bundle): + assert not remnant_sweep_problems(project, bundle) + + +def test_a_remnant_left_in_the_project_for_ever_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + leave_staging_behind(monkeypatch) + + problems = remnant_sweep_problems(project, bundle) + + assert [problem for problem in problems if STAGING_PREFIX in problem] + + +# --------------------------------------------------------------------------- +# Destinations that cannot be read. +# --------------------------------------------------------------------------- + + +def test_an_unreadable_destination_costs_one_destination_not_the_run( + project: pathlib.Path, bundle: Bundle +): + assert not unreadable_destination_problems(project, bundle) + + +def test_a_read_failure_that_ended_the_run_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + abort_on_read_failure(monkeypatch) + + problems = unreadable_destination_problems(project, bundle) + + assert [problem for problem in problems if "cost one destination, not the run" in problem] + + +def test_a_read_failure_reported_as_up_to_date_is_caught( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """Answering an unanswerable question is worse than failing: nothing tells the user.""" + treat_unreadable_as_unchanged(monkeypatch) + + problems = unreadable_destination_problems(project, bundle) + + assert [problem for problem in problems if "UNREADABLE_DESTINATION" in problem.upper()] + + +# --------------------------------------------------------------------------- +# A package with nothing to install. +# --------------------------------------------------------------------------- + + +def test_a_package_without_a_bundle_directory_is_refused( + project: pathlib.Path, tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +): + location = tmp_path / "site-packages" / "great_expectations" + location.mkdir(parents=True) + assert not (location / ".agents").exists() + monkeypatch.setattr( + importlib.util, + "find_spec", + lambda name: types.SimpleNamespace(submodule_search_locations=[str(location)]), + ) + + assert not empty_bundle_problems(project) + + +def test_a_package_whose_bundle_holds_no_skills_is_refused( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + """A partly packaged installation: the reference files shipped, the entry documents + did not. Every directory is still there, so only the entry document tells them apart. + """ + for skill in bundle.skills: + (skill / ENTRY_DOCUMENT).unlink() + assert list(bundle.root.iterdir()), f"{bundle.root} must still hold directories" + + assert not empty_bundle_problems(project) + + +def test_an_empty_bundle_reported_as_a_clean_run_is_caught( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + report_an_empty_bundle_as_success(monkeypatch) + + problems = empty_bundle_problems(project) + + assert [problem for problem in problems if "nothing went wrong" in problem] + + +def test_a_project_root_that_is_not_a_directory_is_refused(tmp_path: pathlib.Path, bundle: Bundle): + missing = tmp_path / "no-such-project" + with pytest.raises(NotADirectoryError, match="not an existing directory"): + install_skills(missing) + assert not missing.exists() + + a_file = tmp_path / "a-file" + a_file.write_text("the user pointed at a file\n", encoding="utf-8") + with pytest.raises(NotADirectoryError, match="not an existing directory"): + install_skills(a_file) + assert a_file.read_text(encoding="utf-8") == "the user pointed at a file\n" + + +# --------------------------------------------------------------------------- +# Walking a tree that points at itself. +# --------------------------------------------------------------------------- + + +@pytest.fixture +def bundle_with_cycles(bundle: Bundle) -> Bundle: + """A skill linking to its own directory, to its parent, and to itself.""" + skill = bundle.root / bundle.names[0] + (skill / "itself").symlink_to(skill, target_is_directory=True) + (skill / REFERENCE_DIR / "upwards").symlink_to(skill, target_is_directory=True) + (skill / REFERENCE_DIR / "here").symlink_to(skill / REFERENCE_DIR, target_is_directory=True) + return bundle + + +def walk_following_links(root: pathlib.Path) -> Iterator[str]: + """The same walk, following symlinked directories: the defect, kept as evidence.""" + pending = [root] + while pending: + for entry in sorted(pending.pop().iterdir()): + yield entry.relative_to(root).as_posix() + if entry.is_dir(): + pending.append(entry) + + +def deepest(paths: Sequence[str]) -> int: + return max(len(pathlib.PurePosixPath(path).parts) for path in paths) + + +def test_the_walk_behind_the_digest_terminates_on_symlink_cycles(bundle_with_cycles: Bundle): + # Capped rather than drained: a walk that started following links would otherwise be + # detected by this test hanging, and a hang is a much worse signal than a failure. + skill = bundle_with_cycles.root / bundle_with_cycles.names[0] + + walked = sorted( + relpath for relpath, _ in itertools.islice(installer._walk(skill), CYCLE_WALK_LIMIT) + ) + + assert len(walked) < CYCLE_WALK_LIMIT, "the walk was still going when it was cut off" + assert {"itself", f"{REFERENCE_DIR}/upwards", f"{REFERENCE_DIR}/here"} <= set(walked) + assert len(walked) == len(set(walked)), f"the walk visited a path twice: {walked}" + assert deepest(walked) <= REAL_TREE_DEPTH, walked + + +def test_a_walk_that_followed_links_would_not_terminate(bundle_with_cycles: Bundle): + """Why the walk above is written out rather than delegated to a recursive glob.""" + skill = bundle_with_cycles.root / bundle_with_cycles.names[0] + + walked: list[str] = [] + with contextlib.suppress(OSError): # the kernel gives up on the link chain eventually + for relpath in walk_following_links(skill): + walked.append(relpath) + if len(walked) >= CYCLE_WALK_LIMIT or deepest(walked) > CYCLE_DEPTH_EVIDENCE: + break + + assert deepest(walked) > CYCLE_DEPTH_EVIDENCE, ( + f"the following walk stopped at depth {deepest(walked)}, so it proves nothing" + ) + + +def test_a_skill_with_symlink_cycles_installs_and_stays_up_to_date( + project: pathlib.Path, bundle_with_cycles: Bundle +): + assert not fresh_install_problems(project, bundle_with_cycles) + assert not idempotency_problems(project, bundle_with_cycles, InstallMode.COPY) + + +# --------------------------------------------------------------------------- +# Symlink mode. +# --------------------------------------------------------------------------- + + +def test_symlink_mode_creates_a_directory_of_working_links(project: pathlib.Path, bundle: Bundle): + assert not symlink_mode_problems(project, bundle) + assert not idempotency_problems(project, bundle, InstallMode.SYMLINK) + + +def test_copies_where_links_were_asked_for_are_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + copy_where_links_were_asked_for(monkeypatch) + + problems = symlink_mode_problems(project, bundle) + + assert [problem for problem in problems if "rather than a link" in problem] + + +@pytest.mark.parametrize( + ["installed_as", "asked_for"], + [ + pytest.param(InstallMode.COPY, InstallMode.SYMLINK, id="copies_to_links"), + pytest.param(InstallMode.SYMLINK, InstallMode.COPY, id="links_to_copies"), + ], +) +def test_asking_for_the_other_mode_converts_the_installed_skills( + project: pathlib.Path, bundle: Bundle, installed_as: InstallMode, asked_for: InstallMode +): + # Keep both directions. Only ``links_to_copies`` rests on the recorded mode: going + # the other way, the freshness check calls ``readlink()`` on a real file, gets an + # OSError and reports the destination stale anyway, so that direction would still + # pass if the mode were never compared at all. + assert not mode_switch_problems(project, bundle, installed_as, asked_for) + + +def test_an_install_left_in_the_mode_it_was_not_asked_for_is_reported( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + ignore_the_install_mode(monkeypatch) + + problems = mode_switch_problems(project, bundle, InstallMode.COPY, InstallMode.SYMLINK) + + assert [problem for problem in problems if "must replace every destination" in problem] + assert [problem for problem in problems if "rather than a link" in problem] + + +def test_a_symlink_install_is_refreshed_when_the_package_moves( + project: pathlib.Path, + bundle: Bundle, + tmp_path: pathlib.Path, + monkeypatch: pytest.MonkeyPatch, +): + """Same version, same content, same mode -- and every link points at nothing. + + Moving the environment is the one way an installed symlink goes wrong that no hash + can see: the content it describes has not changed, only the path it lives at, so the + links themselves are the only thing left to compare. + """ + install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + moved = tmp_path / "relocated-environment" / ".agents" / "skills" + moved.parent.mkdir(parents=True) + shutil.move(str(bundle.root), str(moved)) + relocated = Bundle(root=moved, names=bundle.names) + monkeypatch.setattr(installer, "_bundled_skills_root", lambda: moved) + expected = expected_destinations(project, relocated) + assert not [ + destination for destination in expected if (destination / ENTRY_DOCUMENT).exists() + ], "the installed links must be dangling before the re-run, or nothing is being fixed" + + report = install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + + assert not partition_problems(report, expected) + assert set(report.replaced) == set(expected), describe_report(report) + problems = [ + problem + for destination in expected + for problem in linked_install_problems( + relocated.source_for(destination), destination, INSTALLED_VERSION + ) + ] + assert not problems, "\n".join(problems) + + +def test_links_left_pointing_at_a_package_that_moved_are_reported( + project: pathlib.Path, + bundle: Bundle, + tmp_path: pathlib.Path, + monkeypatch: pytest.MonkeyPatch, +): + install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + moved = tmp_path / "relocated-environment" / ".agents" / "skills" + moved.parent.mkdir(parents=True) + shutil.move(str(bundle.root), str(moved)) + relocated = Bundle(root=moved, names=bundle.names) + monkeypatch.setattr(installer, "_bundled_skills_root", lambda: moved) + trust_links_without_looking(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + + expected = expected_destinations(project, relocated) + problems = [ + problem + for destination in expected + for problem in linked_install_problems( + relocated.source_for(destination), destination, INSTALLED_VERSION + ) + ] + assert set(report.up_to_date) == set(expected), describe_report(report) + assert [problem for problem in problems if "links to" in problem] + + +def replace_a_link_with_a_file(destination: pathlib.Path) -> None: + """What an editor that "saves through" a symlink leaves behind.""" + entry = destination / ENTRY_DOCUMENT + content = entry.read_bytes() + entry.unlink() + entry.write_bytes(content) + + +def remove_every_link(destination: pathlib.Path) -> None: + """What a user clearing out a skill by hand leaves behind: the manifest, alone.""" + for entry in destination.iterdir(): + if entry.name != MANIFEST_NAME: + entry.unlink() + + +DAMAGED_LINKS: Final = [ + pytest.param(replace_a_link_with_a_file, id="link_replaced_by_a_file"), + pytest.param(remove_every_link, id="links_removed"), +] + + +def damaged_links_problems( + project: pathlib.Path, + bundle: Bundle, + monkeypatch: pytest.MonkeyPatch, + damage: Callable[[pathlib.Path], None], + defect: Callable[[pytest.MonkeyPatch], None] | None = None, +) -> list[str]: + """A symlink install the user has interfered with is refused, not quietly rebuilt. + + Symlink installs are judged structurally rather than by content -- the content lives + in the package and changes legitimately on every upgrade -- so the link set is the + whole of the record. ``damage`` is a parameter rather than a fixed step because that + record has more than one way to stop being true, and a single one of them stands in + for the others only until someone changes the code. + """ + install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + destination = project / SkillTarget.AGENTS.value / bundle.names[0] + intact = snapshot(destination) + damage(destination) + before = snapshot(destination) + problems = [] + if before == intact: + problems.append(f"{destination} was not changed, so refusing it proves nothing.") + if not (destination / MANIFEST_NAME).is_file(): + problems.append( + f"{destination} lost its {MANIFEST_NAME}, so it would be refused as a" + " directory this package never installed rather than as an edited one." + ) + if defect is not None: + defect(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + + problems += refusal_problems(report, destination, SkillFailureKind.LOCALLY_MODIFIED) + problems += unchanged_problems(destination, before) + return problems + + +@pytest.mark.parametrize("damage", DAMAGED_LINKS) +def test_a_damaged_symlink_install_counts_as_a_local_edit( + project: pathlib.Path, + bundle: Bundle, + monkeypatch: pytest.MonkeyPatch, + damage: Callable[[pathlib.Path], None], +): + assert not damaged_links_problems(project, bundle, monkeypatch, damage) + + +@pytest.mark.parametrize("damage", DAMAGED_LINKS) +def test_a_damaged_symlink_install_treated_as_intact_is_reported( + project: pathlib.Path, + bundle: Bundle, + monkeypatch: pytest.MonkeyPatch, + damage: Callable[[pathlib.Path], None], +): + problems = damaged_links_problems( + project, bundle, monkeypatch, damage, defect=ignore_local_edits + ) + + assert [problem for problem in problems if "LOCALLY_MODIFIED" in problem.upper()] + + +def test_a_platform_that_refuses_links_is_reported_per_skill( + project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch +): + refuse_to_make_links(monkeypatch) + + report = install_skills(project, targets=ALL_TARGETS, mode=InstallMode.SYMLINK) + + expected = expected_destinations(project, bundle) + assert not non_vacuity_problems(expected) + assert not partition_problems(report, expected) + problems = [ + problem + for destination in expected + for problem in refusal_problems(report, destination, SkillFailureKind.SYMLINKS_UNSUPPORTED) + ] + assert not problems, "\n".join(problems) + assert [destination for destination in expected if destination.exists()] == [] + assert not staging_remnant_problems(project) + + +# --------------------------------------------------------------------------- +# The command around the installer. +# --------------------------------------------------------------------------- + + +def unwrapped(text: str) -> str: + """Collapse the command's line wrapping so a sentence can be looked for whole.""" + return " ".join(text.split()) + + +def displayed(destination: pathlib.Path, project: pathlib.Path) -> str: + return str(destination.relative_to(project)) + + +def test_the_command_installs_and_exits_zero( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str] +): + status = command_line.main(["skills", "install", "--project-root", str(project)]) + + output = capsys.readouterr().out + assert status == 0 + expected = expected_destinations(project, bundle) + assert not non_vacuity_problems(expected) + problems = [ + problem + for destination in expected + for problem in installed_copy_problems( + bundle.source_for(destination), destination, INSTALLED_VERSION + ) + ] + assert not problems, "\n".join(problems) + for destination in expected: + assert displayed(destination, project) in output + + +def refused_destination_problems( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str] +) -> list[str]: + """One destination refused, the rest installed: the run a script must not read as clean.""" + foreign = project / SkillTarget.CLAUDE.value / bundle.names[0] + foreign.mkdir(parents=True) + (foreign / "notes.md").write_text("a directory the user made\n", encoding="utf-8") + before = snapshot(foreign) + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + + status = command_line.main(["skills", "install", "--project-root", str(project)]) + + output = capsys.readouterr().out + if status == 0: + problems.append( + "a run that refused a destination must exit nonzero, or a script goes on to" + " run an agent that is missing a skill" + ) + problems += unchanged_problems(foreign, before) + if displayed(foreign, project) not in output: + problems.append(f"{foreign} is missing from the report the command printed.") + for destination in expected: + if destination == foreign: + continue + problems += installed_copy_problems( + bundle.source_for(destination), destination, INSTALLED_VERSION + ) + if displayed(destination, project) not in output: + problems.append(f"{destination} is missing from the report the command printed.") + return problems + + +def test_the_command_exits_nonzero_when_any_destination_was_refused( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str] +): + assert not refused_destination_problems(project, bundle, capsys) + + +def test_a_command_that_hid_a_refusal_is_caught( + project: pathlib.Path, + bundle: Bundle, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +): + hide_failures(monkeypatch) + + problems = refused_destination_problems(project, bundle, capsys) + + assert [problem for problem in problems if "exit nonzero" in problem] + + +def reported_destination_problems( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str], outcome: str +) -> list[str]: + """A run must name every destination it considered, whatever became of it. + + ``outcome`` is what this run does to the destinations, and it is the caller's to set: + the two runs that change nothing visible at the destination -- leaving a skill alone + and bringing it up to a new version -- are the two whose output is the only evidence + the user gets. A first install is not a substitute for either, because it is the one + case where the destination appearing on disk says what happened by itself. + """ + version = EARLIER_VERSION if outcome == "updated" else INSTALLED_VERSION + with expected_package_version(version): + command_line.main(["skills", "install", "--project-root", str(project)]) + capsys.readouterr() + expected = expected_destinations(project, bundle) + problems = non_vacuity_problems(expected) + # Anchored on the filesystem rather than on the previous run's output: the manifests + # decide what the second run must do, so this is what makes the run under test the + # run this is named for. + for destination in expected: + problems += manifest_problems( + destination, bundle.source_for(destination), version, InstallMode.COPY + ) + + status = command_line.main(["skills", "install", "--project-root", str(project)]) + + output = capsys.readouterr().out + if status != 0: + problems.append(f"a run that {outcome} every skill must exit 0; got {status}") + for destination in expected: + if displayed(destination, project) not in output: + problems.append( + f"{destination} was {outcome} and the command's report does not name it," + f" so nothing tells the user it happened. Printed:\n{output}" + ) + return problems + + +@contextlib.contextmanager +def expected_package_version(version: str) -> Iterator[None]: + """Run a block with the package claiming ``version``, then put it back.""" + with pytest.MonkeyPatch.context() as patch: + patch.setattr(great_expectations, "__version__", version) + yield + + +@pytest.mark.parametrize( + "outcome", + ["left alone", "updated"], + ids=["already_up_to_date", "updated_by_an_upgrade"], +) +def test_the_command_names_every_destination_it_considered( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str], outcome: str +): + assert not reported_destination_problems(project, bundle, capsys, outcome) + + +@pytest.mark.parametrize( + ["outcome", "group"], + [ + pytest.param("left alone", "up_to_date", id="already_up_to_date"), + pytest.param("updated", "replaced", id="updated_by_an_upgrade"), + ], +) +def test_an_outcome_group_missing_from_the_report_is_caught( + project: pathlib.Path, + bundle: Bundle, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, + outcome: str, + group: str, +): + """A group that stopped being printed is invisible: the command still exits 0 and + the destinations are still correct on disk, and the user is simply not told. + """ + hide_the_outcome_group(monkeypatch, group) + + problems = reported_destination_problems(project, bundle, capsys, outcome) + + assert [problem for problem in problems if "does not name it" in problem] + + +@contextlib.contextmanager +def an_edited_skill(project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch): + install_skills(project, targets=ALL_TARGETS) + edited = project / SkillTarget.AGENTS.value / bundle.names[0] + (edited / REFERENCE_DIR / "notes.md").write_text("notes the user added\n", encoding="utf-8") + yield + + +@contextlib.contextmanager +def a_foreign_directory(project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch): + foreign = project / SkillTarget.AGENTS.value / bundle.names[0] + foreign.mkdir(parents=True) + (foreign / "notes.md").write_text("a directory the user made\n", encoding="utf-8") + yield + + +@contextlib.contextmanager +def an_unreadable_skill(project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch): + install_skills(project, targets=ALL_TARGETS) + unreadable = project / SkillTarget.AGENTS.value / bundle.names[0] / REFERENCE_DIR + with made_unreadable(unreadable): + if is_readable(unreadable): + pytest.fail(f"{unreadable} is readable with no permissions at all; check the user") + yield + + +@contextlib.contextmanager +def a_write_that_fails(project: pathlib.Path, bundle: Bundle, monkeypatch: pytest.MonkeyPatch): + break_writing(monkeypatch) + yield + + +@pytest.mark.parametrize( + ["prepare", "kind", "explains_edits"], + [ + pytest.param(an_edited_skill, SkillFailureKind.LOCALLY_MODIFIED, True, id="edited"), + pytest.param( + a_foreign_directory, SkillFailureKind.FOREIGN_DESTINATION, False, id="foreign" + ), + pytest.param( + an_unreadable_skill, SkillFailureKind.UNREADABLE_DESTINATION, False, id="unreadable" + ), + pytest.param(a_write_that_fails, SkillFailureKind.WRITE_FAILED, False, id="write_failed"), + ], +) +def test_the_command_explains_local_edits_only_where_a_skill_was_edited( + project: pathlib.Path, + bundle: Bundle, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, + prepare: Callable[..., contextlib.AbstractContextManager[None]], + kind: SkillFailureKind, + explains_edits: bool, +): + """Advice meant for one kind of failure sends the user hunting at the others. + + The explanation of what counts as an edit is keyed on the recorded failure kind, and + it has to be: an edited destination and one whose subdirectory could not be read both + still exist and both still hold a valid ownership manifest. + """ + with prepare(project, bundle, monkeypatch): + report = install_skills(project, targets=ALL_TARGETS) + assert report.failed, "this scenario must produce a failure to explain" + assert {failure.kind for failure in report.failed} == {kind}, describe_report(report) + capsys.readouterr() + + status = command_line.main(["skills", "install", "--project-root", str(project)]) + + output = unwrapped(capsys.readouterr().out) + assert status == 1 + assert unwrapped(command_line._FAILURE_FOOTER) in output + assert unwrapped(command_line._LOCAL_EDIT_FOOTER) + assert (unwrapped(command_line._LOCAL_EDIT_FOOTER) in output) is explains_edits + + +def test_a_failure_kind_derived_after_the_fact_is_caught( + project: pathlib.Path, + bundle: Bundle, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +): + """The disproved heuristic, kept executable: it calls a permission error an edit.""" + install_skills(project, targets=ALL_TARGETS) + label_failures_by_appearance(monkeypatch) + unreadable = project / SkillTarget.AGENTS.value / bundle.names[0] / REFERENCE_DIR + + with made_unreadable(unreadable): + assert not is_readable(unreadable) + capsys.readouterr() + status = command_line.main(["skills", "install", "--project-root", str(project)]) + + assert status == 1 + assert unwrapped(command_line._LOCAL_EDIT_FOOTER) in unwrapped(capsys.readouterr().out) + + +def raise_no_working_directory() -> pathlib.Path: + raise FileNotFoundError(2, "No such file or directory") + + +@pytest.mark.parametrize( + "arguments", + [[], ["skills"], ["skills", "install"], ["skills", "list"]], + ids=["root", "skills", "install", "list"], +) +def test_help_never_reads_the_working_directory( + arguments: list[str], capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch +): + """Help has to work from a directory that no longer exists, which is where a user + who has just deleted a build directory reaches for it. + """ + monkeypatch.setattr(pathlib.Path, "cwd", staticmethod(raise_no_working_directory)) + + with pytest.raises(SystemExit) as exit_status: + command_line.main([*arguments, "--help"]) + + assert exit_status.value.code == 0 + assert "usage:" in capsys.readouterr().out + + +def test_a_working_directory_resolved_while_defining_the_arguments_is_caught( + monkeypatch: pytest.MonkeyPatch, +): + """Why the default is resolved when it is used and not when it is declared.""" + monkeypatch.setattr(pathlib.Path, "cwd", staticmethod(raise_no_working_directory)) + parser = argparse.ArgumentParser() + + with pytest.raises(FileNotFoundError): + parser.add_argument("--project-root", type=pathlib.Path, default=pathlib.Path.cwd()) + + +def test_a_deleted_working_directory_is_reported_rather_than_raised( + capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch +): + monkeypatch.setattr(pathlib.Path, "cwd", staticmethod(raise_no_working_directory)) + + status = command_line.main(["skills", "install"]) + + assert status == 1 + error = unwrapped(capsys.readouterr().err) + assert "current directory" in error + assert "--project-root" in error + + +def test_the_listing_reports_each_skill_against_the_given_project( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str] +): + """A listing that could only ever describe the working directory would contradict an + install aimed somewhere else. + """ + assert command_line.main(["skills", "install", "--project-root", str(project)]) == 0 + capsys.readouterr() + + assert command_line.main(["skills", "list", "--project-root", str(project)]) == 0 + + output = capsys.readouterr().out + for name in bundle.names: + assert name in output + for target in ALL_TARGETS: + assert target.value in output + assert INSTALLED_VERSION in output + + +def test_the_listing_shows_skills_installed_by_another_version( + project: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str] +): + install_skills(project, targets=ALL_TARGETS) + for destination in expected_destinations(project, bundle): + stamp_manifest_version(destination, EARLIER_VERSION) + capsys.readouterr() + + assert command_line.main(["skills", "list", "--project-root", str(project)]) == 0 + + output = capsys.readouterr().out + assert EARLIER_VERSION in output + assert INSTALLED_VERSION in output + + +def test_the_listing_refuses_a_project_that_is_not_a_directory( + tmp_path: pathlib.Path, bundle: Bundle, capsys: pytest.CaptureFixture[str] +): + status = command_line.main(["skills", "list", "--project-root", str(tmp_path / "nowhere")]) + + assert status == 1 + assert "not an existing directory" in unwrapped(capsys.readouterr().err) diff --git a/tests/agent_skills/test_skill_content.py b/tests/agent_skills/test_skill_content.py new file mode 100644 index 000000000000..e2099d3b3582 --- /dev/null +++ b/tests/agent_skills/test_skill_content.py @@ -0,0 +1,543 @@ +"""Conformance tests for the agent skills bundled in the ``great_expectations`` package. + +The skills are plain markdown, so nothing about them is checked by the interpreter. +Two separate contracts rest on that markdown and both fail silently when broken: + +1. **Discoverability.** Coding agents load a skill by reading the YAML frontmatter of + its entry document. A skill whose frontmatter does not parse, or whose ``name`` + disagrees with its directory, is simply never offered to the user -- there is no + error anywhere. +2. **Self-containment.** Each skill directory must stand on its own, and the shared + session references are committed once per skill directory rather than shared + through a symlink or a build step. Nothing but a test stops the copies from + drifting apart, and drift means one skill quietly teaches an older procedure. + +Every check below is paired with a test that introduces the corresponding violation +into a throwaway copy of the real content and asserts the check reports it. Without +that pairing a conformance check can degrade into a no-op -- for example by looking +for markdown links in content that spells its references as inline code -- and keep +passing forever while asserting nothing. +""" + +from __future__ import annotations + +import pathlib +import re +import shutil +from typing import Final + +import pytest +from ruamel.yaml import YAML +from ruamel.yaml.error import YAMLError + +pytestmark = [pytest.mark.unit] + +PROJECT_ROOT: Final = pathlib.Path(__file__).parents[2] +SKILLS_ROOT: Final = PROJECT_ROOT / "great_expectations" / ".agents" / "skills" + +ENTRY_DOCUMENT: Final = "SKILL.md" +REFERENCE_DIR: Final = "references" + +#: The data-source skill holds the authoritative copy of every shared reference. +CANONICAL_SKILL: Final = "gx-configure-data-source" +SHARED_REFERENCES: Final = ("preflight.md", "write-out.md", "robustness.md") + +#: Limits imposed by the agent skills format that the bundled content targets. +MAX_NAME_LENGTH: Final = 64 +MAX_DESCRIPTION_LENGTH: Final = 1024 +#: A reference resolves at most one directory below the skill root, so a path +#: relative to the skill root has at most two parts (``references/.md``). +MAX_REFERENCE_PARTS: Final = 2 + +#: The entry document is loaded into the agent's context in full, so detail belongs in +#: references that are read on demand instead. +MAX_ENTRY_DOCUMENT_LINES: Final = 500 + +#: Lowercase letters, digits and single interior hyphens. +SKILL_NAME_PATTERN: Final = re.compile(r"[a-z0-9]+(?:-[a-z0-9]+)*") + +#: Number of skills the package is known to bundle. Guards against a discovery bug +#: silently reducing every parametrized test below to zero cases. +MIN_BUNDLED_SKILLS: Final = 2 + +FRONTMATTER_PATTERN: Final = re.compile(r"\A---\n(?P.*?)\n---\n", re.DOTALL) +CODE_SPAN_PATTERN: Final = re.compile(r"`([^`\n]+)`") +MARKDOWN_LINK_PATTERN: Final = re.compile(r"\[[^\]]*\]\(([^)\s]+)\)") + + +class SkillContentError(Exception): + """Raised when a skill's entry document cannot be read as a skill at all.""" + + +def discover_skills(skills_root: pathlib.Path) -> list[pathlib.Path]: + """Return every bundled skill directory, identified by its entry document. + + Discovery is by directory contents rather than a hardcoded list so that a skill + added later is covered without anyone remembering to update this file. + """ + if not skills_root.is_dir(): + return [] + return sorted( + candidate for candidate in skills_root.iterdir() if (candidate / ENTRY_DOCUMENT).is_file() + ) + + +SKILL_DIRS: Final = discover_skills(SKILLS_ROOT) + + +def read_frontmatter(skill_dir: pathlib.Path) -> dict[str, object]: + """Parse the YAML frontmatter of a skill's entry document.""" + entry = skill_dir / ENTRY_DOCUMENT + match = FRONTMATTER_PATTERN.match(entry.read_text(encoding="utf-8")) + if match is None: + raise SkillContentError( + f"{entry}: no YAML frontmatter found." + " The file must open with a '---' line and close the block with another." + ) + try: + loaded = YAML(typ="safe").load(match.group("body")) + except YAMLError as exc: + raise SkillContentError(f"{entry}: frontmatter is not valid YAML: {exc}") from exc + if not isinstance(loaded, dict): + raise SkillContentError( + f"{entry}: frontmatter must be a YAML mapping, got {type(loaded).__name__}." + ) + return loaded + + +def name_problems(skill_dir: pathlib.Path) -> list[str]: + """Return every way the ``name`` field fails the format's rules.""" + entry = skill_dir / ENTRY_DOCUMENT + name = read_frontmatter(skill_dir).get("name") + if not isinstance(name, str): + return [f"{entry}: frontmatter 'name' must be a string, got {type(name).__name__}."] + + problems: list[str] = [] + if name != skill_dir.name: + problems.append( + f"{entry}: frontmatter name {name!r} must equal the directory name" + f" {skill_dir.name!r}. Rename one to match the other." + ) + if not SKILL_NAME_PATTERN.fullmatch(name): + problems.append( + f"{entry}: frontmatter name {name!r} must be lowercase letters, digits and" + " single interior hyphens only." + ) + if len(name) > MAX_NAME_LENGTH: + problems.append( + f"{entry}: frontmatter name is {len(name)} characters; the limit is {MAX_NAME_LENGTH}." + ) + return problems + + +def description_problems(skill_dir: pathlib.Path) -> list[str]: + """Return every way the ``description`` field fails the format's rules.""" + entry = skill_dir / ENTRY_DOCUMENT + description = read_frontmatter(skill_dir).get("description") + if not isinstance(description, str): + return [ + f"{entry}: frontmatter 'description' must be a string," + f" got {type(description).__name__}." + ] + + problems: list[str] = [] + if not description.strip(): + problems.append( + f"{entry}: frontmatter description is empty. It is the only text an agent" + " reads when deciding whether to load the skill." + ) + if len(description) > MAX_DESCRIPTION_LENGTH: + problems.append( + f"{entry}: frontmatter description is {len(description)} characters;" + f" the limit is {MAX_DESCRIPTION_LENGTH}." + ) + return problems + + +def _strip_code_fences(text: str) -> str: + """Drop fenced code blocks. + + Snippets mention neighbouring documents in comments ("per preflight.md") without + meaning them as paths to follow, so they are not references and must not be + resolved as such. + """ + kept: list[str] = [] + inside_fence = False + for line in text.splitlines(): + if line.lstrip().startswith("```"): + inside_fence = not inside_fence + continue + if not inside_fence: + kept.append(line) + return "\n".join(kept) + + +def _is_relative_document_reference(candidate: str) -> bool: + target = candidate.split("#", 1)[0] + if not target.endswith(".md"): + return False + if "://" in target or target.startswith(("/", "~")): + return False + # Placeholders such as `.md` and prose containing spaces are not paths. + return not any(character in target for character in "<>| \t") + + +def find_relative_references(document: pathlib.Path) -> set[str]: + """Return the relative document references a markdown file points at. + + The content spells its references as inline code spans, but markdown links are + accepted too: an extractor that recognised only one spelling would return nothing + for the other and every downstream assertion would pass over an empty set. + """ + text = _strip_code_fences(document.read_text(encoding="utf-8")) + candidates = set(CODE_SPAN_PATTERN.findall(text)) | set(MARKDOWN_LINK_PATTERN.findall(text)) + return {candidate for candidate in candidates if _is_relative_document_reference(candidate)} + + +def reference_problems(skill_dir: pathlib.Path) -> list[str]: + """Return every reference in a skill that fails to resolve inside the skill.""" + skill_root = skill_dir.resolve() + problems: list[str] = [] + for document in sorted(skill_dir.rglob("*.md")): + for reference in sorted(find_relative_references(document)): + target = (document.parent / reference.split("#", 1)[0]).resolve() + try: + relative = target.relative_to(skill_root) + except ValueError: + problems.append( + f"{document}: reference {reference!r} points outside" + f" {skill_dir.name}. A skill directory must be self-contained." + ) + continue + if not target.is_file(): + problems.append( + f"{document}: reference {reference!r} does not exist" + f" (resolved to {target}). Add the file or drop the reference." + ) + continue + if len(relative.parts) > MAX_REFERENCE_PARTS: + problems.append( + f"{document}: reference {reference!r} resolves to {relative}," + " which is more than one directory below the skill root." + " Flatten it into the references directory." + ) + return problems + + +def count_references(skill_dir: pathlib.Path) -> int: + return sum(len(find_relative_references(document)) for document in skill_dir.rglob("*.md")) + + +def shared_reference_problems(skills_root: pathlib.Path) -> list[str]: + """Return every shared reference copy that has drifted from the canonical one.""" + problems: list[str] = [] + for shared_name in SHARED_REFERENCES: + canonical = skills_root / CANONICAL_SKILL / REFERENCE_DIR / shared_name + if not canonical.is_file(): + problems.append( + f"{canonical} is missing. It is the canonical copy every other skill's" + f" {shared_name} is compared against." + ) + continue + canonical_bytes = canonical.read_bytes() + for skill_dir in discover_skills(skills_root): + sibling = skill_dir / REFERENCE_DIR / shared_name + if sibling == canonical or not sibling.is_file(): + continue + if sibling.read_bytes() != canonical_bytes: + problems.append( + f"{sibling} has drifted from the canonical copy." + f" Copy {canonical} over {sibling} so the two are byte-identical." + ) + return problems + + +def skills_holding(skills_root: pathlib.Path, shared_name: str) -> list[pathlib.Path]: + return [ + skill_dir + for skill_dir in discover_skills(skills_root) + if (skill_dir / REFERENCE_DIR / shared_name).is_file() + ] + + +def entry_document_size_problems(skill_dir: pathlib.Path) -> list[str]: + """Return a problem when the entry document exceeds its context budget.""" + entry = skill_dir / ENTRY_DOCUMENT + line_count = len(entry.read_text(encoding="utf-8").splitlines()) + if line_count <= MAX_ENTRY_DOCUMENT_LINES: + return [] + return [ + f"{entry} is {line_count} lines; the budget is {MAX_ENTRY_DOCUMENT_LINES}." + f" Move detail into {REFERENCE_DIR}/." + ] + + +@pytest.fixture +def violating_skills(tmp_path: pathlib.Path) -> pathlib.Path: + """A disposable copy of the real content for violations to be introduced into.""" + destination = tmp_path / "skills" + shutil.copytree(SKILLS_ROOT, destination) + assert len(discover_skills(destination)) == len(SKILL_DIRS), ( + f"the copy at {destination} does not hold the same skills as {SKILLS_ROOT}" + ) + return destination + + +def rewrite_frontmatter_field(skill_dir: pathlib.Path, field: str, value: str) -> None: + entry = skill_dir / ENTRY_DOCUMENT + text = entry.read_text(encoding="utf-8") + rewritten = re.sub(rf"^{field}: .*$", f"{field}: {value}", text, count=1, flags=re.MULTILINE) + assert rewritten != text, f"fixture setup did not rewrite {field!r} in {entry}" + entry.write_text(rewritten, encoding="utf-8") + + +# --------------------------------------------------------------------------- +# The real bundled content conforms. +# --------------------------------------------------------------------------- + + +def test_bundled_skills_are_discovered(): + """Everything below is parametrized over discovery, so discovery is checked first.""" + assert SKILLS_ROOT.is_dir(), f"{SKILLS_ROOT} does not exist" + assert len(SKILL_DIRS) >= MIN_BUNDLED_SKILLS, ( + f"expected at least {MIN_BUNDLED_SKILLS} skill directories with an" + f" {ENTRY_DOCUMENT} under {SKILLS_ROOT}, found" + f" {[skill_dir.name for skill_dir in SKILL_DIRS]}" + ) + + +@pytest.mark.parametrize("skill_dir", SKILL_DIRS, ids=lambda skill_dir: skill_dir.name) +def test_frontmatter_parses(skill_dir: pathlib.Path): + frontmatter = read_frontmatter(skill_dir) + assert set(frontmatter) >= {"name", "description"}, ( + f"{skill_dir / ENTRY_DOCUMENT}: frontmatter is missing required fields;" + f" found {sorted(frontmatter)}" + ) + + +@pytest.mark.parametrize("skill_dir", SKILL_DIRS, ids=lambda skill_dir: skill_dir.name) +def test_name_matches_directory_and_format(skill_dir: pathlib.Path): + problems = name_problems(skill_dir) + assert not problems, "\n".join(problems) + + +@pytest.mark.parametrize("skill_dir", SKILL_DIRS, ids=lambda skill_dir: skill_dir.name) +def test_description_is_present_and_within_limit(skill_dir: pathlib.Path): + problems = description_problems(skill_dir) + assert not problems, "\n".join(problems) + + +@pytest.mark.parametrize("skill_dir", SKILL_DIRS, ids=lambda skill_dir: skill_dir.name) +def test_entry_document_references_its_reference_documents(skill_dir: pathlib.Path): + """A skill that appears to reference nothing is the signature of a broken extractor.""" + references = find_relative_references(skill_dir / ENTRY_DOCUMENT) + assert references, ( + f"{skill_dir / ENTRY_DOCUMENT}: no relative document references were extracted." + " Either the entry document stopped routing into its references or the" + " extractor no longer recognises how they are written." + ) + assert {reference for reference in references if reference.startswith(f"{REFERENCE_DIR}/")}, ( + f"{skill_dir / ENTRY_DOCUMENT}: no references into {REFERENCE_DIR}/: {sorted(references)}" + ) + + +@pytest.mark.parametrize("skill_dir", SKILL_DIRS, ids=lambda skill_dir: skill_dir.name) +def test_references_resolve_at_most_one_level_deep(skill_dir: pathlib.Path): + assert count_references(skill_dir) > 0, f"no references were extracted from {skill_dir}" + problems = reference_problems(skill_dir) + assert not problems, "\n".join(problems) + + +@pytest.mark.parametrize("shared_name", SHARED_REFERENCES) +def test_shared_reference_is_carried_by_every_skill_that_needs_it(shared_name: str): + """The equality check below is vacuous unless at least two copies exist.""" + holders = skills_holding(SKILLS_ROOT, shared_name) + assert len(holders) >= MIN_BUNDLED_SKILLS, ( + f"{shared_name} was found in {[holder.name for holder in holders]};" + f" expected at least {MIN_BUNDLED_SKILLS} skills to carry their own copy" + ) + + +def test_shared_references_are_byte_identical(): + problems = shared_reference_problems(SKILLS_ROOT) + assert not problems, "\n".join(problems) + + +@pytest.mark.parametrize("skill_dir", SKILL_DIRS, ids=lambda skill_dir: skill_dir.name) +def test_entry_document_within_size_budget(skill_dir: pathlib.Path): + problems = entry_document_size_problems(skill_dir) + assert not problems, "\n".join(problems) + + +# --------------------------------------------------------------------------- +# Each check above catches the violation it exists for. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ["entry_text", "expected_message"], + [ + pytest.param("# no frontmatter here\n", "no YAML frontmatter", id="delimiters_missing"), + pytest.param( + '---\nname: gx-configure-data-source\ndescription: "unterminated\n---\n# body\n', + "not valid YAML", + id="unparseable_yaml", + ), + pytest.param( + "---\njust a string\n---\n# body\n", + "must be a YAML mapping", + id="not_a_mapping", + ), + ], +) +def test_broken_frontmatter_is_reported( + violating_skills: pathlib.Path, entry_text: str, expected_message: str +): + skill_dir = violating_skills / CANONICAL_SKILL + (skill_dir / ENTRY_DOCUMENT).write_text(entry_text, encoding="utf-8") + + with pytest.raises(SkillContentError, match=expected_message): + read_frontmatter(skill_dir) + + +def test_name_that_disagrees_with_the_directory_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + rewrite_frontmatter_field(skill_dir, "name", "some-other-skill") + + problems = name_problems(skill_dir) + + assert [problem for problem in problems if "must equal the directory name" in problem] + + +def test_name_outside_the_allowed_character_set_is_reported(violating_skills: pathlib.Path): + # The directory is renamed to match, so the character-set rule is the only one left + # that can fail -- otherwise this would ride on the directory-match check instead. + disallowed = "GX_Configure_Data_Source" + skill_dir = (violating_skills / CANONICAL_SKILL).rename(violating_skills / disallowed) + rewrite_frontmatter_field(skill_dir, "name", disallowed) + + problems = name_problems(skill_dir) + + assert [problem for problem in problems if "lowercase letters" in problem] + assert not [problem for problem in problems if "must equal the directory name" in problem] + + +def test_name_over_the_length_limit_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + over_limit = "-".join(["gx"] * ((MAX_NAME_LENGTH // 3) + 1)) + assert len(over_limit) > MAX_NAME_LENGTH + skill_dir = skill_dir.rename(skill_dir.parent / over_limit) + rewrite_frontmatter_field(skill_dir, "name", over_limit) + + problems = name_problems(skill_dir) + + assert [problem for problem in problems if "the limit is" in problem] + + +def test_empty_description_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + rewrite_frontmatter_field(skill_dir, "description", '" "') + + problems = description_problems(skill_dir) + + assert [problem for problem in problems if "description is empty" in problem] + + +def test_description_over_the_length_limit_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + rewrite_frontmatter_field(skill_dir, "description", "d" * (MAX_DESCRIPTION_LENGTH + 1)) + + problems = description_problems(skill_dir) + + assert [problem for problem in problems if "the limit is" in problem] + + +def test_dangling_reference_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + entry = skill_dir / ENTRY_DOCUMENT + entry.write_text( + f"{entry.read_text(encoding='utf-8')}\nSee `{REFERENCE_DIR}/does-not-exist.md`.\n", + encoding="utf-8", + ) + + problems = reference_problems(skill_dir) + + assert [problem for problem in problems if "does not exist" in problem] + + +def test_reference_more_than_one_level_deep_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + nested = skill_dir / REFERENCE_DIR / "nested" / "buried.md" + nested.parent.mkdir() + nested.write_text("# buried\n", encoding="utf-8") + entry = skill_dir / ENTRY_DOCUMENT + entry.write_text( + f"{entry.read_text(encoding='utf-8')}\nSee `{REFERENCE_DIR}/nested/buried.md`.\n", + encoding="utf-8", + ) + + problems = reference_problems(skill_dir) + + assert [problem for problem in problems if "more than one directory below" in problem] + + +def test_reference_escaping_the_skill_directory_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + entry = skill_dir / ENTRY_DOCUMENT + sibling = next( + candidate + for candidate in discover_skills(violating_skills) + if candidate.name != CANONICAL_SKILL + ) + escaping = f"../{sibling.name}/{REFERENCE_DIR}/preflight.md" + entry.write_text(f"{entry.read_text(encoding='utf-8')}\nSee `{escaping}`.\n", encoding="utf-8") + + # The reference resolves to a real file, so only the containment rule catches it. + assert (entry.parent / escaping).is_file() + problems = reference_problems(skill_dir) + + assert [problem for problem in problems if "points outside" in problem] + + +@pytest.mark.parametrize("shared_name", SHARED_REFERENCES) +def test_diverged_shared_reference_is_reported(violating_skills: pathlib.Path, shared_name: str): + sibling = next( + skill_dir / REFERENCE_DIR / shared_name + for skill_dir in discover_skills(violating_skills) + if skill_dir.name != CANONICAL_SKILL + ) + sibling.write_text( + f"{sibling.read_text(encoding='utf-8')}\nAn edit made to one copy only.\n", + encoding="utf-8", + ) + + problems = shared_reference_problems(violating_skills) + + canonical = violating_skills / CANONICAL_SKILL / REFERENCE_DIR / shared_name + remedy = f"Copy {canonical} over {sibling}" + assert [problem for problem in problems if "drifted from the canonical copy" in problem] + assert [problem for problem in problems if remedy in problem], ( + f"the failure must name the fix; got {problems}" + ) + + +@pytest.mark.parametrize("shared_name", SHARED_REFERENCES) +def test_missing_canonical_shared_reference_is_reported( + violating_skills: pathlib.Path, shared_name: str +): + (violating_skills / CANONICAL_SKILL / REFERENCE_DIR / shared_name).unlink() + + problems = shared_reference_problems(violating_skills) + + assert [problem for problem in problems if "is missing" in problem] + + +def test_over_budget_entry_document_is_reported(violating_skills: pathlib.Path): + skill_dir = violating_skills / CANONICAL_SKILL + entry = skill_dir / ENTRY_DOCUMENT + padding = "\n".join(["padding"] * (MAX_ENTRY_DOCUMENT_LINES + 1)) + entry.write_text(f"{entry.read_text(encoding='utf-8')}\n{padding}\n", encoding="utf-8") + + problems = entry_document_size_problems(skill_dir) + + assert [problem for problem in problems if "the budget is" in problem] diff --git a/tests/agent_skills/test_snippets.py b/tests/agent_skills/test_snippets.py new file mode 100644 index 000000000000..ab2595d20470 --- /dev/null +++ b/tests/agent_skills/test_snippets.py @@ -0,0 +1,1244 @@ +"""Execution tests for the code in the agent skills bundled with ``great_expectations``. + +The skills teach an agent how to drive this library, and every instruction they give is +carried by a code snippet. Markdown is not compiled, imported or linted by anything, so +a snippet that stopped working would keep being handed to users indefinitely. Three +contracts are checked here, each of which fails silently without a test: + +1. **Every fenced Python block parses.** A block is dedented first -- some are nested + inside list items -- so the check covers the source as an agent would copy it. +2. **The blocks tagged ``executable`` on their fence really do run, in order, as one + program.** They are executed against a throwaway in-memory session backed by a local + SQLite database and an in-memory dataframe, and the run has to reach the end states + the skills promise: a batch definition proven by reading data through it, a + validation result carrying one entry per expectation, and a written-out project + directory that a *fresh* file-backed context opens with its batch definitions and + suites usable as they are. +3. **The failure behavior the guidance is built on still behaves that way.** The skills + tell an agent that retrieving a batch proves nothing, that a false ``success`` means + two different things, and that empty tables and all-null columns produce ordinary + results rather than errors. Each of those claims is pinned below against real + execution, so a change in library behavior fails here -- next to a message naming + the document whose text has to be updated -- instead of quietly turning the shipped + guidance into misinformation. + +Nothing here reaches the network. Everything runs against a local SQLite file and an +in-memory dataframe, because a check that needs a warehouse is a check that never runs. + +The tagging mechanism is the fence itself: ``` ```python executable ``` marks a block as +part of the runnable sequence, which keeps the tag attached to the block it describes +instead of in a list somewhere that content edits can silently invalidate. The order the +tagged blocks run in is ``EXECUTABLE_SEQUENCE``, and its per-document counts are +asserted against the content so a tag that is added, moved or dropped fails loudly +rather than quietly leaving a block unexecuted. +""" + +from __future__ import annotations + +import dataclasses +import json +import pathlib +import sqlite3 +import textwrap +from typing import TYPE_CHECKING, Any, Callable, Final, Iterator + +import pandas as pd +import pytest + +import great_expectations as gx +from great_expectations.data_context import EphemeralDataContext, FileDataContext +from great_expectations.datasource.fluent.interfaces import Batch +from great_expectations.exceptions.exceptions import ( + InvalidBatchRequestError, + NoAvailableBatchesError, +) + +if TYPE_CHECKING: + from great_expectations.core import ExpectationSuite + from great_expectations.core.expectation_validation_result import ( + ExpectationSuiteValidationResult, + ExpectationValidationResult, + ) + from great_expectations.datasource.fluent.sqlite_datasource import SqliteDatasource + from great_expectations.expectations.expectation_configuration import ( + ExpectationConfiguration, + ) + +PROJECT_ROOT: Final = pathlib.Path(__file__).parents[2] +SKILLS_ROOT: Final = PROJECT_ROOT / "great_expectations" / ".agents" / "skills" + +ENTRY_DOCUMENT: Final = "SKILL.md" +REFERENCE_DIR: Final = "references" +CANONICAL_SKILL: Final = "gx-configure-data-source" +SHARED_REFERENCES: Final = ("preflight.md", "write-out.md", "robustness.md") + +FENCE: Final = "```" +PYTHON: Final = "python" +EXECUTABLE_TAG: Final = "executable" + +#: Guards every parametrized compile check against a discovery bug reducing it to zero +#: cases. The content holds comfortably more than this; the number is a floor, not a +#: count, so ordinary editing does not have to keep it in step. +MIN_PYTHON_BLOCKS: Final = 40 + +#: The documents whose ``executable`` blocks make up the runnable sequence, in the order +#: they run, with the number of tagged blocks each one must contribute. Shared +#: references are listed under the skill that owns the canonical copy; the byte-identical +#: copy in the other skill is the same content and is not run twice. +EXECUTABLE_SEQUENCE: Final[tuple[tuple[str, int], ...]] = ( + (f"{CANONICAL_SKILL}/{REFERENCE_DIR}/preflight.md", 3), + (f"{CANONICAL_SKILL}/{ENTRY_DOCUMENT}", 3), + (f"{CANONICAL_SKILL}/{REFERENCE_DIR}/robustness.md", 1), + (f"gx-configure-expectations/{ENTRY_DOCUMENT}", 5), + (f"{CANONICAL_SKILL}/{REFERENCE_DIR}/write-out.md", 2), +) + +#: Values the content deliberately leaves for the user to supply, spelled in angle +#: brackets so they are unmistakably placeholders. The runner fills them in and then +#: asserts it actually did, so a snippet that stopped carrying its placeholder cannot +#: leave the substitution silently doing nothing. +CONFIRMED_PATH_PLACEHOLDER: Final = "" + +#: Environment left over from another project or from the retired managed cloud offering +#: changes what context discovery returns. The runnable sequence starts by discovering a +#: context, so the ambient environment has to be neutral for the run to mean anything. +AMBIENT_ENVIRONMENT: Final = ( + "GX_HOME", + "GX_CLOUD_ACCESS_TOKEN", + "GX_CLOUD_ORGANIZATION_ID", + "GX_CLOUD_BASE_URL", +) + +#: The table the runnable sequence configures, and the rows the skills' own worked +#: example describes: four rows, one missing customer, one negative amount, all inside a +#: single month so a monthly batch definition selects all of them. +ORDERS_ROWS: Final = ( + ("alice", 10.0, "2024-03-01"), + (None, 20.0, "2024-03-05"), + ("carol", -5.0, "2024-03-09"), + ("dan", 40.0, "2024-03-20"), +) +ORDERS_COLUMNS: Final = ("customer", "amount", "ordered_at") +NULL_ROW_COUNT: Final = 3 + +MISSING_TABLE: Final = "does_not_exist" + +#: The ceiling the guidance's own error-extraction snippet applies to a recovered cause, +#: plus the suffix it appends when it truncates. +CAUSE_CEILING: Final = 500 +TRUNCATION_SUFFIX: Final = "... (truncated)" + +#: Text unique to the write-out procedure snippet, used to locate it after it ran. +WRITE_OUT_STEPS_NEEDLE: Final = "for label, step in steps:" + + +@dataclasses.dataclass(frozen=True) +class CodeBlock: + """One fenced block, with the fence's info string split into language and tags.""" + + document: pathlib.Path + line: int + info: str + source: str + + @property + def language(self) -> str: + parts = self.info.split() + return parts[0] if parts else "" + + @property + def tags(self) -> frozenset[str]: + return frozenset(self.info.split()[1:]) + + @property + def identifier(self) -> str: + """A stable ``/:`` label, used as the compiled filename.""" + try: + name = self.document.relative_to(SKILLS_ROOT).as_posix() + except ValueError: # a document built by a test rather than shipped content + name = self.document.name + return f"{name}:{self.line}" + + +def iter_code_blocks(document: pathlib.Path) -> list[CodeBlock]: + """Return every fenced block in a markdown document, dedented. + + Blocks nested inside a list item are indented in the source; an agent copying one + out reads it without that indentation, so it is removed before the source is + compiled or executed. Without the dedent every indented block would fail to parse. + """ + blocks: list[CodeBlock] = [] + inside = False + opening_line = 0 + info = "" + body: list[str] = [] + for number, line in enumerate(document.read_text(encoding="utf-8").splitlines(), start=1): + stripped = line.lstrip() + if stripped.startswith(FENCE): + if inside: + blocks.append( + CodeBlock( + document=document, + line=opening_line, + info=info, + source=textwrap.dedent("\n".join(body)), + ) + ) + inside = False + else: + inside = True + opening_line = number + info = stripped[len(FENCE) :].strip() + body = [] + continue + if inside: + body.append(line) + assert not inside, ( + f"{document}: a code fence opened at line {opening_line} is never closed." + " Every fence needs a matching closing fence." + ) + return blocks + + +def python_blocks(document: pathlib.Path) -> list[CodeBlock]: + return [block for block in iter_code_blocks(document) if block.language == PYTHON] + + +def discover_documents(skills_root: pathlib.Path) -> list[pathlib.Path]: + """Every markdown document in every bundled skill, found by walking the tree. + + Discovery is by directory contents rather than a hardcoded list so a document added + later is covered without anyone remembering to update this file. + """ + if not skills_root.is_dir(): + return [] + return sorted(skills_root.rglob("*.md")) + + +def canonical_relative_path(document: pathlib.Path) -> str: + """Map a shared reference onto the skill that owns its canonical copy. + + The shared references are committed once per skill directory and asserted + byte-identical elsewhere, so the copies carry identical tags. Collapsing them here + keeps the runnable sequence from executing the same block twice. + """ + relative = document.relative_to(SKILLS_ROOT) + parts = relative.parts + if len(parts) == 3 and parts[1] == REFERENCE_DIR and parts[2] in SHARED_REFERENCES: + return f"{CANONICAL_SKILL}/{parts[1]}/{parts[2]}" + return relative.as_posix() + + +DOCUMENTS: Final = discover_documents(SKILLS_ROOT) +ALL_PYTHON_BLOCKS: Final = [block for document in DOCUMENTS for block in python_blocks(document)] + + +def executable_blocks_in_sequence() -> list[CodeBlock]: + """The tagged blocks, in the order ``EXECUTABLE_SEQUENCE`` declares.""" + ordered: list[CodeBlock] = [] + for relative_path, _expected in EXECUTABLE_SEQUENCE: + document = SKILLS_ROOT / relative_path + ordered.extend(block for block in python_blocks(document) if EXECUTABLE_TAG in block.tags) + return ordered + + +def configuration_of(result: ExpectationValidationResult) -> ExpectationConfiguration: + """The configuration a result came from. + + Pairing a result with the expectation it belongs to is the rule the skills state, so + a result that arrived without its configuration would make that rule unfollowable. + """ + configuration = result.expectation_config + assert configuration is not None, ( + "a validation result arrived without the expectation configuration it came from," + " so results can no longer be paired with their expectations at all" + ) + return configuration + + +def sole_block_containing(document: pathlib.Path, needle: str) -> CodeBlock: + """The one Python block in ``document`` holding ``needle``. + + Selecting a block by something it says, rather than by position, means a block that + moves is still found and a block that is deleted or duplicated fails here instead of + silently changing which snippet a test exercises. + """ + matches = [block for block in python_blocks(document) if needle in block.source] + assert len(matches) == 1, ( + f"expected exactly one Python block in {document} containing {needle!r}," + f" found {len(matches)} (at lines {[block.line for block in matches]})" + ) + return matches[0] + + +# --------------------------------------------------------------------------- +# Every fenced Python block parses. +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_python_blocks_are_extracted_from_every_skill(): + """The compile checks below are parametrized over extraction, so it is checked first.""" + assert SKILLS_ROOT.is_dir(), f"{SKILLS_ROOT} does not exist" + assert len(ALL_PYTHON_BLOCKS) >= MIN_PYTHON_BLOCKS, ( + f"only {len(ALL_PYTHON_BLOCKS)} fenced {PYTHON} blocks were extracted from" + f" {len(DOCUMENTS)} documents under {SKILLS_ROOT}; expected at least" + f" {MIN_PYTHON_BLOCKS}. Either the content lost its snippets or the extractor no" + " longer recognises how they are fenced." + ) + skills_with_snippets = { + block.document.relative_to(SKILLS_ROOT).parts[0] for block in ALL_PYTHON_BLOCKS + } + assert len(skills_with_snippets) >= 2, ( + f"snippets were only extracted from {sorted(skills_with_snippets)};" + " every bundled skill teaches through code and should contribute some" + ) + + +@pytest.mark.unit +@pytest.mark.parametrize( + "block", ALL_PYTHON_BLOCKS, ids=[block.identifier for block in ALL_PYTHON_BLOCKS] +) +def test_python_block_compiles(block: CodeBlock): + """A snippet an agent is told to run has to be valid Python before anything else.""" + try: + compile(block.source, block.identifier, "exec") + except SyntaxError as error: + pytest.fail(f"{block.identifier} does not parse as Python: {error}") + + +# --------------------------------------------------------------------------- +# The extraction each check above rests on reports the problems it exists for. +# --------------------------------------------------------------------------- + + +def _write_markdown(directory: pathlib.Path, text: str) -> pathlib.Path: + document = directory / "sample.md" + document.write_text(textwrap.dedent(text), encoding="utf-8") + return document + + +@pytest.mark.unit +def test_a_block_that_does_not_parse_is_reported(tmp_path: pathlib.Path): + document = _write_markdown( + tmp_path, + """\ + # sample + + ```python + def broken( + ``` + """, + ) + + (block,) = python_blocks(document) + + with pytest.raises(SyntaxError): + compile(block.source, block.identifier, "exec") + + +@pytest.mark.unit +def test_a_block_nested_in_a_list_item_is_dedented(tmp_path: pathlib.Path): + """Without the dedent an indented block raises ``IndentationError`` on every edit.""" + document = _write_markdown( + tmp_path, + """\ + # sample + + - a bullet holding a snippet: + + ```python + value = 1 + ``` + """, + ) + + (block,) = python_blocks(document) + + assert block.source == "value = 1", f"the block was not dedented: {block.source!r}" + compile(block.source, block.identifier, "exec") + + +@pytest.mark.unit +def test_an_unclosed_fence_is_reported(tmp_path: pathlib.Path): + document = _write_markdown( + tmp_path, + """\ + # sample + + ```python + value = 1 + """, + ) + + with pytest.raises(AssertionError, match="is never closed"): + python_blocks(document) + + +@pytest.mark.unit +def test_only_python_fences_are_collected(tmp_path: pathlib.Path): + """Illustrative output blocks are not code and must not be compiled as code.""" + document = _write_markdown( + tmp_path, + """\ + # sample + + ```python + value = 1 + ``` + + ```text + not python at all: [ + ``` + """, + ) + + blocks = python_blocks(document) + + assert [block.source for block in blocks] == ["value = 1"] + assert len(iter_code_blocks(document)) == 2 + + +@pytest.mark.unit +def test_the_executable_tag_is_read_off_the_fence(tmp_path: pathlib.Path): + document = _write_markdown( + tmp_path, + """\ + # sample + + ```python executable + tagged = True + ``` + + ```python + untagged = True + ``` + """, + ) + + tagged, untagged = python_blocks(document) + + assert tagged.language == PYTHON and EXECUTABLE_TAG in tagged.tags + assert untagged.language == PYTHON and EXECUTABLE_TAG not in untagged.tags + + +@pytest.mark.unit +def test_selecting_a_block_by_its_content_reports_an_ambiguous_match(tmp_path: pathlib.Path): + document = _write_markdown( + tmp_path, + """\ + # sample + + ```python + value = 1 + ``` + + ```python + value = 1 + ``` + """, + ) + + with pytest.raises(AssertionError, match="exactly one Python block"): + sole_block_containing(document, "value = 1") + + +# --------------------------------------------------------------------------- +# The runnable sequence covers exactly the blocks the content tags. +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_every_tagged_block_is_part_of_the_run_sequence(): + """A tag the runner does not know about would leave a snippet silently unexecuted.""" + tagged_by_document: dict[str, int] = {} + for document in DOCUMENTS: + count = sum(1 for block in python_blocks(document) if EXECUTABLE_TAG in block.tags) + if count: + key = canonical_relative_path(document) + assert tagged_by_document.get(key, count) == count, ( + f"{document} carries {count} tagged blocks but its byte-identical" + f" counterpart carries {tagged_by_document[key]}" + ) + tagged_by_document[key] = count + + assert tagged_by_document == dict(EXECUTABLE_SEQUENCE), ( + "the tagged blocks in the content do not match the declared run sequence." + f" Content: {tagged_by_document}. Declared: {dict(EXECUTABLE_SEQUENCE)}." + " Add the document to EXECUTABLE_SEQUENCE, or update its expected count." + ) + + +@pytest.mark.unit +def test_the_write_out_snippet_still_carries_its_placeholder(): + """The runner fills this in; a snippet without it would run against nothing.""" + document = SKILLS_ROOT / CANONICAL_SKILL / REFERENCE_DIR / "write-out.md" + holders = [ + block for block in python_blocks(document) if CONFIRMED_PATH_PLACEHOLDER in block.source + ] + assert len(holders) == 1, ( + f"expected exactly one snippet in {document} to carry" + f" {CONFIRMED_PATH_PLACEHOLDER!r}, found {len(holders)}" + ) + + +# --------------------------------------------------------------------------- +# Fixtures: a local warehouse and an in-memory session, no network anywhere. +# --------------------------------------------------------------------------- + + +def _build_warehouse(path: pathlib.Path) -> pathlib.Path: + """A SQLite database holding an ordinary table and two degenerate ones.""" + types = ("TEXT", "REAL", "TEXT") + columns = ", ".join(f"{name} {kind}" for name, kind in zip(ORDERS_COLUMNS, types, strict=True)) + connection = sqlite3.connect(path) + try: + for table in ("orders", "empty_orders", "all_null_orders"): + connection.execute(f"CREATE TABLE {table} ({columns})") + connection.executemany("INSERT INTO orders VALUES (?, ?, ?)", ORDERS_ROWS) + connection.executemany( + "INSERT INTO all_null_orders VALUES (?, ?, ?)", + [(None, None, "2024-03-01")] * NULL_ROW_COUNT, + ) + connection.commit() + finally: + connection.close() + return path + + +def _customers_frame() -> pd.DataFrame: + """The in-memory dataframe the runnable sequence hands to its dataframe asset.""" + return pd.DataFrame({"customer": ["erin", None], "amount": [12.5, -3.0]}) + + +@pytest.fixture(scope="module") +def warehouse_path(tmp_path_factory: pytest.TempPathFactory) -> pathlib.Path: + return _build_warehouse(tmp_path_factory.mktemp("warehouse") / "warehouse.sqlite") + + +@pytest.fixture +def ephemeral_context( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> EphemeralDataContext: + """An in-memory session, discovered exactly the way the skills tell an agent to.""" + monkeypatch.chdir(tmp_path) + for name in AMBIENT_ENVIRONMENT: + monkeypatch.delenv(name, raising=False) + context = gx.get_context(cloud_mode=False) + assert isinstance(context, EphemeralDataContext), ( + f"expected an in-memory session in an empty directory, got {type(context).__name__}" + ) + return context + + +@pytest.fixture +def warehouse( + ephemeral_context: EphemeralDataContext, warehouse_path: pathlib.Path +) -> SqliteDatasource: + return ephemeral_context.data_sources.add_or_update_sqlite( + name="warehouse", connection_string=f"sqlite:///{warehouse_path}" + ) + + +def whole_table_batch(datasource: SqliteDatasource, table: str) -> Batch: + asset = datasource.add_table_asset(name=table, table_name=table) + return asset.add_batch_definition_whole_table(name="all_rows").get_batch() + + +# --------------------------------------------------------------------------- +# The tagged blocks run, in order, and reach the end states the skills promise. +# --------------------------------------------------------------------------- + + +@dataclasses.dataclass(frozen=True) +class ExecutedFlow: + """The result of running the tagged sequence as one program.""" + + blocks: tuple[CodeBlock, ...] + namespace: dict[str, Any] + #: The namespace as it stood after each block, so an intermediate end state can be + #: asserted even though a later block rebinds the name. + snapshots: tuple[dict[str, Any], ...] + substituted: frozenset[str] + project_root: pathlib.Path + dataframe: pd.DataFrame + + def after(self, needle: str) -> dict[str, Any]: + """The namespace as it stood after the one executed block saying ``needle``. + + Selecting the block by something it says, rather than by line number, keeps + these assertions attached to the snippet they are about when the surrounding + prose is edited -- and turns a snippet that was deleted or duplicated into a + failure here rather than into an assertion that quietly moved to another block. + """ + matched = [ + snapshot + for block, snapshot in zip(self.blocks, self.snapshots, strict=True) + if needle in block.source + ] + assert len(matched) == 1, ( + f"expected exactly one executed block containing {needle!r}, found {len(matched)}" + ) + return matched[0] + + +@pytest.fixture(scope="module") +def executed_flow(tmp_path_factory: pytest.TempPathFactory) -> Iterator[ExecutedFlow]: + """Run every ``executable`` block, in sequence, in one shared namespace.""" + root = tmp_path_factory.mktemp("skill_flow") + warehouse_file = _build_warehouse(root / "warehouse.sqlite") + project_root = root / "written_out" + project_root.mkdir() + working_directory = root / "no_project_here" + working_directory.mkdir() + + blocks = tuple(executable_blocks_in_sequence()) + dataframe = _customers_frame() + namespace: dict[str, Any] = {"df": dataframe} + snapshots: list[dict[str, Any]] = [] + substituted: set[str] = set() + + def load_written_out_project(state: dict[str, Any]) -> None: + """Reopen the written-out project the way a later session would. + + The final snippet retrieves a batch through a batch definition; pointing that + name at the *reloaded* definition is what makes the retrieval evidence that the + written-out project is usable rather than evidence about objects still in memory. + """ + reloaded = gx.get_context(mode="file", project_root_dir=str(project_root)) + state["reloaded_context"] = reloaded + state["batch_definition"] = ( + reloaded.data_sources.get("my_datasource") + .get_asset("my_asset") + .get_batch_definition("my_batch_definition") + ) + + hooks: dict[tuple[str, int], Callable[[dict[str, Any]], None]] = { + (f"{CANONICAL_SKILL}/{REFERENCE_DIR}/write-out.md", 1): load_written_out_project, + } + fired: set[tuple[str, int]] = set() + seen_per_document: dict[str, int] = {} + + with pytest.MonkeyPatch.context() as patch: + patch.chdir(working_directory) + patch.setenv("WAREHOUSE_PATH", str(warehouse_file)) + for name in AMBIENT_ENVIRONMENT: + patch.delenv(name, raising=False) + + for block in blocks: + key = canonical_relative_path(block.document) + position = seen_per_document.get(key, 0) + seen_per_document[key] = position + 1 + hook = hooks.get((key, position)) + if hook is not None: + hook(namespace) + fired.add((key, position)) + + source = block.source + if CONFIRMED_PATH_PLACEHOLDER in source: + source = source.replace(CONFIRMED_PATH_PLACEHOLDER, str(project_root)) + substituted.add(CONFIRMED_PATH_PLACEHOLDER) + + try: + exec(compile(source, block.identifier, "exec"), namespace) + except Exception as error: # pragma: no cover - the failure is the report + raise AssertionError( + f"the snippet at {block.identifier} failed to run as part of the" + f" documented sequence: {type(error).__name__}: {error}" + ) from error + snapshots.append(dict(namespace)) + + assert fired == set(hooks), f"a run-sequence hook never fired: {set(hooks) - fired}" + + yield ExecutedFlow( + blocks=blocks, + namespace=namespace, + snapshots=tuple(snapshots), + substituted=frozenset(substituted), + project_root=project_root, + dataframe=dataframe, + ) + + executor = namespace.get("executor") + if executor is not None: + executor.shutdown(wait=True) + + +@pytest.mark.sqlite +def test_the_whole_tagged_sequence_ran(executed_flow: ExecutedFlow): + """Everything below reads state the sequence produced, so the run is checked first.""" + expected = sum(count for _document, count in EXECUTABLE_SEQUENCE) + assert len(executed_flow.blocks) == expected + assert len(executed_flow.snapshots) == expected, "a block was skipped mid-sequence" + assert executed_flow.substituted == frozenset({CONFIRMED_PATH_PLACEHOLDER}), ( + "the write-out snippet's placeholder was never substituted, so the procedure did" + " not run against the confirmed directory" + ) + + +@pytest.mark.sqlite +def test_preflight_lands_in_an_announced_in_memory_session(executed_flow: ExecutedFlow): + """Discovery in a directory with no project is an in-memory session, not an error.""" + discovered = executed_flow.after("context = gx.get_context(cloud_mode=False)")["context"] + assert isinstance(discovered, EphemeralDataContext) + + branch = executed_flow.after("isinstance(context, FileDataContext)") + assert branch["FileDataContext"] is FileDataContext, ( + "the branch snippet no longer imports the type it branches on" + ) + assert "context_root" not in branch, ( + "the file-backed branch ran against an in-memory session; the snippet's" + " isinstance check is not doing what the guidance says it does" + ) + + +@pytest.mark.sqlite +def test_the_batch_definition_is_verified_by_reading_data_through_it( + executed_flow: ExecutedFlow, +): + """Retrieval plus a probe that returns rows is the end state the flow promises.""" + probed = executed_flow.after("head = batch.head(n_rows=5)") + batch = probed["batch"] + assert isinstance(batch, Batch) + + frame = probed["head"].data + assert list(frame.columns) == list(ORDERS_COLUMNS) + assert len(frame) == len(ORDERS_ROWS) + + batch_definition = probed["batch_definition"] + assert batch_definition.name == "by_month" + + context = executed_flow.namespace["context"] + reachable = ( + context.data_sources.get("warehouse").get_asset("orders").get_batch_definition("by_month") + ) + assert reachable.name == batch_definition.name, ( + "the verified batch definition is not retrievable from the session by name," + " so nothing was actually saved for a later step to use" + ) + + +@pytest.mark.sqlite +def test_the_time_budget_wrapper_returns_the_probe_result(executed_flow: ExecutedFlow): + """The wrapper polls while the call is in flight; a fast call just returns.""" + wrapped = executed_flow.after("BUDGET_SECONDS") + assert wrapped["succeeded"] is True, ( + "the duration-tracked wrapper reported failure for a call that works;" + f" update {CANONICAL_SKILL}/{REFERENCE_DIR}/robustness.md if this is intended" + ) + assert wrapped["checked_in"] is False, "a sub-second probe should not trip the budget" + assert wrapped["result"] is not None + assert wrapped["result"].data is not None + + +@pytest.mark.sqlite +def test_validation_reports_every_expectation_on_its_own_terms(executed_flow: ExecutedFlow): + """One entry per expectation, each carrying the configuration it came from.""" + result: ExpectationSuiteValidationResult = executed_flow.after( + "result = batch.validate(suite)" + )["result"] + by_type = {configuration_of(each).type: each for each in result.results} + assert set(by_type) == { + "expect_column_values_to_not_be_null", + "expect_column_values_to_be_between", + } + + missing_customer = by_type["expect_column_values_to_not_be_null"] + negative_amount = by_type["expect_column_values_to_be_between"] + assert missing_customer.success is False + assert negative_amount.success is False + assert missing_customer.result["element_count"] == len(ORDERS_ROWS) + assert missing_customer.result["unexpected_count"] == 1 + assert missing_customer.result["partial_unexpected_list"] == [None] + assert negative_amount.result["partial_unexpected_list"] == [-5.0] + + described = json.loads(result.describe()) + assert described["statistics"]["evaluated_expectations"] == len(result.results) + + +@pytest.mark.sqlite +def test_the_suite_the_flow_built_is_registered_with_the_session(executed_flow: ExecutedFlow): + """Expectations added to a suite the context never saw are lost without a word. + + Fetching the suite back by name is the only thing that distinguishes a suite that + was registered before its expectations were added from one that was not: building, + adding and validating all work either way. + """ + context = executed_flow.namespace["context"] + registered = context.suites.get("orders_quality") + assert [type(each).__name__ for each in registered.expectations] == [ + "ExpectColumnValuesToNotBeNull", + "ExpectColumnValuesToBeBetween", + ], ( + "the suite the flow built did not come back from the session with its" + " expectations; the register-first ordering in" + f" gx-configure-expectations/{ENTRY_DOCUMENT} is what keeps them" + ) + + +@pytest.mark.sqlite +def test_the_write_out_procedure_reports_every_object_it_wrote(executed_flow: ExecutedFlow): + """The procedure records failures instead of raising, so the record is the evidence.""" + written = executed_flow.after(WRITE_OUT_STEPS_NEEDLE)["written"] + failed = executed_flow.after(WRITE_OUT_STEPS_NEEDLE)["failed"] + assert failed == [], f"write-out steps failed: {failed}" + assert written == [ + "data source my_datasource", + "asset my_asset", + "batch definition my_batch_definition", + "suite my_suite", + ] + + +@pytest.mark.sqlite +def test_a_fresh_file_backed_context_loads_the_written_out_work(executed_flow: ExecutedFlow): + """The written-out project is usable as it stands, from a context that never saw the session.""" + reloaded = gx.get_context(mode="file", project_root_dir=str(executed_flow.project_root)) + assert isinstance(reloaded, FileDataContext) + + batch_definition = ( + reloaded.data_sources.get("my_datasource") + .get_asset("my_asset") + .get_batch_definition("my_batch_definition") + ) + suite: ExpectationSuite = reloaded.suites.get("orders_quality") + assert [type(each).__name__ for each in suite.expectations] == [ + "ExpectColumnValuesToNotBeNull", + "ExpectColumnValuesToBeBetween", + ] + + # A dataframe asset carries configuration but no data, which is the one documented + # caveat on "usable without modification" -- the frame is supplied at retrieval time. + batch = batch_definition.get_batch(batch_parameters={"dataframe": executed_flow.dataframe}) + assert len(batch.head(n_rows=5).data) == len(executed_flow.dataframe) + + result = batch.validate(suite) + assert len(result.results) == len(suite.expectations) + assert all(each.result for each in result.results), ( + "the reloaded suite produced no per-expectation payloads, so it did not actually" + " evaluate against the reloaded batch" + ) + + # The snippet the sequence ended on retrieved a batch through the reloaded definition. + assert isinstance(executed_flow.namespace["batch"], Batch) + + +# --------------------------------------------------------------------------- +# The failure behavior the guidance is built on. +# --------------------------------------------------------------------------- + + +@pytest.mark.sqlite +def test_a_query_over_a_missing_table_yields_a_batch_and_fails_only_when_probed( + warehouse: SqliteDatasource, +): + """Retrieval touches nothing, which is why the guidance mandates a probe.""" + asset = warehouse.add_query_asset(name="broken", query=f"SELECT * FROM {MISSING_TABLE}") + batch = asset.add_batch_definition_whole_table(name="all_rows").get_batch() + + assert isinstance(batch, Batch), ( + "retrieving a batch over a missing table no longer succeeds; the probe-first" + f" rule in {CANONICAL_SKILL}/{REFERENCE_DIR}/robustness.md rests on it doing so" + ) + + with pytest.raises(KeyError) as raised: + batch.head(n_rows=5) + + assert MISSING_TABLE not in str(raised.value), ( + "the probe failure now names the real problem, so the recovery procedure in" + f" {CANONICAL_SKILL}/{REFERENCE_DIR}/robustness.md is heavier than it needs to be" + ) + + +@pytest.mark.sqlite +def test_the_guidance_recovers_the_real_cause_behind_a_bare_probe_failure( + warehouse: SqliteDatasource, +): + """The reference's own capture snippet is executed, not paraphrased.""" + asset = warehouse.add_query_asset(name="broken", query=f"SELECT * FROM {MISSING_TABLE}") + batch = asset.add_batch_definition_whole_table(name="all_rows").get_batch() + + document = SKILLS_ROOT / CANONICAL_SKILL / REFERENCE_DIR / "robustness.md" + snippet = sole_block_containing(document, "class _CaptureHandler") + namespace: dict[str, Any] = {"batch": batch} + exec(compile(snippet.source, snippet.identifier, "exec"), namespace) + + cause = namespace.get("cause") + assert cause is not None, ( + "the capture snippet's KeyError branch never ran, so nothing was recovered" + ) + assert MISSING_TABLE in cause, ( + f"the capture snippet in {document} no longer recovers the underlying database" + f" error; it produced {cause!r}" + ) + assert len(cause) <= CAUSE_CEILING + len(TRUNCATION_SUFFIX) + assert "Traceback (most recent call last)" not in cause, ( + "the recovered cause carries a traceback, which the reporting rule forbids" + ) + + +@pytest.mark.sqlite +def test_a_metric_error_and_a_data_failure_are_told_apart_by_the_result_payload( + warehouse: SqliteDatasource, +): + """``success is False`` alone cannot distinguish broken configuration from bad data.""" + batch = whole_table_batch(warehouse, "orders") + + errored = batch.validate(gx.expectations.ExpectColumnValuesToNotBeNull(column="nope")) + failed = batch.validate(gx.expectations.ExpectColumnValuesToNotBeNull(column="customer")) + passed = batch.validate(gx.expectations.ExpectColumnToExist(column="customer")) + + assert errored.success is False and not errored.result + assert failed.success is False and failed.result + # Both halves of the discriminator are load-bearing: a *passing* expectation also + # carries an empty payload, so emptiness alone would misclassify it. + assert passed.success is True and not passed.result + + def is_metric_error(result: ExpectationValidationResult) -> bool: + return result.success is False and not result.result + + assert [is_metric_error(each) for each in (errored, failed, passed)] == [ + True, + False, + False, + ] + + messages = {key: value["exception_message"] for key, value in errored.exception_info.items()} + assert messages, "a metric error no longer names its cause in exception_info" + assert all(isinstance(key, str) for key in messages), ( + "exception_info keys are no longer strings, so the documented iteration over" + " .items() is the only lookup that works" + ) + assert any( + message == 'Error: The column "nope" in BatchData does not exist.' + for message in messages.values() + ), f"the cause message changed shape: {messages}" + + +@pytest.mark.sqlite +def test_results_come_back_grouped_by_column_rather_than_in_the_order_added( + ephemeral_context: EphemeralDataContext, warehouse: SqliteDatasource +): + """Pairing results with inputs by position mislabels every finding once this bites.""" + batch = whole_table_batch(warehouse, "orders") + suite = ephemeral_context.suites.add(gx.ExpectationSuite(name="ordering")) + added = [ + gx.expectations.ExpectColumnValuesToNotBeNull(column="customer"), + gx.expectations.ExpectColumnMeanToBeBetween(column="amount", min_value=0, max_value=100), + gx.expectations.ExpectColumnValuesToBeUnique(column="customer"), + gx.expectations.ExpectColumnMaxToBeBetween(column="amount", min_value=0, max_value=100), + ] + for expectation in added: + suite.add_expectation(expectation) + + result = batch.validate(suite) + returned = [configuration_of(each).kwargs["column"] for each in result.results] + + assert returned == ["customer", "customer", "amount", "amount"], ( + "validation no longer groups a suite by column; the ordering rule in" + f" gx-configure-expectations/{ENTRY_DOCUMENT} describes this grouping" + ) + assert returned != ["customer", "amount", "customer", "amount"], ( + "results came back in the order they were added, so this suite no longer" + " demonstrates the trap it exists to demonstrate" + ) + assert len(result.results) == len(added), "every added expectation is still reported" + + +@pytest.mark.sqlite +def test_checks_without_a_column_are_grouped_on_their_own( + ephemeral_context: EphemeralDataContext, warehouse: SqliteDatasource +): + """Table-level checks form a group of their own, placed where it first appears.""" + batch = whole_table_batch(warehouse, "orders") + suite = ephemeral_context.suites.add(gx.ExpectationSuite(name="table_level")) + for expectation in ( + gx.expectations.ExpectColumnValuesToNotBeNull(column="customer"), + gx.expectations.ExpectTableRowCountToBeBetween(min_value=1), + gx.expectations.ExpectColumnMeanToBeBetween(column="amount", min_value=0, max_value=100), + gx.expectations.ExpectColumnValuesToBeUnique(column="customer"), + ): + suite.add_expectation(expectation) + + result = batch.validate(suite) + returned = [configuration_of(each).kwargs.get("column", "") for each in result.results] + + assert returned == ["customer", "customer", "", "amount"], ( + "a table-level check no longer forms its own group between the column groups;" + f" the ordering rule in gx-configure-expectations/{ENTRY_DOCUMENT} says it does" + ) + + +@pytest.mark.sqlite +def test_an_expectation_whose_metric_errored_is_moved_to_the_front( + ephemeral_context: EphemeralDataContext, warehouse: SqliteDatasource +): + batch = whole_table_batch(warehouse, "orders") + suite = ephemeral_context.suites.add(gx.ExpectationSuite(name="hoisted")) + for expectation in ( + gx.expectations.ExpectColumnToExist(column="customer"), + gx.expectations.ExpectColumnValuesToNotBeNull(column="customer"), + gx.expectations.ExpectColumnValuesToBeBetween(column="amount", min_value=0), + gx.expectations.ExpectColumnMeanToBeBetween(column="nope", min_value=0, max_value=100), + ): + suite.add_expectation(expectation) + + result = batch.validate(suite) + + assert configuration_of(result.results[0]).type == "expect_column_mean_to_be_between", ( + "the expectation whose metric errored was added last and is no longer reported" + f" first; gx-configure-expectations/{ENTRY_DOCUMENT} says it is" + ) + assert not result.results[0].result, "the hoisted entry should carry no payload" + + +@pytest.mark.sqlite +def test_an_empty_table_produces_results_rather_than_errors(warehouse: SqliteDatasource): + """Degenerate data is an ordinary outcome; reporting it as an error is wrong.""" + batch = whole_table_batch(warehouse, "empty_orders") + + probe = batch.head(n_rows=5) + assert list(probe.data.columns) == list(ORDERS_COLUMNS) + assert len(probe.data) == 0 + + not_null = batch.validate(gx.expectations.ExpectColumnValuesToNotBeNull(column="customer")) + mean = batch.validate( + gx.expectations.ExpectColumnMeanToBeBetween(column="amount", min_value=0, max_value=100) + ) + row_count = batch.validate(gx.expectations.ExpectTableRowCountToBeBetween(min_value=1)) + + assert not_null.success is True + assert not_null.result["element_count"] == 0 + assert mean.success is False + assert mean.result == {"observed_value": None} + assert row_count.success is False + assert row_count.result == {"observed_value": 0} + # ``observed_value: None`` is a *populated* payload, so the discriminator reads these + # as data failures rather than as metric errors -- which is the correct reading. + assert mean.result and row_count.result + + +@pytest.mark.sqlite +def test_an_all_null_column_produces_results_rather_than_errors(warehouse: SqliteDatasource): + batch = whole_table_batch(warehouse, "all_null_orders") + + not_null = batch.validate(gx.expectations.ExpectColumnValuesToNotBeNull(column="customer")) + between = batch.validate( + gx.expectations.ExpectColumnValuesToBeBetween(column="amount", min_value=0, max_value=100) + ) + mean = batch.validate( + gx.expectations.ExpectColumnMeanToBeBetween(column="amount", min_value=0, max_value=100) + ) + + assert not_null.success is False + assert not_null.result["unexpected_count"] == NULL_ROW_COUNT + assert not_null.result["unexpected_percent"] == 100.0 + # The counterintuitive one: nulls count as missing rather than as violations, so a + # range check over a column of nothing but nulls passes and is not reassurance. + assert between.success is True, ( + "a value range check over an all-null column no longer passes; the caveat in" + f" gx-configure-expectations/{ENTRY_DOCUMENT} depends on it doing so" + ) + assert between.result["missing_count"] == NULL_ROW_COUNT + assert between.result["unexpected_count"] == 0 + assert mean.success is False + assert mean.result == {"observed_value": None} + + +@pytest.mark.sqlite +def test_an_empty_window_fails_at_retrieval_before_any_probe_runs( + ephemeral_context: EphemeralDataContext, warehouse: SqliteDatasource, tmp_path: pathlib.Path +): + """An empty *window* is a different outcome from an empty collection, on both families.""" + sql_definition = warehouse.add_table_asset( + name="orders", table_name="orders" + ).add_batch_definition_monthly(name="by_month", column="ordered_at") + + files = tmp_path / "sales" + files.mkdir() + (files / "sales_2024-02.csv").write_text("customer,amount\nalice,1.0\n", encoding="utf-8") + file_definition = ( + ephemeral_context.data_sources.add_or_update_pandas_filesystem( + name="sales_files", base_directory=files + ) + .add_csv_asset(name="monthly_sales") + .add_batch_definition_monthly( + name="by_month", regex=r"sales_(?P\d{4})-(?P\d{2})\.csv" + ) + ) + + with pytest.raises(NoAvailableBatchesError): + sql_definition.get_batch(batch_parameters={"year": 1999, "month": 1}) + with pytest.raises(NoAvailableBatchesError): + file_definition.get_batch(batch_parameters={"year": "1999", "month": "01"}) + + # The same definitions do produce a batch for a window that exists, so the failures + # above are about the window rather than about a broken configuration. + assert sql_definition.get_batch(batch_parameters={"year": 2024, "month": 3}) is not None + assert file_definition.get_batch(batch_parameters={"year": "2024", "month": "02"}) is not None + + +@pytest.mark.sqlite +def test_batch_parameter_types_differ_between_file_and_sql_assets( + ephemeral_context: EphemeralDataContext, warehouse: SqliteDatasource, tmp_path: pathlib.Path +): + """File-based definitions match on strings; SQL definitions partition on integers.""" + files = tmp_path / "sales" + files.mkdir() + (files / "sales_2024-02.csv").write_text("customer,amount\nalice,1.0\n", encoding="utf-8") + file_definition = ( + ephemeral_context.data_sources.add_or_update_pandas_filesystem( + name="sales_files", base_directory=files + ) + .add_csv_asset(name="monthly_sales") + .add_batch_definition_monthly( + name="by_month", regex=r"sales_(?P\d{4})-(?P\d{2})\.csv" + ) + ) + sql_definition = warehouse.add_table_asset( + name="orders", table_name="orders" + ).add_batch_definition_monthly(name="by_month", column="ordered_at") + + with pytest.raises(InvalidBatchRequestError): + file_definition.get_batch(batch_parameters={"year": 2024, "month": 2}) + assert file_definition.get_batch(batch_parameters={"year": "2024", "month": "02"}) is not None + + assert sql_definition.get_batch(batch_parameters={"year": 2024, "month": 3}) is not None + # Strings against a SQL definition are not rejected -- they simply match nothing, + # which is why the two families cannot share one set of batch parameters. + with pytest.raises(NoAvailableBatchesError): + sql_definition.get_batch(batch_parameters={"year": "2024", "month": "03"}) + + +@pytest.mark.sqlite +def test_updating_a_data_source_drops_every_asset_on_it( + ephemeral_context: EphemeralDataContext, warehouse_path: pathlib.Path +): + """Why the data-source factory runs at most once per flow, and never as an update.""" + connection_string = f"sqlite:///{warehouse_path}" + datasource = ephemeral_context.data_sources.add_or_update_sqlite( + name="warehouse", connection_string=connection_string + ) + datasource.add_table_asset(name="orders", table_name="orders") + datasource.add_table_asset(name="empty_orders", table_name="empty_orders") + assert [asset.name for asset in ephemeral_context.data_sources.get("warehouse").assets] == [ + "orders", + "empty_orders", + ] + + ephemeral_context.data_sources.add_or_update_sqlite( + name="warehouse", connection_string=connection_string + ) + + assert [asset.name for asset in ephemeral_context.data_sources.get("warehouse").assets] == [], ( + "updating a data source no longer drops its assets; the reuse-first rule in" + f" {CANONICAL_SKILL}/{ENTRY_DOCUMENT} is written around it doing so" + ) + + +@pytest.mark.sqlite +def test_the_flow_snippet_reuses_a_data_source_rather_than_replacing_it( + ephemeral_context: EphemeralDataContext, + warehouse_path: pathlib.Path, + monkeypatch: pytest.MonkeyPatch, +): + """The shipped configure snippet must fetch before it creates, not just say so. + + The test above pins the destructive behavior as a fact about the library. This one + pins the guidance's response to it as a property of the snippet an agent copies: + run against a session that already holds the data source, the snippet has to leave + the assets already on it alone. Rewriting its fetch-first branch into a plain + ``add_or_update_`` call satisfies every other check in this file while + silently destroying work the user did not ask it to touch, so nothing else here + would notice. + """ + monkeypatch.setenv("WAREHOUSE_PATH", str(warehouse_path)) + seeded = ephemeral_context.data_sources.add_or_update_sqlite( + name="warehouse", connection_string=f"sqlite:///{warehouse_path}" + ) + seeded.add_table_asset(name="empty_orders", table_name="empty_orders") + assert [asset.name for asset in ephemeral_context.data_sources.get("warehouse").assets] == [ + "empty_orders" + ], "the sibling asset this test is about was not seeded" + + snippet = sole_block_containing( + SKILLS_ROOT / CANONICAL_SKILL / ENTRY_DOCUMENT, + "DATASOURCE_NAME, ASSET_NAME, BATCH_DEFINITION_NAME", + ) + assert EXECUTABLE_TAG in snippet.tags, ( + f"{snippet.identifier} is no longer part of the runnable sequence, so this" + " test and the end-to-end run have drifted apart" + ) + namespace: dict[str, Any] = {"context": ephemeral_context} + exec(compile(snippet.source, snippet.identifier, "exec"), namespace) + + surviving = [asset.name for asset in ephemeral_context.data_sources.get("warehouse").assets] + assert "empty_orders" in surviving, ( + f"the configure snippet in {CANONICAL_SKILL}/{ENTRY_DOCUMENT} destroyed an asset" + " that was already on the data source. It must fetch the data source and create" + " one only when absent -- calling add_or_update_ against a name that" + f" already exists replaces it wholesale. Assets left: {surviving}" + ) + assert "orders" in surviving, "the snippet did not add the asset it exists to add" + assert namespace["batch_definition"].name == "by_month" + + +@pytest.mark.sqlite +def test_assets_and_batch_definitions_refuse_a_duplicate_name(warehouse: SqliteDatasource): + """There is no update factory for either, so the flow has to fetch before it creates.""" + asset = warehouse.add_table_asset(name="orders", table_name="orders") + asset.add_batch_definition_whole_table(name="all_rows") + + assert not hasattr(warehouse, "add_or_update_table_asset") + assert not hasattr(asset, "add_or_update_batch_definition_whole_table") + + with pytest.raises(ValueError, match="already exists"): + warehouse.add_table_asset(name="orders", table_name="orders") + with pytest.raises(ValueError, match="already exists"): + asset.add_batch_definition_whole_table(name="all_rows") + + # Fetching the existing objects is the path the guidance takes instead, and both + # signal absence with a LookupError subclass. + assert warehouse.get_asset("orders") is not None + with pytest.raises(LookupError): + warehouse.get_asset("never_created") + with pytest.raises(LookupError): + asset.get_batch_definition("never_created") + + +@pytest.mark.sqlite +def test_updating_a_suite_replaces_it_instead_of_merging(ephemeral_context: EphemeralDataContext): + """A fresh suite under an existing name empties it, with no error and no warning.""" + suite = ephemeral_context.suites.add(gx.ExpectationSuite(name="orders_quality")) + for expectation in ( + gx.expectations.ExpectColumnToExist(column="customer"), + gx.expectations.ExpectColumnValuesToNotBeNull(column="customer"), + gx.expectations.ExpectColumnValuesToBeBetween(column="amount", min_value=0), + ): + suite.add_expectation(expectation) + assert len(ephemeral_context.suites.get("orders_quality").expectations) == 3 + + ephemeral_context.suites.add_or_update(gx.ExpectationSuite(name="orders_quality")) + + assert len(ephemeral_context.suites.get("orders_quality").expectations) == 0, ( + "updating a suite no longer discards its contents; the fetch-first rule in" + f" gx-configure-expectations/{ENTRY_DOCUMENT} is written around it doing so" + ) + + +@pytest.mark.sqlite +def test_adding_expectations_to_an_unregistered_suite_persists_nothing( + ephemeral_context: EphemeralDataContext, warehouse: SqliteDatasource +): + """The ordering rule exists because the wrong order fails silently, not loudly.""" + batch = whole_table_batch(warehouse, "orders") + unregistered = gx.ExpectationSuite(name="never_registered") + unregistered.add_expectation(gx.expectations.ExpectColumnValuesToNotBeNull(column="customer")) + + result = batch.validate(unregistered) + assert len(result.results) == 1, "validating against an unregistered suite still works" + + assert "never_registered" not in {suite.name for suite in ephemeral_context.suites.all()}, ( + "an unregistered suite is now stored anyway; the register-first rule in" + f" gx-configure-expectations/{ENTRY_DOCUMENT} would no longer be necessary" + )