feat: add enrich step that records agent-authored insights on the discover inventory

[matthewmoorcroft]

Intent

Add an enrich step to discovery, run after discover. An agent authors insights about the factory. Enrich checks them against the inventory and records them in metadata/agentic_insights.json. The library then rebuilds inventory.json by itself, from source_graphs.json plus that document under one added insights key, so a later step reads the source graph and the enrichment from one file. flowx itself runs no LLM: the agent authors the insights and passes nothing else, and the library validates them, records them and builds the inventory.

What changed

• models/insights.py, discovery_insights.py: the insights schema and its validator.
  • Insights cover the overview, the system recommendation, per-pipeline insights and pipeline relationships.
  • Every pipeline name must be a listed inventory pipeline, with at most one insight per pipeline. The one exception: a control relationship may name a pipeline discover left unlisted (filtered out with --pipeline, or with no activities) when its edge is a resolved control edge in the inventory.
  • A control relationship must match a real (from, to, via_task_key) lineage edge marked resolved: true. An inferred relationship must carry evidence and a confidence level.
  • Recommended patterns have a count cap, and a simplification pattern must declare its release_state.
  • Authored copies of library-owned keys are rejected.
  • authored_against (the source_graphs_sha256 the agent read) is optional. When given it must match, so insights written before discover ran again are refused; when absent, the library stamps the hash itself. The skill passes it by default.
  • When the inventory records a source_graphs_sha256, the saved source_graphs.json must still be a usable object matching that hash, its source must equal the inventory's source, and source_dir must be a string. With source_graphs.json saved, an inventory missing its hash is refused.
  • Only string names and edge fields count, and a malformed inventory shape is treated as unknown rather than crashing.
  • Every violation is reported in one pass, and nothing is written.
• Recording:
  • agentic_insights.json is stamped with the schema version and with inventory, source-graph and insights (agentic_insights_sha256) hashes. authored_against is checked but not stored.
  • When the inventory records a graphs hash, its deterministic part is projected again from source_graphs.json with the shared build_source_inventory (project_inventory). Only source_dir and the hash come from inventory.json, plus a per-source rule for listing empty pipelines. The result is byte-identical to what discover writes, and the insights are validated against it. Inventories without a graphs hash (Airflow today) keep reusing their existing deterministic part.
  • Re-running with the same insights rewrites the same bytes.
  • discovery_serde.canonical_sha256 is now public so the two modules share one hash.
• Writing both files:
  • Only one enrich writes an output directory at a time. It holds metadata/.enrich.lock from reading the inventory until both files are replaced, and a second call made meanwhile gets a violation and writes nothing.
  • Discover does not wait for that lock; it clears metadata/, the lock with it. So before writing its temp files and again right before the replaces, enrich checks that its own lock and the inventory.json bytes it read are unchanged; if discover ran, it writes nothing and reports "discover ran during this enrich; prepare and enrich again".
  • If replacing the inventory fails, the insights file is put back, by an atomic replace, to the block the unchanged inventory holds, so the live file is never half-written and the pair always matches.
  • Two gaps remain. First, a process killed or interrupted between the two replaces, or a rollback that itself fails, can leave agentic_insights.json one write ahead of inventory.json. The lock then stays behind, so the next enrich refuses. Locally, delete metadata/.enrich.lock and run enrich again; on the hosted MCP server, re-run discover (which clears metadata/ and the lock), then prepare and enrich again. Second, a discover plus two or more enrich runs on one output folder within a few milliseconds can slip past the checks; because the temp file names are shared, one such case can leave a stale inventory.json while enrich reports ok: true. Sequential use is safe. Both gaps are stated in the module docstring and the skill, and per-run temp names are a tracked follow-up.
• Entry points: the adapter enrich subcommand (--output-dir, --insights-path, --out) and the MCP enrich command. On MCP, action: "prepare" returns metadata/inventory.json for a hosted agent that can't read the server's output_dir. It goes through the same size-bounded inline or volume/workspace delivery package uses for bundles. An inventory too large to return inline makes prepare fail with ok: false, naming the upload options. The default action: "apply" records insights given inline or as a path.
• Docs: the flowx-enrich skill (SKILL.md, plus insights.md for the schema). It is registered in plugin.json, the README and the install docs. flowx-discover continues into it unless a deterministic-only run was requested; the one-shot MCP migrate command runs without it.
• Carried from refactor: add source-neutral discovery graph contract and shared lineage core #84 and feat(adf): save source graphs with dataset lineage at ADF discover #94: feat(adf): save source graphs with dataset lineage at ADF discover #94's final head 337d2d6, which includes refactor: add source-neutral discovery graph contract and shared lineage core #84's final head, is merged in (da8f83a, then a45bb58); the conflicts were all in feat(adf): save source graphs with dataset lineage at ADF discover #94-owned ADF files and were resolved to feat(adf): save source graphs with dataset lineage at ADF discover #94's side. That history includes 880c202 and its revert 8e7d302, so uv.lock is unchanged.

How it was tested

• Unit tests cover the validator, the recording, the paired write and its failure paths, the CLI and the MCP command. The full unit suite (1900 passed; 1927 with the mcp extra, which CI does not install), ruff and mypy pass.
• ADF discover, convert and package on the fixture pipelines: output is byte-identical to feat(adf): save source graphs with dataset lineage at ADF discover #94's head 337d2d6, and Airflow output is unchanged. On those fixtures, and on an export with an empty pipeline, the inventory rebuilt from source_graphs.json is byte-identical to discover's, and enrich adds only the insights key.
• Driven live: discover, enrich, convert and package through the adapter CLI on the ADF fixtures and an Airflow DAG, and discover and enrich through the MCP stdio server.
• Codex cross-review, read-only, six times, plus one review of the carried merge. Every fix has a regression test that fails on the previous code.
  • Round 1 found two feat: add enrich step that records agent-authored insights on the discover inventory #95 issues, both fixed: insights could bind to a newer discover (now authored_against), and a failed write could leave the two files disagreeing (now a paired write with rollback).
  • Round 2 found three, all fixed in 278b57e, with 8fa9bac from the no-mistakes review:
    1. Concurrent enrich calls could interleave their writes. Writes now happen under the lock, and the crash window is stated as above.
    2. Duplicate insights for one pipeline passed validation. They are now a violation.
    3. A source_graphs.json that is not a JSON object crashed enrich. It is now a "not usable" violation. The review also found that malformed graph entries in an unhashed file still crashed enrich, so the recorded hash is now compared before any graph is read.
  • Round 3 found one, fixed in 3a8dd03: a failed inventory replace rolled the insights file back by truncating it in place, so a second disk error left it partial and the lock was dropped. It is now restored by atomic replace, and the lock is kept if the rollback fails. The no-mistakes review then found that a Ctrl-C landing just after a successful rename was rolled back as if the rename had failed; fb9b933 rolls back only on OSError and keeps the lock for an interrupt between the two replaces.
  • Round 4 found two, both fixed: a failed recovery run could roll the insights file back to a copy already ahead of the inventory (5d86088), and a hosted MCP agent had no way to read the inventory it authors against (ffe7229, the prepare action, narrowed to inventory.json in 7494460 so large ARM files can't crowd it out of the size bound).
  • Round 5 found one, fixed in 2fb4355: a whitespace-only edge_identity passed validation; it is now stripped before the non-empty check.
  • Round 6 applied the reviewer checklist from refactor: add source-neutral discovery graph contract and shared lineage core #84 and feat(adf): save source graphs with dataset lineage at ADF discover #94 to the full diff, alongside a manual pass. Six findings, fixed in 3f4c456: an inventory with its hash deleted was trusted, the unhashed source and source_dir drove the rebuild, numeric names were stringified into valid names, an unresolved control edge could be annotated, malformed inventory shapes crashed enrich, and the skill did not say the one-shot migrate skips enrich. The no-mistakes review then found that a resolved control edge into an unlisted pipeline could not be annotated, although the skill says it can; fixed in a354544.
  • Greg's round-3 review raised two points, both addressed: prepare reported success when the inventory was dropped by the inline size bound (750037e, 8951cc3), and a discover during an enrich could let old insights overwrite the new inventory (8951cc3; the remaining windows are documented in 117260a, 0fc77bf and 6ed0b4b). A process-id check in discover was tried first and replaced, because it could not tell a stale lock from a live one.
  • The review of the carried merge da8f83a, with the same checklist, found nothing: the resolution keeps every refactor: add source-neutral discovery graph contract and shared lineage core #84 and feat(adf): save source graphs with dataset lineage at ADF discover #94 change, and enrich agrees with the stricter carried rules, including the byte-identical rebuild.
• Aligned with the 8 Oct design note in 9a0ae6b: the rebuild from source_graphs.json, optional authored_against, and the rename from source_insights.json to agentic_insights.json. Each has a test that fails without it.

Merge order

Merge #84 first, then #94 retargeted to main, then this PR (retargeted to main once #94 has merged), then #96 and #97.

The enrich and discover docs point to the flowx-route skill, which lands with #97.

This pull request and its description were written by Isaac.

[ghanse]

The enrich design matches the description, and the validation, hash binding and rollback are well tested. Fix the false success on a truncated inventory before merging.

[ghanse]

Blocking: prepare returns ok: True even when inventory.json was dropped by the 2MB size cap, so the agent gets success with nothing to read. Fail when inventory.json is missing from the bundle (point to the output path instead), and add a test with an oversized inventory.

[ghanse]

This lock only stops two enrich runs from overlapping. discover removes metadata/ (and the lock with it), so a discover during an enrich can let the old insights overwrite the new inventory. Sequential use is fine. The risk is the recovery path that re-runs discover.

[matthewmoorcroft]

Thanks Greg, both round-3 comments are addressed.

• prepare false success (blocking): prepare now returns ok: false when inventory.json is too large to return inline, naming output_volume_path and output_workspace_path, with a test on an oversized inventory (750037e, error size taken from the copied file in 8951cc3).
• Discover during an enrich: enrich now respects discover rather than discover refusing, because discover is also the hosted recovery for a stale lock and a process-id check could not tell a stale lock from a live one. Under its lock, enrich checks its own lock token and the inventory.json bytes it read before the temp files and again right before the replaces, and if discover ran it writes nothing and reports "discover ran during this enrich; prepare and enrich again"; tests cover a discover after the read and after the temp files (8951cc3). Sequential use is safe. The remaining windows, between the first check and the temp files, between the final check and the replaces, and in releasing the lock, are a few file operations wide, need two or more concurrent enrich runs plus a discover on one output folder, and are not closed; they are stated in the module docstring and the enrich skill (117260a, 0fc77bf, 6ed0b4b). One of them can leave a stale inventory with ok: true, because the temp file names are shared between runs; that one is documented and tracked as a follow-up (per-run temp names).