feat: align update-docs skill with SOP -- 10 steps, verification-before-skip

forgejo_admin commented

2026-03-21 17:07:24 +00:00

Author

Contributor

PR #140 Review

DOMAIN REVIEW

Domain: Agent skill definition (markdown instruction file for Claude Code /update-docs command). No executable code, no compiled language, no tests applicable. Review focuses on correctness of instructions, internal consistency, alignment with platform conventions, and dependency safety.

Findings:

Verification-Before-Skip pattern is well-designed. Every step now mandates exactly one of UPDATED, VERIFIED CURRENT, or NOT APPLICABLE (with reason). This eliminates ambiguous "SKIPPED" outcomes. The old file had 6 instances of "skip" language -- all removed in the new version.
Step 6/7 reordering is correct. Old flow was: move item (Step 5), then sync (Step 6). New flow: sync first (Step 6), then move (Step 7). This is the right order -- sync reconciles drift against Forgejo before the board item move is applied to a clean board state.
Step 8 (Memory) correctly scoped to behavioral lessons. The old Step 7 told agents to write project state (phase completion, current state) into MEMORY.md. The new Step 8 restricts to behavioral knowledge only and reports NOT APPLICABLE for project state. This aligns with feedback_memory_scope.md.
Old Step 8 (todo note creation) correctly removed. The old step used mcp__pal-e-docs__create_note with note_type: "todo", which is now hard-blocked by hooks/check-note-template.sh (lines 28-39). The old instruction would have been denied at runtime. Replacing it with Steps 9 and 10 (Forgejo issues) is the correct path.
Reference footer updated from skill-update-docs to sop-post-merge-docs. Correct -- the canonical source callout at the top establishes the SOP as authoritative.
Step 9 references template-issue-nit-bundle -- this template does not currently exist in the codebase. PR #139 (issue #137) appears to be the companion PR that adds nit-bundle support to the issue template hook. This creates a dependency: if #140 merges first, Step 9 will reference a template that does not exist yet. This is not a blocker because the step instructs the agent to "create a Forgejo issue using template-issue-nit-bundle" -- at worst, the agent creates a non-templated issue. But the merge order matters for clean execution. Recommend noting this dependency.
Step 2 references sop-ci-pipeline-recovery -- this SOP is referenced by slug. If it does not exist in pal-e-docs yet, Step 2's failure path ("follow sop-ci-pipeline-recovery") would leave the agent with no instructions. Worth confirming the SOP exists.
Canonical source callout is a strong pattern. The callout "If this file and the SOP diverge, the SOP wins" is exactly right for a skill that implements an SOP. This prevents drift arguments.

BLOCKERS

None. This is a markdown instruction file, not executable code. The BLOCKER criteria (test coverage, input validation, secrets, DRY auth) do not apply to non-executable documentation.

NITS

Step 2 bash snippet is incomplete. The code block shows source ~/secrets/pal-e-services/forgejo.env and a comment # Use mcp__woodpecker__list_pipelines or curl to check the latest pipeline on main but does not provide the actual curl command or MCP tool call. Every other step that uses bash (Step 1) provides the full command. Consider providing the concrete mcp__woodpecker__list_pipelines call or a curl example for consistency.
Step 9 body format uses \\\\n (escaped newline). The instruction says to include ### Type\\nNit-Bundle in the issue body. The double-backslash may confuse agents about whether this is a literal \n string or a newline. Consider using a code block with actual formatting instead.
Completion summary template could be copy-paste ready. The summary shows | NOT APPLICABLE -- no CI alternatives inline with pipes, which is clear for human readers but might cause agents to emit the pipe character literally. Minor -- agents are generally good at interpreting "option A | option B" patterns.
PR #139 dependency not mentioned in Related section. If Step 9's template-issue-nit-bundle depends on #139 merging first, the Related section should note "Depends on #139" or "Companion to #139."

SOP COMPLIANCE

Branch named after issue: 138-align-update-docs-skill-with-sop-add-mis starts with issue #138
PR body follows template: Summary, Changes, Test Plan, Related all present
Related references plan slug: Related section says "Closes #138" but does not reference plan-pal-e-agency
No secrets committed: Single markdown file, no credentials
No unnecessary file changes: 1 file changed, tightly scoped to the update-docs skill
Commit messages are descriptive: Title is clear

PROCESS OBSERVATIONS

Deployment Frequency: This is a config/skill file change. No CI pipeline, no deployment artifact. Merging is effectively deploying since agents read this file from the repo directly.

Change Failure Risk: Low. The verification-before-skip pattern is strictly better than the old "skip if X" pattern. The main risk is Step 9/10 referencing templates or SOPs that do not yet exist, but agents handle missing references gracefully (they will ask or report NOT APPLICABLE).

Documentation: The PR body is thorough -- the Changes section itemizes every step with what changed and why. The review checklist in the PR body is a strong practice.

Merge Order Dependency: PR #139 (nit-bundle hook) and #140 (update-docs rewrite) should ideally merge in the order #139 then #140, so the template exists before the skill references it. Not a hard blocker but worth noting.

VERDICT: APPROVED

The rewrite correctly aligns the skill with the 10-step SOP, removes deprecated todo-note creation that would be blocked at runtime anyway, introduces the verification-before-skip pattern consistently, and reorders sync/move correctly. The missing plan slug in Related and the dependency on #139 are worth noting but not blocking. Clean, well-scoped PR.

## PR #140 Review ### DOMAIN REVIEW **Domain:** Agent skill definition (markdown instruction file for Claude Code `/update-docs` command). No executable code, no compiled language, no tests applicable. Review focuses on correctness of instructions, internal consistency, alignment with platform conventions, and dependency safety. **Findings:** 1. **Verification-Before-Skip pattern is well-designed.** Every step now mandates exactly one of UPDATED, VERIFIED CURRENT, or NOT APPLICABLE (with reason). This eliminates ambiguous "SKIPPED" outcomes. The old file had 6 instances of "skip" language -- all removed in the new version. 2. **Step 6/7 reordering is correct.** Old flow was: move item (Step 5), then sync (Step 6). New flow: sync first (Step 6), then move (Step 7). This is the right order -- sync reconciles drift against Forgejo before the board item move is applied to a clean board state. 3. **Step 8 (Memory) correctly scoped to behavioral lessons.** The old Step 7 told agents to write project state (phase completion, current state) into MEMORY.md. The new Step 8 restricts to behavioral knowledge only and reports NOT APPLICABLE for project state. This aligns with `feedback_memory_scope.md`. 4. **Old Step 8 (todo note creation) correctly removed.** The old step used `mcp__pal-e-docs__create_note` with `note_type: "todo"`, which is now hard-blocked by `hooks/check-note-template.sh` (lines 28-39). The old instruction would have been denied at runtime. Replacing it with Steps 9 and 10 (Forgejo issues) is the correct path. 5. **Reference footer updated from `skill-update-docs` to `sop-post-merge-docs`.** Correct -- the canonical source callout at the top establishes the SOP as authoritative. 6. **Step 9 references `template-issue-nit-bundle`** -- this template does not currently exist in the codebase. PR #139 (issue #137) appears to be the companion PR that adds nit-bundle support to the issue template hook. This creates a dependency: if #140 merges first, Step 9 will reference a template that does not exist yet. This is not a blocker because the step instructs the agent to "create a Forgejo issue using template-issue-nit-bundle" -- at worst, the agent creates a non-templated issue. But the merge order matters for clean execution. Recommend noting this dependency. 7. **Step 2 references `sop-ci-pipeline-recovery`** -- this SOP is referenced by slug. If it does not exist in pal-e-docs yet, Step 2's failure path ("follow sop-ci-pipeline-recovery") would leave the agent with no instructions. Worth confirming the SOP exists. 8. **Canonical source callout is a strong pattern.** The callout "If this file and the SOP diverge, the SOP wins" is exactly right for a skill that implements an SOP. This prevents drift arguments. ### BLOCKERS None. This is a markdown instruction file, not executable code. The BLOCKER criteria (test coverage, input validation, secrets, DRY auth) do not apply to non-executable documentation. ### NITS 1. **Step 2 bash snippet is incomplete.** The code block shows `source ~/secrets/pal-e-services/forgejo.env` and a comment `# Use mcp__woodpecker__list_pipelines or curl to check the latest pipeline on main` but does not provide the actual curl command or MCP tool call. Every other step that uses bash (Step 1) provides the full command. Consider providing the concrete `mcp__woodpecker__list_pipelines` call or a curl example for consistency. 2. **Step 9 body format uses `\\\\n` (escaped newline).** The instruction says to include `### Type\\nNit-Bundle` in the issue body. The double-backslash may confuse agents about whether this is a literal `\n` string or a newline. Consider using a code block with actual formatting instead. 3. **Completion summary template could be copy-paste ready.** The summary shows `| NOT APPLICABLE -- no CI` alternatives inline with pipes, which is clear for human readers but might cause agents to emit the pipe character literally. Minor -- agents are generally good at interpreting "option A | option B" patterns. 4. **PR #139 dependency not mentioned in Related section.** If Step 9's `template-issue-nit-bundle` depends on #139 merging first, the Related section should note "Depends on #139" or "Companion to #139." ### SOP COMPLIANCE - [x] Branch named after issue: `138-align-update-docs-skill-with-sop-add-mis` starts with issue #138 - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [ ] Related references plan slug: Related section says "Closes #138" but does not reference `plan-pal-e-agency` - [x] No secrets committed: Single markdown file, no credentials - [x] No unnecessary file changes: 1 file changed, tightly scoped to the update-docs skill - [x] Commit messages are descriptive: Title is clear ### PROCESS OBSERVATIONS **Deployment Frequency:** This is a config/skill file change. No CI pipeline, no deployment artifact. Merging is effectively deploying since agents read this file from the repo directly. **Change Failure Risk:** Low. The verification-before-skip pattern is strictly better than the old "skip if X" pattern. The main risk is Step 9/10 referencing templates or SOPs that do not yet exist, but agents handle missing references gracefully (they will ask or report NOT APPLICABLE). **Documentation:** The PR body is thorough -- the Changes section itemizes every step with what changed and why. The review checklist in the PR body is a strong practice. **Merge Order Dependency:** PR #139 (nit-bundle hook) and #140 (update-docs rewrite) should ideally merge in the order #139 then #140, so the template exists before the skill references it. Not a hard blocker but worth noting. ### VERDICT: APPROVED The rewrite correctly aligns the skill with the 10-step SOP, removes deprecated todo-note creation that would be blocked at runtime anyway, introduces the verification-before-skip pattern consistently, and reorders sync/move correctly. The missing plan slug in Related and the dependency on #139 are worth noting but not blocking. Clean, well-scoped PR.

2026-03-21 17:04:41 +00:00

Summary

Rewrites the /update-docs skill to match all 10 steps from sop-post-merge-docs. Adds three missing steps (verify deploy, nit-bundle creation, cross-pillar impact check) and replaces all "skip this step" language with verification-before-skip -- every step must report UPDATED, VERIFIED CURRENT, or NOT APPLICABLE (reason).

Changes

commands/update-docs.md -- complete rewrite:
- Added canonical source callout pointing to sop-post-merge-docs
- Added Verification-Before-Skip Rule section at the top
- Step 1 (Close issue): kept, added explicit VERIFIED CLOSED report format
- Step 2 (Verify deploy): NEW -- checks Woodpecker pipeline, stops on failure
- Step 3 (Update phase note): kept from old step 2, added verification language
- Step 4 (Update plan note): kept from old step 3, added verification language
- Step 5 (Update project page): REWRITTEN -- must READ before reporting, checks Status + Issues table
- Step 6 (Sync board): reordered before board item move, reports created/updated/skipped counts
- Step 7 (Move board item): reordered after sync, verification-aware
- Step 8 (Update memory): REWRITTEN -- behavioral lessons only, project state = NOT APPLICABLE
- Step 9 (Nit-bundle): NEW -- bundles QA nits into single Forgejo issue, references plan Epilogue
- Step 10 (Cross-pillar impact): NEW -- creates typed issue on target pillar repo
- Removed old step 8 (Capture Discovered Scope / todo notes) -- replaced by steps 9+10
- Updated completion summary to 10-step format with explicit outcome options
- Reference footer updated to point to sop-post-merge-docs

Test Plan

Run /update-docs after a real PR merge and verify all 10 steps execute
Confirm no step reports "SKIPPED" -- only UPDATED, VERIFIED CURRENT, or NOT APPLICABLE
Verify Step 5 reads the project page before reporting (not a blind skip)
Verify Step 2 checks Woodpecker for CI-enabled repos
Verify Step 9 creates nit-bundle issue when QA nits exist

Review Checklist

All 10 SOP steps present and numbered correctly
No "skip" language remains (grep confirms only "Verification-Before-Skip" rule name)
Completion summary matches the prescribed format exactly
Gather Context preamble preserved
Reference footer points to sop-post-merge-docs
Canonical source callout added

Closes #138

## Summary Rewrites the `/update-docs` skill to match all 10 steps from `sop-post-merge-docs`. Adds three missing steps (verify deploy, nit-bundle creation, cross-pillar impact check) and replaces all "skip this step" language with verification-before-skip -- every step must report UPDATED, VERIFIED CURRENT, or NOT APPLICABLE (reason). ## Changes - `commands/update-docs.md` -- complete rewrite: - Added canonical source callout pointing to `sop-post-merge-docs` - Added Verification-Before-Skip Rule section at the top - Step 1 (Close issue): kept, added explicit VERIFIED CLOSED report format - Step 2 (Verify deploy): **NEW** -- checks Woodpecker pipeline, stops on failure - Step 3 (Update phase note): kept from old step 2, added verification language - Step 4 (Update plan note): kept from old step 3, added verification language - Step 5 (Update project page): **REWRITTEN** -- must READ before reporting, checks Status + Issues table - Step 6 (Sync board): reordered before board item move, reports created/updated/skipped counts - Step 7 (Move board item): reordered after sync, verification-aware - Step 8 (Update memory): **REWRITTEN** -- behavioral lessons only, project state = NOT APPLICABLE - Step 9 (Nit-bundle): **NEW** -- bundles QA nits into single Forgejo issue, references plan Epilogue - Step 10 (Cross-pillar impact): **NEW** -- creates typed issue on target pillar repo - Removed old step 8 (Capture Discovered Scope / todo notes) -- replaced by steps 9+10 - Updated completion summary to 10-step format with explicit outcome options - Reference footer updated to point to `sop-post-merge-docs` ## Test Plan - [ ] Run `/update-docs` after a real PR merge and verify all 10 steps execute - [ ] Confirm no step reports "SKIPPED" -- only UPDATED, VERIFIED CURRENT, or NOT APPLICABLE - [ ] Verify Step 5 reads the project page before reporting (not a blind skip) - [ ] Verify Step 2 checks Woodpecker for CI-enabled repos - [ ] Verify Step 9 creates nit-bundle issue when QA nits exist ## Review Checklist - [x] All 10 SOP steps present and numbered correctly - [x] No "skip" language remains (grep confirms only "Verification-Before-Skip" rule name) - [x] Completion summary matches the prescribed format exactly - [x] Gather Context preamble preserved - [x] Reference footer points to `sop-post-merge-docs` - [x] Canonical source callout added ## Related - Closes #138

forgejo_admin added 1 commit

feat: align update-docs skill with SOP — 10 steps, verification-before-skip 3b89d7d850

The update-docs skill now matches all 10 steps from sop-post-merge-docs.
Adds missing steps: verify deploy (2), nit-bundle creation (9), and
cross-pillar impact check (10). Replaces 'skip this step' with
verification-before-skip — every step reports UPDATED, VERIFIED CURRENT,
or NOT APPLICABLE (reason). No naked skips.

Closes #138

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>