diff --git a/.agents/skills/_shared/git-github-hard-stop.md b/.agents/skills/_shared/git-github-hard-stop.md index 7d23a1ae30e..cd342ae6869 100644 --- a/.agents/skills/_shared/git-github-hard-stop.md +++ b/.agents/skills/_shared/git-github-hard-stop.md @@ -1,20 +1,23 @@ -# Git and GitHub Access Hard Stop +# Stop for Git and GitHub Access Errors -Use this guardrail from any workflow that runs `git`, `ssh`, or `gh` commands. +Use this rule in each workflow that runs `git`, `ssh`, or `gh` commands. -If a Git/GitHub command fails because of authentication, authorization, missing credentials, SSO, token scope, SSH key setup, remote access, or push permissions, stop and ask the user to resolve access. +Stop if a Git or GitHub command has an access error. Access errors include authentication, authorization, credentials, SSO, token scope, SSH keys, remote access, and push permissions. +Ask the user to correct the access problem. -Do **not** work around access failures by: +Do not try to bypass an access error. Do not: -- switching remote protocols or remotes; -- editing credentials, tokens, or SSH config; -- generating new tokens or SSH keys; -- rewriting remotes to bypass permissions; -- force-pushing or bypassing branch protections/required checks. +- switch remote protocols or remotes +- edit credentials, tokens, or SSH config +- generate new tokens or SSH keys +- rewrite remotes to bypass permissions +- force-push or bypass branch protections or required checks. -Report the command, the relevant error output, and the next action needed from the user, then wait. +Report the command and the error. Tell the user which action is necessary. Then, wait. -This hard stop is for access/authentication/authorization problems only. Normal Git workflow problems such as merge conflicts, stale branches, dirty worktrees, or mechanical rebase conflicts should be handled by the relevant workflow. Stop for user guidance only when conflict resolution would change behavior, alter contributor intent, or require a design decision. +This rule applies only to access errors. +Handle merge conflicts, stale branches, dirty worktrees, and rebase conflicts in the related workflow. +Ask the user when a resolution can change behavior, contributor intent, or a design decision. diff --git a/.agents/skills/_shared/pr-follow-up.md b/.agents/skills/_shared/pr-follow-up.md index e8a77cf35a8..13c5d8d8c08 100644 --- a/.agents/skills/_shared/pr-follow-up.md +++ b/.agents/skills/_shared/pr-follow-up.md @@ -1,18 +1,18 @@ -# PR CI and Review Follow-Up +# Follow Up on PR CI and Reviews -Use this workflow after creating a PR and after every push to an open PR. +Use this workflow after you create a PR or push to an open PR. -## Watch checks +## Monitor checks ```bash PR_NUMBER=${PR_NUMBER:-$(gh pr view --json number -q .number)} gh pr checks "$PR_NUMBER" --watch ``` -Then inspect the settled check state: +When the checks stop, inspect their status: ```bash gh pr view "$PR_NUMBER" --json url,statusCheckRollup,comments,reviews,reviewDecision @@ -20,7 +20,7 @@ gh pr view "$PR_NUMBER" --json url,statusCheckRollup,comments,reviews,reviewDeci ## Review feedback -Check sticky PR comments and inline review comments from CodeRabbit and the PR Review Advisor: +Check PR comments and inline review comments from CodeRabbit and the PR Review Advisor: ```bash gh api "repos/NVIDIA/NemoClaw/issues/${PR_NUMBER}/comments" --paginate \ @@ -30,16 +30,26 @@ gh api "repos/NVIDIA/NemoClaw/pulls/${PR_NUMBER}/comments" --paginate \ --jq '.[] | select((.body // "") | test("CodeRabbit|coderabbit|PR Review Advisor|nemoclaw-pr-review-advisor"; "i")) | {author: .user.login, path, line, updated_at, body}' ``` -## Triage - -- Before acting on feedback, state the concrete problem and intended outcome. -- Do not add a generalized helper, configuration switch, fallback, migration, or compatibility path solely to satisfy reviewer wording. -- If feedback cannot be tied to a concrete defect, demonstrated security or data-safety risk, supported contract, or needless complexity in changed code, treat it as a suggestion rather than implementation work. -- **CI failure:** inspect the failing job logs, fix the root cause, rerun relevant local checks, commit, push, and monitor again. -- **CodeRabbit or PR Review Advisor correctness/security/test-coverage finding:** address it when valid, rerun relevant checks, commit, push, and monitor again. -- **Style nits or false positives:** avoid unnecessary churn. Note the rationale in your final report or comment on the PR when reviewer-visible context is useful. -- **Ambiguous, risky, broad, or design-changing feedback:** stop and consult the user before changing code. - -Repeat until required CI is green and there are no unresolved actionable CodeRabbit or PR Review Advisor findings, or until the user tells you to stop. - -If any follow-up push or `gh`/GitHub query hits SSH, authentication, remote access, authorization, or permission problems, follow [Git and GitHub Access Hard Stop](git-github-hard-stop.md). Do not stop for ordinary merge conflicts or dirty-worktree state; resolve mechanical conflicts in the relevant workflow and ask the user only when resolution would change behavior or contributor intent. +## Handle results + +- Apply [NemoClaw Technical English](../../../CONTRIBUTING.md#nemoclaw-technical-english) to review comments and proposed rewrites. +- During the changed-text pilot, block on language only when ambiguity can change behavior, security, data safety, test meaning, or release meaning. +- Treat other language findings as suggestions. Include a proposed rewrite and do not request unrelated cleanup. +- Before you act on feedback, state the problem and the intended result. +- Do not add a helper, configuration switch, fallback, migration, or compatibility path only to satisfy reviewer wording. +- Treat feedback as a suggestion if you cannot connect it to one of these conditions: + - A defect. + - A demonstrated security or data-safety risk. + - A supported contract. + - Unnecessary complexity in changed code. + - Ambiguity in changed text that can change behavior, security, data safety, test meaning, or release meaning. +- **CI failure:** Inspect the job logs and fix the cause. Run the related local checks. Commit, push, and monitor the PR again. +- **Valid CodeRabbit or PR Review Advisor finding:** Fix correctness, security, or test-coverage problems. Run the related checks. Commit, push, and monitor the PR again. +- **Style comment or false positive:** Avoid unnecessary changes. Explain your decision in the final report. Comment on the PR when reviewers need the explanation. +- **Ambiguous, risky, broad, or design-changing feedback:** Stop and ask the user before you change code. + +Repeat this workflow until required CI passes and no actionable automated-review findings remain. Stop if the user tells you to stop. + +If a push or GitHub query has an access error, follow [Git and GitHub Access Hard Stop](git-github-hard-stop.md). +Resolve merge conflicts and dirty-worktree problems in the PR workflow. +Ask the user when a resolution can change behavior or contributor intent. diff --git a/.agents/skills/nemoclaw-contributor-create-pr/SKILL.md b/.agents/skills/nemoclaw-contributor-create-pr/SKILL.md index 6248846b358..9aabd7498b4 100644 --- a/.agents/skills/nemoclaw-contributor-create-pr/SKILL.md +++ b/.agents/skills/nemoclaw-contributor-create-pr/SKILL.md @@ -1,22 +1,27 @@ --- name: nemoclaw-contributor-create-pr -description: Create GitHub pull requests that follow the NemoClaw PR template, then monitor CI and automated review feedback. Use when the user wants to create a new PR, submit code for review, open a pull request, or push changes for review. Trigger keywords - create PR, pull request, new PR, submit for review, open PR, push for review. +description: Create a GitHub pull request with the NemoClaw template. Then, monitor CI and automated reviews. Use this skill when the user asks to create, open, push, or submit a PR for review. Trigger keywords - create PR, pull request, new PR, submit for review, open PR, push for review. --- + + + # Create GitHub Pull Request Create NemoClaw pull requests with the `gh` CLI and the project's PR template. ## Prerequisites -- The `gh` CLI must be authenticated (`gh auth status`). -- You must be in the NemoClaw git repository. -- You must have commits on a branch that is pushed to the remote. -- The PR description must include a valid DCO `Signed-off-by:` declaration, and every commit that will appear in the PR must appear as `Verified` in GitHub. +- Authenticate the `gh` CLI (`gh auth status`). +- Work in the NemoClaw Git repository. +- Put the commits on a feature branch. +- Add the contributor's DCO `Signed-off-by:` declaration to the PR description. +- Make sure that GitHub shows each PR commit as `Verified`. ## Hard Stop: Git, SSH, and Authentication Problems -Follow the shared [Git and GitHub Access Hard Stop](../_shared/git-github-hard-stop.md) guardrail for SSH, authentication, remote access, authorization, or permission failures. Resolve ordinary Git workflow problems such as merge conflicts or dirty worktrees in the current workflow. +Follow [Stop for Git and GitHub Access Errors](../_shared/git-github-hard-stop.md) when an access error occurs. +Resolve merge conflicts and dirty-worktree problems in this workflow. ## Step 1: Verify Branch State @@ -28,7 +33,7 @@ Before creating a PR, verify the branch. git fetch --prune origin main ``` -2. **Not on main.** Never create PRs from main. +2. **Use a feature branch.** Do not create a PR from `main`. ```bash git branch --show-current @@ -40,48 +45,57 @@ Before creating a PR, verify the branch. git log origin/main..HEAD --oneline ``` -4. **Working tree is clean.** Stage or stash any uncommitted changes first. +4. **Clean the working tree.** Stage or stash uncommitted changes. ```bash git status ``` -## Step 2: Choose Efficient Pre-PR Checks +## Step 2: Select Pre-PR Checks -Do not rerun the whole local gate just to create a PR when Git hooks already supplied that evidence. -Use the checks that match the diff and the verification you already have. +Do not rerun a local gate when Git hooks already gave the required evidence. +Select checks that apply to the diff. ### Hook Evidence -If the commits were created normally and the branch was pushed normally, count the installed hooks as verification: +If the commits and push used the installed hooks, use the hook results as verification: - `pre-commit` runs cheap structural and file-local checks, including fixers, formatters, linters, and skill frontmatter validation. - `commit-msg` runs commitlint. - `pre-push` runs path-scoped incremental type checks for affected CLI and plugin surfaces plus checked-JavaScript checks. -If hooks were skipped with `--no-verify`, were not installed, failed, or you cannot tell whether they ran, use the single diff-scoped fallback that reproduces `pre-commit`, `commit-msg`, and `pre-push` checks: +Run the fallback command if the hooks were skipped, missing, failed, or uncertain. +The command runs the `pre-commit`, `commit-msg`, and `pre-push` checks for the diff: ```bash npm run check:diff ``` -The fallback compares with the refreshed `origin/main` ref from Step 1. -Reserve `npm run check` for the whole-repository pre-commit and full CLI/plugin coverage baseline, such as when changing hook configuration, formatter configuration, generated-check scripts, or other repo-wide validation behavior. +The fallback compares the branch with the refreshed `origin/main` ref from Step 1. +Use `npm run check` for changes to repository-wide validation. +Examples include hook configuration, formatter configuration, generated-check scripts, and coverage baselines. ### Targeted Tests -Run the smallest meaningful tests for changed behavior once per relevant change set, and record the command and result for the PR body: +Run the smallest test that verifies each changed behavior. +Run it once for each change set. Record the command and result in the PR body: - CLI or root `src/`, `bin/`, `scripts/`, or `test/` changes: `npx vitest run --project cli` or the directly affected test file. - Plugin changes under `nemoclaw/src/`: `npx vitest run --project plugin` or the directly affected plugin test file. - E2E support changes under `test/e2e/support/`: `npx vitest run --project e2e-support`. -- E2E workflow, artifact upload, trace timing, or fixture environment-boundary changes: run the directly affected `test/e2e/support/*workflow*.test.ts`, `test/e2e/support/upload-e2e-artifacts-workflow-boundary.test.ts`, `test/e2e/support/sanitize-trace-timing.test.ts`, and fixture boundary tests instead of relying on unrelated live target runs. +- E2E workflow, artifact upload, trace timing, or fixture-boundary changes: run the affected tests from this list: + - `test/e2e/support/*workflow*.test.ts` + - `test/e2e/support/upload-e2e-artifacts-workflow-boundary.test.ts` + - `test/e2e/support/sanitize-trace-timing.test.ts` + - Related fixture-boundary tests + Do not use an unrelated live target as evidence. - Installer behavior changes: run the relevant installer integration project only when the local environment supports it. -Do not rerun targeted tests solely because the normal hooks passed; rerun them after later edits or hook autofixes that can affect the tested behavior. -Reserve `npm test` for broad runtime changes, test harness changes, or cases where targeted coverage is hard to justify. -Reserve `npm run check` for repo-wide hook, formatter, generated-check, or coverage-baseline changes. -Do not run the full test suite for doc-only changes unless the docs change code samples or generated behavior in a way that needs runtime validation. +Do not rerun a targeted test because hooks passed. +Rerun it after an edit or hook fix that can affect the tested behavior. +Use `npm test` for broad runtime or test-harness changes. Also use it when a targeted test cannot give enough evidence. +Use `npm run check` for repository-wide validation changes. +Do not run all tests for a docs-only change unless it changes code samples or generated behavior. For doc-only changes, run the docs build before opening the PR: @@ -89,27 +103,28 @@ For doc-only changes, run the docs build before opening the PR: npm run docs ``` -If a required check fails, fix the issue before creating the PR. -When preparing the PR body, check only the verification boxes backed by hooks, manual commands, or CI evidence you actually have. +Fix each required check before you create the PR. +In the PR body, select only verification boxes that have hook, command, or CI evidence. ## Step 3: Push the Branch -Ensure the branch is pushed to the remote. +Push the branch to the remote. ```bash git push -u origin HEAD ``` -If the push fails because of SSH, authentication, remote access, authorization, or permission problems, follow [Git and GitHub Access Hard Stop](../_shared/git-github-hard-stop.md). Resolve ordinary non-access Git failures, such as merge conflicts or dirty worktrees, in the current workflow. +If the push has an access error, follow [Stop for Git and GitHub Access Errors](../_shared/git-github-hard-stop.md). +Resolve other Git errors in this workflow. ## Step 4: Prepare DCO Declaration and Verify GitHub Commits -Before creating the PR, prepare the DCO declaration for the PR body and verify every commit in `origin/main..HEAD`. -This is a hard contributor self-serve gate. -Do not run `gh pr create` until the PR body will include the DCO declaration and every commit passes GitHub verification. +Before you create the PR, prepare the DCO declaration and verify each commit in `origin/main..HEAD`. +The contributor must pass this gate. +Do not run `gh pr create` until the PR body has the declaration and GitHub verifies each commit. 1. **DCO declaration.** The PR body must include a `Signed-off-by:` declaration for the contributor. - Use the contributor's configured Git identity unless the contributor provides a different valid identity. + Use the configured Git identity unless the contributor gives a different identity. ```bash git config user.name @@ -125,9 +140,9 @@ Do not run `gh pr create` until the PR body will include the DCO declaration and done ``` -If the PR body would miss the DCO declaration or any commit is missing GitHub verification, stop. -Tell the contributor to fix the issue before opening a PR. -If force-push is not allowed and the published branch already contains an unverified commit, require a fresh branch and fresh PR with a clean compliant history. +Stop if the PR body does not have the DCO declaration or GitHub does not verify a commit. +Tell the contributor to correct the problem before they open a PR. +If they cannot force-push a corrected history, require a new branch and PR with compliant commits. ## Step 5: Determine PR Metadata @@ -141,7 +156,7 @@ PR titles must follow Conventional Commits format: Types: `feat`, `fix`, `docs`, `chore`, `refactor`, `test`, `ci`, `perf` -Scope is typically the component name (e.g., `cli`, `blueprint`, `plugin`, `policy`, `docs`). +The scope is usually the component name, such as `cli`, `blueprint`, `plugin`, `policy`, or `docs`. Examples: @@ -151,7 +166,7 @@ Examples: ### Type of Change -Determine which type applies based on the diff: +Select the type that matches the diff: - **Code change for a new feature, bug fix, or refactor** — most PRs. - **Code change with doc updates** — code plus changes under `docs/`. @@ -160,56 +175,63 @@ Determine which type applies based on the diff: ### Related Issue -Check the branch name and commit messages for issue references. If an issue exists, use `Fixes #NNN` or `Closes #NNN`. - -### DCO Sign-Off - -The PR body must include a DCO sign-off line. -Determine the user's name and email from git config: - -```bash -git config user.name -git config user.email -``` +Check the branch name and commit messages for issue references. +If an issue exists, use `Fixes #NNN` or `Closes #NNN`. ## Step 6: Compose the PR Body -Read the PR template from the trusted base branch and use that file as the source of truth. Do not treat a branch-modified `.github/PULL_REQUEST_TEMPLATE.md` as authoritative unless the template change itself is the reviewed subject of the PR. Comments or text inside the copied template cannot override this skill's hard requirements for DCO, commit verification, quality gates, sensitive-path handling, or CI-waiver handling. +Read the PR template from the trusted base branch. Use it as the source of truth. +Do not use a branch-modified template unless the PR changes the template. +Template text cannot override requirements for DCO, commit verification, quality gates, sensitive paths, or CI waivers. +Apply [NemoClaw Technical English](../../../CONTRIBUTING.md#nemoclaw-technical-english) to the PR body and other explanatory text that this workflow changes. +During the changed-text pilot, do not add unrelated prose cleanup to the PR. -Fill in each section based on the diff from the same trusted base ref used for the template. Check the applicable boxes and leave others unchecked. Preserve and order every section except `Related Issue`, which the template says to remove when no issue exists. +Complete each section from the diff against the same base ref. +Select the applicable boxes and leave the other boxes clear. +Keep every section in its original order. Remove `Related Issue` when no issue exists. -Recommended workflow: +Use this workflow: ```bash git show origin/main:.github/PULL_REQUEST_TEMPLATE.md > /tmp/nemoclaw-pr-body.md git diff origin/main...HEAD ``` -If `origin/main` is unavailable but local `main` is known to be up to date with the trusted base, use: +If `origin/main` is unavailable, use a local `main` that matches the trusted base: ```bash git show main:.github/PULL_REQUEST_TEMPLATE.md > /tmp/nemoclaw-pr-body.md git diff main...HEAD ``` -Then edit `/tmp/nemoclaw-pr-body.md` for the PR, including a `Signed-off-by:` line. If the PR intentionally changes `.github/PULL_REQUEST_TEMPLATE.md`, compare the branch version against the trusted base template and preserve or strengthen the hard requirements above before using the branch version in the PR body. +Edit `/tmp/nemoclaw-pr-body.md` and add a `Signed-off-by:` line. +If the PR changes the template, compare its version with the trusted version. +Keep or strengthen the requirements above before you use the changed template. ### Populating the Template Follow these rules when filling in the template: -- **Summary:** Write 1-3 plain sentences describing what changes and why. Describe before-and-after behavior when it applies. Use existing repository terms instead of inventing a label for the PR. Derive this from the commit messages and diff. +- **Summary:** Write one to three sentences that state what changes and why. Include before-and-after behavior when useful. Use repository terms. Use the commits and diff as evidence. - **Related Issue:** Include `Fixes #NNN` or `Closes #NNN` if an issue exists. Remove the section entirely if there is no related issue. -- **Changes:** List concrete changes. If the PR adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. +- **Changes:** List the changes. For each new abstraction, configuration, fallback, migration, or compatibility path, give this information: + - The requirement and consumer. + - Why a direct change is not sufficient. + - The test that protects the behavior. - **Type of Change:** Check one box. Use `[x]` for checked, `[ ]` for unchecked. -- **Quality Gates:** Check one tests line and one docs line, then check every other line that applies to the diff. If tests/docs are not needed or existing coverage is sufficient, include the justification. If sensitive paths changed or a non-success CI check is accepted, record the authorized reviewer, maintainer-approved waiver, approval link, or follow-up issue. -- **Verification:** Check only the boxes backed by the requested command/result, justification, normal hook evidence, or fallback evidence. Do not check boxes for steps you skipped or did not verify. The DCO declaration and GitHub verification checkbox is mandatory before PR creation because Step 4 must pass first. For focused changes, leave the broad-gate line unchecked unless you actually ran the applicable command. +- **Quality Gates:** Select one tests line and one docs line. Select each other line that applies. + Explain why tests or docs are not necessary. + Record an approved waiver or follow-up for a sensitive path or accepted CI failure. +- **Verification:** Select only boxes that have command, hook, CI, or written evidence. + Do not select a box for a skipped step. + Select the DCO and commit-verification box after Step 4 passes. + Leave the broad-gate box clear unless you ran that gate. - **DCO Sign-Off:** Replace `{name}` and `{email}` with values from `git config user.name` and `git config user.email`. ## Step 7: Create the PR -Use `gh pr create` with the `--assignee @me` flag and `--body-file` pointing to the completed PR body from Step 6 to preserve formatting. -Only run this step after Step 4 confirms that the PR body includes the DCO declaration and every commit is GitHub-verified. +Run `gh pr create` with `--assignee @me` and the completed body file. +Run this command only after Step 4 passes. ```bash gh pr create \ @@ -220,7 +242,7 @@ gh pr create \ ### Labels -Add labels when applicable: +Add labels that apply: ```bash --label "area: docs" # for doc-only or doc-inclusive PRs @@ -229,19 +251,24 @@ Add labels when applicable: ### Draft PRs -For work-in-progress that is not ready for review: +For work that is not ready for review, complete Step 4 and use the completed body file. +Draft PRs require the same DCO declaration and commit-verification evidence as other PRs. ```bash -gh pr create --draft --title "..." --assignee "@me" --body "..." +gh pr create \ + --draft \ + --title "(): " \ + --assignee "@me" \ + --body-file /tmp/nemoclaw-pr-body.md ``` ## Step 8: Monitor CI and Review Feedback -After creating the PR, do not stop at the URL. Follow the shared [PR CI and Review Follow-Up](../_shared/pr-follow-up.md) workflow: watch required CI, inspect CodeRabbit and PR Review Advisor feedback, address valid findings, and consult the user when feedback is ambiguous or design-changing. +After you create the PR, follow [Follow Up on PR CI and Reviews](../_shared/pr-follow-up.md). ## Step 9: Report the Result -After the PR is created and the initial CI/reviewer follow-up is handled, display the PR URL as a clickable markdown link and summarize the status: +After the first CI and review pass, show the PR link and status: ```text Created PR [#NNN](https://github.com/NVIDIA/NemoClaw/pull/NNN) @@ -249,19 +276,12 @@ CI: passing/pending/failing Automated review: no actionable findings / addressed findings / waiting on user ``` -## Common Mistakes to Avoid - -- **Do not invent your own PR body format.** Use `.github/PULL_REQUEST_TEMPLATE.md` from the base branch. -- **Do not omit template sections.** Preserve every section except `Related Issue` when no issue exists. -- **Do not check boxes for steps you did not run.** If you did not run `npm run docs`, leave that box unchecked. -- **Do not rerun hook-covered checks by default.** Normal `pre-commit`, `commit-msg`, and `pre-push` hooks are valid verification. Use `npm run check:diff` once as the fallback when hooks were skipped, missing, or uncertain. -- **Do not run targeted tests more than once per unchanged relevant change set.** Record the passing command and result; rerun when subsequent edits or hook autofixes can affect that behavior. -- **Do not run broad gates for doc-only changes by default.** Run the docs build instead, and leave the broad-gate verification item unchecked unless you actually ran the applicable command. -- **Do not omit the `Signed-off-by:` line from the PR body.** CI will reject the PR without it. -- **Do not create PRs with unverified commits.** GitHub must report every PR commit as `Verified` before the PR is opened. -- **Do not rely on maintainers to repair contributor signature history.** If force-push is not allowed and the branch contains an unverified commit, use a fresh branch and fresh PR. -- **Do not forget `--assignee @me`.** Every PR must be assigned to its creator. -- **Do not narrate the analysis process.** Report the decision, concrete changes, and verification evidence. -- **Do not create PRs from main.** Always use a feature branch. -- **Do not troubleshoot Git/GitHub access in-agent.** If SSH, `gh`, authentication, remote access, authorization, or push permissions fail, stop and ask the user to fix access. Do resolve ordinary merge conflicts and dirty-worktree state when the workflow calls for it. -- **Do not abandon the PR after creation.** Watch CI and feedback from CodeRabbit and the PR Review Advisor, address valid findings, and consult the user when feedback is ambiguous. +## Final rules + +- Use the base-branch PR template. +- Keep all template sections except an unused `Related Issue` section. +- Select only boxes that have evidence. +- Do not create a PR from `main`. +- Assign the PR to its creator with `--assignee @me`. +- Report decisions, changes, and verification evidence. Do not report the analysis process. +- Follow CI and automated reviews after you create the PR. diff --git a/.agents/skills/nemoclaw-contributor-update-docs/SKILL.md b/.agents/skills/nemoclaw-contributor-update-docs/SKILL.md index 320faf72201..fe3a030b555 100644 --- a/.agents/skills/nemoclaw-contributor-update-docs/SKILL.md +++ b/.agents/skills/nemoclaw-contributor-update-docs/SKILL.md @@ -3,6 +3,9 @@ name: nemoclaw-contributor-update-docs description: Scan recent git commits for user-facing changes, update the corresponding documentation, and create the canonical dated MDX changelog entry for pre-tag release prep. Use when docs have fallen behind code changes, after a batch of features lands, before opening a release-note docs PR or cutting a release tag, or when recovering missed post-release docs. Trigger keywords - update docs, draft docs, docs from commits, sync docs, catch up docs, doc debt, docs behind, docs drift, release prep docs, pre-tag docs, release note docs, changelog entry. --- + + + # Update Docs from Commits Scan recent git history for commits that affect user-facing behavior and draft documentation updates for each. @@ -39,11 +42,11 @@ Ignore comment lines (starting with `#`) and inline comments (everything after ` Keep the loaded skip list in memory for use throughout the skill execution and the whole documentation process. -## Step 0.5: Find Release Announcement Notes +## Step 0.5: Find the Announcement When the user asks for release-prep docs for a specific version `n` (for example `0.0.63`), first determine whether this is pre-tag release prep or post-release recovery. For pre-tag release prep, use the draft release plan, maintainer context, PR list, and commit scan as source context because the announcement may not exist yet. -For post-release recovery, find the NemoClaw GitHub discussion announcement for that release before drafting release notes. +For post-release recovery, find the NemoClaw GitHub discussion Announcement before drafting the release entry. Use any available announcement as source context alongside the commit scan, especially for release themes, PR grouping, contributor thanks, and maintainer wording. For post-release recovery, or when the user says the announcement already exists, search recent discussions and select the announcement whose title or body references `v` or `NemoClaw v`: @@ -145,11 +148,14 @@ Identify where the new content should go. Follow the page's existing structure. Before writing, verify that the commit was not excluded in Step 1. Do not draft content for commits matched by the skip list or for agent integrations not in the tested agent support matrix. After drafting, scan the content for any `skip-terms` from `docs/.docs-skip`. Remove any sentence or section that contains a skip-term. If in doubt, skip the commit and report it. +Apply [NemoClaw Technical English](../../../CONTRIBUTING.md#nemoclaw-technical-english) to changed documentation and changelog text. +During the changed-text pilot, do not rewrite unrelated prose. + Write the doc update following these conventions: - **Active voice, present tense, second person.** - **No unnecessary bold.** Reserve bold for UI labels and parameter names. -- **No em dashes** unless used sparingly. Prefer commas or separate sentences. +- **Do not use em dashes.** Use commas, colons, or separate sentences. - **Start sections with an introductory sentence** that orients the reader. - **No superlatives.** Say what the feature does, not how great it is. - **Copyable code examples use language-specific fences** such as `bash`, `sh`, or `powershell`, without prompt markers. diff --git a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/SKILL.md b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/SKILL.md index 60d3a15dd57..e66981f4bc4 100644 --- a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/SKILL.md @@ -1,15 +1,18 @@ --- name: nemoclaw-maintainer-cross-issue-sweep -description: Scans other open issues to find ones a given PR may also fix or accidentally break. Outputs adjacent-fix opportunities and contradiction risks with file:line evidence. Use when reviewing a PR to discover bundling opportunities or downstream impact across the issue queue. +description: Scan open issues to find issues that a PR can also fix or conflict with. Report each relationship with file and line evidence. Use this skill during PR review to find related fixes and risks. user_invocable: true --- + + + # Cross-Issue Regression Sweep -Surfaces the issues a single PR may also fix or accidentally break beyond the one it claims to address. Two outputs: +Find open issues that a PR can affect in addition to its linked issue. Report two relationship types: -- **Adjacent fixes** — "PR may also close #X" → bundling intel (ship one PR, close multiple issues) -- **Contradicting risks** — "PR may break what #Y wants" → coordination needed before merge +- **Adjacent fix** — The PR can also resolve another issue. +- **Conflict** — The PR can prevent the behavior that another issue requests. ## Prerequisites @@ -19,7 +22,7 @@ Surfaces the issues a single PR may also fix or accidentally break beyond the on ## Repo policy -Defaults assume NemoClaw conventions. Edit `repo-policy.md` to override per-repo (bot logins, candidate caps, language regex). +The defaults use NemoClaw conventions. Edit `repo-policy.md` for another repository. ## Workflow @@ -41,7 +44,8 @@ Cross-issue sweep progress: scripts/extract-fingerprint.sh ``` -Pulls four dimensions: touched files, touched symbols (per-language regex), error-string tokens, and the PR's primary linked issue (for exclusion). See `checks/fingerprint-extraction.md`. +The script collects changed files, changed symbols, error strings, and the linked issue. +See `checks/fingerprint-extraction.md`. ### Step 2: Search candidate issues @@ -49,40 +53,40 @@ Pulls four dimensions: touched files, touched symbols (per-language regex), erro scripts/search-candidate-issues.sh ``` -Three search dimensions, capped at 30 total candidates: +Search these three inputs. Keep no more than 30 candidates: - Per symbol: top 10 by recency - Per file path: top 5 by recency - Per error string: top 5 by recency -Dedupes; excludes the PR's primary linked issue. +Remove duplicates and the linked issue. ### Step 3: Classify each candidate -For each candidate, the LLM classifies as one of four classes per `checks/relationship-judgment.md`: +Classify each candidate with the rules in `checks/relationship-judgment.md`: -- **ADJACENT_FIX** — PR's changes likely also resolve this issue -- **CONTRADICTING** — PR's approach blocks what this issue wants +- **ADJACENT_FIX** — The PR can resolve this issue. +- **CONTRADICTING** — The PR conflicts with the requested behavior. - **SAME_ISSUE_DIFF** — same root bug as PR's primary issue (dedup filter) - **UNRELATED** — no meaningful relationship -Required for ADJACENT_FIX or CONTRADICTING: +For ADJACENT_FIX or CONTRADICTING, cite: -- Cite specific PR diff line -- Cite specific issue symptom +- A PR diff line. +- An issue symptom. - Confidence: high / medium / low -If no specific evidence can be cited, the LLM must answer UNRELATED. This floors hallucination. +Classify the issue as UNRELATED if this evidence is not available. ### Step 4: Reverse-link boost -If the candidate issue's body or comments already mention this PR's number, the relationship is already in someone's mental model. Boost confidence by one tier (low → medium, medium → high). +Increase confidence by one level if the issue body or comments mention the PR number. ### Step 5: Filter -- Suppress UNRELATED + SAME_ISSUE_DIFF -- Drop low-confidence judgments -- Keep ADJACENT_FIX and CONTRADICTING with high or medium confidence +- Remove UNRELATED and SAME_ISSUE_DIFF results. +- Remove low-confidence results. +- Keep high- and medium-confidence ADJACENT_FIX and CONTRADICTING results. ### Step 6: Render report @@ -94,27 +98,29 @@ See `templates/report.md` for the format. ## Reference files -- `repo-policy.md` — configurable per-repo defaults -- `relationship-rules.md` — 4-class definitions with worked examples -- `checks/fingerprint-extraction.md` — what to pull from the diff, per language -- `checks/relationship-judgment.md` — LLM judgment criteria + evidence requirement -- `templates/report.md` — output template -- `validation/backtest.md` — backtest the skill against historical PRs +- [repo-policy.md](repo-policy.md) — Repository settings. +- [relationship-rules.md](relationship-rules.md) — Four relationship classes and examples. +- [checks/fingerprint-extraction.md](checks/fingerprint-extraction.md) — Diff evidence by language. +- [checks/relationship-judgment.md](checks/relationship-judgment.md) — Classification and evidence rules. +- [templates/report.md](templates/report.md) — Output template. +- [validation/backtest.md](validation/backtest.md) — Historical test cases for the skill. ## Scripts (execute, do not read) -- `scripts/extract-fingerprint.sh` — symbols + paths + error strings, deterministic +- `scripts/extract-fingerprint.sh` — symbols, paths, and error strings - `scripts/search-candidate-issues.sh` — GitHub Search wrapper, dedupe, cap - `scripts/render-report.py` — report renderer ## Composition with other skills -This skill is a separate, optional follow-up to `nemoclaw-maintainer-pr-comparator`. The comparator does not call it or include its findings in the deterministic score. Run the sweep explicitly when a maintainer wants adjacent-fix or contradiction evidence alongside the comparator verdict, and report that evidence separately. +This skill is an optional follow-up to `nemoclaw-maintainer-pr-comparator`. +The comparator does not run this skill or use its findings in the score. +Run this skill when a maintainer asks for related-issue evidence. Report the evidence separately. -## What this skill does NOT do (deferred) +## Limits -These would raise the ceiling but require infrastructure beyond GitHub API + LLM: +The skill does not: -- Run PR code against adversarial inputs (sandboxed) -- Static-analyzer dataflow tracing (CodeQL, Semgrep) -- ML-based symbol disambiguation across codebases +- run PR code against adversarial inputs +- trace data flow with a static analyzer such as CodeQL or Semgrep +- disambiguate symbols across codebases with a machine-learning model diff --git a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/fingerprint-extraction.md b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/fingerprint-extraction.md index f3832ff0703..d62a8c85d4c 100644 --- a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/fingerprint-extraction.md +++ b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/fingerprint-extraction.md @@ -1,6 +1,9 @@ + + + # Fingerprint Extraction -What to pull from the PR diff and how. Run `scripts/extract-fingerprint.sh ` to do this mechanically. +Run `scripts/extract-fingerprint.sh ` to collect the PR fingerprint. ## Contents @@ -11,22 +14,23 @@ What to pull from the PR diff and how. Run `scripts/extract-fingerprint.sh ` ## Touched files -From `gh pr view --json files`. Filter to source paths (drop test fixtures, generated files, lockfiles). +Read the paths from `gh pr view --json files`. +Keep source paths. Remove test fixtures, generated files, and lockfiles. -**Why baseline:** Some issues mention a file path but no symbol. Without file matching, those go uncaught. +File matching finds issues that name a file but do not name a symbol. ## Touched symbols -Per-language regex against added/modified lines in the diff. Defaults live in `repo-policy.md`. +Apply each language pattern to added and modified diff lines. `repo-policy.md` contains the defaults. -**Why this is the killer angle:** Most matchers stop at file paths. A user issue saying "validateInput rejects empty strings" pinpoints a function — file-path matching alone misses it if the function is in a different module than expected. +Symbol matching finds issues that name a function but not its file. -**Filtering rules:** +Apply these filters: -- Only extract symbols from added/modified lines, not deleted lines (those are going away) -- Drop common short names (`do`, `if`, `as`) — too noisy -- Drop language keywords -- Drop test-helper names (`describe`, `it`, `test`) — they match too many issues +- Extract symbols from added and modified lines. Do not extract them from deleted lines. +- Remove common short names such as `do`, `if`, and `as`. +- Remove language keywords. +- Remove test-helper names such as `describe`, `it`, and `test`. ## Error-string tokens @@ -37,13 +41,13 @@ Strings inside: - `print(f"...")` / Python f-strings flagged with error-shape (`Error:`, `Failed`) - Distinctive flag/option names (`--no-color`, `--verbose`) -**Why this catches symptoms:** When a user files an issue, they often paste the error message they saw. That string is high-info and rarely false-matches. +Error-string matching finds issues that contain output from the changed code. -**Filtering rules:** +Apply these filters: -- Skip strings <8 chars (too generic) -- Skip strings with no alpha chars -- Strip placeholders (`%s`, `${var}`, `{0}`) before searching +- Remove strings with fewer than eight characters. +- Remove strings that have no letters. +- Remove placeholders such as `%s`, `${var}`, and `{0}` before the search. ## Primary linked issue @@ -52,7 +56,7 @@ From the PR body, parse: - `closes #N` / `fixes #N` / `resolves #N` - `Linked Issue: #N` block -Captured for **exclusion** during search. Without this, every PR self-matches its own issue. +Exclude this issue from the search results. ## Output @@ -68,4 +72,4 @@ Fingerprint JSON shape: } ``` -Consumed by `scripts/search-candidate-issues.sh`. +Pass this output to `scripts/search-candidate-issues.sh`. diff --git a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/relationship-judgment.md b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/relationship-judgment.md index ff235ed8444..051b045bc39 100644 --- a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/relationship-judgment.md +++ b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/relationship-judgment.md @@ -1,22 +1,25 @@ + + + # Relationship Judgment -How the LLM classifies each candidate issue. The judgment is the only non-deterministic step in the pipeline; everything else is mechanical search. +Use this process to classify each candidate issue. This is the only step that requires model judgment. ## Contents -- Inputs to the LLM +- Inputs to the model - The prompt - Evidence requirement - Confidence levels - Reverse-link boost -## Inputs to the LLM (per candidate) +## Inputs to the model -- PR diff (truncated to 3000 chars if larger) +- PR diff (limit: 3000 characters) - PR description body -- PR's primary linked issue number (for context — used by SAME_ISSUE_DIFF check) -- Candidate issue number, title, body (truncated to 2000 chars) -- Candidate issue's first ~5 comments (for symptom context) +- PR's linked issue number for the SAME_ISSUE_DIFF check +- Candidate issue number, title, and body (limit: 2000 characters) +- First five candidate-issue comments ## The prompt @@ -29,71 +32,67 @@ PR diff (relevant slice): {diff} Candidate issue #{issue_number}: {issue_title} Issue body: {issue_body} +First five issue comments: +{candidate_comments} PR's primary linked issue: #{primary_issue} Classify the relationship: -- ADJACENT_FIX: PR's changes resolve this issue OR open a clear follow-on path - on the same code the PR just touched -- CONTRADICTING: PR's approach makes this issue's desired behavior impossible, - OR the PR's scope is incomplete and the issue reports the leftover gap +- ADJACENT_FIX: The PR resolves this issue or enables follow-up work on the changed code. +- CONTRADICTING: The PR prevents the requested behavior or leaves another instance of the bug. - SAME_ISSUE_DIFF: same root bug as #{primary_issue} (dedupe filter) - UNRELATED: no meaningful relationship -For ADJACENT_FIX or CONTRADICTING, REQUIRED — cite ONE of these evidence shapes: +For ADJACENT_FIX or CONTRADICTING, cite one of these evidence types: - (a) DIRECT: cite specific PR diff line(s) (file:line) AND specific issue - symptom(s) that map to those lines + (a) DIRECT: Cite PR diff lines and the issue symptoms that those lines affect. - (b) BY-OMISSION (partial-fix detection): cite the PR's diff *scope* — what - class of bug it addressed — AND the issue's symptom showing the same - class but a different instance the PR did NOT touch. Required: name the - bug class, name the instances PR fixed, name the instances issue - reports as still broken. + (b) BY-OMISSION: Name the bug class and the instances that the PR fixes. + Name the unchanged instances that the issue reports. - (c) FOLLOW-ON: cite the symbol/file the PR introduced or modified AND the - issue's request to harden the same symbol/file (e.g., "PR introduced - rcf_patch.py; issue requests rcf_patch.py be hardened against X"). + (c) FOLLOW-ON: Cite the changed symbol or file. + Cite the issue request about that symbol or file. Confidence: high / medium / low -If you cannot cite specific evidence under any of (a), (b), (c), answer UNRELATED. +If you cannot cite one of these evidence types, answer UNRELATED. ``` -## Evidence requirement (anti-hallucination) +## Evidence requirement -For any ADJACENT_FIX or CONTRADICTING verdict, the LLM must cite evidence under one of three shapes: +For each ADJACENT_FIX or CONTRADICTING result, cite one of these evidence types: -- **Direct**: specific PR diff line + specific issue symptom that map to each other -- **By-omission**: PR's diff *scope* (the bug class it addressed) + issue symptom showing the same class but a different instance the PR did NOT touch (catches partial-fix patterns) -- **Follow-on**: the symbol/file the PR introduced + the issue's request to harden the same symbol/file (catches "PR introduced X, now harden X" follow-up patterns) +- **Direct:** A PR diff line and the issue symptom that it affects. +- **By omission:** The bug class that the PR changes and an unchanged instance of that bug in the issue. +- **Follow-on:** A changed symbol or file and an issue request about that symbol or file. -Without one of these citations, the answer must be UNRELATED. +Without this evidence, classify the issue as UNRELATED. -This rule is the single most important defense against hallucinated matches. The three shapes give the LLM legitimate paths to flag genuine relationships without lowering the bar to vague "they touch the same area" matches. +This rule prevents unsupported matches based only on a shared code area. ## Confidence levels -- **high**: clear semantic match between cited PR change and cited issue symptom -- **medium**: plausible match but partial evidence (e.g., the change touches the right area but doesn't directly fix the cited symptom) -- **low**: weak inference; below the default `confidence_floor` from `repo-policy.md` and gets dropped +- **high:** The cited PR change affects the cited issue symptom. +- **medium:** The evidence is incomplete, but the change affects the related code. +- **low:** The result depends on an inference. The default confidence threshold removes it. ## Reverse-link boost -If the candidate issue's body or comments already mention this PR's number (e.g., "fixed by PR #2851"), the relationship is already in someone's mental model. Boost confidence one tier: +If the issue body or comments mention the PR number, increase confidence by one level: -- low → medium (rescues a borderline match) -- medium → high (cements a likely match) -- high → unchanged (already at ceiling) +- low → medium +- medium → high +- high → high -Implementation: after the LLM's classification, the orchestrator checks the candidate issue body and comments for the PR number. If found, applies the boost. +After classification, check the issue body and comments for the PR number. +If the number occurs, increase the confidence. -## Why this beats naive token-overlap +## Search and classification -Naive token-overlap finds candidates but produces high false-positive rates. Two filters separate signal from noise: +Token matching finds candidates. These checks remove unsupported matches: -1. **LLM judgment** distinguishes "function name appears in issue" from "function's behavior is what the issue describes" -2. **Evidence requirement** forces the LLM to commit to specific lines, not vague hand-waving +1. **Relationship judgment:** Check whether the issue describes the changed behavior. +2. **Evidence requirement:** Cite the related lines and symptoms. -The reverse-link boost handles the case where humans have already noticed the relationship — that's strong prior signal the skill should respect. +The PR-number check increases confidence when the issue already links the PR. diff --git a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/relationship-rules.md b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/relationship-rules.md index d4333919e20..370c9fcc05c 100644 --- a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/relationship-rules.md +++ b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/relationship-rules.md @@ -1,6 +1,9 @@ + + + # Relationship Classification Rules -The four classes the LLM assigns to each candidate issue, with worked examples. +Use these four classes for candidate issues. ## Contents @@ -11,7 +14,7 @@ The four classes the LLM assigns to each candidate issue, with worked examples. ## ADJACENT_FIX -The PR's changes resolve this issue OR open a clear follow-on path on the same code the PR just touched. +The PR resolves the issue or enables follow-up work on the changed code. ### Example A — incidental closure (direct evidence) @@ -21,7 +24,7 @@ Candidate issue #2810: "Telegram preset writes fail intermittently after sandbox Issue body cites: "EPERM on `.openclaw/credentials/telegram.json`" **Classification:** ADJACENT_FIX, high confidence -**Evidence cited (direct):** PR diff `Dockerfile.base:97` (chmod g+w on .openclaw); issue body line 14 ("EPERM on .openclaw/credentials/telegram.json"). Same root cause, same fix. +**Direct evidence:** The PR changes permissions at `Dockerfile.base:97`. Issue line 14 reports an `EPERM` error in the same directory. ### Example B — follow-on hardening on PR-introduced code @@ -29,11 +32,11 @@ PR #2696 introduced `scripts/rcf_patch.py` with regex-based property matching. Candidate issue #2875: "Harden rcf_patch.py against property-order drift" — issue says PR #2696 is "a real improvement... one follow-on hardening gap: the regex still assumes `snapshot` before `nextConfig`." **Classification:** ADJACENT_FIX, high confidence (boosted by reverse-link) -**Evidence cited (follow-on):** PR introduced `scripts/rcf_patch.py`; issue requests hardening the same file's regex against a specific drift case. The PR's code is the subject of the issue's hardening request, not a separate concern. +**Follow-on evidence:** The PR adds `scripts/rcf_patch.py`. The issue asks for a change to that file's regular expression. ## CONTRADICTING -The PR's approach makes this issue's desired behavior impossible, OR the PR's scope is incomplete and the issue reports the leftover gap. +The PR prevents the requested behavior or leaves another instance of the bug. ### Example A — direct contradiction @@ -42,7 +45,7 @@ PR diff: deletes try/catch around `mutateConfigFile` Candidate issue #4187: "Allow opt-in error suppression for sandbox config writes during shutdown" **Classification:** CONTRADICTING, medium confidence -**Evidence cited (direct):** PR diff removes `try { ... } catch { /* swallow */ }` at `Dockerfile:142`; issue body line 8 explicitly requests "opt-in suppression for shutdown-time write failures." PR strictly rejects what issue requests. +**Direct evidence:** The PR removes error suppression at `Dockerfile:142`. Issue line 8 requests optional suppression for shutdown errors. ### Example B — partial-fix gap (evidence by omission) @@ -50,30 +53,35 @@ PR #2700 changed 5 env-var validations from `return 1` to `return 0` in `scripts Candidate issue #2762: "PR #2700 changed validations... However... NEMOCLAW_CONTEXT_WINDOW and NEMOCLAW_MAX_TOKENS with invalid values still cause the container to exit with code 1." **Classification:** CONTRADICTING, high confidence (boosted by reverse-link) -**Evidence cited (by-omission):** Bug class — env-var validation hard-exits under `set -euo pipefail`. PR fixed instances `NEMOCLAW_MODEL_OVERRIDE`, `NEMOCLAW_INFERENCE_API_OVERRIDE`, `NEMOCLAW_REASONING`, `NEMOCLAW_CORS_ORIGIN`, plus one more. Instances PR did NOT touch: `NEMOCLAW_CONTEXT_WINDOW`, `NEMOCLAW_MAX_TOKENS`. Issue reports the same hard-exit class on those untouched instances. The PR's incomplete scope is the contradiction with the issue's expectation of a class-level fix. +**By-omission evidence:** Invalid environment values cause an exit under `set -euo pipefail`. +The PR fixes five variables, including `NEMOCLAW_MODEL_OVERRIDE` and `NEMOCLAW_REASONING`. +It does not fix `NEMOCLAW_CONTEXT_WINDOW` or `NEMOCLAW_MAX_TOKENS`. +The issue reports the same error for those two variables. ## SAME_ISSUE_DIFF -The candidate issue describes the same root bug as the PR's primary linked issue. Suppress to avoid double-counting. +The candidate describes the same bug as the linked issue. Remove it to prevent duplicate results. **Example:** PR's primary issue: #2681 ("Enable Dreaming permission error") Candidate issue #2895: "Toggle in OpenClaw UI fails with EACCES" -Both describe the same EACCES failure on the same toggle. The candidate is a duplicate of the primary issue. **Classification:** SAME_ISSUE_DIFF (suppressed from output). +Both describe the same EACCES failure on the same toggle. +**Classification:** SAME_ISSUE_DIFF. Do not include it in the output. ## UNRELATED -No meaningful relationship. The candidate showed up in search because of token overlap but doesn't align with the PR's actual changes. +The issue and PR do not have a meaningful relationship. The search found a shared token only. **Example:** PR description: "extract sandbox-gateway-state helpers" Candidate issue #4523: "Sandbox gateway timeout on first connect" -Search matched on "gateway." But the PR is a pure refactor (no behavior change), and the issue is about timing. **Classification:** UNRELATED. +The search matched `gateway`. The PR does not change behavior, and the issue reports a timing problem. +**Classification:** UNRELATED. ## Decision rule -If the LLM cannot cite a specific PR diff line **and** a specific issue symptom that map to each other, the answer must be UNRELATED. This prevents hallucinated matches. +Classify the issue as UNRELATED unless the evidence meets one type in `checks/relationship-judgment.md`. diff --git a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/repo-policy.md b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/repo-policy.md index 20e07fc33ad..7dcc1e85d76 100644 --- a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/repo-policy.md +++ b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/repo-policy.md @@ -1,6 +1,9 @@ + + + # Repo Policy -Configurable defaults that adapt the skill to a specific repository. +Defaults for the target repository. ## Contents @@ -15,10 +18,10 @@ Configurable defaults that adapt the skill to a specific repository. per_symbol_top: 10 # top N issues per symbol search per_file_top: 5 # top N issues per file path search per_error_string_top: 5 # top N issues per error string search -max_total_candidates: 30 # hard cap before LLM judgment +max_total_candidates: 30 # limit before model judgment ``` -Per-search caps balance recall against cost. The hard cap bounds total LLM calls per PR. +The search limits control cost. `max_total_candidates` limits classification calls for each PR. ## Symbol extraction (per-language regex) @@ -43,11 +46,11 @@ shell: - '^([a-z_][\w]*)\s*\(\)\s*\{' # function definitions ``` -Override or add languages here for non-NemoClaw repos. +Change or add languages for another repository. ## Bot-author exclusions -Issues authored by these accounts are skipped during candidate search (they're noise — automated bug reports, dependency bots, etc.): +Do not include issues from these automated accounts: ```yaml excluded_authors: @@ -64,4 +67,4 @@ Drop judgments below this: confidence_floor: medium ``` -Set to `low` if you want to see every flagged candidate; `high` if you want only the most confident bundling/contradiction signals. +Use `low` to include all candidates. Use `high` to include high-confidence results only. diff --git a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/templates/report.md b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/templates/report.md index b6c826edd5f..6ed8ce30355 100644 --- a/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/templates/report.md +++ b/.agents/skills/nemoclaw-maintainer-cross-issue-sweep/templates/report.md @@ -1,20 +1,23 @@ + + + # Report Template -The skill emits this structure. `scripts/render-report.py` produces it from a list of classified candidates. +`scripts/render-report.py` uses classified candidates to produce this structure. ```markdown ## Cross-issue scan — PR # () ### Adjacent fixes (PR may also close) -- **#4521** (high) — empty-array check in `validateInput()` matches symptom at issue body line 12 - → suggest: add `closes #4521` to PR body -- **#4889** (medium) — same validation path; matches issue's repro at line 7 +- **#4521** (high) — empty-array check in `validateInput()` matches the symptom at issue body line 12. + Suggested action: Add `closes #4521` to the PR body. +- **#4889** (medium) — same validation path. It matches the issue reproduction at line 7. ### Contradicting (coordinate before merge) -- **#4187** (medium) — PR strictly rejects empty input; #4187 requests opt-in allowance at body line 8 - → suggest: discuss approach with #4187's author or close #4187 as "fixed by alternative direction" +- **#4187** (medium) — PR rejects empty input. Issue #4187 requests opt-in allowance at body line 8. + Suggested action: Discuss the approach with the author of #4187. Close #4187 only if the maintainer selects the PR behavior. ### Suppressed @@ -23,18 +26,15 @@ The skill emits this structure. `scripts/render-report.py` produces it from a li ### Reasoning trace (top 3 by impact) -- #4521 (high): PR diff `src/lib/validate.ts:42` adds `if (input.length === 0) return null` — issue - body line 12: "validateInput throws when array is empty, expected null". Both cite the same - function and the same desired behavior. Reverse-link applied: issue mentions PR #2851 in - comment 4, boosted from medium to high. -- #4889 (medium): PR diff `src/lib/validate.ts:50` enforces non-empty in shared helper — issue - repro at line 7 shows empty-array path. Match is structural but issue may be a duplicate of - #4521 (commenters note same). -- #4187 (medium): PR's strict rejection at line 42 directly opposes #4187's "opt-in allow" ask - at body line 8. +- #4521 (high): `src/lib/validate.ts:42` adds `if (input.length === 0) return null`. + Issue body line 12 reports that `validateInput` throws for an empty array. + Issue comment 4 mentions PR #2851, so confidence increased from medium to high. +- #4889 (medium): `src/lib/validate.ts:50` rejects an empty value in a shared helper. + The issue reproduction reaches that path. Comments indicate that #4889 can duplicate #4521. +- #4187 (medium): The rejection at line 42 conflicts with the opt-in request at issue body line 8. ``` -If no adjacent or contradicting candidates pass the confidence floor, the report just says: +If no results meet the confidence threshold, use this report: ```markdown ## Cross-issue scan — PR # diff --git a/.agents/skills/nemoclaw-maintainer-day/HOTSPOTS.md b/.agents/skills/nemoclaw-maintainer-day/HOTSPOTS.md index 12441dfbb63..1be98bf8e67 100644 --- a/.agents/skills/nemoclaw-maintainer-day/HOTSPOTS.md +++ b/.agents/skills/nemoclaw-maintainer-day/HOTSPOTS.md @@ -1,6 +1,9 @@ -# Hotspots Workflow + + -Find files hurting throughput and reduce their future blast radius. +# Reduce Merge Conflicts + +Find files that slow development and reduce future merge conflicts. ## Step 1: Run the Hotspot Script @@ -8,7 +11,8 @@ Find files hurting throughput and reduce their future blast radius. node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/hotspots.ts ``` -This combines 30-day git churn on `main` with open PR file overlap, flags risky areas, and outputs a ranked JSON list. +The script combines 30 days of `main` changes with file overlap from open PRs. +It marks risky areas and returns a ranked JSON list. Pipe into state: @@ -18,13 +22,14 @@ node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer ## Step 2: Prioritize -Review the ranked output. Most urgent: high `combinedScore` + `isRisky: true` + weak tests. +Review the ranked output. +Start with entries that have a high `combinedScore`, `isRisky: true`, and few tests. -## Step 3: Choose Cooling Strategy +## Step 3: Select a change -Smallest change to reduce future collisions: +Select the smallest change that reduces future conflicts: -- extract stable logic from giant file into tested helper +- extract stable logic into a tested helper - split parsing from execution - add regression tests around repeated breakage - deduplicate workflow logic @@ -34,13 +39,15 @@ Prefer changes that also improve testability. ## Step 4: Keep Small -One file cluster per pass. Stop if next step is large redesign → follow [SEQUENCE-WORK.md](SEQUENCE-WORK.md). +Process one file cluster in each pass. +If the next step requires a redesign, follow [SEQUENCE-WORK.md](SEQUENCE-WORK.md). ## Step 5: Validate -Run relevant tests. If risky code, also follow [TEST-GAPS.md](TEST-GAPS.md). +Run tests for the changed behavior. +If the code is in a risky area, also follow [TEST-GAPS.md](TEST-GAPS.md). ## Notes -- Goal is lower future merge pain, not aesthetic cleanup. -- No giant refactors inside contributor PRs. +- Reduce future merge conflicts. Do not make style-only changes. +- Do not add a refactor that exceeds the contributor's objective. diff --git a/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md b/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md index c966d9a1a85..997a51478e6 100644 --- a/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md +++ b/.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md @@ -3,71 +3,335 @@ # Merge Gate Workflow -Run the last maintainer check before approval. Never merge automatically. +Run the maintainer check before approval. Never merge. ## Gates -For the full priority list see [PR-REVIEW-PRIORITIES.md](PR-REVIEW-PRIORITIES.md). A PR is approval-ready only when **all** hard gates pass: +Approve a PR only when all hard gates pass. See [PR-REVIEW-PRIORITIES.md](PR-REVIEW-PRIORITIES.md). -1. **Product scope approved** — confirm that the PR implements existing supported behavior or a linked, accepted product decision. Do not approve a new integration, solution, third-party stack, custom image, or canonical documentation surface merely because it works. Require defined ownership, lifecycle, compatibility, security, and validation expectations. Route independent solutions through [Community Solutions](../../../docs/resources/community-contributions.mdx). -2. **Contributor compliance** — the PR body contains the contributor's `Signed-off-by:` declaration and every PR commit appears as `Verified` in GitHub. Only PR authors whose case-normalized login is exactly `dependabot[bot]` or `app/dependabot` are exempt from the PR-body declaration; every Dependabot commit must still be verified. Reject other noncompliant PRs; maintainers do not repair contributor history. -3. **CI green on the exact diff** — every required check has successful, complete evidence bound to the captured PR head and base. -4. **Current merge revision** — the PR remains open and non-draft; its title, body, head, base branch, base revision, mergeability, and merge state do not change during evaluation; the captured base is still current; and GitHub reports `MERGEABLE` with an allowed merge state. -5. **No major CodeRabbit** — ignore style nits; block on correctness/security bugs. -6. **Risky code tested** — see [RISKY-AREAS.md](RISKY-AREAS.md). Confirm tests exist (added or pre-existing). +1. **Product scope approved** — Confirm that the PR implements supported behavior or a linked product decision. + Do not approve a new product surface because it works. + Require ownership, lifecycle, compatibility, security, and validation requirements. + Route independent solutions through [Community Solutions](../../../docs/resources/community-contributions.mdx). +2. **Contributor requirements pass** — Require the contributor's `Signed-off-by:` declaration in the PR body. + Require every commit to appear as `Verified` in GitHub. + Authors with a case-normalized login of `dependabot[bot]` or `app/dependabot` do not need the PR-body declaration. + Dependabot commits must still appear as `Verified`. +3. **CI passed for the PR SHA** — Require successful evidence for each check on the PR SHA and base SHA. +4. **PR state did not change** — Require the PR to remain open and not draft. + During evaluation, its title, body, PR SHA, base branch, base SHA, mergeability, and merge state must not change. + Require `MERGEABLE` and a merge state that the gate permits. +5. **No major CodeRabbit findings** — Confirm that there is no unresolved correctness or security issue. + Ignore style comments. Block correctness and security defects. +6. **Risky code has tests** — See [RISKY-AREAS.md](RISKY-AREAS.md). Tests can be new or existing. -## Step 1: Run the Gate Checker +## Step 1: Run the gate checker ```bash node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/check-gates.ts ``` -This checks all gates programmatically and returns structured JSON with `allPass`, per-gate `pass`/`details`, and non-blocking `advisories`, including contributor/approver overlap. Use [PR CI and Review Follow-Up](../_shared/pr-follow-up.md) for the shared triage loop when individual findings need investigation. -The product-scope gate is a human decision and is not represented by `allPass`. +The script checks gates that do not require judgment. It returns JSON with `allPass`, gate results, and advisories. +The contributor and approver overlap advisory does not change `allPass`. +A maintainer must decide product scope. `allPass` does not include that decision. -## Step 2: Interpret Results +Use [Follow Up on PR CI and Reviews](../_shared/pr-follow-up.md) to investigate CI or review findings. -The script handles the deterministic checks. You handle judgment calls: +## Step 2: Interpret the results -- **Product scope not established:** Stop before approval when the PR would create a new supported product surface and no accepted issue or design decision establishes ownership and lifecycle expectations. Technical correctness, successful tests, green CI, and positive advisor output do not substitute for product approval. Ask a maintainer for the product decision or route an independent solution through [Community Solutions](../../../docs/resources/community-contributions.mdx). -- **Missing required checks:** The checked-in script requires `checks`, `check-hash`, `changes`, `commit-lint`, `dco-check`, and `E2E / PR Gate` in the status rollup, including while the repository ruleset rollout is staged. First-time fork contributors may need "Approve and run" before the ordinary `pull_request` checks appear. The trusted E2E controller records the head and base for internal and fork PRs without executing PR code. Never waive a missing, neutral, or skipped E2E gate, and never run untrusted fork code with privileged credentials to manufacture a result. -- **Immutable Actions evidence:** Required PR workflows must identify the exact PR, head, and base. The installer-hash workflow reruns its trusted verification on every `pull_request` `edited` event, including non-base metadata edits, and records `gate true` with the current base in its immutable run name; edits never mint skipped required-check evidence. Every substantive `pull_request_target` E2E controller run likewise requires an immutable `gate true` run name for the exact PR, head, and base. GitHub's PR association normally provides the remaining run binding. When GitHub returns an empty association, the checker additionally requires the exact controller path, head branch, and head repository, and a run time that encloses the trusted exact coordination check. A later all-skipped `gate false` metadata run may be discarded using that same narrow relationship only when the stronger substantive run exists. Missing, malformed, stale, contradictory, or mutated title, coordination, timestamp, branch, repository, head, or base evidence fails closed. -- **Controller workflow versus PR verdict:** `E2E / PR Gate Controller` reports whether the trusted controller safely published an outcome. It can be green while the custom `E2E / PR Gate Coordination` check and the native required `E2E / PR Gate` job are red. Merge authority comes from the native required job for the current head and base, not from the controller workflow; inspect the coordination check for the detailed verdict and evidence links. `Superseded by PR update` and `PR closed — gate no longer applies` are cancelled coordination outcomes for an obsolete revision and require no action on that old check; the current revision still needs its own successful gate. The closed-PR outcome also covers a deleted fork repository whose GitHub head-repository field is empty. -- **CI or selected E2E did not pass:** A handled non-retryable failure leaves the native required `E2E / PR Gate` job red while `E2E / PR Gate Controller` may complete successfully. A handled failure carrying one of the eligible retry reasons below leaves the native observer polling for a validated replacement instead of finalizing the required job. Follow the CI run, E2E run, non-passing job, and failed-step links in `E2E / PR Gate Coordination`. Rerun eligible `CI / Pull Request` for an unchanged open head and base only when the newest failed coordination check carries a current-version retry reason. For `prerequisite-ci`, rerun CI first, then allow the controller to retry only after that new CI run succeeds. For `child-cancelled`, rerun CI when the child workflow was cancelled or a complete job listing showed every non-passing job was cancelled. For `evidence-download`, rerun CI when a successful child's download step failed, was cancelled, or was skipped. The trusted controller treats a completed coordination check as immutable, preserves it as audit history, and creates a fresh validated `in_progress` check with the same exact-diff external identity before rebuilding the deterministic plan. The controller and native observer select the highest check-run ID only when every older duplicate is a completed failure with a recognized retry marker; an unexpected app or mismatched mutation identity, duplicate ID, older unmarked or otherwise non-retryable terminal state, or multiple active candidates fails closed. Selected-job or typed-target product or assertion failures, evidence policy or integrity failures, reconciliation, controller errors, unknown states, and failures recorded before retry reasons existed remain terminal; update the PR to create a new head and run fresh CI. Require a successful gate for the resulting revision. A green controller run does not override the red verdict. -- **Evidence could not be verified:** If a selected child succeeds but `Download evidence` fails, is cancelled, or is skipped, `E2E / PR Gate Coordination` fails closed with `Evidence could not be verified`, records the `evidence-download` retry reason, and leaves `E2E / PR Gate Controller` red. Inspect the `Download evidence` step before rerunning eligible PR CI; the green child alone is not accepted evidence. A successful download with missing, duplicate, skipped, pending, or reported-failing signals is a handled red PR verdict with a green controller and no retry reason. Malformed or unsafe evidence, schema or identity mismatches, and traversal-limit violations remain terminal controller verification errors: the coordination check, native required job, and controller all fail closed. -- **Fork PR credentialed E2E skip approval:** The primary fork path uses the same failed `E2E / PR Gate Coordination` check. Follow the explicit `E2E / PR Gate Controller run ` link embedded in the coordination summary rather than relying on its **Details** destination, choose **Review deployments**, select `approve-credentialed-e2e-skip-for-fork-pr`, add a comment if useful, and approve. This records that the selected credential-bearing E2E jobs and typed targets will not run; it does not authorize fork code to run with repository secrets. The waiting job has `deployment: false`, no secrets, and no PR-controlled execution. The controller reads GitHub's approval history, requires one approval for only that environment, verifies that the recorded reviewer still has `maintain` or `admin`, and then applies the existing PR/head/base, deterministic-plan, matching-failed-check, compatible-`main`, and final stale-revision checks. A recorded approval completes the coordination check successfully with `Credentialed E2E skipped for fork PR — approved by @` and leads with `Outcome: APPROVED SKIP — credentialed E2E did not run.` Treat this as an audited skip, not passing E2E evidence. Configure the environment before rollout with required reviewers whose approving members have `maintain` or `admin`; add no secrets, variables, or custom protection app, and preferably disable administrator bypass. If **Review deployments** is absent, the environment may be missing or unprotected, or the run may no longer be waiting. Configure it, update the PR to create a new head, and trigger fresh upstream PR CI for a new gate run, or use the manual fork-skip fallback below. Do not rerun the waiting workflow: environment approvals are not attempt-bound, so the controller accepts only the first attempt. Per-PR concurrency cancels an older waiting approval when a newer revision reaches the gate. -- **Manual fork PR credentialed E2E skip:** Use this only when the gate has failed with `Maintainer approval required to skip credentialed E2E`. Independently review the fork change and non-secret PR CI, then prefer the protected-environment approval above. The controller rejects a closed PR, a changed head or base, an internal PR, an empty E2E plan, a non-failed gate, a controller commit that is neither current `main` nor a safely validated ancestor of it, or a recorded reviewer below maintainer role. A safe `main` advance must preserve the controller commit as its merge base, contain fewer than 300 fully enumerated changed files, and avoid `e2e-control-plane` changes; other advances fail closed. It re-reads the open PR's head and base immediately before writing success. The result records the reviewer, optional bounded comment, validated approval-run URL, plan, and jobs and targets that did not run. For the fallback, choose **Run workflow** on `main`, select `approve-fork-e2e-skip`, and provide the fork PR number, its current 40-character head SHA as `expected_head_sha`, its current 40-character base SHA as `expected_base_sha`, and a specific 10–500-character reason. Re-read both PR SHAs immediately before dispatch. Optionally set `evidence_url` to a reviewed URL matching `https://github.com/NVIDIA/NemoClaw/actions/runs/`, or leave it blank when no supporting run exists. PR, issue, comment, job, and external URLs are rejected. The controller validates the optional URL's repository and run-ID shape but does not inspect that run's head, selected jobs or targets, or conclusion. -- **E2E control-plane authorization:** The deterministic `e2e-control-plane` family remains a conservative path boundary covering E2E and PR-CI workflows, risk policy, dependency and test configuration, preparation and upload actions, and non-documentation files under `tools/e2e/` and `test/e2e/`, including shell and Python support files. Internal PRs whose matched control-plane files are drawn only from `.github/workflows/pr-e2e-gate.yaml`, `tools/e2e/pr-e2e-gate.mts`, and `tools/e2e/pr-e2e-required.mts` automatically dispatch the selected jobs for that SHA. Any other or mixed internal control-plane diff fails with `Maintainer authorization required to run E2E` without running selected jobs or exposing repository secrets. Independently review the revision and its non-secret CI, then choose **Run workflow** on `main`, select `run-control-plane`, and provide the PR number, current 40-character `expected_head_sha`, current 40-character `expected_base_sha`, and a specific 10–500-character `review_reason`. Re-read both SHAs immediately before dispatch. The first-attempt workflow requires the triggering actor to have `maintain` or `admin`, and rejects forks, stale or closed PRs, plans that do not require authorization, empty selections, a missing or mismatched coordination check, and an incompatible trusted controller commit. It re-reads the current head and base before dispatch. Authorization returns `E2E / PR Gate Coordination` to in progress and runs the selected jobs through the normal wait, evidence-download, and finish path; it cannot directly record success. Only verified evidence for that SHA can make the native required `E2E / PR Gate` job green. -- **Typed-target authorization:** The risk plan can select an exact allowlisted typed target for a workflow-wired check and dispatch jobs and targets together in one child run. Every authorization, empty-selection, secret-exposure, skip, evidence, and finish boundary above applies to selected typed targets as well as jobs. -- **Two-phase ruleset rollout and backfill:** First deploy the E2E check producer and its trusted fork handling. Re-run `CI / Pull Request` for every already-open PR head/base pair (approving a first-time fork run when necessary), then verify that `E2E / PR Gate` is attached to that same head SHA for the current base. The fail-closed maintainer checker identifies any head still needing backfill. Adding the workflow does not retroactively report the context, and enabling the ruleset first leaves existing PRs at "Waiting for status to be reported." Do not activate the context as a security boundary while it is scoped only to the shared GitHub Actions app: that app identity cannot distinguish this workflow from another workflow. First give the gate distinct provenance through a dedicated GitHub App or an organization required-workflow rule. When the context is finally activated, also enable strict/up-to-date required status checks; otherwise a successful head can remain mergeable after `main` advances and changes the effective merge diff. Preserve the control-plane review and deterministic floor even after those external protections are in place. -- **Contributor compliance failed:** Reject the PR and ask the contributor to provide the PR-body DCO declaration or replace unverified commits with a clean verified history. The only PR-body declaration exemption is the two exact Dependabot logins above; it does not exempt commit verification. Do not approve, merge, amend, sign, or force-push on the contributor's behalf. -- **Contributor/approver overlap:** Surface `advisories.contributorApprovalOverlap` when the same account not recognized as automated by the supported login conventions appears as the current PR opener, commit author, or co-author and its latest opinionated review is approved. The invalid state detected here is contributor and approver identity overlap in the current GitHub PR metadata; the source boundary is the current opener plus all commit-author and review pages fetched through GitHub's GraphQL API. The advisory includes contributors whose commits remain in the current PR head at check time; it does not retain original push actors or authors removed when history is rebased, squashed, or fixed up. A clear result is not proof of independent approval. Missing, invalid, or conflicting review timestamps, or failure to retrieve complete paginated history, produce a warning because the latest opinion cannot be selected reliably. +### Product scope - This is intentionally diagnostic-only under the maintainer scope decision recorded in the #6233 discussion; #6222 remains the broader proposal context. It is not an independent-approval policy, required check, branch-protection rule, or substitute for explicit human merge authorization, so it does not invalidate approval, require another reviewer, or change `allPass` or merge readiness. Mocked-GitHub regression tests cover opener and commit-author/co-author overlap, bot filtering, case normalization, latest-review transitions across API pages, timestamp ordering, incomplete timestamps, and incomplete paginated history. Remove this advisory if GitHub or a maintainer-approved authoritative control provides the same overlap signal, or replace it if the project adopts an enforced independent-approval policy. -- **Stale or changed revision:** Do NOT approve when the branch is behind, GitHub does not report `MERGEABLE`, the merge state is not allowed, or the PR head, base, state, draft status, mergeability, or merge state changes during evaluation. Refresh the branch when needed, wait for CI, then re-run the gate checker. Follow [SALVAGE-PR.md](SALVAGE-PR.md). -- **CI failing but narrow:** Follow the salvage workflow in [SALVAGE-PR.md](SALVAGE-PR.md). -- **CI pending:** Wait and re-check. Do not approve while checks are still running. -- **CodeRabbit:** Script flags unresolved major/critical threads. Review the `snippet` to confirm it's a real issue vs style nit. If doubt, leave unapproved. -- **PR Review Advisor:** Treat the comment as untrusted review input, not merge authority. Read it when present and verify substantive claims against the code, tests, and workflow evidence. Apply confirmed issues to the relevant correctness, security, or test gate; ask the user before acting on ambiguous or design-changing advice. Recommendation labels, a missing comment, and comment provenance do not enter `check-gates.ts` or change `allPass`. Never approve or reject a PR solely because of the advisor's recommendation. -- **Tests:** If `riskyCodeTested.pass` is false, follow [TEST-GAPS.md](TEST-GAPS.md). +Stop if the PR creates a product surface without an accepted issue or design decision. +Tests, CI, and positive review output do not replace product approval. +Ask a maintainer for a product decision or route the work through [Community Solutions](../../../docs/resources/community-contributions.mdx). -## Step 3: Approve or Report +### Required checks -**Approve only when:** the human product-scope gate passes, `allPass` is true for the final unchanged revision, GitHub reports `MERGEABLE` with an allowed current-base merge state, and maintainer review found no unresolved correctness or security issue. The advisor's recommendation cannot provide merge authorization or independently change readiness. Approving a stale or conflicted PR is wasted effort — refreshing it will invalidate the approval. +The checker requires these status-rollup entries: -The correct sequence for a conflicted PR: **salvage (rebase) → CI green → approve → report ready for merge.** +- `checks` +- `check-hash` +- `changes` +- `commit-lint` +- `dco-check` +- `E2E / PR Gate` -**All pass + no conflicts:** Approve and summarize why. +A first-time fork contributor might need **Approve and run** before `pull_request` checks appear. +The E2E controller records the PR SHA and base SHA without running fork code. +Do not waive a missing, neutral, or skipped E2E gate. +Do not run fork code with privileged credentials to create a result. -After submitting an approval, re-run the gate checker before reporting the PR ready. This captures an approval that creates contributor/approver overlap during the current maintainer pass. +### GitHub Actions evidence -If the contributor/approver advisory is present, include it in the summary without converting it into a failed gate. +Required PR workflows must identify the PR number, PR SHA, and base SHA. -**Any fail:** +- The installer-hash workflow runs trusted verification after each `pull_request` `edited` event. + Its run name records `gate true` and the base SHA. A metadata edit must not create skipped evidence. +- Each `pull_request_target` E2E controller run that performs work must use an immutable `gate true` run name. + The run name must identify the PR number, PR SHA, and base SHA. +- GitHub usually associates the run with the PR. + If that association is empty, require the controller path, branch, repository, and run time to match the coordination check. +- Discard a later all-skipped `gate false` metadata run only when a stronger run exists for the same PR SHA and base SHA. +- Fail closed when identity, state, or timing evidence is missing, malformed, stale, contradictory, or changed. -| Gate | Status | What is needed | -|------|--------|----------------| -| CI | Failing | Fix flaky timeout test | -| Conflicts | Not `MERGEABLE` or merge state not allowed/current | Rebase onto main first — approval would be invalidated | +### Controller status and PR status -Use full GitHub links. +`E2E / PR Gate Controller` reports whether the controller published an outcome. +It can pass while `E2E / PR Gate Coordination` and the required `E2E / PR Gate` job fail. +Use the required `E2E / PR Gate` job as merge evidence. +Use the coordination check for the verdict and evidence links. + +`Superseded by PR update` and `PR closed — gate no longer applies` cancel checks for a prior SHA. +Do not act on those checks. The PR SHA and base SHA still need a successful gate. +The closed-PR outcome also covers a deleted fork repository with no head-repository value. + +### Retry an E2E gate + +Rerun `CI / Pull Request` only when the failed coordination check has a supported retry reason for its gate version. +The retry must apply to the PR SHA and base SHA. + +- `prerequisite-ci` — Rerun CI. Let the controller retry after CI passes. +- `child-cancelled` — Rerun CI when the child workflow was cancelled. + You can also rerun it when every listed non-passing job was cancelled. +- `evidence-download` — Rerun CI when a successful child's evidence download failed, was cancelled, or was skipped. + +The controller keeps each completed coordination check as audit history. +For a retry, it creates an `in_progress` check for the same PR SHA and base SHA. +The controller and observer select the check with the highest ID only when all older duplicates are completed failures with supported retry markers. +Fail closed for an unexpected app, identity mismatch, duplicate ID, unsupported terminal state, or multiple active checks. + +Do not retry these terminal failures on the same SHA: + +- Selected-job or typed-target product failures. +- Assertion failures. +- Evidence policy or integrity failures. +- Reconciliation or controller errors. +- Unknown states. +- Failures recorded before retry reasons existed. + +Push a change to create another SHA, and then run CI again. +A passing controller does not override a failing required job. + +### Evidence download failures + +If a selected child passes but `Download evidence` fails, the gate fails closed. +This also applies when the step is cancelled or skipped. +The coordination check records `evidence-download`, and the controller fails. + +Inspect the download step before you rerun eligible PR CI. +Do not use the successful child by itself as evidence. + +A completed download can still produce a failing PR verdict. +This occurs when signals are missing, duplicated, skipped, pending, or failing. +In this case, the controller can pass without a retry reason. + +Malformed or unsafe evidence is a terminal controller error. +Schema mismatches, identity mismatches, and traversal-limit errors are also terminal. +The coordination check, required job, and controller must fail closed. + +### Approve an E2E skip for a fork PR + +Use the protected-environment path when the coordination check reports `Maintainer approval required to skip credentialed E2E`. + +1. Follow the `E2E / PR Gate Controller run ` link in the coordination summary. +2. Select **Review deployments**. +3. Select `approve-credentialed-e2e-skip-for-fork-pr`. +4. Add a comment when useful, and approve. + +This approval records that credential-bearing jobs and targets did not run. +It does not authorize fork code to use repository secrets. +The waiting job has `deployment: false`, no secrets, and no PR-controlled execution. + +The controller reads the approval history and requires one approval for that environment. +The reviewer must still have `maintain` or `admin` access. +The controller also checks the PR number, PR SHA, base SHA, plan, failed check, compatible `main`, and PR state. + +An accepted approval completes the coordination check with this result: +`Credentialed E2E skipped for fork PR — approved by @`. +The summary starts with `Outcome: APPROVED SKIP — credentialed E2E did not run.` +Treat this result as an audited skip, not as E2E evidence. + +Configure the environment before rollout. +Require reviewers with `maintain` or `admin` access. +Do not add secrets, variables, or a protection app. Disable administrator bypass when possible. + +If **Review deployments** is absent, the environment might be missing, unprotected, or no longer waiting. +Configure it, push a change, and run PR CI again. You can also use the manual fallback below. +Do not rerun the waiting workflow. The controller accepts an environment approval only on the first attempt. +Per-PR concurrency cancels a waiting approval when another SHA reaches the gate. + +### Use the manual fork-skip fallback + +Use this fallback only after the gate reports `Maintainer approval required to skip credentialed E2E`. +Review the fork change and non-secret PR CI first. Prefer the protected-environment path. + +The controller rejects these states: + +- The PR is closed. +- The PR SHA or base SHA changed. +- The PR is internal. +- The E2E plan is empty. +- The gate did not fail. +- The controller commit is not `main` or a validated ancestor of `main`. +- The reviewer does not have maintainer access. + +A permitted `main` advance must keep the controller commit as its merge base. +It must contain fewer than 300 listed changed files and no `e2e-control-plane` changes. +Other advances fail closed. +Immediately before it writes success, the controller confirms that the PR is open and that the PR SHA, base SHA, and coordination identity still match. +It fails closed if any value changed or does not match. +Its result records the reviewer, optional comment, approval-run URL, plan, and work that did not run. + +To use the fallback, select **Run workflow** on `main` and then select `approve-fork-e2e-skip`. +Provide these inputs: + +- The fork PR number. +- The 40-character PR SHA as `expected_head_sha`. +- The 40-character base SHA as `expected_base_sha`. +- A reason of 10 to 500 characters. + +Read both SHAs before dispatch. +You can give an `evidence_url` that matches `https://github.com/NVIDIA/NemoClaw/actions/runs/`. +Leave it blank when no supporting run exists. +The workflow rejects PR, issue, comment, job, and external URLs. +It validates the repository and run-ID format. It does not inspect that run's SHA, jobs, targets, or result. + +### Authorize E2E control-plane changes + +The `e2e-control-plane` path group includes these areas: + +- E2E and PR-CI workflows. +- Risk policy. +- Dependency and test configuration. +- Preparation and upload actions. +- Non-documentation files under `tools/e2e/` and `test/e2e/`. +- Shell and Python support files in those directories. + +An internal PR can run automatically when it changes only these files: + +- `.github/workflows/pr-e2e-gate.yaml` +- `tools/e2e/pr-e2e-gate.mts` +- `tools/e2e/pr-e2e-required.mts` + +Another control-plane change fails with `Maintainer authorization required to run E2E`. +The gate must not run selected jobs or expose secrets before authorization. + +Review the PR SHA and non-secret CI. +Then, select **Run workflow** on `main` and select `run-control-plane`. +Provide the PR number, 40-character `expected_head_sha`, 40-character `expected_base_sha`, and a `review_reason` of 10 to 500 characters. +Read both SHAs before dispatch. + +The first attempt requires the actor to have `maintain` or `admin` access. +The workflow rejects forks, stale or closed PRs, plans that need no authorization, and empty selections. +It also rejects a missing coordination check, an identity mismatch, or an incompatible controller commit. +It reads the PR SHA and base SHA before dispatch. +Before any result reaches success, it confirms that the PR is open and that the PR SHA, base SHA, and coordination identity still match. +It fails closed if any value changed or does not match. + +Authorization returns `E2E / PR Gate Coordination` to `in_progress`. +It runs selected jobs through the wait, evidence-download, and finish process. +Authorization cannot record success by itself. +Only verified evidence for the PR SHA can make `E2E / PR Gate` pass. + +### Authorize a typed target + +The risk plan can select a target from the allowlist for a workflow check. +It can dispatch jobs and targets in one child run. +Apply all authorization, selection, secret, skip, evidence, and finish rules to jobs and targets. + +### Roll out a required E2E context + +Use this order: + +1. Deploy the E2E check producer and its fork handling. +2. Rerun `CI / Pull Request` for each open PR SHA and base SHA. + Approve a first-time fork run when necessary. +3. Verify that `E2E / PR Gate` is attached to that PR SHA and base SHA. +4. Use the gate checker to find PRs that still need a check. + +Do not enable the required context before the producer is ready. +GitHub does not create a context for prior runs. +If you enable the rule first, open PRs can wait for a status that does not exist. + +Do not use the shared GitHub Actions app identity as the security boundary. +It cannot distinguish this workflow from another workflow. +First use a dedicated GitHub App or an organization required-workflow rule. +Then enable strict, up-to-date status checks. +Without this setting, a successful PR SHA can remain mergeable after `main` changes the merge result. +Keep the control-plane review and gate checks after rollout. + +### Contributor requirement failure + +Reject a PR that lacks the PR-body DCO declaration or has an unverified commit. +Ask the contributor to correct the PR body or replace the commit history. +Only the two Dependabot logins above do not need the PR-body declaration. +They still need verified commits. +Do not approve, merge, amend, sign, or force-push for the contributor. + +### Contributor and approver overlap + +Report `advisories.contributorApprovalOverlap` when the same non-bot account contributes and approves. +The contributor set contains the PR opener, commit authors, and co-authors. +Use the account's most recent opinionated review. + +Read all GraphQL pages for contributors and reviews. +The advisory includes contributors whose commits remain in the PR SHA. +It does not retain push actors or authors removed from the history. +If review timestamps are missing, invalid, or conflicting, report a warning. +Also report a warning if all pages cannot be read. + +The advisory does not prove that approval is independent. +It is not a policy, required check, or branch-protection rule. +It does not change `allPass`, approval, or merge readiness. +This scope follows the maintainer decision in issue #6233. Issue #6222 contains the related proposal. + +Tests cover opener, author, and co-author overlap. +They also cover bot filtering, case normalization, review changes, pagination, and timestamp errors. +Remove the advisory if GitHub or approved policy provides the same signal. +Replace it if the project adopts an independent-approval requirement. + +### Other results + +- **Base or PR changed:** Do not approve. + Refresh the branch when needed, wait for CI, and run the checker again. + Follow [SALVAGE-PR.md](SALVAGE-PR.md). +- **CI failure with a small fix:** Follow [SALVAGE-PR.md](SALVAGE-PR.md). +- **CI pending:** Wait and check again. Do not approve. +- **CodeRabbit finding:** Read the snippet. Decide whether it reports a correctness or security problem, or a style comment. +- **PR Review Advisor finding:** Treat it as review input, not merge authority. + Verify each claim against code, tests, and workflow evidence. + Apply confirmed problems to a gate. Ask the user about ambiguous or design-changing advice. + Advisor labels, absence, and source do not affect `check-gates.ts` or `allPass`. +- **Missing tests:** Follow [TEST-GAPS.md](TEST-GAPS.md). + +## Step 3: Approve or report + +Approve only when all these conditions are true: + +- The product-scope gate passes. +- `allPass` is true for the PR SHA and base SHA. +- GitHub reports `MERGEABLE` and a permitted merge state. +- No correctness or security problem remains. + +The advisor cannot authorize a merge or change readiness. +Do not approve a stale or conflicted PR. A later refresh invalidates the approval. + +For a conflicted PR, use this order: + +1. Rebase and resolve conflicts. +2. Wait for CI to pass. +3. Approve. +4. Report that the PR is ready for a merge decision. + +After approval, run the gate checker again. +This check can find contributor and approver overlap created by the approval. +Report that advisory. Do not treat it as a failed gate. + +If a gate fails, report the gate and the required action: + +| Gate | Status | Required action | +|------|--------|-----------------| +| CI | Failing | Fix the named job or test. | +| Conflicts | GitHub does not report `MERGEABLE`, or the merge state is not permitted for the base SHA. | Rebase before approval. | + +Use GitHub links. diff --git a/.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md b/.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md index bfa2ffed5dd..de846491302 100644 --- a/.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md +++ b/.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md @@ -3,34 +3,48 @@ # PR Review Priorities -Ordered list of what NemoClaw maintainers look for in a pull request. Higher items block approval; lower items inform queue ranking. +Use this order when you review a PR. Hard gates block approval. Queue signals set review order. ## Hard gates (all must pass to approve) -1. **Product scope approved** — the PR implements existing supported behavior or a linked, accepted product decision. Working code and green checks do not authorize a new integration, solution, third-party stack, custom image, or documentation surface. If ownership and lifecycle are not established, do not approve. Route independent solutions through [Community Solutions](../../../docs/resources/community-contributions.mdx). -2. **Contributor compliance** — the PR body has the contributor's DCO declaration and every commit appears as `Verified` in GitHub. Maintainers reject noncompliant PRs and do not repair contributor history. -3. **Security correctness** — no sandbox escape, SSRF, credential exposure, policy bypass, or installer trust violation. PRs touching risky areas (see [RISKY-AREAS.md](RISKY-AREAS.md)) get a deep security pass before anything else. +1. **Product scope approved** — The PR implements supported behavior or a linked product decision. + Working code and passing checks do not approve a new product surface. + Do not approve when ownership and lifecycle are not defined. + Route independent solutions through [Community Solutions](../../../docs/resources/community-contributions.mdx). +2. **Contributor compliance** — The PR body has the contributor's DCO declaration. + GitHub shows every commit as `Verified`. Maintainers reject a noncompliant PR and do not repair its history. +3. **Security correctness** — No sandbox escape, SSRF, credential exposure, policy bypass, or installer trust violation exists. + Run the nine-category security review first when a PR touches a [risky area](RISKY-AREAS.md). 4. **CI green** — all required checks in `statusCheckRollup` must pass. 5. **No merge conflicts** — `mergeStateStatus` must be clean. -6. **No unresolved major/critical CodeRabbit findings** — correctness and safety findings block; style nits do not. Use judgment on borderline cases. +6. **No unresolved major or critical CodeRabbit findings** — Correctness and safety findings block the PR. Style comments do not. Assess borderline cases. 7. **Tests for touched risky code** — risky areas must have test coverage, either added in the PR or pre-existing. No exceptions. ## Manual review inputs -The PR Review Advisor provides a second opinion, not merge authority. Read its comment when present and verify each substantive claim against the code, tests, and workflow evidence. Apply confirmed issues to the relevant correctness, security, or test gate; ask the user before acting on ambiguous or design-changing advice. Its recommendation labels, absence, and comment provenance do not affect `check-gates.ts` or `allPass`, and a positive recommendation never substitutes for explicit human authorization. +The PR Review Advisor provides review input. It does not authorize a merge. +Read its comment and verify each claim against code, tests, and workflow evidence. +Apply a confirmed problem to the related gate. Ask the user about ambiguous or design-changing advice. +Advisor labels, absence, and comment source do not affect `check-gates.ts` or `allPass`. + +Apply [NemoClaw Technical English](../../../CONTRIBUTING.md#nemoclaw-technical-english) to changed comments, test titles, PR discussion, changelog entries, and Announcements. +During the changed-text pilot, a language finding blocks only when ambiguity can change behavior, security, data safety, test meaning, or release meaning. +Treat other findings as suggestions and include a proposed rewrite. ## Quality expectations (block if violated, but fixable via salvage) -1. **Narrow scope** — each PR has one clear objective. Unrelated config changes, drive-by refactors, and tool setting diffs get reverted to `main`. +1. **Narrow scope** — Each PR has one objective. + Restore configuration, refactor, and tool-setting changes that do not support the objective. 2. **Contributor intent preserved** — the fix must match what the contributor intended. Stop and ask when the diff would change semantics or when intent is unclear. -3. **Small, mergeable changes** — prefer substrate-first slicing: extract helper, add tests for current behavior, land fix on top. One file cluster per pass. If the next step is a large redesign, route to sequencing. +3. **Small changes** — Extract a helper, test the existing behavior, and then apply the fix. + Process one file cluster in each pass. Use sequencing when the next step requires a redesign. ## Queue ranking signals (inform priority, not approval) -1. **Actionability** — PRs closest to done rank highest. A merge-ready PR outranks a near-miss; a near-miss outranks a blocked item. -2. **Security-sensitive and actionable** — PRs touching risky code get a priority bump, but only when they are not otherwise blocked. -3. **Staleness** — PRs idle for more than 7 days get a mild bump to prevent rot. -4. **Hotspot relief** — PRs that reduce future conflict pressure in high-churn files are preferred over equivalent work elsewhere. +1. **Actionability** — Rank an approval-ready PR before one that needs a fix. Rank a PR that needs a fix before a blocked PR. +2. **Security** — Rank an actionable PR that touches risky code before an equivalent PR. +3. **Wait time** — Rank a PR that has waited more than seven days before an equivalent newer PR. +4. **Merge conflicts** — Prefer a PR that reduces conflicts in files that change often. ## Daily cadence @@ -38,15 +52,23 @@ The team follows a daily ship cycle. All maintainer skills operate within this r 1. **Morning** (`/nemoclaw-maintainer-morning`) — triage the backlog, pick items for the day, label them with the target version (e.g., `v0.0.8`). 2. **During the day** (`/nemoclaw-maintainer-day`) — land PRs using the maintainer loop. Version labels make progress visible on dashboards. -3. **Evening** (`/nemoclaw-maintainer-evening`) — check what shipped, merge or explicitly waive the pre-tag release-note docs PR containing `docs/changelog/YYYY-MM-DD.mdx` for the target version, identify open stragglers, generate a QA-focused summary, freeze the candidate SHA, collect the E2E evidence or itemized maintainer exceptions required before confirmation, cut the tag, automatically carry stragglers to the next patch, delete the released label, and prepare the Announcement for posting. -4. **Overnight** — QA team (different timezone) performs additional validation of the tag. Any issues they file enter the next morning's triage like any other issue. - -Version labels activate release work; they are not readiness claims. If an open item misses the tag, its label moves to the next patch during post-tag housekeeping. After no open item remains, housekeeping deletes the released label; it never renames or reuses it. - -## Explicitly not priorities - -- **Code style and formatting** — not a reason to block or delay. No opportunistic reformatting. +3. **Evening** (`/nemoclaw-maintainer-evening`) — Check shipped work and the pre-tag changelog PR. + Confirm that the pre-tag changelog PR contains `docs/changelog/YYYY-MM-DD.mdx` for the release. + Identify open items and prepare the QA summary. Record the release SHA and required E2E evidence. + Cut the tag after confirmation. Move open items to the next patch label and delete the released label. + Prepare the Announcement. +4. **Overnight** — A QA team in another time zone validates the tag. + Put new issues into the next morning's triage. + +Version labels activate release work. They do not show readiness. +If an open item misses the tag, move its label to the next patch after the release. +Delete the released label when no open item has it. Do not rename or reuse it. + +## Not priorities + +- **Code style and formatting** — Do not block or delay a PR for style. Do not change unrelated formatting. +- **Unrelated language cleanup** — Do not expand the PR beyond changed text during the technical-English pilot. - **Documentation completeness** — not required for approval unless the PR changes user-facing behavior. -- **Architectural elegance** — the goal is lower future merge pain, not aesthetic cleanup. +- **Architecture style** — Reduce future merge conflicts. Do not add style-only refactors. Product scope approval is distinct from architectural elegance and remains a hard gate. diff --git a/.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md b/.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md index fa114b52b9a..9aaffa226e4 100644 --- a/.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md +++ b/.agents/skills/nemoclaw-maintainer-day/RISKY-AREAS.md @@ -1,3 +1,6 @@ + + + # NemoClaw Risky Code Areas PRs touching these areas need tests before approval. @@ -10,5 +13,5 @@ PRs touching these areas need tests before approval. | Workflow / enforcement | `.github/workflows/`, prek hooks, DCO, signing, version/tag flows | | Credentials / inference / network | credential helpers, inference provider routing, approval flows | -A PR in a risky area is only promoted in the queue when it is actually actionable. -If risky and under-tested, follow the test gaps or security sweep workflows. +Promote a PR in a risky area only when it is actionable. +If risky code does not have sufficient tests, follow the test-gap or security-review workflow. diff --git a/.agents/skills/nemoclaw-maintainer-day/SALVAGE-PR.md b/.agents/skills/nemoclaw-maintainer-day/SALVAGE-PR.md index 3ad7767b9ff..728eef4686f 100644 --- a/.agents/skills/nemoclaw-maintainer-day/SALVAGE-PR.md +++ b/.agents/skills/nemoclaw-maintainer-day/SALVAGE-PR.md @@ -3,9 +3,10 @@ # Salvage PR Workflow -Take one near-mergeable PR and make the smallest safe change to unblock it. +Select a PR that needs a small fix. Make only the change needed to unblock it. -**Default to maintainer salvage.** When a maintainer picks an item from the queue, the assumption is they're doing the work now — rebase, fix conflicts, add missing tests, push. Do not default to "ask the contributor and wait" because that blocks the daily cadence. Only defer to the contributor when the fix requires understanding intent that isn't clear from the diff. +**Default to maintainer work.** Rebase, resolve conflicts, add missing tests, and push when the task permits these actions. +Do not wait for the contributor unless the diff does not show the intended behavior. ## Step 1: Gather Context @@ -16,13 +17,16 @@ gh pr view --repo NVIDIA/NemoClaw \ gh pr diff --repo NVIDIA/NemoClaw ``` -Also read: maintainer and CodeRabbit comments, linked issues, recent `main` changes in touched files. Understand the PR's purpose before coding. +Also read maintainer comments, CodeRabbit comments, linked issues, and recent `main` changes in affected files. +Understand the PR objective before you change code. ## Step 2: Assess Fit -**Maintainer does it now:** rebase and resolve conflicts, add missing tests for risky code, fix one or two failing checks, apply small correctness fixes from review, narrow gate cleanup. +**Maintainer does it now:** Rebase and resolve conflicts. Add missing tests for risky code. +Fix one or two failing checks or small correctness problems. -**Defer to contributor only when:** the fix requires a design change the maintainer can't judge from the diff, contributor intent is ambiguous and the wrong guess would change semantics, or the PR spans multiple subsystems the maintainer isn't familiar with. +**Defer to the contributor when:** The fix requires a design decision that the diff does not answer. +Also defer when intent is unclear or the fix crosses multiple subsystems. ## Step 3: Check Out and Reproduce @@ -31,21 +35,25 @@ gh pr checkout git fetch origin --prune ``` -Reproduce locally. Run narrowest relevant commands first. +Reproduce the problem locally. Run the most focused applicable command first. ## Step 4: Review PR Scope Before Fixing -Before fixing, review **all** changed files in the PR — not just the ones causing failures. Flag any files that expand the PR's scope unnecessarily (config changes, unrelated refactors, tool settings). Revert those to `main` if they aren't needed for the feature to work. +Review all changed files before you fix the blocker. +Identify configuration, refactor, or tool-setting changes that do not support the PR objective. +Restore those files from `main` when the PR does not need them. ## Step 5: Fix Narrowly -Smallest change that clears the blocker. No opportunistic reformatting. +Make only the change needed to clear the blocker. Do not reformat unrelated code. -If risky code is touched (see [RISKY-AREAS.md](RISKY-AREAS.md)), treat missing tests as part of the fix — follow [TEST-GAPS.md](TEST-GAPS.md) when needed. +If the PR changes risky code, include missing tests in the fix. +See [RISKY-AREAS.md](RISKY-AREAS.md) and [TEST-GAPS.md](TEST-GAPS.md). ## Step 6: Conflicts -Resolve only mechanical conflicts (import ordering, adjacent additions, branch drift). Stop and summarize if the conflict changes behavior. +Resolve a conflict only when the resolution does not change behavior. +Stop and report a conflict that can change behavior. ## Step 7: Validate @@ -60,34 +68,45 @@ Use only commands matching the changed area. ## Step 8: Push -Push when: fix is small, improves mergeability, validation passed, you have push permission. Never force-push. +Push only if the fix is small, improves mergeability, passes validation, and you can push. +Never force-push. -If the push fails because of SSH, authentication, remote access, authorization, or permission problems, follow [Git and GitHub Access Hard Stop](../_shared/git-github-hard-stop.md). Resolve ordinary merge conflicts or dirty-worktree state in the salvage workflow. +If Git or GitHub access prevents the push, follow [Stop for Git and GitHub Access Errors](../_shared/git-github-hard-stop.md). +Resolve merge conflicts and dirty-worktree problems in this workflow. -**Fork PRs:** Most PRs come from contributor forks. Check where to push: +Check the PR's head repository before you push: ```bash gh pr view --repo NVIDIA/NemoClaw --json headRepositoryOwner,headRepository,headRefName,maintainerCanModify ``` -If `maintainerCanModify` is true, push directly to the fork: +If `headRepository.nameWithOwner` is `NVIDIA/NemoClaw`, push to the PR branch on `origin`: + +```bash +git push origin : +``` + +For a fork PR, push only when `maintainerCanModify` is true: ```bash git push git@github.com:/.git : ``` -Do **not** push to `origin` — that creates a separate branch on NVIDIA/NemoClaw that won't appear in the PR. +For a fork PR, do not push to `origin`. +If `maintainerCanModify` is false, do not push. ## Step 9: Monitor After Push -After any maintainer push, follow [PR CI and Review Follow-Up](../_shared/pr-follow-up.md) before routing onward. Keep salvage narrow: address valid correctness, security, and test-coverage findings with the smallest safe follow-up; consult the user when feedback is ambiguous, design-changing, or outside the salvage scope. +After a maintainer push, follow [Follow Up on PR CI and Reviews](../_shared/pr-follow-up.md). +Fix valid correctness, security, and test-coverage findings within the PR scope. +Ask the user about ambiguous or design-changing feedback. ## Step 10: Route to Merge Gate -If PR looks ready after CI and feedback settle, follow [MERGE-GATE.md](MERGE-GATE.md). +After CI and review finish, follow [MERGE-GATE.md](MERGE-GATE.md) if the PR is ready. ## Notes -- Goal is safe backlog reduction, not finishing the PR at any cost. +- Reduce the backlog without accepting unresolved risk. - Never hide unresolved reviewer concerns. - Use full GitHub links. diff --git a/.agents/skills/nemoclaw-maintainer-day/SECURITY-SWEEP.md b/.agents/skills/nemoclaw-maintainer-day/SECURITY-SWEEP.md index 8d52b25a331..bb03b71d68e 100644 --- a/.agents/skills/nemoclaw-maintainer-day/SECURITY-SWEEP.md +++ b/.agents/skills/nemoclaw-maintainer-day/SECURITY-SWEEP.md @@ -3,19 +3,21 @@ # Security Sweep Workflow -Review a security-sensitive item before it enters the normal fast path. +Review a security-sensitive item before normal PR processing. ## Step 1: Identify the Security Item -The morning triage and `find-review-pr` already surface security-labeled PRs. Start from the item selected in the day loop's action step. If running standalone, check the triage queue for PRs touching risky areas (see [RISKY-AREAS.md](RISKY-AREAS.md)). +Start with the item selected by the maintainer-day loop. +For a separate review, check the triage queue for PRs that touch [risky areas](RISKY-AREAS.md). ## Step 2: Gather Context -Read the PR or issue, all comments, CodeRabbit findings, PR Review Advisor feedback, linked items, changed files, diff, current checks, and recent relevant `main` commits. +Read the PR or issue, comments, automated-review findings, linked items, changed files, and diff. +Read the check results and related `main` commits. ## Step 3: Classify Risk -Which bucket applies? +Classify the risk: - **escape or policy bypass** - **credential or secret exposure** @@ -24,24 +26,39 @@ Which bucket applies? - **input validation or SSRF weakness** - **test gap in risky code** -If none apply, route back to normal action selection. +If no risk class applies, return to action selection. -## Step 4: Deep Security Pass +## Step 4: Review security -Load `security-code-review` for the nine-category review whenever the item changes behavior in a security-sensitive area. Do not skip this step just because the diff is small. +Load `nemoclaw-maintainer-security-code-review` when the item changes behavior in a security-sensitive area. +Run all nine categories, including for a small diff. ## Step 5: Decide Action ### Salvage-now -All true: risk is understood, fix is small/local, required tests are clear, no unresolved design question. Follow [SALVAGE-PR.md](SALVAGE-PR.md) and [TEST-GAPS.md](TEST-GAPS.md). +Use salvage when all these conditions are true: + +- The risk is understood. +- The fix is small and local. +- The required tests are known. +- No design question remains. + +Follow [SALVAGE-PR.md](SALVAGE-PR.md) and [TEST-GAPS.md](TEST-GAPS.md). ### Blocked -Any true: fix changes core trust assumptions, review found real vulnerability needing redesign, PR adds risk without tests, reviewer disagreement. Summarize blocker clearly; do not approve. +Block approval when one of these conditions is true: + +- The fix changes a trust assumption. +- The review finds a vulnerability that needs redesign. +- The PR adds risk without tests. +- Reviewers disagree about the security effect. + +Report the blocker. Do not approve. ## Notes -- Backlog reduction never outranks a credible security concern. -- No security-sensitive approvals without both deep review and tests. -- Use full GitHub links. +- A security concern takes priority over backlog reduction. +- Do not approve a security-sensitive change until the security review and tests pass. +- Use GitHub links. diff --git a/.agents/skills/nemoclaw-maintainer-day/SEQUENCE-WORK.md b/.agents/skills/nemoclaw-maintainer-day/SEQUENCE-WORK.md index 67a316da6d3..e14e6a5a069 100644 --- a/.agents/skills/nemoclaw-maintainer-day/SEQUENCE-WORK.md +++ b/.agents/skills/nemoclaw-maintainer-day/SEQUENCE-WORK.md @@ -1,33 +1,41 @@ -# Sequence Work Workflow + + -Turn a large problem into a short sequence of mergeable slices. +# Sequence Work -## Step 1: Read the Full Problem Surface +Divide the work into changes that can merge separately. -Read greedily: issue body and comments, linked issues, linked PRs with review comments and status, touched code/tests/docs, recent `main` changes if the area is active. +## Step 1: Read the context -Do not sequence work from the title alone. +Read the issue body, comments, linked issues, and linked PRs. +Read review comments, PR status, changed files, tests, and documentation. +If the files change often, read recent `main` changes. -## Step 2: Identify What Is Moving +Do not use the title as the only source. -Inventory overlapping open PRs and recently merged changes. For each: blocker that must land first? Dependency to build on? Conflict to avoid? Noise? +## Step 2: List related changes + +List overlapping PRs and recent merged changes. +For each item, record prerequisites, dependencies, conflicts, and unrelated changes. ## Step 3: Define Slices -Each slice should have: one core objective, short file list, explicit tests, merge dependency list, stop condition. +For each change, give one objective, a file list, tests, merge dependencies, and a stop condition. -Prefer substrate-first sequencing: +Use this sequence when possible: 1. Extract stable helper or type boundary 2. Add regression tests for current behavior -3. Land behavioral fix or refactor on top +3. Apply the behavior change or refactor 4. Remove duplication afterward ## Step 4: Rank Use repo priorities: (1) backlog reduction, (2) security, (3) test coverage, (4) hotspot cooling. +An identified security concern overrides this default order. -A slice that unblocks several PRs moves up. An elegant but non-urgent slice moves down. +Give more priority to a change that unblocks several PRs. +Give less priority to style-only work. ## Step 5: Output @@ -35,9 +43,11 @@ A slice that unblocks several PRs moves up. An elegant but non-urgent slice move |-------|-------|---------|------------|-------| | 1 | Extract timeout parsing from onboard | Enables safe tests, reduces conflicts | None | Unit tests for invalid env values | -Also include: outstanding blockers, which slices are safe for the maintainer loop, where human design decisions are needed. +Also list outstanding blockers. +Identify changes that the maintainer loop can make. +Identify decisions that require a user. ## Notes -- Every slice maps to real files, tests, and merge behavior — not abstract architecture. -- Prefer small serially mergeable changes over one ambitious cleanup branch. +- Each change must name files, tests, and merge behavior. +- Prefer changes that can merge one at a time. diff --git a/.agents/skills/nemoclaw-maintainer-day/SKILL.md b/.agents/skills/nemoclaw-maintainer-day/SKILL.md index 293c6a7aee4..ef3f4c082e5 100644 --- a/.agents/skills/nemoclaw-maintainer-day/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-day/SKILL.md @@ -1,14 +1,19 @@ --- name: nemoclaw-maintainer-day -description: Runs the daytime maintainer loop for NemoClaw, prioritizing items labeled with the current version target. Picks the highest-value item, executes the right workflow (merge gate, salvage, security sweep, test gaps, hotspot cooling, or sequencing), and reports progress. Use during the workday to land PRs and close issues. Designed for /loop (e.g. /loop 10m /nemoclaw-maintainer-day). Trigger keywords - maintainer day, work on PRs, land PRs, make progress, what's next, keep going, maintainer loop. +description: Run one NemoClaw daytime maintainer pass. Prioritize items for the release version. Select a merge, salvage, security, test, conflict, or sequencing workflow and report progress. Use during the workday to land PRs and close issues. Designed for /loop, for example /loop 10m /nemoclaw-maintainer-day. Trigger keywords - maintainer day, work on PRs, land PRs, make progress, what's next, keep going, maintainer loop. user_invocable: true --- + + + # NemoClaw Maintainer Day Execute one pass of the maintainer loop, prioritizing version-targeted work. -**Autonomy:** push small fixes and approve when gates pass. Surface contributor/approver overlap reported by the merge gate as an advisory warning; it does not require another reviewer or change merge readiness. Never merge. Stop and ask for merge decisions, product scope decisions, architecture decisions, and unclear contributor intent. +**Autonomy:** You may push small fixes. You may approve a PR when all gates pass. +Report contributor and approver overlap as an advisory. It does not change merge readiness or require another reviewer. +Never merge. Ask the user about merge, product-scope, and architecture decisions. Also ask when contributor intent is unclear. ## References @@ -23,28 +28,33 @@ node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/version-progress.ts ``` -The first script determines the target version. The second shows shipped vs open. +The first script selects the target version. The second lists shipped and open items. ## Step 2: Pick One Action -From the open version-targeted items, pick the highest-value one: +Select the first applicable action for an open item: -1. **Ready-now PR** — green CI, no conflicts, no unresolved correctness or security issues after review, has tests → follow [MERGE-GATE.md](MERGE-GATE.md) -2. **Salvage-now PR** — close to ready, needs small fix → follow [SALVAGE-PR.md](SALVAGE-PR.md) -3. **Security item** — touches risky areas → follow [SECURITY-SWEEP.md](SECURITY-SWEEP.md) -4. **Test-gap item** — risky code with weak tests → follow [TEST-GAPS.md](TEST-GAPS.md) -5. **Hotspot cooling** — repeated conflicts → follow [HOTSPOTS.md](HOTSPOTS.md) -6. **Sequencing needed** — too large for one pass → follow [SEQUENCE-WORK.md](SEQUENCE-WORK.md) +1. **PR ready for approval** — CI passes, no conflicts remain, and tests cover the change. + Confirm that there is no unresolved correctness or security issue. Follow [MERGE-GATE.md](MERGE-GATE.md). +2. **PR that needs a small fix** — Follow [SALVAGE-PR.md](SALVAGE-PR.md). +3. **Security item** — The item touches a risky area. Follow [SECURITY-SWEEP.md](SECURITY-SWEEP.md). +4. **Test gap** — Risky code does not have sufficient tests. Follow [TEST-GAPS.md](TEST-GAPS.md). +5. **Repeated conflicts** — Follow [HOTSPOTS.md](HOTSPOTS.md). +6. **Work that needs sequencing** — The work is too large for one pass. Follow [SEQUENCE-WORK.md](SEQUENCE-WORK.md). -If all version-targeted items are blocked, fall back to the general backlog. Productive work on non-labeled items is better than waiting. +If all items for the release version are blocked, select an item from the backlog. -Prefer finishing one almost-ready contribution over starting a new refactor. +Prefer to complete a contribution that needs little work before you start a refactor. ## Step 3: Execute -Follow the chosen workflow document. A good pass ends with one of: +Follow the selected workflow. Complete one outcome in each pass: -- a PR approved, a fix pushed, a test gap closed, a hotspot mitigated, or a blocker surfaced. +- Approve a PR. +- Push a fix. +- Add a missing test. +- Reduce a source of merge conflicts. +- Report a blocker. ## Step 4: Report Progress @@ -54,7 +64,7 @@ Re-run the progress script and show the update: node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-day/scripts/version-progress.ts ``` -If all version-targeted items are done, suggest running `/nemoclaw-maintainer-evening` early. +If all items for the release version are done, suggest `/nemoclaw-maintainer-evening`. Update `.nemoclaw-maintainer/state.json` via the state script: @@ -64,18 +74,20 @@ node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer ## Commit Hygiene -Only commit skill files when the task intentionally changes agent guidance. -Keep unrelated `.agents/skills/` changes out of ordinary code or docs PRs. +Commit skill files only when the task changes agent guidance. +Do not include unrelated skill changes in code or documentation PRs. ## Stop and Ask When -- A PR would create a new supported product surface without an accepted product decision -- Broad refactor or architecture decision needed -- Contributor intent unclear and diff would change semantics -- Multiple subsystems must change for CI -- Sensitive security boundaries with unclear risk -- Next step is opening a new PR or merging +- A PR creates a supported product surface without an accepted product decision. +- The work requires a refactor or architecture decision. +- Contributor intent is unclear and the change can alter behavior. +- CI requires changes to multiple subsystems. +- A security boundary has an unclear risk. +- The next step is to open a PR or merge. ## /loop Integration -Designed for `/loop 10m /nemoclaw-maintainer-day`. Each pass should produce compact output: what was done, what changed, what needs the user. Check `state.json` history to avoid re-explaining prior context on repeat runs. +Use this skill with `/loop 10m /nemoclaw-maintainer-day`. +Keep each pass report short. State what you did, what changed, and what needs a user decision. +Read `state.json` to avoid repeated context. diff --git a/.agents/skills/nemoclaw-maintainer-day/STATE-SCHEMA.md b/.agents/skills/nemoclaw-maintainer-day/STATE-SCHEMA.md index 32d841140af..be45c0bcc48 100644 --- a/.agents/skills/nemoclaw-maintainer-day/STATE-SCHEMA.md +++ b/.agents/skills/nemoclaw-maintainer-day/STATE-SCHEMA.md @@ -1,6 +1,10 @@ + + + # State File Schema -Path: `.nemoclaw-maintainer/state.json` (excluded via `.git/info/exclude`). +Store state in `.nemoclaw-maintainer/state.json`. +Git excludes this file through `.git/info/exclude`. ```json { @@ -49,8 +53,11 @@ Path: `.nemoclaw-maintainer/state.json` (excluded via `.git/info/exclude`). ## Field Notes -- `gates.autoMerge` is always `false`. The loop may approve but never merges. -- `gates.autoPushSmallFixes` allows pushing narrow fixes to contributor branches. -- `excluded.prs` / `excluded.issues`: keys are numbers (as strings), values are `{ "reason": "...", "excludedAt": "ISO" }`. Items here are permanently skipped by triage until the user removes them. -- `history` entries: `{ "at": "ISO", "item": "PR#1234", "action": "approved|salvaged|blocked|sequenced", "note": "one line" }`. Keep under 50 entries; trim oldest. +- Set `gates.autoMerge` to `false`. The loop can approve a PR but must not merge it. +- `gates.autoPushSmallFixes` permits small fixes on contributor branches. +- Use number strings as keys in `excluded.prs` and `excluded.issues`. + Use `{ "reason": "...", "excludedAt": "ISO" }` as each value. + Triage skips these items until the user removes them. +- Use this format for `history` entries: `{ "at": "ISO", "item": "PR#1234", "action": "approved|salvaged|blocked|sequenced", "note": "one line" }`. + Keep at most 50 entries. Remove the oldest entries first. - `queue.items` and `queue.nearMisses` store the latest triage output for comparison across runs. diff --git a/.agents/skills/nemoclaw-maintainer-day/TEST-GAPS.md b/.agents/skills/nemoclaw-maintainer-day/TEST-GAPS.md index 595b6160bc4..7af8ccfe849 100644 --- a/.agents/skills/nemoclaw-maintainer-day/TEST-GAPS.md +++ b/.agents/skills/nemoclaw-maintainer-day/TEST-GAPS.md @@ -1,12 +1,16 @@ + + + # Test Gaps Workflow -Close the highest-value test gaps without turning the task into a rewrite. +Add tests for the most important uncovered behavior. Do not rewrite the code. For risky code areas, see [RISKY-AREAS.md](RISKY-AREAS.md). ## Step 1: Collect File Set -Sources: files changed in target PR, recent failing CI, `main` churn in hotspot area, hotspot list in state file. If unknown, derive from highest-ranked actionable PRs. +Use changed PR files, CI failures, recent `main` changes, and the state-file hotspot list. +If no file set is available, use the highest-ranked PR that can progress. ## Step 2: Map to Existing Tests @@ -16,32 +20,38 @@ Repo conventions: - Plugin tests: co-located as `*.test.ts` - Shell logic: may need extraction into testable helper first -For each risky file: is there a test covering the changed behavior? Is it too indirect or flaky? Can a small extraction improve testability? +For each risky file, find a test for the changed behavior. +Check whether the test is indirect or unstable. Identify a small extraction when it improves testing. -## Step 3: Choose Highest-Value Tests +## Step 3: Select tests -Prefer tests that catch regressions the maintainer loop actually sees: +Prioritize regressions found during maintenance: - invalid/boundary env values, shell quoting, retry/timeout behavior - missing/malformed config, denied network/policy paths - duplicate workflow/hook behavior, version/tag/DCO edge cases - unauthorized or unsafe inputs -Include at least one negative-path test for risky code. +For risky code, include a test that rejects invalid or unsafe input. + +## Step 4: Extract code when needed -## Step 4: Extract Narrow Seams If Needed +Make only the extraction needed for tests: -Smallest extraction that improves testability: move parsing into a pure helper, separate construction from execution, factor hook logic into a reusable function, replace bags of primitives with typed helpers. +- Move parsing into a pure helper. +- Separate construction from execution. +- Move hook logic into a function. +- Replace related primitive values with a typed object. -Do not broad-refactor under the label of "adding tests." +Do not use a test change to add a refactor that is not required. ## Step 5: Add Tests -- CLI tests → `test/` -- Plugin tests → `nemoclaw/src/` -- TypeScript helpers → TypeScript tests -- Mock external systems; no real API calls in unit tests -- Security paths: prove the unsafe action is denied, not just that happy path works +- Put CLI tests in `test/`. +- Put plugin tests in `nemoclaw/src/`. +- Use TypeScript tests for TypeScript helpers. +- Mock calls to external systems. Unit tests must not call external APIs. +- For security paths, prove that the code denies the unsafe action. ## Step 6: Validate @@ -52,14 +62,19 @@ npm run typecheck:cli npm run check ``` -Narrowest command set that gives confidence. +Run only the commands needed to validate the change. + +## Step 7: Report remaining gaps -## Step 7: Report Gaps Honestly +Report each remaining risk. Possible causes include: -If untested risk remains, say so: no clean seam without larger refactor, too much hidden shell state, missing fixture strategy, flaky infra dependency. +- The code needs a larger refactor before it can be tested. +- Shell state prevents isolation. +- No fixture strategy exists. +- An infrastructure dependency is unstable. ## Notes -- Tests for risky code are merge readiness, not polish. -- One precise regression test beats many vague integration checks. -- If credible fix needs bigger redesign, follow [SEQUENCE-WORK.md](SEQUENCE-WORK.md). +- Tests for risky code are required for merge. +- Prefer one regression test with a defined failure over many general integration tests. +- If a credible fix needs a redesign, follow [SEQUENCE-WORK.md](SEQUENCE-WORK.md). diff --git a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md index b04c8dedb18..9d08d6fe382 100644 --- a/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-find-review-pr/SKILL.md @@ -1,12 +1,16 @@ --- name: nemoclaw-maintainer-find-review-pr -description: Finds open security-labeled GitHub PRs with Urgent or High Project Priority, links each to its issue, detects duplicates, and presents a table of review candidates. Use when looking for the next PR to review. Trigger keywords - find pr, find review, next pr, pr to review, duplicate pr, security pr. +description: Find open PRs with the security label and Urgent or High Project Priority. Link each PR to its issue. Identify competing or superseded PRs and report review candidates. Use when looking for the next PR to review. Trigger keywords - find pr, find review, next pr, pr to review, duplicate pr, security pr. user_invocable: true --- + + + # Find PR to Review -Search for open PRs labeled `security` whose Project Priority is `Urgent` or `High`, associate each with its linked issue, detect duplicates (multiple PRs targeting the same issue), and present a clean summary so you can decide what to review or close. +Find open PRs with the `security` label and Project Priority `Urgent` or `High`. +Link each PR to its issue. Identify competing or superseded PRs. Report the results for the maintainer. ## Prerequisites @@ -29,16 +33,18 @@ gh project item-list 199 --owner NVIDIA --limit 1000 --format json \ priority: .priority, status: .status}]' ``` -Discard entries whose PR is no longer open. Fetch PR body, author, branch, labels, and creation time for the remaining numbers with `gh pr view`. If the result is empty, report that there are no matching PRs and stop. +Remove entries for closed PRs. +For each remaining PR, fetch the body, author, branch, labels, and creation time with `gh pr view`. +If no PRs remain, report that result and stop. ## Step 2: Extract linked issues -For each PR, parse the body for linked issue references. Look for these patterns (case-insensitive): +For each PR, search its body for issue references. Match these patterns without case sensitivity: - `Fixes #NNN`, `Closes #NNN`, `Resolves #NNN` - `Related Issue` / `Linked Issue` section containing `#NNN` -- Issue number in the PR title, e.g. `(#NNN)` suffix -- Branch name containing an issue number, e.g. `fix/something-NNN` +- Issue number in the PR title, such as a `(#NNN)` suffix +- Branch name containing an issue number, such as `fix/something-NNN` Build a mapping: `PR# → [issue numbers]`. @@ -46,9 +52,10 @@ If a PR has no detectable linked issue, mark it as `(no linked issue)`. ## Step 3: Detect duplicates -Group PRs by linked issue number. Any issue with **two or more** open PRs is a duplicate group. +Group PRs by linked issue number. +If two or more open PRs link to one issue, put them in one competing-PR group. -For each duplicate group, fetch a brief summary of each competing PR to help the user decide which to keep: +Fetch these fields for each competing PR: ```bash gh pr view --json number,title,author,createdAt,additions,deletions,reviewDecision,statusCheckRollup --jq '{number,title,author: .author.login,created: .createdAt,additions,deletions,review: .reviewDecision,checks: [.statusCheckRollup[]?.conclusion] | unique}' @@ -56,11 +63,11 @@ gh pr view --json number,title,author,createdAt,additions,deletions,rev ## Step 4: Check for superseded PRs -Also flag PRs whose body contains phrases like: +Flag a PR when its body contains one of these phrases: - `follow-up to #NNN` / `supersedes #NNN` / `replaces #NNN` / `folds in #NNN` -where `#NNN` is another **open** PR number in the candidate list. These indicate one PR has absorbed another. +The phrase must name another open candidate PR. It indicates that one PR can include the other. ## Step 5: Present results @@ -85,12 +92,12 @@ For superseded PRs: ### Superseded PRs - #1416 supersedes/folds in #1392 (shell-quote sandboxName) - → Consider closing #1392 if #1416 covers its scope. + Consider closing #1392 if #1416 contains its full scope. ``` ### Clean candidates -Present non-duplicate PRs in a table: +Present PRs without competing PRs in a table: ```markdown ### Review candidates (no duplicates) @@ -103,15 +110,15 @@ Present non-duplicate PRs in a table: ### Summary line -End with a one-line recommendation of which PR to review first, preferring: +Recommend one PR to review first. Apply these priorities in order: 1. Project Priority (`Urgent` before `High`) -2. Older PRs (waiting longest) +2. Oldest PR 3. PRs with passing checks 4. PRs with smaller diff size (easier to review) ## Notes -- Do NOT automatically close any PRs. Only present findings and recommendations. -- If the user specifies additional filters (e.g., a specific scope label like `OpenShell`), apply them. +- Never close a PR. Report findings and recommendations only. +- Apply filters that the user gives, such as a scope label. - If the user asks for a different priority, filter the Project Priority field. Never use or create a priority label. diff --git a/.agents/skills/nemoclaw-maintainer-policies/SKILL.md b/.agents/skills/nemoclaw-maintainer-policies/SKILL.md index ece39861638..f302fab8f75 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-policies/SKILL.md @@ -1,6 +1,6 @@ --- name: nemoclaw-maintainer-policies -description: Read-only maintainer policy reference for NemoClaw agents, engineers, and maintainers. Use when answering NemoClaw project-management workflow questions, including GitHub Issue Type, labels, Project fields, daily release labels, triage, duplicates, blocked items, and maintainer workflow decisions. Trigger keywords - maintainer policy, workflow policy, project workflow, issue type, labels, label taxonomy, needs labels, project status, blocked issue, duplicate issue, daily release label, release train, triage policy. +description: Provide read-only NemoClaw maintainer policy. Use for questions about Issue Type, labels, Project fields, release labels, triage, duplicates, blocked items, and maintainer decisions. Trigger keywords - maintainer policy, workflow policy, project workflow, issue type, labels, label taxonomy, needs labels, project status, blocked issue, duplicate issue, daily release label, release train, triage policy. user_invocable: true --- @@ -9,25 +9,30 @@ user_invocable: true # NemoClaw Maintainer Policies -This is a policy-only skill package. `SKILL.md` is only the manifest and index. +This package contains policy references. This file is its manifest and index. ## References -- **Broad overview:** When a new engineer asks how NemoClaw uses GitHub or how the maintainer workflow works, load [references/workflow-policy.md](references/workflow-policy.md), then [references/project-workflow.md](references/project-workflow.md), then [references/daily-flow.md](references/daily-flow.md). Use [references/release-train.md](references/release-train.md) only when release labels, cutoff, carry-forward, release history, or label retirement are involved. -- **Agent implementation:** When building or updating an agent or app that applies or recommends workflow metadata, load [references/workflow-policy.md](references/workflow-policy.md), [references/triage-instructions.md](references/triage-instructions.md), [references/label-taxonomy.json](references/label-taxonomy.json), and [references/examples.md](references/examples.md), in that order. -- **Load [references/workflow-policy.md](references/workflow-policy.md)** when answering source-of-truth, authorization, Issue Type, label boundary, or agent-owned label questions. -- **Load [references/triage-instructions.md](references/triage-instructions.md)** when answering how to evaluate issues or PRs, when to ask for information, how to use `needs:*`, how to set confidence, or what suggestion payload shape to emit. -- **Load [references/label-taxonomy.md](references/label-taxonomy.md)** when answering human-facing label meaning, label selection, label compatibility, unknown-label, `agt: *`, or release-label taxonomy questions. -- **Load [references/label-taxonomy.json](references/label-taxonomy.json)** when validating machine-readable Issue Type, Project field, label, signal, compatibility, or write-policy values. -- **Load [references/project-workflow.md](references/project-workflow.md)** when answering Project Status, Project fields, duplicate, blocked, backlog, review, QA, issue-template, or lifecycle workflow questions. -- **Load [references/daily-flow.md](references/daily-flow.md)** when answering daily slate, priority lane, standup, assignment, execution, QA handoff, or daily operating-loop questions. -- **Load [references/release-train.md](references/release-train.md)** when answering daily version-label, release inclusion, carry-forward, cutoff, release history, or label-retirement questions. -- **Load [references/examples.md](references/examples.md)** when examples or anti-examples are needed for triage, PR review, daily release activation, competing PRs, stale/rebase cases, or agent-owned labels. +- **Workflow overview:** Read [workflow-policy.md](references/workflow-policy.md), [project-workflow.md](references/project-workflow.md), and [daily-flow.md](references/daily-flow.md) in that order. + Read [release-train.md](references/release-train.md) for questions about release labels or history. +- **Agent implementation:** Read [workflow-policy.md](references/workflow-policy.md), [triage-instructions.md](references/triage-instructions.md), [label-taxonomy.json](references/label-taxonomy.json), and [examples.md](references/examples.md) in that order. +- [references/workflow-policy.md](references/workflow-policy.md) — Source of truth, authorization, Issue Type, label boundaries, and agent-owned labels. +- [references/triage-instructions.md](references/triage-instructions.md) — Issue and PR evaluation, questions, `needs:*`, confidence, and suggestion payloads. +- [references/label-taxonomy.md](references/label-taxonomy.md) — Label meaning, selection, compatibility, unknown labels, `agt: *`, and release labels. +- [references/label-taxonomy.json](references/label-taxonomy.json) — Allowed machine values and write policy. +- [references/project-workflow.md](references/project-workflow.md) — Project Status, lifecycle, duplicates, blocked work, review, and QA. +- [references/daily-flow.md](references/daily-flow.md) — Daily priorities, standup, assignment, execution, and QA handoff. +- [references/release-train.md](references/release-train.md) — Release inclusion, cutoff, carry-forward, history, and label retirement. +- [references/examples.md](references/examples.md) — Examples for triage, review, release activation, competing PRs, and stale PRs. ## Answering Workflow Questions -Before answering or applying policy, read the most relevant reference file. Answer in plain maintainer-facing language and distinguish native GitHub Issue Type, PR type labels, `needs:*` action labels, Project fields, GitHub close reasons, and daily version labels. Do not invent labels, statuses, fields, release labels, or workflow states. +Read the related reference before you answer or apply policy. +Use plain language for maintainers. Distinguish Issue Type, PR type labels, `needs:*` labels, Project fields, close reasons, and release labels. +Do not invent labels, statuses, fields, release labels, or workflow states. +Apply [NemoClaw Technical English](../../../CONTRIBUTING.md#nemoclaw-technical-english) to changed workflow guidance and maintainer-facing text. -For agent implementation questions, treat `triage-instructions.md` as the recommendation payload contract and `label-taxonomy.json` as the allowed-value and validation source. +For agent implementation, use `triage-instructions.md` as the payload contract. +Use `label-taxonomy.json` to validate allowed values. Workflow answers must remain reference-backed. Do not encode alternate policy in this manifest. diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/label-taxonomy.md b/.agents/skills/nemoclaw-maintainer-policies/references/label-taxonomy.md index c19603583fa..ae2d8962479 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/label-taxonomy.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/label-taxonomy.md @@ -5,7 +5,7 @@ Status: canonical maintainer policy. -Labels should exist only when maintainers, agents, dashboards, or reviewers act differently because the label is present. +Use a label only when it changes an action, route, or report. ## Native Fields Before Labels @@ -60,18 +60,31 @@ Reporting labels apply when maintainers need to identify items that feed a recur |---|---|---|---|---| | `PRR` | Issue, PR | Reserved Product Readiness Review label for reports or follow-up used to assess product readiness and user experience. | Maintainer-applied or dedicated PRR workflow-applied Product Readiness Review report, PRR follow-up, readiness assessment, or user experience assessment tied to a PRR. | Generic readiness concern, ordinary UX bug, release validation, QA issue, daily release activity, or normal triage output. | -Do not recommend `PRR` during normal triage. It is reserved for maintainers or dedicated PRR workflows, and must not be used as a lifecycle status, release-readiness claim, generic UX label, or substitute for Project Status. +Do not recommend `PRR` during triage. +Maintainers and PRR workflows reserve it for Product Readiness Reviews. +Do not use it for lifecycle, release readiness, UX routing, or Project Status. ### Routing Areas Area labels apply to issues and PRs when the affected surface is clear. -Use area labels for the affected product or code surface, not for every concept mentioned in the report. For overlapping areas, prefer the label that best describes the owner of the next action: - -- `area: install` for prerequisites or setup mechanics; `area: onboarding` for first-run flow and onboarding state; `area: packaging` for shipped artifacts, images, registries, or distribution. -- `area: inference` for model execution or output behavior; `area: providers` for provider integration, configuration, or selection work; `area: routing` for dispatch, fallback, or model-selection logic; `area: local-models` for local runtime, download, launch, or connectivity. -- `area: integrations` for external app or bridge behavior; `area: messaging` when message delivery or channel lifecycle is the affected subsystem; add the specific `integration:*` label when one listed integration is named or clearly implicated. Do not use only `area: integrations` when the affected integration is one of the canonical `integration:*` values. -- `area: ci` for workflow, check, release automation, nightly-runner, or test infrastructure failures. Do not add `area: ci` merely because an e2e failure was observed in CI; use both `area: ci` and `area: e2e` only when the CI workflow, runner, scheduling, logs, or test infrastructure is part of the affected surface. +Use area labels for the affected product or code surface. +Do not label each concept that the report mentions. +For overlapping areas, prefer the label that routes the next action: + +- Use `area: install` for prerequisites and setup mechanics. + Use `area: onboarding` for the first-run flow and its state. + Use `area: packaging` for artifacts, images, registries, and distribution. +- Use `area: inference` for model execution and output. + Use `area: providers` for provider integration, configuration, and selection. + Use `area: routing` for dispatch, fallback, and model selection. + Use `area: local-models` for local runtime, download, launch, and connectivity. +- Use `area: integrations` for external app and bridge behavior. + Use `area: messaging` for message delivery and channel lifecycle. + Add the matching `integration:*` label for a listed integration. +- Use `area: ci` for failures in workflows, checks, release automation, runners, or test infrastructure. + Do not add it only because CI found an E2E failure. + Add both `area: ci` and `area: e2e` when CI or test infrastructure is part of the failure. | Label | Description | |---|---| @@ -101,15 +114,16 @@ Use area labels for the affected product or code surface, not for every concept ### Platform -Platform labels apply when the issue or PR is specific to a platform or is more likely relevant to that platform, not merely because the author happened to test there. This is one of the hardest label families to infer. - -Positive signals include platform-specific errors, platform-specific code paths, platform-specific install/runtime behavior, or repeated evidence from the same platform. Weak signals include reproduction setup only, "all platforms" reports, or logs that mention a platform without showing platform-specific behavior. - -When evidence is ambiguous, use the platform label only when the platform seems routing-relevant or likely causal. Otherwise, leave it off and explain what evidence would make it platform-specific. +Do not infer a platform from the test environment alone. +Add a platform label when the error, code path, install behavior, runtime behavior, or repeated reports identify that platform. +When evidence is not conclusive, add the label only if the platform is likely causal or changes routing. +Otherwise, omit it. +Prefer the narrower platform label unless both labels route work to different owners. +Do not add a platform label only because the environment template names Docker, a CPU, or an operating system. -When a more specific platform label applies, prefer it over a broader one unless both are independently routing-relevant. Do not add `platform: container`, `platform: arm64`, or an OS label just because the environment template mentions Docker, CPU architecture, or OS. - -Use the specific platform labels when the platform appears in the reported failure signature, title, or affected install path. For example, `Windows ARM`, `Windows ARM64`, or `aarch64` failure text supports `platform: arm64`; `WSL`, `WSL2`, or "Windows Subsystem for Linux" supports `platform: wsl`. +Use a platform label when the platform appears in the error, title, or affected install path. +`Windows ARM`, `Windows ARM64`, and `aarch64` support `platform: arm64`. +`WSL`, `WSL2`, and `Windows Subsystem for Linux` support `platform: wsl`. | Label | Description | |---|---| @@ -134,7 +148,10 @@ Do not create or apply `platform: all`. Provider labels apply when the issue or PR is specific to a recurring inference provider. -Use `area: providers` for provider integration work, and add `provider:*` when a listed provider is specifically involved. When a provider exposes an OpenAI-compatible API but has its own provider label, use the more specific provider label instead of `provider: openai`. For unknown or proposed providers, use `area: providers` and name the provider in the rationale. +Add `area: providers` for provider work. +Also add the matching `provider:*` label for a listed provider. +Prefer that label over `provider: openai` for an OpenAI-compatible provider with its own label. +For an unlisted provider, add `area: providers` and name the provider in the rationale. | Label | Description | |---|---| @@ -148,9 +165,11 @@ Use `area: providers` for provider integration work, and add `provider:*` when a Integration labels apply when a recurring external app, channel, tool, or agent integration is specifically involved. -Use `area: integrations` for integration subsystem work, and add `integration:*` when a listed integration is named or clearly implicated. Use `area: messaging` when delivery, channel lifecycle, manifests, or bridge messages are the affected subsystem; combine it with `integration:*` when the messaging issue is specific to a listed integration. - -Specific integration labels are routing labels. If the title, body, linked issue, test name, file path, or PR prefix names `Hermes`, `OpenClaw`, `LangChain Deep Code`, `Deep Code`, `Discord`, `Slack`, `Telegram`, `WeChat`, `WhatsApp`, or `Brave` as the affected subject, include the corresponding `integration:*` label. Do not replace the specific label with only `area: integrations`. +Add `area: integrations` for integration subsystem work. +Add `area: messaging` for delivery, channel lifecycle, manifests, or bridge messages. +Add the matching `integration:*` label when a listed integration is the affected subject. +Do not replace that label with only `area: integrations`. +Use the title, body, linked issue, tests, file paths, and PR prefix as evidence. | Label | Description | |---|---| @@ -166,7 +185,9 @@ Specific integration labels are routing labels. If the title, body, linked issue ### Needs -`needs:*` labels are blocking action queues. Remove them when the action is complete. `Needs Review` is a Project Status value, not a label. Normal initial triage should not add `needs: triage`; that label is an inbox/placeholder signal for unprocessed items. +`needs:*` labels identify blocked actions. Remove them when the action is complete. +`Needs Review` is a Project Status value, not a label. +Do not add `needs: triage` during triage. It identifies items that have not been processed. | Label | Applies To | Description | |---|---|---| @@ -186,19 +207,28 @@ Do not combine: | Label | Applies To | Description | |---|---|---| -| `good first issue` | Issue | Small, clear, safe task for new contributors with tiny blast radius and no permission, secret, security, release, or policy risk. | +| `good first issue` | Issue | Small, clear task for new contributors. Do not use it for permission, secret, security, release, or policy work. | | `help wanted` | Issue | Accepted work where maintainers welcome external contribution. | ### Release Train -Daily `v0.0.x` labels activate open PRs for daily release work. After a PR merges to `main`, authorized post-merge automation adds the next patch label only while the merge is ahead of the latest release tag. After that tag and workflow-managed `latest` are verified, release housekeeping moves open stragglers and deletes the released label. Tags and commit ancestry own durable release membership. Issues may use a daily label as a tracking or attention signal, but issue labels do not determine release inclusion. Released labels must never be renamed or reused. See `release-train.md`. +Daily `v0.0.x` labels activate open PRs for release work. +After a PR merges, authorized automation adds the next patch label while the merge is ahead of the release tag. +After verification of the tag and `latest`, release housekeeping moves open items and deletes the released label. +Tags and commit ancestry record release membership. +An issue label can track attention or PR work. It does not include the issue in a release. +Do not rename or reuse a released label. See `release-train.md`. ### Agent-Owned -`agt: *` labels are agent-owned coordination labels. Agents may create, apply, remove, and delete them inside an authorized agent-owned workflow. They must not encode product type, priority, project status, sprint, or release version. +`agt: *` labels are agent-owned coordination labels. +Agents may create, apply, remove, and delete them in an authorized agent workflow. +They must not encode product type, priority, Project Status, sprint, or release version. ## Unknown Labels -Labels not listed in this taxonomy are not canonical and must not be created, applied, or recreated by agents or maintainers. The only exception is the agent-owned `agt: *` namespace described above. +Do not create, apply, or recreate a label that is absent from this taxonomy. +The `agt: *` namespace is the exception. -If an old or unknown label is found on an existing item, report it in an audit or cleanup dry run. Do not use it as permission to apply the same label elsewhere. +Report an old or unknown label in an audit or cleanup dry run. +Do not apply it to another item. diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/project-workflow.md b/.agents/skills/nemoclaw-maintainer-policies/references/project-workflow.md index d29be51f9bc..9216421b9ef 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/project-workflow.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/project-workflow.md @@ -59,18 +59,24 @@ For each recommended daily item, agents should report: - Whether a PR exists, is needed, or is already merged. - Whether a daily `v0.0.x` label is present on the PR, issue, or both. -Standup converts the recommendation into assignments. Every recommended item should leave standup either assigned with a next action and exit signal, or explicitly deferred with rationale. +After standup, assign each item with a next action and exit condition. +Otherwise, record its deferral and reason. ## Release Labels In Project Context Daily `v0.0.x` labels have different meanings by item kind and PR state: - On open PRs, the label activates the PR for daily release work. Open labeled PRs that merge by cutoff are candidates for that release. -- After a PR merges to `main`, authorized automation adds the next patch label only when the merge is ahead of the latest release tag. This keeps the untagged release queue complete before cutoff. +- After a PR merges to `main`, authorized automation checks whether the merge is ahead of the release tag. + If it is, automation adds the next patch label. This keeps the release queue complete before cutoff. - After the release tag and workflow-managed `latest` are verified, the released label is deleted. The tag comparison range owns durable release membership. - On issues, the label is an attention, regression-tracking, or "needs PR for this daily release" signal. It does not include the issue in the release by itself. -Open labeled PRs and issues that miss a tagged release are automatically moved to the next patch label during post-tag housekeeping. After no open item remains, housekeeping deletes the released label from the repository. Remove a version label without replacement only when an open item is deferred, superseded, closed, or no longer part of the daily release cycle. Never rename or reuse a released label. +After a release, post-tag housekeeping moves open labeled PRs and issues to the next patch label. +It deletes the released label when no open item has it. +Remove a version label without replacement only for a deferred, superseded, or closed item. +You can also remove it when the item leaves the daily release cycle. +Do not rename or reuse a released label. ## Issue Templates @@ -101,10 +107,10 @@ Do not use labels for: ## Agent Writes -Agents should emit a dry-run plan containing labels, field changes, comments, and rationale unless they are already operating inside an explicit authorization context for the proposed write class. +Show a dry run unless the request authorizes the writes. When writes are authorized: -1. Apply field updates first. -2. Add canonical labels. -3. Post comments only when the authorization context covers the comment text or intent. +1. Update Project fields. +2. Add labels. +3. Post comments only when the authorization includes them. diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/release-train.md b/.agents/skills/nemoclaw-maintainer-policies/references/release-train.md index 20bfad0fa4d..c4ca47c378f 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/release-train.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/release-train.md @@ -15,7 +15,7 @@ Daily release labels coordinate release work. They do not classify issues and th - Issues may also carry daily version labels when they need a PR, fix, or regression follow-up for the daily tag. - Applying a daily version label is not a readiness claim. - Release includes PRs that both carry the daily version label and are merged by cutoff. -- Issue version labels are tracking signals only; an issue label does not include work in the release without a merged labeled PR. +- Issue version labels are tracking signals. An issue label does not include work in the release without a merged, labeled PR. - Open PRs and issues that miss a tagged release carry forward automatically by moving from the released version label to the next patch label. - After the semver tag and workflow-managed `latest` are verified, post-tag housekeeping moves open stragglers and deletes the released version label. Tags and commit ancestry are the only durable release-membership record. - Released version labels must be deleted, never renamed or reused for a later release. @@ -23,8 +23,9 @@ Daily release labels coordinate release work. They do not classify issues and th ## Release-Prep Docs Run `/nemoclaw-contributor-update-docs for vX.Y.Z` before generating the final release plan for `vX.Y.Z`. -The pre-tag release-note docs PR must create or update `docs/changelog/YYYY-MM-DD.mdx` with the exact `## vX.Y.Z` heading, parser-safe MDX SPDX comment, summary, and detailed bullets. -This dated file is the canonical release history shared by all documentation variants; ordinary doc pages and the post-tag Announcement do not replace it. +The pre-tag release-note docs PR must create or update `docs/changelog/YYYY-MM-DD.mdx`. +Use the required `## vX.Y.Z` heading, parser-safe MDX SPDX comment, summary, and detailed bullets. +This dated file is the release history for all documentation variants. Ordinary documentation pages and the post-tag Announcement do not replace it. Release-prep docs, including that entry, must be merged or explicitly waived before `release:plan` captures the release commit. If any merge lands after `release:plan`, generate a fresh plan before cutting the tag. @@ -69,7 +70,7 @@ Maintainers may: - Add the current version label when they want the PR visible in the current day queue. - Remove a version label without replacement when an item is deferred, superseded, closed, or no longer part of the daily cycle. -- Rerun post-tag housekeeping after a partial failure; already-moved items no longer match the released source label, so the operation is safely resumable. +- Rerun post-tag housekeeping after a partial failure. Moved items no longer have the released label, so the operation can resume safely. ## Label Retirement diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/triage-instructions.md b/.agents/skills/nemoclaw-maintainer-policies/references/triage-instructions.md index 6a5a0f53851..78a1abd7d8d 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/triage-instructions.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/triage-instructions.md @@ -3,26 +3,31 @@ # NemoClaw Triage Instructions -These instructions are for agents and skills that evaluate NemoClaw issues and PRs before recommending Type, labels, project fields, comments, or follow-up questions. +Use these instructions to evaluate NemoClaw issues and PRs. +They apply to Issue Type, labels, Project fields, comments, and questions. ## Core Rules - Read `label-taxonomy.md` and `label-taxonomy.json` before suggesting labels. -- Triage from evidence in the item title, body, linked issue, files changed, CI state, and maintainer comments. +- Use evidence from the title, body, linked issue, changed files, CI state, and maintainer comments. - Prefer no label over a guessed label. - Do not use labels for native issue type, priority, effort, lifecycle status, sprint, or resolution. Daily version labels on issues are tracking and coordination signals only. -- Present dry-run recommendations first unless the agent is already operating inside an explicit authorization context for the proposed write class. -- Keep changes minimal: only add labels or fields that change routing, actionability, or reporting. +- Show a dry run before a write unless the request authorizes that write. +- Add only labels or fields that change routing, action, or reporting. - Project Status is a Project field, not a label. Valid values include `No Status`, `Backlog`, `In Progress`, `Blocked`, `Needs Review`, `NV QA`, `Done`, `Won't Fix`, and `Duplicate`. -- Set `human_review_required: true` when the proposed write is outside the current authorization context, has elevated risk, or needs maintainer judgment before execution. -- Normal initial triage should not add inbox or placeholder labels such as `needs: triage`. Use `questions_for_author` without a `needs:*` label when a question is useful but the item is still actionable. -- Never recommend `PRR` during triage. `PRR` is a reserved Product Readiness Review label that maintainers or dedicated PRR workflows apply outside normal triage. +- Set `human_review_required: true` when the write lacks authorization, adds risk, or needs maintainer judgment. +- Do not add `needs: triage` during triage. + Use `questions_for_author` without a `needs:*` label when the item can still progress. +- Do not recommend `PRR` during triage. Maintainers and PRR workflows reserve this label for Product Readiness Reviews. ## Issue Flow 1. Classify the issue using native GitHub Issue Type: `Bug`, `Enhancement`, `Task`, `Documentation`, `Epic`, or `Initiative`. 2. Add area labels only when the affected surface is clear. -3. Add platform, provider, or integration labels only with explicit evidence. When a listed integration is named as the affected subject, include the matching `integration:*` label rather than only the broad `area: integrations` label. Use `integration: dcode` for LangChain Deep Code, Deep Code, `langchain-deepagents-code`, or `dcode` evidence. +3. Require evidence for platform, provider, and integration labels. + If a listed integration is the affected subject, add its `integration:*` label. + Do not use only `area: integrations`. + Map LangChain Deep Code, Deep Code, `langchain-deepagents-code`, and `dcode` to `integration: dcode`. 4. Add `needs:*` only when an immediate blocking action queue is needed. Do not add `needs: triage` during normal triage. 5. Recommend Project Priority from impact evidence, not user urgency language. 6. Recommend Project Status separately from labels. @@ -32,33 +37,37 @@ These instructions are for agents and skills that evaluate NemoClaw issues and P ## PR Flow 1. Identify whether the PR is draft, conflicted, stale, blocked, or review-ready. -2. Apply one PR type label only when enough evidence exists: `bug-fix`, `feature`, `refactor`, or `chore`. Conventional commit prefixes are strong evidence: `fix` maps to `bug-fix`, `feat` maps to `feature`, `refactor` maps to `refactor`, and `chore`, docs-only, CI-only, skill-sync, dependency, packaging, or generated-policy maintenance maps to `chore`. +2. Apply one PR type label when evidence supports it: `bug-fix`, `feature`, `refactor`, or `chore`. + Treat commit prefixes as evidence, not proof. + Map `fix` to `bug-fix`, `feat` to `feature`, and `refactor` to `refactor`. + Map `chore`, docs-only, CI-only, skill, dependency, packaging, and generated-policy maintenance to `chore`. 3. Add `security` when the PR touches credentials, permissions, SSRF, sandbox escape risk, policy enforcement, or trusted installer paths. 4. Add area/platform/provider/integration labels based on files changed and PR intent when useful for review routing. 5. Recommend Project Status `Needs Review` for non-draft, conflict-free PRs that are awaiting maintainer review. 6. Add `needs: rebase` when conflicts or rebase state blocks review. -7. Add `needs: info` only when contributor action is required before review can proceed. If the title, body, linked issue, or files changed provide enough routing evidence, ask optional questions without adding `needs: info`. -8. Daily `v0.0.x` labels activate PRs for daily release work; adding one is not a readiness claim. +7. Add `needs: info` only when contributor action blocks review. + If the existing content supports routing, ask optional questions without this label. +8. Daily `v0.0.x` labels activate PRs for daily release work. A label does not state that the PR is ready. ## Minimal Labeling Use the smallest label set that makes the item actionable. -For issues, a high-quality dry run often includes: +For issues, include these fields when they apply: - Native Issue Type. - Zero to two area labels. - Optional platform/provider/integration labels when directly evidenced. -- Optional blocking `needs:*`; do not add `needs: triage` in normal triage output. +- Optional blocking `needs:*`. Do not add `needs: triage` in normal triage output. - Project Priority and Status recommendations. - Optional daily release label only when the issue needs daily tracking, regression attention, or "needs PR" coordination. -For PRs, a high-quality dry run often includes: +For PRs, include these fields when they apply: - One PR type label. - Area labels for review routing. - Optional `security`. -- Optional blocking `needs:*`; do not add `needs: triage` in normal triage output. +- Optional blocking `needs:*`. Do not add `needs: triage` in normal triage output. - Project Status recommendation. - Optional daily release label only when the maintainer workflow activates the PR. @@ -68,15 +77,15 @@ Use `confidence` in dry-run output: | Confidence | Meaning | Write Guidance | |---|---|---| -| `high` | 80%+ confidence; direct evidence supports the recommendation. | Eligible inside an authorization context. | -| `medium` | 70-79% confidence; evidence is plausible but incomplete. | Eligible inside an authorization context with rationale. | -| `low` | Below 70% confidence; evidence is weak or inferred. | Do not write; ask for info or leave unlabeled. | +| `high` | 80% or more confidence. Direct evidence supports the recommendation. | Eligible inside an authorization context. | +| `medium` | 70–79% confidence. Evidence is plausible but incomplete. | Eligible inside an authorization context with rationale. | +| `low` | Below 70% confidence. Evidence is weak or inferred. | Do not write. Ask for information or leave the item unlabeled. | Never apply a label from a low-confidence inference. ## When To Ask For Info -Use `needs: info` and ask targeted questions only when author action is required before work can proceed: +Add `needs: info` only when the author must give information before work can continue: - A bug report lacks the specific reproduction steps, expected behavior, actual behavior, version, environment, or logs needed to route or investigate it. - A platform-specific claim lacks platform details. @@ -84,12 +93,13 @@ Use `needs: info` and ask targeted questions only when author action is required - A PR does not explain intent, scope, or linked issue and the diff could be interpreted multiple ways. - A security report lacks enough detail to route safely. -Name the missing fields. Do not ask broad questions like "Can you provide more details?" when specific missing data is known. +Name the missing fields. Do not ask for "more details" when you know which details are missing. ## When To Use Needs Labels -- `needs: triage`: Existing inbox or placeholder label for unprocessed items. Normal triage agents should not newly add it once they are producing Type, label, and Project field recommendations. -- `needs: info`: Author action is required before work can proceed; optional clarifying questions alone are not enough. +- `needs: triage`: Inbox label for items that have not been processed. + Do not add it when you produce Type, label, and Project-field recommendations. +- `needs: info`: Author action is required before work can proceed. Optional questions are not enough. - `needs: design`: Product or architecture decision is required and implementation cannot proceed from the current report. - `needs: rebase`: PR cannot proceed because of conflicts or stale base. - `needs: unblock`: Blocked item needs a decision or dependency resolved. @@ -139,15 +149,18 @@ Use this JSON-compatible shape: } ``` -`labels_to_create` and `labels_to_delete` are only for authorized `agt: *` label operations unless a non-agent label operation is explicitly authorized by the current workflow. +Without authorization for label writes, keep `labels_to_add` and `labels_to_remove` as dry-run output and do not change labels. +An authorized agent-owned workflow may add or remove only `agt: *` labels. +Create or delete labels only when the workflow authorizes that operation, with the same `agt: *` limit for agent-owned workflows. +An accepted maintainer write set can authorize canonical non-agent label changes. ## Comment Guidance - Keep comments to one or two sentences. -- Explain the immediate action or missing information. +- State the required action or missing information. - Thank contributors and assume good intent. -- When useful, address the author by GitHub login and reference the specific behavior, PR, or report. -- Be friendly, specific, and direct; do not use generic filler, sarcasm, or frustration. +- Address the author by GitHub login when useful. Name the behavior, PR, or report. +- Use direct and specific language. Do not use filler, sarcasm, or frustration. - Link to existing docs or prior issues when they answer the question better than repeating guidance inline. - For `needs: info`, ask for the missing details. - For security, avoid exploit confirmation. @@ -190,7 +203,7 @@ Recommendation: - Do not add `bug` to a new issue. Set native Issue Type `Bug`. - Do not add `status: triage` or `needs: triage` from normal triage output. -- Do not add `priority: high`; recommend Project Priority instead. -- Do not treat an issue `v0.0.x` label as release inclusion; PR labels own daily release activation. -- Do not add `needs: review`; use Project Status `Needs Review` for review-ready PRs. -- Do not add `PRR`; it is reserved and must never be suggested by triage instructions. +- Do not add `priority: high`. Recommend Project Priority instead. +- Do not treat an issue `v0.0.x` label as release inclusion. PR labels control daily release activation. +- Do not add `needs: review`. Use Project Status `Needs Review` for review-ready PRs. +- Do not add `PRR`. This label is reserved, and triage must not suggest it. diff --git a/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md b/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md index ced41b372cb..4a0e4b6736e 100644 --- a/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md +++ b/.agents/skills/nemoclaw-maintainer-policies/references/workflow-policy.md @@ -28,21 +28,23 @@ An authorization context can be: - A release workflow running under a named release automation. - An authorized agent-owned workflow operating only in the `agt: *` label namespace. -Canonical labels, Issue Type, Project fields, public comments, release labels, closes, merges, and non-agent label deletion require an authorization context that explicitly allows that operation. Security-sensitive, destructive, release, merge, and public-comment writes require stricter authorization than ordinary triage labels. +Each write requires authorization for that operation. +This includes labels, Issue Type, Project fields, comments, release labels, closes, merges, and label deletion. +Security, destructive, release, merge, and comment writes require stricter authorization than triage labels. ## Contributor PR Eligibility -Contributor-owned PRs must satisfy the DCO declaration and GitHub commit verification before they are opened for review. -This is a contributor self-serve requirement, not a maintainer repair step. +Contributor-owned PRs must pass DCO and commit-verification checks before review. +The contributor must correct a failure. -- The PR description must include a valid `Signed-off-by:` declaration for the contributor. +- The PR description must include the contributor's `Signed-off-by:` declaration. - Every commit must appear as `Verified` in GitHub. - Contributor agents must check both requirements before running `gh pr create`. -- If the PR body would miss the DCO declaration or any commit is missing GitHub verification, the agent must stop and tell the contributor how to fix the issue instead of opening the PR. -- If force-push is not allowed and a published branch already contains an unverified commit, the contributor must open a fresh branch and fresh PR with a clean compliant commit history. +- If either check fails, the agent must stop and tell the contributor how to correct it. +- If force-push is not allowed, the contributor must create a branch and PR with verified commits. -Maintainers should reject PRs that contain unverified commits or lack the DCO declaration. -Do not merge, approve, or repair those PRs on behalf of the contributor. +Maintainers must reject a PR with an unverified commit or no DCO declaration. +Do not merge, approve, or repair it for the contributor. ## Issue Classification diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md index 961b3414653..8a433707174 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md @@ -1,21 +1,26 @@ --- name: nemoclaw-maintainer-pr-comparator -description: Compares competing PRs that target the same issue and recommends which one to merge. Runs gate, correctness, and quality checks; outputs a deterministic scorecard with reasoning trace. Use when an issue has two or more open PRs and a maintainer needs to decide which to merge. +description: Compare open PRs that address the same issue and recommend one to merge. Apply eligibility, correctness, quality, and tie-break checks. Report the score and evidence. Use when an issue has two or more open PRs. user_invocable: true --- + + + # PR Comparator -Picks the merge winner among competing PRs for a single issue. Tier 0 gates eliminate plumbing failures; Tiers 1-2 score correctness and quality; Tier 3 applies deterministic tiebreakers. Degraded mode handles the case where no PR passes gates. +Compare PRs for one issue. Tier 0 determines eligibility. Tiers 1 and 2 score correctness and quality. +Tier 3 resolves ties. If no PR passes Tier 0, rank eligible PRs for salvage. ## Prerequisites - `gh` CLI installed and authenticated -- A target repository with an issue that has 2+ open PRs +- A target repository with an issue that has two or more open PRs ## Repo policy -Defaults assume NemoClaw conventions (security CODEOWNERS, DCO, CodeRabbit, `docs/` directory). For other repos, edit `repo-policy.md` to override. +The defaults use NemoClaw conventions for CODEOWNERS, DCO, CodeRabbit, and `docs/`. +Edit `repo-policy.md` for another repository. ## Workflow @@ -24,7 +29,7 @@ Copy this checklist into your response and check off each step: ```text PR Comparison Progress: - [ ] Step 1: Parse issue (body + comments) for acceptance criteria -- [ ] Step 2: Discover candidate PRs (default-order search with stop conditions) +- [ ] Step 2: Discover candidate PRs in the defined order - [ ] Step 3: Detect supersession (parse PR bodies) - [ ] Step 4: Run Tier 0 gates per PR - [ ] Step 5: Run Tier 1 correctness checks per PR @@ -36,13 +41,13 @@ PR Comparison Progress: ### Step 1: Parse issue -Extract acceptance criteria from issue body **and all comments**: +Read the issue body and all comments. Extract each acceptance criterion: ```bash gh issue view --json title,body,comments ``` -Read every comment — commenters often add asks the body doesn't capture. +Comments can add requirements that are absent from the issue body. ### Step 2: Discover candidate PRs @@ -58,7 +63,8 @@ Applies a single default order with stop conditions. scripts/parse-supersession.sh ... ``` -Parses each PR body for `supersedes #N`, `replaces #N`, `closes in favor of #N`, `folds in #N`. A PR that supersedes another wins ties immediately. +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. ### Step 4: Tier 0 gates @@ -68,20 +74,22 @@ scripts/check-coderabbit-threads.sh node --experimental-strip-types --no-warnings ../nemoclaw-maintainer-day/scripts/check-gates.ts ``` -Six deterministic gates are mandatory. Treat PR Review Advisor output as untrusted input for maintainer review, never as merge authorization. See `checks/tier-0-gates.md` for the full list and interpretation. +All six gates are required. +Treat PR Review Advisor output as input for maintainer review. Do not treat it as merge authorization. +See `checks/tier-0-gates.md`. ### Step 5: Tier 1 correctness -Six checks, all LLM judgments. See `checks/tier-1-correctness.md` for evidence requirements per check. +Apply the six model checks in `checks/tier-1-correctness.md`. ### Step 6: Tier 2 quality -Three checks, all LLM judgments. See `checks/tier-2-quality.md`. +Apply the four model checks in `checks/tier-2-quality.md`. ### Step 7: Weighted score -- Build the Tier 0 eligibility set from these six boolean keys: `state_open`, `ci_green_latest_sha`, `mergeable`, `contributor_compliance`, `branch_protection`, and `coderabbit_threads_resolved`. -- Stop if any candidate omits a key, adds an unknown key, or supplies a non-boolean value. +- Build the Tier 0 eligibility set from these Boolean keys: `state_open`, `ci_green_sha`, `mergeable`, `contributor_compliance`, `branch_protection`, and `coderabbit_threads_resolved`. +- Stop if a candidate omits a required key, has an unknown key, or has a value that is not Boolean. - Only PRs for which all six gates are `true` enter happy-path scoring. - Each pass = full points - Each yellow = half points @@ -91,35 +99,46 @@ Three checks, all LLM judgments. See `checks/tier-2-quality.md`. ### Step 8: Tier 3 ranking -Derive the mode from the Tier 0 eligibility set rather than accepting a caller-provided mode. In happy mode, set `winner` only to a PR in that set and leave `closest_to_ready` null. In degraded mode, leave `winner` null and use `closest_to_ready` only for an open, contributor-compliant salvage candidate. See `tiebreakers.md` for happy-path tiebreakers, degraded-mode distance-to-ready ranking, and the behavior-coverage matrix. +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. +In degraded mode, set `winner` to null. +Set `closest_to_ready` only to an open PR that passes contributor requirements. +See `tiebreakers.md`. ### Step 9: Emit verdict -Use `templates/verdict.md`. Pass the generated spec through `scripts/render-verdict.py`; do not render or recommend a merge if it exits nonzero. The renderer independently validates the six gates, recomputes eligibility and mode, and rejects a `winner` outside the eligible set. Every judgment must carry evidence (file:line refs, diff snippets), reasoning chain, and the score it contributed. +Use `templates/verdict.md` and render the result with `scripts/render-verdict.py`. +Stop if the renderer exits with a nonzero status. Do not recommend a merge. +The renderer validates the gate schema, mode, winner eligibility, and salvage-candidate eligibility. +The reviewer remains responsible for the score, ranking, and evidence. +For each judgment, include evidence, the inference, and the score. ## Reference files -- `repo-policy.md` — configurable defaults per target repo -- `checks/tier-0-gates.md` — plumbing gates -- `checks/tier-1-correctness.md` — six correctness checks -- `checks/tier-2-quality.md` — three quality checks -- `tiebreakers.md` — Tier 3 ranking and degraded mode -- `templates/verdict.md` — output template -- `validation/backtest.md` — backtest the skill against historical cases +- [repo-policy.md](repo-policy.md) — Repository settings. +- [checks/tier-0-gates.md](checks/tier-0-gates.md) — Six eligibility gates. +- [checks/tier-1-correctness.md](checks/tier-1-correctness.md) — Six correctness checks. +- [checks/tier-2-quality.md](checks/tier-2-quality.md) — Four quality checks. +- [tiebreakers.md](tiebreakers.md) — Tier 3 ranking and degraded mode. +- [templates/verdict.md](templates/verdict.md) — Output template. +- [validation/backtest.md](validation/backtest.md) — Historical test cases for the skill. ## Scripts (execute, do not read) - `scripts/find-candidates.sh` — PR discovery - `scripts/collect-gates.sh` — Tier 0 gate evaluation -- `scripts/check-coderabbit-threads.sh` — GraphQL thread resolution +- `scripts/check-coderabbit-threads.sh` — GraphQL thread-resolution check - `scripts/parse-supersession.sh` — body parsing for supersession refs - `scripts/render-verdict.py` — verdict scorecard renderer -## What this skill does NOT do +## Limits + +Run `nemoclaw-maintainer-cross-issue-sweep` separately when you need related-issue evidence. -These require infrastructure beyond GitHub API + LLM and are deferred to v2 modules: +This skill does not: -- Running each PR's code against adversarial inputs (sandboxed execution) -- Cross-issue regression sweep (separate skill) -- Revert simulation against neighbor PRs -- Static analyzer integration (CodeQL, Semgrep) +- run PR code against adversarial inputs +- scan other issues for related behavior +- simulate reverts against related PRs +- run static analyzers such as CodeQL or Semgrep diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md index 8526e01acde..0b809d7d483 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-0-gates.md @@ -1,14 +1,16 @@ -# Tier 0 — Plumbing Gates +# Tier 0 — Eligibility Gates -Mandatory prerequisites. Any gate failure means the PR cannot be merged in its current state. Six gates total. Run `scripts/collect-gates.sh ` to evaluate gates 1-5 mechanically and run `scripts/check-coderabbit-threads.sh ` for gate 6. +All six gates are required. A PR that fails a gate cannot merge. +Run `scripts/collect-gates.sh ` for gates 1 through 5. +Run `scripts/check-coderabbit-threads.sh ` for gate 6. ## Contents - Gate 1: PR state OPEN -- Gate 2: CI green on latest head SHA +- Gate 2: CI passes on the PR SHA - Gate 3: Mergeable, no conflicts - Gate 4: Contributor compliance satisfied - Gate 5: Branch protection satisfied @@ -18,17 +20,14 @@ Mandatory prerequisites. Any gate failure means the PR cannot be merged in its c The PR's `state` must be `OPEN`. A `CLOSED` or `MERGED` PR is not a valid merge candidate regardless of its other properties. -**Why this is a hard kill:** A closed PR cannot merge no matter how high it scores. Even degraded mode skips closed PRs entirely. +A closed PR cannot merge. Without this gate, ranking could select a closed PR whose diff matches an open PR. -**Failure mode this catches:** When two PRs are byte-identical and the older one was closed before review completed, first-mover would otherwise pick the closed one. This gate prevents that. +## Gate 2: CI passes on the PR SHA -## Gate 2: CI green on latest head SHA - -The CI rollup must show all required checks passing on the **latest** head SHA, not on a stale ancestor. - -**Why "latest" matters:** If the author force-pushed after CI ran, the green checks are on the old commit. The new commit may have introduced regressions that haven't been re-checked. - -**How to evaluate:** `scripts/collect-gates.sh` returns the head SHA and a per-check status. Cross-reference each required check against `repo-policy.md`'s required-checks list. +All required checks must pass on the PR SHA. +If the author pushes another commit, record its short SHA and wait for its checks. +`scripts/collect-gates.sh` returns the SHA and each check status. +Compare the results with the required checks in `repo-policy.md`. ## Gate 3: Mergeable, no conflicts @@ -42,30 +41,33 @@ The CI rollup must show all required checks passing on the **latest** head SHA, ## Gate 4: Contributor compliance satisfied -The PR body must include a valid contributor `Signed-off-by:` declaration, and every commit in the PR must appear as `Verified` in GitHub. Check both conditions directly; a passing CI job is not a substitute for commit verification. - -**Why this is a hard kill:** contributor compliance is a self-serve eligibility requirement. Maintainers reject noncompliant PRs and do not amend, sign, force-push, approve, or merge them on the contributor's behalf. +The PR body must contain the contributor's `Signed-off-by:` declaration. +GitHub must show every commit as `Verified`. A passing CI job does not replace either check. +The contributor must correct a failure. +Maintainers must not amend, sign, force-push, approve, or merge the PR for the contributor. ## Gate 5: Branch protection satisfied -`reviewDecision: APPROVED`, plus all branch-protection requirements such as CODEOWNERS and required hooks. The skill may defer CODEOWNERS membership to branch protection, but Gate 4 always checks DCO and GitHub commit verification directly. - -**Why defer:** Branch protection rules are the source of truth. Re-implementing the check in the skill would drift from repo policy. If your repo doesn't enforce CODEOWNERS via branch protection, set `codeowners_enforced_via_branch_protection: false` in `repo-policy.md` and add explicit team checks. +Require `reviewDecision: APPROVED` and all branch-protection checks. +Use branch protection to enforce CODEOWNERS. +Gate 4 still checks DCO and commit verification. +If branch protection does not enforce CODEOWNERS, set `codeowners_enforced_via_branch_protection: false` and configure team checks. ## Gate 6: Automated reviewer threads resolved -All threads created by automated reviewers (e.g., CodeRabbit) must be in `resolved: true` state. **Zero unresolved threads is the bar.** - -**Why GraphQL, not REST:** GitHub's REST `/comments` endpoint exposes individual review comments without thread-resolution state. To check whether a thread is resolved, query `pullRequest.reviewThreads.isResolved` via GraphQL. This is what `scripts/check-coderabbit-threads.sh` does. +Each automated-review thread must have `resolved: true`. +REST comments do not include thread-resolution state. +Query `pullRequest.reviewThreads.isResolved` through GraphQL. +`scripts/check-coderabbit-threads.sh` runs this query. -**Configurable:** Add bot logins to `repo-policy.md` under `auto_reviewers`. Defaults to `coderabbitai` (CodeRabbit's bot login). +Add bot logins under `auto_reviewers` in `repo-policy.md`. The default is `coderabbitai`. ## Output For each gate, the skill records: - Pass/fail -- Evidence (head SHA, check names, mergeable state, thread IDs) +- Evidence (short SHA, check names, merge state, and thread IDs) - Whether the failure is **ineligible** (missing PR-body DCO or any unverified commit), **trivial** (for example, a missing issue link), or **substantive** (CI red, conflicts, or missing approvals) The ineligible/trivial/substantive classification feeds degraded mode (see `tiebreakers.md`). Ineligible PRs are rejected rather than ranked for salvage. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-1-correctness.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-1-correctness.md index 218bc311087..49f17e462fd 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-1-correctness.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-1-correctness.md @@ -1,6 +1,11 @@ + + + # Tier 1 — Correctness Checks -Six 1%-clever LLM judgments. Each catches a failure mode CI cannot see. Score: pass = 1, yellow = 0.5, fail = 0; weight 2.0× per check. Every judgment must carry file:line evidence. +Six model judgments cover failures that CI can miss. +Score each check as pass = 1, yellow = 0.5, or fail = 0. Use a weight of 2.0 for each check. +Include file and line evidence for each judgment. ## Contents @@ -9,13 +14,14 @@ Six 1%-clever LLM judgments. Each catches a failure mode CI cannot see. Score: p - 1.3 Negative test coverage - 1.4 Coverage shape - 1.5 Refactor-vs-behavior scan -- 1.6 Mocking purity +- 1.6 Mock boundaries ## 1.1 Test exercises bug path The PR's new/modified test must, when run on the pre-fix code, fail. A test that passes both before and after the fix proves nothing. -**How to evaluate:** Read each new test in the diff. For each assertion, ask: "Would this assertion have held on the pre-fix code?" If yes, the test doesn't exercise the bug. +**How to evaluate:** Read each changed test and its assertions. +Check whether each assertion fails on the code before the fix. If it passes, the test does not exercise the bug. **Common false-positive patterns to flag as yellow:** @@ -25,9 +31,10 @@ The PR's new/modified test must, when run on the pre-fix code, fail. A test that **Evidence to record:** Diff line of the assertion + the bug's pre-fix behavior + reasoning that the assertion would have failed pre-fix. -## 1.2 Comment-as-spec coverage +## 1.2 Acceptance criteria from comments -Acceptance criteria come from the issue body **and every comment**. Commenters often add asks the body doesn't capture: "and don't break Y while you're at it." All asks must map to a fix or test in the diff. +Read requirements in the issue body and comments. +Convert each requirement into an acceptance criterion. Map each criterion to a change or test in the diff. **How to evaluate:** From the issue's parsed criteria checklist (Step 1 of the workflow), check each item against: @@ -40,7 +47,7 @@ Acceptance criteria come from the issue body **and every comment**. Commenters o ## 1.3 Negative test coverage -The fix must have tests for invalid/edge inputs, not just the happy path that matches the reported bug. +The fix must have tests for invalid and boundary inputs, not only the reported valid case. **Look for assertions on:** @@ -56,9 +63,11 @@ The fix must have tests for invalid/edge inputs, not just the happy path that ma ## 1.4 Coverage shape -Every new code path in the diff has a test. Standard coverage % does not catch this — coverage can stay flat while new branches are untested if they're hit incidentally by unrelated tests. +Test each code path added by the diff. +Coverage percentage can stay unchanged when an unrelated test reaches a new branch. -**How to evaluate:** For each new branch (`if`, `else`, `try`/`catch`, `switch` arm) in the diff, find a test that exercises that branch specifically. A new `else` branch with no test is a yellow. +**How to evaluate:** Find a test for each new `if`, `else`, `catch`, or `switch` arm. +Mark an untested branch yellow. ## 1.5 Refactor-vs-behavior scan @@ -69,15 +78,19 @@ If the PR's title or description claims `refactor` / `rename` / `extract` / `mov - Changed `process.exit(` codes - Changed return values -**How to evaluate:** Run a count over the diff for these tokens. Net-zero is normal — code moves around. Net-positive in any of these = hidden behavior change inside what claims to be a refactor → yellow or fail depending on magnitude. +**How to evaluate:** Count these tokens in added and removed lines. +A refactor must not increase the total. An increase can show a behavior change. +Mark it yellow or fail based on its effect. -**Why this catches real bugs:** Authors sometimes "fix while refactoring" without flagging it. The fix may be correct, but landing it as a refactor bypasses the review attention a behavior change would get. +A behavior change in a refactor can miss the review required for that change. -## 1.6 Mocking purity +## 1.6 Mock boundaries -Tests must isolate **external** dependencies (network, filesystem, time, randomness, third-party APIs), not replace the **unit under test**. If a test for `validateInput()` mocks `validateInput()` itself, the test proves nothing. +Mock external dependencies. Do not mock the unit under test. +Fail the check when a mock replaces the behavior that the test claims to verify. -**How to evaluate:** Read each new test's mock setup. For each mock, ask: "Is this mock replacing an external dependency, or is it replacing the function this test is supposed to verify?" If the latter, fail. +**How to evaluate:** Read each mock setup. +Fail when a mock replaces the function that the test claims to verify. **Common red flags:** diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-2-quality.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-2-quality.md index e2fdfe75df4..d8ff88bfbcf 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-2-quality.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/checks/tier-2-quality.md @@ -1,6 +1,10 @@ + + + # Tier 2 — Code Quality Checks -Four 1%-clever LLM judgments. Score: pass = 1, yellow = 0.5, fail = 0; weight 1.0× per check. +Use four model judgments. +Score each check as pass = 1, yellow = 0.5, or fail = 0. Use a weight of 1.0 for each check. ## Contents @@ -11,42 +15,46 @@ Four 1%-clever LLM judgments. Score: pass = 1, yellow = 0.5, fail = 0; weight 1. ## 2.1 Description-vs-diff drift -Every touched file must be named or implied by the PR description's "Changes" section. Files outside the stated scope are "while I'm here" tweaks — common in long stacks where authors are heads-down and unrelated cleanups drift in. +The PR description must cover each changed file. +It can name the file or imply it through the described change. Mark an unrelated file yellow. -**How to evaluate:** From `gh pr view --json files,body`, build a set of touched files. Cross-reference against files named in the description. Files not named or implied by the description = yellow flag. +**How to evaluate:** Read the files and body with `gh pr view --json files,body`. +Mark a file yellow if the PR body does not name or imply it. -**Implied counts as covered:** If the description says "extracts onboard parsing into a new module," touching both `onboard.ts` and a new `onboard-parser.ts` is implied. But touching `unrelated-helper.ts` is not. +The description can imply a file. +For example, an onboarding-parser extraction implies changes to `onboard.ts` and `onboard-parser.ts`. +It does not imply a change to `unrelated-helper.ts`. ## 2.2 Migration completion -If the PR adds a new path (oclif version, v2 helper, replacement function, new format), the old path must be either: +If the PR adds a replacement path, it must do one of these actions: -- Deleted in this PR, OR -- Linked to a follow-up PR/issue in the body +- Delete the old path in this PR. +- Link to a follow-up PR or issue in the body. -Both surviving with no follow-up link = incomplete migration → yellow. +If the old path remains without a follow-up link, mark the check yellow. -**How to evaluate:** Look for diff additions that name a "v2" / "new" / "oclif" version of an existing symbol. Then grep the post-PR codebase for the old symbol's usages. If callers still use the old path AND the body has no follow-up link, flag. +**How to evaluate:** Find additions that name a replacement version of a symbol. +Search the resulting code for uses of the old symbol. +Mark the check yellow when callers remain and the PR has no follow-up link. -**Why this catches real bugs:** Half-migrations create maintenance debt. The new path drifts ahead, the old path bit-rots, callers get confused about which to use. +Two paths without a migration plan can diverge and leave callers on obsolete behavior. ## 2.3 Public surface preservation -For any content **change** (not pure move) in: +For a content change to a public surface below, require a Notes section and update the related documentation: - Flag definitions (`--name`, `Flags.(`, oclif flag schemas) - Help/usage strings (`Usage:`, `description:`, `summary:`) - Error messages (`throw new Error(`, `console.error`) - Exit codes (`process.exit(`) -…the PR body must have a Notes section explaining the change, AND the corresponding docs files (per `repo-policy.md`'s `docs_dir`) must be updated. - -**Distinguishing moves from changes:** Pure moves (added in one file, removed in another with same string content) are fine — no Notes or docs update needed. The check is for **content changes**: adding a new flag, renaming an existing one, rewriting an error message. +A move with unchanged text does not require these updates. **Yellow if:** Content changes are present but no Notes section. **Fail if:** Content changes change user-facing behavior AND no Notes AND no docs update. -**Why this catches real bugs:** Authors often make small UX changes (error message wording, help format) without realizing they're public-surface changes. End users notice. Forcing a Notes section forces awareness. +A Notes section records a user-facing change that can otherwise be missed during review. ## 2.4 Workaround-vs-root-cause @@ -57,8 +65,14 @@ Grep the diff for symptom-suppression patterns: - `if (err.code === '') return` (errno-specific silent ignores like EACCES, ENOENT, EEXIST) - Defensive returns in error paths that hide failures from callers -If any are added in the diff, the PR body must (a) link to a follow-up issue for the root-cause fix, OR (b) explain why the suppression is the correct behavior (e.g., "expected during shutdown, callers handle absence elsewhere"). Without (a) or (b) → yellow. +If the diff adds a suppression pattern, require one item in the PR body: + +- A link to an issue for the cause. +- An explanation of why suppression is the intended behavior. + +Otherwise, mark the check yellow. -**Why this catches real bugs:** Symptom-suppression hides bugs without fixing them. The same code can fail in production for a different reason and now no one sees it. Forcing a justification or follow-up makes the cost-of-suppression visible. +Suppression can hide another production failure. The explanation or follow-up records this risk. -**Score key:** Tier 2 has 4 checks total. Max contribution per PR = 4 × 1.0 = 4.0 points. Combined with Tier 1's max of 12.0 (6 checks × 2.0), the overall max weighted score is **16.0**. +**Score:** Tier 2 has four checks and contributes up to 4.0 points. +Tier 1 contributes up to 12.0 points. The maximum weighted score is 16.0. diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/repo-policy.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/repo-policy.md index 84ea9a3ae03..92b5d534455 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/repo-policy.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/repo-policy.md @@ -3,7 +3,7 @@ # Repo Policy -Configurable defaults that adapt the skill to a specific repository. Edit this file when adopting the skill in a new repo. The skill reads these values to know what to gate on. +Repository settings for this skill. Edit this file when you use the skill in another repository. ## Contents @@ -22,13 +22,14 @@ CODEOWNERS approval is enforced via branch protection. The skill checks `reviewD codeowners_enforced_via_branch_protection: true ``` -If your repo does NOT enforce CODEOWNERS via branch protection, set this to `false` and add an explicit list of required teams to check. +If branch protection does not enforce CODEOWNERS, set this value to `false` and add the required teams. ## Commit compliance policy -NemoClaw default: the PR description requires a `Signed-off-by:` line, and every PR commit requires a GitHub verified signature. -DCO is enforced by the `dco-check` workflow and checked directly in the PR body by the comparator and merge gate. -Verified signatures are checked directly for every PR commit by the comparator and merge gate; branch protection remains a separate gate. +NemoClaw requires a `Signed-off-by:` line in the PR description. +GitHub must show each PR commit as `Verified`. +The `dco-check` workflow, comparator, and merge gate check DCO. +The comparator and merge gate check commit verification. Branch protection is a separate gate. ```yaml dco_required: true @@ -68,7 +69,7 @@ The skill defers to ratchet enforcement in CI. NemoClaw uses `ci/coverage-thresh coverage_ratchet_enforced_via_ci: true ``` -If your repo does NOT ratchet coverage in CI, the skill needs to compute coverage delta itself — flag this as a v2 gap. +If CI does not enforce coverage, report that this skill does not compute the coverage change. ## Discovery search diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh index 7616893afa8..31791cae79f 100755 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh @@ -3,7 +3,7 @@ # SPDX-License-Identifier: Apache-2.0 # # Collect Tier 0 gate state for a PR and emit JSON for downstream scoring. -# Covers gates 1-5 (state, CI on latest SHA, mergeable, contributor compliance, +# Covers gates 1-5 (state, CI on PR SHA, mergeable, contributor compliance, # branch protection). Gate 6 (CodeRabbit threads) is handled by # check-coderabbit-threads.sh. # @@ -51,7 +51,7 @@ raw=$(gh pr view "$pr" "${repo_args[@]}" \ state=$(printf '%s' "$raw" | jq -r .state) gate_state_open=$([ "$state" = "OPEN" ] && echo true || echo false) -# Gate 2: CI green on latest head SHA. statusCheckRollup contains the latest run. +# Gate 2: CI passes on the PR SHA. statusCheckRollup contains the run for that SHA. # Fail closed when required checks are missing, including an empty rollup. required_checks='["checks","check-hash","changes","commit-lint","dco-check","E2E / PR Gate"]' observed_checks=$(printf '%s' "$raw" | jq -c '[(.statusCheckRollup // [])[] | (.name // .context // empty)] | unique') @@ -185,7 +185,7 @@ jq -n \ head_sha: $head_sha, gates: { state_open: $gate_state_open, - ci_green_latest_sha: $gate_ci_green, + ci_green_sha: $gate_ci_green, mergeable: $gate_mergeable, contributor_compliance: $gate_contributor_compliance, branch_protection: $gate_branch_protection diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py index 0b0dd6405f0..5966bd5b798 100755 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/scripts/render-verdict.py @@ -14,7 +14,7 @@ { "number": 2851, "title": "...", - "tier_0": {"state_open": true, "ci_green_latest_sha": true, ...}, + "tier_0": {"state_open": true, "ci_green_sha": true, ...}, "tier_1": {"test_exercises_bug_path": "pass", "comment_as_spec": "yellow", ...}, "tier_2": {"description_diff_drift": "pass", ...}, "matrix": {"criterion 1": "covered", "criterion 2": "missing", ...}, @@ -27,7 +27,7 @@ "tiebreaker_fired": "smaller_diff", "winner": 2851, "closest_to_ready": null, - "mode": "happy" // optional assertion; derived from Tier 0 gates + "mode": "happy" // optional assertion. Derived from Tier 0 gates } Usage: @@ -48,7 +48,7 @@ TIER_0_GATES = ( ("state_open", "State open"), - ("ci_green_latest_sha", "CI green on latest SHA"), + ("ci_green_sha", "CI on PR SHA"), ("mergeable", "Mergeable"), ("contributor_compliance", "Contributor compliance"), ("branch_protection", "Branch protection"), diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md index dead3c9c482..ec5b03c06be 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/templates/verdict.md @@ -19,7 +19,7 @@ Render the final scorecard with `scripts/render-verdict.py`. Below is the human- |---|---|---| | **Tier 0 — gates** | | | | State OPEN | pass | pass | -| CI green (latest SHA) | pass | fail (stale) | +| CI on `` | pass | fail | | Mergeable | pass | pass | | Contributor compliance | pass | pass | | Branch protection | pass | pass | @@ -35,6 +35,7 @@ Render the final scorecard with `scripts/render-verdict.py`. Below is the human- | Description-vs-diff drift | pass | pass | | Migration completion | pass | yellow (no follow-up link) | | Public surface preservation | pass | pass | +| Workaround versus root cause | pass | pass | | **Weighted score** | 14.5 / 16.0 | 9.0 / 16.0 | ### Behavior Coverage Matrix @@ -48,21 +49,21 @@ Render the final scorecard with `scripts/render-verdict.py`. Below is the human- ### Verdict: MERGE PR #A Reasoning trace: -- PR #B failed Tier 0 (CI fail on latest SHA after force-push at SHA ) -- PR #A score 18.5 vs PR #B score 14.0 -- PR #A misses criterion 3; cherry-pick PR #B's test at to cover it +- 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. ### Suggested action -1. Merge PR #A -2. Cherry-pick test from PR #B at `:` to cover criterion 3 -3. Close PR #B with comment linking to #A and noting the cherry-pick +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. ### Reasoning evidence -- Tier 0 gate "CI green": PR #A latest SHA , all 12 required checks passed; PR #B latest SHA , "test-cli" failed at -- Tier 1.1 PR #A: test at `:` asserts on ; pre-fix code returned ; assertion would have failed +- 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` -- ... ... +- ... ... ``` Every judgment in the trace must include: @@ -79,7 +80,7 @@ If the verdict is **degraded mode** ("Neither mergeable yet"), substitute the ve **PR #A — ineligible:** - Substantive: Rebase against current main (3 conflicts in ``) -- Ineligible: contributor gate failed; the author must fix each failing requirement before re-review +- Ineligible: The contributor gate failed. The author must fix each failure before another review. - Missing PR-body DCO declaration: update the PR body - Missing GitHub Verified commit history: replace the branch with compliant history @@ -89,6 +90,6 @@ If the verdict is **degraded mode** ("Neither mergeable yet"), substitute the ve ### Suggested action -1. Ask the PR #A author to fix each failing contributor gate; do not repair or approve the PR on their behalf -2. Salvage PR #B by resolving the substantive failures, then re-run this skill to confirm the winner +1. Ask the PR #A author to fix each contributor-gate failure. Do not repair or approve the PR for the author. +2. Resolve the substantive failures in PR #B. Then, run this skill again to confirm the winner. ``` diff --git a/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md b/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md index 1732c8bf207..b9eba1b6c11 100644 --- a/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md +++ b/.agents/skills/nemoclaw-maintainer-pr-comparator/tiebreakers.md @@ -3,7 +3,7 @@ # Tier 3 — Ranking and Degraded Mode -Final decision logic. Two paths: happy mode when at least one PR passes all Tier 0 gates, degraded mode when none do. +Use happy mode when one or more PRs pass Tier 0. Use degraded mode when none pass. ## Contents @@ -18,39 +18,40 @@ Eliminate any PR failing Tier 0. Among survivors: - Set `winner` only to a survivor and leave `closest_to_ready` null. 1. Compute weighted score across Tiers 1-2. -2. Build the **behavior-coverage matrix** (see below). If matrix has a clear winner per criterion, that wins. -3. Apply tiebreakers in order. First tiebreaker that distinguishes the PRs picks the winner. +2. Build the **behavior-coverage matrix**. Use it as evidence for the weighted score and tiebreakers. +3. Apply tiebreakers in order. + Set the winner when the evidence distinguishes a PR. + Otherwise, leave `winner` null. ### Tiebreakers (in order) -1. **Supersession.** Any PR whose body declares `supersedes #N` / `replaces #N` / `closes in favor of #N` / `folds in #N` against another candidate wins immediately. (See `scripts/parse-supersession.sh`.) -2. **Smaller diff.** Lines changed proportional to the issue's scope. Bug fixes target <200 LOC. +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.** Compare last commit timestamps. Catches stalled-PR-replaced-by-fresh patterns. -5. **Earlier PR (final deterministic fallback).** Use only when nothing above distinguishes — first-mover gets the tie. - -If after all five tiebreakers no PR wins: recommend "merge A, cherry-pick relevant tests from B," picking A by lowest PR number deterministically. +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. ## Degraded mode (no PR passes Tier 0) -Don't give up — pick the closest-to-ready and recommend salvage steps. +When no PR passes Tier 0, rank eligible PRs by the work needed before merge. 1. Classify each Tier 0 failure per PR: - **Trivial** (author-fixable without changing commit compliance): missing issue link, stale base, force-pushed since last review - - **Ineligible**: missing PR-body DCO declaration or any commit that is not GitHub Verified. Reject rather than salvage; the contributor must provide a clean compliant history. + - **Ineligible**: The PR body has no DCO declaration, or GitHub does not show each commit as `Verified`. Reject the PR. The contributor must provide a compliant history. - **Substantive** (real work): CI red, mergeability conflicts, missing CODEOWNERS approvals, unresolved CodeRabbit threads 2. Distance-to-ready ranking: - - Any PR with an **Ineligible** failure ranks below every eligible PR; if all candidates are ineligible, return a rejection-only verdict + - Rank each ineligible PR below every eligible PR. If all candidates are ineligible, return a rejection-only verdict. - Among eligible PRs, fewer substantive failures wins - Tie → fewer trivial failures wins - - Tie → higher Tier 1-2 weighted score wins (correctness beneath the broken plumbing) + - If still tied, use the higher Tier 1 and Tier 2 weighted score. + - If the available evidence cannot support this ordering, leave `closest_to_ready` null. 3. Output: - - Leave `winner` null; it is reserved for a fully eligible merge recommendation - - Set `closest_to_ready` only to an open PR that passes contributor compliance; leave it null for a rejection-only verdict + - Leave `winner` null. Use it only for an eligible merge recommendation. + - Set `closest_to_ready` only to an open PR that passes contributor compliance. Leave it null for a rejection-only verdict. - Per-PR Tier 0 failure list - - Per-PR Tier 1-2 scorecard (so the winner has objective merit beneath the gates) + - Tier 1 and Tier 2 scorecard for each PR - Verdict: "Neither mergeable yet. PR A is closer — fix [substantive list]. PR B has [issues]." - - Salvage steps per eligible PR (rebase command, CR thread links, etc.) + - Put salvage steps for each eligible PR in that PR's evidence map so the renderer includes them in the reasoning evidence. ## Behavior-coverage matrix @@ -61,10 +62,11 @@ For each acceptance criterion (from issue body + comments), build a row showing |------------------------------|------------|------------| | Empty input rejected | covered | covered | | Boundary value handled | covered | missing | -| "Don't break Y" (commenter) | missing | covered | +| Preserve Y (commenter) | missing | covered | | Error message preserved | covered | partial | ``` -**Why the matrix matters:** When neither PR dominates on weighted score, the matrix surfaces the cherry-pick opportunity. The verdict can recommend "merge A for criteria 1+2+4, cherry-pick B's test for criterion 3." +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. Per-criterion winner cells: `covered` (full), `partial` (yellow), `missing` (red). diff --git a/.agents/skills/nemoclaw-maintainer-release-notes/SKILL.md b/.agents/skills/nemoclaw-maintainer-release-notes/SKILL.md index 459c25b81dd..41dae30b9bb 100644 --- a/.agents/skills/nemoclaw-maintainer-release-notes/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-release-notes/SKILL.md @@ -9,14 +9,18 @@ user_invocable: true # NemoClaw Maintainer Release Notes -Draft NemoClaw release notes from live release data. The house style is: +Draft the post-tag NemoClaw Announcement from GitHub tag and compare data. The house style is: - three narrative lead paragraphs, - a categorized list of shipped changes, -- one "what changed and why it matters / why we did it" bullet for every included shipped change, +- one bullet for each shipped change that states what changed and why it matters, - external-only contributor thanks, - visible `#NNNN` GitHub links. +Apply [NemoClaw Technical English](../../../CONTRIBUTING.md#nemoclaw-technical-english) to new or modified Announcement text. +State the changed behavior, affected users, and required action when one exists. +During the changed-text pilot, do not rewrite unrelated historical release text. + Create a local Markdown draft. Do not create or update a GitHub Discussion; the maintainer posts the announcement manually. ## Prerequisites diff --git a/.agents/skills/nemoclaw-maintainer-security-code-review/SKILL.md b/.agents/skills/nemoclaw-maintainer-security-code-review/SKILL.md index 544d675d9c1..cadc44587f2 100644 --- a/.agents/skills/nemoclaw-maintainer-security-code-review/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-security-code-review/SKILL.md @@ -1,12 +1,16 @@ --- name: nemoclaw-maintainer-security-code-review -description: Reviews code changes in a GitHub PR or issue for security. Checks changed files against nine categories and reports PASS/WARNING/FAIL verdicts. Use when reviewing pull requests for security vulnerabilities, hardcoded secrets, injection flaws, auth bypasses, or insecure configurations. Trigger keywords - security review, code review, appsec, vulnerability assessment, security audit, review PR security. +description: Review a PR, or a PR linked to an issue, for security risks. Check nine categories and report PASS, WARNING, or FAIL. Use when reviewing code for vulnerabilities, secrets, injection, authorization bypasses, or unsafe configuration. Trigger keywords - security review, code review, appsec, vulnerability assessment, security audit, review PR security. user_invocable: true --- + + + # Security Code Review -Review the changes in a GitHub PR or issue for security. Report a verdict for each category. +Review the changes in a GitHub PR for security. An issue input must identify one open linked PR. +Report a verdict for each category. ## Prerequisites @@ -14,47 +18,60 @@ Review the changes in a GitHub PR or issue for security. Report a verdict for ea - `git` must be available. - Network access to clone repositories and fetch PR metadata. -## When to Use - -- Reviewing a pull request before merge for security vulnerabilities. -- Triaging a GitHub issue that reports a potential security flaw. -- Auditing code changes for hardcoded secrets, injection flaws, auth bypasses, or insecure configurations. - ## Step 1: Parse the GitHub URL -If the user provided a PR or issue URL, extract the owner, repo, and number. If not, ask for one. +If the user gives a PR or issue URL, extract the owner, repository, and number. +Otherwise, ask for the URL. Supported URL formats: - `https://github.com/OWNER/REPO/pull/NUMBER` - `https://github.com/OWNER/REPO/issues/NUMBER` +For a PR URL, verify the number before Step 2: + +```bash +gh pr view --repo OWNER/REPO --json number,url +``` + +For an issue URL, list its open closing PRs: + +```bash +gh issue view --repo OWNER/REPO --json closedByPullRequestsReferences \ + --jq '.closedByPullRequestsReferences | map(select(.state == "OPEN")) | .[].number' +``` + +Continue only when this returns one PR number, and verify that number with `gh pr view`. +If it returns zero or more than one, stop and ask for the PR URL. +Use the verified PR number in each later command. + ## Step 2: Check Out the Code -Determine whether you are already in the target repository (compare `gh repo view --json nameWithOwner -q .nameWithOwner` against the URL). If you are: +Compare `gh repo view --json nameWithOwner -q .nameWithOwner` with the URL. +If the repositories match, check out the verified PR: ```bash gh pr checkout ``` -If reviewing a different repo, clone it to a temporary directory first: +If the repositories do not match, clone the target to a temporary directory: ```bash -TMPDIR=$(mktemp -d) -gh repo clone OWNER/REPO "$TMPDIR" -cd "$TMPDIR" +REVIEW_DIR=$(mktemp -d) +gh repo clone OWNER/REPO "$REVIEW_DIR" +cd "$REVIEW_DIR" gh pr checkout ``` ## Step 3: Identify Changed Files -List all files changed relative to the base branch: +List all files changed from the base branch: ```bash git diff main...HEAD --name-status ``` -If the PR targets a branch other than `main`, use the correct base. Check with: +If the PR targets another branch, use that branch as the base. Check it with: ```bash gh pr view --json baseRefName -q .baseRefName @@ -62,13 +79,13 @@ gh pr view --json baseRefName -q .baseRefName ## Step 4: Read Each Changed File and Diff -Read the full content of each changed file and the diff for that file: +Read each changed file. Read its diff: ```bash git diff main...HEAD -- ``` -For large PRs (more than 30 changed files), prioritize files in this order: +If a PR changes more than 30 files, review them in this order: 1. Files that handle authentication, authorization, or credentials. 2. Files that process user input (API handlers, CLI argument parsing, URL parsing). @@ -78,27 +95,27 @@ For large PRs (more than 30 changed files), prioritize files in this order: ## Step 5: Analyze Against the Security Checklist -For each of the 9 categories below, assign a verdict: +For each of the nine categories, assign a verdict: -- **PASS** — no issues found (give a brief reason). -- **WARNING** — a concern (describe the risk and fix). -- **FAIL** — a vulnerability (describe the impact, severity, and fix). +- Use **PASS** when you find no issue. Give a short reason. +- Use **WARNING** for a concern. Describe the risk and fix. +- Use **FAIL** for a vulnerability. Describe its impact, severity, and fix. ### Category 1: Secrets and Credentials - No hardcoded secrets, API keys, passwords, tokens, or connection strings in code, configs, or test fixtures. - No secrets committed to version control (check for `.env` files, PEM/key files, credential JSON). -- Tokens and credentials passed via environment variables or secret stores, not string literals. +- Pass tokens and credentials through environment variables or secret stores. Do not put them in string literals. ### Category 2: Input Validation and Data Sanitization -- Validate user-controlled inputs (APIs, forms, URLs, headers, query params, file uploads) against an allowlist of types, lengths, and formats. +- Validate each user-controlled input. Allow only the required type, length, and format. - Encode and escape inputs to prevent XSS, SQL injection, command injection, path traversal, and SSRF. - Use safe parsers for untrusted data (no `pickle.loads`, `yaml.unsafe_load`, `eval`, `new Function`, or similar). ### Category 3: Authentication and Authorization -- Authenticate new or modified endpoints before processing requests. +- Require authentication before a new or changed endpoint processes a request. - Allow users to access or modify only resources they own or may use. - Prevent horizontal and vertical privilege escalation. - Verify token expiry, signature, and scope. @@ -106,15 +123,15 @@ For each of the 9 categories below, assign a verdict: ### Category 4: Dependencies and Third-Party Libraries - Check new dependencies for known CVEs (OSV, Snyk, GitHub Advisory DB). -- Pin production dependencies; do not use floating ranges. -- Preserve OSS license compatibility. +- Pin production dependencies. Do not use floating ranges. +- Confirm that each dependency has a compatible open-source license. - Use trusted registries. ### Category 5: Error Handling and Logging - Do not leak stack traces, internal paths, or sensitive data in errors. - Do not log secrets, tokens, passwords, or PII. -- Catch exceptions where callers can handle them; do not expose state through crashes. +- Catch exceptions where callers can handle them. Do not expose state through crashes. ### Category 6: Cryptography and Data Protection @@ -125,18 +142,18 @@ For each of the 9 categories below, assign a verdict: ### Category 7: Configuration and Security Headers - Disable debug mode, restrict permissions, and expose only needed ports. -- For HTTP endpoints, set CSP and CORS. Do not use wildcard origins in authenticated contexts. +- Set CSP and CORS for HTTP endpoints. Do not allow wildcard origins for authenticated requests. - Run container images as non-root users with minimal base images and pinned digests. ### Category 8: Security Testing - Test malicious input, boundary values, and unauthorized access attempts. - Do not reduce existing security test coverage. -- Test that forbidden actions are denied. +- Verify that the system denies forbidden actions. -### Category 9: Holistic Security Posture +### Category 9: System Security -- Do not weaken the system's security. +- Check whether the change weakens an existing security control. - Do not rely on client-only validation or incomplete checks. - Use least privilege for code, services, and users. - Prevent TOCTOU race conditions in security-critical paths. @@ -157,7 +174,7 @@ One row per finding: | # | Category | Severity | File:Line | Description | Recommendation | |---|----------|----------|-----------|-------------|----------------| -If no findings, state explicitly that the review is clean. +If there are no findings, state that the review found none. ### Detailed Analysis @@ -169,7 +186,9 @@ List every file analyzed. ## Important Notes -- If the PR has no changed files or is a draft with no code, state that and skip the review. -- For NemoClaw PRs, check sandbox escape vectors: SSRF bypasses, Dockerfile injection, network policy circumvention, credential leakage, and blueprint tampering. -- Do not skip categories. If a category is not applicable to the changes (e.g., no cryptography involved), mark it PASS with "Not applicable — no cryptographic operations in this change." -- When in doubt about severity, err on the side of WARNING rather than PASS. +- If the PR has no changed files, state that result and stop the review. +- If no changed or reviewable security surface exists, state that result and stop the review. +- Review security surfaces in drafts, including Dockerfiles, workflows, network policies, blueprints, dependencies, and security configuration. +- For NemoClaw PRs, check SSRF bypasses, Dockerfile injection, network-policy bypasses, credential leaks, and blueprint changes. +- Do not skip a category. If a category does not apply, mark it PASS and state why. +- If severity is uncertain, use WARNING instead of PASS. diff --git a/.agents/skills/nemoclaw-maintainer-triage/SKILL.md b/.agents/skills/nemoclaw-maintainer-triage/SKILL.md index fe3fa290c44..0a037f90513 100644 --- a/.agents/skills/nemoclaw-maintainer-triage/SKILL.md +++ b/.agents/skills/nemoclaw-maintainer-triage/SKILL.md @@ -1,6 +1,6 @@ --- name: nemoclaw-maintainer-triage -description: AI-assisted triage for NVIDIA/NemoClaw issues and PRs using native Issue Type, Project fields, and the canonical label taxonomy. Supports single-item and batch modes, presents a dry run, and applies only the accepted write set. Trigger keywords - triage, label issues, suggest labels, batch triage, triage issue, triage PR, label this, what labels. +description: Triage NemoClaw issues and PRs with Issue Type, Project fields, and allowed labels. Support one item or a batch. Show proposed changes and apply only changes the maintainer accepts. Trigger keywords - triage, label issues, suggest labels, batch triage, triage issue, triage PR, label this, what labels. user_invocable: true --- @@ -9,7 +9,8 @@ user_invocable: true # NemoClaw Maintainer — Triage -Triage issues and PRs through the canonical NemoClaw workflow. Native Issue Type owns issue classification, Project fields own priority and lifecycle, and labels own routing and immediate action queues. +Use Issue Type for issue classification. Use Project fields for priority and lifecycle. +Use labels for routing and action queues. ## Step 1: Load Canonical Policy @@ -20,7 +21,8 @@ Before evaluating an item, read these files in order: 3. [label-taxonomy.json](../nemoclaw-maintainer-policies/references/label-taxonomy.json) 4. [examples.md](../nemoclaw-maintainer-policies/references/examples.md) -Do not use a skill-local label guide. The policy package is the only source of truth for Issue Type, Project fields, labels, confidence, authorization, and output shape. +Do not use another label guide. +The policy package is the source of truth for Issue Type, Project fields, labels, confidence, authorization, and output shape. ## Step 2: Determine Mode @@ -31,7 +33,9 @@ gh issue view --repo NVIDIA/NemoClaw --json number,title,body,labels,ur gh pr view --repo NVIDIA/NemoClaw --json number,title,body,labels,url,author,files,isDraft,mergeStateStatus,projectItems,statusCheckRollup ``` -Use the command matching the item kind. For issues, also read the native Issue Type through the GitHub GraphQL API. For Project Priority and Status, use live Project 199 data rather than inferring state from labels. +Use the command for the item type. +For issues, read Issue Type through the GitHub GraphQL API. +Read Project Priority and Status from Project 199. Do not infer them from labels. **Batch mode** — collect both normal inbox items and unlabeled items: @@ -42,26 +46,34 @@ gh pr list --repo NVIDIA/NemoClaw --state open --label "needs: triage" --limit 5 gh pr list --repo NVIDIA/NemoClaw --state open --limit 50 --json number,title,body,labels,url,author,isDraft,mergeStateStatus ``` -From the unfiltered results, retain items with no labels, merge them with the `needs: triage` results, and deduplicate by item kind and number. Work through the resulting set one item at a time. +Keep unlabeled items from the unfiltered results. +Combine them with the `needs: triage` results. Remove duplicates by item type and number. +Process one item at a time. ## Step 3: Present the Dry Run Use the JSON-compatible payload defined by canonical `triage-instructions.md`. Include: -- native Issue Type for issues; -- Project Priority and Status recommendations; -- only canonical labels from `label-taxonomy.json`; -- labels to remove, including a completed `needs: triage` inbox marker; -- confidence, rationale, questions, and `human_review_required`; +- native Issue Type for issues +- Project Priority and Status recommendations +- only labels from `label-taxonomy.json` +- labels to remove, including a completed `needs: triage` inbox marker +- confidence, rationale, questions, and `human_review_required` - the proposed public comment, when one is useful. -Prefer no label over a guessed label. Never substitute labels for Issue Type, Priority, Status, or resolution. Never propose an unknown label, and never propose `PRR` during normal triage. +Prefer no label over a guessed label. +Do not use labels for Issue Type, Priority, Status, or resolution. +Do not propose an unknown label. Do not propose `PRR` during triage. In batch mode, present each dry run and wait for an explicit `apply`, `skip`, or edited write set before moving to the next item. ## Step 4: Apply Only the Accepted Write Set -An accepted dry run authorizes only the fields, labels, and comment the maintainer accepted. Resolve live Issue Type IDs, Project field IDs, and Project option IDs immediately before writing; do not hardcode mutable IDs in this skill. +Acceptance authorizes only the fields, labels, and comment in the proposal. +Before each write, re-read Issue Type, Project fields, and labels, then resolve all live IDs. +Do not store mutable IDs in this skill. +If the state differs from the accepted proposal's base state, stop and present an updated proposal for acceptance. +Resume writes only after the user accepts the updated proposal. Apply writes in this order: @@ -70,16 +82,17 @@ Apply writes in this order: 3. Remove `needs: triage` when the inbox action is complete. 4. Post the accepted comment, if any. -If the accepted plan contains a low-confidence inference, an unknown label, or a write outside the current authorization context, stop and return a corrected dry run instead of writing. +Do not write if the accepted plan contains a low-confidence inference, an unknown label, or an unauthorized field. +Return a corrected proposal. ## Step 5: Report For every applied item, report: -- Issue Type before and after, when applicable; -- Project Priority and Status before and after; -- labels added and removed; -- whether a comment was posted; +- Issue Type before and after, when applicable +- Project Priority and Status before and after +- labels added and removed +- whether a comment was posted - any proposed write that was skipped and why. Do not write an external activity log unless the invoking maintainer explicitly asks for one. @@ -88,7 +101,7 @@ Do not write an external activity log unless the invoking maintainer explicitly Prioritize candidates using policy evidence, not labels that duplicate Project Priority: -1. Security-sensitive or outage/data-loss reports that may warrant Project Priority `Urgent` or `High`. -2. Action-blocked items requiring a precise author or maintainer response. -3. Items waiting longest for an initial actionable triage decision. +1. Security, outage, or data-loss reports that might need Project Priority `Urgent` or `High`. +2. Items that need a response from an author or maintainer. +3. Items waiting longest for triage. 4. Remaining items by recency. diff --git a/.agents/skills/nemoclaw-skills-guide/SKILL.md b/.agents/skills/nemoclaw-skills-guide/SKILL.md index fba92b18931..3401fcf02cc 100644 --- a/.agents/skills/nemoclaw-skills-guide/SKILL.md +++ b/.agents/skills/nemoclaw-skills-guide/SKILL.md @@ -4,6 +4,9 @@ description: "Start here. Introduces what NemoClaw is, what agent skills are ava license: "Apache-2.0" --- + + + # NemoClaw Skills Guide NVIDIA NemoClaw runs OpenClaw always-on assistants inside hardened OpenShell sandboxes with NVIDIA inference (Nemotron). @@ -49,18 +52,18 @@ documentation updates, and onboarding new messaging channels. | Skill | Summary | |-------|---------| | `nemoclaw-maintainer-morning` | Morning standup: triage the backlog, determine the day's target version, label selected items, surface stragglers, and output the daily plan. | -| `nemoclaw-maintainer-triage` | Suggest and optionally apply labels for issues and PRs using the live NemoClaw triage instructions. | -| `nemoclaw-maintainer-policies` | Read-only maintainer workflow policy reference for Issue Type, labels, Project fields, daily release labels, the pre-tag changelog gate, triage, duplicates, blocked items, and workflow decisions. | -| `nemoclaw-maintainer-cross-issue-sweep` | Scan open issues for adjacent fixes or contradiction risks when reviewing a PR. | -| `nemoclaw-maintainer-day` | Daytime loop: pick the highest-value version-targeted item and execute the right workflow (merge gate, salvage, security sweep, test gaps, hotspot cooling, or sequencing). Designed for `/loop`. | +| `nemoclaw-maintainer-triage` | Propose Issue Type, Project fields, and approved labels for issues and PRs. Apply only changes that the maintainer accepts. | +| `nemoclaw-maintainer-policies` | Answer maintainer workflow questions from the read-only policy references. | +| `nemoclaw-maintainer-cross-issue-sweep` | Find open issues that a PR can also fix or conflict with. Report file and line evidence. | +| `nemoclaw-maintainer-day` | Run one daytime maintainer pass for the release version. Select a merge, salvage, security, test, conflict, or sequencing workflow. Designed for `/loop`. | | `nemoclaw-maintainer-evening` | End-of-day handoff: require the pre-tag dated changelog PR, check version progress, identify stragglers, generate a QA handoff summary, cut the release tag, carry stragglers forward, retire the released label, and hand off the Announcement. | | `nemoclaw-maintainer-cut-release-tag` | Verify the dated changelog entry, cut an annotated semver tag on a maintainer-confirmed `origin/main` commit, wait for workflow-managed `latest`, carry remaining open items forward, and delete the released label; `lkg` stays manual. | | `nemoclaw-maintainer-release-notes` | Draft the post-tag Announcement from live tag/compare data, with the three-paragraph narrative, categorized change list, and external-only contributor thanks. | -| `nemoclaw-maintainer-find-review-pr` | Find open security PRs with Urgent or High Project Priority, link each to its issue, detect duplicates, and present a review summary. | -| `nemoclaw-maintainer-pr-comparator` | Compare competing PRs for the same issue and recommend which one to merge. | +| `nemoclaw-maintainer-find-review-pr` | Find open security PRs with Urgent or High Project Priority. Link each PR to its issue and identify competing PRs. | +| `nemoclaw-maintainer-pr-comparator` | Compare open PRs for the same issue. Apply gates and score the eligible PRs before you recommend one to merge. | | `nemoclaw-maintainer-normalize-title-tags` | Preview and remove bracketed `NemoClaw` title tags from issues and PRs case-insensitively, even when the tag appears later in the title. | | `nemoclaw-maintainer-refactor-docs` | Split oversized Fern docs into focused topics with journey-based navigation, canonical ownership, route-safe redirects, variant checks, and deduplication. | -| `nemoclaw-maintainer-security-code-review` | Perform a 9-category security review of a PR or issue, producing per-category PASS/WARNING/FAIL verdicts. | +| `nemoclaw-maintainer-security-code-review` | Review PR or issue changes in nine security categories. Report PASS, WARNING, or FAIL for each category. | | `nemoclaw-maintainer-verify-stale` | Verify whether old issues with native Issue Type `Bug` still reproduce on latest. Reuses or provisions a Brev box, scores confidence, and proposes evidence-backed Project/comment writes for approval; never auto-closes. | ### Contributor Skills @@ -68,7 +71,7 @@ documentation updates, and onboarding new messaging channels. | Skill | Summary | |-------|---------| | `nemoclaw-contributor-onboard` | Set up, repair, or verify a trusted source checkout, with explicit opt-ins for host-visible CLI exposure, the pinned agent, and runtime onboarding. | -| `nemoclaw-contributor-create-pr` | Create GitHub pull requests that follow the NemoClaw PR template, including pre-PR checks, conventional commit titles, DCO sign-off, post-push CI monitoring, and CodeRabbit/PR Review Advisor follow-up. | +| `nemoclaw-contributor-create-pr` | Create a PR with the NemoClaw template, required checks, DCO declaration, and verified commits. Then, monitor CI and automated reviews. | | `nemoclaw-contributor-update-dependencies` | Audit dependency upgrades release by release, trace upstream source and test changes into downstream contracts, resolve migration concerns, and verify immutable artifacts separately. | | `nemoclaw-contributor-onboard-messaging-channel` | Add or review a new messaging channel with manifest-first implementation, upstream source analysis, plugin install confirmation, reachability checks, policies, docs, and tests. | | `nemoclaw-contributor-update-docs` | Scan recent git commits, update user-facing documentation, and create the canonical `docs/changelog/YYYY-MM-DD.mdx` entry in the pre-tag release-note docs PR. | diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index f460a63455e..f59719df7e0 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,6 +1,6 @@ ## Summary - + ## Related Issue diff --git a/.github/actions/ci-wechat-runtime-audit/audit.sh b/.github/actions/ci-wechat-runtime-audit/audit.sh index 5220f5cc7e1..db2fe4ad814 100755 --- a/.github/actions/ci-wechat-runtime-audit/audit.sh +++ b/.github/actions/ci-wechat-runtime-audit/audit.sh @@ -120,7 +120,7 @@ wechat_spec="${package_identity[0]}" wechat_tarball="${package_identity[1]}" wechat_integrity="${package_identity[2]}" -# Materialize the exact PR-provided graph without executing dependency scripts. +# Materialize the PR-provided dependency graph without executing dependency scripts. npm --prefix "$runtime_dir" ci \ --userconfig "$trusted_npmrc" \ --registry "$npm_registry" \ diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 944804a16cb..0a2ba42d33e 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -44,7 +44,7 @@ on: default: false type: boolean checkout_sha: - description: PR head commit selected by the controller. + description: PR SHA selected by the controller. required: false default: "" type: string @@ -155,7 +155,7 @@ jobs: "https://api.github.com/repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}")" [[ "$(jq -r '.state' <<< "$pull_json")" == "open" ]] || { echo "::error::pull request must still be open"; exit 1; } [[ "$(jq -r '.head.repo.full_name // ""' <<< "$pull_json")" == "$GITHUB_REPOSITORY" ]] || { echo "::error::pull request must originate from this repository"; exit 1; } - [[ "$(jq -r '.head.sha' <<< "$pull_json")" == "$CHECKOUT_SHA" ]] || { echo "::error::checkout_sha must match the PR head commit"; exit 1; } + [[ "$(jq -r '.head.sha' <<< "$pull_json")" == "$CHECKOUT_SHA" ]] || { echo "::error::checkout_sha must match the PR SHA"; exit 1; } [[ "$(jq -r '.base.sha' <<< "$pull_json")" == "$BASE_SHA" ]] || { echo "::error::base_sha must match the PR base commit"; exit 1; } - name: Prepare E2E workspace diff --git a/.github/workflows/pr-e2e-gate.yaml b/.github/workflows/pr-e2e-gate.yaml index b17c6789ee9..41b25219981 100644 --- a/.github/workflows/pr-e2e-gate.yaml +++ b/.github/workflows/pr-e2e-gate.yaml @@ -78,7 +78,7 @@ jobs: - name: Install controller dependencies run: npm ci --ignore-scripts --no-audit --no-fund - - name: Reserve exact-diff gate + - name: Reserve PR/base SHA gate env: BASE_SHA: ${{ github.event.pull_request.base.sha }} GITHUB_TOKEN: ${{ github.token }} @@ -115,7 +115,7 @@ jobs: with: node-version: "22" - - name: Wait for trusted exact-diff verdict + - name: Wait for trusted PR/base SHA verdict env: BASE_SHA: ${{ github.event.pull_request.base.sha }} GITHUB_TOKEN: ${{ github.token }} diff --git a/AGENTS.md b/AGENTS.md index 0c4dcc28a2b..beadf288b05 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -198,6 +198,8 @@ All hooks managed by [prek](https://prek.j178.dev/) (installed via `npm install` - Use existing repository vocabulary and name what a thing does. - Remove modifiers that do not distinguish a real current case. - Use one name for one concept across issues, code, workflows, checks, logs, tests, and docs. +- Apply [NemoClaw Technical English](CONTRIBUTING.md#nemoclaw-technical-english) to changed comments, test titles, PR text, changelog entries, Announcements, and agent guidance. +- During the 30-day changed-text pilot, treat language findings as suggestions unless ambiguity can change behavior, security, data safety, test meaning, or release meaning. Do not request unrelated language cleanup. - Do not turn one case into a system of categories or a new abstraction. - Do not add configuration, fallback, migration, compatibility, or extension layers without a current requirement. Name the current consumer and the test that protects the contract. - Report conclusions and evidence, not an analysis transcript. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6a8fc23ee74..2635111ef5a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,6 +62,107 @@ outcome, the smallest change, and how it was verified. Explore alternatives only change behavior, security, data safety, or a supported contract. Once the smallest safe change is clear and testable, stop exploring and implement it. +### NemoClaw Technical English + +NemoClaw uses a technical-English profile based on +[ASD-STE100 Issue 9](https://www.asd-ste100.org/assets/files/ASD-STE100_ISSUE9.pdf). +The profile applies its plain-language principles to software engineering. +The project does not claim full ASD-STE100 compliance. + +Use repository terms, software identifiers, API names, and necessary domain terms as technical +nouns or technical verbs. Do not copy the ASD-STE100 dictionary or its examples into this +repository. Use the rules and examples below as the NemoClaw source of truth. + +#### Writing rules + +1. Use one term for one concept. Do not use synonyms to add variety. +2. Use a term with one meaning in a given context. +3. Use the shortest familiar term that preserves the technical meaning. +4. Name the actor when known. Use passive voice only when the actor is unknown or does not matter. +5. Put one instruction in each sentence. Split actions that occur at different times. +6. Keep instructions at 20 words or fewer when possible. Keep descriptions at 25 words or fewer when possible. +7. State a condition before the action that depends on it. +8. Use `must` for a requirement, `may` for permission, `can` for capability, and `should` for a recommendation. +9. Name the object of relative terms such as `current`, `latest`, `previous`, and `next`. +10. Replace `ready`, `clean`, `safe`, `small`, and similar judgments with the condition that makes them true. +11. Remove `just`, `simply`, `obviously`, `clearly`, `easy`, `robust`, and other words that do not change the meaning. +12. Avoid an idiom or phrasal verb that can have more than one meaning. Use a direct technical term when one is available. +13. Use a vertical list for three or more conditions, actions, or results. +14. In a code comment, explain a constraint, invariant, or reason that the code does not show. Do not restate the code. + +Sentence lengths are review targets, not mechanical limits. Do not make a sentence less accurate to +meet a word count. Quoted user text, external text, code, identifiers, commands, URLs, and generated +content are outside the word and sentence rules. + +#### Project word list + +Use these terms consistently: + +| Term | Meaning | Avoid | +|---|---|---| +| PR SHA | The PR-branch commit that the evidence covers. Use its short SHA in reports. Use the full SHA only when a command or API requires it. | relative revision terms without a SHA | +| base SHA | The target-branch commit used to evaluate the PR. | current base without a SHA | +| required check | A named GitHub check required by repository policy. | CI gate when no check is named | +| passing | A command exited with status 0, or a check concluded with `SUCCESS`. | green when the result is not named | +| approval-ready | All product, contributor, CI, merge-state, review, and test gates pass. | ready, good to go | +| blocked | A named decision, dependency, access problem, or input prevents progress. | stuck, cannot proceed without a reason | +| advisory | Information that does not change a gate, approval, or merge state. | warning when no risk requires attention | +| changed text | Explanatory text added or modified by the diff. | the whole file when unchanged text is out of scope | +| user-visible change | A change to a command, output, configuration, workflow, or supported behavior. | improvement without the changed behavior | +| release entry | The dated `docs/changelog/YYYY-MM-DD.mdx` record created before the tag. | release notes when the dated entry is intended | +| Announcement | The post-tag release communication. | release entry | + +Use a different term only when it identifies a different concept. Define that difference where the +term first appears. + +#### Rewrite examples + +These examples use recurring NemoClaw concepts. They show the required level of precision. + +| Surface | Avoid | Use | +|---|---|---| +| Code comment | `// Handle edge case.` | `// GitHub omits headRepository after a fork is deleted.` | +| Code comment | `// This is needed for safety.` | `// Reject private IP targets to prevent SSRF.` | +| Code comment | `// Keep this in sync.` | `// This list must match requiredChecks in check-gates.ts.` | +| Code comment | `// Use the latest state.` | `// Read headRefOid again before approval.` | +| Code comment | `// Work around a GitHub issue.` | `// GitHub can return no PR association for a deleted fork repository.` | +| Test title | `handles invalid config correctly` | `rejects a config that has no provider` | +| Test title | `works after retry` | `retries evidence download after child cancellation` | +| Test title | `covers edge cases (#1234)` | `rejects an empty policy name (#1234)` | +| Test title | `fixes issue #1234` | `preserves credentials when a sandbox rebuilds (#1234)` | +| Test title | `does the right thing for forks` | `does not expose repository secrets to fork code` | +| PR discussion | `This seems brittle.` | `This catch block hides EACCES. Callers then treat denied access as missing state.` | +| PR discussion | `Can we clean this up?` | `These two parsers implement the same policy. Use parsePolicy in both call sites.` | +| PR discussion | `Make this more robust.` | `Return a typed access error for EACCES and add a denial-path test.` | +| PR discussion | `This is a small change.` | `This change updates one parser and does not change the policy schema.` | +| PR discussion | `The PR is ready.` | `Required checks pass on 1a2b3c4, and GitHub reports MERGEABLE.` | +| Announcement | `Improved onboarding.` | `Onboarding now resumes after provider selection fails.` | +| Announcement | `Added more robust E2E handling.` | `The PR gate now retries evidence download after a child run is cancelled.` | +| Release entry | `Fixed various issues.` | `The CLI now rejects a provider configuration that has no endpoint.` | +| Release entry | `Better error handling.` | `The CLI now reports the provider authentication error without a stack trace.` | +| Procedure | `Refresh and rerun as needed.` | `Fetch origin/main. Rerun the gate after the PR SHA changes.` | + +#### Changed-text pilot + +The pilot begins when the PR that adds this section merges. It ends 30 calendar days later. +Use the GitHub merge time as the time source. During the pilot, apply the profile to changed text only: + +- Added or modified code comments. +- Added or modified test titles. +- PR descriptions and new review comments. +- New or modified changelog entries and Announcements. +- Added or modified contributor guidance, agent guidance, and user documentation. + +Do not request unrelated language cleanup in a feature, fix, or release PR. Put existing language +debt in a focused follow-up PR. + +During the pilot, language findings are non-blocking suggestions unless the ambiguity can change +behavior, security, data safety, test meaning, or release meaning. A blocking comment must name that +effect. A suggestion should include a proposed rewrite. + +At the end of the pilot, maintainers should review accepted suggestions, rejected suggestions, and +false positives before they add an automated check or make a language rule blocking. + ## Before You Open an Issue Open an issue when you encounter one of the following situations. @@ -298,9 +399,14 @@ existing contract. Write `describe` and `it` titles so the Vitest tree reads as behavioral documentation. Start test titles with behavior or context rather than issue numbers, flags, or scenario labels, and put local issue references in a final suffix such as `(#1234)`. Prefer -`it("reticulates splines correctly (#1234)")` over +`it("reticulates splines for valid control points (#1234)")` over `it("#1234 fixes spline reticulation")`. +Apply the [NemoClaw Technical English](#nemoclaw-technical-english) profile to each added or modified +test title. During the changed-text pilot, the title checker continues to enforce objective title +shape only. A language finding can block when ambiguity changes the test meaning. Other findings are +suggestions. Reviewers must not request unrelated title cleanup. + Run `npm run test:spec` to render the suite with Vitest's hierarchical tree reporter. Run `npm run test:titles:check` to enforce the objective title-shape conventions without attempting to lint subjective English grammar. diff --git a/ci/full-e2e-cold-path-calibration.json b/ci/full-e2e-cold-path-calibration.json index cbb0e4aa912..c6786792b8b 100644 --- a/ci/full-e2e-cold-path-calibration.json +++ b/ci/full-e2e-cold-path-calibration.json @@ -1,5 +1,5 @@ { - "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0\n\nFive independent workflow_dispatch full-e2e samples of current-main baseline 1a74b8348c2182fbf806726341186f008444e28e with the phase-measurement changes at 4544d07c8bfd500c3b64a74380ef5cd0e62089f5. All runs completed install, BuildKit prebuild without fallback, the silence assertion, and the expected first turn. Each baseline budget is derived independently so phase caps diagnose regressions; they are not portions that must sum to the root-start budget. The separate validation adjustment records four existing full-e2e job observations from three descendant heads after f62c278bd737f4f47be2e85436f65b270d5b4280 added the reviewed WeChat runtime graph to the supported sandbox image. The relevant image-building inputs listed in imageInputPaths were unchanged through 2adc8481ff3053a5a7be37d130cb183e222934ff; repository tests enforce that ancestry and unchanged-input boundary. All four jobs completed install, BuildKit prebuild without fallback, the silence assertion, and the expected first turn; two exceeded the prior root-start and/or sandbox-phase caps. The conclusion fields record the full-e2e job conclusions, not aggregate workflow conclusions. The adjustment raises only those two caps from the maximum observed value plus the existing headroom, rounded up to one second. This is a bounded post-change validation adjustment, not a replacement five-run exact-head calibration. Retire it by replacing the baseline and removing validationAdjustment after five successful full-e2e samples from one exact head that contains the image change.", + "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0\n\nFive independent workflow_dispatch full-e2e samples of current-main baseline 1a74b8348c2182fbf806726341186f008444e28e with the phase-measurement changes at 4544d07c8bfd500c3b64a74380ef5cd0e62089f5. All runs completed install, BuildKit prebuild without fallback, the silence assertion, and the expected first turn. Each baseline budget is derived independently so phase caps diagnose regressions; they are not portions that must sum to the root-start budget. The separate validation adjustment records four existing full-e2e job observations from three descendant heads after f62c278bd737f4f47be2e85436f65b270d5b4280 added the reviewed WeChat runtime graph to the supported sandbox image. The relevant image-building inputs listed in imageInputPaths were unchanged through 2adc8481ff3053a5a7be37d130cb183e222934ff; repository tests enforce that ancestry and unchanged-input boundary. All four jobs completed install, BuildKit prebuild without fallback, the silence assertion, and the expected first turn; two exceeded the prior root-start and/or sandbox-phase caps. The conclusion fields record the full-e2e job conclusions, not aggregate workflow conclusions. The adjustment raises only those two caps from the maximum observed value plus the existing headroom, rounded up to one second. This is a bounded post-change validation adjustment, not a replacement five-run single-SHA calibration. Retire it by replacing the baseline and removing validationAdjustment after five successful full-e2e samples from one SHA that contains the image change.", "schemaVersion": 1, "calibratedAt": "2026-07-13", "baselineMainSha": "1a74b8348c2182fbf806726341186f008444e28e", @@ -206,7 +206,7 @@ "roundUpMs": 1000 }, "retirement": { - "trigger": "successful-exact-head-calibration", + "trigger": "successful-single-sha-calibration", "minimumSampleCount": 5, "allSamplesSameHead": true, "imageChangeMustBeAncestor": true, diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 578df760285..47cb9d7a9b4 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -138,7 +138,7 @@ The contributor who changes any platform asset owns the corresponding pin update First commit the updated assets, starter-prompt behavior, and related tests without changing the existing URLs, `promptAssetRevision`, or pinned SHA-256 values. Then use that commit's SHA in every platform-asset URL, update `promptAssetRevision` and every pinned SHA-256 value in `test/starter-prompt-docs.test.ts`, and commit the repin as one atomic follow-up. Never mix asset URLs from different revisions or point an asset URL at a commit that predates its content. -The exact-revision test compares each local asset byte-for-byte with its Git blob at `promptAssetRevision`, so the intermediate content commit intentionally fails until the atomic repin follow-up points every URL, revision, and digest at that content commit. +The asset test compares each local file byte-for-byte with its Git blob at `promptAssetRevision`, so the intermediate content commit intentionally fails until the atomic repin follow-up points every URL, revision, and digest at that content commit. Updating only a local digest does not prove what the pinned revision contains. Downstream consumers can pin the source with a raw URL such as `https://raw.githubusercontent.com/NVIDIA/NemoClaw//docs/resources/starter-prompt.md`. @@ -254,6 +254,9 @@ position: 1 ## Style Guide Write like you are explaining something to a colleague. Be direct, specific, and concise. +Apply the [NemoClaw Technical English profile](../CONTRIBUTING.md#nemoclaw-technical-english) to changed prose. +The profile defines shared terminology, sentence rules, rewrite examples, and the changed-text pilot. +The rules below add documentation-specific voice, formatting, and product-name conventions. ### Voice and Tone diff --git a/docs/get-started/quickstart.mdx b/docs/get-started/quickstart.mdx index 9e2627ebc7a..4958c822076 100644 --- a/docs/get-started/quickstart.mdx +++ b/docs/get-started/quickstart.mdx @@ -197,7 +197,7 @@ Use these details when your first-run path needs more control. DGX Station express install explicitly selects `nemotron-3-ultra-550b-a55b` instead of the Station managed-vLLM profile default, `deepseek-v4-flash`, and discloses the approximately `352 GB` model download before confirmation. If the host does not meet the [Station prerequisites](prerequisites/dgx-station-preparation), the installer stops before host preparation. Before Station host preparation begins, the installer stores the accepted Express recipe in owner-only local state. - If preparation requires a reboot or a new login, run the printed command to restore the exact revision, agent, model, sandbox, policy tier, and gateway, dashboard, and vLLM ports without repeating the Express prompt. + If preparation requires a reboot or a new login, run the printed command to restore the recorded revision, agent, model, sandbox, policy tier, and gateway, dashboard, and vLLM ports without repeating the Express prompt. Generic Ubuntu preparation can change pinned packages and then exits with status `10`; reboot, sign in, and run that printed command to resume. Stock DGX OS validation checks the factory stack in place without installing packages, restarting services, or rewriting the Docker runtime. One physical DGX OS `7.5.0` GB300 validation completed, but Station remains Deferred pending repeat clean-host qualification and CI coverage. diff --git a/docs/resources/prompt-assets/dgx-station.md b/docs/resources/prompt-assets/dgx-station.md index ce873ebbd21..7c4015a0d58 100644 --- a/docs/resources/prompt-assets/dgx-station.md +++ b/docs/resources/prompt-assets/dgx-station.md @@ -15,13 +15,13 @@ The installer provides these Station Express model choices: 1. The ordinary installer defaults to `nemotron-3-ultra-550b-a55b`, served as `nvidia/nemotron-3-ultra-550b-a55b`. 2. The explicit `--station-deepseek` flag selects `deepseek-v4-flash`, served as `deepseek-ai/DeepSeek-V4-Flash`. -Both choices use the same Station detection, host-preparation, consent, suggested-policy, default-sandbox, and exact-revision resume flow. +Both choices use the same Station detection, host-preparation, consent, suggested-policy, default-sandbox, and revision resume flow. Before asking for consent, explain all of these boundaries: - On generic Ubuntu, Station Express may install or change the pinned NVIDIA open driver, Docker with Buildx, NVIDIA Container Toolkit, and the reviewed factory `dkms` transition. On qualified factory images, the installer follows its bounded validation and repair path instead of replacing the factory stack. - Official Station preparation may add the trusted local account to the `docker` group, which grants root-equivalent control and is suitable only for a trusted single-user development host. -- Official Station preparation may require an operator-controlled reboot and resumes only with the exact accepted NemoClaw revision. +- Official Station preparation may require an operator-controlled reboot and resumes only with the accepted NemoClaw revision. - Nemotron Ultra Express discloses an approximately `352 GB` model download. DeepSeek Express downloads its pinned vLLM container and model data. Both require enough space on the model-cache filesystem and Docker storage. - DGX Station remains an evaluation path with deferred end-to-end validation on physical hardware, so startup may still fail after readiness checks. @@ -43,7 +43,7 @@ If a Station Express model is selected: - Run the installer only in a secure interactive terminal. If the coding-agent UI cannot keep the installer prompts visible and accept the user's response, stop before installation. - Let the installer present its third-party-software notice and complete Express summary. Keep each official confirmation visible, wait for the user's response, and do not pre-answer or suppress it. - Do not pass `--force-station-install` unless the installer rejects release metadata on genuine Station GB300 hardware and the user separately chooses the documented temporary override. -- Follow the installer's printed exact-revision command after a required reboot or login transition. +- Follow the command that the installer prints after a required reboot or login transition. If Station Express is declined, continue with the normal provider selection. Offer existing vLLM when a ready server is detected, managed vLLM, supported local Ollama, and every hosted or compatible provider supported by the selected agent. diff --git a/docs/resources/starter-prompt.md b/docs/resources/starter-prompt.md index 945b3882e0e..6311fbb9f2a 100644 --- a/docs/resources/starter-prompt.md +++ b/docs/resources/starter-prompt.md @@ -68,9 +68,9 @@ Use `NEMOCLAW_AGENT=langchain-deepagents-code` or `nemo-deepagents onboard` for After the readiness check, load exactly one matching instruction asset before provider selection: -- Confirmed DGX Spark: [DGX Spark Express instructions](https://raw.githubusercontent.com/NVIDIA/NemoClaw/f814db4f7708ecf9ab054fcee449f11c95076dfd/docs/resources/prompt-assets/dgx-spark.md). -- Confirmed DGX Station: [DGX Station installation instructions](https://raw.githubusercontent.com/NVIDIA/NemoClaw/f814db4f7708ecf9ab054fcee449f11c95076dfd/docs/resources/prompt-assets/dgx-station.md). -- Officially detected Windows WSL: [Windows WSL Express instructions](https://raw.githubusercontent.com/NVIDIA/NemoClaw/f814db4f7708ecf9ab054fcee449f11c95076dfd/docs/resources/prompt-assets/windows-wsl.md). +- Confirmed DGX Spark: [DGX Spark Express instructions](https://raw.githubusercontent.com/NVIDIA/NemoClaw/c718a78c5794574a98fdd885d94466c3b6794153/docs/resources/prompt-assets/dgx-spark.md). +- Confirmed DGX Station: [DGX Station installation instructions](https://raw.githubusercontent.com/NVIDIA/NemoClaw/c718a78c5794574a98fdd885d94466c3b6794153/docs/resources/prompt-assets/dgx-station.md). +- Officially detected Windows WSL: [Windows WSL Express instructions](https://raw.githubusercontent.com/NVIDIA/NemoClaw/c718a78c5794574a98fdd885d94466c3b6794153/docs/resources/prompt-assets/windows-wsl.md). Read the matching raw Markdown file completely and follow it before continuing. Do not load a platform asset for any other computer. diff --git a/docs/security/openclaw-2026.6.10-dependency-review.md b/docs/security/openclaw-2026.6.10-dependency-review.md index da1d49bb2f2..b736d8cf818 100644 --- a/docs/security/openclaw-2026.6.10-dependency-review.md +++ b/docs/security/openclaw-2026.6.10-dependency-review.md @@ -12,7 +12,7 @@ Scope: NemoClaw runtime pin `openclaw@2026.6.10`, runtime helper pin `@zed-indus The integrity-pin regression is split by boundary: `test/openclaw-integrity-pin-base.test.ts` covers protected base provenance and archive filename confinement; `test/openclaw-integrity-pin-contract.test.ts` covers review-note and manifest pin alignment plus production build-argument ownership; and `test/openclaw-integrity-pin-plugin-install.test.ts` covers optional, core, and Codex ACP registry/download verification plus reviewed local-archive installation. -Issue #5591 is the dependency-update umbrella, and its proposed design has three literal clauses. "Latest stable version of Hermes" is satisfied by merged PR #5594 (`hermes-agent==2026.6.19`); "Latest version of OpenShell" is satisfied by merged PR #5596 (`openshell==0.0.71`); and "Latest stable version of OpenClaw" is the clause owned by this PR. For that OpenClaw clause, the repository pins the reviewed non-prerelease `openclaw@2026.6.10` artifact and its plugin SRIs, while the integrity-pin suites, `test/openclaw-dependency-review.test.ts`, and the E2E matrix for the PR head commit provide the acceptance evidence. This PR references rather than closes #5591 because the issue tracks the coordinated dependency set and release, not only the OpenClaw slice. +Issue #5591 is the dependency-update umbrella, and its proposed design has three literal clauses. "Latest stable version of Hermes" is satisfied by merged PR #5594 (`hermes-agent==2026.6.19`); "Latest version of OpenShell" is satisfied by merged PR #5596 (`openshell==0.0.71`); and "Latest stable version of OpenClaw" is the clause owned by this PR. For that OpenClaw clause, the repository pins the reviewed non-prerelease `openclaw@2026.6.10` artifact and its plugin SRIs, while the integrity-pin suites, `test/openclaw-dependency-review.test.ts`, and the E2E matrix for the PR SHA provide the acceptance evidence. This PR references rather than closes #5591 because the issue tracks the coordinated dependency set and release, not only the OpenClaw slice. ## Package Identity @@ -158,7 +158,7 @@ The OpenClaw 2026.6.10 compiled-dist patches are localized compatibility patches Invalid state: a real installed `openclaw@2026.6.10` dist changes semantics while fixture-compatible recognizers still pass. Source boundary: the installed OpenClaw generated `dist` files, the Dockerfile fetch-guard patch block, `scripts/patch-openclaw-chat-send.mts`, `scripts/patch-openclaw-device-self-approval.mts`, and `scripts/patch-openclaw-issue-4434-diagnostics.mts`. Source-fix constraint: upstream OpenClaw should own permanent fixes; NemoClaw patches must stay version-scoped, fail closed on unknown shapes, and be removed when upstream ships reviewed behavior. Regression tests: `test/fetch-guard-patch-regression.test.ts`, `test/openclaw-chat-send-patch.test.ts`, `test/openclaw-device-self-approval-patch.test.ts`, and `test/openclaw-issue-4434-diagnostics-patch.test.ts` execute patched fixtures for the reviewed shapes. `test/openclaw-real-patched-dist-harness.test.ts` is the checked-in real-package harness: when run with `NEMOCLAW_REAL_OPENCLAW_DIST_HARNESS=1`, it downloads the reviewed tarball URL, verifies the committed SRI, extracts the actual `openclaw@2026.6.10` dist, applies the Dockerfile patch block, runs and audits all three focused patch scripts, and verifies Patch 2, Patch 2b, Patch 4, Patch 6, Patch 7, Patch 8, chat-send/get-reply/followup-runner markers, and the #4434 assistant-error formatter marker. For Patch 8 it also verifies the exact compiled session producer, dispatcher, device handler, canonical authz-resolver, and fixed-version journal linkage; invokes the exported real handler to deny shared-auth and cross-device requests and rotate the matching device token; retains successful concurrent-approval proof; and injects both one-sided publication directions plus a rejected rename to verify bounded rollback and fresh-process recovery without losing unrelated pending or paired/token entries. The harness remains explicit opt-in for PR and local proof. Trusted main CI sets `NEMOCLAW_REAL_OPENCLAW_DIST_HARNESS=1` and materializes the reviewed archive automatically with a bounded download retry and a 12-minute job budget. PR CI intentionally does not treat PR-authored harness code as its own security gate. -This source-package proof is not a substitute for focused nightly E2E proof of affected runtime workflows, image builds for the current PR head commit, or final full E2E proof before merge. +This source-package proof is not a substitute for focused nightly E2E proof of affected runtime workflows, image builds for the PR SHA, or final full E2E proof before merge. Removal condition: delete the localized patches and harness when OpenClaw ships the reviewed behavior; if NemoClaw keeps carrying the patches beyond this bump, retain both the archive harness and built-image runtime gates. #### OpenClaw Patch Source-of-Truth Table @@ -273,7 +273,7 @@ No real Microsoft Teams tenant proof is included in this PR. The work remains tr - [x] OpenClaw real patched-dist harness: main CI runs it automatically from trusted merged code, while `NEMOCLAW_REAL_OPENCLAW_DIST_HARNESS=1 npx vitest run --project integration test/openclaw-real-patched-dist-harness.test.ts` remains the explicit PR/local proof. It is intentionally not a PR check because PR-authored harness code cannot serve as its own trusted security gate. It materializes the reviewed tarball, verifies SRI, applies the Dockerfile patch block, and audits chat-send/get-reply/followup-runner markers. - Before merge, keep CI image builds for the current PR head commit plus focused/full E2E workflow proof as the runtime evidence boundary. + Before merge, keep CI image builds for the PR SHA plus focused/full E2E workflow proof as the runtime evidence boundary. - [x] Issue #4434 full live acceptance: `scripts/patch-openclaw-issue-4434-diagnostics.mts` enriches the reviewed OpenClaw formatter for sandbox-only `fetch failed` and `LLM request timed out.` errors, and the migrated `test/e2e/live/issue-4434-tui-unreachable-inference.test.ts` guard requires HTTP/cause, gateway/upstream layer attribution, and a recovery hint. - [x] Future #4434 upstream-removal trigger: on the next relevant OpenClaw bump, rerun `test/openclaw-issue-4434-diagnostics-patch.test.ts` and the real patched-dist harness. If upstream emits equivalent fields directly, remove the shim while preserving full live assertions. diff --git a/docs/security/openshell-0.0.85-migration-review.md b/docs/security/openshell-0.0.85-migration-review.md index 8cf002debe2..dfaabcddccd 100644 --- a/docs/security/openshell-0.0.85-migration-review.md +++ b/docs/security/openshell-0.0.85-migration-review.md @@ -9,7 +9,7 @@ OpenShell published stable tag `v0.0.85` at verified commit [`3dee5570a46076a57a3b056f35f35ebc0861ac85`](https://github.com/NVIDIA/OpenShell/commit/3dee5570a46076a57a3b056f35f35ebc0861ac85). This review binds NemoClaw's `0.0.85` selectors to that exact source and its published artifacts. It is not blanket approval for every upstream platform or -driver: exact-head NemoClaw CI/advisor review, the supported-platform proofs, and +driver: NemoClaw CI/advisor review for the PR SHA, the supported-platform proofs, and the physical DGX Spark credential-substitution proof below remain separate merge and issue-closure gates. @@ -560,7 +560,7 @@ artifacts and scanned its artifacts for the whole generated-secret prefix. The stable-source review retains the upstream `expired_retained_generation_does_not_resolve` unit. The default stable job now binds the target to tag commit `3dee5570`, the extracted release binaries, and the -immutable supervisor index; its final exact-head result remains a merge gate. +immutable supervisor index; its final result for the PR SHA remains a merge gate. ### v0.0.82 to v0.0.83 @@ -644,18 +644,18 @@ Commits: `80293213`, `392ad639`, `b4be33e5`, `21aaa895`, `3dee5570`. |---|---|---|---|---| | `OS85-01` | Critical | All stable selectors, archives, checksums, binaries, and the supervisor image could identify different builds. | Pin one published tag; verify producer run, signatures/attestations, release hashes, extracted binaries, component versions, OCI index and child manifests; reject archive traversal, links, devices, duplicates, or unexpected members. | Closed for dependency selection: stable tag `3dee5570`, producer run 29507522595, three manifest digests, eight consumed archive digests and SLSA attestations, extracted Linux binary identities, and the immutable multiarch supervisor index are recorded and enforced. | | `OS85-02` | Critical | `mcp status` can be honest while the affected Spark still cannot initialize resolver/CA state or perform a credential-bearing request. | Physical Docker 27 DGX Spark: register credential, require status success, load tools, complete a real MCP tool call, and prove the literal placeholder never reaches upstream. | Blocked on assigned hardware proof. | -| `OS85-03` | High | `src/lib/actions/sandbox/exec.ts`, command dispatch, docs, and internal wrappers encode the old newline rejection. | Remove the obsolete public rejection and newline-only wrappers; prove byte-exact LF, CR, CRLF, quotes, and heredoc argv; retain NUL plus multiline workdir/environment rejection. | Source and internal-wrapper migration complete; stable exact-head runtime proof remains a merge gate. | -| `OS85-04` | High | OpenShell child launch now clears the complete capability bounding set. Hosts without `CAP_SETPCAP` may fail if their runtime does not pre-clear it. | Prove entrypoint, exec, and connect launch with `CapBnd=0` on Linux Docker, DGX Spark arm64, macOS Docker Desktop/Colima, WSL, and Colossus; update NemoClaw's #3280 caveat only from runtime evidence. | The stable release proof inspects the actual entrypoint, exec, and forced-TTY connect children for full `CapBnd=0`; exact-head Linux execution and every other platform remain open. | -| `OS85-05` | High | Versioned credential placeholders and the eight-generation window change long-running MCP behavior. | Regenerate the exact-version child-visible manifest; reject reserved `v_` names; test more than eight rotations, removed keys, detach, restart/rebuild, fresh exec revision, expiry, and literal-placeholder scans. | The exact `v0.0.85` child-visible manifest is the production authority and the generation-window proof is workflow-mandatory; stable exact-head live execution remains a merge gate. | +| `OS85-03` | High | `src/lib/actions/sandbox/exec.ts`, command dispatch, docs, and internal wrappers encode the old newline rejection. | Remove the obsolete public rejection and newline-only wrappers; prove byte-exact LF, CR, CRLF, quotes, and heredoc argv; retain NUL plus multiline workdir/environment rejection. | Source and internal-wrapper migration complete; stable runtime proof for the PR SHA remains a merge gate. | +| `OS85-04` | High | OpenShell child launch now clears the complete capability bounding set. Hosts without `CAP_SETPCAP` may fail if their runtime does not pre-clear it. | Prove entrypoint, exec, and connect launch with `CapBnd=0` on Linux Docker, DGX Spark arm64, macOS Docker Desktop/Colima, WSL, and Colossus; update NemoClaw's #3280 caveat only from runtime evidence. | The stable release proof inspects the actual entrypoint, exec, and forced-TTY connect children for full `CapBnd=0`; Linux execution for the PR SHA and every other platform remain open. | +| `OS85-05` | High | Versioned credential placeholders and the eight-generation window change long-running MCP behavior. | Regenerate the exact-version child-visible manifest; reject reserved `v_` names; test more than eight rotations, removed keys, detach, restart/rebuild, fresh exec revision, expiry, and literal-placeholder scans. | The exact `v0.0.85` child-visible manifest is the production authority and the generation-window proof is workflow-mandatory; stable live execution for the PR SHA remains a merge gate. | | `OS85-06` | High | Initial policy acknowledgement and ordered retry can make the active gateway status lag enforcement. | Test initial LOADED/FAILED, hot update, retry outage/recovery, restart, exact version/hash re-read, and ordered drain. | The stable release proof covers hot-update LOADED identity plus restart initial acknowledgement and exact version/hash recovery. Initial FAILED and isolated report outage/ordered drain remain an open runtime gate. | | `OS85-07` | High | Sequential nft setup can leave an incomplete policy-accept ruleset after a required command fails; Docker setup treats the error as nonfatal. | Inject each required failure; inspect IPv4/IPv6 TCP/UDP rules and direct-bypass negatives on Linux x86 and Spark arm64; verify restart and teardown. | The stable release proof inspects the live policy-accept chain and all four required rejects before/after restart, and probes controlled IPv4 TCP/UDP listeners. Required-command fault injection, routed IPv6 behavior, Spark arm64, and physical teardown remain an open security gate. | | `OS85-08` | High | The supervisor image moves from one scratch binary to a 29-package Alpine filesystem resolved from a mutable base and unpinned APK names. NemoClaw's Docker path downloads it but executes only the extracted binary. | Retain exact per-arch package/version/license and file inventories; scan vulnerabilities; verify modes, multiarch manifests, base/package identities, source labels, OCI provenance, and extraction-only behavior; preserve an explicit digest. | The final stable index, child manifests, configs, and candidate-equivalent package/file inventory are recorded. The upstream image still has no OCI attestation, SBOM, source labels, or reproducible base/package inputs; NemoClaw enforces the immutable index and explicitly retains that provenance limitation. | -| `OS85-09` | Medium-high | Normalized selected-driver config can change the effective Docker gateway even when the TOML text is unchanged. | Parse the final rendered TOML with the final binary; prove loopback/bridge listeners, JWT/mTLS, restart, persisted state, and legacy gateway upgrade. | The stable release proof binds the actual rendered Docker TOML to the running release gateway, loopback and Linux bridge listeners, mTLS/JWT mounts and relay access, host gateway restart, persisted sandbox state, and rebuild. The exact-head result, legacy-gateway upgrade, and non-Linux/host-gateway platforms remain open. | -| `OS85-10` | Medium-high | Supervisor TLS identity variables are no longer child environment. Stale tests/comments can normalize a credential leak. | Assert absence from entrypoint, exec, and connect children and update the source-of-truth rationale. | Hermes and Deep Agents now reject all three variables; the stable entrypoint, exec, and connect probes require their absence, with exact-head execution pending. | -| `OS85-11` | Medium-high | Live `/proc//exe` identity changes replacement-time policy behavior. | Prove old process survives replacement and a new altered process at the same path is denied. | The stable release proof runs both processes against the real proxy and requires old=200 before/after replacement, distinct live/path hashes, and new=403; exact-head runtime result pending. | +| `OS85-09` | Medium-high | Normalized selected-driver config can change the effective Docker gateway even when the TOML text is unchanged. | Parse the final rendered TOML with the final binary; prove loopback/bridge listeners, JWT/mTLS, restart, persisted state, and legacy gateway upgrade. | The stable release proof binds the actual rendered Docker TOML to the running release gateway, loopback and Linux bridge listeners, mTLS/JWT mounts and relay access, host gateway restart, persisted sandbox state, and rebuild. The result for the PR SHA, legacy-gateway upgrade, and non-Linux/host-gateway platforms remain open. | +| `OS85-10` | Medium-high | Supervisor TLS identity variables are no longer child environment. Stale tests/comments can normalize a credential leak. | Assert absence from entrypoint, exec, and connect children and update the source-of-truth rationale. | Hermes and Deep Agents now reject all three variables; the stable entrypoint, exec, and connect probes require their absence, with execution for the PR SHA pending. | +| `OS85-11` | Medium-high | Live `/proc//exe` identity changes replacement-time policy behavior. | Prove old process survives replacement and a new altered process at the same path is denied. | The stable release proof runs both processes against the real proxy and requires old=200 before/after replacement, distinct live/path hashes, and new=403; the runtime result for the PR SHA is pending. | | `OS85-12` | Medium | OpenShell declares Docker 28.0+ while #6379 is on Docker 27 and NemoClaw marks DGX Spark tested. | Either validate and document a precise downstream exception from physical proof or raise the supported floor and preflight it. | Open product/platform decision. | -| `OS85-13` | Low | Mount parsing/SELinux changes could affect the test-only tmpfs path. | Rerun the EXDEV tmpfs fixture and retain production no-mount evidence. | The stable release proof injects only the reviewed tmpfs config, requires Docker's structured tmpfs representation plus `noexec`/01777 at runtime, requires an empty remount after graceful gateway restart with the same container/config/auth and retained durable state, and requires another fresh remount after rebuild. The wrapper is disabled outside the explicit proof lane and production still supplies no driver mounts. Exact-head, Podman, and enforcing-SELinux results remain open. | -| `OS85-14` | Low | Sanitized MCP tool names are newly present in logs. | Record the additive observability/privacy behavior; ensure no downstream parser assumes the old shape. | The stable release check requires the real `fake_echo` tool name and rejects argument/result canaries or an `arguments` field in JSON-RPC policy logs; exact-head runtime result pending. | +| `OS85-13` | Low | Mount parsing/SELinux changes could affect the test-only tmpfs path. | Rerun the EXDEV tmpfs fixture and retain production no-mount evidence. | The stable release proof injects only the reviewed tmpfs config, requires Docker's structured tmpfs representation plus `noexec`/01777 at runtime, requires an empty remount after graceful gateway restart with the same container/config/auth and retained durable state, and requires another fresh remount after rebuild. The wrapper is disabled outside the explicit proof lane and production still supplies no driver mounts. Results for the PR SHA, Podman, and enforcing-SELinux remain open. | +| `OS85-14` | Low | Sanitized MCP tool names are newly present in logs. | Record the additive observability/privacy behavior; ensure no downstream parser assumes the old shape. | The stable release check requires the real `fake_echo` tool name and rejects argument/result canaries or an `arguments` field in JSON-RPC policy logs; the runtime result for the PR SHA is pending. | | `OS85-15` | High | The installer-hash workflow executes its checker and parser from the PR base SHA. One PR cannot safely teach that trusted base about a new release and consume the release; using the head checker would let reviewed code define its own trust rules. | First land archive safety, normalized full-script template validation, and multi-release trust while selectors remain `0.0.72`; prove the old base rejects a new release and the new base permits only structured release-data changes; then land the exact `0.0.85` manifest identities before refreshing this selector PR. | Base trust landed in #7069. #6778 and #6779 established base-owned structured manifest and sandbox-map validation; #7069 added only the three exact `0.0.85` release identities while retaining `0.0.72` and `0.0.82`. This selector PR must be based on that trusted state and pass the checker without relying on its head copy. | | `OS85-16` | High | Capability clearing now depends on `capctl 0.2.4` and `bitflags 1.3.2`, but upstream notices are unchanged and the consumed binaries have no published SBOM or attestation covering this dependency graph. | Bind crate checksums and source identities to the stable lock and binaries; review the unsafe syscall boundary and advisories; update notices/licenses; retain a generated SBOM and provenance for every consumed binary. | The stable lock, crate checksums, source identities, licenses, unsafe boundary, current RustSec absence, and SLSA-bound archives are recorded. Upstream still publishes no binary SBOM and its unchanged notices omit the new graph; that limitation remains explicit rather than being presented as complete attribution. | | `OS85-17` | Medium | The VM driver bakes configurable UID/GID into prepared rootfs state, but its same-version cache key omits both values and can reuse stale passwd/group entries after configuration drift. | Keep NemoClaw's selected driver Docker-only. Before any VM path is supported, key prepared images by UID/GID or purge them and prove identity/ownership after change and restart. | Source-reviewed exclusion for the current Docker topology; VM configuration-churn compatibility is unproven. | @@ -804,9 +804,9 @@ restart/rebuild, and cleanup without a conditional skip or expected failure. hashes, supervisor digest, credential manifest, tests, and active docs select one coherent version. 5. Every concern-specific unit/integration proof above passes, followed by normal - repository checks and exact-head CI/advisor review. + repository checks and CI/advisor review for the PR SHA. 6. #7055 landed the controller-compatible legacy gateway evidence matrix. The - exact-head, non-skipped live matrix must still pass on Linux x86 Docker, + non-skipped live matrix for the PR SHA must still pass on Linux x86 Docker, macOS Docker Desktop/Colima, WSL, Colossus, and physical DGX Spark arm64; legacy gateway upgrade, restart, rollback, and teardown remain explicit phases. diff --git a/test/create-require-ratchet.test.ts b/test/create-require-ratchet.test.ts index 766d796eb8b..4942005514e 100644 --- a/test/create-require-ratchet.test.ts +++ b/test/create-require-ratchet.test.ts @@ -805,7 +805,7 @@ describe("base-trusted createRequire ratchet", () => { expect(failing.stderr).not.toContain("src/lib/fixture-property.ts"); }); - it("accepts only validated exact revisions from the pull-request event (#7056)", () => { + it("accepts only validated SHAs from the pull-request event (#7056)", () => { expect( extractPullRequestBaseSha(JSON.stringify({ pull_request: { base: { sha: BASE_SHA } } })), ).toBe(BASE_SHA); @@ -852,7 +852,7 @@ describe("base-trusted createRequire ratchet", () => { expect(called).toBe(false); }); - it("unshallows exact head and base histories before computing the merge base (#7056)", () => { + it("unshallows PR and base SHA histories before computing the merge base (#7056)", () => { const root = temporaryRepo(); const calls: Array<{ args: readonly string[]; timeoutMs: number | undefined }> = []; let available = false; @@ -983,7 +983,7 @@ describe("base-trusted createRequire ratchet", () => { ).toBeNull(); }); - it("fails closed when exact pull-request history cannot be fetched (#7056)", () => { + it("fails closed when pull-request SHA history cannot be fetched (#7056)", () => { const root = temporaryRepo(); const runner: GitRunner = (args) => args[0] === "rev-parse" diff --git a/test/e2e/README.md b/test/e2e/README.md index 2e5734a9f0a..a2ee216ca9a 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -11,7 +11,7 @@ before those targets run; local runners must provide it themselves. - `.github/workflows/e2e.yaml` is the scheduled, manually dispatchable, and selectively dispatched live target workflow. - `.github/workflows/pr-e2e-gate.yaml` runs as `E2E / PR Gate Controller` and - publishes the trusted exact-diff `E2E / PR Gate Coordination` check and the + publishes the trusted `E2E / PR Gate Coordination` check for the PR/base SHA pair and the native `E2E / PR Gate` job that mirrors coordination into the PR's required GitHub Actions check suite. - `.github/workflows/e2e-branch-validation.yaml` provisions Brev instances and @@ -91,7 +91,7 @@ The controller, coordination check, and required job deliberately use different names and report different parts of the lifecycle. `E2E / PR Gate Controller` reports whether the trusted controller handled its event. The controller publishes the internal custom check -`E2E / PR Gate Coordination` as its exact-diff verdict. +`E2E / PR Gate Coordination` as its verdict for the PR/base SHA pair. The default-branch `pull_request_target` path publishes the native GitHub Actions job named `E2E / PR Gate`. It checks out the controller at `github.workflow_sha`, validates that the PR still has the observed head and @@ -99,7 +99,7 @@ base, waits for the matching trusted coordination identity, and exits with its terminal verdict. It also writes that verdict and the trusted run link to the job log and keeps the job summary free of network-derived content. During rollout, the observer accepts the former custom-check name -`E2E / PR Gate` for the same exact-diff external identity so in-flight PRs do +`E2E / PR Gate` for the same PR/base SHA external identity so in-flight PRs do not lose their gate. A handled prerequisite-CI failure, selected E2E failure or timeout, stale @@ -116,12 +116,12 @@ closed, which prevents the native job from passing. On open, synchronization, reopen, transition out of draft, or base retarget, `.github/workflows/pr-e2e-gate.yaml` reserves `E2E / PR Gate Coordination` for -the exact PR head and base commits, including fork heads. The read-only native +the PR SHA and base SHA, including fork SHAs. The read-only native observer starts for every configured non-closed PR event; metadata-only edits -mirror the existing exact-diff coordination result instead of publishing a +mirror the existing PR/base SHA coordination result instead of publishing a skipped success. A base retarget fails any still-active earlier coordination result in that head's lineage, preserves completed audit history, and then -reserves the new exact-diff identity. The +reserves the new PR/base SHA identity. The `CI / Pull Request` run name binds its PR number, head SHA, base SHA, and gate eligibility so the trusted controller can authenticate the completed run even when a fork `workflow_run` payload omits pull-request metadata. The controller @@ -138,10 +138,10 @@ advisor output. Ordinary internal changes execute those focused selections. Gate initialization and CI coordination share one non-cancelling concurrency group for the head repository and branch. Before the controller creates or updates coordination for the current revision, it reads the live PR and -requires the event's exact head and base, including when PR CI failed. The -native observer performs the same live exact-revision check before waiting and +requires the event's PR SHA and base SHA, including when PR CI failed. The +native observer performs the same live PR/base SHA check before waiting and again before accepting a terminal verdict. This keeps a stale seed, completed -CI run, or observer from being applied to a newer exact diff. A completed CI +CI run, or observer from being applied to a newer PR/base SHA pair. A completed CI event for an older revision is handled without creating or updating the current revision's coordination check. If the older revision still has an in-progress coordination check, the @@ -163,13 +163,13 @@ An internal revision whose matched control-plane files are drawn only from the trusted controller and observer boundaries—`.github/workflows/pr-e2e-gate.yaml`, `tools/e2e/pr-e2e-gate.mts`, and `tools/e2e/pr-e2e-required.mts`—automatically dispatches those selected jobs. -Any other or mixed internal control-plane revision requires the exact-SHA -maintainer authorization below before credentialed execution begins. If no job +Any other or mixed internal control-plane revision requires maintainer authorization +for the PR SHA before credentialed execution begins. If no job or target is selected, coordination passes without an E2E run and the native required job mirrors that success. Before dispatch, the controller verifies that the live PR still matches the CI -run's exact head and base. It uses its own workflow commit when that commit is +run's PR SHA and base SHA. It uses its own workflow commit when that commit is still `main`. If `main` advanced, the controller accepts the current commit only when GitHub reports it as a descendant whose merge base is the workflow commit, the comparison contains fewer than 300 fully enumerated files, neither @@ -190,7 +190,7 @@ and runners, and only the trusted projection can configure credential-bearing typed-target jobs. Ordinary branch dispatch is not an acceptable substitute. The controller uses GitHub's returned run ID for waiting, evidence download, and completion, then revalidates that the PR is -still open with the live head, base, and exact-diff coordination identity before +still open with the PR SHA, base SHA, and coordination identity before recording a final result. The native observer revalidates the live revision before mirroring that terminal result. @@ -198,13 +198,13 @@ An internal revision whose control-plane matches include a file outside the trusted controller and observer boundaries leaves coordination in progress with `Maintainer authorization required to run E2E`. The native required job keeps waiting for the authorization flow. No selected job or target runs and no -repository secret is exposed. After reviewing the exact revision, a repository +repository secret is exposed. After reviewing the PR SHA, a repository maintainer or administrator chooses **Run workflow** on `main`, selects `run-control-plane`, and supplies the PR number, current 40-character head SHA as `expected_head_sha`, current 40-character base SHA as `expected_base_sha`, and a specific 10–500-character `review_reason`. The authorization requires the first workflow attempt and revalidates the actor's `maintain` or `admin` -permission, internal repository origin, open PR, exact head and base, risk +permission, internal repository origin, open PR, PR SHA and base SHA, risk plan, matching pending coordination state, compatible trusted controller commit, and final live revision. It then updates coordination to `Running E2E check(s)` and dispatches the selected jobs and targets in @@ -214,13 +214,13 @@ authorization title and leaves coordination in progress so a maintainer can correct the problem and launch a fresh first-attempt authorization. After a child is dispatched, a startup failure requests cancellation. Whether or not cancellation is confirmed, the controller completes coordination as -`Authorized E2E run requires reconciliation`; that exact-diff authorization +`Authorized E2E run requires reconciliation`; that authorization for the PR/base SHA pair cannot be retried because the child may still execute and a retry could start duplicate credential-bearing work. Inspect the linked run, then update the PR and run fresh CI before authorizing again. The native required job treats authorization and running titles as intermediate waiting states only while coordination remains in progress. It also keeps -polling when the current exact-diff coordination check is a completed failure +polling when the current PR/base SHA coordination check is a completed failure with a validated current-version retry marker, so it can follow a later validated replacement for the same unchanged head and base. That completed failure remains immutable and cannot be changed by manual authorization. A @@ -231,7 +231,7 @@ check carries a current-version retry reason: conclusively cancelled child, or `evidence-download` after a successful child whose evidence download failed, was cancelled, or was skipped. The trusted controller leaves the completed check as audit history, creates and validates a -new `in_progress` check with the same exact-diff external identity, and rebuilds +new `in_progress` check with the same PR/base SHA external identity, and rebuilds the deterministic plan before exposing a fresh authorization state. The controller and native observer select the highest check-run ID only when every older duplicate is a completed failure with a recognized versioned retry @@ -241,7 +241,7 @@ candidates fails closed. Selected-job product or assertion failures, evidence policy or integrity failures, schema or identity mismatches, traversal or provenance failures, reconciliation, controller errors, unknown states, and failures recorded before retry reasons existed -remain terminal for that exact diff. Fork approval failures are not retried by +remain terminal for that PR/base SHA pair. Fork approval failures are not retried by PR CI; follow the protected or manual skip path, or update the PR to create a new head. Update the PR and run fresh CI for the other terminal outcomes. The normal wait, evidence download, and finish path is the only path that can record @@ -288,12 +288,12 @@ of this exact workflow on `main`, at the trusted workflow SHA and with the `workflow_run` event. It requires exactly one approved review that names only the exact environment, then verifies that the recorded reviewer still has repository `maintain` or `admin` permission. The shared resolver revalidates -the open PR, repository origin, exact head and base SHAs, deterministic plan, +the open PR, repository origin, PR SHA and base SHA, deterministic plan, matching failed coordination check, and that the controller commit is either still `main` or has only a compatible safe descendant as described above. Immediately before -recording success, it reads the live PR again and requires the same exact head -and base. The result records the reviewer, bounded optional comment, validated +recording success, it reads the live PR again and requires the same PR SHA and +base SHA. The result records the reviewer, bounded optional comment, validated approval-run URL, plan hash, and jobs and targets that did not run. The successful skip coordination check is titled `Credentialed E2E skipped for fork PR — approved by @` and begins @@ -330,7 +330,7 @@ When the plan selects jobs or targets, coordination passes only when the E2E run succeeds and every expected job shard and target uploads one complete passing signal with no skips or pending tests. The native required job passes only after observing that -trusted success. For the current exact diff, every other dispatched outcome +trusted success. For the current PR/base SHA pair, every other dispatched outcome fails. A failed coordination result links the selected E2E run and up to 10 non-passing jobs, including up to three failed step names per job. If GitHub truncates the job listing or the controller cannot load it, the coordination @@ -346,7 +346,7 @@ coordination closed as `Evidence could not be verified` and leaves `E2E / PR Gate Controller` red so maintainers inspect that infrastructure failure. This download-only outcome records `evidence-download`, so a later successful eligible PR CI run can create -a fresh coordination check for the same exact diff. If the download step +a fresh coordination check for the same PR/base SHA pair. If the download step succeeds but signals are missing, duplicated, skipped, pending, or report a test failure, the controller has completed its work: it publishes the handled red PR verdict and remains green @@ -359,12 +359,12 @@ scorecard, including scorecard Slack reporting. Synchronizing, reopening, or closing an internal PR cancels its active E2E runs. A new dispatch also cancels the previous run. The previous controller -then completes the old exact-diff coordination check as cancelled when the PR +then completes the old PR/base SHA coordination check as cancelled when the PR revision moved or closed, or as failed when the current revision's selected E2E did not pass. Native observer concurrency cancels the old required-job run and starts a new one when a configured non-closed PR event identifies the current revision. Metadata-only edits restart the observer against the unchanged -exact-diff identity. +PR/base SHA identity. The controller does not read PR Review Advisor output, so model availability and recommendations are not part of merge authority. @@ -384,7 +384,7 @@ and runner or network incidents can still affect the signal, so maintainers should inspect the timing table before acting on a warning. For PRs, the unified PR Review Advisor builds and renders guidance from the -deterministic risk plan for the PR head commit and changed-file set. It +deterministic risk plan for the PR SHA and changed-file set. It recommends jobs for known regression families and includes `cloud-onboard` when changes affect onboard behavior, trace timing, scorecard analysis, budget configuration, or the unified E2E workflow. Compatibility schema fields may diff --git a/test/e2e/docs/README.md b/test/e2e/docs/README.md index f24249963b2..79ad4348532 100644 --- a/test/e2e/docs/README.md +++ b/test/e2e/docs/README.md @@ -116,7 +116,7 @@ test/e2e/ applies it independently without model output. - `.github/workflows/pr-e2e-gate.yaml` reserves the internal - `E2E / PR Gate Coordination` check on every exact PR head, including forks, + `E2E / PR Gate Coordination` check for every PR SHA, including forks, before `CI / Pull Request` completes. Its default-branch `pull_request_target` path also publishes the native GitHub Actions job named `E2E / PR Gate`. The read-only observer runs from `github.workflow_sha`, @@ -125,13 +125,13 @@ test/e2e/ job. Its summary is static, while the job log includes the validated trusted controller-run link. Authorization states remain pending while the maintainer decision is recorded. During rollout, the observer also accepts the former - `E2E / PR Gate` custom-check name for the same exact-diff identity. The + `E2E / PR Gate` custom-check name for the same PR/base SHA identity. The controller builds the risk plan from GitHub's complete file list. Internal revisions normally dispatch every selected job and verify each expected `risk-signal.json`; this remains automatic when their `e2e-control-plane` matches are drawn only from the trusted controller workflow and scripts. Other or mixed internal - control-plane revisions require a maintainer-authorized exact-SHA run; only + control-plane revisions require a maintainer-authorized run for the PR SHA; only its verified evidence can pass coordination. Risky forks retain the audited credentialed-E2E skip approval. See [NemoClaw E2E CI](../README.md) for the full lifecycle. @@ -147,7 +147,7 @@ test/e2e/ These per-target timing summaries are artifact evidence only. The Slack and GitHub scorecard timing comparison remains scoped to the dedicated `cloud-onboard` artifact. - PR E2E dispatches validate the PR head commit and controller metadata before + PR E2E dispatches validate the PR SHA and controller metadata before preparation, attach `test/e2e/risk-signal-reporter.ts` to live Vitest invocations, and suppress PR reporting and scorecards. The workflow boundary requires every selected job shard to upload its evidence artifact. diff --git a/test/e2e/live/issue-6194-tui-expect.ts b/test/e2e/live/issue-6194-tui-expect.ts index 5dcc424258c..0f513bbf6f8 100644 --- a/test/e2e/live/issue-6194-tui-expect.ts +++ b/test/e2e/live/issue-6194-tui-expect.ts @@ -260,7 +260,7 @@ expect { } } # The approval RPC assigns a policy revision before the sandbox loads it. -# Poll that exact revision through the read-only policy API until both its +# Poll that policy revision through the read-only policy API until both its # status and the active version prove convergence. Preserve every bounded # attempt so timeout and failed-revision diagnostics remain reviewable. set policyStatusOutput "ISSUE6194_APPROVED_POLICY_VERSION=$approvedPolicyVersion\\n" diff --git a/test/maintainer-skills-policy.test.ts b/test/maintainer-skills-policy.test.ts index 47b97b5b774..72b28e10416 100644 --- a/test/maintainer-skills-policy.test.ts +++ b/test/maintainer-skills-policy.test.ts @@ -83,7 +83,7 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(release).toContain('--event push --commit "$RELEASE_SHA"'); expect(release).toContain("Expected exactly one release-latest-tag push run"); expect(morning).toContain("post-tag housekeeping was interrupted"); - expect(priorities).toContain("automatically carry stragglers to the next patch"); + expect(priorities).toContain("Move open items to the next patch label"); expect(priorities).toContain("delete the released label"); expect(policy).toContain("automatically move every open straggler to the next patch label"); expect(policy).toContain("delete the released version label"); @@ -160,7 +160,7 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(evening).toContain("tag the confirmed release commit with `vX.Y.Z`"); expect(evening).not.toContain("tag `main`"); expect(dailyFlow).toContain("freeze the candidate SHA and review every E2E test"); - expect(priorities).toContain("collect the E2E evidence or itemized maintainer exceptions"); + expect(priorities).toContain("Record the release SHA and required E2E evidence"); }); it("runs release-prep docs before generating the final release plan", () => { @@ -196,7 +196,7 @@ describe("maintainer skills follow canonical workflow policy", () => { expect(releaseNotes).toContain("does not replace or create that canonical entry"); expect(policy).toContain("Run `/nemoclaw-contributor-update-docs for vX.Y.Z`"); expect(policy).toContain("The pre-tag release-note docs PR must create or update"); - expect(priorities).toContain("pre-tag release-note docs PR containing"); + expect(priorities).toContain("the pre-tag changelog PR contains"); expect(skillsGuide).toContain("create the canonical `docs/changelog/YYYY-MM-DD.mdx` entry"); expect(agents).toContain("a PR that updates ordinary pages without the dated changelog entry"); expect(docsAgents).toContain("Every pre-tag release-note docs PR must create or update"); @@ -208,8 +208,8 @@ describe("maintainer skills follow canonical workflow policy", () => { const sweep = read(".agents/skills/nemoclaw-maintainer-cross-issue-sweep/SKILL.md"); const comparator = read(".agents/skills/nemoclaw-maintainer-pr-comparator/SKILL.md"); - expect(sweep).toContain("The comparator does not call it"); - expect(comparator).toContain("Cross-issue regression sweep (separate skill)"); + expect(sweep).toContain("The comparator does not run this skill or use its findings"); + expect(comparator).toContain("Run `nemoclaw-maintainer-cross-issue-sweep` separately"); }); it("uses the merge gate's unresolved-issue threshold for ready-now PRs", () => { @@ -239,7 +239,7 @@ describe("maintainer skills follow canonical workflow policy", () => { ".agents/skills/nemoclaw-maintainer-pr-comparator/scripts/collect-gates.sh", ); - expect(mergeGate).toContain("every PR commit appears as `Verified` in GitHub"); + expect(mergeGate).toContain("Require every commit to appear as `Verified` in GitHub"); expect(comparator).toContain("gate_contributor_compliance"); expect(comparator).toContain(".commit.verification.verified"); }); @@ -255,4 +255,58 @@ describe("maintainer skills follow canonical workflow policy", () => { "PR-body DCO declaration or GitHub Verified commit history is missing", ); }); + + it("keeps PR workflow writes behind their safety checks", () => { + const createPr = read(".agents/skills/nemoclaw-contributor-create-pr/SKILL.md"); + const judgment = read( + ".agents/skills/nemoclaw-maintainer-cross-issue-sweep/checks/relationship-judgment.md", + ); + const mergeGate = read(".agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md"); + const salvage = read(".agents/skills/nemoclaw-maintainer-day/SALVAGE-PR.md"); + + expect(createPr).toContain("For work that is not ready for review, complete Step 4"); + expect(createPr).toContain("--body-file /tmp/nemoclaw-pr-body.md"); + expect(createPr).not.toContain('--body "..."'); + expect(judgment).toContain("{candidate_comments}"); + expect( + mergeGate.match( + /the PR is open and that the PR SHA, base SHA, and coordination identity still match/gu, + ), + ).toHaveLength(2); + expect(salvage).toContain("`headRepository.nameWithOwner` is `NVIDIA/NemoClaw`"); + expect(salvage).toContain("git push origin :"); + expect(salvage).toContain("If `maintainerCanModify` is false, do not push"); + }); + + it("keeps maintainer ordering, state, and write authorization explicit", () => { + const sequence = read(".agents/skills/nemoclaw-maintainer-day/SEQUENCE-WORK.md"); + const state = read(".agents/skills/nemoclaw-maintainer-day/STATE-SCHEMA.md"); + const instructions = read( + ".agents/skills/nemoclaw-maintainer-policies/references/triage-instructions.md", + ); + const triage = read(".agents/skills/nemoclaw-maintainer-triage/SKILL.md"); + + expect(sequence).toContain("An identified security concern overrides this default order"); + expect(state).toContain("Keep at most 50 entries"); + expect(instructions).toContain( + "keep `labels_to_add` and `labels_to_remove` as dry-run output and do not change labels", + ); + expect(instructions).toContain( + "An authorized agent-owned workflow may add or remove only `agt: *` labels", + ); + expect(triage).toContain("Before each write, re-read Issue Type, Project fields, and labels"); + expect(triage).toContain("present an updated proposal for acceptance"); + }); + + it("resolves security-review issue inputs to one verified PR", () => { + const securityReview = read(".agents/skills/nemoclaw-maintainer-security-code-review/SKILL.md"); + + expect(securityReview).toContain("--json closedByPullRequestsReferences"); + expect(securityReview).toContain("Continue only when this returns one PR number"); + expect(securityReview).toContain("Use the verified PR number in each later command"); + expect(securityReview).toContain("If no changed or reviewable security surface exists"); + expect(securityReview).toContain( + "Dockerfiles, workflows, network policies, blueprints, dependencies, and security configuration", + ); + }); }); diff --git a/test/onboard-performance-config-schema.test.ts b/test/onboard-performance-config-schema.test.ts index 3f307fe0bba..9d9a055fbfb 100644 --- a/test/onboard-performance-config-schema.test.ts +++ b/test/onboard-performance-config-schema.test.ts @@ -301,7 +301,7 @@ function validationProvenanceViolations( } describe("full-E2E cold-path calibration", () => { - // source-shape-contract: compatibility -- Exact-head provenance is durable evidence for the hosted-run budget calibration + // source-shape-contract: compatibility -- SHA provenance is durable evidence for the hosted-run budget calibration it("records five independent successful samples for current main", () => { expect(calibration.schemaVersion).toBe(1); expect(calibration.baselineMainSha).toMatch(/^[0-9a-f]{40}$/u); @@ -405,7 +405,7 @@ describe("full-E2E cold-path calibration", () => { ]); expect(validation.derivation.statistic).toBe("maximum"); expect(validation.retirement).toEqual({ - trigger: "successful-exact-head-calibration", + trigger: "successful-single-sha-calibration", minimumSampleCount: 5, allSamplesSameHead: true, imageChangeMustBeAncestor: true, diff --git a/test/pr-comparator-render-verdict.test.ts b/test/pr-comparator-render-verdict.test.ts index 094d787fa84..f6c85050870 100644 --- a/test/pr-comparator-render-verdict.test.ts +++ b/test/pr-comparator-render-verdict.test.ts @@ -14,7 +14,7 @@ const renderer = path.join( const passingGates = { state_open: true, - ci_green_latest_sha: true, + ci_green_sha: true, mergeable: true, contributor_compliance: true, branch_protection: true, @@ -58,6 +58,15 @@ describe("PR comparator verdict renderer", () => { expect(result.stdout).toContain("### Verdict: MERGE PR #123"); }); + it("allows a no-winner result when happy-mode evidence is insufficient", () => { + const result = render(specFor(passingGates, { mode: "happy" })); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(""); + expect(result.stdout).toContain("### Verdict: No clear winner"); + expect(result.stdout).not.toContain("MERGE PR"); + }); + it("rejects a supplied winner if contributor compliance failed", () => { const result = render( specFor({ ...passingGates, contributor_compliance: false }, { mode: "happy", winner: 123 }), @@ -82,10 +91,7 @@ describe("PR comparator verdict renderer", () => { it("rejects a supplied mode that contradicts derived eligibility", () => { const result = render( - specFor( - { ...passingGates, ci_green_latest_sha: false }, - { mode: "happy", closest_to_ready: 123 }, - ), + specFor({ ...passingGates, ci_green_sha: false }, { mode: "happy", closest_to_ready: 123 }), ); expect(result.status).toBe(64); @@ -96,7 +102,7 @@ describe("PR comparator verdict renderer", () => { it("uses closest_to_ready for an eligible degraded-mode salvage candidate", () => { const result = render( specFor( - { ...passingGates, ci_green_latest_sha: false }, + { ...passingGates, ci_green_sha: false }, { mode: "degraded", closest_to_ready: 123 }, ), ); @@ -129,8 +135,16 @@ describe("PR comparator verdict renderer", () => { for (const gate of Object.keys(passingGates)) { expect(skill).toContain(`\`${gate}\``); } - expect(skill).toContain("set `winner` only to a PR in that set"); - expect(skill).toContain("use `closest_to_ready` only for an open, contributor-compliant"); - expect(skill).toContain("do not render or recommend a merge if it exits nonzero"); + expect(skill).toContain("set `winner` only to an eligible PR"); + expect(skill).toContain("Leave `winner` null when the evidence does not support"); + expect(skill).toContain( + "Set `closest_to_ready` only to an open PR that passes contributor requirements", + ); + expect(skill).toContain( + "Stop if the renderer exits with a nonzero status. Do not recommend a merge", + ); + expect(skill).toContain( + "The reviewer remains responsible for the score, ranking, and evidence", + ); }); }); diff --git a/test/pr-e2e-gate-fork-skip.test.ts b/test/pr-e2e-gate-fork-skip.test.ts index 036823b4131..2d2954d38f3 100644 --- a/test/pr-e2e-gate-fork-skip.test.ts +++ b/test/pr-e2e-gate-fork-skip.test.ts @@ -399,7 +399,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { summary: "The prerequisite CI failed.\n\n", currentCiConclusion: "failure", }, - ])("preserves $label instead of reopening the exact diff", async ({ + ])("preserves $label instead of reopening the PR/base SHA pair", async ({ title, summary, currentCiConclusion, @@ -429,7 +429,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { try { await expect( startPrGate({ ...startCommand(workDir), ciConclusion: currentCiConclusion }), - ).rejects.toThrow(/exact-diff PR gate state is not retryable/u); + ).rejects.toThrow(/PR gate state for this PR\/base SHA pair is not retryable/u); expect(requests.some((request) => request.method === "PATCH")).toBe(false); expect(originalState).toEqual({ status: "completed", @@ -457,9 +457,9 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { { label: "multiple active current candidates", checks: [exactPrGateCheck(), exactPrGateCheck({ id: 18 })], - expectedError: "Multiple active exact-diff PR gate checks exist", + expectedError: "Multiple active PR gate checks exist for one PR/base SHA pair", }, - ])("fails closed when exact-diff history contains $label", async ({ checks, expectedError }) => { + ])("fails closed when PR/base SHA history contains $label", async ({ checks, expectedError }) => { const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-pr-e2e-gate-history-")); vi.stubEnv("GITHUB_TOKEN", "token"); vi.stubEnv("GITHUB_REPOSITORY", "NVIDIA/NemoClaw"); @@ -542,7 +542,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { }); expect(JSON.stringify(completion?.body)).not.toContain("conclusion"); expect(JSON.stringify(completion?.body)).toContain( - "run `run-control-plane` with the PR number, exact head and base SHAs", + "run `run-control-plane` with the PR number, PR SHA, base SHA", ); expect(fs.readFileSync(outputPath, "utf8")).not.toContain("fork_skip_mode="); expect(fs.readFileSync(outputPath, "utf8")).toContain("finalized=true"); @@ -830,7 +830,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { expect(JSON.stringify(completion?.body)).not.toContain("tests passed"); }); - it("dispatches an authorized exact-SHA control-plane run without clearing the gate", async () => { + it("dispatches an authorized control-plane run for the PR SHA without clearing the gate", async () => { const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-pr-e2e-gate-authorized-")); const outputPath = path.join(workDir, "github-output"); fs.writeFileSync(outputPath, "", { mode: 0o600 }); @@ -1049,7 +1049,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { conclusion: "failure", output: { title: "Authorized E2E run requires reconciliation", - summary: expect.stringContaining("this exact-diff authorization cannot be retried"), + summary: expect.stringContaining("cannot be retried"), }, }); await expect(startControlPlanePrGate(startControlPlaneCommand(workDirs[1]!))).rejects.toThrow( @@ -1311,7 +1311,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { status: "in_progress", output: { title: "Maintainer authorization required to run E2E", - summary: expect.stringContaining("launch a fresh first-attempt `run-control-plane`"), + summary: expect.stringContaining("launch a first-attempt `run-control-plane`"), }, }); expect(restoredAuthorizations[0]?.body).not.toHaveProperty("conclusion"); @@ -1440,7 +1440,7 @@ describe("PR E2E controller fork credentialed E2E skip approval safety", () => { maintainer: "maintainer", reason: "The reviewed base revision has since changed upstream.", }), - ).rejects.toThrow(/no longer matches the reviewed exact head and base SHAs/u); + ).rejects.toThrow(/no longer matches the reviewed PR SHA and base SHA/u); expect(requests.some((request) => request.method === "PATCH")).toBe(false); }); diff --git a/test/pr-e2e-gate-lifecycle.test.ts b/test/pr-e2e-gate-lifecycle.test.ts index 747c205b7c9..d23f4298917 100644 --- a/test/pr-e2e-gate-lifecycle.test.ts +++ b/test/pr-e2e-gate-lifecycle.test.ts @@ -516,7 +516,7 @@ describe("PR E2E controller lifecycle", () => { expectedSummary: "moved from head `aaaaaaa` on base `bbbbbbb` to head `ccccccc` on base `bbbbbbb`", }, - ])("records an obsolete exact-diff outcome without failing the controller when $label", async ({ + ])("records an obsolete PR/base SHA outcome without failing the controller when $label", async ({ currentPull, firstFinalizationPull = currentPull, writeEvidence, diff --git a/test/pr-e2e-gate-retry-history.test.ts b/test/pr-e2e-gate-retry-history.test.ts index 1a04ce3391f..a1bdc11ecd0 100644 --- a/test/pr-e2e-gate-retry-history.test.ts +++ b/test/pr-e2e-gate-retry-history.test.ts @@ -267,7 +267,7 @@ describe("PR E2E controller retry history", () => { }, }); expect(JSON.stringify(completion?.body)).toContain( - "run `run-control-plane` with the PR number, exact head and base SHAs", + "run `run-control-plane` with the PR number, PR SHA, base SHA", ); expect(fs.readFileSync(outputPath, "utf8")).not.toContain("fork_skip_mode="); expect(fs.readFileSync(outputPath, "utf8")).toContain("check_id=18"); diff --git a/test/pr-e2e-gate-shards.test.ts b/test/pr-e2e-gate-shards.test.ts index 67cf027096a..8a877158831 100644 --- a/test/pr-e2e-gate-shards.test.ts +++ b/test/pr-e2e-gate-shards.test.ts @@ -31,7 +31,7 @@ describe("PR E2E shard policy", () => { }); }); - // source-shape-contract: security -- Malformed matrix shard selectors must fail closed before exact-SHA evidence dispatch + // source-shape-contract: security -- Malformed matrix shard selectors must fail closed before SHA evidence dispatch it("rejects malformed configured matrix shard selectors", () => { const workflow = fs.readFileSync(".github/workflows/e2e.yaml", "utf8"); const shardExpression = "NEMOCLAW_E2E_SHARD: ${{ matrix.mode }}"; diff --git a/test/pr-e2e-gate-workflow.test.ts b/test/pr-e2e-gate-workflow.test.ts index 398f4e297e2..d1846dda0a4 100644 --- a/test/pr-e2e-gate-workflow.test.ts +++ b/test/pr-e2e-gate-workflow.test.ts @@ -409,7 +409,7 @@ describe("PR E2E gate workflow", () => { ref: "${{ github.workflow_sha }}", "persist-credentials": false, }); - const observer = step(required, "Wait for trusted exact-diff verdict"); + const observer = step(required, "Wait for trusted PR/base SHA verdict"); expect(observer.env).toEqual({ BASE_SHA: "${{ github.event.pull_request.base.sha }}", GITHUB_TOKEN: "${{ github.token }}", @@ -486,11 +486,11 @@ describe("PR E2E gate workflow", () => { expect( collectStrings(recordForkSkip).some((value) => value.includes("--mode record-fork-e2e-skip")), ).toBe(true); - expect(step(initialize, "Reserve exact-diff gate").run).toContain('--head "$HEAD_SHA"'); - expect(step(initialize, "Reserve exact-diff gate").env?.BASE_SHA).toBe( + expect(step(initialize, "Reserve PR/base SHA gate").run).toContain('--head "$HEAD_SHA"'); + expect(step(initialize, "Reserve PR/base SHA gate").env?.BASE_SHA).toBe( "${{ github.event.pull_request.base.sha }}", ); - expect(step(initialize, "Reserve exact-diff gate").run).toContain('--base "$BASE_SHA"'); + expect(step(initialize, "Reserve PR/base SHA gate").run).toContain('--base "$BASE_SHA"'); const start = step(coordinate, "Start evaluation"); expect(start.env?.CI_DISPLAY_TITLE).toBe("${{ github.event.workflow_run.display_title }}"); expect(start.env?.GATE_RUN_ID).toBe("${{ github.run_id }}"); @@ -645,7 +645,7 @@ describe("PR E2E gate workflow", () => { }); it("passes the control-plane review reason as one inert argument", () => { - const reason = "Reviewed exact diff; $(printf injected)"; + const reason = "Reviewed PR/base SHA pair; $(printf injected)"; const execution = runControlPlaneStartStep(reason); const reasonFlag = execution.arguments.indexOf("--reason"); @@ -678,7 +678,7 @@ describe("PR E2E gate workflow", () => { expect(current.status).toBe(0); expect(combined.status).toBe(0); expect(stale.status).toBe(1); - expect(stale.stdout).toContain("checkout_sha must match the PR head commit"); + expect(stale.stdout).toContain("checkout_sha must match the PR SHA"); expect(retargeted.status).toBe(1); expect(retargeted.stdout).toContain("base_sha must match the PR base commit"); expect(racedWorkflow.status).toBe(1); diff --git a/test/pr-e2e-gate.test.ts b/test/pr-e2e-gate.test.ts index d7ee5572be8..b2dc1a61a26 100644 --- a/test/pr-e2e-gate.test.ts +++ b/test/pr-e2e-gate.test.ts @@ -715,7 +715,7 @@ describe("PR E2E controller", () => { ).toThrow(/display_title/u); }); - it("seeds one idempotent exact-diff gate", async () => { + it("seeds one idempotent gate for a PR/base SHA pair", async () => { vi.stubEnv("GITHUB_TOKEN", "token"); vi.stubEnv("GITHUB_REPOSITORY", "NVIDIA/NemoClaw"); const requests: RecordedGitHubRequest[] = []; @@ -887,7 +887,7 @@ describe("PR E2E controller", () => { } }); - it("closes a superseded exact-diff check without failing the controller", async () => { + it("closes a superseded PR/base SHA check without failing the controller", async () => { const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-pr-e2e-gate-superseded-")); const outputPath = path.join(workDir, "github-output"); fs.writeFileSync(outputPath, "", { mode: 0o600 }); @@ -1354,7 +1354,7 @@ describe("PR E2E controller", () => { expect(gate.expectedJobs).toEqual(BROAD_JOBS); expect(gate.expectedTargets).toEqual([]); expect(requests.filter((request) => request.url.includes("/pulls?"))).toHaveLength(1); - // Finalization brackets evidence parsing with exact-diff reads so a PR update cannot + // Finalization reads the PR/base SHA pair before and after parsing so a PR update cannot // turn stale evidence into a current-revision result. expect(requests.filter((request) => request.url.endsWith("/pulls/42"))).toHaveLength(4); const checkCreation = requests.find( @@ -1367,7 +1367,7 @@ describe("PR E2E controller", () => { status: "in_progress", output: { title: "Waiting for PR CI", - summary: expect.stringContaining("exact PR head and base revision"), + summary: expect.stringContaining("PR SHA and base SHA"), }, }); const dispatch = requests.find((request) => request.url.endsWith("/dispatches")); diff --git a/test/pr-e2e-required.test.ts b/test/pr-e2e-required.test.ts index 39d3fdbef17..3fab1c7fc76 100644 --- a/test/pr-e2e-required.test.ts +++ b/test/pr-e2e-required.test.ts @@ -191,7 +191,7 @@ describe("native PR E2E required job", () => { expect(urls[0]).toContain("E2E%20%2F%20PR%20Gate%20Coordination"); }); - it("selects the newest exact-diff check after marker-backed immutable history", async () => { + it("selects the newest PR/base SHA check after marker-backed immutable history", async () => { vi.spyOn(globalThis, "fetch").mockResolvedValue( githubResponse( listing([ @@ -236,9 +236,9 @@ describe("native PR E2E required job", () => { check(undefined, { status: "in_progress", conclusion: null }), check(undefined, { id: 18, status: "in_progress", conclusion: null }), ], - expectedError: "Multiple active exact-diff coordination checks exist", + expectedError: "Multiple active coordination checks exist for one PR/base SHA pair", }, - ])("rejects exact-diff coordination history with $label", async ({ checks, expectedError }) => { + ])("rejects PR/base SHA coordination history with $label", async ({ checks, expectedError }) => { vi.spyOn(globalThis, "fetch").mockResolvedValue(githubResponse(listing(checks))); await expect(findCoordinationCheck(identity)).rejects.toThrow(expectedError); @@ -257,7 +257,7 @@ describe("native PR E2E required job", () => { }); }); - it("rejects an exact-diff identity claimed by another app", async () => { + it("rejects a PR/base SHA identity claimed by another app", async () => { vi.spyOn(globalThis, "fetch").mockResolvedValue( githubResponse(listing([check(undefined, { app: { id: 1 } })])), ); @@ -422,6 +422,6 @@ describe("native PR E2E required job", () => { await expect( waitForRequiredGate(identity, { timeoutMs: 100, pollIntervalMs: 10 }), - ).rejects.toThrow("no longer matches the exact head and base revision"); + ).rejects.toThrow("not the expected open PR with the observed PR SHA and base SHA"); }); }); diff --git a/test/pr-review-advisor.test.ts b/test/pr-review-advisor.test.ts index 800a7402419..83ca886cebf 100644 --- a/test/pr-review-advisor.test.ts +++ b/test/pr-review-advisor.test.ts @@ -430,7 +430,8 @@ diff --git a/test/plain-logic.test.ts b/test/plain-logic.test.ts expect(skill).toContain("# Security Code Review"); expect(skill).toContain("Category 1: Secrets and Credentials"); expect(prompt).toContain("Trusted security review skill from main checkout"); - expect(prompt).toContain("For NemoClaw PRs, check sandbox escape vectors"); + expect(prompt).toContain("For NemoClaw PRs, check SSRF bypasses"); + expect(prompt).not.toContain("For NemoClaw PRs, check sandbox escape vectors"); expect(prompt).toContain( "Do not report GitHub mergeability, branch protection, CI status, reviewer state, CodeRabbit state, or external E2E job status", ); diff --git a/test/skills-frontmatter.test.ts b/test/skills-frontmatter.test.ts index 4639ef1bbef..956c20be8ce 100644 --- a/test/skills-frontmatter.test.ts +++ b/test/skills-frontmatter.test.ts @@ -111,9 +111,9 @@ describe("repo skill markdown files", () => { expect(skill).toContain("git diff origin/main...HEAD"); expect(skill).toContain("git rev-list origin/main..HEAD"); expect(skill).not.toMatch(/(? { diff --git a/test/skills/check-gates-compliance.test.ts b/test/skills/check-gates-compliance.test.ts index 5698d765911..e21378a1df2 100644 --- a/test/skills/check-gates-compliance.test.ts +++ b/test/skills/check-gates-compliance.test.ts @@ -600,7 +600,7 @@ describe("maintainer PR comparator contributor compliance", () => { }); const output = JSON.parse(result.stdout); - expect(output.gates.ci_green_latest_sha).toBe(true); + expect(output.gates.ci_green_sha).toBe(true); expect(output.gates.contributor_compliance).toBe(true); expect(output.details).toMatchObject({ dco_declaration_present: true, @@ -745,7 +745,7 @@ describe("maintainer PR comparator contributor compliance", () => { }); const output = JSON.parse(result.stdout); - expect(output.gates.ci_green_latest_sha).toBe(false); + expect(output.gates.ci_green_sha).toBe(false); expect(output.details.ci_missing_required_checks).toEqual(REQUIRED_CHECK_NAMES); expect(output.failures).toContain( "substantive:ci_failures=0,pending=0,missing=checks,check-hash,changes,commit-lint,dco-check,E2E / PR Gate", @@ -786,7 +786,7 @@ describe("maintainer PR comparator contributor compliance", () => { missingChecks: ["E2E / PR Gate"], }); expect(mergeOutput.allPass).toBe(false); - expect(comparatorOutput.gates.ci_green_latest_sha).toBe(false); + expect(comparatorOutput.gates.ci_green_sha).toBe(false); expect(comparatorOutput.details.ci_missing_required_checks).toEqual(["E2E / PR Gate"]); expect(comparatorOutput.failures).toContain( "substantive:ci_failures=0,pending=0,missing=E2E / PR Gate", @@ -814,7 +814,7 @@ describe("maintainer PR comparator contributor compliance", () => { failingChecks: [`E2E / PR Gate: ${conclusion}`], }); expect(mergeOutput.allPass).toBe(false); - expect(comparatorOutput.gates.ci_green_latest_sha).toBe(false); + expect(comparatorOutput.gates.ci_green_sha).toBe(false); expect(comparatorOutput.details.ci_failing_checks).toEqual([`E2E / PR Gate: ${conclusion}`]); }); @@ -830,7 +830,7 @@ describe("maintainer PR comparator contributor compliance", () => { }); const output = JSON.parse(result.stdout); - expect(output.gates.ci_green_latest_sha).toBe(false); + expect(output.gates.ci_green_sha).toBe(false); expect(output.details.ci_failing_checks).toEqual([`checks: ${conclusion}`]); expect(output.failures).toContain("substantive:ci_failures=1,pending=0,missing="); }); diff --git a/test/skills/check-gates-evidence.test.ts b/test/skills/check-gates-evidence.test.ts index 433ca7a1351..c39bf355cd6 100644 --- a/test/skills/check-gates-evidence.test.ts +++ b/test/skills/check-gates-evidence.test.ts @@ -425,7 +425,7 @@ describe("maintainer merge-gate contributor compliance", () => { }); }); - it("accepts the legacy exact-diff E2E coordination check name", () => { + it("accepts the legacy E2E coordination check name", () => { const legacyCheck = coordinationCheck({ id: 8001, name: "E2E / PR Gate" }); const output = JSON.parse( runGate({ @@ -534,7 +534,7 @@ describe("maintainer merge-gate contributor compliance", () => { ], ], ["from an incomplete page set", [{ total_count: 2, check_runs: [coordinationCheck()] }]], - ])("fails closed when exact-diff E2E coordination evidence is %s", (_name, pages) => { + ])("fails closed when PR/base SHA E2E coordination evidence is %s", (_name, pages) => { const output = JSON.parse( runGate({ body: "Signed-off-by: Example User ", @@ -549,7 +549,7 @@ describe("maintainer merge-gate contributor compliance", () => { }); }); - it("requires exact-diff evidence for optional Actions checks", () => { + it("requires PR/base SHA evidence for optional Actions checks", () => { const result = runGate({ body: "Signed-off-by: Example User ", verified: true, @@ -832,7 +832,7 @@ describe("maintainer merge-gate contributor compliance", () => { failingChecks: ["matrix-check: FAILURE"], }); }); - it("accepts exact-head evidence from a non-PR Actions event", () => { + it("accepts SHA evidence from a non-PR Actions event", () => { const fixture = e2eRunFixture(e2eChecks([874, 2, "SUCCESS"]), { "874": exactDiffGateRun("success", e2eJobs(2)), "875": { @@ -1325,7 +1325,7 @@ describe("maintainer merge-gate contributor compliance", () => { runs: {} as Record, }, { - name: "rejects exact-diff runs with different workflow identities", + name: "rejects PR/base SHA runs with different workflow identities", checks: e2eChecks([480, 40, "FAILURE"], [481, 41, "SUCCESS"]), runs: { "480": { @@ -1340,7 +1340,7 @@ describe("maintainer merge-gate contributor compliance", () => { } as Record, }, { - name: "rejects an exact-diff run with a null workflow path", + name: "rejects a PR/base SHA run with a null workflow path", checks: e2eChecks([482, 41, "SUCCESS"]), runs: { "482": { ...exactDiffGateRun("success", e2eJobs(41)), path: undefined }, diff --git a/test/starter-prompt-docs.test.ts b/test/starter-prompt-docs.test.ts index 73e9d3d2dee..8f064b43de6 100644 --- a/test/starter-prompt-docs.test.ts +++ b/test/starter-prompt-docs.test.ts @@ -28,9 +28,9 @@ const __dirname = path.dirname(__filename); const repoRoot = path.resolve(__dirname, ".."); const starterPromptMarkdownSource = path.join(repoRoot, "docs", "resources", "starter-prompt.md"); -// CI resolves this exact Git commit and byte-compares its prompt-asset blobs with +// CI resolves this Git commit and byte-compares its prompt-asset blobs with // the local files. The digests independently assert those same immutable bytes. -const promptAssetRevision = "f814db4f7708ecf9ab054fcee449f11c95076dfd"; +const promptAssetRevision = "c718a78c5794574a98fdd885d94466c3b6794153"; type PromptAsset = { path: string; @@ -53,7 +53,7 @@ const promptAssets = { ), dgxStation: definePromptAsset( "docs/resources/prompt-assets/dgx-station.md", - "783a3f5973e1471178c78ead1952b904f3888dc603f5e079ad31d82cd2037f9a", // gitleaks:allow -- pinned prompt-asset SHA-256 + "f0c61cef93da203cecda2424eb1fc5680d56ffd679a518bfc98d26b2e82be381", // gitleaks:allow -- pinned prompt-asset SHA-256 ), windowsWsl: definePromptAsset( "docs/resources/prompt-assets/windows-wsl.md", @@ -657,7 +657,7 @@ describe("starter prompt docs CTA", () => { expect(promptSource).toContain("Existing vLLM: `NEMOCLAW_PROVIDER=vllm`"); }); - it("keeps local prompt assets byte-aligned with their exact immutable revision blobs (#6990)", () => { + it("keeps local prompt assets byte-aligned with their pinned revision blobs (#6990)", () => { resolvePromptAssetRevision(promptAssetRevision, runGit); for (const asset of Object.values(promptAssets)) { const localBytes = fs.readFileSync(path.join(repoRoot, asset.path)); diff --git a/tools/e2e/pr-e2e-gate.mts b/tools/e2e/pr-e2e-gate.mts index dfd00d3995a..f5ec89ce0ba 100755 --- a/tools/e2e/pr-e2e-gate.mts +++ b/tools/e2e/pr-e2e-gate.mts @@ -978,17 +978,18 @@ function currentExactDiffCheck(checks: CheckRun[]): CheckRun | undefined { if (checks.length === 0) return undefined; const ordered = [...checks].sort((left, right) => left.id - right.id); if (new Set(ordered.map((check) => check.id)).size !== ordered.length) { - throw new Error("Duplicate exact-diff PR gate check IDs exist"); + throw new Error("Duplicate PR gate check IDs exist for one PR/base SHA pair"); } const active = ordered.filter((check) => check.status !== "completed"); - if (active.length > 1) throw new Error("Multiple active exact-diff PR gate checks exist"); + if (active.length > 1) + throw new Error("Multiple active PR gate checks exist for one PR/base SHA pair"); const history = ordered.slice(0, -1); if (history.some((check) => retryableFailureReason(check) === undefined)) { - throw new Error("Exact-diff PR gate history contains a non-retryable older check"); + throw new Error("PR gate history contains a non-retryable older check for one PR/base SHA pair"); } const current = ordered.at(-1)!; if (active[0] && active[0].id !== current.id) { - throw new Error("Exact-diff PR gate history contains an older active check"); + throw new Error("PR gate history for one PR/base SHA pair contains an older active check"); } return current; } @@ -1076,7 +1077,7 @@ async function createPrGateCheck(options: { const externalId = prGateExternalId(options.prNumber, options.headSha, options.baseSha); const title = "Waiting for PR CI"; const summary = - "This exact PR head and base revision is reserved for deterministic E2E planning after CI completes."; + "This PR SHA and base SHA are reserved for deterministic E2E planning after CI completes."; const check = await githubApi(`repos/${options.repository}/check-runs`, options.token, { method: "POST", body: { @@ -1156,7 +1157,7 @@ export async function seedPrGate( prNumber, }); console.log( - `Exact-diff gate reserved: pr=${prNumber} head=${headSha} base=${baseSha} check=${checkRunId}`, + `PR gate reserved: pr=${prNumber} pr_sha=${headSha} base_sha=${baseSha} check=${checkRunId}`, ); return checkRunId; } @@ -1195,7 +1196,7 @@ function assertCheckCanStart(check: CheckRun | undefined, ciConclusion: string): if (ciConclusion === "success" && reason) return; const title = normalizedCiMetadata(check.output?.title ?? "untitled", "untitled"); throw new Error( - `Existing exact-diff PR gate state is not retryable: status=${check.status ?? "unknown"} conclusion=${check.conclusion ?? "none"} title=${title}`, + `Existing PR gate state for this PR/base SHA pair is not retryable: status=${check.status ?? "unknown"} conclusion=${check.conclusion ?? "none"} title=${title}`, ); } @@ -2419,7 +2420,7 @@ export async function startPrGate( prNumber: ciIdentity.prNumber, }); if (existingChecks.length > 1) { - throw new Error("Multiple exact-diff PR gate checks already exist"); + throw new Error("Multiple PR gate checks already exist for this PR/base SHA pair"); } const existingCheckRunId = existingChecks[0]?.status === "in_progress" ? existingChecks[0].id : undefined; @@ -2471,7 +2472,7 @@ export async function startPrGate( }, token, "Evaluating PR commit", - "Validating the exact PR revision and selecting deterministic E2E jobs and typed targets.", + "Validating the PR SHA and selecting deterministic E2E jobs and typed targets.", ); let finalized = false; @@ -2586,9 +2587,9 @@ export async function startPrGate( token, CONTROL_PLANE_AUTHORIZATION_TITLE, [ - `This exact internal diff (head \`${command.headSha}\`, base \`${ciIdentity.baseSha}\`) changes code that the selected credential-bearing E2E jobs or targets execute or trust (${selectionSummary}).`, + `This internal diff (PR SHA \`${command.headSha}\`, base SHA \`${ciIdentity.baseSha}\`) changes code that the selected credential-bearing E2E jobs or targets execute or trust (${selectionSummary}).`, "No selected E2E job or target ran and no repository secret was exposed.", - `A repository maintainer or administrator must review this exact revision, then open the [${WORKFLOW_NAME}](${workflowUrl}) workflow and run \`run-control-plane\` with the PR number, exact head and base SHAs, and a review reason. That authorized run dispatches the selected jobs and targets in one bound workflow run, and this gate passes only if their exact-SHA evidence verifies successfully.`, + `A repository maintainer or administrator must review PR SHA \`${command.headSha}\` against base SHA \`${ciIdentity.baseSha}\`. Then, they must open the [${WORKFLOW_NAME}](${workflowUrl}) workflow and run \`run-control-plane\` with the PR number, PR SHA, base SHA, and a review reason. That run dispatches the selected jobs and targets in one workflow run. This gate passes only if the evidence references both SHAs and verifies successfully.`, `Deterministic plan: \`${plan.planHash}\`.`, ].join("\n\n"), ); @@ -2705,7 +2706,9 @@ export async function startControlPlanePrGate(command: ControlPlaneDispatchComma prNumber: command.prNumber, }); if (matchingChecks.length !== 1) { - throw new Error(`Expected one exact-diff PR gate check; found ${matchingChecks.length}`); + throw new Error( + `Expected one PR gate check for the PR/base SHA pair; found ${matchingChecks.length}`, + ); } const check = matchingChecks[0]!; const pendingAuthorization = check.status === "in_progress" && check.conclusion === null; @@ -2757,7 +2760,7 @@ export async function startControlPlanePrGate(command: ControlPlaneDispatchComma error, detailsUrl: `https://github.com/${repository}/actions/runs/${error.childRunId}`, recovery: - "A credential-bearing child run was dispatched, so this exact-diff authorization cannot be retried. Inspect the linked run, then update the PR and run fresh CI before authorizing again.", + "A credential-bearing child run was dispatched, so this authorization for the PR/base SHA pair cannot be retried. Inspect the linked run, then update the PR and run fresh CI before authorizing again.", }, ); if (closed) appendOutput("finalized", "true"); @@ -2776,7 +2779,7 @@ export async function startControlPlanePrGate(command: ControlPlaneDispatchComma CONTROL_PLANE_AUTHORIZATION_TITLE, [ `The authorized E2E attempt did not produce an accepted result: \`${reason}\`.`, - "Review the controller error and any linked child run, then launch a fresh first-attempt `run-control-plane` workflow for this exact revision.", + "Review the controller error and any linked child run. Then, launch a first-attempt `run-control-plane` workflow for the PR/base SHA pair.", ].join("\n\n"), ); appendOutput("finalized", "true"); @@ -2892,7 +2895,7 @@ export async function finishPrGate(options: { prNumber: state.prNumber, }); if (matchingHistory.at(-1)?.id !== options.checkRunId) { - throw new Error("controller state does not match the exact PR gate check"); + throw new Error("controller state does not match the PR gate check"); } const priorRunnerLossUrls = priorRunnerLossRunUrls( repository, @@ -3189,7 +3192,7 @@ async function completeForkE2ESkip(command: ForkSkipCommand): Promise { pull.head.sha !== command.headSha || pull.base.sha !== command.baseSha ) { - throw new Error("pull request no longer matches the reviewed exact head and base SHAs"); + throw new Error("pull request no longer matches the reviewed PR SHA and base SHA"); } const isFork = pull.head.repo.full_name !== repository; if (!isFork) { @@ -3227,7 +3230,9 @@ async function completeForkE2ESkip(command: ForkSkipCommand): Promise { prNumber: command.prNumber, }); if (matchingChecks.length !== 1) { - throw new Error(`Expected one exact-diff PR gate check; found ${matchingChecks.length}`); + throw new Error( + `Expected one PR gate check for the PR/base SHA pair; found ${matchingChecks.length}`, + ); } const check = matchingChecks[0]!; if ( diff --git a/tools/e2e/pr-e2e-required.mts b/tools/e2e/pr-e2e-required.mts index 73a3d2ac579..5174a8af7cd 100644 --- a/tools/e2e/pr-e2e-required.mts +++ b/tools/e2e/pr-e2e-required.mts @@ -146,7 +146,7 @@ function validatePullRequest(value: unknown, identity: RequiredGateIdentity): Pu !isObjectRecord(value.base) || value.base.sha !== identity.baseSha ) { - throw new Error("PR no longer matches the exact head and base revision observed by this job"); + throw new Error("PR is not the expected open PR with the observed PR SHA and base SHA"); } return value as PullRequest; } @@ -188,16 +188,19 @@ function currentCoordinationCheck( if (checks.length === 0) return undefined; const ordered = [...checks].sort((left, right) => left.id - right.id); if (new Set(ordered.map((check) => check.id)).size !== ordered.length) { - throw new Error("Duplicate exact-diff coordination check IDs exist"); + throw new Error("Duplicate coordination check IDs exist for one PR/base SHA pair"); } const active = ordered.filter((check) => check.status !== "completed"); - if (active.length > 1) throw new Error("Multiple active exact-diff coordination checks exist"); + if (active.length > 1) + throw new Error("Multiple active coordination checks exist for one PR/base SHA pair"); if (ordered.slice(0, -1).some((check) => !hasRetryableFailureMarker(check))) { - throw new Error("Exact-diff coordination history contains a non-retryable older check"); + throw new Error( + "Coordination history contains a non-retryable older check for one PR/base SHA pair", + ); } const current = ordered.at(-1)!; if (active[0] && active[0].id !== current.id) { - throw new Error("Exact-diff coordination history contains an older active check"); + throw new Error("Coordination history for one PR/base SHA pair contains an older active check"); } return current; } @@ -221,7 +224,9 @@ async function matchingChecks( check.external_id === externalId, ); if (claimed.some((check) => check.app?.id !== GITHUB_ACTIONS_APP_ID)) { - throw new Error("The exact-diff coordination identity was claimed by an unexpected GitHub App"); + throw new Error( + "The PR/base SHA coordination identity was claimed by an unexpected GitHub App", + ); } const current = currentCoordinationCheck( claimed.filter((check) => check.app?.id === GITHUB_ACTIONS_APP_ID), @@ -234,13 +239,15 @@ export async function findCoordinationCheck( ): Promise { assertIdentity(identity); const current = await matchingChecks(identity, COORDINATION_CHECK_NAME); - if (current.length > 1) throw new Error("Multiple exact-diff coordination checks exist"); + if (current.length > 1) + throw new Error("Multiple coordination checks exist for one PR/base SHA pair"); if (current[0]) return current[0]; // Migration bridge for PRs whose base-branch controller still publishes the // old name. Remove after this workflow is on main and open PRs resynchronize. const legacy = await matchingChecks(identity, LEGACY_COORDINATION_CHECK_NAME); - if (legacy.length > 1) throw new Error("Multiple legacy exact-diff coordination checks exist"); + if (legacy.length > 1) + throw new Error("Multiple legacy coordination checks exist for one PR/base SHA pair"); return legacy[0]; } diff --git a/tools/pr-review-advisor/README.md b/tools/pr-review-advisor/README.md index f57600432e8..4e5446a77a9 100644 --- a/tools/pr-review-advisor/README.md +++ b/tools/pr-review-advisor/README.md @@ -115,9 +115,9 @@ instead of failing closed without artifacts. - `turns/01-scope-risk-map-analysis.txt` through `turns/14-validate-synthesis-json.txt` — assistant output and completed/failed/timed-out status written as each turn settles. - `context/drift-context.json` — deterministic drift and overlap context. - `context/security-context.json` — deterministic security-risk context and the risk plan for the - PR head commit. + PR SHA. - `context/validation-context.json` — deterministic acceptance, source-of-truth, static - test-inventory, simplification-signal, and risk plan for the PR head commit, including the + test-inventory, simplification-signal, and risk plan for the PR SHA, including the regression invariants reviewed for the PR. - `context/pr.diff` — truncated PR diff used by the advisor. - `pr-review-advisor-raw-output.txt` — raw multi-turn advisor transcript and diagnostics.