Skip to content

Commit 56fbf2c

Browse files
authored
Merge pull request #131 from USACE-RMC/docs/dev/branch-conventions-and-notification-fixes
Add non-docs branch conventions, fix lane language, and cut review notification volume
2 parents 94c36d1 + 20a8a40 commit 56fbf2c

18 files changed

Lines changed: 1635 additions & 1500 deletions

‎.claude/skills/git-conventions.md‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,34 @@ Resolve LifeSim User's Guide v1.0 Level 3 QC findings
2121

2222
## Branch Naming
2323

24-
- Feature branches: `feature/{descriptive-name}` (e.g., `feature/site-upgrades`)
25-
- Bug fixes: `fix/{descriptive-name}`
26-
- Documentation: `docs/{descriptive-name}`
24+
Whether a PR gets a review lane is decided by **content**, not by branch name: a PR
25+
that changes any file under `docs/` is a documentation PR; one that changes nothing
26+
under `docs/` has no lane and no review stages.
27+
28+
### Documentation branches (changes under `docs/`)
29+
30+
The prefix routes the PR to a review lane. Use the full two-segment prefix — a bare
31+
`docs/{name}` does **not** match a lane and strands the PR in `stage:needs-lane`.
32+
33+
- `docs/new/{slug}` — new document (Peer → Lead Civil → Technical edit → Director)
34+
- `docs/major/{slug}-v{X.0}` — major revision (Peer → Lead Civil → Technical edit)
35+
- `docs/minor/{slug}-v{X.Y}` — minor revision (Peer → Technical edit)
36+
- `docs/fix/{slug}` — editorial fix (no review; admin self-merge)
37+
- `docs/dev/{slug}` — dev docs under `docs/dev/` (no review; admin self-merge)
38+
39+
### Non-documentation branches (components, plumbing, styling, scripts, CI)
40+
41+
No review lane, no review stages — `CI Build` is the only gate.
42+
43+
- `feature/{descriptive-name}` — new components, new site capabilities, enhancements
44+
- `fix/{descriptive-name}` — bug fixes in components, styles, scripts, build tooling
45+
- `chore/{descriptive-name}` — dependency bumps, refactors, config, cleanup
46+
- `ci/{descriptive-name}` — GitHub workflows and repository automation
47+
48+
Use `feature/` rather than `enhancement/`; older branches used the latter.
49+
50+
A PR that touches both site code and content under `docs/` is a documentation PR —
51+
put it on the matching `docs/…` prefix.
2752

2853
## Pull Requests
2954

‎.claude/skills/new-revision/SKILL.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ The corresponding value in the JSON is the `currentVersion` (e.g., `v1.0`).
2525

2626
Ask: "Is this a major revision or a minor revision?"
2727

28-
- **Major revision** — substantial changes warranting a new major version. Branch prefix: `docs/major/`. Goes through Lane 2 (Peer → Lead Civil review).
29-
- **Minor revision** — smaller updates warranting a minor version bump. Branch prefix: `docs/minor/`. Goes through Lane 3 (Peer review only).
28+
- **Major revision** — substantial changes warranting a new major version. Branch prefix: `docs/major/`. Goes through Lane 2 (Peer → Lead Civil → Technical edit). No Director review.
29+
- **Minor revision** — smaller updates warranting a minor version bump. Branch prefix: `docs/minor/`. Goes through Lane 3 (Peer → Technical edit). No Director review.
3030

3131
### 1c — New version number
3232

‎.claude/skills/technical-edit/SKILL.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,3 +52,6 @@ Tell the user:
5252
- How many files were reviewed
5353
- Which review prompt version was used
5454
- Remind the author to check the "Technical edit comments addressed" checkbox in the PR description when they're done addressing comments
55+
- Remind the author to reply to threads from the **Files changed** tab using **Start a review**, then submit once. Replying from the Conversation tab files each reply as its own standalone review and emails every PR subscriber separately.
56+
57+
Note that step 4 posts a single bundled review on purpose: one `POST /pulls/:pr/reviews` call with every comment in the `comments` array produces one notification. Never loop over `POST /pulls/:pr/comments` — that creates one standalone review per comment and floods every subscriber.

‎.github/pull_request_template.md‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44

55
## Affected documents
66

7+
<!-- List the documents this PR changes. Write "None — non-documentation change"
8+
for component, styling, script, workflow, or other site-plumbing work. -->
9+
710
-
811

912
## Related issue(s)
@@ -13,13 +16,19 @@
1316
## Pre-submission checklist
1417

1518
- [ ] I have previewed these changes locally or via the PR preview URL
16-
- [ ] My branch name uses one of the expected prefixes: `docs/new/`, `docs/major/`, `docs/minor/`, or `docs/fix/`
19+
- [ ] My branch name uses an expected prefix — documentation: `docs/new/`, `docs/major/`, `docs/minor/`, `docs/fix/`, `docs/dev/`; everything else: `feature/`, `fix/`, `chore/`, `ci/`
1720
- [ ] I have updated `00-version-history.mdx` if this change warrants a version entry
1821
- [ ] I have assigned a specific peer reviewer via the Reviewers sidebar (if known)
1922

2023
## Technical edit (Lanes 1, 2, and 3)
2124

22-
- [ ] Technical edit comments addressed — ready for Director review
25+
<!-- Check the box below only after every technical edit comment has been addressed.
26+
Lane 1 (new document) then advances to Director review.
27+
Lanes 2 and 3 (major/minor revision) advance straight to ready-to-merge —
28+
neither lane includes a Director review.
29+
Non-documentation PRs can ignore this section entirely. -->
30+
31+
- [ ] Technical edit comments addressed
2332

2433
## Notes for reviewers
2534

‎.github/workflows/ci-build.yml‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ jobs:
3333
pull_number: prNumber,
3434
per_page: 100,
3535
});
36+
// Branch naming for this path is conventional, not enforced:
37+
// feature/, fix/, chore/, ci/. What actually decides the lane is
38+
// content -- a PR touching nothing under docs/ has no review lane
39+
// and needs only this build to pass.
3640
const touchesDocs = files.some(f => f.filename.startsWith('docs/'));
3741
if (touchesDocs) return; // docs PRs are handled by stage-progression.yml
3842
@@ -51,7 +55,7 @@ jobs:
5155
5256
const marker = '<!-- ci-admin-merge-bot-comment -->';
5357
const sha = headSha.substring(0, 7);
54-
const body = `${marker}\n\n✅ **CI build passed** for commit \`${sha}\`\n\nThis PR does not touch documentation content under \`docs/\`, so no multi-stage review is required.\n\n@usace-rmc/docs-admin may merge.`;
58+
const body = `${marker}\n\n✅ **CI build passed** for commit \`${sha}\`\n\nThis PR changes no files under \`docs/\`, so it carries no review lane. **CI Build is the only gate** - there are no review stages to clear.\n\n@usace-rmc/docs-admin may merge.`;
5559
const comments = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber });
5660
const existing = comments.data.find(c => c.user.type === 'Bot' && c.body.includes(marker));
5761
if (existing) {

‎.github/workflows/stage-progression.yml‎

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,18 @@ jobs:
166166
if (users.length === 0) {
167167
lines.push(`- ${display}: _not assigned_`);
168168
} else {
169-
lines.push(`- ${display}: ${users.map(u => '@' + u).join(', ')}`);
169+
// Rendered WITHOUT an @ prefix on purpose. An @mention here
170+
// permanently subscribes that user to the PR, so every later
171+
// bot comment -- including stages they already finished --
172+
// emails them. This board is a status readout, not a call to
173+
// action; GitHub's Reviewers sidebar does the notifying.
174+
lines.push(`- ${display}: ${users.map(u => '`' + u + '`').join(', ')}`);
170175
}
171176
}
172177
lines.push('');
173178
lines.push('_The first approval from any assigned reviewer at the current stage advances the PR. Approvals from non-assigned reviewers are logged but do not advance the stage._');
179+
lines.push('');
180+
lines.push('_Names above are deliberately unlinked. An @mention would subscribe each person to every later comment on this PR; GitHub notifies assigned reviewers through the Reviewers sidebar instead._');
174181
return `${STATE_MARKER}\n${dataLine}\n\n${lines.join('\n')}`;
175182
}
176183
@@ -255,7 +262,7 @@ jobs:
255262
await setReviewStatus('pending', STAGE_STATUS_DESC['stage:needs-lane']);
256263
const reason = branch.startsWith('docs/')
257264
? `Branch \`${branch}\` doesn't match a known sub-prefix (\`docs/new/\`, \`docs/major/\`, \`docs/minor/\`, \`docs/fix/\`, \`docs/dev/\`), so the lane can't be auto-detected.`
258-
: `Branch \`${branch}\` doesn't follow the \`docs/{new,major,minor,fix,dev}/\` naming convention, and this PR modifies files under \`docs/\`.`;
265+
: `Branch \`${branch}\` doesn't follow the \`docs/{new,major,minor,fix,dev}/\` naming convention, and this PR modifies files under \`docs/\`. Branches for non-documentation work (\`feature/\`, \`fix/\`, \`chore/\`, \`ci/\`) skip the review lanes entirely, but only when the PR changes nothing under \`docs/\`.`;
259266
await postComment(`📋 **Lane assignment needed**\n\n${reason}\n\n@usace-rmc/docs-admin please apply a \`lane:*\` label to route this PR.`);
260267
return;
261268
}
@@ -282,7 +289,7 @@ jobs:
282289
await setReviewStatus('pending', STAGE_STATUS_DESC['stage:needs-lane']);
283290
const reason = branch.startsWith('docs/')
284291
? `Branch \`${branch}\` doesn't match a known sub-prefix.`
285-
: `Branch \`${branch}\` doesn't follow \`docs/{new,major,minor,fix,dev}/\` naming. This PR now modifies files under \`docs/\`.`;
292+
: `Branch \`${branch}\` doesn't follow \`docs/{new,major,minor,fix,dev}/\` naming. This PR now modifies files under \`docs/\`, so it can no longer skip the review lanes.`;
286293
await postComment(`📋 **Lane assignment needed**\n\n${reason}\n\n@usace-rmc/docs-admin please apply a \`lane:*\` label to route this PR.`);
287294
return;
288295
}
@@ -322,7 +329,7 @@ jobs:
322329
if (toNotify.length > 0) {
323330
const mentions = toNotify.map(u => '@' + u).join(', ');
324331
const stageDisplay = STAGE_DISPLAY[existingStage] || existingStage;
325-
await postComment(`🔄 **New commits pushed by @${prAuthor}.**\n\n${mentions} — please backcheck the revisions for **${stageDisplay}**.`);
332+
await postComment(`🔄 **New commits pushed by \`${prAuthor}\`.**\n\n${mentions} — please backcheck the revisions for **${stageDisplay}**.`);
326333
}
327334
}
328335
return;
@@ -416,14 +423,19 @@ jobs:
416423
// or when the author isn't around.
417424
// - Lane 1 (new-doc): advances to Director review
418425
// - Lanes 2 & 3 (major/minor revision): advances to ready-to-merge
419-
if (added === 'admin:advance-to-director') {
426+
//
427+
// `admin:technical-edit-complete` is the accurate name, because
428+
// only Lane 1 continues to a Director. The older
429+
// `admin:advance-to-director` is still accepted as a deprecated
430+
// alias so PRs already carrying it keep working.
431+
if (added === 'admin:technical-edit-complete' || added === 'admin:advance-to-director') {
420432
const lanesWithTechEdit = ['lane:new-doc', 'lane:major-revision', 'lane:minor-revision'];
421433
if (!lanesWithTechEdit.includes(existingLane) || existingStage !== 'stage:ai-editor-review') {
422-
await removeLabel('admin:advance-to-director');
423-
await postComment(`⚠️ **admin:advance-to-director** can only be applied to a PR currently at \`stage:ai-editor-review\` in a lane that includes technical edit. Label removed, no action taken.`);
434+
await removeLabel(added);
435+
await postComment(`⚠️ **${added}** can only be applied to a PR currently at \`stage:ai-editor-review\` in a lane that includes technical edit. Label removed, no action taken.`);
424436
return;
425437
}
426-
await removeLabel('admin:advance-to-director');
438+
await removeLabel(added);
427439
await removeLabel('stage:ai-editor-review');
428440
429441
if (existingLane === 'lane:new-doc') {
@@ -450,7 +462,7 @@ jobs:
450462
}
451463
await removeLabel('admin:approve-merge-after-push');
452464
await setReviewStatus('success', STAGE_STATUS_DESC['stage:ready-to-merge']);
453-
await postComment(`✅ **Post-push merge re-approved** by @${context.payload.sender.login}. The \`review-workflow\` status is back to success and the PR is mergeable.`);
465+
await postComment(`✅ **Post-push merge re-approved** by \`${context.payload.sender.login}\`. The \`review-workflow\` status is back to success and the PR is mergeable.`);
454466
return;
455467
}
456468
@@ -514,13 +526,13 @@ jobs:
514526
515527
if (reviewAction === 'submitted' && reviewState === 'changes_requested') {
516528
const stageDisplay = STAGE_DISPLAY[existingStage] || existingStage;
517-
await postComment(`📝 **Changes requested** by @${reviewer} at **${stageDisplay}**.\n\n@${prAuthor} please address the comments and push fixes. The stage will advance once an assigned reviewer approves.`);
529+
await postComment(`📝 **Changes requested** by \`${reviewer}\` at **${stageDisplay}**.\n\n@${prAuthor} please address the comments and push fixes. The stage will advance once an assigned reviewer approves.`);
518530
return;
519531
}
520532
521533
if (reviewAction === 'dismissed') {
522534
const dismissedBy = (context.payload.sender && context.payload.sender.login) || 'unknown';
523-
await postComment(`⚠️ **Review by @${reviewer} was dismissed** by @${dismissedBy}.\n\nThe stage label has not been changed automatically. @usace-rmc/docs-admin if this dismissal means a prior stage's approval is no longer valid, please manually revert the stage labels and request a re-review.`);
535+
await postComment(`⚠️ **Review by \`${reviewer}\` was dismissed** by \`${dismissedBy}\`.\n\nThe stage label has not been changed automatically. @usace-rmc/docs-admin - if this dismissal means a prior stage's approval is no longer valid, please manually revert the stage labels and request a re-review.`);
524536
return;
525537
}
526538
@@ -530,8 +542,8 @@ jobs:
530542
const state = await readState();
531543
const assigned = (state && state[existingStage]) || [];
532544
if (!assigned.includes(reviewer)) {
533-
const assignedList = assigned.length ? assigned.map(u => '@' + u).join(', ') : '_(none assigned yet)_';
534-
await postComment(`ℹ️ **Approval from @${reviewer} logged, but stage not advanced.**\n\n@${reviewer} is not in the list of assigned reviewers for **${STAGE_DISPLAY[existingStage] || existingStage}**. The stage will advance only when an approval is received from one of the assigned reviewers: ${assignedList}.\n\nIf @${reviewer} should be advancing the stage, @usace-rmc/docs-admin please assign them via the Reviewers sidebar and ask them to re-approve.`);
545+
const assignedList = assigned.length ? assigned.map(u => '`' + u + '`').join(', ') : '_(none assigned yet)_';
546+
await postComment(`ℹ️ **Approval from \`${reviewer}\` logged, but stage not advanced.**\n\n\`${reviewer}\` is not in the list of assigned reviewers for **${STAGE_DISPLAY[existingStage] || existingStage}**. The stage will advance only when an approval is received from one of the assigned reviewers: ${assignedList}.\n\n@usace-rmc/docs-admin - if \`${reviewer}\` should be advancing the stage, assign them via the Reviewers sidebar and ask them to re-approve.`);
535547
return;
536548
}
537549
@@ -540,26 +552,26 @@ jobs:
540552
if (existingLane === 'lane:new-doc') {
541553
if (existingStage === 'stage:peer-review') {
542554
nextStage = 'stage:lead-civil-review';
543-
comment = `✅ **Peer review approved** by @${reviewer}. Advancing to **RMC Lead Civil review**.\n\n@usace-rmc/docs-admin please assign the Lead Civil reviewer(s) via the Reviewers sidebar. The Lead Civil reviews on the preview URL.`;
555+
comment = `✅ **Peer review approved** by \`${reviewer}\`. Advancing to **RMC Lead Civil review**.\n\n@usace-rmc/docs-admin please assign the Lead Civil reviewer(s) via the Reviewers sidebar. The Lead Civil reviews on the preview URL.`;
544556
} else if (existingStage === 'stage:lead-civil-review') {
545557
nextStage = 'stage:ai-editor-review';
546-
comment = `✅ **Lead Civil review approved** by @${reviewer}. Advancing to **technical edit**.\n\n@usace-rmc/docs-admin please run the \`/technical-edit\` Claude Code skill against this PR (or assign a human technical editor). No live deploy is needed at this stage — see the Site Admin Workflow chapter of the Documentation Guide for the full sequence.`;
558+
comment = `✅ **Lead Civil review approved** by \`${reviewer}\`. Advancing to **technical edit**.\n\n@usace-rmc/docs-admin please run the \`/technical-edit\` Claude Code skill against this PR (or assign a human technical editor). No live deploy is needed at this stage — see the Site Admin Workflow chapter of the Documentation Guide for the full sequence.`;
547559
} else if (existingStage === 'stage:director-review') {
548560
nextStage = 'stage:ready-to-merge';
549-
comment = `✅ **Director review approved** by @${reviewer}. This PR is **ready for final merge and publication**.\n\n@usace-rmc/docs-admin please flip the \`draft\` flag, update \`00-version-history.mdx\`, merge to \`main\`, and approve the production deploy. See the Site Admin Workflow chapter of the Documentation Guide for the full sequence.`;
561+
comment = `✅ **Director review approved** by \`${reviewer}\`. This PR is **ready for final merge and publication**.\n\n@usace-rmc/docs-admin please flip the \`draft\` flag, update \`00-version-history.mdx\`, merge to \`main\`, and approve the production deploy. See the Site Admin Workflow chapter of the Documentation Guide for the full sequence.`;
550562
}
551563
} else if (existingLane === 'lane:major-revision') {
552564
if (existingStage === 'stage:peer-review') {
553565
nextStage = 'stage:lead-civil-review';
554-
comment = `✅ **Peer review approved** by @${reviewer}. Advancing to **RMC Lead Civil review**.\n\n@usace-rmc/docs-admin please assign the Lead Civil reviewer(s) via the Reviewers sidebar.`;
566+
comment = `✅ **Peer review approved** by \`${reviewer}\`. Advancing to **RMC Lead Civil review**.\n\n@usace-rmc/docs-admin please assign the Lead Civil reviewer(s) via the Reviewers sidebar.`;
555567
} else if (existingStage === 'stage:lead-civil-review') {
556568
nextStage = 'stage:ai-editor-review';
557-
comment = `✅ **Lead Civil review approved** by @${reviewer}. Advancing to **technical edit**.\n\n@usace-rmc/docs-admin please run the \`/technical-edit\` Claude Code skill against this PR (or assign a human technical editor).`;
569+
comment = `✅ **Lead Civil review approved** by \`${reviewer}\`. Advancing to **technical edit**.\n\n@usace-rmc/docs-admin please run the \`/technical-edit\` Claude Code skill against this PR (or assign a human technical editor).`;
558570
}
559571
} else if (existingLane === 'lane:minor-revision') {
560572
if (existingStage === 'stage:peer-review') {
561573
nextStage = 'stage:ai-editor-review';
562-
comment = `✅ **Peer review approved** by @${reviewer}. Advancing to **technical edit**.\n\n@usace-rmc/docs-admin please run the \`/technical-edit\` Claude Code skill against this PR (or assign a human technical editor).`;
574+
comment = `✅ **Peer review approved** by \`${reviewer}\`. Advancing to **technical edit**.\n\n@usace-rmc/docs-admin please run the \`/technical-edit\` Claude Code skill against this PR (or assign a human technical editor).`;
563575
}
564576
}
565577

0 commit comments

Comments
 (0)