docs: update contribution guidelines#204
Conversation
Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>
Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>
Greptile SummaryThis PR separates contributor documentation into policy (
Confidence Score: 5/5Safe 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
|
|
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 |
binaryaaron
left a comment
There was a problem hiding this comment.
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>
|
Here's everything that I changed in response to feedback.
Intentional differences from DD / notes:
Validation run locally: workflow YAML parse, workflow bash syntax checks, linked-issue parser cases, |
binaryaaron
left a comment
There was a problem hiding this comment.
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.
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:
DEVELOPMENT.mdCONTRIBUTING.mdon contribution policy, PR expectations, branch naming, DCO, release tags, and agent-assisted developmentci.ymlso lint and advisory typecheck use a fixed Python version instead of referencing an undefined matrix valueAGENTS.mdand.copyrightignorefor the new development guideType of Change
Contributor Checklist
fix: handle empty entity listskills/anonymizer/SKILL.mdupdated if neededTesting
make testpasses locallymake checkpasses locallyDocumentation and Artifacts
make docs-buildpasses locallymake convert-notebooksValidation Notes
Validation performed:
git diff --check.github/workflows/*.ymlmake checkmake testwas not run because this PR only changes contributor docs and CI policy wiring.make docs-buildwas not run because this changes repository contributor Markdown, not the docs site content.