Skip to content

docs: update contribution guidelines#204

Open
lipikaramaswamy wants to merge 5 commits into
mainfrom
lramaswamy/docs/update-contribution-guidelines
Open

docs: update contribution guidelines#204
lipikaramaswamy wants to merge 5 commits into
mainfrom
lramaswamy/docs/update-contribution-guidelines

Conversation

@lipikaramaswamy

Copy link
Copy Markdown
Collaborator

Related Issue

No linked issue required as this is contributor documentation and CI-policy cleanup.

Summary

Updates Anonymizer’s contributor guidance to separate contribution policy from day-to-day development workflow.

This PR:

  • Splits local setup, testing, docs, notebooks, and release workflow guidance into DEVELOPMENT.md
  • Refocuses CONTRIBUTING.md on contribution policy, PR expectations, branch naming, DCO, release tags, and agent-assisted development
  • Expands the PR template with checks for linked issues, plan PRs, public API / skill impact, PII fixtures, docs, notebooks, and validation notes
  • Adds a lightweight linked-issue check for external PRs
  • Fixes ci.yml so lint and advisory typecheck use a fixed Python version instead of referencing an undefined matrix value
  • Updates AGENTS.md and .copyrightignore for the new development guide

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Refactoring
  • CI, release, or contributor workflow update

Contributor Checklist

  • PR title follows Conventional Commits, for example fix: handle empty entity list
  • Related issue is linked, or the no-issue reason is documented above
  • For non-trivial changes, a plan PR is linked or not needed
  • Public API impact checked; skills/anonymizer/SKILL.md updated if needed
  • No real PII added to tests, docs, notebooks, fixtures, or artifacts

Testing

  • make test passes locally
  • make check passes locally
  • Added or updated tests, or explained why tests do not apply

Documentation and Artifacts

  • Docs updated, or not needed
  • If docs changed: make docs-build passes locally
  • If tutorial sources changed: notebooks regenerated with make convert-notebooks
  • If e2e, benchmark, or model-provider behavior changed: relevant validation is listed below

Validation Notes

Validation performed:

  • git diff --check
  • YAML parse of .github/workflows/*.yml
  • make check

make test was not run because this PR only changes contributor docs and CI policy wiring.
make docs-build was not run because this changes repository contributor Markdown, not the docs site content.

Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>
@lipikaramaswamy lipikaramaswamy requested a review from a team as a code owner July 2, 2026 06:21
Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR separates contributor documentation into policy (CONTRIBUTING.md) and day-to-day workflow (DEVELOPMENT.md), and adds two new GitHub Actions workflows plus targeted CI fixes.

  • ci.yml bug fix: lint and typecheck jobs referenced ${{ matrix.python-version }}, which is undefined in those non-matrix jobs; the fix pins them to a new DEFAULT_PYTHON_VERSION: "3.11" env var.
  • pr-linked-issue.yml: New pull_request_target workflow that verifies external PRs link a maintainer-triaged issue; uses printf '%s' "$PR_BODY" > file to handle untrusted input safely without checking out fork code. A retrigger job re-evaluates open PRs when a triaged label is applied.
  • pr-stale.yml: New scheduled workflow that posts reminders on PRs with prolonged failing checks and auto-closes external PRs after a grace period; correctly uses flatten in jq to handle --paginate --slurp nested arrays.
  • release.yml: Prerelease regex tightened to (^|[0-9])rc[0-9]+($|\+), matching actual PEP 440 rcN convention and the newly documented release policy.
  • .pre-commit-config.yaml: Adds detect-private-key hook to catch accidentally committed credentials at commit time.

Confidence Score: 5/5

Safe to merge — changes are documentation, CI policy wiring, and a targeted ci.yml bug fix with no production code touched.

All changed files are contributor docs, GitHub Actions workflows, and configuration. The ci.yml fix is a straightforward correction of an undefined matrix variable. The two new workflows handle PR metadata (body parsing, comment posting) without checking out fork code, and follow established safe patterns for pull_request_target usage. No application logic is modified.

No files require special attention.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Bug fix: lint and advisory-typecheck jobs now use DEFAULT_PYTHON_VERSION=3.11 instead of an undefined matrix variable, correcting a long-standing misconfiguration.
.github/workflows/pr-linked-issue.yml New workflow using pull_request_target; safely handles untrusted PR body via env-var to printf to file pattern without checking out fork code. Retrigger-on-triage path edits PR body via --body-file, never interpolating user content into shell commands.
.github/workflows/pr-stale.yml New scheduled stale-PR workflow; uses GNU date -d (ubuntu-latest compatible), flatten before jq traversal handles paginated --slurp arrays correctly, and temp-file pattern correctly escapes the pipe-subshell boundary for PR number collection.
.github/workflows/release.yml Prerelease-detection regex narrowed from a loose (rc
CONTRIBUTING.md Refocused on contribution policy and PR expectations; setup/testing/docs content moved to DEVELOPMENT.md. Release tag convention now matches actual repo practice (v-prefix required, rcN format).
DEVELOPMENT.md New file containing local setup, Makefile targets, validation steps, testing expectations, code quality, pre-commit, secrets guidance, docs/notebooks, and release workflow reference.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PR opened / edited / synchronize] --> B{Repository owner == NVIDIA-NeMo?}
    B -- No --> Z[Skip]
    B -- Yes --> C[Check author permission via GitHub API]
    C --> D{is_collaborator?}
    D -- Yes --> E[Delete existing linked-issue comment, status = pass]
    D -- No --> F[Run parser self-tests on hardcoded cases]
    F --> G[Parse PR body: remove HTML comments, extract fixes/closes/resolves N]
    G --> H{issue_num found?}
    H -- No --> I[Post fail comment: no linked issue]
    H -- Yes --> J[Validate issue exists and has triaged label]
    J --> K{issue_exists?}
    K -- No --> L[Post fail comment: issue not found]
    K -- Yes --> M{is_triaged?}
    M -- No --> N[Post fail comment: needs triaged label]
    M -- Yes --> O[Delete any fail comment, status = pass]
    I & L & N --> P[exit 1, check fails]
    O & E --> Q[Check passes]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[PR opened / edited / synchronize] --> B{Repository owner == NVIDIA-NeMo?}
    B -- No --> Z[Skip]
    B -- Yes --> C[Check author permission via GitHub API]
    C --> D{is_collaborator?}
    D -- Yes --> E[Delete existing linked-issue comment, status = pass]
    D -- No --> F[Run parser self-tests on hardcoded cases]
    F --> G[Parse PR body: remove HTML comments, extract fixes/closes/resolves N]
    G --> H{issue_num found?}
    H -- No --> I[Post fail comment: no linked issue]
    H -- Yes --> J[Validate issue exists and has triaged label]
    J --> K{issue_exists?}
    K -- No --> L[Post fail comment: issue not found]
    K -- Yes --> M{is_triaged?}
    M -- No --> N[Post fail comment: needs triaged label]
    M -- Yes --> O[Delete any fail comment, status = pass]
    I & L & N --> P[exit 1, check fails]
    O & E --> Q[Check passes]
Loading

Reviews (2): Last reviewed commit: "ci: count review replies in stale PR act..." | Re-trigger Greptile

Comment thread DEVELOPMENT.md
Comment thread DEVELOPMENT.md
Comment thread CONTRIBUTING.md

Copy link
Copy Markdown
Collaborator

I did a lightweight pass over the PR on its own. The documentation split is coherent, the PR template covers the relevant contributor checks, and the CI Python-version fix addresses the undefined matrix reference. The linked-issue workflow is also narrowly scoped and safe: it is read-only and never checks out fork code. Its main limitation is that it validates only body text, not whether the referenced issue actually exists.

I also reviewed this against DD’s setup. DD requires a real, maintainer-triaged issue, provides explanatory comments, and has stale-PR reminders and automatic closure. Anonymizer’s version is more of a nudge, so maintainers would still need to triage and close spam manually. If that is intentionally enough for now, that seems reasonable, but since we are already addressing the problem, should we consider porting the scalable parts here or tracking them as a follow-up?

One operational detail: the new check is not currently required by Anonymizer’s main ruleset, so it will remain advisory unless that is configured in repository settings after merge.

@binaryaaron binaryaaron 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.

agent (review-pr): Request changes based on the current PR head and repository configuration.

Linked-issue enforcement is bypassable

.github/workflows/pr-linked-issue.yml:26 scans the raw PR body for closing-keyword text. The default PR template includes Fixes #123, Closes #123, and Resolves #123 inside an HTML comment, so an external PR can leave the template untouched and still pass the new check.

The exemption check also accepts No linked issue required: with no rationale.

Please strip HTML comments before matching, require non-whitespace rationale text, and add focused cases for an untouched template body, a visible issue reference, an empty exemption, and a nonempty exemption. If this policy is intentionally limited to external PRs, make that scope explicit in the contributor guidance.

Validation instructions need to reflect actual checks

DEVELOPMENT.md says pre-commit runs ty, but the ty hook in .pre-commit-config.yaml is commented out. We have follow-up work to enable ty by default. Until then, please either remove ty from the hook description or state that make typecheck is advisory and not run automatically by pre-commit.

The PR template also asks every contributor to attest that make test and make check pass, while the new development guide correctly makes validation scope-dependent. Replace those universal boxes with an evidence-oriented field, for example:

Changed scope Command actually run Result or reason skipped

This lets contributors record relevant validation without claiming unrun work passed.

Keep ownership and policy text aligned with sources of truth

CONTRIBUTING.md says only src and tests go to @NVIDIA-NeMo/anonymizer-reviewers, but .github/CODEOWNERS also assigns docs/ there. Include docs in the summary or link directly to CODEOWNERS.

The plan-PR checklist permits “plan PR linked or not needed” without a link or reason. Require either the plan URL or a short, policy-defined exemption rationale.

Clarify prerelease tags before documenting them as valid

The release guide calls tags such as v1.0.0-alpha valid, while the project uses PEP 440 dynamic versioning and existing prerelease tags use forms such as v0.1.0rc1. The release workflow’s prerelease detection recognizes rc, alpha, beta, and .dev, but not normalized aN or bN forms.

Document the canonical supported tag syntax and make the release detector match it.

Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>

lipikaramaswamy commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Here's everything that I changed in response to feedback.

  • Replaced the lightweight linked-issue workflow with a DD-style pull_request_target workflow that does not check out PR code, exempts collaborators, strips HTML comments, verifies the linked item is a real issue, requires the triaged label, posts explanatory bot comments, and rechecks PRs when triaged is added.
  • Added a stale PR workflow: failing stale PR reminders apply to all PRs, while auto-close applies only to external/non-collaborator PRs. Maintainers can use keep-open to suppress reminders/auto-close.
  • Updated contributor docs and the PR template to describe the stronger external-contributor policy, the triaged and keep-open labels, and the new validation-note format.
  • Fixed the PR-template validation guidance so contributors list the relevant command subset they actually ran instead of checking universal make test / make check boxes.
  • Corrected the pre-commit docs: ty is available via make typecheck / make check, but is not currently run as a pre-commit hook.
  • Aligned CODEOWNERS wording with the repo by including docs under @NVIDIA-NeMo/anonymizer-reviewers.
  • Clarified release tag syntax to stable tags and rcN tags only, e.g. v1.0.0 and v1.0.0rc1, and narrowed release prerelease detection to match rcN.
  • Changed “plan PR” language to “plan document”; plan docs default to plans/<issue-number>/<short-name>.md, while maintainers can accept No plan required: <reason> for small changes.

Intentional differences from DD / notes:

  • Collaborator PRs receive stale reminders only; they are not auto-closed.
  • Plan-document requirements are still maintainer-reviewed rather than mechanically enforced.

Validation run locally: workflow YAML parse, workflow bash syntax checks, linked-issue parser cases, git diff --check, and make check. make check still prints existing advisory ty diagnostics, which the Makefile ignores by design.

Comment thread .github/workflows/pr-stale.yml Outdated
Comment thread .github/workflows/pr-stale.yml Outdated

@binaryaaron binaryaaron 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.

agent (review-pr): Two automation gaps remain after the latest update

The inline-review activity fix in 6db9a4e looks right. Replies are now included, the API reads are paginated, and author activity still excludes maintainers and bots.

The linked-issue workflow still has a dead end. When a maintainer adds triaged, the retrigger job edits the PR body with GITHUB_TOKEN and expects a pull_request_target: edited run. GitHub suppresses edited workflow events created by that token. The bot then says the check is being re-evaluated, but no new run starts. The old result remains until a person edits the PR or pushes another commit. This also makes the automatic-recheck promise in CONTRIBUTING.md inaccurate.

The stale workflow also checks raw states for only FAILURE and ERROR. GitHub can report other unsuccessful outcomes, including TIMED_OUT, STARTUP_FAILURE, ACTION_REQUIRED, and STALE. gh pr checks exposes a bucket field for this exact classification. Filtering on the raw states causes some stuck PRs to be skipped as if they had no failing checks.

Implementation prompt:

Fix the linked-issue and stale-PR automation in PR 204.

1. Make adding the `triaged` label actually re-evaluate the linked-issue check without contributor action. Do not rely on a PR edit performed with `GITHUB_TOKEN`, because GitHub does not start `edited` workflow runs from that event. Use an authentication or event path that GitHub guarantees will trigger, or run the shared validation logic directly and update the PR-head result. Keep bot comments idempotent.

2. In `pr-stale.yml`, request `name,state,bucket` from `gh pr checks` and use the CLI's normalized bucket to identify failed checks. Count every `fail` bucket. Make an explicit choice for `cancel`, document it in the workflow, and keep `pass`, `pending`, and `skipping` out of the failure list.

3. Preserve the current pagination and activity behavior. Commits, author-authored issue comments, inline review comments, and thread replies must all contribute to `last_activity_epoch`. Maintainer and bot activity must remain excluded.

4. Update `CONTRIBUTING.md` and bot messages so they describe the behavior the workflows actually provide.

Add focused validation cases for:
- an external PR whose linked issue changes from untriaged to triaged, with the check updating without a human PR edit or push;
- `FAILURE`, `TIMED_OUT`, and `STARTUP_FAILURE` check results entering the stale path;
- the chosen behavior for `CANCELLED`;
- an inline author reply after a stale reminder clearing that reminder and preventing auto-close.

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.

3 participants