From 2238ddfcfebfefd3a1a3a0cbd71c12670d824288 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Thu, 30 Jul 2026 22:41:52 -0700 Subject: [PATCH 1/3] fix(skills): preserve superseded PR attribution Signed-off-by: Carlos Villela --- .../SKILL.md | 9 +++- .../references/workflow-policy.md | 29 ++++++++++++ .../SKILL.md | 19 ++++++-- .../templates/verdict.md | 13 +++--- .../tiebreakers.md | 16 ++++--- test/maintainer-skills-policy.test.ts | 45 +++++++++++++++++++ 6 files changed, 117 insertions(+), 14 deletions(-) diff --git a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md index 9d08d6fe382..929dd1dc4ef 100644 --- a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md @@ -68,6 +68,13 @@ Flag a PR when its body contains one of these phrases: - `follow-up to #NNN` / `supersedes #NNN` / `replaces #NNN` / `folds in #NNN` The phrase must name another open candidate PR. It indicates that one PR can include the other. +It records a relationship but does not prove that the target carries the source PR's work. + +When the target claims the source PR's full scope, compare their commits and diffs. +If material code, tests, or documentation from another contributor remains in the target, +apply the canonical policy in +`../nemoclaw-maintainer-policies/references/workflow-policy.md`. +Do not recommend closing the source PR until the target contains the required attribution. ## Step 5: Present results @@ -92,7 +99,7 @@ For superseded PRs: ### Superseded PRs - #1416 supersedes/folds in #1392 (shell-quote sandboxName) - Consider closing #1392 if #1416 contains its full scope. + Consider closing #1392 only if #1416 contains its full scope and preserves any required contributor attribution. ``` ### Clean candidates diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md b/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md index 4a0e4b6736e..97bd04d29c3 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md @@ -46,6 +46,35 @@ The contributor must correct a failure. Maintainers must reject a PR with an unverified commit or no DCO declaration. Do not merge, approve, or repair it for the contributor. +## Superseded PR Attribution + +A supersession declaration records a relationship between PRs. +It does not prove that one PR covers the other, satisfies contributor requirements, or ranks above another candidate. + +When a replacement PR carries material code, tests, or documentation from another contributor's PR: + +- Add `Supersedes #` to the replacement PR body and identify the transferred contribution. +- Preserve the source contributor as the Git author when cherry-picking their commit. +- When the replacement combines or reconstructs their work, add a + `Co-authored-by: Name ` commit trailer for each contributor whose work remains. + Use the exact author name and email from the source commit. Never guess or substitute an attribution identity. +- Confirm that the source PR already contains that contributor's `Signed-off-by:` declaration. + Never add or copy a DCO declaration on another contributor's behalf. +- Keep the replacement author's own DCO declaration in the replacement PR body. +- Before recommending merge or closure, verify the replacement PR body and commit metadata, + and confirm that every replacement commit appears as `Verified` in GitHub. + +If the source commit has no usable author identity, leave the winner unset and ask the contributor to provide machine-readable attribution. +If the source PR has no contributor DCO declaration, leave the winner unset and ask the contributor to add it. +A maintainer must not supply either declaration on the contributor's behalf. + +An independent implementation based only on the issue, a reproduction, or public discussion does not require co-authorship. +Still link the related PRs when recommending closure so the decision remains discoverable. +A comment on the superseded PR does not replace attribution in the merged PR history. + +This policy does not authorize a merge, close, comment, or other write. +Each operation still requires its own authorization. + ## Issue Classification Native GitHub Issue Type is the canonical issue-kind field: diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md index 8a433707174..8e48962ba29 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md @@ -20,6 +20,8 @@ Tier 3 resolves ties. If no PR passes Tier 0, rank eligible PRs for salvage. ## Repo policy The defaults use NemoClaw conventions for CODEOWNERS, DCO, CodeRabbit, and `docs/`. +Read the canonical superseded-PR attribution policy in +`../nemoclaw-maintainer-policies/references/workflow-policy.md`. Edit `repo-policy.md` for another repository. ## Workflow @@ -30,7 +32,7 @@ Copy this checklist into your response and check off each step: PR Comparison Progress: - [ ] Step 1: Parse issue (body + comments) for acceptance criteria - [ ] Step 2: Discover candidate PRs in the defined order -- [ ] Step 3: Detect supersession (parse PR bodies) +- [ ] Step 3: Detect supersession and classify transferred work - [ ] Step 4: Run Tier 0 gates per PR - [ ] Step 5: Run Tier 1 correctness checks per PR - [ ] Step 6: Run Tier 2 quality checks per PR @@ -57,14 +59,24 @@ scripts/find-candidates.sh Applies a single default order with stop conditions. -### Step 3: Detect supersession +### Step 3: Detect supersession and transferred work ```bash scripts/parse-supersession.sh ... ``` Parse these statements from each PR body: `supersedes #N`, `replaces #N`, `closes in favor of #N`, and `folds in #N`. -Use supersession as the first tiebreaker. +These statements record a relationship. +They do not rank a candidate or prove that its diff contains another contributor's work. + +For each declared or suspected replacement, compare the commits and diffs and classify the relationship: + +- `independent`: The PR implements the issue without carrying material code, tests, or documentation from another contributor. +- `transferred`: The PR carries material work from another contributor. +- `unclear`: The available evidence does not establish whether another contributor's work remains. + +For `transferred`, apply the canonical superseded-PR attribution policy before setting a winner or recommending that the source PR be closed. +For `unclear`, leave `winner` null and request maintainer judgment. ### Step 4: Tier 0 gates @@ -102,6 +114,7 @@ Apply the four model checks in `checks/tier-2-quality.md`. Compute the mode from the Tier 0 results. Do not accept a mode from the caller. In happy mode, set `winner` only to an eligible PR and set `closest_to_ready` to null. Leave `winner` null when the evidence does not support a merge recommendation. +Do not set `winner` for a replacement with transferred work until the required attribution is present and verified. In degraded mode, set `winner` to null. Set `closest_to_ready` only to an open PR that passes contributor requirements. See `tiebreakers.md`. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md index ec5b03c06be..4f897e32ad5 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md @@ -46,23 +46,26 @@ Render the final scorecard with `scripts/render-verdict.py`. Below is the human- | | covered | missing | | | missing | covered | -### Verdict: MERGE PR #A +### Verdict: No clear winner — see scorecard for recommended action Reasoning trace: - PR #B failed Tier 0. Check `` failed on `` after the force-push. - PR #A scored 14.5. PR #B scored 9.0. -- PR #A misses criterion 3. Cherry-pick PR #B's test at `:` to cover it. +- PR #A misses criterion 3. PR #B contains the test at `:`, so PR #A needs a transfer before it can be selected. ### Suggested action -1. Merge PR #A. -2. Cherry-pick the test from PR #B at `:` to cover criterion 3. -3. Close PR #B with a comment that links to #A and records the cherry-pick. +1. Transfer the test from PR #B before merge. Prefer `git cherry-pick -S -x ` so the source contributor remains the Git author. +2. If the work must be combined or reconstructed, add a `Co-authored-by: Name ` trailer using the exact author identity from the source commit. Never guess it. If no usable identity exists, leave the winner unset and ask the contributor. +3. Add `Supersedes #B` to PR #A's body and identify the transferred test. +4. Verify the updated commits, attribution, and CI, then run the comparator again on the updated SHA. +5. Merge PR #A only if the new verdict selects it. Then close PR #B with a comment that links to #A. ### Reasoning evidence - CI: all 12 required checks passed on PR #A commit ``. On PR #B commit ``, `test-cli` failed at ``. - Tier 1.1 PR #A: The test at `:` asserts on ``. The previous code returned ``, so the assertion would have failed. - Tier 1.3 PR #A fail: no test for empty-input edge case despite issue commenter raising it at `issue.comment.4` +- Attribution: PR #A carries `` from PR #B. Its body names the source PR, and commit `` preserves `` as Git author or co-author. - ... ... ``` diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md index b9eba1b6c11..e4395c451ed 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md @@ -23,13 +23,18 @@ Eliminate any PR failing Tier 0. Among survivors: Set the winner when the evidence distinguishes a PR. Otherwise, leave `winner` null. +### Supersession relationship + +A supersession statement records a relationship between candidates. +It does not prove coverage or attribution, and it does not rank a candidate. +Classify transferred work and complete the canonical attribution checks before recommending the replacement PR. + ### Tiebreakers (in order) -1. **Supersession.** Prefer a PR whose body states that it supersedes another candidate. See `scripts/parse-supersession.sh`. -2. **Smaller diff.** Prefer the smaller diff when both PRs cover the issue scope. -3. **Better edge-case test coverage.** Compare Tier 1.3 (negative test coverage) outputs. -4. **Most recent activity.** Prefer the PR with the most recent commit. -5. **Lower PR number.** Use the lower PR number if the PRs remain tied. +1. **Smaller diff.** Prefer the smaller diff when both PRs cover the issue scope. +2. **Better edge-case test coverage.** Compare Tier 1.3 (negative test coverage) outputs. +3. **Most recent activity.** Prefer the PR with the most recent commit. +4. **Lower PR number.** Use the lower PR number if the PRs remain tied. ## Degraded mode (no PR passes Tier 0) @@ -68,5 +73,6 @@ For each acceptance criterion (from issue body + comments), build a row showing Use the matrix to find tests or changes that the selected PR does not include. The verdict can recommend a small transfer from another PR. +Complete the transfer and required attribution, then rerun the comparator before selecting a winner. Per-criterion winner cells: `covered` (full), `partial` (yellow), `missing` (red). diff --git a/test/maintainer-skills-policy.test.ts b/test/maintainer-skills-policy.test.ts index 2671c48e408..728fc7026ae 100644 --- a/test/maintainer-skills-policy.test.ts +++ b/test/maintainer-skills-policy.test.ts @@ -321,6 +321,51 @@ describe("maintainer skills follow canonical workflow policy", () => { ); }); + it("requires replacement PRs to preserve transferred contributor attribution", () => { + const policy = read( + ".agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md", + ); + const comparator = read(".agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md"); + const tiebreakers = read(".agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md"); + const verdict = read(".agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md"); + const finder = read(".agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md"); + + expect(policy).toContain("Supersedes #"); + expect(policy).toContain("Preserve the source contributor as the Git author"); + expect(policy).toContain("Co-authored-by: Name "); + expect(policy).toContain("Use the exact author name and email from the source commit"); + expect(policy).toContain("Never guess or substitute an attribution identity"); + expect(policy).toContain("Never add or copy a DCO declaration"); + expect(policy).toContain("leave the winner unset and ask the contributor"); + expect(policy).toContain("does not require co-authorship"); + expect(policy).toContain("does not replace attribution in the merged PR history"); + + expect(comparator).toContain("../nemoclaw-maintainer-policies/references/workflow-policy.md"); + expect(comparator).toContain("They do not rank a candidate"); + expect(comparator).toContain("`transferred`"); + expect(comparator).toContain("`unclear`"); + expect(comparator).toContain("leave `winner` null"); + + expect(tiebreakers).toContain("it does not rank a candidate"); + expect(tiebreakers).not.toContain("**Supersession.**"); + expect(tiebreakers).toContain("rerun the comparator before selecting a winner"); + + expect(verdict).toContain("git cherry-pick -S -x "); + expect(verdict).toContain("Co-authored-by: Name "); + expect(verdict).toContain("using the exact author identity from the source commit"); + expect(verdict).toContain("run the comparator again on the updated SHA"); + expect(verdict).toContain("Transfer the test from PR #B before merge"); + expect(verdict).toContain("Merge PR #A only if the new verdict selects it"); + expect(verdict.indexOf("Transfer the test from PR #B before merge")).toBeLessThan( + verdict.indexOf("Merge PR #A only if the new verdict selects it"), + ); + + expect(finder).toContain("../nemoclaw-maintainer-policies/references/workflow-policy.md"); + expect(finder).toContain( + "Do not recommend closing the source PR until the target contains the required attribution", + ); + }); + it("keeps PR workflow writes behind their safety checks", () => { const createPr = read(".agents/skills/nemoclaw-contributor-create-pr/SKILL.md"); const judgment = read( From 9219d9c3d45a5db75d9a2898f4295339e29baaec Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Thu, 30 Jul 2026 23:09:38 -0700 Subject: [PATCH 2/3] fix(skills): tighten supersession workflow gates Signed-off-by: Carlos Villela --- .../SKILL.md | 26 ++++++++-- .../references/workflow-policy.md | 19 +++++--- .../SKILL.md | 4 +- .../templates/verdict.md | 15 ++++-- test/maintainer-skills-policy.test.ts | 48 ++++++++++++++++--- 5 files changed, 88 insertions(+), 24 deletions(-) diff --git a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md index 929dd1dc4ef..51d70028144 100644 --- a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md @@ -63,18 +63,33 @@ gh pr view --json number,title,author,createdAt,additions,deletions,rev ## Step 4: Check for superseded PRs -Flag a PR when its body contains one of these phrases: +Run the comparator's canonical detector with the open candidate PR numbers: -- `follow-up to #NNN` / `supersedes #NNN` / `replaces #NNN` / `folds in #NNN` +```bash +../nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh ... +``` + +It recognizes `supersedes #N`, `replaces #N`, `closes in favor of #N`, +`closed in favor of #N`, and `folds in #N`. +A `follow-up to #N` statement is a related-PR signal, not a supersession declaration, +unless one of these phrases also appears. -The phrase must name another open candidate PR. It indicates that one PR can include the other. +Each supersession phrase must name another open candidate PR. It indicates that one PR can include the other. It records a relationship but does not prove that the target carries the source PR's work. When the target claims the source PR's full scope, compare their commits and diffs. If material code, tests, or documentation from another contributor remains in the target, apply the canonical policy in `../nemoclaw-maintainer-policies/references/workflow-policy.md`. -Do not recommend closing the source PR until the target contains the required attribution. + +This skill reports recommendations only. +Do not recommend closing the source PR until another authorized workflow has: + +- completed any required transfer +- verified the updated commits, attribution, and CI +- rerun the comparator and selected the target +- confirmed that the target contains the source's full scope and required contributor attribution +- merged the selected target ## Step 5: Present results @@ -99,7 +114,8 @@ For superseded PRs: ### Superseded PRs - #1416 supersedes/folds in #1392 (shell-quote sandboxName) - Consider closing #1392 only if #1416 contains its full scope and preserves any required contributor attribution. + Keep #1392 open while an authorized workflow completes any required transfer, verifies the updated commits, attribution, and CI, and reruns the comparator. + After the updated verdict selects #1416 and #1416 merges, consider closing #1392 only if #1416 contains its full scope and preserves any required contributor attribution. ``` ### Clean candidates diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md b/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md index 97bd04d29c3..d0ab9d249d4 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md @@ -53,21 +53,26 @@ It does not prove that one PR covers the other, satisfies contributor requiremen When a replacement PR carries material code, tests, or documentation from another contributor's PR: +- Confirm that the source PR already contains that contributor's `Signed-off-by:` declaration. + Never add or copy a DCO declaration on another contributor's behalf. +- Use the exact author name and email from the source commit. + Never guess or substitute an attribution identity. + +If the source commit has no usable author identity, leave the winner unset and ask the contributor to provide machine-readable attribution. +If the source PR has no contributor DCO declaration, leave the winner unset and ask the contributor to add it. +A maintainer must not supply either declaration on the contributor's behalf. + +After both checks pass: + - Add `Supersedes #` to the replacement PR body and identify the transferred contribution. - Preserve the source contributor as the Git author when cherry-picking their commit. - When the replacement combines or reconstructs their work, add a `Co-authored-by: Name ` commit trailer for each contributor whose work remains. - Use the exact author name and email from the source commit. Never guess or substitute an attribution identity. -- Confirm that the source PR already contains that contributor's `Signed-off-by:` declaration. - Never add or copy a DCO declaration on another contributor's behalf. + Use the verified source-commit identity. - Keep the replacement author's own DCO declaration in the replacement PR body. - Before recommending merge or closure, verify the replacement PR body and commit metadata, and confirm that every replacement commit appears as `Verified` in GitHub. -If the source commit has no usable author identity, leave the winner unset and ask the contributor to provide machine-readable attribution. -If the source PR has no contributor DCO declaration, leave the winner unset and ask the contributor to add it. -A maintainer must not supply either declaration on the contributor's behalf. - An independent implementation based only on the issue, a reproduction, or public discussion does not require co-authorship. Still link the related PRs when recommending closure so the decision remains discoverable. A comment on the superseded PR does not replace attribution in the merged PR history. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md index 8e48962ba29..74af46379a0 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md @@ -65,7 +65,9 @@ Applies a single default order with stop conditions. scripts/parse-supersession.sh ... ``` -Parse these statements from each PR body: `supersedes #N`, `replaces #N`, `closes in favor of #N`, and `folds in #N`. +Parse the canonical statement set implemented by `scripts/parse-supersession.sh`: +`supersedes #N`, `replaces #N`, `closes in favor of #N`, `closed in favor of #N`, and `folds in #N`. +A `follow-up to #N` statement is a related-PR signal, not a supersession declaration, unless one of these phrases also appears. These statements record a relationship. They do not rank a candidate or prove that its diff contains another contributor's work. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md index 4f897e32ad5..9bb9543bc0b 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md @@ -55,11 +55,16 @@ Reasoning trace: ### Suggested action -1. Transfer the test from PR #B before merge. Prefer `git cherry-pick -S -x ` so the source contributor remains the Git author. -2. If the work must be combined or reconstructed, add a `Co-authored-by: Name ` trailer using the exact author identity from the source commit. Never guess it. If no usable identity exists, leave the winner unset and ask the contributor. -3. Add `Supersedes #B` to PR #A's body and identify the transferred test. -4. Verify the updated commits, attribution, and CI, then run the comparator again on the updated SHA. -5. Merge PR #A only if the new verdict selects it. Then close PR #B with a comment that links to #A. +1. Confirm that PR #B contains the contributor's `Signed-off-by:` declaration. Do not add or copy that declaration on the contributor's behalf. If it is absent, leave the winner unset and ask the contributor. +2. Read the exact author name and email from the source commit. Never guess or substitute it. If no usable identity exists, leave the winner unset and ask the contributor. +3. Transfer the test from PR #B before merge. Prefer `git cherry-pick -S -x ` so the source contributor remains the Git author. +4. If the work must be combined or reconstructed, add a `Co-authored-by: Name ` trailer using the verified source-commit identity. +5. Add `Supersedes #B` to PR #A's body and identify the transferred test. +6. Keep the replacement author's own DCO declaration in PR #A's body. +7. Confirm that every replacement commit appears as `Verified` in GitHub. +8. Verify the updated commits, attribution, and CI, then run the comparator again on the updated SHA. +9. Merge PR #A only if the new verdict selects it. +10. After PR #A merges, close PR #B with a comment that links to #A. ### Reasoning evidence - CI: all 12 required checks passed on PR #A commit ``. On PR #B commit ``, `test-cli` failed at ``. diff --git a/test/maintainer-skills-policy.test.ts b/test/maintainer-skills-policy.test.ts index 728fc7026ae..178b2ada454 100644 --- a/test/maintainer-skills-policy.test.ts +++ b/test/maintainer-skills-policy.test.ts @@ -337,6 +337,10 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(policy).toContain("Never guess or substitute an attribution identity"); expect(policy).toContain("Never add or copy a DCO declaration"); expect(policy).toContain("leave the winner unset and ask the contributor"); + const sourceDcoPolicyIndex = policy.indexOf("Confirm that the source PR already contains"); + const transferPolicyIndex = policy.indexOf("After both checks pass"); + expect(sourceDcoPolicyIndex).toBeGreaterThanOrEqual(0); + expect(transferPolicyIndex).toBeGreaterThan(sourceDcoPolicyIndex); expect(policy).toContain("does not require co-authorship"); expect(policy).toContain("does not replace attribution in the merged PR history"); @@ -345,6 +349,19 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(comparator).toContain("`transferred`"); expect(comparator).toContain("`unclear`"); expect(comparator).toContain("leave `winner` null"); + expect(finder).toContain("../nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh"); + for (const pattern of [ + "supersedes #N", + "replaces #N", + "closes in favor of #N", + "closed in favor of #N", + "folds in #N", + ]) { + expect(comparator).toContain(pattern); + expect(finder).toContain(pattern); + } + expect(comparator).toContain("A `follow-up to #N` statement is a related-PR signal"); + expect(finder).toContain("A `follow-up to #N` statement is a related-PR signal"); expect(tiebreakers).toContain("it does not rank a candidate"); expect(tiebreakers).not.toContain("**Supersession.**"); @@ -352,18 +369,37 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(verdict).toContain("git cherry-pick -S -x "); expect(verdict).toContain("Co-authored-by: Name "); - expect(verdict).toContain("using the exact author identity from the source commit"); + expect(verdict).toContain("using the verified source-commit identity"); expect(verdict).toContain("run the comparator again on the updated SHA"); - expect(verdict).toContain("Transfer the test from PR #B before merge"); - expect(verdict).toContain("Merge PR #A only if the new verdict selects it"); - expect(verdict.indexOf("Transfer the test from PR #B before merge")).toBeLessThan( - verdict.indexOf("Merge PR #A only if the new verdict selects it"), + expect(verdict).toContain("contains the contributor's `Signed-off-by:` declaration"); + expect(verdict).toContain("Do not add or copy that declaration"); + expect(verdict).toContain("Keep the replacement author's own DCO declaration"); + expect(verdict).toContain("every replacement commit appears as `Verified` in GitHub"); + + const sourceDcoIndex = verdict.indexOf("Confirm that PR #B contains the contributor's"); + const identityIndex = verdict.indexOf( + "Read the exact author name and email from the source commit", ); + const transferIndex = verdict.indexOf("Transfer the test from PR #B before merge"); + const rerunIndex = verdict.indexOf("run the comparator again on the updated SHA"); + const mergeIndex = verdict.indexOf("Merge PR #A only if the new verdict selects it"); + const closeIndex = verdict.indexOf("After PR #A merges, close PR #B"); + + expect(sourceDcoIndex).toBeGreaterThanOrEqual(0); + expect(identityIndex).toBeGreaterThanOrEqual(0); + expect(transferIndex).toBeGreaterThan(sourceDcoIndex); + expect(transferIndex).toBeGreaterThan(identityIndex); + expect(rerunIndex).toBeGreaterThan(transferIndex); + expect(mergeIndex).toBeGreaterThan(rerunIndex); + expect(closeIndex).toBeGreaterThan(mergeIndex); expect(finder).toContain("../nemoclaw-maintainer-policies/references/workflow-policy.md"); + expect(finder).toContain("This skill reports recommendations only"); expect(finder).toContain( - "Do not recommend closing the source PR until the target contains the required attribution", + "Do not recommend closing the source PR until another authorized workflow", ); + expect(finder).toContain("merged the selected target"); + expect(finder).toContain("After the updated verdict selects #1416 and #1416 merges"); }); it("keeps PR workflow writes behind their safety checks", () => { From 52fdd4a8a58f233570fb224d08aa6854a8ebc4a2 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Thu, 30 Jul 2026 23:34:14 -0700 Subject: [PATCH 3/3] fix(skills): orient supersession references Signed-off-by: Carlos Villela --- .../SKILL.md | 10 ++- .../SKILL.md | 10 ++- .../scripts/parse-supersession.sh | 31 +++++-- test/maintainer-skills-policy.test.ts | 88 +++++++++++++++++-- 4 files changed, 123 insertions(+), 16 deletions(-) diff --git a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md index 51d70028144..2d6da9f81d0 100644 --- a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md @@ -69,8 +69,14 @@ Run the comparator's canonical detector with the open candidate PR numbers: ../nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh ... ``` -It recognizes `supersedes #N`, `replaces #N`, `closes in favor of #N`, -`closed in favor of #N`, and `folds in #N`. +It recognizes the comparator parser's case-insensitive statement families: + +- `supersed[a-z]*` before `#N`: `supersedes #N` points from the current PR to `#N`; `superseded by #N` points from `#N` to the current PR. +- `replac[a-z]*` before `#N`: `replaces #N` points from the current PR to `#N`; `replaced by #N` points from `#N` to the current PR. +- `clos[a-z]* in favor of` before `#N`: `closes in favor of #N` and `closed in favor of #N` point from `#N` to the current PR. +- `fold[a-z]* in` before `#N`: `folds in #N` points from the current PR to `#N`; `folded into #N` points from `#N` to the current PR. + +The bracket expressions describe the parser grammar; they are not literal PR body text. A `follow-up to #N` statement is a related-PR signal, not a supersession declaration, unless one of these phrases also appears. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md index 74af46379a0..9112b1033e3 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md @@ -65,8 +65,14 @@ Applies a single default order with stop conditions. scripts/parse-supersession.sh ... ``` -Parse the canonical statement set implemented by `scripts/parse-supersession.sh`: -`supersedes #N`, `replaces #N`, `closes in favor of #N`, `closed in favor of #N`, and `folds in #N`. +Parse the case-insensitive statement families implemented by `scripts/parse-supersession.sh`: + +- `supersed[a-z]*` before `#N`: `supersedes #N` points from the current PR to `#N`; `superseded by #N` points from `#N` to the current PR. +- `replac[a-z]*` before `#N`: `replaces #N` points from the current PR to `#N`; `replaced by #N` points from `#N` to the current PR. +- `clos[a-z]* in favor of` before `#N`: `closes in favor of #N` and `closed in favor of #N` point from `#N` to the current PR. +- `fold[a-z]* in` before `#N`: `folds in #N` points from the current PR to `#N`; `folded into #N` points from `#N` to the current PR. + +The bracket expressions describe the parser grammar; they are not literal PR body text. A `follow-up to #N` statement is a related-PR signal, not a supersession declaration, unless one of these phrases also appears. These statements record a relationship. They do not rank a candidate or prove that its diff contains another contributor's work. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh index b41be9b3f0e..f262ac34bb2 100755 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh @@ -5,8 +5,10 @@ # Parse PR bodies for supersession references and emit edges: # -> # -# Patterns matched (case-insensitive): "supersedes #N", "replaces #N", -# "closes in favor of #N", "closed in favor of #N", "folds in #N". +# Patterns matched case-insensitively. Active forms such as "supersedes #N", +# "replaces #N", and "folds in #N" point from the current PR to #N. Passive +# forms such as "superseded by #N", "replaced by #N", "closed in favor of +# #N", and "folded into #N" point from #N to the current PR. # # Usage: parse-supersession.sh [...] [--repo OWNER/REPO] @@ -36,19 +38,34 @@ done candidates_set=$(printf '%s\n' "${prs[@]}" | sort -u) edges=() +append_edge() { + local candidate="$1" + local existing + + for existing in "${edges[@]}"; do + [ "$existing" = "$candidate" ] && return + done + edges+=("$candidate") +} + +reverse_pattern='(supersed[a-z]*[[:space:]]+by|replac[a-z]*[[:space:]]+by|clos[a-z]*[[:space:]]+in[[:space:]]+favor[[:space:]]+of|fold[a-z]*[[:space:]]+into)' for pr in "${prs[@]}"; do body=$(gh pr view "$pr" "${repo_args[@]}" --json body --jq .body 2>/dev/null || echo "") [ -z "$body" ] && continue - # Extract referenced PR numbers from supersession patterns. - while IFS= read -r ref; do + # Extract supersession statements and orient passive forms toward this PR. + while IFS= read -r statement; do + ref="${statement##*#}" # Only emit edges where the target is also a candidate. if printf '%s\n' "$candidates_set" | grep -q "^${ref}$"; then - edges+=("$pr -> $ref") + normalized=$(printf '%s' "$statement" | tr '[:upper:]' '[:lower:]') + if [[ $normalized =~ $reverse_pattern ]]; then + append_edge "$ref -> $pr" + else + append_edge "$pr -> $ref" + fi fi done < <(printf '%s' "$body" | grep -oiE '(supersed[a-z]*|replac[a-z]*|clos[a-z]* in favor of|fold[a-z]* in)[^#]*#([0-9]+)' \ - | grep -oE '#[0-9]+' \ - | tr -d '#' \ | sort -u) done diff --git a/test/maintainer-skills-policy.test.ts b/test/maintainer-skills-policy.test.ts index 178b2ada454..6dcc2097748 100644 --- a/test/maintainer-skills-policy.test.ts +++ b/test/maintainer-skills-policy.test.ts @@ -1,7 +1,9 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import { spawnSync } from "node:child_process"; import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; @@ -329,6 +331,9 @@ describe("maintainer skills follow canonical workflow policy", () => { const tiebreakers = read(".agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md"); const verdict = read(".agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md"); const finder = read(".agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md"); + const parser = read( + ".agents/skills/nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh", + ); expect(policy).toContain("Supersedes #"); expect(policy).toContain("Preserve the source contributor as the Git author"); @@ -351,15 +356,24 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(comparator).toContain("leave `winner` null"); expect(finder).toContain("../nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh"); for (const pattern of [ - "supersedes #N", - "replaces #N", - "closes in favor of #N", - "closed in favor of #N", - "folds in #N", + "supersed[a-z]*", + "replac[a-z]*", + "clos[a-z]* in favor of", + "fold[a-z]* in", ]) { + expect(parser).toContain(pattern); expect(comparator).toContain(pattern); expect(finder).toContain(pattern); } + for (const example of [ + "superseded by #N", + "replaced by #N", + "closed in favor of #N", + "folded into #N", + ]) { + expect(comparator).toContain(example); + expect(finder).toContain(example); + } expect(comparator).toContain("A `follow-up to #N` statement is a related-PR signal"); expect(finder).toContain("A `follow-up to #N` statement is a related-PR signal"); @@ -402,6 +416,70 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(finder).toContain("After the updated verdict selects #1416 and #1416 merges"); }); + it("orients active and passive supersession statements", () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "parse-supersession-")); + const bin = path.join(tmp, "bin"); + const mockGh = path.join(bin, "gh"); + fs.mkdirSync(bin); + fs.writeFileSync( + mockGh, + [ + "#!/usr/bin/env bash", + 'case "$3" in', + ' 100) printf "%s" "${PR_BODY_100:-}" ;;', + ' 200) printf "%s" "${PR_BODY_200:-}" ;;', + "esac", + ].join("\n"), + ); + fs.chmodSync(mockGh, 0o755); + + const parser = path.join( + root, + ".agents/skills/nemoclaw-maintainer-pr-comparator/scripts/parse-supersession.sh", + ); + const scenarios = [ + { statement: "Supersedes #200", superseder: 100, superseded: 200 }, + { statement: "Superseded by #200", superseder: 200, superseded: 100 }, + { statement: "Replaces #200", superseder: 100, superseded: 200 }, + { statement: "Replaced by #200", superseder: 200, superseded: 100 }, + { statement: "Closes in favor of #200", superseder: 200, superseded: 100 }, + { statement: "Closed in favor of #200", superseder: 200, superseded: 100 }, + { statement: "Folds in #200", superseder: 100, superseded: 200 }, + { statement: "Folded into #200", superseder: 200, superseded: 100 }, + { + statement: "Supersedes #200\nReplaces #200", + superseder: 100, + superseded: 200, + }, + ]; + + try { + for (const scenario of scenarios) { + const result = spawnSync("bash", [parser, "100", "200"], { + encoding: "utf8", + env: { + ...process.env, + PATH: `${bin}${path.delimiter}${process.env.PATH ?? ""}`, + PR_BODY_100: scenario.statement, + PR_BODY_200: "", + }, + }); + + expect(result.status).toBe(0); + expect(JSON.parse(result.stdout)).toEqual({ + edges: [ + { + superseder: scenario.superseder, + superseded: scenario.superseded, + }, + ], + }); + } + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + it("keeps PR workflow writes behind their safety checks", () => { const createPr = read(".agents/skills/nemoclaw-contributor-create-pr/SKILL.md"); const judgment = read(