ci(skillsaw): add lint job, Makefile targets, and repo config - #754
ci(skillsaw): add lint job, Makefile targets, and repo config#754ralphbean wants to merge 6 commits into
Conversation
PR Summary by QodoCI: run skillsaw lint in strict mode and add local Make targets
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
🤖 Finished Review · ✅ Success · Started 5:01 PM UTC · Completed 5:17 PM UTC Commit: |
Code Review by Qodo
1.
|
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
|
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep: 4 findings (2 HIGH, 2 MEDIUM) posted as inline comments.
|
These three all landed as separate inline threads — handled them there (protected-path is intentional, indentation fixed, strict-mode comment reworded). |
|
Went through these: Doc gaps — added Scope vs #188 — I see the issue only asked for evaluation, but I'd count the PR itself as the evaluation: building the strict lint job out and watching it catch real issues in review (the ones fixed inline on this PR) is the signal that it finds meaningful problems and is worth running ongoing. Comfortable moving straight to CI integration rather than splitting that into a separate PR. persist-credentials — answered on the dedicated thread for that one. PR title — keeping |
|
🤖 Finished Review · ✅ Success · Started 1:16 PM UTC · Completed 1:32 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep: 2 findings (1 HIGH, 1 MEDIUM) posted as inline comments.
b703e8c to
c2ab7a5
Compare
|
🤖 Finished Review · ✅ Success · Started 9:39 PM UTC · Completed 9:54 PM UTC Commit: |
|
🤖 Finished Fix · ✅ Success · Started 9:55 PM UTC · Completed 10:01 PM UTC Commit: |
🔧 Fix agent — iteration 1 (bot-triggered)Addressed 2 of 3 review findings. Fixed invalid uvx syntax in Makefile (== to @) and added lint.yml to README Workflows table. Disagreed with 1 finding (persist-credentials consistency) per reviewer's own suggestion to handle in a follow-up PR. Fixed (2):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:02 PM UTC · Completed 10:14 PM UTC Commit: |
Superseded by updated review
|
🤖 Finished Review · ✅ Success · Started 11:32 PM UTC · Completed 11:45 PM UTC Commit: |
|
🤖 Finished Review · ✅ Success · Started 12:54 AM UTC · Completed 1:05 AM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
All HIGH/MEDIUM findings resolved: marketplace-disable comment now cites the verified real cause, the skillsaw action is pinned to the immutable release commit, context-budget uses the baseline/ratchet mechanism instead of a blanket threshold bump, and rule IDs use the canonical claude- prefix. The composite-action pinning gap and persist-credentials inconsistency are acknowledged, documented follow-ups. CI (including the new skillsaw job) is green.
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 2 MEDIUM findings posted as inline comments.
ac6bb14 to
3bf1f29
Compare
|
🤖 Finished Review · ✅ Success · Started 3:38 PM UTC · Completed 3:53 PM UTC Commit: |
3bf1f29 to
ccfa097
Compare
|
🤖 Finished Review · ✅ Success · Started 4:10 PM UTC · Completed 4:26 PM UTC Commit: |
ccfa097 to
ad49949
Compare
|
🤖 Review · Commit: |
Adds a skillsaw job to the lint workflow, running the linter in --strict mode on every PR and push to main. Adds `make lint` and `make lint-fix` targets so contributors can run the same checks locally. Adds .skillsaw.yaml, tuned for this repo: - Disables marketplace-json-valid and marketplace-registration. plugins/gopls-lsp is an internal fullsend harness plugin config (see harness/code.yaml), not a Claude Code marketplace plugin, so these rules produced false positives. - Raises the context-budget skill thresholds to warn 16000 / error 20000, above code-implementation and pr-review's current sizes. Both are dense, production-tuned orchestration procedures; --strict turns warnings into failures, so the thresholds are set high enough that neither file triggers a violation at either severity. This builds on the skill content fixes in the preceding PRs in this stack, evaluating skillsaw per #188. Related to #188 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Pin skillsaw action to the exact commit SHA (v0.18.0) instead of the v0 tag object, matching the repo's pinact convention. - Fix .skillsaw.yaml exclude block indentation to match the rest of the file. - Correct the marketplace-json-valid/marketplace-registration disable comment: it wasn't gopls-lsp/plugin.json causing the failure, it's that this repo has no .claude-plugin/marketplace.json and isn't a distributable plugin marketplace. - Clarify why strict: false in .skillsaw.yaml doesn't matter for CI. - Document make lint / make lint-fix in README and CONTRIBUTING. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…mment v0.18.0 renamed the Claude Code format rules to claude--prefixed canonical IDs; the checked-in config still used the pre-rename legacy names. Not a functional bug (skillsaw resolves the aliases), but regenerate the keys to match what skillsaw init produces today. Also fix the disabled marketplace-json-valid/marketplace-registration comment, which still cited an already-debunked root cause (gopls-lsp misdetection) instead of the verified one: no top-level .claude-plugin/marketplace.json exists in this repo. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…limit Raising context-budget.limits.skill to warn 16000/error 20000 repo-wide was meant to clear pr-review and code-implementation, but it silently raised the ceiling for every skill file (present or future) and, per review, missed that 3 other files (code-review, fix-review, retro-analysis) were already over the default warn threshold too. skillsaw ships a baseline/ratchet mechanism (baseline_mode: ceiling) built exactly for this: snapshot each currently oversized file's token count as its own ceiling, so it only fails again if it grows past its current size, while every other file keeps the strict default (warn 3000 / error 6000). Revert the limit to defaults and commit .skillsaw-baseline.json with the 5 affected files snapshotted. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
CI's skillsaw job scans the pull_request merge commit (head merged into base, per actions/checkout's default ref), not the head branch alone. This PR's base (skillsaw/fix-pr-review) has a slightly different pr-review/SKILL.md than what's on this branch, so the baseline snapshotted from a plain head-branch checkout undercounted its tokens (15,051 vs the merge tree's actual 15,058) and tripped the ratchet in CI. Regenerated by baselining against refs/pull/754/merge directly. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
ad49949 to
e15628c
Compare
|
🤖 Finished Review · ✅ Success · Started 6:56 PM UTC · Completed 7:34 PM UTC Commit: |
| context-budget: | ||
| enabled: auto | ||
| severity: warning | ||
| limits: |
There was a problem hiding this comment.
[medium] PR body / config mismatch
The PR body states: 'Raises the context-budget skill thresholds to warn 16000 / error 20000.' However, the actual .skillsaw.yaml uses default thresholds (all sub-keys under limits: are commented out). The violations for pr-review (15,058 tokens) and code-implementation (9,116 tokens) are suppressed via the .skillsaw-baseline.json baseline file with ceiling mode, not by raised thresholds. If someone later removes the baseline file expecting the config to have raised thresholds, both skills will fail the lint check.
Suggested fix: Either update the PR body to accurately describe the baseline suppression mechanism, or set the thresholds in .skillsaw.yaml as described.
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| persist-credentials: false |
There was a problem hiding this comment.
[low] consistency
The skillsaw job sets persist-credentials: false on its checkout step, while the test and commit-lint jobs in the same file omit this parameter. Since persist-credentials: false is a security hardening measure, the preferred resolution is to add it to the other two jobs in a follow-up PR rather than removing it here.
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep — 2 findings posted inline (both MEDIUM). Not approving/requesting changes; leaving disposition to the PR author.
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: stbenjam/skillsaw@c325108b6cf94284762948b6a463e75ab5288520 # v0.18.0 |
There was a problem hiding this comment.
[MEDIUM] Three independent skillsaw version pins can silently drift out of sync
The skillsaw version is recorded three separate times with nothing tying them together: this line pins the Action via stbenjam/skillsaw@c325108b6cf94284762948b6a463e75ab5288520 # v0.18.0, Makefile:8 sets SKILLSAW_VERSION := 0.18.0 (consumed by make lint/make lint-fix via uvx skillsaw@$(SKILLSAW_VERSION)), and .skillsaw.yaml:4 has version: "0.18.0". Renovate can bump the Action's SHA pin automatically but has no visibility into the Makefile variable or the YAML version string, so a future Action bump can leave local make lint runs on a different skillsaw release than CI.
Suggestion: Consolidate to one source of truth (e.g., have CI install via uvx skillsaw@$(SKILLSAW_VERSION) reusing the Makefile variable instead of the dedicated Action), or add a Renovate custom regex manager that also bumps SKILLSAW_VERSION in the Makefile and version: in .skillsaw.yaml whenever the Action pin moves.
| ### Opening a PR | ||
|
|
||
| - Stage your changes and fix any lint failures before pushing. | ||
| - Stage your changes and fix any lint failures before pushing (`make lint`, or `make lint-fix` to apply automatic fixes). |
There was a problem hiding this comment.
[MEDIUM] Updated lint guidance omits the separate pre-commit gate CI's test job enforces
This line narrows the generic "fix any lint failures before pushing" wording to point specifically at make lint/make lint-fix (skillsaw). But .github/workflows/lint.yml's pre-existing test job (unchanged by this PR) separately runs pre-commit run --all-files (actionlint, gitleaks, shellcheck, pinact, YAML/JSON checks, etc.), none of which make lint (skillsaw only) touches. A contributor who follows this narrowed instruction, sees make lint pass, and pushes can still fail CI on the unrelated test job's pre-commit checks.
Suggestion: Update this line to reference both checks (e.g., pre-commit run --all-files and make lint), or fold skillsaw into the pre-commit config so make lint and the test job's checks stay in sync.
Adds a skillsaw job to the lint workflow, running the linter in
--strict mode on every PR and push to main. Adds
make lintandmake lint-fixtargets so contributors can run the same checkslocally. Adds .skillsaw.yaml, tuned for this repo:
plugins/gopls-lsp is an internal fullsend harness plugin config
(see harness/code.yaml), not a Claude Code marketplace plugin, so
these rules produced false positives.
20000, above code-implementation and pr-review's current sizes.
Both are dense, production-tuned orchestration procedures; --strict
turns warnings into failures, so the thresholds are set high enough
that neither file triggers a violation at either severity.
This builds on the skill content fixes in the preceding PRs in this
stack, evaluating skillsaw per #188.
Related to #188
Assisted-by: Claude Opus 4.6 noreply@anthropic.com
Signed-off-by: Ralph Bean rbean@redhat.com
Stack created with GitHub Stacks CLI • Give Feedback 💬