Skip to content

docs(AGENTS): codify recurring PR review feedback as rules#1700

Open
wowi42 wants to merge 4 commits intopyinfra-dev:3.xfrom
KalvadTech:docs/agents-md-review-rules
Open

docs(AGENTS): codify recurring PR review feedback as rules#1700
wowi42 wants to merge 4 commits intopyinfra-dev:3.xfrom
KalvadTech:docs/agents-md-review-rules

Conversation

@wowi42
Copy link
Copy Markdown
Collaborator

@wowi42 wowi42 commented Apr 26, 2026

Summary

Mined the last 100 closed PRs on this repo and surfaced the patterns reviewers raised more than once. Added them to AGENTS.md as explicit rules so future agents/contributors follow them up-front instead of learning them in review.

New / extended rules:

  • Shell safety extended to all user-controlled values regardless of type (ports, ints, paths) — reviewers have flagged unquoted ints as an injection risk.
  • Optional parameter defaults clarified to require T | None, not Optional[T].
  • Distinguish unset from empty, branch on if x is not None:, not truthy if x:, when a param is T | None.
  • FactBase typing, annotate command() / process() and don't re-wrap output (it's already a list[str]).
  • Fact "show file or empty" pattern, prefer ! test -e PATH || cat PATH over cat PATH 2>/dev/null || true so real cat errors aren't swallowed.
  • Reuse existing helpers in operations.util.file_utils before adding new chown/chmod/path utilities.
  • No assert in src/, python -O strips them; raise an explicit exception instead.
  • PR atomic scope, one change per PR; split unrelated __init__.py re-exports, drive-by refactors, and unrelated test edits.

Related issues

None.

Test plan

  • AGENTS.md renders correctly as Markdown locally.
  • No code changes — nothing to run for tests/lint.

Labels

Suggested: docs, meta.

Checklist

  • Based on 3.x
  • Focused on a single change (no unrelated refactors / churn)
  • Docs added or updated for user-facing changes

wowi42 added 3 commits April 26, 2026 14:59
Mined the last 100 closed PRs and added the patterns reviewers raised
more than once as explicit rules so future agents follow them up front:

- Shell safety extended to non-string user input (ports/ints).
- Optional params: T | None, not Optional[T].
- Distinguish unset from empty (`is not None` vs truthy).
- FactBase typing: annotate command/process; output is already a list.
- Prefer `! test -e PATH || cat PATH` over `cat PATH || true`.
- Reuse helpers in operations.util before adding new ones.
- No `assert` in src/ — raise an exception (python -O strips asserts).
- PR checklist: atomic scope (no drive-by refactors / __init__.py edits).
@DonDebonair
Copy link
Copy Markdown
Collaborator

I added a new ## Coding Conventions header in AGENTS.md. Some of the additions in this PR should probably be moved there as they are not specific to adding facts/operations.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants