Skip to content

[RFC] docs: add Claude skills for operations/facts/tests conventions - #1701

Open
maisim wants to merge 1 commit into
pyinfra-dev:3.xfrom
maisim:3.x-docs-add-ai-skills
Open

maisim wants to merge 1 commit into
pyinfra-dev:3.xfrom
maisim:3.x-docs-add-ai-skills

Conversation

@maisim

@maisim maisim commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Move my detailed file-scoped conventions (copilot instructions) into tracked Claude skills under .claude/skills/, following the pattern proposed in upstream PR #1683:

  • writing-operations: operation patterns, _inner() composition, error handling
  • writing-facts: FactBase anatomy, parsing, multi-file marker pattern
  • writing-operation-tests: JSON/YAML fixture schema for tests/operations/
  • writing-fact-tests: JSON/YAML fixture schema for tests/facts/

Also enrich AGENTS.md with a few items previously held only locally:

  • Command objects (StringCommand, QuoteString, MaskString, ...)
  • Connector data (connector_data_meta / host.connector_data)
  • Coverage gevent concurrency note
  • Index of the new skills under "Detailed conventions"
  • Pull request is based on the default branch (3.x at this time)
  • Pull request includes tests for any new/updated operations/facts
  • Pull request includes documentation for any new/updated operations/facts
  • Tests pass (see scripts/dev-test.sh)
  • Type checking & code style passes (see scripts/dev-lint.sh)

Move detailed file-scoped conventions out of local-only AI configs and into
tracked Claude skills under .claude/skills/, following the pattern proposed in
upstream PR pyinfra-dev#1683:

- writing-operations: operation patterns, _inner() composition, error handling
- writing-facts: FactBase anatomy, parsing, multi-file marker pattern
- writing-operation-tests: JSON/YAML fixture schema for tests/operations/
- writing-fact-tests: JSON/YAML fixture schema for tests/facts/

Also enrich AGENTS.md with a few items previously held only locally:
- Command objects (StringCommand, QuoteString, MaskString, ...)
- Connector data (connector_data_meta / host.connector_data)
- Coverage gevent concurrency note
- Pointer to pyinfra-dev-deploy for working with multiple PRs
- Index of the new skills under "Detailed conventions"
@maisim

maisim commented Apr 26, 2026 •

Copy link
Copy Markdown
Contributor Author

@Fizzadar @wowi42 @DonDebonair
To share, Just move my untracked copilot instructions files to claude skills.

we should keep all, part, or nothing, let me know

I just did a complete reread and didn't find any major mistakes. 🤞

@wowi42 wowi42 added documentation Issues with docs/examples/docstrings. discussion Issues that need opinions! labels May 7, 2026

@Fizzadar Fizzadar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of thoughts!

{
"args": [],
"kwargs": {},
"exception": "OperationError"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exceptions look like (example):

    "exception": {
        "names": ["OperationValueError"],
        "message": "'content' must be supplied when 'present' == True"
    }

**Prefer YAML for new fixtures.** To cover a new code path, add a fixture — do not
write a new Python test.

## Fixture schema (JSON)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This schema + facts are duplicated in AGENTS.md as well which is likely to drift - we could point agents at each skill file? (Will non-claude harnesses handle that correctly?)

### `output` format

Each string is one line of stdout. Empty strings represent blank lines.
Use real command output whenever possible — copy from an actual system.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Use real command output whenever possible — copy from an actual system.
Always use real command output — copy from an actual system.

@wowi42 wowi42 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skills need another pass before they are safe to use. Several examples do not match the test harness or the project shell quoting rules.

Issues

  • .claude/skills/writing-operation-tests/SKILL.md documents invalid fixture forms. A string exception fails because the parser expects a mapping with name/names and message; file uploads serialize as arrays, not {type: "put"} objects:
    "exception": {"names": ["OperationError"], "message": "..."}
    "commands": [["upload", "local/path", "/remote/path"]]
    The existing maintainer correction is unapplied.
  • .claude/skills/writing-fact-tests/SKILL.md describes arg as one argument list per call variant. Each fixture calls command() only once: lists are expanded as positional arguments, dicts as keyword arguments, and scalars as one argument. The documented semantics will produce incorrect command expectations.
  • .claude/skills/writing-facts/SKILL.md advertises default = None, but the executor unconditionally calls fact.default(). A None result requires the inherited or overridden default() method, not a None attribute.
  • Both implementation skills show unsafe f-string interpolation of user-controlled values, contrary to the StringCommand + QuoteString rule:
    yield f'gpg --dearmor -o "{dest}" "{src}"'
    return f"sha256sum {path} 2>/dev/null | cut -d' ' -f1"
    These examples teach shell injection and contradict the safety text in the operation skill itself.

Notes

  • AGENTS.md says gevent coverage concurrency is configured in pyproject.toml, but [tool.coverage.run] has no such setting, including at the PR head.
  • All three maintainer review threads remain unresolved, including the single-source/cross-harness design question. Maintainer input is still needed on that approach. The branch also conflicts with 3.x.

Done at PyCon FR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion Issues that need opinions! documentation Issues with docs/examples/docstrings.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants