-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add maintenance skills — deps-audit, bench-check, test-health, housekeep #565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d52a3a4
b187fe1
a562b52
4fc994d
89aef6b
3e892d1
ce5d811
01b5110
3b0e293
52de495
8be5cec
0691ffc
87d9213
65d9836
eef2c03
19b14e9
5bda6ba
bd0ba1a
7b91e3c
7fcdd93
5462d32
457e6b9
852003d
eea2954
baf6797
9b4869c
8d92c99
9ad37ea
30ab30e
a8631d2
8fd7430
23f2f76
2616c78
cdcc086
ac1e0b5
5434550
316105c
c7115c2
740299f
a762236
ed12127
3d52aaa
912109f
a94ddf3
ad80d18
6d4de9f
34c50b9
8b636ae
8c467d3
ec1fd56
c9ac370
a2e8be0
7a1c86a
55c5a22
0dc2605
aa3e1f4
75350c7
7aab540
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,14 +22,10 @@ Audit the project's dependency tree for security vulnerabilities, outdated packa | |
| 5. **If `AUTO_FIX` is set:** Save the original manifests now, before any npm commands run, so pre-existing unstaged changes are preserved: | ||
| ```bash | ||
| git stash push -m "deps-audit-backup" -- package.json package-lock.json | ||
| STASH_CREATED=$? | ||
| ``` | ||
| Track `STASH_CREATED` — when `0`, a stash entry was actually created; when `1`, the files had no changes so nothing was stashed. | ||
| If `STASH_CREATED` is `0`, immediately capture the stash ref for later use: | ||
| ```bash | ||
| STASH_REF=$(git stash list --format='%gd %s' | grep 'deps-audit-backup' | head -1 | awk '{print $1}') | ||
| ``` | ||
| Use `$STASH_REF` (not `stash@{0}`) in all later stash drop/pop commands to avoid targeting the wrong entry if other stashes are pushed in the interim. | ||
| `STASH_REF` is non-empty if and only if a stash entry was actually created. Do **not** use `$?` — modern git (2.16+) returns 0 even when nothing was stashed. | ||
| Use `[ -n "$STASH_REF" ]` (stash created) / `[ -z "$STASH_REF" ]` (nothing stashed) for all branching. Use `$STASH_REF` (not `stash@{0}`) in all later stash drop/pop commands to avoid targeting the wrong entry. | ||
|
|
||
| ## Phase 1 — Security Vulnerabilities | ||
|
|
||
|
|
@@ -165,13 +161,24 @@ If `AUTO_FIX` was set: | |
| Summarize all changes made: | ||
| 1. List each package updated/fixed | ||
| 2. Run `npm test` to verify nothing broke | ||
| 3. If tests pass and `STASH_CREATED` is `0`: drop the saved state (`git stash drop $STASH_REF`) — the npm changes are good, no rollback needed | ||
| If tests pass and `STASH_CREATED` is `1`: no action needed — the npm changes are good and no stash entry exists to clean up | ||
| 4. If tests fail and `STASH_CREATED` is `0`: | ||
| 3. If tests pass and `STASH_REF` is non-empty: pop and merge the saved state (`git stash pop $STASH_REF`) — this restores any pre-existing uncommitted changes alongside the npm fix results. | ||
| - If the pop applies cleanly: | ||
| a. Run `npm install` to re-sync `node_modules/` with the merged manifest. | ||
| b. Re-run `npm test` to confirm nothing broke with the merged dependency state. | ||
| c. If tests still pass: confirm the project is consistent. | ||
| d. If tests now fail: warn the user — the pre-existing manifest changes conflict with the audit fixes. | ||
|
Comment on lines
+165
to
+169
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the stash pops cleanly and
Without a recovery path, the user is left with a mixed, broken state and must manually reconstruct which changes to keep. Add explicit recovery guidance: d. If tests now fail: warn the user — the pre-existing manifest changes conflict with the audit fixes.
Recovery options:
- To undo **all** manifest changes (both audit fixes and pre-existing): `git checkout -- package.json package-lock.json && npm ci`
- To keep only the audit fixes and discard pre-existing changes: manually edit `package.json`/`package-lock.json` to remove the pre-existing delta, then `npm ci`
- To keep only the pre-existing changes and discard the audit fixes: re-run `/deps-audit` without `--fix`
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — step 169d now lists three explicit recovery options: undo all changes (git checkout + npm ci), keep only audit fixes (manual edit + npm ci), or keep only pre-existing changes (re-run without --fix). |
||
| Recovery options: | ||
| - To undo **all** manifest changes (both audit fixes and pre-existing): `git checkout -- package.json package-lock.json && npm ci` | ||
| - To keep only the audit fixes and discard pre-existing changes: manually edit `package.json`/`package-lock.json` to remove the pre-existing delta, then `npm ci` | ||
| - To keep only the pre-existing changes and discard the audit fixes: re-run `/deps-audit` without `--fix` | ||
| - If the pop causes conflicts in `package.json`/`package-lock.json`: warn the user, leave conflict markers for manual resolution, and instruct: "After you resolve the conflicts, run `npm install` to re-sync `node_modules/` with the resolved lock file before committing." | ||
| - For conflicts in other files, resolve them by keeping both the npm fixes and the pre-existing changes. | ||
| If tests pass and `STASH_REF` is empty: no action needed — the npm changes are good and no stash entry exists to clean up | ||
| 4. If tests fail and `STASH_REF` is non-empty: | ||
| - Restore the saved manifests: `git stash pop $STASH_REF` | ||
| - Restore `node_modules/` to match the reverted lock file: `npm ci` | ||
| - Report what failed | ||
| 5. If tests fail and `STASH_CREATED` is `1`: | ||
| 5. If tests fail and `STASH_REF` is empty: | ||
| - Discard manifest changes: `git checkout -- package.json package-lock.json` | ||
| - Restore `node_modules/` to match the reverted lock file: `npm ci` | ||
| - Report what failed | ||
|
|
@@ -181,6 +188,6 @@ Summarize all changes made: | |
| - **Never run `npm audit fix --force`** — breaking changes need human review | ||
| - **Never remove a dependency** without asking the user, even if it appears unused — flag it in the report instead | ||
| - **Always run tests** after any auto-fix changes | ||
| - **If `--fix` causes test failures**, restore manifests from the saved state (`git stash pop $STASH_REF` if `STASH_CREATED=0`, or `git checkout` if stash was a no-op) then run `npm ci` to resync `node_modules/`, and report the failure | ||
| - **If `--fix` causes test failures**, restore manifests from the saved state (`git stash pop $STASH_REF` if `STASH_REF` is non-empty, or `git checkout` if nothing was stashed) then run `npm ci` to resync `node_modules/`, and report the failure | ||
| - Treat `optionalDependencies` separately — they're expected to fail on some platforms | ||
| - The report goes in `generated/deps-audit/` — create the directory if it doesn't exist | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pre-condition check in Phase 4 correctly says "Stop here and skip to Phase 6 to write the report with verdict
ABORTED." However, Phase 6 only defines two report branches:SAVE_ONLYor first-run → shortened "BASELINE SAVED" reportAn agent following an ABORTED verdict would fall through to the "Otherwise" branch (there was a prior baseline, no SAVE_ONLY) and produce a report using the "PASSED / FAILED" template, complete with empty "Comparison vs Baseline" and "Regressions" sections. This is misleading — no comparison was performed because all benchmarks failed.
Similarly, Phase 7 line 269's summary only lists
PASSED (0 regressions) | FAILED (N regressions) | BASELINE SAVED, with noABORTEDoption.Add a dedicated ABORTED branch to Phase 6, for example:
And update Phase 7's summary line to include
| ABORTED (all suites failed).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed — added a dedicated ABORTED branch to Phase 6 with a minimal report template (version, git ref, verdict, raw error records). Also added ABORTED to the Phase 7 summary line.