Skip to content

Improve skill workflows: advisory reviews, version coverage, pattern consolidation#217

Merged
igerber merged 3 commits intomainfrom
skill-workflow-updates
Mar 20, 2026
Merged

Improve skill workflows: advisory reviews, version coverage, pattern consolidation#217
igerber merged 3 commits intomainfrom
skill-workflow-updates

Conversation

@igerber
Copy link
Owner

@igerber igerber commented Mar 20, 2026

Summary

  • Change review agent language from approval verdicts (Ready/Needs revision) to advisory-only assessments (No critical issues found/Significant issues found) — only the user should approve plans
  • Add docs/llms-full.txt to /bump-version skill so LLM discoverability doc tracks version number
  • Consolidate duplicated methodology pattern checks (A-D) and secret scanning regexes into canonical definitions in /pre-merge-check (Sections 2.1, 2.6), replacing inline copies in /submit-pr and /push-pr-update
  • Upgrade /submit-pr and /push-pr-update from checks A-B to full A-D coverage
  • Add REGISTRY.md auto-check to /submit-pr and /push-pr-update to catch undocumented methodology deviations

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no methodology changes
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: No test changes (existing test-check-plan-review.sh passes all 10 cases)
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@igerber @claude
Improve skill workflows: advisory-only reviews, version coverage, pat…
2f8d2b2 
…tern consolidation

- Change review agent language from approval verdicts to advisory assessments
  (verdict→assessment, "Ready"→"No critical issues found", etc.)
- Add docs/llms-full.txt to bump-version skill version locations
- Consolidate duplicated pattern checks and secret scanning regexes into
  canonical definitions in pre-merge-check.md (Sections 2.1, 2.6)
- Upgrade submit-pr and push-pr-update from checks A-B to full A-D coverage
- Add REGISTRY.md auto-check to submit-pr and push-pr-update

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

github-actions bot commented Mar 20, 2026

Overall Assessment
Needs changes.

This PR does not touch estimator implementations, weighting, variance formulas, inference code, identification checks, or user-facing defaults in diff_diff/, so I found no methodology mismatch against docs/methodology/REGISTRY.md. The blocking issues are workflow regressions in the PR automation docs: one main /push-pr-update path never runs the new methodology safeguards, and the secret-scan refactor removed the concrete capture steps that the later warning branches still rely on.

Executive Summary

  • push-pr-update only applies the new A-D methodology checks and REGISTRY.md warning on the staged-changes path, not on the clean-working-tree / already-committed path.
  • The secret-scan steps in submit-pr and push-pr-update still refer to secret_files / sensitive_files after removing the commands that populate them.
  • No estimator or methodology code changed, so the Methodology Registry check is not implicated beyond confirming scope.
  • The verdict -> assessment rename is internally consistent across the touched plan-review docs and hook fixture.
  • TODO.md does not track either workflow gap, so both are unmitigated.

Methodology

  • None. I checked docs/methodology/REGISTRY.md; this PR only changes .claude/ workflow docs, a hook test, and CLAUDE.md, so there is no estimator/source-material mismatch to flag.

Code Quality

  • Severity: P1. Impact: the new methodology guardrails in /push-pr-update are only wired into Section 3, which runs when there are local changes to stage. When git status --porcelain is empty and the branch is already ahead, the workflow jumps to Section 3a and then straight to push, so already-committed methodology changes never get the new A-D checks or the REGISTRY.md warning. That leaves one of the two main push paths without the very coverage this PR is adding. References: .claude/commands/push-pr-update.md:L53-L107, .claude/commands/push-pr-update.md:L116-L145. Concrete fix: add the same methodology-file detection, A-D checks, and REGISTRY.md warning to the clean-working-tree / ahead-commits path using git diff --name-only <comparison-ref>..HEAD, or factor that logic into a shared subsection used by both paths.

Performance

  • None.

Maintainability

  • None. The verdict -> assessment rename appears internally consistent across the touched plan-review workflow files.

Tech Debt

  • None. TODO.md does not contain a matching entry that would mitigate the workflow gaps above, so they still count toward the assessment. Reference: TODO.md:L31-L62.

Security

  • Severity: P2. Impact: the secret-scan refactor centralizes the regex definitions, but it also removes the concrete secret_files=$(...) / sensitive_files=$(...) capture steps while the later branches still say “if secret_files / sensitive_files is non-empty.” That leaves the warning/unstage flow under-specified in both PR workflows and makes it easy for an implementation to skip or underreport detections. References: .claude/commands/submit-pr.md:L169-L196, .claude/commands/push-pr-update.md:L94-L107, .claude/commands/push-pr-update.md:L153-L170. Concrete fix: keep the canonical patterns in /pre-merge-check, but restore explicit variable-assignment snippets in each workflow and use those variables in the warning branches.

Documentation/Tests

  • Severity: P3. Impact: the PR changes operational behavior in the bump-version, submit-pr, and push-pr-update workflows, but the only updated test is the plan-review hook fixture. There is no regression coverage for the new canonical cross-file references or the added docs/llms-full.txt version-sync step, so prompt-level regressions like the two above are easy to miss. References: .claude/commands/bump-version.md:L17-L105, .claude/commands/submit-pr.md:L129-L196, .claude/commands/push-pr-update.md:L90-L170, .claude/hooks/test-check-plan-review.sh:L39-L54. Concrete fix: add a lightweight lint/snapshot test that validates both PR workflows still define concrete secret-scan snippets, that both /push-pr-update paths run methodology checks, and that /bump-version includes docs/llms-full.txt.
  • Validation note: I could not execute .claude/hooks/test-check-plan-review.sh in this environment because the sandbox is read-only and the script requires a writable temp directory.

Path to Approval

  1. In /push-pr-update, run the A-D methodology checks and the REGISTRY.md warning on the clean-working-tree / ahead-commits path before pushing, not just on the staged-changes path.
  2. In /submit-pr and /push-pr-update, restore concrete secret_files and sensitive_files capture commands based on the canonical patterns from /pre-merge-check, then use those variables in the warning branches.
  3. Add minimal regression coverage or linting for the modified workflow specs so these prompt-level contract breaks are caught automatically.

@igerber @claude
Address PR #217 review feedback: methodology checks on clean-tree pat…
b6e3633 
…h, restore secret scan variables

- Add Section 3b methodology checks (A-D patterns + REGISTRY.md warning)
  for already-committed changes that bypass Section 3
- Update routing references in Section 2.4 to flow through 3a → 3b → 4
- Restore explicit secret_files/sensitive_files variable capture in all
  three secret scan locations across push-pr-update.md and submit-pr.md
- Fix step numbering gap in Section 3a (was 1,2,4 → now 1,2,3)
- Normalize variable reference in submit-pr.md line 180

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

igerber commented Mar 20, 2026

/ai-review

@github-actions
Copy link

github-actions bot commented Mar 20, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: b6e3633b3415ec7ef5e30128cf42e96e4d213eb8

Overall Assessment

⚠️ Needs changes.

This re-review clears the earlier secret-scan regression and mostly fixes the clean-tree /push-pr-update gap, but the new Section 3b still does not fully deliver the promised A-D methodology coverage. Check C remains hardcoded to git diff HEAD, so it cannot detect newly committed __init__ parameters on the already-committed push path.

Executive Summary

Methodology

Code Quality

  • Severity: P1. Impact: /push-pr-update now says the clean-working-tree / ahead-commits path will run all four methodology checks in Section 3b, but Check C’s canonical definition still extracts new self.<param> assignments via git diff HEAD -- <changed-methodology-files>. On this path the working tree is clean, so that diff is empty and the project’s “new __init__ param missing downstream” anti-pattern can slip through on already-committed methodology changes. References: .claude/commands/push-pr-update.md:L114, .claude/commands/push-pr-update.md:L123, .claude/commands/pre-merge-check.md:L48, .claude/commands/pre-merge-check.md:L50. Concrete fix: parameterize Check C by diff base and use git diff <comparison-ref>..HEAD -- <changed-methodology-files> for Section 3b, or override Check C explicitly in Section 3b before checking get_params() coverage.

Performance

  • None.

Maintainability

Tech Debt

  • None. There is no matching tracked item under TODO.md:L38, and the remaining P1 would not be mitigated by TODO tracking anyway.

Security

Documentation/Tests

  • Severity: P3. Impact: The PR changes /submit-pr, /push-pr-update, and /bump-version, but the only updated regression artifact is the plan-review hook fixture. There is still no automated guardrail for the new Section 3b committed-changes path, which is why the Check C diff-base bug above is easy to miss. References: .claude/commands/bump-version.md:L27, .claude/commands/push-pr-update.md:L114, .claude/commands/submit-pr.md:L136, .claude/hooks/test-check-plan-review.sh:L39. Concrete fix: add a lightweight lint/snapshot test that asserts the already-committed /push-pr-update path uses the correct diff base for Check C and that /bump-version includes docs/llms-full.txt.
  • Validation note: I could not run .claude/hooks/test-check-plan-review.sh in this sandbox because mktemp -d failed on the read-only filesystem.

Path to Approval

  1. In .claude/commands/push-pr-update.md:L123, stop using the unparameterized Check C from .claude/commands/pre-merge-check.md:L50 for the already-committed path. Run Check C against <comparison-ref>..HEAD so newly committed self.<param> assignments are actually discoverable before push.
  2. Add a regression check that covers Section 3b specifically, asserting that the clean-tree/ahead-commits path exercises A-D against the committed diff range rather than HEAD.

@igerber @claude
Fix Check C diff base for already-committed path in push-pr-update
850e5e5 
Add explicit override instructing executor to use git diff
<comparison-ref>..HEAD instead of git diff HEAD when running Check C
on Section 3b's clean-tree path, so new self.X assignments in
committed diffs are properly detected.

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

igerber commented Mar 20, 2026

/ai-review

@github-actions
Copy link

github-actions bot commented Mar 20, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: 850e5e57cc884fca12390f9257819504bf8372b8

Overall Assessment

Looks good.

This re-review resolves the prior P1 on the clean-tree/ahead-commits /push-pr-update path: the already-committed methodology flow now explicitly overrides Check C to diff against the committed range rather than HEAD in .claude/commands/push-pr-update.md:L123-L127, consistent with the canonical Check C definition in .claude/commands/pre-merge-check.md:L48-L55. I did not find any new unmitigated P0/P1 issues in the changed files.

Executive Summary

Methodology

None. No methodology-critical library code changed under diff_diff/. I also checked the new REGISTRY warning labels against existing reviewer-recognized labels in docs/methodology/REGISTRY.md:L238 and docs/methodology/REGISTRY.md:L1117, and found no mismatch.

Code Quality

None.

Performance

None.

Maintainability

None.

Tech Debt

None.

Security

None.

Documentation/Tests

  • Severity: P3
  • Impact: The PR adds the new already-committed methodology-check flow in /push-pr-update and broadens staged methodology checks in both PR workflows, but the touched automated coverage remains the plan-review hook fixture. That leaves the new workflow paths without automated guardrails and makes future instruction drift easier to miss; see .claude/commands/push-pr-update.md:L114-L140, .claude/commands/submit-pr.md:L136-L175, and .claude/hooks/test-check-plan-review.sh:L39-L176.
  • Concrete fix: Add a lightweight snapshot/lint-style test that asserts the clean-tree/ahead-commits /push-pr-update path uses Check C against <comparison-ref>..HEAD, and that staged /submit-pr and /push-pr-update both reference A-D plus the REGISTRY.md warning.

Validation note: I could not meaningfully run .claude/hooks/test-check-plan-review.sh in this sandbox because /tmp is mounted read-only, so mktemp -d failed during test setup.

@igerber igerber merged commit 747a598 into main Mar 20, 2026
@igerber igerber deleted the skill-workflow-updates branch March 20, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igerber