Repository navigation
docs(skills): add rail contribution, review, and recorded-test skills - #2223
Conversation
73a2977 to
b02780f
Compare
cdb9cd5 to
99dc378
Compare
[Low impact] The PR appears safe to merge; no actionable issue remains from this review. SummaryAdds three agent skills for contributing rails, reviewing rails, and placing recorded tests.
Reviews (9) · Last reviewed commit: "docs(skills): point HTTP guidance to the..." · Reviewed by Greptile |
📝 WalkthroughWalkthroughAdded skills and subtree guidance for implementing, testing, and reviewing library rails. The changes also clarify recorded-test placement, cassette workflows, and inline snapshot behavior. ChangesLibrary rail guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/recorded/AGENTS.md (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep placement decisions in the recorded-tests skill.
This section repeats the placement rule, while lines 18-20 say the skill owns placement decisions. Keep this file as a pointer and keep the detailed criterion in
.agents/skills/recorded-tests/SKILL.mdso the copies do not drift.Based on learnings: use
tests/recorded/README.mdfor mechanics and.agents/skills/recorded-tests/SKILL.mdfor placement decisions; do not duplicate placement guidance inAGENTS.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/recorded/AGENTS.md` around lines 5 - 9, Remove the duplicated recorded-test placement rule from tests/recorded/AGENTS.md, leaving it as a pointer to tests/recorded/README.md for mechanics and .agents/skills/recorded-tests/SKILL.md for placement decisions. Preserve the skill file as the single source of truth for the real-provider-request criterion.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/add-library-rail/SKILL.md:
- Around line 9-13: Narrow the introduction’s enforcement claim to say that
structural contracts are checked by conformance gates, while privacy, retry,
fail-closed, telemetry, documentation, cassette provenance, and other behavior
require focused tests or manual review. Update the workflow wording to preserve
the distinction between generic gates and rail-specific behavioral or recorded
end-to-end verification.
In @.agents/skills/review-library-rail/SKILL.md:
- Around line 31-40: Update the “Step 1: Run the mechanical gates” procedure to
run the repository’s locked pre-commit command before any make test or git diff
commands. Preserve the existing mechanical gate commands and ordering after
pre-commit, and use the project’s established locked pre-commit invocation
rather than inventing a new one.
In `@tests/recorded/AGENTS.md`:
- Around line 10-13: Align the marker guidance across
tests/recorded/AGENTS.md:10-13, .agents/skills/recorded-tests/SKILL.md:136-147,
and .agents/skills/add-library-rail/SKILL.md:455-459. Document the actual
behavior of tests/recorded/conftest.py, including that marker enforcement is not
performed at setup, and define one consistent policy for the rare
pure-runtime/public-API exception while accounting for existing non-VCR modules.
Update all three files consistently; no code change to conftest.py is requested.
---
Nitpick comments:
In `@tests/recorded/AGENTS.md`:
- Around line 5-9: Remove the duplicated recorded-test placement rule from
tests/recorded/AGENTS.md, leaving it as a pointer to tests/recorded/README.md
for mechanics and .agents/skills/recorded-tests/SKILL.md for placement
decisions. Preserve the skill file as the single source of truth for the
real-provider-request criterion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a754a4ad-d320-42b6-8762-4a6cdaf12f22
📒 Files selected for processing (6)
.agents/skills/add-library-rail/SKILL.md.agents/skills/recorded-tests/SKILL.md.agents/skills/review-library-rail/SKILL.mdnemoguardrails/library/AGENTS.mdtests/recorded/AGENTS.mdtests/recorded/README.md
tgasser-nv
left a comment
There was a problem hiding this comment.
This needs updating to match the latest code, left individual comments. Some other comments:
uv buildwill includenemoguardrails/library/AGENTS.mdin the package. Recommend adding this to thepyproject.tomlsource-excludelist.
Decides when a change requires a recorded/VCR test and whether a test belongs in tests/recorded/ or tests/. Core rule: no provider request crossing the wire means nothing to record, so the test is a unit test. Covers the when-required triggers (wire path, public output contract, new provider, model-calling rail, provider-shaped bug), the negative-path ladder, the pure_runtime exception, and dialect-single recorded coverage. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Step-by-step guide for adding a rail under nemoguardrails/library/: archetype exemplars, the rail.py manifest contract, RailOutcome actions, manifest-declared Python package requirements, HTTP rules for vendor backends, the three test layers including both-dialect coverage, and docs and examples. Rules are enumerated concretely because evaluation showed agents comply with what a skill names and miss what it omits; the eval's misses are baked in as named rules (data_retention, dialect-consistent flows, RetryingHTTPClient semantics, docs_url extension, flow-level raise test). Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Reviewer counterpart for rail-contribution PRs: scope and duplicate checks, mechanical gates first (and no re-reviewing what a green gate proves), nine judgment dimensions tests cannot check (privacy honesty, retry semantics verified against retry.py, fail-closed tested not asserted, dialect coverage, docs accuracy, and more), a security pass, and a verdict format that reports clean dimensions as well as findings. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Records why the rail and testing skills are checklists rather than principles (eval finding: agents comply with what is named), the division of labor between skills and mechanical gates, and which test layer owns Colang dialect coverage (gate: structure; unit tests: both-dialect behavior; recorded tests: dialect-single with two named exceptions). Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Gates verify what exists but cannot flag what is absent. Reviewing a live inbound rail PR showed the two most load-bearing protections (a working Colang 2 path and mandatory recorded coverage) were only implied via the author skill, not named as review checks. Name them: manifest presence (an unmanifested rail bypasses every catalog-keyed gate), both-dialect unit tests, the flow-level raise test, the recorded outcome triad, and docs plus example config. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Vendor rails sometimes offer fail-open behavior. Name the review rules: fail-closed by default, the switch belongs in the rail config model rather than an env var because it changes the rail's security posture, fail-open results must be distinguishable from genuine vendor clears (no synthesized vendor payloads) and logged, and the fail-open path needs its own test. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
tests/recorded/AGENTS.md is a thin tripwire for agents editing files in the suite: the placement rule, the marker requirement, the dialect-single rule, and pointers to README.md for mechanics and the recorded-tests skill for the decision procedure, without restating either. The README now states the placement rule in its opening paragraph instead of only inside the negative-paths section, since misplacement is the most common mistake and the rule was easy to skim past. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
The snapshot workflow was documented but its semantics were not, and agents route around snapshot() when unsure. Verified empirically and documented: create/fix rewrites the test file in place (the diff is the expected outcome, with a normal teardown notice on the same run); an unfilled snapshot never passes silently (teardown error serially, failed comparison under xdist); always snapshot the normalized form. The contribution skill's recorded-suite step now states the same rules in brief. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Cassettes are claims by their submitters; an agent or contributor without service access can fabricate one that looks recorded. Name the review rule: rely on the sanitizer fixed-point gate for naive fabrication, ask how external cassettes were recorded, and when unverifiable either re-record as a maintainer or demote to the explicit fake_cassette regime rather than merging them as recorded truth. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
The skill predated the Stack 4c conformance layer. Name test_builtin_rail_conformance.py in the catalog-gates step and the final verification loop, explain the schema snapshot it enforces and how to regenerate it, and state the rule that a contributor fixes the manifest rather than editing a generic gate or extending an exception list. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Nothing loads it: skill discovery keys off SKILL.md frontmatter, and AGENTS.md points at the directory rather than this file. What it contained was a restatement of contracts that live in the SKILL.md files, so it drifted with them; it needed correcting three times in one review pass for facts that were already stated, and correctly, elsewhere. Its design-rationale section also asserted that every defect in an evaluation mapped to a rule the skill did not state. A later evaluation on a corrected base showed much of that was drift between the skill and the branch it was written against, so the claim no longer holds as written. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
…ontradiction Four corrections from review feedback, each verified against the code: tests/recorded/AGENTS.md claimed every suite test must carry vcr or pure_runtime and that this is enforced at test setup. pure_runtime appears nowhere except that sentence: it is unregistered in pytest.ini, and neither autouse fixture in tests/recorded/conftest.py inspects markers, so a misplaced deterministic test collects silently. State the real split, name the seven non-vcr modules that exist, and say plainly that nothing enforces it. Drop the duplicated placement rule, which the same file already tells readers not to restate. add-library-rail opened with 'Every contract described here is enforced by a test'. Structural contracts are; privacy, retry, fail-closed, telemetry, docs and cassette provenance are exactly the dimensions the review skill exists to cover because no gate checks them. Separate the two. The registry note said neither registry gate is in the focused loop, added in the same commit that put both into the loop. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
The docs said every non-vcr suite test carries pytestmark = [pytest.mark.recorded, pytest.mark.asyncio]. The asyncio mark follows from the tests being async, not from the module being non-vcr: the four synchronous cassette meta-tests carry pytest.mark.recorded alone, and test_cassette_provenance.py carries no pytestmark at all. Qualify the claim at all three sites and cite a synchronous exemplar alongside the async one. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Both skills stated that a non-vcr suite test carries pytest.mark.recorded, which tests/recorded/test_cassette_provenance.py falsifies: it carries no pytestmark at all. Add the exception to both, and scope the rule to tests that exercise the library through the suite, since the cassette meta-tests are infrastructure for the suite itself rather than replay tests and should not be read as a template. Nothing rejects a missing marker either; tests/recorded/conftest.py still performs no marker enforcement. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
RailSpec.actions is Optional[RailActions], and RailActions holds the ref tuple. The step described the ActionRef contents without the wrapper, so the literal form fails at manifest construction. is_system_action was described as controlling only prompt-history rendering. It also keeps the action local: both runtimes dispatch non-system actions to config.actions_server_url when one is configured, so a rail action left at the default False is executed off-box, carrying the text it was asked to check. That is an egress consequence and belongs in the rule. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
All three skills asserted that the library recorded suite holds one shared Colang 2 smoke pin, maintainer-owned. It does not exist: no config under tests/recorded/ sets colang_version, on this branch or on develop, so the suite is Colang 1 throughout. The operative rule is unchanged and still correct: do not add a per-rail Colang 2 recorded test. Replace the false justification with what is actually true, so a contributor is not sent looking for a pin that is not there. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
…ility Three problems, all verified against the repository. The step never mentioned named optional extras, though pyproject defines four rail-scoped ones (sdd, gcp, jailbreak, multilingual) and their docs install lines use them. Adding one is a legitimate path; document it, including the mirror into the all extra that nothing tests. The extras= rule said to use it only when the package already ships in an extra, which reads as guidance the tree does not follow: four rails qualify and only sensitive_data_detection acts on it, including the very rail cited two paragraphs earlier as the optional_dependencies exemplar. Say plainly that the tree is inconsistent so nobody infers the rule from a neighbour. pytest.importorskip was offered as the packaging-free alternative. No rail test uses it. Name the four patterns that are actually in the tree, including the check_optional_dependency helper the repo ships for this. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
- The per-rail recorded config directory is a convention, not a documented requirement; the suite also holds shared multi-rail directories, so tell contributors to reuse one first. - generator= does not merely bypass the main model: with streaming output rails disabled, stream_async returns the generator and no rail runs at all. - The non-vcr public_api examples were miscited; check() contracts use local-rail configs, and FakeLLMModel appears in generate_async argument validation. - Not every shipped manifest sets RailFlows: a config-only rail such as factchecking omits it and the gate skips it rather than failing. - The modify-an-existing-rail matrix is not identical to the one in library/AGENTS.md; the dependency row differs. - There is no instrumented client to compose. URL sanitization is unconditional in the transport and error layers. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
RailOutcome.transform takes rewrites positionally; only allow and block are fully keyword-only. The skill showed the correct call nine lines above the sentence that got it wrong. The clavata caveat said '8 tests'. That counts tests/test_clavata.py alone; 32 are collected across its two files. The undercount also discarded test_clavata_models.py, which is worth copying as a vendor payload-shape example. Scope the criticism to rail-behavior coverage, where the three substantive claims (no Colang 2, no action-raise test, no recorded entry) were reconfirmed. Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
2fc6a08 to
d634505
Compare
|
Staged Fern docs preview: https://nvidia-preview-pr-2223.docs.buildwithfern.com/nemo/guardrails |
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
fixed in d634505, added the library |
|
@tgasser-nv I've addressed your comments 👍 |
tgasser-nv
left a comment
There was a problem hiding this comment.
Looks good! Just a few nits to clean up prior to merge.
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Description
Adds three agent skills under
.agents/skills/, plus subtree contributordocs, for the workflow around
nemoguardrails/library/:add-library-rail: step-by-step contribution guide for a new rail.Manifest, actions returning
RailOutcome, config schema, both Colangdialects, HTTP rules for vendor backends, optional dependencies, the three
test layers, docs and examples, and a matrix for modifying an existing rail.
review-library-rail: the reviewer counterpart. Runs the mechanicalgates first, then fourteen judgment dimensions the gates cannot check
(privacy honesty, retry semantics, fail-closed behavior, dialect coverage,
cassette provenance, conformance drift, client ownership), then a security
pass.
recorded-tests: decides whether a change needs a recorded/VCR test andwhether a test belongs in
tests/recorded/ortests/.Also included:
nemoguardrails/library/AGENTS.mdas an always-loaded tripwirefor the rail subtree, and additions to
tests/recorded/AGENTS.mdandtests/recorded/README.md.Docs only. No runtime, public API, or test behavior changes.
Design notes worth a reviewer's attention
Skills are checklists, not principles. These enumerate concrete checks with
file paths and commands rather than stating goals. That is deliberate: in
evaluation, agents complied with what a skill named and missed what it omitted,
close to one for one. Where a gap is found, the fix is a named rule, not better
prose.
Anything mechanically checkable is a test, not skill text. The skills own
placement decisions and judgment calls; manifest completeness, the HTTP
transport boundary, flow-file validity, and cross-artifact conformance stay in
their existing gates. The review skill's first step is to run those gates and
then spend attention only on what they cannot prove.
Areas that need careful review
develop. Every path, symbol, test name, marker,fixture, and CLI flag in these files is a checkable assertion, and an earlier
draft of this branch described a state that never shipped. The dependency
section, the verification loops, and the retry semantics paragraph were all
rewritten to match
developspecifically. Please spot-check anything thatlooks familiar from the manifest stack.
add-library-railStep 5 (packaging). The rule distinguishes the userinstall path from the
test_integrationdependency group. That distinction isload-bearing and easy to get wrong in either direction.
do-not-copy caveats (
clavata's test coverage,content_safety's refusalprose). If you disagree with either caveat, the skill text is the place to
argue it.
Verification
Docs-only change, so CI is necessary but not sufficient. Beyond CI:
uv run --locked pre-commit run --files <changed files>: passes.make test: 6277 passed, 178 skipped. Unaffected by this branch, run toconfirm no incidental breakage.
Eight reviewers covering reference integrity, behavioral claims, exemplar
validity, cross-skill consistency, command correctness, and public-surface
coverage. Every finding required a
file:linecitation or a failing command.This produced the corrections in the middle of the commit range.
implemented a fictional vendor rail from these skills, in isolated worktrees,
graded blind against a pre-registered rubric. All three passed the full suite
(6346 / 6340 / 6349 passed) and resisted all twelve planted traps. The three
convergent gaps they surfaced are fixed in the last four commits.
Checks not run, with residual risk:
which traps they prevent; it does not measure marginal value over no skill,
which would need a control arm.
AI Assistance
Checklist
Summary by CodeRabbit