Repository navigation
Ship the ADOP plugin, and fix nine engine defects found by running it - #34
Merged
Merged
Conversation
Add ADOP claude Code Plugin
Adop claude plugin json and marketplace json
Fixes the `Doc & prompt path references` job of the `Validate Prompts` workflow, which has been failing on main since 6ae2673. Last observed at run 36492380862, job 109163771271: README.md:160: broken repo path -> docs/getting-started-amazon-q.md This is the failure mode validate_docs_links.py was written for, and its own header describes it: the orchestrator `Read`s a path named in a markdown file, the read fails, and the run continues without the content — no exception, no log line. ## The file does not exist on the branch either The obvious fix is to point the link at the other branch, so I checked whether the target is there: git ls-tree -r origin/feat/amazon-q-support | grep getting-started -> docs/getting-started.md `docs/getting-started-amazon-q.md` exists nowhere in the repository. And the file that is there is not a substitute — `docs/getting-started.md` on that branch is a generic prompt guide with zero mentions of Amazon Q. A cross-branch URL was also tried and returns HTTP 404. So the sentence promises a document that has not been written. Rather than invent a target or link to something misleading, it now points at the branch, which does resolve (HTTP 200) and is where a reader would look anyway. `ASSERTED_MISSING` in validate_docs_links.py was considered and rejected: its semantics are inverted on purpose — it is for paths prose deliberately describes as *absent*, and the validator fails if one ever appears on disk. Using it here would have recorded "this file is intentionally missing", which is the opposite of the intent, and would then break when the Q doc is eventually written. ## Note for whoever owns the Amazon Q port The missing getting-started doc is a real content gap on feat/amazon-q-support (PR #22), not just a stale link. Worth writing there; this commit only stops the main branch claiming it already exists. ## Verified python3 scripts/validate_docs_links.py -> OK: all referenced paths resolve; all 5 regulations registered in 4 places exit 0
Partly fixes the `Bandit security analysis` job of the `Security Scan` workflow
(run 36492380812, job 109163770932). The job runs bandit twice and only the first
invocation is guarded by `|| true`, so the second one fails the build:
bandit -r shared/ workloads/ --severity-level medium --confidence-level medium
Seven findings. These four cannot occur given how the code is used; the remaining
three are real and handled separately in the next commit rather than silenced here.
## B701 x3 — jinja2 autoescape, HIGH
shared/codegen/renderer.py:98
shared/codegen/renderer.py:170
shared/codegen/slot_extractor.py:19
B701 exists to prevent XSS in HTML templates. This renderer has no HTML: every
template under shared/templates/ emits Python, SQL or YAML —
airflow_dag.py.j2 bronze_ingestion.py.j2 glue_job_config.yaml.j2
gold_aggregate.py.j2 iceberg_ddl.sql.j2 quality_check.py.j2
silver_transform.py.j2
Setting autoescape=True would HTML-escape quotes and ampersands inside generated
PySpark and SQL, so the "fix" bandit asks for would corrupt every artifact the
renderer produces. Escaping is the wrong operation for this output, not a
precaution being skipped.
The slot_extractor case is narrower still: that Environment only calls parse() to
feed jinja2.meta.find_undeclared_variables() and discover slot names. Nothing is
ever rendered through it, so autoescape has no effect either way.
## B310 x1 — urlopen scheme audit, MEDIUM
shared/utils/post_deployment_verifier.py:255
B310 warns that urlopen accepts file:/ and custom schemes. Here the scheme is a
literal in the line above:
url = f"https://{token_response['WebServerHostname']}/aws_mwaa/cli"
The hostname comes from an authenticated mwaa.create_cli_token() response. Nothing
user-supplied reaches the scheme, so the case B310 guards against cannot arise.
## Verified
bandit -r shared/ workloads/ --severity-level medium --confidence-level medium
before 7 findings (3 HIGH, 4 MEDIUM)
after 3 findings (0 HIGH, 3 MEDIUM — all B108, next commit)
pytest tests/unit/codegen tests/integration -q -> 76 passed
Each annotation names the specific check and the reason, so a future reader can
re-evaluate rather than trusting a bare `# nosec`.
Clears the last three bandit findings and finishes greening the `Bandit security
analysis` job:
B108 hardcoded_tmp_directory workloads/claims/dags/claims_pipeline_dag.py:35
B108 hardcoded_tmp_directory workloads/claims/dags/claims_pipeline_dag.py:48
B108 hardcoded_tmp_directory workloads/claims/dags/claims_pipeline_dag.py:49
## Why annotate rather than fix
An earlier version of this commit routed the paths through a `local_data_root`
Airflow Variable defaulting to tempfile.gettempdir(), which matches
.claude/rules/03-python-airflow.md ("zero hardcoded values"). It was reverted
after reviewing what it actually bought:
- On Linux tempfile.gettempdir() returns /tmp, so the effective path in CI and
on an MWAA scheduler was byte-identical. The symlink-preemption risk B108
points at was not removed where this code runs.
- On macOS the path moved to $TMPDIR/data-lake while workloads/claims/README.md
still documents /tmp/data-lake at lines 44, 47 and 48. Anyone running one step
by hand from the README and the next through the DAG would find nothing, with
no error — a new inconsistency created by the fix.
- It put a workload file into a CI-repair changeset, where it would be reviewed
by whoever is checking CI rather than whoever owns the workload.
So the honest position is that B108 is a real pattern but not a real risk in this
context, and saying so beats a change that looks like mitigation without being it.
The annotation records the reasoning and the condition under which it stops being
true.
## What these paths are
Local staging for the two `--local` demo tasks. Both tasks must agree on the same
location because the first writes the silver parquet the second reads, so a
per-run mkdtemp() would need the path passed between them through XCom. The DAG's
real Glue tasks do not touch these paths at all.
## Verified behaviour is unchanged
The diff is comments and `# nosec B108` suffixes only — the path literals are
byte-identical to origin/main:
"/tmp/data-lake/silver/claims/claims.parquet"
"/tmp/data-lake/silver/claims/claims.parquet"
"/tmp/data-lake/gold/claims/claims_analytical.parquet"
ast.parse(claims_pipeline_dag.py) -> syntax OK
workloads/claims/README.md -> 3 references, still correct
bandit -r shared/ workloads/ --severity-level medium --confidence-level medium \
--exclude '*/tests/*,*/test_*'
-> No issues identified. exit 0
The mcp-config-validator pre-commit hook matches any `.mcp*.json` and requires
every `uv run ... server.py` argument to start with `mcp-servers/`. A Claude Code
plugin vendors its own servers and reaches them through ${CLAUDE_PLUGIN_ROOT},
which expands to wherever the plugin was installed, so the hook blocked a
perfectly correct plugin config with four issues:
[BLOCK] server 'glue-athena' runs script
'${CLAUDE_PLUGIN_ROOT}/mcp-servers/glue-athena-server/server.py'
outside mcp-servers/ directory
CI runs `pre-commit run --all-files`, so this had to be fixed before a plugin
could ship a `.mcp.json` in this repo at all.
The prefix is stripped before the containment check, not trusted: it is the
portable spelling of a relative path, so removing it leaves the existing rule
doing exactly what it did. ${CLAUDE_PLUGIN_ROOT}/workloads/evil/server.py is
still rejected, and a test pins that so the fix cannot become an escape hatch.
Mutation-validated — reverting the one-line change fails the new acceptance
test and leaves the escape-hatch test green, which is the intended split.
This repo is where end users will find ADOP's agent tooling, and Claude Code is the first of several planned platform conversions — Kiro and Codex follow. A top-level adop-plugin/ gives the second and third variants nowhere consistent to land, so each would arrive as an unrelated sibling directory. plugins/<platform>/ gives them a shared parent and makes the Claude Code plugin one entry in a set rather than the only shape the repo knows about. Pure rename: git tracks all 24 files as moves, with no content change beyond the three paths the plugin's own README quoted about itself. The nested marketplace.json stays for now, so the plugin remains installable at the new path via `/plugin marketplace add ./plugins/claude-code` — the next commit moves that authority to the repository root, which is what a public user needs.
Claude Code discovers a repository's plugins from .claude-plugin/marketplace.json at the *repository root*. This repo had one only inside the plugin directory, so `/plugin marketplace add aws-samples/sample-Agentic-Ai-Data-Operations` — the way anyone who has not cloned the repo installs it — found nothing. Since this repo is how end users are meant to find ADOP, that was the gap that mattered most. The root manifest points at ./plugins/claude-code, which also means future variants register by adding an entry rather than by restructuring anything. The nested manifest is removed rather than kept: two marketplaces both named adop-marketplace, both declaring a plugin named adop, collide on install. The root one is authoritative. Marketplace `owner` becomes the repo rather than an individual — it identifies who operates the marketplace, and a public listing should not carry a personal address. Authorship stays where it belongs, in the plugin manifest. Verified end to end, not just against the schema: claude plugin validate . -> Validation passed claude plugin marketplace add ./ -> Successfully added claude plugin install adop@... -> Successfully installed claude plugin list -> Status: enabled <- the real check claude plugin details adop -> 6 agents, 4 skills, 1 hook MCP servers still read 0; they arrive with the vendored .mcp.json later.
`sagemaker-catalog:{` is missing the space after the colon, so the file is not
valid YAML at all:
yaml.scanner.ScannerError: mapping values are not allowed here
in "servers.yaml", line 12, column 47
This has been broken since 7cc25d8 and nothing caught it, because the pre-commit
gate was disabled by the hook-id typo fixed in 8dfe86b. `check-yaml` never got to
look at the file. Now that the gate runs, `validate-yaml-syntax` fails on it, so
this is the first real finding the restored gate produced on the plugin.
One character. The file now parses and yields 12 servers.
That count is itself wrong — tool-registry/servers.yaml has 13, and `core` (S3,
KMS, Secrets Manager) is absent from the plugin's copy, which leaves the
zone-scoped-kms and no-credentials-in-code invariants with no server to route to.
Not fixed here: the plugin's config/ is scheduled to be replaced by a vendored
copy of tool-registry/ rather than hand-maintained, and patching a file that is
about to be sourced differently would only hide the divergence. Recorded so the
drift harness has something to assert against.
The repo works by being cloned — open Claude Code inside it and .claude/ loads.
The plugin exists so the same capability works in the user's OWN repo, where none
of these files are present. A plugin is copied into
~/.claude/plugins/cache/<marketplace>/<plugin>/<version>/ at install time and
cannot see this repository, so it must CONTAIN the capability, not reference it.
That means copies, and copies drift silently. The predecessor of this plugin
shipped three skills that were condensations of .claude/rules/ files later
rewritten underneath them, and nothing noticed for weeks — it lived in a separate
repository with no access to the originals. Its own notes record the problem and
the reason it could not be solved there: "Vendored drift. mcp-servers/ and
codegen/ are copies of upstream. If the plugin lands inside the upstream repo, a
CI check comparing them would close this."
The plugin now lands inside the repo, so this closes it.
## TRANSLATION.yaml
Every file under plugins/claude-code/ declares a repo source and a category:
copy byte-identical; the Python library, schemas, templates, data
rewrite same content, paths retargeted to the plugin's vendored lib
transform a different primitive carries it (rules -> skills)
plugin-only no repo source, with a stated reason
untranslatable a repo capability with no plugin equivalent, recorded not hidden
Two statuses mark work in progress: `pending` (target exists, content not yet
translated) and `planned` (target not written yet). Both are reported rather than
failed, so the harness is useful during the port instead of only after it.
## tests/unit/test_plugin_translation.py
Nine tests, each mutation-verified to fail when the thing it guards breaks:
test_every_plugin_file_is_declared adding an undeclared plugin file fails
test_every_declared_source_exists renaming a repo source fails
test_every_rule_file_is_claimed... adding an unclaimed rule fails
test_copy_entries_are_byte_identical editing a vendored copy fails
test_rewrite_entries_keep_their_anchors dropping a named mechanism fails
test_no_declared_target_is_missing a stale declaration fails
test_untranslatable/plugin_only_... an unexplained gap fails
test_report_pending_translations prints the backlog; never fails
Verified by mutation, not assumed:
touch plugins/claude-code/sneaky.md -> test_every_plugin_file_is_declared FAILED
mv .claude/rules/09-glossary.md ... -> test_every_declared_source_exists FAILED
echo > .claude/rules/13-unclaimed.md -> test_every_rule_file_is_claimed... FAILED
## What it found immediately
Writing the manifest surfaced gaps rather than describing a tidy state — 22
pending translations, including:
- all six agents are the freehand-generation shape CLAUDE.md:137 forbids;
.claude/agents/ holds the spec-then-render versions
- commands/onboard-workflow.md is 130 lines against the repo's 546, missing the
Workflow tool, AskUserQuestion, the .discovery_complete marker and the renderer
- servers.yaml carries 12 servers where tool-registry has 13; `core` is absent,
leaving zone-scoped-kms and no-credentials-in-code with no server to route to
- invariants.yaml carries 10 where the repo has 11; iam-simulate-first is absent
- the HIPAA pack is a 21-line summary of a 267-line source
And the coverage guard refused the first draft of the manifest, which claimed only
2 of the 12 rule files. Ten were unaccounted for. That is the failure mode this
exists to prevent, caught on its own first run.
No content is translated in this commit. It establishes the contract so that every
commit after it is verified by construction rather than reviewed by eye.
Phase B1. The plugin is installed into a user's own repository, where shared/ and
contracts/ do not exist, so the deterministic renderer the agents depend on has to
travel with it. 27 files, byte-for-byte, plus one entry point.
## Byte-identical, with no edits to the vendored code
The layout under lib/ mirrors the repo's relative structure on purpose:
lib/shared/codegen/renderer.py
__file__.parent.parent.parent -> lib/ (PROJECT_ROOT)
TEMPLATES_DIR = PROJECT_ROOT/"shared"/"templates" -> lib/shared/templates
CONTRACTS_DIR = PROJECT_ROOT/"contracts" -> lib/contracts
renderer.py and spec_loader.py compute those paths by walking three parents up from
__file__, and every intra-package import is relative (`from .exceptions import`).
Mirroring the structure therefore makes both resolve correctly without touching a
line, so all 27 files are byte-identical to their sources and
test_plugin_translation.py can compare them with cmp.
That matters more than it looks. The predecessor plugin flattened codegen to its
root and rewrote the path resolution to read ADOP_TEMPLATES_DIR, which made
byte-comparison impossible — and an unverifiable copy is exactly how its three
skills silently went stale. Keeping the copies comparable is the whole point of
Phase A's harness.
## scripts/adop_render.py
In a checkout a sub-agent can `import shared.codegen` and call render(). In a
user's repo it cannot, so agents shell out to this instead — they hold Bash, so it
costs nothing and keeps the vendored library out of the user's import path.
Two things it gets right that were not obvious:
- shared/templates/VERSION is an env-style file with a key per template family,
not a single version. .claude/agents/dag-agent.md names DAG_TEMPLATE_VERSION
explicitly, so the CLI maps each template to its family key rather than
passing the file's first line as a version.
- render() requires run_started_at. It defaults to now, but --rendered-at pins
it, which is what makes a re-render byte-identical and therefore what the
drift validator relies on.
## Verified end to end, not by inspection
Rendering silver_transform from the same spec and the same timestamp, once through
the vendored lib/ and once through the repo's shared/:
cmp plugin_out.py repo_out.py -> IDENTICAL
Also confirmed: re-rendering the same spec twice is byte-identical; an invalid spec
is refused with the schema errors and writes nothing; a missing spec names the file
it wanted. The provenance header carries spec_hash, template_id, template_hash,
schema_version and rendered_at, which is what makes hand edits detectable.
## One repo fix this needed
.gitignore carried `lib/` from the standard Python build-output block. Unanchored,
that pattern matches a directory named lib at any depth, so the entire vendored
tree was silently untracked — `git status` showed nothing and `git add -A` added
nothing. Anchored `lib/` and `lib64/` to the repo root, which preserves the intent
(ignore setuptools output beside setup.py) without swallowing legitimate nested
directories. There is no root-level lib/ in this repo, so nothing else changes.
Worth knowing that the neighbouring patterns — build/, dist/, var/, parts/ — are
unanchored too and would bite the same way. Left alone here rather than widening a
vendoring commit into a .gitignore audit.
Phase B2. Completes the Python surface: 42 files now byte-checked, up from 27.
## The list is scraped, not hand-written
Rather than guess which modules matter, the closure is computed — every
`shared/**.py` path named in .claude/agents/, .claude/commands/, .claude/rules/ and
CLAUDE.md, then walked transitively through their imports. That caught two the
first pass had missed:
shared/metadata/semantic_reader.py named by ontology-agent, imported by nothing
in the earlier seed set
shared/utils/deterministic_yaml.py referenced as `deterministic_yaml.ordered_dump`,
so a `\.py$` pattern skipped it
It also confirmed three names in the contract resolve to nothing:
shared/templates/airflow_dag.py prose shorthand for airflow_dag.py.j2
shared/templates/quality_check.py same
shared/utils/quality_checks.py deliberately absent — it is in
validate_docs_links.py's ASSERTED_MISSING set,
the logic lives in rendered quality_check.py.j2
output, and sub-agents are told not to create it
The plugin preserves that absence rather than "fixing" it.
## boto3 is the real dependency boundary, so it is now a test
Importing all 17 vendored modules from the vendored tree in a clean interpreter,
with only jinja2/jsonschema/pyyaml present, gives 13 successes and 4 failures — and
the 4 are exactly the AWS ones:
shared.metadata.glue_fetcher
shared.metadata.lakeformation_fetcher
shared.utils.pii_detection_and_tagging
shared.utils.post_deployment_verifier
That matches the orchestrator/sub-agent split: profiling a source, tagging PII and
verifying a deployment are orchestrator work, and pyproject.toml already keeps
boto3 optional. So a user who installs the plugin only to render a pipeline does
not need the AWS SDK.
Two new test groups encode it. SUBAGENT_SAFE asserts the 13 reachable modules
import from plugins/claude-code/lib in a subprocess — a clean interpreter, so
sys.modules from the test session cannot mask a failure. That is what proves the
lib/shared/... layout works rather than merely that the bytes match: a flattened
vendoring would import fine and then fail to find a template at render time.
NEEDS_AWS_SDK documents the other four, so growth in the plugin's AWS surface shows
up in review.
## Noted, not fixed
.claude/agents/ontology-agent.md line 74 offers shared/metadata/glue_fetcher.py "as
an alternative to the MCP call". That module uses boto3 directly, and ontology-agent
holds Bash, so the suggested path reaches AWS without going through MCP — in
tension with the mcp-first BLOCK invariant and with rules/02, which grants that
agent exactly one read-only MCP tool and nothing else. It is John's file and a
repo-side question, so it is recorded here rather than changed.
## Verified
pytest tests/unit/test_plugin_translation.py 26 passed (was 9)
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 557 passed / 15 errors
(+17 passing, same 35/15)
airflow_dag rendered through the enlarged lib/ provenance header intact
check-yaml earned its place during this commit: rebuilding the manifest's copy:
section appended without a newline and produced a malformed file, and the hook
failed on it immediately rather than letting an unparseable manifest through. The
section is now regenerated from disk deterministically instead of appended to.
Phase B3. Replaces the plugin's compliance summaries with the control sets
themselves, and ships the catalog the IaC agent works from.
## The summaries were 7-9% of their sources
gdpr 23 -> 243 lines
ccpa 20 -> 235
hipaa 21 -> 267
sox 18 -> 266
pci-dss 21 -> 293
"Legal reviews a prompt file, not application code" is the strongest idea in this
plugin, and it does not survive the prompt file being a 21-line precis. The real
packs name the specific columns, masking methods, retention windows and audit
records each framework requires. The five summaries are deleted rather than kept
alongside — a stale copy next to the real thing is worse than no copy.
Also vendored: runbooks/devops-agent/iac-generator.md (1,076 lines), which
.claude/agents/iac-agent.md points to for "the resource catalog", and
regulation/README.md, the picker that maps a stated obligation to a token and
states nothing is applied unless the user selected it.
## Scope is smaller than planned, deliberately
The earlier plan said "vendor runbooks/ — 44 files". Checking what the agent
contract actually references gives 7:
regulation/{README,gdpr,ccpa,hipaa,sox,pci-dss}.md named by onboard-workflow.md
devops-agent/iac-generator.md named by iac-agent.md
The other 37 are not plugin material. environment-setup-agent/ (22 files) sets up
the ADOP platform itself — AgentCore gateway and runtime, 13 IAM policy documents —
which a plugin user does not do. The numbered data-onboarding runbooks
(01-route-check-existing, 04-enrich, 05-govern) are referenced by nothing outside
runbooks/. ontology-staging-agent.md is superseded by .claude/agents/ontology-agent.md.
Shipping them would have padded the plugin with content no agent reads.
## Paths mirror the repo so the agents' own references resolve
The packs land at runbooks/data-onboarding-agent/regulation/ inside the plugin, not
under skills/. .claude/commands/onboard-workflow.md line 351 tells the agent
"Reference runbooks/data-onboarding-agent/regulation/ for details", and mirroring
the path means that instruction works unchanged whether it runs in a checkout or
from the plugin. The data-compliance SKILL.md now points there instead of at the
deleted summaries.
## One file could not stay byte-identical
The doc-links gate caught it: iac-generator.md opens by pointing readers at the
live prompt via ../../.claude/agents/iac-agent.md, which resolves from
runbooks/devops-agent/ in the repo and resolves to nothing from
plugins/claude-code/runbooks/devops-agent/.
In the plugin the live prompt is a different file reached a different way, so two
references are retargeted — the link to agents/devops-agent.md, and
subagent_type "iac-agent" to "adop:devops-agent", since plugin agents are
namespaced. The 1,076-line catalog below them is untouched. The file moves from
copy to rewrite with those strings recorded as anchors, so the harness now fails if
either is lost; verified by reverting one and watching
test_rewrite_entries_keep_their_anchors fail.
This is the first case of vendored prose carrying a repo-relative reference, and it
will not be the last. It is why the rewrite category exists rather than forcing
everything to be byte-identical.
## Verified
pytest tests/unit/test_plugin_translation.py 26 passed, 48 copies byte-checked
scripts/validate_docs_links.py OK: all referenced paths resolve
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 557 passed / 15 errors
claude plugin validate . passed
grep for the deleted summaries no references remain
Phases B4 and B5, completing the COPY category: 57 byte-checked copies and 11
rewrites.
## .mcp.json is generated from the repo's, not hand-written
So the server set cannot drift. Three deliberate differences:
- vendored script paths gain ${CLAUDE_PLUGIN_ROOT}, which Claude Code expands to
the install directory
- AWS_REGION becomes ${AWS_REGION:-us-east-1} so a user's region wins over the
repo's demo default
- AWS_PROFILE: default is dropped from all 13 servers. The repo pins it because
it is a checkout used for demos; a plugin running in someone else's repository
must not override their profile selection, and omitting the key lets the
standard credential chain apply.
This is where 5a614c9 pays off — mcp_config_validator.py now passes on the plugin's
config, where before the fix it produced four [BLOCK] findings because it had never
seen ${CLAUDE_PLUGIN_ROOT}.
## A confirmed launch bug, fixed in the repo too
pii-detection was launched `--with boto3 --with mcp` while server.py:19 does
`from fastmcp import FastMCP`. The other three vendored servers all use
`--with fastmcp --with boto3`, so this one could never start. Fixed in the repo's
.mcp.json rather than only in the plugin's copy — patching the copy would have left
the defect live for anyone cloning, and made the rewrite diverge for a reason the
anchors could not explain.
## Three declared MCP packages are yanked from PyPI
Verified against pypi.org, independently reproducing a finding the predecessor
plugin made:
awslabs-core-mcp-server YANKED
awslabs-lambda-mcp-server YANKED
awslabs-cost-explorer-mcp-server YANKED
Live successors exist — awslabs-aws-api-mcp-server 1.5.5,
awslabs-lambda-tool-mcp-server 2.1.1,
awslabs-billing-cost-management-mcp-server 0.0.38 — and are deliberately NOT
substituted. TOOL_ROUTING.md routes intents to specific tool names, and a package
that starts but exposes a different tool surface is a worse failure than one that
does not start: the first misroutes silently, the second is visible at connect
time. Recorded in the manifest as a repo-side decision with the successor names.
Worth noting the irony: `core` is the server the plugin's hand-maintained
servers.yaml was missing, and restoring it restores a dead entry. The fix is
upstream, not here.
## tool-registry replaces hand-maintained drift
servers.yaml 12 -> 13 servers restores `core`, which zone-scoped-kms and
no-credentials-in-code had no server to route to
invariants.yaml 10 -> 11 rules restores iam-simulate-first (BLOCK), which
TOOL_ROUTING routed an intent for and
standards.md required, while the invariant
enforcing it was absent — the config
contradicted itself
TOOL_ROUTING.md 34 -> 506 lines 5 intents -> 8; Bronze ingestion, the Silver
and Gold transforms and lineage had no
routing at all and fell back on model priors
The three hand-maintained copies are deleted and decision-engine/SKILL.md now
points at the vendored paths, with config/standards.md called out as the one file
meant to be edited in place — it is the deliberately-empty slot for an
organisation's own tech choices.
## Second and third cases of prose that cannot stay byte-identical
The doc-links gate caught TOOL_ROUTING.md's footer linking ./MCP_GUARDRAILS.md and
./SKILLS.md, neither of which the plugin ships. Both retargeted to absolute GitHub
URLs, verified HTTP 200, so a plugin user can still reach them — a relative link
would resolve to nothing, which is exactly what that gate exists to catch.
Left open for Phase C/D, and recorded in the manifest: SKILLS.md is a genuine
runtime dependency. .claude/agents/quality-agent.md and
.claude/rules/{03-python-airflow,05-yaml}.md cite it for config schemas and DAG
templates. Those citations have to be resolved when those files are translated —
either by vendoring the relevant sections or by pointing at the vendored templates
that now carry the same information. MCP_GUARDRAILS.md needs no such treatment: no
.claude/ file references it, so no agent reads it at runtime.
## Verified
pytest tests/unit/test_plugin_translation.py 26 passed
mcp_config_validator.py on the plugin config PASS
scripts/validate_docs_links.py OK: all referenced paths resolve
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 557 passed / 15 errors
claude plugin validate . passed
The anchor guard caught one of my own errors here: I recorded "Intent-to-Tool" as a
TOOL_ROUTING.md anchor and the heading is "Tool Selection for Agentic Data
Onboarding", so the test failed until the manifest matched the file.
test_rewrite_entries_keep_their_anchors skips entries marked pending or planned,
which is correct — their targets are not translated yet, so their anchors cannot be
present. The consequence was that an anchor typo in a pending entry sat undetected
until translation happened, and then failed for a reason that looked like the
translation's fault rather than the manifest's.
That is not hypothetical. I recorded "Intent-to-Tool" as a TOOL_ROUTING.md anchor
when the heading reads "Tool Selection for Agentic Data Onboarding", and only found
it because that entry happened not to be pending. Six anchors on
commands/onboard-workflow.md — Step 1, Step 2, Completion Gate, AskUserQuestion,
discovery_complete, Workflow — were never checked at all.
The new test verifies every anchor appears in the source or the target. Presence in
either is enough, because an anchor may legitimately be one the rewrite introduces:
.mcp.json's ${CLAUDE_PLUGIN_ROOT} paths and iac-generator.md's adop:devops-agent
exist only in the plugin copy.
Swept all 17 declared anchors against both files: 0 match nothing. So the
TOOL_ROUTING typo had no siblings — but it would have been caught at declaration
rather than by accident.
Verified the guard bites: replacing a real anchor with a fabricated one fails both
test_declared_anchors_exist_in_the_source and
test_rewrite_entries_keep_their_anchors.
Also corrected in the amended acadbb7: its message said "16 vendored modules ... 12
successes" in one section and "13 reachable modules" in another. The truth is 17 =
13 + 4; the 16/12 figures came from an ad-hoc probe that omitted
shared.codegen.slot_extractor, while the committed test lists all 17. The
substantive claim was right and the tests passed; the numbers disagreed with each
other, which would have made a reviewer doubt the verification.
## What this does
Adds skills/observability, skills/pipeline-conventions and skills/discovery-protocol,
translating all 12 files under .claude/rules/. Skills go 4 -> 7; always-on cost goes
~659 -> ~1,048 tokens, which is skill *descriptions* only — bodies load on invoke.
Rule -> skill mapping, now complete and enforced by
test_every_rule_file_is_claimed_by_some_target:
observability 07-error-logging, 11-shared-run-context, 10-ascii-stages
pipeline-conventions 03-python-airflow, 04-sql, 05-yaml, 06-quality-testing,
08-transformations, 09-glossary
discovery-protocol 00-zone-questions, 01-human-gate-examples
decision-engine 02-architecture (already existed; still pending rewrite)
## Why this had to come before the agents
The plan had Phase C (agents and commands) before Phase D (skills). That was
backwards, and checking rather than assuming showed why: .claude/agents/*.md cite
.claude/rules/ eight times — 11-shared-run-context.md seven times and
01-human-gate-examples.md once. Translating the agents first would have left eight
references pointing at files the plugin does not ship, which the doc-links gate
would reject, and the natural workaround — writing the skills in a hurry to satisfy
the links — is how the predecessor plugin ended up with thin condensations.
Verified the dependency runs one way. The only reverse reference is
02-architecture.md naming `.claude/agents/*.md` as a glob, which the plugin
satisfies with its own agents/ directory, so there is no cycle. Skills first, agents
next.
## 10-ascii-stages moved to observability, not pipeline-conventions
The manifest originally mapped it to conventions because it looks like a formatting
rule. Reading it, its actual purpose is that terminal output and trace_events.jsonl
stay byte-identical at width 70 — a tracing concern. It sits with the trace layers.
## Two fidelity losses, stated in the skills themselves rather than hidden
A skill is a weaker vehicle than a rule, in two specific ways, and both are written
into discovery-protocol's body rather than left for a user to discover:
- Rules are always in context in a checkout; a skill body loads on demand. So the
load-bearing part of the Phase 1 gate must also live in the command body, which
IS always loaded when /adop:onboard-workflow runs. Phase C2 does that.
- .claude/hooks/check-discovery-gate.sh independently denies writes until
discovery completes. The plugin installs no hook into the user's repository, so
that second line of defence does not exist. The skill says so.
The skill also carries the part of that hook's contract that still applies: its deny
message tells the reader to create `.discovery_complete`, and that instruction is
addressed to the orchestrator, not to a sub-agent. A sub-agent creating the marker
asserts a human conversation that never happened, so the skill tells sub-agents to
return status "blocked" instead.
## One divergence corrected on the way through
pipeline-conventions names all five quality dimensions — Completeness, Accuracy,
Consistency, Validity, Uniqueness — and says why that matters. The predecessor
plugin substituted its own set of five, dropping Accuracy and Consistency, which
makes quality scores incomparable between workloads: exactly the fragmentation ADOP
exists to prevent, occurring inside ADOP. 06-quality-testing.md is cited as the
authority so a future edit has somewhere to check against.
The skills also point at the vendored library rather than the repo layout — for
instance the logging modules now at lib/shared/{logging,utils}/, reachable because
scripts/adop_render.py puts lib/ on sys.path.
## Verified
pytest tests/unit/test_plugin_translation.py 27 passed
all 12 .claude/rules/ files claimed OK (checked individually)
pre-commit 17 pass / 1 fail (deferred drift)
claude plugin validate . passed
claude plugin install + list enabled
claude plugin details adop Skills (7), Agents (6),
MCP servers (13)
always-on cost ~1,048 tok
## What changed and why
The plugin's six agents are now derived from .claude/agents/ rather than being the
versions inherited from the v0.1.0 baseline. The two sets implement opposite
architectures:
baseline (206 ln total) "Generate the ETL scripts under workloads/<name>/scripts/"
.claude/agents (690 ln) build a spec, validate against a contract, call the renderer
CLAUDE.md:137 is unambiguous — "All artifacts under workloads/*/scripts/, dags/, sql/
MUST be produced by shared.codegen.renderer.render(). Free-form code generation is
forbidden." The baseline agents instruct exactly what that forbids. They predate
.claude/agents/ existing, so this is not a disagreement anyone made; it is the repo
formalising the rule afterwards and the plugin never catching up.
The repo's versions are also 3.3x the detail, wired to specific orchestrator steps,
and carry per-agent model routing plus the one sanctioned MCP exception.
## Names now match the repo
data-quality-agent -> quality-agent
orchestration-agent -> dag-agent
devops-agent -> iac-agent
Three of six previously differed for the same roles. The repo's names are also the
subagent_type values SKILLS.md documents in five places, so a reader who follows
TOOL_ROUTING.md through to SKILLS.md now finds names matching what the plugin
dispatches as adop:<name>. Keeping the old ones would have left a 3-of-6 mismatch
and a mapping no reviewer could check by eye.
Verified against the repo's own agent-definitions CI rules: all six have name,
description and tools; each name matches its filename stem; none holds Agent or
SendMessage; the only MCP tool is ontology-agent's read-only
mcp__glue-athena__get_table.
## What the translation had to change
Renderer instructions, in three agents, converted from the Python API to the CLI —
a plugin user has no importable shared.codegen:
python3 "${CLAUDE_PLUGIN_ROOT}/scripts/adop_render.py" \
--workload workloads/{name} --template <id> --out <path> --rendered-at <ts>
Two simplifications fall out of that. The agents no longer pass template_version:
shared/templates/VERSION is env-style with a key per template family, and the CLI
resolves the right one. And --rendered-at is now explicitly tied to
run/context.json#started_at, because pinning it is what makes a re-render
byte-identical and therefore what the drift validator depends on.
Rule references become skill references — eight of them, pointing at the skills
60c4600 created, named the way they are invoked (adop:observability,
adop:discovery-protocol) rather than by a path the plugin has no equivalent for.
Library paths gain the lib/ prefix, and metadata-agent's inline load_spec call now
puts lib/ on sys.path so it works outside a checkout.
## One place the translation is weaker, and says so
metadata-agent's boundaries section described .claude/hooks/check-discovery-gate.sh
denying writes until discovery completes. That hook does not exist for a plugin user
— the plugin installs nothing into their repository. Rather than delete the
paragraph or leave a promise the plugin cannot keep, it now states that the
constraint is the agent's to honour rather than the harness's to impose, and repeats
the part that still holds: never create a .discovery_complete marker to unblock
yourself, because only the orchestrator holds AskUserQuestion and a marker the agent
creates asserts a conversation that never happened.
## Two self-inflicted defects caught in the making
Recording them because both were invisible in the output and only showed up on
inspection:
- My first substitution pass produced `lib/lib/contracts/v1/...`: two overlapping
patterns, the second re-matching what the first had already prefixed. Regenerated
with a (?<!lib/) guard and asserted zero double-prefixes.
- Renaming devops-agent to iac-agent silently invalidated the anchors 3ab183c had
recorded for runbooks/devops-agent/iac-generator.md, which pointed at
agents/devops-agent.md and subagent_type "adop:devops-agent". The anchor test
caught it. Both the file and the manifest are corrected.
## Verified
pytest tests/unit/test_plugin_translation.py 27 passed
agent frontmatter, repo CI rules all 6 valid
scripts/validate_docs_links.py OK: all referenced paths resolve
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 558 passed / 15 errors
claude plugin install + list enabled
claude plugin details adop Agents (6): transformation, iac,
ontology, metadata, dag, quality
Completes Phase C. /adop:onboard-workflow goes from a 130-line prose sketch to 636
lines carrying the repo's actual orchestrator; /adop:devops-workflow from 48 to 397.
## What the baseline versions were missing
The 130-line version described five phases in prose. Measured against the repo's 546,
four mechanisms were absent entirely:
Workflow tool 0 refs -> the 241-line Dynamic Workflow script, 12 agent()
calls, parallel stages, phase tracking, model routing
AskUserQuestion absent from allowed-tools, so the command could not ask a
structured question while claiming a human-in-the-loop Phase 1
.discovery_complete never written, so the marker the repo's gate hook looks for
was never created
renderer no reference, so Phase 4 told sub-agents to write PySpark —
which CLAUDE.md:137 forbids
It also declared `allowed-tools: Task`. Task is not a tool; the subagent tool is Agent.
The fan-out could not have worked as written.
## What the translation retargeted
the renderer fence `from shared.codegen.renderer import render` -> two
scripts/adop_render.py invocations, since a user's repo has no
importable shared.codegen. Located by content rather than line
number after an exact-string replace silently failed on a
three-space indent I had assumed was five.
contracts/v1 (x4) -> lib/contracts/v1
shared/templates -> lib/shared/templates
.claude/agents/ -> ${CLAUDE_PLUGIN_ROOT}/agents/iac-agent.md, in
devops-workflow's script, which Reads the agent file as its
prompt rather than dispatching by name — a third dispatch
pattern in the same repo
command references 12 occurrences -> /adop: prefixed, with a (?<!adop:) guard
Verified the script came through unharmed: 32 balanced fences, and the javascript block
is 241 lines with braces 120/120 and parens 101/101.
## Two additions, both deliberate
**CLAUDE.md's STOP gate is repeated in the command body.** In a checkout that gate is in
CLAUDE.md — always in context for every turn — and `.claude/hooks/check-discovery-gate.sh`
independently denies writes until discovery completes. A plugin has neither: skill bodies
load on demand and the plugin installs no hook into the user's repository. The command
body is the only place the gate is guaranteed to be read, so the checklist and the eight
NEVER constraints live there, with the adop:discovery-protocol skill carrying the
zone-targeted question sets.
**A documented path for running one stage directly.** Six agents with a table of when
each is worth invoking alone — re-deriving a transform spec after a rule change,
regenerating a DAG after a schedule change — plus the constraint that makes it safe:
each expects run/context.json to exist, because that is where the human's Phase 1
answers live. So direct invocation is for re-running or extending a workload that has
been through discovery, never for starting one. Starting one without the gate is the
failure the command exists to prevent.
This is what makes the six agent files first-class rather than definitions the command
happens not to use.
## The repo disagreement the plugin now carries
rules/02 and SKILLS.md describe sub-agents "spawned by name via the Agent tool";
onboard-workflow.md inlines prompts into a Workflow script; devops-workflow.md Reads the
agent file as a prompt. Three patterns. All three arrived in ba97329 (2026-09-16), which
created the five agent files, added the "spawned by name" sentence to rules/02, and left
the command's 12 inline calls unchanged — so it reads as an intent the command was never
migrated to, not a doc that history overtook.
Translating faithfully rather than picking a side means the plugin tracks whatever the
repo settles on. Worth raising with the repo owner; recorded in TRANSLATION.yaml so it is
not lost.
## Verified
pytest tests/unit/test_plugin_translation.py 27 passed
code fences / JS balance 32 balanced, 120/120, 101/101
scripts/validate_docs_links.py OK: all referenced paths resolve
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 558 passed / 15 errors
claude plugin install + list enabled
claude plugin details adop 7 skills, 6 agents, 13 MCP servers,
~1,069 tok always-on
Phase D3. Hooks go from 1 to 3, which turns two of the plugin's most emphasised rules
from prose into enforcement — and does it without the plugin interfering with a
repository that already has its own.
## Why hooks are the one component that needed care
Plugin agents, commands and skills are namespaced: an agent arrives as
adop:metadata-agent, a command as /adop:onboard-workflow. They cannot shadow a
project's own. **Hooks carry no namespace** — every matching hook fires. Verified by
install: adding this plugin to an ADOP checkout took the hook count from 5 to 6,
purely additive.
That is the one way this plugin could affect a repository's own /onboard-workflow,
which is exactly what the repo owner asked us to avoid. So every plugin hook now
calls hooks/_stand_down.py and exits quietly when the project already ships the
equivalent file. Inside a checkout the project's hooks decide; outside one — a user's
own data repository, which is what the plugin is for — the plugin's hooks work.
Tested in both directions:
in this repo (.claude/hooks/ present) both hooks return {} stood down
in a scratch dir (no .claude/) both return deny enforcing
The guard looks for the specific counterpart file rather than merely a .claude/hooks/
directory, so a project with unrelated hooks of its own still gets the plugin's
protection.
## hooks/check_discovery_gate.py — the gate becomes machinery
A Python port of .claude/hooks/check-discovery-gate.sh with identical behaviour:
denies Write/Edit under workloads/{name}/{config,scripts,dags,sql}/ for a workload with
neither config/source.yaml nor a .discovery_complete marker.
Ported rather than vendored because the original shells out to jq twice, and a plugin
installed on an arbitrary machine cannot assume jq exists. A gate that silently fails
to run is worse than no gate — which is not hypothetical here: this repository's entire
pre-commit suite sat disabled for six months behind a one-character hook-id typo, and
nothing distinguished "passing" from "never ran".
Without this hook the Phase 1 gate is advisory prose in a command body. With it, an
attempt to generate before the human has answered is refused.
The deny message keeps the part that matters most: it tells the reader to create
.discovery_complete, and then says that instruction is addressed to the orchestrator,
the only party holding AskUserQuestion and therefore the only party that could have
asked. A marker created by a sub-agent asserts a conversation that never happened and
turns the guardrail into a no-op for every later write in that workload. It also states
that answers already in run/context.json are not a substitute, because the marker
records that the orchestrator verified them.
## hooks/enforce_template_codegen.py — stricter than its source, deliberately
The repo's version allows the write when ADOP_RENDERER_TOKEN is set:
if is_protected_path(file_path) and not token: # line 80
Anything able to export that variable can therefore write a hand-authored artifact,
including the agent the hook exists to constrain. This version has no bypass and does
not need one: the vendored renderer writes through _atomic_write() straight to disk,
not through the Write tool, so a legitimate render never reaches a PreToolUse hook.
Removing the exception costs nothing and closes the hole.
Verified the token no longer unlocks it.
The deny message names the next action rather than only the prohibition — the
adop_render.py invocation, the template that matches the directory being written, and
the reminder that MissingSlotError means extending the spec and never the template. It
also states that config/*.yaml is not protected, so an agent knows where specs go.
## Deliberately not shipped, with reasons
Three of the repo's five hooks are left out rather than ported badly:
check-logging.sh 85 lines, 9 jq invocations. A faithful port is real work and
the failure mode of a partial one is a gate that passes
without checking.
inject_run_context.py SessionStart, 173 lines. Recovers in-flight run state after a
compaction. Valuable, but quality-of-life rather than a
guarantee.
log_conversation.py PostToolUse on AskUserQuestion. The only one where a
double-fire would actively corrupt data — duplicate entries in
trace_events.jsonl — so it wants the stand-down guard plus
more thought about append-safety than a hooks commit should
carry.
Recorded here so the gap is a decision rather than an oversight.
## Verified
4 behaviours of enforce_template_codegen deny on scripts/, deny with the token set,
allow config/*.yaml, names the CLI+template
4 behaviours of check_discovery_gate deny for a new workload, allow when
source.yaml exists, allow when the marker
exists, allow outside workloads/
stand-down, both directions {} inside this repo, deny outside it
pytest tests/unit/test_plugin_translation 27 passed
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 558 passed / 15 errors
claude plugin list enabled
claude plugin details adop Hooks (2): PreToolUse, PostToolUse
## The README had gone stale, and nothing was watching
By the end of Phase D it still described the v0.1.0 baseline:
listed 2 skills 7 ship
named data-quality-agent renamed to quality-agent in C1
named orchestration-agent renamed to dag-agent
named devops-agent renamed to iac-agent
0 mentions of MCP servers 13 ship
0 mentions of the renderer 57 vendored files
A reader following it would have tried to invoke three agents that no longer exist. This
is the same failure mode the predecessor plugin had — its README listed 2 skills when 5
existed and gave the wrong install directory — and the same reason: nothing tested it.
## What the new README does differently
It leads with the problem the plugin solves rather than the feature list: without it you
clone ADOP and work inside the clone; with it, pipelines are generated into the repo you
are already in.
It tells the reader to check `claude plugin list`, and says why — `validate` checks the
manifest schema and `details` lists components read from disk, and both will happily
describe a plugin that failed to load. That trap cost the predecessor four commits to
learn.
It documents requirements honestly: jinja2/jsonschema/pyyaml for the renderer, boto3 only
for the four modules that call AWS, and that the plugin does not set AWS_PROFILE so the
user's own selection applies.
It shows a real provenance header rather than describing one, because that five-line block
is what makes "deterministic" checkable rather than a claim.
It states that three of the thirteen MCP servers cannot start, names them, names the live
successors, and explains why they are not substituted — tool routing maps intents to
specific tool names, so a package that starts while exposing a different surface fails
silently, which is worse than one that visibly does not connect.
## The two fidelity gaps are stated, not buried
Both are structural, and a user who hits them unwarned is owed better:
- Always-on guidance becomes on-demand. CLAUDE.md and .claude/rules/ are in context every
turn in a checkout, and path-scoped rules fire on a matching edit. A plugin has no
equivalent, so the load-bearing Phase 1 gate is repeated in the command body and the
rest is one invocation away rather than already present.
- Commit-time gating is opt-in. The repository's 18 pre-commit hooks can reject a commit;
plugin hooks intercept the agent's tool calls, not commits. So nothing here stops the
*user* committing a secret. The README says to copy .pre-commit-config.yaml and run
`pre-commit install` if they want that.
It also tells contributors to report bugs upstream rather than against the vendored copies,
because an edit here is reverted by the next re-vendoring and fails the drift check
meanwhile.
## Three new tests, so this cannot rot again
test_readme_names_every_skill_and_agent globs skills/ and agents/ and requires
each name to appear
test_readme_does_not_name_components_that_were_removed
catches the other half — a name left
behind after a rename
test_readme_states_both_fidelity_gaps keeps the two limitations documented
Mutation-verified: renaming an agent in the README fails the first, and rewording
"on-demand" fails the third.
## Verified
pytest tests/unit/test_plugin_translation.py 30 passed (was 27)
pre-commit 17 pass / 1 fail (deferred drift)
Completes Phase E. Six behavioural eval cases, the companion entry point they need, and
the fixtures to drive them.
## Why evals at all
The plugin's value proposition is consistency — every builder producing the same
architecture rather than a different one per engineer. That is a testable claim, and evals
are how it gets tested rather than asserted. Run under `--ablation with-without`, each case
executes twice, with the plugin and without, so the report shows whether installing it
actually changes the output.
## Every case targets a defect the baseline really had
Not hypotheticals — each maps to something found during this port:
discovery-gate the baseline shipped an explicit escape hatch from the
Phase 1 gate: "if the user said don't ask questions,
skip straight to Phase 3 with sensible defaults"
discovery-gate-under-pressure and so "use defaults" must reduce prompting depth
without skipping confirmation
quality-dimensions the baseline substituted its own five dimensions,
dropping Accuracy and Consistency, which makes scores
incomparable between workloads
spec-not-script it claimed determinism in three places while shipping no
renderer, and told sub-agents to write PySpark
drift-detection so a hand-edited artifact must be caught and repaired by
re-rendering, not by editing
hipaa-phi-controls its HIPAA pack was 21 lines standing in for 267
Every case carries a non-vacuity guard — verified: 3 to 6 graders each, all with at least
one requiring positive evidence. That is not decoration. A partial run of the predecessor's
suite aborted on an auth error and still invoked the graders on an empty transcript, scoring
0.33, because "no invented config values appear" is trivially true of nothing. A negative
grader alone cannot distinguish a clean run from no run.
## scripts/adop_check_drift.py
The drift-detection case needs it, and its absence was a real gap: the plugin shipped the
renderer without the check that makes rendering verifiable. It exposes the vendored
drift_validator, which in a checkout is reachable as
`python -m shared.codegen.drift_validator` — how .pre-commit-config.yaml and the Validate
Prompts workflow invoke it — but which a user's repository cannot import.
It also refuses an empty argument list rather than exiting 0. The underlying validator
treats no paths as success, which is the same vacuous-pass shape the repo's own CI job
works around by enumerating workload directories explicitly before calling it. Better to
refuse than to report a pass for checking nothing.
Verified end to end: render then check is clean and exits 0; appending one line to the
artifact reports DRIFT and exits 1; no arguments exits 2 with an explanation.
## Fixtures
The spec-not-script and drift-detection cases pre-seed a valid spec so they test the render
path rather than discovery. Vendored byte-identical from tests/fixtures/replay_workload/,
under evals/fixtures/ rather than a shipped tests/ directory — a plugin user has no reason
to receive a test tree, but the eval scaffold does need the specs.
## Two things the harness caught in this commit
- I declared one of six eval cases and the accounting test listed the other five. Exactly
what it is for.
- My manifest rebuild script re-added TOOL_ROUTING.md to the copy: section, where it had
correctly been a rewrite since B5 — its two footer links were retargeted to absolute
GitHub URLs because the plugin does not ship MCP_GUARDRAILS.md or SKILLS.md. The
byte-identity check failed immediately. A regression I introduced and would not have
noticed by eye.
## Verified
6 eval cases parse; 3-6 graders each; all carry a non-vacuity guard
adop_check_drift.py clean exit 0, drift exit 1, no-args exit 2
pytest tests/unit/test_plugin_translation.py 30 passed
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 561 passed / 15 errors
claude plugin validate . passed
Not run: the eval suite itself. `claude plugin eval` needs an environment this machine does
not provide — the cases are schema-verified and load cleanly but have never executed. That
is stated here rather than implied by their presence.
skills/decision-engine/SKILL.md and skills/data-compliance/SKILL.md were marked `status: pending` in Phase A, when they were still the untouched baseline files. Both were rewritten since — B5 pointed the decision-engine config table at the vendored tool-registry/ and TOOL_ROUTING.md, and B3 pointed the compliance framework table at the vendored regulation packs — but the markers were never cleared. Left stale they overstate the remaining work: the pending report listed three items where only one is real. The one that remains is config/standards.md, which is the deliberately-empty slot for an organisation's own tech choices and may be correct to leave as a template rather than derive from the repo. Noticed while gathering counts for a write-up; a stale marker in the manifest is the same class of defect as a stale README, and the manifest is what the drift harness reads.
An end-to-end run in a scratch repository — the plugin installed from its cache, no
ADOP checkout present — rendered every template from a valid spec. Two of seven failed
with MissingSlotError, and the two have different causes.
## glue_job_config was mapped to the wrong contract — my error
Its header asks for `tasks`:
{# required_slots: dataset_name, tasks #}
`tasks` is defined only by dag_spec, and the CLI's TEMPLATES table mapped the template
to silver. So it read config/silver.yaml, found no `tasks`, and could never render.
Remapped to the dag contract; it now renders 73 lines.
Worth noting what this reveals about the vendored templates: template_id does not imply
its spec. `glue_job_config` sounds like Glue configuration and therefore like a Silver
concern, but the slots it needs describe a task graph.
## iceberg_ddl is unrenderable upstream too — a repo defect
It asks for `tables`, and no contract under contracts/v1 defines that property. So no
valid spec can render it in this plugin *or* in a checkout. Independently reproduces a
finding the predecessor plugin made; its fix was to add a `tables` property to
silver_spec.schema.json, which the repository never adopted.
Left as it is rather than worked around. Adding the property to the plugin's vendored
copy would fix the symptom and break byte-identity with contracts/v1, which is the one
thing the drift harness guards. This belongs upstream.
## A test so neither recurs
test_render_cli_maps_each_template_to_a_contract_that_has_its_slots parses each
template's required_slots header and checks the contract the CLI maps it to actually
defines those properties. It asserts the *exact* set of known gaps rather than merely
tolerating failures, so it will start failing the moment `tables` is added upstream —
which is the reminder to drop the exemption.
Mutation-verified: reverting the glue_job_config mapping reproduces
"glue_job_config -> silver_spec is missing ['tasks']".
The test also caught itself being wrong. Its first version read only line 0 of each
template looking for required_slots, which is on line 2, so it found nothing and passed
vacuously — the same vacuous-pass shape that made the predecessor's eval graders score
0.33 on an empty transcript, and that the drift CLI now refuses on an empty argument
list. A test that finds nothing and a test that finds no problems look identical unless
one asserts what it expects to find.
## What else the end-to-end run confirmed
plugin installs to the cache 95 files, self-contained
hooks ENFORCE outside a checkout both deny; inside one they stand down
discovery gate opens on the marker deny -> allow
6 of 7 templates render 65-136 lines each
provenance headers 5/5 lines on every artifact
determinism re-render byte-identical
drift detection 5 clean exit 0; one hand edit -> exit 1
generated Python 5 of 5 parse
Glue conventions in the output GlueContext, getResolvedOptions, writeTo,
iceberg, logger all present
4 vendored MCP servers parse
pytest tests/unit/test_plugin_translation.py 31 passed
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 562 passed / 15 errors
Found by comparing the installed cache against the repo, which is the only check that
would have caught it: `ls` showed the file, `[ -f ]` said no, and `git status` was clean.
## What was wrong
mcp-servers/pii-detection-server/pii_detection_and_tagging.py is a symlink in the repo,
pointing at ../../shared/utils/pii_detection_and_tagging.py. That resolves from
mcp-servers/, but the plugin's vendored copy lives at lib/shared/utils/, so copying it
verbatim produced a link resolving to plugins/claude-code/shared/utils/ — which does not
exist. The plugin shipped a dangling symlink.
It also hid itself from the harness. `Path.is_file()` returns False for a broken symlink,
so the manifest rebuild silently excluded it and test_every_plugin_file_is_declared never
flagged it. One defect, invisible to three separate checks.
## Why it is dropped rather than fixed
The obvious repair — replace the link with a real file — would ship a second 552-line copy
of a module already vendored at lib/shared/utils/. So I looked at whether the plugin needs
it at all, and it does not. The file is vestigial upstream:
server.py 0 references to it
setup.sh links to workloads/us_mutual_funds_etf/scripts/governance/,
a workload that no longer exists
README.md documents a third path as the fix for an ImportError that
cannot occur, since nothing imports the module
the checked-in symlink points at a fourth location, shared/utils/
Four locations named for one file that nothing loads. Recorded as untranslatable with that
reasoning; anything needing the module can import it from lib/. Worth cleaning up upstream.
## The guard
test_plugin_contains_no_broken_or_escaping_symlinks fails on any symlink in the plugin that
either dangles or resolves outside the plugin root. Both matter for a distributed plugin: it
is copied into a cache at install time, so a relative link leaving the plugin root cannot be
relied on to survive.
Mutation-verified — adding `ln -s ../../nowhere/x.py` reproduces
"broken.py -> ../../nowhere/x.py (BROKEN)".
## Verified
pytest tests/unit/test_plugin_translation.py 32 passed
pre-commit 17 pass / 1 fail (deferred drift)
full suite 35 failed / 563 passed / 15 errors
## The gap
Nothing told a user whether their machine was ready. The command's Phase 0 "Health Check" is
an agent prompt asking the model to verify MCP connectivity — not a deterministic test — and
it BLOCKS when glue-athena, lakeformation or iam are down. So someone without `jsonschema`
installed, or without AWS credentials, found out several minutes into a run, from a failure
that looked like a spec problem rather than an install problem. The README stated
requirements in prose, which nobody reads before the first attempt.
## Two tiers, reported separately
The important thing this makes visible: **most of ADOP needs no AWS account and no test
data.**
LOCAL TIER python 3.10+, jinja2, jsonschema, pyyaml
-> discovery questions, specs validated against contracts, rendered
PySpark/SQL/DAG with provenance headers, drift checking
You can point the command at an s3:// path that does not exist and still get
a complete pipeline. The output is code; it runs against real data later.
AWS TIER boto3, uv/uvx, credentials via the standard chain
-> only source profiling (Phase 3) and deployment (Phase 5)
Exit 1 if the local tier is blocked, naming exactly what to install. A degraded AWS tier is
reported but never fails the check, because generating a pipeline without an AWS account is a
legitimate way to use this plugin.
## It also checks the things that are easy to get wrong
- the vendored library is intact — renderer, templates, contracts, the render CLI. A
partial install otherwise fails later in a way that reads as a spec error.
- credentials are actually usable, via sts get-caller-identity when boto3 is present,
rather than merely present on disk. On this machine ~/.aws exists but the call returns
ClientError, which is exactly the distinction worth surfacing.
- the three yanked MCP packages are named, so 10 of 13 connecting is expected rather than
alarming.
- **whether you are inside a project with its own .claude/hooks/**, where the plugin's
PreToolUse hooks stand down. Without that warning someone would test in the ADOP repo,
see the repo's gate fire, and conclude the plugin's gate works — when it had in fact
stood down.
## Verified against all three outcomes
system python3 (no jsonschema) exit 1, "LOCAL TIER BLOCKED ... jsonschema"
venv with the deps exit 0, "LOCAL TIER READY", AWS tier degraded on boto3
run inside this repo warns that the plugin's hooks will stand down here
pytest tests/unit/test_plugin_translation.py 32 passed
pre-commit 17 pass / 1 fail (deferred drift)
e176685 added SessionStart and two PreToolUse/PostToolUse hooks, so an installed copy now behaves differently rather than merely reading differently. Minor bump rather than patch for that reason. Updates are version-gated: `Update now` is a no-op when the version string has not moved, so a content change without this commit would leave anyone on 0.2.1 running two fewer gates with no indication of it.
The script already refused an empty argument list, with a docstring naming the reason: "An empty argument list would exit 0 without checking anything, which reads as a pass". The same shape survived one level in. adop_check_drift.py workloads/sales -> exit 0, zero bytes of output `verify_workload` scans scripts/, dags/ and sql/ for files carrying a provenance header. A workload with none — wrong path, artifacts never rendered, or artifacts sitting somewhere other than those three directories — produces an empty report, and an empty report is a pass. Byte-for-byte indistinguishable from "six artifacts, all clean". Found during end-to-end testing, not by a test: I rendered six artifacts into a clean directory, ran the checker, and it reported success without naming a single file. The success was real; it just was not about my artifacts. Now counts provenance-headed artifacts under the given paths first and exits 2 with an explanation when the count is zero. The count imports `parse_artifact_header` from the validator and mirrors its three directories and four suffixes, rather than re-implementing the rule, so the guard cannot come to disagree with what is inspected. ## Verified no args -> exit 2 "no paths given" path with no artifacts -> exit 2 "no rendered artifacts found" (was exit 0, silent) nonexistent path -> exit 2 one rendered artifact -> exit 0 and names the file artifact hand-edited (mutation) -> exit 1 "DRIFT: ..." edit reverted -> exit 0 The last two matter as much as the rest: a checker that cannot report drift would also pass every test above. `test_drift_cli_refuses_paths_that_contain_no_artifacts` covers it, as a subprocess because the guard lives in `__main__` and importing the helper would not prove the CLI wires it up. Mutation-verified — with the guard replaced by `if False`, the test fails; restored, it passes, and the file is byte-identical to before the mutation. pytest tests/unit/test_plugin_translation.py -> 44 passed, 4 skipped full suite failure set vs origin/main -> 48 = 48, diff empty Bumped to 0.3.1: an installed copy behaves differently, and updates are version-gated.
…a rule no template can run
Two halves of one defect, found by a live HIPAA run (session 349b40ee) whose adversarial
verifier returned 23 findings. Fourteen of them share a single cause.
## The cause
renderer.py:71 is the whole of the spec/template agreement check:
missing = declared - set(spec.keys())
if missing: raise MissingSlotError(template_id, missing)
That is template -> spec: a template needing a slot the spec lacks fails loudly, which is
why iceberg_ddl errors honestly. `tests/unit/codegen/test_template_schema_agreement.py`
tested the same direction three more ways.
Nobody checked spec -> template. A field the contract lets a human set, which no template
reads, produces no error, no warning and no log line. The spec validates, render()
succeeds, the provenance header is written, and the drift validator confirms the artifact
matches its spec. Every gate green while the field does nothing.
That is how the HIPAA masking gate came to be unenforced: the human marked
`phi_masking_applied` CRITICAL and blocking, it landed in `quality_spec.compliance_rules`,
and `quality_check.py.j2` has no loop over `compliance_rules`. The rendered pipeline
counts only rendered results, so the rule could never contribute to `critical_failures`.
Sixteen such fields exist across the five specs. Each is something the wizard asks the
human about:
compliance_rules -> the HIPAA masking gate (verifier C1, CRITICAL)
quarantine -> "how should duplicates be handled" (H1)
null_handling -> "how should nulls be handled" (M3)
sla -> "what SLA before alerting" (M1)
failure_handling -> "how should failures be handled" (M2)
dimensions,
output_tables -> "what Gold schema shape" (M7)
retention -> "7 years", on the only zone holding raw unhashed PHI (L6)
Asked, answered, recorded faithfully in a spec that validates — and no effect on the
generated code. Discovery is honest; delivery was unverified.
## Fix 1 — the other direction, as a ratchet
`TestNoContractFieldGoesUnread` in test_template_schema_agreement.py, where it completes
that file's stated purpose. It does not demand zero unread fields today; it demands the
set never grows. `KNOWN_UNREAD` lists all sixteen with the finding that justifies each,
and a second test fails if an entry becomes stale — a permanent exemption for a field that
no longer needs one is how a ratchet quietly stops ratcheting.
Slots come from the Jinja AST via `extract_slots`, not a regex. A word-boundary grep
counts a field named in a comment as used and undercounted by six, including
`gold.dimensions` and `bronze.retention` — both real findings.
`schema_version` is excluded structurally: render() consumes it for the provenance header,
verified against the `# schema_version:` line it writes.
## Fix 2 — an unimplementable rule must refuse, not score 1.0
`quality_check.py.j2`'s validity branch implements four of the nine check types the
contract allows. Its `{% else %}` arm passed the row count through as the valid count, so
`score = valid_count / total_rows` was exactly 1.0 for the other five.
A rule that never executed reported perfect compliance. Worse, `overall_score` is an
unweighted mean, so **adding a rule the template cannot implement raised the score.** In
the live run three `custom_sql` rules each contributed a fabricated 1.0, diluting a
genuine 0.875 failure to 1/26 of the total.
New `UnsupportedSpecValueError`, and an `unsupported()` global on the Jinja environment so
any template can refuse a value instead of inventing one. A template with no way to say
"I cannot do this" will always find a way to say "fine".
The two inline `jinja2.Environment(...)` constructions are now one `_make_env()`, so the
global is registered once rather than twice.
Failing the render is the lesser harm: a render that stops is a bug someone fixes; a
pipeline reporting 1.0 for a check that never ran is a bug someone trusts.
## Verified
Mutation-tested, each guard against the defect it exists for:
new unread contract property -> test_no_new_property_goes_unread FAILS
stale KNOWN_UNREAD entry -> test_ratchet_lists_nothing_that_is_now_read FAILS
{% else %} restored to the old arm -> 6 tests FAIL
That last one mattered: the first time I ran it, reverting Fix 2 left all 83 codegen tests
passing. Fix 2 had no regression test at all, and I would have shipped it unguarded.
`TestUnimplementableRulesAreRefusedNotScored` now covers all nine check types plus a
literal guard on the template source, so the pattern cannot return by another route.
Behaviour, all nine check types:
render: range, enum, regex, not_null (4, implemented)
refuse: custom_sql, format, length,
referential, unique (5, were silently scoring 1.0)
`workloads/claims_v2/scripts/quality/check_quality.py` re-rendered for the new template
hash, reusing its own `rendered_at` so the diff is the hash line alone. Its one `unique`
rule sits under `rules.uniqueness`, which has a real implementation, so nothing regressed.
drift validator, workloads/claims_v2 -> exit 0, all 5 OK
62 plugin lib copies -> byte-identical to source
full suite failure set vs origin/main -> 48 = 48, diff empty
…stop a PHI column shipping unmasked
Fix 3 of the run analysis. Each of these is an answer a human gave a live run
(session 349b40ee) that no enum could represent, so it was downgraded — and the
downgrade was recorded in a YAML description, where nothing enforces it.
All four changes are widening. Every spec valid before is valid now.
## 1 — A manual DAG had no valid spec (verifier C3)
schedule.cron {type: string, minLength: 9}, required, additionalProperties: false
"@once" is five characters and "None" is four, so `schedule_interval=None` — ordinary
Airflow — was inexpressible. `cron` now accepts null. `minLength` still applies to the
string form, `cron` is still required (null is a deliberate answer; absent is an
unanswered question), and a non-string non-null is still rejected.
`airflow_dag.py.j2` emitted `schedule_interval="{{ schedule.cron }}"` — always quoted. A
null surviving validation would have produced a string literal, which Airflow parses as a
cron expression before failing to load the DAG. Now branches to a bare `None`.
## 2 — hash_salted, because unsalted SHA-256 of a DOB is not one-way (C2)
A date of birth is ~40,000 candidates and a plan member-id space is often smaller, so the
digest reverses by enumeration. The enum offered only `hash`. The human answered
"SHA-256 + salt", the spec recorded `hash`, and the template emitted
`F.sha2(col, 256)` while the LF-Tags and the audit record both attested protection.
New `hash_salted` plus `salt_secret_id` — the secret ID, never the value (CLAUDE.md
security rule 1) — and a conditional making the ID required, because hash_salted without
a salt source would silently degrade to the exact thing it exists to prevent.
The template fetches the salt once per secret per run via `@lru_cache`, and raises if the
secret is empty rather than proceeding unsalted. Both the helper and its `lru_cache`
import are emitted only when a column asks for it; the first version shipped an unused
import into every silver artifact, including workloads with no masking at all.
## 3 — drop_exact_duplicates_only, with somewhere to put the conflicts (H1)
Two different things share the name "duplicate". A byte-identical row is redundant and
safe to drop. Two rows sharing a primary key but differing in content are a conflict, and
picking one by timestamp discards a value nobody reviewed.
The enum had only keep_latest / keep_first / none, so the human's answer became `none` —
no dedup at all. The CLM-100003 duplicate survived, uniqueness scored 0.875,
critical_failures hit 1, and the Silver gate would have blocked on every run forever.
Overall score 0.98, comfortably over the 0.80 threshold, so nothing anyone looks at
showed a problem.
`dropDuplicates()` then routes residual PK collisions to `quarantine.location`, appending
so a later run cannot erase an unreviewed conflict, and writing before the count is
logged so a write failure cannot read as "no conflicts". A new conditional requires a
quarantine block for this strategy, and requires `location` whenever quarantine is
enabled.
The two counts are logged separately: a row dropped as byte-identical is resolved, a row
quarantined is not, and one `duplicates_removed` figure would make an unreviewed conflict
look like a clean drop.
## 4 — phi_type MEMBER_IDENTIFIER (M8)
The human said MEMBER_IDENTIFIER; the enum had no such value; `source.yaml` recorded
ACCOUNT and the quality spec recorded NATIONAL_ID — a plan member id mis-tagged as a
government identifier, in the field that drives the Lake Formation grant policy.
## And one CRITICAL defect found while investigating a drift
`pii_masking`'s loop was `{% if hash %}{% elif redact %}{% elif mask_partial %}{% endif %}`
with **no else arm**, while the enum also allows `encrypt` and `tokenize`. Either fell
straight through and the column was written in clear text, with the spec, the LF-Tags and
the audit record all recording it as protected.
Worse than the unsalted-hash defect in the same template: an unsalted digest over a small
domain is weak; this is absent.
encrypt -> 0 masking statements emitted
tokenize -> 0 masking statements emitted
`workloads/customer_master/config/silver.yaml` specifies `encrypt` for `date_of_birth`
and `address`. Rendering that workload from its own spec of record produced unmasked PHI.
The artifact on disk redacts both, which is how it survived: it was rendered under an
older spec saying `redact`, the spec was later changed to `encrypt`, and nothing reported
that the new value did nothing. That is also why its drift test disagreed with the drift
validator — the test embeds its own copy of the spec, saying `redact`, so it could not
detect drift from the spec of record.
The loop now refuses via `unsupported()`. `customer_master`'s test is xfail(strict) with
the full explanation: resolving it is a decision for the workload owner, not a fix, and
re-rendering from config would have removed two PHI redactions.
## Verified
Every guard mutation-tested against the defect it exists for:
quote the manual schedule_interval -> 2 tests FAIL
narrow cron back to string-only -> FAILS (MISSED first time; see below)
drop the quarantine write -> FAILS
replace the salt with an empty literal -> FAILS
remove the salt-required contract guard -> FAILS
remove the quarantine-required guard -> FAILS
delete the pii_masking else arm -> 3 tests FAIL
The cron mutation initially MISSED: the render tests build a spec in memory and never
validate it, so narrowing the contract broke nothing. `TestManualScheduleIsExpressible`
now covers the contract side — six cases including that null is accepted, a truncated
cron is still rejected, and timezone is still required for a manual DAG.
Behaviour, exhaustively:
dedup keep_latest, keep_first, drop_exact_duplicates_only, none — all 4 render
masking hash, hash_salted, redact, mask_partial — render
encrypt, tokenize — refuse
schedule null -> schedule_interval=None string -> quoted, unchanged
125 codegen tests pass; every rendered variant compiles as Python
62 plugin lib copies byte-identical to source
claims_v2 re-rendered, drift validator exit 0, all 5 OK
full suite failure set vs origin/main -> 48 = 48, diff empty
The unread-field ratchet moved 16 -> 15: `silver.quarantine` is now read, so its
exemption became stale and the ratchet failed until the entry was deleted. That is the
mechanism working.
Fixes 1-3 change vendored contracts and templates, so an installed copy renders differently. A user on 0.3.1 would keep emitting an unsalted digest for a column whose spec says salted, and would still ship a PHI column unmasked for method=encrypt. Minor rather than patch: four new enum values and a new required-conditional field.
… deliver
Found by running `claude plugin eval` for the first time. The hipaa-phi-controls case
failed `phi-masked-in-silver` 3-0, and the reason was not the model: it read the runbook,
picked the method the runbook names, and the method does not work.
## Of the six methods regulation/hipaa.md recommended, three could not work
mask_email not in silver_spec's method enum -> spec validation fails
generalize not in the enum -> spec validation fails
tokenize in the enum, no template branch -> render refused (c08a21b);
before that, emitted NO masking
hash named for ssn and patient_name -> see below
mask_partial ok
redact ok
So a HIPAA workload following its own runbook got two spec-validation failures, one render
failure, and two inadequate controls. Only two of six recommendations worked.
## And plain `hash` was named for the two worst possible columns
An unsalted SHA-256 is a lookup, not a one-way function, once the input space is
enumerable:
ssn ~10^9 candidates reversible by rainbow table
dob ~40,000 reversible in seconds
member_id often <10^6 reversible
patient_name long tail, but common names are few
The table now uses `hash_salted` for every enumerable identifier, states the candidate
counts so the reason is visible, and adds a "Not available — do not name these" section so
the three broken values are recorded rather than silently dropped.
## The real lesson: I widened a contract and left the guidance behind
c08a21b added `hash_salted` to the enum and implemented it in the template. Before this
commit:
hash_salted in lib/contracts/v1/silver_spec.schema.json 5 mentions
hash_salted in agents/ skills/ commands/ runbooks/ README 0
The model cannot choose a value it is never told exists, and hipaa.md:123 was actively
recommending `hash (SHA-256)` for a patient name. **A new enum value nothing recommends is
very nearly no new enum value at all** — the same shape as the defects this branch has
been closing, this time in my own work, and only a real eval run surfaced it.
## Three new guards, so guidance cannot drift from the contract again
`test_recommended_masking_methods_exist_and_are_implemented` — every method a guidance doc
recommends must be in the enum AND have a template branch. Parses only the recommending
half of the doc, so the "Not available" section can name the broken values.
`test_guidance_does_not_recommend_plain_hash_for_an_enumerable_identifier` — plain `hash`
on a row mentioning ssn / dob / member_id fails.
`test_hash_salted_is_discoverable_by_the_model` — hash_salted must appear in guidance the
model reads, excluding `plugins/` because those are byte-identical vendored copies.
That exclusion came from mutation testing: the first version scanned every *.md, so
removing hash_salted from the repo runbook left the test green on the plugin's duplicate.
The guard was satisfied by the copy of the thing it was checking.
Mutation-verified:
restore `hash (SHA-256)` for ssn and `mask_email` -> 2 tests FAIL
remove hash_salted from the repo runbook only -> FAILS (MISSED before the fix)
## Also: running the eval suite polluted the plugin tree
`claude plugin eval` writes reports and aggregate JSON to
`plugins/claude-code/evals/results/<timestamp>/`, and three runs made
`test_every_plugin_file_is_declared` fail on six files nobody authored. Added to
`.gitignore` (anchored — an unanchored pattern once swallowed 27 vendored files) and to the
test's IGNORED tuple, because that test walks the filesystem and never consults git.
## Eval results, for the record
First real run: 4 no-bash cases, all executed, $0.94, 257s, claude 2.1.285.
discovery-gate 0.571 asks-rather-than-assumes FAIL (judge 2-1)
discovery-gate-under-pressure 0.833 responded-substantively FAIL (regex)
hipaa-phi-controls 0.700 phi-masked-in-silver FAIL (judge 3-0) <- this commit
quality-dimensions 0.833 states-the-zone-gates FAIL (judge 3-0)
One run per case, so the two LLM-graded 2-1 splits are noise-prone and not acted on here.
The 3-0 verdicts are not. `states-the-zone-gates` and `asks-rather-than-assumes` still need
looking at.
The two bash-tagged cases could not run: this machine's ~/.aws/config carries an
external-process credential helper for an unrelated profile, and the eval sandbox refuses a
Bash-granting run it cannot prove it has confined. Workaround documented in
ADOP-HOW-TO-RUN-TESTS.md.
## Verified
62 of 62 plugin copies byte-identical
full suite failure set vs origin/main -> 48 = 48, diff empty
`quality_check.py.j2` looped rules.completeness, rules.uniqueness and rules.validity. It had no loop over `compliance_rules.rules`. So in the live HIPAA run (session 349b40ee), phi_masking_applied, phi_masking_member_email, phi_masking_member_dob, phi_masking_member_id, phi_columns_lf_tagged and phi_not_in_logs were **never executed**. `critical_failures` counts only rendered results, so a rule the human explicitly designated CRITICAL and blocking could not contribute to it. Of three critical rules, two actually ran. The masking itself works — c08a21b added hash_salted and made encrypt/tokenize refuse rather than emit clear text. What was missing is the *verification that masking happened*. Until now the spec, the Lake Formation tags and the audit record all attested a control that nothing checked. Verifier finding C1, and the most serious of the 23. ## Rendered as a shared macro, not a second loop The dispatch is now `{% macro scored_rule(rule, dimension) %}`, called by both `rules.validity` and `compliance_rules.rules`. Deliberately one implementation. Two copies of one if/elif chain is how this template and its contract drifted apart in the first place: compliance_rules was never looped, and five of the contract's nine check_types fell through a bare else that passed the row count through as the valid count. A check type added later now reaches every caller. Compliance results carry `dimension: "compliance"` and land in `results`, so they reach `critical_failures` on the same footing as every other rule. ## Rules that are not properties of a dataframe now refuse This closes L2 as well. `phi_columns_lf_tagged` has params `lf_tags_present(...)` and `phi_not_in_logs` has `no_phi_literals_in_logs(...)` — neither is checkable in Spark, and both were marked `severity: critical` and blocking. Declaring an unexecutable check CRITICAL manufactures the appearance of an enforced minimum-necessary control. They now refuse the render, and the message says where such a check belongs: verified out of band via MCP, recorded as severity info, not as a blocking dataframe rule. ## Verified The refactor is output-identical apart from the hash: render from workloads/claims_v2/config/quality.yaml vs the committed artifact -> 2 changed lines, both template_hash, 0 non-hash differences Behaviour: a compliance rule reaches the script, with dimension "compliance" yes a spec with no compliance_rules renders exactly as before yes lf_tags_present / no_phi_literals_in_logs REFUSED a renderable digest-proof rule (regex ^[0-9a-f]{64}$) rendered And the part that matters — it must be able to *block*, not merely appear. The rendered scoring arithmetic was extracted and executed against a synthetic results list: critical_failures = sum(1 for r in results if not r["passed"] and r["severity"] == "critical") -> critical_failures = 1 for a failing phi_masking rule plus an assertion that the compliance appends precede that computation in the output, since a rule scored after the count is a rule that cannot affect the gate. Mutation-tested, all four caught: remove the compliance loop -> 5 tests fail score compliance into a separate list -> 6 tests fail duplicate the dispatch instead of the macro -> 3 tests fail restore the silent row-count fallthrough -> 8 tests fail One self-inflicted detour worth recording: the macro's own documentation comment quoted the literal string that `test_the_template_contains_no_unconditional_fallthrough` searches for, so the guard failed on my explanation of the bug rather than the bug. Fourth time this session that prose has tripped a literal guard. Reworded rather than weakening the guard. ratchet 15 -> 14 (quality.compliance_rules deleted; it failed as stale first) 62 of 62 plugin copies byte-identical claims_v2 re-rendered, drift exit 0 full suite failure set vs origin/main -> 48 = 48, diff empty
…s answer
`silver_transform.py.j2` filtered `F.col(pk).isNotNull()` unconditionally and never read
`null_handling`. A human answering "allow" or "quarantine" got silent drops either way.
That is an override of the one category of decision CLAUDE.md says the agent must never
make for the human — and `workloads/customer_master/config/silver.yaml` has been asking
for `quarantine` on customer_id, email and last_name while getting drops.
## The worse consequence
Dropping the rows first made the critical rule `not_null_<pk>` **unfalsifiable**. The
offending rows were gone before the rule could measure them, so it always scored 1.0. A
guardrail that cannot fail is indistinguishable from one that passes, which is the shape
of nearly every defect on this branch.
With `allow`, the rows survive and the rule can genuinely fail. That is the point.
## All four strategies now render
drop_row filter them out — what it always did, but only when asked
quarantine append to quarantine.location, then exclude
fill_default F.coalesce per column from fill_values
allow keep them, and report null_rows_retained
The checked set is `primary_key + null_handling.critical_columns`. The primary key is
implicitly critical: a row without one cannot be deduped or joined to anything. A spec with
no `null_handling` still gets drop_row over the primary key, so nothing pre-existing
changes behaviour.
## Two refusals rather than two silent guesses
`fill_default` with no `fill_values` entry for a checked column refuses. The contract
permits it — there is no conditional tying the two (gap G-1) — so the spec validates while
naming no default, and inventing one would put a value nobody chose into a column the human
called critical.
`quarantine` with no `quarantine.location` refuses. Otherwise the template must choose
between dropping a row the human asked to keep for review and failing the run.
## One hazard surfaced rather than hidden
Under `allow`, a null primary key partitions with every other null, so `keep_latest` or
`keep_first` would collapse unrelated rows into one. The generated script says so in a
comment and reports `null_rows_retained`, rather than the interaction being discovered from
a wrong row count later.
## Verified
Seven render cases, each compiling as Python:
allow keeps rows, reports null_rows_retained
drop_row filters
quarantine + location appends to the location, then filters
quarantine, no location REFUSED
fill_default, complete F.coalesce with the spec's values
fill_default, missing one REFUSED, naming the column (record_id, the PK)
null_handling absent drop_row over the PK — unchanged
Mutation-tested, all five caught:
ignore the strategy and always drop (the original bug) -> 7 tests fail
quarantine silently drops instead of writing -> 2 tests fail
remove the fill_values completeness guard -> 2 tests fail
remove the quarantine.location guard -> 3 tests fail
exclude the primary key from the checked set -> 5 tests fail
One harness error caught mid-check: a "quarantine with no location" case appeared to render,
because the fixture already carried a quarantine block that my override did not remove. The
guard was fine; the test was wrong. Re-tested with the block and the location each removed
explicitly — both refuse.
claims_v2 re-rendered: 10 lines, behaviour identical (drop_row over claim_id),
now driven by the spec instead of hardcoded
ratchet 14 -> 13
62 of 62 plugin copies byte-identical
drift validator exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
customer_master still refuses, for the unrelated `encrypt` masking gap — but its quarantine
answer would now be honoured once that is resolved.
…-nothing defects in the same template
`gold_aggregate.py.j2` had a duplicated measure dispatch and no else arm anywhere, so every
contract value it did not implement produced something that compiled. Four separate defects,
one shape.
## 1 — output_tables and dimensions were never read (M7)
`schema_type: star_schema` wrote a single flat fact table whatever the spec said. So
`dim_member` and `dim_provider` never existed, `dimensions[].scd_type` was inert,
member_email / member_dob / member_age_years never reached Gold, the semantic model's
"Member 1:many Claim" join had no dimension side, and the Lake Formation grants in
semantic.yaml targeted tables that were never created.
Now renders one table per `output_tables` entry of type "dimension", selected distinct on
its declared columns. A spec with no `output_tables` is unchanged.
## 2 — measures[].filter was never rendered, and this one was live
workloads/claims_v2/config/gold.yaml
denied_count = count(claim_id) WHERE claim_status = 'denied'
the committed artifact
F.count("claim_id").alias("denied_count")
**That table has been counting every claim as denied.** Not hypothetical — it is in the
repo, and the re-rendered artifact in this commit is the fix. It is also what I predicted
during the manual run for the allowed-to-billed ratio: a filtered aggregation silently
becoming unfiltered.
Filtered measures render `F.sum(F.when(F.expr(...), F.col(...)))`. Unfiltered ones keep the
bare column-name form they always emitted, so this does not rewrite every measure line in
every existing artifact for no behavioural gain.
## 3 — `percentile` vanished from the aggregation
In the contract's enum, no branch in either copy of the dispatch, no else arm. The measure
was simply dropped from `.agg()` and the render still compiled — a Gold table asked for a
p95 and got that column absent, with nothing reported. Now refuses, noting that gold_spec
has no parameter for *which* percentile, so the contract needs extending first.
## 4 — `iceberg_dynamodb` produced an empty script
One of the three options the discovery wizard offers and CLAUDE.md documents. No branch:
star_schema groupBy yes agg yes writeTo yes
flat_iceberg groupBy yes agg yes writeTo yes
iceberg_dynamodb groupBy NO agg NO writeTo NO <- and it compiled
A Gold zone that silently contained nothing. Now refuses.
## The dispatch is one macro now
It was two identical copies, one per schema_type branch, which is precisely why percentile
and `filter` went unnoticed in both. A value added later reaches every caller.
## SCD Type 2 refuses rather than downgrading
`gold_spec.dimensions` lists dimension *attributes* — {name, source_column, scd_type} — not
tables. My first guard matched a dimensions[] entry against an output_tables name, which are
different namespaces, so it found nothing to object to in
`workloads/customer_master`: that spec declares scd_type 2 on risk_profile,
employment_status and geographic_region while its only output table is `dim_customer`.
Corrected to check every attribute. Any Type 2 declaration refuses, because
`createOrReplace` is a Type 1 overwrite and **this is the one gap here that re-rendering
cannot repair** — the superseded rows were never captured.
customer_master's gold drift test is xfail(strict) with that explanation. It was previously
passing while the Gold zone contained no dimension table at all.
## Verified
Six render cases:
star, no output_tables unchanged (back-compat)
star + 2 dimension tables both built, written, dropDuplicates
scd_type 2 REFUSED
dimension with no columns REFUSED
iceberg_dynamodb REFUSED
flat_iceberg unchanged
Mutation-tested, all seven caught:
remove the dimension-table block -> 3 fail
let scd_type 2 emit Type 1 (name-match ver) -> 2 fail
let scd_type 2 emit Type 1 (attribute ver) -> 1 fail
drop measures[].filter support -> 2 fail
remove the aggregation else arm -> 2 fail
remove the schema_type else arm -> 2 fail
re-duplicate the measure dispatch -> 12 fail
Two self-inflicted detours: the first version rewrote 21 measure lines in existing artifacts
for no behavioural gain (fixed by keeping the bare column form when unfiltered), and it
leaked one blank line per measure through the macro's whitespace control. The remaining diff
is the hash plus the denied_count fix — nothing cosmetic.
Also caught: `{% if not dt.columns %}` raises under StrictUndefined rather than guarding, so
the check reads `is not defined or not`.
ratchet 13 -> 11
62 of 62 plugin copies byte-identical
claims_v2 re-rendered, drift exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
…ring a perfect 1.0
One `quality_check.py` was rendered from the Silver rule set, and `gold_quality` ran that
same file against the Gold fact with ZONE=gold.
The Gold fact's only non-measure columns are its grain, so every rule naming claim_id,
member_email, the three amounts, submit_date, claim_status or denial_reason raised
AnalysisException on column resolution — including both surviving critical rules. The task
crashed instead of scoring, so the 0.95 gate could not even fail in the form the gate
understands. Not unmet: **unreachable**.
## Rules now carry a zone
`quality_rule.zone` — enum silver | gold | both, default **both**, which is exactly the
behaviour that existed before. So no existing spec changes meaning and no existing artifact
is rewritten: verified at 0 non-hash diff lines against claims_v2.
The guard has to be a *runtime* `if` wrapping the filter construction, not just the
`results.append`. `F.col()` on a column the Gold fact lacks raises when the plan is built,
not when the job runs, so skipping the append is not enough — the filter must never be
constructed. The macro builds its body into `_body` and emits it either as-is or indented
into the guard, rather than keeping two copies at two indent levels.
## And the trap this fix could easily have created
The old scoring was:
if results:
overall_score = sum(...) / len(results)
else:
overall_score = 1.0
Tag every rule "silver" and the Gold gate would have had **zero results and scored a
perfect 1.0** — passing with no checks at all, which is a worse outcome than the crash it
replaced. Adding `zone` without fixing this would have converted a loud failure into a
silent pass.
It now raises, naming the zone and the table. Nothing to check is not the same as nothing
wrong — the same sentence as `adop_check_drift.py`, the quality `{% else %}` arm, the
masking `{% else %}` arm and `compliance_rules`. Fifth instance of one idea on this branch.
## Verified
Rendering:
zone "gold" -> `if zone == "gold":` with the body indented to 8
zone "silver"-> ditto
zone absent -> indent 4, no guard, byte-identical to before
mixed zones in one artifact -> compiles, both guards present
The scoring block was extracted from the rendered script and executed:
results = [] -> RuntimeError("No quality rules applied to zone 'gold' ...")
results = [{score:0.9}] -> overall_score = 0.9
Mutation-tested, all three caught:
restore overall_score = 1.0 for an empty set -> 2 tests fail
ignore rule.zone (all rules everywhere again) -> 1 test fails
guard the append but not the filter -> 1 test fails
That last mutation is the subtle one, and it is the reason the test asserts the *indentation*
of the filter line rather than merely that a guard string appears somewhere.
contract: zone accepted as silver/gold/both, rejected as "bronze"
62 of 62 plugin copies byte-identical
claims_v2 re-rendered, drift exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
Ratchet unchanged at 11: H4 is a template-and-contract fix, not an unread field.
…ared
`failure_handling` and `sla` were both in dag_spec and neither was rendered.
`workloads/claims_v2/config/dag.yaml` declares both — so that DAG has been running with no
alerting and no deadline.
## M2 — a task could fail and nobody was told
`default_args` carried only `email_on_failure: False`. `on_failure_callback: sns_alert` and
`notification_channel` were declarative. With `retries.count: 0`, which is what the human
asked for, there was not even a retry to notice.
sns_alert -> emits _notify_failure(context) publishing to SNS, wired into default_args
email -> email_on_failure True + email: [notification_channel]
none -> no callback
The callback never raises: an alerting failure must not mask the task failure it is
reporting. The topic is read from an Airflow Variable with notification_channel as the
default, so it is overridable per environment.
`slack_notify` and `pagerduty` refuse. Both need a connection or webhook the spec has
nowhere to name — notification_channel is a single string — and a stub that logs and returns
would look like an alerting path while delivering nothing. That is how this field came to be
unrendered in the first place.
`sns_alert` or `email` with no `notification_channel` refuses. Inventing a destination is how
an SNS ARN on account 000000000000 reached a rendered DAG (finding M5), where a failure
resolved to a nonexistent topic and was silent twice over.
## M1 — the SLA was unenforced on the two tasks that gate promotion
`sla.deadline_minutes` now renders `dagrun_timeout`, not Airflow's per-task `sla=`. Airflow
measures an SLA miss against a *scheduled* start, so with `schedule_interval=None` the
callback never fires and the deadline is silently inert. `dagrun_timeout` bounds the whole
run however it was triggered, which is what "the pipeline must complete within N minutes"
actually means.
The `quality_check` task branch emitted no `execution_timeout` while the `glue_job` and
`sensor` branches did. Since per-task `sla=` is deliberately unused, `execution_timeout` *is*
the per-task SLA mechanism — so the human's 120 minutes was unenforced on exactly the two
tasks that decide whether data is promoted. claims_v2 goes from 4 of 5 tasks bounded to 5 of 5.
## One guard I had to make less blunt
The first `sla.critical_tasks` guard refused any manual DAG with critical_tasks set. That was
wrong: it made `schedule_interval=None` unrenderable for exactly the combination the live run
chose, and it broke three of the C3 tests.
`execution_timeout` bounds a task however the run was triggered, so the control is
deliverable by a different mechanism. The guard now refuses only when a task named critical
has no `timeout_minutes` at all — then nothing bounds it and the declaration really is inert.
Better behaviour and a narrower refusal.
## Verified
Ten render combinations, each compiling:
baseline / sla.deadline_minutes / sns_alert+channel / email+channel / none
-> render, with dagrun_timeout, the callback and 5 of 5 execution_timeouts
sns_alert or email with no channel -> REFUSED
slack_notify, pagerduty -> REFUSED
critical task on a manual DAG with a timeout -> renders
critical task on a manual DAG with no timeout -> REFUSED
Mutation-tested, all eight caught:
drop dagrun_timeout -> 5 fail
define the callback but never wire it -> 5 fail
remove execution_timeout from the quality branch -> 5 fail
let the callback re-raise -> 5 fail
drop the notification_channel guard -> 6 fail
stub slack/pagerduty instead of refusing -> 6 fail
drop the critical_tasks guard (blunt version) -> 2 fail
drop the critical_tasks guard (refined version) -> 1 fail
"define the callback but never wire it" is the one worth having: a callback in the file and
not in default_args is exactly as silent as no callback, and it reads as done.
ratchet 11 -> 9 (target for this batch reached: 15 -> 9)
62 of 62 plugin copies byte-identical
claims_v2 re-rendered, drift exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
Plugin bumped to 0.5.0: five fixes in this batch change what an installed copy renders.
…ld crash
Found by a human's end-to-end run, not by this suite.
`DataFrameWriterV2.partitionedBy` has signature `(col: Column, *cols: Column)` — varargs of
Column. Three sites emitted:
.partitionedBy({{ iceberg_partition_spec | tojson }}) -> .partitionedBy(["service_date"])
which raises at write time. Both the Silver write and both Gold writes would fail on any
workload that partitions.
## Why it was never caught
workloads/claims_v2/config/silver.yaml iceberg_partition_spec: []
workloads/claims_v2/config/gold.yaml []
workloads/customer_master/config/silver.yaml []
workloads/customer_master/config/gold.yaml []
Every shipped workload is empty, which takes the `{% else %}` branch and emits no
partitionedBy at all. The only way to reach the defect is a non-empty spec, and nothing in
the repo has one — so the render tests, the drift validator and CI all passed over a call
that cannot execute.
## Not the one-character fix it looks like
Unpacking the list would still pass **strings** where Columns are required. Each entry now
renders as its own `F.col(...)`:
[] -> no partitionedBy emitted (unchanged)
["service_date"] -> .partitionedBy(F.col("service_date"))
["service_date","plan_type"] -> .partitionedBy(F.col("service_date"), F.col("plan_type"))
Identity partitioning only. An Iceberg transform — `days(ts)`, `bucket(16, id)`,
`truncate(4, zip)` — is a different call shape and now refuses: without the guard it would
render `F.col("days(ts)")`, a column of that literal name, and partitioning would fail on a
name nobody wrote. `silver_spec`'s pattern `^[a-z][a-z0-9_]*$` already forbids these;
`gold_spec`'s bare `type: string` does not, which is how one could reach it.
Extracted to a macro per template, since three copies of one expression is how the other
defects on this branch started.
## The guard is now comment-aware, and that is the real change
`test_no_template_passes_a_list_to_partitionedby` first failed on my own macro documentation,
which quotes the old broken line as an example of what it prevents.
That is the **fifth** time in this branch that a literal guard fired on prose describing the
defect it guards — the nosec annotations, the quality fallthrough, the schedule_interval
comment, the hash_salted discoverability check, and now this. Four of those I fixed by
rewording the comment. Rewording a fifth time would be treating the symptom: the guard cannot
tell code from a comment about code.
It now strips `{# ... #}` before scanning. Verified it still catches the real thing —
reverting either template to the JSON list fails 6 tests.
## Verified
13 tests across both templates and 3 partition-spec shapes
every rendered variant compiles as Python
Mutation-tested, all four caught:
silver back to a JSON list -> 6 fail
gold back to a JSON list -> 6 fail
emit bare strings instead of F.col() -> 4 fail
drop the bare-column-name guard -> 3 fail
2 claims_v2 artifacts re-rendered — hash only, since both specs are []
62 of 62 plugin copies byte-identical
drift validator exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
Bumped to 0.5.1.
## Still open from the same run, both needing a decision
- `getResolvedOptions(sys.argv, ["JOB_NAME"])` resolves exactly one argument, so **every**
`--ARG` the DAG passes is structurally unreadable. Broader than the reported
`--logical_date`: any date macro survives as a literal string, to_date() returns NULL, and
an age bucket built on it is silently inert. Either the script resolves a defined set, or
the DAG stops passing args no template reads.
- Silver is written with `createOrReplace` (line 307) and committed (line 348) **before**
`quality_check_silver` runs. So "quality gates block promotion" holds for promotion to
Gold, but nothing gates publication to Silver — including the masking gate added in
9736ae8. Ungated PHI is queryable regardless of the result. Fixing it means staging the
Silver write, gating, then promoting: a real change to the medallion flow.
…e zone-guarded
A live run checked my last four fixes rather than trusting them and found three still open.
It was right on all three. Two are gaps in fixes I shipped.
## H4 was half-applied
`rule.zone` exists so a Silver rule is not evaluated against the Gold fact, where `F.col()`
on an absent column raises AnalysisException when the plan is built. That guard lives in
`scored_rule`.
I routed `rules.validity` and `compliance_rules.rules` through the macro and left
`rules.completeness` and `rules.uniqueness` emitting their own inline copies. So a
completeness rule tagged `zone: gold` still crashed — the exact failure H4 was for.
**My H4 mutation test passed because it only exercised the validity path.** A mutation test
proves the guard covers the code you thought about, and nothing more.
## C1 was one fifth of the job
`.claude/rules/06-quality-testing.md` states "5 dimensions: Completeness, Accuracy,
Consistency, Validity, Uniqueness". The contract allows all five. The template looped three.
allowed by the contract : accuracy completeness consistency uniqueness validity
looped before this : completeness uniqueness validity
never rendered : accuracy, consistency
So `amount_sanity_paid_lte_allowed_lte_billed` (consistency) and `denied_claims_pay_zero`
(accuracy) were written by an agent, validated, and silently discarded. I fixed
`compliance_rules` and left its two siblings — which is how three of five came to look
like five.
Both dimensions naturally need `check_type: custom_sql` — a comparison across columns, or a
condition — so that branch was added in the same change.
## And adding custom_sql immediately regressed L2
F.expr("lf_tags_present(amount)")
`lf_tags_present` is not a Spark function, so this now rendered and would fail at **runtime**
as an undefined function, while the spec recorded the rule as critical and blocking. Strictly
worse than the refusal it replaced.
Caught by the C1 test failing, not by review. Out-of-band assertions are now refused by an
explicit list — `lf_tags_present`, `no_phi_literals_in_logs`, `tbac_grant_exists`,
`kms_key_rotated`, `cloudtrail_event_present` — because whether an arbitrary SQL string calls
a function Spark provides cannot be decided at render time. The list is additive.
real predicate "amount >= 0" -> renders
real conditional "status != 'denied' OR amt=0" -> renders
lf_tags_present(amount) -> REFUSED
no_phi_literals_in_logs() -> REFUSED
## The ratchet had the same blind spot as the code
`TestNoContractFieldGoesUnread` compares **top-level** spec properties against the Jinja AST.
`quality_spec.rules` *is* referenced, so it passed while two of its members were never
looped. A guard presented as a completeness check that only checks one level down.
`test_every_member_of_a_nested_container_is_rendered` now checks members of a nested
container, against both the contract and the documented five dimensions. Mutation-verified:
removing the consistency and accuracy loops fails it.
`test_no_dimension_loop_bypasses_the_macro` is the structural counterpart — every
`{% if rules.X %}` body must call `scored_rule`. That is the check that would have caught the
half-applied H4, where per-dimension tests only cover the dimensions someone remembered.
## Verified
existing artifact non-hash changes : 0 (exact back-compat — per-dimension variable
names preserved: non_null_count,
distinct_count, valid_count)
dimension loops bypassing the macro: NONE
contract dimensions never looped : NONE
zone guard present in all 5 : yes, measurement indented to 8 in each
Mutations, both caught:
remove the consistency + accuracy loops -> the nested-container ratchet fails
revert completeness to the inline form -> 2 tests fail (per-dimension + structural)
One test assertion of mine was too strict and I fixed the test, not the template: it demanded
the zone guard be the immediately preceding line, and custom_sql emits three explanatory
comments between guard and measurement. It now asserts the measurement is indented into a
guard and the nearest enclosing statement is the right one.
196 codegen tests pass
62 of 62 plugin copies byte-identical
claims_v2 re-rendered, drift exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
Bumped to 0.6.0: the quality artifact now evaluates two dimensions it previously dropped.
## Defect 3 from that run is still open, deliberately
`getResolvedOptions(sys.argv, ["JOB_NAME"])` resolves exactly one argument, so every `--ARG`
the DAG passes is structurally unreadable. Fixing it is a decision — either the script
resolves a defined set, or the DAG stops advertising args nothing reads — not a patch. Same
for the Silver-commits-before-the-gate finding.
…ard it
Another session fixed the `${ADOP_LOGICAL_DATE}` defect I had left open as needing a
decision. The fix is good and I have kept it as written. Two things were wrong with how it
landed, and it arrived with no test.
## The defect it fixes
`getResolvedOptions(sys.argv, ["JOB_NAME"])` resolved exactly one argument, so every `--ARG`
the DAG passed was structurally unreadable. For a pre-PII derived column written against the
run's logical date — so a backfill of an old partition recomputes the same value — the token
reached `F.expr()` as the literal string `'${ADOP_LOGICAL_DATE}'`.
With Glue's default ANSI mode off, `to_date()` returns NULL rather than raising. So the
derived column became NULL on every row, any age bucketing built on it was inert, **and the
table still committed**. Silently, not loudly.
## Why the fix is right
getResolvedOptions(sys.argv, ["JOB_NAME", "logical_date"]) # only when the token is used
if not re.fullmatch(r"\d{4}-\d{2}-\d{2}", logical_date): raise ValueError(...)
F.expr(expr.replace("${ADOP_LOGICAL_DATE}", logical_date))
Three judgements I would not have improved on:
- **It raises rather than defaulting.** The whole defect was a silent NULL; a crash is the
correct conversion.
- **It refuses `current_date()` explicitly, and says why in the message** — substituting wall
clock breaks backfill reproducibility, which is the reason the token exists at all.
- **It is conditional on demand.** A spec with no token, or a pre-PII column that does not
reference it, renders byte-identically to before: no `import re`, no extra argument, no
validation block. Verified at 0 non-hash diff lines against claims_v2.
## What was wrong with how it landed
It edited `plugins/claude-code/lib/shared/templates/` — the plugin's **vendored copy** — not
`shared/templates/`, the source those copies are taken from. So 2 of 62 copies drifted and
`test_copy_entries_are_byte_identical` would have failed. Ported to the source and re-synced;
62 of 62 identical again.
The `VERSION` bump to 1.1.0 came with it. Kept: `template_version` never reaches the
provenance header and `drift_validator` hardcodes "1.0.0", so it changes nothing observable,
but bumping on a template change is what CLAUDE.md asks for.
## And it arrived unguarded
No test, so nothing stopped it regressing — the same gap that let the original defect exist.
`TestLogicalDateIsResolvedOrTheRunFails` covers it, including executing the generated
validation against real inputs, because the defect was precisely that nothing failed:
"", " ", "not-a-date", "2026-1-1", "20260101", "current_date()" -> all raise
"2026-09-30" -> accepted
Mutation-tested, all four caught:
revert getResolvedOptions to JOB_NAME only -> 1 fail
drop the format validation -> 2 fail
accept anything instead of raising -> 1 fail
stop substituting the token into the expression -> 1 fail
202 codegen tests pass
62 of 62 plugin copies byte-identical
claims_v2 re-rendered: 0 non-hash changes
drift validator exit 0
full suite failure set vs origin/main -> 48 = 48, diff empty
Bumped to 0.7.0.
## Still open
The Silver-commits-before-the-gate finding. `createOrReplace` then `job.commit` both run
before `quality_check_silver` starts, so nothing gates publication to Silver — including the
masking gate. Needs staging the write, gating, then promoting: a change to the medallion flow,
not a patch.
`verify_workload` appended a report only for files that already had a provenance header:
if parse_artifact_header(content) is not None:
report.reports.append(verify_artifact(artifact))
`verify_artifact` on a headerless file has always returned
`ok=False, reason="Missing or malformed deterministic header"`. The strict check existed;
the loop that calls it stepped around exactly the files that would fail it — so the
directory-level entry point, which is what CI and the orchestrator use, could not see a
violation of CLAUDE.md:137.
Measured on this tree: 6 of 13 artifacts under `workloads/*/{scripts,dags,sql}/` have no
header, and `workloads/claims` is a complete hand-written Bronze->Silver->Gold pipeline plus
DAG that reported clean by containing nothing the validator would look at.
Compounding it, `WorkloadDriftReport.ok` is `all()` over that list and `all([])` is True, so
"checked every artifact and found no drift" and "found no artifact to check" were the same
boolean. Four workloads passed on zero artifacts.
## Why no test caught it
`test_verify_workload_runs_on_every_generated_file` rendered two headed files and asserted two
reports. It never put a headerless file in the directory, so adding one left the count at two
and the suite green. The test's name stated the property; its body tested only the case that
already worked.
## The exemption list is a ratchet
The 6 existing files are three different situations, not one:
- `claims/` x3 — predates deterministic codegen. Its config is the OLD shape
(`transformations.yaml`, `quality_rules.yaml`, `schedule.yaml`), not the renderer's
(`bronze/silver/gold/quality/dag.yaml`), so re-rendering would mean authoring five specs,
i.e. inventing the dedup, null, threshold and timezone answers CLAUDE.md reserves for a
human. Superseded by `claims_v2`, which is the rendered equivalent.
- `customer_master/dags/` — has new-shape specs but no `dag.yaml`, so `airflow_dag` has
nothing to render from. The narrowest entry; renderable as soon as one exists.
- `glue_data_quality.py` x2 — DQDL. No template covers AWS Glue Data Quality;
`quality_check.py.j2` renders the PySpark `check_quality.py`, a different mechanism.
Exact paths, never globs: `*/scripts/quality/glue_data_quality.py` would exempt a third
workload's copy the moment someone hand-wrote one, which is the opposite of a ratchet.
Guarded by five tests — existence, staleness, reason length, no-globs, and a ceiling of 6.
## Verified
Four mutations, each kills at least two tests: revert to skipping; exempt everything; add a
bogus seventh entry; exempt an already-rendered file.
Drift exits 1 on the two pre-existing `customer_master` refusals only (`encrypt` masking
method, `scd_type: 2`), confirmed identical on a pristine HEAD worktree. Full-suite failure
set unchanged at 48.
…row, and gate before publishing
Both came out of a live HIPAA run. Neither was fixable by writing a better spec, because
neither capability existed.
## Fix A — k-anonymity suppression
`post_aggregation_columns` renders as `withColumn()`, and the write is an unconditional
`createOrReplace`. So a human answering "suppress cells below k=11" got a
`k_anonymity_suppressed` boolean sitting next to the disclosed row: still written, still
queryable, still identifying. A label is not a suppression. `gold_aggregate.py.j2` contained no
`.filter()` anywhere, and `gold_spec` had no property expressing a row predicate.
gold_spec.post_aggregation_filter Spark SQL predicate, minLength 1
suppress_rows(df) macro after post_aggregation_columns, before the write
One macro called from both write paths rather than two copies — the same reason
`aggregated_measure` is one macro, and that comment records what two copies cost last time:
`percentile` was silently dropped from the aggregation and the render still compiled.
The suppressed count is logged. A run dropping 90% of its cells to satisfy k=11 has a grain
problem, and the number is the only way anyone finds out.
No shipped Gold spec carries a suppression label, so there was no live disclosure to repair.
## Fix B — the gate publishes
The rendered DAG is
transform_silver -> quality_check_silver -> aggregate_gold -> quality_check_gold
and each transform ended in `createOrReplace()` then `job.commit()` before its gate started.
So the gate blocked the NEXT zone, not its own publication: a failed Silver gate stopped Gold
being built while unmasked PHI was already committed and queryable in Silver. And
`quality_check_gold` is the last task in the graph, so it blocked nothing whatsoever — an alarm
wired to no circuit.
CLAUDE.md says "Quality gates block promotion — no bypassing" and gives Silver a >= 0.80 gate.
What shipped was "gates block the next zone", which is a different property, and for Gold it
was no property at all.
silver_spec / gold_spec.publish_via_gate transform writes {iceberg_table}_staging
quality_spec.gate_before_publish gate resolves PROMOTE_TO, promotes after the raise
`iceberg_table` deliberately keeps the real name: `semantic.yaml`, the Lake Formation grants
and `gold_spec.source_table` all reference it, and encoding staging there would silently
repoint every one of them.
`PROMOTE_TO` is resolved only in that branch, because `getResolvedOptions` raises on a listed
argument the caller did not supply — the same reason `silver_transform` resolves
`logical_date` conditionally.
Two guards the implementation forced:
- **A star schema refuses.** The gate promotes exactly one table. A star schema also writes one
table per dimension straight from the Gold script, so gating the fact table would leave every
dimension publishing ungated while the run reported that publication was gated. Half a gate
is worse than none.
- **`PROMOTE_TO == TABLE_NAME` raises.** Otherwise the promotion rewrites the table onto itself:
a no-op that logs success while publication is still happening in the transform — the
original defect wearing the fix's clothes.
## shared/codegen/gate_consistency.py
Three specs must agree and no existing check sees more than one; the renderer validates one
spec against one contract, and each template is rendered from its own slots.
Stage-without-publisher never writes unapproved data — the safe direction — but the DAG goes
green while the real table keeps its previous contents and the next zone reads them. Silent
staleness is harder to notice than a failure. `check_workload()` catches that and seven other
shapes, including the worst one: scoring the staging table while promoting something else.
Run by the test suite over every workload, so it needs no CI wiring.
## Verified — 46 tests, 13 mutations, all killed
- template (5): promote above the raise; publish unconditionally; drop the self-promotion
guard; silver ignores the flag; gold half-gates a star schema
- contract (4): each of the three new properties deleted, and `boolean` -> `string`. Each
deletion left all 251 codegen tests green on its own, because the render tests build spec
dicts in memory and never call `validate_spec` — the same gap the cron fix hit. That is what
the 10 new contract tests are for.
- checker (3): `check_workload` returns `[]`; drop the stage-without-gate branch; stop checking
`--PROMOTE_TO`
- cross-artifact (1): `"_staging"` is spelled in three places and the templates cannot share a
macro — the renderer loads each from a string through `jinja2.BaseLoader`, so `{% import %}`
has nothing to import from. Mutating the suffix in either the template or the checker alone
now fails.
All three re-rendered `claims_v2` artifacts changed by the `template_hash` line only, 0 body
changes: no workload sets the new flags, so behaviour is unchanged. Full-suite failure set
unchanged at 48; 43/43 vendored files byte-identical; drift exits 1 on the two pre-existing
`customer_master` refusals only.
`silver_transform` 1.1.0 -> 1.2.0, `quality_check` 1.0.0 -> 1.1.0, `gold_aggregate` 1.1.0 ->
1.2.0, plugin 0.7.0 -> 0.8.0.
## Not adopted anywhere yet
Both features are inert until a spec opts in. Gating `claims_v2` means a staging table per zone
in the Glue catalog with the same LF-Tags as its target, plus an IaC update — a deployment
decision, not a codegen one.
`customer_master/config/quality.yaml` does not satisfy `quality_spec.schema.json` and has not
for as long as it has existed. Two rules use `check_type: date_range` and one uses
`conditional`; neither is in the enum, and one rule omits the required `column`.
Nothing in the repo could see it. The drift validator re-renders artifacts and compares hashes,
so it only ever consults a spec that produced an artifact — and this one produced none:
`customer_master/scripts/quality/` holds a hand-written `glue_data_quality.py` and no rendered
`check_quality.py`. So the contract was never consulted, the renderer was never called, and
there was no artifact to drift. An invalid spec whose template is never run is invisible to
every gate here.
I found it by validating the shipped specs by hand while checking something else, which is not
a mechanism. This test is the mechanism: every `workloads/*/config/{bronze,silver,gold,quality,
dag}.yaml` is validated against its contract.
`KNOWN_INVALID` follows EXEMPT_HEADERLESS: the one known-bad file is named together with the
substrings of the errors it is allowed to produce, so a DIFFERENT error in the same file still
fails, and a stale exemption fails as stale. Ceiling of one.
Not fixed here because it is not mine to fix: `date_range` is probably `range` on a date column,
but `conditional` has no equivalent in the enum and may need `custom_sql` — that is a
data-owner decision about what those two rules are meant to assert.
Verified by three mutations, each killing a test: break a currently-valid spec; introduce a NEW
error in the exempted file; make the exemption stop matching its own errors. Full-suite failure
set unchanged at 48.
`enforce_template_codegen.py` ended its refusal with "config/*.yaml is not protected: write
specs there directly." On a brand-new workload that is wrong, and wrong in a way that costs a
turn: `check_discovery_gate.py` denies `config/source.yaml` until the workload has either that
file or a `.discovery_complete` marker. Hooks are not namespaced, so both fire on the same
Write — an agent that follows the first message walks straight into the second denial with no
indication the two are related.
Measured outside an ADOP checkout, which is where a plugin user runs (inside one, this hook
stands down to the repo's own):
brand-new workload, nothing present:
config/source.yaml -> deny <- the message says write it directly
scripts/x.py -> deny
after .discovery_complete:
config/source.yaml -> allowed
scripts/x.py -> allowed
The message now says config/ is unprotected *by this hook* and names the other one.
Kept the exact phrase "config/*.yaml is not protected" because TRANSLATION.yaml declares it as
a REWRITE anchor — the manifest test caught the reword and failed two assertions, which is the
anchor doing its job. Editing the anchor to match my new wording would have been the easy fix
and the wrong one: the anchor exists to prove the rewrite kept that property.
Behaviour is unchanged — deny on scripts/ and dags/, allow on config/, and
`ADOP_RENDERER_TOKEN` still does not unlock the plugin's copy. Failure set unchanged at 48.
CLAUDE.md security rule 2 forbids bucket names and account IDs in source, so specs carry
`${DATA_LAKE_BUCKET}` and `${PHI_HASH_SALT_SECRET_ID}` rather than real values. That is the
right pattern and it was never wired: of five placeholder names in use, only
`${ADOP_LOGICAL_DATE}` had a substitution step. The other four reached rendered artifacts as
literal text.
Measured on a live run (TESTS/ONE, before this commit):
raw_to_bronze.py:31 landing_zone = args.get("landing_zone", "s3://${DATA_LAKE_BUCKET}/bronze/claims/")
bronze_to_silver.py:81 "s3://${DATA_LAKE_BUCKET}/quarantine/claims/" <- cleartext PHI lands here
bronze_to_silver.py:157 F.lit(_phi_salt("${PHI_HASH_SALT_SECRET_ID}")) <- masking cannot run
claims_pipeline.py:35 default_var="${CLAIMS_ALERT_SNS_TOPIC_ARN}" <- the alert is broken
One defect, four consequences: the Bronze write target, the quarantine prefix for PHI, the
PHI masking step, and the alert that announces a failure. `s3://${DATA_LAKE_BUCKET}/...` is a
syntactically plausible S3 URI, so it fails late and obscurely rather than at startup.
## A second, quieter defect in the same lines
source_path = args.get("source_path", "{{ source_path }}")
read as a runtime override and was not one — `getResolvedOptions` listed only `JOB_NAME`, so
the key was never in `args` and the spec literal always won. Dead parameterisation wearing a
live interface. It is why an earlier build asserted this script accepts `--source_path`: the
code says so and the argument list does not. Same shape as the `${ADOP_LOGICAL_DATE}` defect.
Removed rather than repaired; one mechanism is enough.
## One mechanism, not four patches
`runtime_placeholders(*values)` is a Jinja **global**, not a macro: templates are loaded from
strings through `BaseLoader`, so `{% import %}` has nothing to import and a macro would have to
be copied into every template that needs it. It walks nested containers, because
`salt_secret_id` lives at `pii_masking.columns[].salt_secret_id` — two levels down, the same
blind spot that let `rules.accuracy` go unrendered. It excludes `ADOP_LOGICAL_DATE`, which has
its own mechanism and arrives as the lowercase `logical_date`.
A template then lists each name as a **required** Glue argument. Required, not optional,
because `getResolvedOptions` raises on a listed argument the caller omitted — that is the loud
failure we want, in place of a path named after the placeholder. A spec with no placeholders
renders byte-identically to before.
Also added: Bronze refuses a `source_path` with no URI scheme. A bare `sample_claims.csv`
resolves against the Glue container rather than the repository, and Spark reports only "Path
does not exist" — which cost a whole build to diagnose.
## Verified
Six mutations, each killing at least one test: extractor stops excluding ADOP_LOGICAL_DATE;
extractor finds nothing; extractor stops walking dicts; bronze drops the URI guard; bronze
stops substituting; silver stops substituting.
One assertion of mine was wrong and I fixed the test, not the template: I had asserted the
placeholder literal appears zero times, but it legitimately appears as `_resolve`'s first
argument. The real property is that no *bare* use survives, which is what it now checks.
`claims_v2/scripts/extract/ingest_claims.py` changed 17 body lines — the dead override removed
and the guard added. Every other artifact changed by the template_hash line alone. Full-suite
failure set unchanged at 48; 43/43 vendored byte-identical; drift still the two pre-existing
customer_master refusals.
bronze_ingestion 1.0.0 -> 1.1.0, silver_transform 1.2.0 -> 1.3.0.
## Not covered here
`airflow_dag.py.j2` still emits `${CLAIMS_ALERT_SNS_TOPIC_ARN}` into a `Variable.get(...,
default_var=...)`. That is an Airflow Variable, not a Glue argument, so it needs a different
fix — most likely dropping the default so Airflow fails loudly rather than publishing to a
topic named after a placeholder.
The README opened with a one-paragraph pitch and then went straight to install internals. A reader could not answer the three questions they actually arrive with: what do I need, what do I get, and how does this reach my AWS account. Adds one "In short" section covering exactly those, in plain language — a source file, ~20-30 questions it will not guess the answers to, an AWS account that is optional for everything except profiling and deploying; then the artifacts; then the two commands and the `terraform apply` shape, with `APPLY_GUIDE.md` named as the authority since it is ordered and workload-specific. ## Two claims I had to walk back before committing My first draft promised two files the plugin does not guarantee: - `cp terraform.tfvars.example terraform.tfvars` — `iac-agent.md` specifies `APPLY_GUIDE.md` and the framework directories, and never mentions tfvars. - `KNOWN_GAPS.md` — mentioned nowhere in the plugin. Both exist in the one live run I had in front of me, which is how I came to believe they were contractual. They were the model's own additions on that run. A README that names a file the system does not promise is the same defect as a runbook recommending a masking method the contract rejects — this repo already carries a test class for that exact failure, and I reproduced it in prose. `terraform.tfvars` is now described as "set your account id, region and bucket names as the guide directs", and the gap report is described by its behaviour — agents report blockers rather than filling a gap with something that looks finished — which IS specified, via `blocking_issues` in the agent output contract. ## Verified Every command, file and capability the new section names was checked to exist: both commands, `APPLY_GUIDE.md` in the IaC agent's output list, `blocking_issues` in the agent contract, both terraform and CDK offered. Full-suite failure set unchanged at 48; the 3 README guard tests pass; 43/43 vendored files still byte-identical.
Running the suite left three directories behind under `workloads/`, all empty:
workloads/unknown/
workloads/env_workload/
workloads/sales_transactions/
The last is named after a path `tests/unit/test_script_tracer.py` invents
(`/project/workloads/sales_transactions/...`) for an assertion about name inference. No such
workload has ever existed.
Git cannot track an empty directory, so `git status` stayed clean and nothing flagged them. But
everything that globs `workloads/*` counted them — the drift validator iterated six "workloads"
and reported three as "no artifacts under scripts/, dags/ or sql/". A phantom workload dilutes
exactly the check meant to notice a workload with no generated code.
## Cause
`_default_trace_path()` — identical in `script_tracer.py` and `orchestrator_logger.py` — called
`log_dir.mkdir(parents=True, exist_ok=True)` while merely COMPUTING a path string. Constructing
a tracer created a directory tree even when nothing was ever written.
`AgentTracer._emit` already calls `mkdir(parents=True, exist_ok=True)` immediately before
opening the file, so the directory still appears the moment anything is genuinely logged. The
eager call was pure redundancy with a filesystem side effect. Removed from both.
Also fixed in script_tracer:
os.environ.get("TRACE_OUTPUT_PATH", _default_trace_path(self.workload_name))
Python evaluates a default ARGUMENT eagerly, so this called `_default_trace_path()` — timestamp
and mkdir — even when `TRACE_OUTPUT_PATH` was set, then discarded the result. Rewritten as an
explicit `or` chain.
## A wrong fix I made first, and why it was wrong
My initial change also anchored the path to the project root instead of leaving it relative to
cwd. That looks tidier and is wrong: `test_agent_tracer.test_default_trace_path` isolates itself
with `monkeypatch.chdir(tmp_path)`, which only works while the path is relative. Making it
absolute defeated that isolation and caused a correctly-written test to start writing real trace
files into `workloads/my_workload/logs/`. I had traded three empty directories for two committed
trace files.
Reverted. The path stays relative — a trace belongs where the job was launched from — and
`test_the_default_path_stays_relative_to_cwd` now pins that so the same mistake fails.
## And five test edits I reverted as unnecessary
I had added `monkeypatch.chdir(tmp_path)` to five tests in `test_script_tracer.py`. Mutation
testing showed removing one of them changed nothing, so I checked properly: with the eager mkdir
gone and no test edits at all, the full suite leaves zero litter. Those tests never emit an
event, so there is no write and no mkdir. The five edits were churn around the real fix and are
not in this commit.
## Verified
`tests/unit/test_no_workload_litter.py` — 5 tests covering the property (computing a path
creates nothing), the construction path, the relative-path requirement, and the observable
symptom (the real `workloads/` contains only the three real workloads, so a different cause is
still caught).
Three mutations, each killing 3 tests: restore the eager mkdir in either file, or make the path
absolute again.
Full suite run three times consecutively: 785 passed, zero litter each time. Failure set
unchanged at 48; 43/43 vendored files byte-identical.
Work done by a parallel session driving /adop:onboard-workflow against a HIPAA claims workload. I verified it rather than authored it; what follows is what I checked, plus the two things it left incomplete. ## What it implements `gold_aggregate` 1.2.0 -> 1.3.0 — a real SCD Type 2 merge. Previously this refused outright (my guard, commit 0b1bbfd): Type 1 where Type 2 was asked for loses history irrecoverably, because the superseded rows are never captured, so unlike every other gap it cannot be repaired by re-rendering later. Refusing satisfied that property. A merge satisfies it better: - bootstraps with `spark.catalog.tableExists`, then merges on later runs - emits `effective_from` / `effective_to` / `is_current`, dated from the run's logical date rather than wall clock, so a backfill of the same logical date lands identically - never uses `createOrReplace` on a Type 2 dimension — only the fact table overwrites - declines to emit a row-closing merge when the business key is the dimension's only column, with the reasoning stated inline: a `WHEN MATCHED AND (false)` merge would be dead SQL that reads as though it versions rows It also fixed a latent bug in my guard. I checked whether *any* `dimensions[]` entry had `scd_type: 2` and then refused the whole render, so one Type 2 attribute blocked a workload's Type 1 dimensions too. Detection is now per-table. `bronze_ingestion` 1.1.0 -> 1.2.0 — stamps an ingest sequence; Bronze stays append-only. Contracts, all optional and backwards-compatible: `gold_spec.output_tables[].business_key` and `.atomic`, `bronze_spec.ingest_sequence_column`, `quality_spec.de_identification_standard`. ## The refusal that remains, and should A Type 2 dimension with no `business_key` still refuses. A merge has to know which rows are successive versions of the same entity, and `dimensions[]` names attributes rather than tables, so there is nothing to infer it from — guessing would merge unrelated rows. ## Two things it left, which I fixed here **A failing test.** `test_scd_type_2_refuses_rather_than_emitting_type_1` asserted the refusal this replaces, so it failed. The session reported "25 unit tests pass" — the workload's own tests, not the 820-test repo suite. Replaced rather than deleted: the property is unchanged, only how it is satisfied. Three tests now guard it — the dimension is never overwritten, the `business_key` refusal survives, and a Type 1 dimension still renders alongside a Type 2 one (the latent bug above). One of my new assertions was itself vacuous and mutation testing caught it: `assert "effective_from" in body` passed when the column was renamed to `effective_from_renamed`, because the longer name contains the shorter. Now matched as `.withColumn("effective_from",`; all three renames fail. **Two artifacts bumped but not re-rendered.** Drift went 2 -> 4. Re-rendered `claims_v2/scripts/extract/ingest_claims.py` and `.../silver_to_gold.py`: both changed by the `template_hash` line alone, 0 body changes, so no behavioural change to shipped artifacts. ## Verified Full suite 787 passed, failure set unchanged at 48. Contracts 8/8 valid, every new field optional and none added to `required`. 43/43 vendored files byte-identical — its claim to have synced both trees holds. Drift back to the two pre-existing `customer_master` refusals (`encrypt` masking method, `scd_type: 2` on a spec with no business_key). My own earlier fixes survive intact in both touched templates: the k-anonymity `suppress_rows` macro and both call sites, `publish_via_gate`, and `runtime_placeholders` / `_resolve` / the URI-scheme guard in bronze_ingestion. ## Not resolved `bronze_spec.ingestion_mode: cdc` remains declarative — no template reads it. The merge is real, but it is Silver's `keep_latest` ordered by the ingest sequence, not a Bronze upsert. A new unread contract field; the ratchet in test_template_schema_agreement.py should name it. The human answered `drop_exact_duplicates_only` for dedup and that answer was overridden to `keep_latest`, because with CDC a PENDING->PAID restatement is same-PK-different-content and would have been quarantined rather than applied. The reasoning is sound and the determinism objection is addressed by ordering on the ingest sequence, which never ties — but the record of the override lives only in a YAML comment and run/context.json notes.
Every spec can validate against its contract, every rendered script can compile, and the
pipeline can still be unrunnable because the stages do not refer to each other correctly. Three
such defects shipped together in one HIPAA workload and were found by a human reading generated
Terraform — not by any check. Two of the three were also live in this repo's own workloads.
## The four edges
`check_stage_connectivity()` in shared/codegen/gate_consistency.py, run by the test suite and
by the CLI alongside the existing gate check:
1. **The Iceberg catalog prefix, in both directions.** `silver_spec.source_table` must NOT carry
`glue_catalog.` — Bronze is plain Parquet, since bronze_ingestion ends in
`.save(landing_zone)` and registers no table, and the Iceberg catalog cannot read a
Hive/Parquet table. `gold_spec.source_table` MUST carry it, because Silver is written as
Iceberg by its own template. A rule of "never use glue_catalog" would break every Gold spec.
2. **A Silver spec reading a Bronze table its workload does not produce.**
3. **`glue_job` task script_path -> a file that was actually rendered.**
4. **`quality_check` task -> the key the template resolves, which is script_path OR
airflow_dag's default.**
## Why edge 4 is separate, and why the obvious version would have passed
A parallel session told me `airflow_dag.py.j2` hardcoded `quality_check.py` for quality tasks
and ignored script_path entirely — so quality specs deliberately omit script_path, because a
value there was inert. A checker walking only script_path therefore skips every quality task,
which is precisely the pair that was broken in both shipped workloads. I would have written that
version. The correction is theirs.
Fixed at source too: the template now renders
`task.script_path | default('quality_check.py')`, matching its own glue_job branch. Every
existing DAG renders byte-identically because no quality task set script_path, so the default
fires — but a spec can now name the artifact it rendered. Three different names were in play
before: this constant, the onboard command's `scripts/quality/glue_data_quality.py`
(onboard-workflow.md:564), and `check_quality.py` as claims_v2 actually had on disk.
## Why edge 1 matters more than it looks
KNOWN_GAPS §2b logged only "source_table names a table bronze_ingestion never registers", so the
obvious remedy is to register it — and a checker verifying only EXISTENCE would then go green
while Silver still failed, because of the catalog prefix. Fixing the logged half of a defect and
passing a check is worse than having no check. The framing is the parallel session's.
## Instances fixed
claims_v2 silver.source_table glue_catalog.claims_v2_db.bronze_claims_v2 -> claims_v2_db.bronze_claims_v2
customer_master silver.source_table glue_catalog.customer_master_db.bronze_customer_master -> customer_master_db.bronze_customer_master
claims_v2 dag.yaml script_path set on both quality tasks -> scripts/quality/check_quality.py
claims_v2's bronze_to_silver.py re-rendered with the real body change,
`spark.table("claims_v2_db.bronze_claims_v2")`. customer_master's artifacts still refuse to
render for an unrelated pre-existing reason (pii_masking method `encrypt` is unimplemented), so
its spec is corrected while its artifact stays stale.
customer_master having no bronze.yaml is NOT fixed and is ratcheted instead: authoring a Bronze
spec means choosing a source path, format and ingestion mode, which CLAUDE.md reserves for the
human.
## A bug only the CLI caught
I appended the new function AFTER the `if __name__ == "__main__"` block. On import the module
executes fully, the guard is False, and the definition is reached — so all 302 tests passed while
`python -m shared.codegen.gate_consistency` died with
`NameError: name 'check_stage_connectivity' is not defined`. CI invokes it as a module, so a
green suite would have shipped a CLI that cannot start. Moved above the guard, and two tests now
run the CLI in a subprocess — one asserting a consistent workload exits 0, one asserting an
inconsistent one exits nonzero, since exit 0 on a real problem is the failure mode that matters.
## Verified
Seven mutations, each killing at least one test: checker returns []; stop flagging the prefix in
either direction; the quality default stops mirroring the template; skip quality tasks entirely;
the template's default drifts from the mirror; the definition moved back below the guard.
`test_the_quality_default_matches_the_template` reads the template source and pins the two
together, because the constant in the module is a MIRROR of the template's default and silent
drift between them would leave the check testing a key the template no longer emits.
Full suite 800 passed, failure set unchanged at 48. 43/43 vendored byte-identical. airflow_dag
1.0.0 -> 1.1.0 with claims_v2's DAG re-rendered. gate_consistency CLI: claims OK, claims_v2 OK,
customer_master INCONSISTENT (the ratcheted one).
## Correction to something I told the parallel session
I claimed post_aggregation_filter "refuses to render on a star_schema that also has dimension
tables". It does not, and no such guard exists — I conflated my own two fixes. The
`unsupported()` is conditioned on `publish_via_gate`, not on the filter; suppress_rows is invoked
unconditionally for both schema types. Verified by rendering star_schema + a dimension table +
the filter, which renders clean.
I now think the absence is correct rather than a gap. publish_via_gate genuinely cannot cover a
star schema, because the gate promotes exactly one table while the script writes the fact plus N
dimensions. A row filter has no such asymmetry: dimensions are attribute lookups built with
dropDuplicates(), they carry no counts, and dropping rows from them would break the fact-to-
dimension joins without reducing the small-cell disclosure the filter addresses.
…event
The guard rejected a bare path and then named the next failure in its own error message:
f"...Stage the data and use s3://..., or file:///abs/path for a deliberate local run."
There is no local run. bronze_ingestion.py.j2 imports `awsglue.context.GlueContext` and
constructs one, and .claude/rules/03-python-airflow.md requires every ETL script to target Glue.
A Glue worker cannot read the operator's disk, so file:// can never work here.
A live HIPAA workload took the invitation and shipped
`file:///Users/<name>/.../sample_claims.csv` in three files — unreachable from a Glue worker,
and carrying a home directory into source alongside it. The guard caught the shape it was
written for and waved through the shape it suggested. That invitation is mine; I wrote both the
guard and the message in 039d112.
Now accepts `s3://` and `s3a://` and nothing else. No shipped spec uses file://, so this
tightens without breaking anything. The message says why each rejected form fails rather than
only that it does: a relative path resolves against the worker's own container, file:// is the
workstation's disk, and a path under a home directory also leaks a username into the spec.
## The test now runs the guard instead of grepping for it
The previous test asserted `'"://" not in source_path' in body` — a string match on the rendered
source, which passes for a guard that is present and wrong, and which pinned the exact expression
so any rewrite looked like a regression. It extracts the `if` node from the rendered AST and
executes it against four inputs: s3:// and s3a:// accepted, a bare path, a relative path and
file:///Users/... rejected.
Three mutations, each killing at least one test: drop the guard; revert to the permissive
`"://" not in` form; drop s3a:// support.
bronze_ingestion 1.2.0 -> 1.3.0, claims_v2's ingest_claims.py re-rendered. Full suite 806
passed, failure set unchanged at 48, 43/43 vendored byte-identical.
…led copy
Plugin updates are version-gated. Seven commits have touched plugins/ since the 0.8.0 bump, so
an install or update today delivers none of them — the cache stays on 0.8.0 content while the
repo has moved on. That is not hypothetical: before this bump the installed 0.8.0 carried
gold_aggregate 1.2.0 while the repo was at 1.3.0.
Shipping in 0.9.0:
3107dbd Bronze source guard tightened to s3:// or s3a:// only
e4e783b four-edge stage-connectivity checker; airflow_dag reads task.script_path
a5c2b5c SCD Type 2 merge, CDC ingest sequence, 4 contract fields
098f86e test suite stops creating phantom workloads
9afd97b README: inputs, outputs, how to deploy
039d112 ${NAME} runtime parameters resolved instead of shipped as literal text
c5ca81c one hook's deny message no longer contradicts another's
Template versions delivered: bronze_ingestion 1.3.0, silver_transform 1.3.0, gold_aggregate
1.3.0, airflow_dag 1.1.0, quality_check 1.1.0.
Verified after bumping rather than before: 806 passed, failure set unchanged at 48, 43/43
vendored files byte-identical, 63/63 declared COPY entries identical, plugin.json parses.
Installed and confirmed the content shipped rather than only the version number — all nine
fix markers present in the cache, and the installed lib/ is 43/43 identical to the repo. Worth
checking explicitly, because a version bump proves nothing about content on its own.
Member
|
Great work introducing the Claude Code plugin for ADOP. It packages the agents, hooks and skills, and hardens the code generator so it fails loudly instead of shipping silent no-ops. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Packages ADOP as a Claude Code plugin so users install it rather than cloning the repo, and fixes the engine defects that running it end to end exposed.
Most of this diff is not the packaging. 19 of the 33 changed non-plugin files are
shared/,contracts/andtests/— defects in the codegen engine that affect anyone using ADOP, plugin or not..claude/andCLAUDE.mdare untouched, so the agentic layer behaves identically.What the plugin gives a user
Install it, run
/adop:onboard-workflowin your own repository, answer ~30 questions it will not guess the answers to, and get a tested Bronze→Silver→Gold pipeline: PySpark ETL, a quality gate, an Airflow DAG, unit tests, an optional OWL/R2RML semantic layer, and Terraform or CDK. It never applies infrastructure — it generates and hands youAPPLY_GUIDE.md.2 commands · 6 agents · 5 skills · 7 hooks · 13 MCP servers · 63 vendored filesThe nine engine defects
Each was found by running the pipeline, not by review. They share one shape: a check that finds nothing looks identical to a check that finds no problems.
post_aggregation_columnsrenders aswithColumn(), so "suppress cells below k=11" produced a boolean label beside the disclosed row.gold_aggregatecontained no.filter()at allcreateOrReplace()+job.commit()before its gate. A failed Silver gate stopped Gold being built while unmasked PHI was already queryable.quality_check_gold, being last, blocked nothing${NAME}parameters never substitutedverify_workloadonly reported on files that already had a provenance header, so a hand-written pipeline read as clean.workloads/claimsis exactly thatglue_catalog.on a Parquet Bronzeairflow_daghardcodedquality_check.pyand ignoredscript_path, while the command instructed writingglue_data_quality.py. Three names in play; both shipped workloads brokenfile:///abs/pathfor a deliberate local run". There is no local run — a workload took the invitation and shipped a home-directory patheffective_from/effective_to/is_currentmkdirfired while computing a path, creating three phantom workloads — one named after a path a test invents and which has never existedVerified on real AWS
Bronze → Silver → Gold on Glue 5.0 in a non-production account, queried in Athena:
Both halves of the gate were exercised, not just the happy path.
How it was checked
~160 tests added. Every fix was mutation-tested — reverted to confirm a test fails. Several initially survived their own reversal and were rebuilt:
validate_specassert "effective_from" in bodypassed when the column was renamed toeffective_from_renamedscript_pathwould have passed both broken quality tasks, since that is exactly wherescript_pathis absentgate_consistencyCLI died withNameErrorwhile all 302 tests passed, because the function was defined afterif __name__ == "__main__"Ratchets rather than allowlists:
EXEMPT_HEADERLESS(6),KNOWN_INVALID(1),KNOWN_DISCONNECTED(1),KNOWN_UNREAD(9, down from 16). Each entry names the defect that justifies it, must shrink, and fails as stale once fixed.Known-red CI — all three pre-existing
Verified on an
origin/mainworktree: same failures, same counts.CI runs only 4 test files, two of which end in
|| true. None of the ~160 new tests run on a PR. The suite passes locally; nothing enforces it. Worth addressing separately.Not fixed, deliberately
customer_masterhas nobronze.yaml— authoring one means choosing source path, format and ingestion mode, whichCLAUDE.mdreserves for the human. Ratcheted so the checker fails loudlycustomer_master/config/quality.yamlhas never validated —date_rangeandconditionalaren't in thecheck_typeenum. Invisible until now because that workload has no rendered quality artifact, so nothing ever calledvalidate_speciceberg_ddlis unrenderable — needs atablesslot no spec suppliesingestion_mode: cdcis declarative — the merge is Silver'skeep_latest, not a Bronze upsertshared/policies/workloads/claims/permits.cedaris unvalidated —cedaris not installed, so its syntax has never been checkedhooks.jsonmatchesWrite|Edit|MultiEdit— a Bash heredoc to a protected path fires no hookhash_saltedand Lake Formation grants have no runtime evidence — a Secrets Manager write was unavailable in the test environment